refactor(workflow): idiot-proof the workflow end to end - #4874
Open
iscekic wants to merge 32 commits into
Open
Conversation
Delete learnings whose fixes landed in scripts or WORKFLOW.md (slot teardown, steer.sh, dispatch-role.sh, monitor rules, TTY rule, env-facts rule, PR image upload), merge overlapping entries (kilo startup crashes, Android emulator claims, Android cold-start deep link, device-auth pending limit, iOS26 Maestro quirks), and trim stale or contradicting paragraphs.
New scripts encode what WORKFLOW.md previously taught in prose: await-role.sh (round completion, sentinel, void/stall detection), launch-interactive.sh (planner/orchestrator launches: env strip, TTY, tmux targeting), slice-diff.sh (slice diff + tamper fingerprint), pr-threads.sh (review-thread GraphQL plumbing), pick-reviewers.sh (reviewer ranking). e2e-slot.sh now resolves the caller's own tmux session. WORKFLOW.md codifies the principle: anything that can be automated away, should be — agents' tokens go to the work. Merge the two monitor-mode sections, teach every role to grep learnings when stuck, and graduate the review-threads learning into its script.
e2e-slot.sh: tear down before freeing the slot (releasing marker), drop the worktree's Android claims on release, abort on partial slot writes, reject explicit acquire owners. dispatch-role.sh: validate inputs, write a .meta sidecar. await-role.sh: role-specific sentinels, dead-target fast VOID, FINDINGS: 0 is void. slice-diff.sh: --binary content-sound fingerprint, path-scoped commit base, --check mode. pr-threads.sh: cursor pagination, comment counts, asserting resolve, one-step close. pick-reviewers.sh: empty result exits 0, per-PR vote dedupe, no error-body parsing. steer.sh: invocation-tied confirmation, dash-safe greps. New baseline.sh (verifier byte-identical restore proof), await-interactive.sh (monitor states), pr-gate.sh (head-pinned mechanical gate). Runbook claims Android serials at adb visibility; verifier defers teardown to slot release; seven more graduated learnings deleted.
Native build cache key now covers root patches/, pnpm-workspace.yaml, and
pnpm-lock.yaml. dev:start auto-selects a free port offset on collision and
persists it in the manifest (env > manifest > hash); the docker bridge probe
retries transient failures. Guard test fails when a mobile test file matches
no vitest project. GitHub stub serves paginated /pulls/{n}/files. New
e2e/maestro.sh serializes Maestro per device; login.sh holds a per-email OTP
mutex. Slot release kills the worktree's android emulator session (the
session name is the boot provenance). Runbook: fresh githubUserId for stub
seeding, maestro via the wrapper, batch many criteria per device session.
Five more graduated learnings deleted; .playwright-mcp gitignored.
Slot lifecycle: exclusive mkdir-based release transition, teardown failures keep the slot, emulator session killed before claims drop with console-port verification, acquire deadline (45 min) + state-dir validation + owner-last writes, explicit release refused for live sessions. Dispatch/await: wrapper- owned .exit file replaces the spoofable in-log marker, repro vs verify sentinel contracts (--mode repro), role-definition existence and run-id slug checks, machine-global 3s launch-spacing gate (also in launch-interactive, where --log is now mandatory). slice-diff --check judges rounds itself and the spec shows the exact commands. baseline.sh refuses re-snapshots and in-repo dirs, publishes atomically, reports all divergence. pr-gate: thread enumeration failures fail the gate, exact bot login + waiver detection, --assignee/--label/--wait. pr-threads close is retry-safe and repo-checked. pick-reviewers propagates API failures. maestro.sh keeps its lock trap. New init-section.sh replaces the hand-assembled section setup. Planner runs two parallel plan-reviewers per round and overlaps the repro gate with drafting; E2E follow-ups carry cumulative coverage; slots adopt existing stacks; runbook launches emulators on explicit probed ports. Five more graduated learnings deleted.
Ponytail pass over .kilo_workflow plus its dev/local and e2e support code: split the e2e slot state machine into take/free/status/start/stop scripts, prune and dedupe learnings, fail loudly on wrong usage everywhere, and act on three kimi-k3 review rounds (remote-cli required email, slot-status always-exit-0, pick-reviewers wrong-repo refusal, arg-forwarding wrappers, slot-aware recovery hint, unknown-subcommand refusal, repo-absolute tsx in log pipes). Move kilo-design to .agents/skills/kilo-design-cloud and vendor the grilling skill the workflow references.
# Conflicts: # .kilo_workflow/dispatch-role.sh
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Executive SummaryThe biggest risk is a broken caller contract in Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (incremental, 9eb007d..7024c49, 91 files changed)
Fix these issues in Kilo Cloud Previous Review Summaries (7 snapshots, latest commit 9eb007d)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 9eb007d)Status: No Issues Found | Recommendation: Merge Executive SummarySince the last reviewed commit, Files Reviewed (2 files, incremental)
Previous review (commit 5346644)Status: No Issues Found | Recommendation: Merge Executive SummaryThe only change since the last reviewed commit is a formatting-only reflow of a Files Reviewed (1 file, incremental)
Previous review (commit 7083eb4)Status: No Issues Found | Recommendation: Merge Executive SummaryIncremental changes (portability, CWD-independence for repo wrappers, defensive arg parsing, and matching test updates) are correct with no new issues. Files Reviewed (10 files, incremental)
Previous review (commit 55b8827)Status: No Issues Found | Recommendation: Merge Executive SummaryBoth changed files fix real portability bugs correctly: the stat probe order in the slot reaper is swapped so GNU Files Reviewed (2 files, incremental)
Previous review (commit f766903)Status: No Issues Found | Recommendation: Merge Executive SummaryThe latest commit fixes both previously-flagged issues (worktree resolution now uses the script's own on-disk location; the status header comment now documents the exit-1 usage-error case for forwarded args), and the remaining changed files are markdown table-formatting fixes with no functional impact. Files Reviewed (8 files, incremental)
Previous review (commit 13e8eb1)Status: 2 Issues Found | Recommendation: Address before merge Executive SummaryThe new slot-acquisition script resolves the owning worktree from the caller's CWD instead of the script's own location, a behavior change from the deleted Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files, incremental)
Fix these issues in Kilo Cloud Previous review (commit 85e3ac9)Status: 2 Issues Found | Recommendation: Address before merge Executive SummaryThe new slot-acquisition script resolves the owning worktree from the caller's CWD instead of the script's own location, a behavior change from the deleted Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (55+ files)
Reviewed by claude-sonnet-5 · Input: 40 · Output: 17.2K · Cached: 1.1M Review guidance: REVIEW.md from base branch |
Five kilo-auto/efficient rounds over 18 workflow hypotheticals found two repeat misses: agents hand-repairing a non-init-section worktree (5/5 divergent) and over-filing learnings to learnings/system/ (2/5). Fail loud with the remedy in init-section's preflight, document that a hand-created worktree cannot be repaired, and state that system/ is the rare exception.
Kilobot: invoked by absolute path from a sibling repo, acquire recorded the sibling's root in the slot and broke status UNACCOUNTED matching. Also correct the slot-status header: arguments are a usage error, only the no-arg report is exit 0.
GNU stat -f means --file-system and exits 0, defeating the BSD-first mtime fallback in reap; probe GNU syntax first. The lock-active test used pid 2 as its dead pid, which is kthreadd on Linux — use a pid above both platforms' max.
Contributor
Author
|
(bot) @kilocode-bot please review |
Contributor
Review: full diff (143 files,
|
Contributor
|
The review has been posted: #4874 (comment) Summary of findings for @iscekic: Fix before merge:
Minor: No code changes were made — this was a review-only pass. Let me know if you want me to spawn an agent to fix the two blocking issues. |
- probe GNU stat first in await-role, await-interactive, and baseline (the same GNU -f/--file-system trap the slot reap fix closed; baseline was writing mount garbage into fingerprints) - run e2e start/stop-resource from the script's own repo, not the caller's CWD — a sibling-worktree invocation could start/stop the wrong stack - remote-cli status exits 0 on its happy path and refuses extra arguments - login.sh probes sha256sum before shasum, like baseline and slice-diff - launch-gate clamps a corrupt or future-dated launch timestamp - restore the DESIGN.md pointer in the root AGENTS.md design row - cover the new contracts: start without a slot, sibling-CWD wrappers, status/free argument refusal
Contributor
Author
|
(bot) Dispositions on the full-diff review (fixed in 7083eb4 unless noted):
|
Kilobot edits its standing summary comment instead of posting anew, so the gate's createdAt filter never saw approvals after the first review. Fetch issue comments via REST with since=<head-date>, which filters by updated_at, and accept the [bot]-suffixed REST login.
Appium drivers (xcuitest, uiautomator2) install into a machine-global APPIUM_HOME (~/.cache/kilo-appium) on first use instead of living in the dependency tree, so only appium and the webdriverio client are deps.
- wdio/ client (per-device deterministic ports, XCUITest/UiAutomator2 caps), helpers (full-string-regex selectors, topmost-first index, polling waits), run-flow runner (exit code is the verdict), hierarchy dumper - appium.sh: per-device lock via process-lock, per-device appium server lifecycle, driver bootstrap into APPIUM_HOME - the six launch/login flows ported from YAML to JS, same helper names and wait budgets - login.sh/logout.sh rewired to appium.sh; interfaces unchanged Old wrapper and YAML flows remain until device parity is proven.
…apper test appium's postinstall no-ops when installed as a project dependency; appium-ios-tuntap and edgedriver are unused by the e2e setup. The JUnit exit-code hack test is obsolete: a node flow's exit code is the verdict.
Removed all maestro-rooted learnings outright (likely stale under the new driver). Tool-independent product/platform knowledge (a11y bounds, picker races, drag-cancel timing, mVCP blanks, deep links, simulator limits) was kept and reworded to the appium wrapper.
Replaced by appium.sh + flows/*.js earlier in this branch. BREAKING: maestro.sh and the YAML flow files are gone.
… tests - open-app.js answers Android 'isn\u0027t responding' ANR dialogs with Wait and gives Android a 420s launch budget under load (returns early when healthy); iOS keeps 30s - appium.sh bumps whole port blocks when the hashed block is occupied and records the winning port for reconnects - mobile-workflow tests assert the JS flows; appium-wrapper tests cover port determinism, slugs, and usage errors
Triage of every learning, one by one: - folded 9 into the runbook / WORKFLOW.md / extension AGENTS.md (git-token-service binding, deep-link route rule, CLI model id prefix, claim-killed-mid-boot recovery, oxfmt yaml caution, fixture patch discipline, dispatch --auto rationale, extension analytics key, firefox newSession flake) - deleted 12 obsolete or product-bug learnings (fixes landed: mvcp suppression, share-extension pods root, seed collision doc'd; tablet startup is a product bug) - updated 4 stale ones for the appium JS flows and current code 41 -> 28 committed learnings; system copies updated in the main checkout.
…te guards Ponytail deletions: pr-threads reply/resolve (close covers both), pr-gate --bot, await --timeout/--stall/--quiet knobs, upload --version mode, launch-gate impossible clamp, duplicated prose in WORKFLOW.md/README. Automation (learning graduation): - dispatch-role refuses e2e-verifier onto a dirty worktree (baseline restore would read concurrent edits as false failures) - e2e-free-slot refuses while the worktree owns a live stack, emulator session, remote CLI, stub, or device claim - e2e-stop-resource ios/android stops the Appium servers of claimed devices - e2e-start-resource stack refreshes session-ingest bindings, migrates, and fails fast on services or the docker bridge never coming up - same-worktree simulator reclaim boots a device left Shutdown by a killed-mid-boot claim (was: alreadyOwned + build 405) - dev:restart mobile clears this worktree's stale Metro file-map (white-screen-after-relayout learning graduated and deleted)
From two independent read-only reviews, applied: - free-slot refusal names per-kind remedies and matches the runner's per-char slug rule; cleanup docs stop the remote CLI and stub first - appium.sh adopts only its own recorded server (a hash-colliding foreign server is skipped by the bump loop) and rejects two flows per test invocation - preflight session-ingest probe prints its fix; stop-resource surfaces an Appium server that would not stop - planner learnings go to $SCRATCH (dirty tree now blocks repro dispatch) - slice-diff snapshot is per-round; E2E slot teardown waits for serialized cases; VOID/VIOLATION escalation unified; pr-threads close-only wording; starter-turned-planner needs tmux; mock exception keeps justification; yml/yaml format rule covers every file
driver.findElements returns protocol references, not WDIO Element objects, so element commands need the extracted id. First XCUITest session on a device builds WebDriverAgent and blows a 15s connection timeout; give session creation 5 minutes with retries.
…sign-out XCUITest rejects W3C key-action batches, so text entry goes through Element Send Keys on the active element and field clearing through Element Clear. The logout flow's terminal assert raced the sign-out API call; it now waits for the login page.
- UiSelector consumes backslash escapes silently; regex escapes become character classes on the Android selector path - the sign-out alert title carries a literal '?', matched escaped now; a leftover alert from an interrupted run is confirmed and handled - settle-app's first wait gets the long Android budget (bundle refetch after a preflight reconnect), early-returning when healthy - ANDROID_HOME/SDK_ROOT exported by the wrapper for the uiautomator2 driver, mirroring the repo's resolution Verified live: login/logout/re-login on iOS 26.5 sim and Android API 35 emulator, hierarchy on iOS, open-app cold launch on Android.
…d sessions Wall-time optimizer findings applied (no quality/safety trade): - e2e-start-resource bundle: stack ∥ iOS claim+build ∥ Android start+claim+build, one log per chain — slowest chain sets wall time - emulator-start --wait polls liveness/visibility/boot_completed in the wrapper (8-min envelope); manual boot polling prose graduated away - appium.sh test runs N flows on one driver session (5-20s saved each) - selector probes skip the topmost-first sort unless tapping - pr-gate Kilobot wait polls 30s (was 120); await-role 5s (was 15); await-interactive 10s (was 30); pr-threads close fetches thread_info once; preflight takes the wrappers' platform hint Verified: multi-flow runs pass on iOS and Android.
Found by rehearsing the full slot lifecycle end to end: - stack arm retries a failed reuse once, then stops and starts fresh (the runner's own remedy for down never-startable services) - android arm reuses a live own emulator (record + boot check) and starts with --wait otherwise; arg order fixed - stop-resource's claim grep matched nothing (mangled quoting), so Appium servers outlived their bundles - preflight's platform hint skipped the probe but not the claim branch
| echo "recorded emulator not booted; starting fresh" >&2 | ||
| pnpm dev:mobile:android emulator-stop || true | ||
| fi | ||
| exec pnpm dev:mobile:android emulator-start "$@" --wait |
Contributor
There was a problem hiding this comment.
WARNING: --wait is unconditionally appended, but the usage string one line above documents it as a caller-suppliable flag.
The usage string on line 10 (android <avd> [--gpu ...] [--wait]) advertises --wait as an option the caller may pass. But exec pnpm dev:mobile:android emulator-start "$@" --wait always appends --wait itself. If a caller follows the documented usage and passes --wait explicitly, "$@" already contains it, so the final arg list has --wait twice. parseEmulatorStartArgs in dev/local/mobile-android.ts computes expectedArgs from a single args.includes('--wait') check and then requires args.length === expectedArgs, so the duplicate flag makes the length check fail and the dispatch throws its Usage: error instead of starting the emulator.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| last=$(cat "$GATE/last" 2>/dev/null || echo 0) | ||
| case "$last" in ''|*[!0-9]*) last=0 ;; esac | ||
| remaining=$(( 3 - (now - last) )) | ||
| [ "$remaining" -le 0 ] || sleep "$remaining" |
Contributor
There was a problem hiding this comment.
WARNING: Removed upper-bound clamp on remaining can allow an unbounded sleep.
[ "$remaining" -le 3 ] || remaining=3 was deleted. remaining=$(( 3 - (now - last) )) is normally <= 3 when now >= last, but if $GATE/last ever holds a timestamp in the future relative to now (clock skew/NTP resync, or a corrupted file that still parses as digits), now - last goes negative and remaining can become arbitrarily large, so sleep "$remaining" on this line can block far longer than the intended 3-second gate — hanging every subsequent kilo launch that goes through --locked until that sleep completes.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # | ||
| # --assignee gate-fail unless this handle is among the assignees | ||
| # --label gate-fail unless this label is present (monitors: human-ready) | ||
| # substring 'bot' matching would let any bot vouch for the head |
Contributor
There was a problem hiding this comment.
SUGGESTION: Orphaned comment line left over after removing --bot.
The --bot option and its description were removed on the lines above, but this line (substring 'bot' matching would let any bot vouch for the head) is the second half of that removed description and is now left dangling directly under the --label bullet, making it read as a caveat about --label rather than about bot-login matching. Consider deleting this orphaned line.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| | "\(.user.login) @ \(.updated_at): \(.body | gsub("\\s+"; " ") | .[0:200])"] | .[]' <<<"$COMMENTS") | ||
| [ -n "$BOTCOMMENTS" ] && break | ||
| [ "$(date +%s)" -ge "$DEADLINE" ] && break | ||
| sleep 30 |
Contributor
There was a problem hiding this comment.
SUGGESTION: Poll interval no longer matches the header doc.
The header comment (--wait poll every 120s up to this budget ...) still says every 120s, but this line now sleeps 30s between polls — 4x more frequent than documented. Not harmful, but the doc comment should be updated to avoid misleading callers about how the --wait budget is consumed.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
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.
Idiot-proofing (ponytail) pass over
.kilo_workflow/and its dev/local + mobile-e2e support code. Zero product changes — this is the agent workflow only.Three kimi-k3 review rounds on the full diff; round 3 ended No findings. 251/251 dev-local tests, changed-file typecheck/format/lint all green.
What changed
e2e-slot.shbecame five dumb wrappers (e2e-take-slot/e2e-free-slot/e2e-slot-status/e2e-start-resource/e2e-stop-resource) plus one shared state library. Slots never kill foreign resources;statusreports UNACCOUNTED and always exits 0.start.login.shnever creates (now per-platform);execwith no prepared env says to runstart <email>first. Recovery hint points at the slot +e2e-start-resource.shprotocol instead of a bare slotlesspnpm dev:start.dev:capture --followre-targets the tmux pipe instead of silently keeping the old one, and log pipes use repo-absolutenode_modules/.bin/tsx(tmux servers started outside a direnv shell have notsxon PATH)..kilo/skills/to.agents/skills/kilo-design-cloud(synced from Kilo-Org/kilo-design); thegrillingskill the workflow references is vendored into.kilo/skills/.Review rounds