fix: address Phase C review findings#66
Merged
Merged
Conversation
Five-agent review of the combined v0.10.0...main diff (PRs #61-#65) surfaced convergent findings; this is the full fix batch. Critical fixes: - prompt_structured error handling overhauled: provider failures now surface as StructuredPromptError::Provider carrying the real error (previously laundered into Parse { raw: "" }); the raw-text scan is scoped to messages produced by THIS call (a failed run can no longer return an earlier turn's JSON as Ok(T)); the payload is taken from the LAST text block (tool-forcing appends after preamble); Parse now carries #[source] serde_json::Error. - Schema drop-leak closed structurally: the output_schema field on Agent is gone — the schema threads per-call through prompt_messages_internal into the run's AgentLoopConfig, so a timed-out/dropped future can't leave the agent schema-forced. - Bedrock replays Content::Thinking (with signature) as reasoningContent — previously captured then dropped, guaranteeing a ValidationException on multi-turn thinking + tool use. - Anthropic structured outputs disable thinking for that request (forced tool_choice + extended thinking is an API-level 400) with a warning. - Session::append_new verifies the history extends the current path and returns HistoryDiverged instead of silently dropping turns or corrupting the tree when compaction (on by default) rewrites agent messages; returns the appended count. - Session::from_jsonl single-pass validation: DuplicateId + UnknownParent (parents must precede children, which also makes cycles impossible — path_ids provably terminates). API shape (free now — these types are unreleased): - ToolMiddleware::before_tool takes a #[non_exhaustive] ToolCallRequest context struct instead of three frozen positional params. - StreamConfig and OutputSchema are #[non_exhaustive]; StreamConfig::new added (this release alone added output_schema to it). - ToolDecision documents the deliberate StopReason-style exhaustive policy. Hardening: - A panicking ToolMiddleware is contained (catch_unwind) as a Deny instead of killing the loop task and stripping the agent of its tools. - Middleware denials emit tracing::warn! (operator visibility). - unwrap_structured_tool_call removes only the synthetic tool call and preserves non-ToolUse stop reasons (Length truncation no longer laundered into success). - llm_stream span gains an error field; seek_checkpoint is latest-wins on duplicate labels; Vertex round-trips Gemini thought signatures on function calls (parity with google.rs). Docs: tools.md example on the new signature + panic note; structured-outputs error semantics + schema-dialect caveats + thinking caveat; telemetry OTel snippet updated to the current builder API (old one was removed in 0.17) and overhead claims made honest; session-trees compaction caveat + validation; "enforced natively where supported"; CHANGELOG Fixed section; CLAUDE.md. Tests (+15, 376 total): schema propagation via capturing provider (replaces the tautological reset test); Provider-error and stale-history-never-parsed; middleware never sees the synthetic tool; middleware panic containment; Batched-strategy deny; session divergence/dup-id/dangling-parent/latest-wins; bedrock thinking-replay body; anthropic structured-disables-thinking body; google thinking-drop pin; vertex signature round-trip; telemetry field values including exact cost math (on_record collector). Co-Authored-By: Claude Fable 5 <noreply@anthropic.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
1 participant
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.
What
Full fix batch from the five-agent review of the combined Phase C diff (
v0.10.0...main, PRs #61–#65). The reviewers converged independently on the same core issues; everything material is fixed here, plus the API-shape changes that are free now but breaking after 0.11 ships.Critical fixes
prompt_structuredlaundered provider errors intoParse{raw:""}and could return an earlier turn's JSON asOk(T)(4 reviewers)Provider{message}variant fed from the run's error; scan scoped to this call's messages; payload from the last text block;Parsecarries#[source] serde_json::Errortimeout(prompt_structured(...))left the agent permanently schema-forced (3)output_schemafield onAgentis gone; the schema threads per-call into the run's configValidationExceptionon multi-turn thinking+tools (2)Content::Thinkingserialized back asreasoningContentwith signature (+ body test)tool_choice+ thinking = API-level 400 (3)Session::append_newcount-alignment silently dropped turns / corrupted the tree under compaction (2)HistoryDiverged; documented compaction caveatfrom_jsonlaccepted duplicate ids / dangling parents / cycles (infinite hang) (2)DuplicateId,UnknownParent; parents-precede-children makes cycles impossiblecatch_unwind→ contained asDeny("tool middleware panicked")API shape (free now — these types are unreleased)
ToolMiddleware::before_tool(&self, call: &ToolCallRequest<'_>)—#[non_exhaustive]context struct instead of three frozen positional params (extensible without ever breaking implementors).StreamConfig+OutputSchemaare#[non_exhaustive]with constructors — this release alone added a field toStreamConfig; downstream literal constructors stop breaking every minor.ToolDecisiondocuments its deliberateStopReason-style exhaustive policy.Hardening & docs
Denials now visible to telemetry (
warn!) ·llm_streamgains anerrorfield · unwrap removes only the synthetic tool and preservesLength(truncation no longer laundered into success) ·seek_checkpointlatest-wins · Vertex thought-signature parity with google.rs · OTel doc snippet updated to the current builder API (old one was removed in opentelemetry_otlp 0.17) · honest overhead claims · schema-dialect caveats (OpenAI strict / Gemini).Tests (+15, 376 total green)
Highlights: schema propagation pinned with a capturing provider (the old reset test was tautological — MockProvider ignores its config) · provider-error and stale-history-never-parsed · middleware never sees the synthetic tool · panic containment · Batched-strategy deny · session divergence/validation · telemetry field values incl. exact cost math via an
on_recordcollector.Verification
clippy
-Dwarnings --all-featuresclean · 376 passed / 0 failed · docs-Dwarningsclean.🤖 Generated with Claude Code