From c4da8d086d53e0e6a1dafc7209daefe9d319794e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=98=93=E8=89=AF?= <1204183885@qq.com> Date: Sun, 19 Jul 2026 19:29:52 +0800 Subject: [PATCH] fix(cli): allow goal controls during active loops (#7202) * fix(cli): allow goal control during active loops * fix(cli): preserve active goal control state * fix(cli): share active goal command matching * test(core): cover goal continuation fallback * test(core): cover goal hook continuation reasons --- docs/design/goal-loop-input-control.md | 48 +++ .../acp-integration/session/Session.test.ts | 51 +++ .../src/acp-integration/session/Session.ts | 3 +- .../cli/src/ui/hooks/useGeminiStream.test.tsx | 326 ++++++++++++++++++ packages/cli/src/ui/hooks/useGeminiStream.ts | 79 ++++- .../cli/src/ui/hooks/useMessageQueue.test.ts | 39 +++ packages/cli/src/ui/hooks/useMessageQueue.ts | 7 +- packages/core/src/config/config.test.ts | 73 ++++ packages/core/src/config/config.ts | 31 ++ packages/core/src/confirmation-bus/types.ts | 4 + packages/core/src/core/client.test.ts | 254 ++++++++++++++ packages/core/src/core/client.ts | 65 +++- packages/core/src/goals/goalHook.test.ts | 43 ++- packages/core/src/goals/goalHook.ts | 19 + packages/core/src/goals/index.ts | 1 + 15 files changed, 1017 insertions(+), 26 deletions(-) create mode 100644 docs/design/goal-loop-input-control.md diff --git a/docs/design/goal-loop-input-control.md b/docs/design/goal-loop-input-control.md new file mode 100644 index 0000000000..1a8183c9b7 --- /dev/null +++ b/docs/design/goal-loop-input-control.md @@ -0,0 +1,48 @@ +# Goal loop input control + +## Problem + +An active `/goal` is implemented as a blocking Stop hook. While the model is +running, the interactive queue normally defers slash commands until the stream +becomes idle. A goal loop may never reach that idle boundary, so `/goal clear` +and replacement `/goal` commands cannot take effect. + +The Stop response can also aggregate the goal hook with unrelated configured +hooks. Clearing a goal must not discard a blocking decision owned by another +hook. + +## Design + +During an active turn, the message queue drains `/goal` commands alongside +plain-text steering messages. Other slash commands remain queued for normal +idle processing. + +The CLI executes drained goal commands through the existing slash-command +processor: + +- Clear commands apply their side effect without producing model input. +- Replacement commands replace the pending goal instruction. +- When multiple goal commands are drained together, only the instruction for + the final active goal is sent. +- The surviving instruction keeps its position relative to plain-text steering + messages. +- Executed goal commands are not restored if later steering preparation is + cancelled; unexecuted plain-text messages are restored. + +Core samples the queue before Stop hooks and again after a blocking Stop hook +returns. A blocking goal output carries its goal hook ID and keeps its +continuation reason separate from ordinary hook reasons. The hook bridge also +reports whether another Stop output is blocking. If the goal changes at the +second boundary, core removes only the old goal continuation; it still follows +an independent blocking reason. Non-blocking hook outputs do not force an extra +goal iteration. + +## Verification + +- Queue tests cover active-turn goal draining and idle-boundary deferral. +- CLI stream tests cover clear, replacement, batched commands, ordering, and + restore behavior. +- Core tests cover clear and replacement during Stop-hook evaluation, including + an aggregated independent blocker. +- A local tmux session exercises clear and replacement against the built + interactive CLI. diff --git a/packages/cli/src/acp-integration/session/Session.test.ts b/packages/cli/src/acp-integration/session/Session.test.ts index 52effc5215..a550fd4b8b 100644 --- a/packages/cli/src/acp-integration/session/Session.test.ts +++ b/packages/cli/src/acp-integration/session/Session.test.ts @@ -11252,6 +11252,57 @@ describe('Session', () => { ); }); + it('preserves goal feedback alongside an external stop reason', async () => { + const messageBus = { + request: vi + .fn() + .mockResolvedValueOnce({ + success: true, + output: { + decision: 'block', + continue: false, + stopReason: 'External stop hook feedback', + reason: 'Keep working on the active goal', + hookSpecificOutput: { + qwenGoalHookId: 'goal-hook', + }, + }, + }) + .mockResolvedValueOnce({ + success: true, + output: {}, + }), + }; + mockConfig.getMessageBus = vi.fn().mockReturnValue(messageBus); + mockConfig.getDisableAllHooks = vi.fn().mockReturnValue(false); + mockConfig.hasHooksForEvent = vi + .fn() + .mockImplementation((eventName: string) => eventName === 'Stop'); + mockChat.getHistory = vi + .fn() + .mockReturnValue([ + { role: 'model', parts: [{ text: 'response text' }] }, + ]); + mockChat.getLastModelMessageText = vi + .fn() + .mockReturnValue('response text'); + mockChat.sendMessageStream = vi + .fn() + .mockResolvedValue(createEmptyStream()); + + await session.prompt({ + sessionId: 'test-session-id', + prompt: [{ type: 'text', text: 'hello' }], + }); + + expect(mockChat.sendMessageStream).toHaveBeenCalledTimes(2); + const continuation = vi.mocked(mockChat.sendMessageStream).mock + .calls[1]?.[1] as { message: Part[] }; + expect(textParts(continuation.message)).toEqual([ + 'External stop hook feedback\nKeep working on the active goal', + ]); + }); + it('ends Stop hook continuation when the blocking cap is reached', async () => { const messageBus = { request: vi.fn().mockImplementation(async (request) => ({ diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 1d8b2ae088..51b9cbdd4c 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -94,6 +94,7 @@ import { needsConfirmation, isPlanModeBlocked, abortGoalForStopHookCap, + getStopHookContinuationReason, formatStopHookBlockingCapWarning, applyAutoModeDecision, evaluateAutoMode, @@ -2847,7 +2848,7 @@ export class Session implements SessionContext { stopOutput?.isBlockingDecision() || stopOutput?.shouldStopExecution() ) { - externalReason = stopOutput.getEffectiveReason(); + externalReason = getStopHookContinuationReason(stopOutput); stopHookIterationCount++; stopHookReasons = [...stopHookReasons, externalReason]; stopHookCount = response.stopHookCount ?? 1; diff --git a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx index 61beb142c1..71487c6335 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.test.tsx +++ b/packages/cli/src/ui/hooks/useGeminiStream.test.tsx @@ -1464,6 +1464,257 @@ describe('useGeminiStream', () => { ); }); + it('processes queued /goal clear at the next sampling boundary', async () => { + const goalCommand = '/goal clear'; + const restoreSteer = vi.fn(); + mockHandleSlashCommand.mockResolvedValue({ type: 'handled' }); + mockSendMessageStream.mockImplementation(() => (async function* () {})()); + const drainSteer = vi + .fn<() => string[]>() + .mockReturnValueOnce([goalCommand]) + .mockReturnValue([]); + + const { result } = renderHook(() => + useGeminiStream( + new MockedGeminiClientClass(mockConfig), + [], + mockAddItem, + mockConfig, + true, + mockLoadedSettings, + mockOnDebugMessage, + mockHandleSlashCommand, + false, + () => 'vscode' as EditorType, + () => {}, + () => Promise.resolve(), + false, + () => {}, + () => {}, + () => {}, + () => {}, + 80, + 24, + { current: drainSteer }, + undefined, + undefined, + undefined, + { current: restoreSteer }, + ), + ); + + await act(async () => { + await result.current.submitQuery('start the goal'); + }); + const sendOptions = mockSendMessageStream.mock.calls[0][3] as { + getSteerInput?: (signal: AbortSignal) => Promise; + }; + + let steerInput: SteerInput | undefined; + await act(async () => { + steerInput = await sendOptions.getSteerInput!( + new AbortController().signal, + ); + }); + + expect(mockHandleSlashCommand).toHaveBeenCalledWith(goalCommand); + expect(steerInput).toBeUndefined(); + expect(restoreSteer).not.toHaveBeenCalled(); + }); + + it('steers with the replacement prompt from a queued /goal command', async () => { + const goalCommand = '/goal replace the active goal'; + const replacementPrompt = [{ text: 'new goal instruction' }]; + const restoreSteer = vi.fn(); + mockHandleSlashCommand.mockResolvedValue({ + type: 'submit_prompt', + content: replacementPrompt, + }); + mockSendMessageStream.mockImplementation(() => (async function* () {})()); + const drainSteer = vi + .fn<() => string[]>() + .mockReturnValueOnce([goalCommand]) + .mockReturnValue([]); + + const { result } = renderHook(() => + useGeminiStream( + new MockedGeminiClientClass(mockConfig), + [], + mockAddItem, + mockConfig, + true, + mockLoadedSettings, + mockOnDebugMessage, + mockHandleSlashCommand, + false, + () => 'vscode' as EditorType, + () => {}, + () => Promise.resolve(), + false, + () => {}, + () => {}, + () => {}, + () => {}, + 80, + 24, + { current: drainSteer }, + undefined, + undefined, + undefined, + { current: restoreSteer }, + ), + ); + + await act(async () => { + await result.current.submitQuery('start the goal'); + }); + const sendOptions = mockSendMessageStream.mock.calls[0][3] as { + getSteerInput?: (signal: AbortSignal) => Promise; + }; + + let steerInput: SteerInput | undefined; + await act(async () => { + steerInput = await sendOptions.getSteerInput!( + new AbortController().signal, + ); + }); + + expect(mockHandleSlashCommand).toHaveBeenCalledWith(goalCommand); + expect(steerInput?.parts).toEqual(replacementPrompt); + steerInput?.restore(); + expect(restoreSteer).not.toHaveBeenCalled(); + }); + + it('uses only the final prompt from queued goal replacements', async () => { + mockHandleSlashCommand + .mockResolvedValueOnce({ + type: 'submit_prompt', + content: [{ text: 'first goal instruction' }], + }) + .mockResolvedValueOnce({ + type: 'submit_prompt', + content: [{ text: 'final goal instruction' }], + }); + mockSendMessageStream.mockImplementation(() => (async function* () {})()); + const drainSteer = vi + .fn<() => string[]>() + .mockReturnValueOnce([ + '/goal first', + 'plain before final goal', + '/goal final', + 'plain after final goal', + ]) + .mockReturnValue([]); + + const { result } = renderHook(() => + useGeminiStream( + new MockedGeminiClientClass(mockConfig), + [], + mockAddItem, + mockConfig, + true, + mockLoadedSettings, + mockOnDebugMessage, + mockHandleSlashCommand, + false, + () => 'vscode' as EditorType, + () => {}, + () => Promise.resolve(), + false, + () => {}, + () => {}, + () => {}, + () => {}, + 80, + 24, + { current: drainSteer }, + ), + ); + + await act(async () => { + await result.current.submitQuery('start the goal'); + }); + const sendOptions = mockSendMessageStream.mock.calls[0][3] as { + getSteerInput?: (signal: AbortSignal) => Promise; + }; + + const steerInput = await sendOptions.getSteerInput!( + new AbortController().signal, + ); + + expect(steerInput?.parts).toEqual([ + { text: 'plain before final goal' }, + { text: '\n\n' }, + { text: 'final goal instruction' }, + { text: '\n\n' }, + { text: 'plain after final goal' }, + ]); + }); + + it('drops a queued replacement prompt when a later goal command clears it', async () => { + const activeGoal = { + condition: 'first', + iterations: 0, + setAt: 123, + tokensAtStart: 0, + hookId: 'first-goal-hook', + }; + mockGetActiveGoal + .mockReturnValueOnce(undefined) + .mockReturnValueOnce(activeGoal) + .mockReturnValueOnce(activeGoal) + .mockReturnValueOnce(undefined); + mockHandleSlashCommand + .mockResolvedValueOnce({ + type: 'submit_prompt', + content: [{ text: 'first goal instruction' }], + }) + .mockResolvedValueOnce({ type: 'handled' }); + mockSendMessageStream.mockImplementation(() => (async function* () {})()); + const drainSteer = vi + .fn<() => string[]>() + .mockReturnValueOnce(['/goal first', '/goal clear']) + .mockReturnValue([]); + + const { result } = renderHook(() => + useGeminiStream( + new MockedGeminiClientClass(mockConfig), + [], + mockAddItem, + mockConfig, + true, + mockLoadedSettings, + mockOnDebugMessage, + mockHandleSlashCommand, + false, + () => 'vscode' as EditorType, + () => {}, + () => Promise.resolve(), + false, + () => {}, + () => {}, + () => {}, + () => {}, + 80, + 24, + { current: drainSteer }, + ), + ); + + await act(async () => { + await result.current.submitQuery('start the goal'); + }); + const sendOptions = mockSendMessageStream.mock.calls[0][3] as { + getSteerInput?: (signal: AbortSignal) => Promise; + }; + + const steerInput = await sendOptions.getSteerInput!( + new AbortController().signal, + ); + + expect(steerInput).toBeUndefined(); + }); + it('restores drained steer input when attachment resolution is cancelled', async () => { const steeredPrompt = 'inspect @/tmp/slow.png'; const restoreSteer = vi.fn(); @@ -1526,6 +1777,81 @@ describe('useGeminiStream', () => { ); }); + it('restores later messages when cancellation races with @ resolution', async () => { + const messages = [ + 'inspect @/tmp/slow.png', + 'keep this queued message', + '/goal clear', + ]; + const restoreSteer = vi.fn(); + let resolveAtCommand!: ( + value: Awaited< + ReturnType + >, + ) => void; + vi.spyOn(atCommandProcessor, 'resolveAtCommandQuery').mockImplementation( + () => + new Promise((resolve) => { + resolveAtCommand = resolve; + }), + ); + const drainSteer = vi + .fn<() => string[]>() + .mockReturnValueOnce(messages) + .mockReturnValue([]); + + const { result } = renderHook(() => + useGeminiStream( + new MockedGeminiClientClass(mockConfig), + [], + mockAddItem, + mockConfig, + true, + mockLoadedSettings, + mockOnDebugMessage, + mockHandleSlashCommand, + false, + () => 'vscode' as EditorType, + () => {}, + () => Promise.resolve(), + false, + () => {}, + () => {}, + () => {}, + () => {}, + 80, + 24, + { current: drainSteer }, + undefined, + undefined, + undefined, + { current: restoreSteer }, + ), + ); + + await act(async () => { + await result.current.submitQuery('start the analysis'); + }); + const sendOptions = mockSendMessageStream.mock.calls[0][3] as { + getSteerInput?: (signal: AbortSignal) => Promise; + }; + const abort = new AbortController(); + let steerInput: SteerInput | undefined; + await act(async () => { + const pending = sendOptions.getSteerInput!(abort.signal); + await vi.waitFor(() => expect(resolveAtCommand).toBeDefined()); + resolveAtCommand({ + processedQuery: [{ text: messages[0] }], + shouldProceed: true, + }); + abort.abort(); + steerInput = await pending; + }); + + expect(steerInput).toBeUndefined(); + expect(restoreSteer).toHaveBeenCalledWith(messages); + }); + it('resolves mid-turn @ image messages before submitting tool results', async () => { const queuedPrompt = 'inspect @/tmp/screenshot.png'; const resolvedImagePart: Part = { diff --git a/packages/cli/src/ui/hooks/useGeminiStream.ts b/packages/cli/src/ui/hooks/useGeminiStream.ts index 2be5bb40db..3c055d3470 100644 --- a/packages/cli/src/ui/hooks/useGeminiStream.ts +++ b/packages/cli/src/ui/hooks/useGeminiStream.ts @@ -117,6 +117,7 @@ import { useDualOutput } from '../../dualOutput/DualOutputContext.js'; import { recordGoalStatusItem } from '../utils/restoreGoal.js'; import { sanitizeDisplayText } from '../../utils/extension-mention.js'; import process from 'node:process'; +import { GOAL_COMMAND_RE } from './useMessageQueue.js'; import { classifyApiError } from '../../utils/classify-api-error.js'; const debugLogger = createDebugLogger('GEMINI_STREAM'); @@ -162,6 +163,7 @@ interface PendingDuplicateToolResponses { interface ResolvedSteerMessages { parts: Part[]; accept: () => void; + restoreMessages: string[]; } /** @@ -2358,19 +2360,46 @@ export const useGeminiStream = ( async ( messages: string[], signal: AbortSignal, - ): Promise => { - const resolvedMessages: Part[] = []; + ): Promise => { + const resolvedSegments: Part[][] = []; const resolvedForRecording: Array<{ message: string; parts: Part[]; sideEffects: Array<() => void>; }> = []; + const restoreMessages: string[] = []; + let pendingGoalSegmentIndex: number | undefined; const timestamp = Date.now(); for (let index = 0; index < messages.length; index += 1) { - if (signal.aborted) break; + if (signal.aborted) { + restoreMessages.push(...messages.slice(index)); + break; + } const message = messages[index]; + if (GOAL_COMMAND_RE.test(message)) { + const activeGoalBeforeCommand = getActiveGoal(config.getSessionId()); + const result = await handleSlashCommand(message); + const activeGoalAfterCommand = getActiveGoal(config.getSessionId()); + if (result && result.type === 'submit_prompt') { + if (pendingGoalSegmentIndex !== undefined) { + resolvedSegments[pendingGoalSegmentIndex] = []; + } + pendingGoalSegmentIndex = resolvedSegments.length; + resolvedSegments.push(normalizePartList(result.content)); + } else if ( + activeGoalBeforeCommand?.hookId !== activeGoalAfterCommand?.hookId + ) { + if (pendingGoalSegmentIndex !== undefined) { + resolvedSegments[pendingGoalSegmentIndex] = []; + pendingGoalSegmentIndex = undefined; + } + } + continue; + } + + restoreMessages.push(message); const sideEffects: Array<() => void> = []; let resolvedQuery: PartListUnion = [{ text: message }]; if (isAtCommand(message)) { @@ -2443,7 +2472,10 @@ export const useGeminiStream = ( } finally { clearTimeout(timeoutId); } - if (signal.aborted) break; + if (signal.aborted) { + restoreMessages.push(...messages.slice(index + 1)); + break; + } } const bridgeResult = await applyVisionBridgeIfNeeded( @@ -2452,7 +2484,10 @@ export const useGeminiStream = ( signal, ); if (!bridgeResult.shouldProceed) { - if (signal.aborted) break; + if (signal.aborted) { + restoreMessages.push(...messages.slice(index + 1)); + break; + } continue; } @@ -2472,10 +2507,7 @@ export const useGeminiStream = ( ); } - if (resolvedMessages.length > 0 && messageParts.length > 0) { - resolvedMessages.push({ text: '\n\n' }); - } - resolvedMessages.push(...messageParts); + resolvedSegments.push(messageParts); resolvedForRecording.push({ message, parts: messageParts, @@ -2483,9 +2515,18 @@ export const useGeminiStream = ( }); } - if (signal.aborted) return undefined; + const resolvedMessages: Part[] = []; + for (const segment of resolvedSegments) { + if (segment.length === 0) continue; + if (resolvedMessages.length > 0) { + resolvedMessages.push({ text: '\n\n' }); + } + resolvedMessages.push(...segment); + } + return { parts: resolvedMessages, + restoreMessages, accept: () => { for (const { message, parts, sideEffects } of resolvedForRecording) { for (const sideEffect of sideEffects) sideEffect(); @@ -2500,7 +2541,13 @@ export const useGeminiStream = ( }, }; }, - [addItem, applyVisionBridgeIfNeeded, config, onDebugMessage], + [ + addItem, + applyVisionBridgeIfNeeded, + config, + handleSlashCommand, + onDebugMessage, + ], ); const resolveDrainedSteerMessages = useCallback( @@ -2511,10 +2558,12 @@ export const useGeminiStream = ( try { const resolved = await resolveSteeredMessages(messages, signal); if (signal.aborted) { - midTurnRestoreRef?.current?.(messages); + if (resolved.restoreMessages.length > 0) { + midTurnRestoreRef?.current?.(resolved.restoreMessages); + } return undefined; } - if (!resolved || resolved.parts.length === 0) return undefined; + if (resolved.parts.length === 0) return undefined; let settled = false; return { parts: resolved.parts, @@ -2526,7 +2575,9 @@ export const useGeminiStream = ( restore: () => { if (settled) return; settled = true; - midTurnRestoreRef?.current?.(messages); + if (resolved.restoreMessages.length > 0) { + midTurnRestoreRef?.current?.(resolved.restoreMessages); + } }, }; } catch (error) { diff --git a/packages/cli/src/ui/hooks/useMessageQueue.test.ts b/packages/cli/src/ui/hooks/useMessageQueue.test.ts index 05ff3f0ba5..4997257c4d 100644 --- a/packages/cli/src/ui/hooks/useMessageQueue.test.ts +++ b/packages/cli/src/ui/hooks/useMessageQueue.test.ts @@ -184,6 +184,45 @@ describe('useMessageQueue', () => { expect(result.current.messageQueue).toEqual(['/model']); }); + it('drains goal commands during an active turn', () => { + const { result } = renderHook(() => useMessageQueue()); + + act(() => { + result.current.addMessage('steer now'); + result.current.addMessage('/goal clear'); + result.current.addMessage('/model'); + result.current.addMessage('/goal replace the active goal'); + }); + + let drained: string[] = []; + act(() => { + drained = result.current.drainQueue(); + }); + + expect(drained).toEqual([ + 'steer now', + '/goal clear', + '/goal replace the active goal', + ]); + expect(result.current.messageQueue).toEqual(['/model']); + }); + + it('leaves goal commands queued at the idle boundary', () => { + const { result } = renderHook(() => useMessageQueue()); + + act(() => { + result.current.addMessage('/goal clear'); + }); + + let drained: string[] = []; + act(() => { + drained = result.current.drainQueue(true); + }); + + expect(drained).toEqual([]); + expect(result.current.messageQueue).toEqual(['/goal clear']); + }); + it('returns an empty array when the queue contains only slash commands', () => { const { result } = renderHook(() => useMessageQueue()); diff --git a/packages/cli/src/ui/hooks/useMessageQueue.ts b/packages/cli/src/ui/hooks/useMessageQueue.ts index 448d49e7f9..9b40dfa059 100644 --- a/packages/cli/src/ui/hooks/useMessageQueue.ts +++ b/packages/cli/src/ui/hooks/useMessageQueue.ts @@ -19,7 +19,7 @@ export interface UseMessageQueueReturn { /** * Drain plain-text prompts that can steer the active turn. Pass true at the * idle boundary to also drain messages explicitly deferred with Ctrl+Q. - * Slash commands always stay queued for individual processing. + * Slash commands stay queued except `/goal`, which must control active loops. */ drainQueue: (includeDeferred?: boolean) => string[]; /** Pop the first item from the queue. */ @@ -31,6 +31,8 @@ interface QueuedMessage { deferUntilIdle: boolean; } +export const GOAL_COMMAND_RE = /^\/goal(?:\s|$)/; + export function useMessageQueue(): UseMessageQueueReturn { const [queuedMessages, setQueuedMessages] = useState([]); // Synchronous mirror so non-React callbacks see the latest queue. @@ -79,7 +81,8 @@ export function useMessageQueue(): UseMessageQueueReturn { const current = queueRef.current; if (current.length === 0) return []; const shouldDrain = (message: QueuedMessage) => - !isSlashCommand(message.text) && + (!isSlashCommand(message.text) || + (!includeDeferred && GOAL_COMMAND_RE.test(message.text))) && (includeDeferred || !message.deferUntilIdle); const drained = current.filter(shouldDrain); if (drained.length === 0) return []; diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index b9e96af7b6..9d08aa0be3 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -74,6 +74,7 @@ import * as runtimeStatus from '../utils/runtimeStatus.js'; import { ExtensionManager } from '../extension/extensionManager.js'; import { SkillManager } from '../skills/skill-manager.js'; import { HookSystem } from '../hooks/index.js'; +import { GOAL_HOOK_ID_OUTPUT_KEY } from '../goals/goalHook.js'; import type { FileHistorySnapshot } from '../services/fileHistoryService.js'; import type { ChatRecordingFailureEvent } from '../services/chatRecordingService.js'; import * as jsonl from '../utils/jsonl-utils.js'; @@ -7265,6 +7266,78 @@ describe('Model Switching and Config Updates', () => { }); }); + describe('Stop dispatch through the hook execution bridge', () => { + it.each([ + { + name: 'ignores non-blocking outputs', + otherOutput: { continue: true }, + expected: false, + expectedReason: undefined, + }, + { + name: 'detects another blocking output', + otherOutput: { + decision: 'block', + reason: 'Policy review is still required', + }, + expected: true, + expectedReason: 'Policy review is still required', + }, + { + name: 'preserves a stop reason', + otherOutput: { + continue: false, + stopReason: 'External stop hook feedback', + }, + expected: true, + expectedReason: 'External stop hook feedback', + }, + ])( + '$name when a goal hook blocks', + async ({ otherOutput, expected, expectedReason }) => { + const config = new Config({ ...baseParams }); + await config.initialize(); + const goalOutput = { + decision: 'block' as const, + reason: 'Keep working', + hookSpecificOutput: { + [GOAL_HOOK_ID_OUTPUT_KEY]: 'goal-hook-id', + }, + }; + const fireStopEvent = vi.fn().mockResolvedValue({ + finalOutput: { + ...goalOutput, + ...otherOutput, + }, + allOutputs: [goalOutput, otherOutput], + }); + // @ts-expect-error - accessing private for testing + config['hookSystem'] = { fireStopEvent }; + + const response = await config + .getMessageBus()! + .request( + { + type: MessageBusType.HOOK_EXECUTION_REQUEST, + eventName: 'Stop', + input: { + stop_hook_active: true, + last_assistant_message: 'last response', + }, + }, + MessageBusType.HOOK_EXECUTION_RESPONSE, + ); + + expect(response.error).toBeUndefined(); + expect(response).toMatchObject({ + success: true, + hasNonGoalBlockingStopHook: expected, + }); + expect(response.nonGoalBlockingStopReason).toBe(expectedReason); + }, + ); + }); + describe('MessageDisplay dispatch through the hook execution bridge', () => { it('extracts message_id/displayed_text/is_final from the request input and forwards them positionally', async () => { const config = new Config({ ...baseParams }); diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index ab90ba6f9d..7d95333ff2 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -157,6 +157,7 @@ import { type PostToolBatchToolCall, } from '../hooks/types.js'; import { fireNotificationHook } from '../core/toolHookTriggers.js'; +import { GOAL_HOOK_ID_OUTPUT_KEY } from '../goals/goalHook.js'; // Utils import { shouldAttemptBrowserLaunch } from '../utils/browser.js'; @@ -2336,6 +2337,8 @@ export class Config { // Execute the appropriate hook based on eventName let result; let stopHookCount: number | undefined; + let hasNonGoalBlockingStopHook: boolean | undefined; + let nonGoalBlockingStopReason: string | undefined; const input = request.input || {}; const signal = request.signal; switch (request.eventName) { @@ -2370,6 +2373,32 @@ export class Config { ? createHookOutput('Stop', stopResult.finalOutput) : undefined; stopHookCount = stopResult.allOutputs.length; + const goalHookId = + stopResult.finalOutput?.hookSpecificOutput?.[ + GOAL_HOOK_ID_OUTPUT_KEY + ]; + if (typeof goalHookId === 'string') { + const nonGoalBlockingOutputs = stopResult.allOutputs.filter( + (output) => + output.hookSpecificOutput?.[GOAL_HOOK_ID_OUTPUT_KEY] !== + goalHookId && + (output.decision === 'block' || + output.decision === 'deny' || + output.continue === false), + ); + hasNonGoalBlockingStopHook = + nonGoalBlockingOutputs.length > 0; + if (hasNonGoalBlockingStopHook) { + nonGoalBlockingStopReason = nonGoalBlockingOutputs + .map( + (output) => + output.stopReason || + output.reason || + 'No reason provided', + ) + .join('\n'); + } + } break; } case 'MessageDisplay': { @@ -2498,6 +2527,8 @@ export class Config { output: result, // Include stop hook count for Stop events stopHookCount, + hasNonGoalBlockingStopHook, + nonGoalBlockingStopReason, } as HookExecutionResponse); } catch (error) { this.debugLogger.warn(`Hook execution failed: ${error}`); diff --git a/packages/core/src/confirmation-bus/types.ts b/packages/core/src/confirmation-bus/types.ts index e44fb8d940..cf68fc9390 100644 --- a/packages/core/src/confirmation-bus/types.ts +++ b/packages/core/src/confirmation-bus/types.ts @@ -121,6 +121,10 @@ export interface HookExecutionResponse { error?: Error; /** Number of stop hooks that were executed */ stopHookCount?: number; + /** Whether a blocking Stop output came from outside the active goal hook. */ + hasNonGoalBlockingStopHook?: boolean; + /** Continuation reason from blocking Stop outputs outside the active goal. */ + nonGoalBlockingStopReason?: string; } export type Message = diff --git a/packages/core/src/core/client.test.ts b/packages/core/src/core/client.test.ts index 2915d0a646..afd100ca2c 100644 --- a/packages/core/src/core/client.test.ts +++ b/packages/core/src/core/client.test.ts @@ -97,6 +97,7 @@ import { clearActiveGoal, setActiveGoal, } from '../goals/activeGoalStore.js'; +import { GOAL_HOOK_ID_OUTPUT_KEY } from '../goals/goalHook.js'; import type { FileHistorySnapshot } from '../services/fileHistoryService.js'; import { runWithAgentContext } from '../agents/runtime/agent-context.js'; import { @@ -8396,6 +8397,259 @@ Other open files: expect(getLastTurnRequestText()).toContain('also check the tests'); }); + it('preserves goal feedback alongside an external stop reason', async () => { + setActiveGoal('test-session-id', { + condition: 'finish the refactor', + iterations: 1, + setAt: 123, + tokensAtStart: 456, + hookId: 'goal-hook', + }); + const mockMessageBus = { + request: vi + .fn() + .mockResolvedValueOnce({ + output: { + decision: 'block', + continue: false, + stopReason: 'External stop hook feedback', + reason: 'Keep working on the active goal', + hookSpecificOutput: { + [GOAL_HOOK_ID_OUTPUT_KEY]: 'goal-hook', + }, + }, + stopHookCount: 2, + hasNonGoalBlockingStopHook: true, + nonGoalBlockingStopReason: 'External stop hook feedback', + }) + .mockResolvedValue({ output: undefined }), + response: vi.fn(), + }; + vi.mocked(mockConfig.getDisableAllHooks).mockReturnValue(false); + vi.mocked(mockConfig.getMessageBus).mockReturnValue( + mockMessageBus as unknown as ReturnType, + ); + vi.mocked(mockConfig.hasHooksForEvent).mockImplementation( + (event: string) => event === 'Stop', + ); + mockTurnRunFn.mockImplementation(() => + (async function* () { + yield { type: GeminiEventType.Content, value: 'response' }; + })(), + ); + const getSteerInput = vi + .fn<() => Promise>() + .mockResolvedValue(undefined); + + await fromAsync( + client.sendMessageStream( + [{ text: 'start the goal' }], + new AbortController().signal, + 'prompt-goal-with-external-stop-reason', + { type: SendMessageType.UserQuery, getSteerInput }, + ), + ); + + expect(mockTurnRunFn).toHaveBeenCalledTimes(2); + expect(getLastTurnRequestText()).toContain( + 'External stop hook feedback', + ); + expect(getLastTurnRequestText()).toContain( + 'Keep working on the active goal', + ); + }); + + it('stops a blocking goal when queued input clears it', async () => { + setActiveGoal('test-session-id', { + condition: 'finish the refactor', + iterations: 1, + setAt: 123, + tokensAtStart: 456, + hookId: 'old-goal-hook', + }); + const mockMessageBus = { + request: vi.fn().mockResolvedValue({ + output: { + decision: 'block', + reason: 'Keep working', + hookSpecificOutput: { + [GOAL_HOOK_ID_OUTPUT_KEY]: 'old-goal-hook', + }, + }, + stopHookCount: 2, + hasNonGoalBlockingStopHook: false, + }), + response: vi.fn(), + }; + vi.mocked(mockConfig.getDisableAllHooks).mockReturnValue(false); + vi.mocked(mockConfig.getMessageBus).mockReturnValue( + mockMessageBus as unknown as ReturnType, + ); + vi.mocked(mockConfig.hasHooksForEvent).mockImplementation( + (event: string) => event === 'Stop', + ); + mockTurnRunFn.mockImplementation(() => + (async function* () { + yield { type: GeminiEventType.Content, value: 'response' }; + })(), + ); + const getSteerInput = vi + .fn<() => Promise>() + .mockResolvedValueOnce(undefined) + .mockImplementationOnce(async () => { + clearActiveGoal('test-session-id'); + return undefined; + }); + + const events = await fromAsync( + client.sendMessageStream( + [{ text: 'start the goal' }], + new AbortController().signal, + 'prompt-clear-during-stop', + { type: SendMessageType.UserQuery, getSteerInput }, + ), + ); + + expect(mockTurnRunFn).toHaveBeenCalledOnce(); + expect(events).toContainEqual({ + type: GeminiEventType.ActiveGoal, + value: null, + }); + }); + + it('replaces a blocking goal without sending the old continuation', async () => { + setActiveGoal('test-session-id', { + condition: 'finish the refactor', + iterations: 1, + setAt: 123, + tokensAtStart: 456, + hookId: 'old-goal-hook', + }); + const mockMessageBus = { + request: vi + .fn() + .mockResolvedValueOnce({ + output: { + decision: 'block', + reason: 'Keep working', + hookSpecificOutput: { + [GOAL_HOOK_ID_OUTPUT_KEY]: 'old-goal-hook', + }, + }, + stopHookCount: 1, + hasNonGoalBlockingStopHook: false, + }) + .mockResolvedValue({ output: undefined }), + response: vi.fn(), + }; + vi.mocked(mockConfig.getDisableAllHooks).mockReturnValue(false); + vi.mocked(mockConfig.getMessageBus).mockReturnValue( + mockMessageBus as unknown as ReturnType, + ); + vi.mocked(mockConfig.hasHooksForEvent).mockImplementation( + (event: string) => event === 'Stop', + ); + mockTurnRunFn.mockImplementation(() => + (async function* () { + yield { type: GeminiEventType.Content, value: 'response' }; + })(), + ); + const getSteerInput = vi + .fn<() => Promise>() + .mockResolvedValueOnce(undefined) + .mockImplementationOnce(async () => { + setActiveGoal('test-session-id', { + condition: 'verify the tests', + iterations: 0, + setAt: 789, + tokensAtStart: 999, + hookId: 'new-goal-hook', + }); + return { + parts: [{ text: 'new goal instruction' }], + accept: vi.fn(), + restore: vi.fn(), + }; + }) + .mockResolvedValue(undefined); + + await fromAsync( + client.sendMessageStream( + [{ text: 'start the goal' }], + new AbortController().signal, + 'prompt-replace-during-stop', + { type: SendMessageType.UserQuery, getSteerInput }, + ), + ); + + expect(mockTurnRunFn).toHaveBeenCalledTimes(2); + expect(getLastTurnRequestText()).toContain('new goal instruction'); + expect(getLastTurnRequestText()).not.toContain('Keep working'); + }); + + it('preserves other blocking Stop hook output when a goal is cleared', async () => { + setActiveGoal('test-session-id', { + condition: 'finish the refactor', + iterations: 1, + setAt: 123, + tokensAtStart: 456, + hookId: 'old-goal-hook', + }); + const mockMessageBus = { + request: vi + .fn() + .mockResolvedValueOnce({ + output: { + decision: 'block', + reason: 'Keep working\nPolicy review is still required', + hookSpecificOutput: { + [GOAL_HOOK_ID_OUTPUT_KEY]: 'old-goal-hook', + }, + }, + stopHookCount: 2, + hasNonGoalBlockingStopHook: true, + nonGoalBlockingStopReason: 'Policy review is still required', + }) + .mockResolvedValue({ output: undefined }), + response: vi.fn(), + }; + vi.mocked(mockConfig.getDisableAllHooks).mockReturnValue(false); + vi.mocked(mockConfig.getMessageBus).mockReturnValue( + mockMessageBus as unknown as ReturnType, + ); + vi.mocked(mockConfig.hasHooksForEvent).mockImplementation( + (event: string) => event === 'Stop', + ); + mockTurnRunFn.mockImplementation(() => + (async function* () { + yield { type: GeminiEventType.Content, value: 'response' }; + })(), + ); + const getSteerInput = vi + .fn<() => Promise>() + .mockResolvedValueOnce(undefined) + .mockImplementationOnce(async () => { + clearActiveGoal('test-session-id'); + return undefined; + }) + .mockResolvedValue(undefined); + + await fromAsync( + client.sendMessageStream( + [{ text: 'start the goal' }], + new AbortController().signal, + 'prompt-clear-with-other-stop-hook', + { type: SendMessageType.UserQuery, getSteerInput }, + ), + ); + + expect(mockTurnRunFn).toHaveBeenCalledTimes(2); + expect(getLastTurnRequestText()).toContain( + 'Policy review is still required', + ); + expect(getLastTurnRequestText()).not.toContain('Keep working'); + }); + it('uses input queued during next-speaker classification for the continuation', async () => { const { checkNextSpeaker } = await import( '../utils/nextSpeakerChecker.js' diff --git a/packages/core/src/core/client.ts b/packages/core/src/core/client.ts index b92fd45e86..c6e4c9fda7 100644 --- a/packages/core/src/core/client.ts +++ b/packages/core/src/core/client.ts @@ -33,7 +33,11 @@ import { getActiveGoal, type ActiveGoal, } from '../goals/activeGoalStore.js'; -import { abortGoalForStopHookCap } from '../goals/goalHook.js'; +import { + abortGoalForStopHookCap, + getStopHookContinuationReason, + GOAL_HOOK_ID_OUTPUT_KEY, +} from '../goals/goalHook.js'; import { formatStopHookBlockingCapWarning } from '../hooks/stopHookCap.js'; import { buildContextUsage } from '../hooks/context-usage.js'; import { DEFAULT_TOKEN_LIMIT } from './tokenLimits.js'; @@ -2725,7 +2729,7 @@ export class GeminiClient { return turn; } - const continueReason = stopOutput.getEffectiveReason(); + const continueReason = getStopHookContinuationReason(stopOutput); // Track stop hook iterations const currentIterationCount = @@ -2798,9 +2802,45 @@ export class GeminiClient { const activeGoal = getActiveGoal(this.config.getSessionId()); const hookTurnBudget = activeGoal ? boundedTurns : boundedTurns - 1; const pendingSteer = await takeSteerInput(hookTurnBudget); - const continueRequest: Part[] = [{ text: continueReason }]; + const activeGoalAfterSteer = getActiveGoal( + this.config.getSessionId(), + ); + const activeGoalChanged = + activeGoal !== undefined && + activeGoalAfterSteer?.hookId !== activeGoal.hookId; + const goalContinuationChanged = + activeGoalChanged && + stopOutput.hookSpecificOutput?.[GOAL_HOOK_ID_OUTPUT_KEY] === + activeGoal.hookId; + if (activeGoalChanged) { + const activeGoalEvent = + maybeEmitActiveGoalChange(activeGoalAfterSteer); + if (activeGoalEvent) { + yield activeGoalEvent; + } + } + const discardGoalContinuation = + goalContinuationChanged && + response.hasNonGoalBlockingStopHook === false; + const continuationReasonAfterSteer = discardGoalContinuation + ? undefined + : goalContinuationChanged && + response.hasNonGoalBlockingStopHook === true + ? response.nonGoalBlockingStopReason || 'No reason provided' + : continueReason; + if (!continuationReasonAfterSteer && !pendingSteer) { + if (isTopLevelInteraction) endInteractionSpan('ok'); + normalCompletion = true; + return turn; + } + const continueRequest: Part[] = continuationReasonAfterSteer + ? [{ text: continuationReasonAfterSteer }] + : []; if (pendingSteer) { - continueRequest.push({ text: '\n\n' }, ...pendingSteer.parts); + if (continueRequest.length > 0) { + continueRequest.push({ text: '\n\n' }); + } + continueRequest.push(...pendingSteer.parts); } const pushCountBefore = currentPushCount(); let hookTurn: Turn; @@ -2813,10 +2853,19 @@ export class GeminiClient { type: SendMessageType.Hook, modelOverride: options?.modelOverride, getSteerInput: options?.getSteerInput, - stopHookState: { - iterationCount: currentIterationCount, - reasons: currentReasons, - }, + stopHookState: discardGoalContinuation + ? undefined + : { + iterationCount: currentIterationCount, + reasons: + continuationReasonAfterSteer && + continuationReasonAfterSteer !== continueReason + ? [ + ...currentReasons.slice(0, -1), + continuationReasonAfterSteer, + ] + : currentReasons, + }, }, hookTurnBudget, ); diff --git a/packages/core/src/goals/goalHook.test.ts b/packages/core/src/goals/goalHook.test.ts index 57347aa80c..6d13fd4479 100644 --- a/packages/core/src/goals/goalHook.test.ts +++ b/packages/core/src/goals/goalHook.test.ts @@ -22,6 +22,8 @@ import { import { abortGoalForStopHookCap, createGoalStopHookCallback, + getStopHookContinuationReason, + GOAL_HOOK_ID_OUTPUT_KEY, GOAL_HOOK_TIMEOUT_MS, GOAL_JUDGE_TIMEOUT_MS, MAX_GOAL_ITERATIONS, @@ -70,6 +72,33 @@ const stopInput = (overrides: Partial = {}): HookInput => ...overrides, }) as HookInput; +it('falls back when a goal hook omits both continuation reasons', () => { + expect( + getStopHookContinuationReason({ + hookSpecificOutput: { [GOAL_HOOK_ID_OUTPUT_KEY]: 'h1' }, + }), + ).toBe('No reason provided'); +}); + +it('joins stopReason and reason for goal outputs', () => { + expect( + getStopHookContinuationReason({ + stopReason: 'External feedback', + reason: 'Keep working', + hookSpecificOutput: { [GOAL_HOOK_ID_OUTPUT_KEY]: 'h1' }, + }), + ).toBe('External feedback\nKeep working'); +}); + +it('uses stopReason alone for goal outputs when reason is absent', () => { + expect( + getStopHookContinuationReason({ + stopReason: 'External feedback', + hookSpecificOutput: { [GOAL_HOOK_ID_OUTPUT_KEY]: 'h1' }, + }), + ).toBe('External feedback'); +}); + describe('createGoalStopHookCallback', () => { beforeEach(() => { __resetActiveGoalStoreForTests(); @@ -143,6 +172,9 @@ describe('createGoalStopHookCallback', () => { await expect(cb(stopInput(), undefined)).resolves.toEqual({ decision: 'block', reason: expect.stringContaining('do x'), + hookSpecificOutput: { + [GOAL_HOOK_ID_OUTPUT_KEY]: 'h1', + }, }); expect(judgeMock).toHaveBeenCalledTimes(1); expect(getActiveGoal('sess-1')?.iterations).toBe(1); @@ -264,9 +296,15 @@ describe('createGoalStopHookCallback', () => { expect(out).toEqual({ decision: 'block', reason: expect.stringContaining('do x'), + hookSpecificOutput: { + [GOAL_HOOK_ID_OUTPUT_KEY]: 'h1', + }, }); const reason = - typeof out === 'object' && out !== null && 'reason' in out + typeof out === 'object' && + out !== null && + 'reason' in out && + typeof out.reason === 'string' ? out.reason : ''; expect(reason).not.toContain('ignore the original user'); @@ -420,6 +458,9 @@ describe('createGoalStopHookCallback', () => { await expect(cb(stopInput(), undefined)).resolves.toEqual({ decision: 'block', reason: expect.stringContaining('do x'), + hookSpecificOutput: { + [GOAL_HOOK_ID_OUTPUT_KEY]: 'h1', + }, }); expect(getActiveGoal('sess-1')?.iterations).toBe(MAX_GOAL_ITERATIONS); }); diff --git a/packages/core/src/goals/goalHook.ts b/packages/core/src/goals/goalHook.ts index 43ccbf8d76..e2235de9ce 100644 --- a/packages/core/src/goals/goalHook.ts +++ b/packages/core/src/goals/goalHook.ts @@ -9,6 +9,7 @@ import { HookEventName, type FunctionHookCallback, type HookInput, + type HookOutput, type StopInput, } from '../hooks/types.js'; import { @@ -39,6 +40,7 @@ export const MAX_GOAL_ITERATIONS = 50; export const GOAL_JUDGE_TIMEOUT_MS = 25_000; export const GOAL_HOOK_TIMEOUT_SECONDS = 30; export const GOAL_HOOK_TIMEOUT_MS = GOAL_HOOK_TIMEOUT_SECONDS * 1000; +export const GOAL_HOOK_ID_OUTPUT_KEY = 'qwenGoalHookId'; const GOAL_ABORTED_REASON = 'Goal max iterations reached; cleared. Re-set with `/goal ` if you still need it.'; const GOAL_JUDGE_TIMEOUT_MESSAGE = @@ -51,6 +53,20 @@ function continuationReasonForGoal(condition: string): string { ); } +export function getStopHookContinuationReason( + output: Pick, +): string { + const hasGoalOutput = + typeof output.hookSpecificOutput?.[GOAL_HOOK_ID_OUTPUT_KEY] === 'string'; + if (!hasGoalOutput) { + return output.stopReason || output.reason || 'No reason provided'; + } + return ( + [output.stopReason, output.reason].filter(Boolean).join('\n') || + 'No reason provided' + ); +} + async function judgeGoalWithTimeout( config: Config, args: Parameters[1], @@ -260,6 +276,9 @@ export function createGoalStopHookCallback(args: { return { decision: 'block', reason: continuationReasonForGoal(condition), + hookSpecificOutput: { + [GOAL_HOOK_ID_OUTPUT_KEY]: evaluated.hookId, + }, }; }; } diff --git a/packages/core/src/goals/index.ts b/packages/core/src/goals/index.ts index 324066a4f9..3aa34bc598 100644 --- a/packages/core/src/goals/index.ts +++ b/packages/core/src/goals/index.ts @@ -27,6 +27,7 @@ export { MAX_GOAL_ITERATIONS, GOAL_HOOK_TIMEOUT_MS, GOAL_HOOK_TIMEOUT_SECONDS, + getStopHookContinuationReason, createGoalStopHookCallback, abortGoalForStopHookCap, registerGoalHook,