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>
|
Both items are done at One classifier owns the launch-log verdict. The regression you described is a test: Live evidence. The green iOS Smoke Tests job (job 117178907306, run 38961942036) at I also ran it by hand with the source-built CLI at this head, on a simulator I created (
That launch's Per-attempt cost of the 64 KB tail read, measured with the production reader ( One thing I could not verify, unchanged from before: no host here has an Xcode older than its macOS, so the bless refusal itself rests on the reporters' excerpts, and I did not trace the daemon restart policy beyond the |
|
This PR is ready to merge at cf83c86. Both findings from the earlier review are fixed: the two-cause log case is covered, and the classifier verdict is now checked on every starting attempt. All 20 checks pass at this head, including iOS Smoke Tests, which drives the changed connect loop on a real simulator. No conflicts. I did not run the apple-runner tests or open the iOS Smoke log. I had no host with an Xcode older than its macOS, so the fixture rests on the excerpts quoted in #3449. Not blocking: the scoped-set launch with a missing destination now fails on the first attempt that reads the log, with no restart or rebuild, but the docs describe that fast failure only for simulator_service_hub_unavailable. One clause on the simulator_set_destination_not_found entry would cover it. You can take or leave this. The four open inline threads from the other reviewer no longer apply at this head, so you can resolve them: the docs entry for simulator_set_destination_not_found is fixed, the classifier verdict check on every starting attempt is fixed, the xcodeVersion fixture scope is now limited to the two cases that need it, and the runner-device-set JSDoc is rewritten in plain sentences. |
thymikee
added
the
ready-for-human
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. Since the round-2 review the verdict is the classifier's own: each launch-log row declares the cause it names, and the loop throws the built error only when the recovery table says a retry cannot help, so a tail holding two signatures gets the first row's winner on both axes. 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
cf83c868d.pnpm check:affected --run --head cf83c868d— all runnable checks passed,exactCommit=true(receipt.tmp/validation/run-YVEuy2/receipt.json): format, lint, typecheck, layering, fallow, build, vitest-related, docs-links and docs-prose ran green; coverage, the native/device/replay lanes and provider integration stay with GitHub and all pass at this head. Thelinux.gapfailures from the previous round are gone — #3740 landed. iOS Smoke Tests (job 117178907306), including "fixture-backed iOS simulator E2E smoke", passed atcf83c868d: the runner boots through the changed connect loop, preflights, serves the Settings replay, the gesture replay, and the full fixture E2E tier=smoke.Regression evidence —
pnpm exec vitest run --project apple-runnerover the owning test files, production modules reverted (file copies, not stashes):cf83c868d: 911 passed (whole project)d91886f76(round-1 production): the both-signatures test fails and burns its full 5 s connect budget — the reviewer's regression exactlyThe fail-fast tests are
a bless refusal already in the launch log ends the connect loop on its first attemptandone tail holding two launch-log causes gets the classifier's own winner, terminal(row order, not the loop, decides;fetchnever called). Two neighbouring tests hold the boundary — an older generation's refusal and a session that already answered keep ordinary retry behaviour.Live evidence for the changed connect loop (source-built CLI at
cf83c868d, fresh simulator3449 Hub Verify, iPhone 17 Pro / iOS 27.0, deleted afterwards): coldopen Settings(runner rebuilt for the new UDID, 7.0 s request), thensnapshot -i(19 interactive nodes) andpress @e7(tapped, General screen shown).runner.logfor that launch moves the session starting→ready:AGENT_DEVICE_RUNNER_WAITING→AGENT_DEVICE_RUNNER_LISTENER_READY→AGENT_DEVICE_RUNNER_PORT=65268, and every command after it was served by that runner. Per-attempt cost of the 64 KB tail read (readRunnerLogTail, the production reader): ~49 µs on this real session log, ~74 µs worst case against a 2 MB log returning the full 64 KB.Still no bless 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.🤖 Generated with Claude Code