Skip to content

fix(protocol,client): resolve subagent type mismatches breaking CI#312

Merged
dimakis merged 4 commits into
mainfrom
fix/subagent-type-errors
May 9, 2026
Merged

fix(protocol,client): resolve subagent type mismatches breaking CI#312
dimakis merged 4 commits into
mainfrom
fix/subagent-type-errors

Conversation

@dimakis
Copy link
Copy Markdown
Owner

@dimakis dimakis commented May 4, 2026

Summary

  • Fixes the CI build failure on main caused by subagent type mismatches in packages/client and frontend
  • StreamingBlock.subagent was typed as StreamingSubagentState only, but the SUBAGENT_END reducer replaces it with FinishedSubagentState (subagent finishes while parent message is still streaming)
  • finishCurrent() copied b.subagent without converting from streaming to finished form
  • SubagentCard.tsx imported FinishedSubagentState/StreamingSubagentState which weren't exported from @mitzo/protocol

Changes

  • packages/protocol/src/types.ts: Widen StreamingBlock.subagent to StreamingSubagentState | FinishedSubagentState
  • packages/protocol/src/index.ts: Export StreamingSubagentState, FinishedSubagentState, SubagentState, SubagentUsage
  • packages/client/src/slices/messages.ts: Add finishSubagent() helper (handles both Map→array conversion and finished passthrough), cast to StreamingSubagentState in streaming reducer cases

Test plan

  • packages/client/__tests__/messages-slice.test.ts — 64 tests pass
  • packages/harness/__tests__/connection-registry.test.ts — 37 tests pass
  • npx tsc -b in frontend — clean
  • npm run build — full Vite build succeeds

🤖 Generated with Claude Code

StreamingBlock.subagent was typed as StreamingSubagentState only, but
the SUBAGENT_END reducer replaces it with a FinishedSubagentState
(subagent finishes while parent message is still streaming). Also,
finishCurrent() copied the streaming subagent without conversion, and
SubagentCard imported types not exported from @mitzo/protocol.

- Widen StreamingBlock.subagent to accept both streaming and finished
- Export FinishedSubagentState, StreamingSubagentState, SubagentState,
  SubagentUsage from @mitzo/protocol
- Add finishSubagent() helper that handles both states (Map→array for
  streaming, passthrough for already-finished)
- Cast to StreamingSubagentState in streaming reducer cases where the
  subagent is known to still be running

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dimakis
Copy link
Copy Markdown
Owner Author

dimakis commented May 4, 2026

Centaur Review

Found 3 issue(s) (2 warning).

packages/client/src/slices/messages.ts

Type-safe fix for the subagent union type, but the as StreamingSubagentState casts bypass the new union without narrowing, and the existing test assertions may produce tsc errors under the widened type.

  • 🟡 bugs (L560): Unsafe as StreamingSubagentState cast (repeated in 5 reducer cases: lines 560, 590, 614, 642, 671). If a SUBAGENT_BLOCK_* or SUBAGENT_TOOL_RESULT event arrives after SUBAGENT_END has already converted the subagent to FinishedSubagentState, the cast silently lies — sub.blocks is a FinishedBlock[] not a Map, and sub.blockOrder doesn't exist. This would throw at runtime. A narrowing guard (if (!('blockOrder' in parentBlock.subagent)) return state; or checking blocks instanceof Map) would make this safe without relying on event ordering guarantees. [fixable]
  • 🔵 missing_tests (L148): The finishSubagent helper's early-return branch (Array.isArray(sub.blocks) — subagent already finished) is untested. This path is exercised when finishCurrent is called on a message whose subagent already received SUBAGENT_END. A test that runs the full SUBAGENT_START → SUBAGENT_END → MESSAGE_END sequence would cover this. [fixable]

packages/client/__tests__/subagent-reducer.test.ts

Type-safe fix for the subagent union type, but the as StreamingSubagentState casts bypass the new union without narrowing, and the existing test assertions may produce tsc errors under the widened type.

  • 🟡 regressions (L39): Existing tests access subagent?.blocks.size, subagent?.blocks.get(...), and subagent?.blockOrder — all Map/StreamingSubagentState-specific APIs. With the type of StreamingBlock.subagent widened to StreamingSubagentState | FinishedSubagentState, these expressions are type errors (FinishedBlock[] has no .size or .get()). The tests pass at runtime (vitest uses esbuild which strips types) but will fail any tsc --noEmit check that includes test files — potentially re-breaking the CI this PR aims to fix. [fixable]

Address Centaur review findings on PR #312:

- Replace 5 `as StreamingSubagentState` casts with `'blockOrder' in`
  narrowing guards that safely bail if the subagent is already finished
- Fix test type errors: cast to StreamingSubagentState/FinishedSubagentState
  in assertions so tests pass tsc --noEmit (not just esbuild)
- Add two tests for finishSubagent: passthrough path (already-finished)
  and Map→array conversion path (still-streaming at MESSAGE_END)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dimakis
Copy link
Copy Markdown
Owner Author

dimakis commented May 5, 2026

Centaur Review

Found 5 issue(s) (2 warning).

packages/client/src/slices/messages.ts

Solid fix that properly types the streaming→finished subagent transition and adds the missing finishSubagent converter. The guard pattern ('blockOrder' in parentBlock.subagent) is correct but repetitive — consider extracting a helper. The non-null assertions in finishSubagent mirror existing patterns but add another crash site if protocol invariants break.

  • 🟡 bugs (L559): The 'blockOrder' in parentBlock.subagent guard correctly rejects FinishedSubagentState (which lacks blockOrder), but it also means if a second SUBAGENT_BLOCK_START arrives after SUBAGENT_END has already fired (e.g., out-of-order WebSocket delivery), the event is silently dropped with no diagnostic. This is probably the right behavior, but worth a brief comment since the same guard is repeated verbatim in five case arms (SUBAGENT_BLOCK_START/DELTA/END, SUBAGENT_TOOL_RESULT, SUBAGENT_END). Consider extracting a tiny helper like getStreamingSubagent(parentBlock) to DRY it up and centralize the invariant. [fixable]
  • 🟡 unsafe_assumptions (L155): The non-null assertion streaming.blocks.get(blockId)! in finishSubagent mirrors the same pattern in finishCurrent (line 173) and SUBAGENT_END (line 680). If blockOrder ever contains a stale blockId not in the Map (e.g., due to a protocol bug or dropped event), this will throw at runtime. A defensive fallback (.get(blockId) ?? { blockId, blockType: 'text', content: '' }) or a filter(Boolean) would be more resilient, matching the general pattern where the project guards against protocol edge cases (e.g., groupBlocks() guards against undefined input per CLAUDE.md). [fixable]

packages/client/__tests__/subagent-reducer.test.ts

Solid fix that properly types the streaming→finished subagent transition and adds the missing finishSubagent converter. The guard pattern ('blockOrder' in parentBlock.subagent) is correct but repetitive — consider extracting a helper. The non-null assertions in finishSubagent mirror existing patterns but add another crash site if protocol invariants break.

  • 🔵 missing_tests: The new finishCurrent tests cover the happy path (already-finished passthrough) and the edge case (still-streaming conversion), but there is no test for a block with subagent: undefined flowing through finishSubagent — this is implicitly covered by the ternary in finishCurrent (line 184), but an explicit test would document the contract. Additionally, there's no test for finishSubagent when blockOrder references a blockId not present in the Map (the ! assertion on line 155 would throw). This is an unlikely edge case but the ! is a potential null dereference. [fixable]
  • 🔵 style (L40): Several test assertions use as StreamingSubagentState or as FinishedSubagentState type casts. While this is a pragmatic fix for the CI type errors, these casts bypass the type checker and could mask regressions if the underlying type changes. Consider using a type-narrowing helper (e.g., an isStreamingSubagent function that checks sub.blocks instanceof Map) to keep the tests type-safe. [fixable]

packages/client/__tests__/messages-slice.test.ts

Solid fix that properly types the streaming→finished subagent transition and adds the missing finishSubagent converter. The guard pattern ('blockOrder' in parentBlock.subagent) is correct but repetitive — consider extracting a helper. The non-null assertions in finishSubagent mirror existing patterns but add another crash site if protocol invariants break.

  • 🔵 regressions (L1183): The existing finishCurrent test at line 1146 sets up a streaming subagent with running: true but doesn't verify the output shape — it only checks messageId is preserved. After this PR, finishSubagent converts the Map to an array, so the existing test should also verify that finished.blocks[0].subagent!.blocks is now an array (not a Map), to confirm the new conversion logic works in the pre-existing test fixture. [fixable]

dimakis and others added 2 commits May 6, 2026 06:45
…loop

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address Centaur review round 2:

- Extract getStreamingSubagent() helper to DRY the 5 repeated
  'blockOrder' in narrowing guards into a single function
- Replace non-null assertions (!) with .filter(Boolean) in both
  finishSubagent and SUBAGENT_END — handles stale blockOrder entries
- Add test: stale blockId in blockOrder is safely skipped
- Add test: streaming events after SUBAGENT_END are silently dropped
- Update existing finishCurrent test to verify Map→array conversion

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@dimakis dimakis merged commit 5dfbe41 into main May 9, 2026
1 check passed
@dimakis dimakis deleted the fix/subagent-type-errors branch May 9, 2026 13:35
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