From 83b3b961595b43db142dbd78e58512472164c595 Mon Sep 17 00:00:00 2001 From: Sachin Sharma Date: Tue, 18 Aug 2026 12:13:38 +0530 Subject: [PATCH] chore(lint): enforce rule 15 so tests cannot quietly reach into src MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rule 15 — tests are end-to-end only — was documented in a47c4353 but only checked by review, and review is what let the original violation through: on #1304 a review comment of mine told a contributor to keep a unit suite, and nothing contradicted it. Adds `neurolink/e2e-tests-only`, an AST rule over files under `test/`. It flags a RUNTIME import from `src/lib/` or `src/cli/` in either form: import { FileDetector } from "../src/lib/utils/fileDetector.js"; const { X } = await import("../src/lib/whatever.js"); It deliberately does not flag: - type-only imports — `import type { Tool } from …`, and `import { type A, type B } from …` where every specifier is type-only. They are erased at compile time and assert nothing. - anything from `../dist/`, which is what callers actually load. - files on the `allow` list. `allow` is the determinism exception, and it lives in eslint.config.js with a one-line reason per entry: rag (chunk boundaries and reranker ordering), bugfixes (parser edge cases and outgoing wire format), proxy (429-cooldown and quota ordering), autoresearch (a task system with no public surface), and the chroma/pinecone filter translators. Adding to it is a review decision and the file's own header must say what determinism buys. The rule's message points at the fix rather than just the violation, and warns against the trap that cost three silent failures while writing a47c4353: moving an import to `../dist/` in a file that also stubs or spies on that module makes the stub patch a different bundled copy, so the test starts doing real work while still typechecking clean. ## What it found on its first run One file, and it turns out to be a good sign rather than a bad one: `continuous-test-suite-error-classifier-contract.ts`, added in 5502259c after rule 15 landed. Its header already cites rule 15, already declares the determinism exception, already pins the all-src module graph, and justifies each category — synthetic rule tables, duck-typed error shapes no AWS SDK actually produces, module-export-shape checks. So it is allowlisted rather than converted; the convention was applied correctly without the rule existing yet. Verified by breaking it on purpose: a probe file importing a value from src/lib statically AND dynamically reports both, while `import type`, `{ type X }` and a `../dist/` import in the same file report nothing. `pnpm run lint` 0 errors, `pnpm run check` exit 0. --- CLAUDE.md | 5 +- eslint-rules/e2e-tests-only.cjs | 117 ++++++++++++++++++++++++++++++++ eslint-rules/index.cjs | 1 + eslint.config.js | 30 ++++++++ 4 files changed, 152 insertions(+), 1 deletion(-) create mode 100644 eslint-rules/e2e-tests-only.cjs diff --git a/CLAUDE.md b/CLAUDE.md index 6e87e9969..25b5b3eca 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -61,7 +61,9 @@ These are non-negotiable. Violating them breaks the build or introduces bugs. **The one exception is determinism.** A test may sit outside this rule only when it needs deterministic control that a live call cannot give — a pure translation table, a fixed set of inputs, a recorded backend. The vector-store suites are the standing example: they drive real backends (pglite in-process Postgres, recorded fixtures) and cover filter-dialect translation that no live `generate()` could be made to emit. Convenience, speed, and "it is easier to assert on the internal" are not exceptions. When you take the exception, say so in the file's header and name what determinism buys. -**Enforcement:** Rules 2 and 7-14 are enforced via ESLint. Rules 2 and 7-13 use custom rules in `eslint-rules/`; rule 14 uses core `no-restricted-syntax` AST selectors in `eslint.config.js`. Run `pnpm run lint` (or the pre-commit hook) — no shell scripts, no regex heuristics, everything AST-based. **Rule 15 is not lint-enforced** — it is a review check. +**Enforcement:** Rules 2 and 7-15 are enforced via ESLint. Rules 2, 7-13 and 15 use custom rules in `eslint-rules/`; rule 14 uses core `no-restricted-syntax` AST selectors in `eslint.config.js`. Run `pnpm run lint` (or the pre-commit hook) — no shell scripts, no regex heuristics, everything AST-based. + +Rule 15's determinism exception is the `allow` list on `neurolink/e2e-tests-only` in `eslint.config.js`. Adding a file to it is a review decision, and the file's own header must say what determinism buys — it is not a way to silence the rule. The rule ignores type-only imports (`import type`, and `{ type A }` where every specifier is type-only) because they are erased and assert nothing. | Rule | ESLint rule | | -------- | ---------------------------------------- | @@ -74,6 +76,7 @@ These are non-negotiable. Violating them breaks the build or introduces bugs. | 12 | `neurolink/no-type-export-outside-types` | | 13 | `neurolink/barrel-type-imports` | | 14 | `no-restricted-syntax` (AST selectors) | +| 15 | `neurolink/e2e-tests-only` | --- diff --git a/eslint-rules/e2e-tests-only.cjs b/eslint-rules/e2e-tests-only.cjs new file mode 100644 index 000000000..d0b2783e0 --- /dev/null +++ b/eslint-rules/e2e-tests-only.cjs @@ -0,0 +1,117 @@ +/** + * Rule 15 — tests are end-to-end only. + * + * A suite under `test/` must exercise a surface this package ships: construct + * `NeuroLink` and call `generate()` / `stream()`, or drive the built CLI. It + * must not import a module out of `src/lib/` (or `src/cli/`) to assert on it + * directly — that tests an internal shape callers never reach, and it is free + * to change under them. + * + * What this flags: a RUNTIME import from `src/lib/` or `src/cli/` inside + * `test/`. Both forms: + * + * import { FileDetector } from "../src/lib/utils/fileDetector.js"; // ✗ + * const { X } = await import("../src/lib/whatever.js"); // ✗ + * + * What it allows: + * + * - Type-only imports, which are erased and assert nothing: + * import type { Tool } from "../src/lib/types/index.js"; // ok + * import { type A, type B } from "../src/lib/types/index.js"; // ok + * - Anything from `../dist/`, which is what callers actually load. + * - Files on the `allow` list — the determinism exception. A test may sit + * outside this rule only when it needs deterministic control a live call + * cannot give (a pure translation table, a recorded backend, an exact + * chunk boundary, an outgoing wire payload). Convenience and speed are + * not exceptions. Every entry must say so in the file's own header. + * + * ⚠️ Do not "fix" a violation by moving the import to `../dist/` if the file + * also stubs or spies on that module: `dist/index.js` is a separate bundled + * copy, so a stub applied to one graph is invisible to the other and the test + * silently starts doing real work. See CLAUDE.md rule 15, "One module graph + * per suite". + */ + +"use strict"; + +const SRC_IMPORT = /(?:^|\/)\.\.\/(?:\.\.\/)*src\/(?:lib|cli)\//; + +/** `import { type A, type B } from "…"` — every specifier is type-only. */ +function allSpecifiersAreTypeOnly(node) { + const specs = node.specifiers ?? []; + if (specs.length === 0) { + return false; // bare side-effect import: runtime + } + return specs.every( + (s) => s.type === "ImportSpecifier" && s.importKind === "type", + ); +} + +function isSrcPath(value) { + return typeof value === "string" && SRC_IMPORT.test(`/${value}`); +} + +module.exports = { + meta: { + type: "problem", + docs: { + description: + "Tests must drive the shipped surface, not import src/lib directly (CLAUDE.md rule 15)", + }, + schema: [ + { + type: "object", + properties: { + allow: { type: "array", items: { type: "string" } }, + }, + additionalProperties: false, + }, + ], + messages: { + srcImport: + "Rule 15: tests are end-to-end only — '{{source}}' reaches into src/. Drive the surface via NeuroLink/generate/stream or the built CLI, or import the shipped symbol from '../dist/index.js'. If this genuinely needs deterministic control a live call cannot give, add the file to the rule's `allow` list in eslint.config.js and say why in its header.", + }, + }, + + create(context) { + const filename = context.filename ?? context.getFilename(); + const normalized = filename.split("\\").join("/"); + if (!normalized.includes("/test/")) { + return {}; + } + const allow = (context.options?.[0]?.allow ?? []).map((p) => + p.split("\\").join("/"), + ); + if (allow.some((p) => normalized.endsWith(p))) { + return {}; + } + + function report(node, source) { + context.report({ node, messageId: "srcImport", data: { source } }); + } + + return { + ImportDeclaration(node) { + if (node.importKind === "type") { + return; + } + if (!isSrcPath(node.source.value)) { + return; + } + if (allSpecifiersAreTypeOnly(node)) { + return; + } + report(node, node.source.value); + }, + ImportExpression(node) { + if (node.source?.type !== "Literal") { + return; + } + if (!isSrcPath(node.source.value)) { + return; + } + report(node, node.source.value); + }, + }; + }, +}; diff --git a/eslint-rules/index.cjs b/eslint-rules/index.cjs index d0d53b990..9b6b10c0d 100644 --- a/eslint-rules/index.cjs +++ b/eslint-rules/index.cjs @@ -34,5 +34,6 @@ module.exports = { "no-inline-secret-regex": require("./no-inline-secret-regex.cjs"), "provider-typed-errors": require("./provider-typed-errors.cjs"), "provider-base-class": require("./provider-base-class.cjs"), + "e2e-tests-only": require("./e2e-tests-only.cjs"), }, }; diff --git a/eslint.config.js b/eslint.config.js index 374c1da56..42e643723 100644 --- a/eslint.config.js +++ b/eslint.config.js @@ -251,8 +251,38 @@ export default [ }, plugins: { "@typescript-eslint": tseslint, + neurolink, }, rules: { + // Rule 15 — tests drive the shipped surface, not src/lib. The `allow` + // list is the determinism exception: a suite may sit outside the rule + // only when it needs deterministic control a live call cannot give. + // Every entry states why in its own file header. Adding to this list is + // a review decision, not a way to silence the rule. + "neurolink/e2e-tests-only": [ + "error", + { + allow: [ + // Chunk boundaries and reranker ordering are exact outcomes; + // generate({ rag }) only ever shows the model's answer. + "test/continuous-test-suite-rag.ts", + // Parser edge cases, outgoing wire format, proxy cooldown/quota. + "test/continuous-test-suite-bugfixes.ts", + // 429-cooldown planning and quota ordering across accounts. + "test/continuous-test-suite-proxy.ts", + // Background task system with no public surface at all. + "test/continuous-test-suite-autoresearch.ts", + // Filter-dialect translation no live generate() could emit. + "test/continuous-test-suite-vector-chroma.ts", + "test/continuous-test-suite-vector-pinecone.ts", + // Synthetic rule tables, duck-typed error shapes no real SDK + // produces, and module-export-shape checks. Its header already + // states the exception and the all-src module graph. + "test/continuous-test-suite-error-classifier-contract.ts", + ], + }, + ], + // Disable base rules that are covered by TypeScript "no-unused-vars": "off", "no-undef": "off",