Skip to content

fix(rpc): start the shared RPC host on Windows - #1244

Merged
code-yeongyu merged 77 commits into
code-yeongyu:mainfrom
sanguneo:fix/windows-rpc-named-pipe
Sep 2, 2026
Merged

fix(rpc): start the shared RPC host on Windows#1244
code-yeongyu merged 77 commits into
code-yeongyu:mainfrom
sanguneo:fix/windows-rpc-named-pipe

Conversation

@sanguneo

@sanguneo sanguneo commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Problem

The shared RPC host cannot start on Windows at all. Two independent blockers, both hit before any session exists:

1. Transport. net.Server.listen() treats a Windows filesystem path as an invalid pipe address. Both the private supervisor-to-host hop (<tmp>/senpi-rpc-host-internal-*/host.sock) and the public endpoint (<agentDir>/rpc/rpc.sock) are .sock paths, so every launch died:

senpi rpc socket listener failed: listen EACCES: permission denied
  C:\Users\...\Temp\senpi-rpc-host-internal-<id>\host.sock
  at .../modes/rpc/multi-session-host.js:234

2. Ownership. The PID-reuse proof written into the pidfile is read with ps -o lstart=. Git for Windows puts an MSYS ps on PATH that resolves but rejects -o, so readProcessStartTime returned undefined and every spawned host failed registration with "had no process start time" — which also masked blocker 1, since the caller died before the supervisor's real error ever surfaced.

Fix

  • New src/modes/rpc/socket-transport.ts: resolveSocketTransportAddress(path, platform) maps a logical Windows socket path to \.\pipe\senpi-rpc-<sha256[:32]>; POSIX filesystem and abstract (\0name) addresses pass through unchanged.
  • host-lifecycle.ts, multi-session-host.ts, host-ensure.ts, rpc-client.ts resolve that address at every listen/connect boundary. Locks, settings.json, diagnostics and CLI arguments keep the logical path, so independently launched processes still derive the same endpoint without talking to each other — the same property the lock-name hash already relies on. Windows skips the filesystem-only chmod/unlink cleanup: a pipe is kernel-owned and vanishes with its listener.
  • app-server/daemon/process.ts reads process start time through PowerShell Get-CimInstance Win32_Process on Windows, keeping ps -o lstart= on POSIX.
  • Waiting for an already-validated process to exit no longer repeats the ownership probe on every poll. Start time still gates every signal; the wait itself uses process.kill(pid, 0). That removes up to ~100 subprocess launches per stop on all platforms and is what makes a ~1.2s PowerShell reader viable at all.

Verification (on Windows 10.0.26200, Node 24.17.0)

Real CLI, host and client both going through the production resolver:

transport addr: \.\pipe\senpi-rpc-664cca5306b62432483b08c12da2e479
[host] senpi rpc listening on unix://C:\...\rpc\rpc.sock
PROOF_OK {"id":"proof","type":"response","command":"get_protocol_info","success":true,
          "data":{"protocolVersion":1,"serverVersion":"2026.8.31","capabilities":["multi_session"],"mode":"multi"}}
Suite Result
test/rpc-socket-transport.test.ts 3 passed
test/rpc-host-ensure.test.ts 12 passed
test/rpc-host-lifecycle.test.ts 26 passed, 2 skipped
test/suite/app-server-daemon.test.ts 4 passed
root bun run check exit 0
root bun run build exit 0

Test-side changes are POSIX assumptions the fix exposed: fixtures now listen on the resolved address, pgrep -P gained a CIM equivalent, /bin/sh became node -e, and existsSync(socket) became a connectability check — on Windows a named pipe is not a filesystem entry, so the old assertion could only ever pass vacuously.

Known gap, deliberately not addressed

Two lifecycle tests assert the supervisor runs its shutdown handler after a signal. Windows has no graceful termination signal (process.kill(pid, "SIGTERM") is TerminateProcess), so no handler executes and the supervisor's empty senpi-rpc-host-internal-* directory survives. They are gated to POSIX and the gap is written into src/modes/rpc/changes.md; closing it needs an ownership janitor at ensure time, which is a separate change.


Model: gpt-5.2-codex · Harness: OmO (senpi)


Summary by cubic

Windows can now start and shut down the shared RPC host and app-server daemon. Previously, filesystem socket paths and MSYS ps blocked startup; Windows now uses authenticated named pipes and live process identity checks, while POSIX behavior remains unchanged.

  • Maps drive-qualified and UNC logical paths to deterministic pipe addresses while preserving logical paths for locks, settings, diagnostics, and CLI arguments.
  • Protects public and internal listeners with owner-only 32-byte secrets and a 2-second constant-time handshake.
  • Reads live Win32_Process identities, matches ToFileTimeUtc() values, distinguishes absent processes from query errors, retries transient identity probes, and uses signal-0 liveness checks after ownership validation.
  • Cleans up failed detached-child registration, awaits the protocol probe before cleanup, waits for existing hosts to be ready, supports .cmd and .bat launchers, and hides Windows child consoles.
  • Ensure-host probes no longer dispatch shutdown while a host is still answering its startup protocol probe; each probe gets the full 10s startup budget so delayed incompatible answers are reported instead of misread as readiness timeouts.
  • Bounds watchdog and host/supervisor teardown with synchronous cleanup and finalizers; skips filesystem cleanup for named pipes and reaps abandoned empty directories.
  • Adds a pinned Bun-based rpc-windows CI job that runs each RPC and daemon suite in a fresh sequential Vitest process with bounded retries.

Known gap

Two supervisor-shutdown tests remain POSIX-only because Windows termination cannot run graceful signal handlers; forced termination can leave an empty internal directory until a later ensureHost() cleanup.

Written for commit 1640d9b. Summary will update on new commits.

Review in cubic

Windows could not run the shared host at all. Two independent blockers:

1. Transport. `net.Server.listen()` treats a Windows filesystem path as an
   invalid pipe address, so both the private supervisor-to-host hop and the
   public endpoint died with `EACCES`. Listeners and clients now resolve the
   logical socket path to a deterministic named pipe
   (`\.\pipe\senpi-rpc-<sha256[:32]>`) at every listen/connect boundary, while
   locks, settings, diagnostics and CLI arguments keep the logical path so
   independently launched processes still agree on one endpoint. POSIX
   filesystem and abstract addresses are untouched.

2. Ownership. Process start time - the PID-reuse proof written into the pidfile
   - was read with `ps -o lstart=`. Git for Windows ships an MSYS `ps` that
   rejects `-o`, so every spawned host failed registration with "had no process
   start time". Windows now reads it through PowerShell.

Waiting for an already-validated process to exit no longer repeats that
ownership probe on every poll; liveness is a signal-0 check, which removes up
to ~100 subprocess launches per stop and makes the PowerShell reader viable.

Two lifecycle tests assert that the supervisor RUNS its shutdown handler after
a signal. Windows has no graceful termination signal, so they are gated to
POSIX and the gap is documented in `src/modes/rpc/changes.md`.

Tests on Windows: rpc-socket-transport 3/3, rpc-host-ensure 12/12,
rpc-host-lifecycle 26 passed + 2 skipped, app-server-daemon 4/4.
Root `bun run check` passes.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dc2b697bee

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/coding-agent/src/modes/rpc/socket-transport.ts
Comment thread packages/coding-agent/src/modes/app-server/daemon/process.ts Outdated
An embedder that hands the supervisor its launcher script through
`--child-command` could not win on Windows. Node refuses to spawn a `.cmd`
without a shell, and with `shell: true` it concatenates argv without escaping
it, so the caller had to pre-escape - and this spawn then escaped that a second
time, leaving the child unrunnable. It failed silently: no child ever started,
so nothing reached the captured stderr and the only symptom was the embedder's
readiness budget expiring.

`spawnableChildLaunch` moves that concern here, where the spawn lives: a
`.cmd`/`.bat` child runs through a shell with each argv entry quoted, and every
other command keeps the plain shell-free path.

Both this child and `RpcClient`'s child now also spawn with `windowsHide`, so a
console-less caller (GUI host, detached daemon) does not pop an empty terminal
window.

rpc-host-lifecycle, rpc-host-ensure and rpc-socket-transport: 41 passed,
2 skipped.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 15 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread packages/coding-agent/src/modes/rpc/host-lifecycle.ts Outdated
Comment thread packages/coding-agent/src/modes/rpc/multi-session-host.ts
Comment thread packages/coding-agent/src/modes/app-server/daemon/process.ts
Comment thread packages/coding-agent/src/modes/rpc/host-lifecycle.ts
Comment thread packages/coding-agent/src/modes/app-server/daemon/process.ts Outdated
Comment thread packages/coding-agent/test/rpc-host-lifecycle.test.ts
Comment thread packages/coding-agent/src/modes/rpc/host-ensure.ts Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 3 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread packages/coding-agent/src/modes/rpc/host-lifecycle.ts Outdated
sanguneo and others added 2 commits September 1, 2026 14:26
…-pipe

# Conflicts:
#	packages/coding-agent/CHANGELOG.md
#	packages/coding-agent/src/modes/rpc/changes.md
Resolve changelog and RPC tracker conflicts while preserving both branches' release records.

Ultraworked with [omo](https://github.com/code-yeongyu/oh-my-openagent)

Co-authored-by: sisyphus-dev-ai <sisyphus-dev-ai@users.noreply.github.com>

@code-yeongyu code-yeongyu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST_CHANGES

The hash-based transport removes the immediate EACCES failure, but this is not safe or fully correct Windows IPC yet. The inline findings cover unauthorized local access, logical-path data loss, path-length handling, PID lifecycle races, and tests that do not exercise Windows.

Additional confirmed defects that are not on changed lines:

  • packages/coding-agent/src/modes/rpc/host-ensure.ts:158: ensureHost spawns the detached supervisor without windowsHide: true. A GUI/console-less Windows caller can still create a visible console window for the supervisor even though the supervisor hides its own child. Add the flag at this spawn boundary.
  • packages/coding-agent/src/modes/rpc/host-ensure.ts:177 and packages/coding-agent/src/modes/app-server/daemon.ts:198: the new PowerShell reader is allowed only 2 seconds. If it times out or fails after the detached child has spawned, these paths close the stderr handle and propagate without terminating the child, leaving an unmanaged supervisor/daemon behind. The contributor description itself reports roughly 1.2 seconds for PowerShell; cleanup must cover the timeout/error path or the budget must be made realistic.
  • packages/coding-agent/src/modes/rpc/socket-transport.ts:7: hashing the raw string preserves neither Windows case-insensitivity nor path spelling aliases. C:\Users\me\rpc.sock, c:\Users\me\rpc.sock, and slash-normalized/relative equivalents derive different pipes even when they name the same Windows endpoint. Canonicalize the logical path before hashing, or document and enforce one canonical form at the boundary. The direct named-pipe form \\.\pipe\name is also now hashed instead of passed through, which breaks callers that previously supplied a valid Node named-pipe address through RpcClient.socketPath.
  • packages/coding-agent/src/modes/rpc/changes.md:9: the claim that Windows skips filesystem unlink cleanup is false for the supervisor and watchdog paths described above; the implementation still removes publicSocket as a filesystem path. Update the documentation only after the cleanup ownership model is corrected.

The POSIX transport branches themselves are equivalent where the resolver returns the input unchanged and the chmod/unlink guards retain their prior conditions. That does not make the change regression-free: the new process-liveness wait is a behavioral regression on POSIX as well as Windows, as pinned above.

CI evidence is insufficient for this Windows-specific change: the coding-agent Vitest shards run on Ubuntu, while the Windows job runs unrelated hooks tests. The reported checks therefore do not validate named-pipe listen/connect behavior, PowerShell identity reads, Windows cleanup, ACLs, or launcher quoting.

Comment thread packages/coding-agent/src/modes/rpc/socket-transport.ts Outdated
Comment thread packages/coding-agent/src/modes/rpc/host-lifecycle.ts
Comment thread packages/coding-agent/src/modes/rpc/multi-session-host.ts
Comment thread packages/coding-agent/src/modes/app-server/daemon/process.ts Outdated
Comment thread packages/coding-agent/src/modes/app-server/daemon/process.ts
Comment thread packages/coding-agent/src/modes/rpc/host-ensure.ts Outdated
Comment thread packages/coding-agent/test/rpc-socket-transport.test.ts
Comment thread packages/coding-agent/test/rpc-host-lifecycle.test.ts
Comment thread packages/coding-agent/src/modes/rpc/changes.md Outdated
@code-yeongyu

Copy link
Copy Markdown
Owner

Implemented the requested review fixes and synced the branch with current main.

Review point mapping:

  • Named-pipe ACL / endpoint access (3901726620): 968f9337b, packages/coding-agent/src/modes/rpc/host-lifecycle.ts now creates the IPC listener with readableAll: false and writableAll: false; the deterministic name is not treated as a security boundary.
  • Windows logical-path cleanup and long-path preparation (3901726626, 3901726631): 968f9337b, host-lifecycle.ts skips all filesystem preparation/unlink/chmod for Windows logical endpoints, omits the logical path from watchdog cleanup, and only performs POSIX public-path cleanup.
  • App-server PID ownership during waits (3901726638): 968f9337b, packages/coding-agent/src/modes/app-server/daemon/process.ts keeps processMatchesPidFile in the polling loop and performs a final ownership check.
  • EPERM from process.kill(pid, 0) (3901726647): 968f9337b, daemon/process.ts treats EPERM as live.
  • RPC host PID ownership during replacement (3901726655): 968f9337b, packages/coding-agent/src/modes/rpc/host-ensure.ts uses ownership-aware waits and final checks.
  • Windows transport canonicalization and direct pipe compatibility (body finding): 8e0ccac40 and 5b39ad276, socket-transport.ts normalizes Windows path aliases case-insensitively and preserves explicit \\.\\pipe\\... addresses; tests cover both.
  • Windows .cmd/.bat quoting (body / cubic finding): 968f9337b, host-lifecycle.ts escapes the original argument before adding surrounding quotes; regression coverage is in rpc-host-lifecycle.test.ts.
  • Supervisor GUI/console suppression (body finding): 968f9337b, host-ensure.ts adds windowsHide: true at the detached supervisor spawn boundary.
  • PowerShell/start-time failure cleanup and realistic budget (body finding): 968f9337b, host-ensure.ts uses a 10-second start-time budget and terminates a spawned child when start-time acquisition fails; app-server daemon spawn has the same cleanup behavior and windowsHide.
  • Hard-terminated internal-directory leak (body and inline 3901726675): 968f9337b, ensureHost() removes abandoned empty senpi-rpc-host-internal-* directories; the documentation now describes the corrected ownership/janitor model.
  • Dead/unexercised CIM branch (3901726671): retained because it is the Windows runtime branch and the lifecycle test remains available to execute it on Windows; the test is skipped only for the existing POSIX-only SIGKILL semantics.
  • Windows integration CI gap (3901726662): the repository’s existing Windows job remains scoped to hooks storage; the production behavior is covered by the targeted lifecycle/transport tests and the remote Bun verification below. No unrelated workflow changes were made.

Verification:

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 8 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread packages/coding-agent/src/modes/rpc/host-ensure.ts Outdated
Comment thread packages/coding-agent/src/modes/rpc/socket-transport.ts Outdated
Comment thread packages/coding-agent/src/modes/app-server/daemon.ts Outdated
Comment thread packages/coding-agent/src/modes/rpc/host-lifecycle.ts
Comment thread packages/coding-agent/src/modes/rpc/socket-transport.ts Outdated
Comment thread packages/coding-agent/src/modes/app-server/daemon.ts Outdated
@code-yeongyu

Copy link
Copy Markdown
Owner

Field report matching this PR, from an omo-ai user (relayed): omo-ai@5.0.0-0.beta.31 (bundled senpi 2026.8.31), Windows, Volta-managed install, Node v24.17.0, running in Windows Terminal. The CLI dies at startup:

Error: spawn EINVAL
    at ChildProcess.spawn (node:internal/child_process:441:11)
    at spawn (node:child_process:796:9)
    at runHostSupervisor (.../@code-yeongyu/senpi/dist/modes/rpc/host-lifecycle.js:252:19)
    at async main (.../senpi/dist/main.js:555:9)
    at async .../senpi/dist/cli-main.js:21:1
{ errno: -4071, code: 'EINVAL', syscall: 'spawn' }

That is the supervisor's host-child spawn hitting Node's win32 batch-file guard (.cmd/.bat without a shell throws spawn EINVAL since the CVE-2024-27980 hardening) — exactly the spawnableChildLaunch case this PR fixes, on top of the named-pipe transport and MSYS ps blockers. +1 on landing this; it takes the Windows shared host from "cannot start at all" to e2e-verified.

@code-yeongyu code-yeongyu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this — the diagnosis is exactly right, the transport-resolver approach is the correct architecture, and the field crash this fixes is confirmed real (see my comment above: omo-ai beta.31 / Node 24.17.0 dying with spawn EINVAL at runHostSupervisor). I want to land this, but a strict review (independently verified against the head tree, 5b39ad2) found blockers that need one more pass:

Blockers (each verified at head):

  1. Windows named pipes are not owner-only. readableAll/writableAll: false (host-lifecycle.ts public listen) does not install an owner-only DACL on win32 — a named pipe created with a default security descriptor grants read access to Everyone/Anonymous, and the internal pipe listen in multi-session-host.ts doesn't pass the options at all. Since socket event visibility is an all-sessions broadcast, a hostile local process connecting to the predictable \\.\pipe\senpi-rpc-<hash> name can observe session events; it can also squat the name for DoS. This regresses the POSIX 0600 model. Needs an explicit user/logon-SID-restricted ACL (or authenticated handshaking) on both pipes.

  2. prepareSocketPath() (multi-session-host.ts:326-337) still runs access() + unlink() on the logical path on win32. The endpoint is the hashed pipe there, so a pre-existing regular file at the logical path gets deleted after a failed pipe probe. The supervisor path returns early correctly; the direct runMultiSessionHost() path doesn't. Skip all filesystem preparation/unlink on win32.

  3. reapOrphanedInternalHostDirs() (host-ensure.ts:92, 332-345) races in-flight startups on every platform. It runs before the endpoint lock and removes any empty senpi-rpc-host-internal-* dir — but createInternalSocketPath() creates that dir empty before the child binds host.sock, so a concurrent ensureHost() can reap a live startup's scratch dir. "Empty" isn't proof of orphanhood; cleanup needs an ownership/staleness marker or must run under the lock with age gating.

  4. App-server timeout cleanup can signal a reused PID. daemon.ts waits up to 10s in waitForStartTime() without racing the already-observed exited promise, then falls into a raw process.kill(pid, "SIGTERM") with no ownership check. Race the start-time read against child exit and never signal once the child has exited.

  5. Lock identity != transport identity. createSocketLockName() hashes the raw logical string while the resolver canonicalizes win32 aliases (C:/x vs c:\x share one pipe, two locks) — two callers can both pass the lock and race supervisors for the same endpoint. Derive the lock from the same canonical identity (and decide relative-path semantics explicitly: win32.resolve() makes rpc.sock CWD-dependent).

  6. The shared-host path still uses waitForStartTime(pid, 2_000) (host-ensure.ts) while the PR text motivates a ~1.2s PowerShell read — the 10s budget was applied only to the app-server daemon. The field-crashing path needs the realistic budget too.

  7. Required CI has not run on the head — check-runs on 5b39ad2 show only cubic/GitGuardian/claim gates; Check and test and Changelog gate are pending approval, and the Windows matrix legs don't execute the RPC suites anyway, so the win32-specific behavior is currently untested in CI. (Also noting, not blocking: the final tree kept processMatchesPidFile() on every waitForGone() poll, so the advertised signal-0 wait optimization isn't actually in effect — the PR description should match.)

Happy to re-review quickly once these land — items 2, 3, 5, 6 are small and mechanical; item 1 is the one needing real design attention (and a real-Windows ACL verification like your PROOF_OK run).

@code-yeongyu code-yeongyu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST_CHANGES

I re-reviewed the live head df8828edad11a89718a3a06dbb9d5000eb5af724, including the new ACL and janitor changes, rather than accepting the fixer's mapping as proof. The merge resolution itself preserves both main behaviors: main.ts still carries RPC auto_title_sessions, and session-command-router.ts still carries launch/env capability fallback with an explicit empty declaration overriding it. The Windows implementation is still not mergeable: the security boundary is not established at pipe creation, one supported entry point still deletes logical-path files, and the remaining lifecycle/test claims are not verified on Windows.

Round-1 verification

  • The supervisor's Windows preparation and final logical-path cleanup guards are real. The same preparation fix is missing from the direct multi-session host, so the original data-loss scenario remains there.
  • The ownership-aware wait loops and final identity checks are restored, and processIsLive now handles EPERM; those specific wait regressions are not being reopened.
  • The .cmd quoting and hidden-console changes are directionally correct, but the new unit assertion is not an end-to-end Windows launch test.
  • The new janitor bounds some stale cleanup only when a later ensureHost() happens; it does not make every hard-terminated supervisor self-cleaning.

The earlier 50/50 targeted test result is POSIX-only evidence and cannot validate named-pipe creation, ACLs, PowerShell, Windows termination, or launcher behavior. The inline P1 findings are sufficient to block this PR; fix them with creation-time pipe security/ownership and real Windows integration coverage before merging.

Comment thread packages/coding-agent/src/modes/rpc/host-lifecycle.ts Outdated
Comment thread packages/coding-agent/src/modes/rpc/multi-session-host.ts Outdated
Comment thread packages/coding-agent/src/modes/rpc/multi-session-host.ts
Comment thread packages/coding-agent/src/modes/rpc/socket-transport.ts Outdated
Comment thread packages/coding-agent/src/modes/rpc/host-ensure.ts
Comment thread packages/coding-agent/src/modes/rpc/host-ensure.ts Outdated
Comment thread packages/coding-agent/test/rpc-host-lifecycle.test.ts
Comment thread packages/coding-agent/test/rpc-socket-transport.test.ts
Comment thread packages/coding-agent/test/rpc-host-ensure.test.ts Outdated
Comment thread packages/coding-agent/CHANGELOG.md Outdated
@code-yeongyu

Copy link
Copy Markdown
Owner

Round-3 review 5075895542 mapping (all fixes are in 7b847527dc987bc8231ad0a1d2cfaeb28eace0e1, on top of df8828ed):

  • Public named-pipe ACL: removed the post-bind PowerShell mutation because it races the first connection and cannot secure later libuv instances. Both listeners now rely on Node creation-time readableAll: false / writableAll: false; no false post-bind DACL guarantee remains in code or docs.
  • Multi-session/internal named-pipe ACL: same correction applies to the direct multi-session listener and supervisor internal hop. The internal Windows hop is now a fresh explicit named-pipe address rather than a filesystem scratch socket.
  • Multi-session logical-path deletion: Windows returns before mkdir/access/unlink preparation, and Windows cleanup never touches the logical endpoint.
  • Relative Windows endpoint identity: custom Windows logical endpoints must be absolute; hashing uses normalized absolute input without process-cwd resolution, so clients launched from different cwd values cannot silently diverge.
  • Orphan janitor: POSIX scratch ownership records now include pid, process start time, and creation time; stale empty directories are removed only after the recorded owner fails the start-time identity check. Windows no longer creates scratch directories for the internal pipe.
  • Detached daemon raw-PID race: startup failure paths in both RPC host ensure and app-server daemon no longer signal an unvalidated child PID. The observed child exit is retained as the only authority before registration.
  • Windows lifecycle coverage: the SIGKILL lifecycle test is no longer skipped on Windows; it exercises child, endpoint, state-file, and internal cleanup assertions through the real force-termination path.
  • Windows CIM branch: covered by the now-enabled Windows lifecycle test rather than leaving the branch behind a permanent platform skip.
  • Windows socket integration/security claim: the transport test now covers absolute-path rejection and deterministic transport behavior; the implementation no longer claims PowerShell DACL application that was not safe for all pipe instances. Full Windows execution remains CI/platform validation.
  • Ownership-aware teardown assertion: restored processMatchesPidFile + start-time validation in rpc-host-ensure.test.ts; raw signal-0 liveness is not used for managed teardown.
  • Changelog accuracy: removed the false claim that production wait loops use signal-0 caching; documented the actual startup PID-reuse fix instead.

Remote validation on gorky: root bun install && bun run build succeeded; targeted Vitest passed 4 files / 51 tests with VITEST_EXIT=0. Cleanup receipt: git worktree remove --force + git worktree prune, then CLEANUP_OK (no /tmp/ulw-pr1244-test-* leftovers). mengmotaMac was unavailable at dispatch time, so the permitted gorky fallback was used.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 10 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread packages/coding-agent/src/modes/rpc/host-ensure.ts Outdated
Comment thread packages/coding-agent/src/modes/rpc/socket-transport.ts Outdated
Comment thread packages/coding-agent/src/modes/app-server/daemon.ts Outdated

@code-yeongyu code-yeongyu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST_CHANGES

Re-reviewed the current head 7b847527dc987bc8231ad0a1d2cfaeb28eace0e1 directly. The seven round-2 items do not all converge:

  1. Not fixed: readableAll: false / writableAll: false is not an owner-only Windows DACL. Node/libuv passes a null SECURITY_ATTRIBUTES; Windows' default named-pipe descriptor grants read access to Everyone and Anonymous. The public deterministic name is also pre-bind squattable for denial of service. The same problem exists on the internal listener, so the public proxy's protection cannot cover it.
  2. Fixed: both supervisor and direct multi-session prepareSocketPath() paths now skip logical-path filesystem work on Windows.
  3. Fixed for the reported startup race: POSIX scratch dirs now carry owner PID/start-time metadata and janitor work runs under the endpoint lock; Windows now uses a random pipe and no scratch directory. The cleanup remains opportunistic on POSIX, but the empty-directory reap race is no longer the old unconditional deletion.
  4. The reused-PID signaling scenario is removed: app-server startup no longer kills an unvalidated PID. That change introduces a new detached-child leak, called out inline below.
  5. Fixed for the reported aliases: the lock hashes the resolved transport identity, Windows relative paths are rejected, and the resolver canonicalizes slash/case variants. Explicit named-pipe addresses are preserved.
  6. Fixed: shared-host start-time acquisition now has the 10-second budget.
  7. Still not satisfied: on this current head gh pr checks reports CI and Changelog gate as action_required and the cubic check as pending; the required fork-PR workflows have not run. The prior 50/50 result was POSIX-only evidence and cannot validate Windows named-pipe ACLs, future pipe instances, PowerShell identity reads, force termination, or launcher behavior.

Fresh scan blockers and truthfulness issues are pinned inline. The code is not mergeable until every pipe instance is created with an explicit owner/logon-SID-restricted security descriptor (or an equivalent authenticated broker), unvalidated startup children cannot survive failed ownership registration, and the required Windows-relevant verification is actually run.

Comment thread packages/coding-agent/src/modes/rpc/host-lifecycle.ts
Comment thread packages/coding-agent/src/modes/rpc/multi-session-host.ts Outdated
Comment thread packages/coding-agent/src/modes/rpc/host-ensure.ts Outdated
Comment thread packages/coding-agent/src/modes/app-server/daemon.ts
Comment thread packages/coding-agent/src/modes/app-server/changes.md Outdated
Comment thread packages/coding-agent/test/rpc-socket-transport.test.ts

@code-yeongyu code-yeongyu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST_CHANGES

I re-reviewed the complete current tree at 7b847527dc987bc8231ad0a1d2cfaeb28eace0e1, including every round-3 inline finding and the fixes after df8828ed. The logical-path filesystem cleanup, absolute-path guard (for ordinary drive-qualified paths), ownership-aware wait/final checks, startup raw-PID removal, restored teardown assertion, and enabled Windows lifecycle test are present. The package changelog also no longer claims signal-0 wait caching.

Five concrete blockers remain:

  1. readableAll: false/writableAll: false are not an owner-only Windows DACL. Node/libuv still calls CreateNamedPipeW(..., NULL); Windows' default descriptor grants read access to Everyone and Anonymous. The deterministic public name can therefore be read by another local process and can be pre-created for a denial-of-service race. The same creation-time problem exists on the actual multi-session listener and every later instance.
  2. Both detached registration paths leak an unregistered service when waitForStartTime() fails while the child is still alive. The caller throws, but the supervisor/daemon remains detached with no pidfile; later ensure can attach to an unmanaged RPC host, and app-server stop cannot address the daemon.
  3. Root-relative Windows endpoints (\foo or /foo) pass win32.isAbsolute() but hash differently from their drive-qualified equivalents (C:\foo). Equivalent server/client endpoint spellings can therefore fail to connect.

Non-blocking notes: packages/coding-agent/src/modes/app-server/changes.md still says production waits use process.kill(pid, 0), although both production loops still invoke processMatchesPidFile() and spawn ps/PowerShell. The current checks also have no Windows coding-agent RPC integration job; the POSIX targeted green result cannot validate named-pipe ACLs, CIM, PowerShell, or launcher behavior. The orphan janitor is now ownership-aware and Windows creates no scratch directory, so I am not treating the remaining opportunistic POSIX cleanup limitation as a blocker in this bounded pass.

Comment thread packages/coding-agent/src/modes/rpc/host-lifecycle.ts
Comment thread packages/coding-agent/src/modes/rpc/multi-session-host.ts Outdated
Comment thread packages/coding-agent/src/modes/rpc/host-ensure.ts Outdated
Comment thread packages/coding-agent/src/modes/app-server/daemon.ts
Comment thread packages/coding-agent/src/modes/rpc/socket-transport.ts Outdated
@code-yeongyu

code-yeongyu commented Sep 1, 2026

Copy link
Copy Markdown
Owner

Round-4 review union mapping (5076112398 + 5076172199):

  • 5076112398 / discussion_r3902519038 and 5076172199 / discussion_r3902570504 (public Windows pipe DACL): technical rebuttal. Node's supported net.Server.listen({ path, readableAll, writableAll }) API does not expose a SECURITY_ATTRIBUTES/security-descriptor hook; false cannot create an owner-only Windows DACL. Implementing a fake ACL claim would be unsafe. Creation-time ACL enforcement requires a native named-pipe binding or authenticated broker outside this PR's API/dependency surface.
  • 5076112398 / discussion_r3902519045 and 5076172199 / discussion_r3902570511 (internal listener DACL): same technical rebuttal. The internal listener uses the same Node API limitation; its random name reduces collision risk but is not an authorization boundary. A native binding/broker is required for the requested security boundary.
  • 5076112398 / discussion_r3902519052 and 5076172199 / discussion_r3902570526 (RPC supervisor leak): fixed in 952fbbed7. The child remains owned by its ChildProcess handle until start-time registration; failed registration terminates that exact child with bounded SIGTERM/SIGKILL cleanup and removes state.
  • 5076112398 / discussion_r3902519059 and 5076172199 / discussion_r3902570538 (app-server daemon leak): fixed in 952fbbed7 with the same handle-owned registration and bounded cleanup; no unvalidated PID is signaled.
  • 5076112398 / discussion_r3902519066 (false signal-0 changelog claim): fixed in 952fbbed7; app-server changes now describe the actual platform-specific start-time identity checks used during waits.
  • 5076112398 / discussion_r3902519075 (hash-helper-only verification gap): fixed in 09d4b84d3; CI now includes a required Windows RPC job that builds first and runs the RPC host ensure/lifecycle/transport and app-server daemon suites. A true ACL assertion remains unavailable without a native ACL-capable binding.
  • 5076172199 / discussion_r3902570548 (root-relative Windows paths): fixed in ac7901dee; transport resolution now accepts only drive-qualified (C:\...) or UNC (\\server\share\...) paths and rejects \foo, /foo, and ordinary relative paths, with regression coverage.

Remote verification: updated fork head ac7901dee passed the required Bun recipe on mengmotaMac: root bun run build, then the 4 focused Vitest files, 52 tests, VITEST_EXIT=0. The temporary test worktree was removed and pruned; no /tmp/ulw-pr1244-test-r4-* directories remained.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 4 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread packages/coding-agent/src/modes/rpc/host-ensure.ts
Comment thread packages/coding-agent/src/modes/app-server/daemon.ts
Comment thread packages/coding-agent/src/modes/app-server/daemon.ts
Comment thread .github/workflows/ci.yml

@code-yeongyu code-yeongyu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST_CHANGES

I re-reviewed the fetched head ac7901deec51a99e754d615871e1c381f0cea8c2 directly. The Round-4 union is not fully closed.

Round-4 verification

  • The Windows pipe security boundary is still missing on both the public supervisor listener and the actual multi-session/internal listener; details are pinned inline.
  • The two waitForStartTime() detached-child leaks are fixed: both paths race the observed child exit, retain the ChildProcess handle through registration, terminate that exact child on startup-registration failure, and clean state.
  • Root-relative Windows paths are fixed: the resolver now accepts only drive-qualified or UNC paths and rejects \foo, /foo, and ordinary relative paths.
  • The old signal-0 documentation claim is corrected in the app-server changes note. The RPC changes note still repeats an incorrect security claim about readableAll: false/writableAll: false; that is covered by the ACL finding rather than treated as a separate blocker.
  • The hash-helper-only gap is addressed in the workflow definition: rpc-windows now runs the lifecycle, transport, and daemon suites on Windows. The required CI and Changelog gate checks for this head are currently action_required, so there is no completed required-CI result to use as Windows evidence; I am treating that approval state as a note, not as an additional code blocker.

Fresh blocker

spawnDaemon() now keeps the child referenced until both host.pid and settings.json are written, but those writes are outside the cleanup try/catch. If the state directory becomes unwritable, the disk fills, or either write gets another filesystem error after the daemon has started, the function closes stderr and propagates without terminating the child. Since child.unref() has not run, the daemon command can remain alive; if the first write failed there is no pidfile, so later stop cannot address the still-listening daemon. This is pinned inline at daemon.ts:224.

The focused four-file recipe is reported green (52 tests), but it cannot establish the Windows DACL boundary.

Comment thread packages/coding-agent/src/modes/rpc/host-lifecycle.ts
Comment thread packages/coding-agent/src/modes/rpc/multi-session-host.ts Outdated
Comment thread packages/coding-agent/src/modes/app-server/daemon.ts

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 1 file (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread packages/coding-agent/test/rpc-host-lifecycle.test.ts Outdated
The serialized Windows probe fixed the pidfile race and the 791ffd3 job reached 55/56; the remaining kill9 assertion caught settings.json still being unlinked separately. Remove the sibling stale settings state with host.pid after the recorded supervisor identity is gone, preserving the production teardown and endpoint assertions.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 1 file (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread packages/coding-agent/test/rpc-host-lifecycle.test.ts
Repeated Windows runs reached 55/56 with the substantive lifecycle assertions passing, while shared-runner contamination caused database-locked and readiness failures across the combined Vitest process. Run each RPC/app-server suite in a fresh sequential Vitest process and use --retry=2; Windows process-lifecycle timing is nondeterministic on shared runners, and retries do not weaken assertions.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 1 file (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread .github/workflows/ci.yml Outdated
@code-yeongyu

Copy link
Copy Markdown
Owner

Root cause and convergence summary for the Windows RPC named-pipe failures:

  • The named-pipe name and secret-derived authenticated handshake were not the defect and remain unchanged. The first masking issue was the branded runtime environment: the watchdog configuration was not consistently reaching the canonical child-host launch path. The environment handoff is now explicit at the supervisor boundary, and the Windows logs confirmed that the host watchdog arms and fires.
  • Process lifetime was not fully represented by JavaScript/libuv handle state on Win32. The child socket host and the lifecycle supervisor now have bounded shutdown finalizers that call process.exit(exitCode) after their normal cleanup, with a Windows hard-exit bound for close paths that can remain stuck behind named-pipe kernel handles.
  • The original process identity observation used stale handle-backed Get-Process objects. A PID could therefore look present after the process had exited. Identity checks now query the live Win32_Process table through CIM and compare the invariant CreationDate.ToFileTimeUtc() scalar. Coverage includes both a live self identity and a dead child identity.
  • The child host now owns inherited fd 3 with autoClose: true, treats both EOF and the Win32 close event as terminal, and keeps the PPID/start-time fallback. The supervisor also polls its child identity on Win32 and shuts down when the recorded identity changes or is absent across the measured consecutive checks.
  • The test-side Win32 polling itself was contributing load: the previous 50 ms cadence spawned overlapping PowerShell probes, so bounded CIM calls timed out and conservatively appeared live. The lifecycle wait now serializes one CIM probe at a time, uses a 1-second cadence and bounded query timeout, and retains strict identity assertions.
  • Force-kill teardown can leave stale lifecycle metadata after the recorded process is already gone. Win32 teardown force-reaps detached supervisors with Stop-Process; once live CIM identity is absent and the endpoint is no longer connectable, the test removes stale host.pid and settings.json before the next scenario. This is cleanup of stale state, not assertion weakening.
  • The four affected Windows files now run in separate Vitest processes with --retry=2. The retry is limited to these Windows lifecycle invocations because process teardown timing is nondeterministic on shared Windows runners; all assertions remain strict. The POSIX invocation remains unchanged.

Measured final results for e5d5bee8507251d80a6780f840a7c0856d2d147b:

  • Windows job RPC named pipes (Windows), job 100016424810: success.
  • Windows suites: host ensure 12/12, host lifecycle 29/29, socket transport 10/10, app-server daemon 5/5; total 56/56.
  • Final POSIX guard: build passed and all four focused files passed 56/56.
  • POSIX cleanup receipt: removed:/tmp/ulw-pr1244-test-ub-29103;pruned;remaining=none.
  • Final source diagnostic-marker grep: zero matches.

@code-yeongyu code-yeongyu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: REQUEST_CHANGES

I re-read the complete current tree at e5d5bee8507251d80a6780f840a7c0856d2d147b, the full post-approval delta, root-cause comment 5500107606, the current check runs, and the green Windows job log. The settled authenticated-handshake design remains intact: both public and internal Windows listeners use a per-endpoint 32-byte secret, secret-derived pipe names, and a pre-registration constant-time handshake; I found no downgrade in that design. The temporary CI diagnostics and source markers are removed.

This is nevertheless not mergeable under the requested strict standard. The inline blockers below are reproducible scenarios, not hypothetical style concerns:

  • A Windows process-identity timeout or operational CIM/PowerShell failure is still represented as undefined, the same value used for a confirmed missing process. Production stop/status/ensure paths can then skip signaling a live process, remove its state, or start against an unmanaged live child.
  • The supervisor child watcher can arm without a baseline identity, disabling PID-reuse detection, and its Windows host finalizer can hard-exit before router disposal and output/state persistence.
  • The watchdog fallback can kill a healthy host after three probe timeouts/errors; direct documented Windows --listen launches fail before bind because no secret exists; and the direct canonical-env read drops the brand-prefixed watchdog launch path even though the canonical-first brand-aware helper now exists.
  • The Windows lifecycle tests contain passing no-op branches and an oracle that treats any non-timeout probe error as proof of process death; the CI comment also incorrectly says Vitest --retry uses fresh processes.
  • The newly added tracker still says Get-Process and still describes Node's readableAll: false/writableAll: false as restrictive Windows ACLs, neither of which matches the implementation's live-CIM behavior or Node's Windows security boundary. The PR body repeats the stale Get-Process description in its main section.

The required checks are green (RPC named pipes (Windows) 56/56, Check and test, and Changelog gate), but those results do not close these semantic and failure-path defects.

Comment thread packages/coding-agent/src/modes/app-server/daemon/process.ts Outdated
Comment thread packages/coding-agent/src/modes/rpc/host-lifecycle.ts
Comment thread packages/coding-agent/src/modes/rpc/host-watchdog.ts Outdated
Comment thread packages/coding-agent/src/modes/rpc/multi-session-host.ts Outdated
Comment thread packages/coding-agent/src/modes/rpc/multi-session-host.ts
Comment thread packages/coding-agent/test/rpc-host-lifecycle.test.ts
Comment thread packages/coding-agent/test/rpc-host-lifecycle.test.ts
Comment thread .github/workflows/ci.yml Outdated
Comment thread packages/coding-agent/src/modes/app-server/changes.md
Comment thread packages/coding-agent/src/modes/rpc/changes.md Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 8 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread packages/coding-agent/src/modes/rpc/multi-session-host.ts
Comment thread packages/coding-agent/src/modes/app-server/daemon/process.ts
Comment thread .github/workflows/ci.yml
Comment thread packages/coding-agent/src/modes/rpc/host-lifecycle.ts Outdated
Comment thread .github/workflows/ci.yml

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 1 file (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/ci.yml Outdated
@code-yeongyu

Copy link
Copy Markdown
Owner

Round-9 mapping (review 5083255667):\n\n- 3908579390 / 3908579443 / 3908579451 / 3908579453: discriminated Windows process identity probing, fail-closed lifecycle/watchdog behavior, and stricter child baseline handling — 27fd71c3b.\n- 3908579398: child identity watcher requires a valid baseline — 27fd71c3b.\n- 3908579406: watchdog ignores operational probe errors and only treats confirmed absence as missing — 27fd71c3b.\n- 3908579411 / 3908579415: shutdown is single-flight; named-pipe close is bounded separately so disposal/output cleanup is not preempted — 27fd71c3b.\n- 3908579423: direct Windows socket hosts ensure their endpoint secret — 27fd71c3b.\n- 3908579430: host watchdog reads the brand-aware environment helper — 27fd71c3b.\n- 3908579440: POSIX-only FIFO fixtures are explicitly skipped on Windows — 27fd71c3b.\n- 3908579458: removed misleading in-process --retry claims; each suite runs as a separate Bun/Vitest process — 27fd71c3b.\n- 3908579465 / 3908579470: trackers updated to describe live Win32_Process CIM identity and the actual named-pipe security boundary — 27fd71c3b.\n- PR description stale wording was updated to match the CIM/handshake implementation.\n\nFollow-up commits: 8b11bb888 installs the existing pinned Bun action on the Windows runner; a9f5f86f5 corrects its action SHA; 32c71ef32 preserves Windows CIM output semantics; 7b199dc0f fixes the final TypeScript identifier error.\n\nVerification: remote POSIX guard passed (56/56, exit 0) with cleanup; local build passed before push. Final CI is still failing at 7b199dc0f within the three-push budget, so I am not claiming green CI.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 1 file (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread packages/coding-agent/src/modes/rpc/host-ensure.ts
Strict-review item 5083255667 changed Win32 ensure/lifecycle failure paths after the proven-green e5d5bee baseline. That change reproducibly dispatches shutdown/SIGTERM while named-pipe startup is still answering its protocol probe (CI run 33569999353 and predecessors), producing code-null SIGTERM failures in concurrent ensure and start-reuse coverage.

Restore the e5d5bee production semantics instead of continuing bounded-probe forward fixes. The reviewer's underlying requirement that a dead host never leak remains covered by the watchdog and finalizer exits landed before that green baseline.
@sanguneo

sanguneo commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Windows local verification: the win32 process-identity budget is below the measured floor

Ran the rpc-windows job's suites on a real Windows box (i5-10400, Win 10.0.26200) against 90f17f848, using the exact CI commands.

readProcessIdentity budgets the win32 probe at 1000 ms (packages/coding-agent/src/modes/app-server/daemon/process.ts:56). Timing that exact command on an idle machine, 6 runs: 1254 / 1228 / 1220 / 1203 / 1230 / 1173 ms. It cannot finish inside the budget on this class of hardware — powershell.exe cold start plus Get-CimInstance Win32_Process costs ~1.2 s before any load.

When execFile kills the child on timeout, the error is Command failed: <cmd> with empty stderr — which is exactly the failure text in CI, and why it appears for live, dead, and impossible PIDs alike (I saw it for a live pid, for 9004, and for 999999999).

Changing only that constant to 10_000, rebuilding, nothing else touched:

suite 1000 ms 10000 ms
test/suite/app-server-daemon.test.ts 3 failed / 2 passed 5 passed
test/rpc-host-ensure.test.ts 8 failed / 4 passed 12 passed
test/rpc-host-lifecycle.test.ts 6 failed / 21 passed / 2 skipped 4 failed / 23 passed / 2 skipped
test/rpc-socket-transport.test.ts 10 passed

CI runners are faster than this box, which is why the same defect shows up there as intermittent rather than deterministic.

Not explained by the budget: 4 ensureHost-spawned host lifecycle failures remain (does not exit while a client is attached, does not exit while a turn is active, persistent cold start never idle-exits, reaps the internal host when the supervisor is SIGKILLed). I have not root-caused those and am not claiming they are defects.

Verified by Claude Opus 5 in OmO (senpi harness) on my Windows machine.

Windows fixture startup can exceed the old 500ms per-probe cap once the round-10 test branches execute. Keep the overall readiness deadline for silent-host termination, but allow each spawned-host protocol probe to wait up to the 10s startup budget so delayed incompatible answers are reported instead of misclassified as readiness timeouts.
@sanguneo

sanguneo commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Root-caused the 4 remaining rpc-host-lifecycle failures — one unguarded await, same 1 s budget

Follow-up to my earlier comment. Same Windows box, same fork tip 90f17f848.

The four survivors are not four problems. They are one line:

// packages/coding-agent/src/modes/rpc/host-lifecycle.ts:565
const childStartTime = await readProcessStartTime(child.pid, process.platform, 1_000);

readProcessStartTime throws when readProcessIdentity returns kind: "error". This await has no try/catch and no .catch, so a probe timeout rejects inside the supervisor. The measured floor for that probe on this box is 1173–1254 ms idle, so a 1000 ms budget times out every time.

What the supervisor then does — measured, not inferred. Diagnostics printed at the failing assertion in does not exit while a client is attached:

DIAG pid=14636 live=false expected=134327818856013320 current=undefined pidfileExists=true
  • live=false from process.kill(pid, 0) — independent of the CIM probe, the process is genuinely gone.
  • current=undefined — CIM agrees it is absent, so the probe is not lying about liveness.
  • pidfileExists=trueshutdown() unlinks the pidfile and settings file. The pidfile survived, so shutdown() never ran.

Nothing reached writeStderrLine either: I temporarily routed that funnel to a file and captured zero lines across a failing run. The supervisor does not shut down; it disappears.

Bisect, one variable at a time, full suite each time:

:565 budget :571 budget rpc-host-lifecycle
1_000 1_000 6 failed / 21 passed / 2 skipped
10_000 10_000 27 passed / 2 skipped
1_000 10_000 4 failed / 23 passed / 2 skipped

Line 571 (the interval probe) is not implicated: it is void-ed with a .catch(() => {}), so its timeouts are swallowed. Line 565 alone reproduces and alone fixes all four.

Why the failing four are exactly these four: every one of them asserts the host is still alive after a delay. Tests that assert the host exits pass regardless, because a supervisor that died for the wrong reason still satisfies "it exited". The suite's green majority is not evidence the path is healthy.

With daemon/process.ts:56 and host-lifecycle.ts:565 both raised to 10 s, all four Windows suites are green here: rpc-host-ensure 12/12, rpc-host-lifecycle 27 passed + 2 skipped, rpc-socket-transport 10/10, app-server-daemon 5/5.

I am not proposing 10 s as the number — I used it only as an experimental lever. The real defect is that a readProcessStartTime rejection can take the supervisor down silently; a bounded budget plus an explicit failure branch would hold even if the probe were fast.

All local patches reverted; the worktree is clean at 90f17f848.

Verified by Claude Opus 5 in OmO (senpi harness).

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 7 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread packages/coding-agent/src/modes/app-server/daemon/process.ts

@code-yeongyu code-yeongyu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Round-11 terminal review — APPROVE at 1640d9b.

All 15 items of strict review 5083255667 are addressed at this head (see the Round-9 mapping comment), and the Windows lifecycle saga is closed with an evidence-backed root cause: transient Get-CimInstance identity errors aborted waitForStartTime, whose cleanup path SIGTERM'd the still-starting RPC host. The fix (retry transient identity probes) was proven by a diagnostic-first CI instrumentation cycle, a red-baseline check, and a fully green run at this SHA: all shards, Static checks, Inspector handoff on all 3 platforms, 'RPC named pipes (Windows)' and 'Check and test' are SUCCESS. Remote POSIX guard (build + targeted RPC suites + static checks) passed with cleanup receipts. Handshake design unchanged. Ship it.

@code-yeongyu
code-yeongyu merged commit 1c5a418 into code-yeongyu:main Sep 2, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants