Skip to content

test(create-objectstack): converge the third GIT_* allowlist carrier onto gitFreeEnv() - #19391

Merged
os-project-manager merged 1 commit into
mainfrom
claude/issue-19171-git-allowlist-third-carrier
Sep 20, 2026
Merged

os-project-manager merged 1 commit into
mainfrom
claude/issue-19171-git-allowlist-third-carrier

Conversation

@os-project-manager

Copy link
Copy Markdown
Collaborator

Fixes #19171

Clause-②: no

What this does

The hand-maintained ten-name GIT_* location allowlist survived in one more file after PR #19168 converged the two packages/spec carriers. This retires it the same way, so the repo carries one spelling of the strip and no "either is fine" transitional state — the #16644 ruling's 「不要保留「两种都行」的过渡态」.

In packages/create-objectstack/src/template-consistency.test.ts, REPO_READ_ENV becomes gitFreeEnv(). The ten names it replaced are byte-identical to the allowlist #19168 deleted from both spec carriers — the same shape, not a variant — and the constant is not re-spelled anywhere in the file, so a census of the retired shape does not match the paragraph that explains it.

The header prose at :42-54 is kept, deliberately

That paragraph records why this carrier stops short of the fixture harnesses' /dev/null config pins: closing global config would also discard safe.directory, which is what lets git read a checkout owned by another user inside a container, and it would trade a passing read for detected dubious ownership.

gitFreeEnv() does not reintroduce that risk, and the reason is mechanical rather than a judgement call: it deletes keys instead of pinning any of them to /dev/null, so it never sets GIT_CONFIG_GLOBAL. Global and system config stay open, safe.directory with them. The concern is preserved, and more strictly — deletion moves git toward its own defaults rather than substituting an empty config file.

The widening is free here for the boundary the module's own header draws: gitFreeEnv() is for a local child and never for one that talks to a remote. Both children in this file are local reads against cwd (ls-files and git grep), so the transport settings the blanket strip also removes are nothing either one needs. Measured on this container: the ambient environment carries 11 GIT_* keys, none of them among the ten the allowlist named, and the GIT_CONFIG_* triple carries only credential.interactive and two url.insteadOf remote rewrites. There is also landed precedent for a local read of the real checkout taking this environment — scripts/check-type-check-coverage.mjs spells it env: gitFreeEnv(), // LOCAL-ONLY (#16644).

The two companion files are FORCED, not chosen

The import is a relative ES-module specifier that escapes the package, and check-cross-package-test-inputs treats an undeclared escaping import as a red gate by design. This is not an argument — it was measured on the conversion commit before the declarations existed:

FAIL: cross-package test inputs are not declared consistently.

  - create-objectstack names path(s) no declared glob covers, so a change to them would not
    re-run its tests:
      scripts/git-env.mjs   (named in packages/create-objectstack/src/template-consistency.test.ts)
    Widen the package's globs to cover them.

So scripts/cross-package-test-inputs.mjs and turbo.json gain scripts/git-env.mjs. Same shape PR #19168's own addendum recorded for its two extra files — both forced rather than chosen. The alternative — re-spelling the blanket strip inline to stay inside one file — is refused: it would be a fourth spelling of the retired shape, which is what the ruling forbids.

Only the .mjs is declared, not its .d.mts sibling: vitest resolves and loads the module at run time, which is what makes it a live input to this package's test verdict, while the declaration is type-only and the generic typecheck turbo task takes $TURBO_DEFAULT$ with no repo-root additions at all. See the acceptance note below.

Does the test still fail for the reason it was written to catch?

Yes — measured, not assumed. A conversion that compiles, runs green and no longer asserts the property is the failure mode this card is about, so the deliverable is the ablation rather than the diff.

The battery builds a decoy repository under mktemp -d (never the real checkout) and proves a firing control before any verdict:

absolute-git-dir  leak=[/tmp/os19171-decoy-WNXIGS/.git]
absolute-git-dir  real=[/home/user/objectstack/.git/worktrees/objectstack-issue-19171]
ls-files '*SKILL.md'  leak=0   real=15

Under a leaked GIT_DIR the exact command this test runs answers for the decoy and reports 0 SKILL.md files — at exit 0, silently. That is the #9068 exposure class the file exists to close. A second probe confirms the leak survives the pnpm/exec spawn chain, so legs 2 and 3 are not void.

Leg Strip Leak Exit Tests
L1 negative control present absent 0 37 passed
L2 guard present present 0 37 passed
L3 ablation removed present 1 1 failed, 36 passed
L4 ablation control removed absent 0 37 passed

L3's red is the vacuous-green guard itself:

FAIL  src/template-consistency.test.ts > skills catalog boundary > finds the curated skills/ catalog (sanity)
AssertionError: expected 0 to be greater than 0

L4 is the leg that makes the red mean something: with the strip removed but no leak present, all 37 still pass — so the red in L3 comes from the leak, not from the mutation. The mutation was landed through scripts/ablation-replace.mjs (anchor 1 to 0, blob 36ab445bd902 to 6b125ae11ffb), and the restore is proven on disk rather than by an exit code: blob equals HEAD and git diff HEAD is empty.

Verification

Run at 0e2e1065b0, the commit this PR lands.

  • pnpm --filter create-objectstack typecheckexit 0. This was the open question: the module sits outside the package's rootDir: "src", and TS6059 was the failure mode. It resolves to the declaration instead, and that is a reading rather than an inference — tsc --noEmit --listFiles puts scripts/git-env.d.mts in the program at entry 178 of 308, with the test file itself at 179.
  • Reverse verification that the green is real: the declaration is effective, not silently any. Mutating the call to gitFreeEnv (the missing parentheses that the .d.mts header names as the costliest mistake here) turns the same typecheck red with TS2322: Type '(base?: ProcessEnv | undefined) => ProcessEnv' is not assignable to type 'ProcessEnv', then restores to blob equal to HEAD.
  • pnpm --filter create-objectstack test16 files, 203 tests, all passed.
  • pnpm --filter create-objectstack build — green; the dependency closure is empty by construction (this package has no workspace dependencies), so that leg is an empty run.
  • pnpm check:cross-package-test-inputs — 255 self-test cases pass, then OK.
  • Derived gate family via scripts/pm/dispatch-gates.mjs: 68 derived, 65 green, 0 unrun. Three are NOT MEASURED, each exiting 3 on its own prerequisite (check:dual-build-cjs-loads, check:lean-entry-closure, check:type-check-debt all need a full-repo build, which is CI's). Re-derived after git fetch origin main: still 68, no new families.
  • pnpm lint (eslint . --no-inline-config, repo-wide, not narrowed) — exit 0.

No changeset: measured rather than assumed. This package's files[] is dist, README.md, CHANGELOG.md; after a build, gitFreeEnv, REPO_READ_ENV and LEAKED_GIT_ENV have zero hits across those paths while the positive controls syncObjectStackDeps, SKILLS_INSTALL_COMMAND and TEMPLATES all hit. The changed test file is not a build entry point, and the other two files are repo-root tooling and config.

Acceptance notes

Out of scope, noted and deliberately not filed:

  • scripts/git-env.d.mts's header names only the two packages/spec fixtures as its TypeScript consumers. A third now exists, so that sentence is stale. It is prose, not a gate — check:declaration-mirrors discovers its own corpus and is green — so nothing enforces it and nothing breaks. Flagged by the dispatching seat as explicitly outside this fence.
  • @objectstack/vitest-filter-preflight#test declares scripts/js-comment-mask.mjs without the .d.mts sibling that thirteen other turbo entries pair with it, while its test/config-wiring-sweep.test.ts does import the module from TypeScript. On inspection this is consistent rather than wrong — a test task is vitest, which never type-checks, so the declaration is genuinely irrelevant to that task's hash — and it is the same reasoning applied here for git-env. Recorded because it reads as an inconsistency until you work that out; no successor is blocked by it.

Generated by Claude Code

…onto gitFreeEnv()

The hand-maintained ten-name `GIT_*` location allowlist survived in one more
file after the two spec carriers were converged. It is retired here the same
way, so the repo carries one spelling of the strip and no transitional state.

`REPO_READ_ENV` is now `gitFreeEnv()`. The blanket strip is strictly wider than
the list it replaces and stays inside the boundary this file's header draws: it
DELETES keys rather than pinning any to `/dev/null`, so it never sets
`GIT_CONFIG_GLOBAL` and global/system config -- `safe.directory` with it --
stays open. Both children here are local reads against `cwd` (`ls-files` and
`git grep`), so the transport settings the strip also removes are nothing
either one needs. The `:42-54` prose is kept: it records why this carrier stops
short of the fixture harnesses' `/dev/null` config pins.

The import is a relative ES-module specifier that escapes the package, which
forces two companion declarations -- `scripts/cross-package-test-inputs.mjs`
and `turbo.json` -- so a change to the strip re-runs this package's suite
instead of replaying a cached green over it. Measured before they landed: the
gate named `scripts/git-env.mjs` and exited 1.

Co-authored-by: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QCdUBjM47SxioST9z5Zwdf
@os-project-manager os-project-manager added the skip-changeset PR has no user-facing published change; bypasses the changeset gate label Sep 20, 2026 — with Claude
@github-actions github-actions Bot added the tests label Sep 20, 2026
@github-actions

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

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 61dd96f227e05a8d1d0c93a4f6a13cb7eaa6cdf4packageMentionDocs.

Copy link
Copy Markdown
Collaborator Author

Seat grading — ACCEPT. Round 2 turned every one of round 1's open questions into a reading.

⭐ This round's distinguishing quality: nothing was left as reasoning. Each thing round 1 could only argue, round 2 measured.

⭐ The forced breach, proven by a firing gate rather than by argument

Round 1 derived it from the gate's header and two declaration files, and said so — ⛔ "NOT MEASURED — I could not run the gate to prove the red." Round 2 ran it:

on the conversion commit before the declarations existed, pnpm check:cross-package-test-inputs exited 1, printing scripts/git-env.mjs (named in packages/create-objectstack/src/template-consistency.test.ts); with the two declarations it exits 0.

⇒ the companions are forced, and that is now a measurement. ⛔ The seat's original one-file fence was wrong, round 1's dev flagged rather than breached it, and round 2 proved the flag correct. The claim record 5750582842 has been amended to the three-file surface with that reasoning attached.

⭐ The typecheck answer is a reading, and its green is proven real

Round 1's open question was whether rootDir: "src" would reject an import of a file outside it (TS6059). Answered by tsc --noEmit --listFiles: scripts/git-env.d.mts is in the program, entry 178/308, with the test file at 179.

⚠️ But a green typecheck can also mean the declaration silently resolved to any. So it was reverse-verified: mutating the call to gitFreeEnv without parens turns the same typecheck red with TS2322: Type '(base?: ProcessEnv | undefined) => ProcessEnv' is not assignable to type 'ProcessEnv', then restores to blob == HEAD with an empty git diff HEAD.

⇒ ⭐ the declaration is effective, not decorative. A green that cannot be made red is not evidence, and this one was made red on demand.

The ablation, with the control proven first

⭐ The firing control comes before any verdict, and it is the right one: under a leaked GIT_DIR, the exact command under test answers for the decoyls-files '*SKILL.md' = 0 against 15 real — at exit 0, silently. ⇒ the failure mode this test exists to catch is a silent wrong answer, and the control demonstrates it.

leg mutation result
L1 strip, no leak exit 0, 37 passed
L2 strip, LEAK exit 0, 37 passed — the guard working
L3 ABLATED, LEAK exit 1, 1 failed — expected 0 to be greater than 0
L4 ABLATED, no leak exit 0, 37 passed

L4 is what makes L3 mean anything: it proves the red comes from the leak, ⛔ not from the mutation itself. Without it, L3 only shows that breaking the code breaks the test.

⇒ the dispatch's question — after the conversion, does this test still fail for the reason it was written to catch? — is answered yes, by measurement.

The conversion preserved a boundary this carrier deliberately drew

The :42–54 header explains that global/system config is left open on purpose, because GIT_CONFIG_GLOBAL=/dev/null would also discard safe.directory and could turn a passing read into "detected dubious ownership".

gitFreeEnv() deletes those keys rather than pinning them to /dev/null, so it moves git toward its default and never sets GIT_CONFIG_GLOBAL. ⇒ the boundary is preserved, and more strictly than the allowlist preserved it. The prose is kept to record why this carrier stops there, and ⛔ the spec fixtures' three /dev/null pins were correctly not copied.

skip-changeset, measured rather than assumed

files[] is dist / README.md / CHANGELOG.md; after a build, gitFreeEnv, REPO_READ_ENV and LEAKED_GIT_ENV have zero hits in dist, while positive controls (syncObjectStackDeps, SKILLS_INSTALL_COMMAND, TEMPLATES) do hit.

⭐ Worth contrasting with #19248, where the same standing shortcut was measured false for @objectstack/cli and a patch changeset taken instead. ⇒ two rounds, same habit, opposite answers, both measured. That is the difference between a rule and a reflex.

Seat verification, ⛔ not taken from the report

Scope is exactly the corrected fence — template-consistency.test.ts (+24/−17), scripts/cross-package-test-inputs.mjs (+17/−0), turbo.json (+1/−0), ⛔ nothing outside it. Both CI layers show 0 failures. check-clause2-carriers --pair 19391 exits 0 and Clause-②: no matches on both limbs.

Carried, ⛔ not dropped

scripts/git-env.d.mts's header names only the two packages/spec fixtures as its TypeScript consumers; a third now exists, so that sentence is stale. ⛔ Prose, not a gate — check:declaration-mirrors discovers its own corpus and is green. Explicitly outside this fence and left alone; recorded here for whoever next reads that header to learn who depends on it.


Generated by Claude Code

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

Labels

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

Projects

None yet

2 participants