Propose dont apply#37
Merged
Merged
Conversation
… the library facade
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
📝 WalkthroughWalkthroughThis PR replaces the string-based Message/tool-call model with typed ContentBlocks, adds PermissionGate and TurnContext for cancellation and tool-call approval threaded through the agent loop and tool executors, introduces ACP session modes/permission-request/cancel flows, splits conversation history into metadata+append-only logs, sandboxes the /ws filesystem sidecar, adds a bare /acp endpoint, and adds a TUI permission prompt. ChangesCore Model, Permission/Turn Context, and LLM Providers
Estimated code review effort: 5 (Critical) | ~150 minutes ACP Protocol, Session, and Client Library
Estimated code review effort: 5 (Critical) | ~120 minutes History Durability and Config Defaults
Estimated code review effort: 3 (Moderate) | ~30 minutes WebSocket Transport Sandboxing
Estimated code review effort: 4 (Complex) | ~60 minutes TUI Permission Prompt
Estimated code review effort: 3 (Moderate) | ~30 minutes Documentation and Changelog
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AgentState
participant PromptLock
participant AgentLoop as run_agent_streaming_with_history
participant PermissionGate
participant History as HistoryManager
Client->>AgentState: session/prompt
AgentState->>PromptLock: try_acquire_prompt_lock
AgentState->>History: persist_conversation (pre-turn)
AgentState->>AgentLoop: run(TurnContext)
AgentLoop->>PermissionGate: check(tool_call)
PermissionGate-->>AgentLoop: decision
AgentLoop-->>AgentState: StreamEvent updates
AgentState->>History: persist_conversation (final)
AgentState-->>Client: PromptResponse(stop_reason)
sequenceDiagram
participant WsClient as /ws Client
participant FsState
participant Disk as Filesystem
WsClient->>FsState: FsRequest{path}
FsState->>FsState: validate(path, work_dir)
alt inside work_dir
FsState->>Disk: perform operation
Disk-->>FsState: result
FsState-->>WsClient: FsResponse::Success
else outside work_dir
FsState-->>WsClient: FsResponse::Error
end
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/tools/delegate.rs (1)
51-70: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSubagent I/O still bypasses ACP
client_io
DelegateTool::build_executorhardcodesArc::new(NoClientIo), so subagents always read and write local disk even when the parent turn is using ACP-backedclient_io. That can miss unsaved editor buffers or write outside the editor’s authoritative filesystem view. Threadclient_iothroughDelegateTool(orTurnContext) and pass it toSandboxedExecutorinstead ofNoClientIo.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools/delegate.rs` around lines 51 - 70, DelegateTool::build_executor is still hardcoding NoClientIo, so subagents bypass the parent ACP client_io and operate on local disk instead of the editor-backed filesystem. Thread the existing client_io through DelegateTool (or TurnContext) by storing it in DelegateTool::new and passing it along when constructing SandboxedExecutor, replacing the NoClientIo fallback so subagent I/O uses the same authoritative client_io as the parent turn.src/tools/sandboxed_executor.rs (1)
69-119: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winArchitect mode still bypasses the allowlist for built-in tools
src/tools/sandboxed_executor.rs:69-116interceptswrite_file/execute_commandbeforeScopedExecutorcan reject them, andsrc/core/agent.rsdoes not re-checkToolUse.nameagainst the offered tool list before dispatch. In architect mode, a hallucinated or injectedwrite_filecall still runs;execute_commandis affected too when shell is enabled.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tools/sandboxed_executor.rs` around lines 69 - 119, The sandboxed built-in tool handling is bypassing the allowed tool list, so a hallucinated `write_file` or `execute_command` can still run. Update `SandboxedExecutor::execute` to reject built-in names unless they were explicitly offered, and add a pre-dispatch validation in `Agent` before calling the executor. Use the existing `ToolUse.name`, `ScopedExecutor`, and `SandboxedExecutor::execute` flow to enforce the allowlist consistently for built-ins and fall back only for permitted tools.
🧹 Nitpick comments (2)
src/transport/ws.rs (2)
248-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winErrors are swallowed into generic 500s without logging.
Both handlers collapse
spawn_blockingjoin failures and underlying history errors into a bare"failed to load ..."500 via_ =>, discarding the actual error. Logging it (e.g.,tracing::warn!) before returning would materially help diagnosing production issues without changing the response contract.Also applies to: 262-290
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/transport/ws.rs` around lines 248 - 260, The sessions_handler and the related history-loading handler currently map all spawn_blocking and history failures to a generic 500 via the _ match arm, which hides the real cause. Update the match around tokio::task::spawn_blocking in sessions_handler (and the sibling handler in the same ws transport module) to log both the join error and the inner history error with tracing before returning the existing JSON error response. Keep the response contract unchanged, but include enough context in the log to identify which operation failed.
299-374: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate ACP-bridge wiring between
/wsand/acp.
handle_socketandhandle_acp_socketboth build theacp_out_tx/acp_in_txchannels, wrapacp_out_txin a sink, andtokio::spawn(acp::serve(Lines::new(...), state))identically. Consider extracting this into a shared helper (returning the two ends the caller-specific outbound loop needs) to avoid the two copies drifting apart.fn spawn_acp_bridge( state: Arc<AgentState>, ) -> (mpsc::UnboundedReceiver<String>, mpsc::UnboundedSender<std::io::Result<String>>) { let (acp_out_tx, acp_out_rx) = mpsc::unbounded::<String>(); let (acp_in_tx, acp_in_rx) = mpsc::unbounded::<std::io::Result<String>>(); let acp_sink = acp_out_tx .sink_map_err(|e| std::io::Error::new(std::io::ErrorKind::BrokenPipe, e.to_string())); tokio::spawn(acp::serve(Lines::new(acp_sink, acp_in_rx), state)); (acp_out_rx, acp_in_tx) }Also applies to: 376-414
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/transport/ws.rs` around lines 299 - 374, `handle_socket` duplicates the ACP bridge setup that also exists in `handle_acp_socket`, so extract the shared channel/sink/spawn logic into a helper like `spawn_acp_bridge` and reuse it from both paths. Move the `acp_out_tx/acp_in_tx` creation, `sink_map_err` wrapping, and `tokio::spawn(acp::serve(Lines::new(...), state))` into that helper, returning the receiver and sender each caller needs. Keep the caller-specific outbound loops in `handle_socket` and `handle_acp_socket` unchanged except for wiring them to the helper’s վերադարձed ends.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/acp/mod.rs`:
- Around line 63-66: The `Architect` variant’s doc comment is incorrect because
`run_agent_loop` still routes `read_file` through
`turn.permission_gate.check(...)`, and `AcpPermissionGate` can emit
`session/request_permission` unless the decision is already cached. Update the
comment near `Architect` in `mod.rs` to say it only limits the tool list to
`read_file` and does not guarantee no permission prompt, or change
`run_agent_loop`/`AcpPermissionGate` to explicitly bypass the gate for read-only
tools if that is the intended behavior.
In `@src/core/agent.rs`:
- Around line 159-185: The permission approval path in agent::turn is still
blocking independently of cancellation, so a pending turn can remain stuck in
turn.permission_gate.check while waiting for user approval. Update the tool-call
loop in Agent::run/handle_turn to race the permission_gate.check future against
turn.cancel (or otherwise abort the approval wait on cancel), and ensure any
pending approval flow is released when cancellation wins so prompt_lock is not
held until the gate resolves.
In `@src/tools/sandboxed_executor.rs`:
- Around line 78-81: The file I/O path in SandboxedExecutor still waits on
client_io directly, so slow fs/read_text_file and fs/write_text_file requests
can block a turn. Update the relevant read_file/write_file handling in
SandboxedExecutor to race the client_io await against turn.cancel using
tokio::select! (or an equivalent timeout), and return early on cancellation so
session/cancel can interrupt unresponsive clients.
---
Outside diff comments:
In `@src/tools/delegate.rs`:
- Around line 51-70: DelegateTool::build_executor is still hardcoding
NoClientIo, so subagents bypass the parent ACP client_io and operate on local
disk instead of the editor-backed filesystem. Thread the existing client_io
through DelegateTool (or TurnContext) by storing it in DelegateTool::new and
passing it along when constructing SandboxedExecutor, replacing the NoClientIo
fallback so subagent I/O uses the same authoritative client_io as the parent
turn.
In `@src/tools/sandboxed_executor.rs`:
- Around line 69-119: The sandboxed built-in tool handling is bypassing the
allowed tool list, so a hallucinated `write_file` or `execute_command` can still
run. Update `SandboxedExecutor::execute` to reject built-in names unless they
were explicitly offered, and add a pre-dispatch validation in `Agent` before
calling the executor. Use the existing `ToolUse.name`, `ScopedExecutor`, and
`SandboxedExecutor::execute` flow to enforce the allowlist consistently for
built-ins and fall back only for permitted tools.
---
Nitpick comments:
In `@src/transport/ws.rs`:
- Around line 248-260: The sessions_handler and the related history-loading
handler currently map all spawn_blocking and history failures to a generic 500
via the _ match arm, which hides the real cause. Update the match around
tokio::task::spawn_blocking in sessions_handler (and the sibling handler in the
same ws transport module) to log both the join error and the inner history error
with tracing before returning the existing JSON error response. Keep the
response contract unchanged, but include enough context in the log to identify
which operation failed.
- Around line 299-374: `handle_socket` duplicates the ACP bridge setup that also
exists in `handle_acp_socket`, so extract the shared channel/sink/spawn logic
into a helper like `spawn_acp_bridge` and reuse it from both paths. Move the
`acp_out_tx/acp_in_tx` creation, `sink_map_err` wrapping, and
`tokio::spawn(acp::serve(Lines::new(...), state))` into that helper, returning
the receiver and sender each caller needs. Keep the caller-specific outbound
loops in `handle_socket` and `handle_acp_socket` unchanged except for wiring
them to the helper’s վերադարձed ends.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 82875e89-68be-4cf4-be21-c3e3f0ff43dd
📒 Files selected for processing (45)
CHANGELOG.mdREADME.mddocs/api.mddocs/architecture.mddocs/configuration.mddocs/custom-llm-provider.mddocs/custom-tools.mddocs/deployment.mddocs/library.mddocs/subagents.mdsrc/acp/mod.rssrc/acp/session.rssrc/client.rssrc/config/config.toml.defaultsrc/config/mod.rssrc/config/resolve.rssrc/config/types.rssrc/core/agent.rssrc/core/client_io.rssrc/core/llm/anthropic.rssrc/core/llm/gemini.rssrc/core/llm/mod.rssrc/core/llm/openai.rssrc/core/llm/retry.rssrc/core/mod.rssrc/core/models.rssrc/core/permission.rssrc/core/turn.rssrc/error.rssrc/lib.rssrc/rag/history.rssrc/rag/prompt.rssrc/rag/skills.rssrc/tools/delegate.rssrc/tools/mod.rssrc/tools/sandboxed_executor.rssrc/tools/scoped_executor.rssrc/transport/run.rssrc/transport/stdio.rssrc/transport/ws.rssrc/tui/app.rssrc/tui/mod.rssrc/tui/permission.rssrc/tui/render.rssrc/tui/types.rs
Comment on lines
+63
to
+66
| /// Read-only: only `read_file` is offered to the LLM, regardless of | ||
| /// permission decisions. No `session/request_permission` prompts occur | ||
| /// since nothing mutating is ever on the tool list. | ||
| Architect, |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm read_file is not special-cased/auto-allowed in the permission path.
rg -nP -C3 '\bread_file\b' src/acp/mod.rs src/core/agent.rs src/tools/ | rg -n -i 'permission|allow|check|approv|read_file'
# Show the gate.check call site to confirm all tool calls are gated.
rg -nP -C4 'permission_gate\.check' src/core/agent.rsRepository: weirdstuff-dev/openheim
Length of output: 6987
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Architect mode docs and the tool/permission flow around the relevant call sites.
sed -n '55,80p' src/acp/mod.rs
printf '\n---\n'
sed -n '160,205p' src/core/agent.rs
printf '\n---\n'
sed -n '730,780p' src/acp/mod.rs
printf '\n---\n'
rg -n -C3 'session/request_permission|request_permission|PermissionGate|check\(' src/acp/mod.rs src/core/agent.rs src -g '!target'Repository: weirdstuff-dev/openheim
Length of output: 29049
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the ACP permission gate implementation and the Architect tool list setup.
sed -n '640,730p' src/acp/mod.rs
printf '\n---\n'
sed -n '300,330p' src/acp/mod.rs
printf '\n---\n'
rg -n -C2 'Architect|read_file|session/request_permission|AllowAlways|RejectAlways' src/acp/mod.rs src/acp/session.rs src/core/permission.rsRepository: weirdstuff-dev/openheim
Length of output: 12479
Architect still prompts on read_file. run_agent_loop calls turn.permission_gate.check(...) for every tool call, and AcpPermissionGate sends session/request_permission unless the decision is already remembered. Since Architect only narrows the tool list to read_file, the “No session/request_permission prompts occur” line is inaccurate. Reword it, or bypass the gate for read-only tools if that’s the intended behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/acp/mod.rs` around lines 63 - 66, The `Architect` variant’s doc comment
is incorrect because `run_agent_loop` still routes `read_file` through
`turn.permission_gate.check(...)`, and `AcpPermissionGate` can emit
`session/request_permission` unless the decision is already cached. Update the
comment near `Architect` in `mod.rs` to say it only limits the tool list to
`read_file` and does not guarantee no permission prompt, or change
`run_agent_loop`/`AcpPermissionGate` to explicitly bypass the gate for read-only
tools if that is the intended behavior.
Comment on lines
+159
to
185
| for tool_call in &tool_calls { | ||
| if turn.cancel.is_cancelled() { | ||
| stop_reason = StopReason::Cancelled; | ||
| break; | ||
| } | ||
|
|
||
| let id = &tool_call.id; | ||
| let tool_name = &tool_call.name; | ||
| let arguments = &tool_call.arguments; | ||
|
|
||
| if let Some(cb) = callback.as_mut() { | ||
| cb(StreamEvent::ToolCall { | ||
| id: id.clone(), | ||
| tool_name: tool_name.clone(), | ||
| arguments: arguments.clone(), | ||
| }); | ||
| } | ||
|
|
||
| let (result, is_error) = match tool_executor.execute(tool_name, arguments).await { | ||
| Ok(r) => (r, false), | ||
| Err(e) => (format!("Error: {e}"), true), | ||
| let decision = turn.permission_gate.check(id, tool_name, arguments).await; | ||
| let (result, is_error) = if decision.is_allowed() { | ||
| match tool_executor.execute(tool_name, arguments, turn).await { | ||
| Ok(r) => (r, false), | ||
| Err(e) => (format!("Error: {e}"), true), | ||
| } | ||
| } else { | ||
| ("Permission denied by user.".to_string(), true) | ||
| }; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether PermissionGate implementations race against cancellation internally.
rg -n -A 20 'impl PermissionGate for' --type=rust
rg -n -B3 -A 15 'trait PermissionGate' --type=rustRepository: weirdstuff-dev/openheim
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- files mentioning permission_gate / PermissionGate ---'
rg -n --hidden --glob '!target' 'PermissionGate|permission_gate' .
echo '--- agent.rs around cited lines ---'
sed -n '130,230p' src/core/agent.rs
echo '--- acp mod around prompt_guard / cancellation ---'
sed -n '1,260p' src/acp/mod.rsRepository: weirdstuff-dev/openheim
Length of output: 21122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- core permission trait/impls ---'
sed -n '1,220p' src/core/permission.rs
echo '--- ACP permission gate ---'
sed -n '620,720p' src/acp/mod.rs
echo '--- TUI permission gate ---'
sed -n '1,220p' src/tui/permission.rs
echo '--- delegate/subagent permission plumbing ---'
sed -n '200,340p' src/tools/delegate.rsRepository: weirdstuff-dev/openheim
Length of output: 13604
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- acp_prompt / prompt_lock / cancel handling ---'
rg -n -A 30 -B 10 'acp_prompt|prompt_lock|prompt_guard|session/request_permission|cancel_session|CancelNotification|CancellationToken' src/acp/mod.rs src/acp/session.rsRepository: weirdstuff-dev/openheim
Length of output: 32200
Race the permission await with cancellation. turn.permission_gate.check(...) still waits independently of turn.cancel, and the concrete ACP/TUI gates don’t observe that token themselves. If session/cancel arrives while approval is pending, the turn can stay stuck holding prompt_lock until the gate returns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/agent.rs` around lines 159 - 185, The permission approval path in
agent::turn is still blocking independently of cancellation, so a pending turn
can remain stuck in turn.permission_gate.check while waiting for user approval.
Update the tool-call loop in Agent::run/handle_turn to race the
permission_gate.check future against turn.cancel (or otherwise abort the
approval wait on cancel), and ensure any pending approval flow is released when
cancellation wins so prompt_lock is not held until the gate resolves.
Comment on lines
+78
to
+81
| match self.client_io.read_file(&validated).await { | ||
| Some(result) => result, | ||
| None => read_file(&validated).await, | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== sandboxed_executor.rs ==\n'
nl -ba src/tools/sandboxed_executor.rs | sed -n '1,180p'
printf '\n== search: client_io + cancel/select ==\n'
rg -n "client_io\.read_file|client_io\.write_file|turn\.cancel|tokio::select!|cancel token|session/cancel|read_text_file|write_text_file" -S src CHANGELOG* . || true
printf '\n== relevant client_io definitions ==\n'
rg -n "trait .*ClientIo|struct .*ClientIo|read_file\(|write_file\(" -S src || trueRepository: weirdstuff-dev/openheim
Length of output: 230
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== sandboxed_executor.rs (1-180) ==\n'
awk 'NR>=1 && NR<=180 { printf "%5d %s\n", NR, $0 }' src/tools/sandboxed_executor.rs
printf '\n== search: client_io + cancel/select ==\n'
rg -n "client_io\.read_file|client_io\.write_file|turn\.cancel|tokio::select!|session/cancel|read_text_file|write_text_file|cancel token" -S src CHANGELOG* . || true
printf '\n== client_io definitions ==\n'
rg -n "trait .*ClientIo|struct .*ClientIo|read_file\(|write_file\(" -S src || trueRepository: weirdstuff-dev/openheim
Length of output: 21490
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== core/client_io.rs ==\n'
awk 'NR>=1 && NR<=120 { printf "%5d %s\n", NR, $0 }' src/core/client_io.rs
printf '\n== acp/mod.rs (AcpClientIo) ==\n'
awk 'NR>=720 && NR<=790 { printf "%5d %s\n", NR, $0 }' src/acp/mod.rs
printf '\n== acp/mod.rs nearby request handling ==\n'
awk 'NR>=930 && NR<=1085 { printf "%5d %s\n", NR, $0 }' src/acp/mod.rs
printf '\n== turn/cancel context ==\n'
awk 'NR>=1 && NR<=120 { printf "%5d %s\n", NR, $0 }' src/core/turn.rsRepository: weirdstuff-dev/openheim
Length of output: 15152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== core/agent.rs (tool execution + cancel) ==\n'
awk 'NR>=70 && NR<=260 { printf "%5d %s\n", NR, $0 }' src/core/agent.rs
printf '\n== search: block_task/send_request/cancel around ACP client I/O ==\n'
rg -n "block_task\(\)|send_request\(|CancellationToken|cancelled\(\)|tokio::select!|read_text_file|write_text_file" -S src/acp src/core src/tools || true
printf '\n== acp transport / request helper definitions ==\n'
rg -n "struct ConnectionTo|fn send_request|block_task\(" -S src || trueRepository: weirdstuff-dev/openheim
Length of output: 12805
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== block_task definitions/usages ==\n'
rg -n "block_task\(" -S src
printf '\n== send_request definition ==\n'
rg -n "fn send_request|struct .*Request|impl .*send_request" -S src
printf '\n== transport/run.rs ==\n'
awk 'NR>=1 && NR<=130 { printf "%5d %s\n", NR, $0 }' src/transport/run.rs
printf '\n== acp/mod.rs around request permission call ==\n'
awk 'NR>=670 && NR<=705 { printf "%5d %s\n", NR, $0 }' src/acp/mod.rsRepository: weirdstuff-dev/openheim
Length of output: 8095
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== block_task definitions/usages ==\n'
rg -n "block_task\(" -S src
printf '\n== send_request definition ==\n'
rg -n "fn send_request|struct .*Request|impl .*send_request" -S src
printf '\n== transport/run.rs ==\n'
awk 'NR>=1 && NR<=130 { printf "%5d %s\n", NR, $0 }' src/transport/run.rs
printf '\n== acp/mod.rs around request permission call ==\n'
awk 'NR>=670 && NR<=705 { printf "%5d %s\n", NR, $0 }' src/acp/mod.rsRepository: weirdstuff-dev/openheim
Length of output: 8095
Race ACP file I/O against turn.cancel
read_file and write_file await client_io directly here, so an fs/read_text_file / fs/write_text_file round trip can still block the turn until the client replies. Wrap these awaits in a tokio::select! with turn.cancel (or add a timeout) so session/cancel can interrupt a slow or unresponsive client.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tools/sandboxed_executor.rs` around lines 78 - 81, The file I/O path in
SandboxedExecutor still waits on client_io directly, so slow fs/read_text_file
and fs/write_text_file requests can block a turn. Update the relevant
read_file/write_file handling in SandboxedExecutor to race the client_io await
against turn.cancel using tokio::select! (or an equivalent timeout), and return
early on cancellation so session/cancel can interrupt unresponsive clients.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit