feat(shell)!: consolidate env/fs config into nested blocks; hard-reject every removed 0.6.x key - #391
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
skill-check — worker0 verified, 31 skipped (no docs/).
Four for four. Nicely done. |
📝 WalkthroughWalkthroughShell worker 0.7.0 updates configuration parsing, jail-root handling, exec policy, hot-reload rejection handling, CLI boot flow, and related docs/tests. Legacy env and jail-root shapes are replaced with nested ChangesShell worker 0.7.0 config and jail-root refactor
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shell/src/config.rs (1)
623-671: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winReject relative command paths before cwd can retarget them into the jail.
The guard canonicalizes
cmdbeforebuild_commandapplies a per-call jail-confinedcwd. A request with allowlisted basenamels,command: "./ls", andcwdinside the jail can pass this check but execute a jail-planted./ls, bypassing the allowlist’s system-binary intent.Proposed hardening
if cmd.contains('/') { // Unjailed mode (empty host_roots) has NO writable boundary — the @@ if !self.fs.is_jailed() { return Err(format!( @@ )); } + let cmd_path = std::path::Path::new(&cmd); + if !cmd_path.is_absolute() { + return Err(format!( + "relative command path '{}' is not allowed with an fs jail: \ + the effective cwd may be inside the writable jail, allowing \ + agent-planted bytes to bypass the allowlist. Use a bare \ + command name (PATH-resolved) or an absolute path outside the jail.", + cmd + )); + } // Jailed: reject a command path that canonicalizes inside ANY // writable root. A binary planted via shell::fs::write under any // root would otherwise pass the basename allowlist and execute — // host RCE. All roots are writable, so all must be checked. - if let Ok(canon_cmd) = std::fs::canonicalize(&cmd) { - for root in self.fs.roots() { - if let Ok(canon_root) = std::fs::canonicalize(&root) { - if canon_cmd.starts_with(&canon_root) { - return Err(format!( - "command path '{}' resolves inside the writable fs jail ({}); \ - executing files written via shell::fs::write is not allowed. \ - Use a bare command name (PATH-resolved) or a path outside the jail.", - cmd, - root.display() - )); - } + let canon_cmd = std::fs::canonicalize(cmd_path).map_err(|_| { + format!( + "command path '{}' could not be canonicalized; use a bare command name \ + (PATH-resolved) or an existing absolute path outside the jail.", + cmd + ) + })?; + for root in self.fs.roots() { + if let Ok(canon_root) = std::fs::canonicalize(&root) { + if canon_cmd.starts_with(&canon_root) { + return Err(format!( + "command path '{}' resolves inside the writable fs jail ({}); \ + executing files written via shell::fs::write is not allowed. \ + Use a bare command name (PATH-resolved) or a path outside the jail.", + cmd, + root.display() + )); } } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shell/src/config.rs` around lines 623 - 671, The command-path confinement check in the config validation logic should also reject relative paths like "./ls" or "../bin/ls" before `build_command` can apply a jail-confined `cwd`. Update the existing `cmd.contains('/')` handling in `shell::config::Config` so it distinguishes absolute paths from relative ones and refuses any relative command path, while still allowing bare PATH-resolved names and the existing outside-jail absolute-path behavior. Reference the `canonicalize`-based check and the `self.fs.is_jailed()` branch to keep the fix aligned with the current command allowlist enforcement.
🧹 Nitpick comments (1)
shell/src/exec/host.rs (1)
404-440: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale doc paragraph duplicated on
EnvVarGuard.Lines 404-411 describe the pre-mutex design ("residual risk to a logical leak, not a data race... would need a process-wide env mutex... a larger change than this test warrants") while lines 412-420 describe the actual, now-implemented mutex-based design. The first paragraph appears to be leftover from before
ENV_TEST_MUTEXwas added and now contradicts the second. Worth trimming to just the current (mutex) doc block for clarity.📝 Proposed cleanup
- /// Removes the named process env vars on drop, even if the test body - /// panics between `set_var` and where a plain cleanup call would have - /// run — an assertion failure without this leaks the var into every - /// later test in the binary. Unique per-test var names (see the caller) - /// keep the residual risk to a logical leak, not a data race: full - /// concurrent-mutation safety would need a process-wide env mutex shared - /// with `code/config.rs`'s `CODER_TEST_ROOT` test, a larger change than - /// this test warrants on its own. /// Holds [`crate::config::ENV_TEST_MUTEX`] for its whole lifetime AND /// removes the named process env vars on drop, even if the test body /// panics between `set_var` and where a plain cleanup call would have /// run. The mutex serializes against every other test in the crate that /// touches process env (see that constant's doc comment for why); the /// Drop cleanup handles the panic case the mutex alone doesn't cover. /// Held across `.await` below — fine under the default `#[tokio::test]` /// current-thread flavor, where the outer test future need not be /// `Send`. struct EnvVarGuard {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shell/src/exec/host.rs` around lines 404 - 440, The doc comment on EnvVarGuard still contains an outdated pre-mutex paragraph that contradicts the current behavior. Remove the stale text in the EnvVarGuard docs and keep only the description that matches the implemented ENV_TEST_MUTEX locking and Drop-based cleanup, so the comment accurately reflects EnvVarGuard::new and its lifetime behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shell/CHANGELOG.md`:
- Around line 27-43: The changelog entry currently says coder-only installs can
boot via ShellConfig::seed_default, which understates the migration behavior and
implies a permissive fallback. Update the 0.7.0 migration note around
ShellConfig::seed_default and register_config to say that a legacy standalone
coder entry must not silently fall through; either keep the migration bridge
from migrate_legacy_coder or state that startup should fail closed until a shell
value exists. Make the two upgrade cases explicit and remove any wording that
suggests permissive seeding for coder-only state.
In `@shell/skills/SKILL.md`:
- Line 51: Update the wording in SKILL.md to reflect that shell::fs::* does
canonicalize and resolve existing symlink segments during jail validation;
replace the claim that “symlinks are never followed” with language that says
access is restricted to cfg.fs.host_roots and denylisted paths are refused,
while existing symlink segments may be resolved during validation.
In `@shell/src/config.rs`:
- Around line 103-109: Update the schema comment on the folded `code` surface
docs to match `check_removed_keys` behavior: the removed `code.base_path` and
`code.base_paths` keys are not merely ignored, they are rejected with a
migration hint. Adjust the wording near the `coder::*` config description so it
explicitly says these stored values fail closed / are rejected, while keeping
the note that `fs.host_roots` supplies the resolver roots.
In `@shell/src/main.rs`:
- Around line 129-131: Redact the raw engine URL from the startup log in main by
updating the tracing::info! call near Cli::parse so it no longer prints cli.url
directly. Use the existing probe_engine_reachable flow as a guide and log only
host/port (or another sanitized form) for the engine endpoint, while keeping the
rest of the boot context such as seed_config intact.
- Around line 154-171: The seed-loading branch in main currently uses
Path::exists(), which can misclassify unreadable or permission-denied config
files as missing and fall back to the built-in default; update the config check
in the ShellConfig::from_file flow to use try_exists() instead, and make the
Err(_) case fail closed by bailing rather than returning None. Keep the existing
handling around cli.config, the tracing::warn!/bail! paths, and the seed match
structure, but ensure only a confirmed non-existent file is treated as absent.
---
Outside diff comments:
In `@shell/src/config.rs`:
- Around line 623-671: The command-path confinement check in the config
validation logic should also reject relative paths like "./ls" or "../bin/ls"
before `build_command` can apply a jail-confined `cwd`. Update the existing
`cmd.contains('/')` handling in `shell::config::Config` so it distinguishes
absolute paths from relative ones and refuses any relative command path, while
still allowing bare PATH-resolved names and the existing outside-jail
absolute-path behavior. Reference the `canonicalize`-based check and the
`self.fs.is_jailed()` branch to keep the fix aligned with the current command
allowlist enforcement.
---
Nitpick comments:
In `@shell/src/exec/host.rs`:
- Around line 404-440: The doc comment on EnvVarGuard still contains an outdated
pre-mutex paragraph that contradicts the current behavior. Remove the stale text
in the EnvVarGuard docs and keep only the description that matches the
implemented ENV_TEST_MUTEX locking and Drop-based cleanup, so the comment
accurately reflects EnvVarGuard::new and its lifetime behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52cf6cbc-781f-48c1-af1b-ad397d84301e
⛔ Files ignored due to path filters (1)
shell/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (34)
shell/ARCHITECTURE.mdshell/CHANGELOG.mdshell/Cargo.tomlshell/README.mdshell/config.collect.yamlshell/config.yamlshell/skills/SKILL.mdshell/src/code/config.rsshell/src/code/path.rsshell/src/config.rsshell/src/configuration.rsshell/src/exec/host.rsshell/src/exec/policy.rsshell/src/fs/host.rsshell/src/fs/mod.rsshell/src/functions/exec.rsshell/src/functions/exec_bg.rsshell/src/functions/kill.rsshell/src/functions/types.rsshell/src/functions/workspace.rsshell/src/main.rsshell/src/path/mod.rsshell/tests/code_golden_errors.rsshell/tests/e2e/.gitignoreshell/tests/e2e/README.mdshell/tests/e2e/config-jailed.yamlshell/tests/e2e/config.yamlshell/tests/e2e/run-tests-jailed.shshell/tests/e2e/run-tests.shshell/tests/e2e/workers/harness/src/cases-safety.tsshell/tests/e2e/workers/harness/src/cases-vuln-repro-jailed.tsshell/tests/e2e/workers/harness/src/cases-vuln-repro.tsshell/tests/e2e/workers/harness/src/runner.tsshell/tests/golden/errors.json
Comment on lines
+27
to
+43
| - **The one-shot coder→shell config migration is removed** | ||
| (`migrate_legacy_coder`), and the hidden `migrated_from_coder` marker | ||
| field is REJECTED at parse, not silently tolerated. 0.7.0 no longer folds | ||
| a legacy standalone-`coder` configuration entry into the `shell` value at | ||
| boot, and boot no longer probes `configuration::get` for a `coder` entry | ||
| — which also removes the boot-time "configuration 'coder' not found" WARN | ||
| retries. Two distinct upgrade scenarios: | ||
| - An install that ALREADY has a `shell` entry (it went through the fold | ||
| under 0.6.x, so that entry carries `migrated_from_coder: true`) now | ||
| fails closed at 0.7.0 boot with a migration hint, instead of silently | ||
| parsing past the marker. | ||
| - An install with ONLY a standalone `coder` entry and NO `shell` entry at | ||
| all has nothing to reject — `register_config` still seeds the generic | ||
| permissive `/tmp` dev default for `shell`, silently, because there is no | ||
| stored `shell` value to fail closed on. Boot 0.6.x once first (it | ||
| performs the fold and writes the `shell` entry) before upgrading to | ||
| 0.7.0 to avoid this. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Do not let coder-only upgrades fall through to the permissive seed.
As written, a legacy install with only a coder entry and no shell value will boot 0.7.0 from ShellConfig::seed_default() instead of failing closed or preserving the old policy. That is a real security regression for a hard-migration release.
If that state can still exist in the field, keep the migration bridge or reject startup until the shell value is populated.
Also applies to: 136-143
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shell/CHANGELOG.md` around lines 27 - 43, The changelog entry currently says
coder-only installs can boot via ShellConfig::seed_default, which understates
the migration behavior and implies a permissive fallback. Update the 0.7.0
migration note around ShellConfig::seed_default and register_config to say that
a legacy standalone coder entry must not silently fall through; either keep the
migration bridge from migrate_legacy_coder or state that startup should fail
closed until a shell value exists. Make the two upgrade cases explicit and
remove any wording that suggests permissive seeding for coder-only state.
| any allowlisted interpreter. Run untrusted commands with `target: sandbox` | ||
| (needs `iii-sandbox`). | ||
| - `shell::fs::*` is jailed to `cfg.fs.host_root` and refuses denylisted paths; | ||
| - `shell::fs::*` is jailed to `cfg.fs.host_roots` and refuses denylisted paths; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix the symlink wording.
The backend does canonicalize and resolve existing symlink segments during jail validation, so “symlinks are never followed” is too strong and misleading.
♻️ Suggested wording
- `shell::fs::*` is jailed to `cfg.fs.host_roots` and refuses denylisted paths; paths must be absolute and symlinks are never followed.
+ `shell::fs::*` is jailed to `cfg.fs.host_roots` and refuses denylisted paths; paths are canonicalized, and symlink escapes are rejected.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - `shell::fs::*` is jailed to `cfg.fs.host_roots` and refuses denylisted paths; | |
| - `shell::fs::*` is jailed to `cfg.fs.host_roots` and refuses denylisted paths; paths are canonicalized, and symlink escapes are rejected. |
🧰 Tools
🪛 SkillSpector (2.3.7)
[error] 6: [TM1] Tool Parameter Abuse: Tool parameters are crafted to achieve unintended or unsafe behavior. Parameter abuse can bypass intended safety checks (e.g. shell=True, --force, dangerous glob patterns).
Remediation: Validate all tool parameters against an allowlist. Reject dangerous parameter values (shell=True, --force, -rf /) and use safe defaults.
(Tool Misuse (TM1))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shell/skills/SKILL.md` at line 51, Update the wording in SKILL.md to reflect
that shell::fs::* does canonicalize and resolve existing symlink segments during
jail validation; replace the claim that “symlinks are never followed” with
language that says access is restricted to cfg.fs.host_roots and denylisted
paths are refused, while existing symlink segments may be resolved during
validation.
Comment on lines
129
to
+131
| let cli = Cli::parse(); | ||
| tracing::info!(url = %cli.url, seed_config = %cli.config, "connecting to IIIClient engine"); | ||
| probe_engine_reachable(&cli.url); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact the engine URL in boot logs.
Line 130 logs the raw --url, but the probe comment notes that wss:// URLs can embed credentials. Log host/port only here too.
🛡️ Proposed fix
- tracing::info!(url = %cli.url, seed_config = %cli.config, "connecting to IIIClient engine");
+ if let Some((host, port)) = ws_host_port(&cli.url) {
+ tracing::info!(host = %host, port, seed_config = %cli.config, "connecting to IIIClient engine");
+ } else {
+ tracing::info!(seed_config = %cli.config, "connecting to IIIClient engine");
+ }
probe_engine_reachable(&cli.url);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shell/src/main.rs` around lines 129 - 131, Redact the raw engine URL from the
startup log in main by updating the tracing::info! call near Cli::parse so it no
longer prints cli.url directly. Use the existing probe_engine_reachable flow as
a guide and log only host/port (or another sanitized form) for the engine
endpoint, while keeping the rest of the boot context such as seed_config intact.
Comment on lines
154
to
+171
| let seed = match config::ShellConfig::from_file(&cli.config) { | ||
| Ok(cfg) => { | ||
| tracing::info!(path = %cli.config, "loaded seed config for initial registration"); | ||
| Some(cfg) | ||
| } | ||
| Err(e) => { | ||
| tracing::warn!(path = %cli.config, error = %e, "could not load --config seed; using the stored configuration value if present, else the built-in zero-config default"); | ||
| Err(e) if !std::path::Path::new(&cli.config).exists() => { | ||
| tracing::warn!(path = %cli.config, error = %e, "no --config seed file; using the stored configuration value if present, else the built-in zero-config default"); | ||
| None | ||
| } | ||
| Err(e) => { | ||
| anyhow::bail!( | ||
| "--config seed {} exists but failed to parse: {e}. Refusing to boot: falling \ | ||
| back would seed the permissive built-in default in place of the intended \ | ||
| policy. Fix the file (see the migration hint above) or remove it to opt \ | ||
| into the zero-config default.", | ||
| cli.config | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='shell/src/main.rs'
wc -l "$file"
sed -n '120,210p' "$file" | cat -nRepository: iii-hq/workers
Length of output: 4722
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "fn from_file|impl .*ShellConfig|ShellConfig::from_file" shell src . -g '!target' -g '!node_modules'Repository: iii-hq/workers
Length of output: 1974
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='shell/src/config.rs'
wc -l "$file"
sed -n '680,760p' "$file" | cat -nRepository: iii-hq/workers
Length of output: 4084
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "rust-version|toolchain|edition" Cargo.toml **/Cargo.toml rust-toolchain* .github/workflows -g '!target' -g '!node_modules'Repository: iii-hq/workers
Length of output: 2542
Fail closed when --config existence can’t be checked. Path::exists() can collapse metadata/permission errors into “missing,” so an unreadable intended seed file falls through to the permissive built-in default. Use try_exists() and bail on Err(_) instead of treating it as absent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shell/src/main.rs` around lines 154 - 171, The seed-loading branch in main
currently uses Path::exists(), which can misclassify unreadable or
permission-denied config files as missing and fall back to the built-in default;
update the config check in the ShellConfig::from_file flow to use try_exists()
instead, and make the Err(_) case fail closed by bailing rather than returning
None. Keep the existing handling around cli.config, the tracing::warn!/bail!
paths, and the seed match structure, but ensure only a confirmed non-existent
file is treated as absent.
This was referenced Jul 3, 2026
BREAKING CHANGE: the top-level inherit_env and allowed_env config keys are replaced by env.inherit / env.allow, with no legacy aliases. The old keys are rejected at parse with a migration hint — serde would otherwise ignore them and silently boot with env forwarding off. A stored configuration value carrying the old keys fails closed at boot; rewrite it via configuration::set (id: shell) after deploying the new binary. Also: - --version flag; --url documented in --help including the III_URL env var - pre-connect reachability probe: one loud ERROR with a fix hint when the engine is unreachable, before the SDK's silent 2s-backoff retry loop - schema descriptions on every operator-visible config field (incl. the nested env/fs/sandbox blocks) so the console config UI documents each knob inline; pinned by a unit test - README Running section documenting the binary's operator surface (--config, --url/III_URL, --version, RUST_LOG); ARCHITECTURE CLI table and defaults table updated, code-defaults vs shipped-seed distinction spelled out - from_yaml re-deserializes from text after the removed-key check so unquoted boolean-like strings (allowlist: [false]) keep parsing; regression-tested - e2e fixtures migrated to the nested env block; tests/e2e/config/ (engine- externalized runtime state) gitignored and the tracked fixtures kept self-contained
…prefer host_roots, describe every knob Applies the config.yaml review findings: - Anchor mkfs/dd/shutdown/reboot denylist patterns to argv[0] (^(\S*/)?name) so they fire when the tool IS the command, not when the word appears in an argument — 'grep -rn shutdown src/' no longer rejected. rm -rf /, the fork bomb, and /etc/shadow stay unanchored (argument-shaped by nature). Pinned by new allow/deny test cases. - Reword the denylist rejection to say it is an advisory tripwire and to rephrase, so agents stop retrying verbatim. - Seed uses the preferred fs.host_roots: [/tmp] (legacy host_root form dropped from the shipped example). - Seed default_timeout_ms 10s -> 30s (code default unchanged): the seed already raises max_timeout_ms to 120s for real builds; callers omitting timeout_ms shouldn't be reaped at 10s on the same workload. - fs.max_read/write_bytes schema descriptions explain why the code default is unlimited (streaming; the cap bounds caller cost, not worker memory); seed comments mark fs.denylist_paths as defense in depth and explain the passwd/shadow exec-side asymmetry. - Doc-comment every remaining CoderConfig budget field; the schema description test now sweeps ALL nested definitions.
…base_path(s), coder migration fold Rides the 0.7.0 breaking window (stored values already need a rewrite): - fs.host_root (0.6.x single-root alias) removed. Rejected at parse with a migration hint (fs.host_root -> fs.host_roots, one-entry list), same fail-closed rationale as the env rename: serde would silently ignore the stale key and the worker would see no jail configured. FsConfig::roots() and is_jailed() lose the legacy branch; the both-keys-set config error disappears with the alias. - code.base_path removed and code.base_paths taken off the wire (serde+schemars skip). They were inert: the code resolver has always taken its roots from fs.host_roots via code_resolver_config, so stored values still carrying them are silently ignored — no reject, they never had an effect. - The one-shot coder->shell config migration is retired (migrate_legacy_coder + hidden migrated_from_coder marker). Boot no longer probes configuration::get for 'coder', which also removes the "configuration 'coder' not found" WARN retries from every boot. Stacks that still need the fold should boot 0.6.x once before upgrading. Tests: legacy-alias boot test inverted into a rejection test; new yaml/json fs.host_root rejection tests; golden C210 case regenerated (no-reachable-roots replaces both-root-forms). 1270 unit tests, e2e 171/171 + jailed 2/2.
base_paths is serde-skipped since 0.7.0, so code_resolver_config is the SOLE source of coder::* roots — a regression dropping the fill would silently jail the code surface to the default ['./', '/tmp'], wider than the operator's fs jail.
…ylist hardening, half-migration rejection Applies findings from a specialist + red-team + Claude/Codex adversarial review pass over the env-consolidation branch: - Seed-file parse failures now abort boot instead of silently seeding the permissive built-in default in place of the operator's intended policy — every un-migrated 0.6.x --config file now hits this path. A genuinely missing file still falls through gracefully. - Hot-reload no longer misclassifies a fetched-but-unparseable stored value as a transient fetch error (infinite retry against bytes that can never parse); it is now Rejected — keep last-good, ack, record for shell::config-status — the same treatment as an unbuildable config. reload_serialized's fetch closure now returns the raw fetched Value so parsing happens inside the classification, not before it. - EnvConfig denies unknown fields, and the removed-key checker was unified into one function covering the top-level, fs, and env objects in a single traversal — a half-migration (old key names nested under the new env: block) now gets the same friendly hint as every other removed key, instead of a generic serde error. Consolidating the two separate checkers surfaced a real bug: the original used .any(), which short-circuits, so a config carrying both removed env keys only ever named the first in its error; now collects every hit. - The anchored, wrapper-tolerant denylist patterns are now case-insensitive and handle env's idiomatic KEY=VALUE form (env FOO=bar shutdown bypassed the tripwire; only bare env shutdown was covered before). - The boot-time reachability probe runs on a detached thread so its unbounded DNS resolution and bounded TCP connects can never delay boot, and logs host:port instead of the raw URL (a wss://user:pass@host URL could otherwise leak credentials to the log). - cargo fmt violations, an EnvConfig test-fixture helper to deduplicate four near-identical constructions, and a cross-test mutex to fix a real intermittent flake (two tests mutating process env could race on separate cargo-test threads). 1278 unit tests, e2e 171/171 + jailed 2/2, cargo fmt/clippy clean.
Adds a Fixed section to the 0.7.0 CHANGELOG entry covering the seed/reload fail-closed fixes, the half-migration rejection, the wrapper-tolerant denylist case-insensitivity and env KEY=VALUE coverage, and the flaky-test fix. Strengthens the coder-migration-removal wording in both files to spell out the concrete consequence of skipping the boot-0.6.x-first escape hatch (silent seed to the generic /tmp default) rather than only naming the removal. Updates the Running section's probe description to match the detached-thread, host:port-only logging behavior.
…migration, no legacy tolerance
The previous commits silently ignored two removed 0.7.0 keys instead of
rejecting them: code.base_path/base_paths (never had a runtime effect,
so 'harmless to keep' seemed reasonable) and the migrated_from_coder
marker (pure internal bookkeeping, never operator-set). Per explicit
direction: this branch does a hard migration with zero legacy-support
code, no exceptions for keys that happen to be inert or invisible to
the operator.
check_removed_keys is restructured around a RemovedKey{old, new: Option}
type so pure removals (no replacement — just delete the key) share the
same table-driven scan as renames. Both new cases get the same
hard-fail treatment and migration hint as inherit_env/allowed_env/
fs.host_root: named in the error, pointed at configuration::set.
Corrects the CHANGELOG/README claims accordingly, and is precise about
what this does and doesn't fix: an install that already has a stored
shell entry (went through the 0.6.x coder fold, carries
migrated_from_coder: true) now fails closed at boot instead of silently
parsing past the marker. An install with ONLY a standalone coder entry
and no shell entry at all still seeds the generic /tmp default
silently — there's nothing stored to reject in that case, so the
boot-0.6.x-first escape hatch remains the only fix for that scenario.
1282 unit tests (4 new), e2e 171/171 + jailed 2/2, fmt/clippy clean.
…path values Revises comments in ShellConfig and CoderConfig to specify that stored values carrying removed keys like code.base_path are rejected during parsing, enhancing clarity on migration behavior. Removes outdated test for ignored base_path values, aligning with the hard migration approach established in previous commits.
ytallo
force-pushed
the
feat/shell-env-config
branch
from
July 3, 2026 16:19
6c69b43 to
ca935e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shell/README.md`:
- Around line 269-275: The README note for coder-only upgrades currently
describes a permissive boot path that widens access instead of failing closed.
Update the guidance around the standalone coder entry / missing shell entry case
to state that boot must be rejected or an explicit migration must run before
0.7.0 starts, and make it clear that the shell entry must be present in the
migrated state so old roots and protected globs are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d3570f6-2c33-45ed-aa72-9b74aff2f830
⛔ Files ignored due to path filters (1)
shell/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
shell/ARCHITECTURE.mdshell/CHANGELOG.mdshell/Cargo.tomlshell/README.mdshell/config.collect.yamlshell/config.yamlshell/skills/SKILL.mdshell/src/code/config.rsshell/src/code/functions/info.rsshell/src/code/path.rsshell/src/config.rsshell/src/configuration.rsshell/src/exec/host.rsshell/src/exec/policy.rsshell/src/fs/host.rsshell/src/fs/mod.rsshell/src/functions/exec.rsshell/src/functions/exec_bg.rsshell/src/functions/kill.rsshell/src/functions/types.rsshell/src/functions/workspace.rsshell/src/main.rsshell/src/path/mod.rsshell/tests/code_golden_errors.rsshell/tests/e2e/.gitignoreshell/tests/e2e/README.mdshell/tests/e2e/config-jailed.yamlshell/tests/e2e/config.yamlshell/tests/e2e/run-tests-jailed.shshell/tests/e2e/run-tests.shshell/tests/e2e/workers/harness/src/cases-safety.tsshell/tests/e2e/workers/harness/src/cases-vuln-repro-jailed.tsshell/tests/e2e/workers/harness/src/cases-vuln-repro.tsshell/tests/e2e/workers/harness/src/runner.tsshell/tests/golden/errors.json
💤 Files with no reviewable changes (1)
- shell/src/code/functions/info.rs
✅ Files skipped from review due to trivial changes (16)
- shell/tests/e2e/workers/harness/src/runner.ts
- shell/tests/e2e/run-tests.sh
- shell/config.collect.yaml
- shell/src/functions/workspace.rs
- shell/src/functions/kill.rs
- shell/tests/e2e/workers/harness/src/cases-vuln-repro.ts
- shell/src/functions/exec.rs
- shell/src/path/mod.rs
- shell/tests/golden/errors.json
- shell/tests/e2e/workers/harness/src/cases-vuln-repro-jailed.ts
- shell/tests/e2e/README.md
- shell/src/fs/mod.rs
- shell/src/functions/types.rs
- shell/tests/e2e/run-tests-jailed.sh
- shell/tests/e2e/workers/harness/src/cases-safety.ts
- shell/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (15)
- shell/tests/e2e/config.yaml
- shell/tests/e2e/config-jailed.yaml
- shell/src/functions/exec_bg.rs
- shell/tests/e2e/.gitignore
- shell/tests/code_golden_errors.rs
- shell/Cargo.toml
- shell/ARCHITECTURE.md
- shell/src/main.rs
- shell/config.yaml
- shell/src/exec/host.rs
- shell/src/code/path.rs
- shell/src/exec/policy.rs
- shell/src/fs/host.rs
- shell/src/config.rs
- shell/src/configuration.rs
Comment on lines
+269
to
+275
| - An install with ONLY a standalone `coder` entry and NO `shell` entry at | ||
| all has nothing to reject — it still boots 0.7.0 with the generic | ||
| permissive `/tmp` dev seed for `shell`, silently, because there is no | ||
| stored `shell` value to fail closed on. The old `coder` roots and | ||
| protected globs are NOT carried over. Boot 0.6.x once first (it performs | ||
| the migration and writes the `shell` entry) before upgrading to 0.7.0 to | ||
| avoid this. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- shell/README.md excerpt ---\n'
sed -n '240,290p' shell/README.md
printf '\n--- search for coder/shell migration references ---\n'
rg -n "coder.*shell|shell.*coder|migration|dev seed|/tmp" shell -SRepository: iii-hq/workers
Length of output: 243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf -- '--- shell/README.md excerpt ---\n'
sed -n '240,290p' shell/README.md
printf -- '\n--- search for coder/shell migration references ---\n'
rg -n "coder.*shell|shell.*coder|migration|dev seed|/tmp" shell -SRepository: iii-hq/workers
Length of output: 29396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf -- '--- shell/src/main.rs around config registration ---\n'
sed -n '120,220p' shell/src/main.rs
printf -- '\n--- shell/src/config.rs around migration and seed default ---\n'
sed -n '120,240p' shell/src/config.rs
printf -- '\n--- shell/src/config.rs seed/default tests/comments ---\n'
sed -n '480,560p' shell/src/config.rs
printf -- '\n--- shell/src/configuration.rs around fallback behavior ---\n'
sed -n '180,240p' shell/src/configuration.rs
sed -n '470,520p' shell/src/configuration.rsRepository: iii-hq/workers
Length of output: 19426
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf -- '--- configuration registration code ---\n'
rg -n "register_config|CONFIG_ID|coder entry|standalone-`coder`|migrated_from_coder|configuration::get" shell/src shell/tests -S
printf -- '\n--- relevant config docs/comments ---\n'
sed -n '1,140p' shell/ARCHITECTURE.md
sed -n '1,120p' shell/CHANGELOG.mdRepository: iii-hq/workers
Length of output: 18915
Fail closed on coder-only upgrades. An install with only a standalone coder entry still boots from the permissive /tmp seed and drops the old roots/protected globs. Require an explicit migration or reject boot instead of widening access.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shell/README.md` around lines 269 - 275, The README note for coder-only
upgrades currently describes a permissive boot path that widens access instead
of failing closed. Update the guidance around the standalone coder entry /
missing shell entry case to state that boot must be rejected or an explicit
migration must run before 0.7.0 starts, and make it clear that the shell entry
must be present in the migrated state so old roots and protected globs are
preserved.
5 tasks
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
1 participant
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
Consolidates the shell worker's environment-variable config into a nested
envblock, and turns every removed 0.6.x key into a hard, parse-time rejection — no legacy aliases, no silent tolerance, no soft transition period anywhere in the schema.Breaking, all with a migration hint at parse time:
inherit_env/allowed_env→env.inherit/env.allow. A half-migration (old names nested under the newenv:block) is caught too —EnvConfigdenies unknown fields.fs.host_root(single-root alias) →fs.host_roots(list).code.base_path/code.base_pathsremoved from the schema entirely. These never had a runtime effect (the code resolver has always taken its roots fromfs.host_roots), but "harmless to keep" isn't an exception — a stored value carrying either now fails closed like everything else.migrate_legacy_coder) is gone, including the hiddenmigrated_from_codermarker field, which is now rejected at parse rather than silently ignored. An install that already has ashellentry (went through the 0.6.x fold) fails closed at boot with a hint. An install with only a standalonecoderentry has nothing to reject and will still seed the generic/tmpdev default — boot 0.6.x once first to avoid that.All four removed-key checks are driven by one table-scanning function (
check_removed_keys) instead of ad hoc per-case logic, so adding a future removed key means one new table entry, not a new code path.Hardening riding along:
mkfs,dd,shutdown,reboot) now tolerate a bounded wrapper prefix (sudo,doas,nohup,timeout [duration], orenvwith its idiomaticKEY=VALUE...form) and are case-insensitive —sudo shutdown -h nowandenv FOO=bar shutdown -h nowtrip the tripwire;grep -rn shutdown src/still doesn't. Known remaining gap, documented rather than "fixed": flags interposed before the wrapped command (sudo -u root shutdown) still evade it — closing that needs per-argv-token wrapper-skipping, not a joined-string regex.--config) file that exists but fails to parse now aborts boot instead of silently falling back to the permissive built-in default in place of the operator's intended policy.Rejected— same treatment as an unbuildable config, recorded forshell::config-status.wss://user:pass@hostURL can embed credentials.--version, and--url/III_URL/RUST_LOGdocumented in--help/README.Known gaps (deliberate, not fixed here)
coder::*keeps itsPathResolveracross a hot-reload that changesfs.host_roots— this is pre-existing, restart-required-by-design architecture (ARCHITECTURE.md,jail_signature), unrelated to and not worsened by this branch. Worth its own follow-up.iii-runningandharnessdev stacks is deferred until this merges — both have stored configs carrying the removed keys and need aconfiguration::setrewrite in the correct order (deploy binary first, then rewrite the value — see README "Upgrading to 0.7.0" for the exact sequencing trap).Test plan
cargo testinshell/— 1282 passedcargo fmt --check/cargo clippy --all-targets— cleantests/e2e/run-tests.sh— 171/171tests/e2e/run-tests-jailed.sh— 2/2iii-running+harnessdev stacks (deferred until merge, see README)Summary by CodeRabbit
--versionand improved CLI URL handling with a pre-boot reachability probe.envpolicies and jail setup viafs.host_roots.cwd/envvalidation and denylist matching, including wrapper/case-tolerant command patterns.