Skip to content

chore(lint): move the never-throw rule to a Biome GritQL plugin #404

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Vivswan merged 1 commit into from
Sep 22, 2026
Merged

chore(lint): move the never-throw rule to a Biome GritQL plugin #404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
227 changes: 13 additions & 214 deletions .github/scripts/arch-lint.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,10 @@
* type-only imports and re-exports -> edges too
* `import("./x.js").T`, `import X = require("./x.js")` in type positions -> edges too
* a computed `import(x)` or `require(x)` -> fails: the graph cannot follow it
*
* The same walk carries the never-throw rule the `throws` block of architecture.yml states beside its lists.
*/

import { existsSync, readdirSync, readFileSync, statSync } from "node:fs";
import { dirname, join, normalize, relative, resolve } from "node:path";
import { existsSync, readdirSync, readFileSync } from "node:fs";
import { dirname, join, relative, resolve } from "node:path";
import { inspect } from "node:util";
import { err, ok, Result } from "neverthrow";
import { type Node, parseSync } from "oxc-parser";
Expand All @@ -21,35 +19,22 @@ import { countNoun } from "../../src/text.js";

export const ARCHITECTURE_PATH = "architecture.yml";

export interface Throws {
/** Files whose throw is a third party's contract (commander's argParser), each with its reason in the yaml. */
readonly contracts: readonly string[];
/** file -> its exact count of throws outside the rule. */
readonly ratchet: Readonly<Record<string, number>>;
}

export interface Architecture {
/** layer -> the src/ paths it owns (a `/` suffix means a directory). */
readonly layers: Readonly<Record<string, readonly string[]>>;
readonly exclude: readonly string[];
/** from -> the layers it may import. */
readonly edges: Readonly<Record<string, readonly string[]>>;
readonly throws: Throws;
}

const PATHS = z.array(z.string());
const COUNT = { error: "expected a whole number of throws" };
const ARCHITECTURE = z.strictObject({
layers: z.record(z.string(), PATHS),
exclude: PATHS,
edges: z.record(z.string(), PATHS),
throws: z.strictObject({
contracts: PATHS,
ratchet: z.record(z.string(), z.int(COUNT).nonnegative(COUNT)),
}),
}) satisfies z.ZodType<Architecture>;

/** `throws.ratchet["src/x.ts"]`, `throws.contracts[0]`: the yaml key as a reader would write it in code. */
/** `layers.engine[0]`, `edges["plain-data"]`: the yaml key as a reader would write it in code. */
function keyPath(path: readonly PropertyKey[]): string {
return path
.map((segment, index) =>
Expand Down Expand Up @@ -82,8 +67,6 @@ function describeIssue(doc: unknown, issue: z.core.$ZodIssue): string[] {
];
}

/** The declaration, or every way the file fails to be one: yaml, shape, count type, and a `throws` path naming no
* file under src/. A count the lint cannot compare would otherwise silence the ratchet for that file. */
export function parseArchitecture(root: string): Result<Architecture, string[]> {
const document = parseDocument(readFileSync(join(root, ARCHITECTURE_PATH), "utf8"));
if (document.errors.length > 0) {
Expand All @@ -100,35 +83,15 @@ export function parseArchitecture(root: string): Result<Architecture, string[]>
return err(doc.error);
}
const parsed = ARCHITECTURE.safeParse(doc.value);
if (!parsed.success) {
return err(located(parsed.error.issues.flatMap((issue) => describeIssue(doc.value, issue))));
}
const { contracts, ratchet } = parsed.data.throws;
const unknownFiles = [
...contracts.map((file, index) => [["contracts", index], file] as const),
...Object.keys(ratchet).map((file) => [["ratchet", file], file] as const),
]
.filter(([, file]) => !(normalize(file).startsWith("src/") && isFile(join(root, file))))
.map(
([path, file]) =>
`${keyPath(["throws", ...path])} names no file under src/: ${inspect(file)}`,
);
return unknownFiles.length > 0 ? err(located(unknownFiles)) : ok(parsed.data);
return parsed.success
? ok(parsed.data)
: err(located(parsed.error.issues.flatMap((issue) => describeIssue(doc.value, issue))));
}

function located(problems: readonly string[]): string[] {
return problems.map((problem) => `${ARCHITECTURE_PATH}: ${problem}`);
}

/** A path through a file (`src/x.ts/y.ts`) makes stat fail with ENOTDIR; that is as much "no file" as a missing one. */
function isFile(path: string): boolean {
try {
return statSync(path).isFile();
} catch {
return false;
}
}

/** For callers that render rather than lint (docs, tests): a malformed declaration is fatal to them. */
export function readArchitecture(root: string): Architecture {
return parseArchitecture(root).match(
Expand All @@ -145,31 +108,21 @@ function layerOf(arch: Architecture, path: string): string | undefined {
)?.[0];
}

/** Every node under `value`, depth first, each with the state `carry` hands down from its nearest typed ancestor. */
function* nodesOf<S>(
value: unknown,
state: S,
carry: (node: Node, state: S) => S,
): Generator<{ node: Node; state: S }> {
function* nodesOf(value: unknown): Generator<Node> {
if (Array.isArray(value)) {
for (const item of value) {
yield* nodesOf(item, state, carry);
yield* nodesOf(item);
}
} else if (typeof value === "object" && value !== null) {
let inner = state;
if ("type" in value && typeof value.type === "string") {
const node = value as Node;
yield { node, state };
inner = carry(node, state);
yield value as Node;
}
for (const child of Object.values(value)) {
yield* nodesOf(child, inner, carry);
yield* nodesOf(child);
}
}
}

const stateless = (): undefined => undefined;

/** The expression under the wrappers parentheses and TypeScript add: `(require)(x)`, `require!(x)`,
* `(require as any)(x)`, `require<T>(x)`. */
function unwrapped(expression: Node): Node {
Expand Down Expand Up @@ -240,7 +193,7 @@ export function importSpecifiers(text: string, file: string): string[] {
);
}
const specifiers = new Set<string>();
for (const { node } of nodesOf(program, undefined, stateless)) {
for (const node of nodesOf(program)) {
if (!isModuleLoad(node)) {
continue;
}
Expand Down Expand Up @@ -326,156 +279,6 @@ export function lintArchitecture(root: string, arch = readArchitecture(root)): s
return problems;
}

type ThrowStatement = Extract<Node, { type: "ThrowStatement" }>;
/** The names a binding pattern declares; a destructuring key is not one of them. */
function bindingNames(pattern: Node): string[] {
switch (pattern.type) {
case "Identifier":
return [pattern.name];
case "ObjectPattern":
return pattern.properties.flatMap((property) =>
bindingNames(property.type === "RestElement" ? property.argument : property.value),
);
case "ArrayPattern":
return pattern.elements.flatMap((element) => (element ? bindingNames(element) : []));
case "AssignmentPattern":
return bindingNames(pattern.left);
case "RestElement":
return bindingNames(pattern.argument);
default:
return [];
}
}

/** Whether a block redeclares `name` with a value binding of its own: a variable, or anything declared under an
* `id` (a function, class, enum, or namespace). A type alias or interface lives in the type namespace, so
* `throw name` after one still throws the catch binding. */
function redeclares(block: Extract<Node, { type: "BlockStatement" }>, name: string): boolean {
return block.body.some((statement) =>
statement.type === "VariableDeclaration"
? statement.declarations.some((declaration) => bindingNames(declaration.id).includes(name))
: statement.type !== "TSTypeAliasDeclaration" &&
statement.type !== "TSInterfaceDeclaration" &&
"id" in statement &&
statement.id !== null &&
typeof statement.id === "object" &&
statement.id.type === "Identifier" &&
statement.id.name === name,
);
}

/** The catch binding a throw may rethrow: the clause's own identifier, carried only through blocks and if-statements
* that do not redeclare it. A loop, a switch, a nested function, or anything else on the way drops it, so a throw
* there is judged on its own. */
function carryRethrowable(node: Node, rethrowable: string | undefined): string | undefined {
if (node.type === "CatchClause") {
return node.param?.type === "Identifier" ? node.param.name : undefined;
}
return node.type === "IfStatement" ||
(node.type === "BlockStatement" && rethrowable !== undefined && !redeclares(node, rethrowable))
? rethrowable
: undefined;
}

/** `new X("BUG: ...")` or `new X(\`BUG: ${...}\`)`, X any Error class: a programming error no user can cause. */
function isBugInvariant(argument: ThrowStatement["argument"]): boolean {
if (argument.type !== "NewExpression") {
return false;
}
const [first] = argument.arguments;
const head =
first?.type === "Literal"
? first.value
: first?.type === "TemplateLiteral"
? first.quasis[0]?.value.cooked
: undefined;
return typeof head === "string" && head.startsWith("BUG:");
}

export interface ThrowCensus {
bug: number;
rethrow: number;
contract: number;
/** Throws outside the rule, whether or not the ratchet lists them. */
outside: number;
}

const OUTSIDE_RULE =
"not a BUG: invariant, not a bare rethrow inside its catch clause, and the file is not in throws.contracts";

export function lintThrows(
root: string,
arch = readArchitecture(root),
): { problems: string[]; census: ThrowCensus } {
const census: ThrowCensus = { bug: 0, rethrow: 0, contract: 0, outside: 0 };
const spared = new Set(arch.throws.contracts);
const sparedInUse = new Set<string>();
const outside = new Map<string, number[]>();
const problems: string[] = [];
for (const file of sourceFiles(root, arch)) {
const text = readFileSync(join(root, file), "utf8");
const { program, errors } = parseSync(join(root, file), text);
if (errors.length > 0) {
problems.push(`${file} does not parse, so its throws are uncounted: ${errors[0]?.message}`);
continue;
}
for (const { node, state: rethrowable } of nodesOf(program, undefined, carryRethrowable)) {
if (node.type !== "ThrowStatement") {
continue;
}
if (isBugInvariant(node.argument)) {
census.bug += 1;
} else if (node.argument.type === "Identifier" && node.argument.name === rethrowable) {
census.rethrow += 1;
} else if (spared.has(file)) {
census.contract += 1;
sparedInUse.add(file);
} else {
census.outside += 1;
const line = text.slice(0, node.start).split("\n").length;
outside.set(file, [...(outside.get(file) ?? []), line]);
}
}
}
for (const [file, lines] of [...outside].sort()) {
const listed = arch.throws.ratchet[file];
const sites = lines.map((line) => `${file}:${line}`).join(", ");
if (listed === undefined) {
problems.push(
...lines.map(
(line) =>
`${file}:${line} throws outside the rule: ${OUTSIDE_RULE}; return a Result, or add the file to throws.ratchet`,
),
);
} else if (lines.length > listed) {
problems.push(
`${file} throws ${lines.length} times outside the rule, throws.ratchet allows ${listed}: ${sites}; return a Result instead`,
);
} else if (lines.length < listed) {
problems.push(
`${file} throws ${lines.length} times outside the rule, throws.ratchet lists ${listed}; lower it to ${lines.length}`,
);
}
}
for (const file of Object.keys(arch.throws.ratchet).sort()) {
if (!outside.has(file)) {
problems.push(
`stale ratchet ${file}: no throw outside the rule remains; remove it from throws.ratchet`,
);
}
}
for (const file of [...spared].sort()) {
if (!sparedInUse.has(file)) {
problems.push(`stale allowance throws.contracts ${file}: no throw remains there; remove it`);
}
}
return { problems, census };
}

export function describeThrowCensus({ bug, rethrow, contract, outside }: ThrowCensus): string {
return `throws: ${bug} BUG: invariants, ${rethrow} rethrows, ${contract} under a third-party contract, ${outside} outside the rule`;
}

/** A hyphen in a layer name is edge syntax to mermaid, so ids swap it for an underscore. */
export function renderArchitectureMermaid(arch: Architecture): string {
const id = (layer: string): string => layer.replace(/-/g, "_");
Expand All @@ -491,11 +294,7 @@ export function renderArchitectureMermaid(arch: Architecture): string {
if (import.meta.main) {
const root = join(import.meta.dir, "..", "..");
const problems = parseArchitecture(root).match(
(arch) => {
const throws = lintThrows(root, arch);
console.log(`lint:arch: ${describeThrowCensus(throws.census)}`);
return [...lintArchitecture(root, arch), ...throws.problems];
},
(arch) => lintArchitecture(root, arch),
(problems) => problems,
);
if (problems.length > 0) {
Expand All @@ -504,5 +303,5 @@ if (import.meta.main) {
);
process.exit(1);
}
console.log(`lint:arch: src/ imports and throws match ${ARCHITECTURE_PATH}`);
console.log(`lint:arch: src/ imports match ${ARCHITECTURE_PATH}`);
}
2 changes: 1 addition & 1 deletion AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ Code is the source of truth: this section holds only the rules and the decisions
- The import layering of `src/` is declared in `architecture.yml`; a new cross-layer import is a deliberate edit to that file.
- A type a section module exposes is exported from its home module, or the bundled declarations cannot reach it and the package-smoke job fails.
- New sections and endpoints ship with e2e scenarios, and `bun run test:e2e` runs green before they land.
- Errors are values: `throw` only for `BUG:` invariants (programming errors no user can cause), a bare rethrow in its `catch`, and where a third party's contract demands it (`architecture.yml` names the file and the reason). The arch lint enforces it; its ratchet only shrinks.
- Errors are values: `throw` only for `BUG:` invariants (programming errors no user can cause), a bare rethrow in its `catch`, and where a third party's contract demands it. The Biome plugin `lint/never-throw.grit` enforces it.
- No backward-compatibility shims: a change that breaks an input, key, format, or behavior ships the break behind a major with a loud error naming the fix; a one-shot migration only when many files must move at once. A shim that stays anyway carries a `COMPAT(vN)` marker naming the major that deletes it.

### Decisions a reader would otherwise reverse
Expand Down
11 changes: 0 additions & 11 deletions architecture.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,3 @@ edges:
problem: [plain-data, schema, text]
schema: [sections, types]
upstream-gaps: [types]

# The never-throw rule: errors are values, so a `throw` in src/ is a `BUG:` invariant (a programming error no user
# can cause), a bare rethrow sitting directly inside its catch clause, or spared here. `contracts` lists the files
# whose throw IS a third party's contract, each with its reason. `ratchet` lists every other throw by file with its
# exact count: the lint fails when this list and the tree disagree in either direction, and refuses a count it cannot
# compare. That a count only ever goes down is the review rule in AGENTS.md, not the lint's.
throws:
contracts:
# commander's argParser reports a refused value by throwing InvalidArgumentError; it has no value form.
- src/cli/inputs.ts
ratchet: {}
22 changes: 16 additions & 6 deletions biome.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,11 @@
"useIgnoreFile": true
},
"files": {
"includes": [
"**",
"!lib",
"!src/upstream-gaps/index.ts",
"!.github/repo-platform-manifest.json"
]
"includes": ["**", "!lib", "!.github/repo-platform-manifest.json"]
},
"formatter": {
"enabled": true,
"includes": ["**", "!src/upstream-gaps/index.ts"],
"indentStyle": "space",
"indentWidth": 2,
"lineWidth": 100
Expand Down Expand Up @@ -52,6 +48,20 @@
}
},
"overrides": [
{
"includes": ["src/upstream-gaps/index.ts"],
"assist": {
"actions": {
"source": {
"organizeImports": "off"
}
}
}
},
{
"includes": ["src/**", "!src/cli/inputs.ts"],
"plugins": ["./lint/never-throw.grit"]
Comment thread
Vivswan marked this conversation as resolved.
},
{
"includes": ["src/action/**"],
"linter": {
Expand Down
6 changes: 2 additions & 4 deletions docs/reference/architecture.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,9 @@ The check is existence only: a caption-only box (`mode`, `rendered-file`) names

The [module map](#the-module-map) at the end is generated from [architecture.yml](https://github.com/Vivswan/github-settings-as-code/blob/main/architecture.yml). The `lint:arch` script keeps that declaration equal to the import graph, so the map cannot show an edge the code does not draw.

The same lint enforces the never-throw rule: a function that can fail returns a neverthrow `Result` carrying a typed `Problem`, so an error is a value the caller handles. A section's `plan()`, `snapshot()`, and operation hooks carry a `SectionFailure` instead, which the engine loops match on by kind.
A Biome linter plugin, `lint/never-throw.grit`, enforces the never-throw rule: a function that can fail returns a neverthrow `Result` carrying a typed `Problem`, so an error is a value the caller handles. A section's `plan()`, `snapshot()`, and operation hooks carry a `SectionFailure` instead, which the engine loops match on by kind.

A `throw` is allowed as a `BUG:` invariant, as a bare rethrow inside its own `catch`, or where a third party's contract demands it, in a file the `throws` block of `architecture.yml` names with its reason.

That block counts the remaining throws per file. The lint fails when the count and the tree disagree in either direction, so a converted throw lowers its file's count and the entry leaves the list once no throw remains; that a count only goes down is the review rule in AGENTS.md.
A `throw` is allowed as a `BUG:` invariant, as a bare rethrow inside its own `catch`, or where a third party's contract demands it, in a file the plugin's entry in `biome.json` exempts and the plugin's header names with its reason.

## The journey of one settings file

Expand Down
Loading
Loading