-
Notifications
You must be signed in to change notification settings - Fork 49
fix(ci): add workflows:write permission to amber-issue-handler #592
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
The amber-handler job fails at the "Push branch to remote" step when Amber creates or modifies files under .github/workflows/. GitHub requires the explicit `workflows` permission for any token that touches workflow files, even if `contents: write` is already granted. Ref: https://github.com/ambient-code/platform/actions/runs/21777412886/job/62836130769 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude Code ReviewSummaryThis PR adds the Issues by Severity🚫 Blocker IssuesNone identified. The change is safe and correctly implements the fix. 🔴 Critical IssuesNone identified. 🟡 Major IssuesNone identified. 🔵 Minor Issues1. Missing Documentation Update
Positive Highlights✅ Excellent Problem Diagnosis - The PR clearly identifies the root cause with a link to the failing run and the exact error message ✅ Minimal Change Scope - Only adds the necessary permission, follows principle of least privilege ✅ Clear Documentation - The inline comment ✅ Follows Security Best Practices - The permission is scoped to the specific workflow that needs it, not granted globally ✅ Consistent Formatting - The alignment of comments matches existing style (spaces for alignment after ✅ GitHub Security Model Compliance - Correctly implements GitHub's requirement that tokens must have explicit RecommendationsPriority 1: Safe to Merge ✅This PR is ready to merge as-is. The change correctly addresses the identified issue and follows all security and coding standards. Priority 2: Post-Merge Enhancement (Optional)Consider documenting this permission requirement in user-facing documentation: # In docs/amber-automation.md or docs/amber-quickstart.md
## Workflow Permissions
The Amber Issue Handler requires the following GitHub Actions permissions:
- `contents: write` - To push commits to branches
- `issues: write` - To comment on issues
- `pull-requests: write` - To create and update PRs
- `workflows: write` - **Required** when Amber creates/modifies workflow files in `.github/workflows/`
- `id-token: write` - For OIDC authentication with cloud providersTest Plan ValidationThe PR mentions re-running Amber on issue #591. After merge, verify:
Technical AnalysisChange Review+ workflows: write # Required to create/update GitHub Actions workflow filesWhy This is Correct:
Security Validation✅ No Privilege Escalation Risk: The permission is scoped to workflow file modifications only, not execution ✅ Audit Trail: All workflow changes pushed by Amber are visible in git history and PR diffs ✅ No Token Exposure: The permission doesn't expose the GitHub token or expand its scope beyond workflow files ✅ Branch Protection Compatible: Standard branch protection rules still apply to workflow file changes Compliance Check Against Repository StandardsFrom
From CLAUDE.md § Backend and Operator Development Standards:
From CLAUDE.md § Git Workflow:
ConclusionRecommendation: ✅ APPROVE AND MERGE This PR correctly implements the fix for Amber's workflow file permission issue. The change is:
No blocking or critical issues identified. The suggested documentation enhancement is optional and can be addressed in a follow-up PR if desired. 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
#592)" (#594) ## Summary - Reverts PR #592 which had failing CI tests - The `workflows: write` permission fix will be re-applied directly via issue #593 ## Context PR #592 was merged but CI tests failed. Reverting to restore a clean main branch. The fix (adding `workflows: write` to amber-issue-handler) will be re-applied directly from the complete file provided in issue #593. ## Test Plan - [ ] CI tests pass on this revert PR - [ ] Re-apply the fix via direct edit per issue #593 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Summary
workflows: writepermission to the amber-issue-handler workflow.github/workflows/workflowspermission for any token touching workflow files, even withcontents: writeContext
The
amber-handlerjob in run #21777412886 failed because Amber created anightly-builds.ymlworkflow file for issue #591, but GitHub rejected the push:Test Plan
amber:auto-fix)🤖 Generated with Claude Code