-
Notifications
You must be signed in to change notification settings - Fork 0
refactor: 'run_bun' tool adds a security sandbox #29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ PR Review
The PR introduces a comprehensive security sandbox for the run_bun tool and implements a well-designed hook/middleware system for session lifecycle events. The changes appear to maintain good separation of concerns and error handling.
2 issues detected:
🧹 Maintainability - Inconsistent API contract for hook payload properties reduces developer experience and predictability.
Details: The
onFinalhook calls are inconsistent about when thestepproperty is included in the payload. Some calls (lines 191, 226, 369) omitstepwhile others (line 282) include it. This inconsistency could confuse middleware authors about when to expect step context.
File:packages/core/src/runtime/session.ts🧹 Maintainability - Silent test skipping can lead to false confidence about test coverage and functionality verification. 🛠️
Details: Tests are silently skipped when sandbox tools (bwrap/sandbox-exec) are not available, which could mask security functionality issues in CI/CD environments where these tools might not be installed.
File:packages/tools/src/tools/run_bun.test.ts (21-21)
🛠️ A suggested code correction is included in the review comments.
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Review using Guidelines Learn how
| tempDir = await mkdtemp(join(tmpdir(), 'memo-run-bun-test-')) | ||
| process.env.TMPDIR = tempDir | ||
| }) | ||
| const describeRunBun = hasSandbox ? describe : describe.skip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Maintainability - Silent Test Skipping: Add explicit logging when tests are skipped due to missing sandbox tools, or ensure CI environments have the required dependencies installed.
| const describeRunBun = hasSandbox ? describe : describe.skip | |
| const describeRunBun = hasSandbox ? describe : (() => { | |
| console.warn('Skipping run_bun tests: sandbox tools (bwrap/sandbox-exec) not available') | |
| return describe.skip | |
| })() |
Is this review accurate? Use 👍 or 👎 to rate it
If you want to tell us more, use /gs feedback e.g. /gs feedback this review doesn't make sense, I disagree, and it keeps repeating over and over
✨ PR Description
Purpose: Refactor run_bun tool to execute code within platform-specific sandboxes and add lifecycle hook system to enable middleware-based session monitoring and extensions.
Main changes:
Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using.
💡 Tip: You can customize your AI Description using Guidelines Learn how