Mobile review surfaces and invoice download - #4776
Merged
Merged
Conversation
Contributor
Author
|
(bot) Device E2E complete on an iOS simulator, against the committed GitHub API stub and a real Stripe test-mode invoice.
Two findings from E2E were repaired and re-verified on device: the clear control's tap target (38pt → 45×45pt) and the iOS clear write path ( Criteria verified by named automated tests instead (per plan): A1.3 flag-off branch, A2.7 cursored-page behavior, A3.4 sharing-unavailable message, A3.5 null-PDF row. |
The plus-addressed default (e2e-mobile+<slug>@example.com) collapsed to one shared backend user because normalizeEmail strips plus aliases, so concurrent worktrees signed into the same account. Derive e2e-mobile-<slug>@example.com instead; hyphens survive normalization and each worktree gets a distinct user. Lowercase the derived local part so first-time signups pass validateMagicLinkSignupEmail.
Contributor
Author
|
(bot) Standin review — head This is a standin review produced by the mobile workflow orchestrator because the automated reviewer posts nothing on this repository (its
Orchestrator triage: no actionable findings, so no repairs were dispatched and nothing was pushed. The device-level risks listed above are already covered by the on-device E2E results posted earlier on this PR (stub-backed PR-review flows and a real Stripe test-mode invoice). An independent orchestrator full-diff review reached the same conclusion. PR #4776 remains mergeable with all checks green on |
iscekic
enabled auto-merge (squash)
July 26, 2026 15:54
iscekic
disabled auto-merge
July 26, 2026 15:58
jeanduplessis
approved these changes
Jul 27, 2026
iscekic
added a commit
that referenced
this pull request
Jul 28, 2026
Desk review, five fresh-context probes (kimi-k3, grok-4.5, opus, two haiku role-plays, kilo-auto/efficient), and kilo CLI's own memory surfaced gaps; the valid ones are fixed: - Slot ownership: device-phase dispatches get their own tmux session (slots are owned and reaped by session name; window-named owners leak or share slots); verifier tears down services before releasing. - Terminal states: the PR is the deliverable and everything else closes. COMPLETE deletes the scratch dir (its absence is the completion sentinel); BLOCKED leaves scratch + final-report.md as evidence. Monitors distinguish crash from completion by scratch state. - Void rounds: every role definition now requires a fixed sentinel line; a log without one is a crashed round, never a pass. - Dispatches: handoffs travel via --file only (shell substitution through a tmux command string executes backticks in the file); env strip covers all KILO_*/OPENCODE* vars; --model dropped for role agents (definitions pin models); concrete launch snippets with shell-expanded scratch paths; starter monitors planners. - Loops: hard round caps with recorded-resolution escapes; the plan-review deadlock-breaker has an else branch; Kilobot wait has retriggers, a waiver, and concrete gh checks (retriggers observed dead on #4776 per kilo memory - green check on head is the review). - Verifier learnings go to scratch (byte-identical baseline restore) or the main checkout's learnings/system/; the orchestrator commits them. .kilo_workflow/ is exempt from section disjointness. - Six learnings added from kilo CLI memory and private harness memory (backtick execution, startup db race, paid-model auth, steering, orchestrator relaunch, simulator E2E limits, nextjs suspend protocol); stale entries updated (no --model, full env strip, sentinel-based void detection, dead retriggers); e2e runbook fixes (persistent remote relay, multi-sim UDID targeting).
iscekic
added a commit
that referenced
this pull request
Jul 28, 2026
…4812) * Add generalized kilo workflow role agents Four repo-wide role agents (plan-reviewer, implementer, impl-reviewer, e2e-verifier) generalized from apps/mobile/.kilo/agent/, discoverable from any cwd via the root .kilo/ config dir. The e2e-verifier moves to kimi-k3 with no step ceiling and bakes in output discipline and the learnings contract. * Add .kilo_workflow: generalized workflow doc, slot semaphore, learnings Generalizes apps/mobile/.kilo/MOBILE_WORKFLOW.md into a surface-agnostic workflow (starter -> planners -> orchestrator -> role-agent loops -> PR), copies the e2e slot semaphore (same machine-global state dir), and seeds learnings/ from WORKFLOW_LEARNINGS.md as one indexable file per entry; learnings/system/ is gitignored for machine-local entries. The mobile .kilo and e2e/ are left untouched pending the follow-up simplification. * Retire the mobile-specific workflow in favor of the kilo workflow Removes apps/mobile/.kilo (workflow doc, learnings, role agents, slot script) now superseded by .kilo_workflow and root .kilo/agent; repoints apps/mobile docs at the new paths. Worktree prep now copies the machine-local learnings/system/ folder into fresh worktrees, and the workflow doc states that agent parallelism is uncapped - only E2E device/stack phases contend for slots. * Simplify the simulator claim to a worktree lock file The claim protocol predated the machine-global e2e slot semaphore and duplicated its job with a preparing/ready state machine, PID-identity liveness, recovery resets, and phase-aware relabeling. With device phases capped by e2e-slot.sh, a claim only needs to answer which worktree owns which simulator: an atomic JSON lock file, stale once its worktree is deleted, labeled 'Kilo E2E - <worktree>' on claim and restored on release. Boot verification (bootstatus terminal-failure detection) is kept. The --phase flag is gone along with the prewarm concept the workflow no longer has; the build-cache claim validation now checks ownership only. dev/local/mobile-simulator.ts drops from 1093 to ~430 lines and its test file from 2921 to ~470. The one failing mobile-workflow test (settle-app Open guard) is pre-existing on main. * Harden the kilo workflow from probe findings and kilo memory Desk review, five fresh-context probes (kimi-k3, grok-4.5, opus, two haiku role-plays, kilo-auto/efficient), and kilo CLI's own memory surfaced gaps; the valid ones are fixed: - Slot ownership: device-phase dispatches get their own tmux session (slots are owned and reaped by session name; window-named owners leak or share slots); verifier tears down services before releasing. - Terminal states: the PR is the deliverable and everything else closes. COMPLETE deletes the scratch dir (its absence is the completion sentinel); BLOCKED leaves scratch + final-report.md as evidence. Monitors distinguish crash from completion by scratch state. - Void rounds: every role definition now requires a fixed sentinel line; a log without one is a crashed round, never a pass. - Dispatches: handoffs travel via --file only (shell substitution through a tmux command string executes backticks in the file); env strip covers all KILO_*/OPENCODE* vars; --model dropped for role agents (definitions pin models); concrete launch snippets with shell-expanded scratch paths; starter monitors planners. - Loops: hard round caps with recorded-resolution escapes; the plan-review deadlock-breaker has an else branch; Kilobot wait has retriggers, a waiver, and concrete gh checks (retriggers observed dead on #4776 per kilo memory - green check on head is the review). - Verifier learnings go to scratch (byte-identical baseline restore) or the main checkout's learnings/system/; the orchestrator commits them. .kilo_workflow/ is exempt from section disjointness. - Six learnings added from kilo CLI memory and private harness memory (backtick execution, startup db race, paid-model auth, steering, orchestrator relaunch, simulator E2E limits, nextjs suspend protocol); stale entries updated (no --model, full env strip, sentinel-based void detection, dead retriggers); e2e runbook fixes (persistent remote relay, multi-sim UDID targeting). * Converge the workflow via multi-model review loops; fix simulator claim leak Second hardening pass from five more fresh-context reviews (grok-4.5, kimi-k3, kilo-auto/efficient, gpt-5.6-sol via codex, plus a deep verification agent) run against the updated doc, looping until rounds stopped producing valid findings: - .kilo_workflow/dispatch-role.sh: checked-in dispatcher encoding the whole contract (tmux wrapping, dedicated session for device roles, full env strip, quoting, log naming, EXITCODE marker) so no agent hand-assembles it; the doc's dispatch section shrinks to its usage. - starter/planner/orchestrator agent definitions in .kilo/agent/ pin permissions (and the orchestrator's model), so unattended runs no longer depend on this machine's global kilo config. - Reviewer sentinels are machine-checkable (FINDINGS: n / No findings. / STOPPED EARLY. as the last line); the verifier sentinel now follows a truth table; void rounds count toward caps and three consecutive voids are an infrastructure blocker; caps are ceilings with only takeover/BLOCKED left at the cap. - Slice review diffs include new files (git add -N); cumulative review base is origin/main...HEAD and runs after a defined synchronization point (deferred project-wide checks); commits stage owned paths only. - BLOCKED evidence is retained (monitor no longer cleans it); a scratch-gone state without a PR is a crash, not COMPLETE; planners write BLOCKED reports too; monitor relaunches cap at three. - E2E loop: quiescence rule before dispatch, leftover cleanup after a dead verifier, E2E-Kilobot alternation cap; slot script reap has a grace window for mid-acquire slots and hardcodes its contract. - Kilobot thread state via GraphQL (new learning with exact queries); clean state is zero unresolved threads; PRs use the repo template with what/why/how inside it. - mobile-simulator: a failed boot whose recovery shutdown also fails now keeps the claim (flagged error) so a peer cannot adopt a possibly-running device - found by Kilobot on this PR; test added. - Learnings: GitHub threads API added; env-strip and dispatch entries updated; system-learnings carve-out for the main checkout. * Fix convergence-round findings: dispatcher pipefail abort, sentinel position, round definition grok convergence review on the hardened doc found four real defects, all introduced by the prior batch: the env-strip grep aborts dispatch-role.sh under pipefail when no KILO_*/OPENCODE* vars exist (a non-kilo dispatcher - now || true, with the same guard in the launch templates); the sentinel is the line above the EXITCODE marker, so void detection reads tail -2 | head -1 instead of the last line or a whole-log grep; the repro gate's blocker phrase now matches the verifier's exact CANNOT REPRODUCE. sentinel; and one round is defined as one full loop-body iteration so caps are unambiguous. * Make slot-reap mtime portable across BSD and GNU stat Kilobot: stat -f %m is BSD/macOS-only; on GNU/Linux it switches to filesystem mode, the fallback made age always 0, and the ownerless-slot grace reap silently never fired. Try stat -f %m then stat -c %Y. * Use the @kilocode-bot mention handle for review retriggers * Compute the dispatcher env strip inside the tmux pane Panel review: STRIP was evaluated from the dispatcher's environment and baked into the command, but tmux panes inherit the tmux SERVER env, so server-carried KILO_*/OPENCODE* strays missing from the dispatcher's env leaked into role agents - the exact poisoning path the strip exists to close (and the header's 'full strip' claim was false). The strip is now an escaped substitution that evaluates in the pane. Smoke-tested via a stub kilo binary with a poisoned tmux global env: zero leaked vars. * Absorb final panel micro-findings: Sol exit-condition and monitor PR check, grok consistency nits Final convergence panel (kimi-k3, kilo-auto/efficient, gpt-5.6-sol, grok-4.5, plus a deep verification agent) on the finished spec: kimi and efficient returned clean; the rest produced five one-line fixes, all absorbed - the Kilobot waiver is now an explicit loop exit alternative; the completion monitor checks the section branch's PR in every touched repository instead of bare gh pr view; the pre-round reviewer snapshot is ordered after the add -N diff generation; and the verifier definition uses the exact CANNOT REPRODUCE. sentinel and lists the extension runbook. * chore(workflow): drop kilobot-no-findings-state learning * chore(workflow): drop mobile-cloud-agent-deterministic-turns-fake-llm learning * chore(workflow): drop kilo-paid-model-auth-wedge learning * chore(workflow): require an approving Kilobot summary comment, not just a green check * chore(workflow): tighten wording after learning removals Drop the now-undocumented deterministic-turns mocking exception, align the planner's gate shorthand and the retrigger waiver message with the approving-summary contract. * Reconcile ported learnings with the generalized workflow The uncommitted-learnings trap described the retired mobile roles' revert-anything-dirty behavior; the generalized verifier restores only its own edits and WORKFLOW.md already forbids dispatch over uncommitted changes. Fold the durable rule (commit learnings promptly; planner-authored learnings are named in the handoff for the first commit) into the Learnings section and drop the learning file. Cross-link the port-offset learning and the e2e runbook's no-export rule so the per-command prefix does not read as a contradiction. * test(extension): default the gateway fixture to safe-mode tools mockKiloApi asserted dangerousToolNames by default while fresh conversations default to safe mode, so every safe-mode spec had to pass toolNames explicitly and carried its own copy of the safe list. Export safeToolNames and dangerousToolNames from the fixture, default the assertion to safe mode, drop the seven spec-local copies, and pass dangerousToolNames explicitly in the nine specs that switch the conversation to dangerous mode. Full chrome e2e suite: 95 passed, 0 failed. * docs(workflow): add a TL;DR README to .kilo_workflow Reviewer-requested orientation doc: how to launch a run, the two terminal states, the pipeline and roles at a glance, the wall-time-for-correctness trade the workflow makes, and what each file in the directory is for.
2 tasks
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.
What
Four mobile items plus one enabling test-harness slice:
/(app)/pr-review/<owner>/<repo>/<number>whenpr_urlis a github.com PR URL and themobile-pr-reviewfeature flag is on; every other URL (GitLab, Bitbucket, Enterprise hosts,malformed) and the flag-off case keep today's browser behavior. The routing decision is a pure,
unit-tested helper built on the existing
parseGitHubPrUrl.listReviewThreadsnow also fetches the PR'sconversation (issue) comments over GraphQL (first page only, bounded at 5×100 with the ceiling
in a comment), mapped through the same comment DTO as review comments. The tab renders one
merged, time-ordered list: threads stay intact as single items positioned at their first
comment, conversation comments render read-only (no reply affordance, non-interactive
reactions), and the empty state only fires when neither kind exists. Previously a PR with only
conversation comments rendered the empty state.
PDF and presents the native share/save sheet, with a busy state (
accessibilityStatebusy,double-tap disabled) and distinct toasts for retryable download failures vs. sharing being
unavailable on the device. The download/share primitive is extracted from kilo-chat's
attachment flow into a shared module with a discriminable failure reason; kilo-chat behavior is
unchanged. Rows with no PDF show no affordance.
hosted_invoice_urlis never opened.gap-3) convention (thealways-mounted reserved helper slot is gone; helper text mounts conditionally, matching the
rest of the app), and an in-field clear control appears whenever the field has content: a true
44×44pt box that clears through the existing uncontrolled-input write path and keeps focus.
Enabling slice: a committed, dependency-free local GitHub API stub
(
apps/mobile/e2e/github-api-stub/) plus the three-step hermetic setup documented inapps/mobile/e2e/AGENTS.md— PR-review surfaces were previously unreachable in E2E because thebackend only talked to real
api.github.com. Also documents that the per-worktree plus-tag loginemail resolves to one shared backend user.
How
PrReviewConversationCommentsGraphQL operation (node selection identical to thelive review-comment selection,
reactors(first: 0)included); the inline comment mapping wasextracted into a shared
mapReviewComment; everylistReviewThreadsreturn path — includingthe null-connection case — routes through the single Zod-parsed builder, so
conversationisstructurally impossible to omit. Cursored pages return
conversation: []without issuing theoperation.
createdAtasc → threads before comments → stablestring identity; missing/unparseable timestamps last) re-sorts the entire loaded set on every
update, so "Load more" pages insert rows in the right position.
Verification
pnpm format && pnpm typecheck && pnpm lint && pnpm check:unusedclean inapps/mobile;pnpm test230 files / 1904 tests green; web jestgithub-pr-review7 suites / 112 testsgreen (incl. a content lock on the new GraphQL document).
incl. ties and empty threads; invoice row-state and failure-message selectors; clear-button
visibility; backend conversation present/absent/paginated/cursored-empty/null-connection.
real Stripe test-mode invoice runs before merge; results will be posted here.
Notes
reserved slot was the cause of the reported 36pt gap); it matches
rename-modalandmanual-review-screen.