Repository navigation
refactor(host): read host settings from HostEnvironment in root src and remaining packages - #3775
Conversation
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
All reported issues were addressed across 58 files
You’re at about 95% 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
c3fe1a5 to
0b6ab69
Compare
|
The code looks good at 0b6ab69, but I cannot call it done yet because live evidence is still missing. I did not run tests. This read relies on the green checks (12 reported, none failing) and on tracing the code. I did not check out-of-tree plugin hosts that call provider-webdriver helpers directly. I also did not confirm that the AsyncLocalStorage scope reaches every long-lived daemon timer or stream created before the scope that startDaemonRuntime opens. There are no conflicts. No finding blocks the merge. Please decide who owns the provider hub-app cwd default (first note below), and Cubic has not reviewed this head yet. Not blocking, take or leave: (1) BrowserStack (browserstack.ts:174) falls back to commandHostEnvironment().cwd for the hub-app cwd, but TestMu goes through the plugin.ts wrapper (https://github.com/callstack/agent-device/blob/0b6ab69/packages/provider-webdriver/src/plugin.ts#L40), which falls back to process.cwd(). (2) src/io.ts:17 adds createLocalArtifactAdapterAt, and both callers pass cwd explicitly, which createLocalArtifactAdapter already accepts. (3) The custom.test.ts:142 test binds { env: process.env } and then changes process.env.HOME. The old os.homedir() reads HOME too, so the test passes on the old code. Use withCommandHost with { env: { HOME: home }, cwd } and leave process.env.HOME alone, so the old code fails. (4) daemon-composition.ts:91 holds env but createHostDiagnostics reads commandHostEnvironment() ambiently, and the PR body's Remaining list omits the env defaults in plugins/connection.ts, plugins/store.ts, cli/connection/provider-policy.ts and provider-device-runtimes.ts. Could the ambient-host pattern stay and still be simpler? Fill provider request.cwd once from the bound host, or in one helper that both hub providers call. Then resolveHubAppReference takes a required cwd and the plugin.ts wrapper goes away. Use createLocalArtifactAdapter({ cwd }) instead of a new At variant. Pass the env the composition already holds into createHostDiagnostics. Nothing has to change first, because provider-webdriver is private and its plugin subpath has no external compatibility need. Of the four Cubic threads, all four are fixed at this head, so please resolve them: #3775 (comment) (ledger now changes only the resolveMaterializedArtifactPath entry), #3775 (comment) (custom.ts uses resolveUserPath with the host env; the test gap is note 3 above), #3775 (comment) (ledger rationale names the cwd fallback), #3775 (comment) (remote-config.test.ts asserts the released entry reads process.env). |
There was a problem hiding this comment.
All reported issues were addressed across 13 files (changes from recent commits).
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
|
The earlier review at 0b6ab69 asked for a live run. At 6a0fb60 the code looks fine, but there is still no live run, and the PR has merge conflicts with main. Please rebase onto refactor/host-environment and resolve the conflicts. I did not check the conflict content. Fallow audit is failing, and it looks unrelated to this change. The I still need one live daemon run at this head. (a) Run The Cubic thread on Not blocking: the same break also reaches out-of-tree callers of |
49a6474 to
8278b13
Compare
|
Review round at
|
|
Thanks for the update. The cwd fallback change and the ledger fixes at 8278b13 look good, but one regression remains in the Node client, and CI is green (12 checks, none failing). Fixed since the earlier review: the ledger rationales now name the host cwd, the remote-config test covers the Still open: commit 2ad2ed7 replaced the cwd fallback in Not blocking, and you can take or leave it: I read the code at 8278b13. I did not run tests, typecheck or the live daemon, and I know the author's live runs only from the comment, which has no transcript. I relaxed two earlier asks after reading the code, because the daemon env and the remote-artifact On the other threads: the provider_app_cwd_missing break still applies and is the finding above. These are fixed or no longer apply, so please resolve them: ledger digests (fixed), custom reporter HOME (out of the diff, handed to #3763), materializeRemoteArtifacts rationale (fixed), remote-config env test (fixed). Before merge, the Node client must send |
8278b13 to
8eff3d9
Compare
|
Applied every reviewer recommendation at
|
|
The earlier findings at 6a0fb60 are fixed at 8eff3d9, and no code problem remains. The One ask remains before merge. Not blocking, and you can take or leave it: the title of commit eef3345 says "fill the lease cwd from the host", but that commit has no lease.ts change, so you could reword it when squashing. All 12 checks pass at 8eff3d9, and there are no conflicts. I read the code at this commit only. I did not run tests, typecheck, fallow, or a live daemon. Your live run (Android doctor, plugins list, macOS open/snapshot/close) was at 8278b13. The five commits are identical at this head, so I count it as covering this commit. A relative BrowserStack or TestMu The inline thread on |
8eff3d9 to
ae8f733
Compare
|
Applied the 12:36Z review at
The branch is based on the current |
|
Rebased onto refactor/host-environment c9aa2f2 (plugin contract v2 and the merge of main) at |
ae8f733 to
0d8e1b5
Compare
…ation and materialization Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…iagnostics, toolchain and deployment hosts Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…king directory Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… options, binding and auth Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…path Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
0d8e1b5 to
e248942
Compare
|
This PR is ready: at e248942 the earlier findings from 8eff3d9 are fixed and I found nothing new in the code. Smoke Tests are still queued or in progress with no failure so far, and the diff touches the route they run (CLI handler host threading, daemon-composition env wiring and daemonCommand), so a failure there would need attribution to this PR. I did not run tests, typecheck, fallow or check:affected, so the "gate green" claim is unverified. The live run (Android doctor, plugins list, macOS open/snapshot/close) was at 8278b13. The patch is the same at this head apart from conflict-context fixes in daemon-composition.ts and daemon.ts, so I count it as covering this head. I did not rerun Not blocking: the new Unreleased line in CHANGELOG.md says On the open inline threads, none of the P1/P2 threads still apply and no lower-priority threads remain. These can be resolved: #3775 (comment) (the CHANGELOG now records the cwd requirement and the Merge once Smoke Tests finish green. |
|
* 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
Part of #3751 (umbrella #3747). Configuration reads in root
src/**,packages/remoteandprovider-webdrivertake the host explicitly instead of readingprocess.env/process.cwd(): CLI command handlers (newhostparam, set once per run incli.ts), option/binding/auth resolvers, the connection profile chain, remote artifact preparation and materialization (threaded through the daemon-client transport), and the platform runtime gateway, inventory gateways, diagnostics, toolchain and deployment hosts. Released entries keep their process default in a wrapper:resolveRemoteConfigProfile,createLocalArtifactAdapter.Behavior note: a relative
--provider-apppath on a request with no cwd now fails withprovider_app_cwd_missing; the base fills the lease cwd from the host.Remaining, handed to owners
session-journalcap,device-selectionenv,replay-port/maestro/ad-scriptreplay env and cwd.platform-runtime-android-adb-host.ts), app-log output and process env, platform-* packages.provider-device-runtimes.ts,provider-credential-fingerprint.ts,cli/connection/provider-policy.ts,plugins/*.update-check.ts, replay-test reporters and progress,color.ts,diff.ts,commands/replay/index.ts,doctorCliOutput,plugins.ts,cli-config.ts(the MCP caller must pass env first).Validation
check:affected --rungreen at8eff3d9ad6, rebased on 916db0b. Rootsrckeeps 5commandHostEnvironment()reads (4 command execution, 1 doctor output colour); 23 branch-wide against 56 on the base. Live run at an earlier head: real daemon, macOS open/snapshot/close,plugins list, Androiddoctor.