From bbe6227eca406887dfbc556ba8299bf5af81d8af Mon Sep 17 00:00:00 2001 From: Lucky Wenapere Date: Thu, 13 Aug 2026 03:00:33 +0100 Subject: [PATCH] fix(test): prepush hook test builds PATH with a POSIX-only separator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `test/redact-prepush-hook.test.ts` shadows `git` with a stub by prepending a temp dir to PATH, built as `${stubDir}:${process.env.PATH}`. On Windows the separator is `;`, so that produces one unparseable entry, the stub is never found, and the REAL git runs — the diff succeeds, `gitStrict` never throws, and the hook exits 0 where the test expects 1. It fails as a wrong assertion rather than as a portability problem, which is what made it hard to place. Replace it with a `prependPath` helper mirroring the one already in test/gstack-brain-context-load.test.ts, which handles both platform details: `path.delimiter`, and a case-insensitive lookup of the existing env key — Windows commonly spells it `Path`, and adding a second `PATH` alongside an inherited `Path` leaves the winner up to the spawn implementation. On POSIX the helper resolves to `{ PATH: binDir + ":" + process.env.PATH }`, byte-identical to the expression it replaces, so behaviour there is unchanged. Fixing the separator alone does not make the test pass on Windows, and it cannot: the premise is that a signal-killed child yields `spawnSync` status === null, and Windows has no equivalent (a force-killed process reports a non-zero exit code). The stub is also a `#!/bin/sh` file named `git`, which Windows will not execute, since process creation resolves through PATHEXT and ignores the shebang. A Windows variant would assert the non-zero-exit branch instead — a different branch than the test name claims — so the test is gated with test.skipIf(process.platform === "win32"), matching test/session-runner-timeout.test.ts and test/setup-emoji-font.test.ts. Windows before: 14 pass, 1 fail. After: 14 pass, 1 skip, 0 fail (3 consecutive runs). Unchanged on POSIX, where it should still run and pass — worth confirming in CI, since I can only verify the Windows half here. Co-Authored-By: Claude Opus 5 --- test/redact-prepush-hook.test.ts | 80 ++++++++++++++++++++++---------- 1 file changed, 56 insertions(+), 24 deletions(-) diff --git a/test/redact-prepush-hook.test.ts b/test/redact-prepush-hook.test.ts index cf85985237..53b5db9dfb 100644 --- a/test/redact-prepush-hook.test.ts +++ b/test/redact-prepush-hook.test.ts @@ -44,6 +44,24 @@ function runHook( return { code: r.status ?? 0, stderr: r.stderr ?? "" }; } +/** + * Env override that puts `binDir` first on the search path, for tests that + * shadow a real binary with a stub. + * + * Two portability details, both of which a hardcoded `PATH: "dir:" + ...` + * gets wrong (mirrors `prependPath` in test/gstack-brain-context-load.test.ts): + * - The separator is `;` on Windows, not `:`. Using the literal produces one + * unparseable entry, so the stub is never found and the REAL binary runs — + * a test that silently passes through rather than failing loudly. + * - Windows env keys are case-insensitive and commonly spelled `Path`. Adding + * a second `PATH` key alongside an inherited `Path` leaves which one wins up + * to the spawn implementation, so reuse whichever key already exists. + */ +function prependPath(binDir: string): Record { + const pathKey = Object.keys(process.env).find((k) => k.toLowerCase() === "path") || "PATH"; + return { [pathKey]: `${binDir}${path.delimiter}${process.env[pathKey] || ""}` }; +} + const ZERO = "0000000000000000000000000000000000000000"; beforeEach(() => { @@ -141,30 +159,44 @@ describe("fail closed on unscannable diffs (#1946)", () => { expect(stderr).not.toContain("could not compute the pushed diff"); }); - test("a diff killed by a signal (null status — the maxBuffer/kill class) BLOCKS", () => { - // Stub git: probes delegate to the real git; the diff invocation kills - // itself, producing spawnSync status === null. This is the exact branch - // gitStrict's docstring names (oversized-diff overflow is delivered the - // same way) — pre-landing review flagged it as untested. - const realGit = Bun.which("git") || "/usr/bin/git"; - const stubDir = fs.mkdtempSync(path.join(os.tmpdir(), "prepush-stubgit-")); - try { - const stub = `#!/bin/sh\nif [ "$1" = "diff" ]; then kill -KILL $$; fi\nexec "${realGit}" "$@"\n`; - fs.writeFileSync(path.join(stubDir, "git"), stub); - fs.chmodSync(path.join(stubDir, "git"), 0o755); - - const base = git(["rev-parse", "HEAD"]); - const head = commit("clean.txt", "clean content\n", "clean commit"); - const { code, stderr } = runHook(`refs/heads/main ${head} refs/heads/main ${base}\n`, { - PATH: `${stubDir}:${process.env.PATH}`, - }); - expect(code).toBe(1); - expect(stderr).toContain("could not compute the pushed diff"); - expect(stderr).toContain("GSTACK_REDACT_PREPUSH=skip"); - } finally { - fs.rmSync(stubDir, { recursive: true, force: true }); - } - }); + // POSIX-only, for two independent reasons. `spawnSync` reports + // `status === null` only when a child dies from a signal, and Windows has no + // equivalent — a force-killed process surfaces a non-zero exit code there — so + // the branch this test names is unreachable. The stub below is also a + // `#!/bin/sh` file named `git`, which Windows will not execute at all, since + // process creation resolves commands through PATHEXT (.exe/.cmd/.bat) and + // ignores the shebang. A Windows variant would have to assert the non-zero + // exit path instead, i.e. a different branch than the name claims, so it is + // skipped rather than rewritten. Gate style follows + // test/session-runner-timeout.test.ts and test/setup-emoji-font.test.ts. + test.skipIf(process.platform === "win32")( + "a diff killed by a signal (null status — the maxBuffer/kill class) BLOCKS", + () => { + // Stub git: probes delegate to the real git; the diff invocation kills + // itself, producing spawnSync status === null. This is the exact branch + // gitStrict's docstring names (oversized-diff overflow is delivered the + // same way) — pre-landing review flagged it as untested. + const realGit = Bun.which("git") || "/usr/bin/git"; + const stubDir = fs.mkdtempSync(path.join(os.tmpdir(), "prepush-stubgit-")); + try { + const stub = `#!/bin/sh\nif [ "$1" = "diff" ]; then kill -KILL $$; fi\nexec "${realGit}" "$@"\n`; + fs.writeFileSync(path.join(stubDir, "git"), stub); + fs.chmodSync(path.join(stubDir, "git"), 0o755); + + const base = git(["rev-parse", "HEAD"]); + const head = commit("clean.txt", "clean content\n", "clean commit"); + const { code, stderr } = runHook( + `refs/heads/main ${head} refs/heads/main ${base}\n`, + prependPath(stubDir), + ); + expect(code).toBe(1); + expect(stderr).toContain("could not compute the pushed diff"); + expect(stderr).toContain("GSTACK_REDACT_PREPUSH=skip"); + } finally { + fs.rmSync(stubDir, { recursive: true, force: true }); + } + }, + ); }); describe("install UX surfaces (#1946 / eng review D3+D10)", () => {