fix(tests): isolate git config injected through the environment (B01 pilot)

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 });
 }

tests/broadside.test.mjs

@@ -3,25 +3,14 @@
 // 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";
 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.<base>.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

tests/git-config-isolation.test.mjs

@@ -0,0 +1,276 @@
+// 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, pathToFileURL } 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 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");
+
+/** 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 "${HELPER_URL}";
+		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 "${HELPER_URL}";
+		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. 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 "${HELPER_URL}";
+		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("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 "${HELPER_URL}";
+		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
+	// 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");
+	// 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`);
+	}
+});