refactor(mobile-e2e): simplified runbook, scripted stub setup, load-tolerant flows - #4960
refactor(mobile-e2e): simplified runbook, scripted stub setup, load-tolerant flows#4960iscekic wants to merge 26 commits into
Conversation
…olerant flows - rewrite e2e/AGENTS.md in simplified technical English: task-structured, scripts own the mechanics, 410 -> ~215 lines - new e2e/github-stub.sh: PR-review stub server + env line + token seed as one reversible command pair; failed starts clean up after themselves - emulator-start --wait retries a failed launch once itself (boot-envelope timeout keeps the GPU, other failures switch to software rendering); the runbook's manual GPU decision table is gone - appium.sh hierarchy writes to a file and prints the path; a raw XML dump into an agent session is large enough to kill it silently - raise iOS cold-launch/settle budgets to 120s (timed out under parallel dual-bundle load); document ANR-under-load handling - simulator claims record originalDeviceName and bootedByClaim intent in the initial write, so a lost post-boot rewrite can no longer leak a renamed or still-booted device on release
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe increment ( Previous findings resolved in this increment
Verification performed
Notes and assumptions
Files Reviewed (1 file)
Previous Review Summaries (6 snapshots, latest commit df4aac2)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit df4aac2)Status: 2 Issues Found | Recommendation: Address before merge Executive SummaryThe new Overview
Issue Details (click to expand)WARNING
SUGGESTION
Verified in this increment
Notes and assumptions
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous review (commit f752367)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of Verified in this increment
Notes and assumptions
Files Reviewed (1 file)
Previous review (commit 36f3437)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of Verified in this increment
Files Reviewed (3 files)
Notes and assumptions
Previous review (commit 66b7904)Status: 4 Issues Found | Recommendation: Address before merge Executive SummaryThe copy's "already-expired dummy refresh token" does not actually prevent a refresh: git-token-service gates refreshes on the access token's expiry alone, so the bogus token is POSTed to GitHub and the copy row ends up revoked instead of merely expired. Overview
Issue Details (click to expand)WARNING
SUGGESTION
Verified in this increment
Files Reviewed (3 files)
Notes and assumptions
Fix these issues in Kilo Cloud Previous review (commit 1a34adb)Status: 9 Issues Found | Recommendation: Address before merge Executive SummaryThe new Overview
Issue Details (click to expand)WARNING
SUGGESTION
Verified in this increment
Files Reviewed (3 files)
Notes and assumptions
Fix these issues in Kilo Cloud Previous review (commit 973fdf5)Status: 7 Issues Found | Recommendation: Address before merge Executive SummaryThe new Overview
Issue Details (click to expand)WARNING
SUGGESTION
Verified in this increment
Files Reviewed (3 files)
Notes and assumptions
[Snapshot truncated.] Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by claude-opus-5 · Input: 36 · Output: 9.2K · Cached: 770.6K Review guidance: REVIEW.md from base branch |
- emulator-start retry: rethrow the already-exists guard (a retry would tear down a live emulator) and keep attempt 1's error when the interim teardown itself fails - github-stub: curl transport failures now route through seed_fail, so a failed start still rolls back the session and env line - github-stub: guard against a .env.local without a trailing newline before appending, so the marker line can never merge into (and later delete) an existing variable
- tmux targets use exact-match (=name): bare -t prefix-matches, so a sibling worktree whose slug extends ours could be probed or killed in its place - simulator claims write atomically (temp + rename/link): in-place truncation let an unlocked reader see a partial file, judge it corrupt, and delete a live claim - a claim abandoned after its boot (rename failure) now powers the device off, or keeps the claim when even that fails - simctl bootstatus is bounded (15 min) so a wedged CoreSimulator cannot block a claim forever - emulator-start tears down after a failed second attempt too - github-stub start claims its port machine-globally and verifies its own session survived, so two worktrees cannot share one stub silently; stop releases the claim - appium: driver install re-checks inside the install lock; server startup verifies port ownership before adopting a listener; stop escalates to SIGKILL instead of orphaning a live server - record.sh serializes start/stop per device with the shared process lock - open-app consumes its launch budget in 30s slices so an ANR dialog is answered promptly instead of after the full wait - login.sh matches the recipient literally, not as a regex - hierarchy mktemp template puts Xs last (BSD mktemp leaves embedded Xs literal, making every bare call after the first fail) - AGENTS.md: per-platform mixed fixtures, pull-and-grep for ADB dumps, stub-stop token residue note - mobile-workflow tests updated for the raised launch/settle budgets
- appium: the recorded-server fast path now verifies the recorded pid owns the listener before adopting (a recycled pid plus a sibling's server on the same port answered /status and got adopted) - github-stub: seeding extracted into seed_token; new 'seed <email>' subcommand adds a token row for a second signed-in account (the other platform's verifier) without touching the running stub - github-stub: the port claim covers only the choose-to-bind window and an EXIT trap releases it, so an interrupted start cannot leak claims and erode the port range - github-stub: githubUserId carries the pid — concurrent seeds in the same second cannot collide on the unique github_user_id - AGENTS.md: the ADB dump example creates its own mktemp file instead of assuming $SCRATCH
devSeedUserGithubToken can answer 200 with success:false (row not upserted); the status code alone reported a seed that never landed.
A process that survives even SIGKILL kept its state file removed and the stop reported success; the caller then released device claims over a live server. stop now fails and keeps the state in that case.
- claim_port reaps an orphaned port claim (older than 120s with nothing listening) and the start path traps INT/TERM/HUP besides EXIT, so a killed start can no longer shrink the port pool permanently - githubUserId derives deterministically from the account email: the upsert's setWhere only updates a matching row, so the fresh random id made every re-seed of an already-seeded account report success:false and tear the stub down with a misleading hint - lsof ownership probes tolerate no-match exits; pipefail otherwise killed appium.sh before the fresh-start fallback could run
…ling - appium startup adopts a port only on proven ownership (listener pid equals ours); an empty lsof keeps polling instead of adopting a vanished or foreign server - github-stub signal traps exit instead of continuing a start that just released its port claim - success:false with 200 and no error means an account seeded under an older random id — keep that usable row instead of tearing the stub down
success:false can come from an older random-id seed or from a real GitHub connection on the dev account; the keep-message now says so instead of implying only legacy seeds, and notes the stub accepts any token.
…lback - a kept token row is verified to be the stub's (getUserAuthorization githubLogin == kilo-stub-user); a real GitHub connection row fails with a drop-the-row hint — under the stub its refresh bypasses GITHUB_API_BASE_URL, hits real github.com, and revokes itself - start rolls back everything (session, env line, state, port claim) via one EXIT trap when it dies before the final state write, so an aborted start no longer blocks the next one behind 'session already runs' - appium ownership checks degrade to pid+status when lsof is missing or cannot attribute the socket, instead of killing healthy servers across 50 port blocks
- the row probe checks HTTP status and error envelope before advising: a failed probe reports 'could not determine the existing row' instead of confidently telling the operator to delete a row it never examined; a kept row must also be connected (a revoked stub row gets the drop hint) - start's rollback undoes only what this run did (session-created and env-added flags), so a start losing the session-creation race cannot kill the winner's session, env line, or state - the recorded-server reuse path applies the same blind-lsof rule as the start loop: adopt on pid+status after three blind probes instead of orphaning a healthy server and consuming a port block per invocation - drop dead SEED_FAIL_MODE and the stale random-id comment
- github-stub creates its session under a pid-unique name armed before the create and renames to claim the canonical one: a signal at any instant tears down only our own session, and a lost rename race touches nothing but our port claim; the env-added flag arms before the append for the same reason - the appium reuse probe rechecks pid liveness and /status on every pass, so a server dying mid-probe is cleaned up (stop_server) instead of adopted; a foreign listener still only drops our state
…windows Concurrent stub commands raced over the session name, env line, and state dir; each guard narrowed the window without closing it. The shared process-lock (the appium.sh pattern) removes the class: one stub command per worktree at a time, so the entry check is authoritative, the rollback flags only order against signals, and the unique-name rename dance goes away.
Both row branches run after the account answered the seed call, so the sign-in hint contradicted what the script had just proved.
- every network call under the stub lock carries --max-time 30, so a hung local API cannot hold the lock to its 300s wait bound; status runs unlocked (read-only) and answers during a long start - a failed session create (a survivor of a SIGKILLed lock wrapper got there first) resets the created flag before exiting, so cleanup cannot touch the survivor's session - align the env-append comment with the lock (the rename step it cited is gone)
pnpm dev:seed app:github-integration-copy <email> finds the newest real integration in the shared dev database (fakes excluded: kilo-stub* logins and far-future expiries — a real refresh token expires within months), decrypts it with git-token-service's local dev key, re-encrypts for the target account under a synthetic github_user_id (the pair index is unique), and upserts. Scenarios that need a real integration (cloud agents) run against the copy; with no valid donor the command fails with an explicit BLOCKED message, and the runbook tells verifiers to report VERIFICATION BLOCKED rather than fake or silently skip. Verified against the dev db: copy mechanics end to end, and the blocked path on a database holding only seeded rows.
The e2e harness changes belong to #4960. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fqia1fHqrSCcW5WzHQ4gPM
The e2e harness changes belong to #4960. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fqia1fHqrSCcW5WzHQ4gPM
The e2e harness changes belong to #4960. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fqia1fHqrSCcW5WzHQ4gPM
The e2e harness changes belong to #4960. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fqia1fHqrSCcW5WzHQ4gPM
The e2e harness changes belong to #4960. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fqia1fHqrSCcW5WzHQ4gPM
The e2e harness changes belong to #4960. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fqia1fHqrSCcW5WzHQ4gPM
The assertions pin apps/mobile/e2e content that #4960 owns. Point the seed help text at the stub that exists on main.
The assertions pin apps/mobile/e2e content that #4960 owns. Point the seed help text at the stub that exists on main.
The assertions pin apps/mobile/e2e content that #4960 owns. Point the seed help text at the stub that exists on main.
The assertions pin apps/mobile/e2e content that #4960 owns.
The e2e harness changes belong to #4960.
The e2e harness changes belong to #4960.
The e2e harness changes belong to #4960.
The assertions pin apps/mobile/e2e content that #4960 owns. Point the seed help text at the stub that exists on main.
The e2e harness changes belong to #4960.
The assertions pin apps/mobile/e2e content that #4960 owns. Point the seed help text at the stub that exists on main.
The e2e harness changes belong to #4960.
The assertions pin apps/mobile/e2e content that #4960 owns. Point the seed help text at the stub that exists on main.
The e2e harness changes belong to #4960.
The assertions pin apps/mobile/e2e content that #4960 owns.
PR #4960 carries these changes and merges first. This branch keeps no copy. - dev/local/mobile-android.ts - dev/local/mobile-simulator.ts - dev/local/tmux.ts - dev/seed/app/github-integration-copy.ts
PR #4960 carries these changes and merges first. This branch keeps no copy. - dev/local/mobile-android.ts - dev/local/mobile-simulator.ts - dev/local/tmux.ts - dev/seed/app/github-integration-copy.ts
PR #4960 carries these changes and merges first. This branch keeps no copy. - dev/local/mobile-android.ts - dev/local/mobile-simulator.ts - dev/local/tmux.ts - dev/seed/app/github-integration-copy.ts
PR #4960 carries these changes and merges first. This branch keeps no copy. - dev/local/mobile-android.ts - dev/local/mobile-simulator.ts - dev/local/tmux.ts - dev/seed/app/github-integration-copy.ts Drop dev/local/github-integration-copy.test.ts. It imports the reverted seed file.
Summary
Refactors the mobile E2E harness so weak agent models can run it without misreading anything, and removes the flakiness found in a full-load test (two parallel dual-platform bundles: 2 dev stacks, 2 iOS simulators, 2 Android emulators).
e2e/AGENTS.mdrewritten in strict simplified technical English: task-structured, one instruction per sentence, 410 → ~215 lines. Everything mechanical now defers to scripts; the runbook keeps only rules and judgment.e2e/github-stub.sh start|status|stop: the PR-review GitHub stub setup was a three-step manual ritual (env-file edit, next-auth fake-login curl dance to seed the token row, hand-started tmux server). It is now one command each way, with the env line tagged and removed on stop.emulator-start --waitnow applies the launch/GPU retry policy itself (one automatic relaunch: boot-envelope timeout keeps the GPU, every other failure switches to software rendering). The runbook's 40-line decision table is gone; callers just read the error after the second failure.originalDeviceNamein the initial claim write, so a claim that dies between rename and the full record write can still restore the device's real name on release (found a simulator carrying a staleKilo E2E - …label from exactly this window).Verification
bundle <avd>(stack + iOS simulator + Android emulator — both machine slots full), real e2e-verifier sessions executing launch scenarios on all four devices. Final round with these fixes: both sections VERIFICATION PASSED, teardown clean (no leaked slots, devices, emulators, or stacks) in all five rounds. The campaign is what surfaced the iOS launch budgets, the ANR misjudgment, the hierarchy context-dump, and the claim-flag loss this PR fixes.github-stub.shexercised end to end against a live worktree stack: start (server up, env line written, token row seeded through fake-login), fixture served over HTTP, status, stop (env line removed, session gone), plus the failure path (missing user) cleaning up after itself.pnpm typecheck,pnpm lint,pnpm formatgreen.pnpm test: web suite green (8814 passed / 3 skipped); the twodev/local/tmux.test.tsfailures reproduce identically on origin/main on this machine (pre-existing, unrelated).Visual Changes
N/A
Reviewer Notes
dev/local/mobile-android.ts(emulator-starthandler);stopAndroidEmulatoris already safe to call with no record, which is what makes the blind stop-then-retry correct.github-stub.shrefuses to overwrite aGITHUB_API_BASE_URLline it did not write (marker comment).