From c3bee68a602faf10a48ba7dc0112ea37d4183c0d Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Sat, 18 Apr 2026 23:20:29 +0800 Subject: [PATCH] fix(ship): harden Step 12 against whitespace + invalid REPAIR_VERSION Claude adversarial subagent surfaced three correctness risks in the Step 12 state machine: - CURRENT_VERSION and BASE_VERSION were not stripped of CR/whitespace on read. A CRLF VERSION file would mismatch the clean package.json version, falsely classify as DRIFT_STALE_PKG, then propagate the carriage return into package.json via the repair path. - REPAIR_VERSION was unvalidated. The bump path validates NEW_VERSION against the 4-digit semver pattern, but the drift-repair path wrote whatever cat VERSION returned directly into package.json. A manually-corrupted VERSION file would silently poison the repair. - Empty-string CURRENT_VERSION (0-byte VERSION, directory-at-VERSION) fell through to "not equal to base" and misclassified as ALREADY_BUMPED. Template fix strips \r/newlines/whitespace on every VERSION read, guards against empty-string results, and applies the same semver regex gate in the repair path that already protects the bump path. Adds two regression tests (trailing-CR idempotency + invalid-semver repair rejection). Total Step 12 coverage: 14 tests, 14/14 pass. Opens two follow-up TODOs flagged but not fixed in this branch: test/template drift risk (the tests still reimplement template bash) and BASE_VERSION silent fallback on git-show failure. Co-Authored-By: Claude Opus 4.7 (1M context) --- TODOS.md | 24 ++++++++++++++++++++++++ ship/SKILL.md.tmpl | 12 +++++++++--- test/ship-version-sync.test.ts | 30 ++++++++++++++++++++++++++++-- 3 files changed, 61 insertions(+), 5 deletions(-) diff --git a/TODOS.md b/TODOS.md index 3b28fc2e..d3354110 100644 --- a/TODOS.md +++ b/TODOS.md @@ -437,6 +437,30 @@ Linux cookie import shipped in v0.11.11.0 (Wave 3). Supports Chrome, Chromium, B ## Ship +### /ship Step 12 test harness should exec the actual template bash, not a reimplementation + +**What:** `test/ship-version-sync.test.ts` currently reimplements the bash from `ship/SKILL.md.tmpl` Step 12 inside template literals. When the template changes, both sides must be updated — exactly the drift-risk pattern the Step 12 fix is meant to prevent, applied to our own testing strategy. Replace with a helper that extracts the fenced bash blocks from the template at test time and runs them verbatim (similar to the `skill-parser.ts` pattern). + +**Why:** Surfaced by the Claude adversarial subagent during the v1.0.1.0 ship. Today the tests would stay green while the template regresses, because the error-message strings already differ between test and template. It's a silent-drift bug waiting to happen. + +**Context:** The fixed test file is at `test/ship-version-sync.test.ts` (branched off garrytan/ship-version-sync). Existing precedent for extracting-from-skill-md is at `test/helpers/skill-parser.ts`. Pattern: read the template, slice from `## Step 12` to the next `---`, grep fenced bash, feed to `/bin/bash` with substituted fixtures. + +**Effort:** S (human: ~2h / CC: ~30min) +**Priority:** P2 +**Depends on:** None. + +### /ship Step 12 BASE_VERSION silent fallback to 0.0.0.0 when git show fails + +**What:** `BASE_VERSION=$(git show origin/:VERSION 2>/dev/null || echo "0.0.0.0")` silently defaults to `0.0.0.0` in any failure mode — detached HEAD, no origin, offline, base branch renamed. In such states, a real drift could be misclassified or silently repaired with the wrong value. Distinguish "origin/ unreachable" from "origin/:VERSION absent" and fail loudly on the former. + +**Why:** Flagged as CRITICAL (confidence 8/10) by the Claude adversarial subagent during the v1.0.1.0 ship. Low practical risk because `/ship` Step 3 already fetches origin before Step 12 runs — any reachability failure would abort Step 3 long before this code runs. Still, defense in depth: if someone invokes Step 12 bash outside the full /ship pipeline (e.g., via a standalone helper), the fallback masks a real problem. + +**Context:** Fix: wrap with `git rev-parse --verify origin/` probe; if that fails, error out rather than defaulting. Touches `ship/SKILL.md.tmpl` Step 12 idempotency block (around line 409). Tests need a case where `git show` fails. + +**Effort:** S (human: ~1h / CC: ~15min) +**Priority:** P3 +**Depends on:** None. + ### GitLab support for /land-and-deploy **What:** Add GitLab MR merge + CI polling support to `/land-and-deploy` skill. Currently uses `gh pr view`, `gh pr checks`, `gh pr merge`, and `gh run list/view` in 15+ places — each needs a GitLab conditional path using `glab ci status`, `glab mr merge`, etc. diff --git a/ship/SKILL.md.tmpl b/ship/SKILL.md.tmpl index 377d87be..75c73ccf 100644 --- a/ship/SKILL.md.tmpl +++ b/ship/SKILL.md.tmpl @@ -406,8 +406,10 @@ For each comment in `comments`: **Idempotency check:** Before bumping, classify the state by comparing `VERSION` against the base branch AND against `package.json`'s `version` field. Four states: FRESH (do bump), ALREADY_BUMPED (skip bump), DRIFT_STALE_PKG (sync pkg only, no re-bump), DRIFT_UNEXPECTED (stop and ask). ```bash -BASE_VERSION=$(git show origin/:VERSION 2>/dev/null || echo "0.0.0.0") -CURRENT_VERSION=$(cat VERSION 2>/dev/null || echo "0.0.0.0") +BASE_VERSION=$(git show origin/:VERSION 2>/dev/null | tr -d '\r\n[:space:]' || echo "0.0.0.0") +CURRENT_VERSION=$(cat VERSION 2>/dev/null | tr -d '\r\n[:space:]' || echo "0.0.0.0") +[ -z "$BASE_VERSION" ] && BASE_VERSION="0.0.0.0" +[ -z "$CURRENT_VERSION" ] && CURRENT_VERSION="0.0.0.0" PKG_VERSION="" PKG_EXISTS=0 if [ -f package.json ]; then @@ -496,7 +498,11 @@ fi **DRIFT_STALE_PKG repair path** — runs when idempotency reports `STATE: DRIFT_STALE_PKG`. No re-bump; sync `package.json.version` to the current `VERSION` and continue. Reuse `CURRENT_VERSION` for CHANGELOG and PR body. ```bash -REPAIR_VERSION=$(cat VERSION) +REPAIR_VERSION=$(cat VERSION | tr -d '\r\n[:space:]') +if ! printf '%s' "$REPAIR_VERSION" | grep -qE '^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+$'; then + echo "ERROR: VERSION file contents ($REPAIR_VERSION) do not match MAJOR.MINOR.PATCH.MICRO pattern. Refusing to propagate invalid semver into package.json. Fix VERSION manually, then re-run /ship." + exit 1 +fi if command -v node >/dev/null 2>&1; then node -e 'const fs=require("fs"),p=require("./package.json");p.version=process.argv[1];fs.writeFileSync("package.json",JSON.stringify(p,null,2)+"\n")' "$REPAIR_VERSION" || { echo "ERROR: drift repair failed — could not update package.json." diff --git a/test/ship-version-sync.test.ts b/test/ship-version-sync.test.ts index 80908ec1..c657795c 100644 --- a/test/ship-version-sync.test.ts +++ b/test/ship-version-sync.test.ts @@ -37,7 +37,8 @@ const idempotency = (base: string): { stdout: string; code: number } => { const script = ` cd "${dir}" || exit 2 BASE_VERSION="${base}" -CURRENT_VERSION=$(cat VERSION 2>/dev/null || echo "0.0.0.0") +CURRENT_VERSION=$(cat VERSION 2>/dev/null | tr -d '\\r\\n[:space:]' || echo "0.0.0.0") +[ -z "$CURRENT_VERSION" ] && CURRENT_VERSION="0.0.0.0" PKG_VERSION="" PKG_EXISTS=0 if [ -f package.json ]; then @@ -97,7 +98,10 @@ fi`; const syncRepair = (): { code: number } => { const script = ` cd "${dir}" || exit 2 -REPAIR_VERSION=$(cat VERSION) +REPAIR_VERSION=$(cat VERSION | tr -d '\\r\\n[:space:]') +if ! printf '%s' "$REPAIR_VERSION" | grep -qE '^[0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+$'; then + echo "invalid repair semver" >&2; exit 1 +fi node -e 'const fs=require("fs"),p=require("./package.json");p.version=process.argv[1];fs.writeFileSync("package.json",JSON.stringify(p,null,2)+"\\n")' "$REPAIR_VERSION"`; try { execSync(script, { shell: "/bin/bash", stdio: "pipe" }); @@ -183,6 +187,28 @@ test("bump: no package.json is silent", () => { expect(existsSync(join(dir, "package.json"))).toBe(false); }); +// --- Adversarial review regressions: trailing whitespace + invalid REPAIR_VERSION --- + +test("trailing CR in VERSION does not cause false DRIFT_STALE_PKG", () => { + // Before the tr-strip fix, VERSION="0.1.0.0\r" read via cat would mismatch + // pkg.version="0.1.0.0" and classify as DRIFT_STALE_PKG, then repair would + // write garbage \r into package.json. Now CURRENT_VERSION is stripped. + writeFileSync(join(dir, "VERSION"), "0.1.0.0\r\n"); + writeFileSync(join(dir, "package.json"), pkgJson("0.1.0.0")); + expect(idempotency("0.0.0.0")).toEqual({ stdout: "STATE: ALREADY_BUMPED", code: 0 }); +}); + +test("DRIFT REPAIR rejects invalid VERSION semver instead of propagating", () => { + // If VERSION is corrupted/manually-edited to something non-semver, the + // repair path must refuse rather than writing junk into package.json. + writeFileSync(join(dir, "VERSION"), "not-a-semver\n"); + writeFileSync(join(dir, "package.json"), pkgJson("0.0.0.0")); + const r = syncRepair(); + expect(r.code).toBe(1); + // package.json must NOT have been overwritten with the garbage. + expect(pkgVersion()).toBe("0.0.0.0"); +}); + // --- THE critical regression test: drift-repair does NOT double-bump --- test("DRIFT REPAIR: sync path syncs pkg to VERSION without re-bumping", () => {