Conversation
auriti
left a comment
There was a problem hiding this comment.
Code Review
The core jsonBuffer approach is correct — accumulating streamed deltas and comparing against the canonical final payload in output_item.done is the right pattern. The startTextBlockIfNeeded / closeActiveTextBlock generator refactoring is clean. No conflicts with #237.
Several issues need addressing before merge:
Critical: flush logic ambiguity
The response.output_item.done handler compares finalArgs.length > toolBlock.jsonBuffer.length to decide whether to emit a remainder delta. This silently does nothing when:
finalArgs.length === toolBlock.jsonBuffer.length(assumes buffer is exact prefix — OK in ideal case)finalArgs.length < toolBlock.jsonBuffer.length(malformed stream delivered more deltas than the final payload) — silently proceeds with corrupt buffered JSON
Suggested fix:
```ts
if (finalArgs && finalArgs !== toolBlock.jsonBuffer) {
const remainder = finalArgs.slice(toolBlock.jsonBuffer.length)
if (remainder.length > 0) {
yield { type: 'content_block_delta', index: toolBlock.index,
delta: { type: 'input_json_delta', partial_json: remainder } }
}
toolBlock.jsonBuffer = finalArgs
}
```
Important: toolBlocksByItemId.delete key mismatch
```ts
toolBlocksByItemId.delete(String(item.id))
```
If item.id is undefined in the done event but the block was keyed with the fallback toolUseId, String(undefined) === 'undefined' won't match the original key. The entry leaks and the cleanup loop emits a spurious content_block_stop. Fix: mirror the exact key used during insertion:
```ts
const key = String(item.id ?? toolUseId)
toolBlocksByItemId.delete(key)
```
Important: test doesn't cover the actual flush path
The new test verifies input_json_delta events appear, but doesn't simulate the specific scenario: partial JSON via function_call_arguments.delta followed by a longer item.arguments in output_item.done that requires flushing the remainder. Without this fixture, the regression path is untested.
Minor
- CondensedLogo tagline
'Codex-ready terminal for any LLM'narrows the product positioning — worth a team discussion ⎿→│removes the end-cap visual on message boundaries — subjective but affects every response
What works well
jsonBufferaccumulation pattern is architecturally correctstartTextBlockIfNeeded/closeActiveTextBlockrefactoring is clean- Full SSE sequence test is a solid regression baseline
- No conflicts with #237
Fix the flush logic and the key mismatch, and this is ready.
|
@smittyPNW can you check that issues ? |
Summary
Testing