fix(core): redact the plan argument from history after an approved exit_plan_mode#7197
fix(core): redact the plan argument from history after an approved exit_plan_mode#7197zjunothing wants to merge 8 commits into
Conversation
…it_plan_mode The full plan text a model submits to exit_plan_mode stays in the conversation history as its own functionCall arguments. On long conversations models occasionally regurgitate chunks of that blob into later responses, mixing stale plan sections into answers (QwenLM#6237). After an APPROVED exit (keyed off the approval llmContent prefixes, so rejected/no-action results keep their plan text for revision), the tool scheduler now swaps the `plan` argument in the model turn's functionCall for a short pointer to the plan file that Config.savePlan already persisted. The rewrite is a targeted, immutable replacement by callId in GeminiChat history: sibling parts and other arguments survive, and the partial-push markers are unaffected (they compare by index and role). returnDisplay.plan on the UI side is untouched. Verified with a PTY E2E against a local fake OpenAI-compatible server inspecting the exit_plan_mode arguments of every outgoing request: before, the full plan text rides along on every post-approval request; after, those requests carry only the reference. The scheduler regression test fails on the unpatched source. Known limit: the chat-recording JSONL keeps the original args, so a resumed session re-feeds the full plan text until its next approved exit. Fixes QwenLM#6237 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Verification reportE2E (PTY drives the real
Row 2 matters: the rewrite runs before
Tests (packages/core, 550/550 across the three touched files):
Static checks: Review focus suggestions: (1) the approval detection — keyed off 中文版本E2E（PTY 驱动真实 CLI 测试（packages/core 三文件 550/550）：调度器回归测试「批准后改写 plan 参数」在未修复源码上失败（git stash 验证）；「未批准时保留 plan 参数」覆盖拒绝路径； 审阅要点：(1) 批准判定依赖 |
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with solid evidence. #6237 documents the plan text leaking into subsequent model responses after Direction: aligned. This is the structural fix the maintainer proposed on the issue — replace the in-context plan blob with a file pointer after approval. Clean trade: plan text leaves the attention window, full text stays on disk via Size: core paths touched ( Approach: the scope feels right — one focused change, no scope creep. The three-layer separation is clean: the approval-detection constant lives in Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的 bug，有充分证据。#6237 记录了 方向：对齐。这正是 maintainer 在 issue 中提出的结构性修复——批准后用文件指针替换上下文中的 plan 大文本。权衡合理：plan 文本离开注意力窗口，完整文本通过 规模：触及核心路径（ 方案：范围合理——专注一个改动，无范围蔓延。三层分离清晰：批准判定常量在 进入代码审查 🔍 — Qwen Code · qwen3.7-max Reviewed at |
Code ReviewIndependent proposal (before reading the diff): I'd add a targeted method on The PR's implementation matches this exactly, and handles edge cases I'd have missed:
No correctness bugs, security holes, or AGENTS.md violations found. Tests
Real-Scenario TestingThe core behavior (plan text redaction from API request payloads) is not TUI-visible — it requires inspecting HTTP traffic, which the author did with a PTY harness + fake API server (evidence in the PR body). The tmux test here is a CLI smoke test confirming both builds are functional: Before (installed build)After (this PR via
|
|
Confidence: 5/5 — clean across every stage; focused fix for a confirmed bug with correct implementation and thorough test coverage. This is a well-scoped structural fix that does exactly what the maintainer proposed on #6237. The approach — replacing the in-context plan blob with a file pointer after approval — is the right trade: the plan text leaves the model's attention window, the full text remains on disk via The implementation is tight: ~100 production lines across 3 files, no scope creep, no unnecessary abstractions. The three-layer design (constant in Tests cover both the happy path (approved → redacted) and the rejection path (kept for revision), at both the unit level ( The known limitation (chat-recording JSONL keeps original args, so LGTM. ✅ 中文说明置信度：5/5 — 各阶段均通过；聚焦修复已确认的 bug，实现正确，测试覆盖充分。 这是一个范围合理的结构性修复，完全落地了 maintainer 在 #6237 中提出的方案——批准后用文件指针替换上下文中的 plan 大文本。权衡合理：plan 文本离开模型注意力窗口，完整文本通过 实现紧凑：3 个文件约 100 行生产代码，无范围蔓延，无不必要的抽象。三层设计（ 测试覆盖了批准路径（改写）和拒绝路径（保留供修订），包括单测（ 已知局限（chat-recording JSONL 保留原始参数， LGTM. ✅ — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
added
category/core
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: verification — a verifier ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and the posted findings cannot be counted as verified against it.
— qwen3.7-max via Qwen Code /review
| try { | ||
| const planPath = this.config.getPlanFilePath(); | ||
| this.config | ||
| .getGeminiClient?.() | ||
| ?.getChat() | ||
| .redactApprovedPlanFromHistory( |
There was a problem hiding this comment.
[Suggestion] The redaction rewrites the in-memory GeminiChat.history array, but the chat-recording JSONL (append-only) already captured the full plan text when recordAssistantTurn fired during the model's original response — before tool execution and before this redaction. A --continue / --resume of the same session rehydrates the full plan text from the JSONL, re-introducing the very leak (#6237) this fix addresses in the current session.
Failure scenario: User enters plan mode, model produces a large plan, user approves. In the current session the redaction works correctly. User later runs qwen --continue on the same session — the JSONL is replayed into history with the original full plan text. On a long follow-up conversation, the model regurgitates plan chunks again.
Suggested fix: Apply redactApprovedPlanFromHistory at JSONL load time in the --resume path (scan for approved exit_plan_mode calls and replace their plan args), or add a recording-level redaction alongside the in-memory one.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Implemented in c054057, at load time as suggested — but at the choke point rather than per resume path: GeminiChat.setHistory and the constructor now run a history-wide pass (redactApprovedPlansInHistory, pure and unit-tested) that rewrites approved exit_plan_mode calls, so every rehydration route (--resume, --continue, ACP session load) is covered by the same two entry points. It applies the same never-lie rule as the write side: only calls whose plan matches the current on-disk plan file are rewritten. The pointer text moved into a shared approvedPlanRedactionText helper so the write and load surfaces cannot drift. One stated bound: with several approved plans in one session the file holds only the last, so earlier calls rehydrate unredacted (safe — the file never mismatches the pointer). Wiring pinned by tests with and without the plan file present, plus a constructor-path test.
中文：已在 c054057 按建议在加载侧实现——但收在 choke point 而非逐个 resume 路径：GeminiChat.setHistory 与构造器统一跑历史级净化（纯函数 redactApprovedPlansInHistory），覆盖 --resume/--continue/ACP 加载全部路径；与写侧同一「永不说谎」规则（仅当 plan 与当前磁盘文件一致才改写）；指针文案抽到共享 approvedPlanRedactionText 防两面漂移。已声明的边界：一个会话多次批准时文件只存最后一份，较早的调用恢复时不改写（安全，只是不最小化）。接线由有/无 plan 文件两组测试 + 构造器路径测试固定。
| const planPath = this.config.getPlanFilePath(); | ||
| this.config | ||
| .getGeminiClient?.() | ||
| ?.getChat() | ||
| .redactApprovedPlanFromHistory( | ||
| callId, | ||
| `[Plan approved and saved to ${planPath}. The plan text was ` + | ||
| `removed from the conversation after approval; read that ` + | ||
| `file if you need to consult it again.]`, |
There was a problem hiding this comment.
[Suggestion] The redaction message claims "Plan approved and saved to <path>" but savePlanBestEffort in ExitPlanModeToolInvocation.execute swallows all filesystem errors (disk full, permissions). When save fails silently, the plan text is redacted from the only remaining in-memory copy and replaced with a pointer to a file that does not exist.
Failure scenario: Filesystem runs out of space. User approves the plan. savePlan throws ENOSPC, savePlanBestEffort logs a warning and returns. The redaction block still fires (because llmContent starts with "User approved."). The plan text in history is replaced with a pointer to a non-existent file. If the model later tries to read_file at that path, the file is not there.
Suggested fix: Have savePlanBestEffort return a boolean indicating success, and gate the redaction on that flag — or change the replacement string to not assert the file was saved when it was not.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Fixed in c054057. The scheduler now reads the plan file before redacting, and redactApprovedPlanFromHistory gained an expectedPlan gate — the rewrite only happens when the in-history plan equals the on-disk content byte-for-byte. A missing, unreadable, or stale file (earlier plan overwritten scenario included) skips the redaction with a warning, so the pointer can never claim a save that failed. New regression test: approved exit with a non-existent plan file → plan text stays in history.
中文：已在 c054057 修复。调度器改写前先读 plan 文件，redactApprovedPlanFromHistory 新增 expectedPlan 门——仅当历史中的 plan 与磁盘内容逐字节一致才改写；文件缺失/不可读/内容陈旧则跳过并告警，指针永不谎称保存成功。新增回归测试：批准但 plan 文件不存在 → 历史保留原文。
…story load Review follow-up on QwenLM#7197, addressing both inline findings. Pointer honesty: savePlanBestEffort swallows filesystem errors, so the scheduler previously could replace the only remaining in-memory plan with a pointer to a file that was never written. The redaction now reads the plan file first and redactApprovedPlanFromHistory requires the in-history plan to equal the on-disk content byte-for-byte — a missing, unreadable, or stale file skips the rewrite entirely. Resume leak: the chat-recording JSONL captures the assistant turn (full plan argument) before the tool runs, so --resume/--continue re-fed the text the in-session redaction removed. GeminiChat.setHistory and the constructor now run a history-wide pass (redactApprovedPlansInHistory) that rewrites approved exit_plan_mode calls under the same file-must-match rule. The pointer text moved to a shared approvedPlanRedactionText helper so the write and load sides cannot drift. Known bound, stated in code: with several approved plans in one session the file holds only the last, so earlier calls rehydrate unredacted — safe, just not minimal. New tests: scheduler save-failure case (plan retained), pure-function matrix (approval detection, stale-file guard, non-approval), and setHistory/constructor wiring both with and without the plan file. 557/557 across the three touched suites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both inline findings addressed in c054057 (per-thread replies posted): (1) the redaction is now gated on the on-disk plan file matching the in-history text byte-for-byte — a swallowed save failure or stale file skips the rewrite, so the pointer can never lie; (2) the resume leak is closed at the load choke point — 中文：两条行内发现均已在 c054057 落地（已逐 thread 回复）：(1) 改写以「磁盘 plan 文件与历史文本逐字节一致」为门——保存静默失败或文件陈旧则跳过，指针永不说谎；(2) resume 泄漏在加载 choke point 关闭——setHistory 与构造器经纯函数历史级净化重放改写，与写侧共享文案与同一校验规则。三套件 557/557，静态检查全绿。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
| /** | ||
| * Single source of the pointer text that replaces an approved plan's | ||
| * `functionCall.args.plan` (#6237). Shared by the tool scheduler's | ||
| * post-approval rewrite and the load-side pass below so the two surfaces | ||
| * cannot drift. | ||
| */ |
There was a problem hiding this comment.
[Suggestion] These two new functions (approvedPlanRedactionText, redactApprovedPlansInHistory) are inserted between the pre-existing JSDoc block that documented redactStructuredOutputArgsForRecording and that function itself. TypeScript attaches a doc comment to the next declaration, so with two adjacent /** … */ blocks the first one is orphaned: redactStructuredOutputArgsForRecording silently loses its documentation, and the orphaned "Exported for tests; callers should prefer the inline use inside recordAssistantTurn" guidance now visually precedes approvedPlanRedactionText — which production code (the scheduler and the load-side pass) does call, making the guidance misleading. — Concrete cost: lost/misleading API docs for a recording helper.
Fix: move the two new exported functions below redactStructuredOutputArgsForRecording so the existing doc comment stays attached to its function.
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Fixed in 566dba2 — the two helpers moved below redactStructuredOutputArgsForRecording, so its doc comment is attached to it again and the "prefer the inline use" guidance no longer visually precedes a function that production code does call. 中文：已移到该函数之后，文档注释重新归位，误导性引导消除。
| expect(chat.redactApprovedPlanFromHistory('call-plan', REPLACEMENT)).toBe( | ||
| true, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] The expectedPlan mismatch branch of redactApprovedPlanFromHistory — the if (expectedPlan !== undefined && plan !== expectedPlan) return false guard — has no test at any level: every unit call here omits the third argument, and the scheduler tests only cover a missing file (the read throws), not a file whose content differs from the in-history plan. — Failure scenario: if a refactor inverts the comparison to plan === expectedPlan, the scheduler would redact the plan and point the model at a file holding a different plan, violating the "never lie" invariant the code comments describe, and no test would fail.
Fix: add a unit test, e.g. expect(chat.redactApprovedPlanFromHistory('call-plan', REPLACEMENT, 'a different plan')).toBe(false), and assert the plan arg is left unchanged.
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Added in 566dba2 — unit test drives the expectedPlan mismatch branch directly: a stale/different third argument returns false and leaves the plan arg unchanged, then the same call with the matching plan still rewrites (guards both the comparison direction and the never-lie invariant). 中文：已补单测——不一致的 expectedPlan 返回 false 且历史不变，随后一致的 plan 仍可改写；比较方向反转会被立刻抓住。
| ]); | ||
| } | ||
|
|
||
| it('redacts the plan argument from history after an approved exit_plan_mode', async () => { |
There was a problem hiding this comment.
[Suggestion] PLAN_EXIT_APPROVED_LLM_CONTENT_PREFIXES includes 'Leader approved.', but no test exercises that prefix — every approval-path test (including this one) uses 'User approved.' as the llmContent. — Failure scenario: if the leader prefix string changes (e.g. to 'Leader has approved.') or is dropped from the constant, the scheduler's .some(prefix => …) match stops firing for leader/teammate-approved exits, so the plan stays in history unredacted for that path — silently re-introducing the plan-history leak this PR fixes — and no test would catch it.
Fix: add one test mirroring this one but with llmContent: 'Leader approved. You can now start coding…', asserting the plan arg is redacted.
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Added in 566dba2 — a scheduler-path test mirroring the user-approved one but with llmContent: 'Leader approved. …', asserting the plan is redacted to the file pointer. Dropping or renaming the leader prefix in PLAN_EXIT_APPROVED_LLM_CONTENT_PREFIXES now fails a test. 中文：已补 leader 前缀的调度器测试；该前缀被改名/删除会直接跑挂测试。
…phaned doc Review follow-up on QwenLM#7197 (three inline findings): move the two plan- redaction helpers below redactStructuredOutputArgsForRecording so its doc comment stays attached; add the expectedPlan-mismatch unit test (stale on-disk plan blocks the rewrite, matching plan still rewrites); add a scheduler test for the 'Leader approved.' llmContent prefix so the teammate approval path cannot silently drop out of the redaction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
| /** | ||
| * Replaces the args on a `structured_output` `functionCall` with the | ||
| * same `__redacted` placeholder used by `ToolCallEvent` telemetry | ||
| * (`packages/core/src/telemetry/types.ts`). |
There was a problem hiding this comment.
[Suggestion] The diff inserts a duplicate of the redactStructuredOutputArgsForRecording JSDoc block — the original copy (lines 166–185, unchanged context) already precedes that function, and this second identical copy sits between it and the new approvedPlanRedactionText. Both copies are now orphaned: neither is adjacent to redactStructuredOutputArgsForRecording (line ~283). — Concrete cost: IDE doc-tooltips and typedoc attach the structured_output redaction docs to approvedPlanRedactionText instead; a future maintainer editing the docs updates the wrong copy.
| /** | |
| * Replaces the args on a `structured_output` `functionCall` with the | |
| * same `__redacted` placeholder used by `ToolCallEvent` telemetry | |
| * (`packages/core/src/telemetry/types.ts`). | |
| /** | |
| * Single source of the pointer text that replaces an approved plan's | |
| * `functionCall.args.plan` (#6237). Shared by the tool scheduler's | |
| * post-approval rewrite and the load-side pass below so the two surfaces | |
| * cannot drift. | |
| */ |
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Fixed in df52153 — both stranded copies removed and the single doc block reattached directly above redactStructuredOutputArgsForRecording; the helpers keep their own docs. 中文：两份孤儿副本已删，唯一文档块归位到函数正上方。
| const approvedHistory = (): Content[] => [ | ||
| { role: 'user', parts: [{ text: 'plan it' }] }, | ||
| { |
There was a problem hiding this comment.
[Suggestion] redactApprovedPlansInHistory is only tested with a single exit_plan_mode call in history — the Set-based multi-call path and the per-ID approved.has(fc.id) gate are never exercised with more than one element. — Failure scenario: if a future refactor replaces the approved Set with a boolean flag or drops the approved.has(fc.id) check, every existing test still passes, but a history with an approved and a rejected call sharing the same plan text would incorrectly redact the rejected call — destroying the plan text the model needs for revision.
Add one test with two exit_plan_mode calls: one approved (matching the on-disk plan) and one rejected (same plan text). Assert the approved call is redacted and the rejected call is untouched.
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Added in df52153 — a history with a rejected and an approved exit_plan_mode call sharing the same plan text: only the approved call is rewritten, the rejected one keeps its text for revision. This pins the per-call-id approved.has(fc.id) gate against a boolean-flag refactor. 中文：已补「同 plan 文本的拒绝+批准双调用」测试——仅批准调用被改写，钉死 per-call-id 门。
| const fc = part.functionCall; | ||
| if (!fc?.id || fc.name !== ToolNames.EXIT_PLAN_MODE) return part; | ||
| if (!approved.has(fc.id)) return part; | ||
| if ((fc.args ?? {})['plan'] !== savedPlanContent) return part; |
There was a problem hiding this comment.
[Suggestion] Load-side redaction only matches the last approved plan — earlier approved plans rehydrate unredacted on --resume, re-opening the #6237 regurgitation for multi-plan sessions. — Failure scenario: a session approves plan A, then plan B (overwriting A on disk). On --resume, plan A's functionCall fails the !== savedPlanContent check and rehydrates with the full plan blob in the model's context. The code comment acknowledges this ("earlier calls rehydrate unredacted; safe, just not minimal"), but the original issue is not fully fixed for this case.
Consider persisting each approved plan under a per-call-id file (e.g. <sessionId>-<callId>.md) so the load-side pass can match every approved call against its own file.
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Deferring this one deliberately (stated bound in code/PR): per-call-id plan files change the Config.savePlan single-file contract — getPlanFilePath() / loadPlan() are shared with other plan consumers, so the layout change belongs in its own reviewed change rather than growing this fix. The current behavior is safe (never lies, never redacts against a mismatched file); the cost is only that earlier plans in multi-approval sessions rehydrate unredacted. Happy to open a follow-up issue with the per-call-id file design if maintainers want it tracked. 中文：此条有意留作 follow-up——per-call-id 文件改变 savePlan 单文件契约（getPlanFilePath/loadPlan 有其他消费方），应独立评审而非扩大本修复。现行为安全（永不说谎、不对不匹配文件改写），代价仅是多次批准会话中较早的 plan 恢复时不被改写。如维护者希望跟踪，可开 follow-up issue 附设计。
| if (!entry?.parts) continue; | ||
| for (const part of entry.parts) { | ||
| const fr = part.functionResponse; | ||
| if (!fr?.id || fr.name !== ToolNames.EXIT_PLAN_MODE) continue; |
There was a problem hiding this comment.
[Suggestion] The load-side redactApprovedPlansInHistory compares fr.name / fc.name directly against ToolNames.EXIT_PLAN_MODE, while the write-side scheduler resolves through canonicalToolName(reqInfo.name). The two surfaces handle tool-name migration inconsistently. — Failure scenario: if exit_plan_mode is ever added to ToolNamesMigration (e.g. renamed to exit_plan), the in-session scheduler redaction still fires, but the load-side pass silently stops matching for sessions recorded under the old name — the --resume leak this PR fixes reappears with no error or warning.
| if (!fr?.id || fr.name !== ToolNames.EXIT_PLAN_MODE) continue; | |
| if (!fr?.id || canonicalToolName(fr.name) !== ToolNames.EXIT_PLAN_MODE) continue; |
(and the same for fc.name at line 262)
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Applied in df52153 — every load-side name match (functionResponse collection, functionCall rewrite, the instance method, and the cheap pre-scan) now routes through a local ToolNamesMigration mirror (canonicalPlanToolName). Kept local rather than importing the scheduler's canonicalToolName because that would create a geminiChat → coreToolScheduler import cycle (the scheduler already imports approvedPlanRedactionText from geminiChat); the mirror reads the same migration map, so the two cannot diverge on data. 中文：已在读侧所有 name 匹配点应用 ToolNamesMigration 本地镜像；不直接 import 调度器的 canonicalToolName 是因为会形成 geminiChat→scheduler 循环依赖（调度器已从 geminiChat 导入指针文案），镜像读同一迁移表，数据上不会分叉。
…ol names on load Review follow-up on QwenLM#7197 (second inline round): remove the duplicated redactStructuredOutputArgsForRecording JSDoc copies stranded by the earlier helper move and reattach the single copy to its function; route every load-side tool-name match through a local ToolNamesMigration mirror (canonicalPlanToolName — kept local to avoid a geminiChat -> coreToolScheduler import cycle) so a future exit_plan_mode rename keeps the resume-time redaction working for sessions recorded under the old name; add the approved+rejected same-plan-text regression test pinning the per-call-id approval gate. The per-call-id plan *file* suggestion is deferred as a follow-up — it changes the Config.savePlan single-file contract shared with other plan consumers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.8-max-preview via Qwen Code /review
| /** | ||
| * Single source of the pointer text that replaces an approved plan's | ||
| * `functionCall.args.plan` (#6237). Shared by the tool scheduler's | ||
| * post-approval rewrite and the load-side pass below so the two surfaces | ||
| * cannot drift. | ||
| */ |
There was a problem hiding this comment.
[Suggestion] This JSDoc block ("Single source of the pointer text that replaces an approved plan's functionCall.args.plan…") describes approvedPlanRedactionText but is physically attached to canonicalPlanToolName — which has its own JSDoc immediately below — so two consecutive doc blocks precede one function, while the exported approvedPlanRedactionText (depended on by the scheduler write-side at coreToolScheduler.ts:4373 and the load-side pass at geminiChat.ts:249) has none. — Concrete cost: a developer tracing the redaction pointer text reads this block, expects the next declaration to be approvedPlanRedactionText, and instead finds a name-resolution helper, obscuring the actual single source of truth and making it easier for a future edit to change the format string without realizing two call sites rely on it.
Fix: move this JSDoc block down to directly precede export function approvedPlanRedactionText(planPath: string): string { (spans two locations, so no one-click suggestion block).
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Fixed — the pointer-text doc now directly precedes approvedPlanRedactionText; canonicalPlanToolName keeps only its own doc. 中文：文案文档已归位到其函数正上方。
| try { | ||
| planPath = this.config.getPlanFilePath(); | ||
| savedPlan = fs.readFileSync(planPath, 'utf-8'); | ||
| } catch { | ||
| // No plan file (never saved, or save failed): leave history alone — | ||
| // never swap plan text for a pointer to a file that is not there. | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] This load-side catch swallows the filesystem error with no runtime log, while the write-side scheduler path logs via debugLogger.warn on the same failure class. — Concrete cost: on --resume with a removed/unsaved plan file, readFileSync throws, the redaction silently skips, and the full plan text re-enters the model's context (the #6237 symptom); an oncall engineer enabling DEBUG=* sees no trace the redaction was even attempted and must read the source to discover this silent return path.
| try { | |
| planPath = this.config.getPlanFilePath(); | |
| savedPlan = fs.readFileSync(planPath, 'utf-8'); | |
| } catch { | |
| // No plan file (never saved, or save failed): leave history alone — | |
| // never swap plan text for a pointer to a file that is not there. | |
| return; | |
| } | |
| try { | |
| planPath = this.config.getPlanFilePath(); | |
| savedPlan = fs.readFileSync(planPath, 'utf-8'); | |
| } catch (err) { | |
| // No plan file (never saved, or save failed): leave history alone — | |
| // never swap plan text for a pointer to a file that is not there. | |
| debugLogger.debug( | |
| `Skipping load-side plan redaction, plan file unavailable: ${err}`, | |
| ); | |
| return; | |
| } |
— qwen3.8-max-preview via Qwen Code /review
There was a problem hiding this comment.
Applied as suggested — the load-side catch now logs Skipping load-side plan redaction, plan file unavailable: … at debug level, matching the write-side scheduler's traceability. 中文：已按建议在 catch 中加 debug 日志，与写侧对齐。
…ed load-side redaction Review follow-up on QwenLM#7197 (third inline round): the "single source of the pointer text" JSDoc landed above canonicalPlanToolName instead of approvedPlanRedactionText — moved to its function; the load-side catch now emits a debugLogger.debug line when the plan file is unavailable so a --resume that silently skips the redaction is traceable under DEBUG, matching the write-side scheduler's logging. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Qwen Code review paused — model quota exhausted. Qwen review stopped: the model API quota is exhausted (reset at 07-20 07:32:00 UTC.). Transient errors auto-retry, but a quota reset is too far out to wait on a runner. Re-run once it resets by commenting |
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/29743366222)._ |
Code Review —
|
ReviewVerified locally against branch head What I verified (fresh worktree, hardlinked node_modules)
Findings1. Merge conflict with main — action needed. GitHub reports 2. Prefix lockstep is comment-enforced only — nice-to-have. 3. Degraded cases all fail toward the status quo — no action. For the record, the cases where the plan text silently stays in context: multiple approved plans in one session (the file holds only the last; earlier calls rehydrate unredacted — documented in the code), a plan-required teammate's own history (documented in the PR), recordings whose 4. Sync CI noteThe red Verdict: LGTM after the rebase onto main. The core mechanism (targeted immutable rewrite by callId, gated on approval prefixes and on-disk plan equality, with a load-side counterpart for |
|
@qwen-code /resolve |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge resolution for PR #7197Root causeThe base branch ( Textual or semanticTextual only. The two sides added independent, orthogonal fields to the toolOutputBatchBudget?: number;
getGeminiClient?: () => unknown;
getPlanFilePath?: () => string;
truncateToolOutputThreshold?: number;
truncateToolOutputLines?: number;The mock-config body below the conflict auto-merged cleanly: it already What is load-bearing
What I could not verify
中文说明PR #7197 合并冲突解决根因基线分支 ( 文本冲突还是语义冲突仅为文本冲突。 两边在同一选项对象上独立地增加了正交的字段，没有逻辑 关键约束
无法验证的内容
|
Review — reviewed at
|
The write side discarded redactApprovedPlanFromHistory's boolean and the load side discarded redactApprovedPlansInHistory's null, so a no-match call id, a non-string plan argument, and an expectedPlan mismatch were indistinguishable from a redaction that ran. Both sides now emit a debug log when the rewrite leaves history unchanged (review finding QwenLM#2). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the deep review and the mutation matrix — landed the recommended scope in 074cf8d: finding #2 is closed with a debug trace on both surfaces when the rewrite leaves history unchanged (write side: no-match call id / non-string plan / expectedPlan mismatch after the boolean was previously discarded; load side: null result after the Agreed on filing the rest as follow-ups; summarizing my read for the record:
中文多谢深度 review 和变异矩阵——按建议的范围在 074cf8d 落了 #2：两侧在改写未触及历史时都补了 debug 追踪（写侧：此前被丢弃的布尔返回值对应的 call id 不匹配 / plan 非字符串 / expectedPlan 不一致；读侧：过了 其余按 follow-up 处理，同意。记录一下我的看法：#1 是确认的边界——合成 id 从不写回历史，按 id 匹配注定失效；放宽到名字+位置匹配的风险比漏掉更大（改错目标比跳过更糟），先文档化边界 + 靠新加的 #2 追踪让这种情况可见，更稳妥。#3 同意应在收尾前落地——给 |
|
Follow-up for finding #3 is up as #7678 — |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| describe('redactApprovedPlanFromHistory', () => { | ||
| // After an approved exit_plan_mode the full plan text would otherwise | ||
| // stay in history as the model's own tool-call argument and get | ||
| // regurgitated into later responses (#6237). These tests pin the | ||
| // targeted history rewrite the tool scheduler performs post-approval. |
There was a problem hiding this comment.
[Suggestion] No test exercises canonicalPlanToolName with a legacy tool-name alias — every test here uses name: 'exit_plan_mode' directly, so the ToolNamesMigration lookup path is unverified. — Failure scenario: if ToolNamesMigration gains an exit_plan_mode alias and canonicalPlanToolName has a bug (wrong lookup direction, or the Record<string, string> cast silently drops entries), a --resumed session with history recorded under the legacy name would silently skip redaction — plan text leaks back into the model's context despite being approved and saved. Today this cannot fire (no exit_plan_mode alias exists), but the scheduler's own tests pin the same contract with it.each(Object.entries(ToolNamesMigration)) — the pattern is established and cheap to mirror.
it('resolves legacy tool-name aliases when matching exit_plan_mode calls', () => {
const history: Content[] = [
{
role: 'model',
parts: [{
functionCall: {
id: 'call-a',
name: 'exit_plan_mode_legacy',
args: { plan: PLAN },
},
}],
},
{
role: 'user',
parts: [{
functionResponse: {
id: 'call-a',
name: 'exit_plan_mode_legacy',
response: { output: 'User approved. You can now start coding.' },
},
}],
},
];
const out = redactApprovedPlansInHistory(history, PLAN, PLAN_PATH);
expect(out).not.toBeNull();
expect(out![0]!.parts![0]!.functionCall!.args!['plan']).toBe(
approvedPlanRedactionText(PLAN_PATH),
);
});— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
Agreed it's a real gap, but per wenshao's round-5 guidance this PR lands only the two-line logging change and defers the remaining test pins to a follow-up — I'll bundle a legacy-alias case for canonicalPlanToolName (both surfaces) into that follow-up test PR alongside the scheduler savedPlan gate pin and the setStatusInternal ordering probe.
中文
确实是缺口，但按 wenshao 第 5 轮的意见，本 PR 只落两行日志改动、其余测试钉全部推 follow-up——canonicalPlanToolName 的 legacy 别名用例（两个面）会和调度器 savedPlan 闸门钉、setStatusInternal 顺序探针一起放进那个 follow-up 测试 PR。
…pt (QwenLM#7678) The default plans dir (~/.qwen/plans) sits outside the workspace and was in none of ReadFileTool's permission-free roots, so reading a saved plan back landed on an ask confirmation — popped at exactly the moment the user approved the plan and told the agent to start coding, and resolvable as a denial in non-interactive/ACP flows. With the approved-plan pointer (QwenLM#7197) the saved file is the model's only recovery route for the plan text, so the read must not stall on a prompt. Adds config.getPlansDir() to ReadFileTool.getDefaultPermission's allowedRoots and to AcpAgent.buildAcpLocalReadRoots (per the SYNC comment). The dir holds only session plan files; sibling ~/.qwen files such as settings.json stay confirmation-gated, pinned by a new test. Refs QwenLM#6237 Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Review —
|
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge resolution for PR #7197Root causePR #7671 (landed on main) added two new tests for Textual or semanticTextual only. The two sides added independent, self-contained tests at the same insertion point. Neither side modified the same logic — they were adjacent insertions that git could not auto-merge because the shared surrounding structure ( Resolution: kept all six tests (four from HEAD, two from origin/main) in a single block, HEAD's tests first then origin/main's. What is load-bearing
What I could not verifyNo build or tests were run. The auto-merged files ( 中文说明PR #7197 合并冲突解决根因PR #7671（已合入 main）在 文本冲突还是语义冲突纯文本冲突。 两边添加的是独立、自包含的测试，只是恰好插入位置相同。共享的周边结构（ 解决方式：保留全部六个测试（HEAD 四个 + origin/main 两个），HEAD 的在前，origin/main 的在后。 关键约束
未能验证的内容未执行构建或测试。自动合并的文件（ |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
Maintainer local verification — real E2E at
|
| scenario | request | base 596abd966 |
PR dda19ac2c |
|---|---|---|---|
| approve | #2 · post-approval continuation | full plan ×5 ❌ | pointer ✅ |
| approve | #4 · second user turn | full plan ×5 ❌ | pointer ✅ |
| approve | #6 · later side-query | full plan ×5 ❌ | pointer ✅ |
| resume | #7 · first turn after --continue |
full plan ×5 ❌ | pointer ✅ |
| reject | #2 · next turn | full plan ×5 (correct) | full plan ×5 (correct) |
The --continue row is the one nobody had exercised end-to-end, and it is a genuine load-side result rather than the write side leaking through: the chat-recording JSONL on disk still holds the full 611-char plan (functionCall.args.plan with the marker intact), and the relaunched process still puts only the pointer on the wire. The resume gap is really closed.
2. Degraded paths
- Provider omits the
tool_callid — redaction still fires.openaiContentGenerator/converter.ts:1706synthesizescall_<ts>_<rand>before thefunctionCallreaches history, so the id-keyed lookup always has an id on this path. - Following the pointer is free — after approval the model called
read_fileon the saved plan and it executed with no confirmation prompt, in "Ask permissions" mode. - TUI unchanged —
returnDisplay.planstill renders the full approved plan (screenshot below). - Cost of the load-side pass —
readFileSyncinstrumented inside the real CLI process: 1 plan-file read after approval, 2 after the next turn. ~1 small sync read per turn in a plan-bearing session.
3. Are the new tests load-bearing?
591/591 green at head (exitPlanMode 30 · coreToolScheduler 310 · geminiChat 251). Counterfactual — base source with the PR's own tests — fails 12: 2 in coreToolScheduler.test.ts, 10 in geminiChat.test.ts, while both guardrail tests pass on both sides. Correct shape.
Mutation matrix — delete one guard from the PR source, rerun the two suites:
| # | guard removed | tests failed | |
|---|---|---|---|
| M1 | never-lie gate (in-history plan must equal the on-disk file) | 1 | pinned |
| M2 | approval-prefix gate (only User/Leader approved.) |
0 | unpinned |
| M3 | callId guard in redactApprovedPlanFromHistory |
0 | unpinned |
| M4 | load-side redaction from the GeminiChat constructor |
1 | pinned |
| M5 | load-side redaction from setHistory |
1 | pinned |
| M6 | per-call-id approved gate on the load side | 1 | pinned |
New (Low — for the follow-up test PR, not a blocker): the approval-prefix gate is load-bearing but nothing pins it. keeps the plan argument in history when exit_plan_mode is not approved never writes a plan file, so the scheduler's readFileSync(planPath) throws and skips the redaction regardless of what the prefix check does — the test passes with the gate deleted. I ran the case it misses (plan approved once so the file is on disk, re-proposed, then rejected): green at head, and with the prefix gate removed the rejected plan is rewritten to [Plan approved and saved to …] — a pointer that claims an approval that never happened, and drops the text the model needs to revise. The probe is ~25 lines and is the same test with getPlanFilePath: () => planFile plus a matching writeFileSync.
M3 is the same shape but milder: the only negative test uses write_file, so the tool-name half of the dual guard covers for the id half. A wrong rewrite would additionally need two calls carrying identical plan text, which the equality gate makes unlikely.
Also measured: moving the redaction to after setStatusInternal('success') did not re-open the leak on request #2. The harness does detect a leak there (base does), so this is a real null, not a blind spot — the "must run BEFORE setStatusInternal" comment is a cheap safety margin rather than a proven requirement. No action; just don't bank on it as verified behaviour.
4. Corrections to my earlier rounds
Medium 如何自定义密钥文件 .env可能与其他文件冲突 #3 "new runtime import cycle"— withdrawn. Real ESM resolution graph from the builtpackages/core/dist(type-only imports are erased, so every edge is a runtime edge): base already cyclesgeminiChat → chatRecordingService → config → client → geminiChat, withconfig.jsloaded at runtime. The PR adds one module to that graph (337 → 338) and routes the shortest cycle throughexitPlanModeinstead. Pre-existing; the PR does not introduce it.Medium Are you interested in AI Terminal? #4 "the pointer sends the model to a file— resolved on main. fix(core): allow reading saved plan files without a confirmation prompt #7678 (read_filewill prompt for"331d58c65) addedgetPlansDir()toReadFileTool's allowlist and is an ancestor of this head; verified live above.Low API Key是要设成阿里云的API Key吗？ #7 "no-ops for providers that omit— withdrawn for the OpenAI-compatible path, per §2.functionCall.id"
Still open, all cosmetic and fine as follow-ups: #2 the leader-approved (teammate) test name — createApprovalModeConfigOverride overrides approvalMode/permissionManager/autoModeDenialState but never getGeminiClient, so the teammate path resolves the main session's chat and the redaction is a guaranteed no-op there; #5 if (redacted === false) misses the undefined no-client case (!== true); #8 --fork-session mints a fresh randomUUID() (cli/src/config/config.ts:1974), so the plan file is absent and the load-side pass no-ops — one line next to the existing --resume caveat.
Harness (reproducible)
- Isolated worktree at
dda19ac2c, ownnpm ci,npm run build && npm run bundle. - E2E:
@lydell/node-pty+@xterm/headlessdrivingdist/cli.js, isolatedHOME,startFakeOpenAIServerfromintegration-tests/fake-openai-server.ts; the model returns oneexit_plan_modecall whose plan carries a unique marker in 5 places. Scenarios:approve,reject,resume(--continuein a second process),noid(server omits the tool-call id),reread(model callsread_fileon the saved plan). - Base arm:
git checkout 596abd966 -- <the 5 files>,rm -rf dist, rebundle; verified no chunk containsPlan approved and saved tobefore running. - Plan-read counting:
NODE_OPTIONS=--requireshim patchingfs.readFileSyncbefore the ESM graph snapshotsnode:fs. - Import graph:
module.register()resolve hook recording every edge, BFS for the shortest cycle back to the entry.
中文版本
Maintainer 本地验证 —— 针对 dda19ac2c 的真实 E2E
在隔离 worktree 中重新验证当前 head（独立 npm ci + 完整 bundle 构建）。以下结论全部来自真实运行的 CLI 或真实运行的测试，而非阅读 diff 得出。
结论：LGTM，可以合并。 修复在网络层面确实做到了它声称的事情——写侧与 resume 侧都成立；我能构造的所有降级路径都安全退化。§3 中新发现的一个覆盖缺口值得并入你已经同意的后续测试 PR，但不阻塞合并。我此前三条发现予以撤回或已被 main 解决（§4）。
1. CLI 实际发到线上的内容
用 pty 驱动真实构建产物（--approval-mode plan），对接仓库自带的 integration-tests/fake-openai-server.ts；判据是每个 /chat/completions 请求中的 messages[].tool_calls[].function.arguments。base 组为同一 worktree 把 5 个改动文件回退到 merge base 596abd966 后清理重打包——复现时需注意：npm run bundle 会保留 dist/chunks/ 下旧的哈希 chunk，我第一次的 base 组因此静默跑了 PR 的代码。
| 场景 | 请求 | base 596abd966 |
PR dda19ac2c |
|---|---|---|---|
| 批准 | #2 · 批准后的 continuation | 完整 plan ×5 ❌ | 短引用 ✅ |
| 批准 | #4 · 第二个用户轮次 | 完整 plan ×5 ❌ | 短引用 ✅ |
| 批准 | #6 · 后续 side-query | 完整 plan ×5 ❌ | 短引用 ✅ |
| resume | #7 · --continue 后首轮 |
完整 plan ×5 ❌ | 短引用 ✅ |
| 拒绝 | #2 · 下一轮 | 完整 plan ×5（正确） | 完整 plan ×5（正确） |
--continue 这一行此前没有人做过端到端验证，而且它确实是 load-side 的效果，不是写侧的顺带结果：磁盘上的 chat recording JSONL 仍保存着完整的 611 字符 plan（functionCall.args.plan 中 marker 完好），重启后的进程发出的仍只有短引用。resume 泄漏确实被关闭了。
2. 降级路径
- provider 不返回
tool_callid —— 改写照常生效。openaiContentGenerator/converter.ts:1706在functionCall进入历史之前就合成了call_<ts>_<rand>，因此该路径上按 id 查找总能命中。 - 顺着指针重读没有摩擦 —— 批准后模型对保存的 plan 调用
read_file，在「Ask permissions」模式下没有弹出确认框直接执行。 - TUI 无变化 ——
returnDisplay.plan仍完整渲染已批准的 plan。 - load-side 开销 —— 在真实 CLI 进程内插桩
readFileSync：批准后 1 次、再一轮后 2 次，即含 plan 的会话中每轮约 1 次小文件同步读。
3. 新增测试是否「吃劲」
head 上 591/591 通过（exitPlanMode 30 · coreToolScheduler 310 · geminiChat 251）。反事实实验（base 源码 + PR 自带测试）挂 12 个：coreToolScheduler.test.ts 2 个、geminiChat.test.ts 10 个；两个护栏测试在两侧都通过——形状正确。
变异矩阵（从 PR 源码中删掉某个 guard 后重跑两套件）：M1 never-lie 门挂 1、M2 批准前缀门挂 0、M3 callId 门挂 0、M4 构造器 load-side 挂 1、M5 setHistory load-side 挂 1、M6 按 call-id 的批准门挂 1。
新发现（Low，建议并入后续测试 PR，不阻塞）：批准前缀门在生产中吃劲，但没有任何测试钉住它。 keeps the plan argument in history when exit_plan_mode is not approved 从不写 plan 文件，于是调度器的 readFileSync(planPath) 抛错、无论前缀检查如何都会跳过改写——删掉该门这条测试照样通过。我补跑了它漏掉的场景（先批准一次使文件落盘，再次提出同一 plan，然后拒绝）：head 上通过；移除前缀门后，被拒绝的 plan 被改写成 [Plan approved and saved to …]——一个声称「已批准」而实际并未批准的指针，同时丢掉了模型修订所需的正文。补钉方法就是同一测试加 getPlanFilePath: () => planFile 与配套的 writeFileSync，约 25 行。
M3 同形但更轻：唯一的否定用例用的是 write_file，双重守卫中「工具名」那一半替「call id」那一半兜了底；且等值门要求两次调用 plan 文本完全一致才可能误改。
另外测得：把改写移到 setStatusInternal('success') 之后，请求 #2 并没有重新泄漏。该 harness 确实能检出这里的泄漏（base 组就检出了），所以这是一个真实的 null 结果——「必须在 setStatusInternal 之前执行」是一层廉价的安全余量，而非已被证明的必要条件。无需改动，只是不要把它当作已验证行为。
4. 对我此前几轮的更正
Medium 如何自定义密钥文件 .env可能与其他文件冲突 #3「新增运行时循环依赖」—— 撤回。 基于构建产物packages/core/dist的真实 ESM 解析图（type-only import 已被擦除，因此每条边都是运行时边）：base 本来就存在geminiChat → chatRecordingService → config → client → geminiChat的循环，config.js本来就在运行时被加载。PR 只是让该图多了 1 个模块（337 → 338），最短环改从exitPlanMode经过。循环是既有的，不是本 PR 引入。Medium Are you interested in AI Terminal? #4「指针指向的文件—— 已由 main 解决。 fix(core): allow reading saved plan files without a confirmation prompt #7678（read_file会弹确认」331d58c65）已把getPlansDir()加入ReadFileTool白名单，且是本 head 的祖先；上面已实测确认。Low API Key是要设成阿里云的API Key吗？ #7「provider 缺—— 在 OpenAI 兼容路径上撤回，理由见 §2。functionCall.id时静默失效」
仍然开着、但都属于表面问题，可作后续处理：#2 leader-approved (teammate) 测试名——createApprovalModeConfigOverride 覆盖了 approvalMode/permissionManager/autoModeDenialState，但从未覆盖 getGeminiClient，因此 teammate 路径拿到的是主会话的 chat，该路径上改写必然 no-op；#5 if (redacted === false) 漏掉了无 client 时的 undefined（应为 !== true）；#8 --fork-session 会生成新的 randomUUID()（cli/src/config/config.ts:1974），plan 文件不存在导致 load-side 直接 no-op——建议在既有 --resume 限制旁补一行说明。
4 participants
What this PR does
After the user (or a plan-required teammate's leader) approves an
exit_plan_modecall, the tool scheduler now rewrites theplanargument still sitting in the model turn'sfunctionCallhistory entry, replacing the full plan text with a short pointer to the plan file thatConfig.savePlanalready persisted ([Plan approved and saved to <path>. …]). The rewrite is a targeted, immutable replacement bycallIdvia a newGeminiChat.redactApprovedPlanFromHistorymethod — sibling parts and other arguments (originalRequest,researchSummary) survive, the partial-push markers are unaffected (they compare by index and role), and the UI-sidereturnDisplay.planis untouched. Rejected and no-action results are deliberately left alone: the rewrite is keyed off the approvalllmContentprefixes (exported as a constant fromexitPlanMode.tsso the two files cannot drift), because a rejected plan must stay in context for revision.Why it's needed
#6237: after an approved
exit_plan_mode, the model's later responses can contain verbatim fragments of the plan — sections, code blocks, and tables leak into answers to unrelated follow-ups ("整体完成了么？" answered with the plan text). The tool'sllmContentis already clean and the plan-mode reminder is already stripped on exit; the residual leak surface, as @doudouOUC confirmed on the issue, is exactly thefunctionCall.args.planblob that remains in history as the model's own prior turn. LLMs occasionally regurgitate large blocks of their own earlier tool-call arguments on long conversations. This PR implements the structural fix the maintainer proposed there: rewrite the persisted arg to a short reference after approval so the blob leaves the model's attention window, with the full text still available on disk.Reviewer Test Plan
How to verify
exit_plan_modewith a detailed multi-section plan, and approve it.tool_callsarguments (visible with any request logger). After: those requests carry[Plan approved and saved to <path>. …]instead, starting with the immediate post-approval continuation.npx vitest run src/core/coreToolScheduler.test.ts src/core/geminiChat.test.ts src/tools/exitPlanMode.test.ts(packages/core): 550/550.Evidence (Before & After)
Deterministic E2E: a PTY harness drives the real CLI (
--approval-mode plan) against a local fake OpenAI-compatible server that inspects theexit_plan_modearguments of every outgoing request. The scripted flow is user turn →exit_plan_modetool call with a marker-bearing plan → approve in the TUI dialog → post-approval continuation → second user turn.The new scheduler regression test ("redacts the plan argument from history after an approved exit_plan_mode") fails on the unpatched source — verified via
git stash.Tested on
Environment (optional)
macOS (Darwin 24.6), Node v22.23.1; PTY harness + local fake OpenAI-compatible SSE server against
packages/cli/dist, plus vitest unit tests.npm run typecheck, eslint (--max-warnings 0), prettier all clean.Risk & Scope
setStatusInternal('success')so even the synchronously-submitted continuation turn sees sanitized history, and it is wrapped in try/catch — any failure logs a warning and leaves history unchanged.--resumed session re-feeds the full plan text until its next approved exit — sanitizing the recording/resume path is left as a follow-up (flagged in the commit message). For plan-required teammates the scheduler reaches the main session's chat, so the teammate's own history keeps its plan (the rewrite no-ops on an unmatched callId); the main-session leak this issue reports is fully covered.Linked Issues
Fixes #6237
中文说明
本 PR 做了什么
在用户（或 plan-required teammate 的 leader）批准
exit_plan_mode之后，工具调度器改写仍留在模型轮次functionCall历史条目里的plan参数，用指向Config.savePlan已持久化的 plan 文件的短引用（[Plan approved and saved to <path>. …]）替换完整 plan 文本。改写通过新的GeminiChat.redactApprovedPlanFromHistory按callId定向、不可变地替换——同级 parts 与其它参数（originalRequest、researchSummary）保留，partial-push 标记不受影响（按索引与角色比对），UI 侧returnDisplay.plan不动。被拒绝/未生效的结果有意不改写：改写以批准的llmContent前缀为键（从exitPlanMode.ts导出常量，两个文件不会漂移），因为被拒的 plan 必须留在上下文里供修订。为什么需要
#6237：批准
exit_plan_mode后，模型后续回复会包含 plan 的逐字片段——章节、代码块、表格泄漏进无关追问的回答（「整体完成了么？」被用 plan 文本回答）。工具的llmContent本就干净、plan-mode reminder 退出时已剥离；正如 @doudouOUC 在 issue 中确认的，残留泄漏面恰是历史里作为模型自身前轮的functionCall.args.plan大文本——长对话中 LLM 偶尔会复读自己早前 tool-call 参数的大段内容。本 PR 落地 maintainer 在 issue 中提出的结构性修复：批准后把持久化参数改写为短引用，使大文本离开模型注意力窗口，完整文本仍在磁盘可查。审阅测试计划
如何验证
exit_plan_mode并批准；tool_calls参数里携带完整 plan 文本；之后：从批准后的即时 continuation 起改为短引用；证据（Before & After）
确定性 E2E（PTY 驱动真实 CLI
--approval-mode plan+ 本地伪 OpenAI 服务器检查每个请求的exit_plan_mode参数）：修复前 #2 批准后 continuation 与 #3 第二个用户轮次均带完整 plan 文本（❌）；修复后均为短引用（✅）。新增的调度器回归测试在未修复源码上失败（经git stash验证）。测试平台
macOS 已本地验证（✅）；Windows / Linux 依赖 CI（⚠️ ）。
环境
macOS（Darwin 24.6）、Node v22.23.1；PTY harness + 本地伪 OpenAI SSE 服务器 + vitest；typecheck / eslint / prettier 全绿。
风险与范围
setStatusInternal('success')之前执行，同步提交的 continuation 轮次即已看到净化后的历史；整体包在 try/catch 中，失败只记 warning、历史保持不变。--resume的会话在下一次批准退出前会重新带回完整 plan 文本——录制/恢复路径的净化留作后续（已在提交信息中注明）。plan-required teammate 场景中调度器触达的是主会话 chat，teammate 自身历史保留其 plan（callId 不匹配时改写为 no-op）；本 issue 报告的主会话泄漏已完整覆盖。关联 Issue
Fixes #6237
🤖 Generated with Claude Code