refactor(mobile): one new-session screen for every entry point - #4945
Open
iscekic wants to merge 62 commits into
Open
refactor(mobile): one new-session screen for every entry point#4945iscekic wants to merge 62 commits into
iscekic wants to merge 62 commits into
Conversation
Contributor
Author
|
(bot) E2E is blocked before verifier dispatch: no iOS simulator was unclaimed. The failed bundle's Android emulator, stack, recorders, and slot were released. No visual evidence was fabricated. |
![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 SummaryThe incremental range Files Reviewed (2 files, incremental range 4bee91b..f5c0451)
Resolved since the previous review
Verified as safe (checked and ruled out)
Assumptions
Previous Review Summaries (5 snapshots, latest commit 4bee91b)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 4bee91b)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe new remote-spawn admission gate only rejects files for a CLI that explicitly advertises Overview
Issue Details (click to expand)WARNING
Files Reviewed (7 files, incremental range a4af3a1..4bee91b)
Resolved since the previous review
Verified as safe (checked and ruled out)
Assumptions
Fix these issues in Kilo Cloud Previous review (commit a4af3a1)Status: 3 Issues Found | Recommendation: Address before merge Executive SummaryThe new auto-send latch in Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (22 files, incremental range 261e954..a4af3a1)
Verified as safe (checked and ruled out)
Assumptions
Fix these issues in Kilo Cloud Previous review (commit 261e954)Status: No Issues Found | Recommendation: Merge Executive SummaryThe two commits added since the previous review ( Files Reviewed (21 files)
Scope reconciliation
Verification notes and assumptions
Previous review (commit 88a921c)Status: 6 Issues Found | Recommendation: Address before merge Executive SummaryThe restored Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (23 files changed since afd64f0)
Resolved since the previous review
Verification notes and assumptions
Fix these issues in Kilo Cloud Previous review (commit afd64f0)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental range Files Reviewed (5 files changed since 0381b0d)
Verification notes and assumptions
[Snapshot truncated.] Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by claude-opus-5 · Input: 30 · Output: 4.3K · Cached: 653.6K Review guidance: REVIEW.md from base branch |
Cherry-pick of cloud#4949 adb path only so Android E2E recorder works before that PR merges.
Contributor
Author
|
(bot) BLOCKED: the required Android bot-E2E verdict is still unavailable. The |
…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.
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.
iscekic
force-pushed
the
new-session-github-16aa
branch
from
August 2, 2026 13:27
d1b3ff8 to
0381b0d
Compare
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.
…-github-16aa # Conflicts: # apps/mobile/e2e/AGENTS.md
iscekic
changed the title
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
1 participant
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.
What
Why
A remote CLI spawn now carries the composer text and files to its destination.
This removes the remote-share loss case and keeps one configuration screen.
How
share-submit-params.tssnapshots the trimmed draft and local attachment URIs.share-navigation.tsadds reusable delivery query parameters.use-remote-spawn-dispatch.tsstages the press-time snapshot on ready navigation.use-new-session-share-remote.tssupplies the S1 text and attachments.new-session-configure-form.tsxkeeps the prompt, mode, and model controls for a remote target.new.tsxsettles voice input before both cloud and remote submission paths.share-prefill.tssignals delivery after files queue and clears both delivery parameters.composer-auto-send.tsgates a one-shot destination send on delivery, connection, upload, and attachment capability.[session-id].tsx,session-detail-content.tsx, andchat-composer.tsxforward and consumeautoSend.Verified, unchanged
R1 — one create-session route
session-list-screen.tsx:269and:318pushgetNewAgentSessionPath(...).new-task-button.tsx:25andagents-promo-card.tsx:25push the same path.share-gate-sheet.tsx:226pushesgetNewAgentSessionPath(...).session-list-routes.ts:1resolves each path to/(app)/agent-chat/new.agent-chat/hasnew.tsx,[session-id].tsx, and picker routes only.R2 — existing destination delivery
share-gate-sheet.tsx:222routes New session withshareId;new-session-prompt.tsx:135applies the prefill.share-gate-sheet.tsx:234routes a stored destination withshareId;chat-composer.tsx:261applies the prefill.share-destinations.ts:20includes cloud-agent and CLI rows and hoists live rows.Removed
share-to-new-remote-session.ts,use-share-aware-run-on-change.ts,use-share-staged-latch.ts, and ready-navigation cancellation.Verification
pnpm format— passed.pnpm typecheck— passed.pnpm lint— passed.pnpm check:unused— passed.pnpm test -- --run— 300 files and 2,645 tests passed.pnpm run test:mobile-workflow— 161 tests passed.git diff --check— passed.iOS bot E2E
iscekic/eslint-plugin-zod-utilswith Auto Efficient. A real Safari share selected its live row and left the shared text unsent in the destination composer.hello from the s1 screen; the spawned session export contained it.e7-tiny.PNGin the gate and S1. The connected CLI advertised attachment support. The spawned session helde7-tiny.PNG, uploadedand an unsent composer.The continuation verifier restored its temporary Appium patch, stopped its local share server, and passed its baseline check.
Post-review repair
auto send verification 4945auto-sent to the spawned CLI session.xunsent in the composer.Visual Changes
E1 — one S1 screen
E2 — share gate
E5 — remote S1 start
E6 — shared remote start
E4 — share to a CLI destination
E3 — share to a cloud-agent destination
E7 — shared image remote start
R1 — repaired text and image auto-send
R2 — files-only draft remains unsent
C1 — capable CLI admits image payload