perf(tui): coalesce repeated read-only calls

[Hmbown]

Summary

• track finalized read-only calls per external user turn by resolved tool name plus canonical argument hash
• coalesce eligible same-batch duplicates behind one physical execution while preserving one result, event, and provider tool ID closure per request
• add a replay-stable corrective nudge at occurrence 3, reuse a compact prior-result receipt at occurrence 5, and stop deterministically at occurrence 8
• treat writes, approvals, detached calls, conflicts, and other ineligible executions as barriers; retain the existing stuck guard outside this seam

Safety contract

• counters reset at the next top-level user turn
• interleaved model steps do not reset identical-read counts
• write/read ordering cannot reuse stale same-batch results
• skipped receipts carry executed: false and do not mutate the working set or trigger post-edit diagnostics
• every provider tool-call ID receives exactly one terminal result, including the stop path
• cancellation remains a terminal cancellation outcome

Verification

• 4 pure read-repeat guard tests
• 2 same-batch and write-barrier planner tests
• cross-step fifth-occurrence receipt test
• injected-model duplicate-read lifecycle test
• 7 legacy stuck-guard tests
• strict all-target/all-feature TUI clippy with warnings denied
• fmt and diff checks
• exact-commit gitleaks scan

No provider or model routing behavior changes.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @Hmbown's task in 5m 0s —— View job

---

Review: perf(tui): coalesce repeated read-only calls

Todo list

• Gather context (diff vs origin/main, changed files)
• Read read_repeat_guard.rs (new module)
• Read dispatch.rs changes
• Read turn_loop.rs changes
• Read engine.rs change
• Read new tests
• Check for provider/model routing safety issues
• Check for correctness bugs (panics, non-exhaustive matches, etc.)
• Post final review

Note: cargo/build tooling wasn't invokable in this sandbox (commands required approval that wasn't available), so this is a manual read-through rather than a compiled/tested verification. I traced every new symbol back to its definition (ToolExecutionOutcome, ToolTerminalStatus, crate::hashing::sha256_hex, the mock canned::* helpers, make_plan_at, etc.) to check the diff is self-consistent.

Summary

This is a clean, well-scoped addition. The design cleanly separates counting (per-turn, name+canonical-arg-hash keyed, survives interleaving) from plan-time coalescing (same-batch only, reset by any write/approval/detached/blocked barrier) from execution-time result fan-out (every provider tool-call ID still gets its own event/result). The indexing scheme (plan.index sized to the pre-split plans.len(), outcomes resized accordingly, coalesced followers backfilled into outcomes before the terminal-processing loop) is consistent throughout — I didn't find an off-by-one or panic path in the new code.

Things that check out:

• plan_read_repeat_execution (dispatch.rs:543) correctly treats any ineligible plan (write, interactive, detached, blocked, guard-already-set) as a barrier via leaders.clear(), so a same-batch read after a write can't subscribe to a pre-write leader — matches the stated safety contract, and is covered by write_barrier_prevents_stale_same_batch_read_subscription.
• The 3/5/8 thresholds are implemented as documented: nudge at ≥3 (decorate_model_result), receipt reuse at ≥5 (prior_receipt / coalesced_result), deterministic stop at ≥8. The stop check (turn_loop.rs:2867) runs only after the full outcomes loop finishes adding session messages for every outcome in that step, so the "every tool-call ID gets exactly one terminal result, including the stop path" contract holds.
• Cancellation is preserved correctly for coalesced followers: leader_status == Some(ToolTerminalStatus::Cancelled) is checked against the derived Ok(result) from coalesced_result, and since ToolExecutionOutcome::cancelled always stores result: Some(_), error: None, legacy_result() always yields Ok, so that branch is reachable and correctly re-wraps as ToolExecutionOutcome::cancelled(result) for the follower too.
• remember_success correctly no-ops on receipts/coalesced results (!result_was_executed(result)), so a "not executed" stand-in never clobbers the real remembered result used for future receipts.
• Reuses the existing shared crate::hashing::sha256_hex rather than adding another private copy (the codebase has several ad-hoc private sha256_hex helpers elsewhere — not touched by this PR, but worth a future cleanup pass).
• No provider/model-prefix inference anywhere in this diff — consistent with the PR's "no provider or model routing behavior changes" claim.
• Good test coverage: plan-level dedup/barrier/receipt unit tests plus a full injected-model integration test (injected_model_duplicate_reads_execute_once_and_close_both_tool_ids) that verifies both tool IDs get a ToolCallComplete and that the next request still carries both tool_use_ids in ContentBlock::ToolResult.

Minor observation (non-blocking)

crates/tui/src/core/engine/turn_loop.rs:2837 — in the Err(e) arm of the terminal-outcome match, self.session.working_set.observe_tool_call(...) is called unconditionally, unlike the Ok(output) arm which gates it on tool_was_executed (line 2771). A coalesced follower whose leader failed with a genuine ToolError (not a ToolResult{success:false}, e.g. a timeout) takes this path and gets its "touch" recorded into the working set a second time for content that was never physically re-executed. Practically low-impact (working-set bookkeeping only, read-only tools, no mutation of files/session correctness), and it's a narrow edge case (duplicate read failing with a raw ToolError, coalesced in the same batch) — but it's a small gap versus the PR's own contract line: "skipped receipts carry executed: false and do not mutate the working set." ToolError has no metadata/executed concept to check against, which is presumably why this path wasn't gated — worth a one-line fix (skip observe_tool_call for followers, keyed off read_repeat_occurrences.get(&outcome.index) count > leader's) if you want the contract to hold exactly, but I wouldn't block on it.

No other correctness, safety-contract, or test-coverage gaps found.

[claude]

Claude finished @Hmbown's task in 2m 3s —— View job

---

Review: perf(tui): coalesce repeated read-only calls (re-review after new commit)

Todo list

• Gather context (diff vs origin/main since prior review)
• Re-check read_repeat_guard.rs, dispatch.rs, turn_loop.rs for changes since last review
• Check whether prior minor observation (unconditional observe_tool_call in Err(e) arm) was addressed
• Check for correctness bugs, panics, non-exhaustive matches
• Check provider/model routing safety
• Check reuse/simplification and test coverage
• Post final review

Note: cargo wasn't invokable in this sandbox (approval required, not available), so this is a manual trace of the new commit against the surrounding code rather than a compiled/tested verification.

What changed since the last review

Commit 537d897 (fix(tui): skip coalesced error working-set touches, turn_loop.rs only, +16/-6) directly fixes the minor observation from my prior pass: a coalesced read-repeat follower whose leader failed with a raw ToolError (not a ToolResult{success:false}) was unconditionally re-touching working_set.observe_tool_call, contradicting the PR's own contract that skipped receipts "do not mutate the working set."

The fix builds coalesced_read_indices — a HashSet<usize> of every follower.index from coalesced_read_plans (turn_loop.rs:2030-2033) — before the outcomes loop, and gates the Err(e) arm's observe_tool_call call on !coalesced_read_indices.contains(&outcome.index) (turn_loop.rs:2845).

I traced this for correctness:

• follower.index is the same plan.index used everywhere else to key outcomes[...] (dispatch.rs:45, turn_loop.rs:2676), and outcome.index in the loop comes from that same ToolExecOutcome.index — so the set membership check lines up exactly with the follower entries, no off-by-one.
• The leader itself is never in coalesced_read_indices (only followers are), so a leader that genuinely executes and errors still gets its one physical working-set touch — the fix only suppresses the second touch from the subscribing follower.
• This composes correctly with the pre-existing tool_was_executed gate in the Ok(output) arm (turn_loop.rs:2769-2782): a coalesced leader failure that comes back as ToolResult{success:false} already carries executed: false metadata via coalesced_result (read_repeat_guard.rs:107-118) and was already handled; this fix covers the complementary case where the leader's failure has no ToolResult to stamp metadata onto at all.
• Good comment explaining why (ToolError has no executed metadata concept), matching the repo's comment-only-for-non-obvious-WHY convention.

This closes the gap cleanly — no new correctness issues introduced.

Test coverage gap (non-blocking)

No test exercises this new branch specifically. The existing injected_model_duplicate_reads_execute_once_and_close_both_tool_ids (tests.rs:1289) only covers a successful leader read; there's no test where the leader read errors with a raw ToolError (e.g. a mock tool returning Err(ToolError::execution_failed(...))) and asserts working_set.entries shows a single touch (touches == 1) rather than two. WorkingSetEntry::touches (working_set.rs:978) is already the right observable to assert on — worth adding a focused test for this exact scenario if you want the "no double-touch on coalesced ToolError" contract regression-proofed.

Fix this →

Everything else

No provider/model routing changes in this commit (confirmed, turn_loop.rs-only diff, no model/route inference touched). Rest of the diff is unchanged from my prior pass and still holds up — no other correctness, safety-contract, or coverage gaps found.

[claude]

Claude finished @Hmbown's task in 1m 50s —— View job

---

Review: perf(tui): coalesce repeated read-only calls (re-review after new commit)

Todo list

• Gather context (diff vs origin/main, git log since prior review)
• Check new commit 1e6215f (test(tui): cover coalesced raw read errors) against the test-coverage gap noted in the prior review
• Re-verify no other correctness/safety issues introduced
• Post final review

Note: cargo isn't invokable in this sandbox (approval required, not available), consistent with the prior two reviews — this is a manual trace of the new commit against the working-set extraction logic rather than a compiled/tested verification.

What changed since the last review

Commit 1e6215f (test(tui): cover coalesced raw read errors, tests.rs only, +56) adds coalesced_raw_read_error_touches_working_set_once, closing the coverage gap I flagged previously: no test exercised a coalesced follower whose leader fails with a raw ToolError (as opposed to ToolResult{success:false}).

I verified the test actually exercises that exact path and asserts the right thing:

• The leader genuinely raises a raw ToolError, not ToolResult{success:false}. ReadFileTool::execute (crates/tui/src/tools/file.rs:154-156) opens the file with fs::File::open(...).map_err(|e| ToolError::execution_failed(...))? — a missing missing.rs path returns Err(ToolError) before any ToolResult is constructed, so this test hits the Err(e) arm gated by the 537d897 fix (turn_loop.rs:2845), not the pre-existing tool_was_executed/executed: false metadata path.
• The touches == 2 assertion is correct, not an arbitrary number. I traced working_set.rs: for input {"path":"missing.rs"}, extract_paths_from_value_inner (working_set.rs:1458-1469) matches the path key as path-like, extracts "missing.rs" once via extract_paths_from_text, then pushes it a second time via the key_suggests_path && !contains('/') branch — two candidates from one physical observe_tool_call. record_candidates (working_set.rs:1250-1260) increments touches once per candidate with no in-call dedup, so one physical observation yields two touches. Under the old bug (follower double-touching), that would have been four. The test's inline comment explains this precisely, and the assertion (entry.touches == 2) would correctly fail if the 537d897 fix regressed.
• The test drives the real Engine::handle_deepseek_turn (turn_loop.rs:261, pub(super)) directly rather than the full engine.run() event-loop harness used by the sibling injected_model_duplicate_reads_execute_once_and_close_both_tool_ids test — a legitimate lower-ceremony call since handle_deepseek_turn's signature is invoked identically to the production call site (engine.rs:3082-3089), and the MockLlmClient returns canned responses independent of prior session state, so skipping the external_user_message_op send doesn't change what's being exercised.

This is a solid, minimal, correctly-targeted regression test for exactly the gap identified — no compile-time issues apparent from the trace (imports/helpers all reuse symbols already used by neighboring tests in the same file).

Everything else

No other files changed in this commit. The rest of the diff is unchanged from my prior two passes and still holds up — no correctness, provider/model-routing, or safety-contract gaps found.