ci: block originator self-approval on docs-agent PRs#1265
Open
SahilAujla wants to merge 14 commits intomainfrom
Open
ci: block originator self-approval on docs-agent PRs#1265SahilAujla wants to merge 14 commits intomainfrom
SahilAujla wants to merge 14 commits intomainfrom
Conversation
Adds a workflow that fires on pull_request_review.submitted events for PRs authored by JackReacher0807 (the docs-agent's GH identity). When an approval lands, the workflow parses the PR body for the "## Requested by\n@<user>" attribution that docs-agent emits and dismisses the approval if the approver matches the attributed originator. Net effect: branch protection still only requires 1 approval, but that 1 approval cannot come from the person who originated the docs request via Slack — closing the loophole where a requester could self-merge their own docs-agent-authored PR. If attribution is missing (Slack email hidden, user not in the docs-agent's mapping, etc.), the workflow posts a one-time heads-up comment so reviewers know the rule isn't enforced for that PR. This workflow does NOT affect human-authored PRs — the if: clause restricts it to PRs where pull_request.user.login == 'JackReacher0807'. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…it msg Addresses two P1 codex review comments on PR #1262: 1. Under `set -euo pipefail`, the original `REQUESTED_BY=$(... | grep ...)` substitution aborted the step whenever grep found no match, so PRs without attribution would fail the workflow instead of taking the missing-attribution heads-up path. Added `|| true` to each substitution so the empty-string fallback runs as intended. 2. The original implementation read the @mention from the PR body, which anyone with write access (including the originator) can edit. They could silently strip their own @mention before clicking Approve and slip past the rule. Switched the source to the commit message — docs-agent emits a `Requested-by: @<github_username>` trailer on its first commit, which is GPG-signed (key AB0009C56564B53A on the JackReacher0807 account) and immutable without a force-push. A force-push would either drop the signature (originator doesn't have the key) or, if "Dismiss stale pull request approvals when new commits are pushed" is enabled, invalidate any standing approvals. PR body still carries a human-readable "## Requested by" section for reviewer context, but it's no longer authoritative for the CI rule. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… commits Addresses third P1 codex review comment on PR #1262. Previous version extracted the Requested-by trailer from any commit message in the PR. A collaborator with push access to the branch (or maintainer-edit on a fork PR) could add a commit with "Requested-by: @<other-person>" to redirect the policy at someone else, then approve their own request without being dismissed. Now the jq filter restricts trailer extraction to commits that are BOTH: 1. Authored on GitHub by JackReacher0807 (the docs-agent) 2. GPG-verified by GitHub (.commit.verification.verified == true) The originator can't forge an agent-signed commit because they don't have the agent's private GPG key. Force-pushing also won't help — the resulting commits would either lose verification or be authored by the force-pusher, either way dropping out of the filter. If the PR has zero verified+agent commits (e.g., all commits force-pushed over by a different author), REQUESTED_BY ends up empty and the existing missing-attribution path fires (warn comment + exit 0). This is the correct degraded behavior — the rule simply doesn't apply when we can't trust the attribution chain. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lenient regex for the Requested-by trailer to accept LLM-driven format drift. Previously matched only the exact form "Requested-by: @user". Now matches: Requested-by: @user (canonical) requested-by: @user Requested-By: @user requested by: @user Requested By: @user requested_by: @user Pattern: '[Rr]equested[-_ ][Bb]y[[:space:]]*:[[:space:]]*@[A-Za-z0-9_-]+' Why: the trailer is emitted by docs-agent (a Claude-backed LLM) on each commit message. Even with explicit format instructions in the system prompt, LLMs occasionally drift on minor punctuation/casing. Strict matching meant any drift fell into the missing-attribution degraded path, silently disabling the self-approval rule. The agent-and-verified commit filter from the previous fix still applies, so loosening the trailer match is safe — only commits authored by JackReacher0807 and GPG-verified are even considered. Companion change in the agent's system prompt (v13) adds a verify-before-push step: agent greps its own commit message for the trailer before push and amends if missing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… not just first Addresses fourth P1 codex review comment (id 3140965120) on PR #1262: "Bind Requested-by extraction to this PR, not any signed commit." Previous version took the FIRST Requested-by trailer found in any verified docs-agent commit. An attacker with push access to the branch could ADD a commit with "Requested-by: @<other-victim>" to redirect the policy away from themselves — the workflow would dismiss approvals from @other-victim, not from them. This version collects ALL trailers from verified docs-agent commits in the PR and dismisses if the approver matches ANY of them. The original real trailer is still in the set after an additive attack, so the real originator's approval is still dismissed. Comparisons are case-insensitive (GitHub logins are case-insensitive per the platform). Residual concern: a destructive force-push that REPLACES the entire history with a single attacker-chosen signed commit drops the original trailer entirely. That attack is mitigated at the branch-protection layer, not in this workflow: - "Require signed commits" rejects non-agent commits outright - "Dismiss stale pull request approvals when new commits are pushed" invalidates approvals on force-push, forcing the attacker to re-approve (which then fires this workflow against the new state) - "Restrict who can push to matching branches" limits the universe of potential attackers Workflow logic and branch protection together close the practical threat surface. A malicious repo maintainer who can disable both protections has bigger powers than self-approval anyway. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…s GPG fingerprint Addresses fifth P1 codex review comment (id 3140984637) on PR #1262: "Verify signer identity before trusting Requested-by trailers." Previous filter (`author.login == 'JackReacher0807' && verification.verified == true`) relied on GitHub's verification flag, which only confirms the signature is valid against SOME key on the JackReacher0807 account. If that account is compromised (or has multiple GPG keys uploaded over time), commits signed with any of those keys would pass — not just commits signed with the specific key we issued to docs-agent. This change pins verification to a specific full fingerprint: 48AB9EC753D58474E845E808AB0009C56564B53A The agent's public key is now checked into the repo at `.github/workflows/docs-agent-pubkey.asc` (it's public — no secret risk). The workflow imports it into an isolated gpg keyring, then for each commit extracts `verification.payload` and `verification.signature` from the API and re-verifies cryptographically. Only commits where gpg outputs `VALIDSIG <expected_fpr>` are trusted for trailer extraction. Net effect: - Account compromise that adds a non-pinned key → no longer trusted by this workflow (commits signed with other keys are excluded) - Replacement of the pinned key on the account → existing pinned-key signatures still verify (the public key is in the repo, not fetched from GitHub at runtime) - Author-email spoofing without the pinned private key → fails crypto verification The "missing-attribution" degraded path still fires when zero commits pass the pinned-key filter, posting the warning comment so reviewers apply judgment. Adds a sparse checkout of just the pubkey file (no need to clone the whole repo), keeping the workflow fast. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…I errors Addresses two more codex review comments on PR #1262: P1 (id 3141007384): "Fail closed when commit metadata fetch fails" Previously, `COMMITS_JSON="$(gh api ... || true)"` masked transient API failures (auth, rate limit, network) as "no commits found". The empty ALL_REQUESTED_BY then triggered the missing-attribution warning path and exited 0, leaving the approver's approval active even though we never validated who originated the PR. For a security gate this is the wrong default. Now: fetch is wrapped in a retry loop (3 attempts with backoff). If all attempts fail, the workflow dismisses the approval and exits 1. The approver can re-approve once the API recovers and the workflow re-runs. This makes the worst-case behavior fail-closed — never allow a self-approval to slip through because of an API hiccup. P2 (id 3141007387): "Paginate pull-request commit retrieval" GitHub's pull-request commits endpoint returns 30/page by default. PRs with many fixup commits (CI fixes, review feedback iterations) could span multiple pages, causing the trailer extraction to miss commits on later pages. If the originating commit's trailer happened to fall on a non-first page, the workflow would not see it in ALL_REQUESTED_BY and the originator's approval would not be dismissed. Now: `gh api --paginate -F per_page=100` paginates with the max page size, then `jq -s 'add'` merges the page arrays into a single array. Both fixes hardened in the same retry path: pagination is part of the fetch function, retries cover any pagination failure equally. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…n approvals on day one
Three bugs caught before any workflow run hit production. Two flagged
by codex review (P1 #3141497645, P1 #3141497647); the third I caught
during local end-to-end testing of the fixes.
1. gh api needs --method GET (codex #3141497645)
`gh api -F per_page=100 ...` defaults to POST per the gh manual
("If the request is not GET, automatically uses POST when -f/-F
are given"). The pull-request commits endpoint is GET-only; the
POST request hit a non-existent endpoint and returned 404,
sending every approval through the fail-closed dismissal path.
Net effect would have been: every approval on every docs-agent PR
silently dismissed, blocking all merges.
Fix: explicit --method GET.
2. VALIDSIG fingerprint match was on signing subkey, not primary
(codex #3141497647)
GPG's status-line format:
[GNUPG:] VALIDSIG <signing_key_fpr> <date> ... <primary_key_fpr>
When a key has signing subkeys (the default for keys generated
with `gpg --full-generate-key`), the FIRST fingerprint after
VALIDSIG is the signing subkey and the LAST field is the primary
key. Our pinned EXPECTED_FPR is the primary fingerprint, so
matching against the first field never matched. Every signed
commit was filtered out as untrusted; ALL_REQUESTED_BY ended
up empty; missing-attribution path fired on every PR; rule
never enforced.
Fix: extract the LAST field of the VALIDSIG line via awk and
compare against EXPECTED_FPR.
3. Payload needed trailing newline before gpg verification (caught
in local testing — not flagged by codex)
Git signs the commit object as raw bytes with a trailing \n.
GitHub's verification.payload preserves those bytes, but jq -r
decodes them as a string, and writing back via printf '%s' drops
the trailing newline. Without it, gpg outputs BADSIG on every
commit. Same downstream effect as #2.
Fix: write payload with printf '%s\n'.
End-to-end tested locally against PR #1263's docs-agent commit:
fetch returns the commit, gpg verifies, primary fpr matches
expected. Trailer extraction would proceed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex P1 (id 3141513483) plus three additional gaps caught in a full audit before pushing this commit. End-to-end tested locally against PRs #1261 (no trailer), #1263 (no trailer), and #1264 (trailer present) across 6 cases: originator dismissal (canonical + case-variant), non-originator allow, missing-attribution warn for various approvers. All 6 cases produce the expected outcome. 1. Add `contents: read` permission (codex P1) Setting `permissions:` explicitly removes any unlisted permission (sets to `none`). Without `contents: read`, `actions/checkout` fails on private/internal repos before the dismissal logic runs, so the self-approval rule is never enforced. Adds the read-only permission needed for sparse checkout of the pubkey file. 2. Verify pubkey loaded after gpg --import (audit gap) If the pubkey import silently succeeded-but-loaded-no-keys (corrupted file, empty file, etc.), every commit's signature verification would fail the trust filter, ALL_REQUESTED_BY would end up empty, and the workflow would silently degrade to the missing-attribution warn-only path on EVERY PR — never enforcing. Now: `gpg --list-keys $EXPECTED_FPR` after import; if absent, dismiss the approval and exit 1. 3. Paginate the comments-list call used for duplicate-warning suppression (audit gap) PRs that accumulate >30 comments (over weeks) could push the existing warning comment off the first page; without pagination the duplicate-suppression check would miss it and post a duplicate every approval. Adds `--paginate -F per_page=100` + `jq -s 'add'`. 4. Explicit `event=DISMISS` on dismissals API calls (audit gap) The PUT /dismissals endpoint defaults `event` per most-recent docs but historical behavior has varied. Setting it explicitly avoids relying on an undocumented default for a security-critical call. Test matrix run locally before this commit: PR #1264 approver=SahilAujla expected=dismiss actual=dismiss OK PR #1264 approver=danielcoyle expected=allow actual=allow OK PR #1264 approver=SAHILAUJLA expected=dismiss actual=dismiss OK PR #1263 approver=SahilAujla expected=warn actual=warn OK PR #1263 approver=anyone expected=warn actual=warn OK PR #1261 approver=SahilAujla expected=warn actual=warn OK Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex flagged a P1 where `gpg --verify` returning non-zero (BADSIG on a tampered payload, NO_PUBKEY for an unrelated signer, malformed signature data) would abort the step under `set -euo pipefail` before the else branch could run — turning "skip an untrusted commit" into a hard workflow failure that prevented the dismissal logic from running for legitimately verified commits later in the loop. Wrap the verify in `if gpg_status="$(...)"; then ...` so a non-zero exit just falls through to the SKIP path with primary_fpr="". Pre-push audit also caught four init-path gaps where a transient or silent failure could have left an approval intact: 1. gpg --import itself can fail (corrupted .asc, wrong format) — now wrapped in `if !` and fail-closed. 2. .github/workflows/docs-agent-pubkey.asc could be deleted from main — added existence check before --import, fail-closed if missing. 3. Dismissal API calls were single-attempt — added dismiss_with_retry() with 3 attempts and exponential backoff; used everywhere we dismiss. 4. Originator-match dismissal previously appended `|| true` — now exit 1 if all 3 retries fail, so a transient API hiccup surfaces instead of silently leaving the approval intact. Local test matrix (PR #1264 with Requested-by: @SahilAujla, PR #1263 with no trailer, PR #1261 likewise, plus PR #1262 which has only human-authored commits): all 7 cases produce the expected outcome (dismiss / allow / warn-only). Tampered-payload BADSIG test confirms the step survives instead of aborting. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Made-with: Cursor
Made-with: Cursor
🔗 Preview Mode
|
🔍 Link CheckStatus: ⏭️ Skipped (no content changes) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04cd571278
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Made-with: Cursor
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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
1 participant
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
Requested-bytrailer on a pinned-key-verified docs-agent commit.Requested-byattribution, the workflow logs and allows the approval because there is no originator to enforce.JackReacher0807.Behavior
Requested-bytrailer: approval is left intact.JackReacher0807: workflow job is skipped.