Repository navigation
refactor(daemon): move CLI-shared daemon modules out of the daemon package and gate CLI reachability (R78) - #3625
Conversation
…ckage The device-claim record, store, inspection and settlement, the session artifact paths and the stop path now live in daemon-contracts, which the CLI and the daemon both read. The claim reconciler, its owner-scoped composer and the recovery platform scope sit beside them; the app-log durable resource, its recovery and the app-log file policy sit in capture-kit with the other durable captures. The device identity decode moves to kernel, so no contracts module needs capture-kit for it. `device release --stale` and the platform runtimes it composes reach no daemon module, and `DaemonCommandRoute` is read from command-registry. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
R78 now walks static and dynamic edges from src/bin.ts and src/cli.ts and fails on any module they reach that imports the daemon package, naming the chain, on top of the folder ban over src/cli, src/client, src/remote and src/daemon-client. R10 leaves those importers to R78. The two moved entries that carry the durable-capture machinery are approved over the new-entry ceiling, and ADR 0032/0033 stop describing R78 as recording edges. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
thymikee
changed the title
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
7 issues found across 118 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/daemon-contracts/src/device-claim-recovery.ts">
<violation number="1" location="packages/daemon-contracts/src/device-claim-recovery.ts:4">
P3: This façade re-exports `OwnerScopedClaimRecoveryComposer` but omits the source module's exported `OwnerScopedClaimRecovery` return type, so package consumers cannot name that contract. Re-export it from this module.
(Based on your team's feedback about complete façade exports.) .</violation>
</file>
<file name="docs/adr/0033-collocation-decision-table.md">
<violation number="1" location="docs/adr/0033-collocation-decision-table.md:136">
P2: R78 does not ban every type-only edge in the CLI's transitive graph: a reached root module may import daemon types only. Distinguish the client-side direct-import ban from the runtime CLI-reach ban.</violation>
</file>
<file name="scripts/layering/daemon-client-entry.test.ts">
<violation number="1" location="scripts/layering/daemon-client-entry.test.ts:40">
P3: The protected-prefix cases no longer exercise direct `import()` edges; the dynamic imports in the root-chain test only test traversal. Add a dynamic import variant so regressions in direct client-side import checks are caught.</violation>
</file>
<file name="packages/daemon-contracts/src/recovery-platform-scope.ts">
<violation number="1" location="packages/daemon-contracts/src/recovery-platform-scope.ts:14">
P2: This sink silently drops shutdown recovery diagnostics when no diagnostics scope is active. `teardownDaemonSessions()` runs outside `withDaemonDiagnosticsScope`, so lifecycle binding-cleanup failures are lost; wrap that shutdown work in a daemon diagnostics scope with `logPath`.</violation>
</file>
<file name="packages/daemon-contracts/src/session-artifact-paths.ts">
<violation number="1" location="packages/daemon-contracts/src/session-artifact-paths.ts:92">
P1: The realpath check is skipped when any descendant of the parent is missing, so `sessions/link/new/app.log` passes lexically even if `link` points outside the sessions root. `openAppLogOutput` then creates the parent through that symlink and writes outside the root; validate the nearest existing ancestor and resolve the missing suffix before returning.</violation>
</file>
<file name="scripts/layering/daemon-client-entry.ts">
<violation number="1" location="scripts/layering/daemon-client-entry.ts:83">
P2: The walk consumes only `parseImports` edges, so literal `require()`/`createRequire()` calls never enter the graph and can load daemon internals without a violation. Extend the shared edge model to resolve literal runtime requires and add a reach-check case.
(Based on your team's feedback about shared import-edge coverage.)</violation>
</file>
<file name="docs/adr/0032-layering-graph-engine.md">
<violation number="1" location="docs/adr/0032-layering-graph-engine.md:88">
P3: This ADR sentence mischaracterizes R78 as a flat ban, while the shipped rule performs a BFS over import edges and reports reachability chains. Describe R78 as a reachability ban rather than saying it no longer records edges.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| if (path.basename(resolved) !== input.basename) { | ||
| throw new Error(`${input.label} is outside the daemon-owned sessions directory`); | ||
| } | ||
| if (fs.existsSync(root) && fs.existsSync(path.dirname(resolved))) { |
There was a problem hiding this comment.
P1: The realpath check is skipped when any descendant of the parent is missing, so sessions/link/new/app.log passes lexically even if link points outside the sessions root. openAppLogOutput then creates the parent through that symlink and writes outside the root; validate the nearest existing ancestor and resolve the missing suffix before returning.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/daemon-contracts/src/session-artifact-paths.ts, line 92:
<comment>The realpath check is skipped when any descendant of the parent is missing, so `sessions/link/new/app.log` passes lexically even if `link` points outside the sessions root. `openAppLogOutput` then creates the parent through that symlink and writes outside the root; validate the nearest existing ancestor and resolve the missing suffix before returning.</comment>
<file context>
@@ -74,3 +75,30 @@ export function resolveSessionAppLogPath(sessionsDir: string, address: string):
+ if (path.basename(resolved) !== input.basename) {
+ throw new Error(`${input.label} is outside the daemon-owned sessions directory`);
+ }
+ if (fs.existsSync(root) && fs.existsSync(path.dirname(resolved))) {
+ const realRoot = fs.realpathSync.native(root);
+ const realParent = fs.realpathSync.native(path.dirname(resolved));
</file context>
| importing `packages/daemon/src/` at all; R78 bans every edge from | ||
| the CLI, typed client, remote transports and `src/daemon-client/` into `packages/daemon/src/`, and |
There was a problem hiding this comment.
P2: R78 does not ban every type-only edge in the CLI's transitive graph: a reached root module may import daemon types only. Distinguish the client-side direct-import ban from the runtime CLI-reach ban.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/adr/0033-collocation-decision-table.md, line 136:
<comment>R78 does not ban every type-only edge in the CLI's transitive graph: a reached root module may import daemon types only. Distinguish the client-side direct-import ban from the runtime CLI-reach ban.</comment>
<file context>
@@ -133,9 +133,9 @@ the whole `daemon-contracts` zone has moved into `packages/daemon-contracts` —
-importing `packages/daemon/src/` at all; R78 keeps any client→daemon
-edge at zero runtime and a five-line recorded type-only residue, and #3288's `daemon-contracts`
-zone was the declaration of that shared-below-both ownership — the extraction makes it
+importing `packages/daemon/src/` at all; R78 bans every edge from
+the CLI, typed client, remote transports and `src/daemon-client/` into `packages/daemon/src/`, and
+#3288's `daemon-contracts` zone was the declaration of that shared-below-both ownership — the extraction makes it
</file context>
| importing `packages/daemon/src/` at all; R78 bans every edge from | |
| the CLI, typed client, remote transports and `src/daemon-client/` into `packages/daemon/src/`, and | |
| importing `packages/daemon/src/` at all; R78 bans direct imports from the CLI, typed client, remote transports and | |
| `src/daemon-client/` (including type-only imports), plus runtime daemon imports reachable from CLI entries. |
| return Object.freeze({ | ||
| signal: processLifetimeSignal, | ||
| diagnostics: Object.freeze({ | ||
| emit: (event: PlatformDiagnosticEvent) => emitDiagnostic(event), |
There was a problem hiding this comment.
P2: This sink silently drops shutdown recovery diagnostics when no diagnostics scope is active. teardownDaemonSessions() runs outside withDaemonDiagnosticsScope, so lifecycle binding-cleanup failures are lost; wrap that shutdown work in a daemon diagnostics scope with logPath.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/daemon-contracts/src/recovery-platform-scope.ts, line 14:
<comment>This sink silently drops shutdown recovery diagnostics when no diagnostics scope is active. `teardownDaemonSessions()` runs outside `withDaemonDiagnosticsScope`, so lifecycle binding-cleanup failures are lost; wrap that shutdown work in a daemon diagnostics scope with `logPath`.</comment>
<file context>
@@ -0,0 +1,20 @@
+ return Object.freeze({
+ signal: processLifetimeSignal,
+ diagnostics: Object.freeze({
+ emit: (event: PlatformDiagnosticEvent) => emitDiagnostic(event),
+ }),
+ progress: Object.freeze({
</file context>
| edges: readonly ResolvedImportEdge[], | ||
| ): LayeringViolation[] { | ||
| const outgoing = new Map<string, ResolvedImportEdge[]>(); | ||
| for (const edge of edges) { |
There was a problem hiding this comment.
P2: The walk consumes only parseImports edges, so literal require()/createRequire() calls never enter the graph and can load daemon internals without a violation. Extend the shared edge model to resolve literal runtime requires and add a reach-check case.
(Based on your team's feedback about shared import-edge coverage.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At scripts/layering/daemon-client-entry.ts, line 83:
<comment>The walk consumes only `parseImports` edges, so literal `require()`/`createRequire()` calls never enter the graph and can load daemon internals without a violation. Extend the shared edge model to resolve literal runtime requires and add a reach-check case.
(Based on your team's feedback about shared import-edge coverage.) </comment>
<file context>
@@ -29,11 +66,53 @@ export function checkDaemonClientEntry(edges: readonly ResolvedImportEdge[]): La
+ edges: readonly ResolvedImportEdge[],
+): LayeringViolation[] {
+ const outgoing = new Map<string, ResolvedImportEdge[]>();
+ for (const edge of edges) {
+ if (edge.typeOnly && !edge.dynamic) continue;
+ if (edge.file.startsWith(DAEMON_SOURCE_ROOT)) continue;
</file context>
| export type { ClaimRecoveryGatewayFactory } from './claim-recovery-gateway.ts'; | ||
| export { | ||
| createOwnerScopedDeviceClaimReconciler, | ||
| type OwnerScopedClaimRecoveryComposer, |
There was a problem hiding this comment.
P3: This façade re-exports OwnerScopedClaimRecoveryComposer but omits the source module's exported OwnerScopedClaimRecovery return type, so package consumers cannot name that contract. Re-export it from this module.
(Based on your team's feedback about complete façade exports.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/daemon-contracts/src/device-claim-recovery.ts, line 4:
<comment>This façade re-exports `OwnerScopedClaimRecoveryComposer` but omits the source module's exported `OwnerScopedClaimRecovery` return type, so package consumers cannot name that contract. Re-export it from this module.
(Based on your team's feedback about complete façade exports.) .</comment>
<file context>
@@ -0,0 +1,6 @@
+export type { ClaimRecoveryGatewayFactory } from './claim-recovery-gateway.ts';
+export {
+ createOwnerScopedDeviceClaimReconciler,
+ type OwnerScopedClaimRecoveryComposer,
+} from './device-claim-owner-recovery.ts';
+export { createRecoveryPlatformScope } from './recovery-platform-scope.ts';
</file context>
| type OwnerScopedClaimRecoveryComposer, | |
| type OwnerScopedClaimRecoveryComposer, | |
| type OwnerScopedClaimRecovery, |
| ]) { | ||
| test(`${file} importing a daemon module is red, type-only imports included`, () => { | ||
| const specifier = relative(file); | ||
| for (const source of [`import '${specifier}';`, `import type { scope } from '${specifier}';`]) { |
There was a problem hiding this comment.
P3: The protected-prefix cases no longer exercise direct import() edges; the dynamic imports in the root-chain test only test traversal. Add a dynamic import variant so regressions in direct client-side import checks are caught.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At scripts/layering/daemon-client-entry.test.ts, line 40:
<comment>The protected-prefix cases no longer exercise direct `import()` edges; the dynamic imports in the root-chain test only test traversal. Add a dynamic import variant so regressions in direct client-side import checks are caught.</comment>
<file context>
@@ -1,61 +1,85 @@
+]) {
+ test(`${file} importing a daemon module is red, type-only imports included`, () => {
+ const specifier = relative(file);
+ for (const source of [`import '${specifier}';`, `import type { scope } from '${specifier}';`]) {
+ const violations = check({ [file]: `${source}\n` }).filter((v) => v.file === file);
+ assert.equal(violations.length, 1, source);
</file context>
| for (const source of [`import '${specifier}';`, `import type { scope } from '${specifier}';`]) { | |
| for (const source of [`import '${specifier}';`, `import type { scope } from '${specifier}';`, `void import('${specifier}');`]) { |
| | R14 non-TS file under `src/utils` | flag | ✓ | ✗ | ✗ | | ||
| | R71 file under `src/replay` | flag | ✓ | ✓ | ✓ generic | | ||
|
|
||
| _R78 and R10 no longer record edges: both are flat bans, so the "unrecorded" and "stale" rows describe the spike at its date, not the rules as shipped._ |
There was a problem hiding this comment.
P3: This ADR sentence mischaracterizes R78 as a flat ban, while the shipped rule performs a BFS over import edges and reports reachability chains. Describe R78 as a reachability ban rather than saying it no longer records edges.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At docs/adr/0032-layering-graph-engine.md, line 88:
<comment>This ADR sentence mischaracterizes R78 as a flat ban, while the shipped rule performs a BFS over import edges and reports reachability chains. Describe R78 as a reachability ban rather than saying it no longer records edges.</comment>
<file context>
@@ -85,6 +85,8 @@ finding. Clean tree: custom 0, depcruise 10 (3 R6 survivors, 7 recorded R78 edge
| R14 non-TS file under `src/utils` | flag | ✓ | ✗ | ✗ |
| R71 file under `src/replay` | flag | ✓ | ✓ | ✓ generic |
+_R78 and R10 no longer record edges: both are flat bans, so the "unrecorded" and "stale" rows describe the spike at its date, not the rules as shipped._
+
Custom prints `file:line`, rule id and hint (`report()` in `scripts/layering/check.ts`); depcruise
</file context>
| _R78 and R10 no longer record edges: both are flat bans, so the "unrecorded" and "stale" rows describe the spike at its date, not the rules as shipped._ | |
| _R10 remains a direct ban, while R78 also walks CLI reachability over static and dynamic edges; the "unrecorded" and "stale" rows describe the spike at its date, not the rules as shipped._ |
Size Report
Startup median (7 runs, lower is better):
|
|
The reach walk in R78 duplicates R82's walk, so this needs one change before merge. The other code looks fine at 6b96029, and CI is green (43 checks, 0 failing, including check:layering and the eager-closure budget test). I did not run check:layering or typecheck locally, so "the CLI reaches no daemon module" rests on that CI result, and I did not recount the subpath exports or check removed subpaths for leftover importers beyond what typecheck proves.
Should daemon-contracts own the device-claim reconciler at all? It now depends on capture-kit at runtime because the reconciler drives app-log, audio-probe and perf-capture recovery, and it holds a process-lifetime On the open review threads, the realpath skip for a missing dirname (P1) and emitDiagnostic returning early without a scope (P2) both exist on the base commit and are moved verbatim here. They are fine as separate fixes. Two lower-priority threads still apply: the missing dynamic-import case in the folder-ban tests, and the ADR 0032 "flat ban" wording. These do not apply, so please resolve them: ADR 0033 and type-only edges (the ADR describes the folder ban, which does cover type-only edges), require()/createRequire (no production use exists, and this is a limit of the shared import parser, not R78), and OwnerScopedClaimRecovery missing from the facade (no consumer names it). |
This branch has not been deployed
1 participant
Summary
L1b of the daemon single-entrypoint stack, on top of #3616: the CLI process reaches no module of
@agent-device/daemon, and R78 now proves it by reachability instead of a folder ban.daemon-contracts: the device-claim cluster (record, store, inspection, reboot, settlement, claims),session-artifact-paths(with the managed-path check),daemon-stop, and the claim reconciler + owner-scoped composer + recovery platform scope.capture-kit: the app-log durable resource, its store and recovery, and the app-log file policy (they sit with the other durable captures).kernel/device:decodeDeviceIdentity(was in capture-kit), sodaemon-contractsdoes not depend on capture-kit for claim records.DaemonCommandRouteis read fromcommand-registry.device release --stalecomposes from root platform runtimes (src/platform-runtime-claim-recovery.ts) and reaches no daemon module.src/cli/commands/device-release.tsis gone.daemon-client-entry) keeps the folder ban oversrc/cli.ts,src/cli/,src/client/,src/remote/,src/daemon-client/and adds a breadth-first walk over static and dynamic edges fromsrc/bin.ts/src/cli.ts. The test plants the exact laundering chaincli.ts → cli/commands/device.ts → platform-runtime-stale-claim-release.ts → daemon, a type-only hop that must not count, and a stale-entry case. On the first run it also foundplatform-runtime-app-log-output.ts/-process.tsimportingapp-log-filesandmanaged-session-artifact-pathfrom the daemon; both moved too.AGENTS.mdsession-artifact-paths pointer follows the move.Subpath exports
daemondaemon-contractsdevice-claims,device-claims-fixtures,device-claim-recovery,daemon-stop,session-artifact-paths)capture-kitapp-log-durable-resource,app-log-resource-recovery,app-log-files)kernel,host-kit,test-kitNet: 210 → 207. The device-claim cluster is one explicit-name entry (
device-claims) instead of seven files exported separately. Two judgment calls to review:daemon-stopkeeps its own subpath. It lazily importsdaemon-registration-owner, and its test mocks that seam, so merging it into the owner module (or re-exporting it from one) would either make a static cycle or break the test.daemon-contracts, which depends oncapture-kit. The reverse (capture-kit → daemon-contracts) is a TypeScript project-reference cycle throughcommand-registry → selectors → capture-kit, and there is no package above both. The dependency stays acyclic; if the layering intent is "contracts below capture", the reconciler needs a new package, which I did not create.Eager closure
scripts/__tests__/eager-closure-budgets.test.tspasses with no growth on any existing entry. Three new entries carryAPPROVED_OVER_CEILINGrows (app-log-durable-resource.ts,device-claim-recovery.ts,device-claims.fixtures.ts): each is code moved verbatim from a daemon entry of the same weight, and only the daemon and the lazily loadeddevice release --stalepath import them. The first draft widenedcapture-kit's root barrel (32 → 63 modules) anddaemon-resolution; both were reverted to separate entries for that reason. Four tests that needtest-kitstay underpackages/daemon(test-kit depends on daemon-contracts, and capture-kit/daemon-contracts tests cannot import it): the claim, settlement, reconciliation and app-log-files tests exercise the moved modules through their public entries.Diff size
118 files, +744 −582 over #3616: over the 1,000-line budget. Most of it is moves counted twice (rename detection does not pair the merged or split files: the kernel decode move is +85/−90, the claim reconciler, owner recovery and scope moves another ~300) and import rewrites across the daemon tests. If you want this under budget I would split the kernel/capture-kit moves (
decodeDeviceIdentity, app-log durable resource and file policy) into a lower layer.Validation
pnpm check:affected --runpasses on6b960298f(L1b) and on7d007f5cf(#3616), both rebased on origin/main542fb77;pnpm check:daemon-wire-compatpasses on both. Earlier L1b attempts failed on the eager-closure budget,production-exportsandfallow(all fixed above) and twice ontmpdir-leaks-modelwhile two other threads' gates ran on this host; that check passes alone twice and in the final runs. A fresh-context review of the first L1b draft found only stale path references (fixed). The R78 reachability walk changed the design after that review, so the final shape has not been reviewed cold.No device-facing behavior changes;
device release --staleruns the same reconciler code, composed from the same root gateway.🤖 Generated with Claude Code