-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix(core): redact the plan argument from history after an approved exit_plan_mode #7197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
3f2fd1e
c054057
566dba2
df52153
01c68e5
982e24d
074cf8d
dda19ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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( | ||
|
Comment on lines
+4632
to
+4643
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The redaction rewrites the in-memory Failure scenario: User enters plan mode, model produces a large plan, user approves. In the current session the redaction works correctly. User later runs Suggested fix: Apply — qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented in c054057, at load time as suggested — but at the choke point rather than per resume path: 中文:已在 c054057 按建议在加载侧实现——但收在 choke point 而非逐个 resume 路径: |
||
| 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.]`, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The redaction message claims "Plan approved and saved to <path>" but Failure scenario: Filesystem runs out of space. User approves the plan. Suggested fix: Have — qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in c054057. The scheduler now reads the plan file before redacting, and 中文:已在 c054057 修复。调度器改写前先读 plan 文件, |
||
| ); | ||
| } 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
Comment on lines
+11537
to
+11541
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] No test exercises 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 中文确实是缺口,但按 wenshao 第 5 轮的意见,本 PR 只落两行日志改动、其余测试钉全部推 follow-up—— |
||
|
|
||
| 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, | ||
| ); | ||
|
Comment on lines
+11567
to
+11569
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The Fix: add a unit test, e.g. — qwen3.8-max-preview via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in 566dba2 — unit test drives the |
||
|
|
||
| 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 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion]
PLAN_EXIT_APPROVED_LLM_CONTENT_PREFIXESincludes'Leader approved.', but no test exercises that prefix — every approval-path test (including this one) uses'User approved.'as thellmContent. — 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 inPLAN_EXIT_APPROVED_LLM_CONTENT_PREFIXESnow fails a test. 中文:已补 leader 前缀的调度器测试;该前缀被改名/删除会直接跑挂测试。