feat(protocol): version the runner /command wire protocol + move runner state out of /tmp (#383)#398
Conversation
…sioning + /tmp state relocation Amendments applied from the multi-LLM plan review (Codex + Claude coordinator; Gemini failed on interactive OAuth): - BLOCKER: legacy /tmp state is now adopted once (lenient, protocolVersion 0) so a live pre-upgrade runner is discovered -> classified legacy -> reaped -> relaunched; /tmp files deleted only after the relaunch persists new state. - BLOCKER: stopFastRunner/stopAndroidRunner + device-interact isFastRunnerAvailable fast-paths are adoption-aware (deviceId-scoped), so a post-respawn close/restart/park never leaks a live runner. - BLOCKER: Android mismatch reap reuses releaseAndroidInteractionSlot() (#237, force-stops BOTH owned packages) via dynamic import. - BLOCKER: fixed self-contradictory sanitize-test assertion (no '/' survives). - IMPORTANT: no android-default state key - persist only under a resolved serial; acceptance criterion holds unconditionally. - IMPORTANT: added missed fixture test/unit/runners/rn-android-runner-client .test.js (catch-all fetch mock must branch /health vs /command). - MINOR: pendingUpgradeNote cleared on the mismatch-reject path.
#383) Relocate iOS rn-fast-runner state from the shared /tmp/rn-fast-runner-state.json singleton to a per-UDID hardened file under <stateDir>/runner-state/ios-<udid>.json (schema v1, protocolVersion stamp). Replace the import-time /tmp load with lazy per-device adoption (adoptPersistedFastRunnerState) so a respawned worker rediscovers a live runner. Adoption-aware teardown: stopFastRunner(deviceId?) adopts before reaping; deviceId threaded through every stop call site. Legacy /tmp state is read once (protocolVersion 0) so a live pre-upgrade runner is adopted not orphaned, and deleted only after a fresh relaunch persists the new per-device file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tes in device-session.ts (#383) stopFastRunner(deviceId?) now adopts persisted per-device state before reaping; a bare call after a bridge-worker respawn no-ops and leaks the runner. Both remaining call sites had a deviceId in scope (session and a function parameter, respectively) but weren't passing it.
…stamp on every response (#383)
…NER_PROTOCOL_MISMATCH after failed reinstall (#383)
…nstall on mismatch (#383) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ocs + changeset (#383) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…device-session open paths (#383) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…en catch; type runnerProtocol on StatusResult (#383)
…; tighten open-catch wiring test (#383) Fix 1 (critical): runFlowParked's iOS branch called stopFastRunner() bare, so after a bridge-worker respawn wiped in-memory state, parking the L2 runner ahead of a Maestro flow silently no-op'd instead of adopting the persisted per-device state and killing it — a flow-exclusivity regression. Pass opts.deviceId through and widen the FlowParkOpts.stopFastRunner type to accept it. Fix 2 (important): the two Android close paths (device-session.ts's runner-leak-recovery closeSession, and device-session-close.ts's two teardown branches) called stopAndroidRunner() with no deviceId, missing the same adoption-aware teardown the iOS twins already got. Widened CloseDeviceSessionDeps.stopAndroidRunner to (deviceId?: string) => Promise<void> and threaded the in-scope deviceId through all three call sites. Fix 3 (test): the gh-383 open-catch wiring test sliced from '// Ensure runner + launch.', which includes the try body's own consumePendingAndroidUpgradeNote() call on the success path — so indexOf() matched that call instead of the catch-path discard, making the assertion pass even with the catch-path discard removed. Re-anchored the slice to start at 'catch (err)'. Verified locally (then reverted) that removing the catch-path discard now fails the test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…odebuild forwards it (#383)
![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: 8d8624271f
ℹ️ 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".
| if (!compat.compatible) { | ||
| resolved = true; | ||
| pendingUpgradeNote = undefined; // review amendment: never report an upgrade that failed | ||
| child.kill('SIGTERM'); | ||
| reject( |
There was a problem hiding this comment.
When upgrading with no live runner state but an older rn-android-runner APK still installed, forceReinstall stays false, so ensureAndroidRunnerInstalled() treats the registered instrumentation as reusable and launches the stale APK. This post-start mismatch branch then immediately returns RUNNER_PROTOCOL_MISMATCH instead of doing the intended self-heal; retrying the install/build path with forceReinstall here would let normal plugin upgrades recover without requiring a manual Gradle rebuild/install.
Useful? React with 👍 / 👎.
| forceReinstall = true; | ||
| await reapMismatchedAndroidRunner(deviceId); | ||
| } | ||
| // unreachable/unhealthy: fall through — the fresh start below supersedes it. |
There was a problem hiding this comment.
If the saved Android runner PID is still alive but /health is unreachable or returns ok:false, this fallthrough starts a new instrumentation without stopping the old adb shell am instrument process or removing its forward. In that stale-listener scenario the old instrumentation can still own the UiAutomation slot, so the fresh runner is likely to fail before readiness; release/stop the existing runner before spawning the replacement.
Useful? React with 👍 / 👎.
1 participant
Closes #383 (Story 02 of the Maestro-adoption epic — spec:
docs/stories/02-runner-protocol-versioning.md, PR #381).Why
The native runner HTTP contract was unversioned — bridge/runner skew had no guard and fired live on 2026-07-01 (0.57.1 dist vs 0.57.3 supervisor, undetected). Runner state also sat at fixed shared
/tmppaths, which the session file had already been hardened off for a symlink CVE.What
Protocol handshake
RUNNER_PROTOCOL_VERSION = 1in three synced files (TSprotocol.ts, SwiftRunnerProtocol.swift, KotlinRunnerProtocol.kt) — tri-file grep test enforces agreement.GET /healthnow report{v, protocolVersion, runnerVersion, capabilities: []}; every response carries a"v"stamp (defense-in-depth, checked only when present → rollout order-independent).runnerVersionis a launch-time env echo (iOSTEST_RUNNER_RN_PLUGIN_VERSION— xcodebuild only forwardsTEST_RUNNER_-prefixed vars, device-caught; Android-e RN_PLUGIN_VERSION), so a runner launched by an older bridge is detectable without compile-time version baking and without rebuild loops.runnerVersionskews from the plugin version (fail-open when either side unknown), classifies stale → existing reap+restart path upgrades it transparently; the triggering call carriesmeta.note: "runner upgraded (protocol/version mismatch)". Only a mismatch that survives reinstall surfaces the new typed errorRUNNER_PROTOCOL_MISMATCHwith exact rebuild commands. Android reaps via the bug: agent-device holds Android instrumentation slot, blocking maestro_run UIAutomator2 #237releaseAndroidInteractionSlot(both owned packages) + forced APK reinstall.State relocation
util/secure-state-file.ts(extracted from the CDP-015 session-file hardening: symlink-refusing reads, atomic 0600 writes) now also backs runner state at~/Library/Application Support/rn-dev-agent/runner-state/ios-<udid>.json/android-<serial>.json(Android persists only under a resolved serial — two projects can never share a state file)./tmpfile is adopted once (lenient), reaped, relaunched —/tmpfiles are deleted only after the relaunch persists new state. Grep-enforced test keeps/tmpliterals out of both clients.stopFastRunner(deviceId?)/stopAndroidRunner(deviceId?)+ deviceId threaded through every call site (session close, restart, recovery, maestro park), so a post-worker-respawn stop can't leak a live runner.Surfacing:
cdp_status→deviceSession.runnerProtocol {expected, runner, runnerVersion, pluginVersion, compatible}; doctor guidance forRUNNER_PROTOCOL_MISMATCH.Process
Plan reviewed pre-code by multi-LLM (Codex + Claude coordinator) — 4 blockers fixed in the plan (legacy-adopt-before-delete, adoption-aware teardown, #237-grade Android reap, sanitize-test bug). Executed via subagent-driven TDD (8 tasks, per-task spec+quality reviews, 3 fix waves). Final whole-branch review: ready to merge.
Verification
meta.note✓;/health{ok, v:1, protocolVersion:1, capabilities:[], runnerVersion:"0.57.3"}✓; per-UDID state file mode 0600 ✓;/tmpcleaned post-relaunch ✓; warm reuse 3ms, no restart ✓; teardown clears state ✓. Run 1 device-caught the xcodebuild env-forwarding bug (fixed viaTEST_RUNNER_prefix)./healthproven live on emulator-5554 ({ok, protocolVersion:1, capabilities:[], runnerVersion:"0.57.3", v:1}—-earg echo works). Scripted e2e exercised adopt→classify→reap→force-reinstall; the final readiness wait exceeded the 30s budget purely from host-disk-full emulator degradation (bug: rn-android-runner foreground() pre-flight stalls ~10s (relaunch intent + By.pkg wait) around IME state → client RUNNER_TIMEOUT on guarded verbs #378-class; the emulator now refuses to boot: "not enough disk space" — 891MB free). Bridge-side Android gate fully covered by the unit matrix. Proof artifacts:rn-dev-agent-workspace/docs/proof/383-runner-protocol-versioning/.Rollout note
The first device tool call after upgrading from a pre-protocol plugin pays one transparent runner restart (changeset documents it). Old bridges ignore the new
/healthfields and"v"stamps.🤖 Generated with Claude Code