chore: enforce module boundaries, ports and no vi.mock with ESLint (#88) - #89
Conversation
Eleven red tests that lint snippets under hypothetical paths through the ESLint API, one per rule: Node I/O only from node-*.ts adapters, composition roots and the burn-down list; imports past a module's index.ts from outside; vi.mock in tests.
…#88) Root eslint.config.mjs with an inline module-boundary rule that discovers modules by their index.ts, no-restricted-imports for Node I/O outside node-*.ts adapters and composition roots, and no-restricted-properties for vi.mock in tests. Burn-down lists exempt the 20 files, 3 boundary-crossing tests and 3 mocking tests that predate the rules. appduct and shared gain a lint script; turbo and CI pick it up unchanged. The architecture skill now names the enforcement. 11 failing -> 0 failing
V3RON
left a comment
V3RON
left a comment
There was a problem hiding this comment.
Verdict: comment. 2 should-fix, 2 nit. Spec: issue #88 (no comments) plus the PR criteria table.
Fix first: no-restricted-imports does not see dynamic import(), so await import("node:fs/promises") in any source file passes lint (verified against ESLint 10.11 with a probe on src/daemon/*.ts).
| files: SOURCE, | ||
| ignores: ["**/__tests__/**", "**/node-*.ts", ...COMPOSITION_ROOTS, ...LEGACY_NODE_IO], | ||
| rules: { | ||
| "no-restricted-imports": [ |
There was a problem hiding this comment.
should-fix. no-restricted-imports only visits ImportDeclaration and the two Export* nodes (see node_modules/eslint/lib/rules/no-restricted-imports.js), so this file passes lint:
// packages/appduct/src/daemon/anything.ts
export const read = async (p: string) => (await import("node:fs/promises")).readFile(p, "utf8");I confirmed it with ESLint.lintText on packages/appduct/src/daemon/probe-new.ts: zero messages. The idiom is already used 18 times in src/__tests__, so it will get copied into source. The inline plugin already visits ImportExpression for the boundary rule; give it a second rule that reports a string-literal import() of any NODE_IO name under the same ignores, or add no-restricted-syntax with ImportExpression > Literal[value=/^(node:)?(fs|fs\/promises|child_process|net|tls|http|https|os|dns)$/], and add the dynamic form to the ports tests.
There was a problem hiding this comment.
Resolved in a9cdeaa. appduct/no-node-io-dynamic-import reports a string-literal import() of any of the nine names under the same ignores as the static rule. Re-verified at ceb279b through ESLint.lintText: the round-1 probe, bare import("fs"), import("node:child_process") and a packages/shared/src/domains/*.ts file each report the rule; the same snippet in node-filesystem.ts, cli.ts and __tests__/ passes. The new test at line 42 is red without the rule.
| // Burn-down list: files that reached Node I/O directly before the ports rule existed. Remove | ||
| // a file from here when you convert it (port in the module, node-*.ts adapter beside it, | ||
| // in-memory fake for tests). Never add a file to this list. | ||
| const LEGACY_NODE_IO = [ |
There was a problem hiding this comment.
should-fix. Nothing fails when an entry on these three lists stops being a violator, and nothing fails when one is added; only the comment asks for that. Scenario: daemon/audit.ts converts to a port but the PR forgets this line; a later change puts import { appendFile } from "node:fs/promises" back into audit.ts and lint stays green because the exemption is still here. Export the lists (a named export next to the default is fine for ESLint) and have lint-boundaries.test.ts assert every entry exists and is red for its rule when linted with the exemption removed (e.g. overrideConfig re-enabling the rule for that path). Then a conversion cannot leave a stale exemption and an addition shows up as a test diff instead of a line in a 20-entry list.
There was a problem hiding this comment.
Resolved in a9cdeaa. Ran the test's violators() logic in my own ESLint instance against ceb279b: appending daemon/event-bus.ts to LEGACY_NODE_IO fails the assertion, a nonexistent entry throws No files matching, a non-mocking test appended to LEGACY_VI_MOCK and a same-module test appended to LEGACY_MODULE_BOUNDARY both fail, and lintFiles keeps input order so toEqual is not order-fragile. One nit on the empty-list case, inline in the test.
| }); | ||
|
|
||
| test("a directory without an index.ts is not a module yet, so its files are importable", async () => { | ||
| const rules = await lint("packages/appduct/src/mcp/probe.ts", 'import { createEventBus } from "../daemon/event-bus.js";\nexport const b = createEventBus;\n'); |
There was a problem hiding this comment.
nit. This pins the tree, not the rule: the PR's own out-of-scope list says daemon will gain an index.ts, and the moment it does, this test fails with a message that says the opposite of what happened. Use a directory that cannot become a module, e.g. ../no-such-dir/thing.js (discovery only lists existing directories, so the rule passes it).
There was a problem hiding this comment.
Resolved in 439aa56. ../no-such-dir/thing.js can never be listed by discovery, so the test no longer depends on daemon lacking an index.ts.
| const NODE_IO = ["fs", "fs/promises", "child_process", "net", "tls", "http", "https", "os", "dns"]; | ||
|
|
||
| // The entry points that construct real adapters and hand them to everything else. | ||
| const COMPOSITION_ROOTS = [ |
There was a problem hiding this comment.
nit. .claude/skills/architecture/SKILL.md line 65 still says the CLI composition root is packages/appduct/src/cli/..., but the enforced roots are src/cli.ts and src/bin.ts, and src/cli/open-target.ts is on the burn-down list. An agent following the skill and putting new NodeSomething() in src/cli/runner.ts gets a lint error the skill told it would not happen. Name these three files in the skill.
There was a problem hiding this comment.
Resolved in a9cdeaa. The skill now names src/bin.ts, src/cli.ts and src/daemon/daemon.ts, matching COMPOSITION_ROOTS. docs/ARCHITECTURE.md does not describe the roots, so nothing else to align.
#88) A second inline rule reports import() of a Node I/O module under the same exemptions as the static form. The three lists and the two rule options are named exports so the test can lint every listed file with its exemption removed and fail on a stale or missing entry. The architecture skill names the three composition roots lint enforces. 4 failing -> 0 failing
V3RON
left a comment
V3RON
left a comment
There was a problem hiding this comment.
Verdict: approve (posted as a comment: GitHub refuses the approve event from the PR author's account). 0 blocker, 0 should-fix, 1 nit. Spec: issue #88 plus the PR criteria table; all four round-1 threads resolved and re-verified at ceb279b.
Nit, if you touch it: violators() throws on an empty burn-down list, so the PR that converts the last entry of a list fails this test for the wrong reason.
| const loadConfig = async (): Promise<Config> => import(pathToFileURL(path.join(repoRoot, "eslint.config.mjs")).href); | ||
|
|
||
| const violators = async (files: string[], rules: Linter.RulesRecord, expectedRule: string) => { | ||
| const withoutExemption = new ESLint({ cwd: repoRoot, overrideConfig: [{ files, rules }] }); |
There was a problem hiding this comment.
nit. When a list is burned down to [], this throws Config (unnamed): Key "files": Expected value to be a non-empty array (ESLint 10.11, checked with overrideConfig: [{ files: [] }]), so the PR that converts the last LEGACY_VI_MOCK entry fails this test for the wrong reason. Either if (files.length === 0) return []; here, or state in the describe comment that a test is deleted together with its list, which the toBeGreaterThan(0) on line 113 already implies for LEGACY_NODE_IO.
…val limit for review-pr (#88)
1 participant
Closes #88
What changed
ESLint for
packages/appductandpackages/shared, carrying only the three structural rules fromAGENTS.md. No style rules. The React Native package keeps its own setup. The existing CI lint job runs it through turbo with no workflow change.node:fs,node:fs/promises,node:child_process,node:net,node:tls,node:http,node:https,node:osandnode:dnsare errors outside adapter files (node-*.ts), the three composition roots, and tests. Every file that did this before the rule existed is on a burn-down list in the config with a comment: remove your file when you convert it, never add one.appduct/module-boundary, discovers every directory undersrcthat has anindex.tsand reports any relative import from outside that reaches past it. Discovery runs at lint time, so a directory becomes a module the moment it gains anindex.ts; nothing to maintain.vi.mockandvi.doMockare errors in tests, with the three existing users on the same kind of burn-down list. Three tests that reached inside theclientmodule are on a third list for the boundary rule.eslint src; ESLint 10 finds the root config by walking up from the linted files, and passing--configwould resolve the patterns against the package directory instead.Acceptance criteria
node:fsoutside an adapter or composition root failslint-boundaries.test.ts"a new file reaching Node I/O directly fails", and the shared-package twinnode-*.tsadapters and composition roots passindex.tsfrom outside fails; the index and same-module imports pass; directories without an index are not modulesvi.mockin a test fails; tests may still use Node I/O for temp filesimport()of Node I/O fails like the static formpnpm lintpasses on the unchanged tree; the allowlist names every exempted fileE2E evidence
not applicable (lint configuration only; no runtime behaviour)
Checklist
CHANGELOG.mdhas a line underUnreleased, or the change is not user-visiblewriting-user-docsskill), or the change is not user-visibleindex.ts; no new directnode:*I/O outside an adapterarchitectureskill applied, exceptions explained abovedocs/ARCHITECTURE.mdupdated if a surface it describes changedOut of scope
index.tstodaemon,mcp,rpc,cli,commands: same policy.Status
Implement: done (15/15 green, lint and typecheck pass on all packages) Review: round 1 comment (2 should-fix, 2 nit, all addressed in a9cdeaa and ceb279b), round 2 pending E2E: n/a Ready: no