golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(#2348): derive verification staleness from git commit time, not mtime (#2394)

Browse Source 
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.
This commit is contained in:
Tom Boucher
2026-07-17 21:58:02 -04:00
committed by GitHub
parent f2c077df38
commit 2c54f219c9
3 changed files with 565 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

171
src/verification.cts
View File

@@ -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<string, number>;
/** 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<string, number> {
const out = new Map<string, number>();
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 -- <files…>` 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 -- <files…>` 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<string, number> {
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,