Repository navigation
feat(staged): per-agent environment variables in Doctor settings - #1049
Merged
Merged
Conversation
Let the user configure a list of environment variables for each agent
under its row in Settings → Doctor, and pass the enabled ones to the
agent process when it launches.
Model: `agent-env-vars` in preferences.json, a `Record<agentId, {name,
value, enabled}[]>` (frontend `agentEnvVars.ts`, backend
`agent::env_vars`). A missing key reads as no variables, so existing
settings need no migration; an entry without `enabled` reads as enabled.
UI: `AgentEnvVarsSetting.svelte` renders one row per variable — enabled
checkbox, name, value, remove — with an "Add variable" button. Unchecked
rows stay but render muted. Names that can't be set (empty, `=`,
whitespace) show an inline error and are kept rather than dropped.
Launch: the session runner reads the agent's enabled variables and
merges them under the session's own extra env before `with_extra_env`,
which the driver applies after the inherited shell snapshot so the
user's values override it. Disabled and invalid rows are skipped; a
later duplicate name wins. Local sessions only — a remote session's
extra env reaches the `sq blox acp` proxy, not the workstation agent.
The pikchr diagram sub-agents get the same treatment.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Address the review of the per-agent environment variables feature (2b1168a): make the plaintext storage decision visible, warn about duplicate names, and show an error for every row the backend skips. Storage: the field description now says values are stored unencrypted in the local preferences file and apply to local sessions only (remote Blox sessions don't receive them). The Rust `agent::env_vars` module docs record why plaintext was chosen — the file is private to the local user, the agent sees the value in its environment anyway, and the web UI already serves preferences — and when to revisit it. Duplicates: the backend still lets the last enabled row win, but every enabled row that shares a name (compared exactly, as the backend does) now shows "Duplicate name: the last row wins". Disabled rows and rows the backend would skip don't take part. Value validation: `validateEnvVarValue` rejects a NUL byte, matching the backend filter, and `validateEnvVar` combines name and value checks as the one predicate behind the inline errors. Row errors move out of the component into `agentEnvVarRowErrors` in `agentEnvVars.ts`, since duplicate detection needs the whole list and the helper is unit-tested without the Svelte runtime. The value input is also described by the row's error for assistive tech. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…names, mask values Address the second review of the per-agent environment variables feature (e9b80f3). Four items. Login-shell spawn path: the driver's `$SHELL -ils` fallback (pikchr sub-agents, and a session whose env snapshot capture failed) seeded `extra_env` with `Command::env` before the shell ran its rc files, so an `export ANTHROPIC_API_KEY=…` in `.zshrc` shadowed the Doctor value there while the direct-spawn path overrode it. `shell_exec_line` now takes the extra env and exec's through `/usr/bin/env -- NAME='value' … <binary>`, which sets the variables after every rc file and before the agent starts; `env` accepts any name the OS does (shell `export` rejects `1FOO`), the `--` keeps a leading `-` from reading as an option, and values are single-quoted. The seed is kept so rc files still see the values. Tests cover the plain, explicit-interpreter and PATH-guard variants, plus an end-to-end run through `sh -s` with an rc-style export before the exec line. `with_extra_env` and the `agent::env_vars` module docs now state the precedence for every spawn path. Launcher-reserved names: a row named `PATH` or `GOOSE_SEARCH_PATHS` is applied as written and so replaces what `apply_managed_tools_env` set for the agent. Not refused — the user may mean it — but the row now shows a non-blocking warning, separate from the errors that mark a row the backend skips. `LAUNCHER_RESERVED_NAMES` lives in `agentEnvVars.ts` with a comment pointing at the Rust function to keep them in sync. Normalisation: one malformed entry used to drop the whole agent's list, and `setAgentEnvVars` then wrote the record back without it, erasing it from disk on the next edit of any agent. `normalizeAgentEnvVars` now skips only the entries that aren't objects with a string name. On-screen masking: the value input is `type="password"` with a per-row Show/Hide value toggle (eye icon, aria-label, aria-pressed, keyboard focusable). Storage is unchanged. The inputs also drop their explicit `text-xs`, which the repo's Input font-size test rejects: the default `text-sm`/`mobile:text-base` keeps iOS Safari from zooming the field. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
… entries per row Address the third review of the per-agent environment variables feature (f8f78e7). Three items; the first is a security regression from that commit. Shell history: the fallback `$SHELL -ils` spawn wrote the values onto the exec line as `exec /usr/bin/env -- 'NAME=value' …`, and an interactive zsh or bash records every line it reads from stdin in the user's history file — immediately under INC_APPEND_HISTORY/SHARE_HISTORY, and bash's `exec` builtin saves history before replacing the process — so an API key configured in Doctor landed in ~/.zsh_history on every pikchr sub-agent launch. The line now carries names only. The values are seeded into the shell's starting environment under mangled names (`__STAGED_EXTRA_ENV_<i>`, from `shell_extra_env_seeds`; the prefix grows an underscore until no user variable starts with it) alongside the real names, which rc files still see, and the line copies them across by expansion: set -- 'NAME='"$__STAGED_EXTRA_ENV_0" …; unset __STAGED_EXTRA_ENV_0 …; exec /usr/bin/env -- "$@" <binary> <args> Going through the positional parameters lets the seeds be `unset` before the exec so they don't leak into the agent's environment, in plain POSIX sh with no `env -u`. Names are single-quoted (any byte but NUL passes; bash history expansion can't fire), the expansion is double-quoted so empty values stay one word, and `--` still guards a leading `-`. The driver seeds the mangled copies only on the shell-spawn path. A new test runs the real exec line through `zsh -ils` and `bash -ils` (skipping a shell that isn't installed) with an isolated HOME/ZDOTDIR, an rc file that re-exports the same name and HISTFILE pointed into that HOME, and asserts the agent sees the override, no seed reaches its environment, the shell did record the line, and the value appears in no file under HOME. The `/bin/sh -s` precedence test stays and now checks the seeds are unset too. Malformed entries: the backend deserialised each agent's list as one `Vec<AgentEnvVar>`, so one entry with a non-string name dropped the whole list while the frontend's `normalizeAgentEnvVars` kept the good rows and showed them as valid — every variable the user saw as enabled was silently not applied until the UI next wrote the list back. Entries are now read one at a time (`AgentEnvVar::from_json`): an entry that isn't an object with a string name is skipped with a warning (index only, never the contents), and a non-string value or non-boolean enabled read as empty and enabled, exactly as the frontend reads them. Unit test added; docs updated. Reveal state: the per-row Show/Hide flags are positional and were only shifted by the component's own remove, so a row removed from another window through the synced store left a flag on its index, unmasking a different row's value. The component now tracks the list length it last produced or saw and clears every reveal when the length changes from outside; its own add and remove update the expectation first, so local edits don't re-mask anything. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
… agent env rows Address the two findings of the fourth review of the per-agent environment variables feature (507c7e6). Non-POSIX login shell: the exec line the driver hands the `$SHELL -ils` fallback is POSIX sh (`set --`, `unset`, `"$@"`), which fish rejects at parse time. Checked against fish 4.9: the fallback never worked there at all — fish has no `-s` and refuses `-ils` outright, and given commands on stdin it reads them all to EOF before running any, so with the driver never closing stdin the agent would not start and the shell would have swallowed its first requests besides. The driver now decides by the shell's basename (`LoginShellExec`): sh, bash, zsh, dash, ksh, ksh93, mksh and ash keep `-i -l -s` with the line on stdin; anything else is spawned as `$SHELL -l -c "exec /bin/sh -c '<line>'"`, so the user's own rc files (config.fish) still run in their shell and the env handoff happens in POSIX sh, which then execs `/usr/bin/env` and the agent as before. The whole line goes inside the wrapper, PATH guard included. The inner line is quoted by fish's single-quote rules (`\` and `'` escaped); other non-POSIX shells get the same as a best effort. Values still never appear in command text: the line carries names and the values ride in the mangled seeds, which `/bin/sh` reads from its environment. The spawn now logs the shell path, which path it took and the names-only exec line, and a spawn failure names the shell, so a maintainer can tell which shell was given what. Tests: a unit test on the generated argument text (POSIX set, fish quoting with `'` and `\`, other shells), and a live run through `fish -l -c` with an isolated HOME whose config.fish re-exports the name — skipped with a message when fish isn't installed (system dirs, Homebrew prefixes, PATH, `brew --prefix` are checked) — asserting the agent sees the override, no seed reaches it, a line written to stdin right after the spawn reaches the agent, and the value lands in no file under HOME. Doctor consistency: Doctor's agent checks and the login fix consult the environment — `claude-agent-acp --cli auth status`, `codex-acp cli login status` and the login CLIs read API keys from it — but ran with the shared snapshot only, so a row that made sessions work could sit next to a check reporting the agent unauthenticated. `DoctorEnv` gains per-check overlays (`with_check_overlay`, `for_check`): variables appended to the snapshot for one check id, so they win for that check's own probes (`acp --help`, auth status, the version probe) and no other check sees them. Binary resolution stays on the shared snapshot. Staged fills the overlays from the agent env rows for every `ai-agent-*` check (`agent_env_for_check`, keyed by the provider id after `ai-agent-`, as the frontend derives it) and adds the same rows to the login fix's environment. The field description now says the rows apply to Doctor's checks and login too. One remaining edge is documented in `agent::env_vars`: the probe runs through `zsh -l -c`, whose `.zshenv`/`.zprofile` can still shadow a row for the probe alone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…, default unknown shells to stdin Address the fifth review of the per-agent environment variables feature (f728336). The previous commit's `$SHELL -l -c` wrapper single-quoted the POSIX exec line by fish's rules, and every shell whose basename wasn't on a POSIX allowlist got it — so a versioned or renamed bash/zsh (`bash5`, `zsh-5.9`), yash, oksh, busybox and the like received a wrapper that is a syntax error in POSIX sh (`\'` closes a single-quoted string there) and never started the agent, where the old `-i -l -s` path had worked. Quoting-free wrapper: the exec line now travels in another mangled seed variable, `__STAGED_EXEC_LINE` (same collision-avoiding prefix scheme as the value seeds; the two prefixes can't collide), and the wrapper is the fixed text `exec /bin/sh -c "$__STAGED_EXEC_LINE"`, which expands the same way in fish, bash, zsh, ksh, dash and csh. The seeded line starts with `unset __STAGED_EXEC_LINE;` so the seed never reaches the agent. It still carries names only; values ride in the `__STAGED_EXTRA_ENV_<i>` seeds as before. `fish_quote` is gone. A test runs the identical wrapper through every installed shell (sh, bash, zsh, dash, ksh, tcsh, csh here; fish when present) with a `'` and `\` in the binary path and checks the override, the absence of every seed, and a clean exit. Dispatch inverted: an unknown basename now takes the pre-existing stdin path (`-i -l -s`); the `-l -c` wrapper is reserved for an explicit non-POSIX list — fish, nu, xonsh, elvish, tcsh, csh — matched on the basename of `$SHELL` as given. tcsh and csh refuse `-l` next to `-c` ("Unknown option: `-l'"), so they get `-c` alone; fish is the one shell verified end to end, the rest are best effort for shells that never worked on the stdin path either. The doc records that dropping `-i` on this path means Hermit's fish `--on-variable PWD` hook doesn't fire before the exec, so the agent itself (absolute binary path) is unaffected and only the tools it spawns from the project are. Unit tests cover the argv and seed for `bash5`, `zsh-5.9`, an unknown name, an oddly cased one, a `$SHELL` path that doesn't exist (stdin argv; the spawn then fails against the logged shell path), fish, nu, xonsh, elvish, tcsh, csh, the no-extra- env line, and a user variable that clashes with the seed's name. Live tests: the fish test still skips when fish is absent, but setting `STAGED_REQUIRE_FISH_TEST` turns the skip into a failure with a message naming where it looked, for a CI job that installs fish. The zsh/bash history test now runs each shell twice, under its own name and through a symlink named `zsh-5.9`/`bash5` in a temp dir, asserting both take the stdin path and that the agent sees the override while the value lands in no file under HOME. Doctor: `DoctorEnv::vars` and `get` now document the append-order, last-wins invariant that `get`, `apply_doctor_env` and `build_shell_command` all rely on (a name may repeat after `for_check` appends an overlay; the last pair wins because `Command::env` is applied in order), and a test checks that overlaying a name the snapshot already has yields the overlay value from `get`, for one and for two overlays. Verified with `cargo test -p acp-client` (390 passed), `cargo test -p doctor` (156 passed), `cargo fmt --check` and `cargo clippy --tests` on both crates, all clean. Nothing was installed; fish is absent here, so the fish live test skipped and the opt-in failure was exercised by hand. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…ad Doctor env overlays from one preferences read Address the two remaining items of the sixth review of the per-agent environment variables feature (f479dca). Non-POSIX shell list: nu and elvish were listed in `NON_POSIX_LOGIN_SHELLS` and so got the `$SHELL -l -c 'exec /bin/sh -c "$__STAGED_EXEC_LINE"'` wrapper, but neither expands `$VAR` inside a double-quoted string (nushell interpolates only `$"..."` and reads the environment as `$env.X`; elvish uses `$E:X` and never expands inside quotes, and has no `-l`), so `/bin/sh` received the literal text, the agent never started, and the spawn log said the wrapper path was taken, pointing a maintainer at the wrong layer. Both are removed; they now take the pre-existing stdin path and fail at `-s` with the shell's own error, as they always did. The list keeps fish, xonsh, tcsh and csh, and its doc now states the admission rule — accepts `-c` and expands `$VAR` inside double quotes — with nu and elvish named as deliberately excluded and why. The dispatch unit test asserts nu and elvish take the stdin argv with no seed, and the wrapper loop covers xonsh and fish. The `agent::env_vars` module doc no longer cites nu as an example. Doctor overlays: `agent_env_overlays` called `read_agent_env` once per entry in `AI_AGENT_CHECKS`, re-reading and re-parsing `preferences.json` each time, so a UI write landing mid-loop could give two agents' overlays from different versions of the file. `agent::env_vars` now splits the read out: `read_preferences` reads and parses the file once (`None` on any failure, read as "no variables" as before) and `agent_env_from_preferences` derives one agent's enabled pairs from a parsed value; `read_agent_env` composes the two and is unchanged for the session runner, pikchr sub-agents and the login fix. `agent_env_overlays` reads once and derives every check's overlay from that value through `agent_env_overlays_from`, which a new unit test drives with one parsed document holding an enabled row, a disabled-only agent and a key that isn't an agent. Behaviour is otherwise identical: same keys, same order, same filtering. Verified with `cargo test -p acp-client` (390 passed), the Tauri crate's `doctor::` (41) and `agent::env_vars` (8) tests, `cargo fmt --check` on both crates and `cargo clippy` (acp-client with `--tests`; the Tauri crate as CI runs it), all clean. Nothing was installed or removed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f372109ed9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…p, apply them to ACP config discovery Address the three open Codex review threads on PR #1049 (per-agent environment variables). Doctor overlay vs. shell startup (crates/doctor/src/lib.rs:245): the per-check overlay was placed in the login shell's starting environment only, so an `export` of the same name in `.zshenv`/`.zprofile` replaced it before the auth or version probe ran, and the login fix appended the rows to the snapshot with the same exposure. `DoctorEnv` now records an overlay (`with_overlay`, `overlay()`; `for_check` routes a check's overlay through it), and `build_shell_command` carries each overlaid value in a seed variable (`__BUILDERBOT_DOCTOR_OVERLAY_<i>`, prefix grown past any snapshot name that starts with it) and prepends one line per variable to the `-c` payload that re-exports the real name from the seed and unsets the seed, ahead of the existing `PATH` restore so a row named `PATH` keeps the resolved-binary dirs merged into it. Values never enter the command text. Only shell-identifier names get a line: anything else can't be assigned by an rc file either, so it passes through untouched. `ExecuteFixOptions::with_env` takes a `DoctorEnv`, and the login fix builds its environment as `DoctorEnv::new(snapshot).with_overlay(rows)`. Tests: the generated payload and seeds (including the prefix clash and a non-identifier name), the identifier predicate, overlay bookkeeping through `for_check`, and a live run through `/bin/zsh -l -c` with an isolated HOME whose `.zshenv`/`.zprofile` re-export the name, asserting the probe sees the overlay and no seed. The `agent::env_vars` doc no longer lists the rc-file shadowing as an open edge. ACP config discovery (session_runner.rs:1752): the model/effort picker's discovery launch spawned the agent from the working-directory snapshot alone, so an agent authenticated only through a Doctor row answered `session/new` with `AuthRequired`, which discovery reads as "no options". `apply_acp_config_discovery_env` now takes the agent's enabled rows and applies them last, over the snapshot and the env-shebang `PATH` guard, and the inherited-environment fallback applies them over the inherited env, matching `with_extra_env` on a session launch. Unit test added. Error colour (AgentEnvVarsSetting.svelte:274): the inline row error used `var(--destructive, var(--color-danger, #f85149))`, a hardcoded fallback behind two variables the theme doesn't define; it now uses `--ui-danger`, the theme's danger colour, as the other settings panels do. Verified with `cargo test -p doctor` (overlay and shell tests), the Tauri crate's `doctor::`, `agent::env_vars` and new discovery test (50 passed), `cargo fmt --check` on both crates, `cargo clippy -p doctor --tests` (clean), prettier on the Svelte file and the agentEnvVars vitest file. Tauri-crate clippy was skipped under load; CI runs it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The Crates CI "Check" job on 5fe430a failed one acp-client test, `sessions_fail_as_remote_agent_lost_once_bloxlet_reports_a_quiet_prompts_agent_failed`, with the error naming "no longer lists the agent process as running" where the test expects "reports the agent process as failed". Nothing on this branch touches that code; the race is in the test fake and exists identically on main. The fake `sq` serves `sq blox ws commands` from two files the tests rewrite through `FakeSq::set_commands`, which wrote `active.json` and then `history.json`. The proxy's liveness watcher polls every 200 ms in test builds and `remote_liveness::check` runs two sequential `sq` calls: the active list first, and history only when the command is missing from it. A poll landing between the two writes saw the new empty active list, then read history before it had the new FAILED record (the old contents, or an empty file mid-truncation that `parse_commands` rejects), and both read as "no record", so the check reported the agent as no longer listed. The window only opens when the test thread is descheduled between the writes, hence a loaded CI runner. Inserting a 300 ms sleep between the writes reproduced the CI message on every run. The helper now writes `history.json` first. While the active list still says RUNNING no poll reads history, and by the time a poll sees the empty active list the history record is complete. All 29 callers go through the helper; no production code changes. With the 300 ms gap kept in place during verification, all 31 `remote_process` tests passed; they pass again with the gap removed (`cargo test -p acp-client remote_process`, 31 passed) and `cargo fmt --check` is clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a per-agent Environment variables section to Settings → Doctor. Each agent gets an ordered list of
{ name, value, enabled }rows persisted inpreferences.json(agent-env-vars). Local agent launches apply the enabled rows on top of the inherited environment, and Doctor's auth/version probes for that agent see the same overlay, so Doctor's verdict matches what a session launch would get.Frontend (
apps/staged/src/lib/features/settings)AgentEnvVarsSetting.svelte+agentEnvVars.ts: per-agent row editor with masked values, enable/disable toggles, duplicate and invalid-name validation, and a warning (not a refusal) for launcher-reserved names such asPATHandGOOSE_SEARCH_PATHS.preferences.svelte.ts: newagentEnvVarspreference; malformed entries are skipped individually.Backend (
apps/staged/src-tauri)agent::env_varsmodule reads the preference straight frompreferences.json(same pattern asbranch-prefix) and yieldsKEY=VALUEpairs. One read per Doctor pass, so every agent's overlay comes from the same file version.session_runnerand the pikchr MCP sub-agent pass the pairs asextra_env.doctorwires per-check overlays intoDoctorEnv.crates/acp-clientdriverextra_envis applied last in every local spawn path: over the shell snapshot on direct spawns, and on the exec line via/usr/bin/envon the login-shell fallback, so anexportin rc files can't shadow a Doctor value.__STAGED_EXTRA_ENV_<i>seed variables that the exec line unsets before the agent starts.$SHELLbasename: POSIX shells and unknown names keep the existing-i -l -sstdin path; a known non-POSIX list (fish, xonsh, tcsh, csh) gets$SHELL -l -c 'exec /bin/sh -c "$__STAGED_EXEC_LINE"'so the user's rc files still run while the handoff happens in POSIX sh. nu and elvish are deliberately excluded since they don't expand$VARin double quotes.HOME. The fish test skips when fish is absent unlessSTAGED_REQUIRE_FISH_TESTis set.crates/doctorDoctorEnv::with_check_overlay/for_check: per-check variables appended after the snapshot with a documented last-wins invariant shared byget,apply_doctor_envandbuild_shell_command.Notes
preferences.jsonby design (file is already user-private and served to the web UI); the UI says so in the field description.sq blox acpproxy, not the workstation agent.DoctorEnvfields, so this wants a data-model review perAGENTS.md.Test plan
cargo test -p acp-client(390 passed),cargo test -p doctor(156 passed), Tauridoctor::andagent::env_varstestscargo fmt --checkandcargo clippyon both cratesagentEnvVars.test.tsandpreferences.test.ts(vitest)just ciclean on the final push (one earlier run hit a known-flakyattach_cancel_is_unconfirmed_past_its_deadlineSIGINT; passed on rerun)🤖 Generated with Claude Code