fix(plan-mode): end the turn when the user dismisses a plan review
Choosing "Request changes" on a plan review rejected the question, and
exit_plan_mode turned that into a failed tool result asking the model to
stop. The loop then spent one more model request only to have the model
wait. A dismissed review now returns { approved: false } and concludes the
turn, so the user's next message starts the following request.
This commit is contained in:
1 parent
d16abc2745
commit
18a4bc4093
19 files changed
+93
-52
No files matched your search
@@ -7,12 +7,14 @@
|
||||
// Replay is deterministic: the plan content arrives from replayed chunks, the
|
||||
// review wait is real, and the approve click is the test's own gesture (the
|
||||
// turn cannot complete without it, in record and replay alike).
|
||||
import { readFile } from 'node:fs/promises'
|
||||
import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'
|
||||
import { tmpdir } from 'node:os'
|
||||
import { fileURLToPath } from 'node:url'
|
||||
import { join } from 'node:path'
|
||||
import type { Browser, ConsoleMessage, Page } from 'playwright'
|
||||
import { chromium } from 'playwright'
|
||||
import { afterAll, beforeAll, describe, expect, it, onTestFailed } from 'vitest'
|
||||
import { deriveReplayScript, parseSessionLog } from '@deepseek-ai/dsh-llm-replay'
|
||||
import type { SessionEvent } from '@deepseek-ai/dsh-session'
|
||||
import type { SessionId } from '@deepseek-ai/dsh-session/types'
|
||||
import {
|
||||
@@ -390,8 +392,12 @@ describe.skipIf(MODE === 'record')('web e2e: pending plan review across Sidebar
|
||||
|
||||
describe('web e2e: dismissed plan history', () => {
|
||||
it.skipIf(MODE === 'record')('reopens the permanent plan card after dismissing its review', async () => {
|
||||
// Replay supplies a finite continuation; the real question rejection and retained document are the assertions.
|
||||
const scaffold = await launchWebScaffold({ replayFixture: FIXTURE, compareReplaySession: false, paceMs: 15 })
|
||||
// A dismissed review concludes the turn, so only the call presenting the plan replays.
|
||||
const replayRoot = await mkdtemp(join(tmpdir(), 'dsh-plan-review-dismiss-replay-'))
|
||||
const replayOverride = join(replayRoot, 'replay.override.json')
|
||||
const script = deriveReplayScript(parseSessionLog(await readFile(FIXTURE, 'utf8')))
|
||||
await writeFile(replayOverride, JSON.stringify(script.slice(0, 1)))
|
||||
const scaffold = await launchWebScaffold({ replayFixture: FIXTURE, replayOverride, compareReplaySession: false, paceMs: 15 })
|
||||
let browser: Browser | undefined
|
||||
const events: SessionEvent[] = []
|
||||
scaffold.ctx.on('session/event', (_session, event: SessionEvent) => { events.push(event) })
|
||||
@@ -417,9 +423,10 @@ describe('web e2e: dismissed plan history', () => {
|
||||
expect(call).toBeDefined()
|
||||
const results = events.filter(event => event.type === 'tool/result')
|
||||
const result = results.find(event => event.data.message.source.callId === call?.data.callId)
|
||||
expect(result?.data.message).toMatchObject({ role: 'tool', toolCallId: call?.data.callId, isError: true })
|
||||
expect(result?.data.message).toMatchObject({ role: 'tool', toolCallId: call?.data.callId, isError: false })
|
||||
expect(JSON.stringify(result)).toContain('dismissed the plan review')
|
||||
expect(results.some(event => JSON.stringify(event).includes('Plan approved'))).toBe(false)
|
||||
expect(events.filter(event => event.type === 'assistant/message')).toHaveLength(1)
|
||||
const modes = events.filter(event => event.type === 'plan/mode')
|
||||
expect(modes).toHaveLength(1)
|
||||
expect(modes[0]).toMatchObject({ data: { active: true } })
|
||||
@@ -443,6 +450,7 @@ describe('web e2e: dismissed plan history', () => {
|
||||
} finally {
|
||||
await browser?.close()
|
||||
await scaffold.close()
|
||||
await rm(replayRoot, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
})
|
||||
@@ -90,8 +90,8 @@
|
||||
en: 9622e96bdc01276d
|
||||
zh: 53801e68a49f3d1d
|
||||
/deepseek-ai-dsh-plan-mode/model-experience/exit-tool-schema-and-review-exchange/what-the-model-sees:
|
||||
en: 3768d0189f9f13af
|
||||
zh: 2168a9b24551b7c6
|
||||
en: 16c57e8e64ad5a85
|
||||
zh: d81932553025c875
|
||||
/deepseek-ai-dsh-plan-mode/model-experience/exit-tool-schema-and-review-exchange/token-effect:
|
||||
en: d1b1418f20ee119d
|
||||
zh: c2b89ea3cae57175
|
||||
|
||||
@@ -164,7 +164,7 @@ The user block is append-only conversation growth. Entering or leaving plan mode
|
||||
|
||||
#### What the model sees
|
||||
|
||||
The [`exit_plan_mode` schema](../../../docs/tool-catalog.md#deepseek-aidsh-plan-mode) remains available in both states; execution outside plan mode fails, while an approved in-mode review returns the canonical `{ approved: true }` value and renders the existing confirmation text. Rejection remains a failed call carrying review feedback, and a dismissed review a failed call naming the user's takeover.
|
||||
The [`exit_plan_mode` schema](../../../docs/tool-catalog.md#deepseek-aidsh-plan-mode) remains available in both states; execution outside plan mode fails, while an approved in-mode review returns the canonical `{ approved: true }` value and renders the existing confirmation text. Rejection remains a failed call carrying review feedback. A dismissed review returns `{ approved: false }`, renders a note that the user will reply, and concludes the turn without another model request.
|
||||
|
||||
#### Token effect
|
||||
|
||||
|
||||
@@ -164,7 +164,7 @@ You are in plan mode. Explore and design before presenting the complete plan thr
|
||||
|
||||
#### 模型看到什么
|
||||
|
||||
[`exit_plan_mode` schema](../../../docs/tool-catalog.zh.md#deepseek-aidsh-plan-mode) 在两种状态下均可用;在计划模式之外执行会失败,而计划模式内经批准的评审返回规范的 `{ approved: true }` 值,并渲染既有的确认文本。拒绝仍是携带评审反馈的失败调用,放弃评审则是一次指明用户接手的失败调用。
|
||||
[`exit_plan_mode` schema](../../../docs/tool-catalog.zh.md#deepseek-aidsh-plan-mode) 在两种状态下均可用;在计划模式之外执行会失败,而计划模式内经批准的评审返回规范的 `{ approved: true }` 值,并渲染既有的确认文本。拒绝仍是携带评审反馈的失败调用。放弃评审返回 `{ approved: false }`,渲染一条说明用户将自行回复的文本,并结束当前轮次,不再发起模型请求。
|
||||
|
||||
#### Token 影响
|
||||
|
||||
|
||||
@@ -30,7 +30,7 @@ import { createUserMessage } from '@deepseek-ai/dsh-llm'
|
||||
import type { ContextFormed } from '@deepseek-ai/dsh-llm'
|
||||
import type { Session, UserMessage } from '@deepseek-ai/dsh-session'
|
||||
import { defineTool } from '@deepseek-ai/dsh-tools'
|
||||
import { UserQuestionError } from '@deepseek-ai/dsh-user-questions'
|
||||
import { UserQuestionError, type AskUserQuestionAnswer } from '@deepseek-ai/dsh-user-questions'
|
||||
import type { CommandDefinitionId, CommandId } from '@deepseek-ai/dsh-commands'
|
||||
import type {} from '@deepseek-ai/dsh-session-projection'
|
||||
import type { ProjectionDefinition } from '@deepseek-ai/dsh-session-projection'
|
||||
@@ -285,10 +285,15 @@ export class PlanModeController extends Service {
|
||||
type: 'object',
|
||||
additionalProperties: false,
|
||||
properties: {
|
||||
approved: { type: 'boolean', const: true, required: true },
|
||||
approved: { type: 'boolean', required: true },
|
||||
},
|
||||
},
|
||||
render: () => [{ type: 'text', text: 'Plan approved — plan mode exited; carry out the plan starting with your next step.' }],
|
||||
render: (_args, { approved }) => [{
|
||||
type: 'text',
|
||||
text: approved
|
||||
? 'Plan approved — plan mode exited; carry out the plan starting with your next step.'
|
||||
: 'The user dismissed the plan review to reply in their own words; plan mode remains active.',
|
||||
}],
|
||||
},
|
||||
execute: async (args, exec) => {
|
||||
const agent = exec.agent
|
||||
@@ -303,35 +308,37 @@ export class PlanModeController extends Service {
|
||||
if (interaction === undefined) {
|
||||
throw new Error('no user-questions channel is available to review the plan; ask the user to switch the session mode instead')
|
||||
}
|
||||
const answer = await interaction.ask({
|
||||
questions: [{
|
||||
id: REVIEW_ID,
|
||||
header: 'Plan review',
|
||||
question: 'Approve this plan and leave plan mode?',
|
||||
detail: args.plan,
|
||||
options: [
|
||||
{ label: APPROVE_LABEL, description: 'Leave plan mode; the plan is carried out from the next step.' },
|
||||
{ label: KEEP_PLANNING_LABEL, description: 'Stay in plan mode; feedback goes back to the model.' },
|
||||
],
|
||||
// Presentation only: a capable UI renders the plan as a review
|
||||
// decision instead of a generic question, and answers with one of
|
||||
// the labels above either way.
|
||||
intent: { kind: 'plan-review', approve: APPROVE_LABEL, callId: exec.callId },
|
||||
}],
|
||||
agent,
|
||||
signal: exec.signal,
|
||||
}).catch((cause: unknown) => {
|
||||
let answer: AskUserQuestionAnswer
|
||||
try {
|
||||
answer = await interaction.ask({
|
||||
questions: [{
|
||||
id: REVIEW_ID,
|
||||
header: 'Plan review',
|
||||
question: 'Approve this plan and leave plan mode?',
|
||||
detail: args.plan,
|
||||
options: [
|
||||
{ label: APPROVE_LABEL, description: 'Leave plan mode; the plan is carried out from the next step.' },
|
||||
{ label: KEEP_PLANNING_LABEL, description: 'Stay in plan mode; feedback goes back to the model.' },
|
||||
],
|
||||
// Presentation only: a capable UI renders the plan as a review
|
||||
// decision instead of a generic question, and answers with one of
|
||||
// the labels above either way.
|
||||
intent: { kind: 'plan-review', approve: APPROVE_LABEL, callId: exec.callId },
|
||||
}],
|
||||
agent,
|
||||
signal: exec.signal,
|
||||
})
|
||||
} catch (cause: unknown) {
|
||||
// A dismissed review is not a failed one: the user took the turn back
|
||||
// to say something the two options do not cover. Say so, because the
|
||||
// generic channel message names ask_user_question, which the model
|
||||
// never called. An abort (turn cancel, provider teardown) keeps its
|
||||
// own message — there is no user to wait for.
|
||||
// to say something the two options do not cover, so the turn ends
|
||||
// here instead of spending a model request on waiting. An abort (turn
|
||||
// cancel, provider teardown) keeps its own failure.
|
||||
if (cause instanceof UserQuestionError && cause.code === 'ASK_CANCELLED') {
|
||||
throw new Error('The user dismissed the plan review to speak instead; '
|
||||
+ 'stay in plan mode, stop here, and wait for their message.')
|
||||
exec.concludeTurn()
|
||||
return { approved: false }
|
||||
}
|
||||
throw cause
|
||||
})
|
||||
}
|
||||
// A review may outlive this plugin fiber. Without its pre-step listener,
|
||||
// an approved selection could never be appended, so fail and keep planning.
|
||||
if (disposed) {
|
||||
|
||||
@@ -8,6 +8,7 @@ import ToolRuntime, { defineContentToolFixture } from '@deepseek-ai/dsh-tools'
|
||||
import AgentRegistry, { type Agent } from '@deepseek-ai/dsh-agent'
|
||||
import AgentLoop from '@deepseek-ai/dsh-agent-loop'
|
||||
import PlanModeController from '@deepseek-ai/dsh-plan-mode'
|
||||
import UserQuestionService, { UserQuestionError } from '@deepseek-ai/dsh-user-questions'
|
||||
import { MockAdapter, textResponse, toolCallResponse } from '../../../core/agent-loop/tests/mock-adapter.ts'
|
||||
|
||||
const PLAN_CONFIG = { section: 'Test plan mode instructions.' }
|
||||
@@ -223,4 +224,28 @@ describe('plan mode through the agent loop', () => {
|
||||
{ type: 'text', text: 'The user switched this session to plan mode.' },
|
||||
])
|
||||
})
|
||||
|
||||
it('a dismissed plan review ends the turn without another model request', async () => {
|
||||
const adapter = new MockAdapter([
|
||||
toolCallResponse('call-exit', 'exit_plan_mode', { plan: '# Plan\n\ndo things' }),
|
||||
textResponse('Waiting for your message.'),
|
||||
])
|
||||
const ctx = await harness(adapter)
|
||||
await ctx.plugin(UserQuestionService)
|
||||
ctx.on('user-questions/request', () => Promise.reject(new UserQuestionError('the user cancelled ask_user_question', 'ASK_CANCELLED')))
|
||||
const agent = await ctx.agentLoop.create(SessionId('it-plan-dismiss'), { provider: 'mock', model: 'mock' })
|
||||
ctx.planMode.set(agent, true)
|
||||
|
||||
const idle = waitForIdle(ctx, agent)
|
||||
agent.followup(createUserMessage({ content: [{ type: 'text', text: 'plan it' }], source: { kind: 'user' } }))
|
||||
await idle
|
||||
|
||||
expect(adapter.requests).toHaveLength(1)
|
||||
const log = agent.session.snapshotEvents()
|
||||
const result = findEvent(log, 'tool/result')
|
||||
expect(result.data.message.isError).toBe(false)
|
||||
expect(result.data.message.content).toEqual([{ type: 'text', text: 'The user dismissed the plan review to reply in their own words; plan mode remains active.' }])
|
||||
expect(findEvent(log, 'turn/end').data.reason).toEqual({ kind: 'completed' })
|
||||
expect(planActive(ctx, agent)).toBe(true)
|
||||
})
|
||||
})
|
||||
@@ -165,7 +165,7 @@ function expectPlanPtcSdkBindings(sdk: string): void {
|
||||
expect(sdk).toContain('read: Record<string, JsonValue>;')
|
||||
expect(sdk).toContain('write: Record<string, JsonValue>;')
|
||||
expect(sdk).toContain('interface ToolOutputMap {')
|
||||
expect(sdk).toContain('exit_plan_mode: {\n approved: true;\n };')
|
||||
expect(sdk).toContain('exit_plan_mode: {\n approved: boolean;\n };')
|
||||
expect(sdk).toContain('[K in ToolName]: (args: ToolArgsMap[K]) => Promise<ToolOutputMap[K]>;')
|
||||
}
|
||||
|
||||
@@ -1058,7 +1058,7 @@ describe('exit_plan_mode', () => {
|
||||
expect(question?.options?.map(option => option.label)).toContain(question?.intent?.approve)
|
||||
})
|
||||
|
||||
it('reads a dismissed review as the user taking the turn back, not as a failure', async () => {
|
||||
it('reads a dismissed review as the user taking the turn back: a turn-concluding success, not a failure', async () => {
|
||||
const { ctx, agent } = await setupWithReview()
|
||||
registerQuestionAnswerer(ctx, {
|
||||
ask: () => Promise.reject(Object.assign(
|
||||
@@ -1067,8 +1067,9 @@ describe('exit_plan_mode', () => {
|
||||
)),
|
||||
})
|
||||
const result = await callExit(ctx, agent)
|
||||
expect(result.isError).toBe(true)
|
||||
expect(result.content).toEqual([{ type: 'text', text: 'Error: The user dismissed the plan review to speak instead; stay in plan mode, stop here, and wait for their message.' }])
|
||||
expect(result.isError).toBe(false)
|
||||
expect(result.concludesTurn).toBe(true)
|
||||
expect(result.content).toEqual([{ type: 'text', text: 'The user dismissed the plan review to reply in their own words; plan mode remains active.' }])
|
||||
expect(foldPlanMode(agent.session.snapshotEvents())).toBe(true)
|
||||
})
|
||||
|
||||
|
||||
@@ -360,7 +360,7 @@ interface ToolOutputMap {
|
||||
after: string;
|
||||
};
|
||||
exit_plan_mode: {
|
||||
approved: true;
|
||||
approved: boolean;
|
||||
};
|
||||
get_goal: {
|
||||
goal: null;
|
||||
|
||||
@@ -375,7 +375,7 @@ interface ToolOutputMap {
|
||||
after: string;
|
||||
};
|
||||
exit_plan_mode: {
|
||||
approved: true;
|
||||
approved: boolean;
|
||||
};
|
||||
get_goal: {
|
||||
goal: null;
|
||||
|
||||
@@ -327,7 +327,7 @@ interface ToolOutputMap {
|
||||
after: string;
|
||||
};
|
||||
exit_plan_mode: {
|
||||
approved: true;
|
||||
approved: boolean;
|
||||
};
|
||||
get_goal: {
|
||||
goal: null;
|
||||
|
||||
@@ -356,7 +356,7 @@ interface ToolOutputMap {
|
||||
after: string;
|
||||
};
|
||||
exit_plan_mode: {
|
||||
approved: true;
|
||||
approved: boolean;
|
||||
};
|
||||
get_goal: {
|
||||
goal: null;
|
||||
|
||||
@@ -331,7 +331,7 @@ interface ToolOutputMap {
|
||||
after: string;
|
||||
};
|
||||
exit_plan_mode: {
|
||||
approved: true;
|
||||
approved: boolean;
|
||||
};
|
||||
get_goal: {
|
||||
goal: null;
|
||||
|
||||
@@ -331,7 +331,7 @@ interface ToolOutputMap {
|
||||
after: string;
|
||||
};
|
||||
exit_plan_mode: {
|
||||
approved: true;
|
||||
approved: boolean;
|
||||
};
|
||||
get_goal: {
|
||||
goal: null;
|
||||
|
||||
@@ -158,7 +158,7 @@ class ExitPlanModeArgs(TypedDict):
|
||||
# Additional keys beyond those declared are allowed.
|
||||
|
||||
class ExitPlanModeOutput(TypedDict):
|
||||
approved: Literal[True]
|
||||
approved: bool
|
||||
|
||||
class GetGoalOutput1(TypedDict):
|
||||
goal: None
|
||||
|
||||
@@ -327,7 +327,7 @@ interface ToolOutputMap {
|
||||
after: string;
|
||||
};
|
||||
exit_plan_mode: {
|
||||
approved: true;
|
||||
approved: boolean;
|
||||
};
|
||||
get_goal: {
|
||||
goal: null;
|
||||
|
||||
@@ -331,7 +331,7 @@ interface ToolOutputMap {
|
||||
after: string;
|
||||
};
|
||||
exit_plan_mode: {
|
||||
approved: true;
|
||||
approved: boolean;
|
||||
};
|
||||
get_goal: {
|
||||
goal: null;
|
||||
|
||||
@@ -429,7 +429,7 @@ interface ToolOutputMap {
|
||||
after: string;
|
||||
};
|
||||
exit_plan_mode: {
|
||||
approved: true;
|
||||
approved: boolean;
|
||||
};
|
||||
get_goal: {
|
||||
goal: null;
|
||||
|
||||
@@ -437,7 +437,7 @@ interface ToolOutputMap {
|
||||
after: string;
|
||||
};
|
||||
exit_plan_mode: {
|
||||
approved: true;
|
||||
approved: boolean;
|
||||
};
|
||||
get_goal: {
|
||||
goal: null;
|
||||
|
||||
@@ -420,7 +420,7 @@ interface ToolOutputMap {
|
||||
after: string;
|
||||
};
|
||||
exit_plan_mode: {
|
||||
approved: true;
|
||||
approved: boolean;
|
||||
};
|
||||
get_goal: {
|
||||
goal: null;
|
||||
|
||||
Reference in new issue
Block a user