Add Sandboxie-backed LLM tool sandbox#356
Merged
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 15 minutes and 14 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR introduces sandboxed tool execution using Sandboxie Plus: tools like bash and file operations can now run inside OS-level isolated "portable boxes" per chat. Configuration, data contracts, authorization logic, box/process management, Redis-based task queueing, and startup integration are all implemented. ChangesSandboxie LLM Sandbox Infrastructure
Sequence DiagramsequenceDiagram
participant Client as Tool Caller
participant GeneralApp as GeneralBootstrap
participant SandboxService as SandboxieToolHostService
participant Redis as Redis
participant ToolHost as SandboxToolHost Process
participant Consumer as SandboxToolConsumer
Client->>GeneralApp: call sandboxed tool (with __chatId, __userId, __messageId)
GeneralApp->>SandboxService: ExecuteToolAsync(toolName, args, chatId, userId, messageId)
SandboxService->>SandboxService: EnsureToolHostAsync(chatId) - build box, start process
SandboxService->>Redis: enqueue SandboxToolTask
SandboxService->>Redis: poll SandboxToolResult (with timeout)
ToolHost->>Consumer: alive, waiting for tasks
Consumer->>Redis: BRPOP SandboxToolQueue(chatId)
Consumer->>Consumer: ExecuteToolAsync - build ToolContext(IsSandboxed=true, chatId, userId, messageId)
Consumer->>Redis: write SandboxToolResult with TTL
SandboxService->>Redis: receive result
SandboxService->>Client: return tool output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
PR Check ReportSummary
Test Results
Code Quality
Test Artifacts
LinksThis report is auto-generated by GitHub Actions |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
TelegramSearchBot/Service/AI/LLM/SandboxieToolHostService.cs (1)
231-266: ⚖️ Poor tradeoffPID tracking may be unreliable with Sandboxie Start.exe.
Sandboxie's
Start.exelaunches the target process inside the sandbox and may exit quickly or return a different PID than the actual sandboxed process. This means_toolHostProcessesstores the launcher PID, andIsProcessAlivemay returnfalseeven when the sandboxed tool host is still running—causing unnecessary process restarts.Consider tracking liveness via the Redis queue activity (e.g., heartbeat key) or using Sandboxie's management APIs/CLI to query active processes in the box.
🤖 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 `@TelegramSearchBot/Service/AI/LLM/SandboxieToolHostService.cs` around lines 231 - 266, StartToolHost currently stores the PID returned by Sandboxie Start.exe into _toolHostProcesses and IsProcessAlive checks that PID, but Sandboxie Start.exe often exits or returns a launcher PID rather than the sandboxed tool PID; change the liveness strategy: stop relying on Process.Start PID from StartToolHost and IsProcessAlive, and instead detect sandboxed tool liveness via an external signal — e.g., have the sandboxed process push periodic heartbeats to Redis (or a scheduler queue) and check that heartbeat key TTL in place of IsProcessAlive, or query Sandboxie management/CLI to enumerate processes in the named box (using instance.BoxName) to determine if the tool host is running; update uses of _toolHostProcesses and IsProcessAlive to reference the new heartbeat/box-query mechanism and remove dependence on the Start.exe PID.
🤖 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 `@TelegramSearchBot.Common/Env.cs`:
- Line 75: Env currently assigns SandboxieToolTimeoutSeconds from config without
validation; ensure you validate its bounds in the Env loader (where
SandboxieToolTimeoutSeconds is set) by checking the value read from config
(e.g., config.SandboxieToolTimeoutSeconds) and handling zero/negative or
unreasonably large values: either clamp to a safe default minimum (e.g., >0) or
throw a descriptive exception/log and replace with a default constant. Apply the
same validation logic to the other occurrences noted (lines where
SandboxieToolTimeoutSeconds is assigned) so the Env class always stores a
validated, non-zero positive timeout.
- Around line 64-65: SandboxieStartExe and SandboxieIniPath are assigned
directly from config and don't guard against whitespace-only values; update the
Env initialization so these properties use the same null/whitespace fallback
logic as the nearby fields (e.g., call Trim() then check
string.IsNullOrWhiteSpace on config.SandboxieStartExe and
config.SandboxieIniPath) and assign the existing default when the trimmed value
is null/empty/whitespace; reference the SandboxieStartExe and SandboxieIniPath
assignments in Env.cs and apply the same pattern used for other config fields in
that file.
In `@TelegramSearchBot.LLM/Service/Tools/BashToolService.cs`:
- Around line 90-93: The class BashToolService and its [BuiltInTool] metadata
still claim "admin-only" despite the authorization in the toolContext check (see
BashToolService and the conditional that allows sandboxed hosts:
toolContext.UserId != Env.AdminId && !toolContext.IsSandboxed); update the class
summary/docstring and the [BuiltInTool] description to say that command
execution is allowed for admin users or sandboxed tool hosts (or similar
wording), so the tool metadata matches the actual authorization behavior. Ensure
any user-facing description strings or attributes associated with
BashToolService are updated consistently.
In `@TelegramSearchBot/AppBootstrap/GeneralBootstrap.cs`:
- Around line 273-295: RegisterSandboxieTools currently silently leaves
chatId/userId/messageId as 0 when parsing fails and calls
sandboxService.ExecuteToolAsync, which leads to opaque "missing ChatId" errors;
update the lambda passed to McpToolHelper.RegisterProxyTools (the async
(toolName, arguments) => { ... }) to validate that parsed __chatId (and
optionally __userId and __messageId) are > 0 before calling
SandboxieToolHostService.ExecuteToolAsync, and if validation fails return a
clear error result (e.g., an error/exception describing that required context
keys like "__chatId" are missing or invalid) so callers receive a descriptive
failure instead of the generic runtime error.
---
Nitpick comments:
In `@TelegramSearchBot/Service/AI/LLM/SandboxieToolHostService.cs`:
- Around line 231-266: StartToolHost currently stores the PID returned by
Sandboxie Start.exe into _toolHostProcesses and IsProcessAlive checks that PID,
but Sandboxie Start.exe often exits or returns a launcher PID rather than the
sandboxed tool PID; change the liveness strategy: stop relying on Process.Start
PID from StartToolHost and IsProcessAlive, and instead detect sandboxed tool
liveness via an external signal — e.g., have the sandboxed process push periodic
heartbeats to Redis (or a scheduler queue) and check that heartbeat key TTL in
place of IsProcessAlive, or query Sandboxie management/CLI to enumerate
processes in the named box (using instance.BoxName) to determine if the tool
host is running; update uses of _toolHostProcesses and IsProcessAlive to
reference the new heartbeat/box-query mechanism and remove dependence on the
Start.exe PID.
🪄 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
Run ID: a74625b0-f6fb-4ad3-9b03-b29608302b32
📒 Files selected for processing (12)
README.mdTelegramSearchBot.Common/Env.csTelegramSearchBot.Common/Model/AI/LlmAgentContracts.csTelegramSearchBot.Common/Model/ToolContext.csTelegramSearchBot.LLM/Service/AI/LLM/McpToolHelper.csTelegramSearchBot.LLM/Service/Tools/BashToolService.csTelegramSearchBot.LLM/Service/Tools/FileToolService.csTelegramSearchBot.LLMAgent/LLMAgentProgram.csTelegramSearchBot.LLMAgent/Service/SandboxToolConsumer.csTelegramSearchBot/AppBootstrap/GeneralBootstrap.csTelegramSearchBot/AppBootstrap/SandboxToolHostBootstrap.csTelegramSearchBot/Service/AI/LLM/SandboxieToolHostService.cs
ModerRAS
force-pushed
the
feature/llm-sandboxie-toolhost
branch
7 times, most recently
from
82e42d2 to
1e86fb4
Compare
ModerRAS
force-pushed
the
feature/llm-sandboxie-toolhost
branch
from
1e86fb4 to
be4f850
Compare
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
Validation
Summary by CodeRabbit
Release Notes
New Features
Documentation