From b47e716a037787f15cd736fa14843fe4fcdea020 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto Date: Mon, 12 Oct 2026 09:45:14 +0200 Subject: [PATCH 1/4] feat(lint): enforce ARCH-025 loop shapes with no-loop-test-cases `test-cases/no-loop-test-cases` is an oxlint JS plugin enabled as an error for `tests/**/*.test.ts`. It reports a `test()`/`it()`/`describe()` registered inside a loop or iteration callback, an `expect()` looped over rows that exist at registration time (a non-empty array literal or a `const` array visible from the test, `as const`/`satisfies` unwrapped), and a loop that carries a test's only `expect()` calls. A runtime-derived loop guarded by an assertion on the collection is not reported. Six existing loops become `test.each` tables, 1:1. ARCH-025 describes the rule as its automated enforcement and keeps boolean collapse as the remaining unenforced pattern. Signed-off-by: Rhuan Barreto --- ...c-bun-test-parametrization-and-matchers.md | 25 +- .oxlintrc.json | 4 +- lint/no-loop-test-cases.ts | 372 ++++++++++++++++++ tests/engine/check-case.test.ts | 7 +- tests/formats/adr-fuzz.test.ts | 151 ++++--- tests/lint/no-loop-test-cases.test.ts | 339 ++++++++++++++++ 6 files changed, 799 insertions(+), 99 deletions(-) create mode 100644 lint/no-loop-test-cases.ts create mode 100644 tests/lint/no-loop-test-cases.test.ts diff --git a/.archgate/adrs/ARCH-025-idiomatic-bun-test-parametrization-and-matchers.md b/.archgate/adrs/ARCH-025-idiomatic-bun-test-parametrization-and-matchers.md index 2d001e9f..dd2abffd 100644 --- a/.archgate/adrs/ARCH-025-idiomatic-bun-test-parametrization-and-matchers.md +++ b/.archgate/adrs/ARCH-025-idiomatic-bun-test-parametrization-and-matchers.md @@ -19,7 +19,7 @@ Both patterns compile, pass `oxlint`, and pass `bun test` — they are not caugh **Alternatives considered:** - **Add these as more Do's/Don'ts to ARCH-005** — Rejected: ARCH-005's own Compliance section already documents that its Do's and Don'ts section exceeds the `archgate review-context` briefing budget; appending more would push more of it out of every future briefing, including the parts already there. -- **Enforce via a new oxlint plugin immediately** — Deferred, not rejected: this repo already has a precedent (`lint/expect-expect.ts`, `lint/no-bare-env-restore.ts`) for exactly this kind of AST-detectable test-shape rule, and it is the right enforcement layer for both patterns (syntax-detectable, not behavioral). Building it is out of scope for the session that produced this ADR, which only fixed existing instances; the rule is recorded as future work in Compliance and Enforcement below. +- **Enforce via an oxlint plugin** — Adopted for the loop pattern: this repo's precedent (`lint/expect-expect.ts`, `lint/no-bare-env-restore.ts`) is exactly this kind of AST-detectable test-shape rule, and `lint/no-loop-test-cases.ts` follows it. The boolean-collapse pattern is syntax-detectable too and is a candidate for the same layer (Compliance and Enforcement below). - **Leave undocumented, rely on code review alone** — Rejected: the instances this ADR responds to were already written by reviewed, merged PRs; undocumented convention did not prevent the drift. ## Decision @@ -41,20 +41,20 @@ A loop that only builds shared setup data or fixtures for a single overall asser ### Do -- **DO** use `test.each(cases)("", (...args) => {...})` for the same check against independent inputs — each case becomes its own named, independently-reportable test. -- **DO** use `describe.each(cases)(...)` when a case needs its own group of related tests, not just one assertion. +- **DO** use `test.each(cases)("<title with %s/%p/%d>", (...args) => {...})` for the same check against independent inputs — each case becomes its own named test. +- **DO** use `describe.each(cases)(...)` when a case needs a group of related tests, not one assertion. - **DO** pass array rows (`[a, b, expected]`) for positional destructuring, object rows (`{a, b, expected}`) for named fields (title with `$field`). - **DO** assert derived comparisons directly on the values: `expect(actual).toBe(expected)` / `.toEqual(expected)`. -- **DO** match the matcher to the check: `.toContain()`/`.toMatch()` for substrings, `.toBeInstanceOf()` for type checks (including `Array.isArray` replacements), `.toHaveLength()` for counts, `.find(...)` + `.toBeDefined()`/`.toBeUndefined()` for "does any item satisfy X." -- **DO** keep loop→`test.each()` conversions 1:1 — every assertion that ran per-iteration in the original loop must still run per-case; never drop or merge assertions. +- **DO** match the matcher to the check: `.toContain()`/`.toMatch()` for substrings, `.toBeInstanceOf()` for type checks, `.toHaveLength()` for counts, `.find(...)` + `.toBeDefined()`/`.toBeUndefined()` for "does any item satisfy X." +- **DO** keep loop→`test.each()` conversions 1:1 — every per-iteration assertion still runs per case; never drop or merge assertions. ### Don't -- **DON'T** register a `test()`/`it()` or call `expect()` once per iteration inside a `for`/`.forEach` loop — use `test.each()`/`describe.each()` instead. +- **DON'T** register `test()`/`it()`/`describe()` inside a loop, loop `expect()` over rows that exist at registration time (an array literal or `const` array), or let a loop carry a test's only `expect()` — use `test.each()`, or assert the collection outside the loop. - **DON'T** write `expect(a === b).toBe(true)` or `.toBe(false)` — assert `expect(a).toBe(b)` / `.not.toBe(b)` directly. -- **DON'T** collapse `.some(predicate)`/`.every(predicate)` into `.toBe(true)` — it reports `false !== true` with no indication of which element (or none) satisfied the predicate. Use `.find(predicate)` + `.toBeDefined()`, a per-item `expect()` loop, or a matcher over the mapped values (e.g. `expect(arr.map(x => x.message)).toContain(...)`). +- **DON'T** collapse `.some(predicate)`/`.every(predicate)` into `.toBe(true)` — it reports `false !== true` without saying which element satisfied the predicate. Use `.find(predicate)` + `.toBeDefined()`, a per-item `expect()` loop guarded by an assertion on the whole collection, or a matcher over the mapped values (e.g. `expect(arr.map(x => x.message)).toContain(...)`). - **DON'T** write `expect(Array.isArray(x)).toBe(true)` — use `expect(x).toBeInstanceOf(Array)`. -- **DON'T** precompute a boolean just to assert it (`const equal = JSON.stringify(a) === JSON.stringify(b); expect(equal).toBe(true);`) — assert on the values directly with `.toEqual()`/`.toBe()` so a failure shows the actual diff. +- **DON'T** precompute a boolean just to assert it (`const equal = a === b; expect(equal).toBe(true);`) — assert on the values directly with `.toEqual()`/`.toBe()` so a failure shows the actual diff. ## Implementation Pattern @@ -111,8 +111,8 @@ expect(violations.some((v) => v.message.includes('"fetch" global'))).toBe(true); ### Risks -- **No automated enforcement yet**: neither pattern is currently caught by `oxlint` or `archgate check`, so new instances can still be introduced and merged undetected, the same way the instances motivating this ADR were. - - **Mitigation:** tracked as future work in Compliance and Enforcement below (a candidate `oxlint` plugin following the existing `bun-test/expect-expect` precedent); until it exists, code review is the only enforcement layer, so reviewers MUST check new/changed test files against the Do's and Don'ts above. +- **Boolean collapse has no automated enforcement**: `expect(<comparison>).toBe(true)` is caught by neither `oxlint` nor `archgate check`, so new instances can be introduced and merged undetected. + - **Mitigation:** a `test-cases/no-boolean-collapse` rule beside `no-loop-test-cases` is the intended layer (Compliance and Enforcement below); until it exists, reviewers MUST check new/changed test files against Manual Enforcement item 2. - **Existing violations elsewhere in the codebase may resurface**: this ADR's Context reflects a repository-wide audit at a point in time; files not covered by that audit, or added afterward, may still contain either anti-pattern. - **Mitigation:** treat any test file touched for an unrelated change as an opportunity to fix nearby instances of either pattern, consistent with ARCH-005's existing test-hygiene expectations. @@ -120,13 +120,14 @@ expect(violations.some((v) => v.message.includes('"fetch" global'))).toBe(true); ### Automated Enforcement -- **Not yet implemented.** No `oxlint` rule or `archgate` rule currently detects either anti-pattern. A future `oxlint` plugin (e.g. `bun-test/no-loop-test-cases` and `bun-test/no-boolean-collapse`, registered via `jsPlugins` in `.oxlintrc.json` alongside the existing `bun-test/expect-expect` and `test-isolation/no-bare-env-restore` plugins) is the intended enforcement layer once written, matching this repo's existing pattern of using oxlint for syntax-detectable test-shape rules and reserving `archgate` rules for structural/governance checks. This ADR's `rules: false` reflects that no companion `.rules.ts` exists. +- **`test-cases/no-loop-test-cases`** (`lint/no-loop-test-cases.ts`, registered in `.oxlintrc.json` `jsPlugins` and enabled as an error for `tests/**/*.test.ts`) reports three shapes: a `test()`/`it()`/`describe()` registered inside a `for`/`for...in`/`for...of`/`while`/`do...while` body or an iteration callback (`.forEach`, `.map`, `.flatMap`, `.filter`, `.some`, `.every`); an `expect()` inside a `for...of` or iteration callback over rows that exist at registration time (a non-empty array literal, or a `const NAME = [...]` binding in module, `describe`, or test scope, including `as const`/`satisfies` forms); and a loop that carries a test's only `expect()` calls. A runtime-derived loop of per-item `expect()`s with an assertion on the collection outside the loop is not reported. Plugin names are unique within `jsPlugins`, so this plugin is `test-cases`. +- **Boolean collapse is unenforced.** No rule detects `expect(<comparison>).toBe(true)`; a `test-cases/no-boolean-collapse` rule in the same plugin is the intended layer. `rules: false` reflects that no companion `.rules.ts` exists: both patterns are syntax-detectable, which this repo enforces with oxlint plugins, reserving `archgate` rules for structural checks. ### Manual Enforcement Code reviewers MUST verify, for any new or changed file under `tests/**/*.test.ts`: -1. No `for`/`.forEach` loop registers a `test()`/`it()`/`describe()` call, and no `for`/`.forEach` loop inside a single test calls `expect()` once per logically independent case — either shape MUST be `test.each()`/`describe.each()` instead. +1. Beyond what `test-cases/no-loop-test-cases` reports, no loop inside a test calls `expect()` once per logically independent case over rows that exist at registration time but are not an array literal or `const` array (for example rows returned by a helper call) — that shape MUST be `test.each()`/`describe.each()` too. 2. No `expect(<comparison>).toBe(true)` or `.toBe(false)` where `<comparison>` is itself a boolean expression (`===`, `.some()`, `.every()`, `Array.isArray()`, `.includes()`) — the assertion MUST target the underlying values with a specific matcher. 3. A `test.each()`/`describe.each()` conversion preserves every assertion that ran in the original loop — none dropped, none merged into a single case. diff --git a/.oxlintrc.json b/.oxlintrc.json index 1f777106..5548103f 100644 --- a/.oxlintrc.json +++ b/.oxlintrc.json @@ -7,6 +7,7 @@ "./lint/no-bare-fetch.ts", "./lint/no-shared-module-mock.ts", "./lint/no-lowered-test-timeout.ts", + "./lint/no-loop-test-cases.ts", "./.archgate/lint/oxlint.ts" ], "options": { "typeAware": true }, @@ -141,7 +142,8 @@ "bun-test/expect-expect": "error", "test-mocking/no-shared-module-mock": "error", "test-isolation/no-bare-env-restore": "error", - "test-timeout/no-lowered-test-timeout": "error" + "test-timeout/no-lowered-test-timeout": "error", + "test-cases/no-loop-test-cases": "error" } }, { diff --git a/lint/no-loop-test-cases.ts b/lint/no-loop-test-cases.ts new file mode 100644 index 00000000..47e76312 --- /dev/null +++ b/lint/no-loop-test-cases.ts @@ -0,0 +1,372 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 Archgate + +// Custom oxlint JS plugin: a loop must not stand in for `test.each()` and +// must not carry a test's only assertions (ARCH-025). Reports three shapes: +// a registration inside a loop, `expect()` over rows that exist at +// registration time, and a loop holding every `expect()` of a test. + +/** Minimal ESTree-ish node shape. The oxlint AST is ESLint-compatible. */ +type AstNode = { type: string } & Record<string, unknown>; + +function isAstNode(value: unknown): value is AstNode { + return ( + typeof value === "object" && + value !== null && + "type" in value && + typeof value.type === "string" + ); +} + +function asNode(value: unknown): AstNode | undefined { + return isAstNode(value) ? value : undefined; +} + +function asArray(value: unknown): unknown[] { + return Array.isArray(value) ? value : []; +} + +const MODIFIERS: ReadonlySet<string> = new Set([ + "skip", + "only", + "todo", + "skipIf", + "runIf", + "todoIf", + "each", + "concurrent", + "failing", + "if", +]); + +/** + * The bare identifier a registration callee is rooted at: `test`, `it` or + * `describe`, reached only through bun:test modifier members and call links + * (`test.skipIf(c)`, `test.each(rows)`). Any other member (`it.run`, + * `test.tests`) is not a registration. + */ +function registrationRoot(node: AstNode | undefined): string | undefined { + let current = node; + while (current) { + switch (current.type) { + case "Identifier": { + return typeof current.name === "string" && + (current.name === "test" || + current.name === "it" || + current.name === "describe") + ? current.name + : undefined; + } + case "MemberExpression": { + const property = asNode(current.property); + if ( + current.computed === true || + property?.type !== "Identifier" || + typeof property.name !== "string" || + !MODIFIERS.has(property.name) + ) { + return undefined; + } + current = asNode(current.object); + break; + } + case "CallExpression": { + current = asNode(current.callee); + break; + } + default: { + return undefined; + } + } + } + return undefined; +} + +const TS_WRAPPERS: ReadonlySet<string> = new Set([ + "TSAsExpression", + "TSSatisfiesExpression", + "TSNonNullExpression", + "TSTypeAssertion", +]); + +/** Strip TypeScript wrappers (`as`, `satisfies`, `!`, `<T>`) that hide a literal. */ +function unwrap(node: AstNode | undefined): AstNode | undefined { + let current = node; + while (current && TS_WRAPPERS.has(current.type)) { + current = asNode(current.expression); + } + return current; +} + +/** Child nodes of an AST node, skipping back-references and locations. */ +function childNodes(node: AstNode): AstNode[] { + const out: AstNode[] = []; + for (const key of Object.keys(node)) { + if (key === "parent" || key === "loc" || key === "range") continue; + const value = node[key]; + if (Array.isArray(value)) { + for (const item of value) { + const child = asNode(item); + if (child) out.push(child); + } + } else { + const child = asNode(value); + if (child) out.push(child); + } + } + return out; +} + +function isFunction(node: AstNode | undefined): boolean { + return ( + node?.type === "ArrowFunctionExpression" || + node?.type === "FunctionExpression" + ); +} + +function isFunctionScope(node: AstNode): boolean { + return isFunction(node) || node.type === "FunctionDeclaration"; +} + +/** A non-empty array literal; `[]` is a runtime accumulator, not a row table. */ +function isRowTable(node: AstNode | undefined): boolean { + const array = unwrap(node); + return ( + array?.type === "ArrayExpression" && asArray(array.elements).length > 0 + ); +} + +/** + * Names bound by `const NAME = [row, ...]` in `root`'s own scope. Nested + * functions are skipped, except the callbacks of `describe()` calls when + * `intoDescribe` is set, since a data table declared in a describe block is + * visible to every test below it. + */ +function collectStaticArrays( + root: AstNode, + intoDescribe: boolean +): ReadonlySet<string> { + const names = new Set<string>(); + const visit = (node: AstNode, entered: boolean): void => { + if (isFunctionScope(node) && !entered) return; + if (node.type === "VariableDeclaration" && node.kind === "const") { + for (const item of asArray(node.declarations)) { + const declarator = asNode(item); + const id = asNode(declarator?.id); + if ( + id?.type === "Identifier" && + typeof id.name === "string" && + isRowTable(asNode(declarator?.init)) + ) { + names.add(id.name); + } + } + } + const describeCall = + intoDescribe && + node.type === "CallExpression" && + registrationRoot(asNode(node.callee)) === "describe"; + for (const child of childNodes(node)) { + visit(child, describeCall && isFunction(child)); + } + }; + visit(root, true); + return names; +} + +/** True for a non-empty array literal or an identifier bound to one by `const`. */ +function isStaticRows( + node: AstNode | undefined, + staticArrays: ReadonlySet<string> +): boolean { + const target = unwrap(node); + if (!target) return false; + if (target.type === "ArrayExpression") return isRowTable(target); + return ( + target.type === "Identifier" && + typeof target.name === "string" && + staticArrays.has(target.name) + ); +} + +const ITERATION_METHODS: ReadonlySet<string> = new Set([ + "forEach", + "map", + "flatMap", + "filter", + "some", + "every", +]); + +/** The receiver of a `<receiver>.forEach/map/...(...)` call, or undefined for any other call. */ +function iterationReceiver(call: AstNode): AstNode | undefined { + const callee = asNode(call.callee); + if (callee?.type !== "MemberExpression" || callee.computed === true) { + return undefined; + } + const property = asNode(callee.property); + return property?.type === "Identifier" && + typeof property.name === "string" && + ITERATION_METHODS.has(property.name) + ? asNode(callee.object) + : undefined; +} + +/** `expect(...)`, `expect.soft(...)`, `expect.assertions(...)`: not the outer `.toBe()` link. */ +function isExpectCall(callee: AstNode | undefined): boolean { + if (callee?.type === "Identifier") return callee.name === "expect"; + if (callee?.type !== "MemberExpression") return false; + const object = asNode(callee.object); + return object?.type === "Identifier" && object.name === "expect"; +} + +interface ReportDescriptor { + node: AstNode; + message: string; +} + +interface RuleContext { + report(descriptor: ReportDescriptor): void; +} + +interface ExpectSite { + /** Enclosing loops inside the test callback, outermost first. */ + loops: AstNode[]; +} + +interface TestRecord { + expects: ExpectSite[]; + /** Static row tables visible to this test callback. */ + statics: ReadonlySet<string>; +} + +interface WalkContext { + loops: AstNode[]; + test: TestRecord | undefined; +} + +const LOOP_TYPES: ReadonlySet<string> = new Set([ + "ForStatement", + "ForInStatement", + "ForOfStatement", + "WhileStatement", + "DoWhileStatement", +]); + +const LOOP_REGISTRATION_MESSAGE = + "`test()`/`it()`/`describe()` is registered inside a loop. Use `test.each()`/`describe.each()` so each case is its own named test (ARCH-025)."; +const STATIC_ROWS_MESSAGE = + "`expect()` runs once per iteration over rows that exist at registration time. Use `test.each(rows)` so each row is its own named test (ARCH-025)."; +const ONLY_ASSERTIONS_MESSAGE = + "This loop carries the test's only `expect()` calls, so an empty collection asserts nothing. Assert the collection itself (`toEqual([...])`, `toHaveLength(n)`) outside the loop (ARCH-025)."; + +const noLoopTestCases = { + create(context: RuleContext) { + return { + Program(root: AstNode) { + const moduleStatics = collectStaticArrays(root, true); + // The inner call of `test.each(rows)(name, fn)` is the callee of the + // outer one; only the outer call is a registration. + const innerCalls = new Set<AstNode>(); + + const finishTest = (record: TestRecord): void => { + const isStaticLoop = (loop: AstNode): boolean => + (loop.type === "ForOfStatement" && + isStaticRows(asNode(loop.right), record.statics)) || + (loop.type === "CallExpression" && + isStaticRows(iterationReceiver(loop), record.statics)); + const reported = new Set<AstNode>(); + for (const site of record.expects) { + const staticLoop = site.loops.find(isStaticLoop); + if (staticLoop && !reported.has(staticLoop)) { + reported.add(staticLoop); + context.report({ + node: staticLoop, + message: STATIC_ROWS_MESSAGE, + }); + } + } + if (record.expects.some((site) => site.loops.length === 0)) return; + for (const site of record.expects) { + if (site.loops.some((loop) => reported.has(loop))) continue; + const outermost = site.loops.at(0); + if (!outermost) continue; + reported.add(outermost); + context.report({ + node: outermost, + message: ONLY_ASSERTIONS_MESSAGE, + }); + } + }; + + const visit = (node: AstNode, ctx: WalkContext): void => { + if (LOOP_TYPES.has(node.type)) { + const inner = { ...ctx, loops: [...ctx.loops, node] }; + for (const child of childNodes(node)) visit(child, inner); + return; + } + if (node.type !== "CallExpression") { + for (const child of childNodes(node)) visit(child, ctx); + return; + } + + const callee = asNode(node.callee); + if (callee?.type === "CallExpression") innerCalls.add(callee); + + if (isExpectCall(callee) && ctx.test) { + ctx.test.expects.push({ loops: ctx.loops }); + } + + const base = registrationRoot(callee); + if (!innerCalls.has(node) && base !== undefined) { + if (ctx.loops.length > 0) { + context.report({ node, message: LOOP_REGISTRATION_MESSAGE }); + } + const callback = asArray(node.arguments) + .map((arg) => asNode(arg)) + .find((arg) => isFunction(arg)); + const record: TestRecord | undefined = + base === "describe" || !callback + ? undefined + : { + expects: [], + statics: new Set([ + ...moduleStatics, + ...collectStaticArrays(callback, false), + ]), + }; + for (const child of childNodes(node)) { + visit( + child, + child === callback ? { loops: [], test: record } : ctx + ); + } + if (record) finishTest(record); + return; + } + + const callback = iterationReceiver(node) + ? asNode(asArray(node.arguments)[0]) + : undefined; + for (const child of childNodes(node)) { + visit( + child, + child === callback && isFunction(callback) + ? { ...ctx, loops: [...ctx.loops, node] } + : ctx + ); + } + }; + + visit(root, { loops: [], test: undefined }); + }, + }; + }, +}; + +const plugin = { + meta: { name: "test-cases" }, + rules: { "no-loop-test-cases": noLoopTestCases }, +}; + +export default plugin; diff --git a/tests/engine/check-case.test.ts b/tests/engine/check-case.test.ts index 6d786b88..002f54e8 100644 --- a/tests/engine/check-case.test.ts +++ b/tests/engine/check-case.test.ts @@ -89,11 +89,12 @@ describe("checkCase", () => { } ); - test("empty string matches no scheme", () => { - for (const scheme of schemeKeys(cases)) { + test.each(schemeKeys(cases))( + "empty string matches no scheme (%s)", + (scheme) => { expect(checkCase("", scheme)).toBe(false); } - }); + ); test("non-ASCII letters are rejected", () => { expect(checkCase("café-menu", "kebab-case")).toBe(false); diff --git a/tests/formats/adr-fuzz.test.ts b/tests/formats/adr-fuzz.test.ts index 2c564497..848b2dbd 100644 --- a/tests/formats/adr-fuzz.test.ts +++ b/tests/formats/adr-fuzz.test.ts @@ -69,37 +69,31 @@ describe("parseFrontmatter fuzz", () => { ); }); - test("handles null bytes and control characters", () => { - const inputs = [ - "id: \0", - "title: ", - "domain: éñüßαβγ☃\u{1F680}​ ", - "rules:  true", - "​id: GEN-001", - "id: GEN-001\r\ntitle: test\r\nrules: true\r\ndomain: general", - ]; - for (const input of inputs) { - try { - parseFrontmatter(input); - } catch (e: unknown) { - expect(e).toBeInstanceOf(Error); - } + test.each([ + "id: \0", + "title: ", + "domain: éñüßαβγ☃\u{1F680}​ ", + "rules:  true", + "​id: GEN-001", + "id: GEN-001\r\ntitle: test\r\nrules: true\r\ndomain: general", + ])("handles null bytes and control characters (%j)", (input) => { + try { + parseFrontmatter(input); + } catch (e: unknown) { + expect(e).toBeInstanceOf(Error); } }); - test("handles deeply nested YAML structures", () => { - const inputs = [ - "a: {b: {c: {d: {e: {f: {g: 1}}}}}}", - "a:\n b:\n c:\n d:\n e: 1", - "a: [[[[[[1]]]]]]", - "a: &anchor\n b: 1\nc: *anchor", - ]; - for (const input of inputs) { - try { - parseFrontmatter(input); - } catch (e: unknown) { - expect(e).toBeInstanceOf(Error); - } + test.each([ + "a: {b: {c: {d: {e: {f: {g: 1}}}}}}", + "a:\n b:\n c:\n d:\n e: 1", + "a: [[[[[[1]]]]]]", + "a: &anchor\n b: 1\nc: *anchor", + ])("handles deeply nested YAML structures (%j)", (input) => { + try { + parseFrontmatter(input); + } catch (e: unknown) { + expect(e).toBeInstanceOf(Error); } }); }); @@ -135,46 +129,40 @@ describe("parseAdr fuzz", () => { ); }); - test("handles adversarial frontmatter delimiters", () => { - const inputs = [ - "------\nid: GEN-001\n------", - "---\n---", - "---\n\n---", - "---\r\nid: GEN-001\r\n---", - "--- \nid: GEN-001\n---", - "---\nid: GEN-001\n--- trailing", - "---\nid: GEN-001\n---\n---\nid: GEN-002\n---", - "\n---\nid: GEN-001\n---", - " ---\nid: GEN-001\n---", - "---\n".repeat(50), - ]; - for (const input of inputs) { - try { - parseAdr(input, "adversarial.md"); - } catch (e: unknown) { - expect(e).toBeInstanceOf(Error); - } + test.each([ + "------\nid: GEN-001\n------", + "---\n---", + "---\n\n---", + "---\r\nid: GEN-001\r\n---", + "--- \nid: GEN-001\n---", + "---\nid: GEN-001\n--- trailing", + "---\nid: GEN-001\n---\n---\nid: GEN-002\n---", + "\n---\nid: GEN-001\n---", + " ---\nid: GEN-001\n---", + "---\n".repeat(50), + ])("handles adversarial frontmatter delimiters (%j)", (input) => { + try { + parseAdr(input, "adversarial.md"); + } catch (e: unknown) { + expect(e).toBeInstanceOf(Error); } }); - test("handles YAML injection attempts", () => { - const inputs = [ - '---\nid: "GEN-001\\nrules: true"\ntitle: test\ndomain: general\nrules: false\n---', - "---\nid: !!python/object:os.system 'echo pwned'\ntitle: test\ndomain: general\nrules: true\n---", - "---\nid: !<!tag> value\ntitle: test\ndomain: general\nrules: true\n---", - "---\nid: &id GEN-001\ntitle: *id\ndomain: general\nrules: true\n---", - "---\nid: |\n multi\n line\ntitle: test\ndomain: general\nrules: true\n---", - "---\nid: >\n folded\n text\ntitle: test\ndomain: general\nrules: true\n---", - ]; - for (const input of inputs) { - try { - const doc = parseAdr(input, "injection.md"); - expect(doc).toHaveProperty("frontmatter"); - expect(doc).toHaveProperty("body"); - expect(doc).toHaveProperty("filePath"); - } catch (e: unknown) { - expect(e).toBeInstanceOf(Error); - } + test.each([ + '---\nid: "GEN-001\\nrules: true"\ntitle: test\ndomain: general\nrules: false\n---', + "---\nid: !!python/object:os.system 'echo pwned'\ntitle: test\ndomain: general\nrules: true\n---", + "---\nid: !<!tag> value\ntitle: test\ndomain: general\nrules: true\n---", + "---\nid: &id GEN-001\ntitle: *id\ndomain: general\nrules: true\n---", + "---\nid: |\n multi\n line\ntitle: test\ndomain: general\nrules: true\n---", + "---\nid: >\n folded\n text\ntitle: test\ndomain: general\nrules: true\n---", + ])("handles YAML injection attempts (%j)", (input) => { + try { + const doc = parseAdr(input, "injection.md"); + expect(doc).toHaveProperty("frontmatter"); + expect(doc).toHaveProperty("body"); + expect(doc).toHaveProperty("filePath"); + } catch (e: unknown) { + expect(e).toBeInstanceOf(Error); } }); }); @@ -223,23 +211,20 @@ describe("AdrFrontmatterSchema fuzz", () => { ); }); - test("handles extreme field values", () => { - const cases = [ - { id: "", title: "", domain: "", rules: false }, - { - id: "a".repeat(10_000), - title: "b".repeat(10_000), - domain: "c".repeat(10_000), - rules: true, - }, - { id: "\0\0\0", title: "\n\r\t", domain: "​", rules: true }, - { id: 0, title: false, domain: [], rules: "yes" }, - { id: null, title: undefined, domain: NaN, rules: Infinity }, - { id: {}, title: [], domain: () => {}, rules: Symbol("test") }, - ]; - for (const c of cases) { - const result = AdrFrontmatterSchema.safeParse(c); - expect(result).toHaveProperty("success"); - } + test.each([ + { id: "", title: "", domain: "", rules: false }, + { + id: "a".repeat(10_000), + title: "b".repeat(10_000), + domain: "c".repeat(10_000), + rules: true, + }, + { id: "\0\0\0", title: "\n\r\t", domain: "​", rules: true }, + { id: 0, title: false, domain: [], rules: "yes" }, + { id: null, title: undefined, domain: NaN, rules: Infinity }, + { id: {}, title: [], domain: () => {}, rules: Symbol("test") }, + ])("handles extreme field values (case %#)", (c) => { + const result = AdrFrontmatterSchema.safeParse(c); + expect(result).toHaveProperty("success"); }); }); diff --git a/tests/lint/no-loop-test-cases.test.ts b/tests/lint/no-loop-test-cases.test.ts new file mode 100644 index 00000000..bcfa17d4 --- /dev/null +++ b/tests/lint/no-loop-test-cases.test.ts @@ -0,0 +1,339 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 Archgate +import { expect, test } from "bun:test"; + +import plugin from "../../lint/no-loop-test-cases"; +import { parseJsModule } from "../../src/engine/js-parser"; + +/** Minimal ESTree-ish node shape, matching the plugin's own definition. */ +type AstNode = { type: string } & Record<string, unknown>; + +interface ReportedViolation { + message: string; +} + +/** + * Run the `test-cases/no-loop-test-cases` rule against a source snippet and + * return every reported violation. Parses via the in-process `meriyah` + * entry point (`parseJsModule`, ARCH-022) the engine itself uses. + */ +function lint(source: string): ReportedViolation[] { + // meriyah's Program type structurally satisfies the plugin's own loose + // AstNode shape; the plugin doesn't export a type to convert through. + // oxlint-disable-next-line typescript/no-unsafe-type-assertion + const program = parseJsModule(source) as unknown as AstNode; + const violations: ReportedViolation[] = []; + const rule = plugin.rules["no-loop-test-cases"]; + const visitor = rule.create({ + report({ message }) { + violations.push({ message }); + }, + }); + visitor.Program(program); + return violations; +} + +const REGISTRATION = "registered inside a loop"; +const STATIC_ROWS = "rows that exist at registration time"; +const ONLY = "only `expect()` calls"; + +test.each([ + { + name: "for...of registering test()", + source: `for (const c of cases) { test("x " + c, () => { expect(c).toBe(1); }); }`, + expectedCount: 1, + keyword: REGISTRATION, + }, + { + name: ".forEach registering it()", + source: `cases.forEach((c) => { it("x", () => { expect(c).toBe(1); }); });`, + expectedCount: 1, + keyword: REGISTRATION, + }, + { + name: "for registering describe() whose body registers test (one report)", + source: `for (let i = 0; i < 3; i++) { describe("d", () => { test("x", () => { expect(i).toBe(1); }); }); }`, + expectedCount: 1, + keyword: REGISTRATION, + }, + { + name: "test.each(rows)(...) inside a for", + source: `for (const g of groups) { test.each(rows)("x %s", (r) => { expect(r).toBe(g); }); }`, + expectedCount: 1, + keyword: REGISTRATION, + }, + { + name: "test.skipIf(c)(...) inside a while", + source: `while (more()) { test.skipIf(c)("x", () => { expect(1).toBe(1); }); }`, + expectedCount: 1, + keyword: REGISTRATION, + }, + { + name: "describe.each(rows)(...) at top level", + source: `describe.each(rows)("d %s", (r) => { test("x", () => { expect(r).toBe(1); }); });`, + expectedCount: 0, + keyword: "", + }, + { + name: "array literal for...of inside a test", + source: `test("t", () => { for (const c of [1, 2]) { expect(f(c)).toBe(c); } });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: "const array identifier for...of inside a test", + source: `const CASES = [1, 2]; test("t", () => { for (const c of CASES) expect(f(c)).toBe(c); });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: ".forEach over a const array inside a test", + source: `const CASES = [1, 2]; test("t", () => { CASES.forEach((c) => expect(f(c)).toBe(c)); });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: ".forEach over an array literal inside a test", + source: `test("t", () => { [1, 2].forEach((c) => { expect(f(c)).toBe(c); }); });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: "static rows still flag with another expect outside the loop", + source: `test("t", () => { expect(g()).toBe(1); for (const c of [1, 2]) { expect(f(c)).toBe(c); } });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: "negated expect over static rows", + source: `test("t", () => { for (const c of [1, 2]) { expect(f(c)).not.toBe(0); } });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: "two expects in one static loop report once", + source: `test("t", () => { for (const c of [1, 2]) { expect(f(c)).toBe(c); expect(g(c)).toBe(c); } });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: "runtime for...of carrying the only expect", + source: `test("t", () => { for (const r of results) { expect(r.ok).toBe(true); } });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: "while loop carrying the only expect", + source: `test("t", () => { while (next()) { expect(cur()).toBe(1); } });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: "do...while loop carrying the only expect", + source: `test("t", () => { do { expect(cur()).toBe(1); } while (next()); });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: ".forEach over a non-literal identifier carrying the only expect", + source: `test("t", () => { results.forEach((r) => { expect(r.ok).toBe(true); }); });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: "nested runtime loops report the outermost once", + source: `test("t", () => { for (const a of as) { for (const b of a.items) { expect(b).toBe(1); } } });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: "let-bound array carrying the only expect is a runtime loop", + source: `let rows = [1, 2]; test("t", () => { for (const r of rows) { expect(r).toBe(1); } });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: "guarded shape: expect outside plus per-item expects", + source: `test("t", () => { expect(results).toHaveLength(3); for (const r of results) expect(r.ok).toBe(true); });`, + expectedCount: 0, + keyword: "", + }, + { + name: "loop that only builds fixtures before the assertion", + source: `test("t", () => { const out = []; for (const r of results) { out.push(r.name); } expect(out).toEqual(["a"]); });`, + expectedCount: 0, + keyword: "", + }, + { + name: "guarded Promise.all(items.map(...))", + source: `test("t", async () => { expect(items).toHaveLength(2); await Promise.all(items.map(async (i) => { expect(await f(i)).toBe(1); })); });`, + expectedCount: 0, + keyword: "", + }, + { + name: "unguarded Promise.all(items.map(...)) carries the only expects", + source: `test("t", async () => { await Promise.all(items.map(async (i) => { expect(await f(i)).toBe(1); })); });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: ".map callback carrying the only expect", + source: `test("t", () => { items.map((i) => expect(i).toBe(1)); });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: ".some callback carrying the only expect", + source: `test("t", () => { items.some((i) => expect(i).toBe(1)); });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: ".map over an array literal", + source: `test("t", () => { [1, 2].map((x) => expect(f(x)).toBe(x)); });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: "a callback outside the iteration methods is not a loop", + source: `test("t", () => { run(items, (i) => expect(i).toBe(1)); });`, + expectedCount: 0, + keyword: "", + }, + { + name: "an `it` variable that is not a registration", + source: `for (const it of items) { it.run(); }`, + expectedCount: 0, + keyword: "", + }, + { + name: "a `test` variable that is not a registration", + source: `for (const test of suite.tests) { test.run(); }`, + expectedCount: 0, + keyword: "", + }, + { + name: "test.concurrent registered in a loop", + source: `for (const c of rows) { test.concurrent(c.name, () => {}); }`, + expectedCount: 1, + keyword: REGISTRATION, + }, + { + name: "local const table in one test, runtime value of the same name in another", + source: `test("a", () => { const cases = [1, 2]; for (const c of cases) expect(c).toBe(c); }); test("b", async () => { const cases = await load(); expect(cases).toHaveLength(2); for (const c of cases) expect(c).toBe(c); });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: "empty array accumulator is a runtime collection", + source: `test("t", () => { const found = []; found.push(1); for (const f of found) expect(f).toBe(1); });`, + expectedCount: 1, + keyword: ONLY, + }, + { + name: "const table declared in a describe block", + source: `describe("d", () => { const ROWS = [1, 2]; test("t", () => { for (const r of ROWS) expect(r).toBe(r); }); });`, + expectedCount: 1, + keyword: STATIC_ROWS, + }, + { + name: "expect.soft outside the loop guards a runtime loop", + source: `test("t", () => { expect.soft(results.length).toBeGreaterThan(0); for (const r of results) expect(r.ok).toBe(true); });`, + expectedCount: 0, + keyword: "", + }, + { + name: "expect inside a loop in a beforeEach hook", + source: `beforeEach(() => { for (const r of results) { expect(r.ok).toBe(true); } });`, + expectedCount: 0, + keyword: "", + }, + { + name: "loop in a describe callback outside any test", + source: `describe("d", () => { for (const r of [1, 2]) { expect(r).toBe(1); } });`, + expectedCount: 0, + keyword: "", + }, + { + name: "test.each(rows) with a loop-free body", + source: `test.each(rows)("x %s", (r) => { expect(f(r)).toBe(1); });`, + expectedCount: 0, + keyword: "", + }, + { + name: "expect inside a nested arrow helper that is not a loop", + source: `test("t", () => { const check = (x) => { expect(x).toBe(1); }; check(1); });`, + expectedCount: 0, + keyword: "", + }, + { + name: "test with no loop at all", + source: `test("t", () => { expect(1).toBe(1); });`, + expectedCount: 0, + keyword: "", + }, +])("$name", ({ source, expectedCount, keyword }) => { + const violations = lint(source); + expect(violations).toHaveLength(expectedCount); + expect(violations.filter((v) => !v.message.includes(keyword))).toEqual([]); +}); + +test.each([ + { + shape: "registration inside a loop", + source: `for (const c of cases) { test("x", () => { expect(c).toBe(1); }); }`, + }, + { + shape: "expect over static rows", + source: `test("t", () => { for (const c of [1]) { expect(c).toBe(1); } });`, + }, + { + shape: "loop carrying the only expects", + source: `test("t", () => { for (const r of results) { expect(r).toBe(1); } });`, + }, +])("the $shape message names ARCH-025", ({ source }) => { + const violations = lint(source); + expect(violations).toHaveLength(1); + expect(violations[0]?.message).toContain("(ARCH-025)"); +}); + +function findDeclarator(node: AstNode): AstNode | undefined { + if (node.type === "VariableDeclarator") return node; + for (const value of Object.values(node)) { + for (const item of Array.isArray(value) ? value : [value]) { + if (typeof item === "object" && item !== null && "type" in item) { + // oxlint-disable-next-line typescript/no-unsafe-type-assertion + const found = findDeclarator(item as AstNode); + if (found) return found; + } + } + } + return undefined; +} + +// oxlint's TypeScript AST wraps `[...] as const` and similar in a +// TSAsExpression; meriyah cannot parse those, so the wrapper is added to the +// parsed tree by hand. +test("static rows behind a TypeScript wrapper are still static", () => { + const program = parseJsModule( + `const CASES = [1, 2]; test("x", () => { for (const c of CASES) expect(c).toBe(c); });` + ); + // oxlint-disable-next-line typescript/no-unsafe-type-assertion + const ast = program as unknown as AstNode; + const declarator = findDeclarator(ast); + if (!declarator) throw new Error("fixture has no declarator"); + declarator.init = { + type: "TSAsExpression", + expression: declarator.init, + typeAnnotation: { type: "TSTypeReference" }, + }; + const violations: ReportedViolation[] = []; + const visitor = plugin.rules["no-loop-test-cases"].create({ + report({ message }) { + violations.push({ message }); + }, + }); + visitor.Program(ast); + expect(violations).toHaveLength(1); + expect(violations[0]?.message).toContain(STATIC_ROWS); +}); From 76d02695c6fb134c428af9c7ca44f69518e4074c Mon Sep 17 00:00:00 2001 From: Rhuan Barreto <rhuan@barreto.work> Date: Mon, 12 Oct 2026 09:45:15 +0200 Subject: [PATCH 2/4] chore(memory): note worktree base branch and regression fire-test practice Signed-off-by: Rhuan Barreto <rhuan@barreto.work> --- .claude/agent-memory/archgate-developer/MEMORY.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.claude/agent-memory/archgate-developer/MEMORY.md b/.claude/agent-memory/archgate-developer/MEMORY.md index b9f64e49..ec8b7616 100644 --- a/.claude/agent-memory/archgate-developer/MEMORY.md +++ b/.claude/agent-memory/archgate-developer/MEMORY.md @@ -22,9 +22,10 @@ Exceptions: minor follow-up tweaks after validation already passed, and non-code - **This repo sets `strict: true`** in `.archgate/config.json`, so the `[suppression]`, `[briefing]`, and `[adr]` advisories block `check`, `review-context`, and `adr sync` alike, with no flag passed (ARCH-026). The practical bite: an over-budget ADR section fails the build rather than warning it — see [[adr-briefing-budget]]. - **Commit with `--signoff`** — the DCO check rejects commits without `Signed-off-by`. - **This repo is PUBLIC** — no private sibling-repo internals, no Claude session links in PRs or commits. +- **A hook-created worktree is cut from whatever branch the primary checkout has out, not from `origin/main`.** Run `git log --oneline origin/main..HEAD` before the first commit; when foreign commits show, commit the work, then `git rebase --onto origin/main HEAD~1` so the PR carries only its own commit (`git diff --name-only origin/main HEAD -- <your files>` first confirms the foreign commits touch none of them). - **Commit before fire-testing a rule or guard.** The loop (mutate → confirm the check fails → restore) restores with `git checkout <file>`, which discards all uncommitted work in that file, including the fix under test. - **Fire-test with `ARCHGATE_TELEMETRY=0` whenever the guard under test throws.** A local `bun run cli` run reports `environment: production` to Sentry, so a guard reaching the exit-2 boundary files a real issue against the product from a deliberately broken tree. A guard reporting an ADR violation is safe; one that throws is not. -- **Fire-test a guard in BOTH directions** — that it blocks the bad case AND still permits the legitimate one. A green suite proves only that the gate closes, not that it isn't over-rejecting. Fire-testing also exposes branches that can never fire: when an earlier pipeline stage aborts on the same condition, the later branch is dead governance (`check` regenerates `rules.d.ts` before any rule runs, so a throw from `generateRulesDts()` preempts any rule testing the same thing). Delete the unreachable branch and name the real enforcement point. +- **Fire-test a guard in BOTH directions** — that it blocks the bad case AND still permits the legitimate one. A regression test for a timing or scheduling bug gets the same treatment: run it against the pre-fix module (`git show origin/main:<path>` copied into place, restored from a saved copy) and make the victim do what the bug needs — a rule that never yields is never charged by a shared clock, so a no-op victim passes on the buggy code. A green suite proves only that the gate closes, not that it isn't over-rejecting. Fire-testing also exposes branches that can never fire: when an earlier pipeline stage aborts on the same condition, the later branch is dead governance (`check` regenerates `rules.d.ts` before any rule runs, so a throw from `generateRulesDts()` preempts any rule testing the same thing). Delete the unreachable branch and name the real enforcement point. - **Confirm a probe or fire-test fixture holds the bytes you meant before believing the result.** A `bun -e` body inside shell single quotes is still parsed as a JS string literal, so an escape written for the file collapses one level on the way in and the injected "violation" lands as ordinary well-formed text — a false negative that looks like a defect in the rule. A quoted `<<'EOF'` heredoc through the Bash tool is not reliably literal either: a backslash-heavy regex probe run that way reported no match for a pattern that matches fine. Write any escape-sensitive probe to a real file with the Write tool instead of inlining it in a shell command, build such fixtures from `String.fromCodePoint` (oxlint's `unicorn/prefer-code-point` rejects `fromCharCode`), and print the file back (`cat -A`) first. The same applies to WHERE a mutation lands: a first-occurrence substitution of `});` in a test file hits the `beforeEach`/`afterEach` closings before any `test(...)`, so a rule that ignores hooks reports nothing and the fire-test reads as a defect in the rule. Target the line numbers of the construct under test and show `git diff -U0` before running the check. - **A Sentry issue whose `install_path` is a local worktree is an artifact of that worktree, not a user report.** Confirm provenance from `install_path`, `is_ci`, and the timestamp — then still ask whether the error was classified correctly, because reaching Sentry at all claims archgate has a bug rather than the caller. - **`--update-snapshots` is never on its own the fix for a failing snapshot.** `tests/helpers/__snapshots__/rules-shim.test.ts.snap` is the entire `rules.d.ts` a governed project receives, and its diff is the review artifact — read every hunk and confirm it follows from an intended `src/formats/rules.ts` edit before regenerating. Deleting the file routes around nothing: Bun fails a missing snapshot whenever `CI` is set, and passes it locally. From 5101631a99096ba28e84b6d694215512ef109ff3 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto <rhuan@barreto.work> Date: Mon, 12 Oct 2026 09:45:53 +0200 Subject: [PATCH 3/4] chore(memory): check every section that restates an ADR's enforcement status Signed-off-by: Rhuan Barreto <rhuan@barreto.work> --- .claude/agent-memory/archgate-developer/MEMORY.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.claude/agent-memory/archgate-developer/MEMORY.md b/.claude/agent-memory/archgate-developer/MEMORY.md index ec8b7616..245638a6 100644 --- a/.claude/agent-memory/archgate-developer/MEMORY.md +++ b/.claude/agent-memory/archgate-developer/MEMORY.md @@ -22,6 +22,7 @@ Exceptions: minor follow-up tweaks after validation already passed, and non-code - **This repo sets `strict: true`** in `.archgate/config.json`, so the `[suppression]`, `[briefing]`, and `[adr]` advisories block `check`, `review-context`, and `adr sync` alike, with no flag passed (ARCH-026). The practical bite: an over-budget ADR section fails the build rather than warning it — see [[adr-briefing-budget]]. - **Commit with `--signoff`** — the DCO check rejects commits without `Signed-off-by`. - **This repo is PUBLIC** — no private sibling-repo internals, no Claude session links in PRs or commits. +- **An ADR restates its enforcement status in up to five places** — the Context alternatives, a Risk with its mitigation, Compliance → Automated Enforcement, Compliance → Manual Enforcement, and the Don'ts. After changing one, grep the whole file for the old claim ("not yet", "future work", the old rule name) before calling the edit done; `check` validates only the section budget, not consistency. - **A hook-created worktree is cut from whatever branch the primary checkout has out, not from `origin/main`.** Run `git log --oneline origin/main..HEAD` before the first commit; when foreign commits show, commit the work, then `git rebase --onto origin/main HEAD~1` so the PR carries only its own commit (`git diff --name-only origin/main HEAD -- <your files>` first confirms the foreign commits touch none of them). - **Commit before fire-testing a rule or guard.** The loop (mutate → confirm the check fails → restore) restores with `git checkout <file>`, which discards all uncommitted work in that file, including the fix under test. - **Fire-test with `ARCHGATE_TELEMETRY=0` whenever the guard under test throws.** A local `bun run cli` run reports `environment: production` to Sentry, so a guard reaching the exit-2 boundary files a real issue against the product from a deliberately broken tree. A guard reporting an ADR violation is safe; one that throws is not. From ea53c0594912b7af165504edb1c021af1d9b1436 Mon Sep 17 00:00:00 2001 From: Rhuan Barreto <rhuan@barreto.work> Date: Mon, 12 Oct 2026 11:07:27 +0200 Subject: [PATCH 4/4] test(formats): assert fuzz outcomes outside the catch An `attempt()` helper captures either the parser result or the thrown value, and each fuzz table asserts on the matching branch, so an assertion failure is never swallowed by the catch that checks the parser error. The memory note on hook-created worktrees orders the foreign-file check before the commit it protects. Signed-off-by: Rhuan Barreto <rhuan@barreto.work> --- .../agent-memory/archgate-developer/MEMORY.md | 2 +- tests/formats/adr-fuzz.test.ts | 52 ++++++++++++------- 2 files changed, 34 insertions(+), 20 deletions(-) diff --git a/.claude/agent-memory/archgate-developer/MEMORY.md b/.claude/agent-memory/archgate-developer/MEMORY.md index 245638a6..da9af509 100644 --- a/.claude/agent-memory/archgate-developer/MEMORY.md +++ b/.claude/agent-memory/archgate-developer/MEMORY.md @@ -23,7 +23,7 @@ Exceptions: minor follow-up tweaks after validation already passed, and non-code - **Commit with `--signoff`** — the DCO check rejects commits without `Signed-off-by`. - **This repo is PUBLIC** — no private sibling-repo internals, no Claude session links in PRs or commits. - **An ADR restates its enforcement status in up to five places** — the Context alternatives, a Risk with its mitigation, Compliance → Automated Enforcement, Compliance → Manual Enforcement, and the Don'ts. After changing one, grep the whole file for the old claim ("not yet", "future work", the old rule name) before calling the edit done; `check` validates only the section budget, not consistency. -- **A hook-created worktree is cut from whatever branch the primary checkout has out, not from `origin/main`.** Run `git log --oneline origin/main..HEAD` before the first commit; when foreign commits show, commit the work, then `git rebase --onto origin/main HEAD~1` so the PR carries only its own commit (`git diff --name-only origin/main HEAD -- <your files>` first confirms the foreign commits touch none of them). +- **A hook-created worktree is cut from whatever branch the primary checkout has out, not from `origin/main`.** Run `git log --oneline origin/main..HEAD` before the first commit; when foreign commits show, confirm they touch none of your files while HEAD is still the hook-created branch (`git diff --name-only origin/main HEAD -- <your files>` must print nothing), then commit the work and `git rebase --onto origin/main HEAD~1` so the PR carries only its own commit. - **Commit before fire-testing a rule or guard.** The loop (mutate → confirm the check fails → restore) restores with `git checkout <file>`, which discards all uncommitted work in that file, including the fix under test. - **Fire-test with `ARCHGATE_TELEMETRY=0` whenever the guard under test throws.** A local `bun run cli` run reports `environment: production` to Sentry, so a guard reaching the exit-2 boundary files a real issue against the product from a deliberately broken tree. A guard reporting an ADR violation is safe; one that throws is not. - **Fire-test a guard in BOTH directions** — that it blocks the bad case AND still permits the legitimate one. A regression test for a timing or scheduling bug gets the same treatment: run it against the pre-fix module (`git show origin/main:<path>` copied into place, restored from a saved copy) and make the victim do what the bug needs — a rule that never yields is never charged by a shared clock, so a no-op victim passes on the buggy code. A green suite proves only that the gate closes, not that it isn't over-rejecting. Fire-testing also exposes branches that can never fire: when an earlier pipeline stage aborts on the same condition, the later branch is dead governance (`check` regenerates `rules.d.ts` before any rule runs, so a throw from `generateRulesDts()` preempts any rule testing the same thing). Delete the unreachable branch and name the real enforcement point. diff --git a/tests/formats/adr-fuzz.test.ts b/tests/formats/adr-fuzz.test.ts index 848b2dbd..b3274c0c 100644 --- a/tests/formats/adr-fuzz.test.ts +++ b/tests/formats/adr-fuzz.test.ts @@ -15,6 +15,17 @@ import { // --------------------------------------------------------------------------- /** Arbitrary that produces strings resembling kebab-case domain names. */ +/** Run one fuzz input through a parser, keeping the thrown value apart from a result. */ +function attempt<T>( + run: () => T +): { ok: true; value: T } | { ok: false; error: unknown } { + try { + return { ok: true, value: run() }; + } catch (error: unknown) { + return { ok: false, error }; + } +} + const domainArb = fc.oneof( fc.stringMatching(/^[a-z][a-z0-9-]{1,31}$/u), fc.constant(""), @@ -77,10 +88,11 @@ describe("parseFrontmatter fuzz", () => { "​id: GEN-001", "id: GEN-001\r\ntitle: test\r\nrules: true\r\ndomain: general", ])("handles null bytes and control characters (%j)", (input) => { - try { - parseFrontmatter(input); - } catch (e: unknown) { - expect(e).toBeInstanceOf(Error); + const outcome = attempt(() => parseFrontmatter(input)); + if (outcome.ok) { + expect(outcome.value).toBeDefined(); + } else { + expect(outcome.error).toBeInstanceOf(Error); } }); @@ -90,10 +102,11 @@ describe("parseFrontmatter fuzz", () => { "a: [[[[[[1]]]]]]", "a: &anchor\n b: 1\nc: *anchor", ])("handles deeply nested YAML structures (%j)", (input) => { - try { - parseFrontmatter(input); - } catch (e: unknown) { - expect(e).toBeInstanceOf(Error); + const outcome = attempt(() => parseFrontmatter(input)); + if (outcome.ok) { + expect(outcome.value).toBeDefined(); + } else { + expect(outcome.error).toBeInstanceOf(Error); } }); }); @@ -141,10 +154,11 @@ describe("parseAdr fuzz", () => { " ---\nid: GEN-001\n---", "---\n".repeat(50), ])("handles adversarial frontmatter delimiters (%j)", (input) => { - try { - parseAdr(input, "adversarial.md"); - } catch (e: unknown) { - expect(e).toBeInstanceOf(Error); + const outcome = attempt(() => parseAdr(input, "adversarial.md")); + if (outcome.ok) { + expect(outcome.value).toBeDefined(); + } else { + expect(outcome.error).toBeInstanceOf(Error); } }); @@ -156,13 +170,13 @@ describe("parseAdr fuzz", () => { "---\nid: |\n multi\n line\ntitle: test\ndomain: general\nrules: true\n---", "---\nid: >\n folded\n text\ntitle: test\ndomain: general\nrules: true\n---", ])("handles YAML injection attempts (%j)", (input) => { - try { - const doc = parseAdr(input, "injection.md"); - expect(doc).toHaveProperty("frontmatter"); - expect(doc).toHaveProperty("body"); - expect(doc).toHaveProperty("filePath"); - } catch (e: unknown) { - expect(e).toBeInstanceOf(Error); + const outcome = attempt(() => parseAdr(input, "injection.md")); + if (outcome.ok) { + expect(outcome.value).toHaveProperty("frontmatter"); + expect(outcome.value).toHaveProperty("body"); + expect(outcome.value).toHaveProperty("filePath"); + } else { + expect(outcome.error).toBeInstanceOf(Error); } }); });