From aa68c3e3b3719171ed1e46d80089d15d817616f2 Mon Sep 17 00:00:00 2001 From: mavericksea-ai <274285725+mavericksea-ai@users.noreply.github.com> Date: Sat, 26 Sep 2026 19:46:13 +0800 Subject: [PATCH] fix(constraints): floor-guard reference reads hunk lines as content and runs from the top Two follow-ups from the review on #600, both already on main before it: - The diff walk treated any line starting with `---` or `+++` as a file header. Inside a hunk those lines are content. An added line that starts with `++` (`++n; // eslint-disable-line no-plusplus`) shows as `+++n; ...` and was dropped, so the suppression passed. An added line that starts with `++ ` was taken as a header, so the lines after it were filed under a made-up path and a removed assertion further down the same test file went unreported. A removed line starting with `--` never reached the removed list either. Headers are now read only between a file's `diff` line and its first `@@`. - Run from a subfolder, `git ls-files --others` listed only that subfolder's untracked files, relative to it, while `git diff` covers the whole tree from the top. An untracked suppression elsewhere in the repository was missed, and one inside the subfolder was reported under a different name from the tracked files. The guard now changes to `git rev-parse --show-toplevel` first, and exits 2 outside a work tree. The test gains six cases: five fail on main, and the sixth (outside a work tree) passes on both and covers the new exit-2 path; 28 pass. The test now also runs in the macOS/Windows matrix. --- .github/workflows/test-plugin-install.yml | 1 + scripts/floor-guard-reference-test.js | 89 ++++++++++++++++++- .../references/floor-guard.md | 32 +++++-- 3 files changed, 109 insertions(+), 13 deletions(-) diff --git a/.github/workflows/test-plugin-install.yml b/.github/workflows/test-plugin-install.yml index 6256f41d..b25ba319 100644 --- a/.github/workflows/test-plugin-install.yml +++ b/.github/workflows/test-plugin-install.yml @@ -99,6 +99,7 @@ jobs: scripts/validate-reference-links-test.js scripts/validate-commands-test.js scripts/validate-artifact-paths-test.js + scripts/floor-guard-reference-test.js - name: Validate all skills run: node scripts/validate-skills.js diff --git a/scripts/floor-guard-reference-test.js b/scripts/floor-guard-reference-test.js index a66cabe6..39d8c903 100644 --- a/scripts/floor-guard-reference-test.js +++ b/scripts/floor-guard-reference-test.js @@ -3,8 +3,8 @@ 'use strict'; // Contract test for skills/constraint-driven-development/references/floor-guard.md. -// The fenced `js` block is extracted unchanged and run, with real git, against fourteen -// small fixtures: each is a fresh repository with one source file, one test file and a +// The fenced `js` block is extracted unchanged and run, with real git, against small +// fixtures: each is a fresh repository with one source file, one test file and a // CONSTRAINTS.md carrying a floor bullet, a coverage minimum, a bundle maximum and an // Exceptions table. Loosening moves must exit 1, tightening and no-op moves must exit 0. @@ -97,8 +97,14 @@ function editConstraints(root, from, to) { fs.writeFileSync(file, text.replace(from, to)); } -function runGuard(root) { - return spawnSync(process.execPath, [guard, '--base', 'HEAD'], { cwd: root, encoding: 'utf8' }); +function commitFile(root, name, text) { + fs.writeFileSync(path.join(root, name), text); + git(root, 'add', name); + git(root, 'commit', '-qm', `add ${name}`); +} + +function runGuard(root, cwd = root) { + return spawnSync(process.execPath, [guard, '--base', 'HEAD'], { cwd, encoding: 'utf8' }); } const cases = [ @@ -208,3 +214,78 @@ test('floor guard: a threshold that loses its direction words is reported as rem assert.match(result.stderr, /\[threshold-removed\]/); assert.match(result.stderr, /direction words/); }); + +// Inside a hunk every line is content. An added line that starts with `++` shows in the diff +// as `+++...`, and a removed line that starts with `--` as `---...`: neither is a file header. +test('floor guard: an added line that starts with ++ is still checked', () => { + const root = makeRepo(); + fs.writeFileSync( + path.join(root, 'app.js'), + 'let n = 0;\n++n; // eslint-disable-line no-plusplus\nmodule.exports = n;\n', + ); + const result = runGuard(root); + assert.equal(result.status, 1, result.stderr); + assert.match(result.stderr, /\[silenced-checker\] app\.js:/); +}); + +test('floor guard: an added line that starts with "++ " does not rename the file for later hunks', () => { + const root = makeRepo(); + commitFile(root, 'counter.test.js', [ + "const assert = require('assert');", + 'let n = 0;', + 'n += 1;', + 'const m = 2;', + 'assert.equal(n, 1);', + 'assert.equal(m, 2);', + '', + ].join('\n')); + const file = path.join(root, 'counter.test.js'); + const text = fs.readFileSync(file, 'utf8'); + fs.writeFileSync(file, text.replace('n += 1;', '++ n;').replace('assert.equal(m, 2);\n', '')); + const result = runGuard(root); + assert.equal(result.status, 1, result.stderr); + assert.match(result.stderr, /\[assertion-removed\] counter\.test\.js:/); +}); + +test('floor guard: a removed line that starts with -- is still checked', () => { + const root = makeRepo(); + commitFile(root, 'countdown.test.js', [ + "const assert = require('assert');", + 'let n = 1;', + '--n; assert.equal(n, 0);', + '', + ].join('\n')); + const file = path.join(root, 'countdown.test.js'); + fs.writeFileSync(file, fs.readFileSync(file, 'utf8').replace('--n; assert.equal(n, 0);\n', '')); + const result = runGuard(root); + assert.equal(result.status, 1, result.stderr); + assert.match(result.stderr, /\[assertion-removed\] countdown\.test\.js:/); +}); + +// `git ls-files` lists only the current directory's subtree, relative to it, while `git diff` +// covers the whole tree with paths from the top. Run from a subfolder, the guard has to see the +// same files, under the same names, as it does from the top. +test('floor guard: run from a subfolder, an untracked file outside it is still checked', () => { + const root = makeRepo(); + fs.mkdirSync(path.join(root, 'sub')); + fs.writeFileSync(path.join(root, 'new.js'), '// @ts-ignore\nmodule.exports = 1;\n'); + const result = runGuard(root, path.join(root, 'sub')); + assert.equal(result.status, 1, result.stderr); + assert.match(result.stderr, /\[silenced-checker\] new\.js:/); +}); + +test('floor guard: run from a subfolder, an untracked file inside it is named from the top', () => { + const root = makeRepo(); + fs.mkdirSync(path.join(root, 'sub')); + fs.writeFileSync(path.join(root, 'sub', 'new.js'), '// @ts-ignore\nmodule.exports = 1;\n'); + const result = runGuard(root, path.join(root, 'sub')); + assert.equal(result.status, 1, result.stderr); + assert.match(result.stderr, /\[silenced-checker\] sub\/new\.js:/); +}); + +test('floor guard: outside a git work tree exits 2, never 0', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'agent-skills-floor-guard-no-repo-')); + sandboxes.push(dir); + const result = runGuard(dir); + assert.equal(result.status, 2, result.stderr); +}); diff --git a/skills/constraint-driven-development/references/floor-guard.md b/skills/constraint-driven-development/references/floor-guard.md index 259b3221..0fa8a694 100644 --- a/skills/constraint-driven-development/references/floor-guard.md +++ b/skills/constraint-driven-development/references/floor-guard.md @@ -33,6 +33,13 @@ const git = (args, { diffExit = false } = {}) => { }; const bail = (msg) => { console.error('floor-guard: ' + msg); process.exit(2); }; +// Run from the top of the work tree. `git ls-files` lists only the current directory's subtree, +// relative to it, so a guard started in a subfolder would miss untracked files elsewhere and name +// the rest differently from `git diff`, which always covers the whole tree. +const top = git(['rev-parse', '--show-toplevel'])?.trim(); +if (!top) bail('not inside a git work tree'); +process.chdir(top); + // Merge base; bail to exit 2 rather than pretending a shallow/rootless clone is clean. const mergeBase = git(['merge-base', base, 'HEAD'])?.trim(); if (!mergeBase) bail('no merge base against ' + base); @@ -49,19 +56,26 @@ const untracked = untrackedFiles.split('\n').filter(Boolean).map((f) => { }).join('\n'); const diff = tracked + '\n' + untracked; -// Walk the diff. Both headers name the file, so a deletion (`+++ /dev/null`) keeps its name. +// Walk the diff. `---` and `+++` are file headers only between a file's `diff` line and its first +// `@@` hunk; inside a hunk every line is content, so an added `++i` (shown as `+++i`) or a removed +// `-- comment` is a change, not a header. Both headers name the file, so a deletion +// (`+++ /dev/null`) keeps its name. const added = [], removed = [], deleted = []; const pathOf = (s) => s.replace(/^[ab]\//, ''); -let file = '', oldFile = ''; +let file = '', oldFile = '', inHeader = false; for (const line of diff.split('\n')) { - if (line.startsWith('--- ')) oldFile = pathOf(line.slice(4)); - else if (line.startsWith('+++ ')) { - const newFile = pathOf(line.slice(4)); - file = newFile === '/dev/null' ? oldFile : newFile; - if (newFile === '/dev/null') deleted.push(file); + if (line.startsWith('diff ')) inHeader = true; + else if (line.startsWith('@@')) inHeader = false; + else if (inHeader) { + if (line.startsWith('--- ')) oldFile = pathOf(line.slice(4)); + else if (line.startsWith('+++ ')) { + const newFile = pathOf(line.slice(4)); + file = newFile === '/dev/null' ? oldFile : newFile; + if (newFile === '/dev/null') deleted.push(file); + } } - else if (line.startsWith('+') && !line.startsWith('+++')) added.push({ file, text: line.slice(1) }); - else if (line.startsWith('-') && !line.startsWith('---')) removed.push({ file, text: line.slice(1) }); + else if (line.startsWith('+')) added.push({ file, text: line.slice(1) }); + else if (line.startsWith('-')) removed.push({ file, text: line.slice(1) }); } const findings = [];