fix(tools): use commit-time as fallback in spec-mtime and spec-coverage freshness gate#18
fix(tools): use commit-time as fallback in spec-mtime and spec-coverage freshness gate#18ThePlenkov wants to merge 6 commits into
Conversation
Root cause: the spec-mtime freshness gate (and the duplicate mtime gate inside spec-coverage) compared filesystem mtimes. On fresh git checkouts / worktrees / CI clones, every file gets a wall-clock mtime equal to checkout time, in materialization order, NOT to its commit time. For every package in this repo, SPEC.md happened to land on disk ~0.29 s before its newest src/ file, so the gate fired a false-positive on every clone regardless of whether the SPEC content was up to date with the code (and it is — spec-drift passes, git history shows SPEC.md commits post-date src commits in every package). The previous attempt (closed PR #2) worked around the symptom with trailing-whitespace touches on SPEC.md. That made the gate pass on the worktree that produced the commit, but did not address the underlying conflation of filesystem mtime with commit time, so the next fresh checkout reproduces the same failure. Fix: make the gate git-aware. Each file's effective mtime is now max(commit_time_seconds, filesystem_mtime_seconds), with filesystem mtime winning for dirty/uncommitted files. This is deterministic across checkouts while preserving the original intent for in-flight edits. Changes: - tools/spec-mtime.mjs: add commitTimesUnder() + effectiveMtime(); replace filesystem-mtime comparison in the per-package loop. - tools/spec-coverage.mjs: same helpers; rewrite the "2. mtime gate" block in checkPackage() to use them. - No SPEC.md, src/, package.json, lockfile, or spec-check.mjs changes; none of the three spec-checks are skipped or disabled.
… fix Documents the root cause (filesystem-mtime gate firing false-positive on fresh checkouts because git worktree assigns wall-clock mtimes in materialization order, not commit order), the fix (git-aware effective mtime in tools/spec-mtime.mjs and tools/spec-coverage.mjs), and the full pnpm run verify result (exit 0, 49.2 s wall time, all sub-steps green).
…rCloud duplication The issue abapify#11 fix introduced near-identical git-aware mtime logic (walk, rel, commitTimesUnder, effectiveMtime, newestEffectiveMtime) in both tools/spec-mtime.mjs and tools/spec-coverage.mjs, which tripped the SonarCloud New Code Duplication gate (37.6%, limit 3%). Extract the shared helpers into tools/lib/git-mtime.mjs and have both tools import from it. No behavior change; pnpm run verify still passes (spec-check, typecheck, lint, test, bench:smoke).
Address qodo-code-review feedback: commitTimesUnder() previously forwarded whatever path the caller supplied (an absolute path) to `git log -- <dir>`. In some environments (symlinked worktrees, custom cwd) git may not resolve an absolute pathspec the way the helper expects, which would cause the silent fallback to filesystem mtimes and re-introduce the issue abapify#11 false-positive. Always relativize against the repo root before passing to git log. No behavior change on the happy path; pnpm run verify still green.
- Make git-mtime path resolution platform-independent (rel() and commitTimesUnder() now use path.sep + forward-slash normalization so the helpers work correctly on Windows worktrees, per CodeRabbit/cubic review). - Use newestEffectiveMtime() helper in tools/spec-mtime.mjs for consistency with tools/spec-coverage.mjs (eliminates the manual effective-mtime loop). - Fix docs/issues/11-verify-result.md: replace 'effectiveMtimeOf' typo with 'effectiveMtime', and add 'text' language to the two fenced code blocks (markdownlint MD040). pnpm run verify: PASS (spec-check, typecheck, lint, test, bench:smoke).
Collapse the redundant ternary in toRepoRelative() — both branches returned the same slice expression — and have rel() delegate entirely to toRepoRelative() so the path-normalization logic lives in one place.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughAdds ChangesGit-aware spec freshness gate
Sequence DiagramsequenceDiagram
participant Script as spec-mtime / spec-coverage
participant GitMtime as tools/lib/git-mtime.mjs
participant Git as git log
participant FS as filesystem stat
Script->>GitMtime: commitTimesUnder(pkgPath)
GitMtime->>Git: git log --name-only --format=%ct (subtree)
Git-->>GitMtime: NUL-delimited path/timestamp output
GitMtime-->>Script: Map(relPath → commitSeconds)
Script->>GitMtime: newestEffectiveMtime(srcFiles, commitTimes)
GitMtime->>FS: stat each srcFile
FS-->>GitMtime: fsMtime (seconds)
GitMtime-->>Script: max(commitTime, fsMtime) across srcFiles
Script->>GitMtime: effectiveMtime(specPath, commitTimes)
GitMtime->>FS: stat specPath
FS-->>GitMtime: fsMtime (seconds)
GitMtime-->>Script: max(commitTime, fsMtime) for SPEC.md
Script->>Script: FAIL if specEffective < srcNewest
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
The implementation successfully addresses the false-positive failures in the spec-mtime gate by making it git-aware. The approach of using max(commit_time, filesystem_mtime) is correct and maintains backward compatibility with uncommitted files. The code refactoring extracts shared helpers appropriately, error handling is robust, and the git command execution is secure. All verification tests pass as documented.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
codeant-ai
Bot
added
the
size:L
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 9 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8edadb30a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch {} | ||
| const ct = commitTimes.get(rel(file)); | ||
| if (ct === undefined) return fsMtime; // never committed: filesystem only | ||
| return Math.max(ct, fsMtime); // dirty file: filesystem mtime wins |
There was a problem hiding this comment.
For clean tracked files in a fresh checkout, stat().mtime is usually the checkout/materialization time, which is later than the file's commit time, so this Math.max makes filesystem metadata override git history for every clean file. If the checkout, cache restore, or worktree materializes src/ even one second after specs/SPEC.md, both spec-mtime and the spec-coverage freshness gate will still report a stale spec even though the spec commit post-dates the source commit. The helper needs to use filesystem time only for paths that are actually dirty/untracked, not for all tracked files whose checkout mtime happens to be newer.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses issue #11 by making the spec-check freshness gate git-aware, resolving false-positive failures on fresh checkouts where filesystem mtimes do not reflect commit-time ordering. It introduces a shared helper module tools/lib/git-mtime.mjs to calculate an effective mtime using git commit times and updates tools/spec-coverage.mjs and tools/spec-mtime.mjs accordingly. Feedback on the pull request highlights a critical regex parsing bug with git log -z and a logical flaw where Math.max(ct, fsMtime) will always default to the filesystem mtime on fresh checkouts (since checkout time is in the future), thereby defeating the purpose of the fix. A robust solution using git status --porcelain to explicitly track dirty files is suggested.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export async function commitTimesUnder(dir) { | ||
| const out = new Map(); | ||
| const relDir = toRepoRelative(dir); | ||
| let res; | ||
| try { | ||
| res = await execFileP( | ||
| 'git', | ||
| [ | ||
| 'log', | ||
| '--pretty=format:COMMIT_TIME %ct', | ||
| '--name-only', | ||
| '--no-renames', | ||
| '-z', | ||
| 'HEAD', | ||
| '--', | ||
| relDir, | ||
| ], | ||
| { cwd: root } | ||
| ); | ||
| } catch { | ||
| return out; | ||
| } | ||
| // With `-z`, `git log` emits records separated by NUL. Each | ||
| // record is: "COMMIT_TIME <digits>\n<path1>\0<path2>\0...\0". | ||
| const tokens = res.stdout.split('\0'); | ||
| let i = 0; | ||
| while (i < tokens.length) { | ||
| const tok = tokens[i]; | ||
| if (!tok) { i++; continue; } | ||
| const m = tok.match(/^COMMIT_TIME (\d+)\n([\s\S]*)$/); | ||
| if (!m) { i++; continue; } | ||
| const ct = Number(m[1]); | ||
| const inline = m[2].trim(); | ||
| if (inline) out.set(inline, Math.max(out.get(inline) ?? 0, ct)); | ||
| i++; | ||
| while (i < tokens.length && tokens[i] && !tokens[i].startsWith('COMMIT_TIME ')) { | ||
| const pth = tokens[i].trim(); | ||
| if (pth) out.set(pth, Math.max(out.get(pth) ?? 0, ct)); | ||
| i++; | ||
| } | ||
| } | ||
| return out; | ||
| } | ||
|
|
||
| export async function effectiveMtime(file, commitTimes) { | ||
| let fsMtime = 0; | ||
| try { | ||
| const s = await stat(file); | ||
| fsMtime = Math.floor(s.mtimeMs / 1000); | ||
| } catch {} | ||
| const ct = commitTimes.get(rel(file)); | ||
| if (ct === undefined) return fsMtime; // never committed: filesystem only | ||
| return Math.max(ct, fsMtime); // dirty file: filesystem mtime wins | ||
| } | ||
|
|
||
| export async function newestEffectiveMtime(files, commitTimes) { | ||
| let max = 0; | ||
| for (const f of files) { | ||
| const t = await effectiveMtime(f, commitTimes); | ||
| if (t > max) max = t; | ||
| } | ||
| return max; | ||
| } |
There was a problem hiding this comment.
Critical Issues Identified
-
Regex Parsing Bug (
git log -z):
The current regex/^COMMIT_TIME (\d+)\n([\s\S]*)$/expects a newline character after the timestamp. However, when usinggit log -z, the commit header is terminated by a NUL byte (\0). On many platforms and Git versions, this means there is no newline in the token, causing the regex match to fail. As a result,commitTimesUnderreturns an empty map, and the tool silently falls back to filesystem-only mtimes. -
Logical Flaw with
Math.max(ct, fsMtime):
On a fresh checkout or CI clone, the filesystem mtime of all files is set to the checkout/clone time, which is always in the future relative to the commit time. Therefore,Math.max(ct, fsMtime)will always returnfsMtimefor all files. This means the check still compares filesystem mtimes, which will fail if the checkout spans across a second boundary or if sub-second precision is lost.
Proposed Solution
To make this check 100% robust and deterministic:
- Use
git status --porcelain -zto identify truly dirty/untracked files. - If a file is clean, use its commit time (
ct). - If a file is dirty, use its filesystem mtime (
fsMtime). - Update the regex to robustly handle optional newlines.
export async function commitTimesUnder(dir) {
const commitTimes = new Map();
const dirtyFiles = new Set();
const relDir = toRepoRelative(dir);
let res;
try {
res = await execFileP(
'git',
[
'log',
'--pretty=format:COMMIT_TIME %ct',
'--name-only',
'--no-renames',
'-z',
'HEAD',
'--',
relDir,
],
{ cwd: root }
);
const tokens = res.stdout.split('\u0000');
let i = 0;
while (i < tokens.length) {
const tok = tokens[i];
if (!tok) { i++; continue; }
const m = tok.match(/^COMMIT_TIME (\d+)(?:\n([\s\S]*))?$/);
if (!m) { i++; continue; }
const ct = Number(m[1]);
const inline = (m[2] ?? '').trim();
if (inline) commitTimes.set(inline, Math.max(commitTimes.get(inline) ?? 0, ct));
i++;
while (i < tokens.length && tokens[i] && !tokens[i].startsWith('COMMIT_TIME ')) {
const pth = tokens[i].trim();
if (pth) commitTimes.set(pth, Math.max(commitTimes.get(pth) ?? 0, ct));
i++;
}
}
} catch {}
try {
const statusRes = await execFileP(
'git',
['status', '--porcelain', '-z', '--', relDir],
{ cwd: root }
);
const lines = statusRes.stdout.split('\u0000');
for (let j = 0; j < lines.length; j++) {
const line = lines[j];
if (!line) continue;
if (line.length > 3 && (line[2] === ' ' || line[1] === ' ')) {
const path = line.slice(3);
dirtyFiles.add(path);
if (line.startsWith('R') || line.startsWith('C')) {
j++;
if (j < lines.length && lines[j]) {
dirtyFiles.add(lines[j]);
}
}
} else {
dirtyFiles.add(line);
}
}
} catch {}
return { commitTimes, dirtyFiles };
}
export async function effectiveMtime(file, combined) {
let fsMtime = 0;
try {
const s = await stat(file);
fsMtime = Math.floor(s.mtimeMs / 1000);
} catch {}
const rPath = rel(file);
const dirtyFiles = combined?.dirtyFiles;
if (dirtyFiles instanceof Set && dirtyFiles.has(rPath)) {
return fsMtime;
}
const commitTimes = combined?.commitTimes ?? combined;
const ct = commitTimes instanceof Map ? commitTimes.get(rPath) : undefined;
if (ct === undefined) return fsMtime;
return ct;
}
export async function newestEffectiveMtime(files, combined) {
let max = 0;
for (const f of files) {
const t = await effectiveMtime(f, combined);
if (t > max) max = t;
}
return max;
}![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| try { | ||
| entries = await readdir(dir, { withFileTypes: true }); | ||
| } catch { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Suggestion: walk suppresses all directory-read errors and pretends the tree is empty, so permission or I/O failures are misreported as "no src tree" and checks are skipped instead of failing. Only ENOENT should be treated as missing-directory; other errors should be surfaced. [incorrect condition logic]
Severity Level: Major ⚠️
- ❌ spec-mtime skips freshness checks for unreadable src trees.
- ❌ spec-coverage skips coverage checks for unreadable src trees.
- ⚠️ Packages misreported as data-only despite existing src/ code.
- ⚠️ I/O/permission misconfigurations silently masked during verification.Steps of Reproduction ✅
1. Create a package `packages/foo` with a readable `specs/SPEC.md` and a `src/` directory
whose contents are not readable (e.g., make `packages/foo/src` owned by another user or
chmod it to `000`) so that `readdir('packages/foo/src')` fails with a permission or I/O
error rather than ENOENT.
2. Run `node tools/spec-mtime.mjs` from the repo root; for each package it constructs
`srcDir = join(pkg, 'src')` and then iterates `for await (const f of walk(srcDir))
srcFiles.push(f);` inside a `try` block at `tools/spec-mtime.mjs:57-61`, expecting errors
to indicate a missing `src/` tree.
3. When `walk(srcDir)` calls `readdir(dir, { withFileTypes: true })` at
`tools/lib/git-mtime.mjs:41-42`, the permission/I/O error is thrown, but the broad `catch`
at `tools/lib/git-mtime.mjs:43-45` swallows it and simply `return`s, yielding no files and
no error to the caller.
4. Back in `tools/spec-mtime.mjs`, the `try` block at lines 59-61 observes no error and
`srcFiles.length === 0`, so the code treats `foo` as having "no src/ tree" and prints `OK:
foo/specs/SPEC.md (no src/ tree)` at lines 66-68, incorrectly skipping the freshness gate
for a package that actually has a `src/` tree but could not be read;
`tools/spec-coverage.mjs` shows similar behavior when it uses `for await (const f of
walk(join(pkgPath, 'src')))` at lines 164 and 205 and then treats `srcFiles.length === 0`
as "no src/ tree" at lines 165-176.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tools/lib/git-mtime.mjs
**Line:** 41:45
**Comment:**
*Incorrect Condition Logic: `walk` suppresses all directory-read errors and pretends the tree is empty, so permission or I/O failures are misreported as "no src tree" and checks are skipped instead of failing. Only `ENOENT` should be treated as missing-directory; other errors should be surfaced.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| } catch { | ||
| return out; | ||
| } |
There was a problem hiding this comment.
Suggestion: commitTimesUnder swallows every git log failure and returns an empty map, which silently degrades both freshness gates back to filesystem-only behavior. That reintroduces the original false-positive problem on fresh checkouts/worktrees and makes git failures invisible; propagate the error (or fail the gate) instead of silently falling back. [logic error]
Severity Level: Major ⚠️
- ⚠️ spec-mtime gate reverts to nondeterministic filesystem timestamps.
- ⚠️ spec-coverage mtime gate also loses git-awareness silently.
- ⚠️ Underlying git/log failures are completely hidden from developers.
- ⚠️ False-positive/negative freshness results possible in gitless runs.Steps of Reproduction ✅
1. From the repository root `/workspace/adt-bench`, run the spec freshness gate script
`node tools/spec-mtime.mjs` (entry point at `tools/spec-mtime.mjs:1`), which iterates
packages under `packages/` and, for each, calls `commitTimesUnder(pkg)` at
`tools/spec-mtime.mjs:72`.
2. In an environment where `git log` fails for `root` (e.g., `.git` directory missing,
non‑git copy of the tree, or `git` not installed), the `execFileP('git', [...], { cwd:
root })` invocation in `commitTimesUnder` at `tools/lib/git-mtime.mjs:73-85` throws.
3. The error is caught by the broad `catch` block at `tools/lib/git-mtime.mjs:87-89`,
which returns the empty map `out` without logging or failing, so `commitTimesUnder(pkg)`
yields an empty `Map<path, time>`.
4. Back in `tools/spec-mtime.mjs`, both `newestEffectiveMtime(srcFiles, combined)` at line
74 and `effectiveMtime(specPath, combined)` at line 75 fall back to filesystem mtimes
because `combined` lacks commit entries (see `effectiveMtime` at
`tools/lib/git-mtime.mjs:112-121`), silently degrading the gate to the old filesystem-only
behavior and hiding the underlying git failure from the user;
`tools/spec-coverage.mjs:166-168` uses the same pattern and is affected identically.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tools/lib/git-mtime.mjs
**Line:** 87:89
**Comment:**
*Logic Error: `commitTimesUnder` swallows every `git log` failure and returns an empty map, which silently degrades both freshness gates back to filesystem-only behavior. That reintroduces the original false-positive problem on fresh checkouts/worktrees and makes git failures invisible; propagate the error (or fail the gate) instead of silently falling back.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/issues/11-verify-result.md`:
- Around line 95-129: Update the documentation in
docs/issues/11-verify-result.md (lines 95-129) to accurately reflect the final
three-file structure. The diff stats block (lines 98-100) currently shows only 2
files changed but should show 3 files including the new tools/lib/git-mtime.mjs.
Replace the implementation descriptions that claim helpers like
commitTimesUnder, effectiveMtime, and newestEffectiveMtime were "added to"
spec-mtime.mjs and spec-coverage.mjs separately with a description of the actual
architecture: a new shared tools/lib/git-mtime.mjs module that exports these
helpers, followed by descriptions of how spec-mtime.mjs and spec-coverage.mjs
import from this shared module rather than defining helpers inline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af293b74-42b9-4b69-a07d-1bbc6b8d02b7
📒 Files selected for processing (4)
docs/issues/11-verify-result.mdtools/lib/git-mtime.mjstools/spec-coverage.mjstools/spec-mtime.mjs
| Concrete changes (`git diff --stat origin/main`): | ||
|
|
||
| ```text | ||
| tools/spec-coverage.mjs | 87 +++++++++++++++++++++++++++++++++-- | ||
| tools/spec-mtime.mjs | 120 +++++++++++++++++++++++++++++++++++++++++------- | ||
| 2 files changed, 187 insertions(+), 20 deletions(-) | ||
| ``` | ||
|
|
||
| - `tools/spec-mtime.mjs` | ||
| - New helper `commitTimesUnder(dir)` does a single | ||
| `git log --pretty=format:'COMMIT_TIME %ct' --name-only --no-renames -z HEAD -- <dir>` | ||
| and parses the NUL-separated output into | ||
| `Map<repo-relative-path, latest-commit-time-seconds>`. | ||
| - New helper `effectiveMtime(file, commitTimes)` returns | ||
| `Math.max(fs_mtime, commit_time)` (filesystem-only when the | ||
| file has never been committed). | ||
| - The per-package loop now compares `effectiveMtime(SPEC.md)` | ||
| against `max(effectiveMtime(src/*))` instead of filesystem | ||
| mtimes. | ||
| - The old per-file `newestMtime` filesystem helper is removed. | ||
| - The user-facing error message is unchanged (still suggests | ||
| `touch specs/SPEC.md`), so the existing `docs/spec-style.md` | ||
| §"Common failures and fixes" guidance remains accurate for the | ||
| cases it covers (uncommitted SPEC needs a bump, dirty SPEC needs | ||
| a commit). | ||
|
|
||
| - `tools/spec-coverage.mjs` | ||
| - Same three helpers (`commitTimesUnder`, `rel`, | ||
| `effectiveMtime`) plus `newestEffectiveMtime` are added. | ||
| - The §"2. mtime gate" inside `checkPackage` is rewritten to use | ||
| `commitTimesUnder(pkgPath)` once and then | ||
| `newestEffectiveMtime(srcFiles, combined)` / | ||
| `effectiveMtime(specPath, combined)`. No other behavior | ||
| changes. | ||
|
|
There was a problem hiding this comment.
Documentation describes outdated implementation; doesn't reflect final refactored code.
The diff stats (lines 98-100) show only 2 files changed, but the actual PR includes a new third file tools/lib/git-mtime.mjs. The implementation descriptions (lines 103-128) claim helpers like commitTimesUnder, effectiveMtime, and newestEffectiveMtime were "added to" each file separately, but the actual code extracts these helpers into a shared module and both scripts import them.
This inconsistency will confuse future maintainers who read this doc expecting to find the helpers inline in spec-mtime.mjs and spec-coverage.mjs. The PR objectives explicitly state: "Shared git-aware mtime helpers were extracted into tools/lib/git-mtime.mjs to reduce code duplication and improve maintainability."
Update the doc to describe the final three-file structure:
📝 Suggested updated description
Replace lines 95-129 with:
Concrete changes (`git diff --stat origin/main`):
```text
+ tools/lib/git-mtime.mjs | <new file>
tools/spec-coverage.mjs | <modified>
tools/spec-mtime.mjs | <modified>
- 2 files changed, 187 insertions(+), 20 deletions(-)
+ 3 files changed, ...
```
+- `tools/lib/git-mtime.mjs` (new shared module)
+ - Exports `commitTimesUnder(dir)` which does a single
+ `git log --pretty=format:'COMMIT_TIME %ct' --name-only --no-renames -z HEAD -- <dir>`
+ and parses the NUL-separated output into
+ `Map<repo-relative-path, latest-commit-time-seconds>`.
+ - Exports `effectiveMtime(file, commitTimes)` returning
+ `Math.max(fs_mtime, commit_time)` (filesystem-only when the
+ file has never been committed).
+ - Exports `newestEffectiveMtime(files, commitTimes)` to compute
+ the maximum effective mtime across a set of files.
+ - Exports `walk(dir)` generator and `rel(file)` path helper.
+ - Exports the computed repo `root` constant.
+
- `tools/spec-mtime.mjs`
- - New helper `commitTimesUnder(dir)` does a single...
- - New helper `effectiveMtime(file, commitTimes)` returns...
+ - Imports helpers from `./lib/git-mtime.mjs`.
- The per-package loop now compares `effectiveMtime(SPEC.md)`
against `max(effectiveMtime(src/*))` instead of filesystem
mtimes.
- - The old per-file `newestMtime` filesystem helper is removed.
...
- `tools/spec-coverage.mjs`
- - Same three helpers (`commitTimesUnder`, `rel`,
- `effectiveMtime`) plus `newestEffectiveMtime` are added.
+ - Imports helpers from `./lib/git-mtime.mjs`.
- The §"2. mtime gate" inside `checkPackage` is rewritten to use
`commitTimesUnder(pkgPath)` once and then
`newestEffectiveMtime(srcFiles, combined)` /
- `effectiveMtime(specPath, combined)`. No other behavior
- changes.
+ `effectiveMtime(specPath, combined)`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Concrete changes (`git diff --stat origin/main`): | |
| ```text | |
| tools/spec-coverage.mjs | 87 +++++++++++++++++++++++++++++++++-- | |
| tools/spec-mtime.mjs | 120 +++++++++++++++++++++++++++++++++++++++++------- | |
| 2 files changed, 187 insertions(+), 20 deletions(-) | |
| ``` | |
| - `tools/spec-mtime.mjs` | |
| - New helper `commitTimesUnder(dir)` does a single | |
| `git log --pretty=format:'COMMIT_TIME %ct' --name-only --no-renames -z HEAD -- <dir>` | |
| and parses the NUL-separated output into | |
| `Map<repo-relative-path, latest-commit-time-seconds>`. | |
| - New helper `effectiveMtime(file, commitTimes)` returns | |
| `Math.max(fs_mtime, commit_time)` (filesystem-only when the | |
| file has never been committed). | |
| - The per-package loop now compares `effectiveMtime(SPEC.md)` | |
| against `max(effectiveMtime(src/*))` instead of filesystem | |
| mtimes. | |
| - The old per-file `newestMtime` filesystem helper is removed. | |
| - The user-facing error message is unchanged (still suggests | |
| `touch specs/SPEC.md`), so the existing `docs/spec-style.md` | |
| §"Common failures and fixes" guidance remains accurate for the | |
| cases it covers (uncommitted SPEC needs a bump, dirty SPEC needs | |
| a commit). | |
| - `tools/spec-coverage.mjs` | |
| - Same three helpers (`commitTimesUnder`, `rel`, | |
| `effectiveMtime`) plus `newestEffectiveMtime` are added. | |
| - The §"2. mtime gate" inside `checkPackage` is rewritten to use | |
| `commitTimesUnder(pkgPath)` once and then | |
| `newestEffectiveMtime(srcFiles, combined)` / | |
| `effectiveMtime(specPath, combined)`. No other behavior | |
| changes. | |
| Concrete changes (`git diff --stat origin/main`): | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/issues/11-verify-result.md` around lines 95 - 129, Update the
documentation in docs/issues/11-verify-result.md (lines 95-129) to accurately
reflect the final three-file structure. The diff stats block (lines 98-100)
currently shows only 2 files changed but should show 3 files including the new
tools/lib/git-mtime.mjs. Replace the implementation descriptions that claim
helpers like commitTimesUnder, effectiveMtime, and newestEffectiveMtime were
"added to" spec-mtime.mjs and spec-coverage.mjs separately with a description of
the actual architecture: a new shared tools/lib/git-mtime.mjs module that
exports these helpers, followed by descriptions of how spec-mtime.mjs and
spec-coverage.mjs import from this shared module rather than defining helpers
inline.
1 participant
User description
Closes #11.
Summary
The spec-mtime freshness gate was using filesystem mtime, which gives false-positive failures on fresh checkouts / worktrees / CI clones. This PR makes the gate git-aware: each file's effective mtime is max(commit_time, filesystem_mtime). Deterministic across checkouts, still catches truly stale SPECs.
Test plan
pnpm run verifypasses end-to-end (49.2s wall, all 5 sub-steps exit 0)Risk
Low. The gate's intent is preserved; only the time source changes. Error message text is unchanged, so docs/spec-style.md guidance still applies to the cases it covers.
Summary by cubic
Make the spec freshness checks git-aware by using an effective mtime = max(commit time, filesystem mtime). This removes false positives on fresh checkouts and CI while keeping the gate strict for real stale specs.
Bug Fixes
tools/spec-mtime.mjs: compareeffectiveMtime(specs/SPEC.md)to newesteffectiveMtime(src/*).tools/spec-coverage.mjs: replace the duplicate mtime gate with the same git-aware logic.Refactors
tools/lib/git-mtime.mjs(commitTimesUnder,effectiveMtime,newestEffectiveMtime,walk) to remove duplication.Written for commit e8edadb. Summary will update on new commits.
CodeAnt-AI Description
Fix spec freshness checks on fresh checkouts
What Changed
pnpm run verifyno longer fails on fresh clones, worktrees, or CI checkouts when the spec is actually up to date.Impact
✅ Fewer false verify failures on fresh checkouts✅ Reliable spec checks in CI and worktrees✅ Clearer troubleshooting for spec-check failures💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Documentation
Bug Fixes
Refactor