Skip to content

Commit 4851e8d

Browse files
committed
fix(rules): distinguish an absent rule directory from an unreadable one
`findRuleEngine`'s blanket `catch` read every `stat` failure as "not under this engine". `EACCES` on a rule directory would therefore be reported as absence, and `rule delete` would tell the user a rule that exists, and is merely unreadable, was not found. That is the same class of bug this function was added to fix, moved one step: from "wrong engine assumed" to "real error misreported as absence". `isMissingDirectory` exists for exactly this line and says so in its own doc comment — `EACCES` above all is a real IO problem, and reading it as "nothing here" is what makes an unreadable bucket indistinguishable from an unwritten one. `dispatch.ts` already uses it. Now so does this. Found by the review on #214.
1 parent 1045c80 commit 4851e8d

2 files changed

Lines changed: 39 additions & 4 deletions

File tree

‎packages/cli/src/rules/engines.ts‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { readdir, stat } from "node:fs/promises";
22
import { join } from "node:path";
33

44
import { CLIError } from "../util/cli-error";
5+
import { isMissingDirectory } from "./errno";
56

67
/**
78
* Engines this CLI knows. The directory name under `.taskless/rules/` **is**
@@ -255,8 +256,14 @@ export async function findRuleEngine(
255256
try {
256257
const stats = await stat(ruleDirectory(cwd, engine, id));
257258
if (stats.isDirectory()) return engine;
258-
} catch {
259-
// Not under this engine; try the next.
259+
} catch (error) {
260+
// Only a genuinely absent directory means "not this engine". Swallowing
261+
// every failure would read `EACCES` on a rule directory as absence, and
262+
// the caller would then report a rule that is present but unreadable as
263+
// not found — telling the user to treat an existing rule as gone. That
264+
// is the same class of bug this function exists to fix, moved from
265+
// "wrong engine assumed" to "real error misreported as absence".
266+
if (!isMissingDirectory(error)) throw error;
260267
}
261268
}
262269
return undefined;

‎packages/cli/test/engine-dispatch.test.ts‎

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,16 @@
11
import { execFile } from "node:child_process";
2-
import { mkdir, mkdtemp, readdir, rm, stat, writeFile } from "node:fs/promises";
2+
import {
3+
chmod,
4+
mkdir,
5+
mkdtemp,
6+
readdir,
7+
rm,
8+
stat,
9+
writeFile,
10+
} from "node:fs/promises";
311
import { existsSync } from "node:fs";
412
import { tmpdir } from "node:os";
5-
import { join, relative, resolve } from "node:path";
13+
import { dirname, join, relative, resolve } from "node:path";
614
import { promisify } from "node:util";
715
import { afterEach, beforeEach, describe, expect, it } from "vitest";
816

@@ -259,6 +267,26 @@ describe("engine dispatch by directory", () => {
259267
}
260268
);
261269

270+
it("propagates a real IO error instead of reporting the rule absent", async () => {
271+
// `EACCES` on a rule directory is not "no rule here". Reading it as
272+
// absence would tell the user to treat a rule that exists, and is merely
273+
// unreadable, as gone — the same class of bug this search exists to fix.
274+
const directory = ruleDirectory(
275+
temporaryDirectory,
276+
"vale",
277+
"logs-abc12345"
278+
);
279+
await mkdir(directory, { recursive: true });
280+
await chmod(dirname(directory), 0o000);
281+
try {
282+
await expect(
283+
deleteRuleFiles(temporaryDirectory, "logs-abc12345")
284+
).rejects.toMatchObject({ code: "EACCES" });
285+
} finally {
286+
await chmod(dirname(directory), 0o755);
287+
}
288+
});
289+
262290
it("reports not-found for an id no engine holds", async () => {
263291
expect(await deleteRuleFiles(temporaryDirectory, "absent-abc12345")).toBe(
264292
false

0 commit comments

Comments
 (0)