Skip to content

refactor(cli): route every CLI process through the Effect process layer - #1069

Draft
zxch3n wants to merge 5 commits into
feat/effect-process-servicefrom
feat/effect-process-callers
Draft

zxch3n wants to merge 5 commits into
feat/effect-process-servicefrom
feat/effect-process-callers

Conversation

@zxch3n

@zxch3n zxch3n commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Related issue

Refs #429

Stack

  1. docs: propose Effect-scoped turn execution and lifecycle migration roadmap #1057 — migration plan (docs), base main
  2. feat(cli): terminate ACP process trees through an Effect process layer #1065 — L0 platform + L1 process-tree layer, ACP processes
  3. refactor(cli): route every CLI process through the Effect process layer #1069 — every remaining CLI process caller + boundary guard
  4. refactor: one Effect process layer for the CLI, Electron main and supervisor #1070 — one process layer for CLI, Electron main, supervisor and shared helpers

Merge in order; after each merge retarget the next PR to main.

Problem / pressure

#1065 moved only the ACP-related processes onto the Effect process layer. About 30 other files in apps/cli/src still called child_process, cross-spawn or process.kill directly, each with its own timeout and kill handling. With two implementations side by side, new code would keep copying the old one.

Summary

  • New L1 capabilities (apps/cli/src/platform/process/command.ts and others):
    • runCommand / runCommandOk collect output with a per-stream ceiling. The whole process tree is ended only when the caller stops waiting: timeout, interruption, or oversized output.
    • runCommandSync is for callers that must stay synchronous; it requires a timeout.
    • isPidAlive.
    • ManagedProcess.closed: exit plus drained stdio.
    • SpawnSpec.windowsDetached.
    • Promise facades for all of the above: runCommandText, runCommandTextSync, startProcess, isPidAliveSync.
  • Migrated callers (all of them):
    • git and gh calls: code-collab, file index, fork, workspace git, worktree gc and manager, local project control, attachments, title generator, credential resolver, review automation;
    • memory and process-table probes; open-browser, which now uses rundll32 on Windows so URLs skip cmd parsing;
    • the daemon runner and Worker; the watch worker; MCP host / lody subcommand children;
    • cloudflared; the upgrade installer; worktree setup scripts; the IPC lock liveness check; PTY termination.
  • Enforcement:
    • scripts/check-cli-process-boundary.mjs runs in pnpm check and check:quick.
    • It fails when CLI source other than platform/process/node-process.ts imports child_process / cross-spawn, references node-pty, or calls process.kill.
    • Its allowlist has only standalone-script source text and the node-pty loader, each with a stated reason.
    • apps/cli/AGENTS.md points new code at the layer. A stale, duplicated pr-poller paragraph was replaced with a link to its scoped AGENTS.md to stay under the 8 KiB gate.
  • Decisions and trade-offs are recorded in the process-tree-layer note under "Follow-up: every CLI process caller":
    • a finished command keeps what it deliberately started;
    • setup scripts are ended as a tree only on failure;
    • the PTY gets SIGHUP before its group is ended;
    • the MCP lody subcommand stays in the agent's process group;
    • sync callers gained timeouts;
    • own process groups mean no controlling terminal for these commands in foreground CLI runs.

Visual explanation

flowchart TD
    subgraph Promise callers
      G["git / gh / probes"] --> RT["runCommandText(Sync)"]
      L["daemon, worker, MCP, cloudflared, installer, setup"] --> SP["startProcess"]
      I["IPC lock"] --> PA["isPidAliveSync"]
      P["PTY"] --> TP["terminatePtyProcessGroup"]
    end
    RT --> RC["runCommand / runCommandSync"]
    SP --> MP["spawnProcess + terminateTree"]
    PA --> AL["isPidAlive"]
    TP --> MP
    RC --> MP
    MP --> NP["NodeProcess (only OS access)"]
    AL --> NP
    Guard["check:cli-process-boundary"] -.forbids direct use.-> G & L & I & P
Loading

Before / after

Before After
~30 files with their own spawn/exec/kill handling One process layer; a guard fails pnpm check on bypasses
A setup-script timeout signalled the shell only, leaking descendants The whole tree ends on failure or timeout
Unbounded waits after SIGKILL (watch worker, MCP host, cloudflared) Every wait bounded; TerminationFailed surfaced
Sync git calls with no timeout Sync calls carry explicit timeouts
open-browser via a cmd shell string Explicit argv; rundll32 on Windows

Test plan

  • corepack pnpm check passes end-to-end: typecheck; lint with 0 errors; all package tests (CLI 3219 passed, 4 skipped); i18n; code-collab, platform, CLI process and public boundary guards.
  • New tests:
    • runCommand over real processes: output, stdin, CommandFailed, ENOENT.
    • runCommand over the fake table: timeout ends the tree; a finished command keeps its daemon; oversized output fails at once and ends the tree.
    • Windows detach.
    • A real setup script whose background sleep is gone after a later step fails.
    • Upgrade cancellation ending npm's lifecycle-script descendant.
    • A real PTY whose shell ignores HUP and TERM gets SIGKILLed.
    • The daemon runner lifecycle on the fake table.
    • open-browser argv per platform.
    • A real-git git-identity test, and a scattered-diff diff-line-counts test.
  • Tests that mocked cross-spawn now use the nodeProcess seam, so they no longer assert mock call counts.
  • Not verified:
    • Windows hosts (taskkill trees, rundll32, windowsDetached);
    • a production vite build of the file-index and diff workers, which now pull in the facade (effect plus node builtins; no wasm or top-level await);
    • lody-mcp-http-server.ts and the daemon-runner Worker spawn, which have typecheck coverage only.

Context handoff

The Lody team asked that every dependent of the L0/L1 responsibilities move to the Effect implementation, so that no second implementation keeps pulling new code back, with AGENTS.md guidance and the PRs stacked. Electron main, cli-supervisor and packages/shared (13 files) run outside the CLI and are not covered here: that needs the layer moved into a shared package, which is a separate decision.

🤖 Generated with Claude Code

Move every remaining process caller in apps/cli/src onto
apps/cli/src/platform/process, so the CLI has a single process implementation:
git and gh invocations, daemon/worker/MCP-host children, cloudflared tunnels,
worktree setup scripts, memory probes, the upgrade installer, the pid
liveness probe, and PTY termination.

Add runCommand/runCommandOk (bounded output, tree ended only when the caller
abandons the command), runCommandSync for synchronous-by-contract callers
(timeout required), isPidAlive, ManagedProcess.closed, SpawnSpec.windowsDetached,
and their Promise facades.

Enforce the boundary with scripts/check-cli-process-boundary.mjs, wired into
pnpm check and check:quick, and point new code at the layer from
apps/cli/AGENTS.md (replacing a stale, duplicated pr-poller paragraph to stay
under the size gate). Record the decisions in the process tree layer note.

Model: claude-opus-5-5

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Model: claude-opus-5-5

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
zxch3n and others added 3 commits September 28, 2026 01:50
Model: claude-opus-5-5

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Resolve the Codex profile login spawn onto startProcess, and move main's
new direct OS calls onto the process layer: the Codex profile pid probe
uses isPidAliveSync and the profile logout uses runCommandText, recording
its pid through a new CommandSpec.onSpawned hook.

Model: claude-opus-5-5

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant