fix(agent): make in-band is_error tool-scoped and exit-code-aware

[efecnc]

Summary

Refines the in-band is_error detection introduced in #46 so that:

1. Non-zero exit codes from exec/python_run are detected (they were previously missed unless the output happened to begin with Error:).
2. Content tools like read_file are no longer false-flagged when their payload legitimately begins with Error:.
3. The exec Exit code: marker survives truncation and the grep advisory, so the signal is reliable even on large or grep-like output.

Why this overlaps with main, and how it's resolved

This change was developed in parallel with #46 (8d7dafde), which independently added in-band is_error via utils::tool_output_looks_like_failure(text) — a text-only check (starts_with "Error:" / "error:") applied to every tool's finalized output at the two reasoning-loop tool-result sites in src/agent/mod.rs.

Because both touch the same call sites and the same helper area in src/utils.rs, they overlap. Rather than duplicate the mechanism, this PR builds on #46 and closes two real gaps in the text-only heuristic:

Gap in the text-only check	Consequence	Fix here
Misses non-zero exit codes. exec/python_run signal failure by appending a trailing Exit code: <N> line, not by starting with Error:.	A failed cargo build (exit 1 whose output doesn't start with Error:) was recorded as a success — the model builds on broken state, and the signature-based doom-loop detector reasons over a wrong outcome.	exec/python_run are flagged when the last non-empty line is Exit code: <non-zero> (tail-anchored).
False-positives on file content. The text-only check ran on every tool's output, so read_file returning a log/file that begins with Error: was misclassified as a failed call.	Spurious is_error on legitimate reads.	The Error: prefix rule is tool-scoped to exactly the four tools whose entire Ok payload is a control message: edit_file / list_dir / glob_files / search_text.
Checks the finalized (truncated) text.	A non-zero exit on >10 KB or grep-like output could have its Exit code: line truncated/pushed off the tail before the check ran.	is_error is computed on the raw Ok payload before finalize_tool_output, and exec now appends the marker last (after the grep advisory and the internal 10 KB cap) so it is always the final line.

utils::tool_output_looks_like_failure from #46 is kept — it still backs channels/terminal.rs UI result tinting (tool-agnostic, already-finalized text). The new tool_output_signals_failure / tool_call_is_error are the precise, tool-aware path used for the agent's structured is_error. The two concerns (UI tinting vs. agent semantics) stay cleanly separated.

Why tool-scoping is precise, not arbitrary

The {edit_file, list_dir, glob_files, search_text} set is the complete list of tools that emit the Ok("Error: …") / Ok("Error reading dir: …") prefix convention in tools/builtin.rs (verified by enumerating every in-band Ok(...) failure return). read_file and the other content-returning tools are intentionally excluded because their payload is data, not a control message.

Changes

• src/utils.rs — add tool_output_signals_failure(tool_name, raw_output) (tool-scoped, exit-code-aware) and tool_call_is_error(tool_name, &Result<String,String>). 9 unit tests covering exit-code detection, tail-anchoring, scoped-prefix matching, the read_file non-misclassification, exit-code edge cases (zero / garbage / empty), and the documented spoofed-marker residual.
• src/agent/mod.rs — both tool-result sites compute is_error via tool_call_is_error on the raw result, before finalize_tool_output consumes it.
• src/tools/builtin.rs — exec appends the non-zero Exit code: marker last (survives the advisory + 10 KB truncation); a guard comment on python_run documents the same marker-last invariant. 3 unix integration tests (large failing output, grep-like failing output, success).

Verification

• cargo test --lib utils:: → 15 passed (9 new).
• cargo test --lib exec_failure_tests:: → 3 passed.
• cargo clippy --release --all-targets → no new warnings on the changed code.

Notes / accepted residuals

• Accepted false positive (documented + tested): a successful command whose own final output line is literally Exit code: <non-zero>. The harness appends the authoritative marker after all command-controlled bytes, so this can only ever produce a spurious is_error (at worst a wasted retry) — it can never mask a real failure (the dangerous direction is closed by construction).
• Pre-existing, out of scope: src/tools/builtin.rs truncates exec output with &result[..10000], a raw byte slice that can panic on a multi-byte UTF-8 boundary. This predates the change (identical to main; only relocated here) and is left for a separate follow-up that can switch it to the existing truncate_utf8_safe helper without altering this PR's truncation-notice semantics.

[efecnc]

Independent review (rust + security), posted for transparency

Two independent reviewer passes were run against git diff upstream/main before opening. Both APPROVE; no CRITICAL or HIGH issues. Summary:

Correctness (rust):

• Tail-anchoring (lines().rev().find(|l| !l.trim().is_empty()) → strip_prefix("Exit code:") → parse::<i64>() != 0) is sound; str::lines() drops the trailing newline so the appended marker is a clean final element.
• Computing is_error before finalize_tool_output is not just safe but required: that closure runs truncate_utf8_safe (front-anchored), which would delete the trailing marker on long output.
• The exec reorder keeps result correctly mut; the (no output) sentinel is now result.is_empty() && exit_marker.is_none(), so a no-stdout non-zero exit (false, exit 7) correctly surfaces Exit code: instead of being swallowed as (no output) — a strict improvement over the prior behavior.
• No new unwrap/panic in production paths; no ReDoS surface (no regex).

Security (signal integrity):

• The most dangerous direction — a real failure masked as success (false negative) — is closed by construction: the harness appends the authoritative Exit code: marker after all command-controlled bytes, so command output can never become the last line and override it. Spoofing can only yield a spurious is_error (wasted retry).
• The doom-loop detector keys on tool-call signatures (name + args hash), not on is_error, so a spurious flag cannot trip or suppress loop detection.
• is_error on the raw pre-truncation payload leaks nothing: only a bool is derived; the model still sees exactly the finalized tool_result_text.

Addressed from review before opening:

• Doc-comment precision: reworded to claim completeness of the Error:-prefix convention only, and explicitly noted other phrasings (e.g. cron's "was not found") that neither this nor the prior text-only check flags — low-risk, non-state-corrupting, message still informs the model.
• Added a marker-last guard comment on the python_run path to prevent a future advisory/truncation from regressing the invariant.

Noted pre-existing (out of scope, flagged for follow-up): the &result[..10000] byte slice in exec truncation can panic on a multi-byte UTF-8 boundary; identical to main, only relocated here.

[gemini-code-assist]

Code Review

This pull request improves tool execution failure detection by introducing tool-scoped and exit-aware checks (tool_call_is_error and tool_output_signals_failure), ensuring that non-zero exit codes are reliably detected and content-returning tools are not false-flagged. Feedback was provided regarding a potential panic in src/tools/builtin.rs where slicing a string at a fixed byte limit of 10,000 could violate UTF-8 character boundaries; a safer slicing approach using is_char_boundary was suggested.

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.

[gemini-code-assist]

Slicing result directly with &result[..10000] can panic if the index 10000 does not fall on a valid UTF-8 character boundary (e.g., if the output contains multi-byte characters like emojis or non-ASCII text). To prevent potential panics, find the nearest valid character boundary at or below 10000 before slicing.

Suggested change

	if result.len() > 10000 {
	Ok(format!(
	result = format!(
	"{}\n... (truncated, {} more chars)",
	&result[..10000],
	result.len() - 10000
	))
	} else {
	Ok(result)
	);
	}
	if result.len() > 10000 {
	let mut limit = 10000;
	while limit > 0 && !result.is_char_boundary(limit) {
	limit -= 1;
	}
	result = format!(
	"{}\n... (truncated, {} more chars)",
	&result[..limit],
	result.len() - limit
	);
	}

[efecnc]

Thanks — addressed the UTF-8 boundary panic in 9e… (latest commit): the exec >10 KB truncation now steps back to the nearest is_char_boundary at/below the cap before slicing, matching the idiom already used at the other truncation sites in builtin.rs. Added a regression test (large_multibyte_output_truncates_on_a_char_boundary) that emits 5000 × 3-byte ₺ so byte 10000 lands mid-character — it panicked before the fix and passes now. Note this slice predates the PR (it was relocated, not introduced, by the is_error change), but since it sits in touched code I fixed it here rather than deferring.