test: fill coverage gaps for PRs #1606, #1612, #1620

Three cherry-picked PRs in this wave landed without unit-test coverage for
the specific invariant they protect:

  #1606 (@andrey-esipov) — LC_ALL=C pin in _gstack_gbrain_validate_varname
    8 tests by sourcing bin/gstack-gbrain-lib.sh and calling the validator
    directly. Asserts uppercase/digit/underscore accepted, lowercase
    REJECTED (the macOS-locale regression case), mixed-case rejected,
    LC_ALL=C scoping is local (doesn't leak to caller).

  #1612 (@bharat2913) — setsid daemonize via Node child_process.spawn
    4 static-invariant tests on browse/src/cli.ts. The actual setsid
    syscall is hard to assert without a real spawn, so we pin the source
    shape: nodeSpawn imported from child_process; non-Windows branch uses
    nodeSpawn(...) with detached:true and .unref(); comment documents
    setsid/SIGHUP root cause; Bun.spawn() is NOT used on macOS/Linux.

  #1620 (@davidfoy, re-authored into .tmpl per A3) — §4a-postfail
    12 static invariants on land-and-deploy/SKILL.md.tmpl + generated
    SKILL.md. Pins all three state branches (MERGED/OPEN/CLOSED), the
    authoritative state query, the merge-SHA capture, non-destructive
    worktree cleanup with uncommitted-work guard, autoMergeRequest probe
    on OPEN, hard "never retry gh pr merge" rule, and atomic regen
    propagation.

Failing build if any of the three invariants regresses.

Note: gbrain-lib-validate-varname.test.ts also surfaces a pre-existing
glob-pattern overpermissiveness (hyphens + dots accepted) — not in
#1606's scope; documented inline as a separate cleanup target.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

browse/test/cli-setsid-daemonize.test.ts

@@ -0,0 +1,75 @@
+/**
+ * Coverage for #1612 — macOS/Linux server must survive sandboxed-shell
+ * harnesses by becoming its own session leader (setsid).
+ *
+ * Pre-#1612, Bun.spawn().unref() removed the child from Bun's event loop
+ * but did NOT call setsid(). When the CLI ran inside Claude Code's
+ * per-command sandbox, Conductor, or CI step runners, the session leader's
+ * exit sent SIGHUP to every PID in the session, killing the bun server.
+ *
+ * The fix routes macOS/Linux spawn through Node's child_process.spawn with
+ * detached:true, which calls setsid() so the server becomes its own session
+ * leader (PPID=1 on Linux, similar reparenting on Darwin).
+ *
+ * The actual setsid syscall is hard to assert in a unit test without a
+ * real spawn — testing here is static: the cli.ts source must use the
+ * Node spawn path on macOS/Linux, with detached:true and .unref(). If a
+ * future refactor reverts to Bun.spawn().unref() on the macOS/Linux branch
+ * the regression returns and these tests fail.
+ */
+import { describe, expect, test } from "bun:test";
+import * as fs from "node:fs";
+import * as path from "node:path";
+
+const ROOT = path.resolve(import.meta.dir, "..", "..");
+const CLI = path.join(ROOT, "browse", "src", "cli.ts");
+
+function read(): string {
+  return fs.readFileSync(CLI, "utf-8");
+}
+
+describe("#1612 macOS/Linux daemonize via Node setsid path", () => {
+  test("cli.ts imports nodeSpawn from child_process (Node spawn alias)", () => {
+    const body = read();
+    // The fix relies on Node's child_process.spawn (which calls setsid on
+    // detached:true), aliased to avoid name collision with Bun.spawn. Match
+    // either `nodeSpawn` or `spawn as nodeSpawn` to be flexible to the
+    // exact import style.
+    expect(body).toMatch(/(spawn as nodeSpawn|nodeSpawn\s*[,}])/);
+    expect(body).toMatch(/from\s+['"]child_process['"]/);
+  });
+
+  test("non-Windows branch uses nodeSpawn(...).unref() with detached:true", () => {
+    const body = read();
+    // Find the non-Windows branch and assert it uses the Node spawn alias
+    // with detached:true. Match the pattern `nodeSpawn(...) ... detached:true`.
+    expect(body).toMatch(/nodeSpawn\([\s\S]{0,500}detached:\s*true/);
+    expect(body).toMatch(/nodeSpawn\([\s\S]{0,500}\.unref\(\)/);
+  });
+
+  test("non-Windows branch comment documents setsid/SIGHUP root cause", () => {
+    const body = read();
+    // The comment block must mention setsid() so a future refactor sees the
+    // why before changing the spawn call.
+    expect(body).toMatch(/setsid/);
+    expect(body).toMatch(/SIGHUP/);
+  });
+
+  test("the spawn call on macOS/Linux is nodeSpawn, not Bun.spawn", () => {
+    const body = read();
+    // Strip line comments before regex matching, so the "Bun.spawn().unref()"
+    // mentions inside the explanatory comment don't trigger false positives.
+    const codeOnly = body
+      .split("\n")
+      .filter((line) => !line.trim().startsWith("//"))
+      .join("\n");
+    // Find the non-Windows branch. The `} else {` block following the
+    // Windows branch. We then require its first ~400 chars contain a
+    // nodeSpawn() call and NOT a Bun.spawn() call (excluding the comment).
+    const nonWindowsStart = codeOnly.indexOf("nodeSpawn('bun'");
+    expect(nonWindowsStart).toBeGreaterThan(-1);
+    const slice = codeOnly.slice(nonWindowsStart, nonWindowsStart + 400);
+    expect(slice).toMatch(/nodeSpawn\(/);
+    expect(slice).not.toMatch(/Bun\.spawn\(/);
+  });
+});

test/gbrain-lib-validate-varname.test.ts

@@ -0,0 +1,98 @@
+/**
+ * Coverage for #1606 — `_gstack_gbrain_validate_varname` LC_ALL=C pin.
+ *
+ * Without the `local LC_ALL=C`, macOS default locale (en_US.UTF-8) makes
+ * `case "$name" in [A-Z_][A-Z0-9_]*)` match lowercase letters too —
+ * lower-case identifiers pass validation and then trip `printf -v "$varname"`
+ * with "not a valid identifier" the caller can't distinguish from other
+ * failures.
+ *
+ * Tests exercise the validator by sourcing bin/gstack-gbrain-lib.sh and
+ * calling _gstack_gbrain_validate_varname directly. Asserts:
+ *   - Valid uppercase identifiers accepted (return 0)
+ *   - Lowercase identifiers REJECTED (return 2) — pre-#1606 regression case
+ *   - Mixed-case rejected
+ *   - Empty name rejected
+ *   - Names starting with digit rejected
+ *   - Underscore prefix accepted
+ *   - LC_ALL=C does not leak to caller (local scope preserved)
+ */
+import { describe, expect, test } from "bun:test";
+import { spawnSync } from "node:child_process";
+import * as path from "node:path";
+
+const ROOT = path.resolve(import.meta.dir, "..");
+const LIB = path.join(ROOT, "bin", "gstack-gbrain-lib.sh");
+
+function runValidator(name: string): { status: number | null } {
+  // Source the lib then run the validator against the input. Use bash -c with
+  // single-quoted body to avoid double interpolation. LANG=en_US.UTF-8 set
+  // explicitly so the test catches the macOS locale FP case even when CI's
+  // default locale would mask it.
+  const result = spawnSync(
+    "bash",
+    ["-c", `. "${LIB}"; _gstack_gbrain_validate_varname "$1"`, "bash", name],
+    {
+      encoding: "utf-8",
+      timeout: 5000,
+      env: { ...process.env, LANG: "en_US.UTF-8", LC_ALL: "en_US.UTF-8" },
+    },
+  );
+  return { status: result.status };
+}
+
+describe("#1606 _gstack_gbrain_validate_varname — LC_ALL=C pin", () => {
+  test("ACCEPTS uppercase identifier (canonical happy path)", () => {
+    expect(runValidator("DATABASE_URL").status).toBe(0);
+  });
+
+  test("ACCEPTS uppercase + digits + underscores", () => {
+    expect(runValidator("GBRAIN_DB_URL_v2".toUpperCase()).status).toBe(0);
+    expect(runValidator("X1_2_3").status).toBe(0);
+  });
+
+  test("ACCEPTS underscore-prefixed identifier", () => {
+    expect(runValidator("_PRIVATE_VAR").status).toBe(0);
+  });
+
+  test("REJECTS lowercase identifier (#1606 regression — would pass on macOS without LC_ALL=C)", () => {
+    expect(runValidator("lower_case").status).toBe(2);
+  });
+
+  test("REJECTS mixed-case identifier", () => {
+    expect(runValidator("MixedCase").status).toBe(2);
+    expect(runValidator("camelCase").status).toBe(2);
+  });
+
+  test("REJECTS name starting with digit", () => {
+    expect(runValidator("1ABC").status).toBe(2);
+  });
+
+  test("REJECTS empty name", () => {
+    expect(runValidator("").status).toBe(2);
+  });
+
+  // Note: hyphen/dot acceptance is a pre-existing overpermissiveness in the
+  // glob pattern `[A-Z_][A-Z0-9_]*` — `*` matches any chars after the bracket
+  // class. NOT in scope for #1606; tracked separately for a future cleanup
+  // wave. Tests intentionally do not assert hyphen/dot rejection so this
+  // file doesn't regress when that future fix lands.
+
+  test("LC_ALL=C is local to the validator (does not leak to caller)", () => {
+    // After sourcing + calling the validator, $LC_ALL in the caller scope
+    // must remain whatever LANG/LC_ALL the caller set. We seed LC_ALL with a
+    // distinctive value, call the validator, then print $LC_ALL — the
+    // distinctive value must survive.
+    const result = spawnSync(
+      "bash",
+      ["-c", `. "${LIB}"; LC_ALL=fr_FR.UTF-8; _gstack_gbrain_validate_varname FOO; echo "$LC_ALL"`],
+      {
+        encoding: "utf-8",
+        timeout: 5000,
+        env: { ...process.env, LANG: "en_US.UTF-8" },
+      },
+    );
+    expect(result.status).toBe(0);
+    expect(result.stdout.trim()).toBe("fr_FR.UTF-8");
+  });
+});

test/land-and-deploy-postfail.test.ts

@@ -0,0 +1,111 @@
+/**
+ * Coverage for PR #1620 — Post-failure PR-state check after `gh pr merge`
+ * non-zero exit.
+ *
+ * The fix lives in land-and-deploy/SKILL.md.tmpl as Step §4a-postfail.
+ * After ANY non-zero `gh pr merge`, the skill must query authoritative PR
+ * state via `gh pr view --json state,mergeCommit,mergedAt,mergedBy` and
+ * branch on the result instead of retrying `gh pr merge` (cli/cli#3442,
+ * cli/cli#13380).
+ *
+ * Static invariants pin:
+ *   - §4a-postfail header present
+ *   - Universal invariant text + reference to upstream gh bugs
+ *   - All three state branches (MERGED, OPEN, CLOSED) named explicitly
+ *   - MERGED branch: capture merge SHA via mergeCommit.oid
+ *   - MERGED branch: non-destructive worktree cleanup with uncommitted-work guard
+ *   - MERGED branch: continues to §4a CI watch
+ *   - OPEN branch: checks autoMergeRequest before treating as failure
+ *   - CLOSED branch: STOPs
+ *   - Hard rule: never retry `gh pr merge`
+ *   - .tmpl edit propagated to generated SKILL.md (atomic per T-Codex-3)
+ */
+import { describe, expect, test } from "bun:test";
+import * as fs from "node:fs";
+import * as path from "node:path";
+
+const ROOT = path.resolve(import.meta.dir, "..");
+const TMPL = path.join(ROOT, "land-and-deploy", "SKILL.md.tmpl");
+const MD = path.join(ROOT, "land-and-deploy", "SKILL.md");
+
+function readTmpl(): string {
+  return fs.readFileSync(TMPL, "utf-8");
+}
+function readMd(): string {
+  return fs.readFileSync(MD, "utf-8");
+}
+
+describe("PR #1620 §4a-postfail in land-and-deploy template", () => {
+  test("§4a-postfail header present in template", () => {
+    expect(readTmpl()).toMatch(/### 4a-postfail: Post-failure PR-state check/);
+  });
+
+  test("§4a-postfail comes before §4a (Merge queue detection)", () => {
+    const body = readTmpl();
+    const postfail = body.indexOf("### 4a-postfail:");
+    const queue = body.indexOf("### 4a: Merge queue detection");
+    expect(postfail).toBeGreaterThan(-1);
+    expect(queue).toBeGreaterThan(-1);
+    expect(postfail).toBeLessThan(queue);
+  });
+
+  test("Universal invariant + upstream gh bug references", () => {
+    const body = readTmpl();
+    expect(body).toMatch(/Universal invariant/);
+    expect(body).toMatch(/non-zero exit from `gh pr merge`/);
+    expect(body).toMatch(/cli\/cli#3442/);
+    expect(body).toMatch(/cli\/cli#13380/);
+  });
+
+  test("Authoritative state query uses gh pr view --json", () => {
+    const body = readTmpl();
+    expect(body).toMatch(/gh pr view --json state,mergeCommit,mergedAt,mergedBy/);
+  });
+
+  test("All three state branches named: MERGED, OPEN, CLOSED", () => {
+    const body = readTmpl();
+    expect(body).toMatch(/state == "MERGED"/);
+    expect(body).toMatch(/state == "OPEN"/);
+    expect(body).toMatch(/state == "CLOSED"/);
+  });
+
+  test("MERGED branch captures merge SHA via mergeCommit.oid", () => {
+    const body = readTmpl();
+    expect(body).toMatch(/gh pr view --json mergeCommit -q \.mergeCommit\.oid/);
+  });
+
+  test("MERGED worktree cleanup is non-destructive (uncommitted-work guard)", () => {
+    const body = readTmpl();
+    expect(body).toMatch(/uncommitted work/);
+    expect(body).toMatch(/STOP worktree cleanup without removing/);
+    expect(body).toMatch(/Do NOT use `--force`/);
+    expect(body).toMatch(/Do NOT remove the user's primary working tree/);
+  });
+
+  test("MERGED branch continues to §4a CI auto-deploy detection", () => {
+    const body = readTmpl();
+    expect(body).toMatch(/continue to §4a/);
+  });
+
+  test("OPEN branch checks autoMergeRequest before treating as failure", () => {
+    const body = readTmpl();
+    expect(body).toMatch(/gh pr view --json autoMergeRequest/);
+    expect(body).toMatch(/auto-merge is enabled or merge queue is in use/);
+  });
+
+  test("CLOSED branch STOPs", () => {
+    const body = readTmpl();
+    expect(body).toMatch(/state == "CLOSED".*[\s\S]{0,200}STOP/);
+  });
+
+  test("Hard rule: never retry gh pr merge after non-zero exit", () => {
+    const body = readTmpl();
+    expect(body).toMatch(/never call `gh pr merge` a second time/);
+  });
+
+  test("Generated SKILL.md carries the §4a-postfail section (atomic regen per T-Codex-3)", () => {
+    const md = readMd();
+    expect(md).toMatch(/### 4a-postfail: Post-failure PR-state check/);
+    expect(md).toMatch(/state == "MERGED"/);
+  });
+});