From 15fee9e5fbdabcd8b04781163c18d7f4e82bbca7 Mon Sep 17 00:00:00 2001 From: James Sesler Date: Sun, 20 Sep 2026 15:13:57 -0400 Subject: [PATCH 1/3] fix(tests): isolate git config injected through the environment 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. --- tests/broadside-repo-collection.test.mjs | 6 +- tests/broadside.test.mjs | 14 +- tests/git-config-isolation.test.mjs | 211 +++++++++++++++++++++++ tests/helpers/git-config-isolation.mjs | 64 +++++++ tests/library.test.mjs | 13 +- tests/mcp-uncovered-handlers.test.mjs | 5 +- tests/pi-broadside.test.mjs | 13 +- tests/pi-command-handlers.test.mjs | 6 +- tests/pi-publish.test.mjs | 13 +- 9 files changed, 282 insertions(+), 63 deletions(-) create mode 100644 tests/git-config-isolation.test.mjs create mode 100644 tests/helpers/git-config-isolation.mjs diff --git a/tests/broadside-repo-collection.test.mjs b/tests/broadside-repo-collection.test.mjs index b047dc1..ed2e817 100644 --- a/tests/broadside-repo-collection.test.mjs +++ b/tests/broadside-repo-collection.test.mjs @@ -12,6 +12,7 @@ // refused before pricing, and the manifests present name candidates // that the source-file counts decide between. +import "./helpers/git-config-isolation.mjs"; import { test } from "node:test"; import assert from "node:assert/strict"; import { execFile } from "node:child_process"; @@ -36,11 +37,6 @@ const { statusText, } = await import(pathToFileURL(`${REPO_ROOT}/core/broadside.ts`).href); -// Git fixtures must not inherit the developer's global config (see -// release-cycle notes): a `url.insteadOf` or `commit.gpgsign` breaks them. -process.env.GIT_CONFIG_GLOBAL = join(tmpdir(), "cc-no-such-gitconfig"); -process.env.GIT_CONFIG_SYSTEM = join(tmpdir(), "cc-no-such-gitconfig"); - async function git(dir, ...args) { await execFileAsync("git", ["-C", dir, ...args], { maxBuffer: 16 * 1024 * 1024 }); } diff --git a/tests/broadside.test.mjs b/tests/broadside.test.mjs index 54efc71..0839c91 100644 --- a/tests/broadside.test.mjs +++ b/tests/broadside.test.mjs @@ -3,6 +3,7 @@ // in-memory. The real OpenRouter API is covered by a manual smoke path, not // this suite. +import "./helpers/git-config-isolation.mjs"; import { test } from "node:test"; import assert from "node:assert/strict"; import { mkdtemp, mkdir, readdir, readFile, rm, writeFile } from "node:fs/promises"; @@ -10,18 +11,6 @@ import { tmpdir } from "node:os"; import { basename, dirname, join, resolve } from "node:path"; import { fileURLToPath, pathToFileURL } from "node:url"; -// Git fixtures must not inherit the developer's git configuration. A global -// `url..insteadOf` rewrites what `git remote get-url` reports — which is -// exactly what resolvePublishSourceRepo reads — so a verbatim-URL assertion -// fails on any machine carrying that common setting while staying green on -// CI's bare runners. `commit.gpgsign` and `init.defaultBranch` reach the -// committing fixtures the same way. Point both config layers at a path that -// does not exist: git reads a missing file as empty config. Identity is set -// per fixture in repo-local config, so commits still work. -const ABSENT_GIT_CONFIG = join(tmpdir(), "codecarto-tests-absent-gitconfig"); -process.env.GIT_CONFIG_GLOBAL = ABSENT_GIT_CONFIG; -process.env.GIT_CONFIG_SYSTEM = ABSENT_GIT_CONFIG; - const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), ".."); const core = await import(pathToFileURL(join(REPO_ROOT, "core/index.ts")).href); @@ -1902,7 +1891,6 @@ test("saveLensResults marks truncated content and writes parsed JSON cleanly", a } }); - // ---------- outcomes reported for non-completed batches ---------- // // Leads from the second live Broad-Side scan of this repository, each verified diff --git a/tests/git-config-isolation.test.mjs b/tests/git-config-isolation.test.mjs new file mode 100644 index 0000000..7c04f2f --- /dev/null +++ b/tests/git-config-isolation.test.mjs @@ -0,0 +1,211 @@ +// Git-configuration isolation in the test harness (#412, pilot B01). +// +// Seven test files build real Git fixtures and must not inherit the +// developer's Git configuration. The guard they carried pointed +// GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM at a path that does not exist, +// which neutralizes the two FILE sources — but git has a third source, +// injected through GIT_CONFIG_COUNT / GIT_CONFIG_KEY_n / GIT_CONFIG_VALUE_n, +// whose origin git reports as `command line:`. Redirecting file lookups +// cannot reach it, so a contributor with +// +// url.https://github.com/.insteadOf = git@github.com: +// +// in their environment saw `resolvePublishSourceRepo records origin's fetch +// URL verbatim` fail on a tree that is green on CI's bare runners. +// +// These tests exercise the isolation boundary itself. Every case runs the +// real thing in a FRESH CHILD PROCESS with the rewrite injected into that +// child's environment, because the boundary is about what a process inherits +// at startup: asserting on `process.env` in this process, or letting an +// earlier test's cleanup set the stage, would manufacture a pass. +// +// The injected variables never leave the child. Nothing here reads or writes +// the user's Git configuration. + +import { execFile } from "node:child_process"; +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { dirname, join, resolve } from "node:path"; +import { fileURLToPath } from "node:url"; +import { promisify } from "node:util"; +import test from "node:test"; +import assert from "node:assert/strict"; + +const execFileAsync = promisify(execFile); +const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), ".."); + +/** The rewrite that reproduces #412: SSH-syntax remotes routed over HTTPS. */ +const INJECTED = { + GIT_CONFIG_COUNT: "1", + GIT_CONFIG_KEY_0: "url.https://github.com/.insteadOf", + GIT_CONFIG_VALUE_0: "git@github.com:", +}; + +/** The seven files that build Git fixtures and carry the guard. */ +const GUARDED_FILES = [ + "tests/broadside-repo-collection.test.mjs", + "tests/broadside.test.mjs", + "tests/library.test.mjs", + "tests/mcp-uncovered-handlers.test.mjs", + "tests/pi-broadside.test.mjs", + "tests/pi-command-handlers.test.mjs", + "tests/pi-publish.test.mjs", +]; + +async function withTempDir(fn) { + const dir = await mkdtemp(join(tmpdir(), "codecarto-gitconfig-")); + try { + return await fn(dir); + } finally { + await rm(dir, { recursive: true, force: true }); + } +} + +/** Run a script in a fresh child process with `env` merged over the parent's. */ +async function runChild(source, env) { + return await withTempDir(async (dir) => { + const file = join(dir, "probe.mjs"); + await writeFile(file, source, "utf8"); + const { stdout } = await execFileAsync(process.execPath, ["--experimental-strip-types", "--disable-warning=ExperimentalWarning", file], { + env: { ...process.env, ...env }, + cwd: REPO_ROOT, + }); + return stdout.trim(); + }); +} + +test("the injected rewrite really does reach git — the defect is real", async () => { + // The negative control. Without this, every assertion below could pass + // because the rewrite never worked in the first place. + await withTempDir(async (dir) => { + await execFileAsync("git", ["-C", dir, "init", "--quiet"]); + await execFileAsync("git", ["-C", dir, "remote", "add", "origin", "git@github.com:Acme/Tool.git"]); + const { stdout } = await execFileAsync("git", ["-C", dir, "remote", "get-url", "origin"], { + env: { ...process.env, ...INJECTED, GIT_CONFIG_GLOBAL: "/nonexistent", GIT_CONFIG_SYSTEM: "/nonexistent" }, + }); + assert.equal( + stdout.trim(), + "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/Acme/Tool.git", + "with only the file lookups redirected, the injected rewrite still applies — this is #412", + ); + }); +}); + +test("the helper neutralizes the injected rewrite in a fresh child process", async () => { + const out = await runChild( + `import "${REPO_ROOT}/tests/helpers/git-config-isolation.mjs"; + import { execFile } from "node:child_process"; + import { mkdtemp } from "node:fs/promises"; + import { tmpdir } from "node:os"; + import { join } from "node:path"; + import { promisify } from "node:util"; + const run = promisify(execFile); + const dir = await mkdtemp(join(tmpdir(), "probe-")); + await run("git", ["-C", dir, "init", "--quiet"]); + await run("git", ["-C", dir, "remote", "add", "origin", "git@github.com:Acme/Tool.git"]); + const { stdout } = await run("git", ["-C", dir, "remote", "get-url", "origin"]); + process.stdout.write(stdout.trim());`, + INJECTED, + ); + assert.equal(out, "git@github.com:Acme/Tool.git", "the SCP-syntax remote is preserved verbatim"); +}); + +test("isolation survives a large injected config set, not just one entry", async () => { + // GIT_CONFIG_COUNT=0 makes every KEY_n/VALUE_n inert without needing to + // know how many were supplied. Deleting them one at a time would not. + const many = { GIT_CONFIG_COUNT: "3" }; + for (const [i, [k, v]] of [ + ["url.https://github.com/.insteadOf", "git@github.com:"], + ["core.autocrlf", "true"], + ["commit.gpgsign", "true"], + ].entries()) { + many[`GIT_CONFIG_KEY_${i}`] = k; + many[`GIT_CONFIG_VALUE_${i}`] = v; + } + const out = await runChild( + `import "${REPO_ROOT}/tests/helpers/git-config-isolation.mjs"; + import { execFile } from "node:child_process"; + import { promisify } from "node:util"; + const run = promisify(execFile); + const { stdout } = await run("git", ["config", "--list", "--show-origin"]); + process.stdout.write(String(stdout.split("\\n").filter((l) => l.startsWith("command line:")).length));`, + many, + ); + assert.equal(out, "0", "no configuration is still arriving from the command line"); +}); + +test("the helper runs before the modules under test, not merely at module scope", async () => { + // The ordering property the old guard lacked. 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 has been imported — and in + // broadside-repo-collection.test.mjs the guard sat after a dynamic + // `await import()` of that very code. A module that reads the rewrite at + // import time therefore saw it. Here the probe imports the isolation + // first, so the observer module must see a clean environment. + const out = await runChild( + `import "${REPO_ROOT}/tests/helpers/git-config-isolation.mjs"; + import { execFile } from "node:child_process"; + import { promisify } from "node:util"; + const run = promisify(execFile); + // Simulates a module that resolves git config during its own evaluation. + const { stdout } = await run("git", ["config", "--get", "url.https://github.com/.insteadOf"]).catch(() => ({ stdout: "" })); + process.stdout.write(stdout.trim() === "" ? "clean" : "leaked:" + stdout.trim());`, + INJECTED, + ); + assert.equal(out, "clean", "the rewrite is gone before anything else is imported"); +}); + +test("every guarded file imports the isolation helper first", async () => { + // One passing file is insufficient (B01-A3): assert the boundary is + // applied at all seven sites, and that it is genuinely FIRST — the defect + // in broadside-repo-collection.test.mjs was position, not absence. + const { readFile } = await import("node:fs/promises"); + for (const relative of GUARDED_FILES) { + const source = await readFile(join(REPO_ROOT, relative), "utf8"); + const firstImport = source.match(/^import .*$/m); + assert.ok(firstImport, `${relative}: expected at least one import`); + assert.equal( + firstImport[0], + 'import "./helpers/git-config-isolation.mjs";', + `${relative}: the isolation import must be the first import in the file`, + ); + assert.ok( + !/process\.env\.GIT_CONFIG_(GLOBAL|SYSTEM)\s*=/.test(source), + `${relative}: the open-coded guard should be gone, replaced by the shared helper`, + ); + } +}); + +test("the named failing test passes with the rewrite injected into its runner", async () => { + // B01-A2 as an assertion rather than a claim: the regression suite passes + // while its PARENT still supplies the rewrite. Removing the injection + // from the command would not count. + // + // `node --test` refuses to recurse ("run() is being called recursively + // within a test file. skipping running files"), so the child runs the + // file DIRECTLY. `node:test` still executes and reports on exit, and a + // failing assertion still yields a non-zero exit code — which is the + // signal this case needs. + const child = await execFileAsync( + process.execPath, + ["--experimental-strip-types", "--disable-warning=ExperimentalWarning", "tests/library.test.mjs"], + { env: { ...process.env, ...INJECTED }, cwd: REPO_ROOT, maxBuffer: 32 * 1024 * 1024 }, + ).catch((error) => error); + const output = `${child.stdout ?? ""}${child.stderr ?? ""}`; + assert.ok(!(child instanceof Error), `library.test.mjs must pass under injection; exit ${child.code}\n${output.slice(-1500)}`); + assert.match(output, /resolvePublishSourceRepo records origin's fetch URL verbatim/, "the named test really ran"); + assert.ok(!/^not ok/m.test(output), "no test failed"); +}); + +test("isolation does not touch the user's Git configuration", async () => { + // B01-A5. The helper only ever assigns to process.env of its own process. + const { readFile } = await import("node:fs/promises"); + const source = await readFile(join(REPO_ROOT, "tests/helpers/git-config-isolation.mjs"), "utf8"); + assert.ok(!/execFile|spawn|writeFile|git config/.test(source), "the helper runs no commands and writes no files"); + const assignments = [...source.matchAll(/env\.[A-Z_]+\s*=/g)].map((m) => m[0]); + assert.deepEqual( + assignments.sort(), + ["env.GIT_CONFIG_COUNT =", "env.GIT_CONFIG_GLOBAL =", "env.GIT_CONFIG_SYSTEM ="].sort(), + "it sets exactly the three Git configuration sources and nothing else", + ); +}); diff --git a/tests/helpers/git-config-isolation.mjs b/tests/helpers/git-config-isolation.mjs new file mode 100644 index 0000000..f304546 --- /dev/null +++ b/tests/helpers/git-config-isolation.mjs @@ -0,0 +1,64 @@ +// Git-configuration isolation for tests that build real Git fixtures. +// +// Import this FIRST, for its side effects, before any module that shells out +// to git: +// +// import "./helpers/git-config-isolation.mjs"; +// +// Git takes configuration from three independent sources, and neutralizing +// two of them is not isolation: +// +// 1. the system config file -> GIT_CONFIG_SYSTEM +// 2. the global config file -> GIT_CONFIG_GLOBAL +// 3. config injected through the environment +// -> GIT_CONFIG_COUNT / _KEY_n / _VALUE_n +// +// The first two are FILE lookups, so pointing them at a path that does not +// exist makes git read them as empty. The third is not a file at all — git +// reports its origin as `command line:` — so redirecting file lookups cannot +// reach it. A contributor whose environment supplies, say, +// +// url.https://github.com/.insteadOf = git@github.com: +// +// (a common setup that routes SSH-syntax traffic over HTTPS) would still see +// that rewrite applied to fixture remotes, and +// `resolvePublishSourceRepo records origin's fetch URL verbatim` would fail +// on their machine while staying green on CI's bare runners. Setting +// GIT_CONFIG_COUNT=0 tells git there are zero injected entries, which +// neutralizes the whole GIT_CONFIG_KEY_n/VALUE_n set regardless of how many +// were supplied. +// +// Why a module rather than three lines in each test file: this must run +// before the code under test is imported. In ESM, static imports are +// evaluated before the importing module's own body, so three lines at module +// scope run AFTER every static import — and in one file they sat after a +// dynamic `await import()` of the module under test. A side-effect import +// placed first is ordered by the module system instead of by line position, +// which is the property the guard actually needs. +// +// This only ever writes to `process.env` of the current process. It does not +// read, write, or modify the user's Git configuration. + +import { join } from "node:path"; +import { tmpdir } from "node:os"; + +/** A path git will read as empty config. It is never created. */ +export const ABSENT_GIT_CONFIG = join(tmpdir(), "codecarto-tests-absent-gitconfig"); + +/** + * Neutralize all three Git configuration sources for this process. + * + * Idempotent, and safe to call from a child process that inherited an + * injected environment: the point is to overwrite what was inherited. + */ +export function isolateGitConfig(env = process.env) { + env.GIT_CONFIG_GLOBAL = ABSENT_GIT_CONFIG; + env.GIT_CONFIG_SYSTEM = ABSENT_GIT_CONFIG; + // Zero injected entries. Git stops reading GIT_CONFIG_KEY_n/VALUE_n + // entirely, so any number of already-set pairs become inert; deleting + // them one by one would require knowing the original count. + env.GIT_CONFIG_COUNT = "0"; + return env; +} + +isolateGitConfig(); diff --git a/tests/library.test.mjs b/tests/library.test.mjs index ca8bbd9..fd27540 100644 --- a/tests/library.test.mjs +++ b/tests/library.test.mjs @@ -16,6 +16,7 @@ // gate describes a publish with (#162), and the git-remote resolver the Pi // command records source_repo from (#147). +import "./helpers/git-config-isolation.mjs"; import { test } from "node:test"; import assert from "node:assert/strict"; import { execFile } from "node:child_process"; @@ -25,18 +26,6 @@ import { basename, dirname, join, resolve } from "node:path"; import { fileURLToPath, pathToFileURL } from "node:url"; import { promisify } from "node:util"; -// Git fixtures must not inherit the developer's git configuration. A global -// `url..insteadOf` rewrites what `git remote get-url` reports — which is -// exactly what resolvePublishSourceRepo reads — so a verbatim-URL assertion -// fails on any machine carrying that common setting while staying green on -// CI's bare runners. `commit.gpgsign` and `init.defaultBranch` reach the -// committing fixtures the same way. Point both config layers at a path that -// does not exist: git reads a missing file as empty config. Identity is set -// per fixture in repo-local config, so commits still work. -const ABSENT_GIT_CONFIG = join(tmpdir(), "codecarto-tests-absent-gitconfig"); -process.env.GIT_CONFIG_GLOBAL = ABSENT_GIT_CONFIG; -process.env.GIT_CONFIG_SYSTEM = ABSENT_GIT_CONFIG; - const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), ".."); const lib = await import(pathToFileURL(`${REPO_ROOT}/core/library.ts`).href); const core = await import(pathToFileURL(`${REPO_ROOT}/core/index.ts`).href); diff --git a/tests/mcp-uncovered-handlers.test.mjs b/tests/mcp-uncovered-handlers.test.mjs index f0c4663..89502b5 100644 --- a/tests/mcp-uncovered-handlers.test.mjs +++ b/tests/mcp-uncovered-handlers.test.mjs @@ -8,6 +8,7 @@ // // Everything here runs offline. No model, no network, no API key. +import "./helpers/git-config-isolation.mjs"; import { test } from "node:test"; import assert from "node:assert/strict"; import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; @@ -15,10 +16,6 @@ import { tmpdir } from "node:os"; import { dirname, join, resolve } from "node:path"; import { fileURLToPath, pathToFileURL } from "node:url"; -const ABSENT_GIT_CONFIG = join(tmpdir(), "codecarto-tests-absent-gitconfig"); -process.env.GIT_CONFIG_GLOBAL = ABSENT_GIT_CONFIG; -process.env.GIT_CONFIG_SYSTEM = ABSENT_GIT_CONFIG; - const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), ".."); const { handleInit, diff --git a/tests/pi-broadside.test.mjs b/tests/pi-broadside.test.mjs index c2f0d9e..346500d 100644 --- a/tests/pi-broadside.test.mjs +++ b/tests/pi-broadside.test.mjs @@ -4,6 +4,7 @@ // asks a human about the money instead of refusing over max_cost, and it runs // on a repository with no CodeCartographer workspace. +import "./helpers/git-config-isolation.mjs"; import { test } from "node:test"; import assert from "node:assert/strict"; import { execFile } from "node:child_process"; @@ -13,18 +14,6 @@ import { promisify } from "node:util"; import { dirname, join, resolve } from "node:path"; import { fileURLToPath, pathToFileURL } from "node:url"; -// Git fixtures must not inherit the developer's git configuration. A global -// `url..insteadOf` rewrites what `git remote get-url` reports — which is -// exactly what resolvePublishSourceRepo reads — so a verbatim-URL assertion -// fails on any machine carrying that common setting while staying green on -// CI's bare runners. `commit.gpgsign` and `init.defaultBranch` reach the -// committing fixtures the same way. Point both config layers at a path that -// does not exist: git reads a missing file as empty config. Identity is set -// per fixture in repo-local config, so commits still work. -const ABSENT_GIT_CONFIG = join(tmpdir(), "codecarto-tests-absent-gitconfig"); -process.env.GIT_CONFIG_GLOBAL = ABSENT_GIT_CONFIG; -process.env.GIT_CONFIG_SYSTEM = ABSENT_GIT_CONFIG; - const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), ".."); const { default: codeCartographerExtension } = await import( pathToFileURL(`${REPO_ROOT}/extensions/codecarto/index.ts`).href diff --git a/tests/pi-command-handlers.test.mjs b/tests/pi-command-handlers.test.mjs index 8343323..4172980 100644 --- a/tests/pi-command-handlers.test.mjs +++ b/tests/pi-command-handlers.test.mjs @@ -13,6 +13,7 @@ // contract — refuse cleanly, report through the UI, never throw — is fully // observable here. +import "./helpers/git-config-isolation.mjs"; import { test } from "node:test"; import assert from "node:assert/strict"; import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; @@ -20,11 +21,6 @@ import { tmpdir } from "node:os"; import { dirname, join, resolve } from "node:path"; import { fileURLToPath, pathToFileURL } from "node:url"; -// Keep git out of the user's real config, as the sibling fixtures do. -const ABSENT_GIT_CONFIG = join(tmpdir(), "codecarto-tests-absent-gitconfig"); -process.env.GIT_CONFIG_GLOBAL = ABSENT_GIT_CONFIG; -process.env.GIT_CONFIG_SYSTEM = ABSENT_GIT_CONFIG; - const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), ".."); const { default: codeCartographerExtension } = await import(pathToFileURL(`${REPO_ROOT}/extensions/codecarto/index.ts`).href); const { getWorkspaceState } = await import(pathToFileURL(`${REPO_ROOT}/core/index.ts`).href); diff --git a/tests/pi-publish.test.mjs b/tests/pi-publish.test.mjs index 45fa618..59cc8b0 100644 --- a/tests/pi-publish.test.mjs +++ b/tests/pi-publish.test.mjs @@ -7,6 +7,7 @@ // upgraded Pi trips it on its own, when an entry recorded under the old // directory shape meets a publish carrying the new remote shape. +import "./helpers/git-config-isolation.mjs"; import { test } from "node:test"; import assert from "node:assert/strict"; import { execFile } from "node:child_process"; @@ -16,18 +17,6 @@ import { promisify } from "node:util"; import { dirname, join, resolve } from "node:path"; import { fileURLToPath, pathToFileURL } from "node:url"; -// Git fixtures must not inherit the developer's git configuration. A global -// `url..insteadOf` rewrites what `git remote get-url` reports — which is -// exactly what resolvePublishSourceRepo reads — so a verbatim-URL assertion -// fails on any machine carrying that common setting while staying green on -// CI's bare runners. `commit.gpgsign` and `init.defaultBranch` reach the -// committing fixtures the same way. Point both config layers at a path that -// does not exist: git reads a missing file as empty config. Identity is set -// per fixture in repo-local config, so commits still work. -const ABSENT_GIT_CONFIG = join(tmpdir(), "codecarto-tests-absent-gitconfig"); -process.env.GIT_CONFIG_GLOBAL = ABSENT_GIT_CONFIG; -process.env.GIT_CONFIG_SYSTEM = ABSENT_GIT_CONFIG; - const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), ".."); const { default: codeCartographerExtension } = await import( pathToFileURL(`${REPO_ROOT}/extensions/codecarto/index.ts`).href From d631c4a4fe8bce6d21ec7a79253402372cf3c5bd Mon Sep 17 00:00:00 2001 From: James Sesler Date: Sun, 20 Sep 2026 15:28:02 -0400 Subject: [PATCH 2/3] fix(tests): neutralize GIT_CONFIG_PARAMETERS, git's fourth config source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- tests/git-config-isolation.test.mjs | 78 ++++++++++++++++++++++---- tests/helpers/git-config-isolation.mjs | 33 ++++++++--- 2 files changed, 94 insertions(+), 17 deletions(-) diff --git a/tests/git-config-isolation.test.mjs b/tests/git-config-isolation.test.mjs index 7c04f2f..f08a6ab 100644 --- a/tests/git-config-isolation.test.mjs +++ b/tests/git-config-isolation.test.mjs @@ -34,6 +34,9 @@ import assert from "node:assert/strict"; const execFileAsync = promisify(execFile); const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), ".."); +/** A path git reads as empty config. Matches the helper's own constant. */ +const ABSENT = join(tmpdir(), "codecarto-tests-absent-gitconfig"); + /** The rewrite that reproduces #412: SSH-syntax remotes routed over HTTPS. */ const INJECTED = { GIT_CONFIG_COUNT: "1", @@ -139,9 +142,11 @@ test("the helper runs before the modules under test, not merely at module scope" // is evaluated before the importing module's body, so three lines at // module scope run AFTER the code under test has been imported — and in // broadside-repo-collection.test.mjs the guard sat after a dynamic - // `await import()` of that very code. A module that reads the rewrite at - // import time therefore saw it. Here the probe imports the isolation - // first, so the observer module must see a clean environment. + // `await import()` of that very code. No module under test currently + // resolves git configuration while it is being evaluated, so that was a + // latent hazard rather than a live failure — but it is the kind that + // appears silently the first time one does. Here the probe imports the + // isolation first, so the observer must see a clean environment. const out = await runChild( `import "${REPO_ROOT}/tests/helpers/git-config-isolation.mjs"; import { execFile } from "node:child_process"; @@ -155,6 +160,44 @@ test("the helper runs before the modules under test, not merely at module scope" assert.equal(out, "clean", "the rewrite is gone before anything else is imported"); }); +test("GIT_CONFIG_PARAMETERS is neutralized, not merely counted down", async () => { + // The fourth source, and the one the first version of this fix missed. + // Git uses it to hand `-c key=value` to its OWN subprocesses, 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. + const injected = { GIT_CONFIG_PARAMETERS: "'url.https://github.com/.insteadOf'='git@github.com:'" }; + + // First prove the vector is real: with the other three neutralized but + // this one left alone, the rewrite still reaches a fixture. + await withTempDir(async (dir) => { + await execFileAsync("git", ["-C", dir, "init", "--quiet"]); + await execFileAsync("git", ["-C", dir, "remote", "add", "origin", "git@github.com:Acme/Tool.git"]); + const { stdout } = await execFileAsync("git", ["-C", dir, "remote", "get-url", "origin"], { + env: { ...process.env, ...injected, GIT_CONFIG_GLOBAL: ABSENT, GIT_CONFIG_SYSTEM: ABSENT, GIT_CONFIG_COUNT: "0" }, + }); + assert.equal(stdout.trim(), "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/Acme/Tool.git", "the vector is real: COUNT=0 does not disarm it"); + }); + + // Then prove the helper closes it. + const out = await runChild( + `import "${REPO_ROOT}/tests/helpers/git-config-isolation.mjs"; + import { execFile } from "node:child_process"; + import { mkdtemp } from "node:fs/promises"; + import { tmpdir } from "node:os"; + import { join } from "node:path"; + import { promisify } from "node:util"; + const run = promisify(execFile); + const dir = await mkdtemp(join(tmpdir(), "probe-")); + await run("git", ["-C", dir, "init", "--quiet"]); + await run("git", ["-C", dir, "remote", "add", "origin", "git@github.com:Acme/Tool.git"]); + const { stdout } = await run("git", ["-C", dir, "remote", "get-url", "origin"]); + process.stdout.write(stdout.trim());`, + injected, + ); + assert.equal(out, "git@github.com:Acme/Tool.git", "the SCP-syntax remote survives GIT_CONFIG_PARAMETERS"); +}); + test("every guarded file imports the isolation helper first", async () => { // One passing file is insufficient (B01-A3): assert the boundary is // applied at all seven sites, and that it is genuinely FIRST — the defect @@ -201,11 +244,26 @@ test("isolation does not touch the user's Git configuration", async () => { // B01-A5. The helper only ever assigns to process.env of its own process. const { readFile } = await import("node:fs/promises"); const source = await readFile(join(REPO_ROOT, "tests/helpers/git-config-isolation.mjs"), "utf8"); - assert.ok(!/execFile|spawn|writeFile|git config/.test(source), "the helper runs no commands and writes no files"); - const assignments = [...source.matchAll(/env\.[A-Z_]+\s*=/g)].map((m) => m[0]); - assert.deepEqual( - assignments.sort(), - ["env.GIT_CONFIG_COUNT =", "env.GIT_CONFIG_GLOBAL =", "env.GIT_CONFIG_SYSTEM ="].sort(), - "it sets exactly the three Git configuration sources and nothing else", - ); + // Scan the CODE, not the prose: the comments legitimately discuss + // `git config` and the variables git reads, and an assertion that greps + // the whole file fails the moment the documentation improves. + const code = source + .split("\n") + .filter((line) => !line.trim().startsWith("//") && !line.trim().startsWith("*") && !line.trim().startsWith("/*")) + .join("\n"); + assert.ok(!/execFile|spawn|writeFile|appendFile|"git"|'git'/.test(code), "the helper runs no commands and writes no files"); + // An allow-list, not an exact set: the point is that the helper touches + // NOTHING outside git's own configuration variables. Pinning the exact + // triple made this test fail the moment a fourth source ( + // GIT_CONFIG_PARAMETERS) had to be neutralized — a test that resists + // widening the isolation it exists to protect. + const touched = [...source.matchAll(/(?:delete\s+)?env\.([A-Z_]+)/g)].map((m) => m[1]); + assert.ok(touched.length > 0, "the helper must touch something"); + for (const name of touched) { + assert.match(name, /^GIT_CONFIG(_[A-Z]+)?$/, `${name} is outside git's configuration variables`); + } + // And every source git reads from the environment is actually covered. + for (const required of ["GIT_CONFIG_GLOBAL", "GIT_CONFIG_SYSTEM", "GIT_CONFIG_COUNT", "GIT_CONFIG_PARAMETERS"]) { + assert.ok(touched.includes(required), `${required} must be neutralized`); + } }); diff --git a/tests/helpers/git-config-isolation.mjs b/tests/helpers/git-config-isolation.mjs index f304546..d8d3b46 100644 --- a/tests/helpers/git-config-isolation.mjs +++ b/tests/helpers/git-config-isolation.mjs @@ -5,13 +5,15 @@ // // import "./helpers/git-config-isolation.mjs"; // -// Git takes configuration from three independent sources, and neutralizing -// two of them is not isolation: +// Git takes configuration from four environment-reachable sources, and +// neutralizing some of them is not isolation: // // 1. the system config file -> GIT_CONFIG_SYSTEM // 2. the global config file -> GIT_CONFIG_GLOBAL // 3. config injected through the environment // -> GIT_CONFIG_COUNT / _KEY_n / _VALUE_n +// 4. config git passes to its own subprocesses +// -> GIT_CONFIG_PARAMETERS // // The first two are FILE lookups, so pointing them at a path that does not // exist makes git read them as empty. The third is not a file at all — git @@ -28,13 +30,25 @@ // neutralizes the whole GIT_CONFIG_KEY_n/VALUE_n set regardless of how many // were supplied. // +// The fourth is how git hands `-c key=value` down to the subprocesses it +// spawns, so it arrives without anyone setting it deliberately: run the suite +// under `git -c … `, or inside `git bisect run`, and every fixture +// command inherits it. It carries its own count, so GIT_CONFIG_COUNT=0 does +// not disarm it. Deleting it and setting it to "" both work — git reads an +// empty string as no entries — but deleting is what the name of the +// operation should say: the variable has no business being in the +// environment of a fixture command at all. +// // Why a module rather than three lines in each test file: this must run // before the code under test is imported. In ESM, static imports are -// evaluated before the importing module's own body, so three lines at module +// evaluated before the importing module's body, so three lines at module // scope run AFTER every static import — and in one file they sat after a -// dynamic `await import()` of the module under test. A side-effect import -// placed first is ordered by the module system instead of by line position, -// which is the property the guard actually needs. +// dynamic `await import()` of the module under test. That did not break +// anything at the time (the imported module makes no git call while it is +// being evaluated), so it was a latent hazard rather than a realized +// defect. A side-effect import placed first is ordered by the module system +// instead of by line position, which is the property the guard needs if a +// module under test ever does resolve git configuration at import time. // // This only ever writes to `process.env` of the current process. It does not // read, write, or modify the user's Git configuration. @@ -46,7 +60,8 @@ import { tmpdir } from "node:os"; export const ABSENT_GIT_CONFIG = join(tmpdir(), "codecarto-tests-absent-gitconfig"); /** - * Neutralize all three Git configuration sources for this process. + * Neutralize every environment-reachable Git configuration source for this + * process. * * Idempotent, and safe to call from a child process that inherited an * injected environment: the point is to overwrite what was inherited. @@ -58,6 +73,10 @@ export function isolateGitConfig(env = process.env) { // entirely, so any number of already-set pairs become inert; deleting // them one by one would require knowing the original count. env.GIT_CONFIG_COUNT = "0"; + // This variable carries its own entries, so GIT_CONFIG_COUNT cannot + // disarm it. Deleted rather than blanked: it should not be in a fixture + // command's environment at all. + delete env.GIT_CONFIG_PARAMETERS; return env; } From 399ee76a376a143d1c15322a76edd2bc95bc3b2d Mon Sep 17 00:00:00 2001 From: James Sesler Date: Sun, 20 Sep 2026 15:31:15 -0400 Subject: [PATCH 3/3] fix(tests): use a file:// URL for the helper in generated probes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- tests/git-config-isolation.test.mjs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/tests/git-config-isolation.test.mjs b/tests/git-config-isolation.test.mjs index f08a6ab..ba61ad1 100644 --- a/tests/git-config-isolation.test.mjs +++ b/tests/git-config-isolation.test.mjs @@ -26,7 +26,7 @@ import { execFile } from "node:child_process"; import { mkdtemp, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { dirname, join, resolve } from "node:path"; -import { fileURLToPath } from "node:url"; +import { fileURLToPath, pathToFileURL } from "node:url"; import { promisify } from "node:util"; import test from "node:test"; import assert from "node:assert/strict"; @@ -34,6 +34,13 @@ import assert from "node:assert/strict"; const execFileAsync = promisify(execFile); const REPO_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), ".."); +/** + * The isolation helper as a file:// URL. A bare Windows path (`D:\\...`) is + * not a valid ESM specifier, so interpolating REPO_ROOT directly into an + * `import` inside a generated probe fails with ERR_UNSUPPORTED_ESM_URL_SCHEME. + */ +const HELPER_URL = pathToFileURL(join(REPO_ROOT, "tests/helpers/git-config-isolation.mjs")).href; + /** A path git reads as empty config. Matches the helper's own constant. */ const ABSENT = join(tmpdir(), "codecarto-tests-absent-gitconfig"); @@ -96,7 +103,7 @@ test("the injected rewrite really does reach git — the defect is real", async test("the helper neutralizes the injected rewrite in a fresh child process", async () => { const out = await runChild( - `import "${REPO_ROOT}/tests/helpers/git-config-isolation.mjs"; + `import "${HELPER_URL}"; import { execFile } from "node:child_process"; import { mkdtemp } from "node:fs/promises"; import { tmpdir } from "node:os"; @@ -126,7 +133,7 @@ test("isolation survives a large injected config set, not just one entry", async many[`GIT_CONFIG_VALUE_${i}`] = v; } const out = await runChild( - `import "${REPO_ROOT}/tests/helpers/git-config-isolation.mjs"; + `import "${HELPER_URL}"; import { execFile } from "node:child_process"; import { promisify } from "node:util"; const run = promisify(execFile); @@ -148,7 +155,7 @@ test("the helper runs before the modules under test, not merely at module scope" // appears silently the first time one does. Here the probe imports the // isolation first, so the observer must see a clean environment. const out = await runChild( - `import "${REPO_ROOT}/tests/helpers/git-config-isolation.mjs"; + `import "${HELPER_URL}"; import { execFile } from "node:child_process"; import { promisify } from "node:util"; const run = promisify(execFile); @@ -181,7 +188,7 @@ test("GIT_CONFIG_PARAMETERS is neutralized, not merely counted down", async () = // Then prove the helper closes it. const out = await runChild( - `import "${REPO_ROOT}/tests/helpers/git-config-isolation.mjs"; + `import "${HELPER_URL}"; import { execFile } from "node:child_process"; import { mkdtemp } from "node:fs/promises"; import { tmpdir } from "node:os";