refactor(core): move the command descriptor registry into its own workspace package - #2348
Conversation
|
Size Report
Startup median (7 runs, lower is better):
|
|
Android / Smoke Tests failed on The failing step is the live emulator orientation check in
This lane is also independently red on recent I have re-run the failed job once (my one allowed re-run). Every other completed check on this head is green: Lint & Format, Typecheck & Package, Integration Tests, Compatibility & Provenance, Bundle Size, Ownership and context budgets, command-docs-gate, CodeQL, and the other Smoke Tests lane. If the re-run fails the same way, that is evidence about the emulator lane, not this diff, and I will say so rather than change the PR to chase it. Generated by Claude Code |
|
The package extraction and gates are otherwise clean at e18aa8f, and CI is green. One source-checkout regression blocks readiness: the daemon signature walker follows only relative imports. The registry and derive modules were previously in that graph; their new workspace imports are ignored. Editing the moved command policy can therefore reuse a daemon running the old implementation. This shares the signature-resolution fix needed by #2347. Please make that a shared prerequisite rather than adding a second resolver, and prove a package-owned registry edit changes the source daemon signature. Bundled installations are not affected. |
thymikee
left a comment
There was a problem hiding this comment.
Adversarial review from the #2335 coordinator, at e18aa8f. I verified the claims against the tree rather than reading the body — the substantive ones hold up. Three things to record before this merges; none block.
Verified independently: old paths are gone with no re-export shim (src/command-catalog.ts and src/core/command-descriptor/ absent from the tree); zero suppressions added anywhere in the diff (no fallow-ignore, APPROVED_OVER_CEILING, ts-expect-error, disable comments, skipped tests); enforcement edits are confined to e18aa8f and none leaked into the implementation commit; all 12 subpaths point directly at moved source files with no index.ts.
1. The HUB_ENTRY_FILES deletion deserves an explicit note — it is not a pure relocation.
Dropping registry.ts and platform-execution-entry.ts from that list is the one edit here whose failure mode is silent, so I checked it rather than taking the commit message's word: discoverFacadeEntryFiles delegates to R11's manifest-first facadeEntryFiles, and the new manifest publishes both ./registry and ./platform-execution-entry straight at the moved sources, so both are rediscovered. That part is correct.
What the commit message does not say is that the polarity of denyPlatformImplementations flips with the move: hub rows set it false, package entry surfaces set it true. These two files are now held to the stricter rule. That is fine — and green — but a future reader comparing the lists will read a deletion as lost coverage. One sentence in the commit body stating the flip, and that it was intentional, converts a silent semantic change into a recorded one.
2. Baseline discrepancy against the issue — worth settling now, not at the end.
#2336 states 154 daemon → root src/ value edges; you measured 150 → 114 with scripts/depgraph/build.ts. The delta is −36, exactly the issue's predicted reduction, so the change is right and the ceiling is met either way. But the umbrella (#2335) promises a final re-measurement of this number, and it was originally taken over a "production-only import graph (1,629 files, 8,805 edges)". Please name the filter difference (production-only vs. what you counted) in the PR body so the closing measurement uses one definition rather than silently comparing two.
3. Name the six stranded tests' root dependency.
src/__tests__/command-descriptor-*.test.ts stay behind because a package may not import root src/. That is the correct call, but it leaves AGENTS.md's "tests mirror source topology one-to-one" broken for six files, and the reason is now invisible. List which root-src/ symbol each of the six actually needs. That turns an acknowledged exception into an inventory a later wave can act on — and it is the same kind of residual-seam list #2342 is required to produce.
For the record, the three chmod 0500 unit failures are not yours: the container runs as uid 0, which ignores the mode. #2347 hit the identical three independently, and CI is green here across all 21 checks.
Generated by Claude Code
|
This head now conflicts with main. Resolve that alongside the shared source-signature prerequisite from #2347, then rerun the affected gates. |
|
Fixed in One resolver, in the file #2347 moves. The fix is inside Three properties worth review:
Proof. Cost, measured here: 608 → 1,460 modules; cold walk 35ms → ~80ms; the per-invocation cached path 3.1ms → 7.5ms. The two doc comments quoting "~800 modules / ~30ms" are updated rather than left stale.
Generated by Claude Code |
|
Merged Signature coverage, verified after the merge. #2347 landed the same fix in Conflict: only The three review points are recorded in the merge commit body. In short: dropping the two entries from Re-measured on the new base: daemon → root Generated by Claude Code |
|
Signature-walk cost, re-measured on this head against pre-#2347
The last row is the one on the hot path: Of this package: 11 imported sources plus Two doc comments in Generated by Claude Code |
|
Independent review at |
|
The code review is still clean, but this head now conflicts with main after #2346 merged. I removed ready-for-human; resolve the conflict and revalidate before landing. |
…kage `src/core/command-descriptor/`, `src/command-catalog.ts`, `src/core/wait-positionals.ts` and `src/core/parse-timeout.ts` move as git renames into a new private package `@agent-device/command-registry` (deps: contracts, selectors). One subpath per module points straight at the moved file; no `index.ts`, no re-export at the old path. Every consumer switches to the owning specifier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jqfa11D8QsCMuL17SsLvDz
…de graph The daemon reaches the registry and its catalog only by workspace specifier. A walk that stopped at the package boundary would report an unchanged signature after a descriptor edit, and the client would keep reusing a daemon running the superseded policy. The manifest is asserted beside the sources because its `exports` map is what chose them. The cache doc comment quoting the old ~800-module graph is corrected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jqfa11D8QsCMuL17SsLvDz
R66's `COMMAND_DESCRIPTOR_MODULE`, R16's record-runtime join subject and the Fallow `AssertTrue` totality-guard key follow the registry to its package. The two descriptor hubs leave `HUB_ENTRY_FILES` because the package manifest now publishes them, so the eager-closure gate discovers them as facades and one entry gets one rule; this also flips `denyPlatformImplementations` from false (hub) to true (package entry) for both, which is intentional and stricter. `command-registry` joins the ranked spine at rank 1. No `APPROVED_OVER_CEILING` row: rename detection carries every moved entry's merge-base baseline, so all twelve fall under the no-growth rule rather than a ceiling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jqfa11D8QsCMuL17SsLvDz
fc40090 to
f887766
Compare
|
Conflict resolved. Rebased onto History is now three commits instead of five, and two commits reviewers saw earlier are deliberately gone:
The three conflicts against #2346 were import-line only: package specifiers alongside the split I did not hand-wave the equivalence — I produced the correct merge tree first, rebuilt the branch linearly on top of Also folded in from review: the CI is re-running on Generated by Claude Code |
|
Validated on the rebased head
The rebase preserved the tree exactly: CI on this head is green across everything that has reported — Coverage, Integration Tests, Typecheck & Package, Lint & Format, Repo Guards, Compatibility & Provenance, Bundle Size, Ownership and context budgets, command-docs-gate, CodeQL, and two of the four Smoke lanes; the remaining two are still running. The Android emulator lane that went red on an earlier head has since passed on both Generated by Claude Code |
Main moved the command descriptor registry into @agent-device/command-registry (#2348) and split src/daemon/types.ts (#2346): the planned-operations module and its test follow the registry into the package (exported as ./planned-operations), the executionPlan carrier moves to daemon-request.ts, and the batch step context lands on the new BatchInvoke signature.
Summary
src/core/command-descriptor/,src/command-catalog.ts,src/core/wait-positionals.tsandsrc/core/parse-timeout.tsimported nothing from rootsrc/, yet were the daemon's largest root coupling. They move as git renames into a new private package@agent-device/command-registry(deps: contracts, selectors), one subpath per module pointing straight at the moved file; noindex.ts, no re-export at the old path. Every consumer — daemon, commands, mcp, cli, cli-schema, core, root, client, ai-sdk, scripts, tests — switches to the owning specifier; contracts is untouched. Six descriptor tests that import rootsrc/stay there, which a package may not do; the rest move unchanged. The__OWNER_FILES__declaration moves with its only consumer.Rebased onto
dcd8b65, so it carries #2347's code-signature walker and #2346'sdaemon-request.ts/session-state.tssplit. One test pins that the daemon code graph reaches this package, and two doc comments are corrected off their pre-#2347 figures.Rename-only proof —
git diff -M90% --stat origin/main...HEAD: 42 renames, all 12 moved sources at 92–100%.Closes #2336.
Validation
pnpm check:affected --runon this tree: format, lint, typecheck, layering, di-seams, fallow, mcp-metadata, build, package, integration-node, macos-coverage green. vitest-related: 9746 passed, 3 failed — thechmod 0500tests that fail identically atorigin/mainhere, since uid 0 ignores the mode.Daemon → root
src/value edges 148 → 112 (−36, ceiling 118), frompnpm depgraph. CLI help byte-identical to its base: 82 sections, one sha256.Signature walk, measured against pre-#2347
main: 620 → 1,461 modules, 12 of them this package; cold walk ~90ms; and the CLI hot path — the cached per-invocation signature — 2.9ms → 7.5ms.🤖 Generated with Claude Code
https://claude.ai/code/session_016DkNZCMWtm1KUbseKANADi