🤖 refactor: split turn preparation into a TurnRequestBuilder pipeline - #4014
Conversation
This comment has been minimized.
This comment has been minimized.
|
@codex review |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@codex review |
This comment has been minimized.
This comment has been minimized.
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". |
This comment has been minimized.
This comment has been minimized.
4053219 to
f4b0359
Compare
|
@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.
f4b0359 to
07657a1
Compare
07657a1 to
74bd86d
Compare
![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: 74bd86d57d
ℹ️ 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".
74bd86d to
d59ccd0
Compare
d59ccd0 to
db7b695
Compare
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.
…aitForInit pass-through The thin-router layer below relocated agent-discovery, plugin, skill, and workflow handlers out of router.ts; their aiService.waitForInit calls must follow the pass-through deletion to initStateManager.
db7b695 to
f26e69c
Compare
… pipeline The slow-start timer in streamMessage wrapped the entire prepareModelRequest pipeline (tools, policy, system context, middleware, provider options), so prepareMessagesForProviderMs overlapped the separately recorded subphase timings and misattributed their time to message conversion. Record it inside the builder around only the prepareMessagesForProvider call, preserving the pre-refactor metric semantics. --- _Generated with `xum` • Model: `anthropic:claude-fable-5` • Thinking: `xhigh` • Cost: `$383.31`_ <!-- mux-attribution: model=anthropic:claude-fable-5 thinking=xhigh costs=383.31 -->
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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.
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.
Stack
Layer 6/10 of the architecture refactor stack (net -5,101 LOC overall). This PR's diff is only this layer, against
mike/arch-thin-orpc-router.