Repository navigation
fix(kimi): repair empty items instead of dropping - #101
Conversation
|
CI triage — which failures belong to this PR None of them do, on current evidence:
Happy to help split a follow-up that re-scopes or fixes the |
asto18089
left a comment
There was a problem hiding this comment.
Fresh full re-review of #101 (6 parallel review agents + personal verification; head 1cbcc5fa2, base tip 0bc868ff0 == merge-base, so no rebase was needed).
What verifies clean: the root cause and its chain are real end-to-end — tool_search advertises deferred tools regardless of MFJS validity (tool_catalog.rs:767/795), the activation cache admits by name with no schema validation (session.rs:99), and only the Moonshot wire build drops the tool with a stderr warn + UI-only ToolProjectionWarning that never reaches the model (chat.rs:1118 → chat.rs:990, turn_loop.rs:5421/6862). The browser catalog claim is accurate: browser-core-protocol.mjs:151 ships exactly one empty schema node (args: { type: 'array', items: {} }). The repair is semantics-preserving and traverses exactly the five schema-value positions the MFJS validator can reach at that pipeline stage (oneOf cannot survive to the pass — non-root is converted by normalize_kimi_child_schemas:1403-1408, root composition is stripped unconditionally by sanitize_for_responses). No smuggled changes; both commits split cleanly with DCO; changelog correctly untouched; both new tests are genuinely red-green (mutation-verified below); suites reproduce the body's claims (schema_sanitize 61 passed / 0 failed, tool_catalog 31 passed / 0 failed); fmt clean; no perf risk (one extra O(nodes) walk among ~6 on the request-build path; eviction reporting clones ≤8 strings).
MAJOR
- M1 — the eviction-honesty goal is only realized on the search path; two same-class paths remain fully silent.
activate_result_dependencies(tool_catalog.rs:463-464): everyfetch_url/web_run/truncateresult carryingevidence_available: trueadmitsretrieve_tool_resultand can LRU-evict a recently working tool with no model-visible note anywhere. Hydration admission (turn_loop.rs:4030-4038, childsubagent/mod.rs:14150-14152): a direct call to a remembered deferred name can silently evict another tool. Both are exactly the failure mode the body says it fixes ("a tool that executed successfully earlier in the same conversation silently became uncallable"). Ask: either extend the reporting to these paths, or disclose the boundary in the PR body and file the follow-up.
MINOR
- m1 — walker order misses closely-related shapes (
schema_sanitize.rs:1311-1340): check-first + earlyreturnleaves (a){"type":"array","items":{"items":{}}}unrepaired (the inner node empties to{}after the outer check already ran → stillMissingType), and (b) a root carryingitems:{}plus a repairable$defsentry unrepaired ($defsis validated at:1625before the root keyword check). Children-first recursion without the earlyreturn(the house style ofsanitize_for_kimi:1113) fixes both. Fail-closed today, not a regression — but the same class as the incident shape. - m2 —
toolbox_notecan mis-attribute the cause (session.rs:104-107):activate()seedsdelta.evictedwithrevalidate()output, so a search after a catalog shrink (MCP disconnect / deny rule) reports those names under "evicted … by activating newer matches … until a later search re-activates them" — wrong cause, and a false recovery contract since the tool is gone from the catalog. Split the buckets or reword. - m3 — PR body accuracy: the companion
Pinvou/pinvou-agent#683exists and cross-references this PR, but it (a) does not pin the gitlink (deferred to a later PR per its own body) and (b) was opened ~48 minutes before this PR, not after. Please correct the closing line. - m4 — test gaps: no test drives a 16KiB-byte-bound eviction into the reporting (only the 8-name path); the
metadata["toolbox_note"]clone line and the "8 cached tools" / "by activating newer matches" fragments are unpinned; and no wire-level test shows the incident shape being retained end-to-end (moonshot_stream_emits_one_projection_warningcovers only the drop path, with a different error).
NIT
- n1: hardcoded "8 cached tools" (
tool_catalog.rs:1419) — matches the pre-existing hardcoded rejection prose at:1391, but both drift ifTOOL_ACTIVATION_CACHE_MAX_NAMESchanges; a const-derived format would be safer. - n2: the walker visits
anyOfonly; safe today becauseoneOfcannot survive to the pass, but one comment sentence would defuse the trap for future maintainers (sanitize_for_kimiwalks both). - n3: the "must be idempotent" doc justification states a requirement nothing exercises (the pass runs once per request on a fresh clone).
- n4:
evicted_tool_referencesreuses the callabletool_referencetype;unavailable_tool_reference+ an evictionreasonwould be better typed — taste call. - n5: optional one-line
docs/TOOL_SURFACE.md("Conversation toolbox cache") addition noting evictions are now reported to the model.
Verification receipts (all mutations reverted; worktree pristine): repair call disabled → kimi_parameters_repairs_empty_items_as_unconstrained FAILED (MissingType); payload block disabled → eviction test FAILED (payload Null vs Array); metadata block disabled → FAILED (metadata Null vs Array); name list removed from the note → FAILED; deleting the early return → suites stay green (confirms m1 is unpinned). Clippy note: local clippy reports 7 pre-existing errors in files this PR does not touch (js_execution.rs, shell_dispatcher.rs, web_search.rs, engine/tests.rs) — toolchain/flag noise, not a PR regression.
Recommendation: the PR is useful, root-caused, and correctly layered — but as scoped it makes evictions non-silent on only one of several paths. Please address M1 (extend or explicitly disclose + follow-up) and the m1/m2/m3 small fixes; the rest is optional polish.
|
Fix wave pushed (8558e6c + 1c33184) addressing the fresh-review findings (review comment 5486698768): M1 — eviction honesty now covers the non-search paths. m1 — walker order. m2 — cause accuracy. A search after a catalog shrink now reports the dropped activation under m3 — body corrected (companion pinvou-agent#683 neither pins the gitlink nor postdates this PR; both fixed in the body). m4 — test gaps closed: Verification receipts: |
asto18089
left a comment
There was a problem hiding this comment.
Fresh round-2 review (0 BLOCKER / 5 MAJOR / 5 MINOR)
8 fresh review lanes over the full diff, every MAJOR re-verified by hand against the code before reporting. Rebase: head 1c331846ef already sits on the latest pinjou3-clean (0bc868ff0) — no-op. Local gates re-run on the pristine head: cargo fmt --check clean, clippy (CI flag set) clean on codewhale-tui, schema_sanitize 62/0, tool_catalog 33/0, full lib suite green, CI green on this head.
What holds up (verified, not taken from the body): the two root-cause fixes are real and well-built. The empty-items repair is semantics-preserving against the Moonshot MFJS dialect — removal can only turn already-invalid documents valid (an items: {} value failed validation on every pre-PR path), tuple/boolean items and untyped empties still fail closed, the children-first walker unrolls arbitrary items-in-items nesting, blast radius is provably Moonshot-only, and the repair is structural serde_json work, not string surgery. Red-green claims re-verified empirically: with the repair disabled, both new kimi tests and the wire test fail at the expected asserts. Commit hygiene is clean: every hunk maps to one of the four claimed fixes, exactly 8 files, no lockfile/config churn. The search-side eviction reporting is honest within its own bucket: stale and evicted are provably disjoint, the note's re-search promise holds for every name in it, the capacity label derives from the session constants, and the no-eviction payload is byte-identical to base.
But the "report every toolbox eviction" half has five verified problems — two of them mean the new reporting can put false statements in front of the model, which is worse than the silence this PR exists to fix.
MAJOR 1 — an eager-promoted tool is reported as gone while its schema ships in the same request
revalidate drops any cached name whose entry is no longer defer_loading == Some(true) into stale (session.rs:81-91, catalog_tool requires the deferred flag), and the search reports every stale name under TOOLBOX_STALE_EVICTION_REASON (tool_catalog.rs:1447-1453): "no longer available in the tool catalog; searching cannot re-activate it". But a tool promoted to eager (e.g. a mid-conversation tools_always_load change — the cache is only cleared on Op::SyncSession, engine.rs:3408) is deliberately kept active by is_eager_now (tool_catalog.rs:412-425, whose own doc comment describes exactly this flow) and its schema ships in the head of the same request's tools array. So the search result tells the model the tool is uncataloged and unrevivable while that same request carries the tool, directly callable with no search at all — the payload contradicts itself. Introduced by this PR (base reported nothing here). The existing cache_eviction_does_not_hide_a_tool_that_became_eager (tests.rs:130) pins the retention half but never the search payload, so the false report is untested. Fix: share the eager predicate with the reporting site and skip eager names from the stale report.
MAJOR 2 — revalidate's own bounds pops are misfiled into stale
The bounds loop (session.rs:92-100) pushes its victims into the same Vec as the retain-drops, and activate files the whole return into delta.stale (session.rs:129). For those names the reason string is false on both counts: they are still deferred catalog members (they passed retain), and a search can re-admit them — the correct bucket is bounds-evicted with the re-search note. Reachable when a cached member's serialized schema grows mid-conversation (an MCP refresh shipping a larger schema for the same name; replace_runtime_mcp_tools/boot-refresh paths don't revalidate the cache), pushing the total past 16KiB — the len > 8 arm is dead post-retain, so byte growth is the only trigger. Side effect: on non-search paths these names are silently removed from active (the stale-handling drop) with no note at all — exactly the "tool silently vanished" failure this PR hunts.
MAJOR 3 — the "post-execution refresh" reporting channel is dead code, so commit 4's claim and docs/TOOL_SURFACE.md overstate
touch_cached_tool_after_execution guards on the name already being cached (tool_catalog.rs:462-471). Inside activate, a cached-name re-admission is remove+push-back — membership and byte total are unchanged after the just-run revalidate enforced both bounds — so the eviction loop cannot fire: touch.evicted is provably always empty, both on the main engine (turn_loop.rs:4955-4962) and in the child (subagent/mod.rs:15633-15638, which discards the value outright). The genuine refresh-path eviction scenario is MAJOR 2's revalidate pops, which land in stale and are dropped unreported on this path. So "bounds evictions from … the post-execution LRU refresh — now reach the model too" (commit message) and "…otherwise appended to the triggering tool result (hydration, retrieval activation, or a refresh of an executed tool)" (docs/TOOL_SURFACE.md:84-86) are not true as shipped. The honest fix falls out of MAJOR 2: split revalidate's return (genuine staleness vs bounds pops) and report its bounds pops through the refresh channel — then the doc sentence becomes true and the dead wiring comes alive.
MAJOR 4 — the four non-search reporting channels have zero test coverage
bounded_toolbox_eviction_note / pending_toolbox_eviction_note appear in no test file; ToolSurfaceUpdate.evicted is never asserted anywhere (only .changed); the set→take→attach chain (turn_loop.rs:4061 → 4934 → 5051-5066, including the error branch at 5105-5111) and the child receipt suffix (subagent/mod.rs:14163-14175, exercised only via is_ok()) are unreached by tests. Deleting the take, the error-branch attach, or the child note suffix leaves all 12k lib tests green — the PR's second headline behavior is currently pinned by review, not by CI.
MAJOR 5 — pending_toolbox_eviction_note survives Op::SyncSession
engine.rs:3403-3408 clears tool_activation_cache under the comment "never carry the previous conversation's toolbox across this edge", but the note introduced by this PR is not cleared: one setter (turn_loop.rs:4061), one taker (turn_loop.rs:4934), no other clear point. A turn dropped between set and take (abort/shutdown mid-batch) leaves the note on the session, and the next conversation's first tool batch appends the previous conversation's eviction prose to one of its results. One-line fix next to the cache clear.
MINORs
- An oversize-grown cached tool can yield two contradictory
unavailable_tool_referencesentries in one search:retaindrops it intostale(reason claims it left the catalog — it didn't) while discovery re-rejects it intocache_rejectedwith the truthful oversize reason (tool_catalog.rs:1437-1453). docs/TOOL_SURFACE.md's "surfaced as unavailable references instead" holds only via a latertool_search; hydration stale drops are deliberately unreported and turn-start stale drops (e.g. disconnect between turns) never surface. Worth a scoping clause.- Turn-start
revalidate(turn_loop.rs:1332) and the child's per-requestrevalidate(subagent/mod.rs:14127) produce unreported evictions today — naturally covered once the MAJOR 2 split lands. - Remaining non-empty-items MFJS failure families still drop the tool with nothing model-visible (~14 variants; disclosed as "fail closed" in the body, but the search-advertises-yet-uncallable trap stays open for them). Fine to leave for a follow-up.
schema_sanitize.rs:2584test comment narrates development history ("used to leave$defsunrepaired") instead of stating the invariant;TOOLBOX_STALE_EVICTION_REASONcan be private.
Verdict: the kimi schema-repair family (commits 1, 2 and the wire test) is merge-quality as is. The eviction-reporting family needs the revalidate split + eager filter (MAJOR 1-3, one coherent fix), the SyncSession note clear (MAJOR 5), and channel tests (MAJOR 4) before the "report every toolbox eviction" claim is true. Happy to push the fix wave if that's the preferred path.
asto18089
left a comment
There was a problem hiding this comment.
Fix wave pushed as 2bb381d74 addressing all five MAJORs from the round-2 review above:
- MAJOR 1+2+3 (one coherent fix):
revalidatenow classifies every cache removal by recoverability — catalog-shrink and oversize carry per-cause truthful reasons (the oversize reason is now shared with discovery rejections and derived from the session constants, emitted once instead of twice), eager promotions are forgotten without a report (both vocabularies were false statements about a directly-callable tool), and collective-bounds pops land in the re-activatableevictedbucket. With the split in place the post-execution refresh channel comes alive:touch_cached_tool_after_executiongenuinely evicts when a cached schema grew past the budget, and the note rides the refreshed result — the previously provably-dead wiring is now reachable and pinned. - MAJOR 4: the four non-search note channels are now pinned at the engine level (hydration first result, leading error, retrieval receipt, execution refresh), plus the child hydration receipt in the subagent surface. Every classification pin was hand-mutation-verified red against the pre-fix code before turning green.
- MAJOR 5:
Op::SyncSessionclearspending_toolbox_eviction_notenext to the cache clear, pinned by a live-loop test.
Also folded in: the note's "by activating other tools" clause is gone (the trigger is not always an admission now), remove_evicted_cache_activations' dead eager exemption became the single remove_toolbox_drops helper across all call sites, and docs/TOOL_SURFACE.md states the contract as implemented including the eager carve-out. MINORs on the remaining non-empty-items MFJS families and turn-start/child-revalidate silent stale drops remain open follow-ups by design (the doc now scopes them instead of overclaiming).
Validation on 2bb381d74: fmt clean; full lib 12036 passed / 0 failed (RUST_MIN_STACK=32M locally); PR checks green on CI.
asto18089
left a comment
There was a problem hiding this comment.
@/tmp/cw101-review-r3.md
|
@/tmp/cw101-fixwave-r3.md |
asto18089
left a comment
There was a problem hiding this comment.
Round-4 fresh review (full re-review from scratch, no carryover)
13 fresh review agents over head 7afa8bf (correctness per commit, root cause, docs/body honesty, smuggling, performance, invariants, elegance, plus two mutation-testing agents). Rebase check: pinjou3-clean is still 0bc868ff0, so the branch is already on the latest base and no rebase was needed.
Gates on head 7afa8bf: cargo fmt --all -- --check clean; fork-CI clippy flags clean; cargo test -p codewhale-tui --lib = 12044 passed / 0 failed / 13 ignored on this machine, excluding one environmental flake (model_inventory::tests::ollama_default_prefers_live_local_tags_over_the_unresolved_marker fails under full-suite parallelism here, passes in isolation, and the file is untouched by this PR).
Verdict
The PR remains genuinely useful and correctly root-caused, the kimi repair is semantics-preserving, the reporting machinery is heavily and (mostly) mutation-hardly pinned, and the smuggling audit is clean. But the round-3 netting has one remaining hole: the parked-note channels can still name a tool that the very result carrying the note has just re-admitted, which falsifies the contract sentence this PR adds. Two MAJORs below; both are small, well-localized fixes.
MAJOR-1 — parked notes are frozen prose; netting does not cross the park→attach boundary
docs/TOOL_SURFACE.md (added by baad584): "An eviction never names a tool that the same result re-admits", and body item 6 claims "every note can quote the list verbatim without claiming a tool is uncallable in the very result that returns it". The netting enforcing this lives only inside one ToolActivationDelta (session.rs:243). The parked note is already-fused prose and is appended verbatim at ride time (turn_loop.rs:4954, 5072-5086 success, 5136-5139 error; child mirror subagent/mod.rs:15671-15675) with no re-check against what the riding result re-admits.
Reachable on head, traced end-to-end:
- An MCP refresh grows cached tool
X's schema between turns; turn-start revalidation bounds-evictsXand parks the note (turn_loop.rs:1338-1347);Xleavesactive_tool_names. - The model's first batch directly calls
X— the natural retry, i.e. exactly the incident behavior this PR fixes.maybe_hydrate_requested_deferred_toolpasses (deferred, inactive, under the per-tool cap), the receipt "ToolXwas deferred and has now been loaded. Retry the call…" is returned,plan_tool_callsre-admitsX(turn_loop.rs:4040-4043), and the receipt becomes a recorded outcome (turn_loop.rs:4259-4276). A first-batchtool_searchmatchingXreaches the same contradiction throughtool_references. process_tool_resultsappends the parked note to that receipt: one payload now saysX"has now been loaded. Retry the call" and "the evicted tools are no longer callable until a later search re-activates them: X".
A model that resolves the contradiction toward the note re-searches a tool it is already holding — a soft, self-inflicted recurrence of the motivating bug. The tests pin each half (turn_start_revalidation_parks_an_eviction_note_for_the_turn parks for a resultless turn; search_does_not_name_its_own_admission_as_evicted pins only within-delta netting) but not the combination.
Suggested shape: park structured names (trigger + Vec<String>) and net/format at attach time against the admissions of the result being ridden — the hydration set is known at plan time and a search outcome carries its admitted references. That is round-3's netting extended across the park/attach boundary; it also removes the stringly-typed trigger.
MAJOR-2 — TOOL_SURFACE.md: "Every bounds-driven eviction is reported to the model" is a false universal
The code deliberately does not report four classes: a stranded note is dropped at the next turn's entry sweep (turn_loop.rs:1329-1334; reachable with a plain turn-start eviction and a text-only turn — the pinned sweep test itself demonstrates the strand), a re-parked note dies at turn end (5155-5162), SyncSession clears at the conversation edge (engine.rs:3410-3411), and a child's un-ridden note dies with the surface. Each is disclosed in code comments and body item 7 — but the doc sentence asserts the absolute the implementation does not meet. Same family as the round-3 "Every eviction reported" MAJOR this PR fixed, now overcorrected into a quantifier the sweep paths falsify. One scoping sentence fixes it.
MINOR
- m1 — oversize-refusal side channels still say "loaded". At the live call site,
should_emit_hydration_statusis computed beforemaybe_hydrate_requested_deferred_tool(turn_loop.rs:3917-3918) from set membership the refusal never joins, so the refusal branch still pushes the oversize name intodeferred_tools_hydrated_in_order(3932), emits thetool.schema_hydratedaudit event (3934-3943), and sends the user status "Loaded deferred tool 'X'. Retry the call with its visible schema." (3944-3954) — for a tool that was explicitly not loaded. The model-facing receipt is now truthful, but the UI/audit channels lie, and the new comment attool_catalog.rs:1131-1132("the batch activation is never asked to admit it") is false at this site: the name is in the hydration order andactivaterejects it (inert, but the stated invariant doesn't hold). - m2 — child
hydrateErr path absorbs revalidation evictions.hydrate()runscache.activatebefore the oversize verdict (subagent/mod.rs:14204); on the not-admitted path (14230-14233) the activation'sbounds_evictedvanish — no note, no park. The parent refuses up-front before any activation (tool_catalog.rs:1127-1135). Narrow co-occurrence (collective overflow + oversize request), but this is the function commit 8 touched. - m3 — child parked note rides
Okonly (subagent/mod.rs:15671) while the parent attaches to error results too (turn_loop.rs:5136-5139); the doc says "the child's next recorded result". On an error result the note only delays (becomes a loss if the child ends on text). Either attach onErrlike the parent or scope the sentence. - m4 — mutation-proven pin gaps (hand-applied mutants this round): (a) deleting the object-form
additionalPropertiesvisit inremove_empty_mfjs_items_schemasleaves all 62schema_sanitizetests and all moonshot wire tests green — that position (and theanyOf-branch visit, same shape) is unpinned; (b) removing only theSyncSessionnote-clear (engine.rs:3411) stays green — the turn-entry sweep masks it, only the composed mutant is caught; (c) swapping the sweep (:1334) and park (:1347) at turn entry passes all four new tests — the ordering is unpinned.
NIT (sample)
bounded_toolbox_eviction_note's doc still lists triggers("search", "step")— the turn-start caller passes"turn's start"(tool_catalog.rs:444-445vsturn_loop.rs:1347).session.rs:296-301field doc describes only the hydration park; turn-start now parks there too.- Body wording: the
minItems-beside-empty-items case now failsInvalidSchemaNode, notMissingType(same drop outcome, different class than "the same error classes as before"); and "only atracing::warn!" — the streaming path also raises a user-visible toast viaomitted_tool_names(the load-bearing "nothing the model can see" holds). - Refusal prose doubles the phrase: "could not enter the bounded conversation toolbox: The tool schema exceeds the bounded conversation toolbox (…)".
- The warm-start park in
SubAgentToolSurface::new(14128-14131) is production-dead: the only non-test construction passes&[](11604). park/combineis byte-identical parent/child (turn_loop.rs:4919-4929vssubagent/mod.rs:14136-14145) — one shared helper would pin the mirror;remove_empty_mfjs_items_schemasis the third hand-rolled child-visitor inschema_sanitize.rs(a shared child-schema visitor is worth it on the next touch, not this PR).- Placement: the Kimi wire test rides the tool-search commit (disclosed in both commit message and body; would bisect better beside the kimi commits), and
process_tool_results'pub(super)widening (turn_loop.rs:4912) is claimed nowhere.
What checked out
- Kimi repair: walker ordering claims verified against the real pipeline (
sanitize_for_responses→normalize_kimi_compatibility→sanitize_for_kimi→ repair → validate); every unvisited schema position fails closed under a pre-existing error class (UnsupportedKeyword/InvalidSchemaNode); removals are semantics-preserving under both draft-07 and 2020-12; single wire path (prepare_outbound_requestis the only seam); no new recursion hazard. M1/M2/M3/M5 mutants all killed with diagnostic-accurate failures. - Root cause: the admit-vs-wire divergence is real (engine-side admission has no MFJS check), the disclosure of residual fail-closed classes matches the code, and provider parity is clean — no sibling provider silently drops (xAI/Anthropic/Responses degrade to notes, DeepSeek degrades strict mode).
- Reporting machinery: legacy shape preserved on no-eviction searches (pinned), stale/bounds classification complete with no escape path, oversize emitted exactly once, shared budget gate used by all three callers, capacity label derived from real constants, byte-budget arithmetic sound (strict
>, saturating, re-serialized each check — no double-count), LRU front/back discipline verified at every touch point, sweep-before-park ordering correct in current code, re-park overwrite safe, note survives result truncation on both parent and child paths, and 7 of 8 reporting pins each kill their targeted mutant. - Smuggling: all 10 commits map to body claims; every one of the ~105 deletions accounted for; no Cargo.lock/version/fingerprint churn; commit conventions clean.
Recommendation
Fix MAJOR-1 (park structured names + attach-time netting) and MAJOR-2 (one scoping sentence) before merge. m1–m3 are small and fit the same wave; the m4 pins are three cheap test additions. Everything else can land as-is.
|
Round-4 fix wave pushed (7afa8bf..1834654, 6 commits), addressing every actionable finding from the round-4 review:
Mutation verification this round: 7/7 hand-applied mutants killed by the new pins (netting ×3 tests, hydrate-rejection evictions, Ok-only ride, refused-arm announcement, park-before-sweep, both walker visits). Gates on Not addressed, deliberately: the warm-start park in |
Moonshot MFJS validation fails any empty schema node with MissingType,
and sanitize_moonshot_chat_tools answers by silently excluding the whole
tool from the request — the model only sees a tool_search reference it
can never call and keeps emitting the nearest advertised tool instead.
JSON Schema reads an empty items schema as unconstrained items, which is
exactly the MFJS-legal shape with the keyword absent, so the kimi
parameters sanitizer now removes the empty keyword instead of the tool.
Untyped nodes without a semantics-preserving equivalent (empty property
schemas) still fail closed.
Found in the wild: the Pinvou browser MCP advertised
evaluate_script.args = {"type": "array", "items": {}}, so Kimi
requests never carried the tool no matter how often it was activated.
Signed-off-by: asto <asto18089@126.com>
The bounded activation cache (8 cached tools, 16KiB of serialized schemas) evicts least-recently-used entries when a search admits new tools, and the evicted tools silently vanished from the model's function list between turns — a tool that executed successfully earlier in the same conversation became uncallable with no explanation. The search result now names the evictions (evicted_tool_references plus a toolbox_note stating the recovery contract); searches without evictions keep the legacy result shape unchanged. Signed-off-by: asto <asto18089@126.com>
The empty-items walker checked the node's own items before recursing and
returned immediately after removing, which stranded two same-class shapes
in the old fail-closed behavior: {"items": {"items": {}}} (the inner
repair empties the outer node after its check already ran, so the tool
still died with MissingType) and a root items keyword beside $defs (the
$defs entries were never visited, even though validation reaches them
before the root keyword check). Children now run first and the removal is
a plain last step: {"items": X} is vacuously true for non-arrays and an
empty items schema accepts everything, so dropping the outer keyword once
the inner repair emptied it stays semantics-preserving. The idempotency
sentence in the doc comment stated a requirement nothing exercises and is
replaced by the real invariants (pipeline position and the oneOf
conversion upstream).
Signed-off-by: asto <asto18089@126.com>
The search-result eviction note was accurate only when the eviction came from the admission bounds loop, and several same-class evictions never reached the model at all — exactly the "a tool that executed successfully earlier silently became uncallable" failure this PR exists to close. ToolActivationDelta now separates stale entries (dropped by revalidate: disconnect, deny rule, eager promotion, oversize) from bounds evictions. A search reports stale tools under unavailable_tool_references with a catalog-shrink reason instead of the eviction note, whose "re-search to re-activate" contract would be a false promise for them. Bounds evictions from the three non-search activations — a batch's hydration of directly-called deferred tools, retrieve_tool_result admission on an evidence receipt, and the post-execution LRU refresh — now reach the model too: the hydration note rides the first recorded result of the batch (or its error), retrieval and refresh notes ride the triggering result, and a child hydration receipt carries its own note. The capacity label is derived from the session constants so the prose cannot drift from the real bounds. TOOL_SURFACE.md documents the reporting channels. New tests: byte-bound eviction reporting, stale-vs-bounds classification, the metadata toolbox_note copy, and a wire-level check that the wild-found browser evaluate_script shape is retained on a Kimi request. Signed-off-by: asto <asto18089@126.com>
Round-2 fresh review found the stale/bounds classification lying to the model in two reachable ways, and the "report every toolbox eviction" claim resting on a dead channel. Classify every cache removal by recoverability at the source, so the reporting cannot emit a false statement. revalidate now returns ToolActivationRemovals instead of a flat name list. Retain drops carry a cause: CatalogShrink (disconnect, deny rule, mode switch) or Oversize (schema outgrew the per-tool budget). A tool promoted to eager is forgotten without a report — it is directly callable, so both vocabularies would be false statements about it. The collective-bounds loop pops land in bounds_evicted: those tools are still deferred catalog members, so a later search re-activates them. The search payload reports each stale entry with its truthful reason — the catalog-shrink text, or the shared oversize reason that discovery rejections already used, now derived from the session constants — and a name dropped as oversize is emitted once instead of twice. With the split in place the post-execution refresh channel becomes live: touch_cached_tool_after_execution can evict when a cached schema grew past the budget since its last activation, and the note rides the refreshed result; previously that field was provably always empty while the growth-path evictions landed in stale and were dropped unreported. The eviction note drops its "by activating other tools" clause, since the trigger is not always an admission now. Op::SyncSession clears pending_toolbox_eviction_note next to the activation cache, so a note parked by an interrupted turn cannot ride a result of the next conversation. remove_evicted_cache_activations becomes remove_toolbox_drops: with eager promotions classified out at the source, its eager exemption was dead, and one helper now covers both buckets at every call site. Tests pin the eager carve-out in the search payload, growth overflows as re-activatable evictions rather than unavailable references, the oversize reason emitted exactly once, the four non-search note channels at the engine level (hydration first result, leading error, retrieval receipt, execution refresh), the child hydration receipt in the subagent surface, and SyncSession dropping a parked note. Every classification pin was verified red against the pre-fix classification via hand-applied mutations. docs/TOOL_SURFACE.md states the reporting contract as implemented, including the eager carve-out and the silent absorption of unavailable drops on non-search paths. Signed-off-by: asto <asto18089@126.com>
A revalidation pop of the very tool an activation re-admits (the searched tool was the LRU-front victim of a growth overflow) is not an eviction: the result naming it evicted also hands it back in tool_references, and the note's recovery contract would be a false statement about it. Net admitted names out of delta.evicted so every reporting site can quote the list verbatim. Also make the per-tool budget gate shared (admission, revalidation, and hydration planning) and refuse an oversize direct-call hydration up front with the same reason the admission would give, instead of a "has now been loaded" receipt that turns every retry into another false promise. The catalog-shrink stale reason constant is renamed to say what it names. The hydration receipt refusal is covered by a new test; the netting by a search whose own target is the growth victim. Signed-off-by: asto <asto18089@126.com>
The turn-start revalidation is the fifth bounds-eviction trigger and the only one reported nowhere: when a cached schema grew between turns (an MCP refresh), the LRU victim silently vanished from the next request. Park its note like a hydration's so the turn's first recorded result carries it, combining with any note a hydration batch parks in the same turn. A note stranded by an interrupted turn or a panic is swept at turn entry instead of riding an unrelated later result, and a batch whose outcomes all flatten away re-parks its note for a later batch. The hydration site's comment promised that the next tool_search result names absorbed unavailable drops; that channel structurally cannot fire for a catalog-shrink drop (the absorbing path prunes the cache first, and discovery cannot match a name that left the catalog). The comment now states the split: an oversize drop is re-reported with its reason the next time a search matches the tool, a catalog-shrink one is learned only by the search missing. The success and error paths observed the working set on opposite sides of the eviction-note append; both now observe the pre-note result text. Pinned end-to-end through run_turn (parking, trigger attribution) plus the combine behavior; the SyncSession scan now asserts it saw a tool result so it cannot pass vacuously. Signed-off-by: asto <asto18089@126.com>
The child surface discarded every eviction report its helpers produced: the post-execution refresh dropped ToolSurfaceUpdate on the floor and request_tools discarded its revalidation entirely, so a child's toolbox could shrink with zero explanation while the identical parent event produced a note. Refresh evictions now ride the refreshed tool's result, and evictions observed outside any payload (a catalog refresh's revalidation, a warm start that overflows) park a note on the surface that rides the child's next recorded result — the parent engine's parked-note channel, mirrored. The hydrate refusal now states the shared oversize reason instead of "use tool_search with a narrower query", which no narrower query can satisfy. Pinned: refresh-note riding, refresh-parked note riding, and the load-receipt margin is documented in the existing test. Signed-off-by: asto <asto18089@126.com>
Restate the reporting paragraph to match the implementation: every bounds-driven eviction caused by an activation or a turn's start is reported, child agents report on their own channels, an eviction never names a tool the same result re-admits, and an unavailable drop observed by a non-search path is learned only through an oversize reason on a matching search or a bare miss — not by a later search naming it, which a catalog-shrink drop structurally cannot receive. Signed-off-by: asto <asto18089@126.com>
The turn-entry sweep and the resultless-batch re-park are behavioral changes this PR relies on; without these probes a regression deleting either line keeps the whole suite green. Signed-off-by: asto <asto18089@126.com>
A parked eviction note used to ride as prose frozen at park time. When the riding result re-admitted a named victim — the model retrying the tool a turn-start eviction just dropped, and the batch hydration loading it back before any result records — the payload declared the tool uncallable in the very result that handed it back, breaking the never-name-a-re-admitted-tool contract. Notes now park as structured names and are netted against the live activation cache when they attach, on the parent's success and error paths and on the child surface alike; a note emptied by the netting is dropped entirely. The trigger becomes an enum shared by both surfaces, whose park/combine logic collapses into one helper. Pinned by toolbox_note_rendering_nets_re_admitted_names_at_ride_time (unit) and turn_start_note_does_not_name_a_tool_the_result_re_admits (seam-level, red against the pre-fix attach). Signed-off-by: asto <asto18089@126.com>
Two child-side gaps survived the round-4 review. The parked-note ride attached only to Ok results, so a note whose child errored next stranded until a later Ok — or vanished when the child ended first; the parent engine rides error results too. And the hydrate rejection ran after the activation, so the revalidation evictions on its way were silently absorbed: the rejection error is that call's only result payload and now carries the note beside the shared oversize reason. The ride glue moves from the executor wrapper into a pinned surface method (ride_pending_eviction_notes) covering the Ok, Err, and fully-netted cases. Signed-off-by: asto <asto18089@126.com>
The oversize refusal fixed the model-facing receipt but left the side channels announcing a load: the batch still emitted the user-facing "Loaded deferred tool 'X'. Retry the call" status, audited tool.schema_hydrated, and pushed the refused name into the hydration order — asking the very activation the refusal exists to spare. The receipt is now a DeferredHydration::Refused arm, so the announcement, the audit, and the hydration-order push happen only on Loaded; the refusal prose also stops repeating the toolbox phrase the shared reason already states. plan_tool_calls widens to pub(super) (with its result struct) as the test seam for the new pin, matching the existing process_tool_results hook. Signed-off-by: asto <asto18089@126.com>
The walker's object-form additionalProperties and anyOf-branch visits were proven unpinned by round-4 mutation testing: deleting either block left the whole schema_sanitize module green. Fixtures for both positions now fail the tool with MissingType if a visit is dropped. Signed-off-by: asto <asto18089@126.com>
Round-4 mutation testing showed swapping the turn-entry sweep with the turn-start park passed every test — the ordering that keeps a stranded note from eating this turn's fresh eviction report was unpinned. The new pin strands a note, evicts at the turn's start, and requires exactly the fresh note to survive. The SyncSession clear's composed-only coverage is now stated in the session-sync test instead of being implicit. Signed-off-by: asto <asto18089@126.com>
"Every bounds-driven eviction is reported" overclaimed: the sweep, the session sync, and a child that ends first deliberately drop notes that never reached a result, as the code comments and the body disclose. The paragraph now states the best-effort contract, the drop edges, and the ride-time netting the previous round added, and the child channel gains the same "(or its error)" wording the parent already had. Signed-off-by: asto <asto18089@126.com>
1834654 to
133bdba
Compare
Round-5 fresh review (13-way, post-rebase)Rebase: rebased onto Verdict: no blockers; two test-coverage MAJORs (both verified by full-suite mutation runs this round), two actionable MINORs, rest is polish. The substance of the PR holds up under a fresh adversarial pass: the kimi repair is semantics-preserving and its walker visits exactly the five positions the MFJS validator recurses into (so no empty MAJOR (coverage, mutation-verified)M1 — the parent hydration-park call site survives deletion; "pinned transitively" overstates. M2 — the child's ride glue is unpinned. MINORm1 — the refusal receipt is missing m2 — ride-time netting consults cache membership only, so one window contradicts the doc's absolute promise. m3 (coverage polish) — in m4 (pre-existing, disclosed for completeness) — blocking Nits (at author's discretion)
What was verified clean this roundRoot-cause repair (semantics-preserving under both draft generations; boolean/tuple-form Recommendation: land one parent test through a real |
Ride-time netting consulted only the activation cache, so a name a mid-turn MCP catalog refresh re-advertised into the active set stayed in the note while the next request carried the tool — the note would call a callable tool uncallable, contradicting the toolbox contract. The parent and child ride sites now net against both the cache and the active-name set, and a unit pin covers the refresh window (round-5 review m2). Signed-off-by: asto <asto18089@126.com>
The oversize refusal receipt was a bare success result while the Loaded receipt sets executed:false — and the results phase defaults executed to true, so the working set observed a call that never happened and the audit logged the refusal as a successful execution. The refusal now carries the marker, pinned in the refusal test (round-5 review m1). Signed-off-by: asto <asto18089@126.com>
|
@/tmp/pr101-round8-fixwave.md |
There was a problem hiding this comment.
Round-9 fresh review (head 3281b4def, base pinvou3-clean @ 3118c36b8)
Rebase: no-op — merge-base(head, base) == base and neither ref moved since round 8; the tree under review is byte-identical to the round-8 fix head. 13 fresh lanes covered the full 40-commit diff (schema repair, catalog core, turn loop, session slot, child surface, all four test files, docs-vs-code, commit/body accountability, an end-to-end note-lifecycle trace, performance, engineering quality), with the load-bearing findings re-verified by hand and the headline one settled by a fresh mutation run.
Verdict: the two core fixes remain real and root-caused, and no smuggling or perf regression was found. Two MAJORs remain, both about claims this PR makes rather than the fixes themselves — one pin-hardening claim in the body is not delivered, and the metrics fix's two ends (writer and reader, both introduced by this PR) disagree on where executed lives. Both are cheap to fix.
Gates this round: cargo fmt --all -- --check clean; clippy (fork-CI flags, workspace --all-features --locked) clean; lockfile clean; schema_sanitize 64/0; tool_catalog 39/0; full lib 12133 / 0 / 13 (159 s, no flake this time). All count claims match the body exactly. GitHub CI on the head all green (gate, link, CodeQL ×4, DCO, Gitleaks, check).
MAJOR 1 — the round-8 premise-assert claim is not delivered: both e2e refresh pins stay green when the refresh never runs
The body's Testing section says the two e2e refresh pins "gain premise asserts requiring the pool slice to have actually been replaced (a 'refresh never runs' mutant previously left both green)". That red does not exist. The new premise assert in both tests is
assert!(
grown_catalog.iter().any(|tool| tool.name == "mcp_probe_authenticate"),
"premise: the refresh must have swapped the pool's slice in"
);(engine/tests.rs in step_note_names_a_victim_the_refresh_cannot_resurrect and refresh_between_outcomes_cannot_resurrect_an_evicted_tool), but both fixtures seed grown_catalog with a heavy_deferred_tool("mcp_probe_authenticate", …) before process_tool_results, and the slot splice replaces that entry with a same-named pool tool — so the name is present whether or not the refresh ran. Mutation-verified this round: if false && mcp_catalog_changed in the process_tool_results arm leaves both tests green, and in fact leaves all 86 refresh-named tests in the suite green — the unit pins drive replace_runtime_mcp_tools directly, so the arm itself has no end-to-end pin at all (even mid_turn_refresh_honors_the_turn_policy passes trivially under arm deletion, because its pool-side premise is checked before the action and its deny asserts hold vacuously when nothing arrives). The item-27 anti-resurrection behavior itself is genuinely pinned (!active.contains(victim) + attach-time netting turn the blind-resurrection regression red) — the defect is confined to the claimed premise asserts.
Suggested fix: assert the refreshed entry's content, not its name — e.g. that the entry now carries the pool's eager flag/schema (the way mid_turn_refresh_honors_the_turn_policy asserts pool.to_api_tools() contents), or that the splice froze post-slice positions; plus one positive post-refresh assert in an e2e pin so the arm itself is end-to-end pinned.
MAJOR 2 — item 32's neither-bucket is unreachable for the receipts this PR creates: the reader and the writer (both this PR) disagree on where executed lives
The reader (item 32, crates/cli/src/metrics.rs:338-342) consults only pointer("/details/executed").or_else(pointer("/payload/executed")). The writer (item 25, crates/tui/src/core/engine/turn_loop.rs:5220-5228) emits "executed": tool_was_executed at the top level of the tool.result audit line, into the env-gated CODEWHALE_TOOL_AUDIT_LOG (tool_execution.rs:193-200). codewhale metrics reads codewhale_home()/audit.log, which no shipped writer feeds with tool.result at all (census: log_sensitive_event emits only approval/sandbox/config events, nested under details). So:
- today, the tool.result rollup arm sees no
tool.resultlines from any in-repo producer — the item-32 fix changes nothing observable; - on the only configuration where it could (an operator pointing
CODEWHALE_TOOL_AUDIT_LOGat the file metrics reads), the top-levelexecutedmisses both pointers, defaults totrue, and the refused oversize hydration counts as a success — exactly the inflation item 32 claims to remove (same for a hydration receipt). Thesuccessfield has the identical mismatch, which is pre-existing at base, but this PR wrote both ends and never connected them; the item-37 pin validates adetails-nested fixture that nothing writes.
Mitigations, honestly: the disconnect is inherited (the arm was equally dead at base), the engine audit file is opt-in diagnostics, and the pin is honest about the shape it declares. But within one PR, item 25's writer and item 32's reader should agree.
Suggested fix: one line in the reader — add a top-level fallback (.or_else(|| v.get("executed"))) — and decide which file the tool.result arm is actually for.
m1 — a delivered "unrecoverable" note can be falsified by the same turn's own re-arrival
replace_runtime_mcp_tools builds previous_deferral from the current catalog (turn_loop.rs:1074-1078). Trace: a bounds-evicted deferred tool V is departed by a mid-turn needs-auth flip (removed from catalog + active set, :1107-1111), the ride renders "…has since left the toolbox catalog; a search can no longer re-activate it" (truthful at delivery, slot consumed), and then the same turn's login outcome refreshes the pool re-serving V — V has no previous_deferral entry, keeps the pool's eager flag, and re_advertise inserts it into the active set, so the next request of the same turn carries V on the wire while the transcript holds a promise that V can never come back. The item-27 invariant holds at delivery time (V was genuinely gone then); the violation is the forward-looking sentence, and the trigger needs the bounds-eviction + auth-flip + same-turn-recovery conjunction — hence m1, not MAJOR. Options: re-arrivals absent from previous_deferral arrive deferred rather than adopting the pool's eager flag, or delivered unrecoverable names are re-netted on arrival.
m2
- Body item 1's blanket fail-closed claim is branch-order dependent at
anyOf. The terminator propagates a branch error only if reached before a terminating branch (schema_sanitize.rs:1976-1985): a required property{"anyOf": [{"type": "string"}, {"type": "array", "items": {}, "minItems": 1}]}ships (first branch terminates), the byte-identical schema with branches swapped drops withInvalidSchemaNode. The round-8 pin (:2680-2697) exercises offending-branch-first only. The shipped form is the repaired, instance-set-equivalent schema, so this is claim/pin inexactness, not a wire bug — but "keeps failing closed wherever the terminator checks (… anyOf branches …)" is not universally true. touch_glue_rides_the_refresh_eviction_through_execute_from_surfacelacks the warm-pair premise assert its byte-tuned siblings carry (subagent/tests.rs:7913-7917vs7628-7634/7893-7900). Under serialized-size drift the constructor's own warm-start would park a note and the pin silently stops guarding the touch channel. Mitigated: the siblings share the tuning and their premise asserts fail first.- The wire-identity declaration keys on the name set; C5 fingerprints content. A same-name refresh with changed description/schema splices cleanly,
wire_visible_tool_namesis unchanged, nothing is declared — and the next request's content hash drifts undeclared (turn_loop.rs:1082,1113-1128vsprefix_cache.rs:714-728; debug assert / release cache-miss). Pre-existing class and this PR strictly narrows it (count-keyed base also missed renames and relocated bytes on every refresh), but item 34's sentence claims the class ("a count-preserving identity change … changed the wire while declaring nothing") and the new comment ("that identity — not the catalog's length — is what a declared re-pin answers for") states completeness a name-set key does not provide. A content digest, or declaring when the refreshed slice's serialized bytes differ from the replaced entries, would close it. - The neither-bucket contract doesn't match its own
else if. Comment and body sayexecuted: falsecounts as neither unconditionally;else if !success(metrics.rs:343-347) buckets anexecuted: false ∧ success: falsereceipt as a failure. Unreachable from current writers (bothexecuted:falsereceipts are successes); latent trap if a future writer ever marks a refusal as failed —else if executedwould match the documented contract.
m3
- Item 32's stated motivation still holds against the sibling metrics sources left untouched:
read_session_filesbuckets the persisted refusal receipt (a non-errortool_result) as a success, andread_runtime_eventscountstool.completedunconditionally (metrics.rs:519-528, 630-631) — session files are the dominant real-world source. - Annotation-only
items({"description": "…"}) constrains nothing yet still drops the whole tool — disclosed as fail-closed, but it is one annotation away from the original incident class. - The terminator's
minItems.as_u64().unwrap_or(0)treats an invalidminItemsas 0 (schema_sanitize.rs:2017) — safe only through the call-ordering invariant thatvalidate_mfjs_boundsrejects it earlier; nothing pins that ordering. - A duplicate name inside
refreshedwould duplicate the catalog entry while declaring no change (turn_loop.rs:1097-1099) — structurally unreachable today (the pool dedups by server-prefixed name), a defensive gap in the new splice contract. initial_active_tools' never-empty fallback can activate a deferredcatalog.first()without caching it; a refresh (which re-advertises byactivation_cache.containsonly) then de-wires it — requires an all-deferred catalog, contrived.- The doc's recoverability partition (catalog-left → unrecoverable, otherwise search promise) ignores per-tool oversize growth between park and ride: a still-cataloged name that grows past the per-tool budget before its note renders gets the search promise, which its recovery search will then refuse with the oversize reason. Self-correcting, but the stated binary is not exhaustive.
- Trigger mislabels: the child parks request-rebuild/warm-start evictions under
Step("This step evicted…") where the parent usesTurnStartfor the boundary; theToolboxEvictionTrigger::Stepdoc comment enumerates three kinds, the code uses five (session.rs:96-99, subagent/mod.rs:14131/14195). - Child-surface residue: the touch/hydrate appends bypass the unified render path (no
engine.tool_surfacedebug trace — the diagnosability item 33 promised; future netting/prose rules must be mirrored by hand); the ride's Err arm rebuilds the error and drops theanyhowchain (14163); item 19's "could never be silently dropped" is unreachable-by-construction defense no test can distinguish from assignment.
nits
Session const comments still say "schemas" where the bound measures the whole serialized Tool (session.rs:14,18 — the same drift item 39 fixed elsewhere); the child doc lists two channels that are defensive-only today; the neither-bucket is invisible in the rendered output and --json's successes silently changes meaning (no versioning); dead re-assignment in child_parked_note_rides_success_and_error_results… (subagent/tests.rs:7745); discovery-order assumption in the first eviction test (tool_catalog/tests.rs:361); item-36 title drift ("name the unrecoverable evictions"); one unclaimed comment-only hunk in 4628c9a2a (rewords a test comment schema_sanitize.rs — this PR's own comment, in scope, zero behavior); 16 of 40 subjects exceed 50 chars (this repo's convention doc sets no numeric cap — informational).
Verified sound (highlights)
- Smuggling: none. All 40 commits map onto the 39 body items in both directions;
--numstatexactly 12 files / +3980−197; every changed file serves the two stated purposes; DCO 40/40; merge-base == base; no cherry-pick/revert/fixup artifacts;git diff --checkclean and the 36 whitespace-delta lines are reindentation inside behavioral hunks. - The Kimi repair is sound at every enumerated position. The walker deletes only exact
{}items values, runs before validation, on the one shared seam every Moonshot request (streaming and not) passes through; the surviving-schema path cannot ship an MFJS-illegal shape (any residual empty node still fails validation); the fail-closed classes,minItemsposition matrix, single-element-allOfflattening, and the$defs/nested-repair claims all trace to code and pins;additionalProperties/not/oneOf/if-then-else/patternProperties/propertyNames/contains/unevaluatedItems/$refcycles enumerated and each is repaired, fail-closed, or excluded upstream — nothing silently passes. - The eviction-reporting machinery holds everywhere I could trace it. Every activate/revalidate call site is accounted for; netting covers cache ∪ active set ∪ cross-note dedup with the render consuming the slot; the splice preserves slots, deferral markings, and the provider KV-prefix (the round-8 prefix-bust fix is confirmed — and is a genuine performance win, since base relocated the slice and re-advertised everything eager on every refresh, busting the cached prefix each time); no reachable sequence loses an eviction outside the three documented drop edges, and no note names a callable tool at delivery time.
- Perf: no regression. Wire identity is a name set (no re-serialization); the new sanitize pass is one O(nodes) DFS joining an already multi-pass pipeline; per-tool serializations are pre-existing and marginally cheaper than base (eager promotions now skip serialization).
- Counts (64/39/12133/13) reproduce exactly; test-count reconciliation (39 = 23 module + 16 name-matching, 64 = 63 + 1) checks out statically.
Recommended before merge: fix MAJOR 1 (strengthen the two premise asserts to content, add one positive end-to-end assert on the arm), MAJOR 2 (reader top-level fallback + writer/reader file story), and decide the m1 re-arrival semantics; the m2s are cheap prose/pin alignments. Everything else can ride a follow-up wave.
将 CodeWhale 子模块 gitlink 从 `3118c36b8` 升级到 `480bebf6`
(Pinvou/CodeWhale `pinjou3-clean` 当前 HEAD),聚合 fork 侧本轮已
评审合并的 6 个 PR,并按 guard 结果同步登记与指纹。
- `CodeWhale` gitlink:`3118c36b8a73a91e97378a13bc6c623c1123efce`
→ `480bebf616212e790ff722c92fb165c8d4a2511d`。
- 登记/指纹随批同步(fork-guard --fast 的 guard 结果要求):
- `scripts/fork-guard.sh`:`EXPECTED_HEAD` → `480bebf6…`,
`EXPECTED_COMMITS` 62 → 68;
- `docs/fork-policy.md` / `.en.md`:登记头、提交计数(62→68,
sixty-eight)、批次清单补 2026-10-13/14 批次、更新日期;
- `docs/fork-modifications.md` / `.en.md`:§0 当前状态头前进至
`480bebf61`(r1 基线之上 53 个登记提交)、历史组织行 62→68、
drift 重实测(397 files +53533/−21490,净增回落至 32043)。
- app 侧无需适配:新底座上 `cargo check` 直接通过(本批六个 PR 均
不改父仓消费的底座 API 面)。
- 无依赖、锁文件或版本号变化。
## 来源 PR(fork 侧,均已 APPROVE 并通过 fork CI)
| PR | 摘要 |
|---|---|
| Pinvou/CodeWhale#92 | 保持账户路由逻辑调用在重试中的幂等身份 |
| Pinvou/CodeWhale#96 | Windows 会话导出探测失败关闭 |
| Pinvou/CodeWhale#101 | 修复 kimi 空 items 丢弃(含 deferred-toolbox 有界体制) |
| Pinvou/CodeWhale#102 | 清理已验证的无价值测试 |
| Pinvou/CodeWhale#103 | 按 era 圈定 checkpoint 识别 |
| Pinvou/CodeWhale#104 | 禁用 deferred-toolbox 大小界限(Pinvou fork 姿态) |
## 验证
- 6 个来源 PR 全部为 APPROVED 且非 draft,fork `pinjou3-clean` CI
(check、Analyze rust、CodeQL、Gitleaks、DCO)逐个在最新 base 上
全绿后合并;
- gitlink 目标 `480bebf6` 经 `git merge-base --is-ancestor` 核验包含
上述 6 个 squash 提交;
- 本地(新 gitlink 的 CodeWhale 工作副本)`./scripts/fork-guard.sh
--fast` 全绿(指纹层全过);
- `pinvou3-app/src-tauri` `cargo check --lib` 对新底座直接通过;
- drift 重实测:`397 files, +53533/-21490`(`480bebf61` vs 上游
`dcd4c200f`)。
Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101, confirmed independently by four reviewers. #104's prose follow-up (fd14b6a) was half-lost to its rebase over #101: the comment claiming the oversize reason "claims no bound and interpolates no numbers" landed, but #101 had refactored the inline format! into the shared toolbox_oversize_reason(), so the code still interpolates the session constants — under the fork's usize::MAX / 2 sentinels any render would read "(4611686018427387903 cached tools, 4503599627370495KiB ...)". Gate the oversize reason, the eviction-note capacity label, and the search-context recovery hint on the sentinel: disabled bounds render bound-free prose (a measurement failure is not fixed by a narrower query, so the hint rides only a real budget rejection); real bounds keep the numbers for upstream parity. The child's hydrate refusal now picks its cause truthfully — catalog-absent, not-deferred, or budget — instead of blaming the per-tool budget for every rejection, and drops the "bounded child toolbox" claim #104's commit message made but never landed. The stale "bounded (8 cached tools, 16KiB)" comment at the search eviction block now states the fork posture. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101, confirmed independently by three reviewers. pop_collective_overflow's loop condition is permanently false under the fork's usize::MAX / 2 sentinel bounds, but evaluating it re-serialized every cached tool via serde_json::to_vec — once per revalidate and once per requested name inside activate, i.e. O(cache x requested) full-cache serializations per search or hydration batch, over a cache the disabled bounds let grow for the conversation's lifetime. Early-return when both constants are the disabled sentinel. Behavior is unchanged in every reachable state: the only non-real trigger, the serde-failure measurement sentinel, is classified stale by the revalidation that runs before the pop loop. Also true up the session-side docs for the fork posture: the constants doc measures serialized tool definitions (not just schemas), the struct doc drops the "Bounded" lead-in, and revalidate's doc states the bounds-enforcement half never fires in-fork. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101. initial_active_tools' empty-scan fallback inserts the first catalog entry into the active set before any search runs, so two tool-search tests' post-search active.contains(...) asserts were vacuous — a mutant whose search never activates into the cache stayed green. Both tests now assert the cache population directly. The render test's cross-note dedup pinned the mention count but not which note survives; it now pins first-note-wins attribution. The new forkguard_toolbox_prose_claims_no_bound_under_the_disabled_sentinel pins the disabled-sentinel prose verbatim — red against the pre-fix code, which interpolated the sentinel numbers. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101. #104 deleted all ten child-surface eviction tests as bounds-triggered, but two of them parked notes directly and never touched the bounds — while 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 and its error-path twin); the child lost its only glue-level coverage of the round-2 defect shape (the child surface discarding every report its helpers produced). Restore child_parked_note_rides_success_and_error_results_and_nets_re_admissions and parked_note_rides_through_the_execute_from_surface_glue. Both pass unchanged against the current tree; the former also covers the round-9 {error:#} error-ride fix, which had shipped without a test. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101. refusal_audit_payload_carries_the_executed_marker sized its fixture (heavy_deferred_tool("notify", 20_000)) against the 16KiB per-tool budget #104 removed, so it silently took the DeferredHydration::Loaded arm while its name and docstring said Refused — passing either way, because both arms carry the executed:false marker. Renamed to hydration_receipt_audit_payload_carries_the_executed_marker and re-documented for the Loaded arm it actually drives; the docstring now states the oversize Refused arm is unreachable in this fork because the per-tool budget is the disabled usize::MAX / 2 sentinel. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101, MAJOR. The metrics rollup read only ~/.deepseek/audit.log, but the only in-repo tool.result writer is the engine's opt-in tool-audit log at $CODEWHALE_TOOL_AUDIT_LOG / $DEEPSEEK_TOOL_AUDIT_LOG, which has no default path — so #101's executed-marker consult and the round-9 flat-placement 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 so a co-located file is not counted twice; the writer's variable precedence is mirrored). The reader comment claiming "the sensitive audit log nests them under details" was false — audit.rs has no tool.result call sites; the nested placements are tolerated for foreign or hand-written lines and the comment now says so. The restricted-turn audit projection no longer strips success and executed: both are non-private booleans, and the reader defaults a missing flag to true, so stripping them bucketed every restricted receipt — failures included — as an executed success. The forkguard redaction test now pins that the flags survive while inputs, outputs, and paths stay omitted. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101. Two empty-items repair positions were exercised only by construction: an empty items under a root-level object-form additionalProperties (existing coverage nested it inside a property), and an empty items beside a $ref (pre-#101 the sibling failed closed with InvalidReference; the repair drops the keyword and the clean $ref node ships). Fixture both so a future walker change cannot silently alter them. The walker doc also named the schema-value positions it visits as "exactly" the set — true only for the MFJS-legal subset. It now notes that schema values under keywords MFJS rejects (contains, prefixItems, patternProperties, not, if/then/else, tuple-form items) need no visit: the keyword itself fails closed first. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101, prose only: - replace_runtime_mcp_tools' docstring tail claimed a same-count refresh never moves wire bytes; the round-9 content compare makes a same-name content change declare, and the tail now says so. - The arrivals comment justified eager arrival only by the 401-auth case; it now states the bulk post-login arrival truth (any newly served name arrives eager by the same rule, deliberately). - park_toolbox_eviction_note's doc said "combining" where the implementation queues; repeated names are netted at render. - The SyncSession note clear now states it is redundant with the turn-entry sweep and kept so no future non-turn reader inherits cross-conversation state. - TOOL_SURFACE.md: the cache paragraph measures serialized tool definitions, not just schemas; the bounds-eviction reporting paragraph is marked inert in this fork (upstream-parity machinery); and the catalog-shrink absorption claim is corrected — a search's revalidation reports a cached-but-departed name under unavailable_tool_references regardless of the query, and only a shrink first observed at a turn boundary is absorbed silently. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101, confirmed independently by four reviewers. #104's prose follow-up (fd14b6a) was half-lost to its rebase over #101: the comment claiming the oversize reason "claims no bound and interpolates no numbers" landed, but #101 had refactored the inline format! into the shared toolbox_oversize_reason(), so the code still interpolates the session constants — under the fork's usize::MAX / 2 sentinels any render would read "(4611686018427387903 cached tools, 4503599627370495KiB ...)". Gate the oversize reason, the eviction-note capacity label, and the search-context recovery hint on the sentinel: disabled bounds render bound-free prose (a measurement failure is not fixed by a narrower query, so the hint rides only a real budget rejection); real bounds keep the numbers for upstream parity. The child's hydrate refusal now picks its cause truthfully — catalog-absent, not-deferred, or budget — instead of blaming the per-tool budget for every rejection, and drops the "bounded child toolbox" claim #104's commit message made but never landed. The stale "bounded (8 cached tools, 16KiB)" comment at the search eviction block now states the fork posture. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101, confirmed independently by three reviewers. pop_collective_overflow's loop condition is permanently false under the fork's usize::MAX / 2 sentinel bounds, but evaluating it re-serialized every cached tool via serde_json::to_vec — once per revalidate and once per requested name inside activate, i.e. O(cache x requested) full-cache serializations per search or hydration batch, over a cache the disabled bounds let grow for the conversation's lifetime. Early-return when both constants are the disabled sentinel. Behavior is unchanged in every reachable state: the only non-real trigger, the serde-failure measurement sentinel, is classified stale by the revalidation that runs before the pop loop. Also true up the session-side docs for the fork posture: the constants doc measures serialized tool definitions (not just schemas), the struct doc drops the "Bounded" lead-in, and revalidate's doc states the bounds-enforcement half never fires in-fork. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101. initial_active_tools' empty-scan fallback inserts the first catalog entry into the active set before any search runs, so two tool-search tests' post-search active.contains(...) asserts were vacuous — a mutant whose search never activates into the cache stayed green. Both tests now assert the cache population directly. The render test's cross-note dedup pinned the mention count but not which note survives; it now pins first-note-wins attribution. The new forkguard_toolbox_prose_claims_no_bound_under_the_disabled_sentinel pins the disabled-sentinel prose verbatim — red against the pre-fix code, which interpolated the sentinel numbers. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101. #104 deleted all ten child-surface eviction tests as bounds-triggered, but two of them parked notes directly and never touched the bounds — while 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 and its error-path twin); the child lost its only glue-level coverage of the round-2 defect shape (the child surface discarding every report its helpers produced). Restore child_parked_note_rides_success_and_error_results_and_nets_re_admissions and parked_note_rides_through_the_execute_from_surface_glue. Both pass unchanged against the current tree; the former also covers the round-9 {error:#} error-ride fix, which had shipped without a test. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101. refusal_audit_payload_carries_the_executed_marker sized its fixture (heavy_deferred_tool("notify", 20_000)) against the 16KiB per-tool budget #104 removed, so it silently took the DeferredHydration::Loaded arm while its name and docstring said Refused — passing either way, because both arms carry the executed:false marker. Renamed to hydration_receipt_audit_payload_carries_the_executed_marker and re-documented for the Loaded arm it actually drives; the docstring now states the oversize Refused arm is unreachable in this fork because the per-tool budget is the disabled usize::MAX / 2 sentinel. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101, MAJOR. The metrics rollup read only ~/.deepseek/audit.log, but the only in-repo tool.result writer is the engine's opt-in tool-audit log at $CODEWHALE_TOOL_AUDIT_LOG / $DEEPSEEK_TOOL_AUDIT_LOG, which has no default path — so #101's executed-marker consult and the round-9 flat-placement 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 so a co-located file is not counted twice; the writer's variable precedence is mirrored). The reader comment claiming "the sensitive audit log nests them under details" was false — audit.rs has no tool.result call sites; the nested placements are tolerated for foreign or hand-written lines and the comment now says so. The restricted-turn audit projection no longer strips success and executed: both are non-private booleans, and the reader defaults a missing flag to true, so stripping them bucketed every restricted receipt — failures included — as an executed success. The forkguard redaction test now pins that the flags survive while inputs, outputs, and paths stay omitted. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101. Two empty-items repair positions were exercised only by construction: an empty items under a root-level object-form additionalProperties (existing coverage nested it inside a property), and an empty items beside a $ref (pre-#101 the sibling failed closed with InvalidReference; the repair drops the keyword and the clean $ref node ships). Fixture both so a future walker change cannot silently alter them. The walker doc also named the schema-value positions it visits as "exactly" the set — true only for the MFJS-legal subset. It now notes that schema values under keywords MFJS rejects (contains, prefixItems, patternProperties, not, if/then/else, tuple-form items) need no visit: the keyword itself fails closed first. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101, prose only: - replace_runtime_mcp_tools' docstring tail claimed a same-count refresh never moves wire bytes; the round-9 content compare makes a same-name content change declare, and the tail now says so. - The arrivals comment justified eager arrival only by the 401-auth case; it now states the bulk post-login arrival truth (any newly served name arrives eager by the same rule, deliberately). - park_toolbox_eviction_note's doc said "combining" where the implementation queues; repeated names are netted at render. - The SyncSession note clear now states it is redundant with the turn-entry sweep and kept so no future non-turn reader inherits cross-conversation state. - TOOL_SURFACE.md: the cache paragraph measures serialized tool definitions, not just schemas; the bounds-eviction reporting paragraph is marked inert in this fork (upstream-parity machinery); and the catalog-shrink absorption claim is corrected — a search's revalidation reports a cached-but-departed name under unavailable_tool_references regardless of the query, and only a shrink first observed at a turn boundary is absorbed silently. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101, confirmed independently by four reviewers. #104's prose follow-up (fd14b6a) was half-lost to its rebase over #101: the comment claiming the oversize reason "claims no bound and interpolates no numbers" landed, but #101 had refactored the inline format! into the shared toolbox_oversize_reason(), so the code still interpolates the session constants — under the fork's usize::MAX / 2 sentinels any render would read "(4611686018427387903 cached tools, 4503599627370495KiB ...)". Gate the oversize reason, the eviction-note capacity label, and the search-context recovery hint on the sentinel: disabled bounds render bound-free prose (a measurement failure is not fixed by a narrower query, so the hint rides only a real budget rejection); real bounds keep the numbers for upstream parity. The child's hydrate refusal now picks its cause truthfully — catalog-absent, not-deferred, or budget — instead of blaming the per-tool budget for every rejection, and drops the "bounded child toolbox" claim #104's commit message made but never landed. The stale "bounded (8 cached tools, 16KiB)" comment at the search eviction block now states the fork posture. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101, confirmed independently by three reviewers. pop_collective_overflow's loop condition is permanently false under the fork's usize::MAX / 2 sentinel bounds, but evaluating it re-serialized every cached tool via serde_json::to_vec — once per revalidate and once per requested name inside activate, i.e. O(cache x requested) full-cache serializations per search or hydration batch, over a cache the disabled bounds let grow for the conversation's lifetime. Early-return when both constants are the disabled sentinel. Behavior is unchanged in every reachable state: the only non-real trigger, the serde-failure measurement sentinel, is classified stale by the revalidation that runs before the pop loop. Also true up the session-side docs for the fork posture: the constants doc measures serialized tool definitions (not just schemas), the struct doc drops the "Bounded" lead-in, and revalidate's doc states the bounds-enforcement half never fires in-fork. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101. initial_active_tools' empty-scan fallback inserts the first catalog entry into the active set before any search runs, so two tool-search tests' post-search active.contains(...) asserts were vacuous — a mutant whose search never activates into the cache stayed green. Both tests now assert the cache population directly. The render test's cross-note dedup pinned the mention count but not which note survives; it now pins first-note-wins attribution. The new forkguard_toolbox_prose_claims_no_bound_under_the_disabled_sentinel pins the disabled-sentinel prose verbatim — red against the pre-fix code, which interpolated the sentinel numbers. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101. #104 deleted all ten child-surface eviction tests as bounds-triggered, but two of them parked notes directly and never touched the bounds — while 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 and its error-path twin); the child lost its only glue-level coverage of the round-2 defect shape (the child surface discarding every report its helpers produced). Restore child_parked_note_rides_success_and_error_results_and_nets_re_admissions and parked_note_rides_through_the_execute_from_surface_glue. Both pass unchanged against the current tree; the former also covers the round-9 {error:#} error-ride fix, which had shipped without a test. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101. refusal_audit_payload_carries_the_executed_marker sized its fixture (heavy_deferred_tool("notify", 20_000)) against the 16KiB per-tool budget #104 removed, so it silently took the DeferredHydration::Loaded arm while its name and docstring said Refused — passing either way, because both arms carry the executed:false marker. Renamed to hydration_receipt_audit_payload_carries_the_executed_marker and re-documented for the Loaded arm it actually drives; the docstring now states the oversize Refused arm is unreachable in this fork because the per-tool budget is the disabled usize::MAX / 2 sentinel. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101, MAJOR. The metrics rollup read only ~/.deepseek/audit.log, but the only in-repo tool.result writer is the engine's opt-in tool-audit log at $CODEWHALE_TOOL_AUDIT_LOG / $DEEPSEEK_TOOL_AUDIT_LOG, which has no default path — so #101's executed-marker consult and the round-9 flat-placement 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 so a co-located file is not counted twice; the writer's variable precedence is mirrored). The reader comment claiming "the sensitive audit log nests them under details" was false — audit.rs has no tool.result call sites; the nested placements are tolerated for foreign or hand-written lines and the comment now says so. The restricted-turn audit projection no longer strips success and executed: both are non-private booleans, and the reader defaults a missing flag to true, so stripping them bucketed every restricted receipt — failures included — as an executed success. The forkguard redaction test now pins that the flags survive while inputs, outputs, and paths stay omitted. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101. Two empty-items repair positions were exercised only by construction: an empty items under a root-level object-form additionalProperties (existing coverage nested it inside a property), and an empty items beside a $ref (pre-#101 the sibling failed closed with InvalidReference; the repair drops the keyword and the clean $ref node ships). Fixture both so a future walker change cannot silently alter them. The walker doc also named the schema-value positions it visits as "exactly" the set — true only for the MFJS-legal subset. It now notes that schema values under keywords MFJS rejects (contains, prefixItems, patternProperties, not, if/then/else, tuple-form items) need no visit: the keyword itself fails closed first. Signed-off-by: asto <asto18089@126.com>
Round-10 fresh review of #101, prose only: - replace_runtime_mcp_tools' docstring tail claimed a same-count refresh never moves wire bytes; the round-9 content compare makes a same-name content change declare, and the tail now says so. - The arrivals comment justified eager arrival only by the 401-auth case; it now states the bulk post-login arrival truth (any newly served name arrives eager by the same rule, deliberately). - park_toolbox_eviction_note's doc said "combining" where the implementation queues; repeated names are netted at render. - The SyncSession note clear now states it is redundant with the turn-entry sweep and kept so no future non-turn reader inherits cross-conversation state. - TOOL_SURFACE.md: the cache paragraph measures serialized tool definitions, not just schemas; the bounds-eviction reporting paragraph is marked inert in this fork (upstream-parity machinery); and the catalog-shrink absorption claim is corrected — a search's revalidation reports a cached-but-departed name under unavailable_tool_references regardless of the query, and only a shrink first observed at a turn boundary is absorbed silently. Signed-off-by: asto <asto18089@126.com>
Summary
Two fixes for a failure found live in a Pinvou browser session: a strong model spent an entire turn (30+ steps, twice interrupted) trying to call
mcp_browser_evaluate_scriptwhile every request silently lacked the tool.Root cause
sanitize_moonshot_chat_toolsdrops any tool whose parameters fail Moonshot MFJS validation, with only atracing::warn!on stderr (streaming turns also toast the omitted names) — nothing the model can see. MFJS fails empty schema nodes ({}) withMissingType, and the Pinvou browser catalog shipped exactly one:So
tool_searchkept advertising the reference and the activation cache kept admitting it, but the outboundtoolsarray never carried it. A model trained to only emit listed tools collapses each attempted call into the nearest listed tool (navigate_page) — the observed reload loop.Secondary: the bounded activation cache (8 names / 16KiB) LRU-evicted
take_snapshot/click/fillmid-session when a later search admitted new tools, also with no reporting — a tool that executed successfully earlier in the same conversation silently became uncallable.Changes
fix(kimi): repair empty items instead of dropping—sanitize_kimi_parameters_candidatenow removes emptyitemsschema nodes before validation. JSON Schema reads an absentitemsas unconstrained, which is exactly whatitems: {}asserts, so the repair is semantics-preserving and MFJS-legal. Untyped nodes without an equivalent shape still fail closed: empty property schemas, emptyanyOfbranches (including branches the nested repair empties), empty$defsentries, annotation-onlyitems, boolean or tuple-formitems,itemsinside unsupported keywords (prefixItems,contains, multi-element nestedallOf— a single-elementallOfis flattened by the upstream union-collapsing pass and ships the equivalent repaired form), andminItems ≥ 1beside an emptyitemskeeps failing closed wherever the terminator checks (required properties,anyOfbranches,$reftargets) — the error variant can differ (InvalidSchemaNodefor theminItemscase instead ofMissingType) — while a non-required property position ships the equivalent repaired schema, where pre-repair it dropped.minItems: 0beside an emptyitemsis repaired: its only MFJS-legal shape terminates, and the repaired schema is equivalent.fix(kimi): repair nested empty items shapes— the walker now visits children before checking the node's ownitems, without the early return. This repairs two same-class shapes that previously still failed closed:items: {"items": {}}(the inner repair empties the outer node, and{"items": X}is vacuously true for non-arrays, so dropping the outer keyword preserves semantics) and a rootitemsbeside$defs.fix(tool-search): report evicted activations—execute_tool_search_innernow returnsevicted_tool_referencesplus atoolbox_notenaming the evicted tools and the recovery contract (re-search to re-activate). Searches without evictions keep the legacy result shape unchanged.fix(tool-search): report every toolbox eviction—ToolActivationDeltaseparates stale entries (the tool left the deferred catalog: disconnect, deny rule, oversize) from bounds evictions. A search reports stale tools underunavailable_tool_referenceswith a catalog-shrink reason, since the re-search contract would be a false promise for them. Bounds evictions from the three non-search activations — batch hydration of directly-called deferred tools,retrieve_tool_resultadmission on an evidence receipt, and the post-execution LRU refresh — now reach the model too: the hydration note rides the first recorded result of the batch (or its error), retrieval/refresh notes ride the triggering result, and a child hydration receipt carries its own note. The capacity label is derived from the session constants.docs/TOOL_SURFACE.mddocuments the reporting channels.fix(tool-search): report toolbox drops truthfully— round-2 fresh-review fixes.revalidatenow classifies every cache removal by recoverability: retain drops carry a cause (catalog shrink, or per-tool oversize reported with the same oversize reason discovery rejections use), eager promotions are forgotten without a report (the tool is directly callable, so both vocabularies would be false statements about it), and collective-bounds pops are re-activatableevictedentries instead of stale ones — reachable when a cached schema grows past the budget mid-conversation. The search payload reports each stale entry with its truthful reason and emits an oversize drop once instead of twice. With the split in place the post-execution refresh channel becomes live: a refresh evicts when a cached schema grew past the budget since its last activation, and the note rides the refreshed result; before this commit that channel was provably always empty while the growth-path evictions landed instaleand were dropped unreported.Op::SyncSessionclears the parked eviction note alongside the activation cache so a note left by an interrupted turn cannot ride the next conversation's results.docs/TOOL_SURFACE.mdstates the reporting contract as implemented, including the eager carve-out and the silent absorption of unavailable drops on non-search paths.fix(tool-search): net re-admissions from evictions— round-3 fresh-review fixes. When the searched or hydrated tool is itself the LRU-front victim of a growth overflow, revalidation pops it and the same activation re-admits it; the delta now nets re-admitted names out ofevicted, so the search payload and every note can quote the list verbatim without claiming a tool is uncallable in the very result that returns it. The per-tool budget gate is now shared by admission, revalidation, and hydration planning, and an oversize direct-call hydration is refused up front with the shared oversize bounds statement (without the search-context recovery hint, since there is no query to narrow) — instead of a "has now been loaded" receipt that turned every retry into another false promise. The catalog-shrink stale-reason constant is renamed to say what it names.fix(tool-search): report turn-start toolbox evictions— the turn-start revalidation is the fifth bounds-eviction trigger and the only one reported nowhere: when a cached schema grew between turns (an MCP refresh), the LRU victim silently vanished from the next request. Its note now parks like a hydration's and rides the turn's first recorded result, combining with any note a hydration batch parks in the same turn. A note stranded by an interrupted turn or a panic is swept at turn entry instead of riding an unrelated later result, and a batch whose outcomes all flatten away re-parks its note for a later batch (the two round-3 pins covering the sweep and the re-park land in their own commit,test(engine): pin the stranded-note sweep and re-park backstop). The hydration site's comment promised that the nexttool_searchresult names absorbed unavailable drops; that channel structurally cannot fire for a catalog-shrink drop (the absorbing path prunes the cache first, and discovery cannot match a name that left the catalog), so the comment anddocs/TOOL_SURFACE.mdnow state the split: an oversize drop is re-reported with its reason the next time a search matches the tool, a catalog-shrink one is learned only by the search missing. The success and error paths now observe the working set on the same pre-note result text.fix(subagent): report child toolbox evictions— the child surface discarded every eviction report its helpers produced: the post-execution refresh droppedToolSurfaceUpdateon the floor andrequest_toolsdiscarded its revalidation entirely, so a child's toolbox could shrink with zero explanation while the identical parent event produced a note. Refresh evictions now ride the refreshed tool's result, and evictions observed outside any payload (a catalog refresh's revalidation, or a warm start that overflows — the latter currently defensive, since the only production spawn passes no warm names) park a note on the surface that rides the child's next recorded result — the parent engine's parked-note channel, mirrored. The hydrate refusal states the shared oversize reason instead of a narrower-query hint no narrower query can satisfy.docs(tool-search): truthful toolbox contract—docs/TOOL_SURFACE.mdrestates the reporting paragraph to match the implementation: which channel carries which eviction (now including the turn's start and the child channels), the never-name-a-re-admitted-tool rule, and the unavailable-absorption truth.fix(tool-search): net parked evictions at ride time— round-4 fresh-review fixes. Netting previously lived only inside oneToolActivationDelta, so a note parked at the turn's start (or by a child's catalog refresh) could name a tool that the very result carrying it had re-admitted — the model retrying a tool the turn-start eviction just dropped hydrates it back before any result records, and the payload said both "has now been loaded" and "no longer callable until a later search re-activates them". Parked notes now hold structured names and are netted against the live activation cache when they attach, on the parent's success and error paths and on the child surface; a note emptied by the netting is dropped entirely. The note trigger becomes an enum shared by both surfaces, and the park/combine logic collapses into one helper.fix(subagent): report child evictions on error results— round-4 fresh-review fixes. The child's parked-note ride attached only toOkresults, so a note whose next result was an error stranded until a laterOk— or vanished when the child ended first; the parent engine rides error results too. The child'shydraterejection also ran after the activation, silently absorbing the revalidation evictions on its way; the rejection error now carries that eviction note beside the shared oversize reason. The ride glue becomes a pinned surface method covering theOk,Err, and fully-netted cases.fix(tool-search): stay silent when hydration is refused— round-4 fresh-review fixes. The oversize refusal left the side channels announcing a load: the batch still emitted the user-facing "Loaded deferred tool 'X'. Retry the call" status, auditedtool.schema_hydrated, and pushed the refused name into the hydration order, asking the activation the refusal exists to spare. The receipt is now aDeferredHydration::Refusedarm — announcement, audit, and hydration-order push happen only onLoaded— and the refusal prose stops repeating the toolbox phrase the shared reason already states.test(kimi): pin the remaining empty-items repair visits— round-4 mutation testing proved the walker's object-formadditionalPropertiesandanyOf-branch visits unpinned (deleting either left the module green); fixtures for both positions now fail withMissingTypeif a visit is dropped.test(engine): pin sweep-before-park at turn entry— round-4 mutation testing showed swapping the turn-entry sweep with the turn-start park passed every test; the new pin strands a note, evicts at the turn's start, and requires exactly the fresh note to survive. TheSyncSessionclear's composed-only coverage (the turn-entry sweep masks it) is now stated in the session-sync test: the clear is defense in depth, and removing both is what fails.docs(tool-search): scope the reporting quantifier to the code— round-4 fresh-review fixes. "Every bounds-driven eviction is reported" overclaimed: the sweep, the session sync, and a child that ends first deliberately drop notes that never reached a result. The paragraph now states the best-effort contract, the drop edges, and the ride-time netting; the child channel gains the same "(or its error)" wording the parent already had.fix(tool-search): net notes against active tools— round-5 fresh-review fixes. Ride-time netting consulted only the activation cache, so a name a mid-turn MCP catalog refresh re-advertised into the request's active tool set stayed in the note while the next request carried the tool. Both ride sites (parent success and error, child surface) now net against the cache and the active-name set;docs/TOOL_SURFACE.mdstates the wider scope and a unit pin covers the refresh window.fix(tool-search): mark refusal receipts unexecuted— round-5 fresh-review fixes. The oversize refusal receipt was a bare success result while the Loaded receipt setsexecuted: false— and the results phase defaultsexecutedto true, so the working set observed a call that never happened and the audit logged the refusal as a successful execution. The receipt now carries the marker, pinned in the refusal test.fix(tool-search): truthful oversize prose and dedup— round-5 fresh-review fixes. The shared oversize reason now says what the gate measures (the serialized tool: name, description, and schema together); the search-context recovery hint is appended only in search results, so a direct-call refusal no longer tells a queryless caller to narrow a search; and the search payload's stale/rejected dedup keys on the oversize cause instead of the name, so a catalog-shrink entry can never be silenced by an invariant that holds only emergently.fix(tool-search): re-park notes by extending— round-5 fresh-review fixes. The flattened-batch backstop extends the session slot instead of assigning over it, so a note parked during the results phase could never be silently dropped by a future change.test(engine): pin the batch eviction park— round-5 fresh-review fixes. The hydration batch's park call site now has a red-verified pin through a realplan_tool_callswhose admission overflows the collective bounds; before this commit deleting the park call left the whole suite green, and the earlier "pinned transitively" wording was overstated.test(subagent): pin the note ride through the glue— round-5 fresh-review fixes. The child's parked-note ride is pinned throughexecute_from_surfaceitself; before this commit every child pin called the surface methods directly, so a regression to the round-2 defect (the child discarding reports) would have passed the suite. The tautologicalmem::taketail of the park test is removed.test(client): pin the surviving wire tool name— round-5 fresh-review fixes. The wire test now also asserts the identity of the surviving tool, like the sibling drop-path test.docs(tool-search): tighten the toolbox contract— round-5 fresh-review fixes. Precision pass: the netting scope includes the request's active tool set, the session-sync edge is attributed to the sync op itself, the child channel lists the hydrate-refusal carrier and the warm-start park, the catalog-shrink cause gloss stops narrowing, and the oversize re-report fires when a search returns the tool (not merely matches it).fix(tool-search): net step evictions at ride time— round-6 fresh-review fixes. The result's own touch/retrieval evictions rendered verbatim: only the parked notes netted at attach, so a same-outcomemcp_catalog_changedrefresh — which re-inserts every refreshed pool tool into the active set after the touch removed a victim — could leave the step note declaring a tool the next request carries "no longer callable". The step eviction now joins the parked notes and renders through the same attach-time netting; the pin drives the window throughprocess_tool_resultswith a seeded needs-auth MCP pool whose refreshed list serves the synthetic eager authenticate tool.fix(engine): carry executed into tool.result audit— round-6 fresh-review fixes. The refusal receipt'sexecuted: falsemarker reached the working set and the LSP hook but not thetool.resultaudit payload, which is built from the terminal status and the success flag alone — the audit logged a refused, never-executed call as an unconditional successful execution while the receipt comment claimed the marker covers the audit. The payload now carries the marker; restricted-turn projections keep dropping the field by design.docs(tool-search): name the overflowing budget— round-6 fresh-review fixes. The refresh-eviction parenthetical said a cached schema "grew past the budget"; the eviction fires when the collective budget overflows, not on a per-tool breach. State the collective budget.fix(tool-search): refresh respects evictions— round-7 fresh-review fixes, the cross-outcome window in the never-name-a-callable-tool invariant.replace_runtime_mcp_toolsadopted the pool's flags wholesale: everyto_api_toolsentry is built eager, so a refresh flipped previously-deferred MCP tools back onto the head of the wire list and blindly re-inserted every refreshed name into the active set — including a tool a note in the same pass had just declared uncallable: outcome k's note rendered before outcome j > k'smcp_catalog_changedrefresh resurrected the victim onto the wire behind its back (the search payload has the same window with no parked note at all; every pin drove a single-outcome batch). The refresh now carries each replaced tool's prior deferral marking across — which also keeps replaced entries in the wire list's stable tail instead of busting the cached prefix — and re-advertises only eager entries plus deferred tools this conversation still has activated: an evicted deferred tool stays off the wire, the note naming it stays truthful through delivery, and the next turn's active-set re-derivation no longer reverses the refresh in between. The same-outcome round-6 pin is rewritten to the new truth (the step note names the victim the refresh cannot resurrect), a new pin drives the cross-outcome window, and the unit test pins the preserved marking, the unactivated-deferred eviction, and the still-activated re-admission; blind insertion turns all three red.test(engine): pin refusal audit executed marker— round-7 fresh-review fixes. The round-6 marker fix shipped unpinned: no test read thetool.resultaudit payload for the refusal path, so deleting"executed": tool_was_executedleft the whole suite green. The pin drives the real refusal receipt fromplan_tool_callsthrough the results phase with the audit log enabled and requires the marker verbatim (mutation-verified).test(subagent): pin the child dispatch glue— round-7 fresh-review fixes. The round-5 glue pin covered only the parked-note ride; the hydrate dispatch, the touch channel's eviction reporting (the exact round-2 defect shape), and the warm-start overflow park stayed pinned below the production dispatch — deleting the wiring left the suite green, each verified by mutation. Three pins now driveexecute_from_surfaceitself, and the byte-tuned fixtures gain explicit warm-pair-fits-the-budget premise assertions.fix(tool-search): dedupe and drain the eviction-note render— round-7 fresh-review fixes. The render netted per note but never across notes, so a victim the turn-start park named and the same batch's step eviction named again rendered twice in one result; the second mention is now dropped (mutation-verified). The render also consumes the note slot itself whether or not anything rendered, so a caller cannot forget the clear and re-ride a note; the parent's Ok and Err arms and the child's ride drop their own clears.test(kimi): pin minItems positions and fail-closed shapes— round-7 fresh-review fixes. Fixtures for bothminItemspositions (terminator-checkedInvalidSchemaNode, non-required ships-the-equivalent,minItems: 0repairs) and for the previously unfixtured fail-closed classes: emptyanyOfbranches (including one the nested repair itself empties), empty$defsentries, boolean and tuple-formitems, each asserting its variant.fix(cli): count unexecuted receipts as neither— round-7 fresh-review fixes. The tool.result rollup read only the success flag, so a refused oversize hydration — and a hydration receipt — inflated the tool's success count. Receipts markedexecuted: falsecount as neither success nor failure.fix(tool-search): truthful prose, comments, and one bounds pop— round-7 fresh-review fixes. The note and reason prose said "serialized schemas" where the gate measures the whole serialized tool; the hydration-site comment said a search MATCHES the tool re-reports an oversize drop (a match ranked past the result cap re-reports nothing — the docs were fixed in round 5, the comment kept the old wording) and overstated the catalog-shrink claim as a class property; the eager-carve-out comment stated a wire-layer invariant the engine layer does not provide. The collective-bounds pop loop — the condition and the LRU-front-first policy — lived twice (revalidation and admission), both rewritten by this PR without unifying; one shared method now owns it. The park and ride paths emit debug tracing so the channel whose invisibility caused the original incident is diagnosable.fix(tool-search): splice refreshes and honor turn policy— round-8 fresh-review fixes, two pre-existing defects inside the refresh path.replace_runtime_mcp_toolsrelocated the pool slice to the end of the catalog on every refresh and declared drift by catalog-count change alone: a same-name refresh reordered the wire's tail, and a count-preserving identity change (a server-side rename, or a 1-tool server's live 401 whose real tool is replaced by the synthetic authenticate) changed the wire while declaring nothing — undeclared drift, which the C5 prefix guard treats as a real bug. Replaced entries are now spliced back into their own catalog slot (a same-name refresh reorders nothing; entries after the slice never move), and the declaration keys on the model-visible wire identity — the eager head plus the activated deferred tail — so a wire-visible rename is declared and a no-op refresh truthfully declares nothing. The mid-turnmcp_catalog_changedarm also built its universe and refreshed list straight from the pool with no allow/deny narrowing: an MCP name the build had stripped (disallowed_tools, or absent fromallowed_tools) re-entered the catalog and the active set — eager — on the first 401 or completed login, defeating the denied-name concealment the boot-window refresh deliberately re-applies. The arm now filters both through the sameToolSurfacePolicy::allows_toolspelling the build and the boot window use, threaded from the turn loop intoprocess_tool_results; new names keep the pool's eager flag, because the 401 result's auth hint points straight at the login tool.fix(subagent): rehydrate mid-request evicted retries— round-8 fresh-review fix closing the child's within-request window: the dispatch consulted only the request's active-name snapshot, so a deferred tool a mid-request bounds eviction had removed from the surface's live set stayed dispatchable for the rest of the request while an earlier result's note declared it uncallable. The retry now routes through hydration — re-admitted under the bounds with the truthful load receipt (its own eviction reported), or refused with the shared oversize reason; the snapshot still governs the anti-guessing guard, and the parent engine never had this window because its notes render only in the results phase, after every execution.fix(tool-search): name unrecoverable evictions— round-8 fresh-review fix: a parked note promised "a later search re-activates them" for a name that left the catalog between its eviction and the ride (a mid-turn refresh removing a server). The ride-time render partitions each note by catalog membership — names the catalog still serves keep the recovery contract byte-for-byte, departed names render under the unrecoverable truth; both parent arms and the child ride pass the catalog.test(cli): pin the unexecuted-receipt rollup bucket— round-8. Item 32's neither-success-nor-failure bucket shipped unpinned; the pin drives a temp audit log throughread_audit_log(plain success, plain failure,executed: falsereceipt, executed-marked success) and deleting theexecutedconsult turns it red.test(kimi): pin the anyOf and ref minItems positions— round-8. The position-dependence fixtures covered the required-property position only; ananyOfbranch and a$reftarget, each reached from a required property, now pin the sameInvalidSchemaNodevariant.docs(tool-search): truthful gate prose and child entries— round-8. TheOversizecause gloss,exceeds_per_tool_budget's doc comment, and the hydration-site comment still said "schema" where the gate measures the whole serializedTool(the pins themselves overflow through a 20,000-byte description);TOOL_SURFACE.md's refresh parenthetical kept the same schema-only noun one paragraph after the model-facing prose had been fixed. All now state the measured unit, and the doc states the two child truths above: the mid-request rehydration and the unrecoverable-eviction wording.Testing
kimi_parameters_repairs_empty_items_as_unconstrained(red-green verified: fails with the repair disabled),kimi_parameters_repairs_nested_and_sibling_empty_items,search_result_names_cache_evictions_instead_of_dropping_them_silently,search_result_names_byte_bound_cache_evictions_too,search_reports_catalog_stale_evictions_as_unavailable_not_evicted,moonshot_keeps_empty_items_tool_on_the_wire(wire-level: the incident shape is retained on a Kimi request withitemsrepaired to absent).revalidate_classifies_catalog_shrink_and_eager_removals,revalidate_reports_byte_growth_overflows_as_re_activatable_evictions,search_does_not_report_an_eager_promotion_as_unavailable,search_reports_growth_overflows_as_re_activatable_evictions,search_reports_oversize_drops_with_the_oversize_reason,hydration_eviction_note_rides_the_batch_first_result,hydration_eviction_note_survives_a_leading_error_result,retrieval_admission_eviction_note_rides_the_triggering_result,execution_refresh_eviction_note_rides_the_refreshed_result,child_hydration_receipt_carries_the_eviction_note,session_sync_drops_a_parked_toolbox_eviction_note(driven through the live engine loop).search_does_not_name_its_own_admission_as_evicted(the searched tool is the growth victim and must not be named evicted by its own result),turn_start_revalidation_parks_an_eviction_note_for_the_turn(driven through the liverun_turn),turn_start_sweeps_a_stranded_eviction_note,toolbox_eviction_notes_park_and_combine,toolbox_note_survives_a_batch_without_results,child_execution_refresh_eviction_note_rides_the_child_result,child_parked_note_rides_the_next_result_and_refresh_parks_one,hydration_of_an_oversize_tool_reports_the_rejection_not_a_load. The hydration parking call site is pinned transitively (the shared park/combine helper is tested directly and the consumption channels end-to-end); the turn-start twin is pinned end-to-end.toolbox_note_rendering_nets_re_admitted_names_at_ride_timeandturn_start_note_does_not_name_a_tool_the_result_re_admits(ride-time netting: disabling it turns both plus the child pin red),child_hydrate_rejection_carries_the_revalidations_evictions,child_parked_note_rides_success_and_error_results_and_nets_re_admissions,refused_hydration_emits_no_load_status_and_never_enters_the_batch(driven through a directplan_tool_calls),turn_entry_sweep_precedes_the_turn_start_park, and theadditionalProperties/anyOffixtures inkimi_parameters_repairs_nested_and_sibling_empty_items.hydration_batch_eviction_parks_its_note_through_plan_tool_calls(deleting the batch's park call),parked_note_rides_through_the_execute_from_surface_glue(deleting the child's ride call), the refusal receipt'sexecuted: falsemetadata assertion (dropping the marker), and the active-set segment oftoolbox_note_rendering_nets_re_admitted_names_at_ride_time(netting against the cache only).step_note_names_a_victim_the_refresh_cannot_resurrect(the refresh no longer resurrects an evicted deferred tool, so the note names it and the wire must not carry it; blind insertion turns it red), joined byrefresh_between_outcomes_cannot_resurrect_an_evicted_toolfor the cross-outcome window.refusal_audit_payload_carries_the_executed_marker(dropping the audit field: red),hydrate_dispatch_loads_a_deferred_tool_through_the_execute_from_surface_glue,touch_glue_rides_the_refresh_eviction_through_execute_from_surface,warm_start_overflow_park_rides_the_first_result(breaking each dispatch: 3/3 red), and the cross-note dedup segment oftoolbox_note_rendering_nets_re_admitted_names_at_ride_time(removing the dedup: red).runtime_mcp_refresh_replaces_the_pool_slice(restoring the relocate-and-push body: red) and its wire-keyed declaration phase (restoring the count-keyed return: red),mid_turn_refresh_honors_the_turn_policy(deleting the arm'sallows_toolfilter: red), andmid_request_eviction_routes_the_retry_through_hydration(deleting the dispatch's live-set disjunct: red). The two e2e refresh pins gain premise asserts requiring the pool slice to have actually been replaced (a "refresh never runs" mutant previously left both green); the render pin gains the empty-companion filter and the unrecoverable-wording segments.cargo test -p codewhale-tui --lib schema_sanitize: 64 passed / 0 failed.cargo test -p codewhale-tui --lib tool_catalog: 39 passed / 0 failed.cargo test -p codewhale-tui --lib: 12133 passed / 0 failed / 13 ignored (a prior round also saw one full-suite flake in the untouchedmodel_inventorymodule —ollama_default_prefers_live_local_tags_over_the_unresolved_marker, aprovider_lakelive-snapshot race under full-suite parallelism; it passes in isolation, 1/0) (one earlier full-suite run flaked the same two untouchedruntime_threadstests it flakes under full-suite parallelism on this machine; both pass in isolation, 166/0).cargo fmt --all -- --checkclean; clippy with the fork-CI flag set clean locally and passing on GitHub CI. A local floating 1.99.0 toolchain additionally flags six pre-existing lint patterns; they are present on the pristine PR head and are out of this PR's scope.tools/listserves the repaired schema and the tool is callable on the k3 gateway.Companion Pinvou agent PR pinvou-agent#683 (merged) fixes the browser catalog schema at the source; it was opened shortly before this PR. This PR's merge is pinned by a gitlink bump in that repository as a follow-up, together with the fork-guard register advance (per its own body).
No-Issue: found live in a production browser session; repository issues are disabled for this fork, so the failure mode, root cause and fix evidence are documented in this PR body.