From 49fa1a0b8895998aa3dff928d47f42525a9547e2 Mon Sep 17 00:00:00 2001 From: schienbiz <274676847+schienbiz@users.noreply.github.com> Date: Sun, 30 Aug 2026 00:30:57 +0800 Subject: [PATCH] fix(redact): install-prepush-hook refreshes a stale managed hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The marker check returned before the only writer, so once a repo had the hook, no later change to the wrapper could ever reach it. The `printf x` fail-open fix (v1.64.0.0) has still not landed in any repo that received the hook before it, and a wrapper naming a gstack that has since moved stays pointed at a dead path for the same reason. Compare the body against what this version generates: rewrite on drift, stay a no-op when identical. The chained pre-push.local is untouched on both paths. The existing trailing-newline regression test cannot catch this — it installs into a repo with no prior managed hook, the one case that was never broken. --- bin/gstack-redact | 46 +++++++++++++----- test/redact-prepush-hook.test.ts | 81 ++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 13 deletions(-) diff --git a/bin/gstack-redact b/bin/gstack-redact index 9e5eebb6c..45bb1235f 100755 --- a/bin/gstack-redact +++ b/bin/gstack-redact @@ -62,19 +62,6 @@ function installPrepushHook(): void { const hookPath = path.join(dir, "pre-push"); const prepushBin = path.join(import.meta.dir, "gstack-redact-prepush"); - // If a non-managed hook exists, preserve it as pre-push.local and chain it. - if (fs.existsSync(hookPath)) { - const existing = fs.readFileSync(hookPath, "utf8"); - if (existing.includes(MANAGED_MARKER)) { - process.stdout.write("gstack-redact: pre-push hook already installed.\n"); - return; - } - const localPath = path.join(dir, "pre-push.local"); - fs.renameSync(hookPath, localPath); - fs.chmodSync(localPath, 0o755); - process.stdout.write("gstack-redact: preserved existing hook as pre-push.local (chained).\n"); - } - // stdin is single-consume: capture it once, feed both the chained hook and ours. // The `printf x` sentinel preserves the trailing newline that `$(cat)` strips. // Without it, a chained shell pre-push.local built on `while read` silently @@ -91,6 +78,39 @@ if [ -x "$_local" ]; then fi printf '%s' "$_input" | bun "${prepushBin}" "$@" `; + + // If a non-managed hook exists, preserve it as pre-push.local and chain it. + if (fs.existsSync(hookPath)) { + const existing = fs.readFileSync(hookPath, "utf8"); + if (existing.includes(MANAGED_MARKER)) { + // A hook we already own. Returning here unconditionally froze every + // existing install on whatever wrapper it first received: the `printf x` + // fail-open fix landed in v1.64.0.0 and still had not reached a single + // repo that got the hook before it, because the only writer is gated on + // this branch. A wrapper naming a gstack that has since been moved or + // removed stays pointed at that dead path for the same reason. + // + // Rewrite when the body has drifted from what this version generates; + // stay a no-op when it has not, so the command is still idempotent. The + // chained pre-push.local is never touched on either path — it is the + // user's, not ours. + if (existing === wrapper) { + process.stdout.write("gstack-redact: pre-push hook already installed.\n"); + return; + } + fs.writeFileSync(hookPath, wrapper, { mode: 0o755 }); + fs.chmodSync(hookPath, 0o755); + process.stdout.write( + `gstack-redact: refreshed stale managed pre-push hook at ${hookPath}\n`, + ); + return; + } + const localPath = path.join(dir, "pre-push.local"); + fs.renameSync(hookPath, localPath); + fs.chmodSync(localPath, 0o755); + process.stdout.write("gstack-redact: preserved existing hook as pre-push.local (chained).\n"); + } + fs.writeFileSync(hookPath, wrapper, { mode: 0o755 }); fs.chmodSync(hookPath, 0o755); process.stdout.write(`gstack-redact: installed pre-push hook at ${hookPath}\n`); diff --git a/test/redact-prepush-hook.test.ts b/test/redact-prepush-hook.test.ts index 3c583a3a9..f4b591f2a 100644 --- a/test/redact-prepush-hook.test.ts +++ b/test/redact-prepush-hook.test.ts @@ -381,6 +381,87 @@ describe("install / chaining", () => { expect(r.status).toBe(1); }); + // Regression: install returned early on ANY hook carrying the managed marker, + // so the only writer was unreachable once a hook existed. Every fix to the + // wrapper — including the `printf x` fail-open fix of v1.64.0.0 — stopped at + // repos that had never had the hook. The pre-existing newline test above + // cannot catch this: it installs into a repo with no prior managed hook, which + // is the one case that was never broken. + test("a stale managed hook is rewritten, and the refresh delivers the newline fix", () => { + const hookDir = path.join(repo, ".git", "hooks"); + fs.mkdirSync(hookDir, { recursive: true }); + const hook = path.join(hookDir, "pre-push"); + + // The v1.63-era wrapper, verbatim: same marker, `$(cat)` with no sentinel. + fs.writeFileSync( + hook, + [ + "#!/usr/bin/env bash", + "# gstack-redact pre-push (managed)", + "set -euo pipefail", + '_input="$(cat)"', + '_local="$(git rev-parse --git-path hooks/pre-push.local)"', + 'if [ -x "$_local" ]; then', + ` printf '%s' "$_input" | "$_local" "$@" || exit $?`, + "fi", + `printf '%s' "$_input" | bun ${JSON.stringify(PREPUSH)} "$@"`, + "", + ].join("\n"), + { mode: 0o755 }, + ); + + // A chained local hook of the shape the old wrapper starved: a bare + // `while read`, which never enters its body without a trailing newline. + const seen = path.join(repo, "seen.txt"); + fs.writeFileSync( + path.join(hookDir, "pre-push.local"), + `#!/usr/bin/env bash\nwhile read -r a _b _c _d; do echo "$a" >> ${JSON.stringify(seen)}; done\nexit 0\n`, + { mode: 0o755 }, + ); + + const r = spawnSync("bun", [REDACT, "install-prepush-hook"], { + cwd: repo, + encoding: "utf8", + }); + expect(r.status).toBe(0); + expect(r.stdout).toContain("refreshed stale managed pre-push hook"); + expect(fs.readFileSync(hook, "utf8")).toContain("printf x"); + + // The chained hook is the user's; a refresh must not rename or rewrite it. + expect(fs.readFileSync(path.join(hookDir, "pre-push.local"), "utf8")).toContain("while read"); + + // Behavioural half: the refreshed wrapper actually feeds the final ref line. + const sha = "c".repeat(40); + const run = spawnSync("bash", [hook], { + cwd: repo, + input: Buffer.from(`refs/heads/main ${sha} refs/heads/main ${ZERO}\n`), + encoding: "utf8", + env: { ...process.env, GSTACK_REDACT_PREPUSH: "skip" }, + }); + expect(run.status).toBe(0); + expect(fs.readFileSync(seen, "utf8").trim()).toBe("refs/heads/main"); + }); + + test("install stays idempotent: an up-to-date managed hook is not rewritten", () => { + const hookDir = path.join(repo, ".git", "hooks"); + fs.mkdirSync(hookDir, { recursive: true }); + const hook = path.join(hookDir, "pre-push"); + + spawnSync("bun", [REDACT, "install-prepush-hook"], { cwd: repo }); + const first = fs.readFileSync(hook, "utf8"); + const stamp = fs.statSync(hook).mtimeMs; + + const again = spawnSync("bun", [REDACT, "install-prepush-hook"], { + cwd: repo, + encoding: "utf8", + }); + expect(again.status).toBe(0); + expect(again.stdout).toContain("already installed"); + expect(again.stdout).not.toContain("refreshed"); + expect(fs.readFileSync(hook, "utf8")).toBe(first); + expect(fs.statSync(hook).mtimeMs).toBe(stamp); + }); + test("uninstall restores the chained original", () => { const hookDir = path.join(repo, ".git", "hooks"); fs.mkdirSync(hookDir, { recursive: true });