diff --git a/bin/gstack-redact b/bin/gstack-redact index fa5c1ac90..9e5eebb6c 100755 --- a/bin/gstack-redact +++ b/bin/gstack-redact @@ -123,6 +123,15 @@ function flag(name: string): boolean { function readInput(): string { const file = arg("--from-file"); + // An explicitly-passed EMPTY path must error, not silently fall through to + // stdin: skill blocks pass "$FILE" from a $(mktemp) that may have failed, + // and the stdin fallback then scans nothing while looking green (#2679). + if (file === "") { + process.stderr.write( + "gstack-redact: --from-file requires a non-empty path (did mktemp fail?)\n", + ); + process.exit(1); + } if (file) { const st = fs.statSync(file); if (st.size > MAX_STDIN_BYTES) { diff --git a/gstack-upgrade/SKILL.md b/gstack-upgrade/SKILL.md index b5ec03d42..9055789a8 100644 --- a/gstack-upgrade/SKILL.md +++ b/gstack-upgrade/SKILL.md @@ -170,12 +170,18 @@ If `$STASH_OUTPUT` contains "Saved working directory", warn the user: "Note: loc **For vendored installs** (vendored, vendored-global): ```bash PARENT=$(dirname "$INSTALL_DIR") -TMP_DIR=$(mktemp -d) -git clone --depth 1 https://github.com/garrytan/gstack.git "$TMP_DIR/gstack" +TMP_DIR=$(mktemp -d) || { echo "ERROR: mktemp failed — aborting upgrade (install untouched)." >&2; exit 1; } +git clone --depth 1 https://github.com/garrytan/gstack.git "$TMP_DIR/gstack" || { echo "ERROR: clone failed — aborting upgrade (install untouched)." >&2; rm -rf "$TMP_DIR"; exit 1; } mv "$INSTALL_DIR" "$INSTALL_DIR.bak" -mv "$TMP_DIR/gstack" "$INSTALL_DIR" -cd "$INSTALL_DIR" && ./setup -rm -rf "$INSTALL_DIR.bak" "$TMP_DIR" +if mv "$TMP_DIR/gstack" "$INSTALL_DIR"; then + cd "$INSTALL_DIR" && ./setup + rm -rf "$INSTALL_DIR.bak" "$TMP_DIR" +else + mv "$INSTALL_DIR.bak" "$INSTALL_DIR" + echo "ERROR: swap failed — previous install restored; upgrade aborted." >&2 + rm -rf "$TMP_DIR" + exit 1 +fi ``` ### Step 4.5: Handle local vendored copy diff --git a/gstack-upgrade/SKILL.md.tmpl b/gstack-upgrade/SKILL.md.tmpl index ca211d919..eb112fdea 100644 --- a/gstack-upgrade/SKILL.md.tmpl +++ b/gstack-upgrade/SKILL.md.tmpl @@ -167,12 +167,18 @@ If `$STASH_OUTPUT` contains "Saved working directory", warn the user: "Note: loc **For vendored installs** (vendored, vendored-global): ```bash PARENT=$(dirname "$INSTALL_DIR") -TMP_DIR=$(mktemp -d) -git clone --depth 1 https://github.com/garrytan/gstack.git "$TMP_DIR/gstack" +TMP_DIR=$(mktemp -d) || { echo "ERROR: mktemp failed — aborting upgrade (install untouched)." >&2; exit 1; } +git clone --depth 1 https://github.com/garrytan/gstack.git "$TMP_DIR/gstack" || { echo "ERROR: clone failed — aborting upgrade (install untouched)." >&2; rm -rf "$TMP_DIR"; exit 1; } mv "$INSTALL_DIR" "$INSTALL_DIR.bak" -mv "$TMP_DIR/gstack" "$INSTALL_DIR" -cd "$INSTALL_DIR" && {{SETUP_COMMAND}} -rm -rf "$INSTALL_DIR.bak" "$TMP_DIR" +if mv "$TMP_DIR/gstack" "$INSTALL_DIR"; then + cd "$INSTALL_DIR" && {{SETUP_COMMAND}} + rm -rf "$INSTALL_DIR.bak" "$TMP_DIR" +else + mv "$INSTALL_DIR.bak" "$INSTALL_DIR" + echo "ERROR: swap failed — previous install restored; upgrade aborted." >&2 + rm -rf "$TMP_DIR" + exit 1 +fi ``` ### Step 4.5: Handle local vendored copy diff --git a/scripts/resolvers/redact-doc.ts b/scripts/resolvers/redact-doc.ts index 855bc3d8c..0cfaa71d8 100644 --- a/scripts/resolvers/redact-doc.ts +++ b/scripts/resolvers/redact-doc.ts @@ -62,7 +62,7 @@ REDACT_VIS=$(~/.claude/skills/gstack/bin/gstack-config get redact_repo_visibilit [ -z "$REDACT_VIS" ] && REDACT_VIS=$(gh repo view --json visibility -q .visibility 2>/dev/null | tr 'A-Z' 'a-z') [ -z "$REDACT_VIS" ] && REDACT_VIS=$(glab repo view -F json 2>/dev/null | grep -o '"visibility":"[^"]*"' | head -1 | sed 's/.*:"//;s/"//' | tr 'A-Z' 'a-z') REDACT_VIS="\${REDACT_VIS:-unknown}" -REDACT_FILE=$(mktemp) +REDACT_FILE=$(mktemp) || { echo "ERROR: mktemp failed — refusing to send unscanned ${sink.noun}." >&2; exit 1; } cat > "$REDACT_FILE" <<'REDACT_BODY_EOF' REDACT_BODY_EOF diff --git a/ship/sections/pr-body.md b/ship/sections/pr-body.md index 6373ed1f8..c3e516415 100644 --- a/ship/sections/pr-body.md +++ b/ship/sections/pr-body.md @@ -170,7 +170,7 @@ the PR (a live-format credential inside the fence still blocks). REDACT_VIS=$(~/.claude/skills/gstack/bin/gstack-config get redact_repo_visibility 2>/dev/null) [ -z "$REDACT_VIS" ] && REDACT_VIS=$(gh repo view --json visibility -q .visibility 2>/dev/null | tr 'A-Z' 'a-z') REDACT_VIS="${REDACT_VIS:-unknown}" -PR_BODY_FILE=$(mktemp) +PR_BODY_FILE=$(mktemp) || { echo "ERROR: mktemp failed — cannot scan the PR body; refusing to create the PR unscanned." >&2; exit 1; } cat > "$PR_BODY_FILE" <<'PR_BODY_EOF' PR_BODY_EOF @@ -200,10 +200,10 @@ rm -f "$PR_BODY_FILE" ```bash # MR title MUST start with v$NEW_VERSION — enforced on every run, no exceptions. # (See Step 19 idempotency block + bin/gstack-pr-title-rewrite.sh for the rule.) -glab mr create -b -t "v$NEW_VERSION : " -d "$(cat <<'EOF' - -EOF -)" +# Send the SCANNED file's bytes — scan-at-sink means never re-render the body +# from a fresh heredoc (that reopens the scan-vs-send gap). +glab mr create -b -t "v$NEW_VERSION : " -d "$(cat "$PR_BODY_FILE")" +rm -f "$PR_BODY_FILE" ``` **If neither CLI is available:** diff --git a/ship/sections/pr-body.md.tmpl b/ship/sections/pr-body.md.tmpl index ac3b05274..ba42495c8 100644 --- a/ship/sections/pr-body.md.tmpl +++ b/ship/sections/pr-body.md.tmpl @@ -168,7 +168,7 @@ the PR (a live-format credential inside the fence still blocks). REDACT_VIS=$(~/.claude/skills/gstack/bin/gstack-config get redact_repo_visibility 2>/dev/null) [ -z "$REDACT_VIS" ] && REDACT_VIS=$(gh repo view --json visibility -q .visibility 2>/dev/null | tr 'A-Z' 'a-z') REDACT_VIS="${REDACT_VIS:-unknown}" -PR_BODY_FILE=$(mktemp) +PR_BODY_FILE=$(mktemp) || { echo "ERROR: mktemp failed — cannot scan the PR body; refusing to create the PR unscanned." >&2; exit 1; } cat > "$PR_BODY_FILE" <<'PR_BODY_EOF' PR_BODY_EOF @@ -198,10 +198,10 @@ rm -f "$PR_BODY_FILE" ```bash # MR title MUST start with v$NEW_VERSION — enforced on every run, no exceptions. # (See Step 19 idempotency block + bin/gstack-pr-title-rewrite.sh for the rule.) -glab mr create -b -t "v$NEW_VERSION : " -d "$(cat <<'EOF' - -EOF -)" +# Send the SCANNED file's bytes — scan-at-sink means never re-render the body +# from a fresh heredoc (that reopens the scan-vs-send gap). +glab mr create -b -t "v$NEW_VERSION : " -d "$(cat "$PR_BODY_FILE")" +rm -f "$PR_BODY_FILE" ``` **If neither CLI is available:** diff --git a/spec/sections/gate-and-file.md b/spec/sections/gate-and-file.md index acbede0d5..a442e181b 100644 --- a/spec/sections/gate-and-file.md +++ b/spec/sections/gate-and-file.md @@ -59,7 +59,7 @@ REDACT_VIS=$(~/.claude/skills/gstack/bin/gstack-config get redact_repo_visibilit [ -z "$REDACT_VIS" ] && REDACT_VIS=$(gh repo view --json visibility -q .visibility 2>/dev/null | tr 'A-Z' 'a-z') [ -z "$REDACT_VIS" ] && REDACT_VIS=$(glab repo view -F json 2>/dev/null | grep -o '"visibility":"[^"]*"' | head -1 | sed 's/.*:"//;s/"//' | tr 'A-Z' 'a-z') REDACT_VIS="${REDACT_VIS:-unknown}" -REDACT_FILE=$(mktemp) +REDACT_FILE=$(mktemp) || { echo "ERROR: mktemp failed — refusing to send unscanned the spec body." >&2; exit 1; } cat > "$REDACT_FILE" <<'REDACT_BODY_EOF' REDACT_BODY_EOF diff --git a/test/fixtures/golden/codex-ship-SKILL.md b/test/fixtures/golden/codex-ship-SKILL.md index c218bd9cb..7dbfbaf2c 100644 --- a/test/fixtures/golden/codex-ship-SKILL.md +++ b/test/fixtures/golden/codex-ship-SKILL.md @@ -2452,7 +2452,7 @@ the PR (a live-format credential inside the fence still blocks). REDACT_VIS=$($GSTACK_ROOT/bin/gstack-config get redact_repo_visibility 2>/dev/null) [ -z "$REDACT_VIS" ] && REDACT_VIS=$(gh repo view --json visibility -q .visibility 2>/dev/null | tr 'A-Z' 'a-z') REDACT_VIS="${REDACT_VIS:-unknown}" -PR_BODY_FILE=$(mktemp) +PR_BODY_FILE=$(mktemp) || { echo "ERROR: mktemp failed — cannot scan the PR body; refusing to create the PR unscanned." >&2; exit 1; } cat > "$PR_BODY_FILE" <<'PR_BODY_EOF' PR_BODY_EOF @@ -2482,10 +2482,10 @@ rm -f "$PR_BODY_FILE" ```bash # MR title MUST start with v$NEW_VERSION — enforced on every run, no exceptions. # (See Step 19 idempotency block + bin/gstack-pr-title-rewrite.sh for the rule.) -glab mr create -b -t "v$NEW_VERSION : " -d "$(cat <<'EOF' - -EOF -)" +# Send the SCANNED file's bytes — scan-at-sink means never re-render the body +# from a fresh heredoc (that reopens the scan-vs-send gap). +glab mr create -b -t "v$NEW_VERSION : " -d "$(cat "$PR_BODY_FILE")" +rm -f "$PR_BODY_FILE" ``` **If neither CLI is available:** diff --git a/test/fixtures/golden/factory-ship-SKILL.md b/test/fixtures/golden/factory-ship-SKILL.md index aa0a99b23..0db6979e6 100644 --- a/test/fixtures/golden/factory-ship-SKILL.md +++ b/test/fixtures/golden/factory-ship-SKILL.md @@ -2879,7 +2879,7 @@ the PR (a live-format credential inside the fence still blocks). REDACT_VIS=$($GSTACK_ROOT/bin/gstack-config get redact_repo_visibility 2>/dev/null) [ -z "$REDACT_VIS" ] && REDACT_VIS=$(gh repo view --json visibility -q .visibility 2>/dev/null | tr 'A-Z' 'a-z') REDACT_VIS="${REDACT_VIS:-unknown}" -PR_BODY_FILE=$(mktemp) +PR_BODY_FILE=$(mktemp) || { echo "ERROR: mktemp failed — cannot scan the PR body; refusing to create the PR unscanned." >&2; exit 1; } cat > "$PR_BODY_FILE" <<'PR_BODY_EOF' PR_BODY_EOF @@ -2909,10 +2909,10 @@ rm -f "$PR_BODY_FILE" ```bash # MR title MUST start with v$NEW_VERSION — enforced on every run, no exceptions. # (See Step 19 idempotency block + bin/gstack-pr-title-rewrite.sh for the rule.) -glab mr create -b -t "v$NEW_VERSION : " -d "$(cat <<'EOF' - -EOF -)" +# Send the SCANNED file's bytes — scan-at-sink means never re-render the body +# from a fresh heredoc (that reopens the scan-vs-send gap). +glab mr create -b -t "v$NEW_VERSION : " -d "$(cat "$PR_BODY_FILE")" +rm -f "$PR_BODY_FILE" ``` **If neither CLI is available:** diff --git a/test/regression-pr1169-mktemp-fallbacks.test.ts b/test/regression-pr1169-mktemp-fallbacks.test.ts index 0ed0d3cb2..ea727705b 100644 --- a/test/regression-pr1169-mktemp-fallbacks.test.ts +++ b/test/regression-pr1169-mktemp-fallbacks.test.ts @@ -56,6 +56,77 @@ describe("PR #1169 bug #4: gstack-telemetry-sync mktemp fallback", () => { }); }); +// #2679: three skill-content mktemp sites ran unguarded. An empty result +// ("" on mktemp failure) silently disabled the redaction pass (redact-doc +// resolver + ship pr-body) and — the destructive one — made /gstack-upgrade's +// vendored path clone to "/gstack", fail the swap, then `rm -rf` BOTH the +// live install's backup and "". Guards must abort loudly; the upgrade block +// must also restore the backup when the swap fails (same failure class: +// backup deletion after a failed mv). +describe("#2679: skill-content mktemp guards", () => { + test("redact-doc resolver guards REDACT_FILE=$(mktemp) with a loud exit", () => { + // The guard line contains a ${sink.noun} interpolation in the resolver + // source, so match to end-of-line rather than [^}]* (which stops at the + // interpolation's closing brace). + const body = readScript("scripts/resolvers/redact-doc.ts"); + expect(body).toMatch(/REDACT_FILE=\$\(mktemp\)\s*\|\|\s*\{.*exit 1/); + // And the rendered output (interpolation resolved) carries the guard too. + const rendered = readScript("spec/sections/gate-and-file.md"); + expect(rendered).toMatch(/REDACT_FILE=\$\(mktemp\)\s*\|\|\s*\{[^}]*exit 1/); + }); + + test("ship pr-body template guards PR_BODY_FILE=$(mktemp) with a loud exit", () => { + const body = readScript("ship/sections/pr-body.md.tmpl"); + expect(body).toMatch(/PR_BODY_FILE=\$\(mktemp\)\s*\|\|\s*\{[^}]*exit 1/); + }); + + test("ship pr-body GitLab path sends the SCANNED file, never a re-rendered heredoc", () => { + const body = readScript("ship/sections/pr-body.md.tmpl"); + expect(body).toContain('-d "$(cat "$PR_BODY_FILE")"'); + expect(body).not.toMatch(/glab mr create[^\n]*-d "\$\(cat <<'EOF'/); + }); + + test("gstack-upgrade vendored block guards mktemp -d and clone with loud aborts", () => { + const body = readScript("gstack-upgrade/SKILL.md.tmpl"); + expect(body).toMatch(/TMP_DIR=\$\(mktemp -d\)\s*\|\|\s*\{[^}]*exit 1/); + expect(body).toMatch(/git clone[^\n]*\|\|\s*\{[^}]*exit 1/); + }); + + test("gstack-upgrade vendored block restores the backup on a failed swap (no unconditional backup rm)", () => { + const body = readScript("gstack-upgrade/SKILL.md.tmpl"); + expect(body).toMatch(/if mv "\$TMP_DIR\/gstack" "\$INSTALL_DIR"; then/); + expect(body).toMatch(/mv "\$INSTALL_DIR\.bak" "\$INSTALL_DIR"/); + // The backup rm must live inside the success branch, not after the block. + const block = body.slice(body.indexOf('if mv "$TMP_DIR/gstack"')); + const successRm = block.indexOf('rm -rf "$INSTALL_DIR.bak"'); + const elseBranch = block.indexOf("else"); + expect(successRm).toBeGreaterThan(-1); + expect(successRm).toBeLessThan(elseBranch); + }); + + test("runtime: the guarded assignment aborts when mktemp fails", () => { + const { spawnSync } = require("node:child_process") as typeof import("node:child_process"); + const script = `mktemp() { return 1; } +TMP_DIR=$(mktemp -d) || { echo "ERROR: mktemp failed — aborting upgrade (install untouched)." >&2; exit 1; } +echo "SHOULD NOT REACH: $TMP_DIR"`; + const r = spawnSync("bash", ["-c", script], { encoding: "utf-8", timeout: 10_000 }); + expect(r.status).toBe(1); + expect(r.stderr).toContain("mktemp failed"); + expect(r.stdout).not.toContain("SHOULD NOT REACH"); + }); + + test("runtime: gstack-redact --from-file '' errors loudly instead of falling through to stdin", () => { + const { spawnSync } = require("node:child_process") as typeof import("node:child_process"); + const r = spawnSync( + "bun", + [path.join(ROOT, "bin", "gstack-redact"), "--from-file", "", "--json"], + { encoding: "utf-8", input: "sk-ant-api03-not-really-a-key", timeout: 15_000 }, + ); + expect(r.status).toBe(1); + expect(r.stderr).toContain("non-empty path"); + }); +}); + describe("PR #1169 bug #5: supabase/verify-rls.sh mktemp fallback", () => { const SCRIPT = "supabase/verify-rls.sh";