feat: implement issue #749 — Canary 403 NOT fixed by #745/#746 — release-manager App can't bypass tag ruleset even repo-scoped; need effective-permission diagnostic#750
Conversation
…ase-manager App can't bypass tag ruleset even repo-scoped; need effective-permission diagnostic
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe canary rollout now diagnoses targeted GitHub API 403 tag-move failures by reporting effective permissions and installation repository scope. Bats tests cover diagnostic output, token routing, repository scope, and non-403 behavior. ChangesCanary 403 diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant TagMover
participant GitHubAPI
participant DiagnosticOutput
TagMover->>GitHubAPI: PATCH tag reference
GitHubAPI-->>TagMover: 403 integration-access failure
TagMover->>GitHubAPI: Probe permissions and installation repositories
GitHubAPI-->>TagMover: Permission header and repository scope
TagMover->>DiagnosticOutput: Emit grouped diagnostic and error
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Dev-Lead — waiting on PR blockers (intent: review-changes)PR: #750 |
|
Note @don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically. |
![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 one-shot effective-permission diagnostic function _gh_403_diag in scripts/canary-rollout.sh to troubleshoot 403 errors during tag-move operations, along with comprehensive tests in tests/canary_rollout.bats. The feedback suggests improving the robustness of the diagnostic by guarding the jq parser against empty or null responses using a non-empty check and the optional operator ?.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds a one-shot diagnostic path for the canary tag-move failure case where gh api returns 403 “Resource not accessible by integration”, so future promotion runs can capture effective token permissions/scope data in the logs and guide the next remediation step for issue #749.
Changes:
- Add
_gh_403_diagand invoke it from_gh_move_tagwhen the PATCH failure looks like a 403 integration-access denial. - Add Bats coverage that stubs the 403 path and asserts the diagnostic is emitted and POST fallback is not attempted.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| scripts/canary-rollout.sh | Adds _gh_403_diag and calls it on 403-ish PATCH failures before surfacing the ::error::. |
| tests/canary_rollout.bats | Adds stubs + tests for the 403 diagnostic behavior and token routing. |
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
|
|
CI checks on this PR are still running. Once they complete, re-mention Posted by the donpetry-bot PR-review cascade. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: c271929123b46ac462971de13153bd03b0640b35
Review mode: triage-approved (single reviewer)
Summary
Adds a one-shot, best-effort 403 effective-permission diagnostic (_gh_403_diag) to the _gh_move_tag failure path in scripts/canary-rollout.sh, plus 6 bats tests. The diagnostic runs only on 403 "Resource not accessible by integration" failures, prints the X-Accepted-GitHub-Permissions header and installation-repositories total_count using the same write token the PATCH used, never alters the original exit status, and never falls back to POST. Confirms the triage assessment: low-risk, diagnostic-only change.
Linked issue analysis
Closes #749, which asked for exactly this: a one-shot effective-permission diagnostic on the 403 tag-move failure (after #745/#746 token-scoping did not resolve it), to distinguish (a) a token-minting bug from (b) a lapsed ruleset bypass. The PR implements ask #1 verbatim and respects the issue guardrails: no ruleset changes, #744's un-suppressed error and #746's two-token structure are preserved. Substantively addressed.
Findings
- No blocking findings.
- Security: no token values are logged (only which token name is used for introspection); both probes are read-only GETs; diagnostic is guarded (
|| true,jqoptional operator, always returns 0) so it cannot mask the original 403 or change_gh_move_tag's exit code. - Prior reviewer feedback (gemini: jq null-guarding; Copilot: non-existent
repository_selectionfield on /installation/repositories) was addressed in follow-up commits — the final code usestotal_countwith.total_count? // emptyand non-empty checks, with dedicated edge-case tests. All 5 review threads are resolved. - Test coverage is thorough: 403 diagnostic emission, CANARY_WRITE_TOKEN routing, owner-wide vs repo-scoped count, non-403 (422) exclusion, empty/keyless API responses.
- Secret scan MCP tool (run_secret_scanning) unavailable in this environment; gitleaks CI check passed.
CI status
All checks green: Lint, ShellCheck, Agent Security Scan, Secret scan (gitleaks), Canary-rollout Tests (lint + bats), CodeQL, SonarCloud (quality gate passed, 0 new issues), AgentShield, dependency audits. Remaining SKIPPED checks (dependabot, pnpm/pip/cargo audit, govulncheck, ci-relay) are ecosystem-conditional and expected.
Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.
Closes #749
Implemented by dev-lead agent. Please review.
Summary by CodeRabbit