Uh oh!
There was an error while loading. Please reload this page.
ci(security-review): drop pull_request_review trigger (broken on fork PRs) - #1310
Conversation
…njected on fork PRs GitHub does not inject repo/org secrets (vars.APP_ID, secrets.APP_PRIVATE_KEY, the Bedrock OIDC role) into workflows triggered by pull_request_review when the PR head is a fork, regardless of the reviewer's access level. Approving a fork PR therefore fired the workflow but the App-token step crashed with 'Input required and not supplied: app-id' because vars.APP_ID resolved to empty. The label-on-pull_request_target path does inject secrets and is the only trigger that works end-to-end for fork PRs. Drop the pull_request_review trigger entirely; same-repo maintainer PRs auto-run on opened/synchronize as before, community PRs require the safe-to-review label.
Claude Security Review: no high-confidence findings. (run) |
Package TarballHow to installgh release download pr-1310-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.14.0.tgz |
Coverage Report
|
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
LGTM — the rationale is correct: GitHub does not inject repo/org secrets into pull_request_review runs when the PR head is a fork, so the trigger could never have completed the App-token step on community PRs. Removing it (plus the corresponding branches in the auth filter, the auth step if, and the isApproval script path) is the right fix, and the concurrency group cleanup is consistent with that.
The same-repo auto-run path (opened/reopened/synchronize gated on PR author) and the community label path (safe-to-review gated on the labeler) both still resolve correctly through the simplified if and the simplified script. CONTRIBUTING.md is updated to match.
Nothing blocking from me. One trivial nit you can ignore: the comment on lines 96–97 of .github/workflows/pr-security-review.yml still references pull_request_review events as a reason the default GITHUB_TOKEN is read-only — that event no longer triggers the workflow, so the reference is stale. Worth a one-line update next time you're in the file but not worth blocking on.
…ry on cancel Three fixes rolled in: 1. Use the SDK's bundled /security-review slash command instead of inlining the prompt. The Claude Code SDK that ships with anthropics/claude-code-action exposes /security-review as a registered slash command (visible in the 'slash_commands' list its system init prints). It drives its own git-diff-based context, sub-task fan-out, and false-positive filtering. We were re-implementing all of that in a 23 KB inline prompt, plus a fragile build step that crashed on fork PRs because .github/prompts/security-review.md doesn't exist on the head ref. Drops .github/prompts/security-review.md, the Build/Read prompt steps, and the python templating. Workflow is ~300 lines lighter. 2. Concurrency: cancel-in-progress: false. Cancelling a run mid-analysis leaves the buffer file empty but lets the if: always() summary step fire, posting a misleading 'no high-confidence findings' comment. Letting both runs complete is the safer default. 3. Summary step branches on steps.review.conclusion. Only post a real findings count when the review step actually completed; on failure/cancellation/skip, post a transparent status that says the run didn't complete and a later run will replace it.
Claude Security Review: no high-confidence findings. (run) |
Why
The
pull_request_reviewtrigger was added to give maintainers a way to authorize the security review on community (fork) PRs by approving the review. It works on same-repo PRs but fails silently on fork PRs: GitHub does not inject repo/org secrets intopull_request_reviewruns when the PR head is a fork, regardless of the reviewer's access level. Today's run on PR #1299 (fork fromnotgitika) is the proof — the workflow fired, the auth gate passed, thenGenerate GitHub App tokencrashed withInput required and not supplied: app-idbecausevars.APP_IDandsecrets.APP_PRIVATE_KEYboth resolved to empty.The
pull_request_targetevent with thelabeledaction does receive secrets on fork PRs and is the only trigger that actually works end-to-end for community contributions.What changes
pull_request_reviewfrom the workflow triggers.isApprovalpath,review.state == 'approved'check).safe-to-reviewlabel and explain why approving review isn't the trigger.opened/reopened/synchronizeexactly as before.safe-to-reviewlabel, gated on the labeler's write access.Test plan
safe-to-reviewand confirm the workflow runs end-to-end (App token resolves, label adds, Bedrock review runs, inline comments + summary post).openedcontinues to auto-run as before.