Skip to content

test(cli): hoist the serve-probe child-lifecycle attribution into one helper, and pin it against a driven dead child - #15902

Draft
os-litant wants to merge 4 commits into
mainfrom
claude/issue-15653-serve-probe-child-attribution
Draft

test(cli): hoist the serve-probe child-lifecycle attribution into one helper, and pin it against a driven dead child#15902
os-litant wants to merge 4 commits into
mainfrom
claude/issue-15653-serve-probe-child-attribution

Conversation

@os-litant

@os-litant os-litant commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Fixes #15653

Three packages/cli e2e files fetched a spawned os serve with no read of the child's fate on the failing path. In each, child.on('exit') fed the READINESS promise only, so a death after readiness was invisible and the rejection reached vitest as a bare TypeError: fetch failed — no exit code, no stdout, no stderr. Each runs on Test Core, a required shard, so an occurrence is a merge-queue eviction that teaches nothing.

The population, re-derived — it is FIVE files, not four

The card's recipe, run mechanically on the merge base rather than recalled:

# A: files that spawn the CLI with the `serve` command
grep -rln "'serve'" packages/cli/test/ --include=*.ts | xargs grep -ln "spawn("
# B: of those, the ones that then make an HTTP request, target read per hit
file probes disposition
serve-node-env-production-default.e2e.test.ts 1 already repaired by #15545's PR (#15654)
serve-process-child-env.e2e.test.ts 1 repaired here
serve-mcp-stdio-answers.e2e.test.ts 2 repaired here
serve-mcp-capability-collision.e2e.test.ts 4 repaired here
serve-stdio-stdout-purity.e2e.test.ts 2 NOT touched here — handed back as #15898
serve-port-drift-notice.e2e.test.ts 1 correctly excluded: it fetches neighbour.port, the fixture it owns, never the spawned child

Two corrections to the card, both handed back rather than acted on:

The mechanism, confirmed per file before repairing

file child.on('exit') feeds readiness only? how the probes sit
serve-process-child-env yes — the handler is inside the readiness new Promise, and fail() after settle is a no-op one fetch in a try whose finally stops the child
serve-mcp-stdio-answers yes — handler guarded by if (settled) return two fetches in beforeAll, no try/finally at all; cleanup is afterAll over a children[] array
serve-mcp-capability-collision yes — same if (settled) return guard four fetch sites across beforeAll and two its, same children[] cleanup

The one difference worth naming: the card describes the family as "a fetch inside a try whose finally stops the child". That is true of serve-process-child-env only. The two MCP files have no try around their probes — which makes the missing attribution slightly worse there, not better, since the failure surfaces out of beforeAll and takes the whole file with it.

The repair — #15545's idiom, hoisted, not a second one

The landed idiom is reused verbatim in behaviour, not reinvented: the fate read, the CHILD_EXIT_SETTLE_MS wait, the three-way branch, the bound, the greppable absorbed line and every fence. What changed is where it lives. It was a private ~150-line block inside serve-node-env-production-default.e2e.test.ts with no test of its own#15654 proved it with an ad-hoc harness that was never committed, so nothing in the tree could fail if the attribution stopped working.

It now lives once, as probeThroughChild() in packages/cli/test/helpers/serve-process.ts (the module that already owns childEnv(), portContentionError(), portDriftError(), reservePort()), and all four files consume it. serve-node-env-production-default.e2e.test.ts is in the diff for that reason and that reason only — deleting its private copy is what makes the family have one definition instead of two, which is what the dispatch asked for. Its file-specific narrative stays in place; only the mechanism moved.

Two things the hoist buys that three copy-pastes could not:

  1. It is unit-testable. A private closure inside a 240-second e2e file is not; an exported function is. That is the new pin below.
  2. The body read is now inside the guard everywhere. A connection torn down mid-body rejects out of res.json() / res.text(), not out of fetch(). rpc() in the collision file used to return a bare Response with readFrame() reading it outside; it now returns { status, frame } with the read inside. Every thunk is assertion-free on purpose — the guard reads any throw as a transport failure, so an assertion inside would report a wrong answer as a dropped socket.

Proven against a DRIVEN dead child, not a mocked one

New: packages/cli/test/serve-probe-child-attribution.test.ts. Every case spawns a real child process and makes a real fetch fail on the wire. The child is a ~30-line TCP peer written to a temp dir by the test, which accepts the whole request and then FINs — the cheaper deterministic driver #15545's technique asked for, reproducing the same client-side signature without needing dist/ or a 20-second boot, and able to exit with a chosen code on command.

BEFORE (the shape being repaired, reproduced in-tree so the repair improves on a measurement rather than on a description) — an unguarded fetch against a child that dies mid-request:

message:   'fetch failed'
cause:     UND_ERR_SOCKET / other side closed
signature: bytesWritten=317 bytesRead=0

and asserted absent from what vitest would have been shown: no exit code, no child stdout marker, no child stderr marker — while settleChildFate() confirms the child really did exit 7, so the information existed to be read.

AFTER — the same driven failure, through the guard:

os serve DIED while answering the sign-in probe on port … — exit code 7, signal null.
This is the CHILD's failure, not a dropped socket, so it is NOT retried; the
transcript below is what it printed on its way down.
attempt 1/3: fetch failed [cause UND_ERR_SOCKET: other side closed] socket: bytesWritten=317 bytesRead=0 …
--- child stdout ---  probe-child stdout: mode die
--- child stderr ---  probe-child stderr: FIN without answering / exiting 7

Clean run: 7 passed (7), 8.80 s.

Controls, and the axis each discriminates on

  • HEALTHY CONTROL — a child that ANSWERS, through the same guard, must come back 200 and leave the child alive. Axis: did this harness talk to a live peer at all. Every failing case would look identical if the spawn, the port or the request were broken — this is the one that can only pass if they work. It has to exit green, so a harness that only ever produces failures cannot hide in it.
  • PROBES_EXERCISED — a counter incremented inside each request thunk, printed and asserted. Axis: did the guarded path actually run. A guard that returned early, or a rejection matcher against a promise never created, would leave it at zero. Printed non-zero on the clean run: [probe-child-attribution] probes exercised: 7, and pinned at exactly 7 (1 bare + 1 attributed + 1 healthy + 1 not-absorbed + 3 absorbed).
  • The absorb bound is measured, not asserted: the live-child case checks the counter delta equals PROBE_ATTEMPTS, so "it retried three times" is a count, not a claim.

Ablation

Prediction written before the run, mutation const fate = await settleChildFate(...) replaced by a hard-coded still-alive verdict — removing exactly the "ask the child whether it died" step.

Predicted RED (2): the AFTER case (the dead child is read as alive, so the failure is absorbed and re-sent; the retry hits a gone child, so the throw is the "does not absorb" one and exit code 7 never appears) and the probe count (toBe(7) fails at 8, one extra probe from that retry).

Predicted GREEN (5), each with its reason: BEFORE never calls the guard — staying green is what proves the driver still produces a dead child and its UND_ERR_SOCKET signature under the mutation; HEALTHY CONTROL never enters the catch branch; the not-absorbed and absorbed cases both run against a LIVE child, where fate.exited was already false, so the mutation substitutes the value it would have computed; and the settleChildFate timing pin calls the function directly, untouched.

Observed: Tests 2 failed | 5 passed (7) — the exact two, for the exact stated reasons:

AssertionError: the exit code must be named: expected '…' to contain 'exit code 7'
+ the sign-in probe on port 33905 failed with a transport error this harness does not absorb …
AssertionError: expected 8 to be 7

Mutation proven on disk, both counts, since a zero-hit edit exits 0 and reads exactly like one that changed nothing: removed-text 1 → 0, injected marker 0 → 1, blob 37deedfd… → 7b0ac939….

Source, not dist/ — proven positively, both ways. (a) packages/cli/tsconfig.build.json declares rootDir: "src" and include: ["src"], so packages/cli/test/** — the helper included — is never compiled into dist/ at all. (b) The mutation changed the pin's behaviour with no rebuild, which is itself the proof that the code running is that source file. The pin's own spawned child is a temp .mjs driver the test writes, not the CLI, so no build participates in it in either direction.

Restore baseline: the working-tree blob captured with git hash-object -w immediately before the mutation, restored with git cat-file blob under an EXIT INT TERM trap using an absolute path, and proved equal by hash (37deedfd… back, marker count 0). That baseline rather than git checkout HEAD -- … because this branch has origin/main merged in: HEAD is fine here, but the blob is correct whether or not the branch is mid-merge, and it is the exact bytes that were measured. The working tree is clean and the marker appears nowhere in it.

Gates

Gate union re-derived on the final change set with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (no paths passed; it takes its own change set from the merge base): 122 bullet lines under "Local gates for this card", counted from the bullet lines and not filtered by any pnpm check: pattern. origin/main was merged in first, so the derivation carries no STALE TREE warning.

Every verdict below was taken on the FINAL commit, 5c5bcfd71d9 — the head this PR carries. The four cheapest gates had first been run one commit earlier; they were re-run at 5c5bcfd71d9 and the union was re-derived there too (still 122), so nothing in this table describes a tree that is no longer the head.

Run locally, exit codes captured before any pipe:

gate verdict
pnpm lint (whole repo, eslint . --no-inline-config) exit 0
pnpm check:cli-test-child-env exit 0 — 61 spawn calls all declare their child's env
pnpm check:cross-package-test-inputs exit 0
pnpm check:test-source-alias exit 0
pnpm check:nul-bytes exit 0 — 7665 files, no raw control bytes
node scripts/check-comment-mask-adoption.mjs (+ self-test) exit 0
node scripts/check-comment-mask-corpus.mjs exit 0
node scripts/check-scripts-symbol-anchors.mjs exit 0
the new pin, clean exit 0 — 7 passed

check:cli-test-child-env was red first: the pin's driver spawn had no env key, so the child inherited the vitest worker environment. Repaired at the choke point (env: childEnv()) rather than baselined, and re-run green through the same entry point.

Declared narrowing. The three repaired e2e files were not run locally. Each needs a full @objectstack/cli... platform build plus real os serve boots at 180–240 s per test, and the shared verify lock was held by another agent's full packages/cli vitest suite for 28 minutes across two of my queue budgets (slot kept both times, place never lost). The load-bearing new evidence — the attribution logic itself — needs neither, which is exactly what the hoist bought, and Test Core runs all four files on this PR regardless. A standalone tsc --noEmit carrying the package's strict flags over all five edited files is clean; pnpm --filter @objectstack/cli typecheck (which names tsconfig.test.json, so it does cover test/**) was queued behind the same contention and is left to CI.

Clause ② (contract review) — declared, from the delivered diff

  • Mechanical / path limb — NO. The diff touches packages/cli/test/** and nothing else: no packages/spec/src/** path, no new key on any published payload, no change to any shipped module. packages/cli/tsconfig.build.json excludes the whole test/ tree from dist/, so nothing here is published.
  • Non-mechanizable conformance limb — NO. No input class is re-selected between two published verdicts on a shipped face. The only behavioural change is inside the test harness's failure path, and it strictly adds information to a message that was already a failure. Explicitly not a widening: the guard cannot convert a wrong answer into a pass — it absorbs transport failures only, and a 200 where a pin wants 403 is returned untouched on the first attempt.

Graded no on both limbs with the doctrine's tie-break in mind, and the grounds are stated so the call is re-judgeable if the diff ever grows past packages/cli/test/.

Publishes nothing, so skip-changeset rather than a changeset — applied as a set write over the existing labels and read back (size/xl, skip-changeset; nothing stripped).

Authored by Claude Code in session session_01D47qPfEWVPmhguWgBZCi5N (https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N), dispatched from the domain:cli execution PM seat.

Three more e2e files here fetched a spawned `os serve` with no read of the
child's fate on the failing path: `child.on('exit')` fed the READINESS promise
only, so a death after readiness reached vitest as a bare `TypeError: fetch
failed` with no exit code, no stdout and no stderr.

The idiom that repaired the fourth file is hoisted into
`test/helpers/serve-process.ts` as `probeThroughChild()` and all four files now
consume it, so the family has one definition rather than four copies — and one
pin, `serve-probe-child-attribution.test.ts`, which drives a real child that
accepts the whole request and then FINs.

No skip, no todo, no quarantine, no timeout bump.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N
…ullStreams

The pin spawns with `stdio: ['ignore', 'pipe', 'pipe']`, so the handle has no
`stdin` and the cast was a type error.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N
`check:cli-test-child-env` requires every child spawned from packages/cli/test
to declare its environment. The driver reads no variable of its own, so
`childEnv()` — the environment minus the vitest worker family — is the honest
declaration rather than an omitted `env` key.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D47qPfEWVPmhguWgBZCi5N
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

Nothing in this diff resolved to a documentable surface (no symbol, route or SDK anchor derived from 0 changed package(s)), so this run has no opinion about the docs.

What this run could not see
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 4f379125e31e47bc0fe403fde930292639de5b3cpackageMentionDocs.

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

Labels

size/xl skip-changeset PR has no user-facing published change; bypasses the changeset gate

Projects

None yet

2 participants