chore(layering): document R12/R18/R19, retire R8, make R9 shrink mandatory (#1781 A6) - #1825
Conversation
…atory (#1781 A6) The A6 review kept `check:layering` in full (15/15 planted violations fired, no other enforcer exists) and left four follow-throughs. R12 bin-alias-fast-path, R18 contracts-implementation-authority and R19 selector-pipeline-ownership were live rules with no ADR or CONTEXT anchor — they now carry one each, in the same list as R7/R9/R10/R13. R8 zero-dep-job-closure is retired: no CI job sets `install-deps: false` and ci.yml records why each keeps it enabled, so the invariant has no subjects. R11's relative-into-packages exception existed only because a zero-dep closure cannot coexist with specifier loads, so it retires with R8; the route is now closed to every caller. R1 was retired the same way at #1490. R9 was growth-only and merely suggested lowering the ceiling, which is headroom the next change spends without a number moving. It is now an equality pin like R6 and the R10 R7 counts, and the committed baseline drops 47 -> 46 (daemon-server ceiling 17 -> 16) to match the measurement. ADR 0019 §6 now says each runtime-command-cutover row is deleted when that command's migration is declared closed.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
thymikee
left a comment
There was a problem hiding this comment.
Adversarial review of #1825, checked out at 6e47400b0 in a scratch worktree, pnpm install --frozen-lockfile + pnpm check:layering run live.
Findings
1. (nit, low) .github/workflows/ci.yml diff is comment-only — confirmed safe. Both hunks (~L109-20, ~L154-33) only reword the R8-referencing comments; no install-deps, step, or job structure changed. Matches the PR body's claim. No behavior risk.
2. (nit, low) PR body's "18 command cutovers" doesn't match the current rule count. The Evidence bullet says "18 command cutovers," but the R14-R17 + R20-R31 cutover family is 16 rule ids (grep -oE "'R1[4-9] ...'|'R2[0-9] ...'|'R3[0-1] ...'" across scripts/layering/*.ts → 16), and the live check:layering success line enumerates exactly 16 commands (appstate, boot, apps, shutdown, install, reinstall, install_source, push, devices, logs, network, record, open, prepare, close, runtime). Not a code or doc-anchor defect — the number only appears in the PR description, not in CONTEXT.md or the ADR — but it's a stale/imprecise claim worth fixing before merge so reviewers don't anchor on it.
3. (informational) ADR 0019 section numbering. The reviewer brief referred to "§8" for the cutover-expiry sentence; the diff actually lands it in §6 ("Command cutover is the abandonment-safe migration unit," ~L429) and the R9-equality follow-up in §7 ("Adoption checkpoint," ~L469) — matching the PR body's own "§6" claim. Both placements read correctly in context and don't contradict surrounding text. No action needed; flagging only so nobody chases a phantom §8 diff.
Verified correct (no defects found)
- R8 retirement is complete and clean.
scripts/layering/zero-dep-jobs.tsdeleted (278 LOC), itsmodel.test.tsblock removed,'zero-dep-job-closure'dropped fromLAYERING_RULE_IDSandLAYERING_RULES, and every downstream consumer (checkPackageBoundaries,checkRootSites) had itszeroDepClosureFiles/zeroDepClosureparameter removed rather than stubbed.grepacross the tree forzero-dep|zeroDep|ZeroDepafter the change turns up only retirement-rationale comments (check.ts header, package-boundaries.ts header, ci.yml, test file) — no dangling imports/exports/params. Retirement convention matches R1's precedent (#1490 W0): a header note incheck.tsrecording both the reason and that "the number is spent," reinforced here with CONTEXT.md and ADR text since R8, unlike R1, had no surviving module to carry a doc comment next to its old declaration. LAYERING_RULE_IDS/ rule-id registry stay consistent.rule-ids.ts's collision check scans live'Rn name'string literals rather than an allowlist, so a fully-deleted rule needs no entry there;rule-ids.test.tsandKNOWN_RULE_ID_COLLISIONSare untouched and correctly not referencing R8.- R9 mandatory-shrink logic is correct in both directions, in
scripts/layering/daemon-modularity.ts:139-166:members.length > TYPE_CYCLE_BASELINEstill fails growth; the newelse if (members.length < TYPE_CYCLE_BASELINE)branch fails a stale-high baseline with a message naming the exact zone-ceiling delta; equal values hit neither branch and pass.TYPE_CYCLE_BASELINEis derived assum(LARGEST_TYPE_CYCLE_ZONE_CEILINGS)= 3+1+14+10+16+2 = 46, matching the committed baseline drop from 47→46 (daemon-server17→16) and the doc/PR-body claims. - The new unit test exercises the stale-high path specifically, not just growth:
daemon-modularity.test.ts's'R9 rejects a baseline left above the measured cycle'shrinksdaemon-serverby 1 (16→15, total 45 < baseline 46) and asserts exactly one violation with/dropped to 45 files \(baseline 46\)/and/Lower LARGEST_TYPE_CYCLE_ZONE_CEILINGS by the same 1/. Traced the per-zone math by hand — zone counts stay ≤ their ceilings so no extra R10 violations leak in, matchingviolations.length === 1. The adjacentbaselineTypeCycleMembershelper and the reworked "R9 records zone ceilings…" test (now trading members between zones to net zero total change) correctly isolate the per-zone claim from the total-size claim. - R12/R18/R19 CONTEXT.md bullets accurately describe the code, checked against
checkBinAliasFastPath/bin-alias-fast-path.ts(3 structural checks: value import present, delegation intobuildCommandUsageText, no local alias-token literal),contracts-implementation-policy.ts(FORBIDDEN_HOST_MODULES = child_process|fs|timers,FORBIDDEN_TIMER_CALLS,network-trafficparser-mechanics check), andselector-pipeline-ownership.ts(specifier-keyed single-importer check over the resolved graph, fails loud if the subpath stops resolving). No mismatch between doc prose and enforcement. - No rule was weakened. Read every changed
.tsunderscripts/layering/: no threshold moved except the documented, intentional R9 baseline drop (a tightening, not a relaxation — old rule was growth-only, new rule also catches stale-high); no allowlist widened; R11'scheckRootSiteslost its zero-dep exception entirely (strictly stricter — the old exception is gone, not generalized);package-boundaries.ts's "no relative route into a package, ever" is a strict superset of the old rule. - Live verification:
pnpm install --frozen-lockfile+pnpm check:layeringon6e47400b0→ 174/174 unit tests pass, checker green, success line reads "the largest type-level cycle is 46 files (R9)" with no R8 clause — exactly matching the PR body's test-plan claim.
VERDICT: approve
|
Reviewed exact head 6e47400: clean and merge-ready. The R12/R18/R19 documentation matches the implemented AST/import policies; R8 and its co-extensive R11 exception are honestly retired with rule IDs spent. R9 now requires the measured total to equal the zone-ceiling sum while preserving existing guards, and its stale-high/planted regression evidence is non-vacuous. All exact-head checks are green; device evidence does not apply to this tooling/docs change. |
…xceeded The per-zone R10 violation named members.find(<zone match>) — the alphabetically-first zone member, a file that had been in the cycle all along — so the +1 in #1825 x #1779 was found only by diffing largestTypeCycleMembers between commits. The ceiling records a count, not a membership, so the gate cannot name the joining file; it now lists every member of the over-budget zone and annotates the ceiling table instead. Closes #1837
…xceeded (#1852) * fix(layering): list the whole zone when R10's type-cycle ceiling is exceeded The per-zone R10 violation named members.find(<zone match>) — the alphabetically-first zone member, a file that had been in the cycle all along — so the +1 in #1825 x #1779 was found only by diffing largestTypeCycleMembers between commits. The ceiling records a count, not a membership, so the gate cannot name the joining file; it now lists every member of the over-budget zone and annotates the ceiling table instead. Closes #1837 * fix(layering): state the zone overflow in net terms Review nit: the overflow is net growth over the ceiling, not a join count (two joins and one departure print "1"), so the message no longer claims N members joined.
Follow-through on the #1781 A6 review of
check:layering. The verdict was KEEP in full — 15/15 planted violations fired, no other enforcer exists for import direction (tsconfig has no project references, oxlint restricts one module in one directory), and every numeric baseline movement in 65 commits of history is a lowering except one raise (#1590). Four small actions came out of it; three are code, one is a sentence in an ADR.Gate-level four lines (after this change)
exportsmap, and "the retired route is really gone" absence claims for 16 command cutovers.dropped to 46 files (baseline 47), and ascripts/file importing../packages/kernel/src/errors.tsnow fails R11 where the retired R8 exception used to admit it.zero-dep-jobs.tsgone. The cost is concentrated: theruntime-command-cutoverfamily is ~3,264 LOC (33%) serving one in-flight migration, which is why item 4 gives it an expiry.src/zone spine and every zone becomes its own package, at which point R2/R3/R5/R11 become resolution errors. Retire individual cutover rows when ADR 0019 declares that command's migration closed.Per-rule verdict (condensed)
commands/import fromcore/fired; no tsconfig/lint equivalentimport/no-cyclein the oxlint configinstall-deps: false; invariant has no subjectschild_processban coverssrc/**, notpackages/**fs/timers/timer calls have no other ownerexportspublishes the subpath to every consumerKNOWN_RULE_ID_COLLISIONS, stale allowances fail closedWhat changed
1. Documented R12, R18, R19. Three live rules had no ADR/CONTEXT anchor — only a comment in their own policy module, which does not survive the person who wrote it. Each now has a bullet in CONTEXT.md's layering list, in the same place and voice as R7/R9/R10/R13, stating the invariant and the rule id.
2. Retired R8
zero-dep-job-closure.grep 'install-deps: false' .github/workflowsreturns nothing, andci.yml(~L114) explicitly says to keepinstall-depsenabled on the layering guard and the Maestro conformance job. A rule with no subjects is not quiet, it is unemployed — and "the invariant is gone" is the one legitimate retirement reason under the A6 rule. Removed:scripts/layering/zero-dep-jobs.ts(278 LOC), its test block inmodel.test.ts, theLAYERING_RULE_IDSentry, and the doc mentions. R1 was retired the same way at #1490, and this follows that convention: the retirement is recorded incheck.ts's header comment next to R1's, saying both numbers are spent so a new rule takes the next free id rather than reusing them.R11's relative-into-
packages/exception was coupled to R8 — it required membership in an actual zero-dep closure, on the reasoning that nonode_modulesmeans specifier and relative loads cannot coexist and dual-instantiate. With no zero-dep job the closure is always empty, so the exception was already dead code; it retires with R8 andcheckRootSitesloses the parameter. Its test is replaced by one proving ascripts/file gets no relative route either.3. R9 shrink is now mandatory.
daemon-modularity.tsreported the largest type cycle at 46 against a 47 baseline and only suggested lowering it in the success line. That is a ceiling accumulating quiet headroom the next change spends without a reviewer seeing a number move. R9 is now an equality pin, likeTYPE_INVERSION_BASELINE(R6) and the R10 writer/owner counts already are: a baseline above the measured value fails with a message naming the constant and the delta. The committed baseline drops to the measured 46 (daemon-serverceiling 17 → 16, the one zone under its ceiling) so the gate is green in the same PR.4. Cutover family expiry. ADR 0019 §6, where the
runtime-command-cutovergate is specified, now states that each row is deleted in the change where the ADR declares that command's migration closed, and that the table must never outlive the migration. No code change — these 16 rows are ~33% of the gate's LOC and needed a stated end, not a deletion today.Test plan
pnpm check:layering— 174 tests pass, checker green; success line now reportsthe largest type-level cycle is 46 files (R9)with no R8 clause.'daemon-server': 16→17⇒[R9 type-cycle-size] the largest type-level import cycle dropped to 46 files (baseline 47). Lower LARGEST_TYPE_CYCLE_ZONE_CEILINGS by the same 1 ….scripts/a6-probe-tunnel.tsimporting../packages/kernel/src/errors.ts⇒[R11 package-boundaries] bypasses the package boundary. Under the old R8 exception an exports-named target from a zero-dep closure was admitted; there is no closure left to be in.R9 rejects a baseline left above the measured cyclepins the stale-high failure; the existing zone-ceiling test now trades members between zones so it probes the per-zone claim alone rather than the total.pnpm format:check,pnpm lint,pnpm typecheck,pnpm depgraph:test,pnpm check:gate-manifest:test,pnpm check:gate-manifestall green.pnpm check:production-exportsreports the same 17 pre-existing unused exports asmain(none inscripts/).Note on the rule name
R9was namedtype-cycle-growth. Now that it fails in both directions, a CI annotation readingLayering drift (R9 type-cycle-growth):: … dropped to 46 filescontradicts itself, so the rule is renamed toR9 type-cycle-size(two call sites plus the CONTEXT.md heading). The id is unchanged, andrule-ids.test.tsstill sees one name per number.