From 2c54f219c928a20a899767d308afedf9c0dc4df2 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Fri, 17 Jul 2026 21:58:02 -0400 Subject: [PATCH] fix(#2348): derive verification staleness from git commit time, not mtime (#2394) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit readVerificationStatus() decided a phase's verification was `stale` (a *-SUMMARY.md newer than the *-VERIFICATION.md) by comparing filesystem mtimes. mtimes are assigned at checkout time and are not preserved by `git clone` / `cp -R`, and any unrelated `touch` / reformat / editor-save re-stales a valid report — so a committed phase declaring `status: passed` could silently read `stale` on a fresh clone purely from checkout order, falsely rewriting a ROADMAP row and blocking milestone close (#2022 gate). Each file's effective "last changed" time is now its git commit time when the file is committed AND clean, and its mtime otherwise (uncommitted or working-tree-dirty). Both are real wall-clock change times, so a summary committed after — or edited after — the verification reads stale, while a clean fresh clone stays passed. Git commit time is content-tied and clone- stable; mtime is retained only where it is the true last-changed signal. Implementation: - Two bounded git calls per phase (never one-per-file): `git log --first-parent --format=%ct --name-only` for commit times, and `git diff --name-only HEAD` to drop dirty files. readVerificationStatus runs per-phase in the init/roadmap listing loops, so per-file spawning would fan out to P×(S+1) git processes ("Unbounded Subprocesses"). - `--first-parent` so merge commits report their file lists (plain `--name-only` omits merge diffs and would under-date merge-landed content). - The dirty-check fails SAFE: if `git diff` is inconclusive (errors / exits non-zero) the commit times are discarded so every file falls back to mtime, never trusting a possibly-stale commit time (no false "not stale"). - Paths matched back by `/`-bounded suffix (root vs nested `plans/` can't collide) and passed after `--` (dash-named files can't be read as flags). - A phase with no summaries skips git entirely; the scan short-circuits on the first stale summary. A `phaseCleanCommitTimesMs` seam keeps the unit tests hermetic (no git spawn); the resolver's two-call error handling is unit-tested via an injected execGit; two real-git integration tests lock the end-to-end path, the committed-then-edited (dirty) regression, and the `--` argv guard. --- .changeset/lively-elks-romp.md | 5 + src/verification.cts | 171 ++++++++++-- tests/verification-status.test.cjs | 406 ++++++++++++++++++++++++++++- 3 files changed, 565 insertions(+), 17 deletions(-) create mode 100644 .changeset/lively-elks-romp.md diff --git a/.changeset/lively-elks-romp.md b/.changeset/lively-elks-romp.md new file mode 100644 index 000000000..500fb5d0a --- /dev/null +++ b/.changeset/lively-elks-romp.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2394 +--- +**Phase verification no longer reads `stale` from filesystem timestamps alone** — staleness is now derived from git commit times instead of file mtimes, so a phase whose report declares `status: passed` stays passed across a fresh `git clone`, `cp -R`, or an unrelated `touch`/reformat, instead of being silently downgraded to `stale` by a checkout-order mtime skew. (#2348) diff --git a/src/verification.cts b/src/verification.cts index e53f7f146..4d89b51ce 100644 --- a/src/verification.cts +++ b/src/verification.cts @@ -14,6 +14,17 @@ * inside a fenced code block) is ignored — this is the exact failure mode that * issue #586 / PR #650 identified. The shared extractFrontmatter parser anchors * its regex at byte 0 of the document, which provides this guarantee. + * + * #2348 staleness signal: whether a *-VERIFICATION.md is stale (a summary newer + * than it) is decided from git commit time when a file is committed AND clean, + * and from filesystem mtime otherwise. mtimes are assigned at checkout time and + * are not preserved by `git clone` / `cp -R`, and any unrelated `touch` / + * reformat / editor-save re-stales a valid report — so a committed phase could + * read `passed` on one machine and `stale` on a fresh clone purely from checkout + * order. Git commit time is content-tied and clone-stable; mtime is retained + * only for uncommitted or working-tree-dirty files, where it is the true + * last-changed signal. Both are real wall-clock change times, so the comparison + * is sound even when one file uses each. */ import fs from 'node:fs'; @@ -26,6 +37,7 @@ import phaseId = require('./phase-id.cjs'); import frontmatterMod = require('./frontmatter.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports -- plan-scan.cjs is an export= CommonJS module import scanPhasePlans = require('./plan-scan.cjs'); +import { execGit } from './shell-command-projection.cjs'; const { output, error } = io; const { extractPhaseToken } = phaseId; @@ -110,6 +122,114 @@ interface StaleVerificationInfo { summaryFile: string; } +/** + * Resolve the git commit time (epoch-ms) for each of `files` (paths relative to + * `phaseDir`) that is BOTH committed AND clean (its working-tree content matches + * HEAD), keyed by the given relative path. A file that is dirty, untracked, + * uncommitted, or in a non-repo is simply absent — callers then time it by its + * filesystem mtime. Injectable so tests exercise the clock without git. (#2348) + */ +type PhaseCleanCommitTimesFn = (phaseDir: string, files: string[]) => Map; + +/** Normalize separators to posix (git emits `/`; callers may pass `\` on Windows). */ +function toPosix(p: string): string { + return p.replace(/\\/g, '/'); +} + +/** + * Match a git-emitted (repo-root-relative) path back to the caller's + * phaseDir-relative request by exact match or `/`-bounded suffix — precise + * enough that a root file and a nested `plans/` file can never collide (a plain + * basename match could). Returns the original caller-form file string, or null. + */ +function matchRequestedFile(gitPath: string, requested: string[], requestedPosix: string[]): string | null { + const g = toPosix(gitPath); + for (let i = 0; i < requested.length; i++) { + const want = requestedPosix[i]; + if (g === want || g.endsWith('/' + want)) return requested[i]; + } + return null; +} + +/** + * Parse `git log --format=%ct --name-only` output into file → most-recent commit + * time (ms). Output is reverse-chronological, so a file's FIRST appearance + * top-down is its latest commit. `%ct` headers are pure digits; path lines + * contain a `.` (the `.md` extension) — so the two are unambiguous. + */ +function parseCommitTimes( + stdout: string, + requested: string[], + requestedPosix: string[], +): Map { + const out = new Map(); + let currentCt: number | null = null; + for (const line of stdout.split('\n')) { + if (line.length === 0) continue; + if (/^\d+$/.test(line)) { + currentCt = Number.parseInt(line, 10); + continue; + } + if (currentCt === null) continue; + const rel = matchRequestedFile(line, requested, requestedPosix); + if (rel !== null && !out.has(rel)) out.set(rel, currentCt * 1000); + } + return out; +} + +/** + * Default resolver: two bounded git calls per phase (never one-per-file — #2348 / + * "Unbounded Subprocesses"; readVerificationStatus runs per-phase in the + * init/roadmap listing loops, so per-file spawning would fan out to P×(S+1)): + * + * 1. `git log --first-parent --format=%ct --name-only -- ` for commit + * times. `--first-parent` makes merge commits report their (first-parent) + * file lists — plain `--name-only` omits merge diffs, which would silently + * under-date content that landed via a conflict-resolving merge. + * 2. `git diff --name-only HEAD -- ` to drop any file whose working + * tree has diverged from HEAD: a committed-then-edited file must be timed by + * its mtime (the edit), never by its now-stale commit time. + * + * Paths pass after `--` so a dash-prefixed filename cannot be read as a flag. Any + * non-answer (no repo, no commits, missing git) yields an empty map → the caller + * times every file by mtime. Never throws. The per-phase file list is small (a + * verification report + a handful of summaries), so the argv stays far below the + * Windows 32K limit. `execGitFn` is injectable so the two-call error handling is + * unit-testable without spawning git. + */ +type ExecGitFn = typeof execGit; + +function defaultPhaseCleanCommitTimesMs( + phaseDir: string, + files: string[], + execGitFn: ExecGitFn = execGit, +): Map { + if (files.length === 0) return new Map(); + const requestedPosix = files.map(toPosix); + + const logRes = execGitFn(['log', '--first-parent', '--format=%ct', '--name-only', '--', ...files], { + cwd: phaseDir, + }); + if (logRes.error || logRes.exitCode !== 0 || logRes.stdout.length === 0) return new Map(); + const commitTimes = parseCommitTimes(logRes.stdout, files, requestedPosix); + if (commitTimes.size === 0) return commitTimes; + + // Drop dirty files (working tree ≠ HEAD) so their mtime is used instead. If the + // dirty-check itself is INCONCLUSIVE (git diff errored / non-zero — as opposed + // to "ran and reported no dirty files"), we cannot prove any file is clean, so + // fail SAFE: discard the commit times and let every file fall back to mtime, + // the same direction as a git-log failure. Trusting possibly-stale commit times + // here would silently mask a real edit (false "not stale"). (#2348) + const diffRes = execGitFn(['diff', '--name-only', 'HEAD', '--', ...files], { cwd: phaseDir }); + if (diffRes.error || diffRes.exitCode !== 0) return new Map(); + for (const line of diffRes.stdout.split('\n')) { + if (line.length === 0) continue; + const rel = matchRequestedFile(line, files, requestedPosix); + if (rel !== null) commitTimes.delete(rel); + } + return commitTimes; +} + /** * Build a 'missing' result from the routing table. * Used for two early-return paths: no *-VERIFICATION.md file found, and @@ -128,6 +248,8 @@ function missingResult(): VerificationStatusResult { interface ReadVerificationStatusOptions { fs?: FsLike; + /** Injectable per-phase clean-commit-time resolver for the staleness clock (#2348). */ + phaseCleanCommitTimesMs?: PhaseCleanCommitTimesFn; } interface VerificationStatusResult { @@ -136,7 +258,11 @@ interface VerificationStatusResult { next_command: string; } -function findStaleVerificationSummary(phaseDir: string, fsImpl: FsLike = fs): StaleVerificationInfo | null { +function findStaleVerificationSummary( + phaseDir: string, + fsImpl: FsLike = fs, + phaseCleanCommitTimesMs: PhaseCleanCommitTimesFn = defaultPhaseCleanCommitTimesMs, +): StaleVerificationInfo | null { // FS errors (TOCTOU: a SUMMARY listed by scanPhasePlans then removed before statSync; // unreadable dir; broken symlink; file->dir swap) must degrade to "not stale" rather // than throw uncaught into callers that are NOT under the planning lock @@ -148,22 +274,34 @@ function findStaleVerificationSummary(phaseDir: string, fsImpl: FsLike = fs): St const verificationFile = phaseFiles.filter((f) => f.endsWith('-VERIFICATION.md')).sort()[0]; if (!verificationFile) return null; - const verificationMtimeMs = fsImpl.statSync(path.join(phaseDir, verificationFile)).mtimeMs; - let newestStaleSummary: { summaryFile: string; mtimeMs: number } | null = null; - const summaryFiles = (scanPhasePlans(phaseDir) as { summaryFiles: string[] }).summaryFiles; - for (const summaryFile of summaryFiles.sort()) { - const summaryMtimeMs = fsImpl.statSync(path.join(phaseDir, summaryFile)).mtimeMs; - if (summaryMtimeMs <= verificationMtimeMs) continue; - if (!newestStaleSummary || summaryMtimeMs > newestStaleSummary.mtimeMs) { - newestStaleSummary = { summaryFile, mtimeMs: summaryMtimeMs }; + const summaryFiles = (scanPhasePlans(phaseDir) as { summaryFiles: string[] }).summaryFiles + .slice() + .sort(); + // No summary can be newer than the verification → never stale. Return before + // touching git so a phase with no summaries costs zero subprocesses. (#2348) + if (summaryFiles.length === 0) return null; + + // Each file's effective "last changed" time = its commit time when committed + // AND clean (content-tied and clone-stable), else its filesystem mtime (the + // uncommitted working-tree edit). Both are real wall-clock change times, so + // comparing a clean file's commit time against a dirty file's mtime is sound. + // One resolver call = two git subprocesses for the whole phase. (#2348) + const cleanCommitMs = phaseCleanCommitTimesMs(phaseDir, [verificationFile, ...summaryFiles]); + const effectiveTimeMs = (file: string): number => + cleanCommitMs.has(file) + ? (cleanCommitMs.get(file) as number) + : fsImpl.statSync(path.join(phaseDir, file)).mtimeMs; + + const verificationTimeMs = effectiveTimeMs(verificationFile); + for (const summaryFile of summaryFiles) { + // The caller only needs whether the phase is stale, not which summary — + // the first stale summary (in sorted order) is enough. Short-circuit. + if (effectiveTimeMs(summaryFile) > verificationTimeMs) { + return { verificationFile, summaryFile }; } } - if (!newestStaleSummary) return null; - return { - verificationFile, - summaryFile: newestStaleSummary.summaryFile, - }; + return null; } catch { return null; } @@ -189,6 +327,8 @@ function readVerificationStatus( opts: ReadVerificationStatusOptions = {}, ): VerificationStatusResult { const fsImpl: FsLike = opts.fs ?? fs; + const phaseCleanCommitTimesMs: PhaseCleanCommitTimesFn = + opts.phaseCleanCommitTimesMs ?? defaultPhaseCleanCommitTimesMs; // Phase token for the gaps_found command const baseName = path.basename(phaseDir); @@ -243,7 +383,7 @@ function readVerificationStatus( }; } - const staleVerification = findStaleVerificationSummary(phaseDir, fsImpl); + const staleVerification = findStaleVerificationSummary(phaseDir, fsImpl, phaseCleanCommitTimesMs); if (staleVerification) { const entry = VERIFICATION_ROUTING_TABLE['stale']; return { @@ -300,6 +440,7 @@ function cmdVerificationStatus(cwd: string, phaseDirArg: string | undefined, raw export = { VERIFIER_STATUSES, VERIFICATION_ROUTING_TABLE, + defaultPhaseCleanCommitTimesMs, findStaleVerificationSummary, readVerificationStatus, cmdVerificationStatus, diff --git a/tests/verification-status.test.cjs b/tests/verification-status.test.cjs index e44d32530..3f49c9445 100644 --- a/tests/verification-status.test.cjs +++ b/tests/verification-status.test.cjs @@ -32,6 +32,7 @@ const { cleanup } = require('./helpers.cjs'); const { VERIFIER_STATUSES, VERIFICATION_ROUTING_TABLE, + defaultPhaseCleanCommitTimesMs, readVerificationStatus, } = require('../gsd-core/bin/lib/verification.cjs'); @@ -329,7 +330,9 @@ describe('verification-status', () => { setMtime(verificationPath, '2026-01-01T00:00:00.000Z'); setMtime(summaryPath, '2026-01-01T00:01:00.000Z'); - const result = readVerificationStatus(dir); + // git times unavailable → mtime-fallback path (#2348). Injected so the + // test stays hermetic (no git spawn) regardless of tmpdir repo state. + const result = readVerificationStatus(dir, { phaseCleanCommitTimesMs: () => new Map() }); assert.equal(result.status, 'stale'); assert.match(result.next_action, /stale/i); assert.equal(result.next_command, '/gsd:verify-work 01'); @@ -372,7 +375,8 @@ describe('verification-status', () => { setMtime(verificationPath, '2026-01-01T00:00:00.000Z'); setMtime(summaryPath, '2026-01-01T00:01:00.000Z'); - const result = readVerificationStatus(dir); + // git times unavailable → mtime-fallback path (#2348). + const result = readVerificationStatus(dir, { phaseCleanCommitTimesMs: () => new Map() }); assert.equal(result.status, 'stale'); assert.equal(result.next_command, '/gsd:verify-work 01'); } finally { @@ -380,6 +384,404 @@ describe('verification-status', () => { } }); + // ── #2348: staleness derived from git commit time, not filesystem mtime ──── + // + // The verification staleness gate must survive a fresh `git clone` / `cp -R` + // and an unrelated `touch`. It compares git commit times (content-tied) and + // only falls back to mtime when a file has no commit time (uncommitted / no + // repo), always reading both sides of a comparison from the same clock. + + // Injectable per-phase git-commit-time resolver: given the phase-relative file + // names, returns Map. A file whose basename is absent from + // `byBase` resolves to "no git time" (uncommitted / not in git) → mtime clock. + const phaseCleanTimes = (byBase) => (_phaseDir, files) => { + const m = new Map(); + for (const file of files) { + const base = file.split(/[\\/]/).pop(); + if (Object.prototype.hasOwnProperty.call(byBase, base)) m.set(file, byBase[base]); + } + return m; + }; + + // git availability for the real-subprocess integration test below. + const GIT_AVAILABLE = (() => { + try { + require('node:child_process').execFileSync('git', ['--version'], { stdio: 'ignore' }); + return true; + } catch { + return false; + } + })(); + + test('committed passed verification is NOT stale from mtime skew alone when the summary was not committed later (#2348)', () => { + const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2348-parent-')); + const dir = path.join(baseDir, '02-clone-skew'); + fs.mkdirSync(dir); + try { + const verificationPath = path.join(dir, '02-VERIFICATION.md'); + const summaryPath = path.join(dir, '02-02-SUMMARY.md'); + writeVerificationMd(dir, '02-VERIFICATION.md', 'passed'); + fs.writeFileSync(summaryPath, '# Summary'); + // Filesystem mtimes reproduce the reported 49s checkout skew (summary newer). + setMtime(verificationPath, '2026-07-16T22:53:49.000Z'); + setMtime(summaryPath, '2026-07-16T22:54:38.000Z'); + // But in git both were committed together — the summary is not newer. + const phaseCleanCommitTimesMs = phaseCleanTimes({ + '02-VERIFICATION.md': Date.parse('2026-07-16T22:50:00.000Z'), + '02-02-SUMMARY.md': Date.parse('2026-07-16T22:50:00.000Z'), + }); + + const result = readVerificationStatus(dir, { phaseCleanCommitTimesMs }); + assert.equal( + result.status, + 'passed', + 'mtime skew alone must not override a committed passing verification', + ); + assert.equal(result.next_command, ''); + } finally { + cleanup(baseDir); + } + }); + + test('committed verification IS stale when the summary was committed later, even if its mtime is older — git clock wins (#2348)', () => { + const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2348-parent-')); + const dir = path.join(baseDir, '02-git-stale'); + fs.mkdirSync(dir); + try { + const verificationPath = path.join(dir, '02-VERIFICATION.md'); + const summaryPath = path.join(dir, '02-02-SUMMARY.md'); + writeVerificationMd(dir, '02-VERIFICATION.md', 'passed'); + fs.writeFileSync(summaryPath, '# Summary'); + // mtimes point the OTHER way (verification newer) to prove git is authoritative. + setMtime(verificationPath, '2026-07-16T23:00:00.000Z'); + setMtime(summaryPath, '2026-07-16T22:00:00.000Z'); + const phaseCleanCommitTimesMs = phaseCleanTimes({ + '02-VERIFICATION.md': Date.parse('2026-07-16T22:50:00.000Z'), + '02-02-SUMMARY.md': Date.parse('2026-07-16T22:55:00.000Z'), // committed later + }); + + const result = readVerificationStatus(dir, { phaseCleanCommitTimesMs }); + assert.equal(result.status, 'stale'); + assert.equal(result.next_command, '/gsd:verify-work 02'); + } finally { + cleanup(baseDir); + } + }); + + test('git-clock staleness boundary: summary committed at V-1 / V / V+1 relative to verification (#2348)', () => { + const V = Date.parse('2026-07-16T22:50:00.000Z'); + for (const { deltaMs, expected } of [ + { deltaMs: -1, expected: 'passed' }, + { deltaMs: 0, expected: 'passed' }, + { deltaMs: 1, expected: 'stale' }, + ]) { + const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2348-boundary-')); + const dir = path.join(baseDir, '03-boundary'); + fs.mkdirSync(dir); + try { + const verificationPath = path.join(dir, '03-VERIFICATION.md'); + const summaryPath = path.join(dir, '03-03-SUMMARY.md'); + writeVerificationMd(dir, '03-VERIFICATION.md', 'passed'); + fs.writeFileSync(summaryPath, '# Summary'); + setMtime(verificationPath, '2026-07-16T22:50:00.000Z'); + setMtime(summaryPath, '2026-07-16T22:50:00.000Z'); + const phaseCleanCommitTimesMs = phaseCleanTimes({ + '03-VERIFICATION.md': V, + '03-03-SUMMARY.md': V + deltaMs, + }); + + const result = readVerificationStatus(dir, { phaseCleanCommitTimesMs }); + assert.equal( + result.status, + expected, + `summary committed at V${deltaMs >= 0 ? '+' : ''}${deltaMs}ms should be ${expected}`, + ); + } finally { + cleanup(baseDir); + } + } + }); + + test('a committed-clean verification is stale when a summary is edited afterward (dirty) — the edit is not shadowed by the summary commit time (#2348 dirty regression)', () => { + const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2348-dirty-')); + const dir = path.join(baseDir, '02-dirty-summary'); + fs.mkdirSync(dir); + try { + const verificationPath = path.join(dir, '02-VERIFICATION.md'); + const summaryPath = path.join(dir, '02-02-SUMMARY.md'); + writeVerificationMd(dir, '02-VERIFICATION.md', 'passed'); + fs.writeFileSync(summaryPath, '# Summary'); + // Verification is committed & clean at 22:50. The summary is DIRTY (edited + // on disk after its commit) so it is absent from the clean-commit map and + // must be timed by its mtime — a later edit at 22:54. + setMtime(verificationPath, '2026-07-16T22:50:00.000Z'); // unused (clean → commit time) + setMtime(summaryPath, '2026-07-16T22:54:00.000Z'); + const phaseCleanCommitTimesMs = phaseCleanTimes({ + '02-VERIFICATION.md': Date.parse('2026-07-16T22:50:00.000Z'), + // '02-02-SUMMARY.md' intentionally omitted → treated as dirty → mtime. + }); + + const result = readVerificationStatus(dir, { phaseCleanCommitTimesMs }); + assert.equal( + result.status, + 'stale', + 'a dirty summary edited after the verification must stale it via mtime, not be shadowed by an equal/earlier commit time', + ); + assert.equal(result.next_command, '/gsd:verify-work 02'); + } finally { + cleanup(baseDir); + } + }); + + test('both files uncommitted (no clean-commit time) fall back to mtime ordering (#2348)', () => { + const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2348-uncommitted-')); + const dir = path.join(baseDir, '02-uncommitted'); + fs.mkdirSync(dir); + try { + const verificationPath = path.join(dir, '02-VERIFICATION.md'); + const summaryPath = path.join(dir, '02-02-SUMMARY.md'); + writeVerificationMd(dir, '02-VERIFICATION.md', 'passed'); + fs.writeFileSync(summaryPath, '# Summary'); + // Neither file is committed → empty clean map → pure mtime comparison. + setMtime(verificationPath, '2026-07-16T23:00:00.000Z'); + setMtime(summaryPath, '2026-07-16T22:00:00.000Z'); // summary older → not stale + const phaseCleanCommitTimesMs = phaseCleanTimes({}); + + const result = readVerificationStatus(dir, { phaseCleanCommitTimesMs }); + assert.equal(result.status, 'passed', 'summary older on the mtime clock → not stale'); + } finally { + cleanup(baseDir); + } + }); + + test('the git-commit-time resolver is invoked at most once per phase, regardless of summary count (#2348 no per-file fan-out)', () => { + const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2348-fanout-')); + const dir = path.join(baseDir, '01-fanout'); + fs.mkdirSync(dir); + try { + writeVerificationMd(dir, '01-VERIFICATION.md', 'passed'); + for (const n of ['01', '02', '03']) { + fs.writeFileSync(path.join(dir, `01-${n}-SUMMARY.md`), '# Summary'); + } + let calls = 0; + let filesSeen = 0; + const phaseCleanCommitTimesMs = (_phaseDir, files) => { + calls += 1; + filesSeen = files.length; + return new Map(); + }; + + readVerificationStatus(dir, { phaseCleanCommitTimesMs }); + assert.equal(calls, 1, 'exactly one git walk for the whole phase, not one per summary file'); + assert.equal(filesSeen, 4, 'the single walk receives the verification file + all 3 summaries'); + } finally { + cleanup(baseDir); + } + }); + + test('a phase with no summary files performs zero git walks and is never stale (#2348)', () => { + const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2348-nosummary-')); + const dir = path.join(baseDir, '01-no-summary'); + fs.mkdirSync(dir); + try { + writeVerificationMd(dir, '01-VERIFICATION.md', 'passed'); + let calls = 0; + const phaseCleanCommitTimesMs = () => { + calls += 1; + return new Map(); + }; + + const result = readVerificationStatus(dir, { phaseCleanCommitTimesMs }); + assert.equal(result.status, 'passed'); + assert.equal(calls, 0, 'no summaries → nothing can be newer → skip the git subprocess entirely'); + } finally { + cleanup(baseDir); + } + }); + + test( + 'real git: a summary committed after the verification reads stale via the real git clock, even for a dash-named file (#2348 end-to-end + `--` argv guard)', + { skip: GIT_AVAILABLE ? false : 'git binary not available' }, + () => { + const { execFileSync } = require('node:child_process'); + const repo = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2348-realgit-')); + const runGit = (args, extraEnv) => + execFileSync('git', args, { + cwd: repo, + stdio: 'pipe', + env: { ...process.env, GIT_TERMINAL_PROMPT: '0', ...(extraEnv || {}) }, + }); + const commitEnvAt = (iso) => ({ GIT_AUTHOR_DATE: iso + '+00:00', GIT_COMMITTER_DATE: iso + '+00:00' }); + try { + runGit(['init', '-q']); + runGit(['config', 'user.email', 'test@example.com']); + runGit(['config', 'user.name', 'Test']); + runGit(['config', 'commit.gpgsign', 'false']); + + const dir = path.join(repo, '.planning', 'phases', '01-real'); + fs.mkdirSync(dir, { recursive: true }); + const verificationPath = path.join(dir, '01-VERIFICATION.md'); + // A leading-dash filename exercises the `--` pathspec guard in the real + // `git log` argv: if `--` were dropped git would read it as a flag. + const summaryName = '-danger-SUMMARY.md'; + const summaryPath = path.join(dir, summaryName); + + fs.writeFileSync(verificationPath, '---\nstatus: passed\n---\n'); + runGit(['add', '--', verificationPath]); + runGit(['commit', '-q', '-m', 'add verification'], commitEnvAt('2026-07-16T22:50:00')); + + fs.writeFileSync(summaryPath, '# Summary'); + runGit(['add', '--', summaryPath]); + runGit(['commit', '-q', '-m', 'add summary later'], commitEnvAt('2026-07-16T22:55:00')); + + // Make mtimes claim the OPPOSITE order so only the git clock can stale it. + setMtime(summaryPath, '2000-01-01T00:00:00.000Z'); + setMtime(verificationPath, '2030-01-01T00:00:00.000Z'); + + // No seam injected → the real defaultPhaseCleanCommitTimesMs / execGit path. + const result = readVerificationStatus(dir); + assert.equal( + result.status, + 'stale', + 'summary committed after the verification must read stale on the real git clock, and the dash-named file must resolve through the `--` pathspec guard', + ); + assert.equal(result.next_command, '/gsd:verify-work 01'); + } finally { + cleanup(repo); + } + }, + ); + + test( + 'real git: a committed summary edited on disk (dirty) reads stale via mtime, not shadowed by its commit time (#2348 dirty regression, end-to-end)', + { skip: GIT_AVAILABLE ? false : 'git binary not available' }, + () => { + const { execFileSync } = require('node:child_process'); + const repo = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-2348-realgit-dirty-')); + const runGit = (args, extraEnv) => + execFileSync('git', args, { + cwd: repo, + stdio: 'pipe', + env: { ...process.env, GIT_TERMINAL_PROMPT: '0', ...(extraEnv || {}) }, + }); + const commitEnvAt = (iso) => ({ GIT_AUTHOR_DATE: iso + '+00:00', GIT_COMMITTER_DATE: iso + '+00:00' }); + try { + runGit(['init', '-q']); + runGit(['config', 'user.email', 'test@example.com']); + runGit(['config', 'user.name', 'Test']); + runGit(['config', 'commit.gpgsign', 'false']); + + const dir = path.join(repo, '.planning', 'phases', '01-real'); + fs.mkdirSync(dir, { recursive: true }); + const verificationPath = path.join(dir, '01-VERIFICATION.md'); + const summaryPath = path.join(dir, '01-01-SUMMARY.md'); + + fs.writeFileSync(verificationPath, '---\nstatus: passed\n---\n'); + fs.writeFileSync(summaryPath, '# Summary'); + // Commit BOTH together — identical commit time, so commit time alone + // would read "not stale". + runGit(['add', '--', verificationPath, summaryPath]); + runGit(['commit', '-q', '-m', 'add phase'], commitEnvAt('2026-07-16T22:50:00')); + + // Edit the summary again WITHOUT committing → working tree diverges from HEAD. + fs.writeFileSync(summaryPath, '# Summary edited'); + setMtime(verificationPath, '2026-07-16T22:50:00.000Z'); // clean → commit time used + setMtime(summaryPath, '2026-07-16T22:54:00.000Z'); // dirty → this later mtime is used + + const result = readVerificationStatus(dir); + assert.equal( + result.status, + 'stale', + 'a committed-then-edited (dirty) summary must read stale via mtime, not be shadowed by its now-stale commit time', + ); + assert.equal(result.next_command, '/gsd:verify-work 01'); + } finally { + cleanup(repo); + } + }, + ); + + // ── #2348: default resolver two-call error handling (hermetic, injected execGit) ── + + const okResult = (stdout) => ({ exitCode: 0, stdout, stderr: '', signal: null, error: null }); + const errResult = () => ({ + exitCode: 127, + stdout: '', + stderr: 'git: not found', + signal: null, + error: new Error('ENOENT'), + }); + const nonzeroResult = () => ({ exitCode: 128, stdout: '', stderr: 'fatal', signal: null, error: null }); + // Fake execGit dispatching on the git subcommand (args[0]). + const fakeExecGit = ({ log, diff }) => (args) => { + if (args[0] === 'log') return log; + if (args[0] === 'diff') return diff; + throw new Error(`unexpected git ${args.join(' ')}`); + }; + // Reverse-chronological `git log --name-only` fixture: summary newer than verification. + const LOG_OUT = [ + '2000', + '', + '.planning/phases/01-x/01-01-SUMMARY.md', + '', + '1000', + '', + '.planning/phases/01-x/01-VERIFICATION.md', + ].join('\n'); + const FILES = ['01-VERIFICATION.md', '01-01-SUMMARY.md']; + + test('resolver: parses commit times and drops a file the dirty-check reports (#2348)', () => { + const map = defaultPhaseCleanCommitTimesMs( + '/repo/.planning/phases/01-x', + FILES, + fakeExecGit({ log: okResult(LOG_OUT), diff: okResult('.planning/phases/01-x/01-01-SUMMARY.md') }), + ); + assert.equal(map.get('01-VERIFICATION.md'), 1000 * 1000, 'verification commit time (seconds→ms)'); + assert.equal(map.has('01-01-SUMMARY.md'), false, 'dirty summary dropped → will use mtime'); + }); + + test('resolver: clean tree (dirty-check reports nothing) keeps all commit times (#2348)', () => { + const map = defaultPhaseCleanCommitTimesMs( + '/repo/.planning/phases/01-x', + FILES, + fakeExecGit({ log: okResult(LOG_OUT), diff: okResult('') }), + ); + assert.equal(map.get('01-VERIFICATION.md'), 1000 * 1000); + assert.equal(map.get('01-01-SUMMARY.md'), 2000 * 1000); + }); + + test('resolver: FAILS SAFE (empty map) when the dirty-check errors after git log succeeds (#2348)', () => { + const map = defaultPhaseCleanCommitTimesMs( + '/repo/.planning/phases/01-x', + FILES, + fakeExecGit({ log: okResult(LOG_OUT), diff: errResult() }), + ); + assert.equal( + map.size, + 0, + 'an inconclusive dirty-check must discard commit times so every file falls back to mtime', + ); + }); + + test('resolver: FAILS SAFE (empty map) when the dirty-check exits non-zero (#2348)', () => { + const map = defaultPhaseCleanCommitTimesMs( + '/repo/.planning/phases/01-x', + FILES, + fakeExecGit({ log: okResult(LOG_OUT), diff: nonzeroResult() }), + ); + assert.equal(map.size, 0); + }); + + test('resolver: empty map (mtime fallback) when git log itself fails (#2348)', () => { + const map = defaultPhaseCleanCommitTimesMs( + '/repo/.planning/phases/01-x', + FILES, + // diff would throw if consulted — proves log-failure short-circuits before it. + fakeExecGit({ log: errResult(), diff: undefined }), + ); + assert.equal(map.size, 0); + }); + // ── Task 2 (B1): ship.md gate sentinel contract anchor ──────────────────── // // The deleted tests/ship-586-verification-routing.test.cjs was the only