feat: add configurable sandbox mode setting #282
Conversation
Add a global setting to enable/disable sandbox mode for Claude Agent SDK. This allows users to control sandbox behavior based on their authentication setup and system compatibility. Changes: - Add enableSandboxMode to GlobalSettings (default: true) - Add sandbox mode checkbox in Claude settings UI - Wire up setting through app store and settings service - Update createChatOptions and createAutoModeOptions to use setting - Add getEnableSandboxModeSetting helper function - Remove hardcoded sandbox configuration from ClaudeProvider - Add detailed logging throughout agent execution flow The sandbox mode requires API key or OAuth token authentication. Users experiencing issues with CLI-only auth can disable it in settings. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a global Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (UI)
participant UI as Settings View
participant Store as App Store
participant API as Server API
participant Settings as SettingsService
participant Provider as Claude Provider
Note over User,UI: User toggles "Enable Sandbox Mode"
User->>UI: click toggle
UI->>Store: setEnableSandboxMode(enabled)
Store->>API: syncSettingsToServer({ enableSandboxMode })
API->>Settings: updateGlobalSettings({ enableSandboxMode })
Settings-->>API: ack
Note over API,Provider: When invoking agent/auto-mode later
API->>Settings: getEnableSandboxModeSetting()
Settings-->>API: return enableSandboxMode
API->>Provider: executeQuery(..., sdkOptions { enableSandboxMode }, ExecuteOptions { sandbox })
Provider-->>API: stream events (verbose logs)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @casiusss, 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 empowers users with greater control over the Claude Agent SDK's sandbox behavior by introducing a configurable setting. This allows users to adapt the agent's operation based on their authentication methods or system environments, particularly addressing scenarios where CLI-only authentication might conflict with sandboxing. Alongside this new feature, comprehensive logging has been integrated throughout the agent's execution path, significantly improving observability and diagnostic capabilities for developers and users alike. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 a new 'Sandbox Mode' feature, allowing users to enable or disable bash command isolation for enhanced security. The changes involve adding an enableSandboxMode setting to the application's global settings, UI store, and SDK options configuration. The UI now includes a checkbox for controlling this setting, accompanied by a warning about potential agent hanging issues on certain systems. The server-side logic conditionally applies the sandbox configuration to the Claude Agent SDK options based on this setting, and the ClaudeProvider's permissionMode was changed from 'acceptEdits' to 'default'. Additionally, the pull request includes a significant amount of new console.log statements across apps/server/src/index.ts, apps/server/src/providers/claude-provider.ts, apps/server/src/routes/agent/routes/send.ts, and apps/server/src/services/agent-service.ts for debugging purposes, including a watchdog timer in the ClaudeProvider. Review comments highlight an inconsistency in the default value for enableSandboxMode in apps/server/src/lib/settings-helpers.ts (where it defaults to false instead of true like in other parts of the application) and point out that the numerous console.log statements are debugging-related and should be removed or replaced with a proper logging mechanism for production.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
apps/server/src/index.ts (1)
193-217: WebSocket observability improvements look good; consider gating verbosityThe added logs (connection state, event metadata, payload keys, message length, and non-open warnings) are useful and non-invasive. In higher-traffic or hosted deployments you may eventually want to route these through whatever structured logger you standardize on or guard them behind a debug flag to avoid noisy stdout, but functionally this is fine as-is.
Also applies to: 227-227
apps/server/src/routes/agent/routes/send.ts (1)
22-61: Send handler logging is helpful; consider using the shared loggerThe added console logging around request receipt, validation, background errors, and synchronous errors improves traceability without changing behavior. Since a
loggerfromcreateLogger('Agent')already exists in this module, you may want to route these new messages through it for consistent formatting and centralized control in the future.apps/server/src/services/auto-mode-service.ts (1)
35-39: Auto‑mode sandbox flag propagation is correctImporting
getEnableSandboxModeSettingand wiring its result through tocreateAutoModeOptionscleanly ties auto‑mode execution to the global sandbox setting. Combined with the new defaults in global settings, this should give the intended “sandbox on by default, user‑controllable” behavior.If you ever find settings reads becoming a hot path, you could cache
enableSandboxModeperAutoModeServiceinstance and invalidate on settings changes, but that’s not required for correctness here.Also applies to: 1834-1850
apps/ui/src/components/views/settings-view/claude/claude-md-settings.tsx (1)
13-26: Stale JSDoc example.The usage example only shows
autoLoadClaudeMdprops but doesn't include the newenableSandboxModeandonEnableSandboxModeChangeprops.🔎 Proposed fix
/** * ClaudeMdSettings Component * - * UI control for the autoLoadClaudeMd setting which enables automatic loading - * of project instructions from .claude/CLAUDE.md files via the Claude Agent SDK. + * UI controls for Claude SDK settings including automatic loading of project + * instructions from .claude/CLAUDE.md files and sandbox mode configuration. * * Usage: * ```tsx * <ClaudeMdSettings * autoLoadClaudeMd={autoLoadClaudeMd} * onAutoLoadClaudeMdChange={setAutoLoadClaudeMd} + * enableSandboxMode={enableSandboxMode} + * onEnableSandboxModeChange={setEnableSandboxMode} * /> * ``` */apps/server/src/providers/claude-provider.ts (2)
116-141: Redundant logging - SDK options already logged above.Lines 74-81 already log the SDK options summary. This second detailed log at lines 117-135 duplicates much of the same information. Consider consolidating into a single log statement.
149-176: Consider making the watchdog configurable or removing for production.The watchdog timer is useful for debugging streaming issues but adds overhead. The 10-second threshold before warning seems reasonable, but this diagnostic code should ideally be gated behind a debug/development flag for production builds.
apps/server/src/services/agent-service.ts (3)
331-338: High-volume per-message logging may impact performance.Logging every stream message with metadata could generate significant log volume during long conversations. Consider using a debug log level or sampling for production.
🔎 Proposed fix - log level gating
for await (const msg of stream) { messageCount++; - console.log(`[AgentService] Stream message #${messageCount}:`, { - type: msg.type, - subtype: (msg as any).subtype, - hasContent: !!(msg as any).message?.content, - session_id: msg.session_id, - }); + if (process.env.DEBUG_AGENT_STREAM) { + console.log(`[AgentService] Stream message #${messageCount}:`, { + type: msg.type, + subtype: (msg as any).subtype, + hasContent: !!(msg as any).message?.content, + session_id: msg.session_id, + }); + }
147-166: Consider consolidating entry-point logging.The initial log at lines 147-153 and the session-found log at lines 161-166 could be combined into a single diagnostic log to reduce verbosity.
821-828: Event emission logging adds overhead on every event.Logging every event emission with metadata (lines 821-827) may be excessive for production. Consider gating behind a debug flag.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/server/src/index.tsapps/server/src/lib/sdk-options.tsapps/server/src/lib/settings-helpers.tsapps/server/src/providers/claude-provider.tsapps/server/src/routes/agent/routes/send.tsapps/server/src/services/agent-service.tsapps/server/src/services/auto-mode-service.tsapps/ui/src/components/views/settings-view.tsxapps/ui/src/components/views/settings-view/claude/claude-md-settings.tsxapps/ui/src/store/app-store.tslibs/types/src/settings.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/server/src/services/auto-mode-service.ts (2)
apps/server/src/lib/settings-helpers.ts (1)
getEnableSandboxModeSetting(56-74)apps/server/src/lib/sdk-options.ts (1)
createAutoModeOptions(360-382)
apps/ui/src/store/app-store.ts (1)
apps/ui/src/hooks/use-settings-migration.ts (1)
syncSettingsToServer(195-242)
apps/server/src/services/agent-service.ts (1)
apps/server/src/lib/settings-helpers.ts (1)
getEnableSandboxModeSetting(56-74)
🔇 Additional comments (7)
libs/types/src/settings.ts (1)
305-305: GlobalenableSandboxModewiring and defaults are consistentAdding
enableSandboxMode?: booleantoGlobalSettingsand defaulting it totrueinDEFAULT_GLOBAL_SETTINGSaligns with the intended behavior, and the existingSettingsService.getGlobalSettingsmerge logic will correctly backfill this for older settings files.Also applies to: 464-465
apps/ui/src/components/views/settings-view.tsx (1)
53-55: Claude settings view correctly surfaces sandbox toggleThe settings view now cleanly pulls
enableSandboxModeandsetEnableSandboxModefrom the store and passes them intoClaudeMdSettings, consistent with howautoLoadClaudeMdis handled. This should make the new sandbox option visible and controllable from the UI without additional plumbing.Also applies to: 111-115
apps/server/src/lib/settings-helpers.ts (1)
48-74: Fallback semantics are properly handled in production codeAll production server entry points (apps/server/src/index.ts) correctly instantiate both
AutoModeServiceandAgentServicewith a non-nullSettingsService. Test code intentionally omits it, and thefalsefallback is an appropriate safe default for test contexts. The helper is well-designed for its use cases.apps/server/src/lib/sdk-options.ts (1)
220-222: Tests expect sandbox to be enabled without passingenableSandboxModeflagThe production callers in
agent-service.tsandauto-mode-service.tscorrectly passenableSandboxModeto bothcreateChatOptions()andcreateAutoModeOptions(), so production behavior is safe. However, the tests insdk-options.test.ts(lines 182, 222) don't pass this flag but still assert thatsandboxis set in the returned options. Since the implementation conditionally spreads the sandbox block only whenconfig.enableSandboxModeis truthy, these tests are broken and should either passenableSandboxMode: trueor remove the sandbox assertion.Also applies to: 320-321, 339-345, 357-358, 373-378
apps/ui/src/components/views/settings-view/claude/claude-md-settings.tsx (1)
83-108: LGTM!The sandbox mode checkbox implementation is well-structured with clear labeling, appropriate iconography, and helpful warning text for users who may experience issues.
apps/server/src/providers/claude-provider.ts (1)
162-172: LGTM on stream handling structure.The try/finally pattern ensures the watchdog interval is properly cleaned up, and message counting provides useful diagnostics.
apps/server/src/services/agent-service.ts (1)
242-246: LGTM on sandbox mode integration.The
enableSandboxModesetting is correctly retrieved from the settings service and passed through tocreateChatOptions. The helper function's error handling (throwing on failure) ensures failures are visible rather than silently defaulting.
The default value should be 'true' to match the defaults in libs/types/src/settings.ts and apps/ui/src/store/app-store.ts. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/server/src/lib/settings-helpers.ts (1)
61-61: Consider upgrading to structured logging.The function uses
console.logandconsole.error(lines 61, 68, 71), which is consistent with the existinggetAutoLoadClaudeMdSettingfunction in this file. However, using a proper logger (e.g.,createLoggerfrom@automaker/utilsat appropriate levels) would provide better control over log output in production environments.This could be addressed file-wide in a follow-up refactor.
Also applies to: 68-68, 71-71
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/server/src/lib/settings-helpers.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/server/src/lib/settings-helpers.ts (1)
apps/server/src/services/settings-service.ts (1)
SettingsService(104-583)
🔇 Additional comments (1)
apps/server/src/lib/settings-helpers.ts (1)
48-74: LGTM! Default value correctly set totrue.The implementation correctly addresses the past review concern about the default value. Line 67 now uses
?? true, which is consistent with the defaults inlibs/types/src/settings.tsandapps/ui/src/store/app-store.ts. The function follows the established pattern fromgetAutoLoadClaudeMdSettingand appropriately reads only global settings (not project settings) for this security-related configuration.
The sandbox configuration was set in createChatOptions() and createAutoModeOptions(), but was never passed to the ClaudeProvider. This caused the sandbox to never actually be enabled. Changes: - Add sandbox field to ExecuteOptions interface - Pass sandbox config from AgentService to provider - Pass sandbox config from AutoModeService to provider - Forward sandbox config in ClaudeProvider to SDK options Now the sandbox configuration from settings is properly used. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/server/src/services/agent-service.ts (1)
147-412: [Same as previous review] Remove or gate verbose debugging logs.The extensive
console.logstatements throughoutsendMessageand the streaming loop are not suitable for production. Past review comments flagged this same issue.These logs should either be:
- Removed entirely, or
- Replaced with a structured logger at appropriate levels (debug/trace), or
- Gated behind a debug flag (e.g.,
if (process.env.DEBUG_AGENT) ...)The current volume of logging could impact performance and clutter production logs.
apps/server/src/providers/claude-provider.ts (1)
26-188: [Same as previous review] Remove excessive debugging logs and sensitive environment details.The extensive
console.logstatements throughout this file, including sensitive environment information (API key length, HOME, USER, CWD at lines 109-116), are not suitable for production. This matches previous review feedback.These logs should be:
- Removed entirely, or
- Wrapped behind a strict debug flag:
if (process.env.DEBUG_CLAUDE) ...- Sensitive details (API key, environment paths) should never be logged
The watchdog timer (lines 151-160) and per-message logging (lines 167-173) create significant log volume that could impact performance and obscure real issues in production.
🧹 Nitpick comments (1)
apps/server/src/providers/types.ts (1)
25-38: Consider importing from shared types to eliminate duplication.The
ExecuteOptionsinterface here duplicates the definition inlibs/types/src/provider.ts. Having identical type definitions in multiple locations creates maintenance risk—changes to one may not be reflected in the other.Consider importing from
@automaker/typesinstead:import type { ExecuteOptions } from '@automaker/types';This ensures a single source of truth for the type definition.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
apps/server/src/providers/claude-provider.tsapps/server/src/providers/types.tsapps/server/src/services/agent-service.tsapps/server/src/services/auto-mode-service.tslibs/types/src/provider.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/server/src/providers/claude-provider.ts (2)
apps/server/src/providers/types.ts (1)
ProviderMessage(56-67)libs/types/src/provider.ts (1)
ProviderMessage(65-76)
apps/server/src/services/agent-service.ts (1)
apps/server/src/lib/settings-helpers.ts (1)
getEnableSandboxModeSetting(56-74)
apps/server/src/services/auto-mode-service.ts (2)
apps/server/src/lib/settings-helpers.ts (1)
getEnableSandboxModeSetting(56-74)apps/server/src/lib/sdk-options.ts (1)
createAutoModeOptions(360-382)
🔇 Additional comments (7)
libs/types/src/provider.ts (1)
46-46: LGTM: Clean type addition for sandbox configuration.The optional
sandboxfield with its nested structure is well-defined and follows TypeScript conventions. The type signature clearly distinguishes between required (enabled) and optional (autoAllowBashIfSandboxed) properties.apps/server/src/services/agent-service.ts (2)
20-24: LGTM: Import additions support sandbox mode feature.The imports correctly add
getEnableSandboxModeSettingneeded for the sandbox configuration functionality. The import is properly sourced fromsettings-helpers.ts.
242-246: LGTM: Sandbox mode integration is correct.The sandbox configuration is properly:
- Retrieved from global settings via
getEnableSandboxModeSetting- Passed through to SDK options via
createChatOptions- Forwarded to provider via
ExecuteOptions.sandboxThe flow correctly threads the setting from configuration through to execution. Based on learnings, the default of
truewhen the settings service is unavailable provides safe-by-default behavior.Also applies to: 272-272, 299-299
apps/server/src/providers/claude-provider.ts (1)
72-73: LGTM: Sandbox configuration is correctly forwarded.The conditional spreading properly forwards the
sandboxconfiguration fromoptions.sandboxtosdkOptionswhen present. This resolves the previous review concern about sandbox never being set.apps/server/src/services/auto-mode-service.ts (3)
35-39: LGTM: Import additions support sandbox mode.The addition of
getEnableSandboxModeSettingto the imports is correct and necessary for the sandbox configuration feature.
1841-1842: LGTM: Sandbox mode correctly integrated in runAgent.The sandbox configuration is properly implemented:
- Retrieved from global settings (lines 1841-1842)
- Passed to
createAutoModeOptions(line 1850)- Forwarded to
ExecuteOptions(line 1891)This follows the correct pattern for threading the setting through the execution flow.
Also applies to: 1850-1850, 1891-1891
1139-1156: Sandbox configuration in analyzeProject is correct for its use case.The
analyzeProjectmethod uses only read-only tools (['Read', 'Glob', 'Grep']), which don't require sandbox or code execution capabilities. Sandbox configuration is intentionally omitted because static analysis doesn't need it. Theundefinedvalue passed tosandboxinExecuteOptions(line 1156) is handled gracefully bycreateCustomOptions, which uses a conditional spread operator (...(config.sandbox && { sandbox: config.sandbox })) to exclude the field when not provided. This pattern is correct and requires no changes.
Address suggestions from gemini-code-assist and coderabbit-ai: Logging Improvements: - Remove excessive debug logging from ClaudeProvider - Remove sensitive environment variable logging (API key length, HOME, USER) - Remove verbose per-message stream logging from AgentService - Remove redundant SDK options logging - Remove watchdog timer logging (diagnostic tool) Documentation: - Update JSDoc example in ClaudeMdSettings to include sandbox props Persistence Fix: - Add enableSandboxMode to syncSettingsToServer updates object - Ensures sandbox setting is properly persisted to server storage This reduces log volume significantly while maintaining important error and state transition logging. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/server/src/services/agent-service.ts (1)
147-166: Consider using structured logging with levels.While these logs provide useful observability, the extensive use of
console.logstatements throughout the service may clutter production logs. Consider using a structured logger (e.g.,pino,winston) with appropriate log levels (debug,info,error) so log verbosity can be controlled via configuration.Example structured logging approach
For example, you could initialize a logger at the service level:
import pino from 'pino'; export class AgentService { private logger = pino({ level: process.env.LOG_LEVEL || 'info' }); // ... }Then replace console.log calls with level-appropriate logger calls:
this.logger.debug({ sessionId, messageLength: message?.length, model }, 'sendMessage called'); this.logger.error({ sessionId }, 'Session not found');
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/server/src/providers/claude-provider.tsapps/server/src/services/agent-service.tsapps/ui/src/components/views/settings-view/claude/claude-md-settings.tsxapps/ui/src/hooks/use-settings-migration.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ui/src/components/views/settings-view/claude/claude-md-settings.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/server/src/services/agent-service.ts (1)
apps/server/src/lib/settings-helpers.ts (1)
getEnableSandboxModeSetting(56-74)
🔇 Additional comments (8)
apps/ui/src/hooks/use-settings-migration.ts (1)
227-227: LGTM! Sandbox mode setting correctly added to sync.The addition of
enableSandboxModeto the settings sync payload follows the established pattern and is consistent with how other global settings are synchronized to the server.apps/server/src/providers/claude-provider.ts (3)
48-48: LGTM! Permission mode aligned with configurable settings.Changing to
'default'allows the sandbox configuration to drive the permission behavior rather than hard-coding'acceptEdits', which aligns with the PR's goal of making sandbox mode configurable.
56-57: LGTM! Sandbox configuration properly forwarded.The conditional spread correctly forwards sandbox configuration when provided, enabling the global
enableSandboxModesetting to control sandbox behavior at the SDK level.
91-92: LGTM! Enhanced error logging with stack traces.Adding the ERROR prefix and stack trace improves debuggability without exposing sensitive information. This is a good practice for production error handling.
apps/server/src/services/agent-service.ts (4)
20-24: LGTM! Imports properly added for sandbox configuration.The new imports support loading the
enableSandboxModesetting and managing Claude MD file handling, which are central to this PR's functionality.
242-246: LGTM! Sandbox mode setting properly loaded.The call to
getEnableSandboxModeSettingcorrectly retrieves the global sandbox configuration, which is then passed through to the SDK options.
272-272: LGTM! Sandbox setting threaded through SDK options.Passing
enableSandboxModetocreateChatOptionsensures the global setting influences the SDK configuration, which is the core behavior change in this PR.
299-299: LGTM! Sandbox configuration forwarded to provider.Passing
sdkOptions.sandboxto the provider'sExecuteOptionscompletes the configuration chain from global settings → SDK options → provider execution, ensuring sandbox mode is applied during agent queries.
Add comprehensive test coverage for sandbox mode configuration: - Added tests for enableSandboxMode=false for both createChatOptions and createAutoModeOptions - Added tests for enableSandboxMode not provided for both functions - Updated existing tests to pass enableSandboxMode=true where sandbox assertions exist This addresses the broken test assertions identified by coderabbit-ai review. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Updated tests to reflect changes made to sandbox mode implementation: 1. Changed permissionMode expectation from 'acceptEdits' to 'default' - ClaudeProvider now uses 'default' permission mode 2. Renamed test "should enable sandbox by default" to "should pass sandbox configuration when provided" - Sandbox is no longer enabled by default in the provider - Provider now forwards sandbox config only when explicitly provided via ExecuteOptions 3. Updated error handling test expectations - Now expects two console.error calls with new format - First call: '[ClaudeProvider] ERROR: executeQuery() error during execution:' - Second call: '[ClaudeProvider] ERROR stack:' with stack trace All 32 tests in claude-provider.test.ts now pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/server/tests/unit/providers/claude-provider.test.ts (1)
103-130: Test correctly validates sandbox forwarding, but consider adding complementary test.The test properly verifies that sandbox configuration is passed through to the SDK when provided. However, consider adding a complementary test to ensure that when the
sandboxoption is not provided (or whenenableSandboxModeis false), the sandbox configuration is absent from SDK options. This would confirm backward compatibility and proper conditional behavior.🔎 Suggested additional test case
it('should not include sandbox configuration when not provided', async () => { vi.mocked(sdk.query).mockReturnValue( (async function* () { yield { type: 'text', text: 'test' }; })() ); const generator = provider.executeQuery({ prompt: 'Test', cwd: '/test', }); await collectAsyncGenerator(generator); expect(sdk.query).toHaveBeenCalledWith({ prompt: 'Test', options: expect.not.objectContaining({ sandbox: expect.anything(), }), }); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/server/tests/unit/providers/claude-provider.test.ts
🔇 Additional comments (2)
apps/server/tests/unit/providers/claude-provider.test.ts (2)
249-263: LGTM! Improved error logging structure.The updated test expectations reflect better structured error logging with separate calls for the error message and stack trace, both with consistent
[ClaudeProvider]prefixes. This makes logs easier to filter and debug.
76-76: permissionMode is correctly set to 'default' and matches the implementation.The test expectation at line 76 is correct. The ClaudeProvider implementation in
apps/server/src/providers/claude-provider.tsexplicitly setspermissionMode: 'default'at line 48, and the test correctly asserts this behavior. This change is intentional and documented—using'default'prevents Claude from writing files to unexpected locations (see GitHub issue #149). Other routes can still use'acceptEdits'where appropriate through the SDK options builder.
Removed all debug console.log statements from agent-service.ts to avoid polluting production logs. This addresses code review feedback from gemini-code-assist. Removed debug logs for: - sendMessage() entry and session state - Event emissions (started, message, stream, complete) - Provider execution - SDK session ID capture - Tool use detection - Queue processing - emitAgentEvent() calls Kept console.error logs for actual errors (session not found, execution errors, etc.) as they are useful for troubleshooting. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
casiusss
force-pushed
the
feat/sandbox-mode-setting
branch
from
2741909 to
71c17e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/services/auto-mode-service.ts (1)
1154-1179: Add sandbox configuration to analyzeProject to match runAgent behavior.The
analyzeProjectmethod doesn't load or pass theenableSandboxModesetting tocreateCustomOptions, whilerunAgentloads this setting and passes it tocreateAutoModeOptions. This causes project analysis to run without sandbox configuration even if enabled globally.Add the following to
analyzeProject(before callingcreateCustomOptions):
- Load
enableSandboxModeviagetEnableSandboxModeSetting(this.settingsService, '[AutoMode]')- Build the sandbox object similar to how
createAutoModeOptionsdoes (convert boolean to{ enabled: boolean })- Pass the sandbox object to
createCustomOptionsThis ensures project analysis respects the same global sandbox setting that feature execution uses.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/server/src/index.tsapps/server/src/services/auto-mode-service.tsapps/ui/src/main.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/src/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/server/src/services/auto-mode-service.ts (2)
apps/server/src/lib/settings-helpers.ts (1)
getEnableSandboxModeSetting(56-74)apps/server/src/lib/sdk-options.ts (1)
createAutoModeOptions(360-382)
🔇 Additional comments (6)
apps/server/src/services/auto-mode-service.ts (6)
35-39: LGTM! Import added correctly.The new import for
getEnableSandboxModeSettingis properly added and will be used to load the global sandbox setting later in the execution flow.
499-509: LGTM! Diagnostic logging for concurrent task tracking.The TASK_START logging provides valuable diagnostic information including concurrent task count, memory usage, and timestamps. This will help correlate Electron crashes with resource usage patterns during concurrent auto-mode execution.
681-694: LGTM! Comprehensive task completion logging.The TASK_END logging effectively closes the task lifecycle tracking loop, capturing duration, remaining concurrent tasks, memory state, and pending approvals. The inclusion of
pendingApprovalsis particularly useful for diagnosing cases where tasks may be blocked waiting for user input.
1864-1874: LGTM! Sandbox setting properly loaded and propagated.The sandbox configuration flow is correctly implemented:
- Global
enableSandboxModesetting is loaded viagetEnableSandboxModeSetting- The setting is passed to
createAutoModeOptionswhich conditionally includes sandbox configuration- The resulting
sdkOptionswill contain the appropriate sandbox config based on the global setting
1905-1915: LGTM! Sandbox configuration correctly threaded through ExecuteOptions.The
sandboxfield fromsdkOptionsis properly passed toExecuteOptions, completing the configuration chain from global settings → SDK options → execution options. The provider will receive the correct sandbox configuration for enforcement.
2408-2416: LGTM! Tool execution logging for crash correlation.The TOOL_EXEC logging captures each tool execution with concurrent task count and memory usage, enabling correlation between specific tool usage patterns and Electron crashes. While this may be verbose, it's consistent with the diagnostic logging strategy throughout this PR.
apps/ui/src/main.ts
Outdated
| // ============================================ | ||
| // Electron Network Service Stability Flags | ||
| // ============================================ | ||
| // Add flags to prevent network service crashes | ||
| // See: https://github.com/electron/electron/issues/18397 | ||
| console.log('[Electron] Applying network stability flags...'); | ||
|
|
||
| // Disable site isolation to reduce process overhead | ||
| app.commandLine.appendSwitch('disable-site-isolation-trials'); | ||
|
|
||
| // Use in-process network service (more stable but uses main process) | ||
| app.commandLine.appendSwitch('disable-features', 'OutOfBlinkCors'); | ||
|
|
||
| // Reduce renderer process limits to prevent resource exhaustion | ||
| app.commandLine.appendSwitch('renderer-process-limit', '10'); | ||
|
|
||
| // Enable more aggressive garbage collection | ||
| app.commandLine.appendSwitch('js-flags', '--expose-gc --max-old-space-size=4096'); | ||
|
|
||
| console.log('[Electron] Network stability flags applied'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its size
wc -l apps/ui/src/main.ts 2>/dev/null || echo "File not found, searching for main.ts files"
fd -name "main.ts" | head -20Repository: AutoMaker-Org/automaker
Length of output: 338
🏁 Script executed:
# Read the main.ts file to see full context around lines 19-38
if [ -f "apps/ui/src/main.ts" ]; then
echo "=== Full file content ==="
cat -n apps/ui/src/main.ts
fiRepository: AutoMaker-Org/automaker
Length of output: 26449
🏁 Script executed:
# Search for environment detection patterns
rg -n "isDev|isProduction|NODE_ENV|process.env" apps/ui/src/main.tsRepository: AutoMaker-Org/automaker
Length of output: 612
🏁 Script executed:
# Search for references to the GitHub issue or related crash reports
rg -n "18397|network.*crash|OutOfBlinkCors" --type-add 'doc:*.md' --type docRepository: AutoMaker-Org/automaker
Length of output: 49
🏁 Script executed:
# Search for concurrent task or auto-mode related issues/tests
rg -i "concurrent|auto.mode|network.*crash|stability" apps/ui/src/ --type ts -C 2 | head -50Repository: AutoMaker-Org/automaker
Length of output: 2495
🏁 Script executed:
# Check if there are any comments about these specific flag trade-offs elsewhere
rg -i "site.isolation|OutOfBlinkCors|expose.gc" --type ts --type jsonRepository: AutoMaker-Org/automaker
Length of output: 413
🏁 Script executed:
# Look for other Electron flag usage patterns in the codebase
rg "commandLine\.(appendSwitch|appendArgument)" apps/ui/src/ --type ts -B 2 -A 1Repository: AutoMaker-Org/automaker
Length of output: 840
Move expose-gc flag to development-only.
The --expose-gc flag in the js-flags is a debugging/diagnostic tool and should not be exposed in production builds. The other flags (disable-site-isolation-trials, disable-features OutOfBlinkCors, renderer-process-limit) appear intentional for stability under concurrent auto-mode tasks, but expose-gc should be conditional:
if (isDev) {
app.commandLine.appendSwitch('js-flags', '--expose-gc --max-old-space-size=4096');
} else {
app.commandLine.appendSwitch('js-flags', '--max-old-space-size=4096');
}Also consider documenting the security/stability trade-offs:
- Disabling site isolation removes process-level origin isolation
- Moving network service in-process means network failures now crash the main process instead of child processes
- The 4GB heap limit should be verified as appropriate for production
🤖 Prompt for AI Agents
In apps/ui/src/main.ts around lines 19 to 38, move the debugging-only
--expose-gc out of production: detect development mode (e.g. isDev or
process.env.NODE_ENV === 'development') and only append the js-flags string that
includes --expose-gc when in dev; in production append js-flags with only
--max-old-space-size=4096. Keep the other commandLine.appendSwitch calls
unchanged and add a brief inline comment documenting the security/stability
trade-offs (site isolation, in-process network service, and 4GB heap) near these
flags.
2 participants
Add a global setting to enable/disable sandbox mode for Claude Agent SDK. This allows users to control sandbox behavior based on their authentication setup and system compatibility.
Changes:
The sandbox mode requires API key or OAuth token authentication. Users experiencing issues with CLI-only auth can disable it in settings.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Improvements — Observability
Tests
✏️ Tip: You can customize this high-level summary in your review settings.