fix(engine): round-9/10 review follow-ups for #101

[asto18089]

No-Issue: review follow-up batch for #101; no tracking issue

Summary

Follow-up to #101 (merged as 6ba52af35). Round 9 landed the findings actionable on the post-merge base (480bebf61, after #102/#104): two real defects, two mutation-verified pins, one truthful-prose cleanup. Round 10 (a fresh full-tree re-review by thirteen scoped reviewers) adds seven more commits: it finishes the disabled-bounds prose true-up that #104's rebase half-dropped (fd14b6a7e's comments landed, its code change did not), removes the per-activation full-cache re-serialization the disabled bounds make pure waste, closes the metrics reader/writer disconnect that left #101's executed-marker rollup inert, and restores the bounds-independent child ride-channel pins #104 deleted while a comment claimed they were kept. The remaining round-10 findings are dispositioned at the bottom.

Round-9 changes

1. fix(cli): read flat tool-result audit receipts — round-9 MAJOR. The codewhale metrics tool.result rollup consulted only the nested details/payload placements, but the only in-repo writer of tool.result lines — the engine's opt-in tool-audit log (CODEWHALE_TOOL_AUDIT_LOG, writing the executed marker introduced by fix(kimi): repair empty items instead of dropping #101's item 25) — emits tool_name/success/executed at the top level of the line. Every receipt from that writer bucketed as an executed success: a refused oversize hydration inflated the very success count the marker exists to protect. The reader now falls back to the top-level placement, and an unexecuted receipt counts as neither success nor failure whatever its success flag says (the old else if !success contradicted the neither-contract for a receipt that never ran). The pin drives the flat shape and the failed-unexecuted combination; both directions mutation-checked (deleting the fallback or the executed guard turns red).
2. test(engine): pin the mid-turn refresh admission — round-9 MAJOR residue. The round-9 mutation deleted the entire mid-turn mcp_catalog_changed arm and left the whole suite green: the only test driving it asserted deny-side outcomes that hold vacuously when nothing arrives (and feat(tools): disable the deferred-toolbox size bounds #104's bounds rewrite removed the two e2e pins without replacing the positive coverage). The test now also drives the arm's positive side: under a permissive policy the pool's synthetic login tool must enter the catalog carrying the pool's own definition and join the active set. Mutation-verified: arm-deleted → red.
3. fix(engine): declare refresh content drift — round-9 m2. The refresh declaration keyed on the model-visible name set, so a same-name refresh serving a changed description or schema spliced new wire bytes into place and declared nothing — undeclared drift, which the C5 prefix guard treats as a real bug (debug panic, release cache miss). Pre-existing class (fix(kimi): repair empty items instead of dropping #101 fixed renames; content slipped through), now closed: when the name set ties, each served tool's wire JSON — the exact bytes the C5 fingerprint hashes, via the shared tool_to_api_json — is compared against the replaced entry's, and any difference declares. The unit phase pins both directions (byte-identical stays silent; a description change declares; the splice keeps the slot).
4. test(kimi): pin the anyOf branch-order ship path — round-9 m2. The terminator stops at the first terminating branch, so minItems beside an empty items fails closed in an anyOf only when the offending branch is reached before a terminating one; a terminating branch ahead of it ships the repaired, instance-set-equivalent schema (the desired outcome). The round-8 pin exercised offending-branch-first only; the ship path is now pinned so the order dependence is stated in the fixtures instead of discovered, and the terminator's minItems fallback carries its call-order invariant comment.
5. fix(tools): truthful child errors and step docs — round-9 nits. The child's ride rebuilt the error with a bare display, silently dropping the source chain ({error:#} now renders it); the Step trigger's doc names the child request-rebuild and warm-start parks it also covers.

Round-10 changes

6. fix(tools): finish the disabled-bounds prose true-up — round-10 minor, four independent reviewers. feat(tools): disable the deferred-toolbox size bounds #104's follow-up fd14b6a7e was half-lost to its rebase over fix(kimi): repair empty items instead of dropping #101: the comment claiming the oversize reason "claims no bound and interpolates no numbers" landed, but fix(kimi): repair empty items instead of dropping #101 had refactored the inline format! into the shared toolbox_oversize_reason(), so the code still interpolates the constants — under the fork's usize::MAX / 2 sentinels any render would read "(4611686018427387903 cached tools, 9007199254740991KiB …)". The reason and the eviction-note capacity label now gate on the sentinel: disabled bounds render bound-free prose ("The serialized tool definition could not be measured for admission to the conversation toolbox."), real bounds keep the numbers (upstream parity). The search-context recovery hint rides only a real budget rejection — a measurement failure is not fixed by a narrower query. The child's hydrate refusal picks its cause truthfully (catalog-absent / not-deferred / budget) instead of blaming the budget for every rejection, and no longer says "bounded child toolbox" — a claim feat(tools): disable the deferred-toolbox size bounds #104's commit message made but never landed. The stale "bounded (8 cached tools, 16KiB)" comment at the eviction block and the revalidate/struct/constant docs now state the fork posture.
7. perf(tools): skip the bounds pop loop under the disabled sentinel — round-10 minor, three independent reviewers. pop_collective_overflow's loop condition is permanently false under the sentinel, but evaluating it re-serialized every cached tool via serde_json::to_vec — once per revalidate and once per requested name in activate, i.e. O(cache × requested) full-cache serializations per search/hydration batch, growing with the unbounded cache for the conversation's lifetime. A const-gated early return skips it; behavior is unchanged in every reachable state (the only non-real trigger, the serde-failure measurement sentinel, is removed as stale by revalidation before the loop runs).
8. test(engine): pin premises against the initial_active_tools fallback — round-10 minor. Two tool-search tests asserted active.contains(name) after the first search, but initial_active_tools' empty-scan fallback had already inserted that name before any search ran — the asserts were vacuous, and a mutant whose search never activates into the cache stayed green. Both tests now assert the cache population directly. The cross-note dedup scenario of the render test also pinned only the mention count, not which note survives; it now pins first-note-wins attribution. New forkguard_toolbox_prose_claims_no_bound_under_the_disabled_sentinel pins the round-10 prose verbatim (red against the pre-fix code, which interpolated the sentinel numbers).
9. test(subagent): restore the bounds-independent ride-channel pins — round-10 minor. feat(tools): disable the deferred-toolbox size bounds #104 deleted all ten child-surface eviction tests as bounds-triggered, but two were bounds-independent by construction (they park notes directly) — and the file's comment claims exactly that class "is kept". The parent engine kept its direct-park pins (hydration_eviction_note_rides_the_batch_first_result); the child lost its only glue-level coverage of the round-2 defect shape (the child discarding its reports). Both tests are restored and pass unchanged against the current tree; the round-11 wave chains the error fixture (Err("boom").context("outer")) so the {error:#} source-chain rendering is pinned too.
10. test(engine): pin the hydration audit marker on the arm that fires — round-10 minor. refusal_audit_payload_carries_the_executed_marker sized its fixture against the 16 KiB budget feat(tools): disable the deferred-toolbox size bounds #104 removed, so it silently took the Loaded arm while its name and docstring said Refused. Renamed and re-documented for the Loaded arm it actually drives; the Refused arm is unreachable in-fork and says so.
11. fix(cli): read the opt-in tool audit log — round-10 MAJOR. The rollup read only ~/.deepseek/audit.log, but the only in-repo tool.result writer is the engine's opt-in log at $CODEWHALE_TOOL_AUDIT_LOG/$DEEPSEEK_TOOL_AUDIT_LOG, which has no default path — so fix(kimi): repair empty items instead of dropping #101's executed consult and round-9's flat fallback were inert unless a user happened to point the variable at the very file metrics reads. codewhale metrics now also reads the opt-in path when set (deduplicated against the default; the writer's precedence is mirrored). The reader comment claiming "the sensitive audit log nests them under details" was false — that log has no tool.result call sites — and now says so. The restricted-turn audit projection no longer strips success/executed: they are non-private booleans, and the reader's missing-flag default of true was bucketing every restricted receipt, failures included, as an executed success. Pinned by the extended redaction test and the new audit_log_paths unit tests.
12. docs(tool-search): round-10 truthfulness pass — replace_runtime_mcp_tools' docstring claimed a same-count refresh never moves wire bytes; round-9's content compare makes a same-name content change declare, and the docstring now says so. The arrivals comment justified eager arrival only by the 401-auth case; it now states the bulk post-login arrival truth. TOOL_SURFACE.md: "serialized schemas" → "serialized tool definitions" (the gate measures the whole serialized Tool), the bounds-eviction reporting paragraph is marked inert in this fork, and the catalog-shrink absorption claim is corrected — a search's revalidation reports a cached-but-departed name regardless of the query, and a shrink first observed by any non-search path (turn boundary, hydration batch, executed-tool refresh, retrieval admission, child request rebuild) is absorbed silently. The schema walker doc names the fail-closed keywords that need no visit, and two previously construction-only repair positions (root-level object-form additionalProperties, $ref with an empty-items sibling) are now fixtured.

Round-11 changes

Round-11 (a fresh thirteen-lane re-review plus eight in-tree mutations; findings comment posted 2026-10-14) added five commits addressing its three MAJORs and five of its minors:

13. fix(engine): scope content drift to the wire — the content compare declared drift for deferred, never-activated tools whose changed definitions never ship; the compare now filters to the wire-visible set (when the visible sets tie, every visible-after name was visible before, so the compare stays exact). Pinned red-green: wire-invisible drift stays silent (red against the unfiltered compare), visible drift still declares.
14. test(subagent): pin the error-chain ride rebuild — the {error:#} fix had shipped with only a chainless-error test that could not fail; the fixture is now chained (Err("boom").context("outer")) and asserts both levels render. Reverting to a bare display turns it red (mutation-checked).
15. fix(tools): drop the sentinel note's bound claim — the sentinel-branch eviction note still said "bounded conversation toolbox" in exactly the state where the bound is disabled; the word is dropped (real bounds keep it), the forkguard literal updated, an explicit no-"bounded" assert added, and the netting count assert re-anchored.
16. fix(cli): audit-source docs and undated pins — the flat fixtures carried a ts the writer never emits; they now mirror the writer, and the since-filter test pins the real consequence (an undatable line is never excluded by --since). The module header now states the home resolution truthfully, including that the approvals/credentials writer resolves to CODEWHALE_HOME/~/.codewhale/audit.log while the reader base falls back to ~/.deepseek (pre-existing divergence, now disclosed instead of implied away).
17. docs(tool-search): round-11 truthfulness fixes — the absorption sentence said "only a shrink first observed at a turn boundary is absorbed silently"; every non-search first observer absorbs silently (the hydration path's own comment says so), and the doc now says that. Also the last "cached schemas" wording, the fail-closed keyword list (propertyNames/unevaluated*/nested allOf added; dependentSchemas noted as stripped rather than rejected), and activate's doc naming the serde sentinel as the sole remaining admission rejection.

Round-11 minors deliberately not taken: the symlinked-path dedup edge and the empty-string env-var divergence (both contrived, no in-repo producer), and the hydrate refusal cause arms' production unreachability (dead-but-truthful defensive prose, kept as reviewed).

Verification

• Round-9: each pin mutation-verified red; cargo test -p codewhale-tui --lib 12024/0; cargo test -p codewhale-cli --lib 351/0; fmt and clippy clean.

• Round-10: the restored child pins and every other new/changed test verified green individually (parked_note_rides: 2/0; child_parked_note_rides: 1/0; hydration_receipt_audit: 1/0; forkguard_toolbox_prose: 1/0; forkguard_restricted_tool_audit_redacts_private_payload: 1/0); the prose forkguard is red by construction against the pre-fix code (it asserts the exact bound-free strings the old code did not produce). cargo test -p codewhale-tui --lib --locked: 12027 passed / 0 failed / 13 ignored (RUST_MIN_STACK=16777216; one earlier full-suite run flaked a single unrelated test under parallelism — the same class prior rounds disclosed — and two full reruns are green). cargo test -p codewhale-cli --lib --locked: 354 passed / 0 failed. cargo fmt --all -- --check clean; cargo clippy --workspace --all-features --locked with the fork-CI allow set clean; Cargo.lock unchanged.

• Round-11: both new pins mutation-verified red-green (unfiltered compare → red; bare-display rebuild → red); targeted tests green (runtime_mcp_refresh_replaces_the_pool_slice, child_parked_note_rides…, forkguard_toolbox_prose…, toolbox_note_rendering…, audit_log_since_filter, tool_result_rollup…); cargo test -p codewhale-tui --lib --locked: three runs 12025-12026 passed with 1-2 failures per run, each in a module this PR does not touch and green in isolation (client retry keys, session export copy, pdf first-line — the same parallelism class round-10 disclosed); cargo test -p codewhale-cli --lib --locked: 354/0 twice; fmt and clippy (fork-CI allow set) clean.

Round-9 findings dispositioned without code changes

• m1 (a delivered "unrecoverable" note can be falsified by a same-turn re-arrival): the window requires a bounds eviction to seed the note, and feat(tools): disable the deferred-toolbox size bounds #104 disabled the size bounds in this fork — the interleaving is currently unreachable here (the machinery stays for upstream parity). A correction-note design for the general case is a deliberate change worth its own discussion; left out of this mechanical follow-up.
• Round-9 items already moot on this base: the two vacuous e2e premise-assert pins (feat(tools): disable the deferred-toolbox size bounds #104 removed the bounds-posture tests this finding was about — their substance is re-delivered positively by change 2), the touch_glue missing-premise-assert (test removed by feat(tools): disable the deferred-toolbox size bounds #104), and the session-constant "schemas" comments (feat(tools): disable the deferred-toolbox size bounds #104 rewrote them).
• Deferred as disclosed in the round-9 review: sibling metrics sources (read_session_files/read_runtime_events) still bucket unexecuted receipts as successes; annotation-only items still fails closed; a duplicate name inside a refreshed list would duplicate the catalog entry (structurally unreachable — the pool dedups); the doc's recoverability partition ignores per-tool oversize growth between park and ride.

Round-10 findings dispositioned without code changes

• Mid-turn refresh arrivals stay eager (two reviewers): a never-seen refreshed name joins the active set with the pool's eager flag, so a 50-tool server's post-login set lands on the wire at once, bypassing the deferral discipline the turn build and boot window apply. Deliberate and test-pinned today; the comment now states the bulk-arrival truth. Routing arrivals through apply_mcp_tool_deferral with an authenticate carve-out is a real design change (wire-shape and prefix-cache implications), worth its own PR, not a mechanical follow-up.
• The mid-turn mcp_catalog_changed arm lacks the boot-window refresh's security/feature gate (active_turn_tool_security / Feature::Mcp early-out). Surface-consistency divergence only — execution stays exact-dispatch gated, so no privilege escalation — and the producer tools cannot execute under the policies that make the gate vacuous. Mirroring the gate is a behavior change that deserves its own pinned test; deferred.
• Residual silence-to-model on genuinely unrepairable schemas: a tool whose schema still fails MFJS after repair is dropped from the wire with only a user-visible warning; the engine's catalog and active set keep it, so a model that knows the name can call it, the engine executes it, and the history then references a function absent from tools — a weaker remainder of fix(kimi): repair empty items instead of dropping #101's incident class. The fix is a cross-layer (client → engine) reporting channel, a design change of its own.
• Pre-existing client-layer nits: the tool-projection warning re-fires identically on every request with no dedup; the forced-choice refusal prints the wire-encoded tool name (my--tool). Both predate fix(kimi): repair empty items instead of dropping #101.
• Err-arm audit asymmetry: the tool.result Err arm carries no executed key, so pre-dispatch failures (policy denial, cancellation, unknown tool) bucket as failures while unexecuted Ok-arm receipts bucket as neither. Semantics-only; classifying pre-dispatch error kinds is a design choice, deferred.
• Flat receipts carry no ts, so --since cannot exclude them and they do not move the period window (pre-existing writer shape). Mistyped-field shadowing in the rollup reader (a non-bool in a higher-precedence placement blocks a valid bool below it) — no in-repo writer produces it; defensive only.
• Bounds-parity notes (unreachable in-fork under feat(tools): disable the deferred-toolbox size bounds #104): a re-parked note is dropped by the next turn-entry sweep if the turn ends resultless; cross-note dedup attributes a twice-evicted name to the first note (now pinned as first-wins); touch_cached_tool_after_execution reports changed: true even for a pure LRU touch (pre-existing); the child touch gates on Result::is_ok rather than ToolResult.success (pre-existing); the warm-start overflow park is doubly dead (production passes no warm names). All worth an upstream note, none reachable here.
• Departed refreshed names stay in the activation cache until the next turn's revalidation (a same-turn depart-and-return loses the deferral marking and re-arrives eager; declared to the wire, so C5-safe). Hygiene item, inert in-fork.
• pending_prefix_change_reason overwrite vs first-set-wins in the results phase: cosmetic attribution-label precision only; both reasons are declared, so the C5 guard is satisfied either way.
• Round-9 disposition record correction: the round-9 deferral said read_runtime_events still buckets unexecuted receipts as successes — in fact its tool.* arm matches no production event name at all (the runtime store emits item.*/turn.*), so it buckets nothing; the deferral described intent, not a live path. The read_session_files half is confirmed and runs deeper: session persistence drops the executed marker (ContentBlock::ToolResult has no field for it), so no reader-side fix is possible without a session-format change.
• Whole-PR design note: fix(kimi): repair empty items instead of dropping #101's eviction-reporting apparatus (~450 production lines) exists to make the 8-name/16 KiB LRU bound honest; feat(tools): disable the deferred-toolbox size bounds #104 — landed the next day, citing the same incident class — disables that bound in this fork. The reporting stays as upstream-parity machinery and is now marked as such in TOOL_SURFACE.md; the fork's durable value from fix(kimi): repair empty items instead of dropping #101 is the empty-items repair plus the truthfulness fixes.

[asto18089]

Round-11 fresh review — 13 scoped lanes + 8 in-tree mutations

Base state: head a23f65a6f is 13/13 commits linear on the current origin/pinvou3-clean tip 480bebf61 — no rebase needed. Smuggle sweep: every hunk of every commit maps to its commit message and the body's 12 changes; the file set is exactly the 13 claimed files (no CI/lockfile/submodule movement; Cargo.lock byte-identical).

Verdict: 3 MAJOR findings, all in the PR's own truthfulness domain. The engineering underneath is sound — the metrics fixes are real and root-caused, the content-drift declare closes a genuine C5 hole, the pop-loop skip is proven behavior-preserving, and every claimed mutation pin reproduced red under my own mutations. Nothing here is smuggled, nothing reinvents the wheel.

MAJOR

M1. TOOL_SURFACE.md's new absorption sentence is false — and regresses the text it replaced.
docs/TOOL_SURFACE.md:134-137 now says "Only a shrink first observed at a turn boundary is absorbed silently." Four other first observers absorb silently too: the parent hydration batch (turn_loop.rs:4282-4315 — parks only activation.evicted, and its own comment at :4306-4311 states the absorption outright), the executed-tool refresh (tool_catalog.rs:619-635), retrieval admission (tool_catalog.rs:641-663), and the child request-rebuild (subagent/mod.rs:14202-14203). The pre-PR text ("Unavailable drops observed by non-search paths are absorbed silently … a catalog-shrink drop left the catalog entirely and no later search can name it") was accurate on this point; the rewrite narrows it to turn boundaries only. Truthful form: a shrink is reported only when a search is the first observer; every other first observer — turn boundary included — absorbs it silently. (The newly-added search-first reporting claim itself verified accurate: every search revalidates before admitting — session.rs:294 via tool_catalog.rs:1601-1602 — and stale names surface in unavailable references regardless of query match.)

M2. The sentinel-branch eviction note still claims a "bounded conversation toolbox" — and the new forkguard test pins that self-contradiction verbatim.
tool_catalog.rs:493: under the disabled sentinel the note reads "evicted tools from the bounded conversation toolbox" — the toolbox is unbounded in exactly the state this branch renders for. The PR's own test doc (tool_catalog/tests.rs:649-652) states the standard — "a bound that never fires is a false statement about the runtime" — and the test then asserts the exact "bounded" string at :669-673. Dead prose only (the note is unreachable in-fork: every caller gates on a bounds-eviction list the pop-loop early return always empties), but this commit is titled "finish the disabled-bounds prose true-up"; dropping "bounded" from the sentinel branch and the pinned literal completes it. (:430's doc header uses the same phrase but describes the general concept — :493 is the one that renders.)

M3. The body's claim that the restored ride tests "also cover #105's {error:#} error-ride fix, which had shipped without a test" is not true — the fix remains unpinned.
subagent/tests.rs:7706-7717 rides Err(anyhow::anyhow!("boom")) — a chainless error for which {error} and {error:#} render identically. Mutation-verified (M8 below): reverting subagent/mod.rs:14171 back to {error} leaves the suite green. The fix itself is real and correct (alternate Display renders the source chain; real chains reach this path via execute_full, mod.rs:15772) — it just has no test that can fail. Please pin it with a chained fixture (e.g. anyhow!("boom").context("outer")) asserting both levels appear in the ridden message, or drop the body claim.

MINOR

4. replace_runtime_mcp_tools's content compare over-declares for wire-invisible tools. Snapshot and compare filter on universe.contains only (turn_loop.rs:1094-1101, :1137-1148), so a description change on a deferred, never-activated pool tool returns true while zero wire bytes move. Downstream is benign — check() returns Stable on fingerprint equality before consulting the declared reason (prefix_cache.rs:569-571), so no re-pin or history entry — but the docstring's "splices new wire bytes and declares" (:1070-1072) is overstated for this case. Filtering both sides to the wire-visible set closes it.
5. The metrics flat fixtures pin a fictional shape: every new flat line carries "ts" (metrics.rs:1119-1123), which the writer never emits (turn_loop.rs:5295-5305). The disclosed consequence — ts-less receipts are never excluded by --since (metrics.rs:302-307 falls through) — has no test either. Dropping ts from the flat fixtures (matching the writer) and/or adding a ts-less-line-under---since pin would cover both.
6. The metrics header this PR edited still misdescribes the default source: metrics.rs:5-9 claims ~/.deepseek/audit.log holds "(approvals, credentials)", but that writer (audit.rs:63-74) resolves to $CODEWHALE_HOME/~/.codewhale/audit.log, and the reader base (deepseek_home(), metrics.rs:872-882) disagrees with it in two of four env states (default and DEEPSEEK_HOME-only). Pre-existing — but the PR edited the line directly above it.
7. Hydrate refusal cause arms: None/not-deferred are production-unreachable — the sole caller proves membership and deferred before calling (subagent/mod.rs:15743-15766) — so the old "only rejection here is the per-tool budget" comment was true on the production path. The new arms are dead-but-truthful defensive prose; they do fix the direct-call/test path (tests.rs:7571-7582) where the old message mislabeled a policy denial. Fine to keep; the commit-message framing ("blaming the budget for every rejection") overstates reachability.
8. The fail-closed keyword list (schema_sanitize.rs:1307-1311) reads exhaustive but isn't: propertyNames, unevaluatedProperties, unevaluatedItems, and nested allOf also fail closed unlisted, and dependentSchemas is stripped (strip_dependent_keywords, :416-435) rather than failing closed — one exception to the stated mechanism. Mark the list representative or extend it.
9. TOOL_SURFACE.md:87 still says "re-serialize cached schemas" — the exact schemas→tool-definitions wording this PR claims to have swept.
10. session.rs:287-288 ("every request naming a deferred catalog member is admitted") and tool_catalog.rs:433-436 ("the only reachable trigger is the serde-failure sentinel") tell two stories; both hold in practice (the sentinel is unreachable for the concrete Tool type) but deserve one cross-reference.

NIT

11. audit_log_paths dedup is exact-path equality only — a symlinked or relatively-spelled opt-in path equal to the default double-counts (contrived; no in-repo writer puts tool.result in the default file).
12. Empty-string env var: the writer logs one tracing::error! per event, the reader silently skips; also the home vars normalize empty→unset while the tool-audit vars don't.
13. Body's "4503599627370495KiB" is half the real render (usize::MAX/2 bytes ≈ 9007199254740991 KiB); cosmetic.
14. Body abbreviates forkguard_restricted_tool_audit_redacts_private_payload to forkguard_restricted_tool_audit.
15. turn_loop.rs:1089-1091 "the exact bytes the fingerprint hashes" — under strict_tool_mode the fingerprint hashes sanitized copies (tool_catalog.rs:871-884); raw-identical ⇒ sanitized-identical so no false negative, but a one-line caveat would prevent future confusion.

Verified clean (coverage record)

• Usefulness / root cause: before this PR the metrics tool.result rollup was genuinely inert — repo-wide, the only in-repo writer is the engine's opt-in log (flat fields, no default path), verified against every emit_tool_audit* call site. The flat fallback + executed guard + opt-in path read make #101's marker actually count, and the restricted-projection fix prevents misbucketing restricted failures as executed successes (reader defaults both flags to true). The content-drift declare closes a real C5 hole: the end-of-loop backstop keys on active-set inequality and catalog length only (turn_loop.rs:5470-5477), so same-name content drift was undeclared before, and the new compare reuses tool_to_api_json — the exact serializer the fingerprint hashes — making it faithful by construction.
• pop_collective_overflow early return: proven behavior-preserving. Both callers (revalidate tail, session.rs:274; the activate loop, :317, after revalidate at :294) run the retain that strips serde-failure sentinels first; no other mutation path can admit a sentinel (push_back gated by exceeds_per_tool_budget, :309); the guard is an AND of exact equality on both constants, so re-enabling either bound revives the loop. The perf claim is real: Θ((k+1)·n) full serializations per activate-of-k-names over n cached entries, gone.
• Mutations I ran (edit → targeted test → restore): M1 drop flat tool_name fallback → metrics pin RED; M2 revert the executed-guard → RED; M3 delete the content-compare (and snapshot) → refresh pin RED; M4 disable the mid-turn arm → policy test RED; M5 drop the sentinel prose branch → forkguard RED; M6 drop the projection flags → redaction pin RED; M8 revert {error:#} → GREEN (finding M3's evidence); M7 drop the pop early-return → full suite green apart from parallelism flakes (the compaction-family failures it surfaced pass 63/63 in isolation on the mutated tree — timing under load, not behavior).
• Gates: codewhale-cli lib 354/0; fmt clean; clippy (fork-CI allow set) clean; Cargo.lock unchanged; codewhale-tui lib full suite 12023-12026 passed per run with 1-4 failures per run — every one in files this PR does not touch and green 3/3 in isolation, matching the disclosed parallelism-flake class (client::operation_tests::forkguard_model_operation_auxiliary_retry_keys, remote_control::tests::separate_predispatch_crashes_on_one_run_get_distinct_recovery_turn_ids, and the compaction family under mutation load).
• Pin-quality audit: every new/changed pin is load-bearing. The mid-turn positive side is the only assert set that catches arm-deletion (the deny side holds vacuously — as the test's own comment honestly admits); the cache.names().count() premise pins are the sole red line in the eager-promotion test; the first-note-wins attribution asserts match real production park ordering; the half-lost-rebase story (fd14b6a7e's code change absent at base while its comment claimed "interpolates no numbers") verified against the base tree.

Disposition requested: please address M1-M3 before merge — M1 and M2 are one-file edits each, M3 is a chained test fixture or a body correction. 4-10 as you see fit; happy to re-review the delta.

[asto18089]

Round-11 fix wave — all three MAJORs and five of the minors are addressed in 69f2fb73d..4d71d0ca3 (five commits, fast-forward on the same base; gates below). Reply-threaded per finding.

• M1 (absorption sentence) → 4d71d0ca3 (docs(tool-search): round-11 truthfulness fixes). The doc now says a shrink is silent under every non-search first observer — turn boundary, hydration batch, executed-tool refresh, retrieval admission, child request rebuild — matching what the hydration path's own comment (turn_loop.rs's hydration park) already stated. The body's change-12 paragraph carried the same false sentence and is corrected too.
• M2 (residual "bounded" claim) → a238eca5d (fix(tools): drop the sentinel note's bound claim). The sentinel branch no longer says "bounded conversation toolbox" (the real-bounds branch keeps it — correct there); the forkguard literal is updated and gains an explicit !note.contains("bounded") assert; the netting count assert in toolbox_note_rendering… that matched the phrase is re-anchored.
• M3 ({error:#} unpinned + body claim) → 0587a13c5 (test(subagent): pin the error-chain ride rebuild). The fixture is now Err(anyhow::anyhow!("boom")).context("outer") and asserts both levels render in the ridden message. Mutation-checked: reverting subagent/mod.rs to a bare {error} drops the "boom" half and turns the test red. The body claim is rewritten to match.
• Minor 4 (over-declare for wire-invisible tools) → 69f2fb73d (fix(engine): scope content drift to the wire). Both the membership tie and the content compare now use the wire-visible set; since tying visible sets means every visible-after name was visible before, the compare stays exact (no missed declarations). Red-green pinned: the new block in runtime_mcp_refresh_replaces_the_pool_slice drives a deferred, never-activated tool whose description changes and asserts silence — it is red against the unfiltered compare. The strict-mode caveat from NIT 15 is now a comment at the compare.
• Minor 5 (fictional ts fixtures + no --since pin) → 41805a63b (fix(cli): audit-source docs and undated pins). Flat fixtures carry no ts (the writer's shape), and audit_log_since_filter now includes an undated flat line asserting it counts past the cutoff.
• Minor 6 (stale header) → same commit. The header states the home-resolution chain and discloses the pre-existing approvals/credentials divergence (CODEWHALE_HOME/~/.codewhale/audit.log writer vs ~/.deepseek reader fallback) instead of implying the default file is read.
• Minor 8 (keyword list), Minor 9 ("cached schemas"), Minor 10 (two-story docs) → 4d71d0ca3: the fail-closed list gains propertyNames/unevaluatedProperties/unevaluatedItems/nested allOf and notes dependentSchemas is stripped rather than rejected; the last "cached schemas" wording is fixed; activate's doc names the serde sentinel as the sole remaining admission rejection.

Not taken, as offered in the review: NIT 11 (symlinked-path dedup) and NIT 12 (empty-string env divergence) — both contrived with no in-repo producer; and the hydrate refusal arms' dead-but-truthful prose (minor 7) is kept as reviewed. Body KiB figure (NIT 13) and test-name abbreviation (NIT 14) corrected in the body edit.

Verification (4d71d0ca3): both new pins mutation-verified red-green (unfiltered compare → red; bare-display rebuild → red); targeted tests green across every touched area; cargo test -p codewhale-tui --lib --locked three runs — 12025-12026 passed each, with 1-2 failures per run all in untouched modules and green in isolation (client retry keys, session export copy, pdf first-line; the disclosed parallelism class); cargo test -p codewhale-cli --lib --locked 354/0 twice; cargo fmt --all -- --check clean; cargo clippy --workspace --all-features --locked with the fork-CI allow set clean; Cargo.lock unchanged.

[asto18089]

Fresh from-scratch re-review (no reliance on earlier rounds): 12 scoped review lanes covering all 18 commits, plus independent verification: fmt, clippy under the fork-CI allow set, cargo test -p codewhale-cli --lib (354/354), three full codewhale-tui --lib runs, a base-branch suite run for the flake question, and four hand-reproduced mutations. The branch is based on the current pinvou3-clean head (480bebf61); no rebase was needed.

Verdict: APPROVE (recorded as a comment review because GitHub forbids self-approval — PR author == reviewer account). All four review questions pass:

1. Useful, complete, root-caused. Every load-bearing claim verified true against the code. The metrics reader was genuinely inert on base (the only in-repo tool.result writer emits top-level fields to an env-var path; the reader consulted nested placements at a default path it is never written to), and the else if executed change makes the code match its own documented neither-contract. Same-name content drift really went undeclared on base — both the name-set compare and the later backstop are blind to it, and the C5 guard does debug-panic / release cache-miss on undeclared drift. The half-lost-fd14b6a7e story checks out (base carries its comments yet still interpolates the sentinel numbers into model-facing prose). The pop-loop waste arithmetic is right (1+|requested| full-cache serialization passes per activate), and the early return is behavior-preserving: I removed it and every session/search/toolbox test stays green. All thirteen "dispositioned without code changes" claims were audited individually; every operative claim is true (two narrative imprecisions below).

2. Elegant. No wheel reinvention: the drift compare hashes exactly the bytes the C5 fingerprint hashes (tool_to_api_json), the child refusal-cause match mirrors the parent's structure, the prose gating routes through the shared helpers, and the strict-mode over-declare-only reasoning is correct (deterministic sanitize; declared-but-stable prefixes are confirmed no-ops). One structural nit: the disabled-sentinel gate is hand-repeated 4× across tool_catalog.rs/session.rs; a shared const fn would be marginally better (today any drift is caught by the forkguard pins, not silent).

3. No smuggling. The 18 commits map 1:1 onto body items 1–17 (6d9aee9 subsumed by item 12); Cargo.lock untouched; no version bumps, debug remnants, or whitespace churn; the engine.rs comment-only hunk is covered by item 12 and its claim is accurate.

4. No defects found. fmt clean; clippy clean under the fork-CI allow set (the one lint that fires without the flags, collapsible_if at turn_loop.rs:5368, is pre-existing #101 code explicitly allow-listed in fork-ci.yml). Full-suite flakes across three runs (2/3/6 per run) are all in remote_control/pdf — untouched by this PR, green in isolation, and the base branch flakes identically, confirming the disclosed environmental parallelism class. Mutation reproductions, all matching the body's claims: flat-fallback removal → red; wire-visible-filter removal → red on exactly the deferred-drift pin; bare-display rebuild → red (chain collapses "outer: boom"→"outer"); pop-early-return removal → green everywhere.

P3 notes for the record (none blocking):

1. Body precision: round-10 "adds seven more commits" — it adds eight, and 6d9aee9 is never named while item 12's backticked subject (a23f65a6f) does not touch schema_sanitize.rs; the title's "round-9/10" omits the five round-11 commits; three backticked commit subjects don't match the actual subjects verbatim; "landed the next day" — #104 landed ~2.5 h after #101, same day.
2. metrics: audit_log_paths dedup is exact-path equality, so a symlink/.. alias of the default would double-count (contrived, honestly dispositioned; fs::canonicalize would close it); the failure-arm comment "Only a receipt whose tool actually ran can be a failure" overclaims — the Err arm emits no executed, so pre-dispatch denies bucket as failures (pre-existing, disclosed as a deferred design item).
3. The child-surface park doc (subagent/mod.rs:14140) still says "combine" — the exact wording this PR corrects everywhere else.
4. Coverage advisories: the active-set insertion half of search admission (tool_catalog.rs:1609-1611) is unpinned while the cache half now is; the admission pin asserts definition equality via description only where a whole-struct compare (Tool: PartialEq) is available; #104's third still-green deleted test (hydrate_dispatch_loads…) remains unrestored, leaving the hydrate-dispatch glue edge unpinned — fine to scope, worth a follow-up.
5. TOOL_SURFACE: the "any other first observer" list omits the child's direct-call hydration (reads as exhaustive); the oversize re-report branch is dead in-fork just like the bounds channel but carries no inertness marker; root-not is stripped rather than failed-closed (the doc lists it under fail-closed; nested not does fail closed — safe either way).

Merge-ready from my side.

[JensenChen28]

Reviewed 4d71d0c against the PR base 480bebf. No new blocking defect found. I checked the metrics reader against the actual opt-in audit writer and restricted projection, the same-name MCP definition comparison against the prefix serializer and its caller, the disabled-cache-bound early return against admission/revalidation ordering, and the child error/refusal changes and regression fixtures.

Independent local validation: cargo fmt --all -- --check and diff whitespace checks passed. A standalone Rust harness ran the complete unchanged metrics.rs module with the real codewhale-paths dependency, plus extracted production refresh/serialization/audit-projection functions and Tool types: test result: ok. 21 passed; 0 failed. The refresh cases covered eager, activated deferred and invisible deferred definitions, schema changes and strict flags. Only the cache membership adapter was substituted in those refresh tests; this does not exercise the full MCP runtime. Removing the flat tool-name fallback made the existing receipt regression fail (0 passed; 1 failed); restoring it returned all 21 tests to green.

All required checks pass on this head. I inspected workflow 38537185007: workspace lint, workspace tests, documentation tests, formatting and lockfile-drift validation succeeded. I did not rerun the full TUI/native suites or live provider/MCP services locally. The documented undated-log --since behavior and exact-path-only audit-log dedup remain limitations; this review does not claim those are fixed.

The parent checkout remains on its existing gitlink. This approval covers the foundation PR; parent adoption and validation against the latest combined maintenance tree remain separate integration steps.