feat: add iii-shell worker - #31
Conversation
Unix shell execution worker — Mike's priority-1 fundamental. Every agent that needs to touch the OS (run build, read file, call CLI) goes through this worker for one place to enforce allowlists, timeouts, and output caps. Functions (5): - shell::exec sync exec, full stdout/stderr - shell::exec_bg spawn background, return job_id - shell::kill kill a running job - shell::status inspect a job - shell::list list all jobs Triggers (5 HTTP): POST /api/shell/exec, /exec_bg, /kill, /status GET /api/shell/list Safety: - allowlist (basename match) - denylist regex patterns - max/default timeout caps - stdout/stderr byte caps with truncation flags - inherit_env off by default, allowed_env forwarded - max_concurrent_jobs - job_retention_secs (pruned on list) SDK: iii-sdk 0.11.0 stable Tests: 18 passing (config, allowlist, denylist, exec, timeout, truncation, jobs)
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 41 minutes and 16 seconds. ⌛ 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 a complete new Rust shell execution worker module called Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
Heads up — main is being bumped to
Release notes: https://github.com/iii-hq/iii/releases/tag/iii/v0.11.3 |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
shell/src/main.rs (1)
163-173: Minor:shell::listis a GET but still declares a JSON request_format.For consistency with how you modeled the other GET-style/no-body flows, consider either setting
request_format: Noneforlistor documenting that the GET body is ignored. This is cosmetic; functionally harmless.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shell/src/main.rs` around lines 163 - 173, The RegisterFunctionMessage for the "shell::list" function currently supplies a JSON request_format even though this is a GET/no-body handler; update the RegisterFunctionMessage used in iii.register_function_with for id "shell::list" (the RegisterFunctionMessage construction passed to functions::list::build_handler(shared.clone())) to set request_format: None (or alternatively add a comment/documentation that the GET body will be ignored) so it matches the other GET-style handlers.shell/README.md (1)
17-23: Nit: add a language to the fenced code block (markdownlint MD040).📝 Proposed fix
-``` +```text POST /api/shell/exec → shell::exec POST /api/shell/exec_bg → shell::exec_bg POST /api/shell/kill → shell::kill POST /api/shell/status → shell::status GET /api/shell/list → shell::list</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@shell/README.mdaround lines 17 - 23, Markdown fence in the README code
block lacks a language tag which triggers markdownlint MD040; update the fenced
block in shell/README.md that contains the POST/GET endpoint list (lines showing
"POST /api/shell/exec", "POST /api/shell/exec_bg", "POST /api/shell/kill", "POST
/api/shell/status", "GET /api/shell/list") to include a language token (e.g.,
change the openingtotext) so the block is marked as plain text and the
linter warning is resolved.</details> </blockquote></details> <details> <summary>shell/src/config.rs (1)</summary><blockquote> `101-130`: **LGTM — allowlist/denylist/timeout logic is correct and well-tested.** One small note (not blocking): `argv.join(" ")` on line 117 does not quote tokens, so regex authors need to remember argv tokens that contain spaces will collapse with their neighbors. Worth a one-line comment on `is_command_allowed` to document that the denylist is matched against a simple space-joined string, not a shell-quoted command line. Also, `resolve_timeout(Some(0))` returns `0` and will trip immediately — consider a floor if that's not desired. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@shell/src/config.rs` around lines 101 - 130, Add a one-line comment inside is_command_allowed near the argv.join(" ") usage to document that the denylist regexes operate against a simple space-joined token string (tokens are not shell-quoted and internal spaces will merge), and update resolve_timeout to enforce a minimum floor (e.g., ensure requested Some(0) doesn't return 0) by clamping the resolved timeout with a min value before returning (modify resolve_timeout to compute t = requested.unwrap_or(self.default_timeout_ms).max(MIN_TIMEOUT).min(self.max_timeout_ms) or similar, where MIN_TIMEOUT is a sensible constant). ``` </details> </blockquote></details> <details> <summary>shell/src/functions/exec_bg.rs (1)</summary><blockquote> `26-48`: **Consider an upper bound on background-job runtime.** `exec_bg` currently has no timeout — a stuck/runaway child lives until explicitly killed or the process exits. With `max_concurrent_jobs: 16` that's an easy way to exhaust the slot budget with zombies-in-waiting. Consider adding an optional `max_bg_timeout_ms` in `ShellConfig` (or reusing `max_timeout_ms`) and applying `tokio::time::timeout` around `child.wait()` in the background task, transitioning status to `TimedOut` on trip. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@shell/src/functions/exec_bg.rs` around lines 26 - 48, Add an optional background-job timeout to prevent runaway children: add a max_bg_timeout_ms: Option<u64> (or reuse max_timeout_ms) to ShellConfig, read it in exec_bg::handle and pass it into the spawned background task that waits on the child; wrap the child.wait() call with tokio::time::timeout(Duration::from_millis(timeout_ms), child.wait()) and on timeout call child.kill().await (or try_shutdown) and set the job status to TimedOut (where job tracking updates occur), ensuring you handle both timeout and normal wait results and convert errors into the existing job/error handling flow; reference symbols: ShellConfig, max_concurrent_jobs, handle, jobs::running_count, parse_argv, and the background task's child.wait(). ``` </details> </blockquote></details> <details> <summary>shell/src/exec.rs (1)</summary><blockquote> `28-49`: **Add `kill_on_drop(true)` to avoid orphaned child processes.** `tokio::process::Command` defaults to `kill_on_drop(false)`. When the spawned child's handle is dropped without calling `wait()` or `kill()`, the process becomes an orphan. This occurs in `run_to_completion` when `.take()` fails on stdout/stderr pipes (lines 58–59), on early returns from match arms (line 71), or when `.await` on spawned tasks errors (lines 79–80). In `exec_bg`, the child is orphaned if `jobs::get()` returns `None` before the task reaches `ch.wait()` (line 80). Setting `kill_on_drop(true)` in `build_command` centralizes cleanup for both code paths and defends against panics in spawned tasks. <details> <summary>Proposed change</summary> ```diff cmd.stdin(Stdio::null()) .stdout(Stdio::piped()) - .stderr(Stdio::piped()); + .stderr(Stdio::piped()) + .kill_on_drop(true); Ok(cmd) ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@shell/src/exec.rs` around lines 28 - 49, The Command built by build_command should set kill_on_drop(true) to ensure child processes are killed if their handles are dropped; update build_command to call .kill_on_drop(true) on the Command before returning so run_to_completion, exec_bg and any spawned tasks won't orphan children when pipes fail, early returns occur, or awaits error. Ensure you modify the Command construction path in build_command (after setting stdin/stdout/stderr) to include .kill_on_drop(true) so all callers (run_to_completion, exec_bg) inherit this behavior. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@shell/config.yaml:
- Around line 12-57: The default allowlist in config.yaml is too permissive
(allowlist) and several entries (git, curl, find, awk, sed, cargo, node,
python3, npm) enable arbitrary execution; either replace allowlist with a narrow
read-only set (ls, cat, pwd, echo, wc, head, tail, sort, uniq, cut, date,
whoami, hostname, which, uname, df, du, ps, env, printenv, basename, dirname,
jq) and make execution-capable tools opt-in, or tighten denylist_patterns to
block execution flags and common bypasses (use \b word boundaries for commands,
block patterns like find\s+[^\n]\s-(exec|execdir)\b,
\bgit\b.--upload-pack\b|\bGIT_SSH_COMMAND\b,
curl\s+[^\n]\s(-o|--output-dir|--form|-F\s@)\b|(?:\bnode\b|\bpython3?\b)\s+-[ec]\b|sed\s+(-i\b|-e\b)|awk\s+.\bsystem\s(|\bnpm\b\s+run\b|cargo\s+build\b),
and add variants to cover /bin/, env prefixes, whitespace/tabs/IFS tricks; treat
denylist as defense-in-depth and update denylist_patterns accordingly.In
@shell/src/functions/exec_bg.rs:
- Around line 79-130: The background task currently drains stdout then stderr
sequentially (using read_bounded on stdout_pipe then stderr_pipe) which can
deadlock if one pipe fills; change the logic to spawn two concurrent tasks that
each call read_bounded for stdout_pipe and stderr_pipe (e.g., tokio::spawn
futures for the stdout reader and stderr reader and await both with join or
join! before proceeding) so both pipes are drained concurrently, and remove the
dead assignmentlet _ = status;while preserving the existing status
computation (keep the block that computesstatusfrom the lockedhandleand
then use it or drop the unused binding).In
@shell/src/functions/kill.rs:
- Around line 36-40: The current block in kill.rs that checks h.child and calls
start_kill() must be changed so missing child handles return killed: false and
any start_kill() error is propagated instead of ignored: replace the if-let that
drops the result with logic that returns Ok(killed: false) when
h.child.is_none(), and when Some(child) call child.start_kill()? (propagating
the Err) — only after a successful start_kill() set h.record.status =
JobStatus::Killed and h.record.finished_at_ms = Some(jobs::now_ms()); ensure the
enclosing function signature/return type is updated to return a Result
containing the killed boolean so the propagated error can bubble up.In
@shell/src/jobs.rs:
- Around line 69-108: The global JOBS.map lock is held while awaiting per-job
locks (handle.lock()) in remove_old, list_all, and running_count, causing
head-of-line blocking; fix by cloning or collecting the JobHandle references
(the values from JOBS.map) into a temporary Vec while holding the JOBS.map lock,
then drop the map guard and iterate that Vec to await each handle.lock() and
inspect record.finished_at_ms / record.clone() / record.status respectively in
remove_old, list_all, and running_count so the global map lock is not held
during await points.In
@shell/src/main.rs:
- Around line 175-190: The HTTP trigger paths in the loop are missing leading
slashes which causes incorrect registration; update the api_path values passed
into the RegisterTriggerInput (created in iii.register_trigger) so each path has
a leading "/" (e.g., "/shell/exec", "/shell/exec_bg", "/shell/kill",
"/shell/status", "/shell/list"); you can either change the hardcoded path
strings in the tuple list or prepend a "/" when building the json!({ "api_path":
path, "http_method": method }) so that RegisterTriggerInput.config.api_path
always contains the leading slash.In
@shell/src/manifest.rs:
- Around line 10-12: Update the "description" for the shell::exec manifest entry
to clarify that output is size-limited: replace the claim of "full
stdout/stderr" with wording that output is capped by max_output_bytes and
truncated flags may be returned; mention the truncation behavior so callers know
to check truncation flags returned by the worker (refer to the "shell::exec"
manifest entry and any consumer-facing docs that mention
max_output_bytes/truncation flags).
Nitpick comments:
In@shell/README.md:
- Around line 17-23: Markdown fence in the README code block lacks a language
tag which triggers markdownlint MD040; update the fenced block in
shell/README.md that contains the POST/GET endpoint list (lines showing "POST
/api/shell/exec", "POST /api/shell/exec_bg", "POST /api/shell/kill", "POST
/api/shell/status", "GET /api/shell/list") to include a language token (e.g.,
change the openingtotext) so the block is marked as plain text and the
linter warning is resolved.In
@shell/src/config.rs:
- Around line 101-130: Add a one-line comment inside is_command_allowed near the
argv.join(" ") usage to document that the denylist regexes operate against a
simple space-joined token string (tokens are not shell-quoted and internal
spaces will merge), and update resolve_timeout to enforce a minimum floor (e.g.,
ensure requested Some(0) doesn't return 0) by clamping the resolved timeout with
a min value before returning (modify resolve_timeout to compute t =
requested.unwrap_or(self.default_timeout_ms).max(MIN_TIMEOUT).min(self.max_timeout_ms)
or similar, where MIN_TIMEOUT is a sensible constant).In
@shell/src/exec.rs:
- Around line 28-49: The Command built by build_command should set
kill_on_drop(true) to ensure child processes are killed if their handles are
dropped; update build_command to call .kill_on_drop(true) on the Command before
returning so run_to_completion, exec_bg and any spawned tasks won't orphan
children when pipes fail, early returns occur, or awaits error. Ensure you
modify the Command construction path in build_command (after setting
stdin/stdout/stderr) to include .kill_on_drop(true) so all callers
(run_to_completion, exec_bg) inherit this behavior.In
@shell/src/functions/exec_bg.rs:
- Around line 26-48: Add an optional background-job timeout to prevent runaway
children: add a max_bg_timeout_ms: Option (or reuse max_timeout_ms) to
ShellConfig, read it in exec_bg::handle and pass it into the spawned background
task that waits on the child; wrap the child.wait() call with
tokio::time::timeout(Duration::from_millis(timeout_ms), child.wait()) and on
timeout call child.kill().await (or try_shutdown) and set the job status to
TimedOut (where job tracking updates occur), ensuring you handle both timeout
and normal wait results and convert errors into the existing job/error handling
flow; reference symbols: ShellConfig, max_concurrent_jobs, handle,
jobs::running_count, parse_argv, and the background task's child.wait().In
@shell/src/main.rs:
- Around line 163-173: The RegisterFunctionMessage for the "shell::list"
function currently supplies a JSON request_format even though this is a
GET/no-body handler; update the RegisterFunctionMessage used in
iii.register_function_with for id "shell::list" (the RegisterFunctionMessage
construction passed to functions::list::build_handler(shared.clone())) to set
request_format: None (or alternatively add a comment/documentation that the GET
body will be ignored) so it matches the other GET-style handlers.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `0dc02d80-ff17-4f04-a6f1-3941472471d9` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 6de3f4b859aa023e8cf96d0aee491fafaf8fa376 and 0a5ec306396018355fe2a21e5373a05da9534abf. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `shell/Cargo.lock` is excluded by `!**/*.lock` </details> <details> <summary>📒 Files selected for processing (15)</summary> * `shell/Cargo.toml` * `shell/README.md` * `shell/build.rs` * `shell/config.yaml` * `shell/src/config.rs` * `shell/src/exec.rs` * `shell/src/functions/exec.rs` * `shell/src/functions/exec_bg.rs` * `shell/src/functions/kill.rs` * `shell/src/functions/list.rs` * `shell/src/functions/mod.rs` * `shell/src/functions/status.rs` * `shell/src/jobs.rs` * `shell/src/main.rs` * `shell/src/manifest.rs` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| for (fn_id, path, method) in [ | ||
| ("shell::exec", "shell/exec", "POST"), | ||
| ("shell::exec_bg", "shell/exec_bg", "POST"), | ||
| ("shell::kill", "shell/kill", "POST"), | ||
| ("shell::status", "shell/status", "POST"), | ||
| ("shell::list", "shell/list", "GET"), | ||
| ] { | ||
| if let Err(e) = iii.register_trigger(RegisterTriggerInput { | ||
| trigger_type: "http".to_string(), | ||
| function_id: fn_id.to_string(), | ||
| config: json!({ "api_path": path, "http_method": method }), | ||
| metadata: None, | ||
| }) { | ||
| tracing::warn!(error = %e, "failed to register http trigger for {}", fn_id); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
iii-sdk 0.11.3 RegisterTriggerInput api_path format leading slash
💡 Result:
In iii-sdk version 0.11.3 (Rust), the RegisterTriggerInput struct for HTTP triggers requires the api_path field in its config JSON object to be formatted as a relative path starting with a leading slash (e.g., "/math/multiply" or "/orders"). This is consistently shown across official documentation and examples. Examples: - config: json!({ "api_path": "/math/multiply", "http_method": "POST" }) - config: json!({ "api_path": "/orders", "http_method": "POST" }) - config: json!({ "api_path": "/users", "http_method": "GET" }) - Supports path parameters like "/products/:id" No sources indicate that api_path should omit the leading slash; all official examples include it. Version 0.11.x documentation and code snippets confirm this format has been standard since at least 0.11.0.
Citations:
- 1: https://iii.dev/docs/how-to/use-functions-and-triggers
- 2: https://www.mintlify.com/iii-hq/sdk/concepts/triggers
- 3: https://docs.rs/crate/iii-sdk/latest
- 4: https://docs.rs/iii-sdk/latest/iii_sdk/
- 5: https://github.com/iii-hq/iii/blob/06a6580d46649804e7635bb3160d8f9709247e74/sdk/README.md
- 6: https://github.com/iii-hq/iii/blob/d2f0fe3b5ff019960998effb739e8f6f02ab961f/sdk/README.md
- 7: https://github.com/iii-hq/iii/blob/1c0cefbf43859fa8ab833a919b18c6eea897790c/sdk/README.md
- 8: https://github.com/iii-hq/sdk
- 9: https://pypi.org/project/iii-sdk/0.11.0.dev8/
- 10: https://crates.io/crates/iii-sdk/0.11.0-next.4
- 11: https://crates.io/crates/iii-sdk/0.11.0-next.6
Add leading slashes to all HTTP trigger paths.
The paths must start with a leading slash per iii-sdk 0.11.3 requirements. Correct format:
"/shell/exec"instead of"shell/exec""/shell/exec_bg"instead of"shell/exec_bg""/shell/kill"instead of"shell/kill""/shell/status"instead of"shell/status""/shell/list"instead of"shell/list"
Without leading slashes, triggers will fail to register or register under incorrect paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shell/src/main.rs` around lines 175 - 190, The HTTP trigger paths in the loop
are missing leading slashes which causes incorrect registration; update the
api_path values passed into the RegisterTriggerInput (created in
iii.register_trigger) so each path has a leading "/" (e.g., "/shell/exec",
"/shell/exec_bg", "/shell/kill", "/shell/status", "/shell/list"); you can either
change the hardcoded path strings in the tuple list or prepend a "/" when
building the json!({ "api_path": path, "http_method": method }) so that
RegisterTriggerInput.config.api_path always contains the leading slash.
There was a problem hiding this comment.
iii-engine convention: api_path is registered WITHOUT a leading slash. Engine prepends '/' when matching; double-slash from a leading-slash config has produced 404s in downstream deploys. All sibling workers (llm-router, image-resize, introspect, etc.) follow the same no-leading-slash rule. Leaving as-is.
There was a problem hiding this comment.
@rohitg00, understood — thank you for the clarification. That's a clear project-specific convention: the iii-engine appends its own leading / during path matching, so supplying one in api_path would produce a double-slash and result in 404s. The current code is correct as-is. Leaving it unchanged makes sense.
✏️ Learnings added
Learnt from: rohitg00
Repo: iii-hq/workers PR: 31
File: shell/src/main.rs:175-190
Timestamp: 2026-04-22T20:49:59.016Z
Learning: In the iii-hq workers repository (Rust, iii-sdk), the `api_path` field in `RegisterTriggerInput` config JSON must NOT include a leading slash (e.g., `"shell/exec"`, not `"/shell/exec"`). The iii-engine prepends its own `/` during path matching; a leading slash in the config causes a double-slash path that results in 404s in downstream deploys. All sibling workers (llm-router, image-resize, introspect, etc.) follow the same no-leading-slash rule.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
…k contention - config.yaml: narrow default allowlist to read-only utilities (drop find, awk, sed, git, curl, node, npm, python3, cargo, rustc). Add denylist patterns for common sub-execution / write escapes if operators add those tools back per deployment. - exec_bg.rs: drain stdout/stderr concurrently via spawned readers; the sequential drain deadlocked when the child filled one pipe buffer before closing the other. Remove dead 'let _ = status' binding. - kill.rs: return structured 'killed:false, reason:missing child handle' when the child handle is already gone; propagate start_kill() errors instead of swallowing them. - jobs.rs: snapshot the JOBS map before awaiting per-job locks so list_all/running_count/remove_old don't head-of-line-block every other job operation. - manifest.rs: reflect that shell::exec stdout/stderr are capped at max_output_bytes with per-stream truncation flags.
Unix shell execution worker. Every agent worker that needs to touch the OS (run a build, call a CLI, read a file) goes through this so there's one place to enforce allowlists, timeouts, and output caps.
Functions (5)
shell::exec{exit_code, stdout, stderr, duration_ms, timed_out, stdout_truncated, stderr_truncated}shell::exec_bg{job_id, argv}shell::killjob_idshell::status{job: JobRecord}for ajob_idshell::listHTTP triggers (5)
Safety
allowlist— basename match; empty list = open.denylist_patterns— regex against joined argv (rm\s+-rf\s+/, fork bomb,mkfs,shutdown, …).max_timeout_ms+default_timeout_ms— per-calltimeout_msis clamped.max_output_bytes— stdout/stderr capped with*_truncatedflags.inherit_env: falseby default; onlyallowed_envforwarded.working_dirpins cwd.max_concurrent_jobscapsexec_bgstarts.job_retention_secsprunes old finished jobs on everylistcall.What this is NOT
sandbox-docker/sandbox-firecrackerfor isolation and route trusted commands throughshell.Deferred
shell::exec_stream— live stdout/stderr via iii Streams. Next iteration.Stack
iii-sdk 0.11.0stabletokio::processfor spawningshell-wordsfor argv parsingregexfor denylistonce_cell::sync::Lazy<Jobs>Tests
18 passing:
Summary by CodeRabbit