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

fix(#4685): a directory artifact fails its own entry instead of aborting the check (#4735)

Browse Source 
* fix(#4685): a directory artifact fails its own entry instead of aborting the check

`must_haves.artifacts` entries are read with `safeReadFile`, which rethrows every
errno except ENOENT. A listed path that is a directory therefore threw EISDIR out
of the per-artifact loop: `query verify.artifacts` printed

    Error: EISDIR: illegal operation on a directory, read

and reported NOTHING — not the offending entry, and not the plan's other,
perfectly checkable artifacts. One directory entry disabled the whole plan's
check. Reproduced against a real plan before the fix, and after.

A directory is now reported as that entry's own failure, with an issue distinct
from `File not found` (the path did resolve; it simply is not the thing an
artifact entry can be checked against), and every other artifact in the plan is
still checked and reported independently. Anything else the stat or read throws
becomes that entry's failure too, carrying its errno, rather than discarding the
run — a check that disappears is worse than one that fails, because a failure is
visible.

Verifying directories properly — matching `contains:`/`min_lines:`/`exports:`
across the files inside one — is a feature decision and deliberately not made
here, per the issue's stated scope. Authoring-time rejection of a directory path
is likewise left alone: the brief raises it as a separate question, and the
runtime fix does not depend on it.

Also, found in pre-PR review and pre-existing: `safeReadFile(...) || ''` turned a
post-stat ENOENT into empty content, so an artifact declaring only
`path`/`provides` had no criterion left to fail and passed, having checked
nothing. A null read now reports that instead of inheriting a pass. It can only
turn a false pass into a failure.

Verified: reverting src/verify.cts to the merge-base turns both new rows red with
the exact EISDIR message; lint:ci exit 0; full suite 24/24 chunks, 37,280 tests,
0 failures; tests/verify.test.cjs 219/219.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S4mJZpSNwoyVtRVUQoijfL

* chore(#4685): backfill changeset PR number to 4735

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S4mJZpSNwoyVtRVUQoijfL

* test(#4685): pin the injected-I/O branches, and narrow the guard's comment

Review findings from #4735.

Major — the two error branches this PR adds were untested, and the PR body
claimed the mid-check ENOENT window was "not deterministically reproducible
through the CLI seam these tests drive." That was wrong: ADR-3574 records this
repo's convention for exactly this — inject filesystem failures by
monkeypatching the fs method, never by chmod or mode-bit tricks, which root
bypasses and yields a test that passes with zero coverage in root Docker and CI.

Both branches are now pinned that way. The injection runs in the CHILD via
NODE_OPTIONS=--require, because `output()` writes fd 1 directly
(`writeAllSync(1, …)`, io.cjs) rather than through console.log, so an in-process
call cannot have its JSON captured. The preload patches the child's own module
objects, which the compiled code reads at call time.

  - a file that disappears between stat and read now fails as that entry rather
    than passing on empty content
  - a non-ENOENT errno (EACCES) is reported as that entry's failure, carrying its
    code, so an operator can tell a permissions problem from an I/O one

The injection matches the target by path SUFFIX, not string equality: the first
cut compared absolute paths, and a /tmp vs /private/tmp prefix difference
silently disarmed it — the test passed while asserting nothing. A disarmed
injection test is worse than no test, so the reason is recorded at the call site.

Nit — the comment above the try block said the guard "covers what the stat and
read below actually throw", which reads as if the min_lines/contains/exports
checks inside the same block were deliberately guarded too. They are pure string
operations and cannot throw; the comment now says so rather than implying a
guarantee it does not make.

Verified: reverting src/verify.cts to the merge-base turns all three #4685 rows
red — the directory row and both new ones; lint:ci exit 0; full suite 24/24
chunks, 37,389 tests, 0 failures; tests/verify.test.cjs 221/221.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01S4mJZpSNwoyVtRVUQoijfL

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
Behruz Nassre Esfahani
2026-09-15 22:39:52 -07:00
committed by GitHub
parent ecc508139a
commit febe6c9885
3 changed files with 230 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/4685-verify-artifacts-directory.md Normal file
View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 4735
---
`verify.artifacts` no longer aborts a plan's whole artifact check when one listed path is a directory. Reading a directory raised `EISDIR` out of the per-artifact loop, so the command printed `Error: EISDIR: illegal operation on a directory, read` and reported nothing at all — neither the offending entry nor the plan's other, perfectly checkable artifacts. A directory now fails as its own entry, with an issue distinct from `File not found`, and every other artifact is still checked and reported independently. A path that stat or read fails on for any other reason (a permissions error, an unreachable mount, or an artifact that disappears mid-check) fails the same way, carrying its errno, instead of discarding the run.

81
src/verify.cts
View File

@@ -1426,27 +1426,72 @@ function cmdVerifyArtifacts(cwd: string, planFilePath: string, raw: boolean): vo
const exists = fs.existsSync(artFullPath);
const check: Record<string, unknown> = { path: artPath, exists, issues: [], passed: false };
if (exists) {
const fileContent = safeReadFile(artFullPath) || '';
const lineCount = fileContent.split('\n').length;
// #4685: one artifact's I/O problem is that artifact's failure, never the
// whole plan's. `safeReadFile` rethrows every errno except ENOENT, so before
// this an unreadable entry — a directory most commonly, but equally an EACCES
// file or a dangling mount — threw out of the loop and the command reported
// NOTHING: not the offending entry, and not the plan's other, perfectly good
// artifacts either. A check that disappears is worse than a check that fails,
// because a failure is visible.
//
// Scope of this guard, stated precisely (review nit): the `try` encloses the
// whole per-artifact body, but the only statements in it that can throw are the
// `statSync` and the read — the `min_lines`/`contains`/`exports` checks below
// are pure string operations. So this catches I/O, and nothing here is a
// deliberate guard around those criteria checks. A path `fs.existsSync` already
// rejected never reaches here either (that is the `File not found` branch), so
// this is not a claim to catch every way a path can be unusable.
try {
if (exists) {
// A directory is reported as its own kind of failure, distinct from
// `File not found`: the path resolved, it simply is not the thing an
// artifact entry can be checked against. Verifying a directory (matching
// `contains:`/`min_lines:`/`exports:` across the files inside it) is a
// feature decision, deliberately not made here.
if (fs.statSync(artFullPath).isDirectory()) {
(check['issues'] as string[]).push('Not a file: path is a directory');
} else {
// `safeReadFile` returns null on ENOENT, and `|| ''` would turn that
// into an empty file — which an entry carrying only `path`/`provides`
// would then PASS, having checked nothing. `statSync` just succeeded, so
// a null here means the artifact went away mid-check. Report that rather
// than inheriting a pass from it. (Pre-existing above this fix, reachable
// through the same race after `existsSync`; found in review.)
const rawContent = safeReadFile(artFullPath);
if (rawContent === null) {
(check['issues'] as string[]).push('Unreadable: disappeared during check');
results.push(check);
continue;
}
const fileContent = rawContent;
const lineCount = fileContent.split('\n').length;
if (artifact['min_lines'] && lineCount < (artifact['min_lines'] as number)) {
(check['issues'] as string[]).push(`Only ${lineCount} lines, need ${artifact['min_lines'] as number}`);
}
if (artifact['contains'] && !fileContent.includes(artifact['contains'] as string)) {
(check['issues'] as string[]).push(`Missing pattern: ${artifact['contains'] as string}`);
}
if (artifact['exports']) {
const exports = Array.isArray(artifact['exports'])
? artifact['exports']
: [artifact['exports']];
for (const exp of exports) {
if (!fileContent.includes(exp as string)) (check['issues'] as string[]).push(`Missing export: ${exp as string}`);
if (artifact['min_lines'] && lineCount < (artifact['min_lines'] as number)) {
(check['issues'] as string[]).push(`Only ${lineCount} lines, need ${artifact['min_lines'] as number}`);
}
if (artifact['contains'] && !fileContent.includes(artifact['contains'] as string)) {
(check['issues'] as string[]).push(`Missing pattern: ${artifact['contains'] as string}`);
}
if (artifact['exports']) {
const exports = Array.isArray(artifact['exports'])
? artifact['exports']
: [artifact['exports']];
for (const exp of exports) {
if (!fileContent.includes(exp as string)) (check['issues'] as string[]).push(`Missing export: ${exp as string}`);
}
}
check['passed'] = (check['issues'] as string[]).length === 0;
}
} else {
(check['issues'] as string[]).push('File not found');
}
check['passed'] = (check['issues'] as string[]).length === 0;
} else {
(check['issues'] as string[]).push('File not found');
} catch (err) {
// Unreadable for some other reason. Record the errno rather than a generic
// message — an operator seeing EACCES acts differently from one seeing EIO —
// and leave `passed` false.
const e = err as NodeJS.ErrnoException;
(check['issues'] as string[]).push(`Unreadable: ${e.code || (e.message ?? String(err))}`);
check['passed'] = false;
}
results.push(check);

162
tests/verify.test.cjs
View File

@@ -1695,6 +1695,168 @@ describe('verify artifacts command', () => {
);
});
// #4685: a directory-valued artifact path used to abort the WHOLE command.
// `safeReadFile`/`platformReadSync` rethrows every errno except ENOENT, so
// `fs.readFileSync` on a directory threw EISDIR out of the per-artifact loop and
// the command printed `Error: EISDIR: illegal operation on a directory, read`
// with no results at all — not for the directory entry, and not for the plan's
// other, perfectly checkable artifacts. Reproduced against a real plan before
// the fix; these rows are the contract that replaced it.
test('#4685: a directory artifact fails as its own entry and does not abort the others', () => {
fs.mkdirSync(path.join(tmpDir, 'src', 'snapshots'), { recursive: true });
fs.writeFileSync(path.join(tmpDir, 'src', 'snapshots', 'a.snap'), 'snap\n');
fs.writeFileSync(path.join(tmpDir, 'src', 'app.js'), 'hello world\n');
writePlanWithArtifacts(tmpDir, [
'- path: src/snapshots',
' provides: "a directory of snapshots"',
'- path: src/app.js',
' contains: "hello"',
]);
const result = runGsdTools('verify artifacts .planning/phases/01-test/01-01-PLAN.md', tmpDir);
assert.ok(result.success, `Command crashed instead of reporting: ${result.error}`);
const output = JSON.parse(result.output);
assert.strictEqual(output.total, 2, `both artifacts must be checked: ${JSON.stringify(output)}`);
assert.strictEqual(output.passed, 1, `the file artifact must still pass: ${JSON.stringify(output)}`);
assert.strictEqual(output.all_passed, false);
const dirCheck = output.artifacts.find((a) => a.path === 'src/snapshots');
assert.ok(dirCheck, 'the directory entry must be reported, not swallowed');
assert.strictEqual(dirCheck.passed, false);
assert.strictEqual(dirCheck.exists, true, 'the path does resolve — this is not "not found"');
assert.ok(
dirCheck.issues.some((i) => /directory/i.test(i)),
`the directory entry needs its own distinct issue, not "File not found": ${JSON.stringify(dirCheck.issues)}`
);
assert.equal(
dirCheck.issues.some((i) => /not found/i.test(i)), false,
'a directory that exists must not be reported as missing'
);
// The point of the fix: the OTHER artifact is still independently checked.
const fileCheck = output.artifacts.find((a) => a.path === 'src/app.js');
assert.ok(fileCheck, 'the file artifact must still be reported');
assert.strictEqual(fileCheck.passed, true, `the file artifact is fine and must say so: ${JSON.stringify(fileCheck)}`);
assert.deepStrictEqual(fileCheck.issues, []);
});
// The degenerate shape: nothing else in the plan can carry the result, so a
// crash here would leave the caller with no verdict at all.
test('#4685: a plan whose only artifact is a directory still returns a structured verdict', () => {
fs.mkdirSync(path.join(tmpDir, 'src', 'snapshots'), { recursive: true });
writePlanWithArtifacts(tmpDir, [
'- path: src/snapshots',
' provides: "a directory of snapshots"',
]);
const result = runGsdTools('verify artifacts .planning/phases/01-test/01-01-PLAN.md', tmpDir);
assert.ok(result.success, `Command crashed instead of reporting: ${result.error}`);
const output = JSON.parse(result.output);
assert.strictEqual(output.total, 1);
assert.strictEqual(output.passed, 0);
assert.strictEqual(output.all_passed, false, 'a directory-only block must never read as a pass');
});
// #4685 review follow-up: the two error branches this PR ADDS are reachable and
// must be pinned deterministically. Per ADR-3574, filesystem failures are injected
// by monkeypatching the fs method and restoring after — never by chmod or mode-bit
// tricks, which root bypasses (yielding a test that passes with zero coverage in
// root Docker and CI).
//
// The injection runs in the CHILD via `NODE_OPTIONS=--require`, because
// `output()` writes fd 1 directly (`writeAllSync(1, …)`, io.cjs) rather than
// through console.log, so an in-process call cannot have its JSON captured. The
// preload patches the child's own module objects, which the compiled code reads at
// call time (`shell_command_projection_cjs_1.platformReadSync(…)`,
// `node_fs_1.default.statSync(…)`), and the process exits at the end of the case,
// so no restore is needed beyond its lifetime.
describe('#4685: injected I/O failures on one artifact (ADR-3574 monkeypatching)', () => {
const LIB = path.join(__dirname, '..', 'gsd-core', 'bin', 'lib');
function withInjection(mode, targetPath) {
const preload = path.join(tmpDir, `inject-${mode}.cjs`);
fs.writeFileSync(preload, `
const target = ${JSON.stringify(targetPath)};
if (${JSON.stringify(mode)} === 'enoent-read') {
const sp = require(${JSON.stringify(path.join(LIB, 'shell-command-projection.cjs'))});
const orig = sp.platformReadSync;
// Match on suffix, not string equality: the child resolves the artifact path
// itself, and a /tmp vs /private/tmp prefix difference would silently disarm the
// injection and leave the test asserting nothing.
sp.platformReadSync = (p, o) => (String(p).endsWith(target) ? null : orig(p, o));
} else {
const nodeFs = require('node:fs');
const orig = nodeFs.statSync;
nodeFs.statSync = (p, o) => {
if (String(p).endsWith(target)) throw Object.assign(new Error('EACCES: permission denied'), { code: 'EACCES' });
return orig(p, o);
};
}
`);
return { NODE_OPTIONS: `--require ${preload}` };
}
function writeFixture() {
fs.writeFileSync(path.join(tmpDir, 'src', 'app.js'), 'hello world\n');
writePlanWithArtifacts(tmpDir, [
'- path: src/app.js',
' provides: "a real file, no criteria declared"',
]);
return path.join('src', 'app.js');
}
test('a file that disappears between stat and read fails instead of passing empty', () => {
// The latent bug this PR also fixes: `safeReadFile(...) || ''` turned a
// post-stat ENOENT into empty content, and an entry declaring only
// `path`/`provides` then had NO criterion left to fail — so it passed, having
// checked nothing. platformReadSync returns null on ENOENT, so returning null
// reproduces exactly that window.
const target = writeFixture();
const result = runGsdTools(
'verify artifacts .planning/phases/01-test/01-01-PLAN.md',
tmpDir,
withInjection('enoent-read', target),
);
assert.ok(result.success, `Command failed: ${result.error}`);
const output = JSON.parse(result.output);
const check = output.artifacts[0];
assert.strictEqual(
check.passed, false,
`an artifact whose content could not be read must not pass: ${JSON.stringify(check)}`
);
assert.ok(
check.issues.some((i) => /disappeared during check/i.test(i)),
`expected the mid-check disappearance to be named: ${JSON.stringify(check.issues)}`
);
assert.strictEqual(output.all_passed, false);
});
test('a non-ENOENT errno is reported as that entry\'s failure, carrying its code', () => {
// The generic catch branch. EACCES is the realistic case (an unreadable parent
// directory); the assertion pins that the errno reaches the operator rather
// than a generic message, because EACCES and EIO call for different responses.
const target = writeFixture();
const result = runGsdTools(
'verify artifacts .planning/phases/01-test/01-01-PLAN.md',
tmpDir,
withInjection('eacces-stat', target),
);
assert.ok(result.success, `Command failed: ${result.error}`);
const output = JSON.parse(result.output);
const check = output.artifacts[0];
assert.strictEqual(check.passed, false);
assert.ok(
check.issues.some((i) => i.includes('EACCES')),
`the errno must reach the operator: ${JSON.stringify(check.issues)}`
);
assert.strictEqual(output.all_passed, false);
});
});
// A MIXED artifacts block (one bare-string prose bullet + one well-formed
// `path:` entry) must not be disturbed by the positive-evidence floor: the
// string is item-skipped, the real entry is checked, results.length === 1 > 0,