fix(ci): grant pull-requests:write so advisor can post PR comments#3366
Merged
Conversation
The e2e-advisor workflow was running successfully on same-repo PRs after PR NVIDIA#3289 merged, but the 'Post E2E advisor PR comment' step was silently skipping with: 403 Resource not accessible by integration despite the YAML declaring 'issues: write'. See for example PR NVIDIA#3364 runs 25697796461 and 25699366979 — both green at the job level, but no advisor comment ever appeared on the PR. Root cause: for the Actions GITHUB_TOKEN, PR comments (including those posted via POST /repos/:o/:r/issues/:n/comments) are gated by the 'pull-requests' permission, not 'issues'. The 'issues' permission only covers true issues. Our previous 'pull-requests: read + issues: write' combination therefore gave the token no write access to PR comments. This matches a long-standing GitHub Actions gotcha documented in https://github.com/orgs/community/discussions/56632. Fix: flip pull-requests from read to write. The advisor's graceful permission-error handling in comment.mjs stays in place as defense in depth for orgs that further restrict token scopes via repo/org policy. Also added an inline comment on the permissions block so the next reviewer doesn't have to rediscover this. This does not address the second configuration gap surfaced by the initial production runs — PI_E2E_ADVISOR_API_KEY is not yet set on NVIDIA/NemoClaw, so the analysis step still degrades to 'No Pi provider credential was available'. That is an admin configuration task, not a code change.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR updates the GitHub Actions e2e-advisor workflow to grant ChangesWorkflow Permissions Update
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
cv
approved these changes
May 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
pull-requests: writeto thee2e-advisorworkflow so the advisor can actually post its recommendation comment on PRs.What's broken on
maintodaySince PR #3289 merged ~2 hours ago, the advisor workflow has been running successfully on same-repo PRs — but the
Post E2E advisor PR commentstep silently skips every time:Concrete evidence on PR #3364:
The runs look green because
comment.mjscatches the 403 as a permission error (a defensive path added during the #3289 review) and keeps the job passing, but the user-visible artifact — the comment on the PR — never materializes.Root cause
The
permissions:block onmaincurrently declares:The token effective permissions confirm this takes effect (from the
Set up joblog group):Even so,
POST /repos/NVIDIA/NemoClaw/issues/3364/commentsreturns403 Resource not accessible by integration.The reason is a well-documented GitHub Actions permission-name trap: for
GITHUB_TOKEN, PR comments are gated bypull-requests: write, notissues: write— even though the REST endpoint lives under/issues/:n/comments. Theissuespermission only applies to true issues; PRs fall underpull-requests. This is tracked in community discussion #56632 and has bitten many workflows.Fix
Flip
pull-requestsfromreadtowrite, and drop an inline comment on the permissions block so the next reviewer doesn't have to rediscover this.issues: writeis kept for now — it's unused on the PR-comment path but it's dirt-cheap to leave in case we ever want the advisor to post on true issues.Defense in depth
The graceful permission-error handling in
tools/e2e-advisor/comment.mjs(added in #3289 per @ericksoa's review) stays in place unchanged. This PR just ensures that for a correctly-configured repo the 403 path is no longer hit on normal same-repo PRs.Testing
python3 -c "import yaml; yaml.safe_load(open('.github/workflows/e2e-advisor.yaml'))"— workflow still parses.Out of scope for this PR
There's a second gap visible on upstream:
PI_E2E_ADVISOR_API_KEYis not set onNVIDIA/NemoClaw, so the analysis step currently degrades to:That's an admin configuration task (Settings → Secrets and variables → Actions → New repository secret), not a code change, so handling it separately.
Related
Summary by CodeRabbit