Uh oh!
There was an error while loading. Please reload this page.
test(cli): route every env-less spawner through childEnv(), and make the gate refuse an undeclared child env - #12530
Merged
Merged
Conversation
…childEnv() Eight spawn call sites passed no `env` option at all, so each child inherited the vitest worker's environment verbatim. Latent rather than live -- every one spawns tsx on a probe or a non-`serve` command -- but the convention's product is that a child's environment is legible at the call site, not that a particular variable happens to be stripped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd
… no env Rule 2, call-anchored: a spawn call under packages/cli/test/** must pass an `env` option. An omitted one hands the child process.env verbatim -- the same leak rule 1 refuses, in a purer form. No baseline and no carve-out: the eight sites that predated the rule were repaired in the commit before this one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjujZN219uFzBhSYfMykCd
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 was referenced Aug 26, 2026
os-litant
marked this pull request as ready for review
August 26, 2026 08:58
This was referenced Aug 26, 2026
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 26, 2026
Merged
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 freeto 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#11595
Two halves, repairs first, in one PR — as the claim ruling on the card directs.
packages/cli/test/**passed noenvoption at all, so each child gotprocess.envverbatim — under vitest that is the worker's:TEST=true,VITEST=true, theVITEST_*family. Each now passesenv: childEnv().scripts/check-cli-test-child-env.mjsgains a second, call-anchored rule: a spawn call in the population must declare anenv. No baseline and no carve-out — the eight predating sites were repaired in the commit before the one that lands the rule, precisely so no baseline had to exist.packages/cli/test/helpers/serve-process.tsis not touched (hard serial, held by #12441). The repaired files importchildEnvfrom it; four gained the import, two already had it.The population, re-derived — not taken from the card
The card's eight were measured at
2dc0a770b. Re-derived AST-first againstorigin/main@e57744536, by an independent walk (resolve everynode:child_process/node:worker_threadsbinding, including aliases, namespace imports andpromisify()wrappers; then classify every call's options object):Same eight files, no ninth site, none vanished. Two carry line drift from the rewrites the card was warned about (
cloud-login-json-ndjson.e2e.test.ts310 → 311,login-json-ndjson.e2e.test.ts298 → 299); the other six are unmoved. The extended gate independently reports the same census (35 spawner source(s) among 95 … all 40 spawn call(s) declare their child's env), derived by different code from the same tree.The ruling's falsifiable claim, tested
It holds. All six affected files, run together:
Nothing needed anything
childEnv()strips — which is the interesting half, because that strip family moved today: #12294 addedNODE_PATHto it. Three of the eight spawnnode bin/run-dev.jsfrom a tempcwd, and those resolve@oclif/coreand the CLI's own sources through the ordinarynode_moduleswalk from the entry file, never through the hoisted store. So there is no finding aboutchildEnv()'s strip family to report.Overrides were deliberately kept empty (
childEnv(), notchildEnv({ NO_COLOR: '1' })): three of these sites assert on captured CLI output, and adding an override would have made the change a behaviour change instead of a legibility one.Proving the extended gate can say no
The card's whole subject is a gate that is silent on a shape, so a second silent gate would be the same defect with a new number. Two ablation legs, both with a restore trap and both restored by blob hash, never by an exit code:
Leg A — the pre-repair tree. The six files reverted to
e57744536(mutation confirmed on disk:env: childEnv()occurrences 0). The gate exits 1 and names all eight:Leg B — a brand-new env-less spawn planted in a file that had none (blob
46eaefc8→7cbdf82a, so the mutation is proven on disk rather than assumed). Gate exits 1, namingserve-stdio-stdout-purity.e2e.test.ts:430.Restore, proven: every one of the seven files back to its
HEADblob hash —git hash-objecton the working file compared againstgit rev-parseof the same path atHEAD, non-empty on both sides — plusgit diff HEAD --statempty, planted textgrep -cback to 0, and the gate green again on the restored tree.The durable version of both legs is in the self-test, out of process, where an exit status can actually be observed:
OUT OF PROCESS: a spawn with NO env option exits NON-ZERO and names the site.What the new rule refuses, and what it deliberately does not
The rule asks only that
envbe declared — it does not namechildEnv(). That keeps it negative, like rule 1, so it does not go stale when the choke point is renamed and does not force a call that genuinely needs something else through a helper. What may not be declared is the whole ofprocess.env; that is rule 1. The pair reaches the same place from two sides, and the self-test pins that they red independently of each other.Four refused shapes, each with its own reason string and its own case:
spawn(cmd, args, { cwd })envkeyspawn(cmd, args)spawn(cmd, args, { env: undefined })spawn(cmd, args, opts)/{ ...opts }The options object is located as the last object literal among the arguments rather than by position (every roster API takes a string first, an argv array second, a callback last), so the roster can grow without a positional table rotting behind it.
promisify()wrappers resolve one level, which is every spelling in the population; that limit is named in the header rather than left to be discovered.The header's own FALSE NEGATIVE roster is updated: item 1 was this gap, and it is now closed rather than merely re-worded.
Verification — all at
4109b81f7, the final commit, on a clean treeRun through the shared heavy-verify lock (
TREE_AT=4109b81f7 dirty=[]):pnpm --filter @objectstack/cli exec vitest run(the 6 affected files)Test Files 6 passed (6) · Tests 61 passed (61)pnpm --filter @objectstack/cli typechecktsc --noEmit, exit 0 —pnpm lint(eslint . --no-inline-config, whole repo, not narrowed)pnpm check:cli-test-child-envpnpm check:type-check-coverage·check:engine-double-contract·check:where-matcher·check:query-options-erasure·check:cross-package-test-inputs·check:test-source-alias·check:parse-guard·check:entry-guard·check:agent-test-spelling·check:bash32-floor·check:pnpm-filter-targets·check:pm-dispatch-gates·scripts/pm/bare-root-worklist.mjs --self-test·check:comment-mask-adoption·check:ci-filter-parity·check:nul-bytesGate list derived with
node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstackagainst the real change set (7 paths), not from a hand-written list.pnpm --filter @objectstack/cli typechecksays nothing about the six edited files, and is reported here as NOT MEASURED rather than as a green.packages/cli/tsconfig.jsonisinclude: ["src"]with noexcludeat all, so the siblingtest/tree is read by nothing — that is precisely theTEST_DEBT['@objectstack/cli']entry (65 hidden files, 146 errors). The gate that does read it,check:type-check-debt --re-measure, rebuilds the entire workspace and runs the whole ledger; that run is CI's.Declared narrowing, in its place: a
tsc --noEmitprogram over exactly the six edited files, run on this tree and on the pre-repair tree, with--listFilesproving membership rather than assuming it:The single error is pre-existing and untouched by this change:
format-zod-union.test.ts(38,33) TS2835—'../src/utils/format'without the.jsextension, one of the 57 extension-less relative imports the ledger's own note already inventories. Repairing it would lower a shrink-only ledger, which that note says is a deliberate PR of its own; it is not folded in here.No changeset
Test files plus one repo gate script. Nothing published changes, so this PR releases nothing —
skip-changeset.POST /issues/12530/labelsanswered "GitHub access is not enabled for this session"). The set written was the read-back union of what the PR already carried —size/l,tests— plusskip-changeset; nothing was dropped.And per the card's last ruling: all eight were latent, not live. Every one spawned
tsxon a probe or a non-servecommand, so nothing in those children readTESTorVITEST. This closes a structural gap; it does not repair a measured wrong answer, and nothing here should be read as claiming it does.Generated by Claude Code