feat(shell): unix shell + filesystem worker with typed RegisterFunction + E2E CI - #80
Merged
Conversation
New `shell` worker exposing 15 functions: 5 shell::* (exec, exec_bg,
kill, status, list) and 10 shell::fs::* (ls, stat, mkdir, rm, chmod,
mv, grep, sed, write, read). Wire-compatible with the engine
daemon's sandbox::fs::* surface; sandbox-target requests forward
through `iii.trigger` with the caller's StreamChannelRef flowing
verbatim (worker is never in the byte path).
Built on iii-sdk 0.11.3's typed `RegisterFunction::new_async`. shell::
kill / shell::status take typed request structs; the other 13
handlers accept Value at the registration boundary to preserve
legacy wire-error contracts (S210 for malformed fs payloads,
"missing 'command'" / "must be a string" for exec, silent
timeout_ms fallback) that schemars-derived deserialization can't
reproduce. schemars 0.8 with the uuid1 feature is added because
Target carries a Uuid.
Security model:
- Allowlist: command basename gate (empty list = open).
- Denylist patterns: advisory tripwires only — regex on
argv.join(" "), trivially bypassable once any shell or
interpreter is allowlisted (variable construction, eval, IFS).
README documents this explicitly.
- host_root jail: validate_path canonicalizes every existing
ancestor (resolving symlinks), lexically collapses the
non-existent tail, and reflects each tail component through
symlink_metadata to refuse dangling-symlink jail escapes. Both
host_root and denylist entries are precomputed at startup
(HostFsBackend::try_new); zero canonicalize syscalls per fs op.
- allow_unjailed: explicit opt-in for running with host_root null.
Worker refuses to start otherwise (validate_fs_jail), in both
load_config success and fallback paths.
- chmod recursive walk skips symlink entries; refuses with S212
if the walk root itself is a symlink (no silent no-op).
- shell::list returns JobSummary records (id, status, timestamps,
exit_code, *_truncated). argv/stdout/stderr cap-gated via
shell::status by random UUID job_id — the global JOBS map has
no per-caller scope.
- exec_bg uses atomic try_reserve_and_insert: counts running
handles under the map mutex, inserts only if under the cap,
conservatively counts locked handles as running. Rejection
returns the spawned child so it can be killed; no zombies.
- max_concurrent_jobs cap, max_output_bytes truncation, timeout
clamp, env scrubbing, working_dir pin, optional fs read/write
caps, atomic write via temp+rename.
Tests:
- 277 unit + integration tests across 7 suites (lib, jobs lifecycle,
host fs branches, sandbox dispatch, function handlers, config
validation, manifest).
- E2E TS harness: 144 default cases (happy paths, safety, jobs,
fs host + sandbox, encoding, concurrency, adversarial protocol
breaks for streaming/exec/jobs/fs/sandbox, plus 5 vuln-regression
cases under cases-vuln-repro.ts).
- Jailed e2e: 2 cases under cases-vuln-repro-jailed.ts covering
the symlink-parent escape (validate_path site + parents:true
defense-in-depth site), runs against config-jailed.yaml via
run-tests-jailed.sh.
README documents the full surface, threat model,
allow_unjailed contract, JobSummary redaction, and the canonicalize-
with-fallback semantics.
Runs the shell tests/e2e harness on PRs touching shell/**. Two suites: - ./run-tests.sh — 144 default cases (host_root unset, allow_unjailed=true) - ./run-tests-jailed.sh --no-build — 2 cases for S-C1 (host_root set, reusing the binary the default run built) Pre-creates /private/tmp 1777 on the Linux runner because the jailed suite hardcodes JAIL_ROOT=/private/tmp/iii-shell-jailed-root (macOS's canonical /tmp), which doesn't exist on Ubuntu.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis pull request introduces ChangesShell Worker Implementation
Sequence DiagramssequenceDiagram
actor Client
participant Handler as shell::exec Handler
participant Config as ShellConfig
participant Exec as exec::run_to_completion
participant Process as tokio::process::Command
Client->>Handler: call(payload: {command, args?, timeout_ms?})
Handler->>Handler: parse command + args → argv
Handler->>Config: is_command_allowed(argv)
alt Command denied
Handler-->>Client: Error (not in allowlist/denylist)
else Command allowed
Handler->>Config: resolve_timeout(requested)
Handler->>Exec: run_to_completion(argv, cfg, timeout_ms)
Exec->>Process: spawn Command
Exec->>Exec: read_bounded(stdout, max_bytes)
Exec->>Exec: read_bounded(stderr, max_bytes)
Exec->>Process: wait with Tokio timeout
alt Timeout exceeded
Exec->>Process: kill child
Exec-->>Handler: ExecOutcome {timed_out: true, exit_code: None}
else Completed in time
Exec-->>Handler: ExecOutcome {exit_code, stdout, stderr, ...}
end
Handler-->>Client: ExecResponse {exit_code, stdout, stderr, timed_out, ...}
end
sequenceDiagram
actor Client
participant Handler as shell::fs::read Handler
participant Dispatch as fs_dispatch::pick_backend
participant HostBackend as HostFsBackend
participant Channel as iii SDK Channel
participant File as Host Filesystem
Client->>Handler: call(payload: {target, path})
Handler->>Dispatch: pick_backend(target, host, iii, sandbox_enabled)
alt Target = Host
Dispatch-->>Handler: Arc<HostFsBackend>
else Target = Sandbox {id}
Dispatch-->>Handler: Arc<SandboxFsBackend>
end
Handler->>HostBackend: read(ReadArgs {path})
HostBackend->>HostBackend: validate_path(path) → canonical
HostBackend->>HostBackend: check max_read_bytes
HostBackend->>Channel: create_channel()
HostBackend->>HostBackend: spawn pump_file_to_channel task
par File pump
HostBackend->>File: open(path)
loop read chunks
File-->>Channel: write bytes
end
File-->>Channel: close writer on EOF
and Main flow
HostBackend-->>Handler: ReadResponse {content: StreamChannelRef, size, mode, mtime}
end
Handler-->>Client: ReadResponseWire {content: ContentRef, size, mode, mtime}
sequenceDiagram
actor Client
participant Handler as shell::exec_bg Handler
participant Jobs as jobs registry
participant JobTask as background task
participant Process as tokio::process::Command
Client->>Handler: call(payload: {command, args?})
Handler->>Handler: parse argv + validate allowlist
Handler->>Process: spawn Command
Handler->>Jobs: try_reserve_and_insert(JobHandle {record, child})
alt Concurrency cap hit
Handler->>Process: kill spawned child
Handler-->>Client: Error (max concurrent jobs)
else Reserved & inserted
Jobs-->>Handler: job_id
par Background pump (task)
JobTask->>Process: read_bounded(stdout)
JobTask->>Process: read_bounded(stderr)
JobTask->>Process: wait for exit
JobTask->>Jobs: update record {status: Finished, exit_code, output, finished_at_ms}
and Immediate response
Handler-->>Client: ExecBgResponse {job_id, argv}
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This is a substantial new worker implementation spanning ~5500 lines across configuration, multiple Rust modules (config, exec, jobs, filesystem backends, API handlers), worker integration, and comprehensive e2e tests in TypeScript. While the code exhibits consistent patterns (filesystem handlers follow repetitive structure, many e2e test cases follow similar templates), the reviewer must understand:
The PR is well-organized and extensively tested, reducing ambiguity, but the breadth of concerns and interaction surface area warrants careful review of security boundaries and error handling paths. Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
…ability - Compact multi-line function signatures and error handling into single-line equivalents. - Simplified string formatting in error messages. - No functional changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (8)
shell/tests/e2e/workers/harness/src/cases-fs-host-jail.ts (1)
3-44: 💤 Low valueConsider adding a positive "allowed path" case to the jailed suite.
All four test cases only verify that disallowed inputs are rejected. A single case asserting that a path inside
/private/tmp/iii-shell-jailed-rootis accepted (e.g., astatorlsof a file pre-created by the CI job) would guard against accidental regression where the jailed worker rejects all requests.🤖 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/e2e/workers/harness/src/cases-fs-host-jail.ts` around lines 3 - 44, Add a positive "allowed path" test to FS_HOST_JAIL_CASES that calls the jailed worker (use ctx.call with 'shell::fs::stat' or 'shell::fs::ls') on a known CI-precreated path under /private/tmp/iii-shell-jailed-root (e.g. '/private/tmp/iii-shell-jailed-root/somefile') and assert success (e.g. await the call and assert the returned stat/entries are truthy) so the suite verifies permitted paths are accepted; locate FS_HOST_JAIL_CASES and append a case object with a unique name like 'fs_host_allowed_private_tmp_stat' using the CaseContext ctx to perform the positive check.shell/src/functions/fs_sed.rs (1)
9-20: 💤 Low valueAll ten
fs_*.rshandlers share identical boilerplate — a macro would eliminate the duplication.With 10 near-identical files (
fs_sed,fs_chmod,fs_write,fs_stat,fs_ls,fs_rm,fs_mkdir,fs_mv,fs_grep,fs_read), any change to theFsBackendmethod signature, thepick_backendcall site, or the error-mapping strategy requires the same edit in all ten places.A
macro_rules!invocation centralises the pattern:♻️ Suggested macro (in a shared module, e.g.,
functions/macros.rs)+/// Generates the standard thin `handle` entrypoint for filesystem handlers. +macro_rules! fs_handler { + ($req:ty, $resp:ty, $method:ident, $msg:literal) => { + pub async fn handle( + host: std::sync::Arc<dyn crate::fs::FsBackend>, + iii: iii_sdk::III, + sandbox_enabled: bool, + payload: serde_json::Value, + ) -> Result<$resp, String> { + let req: $req = serde_json::from_value(payload).map_err(|e| { + crate::fs::error::FsError::new("S210", format!("{}: {e}", $msg)).to_json() + })?; + let (target, args) = req.split(); + let backend = crate::functions::fs_dispatch::pick_backend( + target, host, iii, sandbox_enabled, + ); + backend.$method(args).await.map_err(crate::functions::fs_dispatch::err_to_string) + } + }; +} +pub(crate) use fs_handler;Each handler file then becomes a one-liner:
-use std::sync::Arc; -use serde_json::Value; -use crate::fs::error::FsError; -use crate::fs::{FsBackend, SedRequest, SedResponse}; -use crate::functions::fs_dispatch::{err_to_string, pick_backend}; - -pub async fn handle( - host: Arc<dyn FsBackend>, - iii: iii_sdk::III, - sandbox_enabled: bool, - payload: Value, -) -> Result<SedResponse, String> { - let req: SedRequest = serde_json::from_value(payload) - .map_err(|e| FsError::new("S210", format!("bad sed payload: {e}")).to_json())?; - let (target, args) = req.split(); - let backend = pick_backend(target, host, iii, sandbox_enabled); - backend.sed(args).await.map_err(err_to_string) -} +crate::functions::macros::fs_handler!( + crate::fs::SedRequest, + crate::fs::SedResponse, + sed, + "bad sed payload" +);🤖 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/functions/fs_sed.rs` around lines 9 - 20, Multiple fs_* handlers repeat the same boilerplate: parse payload into a request type, call req.split(), pick_backend(target, host, iii, sandbox_enabled), invoke backend.<method>(args).await and map_err(err_to_string); replace this pattern with a macro to avoid duplication. Add a macro_rules! (e.g., in functions::macros.rs) that accepts the handler name, request type (e.g., SedRequest), backend method name (e.g., sed), and response type (e.g., SedResponse) and expands to the async fn handle(host: Arc<dyn FsBackend>, iii: iii_sdk::III, sandbox_enabled: bool, payload: Value) -> Result<RespType, String> { let req: ReqType = serde_json::from_value(payload).map_err(|e| FsError::new("S210", format!("bad {} payload: {e}")).to_json())?; let (target, args) = req.split(); let backend = pick_backend(target, host, iii, sandbox_enabled); backend.method(args).await.map_err(err_to_string) }. Then replace each fs_*.rs file’s handle implementation with a single macro invocation using the appropriate ReqType, method name, and RespType (e.g., SedRequest, sed, SedResponse), keeping existing symbols like pick_backend, FsError, err_to_string and FsBackend intact.shell/src/functions/fs_mkdir.rs (1)
9-20: 🏗️ Heavy liftConsider collapsing all
fs_*handlers into a singlemacro_rules!.All six handlers in this PR —
fs_mkdir,fs_grep,fs_ls,fs_rm,fs_mv,fs_read— and the four implied by the PR description (fs_stat,fs_chmod,fs_sed,fs_write) are structurally identical. The only per-file variation is the request/response types, the method name dispatched on the backend, and the error-message string. Any future change to the dispatch pattern, error code, or function signature must be applied to all ten files independently.A single
macro_rules!infs_dispatch.rs(or a shared module) can generate all ten:♻️ Proposed macro-based refactor
// In shell/src/functions/fs_dispatch.rs (or a new fs_handlers.rs) +/// Generates a standard `pub async fn handle(...)` for a filesystem operation. +/// The optional `$convert` arm handles types like `ReadResponseWire` that need +/// an `.into()` conversion from the backend's response type. +macro_rules! fs_handle { + // Without wire-type conversion (most operations) + ($req:ty, $resp:ty, $method:ident, $op_name:literal) => { + pub async fn handle( + host: std::sync::Arc<dyn crate::fs::FsBackend>, + iii: iii_sdk::III, + sandbox_enabled: bool, + payload: serde_json::Value, + ) -> Result<$resp, String> { + let req: $req = serde_json::from_value(payload).map_err(|e| { + crate::fs::error::FsError::new("S210", format!("bad {} payload: {e}", $op_name)) + .to_json() + })?; + let (target, args) = req.split(); + let backend = crate::functions::fs_dispatch::pick_backend(target, host, iii, sandbox_enabled); + backend.$method(args).await.map_err(crate::functions::fs_dispatch::err_to_string) + } + }; + // With explicit wire-type conversion (e.g., read → ReadResponseWire) + ($req:ty, $inner_resp:ty => $wire_resp:ty, $method:ident, $op_name:literal) => { + pub async fn handle( + host: std::sync::Arc<dyn crate::fs::FsBackend>, + iii: iii_sdk::III, + sandbox_enabled: bool, + payload: serde_json::Value, + ) -> Result<$wire_resp, String> { + let req: $req = serde_json::from_value(payload).map_err(|e| { + crate::fs::error::FsError::new("S210", format!("bad {} payload: {e}", $op_name)) + .to_json() + })?; + let (target, args) = req.split(); + let backend = crate::functions::fs_dispatch::pick_backend(target, host, iii, sandbox_enabled); + let resp = backend.$method(args).await.map_err(crate::functions::fs_dispatch::err_to_string)?; + Ok(resp.into()) + } + }; +} +pub(crate) use fs_handle;Each individual
fs_*.rsfile then reduces to:// fs_mkdir.rs -use std::sync::Arc; -use serde_json::Value; -use crate::fs::error::FsError; -use crate::fs::{FsBackend, MkdirRequest, MkdirResponse}; -use crate::functions::fs_dispatch::{err_to_string, pick_backend}; - -pub async fn handle( - host: Arc<dyn FsBackend>, - iii: iii_sdk::III, - sandbox_enabled: bool, - payload: Value, -) -> Result<MkdirResponse, String> { - let req: MkdirRequest = serde_json::from_value(payload) - .map_err(|e| FsError::new("S210", format!("bad mkdir payload: {e}")).to_json())?; - let (target, args) = req.split(); - let backend = pick_backend(target, host, iii, sandbox_enabled); - backend.mkdir(args).await.map_err(err_to_string) -} +crate::functions::fs_dispatch::fs_handle!(MkdirRequest, MkdirResponse, mkdir, "mkdir"); // fs_read.rs -// (current 21-line file) +crate::functions::fs_dispatch::fs_handle!(ReadRequest, ReadResponse => ReadResponseWire, read, "read");🤖 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/functions/fs_mkdir.rs` around lines 9 - 20, The six nearly-identical handler functions (e.g., fs_mkdir::handle) should be generated by a single macro instead of duplicated: create a macro_rules! (e.g., in fs_dispatch.rs) that accepts the handler function name, request/response types (e.g., MkdirRequest, MkdirResponse), and the backend method identifier (e.g., mkdir) and emits the async fn handle(host: Arc<dyn FsBackend>, iii: iii_sdk::III, sandbox_enabled: bool, payload: Value) -> Result<Res, String> body which deserializes with serde_json::from_value into the Req type, calls req.split(), selects the backend via pick_backend, invokes backend.<method>(args).await, and maps errors with err_to_string (preserve FsError mapping behavior if needed); then replace each fs_* file’s duplicated handle implementation with a single invocation of that macro providing the correct types and method names (e.g., MkdirRequest/MkdirResponse/mkdir) so all handlers (handle functions) share the same generated code and signature.shell/tests/e2e/workers/harness/src/cases-fs-encoding.ts (1)
89-114: ⚡ Quick winGrep truncation test doesn't assert the byte-length boundary
The check
buf.toString('utf8') === contentvalidates that the JSON-decoded JS string is encodable to UTF-8 cleanly (which is always true for any valid JS string received from JSON). It does not verify that the truncated content is actually ≤max_line_bytes(50) bytes, so the test name's promise of "floors_to_char_boundary" is unverified.
'世'.repeat(30)is 90 UTF-8 bytes; floor-to-boundary at 50 should yield 48 bytes (16 ×'世').💡 Suggested addition
const content = r.matches[0].content as string; const buf = Buffer.from(content, 'utf8'); + expect( + buf.length <= 50, + `truncated byte length must be ≤ max_line_bytes=50 (got ${buf.length})`, + ); expect( buf.toString('utf8') === content, `truncated content must remain valid UTF-8 (got ${buf.length} bytes)`, );🤖 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/e2e/workers/harness/src/cases-fs-encoding.ts` around lines 89 - 114, The test currently only checks that the JSON string is valid UTF-8; update the assertions to verify the truncated match respects max_line_bytes and floors to a character boundary: compute the UTF-8 byte length of r.matches[0].content (e.g. Buffer.byteLength(content, 'utf8') or Buffer.from(content, 'utf8').length) and assert it is <= max_line_bytes (50) and, for this input of '世'.repeat(30, created as line), assert it equals the expected floored byte count (48 bytes, i.e. 16 characters), using the existing variables max_line_bytes, content and r.matches[0].content to locate the check in this case (fs_grep_utf8_max_line_bytes_floors_to_char_boundary).shell/tests/function_handlers.rs (1)
469-470: 💤 Low valueDrop the dead
_unused_value_markershim.
Valueis already imported and used throughout the file (e.g. lines 6, 42, 46), so this#[allow(dead_code)]-gated stub doesn't suppress any warning — it looks like a refactor leftover.♻️ Proposed cleanup
-#[allow(dead_code)] -fn _unused_value_marker(_: Value) {}🤖 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/function_handlers.rs` around lines 469 - 470, Remove the dead shim function _unused_value_marker and its #[allow(dead_code)] attribute: locate the fn _unused_value_marker(_: Value) {} definition and delete it, since the Value type is already imported/used elsewhere (e.g., Value at top of file and other usages) and the stub is a leftover no-op that serves no purpose.shell/tests/e2e/workers/harness/src/cases-fs-sandbox.ts (1)
86-95: 💤 Low valueDrop the redundant
asyncIIFE.The wrapper has no
await; the body is fully callback-driven, so theasyncIIFE adds a microtask hop with no benefit. Inlining is clearer and avoids the “fire-and-forget Promise” shape that lints typically flag.♻️ Proposed cleanup
- const channel = await iii.createChannel(64); - (async () => { - channel.writer.stream.write(bytes, (err: Error | null | undefined) => { - if (err) { - channel.writer.close(); - return; - } - channel.writer.stream.end(() => channel.writer.close()); - }); - })(); + const channel = await iii.createChannel(64); + channel.writer.stream.write(bytes, (err: Error | null | undefined) => { + if (err) { + channel.writer.close(); + return; + } + channel.writer.stream.end(() => channel.writer.close()); + });🤖 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/e2e/workers/harness/src/cases-fs-sandbox.ts` around lines 86 - 95, Remove the redundant async IIFE around the write callback: inline the callback-driven code instead of wrapping it in (async () => { ... })(), since there is no await and it creates an unnecessary Promise/microtask. Locate the sequence starting with iii.createChannel(64) and update the block that calls channel.writer.stream.write(...) to directly perform the write and use its callback to call channel.writer.close() or channel.writer.stream.end(() => channel.writer.close()) as currently implemented, removing the outer async IIFE wrapper..github/workflows/shell-e2e.yml (1)
36-47: 🏗️ Heavy liftPin the iii install script version in CI.
The workflow pipes
install.iii.dev/iii/main/install.shthroughshwithout a version pin or checksum verification. For security-sensitive CI workers, pin a release tag and verify a checksum so a compromised host or a brokenmainpush cannot silently inject arbitrary engine bits into your runners.Consider updating lines 43–47 to use a stable release tag and verify the checksum before execution.
🤖 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 @.github/workflows/shell-e2e.yml around lines 36 - 47, The "Install iii engine (latest from main)" CI step currently pipes https://install.iii.dev/iii/main/install.sh directly to sh; change it to download a pinned release URL (e.g., replace "main" with a specific release tag) into a file, verify its checksum (compare its sha256/sha512 against a checked-in or workflow-secret expected value), and only execute the installer after verification (do not pipe from curl directly). Update the step name/command in the workflow so the runner uses the tagged installer URL and includes the checksum comparison and conditional execution logic.shell/src/fs/host.rs (1)
976-1078: 💤 Low value
writepath: solid atomic write with idle timeout and cap enforcement.Temp-sibling-with-UUID + restrictive 0o600 during streaming + per-chunk 30s idle timeout + cap enforcement + flush/sync_all/rename +
TempGuardfor cleanup is a thorough atomic-write recipe. The defensive parent re-check againsthost_root_canonat lines 985–993 is a reasonable belt-and-suspenders givenparents:truecreates intermediate directories.One small observation worth verifying: at line 1035,
let new_total = total + chunk.len() as u64;could overflow ifcap == 0(uncapped) and an enormous amount is streamed. In practice, the file write would hit OS errors first, but achecked_addwould be defensive. Low priority.🤖 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 976 - 1078, The addition total + chunk.len() in async fn write (variables total, cap, new_total, chunk) can overflow when cap == 0 (uncapped); change the logic in the read loop to use checked_add (or saturating_add) when computing new_total, return an FsError (e.g. S218 or a new code) if checked_add returns None (overflow) or otherwise treat the overflow as exceeding capacity, and then proceed with the existing cap check and write; update the new_total assignment and error path in the block that currently computes new_total to defensively handle arithmetic overflow before writing the chunk.
🤖 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/config.yaml`:
- Around line 12-64: Remove "env" from the default allowlist (allowlist: ...
"env" ...) to restore the "read-only" intent, or if you must keep it add a
denylist tripwire in denylist_patterns that catches use of env to exec arbitrary
commands (e.g., a regex matching "\benv\b" followed by a token that is not VAR=,
flagging forms like "env <cmd>"); update the configuration where allowlist and
denylist_patterns are defined and ensure is_command_allowed
(shell/src/config.rs) continues to treat printenv as permitted while env is
either removed or flagged by the new denylist entry.
In `@shell/README.md`:
- Line 336: Update the stale test count in the README entry for `tests/e2e/` so
it matches the actual harness and PR summary: change the text that currently
says "143 default cases + 1 jailed case" to "144 default cases + 2 jailed cases"
(the sentence referencing `run-tests.sh` and `run-tests-jailed.sh`), and verify
the adjacent wording still reads correctly with the new counts.
In `@shell/src/fs/host.rs`:
- Around line 121-150: Run rustfmt (cargo fmt --all) and commit the formatted
changes to fix CI formatting failures; specifically reformat the try_new
signature in HostFs::try_new, the format! invocation used when reporting a
dangling symlink via canonicalize_with_fallback, and the
symlink_metadata/map_err chain inside the chmod recursive root-check helper so
they conform to rustfmt style. After running cargo fmt --all, verify changes
include the try_new signature, the dangling symlink format! call, and the chmod
recursive check (symlink_metadata/map_err) and push the updated, formatted
files.
- Around line 1108-1133: The pump task can hang if the consumer is slow; update
pump_file_to_channel to wrap each per-chunk writer.write call in
tokio::time::timeout (use the same 30s idle timeout used on the write path) and
return an error if the timeout elapses so the spawned task can send an error and
close the writer; specifically, locate pump_file_to_channel and replace the
direct writer.write(&buf[..n]) await with a timeout wrapper that bails with a
clear error (propagated up to the spawned task) on timeout to avoid leaking
tasks and file handles.
- Around line 365-388: The case-insensitive branch incorrectly escales `$` into
`$$` (escaped_replacement = replacement.replace('$', "$$")) before returning it
from the closure passed to regex::Regex::replacen/replac_all, which corrupts
user-provided literal `$` sequences because closure returns are inserted
verbatim; remove the `$`->`$$` escaping and use the original `replacement` (or a
cloned String of it) inside the closure in the case_insensitive branch (the
variables to change are `escaped_replacement`, the closure passed to
`re.replacen`/`re.replace_all`, and the `case_insensitive` branch), and add a
unit test that runs the sed path with regex=false, ignore_case=true and
replacement="$1" to assert the output contains the literal "$1".
In `@shell/src/functions/kill.rs`:
- Around line 26-30: The code sets h.record.finished_at_ms immediately after
calling start_kill(), prematurely marking the job finished; change the flow so
start_kill() only sets h.record.status = JobStatus::Killed and do not set
h.record.finished_at_ms there—defer setting h.record.finished_at_ms until the
background finalizer (the exec_bg waiter) actually reaps the child and calls
wait(), so update the finalizer that currently observes process exit to write
h.record.finished_at_ms = Some(jobs::now_ms()) and keep remove_old()’s pruning
logic unchanged.
In `@shell/src/functions/types.rs`:
- Around line 56-71: JobSummary currently exposes the job capability via its pub
id field which lets any caller enumerate job_ids via shell::list and then call
shell::status to read full output; fix by removing or making id non-exported
from JobSummary (delete or make id private/omit from serialized struct) and
instead return only caller-scoped identifiers or scoped summaries, or implement
caller scoping in the JOBS lookup used by shell::list/shell::status so that
shell::list returns only jobs belonging to the calling principal; update the
JobSummary definition and the serialization/return code in shell::list and the
access checks in shell::status/JOBS to enforce the capability boundary.
In `@shell/src/manifest.rs`:
- Around line 8-29: The manifest's "functions" array currently lists only the
five non-filesystem APIs (ids like "shell::exec", "shell::exec_bg",
"shell::kill", "shell::status", "shell::list") but omits the worker's
shell::fs::* APIs; update the "functions" array in manifest.rs to include the
full set of shell::fs::* function IDs exposed by the worker (e.g.,
"shell::fs::read", "shell::fs::write", "shell::fs::stat", "shell::fs::list" or
whichever exact fs function names the worker implements) with concise
descriptions so the manifest advertises all 15 functions and matches the runtime
capability surface.
In `@shell/tests/e2e/config-jailed.yaml`:
- Line 40: Add a short explanatory comment above the host_root YAML key
(host_root) noting that "/private/tmp" is the macOS path (since /tmp →
/private/tmp on macOS) and that on Linux users must either create /private/tmp
before running run-tests-jailed.sh or change the value to a Linux-native path
(e.g., /tmp); update the comment so local developers and CI maintainers
understand the prerequisite and how to fix failures when running tests locally.
In `@shell/tests/e2e/run-tests-jailed.sh`:
- Around line 128-143: The sentinel grep is too strict and fails when ANSI color
codes are present in HARNESS_LOG; update both grep calls (the checks that
currently use '^HARNESS_DONE: (PASS|FAIL) [0-9]+/[0-9]+$') to a looser pattern
that allows anything between "HARNESS_DONE:" and the "PASS|FAIL" token, e.g. use
'HARNESS_DONE: .* (PASS|FAIL) [0-9]+/[0-9]+' for both the existence test and the
sentinel capture (the two grep invocations that set/check sentinel and reference
HARNESS_LOG and HARNESS_DONE), or alternatively remove color codes at read time
by piping HARNESS_LOG through a strip-ANSI regex before grepping; either
approach ensures the loop can detect the sentinel even when ANSI sequences are
present.
In `@shell/tests/e2e/run-tests.sh`:
- Around line 148-160: The sentinel grep in run-tests.sh fails because ANSI
escape codes from the harness (written into HARNESS_LOG) prevent the strict
regex '^HARNESS_DONE: (PASS|FAIL) [0-9]+/[0-9]+$' from matching; change the two
grep invocations that read HARNESS_LOG (the checks that set sentinel) to allow
optional/any characters (including ANSI escapes) between "HARNESS_DONE:" and the
PASS/FAIL token (for example use '^HARNESS_DONE: .*?(PASS|FAIL) [0-9]+/[0-9]+$'
or otherwise permit escape sequences), and apply the same change to the
identical grep uses in run-tests-jailed.sh; alternatively, fix the root cause in
worker.ts by gating GREEN/RED/RESET on process.stdout.isTTY (check
process.stdout.isTTY and make GREEN/RED/RESET empty when not TTY) so HARNESS_LOG
contains plain PASS/FAIL.
In `@shell/tests/e2e/workers/harness/src/cases-concurrency.ts`:
- Around line 47-53: The assertion uses jobs.length <= 2 on the raw result from
shell::list (iterating over fulfilled → jobs) which can fail due to cross-test
retained jobs; instead filter the returned jobs to only those started by this
test (e.g., compare job_id against the local a.job_id and b.job_id set when
spawning) and then assert the filtered list contains the expected IDs (or that
both a.job_id and b.job_id are present) rather than asserting an absolute upper
bound on jobs.length.
In `@shell/tests/e2e/workers/harness/src/cases-fs-encoding.ts`:
- Around line 56-73: The test fs_path_with_consecutive_slashes_resolves masks
inner expect failures by swallowing errors in the catch; change the try/catch so
assertion failures propagate: inside run, when calling ctx.call('shell::fs::ls')
keep the expect(Array.isArray(r.entries)) but in the catch block rethrow the
caught error (or remove the try/catch entirely) so failures from the expect are
not swallowed; ensure you no longer rely on the succeeded/threw tautology
(remove the final expect(succeeded || threw) or replace it with meaningful
assertions).
In `@shell/tests/e2e/workers/harness/src/cases-fs-errors.ts`:
- Around line 52-63: The test fs_mkdir_existing_with_parents_is_idempotent
currently only type-checks second.created; change the assertion to verify
idempotency by asserting the second call to shell::fs::mkdir returns created ===
false (e.g. replace the expect(...) with expectEqual(second.created, false,
'second mkdir should not report created for existing dir')) so the test fails if
duplicate mkdir with parents incorrectly reports created: true.
In `@shell/tests/e2e/workers/harness/src/cases-jobs-break.ts`:
- Around line 76-99: The test embeds magic numbers (2 and 3) tied to the
configured max_concurrent_jobs=2 which makes it fragile; update the block around
the promises/expect checks (references: the promises creation using
call('shell::exec_bg'), the settled/succeeded/rejected arrays, the expect
assertions, and sawCapMessage) to (1) replace the hard-coded 2/3 with a single
local constant derived from the test config (e.g. const CAP = /* read
test-config or set to 2 */) or at minimum add a clear comment stating those
numbers correspond to max_concurrent_jobs, and (2) strengthen the failure check
by asserting sawCapMessage is true (i.e. require the rejection message mentions
the cap) instead of relying solely on counts so the test still validates the cap
semantics if counts change.
In `@shell/tests/e2e/workers/harness/src/cases-jobs.ts`:
- Around line 74-88: The test "max_concurrent_jobs cap rejects third spawn"
relies on an implicit config value (max_concurrent_jobs = 2); update the test to
explicitly document that dependency by either adding an inline comment
referencing the config key "max_concurrent_jobs" or by changing the expectError
call's assertion message to include that key (e.g. pass 'max_concurrent_jobs' or
'max_concurrent_jobs=2' to expectError). Locate the block inside the async run
function where the three call('shell::exec_bg', ...) invocations and expectError
are used and modify the message or add a brief comment above the third spawn to
make the coupling explicit. Ensure cleanup (calls to shell::kill and sleep)
remains unchanged.
In `@shell/tests/e2e/workers/harness/src/cases-vuln-repro-jailed.ts`:
- Around line 45-73: The assertions and cleanup after the fsWriteStream test can
abort before rmSync runs, leaving externalDir or linkInsideJail polluted; wrap
the assertion checks (expect(rejected...), expect(observed.includes('S215')...),
expect(!existsSync(externalTarget)...)) and the subsequent rmSync calls in a
try/finally so the rmSync cleanup always executes, and apply the same
try/finally pattern to the second case as well (use the same variables:
rejected, observed, externalTarget, externalDir, linkInsideJail and the
fsWriteStream/escapePath test blocks to locate where to wrap).
In `@shell/tests/e2e/workers/harness/src/cases-vuln-repro.ts`:
- Around line 47-74: The temp directory created by newWorkdir('h1-target') is
never removed because only the file path stored in target is deleted; change the
setup to capture the temp directory path (e.g., store the return of
newWorkdir('h1-target') in a variable like targetDir), use join(targetDir,
'external-file') for target, and in the cleanup call rmSync on the directory
(rmSync(targetDir, { recursive: true, force: true })) in addition to or instead
of rmSync(target, { force: true }) so the anonymous temp directory is removed;
update references in this test block (newWorkdir/newWorkdir('h1-target'),
target, and cleanup rmSync calls) accordingly.
---
Nitpick comments:
In @.github/workflows/shell-e2e.yml:
- Around line 36-47: The "Install iii engine (latest from main)" CI step
currently pipes https://install.iii.dev/iii/main/install.sh directly to sh;
change it to download a pinned release URL (e.g., replace "main" with a specific
release tag) into a file, verify its checksum (compare its sha256/sha512 against
a checked-in or workflow-secret expected value), and only execute the installer
after verification (do not pipe from curl directly). Update the step
name/command in the workflow so the runner uses the tagged installer URL and
includes the checksum comparison and conditional execution logic.
In `@shell/src/fs/host.rs`:
- Around line 976-1078: The addition total + chunk.len() in async fn write
(variables total, cap, new_total, chunk) can overflow when cap == 0 (uncapped);
change the logic in the read loop to use checked_add (or saturating_add) when
computing new_total, return an FsError (e.g. S218 or a new code) if checked_add
returns None (overflow) or otherwise treat the overflow as exceeding capacity,
and then proceed with the existing cap check and write; update the new_total
assignment and error path in the block that currently computes new_total to
defensively handle arithmetic overflow before writing the chunk.
In `@shell/src/functions/fs_mkdir.rs`:
- Around line 9-20: The six nearly-identical handler functions (e.g.,
fs_mkdir::handle) should be generated by a single macro instead of duplicated:
create a macro_rules! (e.g., in fs_dispatch.rs) that accepts the handler
function name, request/response types (e.g., MkdirRequest, MkdirResponse), and
the backend method identifier (e.g., mkdir) and emits the async fn handle(host:
Arc<dyn FsBackend>, iii: iii_sdk::III, sandbox_enabled: bool, payload: Value) ->
Result<Res, String> body which deserializes with serde_json::from_value into the
Req type, calls req.split(), selects the backend via pick_backend, invokes
backend.<method>(args).await, and maps errors with err_to_string (preserve
FsError mapping behavior if needed); then replace each fs_* file’s duplicated
handle implementation with a single invocation of that macro providing the
correct types and method names (e.g., MkdirRequest/MkdirResponse/mkdir) so all
handlers (handle functions) share the same generated code and signature.
In `@shell/src/functions/fs_sed.rs`:
- Around line 9-20: Multiple fs_* handlers repeat the same boilerplate: parse
payload into a request type, call req.split(), pick_backend(target, host, iii,
sandbox_enabled), invoke backend.<method>(args).await and
map_err(err_to_string); replace this pattern with a macro to avoid duplication.
Add a macro_rules! (e.g., in functions::macros.rs) that accepts the handler
name, request type (e.g., SedRequest), backend method name (e.g., sed), and
response type (e.g., SedResponse) and expands to the async fn handle(host:
Arc<dyn FsBackend>, iii: iii_sdk::III, sandbox_enabled: bool, payload: Value) ->
Result<RespType, String> { let req: ReqType =
serde_json::from_value(payload).map_err(|e| FsError::new("S210", format!("bad {}
payload: {e}")).to_json())?; let (target, args) = req.split(); let backend =
pick_backend(target, host, iii, sandbox_enabled);
backend.method(args).await.map_err(err_to_string) }. Then replace each fs_*.rs
file’s handle implementation with a single macro invocation using the
appropriate ReqType, method name, and RespType (e.g., SedRequest, sed,
SedResponse), keeping existing symbols like pick_backend, FsError, err_to_string
and FsBackend intact.
In `@shell/tests/e2e/workers/harness/src/cases-fs-encoding.ts`:
- Around line 89-114: The test currently only checks that the JSON string is
valid UTF-8; update the assertions to verify the truncated match respects
max_line_bytes and floors to a character boundary: compute the UTF-8 byte length
of r.matches[0].content (e.g. Buffer.byteLength(content, 'utf8') or
Buffer.from(content, 'utf8').length) and assert it is <= max_line_bytes (50)
and, for this input of '世'.repeat(30, created as line), assert it equals the
expected floored byte count (48 bytes, i.e. 16 characters), using the existing
variables max_line_bytes, content and r.matches[0].content to locate the check
in this case (fs_grep_utf8_max_line_bytes_floors_to_char_boundary).
In `@shell/tests/e2e/workers/harness/src/cases-fs-host-jail.ts`:
- Around line 3-44: Add a positive "allowed path" test to FS_HOST_JAIL_CASES
that calls the jailed worker (use ctx.call with 'shell::fs::stat' or
'shell::fs::ls') on a known CI-precreated path under
/private/tmp/iii-shell-jailed-root (e.g.
'/private/tmp/iii-shell-jailed-root/somefile') and assert success (e.g. await
the call and assert the returned stat/entries are truthy) so the suite verifies
permitted paths are accepted; locate FS_HOST_JAIL_CASES and append a case object
with a unique name like 'fs_host_allowed_private_tmp_stat' using the CaseContext
ctx to perform the positive check.
In `@shell/tests/e2e/workers/harness/src/cases-fs-sandbox.ts`:
- Around line 86-95: Remove the redundant async IIFE around the write callback:
inline the callback-driven code instead of wrapping it in (async () => { ...
})(), since there is no await and it creates an unnecessary Promise/microtask.
Locate the sequence starting with iii.createChannel(64) and update the block
that calls channel.writer.stream.write(...) to directly perform the write and
use its callback to call channel.writer.close() or channel.writer.stream.end(()
=> channel.writer.close()) as currently implemented, removing the outer async
IIFE wrapper.
In `@shell/tests/function_handlers.rs`:
- Around line 469-470: Remove the dead shim function _unused_value_marker and
its #[allow(dead_code)] attribute: locate the fn _unused_value_marker(_: Value)
{} definition and delete it, since the Value type is already imported/used
elsewhere (e.g., Value at top of file and other usages) and the stub is a
leftover no-op that serves no purpose.
🪄 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: c553f81e-d1cd-40e5-983e-643d2ff25f1c
⛔ Files ignored due to path filters (2)
shell/Cargo.lockis excluded by!**/*.lockshell/tests/e2e/workers/harness/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (70)
.github/workflows/shell-e2e.yml.gitignoreshell/Cargo.tomlshell/README.mdshell/build.rsshell/config.yamlshell/iii.worker.yamlshell/src/config.rsshell/src/exec.rsshell/src/fs/error.rsshell/src/fs/host.rsshell/src/fs/mod.rsshell/src/fs/sandbox.rsshell/src/fs/wire.rsshell/src/functions/exec.rsshell/src/functions/exec_bg.rsshell/src/functions/fs_chmod.rsshell/src/functions/fs_dispatch.rsshell/src/functions/fs_grep.rsshell/src/functions/fs_ls.rsshell/src/functions/fs_mkdir.rsshell/src/functions/fs_mv.rsshell/src/functions/fs_read.rsshell/src/functions/fs_rm.rsshell/src/functions/fs_sed.rsshell/src/functions/fs_stat.rsshell/src/functions/fs_write.rsshell/src/functions/kill.rsshell/src/functions/list.rsshell/src/functions/mod.rsshell/src/functions/status.rsshell/src/functions/types.rsshell/src/jobs.rsshell/src/lib.rsshell/src/main.rsshell/src/manifest.rsshell/tests/e2e/.gitignoreshell/tests/e2e/README.mdshell/tests/e2e/config-jailed.yamlshell/tests/e2e/config.yamlshell/tests/e2e/data/.gitkeepshell/tests/e2e/reports/.gitkeepshell/tests/e2e/run-tests-jailed.shshell/tests/e2e/run-tests.shshell/tests/e2e/workers/harness/iii.worker.yamlshell/tests/e2e/workers/harness/package.jsonshell/tests/e2e/workers/harness/src/cases-concurrency.tsshell/tests/e2e/workers/harness/src/cases-edge.tsshell/tests/e2e/workers/harness/src/cases-exec-break.tsshell/tests/e2e/workers/harness/src/cases-fs-encoding.tsshell/tests/e2e/workers/harness/src/cases-fs-errors.tsshell/tests/e2e/workers/harness/src/cases-fs-host-jail.tsshell/tests/e2e/workers/harness/src/cases-fs-host.tsshell/tests/e2e/workers/harness/src/cases-fs-protocol-break.tsshell/tests/e2e/workers/harness/src/cases-fs-sandbox.tsshell/tests/e2e/workers/harness/src/cases-jobs-break.tsshell/tests/e2e/workers/harness/src/cases-jobs.tsshell/tests/e2e/workers/harness/src/cases-safety.tsshell/tests/e2e/workers/harness/src/cases-sandbox-break.tsshell/tests/e2e/workers/harness/src/cases-streaming-break.tsshell/tests/e2e/workers/harness/src/cases-vuln-repro-jailed.tsshell/tests/e2e/workers/harness/src/cases-vuln-repro.tsshell/tests/e2e/workers/harness/src/cases.tsshell/tests/e2e/workers/harness/src/runner.tsshell/tests/e2e/workers/harness/src/worker.tsshell/tests/e2e/workers/harness/tsconfig.jsonshell/tests/function_handlers.rsshell/tests/host_fs_branches.rsshell/tests/jobs_lifecycle.rsshell/tests/sandbox_dispatch.rs
build_manifest() listed only the 5 shell::* ids; the 10 shell::fs::* ids registered at runtime were missing, so `--manifest` under-reported the worker's capability surface. Added the fs entries with descriptions copied verbatim from the runtime .description() calls in main.rs and updated the count assertion to 15 with fs id presence checks.
`is_command_allowed` only checks argv[0]'s basename, so allowlisting `env` let `argv = ["env", "nmap", "host"]` pass the gate even though the config comment block explicitly states the default allowlist is "intentionally read-only" and lists exec-escapes (`node -e`, `python3 -c`, `npm run`, etc.) as left-out-on-purpose. `printenv` is also allowlisted and covers the legitimate read-only env-inspection use case without the exec capability — `env` was redundant. Adding an `env <cmd>` denylist regex would be the wrong layer: the config comment block (lines 42-47) calls denylist patterns "advisory, not a security boundary" because allowlisted shells/interpreters can defeat any pattern by token construction. Removing the entry is the clean fix. Regression test parses the shipped config.yaml and asserts `printenv` is allowed while `env nmap host` is rejected with "not in allowlist".
literal_replace_line's case-insensitive branch pre-escaped $ to $$ before returning from a closure passed to Regex::replacen / replace_all, on the assumption that closure returns go through $N capture substitution. They don't — the regex crate's Replacer blanket impl for `FnMut(&Captures) -> String` inserts the returned string verbatim; the $N rewrite only fires for the &str / String Replacer impls. So regex=false, ignore_case=true, replacement="$1" emitted "$$1" instead of the user-intended literal "$1". Drop the escaped_replacement line and clone the original replacement inside the closures. The regex=true path is unchanged: it correctly routes through expand_regex_replacement (caps.expand) where capture substitution is supposed to happen. Regression test drives the public sed API with the bug-triggering inputs (HELLO + needle "hello" + replacement "$1", regex=false, ignore_case=true) and asserts the output is "$1 world\n".
ytallo
approved these changes
May 5, 2026
This was referenced May 8, 2026
6 tasks
7 tasks
This was referenced Jun 26, 2026
This was referenced Jul 3, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
New
shellworker — Unix shell execution and filesystem operations for iii agents — and the CI scaffolding to keep it honest.RegisterFunction::new_asyncSDK path:shell::exec,shell::exec_bg,shell::kill,shell::status,shell::list, plus 10shell::fs::*(ls, stat, mkdir, rm, chmod, mv, grep, sed, write, read). FS ops dispatch to host or sandbox via atargetfield.shell::exec).host_rootjail withcanonicalize_with_fallback(walks longest existing ancestor, lexically collapses non-existent tail, rejects dangling symlinks per tail component → S215). Cachedhost_root_canon+denylist_canonat construction.fs.allow_unjailed: boolopt-in: refuse to start whenhost_root=None && allow_unjailed=false. Closes silent jail bypass when config.yaml is missing (validated in both load_config success and fallback paths).chmod -Rrejects symlink walk root with S212; per-entry walk skips symlink entries (no chmod-through-link).JobSummaryredaction:shell::listreturns{id, status, *_at_ms, exit_code, *_truncated}only — argv/stdout/stderr stay reachable throughshell::status(UUID job_id is the capability).shell::exec_bguses atomictry_reserve_and_insertfor concurrent-job cap..github/workflows/shell-e2e.ymlmirroringiii-database-e2e.yml— runs default + jailed harness on PRs touchingshell/**. Jailed config hardcodes macOS-shaped/private/tmp/iii-shell-jailed-root, so the workflow pre-creates that path 1777 on Ubuntu.Test plan
cargo build -p iii-shell --releasecleancargo test -p iii-shell --all-features— 277 unit/integration tests passcd shell/tests/e2e && ./run-tests.sh— 144/144 PASScd shell/tests/e2e && ./run-tests-jailed.sh --no-build— 2/2 PASSshell-e2eworkflow runs on this PR and goes greenci.ymlrust matrix still passes forshelliii-shell --manifeststill produces valid JSON listing all 15 functionsOut of scope (explicitly deferred)
Architecture findings A-H1/H2/H3, P-H1/P-H3, and per-caller
JOBSscoping are documented but not addressed here — they're architectural reshaping, not security fixes.Summary by CodeRabbit
New Features
Documentation