feat(client): serve getEnv over runner IPC - #446
Conversation
Sorry, something went wrong.
Sorry, something went wrong.
Motivation: The JS client already exposes getEnv, but before this change it always fell back to no-op behavior. Runner-aware tools need a real round-trip way to ask the runner for an env value before those reads can be used for cache tracking. Scope: Add the GetEnv request/response frame, response reading in the Rust client, server-side resolution from the spawned task env map, and a real NAPI getEnv implementation. This PR intentionally does not add tracked envs to cache fingerprints. Verification: - cargo test -p vite_task_server --test integration - UPDATE_SNAPSHOTS=1 cargo test -p vite_task_bin --test e2e_snapshots fetch_env_reads_declared_env -- --ignored - cargo test -p vite_task_bin --test e2e_snapshots fetch_env_reads_declared_env -- --ignored
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01191f8ed1
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Sorry, something went wrong.
| // TODO(env-track): A later PR in this stack replaces this child env | ||
| // map with the full planned env context so IPC can serve envs even | ||
| // when they were not declared in `env` or `untrackedEnv`. | ||
| &spawn_execution.spawn_command.all_envs, |
There was a problem hiding this comment.
Avoid serving untracked envs without cache-key tracking
When a cached task declares an output-affecting variable under untrackedEnv, SpawnCommand::all_envs still contains that value, so this handoff lets getEnv("FOO") return it even though this commit does not record env reads in Reports or add them to the cache fingerprint. A task can then cache output that depends on FOO and incorrectly hit after FOO changes; until tracked reads are implemented, serve only fingerprinted envs or force tracked getEnv reads to affect/disable caching.
Useful? React with 👍 / 👎.
Sorry, something went wrong.
| } | ||
|
|
||
| fn get_env(&mut self, name: &OsStr) -> Option<Arc<OsStr>> { | ||
| self.envs.get(name).cloned() |
There was a problem hiding this comment.
Match env names case-insensitively on Windows
On Windows, env names are case-insensitive, and the planner already matches env config case-insensitively while preserving the parent variable's original casing. If the spawned env contains Path but a tool calls getEnv("PATH") (or declares env: ["PATH"]), this exact FxHashMap lookup returns None even though the child process has that variable; use a case-insensitive lookup under cfg(windows) to match spawned-process semantics.
Useful? React with 👍 / 👎.
Sorry, something went wrong.
Merge activity |
Sorry, something went wrong.
Motivation
The JS client exposes getEnv, but before this slice it still fell back to no-op behavior. Runner-aware tools need a real request/response path for single env reads before those reads can be used for cache tracking.
Scope
Add the GetEnv protocol frame, Rust client response handling, server-side resolution from the spawned task env map, and a real NAPI getEnv implementation. This PR intentionally does not add tracked env reads to cache fingerprints.
Verification