refactor(daemon): server.ts strangler-fig slice 3 — preamble module extractions - #5147
Draft
leonaburime-ucla wants to merge 15 commits into
Draft
refactor(daemon): server.ts strangler-fig slice 3 — preamble module extractions#5147leonaburime-ucla wants to merge 15 commits into
leonaburime-ucla wants to merge 15 commits into
Conversation
added 15 commits
July 3, 2026 08:50
…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.
…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.
…-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.
Contributor
|
Thanks @leonaburime-ucla — the stacked slice breakdown and the byte-identical export-surface guard make the intent easy to follow. Since this is opened as a draft on top of #5132 and #5128, we’ll hold off on a full review until the parent slices land and this is marked ready for review. |
lefarcen
added
size/XXL
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Slice-3 commits to review (the rest belong to #5128 / #5132)
f8718bce761d5546prompts/run-context.ts008184c6run-event-analytics.ts722f61f2http/error-payloads.ts+project-display-status.ts095886ebplugin-share.tsc8932061daemon-request-guards.tse4c03f50run-telemetry.ts+ chat-request composition →chat-request-composition.tse38e2b59assistant-message-persistence.tsWhy
apps/daemon/src/server.tsis a ~9.5k-line// @ts-nocheckgod file whosestartServerclosure and preamble mix dozens of unrelated concerns, making it hard to review, test, or type. This is the third strangler-fig slice: it peels self-contained preamble helper clusters out into focused sibling modules so the eventual thin-composition-root + typedstartServerwork has a much smaller surface to fight. Slice 3 alone takes server.ts from ~5.6k to 4,474 lines.Author use case: continuing the agreed decomposition (see #5128 / #5132) one verified, byte-identical slice at a time.
What users will see
Nothing — this is a pure internal refactor. The daemon's public API, HTTP routes, and runtime behavior are unchanged; server.ts's exported symbol surface is byte-for-byte identical (74 exports before and after). QA regression surface: any daemon chat-run / live-artifact / plugin-share / skill-plugin-candidate / telemetry path.
What this does
Moves eight preamble concern-clusters out of
server.tsinto newapps/daemon/srcsibling modules as byte-identical moves (function bodies unchanged; onlyexportkeywords and import paths adjusted).server.tsimports back the symbols it references and re-exports the previously-public ones, so no external importer changes.Deliberately left in place (not byte-identically movable): agent-runtime-env (references
RUNTIME_DATA_DIR/SANDBOX_RUNTIMEmodule consts → server↔module cycle), uploads/multer + event-sinks (module-level mutable state), marketplace-seed + the paths/dir block (daemon data-dir contract, off-limits), and the cloudflare cluster (deferred typed migration intodeploy.ts).Scope / boundary
Daemon-only, one subsystem (server.ts preamble). No behavior changes, no contract changes, no new dependencies. The
startServerclosure (Tier-3) is intentionally out of scope for this slice.Surface area
None (internal refactor — no UI, CLI, contracts, skills, design-systems, env vars, or i18n touched).
Validation
pnpm --filter @open-design/daemon typecheck(src + tests): 0 errorspnpm guard: 55 pass / 0 fail@ts-nocheckblind-spot guard)