From 3f2fd1e7766317bea727418f7eccbc4bc02768ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=BA=B3=E4=BF=A1?= Date: Sun, 19 Jul 2026 11:45:28 +0800 Subject: [PATCH 1/6] fix(core): redact the plan argument from history after an approved exit_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 (#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 #6237 Co-Authored-By: Claude Fable 5 --- .../core/src/core/coreToolScheduler.test.ts | 108 +++++++++++++++++- packages/core/src/core/coreToolScheduler.ts | 38 ++++++ packages/core/src/core/geminiChat.test.ts | 92 +++++++++++++++ packages/core/src/core/geminiChat.ts | 47 ++++++++ packages/core/src/tools/exitPlanMode.ts | 12 ++ 5 files changed, 296 insertions(+), 1 deletion(-) diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index d56dcfe75e9..5bc53b7c113 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -53,6 +53,7 @@ import { MOCK_TOOL_GET_DEFAULT_PERMISSION, MOCK_TOOL_GET_CONFIRMATION_DETAILS, } from '../test-utils/mock-tool.js'; +import { GeminiChat } from './geminiChat.js'; import { MessageBusType } from '../confirmation-bus/types.js'; import type { HookExecutionResponse } from '../confirmation-bus/types.js'; import { type NotificationType } from '../hooks/types.js'; @@ -707,6 +708,7 @@ describe('CoreToolScheduler', () => { onToolCallsUpdate?: ReturnType; memoryMonitor?: { scheduleCheck: () => void }; toolOutputBatchBudget?: number; + getGeminiClient?: () => unknown; }) { const ensureTool = vi.fn( async (name: string) => @@ -761,7 +763,8 @@ describe('CoreToolScheduler', () => { getToolRegistry: () => mockToolRegistry, getCwd: () => '/repo', getUseModelRouter: () => false, - getGeminiClient: () => null, + getGeminiClient: options.getGeminiClient ?? (() => null), + getPlanFilePath: () => '/tmp/plans/test-session-id.md', getChatRecordingService: () => undefined, getMemoryPressureMonitor: () => options.memoryMonitor, getMessageBus: vi.fn().mockReturnValue(options.messageBus), @@ -880,6 +883,109 @@ describe('CoreToolScheduler', () => { expect(execute).toHaveBeenCalledWith({ plan: 'Original plan' }); }); + function createChatWithPlanCall(callId: string, plan: string): GeminiChat { + return new GeminiChat({} as unknown as Config, {}, [ + { role: 'user', parts: [{ text: 'please plan this' }] }, + { + role: 'model', + parts: [ + { text: 'Here is my plan.' }, + { + functionCall: { + id: callId, + name: ToolNames.EXIT_PLAN_MODE, + args: { plan, originalRequest: 'please plan this' }, + }, + }, + ], + }, + ]); + } + + it('redacts the plan argument from history after an approved exit_plan_mode', async () => { + const bigPlan = '## Plan\n\n1. huge section\n2. code blocks\n3. tables'; + const chat = createChatWithPlanCall('plan-call-1', bigPlan); + const tool = new MockTool({ + name: ToolNames.EXIT_PLAN_MODE, + execute: vi.fn().mockResolvedValue({ + llmContent: + 'User approved. You can now start coding. Start with updating your todo list if applicable.', + returnDisplay: { + type: 'plan_summary', + message: 'User approved.', + plan: bigPlan, + }, + }), + }); + const onAllToolCallsComplete = vi.fn(); + const { scheduler } = createSchedulerForLegacyToolTests({ + toolsByName: new Map([[ToolNames.EXIT_PLAN_MODE, tool]]), + approvalMode: ApprovalMode.YOLO, + onAllToolCallsComplete, + getGeminiClient: () => ({ getChat: () => chat }), + }); + + await scheduler.schedule( + [ + { + callId: 'plan-call-1', + name: ToolNames.EXIT_PLAN_MODE, + args: { plan: bigPlan }, + isClientInitiated: false, + prompt_id: 'prompt-plan-approved', + }, + ], + new AbortController().signal, + ); + await vi.waitFor(() => expect(onAllToolCallsComplete).toHaveBeenCalled()); + + const history = chat.getHistory(); + const fnCall = history[1]!.parts![1]!.functionCall!; + expect(fnCall.args!['plan']).not.toContain('huge section'); + expect(fnCall.args!['plan']).toContain( + 'Plan approved and saved to /tmp/plans/test-session-id.md', + ); + // Sibling parts and other args survive untouched. + expect(history[1]!.parts![0]).toEqual({ text: 'Here is my plan.' }); + expect(fnCall.args!['originalRequest']).toBe('please plan this'); + }); + + it('keeps the plan argument in history when exit_plan_mode is not approved', async () => { + const bigPlan = '## Plan\n\nkeep me for revision'; + const chat = createChatWithPlanCall('plan-call-2', bigPlan); + const tool = new MockTool({ + name: ToolNames.EXIT_PLAN_MODE, + execute: vi.fn().mockResolvedValue({ + llmContent: 'Plan execution was not approved. Remaining in plan mode.', + returnDisplay: 'Plan execution was not approved.', + }), + }); + const onAllToolCallsComplete = vi.fn(); + const { scheduler } = createSchedulerForLegacyToolTests({ + toolsByName: new Map([[ToolNames.EXIT_PLAN_MODE, tool]]), + approvalMode: ApprovalMode.YOLO, + onAllToolCallsComplete, + getGeminiClient: () => ({ getChat: () => chat }), + }); + + await scheduler.schedule( + [ + { + callId: 'plan-call-2', + name: ToolNames.EXIT_PLAN_MODE, + args: { plan: bigPlan }, + isClientInitiated: false, + prompt_id: 'prompt-plan-rejected', + }, + ], + new AbortController().signal, + ); + await vi.waitFor(() => expect(onAllToolCallsComplete).toHaveBeenCalled()); + + const fnCall = chat.getHistory()[1]!.parts![1]!.functionCall!; + expect(fnCall.args!['plan']).toBe(bigPlan); + }); + it('does not let AUTO_EDIT approve an interaction-required info tool', async () => { const execute = vi.fn().mockResolvedValue({ llmContent: 'executed', diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 3d150f985a7..2c5cada3772 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -57,6 +57,7 @@ import type { } from '@google/genai'; import { fileURLToPath } from 'node:url'; import { ToolNames, ToolNamesMigration } from '../tools/tool-names.js'; +import { PLAN_EXIT_APPROVED_LLM_CONTENT_PREFIXES } from '../tools/exitPlanMode.js'; import { collectAvailableSkillEntries, renderAvailableSkillsBlock, @@ -4338,6 +4339,43 @@ export class CoreToolScheduler { : {}), ...(artifacts.length > 0 ? { artifacts } : {}), }; + // After an APPROVED exit_plan_mode, swap the large `plan` argument + // still sitting in the model turn's functionCall for a pointer to the + // saved plan file. The blob otherwise stays in the model's attention + // window and gets regurgitated into later responses (#6237). Keyed off + // the approval llmContent prefixes (not just tool success) so + // rejected/no-action results keep their plan text for revision. Must + // run BEFORE setStatusInternal: completion callbacks may submit the + // continuation turn synchronously, and it should already see the + // sanitized history. + if ( + canonicalName === ToolNames.EXIT_PLAN_MODE && + typeof toolResult.llmContent === 'string' && + PLAN_EXIT_APPROVED_LLM_CONTENT_PREFIXES.some((prefix) => + (toolResult.llmContent as string).startsWith(prefix), + ) + ) { + try { + 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.]`, + ); + } catch (redactErr) { + debugLogger.warn( + `Failed to redact approved plan from history for ${callId}: ${ + redactErr instanceof Error + ? redactErr.message + : String(redactErr) + }`, + ); + } + } this.setStatusInternal(callId, 'success', successResponse); safeSetStatus(span, { code: SpanStatusCode.OK }); // Mirrors setToolSpanFailure/setToolSpanCancelled — every tool span diff --git a/packages/core/src/core/geminiChat.test.ts b/packages/core/src/core/geminiChat.test.ts index da740c05085..23b3dd7344b 100644 --- a/packages/core/src/core/geminiChat.test.ts +++ b/packages/core/src/core/geminiChat.test.ts @@ -11359,6 +11359,98 @@ describe('GeminiChat', async () => { }); }); + 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. + + const REPLACEMENT = '[Plan approved and saved to /tmp/p.md]'; + + function chatWith(history: Content[]): GeminiChat { + return new GeminiChat({} as unknown as Config, {}, history); + } + + it('rewrites only the plan arg of the matching exit_plan_mode call', () => { + const chat = chatWith([ + { role: 'user', parts: [{ text: 'plan it' }] }, + { + role: 'model', + parts: [ + { text: 'My plan follows.' }, + { + functionCall: { + id: 'call-plan', + name: 'exit_plan_mode', + args: { plan: 'SECRET BIG PLAN', originalRequest: 'plan it' }, + }, + }, + ], + }, + ]); + + expect(chat.redactApprovedPlanFromHistory('call-plan', REPLACEMENT)).toBe( + true, + ); + + const entry = chat.getHistory()[1]!; + const fnCall = entry.parts![1]!.functionCall!; + expect(fnCall.args!['plan']).toBe(REPLACEMENT); + expect(fnCall.args!['originalRequest']).toBe('plan it'); + expect(fnCall.id).toBe('call-plan'); + expect(entry.parts![0]).toEqual({ text: 'My plan follows.' }); + expect(JSON.stringify(chat.getHistory())).not.toContain( + 'SECRET BIG PLAN', + ); + }); + + it('returns false when no matching call id or tool name exists', () => { + const chat = chatWith([ + { + role: 'model', + parts: [ + { + functionCall: { + id: 'call-other', + name: 'write_file', + args: { plan: 'not a plan tool' }, + }, + }, + ], + }, + ]); + expect(chat.redactApprovedPlanFromHistory('call-plan', REPLACEMENT)).toBe( + false, + ); + expect( + chat.redactApprovedPlanFromHistory('call-other', REPLACEMENT), + ).toBe(false); + expect(chat.getHistory()[0]!.parts![0]!.functionCall!.args!['plan']).toBe( + 'not a plan tool', + ); + }); + + it('returns false when the matching call has no string plan arg', () => { + const chat = chatWith([ + { + role: 'model', + parts: [ + { + functionCall: { + id: 'call-plan', + name: 'exit_plan_mode', + args: {}, + }, + }, + ], + }, + ]); + expect(chat.redactApprovedPlanFromHistory('call-plan', REPLACEMENT)).toBe( + false, + ); + }); + }); + describe('redactStructuredOutputArgsForRecording', () => { // The chat-recording JSONL persists assistant turns to disk and re-feeds // them on `--continue` / `--resume`. For `--json-schema` runs the diff --git a/packages/core/src/core/geminiChat.ts b/packages/core/src/core/geminiChat.ts index 229be46a7fe..fca2cf74f43 100644 --- a/packages/core/src/core/geminiChat.ts +++ b/packages/core/src/core/geminiChat.ts @@ -3520,6 +3520,53 @@ export class GeminiChat { this.clearPendingPartialState(); } + /** + * Replaces the `plan` argument of an `exit_plan_mode` `functionCall` in + * history with a short reference, keeping every other part and argument + * intact. + * + * The full plan text a model submits to `exit_plan_mode` stays in history + * as its own tool-call arguments; on long conversations models + * occasionally regurgitate chunks of that blob in later responses + * (#6237). Once the plan is approved it is persisted to disk by + * `Config.savePlan`, so the in-context copy can be swapped for a pointer + * without losing information. Rejected plans are left untouched — the + * model needs the text to revise them. + * + * The entry is replaced immutably at the same index; the partial-push + * markers compare by index and role, so this cannot desync them. + * + * @returns true when a matching functionCall was found and rewritten. + */ + redactApprovedPlanFromHistory(callId: string, replacement: string): boolean { + for (let i = this.history.length - 1; i >= 0; i--) { + const entry = this.history[i]; + if (entry?.role !== 'model' || !entry.parts) continue; + const partIdx = entry.parts.findIndex( + (part) => + part.functionCall?.id === callId && + part.functionCall.name === ToolNames.EXIT_PLAN_MODE, + ); + if (partIdx === -1) continue; + const part = entry.parts[partIdx]!; + const functionCall = part.functionCall!; + if (typeof (functionCall.args ?? {})['plan'] !== 'string') { + return false; + } + const newParts = [...entry.parts]; + newParts[partIdx] = { + ...part, + functionCall: { + ...functionCall, + args: { ...functionCall.args, plan: replacement }, + }, + }; + this.history[i] = { ...entry, parts: newParts }; + return true; + } + return false; + } + setHistory(history: Content[]): void { this.history = history; // History replacement (compression, /clear, --resume reload) wipes diff --git a/packages/core/src/tools/exitPlanMode.ts b/packages/core/src/tools/exitPlanMode.ts index 6afcbb03f37..eb1113adf5c 100644 --- a/packages/core/src/tools/exitPlanMode.ts +++ b/packages/core/src/tools/exitPlanMode.ts @@ -86,6 +86,18 @@ const exitPlanModeToolSchemaData: FunctionDeclaration = { }, }; +/** + * `llmContent` prefixes that mark a successful plan-mode exit (user or + * leader approval). The tool scheduler keys its post-execution history + * sanitization (#6237) off these, so they must stay in lockstep with the + * success returns in `ExitPlanModeToolInvocation.execute` / + * `executePlanRequiredTeammate` below. + */ +export const PLAN_EXIT_APPROVED_LLM_CONTENT_PREFIXES = [ + 'User approved.', + 'Leader approved.', +] as const; + interface ExitApprovalSnapshot { plan: string; approvalModeRevision: number; From c054057f729ea8d89cc964fc38fba0bc3f9cf97e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=BA=B3=E4=BF=A1?= Date: Sun, 19 Jul 2026 18:07:11 +0800 Subject: [PATCH 2/6] fix(core): verify the saved plan file and redact approved plans on history load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on #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 --- .../core/src/core/coreToolScheduler.test.ts | 59 +++++++- packages/core/src/core/coreToolScheduler.ts | 24 ++- packages/core/src/core/geminiChat.test.ts | 123 ++++++++++++++++ packages/core/src/core/geminiChat.ts | 137 +++++++++++++++++- 4 files changed, 331 insertions(+), 12 deletions(-) diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 5bc53b7c113..18287cd94df 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -30,6 +30,8 @@ import { ToolErrorType, } from '../index.js'; import * as path from 'node:path'; +import * as os from 'node:os'; +import * as fsSync from 'node:fs'; import { pathToFileURL } from 'node:url'; import { SkillTool } from '../tools/skill.js'; import { StructuredToolError } from '../tools/priorReadEnforcement.js'; @@ -709,6 +711,7 @@ describe('CoreToolScheduler', () => { memoryMonitor?: { scheduleCheck: () => void }; toolOutputBatchBudget?: number; getGeminiClient?: () => unknown; + getPlanFilePath?: () => string; }) { const ensureTool = vi.fn( async (name: string) => @@ -764,7 +767,8 @@ describe('CoreToolScheduler', () => { getCwd: () => '/repo', getUseModelRouter: () => false, getGeminiClient: options.getGeminiClient ?? (() => null), - getPlanFilePath: () => '/tmp/plans/test-session-id.md', + getPlanFilePath: + options.getPlanFilePath ?? (() => '/tmp/plans/test-session-id.md'), getChatRecordingService: () => undefined, getMemoryPressureMonitor: () => options.memoryMonitor, getMessageBus: vi.fn().mockReturnValue(options.messageBus), @@ -904,6 +908,11 @@ describe('CoreToolScheduler', () => { it('redacts the plan argument from history after an approved exit_plan_mode', async () => { const bigPlan = '## Plan\n\n1. huge section\n2. code blocks\n3. tables'; + const planFile = path.join( + os.tmpdir(), + `qwen-plan-${process.pid}-${Math.random().toString(16).slice(2)}.md`, + ); + fsSync.writeFileSync(planFile, bigPlan, 'utf-8'); const chat = createChatWithPlanCall('plan-call-1', bigPlan); const tool = new MockTool({ name: ToolNames.EXIT_PLAN_MODE, @@ -923,6 +932,7 @@ describe('CoreToolScheduler', () => { approvalMode: ApprovalMode.YOLO, onAllToolCallsComplete, getGeminiClient: () => ({ getChat: () => chat }), + getPlanFilePath: () => planFile, }); await scheduler.schedule( @@ -943,13 +953,58 @@ describe('CoreToolScheduler', () => { const fnCall = history[1]!.parts![1]!.functionCall!; expect(fnCall.args!['plan']).not.toContain('huge section'); expect(fnCall.args!['plan']).toContain( - 'Plan approved and saved to /tmp/plans/test-session-id.md', + `Plan approved and saved to ${planFile}`, ); + fsSync.unlinkSync(planFile); // Sibling parts and other args survive untouched. expect(history[1]!.parts![0]).toEqual({ text: 'Here is my plan.' }); expect(fnCall.args!['originalRequest']).toBe('please plan this'); }); + it('keeps the plan argument when the plan file was never written (save failed)', async () => { + const bigPlan = '## Plan\n\nnothing on disk backs this'; + const chat = createChatWithPlanCall('plan-call-3', bigPlan); + const tool = new MockTool({ + name: ToolNames.EXIT_PLAN_MODE, + execute: vi.fn().mockResolvedValue({ + llmContent: + 'User approved. You can now start coding. Start with updating your todo list if applicable.', + returnDisplay: { + type: 'plan_summary', + message: 'User approved.', + plan: bigPlan, + }, + }), + }); + const onAllToolCallsComplete = vi.fn(); + const { scheduler } = createSchedulerForLegacyToolTests({ + toolsByName: new Map([[ToolNames.EXIT_PLAN_MODE, tool]]), + approvalMode: ApprovalMode.YOLO, + onAllToolCallsComplete, + getGeminiClient: () => ({ getChat: () => chat }), + getPlanFilePath: () => + path.join(os.tmpdir(), 'qwen-plan-that-does-not-exist.md'), + }); + + await scheduler.schedule( + [ + { + callId: 'plan-call-3', + name: ToolNames.EXIT_PLAN_MODE, + args: { plan: bigPlan }, + isClientInitiated: false, + prompt_id: 'prompt-plan-save-failed', + }, + ], + new AbortController().signal, + ); + await vi.waitFor(() => expect(onAllToolCallsComplete).toHaveBeenCalled()); + + // Never point the model at a file that is not there — the plan stays. + const fnCall = chat.getHistory()[1]!.parts![1]!.functionCall!; + expect(fnCall.args!['plan']).toBe(bigPlan); + }); + it('keeps the plan argument in history when exit_plan_mode is not approved', async () => { const bigPlan = '## Plan\n\nkeep me for revision'; const chat = createChatWithPlanCall('plan-call-2', bigPlan); diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 2c5cada3772..becdb336fee 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -58,6 +58,8 @@ import type { import { fileURLToPath } from 'node:url'; import { ToolNames, ToolNamesMigration } from '../tools/tool-names.js'; import { PLAN_EXIT_APPROVED_LLM_CONTENT_PREFIXES } from '../tools/exitPlanMode.js'; +import { approvedPlanRedactionText } from './geminiChat.js'; +import * as fsSync from 'node:fs'; import { collectAvailableSkillEntries, renderAvailableSkillsBlock, @@ -4357,22 +4359,28 @@ export class CoreToolScheduler { ) { try { const planPath = this.config.getPlanFilePath(); + // Gate on the on-disk plan actually matching the in-history + // text: savePlanBestEffort swallows filesystem errors, and the + // pointer must never claim a save that failed or reference a + // file holding a different plan. A missing/unreadable file + // throws here and skips the redaction entirely. + const savedPlan = fsSync.readFileSync(planPath, 'utf-8'); 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.]`, + approvedPlanRedactionText(planPath), + savedPlan, ); } catch (redactErr) { debugLogger.warn( - `Failed to redact approved plan from history for ${callId}: ${ - redactErr instanceof Error - ? redactErr.message - : String(redactErr) - }`, + `Skipping approved-plan redaction for ${callId} (plan file ` + + `unavailable?): ${ + redactErr instanceof Error + ? redactErr.message + : String(redactErr) + }`, ); } } diff --git a/packages/core/src/core/geminiChat.test.ts b/packages/core/src/core/geminiChat.test.ts index 23b3dd7344b..441f40dff23 100644 --- a/packages/core/src/core/geminiChat.test.ts +++ b/packages/core/src/core/geminiChat.test.ts @@ -17,6 +17,8 @@ import { AuthType, type ContentGenerator } from '../core/contentGenerator.js'; import { GeminiChat, InvalidStreamError, + approvedPlanRedactionText, + redactApprovedPlansInHistory, redactStructuredOutputArgsForRecording, StreamEventType, type StreamEvent, @@ -11451,6 +11453,127 @@ describe('GeminiChat', async () => { }); }); + describe('redactApprovedPlansInHistory (load-side, #6237)', () => { + // The chat-recording JSONL captures the assistant turn with the full + // plan argument before the tool runs, so --resume re-feeds the text the + // in-session redaction removed. These tests pin the history-wide pass + // applied on every wholesale history load. + const PLAN = '## Plan\n\nresume leak fixture'; + const PLAN_PATH = '/plans/session.md'; + + const approvedHistory = (): Content[] => [ + { role: 'user', parts: [{ text: 'plan it' }] }, + { + role: 'model', + parts: [ + { + functionCall: { + id: 'call-a', + name: 'exit_plan_mode', + args: { plan: PLAN, originalRequest: 'plan it' }, + }, + }, + ], + }, + { + role: 'user', + parts: [ + { + functionResponse: { + id: 'call-a', + name: 'exit_plan_mode', + response: { + output: + 'User approved. You can now start coding. Start with updating your todo list if applicable.', + }, + }, + }, + ], + }, + ]; + + it('rewrites approved calls whose plan matches the on-disk file', () => { + const out = redactApprovedPlansInHistory( + approvedHistory(), + PLAN, + PLAN_PATH, + ); + expect(out).not.toBeNull(); + const fnCall = out![1]!.parts![0]!.functionCall!; + expect(fnCall.args!['plan']).toBe(approvedPlanRedactionText(PLAN_PATH)); + expect(fnCall.args!['originalRequest']).toBe('plan it'); + expect(JSON.stringify(out)).not.toContain('resume leak fixture'); + }); + + it('returns null when the response was not an approval', () => { + const history = approvedHistory(); + ( + history[2]!.parts![0]!.functionResponse!.response as { + output: string; + } + ).output = 'Plan execution was not approved. Remaining in plan mode.'; + expect(redactApprovedPlansInHistory(history, PLAN, PLAN_PATH)).toBeNull(); + }); + + it('returns null when the on-disk plan differs (stale file)', () => { + expect( + redactApprovedPlansInHistory( + approvedHistory(), + 'a different, later plan', + PLAN_PATH, + ), + ).toBeNull(); + }); + + it('is applied by setHistory when the plan file exists', () => { + // The module-level node:fs mock backs readFileSync with + // mockFileSystem, so "writing" the plan file is a Map insert. + const planFile = '/plans/wired-session.md'; + mockFileSystem.set(planFile, PLAN); + try { + const chat = new GeminiChat( + { getPlanFilePath: () => planFile } as unknown as Config, + {}, + [], + ); + chat.setHistory(approvedHistory()); + const fnCall = chat.getHistory()[1]!.parts![0]!.functionCall!; + expect(fnCall.args!['plan']).toBe(approvedPlanRedactionText(planFile)); + } finally { + mockFileSystem.delete(planFile); + } + }); + + it('is applied by the constructor for rehydrated history', () => { + const planFile = '/plans/ctor-session.md'; + mockFileSystem.set(planFile, PLAN); + try { + const chat = new GeminiChat( + { getPlanFilePath: () => planFile } as unknown as Config, + {}, + approvedHistory(), + ); + const fnCall = chat.getHistory()[1]!.parts![0]!.functionCall!; + expect(fnCall.args!['plan']).toBe(approvedPlanRedactionText(planFile)); + } finally { + mockFileSystem.delete(planFile); + } + }); + + it('setHistory leaves history alone when no plan file exists', () => { + const chat = new GeminiChat( + { + getPlanFilePath: () => '/plans/never-written.md', + } as unknown as Config, + {}, + [], + ); + chat.setHistory(approvedHistory()); + const fnCall = chat.getHistory()[1]!.parts![0]!.functionCall!; + expect(fnCall.args!['plan']).toBe(PLAN); + }); + }); + describe('redactStructuredOutputArgsForRecording', () => { // The chat-recording JSONL persists assistant turns to disk and re-feeds // them on `--continue` / `--resume`. For `--json-schema` runs the diff --git a/packages/core/src/core/geminiChat.ts b/packages/core/src/core/geminiChat.ts index fca2cf74f43..a7a6d956669 100644 --- a/packages/core/src/core/geminiChat.ts +++ b/packages/core/src/core/geminiChat.ts @@ -47,6 +47,8 @@ import { } from './tokenLimits.js'; import { hasCycleInSchema } from '../tools/tools.js'; import { ToolNames } from '../tools/tool-names.js'; +import * as fs from 'node:fs'; +import { PLAN_EXIT_APPROVED_LLM_CONTENT_PREFIXES } from '../tools/exitPlanMode.js'; import { isManagedMemoryPath } from '../memory/paths.js'; import { STRUCTURED_OUTPUT_REDACTED_ARGS } from '../tools/syntheticOutput.js'; import type { StructuredError } from './turn.js'; @@ -181,6 +183,81 @@ function syncFunctionCallsField( * Exported for tests; callers should prefer the inline use inside * `recordAssistantTurn` invocation below. */ +/** + * 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. + */ +export function approvedPlanRedactionText(planPath: string): string { + return ( + `[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.]` + ); +} + +/** + * Pure history-wide variant of the approved-plan redaction: rewrites the + * `plan` argument of every `exit_plan_mode` `functionCall` whose paired + * `functionResponse` carries an approval `llmContent` AND whose plan text + * equals `savedPlanContent` (the current on-disk plan file). Returns a new + * array when anything changed, or null when the history is untouched. + * + * Exported for tests; production callers go through + * `GeminiChat.setHistory` / the constructor. + */ +export function redactApprovedPlansInHistory( + history: Content[], + savedPlanContent: string, + planPath: string, +): Content[] | null { + const approved = new Set(); + for (const entry of history) { + if (!entry?.parts) continue; + for (const part of entry.parts) { + const fr = part.functionResponse; + if (!fr?.id || fr.name !== ToolNames.EXIT_PLAN_MODE) continue; + const output = (fr.response as { output?: unknown } | undefined)?.[ + 'output' + ]; + if ( + typeof output === 'string' && + PLAN_EXIT_APPROVED_LLM_CONTENT_PREFIXES.some((prefix) => + output.startsWith(prefix), + ) + ) { + approved.add(fr.id); + } + } + } + if (approved.size === 0) return null; + + let changed = false; + const out = history.map((entry) => { + if (entry?.role !== 'model' || !entry.parts) return entry; + let entryChanged = false; + const parts = entry.parts.map((part) => { + 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; + entryChanged = true; + return { + ...part, + functionCall: { + ...fc, + args: { ...fc.args, plan: approvedPlanRedactionText(planPath) }, + }, + }; + }); + if (!entryChanged) return entry; + changed = true; + return { ...entry, parts }; + }); + return changed ? out : null; +} + export function redactStructuredOutputArgsForRecording( part: Part, ): { functionCall: NonNullable } | null { @@ -1589,6 +1666,7 @@ export class GeminiChat { private readonly telemetryService?: UiTelemetryService, ) { validateHistory(history); + this.redactApprovedPlansFromLoadedHistory(); } /** @@ -3533,12 +3611,22 @@ export class GeminiChat { * without losing information. Rejected plans are left untouched — the * model needs the text to revise them. * + * When `expectedPlan` is provided the rewrite additionally requires the + * in-history plan to equal it byte-for-byte. Callers pass the on-disk + * plan-file content here so the pointer can never claim a save that + * failed (`savePlanBestEffort` swallows filesystem errors) or reference + * a file that holds a different plan. + * * The entry is replaced immutably at the same index; the partial-push * markers compare by index and role, so this cannot desync them. * * @returns true when a matching functionCall was found and rewritten. */ - redactApprovedPlanFromHistory(callId: string, replacement: string): boolean { + redactApprovedPlanFromHistory( + callId: string, + replacement: string, + expectedPlan?: string, + ): boolean { for (let i = this.history.length - 1; i >= 0; i--) { const entry = this.history[i]; if (entry?.role !== 'model' || !entry.parts) continue; @@ -3550,7 +3638,11 @@ export class GeminiChat { if (partIdx === -1) continue; const part = entry.parts[partIdx]!; const functionCall = part.functionCall!; - if (typeof (functionCall.args ?? {})['plan'] !== 'string') { + const plan = (functionCall.args ?? {})['plan']; + if (typeof plan !== 'string') { + return false; + } + if (expectedPlan !== undefined && plan !== expectedPlan) { return false; } const newParts = [...entry.parts]; @@ -3567,6 +3659,46 @@ export class GeminiChat { return false; } + /** + * Read-side counterpart of {@link redactApprovedPlanFromHistory}: the + * chat-recording JSONL captured the assistant turn (with the full plan + * argument) before the tool ran, so a `--resume` / `--continue` reload + * re-feeds the plan text the in-session redaction already removed + * (#6237). Every wholesale history load re-applies the redaction to + * approved `exit_plan_mode` calls. + * + * Only calls whose in-history plan matches the current on-disk plan file + * are rewritten — same never-lie rule as the write side. With several + * approved plans in one session the file holds only the last one, so + * earlier calls rehydrate unredacted; safe, just not minimal. + */ + private redactApprovedPlansFromLoadedHistory(): void { + const hasPlanCall = this.history.some((entry) => + entry?.parts?.some( + (part) => part.functionCall?.name === ToolNames.EXIT_PLAN_MODE, + ), + ); + if (!hasPlanCall) return; + let planPath: string; + let savedPlan: string; + 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; + } + const redacted = redactApprovedPlansInHistory( + this.history, + savedPlan, + planPath, + ); + if (redacted) { + this.history = redacted; + } + } + setHistory(history: Content[]): void { this.history = history; // History replacement (compression, /clear, --resume reload) wipes @@ -3577,6 +3709,7 @@ export class GeminiChat { // push, corrupting the conversation. Drop the paired deferred-record // stash too: its referent (the model turn at the old index) is gone. this.clearPendingPartialState(); + this.redactApprovedPlansFromLoadedHistory(); } truncateHistory(keepCount: number): void { From 566dba220f0194fb1666ea84aa7bbde3634dd133 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=BA=B3=E4=BF=A1?= Date: Sun, 19 Jul 2026 20:24:15 +0800 Subject: [PATCH 3/6] test(core): cover leader-approved and stale-plan branches; restore orphaned doc Review follow-up on #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 --- .../core/src/core/coreToolScheduler.test.ts | 51 +++++++++++++++++++ packages/core/src/core/geminiChat.test.ts | 37 ++++++++++++++ packages/core/src/core/geminiChat.ts | 20 ++++++++ 3 files changed, 108 insertions(+) diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 18287cd94df..aeb5355b47a 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -961,6 +961,57 @@ describe('CoreToolScheduler', () => { expect(fnCall.args!['originalRequest']).toBe('please plan this'); }); + it('redacts the plan for a leader-approved (teammate) exit_plan_mode', async () => { + const bigPlan = '## Plan\n\nleader path fixture'; + const planFile = path.join( + os.tmpdir(), + `qwen-plan-leader-${process.pid}-${Math.random().toString(16).slice(2)}.md`, + ); + fsSync.writeFileSync(planFile, bigPlan, 'utf-8'); + const chat = createChatWithPlanCall('plan-call-4', bigPlan); + const tool = new MockTool({ + name: ToolNames.EXIT_PLAN_MODE, + execute: vi.fn().mockResolvedValue({ + llmContent: + 'Leader approved. You can now start coding. Start with updating your todo list if applicable.', + returnDisplay: { + type: 'plan_summary', + message: 'Leader approved.', + plan: bigPlan, + }, + }), + }); + const onAllToolCallsComplete = vi.fn(); + const { scheduler } = createSchedulerForLegacyToolTests({ + toolsByName: new Map([[ToolNames.EXIT_PLAN_MODE, tool]]), + approvalMode: ApprovalMode.YOLO, + onAllToolCallsComplete, + getGeminiClient: () => ({ getChat: () => chat }), + getPlanFilePath: () => planFile, + }); + + await scheduler.schedule( + [ + { + callId: 'plan-call-4', + name: ToolNames.EXIT_PLAN_MODE, + args: { plan: bigPlan }, + isClientInitiated: false, + prompt_id: 'prompt-plan-leader-approved', + }, + ], + new AbortController().signal, + ); + await vi.waitFor(() => expect(onAllToolCallsComplete).toHaveBeenCalled()); + + const fnCall = chat.getHistory()[1]!.parts![1]!.functionCall!; + expect(fnCall.args!['plan']).not.toContain('leader path fixture'); + expect(fnCall.args!['plan']).toContain( + `Plan approved and saved to ${planFile}`, + ); + fsSync.unlinkSync(planFile); + }); + it('keeps the plan argument when the plan file was never written (save failed)', async () => { const bigPlan = '## Plan\n\nnothing on disk backs this'; const chat = createChatWithPlanCall('plan-call-3', bigPlan); diff --git a/packages/core/src/core/geminiChat.test.ts b/packages/core/src/core/geminiChat.test.ts index 441f40dff23..36431a72dc9 100644 --- a/packages/core/src/core/geminiChat.test.ts +++ b/packages/core/src/core/geminiChat.test.ts @@ -11432,6 +11432,43 @@ describe('GeminiChat', async () => { ); }); + it('returns false when expectedPlan differs from the in-history plan', () => { + const chat = chatWith([ + { + role: 'model', + parts: [ + { + functionCall: { + id: 'call-plan', + name: 'exit_plan_mode', + args: { plan: 'SECRET BIG PLAN' }, + }, + }, + ], + }, + ]); + // Never-lie invariant: a stale/different on-disk plan blocks the + // rewrite entirely. + expect( + chat.redactApprovedPlanFromHistory( + 'call-plan', + REPLACEMENT, + 'a different plan', + ), + ).toBe(false); + expect(chat.getHistory()[0]!.parts![0]!.functionCall!.args!['plan']).toBe( + 'SECRET BIG PLAN', + ); + // Matching expectedPlan still rewrites. + expect( + chat.redactApprovedPlanFromHistory( + 'call-plan', + REPLACEMENT, + 'SECRET BIG PLAN', + ), + ).toBe(true); + }); + it('returns false when the matching call has no string plan arg', () => { const chat = chatWith([ { diff --git a/packages/core/src/core/geminiChat.ts b/packages/core/src/core/geminiChat.ts index a7a6d956669..d56cd853b43 100644 --- a/packages/core/src/core/geminiChat.ts +++ b/packages/core/src/core/geminiChat.ts @@ -163,6 +163,26 @@ function syncFunctionCallsField( } } +/** + * Replaces the args on a `structured_output` `functionCall` with the + * same `__redacted` placeholder used by `ToolCallEvent` telemetry + * (`packages/core/src/telemetry/types.ts`). + * + * The chat-recording JSONL (`/chats/.jsonl`) + * persists assistant turns to disk and re-feeds them on + * `--continue` / `--resume`. For `--json-schema` runs the tool args + * ARE the user's structured payload — already emitted on stdout via + * `result` / `structured_result`. Recording them verbatim here would + * mean the same payload (and every validation-failure retry along the + * way) sits on disk indefinitely, contradicting the privacy contract + * documented next to the telemetry redaction. Mirror the placeholder + * here so the chat-recording surface matches. + * + * Non-`structured_output` `functionCall`s pass through untouched. + * + * Exported for tests; callers should prefer the inline use inside + * `recordAssistantTurn` invocation below. + */ /** * Replaces the args on a `structured_output` `functionCall` with the * same `__redacted` placeholder used by `ToolCallEvent` telemetry From df521536ee0a09425b383b9fd9730a8c6d00f35e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=BA=B3=E4=BF=A1?= Date: Sun, 19 Jul 2026 23:37:50 +0800 Subject: [PATCH 4/6] fix(core): reattach the recording-redaction doc; canonicalize plan tool names on load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on #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 --- packages/core/src/core/geminiChat.test.ts | 68 +++++++++++++++++ packages/core/src/core/geminiChat.ts | 92 ++++++++++++----------- 2 files changed, 115 insertions(+), 45 deletions(-) diff --git a/packages/core/src/core/geminiChat.test.ts b/packages/core/src/core/geminiChat.test.ts index 36431a72dc9..3798fab964f 100644 --- a/packages/core/src/core/geminiChat.test.ts +++ b/packages/core/src/core/geminiChat.test.ts @@ -11542,6 +11542,74 @@ describe('GeminiChat', async () => { expect(JSON.stringify(out)).not.toContain('resume leak fixture'); }); + it('redacts only the approved call when a rejected call shares the plan text', () => { + const history: Content[] = [ + { role: 'user', parts: [{ text: 'plan it' }] }, + { + role: 'model', + parts: [ + { + functionCall: { + id: 'call-rejected', + name: 'exit_plan_mode', + args: { plan: PLAN }, + }, + }, + ], + }, + { + role: 'user', + parts: [ + { + functionResponse: { + id: 'call-rejected', + name: 'exit_plan_mode', + response: { + output: + 'Plan execution was not approved. Remaining in plan mode.', + }, + }, + }, + ], + }, + { + role: 'model', + parts: [ + { + functionCall: { + id: 'call-approved', + name: 'exit_plan_mode', + args: { plan: PLAN }, + }, + }, + ], + }, + { + role: 'user', + parts: [ + { + functionResponse: { + id: 'call-approved', + name: 'exit_plan_mode', + response: { + output: 'User approved. You can now start coding.', + }, + }, + }, + ], + }, + ]; + + const out = redactApprovedPlansInHistory(history, PLAN, PLAN_PATH); + expect(out).not.toBeNull(); + // The rejected call keeps its plan text (the model needs it for + // revision); only the approved call is rewritten. + expect(out![1]!.parts![0]!.functionCall!.args!['plan']).toBe(PLAN); + expect(out![3]!.parts![0]!.functionCall!.args!['plan']).toBe( + approvedPlanRedactionText(PLAN_PATH), + ); + }); + it('returns null when the response was not an approval', () => { const history = approvedHistory(); ( diff --git a/packages/core/src/core/geminiChat.ts b/packages/core/src/core/geminiChat.ts index d56cd853b43..e593d2eaf7d 100644 --- a/packages/core/src/core/geminiChat.ts +++ b/packages/core/src/core/geminiChat.ts @@ -46,7 +46,7 @@ import { parsePositiveIntegerEnvValue, } from './tokenLimits.js'; import { hasCycleInSchema } from '../tools/tools.js'; -import { ToolNames } from '../tools/tool-names.js'; +import { ToolNames, ToolNamesMigration } from '../tools/tool-names.js'; import * as fs from 'node:fs'; import { PLAN_EXIT_APPROVED_LLM_CONTENT_PREFIXES } from '../tools/exitPlanMode.js'; import { isManagedMemoryPath } from '../memory/paths.js'; @@ -163,52 +163,23 @@ function syncFunctionCallsField( } } -/** - * Replaces the args on a `structured_output` `functionCall` with the - * same `__redacted` placeholder used by `ToolCallEvent` telemetry - * (`packages/core/src/telemetry/types.ts`). - * - * The chat-recording JSONL (`/chats/.jsonl`) - * persists assistant turns to disk and re-feeds them on - * `--continue` / `--resume`. For `--json-schema` runs the tool args - * ARE the user's structured payload — already emitted on stdout via - * `result` / `structured_result`. Recording them verbatim here would - * mean the same payload (and every validation-failure retry along the - * way) sits on disk indefinitely, contradicting the privacy contract - * documented next to the telemetry redaction. Mirror the placeholder - * here so the chat-recording surface matches. - * - * Non-`structured_output` `functionCall`s pass through untouched. - * - * Exported for tests; callers should prefer the inline use inside - * `recordAssistantTurn` invocation below. - */ -/** - * Replaces the args on a `structured_output` `functionCall` with the - * same `__redacted` placeholder used by `ToolCallEvent` telemetry - * (`packages/core/src/telemetry/types.ts`). - * - * The chat-recording JSONL (`/chats/.jsonl`) - * persists assistant turns to disk and re-feeds them on - * `--continue` / `--resume`. For `--json-schema` runs the tool args - * ARE the user's structured payload — already emitted on stdout via - * `result` / `structured_result`. Recording them verbatim here would - * mean the same payload (and every validation-failure retry along the - * way) sits on disk indefinitely, contradicting the privacy contract - * documented next to the telemetry redaction. Mirror the placeholder - * here so the chat-recording surface matches. - * - * Non-`structured_output` `functionCall`s pass through untouched. - * - * Exported for tests; callers should prefer the inline use inside - * `recordAssistantTurn` invocation below. - */ /** * 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. */ +/** + * Local mirror of the scheduler's `canonicalToolName` (kept here to avoid a + * geminiChat -> coreToolScheduler import cycle): resolves legacy tool-name + * aliases so the load-side plan redaction keeps matching sessions recorded + * under a pre-migration name, in lockstep with the write-side scheduler. + */ +function canonicalPlanToolName(toolName: string | undefined): string { + if (!toolName) return ''; + return (ToolNamesMigration as Record)[toolName] ?? toolName; +} + export function approvedPlanRedactionText(planPath: string): string { return ( `[Plan approved and saved to ${planPath}. The plan text was ` + @@ -237,7 +208,11 @@ export function redactApprovedPlansInHistory( if (!entry?.parts) continue; for (const part of entry.parts) { const fr = part.functionResponse; - if (!fr?.id || fr.name !== ToolNames.EXIT_PLAN_MODE) continue; + if ( + !fr?.id || + canonicalPlanToolName(fr.name) !== ToolNames.EXIT_PLAN_MODE + ) + continue; const output = (fr.response as { output?: unknown } | undefined)?.[ 'output' ]; @@ -259,7 +234,11 @@ export function redactApprovedPlansInHistory( let entryChanged = false; const parts = entry.parts.map((part) => { const fc = part.functionCall; - if (!fc?.id || fc.name !== ToolNames.EXIT_PLAN_MODE) return part; + if ( + !fc?.id || + canonicalPlanToolName(fc.name) !== ToolNames.EXIT_PLAN_MODE + ) + return part; if (!approved.has(fc.id)) return part; if ((fc.args ?? {})['plan'] !== savedPlanContent) return part; entryChanged = true; @@ -278,6 +257,26 @@ export function redactApprovedPlansInHistory( return changed ? out : null; } +/** + * Replaces the args on a `structured_output` `functionCall` with the + * same `__redacted` placeholder used by `ToolCallEvent` telemetry + * (`packages/core/src/telemetry/types.ts`). + * + * The chat-recording JSONL (`/chats/.jsonl`) + * persists assistant turns to disk and re-feeds them on + * `--continue` / `--resume`. For `--json-schema` runs the tool args + * ARE the user's structured payload — already emitted on stdout via + * `result` / `structured_result`. Recording them verbatim here would + * mean the same payload (and every validation-failure retry along the + * way) sits on disk indefinitely, contradicting the privacy contract + * documented next to the telemetry redaction. Mirror the placeholder + * here so the chat-recording surface matches. + * + * Non-`structured_output` `functionCall`s pass through untouched. + * + * Exported for tests; callers should prefer the inline use inside + * `recordAssistantTurn` invocation below. + */ export function redactStructuredOutputArgsForRecording( part: Part, ): { functionCall: NonNullable } | null { @@ -3653,7 +3652,8 @@ export class GeminiChat { const partIdx = entry.parts.findIndex( (part) => part.functionCall?.id === callId && - part.functionCall.name === ToolNames.EXIT_PLAN_MODE, + canonicalPlanToolName(part.functionCall.name) === + ToolNames.EXIT_PLAN_MODE, ); if (partIdx === -1) continue; const part = entry.parts[partIdx]!; @@ -3695,7 +3695,9 @@ export class GeminiChat { private redactApprovedPlansFromLoadedHistory(): void { const hasPlanCall = this.history.some((entry) => entry?.parts?.some( - (part) => part.functionCall?.name === ToolNames.EXIT_PLAN_MODE, + (part) => + canonicalPlanToolName(part.functionCall?.name) === + ToolNames.EXIT_PLAN_MODE, ), ); if (!hasPlanCall) return; From 01c68e5c0a68933707fe5f27d8de4a2b4fa1a12c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=BA=B3=E4=BF=A1?= Date: Mon, 20 Jul 2026 11:46:39 +0800 Subject: [PATCH 5/6] fix(core): attach the pointer-text doc to its function; log the skipped load-side redaction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on #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 --- packages/core/src/core/geminiChat.ts | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/packages/core/src/core/geminiChat.ts b/packages/core/src/core/geminiChat.ts index e593d2eaf7d..fb4a925213c 100644 --- a/packages/core/src/core/geminiChat.ts +++ b/packages/core/src/core/geminiChat.ts @@ -163,12 +163,6 @@ function syncFunctionCallsField( } } -/** - * 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. - */ /** * Local mirror of the scheduler's `canonicalToolName` (kept here to avoid a * geminiChat -> coreToolScheduler import cycle): resolves legacy tool-name @@ -180,6 +174,12 @@ function canonicalPlanToolName(toolName: string | undefined): string { return (ToolNamesMigration as Record)[toolName] ?? toolName; } +/** + * 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. + */ export function approvedPlanRedactionText(planPath: string): string { return ( `[Plan approved and saved to ${planPath}. The plan text was ` + @@ -3706,9 +3706,14 @@ export class GeminiChat { try { planPath = this.config.getPlanFilePath(); savedPlan = fs.readFileSync(planPath, 'utf-8'); - } catch { + } 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. + // Logged (unlike a bare swallow) so a --resume that silently skips + // the redaction is traceable under DEBUG. + debugLogger.debug( + `Skipping load-side plan redaction, plan file unavailable: ${err}`, + ); return; } const redacted = redactApprovedPlansInHistory( From 074cf8dbcf25c9ee2270842c38494c59fee979cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=BA=B3=E4=BF=A1?= Date: Fri, 24 Jul 2026 21:50:22 +0800 Subject: [PATCH 6/6] fix(core): trace silent plan-redaction skips on both surfaces 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 #2). Co-Authored-By: Claude Fable 5 --- packages/core/src/core/coreToolScheduler.ts | 13 ++++++++++++- packages/core/src/core/geminiChat.ts | 9 +++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index 793bec567e1..77957504458 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -4650,7 +4650,7 @@ export class CoreToolScheduler { // file holding a different plan. A missing/unreadable file // throws here and skips the redaction entirely. const savedPlan = fsSync.readFileSync(planPath, 'utf-8'); - this.config + const redacted = this.config .getGeminiClient?.() ?.getChat() .redactApprovedPlanFromHistory( @@ -4658,6 +4658,17 @@ export class CoreToolScheduler { approvedPlanRedactionText(planPath), savedPlan, ); + // The rewrite declines silently on a no-match call id, a + // non-string plan argument, or in-history text differing from + // the saved file — trace those so a "plan still in history" + // report can tell a skipped rewrite from a run one. + if (redacted === false) { + debugLogger.debug( + `Approved-plan redaction left history unchanged for ` + + `${callId}: no matching exit_plan_mode call, or its ` + + `plan text does not match ${planPath}.`, + ); + } } catch (redactErr) { debugLogger.warn( `Skipping approved-plan redaction for ${callId} (plan file ` + diff --git a/packages/core/src/core/geminiChat.ts b/packages/core/src/core/geminiChat.ts index 183c24c3c02..ea3ecf73ea6 100644 --- a/packages/core/src/core/geminiChat.ts +++ b/packages/core/src/core/geminiChat.ts @@ -3731,6 +3731,15 @@ export class GeminiChat { ); if (redacted) { this.history = redacted; + } else { + // hasPlanCall was true, so a null here means every exit_plan_mode + // call was skipped (unapproved, id-less, or plan text differing + // from the saved file) — trace it for "plan still in history" + // triage, mirroring the write side. + debugLogger.debug( + `Load-side plan redaction left history unchanged: no approved ` + + `exit_plan_mode call matches the plan file at ${planPath}.`, + ); } }