refactor(contracts): declare shared vocabulary below its consumers, ratchet what remains - #1435
Conversation
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
Adversarial self-review of #1435 found four things worth fixing. R9 shipped with no unit test. Every other rule in this gate has one (R5 back-edges, R6 inversions, R7 session state, R8 zero-dep closures); R9's only verification was a manual injection CI cannot repeat. Added tests for the three distinctions it depends on: a type-only loop counts, a dynamic-only loop does not, a value loop still does. Writing that test immediately found an undocumented edge case, which is the argument for it. largestTypeCycleSize returns 1 for an acyclic graph that has non-dynamic edges but 0 when every edge is dynamic, because only edge-participating files enter the walk. Immaterial to a growth ratchet, but an inconsistent floor nobody had written down. Now specified in the doc comment and pinned by the test, so 0 and 1 cannot later be read as a meaningful difference. largestTypeCycleMembers was exported with no consumer - speculative API, and scripts/layering is in Fallow's ignorePatterns so nothing would have flagged it. Same pattern review caught on the previous head with MaestroRuntimeFlags and TargetRect. Made module-private. ResolvedMetroKind was declared twice after the Metro payload move: exported from contracts/metro.ts and still private in metro/client-metro.ts. client-metro.ts now imports it. The gate computed the SCC twice per run, once in the rule and once for the success line. Computed once and threaded, so the two can no longer disagree. Also re-verified the claim this PR rests on, with a stronger check than the one in the body: comparing DECLARATION names in index.d.ts counts inlined internals, and by that measure this branch appears to lose five names (PrepareMetroRuntimeResult, ReloadMetroResult, ResolvedMetroKind, SCROLL_INPUT_DIRECTIONS, ScrollInputDirection). All five are declared-but-not-exported helpers. The real surface - exported names across all eleven published entrypoints - is 69 on both sides, identical. Also proved DaemonRequest structurally equal to its pre-split shape with a type-level assertion rather than by reasoning, and confirmed SessionAction, CommandFlags and TargetAnnotationV1 moved byte-identically. pnpm check green, 4535 unit tests, 24 layering tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur
|
Code semantics and exact-head CI are clean, but this is not ready yet: |
Addresses review on #1435. contracts/client-api.ts was 1,064 LOC and grouped session, app, interaction, replay, observability and recording contracts together, so it answered no one question and crossed the >1,000-LOC architecture-debt tripwire in AGENTS.md:124. Split it into 14 domain-family files by the command families that already exist (client-connection, client-device-view, client-session, client-lease, client-app, client-capture, client-target, client-gesture, client-selector-read, client-replay, client-observability, client-settings, client-system, client-request); the four Metro client shapes went into the existing contracts/metro.ts so one file answers the Metro question. Largest resulting file is 137 LOC. client/client-types.ts re-exports one wildcard per family, so the published import path is unchanged. Published surface verified unchanged against main two ways: the exported-name set of all 11 published entrypoints is identical (70 names), and every declaration in the built index.d.ts is byte-identical after normalization -- 0 names added, 0 shapes changed. index.d.ts got smaller (1,726 -> 1,682 lines): 10 declarations main duplicated into it now resolve through a shared chunk. Also, from re-examining the two findings the review flagged as blind spots: - CommandRequest was a third name for "a request" that no consumer needed. Every core/command-descriptor/ use read only command/positionals/flags, in two spellings (the full type and a Pick of it). Replaced by contracts/dispatched-command.ts DispatchedCommand -- those three fields and nothing else, with command/positionals Picked from the wire type so they cannot drift. daemon/types.ts DaemonRequest now extends the wire shape directly. Two request shapes again, at two ranks. - The 7 remaining R6 inversions each get a mechanical reason rather than an appeal to an ADR: the 4 AgentDeviceClient edges are a real zone-level cycle (client-types.ts imports ProjectedNavigationCommandClient from commands/), and no narrower port exists (26 call sites across 13 namespaces); the 2 DaemonCommandDescriptor edges are unavoidable because that shape is stated in terms of the server-private DaemonRequest; the 1 DaemonCommandRoute edge is unavoidable because the type is computed from the daemon's handler table. Cleanups found on the way: three doc comments this branch had orphaned from their declarations (SettleCommandOptions, RecordControlOptions, ReloadMetroResult -- the last had drifted onto an unrelated type it misdescribed) are reattached; intra-contracts imports normalized from '../contracts/x.ts' to './x.ts', which is what the duplicate-import lint caught; and stale references to the deleted file removed from the docs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur
|
Both addressed in The split. The disclosure is now the first section of the PR body, stating plainly that this is an intentional architecture-wide type-boundary/layering refactor and why the breadth is inherent to the change rather than incidental. One correction to the number: it's 93 files, not 79 — the split itself touched 68, since every consumer that imported from Verification the split changed nothing published, done two independent ways against
That second check turned up something worth knowing: Two things from re-examining the findings I'd flagged as my own blind spots, since you didn't ask for either and both turned out to be real:
The 7 remaining R6 inversions now have mechanical reasons rather than appeals to ADRs, which is what you should be able to check:
Also worth flagging as a defect of my own earlier commits on this branch: scanning every branch-touched file for comment blocks not followed by a declaration found three doc comments the moves had orphaned. The
Generated by Claude Code |
Adversarial self-review of #1435 found four things worth fixing. R9 shipped with no unit test. Every other rule in this gate has one (R5 back-edges, R6 inversions, R7 session state, R8 zero-dep closures); R9's only verification was a manual injection CI cannot repeat. Added tests for the three distinctions it depends on: a type-only loop counts, a dynamic-only loop does not, a value loop still does. Writing that test immediately found an undocumented edge case, which is the argument for it. largestTypeCycleSize returns 1 for an acyclic graph that has non-dynamic edges but 0 when every edge is dynamic, because only edge-participating files enter the walk. Immaterial to a growth ratchet, but an inconsistent floor nobody had written down. Now specified in the doc comment and pinned by the test, so 0 and 1 cannot later be read as a meaningful difference. largestTypeCycleMembers was exported with no consumer - speculative API, and scripts/layering is in Fallow's ignorePatterns so nothing would have flagged it. Same pattern review caught on the previous head with MaestroRuntimeFlags and TargetRect. Made module-private. ResolvedMetroKind was declared twice after the Metro payload move: exported from contracts/metro.ts and still private in metro/client-metro.ts. client-metro.ts now imports it. The gate computed the SCC twice per run, once in the rule and once for the success line. Computed once and threaded, so the two can no longer disagree. Also re-verified the claim this PR rests on, with a stronger check than the one in the body: comparing DECLARATION names in index.d.ts counts inlined internals, and by that measure this branch appears to lose five names (PrepareMetroRuntimeResult, ReloadMetroResult, ResolvedMetroKind, SCROLL_INPUT_DIRECTIONS, ScrollInputDirection). All five are declared-but-not-exported helpers. The real surface - exported names across all eleven published entrypoints - is 69 on both sides, identical. Also proved DaemonRequest structurally equal to its pre-split shape with a type-level assertion rather than by reasoning, and confirmed SessionAction, CommandFlags and TargetAnnotationV1 moved byte-identically. pnpm check green, 4535 unit tests, 24 layering tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur
1c0f359 to
0519919
Compare
Addresses review on #1435. contracts/client-api.ts was 1,064 LOC and grouped session, app, interaction, replay, observability and recording contracts together, so it answered no one question and crossed the >1,000-LOC architecture-debt tripwire in AGENTS.md:124. Split it into 14 domain-family files by the command families that already exist (client-connection, client-device-view, client-session, client-lease, client-app, client-capture, client-target, client-gesture, client-selector-read, client-replay, client-observability, client-settings, client-system, client-request); the four Metro client shapes went into the existing contracts/metro.ts so one file answers the Metro question. Largest resulting file is 137 LOC. client/client-types.ts re-exports one wildcard per family, so the published import path is unchanged. Published surface verified unchanged against main two ways: the exported-name set of all 11 published entrypoints is identical (70 names), and every declaration in the built index.d.ts is byte-identical after normalization -- 0 names added, 0 shapes changed. index.d.ts got smaller (1,726 -> 1,682 lines): 10 declarations main duplicated into it now resolve through a shared chunk. Also, from re-examining the two findings the review flagged as blind spots: - CommandRequest was a third name for "a request" that no consumer needed. Every core/command-descriptor/ use read only command/positionals/flags, in two spellings (the full type and a Pick of it). Replaced by contracts/dispatched-command.ts DispatchedCommand -- those three fields and nothing else, with command/positionals Picked from the wire type so they cannot drift. daemon/types.ts DaemonRequest now extends the wire shape directly. Two request shapes again, at two ranks. - The 7 remaining R6 inversions each get a mechanical reason rather than an appeal to an ADR: the 4 AgentDeviceClient edges are a real zone-level cycle (client-types.ts imports ProjectedNavigationCommandClient from commands/), and no narrower port exists (26 call sites across 13 namespaces); the 2 DaemonCommandDescriptor edges are unavoidable because that shape is stated in terms of the server-private DaemonRequest; the 1 DaemonCommandRoute edge is unavoidable because the type is computed from the daemon's handler table. Cleanups found on the way: three doc comments this branch had orphaned from their declarations (SettleCommandOptions, RecordControlOptions, ReloadMetroResult -- the last had drifted onto an unrelated type it misdescribed) are reattached; intra-contracts imports normalized from '../contracts/x.ts' to './x.ts', which is what the duplicate-import lint caught; and stale references to the deleted file removed from the docs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur
|
Rebased onto Verified by checking out I'm not fixing it here. The gate's contract is that a waiver names why the topic isn't benchmarked yet — that's a content judgement about #1395's iOS SpringBoard/widget guidance, and folding it into a 93-file type-boundary refactor would put an unrelated docs decision inside this diff. The fix is one line in Everything else on the current base is green: 4548 passed, 4 skipped, 1 failed (the inherited one). Layering guard OK across 931 source files — R6 = 7, R7 = 42 fields all classified, R8 clean, R9 = 102. Lint, typecheck, I'll re-run once Generated by Claude Code |
|
Re-review of the latest head is clean on code quality. The prior |
…umers The layering gate's largest remaining cluster was 28 type-only inversions from a single edge: `commands/` declaring itself in terms of `client/client-types.ts`. R2 forbids the reverse import, so a shape both surfaces need has to sit below both. The command/device vocabulary — connection config, the device and session views, and every per-command Options/Result — now lives in `contracts/client-api.ts`; `client/client-types.ts` keeps the `AgentDeviceClient` facade and re-exports the rest through one wildcard. R6 total: 42 -> 18. No new inversion in any pair. The published surface is unchanged, and that is verified rather than asserted: the built `index.d.ts` exports the same 216 type names as main, byte-identical. Eight shapes deliberately did NOT move, because each is stated in terms of a HIGHER-ranked zone: `ScrollOptions` (ScrollInputDirection, commands/), the four navigation Options plus `AgentDeviceCommandClient` (navigation-projection, commands/), and the two Metro result aliases (metro/). Declaring those in contracts/ would trade 28 commands->client edges for contracts->commands and contracts->metro ones — the foundation depending on the layers above it, worse in kind even though fewer in number. This is measured, not assumed: moving the whole file to contracts/ first took the gate from 42 to 48, which is how the floor was found. Two keystone moves made the other 84 movable: - `RemoteConnectionProfileFields` joined its sibling `CloudProviderProfileFields` in contracts/remote-config-fields.ts. It was the root of the base chain (AgentDeviceClientConfig -> AgentDeviceRequestOverrides -> DeviceCommandBaseOptions -> every per-command Options), so one rank-4 declaration was pinning ~80 shapes up with it. - `DaemonBatchStep` moved to contracts/batch-step.ts. Its `runtime` field was written `DaemonRequest['runtime']`, dragging the whole daemon request type in to say `SessionRuntimeHints` — the same type, three zones lower. `CompanionTunnelScope`/`MetroBridgeScope` also moved to contracts/, since the vocabulary needs the scope shape and it sat next to client-local env-var names. Six pass-through re-exports in client-types.ts are suppressed per-name with the reason inline: they exist only to publish contracts/kernel types through the package entrypoint wildcard, every internal consumer imports them from the declaring module, so "no consumer" is correct and not actionable — deleting them would remove names from the public types. `pnpm check` green, 4488 unit tests. Findings doc records the sequencing for the last 5: the upstream declarations have to come down before the shapes that need them can. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur
The rendered dependency-graph viewer is not being merged (PR #1409 closed). It cost ~2200 lines plus a Fallow exemption for a 920-line canvas renderer, and nobody — human or agent — reached a conclusion from the picture. Every finding in this document came from short queries against the gate's own model. This file pointed at the `claude/depgraph-viewer` branch for the tooling, which would have dangled once that branch is deleted. Replaced with the thing that was actually load-bearing: a throwaway probe script, inlined, that re-derives the numbers from `scripts/layering/model.ts` and nothing else. Verified verbatim — it reproduces TYPE_INVERSION_BASELINE exactly, which is also the check that tells you whether either side has gone stale. Two numbers in the summary table were stale, describing an intermediate state rather than what shipped: R6 said "35 across 4" (actually 18 across 5 after the vocabulary move) and ranked coverage said "729 of 894" (actually 888 of 901). Both corrected, along with the file/edge counts in the header. Also notes the deduplication detail that makes the query agree with the gate: each file pair counts once, so a raw edge count reads higher. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur
…the inversions R6 type-only spine inversions: 18 -> 7, and every one of the 7 that remains is a deliberate architectural position rather than a misplaced declaration. Four keystones moved to contracts/, each of which was pinning a much larger set: - `CommandFlags` (was core/dispatch-context.ts). One rank-2 declaration holding the daemon's request type and every recorded action above it. Its last non-contracts dependency was `DaemonBatchStep`, already moved in 3fdbfe0. - `SessionAction` (was daemon/types.ts). replay/ (6 modules) and compat/maestro/ read and write session scripts; declaring the shape inside the daemon made both depend on the server to describe a file format neither asks it to produce. The daemon still owns the recording — only the shape moved. - `TargetAnnotationV1` shape (was replay/target-identity.ts). ADR 0012 target evidence, written by 8 daemon modules and read by commands/; the parsing and classification logic stays in replay/. - `ScrollInputDirection` and the Metro prepare/reload result payloads, which unblocked `ScrollOptions` and `MetroPrepareResult`/`MetroReloadResult`. `DaemonRequest` also split into the three shapes it had been conflating: the kernel WIRE shape (`flags?: Record<string, unknown>`, because a process boundary cannot enforce a vocabulary), the new `contracts/command-request.ts` `CommandRequest` (wire shape with flags typed — what a command surface needs), and the daemon's own refinement (+ `internal?: DaemonRequestInternal`, carrying SessionState callbacks and the admitted lease). core/command-descriptor/ had been importing the third to read `command`, `positionals` and `flags`. Two things deliberately NOT moved, because moving them would add coupling rather than remove it, and the baseline now argues both: - `DaemonCommandDescriptor`/`DaemonCommandRoute` — the route type is `keyof typeof DAEMON_ROUTE_HANDLERS`, derived from what the server implements. Moving it down means re-declaring route names in contracts plus a gate to prove the handler map still covers them. ADR 0003/0008 own that boundary. - `AgentDeviceClient` — used as an opaque handle by 4 files. The facade is built from commands/'s own NAVIGATION_COMMAND_PROJECTIONS, so this is a genuine zone-level cycle; breaking it is a design call about where that registry belongs. R5 is zero here: nothing imports the client at runtime, only its type. Also records the largest structural finding, which R6 does not measure: cycles by edge kind are 1 (value only), 87 (value + type-only), 1 (value + dynamic), 213 (all). At runtime the graph is a clean DAG; the 87-file type-level cluster means no one of those files' types can be read in isolation. Hubs are runtime-contract.ts, commands/runtime-types.ts, backend.ts, commands/runtime-common.ts. Not attempted here — it is a different and much larger change. `pnpm check` green, 4488 unit tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur
…er client port R9: the largest strongly-connected component over value + type-only edges may not grow. R4 keeps the VALUE graph acyclic, so every cycle counted here is created by type-only imports - free at runtime, invisible to R5/R6, and the largest single obstacle to reading a subsystem in isolation: inside a component of 102 files, no file has a self-contained slice. Baseline set to 102, which is what THIS branch achieves - main carries 107 and the boundary moves here bring it to 102. An earlier revision baselined 87, measured against an older main; after rebasing onto f19864e the real figure was 102 and the new rule fired on its own stale baseline. Worth stating because the failure looked like a regression and was not: attribution showed main at 107 and this branch reducing it, which is the check working rather than complaining. Growth-only, deliberately unlike R6. Reducing 102 is a real refactor rather than a file move, so a hard equality would turn every unrelated improvement into a baseline edit. A shrunk tree is reported in the success line instead of failing. Verified at the new baseline by adding one type-only import that closes a loop and watching 102 become 108 and the gate reject it. The refactor itself is still not attempted. Hubs by in-component dependents are runtime-contract.ts, commands/runtime-types.ts, backend.ts, commands/runtime-common.ts; a pass starts there. Separately, investigated the narrower-port idea for the 4 remaining -> client inversions and it does not work. Measured first: files NAMING AgentDeviceClient (the inversions) 4 files CALLING client methods 26 distinct facade namespaces reached 13 The narrowness is an artifact of where the type is named, not of what is used. Making those four generic over the client type pushes the concrete type into the 26 implementations, turning 4 inversions into up to 26. A port spanning 13 namespaces is the whole facade, so it would either duplicate the public API shape - a second source of truth for it - or derive from the facade and carry the same dependency. So the four are the minimum number of naming sites rather than an accident: they are the choke point. Recorded as a position with the numbers behind it. The remaining option is the question underneath it - whether NAVIGATION_COMMAND_PROJECTIONS belongs in commands/ - and that is a design decision about the command surface, not a dependency cleanup. pnpm check green, 4535 unit tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur
Adversarial self-review of #1435 found four things worth fixing. R9 shipped with no unit test. Every other rule in this gate has one (R5 back-edges, R6 inversions, R7 session state, R8 zero-dep closures); R9's only verification was a manual injection CI cannot repeat. Added tests for the three distinctions it depends on: a type-only loop counts, a dynamic-only loop does not, a value loop still does. Writing that test immediately found an undocumented edge case, which is the argument for it. largestTypeCycleSize returns 1 for an acyclic graph that has non-dynamic edges but 0 when every edge is dynamic, because only edge-participating files enter the walk. Immaterial to a growth ratchet, but an inconsistent floor nobody had written down. Now specified in the doc comment and pinned by the test, so 0 and 1 cannot later be read as a meaningful difference. largestTypeCycleMembers was exported with no consumer - speculative API, and scripts/layering is in Fallow's ignorePatterns so nothing would have flagged it. Same pattern review caught on the previous head with MaestroRuntimeFlags and TargetRect. Made module-private. ResolvedMetroKind was declared twice after the Metro payload move: exported from contracts/metro.ts and still private in metro/client-metro.ts. client-metro.ts now imports it. The gate computed the SCC twice per run, once in the rule and once for the success line. Computed once and threaded, so the two can no longer disagree. Also re-verified the claim this PR rests on, with a stronger check than the one in the body: comparing DECLARATION names in index.d.ts counts inlined internals, and by that measure this branch appears to lose five names (PrepareMetroRuntimeResult, ReloadMetroResult, ResolvedMetroKind, SCROLL_INPUT_DIRECTIONS, ScrollInputDirection). All five are declared-but-not-exported helpers. The real surface - exported names across all eleven published entrypoints - is 69 on both sides, identical. Also proved DaemonRequest structurally equal to its pre-split shape with a type-level assertion rather than by reasoning, and confirmed SessionAction, CommandFlags and TargetAnnotationV1 moved byte-identically. pnpm check green, 4535 unit tests, 24 layering tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur
Addresses review on #1435. contracts/client-api.ts was 1,064 LOC and grouped session, app, interaction, replay, observability and recording contracts together, so it answered no one question and crossed the >1,000-LOC architecture-debt tripwire in AGENTS.md:124. Split it into 14 domain-family files by the command families that already exist (client-connection, client-device-view, client-session, client-lease, client-app, client-capture, client-target, client-gesture, client-selector-read, client-replay, client-observability, client-settings, client-system, client-request); the four Metro client shapes went into the existing contracts/metro.ts so one file answers the Metro question. Largest resulting file is 137 LOC. client/client-types.ts re-exports one wildcard per family, so the published import path is unchanged. Published surface verified unchanged against main two ways: the exported-name set of all 11 published entrypoints is identical (70 names), and every declaration in the built index.d.ts is byte-identical after normalization -- 0 names added, 0 shapes changed. index.d.ts got smaller (1,726 -> 1,682 lines): 10 declarations main duplicated into it now resolve through a shared chunk. Also, from re-examining the two findings the review flagged as blind spots: - CommandRequest was a third name for "a request" that no consumer needed. Every core/command-descriptor/ use read only command/positionals/flags, in two spellings (the full type and a Pick of it). Replaced by contracts/dispatched-command.ts DispatchedCommand -- those three fields and nothing else, with command/positionals Picked from the wire type so they cannot drift. daemon/types.ts DaemonRequest now extends the wire shape directly. Two request shapes again, at two ranks. - The 7 remaining R6 inversions each get a mechanical reason rather than an appeal to an ADR: the 4 AgentDeviceClient edges are a real zone-level cycle (client-types.ts imports ProjectedNavigationCommandClient from commands/), and no narrower port exists (26 call sites across 13 namespaces); the 2 DaemonCommandDescriptor edges are unavoidable because that shape is stated in terms of the server-private DaemonRequest; the 1 DaemonCommandRoute edge is unavoidable because the type is computed from the daemon's handler table. Cleanups found on the way: three doc comments this branch had orphaned from their declarations (SettleCommandOptions, RecordControlOptions, ReloadMetroResult -- the last had drifted onto an unrelated type it misdescribed) are reattached; intra-contracts imports normalized from '../contracts/x.ts' to './x.ts', which is what the duplicate-import lint caught; and stale references to the deleted file removed from the docs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Bfu8HofkhybiAm5LECfqur
0519919 to
ee47b59
Compare
|
Rebased onto
Two things worth calling out from the rebase, since #1392 touched files this PR also depends on: R7 moved with it, correctly. #1392's
Re-verified every published-surface claim against the new base, not carried over from the old measurement:
Nothing left on my side. Coverage should be green on this head. Generated by Claude Code |
|
Re-review at head ee47b59: clean readiness verdict. The rebase preserves the reviewed patches, the prior Coverage blocker is resolved, the PR is mergeable/CLEAN, and all 24 reported checks are successful, including Coverage, Integration Tests, Affected-check Selector, No test-only DI seams, Layering Guard, Fallow, typecheck, lint, and platform smokes. No code findings remain. Marked ready-for-human. |
|
Scope disclosure
This is an intentional architecture-wide type-boundary and layering refactor spanning 93 files.
The breadth is the point, not an accident of tooling: a shared shape declared one rank too high pins
every shape stated in terms of it, so fixing one keystone necessarily touches every consumer that
named it. Nothing here changes runtime behaviour — the changes are declaration moves, import
rewrites, and new gate rules. Reviewing it as a whole-tree boundary change rather than file-by-file
is the intended reading; §"Four keystones" and §"One file per command family" are where the actual
decisions are.
The published package surface is unchanged, verified two independent ways against
main(seeValidation).
Summary
Type-only spine inversions (R6): 61 → 7, and every one of the 7 that remains is a deliberate
architectural position with a mechanical reason written at the baseline — not an appeal to an ADR
and not leftover debt. Adds R9, a growth-only ratchet on the largest type-level import cycle —
the one structural problem R6 never measured.
Rebased onto
f19864e(#1410). The merged two-sources check works across the merge: the depgraphreport now computes 7 and
TYPE_INVERSION_BASELINEis 7, soscripts/depgraph/model.test.tspasseswithout special handling.
replay → daemon-servermain)Four keystones, each pinning a much larger set
The pattern repeated, and neither of the first two looked like a keystone from the graph. Both were
found by asking what does the target itself import, and what rank is that?
contracts/DaemonBatchStep— itsruntimefield was writtenDaemonRequest['runtime'], dragging the whole daemon request type in to saySessionRuntimeHints, the same type three zones lowerCommandFlagsCommandFlags— one rank-2 declaration pinning ~80 public API shapes above itSessionAction,DispatchedCommandTargetAnnotationV1shape — ADR 0012 target evidence, written by 8 daemon modules, read bycommands/; the parsing and classification logic stays inreplay/SessionActionScrollInputDirection, Metro prepare/reload payloadsScrollOptions,MetroPrepareResult/MetroReloadResultSessionActionfreedreplay/entirely — 6 inversions to 0. The daemon still owns the recording;only the shape moved.
One file per command family
(Added in response to review.) The vocabulary's first landing place was a single
contracts/client-api.tsat 1,064 LOC, which grouped session, app, interaction, replay,observability and recording contracts together. That answered no one question and crossed the
>1,000 LOC is architecture debttripwire atAGENTS.md:124— it relocated the whole-file contextcost rather than removing it.
Split by the command/domain families that already exist in the tree, one file each:
client-app.tsclient-gesture.tsclient-request.tsclient-system.tsclient-connection.tsclient-device-view.tsclient-capture.tsclient-replay.tsclient-settings.tsclient-observability.tsclient-selector-read.tsclient-lease.tsclient-session.tsclient-target.tsThe four Metro client shapes went into the existing
contracts/metro.tsrather than a 15th file,so one file answers the Metro question.
client/client-types.tsre-exports one wildcard per family,so every consumer's import path is unchanged.
DispatchedCommand— two request shapes, not threeAn earlier revision of this PR introduced
contracts/command-request.tsCommandRequest, and Iflagged the third name for one concept as a real cost. Re-examining it, no consumer wanted a third
request: every
core/command-descriptor/use read onlycommand,positionalsandflags, intwo spellings (the full type, and
Pick<CommandRequest, 'command' | 'positionals' | 'flags'>— bothin the same file). So the honest contract is not a request at all:
kernel/contracts.tsDaemonRequest— the wire shape,flags?: Record<string, unknown>,because a process boundary cannot enforce a flag vocabulary.
daemon/types.tsDaemonRequest— the wire shape withtoken/sessionrequired,flagsnarrowedto
CommandFlags, andinternal?: DaemonRequestInternalcarryingSessionStatecallbacks and theadmitted lease. Server-private, and why it cannot move down.
contracts/dispatched-command.tsDispatchedCommand— not a request: the command a requestdispatches. Those three fields and nothing else, with
command/positionalsPicked from the wiretype so they cannot drift from it.
core/command-descriptor/had been importing the daemon's version to read three fields — reaching uptwo ranks, and depending on the server's private extension to get a command name. One narrow name
replaced both spellings and the now-redundant
RecordingEffectRequestalias.The 7 remaining inversions, argued mechanically
Restated from "position, trust the ADR" to a reason you can check:
AgentDeviceClient—client/client-types.tsimportsProjectedNavigationCommandClientfrom
commands/system/navigation-projection.ts, so this is a real zone-level type cycle, not amisplaced declaration. Breaking it means deciding where the projection registry belongs. A narrower
port doesn't exist either — 4 files name the facade, but 26 call sites use methods across 13 of
its namespaces, so a port spanning them is the facade.
DaemonCommandDescriptor— that shape is stated in terms of the server-privatedaemon/types.tsDaemonRequest(refFrameEffect,allowSessionlessDefaultDevice,skipSessionlessProviderDeviceare all(req: DaemonRequest) => …), so it cannot be declared belowthe daemon. Having
core/re-declare a parallel 13-field shape would trade one erased edge for asecond source of truth.
DaemonCommandRoute— it iskeyof typeof DAEMON_ROUTE_HANDLERS, computed from thedaemon's handler table, so it cannot exist below that table.
commands/command-explain.tsuses itto key an exhaustive
Record<DaemonCommandRoute, string>of owner files; a hand-written union incontracts/would drop exactly that exhaustiveness.What's left underneath is the real question: does
NAVIGATION_COMMAND_PROJECTIONSbelong incommands/? That's a design decision about the command surface, deliberately not pre-decided here.R9 — type-cycle growth
Cycle size by edge kind on this branch:
At runtime the module graph is a clean DAG. The 102-file cluster is purely type-level: no file in it
has a self-contained slice, which is the largest single obstacle to reading a subsystem in isolation —
and a real cost for agents loading context. Growth-only on purpose: reducing 102 is a refactor, not a
file move, so a hard equality would turn every unrelated improvement into a baseline edit. A shrunk
tree is reported in the success line rather than failing.
One thing worth knowing about how this landed. An earlier revision baselined 87, measured against
the older main. After rebasing, the real figure was 102 and R9 fired on its own stale baseline. That
looked like a regression and wasn't: attribution showed
mainat 107 and this branch reducing it to102. The rule caught a genuine number change on its first outing — I just had to check which side
moved before reacting. Baseline is now set at what this branch achieves.
The refactor itself is deliberately not attempted; hubs are
runtime-contract.ts,commands/runtime-types.ts,backend.ts,commands/runtime-common.ts.Validation
pnpm checkgreen — lint, typecheck, R1–R9,depgraph:test, production exports, MCP metadata, build,bundle owner files, Fallow, 4535 unit tests.
Published surface unchanged, verified two independent ways against
main:and no removals. (This is the right check; counting declarations in the built
.d.tsis a weakerproxy because it also counts inlined internals.)
index.d.tsis identical after whitespace normalization — 0 namesadded, 0 shapes changed (compared by parsing both with
oxc-parser, not by grep).index.d.tsin fact got smaller — 1,726 → 1,682 lines — because 10 declarationsmainduplicated into it (the Metro option/result shapes,
ScrollInputDirection) now resolve through ashared chunk once the vocabulary sits below both its consumers. Five of those 10 were unreferenced on
main: dead weight in the shipped types.Verified by injection rather than assertion, each reverted after:
contracts/— took the gatefrom 42 to 48, which is how the floor was found. Recorded in
docs/dependency-graph-findings.mdso the next person doesn't repeat it.Three doc comments this branch had orphaned from their declarations are reattached — the
ReloadMetroResultone had drifted onto an unrelated type it actively misdescribed. Found by scanningevery branch-touched file for comment blocks not followed by a declaration, which is worth doing after
any move of this size.
No device evidence. These are declaration moves and types are erased, but
SessionActionandDispatchedCommandsit on the replay and dispatch paths, so a livereplayrun is the residual riskand I can't perform it here.
Generated by Claude Code