refactor(layering): declare R1-R3 as a policy table, and test them - #1449
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. |
|
|
Code review is clean at Exact-head targeted tests and the guard are green. Not ready solely because GitHub reports a real |
1e3176a to
6557578
Compare
|
Rebased onto The conflict was - "check:layering": "… --test scripts/layering/model.test.ts && … check.ts",
+ "check:layering": "… --test scripts/layering/model.test.ts scripts/layering/zone-policy.test.ts && … check.ts",with main's two coverage scripts untouched above it. Re-ran the exact-head checks. One thing I checked because the new base makes it a fair question: #1447's changed-line coverage gate scopes to Nothing else changed in the rebase — the six new commits on main touch replay, packaging smoke, error-code quizzes, Renovate config and the command-surface gate, none of which overlap Generated by Claude Code |
R1-R3 were three hand-written predicate functions. Each was short, but each
buried its boundary in control flow: you had to read the early-returns to learn
that R3 tolerates dynamic imports, or that R1 opens exactly one door. They are
now data in scripts/layering/zone-policy.ts -- which zones a boundary governs,
which import kinds it tolerates, which path prefixes are its declared seam --
walked by one small evaluator. A fourth zone boundary becomes a table entry
rather than a fourth predicate to keep consistent with the other three.
R1 is deliberately two entries rather than one with a special case, because
"kernel may import contracts type-only" and "kernel may import nothing else at
all" are two statements, and writing them separately is what makes the single
open door visible.
The refactor exposed a real gap: checkLayeringRules had NO unit test. The only
thing exercising R1-R3 was the real tree, which is clean, so a rule that had
silently stopped matching would have looked exactly like a rule being obeyed.
zone-policy.test.ts now asserts each boundary fires and each documented
exemption holds, including that src/daemon/client/ is excluded from the daemon
seam. Verified end-to-end by injecting one violation per rule plus an exempt
file: the gate reports 4 zone-policy violations (R3 twice, catching the
daemon/client case), ignores the type-only and dynamic edges, and exits 1.
Also records in docs/dependency-graph-findings.md the result of spiking
eslint-plugin-boundaries under oxlint's jsPlugins, so nobody repeats it. It
does work -- jsPlugins loads npm ESLint plugins with no ESLint install, and
R1-R3 are all expressible once you know importKind: "value" and
settings["boundaries/dependency-nodes"]. Not adopted: no ratchet mechanism (the
thing that took R6 from 61 to 7 incrementally), it cannot express R4-R9 so the
architecture would be defined in two places, it misreads inline `{ type Foo }`
specifiers as value imports (one false positive on providers/limrun/android.ts),
message interpolation renders empty under the current selector syntax, and it
costs 230 transitive packages on an API documented as alpha.
No behaviour change: 932 source files, R6 = 7, R7 = 41 fields, R8 clean,
R9 = 102, all identical to before.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur
6557578 to
ffd8316
Compare
|
Lint & Format was red on The failure: Two formatting nits only — a long Why my local run missed it. I ran The gap worth flagging: No I have deliberately not bundled that here — it changes a shared script on a PR that's otherwise a single-purpose refactor, and it wasn't part of your feedback. Happy to add it to this PR or open a one-liner separately, whichever you prefer. Re-verified on Generated by Claude Code |
|
Reviewed clean at The policy evaluator preserves the R1–R3 contracts, including kernel’s sole type-only contracts escape, the daemon-client platform exclusion, and dynamic/type-only platform exemptions. The added tests prove both violations and documented exemptions, and all checks are green. Ready for human review. |
Summary
R1–R3 were three hand-written predicate functions. Each was short, but each buried its boundary in
control flow — you had to read the early-returns to learn that R3 tolerates dynamic imports, or that
R1 opens exactly one door. They are now data in
scripts/layering/zone-policy.ts: which zones aboundary governs, which import kinds it tolerates, which path prefixes are its declared seam, walked
by one small evaluator. A fourth zone boundary becomes a table entry rather than a fourth predicate
to keep consistent with the other three.
check.tsis 86 lines lighter. No behaviour change: 932 source files, R6 = 7, R7 = 41 fields,R8 clean, R9 = 102 — all identical to before.
R1 is deliberately two entries rather than one with a special case, because "kernel may import
contracts type-only" and "kernel may import nothing else at all" are two statements. Writing them
separately is what makes the single open door visible — and a test pins that, so a future merge of
the two fails and says why.
The refactor exposed a real gap: R1–R3 had no unit test
checkLayeringRuleshad none. The only thing exercising these three rules was the real tree, whichis clean — so a rule that had silently stopped matching would have looked exactly like a rule being
obeyed.
zone-policy.test.tsadds 6 tests asserting each boundary fires and each documentedexemption holds, including that
src/daemon/client/is excluded from the daemon seam (the old codeexpressed that as a negated path check nothing verified).
Verified end-to-end by injection, not just by unit test — one violation per rule plus an exempt file:
kernel/ → utils/value importcore/ → commands/value importcli/ → platforms/static value importdaemon/client/ → platforms/static value importseamExceptworkscli/with a type-only and a dynamic platforms importGate reported 4 zone-policy violations (+2 from R5, independently) and exited 1. Cleaned up, back
to
OK.Also:
eslint-plugin-boundariesunder oxlint — spiked, works, not adoptedRecorded in
docs/dependency-graph-findings.md §7with the working config so nobody repeats theinvestigation.
It does work. oxlint's
jsPluginsloads npm ESLint plugins directly — no ESLint install, nosecond config, no extra CI step. R1–R3 are all expressible and all three were confirmed firing on
injected violations. The two non-obvious settings:
importKind: "value"on the policyimport()settings["boundaries/dependency-nodes"] = ["import", "export"]Why not adopted. No single item is fatal; together they lose more than the declarative syntax
gains, and
ZONE_POLICIESprovides that syntax anyway:tolerates N and fails on N+1. A per-file lint rule has no cross-run aggregate. A new boundary with
existing violations — a platforms facade would have ~89 — needs one disable comment per site.
not an import rule at all), R8 (CI job closure) and R9 (cycle size) are whole-graph or non-import
properties. Adopting it means the architecture is defined in two places.
import { type A, type B } from '…'is fully erased atruntime, but the plugin classifies it as a value import — its
importKindis statement-level.One genuine false positive here:
providers/limrun/android.tsflagged for R3, which the gatecorrectly ignores. 10 of 1,481 type-only edges use that form, so the blast radius is small, but
statementIsTypeOnlyhandles both spellings and the plugin does not.{{dependency.type}}and{{file.type}}interpolated to empty, so violations readmust not import commands/with no zone named. ADR 0010 wants every error actionable.jsPluginsdocumented as alpha and not subject to semver, andfive deprecation warnings from one config attempt (
mode,rules, legacy selectors, legacytemplates, rule-level
importKind) spanning two major migrations.Two things nearly fooled me and are worth knowing: an invalid rule option makes the plugin silently
report zero findings — I briefly concluded "R3 is expressible!" from a rule that wasn't running
(oxlint does exit 1, so CI would catch it, but the lint output is clean). And my first two configs
produced 27 then 18 "violations" that were entirely classification bugs from
mode/partialMatchsemantics. Injection testing caught both; reading the output would not have.
Worth re-evaluating under two conditions, both noted in the doc: a monorepo migration (per-package
ESLint configs change the calculus), or
jsPluginsgoing stable with the ratchet gap addressable.Validation
pnpm checkgreen — lint, typecheck, R1–R9,depgraph:test, production exports, MCP metadata,build, bundle owner files, Fallow, 4642 unit tests. The new test file is wired into both
check:layeringand the Layering Guard CI job, so it cannot pass locally and be skipped in CI.No dependency changes: the spike's plugin was removed and
pnpm-lock.yamlis untouched.🤖 Generated with Claude Code
https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur
Generated by Claude Code