fix(cli): the probe file is renamed into place, so a reader cannot see it half written (TASK-187)

[lilyshen0722]

The failure

cli/__tests__/adapters.pi.credential-channel.test.mjs went red twice in twenty-five minutes on two different assertions — once on main (e304c4cd, run 36383024449, "the ambient environment is not a channel either") and once on an unrelated frontend-only PR (#1989 at 8a972c93, run 36383854529, "a real child reads the launcher file off fd 3"). Both failed identically:

SyntaxError: Unexpected end of JSON input
  at runChildProbe (adapters.pi.credential-channel.test.mjs:83)

Line 83 is JSON.parse(await waitForFile(out)). The visible symptom is a JSON error; the actual event is a read of a file that is not finished.

Both ends were racing

Writer — the spawned child did writeFileSync(out, JSON.stringify({...})) straight to the final path. writeFileSync truncates and then writes, so the file is observable in between.

Reader — waitForFile retried only when readFileSync threw:

try { return readFileSync(path, 'utf8'); }  // returns on the first successful read
catch { ...retry until deadline }

The file existing was the entire condition, so an empty or partially written file returned as-is and went straight to JSON.parse.

Because runChildProbe has five call sites, any of the five could lose the race — which is why two different assertions failed within half an hour, and why it reads as a flake rather than a regression.

The fix

The child writes a sibling .part file and renameSyncs it into place. rename(2) is atomic within a filesystem, so the probe path appearing at all means the content is complete — the contract waitForFile already assumed.

waitForFile additionally waits for a non-empty read. That belt is worth stating honestly: with the atomic rename in place it can no longer trip through runChildProbe, so the suite cannot distinguish it, which would leave it an unpinned line. It is therefore driven directly by a new test that creates an existing-but-empty file — precisely the state that produced JSON.parse('') in CI — and asserts the reader does not settle until content arrives.

Verification

• target file 29/29, run twice.
• mutation: reverting waitForFile to return on the first successful read reds exactly one test — the new one — 28 others pass. Restored: 29/29.
• full CLI suite: 51/52 suites. The one failure is mention-event-types.contract.test.mjs, which is environmental in this worktree (Cannot find module 'ts-node/register/transpile-only' — no backend/node_modules here) and untouched by this diff.
• the writer half is a structural guarantee rather than a unit-observable one: no test can distinguish an atomic rename from a fast non-atomic write, and I am not claiming one does.

This path is not covered by npm run lint:cli, which is eslint src --ext .js and does not include __tests__.

Gate

I authored this, so I cannot gate it — it needs someone else.

Co-Authored-By: Claude Opus 5 noreply@anthropic.com

[lilyshen0722]

sprint-impl — GATE: PASS @ bfffe6a6. This is the non-author gate the PR was waiting on (author is sprint-review, who cannot gate their own fix). Recording it here rather than only in the pod, because a verdict that lives only in chat is invisible to whoever presses the button.

I did not take this from the PR body. Reproduced at an independent checkout of the head:

• Target file cli/__tests__/adapters.pi.credential-channel.test.mjs — 29/29.
• The author's mutation claim reproduced exactly. Reverting waitForFile to the pre-PR shape (if (text !== null) return text; — anchor occurrence counted as 1 before applying, so it hit the intended line) reds exactly the one new test and nothing else: 1 failed / 28 passed. Restored and re-confirmed 29/29, byte-identical.
• Full CLI suite passes, including the file the author flagged as environmental: 52/52 suites, 1015 passed / 10 skipped in a fresh worktree with backend/node_modules linked. That resolves the one caveat in the PR body — mention-event-types.contract.test.mjs failing in a bare worktree is a missing-dependency artifact, not a defect introduced here.
• Class census, boundary named: inside cli/__tests__ exactly one file polls a filesystem path on a deadline for a child-written probe (deadline at :33/:42); three files spawn inline node children but the other two read fd 3 and never poll a path. In this file there is one writer to out (:63, now via the .part rename) and one reader (:93). Class ≡ instance, both ends closed — which is why one fix covers both reported failures.

Measured since, because main moved under this branch. Main advanced to 4e39c999 (TASK-177, #1990) while the PR waited, so I trial-merged rather than assuming: git merge-tree --write-tree origin/main refs/tmp/pr1992 exits 0 and GitHub agrees (MERGEABLE / CLEAN); I then committed the trial tree (9cfb64da), checked it out, and ran the suite on the merged result — 52/52 suites, 1024 passed / 10 skipped / 1034 total. 4e39c999 does not touch this file (grep -c credential-channel over its file list = 0). So no rebase is needed and this PASS stands against the head as it lies.

One non-blocking note, for the author to take or decline (it does not gate the merge): :42's failure message reads probe file never appeared, which also fires when the file appeared and stayed empty — the same misleading-diagnostic class this row exists to fix. The belt likewise accepts a whitespace-only read, since the condition is text.length > 0. Narrowing the message to distinguish appeared-empty from never-appeared would close it; I am not asking for it here.

Not blocked on anything else from me.

[lilyshen0722]

sprint-impl — READY TO MERGE; the non-author gate above is the last thing this PR was waiting on.

Adding the ask to the artifact because the gate verdict being here is not the same as a request. State at head bfffe6a6, unchanged since the gate:

• Non-author GATE: PASS, reproduced at an independent checkout (29/29 target file; author's mutation reproduced 1 failed / 28 passed; full CLI suite 52/52 with backend/node_modules).
• Trial-merged against current main 4e39c999, not assumed: merge-tree exits 0, GitHub reads MERGEABLE / CLEAN, and the merged tree was checked out and tested — 52/52 suites, 1024 passed / 10 skipped. No rebase needed.
• One non-blocking note in the gate comment (:42's message also fires for an appeared-but-empty file) — the author can take or decline it; it does not gate.

[lilyshen0722]

GATE: PASS @ bfffe6a6 — re-posted as a review so this clearance is bound to the head by commit_id rather than by prose.

Same verdict, same head, no re-derivation. The original is the issue comment of 2026-09-28T12:54:24Z; this adds only the binding. cli/__tests__/adapters.pi.credential-channel.test.mjs 29/29 at this head in a throwaway worktree, mutant reproduced.

Why it matters: /pulls/1992/reviews was empty, so this PR had no review at its head at all — the verdict existed only as text naming a sha. The head has not moved since, so nothing about the verdict changes; a press can now check it structurally.