Conversation
📝 WalkthroughWalkthroughWorkspace members and TypeScript worker modules/tests were removed and replaced by new Rust worker crates under Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/API
participant III as Rust Browser Worker
participant State as III State
participant Python as Python Playwright Bridge
rect rgba(200,230,255,0.5)
Client->>III: browser::create_session(agentId, viewport, headless)
III->>State: state::get("_index")
III->>State: state::set("session:{id}", BrowserSession)
III->>Python: spawn python3 (temp script + JSON payload)
Python-->>III: stdout (JSON or raw)
III->>State: state::set("session:{id}", updated with lastActivity/currentUrl)
III-->>Client: session created (id, metadata)
end
rect rgba(200,255,200,0.5)
Client->>III: tool::browser_navigate(agentId, url)
III->>State: state::get("session:{id}")
III->>III: assert_no_ssrf(url)
III->>Python: spawn python3 (navigate action)
Python-->>III: action result (JSON/raw)
III->>State: state::set("session:{id}", updated)
III-->>Client: navigate result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Review rate limit: 0/3 reviews remaining, refill in 51 minutes and 8 seconds. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| fn assert_no_ssrf(url_str: &str) -> Result<(), IIIError> { | ||
| let parsed = url::Url::parse(url_str).map_err(|e| IIIError::Handler(format!("invalid url: {e}")))?; | ||
| let scheme = parsed.scheme(); | ||
| if scheme != "http" && scheme != "https" { | ||
| return Err(IIIError::Handler(format!("blocked scheme: {scheme}"))); | ||
| } | ||
| let host = parsed.host_str().unwrap_or(""); | ||
| let blocked = ["localhost", "127.0.0.1", "0.0.0.0", "169.254.169.254", "metadata.google.internal"]; | ||
| if blocked.contains(&host) { | ||
| return Err(IIIError::Handler(format!("blocked host: {host}"))); | ||
| } | ||
| if host.starts_with("10.") | ||
| || host.starts_with("192.168.") | ||
| || host.starts_with("fc00:") | ||
| || host.starts_with("fd") | ||
| { | ||
| return Err(IIIError::Handler(format!("blocked private host: {host}"))); | ||
| } | ||
| if let Some(rest) = host.strip_prefix("172.") | ||
| && let Some(second_octet) = rest.split('.').next() | ||
| && let Ok(n) = second_octet.parse::<u32>() | ||
| && (16..=31).contains(&n) | ||
| { | ||
| return Err(IIIError::Handler(format!("blocked private host: {host}"))); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
🔴 SSRF check misses entire IP ranges (127.x, 169.254.x, 0.x, CGN, IPv6) and lacks DNS resolution
The new assert_no_ssrf in the browser Rust worker only blocks exact IPs (127.0.0.1, 169.254.169.254, 0.0.0.0) rather than the full private ranges. The shared assertNoSsrf in packages/shared/src/utils.ts blocks ^127\. (entire /8), ^169\.254\. (entire /16), ^0\. (entire /8), CGN range 100.64.0.0/10, IPv6 loopback ::1, link-local fe80:, and IPv4-mapped ::ffff:. Critically, it also performs DNS resolution to catch hostnames that resolve to private IPs. The new Rust check has none of this — an attacker can bypass SSRF protection with http://127.0.0.2, http://169.254.0.1, http://[::1], or any hostname that DNS-resolves to a private IP. Additionally, host.starts_with("fd") at line 87 false-positives on legitimate domains like fdroid.org.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let safe_plan_id = sanitize_id(&plan_id); | ||
| let safe_key = sanitize_id(&key); | ||
| let safe_agent_id = agent_id.as_deref().map(sanitize_id); | ||
| let written_by = safe_agent_id.unwrap_or_else(|| "system".to_string()); |
There was a problem hiding this comment.
🔴 Missing _meta key protection in workspace_write_handler allows metadata corruption
The original TypeScript orchestrator at src/orchestrator.ts:416-418 blocks writes to the reserved _meta key from HTTP requests:
if (req.headers && safeKey === "_meta") {
throw Object.assign(new Error("_meta is reserved"), { statusCode: 403 });
}The new Rust workspace_write_handler has no such check, allowing any caller (including external HTTP requests) to overwrite the _meta workspace entry. The _meta key stores critical workspace metadata (planId, rootId, description) seeded during orchestrator::execute (workers/orchestrator/src/main.rs:237-248), and overwriting it could corrupt plan execution state.
| let safe_plan_id = sanitize_id(&plan_id); | |
| let safe_key = sanitize_id(&key); | |
| let safe_agent_id = agent_id.as_deref().map(sanitize_id); | |
| let written_by = safe_agent_id.unwrap_or_else(|| "system".to_string()); | |
| let safe_plan_id = sanitize_id(&plan_id); | |
| let safe_key = sanitize_id(&key); | |
| if safe_key == "_meta" { | |
| return Err(IIIError::Handler("_meta is reserved".into())); | |
| } | |
| let safe_agent_id = agent_id.as_deref().map(sanitize_id); | |
| let written_by = safe_agent_id.unwrap_or_else(|| "system".to_string()); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn validate_command(cmd: &str) -> Result<(), IIIError> { | ||
| if cmd.is_empty() || cmd.contains(';') || cmd.contains('|') || cmd.contains('&') || cmd.contains('`') { | ||
| return Err(IIIError::Handler("invalid mcp command".into())); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🔴 MCP command validation replaced allowlist with weak blocklist, allowing arbitrary command execution
The original TypeScript validateMcpCommand at packages/shared/src/utils.ts:132-150 uses an allowlist of safe commands (npx, node, python3, python, uv, uvx, bun, deno). The new Rust validate_command at workers/mcp-client/src/main.rs:60-64 only blocks shell metacharacters (;, |, &, `), allowing any arbitrary command like rm, curl, cat /etc/passwd, etc. This is a significant security regression — an MCP connection request with command: "rm" and args: ["-rf", "/"] would pass validation.
| fn validate_command(cmd: &str) -> Result<(), IIIError> { | |
| if cmd.is_empty() || cmd.contains(';') || cmd.contains('|') || cmd.contains('&') || cmd.contains('`') { | |
| return Err(IIIError::Handler("invalid mcp command".into())); | |
| } | |
| Ok(()) | |
| } | |
| fn validate_command(cmd: &str) -> Result<(), IIIError> { | |
| const ALLOWED: &[&str] = &["npx", "node", "python3", "python", "uv", "uvx", "bun", "deno"]; | |
| let base = cmd.rsplit('/').next().unwrap_or(cmd); | |
| if !ALLOWED.contains(&base) { | |
| return Err(IIIError::Handler(format!( | |
| "MCP command not allowed: {cmd}. Allowed: {}", | |
| ALLOWED.join(", ") | |
| ))); | |
| } | |
| Ok(()) | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| let start = Instant::now(); | ||
| let join = tokio::task::spawn_blocking(move || run_sandboxed_js(&code)); | ||
| let outcome = tokio::time::timeout(Duration::from_millis(timeout_ms + 1_000), join).await; |
There was a problem hiding this comment.
🔴 QuickJS infinite loop permanently leaks a blocking thread — no interrupt handler set
When agent::code_execute receives an infinite loop (e.g. while(true) {}), the code wraps run_sandboxed_js in tokio::task::spawn_blocking with a tokio::time::timeout. On timeout, the caller receives an error response, but the spawn_blocking thread continues running the QuickJS infinite loop forever — tokio::time::timeout does not abort the underlying blocking task. The old TypeScript used vm.Script.runInContext with a native timeout option that actually interrupted execution. The QuickJS runtime supports set_interrupt_handler which can periodically check a deadline and abort execution, but it's not used here. Repeated submissions of infinite-loop code would exhaust the tokio blocking thread pool.
Prompt for agents
In workers/code-agent/src/main.rs, the run_sandboxed_js function at line 30 needs a QuickJS interrupt handler to enforce the timeout. The rquickjs Runtime supports set_interrupt_handler which is called periodically during JS execution. Use it to check an elapsed time deadline and abort execution:
1. Accept a timeout_ms parameter in run_sandboxed_js
2. Record the start time (Instant::now())
3. Call runtime.set_interrupt_handler(Some(move || start.elapsed() > Duration::from_millis(timeout_ms))) before evaluating code
4. This ensures the QuickJS engine actually stops executing when the timeout fires, rather than continuing to consume a thread indefinitely
The spawn_blocking + tokio::time::timeout wrapper at line 124-125 is still useful as an outer safety net, but the interrupt handler is needed to actually stop the JS engine.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn sanitize_id(s: &str) -> String { | ||
| s.chars() | ||
| .filter(|c| c.is_alphanumeric() || matches!(c, '_' | '-' | ':' | '.')) | ||
| .take(256) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
🔴 sanitize_id silently strips invalid characters instead of rejecting, enabling empty-key writes
The original TypeScript sanitizeId at packages/shared/src/utils.ts:174-181 validates and throws if the ID doesn't match ^[a-zA-Z0-9_\-:.]{1,256}$. The new Rust sanitize_id at workers/orchestrator/src/main.rs:52-57 silently strips invalid characters and returns the result. This means an ID like "!!!" produces an empty string "", and "plan/../etc" produces "plan..etc". In workspace_write_handler, this could result in writing to scope = "workspace:" with key = "" when given malformed input, which is unexpected behavior and could collide with other scopes.
| fn sanitize_id(s: &str) -> String { | |
| s.chars() | |
| .filter(|c| c.is_alphanumeric() || matches!(c, '_' | '-' | ':' | '.')) | |
| .take(256) | |
| .collect() | |
| } | |
| fn sanitize_id(s: &str) -> Result<String, IIIError> { | |
| if s.is_empty() | |
| || s.len() > 256 | |
| || !s.chars().all(|c| c.is_alphanumeric() || matches!(c, '_' | '-' | ':' | '.')) | |
| { | |
| return Err(IIIError::Handler(format!("Invalid ID format: {s}"))); | |
| } | |
| Ok(s.to_string()) | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workers/browser/src/main.rs`:
- Around line 293-324: The current create path uses separate calls
(load_session, get_session_index, save_session, set_session_index) causing a
read-modify-write race; implement an atomic reservation/update in the state
layer instead: add a single transactional API (e.g., state.create_session_atomic
or state.reserve_and_save_session) that performs the existence check,
MAX_SESSIONS check, generates the session_id and script_path, writes the
BrowserSession and updates the session index in one atomic operation (or under a
state-backed lock/CAS), and returns an error on conflict; replace the multiple
calls in the create_session handler with this single atomic call to avoid races
and clobbered index updates.
- Around line 587-602: The triggers array used in the loop that calls
iii.register_trigger (RegisterTriggerInput) is missing entries for
tool::browser_click and tool::browser_type, so their HTTP endpoints never get
registered; add entries for ("tool::browser_click", "POST", "api/browser/click")
and ("tool::browser_type", "POST", "api/browser/type") (matching the other tool
endpoints) to the triggers list so iii.register_trigger registers those routes
with the same pattern and config JSON used for the other handlers.
- Around line 73-98: In assert_no_ssrf: replace the brittle host_str()
string-prefix checks with a match on parsed.host() (use url::Host variants) and
classify IPs properly—if parsed.host() yields Host::Ipv4(ip) or Host::Ipv6(ip)
call the appropriate methods (is_loopback(), is_private()/is_global() semantics,
is_link_local()) on the std::net::Ipv4Addr/Ipv6Addr to reject loopback, private
and link-local ranges (e.g. 127.0.0.0/8, 10/8, 172.16/12, 192.168/16, ::1,
fc00::/7, fe80::/10, 169.254.0.0/16, etc.); for Host::Domain keep the explicit
domain blacklist (metadata.google.internal, localhost) and the scheme check in
assert_no_ssrf, returning IIIError::Handler on any rejected host.
- Around line 181-188: The current use of Command::output() wrapped by
tokio::time::timeout can leave the spawned python3 process running when the
future times out; change the logic to use
Command::new("python3").arg(&session.script_path).arg(&payload_str).spawn() to
obtain a Child, then await child.wait_with_output() inside the timeout, and if
the timeout elapses call child.kill().await (or child.kill() then await
child.wait()) to ensure the process is terminated; update the error handling
around the exec/spawn, wait_with_output, and timeout branches (references: the
timeout variable, exec/spawn call, and the Child returned by
spawn/wait_with_output) so that timeouts return IIIError::Handler("browser
script timed out") only after attempting to kill the child and collecting its
exit to avoid leaking processes.
In `@workers/code-agent/src/main.rs`:
- Around line 123-142: The blocking task running run_sandboxed_js must register
a QuickJS interrupt handler so execution can be stopped when the wall-clock
timeout elapses; modify run_sandboxed_js (or its Runtime setup) to accept a
runtime deadline or a reference to an AtomicBool/Instant and call
Runtime::set_interrupt_handler(...) to check elapsed time on each interrupt tick
and return true when exceeded, causing an uncatchable exception and terminating
JS execution; then ensure the spawn_blocking closure supplies the timeout
(timeout_ms or a computed Instant) or a shared atomic flag so the handler can
enforce the same timeout used by the tokio::time::timeout wrapper.
In `@workers/mcp-client/src/main.rs`:
- Around line 215-230: The code inserts the connection into state.connections
before running the transport-specific handshake, causing dead entries when
send_rpc (used to call "initialize" and subsequent "tools/list") fails (e.g.,
SSE without stdin). Fix by deferring state.connections.insert(name.clone(),
conn.clone()) until after the handshake completes successfully (after await on
send_rpc and the tools list call), or early-return for unsupported placeholder
transports (when transport == "sse" && url.is_none()) before inserting; update
the code paths around send_rpc, conn, and transport to ensure the insert only
happens on successful initialize/tools/list.
- Around line 179-181: The code pipes child process stderr via
child_cmd.stderr(std::process::Stdio::piped()) but never reads from it (same
pattern around the stdout reader in the block covering the 191-211 region),
which can deadlock if the process writes to stderr; fix by either changing
child_cmd.stderr to inherit the parent's stderr
(child_cmd.stderr(std::process::Stdio::inherit())) or, if you need piping, after
spawning the child create a concurrent drain task that reads from the child's
stderr (similar to the existing stdout reader) and drops/logs the bytes so the
pipe never fills; update the logic around child_cmd and the stdout reader to
ensure stderr is handled in the same lifecycle.
In `@workers/orchestrator/src/main.rs`:
- Around line 250-258: The code currently swallows errors from iii.trigger(...)
for the "task::spawn_workers" invocation by calling
.await.unwrap_or(Value::Null), so failures to spawn workers go unnoticed; change
this so the trigger error is propagated or the system rolls back the persisted
plan/run to a non-executing state before returning. Specifically, remove the
unwrap_or(Value::Null) on spawn_result (the call to iii.trigger(TriggerRequest {
function_id: "task::spawn_workers", payload: json!({ "rootId": root_id }), ...
})), handle the Result/Option properly: on Err (or non-successful response)
return the error to the caller (propagate it) or call the existing plan/run
status update function to revert the run/plan from executing back to a
failed/ready state using root_id, then return an error; ensure any DB
persistence/transaction around persisting the run and marking it executing is
consistent with this rollback path.
In `@workers/skillkit-bridge/src/main.rs`:
- Around line 209-219: The code currently joins untrusted scan_path into
workspace_root (in the RegisterFunction::new_async closure) and then calls
scan_dir on root; to fix, canonicalize/resolve both workspace_root() and the
user-provided path (scan_path) into absolute paths, normalize them (e.g., with
std::fs::canonicalize or equivalent) and ensure the resulting target path starts
with the canonical workspace root prefix; if canonicalization fails or the
target is not inside the workspace root, return an error instead of calling
scan_dir; apply this check where root is computed and before invoking scan_dir
to prevent absolute paths or “..” escaping the workspace.
- Around line 34-69: The timeout branch currently leaves the `npx skillkit`
child running because `Command::output()` is used inside `tokio::time::timeout`,
so update `run_skillkit` to ensure subprocesses are killed on timeout: either
call `cmd.kill_on_drop(true)` before running `cmd.output()` or (preferred) spawn
the process with `cmd.spawn()` into a `Child`, await its `output` with
`timeout`, and on timeout call `child.kill().await` (and `child.wait().await` to
reaping) to ensure the process is terminated; reference `run_skillkit`,
`Command`, `cmd.output()`, `cmd.spawn()`, `kill_on_drop`, `Child::kill`, and
`Child::wait` when making the change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b286ff0b-d3d8-4c77-87f3-01987cf46a31
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
Cargo.tomlsrc/__tests__/browser.test.tssrc/__tests__/code-agent.test.tssrc/__tests__/mcp-client.test.tssrc/__tests__/orchestrator.test.tssrc/__tests__/skillkit-bridge.test.tssrc/browser.tssrc/code-agent.tssrc/index.tssrc/mcp-client.tssrc/orchestrator.tssrc/skillkit-bridge.tsworkers/browser/Cargo.tomlworkers/browser/iii.worker.yamlworkers/browser/src/main.rsworkers/browser/src/types.rsworkers/code-agent/Cargo.tomlworkers/code-agent/iii.worker.yamlworkers/code-agent/src/main.rsworkers/mcp-client/Cargo.tomlworkers/mcp-client/iii.worker.yamlworkers/mcp-client/src/main.rsworkers/orchestrator/Cargo.tomlworkers/orchestrator/iii.worker.yamlworkers/orchestrator/src/main.rsworkers/skillkit-bridge/Cargo.tomlworkers/skillkit-bridge/iii.worker.yamlworkers/skillkit-bridge/src/main.rs
💤 Files with no reviewable changes (11)
- src/tests/orchestrator.test.ts
- src/tests/mcp-client.test.ts
- src/tests/code-agent.test.ts
- src/mcp-client.ts
- src/code-agent.ts
- src/tests/skillkit-bridge.test.ts
- src/orchestrator.ts
- src/index.ts
- src/skillkit-bridge.ts
- src/browser.ts
- src/tests/browser.test.ts
| fn assert_no_ssrf(url_str: &str) -> Result<(), IIIError> { | ||
| let parsed = url::Url::parse(url_str).map_err(|e| IIIError::Handler(format!("invalid url: {e}")))?; | ||
| let scheme = parsed.scheme(); | ||
| if scheme != "http" && scheme != "https" { | ||
| return Err(IIIError::Handler(format!("blocked scheme: {scheme}"))); | ||
| } | ||
| let host = parsed.host_str().unwrap_or(""); | ||
| let blocked = ["localhost", "127.0.0.1", "0.0.0.0", "169.254.169.254", "metadata.google.internal"]; | ||
| if blocked.contains(&host) { | ||
| return Err(IIIError::Handler(format!("blocked host: {host}"))); | ||
| } | ||
| if host.starts_with("10.") | ||
| || host.starts_with("192.168.") | ||
| || host.starts_with("fc00:") | ||
| || host.starts_with("fd") | ||
| { | ||
| return Err(IIIError::Handler(format!("blocked private host: {host}"))); | ||
| } | ||
| if let Some(rest) = host.strip_prefix("172.") | ||
| && let Some(second_octet) = rest.split('.').next() | ||
| && let Ok(n) = second_octet.parse::<u32>() | ||
| && (16..=31).contains(&n) | ||
| { | ||
| return Err(IIIError::Handler(format!("blocked private host: {host}"))); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
Harden the SSRF guard against loopback/private IP variants.
The current host blocklist only rejects a few literal strings and prefixes. Inputs like 127.0.0.2, 127.1, ::1, and other IP forms will bypass this check, so the worker can still reach local services. Match on parsed.host() and reject loopback/private/link-local addresses by IP classification instead of string prefixes.
🔒 Proposed fix
fn assert_no_ssrf(url_str: &str) -> Result<(), IIIError> {
let parsed = url::Url::parse(url_str).map_err(|e| IIIError::Handler(format!("invalid url: {e}")))?;
let scheme = parsed.scheme();
if scheme != "http" && scheme != "https" {
return Err(IIIError::Handler(format!("blocked scheme: {scheme}")));
}
- let host = parsed.host_str().unwrap_or("");
- let blocked = ["localhost", "127.0.0.1", "0.0.0.0", "169.254.169.254", "metadata.google.internal"];
- if blocked.contains(&host) {
- return Err(IIIError::Handler(format!("blocked host: {host}")));
- }
- if host.starts_with("10.")
- || host.starts_with("192.168.")
- || host.starts_with("fc00:")
- || host.starts_with("fd")
- {
- return Err(IIIError::Handler(format!("blocked private host: {host}")));
- }
- if let Some(rest) = host.strip_prefix("172.")
- && let Some(second_octet) = rest.split('.').next()
- && let Ok(n) = second_octet.parse::<u32>()
- && (16..=31).contains(&n)
- {
- return Err(IIIError::Handler(format!("blocked private host: {host}")));
- }
+ match parsed.host() {
+ Some(url::Host::Ipv4(ip)) => {
+ if ip.is_loopback() || ip.is_private() || ip.is_link_local() || ip.is_unspecified() {
+ return Err(IIIError::Handler(format!("blocked host: {ip}")));
+ }
+ }
+ Some(url::Host::Ipv6(ip)) => {
+ if ip.is_loopback()
+ || ip.is_unique_local()
+ || ip.is_unicast_link_local()
+ || ip.is_unspecified()
+ {
+ return Err(IIIError::Handler(format!("blocked host: {ip}")));
+ }
+ }
+ Some(url::Host::Domain(host)) => {
+ if matches!(host, "localhost" | "metadata.google.internal") {
+ return Err(IIIError::Handler(format!("blocked host: {host}")));
+ }
+ }
+ None => return Err(IIIError::Handler("missing host".into())),
+ }
Ok(())
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/browser/src/main.rs` around lines 73 - 98, In assert_no_ssrf: replace
the brittle host_str() string-prefix checks with a match on parsed.host() (use
url::Host variants) and classify IPs properly—if parsed.host() yields
Host::Ipv4(ip) or Host::Ipv6(ip) call the appropriate methods (is_loopback(),
is_private()/is_global() semantics, is_link_local()) on the
std::net::Ipv4Addr/Ipv6Addr to reject loopback, private and link-local ranges
(e.g. 127.0.0.0/8, 10/8, 172.16/12, 192.168/16, ::1, fc00::/7, fe80::/10,
169.254.0.0/16, etc.); for Host::Domain keep the explicit domain blacklist
(metadata.google.internal, localhost) and the scheme check in assert_no_ssrf,
returning IIIError::Handler on any rejected host.
| let timeout = Duration::from_millis(DEFAULT_TIMEOUT_MS + 5_000); | ||
|
|
||
| let exec = Command::new("python3").arg(&session.script_path).arg(&payload_str).output(); | ||
|
|
||
| let output = tokio::time::timeout(timeout, exec) | ||
| .await | ||
| .map_err(|_| IIIError::Handler("browser script timed out".into()))? | ||
| .map_err(|e| IIIError::Handler(format!("spawn failed: {e}")))?; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Tokio's process API, if tokio::time::timeout(...)wrapstokio::process::Command::output()and the timeout elapses, is the spawned child process automatically killed, or must it be terminated explicitly (for example viaspawn+killorkill_on_drop)?
💡 Result:
When you wrap tokio::process::Command::output with tokio::time::timeout, the timeout does NOT automatically kill the spawned child process. What happens: - tokio::time::timeout cancels the wrapped future by dropping it when the deadline elapses. - For tokio::process::Command/Child, dropping the Child/command future/cancellation does not imply killing the process (the child continues running by default). The tokio docs explicitly state that a spawned process continues executing after the Child handle is dropped, unless Command::kill_on_drop is enabled. So you must explicitly terminate the process yourself on timeout (e.g., spawn a Child handle and call child.kill.await, or use Command::kill_on_drop(true) and ensure the handle is dropped in the timeout path). Practical takeaway: - Pattern like timeout(..., cmd.output).await returning Err(_) => treat it as “future cancelled”, not “process killed”. - Use cmd.spawn + child.kill.await in the timeout branch, or configure cmd.kill_on_drop(true) if dropping the handle is acceptable.
Citations:
- 1: https://docs.rs/tokio/latest/tokio/process/struct.Command.html
- 2: https://docs.rs/tokio/latest/tokio/time/fn.timeout.html
- 3: https://docs.rs/tokio/latest/tokio/process/struct.Child.html
- 4: https://docs.rs/tokio/latest/%20tokio/process/struct.Child.html
- 5: https://docs.rs/tokio/latest/aarch64-apple-darwin/tokio/process/index.html
🏁 Script executed:
cat -n workers/browser/src/main.rs | sed -n '170,200p'Repository: iii-experimental/agentos
Length of output: 1425
🏁 Script executed:
# Search for other timeout patterns wrapping Command::output or process spawning
rg -A 5 'Command::new.*output|timeout.*Command' workers/browser/src/main.rsRepository: iii-experimental/agentos
Length of output: 378
🏁 Script executed:
# Check if there are other similar patterns in the codebase
rg -B 2 -A 5 'Command::.*output' workers/Repository: iii-experimental/agentos
Length of output: 672
🏁 Script executed:
# Check imports at top of file and function signature
head -30 workers/browser/src/main.rsRepository: iii-experimental/agentos
Length of output: 1037
🏁 Script executed:
# Get the full function containing lines 181-188 to understand context
sed -n '140,190p' workers/browser/src/main.rsRepository: iii-experimental/agentos
Length of output: 1814
Child processes leak on timeout; add explicit process termination.
When tokio::time::timeout wraps Command::output() and the deadline elapses, the timeout cancels the future but does not kill the spawned python3 process—it continues running in the background. Repeated timeouts accumulate orphaned processes, exhausting worker resources. Use spawn() + wait_with_output() with explicit kill() on timeout instead.
Suggested fix
- let exec = Command::new("python3").arg(&session.script_path).arg(&payload_str).output();
-
- let output = tokio::time::timeout(timeout, exec)
- .await
- .map_err(|_| IIIError::Handler("browser script timed out".into()))?
- .map_err(|e| IIIError::Handler(format!("spawn failed: {e}")))?;
+ let mut child = Command::new("python3")
+ .arg(&session.script_path)
+ .arg(&payload_str)
+ .spawn()
+ .map_err(|e| IIIError::Handler(format!("spawn failed: {e}")))?;
+
+ let output = match tokio::time::timeout(timeout, child.wait_with_output()).await {
+ Ok(result) => result.map_err(|e| IIIError::Handler(format!("wait failed: {e}")))?,
+ Err(_) => {
+ let _ = child.kill().await;
+ return Err(IIIError::Handler("browser script timed out".into()));
+ }
+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/browser/src/main.rs` around lines 181 - 188, The current use of
Command::output() wrapped by tokio::time::timeout can leave the spawned python3
process running when the future times out; change the logic to use
Command::new("python3").arg(&session.script_path).arg(&payload_str).spawn() to
obtain a Child, then await child.wait_with_output() inside the timeout, and if
the timeout elapses call child.kill().await (or child.kill() then await
child.wait()) to ensure the process is terminated; update the error handling
around the exec/spawn, wait_with_output, and timeout branches (references: the
timeout variable, exec/spawn call, and the Child returned by
spawn/wait_with_output) so that timeouts return IIIError::Handler("browser
script timed out") only after attempting to kill the child and collecting its
exit to avoid leaking processes.
| if load_session(&iii, &agent_id).await.is_some() { | ||
| return Err(IIIError::Handler(format!( | ||
| "Session already exists for agent: {agent_id}" | ||
| ))); | ||
| } | ||
| let mut index = get_session_index(&iii).await; | ||
| if index.len() >= MAX_SESSIONS { | ||
| return Err(IIIError::Handler(format!("Max sessions ({MAX_SESSIONS}) reached"))); | ||
| } | ||
|
|
||
| let session_id = uuid::Uuid::new_v4().to_string(); | ||
| let script_path = | ||
| std::env::temp_dir().join(format!("browser-bridge-{session_id}.py")); | ||
| tokio::fs::write(&script_path, BRIDGE_SCRIPT) | ||
| .await | ||
| .map_err(|e| IIIError::Handler(format!("write script failed: {e}")))?; | ||
|
|
||
| let now = now_ms(); | ||
| let session = BrowserSession { | ||
| id: session_id.clone(), | ||
| agent_id: agent_id.clone(), | ||
| current_url: "about:blank".into(), | ||
| headless, | ||
| viewport: viewport.clone(), | ||
| created_at: now, | ||
| last_activity: now, | ||
| script_path: script_path.to_string_lossy().to_string(), | ||
| }; | ||
|
|
||
| save_session(&iii, &session).await?; | ||
| index.push(agent_id.clone()); | ||
| set_session_index(&iii, index).await?; |
There was a problem hiding this comment.
Session creation is a read-modify-write race.
This path does load_session / get_session_index / save_session / set_session_index as separate state calls. Two concurrent create_session requests can both pass the existence check, exceed MAX_SESSIONS, or clobber each other's index updates. This needs one atomic reservation/update path in the state layer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/browser/src/main.rs` around lines 293 - 324, The current create path
uses separate calls (load_session, get_session_index, save_session,
set_session_index) causing a read-modify-write race; implement an atomic
reservation/update in the state layer instead: add a single transactional API
(e.g., state.create_session_atomic or state.reserve_and_save_session) that
performs the existence check, MAX_SESSIONS check, generates the session_id and
script_path, writes the BrowserSession and updates the session index in one
atomic operation (or under a state-backed lock/CAS), and returns an error on
conflict; replace the multiple calls in the create_session handler with this
single atomic call to avoid races and clobbered index updates.
| assert_no_ssrf(url_str)?; | ||
| let mut session = load_session(&iii, agent_id) | ||
| .await | ||
| .ok_or_else(|| IIIError::Handler(format!("No browser session for agent: {agent_id}")))?; | ||
| touch_session(&iii, &mut session).await?; | ||
|
|
||
| let result = run_browser_script(&session, "navigate", json!({ "url": url_str })).await?; | ||
| let new_url = result["url"].as_str().unwrap_or(url_str).to_string(); | ||
| session.current_url = new_url.clone(); | ||
| save_session(&iii, &session).await?; |
There was a problem hiding this comment.
Redirects still give this worker an SSRF path.
assert_no_ssrf(url_str) only validates the initial input. page.goto() can follow a redirect to an internal address, and this code then stores that final URL in session.current_url, which later powers read/screenshot/click/type. That turns a single external redirect into a reusable SSRF primitive. The bridge needs to block redirected/requested destinations too, not just the first URL.
| let triggers = [ | ||
| ("browser::create_session", "POST", "api/browser/session"), | ||
| ("browser::list_sessions", "GET", "api/browser/sessions"), | ||
| ("tool::browser_navigate", "POST", "api/browser/navigate"), | ||
| ("tool::browser_screenshot", "POST", "api/browser/screenshot"), | ||
| ("tool::browser_read_page", "POST", "api/browser/read"), | ||
| ("tool::browser_close", "POST", "api/browser/close"), | ||
| ]; | ||
| for (fid, method, path) in triggers { | ||
| iii.register_trigger(RegisterTriggerInput { | ||
| trigger_type: "http".into(), | ||
| function_id: fid.to_string(), | ||
| config: json!({ "http_method": method, "api_path": path }), | ||
| metadata: None, | ||
| })?; | ||
| } |
There was a problem hiding this comment.
HTTP triggers for click and type were dropped.
tool::browser_click and tool::browser_type are registered above, but they never get HTTP triggers here. That breaks route parity with the TS worker and will 404 any callers still using those endpoints.
🌐 Proposed fix
let triggers = [
("browser::create_session", "POST", "api/browser/session"),
("browser::list_sessions", "GET", "api/browser/sessions"),
("tool::browser_navigate", "POST", "api/browser/navigate"),
+ ("tool::browser_click", "POST", "api/browser/click"),
+ ("tool::browser_type", "POST", "api/browser/type"),
("tool::browser_screenshot", "POST", "api/browser/screenshot"),
("tool::browser_read_page", "POST", "api/browser/read"),
("tool::browser_close", "POST", "api/browser/close"),
];📝 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.
| let triggers = [ | |
| ("browser::create_session", "POST", "api/browser/session"), | |
| ("browser::list_sessions", "GET", "api/browser/sessions"), | |
| ("tool::browser_navigate", "POST", "api/browser/navigate"), | |
| ("tool::browser_screenshot", "POST", "api/browser/screenshot"), | |
| ("tool::browser_read_page", "POST", "api/browser/read"), | |
| ("tool::browser_close", "POST", "api/browser/close"), | |
| ]; | |
| for (fid, method, path) in triggers { | |
| iii.register_trigger(RegisterTriggerInput { | |
| trigger_type: "http".into(), | |
| function_id: fid.to_string(), | |
| config: json!({ "http_method": method, "api_path": path }), | |
| metadata: None, | |
| })?; | |
| } | |
| let triggers = [ | |
| ("browser::create_session", "POST", "api/browser/session"), | |
| ("browser::list_sessions", "GET", "api/browser/sessions"), | |
| ("tool::browser_navigate", "POST", "api/browser/navigate"), | |
| ("tool::browser_click", "POST", "api/browser/click"), | |
| ("tool::browser_type", "POST", "api/browser/type"), | |
| ("tool::browser_screenshot", "POST", "api/browser/screenshot"), | |
| ("tool::browser_read_page", "POST", "api/browser/read"), | |
| ("tool::browser_close", "POST", "api/browser/close"), | |
| ]; | |
| for (fid, method, path) in triggers { | |
| iii.register_trigger(RegisterTriggerInput { | |
| trigger_type: "http".into(), | |
| function_id: fid.to_string(), | |
| config: json!({ "http_method": method, "api_path": path }), | |
| metadata: None, | |
| })?; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/browser/src/main.rs` around lines 587 - 602, The triggers array used
in the loop that calls iii.register_trigger (RegisterTriggerInput) is missing
entries for tool::browser_click and tool::browser_type, so their HTTP endpoints
never get registered; add entries for ("tool::browser_click", "POST",
"api/browser/click") and ("tool::browser_type", "POST", "api/browser/type")
(matching the other tool endpoints) to the triggers list so iii.register_trigger
registers those routes with the same pattern and config JSON used for the other
handlers.
| child_cmd.stdin(std::process::Stdio::piped()); | ||
| child_cmd.stdout(std::process::Stdio::piped()); | ||
| child_cmd.stderr(std::process::Stdio::piped()); |
There was a problem hiding this comment.
Don't pipe stderr unless you drain it.
Lines 179-181 create a piped stderr, but nothing ever reads from it. MCP servers that log steadily will eventually fill that pipe and block, which stalls the session even if stdout is still being consumed. Either inherit stderr or spawn a drain task for it alongside the stdout reader.
Also applies to: 191-211
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/mcp-client/src/main.rs` around lines 179 - 181, The code pipes child
process stderr via child_cmd.stderr(std::process::Stdio::piped()) but never
reads from it (same pattern around the stdout reader in the block covering the
191-211 region), which can deadlock if the process writes to stderr; fix by
either changing child_cmd.stderr to inherit the parent's stderr
(child_cmd.stderr(std::process::Stdio::inherit())) or, if you need piping, after
spawning the child create a concurrent drain task that reads from the child's
stderr (similar to the existing stdout reader) and drops/logs the bytes so the
pipe never fills; update the logic around child_cmd and the stdout reader to
ensure stderr is handled in the same lifecycle.
| } else if transport == "sse" && url.is_none() { | ||
| return Err(IIIError::Handler("SSE transport requires url".into())); | ||
| } | ||
|
|
||
| state.connections.insert(name.clone(), conn.clone()); | ||
|
|
||
| let init_result = send_rpc( | ||
| &conn, | ||
| "initialize", | ||
| json!({ | ||
| "protocolVersion": "2024-11-05", | ||
| "capabilities": { "tools": {} }, | ||
| "clientInfo": { "name": "agentos", "version": "0.0.1" }, | ||
| }), | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
Do not insert the connection before a transport-specific handshake succeeds.
Lines 215-230 still call send_rpc() for transport == "sse", even though no stdin transport was created, so SSE connects fail immediately with "connection has no stdin". Because the connection is inserted on Line 219 first, the failed handshake also leaves a dead entry behind. Return early for unsupported placeholder transports, or move state.connections.insert() until after initialize/tools/list succeeds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/mcp-client/src/main.rs` around lines 215 - 230, The code inserts the
connection into state.connections before running the transport-specific
handshake, causing dead entries when send_rpc (used to call "initialize" and
subsequent "tools/list") fails (e.g., SSE without stdin). Fix by deferring
state.connections.insert(name.clone(), conn.clone()) until after the handshake
completes successfully (after await on send_rpc and the tools list call), or
early-return for unsupported placeholder transports (when transport == "sse" &&
url.is_none()) before inserting; update the code paths around send_rpc, conn,
and transport to ensure the insert only happens on successful
initialize/tools/list.
| let spawn_result = iii | ||
| .trigger(TriggerRequest { | ||
| function_id: "task::spawn_workers".into(), | ||
| payload: json!({ "rootId": root_id }), | ||
| action: None, | ||
| timeout_ms: None, | ||
| }) | ||
| .await | ||
| .unwrap_or(Value::Null); |
There was a problem hiding this comment.
Propagate task::spawn_workers failures.
Lines 250-258 swallow any spawn error with unwrap_or(Value::Null) after the run has already been persisted and the plan has already been marked executing. If worker spawning is down, callers still get a success response even though nothing actually started. Return the error or roll the plan/run status back before responding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/orchestrator/src/main.rs` around lines 250 - 258, The code currently
swallows errors from iii.trigger(...) for the "task::spawn_workers" invocation
by calling .await.unwrap_or(Value::Null), so failures to spawn workers go
unnoticed; change this so the trigger error is propagated or the system rolls
back the persisted plan/run to a non-executing state before returning.
Specifically, remove the unwrap_or(Value::Null) on spawn_result (the call to
iii.trigger(TriggerRequest { function_id: "task::spawn_workers", payload:
json!({ "rootId": root_id }), ... })), handle the Result/Option properly: on Err
(or non-successful response) return the error to the caller (propagate it) or
call the existing plan/run status update function to revert the run/plan from
executing back to a failed/ready state using root_id, then return an error;
ensure any DB persistence/transaction around persisting the run and marking it
executing is consistent with this rollback path.
| async fn run_skillkit(args: &[&str], timeout_ms: u64) -> CliResult { | ||
| let mut cmd = Command::new("npx"); | ||
| cmd.arg("skillkit").args(args); | ||
| cmd.env_clear(); | ||
| for (k, v) in safe_env() { | ||
| cmd.env(k, v); | ||
| } | ||
| cmd.current_dir(workspace_root()); | ||
|
|
||
| let timeout = Duration::from_millis(timeout_ms); | ||
| let exec = cmd.output(); | ||
|
|
||
| match tokio::time::timeout(timeout, exec).await { | ||
| Ok(Ok(output)) => { | ||
| let mut stdout = String::from_utf8_lossy(&output.stdout).to_string(); | ||
| let mut stderr = String::from_utf8_lossy(&output.stderr).to_string(); | ||
| stdout.truncate(100_000); | ||
| stderr.truncate(50_000); | ||
| let exit_code = output.status.code().unwrap_or(if output.status.success() { 0 } else { 1 }); | ||
| CliResult { | ||
| stdout, | ||
| stderr, | ||
| exit_code, | ||
| } | ||
| } | ||
| Ok(Err(e)) => CliResult { | ||
| stdout: String::new(), | ||
| stderr: format!("spawn failed: {e}"), | ||
| exit_code: 1, | ||
| }, | ||
| Err(_) => CliResult { | ||
| stdout: String::new(), | ||
| stderr: "skillkit timed out".to_string(), | ||
| exit_code: 124, | ||
| }, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the target file
fd -t f "main.rs" | grep skillkit-bridgeRepository: iii-experimental/agentos
Length of output: 104
🏁 Script executed:
# Read the specific lines mentioned in the review (34-69)
head -n 75 workers/skillkit-bridge/src/main.rs | tail -n +30Repository: iii-experimental/agentos
Length of output: 1527
🏁 Script executed:
# Check the full file structure to understand context
wc -l workers/skillkit-bridge/src/main.rsRepository: iii-experimental/agentos
Length of output: 108
🌐 Web query:
Tokio Command::output() future drop cancellation behavior subprocess
💡 Result:
When the future returned by tokio::process::Command::output is dropped before completion, the subprocess is NOT automatically killed or cancelled by default. Unlike the typical async future paradigm where dropping implies cancellation, Tokio's process handling mimics std::process::Command behavior: the child process continues running independently even after the Child handle (and thus the output future) is dropped. To change this, use Command::kill_on_drop(true) before calling output. With this set, dropping the future will trigger a kill signal to the child process. Key caveats from official docs: - On Unix, killed processes become zombies until reaped. Tokio makes best-effort background reaping but provides no guarantees on timing. Avoid dropping Child handles without awaiting if strict cleanup is needed; prefer child.wait.await or child.kill.await. - The output future's destructor kills the child only if kill_on_drop is true. This behavior is explicitly documented in Tokio's source and docs.rs pages, current as of latest versions.
Citations:
- 1: https://github.com/tokio-rs/tokio/blob/master/tokio/src/process/mod.rs
- 2: https://docs.rs/tokio/latest/tokio/process/struct.Command.html
- 3: https://docs.rs/tokio/latest/aarch64-apple-darwin/tokio/process/index.html
🏁 Script executed:
# Check if there are any other subprocess management patterns in the codebase
rg "Command::new\|Child\|kill\|wait" workers/skillkit-bridge/src/main.rs -A 3 -B 3Repository: iii-experimental/agentos
Length of output: 50
🏁 Script executed:
# Check Tokio version in the Cargo.toml
find . -name "Cargo.toml" -path "*/skillkit-bridge/*" | xargs catRepository: iii-experimental/agentos
Length of output: 449
🏁 Script executed:
# Check if kill_on_drop is used anywhere
rg "kill_on_drop" workers/skillkit-bridge/src/main.rsRepository: iii-experimental/agentos
Length of output: 50
🏁 Script executed:
# Get the exact code lines 34-69 with context to ensure accuracy
sed -n '34,69p' workers/skillkit-bridge/src/main.rsRepository: iii-experimental/agentos
Length of output: 1285
🏁 Script executed:
# Check if there's a spawn() call instead of output()
rg "spawn\|output" workers/skillkit-bridge/src/main.rsRepository: iii-experimental/agentos
Length of output: 50
Kill the timed-out skillkit subprocess.
Line 44 uses Command::output() under tokio::time::timeout(), but when the timeout fires, the child process continues running. Tokio does not automatically kill subprocesses when futures are dropped—you must either call kill_on_drop(true) on the command or spawn the child explicitly and invoke kill()/wait() on timeout. Without this, slow npx skillkit calls will accumulate as orphaned processes after their requests return.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/skillkit-bridge/src/main.rs` around lines 34 - 69, The timeout branch
currently leaves the `npx skillkit` child running because `Command::output()` is
used inside `tokio::time::timeout`, so update `run_skillkit` to ensure
subprocesses are killed on timeout: either call `cmd.kill_on_drop(true)` before
running `cmd.output()` or (preferred) spawn the process with `cmd.spawn()` into
a `Child`, await its `output` with `timeout`, and on timeout call
`child.kill().await` (and `child.wait().await` to reaping) to ensure the process
is terminated; reference `run_skillkit`, `Command`, `cmd.output()`,
`cmd.spawn()`, `kill_on_drop`, `Child::kill`, and `Child::wait` when making the
change.
| RegisterFunction::new_async("skillkit::scan", move |input: Value| async move { | ||
| let scan_path = input["path"].as_str().unwrap_or("."); | ||
| let root = workspace_root().join(scan_path); | ||
| let mut found: Vec<Value> = Vec::new(); | ||
| scan_dir(&root, 0, &mut found).await; | ||
| let count = found.len(); | ||
| Ok::<Value, IIIError>(json!({ | ||
| "found": found, | ||
| "count": count, | ||
| "root": root.to_string_lossy(), | ||
| })) |
There was a problem hiding this comment.
Constrain scan_path to WORKSPACE_ROOT.
Line 211 joins user input directly into the workspace path. An absolute path will replace the workspace root entirely, and .. segments can escape it, so skillkit::scan can read SKILL.md / .well-known/* outside the workspace. Canonicalize both paths and reject anything that does not stay under the workspace root before scanning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/skillkit-bridge/src/main.rs` around lines 209 - 219, The code
currently joins untrusted scan_path into workspace_root (in the
RegisterFunction::new_async closure) and then calls scan_dir on root; to fix,
canonicalize/resolve both workspace_root() and the user-provided path
(scan_path) into absolute paths, normalize them (e.g., with
std::fs::canonicalize or equivalent) and ensure the resulting target path starts
with the canonical workspace root prefix; if canonicalization fails or the
target is not inside the workspace root, return an error instead of calling
scan_dir; apply this check where root is computed and before invoking scan_dir
to prevent absolute paths or “..” escaping the workspace.
rohitg00
force-pushed
the
feat/port-skillkit-browser-code-mcp-orchestrator
branch
from
6f43390 to
a3a90e9
Compare
| import "./approval.js"; | ||
| import "./approval-tiers.js"; | ||
| import "./context-cache.js"; | ||
| import "./context-manager.js"; | ||
| import "./context-monitor.js"; |
There was a problem hiding this comment.
🔴 index.ts adds imports for 8 TypeScript files that no longer exist, crashing the app at startup
The PR adds 8 new imports to src/index.ts for TypeScript worker modules (lsp-tools, context-cache, context-manager, context-monitor, security-headers, security-map, security-zeroize, skill-security) whose .ts source files were already deleted in previously-merged PRs (commits ba9c24a, 983f5d5) when those workers were ported to Rust. Since the application runs directly via tsx src/ (see package.json start script), these unresolvable imports will cause the entire application to crash on boot. This appears to be a merge/rebase artifact where lines from the merge-base index.ts were incorrectly re-introduced.
Was this helpful? React with 👍 or 👎 to provide feedback.
| var __result = (function(){{ | ||
| {code} | ||
| }})(); |
There was a problem hiding this comment.
🔴 Code-agent sandbox wraps code in IIFE that discards return value of last expression
The old TypeScript code-agent used vm.Script.runInContext() which returns the value of the last evaluated expression (e.g., 2 + 2 returns 4). The new Rust port wraps user code inside var __result = (function(){ CODE })(), but a JavaScript function without an explicit return statement returns undefined — so expressions like 2 + 2 or console.log('x'); 42 will produce null instead of 4 or 42. This breaks the core agent::code_execute contract.
How the wrapping causes the issue
For input code 2 + 2, the generated JS becomes:
var __result = (function(){ 2 + 2 })();Since the IIFE has no return, __result is undefined, which serializes as null.
Prompt for agents
The IIFE wrapper in run_sandboxed_js (workers/code-agent/src/main.rs, around line 44-62) wraps user code as `var __result = (function(){ CODE })()` which discards the last expression's value since there's no `return` statement.
The old TypeScript code used vm.Script.runInContext which natively returns the last expression value.
To fix this while keeping the sandbox setup (console mock, global shadowing), you need the inner function to return the last expression's value. Options:
1. Parse the code to add `return` before the last expression/statement. This is fragile but common.
2. Use QuickJS eval directly for the user code portion and set up console/globals separately via the context API rather than via JS wrapping.
3. Wrap as `var __result = (function(){ return eval(CODE_AS_STRING) })()` — but eval is currently shadowed as undefined in the sandbox. You'd need to capture a reference to the real eval before shadowing it.
The simplest approach: capture the real eval first, then shadow it, then use the captured reference:
var __eval = eval;
var eval = undefined;
var __result = __eval(USER_CODE_STRING);
But the user code string needs to be properly escaped as a JS string literal.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
workers/browser/src/main.rs (5)
587-594:⚠️ Potential issue | 🔴 Critical
clickandtyperoutes are missing from the HTTP trigger list.The functions are registered, but the HTTP endpoints are not, so route parity with the TS worker is broken and these calls will 404.
Suggested fix
let triggers = [ ("browser::create_session", "POST", "api/browser/session"), ("browser::list_sessions", "GET", "api/browser/sessions"), ("tool::browser_navigate", "POST", "api/browser/navigate"), + ("tool::browser_click", "POST", "api/browser/click"), + ("tool::browser_type", "POST", "api/browser/type"), ("tool::browser_screenshot", "POST", "api/browser/screenshot"), ("tool::browser_read_page", "POST", "api/browser/read"), ("tool::browser_close", "POST", "api/browser/close"), ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/browser/src/main.rs` around lines 587 - 594, The HTTP trigger list in the triggers array is missing entries for the click and type tool functions; add entries for ("tool::browser_click", "POST", "api/browser/click") and ("tool::browser_type", "POST", "api/browser/type") to the triggers array so the registered functions (browser_click and browser_type) have corresponding HTTP endpoints and won't 404; update the array near where triggers is defined so it includes those two tuples alongside the other routes.
183-188:⚠️ Potential issue | 🟠 MajorTimeout still does not kill the Playwright bridge process.
Dropping
Command::output()on timeout leaves the spawnedpython3child running. Repeated timeouts will leak bridge processes and exhaust worker resources. Usespawn()+wait_with_output()and callkill()in the timeout branch.If `tokio::time::timeout` wraps `tokio::process::Command::output()`, is the child process terminated automatically on timeout, or must it be killed explicitly?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/browser/src/main.rs` around lines 183 - 188, The current use of Command::output() with tokio::time::timeout can leave the spawned python3 child running on timeout; change the logic to call Command::new("python3").arg(&session.script_path).arg(&payload_str).spawn() to get a child handle (the variable currently named exec), then await child.wait_with_output() inside the timeout; if the timeout elapses, call child.kill() (and optionally child.wait() to reap it) before returning the timeout IIIError; ensure error handling still maps spawn and wait errors into IIIError::Handler as before.
390-393:⚠️ Potential issue | 🔴 CriticalPersisting the final URL keeps redirects as an SSRF foothold.
Only the requested URL is checked. If navigation or click lands on an internal destination, that final URL is stored in
session.current_urland then reused byread,screenshot,click, andtype. Re-validate the resolved URL before persisting it. Based on learnings: Applies to src/**/*.ts : Check for SSRF vulnerabilities usingassertNoSsrf(url)before any outbound HTTP requests.Also applies to: 428-430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/browser/src/main.rs` around lines 390 - 393, The code stores the post-navigation URL into session.current_url without re-validating it, allowing redirects to internal targets (SSRF); before assigning and persisting the resolved URL in the run_browser_script flow (where you call run_browser_script, set session.current_url, and call save_session), validate the resolved URL with the SSRF check function (e.g., assert_no_ssrf or assertNoSsrf equivalent) and only assign/save if the check passes; also apply the same validation before any later uses of session.current_url in read, screenshot, click, and type handlers to ensure reused URLs are re-validated prior to outbound requests.
293-324:⚠️ Potential issue | 🟠 MajorSession creation is still a read-modify-write race.
load_session/get_session_index/save_session/set_session_indexare separate state calls. Two concurrent creates can both pass the existence and capacity checks, then clobber each other's index update. This needs one atomic state-layer reservation/update path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/browser/src/main.rs` around lines 293 - 324, The current read-modify-write sequence using load_session, get_session_index, save_session and set_session_index is racy; replace it with a single atomic reservation/update in the state layer (e.g., add a new function like create_session_atomic or reserve_session) that performs: (1) check for existing session for agent and current index length, (2) if ok, persist the new BrowserSession and update the index in one atomic operation or transaction (or using a CAS/versioned update or backend transaction API), and return an error if the reservation fails; call that new atomic function from this handler instead of calling load_session/get_session_index/save_session/set_session_index separately.
73-98:⚠️ Potential issue | 🔴 CriticalThe SSRF guard is still bypassable by host variants.
String checks on
host_str()still miss loopback/private forms like127.0.0.2,127.1,::1, and other non-literal variants. Match onparsed.host()and classifyHost::Ipv4/Host::Ipv6directly instead of prefix matching.In Rust, does `url::Url::host()` distinguish IPv4/IPv6 hosts so loopback, private, link-local, and unspecified addresses can be blocked with `std::net::Ipv4Addr` / `Ipv6Addr` classification methods?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/browser/src/main.rs` around lines 73 - 98, The SSRF guard in assert_no_ssrf currently relies on host_str prefix checks and can be bypassed; change it to use parsed.host() and match on url::Host::Ipv4 and url::Host::Ipv6, convert to std::net::Ipv4Addr / Ipv6Addr and call their classification helpers (e.g., is_loopback(), is_private(), is_link_local()/is_unicast_link_local(), is_unspecified(), and for IPv6 also is_unique_local()) to deterministically reject loopback/private/link-local/unspecified addresses; keep the existing checks for non-http(s) schemes and literal hostnames like "localhost" and metadata names but remove ad-hoc starts_with prefix checks in assert_no_ssrf.workers/orchestrator/src/main.rs (1)
250-258:⚠️ Potential issue | 🟠 MajorDon't hide
task::spawn_workersfailures.This still marks the plan as executing and persists the run before calling
unwrap_or(Value::Null). If spawning fails, callers get a success payload even though nothing started. Return the trigger error or roll the plan/run state back first.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/orchestrator/src/main.rs` around lines 250 - 258, The current code hides failures from iii.trigger(...) for function_id "task::spawn_workers" by calling .await.unwrap_or(Value::Null) on the trigger result (assigned to spawn_result) after persisting the plan/run; instead, handle the trigger Result explicitly: await the trigger without unwrap_or, propagate the error (use ? or return Err) or, if you must persist first, perform a compensating rollback of the plan/run state when the trigger returns Err; update the spawn_result handling (remove unwrap_or(Value::Null) and match on the Result from iii.trigger/TriggerRequest) so callers receive the actual trigger error or the run/plan is rolled back before returning success.workers/code-agent/src/main.rs (1)
123-125:⚠️ Potential issue | 🔴 CriticalTokio timeout still does not stop the QuickJS execution.
timeout(..., spawn_blocking(...))only stops waiting on theJoinHandle;run_sandboxed_jskeeps running after the deadline. A payload likewhile (true) {}can continue burning a blocking thread and starve later executions. Enforce the deadline inside QuickJS with an interrupt handler and use the same deadline in both places.Does `tokio::time::timeout` cancel a running `tokio::task::spawn_blocking` task, and does `rquickjs` 0.9 expose `Runtime::set_interrupt_handler` for cooperative cancellation?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/code-agent/src/main.rs` around lines 123 - 125, The current timeout only stops awaiting the JoinHandle so run_sandboxed_js keeps running; fix by installing a QuickJS interrupt handler inside run_sandboxed_js (use rquickjs Runtime::set_interrupt_handler or equivalent) that checks a shared deadline value (pass the Instant deadline or an AtomicU64/ms timestamp into the spawn_blocking closure) and returns true to abort execution when now > deadline, and also use that same deadline from the caller side: create the deadline before spawning, move it into spawn_blocking/run_sandboxed_js to set the interrupt handler, and if tokio::time::timeout returns Err call the runtime's interrupt mechanism (or flip the shared flag) to ensure the VM stops and the blocking thread is freed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workers/orchestrator/src/main.rs`:
- Around line 385-401: The handler currently sets plan.status = "executing" and
r.status = "running", persists via state_set, then tokio::spawns an async block
that calls iii.trigger(TriggerRequest { function_id: "task::spawn_workers", ...
}) and ignores the result, so failures can leave a run marked running with no
workers; change this to await the trigger call (do not fire-and-forget) and
check its Result, and on Err (or non-success) revert plan.status and r.status
back (or set an error state) and persist via state_set (same keys used now) so
the caller only reports success when the spawn_workers trigger actually
succeeded; references: the "resume" match arm, plan.status, run.as_mut()/r,
state_set(...), iii.trigger(TriggerRequest { function_id: "task::spawn_workers",
... }) and the tokio::spawn usage.
---
Duplicate comments:
In `@workers/browser/src/main.rs`:
- Around line 587-594: The HTTP trigger list in the triggers array is missing
entries for the click and type tool functions; add entries for
("tool::browser_click", "POST", "api/browser/click") and ("tool::browser_type",
"POST", "api/browser/type") to the triggers array so the registered functions
(browser_click and browser_type) have corresponding HTTP endpoints and won't
404; update the array near where triggers is defined so it includes those two
tuples alongside the other routes.
- Around line 183-188: The current use of Command::output() with
tokio::time::timeout can leave the spawned python3 child running on timeout;
change the logic to call
Command::new("python3").arg(&session.script_path).arg(&payload_str).spawn() to
get a child handle (the variable currently named exec), then await
child.wait_with_output() inside the timeout; if the timeout elapses, call
child.kill() (and optionally child.wait() to reap it) before returning the
timeout IIIError; ensure error handling still maps spawn and wait errors into
IIIError::Handler as before.
- Around line 390-393: The code stores the post-navigation URL into
session.current_url without re-validating it, allowing redirects to internal
targets (SSRF); before assigning and persisting the resolved URL in the
run_browser_script flow (where you call run_browser_script, set
session.current_url, and call save_session), validate the resolved URL with the
SSRF check function (e.g., assert_no_ssrf or assertNoSsrf equivalent) and only
assign/save if the check passes; also apply the same validation before any later
uses of session.current_url in read, screenshot, click, and type handlers to
ensure reused URLs are re-validated prior to outbound requests.
- Around line 293-324: The current read-modify-write sequence using
load_session, get_session_index, save_session and set_session_index is racy;
replace it with a single atomic reservation/update in the state layer (e.g., add
a new function like create_session_atomic or reserve_session) that performs: (1)
check for existing session for agent and current index length, (2) if ok,
persist the new BrowserSession and update the index in one atomic operation or
transaction (or using a CAS/versioned update or backend transaction API), and
return an error if the reservation fails; call that new atomic function from
this handler instead of calling
load_session/get_session_index/save_session/set_session_index separately.
- Around line 73-98: The SSRF guard in assert_no_ssrf currently relies on
host_str prefix checks and can be bypassed; change it to use parsed.host() and
match on url::Host::Ipv4 and url::Host::Ipv6, convert to std::net::Ipv4Addr /
Ipv6Addr and call their classification helpers (e.g., is_loopback(),
is_private(), is_link_local()/is_unicast_link_local(), is_unspecified(), and for
IPv6 also is_unique_local()) to deterministically reject
loopback/private/link-local/unspecified addresses; keep the existing checks for
non-http(s) schemes and literal hostnames like "localhost" and metadata names
but remove ad-hoc starts_with prefix checks in assert_no_ssrf.
In `@workers/code-agent/src/main.rs`:
- Around line 123-125: The current timeout only stops awaiting the JoinHandle so
run_sandboxed_js keeps running; fix by installing a QuickJS interrupt handler
inside run_sandboxed_js (use rquickjs Runtime::set_interrupt_handler or
equivalent) that checks a shared deadline value (pass the Instant deadline or an
AtomicU64/ms timestamp into the spawn_blocking closure) and returns true to
abort execution when now > deadline, and also use that same deadline from the
caller side: create the deadline before spawning, move it into
spawn_blocking/run_sandboxed_js to set the interrupt handler, and if
tokio::time::timeout returns Err call the runtime's interrupt mechanism (or flip
the shared flag) to ensure the VM stops and the blocking thread is freed.
In `@workers/orchestrator/src/main.rs`:
- Around line 250-258: The current code hides failures from iii.trigger(...) for
function_id "task::spawn_workers" by calling .await.unwrap_or(Value::Null) on
the trigger result (assigned to spawn_result) after persisting the plan/run;
instead, handle the trigger Result explicitly: await the trigger without
unwrap_or, propagate the error (use ? or return Err) or, if you must persist
first, perform a compensating rollback of the plan/run state when the trigger
returns Err; update the spawn_result handling (remove unwrap_or(Value::Null) and
match on the Result from iii.trigger/TriggerRequest) so callers receive the
actual trigger error or the run/plan is rolled back before returning success.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e4508152-0361-4479-b597-fec4a850b059
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
Cargo.tomlsrc/__tests__/browser.test.tssrc/__tests__/code-agent.test.tssrc/__tests__/mcp-client.test.tssrc/__tests__/orchestrator.test.tssrc/__tests__/skillkit-bridge.test.tssrc/browser.tssrc/code-agent.tssrc/index.tssrc/mcp-client.tssrc/orchestrator.tssrc/skillkit-bridge.tsworkers/browser/Cargo.tomlworkers/browser/iii.worker.yamlworkers/browser/src/main.rsworkers/browser/src/types.rsworkers/code-agent/Cargo.tomlworkers/code-agent/iii.worker.yamlworkers/code-agent/src/main.rsworkers/mcp-client/Cargo.tomlworkers/mcp-client/iii.worker.yamlworkers/mcp-client/src/main.rsworkers/orchestrator/Cargo.tomlworkers/orchestrator/iii.worker.yamlworkers/orchestrator/src/main.rsworkers/skillkit-bridge/Cargo.tomlworkers/skillkit-bridge/iii.worker.yamlworkers/skillkit-bridge/src/main.rs
💤 Files with no reviewable changes (10)
- src/mcp-client.ts
- src/tests/skillkit-bridge.test.ts
- src/tests/orchestrator.test.ts
- src/browser.ts
- src/skillkit-bridge.ts
- src/tests/browser.test.ts
- src/code-agent.ts
- src/tests/code-agent.test.ts
- src/tests/mcp-client.test.ts
- src/orchestrator.ts
✅ Files skipped from review due to trivial changes (11)
- workers/code-agent/iii.worker.yaml
- workers/mcp-client/iii.worker.yaml
- workers/skillkit-bridge/iii.worker.yaml
- workers/browser/iii.worker.yaml
- workers/orchestrator/iii.worker.yaml
- workers/mcp-client/Cargo.toml
- workers/orchestrator/Cargo.toml
- workers/skillkit-bridge/Cargo.toml
- workers/code-agent/Cargo.toml
- workers/browser/src/types.rs
- workers/skillkit-bridge/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/index.ts
- workers/mcp-client/src/main.rs
| fn sanitize_id(s: &str) -> String { | ||
| s.chars() | ||
| .filter(|c| c.is_alphanumeric() || matches!(c, '_' | '-' | ':' | '.')) | ||
| .take(256) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
Reject invalid IDs instead of rewriting them.
Filtering and truncating silently aliases distinct inputs onto the same workspace scope/key, so one request can read or overwrite another plan's data. workers/task-decomposer/src/types.rs:53-75 already uses reject-on-invalid semantics; mirroring that here avoids collisions.
| "resume" => { | ||
| plan.status = "executing".into(); | ||
| if let Some(r) = run.as_mut() { | ||
| r.status = "running".into(); | ||
| let root_id = r.root_id.clone(); | ||
| let _ = state_set(iii, "orchestrator_runs", &plan_id, serde_json::to_value(r).unwrap()).await; | ||
| let iii_clone = iii.clone(); | ||
| tokio::spawn(async move { | ||
| let _ = iii_clone | ||
| .trigger(TriggerRequest { | ||
| function_id: "task::spawn_workers".into(), | ||
| payload: json!({ "rootId": root_id }), | ||
| action: None, | ||
| timeout_ms: None, | ||
| }) | ||
| .await; | ||
| }); |
There was a problem hiding this comment.
Resume reports success before workers are actually respawned.
The handler persists executing/running, then detaches task::spawn_workers and ignores its result. A failed trigger leaves the run stuck in a running state with no workers. Await the trigger here or revert the status on failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/orchestrator/src/main.rs` around lines 385 - 401, The handler
currently sets plan.status = "executing" and r.status = "running", persists via
state_set, then tokio::spawns an async block that calls
iii.trigger(TriggerRequest { function_id: "task::spawn_workers", ... }) and
ignores the result, so failures can leave a run marked running with no workers;
change this to await the trigger call (do not fire-and-forget) and check its
Result, and on Err (or non-success) revert plan.status and r.status back (or set
an error state) and persist via state_set (same keys used now) so the caller
only reports success when the spawn_workers trigger actually succeeded;
references: the "resume" match arm, plan.status, run.as_mut()/r, state_set(...),
iii.trigger(TriggerRequest { function_id: "task::spawn_workers", ... }) and the
tokio::spawn usage.
a3a90e9 to
7e9571e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
workers/mcp-client/src/main.rs (1)
215-230:⚠️ Potential issue | 🟠 MajorSSE transport with URL falls through to guaranteed failure.
When
transport == "sse"andurl.is_some(), the condition on line 215 is false, so code continues. Line 219 inserts the connection, then lines 221-230 callsend_rpc(), which fails immediately at line 88-90 becausestdinwas never initialized for SSE. This leaves a dead entry instate.connections.Either return early for unimplemented SSE transport, or defer the insert until after successful handshake.
🐛 Proposed fix: Return early for SSE placeholder
- } else if transport == "sse" && url.is_none() { - return Err(IIIError::Handler("SSE transport requires url".into())); + } else if transport == "sse" { + // SSE transport not yet implemented + return Err(IIIError::Handler("SSE transport not yet implemented".into())); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/mcp-client/src/main.rs` around lines 215 - 230, The SSE branch allows transport == "sse" with url.is_some() to continue and inserts the connection into state.connections before performing the handshake, which leaves a stale entry when send_rpc (called with conn and "initialize") fails because SSE-specific fields (e.g., stdin) aren’t initialized; fix by either returning early for unimplemented SSE (e.g., return Err(IIIError::Handler("SSE transport not implemented".into())) when transport == "sse") or move the state.connections.insert(name.clone(), conn.clone()) to after the send_rpc("initialize", ...) handshake succeeds so the connection is only stored on successful initialization (update the code paths around transport, url, state.connections.insert, and the send_rpc/initialize call accordingly).workers/code-agent/src/main.rs (1)
30-37:⚠️ Potential issue | 🔴 CriticalTimeout does not terminate runaway JS execution (still an unresolved blocker).
Line 124-125 wraps the blocking task with
tokio::time::timeout, but that only stops waiting; it does not stop an already-runningspawn_blockingclosure. Without a QuickJS interrupt check, infinite loops can continue consuming blocking threads after"execution timeout"is returned.Suggested fix (deadline + QuickJS interrupt handler)
-fn run_sandboxed_js(code: &str) -> (Value, String) { +fn run_sandboxed_js(code: &str, deadline: Instant) -> (Value, String) { use rquickjs::{Context, Runtime}; let runtime = match Runtime::new() { Ok(r) => r, Err(e) => return (json!({ "error": format!("runtime init failed: {e}") }), String::new()), }; runtime.set_memory_limit(64 * 1024 * 1024); + runtime.set_interrupt_handler(Some(move || Instant::now() >= deadline)); @@ - let join = tokio::task::spawn_blocking(move || run_sandboxed_js(&code)); + let deadline = Instant::now() + Duration::from_millis(timeout_ms); + let join = tokio::task::spawn_blocking(move || run_sandboxed_js(&code, deadline));#!/bin/bash # Verify current code has timeout+spawn_blocking but no interrupt handler. rg -n -C3 'spawn_blocking|tokio::time::timeout|set_interrupt_handler' workers/code-agent/src/main.rsIn Tokio, does `tokio::time::timeout` cancel an already-running `spawn_blocking` task, and in rquickjs 0.9 is `Runtime::set_interrupt_handler` the supported way to stop long-running JS?Also applies to: 123-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/code-agent/src/main.rs` around lines 30 - 37, The current timeout only stops waiting but doesn't stop the spawned blocking JS execution; modify run_sandboxed_js and the surrounding execution logic to install a QuickJS interrupt handler on the Runtime (use Runtime::set_interrupt_handler) that checks an Arc<AtomicBool>/AtomicUsize flag and returns true to abort when the flag is set, then have the async side wrap the spawn_blocking future with tokio::time::timeout and on timeout set that shared flag so the interrupt handler will terminate runaway JS; ensure the interrupt flag is created before Runtime::new and passed into the spawn_blocking closure and into Runtime::set_interrupt_handler, and keep memory limits and proper cleanup after interrupt.
🧹 Nitpick comments (2)
workers/mcp-client/src/main.rs (2)
303-306: Remove dead code in disconnect.The loop on lines 303-305 iterates over pending entries but only accesses and discards each key—it has no observable effect. The subsequent
clear()is sufficient to drop the senders (which notifies waiting receivers).♻️ Proposed fix
- for kv in conn.pending.iter() { - let _ = kv.key(); - } conn.pending.clear();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/mcp-client/src/main.rs` around lines 303 - 306, The loop in disconnect that does `for kv in conn.pending.iter() { let _ = kv.key(); }` is dead code—remove that loop and only call `conn.pending.clear()` to drop the pending senders; update the function/method containing `conn.pending.iter()` and `conn.pending.clear()` (the disconnect code path) to delete the unused iteration over `kv.key()` so the behavior relies solely on `conn.pending.clear()`.
467-474: Consider adding a timeout to the trigger call.The
iii.trigger()call withintools/callhas notimeout_msset. If the target function hangs, the MCP request will block indefinitely. Consider adding a reasonable timeout to prevent unbounded waits.♻️ Proposed fix
match iii .trigger(TriggerRequest { function_id: function_id.clone(), payload: arguments, action: None, - timeout_ms: None, + timeout_ms: Some(30_000), // Match RPC_TIMEOUT_MS }) .await🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/mcp-client/src/main.rs` around lines 467 - 474, The trigger call on iii.trigger(TriggerRequest { ... }) in tools/call currently omits timeout_ms and can block indefinitely; set a sensible timeout_ms (e.g., milliseconds constant or derived from the incoming MCP request) on the TriggerRequest before awaiting iii.trigger so the call fails fast on hung targets; update any related error handling around the returned Result from iii.trigger to handle timeout errors (reference: iii.trigger, TriggerRequest, tools/call).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workers/code-agent/src/main.rs`:
- Around line 76-78: The handler currently calls
stdout_trim.truncate(MAX_OUTPUT_LENGTH) which can panic on UTF-8 multi-byte
boundaries; change the logic that prepares stdout_trim (the value paired with
truncate_result(result)) to compute the last valid byte boundary at or before
MAX_OUTPUT_LENGTH using stdout_trim.char_indices() (or similar) and then
truncate/slice by that byte index instead of calling truncate with a
possibly-invalid boundary; update the code around stdout_trim, MAX_OUTPUT_LENGTH
and the tuple returned with (truncate_result(result), stdout_trim) so stdout is
safely shortened to a valid UTF-8 boundary.
In `@workers/skillkit-bridge/src/main.rs`:
- Around line 44-51: The current code uses cmd.output() (assigned to exec) which
buffers entire stdout/stderr then truncates; change to cmd.spawn() to obtain a
Child and piped ChildStdout/ChildStderr, then drive the child under
tokio::time::timeout(timeout, ...), reading both streams incrementally with
tokio::io::BufReader (or async read_buf) and enforcing per-stream byte limits
while reading (e.g., stop reading and record "truncated" once limits reached)
instead of truncating after collecting; ensure you handle killing the Child on
timeout and await its exit, and remove the post-hoc
stdout.truncate/stderr.truncate logic tied to the output variable.
---
Duplicate comments:
In `@workers/code-agent/src/main.rs`:
- Around line 30-37: The current timeout only stops waiting but doesn't stop the
spawned blocking JS execution; modify run_sandboxed_js and the surrounding
execution logic to install a QuickJS interrupt handler on the Runtime (use
Runtime::set_interrupt_handler) that checks an Arc<AtomicBool>/AtomicUsize flag
and returns true to abort when the flag is set, then have the async side wrap
the spawn_blocking future with tokio::time::timeout and on timeout set that
shared flag so the interrupt handler will terminate runaway JS; ensure the
interrupt flag is created before Runtime::new and passed into the spawn_blocking
closure and into Runtime::set_interrupt_handler, and keep memory limits and
proper cleanup after interrupt.
In `@workers/mcp-client/src/main.rs`:
- Around line 215-230: The SSE branch allows transport == "sse" with
url.is_some() to continue and inserts the connection into state.connections
before performing the handshake, which leaves a stale entry when send_rpc
(called with conn and "initialize") fails because SSE-specific fields (e.g.,
stdin) aren’t initialized; fix by either returning early for unimplemented SSE
(e.g., return Err(IIIError::Handler("SSE transport not implemented".into()))
when transport == "sse") or move the state.connections.insert(name.clone(),
conn.clone()) to after the send_rpc("initialize", ...) handshake succeeds so the
connection is only stored on successful initialization (update the code paths
around transport, url, state.connections.insert, and the send_rpc/initialize
call accordingly).
---
Nitpick comments:
In `@workers/mcp-client/src/main.rs`:
- Around line 303-306: The loop in disconnect that does `for kv in
conn.pending.iter() { let _ = kv.key(); }` is dead code—remove that loop and
only call `conn.pending.clear()` to drop the pending senders; update the
function/method containing `conn.pending.iter()` and `conn.pending.clear()` (the
disconnect code path) to delete the unused iteration over `kv.key()` so the
behavior relies solely on `conn.pending.clear()`.
- Around line 467-474: The trigger call on iii.trigger(TriggerRequest { ... })
in tools/call currently omits timeout_ms and can block indefinitely; set a
sensible timeout_ms (e.g., milliseconds constant or derived from the incoming
MCP request) on the TriggerRequest before awaiting iii.trigger so the call fails
fast on hung targets; update any related error handling around the returned
Result from iii.trigger to handle timeout errors (reference: iii.trigger,
TriggerRequest, tools/call).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: af48bb75-f592-4efa-aa0f-a7a87558e4b4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
Cargo.tomlsrc/__tests__/browser.test.tssrc/__tests__/code-agent.test.tssrc/__tests__/mcp-client.test.tssrc/__tests__/orchestrator.test.tssrc/__tests__/skillkit-bridge.test.tssrc/browser.tssrc/code-agent.tssrc/index.tssrc/mcp-client.tssrc/orchestrator.tssrc/skillkit-bridge.tsworkers/browser/Cargo.tomlworkers/browser/iii.worker.yamlworkers/browser/src/main.rsworkers/browser/src/types.rsworkers/code-agent/Cargo.tomlworkers/code-agent/iii.worker.yamlworkers/code-agent/src/main.rsworkers/mcp-client/Cargo.tomlworkers/mcp-client/iii.worker.yamlworkers/mcp-client/src/main.rsworkers/orchestrator/Cargo.tomlworkers/orchestrator/iii.worker.yamlworkers/orchestrator/src/main.rsworkers/skillkit-bridge/Cargo.tomlworkers/skillkit-bridge/iii.worker.yamlworkers/skillkit-bridge/src/main.rs
💤 Files with no reviewable changes (10)
- src/skillkit-bridge.ts
- src/tests/orchestrator.test.ts
- src/tests/mcp-client.test.ts
- src/tests/skillkit-bridge.test.ts
- src/mcp-client.ts
- src/orchestrator.ts
- src/tests/code-agent.test.ts
- src/code-agent.ts
- src/browser.ts
- src/tests/browser.test.ts
✅ Files skipped from review due to trivial changes (10)
- workers/skillkit-bridge/iii.worker.yaml
- workers/orchestrator/iii.worker.yaml
- workers/browser/iii.worker.yaml
- workers/browser/Cargo.toml
- workers/mcp-client/iii.worker.yaml
- workers/skillkit-bridge/Cargo.toml
- workers/mcp-client/Cargo.toml
- workers/orchestrator/Cargo.toml
- workers/code-agent/iii.worker.yaml
- workers/code-agent/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- Cargo.toml
- src/index.ts
- workers/orchestrator/src/main.rs
| let mut stdout_trim = stdout; | ||
| stdout_trim.truncate(MAX_OUTPUT_LENGTH); | ||
| (truncate_result(result), stdout_trim) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate potentially unsafe truncation points for manual validation.
rg -n -C2 'truncate\s*\(' workers/code-agent/src/main.rsRepository: iii-experimental/agentos
Length of output: 329
🌐 Web query:
Does Rust String::truncate panic when the index is not on a UTF-8 character boundary?
💡 Result:
Yes, Rust's String::truncate panics when the provided index is not on a UTF-8 character boundary. The official documentation explicitly states: "Panics if new_len does not lie on a char boundary."
Citations:
- 1: https://doc.rust-lang.org/stable/std/string/struct.String.html
- 2: https://doc.rust-lang.org/std/string/struct.String.html
String::truncate() panics on non-UTF-8 char boundaries, crashing the handler for large non-ASCII stdout.
Line 77 calls stdout_trim.truncate(MAX_OUTPUT_LENGTH) which panics if MAX_OUTPUT_LENGTH is not aligned to a UTF-8 character boundary. With any multi-byte Unicode characters in stdout, this will crash the handler.
Use char_indices() to find the last valid char boundary at or before MAX_OUTPUT_LENGTH:
Safe truncation at char boundary
- let mut stdout_trim = stdout;
- stdout_trim.truncate(MAX_OUTPUT_LENGTH);
+ let mut stdout_trim = stdout;
+ if stdout_trim.len() > MAX_OUTPUT_LENGTH {
+ let cut = stdout_trim
+ .char_indices()
+ .map(|(i, _)| i)
+ .take_while(|&i| i <= MAX_OUTPUT_LENGTH)
+ .last()
+ .unwrap_or(0);
+ stdout_trim.truncate(cut);
+ }
(truncate_result(result), stdout_trim)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/code-agent/src/main.rs` around lines 76 - 78, The handler currently
calls stdout_trim.truncate(MAX_OUTPUT_LENGTH) which can panic on UTF-8
multi-byte boundaries; change the logic that prepares stdout_trim (the value
paired with truncate_result(result)) to compute the last valid byte boundary at
or before MAX_OUTPUT_LENGTH using stdout_trim.char_indices() (or similar) and
then truncate/slice by that byte index instead of calling truncate with a
possibly-invalid boundary; update the code around stdout_trim, MAX_OUTPUT_LENGTH
and the tuple returned with (truncate_result(result), stdout_trim) so stdout is
safely shortened to a valid UTF-8 boundary.
| let exec = cmd.output(); | ||
|
|
||
| match tokio::time::timeout(timeout, exec).await { | ||
| Ok(Ok(output)) => { | ||
| let mut stdout = String::from_utf8_lossy(&output.stdout).to_string(); | ||
| let mut stderr = String::from_utf8_lossy(&output.stderr).to_string(); | ||
| stdout.truncate(100_000); | ||
| stderr.truncate(50_000); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does tokio::process::Command::output() buffer the entire child process stdout/stderr in memory before returning, and what Tokio API pattern is recommended to enforce byte limits while reading?
💡 Result:
Yes, tokio::process::Command::output buffers the entire child process stdout and stderr in memory (as Vec<u8>) before returning. It internally spawns the process with piped stdout/stderr, then concurrently awaits the child exit status and calls read_to_end on both streams to collect all data into vectors via tokio::io::util::read_to_end. To enforce byte limits while reading without full buffering, use Command::spawn to get a Child, take the ChildStdout/ChildStderr handles (which implement AsyncRead), and read them incrementally in a loop with custom bounds. Use tokio::io::BufReader for line-based reading or raw read/read_buf for bytes. Example pattern from official docs: use tokio::io::{self, BufReader, AsyncBufReadExt}; use tokio::process::Command; use std::process::Stdio; #[tokio::main] async fn main -> io::Result<> { let mut cmd = Command::new("your-command") .stdout(Stdio::piped) .stderr(Stdio::piped) .spawn?; let mut stdout = BufReader::new(cmd.stdout.take.unwrap).lines; let mut stderr = BufReader::new(cmd.stderr.take.unwrap).lines; // In separate tasks or select! for concurrent reading tokio::spawn(async move { while let Some(line) = stdout.next_line.await? { // Process line, enforce limits (e.g., drop if buffer exceeds limit) if your_output_buffer.len > MAX_BYTES { your_output_buffer.clear; // or other limit logic } your_output_buffer.push(line); } Ok::<, io::Error>() }); tokio::spawn(async move { while let Some(line) = stderr.next_line.await? { // Similar limit enforcement } Ok::<, io::Error>() }); // Wait for process let status = cmd.wait.await?; Ok() } For raw byte limits, use read_buf in a loop and truncate/discard when exceeding limits. This streams output as produced, avoiding OOM from large outputs. Official docs recommend spawning child.wait in a task to allow concurrent progress.
Citations:
- 1: https://github.com/tokio-rs/tokio/blob/master/tokio/src/process/mod.rs
- 2: https://docs.rs/tokio/latest/tokio/process/struct.Command.html
- 3: https://docs.rs/tokio/latest/tokio/process/index.html
- 4: https://docs.rs/tokio/latest/aarch64-apple-darwin/tokio/process/index.html
Refactor to stream child process output instead of buffering entirely.
tokio::process::Command::output() buffers the entire stdout/stderr in memory before returning, so truncating at lines 50–51 does not prevent a noisy skillkit invocation from exhausting worker memory. Replace with Command::spawn() to get piped handles (ChildStdout/ChildStderr), then read incrementally using tokio::io::BufReader or raw read_buf calls with byte-limit enforcement while reading, not after collection.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workers/skillkit-bridge/src/main.rs` around lines 44 - 51, The current code
uses cmd.output() (assigned to exec) which buffers entire stdout/stderr then
truncates; change to cmd.spawn() to obtain a Child and piped
ChildStdout/ChildStderr, then drive the child under
tokio::time::timeout(timeout, ...), reading both streams incrementally with
tokio::io::BufReader (or async read_buf) and enforcing per-stream byte limits
while reading (e.g., stop reading and record "truncated" once limits reached)
instead of truncating after collecting; ensure you handle killing the Child on
timeout and await its exit, and remove the post-hoc
stdout.truncate/stderr.truncate logic tied to the output variable.
Spawns skillkit CLI via tokio::process::Command instead of npx in Node.js.
Preserves function_ids skillkit::{search,install,list,recommend,scan} and
HTTP routes under api/skillkit/*. Safe-env allowlist, validation, and
JSON parsing match the TS contract.
Reuses the same Python+playwright bridge subprocess approach as the TS
version (avoids bundling Chromium). Tokio runs the cleanup loop and
spawns python3 with serialized payload. Preserves function_ids
browser::{create_session,list_sessions} and tool::browser_{navigate,
click,type,screenshot,read_page,close}, plus all api/browser/* routes
and SSRF host blocklist.
agent::code_detect uses regex to find typescript/javascript fenced blocks. agent::code_execute swaps Node.js vm sandbox for QuickJS via rquickjs with a 64MB memory cap, console.log capture, disabled eval/Function/setTimeout/process/require, and a tokio task with a hard wall-clock timeout. Output truncation matches the TS contract.
Ports the agentos-specific mcp-client (separate from iii-hq/workers/mcp-client because of mcp::serve_handler, mcp::serve, mcp::unserve, and the mcp_<server>_<tool> namespacing pattern). stdio transport uses tokio process pipes; JSON-RPC framing reads line-delimited messages from child stdout. SSE transport accepts a url but is a placeholder (matches TS behavior). Pending RPC requests use oneshot channels with a 30s timeout. mcp::serve dynamically registers a Rust handler + HTTP trigger and tracks the FunctionRef/Trigger refs so mcp::unserve can revoke them.
Six functions plus HTTP triggers preserved exactly: orchestrator::plan,
execute, status, intervene, workspace_write, workspace_read. Plan/Run
state machine routed through state::{get,set,list}; reactions persisted
to lifecycle_reactions; resume re-fires task::spawn_workers in the
background. workspace_write/read sanitize planId/key with the same
allowlist as the TS sanitizeId helper.
rohitg00
force-pushed
the
feat/port-skillkit-browser-code-mcp-orchestrator
branch
from
7e9571e to
5c62ba7
Compare
| let blocked = ["localhost", "127.0.0.1", "0.0.0.0", "169.254.169.254", "metadata.google.internal"]; | ||
| if blocked.contains(&host) { | ||
| return Err(IIIError::Handler(format!("blocked host: {host}"))); | ||
| } |
There was a problem hiding this comment.
🟡 SSRF check only blocks 169.254.169.254 instead of entire 169.254.0.0/16 link-local range
The Rust assert_no_ssrf at workers/browser/src/main.rs:80 only includes 169.254.169.254 in its blocked host list. The original TypeScript assertNoSsrf in packages/shared/src/utils.ts blocks the entire 169.254.0.0/16 range via /^169\.254\./. Other link-local addresses like 169.254.1.1 would bypass the Rust SSRF check. While 169.254.169.254 is the most critical (cloud metadata endpoint), the full range is reserved and should be blocked.
| let blocked = ["localhost", "127.0.0.1", "0.0.0.0", "169.254.169.254", "metadata.google.internal"]; | |
| if blocked.contains(&host) { | |
| return Err(IIIError::Handler(format!("blocked host: {host}"))); | |
| } | |
| let blocked = ["localhost", "127.0.0.1", "0.0.0.0", "metadata.google.internal"]; | |
| if blocked.contains(&host) { | |
| return Err(IIIError::Handler(format!("blocked host: {host}"))); | |
| } | |
| if host.starts_with("169.254.") { |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Cargo.toml (1)
3-45:⚠️ Potential issue | 🟠 MajorRemove orphaned imports from
src/index.ts.Lines 22-25 of
src/index.tsstill importsecurity-headers.js,security-map.js,security-zeroize.js, andskill-security.js, but these corresponding workspace members have been removed from Cargo.toml. Remove these orphaned imports to prevent runtime failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Cargo.toml` around lines 3 - 45, The src/index.ts file has orphaned imports for security-headers.js, security-map.js, security-zeroize.js, and skill-security.js (the import statements around lines 22-25) that correspond to workspace members removed from Cargo.toml; remove those import lines and any exported symbols that depend solely on them (or replace usages with existing alternatives) so the module no longer imports or re-exports security-headers.js, security-map.js, security-zeroize.js, or skill-security.js and builds without runtime import errors.
🧹 Nitpick comments (2)
workers/mcp-client/src/main.rs (2)
303-306: Dead code: pending iteration does nothing.This loop iterates over
conn.pendingbut only callskv.key()and discards the result. It appears to be incomplete cleanup logic. Theconn.pending.clear()on line 306 already removes all entries, making this loop unnecessary.Proposed fix
- for kv in conn.pending.iter() { - let _ = kv.key(); - } conn.pending.clear();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/mcp-client/src/main.rs` around lines 303 - 306, The loop iterating over conn.pending is dead code because it only calls kv.key() and discards the result, and conn.pending.clear() already removes entries; remove the for loop entirely (the block referencing kv.key()) to avoid unnecessary work, or if side-effects were intended, replace the loop with an explicit intended operation (e.g., logging or dropping resources) on each Pending entry before calling conn.pending.clear(); locate the loop using conn.pending, kv.key(), and conn.pending.clear() to update accordingly.
60-65: Consider using an allowlist for command validation instead of the current blocklist approach.The current
validate_commandfunction blocks specific characters, but a more robust pattern would be to validate that the command is an expected executable name or path. The codebase already uses this approach inworkers/approval-tiers/src/main.rswithsafe_shell_commands()—consider adopting a similar allowlist pattern here.While
tokio::process::Commandsafely avoids shell invocation (making the blocked characters technically irrelevant), an explicit allowlist provides clearer security intent and prevents execution of arbitrary user-provided binaries.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workers/mcp-client/src/main.rs` around lines 60 - 65, The current validate_command function uses a blocklist of characters; replace it with an allowlist check that validates the input command against known-safe executables/paths (mirror the safe_shell_commands() pattern used in workers/approval-tiers/src/main.rs) by looking up the command in that allowlist (or verifying it is an allowed absolute/relative path) and returning Err(IIIError::Handler(...)) if not allowed; update the validate_command function to perform this explicit allowlist membership test and add/update any helper (e.g., safe_shell_commands or a new allowed_commands set) used to source the permitted command names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Cargo.toml`:
- Around line 3-45: The src/index.ts file has orphaned imports for
security-headers.js, security-map.js, security-zeroize.js, and skill-security.js
(the import statements around lines 22-25) that correspond to workspace members
removed from Cargo.toml; remove those import lines and any exported symbols that
depend solely on them (or replace usages with existing alternatives) so the
module no longer imports or re-exports security-headers.js, security-map.js,
security-zeroize.js, or skill-security.js and builds without runtime import
errors.
---
Nitpick comments:
In `@workers/mcp-client/src/main.rs`:
- Around line 303-306: The loop iterating over conn.pending is dead code because
it only calls kv.key() and discards the result, and conn.pending.clear() already
removes entries; remove the for loop entirely (the block referencing kv.key())
to avoid unnecessary work, or if side-effects were intended, replace the loop
with an explicit intended operation (e.g., logging or dropping resources) on
each Pending entry before calling conn.pending.clear(); locate the loop using
conn.pending, kv.key(), and conn.pending.clear() to update accordingly.
- Around line 60-65: The current validate_command function uses a blocklist of
characters; replace it with an allowlist check that validates the input command
against known-safe executables/paths (mirror the safe_shell_commands() pattern
used in workers/approval-tiers/src/main.rs) by looking up the command in that
allowlist (or verifying it is an allowed absolute/relative path) and returning
Err(IIIError::Handler(...)) if not allowed; update the validate_command function
to perform this explicit allowlist membership test and add/update any helper
(e.g., safe_shell_commands or a new allowed_commands set) used to source the
permitted command names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 920ee48a-f45a-47ec-96f4-41f1c26b6959
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
Cargo.tomlsrc/__tests__/browser.test.tssrc/__tests__/code-agent.test.tssrc/__tests__/mcp-client.test.tssrc/__tests__/orchestrator.test.tssrc/__tests__/skillkit-bridge.test.tssrc/browser.tssrc/code-agent.tssrc/index.tssrc/mcp-client.tssrc/orchestrator.tssrc/skillkit-bridge.tsworkers/browser/Cargo.tomlworkers/browser/iii.worker.yamlworkers/browser/src/main.rsworkers/browser/src/types.rsworkers/code-agent/Cargo.tomlworkers/code-agent/iii.worker.yamlworkers/code-agent/src/main.rsworkers/mcp-client/Cargo.tomlworkers/mcp-client/iii.worker.yamlworkers/mcp-client/src/main.rsworkers/orchestrator/Cargo.tomlworkers/orchestrator/iii.worker.yamlworkers/orchestrator/src/main.rsworkers/skillkit-bridge/Cargo.tomlworkers/skillkit-bridge/iii.worker.yamlworkers/skillkit-bridge/src/main.rs
💤 Files with no reviewable changes (10)
- src/tests/code-agent.test.ts
- src/skillkit-bridge.ts
- src/mcp-client.ts
- src/tests/browser.test.ts
- src/tests/orchestrator.test.ts
- src/tests/mcp-client.test.ts
- src/code-agent.ts
- src/browser.ts
- src/orchestrator.ts
- src/tests/skillkit-bridge.test.ts
✅ Files skipped from review due to trivial changes (10)
- workers/mcp-client/iii.worker.yaml
- workers/code-agent/iii.worker.yaml
- workers/browser/iii.worker.yaml
- workers/skillkit-bridge/iii.worker.yaml
- workers/mcp-client/Cargo.toml
- workers/skillkit-bridge/Cargo.toml
- workers/orchestrator/iii.worker.yaml
- workers/code-agent/Cargo.toml
- workers/browser/src/types.rs
- workers/skillkit-bridge/src/main.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- workers/browser/Cargo.toml
- workers/orchestrator/Cargo.toml
- src/index.ts
…/session/streaming) Addresses 32 CodeRabbit findings across 11 workers from PRs #56 (wrappers) and #59 (context/session/streaming). Re-registers the affected workers in the workspace so they actually compile under cargo check/test. PR #56 — wrappers - browser: tighten SSRF to IP-classification (loopback/private/link-local/ unique-local + IPv4-mapped IPv6); revalidate post-navigation URLs in navigate/click/read/screenshot/type to close the redirect bypass; spawn + kill_on_drop the python3 bridge so timeouts don't leak; add missing HTTP triggers for browser_click and browser_type; reserve session slots via atomic state::update so two concurrent create_session calls cannot exceed MAX_SESSIONS or clobber the index. - code-agent: install rquickjs interrupt handler against a shared deadline + AtomicBool so wall-clock timeouts actually stop runaway JS instead of just dropping the join handle; replace String::truncate with a UTF-8 char- boundary safe helper. - skillkit-bridge: switch to spawn + kill_on_drop so timed-out npx skillkit children are reaped; canonicalize scan_path against WORKSPACE_ROOT and reject absolute / `..` paths; use char-boundary safe truncation. - mcp-client: drain piped stderr with a logging task so chatty servers can't fill the pipe; reject non-stdio transports up front and only insert into state.connections after the initialize + tools/list handshake; remove dead pending iter loop. - orchestrator: propagate task::spawn_workers errors and roll plan/run state back on failure (execute + resume); replace silent sanitize_id with a validate_id that rejects invalid characters / oversized / empty ids, matching task-decomposer parity. PR #59 — context/session/streaming - context-cache: only count cleared on successful state::delete; sanitize the agentId on stats lookup so it matches the recording key; expand sanitize_id to allow `.` and cap at 256 chars. - context-manager: report the post-scale effective budget (not the original total) when maxTokensPerHour clamps allocations; never duplicate the first system message when keep_last_n covers the whole conversation; account for per-message overhead while picking tail messages and truncate the preserved system prompt so trim never returns above maxTokens; re-truncate the assembled system_message in build_prompt so headers + skills + memories stay within the total budget. - context-monitor: insert synthetic tool-result stubs in original message order (right after the originating assistant message); run sanitize_tool_pairs before the under-budget early return; register the HTTP trigger for context::stats and unwrap payload_body inside it; clamp future timestamps in score_relevance_decay so the score cannot exceed 25. - session-lifecycle: include classification in auto_recover payload (default "wake-up"); allow add_reaction to omit agentId so global-scope reactions match what transition() already evaluates. - session-replay: propagate state::update failures instead of falling back to now_ms (which breaks atomic ordering and causes key collisions); register the missing HTTP trigger for replay::record. - streaming: track the byte offset of the opening fence and search for the closing fence after it so chunk_markdown_aware can no longer split between the opening ``` and the next character. Skipped: - stream::sse incremental flushing — requires a chunked HTTP response API the iii-sdk function-trigger model does not currently expose; called out as a follow-up rather than a wave-2 fix.
1 participant
Summary
Five wrapper/glue TS workers ported to Rust narrow workers under
workers/. Each preserves function_ids and HTTP routes exactly so callers see no behavioral change.tokio::process::Commandspawnsnpx skillkit ... --json. Safe-env allowlist + skill-id/agent-name validators match TS.agent::code_detectis aregexcrate port;agent::code_executeuses rquickjs (QuickJS bindings) with a 64MB memory cap, capturedconsole.log, and disabledeval/Function/setTimeout/process/require. Wall-clock timeout enforced via tokio.mcp::serve_handler,mcp::serve,mcp::unserve, and themcp_<server>_<tool>namespacing — agentos-specific glue not iniii-hq/workers/mcp-client. stdio transport via tokio process pipes; line-delimited JSON-RPC framing;oneshotchannels for pending requests. SSE matches TS placeholder behavior.state::{get,set,list}; resume re-firestask::spawn_workersin the background.Per worker:
src/<name>.ts+ matching__tests__/<name>.test.tsimport "./<name>.js"fromsrc/index.ts"workers/<name>"to root Cargo workspace membersTest plan
cargo checkclean for each new workercargo clippy -- -D warningsclean for all 5 new workerscargo test --workspace --releasepassesnpx vitest runpasses (1216 tests across 48 files)cargo test -p agentos-code-agentand-p agentos-orchestratorcover regex + sanitizer helpersSummary by CodeRabbit
Refactor
Tests