Conversation
The Actions GITHUB_TOKEN cannot call the resolveReviewThread GraphQL mutation: every call fails with "Resource not accessible by integration" even though the workflow grants pull-requests: write, so stale comment-cop threads have piled up unresolved on every claude-labeled PR since the workflow landed. Move the resolution into a separate best-effort step that authenticates with the existing ROBOBUN_TOKEN secret (a user token can run this mutation; verified directly). Scanning and posting review comments keep using GITHUB_TOKEN, and the scan step hands the stale thread ids to the resolve step through a step output.
WalkthroughThe workflow identifies trusted stale review threads by root-comment author, emits their IDs from the scan step, and resolves them in a separate PAT-authenticated step with partial-failure reporting. ChangesComment-cop stale-thread handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
I didn't find any bugs, and the security posture looks sound (no PR-code checkout, thread IDs are GitHub-generated node IDs passed via env: rather than interpolated into the script, label-gated trigger, continue-on-error). That said, this introduces a PAT into a pull_request_target workflow — the classic pwn-request surface — so a human should sign off on the credential exposure.
What was reviewed
- Injection:
STALE_THREADSis passed viaenv:and read withprocess.env, not${{ }}inside the script; IDs originate from GitHub's GraphQLreviewThreads.nodes[].id. - Scope: PAT step only runs
resolveReviewThreadon IDs whose root comment carries the<!-- comment-cop:... -->marker; scan step still usesGITHUB_TOKEN. - Control flow:
core.setOutputruns before the earlyreturn, and an emptytoResolveyields''so theif:guard skips the resolve step. - Confirmed
ROBOBUN_TOKENis an existing repo secret already consumed byrelease.yml.
Extended reasoning...
Overview
This PR modifies .github/workflows/comment-cop.yml — a pull_request_target workflow that scans claude-labeled PRs for multi-line comment blocks and posts review comments. The change splits the previously-broken "resolve stale threads" logic into its own step authenticated with the ROBOBUN_TOKEN PAT instead of the Actions GITHUB_TOKEN, because GitHub rejects the resolveReviewThread GraphQL mutation from integration tokens. The scan step now exports stale thread IDs via core.setOutput; a new step consumes them via an env: var and runs the mutation with per-ID try/catch and continue-on-error.
Security risks
This is the crux. pull_request_target workflows run with base-repo secrets and can be triggered by fork PRs, so exposing a PAT here is the canonical pwn-request risk surface. I walked through it and the design is defensible:
- No PR code is checked out or executed. The scan step reads the diff via
github.rest.pulls.listFilesand parses patch text; nothing from the PR reaches a shell oreval. - The PAT step receives only GitHub-generated node IDs (
reviewThreads.nodes[].id, e.g.PRRT_...), filtered to threads whose root comment carries the bot's own<!-- comment-cop:KEY -->marker. An attacker cannot control these values. - No script injection: the output is passed via
env: STALE_THREADS: ${{ ... }}and read withprocess.env.STALE_THREADS.split(' '), not interpolated into the script body. - Label-gated: the job only runs on PRs carrying the
claudelabel, which requires triage/write to add. - Blast radius of misuse is tiny: even if an attacker could somehow inject arbitrary IDs, the only operation is
resolveReviewThread— resolving review threads, not reading secrets or writing code.
I did not find a path by which PR-controlled content reaches the PAT-authenticated step in a way that could exfiltrate or misuse the token. Nonetheless, granting a workflow access to a user PAT in a fork-triggerable context is exactly the kind of decision a maintainer should explicitly ratify rather than have auto-approved.
Level of scrutiny
High. Workflow files under pull_request_target that touch secrets are one of the most common sources of supply-chain compromise in GitHub Actions. The change itself is mechanically simple (~30 lines net), and the correctness is easy to verify — core.setOutput is called before the early return, empty toResolve produces '' which is falsy in the if: expression so the step is skipped, and .split(' ').filter(Boolean) handles edges. But "simple" and "security-sensitive" are orthogonal.
Other factors
ROBOBUN_TOKENis already a repo secret consumed byrelease.yml(lines 209, 312, 317), so this isn't introducing a new secret — butrelease.ymlis not fork-triggerable, whereas this workflow is.- No CODEOWNERS entry covers
.github/workflows/. - The PR description accurately describes the security posture and the author clearly thought it through; I'm deferring on the principle that credential-scope changes in
pull_request_targetwarrant human sign-off, not because I found a flaw.
|
Agreed that the credential question is the part that needs a human decision, which is why the PR is assigned to Jarred rather than self-merging. To restate the trade-off in one place: the workflow never checks out or executes PR code, the PAT step receives only GitHub-generated thread ids through env (never interpolated into the script), and the only mutation it can run is resolveReviewThread on threads the bot itself opened. If exposing ROBOBUN_TOKEN here is still unwanted, the fallback would be deleting the stale bot comments over REST with the integration token, at the cost of losing thread history and any human replies. |
…lure Check that the root comment author is github-actions before trusting the comment-cop marker: the marker alone is forgeable by pasting it into any review comment, and the resolve step now acts with a PAT. Fail the resolve step (kept non-blocking by continue-on-error) when every mutation fails, so a dead token surfaces as an error annotation instead of the silent warnings that let the original breakage go unnoticed.
|
Pushed a second commit addressing what came out of a self-review pass:
On CI: the two red tests on the asan lane (worker-transfer-terminate-stress SIGABRT, AsyncLocalStorage-tracking leak) are pre-existing on main and unrelated to this workflow-only diff, which Buildkite does not execute anyway. |
There was a problem hiding this comment.
No issues found in the implementation, but this exposes ROBOBUN_TOKEN (a broad PAT that release.yml uses to push to other oven-sh repos) to a pull_request_target workflow — a credential-scope decision that a maintainer should sign off on, as the PR description itself notes.
What was reviewed:
- Injection surface: thread IDs are read from
process.env, not interpolated into the script; the scan step passes only GitHub-generated GraphQL node IDs. pull_request_targetexposure: workflow file runs from base branch, no checkout of PR code, gated on theclaudelabel.- Author-check hardening (
github-actionslogin) correctly narrows which threads the PAT step will act on.
Extended reasoning...
Overview
This PR modifies .github/workflows/comment-cop.yml to fix stale-thread auto-resolution, which has never worked because GitHub's Actions GITHUB_TOKEN cannot call the resolveReviewThread GraphQL mutation. The fix splits resolution into a separate step authenticated with secrets.ROBOBUN_TOKEN, passes stale thread IDs via a step output, adds an author check so only threads opened by github-actions are considered, and marks the resolve step failed (non-blocking via continue-on-error) if every mutation fails.
Security risks
The material change is exposing ROBOBUN_TOKEN to a pull_request_target-triggered job. That token is currently used in release.yml to push to oven-sh/homebrew-bun, so it has write access beyond this repo. The mitigations are sound: the workflow file runs from the base branch (attackers can't edit it), no PR code is checked out or executed, the resolve step only receives GitHub-generated node IDs via env (never string-interpolated into the script body), and thread selection now requires both the github-actions author and the marker. The claude label gate requires triage access. I did not find an injection or exfiltration path. However, the PR description explicitly flags that ROBOBUN_TOKEN is broader than needed and offers two narrower alternatives (a fine-grained PAT or a GitHub App installation token) that need org-side provisioning — that policy trade-off is a human call.
Level of scrutiny
High. Any change that puts a repo secret with cross-repo write scope behind a PR-triggered event deserves a maintainer's explicit approval, even when the implementation is careful. The author's own comment on the PR ("the credential question is the part that needs a human decision, which is why the PR is assigned to Jarred rather than self-merging") agrees.
Other factors
The logic changes themselves look correct: core.setOutput + if: steps.scan.outputs.stale-threads skips the resolve step when the output is empty, .split(' ').filter(Boolean) is defensive against that anyway, and the total-failure check surfaces a dead token without blocking the job. The author-login check tightens a real (if minor) forgery gap that mattered more once a PAT is behind the action. No automated test is expected for a workflow-only change; the PR documents manual verification against a live failing run.
Under the Actions GITHUB_TOKEN every resolveReviewThread mutation fails with "Resource not accessible by integration" (runs 31900717034 and 31900354924 today: 20/20 and 4/4 failed, each followed by a "Resolved N" log line), so keeping the thread lookup behind a try only preserved requests that fail and a misleading log line. With the lookup gone the step makes no GraphQL request at all; dedup comes from the REST review comment list. Resolving stale threads stays with #36959. The test shrinks to the dedup contract with GraphQL unavailable.
|
Heads-up from #39183: that PR makes comment-cop's scan step REST-only. It replaces the If #39183 lands first, the rebase here would need the |
|
The premise here needs a correction: the Actions token is not blocked from Evidence: open-code-review#27 ran one script twice, a minute apart, against its own review thread. With So the PAT is not needed. #40468 makes the one-line permission change, counts the mutations that succeed in the summary line, and adds a source lint that pins the permission. The |
|
Correction to my comment above: #40468 is closed. A review of that change found the stale threads already get resolved without the workflow's help (all 61 comment-cop threads on the 15 most recent merged |
|
Closing this PR in favor of #39183. This PR makes the comment-cop resolve loop work by running the The review of #40468 showed the loop is not needed. On the 15 most recent merged For the record, the original failure has a known cause: GitHub gates |
…austed (#39183) ### Problem - The `Comment Cop` check is red on every claude-labeled PR whenever the repo's GraphQL quota is used up. The step dies before it scans anything: ``` GraphqlResponseError: Request failed due to following response errors: - API rate limit already exceeded for site ID installation. ##[error]Unhandled error: GraphqlResponseError: ... ``` with `x-ratelimit-resource: graphql`, `x-ratelimit-limit: 10000`, `x-ratelimit-remaining: 0` (run [31898901053](https://github.com/oven-sh/bun/actions/runs/31898901053) on #39139). All 33 comment-cop failures on Aug 15 are this error (197 runs succeeded, 171 were skipped); they hit unrelated PRs at the same time because the quota is shared by every workflow run in the repo, and they come back whenever PR volume is high. - Cause, `.github/workflows/comment-cop.yml:124` on main: the step reads the PR's existing review threads with `github.graphql()`, outside any `try`, and dedup and posting both sit behind that call. The REST call just before it (`pulls.listFiles`) had succeeded in each failing run, so the step had the diff; it only lacked the dedup data, which it was reading from the exhausted quota. - The same thread data also fed an auto-resolve of stale threads. That part does not work: under the Actions `GITHUB_TOKEN` every `resolveReviewThread` mutation fails with `Resource not accessible by integration`, after which the step still logs `Resolved N stale comment-cop thread(s).` Two successful runs from today: [31900717034](https://github.com/oven-sh/bun/actions/runs/31900717034) (20 attempted, 20 failed, "Resolved 20") and [31900354924](https://github.com/oven-sh/bun/actions/runs/31900354924) (4 attempted, 4 failed, "Resolved 4"). #36959 documents the same thing and is the PR that moves resolution to a token that can do it. So the GraphQL query was paying for one thing REST can provide and one thing that does not happen. ### Fix - Dedup reads the PR's review comments with `pulls.listReviewComments` (REST) and collects the `<!-- comment-cop:KEY -->` markers from them. Every comment the step posts starts with that marker, so the review comments carry the same keys the thread roots did; REST is the quota the step already needs for `listFiles` and `createReviewComment`, and it was available in every failing run. - The GraphQL query and the resolve loop are removed, so the step makes no GraphQL request at all; the failure in the Problem section cannot happen, rather than being caught. Nothing observable is lost: the mutations the loop issued all fail today, and the only other thing it did was log `Resolved N stale comment-cop thread(s).` after they had failed. Resolving stale threads stays with #36959, which will also need to move its thread lookup into its token step, since this PR removes the `GITHUB_TOKEN` lookup it currently reuses (noted there). - What the step posts is unchanged: the script on main and the REST dedup were dry-run (real reads, writes recorded) against 9 PRs carrying existing comment-cop threads (#35988, #36956, #36713, #35635, #33632, #35596, #39139, #30609, #36959) and chose the same comments to post on every one of them; the canned scenarios below show the same thing with GraphQL working and with it exhausted. - Test: `test/internal/source-lints/comment-cop.test.ts` extracts the script from the workflow and runs it the way `actions/github-script` does, against a fake `github` whose GraphQL requests all fail with the rate limit error above. It checks that groups not yet flagged are posted with the right line ranges, that a group already flagged (and a stale marker) are left alone, that a second run recognizes the comments the first run posted and posts nothing, and that no GraphQL request is made. Both tests fail against the workflow on main (the script throws the error above) and pass with this change. `source-lints.yml` now also triggers on changes to `comment-cop.yml`, so the test runs whenever the script is edited; it is excluded from the Buildkite shards like the rest of that directory. - Also ran: `bun bd test test/internal/source-lints/comment-cop.test.ts`, `bun test test/internal/source-lints/` (whole directory green), prettier on the three files. The comment-cop run on this PR itself still executes the script from main (`pull_request_target`), so the `Source lints` job is the one that exercises the change here. - Does not overlap with #37948 (which groups are flagged) or #38127 (where the file list comes from); both touch other parts of the script. #36959 rewrites the block this PR deletes and will need a rebase either way. ### Background - Comment Cop (`.github/workflows/comment-cop.yml`): on each push to a claude-labeled PR it reads the PR diff, finds multi-line comments added under `src/`, and posts one review comment per comment block. Each bot comment starts with `<!-- comment-cop:KEY -->`, KEY being the file path plus a hash of the block's text; a block whose KEY is already on the PR is not posted again. The check is advisory (not required for merge). - GitHub API quotas: REST and GraphQL requests count against separate hourly quotas (`x-ratelimit-resource` is `core` for REST and `graphql` for GraphQL). For the `GITHUB_TOKEN` Actions hands out, each quota is per repository, so every workflow run in oven-sh/bun draws on the same two pools, and GraphQL-heavy automation (the `gh pr` / `gh issue` / `gh search` commands used by other workflows go through GraphQL) empties the GraphQL pool for everything else when PR volume is high. - Review threads vs review comments: a review thread is GraphQL's grouping of a line comment with its replies, and is the only place a thread's id (what `resolveReviewThread` takes) and its resolved flag exist. `GET /repos/{owner}/{repo}/pulls/{n}/comments` returns every review comment on the PR, including each thread's root comment, so the markers are reachable from REST; only resolving needs GraphQL, and under `GITHUB_TOKEN` GitHub refuses that mutation regardless of the `pull-requests: write` permission. <details> <summary>Script on main vs this branch against a fake github (the fake's resolve mutation fails the way GITHUB_TOKEN's does)</summary> ``` main | quota ok, stale threads present | exit 0 | posts ["src/foo.ts:5-6"] | resolve attempts ["T_STALE_OPEN"] | graphql ["query","query","mutation"] | warnings 1 main | graphql quota exhausted, stale threads present | step fails (unhandled rate limit error) | posts [] | resolve attempts [] | graphql ["query"] | warnings 0 main | graphql quota exhausted, nothing stale | step fails (unhandled rate limit error) | posts [] | resolve attempts [] | graphql ["query"] | warnings 0 main | graphql quota exhausted, PR has no review comments yet | step fails (unhandled rate limit error) | posts [] | resolve attempts [] | graphql ["query"] | warnings 0 fixed | quota ok, stale threads present | exit 0 | posts ["src/foo.ts:5-6"] | resolve attempts [] | graphql [] | warnings 0 fixed | graphql quota exhausted, stale threads present | exit 0 | posts ["src/foo.ts:5-6"] | resolve attempts [] | graphql [] | warnings 0 fixed | graphql quota exhausted, nothing stale | exit 0 | posts ["src/foo.ts:5-6"] | resolve attempts [] | graphql [] | warnings 0 fixed | graphql quota exhausted, PR has no review comments yet | exit 0 | posts ["src/foo.ts:2-3","src/foo.ts:5-6"] | resolve attempts [] | graphql [] | warnings 0 ``` </details> <details> <summary>Dry-run against live PRs: comments the script on main would post vs the REST dedup (real reads through a user token, writes recorded)</summary> ``` PR #35988: same posts (0 vs 0) 245 threads on the PR are stale; main attempts to resolve them, this branch does not PR #36956: same posts (0 vs 0) PR #36713: same posts (1832 vs 1832) the stale cached file list that #38127 fixes; identical on both PR #35635: same posts (0 vs 0) PR #33632: same posts (0 vs 0) PR #35596: same posts (0 vs 0) PR #39139: same posts (1 vs 1) the comment the failing run above did not get to post PR #30609: same posts (0 vs 0) PR #36959: same posts (0 vs 0) ``` </details> <details> <summary>Headers from the failing run</summary> ``` errors: [ { type: 'RATE_LIMIT', code: 'graphql_rate_limit', message: 'API rate limit already exceeded for site ID installation.' } ] variables: { owner: 'oven-sh', repo: 'bun', pr: 39139, after: null } 'x-ratelimit-limit': '10000' 'x-ratelimit-remaining': '0' 'x-ratelimit-resource': 'graphql' 'x-ratelimit-used': '10000' ``` </details>
Problem
Comment Cop is supposed to auto-resolve its own review threads once the flagged comment block disappears from the PR diff. That has never worked: every
resolveReviewThreadcall fails withso stale bot threads accumulate on every claude-labeled PR and get cleaned up by hand. Example: run 31002896415 on #36956 logged 64 consecutive
resolveReviewThread failedwarnings; earlier runs show the same, so it is not transient.Cause
GitHub rejects the
resolveReviewThreadGraphQL mutation specifically for the ActionsGITHUB_TOKEN, even though the workflow grantspull-requests: write(posting review comments through REST works fine with the same token). The restriction does not apply to app tokens in general (CodeRabbit's installation token resolves threads on this repo) or to user tokens: I verified by running the exact mutation as robobun against one of the threads from that run (PRRT_kwDOFVKCyc6Wol1B), which succeeded, while the workflow's attempt on the same thread had failed.Fix
Split thread resolution out of the scan step:
GITHUB_TOKENto read the diff and post review comments, and now exports the stale thread ids as a step output instead of resolving them.resolveReviewThreadmutations authenticated with the existingROBOBUN_TOKENrepo secret, the same PAT release.yml already uses. The step is non-blocking (continue-on-error), but if every mutation fails it marks itself failed so a dead or under-scoped token surfaces as an error annotation in the run instead of repeating the silent-warning failure mode this PR fixes.Since the resolve step now acts with real write authority, thread selection also got stricter: a thread only counts as comment-cop's if its root comment is authored by
github-actionsand carries the<!-- comment-cop:... -->marker. Previously the marker alone was trusted, and anyone can paste the marker into their own review comment.The PAT's exposure stays narrow: the workflow checks out no PR code, the resolve step receives only GitHub-generated thread ids through
env(never interpolated into the script), and the only operation it performs is resolving threads the bot itself opened. Fork PRs cannot trigger this workflow on their own: theclaudelabel requires triage access to add, and the auto-labeler runs with a read-only token on fork PRs.Token choice
ROBOBUN_TOKENis used because it already exists and demonstrably works. It is broader than this job needs (release.yml uses it to push to other oven-sh repos). If narrower credentials are preferred, either of these is a drop-in replacement forgithub-tokenin the resolve step, and both need someone with org access to provision:actions/create-github-app-token(app installation tokens can run this mutation; only the Actions token is blocked).Verification
ROBOBUN_TOKENis a plain repo secret already consumed by non-environment jobs in release.yml, so it is available here.Note:
pull_request_targetruns the workflow from the base branch, so CI on this PR cannot exercise the change; it takes effect once merged. The CI failures on this PR are pre-existing on main (asan lane) and unrelated to this workflow-only diff.no test proof · iteration 0 · build/CI scripts only; test-proof not applicable