fix(#1883): distinguish a permission/IO error from genuine emptiness in dir scans (#2802)

* 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
This commit is contained in:
Tom Boucher
2026-07-28 21:11:48 -04:00
committed by GitHub
parent 5296ff152d
commit d626dbc6e3
6 changed files with 159 additions and 12 deletions

View File

@@ -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;
}
}

View File

@@ -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,
};