fix: pre-landing review fixes

Review army findings (1 critical, auto-fixed with regression tests):

- CRITICAL (security specialist, verified live): redactFindingSpans spliced
  only the regex capture span, and pem.private_key / gcp.service_account
  capture just the BEGIN-header — the key body survived "redaction" and
  shipped via telemetry. Marker-only patterns now drop the whole payload
  (null, fail closed). Overlapping spans (Bearer+JWT on the same bytes) are
  coalesced before splicing so stale offsets can't leave partial secret
  bytes behind.
- gitStrict: drop the dead `|| r.status === null` disjunct (null !== 0
  already covers it); add the signal-kill/null-status regression test the
  docstring promised.
- security-dashboard human mode flags stale snapshots ("figures may be out
  of date") instead of presenting frozen counts as current.
- community-dashboard marker check uses jq when available — the grep-only
  variant misclassified whitespaced/reserialized bodies as legacy.
- telemetry fail-closed test now shadows bun with a failing stub
  (deterministic on any host layout); stale "five status cases" describe
  title renamed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

bin/gstack-community-dashboard

@@ -57,7 +57,14 @@ WEEKLY="$(echo "$DATA" | grep -o '"weekly_active":[0-9]*' | grep -o '[0-9]*' ||
 CHANGE="$(echo "$DATA" | grep -o '"change_pct":[0-9-]*' | grep -o '[0-9-]*' || echo "0")"
 
 echo "Weekly active installs: ${WEEKLY}"
-if ! printf '%s' "$DATA" | grep -q '"status":"ok"'; then
+# Marker check: jq when available (whitespace/reserialization-proof); grep
+# fallback only matches the compact form the edge function emits today.
+if command -v jq >/dev/null 2>&1; then
+  _MARKER="$(printf '%s' "$DATA" | jq -r '.status // empty' 2>/dev/null)"
+else
+  _MARKER="$(printf '%s' "$DATA" | grep -q '"status":"ok"' && echo ok || true)"
+fi
+if [ "$_MARKER" != "ok" ]; then
   echo "  (unverified — legacy backend response; deploy the latest community-pulse for verified figures)"
 fi
 if [ "$CHANGE" -gt 0 ] 2>/dev/null; then

bin/gstack-redact-prepush

@@ -54,7 +54,9 @@ function git(args: string[]): string {
  */
 function gitStrict(args: string[]): string {
   const r = spawnSync("git", args, { encoding: "utf8", maxBuffer: 64 * 1024 * 1024 });
-  if (r.status !== 0 || r.status === null) {
+  // status !== 0 covers BOTH a non-zero exit AND null (process killed by a
+  // signal or maxBuffer overflow — null !== 0 is true).
+  if (r.status !== 0) {
     throw new Error(
       `git ${args[0]} failed (status=${r.status ?? "killed/overflow"}): ${(r.stderr ?? "").slice(0, 300)}`,
     );

bin/gstack-security-dashboard

@@ -110,6 +110,10 @@ TOTAL="$(echo "$DATA" | jq -r '.security.attacks_last_7_days // 0' 2>/dev/null |
 echo "Attacks detected last 7 days: ${TOTAL}"
 if [ "$STATE" = "legacy" ]; then
   echo "  (unverified — legacy backend response; deploy the latest community-pulse for verified figures)"
+elif [ "$(echo "$DATA" | jq -r '.stale // false' 2>/dev/null)" = "true" ]; then
+  # The backend serves its last good snapshot when recompute fails — figures
+  # are real but frozen. Don't present them as current.
+  echo "  (stale snapshot — backend recompute failing; figures may be out of date)"
 elif [ "$TOTAL" = "0" ]; then
   echo "  (No attack attempts reported by the community yet. Good news.)"
 fi

lib/redact-engine.ts

@@ -427,24 +427,54 @@ export function applyRedactions(
   return { body, diff: diffLines.reverse().join("\n"), skipped };
 }
 
+/**
+ * Patterns whose regex captures only a MARKER, not the secret payload itself
+ * (the PEM header line; the GCP JSON key prefix). Span replacement on these
+ * would redact the header and forward the key body — so redactFindingSpans
+ * drops the whole payload instead.
+ */
+const MARKER_ONLY_PATTERN_IDS = new Set(["pem.private_key", "gcp.service_account"]);
+
 /**
  * Replace EVERY finding's span with `<REDACTED-{id}>`, regardless of tier or
  * autoRedactable. For machine egress surfaces (telemetry error_message,
  * #1947) where structure preservation doesn't matter and fail-closed beats
- * fidelity. Returns null when any finding's span cannot be located — the
- * caller must then drop the whole payload rather than risk leaking an
- * unlocated secret. (Contrast applyRedactions, which is the interactive,
- * autoRedactable-only, structure-preserving path.)
+ * fidelity. Returns null — caller must drop the whole payload — when:
+ *   - any finding's span cannot be located, or
+ *   - any finding matched a marker-only pattern (PEM / GCP service-account
+ *     JSON): their regexes capture the header, not the key material, so a
+ *     span splice would leak the body that follows the marker.
+ * Overlapping spans (e.g. a Bearer token that is also a JWT) are coalesced
+ * before splicing so stale offsets never leave partial secret bytes behind.
+ * (Contrast applyRedactions, which is the interactive, autoRedactable-only,
+ * structure-preserving path.)
  */
 export function redactFindingSpans(input: string, opts: ScanOptions = {}): string | null {
   const { findings } = scan(input, opts);
+  if (findings.some((f) => MARKER_ONLY_PATTERN_IDS.has(f.id))) return null;
   const targets = findings.map((f) => ({ f, ...locateSpan(input, f) }));
   if (targets.some((t) => t.start < 0)) return null;
-  // Right-to-left so earlier offsets remain valid after splicing.
-  targets.sort((a, b) => b.start - a.start);
-  let body = input;
+
+  // Coalesce overlapping/touching ranges — splicing two intersecting spans
+  // independently applies a stale end offset to already-modified text and
+  // can leave trailing secret bytes in place.
+  targets.sort((a, b) => a.start - b.start);
+  const merged: Array<{ start: number; end: number; ids: string[] }> = [];
   for (const t of targets) {
-    body = body.slice(0, t.start) + `<REDACTED-${t.f.id}>` + body.slice(t.end);
+    const last = merged[merged.length - 1];
+    if (last && t.start <= last.end) {
+      last.end = Math.max(last.end, t.end);
+      if (!last.ids.includes(t.f.id)) last.ids.push(t.f.id);
+    } else {
+      merged.push({ start: t.start, end: t.end, ids: [t.f.id] });
+    }
+  }
+
+  // Right-to-left so earlier offsets remain valid after splicing.
+  let body = input;
+  for (let i = merged.length - 1; i >= 0; i--) {
+    const m = merged[i];
+    body = body.slice(0, m.start) + `<REDACTED-${m.ids.join("+")}>` + body.slice(m.end);
   }
   return body;
 }

test/gbrain-local-status.test.ts

@@ -174,7 +174,7 @@ function applyEnv(env: FakeEnv): () => void {
   };
 }
 
-describe("lib/gbrain-local-status — five status cases", () => {
+describe("lib/gbrain-local-status — status classification", () => {
   let env: FakeEnv | null = null;
   let restoreEnv: (() => void) | null = null;
 

test/redact-engine.test.ts

@@ -409,6 +409,36 @@ describe("redactFindingSpans — machine-egress masking (#1947)", () => {
     });
     expect(out).toBe("line one\nline two has <REDACTED-github.pat>\nline three");
   });
+
+  // Pre-landing review CRITICAL: pem.private_key and gcp.service_account
+  // capture only the HEADER, not the key material — a span splice would
+  // redact the marker and forward the key body. Marker-only patterns must
+  // drop the whole payload.
+  test("PEM private key → null (header-only span must not forward the key body)", () => {
+    const msg =
+      "deploy failed: -----BEGIN PRIVATE KEY-----\nMIIEvQIBADANBgkqhkiG9w0BAQEFAASC\n-----END PRIVATE KEY-----";
+    expect(redactFindingSpans(msg, { repoVisibility: "private" })).toBeNull();
+  });
+
+  test("GCP service-account JSON → null (key body follows the captured marker)", () => {
+    const msg =
+      'config dump: {"private_key_id": "abc123", "private_key": "-----BEGIN PRIVATE KEY-----\\nMIIEvQIBADANBg..."}';
+    expect(redactFindingSpans(msg, { repoVisibility: "private" })).toBeNull();
+  });
+
+  // Pre-landing review: overlapping spans (a Bearer token that is also a
+  // JWT) must coalesce — independent splices apply stale offsets and can
+  // leave trailing secret bytes or mangled markers.
+  test("overlapping spans (Bearer JWT fires auth.bearer + jwt) never leak and produce clean markers", () => {
+    const jwt = "eyJ" + "a".repeat(20) + ".eyJ" + "b".repeat(20) + "." + "c".repeat(20);
+    const out = redactFindingSpans(`Authorization: Bearer ${jwt}`, { repoVisibility: "private" });
+    expect(out).not.toBeNull();
+    expect(out!).not.toContain("eyJ");
+    expect(out!).not.toContain("aaaa");
+    expect(out!).not.toContain("cccc");
+    // One coalesced, well-formed marker — no truncated fragments.
+    expect(out!).toMatch(/^Authorization: Bearer <REDACTED-[a-z._+]+>$/);
+  });
 });
 
 describe("taxonomy integrity", () => {

test/redact-prepush-hook.test.ts

@@ -127,6 +127,31 @@ describe("fail closed on unscannable diffs (#1946)", () => {
     const { code } = runHook(`refs/heads/main ${head} refs/heads/main ${head}\n`);
     expect(code).toBe(0);
   });
+
+  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 });
+    }
+  });
 });
 
 describe("install UX surfaces (#1946 / eng review D3+D10)", () => {

test/security-dashboard-fallback.test.ts

@@ -182,6 +182,13 @@ describe("gstack-security-dashboard — never reports fake zeros (#1947)", () =>
     expect(parsed.stale).toBe(true);
   });
 
+  it("stale snapshot is flagged in human mode too — frozen figures never read as current", () => {
+    const staleBody = JSON.stringify({ ...JSON.parse(GOOD_BODY_MARKER), stale: true });
+    const r = run(SEC_BIN, { mode: "ok", body: staleBody });
+    expect(r.stdout).toContain("Attacks detected last 7 days: 3");
+    expect(r.stdout).toContain("stale snapshot");
+  });
+
   it("200 without marker (legacy backend) → figures shown with unverified note", () => {
     const r = run(SEC_BIN, { mode: "ok", body: GOOD_BODY_LEGACY });
     expect(r.stdout).toContain("Attacks detected last 7 days: 3");
@@ -238,4 +245,13 @@ describe("gstack-community-dashboard — never reports fake zeros (#1947)", () =
     expect(r.stdout).toContain("unknown — backend error (HTTP 200)");
     expect(r.stdout).not.toContain("Weekly active installs:");
   });
+
+  it("whitespaced marker ('\"status\": \"ok\"') still classified as verified when jq is present", () => {
+    // Pre-landing review: the grep-only marker check was whitespace-sensitive;
+    // a proxy-reserialized body must not be misclassified as legacy.
+    const spaced = GOOD_BODY_MARKER.replace('"status":"ok"', '"status": "ok"');
+    const r = run(COMM_BIN, { mode: "ok", body: spaced });
+    expect(r.stdout).toContain("Weekly active installs: 42");
+    expect(r.stdout).not.toContain("unverified");
+  });
 });

test/telemetry.test.ts

@@ -222,12 +222,21 @@ describe('gstack-telemetry-log', () => {
   test('fails closed: error_message becomes null when the redactor is unavailable (#1947)', () => {
     setConfig('telemetry', 'anonymous');
     const token = 'ghp_' + 'A1b2C3d4E5f6G7h8I9j0K1l2M3n4O5p6Q7r8';
-    // PATH without bun: the redaction snippet cannot run, so the whole
-    // message must drop — never raw passthrough.
-    run(
-      `${BIN}/gstack-telemetry-log --skill qa --duration 10 --outcome error --error-message 'auth ${token} rejected' --session-id red-2`,
-      { PATH: '/usr/bin:/bin' },
-    );
+    // Shadow bun with a failing stub on a prepended PATH (deterministic on
+    // any host layout — pre-landing review flagged the bare '/usr/bin:/bin'
+    // variant as environment-dependent): the redaction snippet cannot run,
+    // so the whole message must drop — never raw passthrough.
+    const stubBin = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-tel-nobun-'));
+    try {
+      fs.writeFileSync(path.join(stubBin, 'bun'), '#!/bin/sh\nexit 127\n');
+      fs.chmodSync(path.join(stubBin, 'bun'), 0o755);
+      run(
+        `${BIN}/gstack-telemetry-log --skill qa --duration 10 --outcome error --error-message 'auth ${token} rejected' --session-id red-2`,
+        { PATH: `${stubBin}:${process.env.PATH}` },
+      );
+    } finally {
+      fs.rmSync(stubBin, { recursive: true, force: true });
+    }
 
     const lines = readJsonl();
     expect(lines).toHaveLength(1);
@@ -236,6 +245,21 @@ describe('gstack-telemetry-log', () => {
     expect(lines[0]).not.toContain(token);
   });
 
+  test('fails closed: PEM key in error_message drops the whole message (#1947 review fix)', () => {
+    setConfig('telemetry', 'anonymous');
+    // Header-only pattern: span replacement would forward the key body, so
+    // the engine returns null and the bin must store null.
+    run(
+      `${BIN}/gstack-telemetry-log --skill qa --duration 10 --outcome error --error-message 'deploy failed: -----BEGIN PRIVATE KEY----- MIIEvQIBADANBgkqhkiG9w0BAQEFAASC' --session-id red-5`,
+    );
+
+    const lines = readJsonl();
+    expect(lines).toHaveLength(1);
+    const event = JSON.parse(lines[0]);
+    expect(event.error_message).toBeNull();
+    expect(lines[0]).not.toContain('MIIEvQIBADAN');
+  });
+
   test('truncates error_message to 200 chars after redaction (#1947)', () => {
     setConfig('telemetry', 'anonymous');
     const long = 'x'.repeat(300);