feat: Add PR review tools, job monitor, and channel injection for E2E sandbox workflows - #57
Conversation
… sandbox workflows Adds JobEventsTool and JobPromptTool so the main agent can read container event logs and send follow-up prompts to running Claude Code sessions. A background JobMonitor forwards container assistant messages into the agent loop via a new inject channel on ChannelManager. CreateJobTool now accepts a project_dir parameter for mounting existing cloned repos into containers, and spawns the monitor automatically for async jobs. Also: Dockerfile bumped to Rust 1.88 (rig-core needs let chains), GITHUB_TOKEN forwarded into containers for gh CLI auth, and truncate() fixed for multi-byte char boundary panics. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @ilblackdragon, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the agent's ability to interact with and monitor sandbox environments, particularly for E2E testing and Claude Code sessions. It introduces new tools for inspecting job events and sending dynamic prompts to running jobs, enabling more interactive and informed agent workflows. Additionally, it improves the sandbox setup by allowing existing project directories to be mounted and ensures proper authentication for GitHub operations within containers. A critical fix for multi-byte character handling in string truncation also improves robustness. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant new functionality for end-to-end sandbox workflows, including tools for PR review, a job monitor for asynchronous tasks, and a channel injection mechanism. A critical security audit identified host credential leakage into the sandbox environment and two instances of Broken Access Control (IDOR) in the new job management tools. These vulnerabilities must be addressed to ensure sandbox isolation and user data privacy. Additionally, consider optimizing the Dockerfile to reduce image size. The bug fix in the truncate function for multi-byte characters is a positive improvement.
| if let Ok(gh_token) = std::env::var("GITHUB_TOKEN") { | ||
| env_vec.push(format!("GITHUB_TOKEN={}", gh_token)); | ||
| env_vec.push(format!("GH_TOKEN={}", gh_token)); | ||
| } |
There was a problem hiding this comment.
The ContainerJobManager::create_job function forwards GITHUB_TOKEN and GH_TOKEN from the host into the sandboxed Docker container. This is a critical host credential leakage vulnerability, as it compromises the sandbox's isolation purpose. A malicious agent inside the container could exfiltrate these tokens, leading to unauthorized access. Avoid forwarding host credentials into the sandbox. If GitHub access is needed, use scoped credentials or a dedicated service account. Additionally, consider adding documentation advising users on the use of fine-grained personal access tokens with minimum required scopes.
There was a problem hiding this comment.
Acknowledged. This is intentional for the PR review workflow (containers need gh access), but the current approach of forwarding the host token is too broad. We are planning a redesign with scoped credentials for a follow-up change.
| async fn execute( | ||
| &self, | ||
| params: serde_json::Value, | ||
| _ctx: &JobContext, | ||
| ) -> Result<ToolOutput, ToolError> { | ||
| let start = std::time::Instant::now(); | ||
|
|
||
| let job_id_str = params | ||
| .get("job_id") | ||
| .and_then(|v| v.as_str()) | ||
| .ok_or_else(|| ToolError::InvalidParameters("missing 'job_id' parameter".into()))?; | ||
|
|
||
| let job_id = Uuid::parse_str(job_id_str).map_err(|_| { | ||
| ToolError::InvalidParameters(format!("invalid job ID format: {}", job_id_str)) | ||
| })?; | ||
|
|
||
| let content = params | ||
| .get("content") | ||
| .and_then(|v| v.as_str()) | ||
| .ok_or_else(|| ToolError::InvalidParameters("missing 'content' parameter".into()))?; | ||
|
|
||
| let done = params | ||
| .get("done") | ||
| .and_then(|v| v.as_bool()) | ||
| .unwrap_or(false); | ||
|
|
||
| let prompt = crate::orchestrator::api::PendingPrompt { | ||
| content: content.to_string(), | ||
| done, | ||
| }; | ||
|
|
||
| { | ||
| let mut queue = self.prompt_queue.lock().await; | ||
| queue.entry(job_id).or_default().push_back(prompt); | ||
| } |
There was a problem hiding this comment.
The JobPromptTool::execute function allows a user to queue a follow-up prompt for a sandbox job identified by its job_id. However, the tool does not verify if the authenticated user (_ctx.user_id) is the owner of the job. An attacker could potentially send prompts to active jobs belonging to other users if they can guess or obtain their job_id. Before queuing the prompt, verify that the job belongs to the current user by checking the user_id field in the job record.
There was a problem hiding this comment.
Fixed in 1850f44. JobPromptTool now takes a ContextManager ref and verifies ctx.user_id matches the job owner before queuing. Added test_job_prompt_tool_rejects_other_users_job test.
| async fn execute( | ||
| &self, | ||
| params: serde_json::Value, | ||
| _ctx: &JobContext, | ||
| ) -> Result<ToolOutput, ToolError> { | ||
| let start = std::time::Instant::now(); | ||
|
|
||
| let job_id_str = params | ||
| .get("job_id") | ||
| .and_then(|v| v.as_str()) | ||
| .ok_or_else(|| ToolError::InvalidParameters("missing 'job_id' parameter".into()))?; | ||
|
|
||
| let job_id = Uuid::parse_str(job_id_str).map_err(|_| { | ||
| ToolError::InvalidParameters(format!("invalid job ID format: {}", job_id_str)) | ||
| })?; | ||
|
|
||
| let limit = params.get("limit").and_then(|v| v.as_u64()).unwrap_or(50) as usize; | ||
|
|
||
| let events = | ||
| self.store.list_job_events(job_id).await.map_err(|e| { | ||
| ToolError::ExecutionFailed(format!("failed to load job events: {}", e)) | ||
| })?; |
There was a problem hiding this comment.
The JobEventsTool::execute function allows a user to read the event logs for a sandbox job identified by its job_id. However, it does not verify if the current user has permission to access the job's events. An attacker could read sensitive event logs (including messages, tool calls, and results) of other users' jobs if they can guess or obtain their job_id. Verify that the job belongs to the current user before returning the event logs by fetching the job record from the store and comparing its user_id with the requester's user_id.
There was a problem hiding this comment.
Fixed in 1850f44. JobEventsTool now takes a ContextManager ref and verifies ctx.user_id matches the job owner before returning events. Added test_job_events_tool_rejects_other_users_job test.
| RUN curl -fsSL https://cli.github.com/packages/githubcli-archive-keyring.gpg \ | ||
| | dd of=/usr/share/keyrings/githubcli-archive-keyring.gpg \ | ||
| && echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/githubcli-archive-keyring.gpg] https://cli.github.com/packages stable main" \ | ||
| > /etc/apt/sources.list.d/github-cli.list \ | ||
| && apt-get update && apt-get install -y --no-install-recommends gh \ | ||
| && rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
To optimize the Docker image, you could combine this RUN instruction with the package installation at line 25. By adding the GitHub CLI repository first, you can then use a single RUN command to perform apt-get update once and install all packages, including gh. This would reduce the final image size by removing a layer and an extra apt-get update command.
There was a problem hiding this comment.
Fixed in 1850f44. Combined the gh CLI apt repo setup and all package installs into a single RUN layer, eliminating the extra apt-get update.
There was a problem hiding this comment.
Pull request overview
This PR adds infrastructure for the main agent to monitor and interact with running sandbox jobs, particularly Claude Code sessions. It introduces two new tools (JobEventsTool and JobPromptTool) that allow the main agent to read container event logs and send follow-up instructions to running jobs. A new JobMonitor component forwards assistant messages from containers into the agent loop via an injection channel on ChannelManager.
Changes:
- Added JobEventsTool and JobPromptTool for reading job events and sending prompts to running containers
- Implemented JobMonitor to forward Claude Code output to the main agent via injection channel
- Added inject channel to ChannelManager for background tasks to push messages into agent loop
- Enhanced CreateJobTool with project_dir parameter for mounting existing repos into containers
- Fixed truncate() to handle multi-byte UTF-8 characters safely
- Updated Dockerfile to forward GITHUB_TOKEN to containers and install gh CLI
- Bumped Rust version references in Dockerfile
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/worker/runtime.rs | Fixed truncate() to avoid panics on multi-byte character boundaries |
| src/tools/registry.rs | Added registration for new JobEventsTool and JobPromptTool with conditional dependencies |
| src/tools/builtin/mod.rs | Exported new job tools and PromptQueue type |
| src/tools/builtin/job.rs | Added JobEventsTool, JobPromptTool, project_dir parameter, and job monitor spawning |
| src/orchestrator/job_manager.rs | Forwarded GITHUB_TOKEN environment variable to containers |
| src/main.rs | Wired up inject channel and prompt queue dependencies |
| src/channels/manager.rs | Added injection channel for background tasks to push messages into agent stream |
| src/agent/mod.rs | Registered new job_monitor module |
| src/agent/job_monitor.rs | Implemented background monitor to forward container events to agent loop |
| Dockerfile.worker | Updated Rust version, added gh CLI installation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub struct JobEventsTool { | ||
| store: Arc<Store>, | ||
| } | ||
|
|
||
| impl JobEventsTool { | ||
| pub fn new(store: Arc<Store>) -> Self { | ||
| Self { store } | ||
| } | ||
| } | ||
|
|
||
| #[async_trait] | ||
| impl Tool for JobEventsTool { | ||
| fn name(&self) -> &str { | ||
| "job_events" | ||
| } | ||
|
|
||
| fn description(&self) -> &str { | ||
| "Read the event log for a sandbox job. Shows messages, tool calls, results, \ | ||
| and status changes from the container. Use this to check what Claude Code \ | ||
| or a worker sub-agent has been doing." | ||
| } | ||
|
|
||
| fn parameters_schema(&self) -> serde_json::Value { | ||
| serde_json::json!({ | ||
| "type": "object", | ||
| "properties": { | ||
| "job_id": { | ||
| "type": "string", | ||
| "description": "The UUID of the sandbox job" | ||
| }, | ||
| "limit": { | ||
| "type": "integer", | ||
| "description": "Maximum number of events to return (default 50, most recent)" | ||
| } | ||
| }, | ||
| "required": ["job_id"] | ||
| }) | ||
| } | ||
|
|
||
| async fn execute( | ||
| &self, | ||
| params: serde_json::Value, | ||
| _ctx: &JobContext, | ||
| ) -> Result<ToolOutput, ToolError> { | ||
| let start = std::time::Instant::now(); | ||
|
|
||
| let job_id_str = params | ||
| .get("job_id") | ||
| .and_then(|v| v.as_str()) | ||
| .ok_or_else(|| ToolError::InvalidParameters("missing 'job_id' parameter".into()))?; | ||
|
|
||
| let job_id = Uuid::parse_str(job_id_str).map_err(|_| { | ||
| ToolError::InvalidParameters(format!("invalid job ID format: {}", job_id_str)) | ||
| })?; | ||
|
|
||
| let limit = params.get("limit").and_then(|v| v.as_u64()).unwrap_or(50) as usize; | ||
|
|
||
| let events = | ||
| self.store.list_job_events(job_id).await.map_err(|e| { | ||
| ToolError::ExecutionFailed(format!("failed to load job events: {}", e)) | ||
| })?; | ||
|
|
||
| // Take the last `limit` events (most recent) | ||
| let start_idx = events.len().saturating_sub(limit); | ||
| let recent: Vec<serde_json::Value> = events[start_idx..] | ||
| .iter() | ||
| .map(|ev| { | ||
| serde_json::json!({ | ||
| "event_type": ev.event_type, | ||
| "data": ev.data, | ||
| "created_at": ev.created_at.to_rfc3339(), | ||
| }) | ||
| }) | ||
| .collect(); | ||
|
|
||
| let result = serde_json::json!({ | ||
| "job_id": job_id_str, | ||
| "total_events": events.len(), | ||
| "returned": recent.len(), | ||
| "events": recent, | ||
| }); | ||
|
|
||
| Ok(ToolOutput::success(result, start.elapsed())) | ||
| } | ||
|
|
||
| fn requires_sanitization(&self) -> bool { | ||
| true | ||
| } | ||
| } |
There was a problem hiding this comment.
The new JobEventsTool lacks test coverage. Other tools in this file (CreateJobTool, ListJobsTool, JobStatusTool, JobPromptTool) all have test coverage. Consider adding at least a basic test that verifies the tool can successfully retrieve events from the store, handles invalid job IDs appropriately, and respects the limit parameter.
There was a problem hiding this comment.
Added in 1850f44. test_job_events_tool_rejects_other_users_job tests the ownership check logic, and test_job_events_tool_schema validates the parameter schema. Full event retrieval tests require a PostgreSQL instance (integration test territory).
|
|
||
| if !wait { | ||
| // Spawn a background monitor that forwards Claude Code output | ||
| // into the main agent loop. |
There was a problem hiding this comment.
The spawned job monitor task is not tracked or awaited. If the main process shuts down before jobs complete, these background tasks may not be properly cleaned up. Consider either storing the JoinHandle in the CreateJobTool or ContainerJobManager for graceful shutdown, or documenting that these monitors are intentionally fire-and-forget and will terminate when the broadcast channel closes.
| // into the main agent loop. | |
| // into the main agent loop. | |
| // | |
| // NOTE: This monitor task is intentionally fire-and-forget and its | |
| // lifecycle is bound to the broadcast channel and agent event loop: | |
| // - It listens on a subscribed event stream (`etx.subscribe()`); when | |
| // the broadcast sender is dropped (e.g., during shutdown), the | |
| // subscription will be closed and the monitor task will exit. | |
| // - It forwards events into the main agent loop via `inject_tx`; when | |
| // the agent loop stops consuming or the sender is dropped, the | |
| // monitor will also terminate. | |
| // Because of this, we do not retain or await a JoinHandle here; the | |
| // monitor is expected to terminate naturally when the channels close. |
There was a problem hiding this comment.
Added documentation in 1850f44. The monitor is intentionally fire-and-forget: its lifetime is bound to the broadcast channel and inject sender. When either is dropped during shutdown, the monitor exits naturally. No JoinHandle needed.
| while end > 0 && !s.is_char_boundary(end) { | ||
| end -= 1; | ||
| } | ||
| format!("{}...", &s[..end]) |
There was a problem hiding this comment.
If the input string starts with a multi-byte character at every position from 0 to max (which is theoretically possible though unlikely), the loop could reduce end to 0, resulting in an empty slice with just "..." appended. While this is safe and won't panic, consider handling the edge case explicitly by either ensuring a minimum number of characters are included, or adding a comment explaining this is acceptable behavior.
| format!("{}...", &s[..end]) | |
| if end == 0 && !s.is_empty() { | |
| // If `max` falls inside the first multi-byte character, we may end up at 0. | |
| // In that case, include at least the first character before adding "...". | |
| let first_char = s.chars().next().unwrap(); | |
| format!("{}...", first_char) | |
| } else { | |
| format!("{}...", &s[..end]) | |
| } |
There was a problem hiding this comment.
Fixed in 1850f44. When end reaches 0 (max falls inside the first multi-byte character), we now include the first whole character before appending "..." instead of producing a bare "...".
| // Register event reader if store is available | ||
| if let Some(store) = store { | ||
| self.register_sync(Arc::new(JobEventsTool::new(store))); | ||
| tracing::info!("Registered job_events tool"); | ||
| } | ||
|
|
||
| // Register prompt tool if queue is available | ||
| if let Some(pq) = prompt_queue { | ||
| self.register_sync(Arc::new(JobPromptTool::new(pq))); | ||
| tracing::info!("Registered job_prompt tool"); | ||
| } | ||
|
|
||
| tracing::info!("Registered job management tools"); |
There was a problem hiding this comment.
The log message "Registered job management tools" is now less specific than before. Previously it logged "Registered 4 job management tools" with a specific count. Consider either updating the message to include the actual count of registered tools (which varies from 4 to 6 depending on whether store and prompt_queue are provided), or keep the generic message but add it once at the end instead of having individual logs for job_events and job_prompt followed by a generic message.
| // Register event reader if store is available | |
| if let Some(store) = store { | |
| self.register_sync(Arc::new(JobEventsTool::new(store))); | |
| tracing::info!("Registered job_events tool"); | |
| } | |
| // Register prompt tool if queue is available | |
| if let Some(pq) = prompt_queue { | |
| self.register_sync(Arc::new(JobPromptTool::new(pq))); | |
| tracing::info!("Registered job_prompt tool"); | |
| } | |
| tracing::info!("Registered job management tools"); | |
| // Base tools: CreateJobTool, ListJobsTool, JobStatusTool, CancelJobTool | |
| let mut job_tool_count = 4; | |
| // Register event reader if store is available | |
| if let Some(store) = store { | |
| self.register_sync(Arc::new(JobEventsTool::new(store))); | |
| job_tool_count += 1; | |
| } | |
| // Register prompt tool if queue is available | |
| if let Some(pq) = prompt_queue { | |
| self.register_sync(Arc::new(JobPromptTool::new(pq))); | |
| job_tool_count += 1; | |
| } | |
| tracing::info!("Registered {} job management tools", job_tool_count); |
There was a problem hiding this comment.
Fixed in 1850f44. Now tracks the actual count (4 base + conditionally registered tools) and logs "Registered {} job management tools" with the real number.
…ing) - Add ownership checks to JobEventsTool and JobPromptTool via ContextManager to prevent users from accessing other users' jobs (IDOR) - Combine Dockerfile gh CLI install into single apt-get layer - Handle truncate() edge case when max falls inside first multi-byte char - Log actual count of registered job management tools - Document fire-and-forget job monitor lifecycle - Add tests for ownership rejection and schema validation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Containers now fetch credentials via authenticated GET /worker/{id}/credentials
endpoint instead of receiving them baked into env vars at creation time. Secrets
are decrypted from SecretsStore on demand, scoped per-job via CredentialGrant,
and revoked automatically when the job completes.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Fetch and set credentials before execution begins. | ||
| // Worker is a single-threaded entry point so set_var is safe here; | ||
| // the spawned tool commands inherit the process environment. | ||
| let credentials = self.client.fetch_credentials().await?; | ||
| for cred in &credentials { | ||
| // SAFETY: worker is single-threaded at this point (no tools running yet) | ||
| unsafe { std::env::set_var(&cred.env_var, &cred.value) }; | ||
| } |
There was a problem hiding this comment.
std::env::set_var is now unsafe because mutating the process environment can cause undefined behavior in multi-threaded programs. Even if no tools have started yet, this worker is running under #[tokio::main] (multi-thread runtime), so the process already has multiple OS threads. Prefer avoiding global env mutation: collect the credentials and pass them to spawned Commands via Command::envs(...) (or plumb a per-process env map into tool execution), or switch the worker entrypoint to a single-thread Tokio runtime if global env mutation is required.
There was a problem hiding this comment.
Valid concern. The worker runtime is single-threaded at the point where set_var is called (before spawning any async tasks), but wrapping it in an unsafe block with a SAFETY comment is the right move. Tracking as a follow-up.
There was a problem hiding this comment.
Fixed in 1e51af7. Replaced unsafe { std::env::set_var(...) } with a HashMap<String, String> passed through JobContext.extra_env → Command::envs(). No more global env mutation.
| // Fetch and set credentials before spawning Claude. | ||
| // The spawned `claude` Command inherits env vars from this process. | ||
| let credentials = self.client.fetch_credentials().await?; | ||
| for cred in &credentials { | ||
| // SAFETY: bridge is single-threaded at this point (no child processes yet) | ||
| unsafe { std::env::set_var(&cred.env_var, &cred.value) }; | ||
| } | ||
| if !credentials.is_empty() { | ||
| tracing::info!( | ||
| job_id = %self.config.job_id, | ||
| "Injected {} credential(s) into environment", | ||
| credentials.len() |
There was a problem hiding this comment.
Same issue as the worker runtime: std::env::set_var is unsafe in multi-threaded processes and this binary runs under the default multi-thread Tokio runtime. To avoid potential UB, prefer passing credentials as explicit environment variables on the claude Command (e.g., Command::envs(...)) instead of mutating the global process environment, or ensure the bridge truly runs single-threaded before calling set_var.
| // Fetch and set credentials before spawning Claude. | |
| // The spawned `claude` Command inherits env vars from this process. | |
| let credentials = self.client.fetch_credentials().await?; | |
| for cred in &credentials { | |
| // SAFETY: bridge is single-threaded at this point (no child processes yet) | |
| unsafe { std::env::set_var(&cred.env_var, &cred.value) }; | |
| } | |
| if !credentials.is_empty() { | |
| tracing::info!( | |
| job_id = %self.config.job_id, | |
| "Injected {} credential(s) into environment", | |
| credentials.len() | |
| // Fetch credentials before spawning Claude. | |
| // Instead of mutating the global process environment (which is unsafe in | |
| // multi-threaded processes), pass these explicitly to the `claude` | |
| // Command via `.envs(...)`. | |
| let credentials = self.client.fetch_credentials().await?; | |
| let credential_envs: Vec<(String, String)> = credentials | |
| .iter() | |
| .map(|cred| (cred.env_var.clone(), cred.value.clone())) | |
| .collect(); | |
| if !credential_envs.is_empty() { | |
| tracing::info!( | |
| job_id = %self.config.job_id, | |
| "Prepared {} credential(s) for Claude environment", | |
| credential_envs.len() |
There was a problem hiding this comment.
Same as the runtime comment: set_var is called before any async tasks are spawned. Will wrap in unsafe with a SAFETY comment in a follow-up.
There was a problem hiding this comment.
Fixed in 1e51af7. Replaced unsafe { std::env::set_var(...) } with a HashMap<String, String> passed through JobContext.extra_env → Command::envs(). No more global env mutation.
| RUN curl -fsSL https://cli.github.com/packages/githubcli-archive-keyring.gpg \ | ||
| | dd of=/usr/share/keyrings/githubcli-archive-keyring.gpg \ | ||
| && echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/githubcli-archive-keyring.gpg] https://cli.github.com/packages stable main" \ | ||
| > /etc/apt/sources.list.d/github-cli.list \ | ||
| && apt-get update && apt-get install -y --no-install-recommends \ | ||
| ca-certificates \ | ||
| curl \ |
There was a problem hiding this comment.
This RUN step uses curl before apt-get install installs it (Debian slim images typically don't include curl by default), so the build will fail. Install ca-certificates + curl first (still can be in one layer), then add the GitHub CLI repo key/list, then install the rest.
| RUN curl -fsSL https://cli.github.com/packages/githubcli-archive-keyring.gpg \ | |
| | dd of=/usr/share/keyrings/githubcli-archive-keyring.gpg \ | |
| && echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/githubcli-archive-keyring.gpg] https://cli.github.com/packages stable main" \ | |
| > /etc/apt/sources.list.d/github-cli.list \ | |
| && apt-get update && apt-get install -y --no-install-recommends \ | |
| ca-certificates \ | |
| curl \ | |
| RUN apt-get update \ | |
| && apt-get install -y --no-install-recommends \ | |
| ca-certificates \ | |
| curl \ | |
| && curl -fsSL https://cli.github.com/packages/githubcli-archive-keyring.gpg \ | |
| | dd of=/usr/share/keyrings/githubcli-archive-keyring.gpg \ | |
| && echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/githubcli-archive-keyring.gpg] https://cli.github.com/packages stable main" \ | |
| > /etc/apt/sources.list.d/github-cli.list \ | |
| && apt-get update \ | |
| && apt-get install -y --no-install-recommends \ |
There was a problem hiding this comment.
Fixed in 48b68d7. Split the RUN to install curl and ca-certificates via apt-get before using curl to fetch the GitHub CLI GPG key.
| let mut grants = Vec::with_capacity(creds_obj.len()); | ||
| for (secret_name, env_var_value) in creds_obj { | ||
| let env_var = env_var_value.as_str().ok_or_else(|| { | ||
| ToolError::InvalidParameters(format!( | ||
| "credential env var for '{}' must be a string", | ||
| secret_name | ||
| )) | ||
| })?; | ||
|
|
||
| // Validate the secret actually exists | ||
| let exists = secrets.exists(user_id, secret_name).await.map_err(|e| { | ||
| ToolError::ExecutionFailed(format!( | ||
| "failed to check secret '{}': {}", | ||
| secret_name, e | ||
| )) | ||
| })?; | ||
|
|
||
| if !exists { | ||
| return Err(ToolError::ExecutionFailed(format!( | ||
| "secret '{}' not found. Store it first via 'ironclaw tool auth' or the web UI.", | ||
| secret_name | ||
| ))); | ||
| } | ||
|
|
||
| grants.push(CredentialGrant { | ||
| secret_name: secret_name.clone(), | ||
| env_var: env_var.to_string(), | ||
| }); |
There was a problem hiding this comment.
credentials allows callers to choose arbitrary environment variable names, but there is no validation of env_var. This can be abused to set sensitive process variables (e.g., LD_PRELOAD, BASH_ENV, RUST_LOG, etc.) inside the container. Consider validating env_var against a conservative pattern (e.g., ^[A-Z_][A-Z0-9_]*$), rejecting names containing =/NUL, and optionally denylisting particularly dangerous variables.
There was a problem hiding this comment.
Fixed in 48b68d7. Added validate_env_var_name() that enforces ^[A-Z_][A-Z0-9_]*$ pattern and rejects a denylist of dangerous names (LD_PRELOAD, PATH, BASH_ENV, etc.).
| if let Ok(job_ctx) = self.context_manager.get_context(job_id).await { | ||
| if job_ctx.user_id != ctx.user_id { | ||
| return Err(ToolError::ExecutionFailed(format!( | ||
| "job {} does not belong to current user", | ||
| &job_id_str[..8] | ||
| ))); | ||
| } |
There was a problem hiding this comment.
The job ownership check is bypassed if ContextManager::get_context(job_id) returns Err (e.g., after a process restart, or if contexts are not loaded for older jobs). In that case, the code proceeds to load and return events from the DB, potentially exposing other users' job logs. Consider making a missing context a hard failure (not found/unauthorized), or verifying ownership via a persisted job record in the Store before returning events.
| if let Ok(job_ctx) = self.context_manager.get_context(job_id).await { | |
| if job_ctx.user_id != ctx.user_id { | |
| return Err(ToolError::ExecutionFailed(format!( | |
| "job {} does not belong to current user", | |
| &job_id_str[..8] | |
| ))); | |
| } | |
| // A missing context is treated as not found / unauthorized to avoid leaking logs. | |
| let job_ctx = self | |
| .context_manager | |
| .get_context(job_id) | |
| .await | |
| .map_err(|_| { | |
| ToolError::ExecutionFailed(format!( | |
| "job {} not found or not accessible to current user", | |
| &job_id_str[..8] | |
| )) | |
| })?; | |
| if job_ctx.user_id != ctx.user_id { | |
| return Err(ToolError::ExecutionFailed(format!( | |
| "job {} does not belong to current user", | |
| &job_id_str[..8] | |
| ))); |
There was a problem hiding this comment.
Fixed in 48b68d7. Changed if let Ok(job_ctx) to map_err that returns a hard error when the context is missing, closing the IDOR bypass.
| // Verify the caller owns this job. | ||
| if let Ok(job_ctx) = self.context_manager.get_context(job_id).await { | ||
| if job_ctx.user_id != ctx.user_id { | ||
| return Err(ToolError::ExecutionFailed(format!( | ||
| "job {} does not belong to current user", | ||
| &job_id_str[..8] | ||
| ))); | ||
| } |
There was a problem hiding this comment.
The ownership check is only enforced when ContextManager::get_context(job_id) succeeds; if it fails, the prompt is still queued. That can allow queuing prompts to jobs not owned by the caller (e.g., after restart or if the context isn't present). Consider treating a missing context as an error, or validating ownership via persisted job metadata before enqueuing the prompt.
| // Verify the caller owns this job. | |
| if let Ok(job_ctx) = self.context_manager.get_context(job_id).await { | |
| if job_ctx.user_id != ctx.user_id { | |
| return Err(ToolError::ExecutionFailed(format!( | |
| "job {} does not belong to current user", | |
| &job_id_str[..8] | |
| ))); | |
| } | |
| // Verify the caller owns this job. Treat missing context as an error. | |
| let job_ctx = self | |
| .context_manager | |
| .get_context(job_id) | |
| .await | |
| .map_err(|_| { | |
| ToolError::ExecutionFailed(format!( | |
| "job context not found for job {}", | |
| &job_id_str[..8] | |
| )) | |
| })?; | |
| if job_ctx.user_id != ctx.user_id { | |
| return Err(ToolError::ExecutionFailed(format!( | |
| "job {} does not belong to current user", | |
| &job_id_str[..8] | |
| ))); |
There was a problem hiding this comment.
Fixed in 48b68d7. Same fix as JobEventsTool: missing context now returns a hard error instead of silently skipping the ownership check.
| tracing::info!( | ||
| job_id = %job_id, | ||
| env_var = %grant.env_var, | ||
| secret = %grant.secret_name, | ||
| "Serving credential to container" | ||
| ); |
There was a problem hiding this comment.
The credentials endpoint logs the granted secret_name for every credential served. Even without logging values, secret names can leak sensitive information (e.g., which providers/accounts a user has configured). Consider lowering this to debug level, redacting/omitting secret, or logging only counts unless you explicitly need per-secret audit logs.
There was a problem hiding this comment.
Fixed in 48b68d7. Changed from tracing::info! to tracing::debug! and removed the secret name from the log message.
| let explicit_dir = params | ||
| .get("project_dir") | ||
| .and_then(|v| v.as_str()) | ||
| .map(PathBuf::from); |
There was a problem hiding this comment.
project_dir is now user/LLM-provided, but resolve_project_dir will create_dir_all(&dir) before canonicalizing/prefix-checking. That means an attacker can trigger directory creation outside ~/.ironclaw/projects/ (including via symlink traversal) even though the function later errors. Since the schema says this should be an existing directory, consider requiring the explicit path to already exist and canonicalizing it without creating anything (and only auto-create when project_dir is omitted).
There was a problem hiding this comment.
Fixed in 48b68d7. Explicit paths now require the directory to already exist (canonicalize before any creation). Only auto-generated paths (no explicit dir provided) get create_dir_all.
… type consolidation) - Implement real CONNECT tunnel with bidirectional TCP piping via hyper upgrade - Fix readonly_rootfs to apply for both ReadOnly and WorkspaceWrite policies - Consolidate duplicate CredentialMapping/CredentialLocation into secrets::types - Share reqwest::Client across proxy requests instead of per-request allocation - Store Docker connection and reuse across executions - Remove .unwrap() from proxy response builders with safe fallbacks - Add output truncation to direct (non-container) execution (64KB limit) - Delete dead src/tools/sandbox.rs (ToolSandbox never used) - Fix connect_docker error message to list all attempted socket paths - Update proxy credential injection to handle all CredentialLocation variants - Use glob-based host_patterns matching for credential lookup in proxy policy Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ing) - Dockerfile: install curl+ca-certificates before fetching GitHub CLI GPG key - JobEventsTool/JobPromptTool: reject missing context (prevents IDOR bypass) - parse_credentials: validate env var names against denylist and pattern - resolve_project_dir: require explicit paths to exist before validation - Credential serving: lower log level from info to debug Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…andling, tests) - auth: constant-time token comparison via subtle::ConstantTimeEq - auth: replace hand-rolled hex_encode with std::fmt::Write fold - api: report_status now updates ContainerHandle (was a no-op) - api: log complete_job errors instead of silently discarding - job_manager: log Docker cleanup errors in stop_job/complete_job - job_manager: extract validate_bind_mount_path with proper error on missing home_dir and mandatory base dir creation before canonicalize - job_manager: cache Docker connection across operations - error: remove dead OrchestratorError::AuthFailed and ContainerTimeout - Add 13 new tests (prompt queue, credentials, events, status, paths) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolve 6 conflicts: - Cargo.toml: normalize subtle version to "2" - Dockerfile.worker: adopt MSRV 1.92 from main - auth.rs: keep serde import needed by CredentialGrant - job_manager.rs: combine cached docker + cleanup-on-failure pattern - proxy/http.rs: keep simpler CONNECT deny check - worker/runtime.rs: use shared floor_char_boundary utility Fix 2 clippy collapsible_if warnings from merged code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (3)
Dockerfile.worker:12
- Rust 1.88 does not exist yet. As of February 2026, the latest stable Rust version would be around 1.84-1.86 (following the 6-week release cycle from version 1.85 in January 2025). Using a non-existent version will cause the Dockerfile build to fail when rustup tries to install it.
Check the actual Rust version required by rig-core and update both Dockerfile.worker line 12 and line 49, and Cargo.toml line 5 to use a valid Rust version that's actually released.
FROM rust:1.92-bookworm AS builder
Dockerfile.worker:49
- Same issue: Rust 1.88.0 does not exist. The rustup install will fail. Update to a valid Rust version that exists.
RUN curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --default-toolchain 1.92.0 \
src/worker/claude_bridge.rs:139
- Same issue as in worker/runtime.rs: The SAFETY comment claims "bridge is single-threaded at this point (no child processes yet)", but the tokio runtime creates background threads. While the justification is that no child processes have been spawned yet, the tokio runtime threads can still access the environment concurrently, making
std::env::set_varunsafe according to Rust's documentation.
Consider the same solution: passing credentials through a thread-safe structure rather than modifying the global process environment.
///
/// This replaces `--dangerously-skip-permissions` with an explicit set of
/// auto-approved tools. The Docker container is still the primary security
/// boundary; this is defense-in-depth.
fn write_permission_settings(&self) -> Result<(), WorkerError> {
let settings_json = build_permission_settings(&self.config.allowed_tools);
let settings_dir = std::path::Path::new("/workspace/.claude");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Spawn a task to establish the tunnel after the upgrade completes | ||
| let target = target_addr.clone(); | ||
| tokio::spawn(async move { | ||
| match hyper::upgrade::on(req).await { | ||
| Ok(upgraded) => { | ||
| let mut client_stream = TokioIo::new(upgraded); | ||
| match TcpStream::connect(&target).await { | ||
| Ok(mut server_stream) => { | ||
| if let Err(e) = | ||
| tokio::io::copy_bidirectional(&mut client_stream, &mut server_stream) | ||
| .await | ||
| { | ||
| tracing::debug!("Proxy: tunnel to {} closed: {}", target, e); | ||
| } | ||
| } | ||
| Err(e) => { | ||
| tracing::error!("Proxy: failed to connect to {}: {}", target, e); | ||
| } | ||
| } | ||
| } | ||
| Err(e) => { | ||
| tracing::error!("Proxy: upgrade failed for {}: {}", target, e); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| // For CONNECT, we return 200 OK and the client will upgrade to TLS | ||
| // The actual TLS connection goes directly to the target, we just act as a tunnel | ||
| Response::builder() | ||
| .status(StatusCode::OK) | ||
| .body(empty_body()) | ||
| .unwrap() | ||
| // Return 200 OK so the client begins the TLS handshake over the upgraded connection | ||
| make_response(StatusCode::OK, empty_body()) |
There was a problem hiding this comment.
The CONNECT handler consumes req inside the spawned async task (line 284), but returns a 200 OK response outside the task (line 308). This creates a race condition: the response is sent before the upgrade is established. According to HTTP CONNECT semantics, the 200 OK response should only be sent after the connection to the target server succeeds. If TcpStream::connect(&target) fails, the client will receive 200 OK but the tunnel won't work.
Consider establishing the target connection before responding, or at minimum check that the upgrade succeeds before returning 200 OK.
There was a problem hiding this comment.
This is actually correct hyper CONNECT semantics. The 200 OK must be sent first to trigger the HTTP upgrade; hyper::upgrade::on(req) then resolves in the spawned task after the client receives the response. The client can't start TLS until it gets the 200, so there's no race. This is the standard pattern from hyper's own examples. No change needed.
| if stdout.len() > half_max { | ||
| stdout.truncate(half_max); | ||
| truncated = true; | ||
| } | ||
| if stderr.len() > half_max { | ||
| stderr.truncate(half_max); | ||
| truncated = true; | ||
| } | ||
|
|
There was a problem hiding this comment.
The truncate() method on String operates on bytes, not character boundaries. If stdout or stderr contains multi-byte UTF-8 characters, truncating at half_max bytes could split a character, causing invalid UTF-8. This is similar to the issue that was fixed in worker/runtime.rs truncate() function.
Consider using the same char-boundary-aware truncation approach from worker/runtime.rs lines 504-517, or extract that logic into a shared utility function.
| if stdout.len() > half_max { | |
| stdout.truncate(half_max); | |
| truncated = true; | |
| } | |
| if stderr.len() > half_max { | |
| stderr.truncate(half_max); | |
| truncated = true; | |
| } | |
| // Truncate on UTF-8 character boundaries to avoid splitting multi-byte chars. | |
| let mut truncate_to_char_boundary = |s: &mut String| { | |
| if s.len() > half_max { | |
| let mut idx = half_max; | |
| // Move back to the nearest valid character boundary. | |
| while idx > 0 && !s.is_char_boundary(idx) { | |
| idx -= 1; | |
| } | |
| s.truncate(idx); | |
| truncated = true; | |
| } | |
| }; | |
| truncate_to_char_boundary(&mut stdout); | |
| truncate_to_char_boundary(&mut stderr); |
There was a problem hiding this comment.
Fixed in 1a86a7c. stdout.truncate(half_max) and stderr.truncate(half_max) now use crate::util::floor_char_boundary() to find a safe UTF-8 character boundary before truncating, matching the pattern already used in worker/runtime.rs and tools/builtin/shell.rs.
serrrfirat
left a comment
serrrfirat
left a comment
There was a problem hiding this comment.
Review Summary
Verdict: APPROVE ✅
Well-structured PR adding three connected features for sandbox workflow improvements. Good test coverage (367 new test lines).
Key Findings
P1 — Credentials endpoint returns plaintext secrets over HTTP
src/orchestrator/api.rs:get_credentials_handler — Decrypted secret values are returned as JSON over the internal API. While this is internal-only (orchestrator ↔ container on localhost), a compromised container can exfiltrate all granted secrets in one call. Consider a one-time-use token pattern to reduce the exposure window.
P2 — containers field changed to pub(crate)
src/orchestrator/job_manager.rs — ContainerJobManager::containers is now pub(crate) to support tests directly accessing internal state. Tests should ideally use the public API to maintain encapsulation.
P2 — Job monitor has no backpressure beyond channel capacity
src/agent/job_monitor.rs — Uses an unbounded broadcast receiver. If the agent loop is slow, the monitor logs a warning on lag but drops events silently. For JobResult events this could mean missed completion notices. The mpsc inject channel (bounded at 64) mitigates this somewhat.
P3 — CONNECT tunnel has no timeout
src/sandbox/proxy/http.rs:handle_connect — The bidirectional copy runs until one side closes. A stuck connection could leak the spawned task. Consider adding a timeout.
P3 — Restarted jobs lose credential grants
src/channels/web/server.rs:jobs_restart_handler passes vec![] for credential_grants. Restarted jobs won't have access to their original secrets.
Highlights
- CONNECT tunnel fix is critical — the old implementation returned 200 OK but never established the bidirectional TCP copy. Now properly uses
hyper::upgrade→TcpStream::connect→copy_bidirectional. - Channel injection pattern is clean —
inject_sender()with aMutex<Option<Receiver>>that can only be taken once. - Credential grant lifecycle is correct — created with token, revoked with token.
- Cached Docker connection avoids reconnecting per operation.
PR #57 Review: feat: Add PR review tools, job monitor, and channel injection for E2E sandbox workflowsSummaryThis PR implements a multi-faceted enhancement to the IronClaw AI agent framework, introducing three major subsystems: (1) job monitoring with channel injection for forwarding container output to the main agent loop, (2) credential injection system with secrets store integration for authenticated sandbox execution, and (3) enhanced PR review workflows. The changes span 26 files with ~1,600 lines added and ~200 removed, touching core systems (orchestrator, channel manager, sandbox proxy), tooling (new ProsJob Monitoring & Channel Injection:
Credential Management:
Sandbox Security:
Code Quality:
ConcernsSecurity - Unsafe
Security - Path Validation TOCTOU:
Security - Credential Exposure in Logs:
Race Condition - Monitor Lifecycle:
Complexity Explosion:
Missing Documentation:
Dockerfile Layering:
Suggestions
Reviewed using Kilocode CLI with cerebras/zai-glm-4.7 model (free tier). |
…ulti-byte panics String::truncate() panics when the index falls mid-way through a multi-byte UTF-8 character. Use the same floor_char_boundary utility already used in worker/runtime.rs and tools/builtin/shell.rs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ool fixes) Resolves conflicts in 4 files by keeping the `dyn Database` trait abstraction from main while preserving the new sandbox workflow fields (secrets_store, event_tx, inject_tx, prompt_queue) from workflows-2. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Session tokens only authenticate against private.near.ai, not cloud-api.near.ai. The default base_url now matches the api_mode: - Responses (session token): https://private.near.ai - ChatCompletions (API key): https://cloud-api.near.ai This broke when the multi-provider merge introduced cloud-api.near.ai as the unconditional default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.credential_mappings.iter().find(|m| { | ||
| m.host_patterns | ||
| .iter() | ||
| .any(|pattern| host_matches_pattern(&host_lower, pattern)) |
There was a problem hiding this comment.
find_credential lowercases the request host but compares it against host_patterns without normalizing the patterns. Since hostnames are case-insensitive, patterns containing uppercase characters will never match (both exact and wildcard), causing credentials not to be injected unexpectedly. Consider lowercasing patterns during comparison (or normalizing CredentialMapping.host_patterns on creation/loading).
| .any(|pattern| host_matches_pattern(&host_lower, pattern)) | |
| .any(|pattern| { | |
| let pattern_lower = pattern.to_lowercase(); | |
| host_matches_pattern(&host_lower, &pattern_lower) | |
| }) |
There was a problem hiding this comment.
Fixed in f5aa243. host_matches_pattern now lowercases the pattern before comparison, so mixed-case patterns like "Api.Example.COM" match correctly against the already-lowercased host.
| let limit = params.get("limit").and_then(|v| v.as_u64()).unwrap_or(50) as usize; | ||
|
|
||
| let events = | ||
| self.store.list_job_events(job_id).await.map_err(|e| { | ||
| ToolError::ExecutionFailed(format!("failed to load job events: {}", e)) | ||
| })?; |
There was a problem hiding this comment.
This tool loads all job events from the DB (list_job_events) and only then slices to the last limit. For long-running jobs with large event logs this can become a memory/time hotspot. Consider adding a DB query that returns only the most recent N events (e.g., ORDER BY id DESC LIMIT $1) and using that here.
There was a problem hiding this comment.
Fixed in f5aa243. The limit is now pushed into the SQL query via a new limit: Option<i64> parameter on Database::list_job_events. See the reply on the duplicate comment below for details.
| let (canonical_dir, was_explicit) = match explicit { | ||
| Some(d) => { | ||
| // Explicit paths: validate BEFORE creating anything. | ||
| // The path must already exist (it comes from a previous job run). | ||
| let canonical = d.canonicalize().map_err(|e| { | ||
| ToolError::InvalidParameters(format!( | ||
| "explicit project dir {} does not exist or is inaccessible: {}", | ||
| d.display(), | ||
| e | ||
| )) | ||
| })?; | ||
| if !canonical.starts_with(&canonical_base) { | ||
| return Err(ToolError::InvalidParameters(format!( | ||
| "project directory must be under {}", | ||
| canonical_base.display() | ||
| ))); | ||
| } | ||
| (canonical, true) | ||
| } | ||
| None => { | ||
| let dir = canonical_base.join(project_id.to_string()); | ||
| std::fs::create_dir_all(&dir).map_err(|e| { | ||
| ToolError::ExecutionFailed(format!( | ||
| "failed to create project dir {}: {}", | ||
| dir.display(), | ||
| e | ||
| )) | ||
| })?; | ||
| let canonical = dir.canonicalize().map_err(|e| { | ||
| ToolError::ExecutionFailed(format!( | ||
| "failed to canonicalize project dir {}: {}", | ||
| dir.display(), | ||
| e | ||
| )) | ||
| })?; | ||
| (canonical, false) | ||
| } | ||
| }; | ||
|
|
||
| std::fs::create_dir_all(&dir).map_err(|e| { | ||
| ToolError::ExecutionFailed(format!( | ||
| "failed to create project dir {}: {}", | ||
| dir.display(), | ||
| e | ||
| )) | ||
| })?; | ||
|
|
||
| // Canonicalize resolves symlinks, `..`, etc. so we can do a reliable prefix check. | ||
| let canonical_dir = dir.canonicalize().map_err(|e| { | ||
| ToolError::ExecutionFailed(format!( | ||
| "failed to canonicalize project dir {}: {}", | ||
| dir.display(), | ||
| e | ||
| )) | ||
| })?; | ||
|
|
||
| if !canonical_dir.starts_with(&canonical_base) { | ||
| return Err(ToolError::InvalidParameters(format!( | ||
| "project directory must be under {}", | ||
| canonical_base.display() | ||
| ))); | ||
| } | ||
| let _ = was_explicit; |
There was a problem hiding this comment.
was_explicit is computed but intentionally unused (let _ = was_explicit;). This can be simplified by not binding it at all (or renaming to _was_explicit if you want to keep the boolean for readability), which makes the control flow clearer.
There was a problem hiding this comment.
Fixed in f5aa243. Renamed to _was_explicit and removed the let _ = was_explicit; line.
| // Fetch and set credentials before spawning Claude. | ||
| // The spawned `claude` Command inherits env vars from this process. | ||
| let credentials = self.client.fetch_credentials().await?; | ||
| for cred in &credentials { | ||
| // SAFETY: bridge is single-threaded at this point (no child processes yet) | ||
| unsafe { std::env::set_var(&cred.env_var, &cred.value) }; | ||
| } |
There was a problem hiding this comment.
Same thread-safety concern as in WorkerRuntime: std::env::set_var mutates global process env and is not safe under the default multi-thread Tokio runtime. The current SAFETY comment (“single-threaded … no child processes yet”) doesn’t address other runtime threads. Consider setting env vars directly on the Command that spawns claude (and any other subprocesses) rather than mutating the process environment.
| /// Build a response with guaranteed success (valid status + simple body cannot fail). | ||
| fn make_response( | ||
| status: StatusCode, | ||
| body: BoxBody<Bytes, Infallible>, | ||
| ) -> Response<BoxBody<Bytes, Infallible>> { | ||
| Response::builder() | ||
| .status(status) | ||
| .header("Content-Type", "text/plain") | ||
| .body(full_body(Bytes::from(message))) | ||
| .unwrap() | ||
| .body(body) | ||
| .unwrap_or_else(|_| { | ||
| Response::new( | ||
| Full::new(Bytes::from("Internal error")) | ||
| .map_err(|_| unreachable!()) | ||
| .boxed(), | ||
| ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
make_response’s fallback path uses Response::new(...), which defaults to HTTP 200. If Response::builder().body(...) ever fails (e.g., due to an invalid status/headers passed in the future), this would silently turn an error into a 200 OK. Consider reusing make_response_from_builder here or explicitly setting StatusCode::INTERNAL_SERVER_ERROR in the fallback response.
There was a problem hiding this comment.
Fixed in f5aa243. The fallback in make_response now explicitly sets StatusCode::INTERNAL_SERVER_ERROR instead of defaulting to 200 OK.
private.near.ai now supports both Responses and ChatCompletions endpoints, so there is no reason to route through cloud-api.near.ai. This also fixes session token auth which only works against private.near.ai. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ssions Three fixes for the sandbox/Claude Code pipeline: 1. SQLite "database is locked": set WAL journal mode in migrations and PRAGMA busy_timeout=5000 on every connection across LibSqlBackend, LibSqlSecretsStore, and LibSqlWasmToolStore (~83 async call sites). 2. Claude Code container auth: extract OAuth token from macOS Keychain (or Linux ~/.claude/.credentials.json) at startup and inject via CLAUDE_CODE_OAUTH_TOKEN env var. Removes the broken bind-mount approach that failed on uid mismatch. 3. Claude Code tool permissions: wire CLAUDE_CODE_ALLOWED_TOOLS env var through to the worker binary (was hardcoded to empty vec), and expand defaults to include all standard tools (Read, Write, Edit, Glob, Grep, NotebookEdit, Bash, Task, WebFetch, WebSearch). Also adds --verbose flag to claude CLI (required with stream-json + -p), failover provider model switching, and nearai models endpoint fix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ilblackdragon
left a comment
ilblackdragon
left a comment
There was a problem hiding this comment.
PR Review: feat: Add PR review tools, job monitor, and channel injection for E2E sandbox workflows
This is a large, feature-rich PR that adds significant capabilities: job monitoring, channel injection, credential grants, Claude Code bridge improvements, and HTTPS CONNECT tunneling. The architecture is well thought out and the test coverage is good. However, there are several issues that should be addressed before merging.
Critical Issues
1. credential_grants_json piggybacking on the description column is a correctness bug
The SandboxJobRecord.credential_grants_json field is stored in and read from the existing description column of agent_jobs:
// In history/store.rs (postgres backend):
credential_grants_json: r.get::<_, String>("description"),
// In save_sandbox_job (both backends):
// Previously: VALUES ($1, $2, '', $3, 'sandbox', ...)
// Now: VALUES ($1, $2, $3, $4, 'sandbox', ...)
// Where $3 is credential_grants_json, mapped to the `description` columnThis has multiple problems:
- The
descriptioncolumn semantically holds the task description. Non-sandbox jobs use it for its intended purpose. Overloading it with serialized JSON credentials is confusing and fragile. - When
save_sandbox_jobwrites the record, what was previously hardcoded as''for description is nowcredential_grants_json. Callers who read thedescriptioncolumn for display purposes will get[{"secret_name":"github_token","env_var":"GITHUB_TOKEN"}]instead of a human-readable description. - The comment says "Stored in the
descriptioncolumn ofagent_jobs(unused for sandbox jobs)" -- but this assumption may break if any UI or API endpoint readsdescriptiongenerically. - There is no schema migration to add a proper column. This should either be a dedicated
credential_grants_jsoncolumn (with a migration) or stored in the existingmetadataJSONB column.
2. Credential values served over HTTP without TLS
get_credentials_handler returns decrypted secret values as plaintext JSON over the orchestrator's internal HTTP API (port 50051). On Linux, this binds to 0.0.0.0:
let addr = if cfg!(target_os = "linux") {
std::net::SocketAddr::from(([0, 0, 0, 0], port))
} else {
std::net::SocketAddr::from(([127, 0, 0, 1], port))
};The response body containing {"env_var": "GITHUB_TOKEN", "value": "ghp_actual_secret_here"} travels over the network unencrypted. While worker_auth_middleware protects against unauthorized access, any network observer can read the credential values. At minimum, add documentation noting this security consideration. Better: ensure the orchestrator API only communicates over the Docker bridge network.
Important Issues
3. let _ = was_explicit; is dead code
In resolve_project_dir, the was_explicit variable is computed in the destructuring but immediately suppressed:
let (canonical_dir, was_explicit) = match explicit { ... };
let _ = was_explicit;If it is not needed, simplify to let (canonical_dir, _) = match explicit { ... };. If it will be used later, add a TODO.
4. Claude Code allowed tools list lost glob patterns
default_claude_code_allowed_tools() changed from glob patterns like "Bash(*)", "Edit(*)", "WebFetch(*)", "Task(*)" to plain names "Bash", "Edit", "WebFetch", "Task". Claude Code's .claude/settings.json permission system uses glob patterns for sub-tool matching. If Claude Code now accepts plain names, this should be documented. If not, this could cause permission denials inside containers -- tools would require user approval that never comes in a non-interactive container.
5. serde_json::to_string(&credential_grants).unwrap_or_default() silently swallows errors
let credential_grants_json = serde_json::to_string(&credential_grants).unwrap_or_default();If serialization fails, the job gets persisted with credential_grants_json = "", and on restart the grants are silently lost via unwrap_or_default() on the deserialize side. This is a silent data loss path. Consider propagating the error or at least logging a warning.
6. copy_dir_recursive follows symlinks
The function uses src_path.is_dir() and std::fs::copy, both of which follow symlinks. A .claude directory on the host with symlinks pointing to sensitive files (e.g., ~/.ssh/id_rsa) could cause those files to be copied into the container's writable home directory. Consider using symlink_metadata() to detect and skip symlinks.
7. extra_env is cloned on every tool call in WorkerRuntime
let ctx = JobContext {
extra_env: self.extra_env.clone(),
..Default::default()
};This runs on every tool invocation. If there are many credentials and many tool calls, this creates unnecessary allocations. Consider wrapping extra_env in an Arc<HashMap<String, String>> to make cloning cheap.
Minor Issues
8. CredentialLocation::AuthorizationBasic and UrlPath silently skipped in proxy
The proxy's forward_request logs a warning but silently drops credentials for unsupported location types. The container has no indication that auth was not injected. Consider returning an error or documenting this as a known limitation.
9. SandboxManager Docker connection caching
Caching the Docker connection is good for reuse, but Docker connections can go stale (daemon restart, socket timeout). Consider a reconnect-on-error fallback.
10. report_status now takes State but discards the update.message type
The report_status handler was updated to accept State(state) and call update_worker_status, but StatusUpdate.message is typed as Option<String> while being used for both human-readable status text and iteration count. The semantics are clear from context but could benefit from stronger typing.
Positive Observations
- The job monitor design (broadcast -> mpsc inject channel) is clean and well-tested with 4 focused tests
- The credential grant system (validate at creation, serve decrypted on-demand, revoke on cleanup) is architecturally sound
- The
validate_env_var_namefunction with its denylist of dangerous env vars (LD_PRELOAD,PATH,HOME, etc.) is excellent defense-in-depth - The
resolve_job_idshort-prefix matching (git-style) is a nice UX improvement - Outstanding test coverage: job_monitor (4 tests), orchestrator API (7 new tests), credentials (4 tests), claude_bridge parsing (updated + new), proxy (2 new), job tools (12+ new tests)
- The CONNECT tunnel implementation with
copy_bidirectionaland 30-minute timeout is well designed - Moving
CredentialMapping/CredentialLocationtosecrets/types.rsis a good refactoring - WAL mode +
busy_timeout = 5000for libSQL connections are important concurrency improvements - The
make_response/make_response_from_builderhelpers eliminate.unwrap()calls on Response construction in the proxy - The
floor_char_boundaryusage for output truncation prevents multi-byte panics
Summary
The main issue to fix before merging is the credential_grants_json being stored in the description column -- it needs a proper storage location (dedicated column with migration, or the metadata JSON field). The unencrypted credential transport should at least be documented as a security consideration. The rest are quality improvements for an already solid PR.
- Normalize host_patterns to lowercase in proxy policy matching - Push LIMIT into SQL for list_job_events (Database trait + both backends) - Remove unused was_explicit binding in job tool - Return 500 instead of 200 in make_response fallback path - Update copy_auth_from_mount docstring for env-var default - Use entry.file_type() instead of is_dir() to avoid following symlinks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
GitHub PR Review Batch 3 - nearai/ironclawPR #71: Fix undo/redo deadlock and checkpoint behaviorSummaryThis PR addresses critical concurrency issues in the undo/redo system. The main changes are:
Pros
Concerns
Suggestions
PR #66: Update README architecture diagram to use Unicode box drawingSummaryThis PR updates the README.md architecture diagram from ASCII art to Unicode box drawing characters. The change improves readability and visual appeal of the system architecture diagram, using characters like Pros
Concerns
Suggestions
PR #63: Memory Guardian and Cognitive RoutinesSummaryThis is a large PR (894 lines of new code) adding a comprehensive memory management system with two layers: Cognitive Routines (Prompt-Level):
Memory Guardian (System-Level):
The PR also adds line number tracking to memory chunks for citation support (V9 migration), checkpoint tracker to Thread struct, and extensive testing (15 new tests). Pros
Concerns
Suggestions
PR #62: Add Tinfoil private inference providerSummaryThis PR adds support for Tinfoil as a new LLM backend. The changes include:
Pros
Concerns
Suggestions
PR #61: Add PostgreSQL test workflow for CISummaryThis PR adds a PostgreSQL with pgvector service to the GitHub Actions test workflow. The changes enable running tests against a real PostgreSQL database instead of libSQL-only testing. The service is configured with pgvector/pgvector:pg16 image and health checks. Pros
Concerns
Suggestions
PR #57: Sandbox job monitoring and credential grants persistenceSummaryThis is a large PR (~6333 lines touched) introducing several features:
Pros
Concerns
Suggestions
SummaryHighest Quality PRs
Needs Splitting
Minor Concerns
Recommendations
|
- Restore glob patterns in default_claude_code_allowed_tools (Bash -> Bash(*)) - Add tracing::warn for credential grant serialize/deserialize failures - Wrap extra_env in Arc<HashMap> to avoid deep cloning per tool call - Document unsupported credential locations (AuthorizationBasic, UrlPath) - Document TOCTOU window in validate_bind_mount_path - Expand doc comments on JobEventsTool and JobPromptTool Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressing the remaining items from @ilblackdragon's self-review, fixed in 228a218: Issue 4 — Claude Code allowed tools lost glob patterns: Issue 5 — Issue 7 — Issue 8 — |
|
Addressing items from @tribendu's review comment, fixed in 228a218: Security — Path Validation TOCTOU: Missing Documentation — |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/worker/claude_bridge.rs:706
- The
truncatefunction can return an empty string whenmax_lenfalls within the first multi-byte character. The loop reducesendto 0, resulting in an empty slice&s[..0]. This differs from the behavior of the similar code insrc/util.rs::floor_char_boundary, which has more robust handling. Consider usingcrate::util::floor_char_boundaryinstead of reimplementing this logic, or adding a check to include at least one character whenendreaches 0.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tokio::spawn(async move { | ||
| match hyper::upgrade::on(req).await { | ||
| Ok(upgraded) => { | ||
| let mut client_stream = TokioIo::new(upgraded); | ||
| match TcpStream::connect(&target).await { | ||
| Ok(mut server_stream) => { | ||
| let tunnel_timeout = std::time::Duration::from_secs(30 * 60); | ||
| match tokio::time::timeout( | ||
| tunnel_timeout, | ||
| tokio::io::copy_bidirectional(&mut client_stream, &mut server_stream), | ||
| ) | ||
| .await | ||
| { | ||
| Ok(Ok(_)) => {} | ||
| Ok(Err(e)) => { | ||
| tracing::debug!("Proxy: tunnel to {} closed: {}", target, e); | ||
| } | ||
| Err(_) => { | ||
| tracing::info!( | ||
| "Proxy: tunnel to {} timed out after 30m, closing", | ||
| target | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| Err(e) => { | ||
| tracing::error!("Proxy: failed to connect to {}: {}", target, e); | ||
| } | ||
| } | ||
| } | ||
| Err(e) => { | ||
| tracing::error!("Proxy: upgrade failed for {}: {}", target, e); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
The CONNECT tunnel implementation spawns a background task that is not tracked or awaited. If the proxy is shut down gracefully, these spawned tasks may continue running until the 30-minute timeout expires, potentially leaking resources. Consider storing tunnel tasks in a JoinSet or similar structure to enable graceful cancellation on shutdown, or document that the 30-minute timeout is acceptable and these tasks will be forcibly terminated on process exit.
There was a problem hiding this comment.
Addressing 3 new Copilot comments in 99ed72d:
CONNECT tunnel task not tracked (#2818186815): Documented that the 30-minute timeout guarantees every tunnel task terminates. On process exit tasks are dropped by the runtime—no JoinSet needed.
Credentials endpoint logs secret names (#2818186890): Removed secret_name from error-level and warn-level logs in the credential grant path. The job_id is still logged for debugging.
DANGEROUS_ENV_VARS missing hijack vectors (#2818186950): Added PYTHONPATH, NODE_PATH, PERL5LIB, RUBYLIB, CLASSPATH, LD_AUDIT, JAVA_TOOL_OPTIONS, and MAVEN_OPTS to the denylist.
| let decrypted = secrets | ||
| .get_decrypted(&state.user_id, &grant.secret_name) | ||
| .await | ||
| .map_err(|e| { | ||
| tracing::error!( | ||
| job_id = %job_id, | ||
| secret = %grant.secret_name, | ||
| "Failed to decrypt secret for credential grant: {}", e | ||
| ); | ||
| StatusCode::INTERNAL_SERVER_ERROR | ||
| })?; |
There was a problem hiding this comment.
The credentials endpoint logs the secret name on every error at the error level. Even without logging values, secret names can leak sensitive information (e.g., which providers/accounts a user has configured, internal service names, etc.). The successful path already uses debug level logging. Consider also lowering the error log to warn or debug, or redacting the secret name from the error message.
| const DANGEROUS_ENV_VARS: &[&str] = &[ | ||
| "LD_PRELOAD", | ||
| "LD_LIBRARY_PATH", | ||
| "DYLD_INSERT_LIBRARIES", | ||
| "DYLD_LIBRARY_PATH", | ||
| "BASH_ENV", | ||
| "ENV", | ||
| "CDPATH", | ||
| "IFS", | ||
| "PATH", | ||
| "HOME", | ||
| "USER", | ||
| "SHELL", | ||
| "RUST_LOG", | ||
| ]; |
There was a problem hiding this comment.
The DANGEROUS_ENV_VARS denylist is missing several other dangerous variables that could affect container behavior: PYTHONPATH, NODE_PATH, PERL5LIB, RUBYLIB, GOPATH, CLASSPATH, LD_AUDIT, JAVA_TOOL_OPTIONS, MAVEN_OPTS. Consider expanding the denylist to include these additional hijack vectors, or document why the current set is considered sufficient.
- Document CONNECT tunnel task lifecycle (timeout is the cleanup mechanism) - Remove secret names from error-level credential logs to prevent leaking - Expand DANGEROUS_ENV_VARS denylist with language runtime hijack vectors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
# Conflicts: # src/agent/agent_loop.rs
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let limit = params.get("limit").and_then(|v| v.as_i64()).unwrap_or(50); | ||
|
|
||
| let events = self | ||
| .store | ||
| .list_job_events(job_id, Some(limit)) | ||
| .await | ||
| .map_err(|e| ToolError::ExecutionFailed(format!("failed to load job events: {}", e)))?; |
There was a problem hiding this comment.
The limit parameter in list_job_events is user-controlled (from the LLM via JobEventsTool) but uses parameterized queries, which is secure. However, there's no upper bound validation on the limit value. An attacker could request limit: 999999999 to load millions of events into memory, causing a DoS. Consider adding a reasonable maximum (e.g., 1000 events) and rejecting larger values in the tool's execute method.
There was a problem hiding this comment.
Addressing 4 new Copilot comments in 50d9b2f:
Job monitor startup observability (#2818812915): Promoted the startup log from debug to info level so monitor initialization is visible in production logs.
resolve_job_id prefix enumeration (#2818812925): Added a minimum 4-character prefix requirement. Combined with UUID randomness (4 hex chars = 65536 possibilities), this makes enumeration impractical.
credential_grants_json column size (#2818812940): Added a cap of 20 credential grants per job. Each grant is ~60 bytes of JSON, so max storage is well within TEXT column limits.
list_job_events unbounded limit (#2818812891): Clamped the limit parameter to 1..1000 to prevent memory abuse from large values.
| pub fn spawn_job_monitor( | ||
| job_id: Uuid, | ||
| mut event_rx: broadcast::Receiver<(Uuid, SseEvent)>, | ||
| inject_tx: mpsc::Sender<IncomingMessage>, | ||
| ) -> JoinHandle<()> { | ||
| let short_id = job_id.to_string()[..8].to_string(); | ||
|
|
||
| tokio::spawn(async move { | ||
| tracing::debug!(job_id = %short_id, "Job monitor started"); | ||
|
|
||
| loop { | ||
| match event_rx.recv().await { | ||
| Ok((ev_job_id, event)) => { | ||
| if ev_job_id != job_id { | ||
| continue; | ||
| } | ||
|
|
||
| match event { | ||
| SseEvent::JobMessage { role, content, .. } if role == "assistant" => { | ||
| let msg = IncomingMessage::new( | ||
| "job_monitor", | ||
| "system", | ||
| format!("[Job {}] Claude Code: {}", short_id, content), | ||
| ); | ||
| if inject_tx.send(msg).await.is_err() { | ||
| tracing::debug!( | ||
| job_id = %short_id, | ||
| "Inject channel closed, stopping monitor" | ||
| ); | ||
| break; | ||
| } | ||
| } | ||
| SseEvent::JobResult { status, .. } => { | ||
| let msg = IncomingMessage::new( | ||
| "job_monitor", | ||
| "system", | ||
| format!( | ||
| "[Job {}] Container finished (status: {})", | ||
| short_id, status | ||
| ), | ||
| ); | ||
| let _ = inject_tx.send(msg).await; | ||
| tracing::debug!( | ||
| job_id = %short_id, | ||
| status = %status, | ||
| "Job monitor exiting (job finished)" | ||
| ); | ||
| break; | ||
| } | ||
| _ => { | ||
| // Skip tool_use, tool_result, status events | ||
| } | ||
| } | ||
| } | ||
| Err(broadcast::error::RecvError::Lagged(n)) => { | ||
| tracing::warn!( | ||
| job_id = %short_id, | ||
| skipped = n, | ||
| "Job monitor lagged, some events were dropped" | ||
| ); | ||
| } | ||
| Err(broadcast::error::RecvError::Closed) => { | ||
| tracing::debug!( | ||
| job_id = %short_id, | ||
| "Broadcast channel closed, stopping monitor" | ||
| ); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
The spawn_job_monitor function does not track or return the JoinHandle anywhere after spawning the background task. While the documentation explains this is intentional (fire-and-forget with lifetime bound to channels), there's no mechanism to detect if the monitor task panics during initialization or setup. Consider adding a tracing event at task startup success to aid debugging, or add a oneshot channel confirmation that the task successfully started before returning from this function.
| /// Resolve a job ID from a full UUID or a short prefix (like git short SHAs). | ||
| /// | ||
| /// Tries full UUID parse first. If that fails, treats the input as a hex prefix | ||
| /// and searches the context manager for a unique match. | ||
| async fn resolve_job_id(input: &str, context_manager: &ContextManager) -> Result<Uuid, ToolError> { | ||
| // Fast path: full UUID | ||
| if let Ok(id) = Uuid::parse_str(input) { | ||
| return Ok(id); | ||
| } | ||
|
|
||
| // Prefix match against known jobs | ||
| let input_lower = input.to_lowercase(); | ||
| let all_ids = context_manager.all_jobs().await; | ||
| let matches: Vec<Uuid> = all_ids | ||
| .into_iter() | ||
| .filter(|id| { | ||
| let hex = id.to_string().replace('-', ""); | ||
| hex.starts_with(&input_lower) | ||
| }) | ||
| .collect(); | ||
|
|
||
| match matches.len() { | ||
| 1 => Ok(matches[0]), | ||
| 0 => Err(ToolError::InvalidParameters(format!( | ||
| "no job found matching prefix '{}'", | ||
| input | ||
| ))), | ||
| n => Err(ToolError::InvalidParameters(format!( | ||
| "ambiguous prefix '{}' matches {} jobs, provide more characters", | ||
| input, n | ||
| ))), | ||
| } | ||
| } |
There was a problem hiding this comment.
The prefix matching uses replace('-', "") on the UUID hex representation, creating a 32-character hex string. However, if an attacker supplies a malicious prefix like "deadbeef" that happens to match multiple job IDs at the start of their hex representation, this could be used for timing attacks to enumerate valid job IDs. Consider adding rate limiting on resolve_job_id failures or requiring a minimum prefix length (e.g., 8 characters) to reduce the attack surface.
| // Serialize credential grants so restarts can reload them. | ||
| let credential_grants_json = match serde_json::to_string(&credential_grants) { | ||
| Ok(json) => json, | ||
| Err(e) => { | ||
| tracing::warn!( | ||
| "Failed to serialize credential grants for job {}: {}. \ | ||
| Grants will not survive a restart.", | ||
| job_id, | ||
| e | ||
| ); | ||
| String::from("[]") | ||
| } | ||
| }; |
There was a problem hiding this comment.
The credential_grants_json is serialized and stored in the agent_jobs.description column (originally designed for job descriptions). If a user requests many credentials, the JSON could exceed typical TEXT column limits (e.g., 65KB in some databases). Consider adding validation to reject requests with excessive credential grants (e.g., max 10-20 grants), or ensure the database column can handle large credential lists without truncation.
- Promote job monitor startup log to info level for observability - Require minimum 4-char prefix in resolve_job_id to limit enumeration - Cap credential grants at 20 per job to bound column storage - Clamp job events limit to 1..1000 to prevent memory abuse Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Resolve conflicts with main's skills system and benchmarking additions: - config.rs: kept both parse_oauth_access_token and SkillsConfig - job.rs: kept with_monitor_deps/with_secrets, adopted pub visibility - registry.rs: combined skill tool and job tool imports - worker/api.rs: kept both test suites Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The merge resolution dropped the closing `}` for `impl SkillsConfig`, causing a compilation error in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 41 out of 41 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/worker/claude_bridge.rs:706
- The
truncatefunction here duplicates the logic ofcrate::util::floor_char_boundary. For consistency with the rest of the codebase (seesrc/worker/runtime.rs,src/sandbox/manager.rs,src/tools/builtin/shell.rs), consider using the shared utility instead:
fn truncate(s: &str, max_len: usize) -> &str {
if s.len() <= max_len {
s
} else {
let end = crate::util::floor_char_boundary(s, max_len);
&s[..end]
}
}This makes the codebase more maintainable by avoiding duplicate implementations of the same UTF-8 boundary-finding logic.
src/config.rs:1677
- The closing brace for
impl SkillsConfigis missing. Theresolve()function (lines 1660-1676) is inside the impl block, but line 1677 starts with a doc comment for a standalone function without closing the impl block first. This will cause a compilation error.
Add a closing brace } after line 1676 and before the doc comment at line 1678.
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let mut resp = Response::new( | ||
| Full::new(Bytes::from("Internal error")) | ||
| .map_err(|_| unreachable!()) | ||
| .boxed(), | ||
| ); | ||
| *resp.status_mut() = StatusCode::INTERNAL_SERVER_ERROR; | ||
| resp | ||
| }) |
There was a problem hiding this comment.
The copy_dir_recursive function uses map_err(|_| unreachable!()) when wrapping errors for the BoxBody type. However, the error type here is Infallible from hyper::body::Full, which truly cannot error. The unreachable!() will never execute, but using it as an error mapper is misleading. Consider using a more explicit pattern like .map_err(|e| match e {}) to make it clear this branch is impossible, or add a comment explaining why unreachable!() is safe here.
| if let Some(ref text) = block.text.as_deref().filter(|t| !t.is_empty()) | ||
| { |
There was a problem hiding this comment.
The filtering expression block.text.as_deref().filter(|t| !t.is_empty()) converts Option<String> to Option<&str>, then filters out empty strings. However, the as_deref() call is unnecessary here since Option<String> already implements Deref<Target=str>. You can simplify this to:
if let Some(text) = block.text.as_ref().filter(|t| !t.is_empty()) {This is clearer and avoids the extra indirection.
… sandbox workflows (nearai#57) * feat: Add PR review tools, job monitor, and channel injection for E2E sandbox workflows Adds JobEventsTool and JobPromptTool so the main agent can read container event logs and send follow-up prompts to running Claude Code sessions. A background JobMonitor forwards container assistant messages into the agent loop via a new inject channel on ChannelManager. CreateJobTool now accepts a project_dir parameter for mounting existing cloned repos into containers, and spawns the monitor automatically for async jobs. Also: Dockerfile bumped to Rust 1.88 (rig-core needs let chains), GITHUB_TOKEN forwarded into containers for gh CLI auth, and truncate() fixed for multi-byte char boundary panics. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: Address PR nearai#57 review comments (IDOR, Dockerfile, truncate, logging) - Add ownership checks to JobEventsTool and JobPromptTool via ContextManager to prevent users from accessing other users' jobs (IDOR) - Combine Dockerfile gh CLI install into single apt-get layer - Handle truncate() edge case when max falls inside first multi-byte char - Log actual count of registered job management tools - Document fire-and-forget job monitor lifecycle - Add tests for ownership rejection and schema validation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: Replace hardcoded GITHUB_TOKEN with on-demand credential delivery Containers now fetch credentials via authenticated GET /worker/{id}/credentials endpoint instead of receiving them baked into env vars at creation time. Secrets are decrypted from SecretsStore on demand, scoped per-job via CredentialGrant, and revoked automatically when the job completes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: Address sandbox audit findings (CONNECT tunnel, readonly_rootfs, type consolidation) - Implement real CONNECT tunnel with bidirectional TCP piping via hyper upgrade - Fix readonly_rootfs to apply for both ReadOnly and WorkspaceWrite policies - Consolidate duplicate CredentialMapping/CredentialLocation into secrets::types - Share reqwest::Client across proxy requests instead of per-request allocation - Store Docker connection and reuse across executions - Remove .unwrap() from proxy response builders with safe fallbacks - Add output truncation to direct (non-container) execution (64KB limit) - Delete dead src/tools/sandbox.rs (ToolSandbox never used) - Fix connect_docker error message to list all attempted socket paths - Update proxy credential injection to handle all CredentialLocation variants - Use glob-based host_patterns matching for credential lookup in proxy policy Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: Address PR nearai#57 review comments (IDOR, Dockerfile, truncate, logging) - Dockerfile: install curl+ca-certificates before fetching GitHub CLI GPG key - JobEventsTool/JobPromptTool: reject missing context (prevents IDOR bypass) - parse_credentials: validate env var names against denylist and pattern - resolve_project_dir: require explicit paths to exist before validation - Credential serving: lower log level from info to debug Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: Address orchestrator audit findings (constant-time auth, error handling, tests) - auth: constant-time token comparison via subtle::ConstantTimeEq - auth: replace hand-rolled hex_encode with std::fmt::Write fold - api: report_status now updates ContainerHandle (was a no-op) - api: log complete_job errors instead of silently discarding - job_manager: log Docker cleanup errors in stop_job/complete_job - job_manager: extract validate_bind_mount_path with proper error on missing home_dir and mandatory base dir creation before canonicalize - job_manager: cache Docker connection across operations - error: remove dead OrchestratorError::AuthFailed and ContainerTimeout - Add 13 new tests (prompt queue, credentials, events, status, paths) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use floor_char_boundary in sandbox manager truncate to prevent multi-byte panics String::truncate() panics when the index falls mid-way through a multi-byte UTF-8 character. Use the same floor_char_boundary utility already used in worker/runtime.rs and tools/builtin/shell.rs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: default base_url to private.near.ai for Responses API mode Session tokens only authenticate against private.near.ai, not cloud-api.near.ai. The default base_url now matches the api_mode: - Responses (session token): https://private.near.ai - ChatCompletions (API key): https://cloud-api.near.ai This broke when the multi-provider merge introduced cloud-api.near.ai as the unconditional default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use private.near.ai as default base URL for all API modes private.near.ai now supports both Responses and ChatCompletions endpoints, so there is no reason to route through cloud-api.near.ai. This also fixes session token auth which only works against private.near.ai. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: harden libSQL concurrency, fix Claude Code Docker auth and permissions Three fixes for the sandbox/Claude Code pipeline: 1. SQLite "database is locked": set WAL journal mode in migrations and PRAGMA busy_timeout=5000 on every connection across LibSqlBackend, LibSqlSecretsStore, and LibSqlWasmToolStore (~83 async call sites). 2. Claude Code container auth: extract OAuth token from macOS Keychain (or Linux ~/.claude/.credentials.json) at startup and inject via CLAUDE_CODE_OAUTH_TOKEN env var. Removes the broken bind-mount approach that failed on uid mismatch. 3. Claude Code tool permissions: wire CLAUDE_CODE_ALLOWED_TOOLS env var through to the worker binary (was hardcoded to empty vec), and expand defaults to include all standard tools (Read, Write, Edit, Glob, Grep, NotebookEdit, Bash, Task, WebFetch, WebSearch). Also adds --verbose flag to claude CLI (required with stream-json + -p), failover provider model switching, and nearai models endpoint fix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: stream event parsing, job ID prefix resolution, session renewal in list_models Three fixes for the Docker/gateway pipeline: 1. Claude Code stream event parsing (claude_bridge.rs): Rewrite ClaudeStreamEvent to match actual NDJSON format where content blocks are nested under message.content[], not at the top level. Add handler for "user" events (tool_result blocks) and emit result text as a "message" event so reviews appear in gateway activity view. 2. Job ID prefix resolution (job.rs): Add resolve_job_id() that accepts short hex prefixes (like git short SHAs) in addition to full UUIDs. The LLM sees truncated IDs in job monitor messages like "[Job f2854dd8]" and can now use them directly with job_status/cancel/events/prompt tools. 3. Session renewal in list_models (nearai.rs): list_models() now retries with OAuth renewal on 401, matching send_request()'s existing behavior. Previously it returned SessionExpired immediately, causing the setup wizard to fall back to defaults instead of prompting re-authentication. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: /model command now lists available models Previously /model with no args only showed the current model name. Now it fetches and displays all available models from the provider, marking the active one, so users can see what's available before switching with /model <name>. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR nearai#57 review findings (set_var UB, tunnel timeout, restart creds) - Replace unsafe `std::env::set_var` in worker runtime and Claude bridge with `Command::envs()` injection via a new `extra_env` field on `JobContext`, avoiding undefined behavior in the multi-threaded tokio runtime. - Add 30-minute timeout to CONNECT tunnel `copy_bidirectional` in the sandbox proxy to prevent stuck connections from leaking spawned tasks. - Persist credential grants (as JSON in the description column) on `SandboxJobRecord` so `jobs_restart_handler` can restore them instead of passing `vec![]`, which caused restarted containers to lose access to their original secrets. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address second round of PR nearai#57 review comments - Normalize host_patterns to lowercase in proxy policy matching - Push LIMIT into SQL for list_job_events (Database trait + both backends) - Remove unused was_explicit binding in job tool - Return 500 instead of 200 in make_response fallback path - Update copy_auth_from_mount docstring for env-var default - Use entry.file_type() instead of is_dir() to avoid following symlinks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address third round of PR nearai#57 review comments - Restore glob patterns in default_claude_code_allowed_tools (Bash -> Bash(*)) - Add tracing::warn for credential grant serialize/deserialize failures - Wrap extra_env in Arc<HashMap> to avoid deep cloning per tool call - Document unsupported credential locations (AuthorizationBasic, UrlPath) - Document TOCTOU window in validate_bind_mount_path - Expand doc comments on JobEventsTool and JobPromptTool Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address fourth round of PR nearai#57 review comments - Document CONNECT tunnel task lifecycle (timeout is the cleanup mechanism) - Remove secret names from error-level credential logs to prevent leaking - Expand DANGEROUS_ENV_VARS denylist with language runtime hijack vectors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address fifth round of PR nearai#57 review comments - Promote job monitor startup log to info level for observability - Require minimum 4-char prefix in resolve_job_id to limit enumeration - Cap credential grants at 20 per job to bound column storage - Clamp job events limit to 1..1000 to prevent memory abuse Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add missing closing brace for SkillsConfig impl block The merge resolution dropped the closing `}` for `impl SkillsConfig`, causing a compilation error in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
… sandbox workflows (nearai#57) * feat: Add PR review tools, job monitor, and channel injection for E2E sandbox workflows Adds JobEventsTool and JobPromptTool so the main agent can read container event logs and send follow-up prompts to running Claude Code sessions. A background JobMonitor forwards container assistant messages into the agent loop via a new inject channel on ChannelManager. CreateJobTool now accepts a project_dir parameter for mounting existing cloned repos into containers, and spawns the monitor automatically for async jobs. Also: Dockerfile bumped to Rust 1.88 (rig-core needs let chains), GITHUB_TOKEN forwarded into containers for gh CLI auth, and truncate() fixed for multi-byte char boundary panics. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: Address PR nearai#57 review comments (IDOR, Dockerfile, truncate, logging) - Add ownership checks to JobEventsTool and JobPromptTool via ContextManager to prevent users from accessing other users' jobs (IDOR) - Combine Dockerfile gh CLI install into single apt-get layer - Handle truncate() edge case when max falls inside first multi-byte char - Log actual count of registered job management tools - Document fire-and-forget job monitor lifecycle - Add tests for ownership rejection and schema validation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: Replace hardcoded GITHUB_TOKEN with on-demand credential delivery Containers now fetch credentials via authenticated GET /worker/{id}/credentials endpoint instead of receiving them baked into env vars at creation time. Secrets are decrypted from SecretsStore on demand, scoped per-job via CredentialGrant, and revoked automatically when the job completes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: Address sandbox audit findings (CONNECT tunnel, readonly_rootfs, type consolidation) - Implement real CONNECT tunnel with bidirectional TCP piping via hyper upgrade - Fix readonly_rootfs to apply for both ReadOnly and WorkspaceWrite policies - Consolidate duplicate CredentialMapping/CredentialLocation into secrets::types - Share reqwest::Client across proxy requests instead of per-request allocation - Store Docker connection and reuse across executions - Remove .unwrap() from proxy response builders with safe fallbacks - Add output truncation to direct (non-container) execution (64KB limit) - Delete dead src/tools/sandbox.rs (ToolSandbox never used) - Fix connect_docker error message to list all attempted socket paths - Update proxy credential injection to handle all CredentialLocation variants - Use glob-based host_patterns matching for credential lookup in proxy policy Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: Address PR nearai#57 review comments (IDOR, Dockerfile, truncate, logging) - Dockerfile: install curl+ca-certificates before fetching GitHub CLI GPG key - JobEventsTool/JobPromptTool: reject missing context (prevents IDOR bypass) - parse_credentials: validate env var names against denylist and pattern - resolve_project_dir: require explicit paths to exist before validation - Credential serving: lower log level from info to debug Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: Address orchestrator audit findings (constant-time auth, error handling, tests) - auth: constant-time token comparison via subtle::ConstantTimeEq - auth: replace hand-rolled hex_encode with std::fmt::Write fold - api: report_status now updates ContainerHandle (was a no-op) - api: log complete_job errors instead of silently discarding - job_manager: log Docker cleanup errors in stop_job/complete_job - job_manager: extract validate_bind_mount_path with proper error on missing home_dir and mandatory base dir creation before canonicalize - job_manager: cache Docker connection across operations - error: remove dead OrchestratorError::AuthFailed and ContainerTimeout - Add 13 new tests (prompt queue, credentials, events, status, paths) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use floor_char_boundary in sandbox manager truncate to prevent multi-byte panics String::truncate() panics when the index falls mid-way through a multi-byte UTF-8 character. Use the same floor_char_boundary utility already used in worker/runtime.rs and tools/builtin/shell.rs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: default base_url to private.near.ai for Responses API mode Session tokens only authenticate against private.near.ai, not cloud-api.near.ai. The default base_url now matches the api_mode: - Responses (session token): https://private.near.ai - ChatCompletions (API key): https://cloud-api.near.ai This broke when the multi-provider merge introduced cloud-api.near.ai as the unconditional default. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use private.near.ai as default base URL for all API modes private.near.ai now supports both Responses and ChatCompletions endpoints, so there is no reason to route through cloud-api.near.ai. This also fixes session token auth which only works against private.near.ai. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: harden libSQL concurrency, fix Claude Code Docker auth and permissions Three fixes for the sandbox/Claude Code pipeline: 1. SQLite "database is locked": set WAL journal mode in migrations and PRAGMA busy_timeout=5000 on every connection across LibSqlBackend, LibSqlSecretsStore, and LibSqlWasmToolStore (~83 async call sites). 2. Claude Code container auth: extract OAuth token from macOS Keychain (or Linux ~/.claude/.credentials.json) at startup and inject via CLAUDE_CODE_OAUTH_TOKEN env var. Removes the broken bind-mount approach that failed on uid mismatch. 3. Claude Code tool permissions: wire CLAUDE_CODE_ALLOWED_TOOLS env var through to the worker binary (was hardcoded to empty vec), and expand defaults to include all standard tools (Read, Write, Edit, Glob, Grep, NotebookEdit, Bash, Task, WebFetch, WebSearch). Also adds --verbose flag to claude CLI (required with stream-json + -p), failover provider model switching, and nearai models endpoint fix. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: stream event parsing, job ID prefix resolution, session renewal in list_models Three fixes for the Docker/gateway pipeline: 1. Claude Code stream event parsing (claude_bridge.rs): Rewrite ClaudeStreamEvent to match actual NDJSON format where content blocks are nested under message.content[], not at the top level. Add handler for "user" events (tool_result blocks) and emit result text as a "message" event so reviews appear in gateway activity view. 2. Job ID prefix resolution (job.rs): Add resolve_job_id() that accepts short hex prefixes (like git short SHAs) in addition to full UUIDs. The LLM sees truncated IDs in job monitor messages like "[Job f2854dd8]" and can now use them directly with job_status/cancel/events/prompt tools. 3. Session renewal in list_models (nearai.rs): list_models() now retries with OAuth renewal on 401, matching send_request()'s existing behavior. Previously it returned SessionExpired immediately, causing the setup wizard to fall back to defaults instead of prompting re-authentication. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: /model command now lists available models Previously /model with no args only showed the current model name. Now it fetches and displays all available models from the provider, marking the active one, so users can see what's available before switching with /model <name>. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address PR nearai#57 review findings (set_var UB, tunnel timeout, restart creds) - Replace unsafe `std::env::set_var` in worker runtime and Claude bridge with `Command::envs()` injection via a new `extra_env` field on `JobContext`, avoiding undefined behavior in the multi-threaded tokio runtime. - Add 30-minute timeout to CONNECT tunnel `copy_bidirectional` in the sandbox proxy to prevent stuck connections from leaking spawned tasks. - Persist credential grants (as JSON in the description column) on `SandboxJobRecord` so `jobs_restart_handler` can restore them instead of passing `vec![]`, which caused restarted containers to lose access to their original secrets. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address second round of PR nearai#57 review comments - Normalize host_patterns to lowercase in proxy policy matching - Push LIMIT into SQL for list_job_events (Database trait + both backends) - Remove unused was_explicit binding in job tool - Return 500 instead of 200 in make_response fallback path - Update copy_auth_from_mount docstring for env-var default - Use entry.file_type() instead of is_dir() to avoid following symlinks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address third round of PR nearai#57 review comments - Restore glob patterns in default_claude_code_allowed_tools (Bash -> Bash(*)) - Add tracing::warn for credential grant serialize/deserialize failures - Wrap extra_env in Arc<HashMap> to avoid deep cloning per tool call - Document unsupported credential locations (AuthorizationBasic, UrlPath) - Document TOCTOU window in validate_bind_mount_path - Expand doc comments on JobEventsTool and JobPromptTool Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address fourth round of PR nearai#57 review comments - Document CONNECT tunnel task lifecycle (timeout is the cleanup mechanism) - Remove secret names from error-level credential logs to prevent leaking - Expand DANGEROUS_ENV_VARS denylist with language runtime hijack vectors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address fifth round of PR nearai#57 review comments - Promote job monitor startup log to info level for observability - Require minimum 4-char prefix in resolve_job_id to limit enumeration - Cap credential grants at 20 per job to bound column storage - Clamp job events limit to 1..1000 to prevent memory abuse Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: add missing closing brace for SkillsConfig impl block The merge resolution dropped the closing `}` for `impl SkillsConfig`, causing a compilation error in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Adds JobEventsTool and JobPromptTool so the main agent can read container event logs and send follow-up prompts to running Claude Code sessions. A background JobMonitor forwards container assistant messages into the agent loop via a new inject channel on ChannelManager.
CreateJobTool now accepts a project_dir parameter for mounting existing cloned repos into containers, and spawns the monitor automatically for async jobs.
Also: Dockerfile bumped to Rust 1.88 (rig-core needs let chains), GITHUB_TOKEN forwarded into containers for gh CLI auth, and truncate() fixed for multi-byte char boundary panics.