Skip to content

fix: guard bounded lifecycle and claim recovery - #3911

Merged
thymikee merged 7 commits into
mainfrom
t3/3325/bounded-claim-recovery-guard
Oct 23, 2026
Merged

thymikee merged 7 commits into
mainfrom
t3/3325/bounded-claim-recovery-guard

Conversation

@thymikee

@thymikee thymikee commented Oct 22, 2026 •

Copy link
Copy Markdown
Member

Scope

Ref #3325. Guard bounded lifecycle disposal and claim recovery before refused steps. Outer batch/replay authorization, progress, takeover fields, and public command-name guidance with original arguments and session remain. Recovery gateways recheck claims.

Capture/provider disposal and unsafe recovery refuse. Ordinary reopen preserves resources. Under the approved IME policy, IME-only bounded close restores the keyboard and retires the session. No universal native deadline, zero-bind, or cross-daemon atomicity guarantee is claimed.

35 files, +2341/−157 = 2498 gross on merged main b08b5e7; head 87e8e21 unchanged. Screened evidence holds exact clean run-wOpmey, Sol/Sonnet reviews, negatives, range-diff, and native JSON. CI: 17 success, one skipped, one neutral.

Android bounded batch/replay IME close restored the keyboard; app-log close still refused, and reopen preserved its manifest. New bounded batches proved fresh iOS Settings open matched standalone device selection, no-app foreground selected the sole allowed booted iOS simulator with a 19-node snapshot, and stopped owned AVD boot by name returned its serial. Initial native foreground ambiguity cleared after rebooting only the disposable simulator and launching Settings alone. Earlier iOS/AVD runs were direct.

Same-session alternateFrom after direct close returned SESSION_NOT_FOUND; native failed-IME-restore has routed/unit evidence only. Owned devices and claims were released. #3325 awaits acceptance and merge.

View guided diff Turn on auto-fix

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.56 MB 5.57 MB +8.6 kB
Package (unpacked) 5.56 MB 5.57 MB +8.6 kB
Package (download) 1.69 MB 1.69 MB +3.1 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 41.5 ms 40.8 ms -0.7 ms
CLI --help 85.8 ms 82.6 ms -3.2 ms

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-23 09:41 UTC

@thymikee

Copy link
Copy Markdown
Member Author

Independent Claude Sonnet assessment of the actual Size artifact found no actionable smaller equivalent design. This layer adds 8,611 B unpacked / 3,312 B download, with six more packed files and no dependency changes. All 19 changed chunk entries sum to the reported growth.

Approximately 2.7 kB is attributed to lazy-import chunk overhead; the remaining growth is resource/lifecycle admission logic. Static imports would regress existing eager-closure constraints; blanket lease refusal would change accepted behavior. Source remains unchanged at 37c681c. This does not offset the growth against #3910 or establish a causal startup-performance change.

Full assessment, exact artifact measurements, attribution limits and rejected alternatives. The reviewer read the production diff and size evidence; this is a focused size assessment, not another correctness clearance.

@thymikee

thymikee commented Oct 22, 2026 •

Copy link
Copy Markdown
Member Author

Verified actual head 37c681cf23c30833998e4dda52eeb0ac4f247b71: all 14 checks completed with 12 success, 1 skipped, and 1 neutral. Coverage, Integration, Repo Guards, package checks, and all three Android/Apple/Linux smoke lanes succeeded. No unresolved inline review threads were present.

Exact-head CI evidence. Source, local gate/review attribution, and documented live limits are unchanged. The #3910 prerequisite is now also green at 56c0f47 after its single Linux infrastructure retry succeeded. No merge or approval action was taken.

@thymikee

Copy link
Copy Markdown
Member Author

Thanks for the work here. At 37c681c, a bounded close on any Android emulator is now refused, and that needs a fix before merge. CI is green (14 checks, none failing), but the Android smoke lane runs the unbounded test command, so it does not exercise a bounded replay or batch close on an emulator.

Every Android emulator open activates the test IME by default (session-open-execution.ts#L181). inspectAndroidSessionTeardownResources then reports android-test-ime, so composite-resource-admission.ts#L114 refuses the close with session-resource-disposal:android-test-ime. agent-device replay flow.ad with open settings … close, and batch with open…close, passed on an emulator at base 56c0f47. Now they fail at the last step and leave the session open with the helper IME active, so the user must run close by hand. Issue #3325 asks for typed timeout results, not for refusing the ordinary close, and #3910 already restores the same IME inside bounded nested-test cleanup. The rule should be: a bounded close is refused only for disposal whose worst case exceeds the composite budget, using the same classification nested-test cleanup applies to the IME. Please stop counting android-test-ime as a refusing disposal, keep the capture-handle and provider-release refusals, and update the batching and replay doc sentences about the IME.

The open refusal for an existing session with owned resources (composite-resource-admission.ts#L115, shape session-reopen-with-resources) looks like a prediction with no disposal behind it. As far as I can read, publishOpenSession only updates device, surface, app and lease fields, and only the close lifecycle touches appLog or screenRecording. So after logs start or record start, a bounded open com.other.app step in the same session is refused, where base admitted it and the resources would have survived. Does reopen dispose any session resource today? If not, please drop the refusal (and validateSessionReopen if nothing else needs it) and fix the help, batching.md and replay-e2e.md sentences that describe a replacement the code does not perform. If one reopen variant does dispose a resource, refuse only that variant from the decision that performs the disposal. I did not trace whether a platform relaunch indirectly ends a pid-bound log stream.

The new preview runs live inventory selection and claim inspection before every bounded fresh open and transient-exclusive step (request-execution-scope.ts#L137). The PR body says non-Android selection is covered by unit tests only, and I took the live claims from the PR body. Please run at the PR head on an iOS simulator: agent-device batch with [{open <app>}], then a separate batch with [{open --foreground}] on a fresh session. Each must return ok on the same device id a standalone open selects, with no bounded_composite_resource_unsupported. On Android, if the evidence does not already show it, run a bounded batch with [{boot --device <stopped AVD name>}] that reaches the boot handler. After the IME fix, an Android emulator replay of an open…close .ad file should end ok and restore the IME. Please record the SHA, device and JSON responses.

I did not run tests or mutations locally, so regression validity is judged from the diff and the base code. I also did not measure whether Android IME restore can exceed the composite budget; the argument above relies on #3910 already restoring it inside bounded cleanup.

Not blocking, take or leave: inspectDeviceClaimAcquisition (device-claims.ts#L162) re-implements the acquisition walk, so one walk with an inspect|settle strategy would keep the preview from drifting; resolveFreshBoundedOpenDevice and resolveBoundedClaimDevice re-compose handler selection (and enumerate inventory twice), so exporting each handler's resolver and calling it from both places would be safer (request-execution-scope.ts#L113); the claimRecovery bag at L428 rebuilds what createRequestDeviceAccess has, and an inspect(device) on DeviceClaimAdmission would replace it; inspectLeaseForClose repeats getLease (lease-registry.ts#L271) and closeLeaseDecision could return a union that avoids the ! assertions; the bounded predicate is spelled three ways and the lease-not-active error is hand-built at L442; the Invalid open target branch at L120 cannot fire, and the resolveForegroundOpenRequest re-export in session-open.ts can be an import at the definition site; and the production diff is +559/-86 with 3.3 kB of package growth, above the 3 kB review threshold.

Could a smaller design cover this? Keep the close refusal only for session-owned capture handles and the real provider-release decision, drop the IME and reopen refusals, and let admitSessionLifecycleRequest shrink to a close check plus claim recovery. Make claim acquisition one walk exposed as inspect(device), and have each handler export its device resolver so the preview and the handler call the same function. That would remove most of the growth. It would first need ADR 0036 amended to classify Android IME restore as bounded, like nested-test cleanup, and to state that reopen does not dispose session resources.

@thymikee

Copy link
Copy Markdown
Member Author

I’m tracing the actual close and reopen owners before changing these guards. The accepted resource policy explicitly included IME restoration, while trusted suite cleanup is a separate obligation; I will distinguish a concrete over-refusal from a change to that classification rather than infer equivalence from cleanup alone. The reopen assessment will check for actual disposal on each variant. The requested device routes need fresh parent assignments after a stable candidate; none are currently assigned. This review remains open while that assessment and the #3910 retry correction are completed.

@thymikee
thymikee added this pull request to stack #3917 October 22, 2026 19:51
Base automatically changed from t3/3325/bounded-operation-resource-guard to main October 22, 2026 19:52
@thymikee
thymikee force-pushed the t3/3325/bounded-claim-recovery-guard branch from 37c681c to 87e8e21 Compare October 22, 2026 21:30
@thymikee thymikee changed the title fix: refuse bounded session teardown and claim recovery fix: guard bounded lifecycle and claim recovery Oct 22, 2026
@thymikee

Copy link
Copy Markdown
Member Author

Replying to the 37c681 review: the current 87e8e21 branch admits IME-only bounded caller close under the approved policy. It still refuses capture-handle and actual provider-release disposal. The generic existing-session open resource veto is gone; same-app reopen, app switch, and relaunch preserved the live app-log manifest. The trace ownership guard remains for actual lifecycle effects. ADR 0036 and batching/replay help reflect ordinary IME restoration and its time limits.

Exact-source Android batch and authored replay open/home/close restored the original keyboard and retired the owned session/claim. App-log plus IME refused bounded close with the typed app-log shape and truthful prior progress. Explicit iOS Settings open reached a native nine-node snapshot and closed. A no-app foreground request against mixed private inventory returned AMBIGUOUS_MATCH; it is not a successful foreground selection proof. An owned stopped AVD booted by name and shut down by serial. The same-session alternateFrom after direct close returned SESSION_NOT_FOUND. Native failed-IME-restore is routed/unit evidence only; no host fault was induced. All owned device state was released.

The optional shared-walk/resolver and size suggestions remain design considerations; no current drift or smaller equivalent was demonstrated by the focused assessment. The exact run-wOpmey receipt, full review results, range-diff and live record are in the screened evidence gist. New-head CI is pending; old-head green checks do not transfer. This does not claim the whole issue is accepted or merged.

@thymikee

Copy link
Copy Markdown
Member Author

Exact published 87e8e21 now has all 19 checks completed: 17 successes, one skipped and one neutral. Coverage, Integration, repository guards and all three Android/Apple/Linux smoke lanes succeeded. Raw exact-head CI, local receipts/reviews and live evidence. The disclosed iOS foreground ambiguity, retired-session alternate-resume result and native failed-IME-restore coverage limit remain unchanged. All owned device state is released. #3910 is merged; #3325 remains open for the final merge and acceptance/admin verification. No merge, approval or label action was taken.

@thymikee

Copy link
Copy Markdown
Member Author

Thanks for the update. At 87e8e21 the Android IME restore and the reopen refusal from the earlier review are fixed, and the live record now covers Android bounded batch and replay open/home/close with IME restore. One live proof is still missing. All 19 checks on 87e8e21 pass, but the Android, Apple and Linux smoke lanes run unbounded commands, so they do not exercise the bounded preview or the bounded IME close. I ran no tests or mutations locally, and I judged the code from the diff.

The bounded open and boot preview still has no live proof on its own route. resolveFreshBoundedOpenDevice and resolveBoundedClaimDevice run only for a bounded step. The live record shows the iOS open and the AVD boot as plain requests, and only the Android IME items are labelled bounded. A direct open or boot never enters the preview, so the record does not show that the preview picks the same device as the handler. If the preview picks another device or throws, every bounded fresh open on iOS and every bounded boot by name would be refused or misrouted. Please record the JSON and the SHA for three batch runs: (a) on an iOS simulator, a fresh session running agent-device batch --steps '[{"command":"open","positionals":["com.apple.Preferences"]}]', returning ok on the same device id a standalone open com.apple.Preferences selects; (b) the no-app open --foreground as a single batch step with exactly one booted iOS simulator in the allowed inventory, returning ok with no bounded_composite_resource_unsupported; (c) agent-device batch --steps '[{"command":"boot","flags":{"device":"<stopped AVD name>"}}]', reaching the boot handler and returning the booted serial. If the earlier runs were already batch steps, saying so with the JSON is enough.

Not blocking, and you can take or leave these: validateSessionReopen in composite-resource-admission.ts now only reruns resolveExistingOpenRequest, which handleOpenCommand runs right after admission, so the parameter, its closure at request-execution-scope.ts:384-395 and the ADR sentence about it can go; the androidHelperWorld restore double in composite-resource-admission-ime.test.ts hand-builds the android_test_ime_restore_incomplete error instead of using the production conversion, so routing it through the real adapter would turn the test red if the mapping changes; and session-replay-runtime-failure-response.ts matches the literal 'session_cleanup_incomplete', which session-teardown.ts also writes as a literal, so one exported reason constant used at both sites would keep them in step.

No conflicts are known. Before merge, please post the three bounded batch runs above for 87e8e21 or a later head.

@thymikee

Copy link
Copy Markdown
Member Author

The route distinction is correct: the earlier iOS open/foreground and stopped-AVD boot checks were direct requests, and I am not counting them as bounded-preview proof. The existing owner is now assigned the three actual single-step batch cases on unchanged 87e8e21, with raw request/response JSON and SHA attribution. A fresh disposable iOS simulator will isolate the foreground case without altering other sessions or apps; the boot case uses a new owned stopped AVD. No source change or extra review round is planned unless these runs expose a behavior failure. The optional source notes remain nonblocking and recorded. CI stays green, while the PR body now marks the missing bounded-route live evidence as pending.

@thymikee

Copy link
Copy Markdown
Member Author

Replying to the bounded preview review: agreed—the earlier iOS and AVD runs were direct, so they were not preview proof. These are new actual single-step batch runs on unchanged 87e8e2113938b9a324fbdd1fca3ed57d374c069b. The public batch grammar uses command/input, not the illustrative positionals/flags fields:

  • A: [{"command":"open","input":{"app":"com.apple.Preferences"}}] — success, 1/1; selected <OWN_IOS_UDID>, identical to the standalone open control; its same-session native snapshot had 19 nodes. Full A JSON.
  • B: [{"command":"open","input":{"foreground":true}}] — success, 1/1; no app or UDID, with <OWN_IOS_UDID> plus Android in the private allowed inventory. Selection was single-booted-local (one candidate); the returned native Settings snapshot had 19 nodes. Full B JSON.
  • C: [{"command":"boot","input":{"device":"<OWN_ANDROID_AVD>","headless":true}}] — success, 1/1 from a stopped owned AVD name; booted:true, actual <OWN_ANDROID_SERIAL> matched the AVD identity. Full C JSON.

B's first setup returned native AMBIGUOUS_MATCH with zero steps dispatched: Settings, Calendar and this run's XCTest runner were present. After rebooting only the disposable simulator and launching Settings as its sole installed running app, the bounded B request succeeded. A preparatory input.target attempt was rejected by the parser before admission; the valid key is input.app. Both histories, exact request contexts with private values screened, original raw-result SHA-256 hashes and cleanup are in the live report and original/screened hash ledger.

Both disposable devices were deleted; owned sessions, claims, daemon registrations/locks and panels are empty; borrowed devices retained their boot states. The optional source nits remain nonblocking and unchanged. This closes this specific live-route evidence gap; it does not claim merge or whole-issue acceptance.

@thymikee

Copy link
Copy Markdown
Member Author

Thanks, that closes the evidence gap at 87e8e21. The three single-step batch runs go through the bounded preview: a fresh open selects the same device as the standalone control, the no-app open --foreground resolves through single-booted-local, and boot by stopped AVD name reaches the handler and returns the matching serial. Nothing else blocks this from my side. The earlier notes (the validateSessionReopen parameter, the hand-built IME restore error in the test, and the shared session_cleanup_incomplete constant) stay optional.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 22, 2026
@thymikee
thymikee merged commit 7a1bc78 into main Oct 23, 2026
19 checks passed
@thymikee
thymikee deleted the t3/3325/bounded-claim-recovery-guard branch October 23, 2026 09:41
thymikee added a commit that referenced this pull request Oct 24, 2026
* origin/main:
  fix(android): bound the dumpsys reads behind dialog and foreground checks (#3936)
  feat(android): detect Fire OS devices as Android TV targets and document Fire TV support (#3923)
  docs(macos): document Electron/Chromium app automation through the accessibility tree (#3932)
  test(capture-kit): encode solid test PNGs unfiltered to stay inside the unit budget (#3933)
  fix: guard bounded lifecycle and claim recovery (#3911)
  fix(ios): clear a long text value wherever the caret lands (#3908)
  fix(snapshot): stop iOS 27 floating-bar containers from covering content (#3912)
  ci: cache and bound the Linux desktop dependency install (#3919)
  fix: refuse resource operations in bounded composites (#3910)
  refactor(ios): decide bridge-failure scope and warning once (#3914)
  refactor(capture-kit): encode post-gesture stabilization baseline provenance (#3915)
  perf: expect the persistent session on Android snapshot-raw (#3913)
  test(ios): hold the fill read-count pin to the route it measures (#3909)
  perf(ios): admit tap text-input probes from fresh target evidence (#3881)
  feat(perf): time every Android command against the fixture app (#3902)
  chore: mark 0.21.31 as released
thymikee added a commit that referenced this pull request Oct 24, 2026
* origin/main:
  fix(android): bound the dumpsys reads behind dialog and foreground checks (#3936)
  feat(android): detect Fire OS devices as Android TV targets and document Fire TV support (#3923)
  docs(macos): document Electron/Chromium app automation through the accessibility tree (#3932)
  test(capture-kit): encode solid test PNGs unfiltered to stay inside the unit budget (#3933)
  fix: guard bounded lifecycle and claim recovery (#3911)
  fix(ios): clear a long text value wherever the caret lands (#3908)
  fix(snapshot): stop iOS 27 floating-bar containers from covering content (#3912)
  ci: cache and bound the Linux desktop dependency install (#3919)
thymikee added a commit that referenced this pull request Oct 24, 2026
* origin/main:
  fix(android): bound the dumpsys reads behind dialog and foreground checks (#3936)
  feat(android): detect Fire OS devices as Android TV targets and document Fire TV support (#3923)
  docs(macos): document Electron/Chromium app automation through the accessibility tree (#3932)
  test(capture-kit): encode solid test PNGs unfiltered to stay inside the unit budget (#3933)
  fix: guard bounded lifecycle and claim recovery (#3911)
  fix(ios): clear a long text value wherever the caret lands (#3908)
  fix(snapshot): stop iOS 27 floating-bar containers from covering content (#3912)
  ci: cache and bound the Linux desktop dependency install (#3919)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant