Repository navigation
chore(lint): enforce rule 15 so tests cannot quietly reach into src - #1344
Conversation
|
Warning Review limit reached
Next review available in: 10 minutes Limit details: You’ve used all 2 included reviews currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
There was a problem hiding this comment.
Pull request overview
This PR turns CLAUDE.md Rule 15 (“tests are end-to-end only”) from a review-time convention into an ESLint-enforced guarantee by adding a new custom rule that blocks runtime imports from src/lib/ and src/cli/ inside test/, with an explicit allowlist for deterministic-contract suites.
Changes:
- Add a new custom ESLint rule
neurolink/e2e-tests-onlyto flag runtime imports fromsrc/withintest/while allowing type-only imports and allowlisted deterministic suites. - Wire the rule into
eslint.config.js(including a documentedallowlist for determinism exceptions). - Update CLAUDE.md to reflect Rule 15 is now lint-enforced.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| eslint.config.js | Enables neurolink/e2e-tests-only for test/**/*.ts and defines the determinism allowlist. |
| eslint-rules/index.cjs | Exposes the new e2e-tests-only rule from the local neurolink ESLint plugin. |
| eslint-rules/e2e-tests-only.cjs | Implements the AST-based rule to ban runtime src/ imports from tests (with allowlist + type-only exceptions). |
| CLAUDE.md | Updates documentation to reflect Rule 15 is enforced via ESLint and documents the allowlist mechanism. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "no-local-type-alias": require("./no-local-type-alias.cjs"), | ||
| "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"), |
There was a problem hiding this comment.
Fixed in #1895: the plugin header in eslint-rules/index.cjs now lists the omitted rule and the related provider and security rules.
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
This PR enforces Rule 15 (tests are end-to-end only) by adding a new ESLint rule.
Review Summary
The implementation is correct and complete:
-
New ESLint Rule (
eslint-rules/e2e-tests-only.cjs): Correctly detects runtime imports fromsrc/lib/orsrc/cli/in test files, while allowing type-only imports and imports fromdist/. -
Documentation Updates: CLAUDE.md properly documents that Rule 15 is now lint-enforced, with clear explanation of the determinism exception mechanism.
-
Configuration: eslint.config.js correctly configures the rule with appropriate allow list for files that need deterministic control (chunk boundaries, parser edge cases, etc.).
-
No Breaking Changes: This is purely additive - no existing functionality is affected.
Impact Analysis
- Blast Radius: Minimal - adds one new ESLint rule
- Affected Code: Test files only
- Backward Compatibility: Existing tests that follow proper E2E patterns will continue to work; tests that incorrectly import from src/lib will now be flagged
Recommendation
✅ APPROVED - The PR correctly implements Rule 15 enforcement with proper documentation and configuration. No issues found during review.
f57f84d to
0e19104
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Rule 15 — tests are end-to-end only — was documented in a47c435 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 a47c435: 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 5502259 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.
0e19104 to
83b3b96
Compare
🤖 AI Review & Build Compliance ✅Status: AI analysis complete • Build rules validated • Ready for review 📊 View detailed analysis results🛡️ Analysis Complete
📋 Ready for Merge When
🤖 AI analysis complete - check individual code comments for specific feedback |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review Summary
Decision: APPROVED
This PR successfully adds automated enforcement for Rule 15 (e2e-tests-only), converting what was previously a manual review check into an automated ESLint rule.
Changes Made
-
New ESLint Rule (
eslint-rules/e2e-tests-only.cjs): Implements AST-based detection of runtime imports fromsrc/lib/orsrc/cli/in test files, with proper handling of:- Type-only imports (allowed - they're erased)
- Dist imports (allowed - this is the shipped entry)
- Allow list mechanism for deterministic control cases
- Clear warning about not "fixing" violations by moving to dist/ when stubbing
-
Documentation Updates (
CLAUDE.md):- Added Rule 15 to the lint-enforced rules list
- Documented the allow list mechanism
- Added table entry mapping Rule 15 to the ESLint rule
-
Configuration (
eslint.config.js,eslint-rules/index.cjs):- Properly integrated the new plugin and rule
- Configured allow list with 7 existing suites that legitimately need src imports
- Each allow-listed file has a header explaining why it needs the exception
Impact Analysis
- Changed files: 4 (all config/documentation files)
- Affected files: ~11 files impacted by graph traversal (mostly docs-site scripts)
- Risk score: 0.00 (configuration-only change)
- Breaking changes: None - this adds enforcement without changing behavior
Quality Verification
✅ Rule logic is correct and matches CLAUDE.md specification
✅ Documentation is clear and complete
✅ Configuration properly integrates with existing setup
✅ All existing test suites accounted for in allow list
✅ No security issues or functional changes
Recommendation
This is a well-implemented infrastructure improvement that enforces a critical testing discipline. The rule will catch future violations that would otherwise go unnoticed, improving test quality and preventing silent failures due to mixing source and dist module graphs.
Review SummaryDecision: APPROVED ✅This PR successfully adds automated enforcement for Rule 15 (e2e-tests-only), converting what was previously a manual review check into an automated ESLint rule. Changes Made1. New ESLint Rule (
|
| Metric | Value |
|---|---|
| Changed files | 4 (all config/documentation) |
| Risk score | 0.00 (configuration-only) |
| Breaking changes | None |
| Functional impact | Adds enforcement, no behavior change |
Quality Verification
✅ Rule logic matches CLAUDE.md specification exactly
✅ Documentation is clear and complete
✅ Configuration properly integrates with existing setup
✅ All 7 existing test suites accounted for with proper justifications
✅ No security issues introduced
✅ Follows NeuroLink conventions and best practices
Recommendation
This is a well-implemented infrastructure improvement that enforces a critical testing discipline. The rule will catch future violations that would otherwise go unnoticed, preventing silent failures due to mixing source and dist module graphs in tests. This strengthens the project's quality guarantees and aligns with the principle that tests should exercise the shipped surface, not internal implementation details.
|
📝 MINOR SUGGESTION: Rule 15 now lint-enforced - no code issues This PR is a configuration-only change that adds automated enforcement for Rule 15 (e2e-tests-only). The ESLint rule implementation correctly detects violations while allowing legitimate exceptions (type imports, dist imports, and allow-listed deterministic cases). Documentation is complete and accurate. No code issues found - this is an infrastructure improvement with zero functional changes. |
🛡️ Yama Review Verdict: CHANGES_REQUESTEDSeverity counts — 🔒 CRITICAL: 0 · 🤖 Yama Review Summary
Verified findings (1):
Findings behind this verdict
|
|
🎉 This PR is included in version 11.2.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes the review threads left open on merged PRs against the CI workflows, the repo's gate scripts and a few dev tools. Each change is the smallest one that closes its finding. Behaviour changes are covered by a new suite (test/continuous-test-suite-tooling-scripts.ts, `pnpm run test:tooling-scripts`) and by additions to the provider-structure and provider-descriptors suites; every new test was run red against the unfixed source first. Workflows - ci.yml: persist-credentials: false on the seven checkouts that never push, semantic-release-validation keeps its token (T3790294038, #1335). The permissions comment names that job as the one contents: write exception (T3858986829-a, #1552). The pinned suite counts and the 373-assertion figure are gone (T3869180755-f1, #1580). The new tooling-scripts suite is added to extended-suites; its weight (100) is an estimate, not a CI median. - release.yml: the ffmpeg note no longer says build-check gates this workflow (T3810295618-a, #1360). - single-commit-enforcement.yml: one SKIP_RE shared by both greps, printf instead of echo, and the guidance names the push/pull_request workflows rather than "every workflow" (T3813387872-printf-regex, T3813416696-overstated-guidance, #1364). Config and lint docs - config/models.json: Opus 4.5 uses the real snapshot id 20251101 for anthropic, bedrock and vertex instead of the 20251124 launch date (T3816077440, #1375). provider-structure now checks every Claude id in the file against the model enums. - eslint-rules/index.cjs: header lists e2e-tests-only, no-inline-secret-regex, provider-typed-errors and provider-base-class (T3801758166-1, #1344). Scripts - build-validations.ts: fails when typedoc.json carries an unanchored `**/<dir>/**` exclude, which drops every file under a checkout whose path contains that directory (T4042344752-guard, #1723). - check-banned-deps.ts: scans each file as a whole, so import(), require() and `from` followed by a specifier on the next line are found, and a `//` inside a string no longer hides the rest of the line (T3956062753, #1662). Files in the repo root and .mts/.cts are scanned too (T3956062775, #1662). - check-shipped-types.ts: the declarations under dist/ must equal the set the source tree emits, so a partial or stale build above the 100-file floor fails (PF-T3927528338, #1627). A wildcard export is matched against the whole pattern, including a `*` in a directory component (T3931686738-wildcard-match, #1632). - codex-replay-listener.ts: the tool-call script names `replay_tool` instead of `exec`, which Codex declares as a custom tool and which raised a Fatal "incompatible payload" error (F1-T4087477953-custom-tool-shape, #1783); reproduced and cleared against codex-cli 0.160.0. --requests counts served /responses turns, so a 404 probe cannot shut the listener down first (F2-T4087477985-requests-limit-counts-404s, #1783). - commit-validation.ts: execFileSync("git", [...]) instead of a shell string; behaviour unchanged (T3838161513-b, #1499). - migration-symbol-diff.mjs: this/super-rooted paths keep their full name, and tagged templates, obj["name"](), super() and import() are tracked; the header says it follows calls (T3835058026-residual, PF-T3833252257, #1448). - tools/automation/environmentManager.ts: credential-free providers count as configured only when the .env sets one of their variables, the score no longer divides by the size of the catalog, and the report lists the configured providers plus one count instead of every missing one (T3792794348, T3792807279, #1337). Not done, on purpose - The skip-checks trailer in the single-commit grep (optional in the finding). - Checkouts in workflows other than ci.yml: the findings named only ci.yml. - migration-symbol-diff still does not record a function passed by reference (`items.forEach(handler)`); the header now says so. Pre-existing, not touched: test:dynamic fails its five live cases without provider credentials, identically with config/models.json reverted.
Rule 15 — tests are end-to-end only — was documented in
a47c4353but only checked by review. Review is exactly 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.The rule
neurolink/e2e-tests-only, an AST rule over files undertest/. It flags a runtime import fromsrc/lib/orsrc/cli/, in either form:It deliberately does not flag:
The
allowlist is the determinism exceptionIt lives in
eslint.config.jswith a one-line reason per entry:raggenerate({ rag })only shows the model's answerbugfixesproxyautoresearchvector-chroma,vector-pineconeAdding 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.
What it found on its first run
One file — and it's a good sign.
continuous-test-suite-error-classifier-contract.ts, added in5502259cafter 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's allowlisted rather than converted — the convention was applied correctly before the rule existed to enforce it.
The trap the message warns about
The rule's message points at the fix, and specifically warns against "fixing" a violation by moving the import to
../dist/in a file that also stubs or spies on that module.dist/index.jsis a separate bundled copy, so the stub patches a different graph and the test silently starts doing real work — while typechecking clean. That cost three separate silent failures while writinga47c4353.Verification
Broke it on purpose. A probe file importing a value from
src/libstatically and dynamically reports both violations;import type,{ type X }and a../dist/import in the same file report nothing.pnpm run lint→ 0 errors (52 pre-existing warnings) ·pnpm run check→ exit 0.