Repository navigation
feat: keep injected CLI turns in one opt-in stream - #217
krislavten wants to merge 1 commit into
Conversation
Track injected UUID receipts across native result boundaries, bound continuation, and expose stream lifecycle callbacks for caller-controlled recovery. Co-Authored-By: Codex <noreply@openai.com>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b84c90c577
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| notifyStreamEnd(); | ||
| done(); |
There was a problem hiding this comment.
With continuation disabled, streaming input enabled, and onStreamStart plus an active onPromptSuggestion callback configured, deferring done() until this finally keeps the injector open throughout the post-result suggestion drain, potentially for 10 seconds after the AI SDK stream finishes. A late injection is now accepted and written to the native query while the drain discards continuation output. Replaying the parent and target implementations confirmed that the parent reports delivery as false, whereas this commit sends a second input message and reports true. End streaming input at the terminal result, after the synchronous lifecycle notification but before awaiting the drain.
Useful? React with 👍 / 👎.
| ((message.type === 'system' && message.subtype === 'init') || | ||
| (message.type === 'stream_event' && message.event.type === 'message_start')) | ||
| ) { | ||
| this.accepting = true; |
There was a problem hiding this comment.
After an intermediate result, accepting is set to false, but these conditions restore it only for another init or a partial message_start. When a continuation contains complete assistant messages without another init, as supported by includePartialMessages: false, admission remains closed throughout that turn. Further MessageInjector.inject() calls therefore immediately receive onResult(false) even while a tracked continuation is running. Reopen admission for main-thread assistant or command-start activity as well, or keep it open until the actual terminal boundary.
Useful? React with 👍 / 👎.
| if (lastResult) yield { ...lastResult, usage: { ...lastResult.usage, ...totals } }; | ||
| return; |
There was a problem hiding this comment.
In the supported recovery path where the initial command's completion starts the deadline but its result never arrives, lastResult is undefined at expiry. This branch then returns normally, and doStream closes with neither an error nor a finish containing timeout metadata. A runtime probe produced only stream-start; callers without the optional receipt or lifecycle callbacks cannot observe the timeout through the stream API despite messages remaining unconfirmed. Emit a timeout error on this path without synthesizing usage, analogous to the pending-message EOF handling below.
Useful? React with 👍 / 👎.
| : await Promise.race([ | ||
| iterator.next(), | ||
| new Promise<typeof timeout>((resolve) => { | ||
| timer = setTimeout(() => resolve(timeout), Math.max(0, deadline! - Date.now())); |
There was a problem hiding this comment.
If the native iterator has buffered frames, iterator.next() can resolve through microtasks on every iteration. Even after the absolute deadline has passed, it beats the newly scheduled setTimeout(0), which is then cleared in finally. Activity can consequently postpone expiry and allow a late result to finish with continuationTimedOut: false; a dependency-free probe with a 5 ms budget and 30 ms of buffered frames reproduced this. Check the wall-clock deadline before consuming another frame rather than relying solely on a newly created timer race.
Useful? React with 👍 / 👎.
| const responseMessages = | ||
| this.settings.continueInjectedTurns && this.settings.onStreamStart | ||
| ? injectionSession.messages(response, () => response.close()) |
There was a problem hiding this comment.
When a non-final native result contains permission_denials that the final result does not repeat, this wrapper hides that result from the main loop's mergeResultPermissionDenials() call. As that helper and its existing tests document, PreToolUse-hook denials can have no corresponding permission_denied system event, so this removes their only source and silently omits denied tool calls from the final provider metadata. A two-result runtime probe reproduced the omission. Merge each native result's denial metadata before filtering it, or aggregate and deduplicate these lists across intermediate results.
Useful? React with 👍 / 👎.
Summary
Add an opt-in
continueInjectedTurnsstreaming mode for callers that submit messages throughonStreamStartwhile a CLI turn is running. The provider keeps reading intermediate native results until the tracked injected UUIDs are confirmed, emitting one AI SDK finish with the sum of per-result token usage.This targets the maintained
ai-sdk-v6branch. Default streaming behavior is unchanged.onStreamEnddetaches a failed channel synchronously before queued delivery callbacks are rejected, allowing a caller to reinject unconfirmed messages into an explicit recovery stream. Recovery duplicate-completion receipts require both a frozen prior main-thread consumption witness for the exact resume session and a successful result for the new initial prompt; a completion notification alone never confirms consumption.Evidence
lengthfinish.lengthfinish before the guard was added, then passed.npx vitest run --project=node src/injection-session.test.ts src/injected-stream-usage.test.ts src/claude-code-language-model.test.ts src/validation.test.ts --maxWorkers=1: 363 passed.--project=edge: 363 passed.npm run typecheck,npm run build: passed.query()against a scripted Anthropic HTTP peer for final-answer injection, hidden CLI continuation, recovery reinjection without repeating a tool side effect, and connection closure at timeout. These are local tests, not a live-provider release claim.Merge Danger
Door: two-way. All continuation behavior requires explicit opt-in and the tracked injection callback; removing the opt-in restores the existing result boundary.
Blast Radius: streaming. The native SDK UUID and result attribution semantics remain the confirmation boundary.
resumedInjectionUuidsis an advanced caller-supplied recovery witness set, not an inference from a queued or completed notification. Timeouts close the query and report pending IDs; callers must decide their application-level dropped state. Existing cumulative cost/model usage metadata is retained, while per-result token usage is summed.