diff --git a/docs/users/features/goals.md b/docs/users/features/goals.md index c5f3a7dd767..475a74e4ac8 100644 --- a/docs/users/features/goals.md +++ b/docs/users/features/goals.md @@ -73,13 +73,13 @@ For an audit, completion means covering the agreed scenarios and reporting evide If a success criterion, command, input path, or essential decision cannot be established, the skill returns a draft marked "Needs clarification" with `` items. It does not offer that draft for approval or print a runnable `/goal set` or `/goal edit` command. Nonessential defaults are marked `[ASSUMPTION]`; they do not stand in for missing success criteria. -Once the objective is ready, an interactive terminal session can show the `propose_goal` approval dialog described below. Web Shell and other ACP clients, headless runs, sessions with the tool disabled, and sessions with an active Goal receive a command to run manually instead. The hand-off says that the draft has not been applied. The skill never starts the work itself, and nothing is set without your approval. +Once the objective is ready, an interactive terminal or Web Shell session can show the `propose_goal` approval dialog described below. Clients without Goal proposal support, headless runs, sessions with the tool disabled, and sessions with an active Goal receive a command to run manually instead. The hand-off says that the draft has not been applied. The skill never starts the work itself, and nothing is set without your approval. Pass an existing objective to tighten it: `/goal-draft all tests pass and the lint is clean`. For an active Goal, an explicit request to tighten it produces `/goal edit`; a replacement uses `/goal set`. If the intended operation is unclear, the skill includes that choice in its single round of questions. ### Approve a Goal the model proposes -In an interactive terminal session the model has a `propose_goal` tool. When `/goal-draft` finishes, or when you ask for an outcome that spans several turns, it can propose the objective instead of printing a `/goal set …` line for you to copy. The proposal appears as an approval dialog showing the full objective. Approving it sets the Goal exactly as `/goal set` would, the moment the current turn ends (the model acknowledges and stops; the first Goal turn then starts on its own), and declining sets nothing — the model sees only that the tool call was not allowed, and its instructions tell it not to ask why and not to propose the same objective again. The approval is bound to the turn that asked for it: if that turn is cancelled or otherwise never reaches its end, the approval is dropped rather than applied under a later message or an automated turn. No permission rule or approval mode (including YOLO) skips this dialog, and the tool refuses while another Goal is active, in plan mode, and in untrusted folders; subagents are never offered it. It is not available in headless runs, nor yet in Web Shell or other ACP-driven sessions (they do not pass through the turn boundary that applies the approval); there the printed `/goal set` line remains the hand-off. +In an interactive terminal or a Web Shell turn with an attached client, the model has a `propose_goal` tool. When `/goal-draft` finishes, or when you ask for an outcome that spans several turns, it can propose the objective instead of printing a `/goal set …` line for you to copy. The proposal appears as an approval dialog showing the full objective. Approving it sets the Goal exactly as `/goal set` would, the moment the current turn ends (the model acknowledges and stops; the first Goal turn then starts on its own), and declining sets nothing — the model sees only that the tool call was not allowed, and its instructions tell it not to ask why and not to propose the same objective again. The approval is bound to the turn that asked for it: if that turn is cancelled or otherwise never reaches its end, the approval is dropped rather than applied under a later message or an automated turn. No permission rule or approval mode (including YOLO) skips this dialog, and the tool refuses while another Goal is active, in plan mode, and in untrusted folders; subagents are never offered it. Web Shell uses its existing Allow/Reject permission panel. A stopped Goal can be replaced only if it still matches the version shown for approval; changing it invalidates the proposal. Headless runs, Web Shell channel deliveries and automatic turns, and ACP clients without the required approval and turn-lifecycle support retain the printed `/goal set` hand-off. Turn it off with `goals.modelProposed: "disabled"` in your user settings. Because the setting decides whether the model may ask you to start an autonomous loop, it is honored only from user and system scope; a workspace `.qwen/settings.json` value is ignored with a warning. diff --git a/packages/acp-bridge/src/bridge.test.ts b/packages/acp-bridge/src/bridge.test.ts index dc4c9fef28a..4c66e506ebf 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -431,6 +431,11 @@ describe('createAcpSessionBridge', () => { v: CHANNEL_STARTUP_PROFILE_VERSION, }, }); + expect( + handle.agent.initializeCalls[0]?.clientCapabilities?._meta, + ).toEqual({ + 'qwen.goalProposals': true, + }); // No snapshot has arrived yet, so the session is "unknown": busy rather // than idle, and graded `partial` — the channel did negotiate, it just @@ -15862,6 +15867,51 @@ describe('createAcpSessionBridge', () => { await bridge.shutdown(); }); + it('only grants Goal proposal approval to an attached prompt originator', async () => { + const handle = makeChannel(); + const bridge = makeBridge({ channelFactory: async () => handle.channel }); + const session = await bridge.spawnOrAttach({ + workspaceCwd: WS_A, + }); + const request = { + sessionId: session.sessionId, + prompt: [{ type: 'text' as const, text: 'draft a goal' }], + _meta: { 'qwen.goalProposalApproval': true }, + }; + await bridge.sendPrompt(session.sessionId, request); + expect( + handle.agent.promptCalls[0]?._meta?.['qwen.goalProposalApproval'], + ).toBeUndefined(); + await bridge.sendPrompt(session.sessionId, request, undefined, { + clientId: session.clientId, + }); + expect( + handle.agent.promptCalls[1]?._meta?.['qwen.goalProposalApproval'], + ).toBe(true); + await bridge.sendPrompt(session.sessionId, request, undefined, { + clientId: session.clientId, + channelPrompt: true, + }); + expect( + handle.agent.promptCalls[2]?._meta?.['qwen.goalProposalApproval'], + ).toBeUndefined(); + await bridge.sendPrompt(session.sessionId, request, undefined, { + clientId: session.clientId, + restoreAskUserQuestion: true, + }); + expect( + handle.agent.promptCalls[3]?._meta?.['qwen.goalProposalApproval'], + ).toBeUndefined(); + await bridge.sendPrompt(session.sessionId, request, undefined, { + clientId: session.clientId, + continue: true, + }); + expect( + handle.agent.promptCalls[4]?._meta?.['qwen.goalProposalApproval'], + ).toBeUndefined(); + await bridge.shutdown(); + }); + it('strips both spoofed retry and continue meta keys from one prompt', async () => { const handle = makeChannel(); const bridge = makeBridge({ channelFactory: async () => handle.channel }); @@ -37894,9 +37944,11 @@ describe('createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessa it('promotes messages after their client detaches without reapplying the cap', async () => { const prompts: string[] = []; + const goalApprovalMeta: unknown[] = []; const releases: Array<() => void> = []; const handle = makeChannel({ promptImpl: async (req) => { + goalApprovalMeta.push(req._meta?.['qwen.goalProposalApproval']); prompts.push( (req.prompt[0] as { text?: string } | undefined)?.text ?? '', ); @@ -37943,6 +37995,7 @@ describe('createAcpSessionBridge — mid-turn message queue (enqueueMidTurnMessa releases[0]!(); await first; await vi.waitFor(() => expect(prompts).toEqual(['first', 'follow up'])); + expect(goalApprovalMeta).toEqual([true, undefined]); expect(bridge.getMidTurnMessages(session.sessionId)).toEqual({ messages: [], settledMessageIds: [], diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 054ffcf63df..078dfbac7b6 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -5056,6 +5056,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { privateParentCapability, }, clientCapabilities: { + _meta: { 'qwen.goalProposals': true }, fs: { readTextFile: delegateReadTextFileToClient, writeTextFile: true, @@ -10537,6 +10538,16 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { // authorization and re-arms it through the trusted // `channelPrompt` context flag below. delete meta[CHANNEL_PROMPT_META_KEY]; + delete meta['qwen.goalProposalApproval']; + if ( + originatorClientId !== undefined && + entry.clientIds.has(originatorClientId) && + !context?.channelPrompt && + !isContinue && + !isRestoreAskUserQuestion + ) { + meta['qwen.goalProposalApproval'] = true; + } if (isRetry) { meta[DAEMON_RETRY_META_KEY] = true; } diff --git a/packages/acp-bridge/src/bridgeTypes.ts b/packages/acp-bridge/src/bridgeTypes.ts index 461dc08d243..870d8ac5f0b 100644 --- a/packages/acp-bridge/src/bridgeTypes.ts +++ b/packages/acp-bridge/src/bridgeTypes.ts @@ -956,9 +956,10 @@ export interface BridgeClientRequestContext { promptId?: string; /** * Internal originator for a daemon-owned mid-turn message promoted into the - * normal prompt FIFO. It was authenticated when the message was enqueued, - * so promotion must not revalidate it after that client has detached. - * Transport routes never populate this field from request input. + * normal prompt FIFO. It was authenticated when the message was enqueued, so + * promotion may still deliver it after that client detaches; turn capabilities + * that require a live client must re-check attachment. Transport routes never + * populate this field from request input. */ promotedMidTurn?: { originatorClientId?: string }; /** diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index 271ac2a42a8..c13df939713 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -2435,6 +2435,7 @@ describe('QwenAgent MCP SSE/HTTP support', () => { 'qwen.daemon.modelPrompt': 'trusted model-only prompt', 'qwen.daemon.promptDisplayText': 'trusted display text', 'qwen.channel.prompt': true, + 'qwen.goalProposalApproval': true, 'qwen.daemon.channelDelivery': { deliveryId: 'delivery-trusted', target: { channelName: 'dingtalk', type: 'user', id: 'user-1' }, @@ -2450,6 +2451,7 @@ describe('QwenAgent MCP SSE/HTTP support', () => { keep: true, 'qwen.daemon.promptDisplayText': 'trusted display text', 'qwen.channel.prompt': true, + 'qwen.goalProposalApproval': true, 'qwen.daemon.channelDelivery': { deliveryId: 'delivery-trusted', target: { channelName: 'dingtalk', type: 'user', id: 'user-1' }, @@ -2588,6 +2590,7 @@ describe('QwenAgent MCP SSE/HTTP support', () => { _meta: { keep: true, 'qwen.channel.prompt': true, + 'qwen.goalProposalApproval': true, 'qwen.daemon.channelDelivery': { deliveryId: 'delivery-forged', target: { channelName: 'dingtalk', type: 'user', id: 'user-1' }, @@ -3966,6 +3969,56 @@ describe('QwenAgent MCP SSE/HTTP support', () => { }, ); + it.each([ + [true, undefined, true], + [false, undefined, false], + [true, 'channel', true], + ])( + 'gates Goal proposals on client support %s and session source %s', + async (supported, sourceType, expected) => { + const innerConfig = await setupSessionMocks('goal-proposal-session'); + const agentPromise = runAcpAgent( + mockConfig, + makeSessionSettings(), + mockArgv, + ); + await vi.waitFor(() => expect(capturedAgentFactory).toBeDefined()); + const agent = capturedAgentFactory!({ + get closed() { + return mockConnectionState.promise; + }, + }) as AgentLike; + try { + await agent.initialize({ + clientCapabilities: { _meta: { 'qwen.goalProposals': supported } }, + }); + await agent.newSession({ + cwd: '/tmp', + mcpServers: [], + ...(sourceType + ? { _meta: { [SESSION_SOURCE_META_KEY]: { sourceType } } } + : {}), + }); + if (expected) { + expect(innerConfig.setGoalProposalHostSupported).toHaveBeenCalledWith( + true, + ); + expect( + innerConfig.setGoalProposalHostSupported.mock + .invocationCallOrder[0], + ).toBeLessThan(innerConfig.initialize.mock.invocationCallOrder[0]); + } else { + expect( + innerConfig.setGoalProposalHostSupported, + ).not.toHaveBeenCalled(); + } + } finally { + mockConnectionState.resolve(); + await agentPromise; + } + }, + ); + it('profiles newSession stages under the daemon trace context', async () => { const parentContext = { trace: 'parent' }; mockExtractDaemonTraceContext.mockReturnValue(parentContext); @@ -4462,6 +4515,7 @@ describe('QwenAgent MCP SSE/HTTP support', () => { setSessionWriterReclaimPolicy: vi.fn(), setSessionWriterTakeoverPolicy: vi.fn(), setSessionSource: vi.fn(), + setGoalProposalHostSupported: vi.fn(), setSessionSourceServiceFactory: vi.fn(), registerSessionSourceTool: vi.fn().mockResolvedValue(undefined), getSessionSourceService: vi.fn(), @@ -24378,6 +24432,7 @@ describe('QwenAgent loadSession / unstable_resumeSession', () => { setSessionWriterReclaimPolicy: vi.fn(), setSessionWriterTakeoverPolicy: vi.fn(), setSessionSource: vi.fn(), + setGoalProposalHostSupported: vi.fn(), setSessionSourceServiceFactory: vi.fn(), registerSessionSourceTool: vi.fn().mockResolvedValue(undefined), getSessionSourceService: vi.fn(), diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index 57a8287ca45..c4303108f0d 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -6304,12 +6304,20 @@ class QwenAgent implements Agent { const suppliedModelPrompt = meta[DAEMON_MODEL_PROMPT_META_KEY]; const suppliedPromptDisplayText = meta[DAEMON_PROMPT_DISPLAY_TEXT_META_KEY]; const suppliedChannelPrompt = meta[CHANNEL_PROMPT_META_KEY]; + const suppliedGoalProposalApproval = meta['qwen.goalProposalApproval']; const suppliedChannelDelivery = meta[DAEMON_CHANNEL_DELIVERY_META_KEY]; delete meta[INVOCATION_CONTEXT_META_KEY]; delete meta[DAEMON_MODEL_PROMPT_META_KEY]; delete meta[PRIVATE_PARENT_CAPABILITY_META_KEY]; delete meta[DAEMON_PROMPT_DISPLAY_TEXT_META_KEY]; delete meta[CHANNEL_PROMPT_META_KEY]; + delete meta['qwen.goalProposalApproval']; + if ( + this.privateParentState === 'trusted' && + suppliedGoalProposalApproval === true + ) { + meta['qwen.goalProposalApproval'] = true; + } delete meta[DAEMON_CHANNEL_DELIVERY_META_KEY]; // The user-facing display projection is caller-controlled metadata; honor // it only for trusted parents (the daemon bridge re-injects the trusted @@ -14317,6 +14325,9 @@ class QwenAgent implements Agent { if (sessionSource) { config.setSessionSource(sessionSource.sourceType, sessionSource.sourceId); } + if (this.clientCapabilities?._meta?.['qwen.goalProposals'] === true) { + config.setGoalProposalHostSupported(true); + } if (chatRecording !== false) { this.initializingConfigs.add(config); } diff --git a/packages/cli/src/acp-integration/session/Session.review-lease.test.ts b/packages/cli/src/acp-integration/session/Session.review-lease.test.ts index 59ca8b19663..a30b4169b06 100644 --- a/packages/cli/src/acp-integration/session/Session.review-lease.test.ts +++ b/packages/cli/src/acp-integration/session/Session.review-lease.test.ts @@ -180,6 +180,7 @@ describe('Session review-worktree lease sweep', () => { }), setSubSessionSpawner: vi.fn(), getSubSessionSpawner: vi.fn(), + getGoalProposalHostSupported: vi.fn().mockReturnValue(false), // The Session constructor and Session.prompt both reach for the // canonical Goal runtime. A real Config throws this exact error when // Goal persistence is off, and both call sites are written to fall diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index df45833a2c0..a9818335b00 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -72,6 +72,7 @@ import { createReplayCumulativeUsage, } from './history-replay-page.js'; import { createRepeatedToolFailureGuardState } from './repeated-tool-failure-guard.js'; +import type { DaemonTodoStopGuard } from './daemon-todo-stop-guard.js'; const debugLoggerWarnSpy = vi.hoisted(() => vi.fn()); const debugLoggerDebugSpy = vi.hoisted(() => vi.fn()); @@ -493,6 +494,7 @@ describe('Session', () => { let session: Session; let currentModel: string; let currentAuthType: AuthType; + let goalProposalTurnKey: string | undefined; let originalProcessGuardMode: string | undefined; let originalServeStamp: string | undefined; let switchModelSpy: ReturnType; @@ -724,6 +726,7 @@ describe('Session', () => { transcribeVoiceAudioSpy.mockReset(); currentModel = 'qwen3-code-plus'; currentAuthType = AuthType.USE_OPENAI; + goalProposalTurnKey = undefined; switchModelSpy = vi .fn() .mockImplementation(async (authType: AuthType, modelId: string) => { @@ -988,6 +991,13 @@ describe('Session', () => { getMemoryManager: vi.fn().mockReturnValue(mockMemoryManager), getGoalRuntime: vi.fn().mockReturnValue(mockGoalRuntime), getGoalRuntimeReady: vi.fn().mockResolvedValue(mockGoalRuntime), + takePendingGoalProposal: vi.fn(), + getGoalProposalHostSupported: vi.fn().mockReturnValue(false), + setGoalProposalTurnKey: vi.fn((turnKey: string | undefined) => { + if (goalProposalTurnKey === turnKey) return false; + goalProposalTurnKey = turnKey; + return true; + }), getGoalRuntimePrepared: vi.fn().mockResolvedValue(mockGoalRuntime), bindGoalTurnHost: vi.fn().mockImplementation((host) => { boundGoalHost = host; @@ -33044,6 +33054,1048 @@ describe('Session', () => { }); }); + describe('ordinary-turn Goal proposals', () => { + const objective = 'Outcome: tests pass. Done when: npm test exits 0.'; + let pending: core.PendingGoalProposal | undefined; + + function useRealApprovalState() { + const config = new core.Config({ + cwd: '/tmp', + targetDir: '/tmp', + model: 'test-model', + approvalMode: ApprovalMode.DEFAULT, + debugMode: false, + chatRecording: false, + usageStatisticsEnabled: false, + overrideExtensions: [], + }); + Object.assign(mockConfig, { + getApprovalMode: config.getApprovalMode.bind(config), + setApprovalMode: config.setApprovalMode.bind(config), + hasPendingGoalProposal: config.hasPendingGoalProposal.bind(config), + setPendingGoalProposal: config.setPendingGoalProposal.bind(config), + takePendingGoalProposal: config.takePendingGoalProposal.bind(config), + }); + return config; + } + + function proposalStream() { + return createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'goal-proposal', + name: 'propose_goal', + args: { objective }, + }, + ], + }, + }, + ]); + } + + function prompt(invocationContext?: core.InvocationContextV1) { + return session.prompt( + { + sessionId: 'test-session-id', + _meta: { 'qwen.goalProposalApproval': true }, + prompt: [ + { type: 'text', text: 'Draft a Goal to make the tests pass.' }, + ], + }, + invocationContext, + ); + } + + beforeEach(() => { + pending = undefined; + mockConfig.getGoalProposalHostSupported = vi.fn().mockReturnValue(true); + mockConfig.hasPendingGoalProposal = vi.fn(() => pending !== undefined); + mockConfig.setPendingGoalProposal = vi.fn((proposal) => { + if (pending) return false; + pending = proposal; + return true; + }); + mockConfig.takePendingGoalProposal = vi.fn((turnKey) => { + if (turnKey !== undefined && pending?.turnKey !== turnKey) { + return undefined; + } + const proposal = pending; + pending = undefined; + return proposal; + }); + const tool = new core.ProposeGoalTool(mockConfig); + mockToolRegistry.getTool.mockImplementation((name) => + name === 'propose_goal' ? tool : undefined, + ); + mockGoalRuntime.dispatch.mockResolvedValue({ + snapshot: { + v: 2, + activity: 'idle', + goal: { goalId: 'approved-goal', revision: 1, status: 'active' }, + }, + }); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(proposalStream()) + .mockImplementation(async () => createEmptyStream()); + }); + + it('requires explicit approval in YOLO even with an allow rule, then applies after acknowledgement exactly once', async () => { + mockConfig.getApprovalMode = vi.fn().mockReturnValue(ApprovalMode.YOLO); + mockConfig.getPermissionManager = vi.fn().mockReturnValue({ + isToolEnabled: vi.fn().mockResolvedValue(true), + hasRelevantRules: vi.fn().mockReturnValue(true), + evaluate: vi.fn().mockResolvedValue('allow'), + }); + let releaseAcknowledgement!: () => void; + const acknowledgement = new Promise((resolve) => { + releaseAcknowledgement = resolve; + }); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(proposalStream()) + .mockImplementationOnce(async () => { + expect(pending?.objective).toBe(objective); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + return (async function* () { + await acknowledgement; + yield { + type: core.StreamEventType.CHUNK, + value: { + candidates: [ + { content: { parts: [{ text: 'Goal approved.' }] } }, + ], + }, + }; + })(); + }) + .mockImplementation(async () => createEmptyStream()); + + const result = prompt(); + await vi.waitFor(() => { + expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(2); + }); + expect(mockClient.requestPermission).toHaveBeenCalledOnce(); + expect(mockClient.requestPermission).toHaveBeenCalledWith( + expect.objectContaining({ + toolCall: expect.objectContaining({ + content: [ + expect.objectContaining({ + content: expect.objectContaining({ + text: expect.stringContaining(objective), + }), + }), + ], + }), + options: [ + expect.objectContaining({ kind: 'allow_once' }), + expect.objectContaining({ kind: 'reject_once' }), + ], + }), + ); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + releaseAcknowledgement(); + await expect(result).resolves.toEqual({ stopReason: 'end_turn' }); + expect(mockGoalRuntime.dispatch).toHaveBeenCalledExactlyOnceWith({ + action: 'create', + objective, + }); + expect(pending).toBeUndefined(); + expect(mockChatRecordingService.recordToolResult).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ + functionResponse: expect.objectContaining({ + id: 'goal-proposal', + name: 'propose_goal', + }), + }), + ]), + expect.anything(), + ); + await prompt(); + expect(mockGoalRuntime.dispatch).toHaveBeenCalledTimes(1); + }); + + it.each(['plan', 'default'])( + 'revokes an approved Goal after entering Plan, even if the final mode is %s', + async (finalMode) => { + const config = useRealApprovalState(); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(proposalStream()) + .mockImplementationOnce(async () => { + expect(config.hasPendingGoalProposal()).toBe(true); + await session.setMode({ + sessionId: 'test-session-id', + modeId: 'plan', + }); + if (finalMode === 'default') { + await session.setMode({ + sessionId: 'test-session-id', + modeId: 'default', + }); + } + return createEmptyStream(); + }); + + await prompt(); + + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + expect(config.hasPendingGoalProposal()).toBe(false); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'agent_message_chunk', + content: expect.objectContaining({ + text: expect.stringContaining('approval was revoked'), + }), + }), + }), + ); + }, + ); + + it('revokes approval when Plan mode begins during runtime loading', async () => { + useRealApprovalState(); + vi.mocked(mockConfig.getGoalRuntimeReady) + .mockReset() + .mockResolvedValueOnce(mockGoalRuntime as unknown as core.GoalRuntime) + .mockResolvedValueOnce(mockGoalRuntime as unknown as core.GoalRuntime) + .mockImplementationOnce(async () => { + await session.setMode({ + sessionId: 'test-session-id', + modeId: 'plan', + }); + return mockGoalRuntime as unknown as core.GoalRuntime; + }); + + await prompt(); + + expect(mockConfig.getGoalRuntimeReady).toHaveBeenCalledTimes(3); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + }); + + it('pauses a Goal when Plan mode begins during creation persistence', async () => { + useRealApprovalState(); + mockGoalRuntime.dispatch.mockImplementationOnce(async () => { + await session.setMode({ sessionId: 'test-session-id', modeId: 'plan' }); + return { + snapshot: { + goal: { goalId: 'approved-goal', revision: 1, status: 'active' }, + }, + }; + }); + + await prompt(); + + expect(mockGoalRuntime.dispatch).toHaveBeenNthCalledWith(2, { + action: 'pause', + expectedGoalId: 'approved-goal', + expectedRevision: 1, + reason: GOAL_PAUSE_REASON_USER_INTERRUPT, + }); + }); + + it('arms and clears declarations only for an approval-capable user turn', async () => { + mockConfig.getGoalProposalHostSupported = vi.fn().mockReturnValue(true); + + await prompt(); + + expect(mockConfig.setGoalProposalTurnKey).toHaveBeenNthCalledWith( + 1, + 'test-session-id########1', + ); + expect(mockConfig.setGoalProposalTurnKey).toHaveBeenNthCalledWith( + 2, + undefined, + ); + expect(mockLlmClient.setTools).toHaveBeenCalledTimes(2); + + vi.mocked(mockConfig.setGoalProposalTurnKey).mockClear(); + mockLlmClient.setTools.mockClear(); + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'An unarmed turn.' }], + }); + expect(mockConfig.setGoalProposalTurnKey).toHaveBeenCalledTimes(2); + expect(mockConfig.setGoalProposalTurnKey).toHaveBeenNthCalledWith( + 1, + undefined, + ); + expect(mockConfig.setGoalProposalTurnKey).toHaveBeenNthCalledWith( + 2, + undefined, + ); + expect(mockLlmClient.setTools).not.toHaveBeenCalled(); + }); + + it('applies an approved proposal when a sibling permission is rejected', async () => { + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce( + createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'goal-proposal', + name: 'propose_goal', + args: { objective }, + }, + { + id: 'question', + name: core.ToolNames.ASK_USER_QUESTION, + args: { questions: [] }, + }, + ], + }, + }, + ]), + ) + .mockImplementation(async () => createEmptyStream()); + const proposalTool = new core.ProposeGoalTool(mockConfig); + mockToolRegistry.getTool.mockImplementation((name) => + name === 'propose_goal' + ? proposalTool + : mockConfirmingTool(name, vi.fn()), + ); + vi.mocked(mockClient.requestPermission) + .mockResolvedValueOnce({ + outcome: { outcome: 'selected', optionId: 'proceed_once' }, + }) + .mockResolvedValueOnce({ + outcome: { + outcome: 'selected', + optionId: core.ToolConfirmationOutcome.Cancel, + }, + }); + + await expect(prompt()).resolves.toEqual({ stopReason: 'end_turn' }); + expect(mockGoalRuntime.dispatch).toHaveBeenCalledWith({ + action: 'create', + objective, + }); + }); + + it.each(['reject', 'cancel'] as const)( + 'does not create a Goal after permission %s', + async (outcome) => { + vi.mocked(mockClient.requestPermission).mockResolvedValueOnce({ + outcome: + outcome === 'reject' + ? { + outcome: 'selected', + optionId: core.ToolConfirmationOutcome.Cancel, + } + : { outcome: 'cancelled' }, + }); + await prompt(); + expect(mockClient.requestPermission).toHaveBeenCalledOnce(); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + expect(pending).toBeUndefined(); + }, + ); + + it('discards approval when the acknowledgement fails', async () => { + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(proposalStream()) + .mockRejectedValueOnce(new Error('provider failed')); + await expect(prompt()).rejects.toThrow('provider failed'); + expect(mockConfig.setPendingGoalProposal).toHaveBeenCalledOnce(); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + expect(pending).toBeUndefined(); + expect(mockConfig.setGoalProposalTurnKey).toHaveBeenLastCalledWith( + undefined, + ); + expect(mockLlmClient.setTools).toHaveBeenCalled(); + }); + + it('discards approval on cancellation before acknowledgement ends', async () => { + let finishStream!: () => void; + const gate = new Promise((resolve) => { + finishStream = resolve; + }); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(proposalStream()) + .mockResolvedValueOnce( + (async function* () { + await gate; + yield* createEmptyStream(); + })(), + ); + const result = prompt(); + await vi.waitFor(() => + expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(2), + ); + await session.cancelPendingPrompt(); + finishStream(); + await expect(result).resolves.toEqual({ stopReason: 'cancelled' }); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + expect(pending).toBeUndefined(); + }); + + it('does not apply after cancellation while the Goal runtime loads', async () => { + let finishRuntimeLoad!: () => void; + const runtimeLoad = new Promise((resolve) => { + finishRuntimeLoad = () => + resolve(mockGoalRuntime as unknown as core.GoalRuntime); + }); + vi.mocked(mockConfig.getGoalRuntimeReady) + .mockReset() + .mockResolvedValueOnce(mockGoalRuntime as unknown as core.GoalRuntime) + .mockResolvedValueOnce(mockGoalRuntime as unknown as core.GoalRuntime) + .mockReturnValueOnce(runtimeLoad); + + const result = prompt(); + await vi.waitFor(() => + expect(mockConfig.getGoalRuntimeReady).toHaveBeenCalledTimes(3), + ); + expect(mockConfig.setPendingGoalProposal).toHaveBeenCalledOnce(); + await session.cancelPendingPrompt(); + finishRuntimeLoad(); + + await expect(result).resolves.toEqual({ stopReason: 'end_turn' }); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + expect(pending).toBeUndefined(); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'agent_message_chunk', + content: expect.objectContaining({ + text: `The approved Goal was not started because the turn did not finish normally. To start it, run:\n/goal set ${objective}`, + }), + }), + }), + ); + }); + + it('pauses a Goal if cancellation arrives while its creation is persisted', async () => { + let completeCreate!: () => void; + const gate = new Promise((resolve) => { + completeCreate = resolve; + }); + mockGoalRuntime.dispatch.mockImplementationOnce(async () => { + await gate; + return { + snapshot: { + goal: { goalId: 'approved-goal', revision: 1, status: 'active' }, + }, + }; + }); + const result = prompt(); + await vi.waitFor(() => + expect(mockGoalRuntime.dispatch).toHaveBeenCalledOnce(), + ); + await session.cancelPendingPrompt(); + completeCreate(); + await result; + expect(mockGoalRuntime.dispatch).toHaveBeenNthCalledWith(2, { + action: 'pause', + expectedGoalId: 'approved-goal', + expectedRevision: 1, + reason: GOAL_PAUSE_REASON_USER_INTERRUPT, + }); + }); + + it('attributes a close during Goal creation to session disposal', async () => { + let completeCreate!: () => void; + const gate = new Promise((resolve) => { + completeCreate = resolve; + }); + mockGoalRuntime.dispatch.mockImplementationOnce(async () => { + await gate; + return { + snapshot: { + goal: { goalId: 'approved-goal', revision: 1, status: 'active' }, + }, + }; + }); + const result = prompt(); + await vi.waitFor(() => + expect(mockGoalRuntime.dispatch).toHaveBeenCalledOnce(), + ); + const releaseClose = session.beginClose(); + completeCreate(); + await result; + + expect(mockGoalRuntime.dispatch).toHaveBeenNthCalledWith(2, { + action: 'pause', + expectedGoalId: 'approved-goal', + expectedRevision: 1, + reason: GOAL_PAUSE_REASON_SESSION_DISPOSED, + }); + releaseClose(); + }); + + it('reports when the approved proposal no longer matches the Goal', async () => { + mockGoalRuntime.getSnapshot.mockReturnValue({ + v: 2, + activity: 'idle', + goal: null, + }); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(proposalStream()) + .mockImplementationOnce(async () => { + mockGoalRuntime.getSnapshot.mockReturnValue({ + v: 2, + activity: 'idle', + goal: { goalId: 'newer-goal', revision: 2, status: 'paused' }, + }); + return createEmptyStream(); + }); + + await prompt(); + + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + expect(debugLoggerDebugSpy).toHaveBeenCalledWith( + expect.stringContaining( + 'The Goal changed after the proposal was shown', + ), + ); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'agent_message_chunk', + content: expect.objectContaining({ + text: expect.stringContaining( + 'Goal changed after the proposal was shown', + ), + }), + }), + }), + ); + expect(mockClient.sessionUpdate).not.toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'agent_message_chunk', + content: expect.objectContaining({ + text: expect.stringContaining('/goal set'), + }), + }), + }), + ); + }); + + it('reports when applying the approved proposal fails', async () => { + mockGoalRuntime.dispatch.mockRejectedValueOnce( + new Error('goal journal unavailable'), + ); + + await prompt(); + + expect(mockGoalRuntime.dispatch).toHaveBeenCalledOnce(); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'agent_message_chunk', + content: expect.objectContaining({ + text: `The approved Goal could not be started. Check the Goal status before trying again, or run:\n/goal set ${objective}`, + }), + }), + }), + ); + }); + + it('holds a cancelled Goal when pause persistence fails, then permits a user pause and resume', async () => { + const goal = { + goalId: 'approved-goal', + revision: 1, + objective, + status: 'active' as const, + evidenceCursor: { recordId: 'created-goal' }, + turnCount: 0, + activeTimeMs: 0, + tokensUsed: 0, + createdAt: 1, + updatedAt: 1, + }; + const snapshot = { v: 2 as const, activity: 'idle' as const, goal }; + const listener = mockGoalRuntime.subscribe.mock.calls[0][0] as ( + snapshot: core.GoalSnapshotV2, + cause: core.GoalStateCause, + ) => void; + let completeCreate!: () => void; + const gate = new Promise((resolve) => { + completeCreate = resolve; + }); + mockGoalRuntime.dispatch + .mockImplementationOnce(async () => { + await gate; + mockGoalRuntime.getSnapshot.mockReturnValue(snapshot); + listener(snapshot, 'create'); + await boundGoalHost!.startGoalTurn({ + permit: { + goalId: goal.goalId, + revision: 1, + turnId: 'cancelled-start', + }, + continuationContext: objective, + }); + return { snapshot }; + }) + .mockRejectedValueOnce(new Error('pause journal write failed')); + + const result = prompt(); + await vi.waitFor(() => + expect(mockGoalRuntime.dispatch).toHaveBeenCalledOnce(), + ); + await session.cancelPendingPrompt(); + completeCreate(); + await result; + await new Promise((resolve) => setTimeout(resolve, 0)); + + expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(2); + expect(mockGoalRuntime.finishTurn).not.toHaveBeenCalled(); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'agent_message_chunk', + content: expect.objectContaining({ + text: 'The Goal was created, but its cancellation could not be saved. Automatic execution is held in this session. Run:\n/goal pause\nThen, when ready:\n/goal resume', + }), + }), + }), + ); + + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'What is the weather?' }], + }); + expect(mockGoalRuntime.beginTurn).not.toHaveBeenCalled(); + expect(mockGoalRuntime.finishTurn).not.toHaveBeenCalled(); + expect(mockGoalRuntime.releaseTurn).toHaveBeenCalledWith( + 'goal-runtime:cancelled-start', + { requeue: false }, + ); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'agent_message_chunk', + content: expect.objectContaining({ + text: 'Automatic Goal execution is still held. Run:\n/goal pause\nThen, when ready:\n/goal resume', + }), + }), + }), + ); + + const paused = { + ...snapshot, + goal: { ...goal, status: 'paused' as const }, + }; + mockGoalRuntime.getSnapshot.mockReturnValue(paused); + listener(paused, 'pause'); + await boundGoalHost!.preemptGoalTurn('Goal pause'); + mockGoalRuntime.getSnapshot.mockReturnValue(snapshot); + listener(snapshot, 'resume'); + const permit = { + goalId: goal.goalId, + revision: 1, + turnId: 'resumed-start', + }; + mockGoalRuntime.permitForTurn.mockReturnValue(permit); + await boundGoalHost!.startGoalTurn({ + permit, + continuationContext: objective, + }); + await vi.waitFor(() => + expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(3), + ); + await vi.waitFor(() => + expect(mockGoalRuntime.finishTurn).toHaveBeenCalledWith(permit), + ); + }); + + it('does not settle a proposal owned by another turn', async () => { + mockConfig.setPendingGoalProposal = vi.fn((proposal) => { + pending = { ...proposal, turnKey: 'another-turn' }; + return true; + }); + await prompt(); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + expect(pending?.turnKey).toBe('another-turn'); + }); + + it('does not start a Goal when a protective Stop-hook cap returns end_turn', async () => { + mockConfig.getStopHookBlockingCap = vi.fn().mockReturnValue(1); + mockConfig.getDisableAllHooks = vi.fn().mockReturnValue(false); + mockConfig.hasHooksForEvent = vi.fn((event) => event === 'Stop'); + mockConfig.getMessageBus = vi.fn().mockReturnValue({ + request: vi.fn().mockImplementation(async (request) => ({ + success: true, + output: + request.eventName === 'Stop' + ? { + decision: 'block', + reason: 'The requested work is unfinished', + } + : {}, + })), + }); + + await expect(prompt()).resolves.toEqual({ stopReason: 'end_turn' }); + + expect(mockConfig.setPendingGoalProposal).toHaveBeenCalledOnce(); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + expect(pending).toBeUndefined(); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'agent_message_chunk', + content: expect.objectContaining({ + text: expect.stringContaining('/goal set'), + }), + }), + }), + ); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + content: expect.objectContaining({ + text: expect.stringContaining(objective), + }), + }), + }), + ); + }); + + it('does not start a Goal when a queued prompt takes priority', async () => { + const internals = session as unknown as { + todoStopGuardQueuedPromptPriority: boolean; + }; + const coreSettler = new core.LlmClient(mockConfig) as unknown as { + settlePendingGoalProposal: ( + turnEnded: boolean, + signal: AbortSignal, + loadGoalRuntime: ( + required: boolean, + ) => Promise, + turnKey: string, + reportFailure: (message: string) => void, + ) => Promise; + }; + mockConfig.setPendingGoalProposal = vi.fn((proposal) => { + pending = proposal; + internals.todoStopGuardQueuedPromptPriority = true; + return true; + }); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(proposalStream()) + .mockImplementationOnce(async () => { + await coreSettler.settlePendingGoalProposal( + true, + new AbortController().signal, + async () => mockGoalRuntime as unknown as core.GoalRuntime, + 'test-session-id########1', + vi.fn(), + ); + return createEmptyStream(); + }); + + await expect(prompt()).resolves.toEqual({ stopReason: 'end_turn' }); + + expect(mockConfig.setPendingGoalProposal).toHaveBeenCalledOnce(); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + expect(pending).toBeUndefined(); + }); + + it('does not start a Goal when an empty drain yields to a queued prompt', async () => { + const internals = session as unknown as { + todoStopGuard: DaemonTodoStopGuard; + }; + Object.defineProperty(internals.todoStopGuard, 'needsStopInspection', { + configurable: true, + get: () => true, + }); + vi.mocked(mockClient.extMethod).mockImplementation(async (method) => { + if (method === 'craft/drainMidTurnQueue') { + return { messages: [], hasQueuedPrompt: true }; + } + if (method === TODO_STOP_GUARD_CONTINUATION_CLAIM_METHOD) { + return { claimed: false, hasQueuedPrompt: true }; + } + return {}; + }); + + await expect( + prompt({ + version: 1, + sessionId: 'test-session-id', + promptId: 'queued-prompt-owner', + }), + ).resolves.toEqual({ stopReason: 'end_turn' }); + + expect(mockConfig.setPendingGoalProposal).toHaveBeenCalledOnce(); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + expect(pending).toBeUndefined(); + }); + + it('does not start a Goal when a guard continuation yields to a queued prompt before sending', async () => { + const internals = session as unknown as { + todoStopGuard: DaemonTodoStopGuard; + }; + vi.spyOn(internals.todoStopGuard, 'decide').mockReturnValue({ + kind: 'continue', + attempt: 1, + maxAttempts: 2, + unfinishedCount: 1, + }); + vi.mocked(mockClient.extMethod).mockImplementation(async (method) => { + if (method === 'craft/drainMidTurnQueue') { + return { messages: [], hasQueuedPrompt: true }; + } + if (method === TODO_STOP_GUARD_CONTINUATION_CLAIM_METHOD) { + return { claimed: false, hasQueuedPrompt: true }; + } + return {}; + }); + + await expect( + prompt({ + version: 1, + sessionId: 'test-session-id', + promptId: 'queued-prompt-owner', + }), + ).resolves.toEqual({ stopReason: 'end_turn' }); + + expect(mockConfig.setPendingGoalProposal).toHaveBeenCalledOnce(); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + expect(pending).toBeUndefined(); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'agent_message_chunk', + content: expect.objectContaining({ + text: expect.stringContaining('/goal set'), + }), + }), + }), + ); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + content: expect.objectContaining({ + text: expect.stringContaining(objective), + }), + }), + }), + ); + }); + + it('does not start a Goal when a guard continuation stops on an unreliable queue drain', async () => { + const internals = session as unknown as { + todoStopGuard: DaemonTodoStopGuard; + }; + vi.spyOn(internals.todoStopGuard, 'decide').mockReturnValue({ + kind: 'continue', + attempt: 1, + maxAttempts: 2, + unfinishedCount: 1, + }); + // A response with no 'messages'/'items' payload fails validation, so + // the continuation is skipped without ever consulting the claim. + vi.mocked(mockClient.extMethod).mockImplementation(async () => ({})); + + await expect(prompt()).resolves.toEqual({ stopReason: 'end_turn' }); + + expect(mockConfig.setPendingGoalProposal).toHaveBeenCalledOnce(); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + expect(pending).toBeUndefined(); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'agent_message_chunk', + content: expect.objectContaining({ + text: expect.stringContaining('/goal set'), + }), + }), + }), + ); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + content: expect.objectContaining({ + text: expect.stringContaining(objective), + }), + }), + }), + ); + }); + + it('does not start a Goal when a queued prompt claims the guard continuation at send time', async () => { + const internals = session as unknown as { + todoStopGuard: DaemonTodoStopGuard; + }; + vi.spyOn(internals.todoStopGuard, 'decide').mockReturnValue({ + kind: 'continue', + attempt: 1, + maxAttempts: 2, + unfinishedCount: 1, + }); + vi.mocked(mockClient.extMethod).mockImplementation(async (method) => { + if (method === 'craft/drainMidTurnQueue') { + return { messages: [], hasQueuedPrompt: false }; + } + if (method === TODO_STOP_GUARD_CONTINUATION_CLAIM_METHOD) { + return { claimed: false, hasQueuedPrompt: true }; + } + return {}; + }); + + await expect( + prompt({ + version: 1, + sessionId: 'test-session-id', + promptId: 'queued-prompt-owner', + }), + ).resolves.toEqual({ stopReason: 'end_turn' }); + + expect(mockConfig.setPendingGoalProposal).toHaveBeenCalledOnce(); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + expect(pending).toBeUndefined(); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'agent_message_chunk', + content: expect.objectContaining({ + text: expect.stringContaining('/goal set'), + }), + }), + }), + ); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + content: expect.objectContaining({ + text: expect.stringContaining(objective), + }), + }), + }), + ); + }); + + it('does not start a Goal when the Todo Stop Guard is exhausted', async () => { + const internals = session as unknown as { + todoStopGuard: { + decide: () => { + kind: 'exhausted'; + attempt: number; + maxAttempts: number; + unfinishedCount: number; + }; + }; + }; + vi.spyOn(internals.todoStopGuard, 'decide').mockReturnValue({ + kind: 'exhausted', + attempt: 2, + maxAttempts: 2, + unfinishedCount: 1, + }); + + await expect(prompt()).resolves.toEqual({ stopReason: 'end_turn' }); + + expect(mockConfig.setPendingGoalProposal).toHaveBeenCalledOnce(); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + expect(pending).toBeUndefined(); + }); + + it('discards approval when new user input is injected after the tool result', async () => { + vi.mocked(mockClient.extMethod).mockImplementation(async (method) => { + if (method === 'craft/drainMidTurnQueue' && pending) { + return { + messages: ['Do not start yet; first change the objective.'], + hasQueuedPrompt: false, + }; + } + return { messages: [], hasQueuedPrompt: false }; + }); + + await prompt(); + + expect(mockConfig.setPendingGoalProposal).toHaveBeenCalledOnce(); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + expect(pending).toBeUndefined(); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'agent_message_chunk', + content: expect.objectContaining({ + text: expect.stringContaining('/goal set'), + }), + }), + }), + ); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + content: expect.objectContaining({ + text: expect.stringContaining(objective), + }), + }), + }), + ); + }); + + it('refuses proposals from an automatic background-notification turn', async () => { + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValueOnce(createEmptyStream()) + .mockResolvedValueOnce(proposalStream()) + .mockImplementation(async () => createEmptyStream()); + await prompt(); + const notify = mockBackgroundTaskRegistry.setNotificationCallback.mock + .calls[0][0] as ( + displayText: string, + modelText: string, + meta: { agentId: string; status: string; toolUseId?: string }, + ) => void; + notify('Background work finished', '', { + agentId: 'background-agent', + status: 'completed', + }); + await vi.waitFor(() => + expect(mockClient.extNotification).toHaveBeenCalledWith( + '_qwencode/end_turn', + expect.objectContaining({ source: 'background_notification' }), + ), + ); + expect(mockClient.requestPermission).not.toHaveBeenCalled(); + expect(mockConfig.setPendingGoalProposal).not.toHaveBeenCalled(); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'tool_call_update', + content: expect.arrayContaining([ + expect.objectContaining({ + content: expect.objectContaining({ + text: expect.stringContaining('/goal set'), + }), + }), + ]), + }), + }), + ); + }); + + it('refuses a prompt without a trusted approval responder', async () => { + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'Draft a Goal.' }], + }); + expect(mockClient.requestPermission).not.toHaveBeenCalled(); + expect(mockConfig.setPendingGoalProposal).not.toHaveBeenCalled(); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + }); + }); + describe('runToolCalls', () => { type ToolCallInternals = { runToolCalls: ( diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index d6c79bd41f3..7aa9e3aae5b 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -189,6 +189,9 @@ import { GOAL_PAUSE_REASON_SESSION_DISPOSED, GOAL_PAUSE_REASON_STOP_HOOK_CAP, GOAL_PAUSE_REASON_USER_INTERRUPT, + applyPendingGoalProposal, + formatProposeGoalRecoveryFailed, + formatProposeGoalRecoveryNotStarted, goalPauseReasonForFailure, ambientGoalToolResultProvenance, goalTurnContext, @@ -433,6 +436,8 @@ const MAX_RETAINED_SESSION_ROUTE_COUNTS = 8; const USER_CANCEL_ABORT_REASON = 'qwen:user-cancel'; const NEW_PROMPT_ABORT_REASON = 'qwen:new-prompt'; const SESSION_DISPOSE_ABORT_REASON = 'qwen:session-dispose'; +const GOAL_HELD_RECOVERY_COMMANDS = + 'Run:\n/goal pause\nThen, when ready:\n/goal resume'; const DAEMON_RETRY_META_KEY = 'qwen.daemon.retry'; const DAEMON_CONTINUE_META_KEY = 'qwen.daemon.continueLastTurn'; const MAX_DAEMON_ATTACHMENT_REFERENCES = 256; @@ -1533,6 +1538,12 @@ interface PromptChannelDelivery { } interface AgentResponseCapture { + goalProposalTurn?: { + turnKey: string; + controller: AbortController; + completedNormally: boolean; + settlementBlocked?: boolean; + }; channelDelivery?: { finalText: string; }; @@ -1992,6 +2003,7 @@ export async function buildAvailableCommandsSnapshot( */ export class Session implements SessionContext { private pendingPrompt: AbortController | null = null; + private activeGoalProposalTurn?: AgentResponseCapture['goalProposalTurn']; /** * Tracks the completion of the current prompt so that the next prompt * can await it. This prevents a new prompt from reading chat history @@ -2091,6 +2103,7 @@ export class Session implements SessionContext { private readonly activeNotificationAcceptances = new Set(); private readonly goalQueue: AcpGoalTurn[] = []; + private heldGoalProposal?: Pick; private goalProcessing = false; private activeGoalTurn: AcpGoalTurn | undefined; private goalHostUnbind?: () => void; @@ -2316,6 +2329,15 @@ export class Session implements SessionContext { this.goalRuntimeUnsubscribe = runtime.subscribe((snapshot, cause) => { const previousGoal = this.lastGoalSnapshot?.goal ?? null; this.lastGoalSnapshot = snapshot; + if ( + this.heldGoalProposal && + (snapshot.goal?.goalId !== this.heldGoalProposal.goalId || + snapshot.goal.revision !== this.heldGoalProposal.revision || + snapshot.goal.status !== 'active' || + cause === 'resume') + ) { + this.heldGoalProposal = undefined; + } void this.#queueGoalState(snapshot, cause, previousGoal).catch( (error) => debugLogger.warn( @@ -2553,6 +2575,14 @@ export class Session implements SessionContext { ) { return; } + const next = this.goalQueue[0]; + if ( + this.heldGoalProposal && + next?.permit.goalId === this.heldGoalProposal.goalId && + next.permit.revision === this.heldGoalProposal.revision + ) { + return; + } const turn = this.goalQueue.shift(); if (!turn) return; @@ -4713,7 +4743,19 @@ export class Session implements SessionContext { if (!goalTurn) { try { const runtime = this.config.getGoalRuntime(); - if (runtime.getSnapshot().goal?.status === 'active') { + const activeGoal = runtime.getSnapshot().goal; + const heldGoalMatches = + this.heldGoalProposal?.goalId === activeGoal?.goalId && + this.heldGoalProposal?.revision === activeGoal?.revision; + if (activeGoal?.status === 'active' && heldGoalMatches) { + for (const queued of this.goalQueue.splice(0)) { + queued.controller.abort(NEW_PROMPT_ABORT_REASON); + await runtime.releaseTurn(queued.turnKey, { requeue: false }); + } + await this.messageEmitter.emitAgentMessage( + `Automatic Goal execution is still held. ${GOAL_HELD_RECOVERY_COMMANDS}`, + ); + } else if (activeGoal?.status === 'active') { reservedGoalRuntime = runtime; reservedGoalTurnKey = `goal-user:${randomUUID()}`; runtime.beginTurn(reservedGoalTurnKey); @@ -4956,6 +4998,14 @@ export class Session implements SessionContext { } : result; promptResult = completedResult; + const proposalTurn = responseCapture.goalProposalTurn; + if (proposalTurn) { + proposalTurn.completedNormally = + completedResult.stopReason === 'end_turn' && + !pendingSend.signal.aborted && + proposalTurn.settlementBlocked !== true; + } + await this.#settleGoalProposal(responseCapture); releasePendingSend(); // Drain any cron prompts that queued while the prompt was active void this.#drainCronQueue(); @@ -4985,6 +5035,26 @@ export class Session implements SessionContext { rejectedByLoopProtection = isLoopDetectedTurnError(error); throw error; } finally { + const proposalTurn = responseCapture.goalProposalTurn; + if (proposalTurn) { + this.config.takePendingGoalProposal(proposalTurn.turnKey); + if (this.activeGoalProposalTurn === proposalTurn) { + this.activeGoalProposalTurn = undefined; + } + } + if ( + this.config.getGoalProposalHostSupported() && + this.config.setGoalProposalTurnKey(undefined) + ) { + try { + await this.config.getLlmClient().setTools(); + } catch (error) { + debugLogger.warn( + 'Failed to refresh Goal proposal availability', + error, + ); + } + } const stillOwnsPendingPrompt = this.pendingPrompt === pendingSend; releasePendingSend(); const shouldDrainAutomaticQueues = @@ -5242,6 +5312,88 @@ export class Session implements SessionContext { : goalTurnContext.exit(execute); } + async #settleGoalProposal(capture: AgentResponseCapture): Promise { + const turn = capture.goalProposalTurn; + if (!turn) return; + const proposal = this.config.takePendingGoalProposal(turn.turnKey); + if (!proposal) return; + const ownsTurn = () => + turn.completedNormally && + !turn.controller.signal.aborted && + this.pendingPrompt === turn.controller && + this.activeGoalProposalTurn === turn && + !this.disposed && + !this.closing; + if (!ownsTurn()) { + if (!this.disposed && !this.closing) { + await this.messageEmitter.emitAgentMessage( + formatProposeGoalRecoveryNotStarted(proposal.objective), + ); + } + return; + } + try { + const runtime = await this.config.getGoalRuntimeReady(); + if (!ownsTurn()) { + if (!this.disposed && !this.closing) { + await this.messageEmitter.emitAgentMessage( + formatProposeGoalRecoveryNotStarted(proposal.objective), + ); + } + return; + } + const result = await applyPendingGoalProposal(runtime, proposal); + // The automatic queue remains blocked until this prompt releases its + // completion. Cancellation during persistence must pause before then. + if (result.applied && (!ownsTurn() || proposal.approvalSignal?.aborted)) { + try { + await runtime.dispatch({ + action: 'pause', + expectedGoalId: result.goal.goalId, + expectedRevision: result.goal.revision, + reason: + this.closing || this.disposed + ? GOAL_PAUSE_REASON_SESSION_DISPOSED + : GOAL_PAUSE_REASON_USER_INTERRUPT, + }); + } catch (error) { + const current = runtime.getSnapshot().goal; + if ( + current?.goalId === result.goal.goalId && + current.revision === result.goal.revision && + current.status === 'active' + ) { + this.heldGoalProposal = { + goalId: current.goalId, + revision: current.revision, + }; + debugLogger.warn( + 'Failed to pause the cancelled Goal proposal', + error, + ); + await this.messageEmitter.emitAgentMessage( + `The Goal was created, but its cancellation could not be saved. Automatic execution is held in this session. ${GOAL_HELD_RECOVERY_COMMANDS}`, + ); + } + } + } else if (!result.applied) { + debugLogger.debug( + `Dropping an approved Goal proposal: ${result.reason}`, + ); + await this.messageEmitter.emitAgentMessage( + result.kind === 'changed' + ? result.reason + : formatProposeGoalRecoveryFailed(proposal.objective), + ); + } + } catch (error) { + debugLogger.warn('Failed to apply an approved Goal proposal', error); + await this.messageEmitter.emitAgentMessage( + formatProposeGoalRecoveryFailed(proposal.objective), + ); + } + } + async #executePromptInner( params: PromptRequest, pendingSend: AbortController, @@ -5264,6 +5416,26 @@ export class Session implements SessionContext { this.turn += 1; const promptId = this.config.getSessionId() + '########' + this.turn; + if ( + !goalTurn && + !channelTurn && + params._meta?.['qwen.goalProposalApproval'] === true + ) { + responseCapture.goalProposalTurn = { + turnKey: promptId, + controller: pendingSend, + completedNormally: false, + }; + this.activeGoalProposalTurn = responseCapture.goalProposalTurn; + } + if ( + this.config.getGoalProposalHostSupported() && + this.config.setGoalProposalTurnKey( + responseCapture.goalProposalTurn?.turnKey, + ) + ) { + await this.config.getLlmClient().setTools(); + } const daemonPromptId = getInvocationContext()?.promptId; const promptMetadata = (params as { _meta?: Record }) ._meta; @@ -6385,6 +6557,11 @@ export class Session implements SessionContext { modelOverride = model; return true; }; + const blockGoalProposalSettlement = () => { + if (responseCapture?.goalProposalTurn) { + responseCapture.goalProposalTurn.settlementBlocked = true; + } + }; let midTurnContinuationCount = 0; while (true) { @@ -6401,6 +6578,7 @@ export class Session implements SessionContext { } if (this.todoStopGuardQueuedPromptPriority) { + blockGoalProposalSettlement(); return { stopReason: 'end_turn' }; } @@ -6415,6 +6593,7 @@ export class Session implements SessionContext { pendingSend.signal, ); if (claim === 'queued') { + blockGoalProposalSettlement(); this.#preserveUnsentMessageHistory( { role: 'user', parts: drained.parts }, true, @@ -6426,6 +6605,7 @@ export class Session implements SessionContext { } } this.todoStopGuard.acceptMidTurnUserInput(); + blockGoalProposalSettlement(); const continuation = await this.#runStopContinuation( pendingSend, promptId + '_mid_turn_' + ++midTurnContinuationCount, @@ -6453,6 +6633,7 @@ export class Session implements SessionContext { pendingSend.signal, ); if (claim === 'queued') { + blockGoalProposalSettlement(); return { stopReason: 'end_turn' }; } if (claim === 'unavailable') { @@ -6517,6 +6698,7 @@ export class Session implements SessionContext { pendingSend.signal, ); if (claim === 'queued') { + blockGoalProposalSettlement(); this.#preserveUnsentMessageHistory( { role: 'user', parts: drained.parts }, true, @@ -6528,6 +6710,7 @@ export class Session implements SessionContext { } } this.todoStopGuard.acceptMidTurnUserInput(); + blockGoalProposalSettlement(); const continuation = await this.#runStopContinuation( pendingSend, promptId + '_mid_turn_' + ++midTurnContinuationCount, @@ -6555,6 +6738,7 @@ export class Session implements SessionContext { pendingSend.signal, ); if (claim === 'queued') { + blockGoalProposalSettlement(); return { stopReason: 'end_turn' }; } if (claim === 'unavailable') { @@ -6601,7 +6785,10 @@ export class Session implements SessionContext { if (guardDecision?.kind === 'exhausted') { await this.#emitTodoStopGuardExhausted(guardDecision); - if (!externalReason) return { stopReason: 'end_turn' }; + if (!externalReason) { + blockGoalProposalSettlement(); + return { stopReason: 'end_turn' }; + } } if (externalReason && stopHookIterationCount >= stopHookBlockingCap) { @@ -6624,6 +6811,7 @@ export class Session implements SessionContext { await this.#pauseGoalForStopHookCap(); } this.todoStopGuard.suspend(); + blockGoalProposalSettlement(); await this.messageEmitter.emitAgentMessage(warning); debugLogger.warn(warning); return { stopReason: 'end_turn' }; @@ -6726,6 +6914,15 @@ export class Session implements SessionContext { true, ); }; + // A skipped send that yields to a queued user prompt (or to an + // unreliable queue read) is not a normally-finished turn for a parked + // Goal proposal; the loop-level exits already block settlement there. + const blockGoalProposalSettlement = () => { + const proposalTurn = options.responseCapture?.goalProposalTurn; + if (proposalTurn) { + proposalTurn.settlementBlocked = true; + } + }; while (nextMessage !== null) { if (this.pendingPrompt && this.pendingPrompt !== pendingSend) { @@ -6859,6 +7056,7 @@ export class Session implements SessionContext { nextGuardContinuation = undefined; if (!options.externalParts) { preserveGuardOnSkippedSend = true; + blockGoalProposalSettlement(); return { kind: 'stop', stopReason: 'end_turn' }; } if (!initialSend && nextMessage) { @@ -6881,6 +7079,7 @@ export class Session implements SessionContext { guardForThisSend = undefined; nextGuardContinuation = undefined; preserveGuardOnSkippedSend = true; + blockGoalProposalSettlement(); if (initialSend) { supersededAutomaticContinuation = true; } @@ -6957,6 +7156,7 @@ export class Session implements SessionContext { guardForThisSend = undefined; nextGuardContinuation = undefined; preserveGuardOnSkippedSend = true; + blockGoalProposalSettlement(); if (initialSend) { supersededAutomaticContinuation = true; } @@ -8625,6 +8825,13 @@ export class Session implements SessionContext { preserveFallbackOnAbort?: boolean; } = {}, ): Promise { + const proposalTurn = this.activeGoalProposalTurn; + if ( + messages.length > 0 && + proposalTurn?.controller.signal === abortSignal + ) { + proposalTurn.settlementBlocked = true; + } const parts: Part[] = []; for (const message of messages) { const displayText = @@ -12029,6 +12236,24 @@ export class Session implements SessionContext { const entryCancellation = cancelBeforeExecutionIfAborted(toolName); if (entryCancellation) return entryCancellation; + if ( + policyToolName === ToolNames.PROPOSE_GOAL && + (this.activeGoalProposalTurn?.turnKey !== promptId || + this.activeGoalProposalTurn.controller.signal.aborted) + ) { + return earlyErrorResponse( + new Error( + 'The Goal was not set: this turn cannot own a Goal approval. Hand the user a `/goal set ` line instead.', + ), + toolName, + { + status: 'error', + errorType: ToolErrorType.EXECUTION_DENIED, + executionStatus: 'not_started', + }, + ); + } + // ---- L1: Tool enablement check ---- const isTrustedLiveScreenContextTool = tool === this.liveScreenContextTool; diff --git a/packages/cli/src/acp-integration/session/Session.worktree.test.ts b/packages/cli/src/acp-integration/session/Session.worktree.test.ts index 679adba1743..7278dad5adb 100644 --- a/packages/cli/src/acp-integration/session/Session.worktree.test.ts +++ b/packages/cli/src/acp-integration/session/Session.worktree.test.ts @@ -200,6 +200,7 @@ describe('Session.pendingWorktreeNotice', () => { }), setSubSessionSpawner: vi.fn(), getSubSessionSpawner: vi.fn(), + getGoalProposalHostSupported: vi.fn().mockReturnValue(false), // The Session constructor and Session.prompt both reach for the // canonical Goal runtime. A real Config throws this exact error when // Goal persistence is off, and both call sites are written to fall diff --git a/packages/cli/src/acp-integration/session/permissionUtils.test.ts b/packages/cli/src/acp-integration/session/permissionUtils.test.ts index 459272569ec..5e200ccd5b5 100644 --- a/packages/cli/src/acp-integration/session/permissionUtils.test.ts +++ b/packages/cli/src/acp-integration/session/permissionUtils.test.ts @@ -273,6 +273,30 @@ describe('permissionUtils', () => { }); }); + it('forwards the complete plain-text Goal approval prompt', () => { + const prompt = 'Replace the paused Goal?\n\nOutcome: fix **all** tests.'; + expect( + buildPermissionRequestContent({ + type: 'info', + title: 'Set this as the session Goal?', + prompt, + renderPromptAsPlainText: true, + onConfirm: async () => undefined, + }), + ).toEqual([{ type: 'content', content: { type: 'text', text: prompt } }]); + }); + + it('preserves the existing content contract for other info approvals', () => { + expect( + buildPermissionRequestContent({ + type: 'info', + title: 'Confirm', + prompt: 'Existing informational prompt', + onConfirm: async () => undefined, + }), + ).toEqual([]); + }); + it('places warnings before edit diff content', () => { const content = buildPermissionRequestContent({ type: 'edit', diff --git a/packages/cli/src/acp-integration/session/permissionUtils.ts b/packages/cli/src/acp-integration/session/permissionUtils.ts index d25489ef0f7..6bd4393dfb3 100644 --- a/packages/cli/src/acp-integration/session/permissionUtils.ts +++ b/packages/cli/src/acp-integration/session/permissionUtils.ts @@ -177,6 +177,13 @@ export function buildPermissionRequestContent( }); } + if (confirmation.type === 'info' && confirmation.renderPromptAsPlainText) { + content.push({ + type: 'content', + content: { type: 'text', text: confirmation.prompt }, + }); + } + return content; } diff --git a/packages/cli/src/commands/review/ab-drive.test.ts b/packages/cli/src/commands/review/ab-drive.test.ts index bf810ba7baa..13e379f4cba 100644 --- a/packages/cli/src/commands/review/ab-drive.test.ts +++ b/packages/cli/src/commands/review/ab-drive.test.ts @@ -19,6 +19,7 @@ import { createHash } from 'node:crypto'; import { spawnSync } from 'node:child_process'; import { chmodSync, + existsSync, mkdirSync, mkdtempSync, readdirSync, @@ -573,30 +574,82 @@ describe('runAbDrive, harnessed', () => { it('drives both arms with one script and pairs the captures', () => { const args = baseArgs({}); + // Count inside a PRIVATE TMPDIR, not the machine-wide one: another job's + // ab-drive run (the review pipeline drives it for real on shared runners) + // landing a run dir between the two reads is indistinguishable from a + // leak this run made. On Windows TMPDIR is a no-op and the count reads + // the global temp dir as before (same precedent as the TMPDIR test above). + const savedTmp = process.env['TMPDIR']; + process.env['TMPDIR'] = tempDir('ab-privtmp-'); const runDirs = () => readdirSync(tmpdir()).filter((d) => d.startsWith('qwen-review-ab-drive-')) .length; - const before = runDirs(); - const h = harness({ server: args.server }); - const r = runAbDrive({ ...args, exec: h.exec }); - expect(r.observed).toBe(true); - expect(r.a?.outcome).toBe('completed'); - expect(r.b?.outcome).toBe('completed'); - expect(r.a?.output).toContain('arm-a output'); - expect(r.b?.output).toContain('arm-b output'); - expect(r.identicalOutput).toBe(false); - // The digest is the same-bytes fact a witness quotes — pinned to the - // script's actual bytes, not merely to being 64 hex characters. - expect(r.scriptSha256).toBe( - createHash('sha256').update('true').digest('hex'), - ); - // Cleanup is unconditional and last — a leaked server is the next run's - // wrong observation. - expect(h.log.at(-1)).toEqual(['tmux', '-L', args.server, 'kill-server']); - // The mkdtemp'd run dir is swept on the success path too. - expect(runDirs()).toBe(before); + try { + const before = runDirs(); + const h = harness({ server: args.server }); + const r = runAbDrive({ ...args, exec: h.exec }); + expect(r.observed).toBe(true); + expect(r.a?.outcome).toBe('completed'); + expect(r.b?.outcome).toBe('completed'); + expect(r.a?.output).toContain('arm-a output'); + expect(r.b?.output).toContain('arm-b output'); + expect(r.identicalOutput).toBe(false); + // The digest is the same-bytes fact a witness quotes — pinned to the + // script's actual bytes, not merely to being 64 hex characters. + expect(r.scriptSha256).toBe( + createHash('sha256').update('true').digest('hex'), + ); + // Cleanup is unconditional and last — a leaked server is the next run's + // wrong observation. + expect(h.log.at(-1)).toEqual(['tmux', '-L', args.server, 'kill-server']); + // The mkdtemp'd run dir is swept on the success path too. + expect(runDirs()).toBe(before); + } finally { + if (savedTmp === undefined) delete process.env['TMPDIR']; + else process.env['TMPDIR'] = savedTmp; + } }); + it.skipIf(process.platform === 'win32')( + 'the sweep count is hermetic — a foreign run dir in the shared temp dir does not move it', + () => { + // Pins the private-TMPDIR redirect above: another job's ab-drive run + // dir appearing mid-run in the MACHINE-WIDE temp dir must not register. + // Without the redirect this fails as `expected 1 to be +0` — the exact + // assertion signature a foreign run dir produced on a shared runner. + const sharedTmp = tmpdir(); + const args = baseArgs({}); + let foreignDir = ''; + const h = harness({ + server: args.server, + onSession: (name) => { + if (name === 'hold' && foreignDir === '') { + foreignDir = mkdtempSync(join(sharedTmp, 'qwen-review-ab-drive-')); + } + }, + }); + const savedTmp = process.env['TMPDIR']; + process.env['TMPDIR'] = tempDir('ab-privtmp-'); + const runDirs = () => + readdirSync(tmpdir()).filter((d) => + d.startsWith('qwen-review-ab-drive-'), + ).length; + try { + const before = runDirs(); + const r = runAbDrive({ ...args, exec: h.exec }); + expect(r.observed).toBe(true); + // The pollution really landed inside the measured window. + expect(existsSync(foreignDir)).toBe(true); + expect(runDirs()).toBe(before); + } finally { + if (savedTmp === undefined) delete process.env['TMPDIR']; + else process.env['TMPDIR'] = savedTmp; + if (foreignDir !== '') + rmSync(foreignDir, { recursive: true, force: true }); + } + }, + ); + it('carries each arm script’s own exit code — never a fabricated zero', () => { // The success note quotes `a: exit N`, and a verifier quotes the note; // an exit-code-discarding regression would write "exit 0" for a script @@ -1060,29 +1113,38 @@ describe.skipIf(!hasTmux || process.platform === 'win32')( }); it('an upstream that exits at birth is a confound, not a comparison — and still cleans up', () => { - const before = readdirSync(tmpdir()).filter((d) => - d.startsWith('qwen-review-ab-drive-'), - ).length; - const r = runAbDrive({ - shared: 'true', - script: 'sleep 1; echo watched-nothing', - armA: tempDir('ab-real-da-'), - armB: tempDir('ab-real-db-'), - readyTimeout: 5, - timeout: 30, - sharedReadyTimeout: 5, - sharedOnce: false, - server: `abreald-${process.pid}`, - }); - expect(r.a?.outcome).toBe('completed'); - expect(r.a?.sharedAliveAtEnd).toBe(false); - expect(r.observed).toBe(false); - expect(r.note).toContain('died'); - // The mkdtemp'd run dir is swept on this failing path too. - const after = readdirSync(tmpdir()).filter((d) => - d.startsWith('qwen-review-ab-drive-'), - ).length; - expect(after).toBe(before); + // Private TMPDIR for the same reason as the harnessed sweep count + // above — and this drive runs for seconds against real tmux, a far + // wider window for another job's run dir to land mid-measurement. + const savedTmp = process.env['TMPDIR']; + process.env['TMPDIR'] = tempDir('ab-privtmp-'); + const runDirs = () => + readdirSync(tmpdir()).filter((d) => + d.startsWith('qwen-review-ab-drive-'), + ).length; + try { + const before = runDirs(); + const r = runAbDrive({ + shared: 'true', + script: 'sleep 1; echo watched-nothing', + armA: tempDir('ab-real-da-'), + armB: tempDir('ab-real-db-'), + readyTimeout: 5, + timeout: 30, + sharedReadyTimeout: 5, + sharedOnce: false, + server: `abreald-${process.pid}`, + }); + expect(r.a?.outcome).toBe('completed'); + expect(r.a?.sharedAliveAtEnd).toBe(false); + expect(r.observed).toBe(false); + expect(r.note).toContain('died'); + // The mkdtemp'd run dir is swept on this failing path too. + expect(runDirs()).toBe(before); + } finally { + if (savedTmp === undefined) delete process.env['TMPDIR']; + else process.env['TMPDIR'] = savedTmp; + } }); it('an arm path that is a FILE is refused with the directory message', () => { diff --git a/packages/cli/src/ui/hooks/use-llm-stream.test.tsx b/packages/cli/src/ui/hooks/use-llm-stream.test.tsx index fe4b65ccacf..86b2065b46c 100644 --- a/packages/cli/src/ui/hooks/use-llm-stream.test.tsx +++ b/packages/cli/src/ui/hooks/use-llm-stream.test.tsx @@ -19996,6 +19996,37 @@ describe('useLlmStream', () => { }); describe('HookSystemMessage Event', () => { + it('shows Goal settlement failures as warnings without Stop attribution', async () => { + mockSendMessageStream.mockReturnValue( + (async function* () { + yield { + type: ServerLlmEventType.GoalSettlementFailed, + value: 'The approved Goal could not be started.', + }; + })(), + ); + + const { result } = renderTestHook(); + + await act(async () => { + await result.current.submitQuery('set a goal'); + }); + + await waitFor(() => { + expect(mockAddItem).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'warning', + text: 'The approved Goal could not be started.', + }), + expect.any(Number), + ); + }); + expect(mockAddItem).not.toHaveBeenCalledWith( + expect.objectContaining({ type: 'stop_hook_system_message' }), + expect.any(Number), + ); + }); + it('commits staged inline content and restarts after a displayed Goal state', async () => { const image = { data: 'aW1hZ2U=', diff --git a/packages/cli/src/ui/hooks/use-llm-stream.ts b/packages/cli/src/ui/hooks/use-llm-stream.ts index 4d109aec3c7..995fc4f313a 100644 --- a/packages/cli/src/ui/hooks/use-llm-stream.ts +++ b/packages/cli/src/ui/hooks/use-llm-stream.ts @@ -3007,6 +3007,22 @@ export const useLlmStream = ( llmMessageBuffer = ''; assistantOutputStarted = false; break; + case ServerLlmEventType.GoalSettlementFailed: + flushBufferedStreamEvents(); + if (pendingHistoryItemRef.current) { + commitItemInOrder( + pendingHistoryItemRef.current, + userMessageTimestamp, + ); + setPendingHistoryItem(null); + } + addItem( + { type: 'warning', text: event.value }, + userMessageTimestamp, + ); + llmMessageBuffer = ''; + assistantOutputStarted = false; + break; case ServerLlmEventType.UserPromptSubmitBlocked: flushBufferedStreamEvents(); userPromptBlocked = true; diff --git a/packages/cli/src/ui/opentui/event-adapter.test.ts b/packages/cli/src/ui/opentui/event-adapter.test.ts index c2c8bce52bc..d4aa7d51765 100644 --- a/packages/cli/src/ui/opentui/event-adapter.test.ts +++ b/packages/cli/src/ui/opentui/event-adapter.test.ts @@ -465,6 +465,20 @@ describe('event-adapter (ServerGeminiStreamEvent -> neutral)', () => { ]); }); + it('maps goal_settlement_failed to a warning', () => { + const map = createEventMapper(); + const out = map({ + type: 'goal_settlement_failed', + value: 'The approved Goal could not be started.', + } as unknown as AnyEv); + expect(out).toEqual([ + { + type: 'warning', + text: 'The approved Goal could not be started.', + }, + ]); + }); + it('maps user_prompt_submit_blocked to reason + original prompt', () => { const map = createEventMapper(); const out = map({ diff --git a/packages/cli/src/ui/opentui/event-adapter.ts b/packages/cli/src/ui/opentui/event-adapter.ts index d36dfde8d0c..c795fada091 100644 --- a/packages/cli/src/ui/opentui/event-adapter.ts +++ b/packages/cli/src/ui/opentui/event-adapter.ts @@ -711,6 +711,11 @@ export function createEventMapper( out.push({ type: 'stop-hook-message', message: ev.value as string }); break; } + case 'goal_settlement_failed': { + closeThought(); + out.push({ type: 'warning', text: ev.value as string }); + break; + } case 'user_prompt_submit_blocked': { closeThought(); const v = ev.value as { reason: string; originalPrompt: string }; diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index bea7d53c208..bf0768a7792 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -3343,12 +3343,14 @@ describe('Server Config (config.ts)', () => { config.setPendingGoalProposal({ objective: 'first', turnKey: 'turn-1', + reviewedGoal: null, }), ).toBe(true); expect( config.setPendingGoalProposal({ objective: 'second', turnKey: 'turn-1', + reviewedGoal: null, }), ).toBe(false); expect(config.hasPendingGoalProposal()).toBe(true); @@ -3357,6 +3359,8 @@ describe('Server Config (config.ts)', () => { expect(config.takePendingGoalProposal('turn-1')).toEqual({ objective: 'first', turnKey: 'turn-1', + reviewedGoal: null, + approvalSignal: expect.any(AbortSignal), }); expect(config.hasPendingGoalProposal()).toBe(false); expect(config.takePendingGoalProposal()).toBeUndefined(); @@ -3365,11 +3369,14 @@ describe('Server Config (config.ts)', () => { config.setPendingGoalProposal({ objective: 'explicitly cleared', turnKey: 'turn-3', + reviewedGoal: null, }), ).toBe(true); expect(config.takePendingGoalProposal()).toEqual({ objective: 'explicitly cleared', turnKey: 'turn-3', + reviewedGoal: null, + approvalSignal: expect.any(AbortSignal), }); expect(config.hasPendingGoalProposal()).toBe(false); }); @@ -3379,6 +3386,7 @@ describe('Server Config (config.ts)', () => { config.setPendingGoalProposal({ objective: 'stale approval', turnKey: 'turn-1', + reviewedGoal: null, }); config.startNewSession('replacement-session'); @@ -5678,6 +5686,24 @@ describe('Server Config (config.ts)', () => { expect(registeredNames).not.toContain(ToolNames.PROPOSE_GOAL); }, ); + it.each(['alwaysAsk', 'disabled'] as const)( + 'honors %s for an ACP host with explicit Goal proposal support', + async (modelProposedGoals) => { + const config = new Config({ + ...baseParams, + experimentalZedIntegration: true, + modelProposedGoals, + }); + config.setGoalProposalHostSupported(true); + await config.initialize(); + const registeredNames = ( + ToolRegistry.prototype.registerFactory as Mock + ).mock.calls.map((call) => call[0]); + expect(registeredNames.includes(ToolNames.PROPOSE_GOAL)).toBe( + modelProposedGoals === 'alwaysAsk', + ); + }, + ); it('does not register propose_goal when goals.modelProposed is disabled', async () => { const config = new Config({ ...baseParams, diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index ae3662de0ae..c01df93ed3c 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -2382,6 +2382,7 @@ export class Config { private goalRuntimeReady: Promise | undefined; /** A `propose_goal` approval waiting for its turn to end; see PendingGoalProposal. */ private pendingGoalProposal: PendingGoalProposal | undefined; + private goalProposalApprovalController: AbortController | undefined; /** * A Goal restore held back because the session writer is not accepting * writes yet. Settled by {@link startPendingGoalRestore} once the @@ -2471,6 +2472,8 @@ export class Config { */ private externalAgentExecutor?: ExternalAgentExecutor; private readonly modelProposedGoals: ModelProposedGoalsMode; + private goalProposalHostSupported = false; + private goalProposalTurnKey: string | undefined; private readonly skipWorkflowUsageWarning: boolean = false; private readonly emitToolUseSummaries: boolean = true; private readonly chatRecordingEnabled: boolean; @@ -7329,6 +7332,9 @@ export class Config { this.manualPlanExitNoticeEventState = noticeEvent; } if (mode === ApprovalMode.PLAN && fromMode !== ApprovalMode.PLAN) { + if (!isDerivedConfig(this)) { + this.goalProposalApprovalController?.abort(); + } this.prePlanMode = fromMode; noticeEvent.version++; noticeEvent.kind = 'clear'; @@ -8137,6 +8143,27 @@ export class Config { return this.modelProposedGoals; } + setGoalProposalHostSupported(supported: boolean): void { + this.goalProposalHostSupported = supported; + } + + getGoalProposalHostSupported(): boolean { + return this.goalProposalHostSupported; + } + + setGoalProposalTurnKey(turnKey: string | undefined): boolean { + if (this.goalProposalTurnKey === turnKey) return false; + this.goalProposalTurnKey = turnKey; + return true; + } + + isGoalProposalAvailable(): boolean { + return ( + resolveInteractionMode(this) === 'interactive' || + (this.goalProposalHostSupported && this.goalProposalTurnKey !== undefined) + ); + } + hasPendingGoalProposal(): boolean { return this.pendingGoalProposal !== undefined; } @@ -8144,7 +8171,14 @@ export class Config { /** Parks a `propose_goal` approval until the proposing turn ends. */ setPendingGoalProposal(proposal: PendingGoalProposal): boolean { if (this.pendingGoalProposal) return false; - this.pendingGoalProposal = proposal; + this.goalProposalApprovalController = new AbortController(); + if (this.approvalMode === ApprovalMode.PLAN) { + this.goalProposalApprovalController.abort(); + } + this.pendingGoalProposal = { + ...proposal, + approvalSignal: this.goalProposalApprovalController.signal, + }; return true; } @@ -8985,6 +9019,8 @@ export class Config { ), ); // An approval belongs to the session that produced it. + this.goalProposalApprovalController?.abort(); + this.goalProposalApprovalController = undefined; this.pendingGoalProposal = undefined; if (!this.chatRecordingService) { this.goalRuntime = undefined; @@ -9715,7 +9751,8 @@ export class Config { // keep the text hand-off (`/goal set …`) that /goal-draft prints. if ( this.getModelProposedGoals() !== 'disabled' && - resolveInteractionMode(this) === 'interactive' + (resolveInteractionMode(this) === 'interactive' || + this.goalProposalHostSupported) ) { await registerLazy(ToolNames.PROPOSE_GOAL, async () => { const { ProposeGoalTool } = await import('../goals/goal-tools.js'); diff --git a/packages/core/src/core/client-goal-plan-switch.test.ts b/packages/core/src/core/client-goal-plan-switch.test.ts new file mode 100644 index 00000000000..3bf26f3a252 --- /dev/null +++ b/packages/core/src/core/client-goal-plan-switch.test.ts @@ -0,0 +1,157 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { mkdtemp, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { + Config, + ApprovalMode, + deriveApprovalModeConfig, +} from '../config/config.js'; +import { createGoalRuntime } from '../goals/goal-runtime.js'; +import { LlmClient } from './client.js'; + +describe('approved Goal proposals and Plan mode', () => { + const directories: string[] = []; + + afterEach(async () => { + await Promise.all( + directories + .splice(0) + .map((directory) => rm(directory, { recursive: true, force: true })), + ); + }); + + async function setup() { + const directory = await mkdtemp(path.join(tmpdir(), 'goal-plan-switch-')); + directories.push(directory); + const config = new Config({ + cwd: directory, + targetDir: directory, + model: 'test-model', + approvalMode: ApprovalMode.DEFAULT, + debugMode: false, + chatRecording: false, + usageStatisticsEnabled: false, + overrideExtensions: [], + }); + const recordGoalState = vi.fn(async (): Promise => undefined); + const runtime = createGoalRuntime({ + journal: { + getTranscriptCursor: () => ({ recordId: 'test-cursor' }), + recordGoalState, + }, + }); + const client = new LlmClient(config); + config.setPendingGoalProposal({ + objective: 'Verify the approved objective.', + turnKey: 'approved-turn', + reviewedGoal: null, + }); + const reportFailure = vi.fn(); + const settle = (loadRuntime = async () => runtime) => + client['settlePendingGoalProposal']( + true, + new AbortController().signal, + loadRuntime, + 'approved-turn', + reportFailure, + ); + return { config, runtime, settle, recordGoalState, reportFailure }; + } + + it('does not create a Goal after the user enters Plan mode before settlement', async () => { + const { config, runtime, settle, reportFailure } = await setup(); + config.setApprovalMode(ApprovalMode.PLAN); + await settle(); + expect(runtime.getSnapshot().goal).toBeNull(); + expect(reportFailure).toHaveBeenCalledWith( + expect.stringContaining('approval was revoked'), + ); + }); + + it('does not revive the approval after leaving Plan mode', async () => { + const { config, runtime, settle } = await setup(); + config.setApprovalMode(ApprovalMode.PLAN); + config.setApprovalMode(ApprovalMode.DEFAULT); + await settle(); + expect(runtime.getSnapshot().goal).toBeNull(); + }); + + it('does not create a Goal when Plan mode begins while its runtime loads', async () => { + const { config, runtime, settle } = await setup(); + await settle(async () => { + config.setApprovalMode(ApprovalMode.PLAN); + return runtime; + }); + expect(runtime.getSnapshot().goal).toBeNull(); + }); + + it('creates an ordinary approved Goal exactly once', async () => { + const { runtime, settle } = await setup(); + const dispatch = vi.spyOn(runtime, 'dispatch'); + await settle(); + await settle(); + expect(runtime.getSnapshot().goal).toMatchObject({ + objective: 'Verify the approved objective.', + status: 'active', + }); + expect(dispatch).toHaveBeenCalledExactlyOnceWith({ + action: 'create', + objective: 'Verify the approved objective.', + }); + }); + + it('pauses creation if Plan mode begins while the Goal is being saved', async () => { + const { config, runtime, settle, recordGoalState } = await setup(); + recordGoalState.mockImplementationOnce(async () => { + config.setApprovalMode(ApprovalMode.PLAN); + }); + await settle(); + expect(runtime.getSnapshot().goal?.status).toBe('paused'); + }); + + it('permits a newly approved proposal after leaving Plan mode', async () => { + const { config, runtime, settle } = await setup(); + config.setApprovalMode(ApprovalMode.PLAN); + await settle(); + config.setApprovalMode(ApprovalMode.DEFAULT); + config.setPendingGoalProposal({ + objective: 'A newly approved objective.', + turnKey: 'approved-turn', + reviewedGoal: null, + }); + await settle(); + expect(runtime.getSnapshot().goal).toMatchObject({ + objective: 'A newly approved objective.', + status: 'active', + }); + }); + + it('rejects approval that reaches the store after Plan mode was entered', async () => { + const { config, runtime, settle } = await setup(); + config.takePendingGoalProposal(); + config.setApprovalMode(ApprovalMode.PLAN); + config.setPendingGoalProposal({ + objective: 'Approval delivered too late.', + turnKey: 'approved-turn', + reviewedGoal: null, + }); + await settle(); + expect(runtime.getSnapshot().goal).toBeNull(); + }); + + it('keeps the parent approval when a derived agent enters Plan mode', async () => { + const { config, runtime, settle } = await setup(); + const child = deriveApprovalModeConfig(config, ApprovalMode.DEFAULT); + child.config.setApprovalMode(ApprovalMode.PLAN); + await settle(); + expect(runtime.getSnapshot().goal?.status).toBe('active'); + child.cleanup(); + }); +}); diff --git a/packages/core/src/core/client-goal.test.ts b/packages/core/src/core/client-goal.test.ts index 31e64a7618a..bfbb653cbad 100644 --- a/packages/core/src/core/client-goal.test.ts +++ b/packages/core/src/core/client-goal.test.ts @@ -259,6 +259,7 @@ function setupGoalClient() { toolResultsNumToKeep: 5, })), getApprovalMode: vi.fn(() => ApprovalMode.DEFAULT), + getGoalProposalHostSupported: vi.fn(() => false), getSystemPrompt: vi.fn(() => undefined), getOutputStyle: vi.fn(() => undefined), getExperimentalZedIntegration: vi.fn(() => false), @@ -304,7 +305,11 @@ describe('LlmClient Goal admission', () => { const takePendingGoalProposal = vi .fn() .mockReturnValueOnce(undefined) // the new-query discard - .mockReturnValueOnce({ objective: 'ship it', turnKey: 'real-user-key' }); + .mockReturnValueOnce({ + objective: 'ship it', + turnKey: 'real-user-key', + reviewedGoal: null, + }); Object.assign(config, { takePendingGoalProposal, getUsageStatisticsEnabled: vi.fn(() => false), @@ -326,6 +331,160 @@ describe('LlmClient Goal admission', () => { }); }); + it('leaves a host-owned approved proposal for Session settlement', async () => { + const { client, config, runtime } = setupGoalClient(); + vi.mocked(config.getGoalProposalHostSupported).mockReturnValue(true); + const store = pendingGoalProposalStore({ + objective: 'ship it', + turnKey: 'host-owned-key', + reviewedGoal: null, + }); + const loadGoalRuntime = vi.fn(async () => runtime); + Object.assign(config, { takePendingGoalProposal: store.take }); + + await client['settlePendingGoalProposal']( + true, + new AbortController().signal, + loadGoalRuntime, + 'host-owned-key', + vi.fn(), + ); + + expect(store.get()).toEqual({ + objective: 'ship it', + turnKey: 'host-owned-key', + reviewedGoal: null, + }); + expect(store.take).not.toHaveBeenCalled(); + expect(loadGoalRuntime).not.toHaveBeenCalled(); + expect(runtime.dispatch).not.toHaveBeenCalled(); + expect(store.take('host-owned-key')).toEqual({ + objective: 'ship it', + turnKey: 'host-owned-key', + reviewedGoal: null, + }); + }); + + it('reports recovery when the core-owned proposal runtime is unavailable', async () => { + const { client, config } = setupGoalClient(); + const store = pendingGoalProposalStore({ + objective: 'ship it', + turnKey: 'runtime-unavailable-key', + reviewedGoal: null, + }); + const reportFailure = vi.fn(); + Object.assign(config, { takePendingGoalProposal: store.take }); + + await client['settlePendingGoalProposal']( + true, + new AbortController().signal, + async () => undefined, + 'runtime-unavailable-key', + reportFailure, + ); + + expect(store.get()).toBeUndefined(); + expect(reportFailure).toHaveBeenCalledWith( + 'The approved Goal could not be started. Check the Goal status before trying again, or run:\n/goal set ship it', + ); + }); + + it('reports a proposal that could not be applied to the terminal user', async () => { + const { client, config, runtime } = setupGoalClient(); + vi.mocked(config.getSkipNextSpeakerCheck).mockReturnValue(true); + vi.mocked(runtime.getSnapshot).mockReturnValue({ + v: 2, + activity: 'idle', + goal: null, + }); + vi.mocked(runtime.dispatch).mockResolvedValueOnce({ + snapshot: { v: 2, activity: 'idle', goal: null }, + }); + const store = pendingGoalProposalStore(); + Object.assign(config, { + takePendingGoalProposal: store.take, + getUsageStatisticsEnabled: vi.fn(() => false), + }); + turnMocks.run.mockImplementationOnce(() => { + store.set({ + objective: 'ship it', + turnKey: 'failed-settlement-key', + reviewedGoal: null, + }); + return emptyStream(); + }); + + const events = await collect( + client.sendMessageStream( + [{ text: 'set a goal for this' }], + new AbortController().signal, + 'failed-settlement-key', + { type: SendMessageType.UserQuery }, + ), + ); + + expect(events).toContainEqual({ + type: LlmEventType.GoalSettlementFailed, + value: + 'The approved Goal could not be started. Check the Goal status before trying again, or run:\n/goal set ship it', + }); + }); + + it('does not suggest replacing a Goal that changed after approval', async () => { + const { client, config, runtime } = setupGoalClient(); + vi.mocked(config.getSkipNextSpeakerCheck).mockReturnValue(true); + vi.mocked(runtime.getSnapshot).mockReturnValue({ + v: 2, + activity: 'idle', + goal: { + goalId: 'newer-goal', + revision: 2, + objective: 'newer objective', + status: 'paused', + evidenceCursor: { recordId: 'newer-record' }, + turnCount: 0, + activeTimeMs: 0, + tokensUsed: 0, + createdAt: 1, + updatedAt: 2, + }, + }); + const store = pendingGoalProposalStore(); + Object.assign(config, { + takePendingGoalProposal: store.take, + getUsageStatisticsEnabled: vi.fn(() => false), + }); + turnMocks.run.mockImplementationOnce(() => { + store.set({ + objective: 'stale objective', + turnKey: 'changed-goal-key', + reviewedGoal: null, + }); + return emptyStream(); + }); + + const events = await collect( + client.sendMessageStream( + [{ text: 'set a goal for this' }], + new AbortController().signal, + 'changed-goal-key', + { type: SendMessageType.UserQuery }, + ), + ); + const failure = events.find( + (event) => + (event as { type?: LlmEventType }).type === + LlmEventType.GoalSettlementFailed, + ) as Extract< + ServerLlmStreamEvent, + { type: LlmEventType.GoalSettlementFailed } + >; + + expect(failure.value).toContain('Goal changed after the proposal'); + expect(failure.value).not.toContain('/goal set'); + expect(runtime.dispatch).not.toHaveBeenCalled(); + }); + it('settles an approved proposal on the default skip-next-speaker exit', async () => { const { client, config, runtime } = setupGoalClient(); vi.mocked(runtime.getSnapshot).mockReturnValue({ @@ -333,7 +492,7 @@ describe('LlmClient Goal admission', () => { activity: 'idle', goal: null, }); - let pending: { objective: string; turnKey: string } | undefined; + let pending: PendingGoalProposal | undefined; const takePendingGoalProposal = vi.fn(() => { const proposal = pending; pending = undefined; @@ -345,7 +504,11 @@ describe('LlmClient Goal admission', () => { getUsageStatisticsEnabled: vi.fn(() => false), }); turnMocks.run.mockImplementationOnce(() => { - pending = { objective: 'ship it', turnKey: 'default-exit-key' }; + pending = { + objective: 'ship it', + turnKey: 'default-exit-key', + reviewedGoal: null, + }; return emptyStream(); }); @@ -372,7 +535,7 @@ describe('LlmClient Goal admission', () => { activity: 'idle', goal: null, }); - let pending: { objective: string; turnKey: string } | undefined; + let pending: PendingGoalProposal | undefined; const takePendingGoalProposal = vi.fn(() => { const proposal = pending; pending = undefined; @@ -392,7 +555,11 @@ describe('LlmClient Goal admission', () => { getUsageStatisticsEnabled: vi.fn(() => false), }); turnMocks.run.mockImplementationOnce(() => { - pending = { objective: 'ship it', turnKey: 'stop-cap-key' }; + pending = { + objective: 'ship it', + turnKey: 'stop-cap-key', + reviewedGoal: null, + }; return emptyStream(); }); @@ -411,13 +578,63 @@ describe('LlmClient Goal admission', () => { }); }); + it('reports a failed proposal settlement from the Stop-hook-cap exit', async () => { + const { client, config, runtime } = setupGoalClient(); + vi.mocked(runtime.getSnapshot).mockReturnValue({ + v: 2, + activity: 'idle', + goal: null, + }); + vi.mocked(runtime.dispatch).mockResolvedValueOnce({ + snapshot: { v: 2, activity: 'idle', goal: null }, + }); + const store = pendingGoalProposalStore(); + Object.assign(config, { + takePendingGoalProposal: store.take, + getDisableAllHooks: vi.fn(() => false), + hasHooksForEvent: vi.fn((event) => event === 'Stop'), + getMessageBus: vi.fn(() => ({ + request: vi.fn(async () => ({ + output: { decision: 'block', reason: 'Keep working' }, + stopHookCount: 1, + })), + })), + getStopHookBlockingCap: vi.fn(() => 1), + getUsageStatisticsEnabled: vi.fn(() => false), + }); + turnMocks.run.mockImplementationOnce(() => { + store.set({ + objective: 'ship it', + turnKey: 'stop-cap-failure-key', + reviewedGoal: null, + }); + return emptyStream(); + }); + + const events = await collect( + client.sendMessageStream( + [{ text: 'set a goal for this' }], + new AbortController().signal, + 'stop-cap-failure-key', + { type: SendMessageType.UserQuery }, + ), + ); + + expect(events).toContainEqual({ + type: LlmEventType.GoalSettlementFailed, + value: + 'The approved Goal could not be started. Check the Goal status before trying again, or run:\n/goal set ship it', + }); + }); + it('discards an approved proposal when settlement starts already aborted', async () => { const { client, config, runtime } = setupGoalClient(); const controller = new AbortController(); controller.abort(); - let pending: { objective: string; turnKey: string } | undefined = { + let pending: PendingGoalProposal | undefined = { objective: 'ship it', turnKey: 'settle-key', + reviewedGoal: null, }; const loadGoalRuntime = vi.fn(async () => runtime); Object.assign(config, { @@ -433,6 +650,7 @@ describe('LlmClient Goal admission', () => { controller.signal, loadGoalRuntime, 'settle-key', + vi.fn(), ); expect(pending).toBeUndefined(); @@ -448,7 +666,7 @@ describe('LlmClient Goal admission', () => { activity: 'idle', goal: null, }); - let pending: { objective: string; turnKey: string } | undefined; + let pending: PendingGoalProposal | undefined; const takePendingGoalProposal = vi.fn(() => { const proposal = pending; pending = undefined; @@ -462,7 +680,11 @@ describe('LlmClient Goal admission', () => { turnMocks.pendingToolCalls.push([{ name: 'read_file' }], []); turnMocks.run .mockImplementationOnce(() => { - pending = { objective: 'ship it', turnKey: 'pending-tool-key' }; + pending = { + objective: 'ship it', + turnKey: 'pending-tool-key', + reviewedGoal: null, + }; return emptyStream(); }) .mockImplementation(emptyStream); @@ -481,6 +703,7 @@ describe('LlmClient Goal admission', () => { expect(pending).toEqual({ objective: 'ship it', turnKey: 'pending-tool-key', + reviewedGoal: null, }); await drain( @@ -506,11 +729,55 @@ describe('LlmClient Goal admission', () => { }); }); + it('reports a failed proposal settlement from the ToolResult exit', async () => { + const { client, config, runtime } = setupGoalClient(); + nextSpeakerMocks.check.mockResolvedValue({ next_speaker: 'user' }); + vi.mocked(runtime.getSnapshot).mockReturnValue({ + v: 2, + activity: 'idle', + goal: null, + }); + vi.mocked(runtime.dispatch).mockResolvedValueOnce({ + snapshot: { v: 2, activity: 'idle', goal: null }, + }); + const store = pendingGoalProposalStore({ + objective: 'ship it', + turnKey: 'tool-result-failure-key', + reviewedGoal: null, + }); + Object.assign(config, { + takePendingGoalProposal: store.take, + getUsageStatisticsEnabled: vi.fn(() => false), + }); + + const events = await collect( + client.sendMessageStream( + [ + { + functionResponse: { + name: 'read_file', + response: { output: 'ok' }, + }, + }, + ], + new AbortController().signal, + 'tool-result-failure-key', + { type: SendMessageType.ToolResult }, + ), + ); + + expect(events).toContainEqual({ + type: LlmEventType.GoalSettlementFailed, + value: + 'The approved Goal could not be started. Check the Goal status before trying again, or run:\n/goal set ship it', + }); + }); + it('keeps an approved proposal parked through a queued steer continuation', async () => { const { client, config, runtime } = setupGoalClient(); vi.mocked(config.getMaxSessionTurns).mockReturnValue(0); nextSpeakerMocks.check.mockResolvedValue({ next_speaker: 'user' }); - let pending: { objective: string; turnKey: string } | undefined; + let pending: PendingGoalProposal | undefined; const takePendingGoalProposal = vi.fn(() => { const proposal = pending; pending = undefined; @@ -547,7 +814,11 @@ describe('LlmClient Goal admission', () => { }); turnMocks.run .mockImplementationOnce(() => { - pending = { objective: 'ship it', turnKey: 'real-user-key' }; + pending = { + objective: 'ship it', + turnKey: 'real-user-key', + reviewedGoal: null, + }; return emptyStream(); }) .mockImplementationOnce(() => { @@ -599,6 +870,7 @@ describe('LlmClient Goal admission', () => { store.set({ objective: 'stale proposal', turnKey: 'failed-user-key', + reviewedGoal: null, }); yield { type: LlmEventType.Error, @@ -638,9 +910,10 @@ describe('LlmClient Goal admission', () => { goal: null, }); const controller = new AbortController(); - let pending: { objective: string; turnKey: string } | undefined = { + let pending: PendingGoalProposal | undefined = { objective: 'ship it', turnKey: 'settle-key', + reviewedGoal: null, }; const takePendingGoalProposal = vi.fn(() => { const proposal = pending; @@ -657,6 +930,7 @@ describe('LlmClient Goal admission', () => { return runtime; }, 'settle-key', + vi.fn(), ); // Dropped means taken and not applied: the slot is empty afterwards, so @@ -673,6 +947,7 @@ describe('LlmClient Goal admission', () => { takePendingGoalProposal: vi.fn(() => ({ objective: 'ship it', turnKey: 'settle-key', + reviewedGoal: null, })), }); const appliedGoal = { @@ -712,6 +987,7 @@ describe('LlmClient Goal admission', () => { controller.signal, async () => runtime, 'settle-key', + vi.fn(), ); expect(runtime.dispatch).toHaveBeenNthCalledWith(1, { @@ -739,6 +1015,7 @@ describe('LlmClient Goal admission', () => { const store = pendingGoalProposalStore({ objective: 'approved earlier', turnKey: 'owner-key', + reviewedGoal: null, }); Object.assign(config, { takePendingGoalProposal: store.take, @@ -754,6 +1031,7 @@ describe('LlmClient Goal admission', () => { controller.signal, async () => runtime, 'foreign-key', + vi.fn(), ); } else { if (exit === 'throwing') { @@ -785,6 +1063,7 @@ describe('LlmClient Goal admission', () => { expect(store.get()).toEqual({ objective: 'approved earlier', turnKey: 'owner-key', + reviewedGoal: null, }); expect(runtime.dispatch).not.toHaveBeenCalled(); @@ -793,6 +1072,7 @@ describe('LlmClient Goal admission', () => { new AbortController().signal, async () => runtime, 'owner-key', + vi.fn(), ); expect(store.get()).toBeUndefined(); @@ -808,6 +1088,7 @@ describe('LlmClient Goal admission', () => { const store = pendingGoalProposalStore({ objective: 'stale approval', turnKey: 'cancelled-key', + reviewedGoal: null, }); Object.assign(config, { takePendingGoalProposal: store.take }); vi.mocked(config.getDisableAllHooks).mockReturnValue(false); @@ -851,6 +1132,7 @@ describe('LlmClient Goal admission', () => { const store = pendingGoalProposalStore({ objective: 'stale approval', turnKey: 'cancelled-key', + reviewedGoal: null, }); Object.assign(config, { takePendingGoalProposal: store.take, @@ -884,6 +1166,7 @@ describe('LlmClient Goal admission', () => { const store = pendingGoalProposalStore({ objective: 'ship it', turnKey: 'owner-key', + reviewedGoal: null, }); Object.assign(config, { takePendingGoalProposal: store.take, @@ -981,7 +1264,11 @@ describe('LlmClient Goal admission', () => { nextSpeakerMocks.check.mockResolvedValue({ next_speaker: 'model' }); } turnMocks.run.mockImplementationOnce(() => { - store.set({ objective: 'ship it', turnKey: 'owner-key' }); + store.set({ + objective: 'ship it', + turnKey: 'owner-key', + reviewedGoal: null, + }); return emptyStream(); }); @@ -1018,6 +1305,7 @@ describe('LlmClient Goal admission', () => { .mockReturnValueOnce({ objective: 'stale', turnKey: 'cancelled-turn-key', + reviewedGoal: null, }) .mockReturnValue(undefined); Object.assign(config, { diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index a80fb699b01..d41c0bc8acf 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -43,7 +43,10 @@ import { GoalPersistenceUnavailableError, type GoalRuntime, } from '../goals/goal-runtime.js'; -import { applyPendingGoalProposal } from '../goals/goal-tools.js'; +import { + applyPendingGoalProposal, + formatProposeGoalRecoveryFailed, +} from '../goals/goal-tools.js'; import { formatStopHookBlockingCapWarning } from '../hooks/stopHookCap.js'; import { buildContextUsage } from '../hooks/context-usage.js'; import { DEFAULT_TOKEN_LIMIT, tokenLimit } from './tokenLimits.js'; @@ -883,13 +886,17 @@ export class LlmClient { * continuations keep the proposal parked until their final boundary. An * aborted turn drops the approval instead of starting a loop the user just * cancelled; an abort during dispatch pauses the new Goal. + * Host-supported sessions settle after classifying their own protective + * exits, so core must leave their single-take proposal latch untouched. */ private async settlePendingGoalProposal( turnEnded: boolean, signal: AbortSignal, loadGoalRuntime: (required: boolean) => Promise, turnKey: string, + reportFailure: (message: string) => void, ): Promise { + if (this.config.getGoalProposalHostSupported?.()) return; const take = this.config.takePendingGoalProposal; if (typeof take !== 'function') return; if (!turnEnded && !signal.aborted) return; @@ -901,11 +908,15 @@ export class LlmClient { debugLogger.debug( 'Dropping an approved Goal proposal: the Goal runtime is unavailable', ); + reportFailure(formatProposeGoalRecoveryFailed(proposal.objective)); return; } if (signal.aborted) return; const result = await applyPendingGoalProposal(runtime, proposal); - if (signal.aborted && result.applied) { + if ( + (signal.aborted || proposal.approvalSignal?.aborted) && + result.applied + ) { try { await runtime.dispatch({ action: 'pause', @@ -923,6 +934,11 @@ export class LlmClient { } if (!result.applied) { debugLogger.debug(`Dropping an approved Goal proposal: ${result.reason}`); + reportFailure( + result.kind === 'changed' + ? result.reason + : formatProposeGoalRecoveryFailed(proposal.objective), + ); } } @@ -2892,6 +2908,13 @@ export class LlmClient { let goalPermitReleased = false; let unsubscribeGoalState: (() => void) | undefined; const pendingGoalStateEvents: GoalStateStreamEvent[] = []; + const pendingGoalSettlementMessages: ServerLlmStreamEvent[] = []; + const reportGoalSettlementFailure = (message: string) => { + pendingGoalSettlementMessages.push({ + type: LlmEventType.GoalSettlementFailed, + value: message, + }); + }; let hasEmittedActiveGoalProjection = false; let lastEmittedActiveGoal: ActiveGoalEventValue | undefined; const closeGoalStateEvents = () => { @@ -2914,7 +2937,10 @@ export class LlmClient { }); }; const takePendingGoalEvents = (): ServerLlmStreamEvent[] => { - const events: ServerLlmStreamEvent[] = []; + const events = pendingGoalSettlementMessages.splice( + 0, + pendingGoalSettlementMessages.length, + ); for (const stateEvent of pendingGoalStateEvents.splice( 0, pendingGoalStateEvents.length, @@ -3273,6 +3299,7 @@ export class LlmClient { return runtime; }, prompt_id, + reportGoalSettlementFailure, ); for (const goalEvent of takePendingGoalEvents()) { yield goalEvent; @@ -4515,6 +4542,7 @@ export class LlmClient { signal, loadGoalRuntime, prompt_id, + reportGoalSettlementFailure, ); for (const goalEvent of takePendingGoalEvents()) { yield goalEvent; @@ -4580,6 +4608,7 @@ export class LlmClient { signal, loadGoalRuntime, prompt_id, + reportGoalSettlementFailure, ); for (const goalEvent of takePendingGoalEvents()) { yield goalEvent; @@ -4654,6 +4683,7 @@ export class LlmClient { signal, loadGoalRuntime, prompt_id, + reportGoalSettlementFailure, ); for (const goalEvent of takePendingGoalEvents()) { yield goalEvent; @@ -4711,6 +4741,7 @@ export class LlmClient { signal, loadGoalRuntime, prompt_id, + reportGoalSettlementFailure, ); for (const goalEvent of takePendingGoalEvents()) { yield goalEvent; @@ -4754,6 +4785,7 @@ export class LlmClient { signal, loadGoalRuntime, prompt_id, + reportGoalSettlementFailure, ); for (const goalEvent of takePendingGoalEvents()) { yield goalEvent; diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index 45f24d9d894..3e52240eb6e 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -7179,6 +7179,35 @@ describe('CoreToolScheduler', () => { }); describe('getToolSuggestion', () => { + it('does not suggest a tool that is unavailable for the current turn', () => { + const mockToolRegistry = { + getAllToolNames: () => ['propose_goal', 'read_file'], + isToolDeclared: (name: string) => name !== 'propose_goal', + getTool: () => undefined, + ensureTool: async () => undefined, + } as unknown as ToolRegistry; + const mockConfig = { + getToolRegistry: () => mockToolRegistry, + getUseModelRouter: () => false, + getLlmClient: () => null, + getPermissionsDeny: () => undefined, + isInteractive: () => false, + getMessageBus: vi.fn().mockReturnValue(undefined), + getDisableAllHooks: vi.fn().mockReturnValue(true), + } as unknown as Config; + const scheduler = new CoreToolScheduler({ + config: mockConfig, + getPreferredEditor: () => 'vscode', + onEditorClose: vi.fn(), + }); + + // @ts-expect-error accessing private method + const suggestion = scheduler.getToolSuggestion('propose_goals'); + + expect(suggestion).not.toContain('propose_goal'); + expect(suggestion).toContain('read_file'); + }); + it('should suggest the top N closest tool names for a typo', () => { // Create mocked tool registry const mockToolRegistry = { diff --git a/packages/core/src/core/coreToolScheduler.ts b/packages/core/src/core/coreToolScheduler.ts index fba1edc0a73..b98aefe96fd 100644 --- a/packages/core/src/core/coreToolScheduler.ts +++ b/packages/core/src/core/coreToolScheduler.ts @@ -2307,7 +2307,9 @@ export class CoreToolScheduler { /** Suggests similar tool names using Levenshtein distance. */ private getToolSuggestion(unknownToolName: string, topN = 3): string { - const allToolNames = this.toolRegistry.getAllToolNames(); + const allToolNames = this.toolRegistry + .getAllToolNames() + .filter((name) => this.toolRegistry.isToolDeclared?.(name) ?? true); const matches = allToolNames.map((toolName) => ({ name: toolName, diff --git a/packages/core/src/core/turn.ts b/packages/core/src/core/turn.ts index 06c4f40d6a9..eb7353d6d8a 100644 --- a/packages/core/src/core/turn.ts +++ b/packages/core/src/core/turn.ts @@ -75,6 +75,7 @@ export enum LlmEventType { Citation = 'citation', Retry = 'retry', HookSystemMessage = 'hook_system_message', + GoalSettlementFailed = 'goal_settlement_failed', UserPromptSubmitBlocked = 'user_prompt_submit_blocked', StopHookLoop = 'stop_hook_loop', GoalState = 'goal_state', @@ -488,6 +489,11 @@ export type ServerLlmHookSystemMessageEvent = { value: string; }; +export type ServerLlmGoalSettlementFailedEvent = { + type: LlmEventType.GoalSettlementFailed; + value: string; +}; + export type ServerLlmUserPromptSubmitBlockedEvent = { type: LlmEventType.UserPromptSubmitBlocked; value: { @@ -525,6 +531,7 @@ export type ServerLlmStreamEvent = | ServerLlmContentEvent | ServerLlmErrorEvent | ServerLlmFinishedEvent + | ServerLlmGoalSettlementFailedEvent | ServerLlmHookSystemMessageEvent | ServerLlmUserPromptSubmitBlockedEvent | ServerLlmStopHookLoopEvent diff --git a/packages/core/src/goals/goal-tools.test.ts b/packages/core/src/goals/goal-tools.test.ts index c25968509b7..d671e903f67 100644 --- a/packages/core/src/goals/goal-tools.test.ts +++ b/packages/core/src/goals/goal-tools.test.ts @@ -27,6 +27,8 @@ import { type PendingGoalProposal, type ProposeGoalToolConfig, applyPendingGoalProposal, + formatProposeGoalRecoveryFailed, + formatProposeGoalRecoveryNotStarted, UpdateGoalTool, type GoalToolConfig, } from './goal-tools.js'; @@ -1665,9 +1667,22 @@ describe('ProposeGoalTool', () => { objective: 'x'.repeat(PROPOSE_GOAL_OBJECTIVE_MAX_CHARACTERS + 1), }), ).not.toBeNull(); + expect( + tool.validateToolParams({ objective: 'Outcome: ship it.\nBudget: 20.' }), + ).toBe('objective must be written on one line.'); expect(tool.validateToolParams({ objective })).toBeNull(); }); + it('prints each recovery command as a complete final line', () => { + for (const message of [ + formatProposeGoalRecoveryNotStarted(objective), + formatProposeGoalRecoveryFailed(objective), + ]) { + expect(message.split('\n').at(-1)).toBe(`/goal set ${objective}`); + expect(message.match(/\/goal/g)).toHaveLength(1); + } + }); + it('shows the objective in a plain-text info dialog and parks it on approval', async () => { const { runtime, host } = idleRuntime(); const config = proposeConfig(runtime); @@ -1680,6 +1695,7 @@ describe('ProposeGoalTool', () => { expect(details.type).toBe('info'); if (details.type !== 'info') return; expect(details.renderPromptAsPlainText).toBe(true); + expect(details.hideAlwaysAllow).toBe(true); expect(details.prompt).toContain('Set this as the session Goal?'); expect(details.prompt).toContain(objective); @@ -1695,7 +1711,11 @@ describe('ProposeGoalTool', () => { // proposing turn of its Goal permit. The client applies it at the // turn boundary (see applyPendingGoalProposal below). expect(config.setPendingGoalProposal).toHaveBeenCalledTimes(1); - expect(config.pending()).toEqual({ objective, turnKey: 'user-turn-key' }); + expect(config.pending()).toEqual({ + objective, + turnKey: 'user-turn-key', + reviewedGoal: null, + }); expect(runtime.getSnapshot().goal).toBeNull(); expect(host.started).toHaveLength(0); @@ -1882,6 +1902,78 @@ describe('ProposeGoalTool', () => { expect(runtime.getSnapshot().goal?.objective).toBe('Typed by hand'); }); + describe.each(['before approval', 'before settlement'] as const)( + 'rejects a changed reviewed target %s', + (phase) => { + it.each(['create', 'edit', 'replace', 'clear'] as const)( + 'preserves the result of a concurrent %s', + async (action) => { + const { runtime, host } = idleRuntime(); + const pauseCurrent = async () => { + const current = runtime.getSnapshot().goal!; + await runtime.dispatch({ + action: 'pause', + expectedGoalId: current.goalId, + expectedRevision: current.revision, + }); + }; + if (action !== 'create') { + await runtime.dispatch({ action: 'create', objective: 'Original' }); + await pauseCurrent(); + } + const config = proposeConfig(runtime); + const invocation = new ProposeGoalTool(config).build({ objective }); + const details = await invocation.getConfirmationDetails( + new AbortController().signal, + ); + if (phase === 'before settlement') { + await details.onConfirm(ToolConfirmationOutcome.ProceedOnce); + expect((await execute(invocation)).error).toBeUndefined(); + } + + if (action === 'create') { + await runtime.dispatch({ action, objective: 'Concurrent' }); + await pauseCurrent(); + } else { + const current = runtime.getSnapshot().goal!; + const version = { + expectedGoalId: current.goalId, + expectedRevision: current.revision, + }; + await runtime.dispatch( + action === 'clear' + ? { action, ...version } + : { action, objective: 'Concurrent', ...version }, + ); + if (action === 'replace') await pauseCurrent(); + } + const changed = runtime.getSnapshot().goal; + const startedBefore = host.started.length; + + if (phase === 'before approval') { + await details.onConfirm(ToolConfirmationOutcome.ProceedOnce); + const result = await execute(invocation); + expect(result.error?.type).toBe(ToolErrorType.EXECUTION_DENIED); + expect(result.llmContent).toContain('changed after the proposal'); + expect(config.pending()).toBeUndefined(); + } else { + const result = await applyPendingGoalProposal( + runtime, + config.pending()!, + ); + expect(result).toMatchObject({ + applied: false, + kind: 'changed', + reason: expect.stringContaining('changed after the proposal'), + }); + } + expect(runtime.getSnapshot().goal).toEqual(changed); + expect(host.started).toHaveLength(startedBefore); + }, + ); + }, + ); + it('replaces a stopped Goal when the parked approval is applied', async () => { const { runtime, host } = idleRuntime(); await runtime.dispatch({ action: 'create', objective: 'Ship Goal v3' }); @@ -1906,6 +1998,10 @@ describe('ProposeGoalTool', () => { const result = await execute(invocation); const payload = JSON.parse(result.llmContent as string); expect(payload.replacesGoalId).toBe(paused.goalId); + expect(config.pending()?.reviewedGoal).toEqual({ + goalId: paused.goalId, + revision: paused.revision, + }); expect(runtime.getSnapshot().goal?.goalId).toBe(paused.goalId); const applied = await applyPendingGoalProposal(runtime, config.pending()!); @@ -1933,6 +2029,7 @@ describe('ProposeGoalTool', () => { const applied = await applyPendingGoalProposal(runtime, config.pending()!); expect(applied.applied).toBe(false); if (applied.applied) return; + expect(applied.kind).toBe('changed'); expect(applied.reason).toContain('became active'); expect(runtime.getSnapshot().goal?.objective).toBe('Typed by hand'); }); @@ -1955,6 +2052,7 @@ describe('ProposeGoalTool', () => { const applied = applyPendingGoalProposal(runtime, { objective, turnKey: 'user-turn-key', + reviewedGoal: { goalId: original.goalId, revision: original.revision }, }); await expect(resumed).resolves.toMatchObject({ @@ -1966,7 +2064,10 @@ describe('ProposeGoalTool', () => { }, }, }); - await expect(applied).resolves.toMatchObject({ applied: false }); + await expect(applied).resolves.toMatchObject({ + applied: false, + kind: 'changed', + }); expect(runtime.getSnapshot().goal).toMatchObject({ goalId: original.goalId, objective: 'Paused by user', @@ -1995,8 +2096,11 @@ describe('ProposeGoalTool', () => { const applied = await applyPendingGoalProposal(stale, { objective, turnKey: 'user-turn-key', + reviewedGoal: { goalId: paused.goalId, revision: paused.revision - 1 }, }); expect(applied.applied).toBe(false); + if (applied.applied) return; + expect(applied.kind).toBe('changed'); expect(runtime.getSnapshot().goal?.goalId).toBe(paused.goalId); }); }); diff --git a/packages/core/src/goals/goal-tools.ts b/packages/core/src/goals/goal-tools.ts index 1696a362121..cb7122eb68d 100644 --- a/packages/core/src/goals/goal-tools.ts +++ b/packages/core/src/goals/goal-tools.ts @@ -624,6 +624,12 @@ function summarizeCatalog( */ export const PROPOSE_GOAL_OBJECTIVE_MAX_CHARACTERS = 1500; +export const formatProposeGoalRecoveryNotStarted = (objective: string) => + `The approved Goal was not started because the turn did not finish normally. To start it, run:\n/goal set ${objective}`; + +export const formatProposeGoalRecoveryFailed = (objective: string) => + `The approved Goal could not be started. Check the Goal status before trying again, or run:\n/goal set ${objective}`; + export interface ProposeGoalToolParams { objective: string; } @@ -637,6 +643,9 @@ export interface ProposeGoalToolParams { */ export interface PendingGoalProposal { objective: string; + reviewedGoal: Pick | null; + /** Plan mode revokes approval even after the host takes the proposal. */ + approvalSignal?: AbortSignal; /** * The `prompt_id` of the turn whose dialog approved it. Only that turn's * terminal boundary may set or discard the Goal; unrelated frames leave it @@ -657,29 +666,44 @@ type ProposeGoalRuntime = Pick; export type ApplyPendingGoalProposalResult = | { applied: true; goal: GoalRecord } - | { applied: false; reason: string }; + | { applied: false; reason: string; kind: 'changed' | 'unavailable' }; /** * Sets an approved proposal as the session Goal. Called by the client once * the proposing turn has ended; never from inside a turn. * * Re-reads the snapshot because `/goal` may have changed the session since - * the dialog: an active Goal is never replaced (someone is already running - * it), a stopped one is replaced through its expected version, and no Goal - * creates. + * the dialog: only the reviewed Goal can be replaced, through its expected + * version, and a reviewed empty session can only create a new Goal. */ export async function applyPendingGoalProposal( runtime: ProposeGoalRuntime, proposal: PendingGoalProposal, ): Promise { + if (proposal.approvalSignal?.aborted) { + return { + applied: false, + kind: 'changed', + reason: + 'The approved Goal was not started because its approval was revoked. Ask for a new draft when you are ready to start.', + }; + } const objective = proposal.objective.trim(); const current = runtime.getSnapshot().goal; if (current?.status === 'active') { return { applied: false, + kind: 'changed', reason: `A Goal became active (revision ${current.revision}) before the approved proposal could be set.`, }; } + if (!matchesReviewedGoal(current, proposal.reviewedGoal)) { + return { + applied: false, + kind: 'changed', + reason: PROPOSE_GOAL_CHANGED_MESSAGE, + }; + } const request: GoalControlRequest = current ? { action: 'replace', @@ -697,6 +721,7 @@ export async function applyPendingGoalProposal( if (!goal) { return { applied: false, + kind: 'unavailable', reason: 'The Goal runtime accepted the request but reported no Goal.', }; } @@ -707,10 +732,12 @@ export async function applyPendingGoalProposal( } catch (error) { if ( error instanceof GoalConflictError || - error instanceof GoalInvalidTransitionError || - error instanceof GoalPersistenceUnavailableError + error instanceof GoalInvalidTransitionError ) { - return { applied: false, reason: error.message }; + return { applied: false, kind: 'changed', reason: error.message }; + } + if (error instanceof GoalPersistenceUnavailableError) { + return { applied: false, kind: 'unavailable', reason: error.message }; } throw error; } @@ -740,6 +767,19 @@ export const PROPOSE_GOAL_NO_TURN_MESSAGE = 'The Goal was not set: this call is not attributable to a turn, so its approval could not be bound to one. Hand the user a `/goal set ` line instead.'; export const PROPOSE_GOAL_PENDING_MESSAGE = 'Another approved Goal proposal is already waiting for this turn to end. Do not propose another one.'; +const PROPOSE_GOAL_CHANGED_MESSAGE = + 'The Goal changed after the proposal was shown. The approved proposal was not applied; review the current Goal before proposing again.'; + +function matchesReviewedGoal( + current: GoalRecord | null, + reviewed: PendingGoalProposal['reviewedGoal'] | undefined, +): boolean { + if (reviewed === undefined) return false; + return reviewed === null + ? current === null + : current?.goalId === reviewed.goalId && + current.revision === reviewed.revision; +} function activeGoalMessage(revision: number): string { return `A Goal is already active (revision ${revision}); this tool does not replace a running Goal. Hand the user a \`/goal edit \` line to tighten it or a \`/goal set \` line to replace it, and stop.`; @@ -757,6 +797,7 @@ class ProposeGoalInvocation extends BaseToolInvocation< GoalToolResult > { private approved = false; + private reviewedGoal: PendingGoalProposal['reviewedGoal'] | undefined; constructor( params: ProposeGoalToolParams, @@ -766,9 +807,7 @@ class ProposeGoalInvocation extends BaseToolInvocation< } /** - * The description is the one piece of the confirmation every host shows - * (the Web Shell does not forward an `info` prompt), so the objective has - * to be in it. + * Include the objective for hosts that show only the tool description. */ getDescription(): string { return `Propose Goal: ${this.params.objective.trim()}`; @@ -842,11 +881,15 @@ class ProposeGoalInvocation extends BaseToolInvocation< throw new StructuredToolError(blocker.message, blocker.type); } const current = this.config.getGoalRuntime().getSnapshot().goal; + this.reviewedGoal = current + ? { goalId: current.goalId, revision: current.revision } + : null; return { type: 'info', title: 'Set this as the session Goal?', prompt: `${proposalPromptHeadline(current)}\n\n${this.params.objective.trim()}`, renderPromptAsPlainText: true, + hideAlwaysAllow: true, onConfirm: async (outcome: ToolConfirmationOutcome) => { this.approved = outcome !== ToolConfirmationOutcome.Cancel; }, @@ -865,6 +908,15 @@ class ProposeGoalInvocation extends BaseToolInvocation< const objective = this.params.objective.trim(); const current = this.config.getGoalRuntime().getSnapshot().goal; + if ( + this.reviewedGoal === undefined || + !matchesReviewedGoal(current, this.reviewedGoal) + ) { + return this.errorResult( + PROPOSE_GOAL_CHANGED_MESSAGE, + ToolErrorType.EXECUTION_DENIED, + ); + } // Parked, not dispatched: the client sets it when this turn ends. Doing // it here would strip the rest of the turn of its Goal permit. The // approval is bound to this turn's prompt id so no other frame can @@ -876,7 +928,13 @@ class ProposeGoalInvocation extends BaseToolInvocation< ToolErrorType.EXECUTION_DENIED, ); } - if (!this.config.setPendingGoalProposal({ objective, turnKey })) { + if ( + !this.config.setPendingGoalProposal({ + objective, + turnKey, + reviewedGoal: this.reviewedGoal, + }) + ) { return this.errorResult( PROPOSE_GOAL_PENDING_MESSAGE, ToolErrorType.EXECUTION_DENIED, @@ -945,6 +1003,9 @@ export class ProposeGoalTool extends BaseDeclarativeTool< if (params.objective.length > PROPOSE_GOAL_OBJECTIVE_MAX_CHARACTERS) { return `objective must be at most ${PROPOSE_GOAL_OBJECTIVE_MAX_CHARACTERS} characters.`; } + if (/[\r\n]/.test(params.objective)) { + return 'objective must be written on one line.'; + } return null; } diff --git a/packages/core/src/goals/index.ts b/packages/core/src/goals/index.ts index f9f65818c48..2516c09b519 100644 --- a/packages/core/src/goals/index.ts +++ b/packages/core/src/goals/index.ts @@ -69,6 +69,13 @@ export * from './goal-checkpoint.js'; export * from './goal-checkpoint-verifier.js'; export * from './goal-verifier.js'; export * from './goal-runtime.js'; +export { + applyPendingGoalProposal, + formatProposeGoalRecoveryFailed, + formatProposeGoalRecoveryNotStarted, + ProposeGoalTool, +} from './goal-tools.js'; +export type { PendingGoalProposal } from './goal-tools.js'; export { goalTurnContext } from './goal-turn-context.js'; export { buildGoalContinuationParts, diff --git a/packages/core/src/skills/bundled/goal-draft/SKILL.md b/packages/core/src/skills/bundled/goal-draft/SKILL.md index 9d17d6dfeda..965eb996868 100644 --- a/packages/core/src/skills/bundled/goal-draft/SKILL.md +++ b/packages/core/src/skills/bundled/goal-draft/SKILL.md @@ -114,10 +114,10 @@ Then hand off, and nothing else: **If the `propose_goal` tool is available and no Goal is active**, call it with the objective on one line. The user approves or declines it in a dialog; only their approval sets the Goal. If they decline you will not be told why: stop, do not ask about it, and do not propose the same or a reworded objective again. After approval, acknowledge it in one sentence and end the turn — the Goal runtime starts the first Goal turn on its own. -**Otherwise** (Web Shell or another ACP client, headless, the tool is disabled, or a Goal is active), print: +**Otherwise** (a client without Goal proposal support, headless, the tool is disabled, or a Goal is active), print: 1. The objective in a fenced code block. 2. One line the user can run as-is: `/goal set ` (or `/goal edit …` when tightening the active goal). Print it as plain text with no code markers, so it can be copied verbatim. -3. One sentence explaining that the draft has not been applied and the user must run the command to apply it; include any `[ASSUMPTION]` items. Do not promise a dialog in Web Shell or other ACP sessions. +3. One sentence explaining that the draft has not been applied and the user must run the command to apply it; include any `[ASSUMPTION]` items. Do not promise a dialog when the tool is unavailable. Do not run /goal yourself. Do not begin the task. Stop and wait for the user. diff --git a/packages/core/src/skills/bundled/goal-draft/SKILL.test.ts b/packages/core/src/skills/bundled/goal-draft/SKILL.test.ts index 628c387393d..0b9823319bc 100644 --- a/packages/core/src/skills/bundled/goal-draft/SKILL.test.ts +++ b/packages/core/src/skills/bundled/goal-draft/SKILL.test.ts @@ -257,11 +257,11 @@ describe('bundled goal-draft skill', () => { expect(body).toContain('acknowledge it in one sentence and end the turn'); // The text hand-off survives for headless runs and disabled tools. expect(body).toContain( - '**Otherwise** (Web Shell or another ACP client, headless, the tool is disabled, or a Goal is active)', + '**Otherwise** (a client without Goal proposal support, headless, the tool is disabled, or a Goal is active)', ); expect(body).toContain('the draft has not been applied'); expect(body).toContain( - 'Do not promise a dialog in Web Shell or other ACP sessions', + 'Do not promise a dialog when the tool is unavailable', ); }); diff --git a/packages/core/src/tools/tool-registry.test.ts b/packages/core/src/tools/tool-registry.test.ts index 42b2663eb19..8e78ed79831 100644 --- a/packages/core/src/tools/tool-registry.test.ts +++ b/packages/core/src/tools/tool-registry.test.ts @@ -396,6 +396,51 @@ describe('ToolRegistry', () => { expect(names).toEqual(['alpha', 'middle', 'zeta']); }); + it('only declares Goal proposals during an ACP turn with a responder', () => { + const acpConfig = new Config({ + ...baseConfigParams, + experimentalZedIntegration: true, + }); + acpConfig.setGoalProposalHostSupported(true); + const registry = new ToolRegistry(acpConfig); + registry.registerTool(new MockTool({ name: 'other_tool' })); + registry.registerTool(new MockTool({ name: 'propose_goal' })); + expect( + registry.getFunctionDeclarations().map((tool) => tool.name), + ).toEqual(['other_tool']); + expect( + registry + .getFunctionDeclarationsFiltered(['other_tool', 'propose_goal']) + .map((tool) => tool.name), + ).toEqual(['other_tool']); + acpConfig.setGoalProposalTurnKey('user-turn'); + expect( + registry.getFunctionDeclarations().map((tool) => tool.name), + ).toEqual(['other_tool', 'propose_goal']); + expect( + registry + .getFunctionDeclarationsFiltered(['other_tool', 'propose_goal']) + .map((tool) => tool.name), + ).toEqual(['other_tool', 'propose_goal']); + acpConfig.setGoalProposalTurnKey(undefined); + expect( + registry.getFunctionDeclarations().map((tool) => tool.name), + ).toEqual(['other_tool']); + }); + + it('keeps Goal proposals declared in an interactive terminal', () => { + const interactiveConfig = new Config({ + ...baseConfigParams, + interactive: true, + }); + const registry = new ToolRegistry(interactiveConfig); + registry.registerTool(new MockTool({ name: 'propose_goal' })); + + expect( + registry.getFunctionDeclarations().map((tool) => tool.name), + ).toEqual(['propose_goal']); + }); + it('excludes shouldDefer tools from getFunctionDeclarations by default', () => { toolRegistry.registerTool(new MockTool({ name: 'visible' })); toolRegistry.registerTool( @@ -797,6 +842,28 @@ describe('ToolRegistry', () => { expect(summary).toEqual([{ name: 'beta', description: 'b' }]); }); + it('excludes unavailable Goal proposals from the deferred summary', () => { + const acpConfig = new Config({ + ...baseConfigParams, + experimentalZedIntegration: true, + }); + acpConfig.setGoalProposalHostSupported(true); + const registry = new ToolRegistry(acpConfig); + registry.registerTool( + new MockTool({ + name: 'propose_goal', + description: 'propose a Goal', + shouldDefer: true, + }), + ); + + expect(registry.getDeferredToolSummary()).toEqual([]); + acpConfig.setGoalProposalTurnKey('user-turn'); + expect(registry.getDeferredToolSummary()).toEqual([ + { name: 'propose_goal', description: 'propose a Goal' }, + ]); + }); + it('visibleTools has no effect on non-deferred tools', () => { const visibleConfig = new Config({ ...baseConfigParams, diff --git a/packages/core/src/tools/tool-registry.ts b/packages/core/src/tools/tool-registry.ts index 65b77409697..3510e9fa92e 100644 --- a/packages/core/src/tools/tool-registry.ts +++ b/packages/core/src/tools/tool-registry.ts @@ -21,6 +21,7 @@ import { McpClientManager } from './mcp-client-manager.js'; import { DiscoveredMCPTool } from './mcp-tool.js'; import { parse } from 'shell-quote'; import { ToolErrorType } from './tool-error.js'; +import { ToolNames } from './tool-names.js'; import { safeJsonStringify } from '../utils/safeJsonStringify.js'; import type { EventEmitter } from 'node:events'; import { createDebugLogger } from '../utils/debugLogger.js'; @@ -820,6 +821,7 @@ export class ToolRegistry { }): FunctionDeclaration[] { const includeDeferred = options?.includeDeferred === true; return Array.from(this.tools.values()) + .filter((tool) => this.isToolDeclared(tool.name)) .filter( (tool) => includeDeferred || @@ -923,6 +925,7 @@ export class ToolRegistry { if ( this.isEffectivelyDeferred(tool) && !tool.alwaysLoad && + this.isToolDeclared(tool.name) && !this.config.getVisibleTools().has(tool.name) ) { summary.push({ @@ -1019,13 +1022,19 @@ export class ToolRegistry { const declarations: FunctionDeclaration[] = []; for (const name of toolNames) { const tool = this.tools.get(name); - if (tool) { + if (tool && this.isToolDeclared(tool.name)) { declarations.push(tool.schema); } } return declarations; } + isToolDeclared(name: string): boolean { + return ( + name !== ToolNames.PROPOSE_GOAL || this.config.isGoalProposalAvailable() + ); + } + /** * Returns an array of all registered and discovered tool names, * including tools that are registered via factory but not yet loaded. diff --git a/packages/core/src/tools/tool-search.test.ts b/packages/core/src/tools/tool-search.test.ts index c72eebebd2e..59939a09c49 100644 --- a/packages/core/src/tools/tool-search.test.ts +++ b/packages/core/src/tools/tool-search.test.ts @@ -208,6 +208,29 @@ describe('ToolSearchTool', () => { expect(registry.isDeferredToolRevealed('cron_create')).toBe(true); }); + it('does not publish a Goal proposal schema outside an armed ACP turn', async () => { + const acpConfig = new Config({ + ...baseConfigParams, + experimentalZedIntegration: true, + }); + acpConfig.setGoalProposalHostSupported(true); + const acpRegistry = new ToolRegistry(acpConfig); + vi.spyOn(acpConfig, 'getToolRegistry').mockReturnValue(acpRegistry); + acpRegistry.registerTool(new MockTool({ name: 'propose_goal' })); + const tool = new ToolSearchTool(acpConfig); + + const unavailable = await tool + .build({ query: 'select:propose_goal' }) + .execute(new AbortController().signal); + expect(String(unavailable.llmContent)).toBe('Not found: propose_goal'); + + acpConfig.setGoalProposalTurnKey('user-turn'); + const available = await tool + .build({ query: 'select:propose_goal' }) + .execute(new AbortController().signal); + expect(String(available.llmContent)).toContain('"name":"propose_goal"'); + }); + it('escapes `<` in schema JSON so embedded cannot close the wrapper', async () => { // MCP descriptions are remote-supplied untrusted text. A description // containing the literal substring `` would prematurely diff --git a/packages/core/src/tools/tool-search.ts b/packages/core/src/tools/tool-search.ts index 8e04796b9e3..5018ee0836c 100644 --- a/packages/core/src/tools/tool-search.ts +++ b/packages/core/src/tools/tool-search.ts @@ -287,6 +287,10 @@ class ToolSearchInvocation extends BaseToolInvocation< missing.push(requested); continue; } + if (!registry.isToolDeclared(canonical)) { + missing.push(requested); + continue; + } if ( isPlanLifecycleToolUnavailableInSubagent(canonical) || isLeaderOnlyToolUnavailableInSubagent(canonical)