Support active goal stream events and non-interactive goals#4273
Conversation
📋 Review SummaryThis PR introduces a new 🔍 General Feedback
🎯 Specific Feedback🔴 Critical
🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
wenshao
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing: Test (windows-latest, Node 22.x). — gpt-5.5 via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing: Test (windows-latest, Node 22.x). — DeepSeek/deepseek-v4-pro via Qwen Code /review
| }; | ||
| } | ||
| let lastEmittedActiveGoal: ActiveGoal | undefined = activeGoalAtTurnStart; | ||
| const createActiveGoalChangeEvent = ( |
There was a problem hiding this comment.
[Suggestion] createActiveGoalChangeEvent has hidden mutable state and the outer didActiveGoalChange guards are inconsistent across the three Stop hook paths.
- This closure mutates
lastEmittedActiveGoalvia reference capture (L1475), but its name suggests a pure "create" function — making it easy to call multiple times without realizing the side effect. - The blocking loop path (L1681) and non-blocking path (L1721) wrap calls in
if (didActiveGoalChange), which is redundant — the closure already does the same equality check internally viaactiveGoalEquals. The cap path (L1668) correctly omits the guard.
| const createActiveGoalChangeEvent = ( | |
| // Tracks the last emitted goal value to suppress duplicate events. | |
| // Mutates `lastEmittedActiveGoal` — call at most once per goal change. | |
| const maybeEmitActiveGoalChange = ( | |
| nextActiveGoal: ActiveGoal | undefined, | |
| ): ServerGeminiStreamEvent | undefined => { | |
| if (activeGoalEquals(lastEmittedActiveGoal, nextActiveGoal)) { | |
| return undefined; | |
| } | |
| lastEmittedActiveGoal = nextActiveGoal; | |
| return { | |
| type: GeminiEventType.ActiveGoal, | |
| value: nextActiveGoal ?? null, | |
| }; | |
| }; |
Also remove the redundant if (didActiveGoalChange) wrappers on L1681 and L1721 to match the cap path style.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| : undefined; | ||
|
|
||
| const stopOutput = hookOutput as StopHookOutput | undefined; | ||
| const activeGoalAfterStopHook = getActiveGoal( |
There was a problem hiding this comment.
[Suggestion] If the Stop hook callback clears the goal during messageBus.request() and signal.aborted is true after the request returns, activeGoalAfterStopHook (this line) is never reached. The active_goal: null event is silently dropped.
activeGoalBeforeStopHook is snapshotted at L1582 before the hook runs. The hook callback can call finishGoal → clearActiveGoal inside messageBus.request(). If the signal is then aborted at the check ~10 lines above this block, we return without emitting the goal-cleared event.
Consider snapshotting activeGoalAfterStopHook immediately after messageBus.request() returns, before the signal.aborted guard, so the change event is emitted even on cancellation.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
# Conflicts: # packages/core/src/core/client.test.ts # packages/core/src/core/client.ts
wenshao
left a comment
There was a problem hiding this comment.
No new findings. LGTM! ✅
— DeepSeek/deepseek-v4-pro via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
补充一个发现（blocking-decision 块内的第二个 signal.aborted 检查未 patch，但因该行不在 diff 范围内无法作为 inline comment 发布）:
packages/core/src/core/client.ts L1638 — 第一个 signal.aborted 检查（L1607）正确地通过 maybeEmitActiveGoalChange(activeGoalAfterStopHook) 发射了 goal 变更事件，但 blocking-decision 块内的第二个 signal.aborted 检查直接 return turn，未发射 goal 状态。如果 Stop hook 回调修改了 goal store（如 recordGoalIteration），且 signal 在两次检查之间变为 aborted，UI 将永远不会收到迭代计数变更。窗口极窄（两次检查间无 await），但确实存在。
if (signal.aborted) {
const activeGoalEvent = maybeEmitActiveGoalChange(
activeGoalAfterStopHook,
);
if (activeGoalEvent) {
yield activeGoalEvent;
}
if (isTopLevelInteraction) endInteractionSpan('cancelled');
return turn;
}
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| flushBufferedStreamEvents(); | ||
| handleStopHookLoopEvent(event.value, userMessageTimestamp); | ||
| break; | ||
| case ServerGeminiEventType.ActiveGoal: |
There was a problem hiding this comment.
[Critical] ActiveGoal case is the sole exception among 22+ event handlers that skips flushBufferedStreamEvents(). Every other case (Content, Thought, Citation, HookSystemMessage, StopHookLoop, etc.) calls flush before processing.
When ActiveGoal arrives 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 making handleActiveGoalEvent produce a history item or trigger a UI update will immediately cause ordering bugs.
| case ServerGeminiEventType.ActiveGoal: | |
| case ServerGeminiEventType.ActiveGoal: | |
| flushBufferedStreamEvents(); | |
| handleActiveGoalEvent(event.value); | |
| break; |
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
| } | ||
|
|
||
| const activeGoalAtTurnStart = getActiveGoal(this.config.getSessionId()); | ||
| if (activeGoalAtTurnStart) { |
There was a problem hiding this comment.
[Critical] Recursive yield* at L1710 produces duplicate ActiveGoal events in the raw stream. When a Stop hook blocks and triggers continuation, the child sendMessageStream creates a fresh lastEmittedActiveGoal closure and unconditionally emits ActiveGoal(activeGoalAtTurnStart) here — even though the parent already emitted the same value at its own turn start.
The raw stream contains:
ActiveGoal(G) ← parent turn start
... content ...
ActiveGoal(G) ← child turn start (DUPLICATE)
The UI handler masks this via activeGoalEquals dedup, but the stream contract ("emit on change") is violated. Any future stream consumer trusting the dedup contract will silently misbehave. No test covers this path.
Fix: Skip the initial emit for hook continuations:
| if (activeGoalAtTurnStart) { | |
| const activeGoalAtTurnStart = getActiveGoal(this.config.getSessionId()); | |
| if (activeGoalAtTurnStart && options?.type !== SendMessageType.Hook) { | |
| yield { | |
| type: GeminiEventType.ActiveGoal, | |
| value: activeGoalAtTurnStart, | |
| }; | |
| } |
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
| let lastEmittedActiveGoal: ActiveGoal | undefined = activeGoalAtTurnStart; | ||
| // Tracks the last emitted goal value to suppress duplicate events. | ||
| // Mutates `lastEmittedActiveGoal` when an event is returned. | ||
| const maybeEmitActiveGoalChange = ( |
There was a problem hiding this comment.
[Suggestion] Zero observability on the active_goal state-sync path. Neither maybeEmitActiveGoalChange nor handleActiveGoalEvent (in useGeminiStream.ts) produces any debug logging. A stuck or missed goal event is undiagnosable from logs — you'd need a debugger to trace whether core emitted the event, dedup suppressed it, or the CLI handler processed it.
Consider adding debugLogger.debug(...) at emit/suppress/clear decision points in both functions. Even a single line like debugLogger.debug('active_goal emit', { sessionId, value }) would transform this from undiagnosable to trivially traceable.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
|
|
||
| // Check if aborted after hook execution | ||
| if (signal.aborted) { | ||
| const activeGoalEvent = maybeEmitActiveGoalChange( |
There was a problem hiding this comment.
[Suggestion] The pattern const event = maybeEmitActiveGoalChange(...); if (event) { yield event; } is repeated 4 times across exit paths (L1608, L1639, L1691, L1729). Adding a new exit path and forgetting the call would silently leave the CLI with a stale goal view — and because of the missing observability (see L1471 comment), there would be no log to point to the missing emission.
Consider restructuring with a deferred/finally pattern — capture the goal state after the Stop hook, then emit the change in a single place that all paths flow through. This eliminates the class of bug entirely rather than relying on manual discipline across 4+ paths.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
| client['chat'] = mockChat as GeminiChat; | ||
| }); | ||
|
|
||
| it('emits active_goal when a goal is active for the turn', async () => { |
There was a problem hiding this comment.
[Suggestion] All 5 new active_goal tests verify either goal-cleared→null or goal-unchanged→dedup-skipped. No test covers the recordGoalIteration path where the goal is modified mid-turn (new iterations/lastReason), which should emit an updated ActiveGoal event (non-null) through maybeEmitActiveGoalChange.
This is the most common production path (every Stop hook iteration increments iterations), yet it has zero test coverage. Consider adding a test that calls recordGoalIteration(sessionId, 'progress note') inside the messageBus.request callback and asserts the stream contains an ActiveGoal event with the updated iteration count.
— qwen-latest-series-invite-beta-v28 via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen-latest-series-invite-beta-v28 via Qwen Code /review
维护者本地真实测试报告在 worktree（HEAD 通过项
兼容性审查
风险面
LGTM，merge。 |
Summary
active_goalstream event carrying the current active goal snapshot ornull, emitted at turn start and when Stop hooks update or clear the goal. The CLI now consumes the event to keepactiveGoalStoresynchronized./goalis also available in non-interactive CLI mode and is handled as asubmit_promptslash command./goal.Validation
/goal, Stop hook goal clearing,active_goal/nullstream events, and/goal write a hello world scriptthrough the non-interactive slash-command handler./goal <condition>registers the goal then submits the instructional prompt.src/core/client.test.tsnow covers active goal snapshot emission andactive_goal: nullafter Stop hook cleanup;src/ui/hooks/useGeminiStream.test.tsxcovers CLI store synchronization;src/nonInteractiveCliCommands.test.tscovers non-interactive/goalexecution.Scope / Risk
active_goalintentionally. Non-interactive/goalnow registers a session Stop hook in headless runs.GeminiEventTypeswitches may need to add anActiveGoalcase.Testing Matrix
Testing matrix notes:
npx vitestcommands plusnpm run build && npm run typecheck.Linked Issues / Bugs