Conversation
GitHub gates the resolveReviewThread GraphQL mutation on the token's contents permission, not on pull-requests. The workflow granted contents: read, so every mutation failed with "Resource not accessible by integration" and stale comment-cop threads stayed open on every claude-labeled PR. Grant contents: write. The job checks out no code. Count the mutations that succeed and log "Resolved X of Y" instead of reporting every attempt as resolved. Add a source lint that runs the embedded script against a fake GitHub and pins the permission, and run the lints when the workflow changes.
|
Warning Review limit reached
On-demand reviews are free for the next 26 days. After that, they cost $0.25 per reviewed file. Or wait 7 minutes for your next included review. View limit detailsLimit details: You’ve used all 5 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Status: closed, superseded by #39183.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. The logic and tests check out, but since it widens GITHUB_TOKEN to contents: write on a pull_request_target workflow, a maintainer should sign off on the permission change.
What was reviewed:
- Confirmed the job has no
actions/checkoutstep and the embedded script runs only GitHub API calls from the base-branch workflow file, so the write token is not exposed to PR-controlled code. - Traced the new
resolvedcounter and log message — accurate on both the all-succeed and partial-reject paths. - Walked the test's fake-GitHub harness against the embedded script:
presentKeys/seenKeys/toResolvederive as expected, and all three tests fail on main's workflow.
Extended reasoning...
Overview
This PR touches CI configuration only: it flips permissions.contents from read to write in .github/workflows/comment-cop.yml, adds an accurate success counter to the resolve loop's log line, wires comment-cop.yml into the source-lints.yml path triggers, and adds test/internal/source-lints/comment-cop.test.ts which parses the workflow YAML, extracts the embedded github-script body, and executes it against a mock github/context/core to pin the permission and the resolve behavior. No runtime or product code is affected.
Security risks
The substantive risk is granting contents: write to a pull_request_target workflow. That combination is the classic vector for fork-PR privilege escalation when the workflow checks out and runs PR head code. Here the job has no checkout step at all — its only step is actions/github-script with a script literal that lives in the base-branch workflow file, and it interacts with the PR solely via REST/GraphQL. The if: guard also pins github.repository == 'oven-sh/bun'. Under those constraints the widened token cannot be reached by untrusted code. That said, the blast radius if this reasoning were wrong (or if a later edit added a checkout) is write access to the repo, so this is exactly the kind of change a human maintainer should explicitly approve rather than an automated reviewer.
Level of scrutiny
High, because it is a permission grant on a fork-triggerable workflow. The code change itself is trivial (one YAML value, one counter, two path-filter lines) and the test is well-constructed — it actually executes the embedded script the same way actions/github-script does and asserts exact resolve attempts and log output, and it fails on main. But per the approval guidelines, security-sensitive permission changes should not be auto-approved regardless of how sound the accompanying analysis is.
Other factors
The PR description cites an external probe demonstrating the contents gating and a community discussion corroborating it, and notes two overlapping open PRs (#36959, #39183) that take different approaches — a maintainer will want to reconcile those anyway. No prior human review comments exist on the timeline.
|
Agreed that the permission grant is the part that needs a human decision, which is why the PR is assigned to @Jarred-Sumner rather than self-merging. For that decision, the relevant facts in one place: the job has no checkout step and runs a fixed script from the base branch. |
|
Closing this PR. A self-review of the change made a case against it that holds up, even though the diagnosis (the mutation needs
#39183 is the direction to take for this workflow. It already includes the |
2 participants
Problem
resolveReviewThread failed for PRRT_...: Resource not accessible by integration, then the step logsResolved 10 stale comment-cop thread(s).anyway (run 32848460003 on css: keep incompatible selectors that fail the same way in one rule #40464). Someone has to resolve the stale threads by hand.resolveReviewThreadon the token'scontentspermission, not onpull-requests..github/workflows/comment-cop.yml:12grantscontents: read.Fix
contents: write. The job checks out no code and runs a fixed script from the base branch, so PR content never reaches the wider token.Resolved X of Y. A failure is a warning, not a resolved thread.contents: readfails with this exact error,contents: writesucceeds (open-code-review#27, run links in Notes). This supersedes the PAT in ci: use a PAT to resolve stale comment-cop review threads #36959: the Actions token is not blocked, it is under-scoped.test/internal/source-lints/comment-cop.test.tsruns the embedded script against a fake GitHub and pins the permission. All 3 tests fail on main.Background
pull_request_targetruns the base branch's workflow file with a write-capableGITHUB_TOKEN. Thepermissionsblock sets that token's scope. The risk with this event is to run PR code under that token. This job never does.source-lints.ymlrunstest/internal/source-lints/against a released bun. It now also triggers oncomment-cop.ymlchanges.Notes
contents: read+pull-requests: write,RESOLVE_RESULT=FAILED ... Resource not accessible by integration) and 31245826522 (contents: write+pull-requests: write,RESOLVE_RESULT=OK). Community discussion 44650 reports the same requirement for GitHub App tokens.viewerCanResolve: falsein both runs, including the one that succeeded, so that field cannot serve as a pre-flight check.pull_request_targetruns the base branch's copy of the workflow, so the Comment Cop run on this PR still executes the script from main. The change takes effect after merge. TheSource lintsjob is what exercises the new test here.ROBOBUN_TOKEN, a long-lived PAT with push access to other oven-sh repos), ci: keep comment-cop working when GITHUB_TOKEN's GraphQL quota is exhausted #39183 (drops the resolve loop because it always failed, and moves dedup to REST to survive GraphQL quota exhaustion). With this change the loop works, so ci: keep comment-cop working when GITHUB_TOKEN's GraphQL quota is exhausted #39183 would need to keep it and guard thereviewThreadsquery instead of removing it.Expected: "write", Received: "read"andReceived: "Resolved 2 stale comment-cop thread(s).".bun test test/internal/source-lints/(168 pass) andbun bd test test/internal/source-lints/comment-cop.test.ts.