Repository navigation
Conversation
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. |
…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>
…request importers R78 and R10 inventories are empty, so both become bans. 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>
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. |
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-client-entry) bans anysrc/daemon-client→ daemon-package edge; R10 (daemon-modularity) bans any production importer ofdaemon-request.ts/session-state.tsoutside the daemon. Both inventories were empty after the move, so both rules became flat bans.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
7d007f5cf: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