Skip to content

Cover active goal stream JSON and headless status - #4292

Closed
qqqys wants to merge 10 commits into
QwenLM:mainfrom
qqqys:feat/goal-p1-protocol-coverage
Closed

Cover active goal stream JSON and headless status#4292
qqqys wants to merge 10 commits into
QwenLM:mainfrom
qqqys:feat/goal-p1-protocol-coverage

Conversation

@qqqys

@qqqys qqqys commented May 18, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • emit active goal updates as stream-json partial events when partial messages are enabled
  • add the active goal stream event type to the headless message schema
  • cover headless /goal status, active status, and clear output semantics

Validation

  • cd packages/cli && npx vitest run src/nonInteractive/io/StreamJsonOutputAdapter.test.ts src/nonInteractiveCliCommands.test.ts src/ui/commands/goalCommand.test.ts src/ui/hooks/useGeminiStream.test.tsx
  • npm run build && npm run typecheck

Notes

@github-actions

Copy link
Copy Markdown
Contributor

📋 Review Summary

This PR implements active goal stream event emission for the headless JSON output protocol, adding comprehensive coverage for goal status tracking during automatic /goal loops. The changes are well-structured, follow existing patterns, and include thorough test coverage for edge cases including abort scenarios and stop hook interactions.

🔍 General Feedback

  • Strong test coverage: The PR includes extensive tests for edge cases (abort during stop hook, goal clearing, iteration changes, blocking cap scenarios)
  • Good separation of concerns: Core event emission logic is cleanly separated from the output adapter
  • Consistent patterns: Follows existing stream event patterns (similar to tool progress events)
  • Type safety: Proper TypeScript types with ServerGeminiActiveGoalEvent and ActiveGoalStreamEvent
  • Deduplication logic: The activeGoalEquals function prevents duplicate event emission with a stable key comparison

🎯 Specific Feedback

🟡 High

  • File: packages/core/src/core/client.ts:1470-1480 - The maybeEmitActiveGoalChange closure captures lastEmittedActiveGoal by reference and mutates it, which works but is slightly confusing. Consider extracting this into a class member or a small helper class for clarity:

    // Current approach (works but subtle)
    let lastEmittedActiveGoal: ActiveGoal | undefined = activeGoalAtTurnStart;
    const maybeEmitActiveGoalChange = (nextActiveGoal: ActiveGoal | undefined) => {
      if (activeGoalEquals(lastEmittedActiveGoal, nextActiveGoal)) {
        return undefined;
      }
      lastEmittedActiveGoal = nextActiveGoal; // mutation inside closure
      ...
    };

    Suggestion: Extract to a private method or class field for better readability.

  • File: packages/core/src/goals/activeGoalStore.ts:23-40 - The stableActiveGoalKey function uses JSON.stringify on a sorted key object for comparison. This is correct but could be fragile if new fields are added to ActiveGoal with different equality semantics (e.g., if a field should be ignored in comparisons).
    Suggestion: Add a comment documenting that all ActiveGoal fields participate in equality, or explicitly list which fields are compared.

🟢 Medium

  • File: packages/cli/src/nonInteractive/io/StreamJsonOutputAdapter.ts:127-139 - The processEvent override handles only ActiveGoal events and delegates everything else to super.processEvent(). This is correct, but consider adding a comment explaining why this adapter needs to handle ActiveGoal events separately (i.e., because it emits stream events for partial messages while the base class doesn't).

  • File: packages/core/src/core/turn.ts:68 - The ActiveGoal enum member is added but lacks documentation.
    Suggestion: Add a brief comment explaining when this event type is emitted:

    /** Emitted when the active goal state changes or is queried at turn start */
    ActiveGoal = 'active_goal',
  • File: packages/cli/src/nonInteractive/types.ts:250-254 - The ActiveGoalStreamEvent interface uses active_goal: ActiveGoal | null. The null case represents goal clearing, which is clear from context, but a brief JSDoc comment would help future maintainers:

    /** Stream event for active goal updates; null indicates goal was cleared/completed */
    export interface ActiveGoalStreamEvent {
      type: 'active_goal';
      active_goal: ActiveGoal | null;
    }

🔵 Low

  • File: packages/core/src/goals/activeGoalStore.test.ts:65-77 - The activeGoalEquals test is good but could benefit from one more edge case: testing that undefined vs null are distinguished (since the function handles undefined but events use null for cleared goals). This is currently covered indirectly but an explicit test would document the expected behavior.

  • File: packages/cli/src/nonInteractive/io/StreamJsonOutputAdapter.test.ts:123-157 - The test filters events by parsing JSON from mock calls. Consider extracting this filtering logic into a small helper function to reduce duplication if more stream event tests are added in the future.

  • File: packages/core/src/hooks/promptHookRunner.test.ts:474-498 - The test change to use fake timers is good. However, the try/finally block adds indentation. Consider using beforeEach/afterEach for timer setup/teardown:

    beforeEach(() => {
      vi.useFakeTimers();
      vi.setSystemTime(0);
    });
    afterEach(() => {
      vi.useRealTimers();
    });

✅ Highlights

  • Excellent edge case coverage: Tests for abort scenarios during stop hook execution, blocking cap interactions, and goal state transitions demonstrate thorough thinking about the async control flow
  • Clean deduplication: The activeGoalEquals function with stable key comparison is a robust solution for preventing duplicate events
  • Minimal invasive changes: The core logic changes are localized to client.ts and turn.ts, with adapter changes following established patterns
  • Good use of TypeScript: Proper typing throughout with new event types properly integrated into the union types
  • Session isolation: Goal state is properly scoped by sessionId, preventing cross-session contamination

@qqqys

qqqys commented May 18, 2026

Copy link
Copy Markdown
Collaborator Author

Closing because this branch was based on feat/active-goal-event and unintentionally includes the #4273 changes. I will reopen from a clean upstream main branch with only the intended delta.

@qqqys qqqys closed this May 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant