Skip to content

Commit ba53691

Browse files
committed
fix(layering): derive the rule namespace and stop R18 double-reporting
Review follow-ups on the R11/R13 split. Make the collision class impossible. Nothing enforced one number to one rule name; the split was verified by a one-off grep. rule-catalog.ts derives the catalog from the guard's own sources by matching rule-shaped string literals in any syntactic form, and check.ts refuses to report through a colliding namespace before grouping anything by rule -- an ambiguous id makes every `Layering drift (R…)` annotation untraceable, so it fails as a guard-integrity fault rather than as one more finding under a bad id. Matching literals rather than `const RULE =` and `rule:` call sites keeps a new declaration form from being invisible, and reading the directory keeps the catalog off a hand-maintained list. Planted-red covers the exact pre-fix collisions; the real pre-fix tree reproduces them and exits 1. Stop R18 double-reporting. check.ts spread checkContractsImplementationAuthority(sources) twice, so one contracts violation produced two counts and two annotations. One planted contracts violation now reports once (verified: 2 annotations before, 1 after). Correct the normative ADR reference. ADR 0019's platform-package import rules -- contracts-to-platform, sibling-platform, root/daemon, raw-process -- are R13's, as CONTEXT.md:420 already says. The R11 attribution predates platform-package-policy (#1697, a day before #1699), when R11 was the only package rule. Namespace is one-to-one across R2-R18; the OK line now reports that.
1 parent 6b02dfe commit ba53691

5 files changed

Lines changed: 291 additions & 4 deletions

File tree

docs/adr/0019-request-bound-platform-runtime.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ only that composition module may import a concrete platform package; reusable ty
111111
through type-only platform imports. Platform packages may import contracts, kernel/domain packages,
112112
and explicitly injected host capabilities; they may not import daemon requests or responses, mutable
113113
session state, command catalogs/grammar, root implementation files, sibling platform packages, or raw
114-
process primitives outside the shared host-command port. R11 applies these rules to static, type-only,
114+
process primitives outside the shared host-command port. R13 applies these rules to static, type-only,
115115
dynamic, and re-export edges; package-owned tests may import their own public façade. Contracts may
116116
depend on kernel vocabulary but never on concrete platform packages or daemon implementation types.
117117

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,7 @@
130130
"check:affected:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/check-affected/model.test.ts scripts/check-affected/platform-packages.test.ts scripts/check-affected/run.test.ts",
131131
"check:coverage-changed": "node --experimental-strip-types scripts/coverage-changed/run.ts",
132132
"check:coverage-changed:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/coverage-changed/model.test.ts scripts/coverage-changed/run.test.ts",
133-
"check:layering": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/layering/model.test.ts scripts/layering/zone-policy.test.ts scripts/layering/daemon-modularity.test.ts scripts/layering/package-boundaries.test.ts scripts/layering/platform-package-policy.test.ts scripts/layering/platform-package-repository.test.ts scripts/layering/platform-package-source-policy.test.ts scripts/layering/device-inventory-cutover-policy.test.ts scripts/layering/logs-runtime-cutover-policy.test.ts scripts/layering/network-runtime-cutover-policy.test.ts scripts/layering/record-runtime-cutover-policy.test.ts scripts/layering/contracts-implementation-policy.test.ts scripts/layering/facade-exports.test.ts scripts/layering/bin-alias-fast-path.test.ts && node --experimental-strip-types scripts/layering/check.ts",
133+
"check:layering": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/layering/model.test.ts scripts/layering/zone-policy.test.ts scripts/layering/daemon-modularity.test.ts scripts/layering/package-boundaries.test.ts scripts/layering/platform-package-policy.test.ts scripts/layering/platform-package-repository.test.ts scripts/layering/platform-package-source-policy.test.ts scripts/layering/device-inventory-cutover-policy.test.ts scripts/layering/logs-runtime-cutover-policy.test.ts scripts/layering/network-runtime-cutover-policy.test.ts scripts/layering/record-runtime-cutover-policy.test.ts scripts/layering/contracts-implementation-policy.test.ts scripts/layering/facade-exports.test.ts scripts/layering/bin-alias-fast-path.test.ts scripts/layering/rule-catalog.test.ts && node --experimental-strip-types scripts/layering/check.ts",
134134
"depgraph": "node --experimental-strip-types scripts/depgraph/build.ts",
135135
"depgraph:test": "node --experimental-strip-types scripts/node-test-tmpdir.ts --experimental-strip-types --test scripts/depgraph/model.test.ts scripts/depgraph/affected.test.ts",
136136
"check:production-exports": "fallow dead-code --config fallow-production-exports.json --production --unused-exports --fail-on-issues",

scripts/layering/check.ts

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,13 @@ import {
104104
sourceExecutedUsingDeclarationViolations,
105105
} from './logs-runtime-cutover-policy.ts';
106106
import { contractsImplementationAuthorityViolations } from './contracts-implementation-policy.ts';
107+
import {
108+
layeringPolicySources,
109+
ruleCatalogSummary,
110+
ruleDeclarations,
111+
ruleNamespaceCollisionReport,
112+
ruleNamespaceCollisions,
113+
} from './rule-catalog.ts';
107114
import {
108115
networkLegacyRouteViolations,
109116
networkRuntimeNarrowingViolations,
@@ -121,6 +128,9 @@ const repoRoot = execFileSync('git', ['rev-parse', '--show-toplevel'], {
121128
encoding: 'utf8',
122129
}).trim();
123130

131+
/** The guard's own sources, where every rule identifier is declared. */
132+
const policyDir = import.meta.dirname;
133+
124134
export function listTypeScriptFiles(): string[] {
125135
// `src/**/*.ts` only matches nested files; root-level `src/*.ts` (e.g.
126136
// src/cli.ts, src/command-catalog.ts) needs its own pathspec or it silently
@@ -622,7 +632,7 @@ function report(
622632
`${packageBoundariesSummary(repoRoot)}; ${platformPackagePolicySummary()}; ` +
623633
`${deviceInventoryCutoverSummary()}; and bin.ts imports normalizeCliCommandAlias, ` +
624634
`actually passes it into buildCommandUsageText, and holds no local alias literals ` +
625-
`(R12).\n`,
635+
`(R12); ${ruleCatalogSummary(ruleDeclarations(layeringPolicySources(policyDir)))}.\n`,
626636
);
627637
return 0;
628638
}
@@ -649,6 +659,19 @@ function report(
649659
}
650660

651661
export function main(): number {
662+
// Before anything is grouped by rule: a number answering to two rules makes every
663+
// `Layering drift (R…)` annotation below untraceable, so this fails as a guard-integrity
664+
// fault rather than as one more drift finding under an ambiguous id.
665+
const collisions = ruleNamespaceCollisions(ruleDeclarations(layeringPolicySources(policyDir)));
666+
if (collisions.length > 0) {
667+
process.stderr.write(
668+
`Layering guard: rule namespace is not one-to-one\n\n` +
669+
`${ruleNamespaceCollisionReport(collisions)}\n\n` +
670+
` Give each rule its own number before the report can name what fired.\n`,
671+
);
672+
return 1;
673+
}
674+
652675
const sourceFiles = listSourceFiles();
653676
const sources = readSources(sourceFiles);
654677
const allTypeScriptSources = readSources(listTypeScriptFiles());
@@ -663,7 +686,6 @@ export function main(): number {
663686
...checkContractsImplementationAuthority(sources),
664687
...checkNetworkRuntimeCutover(sources),
665688
...checkRecordRuntimeCutover(sources),
666-
...checkContractsImplementationAuthority(sources),
667689
...checkBackEdges(edges),
668690
...checkTypeInversions(edges),
669691
...checkSessionStateOwnership(sources),
Lines changed: 171 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
// The rule namespace holds one number to one rule, and the planted-red cases are the exact
2+
// collisions this catalog was introduced to reject: R11 named both package-boundaries and
3+
// contracts-implementation-authority, R13 named both platform-package-substrate and
4+
// device-inventory-cutover.
5+
6+
import assert from 'node:assert/strict';
7+
import path from 'node:path';
8+
import { test } from 'node:test';
9+
import {
10+
layeringPolicySources,
11+
ruleCatalogSummary,
12+
ruleDeclarations,
13+
ruleNamespaceCollisions,
14+
type LayeringPolicySource,
15+
} from './rule-catalog.ts';
16+
17+
const policyDir = import.meta.dirname;
18+
19+
function sources(...entries: readonly (readonly [string, string])[]): LayeringPolicySource[] {
20+
return entries.map(([file, source]) => ({ path: file, source }));
21+
}
22+
23+
function collisionsIn(...entries: readonly (readonly [string, string])[]) {
24+
return ruleNamespaceCollisions(ruleDeclarations(sources(...entries)));
25+
}
26+
27+
test('the pre-fix R11 collision is rejected: one number cannot name two rules', () => {
28+
assert.deepEqual(
29+
collisionsIn(
30+
[
31+
'contracts-implementation-policy.ts',
32+
"const RULE = 'R11 contracts-implementation-authority';",
33+
],
34+
['package-boundaries.ts', "const violation = { rule: 'R11 package-boundaries' };"],
35+
),
36+
[{ number: 11, names: ['contracts-implementation-authority', 'package-boundaries'] }],
37+
);
38+
});
39+
40+
test('the pre-fix R13 collision is rejected across three declaring files', () => {
41+
assert.deepEqual(
42+
collisionsIn(
43+
['device-inventory-cutover-policy.ts', "const RULE = 'R13 device-inventory-cutover';"],
44+
['platform-package-policy.ts', "const RULE = 'R13 platform-package-substrate';"],
45+
['platform-composition-policy.ts', "const RULE = 'R13 platform-package-substrate';"],
46+
),
47+
[{ number: 13, names: ['device-inventory-cutover', 'platform-package-substrate'] }],
48+
);
49+
});
50+
51+
test('the post-fix allocation of those same five rules is one-to-one', () => {
52+
assert.deepEqual(
53+
collisionsIn(
54+
[
55+
'contracts-implementation-policy.ts',
56+
"const RULE = 'R18 contracts-implementation-authority';",
57+
],
58+
['package-boundaries.ts', "const violation = { rule: 'R11 package-boundaries' };"],
59+
['device-inventory-cutover-policy.ts', "const RULE = 'R17 device-inventory-cutover';"],
60+
['platform-package-policy.ts', "const RULE = 'R13 platform-package-substrate';"],
61+
['platform-composition-policy.ts', "const RULE = 'R13 platform-package-substrate';"],
62+
),
63+
[],
64+
);
65+
});
66+
67+
test('one rule declared under two numbers is allowed; only a shared number is drift', () => {
68+
// R14-R16 are three distinct cutover rules; repeating a NAME is not what breaks the report.
69+
assert.deepEqual(
70+
collisionsIn([
71+
'a.ts',
72+
"const rules = ['R14 logs-runtime-cutover', 'R15 logs-runtime-cutover'];",
73+
]),
74+
[],
75+
);
76+
});
77+
78+
test('declarations are found in every syntactic form, not just `const RULE =`', () => {
79+
const declarations = ruleDeclarations(
80+
sources([
81+
'mixed.ts',
82+
[
83+
"const RULE = 'R2 commands-floor';",
84+
'const inline = { rule: "R3 platforms-seam" };',
85+
"const nested = [{ rule: 'R4 value-import-cycle' }];",
86+
"function emit() { return { rule: 'R5 zero-back-edges' }; }",
87+
].join('\n'),
88+
]),
89+
);
90+
91+
assert.deepEqual(
92+
declarations.map(({ number, name }) => `R${number} ${name}`),
93+
['R2 commands-floor', 'R3 platforms-seam', 'R4 value-import-cycle', 'R5 zero-back-edges'],
94+
);
95+
});
96+
97+
test('summary prose that merely opens with a number is not a declaration', () => {
98+
// `R13 holds six private ...` and `R17 holds the devices command ...` are OK-line prose.
99+
assert.deepEqual(
100+
ruleDeclarations(
101+
sources([
102+
'summaries.ts',
103+
[
104+
"const a = 'R13 holds six private implementation-lazy platform packages';",
105+
"const b = 'R17 holds the devices command to one gateway-owned inventory route';",
106+
"const c = 'R11 holds 17 workspace packages behind 39 exported subpaths';",
107+
].join('\n'),
108+
]),
109+
),
110+
[],
111+
);
112+
});
113+
114+
test('rule numbers named only in comments are not declarations', () => {
115+
assert.deepEqual(
116+
ruleDeclarations(
117+
sources([
118+
'commented.ts',
119+
[
120+
'// R11 package-boundaries owns these physical seams, and R13 owns the substrate.',
121+
"/* superseded: 'R13 device-inventory-cutover' moved to R17 */",
122+
"const RULE = 'R17 device-inventory-cutover';",
123+
].join('\n'),
124+
]),
125+
).map(({ name }) => name),
126+
['device-inventory-cutover'],
127+
);
128+
});
129+
130+
test('the real guard tree declares a one-to-one rule namespace', () => {
131+
const declarations = ruleDeclarations(layeringPolicySources(policyDir));
132+
133+
assert.deepEqual(ruleNamespaceCollisions(declarations), []);
134+
assert.ok(declarations.length > 0, 'expected the catalog to find real rule declarations');
135+
});
136+
137+
test('the real tree still declares both renumbered rules under their own numbers', () => {
138+
const byName = new Map(
139+
ruleDeclarations(layeringPolicySources(policyDir)).map(({ name, number }) => [name, number]),
140+
);
141+
142+
assert.equal(byName.get('package-boundaries'), 11);
143+
assert.equal(byName.get('platform-package-substrate'), 13);
144+
assert.equal(byName.get('device-inventory-cutover'), 17);
145+
assert.equal(byName.get('contracts-implementation-authority'), 18);
146+
});
147+
148+
test('the catalog reads the guard sources and skips its own test fixtures', () => {
149+
const scanned = layeringPolicySources(policyDir).map(({ path: file }) => path.basename(file));
150+
151+
assert.ok(scanned.includes('package-boundaries.ts'), 'expected policy sources to be scanned');
152+
assert.deepEqual(
153+
scanned.filter((file) => file.endsWith('.test.ts')),
154+
[],
155+
'test fixtures declare rule strings as data and must not enter the catalog',
156+
);
157+
});
158+
159+
test('the summary names the span the catalog actually found', () => {
160+
assert.equal(
161+
ruleCatalogSummary(
162+
ruleDeclarations(
163+
sources([
164+
'a.ts',
165+
"const rules = ['R2 commands-floor', 'R18 contracts-implementation-authority'];",
166+
]),
167+
),
168+
),
169+
"the guard's own 2 rule ids each name exactly one rule (R2-R18)",
170+
);
171+
});

scripts/layering/rule-catalog.ts

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,94 @@
1+
// The guard's own rule namespace, derived from the declarations it actually emits.
2+
//
3+
// Every rule identifier the report groups by has the shape `R<number> <name>`. Nothing
4+
// used to hold one number to one name: R11 named both package-boundaries and
5+
// contracts-implementation-authority, and R13 named both platform-package-substrate and
6+
// device-inventory-cutover, so `Layering drift (R11)` could not be traced to a rule. This
7+
// derives the catalog from the policy sources rather than restating it in a list that can
8+
// drift out of step with them, and check.ts refuses to report through a colliding namespace.
9+
10+
import fs from 'node:fs';
11+
import path from 'node:path';
12+
import { parseSync } from 'oxc-parser';
13+
import { visitAst } from './record-runtime-policy-ast.ts';
14+
15+
/** A rule identifier exactly as the report emits it: a number and one kebab-case name. */
16+
const RULE_IDENTIFIER = /^R(\d+) ([a-z0-9]+(?:-[a-z0-9]+)*)$/;
17+
18+
export type RuleDeclaration = Readonly<{ number: number; name: string; file: string }>;
19+
export type RuleNamespaceCollision = Readonly<{ number: number; names: readonly string[] }>;
20+
export type LayeringPolicySource = Readonly<{ path: string; source: string }>;
21+
22+
/**
23+
* Every rule identifier declared as a string literal in the guard's own sources, in any
24+
* syntactic form. Matching literals rather than `const RULE =` and `rule:` call sites keeps
25+
* a new declaration form from being invisible here, and the anchored pattern keeps summary
26+
* prose that merely opens with a number (`R13 holds six private ...`) out of the catalog.
27+
*/
28+
export function ruleDeclarations(
29+
sources: readonly LayeringPolicySource[],
30+
): readonly RuleDeclaration[] {
31+
const declarations: RuleDeclaration[] = [];
32+
const seen = new Set<string>();
33+
for (const file of sources) {
34+
const parsed = parseSync(file.path, file.source);
35+
visitAst(parsed.program.body, (node) => {
36+
if (node['type'] !== 'Literal' || typeof node['value'] !== 'string') return;
37+
const match = RULE_IDENTIFIER.exec(node['value']);
38+
if (!match) return;
39+
const key = `${file.path} ${node['value']}`;
40+
if (seen.has(key)) return;
41+
seen.add(key);
42+
declarations.push({ number: Number(match[1]), name: String(match[2]), file: file.path });
43+
});
44+
}
45+
return declarations;
46+
}
47+
48+
/** Numbers answering to more than one rule name — the drift this catalog exists to reject. */
49+
export function ruleNamespaceCollisions(
50+
declarations: readonly RuleDeclaration[],
51+
): readonly RuleNamespaceCollision[] {
52+
const namesByNumber = new Map<number, Set<string>>();
53+
for (const declaration of declarations) {
54+
const names = namesByNumber.get(declaration.number) ?? new Set<string>();
55+
names.add(declaration.name);
56+
namesByNumber.set(declaration.number, names);
57+
}
58+
return [...namesByNumber]
59+
.filter(([, names]) => names.size > 1)
60+
.map(([number, names]) => ({ number, names: [...names].sort() }))
61+
.sort((left, right) => left.number - right.number);
62+
}
63+
64+
/** The guard's non-test sources. Tests declare rule strings as fixtures, not as declarations. */
65+
export function layeringPolicySources(directory: string): readonly LayeringPolicySource[] {
66+
return fs
67+
.readdirSync(directory)
68+
.filter((entry) => entry.endsWith('.ts') && !entry.endsWith('.test.ts'))
69+
.sort()
70+
.map((entry) => ({
71+
path: path.join(directory, entry),
72+
source: fs.readFileSync(path.join(directory, entry), 'utf8'),
73+
}));
74+
}
75+
76+
export function ruleCatalogSummary(declarations: readonly RuleDeclaration[]): string {
77+
const numbers = [...new Set(declarations.map(({ number }) => number))].sort(
78+
(left, right) => left - right,
79+
);
80+
const first = numbers.at(0);
81+
const last = numbers.at(-1);
82+
return (
83+
`the guard's own ${numbers.length} rule ids each name exactly one rule ` +
84+
`(R${first ?? 0}-R${last ?? 0})`
85+
);
86+
}
87+
88+
export function ruleNamespaceCollisionReport(
89+
collisions: readonly RuleNamespaceCollision[],
90+
): string {
91+
return collisions
92+
.map(({ number, names }) => ` R${number} names ${names.length} rules: ${names.join(', ')}`)
93+
.join('\n');
94+
}

0 commit comments

Comments
 (0)