-
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
Changes from 3 commits
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,9 @@ 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 { approvedPlanRedactionText } from './geminiChat.js'; | ||
| import * as fsSync from 'node:fs'; | ||
| import { | ||
| collectAvailableSkillEntries, | ||
| renderAvailableSkillsBlock, | ||
|
|
@@ -4338,6 +4341,49 @@ 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(); | ||
| // 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( | ||
|
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, | ||
| approvedPlanRedactionText(planPath), | ||
| savedPlan, | ||
| ); | ||
| } catch (redactErr) { | ||
| debugLogger.warn( | ||
| `Skipping approved-plan redaction for ${callId} (plan file ` + | ||
| `unavailable?): ${ | ||
| redactErr instanceof Error | ||
| ? redactErr.message | ||
| : String(redactErr) | ||
| }`, | ||
| ); | ||
| } | ||
| } | ||
| this.setStatusInternal(callId, 'success', successResponse); | ||
| safeSetStatus(span, { code: SpanStatusCode.OK }); | ||
| // Mirrors setToolSpanFailure/setToolSpanCancelled — every tool span | ||
|
|
||
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 前缀的调度器测试;该前缀被改名/删除会直接跑挂测试。