feat(mobile): continue read-only sessions - #4958
Open
iscekic wants to merge 36 commits into
Open
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of Incremental scope analysis
Files Reviewed (PR scope: 6 files, 0 changed since last review)
Assumptions
Previous Review Summaries (2 snapshots, latest commit 05af6e2)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 05af6e2)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental review of commit Resolved since last review
Verification of the fix against current code:
Files Reviewed (2 files in incremental diff)
Files unchanged since the previous review ( Assumptions
Previous review (commit 94eb664)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe continue-session repo prefill derives an Overview
Issue Details (click to expand)WARNING
Files Reviewed (6 files)
Verified as non-issues
Assumptions
Reviewed by claude-opus-5 · Input: 28 · Output: 4.6K · Cached: 675.3K Review guidance: REVIEW.md from base branch |
…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
- 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.
- donor filter excludes 13-digit synthetic github_user_ids (stub seeds and earlier copies): revocation lands only on the real row, so a surviving copy could hand out dead credentials as a fresh success - app:github-integration-copy --remove <email> deletes the account's token row, so an account can return to stub use; runbook and usage text now require separate accounts for stub and integration scenarios - verified live: remove clears the copied row and is idempotent
One instruction per sentence in the integration and stub sections; no rule changed meaning.
- the donor's access token is checked against api.github.com before the copy: database metadata cannot prove a rotated or GitHub-revoked token dead, so silent dead-credential copies reported success - the copy row carries a dummy, already-expired refresh token: GitHub refresh tokens rotate on use, so a refreshing copy would kill the donor's stored refresh token and poison every later copy - usage and runbook text updated to the access-token-only contract
The copy relocates real credentials, so every destructive path is now guarded and every shared-grant hazard is stated: - assertLocalDatabaseTarget: USE_PRODUCTION_DB=true and non-loopback hosts are refused, making the dev-only claim enforceable (verified live) - --remove deletes only a synthetic row (copy or stub seed) and names what it removed; a real connection is refused with instructions - a copy refuses to overwrite a real connection on the target account - donor lookup also matches google_user_email (normalized_email is nullable) - usage and runbook state the real blast radius: the copy shares the donor's OAuth grant, so disconnect on a copied account revokes the DONOR's authorization and no re-copy repairs it — disconnect scenarios are blocked on copied accounts; commit attribution carries a synthetic id - stub status reports 'starting' when a session has no state yet, instead of claiming up for a stub that is not usable - lock-bounded curls raised to 120s (30s was the budget this PR already found too small under full parallel load) - a failed tmux create distinguishes a concurrent session from a tmux error
A dead database dumped a driver AggregateError stack trace, and a dead network read as 'no valid donor'. Both now fail with one line naming the command that fixes them, so an agent does not spend a round guessing at ECONNREFUSED output or chasing a false BLOCKED. Verified live with postgres stopped.
- the synthetic-only rule is re-asserted in the upsert's setWhere, so a real OAuth callback landing between the pre-check and the write cannot be overwritten; zero updated rows reports that explicitly (postgres predicate verified against synthetic and real id shapes) - donors need >10 minutes of access-token life (twice git-token-service's 5-minute refresh buffer): a token inside the buffer passes the live probe, then triggers a refresh the copy cannot perform - the target lookup tries the exact sign-in email before the normalized fallback, since normalized_email is not unique
…dget - the 'can never refresh' claim was wrong: git-token-service gates refresh on the ACCESS token's expiry, so the dummy refresh token IS sent and the copy ends up revoked. Docs, usage text and the runbook now describe that end state, the tool reports usableUntil, and donors need 35 minutes of access life so a copy is usable for at least 30 - --remove reports what actually went away (.returning()), so a row replaced mid-command cannot read as removed - stub lock --wait raised to 900s, above the ~8 minute worst-case hold the 120s per-call budgets allow, so a queued stop or seed waits instead of failing - drop the now-unused 'or' import
The 35-minute SQL margin is measured before up to ten 15-second GitHub probes, so a copy could return with less than the promised 30 usable minutes. The selected donor is now re-checked against the clock after its probe, and one helper owns the usable-until arithmetic the filter, the check, and the report all share.
A campaign verifier died with no sentinel right after a doctor --json dump (the whole environment, PATH included). The runbook now states the general rule — bound anything that can print a lot — and names --json diagnostics alongside the Appium and hierarchy cases it already covered.
Two review corrections, both verified against dev/local/mobile-android.ts: doctor ignores flags and always prints the whole environment (PATH included), so the runbook's 'plain doctor is safe' advice pointed at the unsafe form; and tail on JSON leaves an unparseable fragment while the runbook requires parsing dev:status --json. The bounded form is a jq field selector, shown inline for both cases.
jeanduplessis
approved these changes
Aug 3, 2026
main moved the mobile E2E harness to the kilo-workflow repo (#4968). This branch had modified eight of the deleted files plus the simulator claim CLI. Every one of those changes already exists in kilo-workflow, so the merge takes main's deletion. - Delete apps/mobile/e2e/ and dev/local/mobile-simulator.ts. - Delete apps/mobile/e2e/github-stub.sh: kilo-workflow holds a newer copy. - Delete dev/seed/app/github-integration-copy.ts: main dropped the seed tool and kilo-workflow replaced it with mobile_e2e/github-installation.sh. - Take main's dev/local/mobile-workflow.test.ts: the harness assertions moved with the harness. dev/local/tmux.ts and dev/local/mobile-android.ts now match main exactly, so this branch changes only apps/mobile/src.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
What
Why
How
Verification
pnpm format && pnpm typecheck && pnpm lint && pnpm check:unused && pnpm testinapps/mobilepassed before this evidence run (297 files, 2,626 tests).v-r1:VERIFICATION PASSED.S4 observed the normal-entry defaultsMode: Code,Auto Frontier, andRepository: Select repository. S1 observed the read-only banner and CTA without a composer. S2 observedMode: DebugandClaude Sonnet 5, Low thinking effort, both different from S4. S2R observedRepository: kilo-e2e/continue-source.v-r2:VERIFICATION PASSED.S6 observed the S4 defaults with no toast for the empty source. S3 observed the exact unavailable-repository toast, then an usable empty selector listingkilo-e2e/continue-sourceandkilo-e2e/other-repo.debug, modelanthropic/claude-sonnet-5, and variantlow.Visual Changes
Additional runtime evidence:
Reviewer Notes
Runtime limits
The repository prefill and fallback toast ran against a locally seeded
platform_integrationsrow with installation ide2e-pr4958-retry-5407-install; its repository list is a local fixture. The production query path is real, but the list content is not.app:github-integration-copyandgithub-stub.shwriteuser_github_app_tokens, which this feature never reads. The prefilled flow was not submitted to a live cloud-agent session because the seeded integration has no GitHub credentials and cannot clone.