Repository navigation
refactor(daemon): move wire request and response types out of the daemon package - #3616
Conversation
![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 203 files
Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
View guided diff | Turn on auto-fix | Re-trigger cubic
Size Report
Startup median (7 runs, lower is better):
|
|
The code at bcdaa44 looks good. The diff only changes type imports and type aliases, so the moved wire types are a clean split from the daemon package. Not blocking: the re-acked entries in test/wire-compat/ledger.json (DownloadRemoteArtifactParams, materializeRemoteArtifacts, buildHttpRpcPayload and handleDaemonHttpResponseBody) keep their old #2246/#1801 rationale, and only rejectDaemonHttpRpcError mentions the type-name move. A one-sentence note on each would keep the ledger consistent. Take it or leave it. The cubic-dev-ai P2 thread on the capture-disclosure test does not apply, so you can resolve it: #3616 (comment). That test feeds the daemon's own handlers, which take the daemon request type, and this PR covers production client, remote and CLI importers. Switching it to DaemonWireRequest is optional tidying. I did not run typecheck, layering or wire-compat locally, and I did not recompute the wire-compat digests. I judged the ledger changes against the type-only diff. I also did not check whether the layering edge set includes test files, which decides whether the three src/daemon-client/tests DaemonInvokeFn imports could trip R78. The branch has merge conflicts with main, in packages/daemon/src/execution/interaction/internal/find.ts. Both Smoke Tests jobs are still queued or in progress, and none has failed. The size report shows 0 B installed delta and -7 B download, which fits erased type imports. Please rebase onto main, then let Smoke Tests finish before merging. |
bcdaa44 to
5bf3aec
Compare
|
The code at 5bf3aec looks good, and the rebase keeps the change type-only. The capture-disclosure test imports On the open threads: the cubic-dev-ai P2 on the capture-disclosure test (#3616 (comment)) is fixed at this head, so the author can resolve it. I did not run the typecheck, layering or wire-compat gates, and I did not re-read the full 206-file diff. I checked the range-diff changes and searched the head for importers of the daemon request and response types. The PR still has merge conflicts with main, and I did not look at the new conflict. Both Smoke Tests jobs are queued, not failed, and this diff only changes type imports and aliases, so it cannot change what the smoke route does at runtime. Before merge, please resolve the conflict with current main and let the queued Smoke Tests finish. |
5bf3aec to
7d007f5
Compare
|
The earlier conflict at 5bf3aec is fixed at 7d007f5, and I found no code problem in the diff. The open cubic-dev-ai thread on the DaemonWireRequest import in capture-disclosure.test.ts is fixed at this head, so you can resolve it: #3616 (comment). I read the diff and grepped the importers at this head to check that the type-only imports resolve. I did not run typecheck or the layering gates locally. The Compatibility & Provenance check was cancelled, not failed, and it has no failed-step log, so its result at this head is unknown. The only change in the latest delta is type imports in one daemon unit test, so I think it is unrelated, but the PR also touches test/wire-compat/closure-policy.ts entries from earlier rounds. Please re-run that check to green. After that I see nothing else before merge. One question on simplicity before this is ready: https://github.com/callstack/agent-device/blob/7d007f5/scripts/layering/daemon-client-entry.ts#L18 is now a flat ban on any edge from src/daemon-client/ into the daemon root, and LOGICAL_MODULE_POLICIES in architecture-ownership.ts already expresses that for ad-replay, maestro, replay-test and replay-port. Now that R78 and the R10 importer check are both flat bans, could the client ban be one entry there ({roots: ['src/daemon-client/'], forbiddenTargetRoots: [DAEMON_SOURCE_ROOT]})? Then the bans planned in #3625 for src/cli*, src/client and src/remote would be more entries in the same declaration, not more bespoke rule files with skip lists. If you take it, plant the test edge on src/daemon-client/daemon-client.ts so the test pins the removed allowance. If you considered this and kept the separate rule, a short reason is enough. |
…mon package The client, remote, and CLI import DaemonWireRequest from contracts and DaemonResponse/DaemonArtifact/DaemonResponseData from kernel, so nothing outside packages/daemon imports it. The daemon keeps its own DaemonRequest (wire request plus internal) and drops the response aliases. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The client's edges into the daemon package are a flat ban, so the daemon-client policy in LOGICAL_MODULE_POLICIES carries it instead of a separate rule, and R10 leaves the importers that policy covers to it. The wire-compat ledger acks the type-name-only digest changes and waives the DaemonWireRequest narrowing in the closure policy. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
7d007f5 to
824077b
Compare
|
Addressed at
|
|
This PR is ready. The code is clean at 824077b, and the earlier review at 7d007f5 is now resolved. The 19 checks at 824077b are green. Not blocking: lines 24 and 225 of docs/adr/0033-collocation-decision-table.md still name R78 ("R78's scope" and "(R13, R76, R78)") although this PR deletes that rule, so you can swap in the daemon-client logical-module policy or drop the mentions, or leave them. I did not run the layering gate locally. I checked by reading the code that each planted edge gives exactly one violation, and I did not run the tests. I also did not review the upstream main commits in the delta (replay-port, lease, settle, CI config), as they are out of scope. Nothing else stands between this PR and maintainer merge. |
thymikee
added
the
ready-for-human
|
…3623) * refactor(maestro): shape the replay backend as a create(host) plugin * chore(gates): pin the maestro replay backend plugin surface and closure approval * fix(maestro): assert the suggestion source, first trace write, and pinned help URLs * refactor(replay-port): tie each backend's declaration and plugin to its id, drop unread fields Both id-keyed tables are mapped over ReplayBackendId, so a declaration or plugin registered under another backend's id fails to type-check. The tables stay apart so the daemon routes that load backends do not evaluate help text. ReplayBackendPlugin.declaration and ReplayBackendDeclaration.apiVersion had no reader; the installed-plugin loader that would check them is not part of #3377 yet. * chore(gates): drop the #3616 over-ceiling approvals the merge-base now carries
1 participant
Summary
Step L1 of the daemon single-entrypoint stack: the wire request/response types leave
@agent-device/daemon, andsrc/daemon-clientno longer imports the daemon package at all.src/daemon-client,src/client,src/remoteand the CLI's wire-typed code useDaemonWireRequestfrom@agent-device/contracts/commandandDaemonResponse/DaemonArtifact/DaemonResponseDatafrom@agent-device/kernel/contracts.packages/daemon/src/core/daemon-request.tskeeps only the daemon's ownDaemonRequest(wire request plusinternal) andDaemonInvokeFn; the response aliases are gone and daemon internals import from kernel.daemon-cliententry inLOGICAL_MODULE_POLICIES(any edge fromsrc/daemon-client/into the daemon package, type-only included); R10's importer check bans production importers ofdaemon-request.ts/session-state.tsoutside the daemon and leaves the policy's roots to it. The recorded-edge inventories were empty after the move, so they are gone.DaemonWireRequest/DaemonWireRequestMetaare waived in the closure policy as narrowings of the listed kernel types.Not in this PR
The CLI and remote code still import value/type modules from the daemon package (
session-artifact-paths,daemon-stop, the device-claim modules,platform-request-scope,DaemonCommandRoute). Moving those, and banning@agent-device/daemonundersrc/cli*,src/client,src/remoteandsrc/daemon-client, is the next layer (L1b, #3625), stacked on this branch (stack #3626).Two commits: the implementation, then the enforcement edits (
chore(gates): …).Validation
On head
824077b38:pnpm format:check,pnpm typecheck,pnpm lint,pnpm check:layering,pnpm check:daemon-wire-compatandpnpm check:affected --runpass. Onecheck:affectedrun hit a process-spawn timing failure insrc/__tests__/daemon-registration-owner.test.ts(untouched by this diff); it passes in isolation twice and in the full rerun. No device-facing behavior changes.🤖 Generated with Claude Code