fix(layering): stop double-reporting contracts-authority violations - #1746
Conversation
main()'s violation list spread checkContractsImplementationAuthority(sources) twice, so every R11 contracts-implementation-authority finding was printed and ::error-annotated twice on a red run — inflating the headline violation count and producing duplicate GitHub annotations on the same file:line. Verified by planting a `setTimeout` call in a contracts production source: the rule reported 2 identical violations before and 1 after, with the extra annotation gone. `pnpm check:layering` stays green (136/136 policy tests). Nothing in the suite covers main()'s assembly of the violation list — the policy tests all call their rule functions directly — so neither a duplicated nor a dropped entry there is currently detectable.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. |
The duplicate this branch removed survived because nothing enumerates the guard's rules: main()'s violation list is hand-written, and the per-policy tests call their rule functions directly, never seeing the wiring. A lost spread is the dangerous version of the same gap — the rule stops being enforced and the run still prints OK. Make the file's own bindings the oracle: every in-scope `check*` value, local or imported, must be spread into main()'s violation list exactly once. That covers both directions plus a third case — a policy written and never wired in. Fails closed if main() or the array is renamed, so the instrument cannot pass by finding nothing. Test-only rather than an R17 inside the guard: a self-referential rule is defeated by dropping its own spread, which is exactly the failure it exists to catch. Verified by mutating the real check.ts in both directions (re-planting the duplicate, then dropping checkZeroDepJobs) — each turns the run red, and the restored file is green at 143/143.
|
check:layering named its 14 test files one by one, so adding a policy test meant remembering to register it — and twice nobody did. Both halves of the R16 record cutover shipped with tests that have never run: scripts/layering/record-runtime-mechanics-policy.test.ts (2 tests) scripts/layering/record-runtime-registry-policy.test.ts (1 test) Their policies are live in the guard; only the tests were dormant. All three pass, so nothing had rotted — the coverage was simply never being collected. Glob the directory the way mutation:test already globs its own, which makes the filesystem the enumeration and retires the registration step. 143 -> 146 tests, still green. This is the same defect as the duplicate spread this branch opened with, one level up: a hand-maintained list with nothing checking it against reality.
|
Reviewed exact head P2 — replace the AST wiring guard with a construction that makes duplicate registration impossible. Keep the one-line dedupe and the scoped The 112-LOC Please replace it with a typed, keyed rule registry plus a uniform adapter (for example, each entry consumes a shared layering context and returns violations), then execute the registry values once. Unique object keys make duplicate registration impossible, exhaustiveness can be checked against the rule-id catalog, and lint/typechecking exposes dropped wiring without reconstructing TypeScript syntax. Keep one small regression through that production interface. #1745 rewrites the same |
Replaces the AST wiring guard with a construction that cannot express the defect, per review on #1746. The parser was the wrong instrument: it reconstructed one array's shape from TypeScript syntax, so it only recognised top-level function declarations and imports whose local name matched /^check[A-Z]/. A const-defined or aliased rule was invisible to it, a helper named checkX was a false positive, and naming and syntax became part of the interface — all to detect a mistake rather than prevent it. Rules now live in a keyed table over a shared context, executed once via Object.values. An object cannot hold a key twice, so double registration is unrepresentable rather than merely detected, and oxlint's no-dupe-keys rejects the attempt at the source. LayeringRuleId makes a missing key a type error, and LAYERING_RULE_IDS gives the catalog to check exhaustiveness against. Call sites and order are unchanged, so grouped output and the success line are byte-identical. One regression test remains, through the production interface: scripts/ is outside tsconfig.json's `include`, so the Record's exhaustiveness is an editor signal rather than a CI gate, and the catalog assertion is what fails the build when wiring goes missing. Verified by mutation: dropping an entry and registering an uncatalogued one both fail the test, a duplicated key fails oxlint, and re-planting the original contracts violation reports it exactly once. Net -133 LOC.
|
P2 addressed in Your read of What's there now —
One correction worth your attention. Typecheck can't expose dropped wiring here: Lint does gate the duplicate direction, verified directly: a planted duplicate key errors under Mutation-verified, since the construction is only worth what it catches:
Dedupe and the scoped glob are untouched, as you asked. Noted on #1745 — its rebase needs to land the single registration and keep 🤖 Addressed by Claude Code |
thymikee
added
the
ready-for-human
|
Re-reviewed exact head 01da41e. The prior AST parallel-compiler issue is fixed: the syntax scanner is deleted, and production now constructs layering enforcement through one typed, keyed Stack note: #1745 currently rewrites the same assembly and package script, so its eventual rebase must preserve this registry and glob while registering its consolidated cutover rule. Current head is mergeable, but authoritative CI is still in progress; code-review clean, with merge readiness gated on those checks. No label yet. |
1 participant
Three findings, all in the layering guard's wiring. Each commit stands alone.
1.
fix— a rule was reported twicemain()spreadcheckContractsImplementationAuthority(sources)twice, so everyR11 contracts-implementation-authorityfinding was printed and::error-annotated twice: inflated headline count, duplicated file:line rows, and GitHub double-annotating the same line. Pre-existing.Proof: planted one violation (
setTimeoutinpackages/contracts/src/back-mode.ts) and ran the guard on both sides. The probe was reverted; it is not part of this PR.The pair was byte-identical in file, line, and message — double-reporting, not two overlapping rules.
2.
refactor— rules are registered, not hand-spreadThe duplicate survived because nothing enumerated the rules:
main()held a hand-written array of spreads, and every policy test calls its rule function directly —bin-alias-fast-path.test.ts:1says as much: "independently of the check.ts wiring that turns it into a violation." A duplicated entry is the mild failure; a dropped one retires a rule while the run still printsOK.Rules now live in
LAYERING_RULES, aReadonly<Record<LayeringRuleId, LayeringRule>>over a sharedLayeringContext, executed once viaObject.values(...).flatMap(...). Call sites and order are unchanged, so grouped output and the success line are byte-identical.oxlint no-dupe-keysrejects it at sourceRecord<LayeringRuleId, …>, plus the catalog testOne small regression test remains, through the production interface. Note:
tsconfig.json'sincludeis["src", "test", "scripts/help-conformance-command-validator.ts"], soscripts/layering/**is not typechecked — true of every policy there. TheRecord's exhaustiveness is an editor signal, not a CI gate, which is why the catalog assertion is load-bearing rather than belt-and-braces. Closing that properly needs ascripts/typecheck project; too big for this PR.Mutation-verified: dropping an entry ✖, duplicating a key ✖ (oxlint), registering an uncatalogued rule ✖, restored ✔.
3.
test— two suites had never runFound while verifying the above:
check:layeringnamed its 14 test files one at a time, so registration was manual — and twice it was missed. Both halves of the R16 record cutover shipped with tests that have never run:record-runtime-mechanics-policy.test.tsrecord-runtime-registry-policy.test.tsTheir policies are live in the guard; only the tests were dormant, and all three pass — the coverage was simply never collected. Fixed by globbing the directory the way
mutation:testalready globs its own, which makes the filesystem the enumeration.Same defect as #1, one level up: a hand-maintained list with nothing checking it against reality.
Reviewer notes
check.tsassembly and thepackage.jsontest command. Its rebase must preserve the single registration and the directory glob. The catalog test fails loudly if the merge drops a rule.contracts-implementation-authorityandpackage-boundaries; R13 is bothdevice-inventory-cutoverandplatform-package-substrate. Every other number is 1:1. The success line's "R11 holds 17 workspace package(s)…" describespackage-boundaries, so reading it to learn what R11 covers gives the wrong rule.scripts/integration-progress-model.test.tsis run by nothing (passes when run). No glob to fold it into; needs a deliberate home.