diff --git a/.changeset/exit-plan-mode-auto-review.md b/.changeset/exit-plan-mode-auto-review.md new file mode 100644 index 000000000..0b4581da1 --- /dev/null +++ b/.changeset/exit-plan-mode-auto-review.md @@ -0,0 +1,5 @@ +--- +"@moonshot-ai/kimi-code": minor +--- + +Exit plan mode now requires reviewing the current plan, even in auto permission mode. Approve or revise the plan when prompted. diff --git a/packages/agent-core/src/agent/permission/policies/exit-plan-mode-review-ask.ts b/packages/agent-core/src/agent/permission/policies/exit-plan-mode-review-ask.ts index 7cd90eaea..9cc257bb4 100644 --- a/packages/agent-core/src/agent/permission/policies/exit-plan-mode-review-ask.ts +++ b/packages/agent-core/src/agent/permission/policies/exit-plan-mode-review-ask.ts @@ -19,7 +19,6 @@ export class ExitPlanModeReviewAskPermissionPolicy implements PermissionPolicy { evaluate(context: PermissionPolicyContext): PermissionPolicyResult | undefined { if (context.toolCall.name !== 'ExitPlanMode') return; - if (this.agent.permission.mode === 'auto') return; if (!this.agent.planMode.isActive) return; const display = context.execution.display; if (display?.kind !== 'plan_review') return; diff --git a/packages/agent-core/src/agent/permission/policies/index.ts b/packages/agent-core/src/agent/permission/policies/index.ts index a0ba9bdfe..3770f0d2a 100644 --- a/packages/agent-core/src/agent/permission/policies/index.ts +++ b/packages/agent-core/src/agent/permission/policies/index.ts @@ -37,6 +37,8 @@ export function createPermissionDecisionPolicies(agent: Agent): PermissionPolicy new PlanModeGuardDenyPermissionPolicy(agent), // User-configured deny rule matches → deny. new UserConfiguredDenyPermissionPolicy(agent), + // ExitPlanMode with active plan_review + non-empty plan → ask (tracks plan_submitted/plan_resolved itself). Runs before session history so a stale session approval can't bypass review of a new plan body and before auto mode permission policy so auto mode cannot bypass review of a new plan body. + new ExitPlanModeReviewAskPermissionPolicy(agent), // auto mode → approve (any auto-mode block must be a deny rule above this). new AutoModeApprovePermissionPolicy(agent), // Approve-for-session memorized rule matches → approve. Runs before user-configured ask rules so an in-session grant beats a still-matching ask rule on later calls. @@ -45,8 +47,6 @@ export function createPermissionDecisionPolicies(agent: Agent): PermissionPolicy new UserConfiguredAskPermissionPolicy(agent), // User-configured allow rule matches → approve. new UserConfiguredAllowPermissionPolicy(agent), - // ExitPlanMode with active plan_review + non-empty plan + non-auto → ask (tracks plan_submitted/plan_resolved itself). Runs before session history so a stale session approval can't bypass review of a new plan body. - new ExitPlanModeReviewAskPermissionPolicy(agent), // CreateGoal (non-auto) → ask with the same start menu as /goal: choose the // permission mode to run the goal under, or decline. Applies the mode, then // lets the tool create the goal. diff --git a/packages/agent-core/src/tools/builtin/planning/exit-plan-mode.ts b/packages/agent-core/src/tools/builtin/planning/exit-plan-mode.ts index 01c6364f4..b839d9465 100644 --- a/packages/agent-core/src/tools/builtin/planning/exit-plan-mode.ts +++ b/packages/agent-core/src/tools/builtin/planning/exit-plan-mode.ts @@ -136,7 +136,7 @@ export class ExitPlanModeTool implements BuiltinTool { const failed = this.exitPlanMode(); if (failed !== undefined) return failed; - this.agent.telemetry.track('plan_resolved', { outcome: 'auto_approved' }); + this.agent.telemetry.track('plan_resolved', { outcome: 'approved' }); return { isError: false, diff --git a/packages/agent-core/test/agent/permission.test.ts b/packages/agent-core/test/agent/permission.test.ts index 8d0b27712..18b0d5e3e 100644 --- a/packages/agent-core/test/agent/permission.test.ts +++ b/packages/agent-core/test/agent/permission.test.ts @@ -704,11 +704,11 @@ describe('Permission policy chain', () => { 'auto-mode-ask-user-question-deny', 'plan-mode-guard-deny', 'user-configured-deny', + 'exit-plan-mode-review-ask', 'auto-mode-approve', 'session-approval-history', 'user-configured-ask', 'user-configured-allow', - 'exit-plan-mode-review-ask', 'goal-start-review-ask', 'plan-mode-tool-approve', 'sensitive-file-access-ask', @@ -1842,18 +1842,31 @@ describe('ExitPlanMode permission policy', () => { { label: 'Approach B', description: 'Use the complete refactor.' }, ]; - it('allows ExitPlanMode directly in auto mode without requesting approval', async () => { - const { manager, requestApproval } = makePlanPermissionManager({ + it('requests plan-review approval in auto mode and returns formatted output', async () => { + const { manager, requestApproval, exit } = makePlanPermissionManager({ mode: 'auto', plan: '# Auto Plan', + path: '/tmp/plan.md', + approval: { decision: 'approved' }, }); const result = await manager.beforeToolCall( - hookContext({ id: 'call_exit', toolName: 'ExitPlanMode', args: {} }), + hookContext({ + id: 'call_exit', + toolName: 'ExitPlanMode', + args: {}, + execution: planReviewExecution({ plan: '# Auto Plan', path: '/tmp/plan.md' }), + }), ); - expect(result).toBeUndefined(); - expect(requestApproval).not.toHaveBeenCalled(); + expect(requestApproval).toHaveBeenCalledTimes(1); + expect(exit).toHaveBeenCalled(); + expect(result).toMatchObject({ + syntheticResult: { + isError: false, + output: expect.stringContaining('Exited plan mode.'), + }, + }); }); it('requests plan-review approval in yolo mode and returns formatted output', async () => { @@ -1910,7 +1923,7 @@ describe('ExitPlanMode permission policy', () => { }); }); - it('reuses session approval for ExitPlanMode without re-prompting plan review', async () => { + it('re-prompts plan review for a new plan body even when a session approval exists', async () => { const { manager, requestApproval, exit } = makePlanPermissionManager({ mode: 'manual', plan: '# Updated Plan', @@ -1934,9 +1947,14 @@ describe('ExitPlanMode permission policy', () => { }), ); - expect(requestApproval).not.toHaveBeenCalled(); - expect(exit).not.toHaveBeenCalled(); - expect(result).toBeUndefined(); + expect(requestApproval).toHaveBeenCalledTimes(1); + expect(exit).toHaveBeenCalled(); + expect(result).toMatchObject({ + syntheticResult: { + isError: false, + output: expect.stringContaining('Exited plan mode.'), + }, + }); }); it('returns a synthetic stop-turn result when the user rejects the plan', async () => { diff --git a/packages/agent-core/test/agent/plan.test.ts b/packages/agent-core/test/agent/plan.test.ts index 9996e7829..53e781245 100644 --- a/packages/agent-core/test/agent/plan.test.ts +++ b/packages/agent-core/test/agent/plan.test.ts @@ -128,7 +128,7 @@ describe('plan clear', () => { }); describe('plan exit tool', () => { - it('reads the current plan file and exits plan mode directly in auto mode', async () => { + it('requests plan review before exiting plan mode in auto mode', async () => { const files = new Map(); const readText = vi.fn(async (path: string) => files.get(path) ?? ''); const ctx = testAgent({ @@ -152,10 +152,10 @@ describe('plan exit tool', () => { ctx.mockNextResponse({ type: 'text', text: 'I can execute after approval.' }); await ctx.rpc.prompt({ input: [{ type: 'text', text: 'Show the plan' }] }); + const approval = await ctx.takeApprovalRequest(); + approval.respond({ decision: 'approved' }); + await ctx.untilTurnEnd(); - expect( - ctx.allEvents.some((event) => event.type === '[rpc]' && event.event === 'requestApproval'), - ).toBe(false); expect(readText).toHaveBeenCalledWith(planPath); expect(ctx.agent.planMode.isActive).toBe(false); const llmInput = ctx.llmCalls[1]!; diff --git a/packages/agent-core/test/tools/planning/exit-plan-mode-telemetry.test.ts b/packages/agent-core/test/tools/planning/exit-plan-mode-telemetry.test.ts index 2dea41d53..d002e63f6 100644 --- a/packages/agent-core/test/tools/planning/exit-plan-mode-telemetry.test.ts +++ b/packages/agent-core/test/tools/planning/exit-plan-mode-telemetry.test.ts @@ -123,7 +123,7 @@ function permissionContext(args: ExitPlanModeInput): PermissionPolicyContext { } describe('ExitPlanMode telemetry', () => { - it('tracks submitted without options and auto approval', async () => { + it('tracks submitted without options and review approval in auto mode', async () => { const { agent, telemetryTrack, exitPlanMode } = makeAgent({ mode: 'auto' }); const result = await execute(agent); @@ -132,7 +132,7 @@ describe('ExitPlanMode telemetry', () => { expect(exitPlanMode).toHaveBeenCalledTimes(1); expect(telemetryTrack).toHaveBeenCalledWith('plan_submitted', { has_options: false }); expect(telemetryTrack).toHaveBeenCalledWith('plan_resolved', { - outcome: 'auto_approved', + outcome: 'approved', }); });