fix: stop declaring the whole tool world, and page the biggest result - #110
Merged
Conversation
Before narrowing the active tool list per stage, the allowed set has to be a thing that cannot change by accident. Today it can: a tool dropped from a step's list produces no error a model can act on — the call is refused and the step has no way forward, so the only symptom is a run that stops. Writes the whole matrix out as a literal (65 assertions, one per stage/step plus the state transitions) rather than recomputing it from the table under test. Order is pinned too, not just membership: if the active list is ever narrowed to this set, the matrix becomes the head of the prompt, and a reordering re-reads the whole prefix on any backend that caches one. Also pins what appears and disappears without the step changing: the four planner_debug_* wrappers while a stuck debug session is open, the recovery set while broken or awaiting a user decision, planner_elenchus_check at the repair decision only, and the read-only pair while a compact boundary is pending. Two findings, recorded as assertions rather than fixed here: - planner_create_plan is reachable from no step at all. It is offered by the preflight decision before a plan exists (STATUS_ONLY_TOOLS), so a naive "every wrapper must appear in some step" check would call it dead. - planner_report_stuck is allowed on 7 execution steps but the debug wrappers are listed on only 6. At contract_check the model can report stuck — which opens a debug session — and then cannot call a single planner_debug_* tool, including the cleanup that closes it. Nothing gates on the leftover session, so the run is stranded rather than deadlocked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…listing them planner_report_stuck unconditionally opens a debug session (initializePlannerDebugSession), so the stuck report and the four planner_debug_* wrappers are two halves of one flow. They were maintained as separate hand-written lists in two places, and both had drifted at the same step. At execution/contract_check the model could report stuck — opening a session and writing a README into the artifacts directory — and then call none of the debug wrappers. That is not merely a missing feature: the open directory makes assertNoPlannerDebugArtifactsBeforeCommit block planner_git_commit, and its error message tells the model to run planner_debug_cleanup, which the step refuses. The run escapes only via finish_step (commitPolicy there is allowed_if_dirty, not required), i.e. by ignoring the instruction it was just given, while already stuck. The guard now derives them — offered exactly when a session is open AND the step allows planner_report_stuck — so the two halves cannot drift again. The stage-behavior expectedTools list is still per-step, but its drift is already caught by tool-gating-invariant.test.ts, which failed on this change and is what surfaced the second list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…he world updateToolVisibility rebuilt the active list from getAllTools() on every before_provider_request. setActiveTools is a global setter with no notion of whose tools are whose, so that resurrected every tool another extension had just hidden — and at least one other extension in the same session does the same thing, from the other side. Two extensions each declaring the whole world, on every request. The cost lands where it is hardest to see. Tool schemas sit at the head of the prompt, and a prefix cache reuses exactly one thing: bytes it has already read. A head that changes between two calls of one turn is a whole prompt re-read; measured elsewhere on this setup at prefill 24 302 -> 11 653 with the cache going 24 348 -> 0. So the list is now a subtraction from what is already active: next = (active now u what we claim) \ what we hide Nobody else's decision is overwritten, and it converges — our claim/hide sets are a pure function of planner state, so recomputing over the output yields the same list. A stable set of tools is a stable head. Order comes from walking the registry, since order is part of those bytes. The contract gate stays a sandbox rather than a subtraction: while it is up the model must not reach project reads at all. That needs the world remembered on the way in, because the live list IS the sandbox while it is up — subtracting from it on the way out would leave every other extension's tools hidden for good. Both new properties were checked against the old behaviour: reverting the subtraction fails the resurrection test, and dropping the remembered world fails both sandbox-exit tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Narrowing the active tool list per stage would save real tokens — 54 planner schemas are ~13k of a fixed ~24k head on this project's own session, ~18% of a 131072 window that no compaction can reclaim. But it is a deliberate head change, and every serving backend reuses exactly one thing: a prefix of bytes it has already read. Deciding that without measuring is precisely how the compaction floor removed in 1b60f0d was built. So: measure first. `runtime/prefix-watch.ts` reduces each provider payload to what a prefix cache cares about (system prompt + serialized tool schemas, their fingerprint, the tools in the order offered) and reports when the head moved. The report separates two different questions. WHEN: between runs a changed tool set is expected; between two calls of one run nothing about the model's situation changed and the prefix was thrown away for nothing. WHO: setActiveTools is a global setter and this extension is not the only writer, so record() takes the list we last set. Our own mid-run changes are a cost chosen with open eyes, not news — an alarm that cannot recognise its owner's footsteps is one nobody keeps armed. Only mid-run AND foreign is a defect, surfaced as one ctx.ui.notify per run, locally, with nothing sent anywhere. Ported from pi-telegram-manager's core/payload-probe.ts, which is where this class of bug was first named — and where this extension was the one named. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ng it The listing is one line per file in the HEAD commit, and nothing bounded it. A repository that committed its build output turned that into a single tool result of 79 802 characters; across one measured session planner_contract_check spent 215 211 characters in eight calls — 31.5% of everything the model read from tools. A result that size cannot be re-read, cannot be summarized away, and dominates the next compaction's cut point. So the map is paged rather than silently truncated: eight directories and ten files each, with the count of what is missing and the exact call that shows it. Paging is the model's decision, because a listing this large is usually noise and the one directory it needs is reachable directly — coveragePath takes a directory, or a file whose directory is shown. A page turn re-sends the required fields, which makes it indistinguishable from a second check except by what is already stored. So the stored check decides: matching task, action and summary means the tdd.md append already happened. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hind it PrefixWatch shipped a churn history, a defect filter and a ring buffer for a panel that does not exist. A grep for a reader outside the module finds none: index.ts calls runStarted() and record(), reads the returned churn once, and notifies. Everything else was a surface built for a future caller. What stays is the alarm, and it stays for a reason that stopped being hypothetical: setActiveTools is a global setter, and this session runs pi-telegram-manager, confirmed by four compactions it answered through session_before_compact. A mid-run head somebody else wrote is still worth naming, and the watch now holds exactly what naming it requires — the previous shape. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL is right: /[/\\]+$/ is a repeated character class anchored at the end, so a failed match backtracks from every position and a path that is mostly separators costs quadratic time. coveragePath arrives as a tool argument, which is exactly the input this must not be quadratic in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.