[thymikee]

Summary

Extract daemon transport, request guards, lifecycle and launch identity into private @agent-device/daemon-client. Its single entry names APIs explicitly; lifecycle implementations stay lazy. Related: #3276.

Scope: 171 files, mainly moves and consumer rewiring. Fingerprints live in plugins; RPC envelopes live in contracts. Root composition tests retain real registered-daemon and MCP coverage.

Production observation and recorded-daemon tools now share one admission reader. Missing/foreign registrations retain nullable/not-running behavior. Present unreadable, corrupt or unusable records retain their metadata and fail with daemon-record-unreadable; unproven ownership keeps its refusal. Pinned clients retain binding-mismatch diagnostics. Benchmark refusals preserve cell-state and both errors if cleanup fails. PID/port decoding and error constructors each have one owner, without value cycles.

Rebased onto 3a218f573f after #4097, preserving both package configurations.

Review fixes document provider trimming and verbatim envelope forwarding, preserve the virtual drain's real exit/deadline checks, and compare shared build-output contents rather than directory timestamps.

Validation

Tested head: aac253f77ecb06fe10e850943fab631a1e5b5fb2.

• pnpm check:affected --run --head HEAD: passed at the exact unchanged commit (run-GFwZPd). Entry closure remains 126; main merge-tree is clean.
• Focused package/tooling, composed cleanup and owner-file bundle tests passed. Isolated regression experiments detect corrupt-to-absent admission, lost benchmark classification and in-place bundle overwrites; the old timestamp guard misses the overwrite. Static error imports grow closure from 126 to 130, so laziness is retained.
• Earlier cross-build smoke remains attributed to bc137013e83a; no new manual device run. Fresh GitHub native lanes, Coverage and configured reviews must pass at this published head.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
agent-device-website	Ready	Preview	Oct 31, 2026 8:50am UTC

[github-actions]

Size Report

Metric	Base	Current	Diff
Installed (including dependencies)	5.63 MB	5.63 MB	+2.5 kB
Package (unpacked)	5.63 MB	5.63 MB	+2.5 kB
Package (download)	1.71 MB	1.71 MB	+864 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	40.1 ms	40.1 ms	+0.1 ms
CLI --help	87.3 ms	86.4 ms	-0.8 ms

[cubic-dev-ai]

All reported issues were addressed across 164 files

<sub>Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic</sub>

[thymikee]

I found no code problem at 7b31aa0 that must change before merge, but the Apple Smoke Tests job is still running, so CI is not green yet.

That job runs the built CLI through the relocated daemon-client package (ensure, attach and the iOS and macOS phases), so it is the live check for this move. It must finish green on 7b31aa0. This change is not device-facing, so no manual device run is needed beyond that job.

Not blocking, and you can take or leave these: readRecordedDaemon in scripts/lib/recorded-daemon.ts copies readDaemonInfo, so it drops the empty-token and port checks and the recorded transport, and a dual daemon is now tried http-first instead of socket-first. One function should read a daemon record into DaemonInfo: export readDaemonInfo from the @agent-device/daemon-client entry, return its result from this script helper and delete RecordedDaemon. That also closes the two Cubic threads on this reader. The lease method names are still literal tables in http-rpc-methods.ts and daemon-proxy.ts, so lease-scope.ts is a third copy; either move one table next to the daemon HTTP contract and have both read it, or drop "vocabulary" from the commit text. ensure-daemon.ts and src/host.ts now import the single entry, whose static imports add the request path to what ensure loads; that may be an acceptable cost of one entry. store.ts re-exports legacyStoreManifests only for connection.ts, so import it there from store-legacy.ts and delete the re-export.

Of the open Cubic threads, these still apply: the missing dispatched === 'no' assertion (#4101 (comment)), the dropped ensure-per-call assertions (#4101 (comment) and #4101 (comment)), the transport order (#4101 (comment)), the empty-token reader (#4101 (comment)), the leaked stub server (#4101 (comment)) and the inert fixture line in root-module-zones (#4101 (comment)). The index-host mock thread (#4101 (comment)) applies but is low value, because the real route is covered by src/__tests__/daemon-client-host.test.ts. I did not verify the README pointer thread (#4101 (comment)).

These two do not apply and can be resolved: the missing registration-claim check (#4101 (comment)), because the old scripts also sent with the recorded token and no claim check, and the provider trimming (#4101 (comment)), because the fingerprint code is a byte-identical move of existing behavior (file it separately if you want it changed).

Before merge, wait for Apple Smoke Tests to go green on 7b31aa0, restore the dropped MCP ensure assertions, and reuse readDaemonInfo in the recorded-daemon reader. I did not rerun the unit, provider-integration or wire-compat suites, and I did not read the wire-compat ledger delta.

[thymikee]

Thanks for the review. Pushed 5f60896; all 11 inline threads are answered and resolved.

• The recorded-daemon reader now validates token, endpoint, and registration claim through the existing process-lock owner, keeps the recorded transport mode (socket-first), and has tests for each.
• The MCP lifecycle test restores the per-host ensure assertions with the public factory spied, not replaced. The package test covers ensure-once per transport.
• I did not export readDaemonInfo from the package entry or collapse RecordedDaemon into it; the script reader reuses the existing lock and transport owners instead. That consolidation, the lease-method table duplication, and the legacyStoreManifests re-export are left for a follow-up to keep this PR a move.
• Apple Smoke Tests must go green on the new head; I did not run the two live-lane script sends locally.

[cubic-dev-ai]

All reported issues were addressed across 10 files (changes from recent commits).

<sub>Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic</sub>

[apex-sentinel-staging]

Apex Sentinel

Verdict: approve

Since the last review: 8 new · 0 still open · 3 no longer raised

8 finding(s): 3 minor, 5 nit. Verdict: approve. reached max rounds (1).

🟡 Minor (3)

Where	Finding
scripts/ios-snapshot-benchmark/cell-admission.ts:206	🆕 Benchmark cell admission loses its typed error when the lock claim is unproven
test/output-economy/owner-files-no-leak.test.ts:2	🆕 owner-files-no-leak guard passes trivially when dist/ is absent, and its mtime check cannot see an in-place overwrite
scripts/ios-snapshot-benchmark/cell-admission.ts:206	🆕 Benchmark cell admission now reports an unproven registration as "daemon not running" and drops its typed error

🔵 Nit (5)

Where	Finding
packages/daemon-client/src/daemon-client-metadata.ts:114	🆕 readDaemonInfo now rejects records without a valid pid, changing "unreadable record" into "no daemon" for non-client writers
packages/contracts/src/lease-scope.ts:565	🆕 provider param is no longer trimmed while leaseProvider in meta still is
packages/daemon-client/src/daemon-client-metadata.ts:86	🆕 readRegisteredDaemon imports daemon-lifecycle-errors.ts on every call instead of importing it statically
packages/daemon-client/src/daemon-client-startup.ts:93	🆕 Doc comment dropped from the reshaped exported observeLocalDaemon
packages/plugins/src/store.ts:1	🆕 ./store-legacy.ts import placed above the node builtins in store.ts

Not in the reviewers' prompt (1 file)

• pnpm-lock.yaml: generated or a lockfile, so its lines were left out

Reply under an inline comment with @apex-sentinel-staging note ... or @apex-sentinel-staging pivot ... to re-check that finding.
Comment @apex-sentinel-staging review on this conversation to run a full review.

[apex-sentinel-staging]

Apex Sentinel: 2 new findings on 5f60896. The verdict and the full list are in the summary comment.

[apex-sentinel-staging]

Apex Sentinel: 4 new findings on bc13701. The verdict and the full list are in the summary comment.

[thymikee]

Thanks for the update. This review covers bc13701; the newer c5537a0 only regroups the store.ts export and does not change the finding below. At bc13701 the earlier findings from 7b31aa0 are fixed, except one: the recorded-daemon script still keeps its own copy of the daemon-client admission rule.

scripts/lib/recorded-daemon.ts:70 reads daemon.json and checks the registration claim with its own code. Lines 28-52 repeat readDaemonInfo (daemon-client-metadata.ts:19). Lines 70-83 repeat registrationAllowsDaemonObservation and daemonRegistrationUnprovenError (daemon-client-startup.ts:169), with the same message and the same daemon_registration_unproven reason. The two rules already differ. The script rejects pid <= 0 and port > 65535, but readDaemonInfo maps a bad pid to 0 and has no upper port bound. For a foreign held claim, observeLocalDaemon returns null, but the script throws "unproven". This only affects tooling (perf forced-backend, iOS live-runner conformance, snapshot-benchmark cell admission, Android live observability), not a production route. The next change to record admission or the claim rule will not reach these lanes, and tests on either side will stay green. Exactly one daemon-client function should decide whether the daemon a state directory records may be used without starting it. Please move the first four steps of observeLocalDaemon into an exported readRegisteredDaemon(paths): DaemonInfo | null next to readDaemonInfo, and have observeLocalDaemon call it. Then make sendToRecordedDaemon, cell-admission.ts:207 and live-observability-scenario.ts:206 use it, and delete RecordedDaemon, readRecordedDaemon, the inline lock check and the hand-built AppError. A grep for daemon_registration_unproven outside tests should then return only daemon-client-startup.ts.

Could sendToRecordedDaemon then shrink to resolveDaemonPaths, readRegisteredDaemon and sendDaemonRequest(..., timeoutRecovery 'none')? That would delete about 40 lines of copied logic. Nothing has to change first, because the module that holds readDaemonInfo is already loaded by the entry, so the export adds no eager closure.

One non-blocking note still stands: packages/plugins/src/store.ts:10 re-exports legacyStoreManifests between imports. Import it from store-legacy.ts in its consumer and delete the re-export (#4101 (comment)). No other open P1 or P2 thread still applies. These can be resolved if still open: the fixed ones are r4424651951, r4424651958, r4424651961, r4424651970, r4424651975, r4424651983, r4424651991, r4424651995, r4424652001, r4424652007, r4424652010, r4425465608 and r4425638631 (all under #4101 (comment)_), and the points that need no change are r4425638636 (the absent lock matches the production rule; the real issue is the copy above), r4426889884 (both digest sides call the same providerCredentialFingerprint), r4426889897 (the added refusals are the requested claim check, and the smoke conformance lane that sends through it passed) and r4426889922 (d7f44cb already imported createDaemonTransport statically, so no new eager closure).

CI shows one failing check, Apple / Smoke Tests (run 41098111773), which looks unrelated. In the iOS phase, scenario smoke:automation-input step 20 got selector_not_found for click id="automation-close-sheet", a typed daemon result that came back over the transport after 19 earlier steps and 6 other scenarios succeeded. The macOS phase passed, and the same logical patch on the old base and on current main both passed Apple. Please rerun that job. I did not rerun it or inspect its screenshot and snapshot artifacts, and I did not rerun check:affected, unit, provider-integration or wire-compat suites. I could not verify the receipt you noted, and the cross-build manual send in your reply has no attached output. The MCP and index-policy-reuse threads were judged fixed from the earlier review of the identical commit, not re-read line by line.

No conflicts are known. Before merge, the script needs to use one exported readRegisteredDaemon, and Apple Smoke Tests needs a green rerun.

[apex-sentinel-staging]

Apex Sentinel: 8 new findings on c5537a0. The verdict and the full list are in the summary comment.

[apex-sentinel-staging]

Apex Sentinel: 3 new findings on 06879b9. The verdict and the full list are in the summary comment.

[thymikee]

Reviewed 06879b9 against the earlier findings. The store.ts note and most of the inline threads are fixed, but the duplicated daemon admission logic in the recorded-daemon script is still there, so the code needs one more change. All 30 checks pass at this commit, and the Apple Smoke failure from the previous head is gone.

scripts/lib/recorded-daemon.ts:76 is unchanged from the earlier review and still unanswered. Lines 28-52 re-implement readDaemonInfo. Lines 76-89 copy registrationAllowsDaemonObservation and daemonRegistrationUnprovenError from daemon-client-startup.ts:169-191. The copies already disagree with the owner: for a held foreign claim observeLocalDaemon returns null (startup.ts:111), while the script throws daemon_registration_unproven, and the script also rejects pid <= 0 and port > 65535 where readDaemonInfo does not. This commit grew the copy: it writes the divergent rule into the doc comment (lines 54-61) and adds a hand-built not-running error (lines 71-75). The perf forced-backend, iOS live-runner conformance, snapshot-benchmark and Android observability lanes use this reader, so the next change to the record or claim rule will not reach them, and the tests on both sides will stay green. The rule to satisfy: exactly one daemon-client function decides whether the daemon a state directory records may be used without starting it, and that function owns its refusals. Please export readRegisteredDaemon(paths): DaemonInfo | null beside readDaemonInfo, built from the first four steps of observeLocalDaemon (unproven-lock throw, readDaemonInfo, registrationAllowsDaemonObservation), and have observeLocalDaemon call it. Then every script reader that today goes through the copy should call it: sendToRecordedDaemon, cell-admission.ts:207 and live-observability-scenario.ts:206. Delete RecordedDaemon, readRecordedDaemon, the inline lock check and both hand-built errors. A null result should map to daemonNotRunningError, and the send should be sendDaemonRequest(..., timeoutRecovery 'none'). This is done when git grep daemon_registration_unproven outside tests returns only daemon-client-startup.ts.

Could the simplest version be better here? Exporting readRegisteredDaemon and reducing sendToRecordedDaemon to resolveDaemonPaths, readRegisteredDaemon and sendDaemonRequest would delete about 45 lines of copied logic and both hand-built errors, and the script docs would shrink to one sentence.

Not blocking, and fine to take or leave: the new not-running error at recorded-daemon.ts:73 uses reason daemon_not_running, while the owner spells it daemon-not-running (DAEMON_UNAVAILABLE_REASONS.notRunning) and adds a start hint, so throwing daemonNotRunningError(paths.baseDir) from the owner and asserting that reason in recorded-daemon.test.ts:121-127 would fix it.

On the inline threads from the other review: the unresolved doc nit on credential-fingerprint.ts still applies (the doc omits the provider.trim()). The following do not apply (resolve any still open): index.ts wrappers (they do real async work through the lazy import), fingerprint trim (the daemon already fingerprints the trimmed id, and the ledger rationale says so), refusal without processStartTime (it matches the production rule; the real issue is the copy above), typed not-running error (fixed; only the reason spelling remains), held-lock doc, proxy-upstream types, createRequestGuard doc, ledger rationale and package-entry-surface wording (all fixed), and single entry load cost (the extraction requires one entry, index.ts now states the cost, and no new eager closure appeared).

I ran no tests or gates; CI state comes from the check results. I read the test-isolation and recorder-shutdown changes in 06879b9 but did not run them or confirm which CI failure they addressed. I judged the rebase from the range-diff for patches 2, 6 and 8, not line by line.

[thymikee]

Addressed the remaining admission finding from #4101 (comment) (and the earlier comment) in bae7564.

readRegisteredDaemon is now the single exported admission function beside readDaemonInfo. Production observeLocalDaemon, the recorded-send helper, benchmark admission and Android observability all call it. RecordedDaemon, readRecordedDaemon, the copied lock policy and both hand-built errors are deleted. Foreign/missing records return null; { required: true } converts that result through the canonical daemonNotRunningError, including its reason and hint. Unproven ownership is refused before record lookup. PID and port validity now have one decoder owner, including the previous script-only bounds.

The reader is asynchronous to keep registration/error machinery lazy. Error constructors and diagnostic hints now share daemon-lifecycle-errors.ts; startup retains only its existing unproven-reason check. Moving those together avoids a metadata-to-startup import cycle. The entry remains at 126 eager modules, with no raised ceiling or suppression.

Validation: the exact-head affected gate passed with a clean checkout (run-bUPLaQ, exactCommit: true), including Fallow, package, Node integration, related tests and unit checks. The earlier d35be10 gate caught the cycle before this final ownership adjustment. In a disposable checkout at d35be10, the same focused suite passed 8 tests, then failed both socket/HTTP goldens when client fingerprint trimming was bypassed, and failed port admission when the upper bound was removed. The checkout was removed; implementation state was unchanged. The relocated drain fixture also includes the runtime PR's identity-snapshot fix.

The trim documentation and parity threads are answered/resolved. The earlier cross-build smoke remains attributed to bc13701; no new device run is claimed. Fresh GitHub checks and configured reviews at this published head must finish before merge.

[cubic-dev-ai]

All reported issues were addressed across 16 files (changes from recent commits).

<sub>Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic</sub>

[apex-sentinel-staging]

Apex Sentinel: 7 new findings on bae7564. The verdict and the full list are in the summary comment.

[thymikee]

The code in bae7564 looks good, and it fixes the findings from the earlier review of 06879b9. Not blocking, and you can take or leave it: the comment at daemon-client-liveness-probe.ts:89 says readDaemonInfo accepts any positive integer port, but it now rejects ports above 65535 (readDaemonPort in daemon-client-metadata.ts), so please reword it to name the DaemonInfo sources that are not decoded from the record, or drop that clause.

The Coverage check (pnpm gate unit-ci) is failing. It runs every test file this change touched, and all 30 checks were green at 06879b9, so the failure is likely related. The log is cut off before the failing test name, so I could not name it. The PR also has merge conflicts with main in oxlint.config.ts and packages/daemon-client/src/__tests__/daemon-client-ensure-drain.test.ts, both from #4097 and both look mechanical. Before merge, please rebase onto main and get Coverage green at the new head, or name the failing test if it is unrelated. I ran no tests or gates, and no device run is needed because this change does not touch dispatch, capture, interaction or app lifecycle.

Of the open inline threads, two still apply as lower-priority notes. These do not apply, so please resolve them: Cubic's undecodable-record thread (same behavior as the base, and callers still need the registration lock); r4431031896 (duplicate; an unproven lock throws daemon_registration_unproven); r4431031898 (the old pid: 0 fallback was never a real process identity); r4431031899 (the daemon trims the provider on intake and a test covers it); r4431031900 (the entry imports the metadata module statically, so the dynamic imports add no eager closure); r4431031903 (observeLocalDaemon is internal, so no doc comment is required). These are already fixed or benign: r4428374485, r4428374473, r4428374499.

[thymikee]

Published the next repository-review round at aac253f.

• The shared admission owner now preserves unreadable/corrupt/unusable present metadata as the existing typed daemon-record-unreadable refusal. Only absent metadata and the existing foreign-claim rule are nullable. No metadata is cleared or daemon started on that refusal; pinned clients keep binding-mismatch diagnostics.
• Benchmark ownership refusals keep the cell-state classification and original cause, including when cleanup fails; both errors are reported. The shared-dist guard now hashes recursive output contents. Disposable regression experiments detect both lost classification and an in-place rewrite that the previous timestamp guard missed.
• Restored the observation summary and documented verbatim provider forwarding. I declined static error imports: the isolated entry walk grows from 126 to 130 modules, while keeping the transport closure unchanged is required. Type-only backreferences do not create a runtime cycle, and TypeScript checks the literal dynamic imports.

The exact-head affected gate passed (run-GFwZPd, exactCommit: true, unchanged checkout). The focused 316 tests and the owner-file bundle test passed; the isolated mutation/regression checks fail at their intended assertions and were cleaned up. All eight current threads are answered and resolved. Fresh GitHub checks and configured reviews at this head remain required before merge.

Evidence attribution: the record/classification/content-guard experiments ran at 118b10701f, and the composed cleanup experiment ran at 374d32793e. The daemon-client package tree is byte-identical after rebasing onto main 3a218f5; the new exact-head run-GFwZPd validates aac253f (16,581 unit tests passed). The rebase retains both migrations and the upstream drain fix; its two conflicts were the lint-path rows and relocated fixture import. Lockfile regeneration made no changes.

[thymikee]

Follow-up to #4101 (comment): the required items are resolved at aac253f. The branch is rebased onto main after #4097, preserving both lint-path rows and the relocated drain fixture; the daemon-client package tree matches the tested pre-rebase candidate. Coverage is now green: https://github.com/callstack/agent-device/actions/runs/41186222871/job/124493576223. The current rollup has 27 successful checks, one intentional skip and one neutral result, with no pending or failing checks. GitHub reports CLEAN/MERGEABLE and zero unresolved review threads.

I am leaving the explicitly optional liveness-probe wording nit for a later cleanup. The stale readDaemonInfo clause does not affect the bounded record decoder or the probe’s guarded transport construction; no runtime change is needed for it. The current validated head remains unchanged.

The exact-head local gate also passed (run-GFwZPd, exactCommit: true, unchanged checkout; 16,581 unit tests passed). No new device run is claimed.

Update: the optional wording correction is now addressed by #4117 after #4101 merged. The follow-up contains only the comment correction.

[thymikee]

The rebase at aac253f resolves the #4097 conflicts mechanically (moved test paths in the line-cap list, the ADR collocation row and the ensure-drain imports), and Coverage now passes along with the rest of CI. With the code verdict from bae7564 unchanged, this is ready for a human review. The optional readDaemonInfo port comment note still stands.

[thymikee]

The optional port-comment correction is now in #4117, since #4101 merged while it was validating. It removes the stale readDaemonInfo port claim and retains the detached-probe error-settlement rationale. Only one comment changes (+1/−3); no runtime code, APIs or new package are involved. The old extraction branch was not pushed or recreated.

Follow-up head 5ae0b05 passed the exact-head affected gate (run-pIdMOm: 8 runnable checks passed, 7 owner skips; 16,738 unit tests passed). OXC program ASTs match the parent after excluding comments and locations. Fresh CI and repository reviews are pending on #4117.