From ce6688c32ed2a9fb6891d5aa8ae9b681fb60b8c4 Mon Sep 17 00:00:00 2001 From: qqqys <266654365+qqqys@users.noreply.github.com> Date: Tue, 8 Sep 2026 17:02:36 +0800 Subject: [PATCH 1/7] feat(goal): start approved Web Shell proposals after their owning turn --- docs/users/features/goals.md | 4 +- packages/acp-bridge/src/bridge.test.ts | 36 ++ packages/acp-bridge/src/bridge.ts | 8 + .../cli/src/acp-integration/acpAgent.test.ts | 55 +++ packages/cli/src/acp-integration/acpAgent.ts | 15 + .../acp-integration/session/Session.test.ts | 404 ++++++++++++++++++ .../src/acp-integration/session/Session.ts | 148 +++++++ .../session/permissionUtils.test.ts | 24 ++ .../session/permissionUtils.ts | 7 + packages/core/src/config/config.test.ts | 24 ++ packages/core/src/config/config.ts | 24 +- packages/core/src/core/client-goal.test.ts | 67 ++- packages/core/src/goals/goal-tools.test.ts | 84 +++- packages/core/src/goals/goal-tools.ts | 48 ++- packages/core/src/goals/index.ts | 2 + .../src/skills/bundled/goal-draft/SKILL.md | 4 +- .../skills/bundled/goal-draft/SKILL.test.ts | 4 +- packages/core/src/tools/tool-registry.test.ts | 17 + packages/core/src/tools/tool-registry.ts | 6 + 19 files changed, 952 insertions(+), 29 deletions(-) diff --git a/docs/users/features/goals.md b/docs/users/features/goals.md index ad3beb30e19..4f401b6486e 100644 --- a/docs/users/features/goals.md +++ b/docs/users/features/goals.md @@ -69,13 +69,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, channel deliveries, 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 27a618f585d..bb3475b2d67 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 @@ -15438,6 +15443,37 @@ 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.shutdown(); + }); + it('strips both spoofed retry and continue meta keys from one prompt', async () => { const handle = makeChannel(); const bridge = makeBridge({ channelFactory: async () => handle.channel }); diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index a53dbc1272b..92cba80bcda 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -5020,6 +5020,7 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { privateParentCapability, }, clientCapabilities: { + _meta: { 'qwen.goalProposals': true }, fs: { readTextFile: delegateReadTextFileToClient, writeTextFile: true, @@ -10399,6 +10400,13 @@ 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 && + !context?.channelPrompt + ) { + meta['qwen.goalProposalApproval'] = true; + } if (isRetry) { meta[DAEMON_RETRY_META_KEY] = true; } diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index 58de03b758d..1d161679f21 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -2406,6 +2406,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' }, @@ -2421,6 +2422,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' }, @@ -2559,6 +2561,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' }, @@ -3937,6 +3940,56 @@ describe('QwenAgent MCP SSE/HTTP support', () => { }, ); + it.each([ + [true, undefined, true], + [false, undefined, false], + [true, 'channel', false], + ])( + '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); @@ -4430,6 +4483,7 @@ describe('QwenAgent MCP SSE/HTTP support', () => { setSessionWriterReclaimPolicy: vi.fn(), setSessionWriterTakeoverPolicy: vi.fn(), setSessionSource: vi.fn(), + setGoalProposalHostSupported: vi.fn(), getCronScheduler: vi.fn(), getSessionSourceType: vi.fn().mockReturnValue(undefined), waitForMcpReady: vi.fn().mockResolvedValue(undefined), @@ -22797,6 +22851,7 @@ describe('QwenAgent loadSession / unstable_resumeSession', () => { setSessionWriterReclaimPolicy: vi.fn(), setSessionWriterTakeoverPolicy: vi.fn(), setSessionSource: vi.fn(), + setGoalProposalHostSupported: vi.fn(), waitForMcpReady: vi.fn().mockResolvedValue(undefined), getModelsConfig: vi.fn().mockReturnValue({ getCurrentAuthType: vi.fn().mockReturnValue('api-key'), diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index fdcf5b2895a..ae8ae3acb25 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -6070,12 +6070,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 @@ -13908,6 +13916,13 @@ class QwenAgent implements Agent { if (sessionSource) { config.setSessionSource(sessionSource.sourceType, sessionSource.sourceId); } + if ( + this.clientCapabilities?._meta?.['qwen.goalProposals'] === true && + sessionSource?.sourceType !== 'channel' && + !provisionalWorkspace + ) { + config.setGoalProposalHostSupported(true); + } if (chatRecording !== false) { this.initializingConfigs.add(config); } diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index fd249ac59bc..2f3f2d2881a 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -986,6 +986,8 @@ 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), getGoalRuntimePrepared: vi.fn().mockResolvedValue(mockGoalRuntime), bindGoalTurnHost: vi.fn().mockImplementation((host) => { boundGoalHost = host; @@ -32181,6 +32183,408 @@ describe('Session', () => { }); }); + describe('ordinary-turn Goal proposals', () => { + const objective = 'Outcome: tests pass. Done when: npm test exits 0.'; + let pending: core.PendingGoalProposal | undefined; + + function proposalStream() { + return createStreamWithChunks([ + { + type: core.StreamEventType.CHUNK, + value: { + functionCalls: [ + { + id: 'goal-proposal', + name: 'propose_goal', + args: { objective }, + }, + ], + }, + }, + ]); + } + + function prompt() { + return session.prompt({ + sessionId: 'test-session-id', + _meta: { 'qwen.goalProposalApproval': true }, + prompt: [ + { type: 'text', text: 'Draft a Goal to make the tests pass.' }, + ], + }); + } + + beforeEach(() => { + pending = undefined; + 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(['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(); + }); + + 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('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('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: expect.stringContaining('Automatic execution is held'), + }), + }), + }), + ); + + 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(); + }); + + 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(); + }); + + 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(); + }); + + 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 7bf60aa0692..64e6c4e6c05 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -185,6 +185,7 @@ import { GOAL_PAUSE_REASON_SESSION_DISPOSED, GOAL_PAUSE_REASON_STOP_HOOK_CAP, GOAL_PAUSE_REASON_USER_INTERRUPT, + applyPendingGoalProposal, goalPauseReasonForFailure, ambientGoalToolResultProvenance, goalTurnContext, @@ -1508,6 +1509,11 @@ interface PromptChannelDelivery { } interface AgentResponseCapture { + goalProposalTurn?: { + turnKey: string; + controller: AbortController; + completedNormally: boolean; + }; channelDelivery?: { finalText: string; }; @@ -1968,6 +1974,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 @@ -2061,6 +2068,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; @@ -2286,6 +2294,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( @@ -2529,6 +2546,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; @@ -4929,6 +4954,7 @@ export class Session implements SessionContext { } : result; promptResult = completedResult; + await this.#settleGoalProposal(responseCapture); releasePendingSend(); // Drain any cron prompts that queued while the prompt was active void this.#drainCronQueue(); @@ -4958,6 +4984,24 @@ 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 = @@ -5212,6 +5256,66 @@ 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()) return; + try { + const runtime = await this.config.getGoalRuntimeReady(); + if (!ownsTurn()) 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()) { + try { + await runtime.dispatch({ + action: 'pause', + expectedGoalId: result.goal.goalId, + expectedRevision: result.goal.revision, + reason: 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. Run /goal pause, then /goal resume when ready.', + ); + } + } + } else if (!result.applied) { + await this.messageEmitter.emitAgentMessage( + `The approved Goal could not be started: ${result.reason}`, + ); + } + } catch (error) { + debugLogger.warn('Failed to apply an approved Goal proposal', error); + await this.messageEmitter.emitAgentMessage( + 'The approved Goal could not be started. Check /goal before trying again.', + ); + } + } + async #executePromptInner( params: PromptRequest, pendingSend: AbortController, @@ -5234,6 +5338,24 @@ 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 promptMetadata = (params as { _meta?: Record }) ._meta; const continuesCurrentWorkChain = @@ -6600,6 +6722,9 @@ export class Session implements SessionContext { } if (!externalReason && !guardContinuation) { + if (responseCapture?.goalProposalTurn?.turnKey === promptId) { + responseCapture.goalProposalTurn.completedNormally = true; + } return { stopReason: 'end_turn' }; } @@ -8595,6 +8720,13 @@ export class Session implements SessionContext { preserveFallbackOnAbort?: boolean; } = {}, ): Promise { + const proposalTurn = this.activeGoalProposalTurn; + if ( + messages.length > 0 && + proposalTurn?.controller.signal === abortSignal + ) { + this.config.takePendingGoalProposal(proposalTurn.turnKey); + } const parts: Part[] = []; for (const message of messages) { const displayText = @@ -11935,6 +12067,22 @@ 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('Goal proposals require an interactive user turn.'), + 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/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/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 0439d50fdb8..bb1a0921455 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -3311,12 +3311,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); @@ -3325,6 +3327,7 @@ describe('Server Config (config.ts)', () => { expect(config.takePendingGoalProposal('turn-1')).toEqual({ objective: 'first', turnKey: 'turn-1', + reviewedGoal: null, }); expect(config.hasPendingGoalProposal()).toBe(false); expect(config.takePendingGoalProposal()).toBeUndefined(); @@ -3333,11 +3336,13 @@ 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, }); expect(config.hasPendingGoalProposal()).toBe(false); }); @@ -3347,6 +3352,7 @@ describe('Server Config (config.ts)', () => { config.setPendingGoalProposal({ objective: 'stale approval', turnKey: 'turn-1', + reviewedGoal: null, }); config.startNewSession('replacement-session'); @@ -5467,6 +5473,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 0d59e87ef04..d7ece810dca 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -2376,6 +2376,8 @@ export class Config { private sessionWorkflowEnabledProvider?: () => boolean; private sessionWorkflowPlanRevision?: SessionWorkflowPlanRevision; 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; @@ -7918,6 +7920,25 @@ export class Config { return this.modelProposedGoals; } + setGoalProposalHostSupported(supported: boolean): void { + this.goalProposalHostSupported = supported; + } + + getGoalProposalHostSupported(): boolean { + return this.goalProposalHostSupported; + } + + setGoalProposalTurnKey(turnKey: string | undefined): void { + this.goalProposalTurnKey = turnKey; + } + + isGoalProposalAvailable(): boolean { + return ( + resolveInteractionMode(this) === 'interactive' || + (this.goalProposalHostSupported && this.goalProposalTurnKey !== undefined) + ); + } + hasPendingGoalProposal(): boolean { return this.pendingGoalProposal !== undefined; } @@ -9440,7 +9461,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.test.ts b/packages/core/src/core/client-goal.test.ts index 787b0a09f7f..43179ca11f1 100644 --- a/packages/core/src/core/client-goal.test.ts +++ b/packages/core/src/core/client-goal.test.ts @@ -315,7 +315,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), @@ -344,7 +348,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; @@ -356,7 +360,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(); }); @@ -383,7 +391,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; @@ -403,7 +411,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(); }); @@ -436,7 +448,7 @@ describe('LlmClient Goal admission', () => { tokensAtStart: 1, hookId: 'old-goal-hook', }); - let pending: { objective: string; turnKey: string } | undefined; + let pending: PendingGoalProposal | undefined; const takePendingGoalProposal = vi.fn(() => { const proposal = pending; pending = undefined; @@ -463,7 +475,11 @@ describe('LlmClient Goal admission', () => { getUsageStatisticsEnabled: vi.fn(() => false), }); turnMocks.run.mockImplementationOnce(() => { - pending = { objective: 'ship it', turnKey: 'stop-clear-key' }; + pending = { + objective: 'ship it', + turnKey: 'stop-clear-key', + reviewedGoal: null, + }; return emptyStream(); }); const getSteerInput = vi @@ -496,9 +512,10 @@ describe('LlmClient Goal admission', () => { 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, { @@ -529,7 +546,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; @@ -543,7 +560,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); @@ -562,6 +583,7 @@ describe('LlmClient Goal admission', () => { expect(pending).toEqual({ objective: 'ship it', turnKey: 'pending-tool-key', + reviewedGoal: null, }); await drain( @@ -591,7 +613,7 @@ describe('LlmClient Goal admission', () => { 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; @@ -628,7 +650,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(() => { @@ -680,6 +706,7 @@ describe('LlmClient Goal admission', () => { store.set({ objective: 'stale proposal', turnKey: 'failed-user-key', + reviewedGoal: null, }); yield { type: LlmEventType.Error, @@ -719,9 +746,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; @@ -754,6 +782,7 @@ describe('LlmClient Goal admission', () => { takePendingGoalProposal: vi.fn(() => ({ objective: 'ship it', turnKey: 'settle-key', + reviewedGoal: null, })), }); const appliedGoal = { @@ -820,6 +849,7 @@ describe('LlmClient Goal admission', () => { const store = pendingGoalProposalStore({ objective: 'approved earlier', turnKey: 'owner-key', + reviewedGoal: null, }); Object.assign(config, { takePendingGoalProposal: store.take, @@ -866,6 +896,7 @@ describe('LlmClient Goal admission', () => { expect(store.get()).toEqual({ objective: 'approved earlier', turnKey: 'owner-key', + reviewedGoal: null, }); expect(runtime.dispatch).not.toHaveBeenCalled(); @@ -889,6 +920,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); @@ -932,6 +964,7 @@ describe('LlmClient Goal admission', () => { const store = pendingGoalProposalStore({ objective: 'stale approval', turnKey: 'cancelled-key', + reviewedGoal: null, }); Object.assign(config, { takePendingGoalProposal: store.take, @@ -965,6 +998,7 @@ describe('LlmClient Goal admission', () => { const store = pendingGoalProposalStore({ objective: 'ship it', turnKey: 'owner-key', + reviewedGoal: null, }); Object.assign(config, { takePendingGoalProposal: store.take, @@ -1062,7 +1096,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(); }); @@ -1099,6 +1137,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/goals/goal-tools.test.ts b/packages/core/src/goals/goal-tools.test.ts index c25968509b7..cd85ebc74a9 100644 --- a/packages/core/src/goals/goal-tools.test.ts +++ b/packages/core/src/goals/goal-tools.test.ts @@ -1680,6 +1680,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 +1696,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 +1887,77 @@ 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, + 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 +1982,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()!); @@ -1955,6 +2035,7 @@ describe('ProposeGoalTool', () => { const applied = applyPendingGoalProposal(runtime, { objective, turnKey: 'user-turn-key', + reviewedGoal: { goalId: original.goalId, revision: original.revision }, }); await expect(resumed).resolves.toMatchObject({ @@ -1995,6 +2076,7 @@ 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); 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..0fc49095fd8 100644 --- a/packages/core/src/goals/goal-tools.ts +++ b/packages/core/src/goals/goal-tools.ts @@ -637,6 +637,7 @@ export interface ProposeGoalToolParams { */ export interface PendingGoalProposal { objective: string; + reviewedGoal: Pick | null; /** * 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 @@ -664,9 +665,8 @@ export type ApplyPendingGoalProposalResult = * 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, @@ -680,6 +680,9 @@ export async function applyPendingGoalProposal( reason: `A Goal became active (revision ${current.revision}) before the approved proposal could be set.`, }; } + if (!matchesReviewedGoal(current, proposal.reviewedGoal)) { + return { applied: false, reason: PROPOSE_GOAL_CHANGED_MESSAGE }; + } const request: GoalControlRequest = current ? { action: 'replace', @@ -740,6 +743,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 +773,7 @@ class ProposeGoalInvocation extends BaseToolInvocation< GoalToolResult > { private approved = false; + private reviewedGoal: PendingGoalProposal['reviewedGoal'] | undefined; constructor( params: ProposeGoalToolParams, @@ -766,9 +783,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 +857,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 +884,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 +904,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, diff --git a/packages/core/src/goals/index.ts b/packages/core/src/goals/index.ts index 7395fde648e..27008fb38fb 100644 --- a/packages/core/src/goals/index.ts +++ b/packages/core/src/goals/index.ts @@ -69,6 +69,8 @@ export * from './goal-checkpoint.js'; export * from './goal-checkpoint-verifier.js'; export * from './goal-verifier.js'; export * from './goal-runtime.js'; +export { applyPendingGoalProposal, 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..2362e856f55 100644 --- a/packages/core/src/tools/tool-registry.test.ts +++ b/packages/core/src/tools/tool-registry.test.ts @@ -396,6 +396,23 @@ 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: 'propose_goal' })); + expect(registry.getFunctionDeclarations()).toEqual([]); + acpConfig.setGoalProposalTurnKey('user-turn'); + expect( + registry.getFunctionDeclarations().map((tool) => tool.name), + ).toEqual(['propose_goal']); + acpConfig.setGoalProposalTurnKey(undefined); + expect(registry.getFunctionDeclarations()).toEqual([]); + }); + it('excludes shouldDefer tools from getFunctionDeclarations by default', () => { toolRegistry.registerTool(new MockTool({ name: 'visible' })); toolRegistry.registerTool( diff --git a/packages/core/src/tools/tool-registry.ts b/packages/core/src/tools/tool-registry.ts index 65b77409697..e8d1af6a9b6 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,11 @@ export class ToolRegistry { }): FunctionDeclaration[] { const includeDeferred = options?.includeDeferred === true; return Array.from(this.tools.values()) + .filter( + (tool) => + tool.name !== ToolNames.PROPOSE_GOAL || + this.config.isGoalProposalAvailable(), + ) .filter( (tool) => includeDeferred || From ea2dce77eb89bb77e34a7938238b738e520a9222 Mon Sep 17 00:00:00 2001 From: qqqys <266654365+qqqys@users.noreply.github.com> Date: Wed, 9 Sep 2026 10:53:50 +0800 Subject: [PATCH 2/7] fix(goal): close proposal lifecycle gaps --- docs/users/features/goals.md | 2 +- packages/acp-bridge/src/bridge.test.ts | 17 ++ packages/acp-bridge/src/bridge.ts | 5 +- packages/acp-bridge/src/bridgeTypes.ts | 7 +- .../cli/src/acp-integration/acpAgent.test.ts | 2 +- packages/cli/src/acp-integration/acpAgent.ts | 6 +- .../session/Session.review-lease.test.ts | 1 + .../acp-integration/session/Session.test.ts | 238 ++++++++++++++++++ .../src/acp-integration/session/Session.ts | 97 +++++-- .../session/Session.worktree.test.ts | 1 + packages/core/src/config/config.ts | 4 +- packages/core/src/core/client-goal.test.ts | 41 +++ packages/core/src/core/client.ts | 23 +- packages/core/src/tools/tool-registry.test.ts | 24 +- packages/core/src/tools/tool-registry.ts | 14 +- packages/core/src/tools/tool-search.test.ts | 23 ++ packages/core/src/tools/tool-search.ts | 4 + 17 files changed, 468 insertions(+), 41 deletions(-) diff --git a/docs/users/features/goals.md b/docs/users/features/goals.md index 55417c1f150..98b23975c76 100644 --- a/docs/users/features/goals.md +++ b/docs/users/features/goals.md @@ -79,7 +79,7 @@ Pass an existing objective to tighten it: `/goal-draft all tests pass and the li ### Approve a Goal the model proposes -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, channel deliveries, automatic turns, and ACP clients without the required approval and turn-lifecycle support retain the printed `/goal set` 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 cd7d88de1cc..cecbe8eee5c 100644 --- a/packages/acp-bridge/src/bridge.test.ts +++ b/packages/acp-bridge/src/bridge.test.ts @@ -15516,6 +15516,20 @@ describe('createAcpSessionBridge', () => { 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(); }); @@ -37196,9 +37210,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 ?? '', ); @@ -37245,6 +37261,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 78c38233b7e..51944858a46 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -10434,7 +10434,10 @@ export function createAcpSessionBridge(opts: BridgeOptions): AcpSessionBridge { delete meta['qwen.goalProposalApproval']; if ( originatorClientId !== undefined && - !context?.channelPrompt + entry.clientIds.has(originatorClientId) && + !context?.channelPrompt && + !isContinue && + !isRestoreAskUserQuestion ) { meta['qwen.goalProposalApproval'] = true; } diff --git a/packages/acp-bridge/src/bridgeTypes.ts b/packages/acp-bridge/src/bridgeTypes.ts index f9163cadd5c..a03f8505dd2 100644 --- a/packages/acp-bridge/src/bridgeTypes.ts +++ b/packages/acp-bridge/src/bridgeTypes.ts @@ -950,9 +950,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 1d161679f21..07dffd75b7a 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -3943,7 +3943,7 @@ describe('QwenAgent MCP SSE/HTTP support', () => { it.each([ [true, undefined, true], [false, undefined, false], - [true, 'channel', false], + [true, 'channel', true], ])( 'gates Goal proposals on client support %s and session source %s', async (supported, sourceType, expected) => { diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index ae8ae3acb25..61137cad303 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -13916,11 +13916,7 @@ class QwenAgent implements Agent { if (sessionSource) { config.setSessionSource(sessionSource.sourceType, sessionSource.sourceId); } - if ( - this.clientCapabilities?._meta?.['qwen.goalProposals'] === true && - sessionSource?.sourceType !== 'channel' && - !provisionalWorkspace - ) { + if (this.clientCapabilities?._meta?.['qwen.goalProposals'] === true) { config.setGoalProposalHostSupported(true); } if (chatRecording !== false) { 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 74983d9d415..aaf6c012139 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -491,6 +491,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; @@ -722,6 +723,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 +990,11 @@ describe('Session', () => { 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; @@ -32378,6 +32385,88 @@ describe('Session', () => { expect(mockGoalRuntime.dispatch).toHaveBeenCalledTimes(1); }); + 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) => { @@ -32433,6 +32522,28 @@ describe('Session', () => { 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).mockReturnValueOnce( + runtimeLoad, + ); + + const result = prompt(); + await vi.waitFor(() => + expect(mockConfig.getGoalRuntimeReady).toHaveBeenCalledOnce(), + ); + await session.cancelPendingPrompt(); + finishRuntimeLoad(); + + await expect(result).resolves.toEqual({ stopReason: 'cancelled' }); + expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); + expect(pending).toBeUndefined(); + }); + it('pauses a Goal if cancellation arrives while its creation is persisted', async () => { let completeCreate!: () => void; const gate = new Promise((resolve) => { @@ -32461,6 +32572,88 @@ describe('Session', () => { }); }); + 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(mockClient.sessionUpdate).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(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'agent_message_chunk', + content: expect.objectContaining({ + text: expect.stringContaining('/goal set'), + }), + }), + }), + ); + }); + it('holds a cancelled Goal when pause persistence fails, then permits a user pause and resume', async () => { const goal = { goalId: 'approved-goal', @@ -32522,6 +32715,17 @@ describe('Session', () => { }), ); + 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 }, + ); + const paused = { ...snapshot, goal: { ...goal, status: 'paused' as const }, @@ -32581,6 +32785,16 @@ describe('Session', () => { 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'), + }), + }), + }), + ); }); it('discards approval when new user input is injected after the tool result', async () => { @@ -32599,6 +32813,16 @@ describe('Session', () => { 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'), + }), + }), + }), + ); }); it('refuses proposals from an automatic background-notification turn', async () => { @@ -32627,6 +32851,20 @@ describe('Session', () => { 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 () => { diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index c373378c121..96fdd314a86 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -1510,6 +1510,7 @@ interface AgentResponseCapture { turnKey: string; controller: AbortController; completedNormally: boolean; + settlementBlocked?: boolean; }; channelDelivery?: { finalText: string; @@ -4703,7 +4704,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. Run /goal pause, then /goal resume when ready.', + ); + } else if (activeGoal?.status === 'active') { reservedGoalRuntime = runtime; reservedGoalTurnKey = `goal-user:${randomUUID()}`; runtime.beginTurn(reservedGoalTurnKey); @@ -4945,6 +4958,14 @@ export class Session implements SessionContext { } : result; promptResult = completedResult; + const proposalTurn = responseCapture.goalProposalTurn; + if (proposalTurn) { + proposalTurn.completedNormally = + completedResult.stopReason === 'end_turn' && + result.loopProtectionStopped !== true && + !pendingSend.signal.aborted && + proposalTurn.settlementBlocked !== true; + } await this.#settleGoalProposal(responseCapture); releasePendingSend(); // Drain any cron prompts that queued while the prompt was active @@ -4982,8 +5003,10 @@ export class Session implements SessionContext { this.activeGoalProposalTurn = undefined; } } - if (this.config.getGoalProposalHostSupported()) { - this.config.setGoalProposalTurnKey(undefined); + if ( + this.config.getGoalProposalHostSupported() && + this.config.setGoalProposalTurnKey(undefined) + ) { try { await this.config.getLlmClient().setTools(); } catch (error) { @@ -5213,7 +5236,7 @@ export class Session implements SessionContext { rejectOnLoopDetected = false, goalTurn?: AcpGoalTurn, channelTurn = false, - ): Promise { + ): Promise { const sessionId = this.config.getSessionId(); if ( invocationContext !== undefined && @@ -5259,10 +5282,24 @@ export class Session implements SessionContext { this.activeGoalProposalTurn === turn && !this.disposed && !this.closing; - if (!ownsTurn()) return; + if (!ownsTurn()) { + if (!this.disposed && !this.closing) { + await this.messageEmitter.emitAgentMessage( + 'The approved Goal was not started because the turn did not finish normally. Run `/goal set ` if you still want to start it.', + ); + } + return; + } try { const runtime = await this.config.getGoalRuntimeReady(); - if (!ownsTurn()) return; + if (!ownsTurn()) { + if (!this.disposed && !this.closing) { + await this.messageEmitter.emitAgentMessage( + 'The approved Goal was not started because the turn did not finish normally. Run `/goal set ` if you still want to start it.', + ); + } + 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. @@ -5272,7 +5309,10 @@ export class Session implements SessionContext { action: 'pause', expectedGoalId: result.goal.goalId, expectedRevision: result.goal.revision, - reason: GOAL_PAUSE_REASON_USER_INTERRUPT, + reason: + this.closing || this.disposed + ? GOAL_PAUSE_REASON_SESSION_DISPOSED + : GOAL_PAUSE_REASON_USER_INTERRUPT, }); } catch (error) { const current = runtime.getSnapshot().goal; @@ -5296,13 +5336,13 @@ export class Session implements SessionContext { } } else if (!result.applied) { await this.messageEmitter.emitAgentMessage( - `The approved Goal could not be started: ${result.reason}`, + 'The approved Goal could not be started. Check /goal before trying again, or run `/goal set `.', ); } } catch (error) { debugLogger.warn('Failed to apply an approved Goal proposal', error); await this.messageEmitter.emitAgentMessage( - 'The approved Goal could not be started. Check /goal before trying again.', + 'The approved Goal could not be started. Check /goal before trying again, or run `/goal set `.', ); } } @@ -5315,12 +5355,14 @@ export class Session implements SessionContext { rejectOnLoopDetected = false, goalTurn?: AcpGoalTurn, channelTurn = false, - ): Promise { + ): Promise { let managedMemoryRecallStarted = false; return Storage.runWithRuntimeBaseDir( this.runtimeBaseDir, this.config.getWorkingDir(), - async (): Promise => { + async (): Promise< + PromptResponse & { loopProtectionStopped?: boolean } + > => { await this.assertCanStartTurn(); if (pendingSend.signal.aborted) { return { stopReason: 'cancelled' }; @@ -5341,10 +5383,12 @@ export class Session implements SessionContext { }; this.activeGoalProposalTurn = responseCapture.goalProposalTurn; } - if (this.config.getGoalProposalHostSupported()) { + if ( + this.config.getGoalProposalHostSupported() && this.config.setGoalProposalTurnKey( responseCapture.goalProposalTurn?.turnKey, - ); + ) + ) { await this.config.getLlmClient().setTools(); } const promptMetadata = (params as { _meta?: Record }) @@ -6468,6 +6512,11 @@ export class Session implements SessionContext { modelOverride = model; return true; }; + const blockGoalProposalSettlement = () => { + if (responseCapture?.goalProposalTurn) { + responseCapture.goalProposalTurn.settlementBlocked = true; + } + }; let midTurnContinuationCount = 0; while (true) { @@ -6484,6 +6533,7 @@ export class Session implements SessionContext { } if (this.todoStopGuardQueuedPromptPriority) { + blockGoalProposalSettlement(); return { stopReason: 'end_turn' }; } @@ -6498,6 +6548,7 @@ export class Session implements SessionContext { pendingSend.signal, ); if (claim === 'queued') { + blockGoalProposalSettlement(); this.#preserveUnsentMessageHistory( { role: 'user', parts: drained.parts }, true, @@ -6509,6 +6560,7 @@ export class Session implements SessionContext { } } this.todoStopGuard.acceptMidTurnUserInput(); + blockGoalProposalSettlement(); const continuation = await this.#runStopContinuation( pendingSend, promptId + '_mid_turn_' + ++midTurnContinuationCount, @@ -6536,6 +6588,7 @@ export class Session implements SessionContext { pendingSend.signal, ); if (claim === 'queued') { + blockGoalProposalSettlement(); return { stopReason: 'end_turn' }; } if (claim === 'unavailable') { @@ -6600,6 +6653,7 @@ export class Session implements SessionContext { pendingSend.signal, ); if (claim === 'queued') { + blockGoalProposalSettlement(); this.#preserveUnsentMessageHistory( { role: 'user', parts: drained.parts }, true, @@ -6611,6 +6665,7 @@ export class Session implements SessionContext { } } this.todoStopGuard.acceptMidTurnUserInput(); + blockGoalProposalSettlement(); const continuation = await this.#runStopContinuation( pendingSend, promptId + '_mid_turn_' + ++midTurnContinuationCount, @@ -6638,6 +6693,7 @@ export class Session implements SessionContext { pendingSend.signal, ); if (claim === 'queued') { + blockGoalProposalSettlement(); return { stopReason: 'end_turn' }; } if (claim === 'unavailable') { @@ -6684,7 +6740,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) { @@ -6707,15 +6766,13 @@ export class Session implements SessionContext { await this.#pauseGoalForStopHookCap(); } this.todoStopGuard.suspend(); + blockGoalProposalSettlement(); await this.messageEmitter.emitAgentMessage(warning); debugLogger.warn(warning); return { stopReason: 'end_turn' }; } if (!externalReason && !guardContinuation) { - if (responseCapture?.goalProposalTurn?.turnKey === promptId) { - responseCapture.goalProposalTurn.completedNormally = true; - } return { stopReason: 'end_turn' }; } @@ -8716,7 +8773,7 @@ export class Session implements SessionContext { messages.length > 0 && proposalTurn?.controller.signal === abortSignal ) { - this.config.takePendingGoalProposal(proposalTurn.turnKey); + proposalTurn.settlementBlocked = true; } const parts: Part[] = []; for (const message of messages) { @@ -12064,7 +12121,9 @@ export class Session implements SessionContext { this.activeGoalProposalTurn.controller.signal.aborted) ) { return earlyErrorResponse( - new Error('Goal proposals require an interactive user turn.'), + 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', 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/core/src/config/config.ts b/packages/core/src/config/config.ts index e173cc3f39c..280c5981a02 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -8034,8 +8034,10 @@ export class Config { return this.goalProposalHostSupported; } - setGoalProposalTurnKey(turnKey: string | undefined): void { + setGoalProposalTurnKey(turnKey: string | undefined): boolean { + if (this.goalProposalTurnKey === turnKey) return false; this.goalProposalTurnKey = turnKey; + return true; } isGoalProposalAvailable(): boolean { diff --git a/packages/core/src/core/client-goal.test.ts b/packages/core/src/core/client-goal.test.ts index 43179ca11f1..a376a12f250 100644 --- a/packages/core/src/core/client-goal.test.ts +++ b/packages/core/src/core/client-goal.test.ts @@ -341,6 +341,47 @@ describe('LlmClient Goal admission', () => { }); }); + 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.HookSystemMessage, + value: + 'The approved Goal could not be started. Check /goal before trying again, or run `/goal set `.', + }); + }); + it('settles an approved proposal on the default skip-next-speaker exit', async () => { const { client, config, runtime } = setupGoalClient(); vi.mocked(runtime.getSnapshot).mockReturnValue({ diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index 0c3ef8c4388..ee8d8845bf6 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -899,6 +899,7 @@ export class LlmClient { signal: AbortSignal, loadGoalRuntime: (required: boolean) => Promise, turnKey: string, + reportFailure?: (message: string) => void, ): Promise { const take = this.config.takePendingGoalProposal; if (typeof take !== 'function') return; @@ -933,6 +934,9 @@ export class LlmClient { } if (!result.applied) { debugLogger.debug(`Dropping an approved Goal proposal: ${result.reason}`); + reportFailure?.( + 'The approved Goal could not be started. Check /goal before trying again, or run `/goal set `.', + ); } } @@ -2902,6 +2906,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.HookSystemMessage, + value: message, + }); + }; let hasEmittedActiveGoalProjection = false; let lastEmittedActiveGoal: ActiveGoalEventValue | undefined; const closeGoalStateEvents = () => { @@ -2924,7 +2935,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, @@ -3283,6 +3297,7 @@ export class LlmClient { return runtime; }, prompt_id, + reportGoalSettlementFailure, ); for (const goalEvent of takePendingGoalEvents()) { yield goalEvent; @@ -4581,6 +4596,7 @@ export class LlmClient { signal, loadGoalRuntime, prompt_id, + reportGoalSettlementFailure, ); for (const goalEvent of takePendingGoalEvents()) { yield goalEvent; @@ -4661,6 +4677,7 @@ export class LlmClient { signal, loadGoalRuntime, prompt_id, + reportGoalSettlementFailure, ); for (const goalEvent of takePendingGoalEvents()) { yield goalEvent; @@ -4718,6 +4735,7 @@ export class LlmClient { signal, loadGoalRuntime, prompt_id, + reportGoalSettlementFailure, ); for (const goalEvent of takePendingGoalEvents()) { yield goalEvent; @@ -4798,6 +4816,7 @@ export class LlmClient { signal, loadGoalRuntime, prompt_id, + reportGoalSettlementFailure, ); for (const goalEvent of takePendingGoalEvents()) { yield goalEvent; @@ -4855,6 +4874,7 @@ export class LlmClient { signal, loadGoalRuntime, prompt_id, + reportGoalSettlementFailure, ); for (const goalEvent of takePendingGoalEvents()) { yield goalEvent; @@ -4898,6 +4918,7 @@ export class LlmClient { signal, loadGoalRuntime, prompt_id, + reportGoalSettlementFailure, ); for (const goalEvent of takePendingGoalEvents()) { yield goalEvent; diff --git a/packages/core/src/tools/tool-registry.test.ts b/packages/core/src/tools/tool-registry.test.ts index 2362e856f55..a10fef85db9 100644 --- a/packages/core/src/tools/tool-registry.test.ts +++ b/packages/core/src/tools/tool-registry.test.ts @@ -403,14 +403,32 @@ describe('ToolRegistry', () => { }); 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()).toEqual([]); + expect( + registry.getFunctionDeclarations().map((tool) => tool.name), + ).toEqual(['other_tool']); acpConfig.setGoalProposalTurnKey('user-turn'); expect( registry.getFunctionDeclarations().map((tool) => tool.name), - ).toEqual(['propose_goal']); + ).toEqual(['other_tool', 'propose_goal']); acpConfig.setGoalProposalTurnKey(undefined); - expect(registry.getFunctionDeclarations()).toEqual([]); + 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', () => { diff --git a/packages/core/src/tools/tool-registry.ts b/packages/core/src/tools/tool-registry.ts index e8d1af6a9b6..4f03f5154df 100644 --- a/packages/core/src/tools/tool-registry.ts +++ b/packages/core/src/tools/tool-registry.ts @@ -821,11 +821,7 @@ export class ToolRegistry { }): FunctionDeclaration[] { const includeDeferred = options?.includeDeferred === true; return Array.from(this.tools.values()) - .filter( - (tool) => - tool.name !== ToolNames.PROPOSE_GOAL || - this.config.isGoalProposalAvailable(), - ) + .filter((tool) => this.isToolDeclared(tool.name)) .filter( (tool) => includeDeferred || @@ -1025,13 +1021,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) From 40e2ddf86ce3858326769abe153db78acc51b33b Mon Sep 17 00:00:00 2001 From: qqqys <266654365+qqqys@users.noreply.github.com> Date: Wed, 9 Sep 2026 17:33:35 +0800 Subject: [PATCH 3/7] fix(goal): address settlement review gaps --- .../acp-integration/session/Session.test.ts | 171 ++++++++++++++++-- .../src/acp-integration/session/Session.ts | 27 ++- .../cli/src/ui/hooks/use-llm-stream.test.tsx | 31 ++++ packages/cli/src/ui/hooks/use-llm-stream.ts | 16 ++ .../cli/src/ui/opentui/event-adapter.test.ts | 14 ++ packages/cli/src/ui/opentui/event-adapter.ts | 5 + packages/core/src/core/client-goal.test.ts | 97 +++++++++- packages/core/src/core/client.ts | 11 +- .../core/src/core/coreToolScheduler.test.ts | 29 +++ packages/core/src/core/coreToolScheduler.ts | 4 +- packages/core/src/core/turn.ts | 7 + packages/core/src/goals/goal-tools.ts | 6 + packages/core/src/goals/index.ts | 7 +- packages/core/src/tools/tool-registry.test.ts | 32 ++++ packages/core/src/tools/tool-registry.ts | 1 + 15 files changed, 427 insertions(+), 31 deletions(-) diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index aaf6c012139..3debff388c5 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -70,6 +70,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()); @@ -32266,18 +32267,22 @@ describe('Session', () => { ]); } - function prompt() { - return session.prompt({ - sessionId: 'test-session-id', - _meta: { 'qwen.goalProposalApproval': true }, - prompt: [ - { type: 'text', text: 'Draft a Goal to make the tests pass.' }, - ], - }); + 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; @@ -32495,6 +32500,10 @@ describe('Session', () => { 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 () => { @@ -32528,18 +32537,21 @@ describe('Session', () => { finishRuntimeLoad = () => resolve(mockGoalRuntime as unknown as core.GoalRuntime); }); - vi.mocked(mockConfig.getGoalRuntimeReady).mockReturnValueOnce( - runtimeLoad, - ); + 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).toHaveBeenCalledOnce(), + expect(mockConfig.getGoalRuntimeReady).toHaveBeenCalledTimes(3), ); + expect(mockConfig.setPendingGoalProposal).toHaveBeenCalledOnce(); await session.cancelPendingPrompt(); finishRuntimeLoad(); - await expect(result).resolves.toEqual({ stopReason: 'cancelled' }); + await expect(result).resolves.toEqual({ stopReason: 'end_turn' }); expect(mockGoalRuntime.dispatch).not.toHaveBeenCalled(); expect(pending).toBeUndefined(); }); @@ -32623,12 +32635,28 @@ describe('Session', () => { 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 set'), + text: expect.stringContaining( + 'The approved Goal could not be started', + ), + }), + }), + }), + ); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + content: expect.objectContaining({ + text: expect.stringContaining(objective), }), }), }), @@ -32642,12 +32670,22 @@ describe('Session', () => { await prompt(); + expect(mockGoalRuntime.dispatch).toHaveBeenCalledOnce(); expect(mockClient.sessionUpdate).toHaveBeenCalledWith( expect.objectContaining({ update: expect.objectContaining({ sessionUpdate: 'agent_message_chunk', content: expect.objectContaining({ - text: expect.stringContaining('/goal set'), + text: expect.stringContaining('Check /goal before trying again'), + }), + }), + }), + ); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + content: expect.objectContaining({ + text: expect.stringContaining(objective), }), }), }), @@ -32725,6 +32763,18 @@ describe('Session', () => { 'goal-runtime:cancelled-start', { requeue: false }, ); + expect(mockClient.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'agent_message_chunk', + content: expect.objectContaining({ + text: expect.stringContaining( + 'Automatic Goal execution is still held', + ), + }), + }), + }), + ); const paused = { ...snapshot, @@ -32795,6 +32845,88 @@ describe('Session', () => { }), }), ); + 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; + }; + mockConfig.setPendingGoalProposal = vi.fn((proposal) => { + pending = proposal; + internals.todoStopGuardQueuedPromptPriority = true; + return true; + }); + + 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 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 () => { @@ -32823,6 +32955,15 @@ describe('Session', () => { }), }), ); + 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 () => { diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 96fdd314a86..01fcf0f2f78 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -187,6 +187,8 @@ import { GOAL_PAUSE_REASON_STOP_HOOK_CAP, GOAL_PAUSE_REASON_USER_INTERRUPT, applyPendingGoalProposal, + formatProposeGoalRecoveryFailed, + formatProposeGoalRecoveryNotStarted, goalPauseReasonForFailure, ambientGoalToolResultProvenance, goalTurnContext, @@ -4947,7 +4949,7 @@ export class Session implements SessionContext { } const completedResult: PromptResponse = branchPoint ? { - ...result, + stopReason: result.stopReason, _meta: { ...result._meta, 'qwen.branchPoint': { @@ -4956,7 +4958,10 @@ export class Session implements SessionContext { }, }, } - : result; + : { + stopReason: result.stopReason, + ...(result._meta ? { _meta: result._meta } : {}), + }; promptResult = completedResult; const proposalTurn = responseCapture.goalProposalTurn; if (proposalTurn) { @@ -5285,7 +5290,7 @@ export class Session implements SessionContext { if (!ownsTurn()) { if (!this.disposed && !this.closing) { await this.messageEmitter.emitAgentMessage( - 'The approved Goal was not started because the turn did not finish normally. Run `/goal set ` if you still want to start it.', + formatProposeGoalRecoveryNotStarted(proposal.objective), ); } return; @@ -5295,7 +5300,7 @@ export class Session implements SessionContext { if (!ownsTurn()) { if (!this.disposed && !this.closing) { await this.messageEmitter.emitAgentMessage( - 'The approved Goal was not started because the turn did not finish normally. Run `/goal set ` if you still want to start it.', + formatProposeGoalRecoveryNotStarted(proposal.objective), ); } return; @@ -5335,14 +5340,17 @@ export class Session implements SessionContext { } } } else if (!result.applied) { + debugLogger.debug( + `Dropping an approved Goal proposal: ${result.reason}`, + ); await this.messageEmitter.emitAgentMessage( - 'The approved Goal could not be started. Check /goal before trying again, or run `/goal set `.', + formatProposeGoalRecoveryFailed(proposal.objective), ); } } catch (error) { debugLogger.warn('Failed to apply an approved Goal proposal', error); await this.messageEmitter.emitAgentMessage( - 'The approved Goal could not be started. Check /goal before trying again, or run `/goal set `.', + formatProposeGoalRecoveryFailed(proposal.objective), ); } } @@ -6449,7 +6457,12 @@ export class Session implements SessionContext { responseCapture, isFreshUserTurn, ); - return { stopReason: result.stopReason }; + return { + stopReason: result.stopReason, + ...(result.loopProtectionStopped + ? { loopProtectionStopped: true } + : {}), + }; } finally { logConversationFinishedEvent( this.config, 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 22d0c912e27..7ceb5a5e681 100644 --- a/packages/cli/src/ui/hooks/use-llm-stream.test.tsx +++ b/packages/cli/src/ui/hooks/use-llm-stream.test.tsx @@ -19155,6 +19155,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 4815417326e..095cd6031d7 100644 --- a/packages/cli/src/ui/hooks/use-llm-stream.ts +++ b/packages/cli/src/ui/hooks/use-llm-stream.ts @@ -2965,6 +2965,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 af68e472cf7..d1c56f9dde4 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 ca7e415d29f..c997022b0f0 100644 --- a/packages/cli/src/ui/opentui/event-adapter.ts +++ b/packages/cli/src/ui/opentui/event-adapter.ts @@ -705,6 +705,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/core/client-goal.test.ts b/packages/core/src/core/client-goal.test.ts index a376a12f250..a9fd632279e 100644 --- a/packages/core/src/core/client-goal.test.ts +++ b/packages/core/src/core/client-goal.test.ts @@ -376,9 +376,9 @@ describe('LlmClient Goal admission', () => { ); expect(events).toContainEqual({ - type: LlmEventType.HookSystemMessage, + type: LlmEventType.GoalSettlementFailed, value: - 'The approved Goal could not be started. Check /goal before trying again, or run `/goal set `.', + 'The approved Goal could not be started. Check /goal before trying again, or run /goal set ship it.', }); }); @@ -475,6 +475,55 @@ 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 /goal before trying again, or run /goal set ship it.', + }); + }); + it('settles an approved proposal when a cleared Goal removes the Stop continuation', async () => { const { client, config, runtime } = setupGoalClient(); vi.mocked(runtime.getSnapshot).mockReturnValue({ @@ -650,6 +699,50 @@ describe('LlmClient Goal admission', () => { }); }); + it('reports a failed proposal settlement from the ToolResult exit', 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({ + 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 /goal before trying again, or run /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); diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index ee8d8845bf6..84b0d2755ee 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -53,7 +53,10 @@ import { getStopHookContinuationReason, GOAL_HOOK_ID_OUTPUT_KEY, } from '../goals/goalHook.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'; @@ -934,9 +937,7 @@ export class LlmClient { } if (!result.applied) { debugLogger.debug(`Dropping an approved Goal proposal: ${result.reason}`); - reportFailure?.( - 'The approved Goal could not be started. Check /goal before trying again, or run `/goal set `.', - ); + reportFailure?.(formatProposeGoalRecoveryFailed(proposal.objective)); } } @@ -2909,7 +2910,7 @@ export class LlmClient { const pendingGoalSettlementMessages: ServerLlmStreamEvent[] = []; const reportGoalSettlementFailure = (message: string) => { pendingGoalSettlementMessages.push({ - type: LlmEventType.HookSystemMessage, + type: LlmEventType.GoalSettlementFailed, value: message, }); }; diff --git a/packages/core/src/core/coreToolScheduler.test.ts b/packages/core/src/core/coreToolScheduler.test.ts index f9badb5b2bd..1ede59a5cf8 100644 --- a/packages/core/src/core/coreToolScheduler.test.ts +++ b/packages/core/src/core/coreToolScheduler.test.ts @@ -7126,6 +7126,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 5449cb622dd..83fc2745f85 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 f7c561f1d38..dba2bcb4366 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.ts b/packages/core/src/goals/goal-tools.ts index 0fc49095fd8..0560b709cca 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. Run /goal set ${objective} if you still want to start it.`; + +export const formatProposeGoalRecoveryFailed = (objective: string) => + `The approved Goal could not be started. Check /goal before trying again, or run /goal set ${objective}.`; + export interface ProposeGoalToolParams { objective: string; } diff --git a/packages/core/src/goals/index.ts b/packages/core/src/goals/index.ts index 9b191d10e37..7ff1091b37e 100644 --- a/packages/core/src/goals/index.ts +++ b/packages/core/src/goals/index.ts @@ -69,7 +69,12 @@ export * from './goal-checkpoint.js'; export * from './goal-checkpoint-verifier.js'; export * from './goal-verifier.js'; export * from './goal-runtime.js'; -export { applyPendingGoalProposal, ProposeGoalTool } from './goal-tools.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 { diff --git a/packages/core/src/tools/tool-registry.test.ts b/packages/core/src/tools/tool-registry.test.ts index a10fef85db9..8e78ed79831 100644 --- a/packages/core/src/tools/tool-registry.test.ts +++ b/packages/core/src/tools/tool-registry.test.ts @@ -408,10 +408,20 @@ describe('ToolRegistry', () => { 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), @@ -832,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 4f03f5154df..3510e9fa92e 100644 --- a/packages/core/src/tools/tool-registry.ts +++ b/packages/core/src/tools/tool-registry.ts @@ -925,6 +925,7 @@ export class ToolRegistry { if ( this.isEffectivelyDeferred(tool) && !tool.alwaysLoad && + this.isToolDeclared(tool.name) && !this.config.getVisibleTools().has(tool.name) ) { summary.push({ From 8893cf05a12e309cc3fc3a9a1820d76f7439d5be Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Thu, 10 Sep 2026 02:36:57 +0000 Subject: [PATCH 4/7] fix(goal): block proposal settlement when a guard continuation yields to a queued prompt (#11360) #runStopContinuation's skipped-send funnel returned a plain end_turn for the queued-prompt and unreliable-drain stops without marking the turn's settlementBlocked flag, so a parked proposal approved earlier in the turn was applied even though the turn ceded to the queued user prompt. Set the flag at those three stops, mirroring the loop-level exits, and pin each with a regression that fails without it. Co-authored-by: Qwen-Coder --- .../acp-integration/session/Session.test.ts | 144 ++++++++++++++++++ .../src/acp-integration/session/Session.ts | 12 ++ 2 files changed, 156 insertions(+) diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 17e064467bd..e0eb7d997eb 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -33664,6 +33664,150 @@ describe('Session', () => { 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: { diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 523c8ec62ee..0843b5637a4 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -6911,6 +6911,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) { @@ -7044,6 +7053,7 @@ export class Session implements SessionContext { nextGuardContinuation = undefined; if (!options.externalParts) { preserveGuardOnSkippedSend = true; + blockGoalProposalSettlement(); return { kind: 'stop', stopReason: 'end_turn' }; } if (!initialSend && nextMessage) { @@ -7066,6 +7076,7 @@ export class Session implements SessionContext { guardForThisSend = undefined; nextGuardContinuation = undefined; preserveGuardOnSkippedSend = true; + blockGoalProposalSettlement(); if (initialSend) { supersededAutomaticContinuation = true; } @@ -7142,6 +7153,7 @@ export class Session implements SessionContext { guardForThisSend = undefined; nextGuardContinuation = undefined; preserveGuardOnSkippedSend = true; + blockGoalProposalSettlement(); if (initialSend) { supersededAutomaticContinuation = true; } From 1248cfd69857b12e2661a7f3f95029b1d18311a0 Mon Sep 17 00:00:00 2001 From: qwen-code-ci-bot Date: Thu, 10 Sep 2026 11:07:33 +0000 Subject: [PATCH 5/7] test(cli): deflake the ab-drive run-dir sweep counts against foreign temp dirs (#11360) Co-authored-by: Qwen-Coder --- .../cli/src/commands/review/ab-drive.test.ts | 146 +++++++++++++----- 1 file changed, 104 insertions(+), 42 deletions(-) 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', () => { From aeadac2afb7295cfd6f693407f2dce796ef262d0 Mon Sep 17 00:00:00 2001 From: qqqys <266654365+qqqys@users.noreply.github.com> Date: Thu, 10 Sep 2026 20:01:54 +0800 Subject: [PATCH 6/7] fix(goal): revoke proposal approval when entering Plan mode --- .../acp-integration/session/Session.test.ts | 101 +++++++++++ .../src/acp-integration/session/Session.ts | 2 +- packages/core/src/config/config.test.ts | 2 + packages/core/src/config/config.ts | 15 +- .../src/core/client-goal-plan-switch.test.ts | 157 ++++++++++++++++++ packages/core/src/core/client.ts | 5 +- packages/core/src/goals/goal-tools.ts | 10 ++ 7 files changed, 289 insertions(+), 3 deletions(-) create mode 100644 packages/core/src/core/client-goal-plan-switch.test.ts diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index e0eb7d997eb..0d5a00134d2 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -33010,6 +33010,27 @@ describe('Session', () => { 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([ { @@ -33150,6 +33171,86 @@ describe('Session', () => { 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); diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 0843b5637a4..805e815c75a 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -5342,7 +5342,7 @@ export class Session implements SessionContext { 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()) { + if (result.applied && (!ownsTurn() || proposal.approvalSignal?.aborted)) { try { await runtime.dispatch({ action: 'pause', diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 016d1dcdf07..d6a032b3308 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -3359,6 +3359,7 @@ describe('Server Config (config.ts)', () => { objective: 'first', turnKey: 'turn-1', reviewedGoal: null, + approvalSignal: expect.any(AbortSignal), }); expect(config.hasPendingGoalProposal()).toBe(false); expect(config.takePendingGoalProposal()).toBeUndefined(); @@ -3374,6 +3375,7 @@ describe('Server Config (config.ts)', () => { objective: 'explicitly cleared', turnKey: 'turn-3', reviewedGoal: null, + approvalSignal: expect.any(AbortSignal), }); expect(config.hasPendingGoalProposal()).toBe(false); }); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index abd875d0e52..65ead5db774 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -2379,6 +2379,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 @@ -7328,6 +7329,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'; @@ -8148,7 +8152,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; } @@ -8989,6 +9000,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; 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.ts b/packages/core/src/core/client.ts index 9ed63750c73..34569f4ea92 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -909,7 +909,10 @@ export class LlmClient { } 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', diff --git a/packages/core/src/goals/goal-tools.ts b/packages/core/src/goals/goal-tools.ts index 398146e0f99..cb7122eb68d 100644 --- a/packages/core/src/goals/goal-tools.ts +++ b/packages/core/src/goals/goal-tools.ts @@ -644,6 +644,8 @@ 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 @@ -678,6 +680,14 @@ 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') { From 60e2c85e3b034feafe21e3636b43837ff1cc15ba Mon Sep 17 00:00:00 2001 From: qqqys <266654365+qqqys@users.noreply.github.com> Date: Thu, 10 Sep 2026 22:39:27 +0800 Subject: [PATCH 7/7] fix(goal): give ACP proposal settlement one owner --- .../acp-integration/session/Session.test.ts | 24 ++++++++ packages/core/src/core/client-goal.test.ts | 59 +++++++++++++++++++ packages/core/src/core/client.ts | 4 ++ 3 files changed, 87 insertions(+) diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 88577005ac7..a9818335b00 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -33769,11 +33769,35 @@ describe('Session', () => { 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' }); diff --git a/packages/core/src/core/client-goal.test.ts b/packages/core/src/core/client-goal.test.ts index 19f6e275430..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), @@ -330,6 +331,64 @@ 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); diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index 34569f4ea92..d41c0bc8acf 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -886,6 +886,8 @@ 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, @@ -894,6 +896,7 @@ export class LlmClient { 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; @@ -905,6 +908,7 @@ export class LlmClient { debugLogger.debug( 'Dropping an approved Goal proposal: the Goal runtime is unavailable', ); + reportFailure(formatProposeGoalRecoveryFailed(proposal.objective)); return; } if (signal.aborted) return;