From e75a5e8e5f8184a3a46842a117dd6efbaba4af78 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 21 May 2026 10:03:18 -0700 Subject: [PATCH] test: fill coverage gaps for PRs #1606, #1612, #1620 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- browse/test/cli-setsid-daemonize.test.ts | 75 +++++++++++++++ test/gbrain-lib-validate-varname.test.ts | 98 ++++++++++++++++++++ test/land-and-deploy-postfail.test.ts | 111 +++++++++++++++++++++++ 3 files changed, 284 insertions(+) create mode 100644 browse/test/cli-setsid-daemonize.test.ts create mode 100644 test/gbrain-lib-validate-varname.test.ts create mode 100644 test/land-and-deploy-postfail.test.ts diff --git a/browse/test/cli-setsid-daemonize.test.ts b/browse/test/cli-setsid-daemonize.test.ts new file mode 100644 index 000000000..fb425c1b0 --- /dev/null +++ b/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\(/); + }); +}); diff --git a/test/gbrain-lib-validate-varname.test.ts b/test/gbrain-lib-validate-varname.test.ts new file mode 100644 index 000000000..d29130f95 --- /dev/null +++ b/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"); + }); +}); diff --git a/test/land-and-deploy-postfail.test.ts b/test/land-and-deploy-postfail.test.ts new file mode 100644 index 000000000..f89d77518 --- /dev/null +++ b/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"/); + }); +});