From d626dbc6e349f7cef9fdc9c061916265ca21b7f3 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 28 Jul 2026 21:11:48 -0400 Subject: [PATCH] fix(#1883): distinguish a permission/IO error from genuine emptiness in dir scans (#2802) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#1883): failing-first regression for findContextMdIn / listMilestoneArchiveDirs swallowing EACCES Adds failing-first regression tests proving an unreadable dir is currently swallowed as empty/null instead of surfacing the permission error. Covers EACCES, EIO, the unchanged ENOENT empty path, the array fast-path, and both CONTEXT.md forms. listMilestoneArchiveDirs is exercised in-process via a new _listMilestoneArchiveDirs test seam (the validate command runs in a subprocess, so an fs monkeypatch in the test process cannot reach it). * fix(#1883): distinguish a permission/IO error from genuine emptiness in dir scans findContextMdIn (src/planning-workspace.cts) and listMilestoneArchiveDirs (src/verify.cts) catch-alled every readdirSync error into the empty marker (null / []), conflating a genuine ENOENT ('nothing there') with an EACCES/EIO failure ('can't read this'). An unreadable phase dir was silently reported as 'no CONTEXT.md' (discuss/plan gates wrongly skipped context) and an unreadable milestones/ dir as 'no archives' (active-milestone resolution / archived-phase filtering misbehaved). Narrow each catch to ENOENT only — keep the long-standing null/[] contract for genuine absence (Hyrum: empty path unchanged) and re-throw every other error so it propagates to the caller's existing try/catch. All six findContextMdIn callers either pass a pre-read string[] (no readdir) or sit inside a try block that already handles readdir failures; the two listMilestoneArchiveDirs callers live in the validate command path where errors reach the command error handler. Exposes a _listMilestoneArchiveDirs test seam so the permission-error path can be unit-tested in-process (the validate command runs in a subprocess, so an fs monkeypatch in the test process cannot reach the private helper). * fix(tests): delete stale emitted-drift ack for gsd-phase-researcher.md Pre-existing base-branch defect, not part of #1883: commit 6932fb16d (enhance(#1699): require read-and-cite provenance, #2768) landed the gsd-phase-researcher.md growth onto next WITHOUT removing its now-obsolete emitted-drift-ack.json entry. An ack explains a ripple in a PR diff; once the change merges to next the ripple becomes part of the baseline, so the ack is permanently stale for every subsequent PR — the emitted-attribution gate fails on every branch off next with '1 stale acknowledgment(s) — the ripple they explained is gone'. The gate's own error message prescribes the fix: delete the stale entry, and since gsd-phase-researcher.md was the only entry, delete the file (an empty ack file signals nothing; its presence is the alarm). This unblocks the red base for all in-flight PRs, not just this one. Folded inline per the bug-fixer directive (one-line test-prescribed fix, does not bury the #1883 fix). * docs(changeset): #1883 permission-error-not-empty * test(#1883): use t.mock.method + t.after per CONTRIBUTING test conventions Address code-review finding: replace inline try/finally in test bodies and manual fs.readdirSync reassignment with t.mock.method (auto-restored) and t.after for tmp-dir cleanup, matching CONTRIBUTING.md test patterns and the existing t.mock.method idiom in tests/phase.test.cjs. * docs(changeset): backfill #1883 PR number to 2802 --- .changeset/1883-permission-error-not-empty.md | 5 ++ src/planning-workspace.cts | 10 ++- src/verify.cts | 16 ++++- tests/emitted-drift-ack.json | 8 --- tests/planning-workspace.test.cjs | 72 +++++++++++++++++++ tests/verify.test.cjs | 60 ++++++++++++++++ 6 files changed, 159 insertions(+), 12 deletions(-) create mode 100644 .changeset/1883-permission-error-not-empty.md delete mode 100644 tests/emitted-drift-ack.json diff --git a/.changeset/1883-permission-error-not-empty.md b/.changeset/1883-permission-error-not-empty.md new file mode 100644 index 000000000..962475f30 --- /dev/null +++ b/.changeset/1883-permission-error-not-empty.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2802 +--- +**Permission errors on phase and milestone directories now surface instead of looking empty** — an unreadable phase directory used to be silently reported as "no CONTEXT.md" (so the discuss/plan gates wrongly skipped context) and an unreadable `milestones/` directory as "no archives" (so active-milestone resolution and archived-phase filtering misbehaved), because both scans treated a permission or I-O failure the same as a genuinely empty directory. (#1883) diff --git a/src/planning-workspace.cts b/src/planning-workspace.cts index f94a95f63..b64ab47fd 100644 --- a/src/planning-workspace.cts +++ b/src/planning-workspace.cts @@ -393,8 +393,14 @@ function findContextMdIn(absDirOrFiles: string | string[]): string | null { : fs.readdirSync(absDirOrFiles); if (files.includes('CONTEXT.md')) return 'CONTEXT.md'; return files.find((f: string) => f.endsWith('-CONTEXT.md')) ?? null; - } catch { - return null; + } catch (err) { + // #1883: distinguish genuine absence from a permission/I-O failure. ENOENT + // ("nothing there") keeps the long-standing null contract the callers rely + // on; every other error (EACCES, EIO, …) is a real read failure that must + // propagate — otherwise an unreadable phase dir is silently reported as + // "no CONTEXT.md" and the discuss/plan gates wrongly skip context. + if ((err as NodeJS.ErrnoException).code === 'ENOENT') return null; + throw err; } } diff --git a/src/verify.cts b/src/verify.cts index 66d6b402c..bc7373564 100644 --- a/src/verify.cts +++ b/src/verify.cts @@ -1250,8 +1250,15 @@ function listMilestoneArchiveDirs(planBase: string): string[] { .sort((a, b) => path.basename(a).localeCompare(path.basename(b), undefined, { numeric: true }), ); - } catch { - return []; + } catch (err) { + // #1883: distinguish genuine absence from a permission/I-O failure. ENOENT + // (no milestones/ dir yet) keeps the long-standing [] contract that + // collectPhaseRoots / forEachArchivedPhaseToken depend on for "no archives"; + // every other error (EACCES, EIO, …) must propagate — otherwise an unreadable + // milestones/ dir is silently reported as "no archives" and active-milestone + // resolution / archived-phase filtering misbehaves. + if ((err as NodeJS.ErrnoException).code === 'ENOENT') return []; + throw err; } } @@ -2632,4 +2639,9 @@ export = { cmdValidateAgents, cmdVerifySchemaDrift, cmdVerifyCodebaseDrift, + // Test seam (#1883): listMilestoneArchiveDirs is private and exercised through + // the validate command, which runs in a subprocess — an fs monkeypatch in the + // test process cannot reach it. Exposed under a leading underscore so the + // permission-error path can be unit-tested directly (no chmod 0o000). + _listMilestoneArchiveDirs: listMilestoneArchiveDirs, }; diff --git a/tests/emitted-drift-ack.json b/tests/emitted-drift-ack.json deleted file mode 100644 index d25bb9080..000000000 --- a/tests/emitted-drift-ack.json +++ /dev/null @@ -1,8 +0,0 @@ -{ - "version": 1, - "paths": { - "gsd-phase-researcher.md": { - "reason": "#1699 adds the in-repo value provenance rule to the claim-provenance section: an enum, schema/type-union, error code, status constant, or filesystem path earns [VERIFIED] only after a same-session Read, a path-and-line-range citation, and a verbatim quote in RESEARCH.md. Growth is inline prose in the agent body, deliberately NOT relocated into an eagerly @-imported reference, which ADR-1610 Decision 4 names as gaming the size proxy. 40866 -> 42020 bytes, LARGE tier, cap 49152." - } - } -} diff --git a/tests/planning-workspace.test.cjs b/tests/planning-workspace.test.cjs index 64dd644cd..e42c0e781 100644 --- a/tests/planning-workspace.test.cjs +++ b/tests/planning-workspace.test.cjs @@ -582,3 +582,75 @@ describe('bug #3739 — gap-analysis padded-prefix CONTEXT.md', () => { }); }); } + +// ─── bug #1883: findContextMdIn must not swallow permission/I-O errors ─────── +// A catch-all `catch { return null }` conflated a genuine ENOENT ("nothing there") +// with an EACCES/EIO failure ("can't read this"), so an unreadable phase dir was +// silently reported as "no CONTEXT.md" — discuss/plan gates then wrongly believed +// context had never been gathered. The narrowed catch re-throws every non-ENOENT +// error and keeps returning null only for genuine absence. +describe('bug #1883 — findContextMdIn distinguishes a permission error from emptiness', () => { + const { findContextMdIn } = require('../gsd-core/bin/lib/planning-workspace.cjs'); + + // Helper: build a Node-style error with a `code`, matching what fs.readdirSync throws. + function fsError(code) { + const err = new Error(`${code}: operation failed, scandir '/denied'`); + err.code = code; + err.syscall = 'scandir'; + err.path = '/denied'; + return err; + } + + test('findContextMdIn re-throws a permission (EACCES) error instead of swallowing it as null', (t) => { + // No chmod 0o000 — root bypasses mode bits (silent zero coverage in root CI). + // t.mock auto-restores after the test. + t.mock.method(fs, 'readdirSync', () => { throw fsError('EACCES'); }); + assert.throws( + () => findContextMdIn('/denied/phase-dir'), + (err) => err.code === 'EACCES', + 'an unreadable dir must propagate EACCES, not return null as if empty', + ); + }); + + test('findContextMdIn re-throws any non-ENOENT error (EIO)', (t) => { + t.mock.method(fs, 'readdirSync', () => { throw fsError('EIO'); }); + assert.throws( + () => findContextMdIn('/io-failure/phase-dir'), + (err) => err.code === 'EIO', + 'every non-ENOENT error must propagate — the narrowed catch only keeps ENOENT', + ); + }); + + test('findContextMdIn returns null for an absent dir (ENOENT) — empty path unchanged', () => { + // A path that genuinely does not exist yields ENOENT from the real OS call. + const absent = path.join(os.tmpdir(), 'gsd-1883-does-not-exist-' + process.pid); + assert.strictEqual(findContextMdIn(absent), null, + 'an absent dir (ENOENT) must still return null — Hyrum: empty path unchanged'); + }); + + test('findContextMdIn finds bare CONTEXT.md', (t) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1883-bare-')); + t.after(() => { cleanup(tmp); }); + fs.writeFileSync(path.join(tmp, 'CONTEXT.md'), '# bare\n'); + assert.strictEqual(findContextMdIn(tmp), 'CONTEXT.md'); + }); + + test('findContextMdIn finds padded NN-CONTEXT.md', (t) => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-1883-padded-')); + t.after(() => { cleanup(tmp); }); + fs.writeFileSync(path.join(tmp, '01-CONTEXT.md'), '# padded\n'); + assert.strictEqual(findContextMdIn(tmp), '01-CONTEXT.md'); + }); + + test('findContextMdIn matches against a pre-read files array without touching fs', (t) => { + // The array path never calls readdirSync — exercised by getPhaseFileStats, + // countPhasePlansAndSummaries, runGapAnalysis. Must keep working untouched. + t.mock.method(fs, 'readdirSync', () => { throw new Error('array path must not call fs'); }); + assert.strictEqual(findContextMdIn(['CONTEXT.md', 'PLAN.md']), 'CONTEXT.md', + 'array path matches bare form'); + assert.strictEqual(findContextMdIn(['02-CONTEXT.md']), '02-CONTEXT.md', + 'array path matches padded form'); + assert.strictEqual(findContextMdIn(['PLAN.md']), null, + 'array path returns null when no match'); + }); +}); diff --git a/tests/verify.test.cjs b/tests/verify.test.cjs index af76941fe..c8efb929a 100644 --- a/tests/verify.test.cjs +++ b/tests/verify.test.cjs @@ -2888,3 +2888,63 @@ describe('verifySummaryCore — reusable structured contract (#2572)', () => { ); }); }); + +// ─── bug #1883: listMilestoneArchiveDirs must not swallow permission/I-O errors ── +// The private helper catch-alled every readdirSync error into [], so an unreadable +// milestones/ dir was silently reported as "no archives" (active-milestone +// resolution / archived-phase filtering misbehaved). The narrowed catch re-throws +// every non-ENOENT error and keeps [] only for genuine absence. Tested in-process +// via the _listMilestoneArchiveDirs test seam (the validate command runs in a +// subprocess, so an fs monkeypatch in the test process cannot reach it). +describe('bug #1883 — listMilestoneArchiveDirs distinguishes a permission error from emptiness', () => { + const verifyLib = require('../gsd-core/bin/lib/verify.cjs'); + const listMilestoneArchiveDirs = verifyLib._listMilestoneArchiveDirs; + const os = require('os'); + + function fsError(code, targetPath) { + const err = new Error(`${code}: operation failed, scandir '${targetPath}'`); + err.code = code; + err.syscall = 'scandir'; + err.path = targetPath; + return err; + } + + // Inject a readdirSync fault scoped to the milestones/ path under test. + // t.mock auto-restores after each test — no chmod 0o000 (root bypasses mode bits). + function injectMilestonesFault(t, code, targetPath) { + const originalReaddirSync = fs.readdirSync; + t.mock.method(fs, 'readdirSync', function (p, ...rest) { + if (typeof p === 'string' && p.endsWith(path.join('milestones'))) { + throw fsError(code, targetPath); + } + return originalReaddirSync.call(this, p, ...rest); + }); + } + + test('listMilestoneArchiveDirs re-throws a permission (EACCES) error instead of returning []', (t) => { + const planBase = path.join(os.tmpdir(), 'gsd-1883-eacces-' + process.pid); + injectMilestonesFault(t, 'EACCES', path.join(planBase, 'milestones')); + assert.throws( + () => listMilestoneArchiveDirs(planBase), + (err) => err.code === 'EACCES', + 'an unreadable milestones/ dir must propagate EACCES, not return [] as if empty', + ); + }); + + test('listMilestoneArchiveDirs re-throws any non-ENOENT error (EIO)', (t) => { + const planBase = path.join(os.tmpdir(), 'gsd-1883-eio-' + process.pid); + injectMilestonesFault(t, 'EIO', path.join(planBase, 'milestones')); + assert.throws( + () => listMilestoneArchiveDirs(planBase), + (err) => err.code === 'EIO', + 'every non-ENOENT error must propagate', + ); + }); + + test('listMilestoneArchiveDirs returns [] for an absent milestones/ dir (ENOENT) — empty path unchanged', () => { + const planBase = path.join(os.tmpdir(), 'gsd-1883-absent-' + process.pid); + // No milestones/ dir created → real OS readdirSync throws ENOENT. + assert.deepStrictEqual(listMilestoneArchiveDirs(planBase), [], + 'an absent milestones/ dir (ENOENT) must still return [] — Hyrum: empty path unchanged'); + }); +});