refactor(daemon): dissolve the startServer god-function (slice 4) by leonaburime-ucla · Pull Request #5152 · nexu-io/open-design

leonaburime-ucla · 2026-07-04T06:48:43Z

Stacked PR. This is slice 4 of the server.ts decomposition, stacked on #5147 (→ #5132 → #5128). Against main it shows all of slice 1–4's commits; only the top commit f1aabe9e belongs to this PR — review that one. Rebase to drop the lower commits once #5128/#5132/#5147 merge.

Why

startServer() was the single worst function in the codebase (cyclomatic 493 / cognitive 1065): the composition root and the chat-run engine and the HTTP wiring, all inlined. Slice 1 pulled out the chat-run engine; this slice finishes the job by extracting the remaining inlined business logic so the composition root is only composition. This is the last high-leverage step of the strangler-fig plan in ARCHITECTURE-REVIEW.md.

What users will see

Nothing — this is a pure internal refactor. The public HTTP surface, every route, and all runtime behavior are unchanged. QA regression surface: agent chat runs (system-prompt composition, critique, plugin pipeline/atom blocks, telemetry), the /api/plugins/* install/upgrade/apply/trust/stats/export/share routes, and /api bearer-auth + CORS.

What this does

server.ts: 4,474 → 3,399 lines (9,025 in the original god-file). Moved bodies are byte-identical to the original modulo documented deviations (post-listen live getters for resolvedPort/daemonUrl; dynamic-import depth after files change directory). Public export surface unchanged.

Extractions (explicit-deps factories, mirroring createStartChatRun):

runtimes/compose-daemon-system-prompt.ts — per-run system-prompt composition
runtimes/fire-pipeline-for-run.ts — plugin pipeline firing
http/api-security-middleware.ts — bearer-token auth + CORS origin guard
plugin-registry-view.ts — plugin registry read-model
run-telemetry-fallback.ts — terminal telemetry fallback

Route-handler relocations (behavior-neutral — delete in server.ts + add in routes/plugins/ are visible together):

installOrUpgradePlugin → routes/plugins/index.ts
9 project/share/candidate/trust/stats/export handlers → routes/plugins/project-handlers.ts

Cleanup: remove 140 dead imports server.ts no longer references (export surface identical; 0 ReferenceError across the daemon boot suites).

Surface area

None — internal refactor, docs, tests, or translation update only

Daemon (apps/daemon) internal module structure only. No contracts, no web, no CLI, no extension points, no deps, no env vars, no i18n.

Validation

pnpm --filter @open-design/daemon typecheck → 0 errors (routes/plugins is strict TS and type-checks the relocated handlers)
pnpm guard → 55 pass / 0 fail
Targeted daemon suites green: system-prompt/critique/telemetry composition, plugin pipeline-runner + headless-run, plugins install/upgrade/apply/trust/stats/candidates, api-token-guard + server-cors, and real-server boot (chat-route, daemon-startup, server-bootstrap-regression)
Export-surface diff identical before/after the import sweep; 0 ReferenceError/is not defined across a boot batch spanning every removed-import domain

…t shapes Pins the SSE vocabulary a chat run emits (start, agent text_delta frames, error payload nesting, terminal end event, monotonic ids) against fake agent binaries, with OD_DATA_DIR and OD_AGENT_HOME isolation so detection never reaches host binaries. Baseline net for extracting startChatRun out of server.ts.

…s factory Moves the 3,416-line chat-run closure out of startServer behind an explicit-deps createStartChatRun(deps) factory. The body is byte-identical except four daemonUrl references become deps.daemonUrl, passed as a live getter because server.ts reassigns daemonUrl after listen. 133 captured identifiers become direct imports; ~39 server.ts module-scope helpers, registries, and startServer locals flow through deps. server.ts drops from 9,543 to 6,172 lines with an identical export surface. Verified by the golden SSE characterization suite (4/4), pnpm guard, and a byte-identity diff against the pre-move closure.

@ts-nocheck

…se-shapes test typecheck The startChatRun extraction moved the closure from server.ts (src root) into runtimes/ but left several import specifiers pointing at the old relative base, none of which tsc could catch because the moved module is @ts-nocheck: - dynamic import('./codex-config-normalize.js') -> '../codex-config-normalize.js' (module lives at src root; crashed every real Codex run at spawn) - dynamic import('./runtimes/defs/antigravity.js') -> './defs/antigravity.js' at two sites (model-lock acquire + waitForAgyToReadModel); the doubled 'runtimes/' segment broke Antigravity concrete-model runs - dynamic import('./memory-llm.js') -> '../memory-llm.js'; swallowed by .catch, so LLM memory extraction + annotation distillation silently stopped - removed a duplicate 'resolveProjectDir' named binding in the projects.js import (unique-binding violation) Also fix the slice-1 characterization test under the tests tsconfig: replace .findLast() (ES2023, not in the tests lib) with an ES2022-safe reverse .find() (also resolves the implicit-any param), and assert the indexed ids so noUncheckedIndexedAccess is satisfied.

Address the reviewer's robustness findings on the startChatRun characterization suite so it actually guards the moved SSE contract: - readSseUntil now waits for a COMPLETE frame: it breaks only once the marker has appeared AND a blank-line terminator (\n\n) follows it. Breaking on the bare marker raced SSE chunk boundaries (the event: line can arrive one chunk before its data: line), which could hand parseSseFrames a half-written frame and make the suite flake red even when the daemon is correct. - opencode error test: read through the terminal end frame in one pass and assert the actual SSE shapes (error.code === 'AGENT_EXECUTION_FAILED' and end.status === 'failed') instead of discarding the stream and trusting only the /api/runs record. - missing-binary test: parse the full /api/chat SSE body and assert the real error frame (error.code === 'AGENT_UNAVAILABLE') and terminal end.status === 'failed', instead of a raw substring grep that would pass even if the code landed in the wrong frame or the terminal end was dropped. Verified: suite passes 4/4 across 3 consecutive runs; the daemon does emit a terminal end{status:failed} frame on both negative paths.

…y.ts Strangler-fig slice 2 of the server.ts decomposition (follows the startChatRun extraction in PR nexu-io#5128). Moves the in-memory media-task registry — the `mediaTasks` Map plus hydrate/create/persist/append/ notify/snapshot helpers and the TTL constant — out of server.ts into a new `media/task-registry.ts` sibling module, layered over the existing SQLite persistence in `media/tasks.ts`. Byte-identical move: function bodies are unchanged; server.ts imports the helpers back and wires them into startServer's boot rehydration and the media route deps object exactly as before. The four db helpers the moved code needs (get/insert/update/deleteMediaTask) now import into the new module and leave server.ts's import block; the three still used by startServer (listMediaTasksByProject, listRecentMediaTasks, reconcileMediaTasksOnBoot) stay. server.ts 6172 -> 6062 lines. Public/runtime behavior unchanged. Validation: tsc -p apps/daemon/tsconfig.json --noEmit clean; pnpm guard clean; chat-run-sse-shapes characterization 4/4; media tasks-persistence + tasks-routes suites green (14/14); policy-routes' 7 failures proven pre-existing (identical on base d20acc8).

Strangler-fig slice 2, cluster 2. Moves the Server-Sent Events transport helper — createSseResponse and its default heartbeat cadence SSE_KEEPALIVE_INTERVAL_MS — out of server.ts into http/sse.ts, alongside the other http/ transport helpers (response.ts, api-errors.ts). Byte-identical move. Both names were exported from server.ts, so server.ts now imports createSseResponse back for its route deps objects and re-exports both names — the public export surface is unchanged. Scope is the SSE transport only; createSseErrorPayload (an ApiError-shaped payload builder that depends on createCompatApiError and belongs with http/api-errors) is intentionally left in place for a later api-errors move. Validation: tsc -p apps/daemon/tsconfig.json --noEmit clean; pnpm guard clean; chat-run-sse-shapes characterization 4/4.

Strangler-fig slice 2, cluster 3. Moves the buffered-subprocess and POSIX login-shell command helpers — execFileBuffered, quotePosixShellArg, buildGhShellCommand, buildCommandShellCommand, buildLoginShellCommand, execGhBuffered, execCommandViaLoginShell — out of server.ts into a new shell/commands.ts module. ensureGhReady moves with them (it is its only consumer of execGhBuffered). Byte-identical move. All eight were module-private in server.ts (nothing external imported them), so no re-export is needed; server.ts imports execCommandViaLoginShell back for its route deps object and the inline plugin-share route. The execFile import stays — other server.ts code still uses it directly. Two adjacent findings, deliberately NOT changed here (follow-ups): - ensureGhReady has no callers repo-wide (cli.ts does its own inline gh-readiness checks) — dead code, flagged for a removal PR. - cli.ts keeps its own independent copies of these exec helpers; deduping both onto shell/commands.ts is a separate refactor. Also dropped a stale orphaned JSDoc block (code/message/init -> ApiError) that sat above execFileBuffered but described neither it nor its neighbour; removing it during the move avoids re-attaching it to readProjectPluginManifest. server.ts 6004 -> 5926 lines. Public/runtime behavior unchanged. Validation: tsc -p apps/daemon/tsconfig.json --noEmit clean; pnpm guard clean; chat-run-sse-shapes characterization 4/4; no daemon test references the moved symbols.

…m server.ts server.ts carried its own copies of renderOAuthResultPage + escapeHtml, but they have zero callers: the live, used copies live in mcp-routes.ts (which defines and calls its own renderOAuthResultPage/escapeHtml). The server.ts copies were orphaned when the MCP OAuth callback routes moved to mcp-routes.ts. Both were module-private (never exported), so this is a pure dead-code deletion with no public-surface or behavior change. Verified: zero remaining references to either symbol in server.ts; daemon still boots and the chat-run SSE characterization suite passes 4/4; typecheck + guard clean. server.ts drops ~89 lines.

…-context.ts Byte-identical strangler-fig move of the run-context selection + prompt-block cluster out of server.ts: WORKSPACE_CONTEXT_KINDS, normalizeWorkspaceContextItems, normalizeRunContextSelection, mergeRunContextSelections, projectMetadataContextSelection, formatContextRefList, formatWorkspaceContextList, renderWorkspaceContextToolHints, renderRunContextPrompt. These are pure helpers (they read only their arguments — no daemon module state), so the move is mechanical. server.ts imports back the two externally-used exports (normalizeRunContextSelection for routine context; renderRunContextPrompt for the chat-run deps object); the other seven are now private to the new module. start-chat-run.ts is unchanged (it still receives renderRunContextPrompt through its deps). prompts/ is a flat, directly-imported concern dir (not a barrel domain), matching server.ts's existing './prompts/system.js' import. No behavior change; public surface unchanged. Verified: chat-run SSE characterization suite 4/4 (real server boots), full daemon typecheck (src+tests) clean, guard 55/0. server.ts drops ~233 lines.

@ts-nocheck

…-analytics.ts Strangler-fig slice 3: move the pure run-event analytics/retry scanners out of server.ts (god file) into a new sibling module, byte-identical bodies. Moved: resolveRunProjectKindForAnalytics, scanRunEventsForFinishedProps, scanRunEventsForRetrySideEffects, fileNameFromToolInputPath, filesystemWriteFileNamesFromRunEvents, filesystemEmptyAnswerFallbackText, retryFinalResultForRunStatus, runRetryEventsForAnalytics, plus their seven __forTest* wrappers. All are pure (read only their args + imported helpers, no daemon module state). server.ts imports back the five functions it references and re-exports the seven __forTest* wrappers so its public surface (the daemon test suite imports them from server.ts) is unchanged. server.ts -147 net lines (5603 -> 5434). Validation: tsc (src+tests) clean; pnpm guard 55/0; chat-run-sse-shapes char suite + run-lifecycle-analytics + tool-loop-persistence 29/29 green; orphan-ref and duplicate-binding checks clean (server.ts is @ts-nocheck so tsc can't see moved-symbol breakage).

…atus Strangler-fig slice 3: two small byte-identical preamble extractions. - http/error-payloads.ts: createSseErrorPayload, rewriteKnownAgentStreamError, createAmrModelUnavailablePayload (pure; built on the typed createCompatApiError from http/api-errors). server.ts imports all three back for the chat-run deps. - project-display-status.ts: normalizeProjectDisplayStatus, composeProjectDisplayStatus (pure). server.ts imports both back and re-exports them to preserve its public surface. No behavior change. Orphan/duplicate-binding checks clean.

… plugin-share.ts Strangler-fig slice 3: move readProjectPluginManifest, githubRepoNameFromPluginName, PLUGIN_SHARE_ACTION_LABELS, USER_PLUGIN_SOURCE_KINDS, the plugin-context SKIP sets, normalizePluginShareAction, renderPluginSharePrompt, copyPluginFolderForProjectContext, copyPluginContextDir, shouldSkipPluginContextEntry out of server.ts into a new sibling module, byte-identical. All are pure or filesystem-only (no daemon module state). server.ts imports back the six symbols it references and re-exports __forTestReadProjectPluginManifest to preserve its public surface. normalizePluginShareAction's PLUGIN_SHARE_ACTION_PLUGIN_IDS dep is a @open-design/contracts package import (no cycle). No behavior change. Orphan/duplicate-binding checks clean.

…serving helpers Strangler-fig slice 3: move the daemon's request/response-edge security and serving helpers out of server.ts into a new sibling module, byte-identical. Four related sub-groups: - Local-daemon request validation (loopback peer/host/origin) + the requireLocalDaemonRequest middleware. - Project preview-scope registry (mint/validate short-lived preview scopes) + preview asset-path parsing. - Tool-request authorization (bearer token -> toolTokenRegistry grant) + project/run override checks. - Live-artifact route serving (archive filename sanitize, route-error mapping, preview/code response headers). All deps are imports (net, randomUUID, toolTokenRegistry, sendApiError, the live-artifact error classes) -- no daemon module state, no cycle. server.ts imports back the 13 symbols it references; the 6 internal-only helpers stay module-private. The four sub-groups are a candidate for a later split. No behavior change. Orphan/duplicate-binding checks clean.

…om server.ts Strangler-fig slice 3: split the finalized-run telemetry and turn-2 form-answer/chat-request composition helpers out of server.ts into two cohesive sibling modules, byte-identical (sliced from source to preserve the — / CJK escape sequences and em-dash comments exactly). - run-telemetry.ts: shouldReportRunCompletedFromMessage, telemetryPromptFromRunRequest, createFinalizedMessageTelemetryReporter (dependency-injected design/db/dataDir/ reportedRuns), shouldReportRunCompletionTelemetryFallbackStatus, and the TERMINAL_RUN_STATUSES set (moved here as its sole consumer; private). - chat-request-composition.ts: FORM_ANSWERS_HEADER_RE, FORM_ANSWERED_SYSTEM_OVERRIDE, FORM_ANSWERED_GENERIC_OVERRIDE, formAnswerTransitionForCurrentPrompt, composeChatUserRequestForAgent (all pure). server.ts imports back the symbols it references (incl. FORM_ANSWERS_HEADER_RE for the chat-run deps object) and re-exports the public surface. Deps are all imports (langfuse-bridge, run-result, contracts/analytics) -- no daemon module state. No behavior change; chat-route form-answer wording asserted green. Orphan/duplicate checks clean.

Strangler-fig slice 3: move the assistant-message run-lifecycle persistence, skill-plugin-candidate detection, and agent-event mapping helpers out of server.ts into a new sibling module, byte-identical (sliced from source to preserve raw Unicode: the × in the tool-loop message, em-dash comments, and the SQL strings exactly). Three sub-groups: - Assistant-message lifecycle: pinAssistantMessageOnRunCreate, reconcileAssistantMessageOnRunEnd. - Skill-plugin-candidate detection: isPluginAuthoringRun, hasGeneratedPluginArtifacts, assistantMessageEmittedQuestionForm, deferredSkillPluginCandidateForRun, detectSkillPluginCandidateOnRunSuccess, upsertSkillPluginCandidateAssistantMessage. - Agent-event mapping: persistRunEventToAssistantMessage, runSseEventToPersistedAgentEvent, daemonAgentPayloadToPersistedAgentEvent, normalizePersistedToolInput. All take db/runs/run as args; deps are imports (plugins/index, question-form-detect, db, projects, crypto) -- no daemon module state, no cycle. server.ts imports back the six it references and re-exports the three public ones. No behavior change. Orphan/duplicate checks clean.

…modules Extract the remaining business logic out of startServer's ~2,780-line closure into focused explicit-deps modules, relocate two plugin route-handler groups into routes/plugins, and sweep the dead imports the extractions leave behind. server.ts drops 4,474 -> 3,399 lines (9,025 in the original god-file). Public export surface is unchanged and every moved body is byte-identical to the original modulo documented deviations (resolvedPort/daemonUrl live getters, dynamic-import depth). Extractions (explicit-deps factories, mirroring createStartChatRun): - runtimes/compose-daemon-system-prompt.ts (system-prompt composition) - runtimes/fire-pipeline-for-run.ts (plugin pipeline firing) - http/api-security-middleware.ts (bearer auth + CORS guard) - plugin-registry-view.ts (plugin registry read-model) - run-telemetry-fallback.ts (terminal telemetry fallback) Route-handler relocations (behavior-neutral): - installOrUpgradePlugin -> routes/plugins/index.ts - 9 project/share/candidate handlers -> routes/plugins/project-handlers.ts Cleanup: - remove 140 dead imports server.ts no longer references (export surface identical; 0 ReferenceError across the daemon boot suites). Validation: pnpm typecheck 0, pnpm guard 55/0, targeted daemon suites green (compose/critique/pipeline/plugins/telemetry + real-server boot).

lefarcen · 2026-07-04T06:52:22Z

Thanks @leonaburime-ucla — the stacked split is well scoped, and the PR body makes the intent easy to follow.

Since this is still a draft, I'll hold off on review until you mark it ready for review.

leonaburime-ucla · 2026-07-04T06:52:41Z

This is the final rewrite of server.ts. Please validate however and let me know if there are any issues. I think this is the top file to be refactored to make the daemon more scalable.

lefarcen · 2026-07-04T06:58:57Z

Thanks — the direction makes sense, and this does look like the highest-leverage server.ts split in the stack.

Since the PR is still in draft, let's keep the first validation pass on the current head with the requested reviewer. Once it's marked ready for review, we'll follow up in-thread on any concrete issues that come back.

leonaburime-ucla · 2026-07-04T07:00:08Z

Sorry, @lefarcen should be marked as ready now

nettee

@leonaburime-ucla I reviewed the authored top commit f1aabe9e and checked the changed-range extractions around API auth/origin middleware, plugin route relocation, pipeline firing, telemetry fallback, and daemon system-prompt composition. The moved code and the server.ts rewiring stay behaviorally aligned with the prior implementation from what I could validate in this worktree, and I didn’t find an actionable regression in the refactor.

I wasn’t able to re-run the daemon typecheck locally because this prepared reviewer worktree does not have node_modules installed (pnpm --filter @open-design/daemon typecheck stops immediately on a missing esbuild dependency in packages/contracts), so my validation here is code-review based rather than a fresh local build. This is a substantial cleanup of a high-risk surface, and the extraction boundaries are much easier to reason about now.

_{🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.}

lefarcen · 2026-07-04T07:09:41Z

Thanks — got it.

I'm adding needs-validation here because this refactor moves live daemon execution and API-path logic, so we'd still want a QA pass before merge even though the intent is behavior-neutral.

One small PR-body fix before this wraps up: please update the Surface area section to explicitly check None (right now the prose says internal-only, but no box is checked there).

leonaburime-ucla · 2026-07-04T07:10:58Z

I think I can make it even better from what I see by extracting some stuff into a new server/ folder with core, types, util files and more...but i wanna check first to see if this PR works and you guys will approve it.

I also want to do the same for other free, flat files in daemon/src soon. will need to run codegraph analysis to see what to group

lefarcen · 2026-07-04T07:17:26Z

That follow-up direction makes sense.

I'd keep this PR scoped to proving the current split first, then do the broader server/ regrouping in separate follow-up PRs once this one validates cleanly. I’ve queued QA on the current head; after that, the next extraction pass should be much easier to review in smaller slices.

leonaburime-ucla · 2026-07-04T13:39:55Z

Thanks @lefarcen — updated the Surface area section to explicitly check the None box (kept the internal-only prose right below it as context). Understood on needs-validation; the PR is behavior-neutral but I agree a QA pass on the live daemon/API paths is warranted before merge.

lefarcen · 2026-07-04T13:46:10Z

Thanks — that clears the last PR-body follow-up from my side.

The description looks complete now, so we'll just keep this head in the QA queue and wait for validation before merge.

AmyShang-alt · 2026-07-08T10:17:55Z

Hi @leonaburime-ucla, thank you for putting this together and for taking such a thoughtful pass at the server.ts decomposition.

The direction here is absolutely appreciated, and the write-up made the intent and validation scope easy to follow. We are already handling this specific business-logic split internally as part of our own daemon refactor track, so I am going to close this PR rather than ask you to keep rebasing the stacked branch.

Really appreciate the contribution and the care you put into isolating the risk areas. The cleanup direction is valuable, and we would be happy to review smaller follow-up refactors once this internal pass settles.

LA added 16 commits July 3, 2026 08:50

lefarcen requested a review from nettee July 4, 2026 06:50

lefarcen added size/XXL PR changes 1500+ lines risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps type/refactor Code refactor (no behavior change) labels Jul 4, 2026

leonaburime-ucla mentioned this pull request Jul 4, 2026

refactor(daemon): extract startChatRun from server.ts into an explicit-deps chat-run module #5128

Open

1 task

leonaburime-ucla marked this pull request as ready for review July 4, 2026 06:58

nettee approved these changes Jul 4, 2026

View reviewed changes

lefarcen added the needs-validation Runtime change detected; needs human or /explore agent validation. label Jul 4, 2026

lefarcen requested a review from AmyShang-alt July 4, 2026 07:17

AmyShang-alt closed this Jul 8, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(daemon): dissolve the startServer god-function (slice 4) - #5152

refactor(daemon): dissolve the startServer god-function (slice 4)#5152
leonaburime-ucla wants to merge 16 commits into
nexu-io:mainfrom
leonaburime-ucla:arch/server-startserver-endgame

leonaburime-ucla commented Jul 4, 2026 •

edited

Loading

Uh oh!

lefarcen commented Jul 4, 2026

Uh oh!

leonaburime-ucla commented Jul 4, 2026

Uh oh!

lefarcen commented Jul 4, 2026

Uh oh!

leonaburime-ucla commented Jul 4, 2026 •

edited

Loading

Uh oh!

nettee left a comment

Uh oh!

lefarcen commented Jul 4, 2026

Uh oh!

leonaburime-ucla commented Jul 4, 2026 •

edited

Loading

Uh oh!

lefarcen commented Jul 4, 2026

Uh oh!

leonaburime-ucla commented Jul 4, 2026

Uh oh!

lefarcen commented Jul 4, 2026

Uh oh!

AmyShang-alt commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

leonaburime-ucla commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What users will see

What this does

Surface area

Validation

Uh oh!

lefarcen commented Jul 4, 2026

Uh oh!

leonaburime-ucla commented Jul 4, 2026

Uh oh!

lefarcen commented Jul 4, 2026

Uh oh!

leonaburime-ucla commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nettee left a comment

Choose a reason for hiding this comment

Uh oh!

lefarcen commented Jul 4, 2026

Uh oh!

leonaburime-ucla commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lefarcen commented Jul 4, 2026

Uh oh!

leonaburime-ucla commented Jul 4, 2026

Uh oh!

lefarcen commented Jul 4, 2026

Uh oh!

AmyShang-alt commented Jul 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

leonaburime-ucla commented Jul 4, 2026 •

edited

Loading

leonaburime-ucla commented Jul 4, 2026 •

edited

Loading

leonaburime-ucla commented Jul 4, 2026 •

edited

Loading