Skip to content

fix(kimi): repair empty items instead of dropping - #101

Merged
asto18089 merged 40 commits into
pinvou3-cleanfrom
fix/kimi-mfjs-empty-items-drop
Oct 13, 2026
Merged

asto18089 merged 40 commits into
pinvou3-cleanfrom
fix/kimi-mfjs-empty-items-drop

Conversation

@asto18089

@asto18089 asto18089 commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator

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_script while every request silently lacked the tool.

Root cause

sanitize_moonshot_chat_tools drops any tool whose parameters fail Moonshot MFJS validation, with only a tracing::warn! on stderr (streaming turns also toast the omitted names) — nothing the model can see. MFJS fails empty schema nodes ({}) with MissingType, and the Pinvou browser catalog shipped exactly one:

"args": {"type": "array", "items": {}}

So tool_search kept advertising the reference and the activation cache kept admitting it, but the outbound tools array 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/fill mid-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

  1. fix(kimi): repair empty items instead of dropping — sanitize_kimi_parameters_candidate now removes empty items schema nodes before validation. JSON Schema reads an absent items as unconstrained, which is exactly what items: {} asserts, so the repair is semantics-preserving and MFJS-legal. Untyped nodes without an equivalent shape still fail closed: empty property schemas, empty anyOf branches (including branches the nested repair empties), empty $defs entries, annotation-only items, boolean or tuple-form items, items inside unsupported keywords (prefixItems, contains, multi-element nested allOf — a single-element allOf is flattened by the upstream union-collapsing pass and ships the equivalent repaired form), and minItems ≥ 1 beside an empty items keeps failing closed wherever the terminator checks (required properties, anyOf branches, $ref targets) — the error variant can differ (InvalidSchemaNode for the minItems case instead of MissingType) — while a non-required property position ships the equivalent repaired schema, where pre-repair it dropped. minItems: 0 beside an empty items is repaired: its only MFJS-legal shape terminates, and the repaired schema is equivalent.

  2. fix(kimi): repair nested empty items shapes — the walker now visits children before checking the node's own items, 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 root items beside $defs.

  3. fix(tool-search): report evicted activations — execute_tool_search_inner now returns evicted_tool_references plus a toolbox_note naming the evicted tools and the recovery contract (re-search to re-activate). Searches without evictions keep the legacy result shape unchanged.

  4. fix(tool-search): report every toolbox eviction — ToolActivationDelta separates stale entries (the tool left the deferred catalog: disconnect, deny rule, oversize) from bounds evictions. A search reports stale tools under unavailable_tool_references with 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_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/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.md documents the reporting channels.

  5. fix(tool-search): report toolbox drops truthfully — round-2 fresh-review fixes. revalidate now 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-activatable evicted entries 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 in stale and were dropped unreported. Op::SyncSession clears 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.md states the reporting contract as implemented, including the eager carve-out and the silent absorption of unavailable drops on non-search paths.

  6. 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 of evicted, 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.

  7. 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 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), so the comment and docs/TOOL_SURFACE.md now 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.

  8. fix(subagent): report child toolbox evictions — 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, 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.

  9. docs(tool-search): truthful toolbox contract — docs/TOOL_SURFACE.md restates 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.

  10. fix(tool-search): net parked evictions at ride time — round-4 fresh-review fixes. Netting previously lived only inside one ToolActivationDelta, 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.

  11. fix(subagent): report child evictions on error results — round-4 fresh-review fixes. The child's parked-note ride attached only to Ok results, so a note whose next result was an error stranded until a later Ok — or vanished when the child ended first; the parent engine rides error results too. The child's hydrate rejection 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 the Ok, Err, and fully-netted cases.

  12. 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, audited tool.schema_hydrated, and pushed the refused name into the hydration order, asking the activation the refusal exists to spare. The receipt is now a DeferredHydration::Refused arm — announcement, audit, and hydration-order push happen only on Loaded — and the refusal prose stops repeating the toolbox phrase the shared reason already states.

  13. test(kimi): pin the remaining empty-items repair visits — round-4 mutation testing proved the walker's object-form additionalProperties and anyOf-branch visits unpinned (deleting either left the module green); fixtures for both positions now fail with MissingType if a visit is dropped.

  14. 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. The SyncSession clear'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.

  15. 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.

  16. 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.md states the wider scope and a unit pin covers the refresh window.

  17. 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 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 receipt now carries the marker, pinned in the refusal test.

  18. 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.

  19. 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.

  20. 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 real plan_tool_calls whose 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.

  21. test(subagent): pin the note ride through the glue — round-5 fresh-review fixes. The child's parked-note ride is pinned through execute_from_surface itself; 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 tautological mem::take tail of the park test is removed.

  22. 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.

  23. 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).

  24. 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-outcome mcp_catalog_changed refresh — 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 through process_tool_results with a seeded needs-auth MCP pool whose refreshed list serves the synthetic eager authenticate tool.

  25. fix(engine): carry executed into tool.result audit — round-6 fresh-review fixes. The refusal receipt's executed: false marker reached the working set and the LSP hook but not the tool.result audit 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.

  26. 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.

  27. 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_tools adopted the pool's flags wholesale: every to_api_tools entry 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's mcp_catalog_changed refresh 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.

  28. test(engine): pin refusal audit executed marker — round-7 fresh-review fixes. The round-6 marker fix shipped unpinned: no test read the tool.result audit payload for the refusal path, so deleting "executed": tool_was_executed left the whole suite green. The pin drives the real refusal receipt from plan_tool_calls through the results phase with the audit log enabled and requires the marker verbatim (mutation-verified).

  29. 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 drive execute_from_surface itself, and the byte-tuned fixtures gain explicit warm-pair-fits-the-budget premise assertions.

  30. 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.

  31. test(kimi): pin minItems positions and fail-closed shapes — round-7 fresh-review fixes. Fixtures for both minItems positions (terminator-checked InvalidSchemaNode, non-required ships-the-equivalent, minItems: 0 repairs) and for the previously unfixtured fail-closed classes: empty anyOf branches (including one the nested repair itself empties), empty $defs entries, boolean and tuple-form items, each asserting its variant.

  32. 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 marked executed: false count as neither success nor failure.

  33. 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.

  34. 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_tools relocated 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-turn mcp_catalog_changed arm 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 from allowed_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 same ToolSurfacePolicy::allows_tool spelling the build and the boot window use, threaded from the turn loop into process_tool_results; new names keep the pool's eager flag, because the 401 result's auth hint points straight at the login tool.

  35. 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.

  36. 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.

  37. 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 through read_audit_log (plain success, plain failure, executed: false receipt, executed-marked success) and deleting the executed consult turns it red.

  38. test(kimi): pin the anyOf and ref minItems positions — round-8. The position-dependence fixtures covered the required-property position only; an anyOf branch and a $ref target, each reached from a required property, now pin the same InvalidSchemaNode variant.

  39. docs(tool-search): truthful gate prose and child entries — round-8. The Oversize cause gloss, exceeds_per_tool_budget's doc comment, and the hydration-site comment still said "schema" where the gate measures the whole serialized Tool (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

  • New tests: 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 with items repaired to absent).
  • Round-2 pins, each verified red against the pre-fix classification via hand-applied mutations and green after it: 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).
  • Round-3 pins, each verified red against the pre-fix behavior via hand-applied mutations this round: 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 live run_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.
  • Round-4 pins, each verified red against the pre-fix behavior via hand-applied mutations this round (7/7 mutants killed): toolbox_note_rendering_nets_re_admitted_names_at_ride_time and turn_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 direct plan_tool_calls), turn_entry_sweep_precedes_the_turn_start_park, and the additionalProperties/anyOf fixtures in kimi_parameters_repairs_nested_and_sibling_empty_items.
  • Round-5 pins, each verified red against the pre-fix behavior via hand-applied mutations this round (4/4 mutants killed): 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's executed: false metadata assertion (dropping the marker), and the active-set segment of toolbox_note_rendering_nets_re_admitted_names_at_ride_time (netting against the cache only).
  • Round-6 pin, rewritten by round-7 item 27 to the new truth: 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 by refresh_between_outcomes_cannot_resurrect_an_evicted_tool for the cross-outcome window.
  • Round-7 pins, each verified red against its regression via a hand-applied mutation: the two refresh pins above (blind insertion: 2/2 red), 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 of toolbox_note_rendering_nets_re_admitted_names_at_ride_time (removing the dedup: red).
  • Round-8 pins, each verified red against its regression via a hand-applied mutation: the splice-order phase of 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's allows_tool filter: red), and mid_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 untouched model_inventory module — ollama_default_prefers_live_local_tags_over_the_unresolved_marker, a provider_lake live-snapshot race under full-suite parallelism; it passes in isolation, 1/0) (one earlier full-suite run flaked the same two untouched runtime_threads tests it flakes under full-suite parallelism on this machine; both pass in isolation, 166/0).
  • cargo fmt --all -- --check clean; 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.
  • Live probe of the browser wrapper post-fix: tools/list serves 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.

@asto18089

Copy link
Copy Markdown
Collaborator Author

CI triage — which failures belong to this PR

None of them do, on current evidence:

  • The branch's own CI — Pinvou fork CI (fork-ci.yml, the workflow scoped to pinjou3-clean), DCO, Contribution gate and the issue-link gate — is green.
  • The failing checks (Integrations, Lint, Lint & Type Check, Test (windows-latest), Version drift, cargo-audit, cargo-deny (advisories)) all come from ci.yml/web.yml, whose triggers are scoped branches: [master, main]. Sibling PRs into pinjou3-clean (fix(session): Windows 会话导出探测失败关闭 #96–test(sandbox): ratchet-aware NoNewPrivs child matrices #98) don't run those workflows at all, so their failure modes were never exercised against this branch's content.
  • Locally, cargo clippy -p codewhale-tui --lib --tests on the pristine base (0bc868ff0, without this PR's commits) already fails under the current stable toolchain, with errors in files this PR does not touch: shell_dispatcher.rs:762 (chunks_exact constant chunk), js_execution.rs:386/428 and core/engine/tests.rs:20141 (eprintln!), core/engine/tests.rs:25056 (redundant closure). This PR's three files (schema_sanitize.rs, engine/tool_catalog.rs + its tests) produce no lints.
  • cargo-audit/cargo-deny (advisories) scan Cargo.lock, which this PR does not modify; the web-side failures (tsc, runtime web client tests) are outside its changed paths.

Happy to help split a follow-up that re-scopes or fixes the master/main-line workflows against pinjou3-clean content if maintainers want them enforced here — but that should not block this fix.

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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): every fetch_url/web_run/truncate result carrying evidence_available: true admits retrieve_tool_result and can LRU-evict a recently working tool with no model-visible note anywhere. Hydration admission (turn_loop.rs:4030-4038, child subagent/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 + early return leaves (a) {"type":"array","items":{"items":{}}} unrepaired (the inner node empties to {} after the outer check already ran → still MissingType), and (b) a root carrying items:{} plus a repairable $defs entry unrepaired ($defs is validated at :1625 before the root keyword check). Children-first recursion without the early return (the house style of sanitize_for_kimi:1113) fixes both. Fail-closed today, not a regression — but the same class as the incident shape.
  • m2 — toolbox_note can mis-attribute the cause (session.rs:104-107): activate() seeds delta.evicted with revalidate() 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#683 exists 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_warning covers 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 if TOOL_ACTIVATION_CACHE_MAX_NAMES changes; a const-derived format would be safer.
  • n2: the walker visits anyOf only; safe today because oneOf cannot survive to the pass, but one comment sentence would defuse the trap for future maintainers (sanitize_for_kimi walks 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_references reuses the callable tool_reference type; unavailable_tool_reference + an eviction reason would 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.

@asto18089

Copy link
Copy Markdown
Collaborator Author

Fix wave pushed (8558e6c + 1c33184) addressing the fresh-review findings (review comment 5486698768):

M1 — eviction honesty now covers the non-search paths. ToolActivationDelta splits stale entries (tool left the deferred catalog) from bounds evictions, and the three non-search activation paths report their bounds evictions to the model: batch hydration of directly-called deferred tools (the note rides the first recorded result of the batch, or its error), retrieve_tool_result admission on an evidence receipt, and the post-execution LRU refresh (both ride the triggering result). A child hydration receipt carries its own note. Known residue, disclosed: the child's post-execution touch and turn-start staleness revalidation stay unreported — the former is a follow-up candidate, the latter is not the "worked earlier, now uncallable" mode (the tool left the catalog itself).

m1 — walker order. remove_empty_mfjs_items_schemas now visits children first without the early return, repairing items: {"items": {}} and a root items beside $defs ({"items": X} is vacuously true for non-arrays, so the reorder stays semantics-preserving). The unexercised idempotency sentence in the doc comment is replaced with the real invariants.

m2 — cause accuracy. A search after a catalog shrink now reports the dropped activation under unavailable_tool_references with a catalog-shrink reason instead of the eviction note, whose "re-search to re-activate" contract was a false promise there. The toolbox_note is now only ever about bounds evictions, and its capacity label is derived from TOOL_ACTIVATION_CACHE_MAX_NAMES/_MAX_SCHEMA_BYTES so the prose cannot drift from the bounds.

m3 — body corrected (companion pinvou-agent#683 neither pins the gitlink nor postdates this PR; both fixed in the body).

m4 — test gaps closed: search_result_names_byte_bound_cache_evictions_too (16KiB-bound reporting), search_reports_catalog_stale_evictions_as_unavailable_not_evicted, kimi_parameters_repairs_nested_and_sibling_empty_items, moonshot_keeps_empty_items_tool_on_the_wire (wire-level retention of the incident shape), and the metadata toolbox_note copy is pinned. n1/n2/n3/n5 folded in (const-derived label, oneOf-invariant comment, doc wording, TOOL_SURFACE.md paragraph).

Verification receipts: cargo test -p codewhale-tui --lib → test result: ok. 12026 passed; 0 failed; 13 ignored; cargo fmt --all -- --check clean; clippy with the fork-CI flag set clean on codewhale-tui; PR head 1c331846ef, MERGEABLE. Not changed (taste call, per review): evicted_tool_references keeps its dedicated field rather than reusing unavailable_tool_reference, and the pre-existing hardcoded capacity prose in the over-budget rejection reason is left as-is.

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_references entries in one search: retain drops it into stale (reason claims it left the catalog — it didn't) while discovery re-rejects it into cache_rejected with the truthful oversize reason (tool_catalog.rs:1437-1453).
  • docs/TOOL_SURFACE.md's "surfaced as unavailable references instead" holds only via a later tool_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-request revalidate (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:2584 test comment narrates development history ("used to leave $defs unrepaired") instead of stating the invariant; TOOLBOX_STALE_EVICTION_REASON can 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 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix wave pushed as 2bb381d74 addressing all five MAJORs from the round-2 review above:

  • MAJOR 1+2+3 (one coherent fix): revalidate now 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-activatable evicted bucket. With the split in place the post-execution refresh channel comes alive: touch_cached_tool_after_execution genuinely 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::SyncSession clears pending_toolbox_eviction_note next 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 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@/tmp/cw101-review-r3.md

@asto18089

Copy link
Copy Markdown
Collaborator Author

@/tmp/cw101-fixwave-r3.md

@asto18089 asto18089 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. An MCP refresh grows cached tool X's schema between turns; turn-start revalidation bounds-evicts X and parks the note (turn_loop.rs:1338-1347); X leaves active_tool_names.
  2. The model's first batch directly calls X — the natural retry, i.e. exactly the incident behavior this PR fixes. maybe_hydrate_requested_deferred_tool passes (deferred, inactive, under the per-tool cap), the receipt "Tool X was deferred and has now been loaded. Retry the call…" is returned, plan_tool_calls re-admits X (turn_loop.rs:4040-4043), and the receipt becomes a recorded outcome (turn_loop.rs:4259-4276). A first-batch tool_search matching X reaches the same contradiction through tool_references.
  3. process_tool_results appends the parked note to that receipt: one payload now says X "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_status is computed before maybe_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 into deferred_tools_hydrated_in_order (3932), emits the tool.schema_hydrated audit 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 at tool_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 and activate rejects it (inert, but the stated invariant doesn't hold).
  • m2 — child hydrate Err path absorbs revalidation evictions. hydrate() runs cache.activate before the oversize verdict (subagent/mod.rs:14204); on the not-admitted path (14230-14233) the activation's bounds_evicted vanish — 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 Ok only (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 on Err like the parent or scope the sentence.
  • m4 — mutation-proven pin gaps (hand-applied mutants this round): (a) deleting the object-form additionalProperties visit in remove_empty_mfjs_items_schemas leaves all 62 schema_sanitize tests and all moonshot wire tests green — that position (and the anyOf-branch visit, same shape) is unpinned; (b) removing only the SyncSession note-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-445 vs turn_loop.rs:1347).
  • session.rs:296-301 field doc describes only the hydration park; turn-start now parks there too.
  • Body wording: the minItems-beside-empty-items case now fails InvalidSchemaNode, not MissingType (same drop outcome, different class than "the same error classes as before"); and "only a tracing::warn!" — the streaming path also raises a user-visible toast via omitted_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/combine is byte-identical parent/child (turn_loop.rs:4919-4929 vs subagent/mod.rs:14136-14145) — one shared helper would pin the mirror; remove_empty_mfjs_items_schemas is the third hand-rolled child-visitor in schema_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_request is 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.

@asto18089

asto18089 commented Oct 12, 2026 •

Copy link
Copy Markdown
Collaborator Author

Round-4 fix wave pushed (7afa8bf..1834654, 6 commits), addressing every actionable finding from the round-4 review:

  • MAJOR-1 (144bca1b6): parked notes hold structured names and are netted against the live activation cache at attach time — parent success/error paths and the child surface. A note emptied by the netting is dropped. Trigger is now an enum; park/combine is one shared helper.
  • MAJOR-2 (18346547c): TOOL_SURFACE.md now states the best-effort reporting contract, the drop edges (sweep, session sync, child end), and the ride-time netting; the child channel gains "(or its error)".
  • m1 (1e8b3f6ad): a refused hydration no longer emits the "Loaded deferred tool" status, the tool.schema_hydrated audit, or the hydration-order push — DeferredHydration::Refused is its own arm; refusal prose deduplicated.
  • m2+m3 (a552e485c): the child rides parked notes on error results (parent parity), and the hydrate rejection carries its activation's revalidation evictions beside the shared oversize reason.
  • m4 (2eb3607f1, f3ffb8d1c): mutation-proven unpinned spots now pinned — additionalProperties and anyOf walker visits, and sweep-before-park ordering at turn entry. The SyncSession clear is documented as defense-in-depth covered by the composed mutant (its effect is not observable between the sync and the next turn, so no single-mutant pin is possible).

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 18346547c: cargo fmt --all -- --check clean; fork-CI clippy clean; cargo test -p codewhale-tui --lib 12050 passed / 0 failed / 13 ignored.

Not addressed, deliberately: the warm-start park in SubAgentToolSurface::new stays production-dead-but-defensive (mirrors the parent, costs nothing); the third schema walker and the metadata shape inconsistency in evicted_tool_references are deferred as noted in the review.

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>
@asto18089
asto18089 force-pushed the fix/kimi-mfjs-empty-items-drop branch from 1834654 to 133bdba Compare October 13, 2026 01:28
@asto18089

Copy link
Copy Markdown
Collaborator Author

Round-5 fresh review (13-way, post-rebase)

Rebase: rebased onto pinjou3-clean@3118c36b8 (which now carries main plus #90/#91/#93/#97/#98). Zero textual conflicts; the PR's own hunks are byte-identical pre/post rebase (per-file diff-of-diffs verified — only index lines and hunk offsets moved), the 16 commits map 1:1, and the combined tree compiles. I also checked the five new base commits for semantic interaction: all clean — #97's terminal-event attribution and #91's thinking preservation live in code regions the note machinery never reads or writes, and #90's new "activate via tool_search" guidance is consistent with this PR's recovery contract. CI is green on the rebased head.

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 items can strand unrepaired), every non-repaired shape still fails closed, the stale/evicted classification is correct at every cache-removal site, netting is sound at both delta and ride time, the no-eviction search payload is byte-identical to base, the refused-hydration arm fully suppresses status/audit/order-push, per-turn cost when no deferred tools exist is effectively nil, and every hunk of all 16 commits traces to a numbered claim — no smuggled changes. docs/TOOL_SURFACE.md survived a sentence-by-sentence adversarial check against the code, including the best-effort drop edges.

MAJOR (coverage, mutation-verified)

M1 — the parent hydration-park call site survives deletion; "pinned transitively" overstates. crates/tui/src/core/engine/turn_loop.rs:4173 (park_toolbox_eviction_note(Trigger::Step, &activation.evicted) after the batch activation) is load-bearing in no test: every engine-side note-ride test parks synthetically via park_toolbox_eviction_note(...) by hand, and the only new test that drives a real plan_tool_calls (refused_hydration_emits_no_load_status_and_never_enters_the_batch) hits the refusal arm, which produces zero evictions. Verified by mutation: deleting the call leaves the full lib suite 12121 passed / 0 failed. The PR body's "the hydration parking call site is pinned transitively (the shared park/combine helper is tested directly and the consumption channels end-to-end)" describes real pins of the helper and the consumers — but the connecting production edge is uncovered, and channel #4 (the PR's headline hydration channel) can be silently killed for the parent while the child's twin (SubAgentToolSurface::hydrate) is pinned through a real hydration. One test porting the child fixture (a plan_tool_calls hydration that overflows the cache) closes it.

M2 — the child's ride glue is unpinned. surface.ride_pending_eviction_notes(&mut result) in execute_from_surface (crates/tui/src/tools/subagent/mod.rs:15694) is the production edge the round-2 fix created (previously the child discarded every eviction report); all five new child tests call the SubAgentToolSurface methods directly and never go through execute_from_surface. Verified by mutation: deleting the ride call (touch dispatch left intact) leaves the full lib suite 12121 passed / 0 failed — so a regression back to "the child silently discards parked reports", the exact defect shape this PR fixes, would pass today's suite. (For precision: the touch dispatch at :15731 is pinned — the pre-existing small_surface_successful_cached_use_touches_lru fails when it is removed; it pins LRU retention, not the eviction-note emission.) One test driving execute_from_surface with a parked note closes it.

MINOR

m1 — the refusal receipt is missing "executed": false metadata (introduced by this diff). deferred_tool_oversize_hydration_result returns a bare ToolResult::success(...); the sibling Loaded receipt sets "executed": false, and the consumer (turn_loop.rs:5143) defaults executed to true — so an oversize refusal is observed by working_set.observe_tool_call as if the tool ran and is audited as tool.result with success: true and no executed marker. Model-facing text is truthful ("was not loaded and was not executed"); this is bookkeeping/audit inconsistency, one line to fix.

m2 — ride-time netting consults cache membership only, so one window contradicts the doc's absolute promise. render_toolbox_eviction_notes nets via ToolActivationCache::contains; a name parked as evicted that the same outcome's MCP catalog refresh re-advertises (replace_runtime_mcp_tools inserts into the active set without touching the cache; same shape on the child's request_tools) is callable again while the riding result still says "no longer callable until a later search re-activates them". Narrow window, self-correcting on retry, but it contradicts the new docs/TOOL_SURFACE.md sentence "A note never names a tool the model can call again." Either consult the active set/catalog at ride time or soften that one sentence.

m3 (coverage polish) — in refused_hydration_emits_no_load_status_and_never_enters_the_batch, !active.contains("notify") cannot fail independently (the admission gate already rejects oversize tools, so a mutant that wrongly pushes the refused name into the hydration order still never lands in active_tool_names), and the "no tool.schema_hydrated audit on refusal" leg is asserted nowhere. The receipt-prose and status-drain assertions are strong; the audit leg would need one assertion on the emitted events.

m4 (pre-existing, disclosed for completeness) — blocking create_message never reads omitted_tool_names; only the streaming path toasts dropped tools. Unreachable today (the turn loop is streaming-only, aux blocking calls carry no tools), and outside this PR's eviction claims, but it is the same silent-drop class one door over.

Nits (at author's discretion)

  • PR body prose: the fail-closed enumeration omits empty anyOf-branch and $defs empty-node cases (all still fail closed — the omission is in the conservative direction), and the minItems clause doesn't note that minItems: 0 beside empty items is now repaired (only minItems ≥ 1 still drops); commit 2816d955c (two round-3 pins) has no numbered claim in the 15-claim list; a three-line comment reword in a kimi test fixture rode in 4628c9a2a.
  • tool_catalog.rs:1536-1553: the stale/rejected dedup filters by name and keeps the rejected entry's reason — correct only because rejections can today only mean oversize (discovery only returns catalog members). A cause-aware filter would make the invariant local.
  • toolbox_oversize_reason says "The tool schema exceeds…" but the gate measures the whole serialized Tool (name + description + schema) — byte-identical to the base's wording, so pre-existing imprecision the PR relocated; "serialized" would make it exact. The direct-call refusal likewise ends with "Narrow the search…" although no search occurred (the child's mirror correctly omits it).
  • A name can appear in two notes on one result in a contrived same-batch admit-then-evict window (parked note + evicted_this_result note), and a stale CatalogShrink entry can duplicate a fallback name in unavailable_tool_references — both truthful but redundant.
  • The child warm-start park (SubAgentToolSurface::new with warm names) is production-unreachable — the only constructor call passes &[]; presented in the body as a live channel, it is defensive-only today.
  • dropped_names() is duplicated verbatim on ToolActivationDelta and ToolActivationRemovals; turn_loop.rs:5287 re-parks by assignment rather than extend (safe today, would silently drop a future concurrent park); test nits: hardcoded 16 KiB literals in engine-test fixtures, the heavy closure duplicated 4× in tool_catalog tests, the mock-engine fixture block duplicated 5× in engine tests, the wire test could assert the surviving tool's name (its sibling drop-path test does), and one mem::take-then-assert-empty tail in child_parked_note_rides_the_next_result_and_refresh_parks_one tests Rust, not production.
  • Doc precision: the sync-edge drop actually happens at the sync op, not turn entry; the catalog-shrink cause gloss "(disconnect, deny rule, mode switch)" is non-exhaustive (interpreter-probe loss and policy refiltering also shrink); "the next time a search matches the tool" should be "matches and returns the tool" (a match ranked below max_results re-reports nothing); the child channel itemization omits the hydrate-refusal carrier and the warm-start park (both subsumed by the paragraph's opening sentence).

What was verified clean this round

Root-cause repair (semantics-preserving under both draft generations; boolean/tuple-form items provably untouched; children-before-self ordering pinned by the grid fixture; the four in-file repair tests are genuine mutation-sensitive pins including the end-to-end wire pin through the real Moonshot client and serializer); classification (all ten consumer sites route stale/evicted/eager buckets truthfully; no re-activatable eviction is reported unavailable and vice versa); netting (delta-level and ride-time both net against provably-live cache membership; cache ⊆ active invariant holds on both surfaces); payload compatibility (no-eviction shape byte-identical to base; all sections bounded); refused arm (no status, no audit, no order push, nothing queued); turn integration (sweep strictly before the turn-start park — pinned; ride attaches only to the first recorded tool result on success and error; interrupt records cancelled results so the note still delivers; panic recovery is the next-turn sweep as documented; the note is not serialized into saved sessions); state design (one shared park/combine helper reused by both surfaces, zero raw writes, append-union combine with no overwrite); performance (no new hot-path cost — early-exit guards everywhere, bounded sorts/dedups only); scope (every hunk of 16 commits traces to a claim; no file outside the declared set; no public-API/serialization impact; the one rename is exactly the rename); and no cross-doc contradiction introduced.

Recommendation: land one parent test through a real plan_tool_calls hydration overflow (M1), one child test through execute_from_surface with a parked note (M2), and ideally the executed: false one-liner (m1) plus the m2 doc/code alignment; everything else is optional polish. After M1/M2 the coverage story matches the body's claims and I'd recommend merge.

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>
@asto18089

Copy link
Copy Markdown
Collaborator Author

@/tmp/pr101-round8-fixwave.md

@asto18089 asto18089 left a comment •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.result lines 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_LOG at the file metrics reads), the top-level executed misses both pointers, defaults to true, and the refused oversize hydration counts as a success — exactly the inflation item 32 claims to remove (same for a hydration receipt). The success field 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 a details-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

  1. 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 with InvalidSchemaNode. 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.
  2. touch_glue_rides_the_refresh_eviction_through_execute_from_surface lacks the warm-pair premise assert its byte-tuned siblings carry (subagent/tests.rs:7913-7917 vs 7628-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.
  3. 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_names is unchanged, nothing is declared — and the next request's content hash drifts undeclared (turn_loop.rs:1082,1113-1128 vs prefix_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.
  4. The neither-bucket contract doesn't match its own else if. Comment and body say executed: false counts as neither unconditionally; else if !success (metrics.rs:343-347) buckets an executed: false ∧ success: false receipt as a failure. Unreachable from current writers (both executed:false receipts are successes); latent trap if a future writer ever marks a refusal as failed — else if executed would match the documented contract.

m3

  • Item 32's stated motivation still holds against the sibling metrics sources left untouched: read_session_files buckets the persisted refusal receipt (a non-error tool_result) as a success, and read_runtime_events counts tool.completed unconditionally (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 invalid minItems as 0 (schema_sanitize.rs:2017) — safe only through the call-ordering invariant that validate_mfjs_bounds rejects it earlier; nothing pins that ordering.
  • A duplicate name inside refreshed would 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 deferred catalog.first() without caching it; a refresh (which re-advertises by activation_cache.contains only) 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 uses TurnStart for the boundary; the ToolboxEvictionTrigger::Step doc 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_surface debug 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 the anyhow chain (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; --numstat exactly 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 --check clean 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, minItems position matrix, single-element-allOf flattening, and the $defs/nested-repair claims all trace to code and pins; additionalProperties/not/oneOf/if-then-else/patternProperties/propertyNames/contains/unevaluatedItems/$ref cycles 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.

@asto18089
asto18089 merged commit 6ba52af into pinvou3-clean Oct 13, 2026
15 checks passed
asto18089 added a commit to Pinvou/pinvou-agent that referenced this pull request Oct 13, 2026
将 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>
asto18089 added a commit that referenced this pull request Oct 14, 2026
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>
asto18089 added a commit that referenced this pull request Oct 14, 2026
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>
asto18089 added a commit that referenced this pull request Oct 14, 2026
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>
asto18089 added a commit that referenced this pull request Oct 14, 2026
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>
asto18089 added a commit that referenced this pull request Oct 14, 2026
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>
asto18089 added a commit that referenced this pull request Oct 14, 2026
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>
asto18089 added a commit that referenced this pull request Oct 14, 2026
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>
asto18089 added a commit that referenced this pull request Oct 14, 2026
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>
asto18089 added a commit that referenced this pull request Oct 14, 2026
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>
asto18089 added a commit that referenced this pull request Oct 14, 2026
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>
asto18089 added a commit that referenced this pull request Oct 14, 2026
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>
asto18089 added a commit that referenced this pull request Oct 14, 2026
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>
asto18089 added a commit that referenced this pull request Oct 14, 2026
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>
asto18089 added a commit that referenced this pull request Oct 14, 2026
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>
asto18089 added a commit that referenced this pull request Oct 14, 2026
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>
asto18089 added a commit that referenced this pull request Oct 14, 2026
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>
asto18089 added a commit that referenced this pull request Oct 15, 2026
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>
asto18089 added a commit that referenced this pull request Oct 15, 2026
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>
asto18089 added a commit that referenced this pull request Oct 15, 2026
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>
asto18089 added a commit that referenced this pull request Oct 15, 2026
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>
asto18089 added a commit that referenced this pull request Oct 15, 2026
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>
asto18089 added a commit that referenced this pull request Oct 15, 2026
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>
asto18089 added a commit that referenced this pull request Oct 15, 2026
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>
asto18089 added a commit that referenced this pull request Oct 15, 2026
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants