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. |
|
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.