Repository navigation
Conversation
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>
…irectory Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
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>
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
All reported issues were addressed across 178 files
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
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 39 files (changes from recent commits).
You’re at about 93% 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
…CLI env plumbing Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
The code needs no changes before merge, but the kernel-errors mutation checks are red and must pass first. I reviewed commit 5effcd7. All five kernel-errors shards fail the Stryker dry run on one test, host-kit/src/code-signature.test.ts:149 ("the daemon source graph stamps the walker that produced it"). That test is unchanged and passes in the unit lanes. The shards passed in run 38965344815. The related-test set for these shards grew from 1372 to 1607 files, which pulled this test in. The new global setup file (https://github.com/callstack/agent-device/blob/5effcd7/src/__tests__/host-environment-setup.ts#L8) imports host-kit/command and so exec.ts and kernel/errors, so I think it caused that growth. I did not reproduce the failure in the sandbox. The walker probably resolves workspace symlinks outside the sandbox root, but I have not proven that. The "Affected decision-kernel mutants" check fails after the dry run ("13 report(s), expected 18"), and Smoke Tests was still running when I looked. No conflicts. The next step is to remove the global aroundEach setup, or to make that test work in the sandbox. Re-run the kernel-errors shards after that. Not blocking, and you can take or leave these. The setup file binds every unit test to the live process.env and process.cwd(), so no test can see host_environment_unscoped (exec.ts:140). Please bind createTestHostEnvironment only in suites that enter through the daemon or CLI, and add one test that calls a commandHostEnvironment() consumer outside a scope and expects that reason. Also, createAgentDeviceClient(config, { host }) at https://github.com/callstack/agent-device/blob/5effcd7/src/agent-device-client.ts#L89 uses host only in stateDir(), while the daemon client still reads process.env and process.cwd() (daemon-client-lifecycle.ts:97-140, 818). Either every resolution the client makes should read the same host, or deps.host can wait for #3751. Finally, RequestExecutionScope.host (https://github.com/callstack/agent-device/blob/5effcd7/packages/daemon/src/server/request-execution-scope.ts#L263) has no production reader except its own test, because daemon handlers use the ambient commandHostEnvironment() (19 call sites). Would one channel per layer be simpler? host-kit would keep the ALS scope only for spawn env and the DEVELOPER_DIR overlay. Daemon handlers would read host from RequestExecutionScope or the runtime deps they already get. Platform code would take host from its existing host or tool-provider object, as AndroidAdbHost.environment does. That would remove the throwing ambient accessor and the test-wide shim, and a missing host would become a type error. #3750, #3751 and #3752 will copy whatever shape this PR sets. Could we settle in #3747 or an ADR whether HostEnvironment travels explicitly or ambiently before they land? The open inline threads still apply. The two P2 threads on connection.ts (#3763 (comment) on the home path, #3763 (comment) on provider profile) and the P2 thread on bundletool lookup (#3763 (comment)) are still open. One lower-priority thread also applies: #3763 (comment). |
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
You’re at about 94% 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 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>
…kages Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… scenarios Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Pushed |
|
There was a problem hiding this comment.
All reported issues were addressed across 97 files (changes from recent commits).
You’re at about 94% 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
|
Thanks for the PR. The host capture is a good direction, but one defect in the client needs a fix before merge. CI is green at d522d25 (23 checks, none failing), and there are no conflicts. createAgentDeviceClient now accepts Not blocking, and you can take or leave these: Could the AsyncLocalStorage scope keep only what it already owns, which is spawn env ( I did not trace every public SDK export (artifacts, android-adb, limrun, ai-sdk) to prove none reaches This is reviewed at d522d25. Before merge, the client needs to resolve every state dir read from one host, and the scope question for #3750 and #3751 needs an answer. |
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>
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
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
|
Pushed Also taken from the non-blocking list: On the ambient versus explicit question: the ambient |
…rectory Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks for the update. Since the review at d522d25 (#3763 (comment)), the client now resolves the policy against
Not blocking, and fine to take or leave: Could Of the open inline threads, the cubic-dev-ai thread on the policy path (#3763 (comment)) still applies until the spawn cwd is fixed. These no longer apply and you can resolve them: the I did not run any test or daemon; the daemon cwd finding is traced from the spawn options and the entry capture, and I did not check whether other daemon reads also resolve relative env values against the inherited cwd. Remote artifact upload still reads process env and cwd, which I accepted as deferred to #3751 without tracing it. At f4616b9 Smoke Tests are queued, and Repo Guards & Compatibility and Coverage are still running with no failure yet. This change touches the daemon-client route that smoke runs use, so a failure there would likely be related. There are no conflicts. Before merge, please launch the daemon with |
…ment * origin/main: test: move the daemon-composition tests off the daemon's testing exports (#3742) fix(android): ignore emulator log lines in -list-avds output (#3743) # Conflicts: # packages/daemon/src/server/__tests__/request-router-fill-text-stdin.test.ts # packages/daemon/src/server/__tests__/request-router-macos-app-lease.test.ts # packages/daemon/src/server/__tests__/request-router-provider-credentials.test.ts # packages/daemon/src/server/__tests__/request-router-record-runtime-lock.test.ts # packages/daemon/src/server/__tests__/request-router-recording-health.test.ts # packages/daemon/src/server/__tests__/request-router-replay-scope.test.ts # packages/daemon/src/server/__tests__/request-router-screenshot.test.ts # packages/daemon/src/sessions/request-handler-chain-routing.test.ts # src/__tests__/daemon-composition/app-event-delivery.test.ts # src/__tests__/daemon-composition/gesture-admission-parity.test.ts # src/__tests__/daemon-composition/http-server-limrun-uploaded-apps.test.ts # src/__tests__/daemon-composition/request-router-android-perf.test.ts # src/__tests__/daemon-composition/request-router-android-snapshot-helper.test.ts # src/__tests__/daemon-composition/request-router-open-claim-recovery.test.ts # src/__tests__/daemon-composition/request-router-replay-test-ime.test.ts
…g test Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…rectory Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… 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>
There was a problem hiding this comment.
All reported issues were addressed across 101 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
…n test envs Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
The code is ready at 225e0bd. The merge conflict from the earlier review is gone, and the three inline fixes from the other reviewer are in at this commit. Not blocking: On the inline threads: all three from the other reviewer are fixed at 225e0bd, so please resolve them: #3763 (comment) (unused host removed from the install-from-source route), #3763 (comment) (doc comment now names only the routes that read the host), and #3763 (comment) (idle-expiry tests now use the test host environment). Smoke Tests is still running on 225e0bd with no failure. The diff touches the route it exercises (CLI to daemon launch, and daemon request handling), so a failure there would need a look against these changes. I did not run any tests locally, so your gate claim for this commit is unverified by me. I also did not re-check the inline |
…l call; default the lease provider cwd to the bound host Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ment * origin/main: refactor(daemon): publish a single package entrypoint (#3781) fix(daemon-client): wait for an acknowledged quiesce instead of killing the daemon (#3780) fix(daemon): refuse leases for providers without a registered runtime (#3773) # Conflicts: # packages/daemon/src/sessions/handlers/__tests__/lease.test.ts # src/daemon-client/daemon-client-ensure.ts
|
Pushed
|
|
The PR is ready at 916db0b. Both earlier cubic threads that the delta touched are fixed, and no blocking issues remain. Not blocking, and fine to take or leave: the field doc at https://github.com/callstack/agent-device/blob/916db0b/packages/daemon/src/sessions/request-handler-chain.ts#L34 still lists only the session and record-trace routes, but the lease route now reads host too (line 157), so the doc could name it or just say host is the request's bound host. Also, no test pins the one-transport-per-run hoist or the single MCP host capture, so a runCli test with remoteConfig could assert transportForHost is called exactly once (today https://github.com/callstack/agent-device/blob/916db0b/src/daemon-client/daemon-client-host.test.ts#L62 only checks it gets the host). Of the open inline threads, none are P1 or P2 and still apply, and no lower-priority ones apply. These do not apply and can be resolved: the unused host parameter in session-app-source-deployment.ts is gone since 225e0bd (#3763 (comment)); the idle-expiry test fixtures now use createTestHostEnvironment().env (#3763 (comment)); and the request-handler-chain.ts:34 rewording is superseded by the note above (#3763 (comment)). CI is green at 916db0b: 23 checks, none failing. I did not run the tests myself, and I judged the lease.test.ts regression coverage from a read of the code before the change. I did not re-review the content of #3773, #3780 and #3781, only how their merge interacts with the host plumbing. Nothing else stands between this PR and a human merge. |
…he bound host Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Pushed
|
There was a problem hiding this comment.
All reported issues were addressed across 3 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
…transport test Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ment * origin/main: feat(provider-testmu): publish @agent-device/testmu on plugin contract v2 (#3770) docs(plugins): document plugin contract v2, the inventory and ADR 0034 (#3769) feat(daemon): daemon status lists the registered provider plugins (#3768) feat(plugins): plugin contract v2 and one provider registry (#3767) # Conflicts: # packages/daemon/src/sessions/handlers/lease.ts # src/__tests__/cloud-connect-testmu.test.ts # src/cli/commands/connection.ts # src/cli/commands/daemon.ts # src/daemon-client/daemon-client-metadata.ts # src/daemon-client/daemon-client.ts
…ment * origin/main: fix(ios-runner): stop wide sliders from wedging tree snapshots (#3776)
… leaving their shared prologue untouched Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
This PR is ready at c9aa2f2. The test changes since the earlier review (916db0b) look good, and the doc comment now says the request's bound host, which every route reads. Not blocking: in src/tests/cli-run-transport.test.ts (https://github.com/callstack/agent-device/blob/c9aa2f2/src/__tests__/cli-run-transport.test.ts#L65) a failure other than 'exit' after lease_allocate still shows up only in an assertion message that already passes, the test never asserts the expected exit code, and fs.rmSync sits outside a finally block so a failing run leaks the temp workspace. You can take or leave these. Both open inline threads from the earlier round are fixed at this commit and can be resolved: the doc comment on the request handler chain (#3763 (comment)) and the runCli error in the lease_allocate assertion (#3763 (comment)). The exit code is still not asserted, as noted above. The Smoke Tests and Coverage checks are still queued or in progress, and none has failed. The new changes touch only tests and one doc comment, so they do not overlap with the smoke route. The full PR does change the CLI to daemon host plumbing that the smoke tests exercise, so a smoke failure would need a closer look. No conflicts are known. The PR needs those two checks to pass at c9aa2f2 before merge. I read the code and did not run the new tests. That the vi.spyOn on the @agent-device/host-kit/process namespace takes effect for the dynamic import in command-tools.ts rests on the author's check:affected report and on the same spy pattern working in daemon-registration-owner.test.ts. Code merged from main still reads process.env by default, for example connectCommand's primeProviderConnection and listPlugins(process.env) in plugins.ts. That predates this change and is tracked in #3750 and #3751, and under the CLI it has the same values as the captured host. The request-router redaction test I asked for earlier is not in these changes; the PR only adds a createTestHostEnvironment() host fixture there. |
…t cache its process already had Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ys remove its workspace Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 3 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
…h of its env override Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
* origin/main: fix(daemon): scope a transient device claim to the session that holds it (#3714) # Conflicts: # packages/daemon/src/resources/device/__tests__/device-claim-admission.test.ts # packages/daemon/src/server/request-execution-scope.ts
… cleared it Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Pushed
|
…nd 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>
|
The code is ready for review at 870cf66. The earlier review (#3763 (comment)) found no problems, and the new #3775 changes in this round do not change that. The harness env ordering is now fixed, so the cubic P2 thread on harness.ts:119 no longer applies, so you can resolve it. Not blocking, and you can take or leave these: the harness builds its gateway with The only open item is CI. Smoke Tests are queued or running with no failure yet. #3775 changes the root composition env and the CLI handler host, which the smoke route uses, so a smoke failure here would need a look. Smoke Tests must pass at 870cf66 before merge. I did not run tests. I reviewed #3775's 98 files for new process.env or cwd reads, layer crossings and the resolveHubAppReference contract, but not its test call-site churn line by line. The changes merged from main (#3776, #3714, #3762) are upstream, and I checked them only where they meet host plumbing. |
… host claims directory on a failed release Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 3 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.
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="test/integration/provider-scenarios/ios-record-trace.test.ts">
<violation number="1" location="test/integration/provider-scenarios/ios-record-trace.test.ts:116">
P3: This comment attributes the remaining process reads to 'the root helpers' moving in #3777, but root `src/` was already migrated in #3775, which is merged into this branch. Only the capture-kit reads (e.g. `swift-cache.ts` reading `AGENT_DEVICE_SWIFT_CACHE_DIR`) still depend on #3777, so drop 'and the root helpers' from the comment.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| appleRunnerProvider: () => appleRunnerProvider, | ||
| appleToolProvider: () => appleTool.provider, | ||
| appleSimulatorScreenRecordingTransport: () => recordingTransport, | ||
| deviceInventoryProvider: async () => [PROVIDER_SCENARIO_IOS_SIMULATOR], | ||
| }); | ||
| // The recording finalizer and Swift helper still read PATH and the cache from the process | ||
| // (capture-kit and the root helpers move to the host in #3777), so the scenario sets both. |
There was a problem hiding this comment.
P3: This comment attributes the remaining process reads to 'the root helpers' moving in #3777, but root src/ was already migrated in #3775, which is merged into this branch. Only the capture-kit reads (e.g. swift-cache.ts reading AGENT_DEVICE_SWIFT_CACHE_DIR) still depend on #3777, so drop 'and the root helpers' from the comment.
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 test/integration/provider-scenarios/ios-record-trace.test.ts, line 116:
<comment>This comment attributes the remaining process reads to 'the root helpers' moving in #3777, but root `src/` was already migrated in #3775, which is merged into this branch. Only the capture-kit reads (e.g. `swift-cache.ts` reading `AGENT_DEVICE_SWIFT_CACHE_DIR`) still depend on #3777, so drop 'and the root helpers' from the comment.</comment>
<file context>
@@ -112,6 +112,8 @@ test('iOS simulator recording reports host contention and recovers through the f
deviceInventoryProvider: async () => [PROVIDER_SCENARIO_IOS_SIMULATOR],
});
+ // The recording finalizer and Swift helper still read PATH and the cache from the process
+ // (capture-kit and the root helpers move to the host in #3777), so the scenario sets both.
const previousPath = process.env.PATH;
const previousSwiftCacheDir = process.env.AGENT_DEVICE_SWIFT_CACHE_DIR;
</file context>
| // (capture-kit and the root helpers move to the host in #3777), so the scenario sets both. | |
| // (capture-kit moves to the host in #3777), so the scenario sets both. |
Summary
Part of #3747. Closes #3749.
HostEnvironment = { env, cwd }now has one owner. Entry points capture it once, and the daemon runtime, request scope and command spawning receive it instead of readingprocess. About 260 files change, most of them test call sites.HostEnvironmentis in@agent-device/kernel/source-value.captureHostEnvironment(process)is in@agent-device/host-kit/process. It copies the environment.src/bin.ts,src/cli.ts(runCli(argv, deps, host)),src/daemon.tsandcreateAgentDeviceClient(config, { host })capture once. A client built without a host capturesprocessand then reads only that host: its default transport, daemon launch, state dir andstateDir()all resolve from it.startDaemonRuntimerequireshost.DaemonRuntimeDepsInput.hostreplacesenv.RequestRouterDeps.hostandRequestExecutionScope.hostcarry it. Request handlers, claim writers, lease admission and the reapers inpackages/daemonread configuration from that host explicitly (throughRequestHandlerChainParams.hostor the env their constructor receives), not from the ambient scope.withCommandHost({ host, requestEnv }, fn)replaceswithRequestCommandEnv. Spawned commands start fromhost.env.commandHostEnvironment()still gives platform and root code that host and throws outside one; it is meant for command execution only, and their owners (refactor(platforms): read host settings from HostEnvironment in platform and capture packages #3750, refactor: read host settings from HostEnvironment in root src and remaining packages #3751) convert the remaining readers to explicit hosts.whichCmd,resolveUserPath,expandSessionPath, the device-claim functions,loadDaemonPolicy,launchDaemonProcessand kernel allowlist and simulator-set resolvers take env or cwd as parameters.process.envorprocess.cwd().Behavior changes: a relative state dir without a
cwdis refused (relative_state_dir_without_cwd; callers now pass one), and a relativeAGENT_DEVICE_CLAIMS_DIRis refused (relative_claims_dir; the store is shared by every daemon on the host, so it used to depend on where one of them started). Suites that reach host-reading code outside a daemon request or the CLI bind a host withbindTestCommandHost()from test-kit; there is no global test setup.The root
src/readers moved to the host in #3775, which merged into this branch. The platform and capture packages still read the process directly, for #3777 to migrate, socommandHostEnvironment()stays until it lands.Validation
pnpm check:affected --run --head 9f3e58ffbb(exactCommit) passed, with currentmainand #3775 merged.🤖 Generated with Claude Code