From b613e2d4d99e9215b2de883a677ba2f5805697a1 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 29 Jul 2026 12:40:21 +1000 Subject: [PATCH 1/9] fix(todo-markers): guard three false-verdict paths in stale-marker check Refactor scripts/check-stale-todo-markers.mjs into pure / dependency-injected exports (behind an import-guarded main()) and fix three false-verdict paths: - (a) reused head-branch: prefer an OPEN PR over an older same-named MERGED one, so still-in-flight work is never flagged stale. - (b) number-only (bare (#N) / (PR #N)) matches downgrade to a report-only warning, never a hard STALE verdict (PR numbers are reused across repo generations). - (c) a transient gh error skips only that entry and keeps scanning; gh being unavailable (ENOENT / auth / rate-limit) stops the loop but still returns already-confirmed stale, and a taxonomy-external bug surfaces as exit 2 (never colliding with stale's exit 1). Add test/build/stale-todo-markers.test.ts (23 in-memory cases with a fake ghRunner) pinning all three plus the error taxonomy and honest exit codes. --- scripts/check-stale-todo-markers.mjs | 309 +++++++++++++++++++------- test/build/stale-todo-markers.test.ts | 280 +++++++++++++++++++++++ 2 files changed, 506 insertions(+), 83 deletions(-) create mode 100644 test/build/stale-todo-markers.test.ts diff --git a/scripts/check-stale-todo-markers.mjs b/scripts/check-stale-todo-markers.mjs index 860082d9..c326955a 100755 --- a/scripts/check-stale-todo-markers.mjs +++ b/scripts/check-stale-todo-markers.mjs @@ -14,10 +14,42 @@ // Design notes: // - No new dependencies: it shells out to the `gh` CLI (already a dev tool) // via execFileSync (argv array, no shell — no injection surface). -// - Fail-soft: if gh is missing or unauthenticated it warns and exits 0, -// so a contributor without gh isn't blocked. There is no CI equivalent — -// the TODO file lives under .claude/ (gitignored, not checked in), so -// this script is intentionally local-only (see CLAUDE.md). +// - Fail-soft & advisory: this is a LOCAL-only check (the TODO file lives +// under .claude/, gitignored, absent from CI checkouts), so it never hard- +// blocks a contributor. A missing/unauthenticated/rate-limited `gh` warns +// and exits 0. +// - The pure / dependency-injected exports (`parseEntry`, `resolveEntry`, +// `scanTodo`, `summarize`, `runScan`, `classifyGhError`) are unit-tested +// with in-memory fixtures and a fake `ghRunner`; `main()` is a thin fs + +// console + process.exit wrapper behind the import-guard at the bottom. +// +// Verdict correctness (three false-verdict paths this file deliberately avoids): +// (a) REUSED head-branch name — a branch name may be reused after its first +// PR merged. `resolveEntry` prefers an OPEN PR for the branch, so still- +// in-flight work is never flagged stale by an older same-named merged PR. +// (b) NUMBER-ONLY match — PR numbers are reused across repo generations, so a +// match resting solely on a `(#N)` / `(PR #N)` number is REPORT-ONLY +// (a warning), never a hard STALE verdict. A conforming in-flight entry +// names a `(branch: X)` (the marker `check-todo-hygiene.mjs` lints for) +// and resolves via the branch path, which still exits 1 when stale. +// (c) TRANSIENT gh failure — a one-off 404 / network blip skips only THAT +// entry (recorded for a partial-scan warning) and keeps scanning; it does +// NOT bail the whole loop. See the error taxonomy below. +// +// Error taxonomy (see `classifyGhError`): +// - GhUnavailable — `gh` is unusable for the REST of this run: not installed +// (ENOENT), not authenticated, or rate-limited. All three persist for the +// run, so re-trying every remaining entry is futile; the scan stops but +// still RETURNS what it already found (a stale marker confirmed before a +// mid-scan auth expiry must not be silently discarded). +// - GhTransient — a genuinely per-entry failure (single 404, one-off network +// blip, unparseable JSON): skip that entry, continue. +// Any OTHER error is a real bug and propagates (never swallowed), surfacing as +// exit 2 — kept distinct from the stale exit 1 so a crash can't read as +// "stale found". +// +// Exit codes: 0 = clean or partial scan (fail-soft), 1 = a genuine stale +// marker, 2 = TODO unreadable or an internal error. // // Usage: // node scripts/check-stale-todo-markers.mjs [path-to-todo.md] @@ -25,34 +57,59 @@ import { execFileSync } from "node:child_process"; import { readFileSync } from "node:fs"; +import { pathToFileURL } from "node:url"; const DEFAULT_TODO = ".claude/docs/TODO.md"; -const todoPath = process.argv[2] ?? DEFAULT_TODO; - -let text; -try { - text = readFileSync(todoPath, "utf8"); -} catch (err) { - console.error(`check-stale-todo-markers: cannot read ${todoPath}: ${err.message}`); - process.exit(2); + +// `gh` is unusable for the rest of this run (ENOENT / auth / rate limit). +export class GhUnavailable extends Error {} +// A single-entry `gh` failure (one 404, a network blip, unparseable JSON). +export class GhTransient extends Error {} + +// Map a raw execFile/JSON error to the taxonomy. Pure — fake error objects in, +// the right class out — so the classification is unit-testable without a real +// `gh`. ENOENT (not installed), auth failures, and rate limits all persist for +// the run → GhUnavailable; everything else is treated as per-entry transient. +export function classifyGhError(err) { + const msg = `${err?.stderr ?? ""}${err?.message ?? ""}`; + if ( + err?.code === "ENOENT" || + /not logged|authentication|gh auth login|GH_TOKEN|rate limit/i.test(msg) + ) { + return new GhUnavailable(msg.trim() || String(err)); + } + return new GhTransient(msg.trim() || String(err)); +} + +// Run `gh` and return parsed JSON. `exec` is injectable so tests can exercise +// the JSON.parse → GhTransient path and the happy path without a real binary. +// Throws GhUnavailable / GhTransient per classifyGhError; a JSON parse failure +// is transient (one bad response for one entry). +export function ghExec(args, exec = execFileSync) { + let out; + try { + out = exec("gh", args, { encoding: "utf8", stdio: ["ignore", "pipe", "pipe"] }); + } catch (err) { + throw classifyGhError(err); + } + try { + return JSON.parse(out); + } catch (err) { + throw new GhTransient(`unparseable gh output: ${err.message}`); + } } // An ACTIVE in-flight entry is an unchecked task line carrying the 🚧 glyph. // Checked (`- [x]`) lines are done and never in-flight, so they are skipped. -const inflight = text - .split("\n") - .filter((line) => /^\s*-\s*\[ \]/.test(line) && line.includes("🚧")); - -if (inflight.length === 0) { - console.log("check-stale-todo-markers: no 🚧 in-flight entries — OK"); - process.exit(0); +export function parseInflight(text) { + return text.split("\n").filter((line) => /^\s*-\s*\[ \]/.test(line) && line.includes("🚧")); } // Extract the branch and/or PR number from one entry line. Both the modern // `(branch: X)` form and the legacy `` HTML marker are // recognized (the same two forms /merge-pr's sync understands). PR numbers // appear as `(PR #123)` or a bare `(#123)`. -function parseEntry(line) { +export function parseEntry(line) { const branch = line.match(/\(branch:\s*([^)]+?)\s*\)/)?.[1] ?? line.match(//)?.[1] ?? @@ -63,34 +120,16 @@ function parseEntry(line) { return { branch, pr, title }; } -// Run gh and return parsed JSON, or null if gh is unavailable/unauthenticated -// so the caller can decide to warn-and-pass rather than hard-fail. -function gh(args) { - try { - const out = execFileSync("gh", args, { encoding: "utf8", stdio: ["ignore", "pipe", "pipe"] }); - return JSON.parse(out); - } catch (err) { - const msg = `${err.stderr ?? ""}${err.message ?? ""}`; - if ( - err.code === "ENOENT" || // gh not installed - /not logged|authentication|gh auth login|GH_TOKEN/i.test(msg) - ) { - throw new GhUnavailable(msg.trim()); - } - // Other failures (e.g. transient API error) — treat as unavailable too, - // since a guard that hard-fails on a flaky network would be worse than - // one that warns. CI re-runs catch genuinely stale markers. - throw new GhUnavailable(msg.trim() || String(err)); - } -} - -class GhUnavailable extends Error {} - -// Resolve whether an entry's PR has already merged. Prefer the branch lookup -// (covers entries with only a branch); fall back to the PR number. -function resolveMerged({ branch, pr }) { +// Resolve one entry against GitHub via the injected `ghRunner`. +// Returns { status: "clean" | "stale" | "warn", merged?: { number, title } }. +// - branch path (authoritative): fix (a) — an OPEN PR for the head branch +// means the work is in flight, so it is NEVER stale, even if an OLDER +// same-named PR merged. Only "no OPEN + a MERGED" → stale. +// - number-only path: fix (b) — a match resting solely on a PR number is a +// WARNING, never stale (PR numbers are reused across repo generations). +export function resolveEntry({ branch, pr }, ghRunner) { if (branch) { - const prs = gh([ + const prs = ghRunner([ "pr", "list", "--head", @@ -102,63 +141,167 @@ function resolveMerged({ branch, pr }) { "--limit", "5", ]); + if (prs.some((p) => p.state === "OPEN")) { + return { status: "clean" }; // in flight — never stale (fix (a)) + } const merged = prs.find((p) => p.state === "MERGED"); if (merged) { - return { number: merged.number, title: merged.title }; + return { status: "stale", merged: { number: merged.number, title: merged.title } }; } - return null; + return { status: "clean" }; } if (pr) { - const view = gh(["pr", "view", pr, "--json", "state,title,mergedAt"]); + const view = ghRunner(["pr", "view", pr, "--json", "state,title,mergedAt"]); if (view.state === "MERGED") { - return { number: Number(pr), title: view.title }; + // Report-only: a number can collide across repo generations (fix (b)). + return { status: "warn", merged: { number: Number(pr), title: view.title } }; } - return null; + return { status: "clean" }; } - return null; + return { status: "clean" }; } -const stale = []; -try { +// Walk every in-flight entry, resolving each against `ghRunner`. +// Returns { inflightCount, stale, warnings, skipped, aborted }. +// - GhTransient → record in `skipped`, keep scanning (fix (c)). +// - GhUnavailable → record `aborted` and STOP, but still return everything +// found so far (never discard a stale marker confirmed before the abort). +// - any other error → rethrow (a real bug must not be swallowed into a +// silent clean/continue). +export function scanTodo(text, ghRunner = ghExec) { + const inflight = parseInflight(text); + const stale = []; + const warnings = []; + const skipped = []; + let aborted = null; + for (const line of inflight) { const entry = parseEntry(line); if (!entry.branch && !entry.pr) { continue; // nothing to resolve against } - const merged = resolveMerged(entry); - if (merged) { - stale.push({ entry, merged }); + let result; + try { + result = resolveEntry(entry, ghRunner); + } catch (err) { + if (err instanceof GhTransient) { + skipped.push({ entry, reason: err.message }); + continue; + } + if (err instanceof GhUnavailable) { + aborted = { reason: err.message }; + break; + } + throw err; // taxonomy-external → a real bug, surface it (Finding 2) + } + if (result.status === "stale") { + stale.push({ entry, merged: result.merged }); + } else if (result.status === "warn") { + warnings.push({ entry, merged: result.merged }); } } -} catch (err) { - if (err instanceof GhUnavailable) { - console.warn( - `check-stale-todo-markers: gh unavailable/unauthenticated — skipping check.\n (${err.message})` + + return { inflightCount: inflight.length, stale, warnings, skipped, aborted }; +} + +// Pure: turn a scan result into an exit code + message lines (no I/O, no +// throw). Exit 1 iff a genuine stale marker exists; otherwise 0 (fail-soft). +// The summary line always says "partial scan" when any entry was skipped or +// the scan aborted, so exit 0 never reads as "all verified clean". +export function summarize(scan) { + const out = []; + const err = []; + const { inflightCount, stale, warnings, skipped, aborted } = scan; + + if (inflightCount === 0) { + out.push("check-stale-todo-markers: no 🚧 in-flight entries — OK"); + return { exitCode: 0, out, err }; + } + + const partial = skipped.length > 0 || Boolean(aborted); + + for (const { entry, merged } of warnings) { + err.push(` ⚠ ${entry.title}`); + err.push( + ` PR #${merged.number} looks MERGED (number-only match — verify): ${merged.title}` + ); + } + for (const { entry, reason } of skipped) { + err.push(` … skipped (transient gh error): ${entry.title}\n (${reason})`); + } + if (aborted) { + err.push( + ` … scan stopped early — gh became unavailable; remaining entries unchecked.\n (${aborted.reason})` ); - process.exit(0); } - throw err; -} -if (stale.length === 0) { - console.log( - `check-stale-todo-markers: ${inflight.length} 🚧 in-flight ${ - inflight.length === 1 ? "entry" : "entries" - } checked, none stale — OK` + if (stale.length === 0) { + const scope = partial ? "partial scan" : "checked"; + const noun = inflightCount === 1 ? "entry" : "entries"; + out.push( + `check-stale-todo-markers: ${inflightCount} 🚧 in-flight ${noun} (${scope}), none stale — OK` + + (warnings.length ? ` (${warnings.length} number-only warning(s) above)` : "") + ); + return { exitCode: 0, out, err }; + } + + err.unshift( + `check-stale-todo-markers: ${stale.length} STALE 🚧 marker(s)` + + (partial ? " (partial scan — some entries unchecked)" : "") + + "\n" + ); + for (const { entry, merged } of stale) { + const via = entry.branch ? `branch: ${entry.branch}` : `PR #${merged.number}`; + err.push(` ✗ ${entry.title}`); + err.push(` ${via} — PR #${merged.number} is MERGED: ${merged.title}`); + } + err.push( + "\nRemediation: the PR(s) above merged outside /merge-pr, so the 🚧 entry was\n" + + "never archived. Move each stale entry from the TODO file into TODO-archive.md\n" + + "(collapse to a one-line ✅ entry), then re-run this check. Going forward, merge\n" + + "through /merge-pr so its post-merge sync handles this automatically." ); - process.exit(0); + return { exitCode: 1, out, err }; +} + +// Wrap summarize(scanTodo(...)) so a taxonomy-external error (a real bug) maps +// to exit 2 — distinct from stale's exit 1 — instead of crashing with Node's +// default exit 1 (which would read as "stale found"). The testable exit-2 seam. +export function runScan(text, ghRunner = ghExec) { + try { + return summarize(scanTodo(text, ghRunner)); + } catch (e) { + return { + exitCode: 2, + out: [], + err: [`check-stale-todo-markers: internal error — ${e?.message ?? String(e)}`], + }; + } +} + +function main() { + const todoPath = process.argv[2] ?? DEFAULT_TODO; + let text; + try { + text = readFileSync(todoPath, "utf8"); + } catch (err) { + console.error(`check-stale-todo-markers: cannot read ${todoPath}: ${err.message}`); + process.exit(2); + } + + const { exitCode, out, err } = runScan(text); + for (const line of out) { + console.log(line); + } + for (const line of err) { + console.error(line); + } + process.exit(exitCode); } -console.error(`check-stale-todo-markers: ${stale.length} STALE 🚧 marker(s) in ${todoPath}\n`); -for (const { entry, merged } of stale) { - const branch = entry.branch ? `branch: ${entry.branch}` : `PR #${merged.number}`; - console.error(` ✗ ${entry.title}`); - console.error(` ${branch} — PR #${merged.number} is MERGED: ${merged.title}`); +// Only run main when invoked directly (not when imported by the test suite). +if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { + main(); } -console.error( - "\nRemediation: the PR(s) above merged outside /merge-pr, so the 🚧 entry was\n" + - "never archived. Move each stale entry from the TODO file into TODO-archive.md\n" + - "(collapse to a one-line ✅ entry), then re-run this check. Going forward, merge\n" + - "through /merge-pr so its post-merge sync handles this automatically." -); -process.exit(1); + +export { DEFAULT_TODO }; diff --git a/test/build/stale-todo-markers.test.ts b/test/build/stale-todo-markers.test.ts new file mode 100644 index 00000000..4ee926a7 --- /dev/null +++ b/test/build/stale-todo-markers.test.ts @@ -0,0 +1,280 @@ +// Non-vacuity pins for scripts/check-stale-todo-markers.mjs. +// +// The script is the backstop that catches stale in-flight (🚧) TODO markers +// whose PR merged outside /merge-pr. It shells out to `gh`. These tests feed +// the PURE / dependency-injected exports in-memory fixtures — a fake `ghRunner` +// (and a fake `exec` for `ghExec`) — so no real `gh` binary, network, or auth +// is needed, and the three historical false-verdict paths stay pinned: +// (a) a REUSED head-branch name whose OLD PR merged must not flag still-open +// work as stale — an OPEN PR for the branch wins. +// (b) a bare `(#N)` / number-only match must downgrade to a WARNING, never a +// hard STALE verdict (PR numbers are reused across repo generations). +// (c) one transient gh failure must skip only THAT entry and keep scanning — +// not bail the whole loop — and the exit must reflect a partial scan. +// +// Why in-memory (never the real .claude/docs/TODO.md): that file is git-ignored +// and absent from CI checkouts, so reading it here would fail in CI — same rule +// as test/build/todo-hygiene.test.ts. +// +// @ts-nocheck — importing a plain .mjs with no bundled types; vitest runs this +// transpile-only and tsc does not include test/build/ in `pnpm compile`. +import { describe, expect, it } from "vitest"; + +import { + classifyGhError, + GhTransient, + GhUnavailable, + ghExec, + parseEntry, + resolveEntry, + runScan, + scanTodo, + summarize, +} from "../../scripts/check-stale-todo-markers.mjs"; + +// A fake `ghRunner`, keyed on the argv array. `routes.list[headBranch]` and +// `routes.view[prNumber]` may be a canned value or a 0-arg function (a function +// lets a route throw GhTransient/GhUnavailable/TypeError to simulate failures). +function makeGh(routes = {}) { + return (args) => { + if (args[0] === "pr" && args[1] === "list") { + const head = args[args.indexOf("--head") + 1]; + const r = routes.list?.[head]; + return typeof r === "function" ? r() : (r ?? []); + } + if (args[0] === "pr" && args[1] === "view") { + const r = routes.view?.[args[2]]; + return typeof r === "function" ? r() : (r ?? { state: "OPEN" }); + } + return []; + }; +} + +// A one-entry / two-entry in-flight TODO fixture. Each entry carries the 🚧 +// glyph, a `(branch: X)` (or `(#N)`) marker, and a **bold** title. +function todo(...entries) { + return ["# TODO", "", "## ▶️ Active queue", "", ...entries, ""].join("\n"); +} +const branchEntry = (branch, title) => + `- [ ] 🚧 🐛 [MED] **${title}** (branch: ${branch})\n - Done when: shipped.`; +const numberEntry = (n, title) => `- [ ] 🚧 🐛 [MED] **${title}** (#${n})\n - Done when: shipped.`; + +describe("resolveEntry — fix (a): reused branch prefers an OPEN PR", () => { + it("returns clean when an OPEN PR exists even though an older same-branch PR merged", () => { + const gh = makeGh({ + list: { + "feat/x": [ + { state: "OPEN", number: 99, title: "new work" }, + { state: "MERGED", number: 12, title: "old shipped" }, + ], + }, + }); + expect(resolveEntry({ branch: "feat/x", pr: null }, gh).status).toBe("clean"); + }); + + it("returns stale when there is NO open PR and a merged one exists (exit-1 contract)", () => { + const gh = makeGh({ list: { "feat/x": [{ state: "MERGED", number: 42, title: "shipped" }] } }); + const r = resolveEntry({ branch: "feat/x", pr: null }, gh); + expect(r.status).toBe("stale"); + expect(r.merged.number).toBe(42); + }); + + it("returns clean for the no-OPEN/no-MERGED edge (empty, DRAFT, or CLOSED only)", () => { + expect( + resolveEntry({ branch: "feat/x", pr: null }, makeGh({ list: { "feat/x": [] } })).status + ).toBe("clean"); + const gh = makeGh({ list: { "feat/x": [{ state: "CLOSED", number: 7, title: "abandoned" }] } }); + expect(resolveEntry({ branch: "feat/x", pr: null }, gh).status).toBe("clean"); + }); +}); + +describe("resolveEntry — fix (b): number-only resolution downgrades to a warning", () => { + it("returns warn (not stale) when a bare-number entry's PR is merged", () => { + const gh = makeGh({ view: { "7": { state: "MERGED", title: "shipped elsewhere" } } }); + const r = resolveEntry({ branch: null, pr: "7" }, gh); + expect(r.status).toBe("warn"); + expect(r.merged.number).toBe(7); + }); + + it("returns clean when a number-only entry's PR is still open", () => { + const gh = makeGh({ view: { "7": { state: "OPEN", title: "in flight" } } }); + expect(resolveEntry({ branch: null, pr: "7" }, gh).status).toBe("clean"); + }); +}); + +describe("scanTodo — fix (c): a transient error skips one entry, not the whole loop", () => { + it("skips the transient entry and still finds the stale one that follows", () => { + const text = todo( + branchEntry("feat/flaky", "Flaky lookup"), + branchEntry("feat/done", "Already shipped") + ); + const gh = makeGh({ + list: { + "feat/flaky": () => { + throw new GhTransient("HTTP 404"); + }, + "feat/done": [{ state: "MERGED", number: 5, title: "Already shipped" }], + }, + }); + const scan = scanTodo(text, gh); + expect(scan.skipped.map((s) => s.entry.branch)).toEqual(["feat/flaky"]); + expect(scan.stale.map((s) => s.entry.branch)).toEqual(["feat/done"]); + expect(scan.aborted).toBeNull(); + }); +}); + +describe("scanTodo — GhUnavailable stops the loop but preserves already-found stale", () => { + it("keeps a stale entry found before a mid-scan GhUnavailable and does not throw", () => { + const text = todo( + branchEntry("feat/done", "Already shipped"), + branchEntry("feat/auth", "Auth expired here") + ); + const gh = makeGh({ + list: { + "feat/done": [{ state: "MERGED", number: 5, title: "Already shipped" }], + "feat/auth": () => { + throw new GhUnavailable("gh auth login"); + }, + }, + }); + let scan = null; + expect(() => { + scan = scanTodo(text, gh); + }).not.toThrow(); + expect(scan.stale.map((s) => s.entry.branch)).toEqual(["feat/done"]); + expect(scan.aborted).toBeTruthy(); + const { exitCode, err } = summarize(scan); + expect(exitCode).toBe(1); + expect(err.join("\n")).toMatch(/partial|unchecked|unavailable/i); + }); +}); + +describe("scanTodo — a number-only merged entry warns, never becomes stale", () => { + it("lands the entry in warnings, keeps stale empty, exit 0", () => { + const text = todo(numberEntry(7, "Number only")); + const gh = makeGh({ view: { "7": { state: "MERGED", title: "shipped" } } }); + const scan = scanTodo(text, gh); + expect(scan.stale).toHaveLength(0); + expect(scan.warnings).toHaveLength(1); + expect(summarize(scan).exitCode).toBe(0); + }); +}); + +describe("scanTodo — Finding 2: a taxonomy-external bug is rethrown, never swallowed", () => { + it("propagates a raw TypeError out of the loop instead of reporting clean", () => { + const text = todo(branchEntry("feat/x", "Boom")); + const gh = () => { + throw new TypeError("boom"); + }; + expect(() => scanTodo(text, gh)).toThrow(TypeError); + }); +}); + +describe("runScan — Finding 1: a bug becomes exit 2, never colliding with stale's exit 1", () => { + it("maps a taxonomy-external error to exitCode 2 with an internal-error message", () => { + const text = todo(branchEntry("feat/x", "Boom")); + const { exitCode, err } = runScan(text, () => { + throw new TypeError("boom"); + }); + expect(exitCode).toBe(2); + expect(err.join("\n")).toMatch(/internal error/i); + }); + + it("still returns exitCode 1 for a genuinely stale marker (2 and 1 do not collide)", () => { + const text = todo(branchEntry("feat/done", "Shipped")); + const gh = makeGh({ + list: { "feat/done": [{ state: "MERGED", number: 5, title: "Shipped" }] }, + }); + expect(runScan(text, gh).exitCode).toBe(1); + }); +}); + +describe("summarize — Finding 4 / honest partial-scan wording", () => { + it("says 'partial' and exits 0 when entries were skipped but none are stale", () => { + const scan = { + inflightCount: 2, + stale: [], + warnings: [], + skipped: [{ entry: { branch: "feat/a" }, reason: "404" }], + aborted: null, + }; + const { exitCode, out, err } = summarize(scan); + expect(exitCode).toBe(0); + expect([...out, ...err].join("\n")).toMatch(/partial/i); + }); + + it("does not say 'partial' on a fully clean scan", () => { + const scan = { inflightCount: 1, stale: [], warnings: [], skipped: [], aborted: null }; + const { exitCode, out, err } = summarize(scan); + expect(exitCode).toBe(0); + expect([...out, ...err].join("\n")).not.toMatch(/partial/i); + }); + + it("exits 1 whenever a stale entry is present", () => { + const scan = { + inflightCount: 1, + stale: [ + { + entry: { branch: "feat/done", title: "Shipped" }, + merged: { number: 5, title: "Shipped" }, + }, + ], + warnings: [], + skipped: [], + aborted: null, + }; + expect(summarize(scan).exitCode).toBe(1); + }); +}); + +describe("classifyGhError — ENOENT / auth / rate-limit → GhUnavailable, else GhTransient", () => { + it("classifies ENOENT and auth failures and rate limits as GhUnavailable", () => { + expect(classifyGhError(Object.assign(new Error("spawn"), { code: "ENOENT" }))).toBeInstanceOf( + GhUnavailable + ); + expect( + classifyGhError({ stderr: "gh: Not logged into any GitHub hosts. Run gh auth login" }) + ).toBeInstanceOf(GhUnavailable); + expect(classifyGhError({ stderr: "API rate limit exceeded for user" })).toBeInstanceOf( + GhUnavailable + ); + }); + + it("classifies a one-off HTTP error as GhTransient", () => { + expect(classifyGhError({ stderr: "HTTP 404: Not Found" })).toBeInstanceOf(GhTransient); + }); +}); + +describe("ghExec — injectable exec exercises JSON.parse and the classification paths", () => { + it("throws GhTransient on unparseable stdout", () => { + expect(() => ghExec(["pr", "view", "1"], () => "not json")).toThrow(GhTransient); + }); + + it("returns parsed JSON on success", () => { + expect(ghExec(["x"], () => '{"state":"MERGED"}')).toEqual({ state: "MERGED" }); + }); + + it("throws GhUnavailable when the underlying exec reports ENOENT", () => { + expect(() => + ghExec(["x"], () => { + throw Object.assign(new Error("spawn"), { code: "ENOENT" }); + }) + ).toThrow(GhUnavailable); + }); +}); + +describe("parseEntry — extracts branch / pr / title from every marker form", () => { + it("reads the modern (branch: X) form", () => { + expect(parseEntry("- [ ] 🚧 **Feature** (branch: feat/foo)").branch).toBe("feat/foo"); + }); + it("reads the legacy form", () => { + expect(parseEntry("- [ ] 🚧 **Feature** ").branch).toBe("feat/foo"); + }); + it("reads (PR #12) and bare (#12) as a pr number", () => { + expect(parseEntry("- [ ] 🚧 **Feature** (PR #12)").pr).toBe("12"); + expect(parseEntry("- [ ] 🚧 **Feature** (#12)").pr).toBe("12"); + }); + it("captures the **bold** title", () => { + expect(parseEntry("- [ ] 🚧 **The title** (branch: b)").title).toBe("The title"); + }); +}); From 9f4c7ee7c8dddd640194ef823d8d0dfef163e95f Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 29 Jul 2026 12:59:12 +1000 Subject: [PATCH 2/9] fix(check-todo): query OPEN PRs on their own, not a truncated slice The branch-path staleness check listed --state all --limit 5 and scanned the slice for an OPEN PR. A busy reused branch could push the in-flight OPEN PR past the result cap, letting an older same-named merged PR flag still-open work as stale. Query OPEN independently (--state open) before looking for a merged PR, so the in-flight guarantee no longer depends on gh's implicit sort order or a result limit. --- scripts/check-stale-todo-markers.mjs | 31 +++++++++++---- test/build/stale-todo-markers.test.ts | 54 +++++++++++++++++---------- 2 files changed, 57 insertions(+), 28 deletions(-) diff --git a/scripts/check-stale-todo-markers.mjs b/scripts/check-stale-todo-markers.mjs index c326955a..980e31c7 100755 --- a/scripts/check-stale-todo-markers.mjs +++ b/scripts/check-stale-todo-markers.mjs @@ -129,24 +129,39 @@ export function parseEntry(line) { // WARNING, never stale (PR numbers are reused across repo generations). export function resolveEntry({ branch, pr }, ghRunner) { if (branch) { - const prs = ghRunner([ + // fix (a): an OPEN PR for the head branch means the work is in flight, so + // the entry is NEVER stale — even if an older same-named PR merged. Query + // OPEN on its own (not a truncated `--state all` slice) so a busy reused + // branch can't hide the in-flight PR behind a result cap. + const open = ghRunner([ "pr", "list", "--head", branch, "--state", - "all", + "open", "--json", - "number,state,title,mergedAt", + "number", "--limit", - "5", + "1", ]); - if (prs.some((p) => p.state === "OPEN")) { + if (open.length > 0) { return { status: "clean" }; // in flight — never stale (fix (a)) } - const merged = prs.find((p) => p.state === "MERGED"); - if (merged) { - return { status: "stale", merged: { number: merged.number, title: merged.title } }; + const merged = ghRunner([ + "pr", + "list", + "--head", + branch, + "--state", + "merged", + "--json", + "number,title,mergedAt", + "--limit", + "1", + ]); + if (merged.length > 0) { + return { status: "stale", merged: { number: merged[0].number, title: merged[0].title } }; } return { status: "clean" }; } diff --git a/test/build/stale-todo-markers.test.ts b/test/build/stale-todo-markers.test.ts index 4ee926a7..4a9664b4 100644 --- a/test/build/stale-todo-markers.test.ts +++ b/test/build/stale-todo-markers.test.ts @@ -32,14 +32,18 @@ import { summarize, } from "../../scripts/check-stale-todo-markers.mjs"; -// A fake `ghRunner`, keyed on the argv array. `routes.list[headBranch]` and -// `routes.view[prNumber]` may be a canned value or a 0-arg function (a function -// lets a route throw GhTransient/GhUnavailable/TypeError to simulate failures). +// A fake `ghRunner`, keyed on the argv array. `resolveEntry` issues the branch +// lookup as two independent `--state`-scoped queries (open, then merged), so +// `routes.list[headBranch]` is itself keyed by state: `{ open: [...], merged: +// [...] }`. `routes.view[prNumber]` keeps its single form. Any leaf may be a +// canned value or a 0-arg function (a function lets a route throw +// GhTransient/GhUnavailable/TypeError to simulate failures). function makeGh(routes = {}) { return (args) => { if (args[0] === "pr" && args[1] === "list") { const head = args[args.indexOf("--head") + 1]; - const r = routes.list?.[head]; + const state = args[args.indexOf("--state") + 1]; + const r = routes.list?.[head]?.[state]; return typeof r === "function" ? r() : (r ?? []); } if (args[0] === "pr" && args[1] === "view") { @@ -63,27 +67,33 @@ describe("resolveEntry — fix (a): reused branch prefers an OPEN PR", () => { it("returns clean when an OPEN PR exists even though an older same-branch PR merged", () => { const gh = makeGh({ list: { - "feat/x": [ - { state: "OPEN", number: 99, title: "new work" }, - { state: "MERGED", number: 12, title: "old shipped" }, - ], + "feat/x": { + open: [{ state: "OPEN", number: 99 }], + merged: [{ number: 12, title: "old shipped" }], + }, }, }); expect(resolveEntry({ branch: "feat/x", pr: null }, gh).status).toBe("clean"); }); + it("prefers an OPEN PR regardless of how many merged PRs exist (no truncation dependency)", () => { + const manyMerged = Array.from({ length: 30 }, (_, i) => ({ number: i, title: `m${i}` })); + const gh = makeGh({ list: { "feat/x": { open: [{ number: 99 }], merged: manyMerged } } }); + expect(resolveEntry({ branch: "feat/x", pr: null }, gh).status).toBe("clean"); + }); + it("returns stale when there is NO open PR and a merged one exists (exit-1 contract)", () => { - const gh = makeGh({ list: { "feat/x": [{ state: "MERGED", number: 42, title: "shipped" }] } }); + const gh = makeGh({ + list: { "feat/x": { open: [], merged: [{ number: 42, title: "shipped" }] } }, + }); const r = resolveEntry({ branch: "feat/x", pr: null }, gh); expect(r.status).toBe("stale"); expect(r.merged.number).toBe(42); }); it("returns clean for the no-OPEN/no-MERGED edge (empty, DRAFT, or CLOSED only)", () => { - expect( - resolveEntry({ branch: "feat/x", pr: null }, makeGh({ list: { "feat/x": [] } })).status - ).toBe("clean"); - const gh = makeGh({ list: { "feat/x": [{ state: "CLOSED", number: 7, title: "abandoned" }] } }); + // A CLOSED-only branch surfaces as neither an open nor a merged hit. + const gh = makeGh({ list: { "feat/x": { open: [], merged: [] } } }); expect(resolveEntry({ branch: "feat/x", pr: null }, gh).status).toBe("clean"); }); }); @@ -110,10 +120,12 @@ describe("scanTodo — fix (c): a transient error skips one entry, not the whole ); const gh = makeGh({ list: { - "feat/flaky": () => { - throw new GhTransient("HTTP 404"); + "feat/flaky": { + open: () => { + throw new GhTransient("HTTP 404"); + }, }, - "feat/done": [{ state: "MERGED", number: 5, title: "Already shipped" }], + "feat/done": { open: [], merged: [{ number: 5, title: "Already shipped" }] }, }, }); const scan = scanTodo(text, gh); @@ -131,9 +143,11 @@ describe("scanTodo — GhUnavailable stops the loop but preserves already-found ); const gh = makeGh({ list: { - "feat/done": [{ state: "MERGED", number: 5, title: "Already shipped" }], - "feat/auth": () => { - throw new GhUnavailable("gh auth login"); + "feat/done": { open: [], merged: [{ number: 5, title: "Already shipped" }] }, + "feat/auth": { + open: () => { + throw new GhUnavailable("gh auth login"); + }, }, }, }); @@ -183,7 +197,7 @@ describe("runScan — Finding 1: a bug becomes exit 2, never colliding with stal it("still returns exitCode 1 for a genuinely stale marker (2 and 1 do not collide)", () => { const text = todo(branchEntry("feat/done", "Shipped")); const gh = makeGh({ - list: { "feat/done": [{ state: "MERGED", number: 5, title: "Shipped" }] }, + list: { "feat/done": { open: [], merged: [{ number: 5, title: "Shipped" }] } }, }); expect(runScan(text, gh).exitCode).toBe(1); }); From 2ade4ca7d1da273e97995c5a9982d4a2d820708c Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 29 Jul 2026 13:00:16 +1000 Subject: [PATCH 3/9] fix(check-todo): classify HTTP 401 bad credentials as GhUnavailable An expired or invalid gh token surfaces as "HTTP 401: Bad credentials", which persists for the whole run, so it belongs with the other run-fatal auth failures rather than the per-entry transient bucket. Misclassifying it as transient would silently skip every remaining entry and still exit 0. 403 stays out of the pattern: it can be a genuine per-entry blip. --- scripts/check-stale-todo-markers.mjs | 9 ++++++--- test/build/stale-todo-markers.test.ts | 1 + 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/scripts/check-stale-todo-markers.mjs b/scripts/check-stale-todo-markers.mjs index 980e31c7..ede3398a 100755 --- a/scripts/check-stale-todo-markers.mjs +++ b/scripts/check-stale-todo-markers.mjs @@ -38,8 +38,9 @@ // // Error taxonomy (see `classifyGhError`): // - GhUnavailable — `gh` is unusable for the REST of this run: not installed -// (ENOENT), not authenticated, or rate-limited. All three persist for the -// run, so re-trying every remaining entry is futile; the scan stops but +// (ENOENT), not authenticated (auth failure / HTTP 401 bad credentials), or +// rate-limited. All three persist for the run, so re-trying every remaining +// entry is futile; the scan stops but // still RETURNS what it already found (a stale marker confirmed before a // mid-scan auth expiry must not be silently discarded). // - GhTransient — a genuinely per-entry failure (single 404, one-off network @@ -74,7 +75,9 @@ export function classifyGhError(err) { const msg = `${err?.stderr ?? ""}${err?.message ?? ""}`; if ( err?.code === "ENOENT" || - /not logged|authentication|gh auth login|GH_TOKEN|rate limit/i.test(msg) + /not logged|authentication|gh auth login|GH_TOKEN|rate limit|bad credentials|HTTP 401/i.test( + msg + ) ) { return new GhUnavailable(msg.trim() || String(err)); } diff --git a/test/build/stale-todo-markers.test.ts b/test/build/stale-todo-markers.test.ts index 4a9664b4..409f6493 100644 --- a/test/build/stale-todo-markers.test.ts +++ b/test/build/stale-todo-markers.test.ts @@ -252,6 +252,7 @@ describe("classifyGhError — ENOENT / auth / rate-limit → GhUnavailable, else expect(classifyGhError({ stderr: "API rate limit exceeded for user" })).toBeInstanceOf( GhUnavailable ); + expect(classifyGhError({ stderr: "HTTP 401: Bad credentials" })).toBeInstanceOf(GhUnavailable); }); it("classifies a one-off HTTP error as GhTransient", () => { From 045edbc46907d0282bc33c4e01f881fd6f997db3 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 29 Jul 2026 13:01:21 +1000 Subject: [PATCH 4/9] test(check-todo): pin parseInflight exclusion direction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parseInflight had no direct test — its exclusion of checked (- [x]) and non-🚧 lines rode only on the higher-level scan tests. Pin the contract directly so a widened checkbox pattern (e.g. [ ] -> [.]) that lets done work read as in-flight fails here, and assert scanTodo.inflightCount. --- test/build/stale-todo-markers.test.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/build/stale-todo-markers.test.ts b/test/build/stale-todo-markers.test.ts index 409f6493..968e0a5a 100644 --- a/test/build/stale-todo-markers.test.ts +++ b/test/build/stale-todo-markers.test.ts @@ -26,6 +26,7 @@ import { GhUnavailable, ghExec, parseEntry, + parseInflight, resolveEntry, runScan, scanTodo, @@ -168,6 +169,7 @@ describe("scanTodo — a number-only merged entry warns, never becomes stale", ( const text = todo(numberEntry(7, "Number only")); const gh = makeGh({ view: { "7": { state: "MERGED", title: "shipped" } } }); const scan = scanTodo(text, gh); + expect(scan.inflightCount).toBe(1); expect(scan.stale).toHaveLength(0); expect(scan.warnings).toHaveLength(1); expect(summarize(scan).exitCode).toBe(0); @@ -293,3 +295,15 @@ describe("parseEntry — extracts branch / pr / title from every marker form", ( expect(parseEntry("- [ ] 🚧 **The title** (branch: b)").title).toBe("The title"); }); }); + +describe("parseInflight — selects only unchecked 🚧 lines, excludes the rest", () => { + it("selects an unchecked 🚧 line", () => { + expect(parseInflight("- [ ] 🚧 **A** (branch: x)")).toHaveLength(1); + }); + it("excludes a CHECKED 🚧 line (done work is never in-flight)", () => { + expect(parseInflight("- [x] 🚧 **A** (branch: x)")).toHaveLength(0); + }); + it("excludes an unchecked line WITHOUT the 🚧 glyph", () => { + expect(parseInflight("- [ ] **A** (branch: x)")).toHaveLength(0); + }); +}); From 409be9a79f0106a1d4219a764ccccf1813b0b169 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 29 Jul 2026 13:01:48 +1000 Subject: [PATCH 5/9] docs(check-todo): correct the header test-coverage roster and comment rot List parseInflight among the ghRunner-tested exports and split out classifyGhError (fake error objects) and ghExec (fake exec) as their own seams so the roster matches what the tests actually exercise. Replace the orphaned (Finding 2) review-cycle reference on the rethrow comment with its plain meaning (never swallowed). --- scripts/check-stale-todo-markers.mjs | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/scripts/check-stale-todo-markers.mjs b/scripts/check-stale-todo-markers.mjs index ede3398a..98eafa1b 100755 --- a/scripts/check-stale-todo-markers.mjs +++ b/scripts/check-stale-todo-markers.mjs @@ -18,10 +18,12 @@ // under .claude/, gitignored, absent from CI checkouts), so it never hard- // blocks a contributor. A missing/unauthenticated/rate-limited `gh` warns // and exits 0. -// - The pure / dependency-injected exports (`parseEntry`, `resolveEntry`, -// `scanTodo`, `summarize`, `runScan`, `classifyGhError`) are unit-tested -// with in-memory fixtures and a fake `ghRunner`; `main()` is a thin fs + -// console + process.exit wrapper behind the import-guard at the bottom. +// - The pure / dependency-injected exports (`parseInflight`, `parseEntry`, +// `resolveEntry`, `scanTodo`, `summarize`, `runScan`) are unit-tested with +// in-memory fixtures and a fake `ghRunner`; `classifyGhError` is pinned with +// fake error objects and `ghExec` with a fake `exec` (both a different seam, +// not the `ghRunner`). `main()` is a thin fs + console + process.exit +// wrapper behind the import-guard at the bottom. // // Verdict correctness (three false-verdict paths this file deliberately avoids): // (a) REUSED head-branch name — a branch name may be reused after its first @@ -210,7 +212,7 @@ export function scanTodo(text, ghRunner = ghExec) { aborted = { reason: err.message }; break; } - throw err; // taxonomy-external → a real bug, surface it (Finding 2) + throw err; // taxonomy-external → a real bug, surface it (never swallowed) } if (result.status === "stale") { stale.push({ entry, merged: result.merged }); From d321e6eecc33e913630e6313b7abe2e66ed333c9 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 29 Jul 2026 13:02:24 +1000 Subject: [PATCH 6/9] test(check-todo): drop review-cycle Finding labels from describe names The Finding 1/2/4 prefixes named a past review cycle, not the behaviour under test. Describe the invariant instead; assertions are unchanged. --- test/build/stale-todo-markers.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/build/stale-todo-markers.test.ts b/test/build/stale-todo-markers.test.ts index 968e0a5a..a3ca01fb 100644 --- a/test/build/stale-todo-markers.test.ts +++ b/test/build/stale-todo-markers.test.ts @@ -176,7 +176,7 @@ describe("scanTodo — a number-only merged entry warns, never becomes stale", ( }); }); -describe("scanTodo — Finding 2: a taxonomy-external bug is rethrown, never swallowed", () => { +describe("scanTodo — a taxonomy-external bug is rethrown, never swallowed", () => { it("propagates a raw TypeError out of the loop instead of reporting clean", () => { const text = todo(branchEntry("feat/x", "Boom")); const gh = () => { @@ -186,7 +186,7 @@ describe("scanTodo — Finding 2: a taxonomy-external bug is rethrown, never swa }); }); -describe("runScan — Finding 1: a bug becomes exit 2, never colliding with stale's exit 1", () => { +describe("runScan — a bug becomes exit 2, never colliding with stale's exit 1", () => { it("maps a taxonomy-external error to exitCode 2 with an internal-error message", () => { const text = todo(branchEntry("feat/x", "Boom")); const { exitCode, err } = runScan(text, () => { @@ -205,7 +205,7 @@ describe("runScan — Finding 1: a bug becomes exit 2, never colliding with stal }); }); -describe("summarize — Finding 4 / honest partial-scan wording", () => { +describe("summarize — honest partial-scan wording and exit codes", () => { it("says 'partial' and exits 0 when entries were skipped but none are stale", () => { const scan = { inflightCount: 2, From 9b79b7f670a1a12726d54d931ba41d2fae1cf0e0 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 29 Jul 2026 13:12:16 +1000 Subject: [PATCH 7/9] refactor(check-todo): inline the DEFAULT_TODO export Consolidate the trailing `export { DEFAULT_TODO }` into an inline `export const` so every export in the file is declared inline. No behaviour change. --- scripts/check-stale-todo-markers.mjs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/scripts/check-stale-todo-markers.mjs b/scripts/check-stale-todo-markers.mjs index 98eafa1b..b34bea15 100755 --- a/scripts/check-stale-todo-markers.mjs +++ b/scripts/check-stale-todo-markers.mjs @@ -62,7 +62,7 @@ import { execFileSync } from "node:child_process"; import { readFileSync } from "node:fs"; import { pathToFileURL } from "node:url"; -const DEFAULT_TODO = ".claude/docs/TODO.md"; +export const DEFAULT_TODO = ".claude/docs/TODO.md"; // `gh` is unusable for the rest of this run (ENOENT / auth / rate limit). export class GhUnavailable extends Error {} @@ -323,5 +323,3 @@ function main() { if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) { main(); } - -export { DEFAULT_TODO }; From 25f8fb91ba6f339993118a37ebbc220272fce2c4 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 29 Jul 2026 13:26:02 +1000 Subject: [PATCH 8/9] test(stale-todo-markers): pin short-circuit and independent HTTP-401 alternatives - Replace the vacuous "no truncation dependency" test with one that pins the actual contract: an OPEN PR short-circuits resolveEntry before the merged query ever runs (a throwing merged route proves it). The old test passed the same either way because a hit on open never touched merged. - Add two classifyGhError cases so the HTTP-401 detection is pinned on each regex alternative independently (a bare HTTP 401 with no 'bad credentials' text, and a 'Bad credentials' message with no HTTP status), instead of relying on a single fixture that satisfies both alternatives at once. --- test/build/stale-todo-markers.test.ts | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/test/build/stale-todo-markers.test.ts b/test/build/stale-todo-markers.test.ts index a3ca01fb..3a9787d7 100644 --- a/test/build/stale-todo-markers.test.ts +++ b/test/build/stale-todo-markers.test.ts @@ -77,9 +77,17 @@ describe("resolveEntry — fix (a): reused branch prefers an OPEN PR", () => { expect(resolveEntry({ branch: "feat/x", pr: null }, gh).status).toBe("clean"); }); - it("prefers an OPEN PR regardless of how many merged PRs exist (no truncation dependency)", () => { - const manyMerged = Array.from({ length: 30 }, (_, i) => ({ number: i, title: `m${i}` })); - const gh = makeGh({ list: { "feat/x": { open: [{ number: 99 }], merged: manyMerged } } }); + it("returns clean via the OPEN query without ever consulting merged (short-circuit)", () => { + const gh = makeGh({ + list: { + "feat/x": { + open: [{ number: 99 }], + merged: () => { + throw new Error("merged query must not run when an OPEN PR exists"); + }, + }, + }, + }); expect(resolveEntry({ branch: "feat/x", pr: null }, gh).status).toBe("clean"); }); @@ -257,6 +265,14 @@ describe("classifyGhError — ENOENT / auth / rate-limit → GhUnavailable, else expect(classifyGhError({ stderr: "HTTP 401: Bad credentials" })).toBeInstanceOf(GhUnavailable); }); + it("classifies a bare HTTP 401 (no 'bad credentials' text) as GhUnavailable", () => { + expect(classifyGhError({ stderr: "gh: HTTP 401 Unauthorized" })).toBeInstanceOf(GhUnavailable); + }); + + it("classifies a 'Bad credentials' message (no HTTP status) as GhUnavailable", () => { + expect(classifyGhError({ stderr: "error: Bad credentials" })).toBeInstanceOf(GhUnavailable); + }); + it("classifies a one-off HTTP error as GhTransient", () => { expect(classifyGhError({ stderr: "HTTP 404: Not Found" })).toBeInstanceOf(GhTransient); }); From b8e08e3a2c9372ba91ff2fa78bfdac0275715c97 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 29 Jul 2026 13:26:12 +1000 Subject: [PATCH 9/9] docs(stale-todo-markers): fix test-coverage roster and honest OPEN-PR wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Correct the header's test-coverage roster: parseInflight, parseEntry, and summarize are pure transforms pinned with literal fixtures, not ghRunner-dependent like resolveEntry/scanTodo/runScan. - Narrow the 'never flagged stale' / 'is NEVER stale' claims (header fix (a), resolveEntry's docstring, and its inline comments) to what actually holds: the branch reads clean WHILE an OPEN PR exists. A reused branch whose new work has not yet opened a PR still falls through to the MERGED check and reads stale — a branch-name-identity limitation, not a bug, tracked in the TODO backlog for a durable PR identity fix. No behaviour change. --- scripts/check-stale-todo-markers.mjs | 38 ++++++++++++++++++---------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/scripts/check-stale-todo-markers.mjs b/scripts/check-stale-todo-markers.mjs index b34bea15..bdf58335 100755 --- a/scripts/check-stale-todo-markers.mjs +++ b/scripts/check-stale-todo-markers.mjs @@ -18,17 +18,22 @@ // under .claude/, gitignored, absent from CI checkouts), so it never hard- // blocks a contributor. A missing/unauthenticated/rate-limited `gh` warns // and exits 0. -// - The pure / dependency-injected exports (`parseInflight`, `parseEntry`, -// `resolveEntry`, `scanTodo`, `summarize`, `runScan`) are unit-tested with -// in-memory fixtures and a fake `ghRunner`; `classifyGhError` is pinned with -// fake error objects and `ghExec` with a fake `exec` (both a different seam, -// not the `ghRunner`). `main()` is a thin fs + console + process.exit -// wrapper behind the import-guard at the bottom. +// - Tests split by seam: pure transforms (`parseInflight`, `parseEntry`, +// `summarize`) are pinned with literal in-memory fixtures; the `ghRunner`- +// injected exports (`resolveEntry`, `scanTodo`, `runScan`) are pinned with +// a fake `ghRunner`; `classifyGhError` is pinned with fake error objects +// and `ghExec` with a fake `exec` (a different seam, not the `ghRunner`). +// `main()` is a thin fs + console + process.exit wrapper behind the +// import-guard at the bottom. // // Verdict correctness (three false-verdict paths this file deliberately avoids): // (a) REUSED head-branch name — a branch name may be reused after its first -// PR merged. `resolveEntry` prefers an OPEN PR for the branch, so still- -// in-flight work is never flagged stale by an older same-named merged PR. +// PR merged. `resolveEntry` prefers an OPEN PR for the branch, so +// still-in-flight work is not flagged stale WHILE that OPEN PR exists. +// Residual gap: a reused branch whose new work has not yet opened a PR +// still resolves via the old MERGED PR and reads STALE — durable PR +// identity on the marker (not just a branch name) is the real fix, +// tracked in the TODO backlog. // (b) NUMBER-ONLY match — PR numbers are reused across repo generations, so a // match resting solely on a `(#N)` / `(PR #N)` number is REPORT-ONLY // (a warning), never a hard STALE verdict. A conforming in-flight entry @@ -128,16 +133,21 @@ export function parseEntry(line) { // Resolve one entry against GitHub via the injected `ghRunner`. // Returns { status: "clean" | "stale" | "warn", merged?: { number, title } }. // - branch path (authoritative): fix (a) — an OPEN PR for the head branch -// means the work is in flight, so it is NEVER stale, even if an OLDER -// same-named PR merged. Only "no OPEN + a MERGED" → stale. +// means the work is in flight, so it reads clean WHILE that OPEN PR +// exists, even if an OLDER same-named PR merged. Only "no OPEN + a +// MERGED" → stale. (A reused branch with no new PR yet still resolves +// via the old MERGED PR — see the header note.) // - number-only path: fix (b) — a match resting solely on a PR number is a // WARNING, never stale (PR numbers are reused across repo generations). export function resolveEntry({ branch, pr }, ghRunner) { if (branch) { // fix (a): an OPEN PR for the head branch means the work is in flight, so - // the entry is NEVER stale — even if an older same-named PR merged. Query - // OPEN on its own (not a truncated `--state all` slice) so a busy reused - // branch can't hide the in-flight PR behind a result cap. + // the entry reads clean while that OPEN PR exists — even if an older + // same-named PR merged. Query OPEN on its own (not a truncated `--state + // all` slice) so a busy reused branch can't hide the in-flight PR behind + // a result cap. (If no new PR has been opened yet for a reused branch, + // this still falls through to the MERGED check below and reads stale — + // a fundamental branch-name-identity limitation, not a bug here.) const open = ghRunner([ "pr", "list", @@ -151,7 +161,7 @@ export function resolveEntry({ branch, pr }, ghRunner) { "1", ]); if (open.length > 0) { - return { status: "clean" }; // in flight — never stale (fix (a)) + return { status: "clean" }; // in flight — clean while this OPEN PR exists (fix (a)) } const merged = ghRunner([ "pr",