fix: allow permission_mode to be configured in config.toml for Claude provider - #176
fix: allow permission_mode to be configured in config.toml for Claude provider#176josephfung wants to merge 1 commit into
Conversation
The Claude provider hardcoded bypassPermissions, which fails when
running as root in Docker ("--dangerously-skip-permissions cannot be
used with root/sudo privileges"). Now reads permission_mode from
config.toml, allowing containers to use acceptEdits instead.
|
User does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 52 minutes and 47 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces a permission_mode configuration for the Claude provider to address permission issues when running as root in Docker environments. The changes update the ClaudeProvider class to read this setting and extend the ClaudeProviderConfig and ProviderConfig interfaces. Feedback was provided to ensure that programmatic options take precedence over configuration file settings, allowing for proper runtime overrides.
| // Read permission_mode from provider config, falling back to options, then default. | ||
| // 'bypassPermissions' fails when running as root in Docker, so containers | ||
| // should set permission_mode = "acceptEdits" in config.toml. | ||
| this._permissionMode = config.permission_mode | ||
| ?? options.permissionMode | ||
| ?? 'bypassPermissions'; |
There was a problem hiding this comment.
Programmatic options typically take precedence over configuration file settings to allow for runtime overrides. The current implementation prioritizes config.permission_mode over options.permissionMode. Swapping the order ensures that explicit overrides provided during instantiation are respected.
| // Read permission_mode from provider config, falling back to options, then default. | |
| // 'bypassPermissions' fails when running as root in Docker, so containers | |
| // should set permission_mode = "acceptEdits" in config.toml. | |
| this._permissionMode = config.permission_mode | |
| ?? options.permissionMode | |
| ?? 'bypassPermissions'; | |
| // Read permission_mode from options (programmatic override) or provider config, falling back to default. | |
| // 'bypassPermissions' fails when running as root in Docker, so containers | |
| // should set permission_mode = "acceptEdits" in config.toml. | |
| this._permissionMode = options.permissionMode | |
| ?? config.permission_mode | |
| ?? 'bypassPermissions'; |
There was a problem hiding this comment.
I get the general principle - programmatic options should normally beat config file settings.
However, in this codebase the orchestrator always passes permissionMode: 'default' explicitly when constructing ClaudeProviderOptions (see src/orchestrator/orchestrator.ts), so options.permissionMode is never undefined. With the order suggested, config.permission_mode would never be reached, making the feature silently non-functional.
The intent here is that config.permission_mode is the user's override lever (the only one they have), while options.permissionMode is the orchestrator's internal default. So config should win over the orchestrator default, which is what the current order achieves.
If there's a future need for a true programmatic override that beats the config, the right fix would be a distinct field (e.g., options.permissionModeOverride) that the orchestrator sets only when explicitly requested rather than conflating it with the fallback default.
|
Hi there, I'm going to close this PR for now, as it hasn't received a response or review from the maintainers. I completely understand that open source projects can get busy, and there’s no pressure at all. If there's ever interest in revisiting this, id be happy to reopen or resubmit. |
Summary
Fixes #166.
The Claude provider hardcodes
permissionModeto'bypassPermissions', which becomes--dangerously-skip-permissionswhen spawning the Claude Code subprocess. That flag is rejected when running as root (the Docker default):This adds an optional
permission_modefield to the provider config, letting container deployments useacceptEditsinstead. Existing configs without the field are unaffected — the fallback chain preserves'bypassPermissions'as the default.Changes
src/types.ts— addspermission_mode?: 'default' | 'acceptEdits' | 'bypassPermissions' | 'plan'toClaudeProviderConfigandProviderConfigsrc/providers/claude-provider.ts— readsconfig.permission_modein the constructor before falling back tooptions.permissionModethen'bypassPermissions'Example config for Docker:
Testing
permission_mode = "acceptEdits": Claude provider spawns, executes tasks, policy enforcement and audit logging work correctlypermission_modebehave identically (fallback to'bypassPermissions')