Skip to content

fix(ios): preserve final-probe xcodebuild diagnostics - #1776

Open
emmanuelchucks wants to merge 2 commits into
callstack:mainfrom
emmanuelchucks:fix/ios-runner-final-probe-exit
Open

fix(ios): preserve final-probe xcodebuild diagnostics#1776
emmanuelchucks wants to merge 2 commits into
callstack:mainfrom
emmanuelchucks:fix/ios-runner-final-probe-exit

Conversation

@emmanuelchucks

@emmanuelchucks emmanuelchucks commented Aug 15, 2026

Copy link
Copy Markdown

Summary

  • Preserve the established xcodebuild early-exit error when a physical iOS runner exits during the final connection probe, instead of replacing its actionable diagnostics with Runner did not accept connection.
  • Keep request-cancellation and usbmux-unattached errors ahead of the terminal child-exit check; simulator fallback is unchanged.
  • Separate startup/readiness transport from one-shot command transport and mirror that boundary in the focused tests.

Closes #1775.

Validation

  • Regression proof: removing the terminal child-exit recheck from the current startup module produces 1 failed / 8 passed, with Runner did not accept connection received instead of the xcodebuild early-exit error.
  • Focused startup and one-shot transport suites pass 15/15. Their split preserves all 15 test cases, and a planted type error confirmed the new test module is included by TypeScript.
  • pnpm check:affected --run on Node 22.22.0 passes 274 files and 2,225 tests; changed-line coverage is 92/118 (77.97%) and changed-branch coverage is 64/87 (73.56%). Package verification, formatting, lint, typecheck, layering, Fallow, build, affected coverage, and provider integration pass.
  • The deterministic unit bundle passes 836 files and 6,557 tests. Static smoke coverage passes 38 tests with the three documented live-device/browser tests skipped.

The physical-device reproduction is recorded in #1775. This PR changes terminal error precedence without manufacturing another certificate-trust failure on a device.

Scope: 13 files, confined to Apple runner transport, mirrored tests, shared transport fixtures, and the moved Fallow baseline entry. No CLI, wire, Swift runner, documentation, or skill changes.

Copilot AI lite review requested due to automatic review settings August 15, 2026 12:55

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts the iOS runner connection failure precedence so that when the physical-device runner process exits during the final connection probe, the user-facing error preserves the actionable xcodebuild diagnostics (instead of being replaced by the generic “Runner did not accept connection”).

Changes:

  • After the connection retry loop completes for physical devices, re-check the runner child’s exitCode and throw buildRunnerEarlyExitError when it has exited.
  • Add a regression test that simulates the “child exits during final probe” race and asserts the nested xcodebuild stderr and exit code are preserved.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/platforms/apple/core/runner/runner-transport.ts Adds a post-retry early-exit check for physical-device sessions to preserve xcodebuild diagnostics.
src/platforms/apple/core/__tests__/runner-transport.test.ts Adds a regression test covering the final-probe race and asserting the early-exit error details.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@thymikee

Copy link
Copy Markdown
Member

Exact head 4b4b633d fixes the reported race correctly: cancellation and usbmux-unattached errors retain precedence, simulator fallback is unchanged, and the physical-device terminal path now uses the established early-exit builder. The planted regression is non-vacuous and preserves exit code 65 plus the actionable certificate-trust stderr.

[P2 topology] src/platforms/apple/core/runner/runner-transport.ts was already 519 LOC on the base and is 522 LOC here. AGENTS.md’s implementation tripwire requires extraction before adding behavior past 500 LOC. Please move the startup/readiness connection loop (waitForRunner and its attempt helpers) into a focused runner-startup transport module, mirror the source/test topology, and retain this regression; keep the one-shot command transport in the existing focused module.

CI is also owner-blocked rather than green: all eight exact-head Actions runs are action_required, so a maintainer must approve them after the topology fix. No ready-for-human label while the code finding and authoritative CI blocker remain.

Copilot AI review requested due to automatic review settings August 15, 2026 14:17
@emmanuelchucks

Copy link
Copy Markdown
Author

Addressed in c634be380.

  • waitForRunner and its attempt helpers now live in runner-startup-transport.ts (440 LOC); runner-transport.ts retains the one-shot command path (93 LOC).
  • The tests mirror that split with 9 startup and 6 one-shot cases, shared fixtures, and the final-probe regression retained. The Fallow baseline entry moved with the extracted code.
  • Focused transport tests pass 15/15. pnpm check:affected --run passes 274 files / 2,225 tests with 77.97% changed-line coverage; the deterministic unit bundle passes 836 files / 6,557 tests.

The PR description now reflects the current head. This head is ready for CI approval.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/platforms/apple/core/runner/runner-transport.ts:92

  • fetchWithTimeout can surface a raw AbortError (non-AppError) when the caller-provided requestSignal is aborted mid-request. That error then normalizes to an UNKNOWN error instead of the standard request-canceled AppError shape. Since this helper is now exported and shared, it’s safer to translate request-driven aborts into createRequestCanceledError() here.
  const signal = requestSignal ? AbortSignal.any([requestSignal, timeoutSignal]) : timeoutSignal;
  return await fetch(url, { ...init, signal });

@thymikee

Copy link
Copy Markdown
Member

Exact head c634be380 is code-review clean. The topology blocker is resolved: startup/readiness transport is now isolated in a cohesive 440-LOC module, the one-shot transport is 93 LOC, production imports and the Fallow baseline moved coherently, and the tests mirror the split with shared fixtures. The terminal child-exit recheck and its load-bearing race regression remain intact; cancellation/usbmux precedence and simulator fallback are unchanged.

The only current blocker is owner-action CI: all eight exact-head Actions runs concluded action_required, so a maintainer must approve them and obtain authoritative results. CLEAN/MERGEABLE at the code level, but no ready-for-human label until those workflows run without a confirmed failing/blocked CI state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

iOS: final runner probe can replace an xcodebuild launch failure with a generic connect error

3 participants