From d3c2f127dac9bb2a6e427cf5e22296509699a8b2 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 08:50:23 +0000 Subject: [PATCH 1/3] feat(scripts): dependency-graph report over the layering gate's model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reports what the layering gate deliberately does not enforce, as JSON plus a short summary. No renderer: the productive artifact is the JSON. pnpm depgraph # -> .tmp/depgraph/graph.json + summary pnpm depgraph:test Dependency graph: 898 files, 4627 edges, 25 zones value-import cycles (R4): 0 type-only/dynamic cycles (not gate-rejected): 8 spine back-edges (R5): 0 type-only spine inversions (R6): 42 transitively redundant value edges: 1338 The two numbers worth having are the ones CI cannot give you. Transitively redundant value edges — where the target is still reachable at distance >= 2, so the direct import changes nothing about what the module can see — need a real reachability pass, not a grep. And cycle detection over type-only and dynamic edges covers the loops R4 excludes by design. Both are candidate lists, never work lists; at ~1300 the redundancy set is a place to look. It reuses scripts/layering/model.ts, the same module check.ts uses in CI, so the file set, zone partition, edge kinds and cycle definition are the enforced ones. A second extractor would describe a graph nobody gates. Consequence worth having: its R6 count reproduces TYPE_INVERSION_BASELINE, so a mismatch means one of the two is stale. This is the analysis half of a viewer that was built and dropped. The render cost ~2200 lines and needed a Fallow exemption for a 920-line canvas file, and nobody read it. Everything here clears the repo's bar with NO exemption — scripts/depgraph is deliberately absent from ignorePatterns, unlike scripts/layering, scripts/perf and scripts/maestro-conformance. Getting there meant fixing rather than suppressing: extracted `valueSuccessors` (the value-edge adjacency was built identically in two places — a real clone), split `buildGraph` into four named aggregation steps, split `reachableBeyondDirectEdge` out of `markRedundantEdges`, extracted `compareZoneEdges`/`crossedZonePair`, extracted `edgeKindCode`/`edgeFlags` from a nested ternary scoring CRAP 42, and deleted `fileGroup` plus the `group` node field once the cluster layout went. Two additive exports on scripts/layering/model.ts: `zoneRank` and `targetDagZone` (previously module-private). The gate's behaviour is unchanged. `pnpm check` green, 4488 unit tests, 5 model tests. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur --- package.json | 2 + scripts/depgraph/README.md | 95 +++++++++++++++++++++ scripts/depgraph/build.ts | 139 +++++++++++++++++++++++++++++++ scripts/depgraph/model.test.ts | 148 +++++++++++++++++++++++++++++++++ scripts/depgraph/model.ts | Bin 0 -> 11549 bytes scripts/layering/model.ts | 11 ++- 6 files changed, 394 insertions(+), 1 deletion(-) create mode 100644 scripts/depgraph/README.md create mode 100644 scripts/depgraph/build.ts create mode 100644 scripts/depgraph/model.test.ts create mode 100644 scripts/depgraph/model.ts diff --git a/package.json b/package.json index 2a894e10e8..8dc8c9a085 100644 --- a/package.json +++ b/package.json @@ -118,6 +118,8 @@ "check:affected": "node --experimental-strip-types scripts/check-affected/run.ts", "check:affected:test": "node --experimental-strip-types --test scripts/check-affected/model.test.ts scripts/check-affected/run.test.ts", "check:layering": "node --experimental-strip-types --test scripts/layering/model.test.ts && node --experimental-strip-types scripts/layering/check.ts", + "depgraph": "node --experimental-strip-types scripts/depgraph/build.ts", + "depgraph:test": "node --experimental-strip-types --test scripts/depgraph/model.test.ts", "check:production-exports": "fallow dead-code --config fallow-production-exports.json --production --unused-exports --fail-on-issues", "check:bundle-owner-files": "node --experimental-strip-types scripts/check-bundle-owner-files.ts", "check:freerange": "fr", diff --git a/scripts/depgraph/README.md b/scripts/depgraph/README.md new file mode 100644 index 0000000000..5ea8bdfe0f --- /dev/null +++ b/scripts/depgraph/README.md @@ -0,0 +1,95 @@ +# Dependency graph report + +```sh +pnpm depgraph # -> .tmp/depgraph/graph.json + a text summary +pnpm depgraph --out /tmp/graph.json +pnpm depgraph:test +``` + +Emits the dependency graph of every production file under `src/` (tests excluded) as JSON, +plus a short summary of what the layering gate does not enforce. There is no renderer: the +productive artifact is the JSON, queried directly. + +## When to reach for this + +It pays for itself on three questions, and misleads on a fourth. + +**"What am I about to break?"** `nodes[].in` is the dependent count — blast radius. Size the +nodes by dependents and the files you should touch carefully are the big ones. Faster than +grepping, and it counts type-only and dynamic edges that a grep for `from '...'` misses. + +**"Where is the debt actually concentrated?"** Zone-level counts (`zoneEdges`) answer "which +boundary carries the most traffic" in one query. The pass that produced ADR-adjacent findings +started here. + +**"What is wrong that the gate does not enforce?"** This is the part CI cannot give you. The +gate rejects value-import cycles (R4) and spine back-edges (R5); the graph additionally reports: + +- **transitively redundant value edges** — the target is still reachable from the source at + distance >= 2, so the direct import changes nothing about what the module can see. A + _candidate_, not a defect: a direct import is often clearer than a re-export chain. There are + ~1300 of these, so treat it as a place to look, never a work list. +- **type-only and dynamic cycles** — 8 of them, all outside R4 by design (a type-only import is + free at runtime, a dynamic one is a deliberate cold-start seam). Worth reading when a module + feels hard to reason about. + +**Where it misleads: a cluster's size is not its difficulty.** This is worth stating plainly +because it already cost a day. The `commands -> client` cluster looked like the obvious win — 28 +type-only inversions, all pointing at one file. Moving that file down took the gate from **42 to +48**, because the vocabulary it holds *depends on* `commands/`, `metro/`, `core/` and `remote/`; +declaring it in `contracts/` made the foundation depend on the layers above it. The picture shows +you an edge's weight, not whether it can be reversed. + +So: use the render to find a candidate, then answer "can this move?" numerically before planning +anything. The question is always *what does the target itself import, and what rank is that?* + +```sh +pnpm depgraph +# Zone pairs that invert the ranked spine, by type-only edge count. Reproduces the gate's R6 +# breakdown from the JSON alone — if these disagree with TYPE_INVERSION_BASELINE, regenerate. +node -e "const j=require('./.tmp/depgraph/index.json'); + const rank=Object.fromEntries(j.zones.map(z=>[z.id,z.rank])); + j.zoneEdges + .filter(e=>rank[e.from]!=null && rank[e.to]!=null && rank[e.from]({pair:e.from+' -> '+e.to, typeOnly:e.count-e.valueCount})) + .filter(e=>e.typeOnly>0).sort((a,b)=>b.typeOnly-a.typeOnly) + .forEach(e=>console.log(String(e.typeOnly).padStart(4), e.pair));" +``` + +Note `zoneEdges[].backEdge` flags **R5 value** back-edges only, and there are none — filtering on +it returns an empty list, which is the gate passing, not a broken query. + + +## What is authoritative + +`pnpm check:layering` is. This reads the same model, so the numbers should agree — its R6 +count matching `TYPE_INVERSION_BASELINE` is a useful self-check — but if they ever diverge, the +gate is right and the graph is stale. Nothing here runs in CI, and nothing here should gate a +merge: it is an instrument, not a rule. + +## Why it reuses the layering gate + +The graph is extracted with `scripts/layering/model.ts`, the same module +`scripts/layering/check.ts` uses in CI. File set, zone partition, edge kinds +(value / type-only / dynamic), and cycle definition are therefore identical to the rules +the gate enforces — a separate extractor with its own resolution behaviour would draw a +graph nobody is enforcing. Cross-checked once against `dependency-cruiser` 3.1.1 (at the commit it was written): same +modules and edges, plus 88 dynamic/type-only edges dependency-cruiser fails to resolve. + +## What the JSON carries + +- `zones[]` — id, spine `rank` (`null` when intentionally unranked), `classification`, file + count, LOC. +- `zoneEdges[]` — per zone pair: total `count`, `valueCount`, and `backEdge` (R5 value + back-edges only — see the note above). +- `nodes[]` — per file: zone index, LOC, `in`/`out` degree, `lvl` (longest path to a sink over + value edges; R4 guarantees that subgraph is a DAG), and `cyc` (index into `cycles`, or `-1`). +- `edges[]` — index-addressed `[from, to, kind, flags]`. Kind: `0` value, `1` type-only, `2` + dynamic. Flags bitfield: `1` spine back-edge, `2` transitively redundant, `4` type-only + inversion. +- `cycles[]` — each with `kind` (`value` / `type` / `dynamic`) and its node path. + +A "redundant" edge means the target is still reachable from the source at distance >= 2 over +value edges, so removing the direct import would not change what the module can see. That makes +it a _candidate_, not a defect: plenty of direct imports are clearer than relying on a re-export +chain. diff --git a/scripts/depgraph/build.ts b/scripts/depgraph/build.ts new file mode 100644 index 0000000000..49a4579d23 --- /dev/null +++ b/scripts/depgraph/build.ts @@ -0,0 +1,139 @@ +// Dependency-graph report — the numbers the layering gate does not enforce. +// +// node --experimental-strip-types scripts/depgraph/build.ts [--out ] +// +// Emits a JSON graph plus a short text summary. It reuses scripts/layering/model.ts, the same +// module check.ts uses in CI, so the file set, zone partition, edge kinds and cycle definition +// are the ones actually enforced — a second extractor would describe a graph nobody gates. +// +// What it adds over `pnpm check:layering`: transitively redundant value edges, cycles that are +// deliberately outside R4 (type-only and dynamic), per-zone size, and per-file fan-in/fan-out. +// See README.md for which question each field answers. + +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import path from 'node:path'; +import { pathToFileURL } from 'node:url'; +import { listSourceFiles } from '../layering/check.ts'; +import { resolveImportEdges, zoneRank } from '../layering/model.ts'; +import { buildGraph, computeLevels, type GraphData } from './model.ts'; + +const repoRoot = execFileSync('git', ['rev-parse', '--show-toplevel'], { + encoding: 'utf8', +}).trim(); + +/** Compact wire form. Nodes and edges are index-addressed to keep the payload small. */ +type Payload = { + generated: { commit: string; files: number; edges: number }; + zones: { id: string; rank: number | null; classification: string; files: number; loc: number }[]; + zoneEdges: GraphData['zoneEdges']; + nodes: { + id: string; + z: number; + loc: number; + in: number; + out: number; + lvl: number; + cyc: number; + }[]; +/** + * `[fromIndex, toIndex, kind, flags]`; kind 0=value 1=type 2=dynamic, + * flags bit0=R5 back-edge, bit1=transitively redundant, bit2=R6 type inversion. + */ + edges: [number, number, number, number][]; + cycles: { kind: string; path: number[] }[]; +}; + +function headCommit(): string { + try { + return execFileSync('git', ['rev-parse', '--short', 'HEAD'], { + cwd: repoRoot, + encoding: 'utf8', + }).trim(); + } catch { + return 'unknown'; + } +} + +const EDGE_KIND_CODES = { value: 0, type: 1, dynamic: 2 } as const; + +/** Wire code for an edge kind, so the payload carries a number rather than a string per edge. */ +function edgeKindCode(kind: GraphData['edges'][number]['kind']): number { + return EDGE_KIND_CODES[kind]; +} + +/** Bitfield: 1 = spine back-edge (R5), 2 = transitively redundant, 4 = type-only inversion (R6). */ +function edgeFlags(edge: GraphData['edges'][number]): number { + return (edge.backEdge ? 1 : 0) | (edge.redundant ? 2 : 0) | (edge.typeInversion ? 4 : 0); +} + +function buildPayload(): Payload { + const files = listSourceFiles(); + const sources = new Map( + files.map((file) => [file, fs.readFileSync(path.join(repoRoot, file), 'utf8')]), + ); + const resolved = resolveImportEdges(sources); + const graph = buildGraph(sources, resolved); + const levels = computeLevels(graph.nodes, graph.edges); + + const zoneIndex = new Map(graph.zones.map((zone, index) => [zone.id, index])); + const nodeIndex = new Map(graph.nodes.map((node, index) => [node.id, index])); + + return { + generated: { commit: headCommit(), files: graph.nodes.length, edges: graph.edges.length }, + zones: graph.zones.map((zone) => ({ ...zone, rank: zoneRank(zone.id) })), + zoneEdges: graph.zoneEdges, + nodes: graph.nodes.map((node) => ({ + id: node.id.replace(/^src\//, ''), + z: zoneIndex.get(node.zone)!, + loc: node.loc, + in: node.fanIn, + out: node.fanOut, + lvl: levels.get(node.id) ?? 0, + cyc: node.cycle, + })), + edges: graph.edges.map((edge) => [ + nodeIndex.get(edge.from)!, + nodeIndex.get(edge.to)!, + edgeKindCode(edge.kind), + edgeFlags(edge), + ]), + cycles: graph.cycles.map((cycle) => ({ + kind: cycle.kind, + path: cycle.path.map((file) => nodeIndex.get(file)!), + })), + }; +} + +function main(argv: readonly string[]): number { + const outFlag = argv.indexOf('--out'); + const jsonPath = + outFlag >= 0 && argv[outFlag + 1] + ? path.resolve(argv[outFlag + 1]!) + : path.join(repoRoot, '.tmp/depgraph/graph.json'); + + const payload = buildPayload(); + fs.mkdirSync(path.dirname(jsonPath), { recursive: true }); + fs.writeFileSync(jsonPath, `${JSON.stringify(payload, null, 2)}\n`); + + const valueCycles = payload.cycles.filter((cycle) => cycle.kind === 'value').length; + const otherCycles = payload.cycles.length - valueCycles; + const backEdges = payload.edges.filter(([, , , flags]) => flags & 1).length; + const redundant = payload.edges.filter(([, , , flags]) => flags & 2).length; + const typeInversions = payload.edges.filter(([, , , flags]) => flags & 4).length; + process.stdout.write( + `Dependency graph: ${payload.generated.files} files, ${payload.generated.edges} edges, ` + + `${payload.zones.length} zones\n` + + ` value-import cycles (R4): ${valueCycles}\n` + + ` type-only/dynamic cycles (not gate-rejected): ${otherCycles}\n` + + ` spine back-edges (R5): ${backEdges}\n` + + ` type-only spine inversions (R6): ${typeInversions}\n` + + ` transitively redundant value edges: ${redundant}\n` + + ` wrote ${path.relative(repoRoot, jsonPath)}\n`, + ); + return 0; +} + +if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) { + process.exit(main(process.argv.slice(2))); +} diff --git a/scripts/depgraph/model.test.ts b/scripts/depgraph/model.test.ts new file mode 100644 index 0000000000..9e185fee1a --- /dev/null +++ b/scripts/depgraph/model.test.ts @@ -0,0 +1,148 @@ +import assert from 'node:assert/strict'; +import { test } from 'node:test'; +import { resolveImportEdges } from '../layering/model.ts'; +import { buildGraph, collapseEdges, collectCycles, markRedundantEdges } from './model.ts'; + +function sources(entries: Record): Map { + return new Map(Object.entries(entries)); +} + +test('collapseEdges keeps one edge per pair at the strongest kind', () => { + const edges = resolveImportEdges( + sources({ + 'src/core/a.ts': [ + "import type { Shape } from './b.ts';", + "import { run } from './b.ts';", + "import type { Other } from './c.ts';", + "void import('./d.ts');", + ].join('\n'), + 'src/core/b.ts': 'export const run = 1;', + 'src/core/c.ts': 'export type Other = string;', + 'src/core/d.ts': 'export const lazy = 1;', + }), + ); + + assert.deepEqual( + collapseEdges(edges).map((edge) => ({ to: edge.to, kind: edge.kind })), + [ + { to: 'src/core/b.ts', kind: 'value' }, + { to: 'src/core/c.ts', kind: 'type' }, + { to: 'src/core/d.ts', kind: 'dynamic' }, + ], + ); +}); + +test('redundant marks only value edges whose target is already reachable at distance >= 2', () => { + const edges = collapseEdges( + resolveImportEdges( + sources({ + // a -> b -> c makes the direct a -> c edge removable; a -> d is the only route to d. + 'src/core/a.ts': [ + "import { b } from './b.ts';", + "import { c } from './c.ts';", + "import { d } from './d.ts';", + ].join('\n'), + 'src/core/b.ts': "export { c as b } from './c.ts';", + 'src/core/c.ts': 'export const c = 1;', + 'src/core/d.ts': 'export const d = 1;', + }), + ), + ); + markRedundantEdges(edges); + + const flagged = edges + .filter((edge) => edge.redundant) + .map((edge) => `${edge.from} -> ${edge.to}`); + assert.deepEqual(flagged, ['src/core/a.ts -> src/core/c.ts']); +}); + +test('a type-only shortcut is never treated as redundant against a value path', () => { + const edges = collapseEdges( + resolveImportEdges( + sources({ + 'src/core/a.ts': ["import { b } from './b.ts';", "import type { C } from './c.ts';"].join( + '\n', + ), + 'src/core/b.ts': "export { c as b } from './c.ts';", + 'src/core/c.ts': 'export type C = string;\nexport const c = 1;', + }), + ), + ); + markRedundantEdges(edges); + + assert.deepEqual( + edges.filter((edge) => edge.redundant), + [], + ); +}); + +test('collectCycles separates gate-rejected value cycles from type-only and dynamic loops', () => { + const valueCycle = collectCycles( + resolveImportEdges( + sources({ + 'src/core/a.ts': "import { b } from './b.ts';\nexport const a = 1;", + 'src/core/b.ts': "import { a } from './a.ts';\nexport const b = 1;", + }), + ), + ); + assert.deepEqual( + valueCycle.map((cycle) => cycle.kind), + ['value'], + ); + + const typeCycle = collectCycles( + resolveImportEdges( + sources({ + 'src/core/a.ts': "import type { B } from './b.ts';\nexport type A = B;", + 'src/core/b.ts': "import type { A } from './a.ts';\nexport type B = A | null;", + }), + ), + ); + assert.deepEqual( + typeCycle.map((cycle) => cycle.kind), + ['type'], + ); + + const dynamicCycle = collectCycles( + resolveImportEdges( + sources({ + 'src/core/a.ts': "export const a = () => import('./b.ts');", + 'src/core/b.ts': "export const b = () => import('./a.ts');", + }), + ), + ); + assert.deepEqual( + dynamicCycle.map((cycle) => cycle.kind), + ['dynamic'], + ); +}); + +test('buildGraph reports zone membership, degrees, and cross-zone edge counts', () => { + const files = sources({ + 'src/kernel/errors.ts': 'export const fail = 1;\n', + 'src/core/interactors/tap.ts': "import { fail } from '../../kernel/errors.ts';\n", + 'src/commands/tap.ts': [ + "import { fail } from '../kernel/errors.ts';", + "import '../core/interactors/tap.ts';", + ].join('\n'), + }); + const graph = buildGraph(files, resolveImportEdges(files)); + + const kernel = graph.nodes.find((node) => node.id === 'src/kernel/errors.ts')!; + assert.equal(kernel.zone, 'kernel'); + assert.equal(kernel.fanIn, 2); + assert.equal(kernel.fanOut, 0); + + // A non-root zone member resolves to its folder, not to `(root)`. + const interactor = graph.nodes.find((node) => node.id === 'src/core/interactors/tap.ts')!; + assert.equal(interactor.zone, 'core'); + + assert.deepEqual( + graph.zoneEdges.map((edge) => `${edge.from} -> ${edge.to} (${edge.count})`), + ['commands -> core (1)', 'commands -> kernel (1)', 'core -> kernel (1)'], + ); + assert.deepEqual( + graph.zones.map((zone) => zone.classification), + ['ranked', 'ranked', 'ranked'], + ); +}); diff --git a/scripts/depgraph/model.ts b/scripts/depgraph/model.ts new file mode 100644 index 0000000000000000000000000000000000000000..cc782d29cc5895d2ddfb868bca9e55faea13b32f GIT binary patch literal 11549 zcmc&)?Q+}J5zTKs#kHqXkv0X{nY2IXNSXK}sV5)K*lsdYRWpQuOA-+Tu(*I^M%GLp zqEFZ-={dXi0+$qJJME8p>@f-6&)u`TXLoUUXr9}Zt#Vss>w`s;u9haP(sJzzXD;g8 zmge98{FhmEjWzSG%G#o?oT)EuW7?%PWxBRaQ7z0OZSB4@HeXnsmh2rK;;(s&QMxP^ z#B#-%Z7{Z6<4aRq+T6^W`ohdy))cGO9flPTW%HzUGlTI<*)Oe3$~D_%(c}lKv}xDI zzH6H_YwKnz2lD5b7o|1Mwq~?y>b#Rh@Vfus@sN-+c?bL*w_TUtQVamE%z*1K*Sb;aN zC=;)2u~@bWGJp5R=3SMiRXa6jbzR!Dda`%31prayk8na>l?7BaD3U^rzf)1)6pd?5 zdsXWhoLyYl3RXlPmJ-xB9qiq;!Jp7vxX{FOj=rdF=+o-1r8!*|?b;u=tg{#t^R#+d zZ9RC^;VzrFc?rMz&Ja+ziWcaf}Lz(jQdS{!k)EKIcvLLNqm^t?vO%H`2%ub{>a4#gCQn zLG(=y*L_UE6RRUWg$o19j?AMc{^OTE#3ZGy7VXkMe)w=K3mH?)jo<1-=AN3PBly_A zSNmh#;^9MXaD#u1ZM&w@r#N_YYzmg%dBhLX9nK!dr%c3yUt=lm%CSiZoMG|(!IMX$ z=%ZdC&W`(G;`wa0Daz zXz+o0EFNxR&aH(wu|ECHuU3vjSNl$3qcCBhJ5bDLn* z!w^OzjFn}&a^gaqXaP_qHzAYRp}Uh)XiqGODm*zA;K=5bhM@s8^fV~jOXAU`EsBl`_;j4x%$N0bs#(a zZAjSohny!8MtIDn!)K*EGKe3;<0wW%j8PB-#_U($WD^_Yg1X%t9R>b5h=kwiMkEw@ zoRcI;{4K>2+-Q87IMCOT>hp{x|6=QRBZ1_E)At&(9X=$I;7vS-BBqQX9v3dVq zhqd+2L2v-P5>5g?=N-z=WMLqbFK~va=)lAg&H%BBbfwKt`^jTXU#^NZk}m@*6+S%r7tB{^iwQ-y$3G1jyvFo2X-Aio_-+ZK;$Twv<+= zWAtbvG7)338?^7NMO8)X7{njE+rhG4AQ*3M3~3jjGHHC!2uc6d9@>qI01; zzBuCq!TumzB$(9_DH?eiTqBA==bH4X5eO^~2bSHZtG8qbmrnsie1}nBFme(dB}zNJ z;A9W}(I-$bXe`PFN@`F&@TBH=bkfWERCJ|$<{zC!vV8Iz=MggNt~4S7eAs2{{4(9=827v<4s=bazDyjr z8Ys9Y_b82ds-^?w;2_R=@a<3leQjM#sfnInJU(|4;b>wLQ4OIXu`8EE%~v@APD&Pd zI74Uv5J~zgsTqQk+w>f8mjSeDm!RN-|9{@DoXu!486BZw*_SC1?p;4pK}d!~vR_q#3zw;UM5kUDwRPS#(d$Ma7h~K)6wi4b?(T*~vI~ zWUyJp9!<*;0T!qX?{?g>xVbL`#-ICi6Q1qt}mIgiQI)t~rC55}b-wq-7a7ndksRxK-Fmon$*FlO_&>>G!q<`}9uA+sX{DC5rR{kfH`+``X5sGsBrTpmR z*Se@i`=9OOH)TVXVT_)cRg?dS2^Ni@I{J6)N?E9MiY7!y+R5s@@H8t z>fGsK(i6+=qG4{FW?6U9l}JE*2a(Z_5rkzfbkg8_G~L_4pX9^rC^(i8wA>0E-A(Y0 z$pyuNBeX`@0)LKfvU@47E@6tX8f6`xCe}RfADsN|!I9cEoSdk9^g5^XtNv!eXUdw9 zTCQ_ZqZ&k=;>mJBYkw39uEFH5sn^LGRvw@hFD>Th4a#k`a??O=_K%GdkrdE>M zG;3;9lbWsqqulVHTGa|o1}9~(q3PfYV#5RY83mt6%ezReplM-DbZHK>E~a1JzZdO& zT0w0XdUsI`V+Jq$7G_GAsb~VbmiwZ?%$C@UJIacs3j#tYLf@)y8Jip5qmAOz2e9{5!ow1Q3e!9AHw+6H zl!grYqkNqJ-VB9HPDLqfrnJ( zF%hNApg?5D0n9(yMq%mj8Vm}y3|{cO;&i2OPGWFhOG`agK~>L+#eKX#W3N5{SyUSs zQ9kBJ;4;Liu)BY<&pWvN&6Zh;cMvphi$gk$6M+xQtB-!N*;>90QXYoUT@IB5356i9 zC=1YGj;5iIqeZY1Vf#IS8HwhPv>6AML+h93nG1^6M;K-vYTRVsM3JQHlj-&A!x*^4 zF1^a=Q`o*Snna%zxgUP&^G^8@fwu2R`B#$qc6|Jb(x~7ITU-tdL1h>nKl~#1>+cYl z#aNFay`)=hB#nx^v5$%Ps95uFCiSU|P)Sky|3W2q6&=L4^?-^Z%|LJAjy%;B_YhSJ zcGS4T&|Ea5FcBg~O!CLAqDwSPkcLqaZ}!xZzQ>boPxGJM&VGE+Bg7&Ryev7@Tu$g( zz_Cx}ZmC3wNRjVs*#QO~2XtYuq~TDqnb_jgY{VgN)P7#?akfUOT;;IQRsAHD9s$({ zRaVQs_pb@`rEv++l527y24Qq8it7AUOGLUgLrY)kb6Sa$3SX28iL?&qCyVOL?N)qIWOz0&_96bXXken{MaA0-?Ld7GXvD>^~D4+ zf*&C|2K?keoh7n<8x|GL7)<$Yk(@0m;oVX4$7JoehOmU!9Ce8wQ1gTHe>dXyiBW0zM#e+) z*k6r=!0^3RL1Kb8hEdFpk3_5plVAklQhuU$GJL|b@*S&<>`^Q*1kZ85J%gjauwOwn HPk!+q4N>~| literal 0 HcmV?d00001 diff --git a/scripts/layering/model.ts b/scripts/layering/model.ts index e0fe1dc4f0..da5f380b5c 100644 --- a/scripts/layering/model.ts +++ b/scripts/layering/model.ts @@ -52,6 +52,15 @@ const TARGET_DAG_RANK = new Map([ export const RANKED_ZONES: ReadonlySet = new Set(TARGET_DAG_RANK.keys()); +/** + * Spine rank of a zone, or `null` when the zone is intentionally unranked. The gate compares + * ranks internally; this is exported for the dependency-graph report, which records the rank per + * zone so a consumer can tell an inversion from an ordinary edge without re-deriving the spine. + */ +export function zoneRank(zone: string): number | null { + return TARGET_DAG_RANK.get(zone) ?? null; +} + // The one zone deliberately left OUT of the ranked spine. It is NOT unenforced: every file // in it is still subject to the global production value-import cycle rejection (R4) and the // R1-R3 move rules. It opts out of spine back-edge ranking because `(root)` holds the @@ -144,7 +153,7 @@ export function topFolder(file: string): string { return match ? match[1]! : '(root)'; } -function targetDagZone(file: string): string { +export function targetDagZone(file: string): string { if (file.startsWith('src/daemon/client/')) return 'daemon-client'; if (file.startsWith('src/daemon/')) return 'daemon-server'; return topFolder(file); From e1271cac31ddbeb31386ddfbaf49ec2f301a0d2d Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 09:24:29 +0000 Subject: [PATCH 2/3] ci(layering): assert the depgraph report reproduces the gate's baseline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The report reads the same model as the gate, so its inversion count must equal TYPE_INVERSION_BASELINE. That agreement was previously a nice property nobody checked; the Layering Guard job now runs scripts/depgraph/model.test.ts, so the two cannot be green independently. Verified by bumping a baseline entry by one and confirming the job fails with a message naming the fix. The count feeding the check is computed by `typeInversionsByPair`, which applies the gate's rule — once per FILE pair, over the raw resolved edges — rather than reading the collapsed edge list. That matters: `collapseEdges` keeps one edge per pair with the strongest kind winning, and `dynamic` outranks `type`, so a module imported both lazily and for its types would collapse to `dynamic` and drop out of the count. No such pair exists today (measured: 0 of 42 inverting pairs), but a number wired into a CI equality check must not be able to drift for a reason unrelated to layering. Stated honestly in the README and the test: this is a cross-check of the report's extraction and the baseline against the real tree, not two independent algorithms. The gate remains the authority — if they disagree, the baseline or the tree is wrong, never the test. TYPE_INVERSION_BASELINE is now exported for this purpose. Not done here, deliberately: the ~1338 transitively redundant value edges are a candidate for a loose growth-only ratchet later. They are a candidate list, not a work list, and a hard count would be noise. `pnpm check` green, 4488 unit tests, 6 depgraph model tests. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur --- .github/workflows/ci.yml | 7 +++++++ scripts/depgraph/README.md | 18 ++++++++++++++++ scripts/depgraph/build.ts | 5 ++++- scripts/depgraph/model.test.ts | 37 ++++++++++++++++++++++++++++++++- scripts/depgraph/model.ts | Bin 11549 -> 12824 bytes scripts/layering/check.ts | 4 +++- 6 files changed, 68 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 20b31958ce..4644872723 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -113,6 +113,13 @@ jobs: node --experimental-strip-types --test scripts/layering/model.test.ts node --experimental-strip-types scripts/layering/check.ts + - name: Check the depgraph report agrees with the gate + # scripts/depgraph reads the same model as the gate, so its inversion count must + # reproduce TYPE_INVERSION_BASELINE. Free two-sources check: if the tree changes + # and only one side is updated, this fails and names the difference. Runs here + # rather than in its own job so the two can never be green independently. + run: node --experimental-strip-types --test scripts/depgraph/model.test.ts + affected-selector: name: Affected-check Selector runs-on: ubuntu-latest diff --git a/scripts/depgraph/README.md b/scripts/depgraph/README.md index 5ea8bdfe0f..1c8bfff96e 100644 --- a/scripts/depgraph/README.md +++ b/scripts/depgraph/README.md @@ -62,6 +62,24 @@ it returns an empty list, which is the gate passing, not a broken query. ## What is authoritative +`pnpm check:layering` is. The viewer reads the same model, so the numbers should agree — and that +agreement is now enforced rather than hoped for: the **Layering Guard job runs +`scripts/depgraph/model.test.ts`**, whose last test asserts this report's inversion count reproduces +`TYPE_INVERSION_BASELINE`. If the tree changes and only one side is updated, CI fails and names the +difference. The two cannot be green independently. + +What that check proves precisely: the report's graph build, over the real tree, agrees with the +gate's baseline. It is a cross-check of the extraction and the baseline against reality, not two +independent algorithms — `typeInversionsByPair` deliberately applies the gate's counting rule (once +per file pair, over raw edges) so the numbers cannot diverge for a reason unrelated to layering. In +particular it does NOT count from the collapsed edge list, where `dynamic` outranks `type` and a +module imported both lazily and for its types would drop out. + +If they ever disagree, the gate is right and the baseline or the tree is wrong. + +### Notes + + `pnpm check:layering` is. This reads the same model, so the numbers should agree — its R6 count matching `TYPE_INVERSION_BASELINE` is a useful self-check — but if they ever diverge, the gate is right and the graph is stale. Nothing here runs in CI, and nothing here should gate a diff --git a/scripts/depgraph/build.ts b/scripts/depgraph/build.ts index 49a4579d23..4242be1f7d 100644 --- a/scripts/depgraph/build.ts +++ b/scripts/depgraph/build.ts @@ -42,6 +42,8 @@ type Payload = { */ edges: [number, number, number, number][]; cycles: { kind: string; path: number[] }[]; + /** Type-only spine inversions per zone pair, by the gate's counting rule. */ + typeInversions: Record; }; function headCommit(): string { @@ -102,6 +104,7 @@ function buildPayload(): Payload { kind: cycle.kind, path: cycle.path.map((file) => nodeIndex.get(file)!), })), + typeInversions: graph.typeInversions, }; } @@ -120,7 +123,7 @@ function main(argv: readonly string[]): number { const otherCycles = payload.cycles.length - valueCycles; const backEdges = payload.edges.filter(([, , , flags]) => flags & 1).length; const redundant = payload.edges.filter(([, , , flags]) => flags & 2).length; - const typeInversions = payload.edges.filter(([, , , flags]) => flags & 4).length; + const typeInversions = Object.values(payload.typeInversions).reduce((sum, n) => sum + n, 0); process.stdout.write( `Dependency graph: ${payload.generated.files} files, ${payload.generated.edges} edges, ` + `${payload.zones.length} zones\n` + diff --git a/scripts/depgraph/model.test.ts b/scripts/depgraph/model.test.ts index 9e185fee1a..4a41ede85a 100644 --- a/scripts/depgraph/model.test.ts +++ b/scripts/depgraph/model.test.ts @@ -1,7 +1,15 @@ import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; import { test } from 'node:test'; +import { listSourceFiles, TYPE_INVERSION_BASELINE } from '../layering/check.ts'; import { resolveImportEdges } from '../layering/model.ts'; -import { buildGraph, collapseEdges, collectCycles, markRedundantEdges } from './model.ts'; +import { + buildGraph, + collapseEdges, + collectCycles, + markRedundantEdges, + typeInversionsByPair, +} from './model.ts'; function sources(entries: Record): Map { return new Map(Object.entries(entries)); @@ -146,3 +154,30 @@ test('buildGraph reports zone membership, degrees, and cross-zone edge counts', ['ranked', 'ranked', 'ranked'], ); }); + +// Two-sources-of-truth check, run by the Layering Guard job. +// +// The report and the gate read the same model, so their inversion counts must agree. This locks +// that: if the tree changes and only one side is updated, or if the report's extraction diverges +// from what the gate sees, this fails and names the difference. +// +// What it proves precisely: the report's own graph build, over the real tree, reproduces +// TYPE_INVERSION_BASELINE. It is a cross-check of the extraction and the baseline against reality, +// not two independent algorithms — `typeInversionsByPair` deliberately applies the gate's counting +// rule so the numbers cannot differ for a reason unrelated to layering. The gate stays the +// authority; if these disagree, the baseline or the tree is wrong, never this test. +test("the report's inversion count reproduces the gate's TYPE_INVERSION_BASELINE", () => { + const files = listSourceFiles(); + const sources = new Map(files.map((file) => [file, readFileSync(file, 'utf8')])); + const actual = typeInversionsByPair(resolveImportEdges(sources)); + + assert.deepEqual( + actual, + // Object key order differs between the two literals; compare as sorted entries. + Object.fromEntries( + Object.entries(TYPE_INVERSION_BASELINE).sort(([left], [right]) => left.localeCompare(right)), + ), + 'depgraph and scripts/layering/check.ts disagree about type-only spine inversions. ' + + 'Regenerate with `pnpm depgraph` and update TYPE_INVERSION_BASELINE, or fix the edge.', + ); +}); diff --git a/scripts/depgraph/model.ts b/scripts/depgraph/model.ts index cc782d29cc5895d2ddfb868bca9e55faea13b32f..9312a3532ed0d4f11c1d8f05d4a42c803c2f7b47 100644 GIT binary patch delta 1091 zcmbW0&uSDw5XMo#;DR0^<`Tt%1TwR3CVI_ZG)4%Big-#0Oz(8fZo56*V|PzBld!Ds zFdn@43BtkM9dIq>I+s3ixd+b+}4#y90gm%bn9 zd^RL)JAqdkY?Tdze}E@6Dpk~+)ji*%PkO(Ia2k*!a@sWE?d}79sF)P4hHQx02#Sj4 z^ltVMSf9{|fEy9zE`T!H2D4g0r3^~)o|C|LkgO*A)u8!#YvJD21AA7#p}UNlVx@Pp zJ+}amfXE@Th$ZCLiZmHXFM|9!bv+_E#H!z+o;H{)M7{s2*ed8+^b delta 22 ecmbP{GB;{NDf{Mk>|5nGf6*3W+H9?Nml*(d&> = { +// Exported so scripts/depgraph can assert its own graph build reproduces it — see the +// baseline-parity test there. The gate remains the authority; the report follows. +export const TYPE_INVERSION_BASELINE: Readonly> = { 'commands -> client': 28, 'commands -> daemon-server': 1, 'contracts -> client': 1, From aeecf65648f6ce3b195b73a6ed763b6d59a3f96b Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 11:45:11 +0000 Subject: [PATCH 3/3] fix(depgraph): make the source reviewable, and stop overclaiming removability Four review findings, all of them real. P1 - the implementation was binary. scripts/depgraph/model.ts contained two raw NUL bytes used as map-key delimiters, so Git classified a ~346-line file as binary and hid its entire diff behind `- -`. Replaced with a unicode escape: identical at runtime, textual on disk. I had seen the symptom repeatedly - every grep on that file printed "binary file matches" - and worked around it with python instead of asking why, which is how it survived to review. Guarded repo-wide rather than for this one file: a new test asserts no tracked .ts under src/ or scripts/ contains a raw NUL, verified by reintroducing one and watching it fail. Nothing else would catch a recurrence, and the failure mode is silent - the code works, the review does not. P1 - "transitively redundant" claimed removability it cannot support. Module reachability does not carry bindings: if `a` imports `{ c }` while `b` only re-exports it as `{ c as b }`, the path a -> b -> c exists and deleting a -> c still breaks `a`. The fixture in model.test.ts is exactly that shape and its comment said "removable". Reachability also says nothing about when a module's side effects run. Renamed throughout to what it measures - `transitivelyReachable`, `markTransitivelyReachableEdges`, and a summary line reading "value edges whose target is also reachable at distance >= 2 (reachability only - not a removability claim)". The caveats and the counterexample are now stated in the marker function, the fixture comment and the README, and symbol-level analysis is named as what deciding any individual edge would actually require. P2 - build.ts had no coverage. Every test exercised model.ts, so the CLI could break its output path, wire shape or summary silently. Added three subprocess tests: default path plus summary-agrees-with-payload, `--out` honoured and valid JSON written, and a trailing `--out` falling back rather than crashing (pinned so it is a decision, not an accident). `pnpm depgraph:test` now runs inside `check:tooling`, so `pnpm check` covers it. P2 - README was wrong three ways: it queried `.tmp/depgraph/index.json` after the output moved to `graph.json` (the documented command failed as written), it derived inversions from collapsed `zoneEdges`, which can undercount, and it claimed both that the report runs in CI and that nothing here runs in CI. The query now reads `typeInversions` and was run verbatim; the CI sentence names exactly which single test runs and states that nothing else gates a merge. pnpm check green, 4488 unit tests, 10 depgraph tests. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur --- package.json | 2 +- scripts/depgraph/README.md | 47 ++++++++------- scripts/depgraph/build.ts | 21 +++++-- scripts/depgraph/model.test.ts | 102 ++++++++++++++++++++++++++++++--- scripts/depgraph/model.ts | Bin 12824 -> 13659 bytes 5 files changed, 136 insertions(+), 36 deletions(-) diff --git a/package.json b/package.json index 8dc8c9a085..157b562e07 100644 --- a/package.json +++ b/package.json @@ -127,7 +127,7 @@ "sync:mcp-metadata": "node scripts/sync-mcp-metadata.mjs", "check:mcp-metadata": "node scripts/sync-mcp-metadata.mjs --check", "version": "node scripts/sync-mcp-metadata.mjs && git add server.json", - "check:tooling": "pnpm lint && pnpm typecheck && pnpm check:layering && pnpm check:production-exports && pnpm check:mcp-metadata && pnpm build && pnpm check:bundle-owner-files", + "check:tooling": "pnpm lint && pnpm typecheck && pnpm check:layering && pnpm depgraph:test && pnpm check:production-exports && pnpm check:mcp-metadata && pnpm build && pnpm check:bundle-owner-files", "check:unit": "pnpm test:unit && pnpm test:smoke", "check": "pnpm check:tooling && pnpm check:fallow && pnpm check:unit", "prepack": "pnpm check:mcp-metadata && pnpm build:all && pnpm package:apple-runner:npm && pnpm package:android-snapshot-helper:npm && pnpm package:android-ime-helper:npm", diff --git a/scripts/depgraph/README.md b/scripts/depgraph/README.md index 1c8bfff96e..b3f34f5f78 100644 --- a/scripts/depgraph/README.md +++ b/scripts/depgraph/README.md @@ -25,10 +25,13 @@ started here. **"What is wrong that the gate does not enforce?"** This is the part CI cannot give you. The gate rejects value-import cycles (R4) and spine back-edges (R5); the graph additionally reports: -- **transitively redundant value edges** — the target is still reachable from the source at - distance >= 2, so the direct import changes nothing about what the module can see. A - _candidate_, not a defect: a direct import is often clearer than a re-export chain. There are - ~1300 of these, so treat it as a place to look, never a work list. +- **value edges whose target is also reachable at distance >= 2** — static module reachability, + and *only* that. It is **not** a removability claim, and the obvious reading is wrong: + reachability does not carry bindings (if `a` imports `{ c }` while `b` only re-exports it as + `{ c as b }`, the path exists and deleting `a -> c` still breaks `a`), it does not preserve when + a module's side effects run, and a direct import is often deliberately clearer than reaching + through a barrel. Deciding whether any given edge can go needs symbol-level analysis this does + not attempt. ~1300 of them: a place to look, never a work list. - **type-only and dynamic cycles** — 8 of them, all outside R4 by design (a type-only import is free at runtime, a dynamic one is a deliberate cold-start seam). Worth reading when a module feels hard to reason about. @@ -45,17 +48,18 @@ anything. The question is always *what does the target itself import, and what r ```sh pnpm depgraph -# Zone pairs that invert the ranked spine, by type-only edge count. Reproduces the gate's R6 -# breakdown from the JSON alone — if these disagree with TYPE_INVERSION_BASELINE, regenerate. -node -e "const j=require('./.tmp/depgraph/index.json'); - const rank=Object.fromEntries(j.zones.map(z=>[z.id,z.rank])); - j.zoneEdges - .filter(e=>rank[e.from]!=null && rank[e.to]!=null && rank[e.from]({pair:e.from+' -> '+e.to, typeOnly:e.count-e.valueCount})) - .filter(e=>e.typeOnly>0).sort((a,b)=>b.typeOnly-a.typeOnly) - .forEach(e=>console.log(String(e.typeOnly).padStart(4), e.pair));" +# Zone pairs that invert the ranked spine. Read `typeInversions` rather than deriving it from +# `zoneEdges`: those counts come from the COLLAPSED edge list, where one edge per file pair +# survives and `dynamic` outranks `type`, so a module imported both lazily and for its types +# would drop out. `typeInversions` is counted by the gate's own rule and is what CI compares +# against TYPE_INVERSION_BASELINE. +node -e "const j=require('./.tmp/depgraph/graph.json'); + Object.entries(j.typeInversions) + .sort((a, b) => b[1] - a[1]) + .forEach(([pair, n]) => console.log(String(n).padStart(4), pair));" ``` + Note `zoneEdges[].backEdge` flags **R5 value** back-edges only, and there are none — filtering on it returns an empty list, which is the gate passing, not a broken query. @@ -82,8 +86,12 @@ If they ever disagree, the gate is right and the baseline or the tree is wrong. `pnpm check:layering` is. This reads the same model, so the numbers should agree — its R6 count matching `TYPE_INVERSION_BASELINE` is a useful self-check — but if they ever diverge, the -gate is right and the graph is stale. Nothing here runs in CI, and nothing here should gate a -merge: it is an instrument, not a rule. +gate is right and the graph is stale. + +One thing here DOES run in CI, and only one: the Layering Guard job runs +`scripts/depgraph/model.test.ts`, whose parity test asserts this report's inversion count equals +`TYPE_INVERSION_BASELINE`. Nothing else here gates a merge — the report itself is an instrument, not +a rule, and no finding it produces is enforced. ## Why it reuses the layering gate @@ -103,11 +111,10 @@ modules and edges, plus 88 dynamic/type-only edges dependency-cruiser fails to r - `nodes[]` — per file: zone index, LOC, `in`/`out` degree, `lvl` (longest path to a sink over value edges; R4 guarantees that subgraph is a DAG), and `cyc` (index into `cycles`, or `-1`). - `edges[]` — index-addressed `[from, to, kind, flags]`. Kind: `0` value, `1` type-only, `2` - dynamic. Flags bitfield: `1` spine back-edge, `2` transitively redundant, `4` type-only + dynamic. Flags bitfield: `1` spine back-edge, `2` target also reachable at distance >= 2, `4` type-only inversion. - `cycles[]` — each with `kind` (`value` / `type` / `dynamic`) and its node path. -A "redundant" edge means the target is still reachable from the source at distance >= 2 over -value edges, so removing the direct import would not change what the module can see. That makes -it a _candidate_, not a defect: plenty of direct imports are clearer than relying on a re-export -chain. +Bit `2` means the target is reachable from the source at distance >= 2 over value edges. That is +module reachability, not removability — see the caveats above. Treat it as a question ("why is +this imported directly as well?"), never as an instruction. diff --git a/scripts/depgraph/build.ts b/scripts/depgraph/build.ts index 4242be1f7d..291c28848f 100644 --- a/scripts/depgraph/build.ts +++ b/scripts/depgraph/build.ts @@ -6,7 +6,8 @@ // module check.ts uses in CI, so the file set, zone partition, edge kinds and cycle definition // are the ones actually enforced — a second extractor would describe a graph nobody gates. // -// What it adds over `pnpm check:layering`: transitively redundant value edges, cycles that are +// What it adds over `pnpm check:layering`: value edges whose target is also reachable at distance +// >= 2 (module reachability, NOT a removability claim), cycles that are // deliberately outside R4 (type-only and dynamic), per-zone size, and per-file fan-in/fan-out. // See README.md for which question each field answers. @@ -38,7 +39,7 @@ type Payload = { }[]; /** * `[fromIndex, toIndex, kind, flags]`; kind 0=value 1=type 2=dynamic, - * flags bit0=R5 back-edge, bit1=transitively redundant, bit2=R6 type inversion. + * flags bit0=R5 back-edge, bit1=target also reachable at distance >= 2, bit2=R6 type inversion. */ edges: [number, number, number, number][]; cycles: { kind: string; path: number[] }[]; @@ -64,9 +65,16 @@ function edgeKindCode(kind: GraphData['edges'][number]['kind']): number { return EDGE_KIND_CODES[kind]; } -/** Bitfield: 1 = spine back-edge (R5), 2 = transitively redundant, 4 = type-only inversion (R6). */ +/** + * Bitfield: 1 = spine back-edge (R5), 2 = target also reachable at distance >= 2, 4 = type-only + * inversion (R6). Bit 2 is reachability, NOT removability — see markTransitivelyReachableEdges. + */ function edgeFlags(edge: GraphData['edges'][number]): number { - return (edge.backEdge ? 1 : 0) | (edge.redundant ? 2 : 0) | (edge.typeInversion ? 4 : 0); + return ( + (edge.backEdge ? 1 : 0) | + (edge.transitivelyReachable ? 2 : 0) | + (edge.typeInversion ? 4 : 0) + ); } function buildPayload(): Payload { @@ -122,7 +130,7 @@ function main(argv: readonly string[]): number { const valueCycles = payload.cycles.filter((cycle) => cycle.kind === 'value').length; const otherCycles = payload.cycles.length - valueCycles; const backEdges = payload.edges.filter(([, , , flags]) => flags & 1).length; - const redundant = payload.edges.filter(([, , , flags]) => flags & 2).length; + const transitivelyReachable = payload.edges.filter(([, , , flags]) => flags & 2).length; const typeInversions = Object.values(payload.typeInversions).reduce((sum, n) => sum + n, 0); process.stdout.write( `Dependency graph: ${payload.generated.files} files, ${payload.generated.edges} edges, ` + @@ -131,7 +139,8 @@ function main(argv: readonly string[]): number { ` type-only/dynamic cycles (not gate-rejected): ${otherCycles}\n` + ` spine back-edges (R5): ${backEdges}\n` + ` type-only spine inversions (R6): ${typeInversions}\n` + - ` transitively redundant value edges: ${redundant}\n` + + ` value edges whose target is also reachable at distance >= 2: ${transitivelyReachable}\n` + + ` (reachability only — not a removability claim, see scripts/depgraph/README.md)\n` + ` wrote ${path.relative(repoRoot, jsonPath)}\n`, ); return 0; diff --git a/scripts/depgraph/model.test.ts b/scripts/depgraph/model.test.ts index 4a41ede85a..f9c8d20567 100644 --- a/scripts/depgraph/model.test.ts +++ b/scripts/depgraph/model.test.ts @@ -1,5 +1,8 @@ import assert from 'node:assert/strict'; -import { readFileSync } from 'node:fs'; +import { execFileSync, spawnSync } from 'node:child_process'; +import { existsSync, mkdtempSync, readFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { test } from 'node:test'; import { listSourceFiles, TYPE_INVERSION_BASELINE } from '../layering/check.ts'; import { resolveImportEdges } from '../layering/model.ts'; @@ -7,7 +10,7 @@ import { buildGraph, collapseEdges, collectCycles, - markRedundantEdges, + markTransitivelyReachableEdges, typeInversionsByPair, } from './model.ts'; @@ -40,11 +43,14 @@ test('collapseEdges keeps one edge per pair at the strongest kind', () => { ); }); -test('redundant marks only value edges whose target is already reachable at distance >= 2', () => { +test('flags only value edges whose target is already reachable at distance >= 2', () => { const edges = collapseEdges( resolveImportEdges( sources({ - // a -> b -> c makes the direct a -> c edge removable; a -> d is the only route to d. + // a -> b -> c means c is reachable from a at distance 2, so the direct a -> c edge is + // FLAGGED. Note it is not removable: `b` re-exports c's binding under a different name, + // so deleting a -> c would break a's `c` import. That gap is the point of the rename — + // this measures module reachability, not safe removal. a -> d is the only route to d. 'src/core/a.ts': [ "import { b } from './b.ts';", "import { c } from './c.ts';", @@ -56,15 +62,15 @@ test('redundant marks only value edges whose target is already reachable at dist }), ), ); - markRedundantEdges(edges); + markTransitivelyReachableEdges(edges); const flagged = edges - .filter((edge) => edge.redundant) + .filter((edge) => edge.transitivelyReachable) .map((edge) => `${edge.from} -> ${edge.to}`); assert.deepEqual(flagged, ['src/core/a.ts -> src/core/c.ts']); }); -test('a type-only shortcut is never treated as redundant against a value path', () => { +test('a type-only shortcut is never flagged against a value path', () => { const edges = collapseEdges( resolveImportEdges( sources({ @@ -76,10 +82,10 @@ test('a type-only shortcut is never treated as redundant against a value path', }), ), ); - markRedundantEdges(edges); + markTransitivelyReachableEdges(edges); assert.deepEqual( - edges.filter((edge) => edge.redundant), + edges.filter((edge) => edge.transitivelyReachable), [], ); }); @@ -181,3 +187,81 @@ test("the report's inversion count reproduces the gate's TYPE_INVERSION_BASELINE 'Regenerate with `pnpm depgraph` and update TYPE_INVERSION_BASELINE, or fix the edge.', ); }); + +// A raw NUL byte in a source file makes Git classify it as binary, which hides the whole diff +// behind `- -` and leaves the file unreviewable. This module used a literal NUL as a map-key +// delimiter and shipped that way through a review; it is now the escape sequence, identical at +// runtime and textual on disk. Guarded repo-wide rather than for this one file, because nothing +// else would catch a recurrence and the failure mode is silent: the code works, the review does not. +test('no tracked TypeScript source contains a raw NUL byte', () => { + const tracked = execFileSync('git', ['ls-files', 'src/*.ts', 'src/**/*.ts', 'scripts/**/*.ts'], { + encoding: 'utf8', + }) + .split('\n') + .filter(Boolean); + + const binary = tracked.filter((file) => readFileSync(file).includes(0)); + assert.deepEqual( + binary, + [], + 'these files contain a raw NUL byte, so Git treats them as binary and hides their diff. ' + + 'Use a unicode escape instead of a literal control character.', + ); +}); + +// build.ts had no coverage at all: every test above exercises model.ts, so the CLI could break its +// output path, JSON shape or summary without anything failing. These run it as a subprocess, which +// is the only way to cover argument handling and the file it actually writes. + +function runBuild(args: readonly string[]): { status: number | null; stdout: string; stderr: string } { + const result = spawnSync( + process.execPath, + ['--experimental-strip-types', 'scripts/depgraph/build.ts', ...args], + { encoding: 'utf8' }, + ); + return { status: result.status, stdout: result.stdout ?? '', stderr: result.stderr ?? '' }; +} + +test('build.ts writes the default path and a summary consistent with the JSON', () => { + const { status, stdout } = runBuild([]); + assert.equal(status, 0, stdout); + + const payload = JSON.parse(readFileSync('.tmp/depgraph/graph.json', 'utf8')) as { + generated: { commit: string; files: number; edges: number }; + zones: { id: string; rank: number | null }[]; + nodes: unknown[]; + edges: [number, number, number, number][]; + typeInversions: Record; + }; + + // Wire shape: the fields a consumer queries. A rename here is a breaking change for any script + // following README.md, so it is pinned rather than assumed. + assert.equal(payload.nodes.length, payload.generated.files); + assert.equal(payload.edges.length, payload.generated.edges); + assert.ok(payload.zones.length > 0); + assert.ok(Object.keys(payload.typeInversions).length > 0); + + // The printed summary must agree with the payload it was derived from. + const inversions = Object.values(payload.typeInversions).reduce((sum, n) => sum + n, 0); + assert.match(stdout, new RegExp(`${payload.generated.files} files, ${payload.generated.edges} edges`)); + assert.match(stdout, new RegExp(`type-only spine inversions \\(R6\\): ${inversions}`)); + const reachable = payload.edges.filter(([, , , flags]) => (flags & 2) !== 0).length; + assert.match(stdout, new RegExp(`reachable at distance >= 2: ${reachable}`)); +}); + +test('build.ts honours --out and reports the path it wrote', () => { + const out = join(mkdtempSync(join(tmpdir(), 'depgraph-')), 'custom.json'); + const { status, stdout } = runBuild(['--out', out]); + assert.equal(status, 0, stdout); + assert.ok(existsSync(out), `expected ${out} to exist`); + JSON.parse(readFileSync(out, 'utf8')); + assert.ok(stdout.includes('custom.json'), stdout); +}); + +test('build.ts falls back to the default path when --out has no value', () => { + // Not an error path today: a trailing `--out` is ignored rather than rejected. Pinned so the + // behaviour is a decision rather than an accident, and so changing it is a visible diff. + const { status, stdout } = runBuild(['--out']); + assert.equal(status, 0, stdout); + assert.ok(stdout.includes('.tmp/depgraph/graph.json'), stdout); +}); diff --git a/scripts/depgraph/model.ts b/scripts/depgraph/model.ts index 9312a3532ed0d4f11c1d8f05d4a42c803c2f7b47..790fea2c209c59f6efdf56bdf5ddf95c1701c9f3 100644 GIT binary patch delta 1360 zcmZuxPm3Hy6h}!GT!XMm6wKw_R+pLHHC`kkfelGyA?%7f3WA5O?tarIQQ+Ajizx5Aq=C9qpwrKtLt>0fC zpBh!uN|~OCtC|CyH!c9cL_r5Se0F;B9~MjR+5|7S-h;{iSZ&iD9$GZ0#RI}om(;qd zhXxxpCdSHEG+GxMZEGCxoY7H~8g9Ers+55w$0rL?C~VzI6{%3#mqzJ!M$$sGs`PTn zo<-VL#mGKT!KLbj2|l}GrClFnd?mv$!2-`gH1wDVWJs2qdJ zf)L1H57Sa##Ig5G+KNAHvdAH3f)E~Ds4_x+xD%!cp-~-Y@;(=z{?(}S9N4dzo_u-u z?2E(2(aCW}ha1OtkpNvGTkxkuXWv`M!&XGVl;x;>z*|z1tNYWW%Db);0t&`@)TL;` z3`^K0TXSkLBj@EMDVXF3GtBGYeupBgh)9Nj0(i$ZTwxDtNd7i@lqrSlxF^mZ+o0D!-T!W8hTiY!B+BCrgX=~yS{Efs6e8iG!D!8)6peY z0iXj0^i$Lsg&0Ri#<{auilL%&=g-K%;&eqw?7{21k3K#)nAR4nKt^^Jk;5K6=2A=2 zWyVX1lpUysDE3Ot+TA6~T*0KFEu5Jf#=U|NDKi9s5%aH%mc?qjc=_4R`?nX@T|<{) k=jZpXy=&;frF!Si`jfZmjh7eGS9hN7T;FE>>#V!^51_HsivR!s delta 508 zcmYk3Jx(Ms5QUc&APoov5=%gU9)t*3iEt$dHzPrNfUdD?+6K4X>#iQ;XgI=3LjVz1 zK$?T>0mz7uxB>z0Vfe``SG}*^t9m%U(f4V-7g@Jnlb3P5h{@{0)8gE>$;)yv_gR_z zUHSigbV~68tV?bXxe7z-JI19I6Iy{XmsR8)va(IGUSLMxt3?-;9c5BpB1&8MM$dop!-CK(8*I})wDHZIfa&GFU7eDCaivE$o~yDwKe{c+sOxBR#Ld;PO#^8B-2cm>hK BwA26q