Repository navigation
refactor(layering): rank every root module so R5 sees through (root) - #3288
Conversation
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 12 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
603c570 to
4e00f4a
Compare
|
I reviewed 4e00f4a. The code looks right, and I found no blocking defect. All 19 checks pass, including the jobs that run the layering gate and cover contracts/daemon-http. There are no conflicts. One design question before the label. I looked for a smaller design and mostly found none: each root module needs a declaration, the table is mostly data, R80 is required by the issue, and it reuses I did not run check:layering or the planted-failure variants, so the R6 "7 inversions" count and the planted outputs come from the PR body. Posting that output would close this. Not blocking: the PR body says the helpers moved "unchanged", but isRemoteTempArtifactPath now normalizes the extension, so you can reword the body. Both cubic-dev-ai threads are fixed at this commit, so you can resolve them: the extension normalization thread and the rank ordering thread. |
4e00f4a to
5ddfa57
Compare
|
Thanks for the review. Answers in order. Head is now 1. Allowance or rank: I keep the rank, and the host invariant moves to the host's own declarationMeasured at
Why the rank, not an allowance. The SDK zones use no other rank-5 material. But that one edge is what the zone is for, not an exception. An allowance would keep a rank that says the opposite and make the defining edge the exception:
Your R13 / eager-budget question found exactly such an edge. At
An allowance with The invariant belongs to the host: every spine zone reaches it only through
What rank 6 still allows that an allowance would not: other If you still prefer the allowance, it's a small change: 2. EvidenceBoth blocks come from one script that runs One correction to the old PR body: with an empty declaration the gate reports 65 R5 back-edges plus the R80 cycle, not 67. The 67 was measured while Requested head
|
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
5ddfa57 to
5c90cdf
Compare
|
Design follow-up on
Please make the scope and intended end state explicit:
The useful completion criterion is that an internal rename within an owner eventually needs no central ownership-table edit, while a genuinely forbidden dependency still fails. More declarations alone should not be the long-term architecture. |
…HTTP contract The macos-app lease recognized a remote client's temp artifact path by value-importing src/remote/daemon-artifacts.ts, while src/remote/ imports the daemon's session artifact paths: remote and daemon-server formed a zone-level cycle. The path format (builder and recognizer) is client/daemon wire vocabulary, so it moves verbatim into @agent-device/contracts/daemon-http, which both sides already load; the daemon no longer loads the remote upload client to check a path shape. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…uilder writes it
buildRemoteTempArtifactPath dotted a bare extension ('png' -> '.png') but
isRemoteTempArtifactPath stripped and re-appended the extension as given, so a path the
builder produced for 'png' failed the check for 'png'. Both now go through one
dottedExtension helper. Every production caller passes '.png' today, so no admitted path
changes.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…es (#3280) R5 could not see through the unranked (root) zone: nine zones formed one value cycle through it, and src/sdk/index.ts and src/ai-sdk/index.ts (rank 4) reached daemon-client (rank 5) through src/agent-device-client.ts. - scripts/layering/root-module-zones.ts declares the zone of every src/*.ts module, and targetDagZone reads it. (root) ranks above the spine (9) and holds only bin.ts, cli.ts and daemon.ts, so a ranked import of any undeclared root module is an R5 back-edge. New zones: daemon-contracts (2), command-runtime (3), platform-runtime (4, now including its private src/platform-runtime/ submodule) and platform-runtime-host (8: every spine zone reaches it only through loadHost's import(), so a static import of the lazy host is a back-edge from any of them; its facets read daemon session artifacts). core takes the two root modules it imports; daemon-client takes agent-device-client.ts. - With that declaration and the old ranks, the gate reports exactly the two known inversions (sdk -> daemon-client, ai-sdk -> daemon-client). The published SDK entries must export a client that reaches the daemon with no injected transport, so no code change removes that dependency: sdk and ai-sdk move to rank 6 above daemon-client, cli to 7. - R80 zone-value-dag projects static value imports onto zones and rejects any cycle, which catches same-rank pairs R5 cannot order. - Dynamic imports stay out of R5 and R80; the reason is recorded at R5 in check.ts. R6 now measures seven type-only inversions the unranked root hid (commands -> daemon-client, core -> platform-runtime, core -> platform-runtime-host, plugins -> sdk, and three platform-runtime -> platform-runtime-host types of lazily loaded host modules), ratcheted equally at the merge-base. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
5c90cdf to
66029dd
Compare
|
Re: design follow-up. Applied in 1. Logical-only scope. It is stated in the
2. Bridge and completion criterion. Also in the header:
Every declared zone name is also a usable folder name, so moving a group changes no rank and no rule; its rows just disappear. The drift test's message now says to put a new module under its zone folder first, and only then add a row for anything still directly under 3. #3287 reconciliation. I agree with the resolution: the three moves stay outstanding in #3294, and this PR says reclassification is not collocation. I have not touched #3287. ADR 0033 at One point to keep #3294 and ADR 0033 consistent: the owning location is the file's declared zone, not its consumer's folder. ADR 0033 is right that
The moves that fit are 4. Coverage fix. This was already in Validation: |
|
|
The code in 66029dd looks good to me. The two fixes from my first review are in: the layering test now generates one host back-edge for every ranked zone instead of four hand-listed ones, and the rank table matches the host declaration comment. Your reasons for ranking over adding an sdk -> daemon-client allowance hold up, so I have no simplicity concern left. Moving the physical files (#3294) is what will shrink ROOT_MODULE_ZONES, and this PR does not need it. Smoke Tests is still running on 66029dd and has not failed. The changes since the last green run are wording in a script header, a test message and a docs note. The only runtime hunks are the temp-path refactor in daemon-artifacts.ts and macos-app-lease.ts, and those are the same as the earlier heads where all 19 checks passed. I did not run check:layering, the generated test or the rank-5/7 mutation myself. I judged the regression coverage by reading the test and the rank table, and the check:affected pass is your report. There are no conflicts. The cubic-dev-ai threads on the generated host-importer test, the shared dottedExtension in daemon-http.ts, and the platform-runtime-host rank all look fixed at this head, so you can resolve them. Nothing else blocks merge except Smoke Tests finishing green. |
thymikee
added
the
ready-for-human
…me folder createAgentDeviceRuntime and the command-surface binding over it are the in-process runtime assembly #3288 already classifies as command-runtime. They now live under src/command-runtime/, where topFolder derives that zone from the folder instead of per-file ROOT_MODULE_ZONES rows. Both files move as one group: the surface file composes the factory's result, and neither is importable from the other's consumers separately. Importers (src/runtime.ts and the daemon runtime modules) re-point.
1 participant
Summary
Closes #3280.
(root)holds onlybin.ts,cli.ts,daemon.ts, ranked above the spine.scripts/layering/root-module-zones.tsdeclares every othersrc/*.tsmodule's zone; a test rejects undeclared, stale or duplicate rows.(root)in the logical graph only. The modules stay undersrc/; classifying them is not collocating them. The table is a bridge: rows go as groups move into zone folders or packages, until a rename inside an owner needs no table edit while forbidden dependencies still fail. Outstanding moves start with Architecture WS9: physical root-pass moves for daemon-diagnostics-scope, runtime-command-surface, runtime-factory #3294.zone-value-dagrejects zone-level static value cycles, invisible to R5 within one rank.remote ⇄ daemon-serverbroken by moving the remote temp-path helpers into@agent-device/contracts/daemon-http, with shared extension normalization.sdk/ai-sdkrank 6 (cli7): their entries publish the daemon client;platform-runtime-hostranks 8, so a static import of the lazy host is an R5 back-edge. R6 reports 10 type-only inversions, 7 previously hidden. 12 files.Validation
At
66029dd82:pnpm check:affected --runpassed all runnable checks.Planted
scripts/layering/check.tsruns (outputs in the review thread):(root)daemon-server -> remote -> daemon-serverplatform-runtime.tsorsdk/index.ts: R5root-module-zones.test.tspins thesdkrank and generates one static host importer per ranked zone.🤖 Generated with Claude Code