refactor(daemon): split src/daemon/types.ts into request types and session state - #2346
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
The type split and ref-frame ownership look sound at 688b1a5, but Coverage found two changes this PR still needs:
Please rerun those focused gates and update the validation note. These failures are distinct from the reported root-user permission tests. |
|
Both are fixed in Wire closure ( Test-size ratchet (
No assertion weakened. Splitting these files along the modules they mirror is the standing remedy, but none of those source modules split in this PR, so it stays out. Focused gates at Validation note in the PR body updated to Generated by Claude Code |
|
The follow-up b954217 addresses both gate findings. Wire-closure entries follow the moved declarations, and the test files are back within their existing limits without losing assertions. The type split and ref-frame ownership remain clean. Ready for human review; the new head still has CI running, so merge readiness awaits those results. |
|
Update: Android Smoke has now failed on b954217 at wait for Alert result: cancelled, the same canary failing on other PRs. The two code/gate findings are resolved, but I removed ready-for-human until the required smoke lane passes. |
thymikee
left a comment
There was a problem hiding this comment.
Adversarial review from the #2335 coordinator, at b954217. I checked the load-bearing claims against the tree rather than reading the body. The work is sound; one finding below changes #2341's plan and should be recorded before this merges.
Verified independently: src/daemon/types.ts is gone with no re-export shim; zero suppressions anywhere in the diff; DaemonWireRequest imports only @agent-device/contracts/command, @agent-device/kernel/contracts and @agent-device/kernel/device — there is genuinely no import path from it to session-state.ts, so the headline claim holds structurally and not just by the type test.
I checked the three ratchet-balancing edits in beaaef7 specifically, because "shed one line to stay at base length" is exactly where an assertion quietly dies. It does not here. In snapshot-handler.test.ts and find.test.ts the dropped toHaveLength(n) is strictly implied by the adjacent toEqual on an explicit n-element array literal — including find.test.ts, where the cap of 5 is the point of the test and remains pinned by the literal. The third is a pure two-line reformat of the same toHaveLength(0). The commit body's "no assertion is weakened" is accurate.
That said, this is ratchet-driven editing of tests the ratchet wants split, and you say so honestly. Please open a follow-up for splitting those three files along the modules they mirror, and reference it in the commit body, so the deferral is tracked rather than remembered.
Finding — DaemonWireRequest cannot serve #2341 where it lives, and #2341 does not actually need it.
#2341's completion conditions are that the session-journal package "imports nothing under src/ (R11)" and that its input type has no path to SessionState or DeviceLease. DaemonWireRequest is declared at src/daemon/daemon-request-wire.ts — under src/ — so a workspace package may not import it. As exported, it cannot be F1's input type.
It also does not need to be. @agent-device/kernel/contracts already exports DaemonRequest and DaemonRequestMeta carrying exactly the four fields the journal reads (command, flags, meta.requestId, meta.clientArtifactPaths), in a package F1 may depend on, with no internal key and no session-state path. Your own module is built by narrowing that very type. So F1 should type its inputs on @agent-device/kernel/contracts directly, and #2341's "or kernel/contracts public request/meta types" clause is the path that actually applies rather than the fallback it was written as.
Nothing to change here — this PR satisfies #2338 as written, and DaemonWireRequest remains the right shape for src/-side consumers. I am recording it so F1 is not built against an import it cannot legally make. One line in your Summary naming kernel/contracts as F1's actual source would carry that forward.
I am keeping F1 sequenced behind this PR regardless: it relocates session-event-*.ts, which your 408-file diff also touches, so the ordering still buys conflict avoidance even though the type dependency has dissolved.
Two chore(gates) commits with a test commit between them departs from the "implementation first, one final chore(gates)" convention — both later commits are CI fixes, and this repo squash-merges, so I would not rewrite history for it. Noting it only so the pattern is not taken as precedent.
Generated by Claude Code
|
This head now conflicts with main. The code review remains clean, but conflict resolution and a passing Android smoke lane are needed before merge. |
b954217 to
0fa63fe
Compare
…e modules `src/daemon/types.ts` served two audiences from one file: the dispatch request shape and the daemon's live session record. It also sat in the only daemon type cycle — it imported `RefFrame` from `ref-frame.ts`, which imported `SessionState` back — so neither file could be read in isolation. Three modules replace it, each importing only downward: - `daemon-request-wire.ts` declares `DaemonWireRequest`: a dispatched request with no `internal` key and no property path to `SessionState` or `DeviceLease`, so a consumer can read a request's command, flags and public metadata without depending on the session record. - `daemon-request.ts` adds the daemon-only half (`DaemonRequestInternal`, which stays unexported) plus the response vocabulary. - `session-state.ts` owns `SessionState` and the shapes only it holds. The cycle is cut by `ref-frame-slot.ts`, declared below both `ref-frame.ts` and `session-state.ts`: it owns the frame VALUE (the class stays unexported, so the type remains nominal and unconstructible from outside), while `ref-frame.ts` keeps every lifetime transition and every `session.refFrame` write. No behavior change: every importer moves to the module owning the symbol it uses, with no re-export shim at the old path. `client-normalizers.ts` takes `SessionRuntimeHints` from `@agent-device/kernel/contracts`, which declares it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ujrc8LYmvM249WY8921J1Y
A type-level walk over `DaemonWireRequest` fails `tsc` if the shape regains an `internal` key or grows a property path back to `SessionState` or `DeviceLease`. Positive controls over `DaemonRequest` prove the walk finds both when they are there, so a walk that never matches anything cannot pass by accident. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ujrc8LYmvM249WY8921J1Y
Splitting `daemon/types.ts` turns one combined import into two in every file that used both halves. Three of those test files are already over the 1,000-line tripwire, where the size ratchet allows no growth, so each sheds one line that was carrying nothing: - `snapshot-handler.test.ts` and `find.test.ts` each drop a `toHaveLength` assertion an adjacent `toEqual` on an explicit array literal already makes. - `session-replay-repair-transaction.test.ts` names the filtered close actions instead of wrapping the expression across three lines inside `expect`. No assertion is weakened and no test content is removed. Splitting these files along the modules they mirror is the standing remedy, but none of those modules split here, so it stays out of this change and is tracked in #2353. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ujrc8LYmvM249WY8921J1Y
…e split modules R7 now locates the `SessionState` declaration by the declaration itself rather than by a recorded path: `sessionStateWritePressure` measures the merge-base tree too, and that tree still declares it in `daemon/types.ts` — a path constant would measure it as zero pressure and bank the headroom. R10's external-importer ratchet covers all three modules that replaced `daemon/types.ts`, so moving a symbol between them cannot reopen the boundary to a new outside zone. The recorded membership is unchanged: `client-normalizers.ts` and `remote/daemon-artifacts.ts` both import `daemon-request.ts` only. The daemon RPC closure gate waives `DaemonRequest`, `DaemonResponse` and `DaemonArtifact` by path, so those three keys follow the declarations to `daemon-request.ts`. `DaemonRequest`'s rationale now says what it is — the server-side narrowing of the kernel declaration that fixes the wire shape — rather than calling it a re-export alias. The `live-state-shape` and session-resource declaration sites move with `SessionState`; the depgraph lookalike fixture takes a new plausible path now that `daemon/session-state.ts` is the real root. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ujrc8LYmvM249WY8921J1Y
0fa63fe to
6f2fe7b
Compare
|
Smoke Tests failure on Failing check: Smoke Tests / iOS lane — Why it is not this PR's:
Note this is a different lane from the earlier failure on this PR. That one was the Android emulator smoke ( No fix exists to port: I checked the open PRs and none targets this XCUITest. If the re-run comes back red I will say so rather than re-running again. Generated by Claude Code |
|
Independent review at |
|
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
Closes #2338.
src/daemon/types.tsserved the dispatch request shape and the live session record from one file, and sat in the only daemon type cycle. Three modules replace it:src/daemon/daemon-request-wire.ts—DaemonWireRequest: nointernalkey, no property path toSessionStateorDeviceLease, satisfying the wire-only requirement in refactor(daemon): split src/daemon/types.ts into request types and session state #2338.src/daemon/daemon-request.ts—DaemonRequest= wire +internal(DaemonRequestInternalstays unexported), plusDaemonResponse/DaemonResponseData/DaemonInvokeFn/DaemonArtifact.src/daemon/session-state.ts—SessionState,SessionRef,SessionScope,SessionRuntimeHints, and the interaction-surface shapes.For F1 (#2341):
DaemonWireRequestlives undersrc/, so a journal package cannot import it under R11. The importable equivalent isDaemonRequest/DaemonRequestMetafrom@agent-device/kernel/contracts, which already carry all four fields the journal reads (command,flags,meta.requestId,meta.clientArtifactPaths).The cycle is cut by
ref-frame-slot.ts, declared below bothref-frame.tsandsession-state.ts. It owns the frame value (class still unexported, so the type stays nominal and unconstructible outside);ref-frame.tskeeps every transition andsession.refFramewrite — R7 rowsrefFrameandsnapshotScopeSourceare unchanged.No behavior change; no re-export shim at the old path. 408 files touched, 386 of them import-line-only — the rename/move exemption applies to the diff budget.
R9 type cycles (production graph), measured on the current base
b4ebd77: 19 → 18.src/daemon/ref-frame.ts ↔ src/daemon/types.tsis gone; the other 18 are byte-identical, so no new cycle appeared. Largest cycle unchanged at 6 files (packages/provider-webdriver/*).Production importer split after the change: 197 files — 82 request-only, 42 state-only, 73 both (issue baseline: 79/35/83).
Path-keyed gates follow the declarations, in one final
chore(gates)commit: the daemon-modularity external-importer ratchet (membership unchanged —client/client-normalizers.tsandremote/daemon-artifacts.ts, both ondaemon-request.ts) and the wire-compat closure waivers. R7 now findsSessionStateby its declaration rather than a recorded path, because the same function measures the merge-base tree.Three test files already over the 1,000-line ratchet gain one import line each and shed one redundant line to stay at base length — no assertion weakened. Splitting them along the modules they mirror is tracked in #2353.
Validation
Rebased onto
b4ebd77. One conflict, indaemon/deferred-interaction-outcome.ts: #2347 retargetedrunPostGestureStabilityLoopto@agent-device/capture-kit/post-gesture-stabilitywhile this PR split the type import on the next line — resolved by taking both. (The earlier rebase onto835af32had one conflict inhandlers/session-app-source-deployment.ts, resolved the same way.)Tested at
6f2fe7b(the pushed head) afterpnpm install --frozen-lockfile && pnpm build.pnpm check:affected --run: format, lint, typecheck, layering, di-seams, fallow, gate-manifest, build, package, integration and macos-coverage all pass. Layering guard green against merge-baseb4ebd77: R7 (31 fields classified), R9 (largest cycle 6), R10 (R7 pressure at base, 2 external importers). Fallow: no issues in 408 changed files.vitest-related: 3149 passed, 3 failed, none from this change.app-log-session-resource.test.ts > unwritable tombstone…anddurable-capture-resource.test.ts > an unconfirmed failed-adoption cleanup…fail identically onorigin/mainin this container, which runs as root, so theirchmod-based unwritable-tombstone setup cannot deny a write.cli-config-trust.test.ts > project config rejects remote connection fields…is untouched by this diff and passes in isolation here — a load artifact of the local run. CI is the authority on all three.The type-level test is
src/daemon/__tests__/daemon-request-wire.test.ts; I verified it failstscwhen aninternal/SessionStatepath is reintroduced.🤖 Generated with Claude Code
https://claude.ai/code/session_01Ujrc8LYmvM249WY8921J1Y
Generated by Claude Code