refactor(daemon): server.ts decomposition slice 2 — media/SSE/shell concern modules - #5132
refactor(daemon): server.ts decomposition slice 2 — media/SSE/shell concern modules#5132leonaburime-ucla wants to merge 8 commits into
Conversation
…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.
|
Thanks for splitting this into byte-identical slices — that makes the server.ts unwind much easier to review. One small PR-body follow-up before pool review: please tick the Surface area checklist ( |
lefarcen
added
size/XXL
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
I found one build-blocking issue in the changed daemon extraction. The new start-chat-run.ts module imports the same binding twice, which makes the module fail at ESM parse time before runtime behavior can be validated.
| listProjectFolders, | ||
| resolveProjectDir, | ||
| SandboxImportedProjectError, | ||
| resolveProjectDir, |
There was a problem hiding this comment.
Remove the duplicate resolveProjectDir import. This import block now declares the same ESM binding on lines 230 and 232, and duplicate named imports are a syntax error (Identifier 'resolveProjectDir' has already been declared) before TypeScript or the daemon can run this module. Keeping only one resolveProjectDir entry preserves the moved behavior and unblocks daemon startup/typecheck.
There was a problem hiding this comment.
Fixed on the current head (1cdaf6a6): the import block now declares resolveProjectDir exactly once, so the duplicate ESM binding is gone and the module parses. Verified with a full @open-design/daemon typecheck (src + tests, exit 0). The fix landed via #5128, which this PR is stacked on, so it's already included here.
|
Converted to draft while the CI reds are resolved. Both failing checks (E2E Vitest + Preflight) are inherited from the base PR #5128 — they fail identically there and are not introduced by this slice's extractions:
Both will be fixed on #5128; this PR will be rebased and returned to ready once green. |
|
Got it — thanks for flipping this to draft and for spelling out which reds are inherited from #5128. @PerishCode's blocking note on the duplicate 💡 To drive this PR to merge hands-free, paste this to your AI coding agent (Claude Code / Codex / opencode / Cursor …): |
ac6c0a2 to
660a457
Compare
…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.
660a457 to
986ba3f
Compare
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.
986ba3f to
1cdaf6a
Compare
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
I found one test-suite isolation issue in the new daemon SSE characterization test. The extracted runtime code itself looks like a mechanical move in the scoped ranges, but the new test mutates process-wide env that the daemon test harness treats as shared state.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.Location: apps/daemon/tests/chat-run-sse-shapes.test.ts RIGHT line 181
Restore the process-level daemon env after this suite, or avoid overriding OD_DATA_DIR here. tests/setup.ts already installs one shared OD_DATA_DIR for the Vitest process, and server.ts resolves RUNTIME_DATA_DIR at module import time. This new beforeAll rewrites process.env.OD_DATA_DIR and OD_AGENT_HOME before importing ../src/server.js, but afterAll only closes the server and never restores either variable. In the full daemon suite, files run serially and several later route tests read process.env.OD_DATA_DIR while also importing the cached server module; depending on test order, this can leave the env and the module-scope runtime data root pointing at different directories, making the suite order-dependent. Capture the original values and restore them in afterAll, and either rely on the setup-provided data root or reset/import the server in a way that keeps process.env.OD_DATA_DIR and the module-scope root aligned.
Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges
|
Thanks for clearing the duplicate-import blocker on this head. The remaining blocker is @PerishCode's new note on |
…suite Addresses @PerishCode's CHANGES_REQUESTED on nexu-io#5132. The golden SSE-shapes suite overrides process.env.OD_DATA_DIR and OD_AGENT_HOME in beforeAll (so server.ts resolves RUNTIME_DATA_DIR at import time against an isolated temp root) but afterAll only closed the server and never restored them. Because setup.ts installs one shared OD_DATA_DIR for the whole Vitest process and the daemon suite runs serially against a cached server module, leaving our temp dirs set left a later route test reading process.env.OD_DATA_DIR pointing at this suite's now-removed directory — making the suite order-dependent. Capture the originals alongside originalPath and restore (or delete) them in afterAll, mirroring the existing PATH restore. tests typecheck (tsconfig.tests.json) exit 0; SSE-shapes suite 4/4 green. Note: the branch's separate merge conflict with main is the stacked-strangler divergence on server.ts and is best resolved by rebasing after nexu-io#5128 lands.
|
@leonaburime-ucla I'm holding off on generating review comments for #5132 because this pull request has merge conflicts right now. Please resolve the conflicts with main and push the updated branch. Once that's done, request or wait for the review to run again and I'll take another look. 🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos. |
|
Thanks for pushing the update. The current blocker has shifted from the test-isolation note to the merge conflict with |
3 participants
Why
apps/daemon/src/server.tsis a ~6.2k-line@ts-nocheckgod-file. Slice 1 (#5128) lifted the chat-run engine out; this slice continues the strangler-fig by moving cohesive preamble helpers into their rightful concern modules, one byte-identical move per commit. The pain being addressed is maintainability/reviewability: helpers for unrelated concerns (media task state, SSE transport, shell exec) are tangled into one file that no one can hold in their head, and its@ts-nocheckblanket hides real bugs. Shrinking it toward a thin composition root is the enabling step for eventually peeling@ts-nocheck.My use case: I'm decomposing this file in reviewable slices so each PR is a mechanical, low-risk move a reviewer can verify by inspection rather than a big-bang rewrite.
What users will see
Nothing. This is an internal refactor — the public API and all runtime behavior are identical. No endpoint, CLI, or UI surface changes. QA regression surface: media generation task progress/SSE (task registry), any SSE-streaming route (chat/runs/terminal/memory/library), and the plugin-share
gh/login-shell exec path.What this does
Three byte-identical extractions out of
server.ts, each its own commit:media/task-registry.ts— the in-memory media-task registry (mediaTasksmap + hydrate/create/persist/append/notify/snapshot + TTL), layered overmedia/tasks.tsSQLite persistence.http/sse.ts— the SSE transport helpercreateSseResponse+SSE_KEEPALIVE_INTERVAL_MS, alongside the otherhttp/helpers.server.tsre-exports both names, so its public surface is unchanged.shell/commands.ts— buffered subprocess + POSIX login-shell command helpers (execFileBuffered,quotePosixShellArg,build*ShellCommand,execGhBuffered,execCommandViaLoginShell) +ensureGhReady.server.ts: 6172 → 5924 lines (−248). Every new file carries a@moduledocblock and per-export JSDoc.Scope / boundary
deploy.tsis strict-typed while these fns are@ts-nocheck, so it needs a typed migration, not a byte-identical move; (b) removing the now-confirmed-deadensureGhReady; (c) dedupingcli.ts's independent copies of the shell helpers ontoshell/commands.ts. A stale orphaned JSDoc aboveexecFileBuffered(describing neither it nor its neighbor) was dropped during the move to avoid mis-attaching it.Surface area
Refactor only — no new user-facing surface. No contracts, skills, design-systems, CLI flags, env vars, i18n keys, or dependencies touched. (New internal daemon modules under
apps/daemon/src/.)Validation
Per commit, in the worktree:
npx tsc -p apps/daemon/tsconfig.json --noEmit— exit 0pnpm guard— exit 0pnpm --filter @open-design/daemon exec vitest run tests/chat-run-sse-shapes.test.ts— 4/4tasks-persistence+tasks-routessuites 14/14 green;policy-routes' 7 failures proven pre-existing by reproducing them on the clean base commit.