Repository navigation
Support active goal stream events and non-interactive goals #4273
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
be473f6
4391fa3
19b311b
e86c6c8
9a63db5
8cdb926
c4916d3
5f6da56
71ceb5a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,6 +52,11 @@ import { promptIdContext } from '../utils/promptIdContext.js'; | |
| import { setSimulate429 } from '../utils/testUtils.js'; | ||
| import { ideContextStore } from '../ide/ideContext.js'; | ||
| import { uiTelemetryService } from '../telemetry/uiTelemetry.js'; | ||
| import { | ||
| __resetActiveGoalStoreForTests, | ||
| clearActiveGoal, | ||
| setActiveGoal, | ||
| } from '../goals/activeGoalStore.js'; | ||
|
|
||
| // Mock fs module to prevent actual file system operations during tests | ||
| const mockFileSystem = new Map<string, string>(); | ||
|
|
@@ -514,6 +519,7 @@ describe('Gemini Client (client.ts)', () => { | |
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| __resetActiveGoalStoreForTests(); | ||
| }); | ||
|
|
||
| describe('initialize', () => { | ||
|
|
@@ -4501,6 +4507,89 @@ Other open files: | |
| client['chat'] = mockChat as GeminiChat; | ||
| }); | ||
|
|
||
| it('emits active_goal when a goal is active for the turn', async () => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] All 5 new This is the most common production path (every Stop hook iteration increments — qwen-latest-series-invite-beta-v28 via Qwen Code /review |
||
| setActiveGoal('test-session-id', { | ||
| condition: 'finish the refactor', | ||
| iterations: 2, | ||
| setAt: 123, | ||
| tokensAtStart: 456, | ||
| hookId: 'goal-hook-id', | ||
| lastReason: 'still missing verification', | ||
| }); | ||
|
|
||
| const events = await fromAsync( | ||
| client.sendMessageStream( | ||
| [{ text: 'Hi' }], | ||
| new AbortController().signal, | ||
| 'prompt-active-goal', | ||
| ), | ||
| ); | ||
|
|
||
| expect(events[0]).toEqual({ | ||
| type: GeminiEventType.ActiveGoal, | ||
| value: { | ||
| condition: 'finish the refactor', | ||
| iterations: 2, | ||
| setAt: 123, | ||
| tokensAtStart: 456, | ||
| hookId: 'goal-hook-id', | ||
| lastReason: 'still missing verification', | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| it('emits active_goal null when the Stop hook clears the goal', async () => { | ||
| setActiveGoal('test-session-id', { | ||
| condition: 'finish the refactor', | ||
| iterations: 2, | ||
| setAt: 123, | ||
| tokensAtStart: 456, | ||
| hookId: 'goal-hook-id', | ||
| lastReason: 'still missing verification', | ||
| }); | ||
| const mockMessageBus = { | ||
| request: vi.fn().mockImplementation(async () => { | ||
| clearActiveGoal('test-session-id'); | ||
| return {}; | ||
| }), | ||
| response: vi.fn(), | ||
| }; | ||
| vi.mocked(mockConfig.getDisableAllHooks).mockReturnValue(false); | ||
| vi.mocked(mockConfig.getMessageBus).mockReturnValue( | ||
| mockMessageBus as unknown as ReturnType<Config['getMessageBus']>, | ||
| ); | ||
| vi.mocked(mockConfig.hasHooksForEvent).mockImplementation( | ||
| (event: string) => event === 'Stop', | ||
| ); | ||
| client['chat'] = { | ||
| addHistory: vi.fn(), | ||
| getHistory: vi.fn().mockReturnValue([ | ||
| { | ||
| role: 'model', | ||
| parts: [{ text: 'done' }], | ||
| }, | ||
| ]), | ||
| } as unknown as GeminiChat; | ||
| mockTurnRunFn.mockReturnValue( | ||
| (async function* () { | ||
| yield { type: GeminiEventType.Content, value: 'done' }; | ||
| })(), | ||
| ); | ||
|
|
||
| const events = await fromAsync( | ||
| client.sendMessageStream( | ||
| [{ text: 'Hi' }], | ||
| new AbortController().signal, | ||
| 'prompt-cleared-active-goal', | ||
| ), | ||
| ); | ||
|
|
||
| expect(events).toContainEqual({ | ||
| type: GeminiEventType.ActiveGoal, | ||
| value: null, | ||
| }); | ||
| }); | ||
|
|
||
| it('should skip messageBus.request for UserPromptSubmit when hasHooksForEvent returns false', async () => { | ||
| // Enable hooks and provide messageBus | ||
| const mockMessageBus = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,12 +18,28 @@ import { ApprovalMode, type Config } from '../config/config.js'; | |||||||||||||||||
| import { createDebugLogger } from '../utils/debugLogger.js'; | ||||||||||||||||||
| import { recordStartupEvent } from '../utils/startupEventSink.js'; | ||||||||||||||||||
| import { microcompactHistory } from '../services/microcompaction/microcompact.js'; | ||||||||||||||||||
| import { getActiveGoal } from '../goals/activeGoalStore.js'; | ||||||||||||||||||
| import { getActiveGoal, type ActiveGoal } from '../goals/activeGoalStore.js'; | ||||||||||||||||||
| import { abortGoalForStopHookCap } from '../goals/goalHook.js'; | ||||||||||||||||||
| import { formatStopHookBlockingCapWarning } from '../hooks/stopHookCap.js'; | ||||||||||||||||||
|
|
||||||||||||||||||
| const debugLogger = createDebugLogger('CLIENT'); | ||||||||||||||||||
|
|
||||||||||||||||||
| function activeGoalEquals( | ||||||||||||||||||
| left: ActiveGoal | undefined, | ||||||||||||||||||
| right: ActiveGoal | undefined, | ||||||||||||||||||
| ): boolean { | ||||||||||||||||||
| if (left === right) return true; | ||||||||||||||||||
| if (!left || !right) return false; | ||||||||||||||||||
| return ( | ||||||||||||||||||
| left.condition === right.condition && | ||||||||||||||||||
| left.iterations === right.iterations && | ||||||||||||||||||
| left.setAt === right.setAt && | ||||||||||||||||||
| left.tokensAtStart === right.tokensAtStart && | ||||||||||||||||||
| left.hookId === right.hookId && | ||||||||||||||||||
| left.lastReason === right.lastReason | ||||||||||||||||||
| ); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // Core modules | ||||||||||||||||||
| import { GeminiChat } from './geminiChat.js'; | ||||||||||||||||||
| import { | ||||||||||||||||||
|
|
@@ -1454,6 +1470,14 @@ export class GeminiClient { | |||||||||||||||||
| requestToSend = [...systemReminders, ...requestToSend]; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| const activeGoalAtTurnStart = getActiveGoal(this.config.getSessionId()); | ||||||||||||||||||
| if (activeGoalAtTurnStart) { | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Critical] Recursive The raw stream contains: The UI handler masks this via Fix: Skip the initial emit for hook continuations:
Suggested change
— qwen-latest-series-invite-beta-v28 via Qwen Code /review |
||||||||||||||||||
| yield { | ||||||||||||||||||
| type: GeminiEventType.ActiveGoal, | ||||||||||||||||||
| value: activeGoalAtTurnStart, | ||||||||||||||||||
| }; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| const resultStream = turn.run(model, requestToSend, signal); | ||||||||||||||||||
| let didUpdateIdeContextState = false; | ||||||||||||||||||
| for await (const event of resultStream) { | ||||||||||||||||||
|
|
@@ -1554,6 +1578,9 @@ export class GeminiClient { | |||||||||||||||||
| .map((p) => p.text) | ||||||||||||||||||
| .join('') || '[no response text]'; | ||||||||||||||||||
|
|
||||||||||||||||||
| const activeGoalBeforeStopHook = getActiveGoal( | ||||||||||||||||||
| this.config.getSessionId(), | ||||||||||||||||||
| ); | ||||||||||||||||||
| const response = await messageBus.request< | ||||||||||||||||||
| HookExecutionRequest, | ||||||||||||||||||
| HookExecutionResponse | ||||||||||||||||||
|
|
@@ -1581,6 +1608,13 @@ export class GeminiClient { | |||||||||||||||||
| : undefined; | ||||||||||||||||||
|
|
||||||||||||||||||
| const stopOutput = hookOutput as StopHookOutput | undefined; | ||||||||||||||||||
| const activeGoalAfterStopHook = getActiveGoal( | ||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] If the Stop hook callback clears the goal during
Consider snapshotting — DeepSeek/deepseek-v4-pro via Qwen Code /review |
||||||||||||||||||
| this.config.getSessionId(), | ||||||||||||||||||
| ); | ||||||||||||||||||
| const didActiveGoalChange = !activeGoalEquals( | ||||||||||||||||||
| activeGoalBeforeStopHook, | ||||||||||||||||||
| activeGoalAfterStopHook, | ||||||||||||||||||
| ); | ||||||||||||||||||
|
|
||||||||||||||||||
| // This should happen regardless of the hook's decision | ||||||||||||||||||
| if (stopOutput?.systemMessage) { | ||||||||||||||||||
|
|
@@ -1626,6 +1660,12 @@ export class GeminiClient { | |||||||||||||||||
| this.config.getSessionId(), | ||||||||||||||||||
| warning, | ||||||||||||||||||
| ); | ||||||||||||||||||
| if (activeGoalBeforeStopHook || activeGoalAfterStopHook) { | ||||||||||||||||||
| yield { | ||||||||||||||||||
| type: GeminiEventType.ActiveGoal, | ||||||||||||||||||
| value: null, | ||||||||||||||||||
| }; | ||||||||||||||||||
| } | ||||||||||||||||||
| yield { | ||||||||||||||||||
| type: GeminiEventType.HookSystemMessage, | ||||||||||||||||||
| value: warning, | ||||||||||||||||||
|
|
@@ -1635,6 +1675,13 @@ export class GeminiClient { | |||||||||||||||||
| return turn; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| if (didActiveGoalChange) { | ||||||||||||||||||
| yield { | ||||||||||||||||||
| type: GeminiEventType.ActiveGoal, | ||||||||||||||||||
| value: activeGoalAfterStopHook ?? null, | ||||||||||||||||||
| }; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| yield { | ||||||||||||||||||
| type: GeminiEventType.StopHookLoop, | ||||||||||||||||||
| value: { | ||||||||||||||||||
|
|
@@ -1665,6 +1712,13 @@ export class GeminiClient { | |||||||||||||||||
| endInteractionSpan(signal.aborted ? 'cancelled' : 'ok'); | ||||||||||||||||||
| return hookTurn; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| if (didActiveGoalChange) { | ||||||||||||||||||
| yield { | ||||||||||||||||||
| type: GeminiEventType.ActiveGoal, | ||||||||||||||||||
| value: activeGoalAfterStopHook ?? null, | ||||||||||||||||||
| }; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| if (!turn.pendingToolCalls.length && signal && !signal.aborted) { | ||||||||||||||||||
|
|
||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Critical]
ActiveGoalcase is the sole exception among 22+ event handlers that skipsflushBufferedStreamEvents(). Every other case (Content, Thought, Citation, HookSystemMessage, StopHookLoop, etc.) calls flush before processing.When
ActiveGoalarrives after content events still in the debounce buffer, the goal-state mutation happens before preceding content is committed to history. Today this is mostly latent (the handler only writes to an in-memory store), but any future change makinghandleActiveGoalEventproduce a history item or trigger a UI update will immediately cause ordering bugs.— qwen-latest-series-invite-beta-v28 via Qwen Code /review