feat(shell,approval-gate)!: permissive coder, unified error taxonomy and redaction (MOT-4099, MOT-4104, MOT-4105) - #542
Merged
Merged
Conversation
With the operator opt-in (fs.allow_unjailed: true, empty fs.host_roots) the coder resolver now mirrors shell::fs::*: absolute paths anywhere on the host, the cwd + /tmp fallback roots demoted to relative-path anchors, and the harness-stamped fs_scope.root trusted as the anchor under the configured_roots boundary — matching shell::exec's cwd contract. fs.denylist_paths now applies to coder::* in every mode (redacted C211). Jailed deployments (explicit fs.host_roots) are unchanged, and the workspace boundary keeps its session scoping so the approval flow still triggers. Also closes the unjailed secrets gap in shell::fs::*: non_accessible_globs now match the root-stripped absolute form when no root contains the path, so **/.env-style protection holds with empty host_roots. coder::info gains a mode field (jailed | unjailed).
… posture (MOT-4104) Renumber the three drifted coder codes so equal digits mean the same failure class on both surfaces: already-exists C217 -> C213, too-large C213 -> C218, outside-session C218 -> C220. S-codes are untouched. approval-gate's JAIL_SCOPE_CODES tracks the C220 rename (lockstep release with shell 0.10.0). Unify existence redaction on coder's C211 invariant: shell::fs::* now folds permission-denied, protected-glob, and fs.denylist_paths rejections into S211 with the single 'not found or not accessible' wording. S215 is exclusively a jail-confinement escape (it keeps carrying the filesystem_access_request hint that drives the folder-approval flow). shell 0.9.2 -> 0.10.0, approval-gate 1.0.8 -> 1.0.9; README tables, upgrade notes, and CHANGELOG updated; goldens re-blessed.
…ions (MOT-4105) Every shell::fs::* description now points at its coder::* twin (the reverse link already existed), with the error-code hints updated to the 0.10.0 semantics (S211 not-found-or-not-accessible, S215 jail escape only). README gains a 'Two surfaces, one contract' section: the twin-operation table, the shared code/redaction/batching/naming conventions, and the discovery map.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR releases ChangesAccess contract and filesystem behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Configuration
participant PathResolver
participant FilesystemSurface
participant CoderInfo
Configuration->>PathResolver: Configure jailed or unjailed mode
PathResolver->>FilesystemSurface: Resolve paths with denylist and glob checks
FilesystemSurface-->>PathResolver: Return resolved path or redacted S211
PathResolver->>CoderInfo: Expose unjailed state
CoderInfo-->>Configuration: Return mode as jailed or unjailed
Possibly related PRs
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, 47 skipped (no docs/).
Four for four. Nicely done. |
…action The three harness E2E cases that pinned denylist rejections as S215 now expect the redacted S211 (denylisted reads exactly like missing; S215 is jail-escape only). Symlink jail-escape cases keep S215 — unchanged.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shell/src/fs/host.rs (1)
545-584: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDenylist check must run before the
is_dircheck to actually satisfy the redaction invariant.Right now a missing
scope_roothits the!canon.is_dir()branch (S212) before the denylist loop ever runs, while an existing denylisted directory reaches the denylist loop and returns S211. That lets a caller distinguish "directory exists and is denylisted" (S211) from "path doesn't exist" (S212) — the exact probe the comment on lines 578-580 says this code prevents.shell::exec::policy::confine_scope_root(same PR) gets this right by running the denylist loop first, so denylisted-or-missing both collapse to S211.🔒 Proposed fix: check denylist before directory type
let canon = canonicalize_with_fallback(p).map_err(|e| { let msg = format!("{e}"); if msg.contains("dangling symlink in path") { FsError::new("S215", format!("{scope_root}: {msg}")) } else { FsError::new("S210", format!("{scope_root}: {msg}")) } })?; - if !canon.is_dir() { - return Err(FsError::new( - "S212", - format!("scope_root is not a directory: {scope_root}"), - )); - } for deny_canon in denylist_canon { if canon.starts_with(deny_canon) { // REDACTION INVARIANT: denylisted folds into the same S211 as // missing, so callers cannot probe operator-denied directories. return Err(FsError::not_found_or_denied(scope_root)); } } + if !canon.is_dir() { + return Err(FsError::new( + "S212", + format!("scope_root is not a directory: {scope_root}"), + )); + } Ok(Some(canon))🤖 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/fs/host.rs` around lines 545 - 584, Move the denylist loop in confine_scope_root to immediately after canonicalization and before the canon.is_dir() check, so denylisted paths consistently return FsError::not_found_or_denied (S211) whether the target exists or is missing. Preserve the existing S212 response for non-denylisted paths that are not directories.
🧹 Nitpick comments (4)
shell/src/code/error.rs (1)
261-278: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate test names to reflect the new error code.
The error code for
AlreadyExistswas updated fromC217toC213, but the test names still mentionc217.♻️ Proposed refactor
- fn io_for_path_already_exists_maps_to_c217_with_path_prefix() { + fn io_for_path_already_exists_maps_to_c213_with_path_prefix() { let e = CoderError::io_for_path( std::io::Error::new(std::io::ErrorKind::AlreadyExists, "exists"), "some/file.txt", ); assert_eq!(e.code(), "C213"); assert!( e.message().starts_with("some/file.txt: "), "C213 via io_for_path must prefix the caller path: {}", e.message() ); } #[test] - fn io_already_exists_maps_to_c217() { + fn io_already_exists_maps_to_c213() { let e: CoderError = std::io::Error::new(std::io::ErrorKind::AlreadyExists, "x").into(); assert_eq!(e.code(), "C213"); }🤖 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/error.rs` around lines 261 - 278, Rename the tests io_for_path_already_exists_maps_to_c217_with_path_prefix and io_already_exists_maps_to_c217 to use c213, matching the assertions and current AlreadyExists error code; leave their test logic unchanged.shell/src/code/path.rs (1)
1204-1205: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale
_c218suffixes in test names now assertingC220.
resolve_in_absolute_inside_root_but_outside_scope_root_rejected_c218_naming_session,resolve_in_absolute_in_other_root_outside_scope_root_is_c218, andresolve_in_dotdot_within_root_but_outside_base_is_c218all asserterr.code() == "C220"but their names still sayc218— leftover from the pre-renumbering code. Purely cosmetic (grep-for-code-name confusion), no behavior impact.♻️ Suggested rename
- fn resolve_in_absolute_inside_root_but_outside_scope_root_rejected_c218_naming_session() { + fn resolve_in_absolute_inside_root_but_outside_scope_root_rejected_c220_naming_session() { ... - fn resolve_in_absolute_in_other_root_outside_scope_root_is_c218() { + fn resolve_in_absolute_in_other_root_outside_scope_root_is_c220() { ... - fn resolve_in_dotdot_within_root_but_outside_base_is_c218() { + fn resolve_in_dotdot_within_root_but_outside_base_is_c220() {Also applies to: 1240-1241, 1367-1368
🤖 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/path.rs` around lines 1204 - 1205, Rename the three tests—resolve_in_absolute_inside_root_but_outside_scope_root_rejected_c218_naming_session, resolve_in_absolute_in_other_root_outside_scope_root_is_c218, and resolve_in_dotdot_within_root_but_outside_base_is_c218—to use the C220 suffix, matching their err.code() assertions. Do not change test behavior or assertions.shell/src/fs/host.rs (1)
453-482: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting this glob fallback into a shared helper.
This root-relative-else-absolute-form matching logic is functionally duplicated in the coder path resolver (per the comment "the same fallback the coder resolver uses"). The MOT-4099 gap this PR fixes had to be independently found and patched in both places — a shared helper (crate-internal fs-utils) for "match glob against root-relative form, falling back to absolute-form when uncontained" would remove that drift risk for future policy changes.
🤖 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/fs/host.rs` around lines 453 - 482, Extract the uncontained-path glob fallback from path_is_non_accessible into a shared crate-internal fs utility, then update both path_is_non_accessible and the coder path resolver to use it. Preserve root-relative matching for contained paths and absolute-form matching only when no configured root contains the path, including the existing slash normalization and empty-path handling.shell/src/code/functions/read_file.rs (1)
1496-1496: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest names contain outdated error codes.
The assertions and documentation correctly test for the new error codes, but the corresponding test function names still include the old codes from before the taxonomy rename.
shell/src/code/functions/read_file.rs#L1496-L1496: rename the testbatch_zero_budget_entry_c213_names_key_and_valueto usec218.shell/src/code/functions/read_file.rs#L2087-L2087: rename the testfull_read_over_output_budget_returns_recovery_c213to usec218.shell/src/code/functions/read_file.rs#L2189-L2189: rename the testdenied_huge_file_is_c211_not_c213to usec218.shell/src/code/functions/move_file.rs#L699-L699: rename the testoverwrite_false_dst_exists_c217to usec213.🤖 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/read_file.rs` at line 1496, Rename the outdated test functions to match their current error codes: in shell/src/code/functions/read_file.rs at lines 1496-1496, 2087-2087, and 2189-2189, change c213 to c218 in the named tests; in shell/src/code/functions/move_file.rs at line 699, change overwrite_false_dst_exists_c217 to overwrite_false_dst_exists_c213. No assertion or implementation changes are needed.
🤖 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 189-198: Add the missing S220 entry to the README’s S2xx Errors
table, documenting it as the fs twin for C220 with the same scope-root violation
meaning. Place it between S218 and S300 so the existing C220/S220
cross-reference and JAIL_SCOPE_CODES documentation are complete.
In `@shell/src/code/functions/info.rs`:
- Around line 156-160: Update the resolver root initialization around
primary_root and the AccessMode selection to handle an empty resolver.roots()
collection when resolver.unjailed() is true. Avoid indexing base_paths[0] in
that case, while preserving the existing primary-root behavior whenever roots
are available and the jailed-mode behavior remains unchanged.
In `@shell/src/main.rs`:
- Around line 602-603: Update the `shell::fs::grep` contract description near
the `code` and `message` fields to document S211 for missing, denied, protected,
and unjailed glob-matched paths, while preserving the existing S217 and S215
descriptions.
---
Outside diff comments:
In `@shell/src/fs/host.rs`:
- Around line 545-584: Move the denylist loop in confine_scope_root to
immediately after canonicalization and before the canon.is_dir() check, so
denylisted paths consistently return FsError::not_found_or_denied (S211) whether
the target exists or is missing. Preserve the existing S212 response for
non-denylisted paths that are not directories.
---
Nitpick comments:
In `@shell/src/code/error.rs`:
- Around line 261-278: Rename the tests
io_for_path_already_exists_maps_to_c217_with_path_prefix and
io_already_exists_maps_to_c217 to use c213, matching the assertions and current
AlreadyExists error code; leave their test logic unchanged.
In `@shell/src/code/functions/read_file.rs`:
- Line 1496: Rename the outdated test functions to match their current error
codes: in shell/src/code/functions/read_file.rs at lines 1496-1496, 2087-2087,
and 2189-2189, change c213 to c218 in the named tests; in
shell/src/code/functions/move_file.rs at line 699, change
overwrite_false_dst_exists_c217 to overwrite_false_dst_exists_c213. No assertion
or implementation changes are needed.
In `@shell/src/code/path.rs`:
- Around line 1204-1205: Rename the three
tests—resolve_in_absolute_inside_root_but_outside_scope_root_rejected_c218_naming_session,
resolve_in_absolute_in_other_root_outside_scope_root_is_c218, and
resolve_in_dotdot_within_root_but_outside_base_is_c218—to use the C220 suffix,
matching their err.code() assertions. Do not change test behavior or assertions.
In `@shell/src/fs/host.rs`:
- Around line 453-482: Extract the uncontained-path glob fallback from
path_is_non_accessible into a shared crate-internal fs utility, then update both
path_is_non_accessible and the coder path resolver to use it. Preserve
root-relative matching for contained paths and absolute-form matching only when
no configured root contains the path, including the existing slash normalization
and empty-path handling.
🪄 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: b2eb8b66-9fa5-4b6d-9ec3-317868f50a35
⛔ Files ignored due to path filters (2)
approval-gate/Cargo.lockis excluded by!**/*.lockshell/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (39)
approval-gate/Cargo.tomlapproval-gate/README.mdapproval-gate/src/filesystem_access.rsapproval-gate/src/types.rsapproval-gate/tests/golden/schemas/approval.get-pending.jsonapproval-gate/tests/golden/schemas/approval.list-pending.jsonapproval-gate/tests/golden/schemas/approval.pending-created.jsonshell/CHANGELOG.mdshell/Cargo.tomlshell/README.mdshell/src/code/config.rsshell/src/code/error.rsshell/src/code/functions/create_file.rsshell/src/code/functions/info.rsshell/src/code/functions/mod.rsshell/src/code/functions/move_file.rsshell/src/code/functions/read_file.rsshell/src/code/path.rsshell/src/config.rsshell/src/configuration.rsshell/src/exec/policy.rsshell/src/fs/error.rsshell/src/fs/host.rsshell/src/functions/workspace.rsshell/src/main.rsshell/tests/code_golden_errors.rsshell/tests/code_path_jail.rsshell/tests/code_unified_protection.rsshell/tests/e2e/workers/harness/src/cases-fs-host-jail.tsshell/tests/e2e/workers/harness/src/cases-fs-protocol-break.tsshell/tests/features/coder/create_file.featureshell/tests/features/coder/move.featureshell/tests/features/coder/path_security.featureshell/tests/features/coder/read_file.featureshell/tests/golden/errors.jsonshell/tests/golden/schemas/coder.create-file.jsonshell/tests/golden/schemas/coder.info.jsonshell/tests/golden/schemas/coder.move.jsonshell/tests/golden/schemas/coder.read-file.json
Comment on lines
+189
to
198
| | Code | Meaning | fs twin | | ||
| |---|---|---| | ||
| | `C210` | Malformed input: bad payload, illegal line numbers, overlapping ops. | `S210` | | ||
| | `C211` | Path not found, permission denied, matched `non_accessible_globs`, or under `fs.denylist_paths` — deliberately ONE code and wording for all four, so a caller can't probe for a denied path's existence. | `S211` | | ||
| | `C213` | `create-file`/`move` saw an existing target and `overwrite=false`. | `S213` | | ||
| | `C215` | Path escapes every allowed root, lexically or through a symlink (jailed mode only). | `S215` | | ||
| | `C216` | Underlying I/O error. | `S216` | | ||
| | `C218` | File exceeds `max_read_bytes`/`max_write_bytes`. | `S218` | | ||
| | `C220` | Path resolves inside a configured root but outside the per-call `scope_root` the session is scoped to. | `S220` | | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
S220 is referenced as C220's "fs twin" but never documented in the ## Errors table.
Line 197 names S220 as C220's fs twin, and approval-gate's JAIL_SCOPE_CODES (["S215", "S220", "C215", "C220"]) confirms it's a live code — but the S2xx table below (lines 256-269) jumps from S218 straight to S300 with no S220 entry, leaving the "fs twin" cross-reference dangling for readers.
📝 Suggested addition to the S2xx table
| `S218` | `fs.max_read_bytes` / `fs.max_write_bytes` cap exceeded. |
+| `S220` | Path resolves inside the `fs.host_roots` jail but outside the per-call `cwd`/session scope. |
| `S300` | Sandbox VM boot failed (needs a virtualization host: Apple Silicon or `/dev/kvm`). |Also applies to: 256-269
🤖 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 189 - 198, Add the missing S220 entry to the
README’s S2xx Errors table, documenting it as the fs twin for C220 with the same
scope-root violation meaning. Place it between S218 and S300 so the existing
C220/S220 cross-reference and JAIL_SCOPE_CODES documentation are complete.
Comment on lines
+156
to
+160
| mode: if resolver.unjailed() { | ||
| AccessMode::Unjailed | ||
| } else { | ||
| AccessMode::Jailed | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Panic when fs.host_roots is empty.
The PR objective and AccessMode::Unjailed documentation indicate that fs.host_roots can be empty in unjailed mode. If resolver.roots() is empty, the upstream assignment at line 153 (let primary_root = base_paths[0].clone();) will panic.
🐛 Proposed fix
- let primary_root = base_paths[0].clone();
+ let primary_root = base_paths.first().cloned().unwrap_or_default();📝 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
| mode: if resolver.unjailed() { | |
| AccessMode::Unjailed | |
| } else { | |
| AccessMode::Jailed | |
| }, | |
| let primary_root = base_paths.first().cloned().unwrap_or_default(); |
🤖 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/info.rs` around lines 156 - 160, Update the resolver
root initialization around primary_root and the AccessMode selection to handle
an empty resolver.roots() collection when resolver.unjailed() is true. Avoid
indexing base_paths[0] in that case, while preserving the existing primary-root
behavior whenever roots are available and the jailed-mode behavior remains
unchanged.
Comment on lines
+602
to
+603
| { code, message }; common: S217 bad regex, S215 jail escape. For token-budgeted search with \ | ||
| context lines and noise filtering, prefer coder::search." |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Document S211 redaction for shell::fs::grep.
The updated contract redacts missing, denied, and protected paths—including unjailed glob matches—as S211, but this description only advertises S217 and S215. Add the S211 cases so callers can handle expected redacted failures correctly.
🤖 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 602 - 603, Update the `shell::fs::grep`
contract description near the `code` and `message` fields to document S211 for
missing, denied, protected, and unjailed glob-matched paths, while preserving
the existing S217 and S215 descriptions.
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.
Implements MOT-4099, MOT-4104, MOT-4105 — one commit per ticket.
Context
On the shipped permissive default (
fs.allow_unjailed: true, emptyfs.host_roots),shell::exec/shell::fs::*reach the whole filesystem whilecoder::*stayed jailed to engine-cwd +/tmp. Picking a harness working directory outside the engine cwd made everycoder::*call fail withscope_root is outside every allowed root(Anthony's report), and models routed around it viashell::exec. A follow-up DX audit found the drift is systemic: colliding error codes, opposite existence-redaction postures, one-way cross-referencing, and a latent gap where unjailedshell::fs::*skippednon_accessible_globsentirely.Changes
1.
coder::*follows the unjailed deny-only policy — MOT-4099 (bee3f8f3)fs.allow_unjailed: true+ emptyfs.host_roots), the coder resolver mirrorsshell::fs::*: absolute paths anywhere on the host; the cwd +/tmpfallback roots demoted to relative-path anchors; the harness-stampedfs_scope.roottrusted as the anchor under theconfigured_rootsboundary (matchingshell::exec's cwd contract).workspaceboundary keeps session scoping, so the folder-approval flow still triggers where approval-gate is installed. Jailed deployments (explicitfs.host_roots) are byte-for-byte unchanged.fs.denylist_pathsnow applies tocoder::*in every mode (redactedC211).non_accessible_globsnow match the root-stripped absolute form when no root contains the path — previously unjailedshell::fs::*silently skipped the glob check.coder::inforeports the effectivemode: jailed | unjailed.2. One error taxonomy + one redaction posture — MOT-4104 (
4d6480bb) BREAKINGCoder-only renames so equal digits mean the same failure class on both surfaces (S-codes untouched):
shell::fs::*adopts coder's redaction invariant: permission-denied, protected-glob, andfs.denylist_pathsrejections fold intoS211with the single "not found or not accessible" wording.S215is now exclusively a jail-confinement escape (it keeps carrying thefilesystem_access_requesthint that drives the approval flow).approval-gate
JAIL_SCOPE_CODES:C218→C220.3. Two-way discoverability + conventions — MOT-4105 (
268f0edb)shell::fs::*description now points at itscoder::*twin (the reverse link already existed), with error hints updated to the new semantics.Deploy note — lockstep pair
shell/v0.10.0andapproval-gate/v1.0.9must ship in the same release wave: a stale approval-gate will not prompt onC220coder session-escapes until upgraded (accepted trade-off; no dual-accept bridge).Verification
S215land-gate matcher is unaffected — S-codes unchanged); clippy clean.coder::treeworks; console badge; approval prompt onC220in jailed mode). Recommend before cutting the release tags.Summary by CodeRabbit
New Features
coder::infonow reporting the effective mode.Bug Fixes
S211errors.Documentation