Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions packages/core/src/config/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6460,6 +6460,51 @@ describe('setApprovalMode with folder trust', () => {
expect(config.getApprovalModeRevision()).toBe(initialRevision + 2);
});

it('queues a one-shot manual plan-exit notice on a manual exit', () => {
const config = new Config(baseParams);
vi.spyOn(config, 'isTrustedFolder').mockReturnValue(true);

config.setApprovalMode(ApprovalMode.PLAN);
config.setApprovalMode(ApprovalMode.DEFAULT);

expect(config.consumePendingManualPlanExitNotice()).toBe(true);
// One-shot: consumed on first read.
expect(config.consumePendingManualPlanExitNotice()).toBe(false);
});

it('does not queue the exit notice for an approved plan exit', () => {
const config = new Config(baseParams);
vi.spyOn(config, 'isTrustedFolder').mockReturnValue(true);

config.setApprovalMode(ApprovalMode.PLAN);
config.setApprovalMode(ApprovalMode.DEFAULT, {
fromApprovedPlanExit: true,
});

expect(config.consumePendingManualPlanExitNotice()).toBe(false);
});

it('clears a stale exit notice when plan mode is re-entered', () => {
const config = new Config(baseParams);
vi.spyOn(config, 'isTrustedFolder').mockReturnValue(true);

config.setApprovalMode(ApprovalMode.PLAN);
config.setApprovalMode(ApprovalMode.DEFAULT);
config.setApprovalMode(ApprovalMode.PLAN);

expect(config.consumePendingManualPlanExitNotice()).toBe(false);
});

it('does not queue the exit notice for non-plan mode changes', () => {
const config = new Config(baseParams);
vi.spyOn(config, 'isTrustedFolder').mockReturnValue(true);

config.setApprovalMode(ApprovalMode.AUTO_EDIT);
config.setApprovalMode(ApprovalMode.DEFAULT);

expect(config.consumePendingManualPlanExitNotice()).toBe(false);
});

it('records prePlanMode=yolo for a Shift+Tab cycle into plan mode', () => {
const config = new Config(baseParams);
vi.spyOn(config, 'isTrustedFolder').mockReturnValue(true);
Expand Down
36 changes: 33 additions & 3 deletions packages/core/src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1761,6 +1761,7 @@ export class Config {
private approvalMode: ApprovalMode;
private prePlanMode?: ApprovalMode;
private approvalModeRevision = 0;
private pendingManualPlanExitNotice = false;
private autoModeDenialState: AutoModeDenialState = createDenialState();
private readonly accessibility: AccessibilitySettings;
private readonly showResponseTokensPerSecond: boolean;
Expand Down Expand Up @@ -5403,10 +5404,18 @@ export class Config {

setApprovalMode(
mode: ApprovalMode,
/** @deprecated Model origin no longer changes plan-exit approval. */
options?: { enteredByModel?: boolean },
options?: {
/** @deprecated Model origin no longer changes plan-exit approval. */
enteredByModel?: boolean;
/**
* Set by ExitPlanModeTool for user/leader-approved plan exits. Every
* other PLAN → non-PLAN transition (Shift+Tab, /approval-mode, /plan,
* ACP setSessionMode, confirm-and-switch) is a manual exit the model
* was never told about, and queues a one-shot system reminder.
*/
fromApprovedPlanExit?: boolean;
},
): void {
void options;
if (
!this.isTrustedFolder() &&
mode !== ApprovalMode.DEFAULT &&
Expand Down Expand Up @@ -5435,8 +5444,18 @@ export class Config {
// succeeded, so callers never observe a partially applied mode change.
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;
} 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;
}
}
// Any deliberate mode change invalidates the AUTO denialTracking signal.
if (fromMode !== mode) {
Expand All @@ -5448,6 +5467,17 @@ 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.
*/
consumePendingManualPlanExitNotice(): boolean {
const pending = this.pendingManualPlanExitNotice;
this.pendingManualPlanExitNotice = false;
return pending;
}

/**
* Returns the directory where this session's plan file is stored.
*/
Expand Down
69 changes: 69 additions & 0 deletions packages/core/src/core/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,7 @@ 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),
getSdkMode: vi.fn().mockReturnValue(false),
getExperimentalZedIntegration: vi.fn().mockReturnValue(false),
isInteractive: vi.fn().mockReturnValue(false),
Expand Down Expand Up @@ -5964,6 +5965,74 @@ 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);
Expand Down
10 changes: 10 additions & 0 deletions packages/core/src/core/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import {
getArenaSystemReminder,
getCoreSystemPrompt,
getCustomSystemPrompt,
getManualPlanExitSystemReminder,
getPlanModeSystemReminder,
resolveInteractionMode,
} from './prompts.js';
Expand Down Expand Up @@ -2452,6 +2453,15 @@ 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()),
);
Comment on lines +2462 to +2464

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The exit notice is only consumed on UserQuery/Cron turns (line 2425 gates the whole system-reminder block). If the user Shift+Tabs out of plan mode mid-tool-loop, the next model-bound turn is a ToolResult — the flag stays pending and no exit reminder is injected. — Concrete cost: the model's most recent system reminder still says "plan mode is active"; it may call exit_plan_mode and burn a round-trip before the next user message delivers the reminder. The staleness guard in exitPlanMode.ts prevents harm, but the one wasted turn is the exact scenario this feature targets.

中文说明

退出通知仅在 UserQuery/Cron 轮次消费(第 2425 行的门控覆盖了整个 system-reminder 块)。如果用户在工具循环中途按 Shift+Tab 退出 plan mode,下一个发给模型的轮次是 ToolResult——标志保持 pending 状态,不会注入退出提醒。

具体代价: 模型最近的 system reminder 仍显示"plan mode 激活中";它可能调用 exit_plan_mode,浪费一轮交互后才能在下一条用户消息中获得提醒。exitPlanMode.ts 中的过期检查防止了实际损害,但这一轮浪费正是此功能要解决的问题。

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real bound, deliberately kept: the notice rides the same UserQuery/Cron cadence as the plan-mode reminder it replaces — no reminder in this assembly is injected on ToolResult turns today, and giving this one reminder its own injection point outside that gate would be a new pattern for the block (with its own questions about interleaving system reminders into an active tool loop). The mid-loop window is bounded: the flag stays pending (it is only cleared by consumption or plan re-entry, never dropped), so the reminder lands on the very next user/cron turn, and a premature exit_plan_mode call in that window is caught by the staleness/permission guard — with #7673 it additionally returns explicit guidance instead of a generic deny. If maintainers want reminders on ToolResult turns, happy to do that as a follow-up gate change rather than special-casing this one reminder.

中文

真实边界,且是有意保留的:通知与它所替代的 plan-mode reminder 走同一个 UserQuery/Cron 注入节奏——该装配块今天没有任何 reminder 在 ToolResult 轮注入,为这一个 reminder 单开注入点会给这个块引入新模式(还牵出「向进行中的工具循环插入 system reminder」的问题)。循环中途的窗口是有界的:标志保持 pending(只被消费或重进 plan mode 清除,绝不丢弃),下一个 user/cron 轮必然送达;窗口内过早的 exit_plan_mode 调用被过期/权限守卫拦住——配合 #7673 还会返回明确指引而非泛化拒绝。如果维护者希望 ToolResult 轮也注入 reminder,可以作为 follow-up 改门控整体处理,而不是给单个 reminder 开特例。

}

// add arena system reminder if an arena session is active
Expand Down
22 changes: 22 additions & 0 deletions packages/core/src/core/prompts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest';
import {
getCoreSystemPrompt,
getCustomSystemPrompt,
getManualPlanExitSystemReminder,
getPlanModeSystemReminder,
resolvePathFromEnv,
getCompressionPrompt,
Expand Down Expand Up @@ -786,6 +787,27 @@ describe('getPlanModeSystemReminder', () => {
});
});

describe('getManualPlanExitSystemReminder', () => {
it('should name the new mode and forbid exit_plan_mode', () => {
const result = getManualPlanExitSystemReminder('default');

expect(result).toMatch(/^<system-reminder>[\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');
});

it('should render whichever mode the user switched to', () => {
expect(getManualPlanExitSystemReminder('yolo')).toContain(
'current approval mode: yolo',
);
expect(getManualPlanExitSystemReminder('auto-edit')).toContain(
'current approval mode: auto-edit',
);
});
});

describe('resolvePathFromEnv helper function', () => {
beforeEach(() => {
vi.resetAllMocks();
Expand Down
16 changes: 16 additions & 0 deletions packages/core/src/core/prompts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1098,6 +1098,22 @@ Your plan is ready when you have addressed all ambiguities and it covers: what t
</system-reminder>`;
}

/**
* 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).
*
* @param currentMode - The approval mode the user switched to
*/
export function getManualPlanExitSystemReminder(currentMode: string): string {
return `<system-reminder>
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.
</system-reminder>`;
Comment on lines +1111 to +1114

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] No unit test in prompts.test.ts for getManualPlanExitSystemReminder, while its counterpart getPlanModeSystemReminder has 4 dedicated tests. — Concrete cost: the client tests only assert stringContaining('manually switched out of plan mode') — they would pass even if currentMode renders as undefined or EXIT_PLAN_MODE is replaced with a wrong tool name.

Add a describe('getManualPlanExitSystemReminder', ...) block that verifies: (1) output contains the supplied mode string, (2) output contains the exit_plan_mode tool name, (3) output matches the <system-reminder>...</system-reminder> wrapper.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 18bb51b — a getManualPlanExitSystemReminder describe in prompts.test.ts: pins the <system-reminder> envelope, the "manually switched out of plan mode" phrasing, the rendered mode name (current approval mode: default / yolo / auto-edit), the Do NOT call exit_plan_mode prohibition, and the restrictions-lifted line — so an undefined mode or a renamed tool constant now fails here rather than slipping past the client tests' stringContaining check. 中文:已补 prompts 单测(18bb51ba2),钉住信封、模式名渲染与 exit_plan_mode 禁令,undefined 模式或工具名改动会在此失败。

}

/**
* Generates a system reminder about an active Arena session.
*
Expand Down
33 changes: 32 additions & 1 deletion packages/core/src/tools/exitPlanMode.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,9 @@ describe('ExitPlanModeTool', () => {

expect(result.error).toBeUndefined();
expect(approvalMode).toBe(targetMode);
expect(config.setApprovalMode).toHaveBeenCalledWith(targetMode);
expect(config.setApprovalMode).toHaveBeenCalledWith(targetMode, {
fromApprovedPlanExit: true,
});
expect(config.savePlan).toHaveBeenCalledWith('Approved plan');
},
);
Expand Down Expand Up @@ -371,6 +373,35 @@ describe('ExitPlanModeTool', () => {
expect(config.setApprovalMode).not.toHaveBeenCalled();
});

it('marks a successful leader-approved exit as an approved plan exit', async () => {
vi.mocked(config.getTeamManager).mockReturnValue({
requestPlanApproval: vi.fn(async () => ({
action: 'approve',
targetMode: ApprovalMode.DEFAULT,
})),
} as never);
const invocation = tool.build({ plan: 'Teammate plan' });

const result = await runWithTeammateIdentity(
{
agentId: 'planner@test',
agentName: 'planner',
teamName: 'test',
isTeamLead: false,
planModeRequired: true,
},
() => invocation.execute(new AbortController().signal),
);

expect(result.error).toBeUndefined();
expect(approvalMode).toBe(ApprovalMode.DEFAULT);
// Must mirror the regular approval call site: without the option, a
// leader-approved exit would queue the manual plan-exit reminder.
expect(config.setApprovalMode).toHaveBeenCalledWith(ApprovalMode.DEFAULT, {
fromApprovedPlanExit: true,
});
});

it('saves a leader-approved plan when the teammate transition fails', async () => {
transitionError = new Error('mode locked');
vi.mocked(config.getTeamManager).mockReturnValue({
Expand Down
8 changes: 6 additions & 2 deletions packages/core/src/tools/exitPlanMode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,9 @@ class ExitPlanModeToolInvocation extends BaseToolInvocation<

this.savePlanBestEffort(snapshot.plan);
try {
this.config.setApprovalMode(targetMode);
this.config.setApprovalMode(targetMode, {
fromApprovedPlanExit: true,
});
} catch (error) {
const message = error instanceof Error ? error.message : String(error);
debugLogger.error(
Expand Down Expand Up @@ -331,7 +333,9 @@ class ExitPlanModeToolInvocation extends BaseToolInvocation<

this.savePlanBestEffort(plan);
try {
this.config.setApprovalMode(decision.targetMode);
this.config.setApprovalMode(decision.targetMode, {
fromApprovedPlanExit: true,
});
Comment on lines +336 to +338

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The leader-approval setApprovalMode call site has no test verifying { fromApprovedPlanExit: true } is passed — only the regular approval call site (line 237) has this assertion. — Failure scenario: if a future change drops or renames the option on the leader path but not the regular path, no test fails. After a leader-approved plan exit, the model would receive a spurious "The user has manually switched out of plan mode" reminder, contradicting the approved-exit semantics.

Add a happy-path test for leader approval that asserts config.setApprovalMode was called with { fromApprovedPlanExit: true }, mirroring the assertion at line 118.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 18bb51b — new test marks a successful leader-approved exit as an approved plan exit: a leader approval resolving { action: 'approve', targetMode: DEFAULT } asserts setApprovalMode(DEFAULT, { fromApprovedPlanExit: true }), so dropping the option on the leader path now fails a test instead of queuing a spurious manual-exit reminder. 中文:已补 leader 批准成功路径的断言(18bb51ba2),漏传选项会直接挂测试。

} catch (error) {
const message = error instanceof Error ? error.message : String(error);
return this.errorResult(
Expand Down
Loading