Fix sandbox tests in CI mode#11
Fix sandbox tests in CI mode#11soodoku wants to merge 1 commit intoclaude/add-code-sandboxing-018YT8W7GWG2yyi4JKSvpQASfrom
Conversation
- Apply resource limits in direct execution mode - Fix command validation logic to properly handle echo and dd commands - Echo arguments are not file paths, only check redirections in shell scripts - Parse dd command's of= parameter for write validation This fixes 3 failing tests: 1. should execute simple command in sandbox 2. Should enforce file size limit 3. Should allow operations within limits
Summary of ChangesHello @soodoku, 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 addresses critical issues in the sandbox environment by ensuring that resource limits are properly enforced during direct command execution. It refines the command validation process, specifically for Highlights
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 several important fixes for running sandbox tests in CI mode. The changes correctly apply resource limits during direct execution and significantly improve the command validation logic. Specifically, the validation for dd and echo commands is now handled correctly, which resolves failing tests. The code is well-structured, and the fixes are directly addressing the issues described. I have one minor suggestion to reduce some code duplication for better maintainability, but overall this is a great improvement.
| if (cmd === 'touch') { | ||
| // touch creates/updates files | ||
| for (const arg of args) { | ||
| if (!arg.startsWith('-') && !this.isWriteAllowed(arg)) { | ||
| return { allowed: false, reason: `Write access denied to ${arg}` }; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (cmd === 'tee') { | ||
| // tee writes to files | ||
| for (const arg of args) { | ||
| if (!arg.startsWith('-') && !this.isWriteAllowed(arg)) { | ||
| return { allowed: false, reason: `Write access denied to ${arg}` }; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
To improve maintainability and reduce code duplication, the validation logic for touch and tee can be combined, as they share the same argument parsing logic for identifying file paths.
| if (cmd === 'touch') { | |
| // touch creates/updates files | |
| for (const arg of args) { | |
| if (!arg.startsWith('-') && !this.isWriteAllowed(arg)) { | |
| return { allowed: false, reason: `Write access denied to ${arg}` }; | |
| } | |
| } | |
| } | |
| if (cmd === 'tee') { | |
| // tee writes to files | |
| for (const arg of args) { | |
| if (!arg.startsWith('-') && !this.isWriteAllowed(arg)) { | |
| return { allowed: false, reason: `Write access denied to ${arg}` }; | |
| } | |
| } | |
| } | |
| if (['touch', 'tee'].includes(cmd)) { | |
| // touch and tee write to files | |
| for (const arg of args) { | |
| if (!arg.startsWith('-') && !this.isWriteAllowed(arg)) { | |
| return { allowed: false, reason: `Write access denied to ${arg}` }; | |
| } | |
| } | |
| } |
2 participants
This fixes 3 failing tests: