fix(daemon): retain cleanup locks and preserve healed snapshots

[thymikee]

Summary

• Keep session/device execution locks through binding and claim cleanup on every outcome (ADR 0019).
• Retain authored snapshots and explicit repair-time --record corrections in healed scripts (ADR 0012). Exclude diagnostic and foreground composite captures.
• CLI/MCP ingress now distinguishes per-call intent from project record: true defaults; SDK per-call recording stays explicit.
• Both fixes close the same ADR-conformance audit (Restructure ADRs into short decision records #3744). Twenty-three files; scope includes foreground composition and recording ingress. Binding disposal remains unbounded; existing disposers acquire no execution locks.

Validation

• Tested SHA: a46bcdce611309463f5cc91749f9cc8efaac6402, rebased onto 0563666b09a09b2de039507307d38239dabee272.
• Locked install/build; full-repository pnpm format; 180 focused tests passed. Publication tests use default CLI/MCP ensured clients and SDK HTTP transport against the public daemon runtime; HOME/USERPROFILE remain isolated.
• Exact-head affected gate: passed, run-0VMDJP, exactCommit=true, checkoutChanged=false; nine checks and 3,601 tests passed, seven parked/GitHub-owned skips. Self-reviewed; no independent review or live device run for this rebase.
• Quiesce counts dispatch through binding cleanup, so a held cleanup refuses quiesce as daemon-busy without acquiring its execution lock.
• CI at prior head d7bdb0d70e: guards, integration, coverage, lint/package, docs, preview, size, native fingerprint, CodeQL and two smoke jobs passed; two smoke jobs queued, cache matrix skipped, cubic neutral. Current-head CI, including native smoke, remains required.

[cubic-dev-ai]

No issues found across 7 files

_{You’re at about 91% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.}

_{View guided diff | Turn on auto-fix | Re-trigger cubic}

[github-actions]

Size Report

Metric	Base	Current	Diff
Installed (including dependencies)	5.38 MB	5.38 MB	+819 B
Package (unpacked)	5.38 MB	5.38 MB	+819 B
Package (download)	1.62 MB	1.62 MB	+265 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	27.7 ms	27.8 ms	+0.1 ms
CLI --help	78.2 ms	80.0 ms	+1.8 ms

[thymikee]

The PR is ready. I reviewed commit 49a4033 and found no blocking problems in the code.

Not blocking: these notes can be taken or left. (1) The Unreleased section of CHANGELOG.md has no entry for two user-visible changes: authored and --record'ed snapshot steps now persist in healed scripts (session-script-writer.ts#L315-L321), and requests that share a session or device now queue behind binding disposal. (2) session-action-recorder.ts#L89 stores publicationIntent: 'explicit-record' from flags.record, but sanitizeFlags already keeps that flag on the same action. Could the writer read action.flags.record and keep only the authored-provenance bit? (3) The PR joins two fixes, lock retention and snapshot publication intent. A line in the PR body on why they ship together would help. (4) The lock-retention test in request-runtime-binding-router.test.ts#L95 waits one setImmediate tick for the queued bind call. Any real async work before bind would let it pass without the fix. Could you wait on a deterministic signal, such as the second request entering the lock queue, and show the test failing at merge-base 4c1c786 with the scope change reverted? I did not run it there. I also did not read the per-provider binding disposers, and I did not check whether the .ad parser accepts --record on a snapshot step.

I found no conflicts. The iOS Smoke Tests job failed at "wait for the deep-link destination (5/5)" with APP_NOT_RUNNING, reason wait_capture_stalled, which looks like a held deep-link launch. A lock or disposal fault would show as a delayed or failed request, not a runner-reported app-not-running. The recording changes do not touch that route. I did not compare against the flake history on main. The gesture-pan-duration.ad failure passed on retry. Please rerun the Smoke Tests job and add the CHANGELOG entry before merge.

[cubic-dev-ai]

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

_{You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.}

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

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

[thymikee]

Addressed all four maintainer notes in eb30491:

• Added both Unreleased entries; explicit recording now reads action.flags.record, retaining only authored provenance separately. Ordinary publication behavior stays covered.
• Added the shared ADR-audit rationale to the body.
• Replaced setImmediate with execution-queue insertion. The same router suite gives 6 failed/2 passed on merge-base 4c1c786968 with only the test copied, versus 8 passed on head. The missing predecessor lock causes each base failure; the disposable checkout is removed.

iOS diagnosis: the original artifact screenshot shows unanswered “Open in Agent Device Tester?” confirmation; runner APP_NOT_RUNNING becomes wait_capture_stalled. Independent run fails identically with execution-scope blob 21eef92c7a matching merge-base, without this lock change. Apple binding disposal is a no-op. The single rerun was cancelled while queued after this push.

Validation: five focused suites, 104 passed; affected gate run-XYy1Xj, 2,302 passed, seven checks passed/seven skips, exactCommit=true, checkoutChanged=false. Self-reviewed.

Head CI: lint/typecheck/package, size, native fingerprint and CodeQL passed; smoke, guards, integration, coverage and cubic pending. Not merge-ready yet.

Review threads: 0 fixed / 0 already fixed / 0 rejected (none exist).

[thymikee]

This PR is ready at eb30491. The earlier review of 49a4033 was clean, and this head keeps that result: cleanup locks are now retained and healed snapshots are preserved.

Not blocking: the flags.record !== true && guard in https://github.com/callstack/agent-device/blob/eb30491/packages/daemon/src/core/session-action-recorder.ts#L89 does nothing in a repair-armed session, because the writer already retains the snapshot there, so should authored provenance come from interactiveObservation alone (or can you say why authored plus --record should be stripped outside repair)? Also, publicationIntent?: 'authored' in https://github.com/callstack/agent-device/blob/eb30491/packages/contracts/src/session-action.ts#L32 acts as a boolean and could be collapsed the next time you touch it. You can take or leave both.

The Smoke Tests failure looks unrelated to this change. The log I have is from run 39008383674, which is not from this head. It shows the iOS simulator deep-link destination wait stalling (APP_NOT_RUNNING, wait_capture_stalled, zero readable captures), and the hint points to a deep-link confirmation prompt holding the launch. That is a simulator prompt state. The scope-disposal reorder and noRecord: true on the composed open-snapshot touch the same route, but neither answers prompts or launches apps. I did not run the router test or the replay-repair suites, and I did not check your claim about identical symptoms on the merge-base. The lock-ordering test spies on locks.set, so it ties to the current lock implementation.

Please rerun Smoke Tests on eb30491. Before merge, that run needs to be green, or show that the wait_capture_stalled prompt state comes from the environment. No conflicts are known.

On the open review threads, the P2 about record being settable from projectConfig (#3760 (comment)) does not apply, so you can resolve it. The recorder already treats effective flags.record === true as the force-record opt-in, and the writer mirrors that only in a repair-armed session. Dropping the snapshot would contradict the flag's documented meaning, and the kept step is a harmless replayable snapshot. That thread still needs your reply or dismissal.

[thymikee]

Fixed the config-derived recording leak in 73faf74. sanitizeFlags keeps effective record; CLI/MCP now preserve explicit provenance at ingress, and publication uses durable intent. Authored provenance no longer depends on record. Help, changelog, and replay docs are updated.

Regression before fix: 1 failed / 1 passed (unwanted snapshot). CLI/MCP/SDK publication coverage after fix: 5 passed. Provider integration exercises SDK ingress and rejects an effective flag without intent.

iOS: attempt 1 failed in daemon startup (daemon_startup_failed, 15s), before RPC dispatch; it was neither the earlier launch prompt nor queued binding cleanup. The single rerun passed startup/Settings/gesture, then failed step 84: native simctl openurl exited 60. The prior snapshot finished 234 ms earlier; 11.116s was spent inside simctl, excluding cleanup-queue timeout. Screenshot shows Home. iOS remains red.

At eb304913e7, Android/Linux/macOS smoke, lint/typecheck/package, guards, integration, coverage, size, native fingerprint, CodeQL and cubic passed; cache build matrix skipped.

Validation: pnpm format; 150 focused tests; provider/help suites 47 passed; affected gate run-qlYwKs: 9 checks / 3,598 tests passed, exactCommit=true, checkoutChanged=false; seven parked/GitHub-owned skips. Self-reviewed; no local device run.

Threads: 1 fixed/resolved, 0 already fixed, 0 rejected. New-head CI remains required.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-17 16:48 UTC

[cubic-dev-ai]

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

_{You’re at about 96% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.}

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

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

[thymikee]

Pushed 81e2eba: CLI and MCP publication tests now set HOME and USERPROFILE together. Audited all other tests changed by this PR; no other new home-isolation gap found.

Threads: 1 fixed/resolved, 0 already fixed, 0 rejected. Re-read issue comments, reviews and inline threads after pushing; no unresolved threads or new maintainer action items remain.

Validation: pnpm format; publication suite 5 passed; exact-head affected gate run-WyTI9V: 9 checks / 3,598 tests passed, exactCommit=true, checkoutChanged=false; seven parked/GitHub-owned skips. Self-reviewed.

CI: at 73faf746cf, Linux smoke, size, docs, preview, native fingerprint, CodeQL actions/Java/Python and cubic passed; lint/package, guards, integration, coverage, CodeQL JS and Android/macOS/iOS smoke remained pending. No new failure log was available to diagnose. Cache matrix and aggregate CodeQL skipped. Current-head CI is queued/in progress, including iOS; this is not merge-ready.

T3 rejects CI watching from a sub-agent; the parent thread must continue monitoring and diagnose any failures. The prior simctl openurl exit-60 cause remains unproven. PR validation body updated.

[thymikee]

Rebased 81e2eba0f1 onto current origin/main (3404cfc413); pushed head d7bdb0d70e with an explicit force-with-lease.

Resolved both test conflicts against main’s history: retained its typed action-history cases and the PR’s publication-intent cases; kept provider scenarios on public runtime/HTTP results and healed-script assertions. Moved the five CLI/MCP/SDK publication cases to provider integration through the public daemon runtime, removing all three new daemon testing-export imports. Production changes are unchanged by the rebase.

Validation: locked install/build, full-repository format, 120 focused tests passed. Exact-head affected receipt run-oBaplK: 9 checks / 3,570 tests passed, exactCommit=true, checkoutChanged=false; seven parked/GitHub-owned skips. Current-head GitHub CI remains required. Existing review fixes and HOME/USERPROFILE isolation are preserved.

[thymikee]

Rebased d7bdb0d70e onto origin/main (0563666b09); pushed a46bcdce611309463f5cc91749f9cc8efaac6402 with an explicit force-with-lease.

Resolved CHANGELOG.md by keeping both sets of entries in main’s order/format. Resolved MCP by keeping main’s ensure-then-attach transport and the PR’s recordSource plumbing. CLI/SDK auto-merges preserve intent. Found and fixed a test gap: publication tests now use default CLI/MCP ensured clients and SDK HTTP delivery, rather than injected transports/clients (a46bcdce61).

Quiesce remains compatible: a request stays in flight through binding cleanup, so quiesce returns daemon-busy without acquiring the held execution lock. No production change needed.

Validation: locked install/build, full-repository format, 180 focused tests passed. Exact-head receipt run-0VMDJP: 9 checks / 3,601 tests passed, exactCommit=true, checkoutChanged=false; seven parked/GitHub-owned skips. Self-reviewed; no live device run. PR validation body updated. Current-head CI remains required.

Review threads: 0 newly fixed / 0 already-fixed actions / 0 rejected; both existing threads remain resolved.

[thymikee]

The earlier review at eb30491 was clean, and the changes since then fix both inline issues. At a46bcdc the writer keys on explicit-record intent, which comes only from the CLI or MCP ingress, so a project-default record no longer opts in. The integration test now sets HOME and USERPROFILE together. The code looks right to me, but I did not run the new integration tests. I reasoned about the regression from the code at eb30491. Current-head Smoke Tests are still queued or running with no failure so far, so the evidence is pending. Both jobs need to finish green before merge. The diff touches CLI request-flag construction and the daemon's repair-publication filtering, and the smoke runs of ordinary commands should not reach that route.

Not blocking, and you can take or leave these: the new recordingIntent flag in command-flags.ts and the recordSource dependency threaded through the client, MCP and cli.ts carry the source of record to the daemon, which now ignores flags.record, so a configured record: true has no effect. The docs line and the CHANGELOG name only the project-config default, but env and user-config defaults are also non-explicit, so "any config or environment default" would be accurate. I also did not check whether a project-config record: true still writes --record into recorded script lines through sanitizeFlags, so the key may not be fully dead.

Could record be invocation-only at the config owner instead? That means dropping projectConfig: true from its declaration and skipping it in readEnvFlagDefaults, the user config and resolveMcpConfigDefaults. Then flags.record === true is always explicit, the daemon can read it again, and recordingIntent, both recordSource deps and the cli.ts stamp go away. The one thing to settle first is whether to retire the released project-config key (in tags since v0.21.10) with a CHANGELOG entry. It already has no effect at this head, so retiring it removes dead surface instead of breaking a working one.

Of the earlier inline threads, the one on the writer opting in from a project default and the one on HOME and USERPROFILE in the test no longer apply at a46bcdc, so you can resolve both. No review has covered this head from that reviewer yet. No conflicts are known.

[thymikee]

The code looks good at 9918f7e, and the earlier a46bcdc review points are fixed: the recorder now keys on explicit recording intent, and the integration test sets HOME and USERPROFILE together. This head is a rebase with the same logical patch, so the checks are the open item. The iOS live fixture smoke job fails at open --relaunch --launch-url agent-device-test-app:///snapshot-depth, where xcrun simctl openurl exits 60 (CoreSimulator timeout). The diff touches the request lock and the post-open snapshot, but not the openurl dispatch. The same step and error failed on unrelated PR #3694, so this looks like a flake. I could not check main's own history for this step. Please re-run the iOS Smoke Tests job at 9918f7e. Merge once it passes. I did not run the changed paths on a device. The integration tests cover them through the public daemon, and I did not run those either.

The new concept is per-call recording provenance. It adds a second wire field, recordingIntent, and threads a recordSource argument through the CLI, the MCP ingress, the client deps and buildRequestFlags. After this PR no daemon code reads record itself. So the project-config key record: true (still projectConfig: true in flag-definitions-action.ts, released since v0.21.10) now does nothing, and users get no warning. Would it be smaller to fix this at the flag declaration? For example, set record to projectConfig: false and retire the config key with a migration message through the cli-config retired-key path. Then record: true on the wire is always per-call intent, the daemon keeps keying on flags.record, and recordingIntent and the recordSource plumbing can go.

The two cubic-dev-ai P2 threads are fixed at this head, so please resolve them: #3760 (comment) (recorder keys on explicit intent) and #3760 (comment) (HOME and USERPROFILE set together).

[thymikee]

Implemented your design at 879c810895 + c77252c6a9 (pushed as head c77252c6a9). Evidence and shape:

Claim verified. At 9918f7e, nothing read flags.record for repair retention (the recorder keyed on flags.recordingIntent), so config record: true was dead at the retention owner, and sanitizeFlags never wrote --record into script lines, so it was fully dead with no warning. Your projectConfig: false + retired-key path is strictly smaller: the concept stops living in four layers (CLI/MCP ingress, client deps, buildRequestFlags, wire field) and lives in zero.

Commit 1 (fix(cli): retire the record config key with a migration message) — record set to projectConfig: false, and a retired-key check in cli-config.ts (following the RETIRED_SCREENSHOT_MAX_SIZE precedent): record in a project/user/explicit config file and AGENT_DEVICE_RECORD on a command that accepts --record fail INVALID_ARGS with "was removed; --record is per-invocation only: pass --record on the command, or record: true in the SDK/MCP call…". Remote-config profiles never carried the key. Help, CHANGELOG, replay-e2e.md, and configuration.md updated.

Commit 2 (refactor(cli): key repair recording off flags.record and drop the provenance wire field) — with config/env sources retired, wire flags.record is always per-call intent (SDK per-call options and MCP tool input were already per-call; the SDK client config has no record field), so the recorder reads flags.record again and recordingIntent/recordSource are gone from CommandFlags, cli.ts, mcp/command-tools.ts, agent-device-client.ts, and buildRequestFlags. SessionAction.publicationIntent stays — it is the recorder's authored/explicit stamp consumed by the writer, not wire provenance.

Behavior kept: healed scripts retain authored snapshots, explicit --record corrections stay, no config surface can make retention happen implicitly, and the config key never silently does nothing. Production diff shrank from 17 files/450+ to 13 files/475+ minus the removed layers; the CLI/MCP publication integration tests now pin the retirement message plus explicit-intent retention through the public daemon.

[cubic-dev-ai]

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

_{You’re at about 97% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.}

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

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

[thymikee]

Rebased onto origin/main (CHANGELOG-only conflict; range-diff clean) and implemented the maintainer's smaller design.

Commits: 879c8108 retire the record config key + AGENT_DEVICE_RECORD with a fail-closed migration message at the cli-config owner, record set to projectConfig: false; c77252c6 recorder keys on flags.record again — recordingIntent and the recordSource plumbing deleted (config/env sources are gone, so wire flags.record is always per-call); 98d717c3 fix the test-file-size ratchet (config test back to its 1,282 merge-base lines, retired-record tests split out); 39c9885c pin the retired key in --config/AGENT_DEVICE_CONFIG files.

Threads: 1 fixed + resolved (explicit-config coverage — verified same parseConfigObject check, no loader gap); the two earlier cubic threads confirmed resolved; replied to the design comment with evidence.

CI: all checks green at 39c9885c, iOS Smoke included — the open --relaunch --launch-url exit-60 flake did not recur, so no rerun.

Validation: locked install/build, pnpm format, focused suites passed; exact-head receipt run-sCrqeH (9 checks, exactCommit=true). Not merge-decided here.

[thymikee]

This PR is ready for review at 39c9885. The cleanup locks are retained and the healed snapshots are preserved, and the fixes from the earlier review at 9918f7e are in.

All 20 checks pass at 39c9885, including the iOS smoke job that flaked earlier on simctl openurl, a path this diff does not touch. There are no conflicts. I did not run the tests, so I judged the new regression cases from the diff and the pre-change code. I also did not run a live device check, because the rebase changes CLI config parsing and the daemon recorder's flag read, not a device-facing path. Binding disposal stays unbounded while the lock is held, as the PR body says, so a hung disposer would hold the session or device lock. That predates this change.

Not blocking, and you can take or leave these: the MCP retired-key case in cli-record-publication.test.ts checks only the error message, so it could also assert the INVALID_ARGS code. The help text in flag-definitions-action.ts could drop the retired-key history and read "Force-record this action into a repair-armed heal (per-invocation; mutually exclusive with --no-record)".

The three cubic-dev-ai P2 threads no longer apply, so you can resolve them. The record config and env sources are retired, so the effective-record concern is moot: #3760 (comment). The CLI and MCP cases now set HOME and USERPROFILE together: #3760 (comment). The new retired-record tests cover explicit --config and AGENT_DEVICE_CONFIG: #3760 (comment).