Skip to content

Fixing claude code permissions.#11

Closed
cmungall wants to merge 1 commit intomainfrom
aidocs-issue-39
Closed

Fixing claude code permissions.#11
cmungall wants to merge 1 commit intomainfrom
aidocs-issue-39

Conversation

@cmungall
Copy link
Member

@cmungall cmungall commented Sep 4, 2025

@claude
Copy link
Contributor

claude bot commented Sep 4, 2025

Claude finished @cmungall's task —— View job


PR Review: Fixing claude code permissions

Review Tasks:

  • Read and analyze changed files
  • Review copier.yaml changes
  • Review new template/.claude/settings.json file
  • Review template/.github/workflows/dragon-ai.yml changes
  • Check for security concerns
  • Assess code quality and best practices
  • Provide comprehensive feedback

📋 Review Summary

This PR introduces permissions fixes for Claude Code by adding explicit permission configuration and updating the GitHub workflow. The changes are focused and address what appears to be a permissions issue with the Claude Code integration.

🔍 Detailed Analysis

template/.claude/settings.json (NEW FILE)

Purpose: Explicit permissions configuration for Claude Code
Analysis:

  • Good: Establishes clear, granular permissions for Claude Code tools
  • Good: Uses allow-list approach for security
  • Good: Includes essential tools: Bash(*), Edit, MultiEdit, NotebookEdit, FileEdit, WebFetch, WebSearch, Write
  • Note: Bash(*) is very permissive - consider if this could be more restrictive
  • Code Quality: Clean JSON structure, proper formatting

template/.github/workflows/dragon-ai.yml

Changes: Line 185-186 - Replaced specific --allowedTools with --permission-mode bypassPermissions
Analysis:

  • Good: Simplifies the command by using the new settings file approach
  • Security Concern: bypassPermissions mode is very permissive - ensure this is intentional
  • Good: Maintains verbose logging for debugging
  • Improvement: The old approach with specific tool restrictions ("Bash(git:*)" "Bash(gh:*)" "FileSystem(*)") was more security-conscious

copier.yaml

Change: Line 34 - Fixed typo: "a-controller" → "ai-controller"
Analysis:

  • Good: Simple typo fix that improves clarity
  • Low Impact: Cosmetic improvement to help text

🔒 Security Assessment

Concerns:

  1. High Privilege: --permission-mode bypassPermissions grants very broad access
  2. Bash Wildcard: Bash(*) allows any bash command execution
  3. Reduced Granularity: Previous approach limited bash to specific tools (git:*, gh:*)

Recommendations:

  1. Consider more restrictive bash permissions like Bash(git:*), Bash(gh:*) instead of Bash(*)
  2. Document why bypassPermissions is necessary vs. using the allow-list approach
  3. Consider adding monitoring/logging for the expanded permissions

🚀 Performance & Best Practices

  • Good: JSON formatting is clean and readable
  • Good: Follows Claude Code configuration patterns
  • Good: Maintains workflow structure and error handling
  • Suggestion: Consider adding comments to the workflow explaining the permission approach

🧪 Test Coverage

  • Missing: No tests for the new permissions configuration
  • Recommendation: Consider adding integration tests to verify Claude Code works with these permissions
  • Recommendation: Test the workflow with the new permission model

📈 Overall Assessment

Rating: ✅ APPROVE WITH SUGGESTIONS

Strengths:

  • Solves the identified permissions issue
  • Clean, well-formatted configuration files
  • Maintains existing workflow structure
  • Simple typo fix improves documentation

Areas for Improvement:

  • Security could be enhanced with more granular permissions
  • Documentation of why broad permissions are needed
  • Consider adding tests for the new configuration

This PR effectively addresses the permissions issue while maintaining code quality. The main trade-off is between functionality (broader permissions) and security (more restrictive access). The approach taken prioritizes functionality, which may be appropriate depending on the use case and environment.


@cmungall cmungall requested a review from Copilot September 4, 2025 02:21
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes Claude code permissions by introducing a new permission configuration approach. The changes address issue #39 by replacing command-line permission flags with a dedicated settings file and updating the permission mode.

  • Replaced --allowedTools command-line flags with --permission-mode bypassPermissions
  • Added a new .claude/settings.json file with comprehensive permission configurations
  • Fixed a typo in the copier.yaml help text

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
template/.github/workflows/dragon-ai.yml Updated Claude command to use bypass permissions mode instead of explicit tool allowances
template/.claude/settings.json Added new permissions configuration file with allowed tools and capabilities
copier.yaml Fixed typo in help text from "a-controller" to "ai-controller"


# Run Claude with proper permissions
claude -p "$(cat /tmp/claude-input/claude_prompt.txt)" \
--permission-mode bypassPermissions \
Copy link

Copilot AI Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using bypassPermissions mode circumvents all permission checks, which could be a security risk. Consider using a more restrictive permission mode that still allows the required operations but maintains security boundaries.

Suggested change
--permission-mode bypassPermissions \
--permission-mode default \

Copilot uses AI. Check for mistakes.
@cmungall cmungall closed this Sep 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ensure permissions are set correctly for claude code

1 participant