Repository navigation
Conversation
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Reviewed 5d05dc2. I found no problems in the change: an expired close fence is now a typed refusal, and The open cubic P2 thread on the deadline at Not blocking, take or leave: CI is green and there are no conflicts. The exact-head Android live job covers the confirmed-close relaunch path; the refusal branch is covered by unit tests. The PR can merge after its stacked base #3560. |
30942ef to
78213e2
Compare
5d05dc2 to
3800fea
Compare
|
The code in 3800fea looks good. The close-timeout refusal is now fixed, and I found no problems in the diff. I did not run tests locally. I judged the regression from the pre-change code path and the base-vs-fixed table in the PR body. I also did not check whether the new refusal on plain The cubic-dev-ai thread on the early returns in app-lifecycle.ts does not apply, so you can resolve it: the early returns fire only on a positive stop observation, and every other outcome still reaches the throw (#3562 (comment)). The Coverage failure looks unrelated to this PR. The eager-closure budget test fails because mechanics.ts reaches intent-arguments.ts through app-lifecycle.ts. That import is already in app-lifecycle.ts at the merge base 78213e2, and this PR adds no import. The extra edge comes from the stacked base. Smoke Tests is still queued, and this change only touches Android close, so it cannot reach iOS smoke. I did not re-run either job. I read the Coverage failure from the log excerpt and an import grep. Before merge, the Coverage budget (mechanics.ts at 176 modules against a limit of 175) needs a fix in the stacked base (the #3555 intent-arguments edge) or a rebase. Smoke Tests also needs to finish green. There are no conflicts. |
78213e2 to
964e0a0
Compare
3800fea to
9f428e7
Compare
|
This PR is ready from the code side. At 9f428e7, the earlier clean review still holds. The only change since 3800fea is the switch of the intent-arguments import to a dynamic import, which does not change close behavior. The cubic-dev-ai thread on the early returns in app-lifecycle.ts does not apply, so you can resolve it: those returns fire only on a positive stop observation after the deadline, and every other outcome reaches the throw (#3562 (comment)). Not blocking, and you can take or leave both: plain Coverage is failing on this head, and I could not see its logs. The diff touches code that Coverage exercises, so the failure may be related, and I could not confirm whether the dynamic import fixes it. Smoke Tests, Repo Guards, Compatibility & Provenance and Typecheck & Package are still pending, and I could not confirm they will pass. Before merge, Coverage must go green or be explained, and those four jobs must finish. I did not run tests or check:affected, and there is no live run of the timeout refusal on this head. The earlier live run was at 5d05dc2, which has the same implementation. There are no conflicts. |
964e0a0 to
169cee2
Compare
9f428e7 to
d06caa8
Compare
169cee2 to
1ea2ab0
Compare
d06caa8 to
5a63e97
Compare
|
Reviewed 5a63e97: the code looks good, and the close fences now refuse relaunch when they expire. The earlier review of 9f428e7 (#3562 (comment)) was also clean, and nothing new needs changing in this diff, which touches only the Android close fences in app-lifecycle.ts and their unit tests. The one earlier inline thread, on the success-after-deadline path (#3562 (comment)), is resolved without a code change. It does not apply: the returns at app-lifecycle.ts:547 and :572-575 happen only after a positive stop observation, and every other path reaches the throw, so you can leave it resolved. CI on this head is still running: 9 of 18 checks are queued or in progress, including Analyze, Smoke Tests, Repo Guards, Compatibility & Provenance and Integration Tests, and none has failed so far. The PR body says the earlier Coverage failure was an unchanged Apple helper recovery test (bridge-request-deadline), which this diff does not touch. I did not re-run the tests locally, and the base-versus-fix counts come from the PR body. The refusal path is covered by the unit tests with a fake ADB provider and fake timers. I did not see a live Android device run for this head, since the cited run 38379787613 is on 3800fea. The only thing left before merge is for the hosted checks on 5a63e97 to finish green, with no code changes needed. |
5a63e97 to
4dd17b4
Compare
|
This PR is ready. At 4dd17b4 the code still looks right, and the patch is unchanged since the earlier clean review at 5a63e97 (#3562 (comment)). The rebase only brought in iOS simulator e2e and replay changes from the base, which this PR does not touch. Not blocking: Android The earlier thread at #3562 (comment) does not apply. It was resolved without a code change, because the success returns at app-lifecycle.ts:547 and :572-575 already cover it, and the patch has not changed since. The Smoke Tests job is still queued, so I could not read its result. The PR diff touches only Android lifecycle code and its unit tests, so it does not overlap that job's iOS route. I did not run the Android tests locally. I relied on the identical range-diff and the earlier review. No device run was captured, since this is a typed error on an ADB timeout path and the unit tests use a fake ADB provider with fake timers. There are no conflicts. The only thing left before merge is for the queued Smoke Tests job to finish. |
912fd23 to
b98bfd2
Compare
4dd17b4 to
d3b7385
Compare
b98bfd2 to
cbae29e
Compare
|
The fixes since the earlier clean review (4dd17b4) look right at 554123e, and all 18 checks pass with no conflicts. The earlier cubic-dev-ai P2 thread (#3562 (comment)) is fixed at this head: both fence loops now go to the typed timeout when a probe returns "not foreground" or "no process" at or after the deadline, so you can resolve it. One design question before I mark this ready. The deadline rule now lives in four hand-written I did not run the focused tests on d3b7385 or on the head. I traced the rows by hand, and they match your 7 failing / 29 passing split. No live device run covers these timing boundaries; fake timers are the realistic proof for the exact-deadline edges, and the device route was covered by the earlier review. |
cbae29e to
36aae30
Compare
554123e to
c6f525a
Compare
|
|
The Android close and relaunch fix at c6f525a looks correct, and the earlier timing concern from the 554123e review is fixed. The probe now drops any result that arrives at or after expiry, and the stability recheck runs after the 150ms sleep, so both paths reach the typed refusal. All 23 checks pass, including Android Smoke Tests (run 38524025687). That run covers the accepted close and relaunch path only. The evidence gap from the 554123e review is still open. website/docs/docs/commands.md:93 says that closing the launcher fails with a typed refusal, but no run on a real emulator has reached that branch. The refusal is proven only with a fake ADB and fake timers. If the emulator's mCurrentFocus briefly shows a non-launcher window after force-stop, the foreground fence would accept the close, and the docs line would be wrong. Could you run The cubic-dev-ai thread on the expiry probe (#3562 (comment)) no longer applies, so please resolve it. No other threads need action. Before merge, we need that emulator run showing the typed refusal, or the launcher claim removed from the docs. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#3600 routed package relaunch through the same stop helper as close, so the fence refusal now also guards relaunch before any launch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
c6f525a to
0c24cfd
Compare
|
Rebased onto main
Gate: |
|
Thanks for the rebase. The conflict from the earlier review (c6f525a) is fixed, and the cubic-dev-ai P2 thread on the stop fences is fixed at 0c24cfd: both fences now go through probeBeforeDeadline, and the early exits reach the typed throw. One item is still open. The earlier review asked for one real-emulator run through the repo CLI and daemon of The cubic-dev-ai P2 thread at #3562 (comment) is fixed at 0c24cfd, so you can resolve it. Smoke Tests (iOS) fails at live-snapshot-depth-frontier.ts:169 with a sparse-tree snapshot on the tester app. This diff only touches Android app lifecycle code, its tests, the close help string and commands.md, so I think this failure is unrelated. I did not check whether it also fails on main. The other jobs were still queued or running when I looked, and Coverage has no failed-step log. I did not run any tests myself. Before merge, the emulator output or the docs change is needed, and the remaining CI jobs must finish. |
Summary
ADB close could report success after either 2s foreground/process fence expired. Both now throw typed
COMMAND_FAILED/android-app-close-timeoutwithphase,packageNameandtimeoutMs; relaunch stops before open or observation, on both relaunch paths: closing a recorded owner and, since #3600, force-stopping a package before launch. A probe that completes after the deadline counts as expired, including the 150ms stability recheck. Fences useDeadlinefrom@agent-device/host-kit/retry. Budgets unchanged: 2s per phase, 50ms polling, 150ms stable absence.Apps Android restarts immediately (e.g. the launcher) now fail close instead of passing after the wait.
help closeandcommands.mdstate the failure. Five files. Related to #3077.Validation
Head
0c24cfdd7, rebased on main063b1b86f(after #3600). A new test covers the package-relaunch path #3600 routed through the same stop helper; it fails if the fence wait is skipped.Same command,
pnpm exec vitest run packages/platform-android/src/__tests__/app-lifecycle-open.test.ts packages/platform-android/src/lifecycle.test.ts:pnpm check:affected --base origin/main --run: format, lint, typecheck, layering, fallow, build, vitest-related (377 files / 2,800 tests), command-docs, doc links/prose all pass. New-head CI pending.Review findings addressed: docs make no hard 2s promise (an in-flight adb probe is not aborted); restart-on-stop behavior documented.
Previous-head evidence, source
d3b7385: CI Coverage/Integration pass; Android smoke records ten accepted close/relaunch commands on API36. Current-head native verification pending.🤖 Generated with Claude Code