Repository navigation
fix(coding-agent): keep the tool block stable across plan mode - #296
Merged
Merged
Conversation
Plan mode narrowed the active tool list by dropping the editing aliases and re-appending the plan tools. A real Step profile already offers every plan tool, so that rewrite produced the same set in a different order: 42 tools, 21 positions changed, no capability gained or lost. The provider serializes `tools` ahead of the system prompt and messages, so the request's whole cached prefix is keyed on that block. Entering plan mode therefore invalidated the cache for the entire conversation and re-billed the full prompt. Measured on a live session, one entry at a 233k-token prompt read back 1.6k tokens (0.7%), and an agent-initiated entry cost another 42k. Plan mode is a permission narrowing, not a capability change. The `tool_call` hook already denies mutations outside the plan file, so the editing tools do not need to disappear. Leave the active list untouched when it already covers the plan tools; only a profile that genuinely lacks one gets it appended, which the exit path reverts. Exiting and branch restore also skip reapplying an identical list so a byte-identical prefix is never rebuilt. Covered by a suite regression that captures the request tool block and system prompt across an entry, and by extension tests for the append-only case, the unchanged-order case, and the remaining write guard.
liuedcson
requested review from
MelodyVAR,
Uking-xxx and
ZouR-Ma
as code owners
October 23, 2026 04:33
Review follow-ups for the cache-stable tool block. An entry that added nothing stopped writing `toolsBeforePlanMode`, but restore still searched every entry for one. A later plan session could therefore replay an older session's snapshot: the list was reordered and tools gained since were dropped, which is the same prefix invalidation this change set out to remove. Entries now carry `version: 2`, and a baseline is only replayed when the entry that narrowed the list is a legacy one. Exit now subtracts the tools entering actually appended instead of replaying a snapshot, so a tool activated mid-plan (a late MCP catalog, find_tools) keeps its place. The plan/session guard resolves both paths through `resolveToCwd`, the same helper the file tools use, so `~`, `@`, and unicode-space inputs cannot pass the check and then land elsewhere. The denial now states that plan mode is a permission restriction, forbids retrying or routing around it through a shell, and points at the plan file and `exit_plan_mode`. Tests: byte-identical prefix across a full entry/write/exit cycle (previously only entry was covered), no persisted baseline when entry adds nothing, no reapply on a new-format restore, a stale legacy baseline not replayed, and a mid-plan tool surviving exit. All 17 plan-mode cases fail against `main` and pass here.
Review follow-ups. The plan-mode tool block is now the only thing that changes nothing while the guard does all the enforcement, so the guard needs coverage and the legacy restore needs to stop losing state. - Cover the path normalization the guard depends on. Reverting the comparison to `path.resolve` previously kept every test green; the entry loop now accepts `@`-prefixed, absolute, and `..`-spelled forms of the plan path and rejects a leading-space path, an `@`-prefixed non-plan path, and a traversal out of the plan file for all four editing tools. - Carry a legacy tool snapshot through a todo migration. Rewriting the entry used to drop the only record of the list plan mode narrowed, which stranded the branch on the next restore. - Replay only the baseline the governing entry itself carries, so a newer plan session never inherits an earlier one's list. - Align the denial with the documented policy: `run_command` keeps its normal permissions, so the message tells the model to stay read-only instead of claiming a shell workaround is blocked. - Guard a null tool input, and stop the docs from claiming every file-writing tool is covered — the check names the four editing tools, and MCP tools are outside it. - Remove a leftover debug log and base the regression's working directory on the harness workspace rather than the test process. All 184 plan-related cases pass; `tsgo --noEmit`, `biome check`, `check:ts-imports`, `check:layer-direction`, `check:public-boundary`, and `check:coding-agent-entry-freeze` are clean.
liuedcson
added a commit
that referenced
this pull request
Oct 23, 2026
Brings the seven commits main gained since dff413f: - feat(coding-agent): make a feedback report investigable (#297) - fix(coding-agent): settle extension command turns before returning (#298) - fix(coding-agent): keep the tool block stable across plan mode (#296) - fix(cli): save model selection as default on Enter (#285) - feat(cli): add -C/--cwd flag (#289) - fix(coding-agent): preserve task-state prompt cache prefixes (#293) Two conflicts, both in the plan extension, where this branch had added behaviour on top of files main rewrote: - `plan-mode-tools.ts`: main's comment for `exitPlanMode` ("restore any tools plan mode added") with this branch's optional `submittedPlanHash` parameter, which records the proposal handed to an external reviewer. - `step-plan.ts`: main's `version: 2` entry and `carriesLegacyTools` migration path, plus this branch's `submittedPlanHash` / `planHashAtEnter` on the same entry, and both sides of the restore step (legacy tool baseline and the persisted hashes). Main's `resolveToCwd` guard and `planModeAddedTools` bookkeeping are kept as-is: plan mode now appends tools instead of replacing the list, which is what keeps the request's tool block stable. The host plan-mode test asserted the old refusal wording; the guard now says "Only the plan file may be written", so it matches on that.
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
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.
Problem
Entering plan mode rewrote the active tool list: it dropped the editing aliases and re-appended the plan tools. A real Step profile already offers every plan tool, so the rewrite produced the same 42 tools in a different order — 21 positions changed, no capability gained or lost.
The provider serializes
toolsahead of the system prompt and messages, so the request's entire cached prefix is keyed on that block. Entering plan mode therefore invalidated the prompt cache for the whole conversation.Measured on a live session (188 requests,
stepcode-anthropic/deepseek-v4.1-flash-api):/planentrySame-turn requests outside those events held 98–99.9% for the whole session. The two entries alone re-billed 273k tokens, and one of them was the model calling
enter_plan_modeon its own, so it is not avoidable by asking users not to use plan mode.Change
Plan mode is a permission narrowing, not a capability change. The
tool_callhook already denies mutations outside the plan file, so the editing tools do not need to disappear from the list.enable()no longer rebuilds the list. It appends only plan tools the profile is missing, and leaves an already-complete list untouched.find_tools) keeps its place.version: 2and store no replaced list. A legacy baseline is replayed only when the entry that narrowed the list is a legacy one, so a newer plan session can no longer replay an older session's snapshot and reorder or drop tools.resolveToCwd— the helper the file tools themselves use — so~,@, and unicode-space inputs cannot pass the check and then land elsewhere.exit_plan_mode.This matches how Kimi Code implements plan mode (a
toolExecutor.onBeforeExecuteToolveto, with no tool-list mutation) and MiniMax Code (only the two lifecycle tools change).Verification
plan-mode-prompt-cache-prefix.test.tscaptures the actual request tool block and system prompt across a full entry → write plan → exit cycle and asserts all three requests are byte-identical; it also asserts a non-plan write is still denied.setActiveTools, no persisted baseline), a new-format restore (nothing reapplied), a stale legacy baseline (not replayed), and a mid-plan tool surviving exit.mainand pass here.tsgo --noEmitclean;biome checkclean;check:ts-imports,check:layer-direction,check:coding-agent-entry-freeze,check:no-observability,check:public-boundarypass.Full
vitest --runin this worktree reports 7 failures, all pre-existing and unrelated: 6 intest/completion-check-cli.test.ts(also fails on an unmodifiedorigin/mainworktree) and 1 intest/suite/regressions/2791-fswatch-error-crash.test.ts(spawns the CLI fromdist/, which this worktree has not built).Known gap (not addressed here)
For user-initiated
/plan, the model learns plan mode is on only when a write is denied — the extension injects nothing into context, and the tool list no longer changes. Agent-initiated entry is unaffected (enter_plan_modereturns the mode in its result). A cache-safe tail reminder would close the gap, but it changes request content every turn, so it belongs in its own change rather than this fix.Docs
docs/plan-approval.mdandpackages/coding-agent/docs/step-integration.mdnow describe the append-only restore contract and theversion: 2snapshots.