feat(shell)!: consolidate env/fs config into nested blocks; hard-reject every removed 0.6.x key

shell/ARCHITECTURE.md

@@ -17,7 +17,7 @@ cargo build --release --bin iii-shell
 mkdir -p ~/.iii/workers
 ln -sfn $(pwd)/target/release/iii-shell ~/.iii/workers/shell
 
-# 4. Start the engine (it spawns the worker). Pin a host_root or set
+# 4. Start the engine (it spawns the worker). Pin fs.host_roots or set
 #    fs.allow_unjailed: true in config.yaml first — the worker refuses to
 #    start unjailed by default.
 iii -c ./config.yaml
@@ -30,7 +30,10 @@ iii -c ./config.yaml
 | flag | default | purpose |
 |------|---------|---------|
 | `--config <path>` | `./config.yaml` | Optional seed config: the YAML is passed as `initial_value` when registering the schema with the `configuration` worker on first boot. It is **not** the live source of truth — the live value is fetched over RPC after registration. When the file is absent and nothing is stored yet, the worker seeds a built-in zero-config default (`ShellConfig::seed_default()`, jailed to `/tmp`) instead. |
-| `--url <ws-url>` | `ws://127.0.0.1:49134` | iii engine WebSocket |
+| `--url <ws-url>` | `ws://127.0.0.1:49134` | iii engine WebSocket. Also read from the `III_URL` env var (the flag wins). A pre-connect probe logs one ERROR with a fix hint when the engine is unreachable; the SDK then retries forever with a 2s backoff. |
+| `--version` | — | print the worker version |
+
+Logging is controlled by the `RUST_LOG` env var (tracing `EnvFilter` syntax; default `info`).
 
 ## Configuration
 
@@ -40,26 +43,31 @@ The shell worker integrates with the central `configuration` worker rather than
 2. It immediately fetches the live value over RPC and activates the security policy and fs backend from that response.
 3. It then registers the `configuration:updated` trigger and runs a **fail-closed** boot reconcile before exposing any public function. The reconcile re-fetches the authoritative value (closing the race where an update lands between the initial fetch and trigger registration, leaving no listener). If that re-fetch fails the worker aborts startup — it exits rather than serve a possibly stale security policy, and no `shell::*` / `shell::fs::*` function is ever exposed.
 4. It subscribes to `configuration:updated` events. When the config for schema id `shell` changes, the worker hot-reloads the security policy and fs backend atomically.
-5. If the incoming config is invalid or unsafe (e.g. schema validation passes but the worker cannot build it — bad denylist regex, unreachable `host_root`), the worker keeps the last-good runtime and logs an error — it does **not** crash, and it does **not** retry (re-fetching returns the same bad value, so a retry would storm). The rejection is recorded and surfaced by `shell::config-status` (a `rejected` outcome with a non-zero `rejected_reloads` count) so the divergence between the central store and the enforced policy is detectable instead of silent.
-6. A reload that widens the jail (clearing `host_root`) succeeds, but is logged as a privilege change.
+5. If the incoming config is invalid or unsafe (e.g. schema validation passes but the worker cannot build it — bad denylist regex, unreachable jail root), the worker keeps the last-good runtime and logs an error — it does **not** crash, and it does **not** retry (re-fetching returns the same bad value, so a retry would storm). The rejection is recorded and surfaced by `shell::config-status` (a `rejected` outcome with a non-zero `rejected_reloads` count) so the divergence between the central store and the enforced policy is detectable instead of silent.
+6. A reload that widens the jail (clearing `host_roots`) succeeds, but is logged as a privilege change.
 
 ## Full YAML defaults
 
+These are the CODE defaults (`ShellConfig::default()` — fail-closed: `env.inherit
+false`, unjailed refused). The shipped seed `config.yaml` / `seed_default()` is
+deliberately more permissive for dev use: `env.inherit true`, jailed to `/tmp`,
+`max_timeout_ms 120000`, catastrophic-only denylist.
+
 | key | default | enforced where |
 |-----|---------|----------------|
 | `max_timeout_ms` | `30000` | foreground `exec` hard cap; per-call `timeout_ms` clamped to this |
 | `max_bg_timeout_ms` | `0` | host bg job hard cap in ms; `0` = unbounded (separate from `max_timeout_ms`, which bounds foreground exec) |
 | `default_timeout_ms` | `10000` | applied when caller omits `timeout_ms` |
 | `max_output_bytes` | `1048576` (1 MiB) | stdout/stderr truncated; `*_truncated` flagged |
 | `working_dir` | `null` | pins cwd for spawned commands when set |
-| `inherit_env` | `false` | when `false`, only `allowed_env` keys are forwarded |
-| `allowed_env` | `[PATH, HOME, LANG, LC_ALL, TERM]` | env passthrough allowlist |
+| `env.inherit` | `false` | forward the worker's FULL env to children; when `false`, only `env.allow` keys are forwarded |
+| `env.allow` | `[PATH, HOME, LANG, LC_ALL, TERM]` | dual role: forwarding allowlist when `env.inherit` is false, AND the per-call `env` settable gate (minus the hardcoded dangerous keys, which are never settable) |
 | `allowlist` | `[]` (open) | command basename allowlist; empty = open |
 | `denylist_patterns` | `[]` | advisory regex tripwire on `argv.join(" ")` |
 | `max_concurrent_jobs` | `16` | rejects new `exec_bg` past the cap |
 | `job_retention_secs` | `3600` | finished jobs evicted by a background reaper (interval `min(30s, retention/2)`) — the primary prune path; prune-on-`shell::list` remains as a harmless secondary trigger |
-| `fs.host_root` | `null` | jail root; required unless `fs.allow_unjailed: true` |
-| `fs.allow_unjailed` | `false` | explicit opt-in to running with `host_root: null` |
+| `fs.host_roots` | `[]` | jail roots; first = primary; required non-empty unless `fs.allow_unjailed: true` |
+| `fs.allow_unjailed` | `false` | explicit opt-in to running with an empty `host_roots` |
 | `fs.max_read_bytes` | `0` (unlimited) | pre-flight cap via `fs::metadata` (`S218`) |
 | `fs.max_write_bytes` | `0` (unlimited) | mid-stream cap during write (`S218`) |
 | `fs.denylist_paths` | `[]` | absolute-prefix denylist; rejected with `S215` |
@@ -68,7 +76,7 @@ The shell worker integrates with the central `configuration` worker rather than
 
 ## Threat model
 
-The host backend's path-validation gate is check-then-use: there is a TOCTOU window between validation and the `std::fs::*` call. Validation walks to the longest existing ancestor, canonicalizes that (resolving symlinks in the existing portion), and lexically collapses the non-existent tail before the `starts_with(host_root)` check — so a symlink whose target escapes the jail cannot slip through the lexical fallback. The worker is intended for trusted caller pipelines; for untrusted input, use the sandbox backend.
+The host backend's path-validation gate is check-then-use: there is a TOCTOU window between validation and the `std::fs::*` call. Validation walks to the longest existing ancestor, canonicalizes that (resolving symlinks in the existing portion), and lexically collapses the non-existent tail before the jail-root containment check — so a symlink whose target escapes the jail cannot slip through the lexical fallback. The worker is intended for trusted caller pipelines; for untrusted input, use the sandbox backend.
 
 Host-targeted calls run with the shell worker's OS permissions. The denylist is regex over `argv.join(" ")` and only catches honest typos — a caller invoking an allowlisted shell or interpreter (`sh`, `node`, `python`, …) can bypass it by construction. The actual security boundary is `target: { kind: "sandbox", sandbox_id }`.
 
@@ -138,11 +146,11 @@ let bytes = reader.read_all().await?;
 
 | Symptom | Cause | Fix |
 |---|---|---|
-| `fs.host_root is unset and fs.allow_unjailed is false — refusing to start unjailed` | Default config no longer permits running unjailed. | Set `fs.host_root` to a directory, OR set `fs.allow_unjailed: true`. |
+| `fs.host_roots is empty and fs.allow_unjailed is false — refusing to start unjailed` | Default config no longer permits running unjailed. | Set `fs.host_roots` to at least one directory, OR set `fs.allow_unjailed: true`. |
 | `command 'xyz' not in allowlist` | `allowlist` is non-empty and doesn't include the binary's basename. | Add it to `allowlist`, or empty the list to allow anything. |
 | Worker never connects to engine | Engine isn't running or isn't bound on the URL the worker is configured for. | Start the engine first; check `--url` matches. The default WS port is 49134. |
 | Engine started but doesn't see the worker | Binary isn't symlinked at `~/.iii/workers/shell`. | `ln -sfn $(pwd)/target/release/iii-shell ~/.iii/workers/shell` |
-| `S215 path escapes host_root` on a path inside the jail | A symlink in the path resolves outside the jail. | Resolve the symlink yourself, or move the target inside `host_root`. |
+| `S215 path escapes the fs jail roots` on a path inside the jail | A symlink in the path resolves outside the jail. | Resolve the symlink yourself, or move the target inside a jail root. |
 
 ## Tests
 

shell/CHANGELOG.md

@@ -1,5 +1,146 @@
 # Changelog
 
+## 0.7.0
+
+Environment-variable DX overhaul: one consolidated `env` config block, a
+self-documenting config schema, and a discoverable operator surface for the
+binary itself.
+
+### Breaking
+- **`inherit_env` and `allowed_env` are replaced by a nested `env` block**
+  (`env.inherit`, `env.allow`) — no legacy aliases. The old top-level keys are
+  **rejected at parse** with a migration hint naming the new keys. This is
+  deliberate fail-closed behavior: serde ignores unknown fields, so accepting
+  the old shape would silently boot with `env.inherit false` and stop
+  forwarding the worker's environment to children.
+- **`fs.host_root` (the 0.6.x single-root alias) is removed** — use
+  `fs.host_roots` (one-entry list). Like the env keys it is **rejected at
+  parse** with a migration hint (`fs.host_root` -> `fs.host_roots`); serde
+  would otherwise ignore the stale key and the worker would see no jail
+  configured at all.
+- **`code.base_path` and `code.base_paths` are removed from the schema and
+  REJECTED at parse.** They were inert even before removal — the code
+  resolver has always taken its roots from `fs.host_roots` (one jail
+  config) — but this is a hard migration: "never had an effect" is not an
+  exception. A stored value still carrying either fails closed with a hint
+  naming both keys, same as every other removed key.
+- **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.
+
+### Added
+- `--version` prints the worker version.
+- `--url` is documented in `--help`, including the `III_URL` env var binding.
+- Pre-connect reachability probe: when the engine is unreachable at boot, one
+  ERROR names the URL and the fix (`is the iii engine running? Set --url or
+  III_URL...`) before the SDK's silent 2s-backoff retry loop takes over. The
+  worker still never exits.
+- Every operator-visible config field (including the nested `env`, `fs`, and
+  `sandbox` blocks) now carries a schema description, so the console
+  configuration UI documents each knob inline. Pinned by a unit test.
+- A `## Running` README section documents the binary's full operator surface
+  (`--config`, `--url`/`III_URL`, `--version`, `RUST_LOG`).
+
+### Changed (shipped seed / defaults review)
+- **Command-shaped denylist patterns are anchored to argv[0]**
+  (`^(\S*/)?mkfs|dd|shutdown|reboot`): they fire when the tool IS the command,
+  not when the word appears in an argument — `grep -rn shutdown src/` and
+  `rg "dd if=" docs/` are no longer rejected. Argument-shaped patterns
+  (`rm -rf /`, the fork bomb, `/etc/shadow`) stay unanchored. Stacks that
+  rewrite their stored value by hand should adopt the anchored forms too.
+- The denylist rejection message now says it is an advisory tripwire and to
+  rephrase the command, so agents stop retrying verbatim.
+- The seed uses the multi-root jail form (`fs.host_roots: [/tmp]`) — the
+  singular `fs.host_root` was then removed outright (see Breaking).
+- Seed `default_timeout_ms` raised 10s → 30s: the seed raises
+  `max_timeout_ms` to 120s so real builds survive; callers omitting
+  `timeout_ms` shouldn't be reaped at 10s on the same workload. The CODE
+  default is unchanged (10s).
+- `fs.max_read_bytes`/`fs.max_write_bytes` descriptions now explain why the
+  code default is unlimited (reads/writes stream in chunks; the cap bounds
+  caller cost, not worker memory), and the seed comments say
+  `fs.denylist_paths` is defense in depth (unreachable anyway while jailed).
+- Every `code.*` (CoderConfig) budget field now carries a schema description;
+  the schema test covers all nested definitions, not just the top level.
+
+### Fixed (pre-landing review)
+- **Seed-file parse failures now fail closed.** A `--config` file that EXISTS
+  but fails to parse (e.g. still carries the removed 0.6.x keys) aborts boot
+  instead of warning and silently seeding the permissive built-in default in
+  its place — that fallback would have handed a fresh registration an open
+  allowlist and full env forwarding instead of the operator's intended
+  policy. A genuinely MISSING file still falls through gracefully.
+- **Hot-reload no longer retry-storms on an unparseable stored value.**
+  Previously, a stored config carrying removed keys failed inside the fetch
+  step and was misclassified as a *transient* error (dispatcher retries
+  forever against bytes that can never parse). It is now classified as
+  `Rejected` — the same treatment as an unbuildable-but-parseable config:
+  keep last-good, ack (no storm), and record the rejection for
+  `shell::config-status`.
+- **`env.allow` half-migration is rejected.** `EnvConfig` now denies unknown
+  fields, so nesting the OLD key names under the new block (e.g.
+  `env: { inherit_env: true }`) fails closed instead of silently falling back
+  to the wider default `allow` list.
+- **Command-shaped denylist patterns tolerate a wrapper prefix**
+  (`sudo`, `doas`, `nohup`, `env`, `timeout [duration]`, optionally
+  path-qualified): `sudo shutdown -h now` trips the tripwire again — the
+  argv[0]-anchoring in the first pass of this release had dropped that case,
+  arguably the most likely accidental invocation for commands that normally
+  require root.
+- **The removed-key check found and fixed its own bug during consolidation**:
+  merging the top-level and nested-`fs` checks into one function surfaced
+  that the original used `.any()`, which short-circuits — a config carrying
+  BOTH `inherit_env` and `allowed_env` only ever named the first in its error.
+  Every removed key present is now named in one pass.
+- **The boot-time reachability probe runs detached** so its DNS resolution
+  (unbounded — system resolver) and 2s-per-address TCP connect attempts can
+  never delay startup, and it logs host:port rather than the raw URL (a
+  `wss://user:pass@host` URL could otherwise leak credentials to the log).
+- **A half-migrated `env` block now gets the same migration hint as every
+  other removed key.** Nesting the OLD field names under the NEW `env:`
+  block (e.g. `env: { inherit_env: true }`) previously hit `EnvConfig`'s
+  generic `deny_unknown_fields` serde error with no guidance; it now names
+  the key and points at `env.inherit`/`env.allow` like the other rejections.
+- **The anchored denylist wrapper tolerance is now case-insensitive and
+  handles `env`'s idiomatic `KEY=VALUE...` form.** `SUDO shutdown -h now`
+  and `env FOO=bar shutdown -h now` (env's actual common usage — bare
+  `env cmd` was covered, `env KEY=VAL cmd` was not) now trip the tripwire;
+  previously both silently bypassed it, undermining the wrapper-tolerance
+  feature's own stated purpose for its most-used wrapper.
+- **Fixed a flaky test**: two tests that mutate process-wide environment
+  state (`std::env::set_var`) could race on separate `cargo test` threads
+  within the same binary, producing an intermittent, environment-dependent
+  failure. Both now serialize on a shared test-only mutex.
+
+### Migration
+```yaml
+# 0.6.x                                # 0.7.0
+inherit_env: true                      env:
+allowed_env: [PATH, HOME, LANG]          inherit: true
+                                         allow: [PATH, HOME, LANG]
+```
+- A stored configuration value (id `shell`) still carrying the old keys makes
+  the worker fail closed at boot with the hint above. Rewrite it via
+  `configuration::set` with the nested shape.
+- **Order matters**: deploy the 0.7.0 binary FIRST, then rewrite the stored
+  value. Writing the new shape while 0.6.x is still running makes the old
+  worker hot-reload it, ignore the unknown `env` block, and silently stop
+  forwarding env until restart.
+
 ## 0.6.0
 
 The standalone `coder` worker is folded into `shell`. There is now ONE worker,

shell/Cargo.toml

@@ -2,7 +2,7 @@
 
 [package]
 name = "shell"
-version = "0.6.1"
+version = "0.7.0"
 edition = "2021"
 publish = false
 
@@ -29,6 +29,7 @@ once_cell = "1"
 regex = "1"
 shell-words = "1"
 async-trait = "0.1"
+url = "2"
 base64 = "0.22"
 walkdir = "2"
 globset = "0.4"