feat: Phase 1 of typed on_error routing (#227) - #229
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #229 +/- ##
=======================================
Coverage ? 89.21%
=======================================
Files ? 74
Lines ? 13036
Branches ? 0
=======================================
Hits ? 11630
Misses ? 1406
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Attempted to rebase Conflict summary: v0.1.18's set-step PR initializes output context as Built the dogfood from |
|
Tried rebasing this onto v0.1.18 base for polyphony dogfood integration, and hit a design conflict in The clash: PR #229 commit The Interim: Polyphony dogfood is staying pinned at v0.1.17 base ( |
1 similar comment
|
Tried rebasing this onto v0.1.18 base for polyphony dogfood integration, and hit a design conflict in The clash: PR #229 commit The Interim: Polyphony dogfood is staying pinned at v0.1.17 base ( |
jrob5756
left a comment
There was a problem hiding this comment.
Took a pass on the overall approach. I like the direction: success/error route buckets fit conductor's existing model cleanly, the opt-in synthesis keeps legacy exit_code routing untouched, and the phasing discipline (deferring subworkflow/group propagation as hard validation errors rather than silent no-ops) is the right call. Two pieces of feedback before this locks in the authoring contract:
1. The script transport is good — let's just document the asymmetry.
To be clear for others reading: CONDUCTOR_ERROR_OUT is a file path, not JSON-in-an-env-var, so escaping/size concerns don't apply — this is the $GITHUB_OUTPUT pattern and I think it's the right choice. The reason it's a file (and not a stdout discriminator like agents use) is that type: script already parses stdout as the output: contract, so errors need an out-of-band channel. That justification is sound, but it means the raise mechanism differs by node type (agents: in-band stdout conductor_error: true; scripts: out-of-band file). Please call that split out explicitly in docs/workflow-syntax.md so authors aren't surprised.
The parse-failure handling is solid — malformed JSON and malformed envelopes both downgrade to internal.schema_violation rather than dropping the signal, and reading only after communicate() returns means there's no write/read race on the non-atomic write. 👍 One thing to confirm in the correctness pass: malformed→schema_violation fires regardless of opt-in, whereas synthesized internal.script_error only fires when the node opted in. I think that's intended, but let's make sure a node that writes garbage with no on_error route halting on exit 3 (vs. legacy exit-code routing) is the behavior we want.
2. Trim the bundled multi-language helpers.
The contract itself is great precisely because it's language-neutral — "write a 3-field JSON file" — and you've kept it fully optional (the raw example uses no helper, plain echo > "$CONDUCTOR_ERROR_OUT" works). That part I'm fully on board with.
What I'd push back on is shipping conductor-error.sh, conductor-error.mjs, Conductor.Error.psm1, and ConductorError.cs inside the Python wheel. A pwsh/.NET/node user can't naturally consume a file buried in site-packages/conductor/helpers/error/ — there's no Import-Module/NuGet/npm story and the path is venv- and version-specific, so in practice these are copy-paste snippets dressed up as libraries. It's also five implementations of a trivial contract to keep in sync, and it undercuts the "it's just a JSON file" message.
Proposal: keep the Python helper (conductor is Python; inline python -c and script nodes make a real importable module genuinely useful) and demote the other four to documented copy-paste snippets in docs/ rather than shipping source in the wheel. Same ergonomic win where it's natural, without the maintenance/consumption awkwardness.
Neither of these is a structural objection — the file side-channel and the fail-safe parse handling are the right foundation. Mostly packaging + docs. Will follow up with correctness notes on the router/engine wiring.
| try: | ||
| with open(path, encoding="utf-8") as f: | ||
| raw = f.read() | ||
| except OSError: |
There was a problem hiding this comment.
Silent envelope drop on read failure when exit_code == 0.
Swallowing OSError → None here is fine when the script never wrote a typed error (the file legitimately doesn't exist / is empty), but it conflates that benign case with a real read failure on a path the engine itself created. Consider this path:
- Script writes a valid envelope to
$CONDUCTOR_ERROR_OUT(raising a typed error) and exits 0, relying on the file as the failure channel. - The read here raises
OSError(permissions, transient FS error, racy temp cleanup) →_read_error_envelopereturnsNone. - Back at
execute()(~line 191), synthesis is gated onexit_code != 0 and _node_uses_error_routing(agent), so nothing fires. ScriptOutput.errorstaysNone, the engine sees a success, and the node is routed down the success path — no envelope, no log, no event.
This is the exact silent-failure class typed error routing exists to prevent. An unexpected OSError on an engine-owned path should not be conflated with "script chose not to raise" — at minimum emit a diagnostic, and ideally synthesize a read-failure variant of internal.script_error / internal.schema_violation so the typed-failure signal is preserved regardless of exit code.
Side note: open(..., encoding="utf-8") defers decoding to f.read(), so a UnicodeDecodeError is not caught by except OSError here — that crashes rather than silently dropping, which is arguably the lesser evil but inconsistent.
| _LEAF_TYPES_THAT_RAISE: frozenset[str | None] = frozenset({"agent", "script", None}) | ||
|
|
||
|
|
||
| def _validate_on_error_routes(agent: Any) -> list[str]: |
There was a problem hiding this comment.
Validator gap + two related correctness items it would be natural to address together here.
a) Validator gap — node with only on_error routes crashes generically on the happy path. This validator accepts a node with only error routes and no success fallback. On a successful run, Router._evaluate_success (router.py:103-121) skips every route whose on_error is not None, then raises a plain ValueError("No matching route found..."). That bubbles up as a generic exception → CLI exit 1 → no errors.jsonl, no typed halt panel, despite the workflow being clearly authored for the typed-error world. Verified locally. Recommend rejecting at validation here with a teaching message like "Agent 'X' has on_error routes but no success route. Add a route without on_error to handle the happy path."
b) Related — _evaluate_error silently ignores output_for_route (engine/router.py:138-176, called from _handle_leaf_error at engine/workflow.py:4452). The plumbing accepts output_for_route but _evaluate_error only flattens error into the eval context and drops current_output. Any error-route when: clause referencing the failing node's output (e.g. attempt_count > 3) will silently see the previous agent's output. Either merge output_for_route into the error eval context (more useful) or drop the plumbing (more honest), but the current silent inconsistency is a latent footgun.
c) Related — agent schema-violation auto-upgrades existing workflows from exit 1 → exit 3 (executor/agent.py:282-302). Pre-PR: schema violation → ValidationError → exit 1. Post-PR: synthesizes internal.schema_violation envelope → unhandled → exit 3 + errors.jsonl. This is asymmetric with the script executor's _node_uses_error_routing opt-in (script.py:53-67), which preserves legacy behavior for unopted scripts. Either gate the agent upgrade on the same opt-in (so the asymmetry is principled, not accidental), or document the exit-code change in release notes.
| # path treats it like any other sub-workflow failure. Phase 2 | ||
| # will introduce envelope propagation with parent frames. | ||
| raise ExecutionError( | ||
| f"sub-workflow '{agent.name}' halted on unhandled error envelope " |
There was a problem hiding this comment.
Missing integration tests guard documented Phase-1 invariants.
Four integration boundaries have documented contracts but no tests; a regression on any of them would silently change semantics in ways unit tests won't catch.
-
Sub-workflow downgrade (this site and the mirror at line 1294). The module docstring at
tests/test_engine/test_error_routing.py:15-16claims "Phase 1 does NOT propagate envelopes across sub-workflow boundaries" — but no test exercises either branch. Add a workflow with atype: workflowstep whose child halts on an unhandled envelope; assert the parent seesExecutionError, notUnhandledWorkflowError, and that the parent's ownon_errorroutes do NOT fire. -
Parallel / for_each envelope plumbing.
ParallelAgentError.envelope/ForEachError.envelopeare populated in production atworkflow.py:3853, 3909, 4310, 4343from the_envelopeattribute attached at:3756and:4213. The only tests of these dataclasses (tests/test_engine/test_parallel.py:504-536) pre-date the PR and construct them in isolation. Add a test that runs a real parallel/for_each group with a typed-envelope child and asserts the envelope is preserved on the group's error. -
step.errorinput ref without producer'sraises. The validator (config/validator.py:1351walking output/outputs refs) doesn't cross-check a leaf-nodeproducer.errorreference againstproducer.raises. A consumer that declaresinputs: ["fetch.error"]against afetchnode with noraises:passes validation and onlyKeyErrors at runtime via_add_agent_inputinengine/context.py. -
resumeexit-code-3 path.cli/app.py:930-933mirrors therunexit-code handling forUnhandledWorkflowError, but onlyrunis tested (test_cli/test_run.py:988-1026). Diverging exit codes betweenrunandresumewould break CI scripts that key on exit 3 for typed halts.
| self._errors_jsonl_path = errors_path | ||
| # Attach the path to the exception itself so the CLI handler | ||
| # can render it without needing a reference to the engine. | ||
| e.errors_jsonl_path = errors_path # type: ignore[attr-defined] |
There was a problem hiding this comment.
setattr side-channels on exceptions defeat the type system.
Two patterns here that are mechanically the same problem:
e.errors_jsonl_path = errors_path # type: ignore[attr-defined]onUnhandledWorkflowError(this site), read back viagetattr(error, "errors_jsonl_path", None)incli/app.py:190.exc._envelope = normalized # type: ignore[attr-defined]onExecutionErroratworkflow.py:3758and:4214, read back viagetattr(result, "_envelope", None)atworkflow.py:3853, 3909, 4310, 4343.
Both are undeclared attributes that the type checker can't see. A rename or typo in any of the 4 read sites silently degrades to "no envelope" / "no log path" with no test failure or type error.
Mechanical fix: add typed constructor params:
UnhandledWorkflowError(..., errors_jsonl_path: Path | None = None)ExecutionError(..., envelope: ErrorEnvelope | None = None)(importable underTYPE_CHECKINGto avoid the cycle, sinceerrors.pyis a leaf module)
This removes both # type: ignore[attr-defined] writes and all six defensive getattr(..., None) reads, and makes the carriers checkable end-to-end.
Related (same theme but lower priority): the executor output fields ScriptOutput.error and AgentOutput.error are typed dict[str, Any] instead of ErrorEnvelope | None, which is what forces the cast("dict[str, Any]", envelope) calls in executor/script.py:242,244,251 and executor/agent.py:279,301. Tightening those two fields under TYPE_CHECKING would eliminate every cast introduced by this PR.
| f.write(json.dumps(record, default=str)) | ||
| f.write("\n") | ||
| except OSError as e: | ||
| logger.warning("Failed to write errors.jsonl at %s: %s", path, e) |
There was a problem hiding this comment.
logger.warning here is effectively invisible — route through the event system.
Per project convention, conductor never calls logging.basicConfig/addHandler; every module just does logging.getLogger(__name__). A logger.warning with no configured handler falls through to Python's lastResort handler, which writes to raw stderr at WARNING level and bypasses the Rich console, the event stream, and the --verbose flag entirely. In --web-bg it only lands in the captured .bg.stderr.log, not the dashboard or the .events.jsonl.
This is the worst possible site for that pattern: _write_errors_jsonl is the forensic artifact that pairs with .events.jsonl for post-mortem of an unhandled halt. When the write fails, the operator gets errors_jsonl_path = None rendered as Halt log: None in the CLI panel and no indication that writing actually failed (vs. simply not being attempted).
Fix: emit a WorkflowEvent (e.g. errors_jsonl_write_failed), or attach the failure reason to the existing workflow_failed event as a sibling field — either way it then renders through the same channel as the rest of the engine's diagnostics. The same concern applies to the other logger.warning sites in this file (:2026, :3334, :3376), but this one is on the error-routing critical path.
Implement a narrow Phase 1 with a language-neutral script error envelope, deterministic success/error route buckets, explicit checkpoint precedence, validation, docs, and examples while preserving legacy nonzero-exit routing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 46859721-1d81-4ddd-93a0-995c2198596e
f13791b to
4aec332
Compare
|
Rebased and replaced the obsolete implementation with commit 4aec332. Reviewer feedback is addressed: the transport is explicitly language-neutral, no helper libraries ship in the wheel, ordinary nonzero exits remain compatible, malformed transport fails closed, and scalar/None output plus legacy explicit-input shorthand behavior is preserved. The final high-confidence review found no remaining issues. |
Rebased onto current
main(148b80e) and narrowed to the smallest coherent Phase 1 from RFC #227: typed script failures with deterministic error routing.Phase 1 scope
CONDUCTOR_ERROR_OUTfile transport fortype: script.{kind, message, details}.on_errorselectors: exact kind, list of kinds, ortruecatch-all.when:behavior remain unchanged within each bucket.<step>.outputand<step>.error.raises:as documentation/load-time validation only.internal.,provider.,subworkflow.,retry.) cannot be published or declared by scripts.Compatibility and deliberate exclusions
exit_coderoutes are unchanged.internal.script_error_transportrather than silently succeeding.errors.jsonl, new CLI exit code, or bundled language helpers in Phase 1.on_error/raisesare rejected outside top-level script steps so unsupported handlers cannot silently never run.Resolved precedence
output:schema, allowing handlers to inspect partial stdout/stderr.terminateremains terminal, unroutable, and non-checkpointed.SubworkflowTerminatedErrorretains existing parent behavior; workflow steps cannot useon_errorin Phase 1.UnhandledNodeErrorbefore committing that execution; the checkpoint points at the script for replay.raises:decisionraises:remains useful as optional author-facing documentation and lint. It does not opt into synthesis, rewrite undeclared runtime kinds, or constrainon_error: true; catch-all handlers receive the original runtime kind.Validation
tychecks pass.examples/error-routing.yamlvalidates and all success/exact/catch-all paths run successfully.origin/main.The branch history was intentionally replaced with one rebased commit so the PR no longer carries the obsolete broad implementation.