Merge the coder file worker into shell (one worker, one jail, one config) - #340
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR folds ChangesCoder worker consolidated into shell
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 |
Contributor
skill-check — worker0 verified, 28 skipped (no docs/).
Four for four. Nicely done. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shell/src/main.rs (1)
82-90: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAvoid seeding
/tmpbefore the legacy coder fold.When no shell config exists but a legacy
coderconfig does,register_configcan createShellConfig::seed_default()first. That makesshell.fs.is_jailed()true (/tmp), sofold_coder_into_shellwill not adopt the coder roots, breaking the “only coder was configured” upgrade path.🤖 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 82 - 90, The bootstrapping flow in main currently registers the shell schema before the legacy coder fold, which can seed ShellConfig::seed_default() and lock shell.fs into the jailed /tmp default before migration. Reorder the setup so migrate_legacy_coder(&iii) runs before register_config(&iii, seed.as_ref()) (or otherwise prevent default shell seeding from occurring first), ensuring fold_coder_into_shell can still adopt legacy coder roots when only coder was configured.shell/src/config.rs (1)
360-386: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winReject relative command paths before canonicalizing.
./lsorsubdir/lsis canonicalized relative to the worker process cwd here, but the spawned process resolves it relative to the requestcwd. Withcwdinside the writable jail, a planted./lscan bypass this jail-path guard. Reject non-absolute slash-containing commands, or resolve them against the effective exec cwd before this check.Suggested fix
if cmd.contains('/') { + let cmd_path = std::path::Path::new(&cmd); + if !cmd_path.is_absolute() { + return Err(format!( + "relative command path '{}' is not allowed; use a bare command name \ + (PATH-resolved) or an absolute path outside the writable fs jail.", + cmd + )); + } // Unjailed mode (host_root: null) has NO writable boundary — the // whole host filesystem is reachable via shell::fs::write, so an // agent can plant `/tmp/ls` and run `command: "/tmp/ls"` (basename🤖 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 360 - 386, Reject slash-containing relative command paths in the command validation logic before calling canonicalize in the config::Config path check. In the branch that handles cmd.contains('/'), update the fs.is_jailed() path handling so `./ls` and `subdir/ls` cannot be validated against the worker process cwd while the spawned process uses the request cwd; either require absolute paths here or resolve the command against the effective exec cwd before checking roots. Keep the existing allowlist and canonicalization flow intact for absolute paths, using the same command validation block and canonicalize/root comparison logic.
🧹 Nitpick comments (2)
shell/tests/code_unified_protection.rs (1)
30-42: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a legacy
fs.host_rootcoverage case here.This helper only builds runtimes through
fs.host_roots, so the compatibility path kept for existing configs can regress without tripping this wiring suite. Mirroring one of these assertions through the one-entry alias would pin the migration contract too.🤖 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/tests/code_unified_protection.rs` around lines 30 - 42, Add a legacy fs.host_root coverage case in this test so the compatibility alias is exercised alongside fs.host_roots. Update the build_runtime setup in code_unified_protection.rs to include a one-entry host_root-based configuration path, using the same ShellConfig/FsConfig and build_runtime symbols, and assert it succeeds just like the existing jailed config case.shell/tests/code_lifecycle.rs (1)
217-224: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the delete side effect on disk.
This only checks the wire response. Add an
exists()or failing reread assertion so the lifecycle test also catches handlers that reportremoved: truewithout actually deleting the file.Suggested assertion
assert_eq!(del["results"][0]["success"], true); assert_eq!(del["results"][0]["removed"], true); + assert!( + !s.root.join("hello.txt").exists(), + "delete-file reported success but left hello.txt on disk" + ); }🤖 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/tests/code_lifecycle.rs` around lines 217 - 224, The delete-file lifecycle check only validates the wire response from delete_file::handle and does not verify the on-disk side effect. Update the code around delete_file::handle in code_lifecycle.rs to also assert the file is actually gone after deletion, using an exists() check or a failing reread of hello.txt. Keep the existing response assertions, but add a post-delete filesystem assertion so the test catches handlers that return removed: true without deleting the file.
🤖 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/Cargo.toml`:
- Line 5: The version bump in shell/Cargo.toml is not reflected in the migration
docs, so update the versioned upgrade heading in shell/README.md from the old
0.4.0 label to match the new 0.6.0 release. Locate the upgrade section heading
in the README and keep the rest of the migration guidance unchanged while
renaming that heading to the current version.
In `@shell/config.yaml`:
- Around line 76-78: The seed config comment references fs.host_roots but only
shows the legacy host_root shape, so update the seeded/default example to
demonstrate the new host_roots form or clearly label host_root as the one-entry
legacy alias. Use the existing config comment block around fs.host_roots and the
operator-facing seed section to keep the reference consistent for readers.
In `@shell/README.md`:
- Around line 145-149: Document that the `coder::*` surface is only available
when `shell` is running with a jail, and clarify that `iii worker add shell`
does not always expose those functions; update the `shell::fs::*`/`coder::*`
description in `README.md` to include the `fs.allow_unjailed: true` caveat, and
mirror the same jail precondition in the matching changelog and skill copy so
the `coder::*` registration behavior is consistent across docs.
In `@shell/src/code/config.rs`:
- Around line 238-239: The `jail_signature()` helper is currently test-only,
which prevents production reload logic from comparing jail boundaries against
the current config. Make `Config::jail_signature()` available outside
`#[cfg(test)]` so the reload path can use the same signature when deciding
whether roots, protected globs, or default excludes require a restart. Update
the surrounding reload/validation code that uses `jail_signature()` to rely on
this shared method instead of duplicating the resolver-boundary logic.
In `@shell/src/code/functions/create_file.rs`:
- Around line 361-395: The test in create_file should not use a fixed escaped
target like "../escape.txt" because it can collide with a real file outside the
tempdir and cause flaky failures. Update the create_file test case to use a
unique sibling escape path, or capture and restore/assert the preexisting state
before checking the escaped path, while keeping the existing expectations around
out.results, ok.txt, and the jail-escape failure behavior.
In `@shell/src/code/functions/mod.rs`:
- Around line 237-240: `ConfigCell` is currently only passed into `register_all`
and then effectively lost after startup, so hot-reload can never replace the
`Arc<CoderConfig>` used by handlers. Update the reload flow in `main.rs` to keep
a live clone of the same `ConfigCell` in reload state and swap in the new config
on accepted reloads, so `coder::info`, `coder::search`, `coder::tree`,
`coder::update-file`, and `coder::create-file` see updated settings. If reload
is not meant to work, remove the `RwLock`-based indirection from
`ConfigCell`/`shell/src/code/config.rs` instead of leaving dead reload plumbing
in place.
In `@shell/src/configuration.rs`:
- Around line 257-261: The migration path in fold_coder_into_shell and the
configuration::set persistence flow should validate the merged shell config
before writing it. Before calling trigger_with_retry for the merged payload,
construct the same runtime and code-resolver checks used by
fetch_config/build_runtime and PathResolver::new against merged, and only
persist when that validation succeeds. If validation fails, skip the write and
surface the failure so malformed coder globs or unreachable adopted roots are
caught before becoming persistent.
In `@shell/src/fs/host.rs`:
- Around line 82-89: Recursive delete and directory move currently bypass
descendant protected-path checks, so `rm(recursive=true)` and `mv` can affect
protected files like .env without `path_is_non_accessible` ever running. Update
the `ShellFs` delete/move flow to walk directory descendants before
`remove_dir_all`, and in the `mv` path validate both the source tree and the
destination mapping against `non_accessible_globs`/`path_is_non_accessible` so
any protected descendant blocks the operation. Use the existing
`path_is_non_accessible`, `rm`, and `mv` logic as the entry points to enforce
the D4 contract consistently for recursive and directory operations.
In `@shell/src/main.rs`:
- Around line 147-152: The jailed code setup in main still reads from the
original cfg after reconcile(&state) may have replaced it with the authoritative
runtime config, so the code surface can be built from stale roots/globs. Update
the coder::* setup block to derive code_cfg, PathResolver::new, and the
ConfigCell from the reconciled runtime config/state value instead of the
pre-reconcile cfg, keeping the existing error handling around PathResolver::new
intact.
---
Outside diff comments:
In `@shell/src/config.rs`:
- Around line 360-386: Reject slash-containing relative command paths in the
command validation logic before calling canonicalize in the config::Config path
check. In the branch that handles cmd.contains('/'), update the fs.is_jailed()
path handling so `./ls` and `subdir/ls` cannot be validated against the worker
process cwd while the spawned process uses the request cwd; either require
absolute paths here or resolve the command against the effective exec cwd before
checking roots. Keep the existing allowlist and canonicalization flow intact for
absolute paths, using the same command validation block and canonicalize/root
comparison logic.
In `@shell/src/main.rs`:
- Around line 82-90: The bootstrapping flow in main currently registers the
shell schema before the legacy coder fold, which can seed
ShellConfig::seed_default() and lock shell.fs into the jailed /tmp default
before migration. Reorder the setup so migrate_legacy_coder(&iii) runs before
register_config(&iii, seed.as_ref()) (or otherwise prevent default shell seeding
from occurring first), ensuring fold_coder_into_shell can still adopt legacy
coder roots when only coder was configured.
---
Nitpick comments:
In `@shell/tests/code_lifecycle.rs`:
- Around line 217-224: The delete-file lifecycle check only validates the wire
response from delete_file::handle and does not verify the on-disk side effect.
Update the code around delete_file::handle in code_lifecycle.rs to also assert
the file is actually gone after deletion, using an exists() check or a failing
reread of hello.txt. Keep the existing response assertions, but add a
post-delete filesystem assertion so the test catches handlers that return
removed: true without deleting the file.
In `@shell/tests/code_unified_protection.rs`:
- Around line 30-42: Add a legacy fs.host_root coverage case in this test so the
compatibility alias is exercised alongside fs.host_roots. Update the
build_runtime setup in code_unified_protection.rs to include a one-entry
host_root-based configuration path, using the same ShellConfig/FsConfig and
build_runtime symbols, and assert it succeeds just like the existing jailed
config case.
🪄 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: 0a7084bf-d46c-44ee-bf0f-f8fef18ed737
⛔ Files ignored due to path filters (2)
coder/Cargo.lockis excluded by!**/*.lockshell/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (100)
.github/scripts/parse_publish_workers_input.py.github/scripts/tests/test_parse_publish_workers_input.py.github/workflows/create-tag.yml.github/workflows/release.ymlcoder/CHANGELOG.mdcoder/Cargo.tomlcoder/README.mdcoder/build.rscoder/config.collect.yamlcoder/config.yamlcoder/config.yaml.examplecoder/iii.worker.yamlcoder/scripts/error-frequency.pycoder/skills/SKILL.mdcoder/src/configuration.rscoder/src/lib.rscoder/src/main.rscoder/src/manifest.rscoder/tests/bdd.rscoder/tests/common/engine.rscoder/tests/common/helpers.rscoder/tests/common/mod.rscoder/tests/common/workers.rscoder/tests/common/world.rscoder/tests/features/create_file.featurecoder/tests/features/delete_file.featurecoder/tests/features/lifecycle.featurecoder/tests/features/list_folder.featurecoder/tests/features/path_security.featurecoder/tests/features/read_file.featurecoder/tests/features/search.featurecoder/tests/features/tree.featurecoder/tests/features/update_file.featurecoder/tests/integration.rscoder/tests/manifest.rscoder/tests/steps/common.rscoder/tests/steps/create.rscoder/tests/steps/delete.rscoder/tests/steps/lifecycle.rscoder/tests/steps/list.rscoder/tests/steps/mod.rscoder/tests/steps/read.rscoder/tests/steps/search.rscoder/tests/steps/security.rscoder/tests/steps/tree.rscoder/tests/steps/update.rsconsole/web/src/pages/Configuration/tabs/WorkersTab/index.tsxharness/prompts/anthropic.txtharness/prompts/cli.txtharness/prompts/default.txtharness/prompts/gpt.txtharness/prompts/kimi.txtharness/src/prompt/tests.rsiii-permissions.yamlshell/CHANGELOG.mdshell/Cargo.tomlshell/README.mdshell/config.yamlshell/skills/SKILL.mdshell/src/code/config.rsshell/src/code/error.rsshell/src/code/functions/create_file.rsshell/src/code/functions/delete_file.rsshell/src/code/functions/info.rsshell/src/code/functions/list_folder.rsshell/src/code/functions/mod.rsshell/src/code/functions/move_file.rsshell/src/code/functions/read_file.rsshell/src/code/functions/read_window.rsshell/src/code/functions/search.rsshell/src/code/functions/tree.rsshell/src/code/functions/update_file.rsshell/src/code/mod.rsshell/src/code/path.rsshell/src/code/state.rsshell/src/config.rsshell/src/configuration.rsshell/src/exec/policy.rsshell/src/fs/host.rsshell/src/lib.rsshell/src/main.rsshell/src/path/mod.rsshell/tests/code_golden_errors.rsshell/tests/code_golden_schemas.rsshell/tests/code_lifecycle.rsshell/tests/code_parity.rsshell/tests/code_path_jail.rsshell/tests/code_unified_protection.rsshell/tests/code_update_ops.rsshell/tests/golden/errors.jsonshell/tests/golden/schemas/coder.create-file.jsonshell/tests/golden/schemas/coder.delete-file.jsonshell/tests/golden/schemas/coder.info.jsonshell/tests/golden/schemas/coder.list-folder.jsonshell/tests/golden/schemas/coder.move.jsonshell/tests/golden/schemas/coder.read-file.jsonshell/tests/golden/schemas/coder.search.jsonshell/tests/golden/schemas/coder.tree.jsonshell/tests/golden/schemas/coder.update-file.jsonshell/tests/support/mod.rs
💤 Files with no reviewable changes (45)
- coder/iii.worker.yaml
- coder/tests/features/lifecycle.feature
- coder/tests/features/read_file.feature
- coder/tests/steps/lifecycle.rs
- coder/tests/features/list_folder.feature
- coder/tests/steps/tree.rs
- coder/tests/features/create_file.feature
- coder/tests/features/path_security.feature
- coder/Cargo.toml
- coder/config.yaml.example
- .github/workflows/release.yml
- coder/tests/steps/list.rs
- coder/tests/steps/read.rs
- coder/tests/common/engine.rs
- coder/tests/steps/common.rs
- coder/tests/steps/delete.rs
- coder/tests/common/mod.rs
- coder/src/main.rs
- coder/tests/bdd.rs
- coder/tests/steps/security.rs
- coder/config.yaml
- coder/skills/SKILL.md
- .github/workflows/create-tag.yml
- coder/CHANGELOG.md
- coder/tests/steps/search.rs
- coder/config.collect.yaml
- coder/tests/features/tree.feature
- coder/tests/features/search.feature
- coder/tests/steps/update.rs
- coder/tests/integration.rs
- coder/src/lib.rs
- coder/tests/common/helpers.rs
- coder/scripts/error-frequency.py
- coder/tests/features/delete_file.feature
- coder/build.rs
- coder/src/configuration.rs
- coder/src/manifest.rs
- coder/README.md
- coder/tests/steps/create.rs
- coder/tests/common/workers.rs
- coder/tests/common/world.rs
- .github/scripts/parse_publish_workers_input.py
- coder/tests/steps/mod.rs
- coder/tests/manifest.rs
- coder/tests/features/update_file.feature
| [package] | ||
| name = "shell" | ||
| version = "0.5.5" | ||
| version = "0.6.0" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the README's versioned upgrade heading with this bump.
shell/Cargo.toml moves to 0.6.0, but shell/README.md still says ## Upgrading to 0.4.0. That makes the migration docs look stale in this release.
🤖 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/Cargo.toml` at line 5, The version bump in shell/Cargo.toml is not
reflected in the migration docs, so update the versioned upgrade heading in
shell/README.md from the old 0.4.0 label to match the new 0.6.0 release. Locate
the upgrade section heading in the README and keep the rest of the migration
guidance unchanged while renaming that heading to the current version.
Comment on lines
+76
to
+78
| # The folded `coder::*` code surface. Its ROOTS are NOT set here — the code | ||
| # resolver uses fs.host_roots above, so the operator sets the project root | ||
| # ONCE. `non_accessible_globs` is the unified protected-paths list: BOTH the |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Show the new host_roots shape in the seed config as well.
This comment points readers at fs.host_roots above, but the shipped config only demonstrates the legacy host_root key. Since config.yaml is the seeded/default operator reference, please either add a commented host_roots example or explicitly call out that host_root is the one-entry legacy alias.
🤖 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/config.yaml` around lines 76 - 78, The seed config comment references
fs.host_roots but only shows the legacy host_root shape, so update the
seeded/default example to demonstrate the new host_roots form or clearly label
host_root as the one-entry legacy alias. Use the existing config comment block
around fs.host_roots and the operator-facing seed section to keep the reference
consistent for readers.
Comment on lines
+145
to
+149
| Roots come from `fs.host_roots`; protection globs come from | ||
| `code.non_accessible_globs` — the **same** list `shell::fs::*` enforces (declared | ||
| once, see [Configure](#configure)). `coder::*` returns `C2xx` error codes (its | ||
| own taxonomy), distinct from `shell::*`'s `S2xx`. No separate install: `iii | ||
| worker add shell` brings the whole surface. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the jail precondition for coder::* here.
This reads as if iii worker add shell always exposes the whole coder::* surface, but the fold-in contract in this PR is tighter: coder::* is only registered when shell is running with a jail. Please add that caveat here, and mirror it in the matching changelog/skill copy, so operators using fs.allow_unjailed: true do not expect missing functions to appear.
🤖 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 145 - 149, Document that the `coder::*` surface
is only available when `shell` is running with a jail, and clarify that `iii
worker add shell` does not always expose those functions; update the
`shell::fs::*`/`coder::*` description in `README.md` to include the
`fs.allow_unjailed: true` caveat, and mirror the same jail precondition in the
matching changelog and skill copy so the `coder::*` registration behavior is
consistent across docs.
Comment on lines
+238
to
239
| #[cfg(test)] | ||
| pub fn jail_signature(&self) -> JailSignature { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Keep jail_signature() available to production reload code.
The surrounding contract says roots/protected globs/default excludes are restart-required, but jail_signature() is #[cfg(test)], so production reload logic cannot enforce that distinction without duplicating it. This risks accepting a shell config reload while coder::* keeps the old resolver boundary.
🤖 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/code/config.rs` around lines 238 - 239, The `jail_signature()`
helper is currently test-only, which prevents production reload logic from
comparing jail boundaries against the current config. Make
`Config::jail_signature()` available outside `#[cfg(test)]` so the reload path
can use the same signature when deciding whether roots, protected globs, or
default excludes require a restart. Update the surrounding reload/validation
code that uses `jail_signature()` to rely on this shared method instead of
duplicating the resolver-boundary logic.
Comment on lines
+361
to
+395
| files: vec![ | ||
| CreateFileSpec { | ||
| path: "../escape.txt".into(), | ||
| content: "x".into(), | ||
| mode: "0644".into(), | ||
| parents: true, | ||
| overwrite: false, | ||
| }, | ||
| CreateFileSpec { | ||
| path: "ok.txt".into(), | ||
| content: "y".into(), | ||
| mode: "0644".into(), | ||
| parents: true, | ||
| overwrite: false, | ||
| }, | ||
| ], | ||
| base_dir: None, | ||
| }, | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
| assert!(!out.results[0].success, "escape entry must fail"); | ||
| assert_eq!(out.results[0].error.as_ref().unwrap().code, "C215"); | ||
| assert!( | ||
| out.results[1].success, | ||
| "the in-jail entry must still be written" | ||
| ); | ||
| assert_eq!( | ||
| std::fs::read_to_string(tmp.path().join("ok.txt")).unwrap(), | ||
| "y" | ||
| ); | ||
| assert!( | ||
| !tmp.path().join("../escape.txt").exists(), | ||
| "the escaping path must never be created" | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use a unique escaped target in this test.
Lines 363 and 393 hard-code ../escape.txt, which resolves to the tempdir's parent. On typical runners that means probing a shared path like /tmp/escape.txt; if that file already exists, this test fails even though create_file never escaped the jail. Generate a unique sibling name for the escape attempt, or record the preexisting state before asserting.
🤖 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/code/functions/create_file.rs` around lines 361 - 395, The test in
create_file should not use a fixed escaped target like "../escape.txt" because
it can collide with a real file outside the tempdir and cause flaky failures.
Update the create_file test case to use a unique sibling escape path, or capture
and restore/assert the preexisting state before checking the escaped path, while
keeping the existing expectations around out.results, ok.txt, and the
jail-escape failure behavior.
Comment on lines
+237
to
+240
| pub fn register_all(iii: &IIIClient, resolver: Arc<PathResolver>, cfg: ConfigCell) { | ||
| // DRIFT GUARD: the register_* calls below and the entries in | ||
| // `catalog()` must stay 1:1 — catalog() feeds the wire-schema goldens | ||
| // (tests/golden_schemas.rs). Adding a function to one list but not | ||
| // (tests/code_golden_schemas.rs). Adding a function to one list but not |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
ConfigCell is effectively immutable after startup.
Line 237 threads a mutable config cell into every handler, but shell/src/main.rs creates that cell as a local and drops the last external handle right after register_all. That leaves no place for reload logic to swap in a new Arc<CoderConfig>, so handlers like coder::info, coder::search, coder::tree, coder::update-file, and coder::create-file will keep serving the boot-time limits forever even though shell/src/code/config.rs documents the non-jail fields as hot-applied. Keep a clone of this cell in reload state and update it on accepted reloads, or drop the RwLock indirection if reload is intentionally unsupported.
🤖 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/code/functions/mod.rs` around lines 237 - 240, `ConfigCell` is
currently only passed into `register_all` and then effectively lost after
startup, so hot-reload can never replace the `Arc<CoderConfig>` used by
handlers. Update the reload flow in `main.rs` to keep a live clone of the same
`ConfigCell` in reload state and swap in the new config on accepted reloads, so
`coder::info`, `coder::search`, `coder::tree`, `coder::update-file`, and
`coder::create-file` see updated settings. If reload is not meant to work,
remove the `RwLock`-based indirection from
`ConfigCell`/`shell/src/code/config.rs` instead of leaving dead reload plumbing
in place.
Comment on lines
+257
to
+261
| match fold_coder_into_shell(shell, &coder) { | ||
| None => tracing::debug!("coder→shell migration already applied; skipping"), | ||
| Some(merged) => { | ||
| let payload = json!({ "id": CONFIG_ID, "value": merged.to_json() }); | ||
| match trigger_with_retry(iii, "configuration::set", payload).await { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the merged config before persisting migration output.
configuration::set can store a merged shell config with unreachable adopted roots or malformed coder globs; the immediate fetch_config/build_runtime or PathResolver::new then fails after the bad value is already persistent. Validate merged with the same runtime + code-resolver construction before writing it.
Suggested guard
None => tracing::debug!("coder→shell migration already applied; skipping"),
Some(merged) => {
+ if let Err(e) = build_runtime(&merged, iii) {
+ tracing::warn!(error = %e, "merged coder→shell config is invalid; skipping migration write");
+ return;
+ }
+ if merged.fs.is_jailed() {
+ let code_cfg = merged.code_resolver_config();
+ if let Err(e) = crate::code::path::PathResolver::new(&code_cfg) {
+ tracing::warn!(error = %e, "merged coder→shell code resolver is invalid; skipping migration write");
+ return;
+ }
+ }
let payload = json!({ "id": CONFIG_ID, "value": merged.to_json() });📝 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
| match fold_coder_into_shell(shell, &coder) { | |
| None => tracing::debug!("coder→shell migration already applied; skipping"), | |
| Some(merged) => { | |
| let payload = json!({ "id": CONFIG_ID, "value": merged.to_json() }); | |
| match trigger_with_retry(iii, "configuration::set", payload).await { | |
| match fold_coder_into_shell(shell, &coder) { | |
| None => tracing::debug!("coder→shell migration already applied; skipping"), | |
| Some(merged) => { | |
| if let Err(e) = build_runtime(&merged, iii) { | |
| tracing::warn!(error = %e, "merged coder→shell config is invalid; skipping migration write"); | |
| return; | |
| } | |
| if merged.fs.is_jailed() { | |
| let code_cfg = merged.code_resolver_config(); | |
| if let Err(e) = crate::code::path::PathResolver::new(&code_cfg) { | |
| tracing::warn!(error = %e, "merged coder→shell code resolver is invalid; skipping migration write"); | |
| return; | |
| } | |
| } | |
| let payload = json!({ "id": CONFIG_ID, "value": merged.to_json() }); | |
| match trigger_with_retry(iii, "configuration::set", payload).await { |
🤖 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/configuration.rs` around lines 257 - 261, The migration path in
fold_coder_into_shell and the configuration::set persistence flow should
validate the merged shell config before writing it. Before calling
trigger_with_retry for the merged payload, construct the same runtime and
code-resolver checks used by fetch_config/build_runtime and PathResolver::new
against merged, and only persist when that validation succeeds. If validation
fails, skip the write and surface the failure so malformed coder globs or
unreachable adopted roots are caught before becoming persistent.
Comment on lines
+147
to
+152
| if cfg.fs.is_jailed() { | ||
| let code_cfg = cfg.code_resolver_config(); | ||
| let resolver = code::path::PathResolver::new(&code_cfg) | ||
| .map_err(|e| anyhow::anyhow!("failed to build code PathResolver (coder::*): {e}"))?; | ||
| let cell: code::ConfigCell = | ||
| std::sync::Arc::new(tokio::sync::RwLock::new(std::sync::Arc::new(code_cfg))); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Build coder::* from the reconciled runtime config.
reconcile(&state) can swap in a newer authoritative config on Lines 132-139, but this block still uses the earlier cfg. That can register the code surface over stale roots/globs immediately after a boot-race reconcile.
Suggested fix
- if cfg.fs.is_jailed() {
- let code_cfg = cfg.code_resolver_config();
+ let active_cfg = { state.runtime.read().await.config.clone() };
+ if active_cfg.fs.is_jailed() {
+ let code_cfg = active_cfg.code_resolver_config();📝 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
| if cfg.fs.is_jailed() { | |
| let code_cfg = cfg.code_resolver_config(); | |
| let resolver = code::path::PathResolver::new(&code_cfg) | |
| .map_err(|e| anyhow::anyhow!("failed to build code PathResolver (coder::*): {e}"))?; | |
| let cell: code::ConfigCell = | |
| std::sync::Arc::new(tokio::sync::RwLock::new(std::sync::Arc::new(code_cfg))); | |
| let active_cfg = { state.runtime.read().await.config.clone() }; | |
| if active_cfg.fs.is_jailed() { | |
| let code_cfg = active_cfg.code_resolver_config(); | |
| let resolver = code::path::PathResolver::new(&code_cfg) | |
| .map_err(|e| anyhow::anyhow!("failed to build code PathResolver (coder::*): {e}"))?; | |
| let cell: code::ConfigCell = | |
| std::sync::Arc::new(tokio::sync::RwLock::new(std::sync::Arc::new(code_cfg))); |
🤖 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 147 - 152, The jailed code setup in main
still reads from the original cfg after reconcile(&state) may have replaced it
with the authoritative runtime config, so the code surface can be built from
stale roots/globs. Update the coder::* setup block to derive code_cfg,
PathResolver::new, and the ConfigCell from the reconciled runtime config/state
value instead of the pre-reconcile cfg, keeping the existing error handling
around PathResolver::new intact.
Fold the standalone coder worker's file surface into shell: read, window, search, tree, list-folder, create, update, move, delete, and info now run inside shell against a single multi-root jail driven by `fs.host_roots` (the operator sets the root once; any `code.base_paths` is ignored). - offload the code read/scan handlers to spawn_blocking - honor the unified protected-path globs in `shell::fs` - fold the legacy `coder` config into the `code` block with a one-shot, never-widen migration marker (`migrated_from_coder`) - retire the standalone coder worker (remove its crate, release tooling, and console workers entry) and retarget the agent prompts/docs to shell - migrate the folded surface to iii-sdk 0.20 Preserves #327's per-session working-directory protections (the session dir cannot be deleted via a scoped `delete path="."`).
ytallo
force-pushed
the
feat/merge-coder-into-shell
branch
from
June 28, 2026 11:34
4cceb55 to
a9d754c
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
shell/tests/common/world.rs (1)
61-66: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep the jail surface available in live mode.
setup_live_client()dropssurface, soexpand()/path_matches()can no longer resolve{{root}},{{secondary}}, or{{outside}}for live scenarios. That makes the live path unable to exercise the new multi-root assertions against the same temp jail.🤖 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/tests/common/world.rs` around lines 61 - 66, setup_live_client() is clearing self.surface, which prevents live scenarios from resolving jail roots via expand() and path_matches(). Update setup_live_client() in World so the existing surface stays available when switching to a live_client, and only reset the fields that should actually change in live mode (live_client, skip_reason, and result state). Keep the same temp jail surface intact so {{root}}, {{secondary}}, and {{outside}} continue to work.shell/tests/features/coder/live_registration.feature (1)
4-25: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the non-jailed registration case too.
This proves
coder::*is reachable when jailed, but the PR contract is also that those functions are not registered when shell starts unjailed. Without the inverse scenario, that gating regression can still pass green.🤖 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/tests/features/coder/live_registration.feature` around lines 4 - 25, The live_registration.feature coverage only verifies that coder::* functions are available in the jailed shell worker, but it does not check the inverse unjailed case. Add a complementary scenario in the same feature to start the shell unjailed and assert that coder::info, coder::create-file, coder::read-file, and coder::delete-file are not registered or fail to resolve, so the registration gating is covered end-to-end.
🤖 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/tests/common/world.rs`:
- Around line 329-333: The direct payload decoding path in decode_payload
currently converts deserialization failures into raw “invalid payload” strings
instead of the stable C210 wire-format error expected by the shell coder
surface. Update decode_payload’s error mapping to construct and return the
corresponding C210 error for bad input, keeping the same decode_payload helper
and serde_json::from_value flow but replacing the ad hoc string formatting with
the shared wire-format error representation used elsewhere.
In `@shell/tests/features/coder/search.feature`:
- Around line 31-42: The protected-files search scenario only checks path
matching and does not verify that protected file contents are excluded. Update
the feature around coder::search to also assert content-side behavior, ideally
by querying for a token that exists inside the ".env" file and confirming no
content match is returned. Use the existing jailed code surface and
coder::search scenario as the anchor, and either extend the current scenario or
add a second one that explicitly proves protected contents are not searched.
In `@shell/tests/features/coder/update_file.feature`:
- Around line 42-55: The update-file feature test for overlapping edits only
asserts the C210 failure, so it can still pass if the file is mutated before the
error is raised. Strengthen the Scenario: overlapping line edits are rejected
before write by also asserting the jailed file surface or overlap.txt content
remains unchanged after coder::update-file returns the C210 error, using the
existing update-file flow and file path to confirm the rejection happens before
any write.
In `@shell/tests/steps/common.rs`:
- Around line 20-45: The live jailed shell smoke currently soft-skips when no
`coder::*` worker responds, which lets the gated live check pass without
validating the engine. Update `live_jailed_shell_code_surface` to treat probe
exhaustion as a test failure instead of calling `world.soft_skip`, while keeping
the existing `III_ENGINE_WS_URL` env check and successful `client.trigger` path
that calls `world.setup_live_client`.
- Around line 112-113: The non-Unix fallback in the symlink escape test is no
longer exercising the canonicalization path because it writes a regular file
instead of creating a symlink. Update the setup in the common test step so the
`symlink escapes are rejected after canonicalization` scenario uses a real
platform symlink API where available, and if that cannot be done on a target
platform, skip this scenario there. Keep the behavior aligned with the existing
symlink-based test flow in the `common.rs` step helpers.
- Around line 602-607: The helper assert_error_code currently checks
err.contains(code), which can match the wrong text in a structured wire error.
Update assert_error_code to parse the error payload as structured data and
compare the actual "code" field directly instead of searching the raw string.
Use the existing assert_error_code function in common.rs as the single place to
make this validation more precise.
---
Nitpick comments:
In `@shell/tests/common/world.rs`:
- Around line 61-66: setup_live_client() is clearing self.surface, which
prevents live scenarios from resolving jail roots via expand() and
path_matches(). Update setup_live_client() in World so the existing surface
stays available when switching to a live_client, and only reset the fields that
should actually change in live mode (live_client, skip_reason, and result
state). Keep the same temp jail surface intact so {{root}}, {{secondary}}, and
{{outside}} continue to work.
In `@shell/tests/features/coder/live_registration.feature`:
- Around line 4-25: The live_registration.feature coverage only verifies that
coder::* functions are available in the jailed shell worker, but it does not
check the inverse unjailed case. Add a complementary scenario in the same
feature to start the shell unjailed and assert that coder::info,
coder::create-file, coder::read-file, and coder::delete-file are not registered
or fail to resolve, so the registration gating is covered end-to-end.
🪄 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: ab049980-8f41-4447-9fdc-ab0ac52f1036
⛔ Files ignored due to path filters (1)
shell/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
harness/src/prompt/tests.rsshell/Cargo.tomlshell/tests/bdd.rsshell/tests/code_path_jail.rsshell/tests/common/mod.rsshell/tests/common/world.rsshell/tests/e2e/README.mdshell/tests/e2e/run-tests-jailed.shshell/tests/features/coder/create_file.featureshell/tests/features/coder/delete_file.featureshell/tests/features/coder/info.featureshell/tests/features/coder/lifecycle.featureshell/tests/features/coder/list_folder.featureshell/tests/features/coder/live_registration.featureshell/tests/features/coder/move.featureshell/tests/features/coder/path_security.featureshell/tests/features/coder/read_file.featureshell/tests/features/coder/search.featureshell/tests/features/coder/tree.featureshell/tests/features/coder/update_file.featureshell/tests/steps/common.rsshell/tests/steps/mod.rs
✅ Files skipped from review due to trivial changes (7)
- shell/tests/common/mod.rs
- shell/tests/bdd.rs
- shell/tests/steps/mod.rs
- shell/tests/features/coder/lifecycle.feature
- shell/tests/features/coder/read_file.feature
- shell/tests/e2e/README.md
- shell/Cargo.toml
Comment on lines
+329
to
+333
| fn decode_payload<T>(payload: Value) -> Result<T, String> | ||
| where | ||
| T: serde::de::DeserializeOwned, | ||
| { | ||
| serde_json::from_value(payload).map_err(|err| format!("invalid payload: {err}")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return wire-format C210 errors from direct payload decoding.
Direct calls turn deserialization failures into raw "invalid payload: ..." strings. That breaks parity with the shell-served coder surface, which is supposed to preserve stable C2xx codes for bad input.
🤖 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/tests/common/world.rs` around lines 329 - 333, The direct payload
decoding path in decode_payload currently converts deserialization failures into
raw “invalid payload” strings instead of the stable C210 wire-format error
expected by the shell coder surface. Update decode_payload’s error mapping to
construct and return the corresponding C210 error for bad input, keeping the
same decode_payload helper and serde_json::from_value flow but replacing the ad
hoc string formatting with the shared wire-format error representation used
elsewhere.
Comment on lines
+31
to
+42
| Scenario: protected files are omitted from search results | ||
| Given a jailed code surface | ||
| And a file at ".env" with content: | ||
| """ | ||
| TOKEN=super-secret | ||
| """ | ||
| When I call coder::search with payload: | ||
| """ | ||
| {"query":".env","path":".","search_content":true,"search_paths":true} | ||
| """ | ||
| Then the search has no path match for ".env" | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
This scenario never proves protected content is excluded.
Using ".env" as the query only exercises path matching. If the implementation accidentally still scans protected file contents, this would keep passing because TOKEN=super-secret can never match ".env". Add a content-side assertion (or a second scenario) that queries protected content too.
🤖 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/tests/features/coder/search.feature` around lines 31 - 42, The
protected-files search scenario only checks path matching and does not verify
that protected file contents are excluded. Update the feature around
coder::search to also assert content-side behavior, ideally by querying for a
token that exists inside the ".env" file and confirming no content match is
returned. Use the existing jailed code surface and coder::search scenario as the
anchor, and either extend the current scenario or add a second one that
explicitly proves protected contents are not searched.
Comment on lines
+42
to
+55
| Scenario: overlapping line edits are rejected before write | ||
| Given a jailed code surface | ||
| And a file at "overlap.txt" with content: | ||
| """ | ||
| one | ||
| two | ||
| three | ||
| """ | ||
| When I call coder::update-file with payload: | ||
| """ | ||
| {"files":[{"path":"overlap.txt","ops":[{"op":"remove","from_line":1,"to_line":2},{"op":"update_lines","from_line":2,"to_line":3,"content":"x\n"}]}]} | ||
| """ | ||
| Then the result for "overlap.txt" failed with code "C210" | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Verify overlap rejection is truly pre-write.
Right now this only checks C210. A regression that detects the overlap after mutating the file would still pass, even though the scenario says “before write.”
Suggested assertion
Then the result for "overlap.txt" failed with code "C210"
+ And the file "overlap.txt" equals:
+ """
+ one
+ two
+ three
+ """📝 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
| Scenario: overlapping line edits are rejected before write | |
| Given a jailed code surface | |
| And a file at "overlap.txt" with content: | |
| """ | |
| one | |
| two | |
| three | |
| """ | |
| When I call coder::update-file with payload: | |
| """ | |
| {"files":[{"path":"overlap.txt","ops":[{"op":"remove","from_line":1,"to_line":2},{"op":"update_lines","from_line":2,"to_line":3,"content":"x\n"}]}]} | |
| """ | |
| Then the result for "overlap.txt" failed with code "C210" | |
| Scenario: overlapping line edits are rejected before write | |
| Given a jailed code surface | |
| And a file at "overlap.txt" with content: | |
| """ | |
| one | |
| two | |
| three | |
| """ | |
| When I call coder::update-file with payload: | |
| """ | |
| {"files":[{"path":"overlap.txt","ops":[{"op":"remove","from_line":1,"to_line":2},{"op":"update_lines","from_line":2,"to_line":3,"content":"x\n"}]}]} | |
| """ | |
| Then the result for "overlap.txt" failed with code "C210" | |
| And the file "overlap.txt" equals: | |
| """ | |
| one | |
| two | |
| three | |
| """ |
🤖 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/tests/features/coder/update_file.feature` around lines 42 - 55, The
update-file feature test for overlapping edits only asserts the C210 failure, so
it can still pass if the file is mutated before the error is raised. Strengthen
the Scenario: overlapping line edits are rejected before write by also asserting
the jailed file surface or overlap.txt content remains unchanged after
coder::update-file returns the C210 error, using the existing update-file flow
and file path to confirm the rejection happens before any write.
Comment on lines
+20
to
+45
| #[given("a live jailed shell code surface")] | ||
| async fn live_jailed_shell_code_surface(world: &mut CodeWorld) { | ||
| let Ok(url) = std::env::var("III_ENGINE_WS_URL") else { | ||
| world.soft_skip("III_ENGINE_WS_URL is not set"); | ||
| return; | ||
| }; | ||
|
|
||
| let client = Arc::new(iii_sdk::register_worker(&url, InitOptions::default())); | ||
| for _ in 0..20 { | ||
| let probe = client.trigger(TriggerRequest { | ||
| function_id: "coder::info".to_string(), | ||
| payload: json!({}), | ||
| action: None, | ||
| timeout_ms: Some(2_000), | ||
| }); | ||
| match tokio::time::timeout(Duration::from_secs(3), probe).await { | ||
| Ok(Ok(_)) => { | ||
| world.setup_live_client(client); | ||
| return; | ||
| } | ||
| Ok(Err(_)) | Err(_) => tokio::time::sleep(Duration::from_millis(250)).await, | ||
| } | ||
| } | ||
|
|
||
| world.soft_skip(format!("no live coder::* worker responded at {url}")); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail the live smoke when the probe never comes up.
shell/tests/e2e/run-tests-jailed.sh, Lines 124-127 now set III_ENGINE_WS_URL specifically to gate a live coder::* smoke. If this loop exhausts and calls soft_skip, that stage stops verifying the merged surface and can go green without ever talking to the engine.
🤖 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/tests/steps/common.rs` around lines 20 - 45, The live jailed shell
smoke currently soft-skips when no `coder::*` worker responds, which lets the
gated live check pass without validating the engine. Update
`live_jailed_shell_code_surface` to treat probe exhaustion as a test failure
instead of calling `world.soft_skip`, while keeping the existing
`III_ENGINE_WS_URL` env check and successful `client.trigger` path that calls
`world.setup_live_client`.
Comment on lines
+112
to
+113
| #[cfg(not(unix))] | ||
| fs::write(&link, b"outside").expect("create symlink fallback file"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The non-Unix fallback stops testing symlink escapes.
Writing a regular file here means the symlink escapes are rejected after canonicalization scenario no longer exercises canonicalization at all on non-Unix platforms. Use a platform symlink API there, or skip the scenario on platforms that cannot create one.
🤖 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/tests/steps/common.rs` around lines 112 - 113, The non-Unix fallback in
the symlink escape test is no longer exercising the canonicalization path
because it writes a regular file instead of creating a symlink. Update the setup
in the common test step so the `symlink escapes are rejected after
canonicalization` scenario uses a real platform symlink API where available, and
if that cannot be done on a target platform, skip this scenario there. Keep the
behavior aligned with the existing symlink-based test flow in the `common.rs`
step helpers.
Comment on lines
+602
to
+607
| fn assert_error_code(err: &str, code: &str) { | ||
| assert!( | ||
| err.contains(code), | ||
| "expected error to contain code {code}, got {err:?}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the structured error code, not a substring.
err.contains(code) can pass on the wrong failure if that token appears elsewhere in the JSON/message. These wire errors are structured already, so parse "code" directly.
Suggested fix
fn assert_error_code(err: &str, code: &str) {
- assert!(
- err.contains(code),
- "expected error to contain code {code}, got {err:?}"
- );
+ let actual = serde_json::from_str::<Value>(err)
+ .ok()
+ .and_then(|value| value.get("code").and_then(Value::as_str).map(str::to_owned))
+ .unwrap_or_else(|| panic!("missing structured error code in {err:?}"));
+ assert_eq!(actual, code, "expected error code {code}, got {err:?}");
}📝 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
| fn assert_error_code(err: &str, code: &str) { | |
| assert!( | |
| err.contains(code), | |
| "expected error to contain code {code}, got {err:?}" | |
| ); | |
| } | |
| fn assert_error_code(err: &str, code: &str) { | |
| let actual = serde_json::from_str::<Value>(err) | |
| .ok() | |
| .and_then(|value| value.get("code").and_then(Value::as_str).map(str::to_owned)) | |
| .unwrap_or_else(|| panic!("missing structured error code in {err:?}")); | |
| assert_eq!(actual, code, "expected error code {code}, got {err:?}"); | |
| } |
🤖 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/tests/steps/common.rs` around lines 602 - 607, The helper
assert_error_code currently checks err.contains(code), which can match the wrong
text in a structured wire error. Update assert_error_code to parse the error
payload as structured data and compare the actual "code" field directly instead
of searching the raw string. Use the existing assert_error_code function in
common.rs as the single place to make this validation more precise.
… presence
The working-directory picker browses via the shell-served `coder::*`
functions and scopes a chat to a directory the shell worker enforces, so it
only makes sense when shell is connected. Show the picker and its status
banner only when the shell worker is present, and drop the requirement to
choose a directory before sending — chats fall back to the default workspace.
- Extract the approval-gate presence probe into a reusable `useWorkerPresence`
hook: an initial `engine::workers::list` read plus a live `worker`
add/remove lifecycle trigger. Refactor `useApprovalGateStatus` to wrap it
and add `useShellStatus`.
- Expose `shellAvailable` through the conversations context; `ChatView` gates
the picker and banner on `backend.id === 'real' && shellAvailable`.
- Register the shell worker with `WorkerMetadata { name: "shell" }` so it is
discoverable in `engine::workers::list`; it otherwise connects unnamed and
the console can't detect it.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@console/web/src/hooks/use-worker-presence.ts`:
- Around line 42-58: The matching logic in eventMatchesWorker is too loose
because it uses includes() on evt.worker and JSON-stringified evt.source, which
can cause unrelated lifecycle events to match the wrong worker. Update
eventMatchesWorker to require exact worker identity matching against the
canonical worker name only, and remove or tighten the source-based substring
fallback so probes in use-worker-presence are only flipped by the intended
worker events.
- Around line 117-138: The initial probe in useWorkerPresence should handle all
failures and avoid overwriting newer lifecycle state with stale results. Move
the getIiiClient() call inside the same guarded async flow as checkWorkerPresent
so any bootstrap error still reaches the cleanup path and clears loading, and in
the useEffect for useWorkerPresence ensure the result is only applied when it
still matches the latest workerName/enabled state so an older snapshot cannot
clobber fresher add/remove updates.
🪄 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: 4395c6d2-1e2d-42d3-8c36-a0ba6842784b
📒 Files selected for processing (6)
console/web/src/components/chat/ChatView.tsxconsole/web/src/hooks/use-approval-gate-status.tsconsole/web/src/hooks/use-shell-status.tsconsole/web/src/hooks/use-worker-presence.tsconsole/web/src/lib/conversations-context.tsxshell/src/main.rs
Comment on lines
+42
to
+58
| /** Loose match: is this lifecycle event about the named worker? */ | ||
| function eventMatchesWorker(evt: WorkerEvent, workerName: string): boolean { | ||
| const needle = workerName.toLowerCase() | ||
| const w = typeof evt.worker === 'string' ? evt.worker.toLowerCase() : '' | ||
| if (w === needle || w.includes(needle)) { | ||
| return true | ||
| } | ||
| if (evt.source != null) { | ||
| try { | ||
| if (JSON.stringify(evt.source).toLowerCase().includes(needle)) { | ||
| return true | ||
| } | ||
| } catch { | ||
| // non-serialisable source; fall through | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match worker identities exactly.
includes() on evt.worker and the stringified source can let unrelated lifecycle events flip this probe. A shell watcher will also match names like shell-tools or any payload that merely mentions "shell", which can re-enable UI that still targets missing functions.
🤖 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 `@console/web/src/hooks/use-worker-presence.ts` around lines 42 - 58, The
matching logic in eventMatchesWorker is too loose because it uses includes() on
evt.worker and JSON-stringified evt.source, which can cause unrelated lifecycle
events to match the wrong worker. Update eventMatchesWorker to require exact
worker identity matching against the canonical worker name only, and remove or
tighten the source-based substring fallback so probes in use-worker-presence are
only flipped by the intended worker events.
Comment on lines
+117
to
+138
| useEffect(() => { | ||
| if (!enabled) { | ||
| setLoading(false) | ||
| setPresent(true) | ||
| return | ||
| } | ||
| let cancelled = false | ||
| void (async () => { | ||
| const client = await getIiiClient() | ||
| try { | ||
| const found = await checkWorkerPresent(client, workerName) | ||
| if (!cancelled) setPresent(found) | ||
| } catch { | ||
| if (!cancelled) setPresent(false) | ||
| } finally { | ||
| if (!cancelled) setLoading(false) | ||
| } | ||
| })() | ||
| return () => { | ||
| cancelled = true | ||
| } | ||
| }, [enabled, workerName]) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Harden the initial probe against failures and stale snapshots.
getIiiClient() is outside the try/finally, so a bootstrap failure leaves loading stuck true. Also, the list result is written back even if a newer add/remove event already landed, so an older snapshot can overwrite fresher lifecycle state until the next worker event.
🤖 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 `@console/web/src/hooks/use-worker-presence.ts` around lines 117 - 138, The
initial probe in useWorkerPresence should handle all failures and avoid
overwriting newer lifecycle state with stale results. Move the getIiiClient()
call inside the same guarded async flow as checkWorkerPresent so any bootstrap
error still reaches the cleanup path and clears loading, and in the useEffect
for useWorkerPresence ensure the result is only applied when it still matches
the latest workerName/enabled state so an older snapshot cannot clobber fresher
add/remove updates.
andersonleal
approved these changes
Jun 29, 2026
This was referenced Jul 3, 2026
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
2 participants
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
Merge the path-jailed
coderfile worker into theshellworker. Today anoperator must configure the same project root twice —
coder.base_pathsandshell.fs.host_root— plus duplicate the protected-paths and byte-cap settings,and the security-critical path-jail leaf is maintained as a byte-identical copy
in both crates. After this change there is one worker, one config, and
one jail for both command execution and code-file editing.
Targets
feat/per-session-working-dirbecause it builds directly on thatbranch's per-session
base_dirwork (the harness stamps onebase_dirontoboth
shell::*andcoder::*, which is exactly what this unifies).What changes
coder::*functions (info,read-file,search,list-folder,tree,create-file,update-file,delete-file,move)are served by
shellover the same jail. Function ids and theC2xxerrorcodes are unchanged; the standalone
coder/crate is deleted.(
canonicalize_with_fallback+normalize_lexical) is hoisted intoshell::pathand shared byshell::fsand the folded code resolver — the old"mirror-invariant" duplication is gone (the canonicalization parity vectors
migrated along with it).
fs.host_roots(a list);fs.host_rootis kept as a one-entrylegacy alias (setting both is a config error). Relative paths anchor at the
primary root; absolute paths are accepted inside any root; roots dedup
canonically. The exec allowlist guard now rejects a command path inside any
writable root, and
cwdconfinement inherits the multi-root jail.code.non_accessible_globsis honored by bothsurfaces — the code functions show-but-lock (
C211),shell::fs::*hard-rejects (
S215) — so secrets like.env/*.pemare declared once.fs.denylist_paths(absolute-prefix) remains as a separate hard layer.tree,search,list-folder,read-file) run off the async runtime viaspawn_blocking, soa large traversal can't stall
shell::exec/jobs/config-reload.coderconfiguration entry into theshellvalue at boot (best-effort,non-fatal). The inert
coderentry is left as the rollback artifact and theconsole tombstones it.
coder::*on the shellworker (no separate install); permissions, README, skill, and the seed
config.yamlare updated;coderis removed from the release CI;shellisbumped to
0.6.0with a CHANGELOG + migration note.Security review
A recall-mode review of the diff caught and fixed a real gap:
shell::fs::sedand
shell::fs::grepconfine paths inside aspawn_blockingclosure and sobypassed the
non_accessiblegate thatvalidate_path_scopedapplies — aprotected file could be modified via
sedor have its content leaked via adirectory
grep.sednow hard-rejects (S215),grepskips protected files,and the gate (extracted into a shared helper) now checks every containing
root, not just the first (closing a nested-root edge). Migration hardening from
the same pass: the "code untouched" comparison now covers the whole block (so a
tuned numeric knob isn't overwritten), and an unparseable stored config is no
longer silently overwritten.
Testing
shell: 1195 tests pass (multi-root confinement, exec-guard across roots,the D4 sed-reject / grep-skip / nested-root cases, the migrated parity
vectors, never-widen + idempotent migration, the tuned-knob regression, and a
critical regression that a pre-merge single-
host_rootconfig still boots andjails byte-identically).
harness: prompt tests pass (the code-routing guidance now points at theshell worker).
Migration notes
iii worker add shellbrings the whole surface; the standalonecoderworkeris retired. An existing
coderconfig entry folds intoshellautomaticallyon first boot.
iii worker restart shell(the source watcher does notalways restart the VM process).
Deferred (follow-up)
The code surface's numeric-knob hot-reload is intentionally left out: the code
jail is built once at boot, which is the original
coderdesign (the jail isnever rebuilt at runtime) and not a regression of
shell's fs jail, whichcontinues to reload live. Wiring knob hot-reload would mean moving the code
resolver into
AppState.Summary by CodeRabbit
coder::*) into the shell worker, enabling code browsing/editing from a single service.code.non_accessible_globs.coderworker entry and added shell availability gating; agent prompts now route throughcoder::*.coder::*behavior.coderworker; workflows and releases no longer reference it.