Collapse exact testreport_read re-reads to a notice

[plusky]

Summary

testreport_read collapses an exact re-read of the same window to a short [unchanged since <hash>, N lines; use offset/limit to move] notice instead of resending the full text. Kills the observed 99k-token / 109x re-read spike; edits always resend (hash + line-count gated, bounded 16-window per-session LRU).

Changes

• crates/mtui-mcp/src/session.rs: RereadKey { rrid, relpath, offset, limit } + LRU(16) + inline FNV-1a64 (specified hash, zero new deps; comparison uses full u64, display truncates cosmetically).
• testreport_tools.rs: key from template-arg-or-active-RRID + normalised relpath (./log shares log's key); ambiguous multi-template calls refuse before cache lookup (no cross-template poisoning); escaping relpaths error before lookup; same JSON shape, output-only, no schema change beyond one documented description sentence.
• Cache never suppresses edits: same-length one-line swaps, truncation-total changes, and cap changes all miss and resend (pinned by tests).

Checklist

• Commits follow Conventional Commits.
• cargo fmt --all --check is clean.
• cargo clippy --workspace --all-targets --all-features -- -D warnings is clean.
• cargo test --workspace passes.
• Feature matrix builds (--no-default-features, --all-features, compile-only).
• New/changed code covered (7 unit + dispatch integration; every branch exercised incl. eviction and cross-RRID isolation).
• User-visible changes recorded in CHANGELOG.md.
• Docs updated where relevant (tool description carries the one-sentence collapse note; snapshot regenerated).

Related issues

Part 3 of 4 in the MCP token-reduction stack. Merge order: log-fold → json-crush → reread-dedup → token-nudges. Expected trivial conflict with siblings on CHANGELOG.md only; resolved at merge time in that order.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.85%. Comparing base (98f3fa0) to head (7b2a109).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #611      +/-   ##
==========================================
+ Coverage   96.83%   96.85%   +0.02%     
==========================================
  Files         214      214              
  Lines       62943    63399     +456     
==========================================
+ Hits        60951    61407     +456     
  Misses       1992     1992              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

[mimi1vx]

Blocker

• [crates/mtui-mcp/src/testreport_tools.rs:448] No bypass mechanism (force / no_cache) to retrieve file content on re-read. Collapsing identical reads to "[unchanged since …]" in the "content" field breaks callers (such as LLM subagents or pipelines whose context was compacted) that genuinely need to inspect the file text again. Add an optional parameter (e.g. force: Option<bool>) allowing a client to bypass the notice and get the real file content.
• [crates/mtui-mcp/src/testreport_tools.rs:413] rrid_key discrepancy between explicit and defaulted template calls. If a single template is loaded but not active, resolve_target_path succeeds while guard.templates.active_rrid() is None (resulting in rrid = ""). A subsequent call passing template = Some(rrid) produces a different RereadKey, causing a false cache miss. Using the canonical resolved path: PathBuf as the key eliminates both template-resolution discrepancies and path-normalization variations.
• [crates/mtui-mcp/src/session.rs:643] Notice message missing force-read instructions. Document in the notice message how callers can force a full re-read.

[mimi1vx]

Blockers

1. [P0] Dedup key is canonicalized on the explicit-relpath path but not on the default (no-relpath) path, so two spellings of the same file don't share a key on macOS. testreport_tools.rs::resolve_target_path: when relpath is None it returns resolve_path(session, template) (the raw path stored on the report, uncanonicalized); when relpath is Some(_) it goes through safe_template_file, which calls base.canonicalize() first. On a host where the temp/checkout path crosses a symlink (macOS: /var → /private/var), the two branches produce different PathBufs for the identical on-disk file, so RereadKey differs and the second read never collapses to the "unchanged" notice — this is the exact "path-normalization variations" gap called out in the original review, fixed for the template-resolution case but not this one.

   Reproduced 100% locally (not flaky): cargo test -p mtui-mcp --lib testreport_tools::tests::reread_relpath_spellings_share_one_key fails every run on macOS; this is also why the PR's own "MTUI macOS (Apple Silicon)" CI job is red while the Linux job (no such symlink) is green.

   Fix: canonicalize consistently on both branches — either canonicalize the result of resolve_path the same way safe_template_file canonicalizes base, or canonicalize once at the RereadKey construction site in testreport_read so neither branch has to be trusted individually. Add (or fix) coverage that actually runs on a symlink-crossing temp path so this can't regress silently on Linux-only CI again.

[plusky]

CI fix (cccc70e): the macOS failure was a real platform bug, not flakes. The relpath arm canonicalizes the checkout base while the defaulted log path spells symlinked prefixes verbatim — on macOS TMPDIR lives under /var (→ /private/var), so the two reads keyed different entries and missed dedup. The reread key path is now canonicalized (output path display unchanged), plus a Linux-runnable regression test with a symlinked checkout dir (red-proven by reverting the one-line fix). mtui-mcp lib 297 green, fmt/clippy clean.

[mimi1vx]

BLOCKING: crates/mtui-mcp/src/testreport_tools.rs — path.canonicalize() now runs inline on the async task on every testreport_read call (not just cache hits), not inside the existing spawn_blocking wrapper — this violates the function's own stated policy ("must not block a Tokio thread") on the hottest MCP read path.

BLOCKING: the collapsed "unchanged" response is only detectable by string-matching "[unchanged since" inside content, with no structured flag — a non-LLM consumer expecting content to always be file text will silently receive a synthetic string instead.

FLAG: same root cause as #620 — a blocking syscall added to an async dispatch path without spawn_blocking. Please coordinate a single fix/convention reminder across both PRs instead of independent patches.

[plusky]

Review-sweep responses (2026-09-12) — all findings addressed, each rework adversarially re-reviewed (GO) with red-proven tests, full workspace gates green. Cross-PR summary (per-PR diffs carry the detail):

• Fold repetitive fan-out output (run/reboot/perform) #608 log-fold: traceback-family keywords (traceback/stacktrace/keyerror/assertionerror), blank runs ≥3 fold, fanout_detail grouping-key doc+test.
• MCP token nudges: core profile, terser heavy-tool helps #609 token-nudges: get "full" restored, patch/write relpath examples, --json help made truthful for the pre-Row-budget crush for updates/list_refhosts/openqa_overview #610 tree (limit-capped array + limit hint, zero truncation claims); hunk yields to Row-budget crush for updates/list_refhosts/openqa_overview #610's exact text at land time.
• Row-budget crush for updates/list_refhosts/openqa_overview #610 json-crush: severity-ordered anomalies (u8, K/L kept counts in notice), window-before-probe in list_refhosts with pre-window totals in notice/footer.
• Collapse exact testreport_read re-reads to a notice #611 reread-dedup: additive deduped bool on read output (true on collapse, false on file text), documented; notice text unchanged.
• fix(mcp,testreport): stop steering transport unlock failures at --force #618 unlock-force: two-foreign-owners cancel test; ContendedSurface enum replaces paired check/scope strings, byte-identical rendering.
• feat(mcp): durable audit record of tool calls #620 audit-log+OTLP: real Debug negative pins, OTLP config_set body test, Full-via-dispatch test, always-fingerprint put/write/patch payloads as {bytes,sha256} (sha2, already in lock), resolve gated on auditing(), atomic 0600 create + tightening pre-existing sinks.
• fix(mcp): filter config_show on the headless surface #621 config-show: reuses pub sanitize_url (local dup deleted), edge-case parity tests, const subset assertions + url-defaults coverage test.

One systemic thread across #610/#611/#620: new I/O or truncation reuses the project primitives (spawn_blocking for blocking I/O, in-band truncation notices) — applied in each.

[mimi1vx]

Blocker (P1)

• crates/mtui-mcp/src/testreport_tools.rs:454: _scope lock dropped prematurely before read I/O begins. In testreport_read, let _scope = session.scoped_lock(template).await; is declared inside the let target = { ... }; block despite the comment stating // The gate scope is held for the whole call, the inner mutex only for the path resolution. _scope is dropped as soon as target resolves, releasing gate.shared() and the per-RRID lock before resolve_relpath_async, stream_read, or dedup run. All sibling tools (testreport_patch, testreport_write, testreport_fill, testreport_logs) declare let _scope outside the inner block. Move let _scope = session.scoped_lock(template).await; outside and before let target = { ... };.

Blocker (P2)

• crates/mtui-mcp/src/testreport_tools.rs:480, 495: Consolidate sequential spawn_blocking hops. testreport_read performs up to three separate spawn_blocking hops (resolve_relpath_async, stream_read, canonicalize_for_key_async). stream_read can canonicalize the path during its execution and return (StreamRead, PathBuf), eliminating the separate canonicalize_for_key_async hop.

[mimi1vx]

Blockers

• [P1] docs/src/mcp.md:306-317 — still documents the pre-PR testreport_read contract; doesn't mention the new force param or deduped response field. Please update before merge since this is a user-facing MCP reference and the CHANGELOG calls this an additive schema change.
• [P2] No test exercises the _scope lock's hold duration (only its downstream effects). Consistent with sibling tools in this file, so not a regression — just noting the gap.

All four numbered blockers from the prior four review rounds (force/no_cache bypass, dedup key canonicalization on the default path, canonicalize() inside spawn_blocking, lock held across the whole call) are verified resolved, including against the macOS symlink case.

[mimi1vx]

Reviewed at head 7b2a109 (full diff + changed files). Traced the cache-poisoning-relevant paths specifically: the ambiguous-template and path-escaping checks in resolve_path/safe_template_file both return via ? before the RereadKey/hash are ever constructed, so the cache is not reachable from either. FNV-1a64 constants/order are correct. Cache lives on McpSession — per-client, no cross-session leakage. 16-entry LRU bound and eviction are correct and tested at the exact boundary. deduped field is additive-only, documented, and the schema snapshot was regenerated to match. The "never suppresses an edit" tests are genuine red/green tests (line_count-only and hash-only cases both covered).

No blockers, no Priority items. Approving.