Repository navigation
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Size Report
Startup median (7 runs, lower is better):
|
The refusal names its cause only in runner.log, and the classifier read that log after the connect loop had spent its whole deadline, so the reporter's ~90 seconds of futile retries survived the first fix. Read the launch tail at each attempt while the session has yet to answer and stop there; the tail is bounded to what an error would quote, read from this generation's offset, and costs no subprocess. Also completes the documented reason vocabulary, which was missing simulator_set_destination_not_found at both prose surfaces, and guards it: the union in runner-error-classification.ts is now checked against both by test/repo-guards/runner-startup-reason-docs.test.ts. Refs #3449, #3739 review round 1. Co-Authored-By: Claude <noreply@anthropic.com>
|
The code needs one change before merge. I reviewed 196030c. The connect loop decides on its own that a launch is terminal, using isSimulatorServiceHubLaunchOutput (https://github.com/callstack/agent-device/blob/196030c/packages/platform-apple/src/runner/runner-error-classification.ts#L228). That function copies the line matching from matchesRunnerToolTextLine (https://github.com/callstack/agent-device/blob/196030c/packages/platform-apple/src/runner/runner-error-classification.ts#L627) and ignores row order. In RUNNER_ERROR_RULES, simulator_set_destination_not_found (L449) comes before the hub row (L466). On a scoped-set simulator whose log tail holds both signatures, the pre-check fires, but buildRunnerConnectError classifies the error as simulator_set_destination_not_found. That cause has connectRetry:true, so each remaining attempt re-throws without dialing until the deadline. This breaks the comment's own rule that the loop check must stay no broader than the row. It also makes the "sole text matcher" claim in the PR body untrue. The rule should be: the launch-log verdict is whatever classifyRunnerStartupFailure returns over the tail, and nothing else. Please delete isSimulatorServiceHubLaunchOutput. In ensureRunnerAttemptCanStart, call resolveRunnerLaunchLogCause (it spawns no process, and the Xcode version is memoized). Throw only when the resulting cause is terminal on connectRetry. That one site then covers every row that names a launch-log cause. Each connect attempt of every starting iOS runner session now reads a 64 KB tail of runner.log (https://github.com/callstack/agent-device/blob/196030c/packages/platform-apple/src/runner/runner-startup-transport.ts#L251). The only CI lane that drives this route on a real simulator is "fixture-backed iOS simulator E2E smoke" (job 117115377520). It failed on linux.gap before it reached the runner, and the PR body claims no device run. Nothing yet shows that a healthy cold boot still connects and serves its first command through the changed loop. After #3740 lands, please rerun that smoke job and show it green. Or attach a local run: a cold Could this get simpler? Drop isSimulatorServiceHubLaunchOutput and simulatorServiceHubFailureMessage. Give the buildFailure row a declared message suffix, and let resolveRunnerLaunchLogCause return the cause of any row that carries one, with no reason-by-reason if chain. ensureRunnerAttemptCanStart would then call it on the tail and throw when shouldRetryRunnerConnectError is false for the built error. The transport would know no hub specifics, and the set-destination cause would get the same fast path. Is there a reason this would not work? I see nothing that must change first. The three automated review threads are all fixed at this head, so they can be resolved: the docs lists (#3739 (comment)), the per-attempt tail check (#3739 (comment), which the first finding above supersedes), and the narrowed provenance header (#3739 (comment)). The six failing CI jobs all fail on "linux.gap is not a function" or TS2339 at test/integration/command-coverage/declarations.ts:558. This diff does not touch that file, main fails the same way, and #3740 fixes it. So they are likely unrelated. The iOS simulator smoke job exercises the changed route but failed before reaching it, so CI gives no evidence either way on this change. I did not run the apple-runner vitest project, so the base/head regression table in the PR body is unverified. I also had no host with an Xcode older than its macOS, so the bless fixture rests on the reporter excerpts quoted in #3449. I did not trace the daemon restart policy beyond the shouldRestartRunnerBeforeCommandSend axis. Before merge, route the connect-loop fast path through classifyRunnerStartupFailure, then show a green iOS simulator smoke after #3740 lands. |
…tup failure An Xcode older than the macOS it sits on cannot load its own DTServiceHub: AMFI refuses its version-restriction entitlement, so every `xcodebuild test-without-building` writes "DTServiceHubClient failed to bless service hub" into runner.log and the runner never serves a command. Callers saw only `Runner did not accept connection`, ordered by a classifier that had no name for this cause, so the request restarted the session and rebuilt a binary this macOS refuses to load (#3449). Classify the bless refusal from the launch output into the typed reason `simulator_service_hub_unavailable`, terminal on every recovery axis, with a hint to select a supporting Xcode on the daemon host (`xcode-select -s` or DEVELOPER_DIR). Both connect-stage failures now read the launch log through one classifier, so the message, reason, hint and logPath cannot disagree.
The refusal names its cause only in runner.log, and the classifier read that log after the connect loop had spent its whole deadline, so the reporter's ~90 seconds of futile retries survived the first fix. Read the launch tail at each attempt while the session has yet to answer and stop there; the tail is bounded to what an error would quote, read from this generation's offset, and costs no subprocess. Also completes the documented reason vocabulary, which was missing simulator_set_destination_not_found at both prose surfaces, and guards it: the union in runner-error-classification.ts is now checked against both by test/repo-guards/runner-startup-reason-docs.test.ts. Refs #3449, #3739 review round 1. Co-Authored-By: Claude <noreply@anthropic.com>
The connect loop decided a boot was terminal with its own bless-signature check, a copy of the row's line matching that ignored row order. On a scoped-set launch whose tail held both signatures the loop stopped while the built error carried `simulator_set_destination_not_found`, whose verdict was retryable, so each remaining attempt re-threw without dialing until the deadline. Now the verdict is whatever `classifyRunnerStartupFailure` returns over the tail: a row declares the cause it names in the message, the classifier returns it, and the loop throws the built error only when `shouldRetryRunnerConnectError` says a retry cannot help. The scoped-set destination cause gets the same fast path, the transport knows no hub specifics, and the two message helpers and the signature predicate are gone. Refs #3449, #3739 review round 2. Co-Authored-By: Claude <noreply@anthropic.com>
196030c to
bd0cae4
Compare
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
You’re at about 91% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…ce-set Refs #3739 review round 2 (Cubic r4298841230). Co-Authored-By: Claude <noreply@anthropic.com>
This branch has not been deployed
1 participant
Summary
An Xcode older than the macOS it runs on cannot load its own
DTServiceHub: AMFI refuses its version-restriction entitlement. Everyxcodebuild test-without-buildingthen writesDTServiceHubClient failed to bless service hub for simulator …intorunner.logand the runner never serves a command, while the caller saw onlyRunner did not accept connection— a failure the recovery table orders a session restart, a resend, and a clean rebuild of the very binary this macOS refuses to load.runner.lognow gets its own startup classification:details.reason: "simulator_service_hub_unavailable", terminal on every recovery axis (connectRetry,retryable,restartBeforeSend,restartAfterReadinessPreflight,artifactSuspectall false), withhintnaming the fix on the daemon host.logPathanddiagnosticIdsurvivenormalizeError.It also stops waiting. Classifying at the deadline would still have made the reporter wait out the connect retries they measured at ~90 s, so the connect loop reads the launch tail on each attempt while the session has yet to answer and ends itself: the same terminal error the deadline path produces, thrown before the next connect attempt is dialed. Bounded and cancellable — one 64 KB tail read, the same one an error would already quote, taken through the session's own reader from this generation's offset, no subprocess. The refusal is not recoverable within a launch, so the first attempt that sees it is the last attempt. Scope is a session that has never answered, so a runner that already served commands is never re-judged by its launch log, and a refusal from an older runner generation stays outside the window.
Per the maintainer's decision, the cause is classified from
runner.logonly — noDTServiceHubpreflight or entitlement probe, which would depend on Xcode's internal layout. Recovery keys on the typed reason; the classifier row is the sole text matcher, over the launch output. Both connect-stage failures (early exit, and a child still alive at the deadline) now resolve their cause through one classifier, so message, reason, hint andlogPathcannot disagree. Out of scope:agent-device-clientdroppingdeveloperDirunderdaemonBaseUrl(intended for remote daemons).Along the way: the documented reason vocabulary was missing
simulator_set_destination_not_foundat both exhaustive surfaces (this docs page and the versioned CLI help), so it is added there, andtest/repo-guards/runner-startup-reason-docs.test.tschecks both againstRUNNER_STARTUP_FAILURE_REASONSso the published vocabulary cannot drift again.Fixes #3449
Validation
Head
196030ce0.pnpm check:affected --run --head 196030ce0— all runnable checks passed,exactCommit=true(receipt.tmp/validation/run-hUNKIa/receipt.json): format, lint, typecheck, layering, fallow, build, vitest-related, docs-links and docs-prose ran green;unitandprovider-integrationwere deduplicated into the related-test run; coverage, the native/device/replay lanes and provider integration stay with GitHub.Regression evidence —
pnpm exec vitest run --project apple-runnerover the owning test files, production modules reverted to base (file copies, not stashes):no fixture records the simulator_service_hub_unavailable reasonfetchnever called and the failure arriving with most of the budget unspentinstallation.md must document the simulator_set_destination_not_found reasonapple-runnerproject, head)The fail-fast test is
a bless refusal already in the launch log ends the connect loop on its first attempt: a 5 s connect budget, the signature present from the start, and assertions that no connect attempt is dialed and that the failure arrives in under half the budget. Two neighbouring tests hold the boundary the fast path needs — an older generation's refusal and a session that already answered both keep ordinary retry behaviour.Two GitHub checks are red for a reason this PR does not touch:
declarations.ts:558callslinux.gap(...), a builder #3717 removed, so the command-coverage manifest fails to load onmainitself — reproducible at0149438fain run 38942449476, which breaks Android Smoke Tests and Repo Guards for every PR until #3740 lands. This change touches no coverage declaration.No live repro: this needs an Xcode older than the host macOS supports, which no device here has. The fixture is a
reported-log-excerptfrom the two reporters'runner.logs; its two matched phrases are quoted verbatim. No device-facing dispatch route changed, so no device run is claimed.🤖 Generated with Claude Code