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
Draft
Conversation
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
…rve-probe-child-attribution
…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
Contributor
📓 Docs Drift CheckNothing 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
Coarse fallback — 0 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #15653
Three
packages/clie2e files fetched a spawnedos servewith 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 bareTypeError: fetch failed— no exit code, no stdout, no stderr. Each runs onTest 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:
serve-node-env-production-default.e2e.test.tsserve-process-child-env.e2e.test.tsserve-mcp-stdio-answers.e2e.test.tsserve-mcp-capability-collision.e2e.test.tsserve-stdio-stdout-purity.e2e.test.tsserve-port-drift-notice.e2e.test.tsneighbour.port, the fixture it owns, never the spawned childTwo corrections to the card, both handed back rather than acted on:
serve-stdio-stdout-purity.e2e.test.tsgenuinely spawnsos serve(spawn(process.execPath, [CLI, 'serve', '-p', port, '--dev'])) and fetches it twice, with the identical unguarded shape. The dispatch ruling pins this PR's population at three, so it is reported, not swept in — it isserve-stdio-stdout-purity.e2e.test.tsis a FIFTH file that fetches a spawnedos servewith no child-lifecycle attribution — the family is five, and the unrepaired population in #15653 was four, not three #15898, and it is a one-line-per-probe change once this helper exists.fetchsites, not three. The card's recipe isgrep "await fetch("; the fourth is spelledreturn fetch(inside therpc()helper, which is the most-exercised one in the file (called twice).The mechanism, confirmed per file before repairing
child.on('exit')feeds readiness only?serve-process-child-envnew Promise, andfail()after settle is a no-opfetchin atrywhosefinallystops the childserve-mcp-stdio-answersif (settled) returnfetches inbeforeAll, notry/finallyat all; cleanup isafterAllover achildren[]arrayserve-mcp-capability-collisionif (settled) returnguardfetchsites acrossbeforeAlland twoits, samechildren[]cleanupThe one difference worth naming: the card describes the family as "a
fetchinside atrywhosefinallystops the child". That is true ofserve-process-child-envonly. The two MCP files have notryaround their probes — which makes the missing attribution slightly worse there, not better, since the failure surfaces out ofbeforeAlland 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_MSwait, 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 insideserve-node-env-production-default.e2e.test.tswith 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()inpackages/cli/test/helpers/serve-process.ts(the module that already ownschildEnv(),portContentionError(),portDriftError(),reservePort()), and all four files consume it.serve-node-env-production-default.e2e.test.tsis 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:
res.json()/res.text(), not out offetch().rpc()in the collision file used to return a bareResponsewithreadFrame()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 realfetchfail 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 needingdist/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
fetchagainst a child that dies mid-request:and asserted absent from what vitest would have been shown: no
exit code, no child stdout marker, no child stderr marker — whilesettleChildFate()confirms the child really did exit 7, so the information existed to be read.AFTER — the same driven failure, through the guard:
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 back200and 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).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 7never 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_SOCKETsignature under the mutation; HEALTHY CONTROL never enters the catch branch; the not-absorbed and absorbed cases both run against a LIVE child, wherefate.exitedwas already false, so the mutation substitutes the value it would have computed; and thesettleChildFatetiming pin calls the function directly, untouched.Observed:
Tests 2 failed | 5 passed (7)— the exact two, for the exact stated reasons: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 marker0 → 1, blob37deedfd… → 7b0ac939….Source, not
dist/— proven positively, both ways. (a)packages/cli/tsconfig.build.jsondeclaresrootDir: "src"andinclude: ["src"], sopackages/cli/test/**— the helper included — is never compiled intodist/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.mjsdriver 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 -wimmediately before the mutation, restored withgit cat-file blobunder anEXIT INT TERMtrap using an absolute path, and proved equal by hash (37deedfd…back, marker count 0). That baseline rather thangit checkout HEAD -- …because this branch hasorigin/mainmerged in:HEADis 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 anypnpm check:pattern.origin/mainwas 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 at5c5bcfd71d9and 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:
pnpm lint(whole repo,eslint . --no-inline-config)pnpm check:cli-test-child-envpnpm check:cross-package-test-inputspnpm check:test-source-aliaspnpm check:nul-bytesnode scripts/check-comment-mask-adoption.mjs(+ self-test)node scripts/check-comment-mask-corpus.mjsnode scripts/check-scripts-symbol-anchors.mjscheck:cli-test-child-envwas red first: the pin's driver spawn had noenvkey, 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 realos serveboots at 180–240 s per test, and the shared verify lock was held by another agent's fullpackages/clivitest 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, andTest Coreruns all four files on this PR regardless. A standalonetsc --noEmitcarrying the package's strict flags over all five edited files is clean;pnpm --filter @objectstack/cli typecheck(which namestsconfig.test.json, so it does covertest/**) was queued behind the same contention and is left to CI.Clause ② (contract review) — declared, from the delivered diff
packages/cli/test/**and nothing else: nopackages/spec/src/**path, no new key on any published payload, no change to any shipped module.packages/cli/tsconfig.build.jsonexcludes the wholetest/tree fromdist/, so nothing here is published.200where a pin wants403is returned untouched on the first attempt.Graded
noon 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 pastpackages/cli/test/.Publishes nothing, so
skip-changesetrather 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 thedomain:cliexecution PM seat.