diff --git a/.changeset/4685-verify-artifacts-directory.md b/.changeset/4685-verify-artifacts-directory.md new file mode 100644 index 000000000..aabc760ce --- /dev/null +++ b/.changeset/4685-verify-artifacts-directory.md @@ -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. diff --git a/src/verify.cts b/src/verify.cts index 87fe17cd9..dd9ce2329 100644 --- a/src/verify.cts +++ b/src/verify.cts @@ -1426,27 +1426,72 @@ function cmdVerifyArtifacts(cwd: string, planFilePath: string, raw: boolean): vo const exists = fs.existsSync(artFullPath); const check: Record = { 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); diff --git a/tests/verify.test.cjs b/tests/verify.test.cjs index 2847971bc..091f24f25 100644 --- a/tests/verify.test.cjs +++ b/tests/verify.test.cjs @@ -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,