Return a typed PipelineResult with an execution trace (#332) - #442
Merged
Conversation
execute_pipeline returned a bare {step_id: value} dict -- except when the
pipeline declared `outputs:`, when it returned {"steps":..., "outputs":...}
instead. Two shapes from one method, distinguishable only by inspecting
keys, and a pipeline with a step called `outputs` collided with the
second outright.
Almost everything else the run knew was computed and then discarded at
the return statement. Task has carried status, started_at, completed_at,
retry_count, dependencies and the error all along; Pipeline can already
report dependency levels; execution_id and start_time were sitting in
the executor's context. PipelineResult keeps them.
It is a Mapping, so result["step_id"] returns the step's raw value
exactly as before and the several thousand existing assertions that
index results directly are unaffected. The trace arrives as attributes:
status, success, outputs, steps, execution_order, execution_levels,
timing, and failed/skipped/retried views. Each StepResult carries its
action, status, success, value, structured error and error_type, the
tool or model and provider that ran it, start/end/duration, retries and
dependencies.
status and success answer different questions, and conflating them was a
real defect. status records whether the task finished; success whether
it worked. A tool returning {"success": False} without raising *does*
finish, so selecting failures on status alone reported a failing
pipeline as successful -- caught here by the golden exit-code test going
green when it should have been red. StepResult.success accounts for
both, and the CLI's exit code consults it.
The CLI now serialises to_dict() rather than json.dumps(result,
default=str). That `default=str` silently stringified anything
unserialisable, so the JSON was neither stable nor round-trippable and
CLI/API equivalence could not be asserted at all. normalized() drops the
execution id and wall-clock times -- the only fields that legitimately
differ between runs -- and the golden test now compares whole normalised
documents from both surfaces instead of one nested value.
Three test expectations changed with the contract, none weakened:
- the golden and CLI tests read the typed document rather than the old
flat map;
- my own assertion that a failed step has status "failed" contradicted
the status/success distinction this PR introduces, and was wrong;
- a `len(stdout.splitlines()) < 30` ceiling stood in for "no log noise"
while the payload was compact. The typed document is legitimately
longer, so that bound measured the document rather than the noise.
Round-tripping the whole of stdout already proves the point, and an
explicit check for log markers replaces the magic number.
Blocking suite 476 -> 490 passed, 13 skipped, 0 failed, 0 xfailed.
Collection 3271 -> 3285. ruff, uv lock --check, git diff --check and the
generated action docs all clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
CI 9/9 green, and the legacy backlog improved. Via the annotation route (
Two legacy tests recovered, errors unchanged, nothing regressed. The two recoveries are plausibly tests that asserted on a failing step and previously got the error handler's retry policy where the reason belonged — but I have not attributed them individually, and I am not going to claim it without the per-test diff. |
Stacked on #442, which the routing tests use to assert skipped steps through the typed result. `on_false` and `on_success` did not exist at all. `condition:` was already evaluated and already skipped a step, so what was missing was purely the routing: where to go once the answer is known. Routing jumps forward and marks the steps in between as skipped, reusing `_skip_tasks_between` -- the machinery `goto` already had -- rather than adding a second one beside it. Skipping is not failing: a pipeline that routed around a step still reports success, and PipelineResult separates skipped_steps from failed_steps accordingly. The awkward part is `on_failure`, which already meant a failure *policy* (fail / continue / skip / retry) and now also names a step to jump to. Both readings are supported and disambiguated by value: a reserved policy word keeps its policy meaning, anything else is a step id, and when it names a step the run continues there instead of aborting. That is only safe because the genuinely ambiguous case is refused rather than guessed. A pipeline with a step whose id *is* a policy word -- a step called `retry` -- fails to compile, because `on_failure: retry` could not be told apart from selecting the policy. core/routing.py holds that single definition; the compiler and the executor both consume it. Routing targets are validated at compile time: a jump to a step that does not exist, or a step routing to itself, is exit 2 naming the target. That check went into validation/dependency_validator.py, which is the validator actually on the compile path -- my first attempt put it in compiler/schema_validator.py, whose validate_dependencies() nothing calls, so the tests failed while the code looked right. Only the schema widening stayed there: `on_failure` was constrained to an enum of the four policies, which would have rejected every routing target outright. The runtime decision uses StepResult.success, not TaskStatus. A tool returning {"success": False} without raising leaves its task COMPLETED, so routing on status alone sent a failing step down the on_success path -- the same distinction #442 had to draw, reused here rather than restated a third time. The contract is updated: ADR 0001 gains a control-flow section, and docs/actions.md is generated with the routing table and the policy list so the published vocabulary cannot drift from FAILURE_POLICIES. Blocking suite 490 -> 506 passed, 13 skipped, 0 failed, 0 xfailed. Collection 3285 -> 3301. Examples validating unchanged at 3 of 111. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Implement control-flow routing: on_false, on_success, on_failure (#333)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements #332's result contract. The
result.qc()half of that issue is not here — see the end.Correcting something I told you
I said declared outputs were "parsed, never resolved". That was wrong —
_extract_outputsrenders them, and has all along.The actual defect is worse and less visible: declared outputs changed the shape of the return value.
Two shapes from one method, distinguishable only by inspecting keys — and a pipeline with a step called
outputscollided with the second outright.Mostly recovering discarded state, not new machinery
Task.status/.errorTask.started_at/.completed_atTask.retry_countPipeline.get_execution_levels()All of it was computed, then dropped at
return final_result.PipelineResultis aMapping, soresult["step_id"]returns the raw value exactly as before — the several thousand assertions that index results directly are untouched. The trace arrives as attributes.statusandsuccessare different questions — and conflating them was a live bugstatusrecords whether the task finished;successwhether it worked. A tool returning{"success": False}without raising does finish.My first implementation selected failures on
status == FAILED, and the golden exit-code test went green when it should have been red — a pipeline with a failing step exiting 0. That is precisely the failure mode this project keeps having to re-fix, and it reappeared inside the change meant to make failures legible.StepResult.successnow accounts for both, and the CLI's exit code consults it.Serialisation
The CLI emitted
json.dumps(results, default=str). Thatdefault=silently stringified anything unserialisable, so the output was neither stable nor round-trippable — CLI/API equivalence was impossible to assert, not merely unasserted.It now serialises
to_dict(), which needs no coercion.normalized()drops the execution id and wall-clock times — the only fields that legitimately differ between runs — and the golden test compares whole normalised documents from both surfaces, as you asked, rather than one nested value.Test expectations that changed, and why none is a weakening
status == "failed"contradicted the status/success distinction this PR introduces. The code was right; my expectation was wrong.len(stdout.splitlines()) < 30ceiling stood in for "no log noise" while the payload was compact. The typed document is legitimately longer, so that bound had started measuring the document rather than the noise. Round-tripping the whole of stdout already proves there is no interleaved logging; an explicit check forDEBUG/INFO/WARNING/Tracebackreplaces the magic number.Numbers
uv lock --check/git diff --check/ action-docs driftNot in this PR
result.qc()— #332 also asks for an orchestrator model to grade output quality. That is a quality-evaluation feature, not a result contract, and it cannot sit inside a deterministic acceptance contract; your own quality section says an LLM judge "may supplement — but never replace" deterministic scoring. It wants its own issue.Fallback events.
retriesis a count, and skips are recorded, but nothing in the runtime currently records why a fallback happened — there is no emission point to read. Adding one is a change to the execution path rather than to the result type, so it belongs with whatever introduces fallback policy.provideris read from the step envelope and is currentlyNonefor every model step, because the envelope recordsmodel_usedand no provider. The field is in the contract and the plumbing is one line at the point the envelope is built — but that line belongs with the model-selection work, not smuggled in here.