📋 Review Summary

This PR fixes a critical bug in the TurnBoundaryCompactionEngine.mergeTextSlot where parallel subagent streams were being incorrectly merged, causing text/thought garbling and loss of parentToolCallId attribution. The implementation introduces a dual-path merge logic that properly separates subagent streams by (kind, parentToolCallId) while preserving the original behavior for top-level chunks. The fix is well-tested with 9 comprehensive test cases covering edge cases and parallel stress scenarios.

🔍 General Feedback

• Clean dual-path architecture: The separation between subagent path (indexed merge by parentToolCallId) and top-level path (consecutive merge) is a sound design that mirrors the transcript reducer's behavior.
• Comprehensive test coverage: The 9 new test cases cover critical scenarios including interleaved streams, parallel stress (9 subagents), defensive backfill, and boundary conditions.
• Defensive programming: The defensive backfill in compactCurrentTurn ensures parentToolCallId is preserved in output even if lost from the last chunk's _meta.
• State cleanup: Proper clearing of textSlotIndex in seed(), close(), and compactCurrentTurn() prevents state leakage across turns.

🎯 Specific Feedback

🟡 High

• File: compactionEngine.ts:218-226 - The slot eviction logic on tool calls uses string concatenation for slot keys (text::${toolParent}), but the same pattern in mergeTextSlot (line 204) uses the identical format. Consider extracting this key format into a constant or helper function to prevent future drift:

  // Suggested extraction
  const makeSlotKey = (kind: 'text' | 'thought', parentToolCallId: string) => 
    `${kind}::${parentToolCallId}`;

• File: compactionEngine.ts:269-276 - The defensive backfill logic in compactCurrentTurn checks extractParentToolCallIdFromMeta(meta) !== slot.parentToolCallId. This comparison could be simplified since extractParentToolCallIdFromMeta already handles null/undefined cases. Consider documenting why this check is needed (i.e., when _meta might lose parentToolCallId between chunk ingestion and compaction).

🟢 Medium

• File: compactionEngine.ts:44-49 - The CompactedSlot type now uses a union with parentToolCallId?: string for both text and thought kinds. This is correct, but consider whether parentToolCallId should be part of the discriminated union variant itself to make the type more self-documenting:

  type CompactedSlot =
    | { kind: 'text' | 'thought'; parentToolCallId: string; chunks: string[]; ... }  // subagent
    | { kind: 'text' | 'thought'; parentToolCallId?: undefined; chunks: string[]; ... }  // top-level
    | { kind: 'tool'; ... }
    | { kind: 'misc'; ... }
    | { kind: 'latestWins'; ... };

• File: compactionEngine.test.ts:886-897 - The defensive backfill test verifies that chunks without parentToolCallId in _meta go to the top-level path. However, the test comment says "The chunk without parentToolCallId goes to the top-level path" — this is actually testing a different scenario than the code comment describes (defensive backfill when last chunk loses _meta). Consider clarifying the test to explicitly test the defensive backfill case where the slot HAS parentToolCallId but the last chunk's _meta doesn't.

🔵 Low

• File: compactionEngine.ts:344-351 - The extractParentToolCallIdFromMeta function checks val.length > 0 for string validation. Consider using a more explicit check or extracting this to a helper for consistency with other string validation in the codebase.

• File: compactionEngine.ts:108-113 - The seed() method resets liveJournal to an empty array. This is correct for replay scenarios, but consider adding a comment explaining why liveJournal is cleared (i.e., it's transient state for the current turn, not persisted across snapshots).

• File: compactionEngine.test.ts:684-688 - The UpdatePayload type is defined inside the describe block. While this is fine for test organization, consider moving it to the top of the file with other helper types for better discoverability.

✅ Highlights

• Excellent test design: The 9-parallel-subagent stress test (handles 9 parallel subagent thought streams without garbling) is particularly well-crafted, verifying that each subagent's stream is reconstructed correctly without cross-contamination.
• Correct handling of tool call boundaries: The eviction logic (textSlotIndex.delete on tool calls with parent) correctly mirrors the transcript reducer's clearActiveText(parentToolCallId) behavior, ensuring compacted replay matches live stream segmentation.
• Minimal invasive change: The fix modifies only the mergeTextSlot method and adds the textSlotIndex map, keeping the change focused and reducing regression risk.
• Type-safe implementation: The use of TypeScript's discriminated unions and proper type narrowing in compactCurrentTurn ensures type safety throughout the refactoring.

Review round — wenshao (fe46e37)

All 3 items addressed. 38/38 tests passing.

Review round 2 — wenshao (5be1f3d)

1/1 item addressed. 39/39 tests passing.

PR #4765 Local Verification Report

Branch: daemon_mode_b_dev | Commits: 87fb47ab → 5be1f3d6 (4 commits) | Base: daemon_mode_b_main

Changes Summary

• packages/acp-bridge/src/compactionEngine.ts (+96 −25) — Dual-path text merging: subagent chunks merge by (kind, parentToolCallId) via indexed lookup; top-level chunks preserve consecutive-only merge. Adds textSlotIndex eviction on same-parent tool_call, defensive parentToolCallId meta backfill, and seed() cleanup hardening.
• packages/acp-bridge/src/compactionEngine.test.ts (+465 −6) — 12 new test cases covering parentToolCallId-aware merging, interleaving, eviction, meta backfill, 9-parallel pressure test, and seed cleanup.

Test Results

Pre-existing Errors (on daemon_mode_b_main)

• bridge.ts:22-23 — DAEMON_TRACEPARENT_META_KEY / DAEMON_TRACESTATE_META_KEY not yet exported from @qwen-code/qwen-code-core (pending core-side merge)

Verdict

✅ Ready to merge. All 39 unit tests pass (including 12 new parentToolCallId-aware tests + 9-parallel pressure test). Lint clean. Zero new type errors — the 2 typecheck/build failures are pre-existing on daemon_mode_b_main. The dual-path merging logic is well-tested and correctly preserves subagent stream attribution.

Verified by wenshao

Review round 3 — wenshao (5f88fcd)

2/3 fixed, 1 pushed back. 40/40 tests passing.

Review round 4 — wenshao (fde8474)

1/1 addressed. 40/40 tests passing. Code simplified.