refactor(daemon): extract startChatRun from server.ts into an explicit-deps chat-run module by leonaburime-ucla · Pull Request #5128 · nexu-io/open-design

leonaburime-ucla · 2026-07-03T16:46:25Z

Why

Use case: continuing the strangler-fig decomposition of apps/daemon/src/server.ts that upstream already started with the routes/ registrars and server-context.ts. I'm building on top of the daemon and every non-trivial change funnels through this file.
Pain: server.ts is a 9,543-line @ts-nocheck file whose core is startChatRun, a 3,416-line closure over dozens of startServer locals (cyclomatic ~267, cognitive ~637). It can't be typechecked, reviewed, or tested in isolation, and it's the fastest-growing part of the file (+~280 lines in the last two days on main).

What users will see

Nothing. This is a structural move: the public API, the /api/* surface, SSE event shapes, and server.ts's export list are byte-for-byte identical. The QA regression surface is chat runs end-to-end (all agents), same-run retry/resume, and run SSE streams.

What this does

Two commits, reviewable in order:

test(daemon): golden characterization specs for the SSE event shapes startChatRun emits (apps/daemon/tests/chat-run-sse-shapes.test.ts): happy path (start → agent text_delta → terminal end, status succeeded), agent error frames (error with createSseErrorPayload nesting), missing-binary AGENT_UNAVAILABLE, and monotonic SSE ids. Runs against fake agent binaries with OD_DATA_DIR + OD_AGENT_HOME isolation so detection never reaches host binaries. These went green on pristine main before the move.
refactor(daemon): the extraction. startChatRun moves to apps/daemon/src/runtimes/start-chat-run.ts behind an explicit-deps factory createStartChatRun(deps). The body is byte-identical to the original closure (verified by mechanical diff) with one documented exception: four daemonUrl references become deps.daemonUrl, because server.ts reassigns daemonUrl after listen and the run must observe the live value — the call site passes it as a getter. 133 captured identifiers that server.ts merely re-exported from other modules become direct imports; ~39 server.ts module-scope helpers/registries and startServer locals flow through deps. server.ts drops 9,543 → 6,172 lines.

Scope / boundary

No behavior changes. Latent issues found during the move are listed under Adjacent issues and left untouched, per move-only discipline.
// @ts-nocheck is carried over into the extracted module deliberately (the closure was always under server.ts's file-level @ts-nocheck); typing those 3,400 lines is a separate follow-up, and the module header says new sibling code must not copy the pragma.

Surface area

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

Validation

npx tsc -p apps/daemon/tsconfig.json --noEmit → 0 errors (src and tests)
pnpm guard → pass (0 failures)
Characterization suite: 4/4 before and after the move
Byte-identity: extracted body, after reversing the four deps.daemonUrl rewrites, === the original closure text from the pre-move commit
server.ts export surface: diff of ^export lines vs pre-move → empty
Full pnpm --filter @open-design/daemon test: 21 failures in 6 files, all pre-existing at the baseline commit (same files fail on pristine main + test commit on this machine). The one initially suspicious delta — run-retry-runtime.test.ts "retries a silent first-token stall" — is a pre-existing flake: at baseline it alternates between passing and failing across identical standalone runs (3× branch runs and 3× baseline runs compared).

Adjacent issues (not fixed here, per move-only discipline)

effectiveSkillId is referenced in startChatRun's critique-orchestrator path but is only declared inside composeDaemonSystemPrompt's scope — a latent ReferenceError on that path, hidden by @ts-nocheck. Preserved verbatim (module-scope lookup fails identically).
BufferedStdoutChunk is used as a type annotation but declared nowhere; also @ts-nocheck-tolerated. Preserved verbatim.
run-retry-runtime.test.ts is flaky on main (see Validation).

🤖 Generated with Claude Code

lefarcen · 2026-07-03T16:50:00Z

Hey @leonaburime-ucla — the characterization-suite + byte-identity discipline here make a move of this size much easier to reason about. I'll triage the PR now and hand it to the pool reviewer for the code pass.

💡 To drive this PR to merge hands-free, paste this to your AI coding agent (Claude Code / Codex / opencode / Cursor …):
Take over nexu-io/open-design#5128 until it merges — read https://raw.githubusercontent.com/nexu-io/looper/main/skills/pr-takeover/SKILL.md and follow it.

lefarcen · 2026-07-03T16:53:19Z

🧪 Queued for QA validation — this PR moves a live chat-run execution path, so we'll want a manual QA pass before it merges. Nothing needed from you right now; we'll update here once that validation is done. Thanks for the careful write-up. 🙏

nettee

I found one blocking issue in the extracted startChatRun module that needs to be fixed before this refactor can land safely. The inline comment has the concrete location and suggested change.

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

nettee · 2026-07-03T16:54:45Z

+  resolveProjectDir,
+  SandboxImportedProjectError,
+  resolveProjectDir,


resolveProjectDir is imported twice from ../projects.js in this new module. That is not just redundant: ESM import bindings must be unique, so this file will fail to parse/build before any of the characterization tests can even run. The duplicated binding is visible in the changed import list at lines 230 and 232. Remove the second resolveProjectDir entry so the extracted module keeps the same runtime behavior without introducing a syntax-level failure.
_{🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.}

lefarcen · 2026-07-03T17:02:43Z

Hey @leonaburime-ucla — the current blocker is already called out in @nettee's review: apps/daemon/src/runtimes/start-chat-run.ts imports resolveProjectDir twice, so the extracted module won't parse until that duplicate binding is removed. Once that's fixed and pushed, this should be in a much better spot for the next pass.

…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).

nettee

I still see the unresolved duplicate-import blocker in apps/daemon/src/runtimes/start-chat-run.ts:230-232 on this head, and I found two additional post-move relative-import regressions in the extracted module. The new code-anchored findings are inline below.

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

nettee · 2026-07-03T19:34:35Z

+        : null;
+    if (antigravityConcreteModel) {
+      const { acquireAntigravityModelLock } = await import(
+        './runtimes/defs/antigravity.js'


Moving this code under src/runtimes/ changed the relative base, but this import string still uses the old server.ts-relative path. From start-chat-run.ts, ./runtimes/defs/antigravity.js resolves to src/runtimes/runtimes/defs/antigravity.js, so any Antigravity run with a concrete model now throws before buildArgs, and the same stale path is repeated later for waitForAgyToReadModel at line 2289. That breaks the per-model serialization lock for exactly the runs this refactor was supposed to preserve. Please rewrite both dynamic imports to ./defs/antigravity.js so they still target apps/daemon/src/runtimes/defs/antigravity.ts after the move.
_{🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.}

nettee · 2026-07-03T19:34:35Z

+        chatAgentId: typeof agentId === 'string' ? agentId : null,
+        chatModel: typeof safeModel === 'string' ? safeModel : null,
+      };
+      void import('./memory-llm.js')


This dynamic import has the same post-move path problem. start-chat-run.ts now lives in src/runtimes/, so ./memory-llm.js resolves to src/runtimes/memory-llm.js, but the module still lives at apps/daemon/src/memory-llm.ts. Because this branch already swallows import failures in .catch, the run now appears successful while LLM memory extraction and annotation distillation silently stop happening after every extracted run. Please retarget the import to ../memory-llm.js so the refactor stays behavior-preserving.
_{🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.}

lefarcen · 2026-07-03T19:40:52Z

Hey @leonaburime-ucla — @nettee's current-head review has the actionable blockers to address now: the duplicate resolveProjectDir import is still there, and the move introduced two stale relative imports in start-chat-run.ts (./runtimes/defs/antigravity.js and ./memory-llm.js). Once those are fixed on this head, it should be ready for another pass.

…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).

nettee

I rechecked the extracted startChatRun module on 4263deba51e8b1ae0d09c5cd066291d5a4767831, and the remaining blockers I found are both in the new characterization suite that is supposed to protect this refactor. The inline comments call out the concrete failure modes and the assertions needed to make the tests reliable and behavior-preserving.

_{🔁 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 lines 61-65

readSseUntil stops as soon as the raw buffer contains the marker text, but SSE chunking is arbitrary, so that can happen before the matching frame's data: line or trailing `

arrives. The tests below immediately callparseSseFramesand assert on JSON payload fields, which makes the new suite flaky on chunk boundaries even when the daemon is behaving correctly. Please wait for a complete frame before returning here (for example, only break once the marker appears in a block that is terminated by

`, or parse incrementally and only surface fully-delimited frames).

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

Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges

Location: apps/daemon/tests/chat-run-sse-shapes.test.ts RIGHT lines 271-299

These two negative-path checks don't actually pin the SSE shapes that the PR description says this suite is characterizing. In the opencode case, line 272 reads until event: end but then throws that stream data away and only checks /api/runs/:id, so a regression where the run record becomes failed but the terminal SSE frame is missing or malformed would still pass. In the missing-binary case, the test only searches the raw /api/chat body for AGENT_UNAVAILABLE and the absence of "status":"succeeded", which would also pass if the daemon emitted the code in the wrong frame or omitted the terminal end payload entirely. Please parse the full SSE body in both branches and assert the actual error/end frames (error.code === 'AGENT_UNAVAILABLE' or AGENT_EXECUTION_FAILED, plus end.status === 'failed') so the characterization suite really guards the moved SSE contract.

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

Inline comment could not be anchored: inline anchor is outside the PR diff anchorable ranges

lefarcen · 2026-07-03T20:03:57Z

Hey @leonaburime-ucla — @nettee's latest current-head review has the blockers to focus on now, and they're both in apps/daemon/tests/chat-run-sse-shapes.test.ts: readSseUntil can still return on a partial SSE frame, and the two negative-path checks still need to assert the actual error / terminal end frames instead of only the run record or raw body text. Once those test assertions are tightened up, this should be ready for another pass.

…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).

nettee

@leonaburime-ucla I reviewed the extracted startChatRun module, the server.ts call-site wiring, and the new SSE characterization suite on 6acdfd1782860c526ee32dbe9171b52fe6f3f293. I also re-ran pnpm --filter @open-design/daemon typecheck and pnpm --filter @open-design/daemon exec vitest run tests/chat-run-sse-shapes.test.ts; both passed, and I didn’t find any actionable regressions in the changed ranges. This is a careful extraction, and the added golden coverage makes the move much easier to trust.

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

leonaburime-ucla · 2026-07-04T06:51:15Z

I have fully rewritten server.ts here #5152

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

Thanks for linking #5152 — that helps clarify the stacking.

We'll keep #5128 on its current QA / merge path, and treat #5152 as the follow-on server.ts rewrite once this slice is out of the way.

AmyShang-alt

QA validation passed for current head 6acdfd1782860c526ee32dbe9171b52fe6f3f293.

Evidence:

Automated validation passed locally per QA run: daemon typecheck / focused chat-run-sse-shapes.test.ts / guard.
Manual runtime smoke on PR runtime looked normal.
Failure path sampled with unauthenticated Cursor agent: UI surfaced an explicit recovery/error state, the run did not hang in running, and it did not report succeeded.
Focused SSE suite covers AGENT_UNAVAILABLE / AGENT_EXECUTION_FAILED plus terminal failed end frames.

Result: accepted from QA for the startChatRun extraction. Remaining mergeability/conflict state, if still present, is a merge-maintenance issue rather than a QA runtime blocker.

@PerishCode

…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.

lefarcen · 2026-07-09T12:12:53Z

Hey @leonaburime-ucla — this PR is approved and QA-validated, but there's a merge conflict with main that's blocking the merge. Could you rebase arch/chat-run-extraction onto the latest main? Once that's resolved, we should be able to land this. 🚀

…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.

leonaburime-ucla · 2026-07-09T14:48:27Z

Rebased onto latest main (780917104) — the merge conflict is cleared and the PR shows MERGEABLE again.

Heads-up that this wasn't a clean replay: the only conflict was in server.ts, but it was there because main landed ~8 commits inside the startChatRun closure since this branch was cut. Rather than drop them, I 3-way-merged main's closure edits into the extracted runtimes/start-chat-run.ts, so all of them are preserved in the new module:

token revoke on startup failure (run.onFinalize, fix(daemon): revoke run tool token on startup failure #5207)
AMR default-model resume gating (fix(amr): improve unavailable model gating #5335 / feat(daemon): AMR native session resume across turns #4704) via resolveDefaultModelFromOptions
memory reply-capture rework — memoryReplyText from the send() channel instead of raw stdout (fix(daemon): stop the memory extractor re-mining turns and mining raw transport #5233)
watchdog forced-shutdown targeting the stalled child (fix(daemon): target the stalled child in watchdog forced-shutdown escalation #5202)
plain-stream artifact persistence (fix: persist plain-stream artifact tags #4731)
terminal control-sequence stripping (TerminalControlSequenceStripper)
isPluginAuthoringRun(db, run, getSnapshot) signature + signalChildProcess change

I also repointed the attachAcpSession / attachPiRpcSession imports to ../agent-protocol/index.js (the #5200 acp.ts/pi-rpc.ts split), and dropped the now-unused closure-only imports from server.ts.

Verification on the rebased tree:

closure body is byte-identical to main's, modulo the 4 deps.daemonUrl getter references (the extraction's only intended body change)
pnpm --filter @open-design/daemon typecheck (src + tests) green
pnpm guard green (78/78, incl. the @ts-nocheck import-resolution guard)
golden SSE characterization suite 4/4, plus adjacent chat-run-* / sse-response suites 49/49 green

nettee

@leonaburime-ucla I reviewed the extracted startChatRun module, the server.ts dependency wiring, and the SSE characterization coverage on 780917104305fac935638e875ad53824d758eeb4. I also checked the rebase-specific merges called out in your follow-up note: the token-finalize cleanup, AMR default-model concretization, guarded memory reply capture, forced child-shutdown targeting, terminal-control stripping, and plain-stream artifact persistence are all present in the extracted runtime, and the tightened SSE tests still assert the failure-path error and terminal end frames directly. Nice job keeping a large move reviewable and behavior-focused through the rebase.

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

github-actions · 2026-07-12T18:19:08Z

@leonaburime-ucla friendly reminder: this PR appears to be waiting on author action (merge conflict) and has had no human activity for more than 3 days.