fix(providers): recover from schema-shape failures instead of failing the run (#343) - #345
Merged
Merged
Conversation
… the run (#343) An agent returning syntactically valid JSON with a wrong-typed field killed the workflow immediately with zero recovery attempts, even though `max_parse_recovery_attempts` exists for exactly this class of contract violation. Schema validation ran a layer above the providers, in `executor/agent.py`, after the provider had already returned. For Copilot that is also after `finally: await session.disconnect()`, so the loop that could have re-prompted no longer had a session to re-prompt with. Claude had the same gap: `_execute_with_parse_recovery` returned as soon as any content could be extracted, without checking its shape. Only Hermes validated in-loop. Copilot and Claude now validate inside the recovery loop, matching Hermes: - Copilot validates against `output_schema` and adds `ValidationError` to the caught exceptions. - Claude classifies each response via `_evaluate_structured_response`, validating the emit_output and JSON-fallback paths while leaving the MCP tool-use path alone, since that returns to the agentic loop rather than being a final answer. A tool_use-origin failure is replayed as text, because a bare tool_use block without a matching tool_result would violate the Anthropic message contract. - Both send a schema-specific correction prompt distinct from the syntax one. On budget exhaustion the original `ValidationError` is re-raised, so `Output field 'decision' has wrong type: expected string, got dict` survives rather than collapsing into a generic parse error. Syntax failures still raise `ProviderError`. This also fixes Hermes, which discarded that detail. Hermes additionally now honors `retry.max_parse_recovery_attempts`; it had been hardcoded to 3, ignoring the YAML value the other providers respect. Note that `parse_json_output` wraps syntax errors in `ValidationError` too, so the two failure kinds cannot be told apart by exception type — Hermes splits them by which call failed. Also adds: - `providers/_output_shape.py::unwrap_scalar_wrappers`, a conservative normalization that resolves the common wrapper shape without a paid round-trip. It fires only for scalar targets with one unambiguous candidate of the expected type, logs every unwrap, and lives outside `validate_output` so `set` and `script` step output stays strictly validated. - An `agent_parse_recovery` event across all three providers, surfaced in the console, the structured event log, and the dashboard activity stream. Recovery was previously visible only under verbose logging. - The offending value, truncated, in output validation errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses findings from a multi-agent review of the initial fix.
Two of these were live defects, one of them a regression introduced by the
first commit:
- Non-object JSON (a bare `42`, `null`, or an array) reached `validate_output`,
which does `field_name not in content` and raised an uncaught `TypeError`.
On Copilot this was new: it surfaced as a *retryable* "check that copilot CLI
is installed and authenticated" and burned the whole agent retry budget with
zero parse-recovery attempts. On Claude it predated this work but surfaced as
"check API key, model name, and request parameters". Both now raise a
`ValidationError` naming the real problem, so the recovery loop re-prompts.
- Copilot's interrupt/partial path could return a non-dict as
`AgentOutput.content`, which is declared `dict[str, Any]`. That path has no
recovery loop, so it was unconditionally fatal downstream.
The unwrap heuristic was too eager and its docstring overclaimed. It promised
"exactly one unambiguous candidate" but was first-match-wins including a bare
sole-key fallback, so it would resolve `{"answer": {"error": "I could not
complete the task"}}` into an answer, and flip `{"approved": {"not_approved":
false}}` to `approved=False`. It now fires only when exactly one candidate
under the field's own name or a generic `value`/`result` key has the expected
type; everything else is re-prompted rather than guessed at. The warning also
names the keys it discards.
Hermes was sending the *syntax* correction prompt for schema failures, telling
a model its valid JSON "could not be parsed as valid JSON" — which invites it
to re-send the same payload and burn the budget. It now branches like the other
two, restoring the parity rule this change set added to AGENTS.md.
Also:
- Copilot and Hermes log the expected fields and the response snippet before
re-raising a bare `ValidationError` at exhaustion; that context used to ride
on the `ProviderError` they no longer raise. Claude already did this.
- `_describe_value` renders containers by shape (`object with keys [...]`)
instead of dumping contents, since `validate_output` also runs on `set` and
`script` output that may carry secrets.
- `emit_parse_recovery_event` moves payload rendering inside its guard, so a
non-str error can't break agent execution against the docstring's promise,
and logs at warning rather than debug.
- Hermes resolves the retry policy via typed attribute access instead of
chained `getattr`, so a typo fails loudly.
Test coverage for the parity matrix: zero recovery budget on all three
providers (0 is a legal "fail fast" value, not "unset"), mixed schema/syntax
exhaustion ordering, syntax-path prompt wording and event labels, exhaustion
message content, raising event subscribers, and `_describe_value` redaction.
Hermes' documented divergence on non-object JSON — `parse_json_output` wraps
non-dicts as `{"result": ...}` before normalization sees them — is now pinned
by a test rather than left implicit.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…acy (#343) Follow-up to the code review. One real bug: `_find_scalar_candidate` built its candidate list as `(field_name, "value", "result")` without deduping, so a field literally named `value` or `result` occupied two slots and was rejected as ambiguous against itself. `result` is idiomatic here — `parse_json_output` wraps every non-object response as `{"result": ...}` — so the shape most likely to need unwrapping was the one that silently never did. Documentation accuracy: - The CHANGELOG claimed the non-object failure surfaced on Copilot as a retryable "check that copilot CLI is installed and authenticated". That was never released: Copilot never called `validate_output` on `main`, and the executor backstop rebuilt non-dicts into `{"result": ...}` first. The auth error was a regression introduced and fixed inside this PR, so it does not belong in user-facing notes. The Claude half of the claim does reproduce and is kept. - `_describe_value`'s summary claimed it never echoes contents, but scalars are still rendered via `repr`. Only containers are reduced to shape. - `_execute_with_parse_recovery`'s summary described only the pre-change behavior ("returns text instead of using the tool ... malformed JSON"), which is exactly the case the fix widened past. - `docs/workflow-syntax.md` asserted non-object handling is uniform across providers; a test on this branch asserts the opposite for Hermes. - The `TypeError` rationale was over-broad: only numbers, booleans, and null raise it. Strings and arrays produce a misleading "missing required field" instead, which is bad for a different reason. Dead code: - Copilot printed parse-recovery attempts twice under `--verbose`: once via `_log_parse_recovery` and again via the new `agent_parse_recovery` event. Claude and Hermes print once. Dropped the bespoke console write and moved agent attribution into the shared renderer, removing the now-unused method and its tests. - Dropped a redundant `event_callback is not None` guard in Copilot; the helper is already total over `None`, and the other two providers call it unguarded. - Narrowed Hermes' recovery-call `except` from `(json.JSONDecodeError, ValueError, ValidationError)` to `ValueError`. The block no longer parses anything, so the JSON and Conductor-internal arms were unreachable; `JSONDecodeError` is a `ValueError` subclass anyway. - Removed an unreachable non-dict guard and a stale "both providers" reference. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Restores the reviewed wording that the rebase conflict resolution dropped: the tightened unwrap contract, the non-object JSON entry with the Copilot claim corrected (that error was an intra-PR regression, never released), and shape-based rendering of offending values. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jrob5756
force-pushed
the
fix/343-schema-shape-parse-recovery
branch
from
July 29, 2026 20:27
94ad762 to
77ea5ac
Compare
…e plumbing (#343) Final review pass. No observable behavior change — same error types, event payloads, log content, and prompt wording. Copilot and Hermes had byte-identical recovery-prompt generators. Extracted to `providers/_recovery_prompt.py`, which makes the parity contract AGENTS.md declares for this wording mechanical rather than aspirational: a tweak in one provider can no longer silently diverge from the other. Claude deliberately stays separate — its instruction omits the schema and response echoes and steers toward the `emit_output` tool, so folding it in would need flags to suppress two of three sections. That asymmetry is now documented so it does not read as an oversight. Claude's `_evaluate_structured_response` returns a `_StructuredEvaluation` NamedTuple that also carries the failure description, computed where the failure kind is already known. That removes both copies of the "branch on outcome, then set initial_text/failure_reason" block, along with `last_schema_error` and the per-iteration variable shuffle. `_find_scalar_candidates` returns a list instead of a sentinel: length is unambiguous, so `_NoCandidate` and its `_NO_CANDIDATE` singleton disappear and the return type stops being `Any`. The two-function split in `_output_shape` stays — `unwrap_scalar_wrappers` is independently meaningful, separately tested, and named as its own concept in AGENTS.md. Smaller: Hermes' `_parse_and_validate` returns the `ValidationError | None` directly instead of a bool, dropping an isinstance-narrowing dance; Copilot tracks one failure variable instead of two; a redundant `str()` on an already typed parameter and a comment restating its own docstring are gone. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Fixes #343.
An agent returning syntactically valid JSON with a wrong-typed field killed the workflow immediately with zero recovery attempts, even though
max_parse_recovery_attemptsexists for exactly this class of contract violation.Verified against the real failure: on
mainthe reported payload fails withOutput field 'decision' has wrong type: expected string, got dict— the issue's error verbatim. On this branch it succeeds, driven through the actualWorkflowEnginerather than the provider in isolation.Why the fix has to live in the provider
Schema validation ran a layer above the providers, in
executor/agent.py:318, after the provider had already returned. For Copilot that is also afterfinally: await session.disconnect()— so by the time the error surfaced, the session that could have been re-prompted was already gone. The executor layer could not recover even in principle.Claude had the same gap
Contrary to the table in the issue,
claudewas not failing fast by design. Itsexcept ValidationError: raiseatclaude.py:1271is scoped to the network-retry loop and means "don't burn API retries on a deterministic error" — sensible, and a separate concern. But its actual recovery loop returned the moment_extract_structured_outputor_extract_json_fallbackyielded anything. It never checked shape either.Changes
output_schema. (The snippet proposed in the issue passesschema_for_prompt, a prompt-facing dict of type/description strings that wouldAttributeErroronfield_def.type.)_evaluate_structured_responseclassifies each response. It validates the emit_output and JSON-fallback paths but deliberately not the MCP tool-use path, which returns to the agentic loop rather than being a final answer. A tool_use-origin failure is replayed as text, because a baretool_useblock without a matchingtool_resultviolates the Anthropic message contract.retry.max_parse_recovery_attempts, which it had been ignoring in favour of a hardcoded 3.ValidationError, so the field name and expected type survive. Syntax failures still raiseProviderError. This is deliberately better than the hermes behaviour the issue asked us to copy — hermes discarded that detail.42,null, an array) is re-prompted as a shape failure instead of reachingvalidate_outputand raising an uncaughtTypeError.normalize_agent_outputconservatively unwraps wrapper-shaped scalars: only when the schema declares a scalar and exactly one candidate under the field's own name or a genericvalue/resultkey has the expected type. Ambiguity and any other key shape are re-prompted rather than guessed at. Kept out ofvalidate_output, which also validatessetandscriptoutput where silent reshaping would be a surprise.agent_parse_recoveryevent through provider → console → dashboard. Recovery was previously invisible outside verbose mode — the issue notes "no entry in the log shows a recovery attempt".Review notes
This went through six review passes; several findings were substantive and are worth calling out for reviewers.
Two live defects were found and fixed after the initial implementation. Non-object JSON raised an uncaught
TypeErrorfrom a membership test — on Copilot this was a regression introduced by the first commit here, surfacing as a retryable auth error that burned the whole agent retry budget. Separately, Copilot's interrupt/partial path could return a non-dict asAgentOutput.content, which is declareddict[str, Any]and has no recovery loop.The unwrap heuristic was tightened. It originally included a bare sole-key fallback and would resolve
{"answer": {"error": "I could not complete the task"}}into an answer, and flip{"approved": {"not_approved": false}}. It now requires an unambiguous named match. A separate bug meant a field literally namedvalueorresultcollided with the generic keys and was never unwrapped at all.A trap worth knowing.
parse_json_outputwraps JSON syntax errors inValidationErrortoo, so the two failure kinds cannot be told apart by exception type. Hermes splits them by which call failed. This is documented in AGENTS.md because it is easy to reintroduce.One behaviour change to a public error surface.
test_json_schema_validation_errorasserted the old genericProviderErrorand now asserts the preservedValidationError. A companion test pins theProviderErrorsyntax path.One divergence pinned rather than fixed. Hermes accepts a bare string where Copilot and Claude reject it, because
parse_json_outputrewrites non-dicts to{"result": ...}first. Changing that would affectsetandscriptsteps, so it is covered by a test with an explanatory docstring instead.Validation
make check(lint + typecheck) cleanmake build-frontend;make test-frontendgreenmainafter feat(claude-agent-sdk): support workflow MCP servers (#335) #346 landed