fix(tests): isolate git config injected through the environment (B01 pilot) - #421
Merged
Merged
Conversation
Closes #412. Seven test files neutralized the developer's git configuration by pointing GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM at a path that does not exist. That covers git's two FILE sources but not its third: config injected through GIT_CONFIG_COUNT / GIT_CONFIG_KEY_n / GIT_CONFIG_VALUE_n, whose origin git reports as `command line:`. A contributor whose environment routes git@github.com: traffic over HTTPS still saw the rewrite reach fixture remotes, so `resolvePublishSourceRepo records origin's fetch URL verbatim` failed on their machine while staying green on CI's bare runners. The fix is GIT_CONFIG_COUNT=0, which makes any number of injected entries inert without needing to know how many were supplied. Two things found while implementing that #412 did not describe: - The guard was NOT identical across the seven files. Five shared one block; broadside-repo-collection.test.mjs used a different temp path and a different comment, and pi-command-handlers.test.mjs differed again. - In broadside-repo-collection.test.mjs the guard ran AFTER a dynamic `await import()` of the module under test. Three lines at module scope run after every static import, so the ordering was wrong in a way that copying a third line into each file would have preserved. Both are why this extracts tests/helpers/git-config-isolation.mjs and imports it for its side effects as the FIRST import in each file: a side-effect import is ordered by the module system rather than by line position, which is the property the guard actually needs. Production code is untouched. resolvePublishSourceRepo, sameSourceRepo and `git remote get-url` keep their semantics; reading raw config instead of git's effective remote would change recorded provenance for every publish, which is a product decision and not what #412 asks for. The helper only ever assigns to process.env of its own process. Verified: 7/7 new isolation tests, 1006/1006 full suite under BOTH a clean environment and the injected rewrite, build exit 0. Two mutation checks bite: removing GIT_CONFIG_COUNT=0 fails 5 of 7, and moving the isolation import out of first position fails the ordering test.
Independent review (B01-A6) found the fix incomplete: git reads configuration from FOUR environment-reachable sources, and this neutralized three. GIT_CONFIG_PARAMETERS is how git hands `-c key=value` down to the subprocesses it spawns, so it arrives without anyone setting it deliberately — running the suite under `git bisect run` is enough. It carries its own entries, so GIT_CONFIG_COUNT=0 does not disarm it. On the previous commit the original #412 failure reproduced byte-for-byte through this vector: env GIT_CONFIG_PARAMETERS="'url.https://github.com/.insteadOf'='git@github.com:'" \ node --experimental-strip-types --test tests/library.test.mjs not ok 37 - resolvePublishSourceRepo records origin's fetch URL verbatim Three corrections, two of them to my own work: - The helper now deletes GIT_CONFIG_PARAMETERS, with a regression test that first proves the vector is real (the other three neutralized, this one left alone, rewrite still reaches the fixture) and then proves it is closed. - The A5 test asserted the helper sets EXACTLY three variables, so it failed the moment a fourth source had to be covered — a test that resists widening the isolation it exists to protect. It now checks an allow-list: everything touched must be a GIT_CONFIG* variable, and all four must be present. Its 'runs no commands' assertion also scanned comments, so documenting `git config` broke it; it now scans code only. - The previous commit message claimed the old guard position in broadside-repo-collection.test.mjs was a realized defect. The reviewer checked out that file and ran it under injection: 10/10 passed, because core/broadside.ts makes no git call during module evaluation. Corrected in both comments to a latent hazard removed, not a bug fixed. A comment claiming git errors on an empty GIT_CONFIG_PARAMETERS was also wrong — an empty value reads as no entries. Verified directly and corrected; delete is kept because the variable has no business being there at all. Verified: 8/8 isolation tests, 214/214 across the seven sites under BOTH the COUNT and PARAMETERS vectors, 1007/1007 full suite clean and under PARAMETERS, build exit 0. Removing the delete fails 2 tests.
Member
Author
B01-A6 review: REQUEST_CHANGES → addressed in
|
| Check | Result |
|---|---|
| Isolation suite | 8/8 |
Seven sites, GIT_CONFIG_COUNT vector |
214/214 |
Seven sites, GIT_CONFIG_PARAMETERS vector |
214/214 |
| Full suite, clean | 1007/1007 |
Full suite, under GIT_CONFIG_PARAMETERS |
1007/1007 |
| Build | exit 0 |
Mutation checks: removing GIT_CONFIG_COUNT = "0" fails 5 tests; removing delete env.GIT_CONFIG_PARAMETERS fails 2; moving the isolation import out of first position fails the ordering test.
test-windows failed on d631c4a with ERR_UNSUPPORTED_ESM_URL_SCHEME: Only URLs with a scheme in: file, data, and node are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'd:' The four child-process probes interpolated REPO_ROOT straight into an `import` statement. On Linux an absolute path happens to resolve; on Windows `D:\a\CodeCartographer\...` is read as a URL with scheme `d:`. The repo already uses pathToFileURL everywhere else for exactly this reason — the new file was the one place that did not. Verified: 8/8 isolation tests, 1007/1007 full suite under the PARAMETERS vector. No remaining bare-path interpolation in a generated probe.
This was referenced Sep 20, 2026
Closed
TheAmericanMaker
added a commit
that referenced
this pull request
Sep 21, 2026
Records the first real change run through the engineering contract's preregistered acceptance checks. Bug fixed and merged as c4030a4 (#421, closing #412); all six checks satisfied. States plainly that this was a host agent executing the contract's checks, NOT a CodeCartographer engine running a change: E02 storage and E05/E06 evidence collection do not exist, so no record was minted, no digest bound, no acceptance classified. Also reports that #412 carried a public diagnosis, making this a known-answer protocol test rather than evidence of unaided diagnosis. Key finding: A6's mandated independent review found a blocking defect in a change with green CI whose author believed it complete. Two claims made during implementation are corrected rather than left standing. Gaps filed as #422 and #423.
TheAmericanMaker
added a commit
that referenced
this pull request
Sep 22, 2026
#442) * Fixture git commands see nothing of the developer's environment (#423) What: tests/helpers/git-config-isolation.mjs becomes tests/helpers/git-environment-isolation.mjs (git mv; all nine importers and the test file follow). The side-effect import now applies isolateGitEnvironment(), which keeps the four configuration sources from #412/#421 and adds the rest of the class: GIT_TEMPLATE_DIR (deleted), the repository-location overrides GIT_DIR, GIT_WORK_TREE, GIT_INDEX_FILE, GIT_OBJECT_DIRECTORY, GIT_ALTERNATE_OBJECT_DIRECTORIES, GIT_COMMON_DIR, GIT_NAMESPACE, GIT_CEILING_DIRECTORIES (deleted), the programs git would launch GIT_PAGER, GIT_ASKPASS, GIT_SSH, GIT_SSH_COMMAND (deleted), and GIT_EDITOR / GIT_SEQUENCE_EDITOR = ":" with GIT_TERMINAL_PROMPT=0. isolateGitConfig and ABSENT_GIT_CONFIG keep their names. No core/ change. Why: the helper was named for configuration and covered exactly that, but GIT_TEMPLATE_DIR is not configuration and git copies it, hooks/ included, into every `git init` — so a developer's executable hooks/pre-commit ran inside every fixture commit under "full" isolation. The issue's Option 2: name the helper for what it guarantees so the next member of the class is easier to see, and sweep the class now. Class sweep (git 2.55, `git help git` ENVIRONMENT): neutralize: GIT_TEMPLATE_DIR (proven: failing pre-commit breaks fixture commit); GIT_DIR (proven: `git -C fixture init` + `remote add` land in the exported repo, fixture never gets a .git); GIT_INDEX_FILE (proven: `git add` writes the exported index, fixture status shows ??); GIT_EDITOR (proven: `commit --amend` runs the editor and fails with it); GIT_WORK_TREE / GIT_OBJECT_DIRECTORY / GIT_ALTERNATE_* / GIT_COMMON_DIR / GIT_NAMESPACE / GIT_CEILING_DIRECTORIES (same mechanism as GIT_DIR: redirect the repository a command acts on); GIT_SEQUENCE_EDITOR / GIT_PAGER / GIT_ASKPASS / GIT_SSH(_COMMAND) / GIT_TERMINAL_PROMPT (programs and prompts; fixtures never want one). leave: GIT_AUTHOR_* / GIT_COMMITTER_* (only change identity/date, no test asserts on them, CI sets them deliberately); GIT_TRACE*, GIT_FLUSH, GIT_ADVICE, GIT_PROGRESS_DELAY (stderr diagnostics, no outcome change, wanted when debugging); GIT_DEFAULT_HASH / GIT_DEFAULT_REF_FORMAT (on-disk format the developer chose for every new repo; suite should pass under it); GIT_EXEC_PATH / PATH / HOME (which git runs is out of scope); GIT_EXTERNAL_DIFF / GIT_DIFF_OPTS (porcelain-only; fixtures use plumbing flags). No test in tests/ sets any neutralized variable deliberately (grep). Tests (tests/git-environment-isolation.test.mjs, renamed with the helper because it exercises the whole boundary, not configuration): four new cases in the #421 shape — each first proves the vector is real under configuration-only isolation, then runs a fresh child with the variable injected and the helper imported first. The allow-list test widens from ^GIT_CONFIG to ^GIT_ and requires the new members. Against the main helper: 5/12 fail (6-9 and 12); the TEMPLATE_DIR control shows `template hook ran` on the fixture commit's stderr. Mutation: removing each neutralization line alone fails its own test plus the allow-list (6+12, 7+12, 8+12, 9+12); removing the location loop fails 7+8; applying isolateGitConfig() instead of isolateGitEnvironment() at import fails 6-9. No survivors. npm run build: 0 TS errors. npm test: 1276/1276 twice (was 1272). git diff --check clean. Developer's global git config: 7 entries before and after; the helper writes only process.env. Closes #423 * test: the allow-list guard scans code, not prose; the editor control names its symptom (#442 review) Review found the allow-list test's variable scan ran over the whole source, comments included, so a helper that neutralized nothing passed as long as the names survived in a comment. It now scans the same comment-stripped code the command check already used; the reviewer's probe (every assignment commented out) fails test 12 as it should. The GIT_EDITOR control also asserts the editor was the failure.
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
1 participant
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.
Closes #412. This is also the B01 pilot from
docs/engineering/pilot-selection.md— the first run of a real bug through the engineering contract's acceptance checks.The defect
Git takes configuration from three independent sources. The guard in seven test files neutralized two:
GIT_CONFIG_SYSTEMGIT_CONFIG_GLOBALGIT_CONFIG_COUNT/_KEY_n/_VALUE_nThe first two are file lookups, so a path that does not exist reads as empty config. The third is not a file — git reports its origin as
command line:— so redirecting file lookups cannot reach it. A contributor whose environment carriesurl.https://github.com/.insteadOf = git@github.com:sawresolvePublishSourceRepo records origin's fetch URL verbatimfail on a tree that is green on CI's bare runners.Two things #412 did not describe
Found by inspecting the seven sites rather than trusting the issue's summary:
The guard was not identical across the files. Five shared one block;
broadside-repo-collection.test.mjsused a different temp path (cc-no-such-gitconfig) and a different comment;pi-command-handlers.test.mjsdiffered again. The issue said "identical comment block in each."In
broadside-repo-collection.test.mjsthe guard ran after a dynamicawait import()of the module under test. In ESM every static import is evaluated before the importing module's body, so three lines at module scope run after the code under test is imported. Copying a third line into each file would have preserved that ordering bug.Hence the shared
tests/helpers/git-config-isolation.mjs, imported for side effects as the first import in each file: a side-effect import is ordered by the module system rather than by line position, which is the property the guard actually needs.Acceptance checks (preregistered,
pilot-selection.md)not ok 37 - resolvePublishSourceRepo records origin's fetch URL verbatim, 71/72process.envof its own processVerification
Two mutation checks bite: removing
GIT_CONFIG_COUNT = "0"fails 5 of 7 tests; moving the isolation import out of first position fails the ordering test.The suite includes a negative control asserting the rewrite genuinely reaches git when only the file lookups are redirected — without it, every other assertion could pass because the rewrite never worked.
One implementation note:
node --testrefuses to recurse (run() is being called recursively within a test file), so the A2 assertion runslibrary.test.mjsdirectly and keys on the child's exit code.Production is untouched. Reading raw config instead of git's effective remote would change recorded provenance for every publish — a product decision, not what #412 asks for.