Repository navigation
Add mobile Maestro E2E testing foundation - #11
Merged
Merged
Conversation
Maestro + TS-orchestrator E2E foundation for the iOS Simulator that automates the proven mobile loop (launch, pair, Clerk Connect, agent). Mirrors the Electron E2E foundation: harness/flows split, real services, local-only/no-CI, fail-loud gates, 4 tagged starter flows, authoring skill. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzssDsoM4UtRdJcS1MFsE8
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzssDsoM4UtRdJcS1MFsE8
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzssDsoM4UtRdJcS1MFsE8
Phase 1 of the mobile E2E testing foundation: mobile-e2e/ tree with tsconfig, tag/timeout config, README (prereqs, commands, env, Studio), root scripts (e2e:mobile, :build, :studio), and gitignore entries for local-only artifacts and secrets. Spec: docs/specs/2026-06-24-mobile-e2e-testing-foundation-design.md (Phase 1) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzssDsoM4UtRdJcS1MFsE8
Phase 2: generic harness for the mobile E2E suite — isolated run context (unique KATACODE_HOME + server port), artifacts/manifest, process helpers, server stack (serve + token parse + project add), simulator control (select/boot + dev-client install gate), Maestro runner (arg builder + tag mapping), seeded workspace, and fail-loud prereq gates. TDD: pure logic unit-tested — parseServeOutput, buildMaestroArgs, selectSimulator, requiredCredentialGroupsForTags, toMaestroTag (20 tests). Spec: docs/specs/2026-06-24-mobile-e2e-testing-foundation-design.md (Phase 2) AC: 2, 3, 4, 5, 11, 12 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzssDsoM4UtRdJcS1MFsE8
Phase 3: e2e:mobile CLI (run/list/studio/help) wiring prereq gates, simulator + dev-client checks, server stack, tag-filtered Maestro runs, optional simctl screen recording, and per-run manifest. Adds the @smoke launch flow and @pairing bearer flow with host/token injection via flows/pairing.ts. Ready-state assertion + locators flagged for Studio verification (test contracts noted inline). TDD: parseCliArgs, parseFlowTags, runNeedsServer, debug-output arg (30 tests). Verified: --list lists both flows by tag; --help exits 0; @agent gate exits 1 naming the missing prerequisite. Spec: docs/specs/2026-06-24-mobile-e2e-testing-foundation-design.md (Phase 3) AC: 6, 7, 10 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzssDsoM4UtRdJcS1MFsE8
Phase 4: @auth Clerk Connect flow with a fail-loud Clerk+Google gate (flows/auth.ts) and @agent deterministic-reply flow (flows/agent.ts: run-id-scoped expected token, prompt builder, whitespace/wrapper normalization, model injection). run.ts injects auth/agent Maestro env by tag. Both flows' green runtime pass is deferred to a maintainer; @auth documents the native presentAuth modal as the top automation risk. TDD: expectedAgentText, buildAgentPrompt, normalizeReply, matchesExpected. 35 tests pass; --list shows all four tagged flows. Spec: docs/specs/2026-06-24-mobile-e2e-testing-foundation-design.md (Phase 4) AC: 8, 9, 13 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzssDsoM4UtRdJcS1MFsE8
Phase 5: mobile-e2e-test-author skill (compose from harness/flows/maestro, no mocks, tags, ignored secrets, Studio for locators, smallest command), README cross-link to the skill, guides index Testing entries, and specs roadmap row (Implemented, Verify pending). OKF check passes. Spec: docs/specs/2026-06-24-mobile-e2e-testing-foundation-design.md (Phase 5) AC: 14 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzssDsoM4UtRdJcS1MFsE8
Set status Approved -> Implemented and add the Build completion report (phases, files, verification table, AC status, known follow-ups). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzssDsoM4UtRdJcS1MFsE8
…ke passing Three harness bugs found during live UAT: 1. Server bin path: resolveMobileE2eRoot() returned mobile-e2e/ but was used as repoRoot for requirePrereqs, so the server bin check looked for mobile-e2e/apps/server/dist/bin.mjs (wrong path). Added resolveRepoRoot() in artifacts.ts and used it in run.ts. 2. Maestro flow discovery: maestro test with a directory does not recurse subdirectories. Changed MaestroRunOptions.flowPath to flowPaths[] and added resolveFlowPaths() in run.ts to pass individual flow file paths filtered by tag. 3. Smoke flow locator: clearState: true launched the Expo Dev Launcher (server picker) instead of the app. Changed to clearState: false and asserted on "Kata Code" (app header, always visible) with extendedWaitUntil (90s timeout for Metro bundle loading). Command syntax corrections across all docs/specs/README/skill: - Removed -- -- arg-passing from vp run e2e:mobile commands (vp run passes args directly, the -- separator caused task-not-found errors) - Replaced katacode serve prose with node apps/server/dist/bin.mjs - Annotated e2e commands as repo-root; mapping table uses --filter form Guide cross-links: - Added Automated E2E section to mobile-local-dev-ios-simulator.md - Added Related docs to mobile E2E design spec (reciprocal links) - Updated OKF logs Verified: 35 unit tests pass, vp check clean, smoke test 1/1 passed in 4s on iPhone 17 Pro (JUnit: 0 failures). Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
assertVisible rejects the timeout property; Maestro errored with 'Unknown Property: timeout'. Use extendedWaitUntil/visible for the bounded wait, matching the smoke and pairing flows.
…tusDot Deliberate E2E test contract: a stable accessibility id keyed to connection state (connection-status-ready, -connecting, -disconnected, -idle, -reconnecting) lets Maestro assert pairing readiness deterministically instead of matching the localized status text. Called for by the mobile E2E design spec.
The mobile model picker shows display labels (Codex / GPT-5.4-Mini), not the raw provider key + slug the harness injects. Add providerMenuLabel and modelMenuLabel (mirroring the server's CodexProvider toDisplayName) so the @agent flow can tap the provider submenu then the model row; inject both as KC_PROVIDER_LABEL / KC_MODEL_LABEL. Exclude maestro/shared/ from flow discovery so reusable runFlow subflows are not run as standalone tagged flows.
Replace the TODO(studio) stubs with verified navigation authored against the live app: - shared/open-add-environment.yaml: reusable home -> Settings -> Environments -> + -> Add Environment subflow (composed via runFlow). - @pairing: fill host/token from KC_HOST/KC_TOKEN, submit, and assert the saved environment reaches ready via the connection-status-ready contract. Real loopback bearer handshake. Passes in ~34s. - @agent: pair, dismiss the Settings sheet, open the new-task composer, select the seeded project, type the prompt, select the configured model through the native picker submenus, start the task, and assert the real provider returns the exact E2E_AGENT_OK_<run-id> token. Passes in ~1m8s against OpenAI. Verifies acceptance criteria 7 and 9 of the mobile E2E design spec.
Consolidate scattered Studio workflow guidance into one OKF guide and cross-link it from the test catalog, design spec, README, and skill.
Capture the on-device Verify outcome (iPhone 17 Pro): @smoke/@pairing/@agent green via Maestro Studio; @auth (native presentAuth modal) and the AC-4 distinct-ports clause open. Adds a Verify outcome section to the design spec, updates the specs roadmap status, and logs in docs/ + specs/ logs. Refreshes the mobile-e2e-test-author skill with durable learnings: clearState:false, accessibility-id test contracts (connection-status), the shared/ subflow pattern, and model-picker label derivation in flows/agent.ts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HzssDsoM4UtRdJcS1MFsE8
…eptions Main stopped tracking .agents/skills/ (per-developer dir, untracked). The rebase dropped three project-authored skills as a side effect. Re-include them as tracked exceptions so the kata-code-e2e-testing, mobile-e2e-test-author, and upstream-assess skills ship with the repo; vendor skills stay ignored.
…red display labels
Blockers:
- B1: mobile-e2e is now a workspace package (@kata-sh/code-mobile-e2e) with
@effect/vitest + vitest devDeps, a tsc typecheck script, and a tsconfig that
disables the Effect language-service plugin (plain Node CLI). The 8 *.test.ts
files now run via vp run typecheck/test; add e2e:mobile:test root script.
- B2: runNeedsServer no longer treats @auth as a server-pairing flow. @auth is a
native Clerk modal and never reads KC_HOST/KC_TOKEN; starting a server for it
was dead weight + a token-expiry failure surface.
- B3/B4: extract resolveFlowPaths, buildMaestroEnv, resolveRunTimeoutMs,
resolveSimulatorAction, SettleGuard + logProcessChunk as pure/testable units.
Add intent-encoding tests (orchestration env decisions, sim boot/skip/throw,
multi-chunk serve parse). Test count: 38 -> 63.
High:
- H1: lift modelMenuLabel to packages/shared formatModelDisplayName; both the
server CodexProvider and the harness import it. Removes the 'keep in sync'
duplicate. PROVIDER_LABELS now only maps provider ids (openai/anthropic),
dropping the unreachable driver keys (codex/claudeAgent).
- H2: FLOW_DESCRIPTORS table is the single source of truth for needsServer /
credentials / pairEnv / timeoutMs per tag. Collapses runNeedsServer,
requiredCredentialGroupsForTags, buildMaestroEnv's wants, resolveFlowPaths'
filter, and the dead MobileE2ETag export into lookups.
- H3: maestroFlowMs / agentFlowMs now enforced via runMaestro timeoutMs (resolved
from the descriptor; slowest wins) with gracefulKill on expiry. Previously
declared but never applied.
- H4: cleanupCallbacks live on MobileE2ERunContext; delete the module-level
cleanupCallbacksByRunId Map singleton.
- H5: SettleGuard + logProcessChunk shared between runCommandToCompletion and
waitForServePairing (de-dupe the spawn/buffer/timeout/settle skeleton).
Medium:
- requirePrereqs returns ResolvedCredentials; buildAuthMaestroEnv consumes the
validated email instead of re-reading process.env.
- loadEnv is an explicit loadEnv() call in main(), not a side-effect import.
- gracefulKill({ primarySignal }) shared by stopScreenRecording (SIGINT) and
terminateChildProcess (SIGTERM).
- logProcessChunk awaits + surfaces write failures rather than fire-and-forget.
- Delete dead MobileE2ETimeouts export; narrow ports.ts re-export.
- @auth YAML: gearshape locator (matches open-add-environment), stub marker.
- README: Node 24 prerequisite + failure-diagnosis section pending.
Verified: vp check (0 errors), vp run typecheck (16/16), mobile-e2e 63 tests,
shared 214 tests. One pre-existing flaky ProviderRegistry test fails on clean
HEAD too (provider reprobe timing, unrelated to this change).
…rtifacts Add Node.js 24+ as the first prerequisite (the CLI runs .ts source directly via node's native type-stripping; an older Node fails on the first import with no version hint) and the suite's own test/typecheck commands. Add a Diagnosing failures section listing the per-run manifest, serve/project-add logs, recording, JUnit report, and Maestro debug output under test-results/<runId>/ and artifacts/<runId>/, matching the paths the harness writes and the error messages reference.
📝 WalkthroughWalkthroughThe PR adds two upstream-assessment CLI scripts, a new mobile E2E package with harness and flow modules, shared model display formatting, and workspace/package wiring for the new commands. ChangesUpstream assessment scripts
Shared model labels and mobile E2E harness
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:trusted
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68195ab2fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Release Smoke regenerates the lockfile from scratch, so loose specs (^0.35.4 / >=0.35.5) resolved to the newly published 0.35.9 -> 0.35.10 drift and left the 0.35.9 patch unused (ERR_PNPM_UNUSED_PATCH). Pin the version via overrides, matching the existing pattern for the other patched deps (@ff-labs/fff-node, @expo/metro-config).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (1)
mobile-e2e/src/flows/agent.test.ts (1)
61-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one contract test for
buildAgentMaestroEnv.This suite verifies the helper pieces in isolation, but not the assembled Maestro payload that
mobile-e2e/maestro/agent/deterministic-chat.yamlactually consumes. A focused test here would lock the exported keys and the fail-loud path before this breaks at runtime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mobile-e2e/src/flows/agent.test.ts` around lines 61 - 68, Add a focused contract test for buildAgentMaestroEnv in the agent test suite to verify the assembled Maestro environment payload, not just the helper pieces. Assert that the exported keys expected by deterministic-chat.yaml are present and correctly mapped, and that the fail-loud path is preserved when required data is missing. Use buildAgentMaestroEnv and modelMenuLabel as the entry points to locate the relevant test coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/skills/upstream-assess/scripts/intersection.ts:
- Around line 118-128: The invalid SHA/range case is escaping the script’s
normal error handling because upstreamFiles(arg) runs before the existing
try/catch. Move the upstreamFiles(arg) resolution into the same guarded error
path used for forkModifiedFiles(), and on any thrown error print the clean
[intersection] diagnostic with the error message before exiting. Keep the fix
localized around upstreamFiles and forkModifiedFiles so both resolution steps
share the same failure handling.
In @.agents/skills/upstream-assess/scripts/scan-upstream.ts:
- Line 300: The call to buildReport is passing its arguments in the wrong order,
causing the report header to swap the tip ref and tip SHA. Update the
buildReport invocation in scan-upstream.ts so it matches the expected parameter
order of (base, tipRef, tipSha), using the existing buildReport symbol and the
baseSha, tip, and tipSha values in the correct positions.
- Around line 77-83: The argument parsing in scan-upstream.ts currently lets
`--base` and `--tip` consume the next token even when it is another flag or
missing, which leads to confusing downstream failures. Update the parsing logic
in the argv loop (around `scanArgs`) to validate that `argv[++i]` exists and is
not another option before assigning `out.base` or `out.tip`, and fail fast with
a clear parse error when the value is absent. Keep the existing handling for
`--base=` and `--tip=` forms unchanged, and use the same parser function to
centralize the validation.
In `@apps/mobile/src/features/connection/ConnectionStatusDot.tsx`:
- Around line 83-88: The ConnectionStatusDot visual indicator should not be
exposed to the accessibility tree; remove the accessibility-related props from
the component while keeping the stable E2E selector via testID. Update
ConnectionStatusDot so Maestro can still target the state-specific testID, but
avoid using accessible or accessibilityLabel on this non-interactive dot to
prevent it from becoming focusable to screen readers.
In `@mobile-e2e/maestro/auth/clerk-connect.yaml`:
- Around line 15-31: The current `clerk-connect.yaml` flow is not validating a
real signed-in Connect session because `assertNotVisible: "Sign in"` only proves
the native Clerk modal changed state. Update the `@auth` flow to assert an
actual authenticated outcome using a stable post-login UI element or session
indicator reached after `presentAuth`/`Sign in`, and keep the modal steps only
if you can drive them reliably; otherwise mark this flow non-runnable and gate
it behind a supported automation path.
In `@mobile-e2e/maestro/pairing/bearer-pair.yaml`:
- Around line 15-38: The pairing flow can succeed against a stale ready
environment because launchApp keeps prior state and extendedWaitUntil only
checks for any visible connection-status-ready. Update bearer-pair.yaml so this
test validates the newly added environment specifically, using the
open-add-environment.yaml flow and the saved row’s unique host/token context
instead of a generic ready badge. Either clear prior state before pairing or
scope the final wait/assertion to the just-created environment so stale ready
rows cannot satisfy the check.
In `@mobile-e2e/src/cli/args.ts`:
- Around line 36-41: The argument parsing in `parseArgs` is consuming the token
after `--include-tags` even when it is another flag, which causes option-like
values such as `--list` to be treated as tags. Update the `--include-tags`
branch in `mobile-e2e/src/cli/args.ts` to check the next token before calling
`collectTags`, and if it starts with `-`, fail fast with an error instead of
advancing `i` or swallowing the option. Keep the fix localized to the
`parseArgs` logic and preserve the existing handling for valid tag values.
In `@mobile-e2e/src/cli/flows.ts`:
- Around line 16-39: The parseFlowTags() function currently stops reading tags
as soon as it hits any non-item line after entering the tags block, which causes
blank lines or comment lines to truncate discovery. Update parseFlowTags() in
flows.ts so it skips empty lines and lines starting with # while in the tags
block, and only breaks when the block truly ends or a non-tag content line is
reached; keep the existing tag normalization behavior for parsed items.
In `@mobile-e2e/src/cli/run.ts`:
- Around line 191-214: The CLI bootstrap in main() is being executed as soon as
mobile-e2e/src/cli/run.ts is imported, which makes the module unsafe for tests
and other consumers. Keep the reusable functions like main, parseCliArgs,
runList, runStudio, and runFlows in this module, but move the unconditional
main() invocation into a separate CLI-only entrypoint or wrap it behind an
import-safe guard so run.test.ts can import the helpers without triggering
loadEnv() or argv parsing.
- Around line 181-188: The finally block in run.ts currently runs
stopScreenRecording(), writeRunManifest(), and cleanupRunState() serially, so
any throw from the earlier cleanup steps can prevent later teardown from running
and can mask the original Maestro failure. Update the teardown flow around the
run() cleanup logic to capture each cleanup error separately, always attempt
cleanupRunState() regardless of failures in stopScreenRecording or
writeRunManifest, and then rethrow the original failure while preserving any
cleanup errors for reporting. Use the existing symbols stopScreenRecording,
writeRunManifest, buildManifest, and cleanupRunState to locate and restructure
this logic.
In `@mobile-e2e/src/config/tags.ts`:
- Around line 109-114: The selectedDescriptors helper currently filters unknown
tags into an empty descriptor list, which can later lead to invalid timeout
calculation and skipped setup. Update selectedDescriptors to validate the
incoming selection against allDescriptors() and throw immediately when any
requested tag does not match a descriptor.tag, so invalid --include-tags values
fail fast. Use the selectedDescriptors function and allDescriptors/tag matching
logic to locate the fix.
In `@mobile-e2e/src/harness/isolatedRun.ts`:
- Around line 80-85: cleanupRunState currently stops at the first rejected
cleanup callback, so later teardown steps in the reversed
context.cleanupCallbacks list never run. Update cleanupRunState in
isolatedRun.ts to invoke every callback to completion even if one fails, by
catching per-callback errors inside the loop, continuing through all registered
callbacks, and then deciding how to surface failures after the loop; keep the
behavior localized to cleanupRunState and the cleanupCallbacks handling.
In `@mobile-e2e/src/harness/maestroRunner.ts`:
- Around line 65-66: The `runMaestro()` logging currently emits the fully
expanded argv, which leaks injected `-e` values from `options.env` into harness
logs. Update the `maestroRunner.ts` flow so `buildMaestroArgs()` still builds
the real command, but the `logHarnessPhase()` message uses a redacted/sanitized
version of the args where sensitive `-e` key/value pairs (like pairing tokens
and auth emails) are masked or omitted before logging.
- Around line 67-83: The timeout handling in maestroRunner’s spawn promise can
resolve with the child’s real exit code instead of the timeout sentinel. In the
timeout branch, update the logic around `gracefulKill` and the
`child.once("close")` listener so a timeout always settles the promise with
`124`, even if `close` fires during shutdown. Use the `spawn`, `gracefulKill`,
and `options.timeoutMs` flow in `maestroRunner` to ensure the timeout path wins
over the normal exit path.
In `@mobile-e2e/src/harness/prereqs.ts`:
- Around line 77-79: The prerequisite check in requirePrereqs() is validating
the server binary even for runs that do not need it. Gate the
resolveServerBinPath(input.repoRoot) call behind runNeedsServer(...) so only
server-dependent tags require katacode serve to be built; keep assertMacOsHost()
and assertMaestroInstalled() unconditional, and use the existing
runNeedsServer() helper from run.ts to match the runtime behavior.
In `@mobile-e2e/src/harness/processSpawn.ts`:
- Around line 119-133: The graceful shutdown logic in `gracefulKill` only checks
`child.exitCode`, so processes that already exited via signal can be missed and
leave the promise hanging. Update the `child.exitCode` guards in
`mobile-e2e/src/harness/processSpawn.ts` to also treat a populated
`child.signalCode` as already exited, and keep the same handling around the
`child.once("exit")` listener and the `child.kill(primarySignal)`/SIGKILL
fallback so the function returns immediately for signal-terminated children.
In `@mobile-e2e/src/harness/seededWorkspace.ts`:
- Around line 21-24: The seeded workspace writer currently assumes parent
directories already exist, which breaks for nested relative paths like
src/index.ts or .vscode/settings.json. Update seededWorkspace’s file-writing
loop in the helper that creates the temp root so it ensures each file’s parent
directory is created before calling writeFile, preserving support for arbitrary
relative paths in the files map.
In `@mobile-e2e/src/harness/simulator.ts`:
- Around line 102-116: The simulator boot path in simulator.ts ignores the
outcome of runCommandToCompletion() for the simctl bootstatus call, so a failed
boot still sets context.simulatorUdid and continues. Update the boot logic in
the action.boot branch to inspect the command result and throw or fail fast when
simctl-boot returns a non-zero status before assigning context.simulatorUdid;
use the existing runCommandToCompletion and boot handling around
action.udid/device!.name as the insertion point.
- Around line 147-154: The startScreenRecording helper currently returns the
spawned process immediately, so an xcrun spawn failure can emit an unhandled
error event and crash the runner. Make startScreenRecording in simulator.ts
async and wrap the spawn of xcrun/simctl in a Promise that listens for the child
process "error" event and rejects on failure before returning the
ScreenRecording object. Then update the run.ts caller to await
startScreenRecording(...) so the failure is caught by the existing try/finally
flow.
---
Nitpick comments:
In `@mobile-e2e/src/flows/agent.test.ts`:
- Around line 61-68: Add a focused contract test for buildAgentMaestroEnv in the
agent test suite to verify the assembled Maestro environment payload, not just
the helper pieces. Assert that the exported keys expected by
deterministic-chat.yaml are present and correctly mapped, and that the fail-loud
path is preserved when required data is missing. Use buildAgentMaestroEnv and
modelMenuLabel as the entry points to locate the relevant test coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2494f8b1-2dee-4bd9-8efa-9c20b7f6701a
⛔ Files ignored due to path filters (14)
.agents/skills/kata-code-e2e-testing/SKILL.mdis excluded by!**/*.md.agents/skills/mobile-e2e-test-author/SKILL.mdis excluded by!**/*.md.agents/skills/upstream-assess/SKILL.mdis excluded by!**/*.mddocs/guides/e2e-mobile-authoring-maestro-studio.mdis excluded by!**/*.mddocs/guides/e2e-test-catalog.mdis excluded by!**/*.mddocs/guides/index.mdis excluded by!**/*.mddocs/guides/log.mdis excluded by!**/*.mddocs/guides/mobile-local-dev-ios-simulator.mdis excluded by!**/*.mddocs/log.mdis excluded by!**/*.mddocs/specs/2026-06-24-mobile-e2e-testing-foundation-design.mdis excluded by!**/*.mddocs/specs/index.mdis excluded by!**/*.mddocs/specs/log.mdis excluded by!**/*.mdmobile-e2e/README.mdis excluded by!**/*.mdpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (44)
.agents/skills/upstream-assess/scripts/intersection.ts.agents/skills/upstream-assess/scripts/scan-upstream.tsapps/mobile/src/features/connection/ConnectionStatusDot.tsxapps/server/src/provider/Layers/CodexProvider.tsmobile-e2e/maestro/agent/deterministic-chat.yamlmobile-e2e/maestro/auth/clerk-connect.yamlmobile-e2e/maestro/pairing/bearer-pair.yamlmobile-e2e/maestro/shared/open-add-environment.yamlmobile-e2e/maestro/smoke/launch.yamlmobile-e2e/package.jsonmobile-e2e/src/cli/args.test.tsmobile-e2e/src/cli/args.tsmobile-e2e/src/cli/flows.test.tsmobile-e2e/src/cli/flows.tsmobile-e2e/src/cli/run.test.tsmobile-e2e/src/cli/run.tsmobile-e2e/src/config/loadEnv.tsmobile-e2e/src/config/tags.test.tsmobile-e2e/src/config/tags.tsmobile-e2e/src/config/timeouts.tsmobile-e2e/src/flows/agent.test.tsmobile-e2e/src/flows/agent.tsmobile-e2e/src/flows/auth.tsmobile-e2e/src/flows/pairing.tsmobile-e2e/src/harness/artifacts.tsmobile-e2e/src/harness/env.tsmobile-e2e/src/harness/isolatedRun.tsmobile-e2e/src/harness/log.tsmobile-e2e/src/harness/maestroRunner.test.tsmobile-e2e/src/harness/maestroRunner.tsmobile-e2e/src/harness/ports.tsmobile-e2e/src/harness/prereqs.test.tsmobile-e2e/src/harness/prereqs.tsmobile-e2e/src/harness/processSpawn.tsmobile-e2e/src/harness/seededWorkspace.tsmobile-e2e/src/harness/serverStack.test.tsmobile-e2e/src/harness/serverStack.tsmobile-e2e/src/harness/simulator.test.tsmobile-e2e/src/harness/simulator.tsmobile-e2e/tsconfig.jsonpackage.jsonpackages/shared/src/model.test.tspackages/shared/src/model.tspnpm-workspace.yaml
Address CodeRabbit, Codex, and Greptile review findings on the mobile E2E foundation PR. Functional correctness: - args.ts: reject flag-like values after --include-tags (fail fast) - flows.ts: parseFlowTags skips blank/comment lines instead of truncating - run.ts: resolveFlowPaths includes untagged flows on "run all" - tags.ts: selectedDescriptors throws on unknown tags (prevents -Infinity timeout from Math.max(...[])) - tags.ts: @agent descriptor now declares needsPairingVars so deterministic-chat.yaml receives KC_HOST/KC_TOKEN (it composes bearer-pair.yaml first) - prereqs.ts: gate server-bin check behind runNeedsServer so @smoke/@auth runs don't require a built server - simulator.ts: fail loud when simctl bootstatus returns non-zero Stability/availability: - run.ts: run all finally cleanup steps even if one throws (AggregateError) - run.ts: guard top-level main() so importing the module is side-effect-free (fixes root `vp run test` failing on non-macOS/unequipped hosts) - isolatedRun.ts: cleanupRunState runs every callback to completion - maestroRunner.ts: timeout sentinel (124) always wins over a close during graceful shutdown - processSpawn.ts: gracefulKill treats signalCode as already-exited - simulator.ts: startScreenRecording is async and rejects on spawn error Security: - maestroRunner.ts: redact -e KEY=VALUE pairs in the harness log Maintainability: - seededWorkspace.ts: create parent directories for nested fixture paths - ConnectionStatusDot.tsx: drop accessible/accessibilityLabel (testID alone is the E2E selector); avoids an a11y-tree regression on a visual-only dot Upstream-assess scripts: - intersection.ts: route upstreamFiles errors through the clean diagnostic - scan-upstream.ts: validate --base/--tip values; fix swapped buildReport args (tip ref vs tip SHA) Tests cover each behavioral change.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mobile-e2e/src/flows/agent.test.ts`:
- Around line 93-96: The test for missing provider config in agent.test.ts is
not isolated because it deletes both KATACODE_E2E_AGENT_PROVIDER and
KATACODE_E2E_AGENT_MODEL, so it can pass for the wrong reason. Update the
failing-case setup around buildAgentMaestroEnv to keep KATACODE_E2E_AGENT_MODEL
defined and unset only KATACODE_E2E_AGENT_PROVIDER, so the assertion truly
verifies the provider-missing path described by the test name.
- Around line 74-80: The test setup in agent.test.ts is mutating shared
process.env state and then deleting the variables in afterEach, which can leak
into later tests in the same worker. Update the beforeEach/afterEach pair around
KATACODE_E2E_AGENT_PROVIDER and KATACODE_E2E_AGENT_MODEL to snapshot their prior
values before overwriting them, then restore those original values afterward
instead of always deleting them. Use the existing beforeEach and afterEach hooks
in agent.test.ts to keep the env state isolated per test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 617d1325-f182-4f76-9df0-e02cfac44dab
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
.agents/skills/upstream-assess/scripts/intersection.ts.agents/skills/upstream-assess/scripts/scan-upstream.tsapps/mobile/src/features/connection/ConnectionStatusDot.tsxmobile-e2e/src/cli/args.test.tsmobile-e2e/src/cli/args.tsmobile-e2e/src/cli/flows.test.tsmobile-e2e/src/cli/flows.tsmobile-e2e/src/cli/run.test.tsmobile-e2e/src/cli/run.tsmobile-e2e/src/config/tags.test.tsmobile-e2e/src/config/tags.tsmobile-e2e/src/flows/agent.test.tsmobile-e2e/src/harness/isolatedRun.tsmobile-e2e/src/harness/maestroRunner.tsmobile-e2e/src/harness/prereqs.tsmobile-e2e/src/harness/processSpawn.tsmobile-e2e/src/harness/seededWorkspace.tsmobile-e2e/src/harness/simulator.tspnpm-workspace.yaml
✅ Files skipped from review due to trivial changes (1)
- apps/mobile/src/features/connection/ConnectionStatusDot.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
- mobile-e2e/src/harness/seededWorkspace.ts
- mobile-e2e/src/config/tags.test.ts
- mobile-e2e/src/cli/flows.test.ts
- mobile-e2e/src/cli/args.ts
- .agents/skills/upstream-assess/scripts/intersection.ts
- mobile-e2e/src/config/tags.ts
- mobile-e2e/src/harness/prereqs.ts
- mobile-e2e/src/cli/flows.ts
- mobile-e2e/src/cli/run.test.ts
- .agents/skills/upstream-assess/scripts/scan-upstream.ts
Comment on lines
+74
to
+80
| beforeEach(() => { | ||
| process.env.KATACODE_E2E_AGENT_PROVIDER = "openai"; | ||
| process.env.KATACODE_E2E_AGENT_MODEL = "gpt-5.4-mini"; | ||
| }); | ||
| afterEach(() => { | ||
| delete process.env.KATACODE_E2E_AGENT_PROVIDER; | ||
| delete process.env.KATACODE_E2E_AGENT_MODEL; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Restore the original env values after each test.
Lines 74-80 overwrite shared process.env state and then always delete it. If these vars were already present, later tests in the same worker will see them missing. Snapshot the previous values in beforeEach and restore them in afterEach instead of unconditionally deleting them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mobile-e2e/src/flows/agent.test.ts` around lines 74 - 80, The test setup in
agent.test.ts is mutating shared process.env state and then deleting the
variables in afterEach, which can leak into later tests in the same worker.
Update the beforeEach/afterEach pair around KATACODE_E2E_AGENT_PROVIDER and
KATACODE_E2E_AGENT_MODEL to snapshot their prior values before overwriting them,
then restore those original values afterward instead of always deleting them.
Use the existing beforeEach and afterEach hooks in agent.test.ts to keep the env
state isolated per test.
Comment on lines
+93
to
+96
| it("throws fail-loud when the provider config is missing", () => { | ||
| delete process.env.KATACODE_E2E_AGENT_PROVIDER; | ||
| delete process.env.KATACODE_E2E_AGENT_MODEL; | ||
| expect(() => buildAgentMaestroEnv("run-x")).toThrow(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the provider-missing path in isolation.
This case removes both env vars, so it still passes if buildAgentMaestroEnv() only fails on a missing model. Keep KATACODE_E2E_AGENT_MODEL set here and unset only KATACODE_E2E_AGENT_PROVIDER so the test actually covers the contract named in the title.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mobile-e2e/src/flows/agent.test.ts` around lines 93 - 96, The test for
missing provider config in agent.test.ts is not isolated because it deletes both
KATACODE_E2E_AGENT_PROVIDER and KATACODE_E2E_AGENT_MODEL, so it can pass for the
wrong reason. Update the failing-case setup around buildAgentMaestroEnv to keep
KATACODE_E2E_AGENT_MODEL defined and unset only KATACODE_E2E_AGENT_PROVIDER, so
the assertion truly verifies the provider-missing path described by the test
name.
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 Changed
mobile-e2e/Maestro + TypeScript orchestrator suite for iOS Simulator flows.@smoke,@pairing,@auth, and@agent, plus CLI scripts for running, listing, building, testing, and opening Maestro Studio.KATACODE_HOME, server startup, project registration, artifacts, and Maestro execution.connection-status-*accessibility/test id contract for mobile pairing assertions.Why
This creates the mobile counterpart to the existing Electron E2E foundation: real services, fail-loud prerequisites, reusable harness/flow boundaries, local artifacts, and documented authoring workflows for expanding mobile coverage safely.
UI Changes
N/A. No user-facing UI behavior change beyond adding a deliberate accessibility/test id to
ConnectionStatusDotfor deterministic E2E assertions.Testing
@smoke,@pairing, and@agentgreen.@authremains TODO because the native ClerkNativeClerk.presentAuthmodal still needs Maestro Studio drivability validation.Checklist
Greptile Summary
This PR establishes the mobile Maestro E2E testing foundation for iOS Simulator flows, mirroring the existing Electron E2E structure with real services, fail-loud prerequisites, isolated run contexts, and documented authoring workflows. It also extracts a shared
formatModelDisplayNameformatter to keep the server display layer and mobile E2E harness in sync.mobile-e2e/: New TypeScript orchestrator with harness modules for simulator selection, server lifecycle, port allocation, artifact logging, and Maestro execution; backed by unit tests for all pure functions.packages/shared/src/model.ts: ExtractsformatModelDisplayNamefromCodexProvider.tsinto the shared package, eliminating a fragile duplicate and wiring the mobile picker labels to the same canonical logic.apps/mobile: AddstestID/accessibilityLabelkeyed to connection state onConnectionStatusDotas a stable E2E assertion contract.Confidence Score: 4/5
Safe to merge — all app-production changes are additive and low-risk; the new harness is local-only and doesn't affect any shipped code paths.
The production-code changes (shared model formatter, testID on ConnectionStatusDot, CodexProvider delegation) are minimal and well-tested. The harness itself is large but self-contained and local-only. A few behavioral quirks in the new harness are worth a follow-up: the timeout exit code in runMaestro is never actually emitted (a no-op race with child.once close), resolveFlowPaths silently drops untagged flows in run-all mode, and the --include-tags parser can accidentally consume another flag as a tag value. None of these affect correctness of the current flows or production behaviour.
The four issues flagged all live in mobile-e2e/src/ — specifically maestroRunner.ts (timeout code race), cli/run.ts (untagged flow filter), cli/args.ts (flag consumption), and cli/flows.ts (inline YAML tags). The app-layer files are clean.
Important Files Changed
Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant CLI as run.ts (main) participant Prereqs as prereqs.ts participant Sim as simulator.ts participant Server as serverStack.ts participant Maestro as maestroRunner.ts participant Artifacts as artifacts.ts CLI->>Prereqs: requirePrereqs(tags) Prereqs-->>CLI: ResolvedCredentials CLI->>Artifacts: createIsolatedRun() Artifacts-->>CLI: MobileE2ERunContext (katacodeHome, port, artifactRoot) CLI->>Sim: ensureSimulator(context) Sim->>Sim: xcrun simctl list / bootstatus Sim-->>CLI: simulatorUdid CLI->>Sim: assertDevClientInstalled(context) alt runNeedsServer CLI->>Server: startServerStack(context) Server->>Server: spawn katacode serve Server->>Server: waitForServePairing (stdout scan) Server->>Server: project add (seeded workspace) Server-->>CLI: ServePairingInfo end CLI->>Maestro: runMaestro(flowPaths, env, timeoutMs) Maestro->>Maestro: "maestro test --include-tags ... -e KC_*=..." Maestro-->>CLI: MaestroResult (exit code) CLI->>Artifacts: writeRunManifest CLI->>Server: terminateChildProcess (cleanup) CLI->>Artifacts: rm katacodeHome (cleanup)%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant CLI as run.ts (main) participant Prereqs as prereqs.ts participant Sim as simulator.ts participant Server as serverStack.ts participant Maestro as maestroRunner.ts participant Artifacts as artifacts.ts CLI->>Prereqs: requirePrereqs(tags) Prereqs-->>CLI: ResolvedCredentials CLI->>Artifacts: createIsolatedRun() Artifacts-->>CLI: MobileE2ERunContext (katacodeHome, port, artifactRoot) CLI->>Sim: ensureSimulator(context) Sim->>Sim: xcrun simctl list / bootstatus Sim-->>CLI: simulatorUdid CLI->>Sim: assertDevClientInstalled(context) alt runNeedsServer CLI->>Server: startServerStack(context) Server->>Server: spawn katacode serve Server->>Server: waitForServePairing (stdout scan) Server->>Server: project add (seeded workspace) Server-->>CLI: ServePairingInfo end CLI->>Maestro: runMaestro(flowPaths, env, timeoutMs) Maestro->>Maestro: "maestro test --include-tags ... -e KC_*=..." Maestro-->>CLI: MaestroResult (exit code) CLI->>Artifacts: writeRunManifest CLI->>Server: terminateChildProcess (cleanup) CLI->>Artifacts: rm katacodeHome (cleanup)Comments Outside Diff (2)
mobile-e2e/src/harness/maestroRunner.ts, line 859-883 (link)The
finally(() => resolve({ code: 124 }))call is always a no-op. When the timer fires,gracefulKillsends SIGTERM and awaits the child's exit. Once the child exits,child.once("close")(registered in the outer Promise body) fires first and settles the promise with the real exit code (e.g.nullor143). By the timegracefulKillresolves and thefinallyblock runs, the promise is already settled — the secondresolveis silently discarded. A timed-out run is therefore indistinguishable from a normal Maestro failure in the exit code seen byrunFlows.Prompt To Fix With AI
mobile-e2e/src/cli/flows.ts, line 76-98 (link)parseFlowTagsdoesn't handle inline YAML tag sequencesThe parser only recognises the block-list form (
tags:\n - @smoke). Maestro also accepts inline sequences (tags: [@smoke, @pairing]). A flow authored with the inline form will return[]tags and be excluded from every filtered run without any warning. Adding a guard or a note in the authoring guide would prevent silent omissions.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "docs(mobile-e2e): document Node 24 prere..." | Re-trigger Greptile
Summary by CodeRabbit