diff --git a/docs/design/2026-07-26-manual-plan-exit-notice-delivery.md b/docs/design/2026-07-26-manual-plan-exit-notice-delivery.md new file mode 100644 index 00000000000..bb6ccdd5180 --- /dev/null +++ b/docs/design/2026-07-26-manual-plan-exit-notice-delivery.md @@ -0,0 +1,100 @@ +# Manual Plan-Exit Notice Delivery + +## Problem + +Plan mode is reinforced by a recurring reminder on model-bound user turns. When +the approval mode changes outside the approved `exit_plan_mode` flow, merely +stopping that reminder is not a reliable signal that Plan mode ended. + +The existing one-shot notice is assembled in `GeminiClient` for UserQuery and +Cron turns. That boundary misses model requests sent through other paths, +including tool-result continuations, steering, hooks, ACP/daemon direct sends, +and interactive agents. A single pending boolean on `Config` also lets one +conversation consume a notice intended for every live conversation sharing the +mode. + +## Scope + +The guarantee applies to live conversations in the current process. It does not +persist notices across process restarts and does not change approval checks, +plan approval, or tool execution. + +Notices are enabled for: + +- the main chat created by `GeminiClient.startChat`, including TUI, + non-interactive, ACP, daemon/Web UI, and replacement chats after compression; +- chats created by `AgentCore.createChat` with `interactive: true`. + +They remain disabled for fork/speculation, headless agents, workflows, memory +and compaction side queries, and every other `GeminiChat` unless explicitly +enabled. + +## State and Ownership + +`Config` keeps two independent pieces of in-memory state: + +- a mode event `{ version, kind }`, where `kind` is `clear` or `manual-exit`; +- a conversation cursor `{ seenVersion }`. + +The event is owned with the approval mode. A `Config` created with +`Object.create(parent)` inherits both the parent's approval mode and current +event. On the first write that creates an own approval mode, it copies the +current event and then becomes isolated from later parent events. + +The cursor is always lazily owned by the receiving `Config`. The main +conversation and each interactive agent can therefore claim the same inherited +event independently. Recreating a chat with the same `Config` retains its +cursor and does not deliver the event again. + +Mode transitions update the event as follows: + +- non-Plan to Plan increments the version and writes `clear`; +- Plan to non-Plan increments the version and writes `manual-exit`, except an + approved `exit_plan_mode` writes `clear`; +- non-Plan to non-Plan does not create an event. + +Entering Plan clears an undelivered older exit. A delivery reads the latest +approval mode, so a later non-Plan-to-non-Plan switch changes the mode named in +the pending notice without creating another notice. + +## Delivery and Failure Semantics + +`GeminiChat` exposes an idempotent opt-in. On each send it finishes asynchronous +compression and hard-rescue checks, then synchronously claims a pending event +immediately before committing the user content to history. The notice is added +as the final text part, preserving any function response parts before it. + +The linearization point is the successful history commit containing the notice. +Provider retries and fallbacks reuse that committed request and do not append a +second notice to history. If synchronous send setup throws and rolls back the +history push, the claim is restored only when the same manual-exit event is +still current, the mode is still non-Plan, and the cursor still points at that +version. A later mode event makes an old restore stale and harmless. + +The implementation cannot determine whether a provider received a failed +transport request. A transport retry may send the same request more than once, +but live chat history contains the notice at most once. + +Context-overflow recovery is the exception to reusing the original request: +reactive compression replaces live history before rebuilding the retry payload. +If its compressed history no longer contains the committed notice, the chat +re-appends that exact text before retrying. When compression already ends in a +user turn, the notice is added as its last part rather than creating adjacent +user turns. + +## Notice + +```text + +The approval mode changed outside the approved exit_plan_mode flow. +The current approval mode is: ${currentMode}. +Plan mode is no longer active. This notice supersedes any earlier reminder that Plan mode is active. Do not call exit_plan_mode; no plan approval is pending. Continue under the current mode's permissions and confirmation requirements. + +``` + +## Verification + +Unit tests cover transition semantics, inherited event ownership, independent +conversation cursors, stale restore behavior, opt-in delivery, part ordering, +setup rollback, retries, chat recreation, and chat ownership. The E2E plan +covers PTY, ACP, interactive agents, and approved plan exits. diff --git a/packages/core/src/agents/backends/InProcessBackend.test.ts b/packages/core/src/agents/backends/InProcessBackend.test.ts index 03be29debf7..a38f9dd4c5c 100644 --- a/packages/core/src/agents/backends/InProcessBackend.test.ts +++ b/packages/core/src/agents/backends/InProcessBackend.test.ts @@ -11,7 +11,7 @@ import type { AgentSpawnConfig } from './types.js'; import { AgentCore } from '../runtime/agent-core.js'; import { getTeammateContext } from '../team/identity.js'; import { createContentGenerator } from '../../core/contentGenerator.js'; -import { ApprovalMode, type Config } from '../../config/config.js'; +import { ApprovalMode, Config } from '../../config/config.js'; const DEFAULT_MODE = 'default' as ApprovalMode; const PLAN_MODE = 'plan' as ApprovalMode; @@ -671,6 +671,56 @@ describe('InProcessBackend', () => { expect(parentConfig.setApprovalMode).not.toHaveBeenCalled(); }); + it('copies the inherited plan-exit event into a per-agent mode override', async () => { + const parentConfig = createMockConfig() as unknown as Record< + string, + unknown + >; + Object.assign(parentConfig, { + approvalMode: ApprovalMode.DEFAULT, + manualPlanExitNoticeEventState: { + version: 2, + kind: 'manual-exit', + }, + takePendingManualPlanExitNotice: + Config.prototype.takePendingManualPlanExitNotice, + restorePendingManualPlanExitNotice: + Config.prototype.restorePendingManualPlanExitNotice, + }); + const backendWithParentMode = new InProcessBackend( + parentConfig as unknown as Config, + ); + await backendWithParentMode.init(); + + const config = createSpawnConfig('agent-1'); + config.inProcess!.approvalMode = ApprovalMode.AUTO_EDIT; + await backendWithParentMode.spawnAgent(config); + + const MockAgentCore = AgentCore as unknown as ReturnType; + const { runtimeContext } = destructureAgentCoreCall( + MockAgentCore.mock.calls.at(-1)!, + ); + const agentContext = runtimeContext as unknown as Config; + expect(agentContext.takePendingManualPlanExitNotice()).toEqual({ + version: 2, + currentMode: ApprovalMode.AUTO_EDIT, + }); + + Object.assign(parentConfig['manualPlanExitNoticeEventState'] as object, { + version: 3, + kind: 'manual-exit', + }); + expect(agentContext.takePendingManualPlanExitNotice()).toBeUndefined(); + expect( + Config.prototype.takePendingManualPlanExitNotice.call( + parentConfig as unknown as Config, + ), + ).toEqual({ + version: 3, + currentMode: ApprovalMode.DEFAULT, + }); + }); + it('restores a plan-mode per-agent config to default without mutating the parent config', async () => { const parentConfig = createMockConfig() as unknown as { getApprovalMode: ReturnType; diff --git a/packages/core/src/agents/backends/InProcessBackend.ts b/packages/core/src/agents/backends/InProcessBackend.ts index 1f84a8bcaed..2e195dd8ab0 100644 --- a/packages/core/src/agents/backends/InProcessBackend.ts +++ b/packages/core/src/agents/backends/InProcessBackend.ts @@ -624,6 +624,12 @@ function createApprovalModeConfigOverride( }; override.approvalMode = initialMode; + override.manualPlanExitNoticeEventState = { + ...(override.manualPlanExitNoticeEventState ?? { + version: 0, + kind: 'clear', + }), + }; override.getApprovalMode = Config.prototype.getApprovalMode; override.prePlanMode = initialMode === ApprovalMode.PLAN diff --git a/packages/core/src/agents/runtime/agent-core.test.ts b/packages/core/src/agents/runtime/agent-core.test.ts index fb7bacf6737..1deecc8000f 100644 --- a/packages/core/src/agents/runtime/agent-core.test.ts +++ b/packages/core/src/agents/runtime/agent-core.test.ts @@ -45,6 +45,36 @@ import { runWithInvocationContext, type InvocationContextV1, } from '../../utils/invocation-context.js'; +import { GeminiChat } from '../../core/geminiChat.js'; +import { ContextState } from './agent-headless.js'; + +describe('AgentCore.createChat manual plan-exit notice ownership', () => { + it('enables notices only for interactive agent chats', async () => { + const core = new AgentCore( + 'notice-agent', + {} as Config, + { renderedSystemPrompt: 'system', initialMessages: [] }, + { model: 'test-model' }, + { max_turns: 1 }, + ); + const enableSpy = vi.spyOn( + GeminiChat.prototype, + 'enableManualPlanExitNotices', + ); + + const interactiveChat = await core.createChat(new ContextState(), { + interactive: true, + }); + expect(interactiveChat).toBeDefined(); + expect(enableSpy).toHaveBeenCalledTimes(1); + + const headlessChat = await core.createChat(new ContextState()); + expect(headlessChat).toBeDefined(); + expect(enableSpy).toHaveBeenCalledTimes(1); + + enableSpy.mockRestore(); + }); +}); describe('AgentCore.runInAgentFrames', () => { // The deferred-approval `respond` callback that AgentCore hands to the diff --git a/packages/core/src/agents/runtime/agent-core.ts b/packages/core/src/agents/runtime/agent-core.ts index d718fd7500d..c411ee35db6 100644 --- a/packages/core/src/agents/runtime/agent-core.ts +++ b/packages/core/src/agents/runtime/agent-core.ts @@ -470,6 +470,9 @@ export class AgentCore { generationConfig, startHistory, ); + if (options?.interactive) { + chat.enableManualPlanExitNotices(); + } // Seed the per-chat token count so the auto-compaction threshold // gate sees the inherited history's true size on the first send. // Without this, fork subagents start at 0 and the gate NOOPs even diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 287f1f56629..8c5dcef9d8a 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -6522,6 +6522,138 @@ describe('setApprovalMode with folder trust', () => { expect(config.consumePendingManualPlanExitNotice()).toBe(false); }); + it('claims the latest non-plan mode and supports a matching restore', () => { + const config = new Config(baseParams); + vi.spyOn(config, 'isTrustedFolder').mockReturnValue(true); + + config.setApprovalMode(ApprovalMode.PLAN); + config.setApprovalMode(ApprovalMode.DEFAULT); + config.setApprovalMode(ApprovalMode.YOLO); + + const notice = config.takePendingManualPlanExitNotice(); + expect(notice).toEqual({ + version: expect.any(Number), + currentMode: ApprovalMode.YOLO, + }); + expect(config.takePendingManualPlanExitNotice()).toBeUndefined(); + + config.restorePendingManualPlanExitNotice(notice!.version); + expect(config.takePendingManualPlanExitNotice()).toEqual(notice); + }); + + it('ignores a restore after a newer mode event', () => { + const config = new Config(baseParams); + vi.spyOn(config, 'isTrustedFolder').mockReturnValue(true); + + config.setApprovalMode(ApprovalMode.PLAN); + config.setApprovalMode(ApprovalMode.DEFAULT); + const staleNotice = config.takePendingManualPlanExitNotice()!; + + config.setApprovalMode(ApprovalMode.PLAN); + config.setApprovalMode(ApprovalMode.AUTO_EDIT); + config.restorePendingManualPlanExitNotice(staleNotice.version); + + const currentNotice = config.takePendingManualPlanExitNotice(); + expect(currentNotice?.version).toBeGreaterThan(staleNotice.version); + expect(currentNotice?.currentMode).toBe(ApprovalMode.AUTO_EDIT); + expect(config.takePendingManualPlanExitNotice()).toBeUndefined(); + }); + + it('delivers the same inherited event once to each conversation', () => { + const parent = new Config(baseParams); + vi.spyOn(parent, 'isTrustedFolder').mockReturnValue(true); + const child = Object.create(parent) as Config; + + parent.setApprovalMode(ApprovalMode.PLAN); + parent.setApprovalMode(ApprovalMode.DEFAULT); + + const parentNotice = parent.takePendingManualPlanExitNotice(); + const childNotice = child.takePendingManualPlanExitNotice(); + expect(parentNotice).toEqual(childNotice); + expect(parent.takePendingManualPlanExitNotice()).toBeUndefined(); + expect(child.takePendingManualPlanExitNotice()).toBeUndefined(); + }); + + it('lets a newly created conversation claim the latest inherited event', () => { + const parent = new Config(baseParams); + vi.spyOn(parent, 'isTrustedFolder').mockReturnValue(true); + + parent.setApprovalMode(ApprovalMode.PLAN); + parent.setApprovalMode(ApprovalMode.DEFAULT); + const parentNotice = parent.takePendingManualPlanExitNotice(); + const child = Object.create(parent) as Config; + + expect(child.takePendingManualPlanExitNotice()).toEqual(parentNotice); + }); + + it('copies the event when a child first owns its approval mode', () => { + const parent = new Config(baseParams); + vi.spyOn(parent, 'isTrustedFolder').mockReturnValue(true); + const child = Object.create(parent) as Config; + + parent.setApprovalMode(ApprovalMode.PLAN); + parent.setApprovalMode(ApprovalMode.DEFAULT); + child.setApprovalMode(ApprovalMode.AUTO_EDIT); + + expect(child.takePendingManualPlanExitNotice()?.currentMode).toBe( + ApprovalMode.AUTO_EDIT, + ); + + parent.setApprovalMode(ApprovalMode.PLAN); + parent.setApprovalMode(ApprovalMode.DEFAULT); + expect(child.takePendingManualPlanExitNotice()).toBeUndefined(); + expect(parent.takePendingManualPlanExitNotice()?.currentMode).toBe( + ApprovalMode.DEFAULT, + ); + + child.setApprovalMode(ApprovalMode.PLAN); + child.setApprovalMode(ApprovalMode.YOLO); + expect(child.takePendingManualPlanExitNotice()?.currentMode).toBe( + ApprovalMode.YOLO, + ); + expect(parent.takePendingManualPlanExitNotice()).toBeUndefined(); + }); + + it('isolates an inherited event when approval mode is owned directly', () => { + const parent = new Config(baseParams); + vi.spyOn(parent, 'isTrustedFolder').mockReturnValue(true); + parent.setApprovalMode(ApprovalMode.PLAN); + parent.setApprovalMode(ApprovalMode.DEFAULT); + + const child = Object.create(parent) as Config; + Object.defineProperty(child, 'approvalMode', { + value: ApprovalMode.AUTO_EDIT, + writable: true, + configurable: true, + }); + + expect(child.takePendingManualPlanExitNotice()?.currentMode).toBe( + ApprovalMode.AUTO_EDIT, + ); + + parent.setApprovalMode(ApprovalMode.PLAN); + parent.setApprovalMode(ApprovalMode.DEFAULT); + expect(child.takePendingManualPlanExitNotice()).toBeUndefined(); + expect(parent.takePendingManualPlanExitNotice()?.currentMode).toBe( + ApprovalMode.DEFAULT, + ); + }); + + it('only exposes the latest event after rapid Plan round trips', () => { + const config = new Config(baseParams); + vi.spyOn(config, 'isTrustedFolder').mockReturnValue(true); + + config.setApprovalMode(ApprovalMode.PLAN); + config.setApprovalMode(ApprovalMode.DEFAULT); + config.setApprovalMode(ApprovalMode.PLAN); + config.setApprovalMode(ApprovalMode.YOLO); + + expect(config.takePendingManualPlanExitNotice()?.currentMode).toBe( + ApprovalMode.YOLO, + ); + expect(config.takePendingManualPlanExitNotice()).toBeUndefined(); + }); + it('records prePlanMode=yolo for a Shift+Tab cycle into plan mode', () => { const config = new Config(baseParams); vi.spyOn(config, 'isTrustedFolder').mockReturnValue(true); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 8a7c132d54f..50c75c6f912 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -312,6 +312,22 @@ export interface ApprovalModeInfo { description: string; } +type ManualPlanExitNoticeEventKind = 'clear' | 'manual-exit'; + +interface ManualPlanExitNoticeEventState { + version: number; + kind: ManualPlanExitNoticeEventKind; +} + +interface ManualPlanExitNoticeCursorState { + seenVersion: number; +} + +export interface ManualPlanExitNotice { + version: number; + currentMode: ApprovalMode; +} + /** * Detailed information about each approval mode. * Used for UI display and protocol responses. @@ -1769,7 +1785,13 @@ export class Config { private approvalMode: ApprovalMode; private prePlanMode?: ApprovalMode; private approvalModeRevision = 0; - private pendingManualPlanExitNotice = false; + private manualPlanExitNoticeEventState: ManualPlanExitNoticeEventState = { + version: 0, + kind: 'clear', + }; + private manualPlanExitNoticeCursorState: ManualPlanExitNoticeCursorState = { + seenVersion: 0, + }; private autoModeDenialState: AutoModeDenialState = createDenialState(); private readonly accessibility: AccessibilitySettings; private readonly showResponseTokensPerSecond: boolean; @@ -5422,6 +5444,34 @@ export class Config { return this.approvalModeRevision; } + private getManualPlanExitNoticeEventState(): ManualPlanExitNoticeEventState { + if ( + !Object.prototype.hasOwnProperty.call( + this, + 'manualPlanExitNoticeEventState', + ) && + Object.prototype.hasOwnProperty.call(this, 'approvalMode') + ) { + const inheritedEvent = this.manualPlanExitNoticeEventState; + this.manualPlanExitNoticeEventState = inheritedEvent + ? { ...inheritedEvent } + : { version: 0, kind: 'clear' }; + } + return this.manualPlanExitNoticeEventState; + } + + private getOwnManualPlanExitNoticeCursorState(): ManualPlanExitNoticeCursorState { + if ( + !Object.prototype.hasOwnProperty.call( + this, + 'manualPlanExitNoticeCursorState', + ) + ) { + this.manualPlanExitNoticeCursorState = { seenVersion: 0 }; + } + return this.manualPlanExitNoticeCursorState; + } + setApprovalMode( mode: ApprovalMode, options?: { @@ -5462,20 +5512,22 @@ export class Config { } // Update all mode bookkeeping only after fallible transition work has // succeeded, so callers never observe a partially applied mode change. + let noticeEvent = + Config.prototype.getManualPlanExitNoticeEventState.call(this); + if (!Object.prototype.hasOwnProperty.call(this, 'approvalMode')) { + noticeEvent = { ...noticeEvent }; + this.manualPlanExitNoticeEventState = noticeEvent; + } if (mode === ApprovalMode.PLAN && fromMode !== ApprovalMode.PLAN) { this.prePlanMode = fromMode; - // A stale exit notice must not survive a re-entry: the plan-mode - // reminder takes over again on the next turn. - this.pendingManualPlanExitNotice = false; + noticeEvent.version++; + noticeEvent.kind = 'clear'; } else if (mode !== ApprovalMode.PLAN && fromMode === ApprovalMode.PLAN) { this.prePlanMode = undefined; - if (!options?.fromApprovedPlanExit) { - // While in plan mode the model is told "plan mode is active" on - // every turn; on a manual exit that reminder just stops appearing, - // which models do not reliably notice (#7671). Queue an explicit - // one-shot exit notice for the next turn's reminder assembly. - this.pendingManualPlanExitNotice = true; - } + noticeEvent.version++; + noticeEvent.kind = options?.fromApprovedPlanExit + ? 'clear' + : 'manual-exit'; } // Any deliberate mode change invalidates the AUTO denialTracking signal. if (fromMode !== mode) { @@ -5488,14 +5540,48 @@ export class Config { } /** - * One-shot: returns whether a manual (non-approved) plan-mode exit is - * pending model notification, and clears the flag. Consumed by the - * system-reminder assembly in `GeminiClient` on the next model-bound turn. + * Claims the latest manual plan-exit notice for this conversation. */ + takePendingManualPlanExitNotice(): ManualPlanExitNotice | undefined { + const event = Config.prototype.getManualPlanExitNoticeEventState.call(this); + const cursor = + Config.prototype.getOwnManualPlanExitNoticeCursorState.call(this); + if (event.version <= cursor.seenVersion) { + return undefined; + } + + cursor.seenVersion = event.version; + if ( + event.kind !== 'manual-exit' || + this.approvalMode === ApprovalMode.PLAN + ) { + return undefined; + } + + return { + version: event.version, + currentMode: this.approvalMode, + }; + } + + restorePendingManualPlanExitNotice(version: number): void { + const event = Config.prototype.getManualPlanExitNoticeEventState.call(this); + const cursor = + Config.prototype.getOwnManualPlanExitNoticeCursorState.call(this); + if ( + event.version === version && + event.kind === 'manual-exit' && + this.approvalMode !== ApprovalMode.PLAN && + cursor.seenVersion === version + ) { + cursor.seenVersion = Math.max(0, version - 1); + } + } + consumePendingManualPlanExitNotice(): boolean { - const pending = this.pendingManualPlanExitNotice; - this.pendingManualPlanExitNotice = false; - return pending; + return ( + Config.prototype.takePendingManualPlanExitNotice.call(this) !== undefined + ); } /** diff --git a/packages/core/src/core/client.test.ts b/packages/core/src/core/client.test.ts index 697f0841257..515342d79e1 100644 --- a/packages/core/src/core/client.test.ts +++ b/packages/core/src/core/client.test.ts @@ -571,7 +571,8 @@ describe('Gemini Client (client.ts)', () => { getNoBrowser: vi.fn().mockReturnValue(false), getUsageStatisticsEnabled: vi.fn().mockReturnValue(true), getApprovalMode: vi.fn().mockReturnValue(ApprovalMode.DEFAULT), - consumePendingManualPlanExitNotice: vi.fn().mockReturnValue(false), + takePendingManualPlanExitNotice: vi.fn().mockReturnValue(undefined), + restorePendingManualPlanExitNotice: vi.fn(), getSdkMode: vi.fn().mockReturnValue(false), getExperimentalZedIntegration: vi.fn().mockReturnValue(false), isInteractive: vi.fn().mockReturnValue(false), @@ -975,6 +976,21 @@ describe('Gemini Client (client.ts)', () => { sessionStartProfilerMocks.profilers.length = 0; }); + it('enables manual plan-exit notices on every main chat', async () => { + const enableSpy = vi.spyOn( + GeminiChat.prototype, + 'enableManualPlanExitNotices', + ); + + await client.startChat(); + await client.startChat( + [{ role: 'user', parts: [{ text: 'resumed' }] }], + SessionStartSource.Compact, + ); + + expect(enableSpy).toHaveBeenCalledTimes(2); + }); + it('passes startup, resume, and clear sources to the profiler', async () => { await client.startChat(); await client.startChat([{ role: 'user', parts: [{ text: 'hi' }] }]); @@ -6243,74 +6259,6 @@ hello ); }); - it('injects a one-shot exit notice after a manual plan-mode exit', async () => { - vi.mocked(mockConfig.getApprovalMode).mockReturnValue( - ApprovalMode.DEFAULT, - ); - vi.mocked( - mockConfig.consumePendingManualPlanExitNotice, - ).mockReturnValueOnce(true); - const mockStream = (async function* () { - yield { type: 'content', value: 'Continuing' }; - })(); - mockTurnRunFn.mockReturnValue(mockStream); - client['chat'] = { - addHistory: vi.fn(), - getHistory: vi.fn().mockReturnValue([]), - } as unknown as GeminiChat; - - const stream = client.sendMessageStream( - [{ text: 'Now implement it' }], - new AbortController().signal, - 'prompt-id-manual-plan-exit', - ); - for await (const _ of stream) { - // consume stream - } - - expect(mockTurnRunFn).toHaveBeenCalledWith( - 'test-model', - expect.arrayContaining([ - expect.stringContaining( - 'The user has manually switched out of plan mode', - ), - ]), - expect.any(AbortSignal), - ); - }); - - it('does not inject the exit notice when none is pending', async () => { - vi.mocked(mockConfig.getApprovalMode).mockReturnValue( - ApprovalMode.DEFAULT, - ); - const mockStream = (async function* () { - yield { type: 'content', value: 'Reply' }; - })(); - mockTurnRunFn.mockReturnValue(mockStream); - client['chat'] = { - addHistory: vi.fn(), - getHistory: vi.fn().mockReturnValue([]), - } as unknown as GeminiChat; - - const stream = client.sendMessageStream( - [{ text: 'Hello' }], - new AbortController().signal, - 'prompt-id-no-plan-exit-notice', - ); - for await (const _ of stream) { - // consume stream - } - - const requestArg = mockTurnRunFn.mock.calls.at(-1)![1] as string[]; - expect( - requestArg.some( - (part) => - typeof part === 'string' && - part.includes('manually switched out of plan mode'), - ), - ).toBe(false); - }); - it('uses the subagent plan reminder when a subagent inherits PLAN mode', async () => { vi.mocked(mockConfig.getApprovalMode).mockReturnValue(ApprovalMode.PLAN); vi.mocked(mockConfig.getSdkMode).mockReturnValue(false); diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index 03c3937d57c..b7b534e7708 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -53,7 +53,6 @@ import { getArenaSystemReminder, getCoreSystemPrompt, getCustomSystemPrompt, - getManualPlanExitSystemReminder, getPlanModeSystemReminder, resolveInteractionMode, } from './prompts.js'; @@ -1406,6 +1405,7 @@ export class GeminiClient { uiTelemetryService, ), ); + chat.enableManualPlanExitNotices(); this.chat = chat; // Repair any dangling `model[functionCall]` whose `functionResponse` @@ -2465,15 +2465,6 @@ export class GeminiClient { this.config.getSdkMode(), ), ); - } else if (this.config.consumePendingManualPlanExitNotice()) { - // One-shot counterpart to the reminder above: the model was told - // "plan mode is active" on every turn, so a manual exit - // (Shift+Tab, /approval-mode, /plan) needs an explicit signal — - // the reminder silently disappearing goes unnoticed and the - // model keeps calling exit_plan_mode (#7671). - systemReminders.push( - getManualPlanExitSystemReminder(this.config.getApprovalMode()), - ); } // add arena system reminder if an arena session is active diff --git a/packages/core/src/core/geminiChat.test.ts b/packages/core/src/core/geminiChat.test.ts index d8062605163..7665fa48f01 100644 --- a/packages/core/src/core/geminiChat.test.ts +++ b/packages/core/src/core/geminiChat.test.ts @@ -44,6 +44,7 @@ import { getToolCallPreparations, setToolCallPreparations, } from './tool-call-preparation.js'; +import { ApprovalMode } from '../config/approval-mode.js'; // Mock fs module to prevent actual file system operations during tests const mockFileSystem = new Map(); @@ -194,6 +195,8 @@ describe('GeminiChat', async () => { .fn() .mockReturnValue({ debug: vi.fn(), warn: vi.fn(), info: vi.fn() }), getApprovalMode: vi.fn().mockReturnValue('default'), + takePendingManualPlanExitNotice: vi.fn().mockReturnValue(undefined), + restorePendingManualPlanExitNotice: vi.fn(), getFileReadCache: vi.fn().mockReturnValue({ clear: vi.fn() }), } as unknown as Config; @@ -406,6 +409,382 @@ describe('GeminiChat', async () => { expect(mockSleepInhibitorRelease).toHaveBeenCalledTimes(1); }); + describe('manual plan-exit notices', () => { + beforeEach(() => { + vi.mocked( + mockContentGenerator.generateContentStream, + ).mockImplementation(async () => + streamResponse(stopResponse([{ text: 'ok' }])), + ); + }); + + it('is disabled by default', async () => { + vi.mocked(mockConfig.takePendingManualPlanExitNotice).mockReturnValue({ + version: 1, + currentMode: ApprovalMode.DEFAULT, + }); + + const stream = await chat.sendMessageStream( + 'test-model', + { message: 'continue' }, + 'prompt-id-plan-exit-disabled', + ); + for await (const _ of stream) { + /* consume */ + } + + expect( + mockConfig.takePendingManualPlanExitNotice, + ).not.toHaveBeenCalled(); + expect( + chat + .getHistory() + .flatMap((content) => content.parts ?? []) + .some((part) => + part.text?.includes( + 'changed outside the approved exit_plan_mode flow', + ), + ), + ).toBe(false); + }); + + it('appends one notice after a function response', async () => { + vi.mocked(mockConfig.takePendingManualPlanExitNotice) + .mockReturnValueOnce({ + version: 7, + currentMode: ApprovalMode.AUTO_EDIT, + }) + .mockReturnValue(undefined); + chat.enableManualPlanExitNotices(); + chat.setHistory([ + { role: 'user', parts: [{ text: 'read it' }] }, + { + role: 'model', + parts: [ + { + functionCall: { + id: 'call-plan-exit', + name: 'read_file', + args: { path: '/tmp/input' }, + }, + }, + ], + }, + ]); + + const firstStream = await chat.sendMessageStream( + 'test-model', + { + message: { + functionResponse: { + id: 'call-plan-exit', + name: 'read_file', + response: { output: 'contents' }, + }, + }, + }, + 'prompt-id-plan-exit-tool-result', + ); + for await (const _ of firstStream) { + /* consume */ + } + + const secondStream = await chat.sendMessageStream( + 'test-model', + { message: 'next turn' }, + 'prompt-id-plan-exit-next-turn', + ); + for await (const _ of secondStream) { + /* consume */ + } + + const toolResultTurn = chat.getHistory()[2]!; + expect(toolResultTurn.parts?.[0]?.functionResponse?.id).toBe( + 'call-plan-exit', + ); + expect(toolResultTurn.parts?.at(-1)?.text).toContain( + 'The current approval mode is: auto-edit.', + ); + expect( + chat + .getHistory() + .flatMap((content) => content.parts ?? []) + .filter((part) => + part.text?.includes( + 'changed outside the approved exit_plan_mode flow', + ), + ), + ).toHaveLength(1); + }); + + it('restores a claim when setup rolls back the history push', async () => { + vi.mocked(mockConfig.takePendingManualPlanExitNotice).mockReturnValue({ + version: 11, + currentMode: ApprovalMode.DEFAULT, + }); + chat.enableManualPlanExitNotices(); + vi.spyOn( + chat as unknown as { getRequestHistory: () => Content[] }, + 'getRequestHistory', + ).mockImplementationOnce(() => { + throw new Error('history setup failed'); + }); + + await expect( + chat.sendMessageStream( + 'test-model', + { message: 'first' }, + 'prompt-id-plan-exit-rollback-1', + ), + ).rejects.toThrow('history setup failed'); + + expect( + mockConfig.restorePendingManualPlanExitNotice, + ).toHaveBeenCalledWith(11); + + const stream = await chat.sendMessageStream( + 'test-model', + { message: 'second' }, + 'prompt-id-plan-exit-rollback-2', + ); + for await (const _ of stream) { + /* consume */ + } + + const history = chat.getHistory(); + expect( + history.some((content) => + content.parts?.some((part) => part.text === 'first'), + ), + ).toBe(false); + expect( + history + .flatMap((content) => content.parts ?? []) + .filter((part) => + part.text?.includes( + 'changed outside the approved exit_plan_mode flow', + ), + ), + ).toHaveLength(1); + expect( + mockConfig.restorePendingManualPlanExitNotice, + ).toHaveBeenCalledTimes(1); + }); + + it('commits one history part when provider setup retries', async () => { + vi.mocked( + mockConfig.takePendingManualPlanExitNotice, + ).mockReturnValueOnce({ + version: 13, + currentMode: ApprovalMode.DEFAULT, + }); + chat.enableManualPlanExitNotices(); + vi.mocked(mockContentGenerator.generateContentStream) + .mockRejectedValueOnce(new Error('transient transport setup')) + .mockImplementationOnce(async () => + streamResponse(stopResponse([{ text: 'recovered' }])), + ); + mockRetryWithBackoff.mockImplementationOnce(async (apiCall) => { + try { + return await apiCall(); + } catch { + return apiCall(); + } + }); + + const stream = await chat.sendMessageStream( + 'test-model', + { message: 'retry me' }, + 'prompt-id-plan-exit-provider-retry', + ); + for await (const _ of stream) { + /* consume */ + } + + expect( + mockContentGenerator.generateContentStream, + ).toHaveBeenCalledTimes(2); + expect( + mockConfig.takePendingManualPlanExitNotice, + ).toHaveBeenCalledTimes(1); + expect( + chat + .getHistory() + .flatMap((content) => content.parts ?? []) + .filter((part) => + part.text?.includes( + 'changed outside the approved exit_plan_mode flow', + ), + ), + ).toHaveLength(1); + expect( + mockConfig.restorePendingManualPlanExitNotice, + ).not.toHaveBeenCalled(); + }); + + it.each([ + { + tail: 'model', + compressedHistory: [ + { role: 'user', parts: [{ text: 'summary' }] }, + { role: 'model', parts: [{ text: 'ack' }] }, + ] satisfies Content[], + }, + { + tail: 'user', + compressedHistory: [ + { role: 'user', parts: [{ text: 'summary' }] }, + { role: 'model', parts: [{ text: 'ack' }] }, + { + role: 'user', + parts: [{ text: 'restored attachment context' }], + }, + ] satisfies Content[], + }, + ])( + 'preserves the committed notice across reactive compression with a $tail tail', + async ({ compressedHistory }) => { + vi.mocked( + mockConfig.takePendingManualPlanExitNotice, + ).mockReturnValueOnce({ + version: 15, + currentMode: ApprovalMode.DEFAULT, + }); + chat.enableManualPlanExitNotices(); + vi.spyOn(ChatCompressionService.prototype, 'compress') + .mockResolvedValueOnce({ + newHistory: null, + info: { + originalTokenCount: 0, + newTokenCount: 0, + compressionStatus: CompressionStatus.NOOP, + }, + }) + .mockResolvedValueOnce({ + newHistory: compressedHistory, + info: { + originalTokenCount: 135_000, + newTokenCount: 40_000, + compressionStatus: CompressionStatus.COMPRESSED, + }, + }); + vi.mocked(mockContentGenerator.generateContentStream) + .mockRejectedValueOnce( + new Error('prompt is too long: 135000 tokens > 128000 maximum'), + ) + .mockImplementationOnce(async () => + streamResponse(stopResponse([{ text: 'after compression' }])), + ); + + const stream = await chat.sendMessageStream( + 'test-model', + { message: 'retry after overflow' }, + 'prompt-id-plan-exit-reactive-compression', + ); + for await (const _ of stream) { + /* consume */ + } + + const retryRequest = vi.mocked( + mockContentGenerator.generateContentStream, + ).mock.calls[1]![0] as { contents: Content[] }; + expect( + retryRequest.contents + .flatMap((content) => content.parts ?? []) + .filter((part) => + part.text?.includes( + 'changed outside the approved exit_plan_mode flow', + ), + ), + ).toHaveLength(1); + expect( + chat + .getHistory() + .flatMap((content) => content.parts ?? []) + .filter((part) => + part.text?.includes( + 'changed outside the approved exit_plan_mode flow', + ), + ), + ).toHaveLength(1); + const history = chat.getHistory(); + const noticeTurn = history.find((content) => + content.parts?.some((part) => + part.text?.includes( + 'changed outside the approved exit_plan_mode flow', + ), + ), + ); + expect(noticeTurn?.parts?.at(-1)?.text).toContain( + 'changed outside the approved exit_plan_mode flow', + ); + expect( + history.some( + (content, index) => + content.role === 'user' && history[index + 1]?.role === 'user', + ), + ).toBe(false); + expect( + mockConfig.takePendingManualPlanExitNotice, + ).toHaveBeenCalledTimes(1); + expect( + mockConfig.restorePendingManualPlanExitNotice, + ).not.toHaveBeenCalled(); + }, + ); + + it('does not redeliver after rebuilding a chat with the same cursor', async () => { + let pending = true; + vi.mocked( + mockConfig.takePendingManualPlanExitNotice, + ).mockImplementation(() => { + if (!pending) { + return undefined; + } + pending = false; + return { + version: 17, + currentMode: ApprovalMode.DEFAULT, + }; + }); + chat.enableManualPlanExitNotices(); + + const firstStream = await chat.sendMessageStream( + 'test-model', + { message: 'first chat' }, + 'prompt-id-plan-exit-before-rebuild', + ); + for await (const _ of firstStream) { + /* consume */ + } + + const replacementChat = new GeminiChat(mockConfig, config); + replacementChat.enableManualPlanExitNotices(); + const replacementStream = await replacementChat.sendMessageStream( + 'test-model', + { message: 'replacement chat' }, + 'prompt-id-plan-exit-after-rebuild', + ); + for await (const _ of replacementStream) { + /* consume */ + } + + expect( + replacementChat + .getHistory() + .flatMap((content) => content.parts ?? []) + .some((part) => + part.text?.includes( + 'changed outside the approved exit_plan_mode flow', + ), + ), + ).toBe(false); + expect( + mockConfig.takePendingManualPlanExitNotice, + ).toHaveBeenCalledTimes(2); + }); + }); + it('increments the user-content push counter once per surviving send', async () => { vi.mocked(mockContentGenerator.generateContentStream).mockResolvedValue( (async function* () { diff --git a/packages/core/src/core/geminiChat.ts b/packages/core/src/core/geminiChat.ts index 7a33a060bcb..a64ac2dcadb 100644 --- a/packages/core/src/core/geminiChat.ts +++ b/packages/core/src/core/geminiChat.ts @@ -100,7 +100,10 @@ import { isSystemReminderContent, } from '../utils/environmentContext.js'; import type { SessionStartSource } from '../hooks/types.js'; -import { getCustomSystemPrompt } from './prompts.js'; +import { + getCustomSystemPrompt, + getManualPlanExitSystemReminder, +} from './prompts.js'; import { RETRYABLE_STREAM_TRANSPORT_CODES } from './stream-transport-retry.js'; import { collectToolCallIdsFromHistory, @@ -1546,6 +1549,7 @@ export class GeminiChat { * can't, since compression shrinks history independently of the push. */ private userContentPushCount = 0; + private manualPlanExitNoticesEnabled = false; /** * Reset both partial-push markers in lockstep. Every history-mutation @@ -1594,6 +1598,10 @@ export class GeminiChat { validateHistory(history); } + enableManualPlanExitNotices(): void { + this.manualPlanExitNoticesEnabled = true; + } + /** * Most recent prompt-token count reported by the model for *this* chat, * mirroring the value in {@link UiTelemetryService} for the main session. @@ -1932,6 +1940,8 @@ export class GeminiChat { let compressionInfo: ChatCompressionInfo; let requestContents: Content[]; let userContentAdded = false; + let manualPlanExitNoticeVersion: number | undefined; + let manualPlanExitNoticeText: string | undefined; // Determine the ceiling for this turn's output request. The clamp below // (see clampOutputTokensToWindow) sizes the actual max_tokens to the room @@ -2156,6 +2166,25 @@ export class GeminiChat { }); } + if (this.manualPlanExitNoticesEnabled) { + const notice = this.config.takePendingManualPlanExitNotice(); + if (notice) { + manualPlanExitNoticeVersion = notice.version; + manualPlanExitNoticeText = getManualPlanExitSystemReminder( + notice.currentMode, + ); + userContent = { + ...userContent, + parts: [ + ...(userContent.parts ?? []), + { + text: manualPlanExitNoticeText, + }, + ], + }; + } + } + // Add user content to history ONCE before any attempts. this.history.push(userContent); currentUserContent = userContent; @@ -2244,6 +2273,11 @@ export class GeminiChat { // The push above was rolled back, so undo its count too. this.userContentPushCount--; } + if (manualPlanExitNoticeVersion !== undefined) { + this.config.restorePendingManualPlanExitNotice( + manualPlanExitNoticeVersion, + ); + } streamDoneResolver!(); throw error; } @@ -2518,6 +2552,29 @@ export class GeminiChat { // tryCompress stops resetting it. self.popPendingPartialAssistantTurn(); + // Reactive compression replaces the committed user turn. + // Keep its one-shot notice in the rebuilt retry request. + const noticeText = manualPlanExitNoticeText; + if ( + noticeText && + !self.history.some((content) => + content.parts?.some((part) => + part.text?.includes(noticeText), + ), + ) + ) { + const lastContent = self.history.at(-1); + if (lastContent?.role === 'user') { + lastContent.parts = [ + ...(lastContent.parts ?? []), + { text: noticeText }, + ]; + } else { + self.history.push( + createUserContent([{ text: noticeText }]), + ); + } + } requestContents = self.getRequestHistoryForRoute( currentUserContent, requestModalities, diff --git a/packages/core/src/core/prompts.test.ts b/packages/core/src/core/prompts.test.ts index b03c90822a7..33e120dc3ac 100644 --- a/packages/core/src/core/prompts.test.ts +++ b/packages/core/src/core/prompts.test.ts @@ -792,19 +792,19 @@ describe('getManualPlanExitSystemReminder', () => { it('should name the new mode and forbid exit_plan_mode', () => { const result = getManualPlanExitSystemReminder('default'); - expect(result).toMatch(/^[\s\S]*<\/system-reminder>$/); - expect(result).toContain('manually switched out of plan mode'); - expect(result).toContain('current approval mode: default'); - expect(result).toContain('Do NOT call exit_plan_mode'); - expect(result).toContain('no longer apply'); + expect(result).toBe(` +The approval mode changed outside the approved exit_plan_mode flow. +The current approval mode is: default. +Plan mode is no longer active. This notice supersedes any earlier reminder that Plan mode is active. Do not call exit_plan_mode; no plan approval is pending. Continue under the current mode's permissions and confirmation requirements. +`); }); it('should render whichever mode the user switched to', () => { expect(getManualPlanExitSystemReminder('yolo')).toContain( - 'current approval mode: yolo', + 'current approval mode is: yolo', ); expect(getManualPlanExitSystemReminder('auto-edit')).toContain( - 'current approval mode: auto-edit', + 'current approval mode is: auto-edit', ); }); }); diff --git a/packages/core/src/core/prompts.ts b/packages/core/src/core/prompts.ts index b7cfda86bbb..bd3b02c345a 100644 --- a/packages/core/src/core/prompts.ts +++ b/packages/core/src/core/prompts.ts @@ -1160,18 +1160,19 @@ Your plan is ready when you have addressed all ambiguities and it covers: what t } /** - * One-shot reminder injected on the first model-bound turn after the user - * manually exits plan mode (Shift+Tab, `/approval-mode`, `/plan`, ACP mode - * switch). While plan mode is active {@link getPlanModeSystemReminder} is - * re-injected every turn, so on a manual exit the model's most recent - * context still says "plan mode is active" — the reminder silently - * disappearing is not a signal models reliably notice (#7671). + * One-shot reminder injected on the first model-bound turn after Plan mode + * changes outside the approved `exit_plan_mode` flow. While Plan mode is + * active {@link getPlanModeSystemReminder} is re-injected every turn, so the + * reminder silently disappearing is not a signal models reliably notice + * (#7671). * - * @param currentMode - The approval mode the user switched to + * @param currentMode - The approval mode active when delivery is claimed */ export function getManualPlanExitSystemReminder(currentMode: string): string { return ` -The user has manually switched out of plan mode (current approval mode: ${currentMode}). You are no longer in plan mode. Do NOT call ${ToolNames.EXIT_PLAN_MODE} — there is no plan approval pending. Continue working in the current mode; previous plan-mode restrictions on edits and state-modifying tools no longer apply. +The approval mode changed outside the approved exit_plan_mode flow. +The current approval mode is: ${currentMode}. +Plan mode is no longer active. This notice supersedes any earlier reminder that Plan mode is active. Do not call exit_plan_mode; no plan approval is pending. Continue under the current mode's permissions and confirmation requirements. `; } diff --git a/packages/core/src/tools/agent/agent-override.test.ts b/packages/core/src/tools/agent/agent-override.test.ts index d9e54ad011b..9c884cbd642 100644 --- a/packages/core/src/tools/agent/agent-override.test.ts +++ b/packages/core/src/tools/agent/agent-override.test.ts @@ -70,6 +70,28 @@ describe('createApprovalModeOverride bound-tool isolation', () => { return { stripDangerousRulesForAutoMode, restoreDangerousRules }; } + it('copies the current plan-exit event before isolating approval mode', async () => { + const parent = await createParentWithRegistry(); + parent.setApprovalMode(ApprovalMode.PLAN); + parent.setApprovalMode(ApprovalMode.DEFAULT); + + const { config: child } = await createApprovalModeOverride( + parent, + ApprovalMode.AUTO_EDIT, + ); + + const childNotice = child.takePendingManualPlanExitNotice(); + const parentNotice = parent.takePendingManualPlanExitNotice(); + expect(childNotice?.version).toBe(parentNotice?.version); + expect(childNotice?.currentMode).toBe(ApprovalMode.AUTO_EDIT); + expect(parentNotice?.currentMode).toBe(ApprovalMode.DEFAULT); + + parent.setApprovalMode(ApprovalMode.PLAN); + parent.setApprovalMode(ApprovalMode.DEFAULT); + expect(child.takePendingManualPlanExitNotice()).toBeUndefined(); + expect(parent.takePendingManualPlanExitNotice()).toBeDefined(); + }); + it('returns a Config whose registry is a distinct instance from the parent', async () => { const parent = new Config(baseParams); const parentRegistry = await parent.createToolRegistry(undefined, { diff --git a/packages/core/src/tools/agent/agent.ts b/packages/core/src/tools/agent/agent.ts index 003a8ae4bb5..241dc9e7d31 100644 --- a/packages/core/src/tools/agent/agent.ts +++ b/packages/core/src/tools/agent/agent.ts @@ -665,6 +665,12 @@ export async function createApprovalModeOverride( // These own properties intentionally mirror Config's TS-private field names. // Config prototype methods read/write them at runtime on this override object. override.approvalMode = mode; + override.manualPlanExitNoticeEventState = { + ...(override.manualPlanExitNoticeEventState ?? { + version: 0, + kind: 'clear', + }), + }; override.getApprovalMode = Config.prototype.getApprovalMode; override.prePlanMode = mode === ApprovalMode.PLAN diff --git a/packages/core/src/utils/forkedAgent.cache.test.ts b/packages/core/src/utils/forkedAgent.cache.test.ts index 725a42b15f2..9cdbecca145 100644 --- a/packages/core/src/utils/forkedAgent.cache.test.ts +++ b/packages/core/src/utils/forkedAgent.cache.test.ts @@ -243,10 +243,12 @@ describe('runForkedAgent (cache path)', () => { }, ); + const enableManualPlanExitNotices = vi.fn(); vi.mocked(GeminiChat).mockImplementation( () => ({ sendMessageStream: mockSendMessageStream, + enableManualPlanExitNotices, }) as unknown as GeminiChat, ); @@ -275,6 +277,7 @@ describe('runForkedAgent (cache path)', () => { // to avoid polluting the main session's recordings expect(ctorArgs[3]).toBeUndefined(); // chatRecordingService expect(ctorArgs[4]).toBeUndefined(); // telemetryService + expect(enableManualPlanExitNotices).not.toHaveBeenCalled(); // Verify sendMessageStream was called expect(capturedParams).not.toBeNull();