docs(mobile): workflow learnings log and planner simplicity mandate - #4782
Conversation
|
(bot) Standin review (round 1) — replaces the abandoned Kilobot per run instructions. Fresh mobile-reviewer (kimi-k3, high) over the full origin/main...HEAD diff at head e0e4fa1. Result: No findings. Verified: all eight plan edits present word-for-word at the stated anchors; full-doc consistency of MOBILE_WORKFLOW.md and agent/mobile-plan-reviewer.md (no contradictions, duplication, or broken cross-references; heading levels and voice match); commit split matches the ledger (5e51271 = item 1, e0e4fa1 = item 2, no cross-bleed); no AI attribution; clean working tree. Residual risks (non-actionable): GitHub-side metadata (base, assignee) was outside the reviewer's read-only permission set — confirmed separately by the orchestrator; the one-line usage note in WORKFLOW_LEARNINGS.md compresses the role-agent write path, but is verbatim from the accepted plan and defers to MOBILE_WORKFLOW.md for the full contract — documentation-precision observation, not a defect. |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| owner=$(cat "$s/owner" 2>/dev/null || echo) | ||
| [ -n "$owner" ] || { rm -rf "$s"; continue; } | ||
| # legacy holders predate this protocol; only tmux liveness decides | ||
| tmux has-session -t "=$owner" 2>/dev/null || rm -rf "$s" |
There was a problem hiding this comment.
WARNING: tmux has-session failure is treated as "session is dead" even when it fails for an unrelated reason
tmux has-session -t "=$owner" 2>/dev/null || rm -rf "$s" cannot distinguish "no such session" from any other failure of the tmux command itself (e.g. tmux not on PATH, no tmux server running, socket permission errors). Any of those causes reap() to delete every currently-held slot, silently defeating the semaphore (all callers proceed as if a slot were free) instead of failing loudly. Consider checking the failure mode more precisely, or failing safe (keep the slot) when the tmux invocation itself errors out, rather than only when the session lookup cleanly reports "not found".
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (1 file, incremental)
Fix these issues in Kilo Cloud Previous Review Summaries (4 snapshots, latest commit 5570103)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 5570103)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryUnresolved finding: dropping permission-level denies on mobile role agents removes the technical backstop against prompt injection, relying solely on instructions; the incremental fixes in this update (learnings authorship wording, slot-script path, cleanup checklist) introduce no new issues. Overview
Issue Details (click to expand)WARNING
Files Reviewed (3 files, incremental)
Fix these issues in Kilo Cloud Previous review (commit 6dd36e1)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe latest commit removes permission-level denies (edit/bash/git/gh) from the mobile reviewer, plan-reviewer, implementer, and e2e-verifier agents in favor of instruction-only enforcement, weakening the technical backstop against prompt injection in untrusted content these roles read. Overview
Issue Details (click to expand)WARNING
Files Reviewed (6 files, incremental)
Fix these issues in Kilo Cloud Previous review (commit 491331a)Status: No Issues Found | Recommendation: Merge Executive SummaryThe follow-up commit fixes the prior Files Reviewed (1 file)
Previous review (commit c17ca0a)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe new Overview
Issue Details (click to expand)WARNING
Files Reviewed (5 files)
Reviewed by claude-sonnet-5 · Input: 22 · Output: 4.2K · Cached: 431.4K Review guidance: REVIEW.md from base branch |
|
(bot) Review remark addressed: |
|
|
||
| While a role agent runs, the orchestrator checks on it about every 7 minutes and unsticks infrastructure failures: a wedged or crashed kilo CLI, a dead tmux window, or a hung service or simulator the agent cannot restart itself. Product, logic, or review problems are not stuck states — route those through the escalation ladder (Delegation and Escalation). When the agent's CLI process exits, react immediately: collect its result and continue the loop. The 7-minute cadence is only the ceiling for detecting a wedge, never a wait between dispatch and result. | ||
|
|
||
| Agent definitions allow every command and edit. The only remaining permission denial is `task`, kept because it removes the accidental dispatch path at zero cost — it is not airtight, since a shell `kilo run` can still dispatch; the workflow has exactly one dispatcher, the orchestrator, and role agents never dispatch agents by instruction. Every boundary — no dispatch, reviewers never modify the tree, the implementer never commits, pushes, or opens a PR — is enforced by instruction, not permission. Deny lists caused void review rounds (a reviewer whose blocked command made it exit with no verdict, which read as a pass) and takeover churn; the workflow trades enforcement for reliable rounds and accepts that a misbehaving agent can do what it was previously blocked from. |
There was a problem hiding this comment.
WARNING: Removing permission-level enforcement for reviewer/verifier roles widens prompt-injection blast radius
This commit removes edit: deny / restricted bash allowlists from mobile-plan-reviewer.md and mobile-reviewer.md (now edit: allow and bash: "*": allow), and drops the git commit/push, gh pr*, and socat denies from mobile-implementer.md and mobile-e2e-verifier.md. The rationale documented here (deny lists caused void review rounds) is reasonable, but these reviewer/verifier roles read untrusted content as part of their job — plan files, diffs, and repository files that could contain injected instructions. Previously, permissions provided a technical backstop even if the model followed injected instructions to edit, commit, or push; now that backstop is gone and the only defense is the system-prompt instruction not to. Worth confirming this tradeoff is intentional and acceptable for this workflow, since a successful prompt injection can now actually mutate the repository or push/open a PR instead of merely being blocked at the permission layer.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
(bot) Standin review (round 1, Batch P head 6dd36e1) — supplied by the orchestrator per run instructions; the automated reviewer is not the gate. Fresh mobile-reviewer (kimi-k3, high) over the full Verified with no findings: login-preflight claims (claim verify, both Findings (3), triaged by the orchestrator — all three accepted, repairs in flight:
The existing Kilobot comment on this PR was re-read after this push per run instructions: it currently reports "No Issues Found" for the previous head (its earlier |
|
(bot) Standin review round 1 findings — all three addressed and pushed at head
A fresh mobile-reviewer over the repair diff returned |
|
(bot) Triage of the Kilobot review comment (edited in place for head Disposition: not actionable — replied with evidence, no code change. This is the exact trade-off this PR deliberately ships, and the "technical backstop" the warning describes did not exist in any enforceable sense:
Kilobot remains a non-gate reviewer per run instructions: never waited on, never retriggered; its existing comment is re-read after each push and triaged as above. |
|
(bot) Standin review round 2 (head
A fresh standin review over the full PR diff at the new head follows. |
|
(bot) Standin review (round 3, Batch P head Confirmed accurate with no findings: all Android-section acceptance criteria (GPU policy, bounded liveness→visibility(serial)→readiness loop, envelope bounds-not-SLAs, process-gone handling, glue order, mid-test recovery, App Links one-liner, preserve-list); login-preflight claims (claim verify, both reverses from Finding 1 (minor — Why no repair in this PR: this batch's accepted plan and handoff carry a binding non-goal — no changes to The reviewer's residual note (first With this triage posted, round 3 has zero actionable findings on the exact latest head |
|
(bot) Re the review warning on MOBILE_WORKFLOW.md:43 (permission denies removed): this is a deliberate, requester-directed trade-off, not an oversight. The deny lists caused void review rounds (a reviewer whose blocked command made it exit verdict-less, which read as a pass) and orchestrator takeover churn — both observed repeatedly in real runs. The workflow now trades the technical backstop for reliable rounds: every boundary remains as an explicit instruction in each agent definition, the |
2 participants
Batch M — mobile workflow doc updates: learnings log + planner simplicity mandate
Docs-only batch: three files under
apps/mobile/.kilo/, no code, no device/E2E work. Based onmain(not stacked).Item 1 — Workflow Learnings log and read/write contract
New
WORKFLOW_LEARNINGS.md: a durable log of environment blockers (broken local stacks, credential and env-var traps, simulator quirks, tool wedges) and their fixes, with## Plannerand## Orchestratorsections and a one-line usage note pointing at the contract. No seeded content.The contract lives in
MOBILE_WORKFLOW.md:### Workflow LearningsGround Rules subsection: entry shape (symptom, cause, fix, a few lines each), dedup rule (read first, extend an existing entry instead of duplicating), write ownership (planner and orchestrator only; the orchestrator records blockers role agents hit), and the shell-write path around the kilo.kilo/edit gate (verified by live probe: only the edit/write tools are gated; bash writes succeed).Item 2 — Planner simplicity mandate
MOBILE_WORKFLOW.mdnow directs the planner to plan the simplest viable shape of every item — feature-wise as much as code-wise — and to challenge needlessly complex requests: hands-on, raise it with the user before planning the complex shape; hands-off, decide with best judgment, ship the simpler shape, and record the decision and reasoning in the plan, the handoff, and the PR description. Wired end to end:agent/mobile-plan-reviewer.mdchecklist bullet extended to flag feature shapes needlessly more complex than what delivers the same user valueSimpler-shape decisions (recorded per the mandate)
Both items shipped in their minimal shape: one shared contract paragraph in Ground Rules instead of duplicating read/write rules per role; read points folded into existing step-1 sentences instead of new steps; a one-line usage note in the new file instead of restating the contract; a single-bullet extension of the reviewer agent instead of a new review section. No feature-shape reduction was needed — the request is already minimal. Recorded assumption: "fold into role descriptions" is satisfied by the Ground Rules subsection (shared mechanics) plus inline read points in Planning/Orchestration step 1, since Ground Rules is the doc's designated home for role-shared rules.
Verification
mobile-plan-reviewerloop before implementation (round 1: three findings, all accepted and fixed; round 2: no findings).mobile-reviewerover the complete diff — no findings.pnpm format:check,pnpm typecheck,pnpm lint,pnpm check:unusedinapps/mobile/all pass;git diff --checkclean.Addendum: device-slot semaphore
apps/mobile/.kilo/e2e-slot.sh— a machine-global slot semaphore (default 3) capping concurrent device-bound work (simulators, emulators, local stacks, native builds) across parallel workflow runs, while planning/implementation/review/CI stay uncapped. Slots are owned by tmux session name and auto-reclaimed when the owning session dies. State lives in~/.cache/kilo-e2e-slots(override:E2E_SLOT_DIR) so every worktree's copy contends for the same slots. Referenced from the workflow doc's Local Tooling section ande2e/AGENTS.md; the existing e2e helper scripts stay slot-unaware by design — the slot wraps the phases that call them. Battle-tested across the parallel workflow runs that produced #4771–#4785.Addendum: Android emulator runbook + trust-based role-agent permissions (Batch P)
Two docs items plus a contract amendment, pushed as two commits. No device work; verified by review and checks, not by booting an emulator.
Android emulator runbook (
e2e/AGENTS.md,## Android Emulatorrewritten). The old section flailed under parallel-workflow load: an unboundedadb wait-for-device, a swiftshader-only launch line, no failure envelope, and relaunches attempted against a surviving emulator that still held the AVD lock (observed: 10-minute pointless polls). The rewrite ships: an ordered two-attempt GPU policy with an observable switch signal; a single bounded boot-wait loop (liveness viapgrep→ visibility viaadb devices -l, which yields the serial → readiness viasys.boot_completed) replacing the barewait-for-device; concrete envelope numbers (idle ≈ 1–3 min, up to 8 min under load — relaunch-rule bounds, not SLAs); an observable process-gone check (pgrep,emu killwith serial,pkillwithout) before the single relaunch; the ordered glue sequence slot → launch → boot wait → claim → build →login.sh, withlogin.sh's preflight owning bothadb reversemappings (nextjsAPI port +mobileMetro port frompnpm dev:status --json) and the dev-client deep link, so no manual reverse/am starton the primary path; a mid-test recovery block (pm clear→ rerunlogin.sh, or manual reverse +am start; reinstall only on fingerprint change); and an App Links one-liner. GPU decision record: the docs saidswiftshader_indirectwhile a real run used-gpu host; neither proved the flag caused the timeouts (the readiness probe was wrong). On an Apple Silicon host with a display, host GPU is the emulator's default and offloads the CPU — the contended resource under load — so attempt 1 is-gpu hostand swiftshader is the conditional fallback after observed process death, not the default.Trust-based role-agent permissions (all four
agent/*.md+MOBILE_WORKFLOW.md). Every bash deny list,edit: deny, andbackground_process: denyis removed; all four agents now allow every command and edit, withtask: denythe sole remaining denial (verifier keepsmaestro_*: allow). Deny lists caused void review rounds — a reviewer whose blocked command made it exit with no verdict, which read as a pass — and orchestrator takeover churn; worse, with a broad bash allowlist the denials were unenforceable theater, since shell writes andnohup ... &bypass them anyway. The honest trade-off: a misbehaving agent can now do what it was previously blocked from doing — commit, push, open PRs, edit during review, or even dispatch agents via a shellkilo run, since only the built-in task tool stays denied, closing the accidental path. The workflow accepts that in exchange for eliminating void review rounds and takeover churn; every boundary (no dispatch, reviewers never modify the tree, the implementer never commits/pushes/opens PRs) is now stated as explicit instruction in each agent body and in the workflow doc. The reviewers in this batch's review loop already ran under the new permissive definitions and returned proper verdicts — the change doubles as its own smoke test.Learnings committed with the PR (contract amendment). One sentence appended to
### Workflow Learnings: learnings written during a run are part of the run's deliverable — the orchestrator commits and pushes them with the run's PR so future runs can use them once merged. This run wrote no learnings entries (no environment blockers hit), so there is no third commit.