🤖 refactor: collapse the AIService/StreamManager turn relay into one turn engine - #3999
Conversation
|
@codex review |
This comment has been minimized.
This comment has been minimized.
![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: 95ac2b6ab1
ℹ️ 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".
|
@codex review |
This comment has been minimized.
This comment has been minimized.
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: faa20a726b
ℹ️ 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".
|
@codex review |
This comment has been minimized.
This comment has been minimized.
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
_Generated with `xum` • Model: `openai:gpt-5.6-sol` • Thinking: `high`_ <!-- mux-attribution: model=openai:gpt-5.6-sol thinking=high -->
Review follow-up: the four per-command handlers are only reachable through processSlashCommand now, and /plan open lost its dedicated tests in the result-based rewrite. _Generated with `xum` • Model: `anthropic:claude-fable-5` • Thinking: `xhigh`_ <!-- mux-attribution: model=anthropic:claude-fable-5 thinking=xhigh -->
Codex P1: getDraft captured at command invocation reports that render's input, so async commands cleared newer drafts on consume and never fired restore-if-empty. _Generated with `xum` • Model: `anthropic:claude-fable-5` • Thinking: `xhigh`_ <!-- mux-attribution: model=anthropic:claude-fable-5 thinking=xhigh -->
Codex P2: text equality cannot distinguish a retyped identical draft from the original invocation. Commands already clear through their own clear-input actions (matching trunk), so the terminal clear was additive and could only destroy mid-phase drafts. _Generated with `xum` • Model: `anthropic:claude-fable-5` • Thinking: `xhigh`_ <!-- mux-attribution: model=anthropic:claude-fable-5 thinking=xhigh -->
Codex P2 follow-up: with the terminal consume-path clear gone, /dream and /refine left the executed command re-runnable in the composer. Emit clear-input from the handlers so commands own their composer effects. _Generated with `xum` • Model: `anthropic:claude-fable-5` • Thinking: `xhigh`_ <!-- mux-attribution: model=anthropic:claude-fable-5 thinking=xhigh -->
…itative in background wrappers
…te; drop dead code
The seam file is now the single documented home for ArchiveWorkspaceOptions, SendMessageInternalOptions, and WorkspaceLiveActivity instead of duplicating workspaceService declarations. StreamErrorRecoveryOutcome comes from its canonical agentSession export. The AgentTaskStatus re-export shim is gone; importers use the seam. Method-level optional chaining on both typed ports is removed: the interfaces guarantee the methods, so the narrow-mock hedges and their comments no longer apply.
… suite The typed AgentTaskIntegration port makes several tests unwritable or redundant: the phantom cleanupReportedDescendantsAfterArchive guard (method never existed in production), the archive lock pass-through wiring assertion, and four private updateAgentStatus non-invocation spies whose positive registerSession behavior tests remain. Near-identical send/resume lifecycle, winding-down, auto-resume, and foreground-wait-backgrounding siblings collapse into table-driven tests preserving every case, and dead fake stubs the code under test never reads are dropped.
03cdbca to
3eccf33
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3eccf33214
ℹ️ 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".
Codex round: handleStreamWithHistoryFailure could persist retry state or error rows after dispose() when a startup await (commitPartial, history reads) settled with a failure post-teardown. Guard it like consumeTurnCompletion, settling collected pre-start recovery decisions in memory so waiters cannot hang. Red-green verified via the new dispose-race regression test.
|
@codex review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef4d5116a1
ℹ️ 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".
Codex round: TurnEngineEventSink may return a promise, but emitTurnEvent void'd it, so a rejecting async sink became an unhandled rejection and a slow terminal sink could outlast the settled completion unobserved. Contain rejections with a logged catch; also catch the two abort-side mirrors (abortDelivery.finally re-propagated rejections after settling, and stopStream's no-stream emit was raw void). Red-green verified via a new unhandled-rejection regression test.
|
@codex review |
This comment has been minimized.
This comment has been minimized.
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Merges AIService and StreamManager into one turn-execution module with a single authoritative per-turn completion signal. The 13-event re-emission relay between them is deleted, the 32-positional-argument
startStreamseam becomes one typedTurnExecutionOptionsobject, and AgentSession's dual-delivery dedupe latches (activeStreamErrorEventReceived,activeStreamFailureHandled) are removed because the two failure-delivery paths are now mutually exclusive by construction. Strictly behavior-preserving: no renderer, IPC, or persisted-shape changes.Background
AIService was a shallow relay: it re-emitted 13 StreamManager events (10 verbatim, 3 with enrichment) and forwarded stream-control calls one-for-one, while turn failures were delivered twice, once via the
errorevent and once via the returnedResult. AgentSession kept two boolean latches purely to dedupe those arrival paths. StreamManager was instantiated only inside AIService and had zero external consumers, so the seam carried no isolation value.Implementation
streamManager.tsbecomes the module-private engine core of AIService: it no longer extendsEventEmitter; events flow through a typedTurnEngineEventSinkintoAIService.emitEngineEvent, the single emission point, where the three existing enrichments (DevTools pending-run cleanup, debug LLM snapshot capture, partial commit/delete on abort) run unchanged.startStreamtakes one typedTurnExecutionOptionsobject and returns aTurnStreamHandlewhosecompletionpromise settles exactly once (completed, aborted, or failed with the classified error) after terminal stream cleanup. Request construction shares aStreamRequestOptionsbase between the primary turn and model-fallback hops; no internal positional cascades remain.streamMessagekeeps its fast-return semantics and returns the handle.Err(handled inline, including the runtime startup special case), post-start outcomes arrive only throughhandle.completion. Both dedupe latches are deleted. Failures that emit anerrorevent before startup resolves (runtime readiness, strict agent resolution) are collected per call viaonPreStartError(no mutable session-level capture window) and drained exactly once, so their recovery decisions always resolve.TurnStreamHandlethat settles from the scripted terminal events (scriptedstream-error→ failed,stream-end→ completed, stop/cleanup → aborted), sostreamMessagecallers see one contract everywhere; a disposed session drops late failed completions instead of persisting retry/goal state post-teardown (review-round fixes).Net LOC
Architecture PRs must be net simplifications. Final diff: +1,264 / −1,213 (net +51) across 26 files, down from +799 at first draft and +309 at review round 1. Deleted outright: the EventEmitter relay and
setupStreamEventForwarding, both AgentSession latches, the session-level pre-start capture window, the facade completion observer, the deadstreamTokenhandle property,TurnCompletion.messageId, the latch-era dual-delivery test, a tautological reflection test, the pure-forwarding relay test, the real-API concurrency test (duplicated mock-covered behavior at API cost), theOk(undefined)mock-normalization shim, EventEmitter-era noop listeners, redundant explicit default stream mocks, duplicated per-suite turn-handle helpers (8 suites), and every positional cascade (32-argstartStream, 22-argbuildStreamRequestConfig, five positional.applytest fixtures).Each material surviving addition, one line each:
streamManager.ts+48: the seam contract itself (TurnEngineEventunion,TurnCompletion/TurnStreamHandle, the exactly-once completion controller,StreamRequestOptions/TurnExecutionOptions), replacing implicit EventEmitter event strings and the 32-argument call boundary.agentSession.disposeRace.test.ts+37: review-round-2 regression test (failed completions delivered after disposal are dropped), verified red without the production guard.agentSession.ts+28: single-path completion consumption plus per-callonPreStartErrorcollection, the review-round-1 fix for handle-less failures leaving recovery decisions unresolved.streamManager.testHarness.ts+24: typed sink observer, required because StreamManager no longer exposes.on; consumed by two suites.agentSession.postCompactionRetry.test.ts+20: mocks must construct real failed/started turn handles under the new contract where they previously returnedundefined.mockAiStreamPlayer.ts+18: review-round-2 fix, pending handles settled exactly once from scripted terminal events.agentSession.testHarness.ts+18: sharedcreateStartedTurnHandle/createFailedTurnHandlefactories that deleted eight per-suite copies.streamManager.test.ts−79,aiService.test.ts−28,preStreamError−25,autoCompaction−24), so tests overall are net −40.None of the +51 is deletable at zero risk. Levers evaluated and rejected with evidence: settling completions at their set-points (reintroduces a mid-cleanup race; settlement is deliberately ordered after
workspaceStreams.delete), collapsing the pre-start error event +Errdual delivery into one path (the event has three non-AgentSession consumers: renderer forwarding via serviceContainer, workspaceService, taskService), and inheritance-merging the two classes (~60 lines at the cost of colliding private state in a UAT-validated core). Future work if further shrink is wanted: LOC-neutral consolidations only (test assertion helpers), which do not reduce the net.Validation
workspaceServicewake-store reconciliation) fails identically at the merge-base on this host and passes in CI, so it is pre-existing and unrelated.bun test srcunit suite andmake static-checkgreen on the final head.Ok(undefined)wherestreamMessagenow returns a handle; fixed in the mocks, no production guards added. Integration (compaction) and E2E (linux) streaming-error suites reproduce green locally under xvfb at the final head, and again with the branch merged into latestmain; the remaining localreview scenariofailure reproduces at the merge-base (host-environmental, passes in CI on main).c4d3453a8: streaming with reasoning and tool calls, hard and mid-tool interrupts, provider errors surfacing exactly once with auto-retry, live and persisted token/cost accounting, crash-kill recovery mid-text and mid-tool, and reconnect replay without duplication. Production changes since that SHA are the two review-round fixes (mock playback settlement, mock-mode only; disposal guard), the per-call pre-start collection,TurnCompletion.messageIdremoval, and request-options type restructuring, each covered by the red-green suites and the E2E/integration runs above rather than a full re-UAT.Risks
This is the streaming core, so the main regression surfaces are event ordering, interrupt/resume semantics, crash recovery of partials, and cost accounting. Mitigations: the enrichment and cleanup code was moved verbatim to the single emission point, the abort path still emits asynchronously and always forwards even when partial cleanup fails, and completion settles only after terminal cleanup deregisters the stream. The existing test suites for all three files pin these invariants and passed unmodified in intent (whitebox seams were adapted to the new signature).
Generated with
xum• Model:anthropic:claude-fable-5• Thinking:xhigh• Cost:$259.68