fix(daemon): run Apple tools with the requesting client's DEVELOPER_DIR - #3109
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 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
… classify xcrun tool-not-found on every exec failure
|
Reviewed 55de3f0. The per-request DEVELOPER_DIR idea works, but one shared memo still ignores it, so I can't call this ready to merge yet. CI is green: the one reported check passes. No conflicts. requireRunnerToolchainFingerprint in https://github.com/callstack/agent-device/blob/55de3f0/packages/platform-apple/src/runner/runner-cache-metadata.ts#L295 memoizes the Xcode and SDK fingerprint in a process-wide createTtlMemo with no ttlMs, keyed only on sdkName. Client A (Xcode 27) fills it. Client B (Xcode 26.6) then gets A's fingerprint, so B's runner build key and derived-data path name Xcode 27 while its xcodebuild runs under 26.6. B can reuse A's Xcode 27 xctestrun, or file its own 26.6 build under the 27 key, and the failure reports name the wrong Xcode. Before this PR a daemon had one toolchain, so that key was enough. Two clients with different DEVELOPER_DIR on one daemon is exactly what this PR enables, so runner-backed commands like snapshot, click and type on a simulator can run another client's build. The rule to satisfy: every process-wide memo whose value depends on the selected toolchain is keyed on the effective developer dir of the request that reads it. Could you expose the effective DEVELOPER_DIR from host-kit (the requestCommandEnvScope value, falling back to process.env) and add it to this key? Please also grep platform-apple for createTtlMemo and for module-level single-flight promises that wrap xcrun or xcodebuild output, and apply the same key to each. The reported failure has no regression test on its production route, in https://github.com/callstack/agent-device/blob/55de3f0/src/daemon/request-router.ts#L192. exec.test.ts covers only the host-kit primitive, and the http-server test stubs handleRequest. Reverting the request-router.ts hunk or the daemon-client.ts hunk leaves every added test green, and buildLocalHostEnvMeta (daemon-client.ts:139) has no test for local versus remote daemons. Could you add a router test that calls handleRequest with meta.developerDir and a handler that spawns process.execPath printing DEVELOPER_DIR, and asserts the value? Could you also add a daemon-client test that asserts developerDir is in meta for a local DaemonInfo and absent for a remote one? The change crosses client meta, the kernel contract, the HTTP boundary, the router and a new host-kit AsyncLocalStorage seam, and each per-request toolchain then needs its memos keyed on it. Would it be simpler to treat the client's DEVELOPER_DIR as part of the daemon's launch identity, so a mismatch is refused or respawns the daemon, as daemon-launch-spec does for other drift? The daemon would keep one toolchain, and no memo key or wire field would change. The maintainers need to choose between one daemon serving several Xcodes and one daemon pinned to one. If per-request stays, the memo key above comes first. In that case the xcrun exit 72 classification also belongs in the platform-apple localAppleToolProvider (tool-provider.ts), which owns every xcrun simctl and devicectl call, not in host-kit. Not blocking, take or leave: xcrunToolNotFoundDetails in host-kit exec.ts#L742 runs only in createExitError, so allowFailure results raised later through requireExecSuccess (for example the simctl pbpaste clipboard read in app-device-io.ts:15) get no reason or hint, and classifying in execFailureDetails would cover both paths. Also readXcodeSelectPath (exec.ts#L728) runs a spawnSync of xcode-select -p with a 2 s timeout on the error path and puts Apple knowledge into platform-neutral host-kit, so the hint could just say "the xcode-select developer dir (xcode-select -p)". After the key fix, please run one daemon with two clients that use different DEVELOPER_DIR values against a booted simulator, on a runner-backed command such as snapshot, not only devices. The --debug runner.log or daemon log should show each client's xcodebuild running under its own Xcode and each runner derived-data path or build key naming that Xcode's build version. The current evidence covers only device listing, reported in the PR body without logs, on a host with no working simulators. I read the diff and did not run tests, and the memo problem is inferred from the memo key and derived-path code, not reproduced. |
… prove DEVELOPER_DIR reaches each request
|
@thymikee thanks for the careful review. (Same disclosure as the PR body: this reply and the follow-up commits were written by an AI coding agent working for @GenericJam. You're still welcome to close this and treat it only as a report.) What changed (pushed, head
Why the classification isn't in Design question: your call. Pinning one toolchain per daemon ( Live verification: not done yet. CoreSimulator on this host is currently wedged, so every Checks on
Reviewed with a reviewer subagent (Codex CLI rate-limited until Oct 7); verdict MERGE on |
There was a problem hiding this comment.
1 issue found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/platform-apple/src/runner/__tests__/runner-cache-metadata.test.ts">
<violation number="1" location="packages/platform-apple/src/runner/__tests__/runner-cache-metadata.test.ts:782">
P2: This assertion fails whenever `AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH` is set, because `resolveRunnerDerivedPath` then returns the same override for both metadata values. Save and unset the override for this test, then restore it afterward.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| assert.equal(underB.xcodeBuildVersion, '18A100'); | ||
| // The derived path is what a retained runner is reused by, so a differing one makes it stale. | ||
| assert.notEqual( | ||
| resolveRunnerDerivedPath(IOS_SIMULATOR, underA), |
There was a problem hiding this comment.
P2: This assertion fails whenever AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH is set, because resolveRunnerDerivedPath then returns the same override for both metadata values. Save and unset the override for this test, then restore it afterward.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/platform-apple/src/runner/__tests__/runner-cache-metadata.test.ts, line 782:
<comment>This assertion fails whenever `AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH` is set, because `resolveRunnerDerivedPath` then returns the same override for both metadata values. Save and unset the override for this test, then restore it afterward.</comment>
<file context>
@@ -751,3 +756,38 @@ test('an unreadable or setting-less build log fails the check', () => {
+ assert.equal(underB.xcodeBuildVersion, '18A100');
+ // The derived path is what a retained runner is reused by, so a differing one makes it stale.
+ assert.notEqual(
+ resolveRunnerDerivedPath(IOS_SIMULATOR, underA),
+ resolveRunnerDerivedPath(IOS_SIMULATOR, underB),
+ );
</file context>
There was a problem hiding this comment.
Fixed in 3142915. The test now calls the existing withoutRunnerDerivedPathEnv() fixture (it unsets the variable and restores it in onTestFinished), so the whole test runs without the override. The file passes with AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH=/tmp/pinned set.
…cleared DEVELOPER_DIR
|
(Same disclosure as before: this comment and commit 3142915 were written by an AI coding agent working for @GenericJam. Feel free to close the PR and fix this your own way.) cubic's three findings on
Live verification you asked for (one daemon, two clients, runner-backed command, booted simulator). CoreSimulator works again on this host, so I ran it on the PR build (
Every snapshot succeeded (19–20 nodes from Settings). The retained runner from the other Xcode was refused, not reused. In both directions the request log shows Both caches already existed from earlier runs on this host, so both legs were cache reuses ( Checks on
Reviewed by a reviewer subagent (Codex CLI rate-limited until Oct 7): SHIP (its three P3 notes are applied in this commit). The design question (per-request vs. one toolchain pinned per daemon) is still yours to decide. Happy to rework or close. |
|
Reviewed 3142915. This is ready for maintainer review: at this commit the code in the earlier review (55de3f0) is fixed, and I have no code changes to ask for. Not blocking, take or leave: the xcrun "tool not found" classification in https://github.com/callstack/agent-device/blob/3142915/packages/host-kit/src/internal/exec.ts#L718 now keys on the stderr line alone, so it also applies to non-72 exits, and the repository rules ask us to key on typed reasons, so you could gate it on exit code 72 and use the regex only to read the tool name; the router drops an empty The checks are green, with 1 check and none failing. I did not run any tests, and I judged the revert-failure cases by reading the tests against the old code. I judged the live run from the log excerpts in your comment, not from raw logs. Both legs reused existing runner builds, so a cold build under each Xcode is not shown, and the idle expiry is covered only by the fake-timer test. Both live clients set On the earlier question about pinning the toolchain in the daemon launch identity instead, I am not raising it again. Before merge, a maintainer needs to choose between per-request |
thymikee
added
the
ready-for-human
…lugin * origin/main: (628 commits) fix(ios): pin runner build roots under derived data (callstack#3158) feat: add managed provider plugin infrastructure (callstack#3121) fix(ios): report keyboard focus from the AX bridge's is-editing trait (callstack#3163) feat(devices): report model and osVersion (callstack#3119) fix: guard alert deadline before native tap synthesis (callstack#3113) feat(install-source): accept archive URLs from any public host (callstack#3110) test: keep uptime responsive behind busy runner work (callstack#3114) fix(apple): read the launch confirmation whenever the open cannot see the app (callstack#3115) fix(host-kit): keep extracted directories owner-accessible (callstack#3111) fix(daemon): run Apple tools with the requesting client's DEVELOPER_DIR (callstack#3109) fix(daemon): fence daemon.json removal to its owning process (callstack#3102) fix(snapshot): stop sibling-sized chrome containers from covering their own region (callstack#2996) (callstack#3097) fix(ios): stop reading windows past the one the runner resolved (callstack#3103) refactor(daemon): apply one dispatch-disclosure rule to returned and thrown failures (callstack#3099) chore: drop unused production exports and suppress dynamic consumers (callstack#3100) docs(help): document wait readiness and restart exhaustion (callstack#3098) docs: simplify Host to fresh Simlock devices and lease recovery (callstack#3095) feat(capture): report the display rotation a screenshot was rendered in (callstack#3088) refactor(snapshot): preserve normalized node attributes through presentation (callstack#3092) refactor(help): colocate fold guidance and extract workflows (callstack#3093) ... # Conflicts: # README.md # package.json # packages/kernel/src/snapshot.ts # src/__tests__/eager-closure-budgets.ts # src/cli/commands/connection-presentation.ts # src/commands/schema/cli-help.ts # src/commands/schema/command-overrides.ts
2 participants
Summary
Local clients now run Apple tools with their own
DEVELOPER_DIR, independently of the daemon's startup environment. Unset values inherit the daemon environment;DEVELOPER_DIR=""selects the host'sxcode-selecttoolchain. Remote requests discard the client host path.Runner artifacts and durable leases carry the existing cache fingerprint, so retained-runner reuse and daemon adoption refuse a different toolchain even with a fixed
AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH. External runner artifacts retain their existing reuse behavior. Missing xcrun tools produce actionable diagnostics.35 files, including installation docs, wire compatibility metadata, regression tests, shared runner fixtures, and one test move to avoid growing an oversized test file.
Validation
Commit
015ad0872fb72f5e2b8edeb3aaaa6c6245f9c879:AGENT_DEVICE_VITEST_MAX_WORKERS=2 pnpm check:affected --run: all runnable gates passed; 7,657 tests across 961 files. Earlier runs exposed a load-sensitive child-process cleanup race; the final run passed without test retries.pnpm check:xctest-selection: passed.Only one Xcode is installed on this host; cross-Xcode invalidation is covered by regression tests. GitHub CI on the updated head remains to be verified.