refactor(internal): extract doStream helpers (Commit 2.9)

Author: brandonkachen

REFACTORING_PLAN.md

@@ -39,9 +39,9 @@ This document outlines a prioritized refactoring plan for the 51 issues identifi
 | 2.6 | Simplify use-activity-query.ts | ✅ Complete | Codebuff |
 | 2.7 | Consolidate XML parsing | ✅ Complete | Codebuff |
 | 2.8 | Consolidate analytics | ✅ Complete | Codebuff |
-| 2.9 | Refactor doStream | ⬜ Not Started | - |
-| 2.10 | DRY up OpenRouter stream handling | ⬜ Not Started | - |
-| 2.11 | Consolidate image handling | ⬜ Not Started | - |
+| 2.9 | Refactor doStream | ✅ Complete | Codebuff |
+| 2.10 | DRY up OpenRouter stream handling | ⏭️ Skipped | - |
+| 2.11 | Consolidate image handling | ✅ Not Needed | - |
 | 2.12 | Refactor suggestion-engine | ⬜ Not Started | - |
 | 2.13 | Fix browser actions + string utils | ⬜ Not Started | - |
 | 2.14 | Refactor agent-builder.ts | ⬜ Not Started | - |
@@ -780,59 +780,160 @@ common/src/analytics/
 
 ---
 
-### Commit 2.9: Refactor `doStream` in OpenAI Compatible Model
-**Files:** `packages/internal/src/ai-sdk/openai-compatible-chat-language-model.ts`  
+### Commit 2.9: Refactor `doStream` in OpenAI Compatible Model ✅ COMPLETE
+**Files:** `packages/internal/src/openai-compatible/chat/openai-compatible-chat-language-model.ts`  
 **Est. Time:** 3-4 hours  
-**Est. LOC Changed:** ~350-400
+**Actual Time:** ~2 hours  
+**Est. LOC Changed:** ~350-400  
+**Actual LOC Changed:** ~290 lines (3 new files) + ~180 lines reduced from main file
 
-| Task | Description |
-|------|-------------|
-| Extract `StreamParser` class | Parsing logic |
-| Extract `ChunkProcessor` | Chunk handling |
-| Extract `StreamErrorHandler` | Error handling |
-| Simplify `doStream` | Orchestration only |
+| Task | Description | Status |
+|------|-------------|--------|
+| Create `stream-usage-tracker.ts` | Usage accumulation with factory pattern | ✅ |
+| Create `stream-content-tracker.ts` | Text/reasoning delta handling | ✅ |
+| Create `stream-tool-call-handler.ts` | Tool call state management | ✅ |
+| Simplify `doStream` | Orchestration with extracted helpers | ✅ |
+| Multi-agent review | Codex CLI + Codebuff reviewed, fixes applied | ✅ |
+
+**New Files Created:**
+- `packages/internal/src/openai-compatible/chat/stream-usage-tracker.ts` (~60 lines):
+  - `createStreamUsageTracker()` - factory for usage accumulation
+  - `update()` - process chunk usage data
+  - `getUsage()` - get LanguageModelV2Usage
+  - `getCompletionTokensDetails()` - get detailed token breakdown
+- `packages/internal/src/openai-compatible/chat/stream-content-tracker.ts` (~45 lines):
+  - `createStreamContentTracker()` - factory for content state
+  - `processReasoningDelta()` - emit reasoning-start/delta events
+  - `processTextDelta()` - emit text-start/delta events
+  - `flush()` - emit reasoning-end/text-end events
+  - Constants: `REASONING_ID`, `TEXT_ID`
+- `packages/internal/src/openai-compatible/chat/stream-tool-call-handler.ts` (~120 lines):
+  - `createStreamToolCallHandler()` - factory for tool call state
+  - `processToolCallDelta()` - handle streaming tool call chunks
+  - `flushUnfinishedToolCalls()` - emit incomplete tool calls at end
+  - `emitToolCallCompletion()` - extracted helper for DRY completion logic
+
+**doStream Reduction:**
+- `openai-compatible-chat-language-model.ts`: ~300 → ~120 lines in doStream (-60%)
+- TransformStream now delegates to helpers instead of inline logic
+
+**Multi-Agent Review (Codex CLI, Codebuff):**
+
+| Finding | Agent | Severity | Resolution |
+|---------|-------|----------|------------|
+| **Magic string IDs** | Codebuff | Info | Added `REASONING_ID`, `TEXT_ID` constants |
+| **Unused getters** | Codebuff | Info | Removed `isReasoningActive()`, `isTextActive()`, `getToolCalls()` |
+| **Duplicated completion logic** | Codebuff | Warning | Extracted `emitToolCallCompletion()` helper |
+| **Non-null assertion** | Codex | Info | Removed unnecessary `!` assertion |
+| **Redundant nullish coalescing** | Both | Suggestion | Simplified `?? undefined` to just return value |
+| **Unused type exports** | Codebuff | Info | Made `ToolCallState` internal (not exported) |
+
+**Multi-Agent Review (All 4 CLI Agents: Codex, Codebuff, Claude Code, Gemini):**
+
+| Finding | Agents | Severity | Resolution |
+|---------|--------|----------|------------|
+| **Empty delta emission** | Codebuff, Claude | Warning | Fixed: Only emit delta if arguments truthy |
+| **Invalid JSON in flush** | Codex, Codebuff | Warning | Fixed: Use `isParsableJson` with `'{}'` fallback |
+| **Dead generateId() fallback** | Codebuff, Claude | Info | Fixed: Removed dead `?? generateId()` |
+| **Magic string IDs** | Codex, Claude, Gemini | Suggestion | Fixed: Added `REASONING_ID`, `TEXT_ID` constants |
+| **Side-effect mutation** | Codebuff, Claude, Gemini | Suggestion | Accepted: Keep for simplicity within limited scope |
+| **Hardcoded IDs** | Codex, Claude, Gemini | Suggestion | Documented: Single block assumption |
+| **No unit tests** | Codex | Warning | Deferred: Integration tests sufficient for now |
+| **Premature tool finalization** | Gemini | Critical | Rejected: Matches original behavior, intentional for providers sending complete tool calls |
+
+**Architecture Decisions Validated (All 4 agents agree):**
+- ✅ Factory pattern is correct (vs classes or standalone functions)
+- ✅ Event arrays are cleaner than passing controller (testability)
+- ✅ Helpers are ready for OpenRouter reuse in Commit 2.10
+
+**Review Fixes Applied:**
+| Fix | Description |
+|-----|-------------|
+| Add constants | `REASONING_ID = 'reasoning-0'`, `TEXT_ID = 'txt-0'` |
+| Remove unused getters | Deleted `isReasoningActive()`, `isTextActive()`, `getToolCalls()` |
+| Extract completion helper | `emitToolCallCompletion()` reduces duplication by ~30 lines |
+| Simplify usage tracker | Flattened state to simple variables instead of nested object |
+| Remove redundant code | Cleaned up `?? undefined` patterns |
+| **Empty delta fix** | Moved delta emission inside `if (arguments != null)` block |
+| **Invalid JSON fix** | Added `isParsableJson` check with `'{}'` fallback in flush |
+| **Dead fallback fix** | Removed `?? generateId()` since id is validated earlier |
+
+**Test Results:**
+- 191 internal package tests pass
+- All 13 package typechecks pass
+- Streaming behavior unchanged
 
 **Dependencies:** None  
 **Risk:** Medium - Core streaming  
-**Feature Flag:** `REFACTOR_STREAM=true`  
-**Rollback:** Revert and flag off
+**Rollback:** Revert single commit  
+**Commit:** `559857bc2`
 
 ---
 
-### Commit 2.10: DRY Up OpenRouter Stream Handling
-**Files:** `packages/internal/src/ai-sdk/openrouter-ai-sdk/chat/index.ts`  
+### Commit 2.10: DRY Up OpenRouter Stream Handling ⏭️ SKIPPED
+**Files:** `packages/internal/src/openrouter-ai-sdk/chat/index.ts`  
 **Est. Time:** 2-3 hours  
 **Est. LOC Changed:** ~300-400
 
-| Task | Description |
-|------|-------------|
-| Create shared `stream-utils.ts` | Common streaming utilities |
-| Extract shared chunk processing | Reuse across providers |
-| Update OpenRouter implementation | Use shared code |
-| Update OpenAI compatible | Use shared code |
+> **Decision:** Skipped after multi-agent review of Commit 2.9. All 4 CLI agents reviewed and Codebuff's recommendation was adopted.
+
+**Reason for Skipping:**
+OpenRouter streaming has materially different requirements from OpenAI-compatible streaming:
+- `reasoning_details` array with types (Text, Summary, Encrypted) vs simple `reasoning_content`
+- `annotations` / web search citations support
+- `openrouterUsage` with `cost`, `cost_details`, `upstreamInferenceCost`
+- Different tool call tracking (`inputStarted` flag vs `hasFinished`)
+- Provider routing info
+
+Premature abstraction would add complexity without clear benefit. The helpers are small (45-120 lines each) and the "duplication" cost is low compared to the complexity cost of a forced abstraction.
+
+**Revisit When:** We find ourselves fixing the same streaming bug in both implementations, or the APIs converge.
 
 **Dependencies:** Commit 2.9  
-**Risk:** Medium  
-**Rollback:** Revert single commit
+**Risk:** N/A - Skipped  
+**Rollback:** N/A
 
 ---
 
-### Commit 2.11: Consolidate Image Handling
+### Commit 2.11: Consolidate Image Handling ✅ NOT NEEDED
 **Files:** Clipboard/image related files in CLI  
-**Est. Time:** 2-3 hours  
-**Est. LOC Changed:** ~300-400
+**Est. Time:** 0 hours (skipped)  
+**Est. LOC Changed:** 0
+
+> **Decision:** Skipped after codebase analysis. The image handling architecture is already well-organized.
+
+**Reason for Skipping:**
+The refactoring plan's description was based on outdated analysis. The current architecture is clean:
+
+| File | Purpose | Lines |
+|------|---------|-------|
+| `common/src/constants/images.ts` | Shared constants, MIME types, size limits | ~50 |
+| `cli/src/utils/image-handler.ts` | Core processing, compression, validation | ~290 |
+| `cli/src/utils/clipboard-image.ts` | Cross-platform clipboard operations | ~370 |
+| `cli/src/utils/image-processor.ts` | SDK message content integration | ~70 |
+| `cli/src/utils/pending-attachments.ts` | State management for pending images | ~190 |
+| `cli/src/utils/image-thumbnail.ts` | Pixel extraction for thumbnails | ~75 |
+| `cli/src/utils/terminal-images.ts` | iTerm2/Kitty protocol rendering | ~190 |
+| `cli/src/utils/image-display.ts` | Terminal dimension calculations | ~60 |
+
+**Clean Dependency Chain:**
+```
+common/constants/images.ts (constants)
+    ↓
+cli/utils/image-handler.ts (core processing)
+    ↓
+├── cli/utils/clipboard-image.ts (clipboard operations)
+├── cli/utils/image-processor.ts (SDK integration)
+└── cli/utils/pending-attachments.ts (state management)
+```
 
-| Task | Description |
-|------|-------------|
-| Create `utils/image-handler.ts` | Unified image handling |
-| Extract `processImageFromClipboard()` | Clipboard images |
-| Extract `processImageFromFile()` | File images |
-| Extract `validateImage()` | Image validation |
-| Update all usages | Replace duplicates |
+No duplication found. Architecture follows single responsibility principle.
 
-**Dependencies:** None (can run in parallel with 2.10)  
-**Risk:** Low  
-**Rollback:** Revert single commit
+**Revisit When:** If new image handling code introduces duplication.
+
+**Dependencies:** N/A  
+**Risk:** N/A  
+**Rollback:** N/A
 
 ---