🤖 refactor: split turn preparation into a TurnRequestBuilder pipeline - #4014
🤖 refactor: split turn preparation into a TurnRequestBuilder pipeline#4014ibetitsmike wants to merge 20 commits into
Conversation
Generated with xum • Model: openai:gpt-5.6-sol • Thinking: high • Cost: .41 <!-- mux-attribution: model=openai:gpt-5.6-sol thinking=high costs=3.41 -->
Generated with xum • Model: openai:gpt-5.6-sol • Thinking: high • Cost: .41 <!-- mux-attribution: model=openai:gpt-5.6-sol thinking=high costs=3.41 -->
Generated with xum • Model: openai:gpt-5.6-sol • Thinking: high • Cost: .41 <!-- mux-attribution: model=openai:gpt-5.6-sol thinking=high costs=3.41 -->
_Generated with `xum` • Model: `openai:gpt-5.6-sol` • Thinking: `high`_ <!-- mux-attribution: model=openai:gpt-5.6-sol thinking=high -->
--- _Generated with �0xum�0 • Model: �0openai:gpt-5.6-sol�0 • Thinking: �0high�0 • Cost: �0�40.00�0_ <!-- mux-attribution: model=openai:gpt-5.6-sol thinking=high costs=0.00 -->
The builder only copied codexOauthService/coderOauthService into ProviderModelFactory at build() time, so AIService.createModel callers (branch summaries, refinement, status generation) hit 'Codex OAuth service not initialized' before any turn ran. The factory now reads the shared bindings object at use time. Also restores the slow-startup diagnostic on the caller's error path by writing logSlowStreamStartup back into startupState, and fixes lint in the migrated tests.
AgentSession now asserts stream-lifecycle access on its engine seam, and WorkspaceService reads delegated-turn stream info from the engine. Share one createStreamLifecycleMocks() harness helper across the hand-rolled AIService mocks (file-specific overrides still win), make the harness report failures as Err results like the real implementations, and give the archive-hooks suite an engine stub for the stream exemption.
…tion
Delete the 37 single-line getters/forwarders TurnRequestBuilder grew
during the extraction (direct this.dependencies access instead), reuse
the prepared request's first-step rebuild closure rather than re-wrapping
it, share markProviderMetadataCostsIncluded from streamManager, and
collapse AgentSession.isAiStreaming to the asserted engine reference.
Pass streamManager into the CLI AgentSession constructions ('xum run'
and 'xum workflow' would fail the new lifecycle assert without it, since
AIService no longer carries getStreamInfo/replayStream). Unexport ten
module-private types, fix assert messages and comments still naming
AIService as the request builder, and drop narrative comments.
This comment has been minimized.
This comment has been minimized.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 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". |
This comment has been minimized.
This comment has been minimized.
|
@codex review |
|
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". |
This comment has been minimized.
This comment has been minimized.
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs 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". |
|
Security review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
1 participant
Summary
Splits turn preparation from turn execution. All request preparation that lived inside
AIService.streamMessage(a 2,925-line method) moves into a newTurnRequestBuilderwhose pipeline produces a self-containedTurnExecutionOptions;StreamManager(the turn engine) now owns the execution lifecycle end to end, including pre-start abort tracking and the mock stream lifecycle. AIService's pass-through methods and 12 DI setters are deleted.Net LOC vs merge-base: production -285 (+3,391 / -3,676), tests -1,210 (+859 / -2,069), total -1,495.
Background
aiService.tswas 4,370 lines with one god-method interleaving runtime init, model resolution, memory context, prompt assembly, tool policy/wrapping, history placeholder append, a ~543-line refusal-fallback closure that re-implemented per-model preparation, and thinking-override closures. Around it sat verbatim pass-throughs to StreamManager and 12 DI setters feeding closure state. Changing tool wrapping meant navigating ~3,000 lines of mutable closures, andagentSession.testHarness.tsfaked the whole class viaas unknown as AIService. Builds on #3999 (typedTurnExecutionOptionsseam).Implementation
turnRequestBuilder.ts(new): preparation as one pipeline (context -> tools -> provider request -> fallback plan). The primary request and the refusal-fallbackprepare()now share one per-model preparation path (prepareModelSeed+prepareModelRequest); the old fallback closure was near-duplicated preparation and is gone.StreamManagerabsorbspendingStreamStarts(pre-start aborts) and the mock stream lifecycle, so a turn is stoppable/queryable through the engine for its entire life. AIService'sgetStreamInfo/getStreamState/replayStream/waitForInit/debug pass-throughs are deleted and callers migrated;stopStream/isStreamingremain as 1-line delegations because ~45 taskService/workspaceService callsites still consume them throughAIService(migrating those is mechanical follow-up churn this PR avoids).TurnRequestBuilderBindingsobject populated at wiring time.ProviderModelFactoryreads OAuth services from the live bindings at use time, preserving the old eager-setter semantics forcreateModelcallers outside a turn (branch summaries, refinement).AgentSessiondepends onAgentSessionAIService+ an asserted stream-lifecycle seam instead of theAIServiceclass; the CLI entrypoints (xum run,xum workflow) now passstreamManagerexplicitly. The test harness implements the narrow interface honestly (Errresults, sharedcreateStreamLifecycleMocks()), and prep-focused tests moved to directTurnRequestBuildertests with far less mocking.buildPlanInstructions/buildStreamSystemContext) is untouched and called as an opaque stage, so a future context assembler can slot in as the builder's first stage.Irreducible additions, per item:
turnRequestBuilder.ts(+2,899) is the moved pipeline offset by aiService.ts (-3,741);streamManager.ts(+~125) is the absorbed pre-start/mock lifecycle ownership;turnRequestBuilder.test.ts(+417) replaces ~2,000 lines of heavier aiService tests; thecreateStreamLifecycleMocks()spreads across agentSession/workspaceService tests satisfy the new constructor invariant that a session always has stream-lifecycle access.Validation
/<model>, usage/costs). One finding: an empty assistant placeholder row persists inchat.jsonlwhen an interrupt lands after stream registration but before the first delta. Verified pre-existing on main: the unchanged cleanup comment documents that deletion only covers aborts before registration, and main already filters such rows from provider requests and hides them in the UI. Left as a follow-up candidate rather than a behavior change here.workspaceService.test.tshas one failure that reproduces identically on pristine main (bash monitor wake-store reconciliation);tests/ui/config/modelOneshot.test.tsfails on this host on pristine main too (happy-dom/Lottie environment quirk). Everything else green locally includingmake static-check.Risks
Highest-risk areas are the moved ordering invariants: pre-start abort registration before any await, providers-config snapshot pinning before thinking/tools identity resolution, placeholder append before
startStreamwith delete-on-abort, and the shared mutable refs (advisorTranscriptRef,toolSearchRuntime.state,activeTurnThinkingOverride). These are exercised by the migrated unit suites and the UAT interrupt/override scenarios. The OAuth live-binding read and the CLIstreamManagerwiring are the two spots where behavior intentionally diverged from the intermediate refactor state (both were regressions caught before this PR).Generated with
xum• Model:anthropic:claude-fable-5• Thinking:xhigh• Cost:$85.95Xum acted on Mike's behalf.