Repository navigation
refactor(platforms): read host settings from HostEnvironment in platform and capture packages - #3777
Conversation
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
All reported issues were addressed across 70 files
You’re at about 96% 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
457bd1b to
bc06e18
Compare
|
Thanks for the PR. At bc06e18, one change is needed before merge. The checks show 12 passing and none failing.
Not blocking, and you can take or leave these: On the open threads, this one on the PNG worker diagnostics is marked resolved but still applies. These threads are fixed at this head, so please resolve them: run-script-http-child.ts:9 passes The PR has merge conflicts with its base Before merge, replace the copied helper with the shared host-kit export, then rebase onto the updated |
bc06e18 to
f547d88
Compare
|
Rebased onto the narrowed base and converted configuration reads to explicit passing ( Ambient Converted: Still ambient, and why:
Both gaps close with one change: a |
|
Answering the 09:43Z review ( Duplicate
Waiting on the coordinator to choose: keep the local copy, or move those two test-kit fixtures out so capture-kit can depend on test-kit.
Spawn env assertions (left): they pin the explicit |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
You’re at about 97% 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
|
At 70d3211 I found no blocking defects. Not blocking: The earlier findings from the bc06e18 review (#3777 (comment)) are fixed. I withdraw the duplicate I read the code and did not run it on a device. The live device evidence is still your earlier report, and the runner change in this delta is a rename. I also did not trace that the production daemon's request host is the process host for every route, so "no CLI-visible change today" is inferred. Smoke Tests was still running when I checked, so I cannot attribute CI yet. There are no conflicts. Before merge, wait for Smoke Tests to finish and reply to the open P3 thread on mid-request worker failure (#3777 (comment)): The other review threads are fixed at this head and you can resolve them: #3777 (comment) (prewarm no longer latches the report, and a test covers it), #3777 (comment) ( |
|
Applied the review follow-ups at
|
There was a problem hiding this comment.
All reported issues were addressed across 15 files (changes from recent commits).
You’re at about 97% 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
|
|
Coordinator review at 320af90: please revert the test-kit cycle-break and keep the capture-kit-local To drop test-kit's dependency on capture-kit, this head re-implements production behaviour in test-kit:
Both trade one three-line duplicated test helper for duplicated production semantics. Requested shape:
The PNG-worker and spawn-env changes in this push are good; keep them. |
320af90 to
3fbabc3
Compare
|
Applied the coordinator's ruling at
|
3fbabc3 to
88a82c3
Compare
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
You’re at about 97% 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
|
Applied the 11:38Z recommendation at
|
|
Reply to the review at
Say if you still want the unused local helper restored. |
|
Rebased onto |
867301d to
8b5e97f
Compare
|
Rebased onto |
8b5e97f to
d6273a3
Compare
|
The PR is ready at 8b5e97f. The earlier clean review at 70d3211 still holds, and the four inline threads from cubic-dev-ai are now fixed at this commit: the worker-unavailable event in png-worker-client.ts, the restored host env and cwd test in snapshot-source/host.test.ts, the throwing FakeWorker.last check, and the app-log live handle that now delegates to capture-kit. Not blocking, and you can take or leave these: the memoised paths in packages/capture-kit/src/recording/video.ts#L97 and overlay.ts#L53-L62 come from the first caller's host, so a later host with a different Swift build directory or cwd reuses them; keying the memo on that directory, or documenting that the first host wins, would fix it. The Android and macOS recording scenarios in test/integration/provider-scenarios/android-recording-fixtures.ts#L42 still set PATH and I ran no tests, so the report that 5 provider-integration tests fail without harness.ts:115 is unverified. I could not separate commits 17-18 from base drift, so I checked that result by reading test-kit/src/app-log-live-handle.ts at head. Smoke Tests is still queued, not failed. It may reach the recording finalize and Android install-artifact routes this PR touches, so it needs to finish green before merge. There are no known conflicts. All four cubic-dev-ai threads are fixed at this commit, so you can resolve them: png-worker-client unavailable event (#3777 (comment)), host env and cwd test (#3777 (comment)), FakeWorker.last check (#3777 (comment)), and the app-log live handle (#3777 (comment)). This review covers 8b5e97f. The branch has since been rebased onto 870cf66 as d6273a3; a range-diff shows the PR's own commits unchanged apart from one context line in the Android deployment executor, so this verdict should carry over. I will confirm it on the new head. |
|
Applied the 15:36Z recommendations at
|
d6273a3 to
3279ad2
Compare
…ing the command host The eager-closure budgets forbid the host-kit command import in the install source download, the app-log file policy and the Maestro HTTP helper, so their callers pass the environment they already hold. Client tests that change PATH give the client a host instead. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Redaction patterns come from the command host, the maestro child reads its own environment without a working-directory lookup, local ~ install sources expand against the host environment, and the runner launch resolves its host inside the try that closes its log descriptor. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…he command host Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…e reads configuration Android install artifacts read the environment from the Android adb host port, Apple install artifacts take it as an option from the root deployment executor, and the Apple runner names its host port member hostEnvironment. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ostics scope that sees it A startup pre-warm runs outside any scope, where the event is dropped; it no longer latches the one-time report. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…test the runner's bound host behaviorally test-kit no longer depends on capture-kit: its app-log live-handle alias is gone (callers use the capture-kit constructor) and its keyboard fixture builds its own snapshot state. capture-kit's tests take the helper from test-kit. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… its own entry Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…apture-kit Replaces the capture-kit subpath entry, which grew the declared export surface. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ecording tooling capture-kit no longer reads the command host, so its suites need no bound host and its local test-host copy is gone; the root recording seams supply the host. test-kit keeps depending on capture-kit again, and capture-kit does not depend on test-kit. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…equest Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The Android adb host port's environment is bound at module load, so a host whose environment differs from the process's got the process's proxy settings on a network install. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ectory Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…or scenarios that set it on the process Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…were built for The video validator is cached per Swift cache root and the overlay scripts per working directory, so a second host does not reuse the first host's paths. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…gh the harness Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…forwards the process's Swift cache Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…hat capture-kit takes the host Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Rebased onto |
3279ad2 to
f0b20fa
Compare
|
Reviewed f0b20fa. The code looks good. Since the earlier review at 70d3211, Android install takes its environment from the caller as intended, and the rebase conflict in the iOS recording scenario is resolved correctly: the process The three cubic threads are fixed at this head, so you can resolve them: the test-kit app-log handle now wraps the capture-kit handle (#3777 (comment)), the detached bridge test pins host env and cwd again (#3777 (comment)), and the PNG worker test fails with a clear message when no worker starts (#3777 (comment)). Not blocking: the PR body still says Android reads its adb host port and cites the validation head f547d88, which the rebase dropped. The device validation is from an earlier head. That still covers this change, because the delta only moves the same host-environment read from capture-kit defaults up to the root seams. Smoke Tests is still running; the other 11 checks pass. There are no conflicts. |
…refactor/daemon-runtime-no-process-globals * origin/refactor/host-environment: refactor(platforms): read host settings from HostEnvironment in platform and capture packages (#3777) # Conflicts: # packages/platform-apple/src/runner/runner-artifact.ts # packages/platform-apple/src/runner/runner-cache.ts
* feat(host): capture HostEnvironment once at entry points Add HostEnvironment to the kernel and captureHostEnvironment to host-kit. The CLI, the daemon executable and the client factory capture it once; startDaemonRuntime requires it and the request scope exposes it. Host-kit, kernel and daemon-contracts no longer read the process environment or working directory. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(host): resolve relative state dirs against the captured working directory Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(host): give platform code the command host instead of process reads Platform packages may not read process; they take the environment and working directory of the enclosing command host. Unit tests enter the live process environment as their host. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(host): answer review of the host environment foundation Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(host): keep the test host setup out of mutation scope and finish CLI env plumbing Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(host): bind the test command host per suite, not globally The global setup made every test related to every mutated source. Suites that reach host-reading code through neither a daemon request nor the CLI bind a host with bindTestCommandHost(). Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * chore(deps): declare test-kit in the HarmonyOS and Linux platform packages Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(host): bind the test command host in the stale-provider admission scenarios Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(host): resolve every client read from the client's host createAgentDeviceClient's default transport, the daemon launch and the CLI run's transport read the host the client was given, not the process. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(host): resolve the daemon policy path against the host working directory Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * style: format the daemon policy reuse test Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test: merge the duplicate test-kit import in the handler chain routing test Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(host): start a launched daemon in the launching host's working directory Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test: keep the registration-owner test under its size ratchet Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test: merge the duplicate test-kit import in the stale provider admission scenarios Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(daemon): read configuration from the request host instead of the ambient scope Handlers, claim writers and lease admission take the host (or its env) from the request execution scope, runtime-owned timers take it at construction. commandHostEnvironment() stays for command execution until the platform and root packages convert. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(daemon): drop the unused host from install-from-source and tighten test envs Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(cli): resolve one transport per run and one host per MCP tool call; default the lease provider cwd to the bound host Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test: pin the one-transport CLI run and one-host MCP call; document the bound host Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test: keep the failure that explains a missed materialization in the transport test Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(daemon): read the host cwd where the deploy handlers use it, leaving their shared prologue untouched Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test: give the iOS recording scenario's daemon host the PATH and Swift cache its process already had Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test: fail the transport test on an unexpected exit or throw and always remove its workspace Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test: keep the scenario harness's state dir and transport out of reach of its env override Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(daemon): abandon a transient device claim under the same env that cleared it Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(host): read host settings from HostEnvironment in root src and remaining packages (#3775) * refactor(host): thread the client host through remote artifact preparation and materialization Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(host): pass the composition's env to the platform runtime, diagnostics, toolchain and deployment hosts Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(host): refuse a relative provider app path that names no working directory Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(host): require the host's env and cwd where the CLI resolves options, binding and auth Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(host): keep the upload client suite under its size ratchet Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(host): pass the run's host to CLI command handlers Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(host): bind host-taking commands once in a shared helper Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * docs(changelog): resolveHubAppReference needs cwd for a relative app path Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(host): give the new handler suites a host Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com> * test: build the scenario gateway on the daemon host's env and pin the host claims directory on a failed release Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test: say which package still reads PATH from the process in the recording scenario Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(platforms): read host settings from HostEnvironment in platform and capture packages (#3777) * refactor(capture-kit): read swift cache, app-log and PNG worker settings from the command host The PNG worker starts with an empty environment and reports unavailability through diagnostics instead of a process warning. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(provision-kit): read proxy settings from the command host Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(react-native): read dev-server settings from the command host The detached companion process starts with an explicit env and cwd, and its process entry captures its host from its own spawn environment. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(host): run the dev-server helpers inside the client's and the SDK's host The helpers moved from process reads to the command host, so the released agent-device/metro functions and the client bind one at their boundary. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(platform-apple): give detached runner and bridge children an explicit env and cwd Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(platform-apple): read runner settings from the command host The runner reaches the host through its port. The container bundle ids were fixed at import time from the process environment; they are now resolved from the host a caller passes in. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(host): run the daemon clean script's runner lease cleanup inside its own host Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(host): hand leaf modules their environment instead of importing the command host The eager-closure budgets forbid the host-kit command import in the install source download, the app-log file policy and the Maestro HTTP helper, so their callers pass the environment they already hold. Client tests that change PATH give the client a host instead. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(host): bind the command host in the install-artifact suites Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(host): answer review of the platform host environment change Redaction patterns come from the command host, the maestro child reads its own environment without a working-directory lookup, local ~ install sources expand against the host environment, and the runner launch resolves its host inside the try that closes its log descriptor. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(react-native): take the host explicitly instead of reading the command host Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(platforms): pass the host explicitly where a platform package reads configuration Android install artifacts read the environment from the Android adb host port, Apple install artifacts take it as an option from the root deployment executor, and the Apple runner names its host port member hostEnvironment. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(daemon): hand the app-log marker the request host's environment Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(capture-kit): report an unavailable PNG worker in the first diagnostics scope that sees it A startup pre-warm runs outside any scope, where the event is dropped; it no longer latches the one-time report. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(host): share the test command host binding through test-kit and test the runner's bound host behaviorally test-kit no longer depends on capture-kit: its app-log live-handle alias is gone (callers use the capture-kit constructor) and its keyboard fixture builds its own snapshot state. capture-kit's tests take the helper from test-kit. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(capture-kit): expose the app-log live handle adapter through its own entry Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(test-kit): build the test app-log handle without importing capture-kit Replaces the capture-kit subpath entry, which grew the declared export surface. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(capture-kit): take the host explicitly for Swift cache and recording tooling capture-kit no longer reads the command host, so its suites need no bound host and its local test-host copy is gone; the root recording seams supply the host. test-kit keeps depending on capture-kit again, and capture-kit does not depend on test-kit. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(capture-kit): report a worker that dies under a request in that request Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(platform-android): take the install environment from the caller The Android adb host port's environment is bound at module load, so a host whose environment differs from the process's got the process's proxy settings on a network install. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(platform-apple): pin the detached snapshot bridge's starting directory Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(capture-kit): fail when the pre-warm starts no worker to kill Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(host): keep the provider scenario host's Swift cache directory for scenarios that set it on the process Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * fix(capture-kit): key the cached Swift helper paths on the host they were built for The video validator is cached per Swift cache root and the overlay scripts per working directory, so a second host does not reuse the first host's paths. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(host): give the Android recording scenarios their host env through the harness Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(host): scenarios name their own host env; the harness no longer forwards the process's Swift cache Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * test(host): drop the iOS recording scenario's process-env block now that capture-kit takes the host Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com> * test: pass the host to the iOS e2e live runner's request Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(react-native): drop the redundant outer catch around the scoped Metro session read; changelog the PNG worker change Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(capture-kit): report an unavailable PNG worker in every scope that takes the fallback; the daemon pre-warms it inside a daemon diagnostics scope Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * refactor(capture-kit): report each PNG worker transition to unavailable once, through a daemon-registered sink with a typed reason Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
Summary
Platform and capture packages read the host's environment and working directory from a
HostEnvironmentinstead ofprocess. No behavior change. Closes #3750 (part of #3747; stacked on #3763). 92 files, mostly tests.react-native,provision-kit,maestro: the host or environment is a parameter. Releasedagent-device/metrobinds the process host at its boundary; the client passes its own.environmentfrom their caller (the root deployment executors pass the command host's), so a host whose env differs from the process's gets its own proxy settings.hostEnvironment, R77 intact); detached children get an explicitenvandcwd.resolveRunnerContainerBundleIds(env)replaces the import-time constant.capture-kittakes the host for the Swift cache and recording paths; cached helper paths are keyed per cache root and directory. The PNG worker starts with an empty env and reports unavailability in the first diagnostics scope that sees it.Still ambient
commandHostEnvironment(): root seams (app-log output, deployment executors, recording finalizer, Android recording, contact sheet, runner bundle-id check) and Apple screenshot, file copy and snapshot-bridge spawn.Validation
pnpm check:affected --run --head f0b20fae1b: passed,exactCommit: true; CI green at that head.Device (earlier head): Android emulator and iOS 27.0 simulator passed runner snapshot, screenshot, PNG-worker diff, recording overlay, and app log. Not rerun since; runner detach/adopt across a daemon restart not run.
🤖 Generated with Claude Code