feat(agent): P1.3 — structured tool-error signal to the model (is_error) - #42
Merged
Merged
Conversation
Tool failures previously reached the model only as an "Error:" text prefix; the computed is_error boolean was used for telemetry but dropped before the provider. Now a failed tool call carries the failure through to the API: - ChatMessage gains an internal-only `is_error: Option<bool>` (#[serde(default, skip_serializing)]), so the OpenAI-compatible wire is byte-identical and strict endpoints can't reject an unknown message field. - The Anthropic message builder (convert_messages) sets the native `is_error: true` on the tool_result content block on failure (absent on success). - ChatMessage::tool_with_error sets it; both the parallel and sequential tool-dispatch paths now use it. The "Error:" text prefix is preserved as the OpenAI-compatible signal. Self-correction (the heart of loop robustness): Anthropic models get a first-class failure signal instead of inferring it from text. Tests: native is_error only on failure (success/legacy omit it); is_error never on the OpenAI wire. cargo test --lib green (302); clippy adds no new warnings.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces an internal is_error field to ChatMessage to support Anthropic's native failure flag on tool results, providing a structured error signal instead of relying solely on text prefixes. This field is skipped during serialization to avoid breaking OpenAI-compatible endpoints. The reviewer points out that because is_error is not persisted in the SQLite database, this structured flag is lost when a session is reloaded from history, and suggests adding an is_error column to the database to preserve this signal across restarts.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+889
to
+891
| // Not persisted (skip_serializing internal field); a reloaded | ||
| // message keeps only the "Error:" text already in `content`. | ||
| is_error: None, |
Contributor
There was a problem hiding this comment.
Losing the is_error flag on session reload/resume is a potential issue for loop robustness. When a session is resumed from the database, the model will only see the "Error:" text prefix for past tool failures, losing the structured is_error: true signal. Consider adding an is_error column to the messages table (e.g., as an integer or boolean) to persist this flag across restarts, ensuring consistent self-correction behavior even after a session is reloaded.
…clarify doc Addresses the independent review of altaidevorg#42: - Wrap the two new test asserts to rustfmt's form (provider.rs). Left the pre-existing line-441 fmt drift untouched (CI does not gate fmt). - Strengthen `is_error_is_never_serialized_to_openai_wire` to also assert against the real `{"messages":[...]}` request-body shape, not just the bare struct. - Tighten the `is_error` field doc: it tracks execution/transport failure (dispatch Result::Err); in-band `Ok("Error: ...")` tool errors still travel as text only (widening the structured flag to the textual heuristic is noted as a follow-up).
Contributor
Author
🔍 Code review — independent reviewer pass (+ resolution)Reviewed by independent Verified clean by review
Addressed
Deferred (follow-up)
Verdict: APPROVE — correct, surgical, non-behavioral; nits resolved. |
efecnc
added a commit
to efecnc/isanagent
that referenced
this pull request
Jun 4, 2026
…"Error:"))
A tool message's `is_error` was derived solely from the dispatch `Result`, so
tools that report recoverable failures *inside* `Ok(...)` were recorded as
successes. The model then saw a failed edit or a non-zero command as a success,
which also blinded the doom-loop detector (it reasons about tool outcomes).
Conventions in this codebase:
- `exec` / `python_run` append a trailing `Exit code: <N>` line on non-zero exit.
- `edit_file` / `list_dir` / `glob_files` / `search_text` return `Ok("Error: ...")`.
Changes:
- utils: add `tool_output_signals_failure(tool_name, raw_output)` — a tool-scoped,
anchored heuristic — and `tool_call_is_error(tool_name, &Result)`, the single
source of truth used at both tool-result sites in the reasoning loop. Detection
runs on the RAW `Ok` payload (the exit marker sits at the tail and
`finalize_tool_output` would truncate it away). Scope:
- exec/python_run: only the trailing `Exit code: <non-zero>` marker.
- edit_file/list_dir/glob_files/search_text: an `Error:` prefix (their whole
payload is a control/listing message, never raw file content).
read_file and other content tools are intentionally excluded to avoid false
positives on file content that merely begins with "Error:".
- builtin (`exec`): append the `Exit code:` marker LAST — after the grep advisory
and after the internal 10 KB truncation — so it always survives as the final
line. Previously the advisory and the size-truncation trailed it, so a genuine
non-zero exit on grep-like or large output was silently recorded as a success.
- agent: both tool-result sites now call `tool_call_is_error`.
The Anthropic message builder already maps `is_error` onto the native
`tool_result.is_error`; the OpenAI wire format is unchanged (field is
`skip_serializing`). Follow-up to the internal `is_error` field added in altaidevorg#42.
Known, accepted residual: a *successful* command whose own output ends with a
line literally `Exit code: <non-zero>` is a false positive (the runner only
appends the marker on real non-zero exit). Low frequency; the only consequence
is a spurious retry, never a masked real failure — documented and pinned by test.
Tests: utils heuristic + wrapper (exit cases, anchoring, scoped Error-prefix,
file-content/unrelated-tool guards, documented residual); plus exec contract
tests proving the marker stays the final line under 10 KB truncation and the
grep advisory, and that successful commands carry no marker.
This was referenced Jun 4, 2026
Closed
monatis
pushed a commit
that referenced
this pull request
Jun 4, 2026
…r decay Post-merge follow-ups on #42 (is_error) and #44 (doom-loop): - is_error now also flags tools that report a *recoverable* failure in-band as Ok("Error: ...") (e.g. "old_text not found"), not just a dispatch Err — so the model's structured failure signal matches what the result text already conveys. Extracted shared `utils::tool_output_looks_like_failure` and reused it in the terminal UI failure phase (was a duplicated heuristic) and both tool-dispatch sites. - Doom-loop escalation: on a detected-but-not-tail-active turn, decay the consecutive-detection counter instead of hard-resetting, so an intermittently-varying loop (mostly repeating with the occasional one-off) still escalates over time rather than being fully forgiven by one turn. cargo test --lib green (324 passed); clippy adds no new warnings in changed files.
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
2 participants
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.
Context
Follow-on to the P0/P1 harness work. The review flagged a P1 ACI gap: when a tool call fails, the model only sees an
"Error:"text prefix — it has to infer failure from prose. Theis_errorboolean is computed at dispatch (agent/mod.rs) and sent to telemetry/UI, but dropped before the provider request; even the Anthropic path never set the API's ownis_error. Surfacing a structured failure signal is the single highest-leverage fix for self-correction, which is the heart of loop robustness.What changed, and why
ChatMessagegains an internal-onlyis_error: Option<bool>with#[serde(default, skip_serializing)]. Why internal-only: the OpenAI-compatible request serializesChatMessagedirectly, and strict endpoints reject unknown message fields — so this field must never hit that wire.skip_serializingguarantees the OpenAI-compat wire is byte-identical;defaultkeeps older persisted messages deserializing.convert_messagesnow sets the nativeis_error: trueon thetool_resultcontent block when the tool failed (omitted on success — Anthropic treats absence as success). This is read from the Rust field, not serde, so it's fully controlled.ChatMessage::tool_with_error(...)records the flag; both the parallel and sequential tool-dispatch paths inagent/mod.rsnow use it. The existing"Error:"text prefix is preserved as the signal for OpenAI-compatible providers (which have no native equivalent).Test plan
anthropic_tool_result_sets_is_error_only_on_failure— failed tool →is_error: true; success/legacy(None) → no key.is_error_is_never_serialized_to_openai_wire— serializing a failed-toolChatMessageproduces nois_errorfield.cargo test --libgreen (302 passed);cargo check --all-targetsgreen;cargo clippyadds no new warnings.mod.rs:1778) and repair-placeholder (mod.rs:225) tool messages should ever be markedis_error(currentlyNone).