From a5c23e814a3516df8c33c19d87f95d4707a8c5fa Mon Sep 17 00:00:00 2001 From: Harsha Nalluru Date: Fri, 17 Jul 2026 23:47:50 -0700 Subject: [PATCH 1/4] ci(codex-review): review all PRs incl forks via two-stage split Fork PRs never got a Codex review: fork pull_request runs receive no secrets, so the old single workflow gated itself to same-repo branches. Split into two stages: - Stage 1 (codex-review.yml): runs on every PR incl forks with NO secrets and a read-only token. Checks out the merge ref, computes the diff, and uploads it as an artifact. Executes no PR code. - Stage 2 (codex-review-post.yml): triggered via workflow_run in the base repo context WITH secrets. Downloads the diff artifact, runs Codex over the diff text only (never checks out PR code), and posts the review. Fork code therefore never executes next to the OpenAI key. The artifact's PR number is bound to the run head_sha to block redirection to another PR. Drops the @codex-review comment trigger / trusted-commenter gate in favor of auto-review on all non-draft, non-bot PRs. --- .github/workflows/codex-review-post.yml | 281 ++++++++++++++++++++++++ .github/workflows/codex-review.yml | 281 ++++-------------------- 2 files changed, 322 insertions(+), 240 deletions(-) create mode 100644 .github/workflows/codex-review-post.yml diff --git a/.github/workflows/codex-review-post.yml b/.github/workflows/codex-review-post.yml new file mode 100644 index 00000000..e22c410c --- /dev/null +++ b/.github/workflows/codex-review-post.yml @@ -0,0 +1,281 @@ +name: Codex Review + +# Stage 2 of the two-stage Codex PR review. +# +# Triggered by the completion of Stage 1 (codex-review.yml) via `workflow_run`, +# so it runs in the BASE repository context WITH access to secrets — even for +# fork PRs. It NEVER checks out PR code: it only reads the pre-computed diff +# artifact from Stage 1, so untrusted fork code can never execute next to the +# OpenAI key. That structural guarantee is what makes reviewing fork PRs safe. +# +# Residual risk: Codex still reads the diff as *data*, so a prompt-injection +# payload in the diff could in principle try to coax the key into a review +# comment (the action keeps the key behind a proxy, but read-only Codex can reach +# process memory). Use a scoped, low-limit OPENAI_API_KEY for this workflow. + +on: + workflow_run: + workflows: ["Codex Review (collect diff)"] + types: [completed] + +permissions: + contents: read + +concurrency: + group: codex-review-post-${{ github.event.workflow_run.head_sha }} + cancel-in-progress: true + +jobs: + review: + if: github.event.workflow_run.conclusion == 'success' + runs-on: ubuntu-latest + permissions: + contents: read + actions: read # download-artifact needs this to read another run's artifact + pull-requests: write + issues: write + steps: + - name: Download Stage 1 diff artifact + uses: actions/download-artifact@v4 + with: + name: codex-review-payload + path: codex-payload + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ github.token }} + + - name: Resolve and verify PR + id: pr + uses: actions/github-script@v7 + env: + RUN_HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + with: + github-token: ${{ github.token }} + script: | + const fs = require('fs'); + const raw = fs.readFileSync('codex-payload/pr-number.txt', 'utf8').trim(); + if (!/^\d+$/.test(raw)) { + core.setFailed(`Invalid PR number in artifact: "${raw}"`); + return; + } + const pull_number = Number(raw); + const { owner, repo } = context.repo; + const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number }); + // Bind the artifact to a real PR: the triggering run's head_sha is set by + // GitHub (not the fork), so requiring it to equal the PR head prevents a + // tampered artifact from redirecting the review onto an unrelated PR. + if (pr.head.sha !== process.env.RUN_HEAD_SHA) { + core.setFailed( + `PR #${pull_number} head ${pr.head.sha} != run head ${process.env.RUN_HEAD_SHA}; refusing to post.`, + ); + return; + } + if (pr.state !== 'open') { + core.notice(`PR #${pull_number} is ${pr.state}; skipping review.`); + core.setOutput('skip', 'true'); + return; + } + core.setOutput('skip', 'false'); + core.setOutput('number', String(pull_number)); + + - name: Prepare working directory + if: steps.pr.outputs.skip != 'true' + run: | + set -euo pipefail + # codex-action runs `codex exec --cd `; give it a valid + # (empty) git repo containing ONLY the diff — never any PR code. + cd codex-payload + test -s pr.diff || { echo "pr.diff missing or empty"; exit 1; } + git init -q + + - name: Run Codex on the diff + id: run_codex + if: steps.pr.outputs.skip != 'true' + uses: openai/codex-action@v1 + with: + openai-api-key: ${{ secrets.OPENAI_API_KEY }} + model: gpt-5.5 + effort: xhigh + sandbox: read-only + working-directory: codex-payload + output-schema: | + { + "type": "object", + "additionalProperties": false, + "properties": { + "summary": { "type": "string" }, + "findings": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "properties": { + "path": { "type": "string" }, + "line": { "type": "integer" }, + "severity": { "type": "string", "enum": ["blocking", "consider"] }, + "comment": { "type": "string" } + }, + "required": ["path", "line", "severity", "comment"] + } + } + }, + "required": ["summary", "findings"] + } + prompt: | + You are reviewing a pull request in ${{ github.repository }}. + + The complete set of changes is the unified diff in the file `pr.diff` in + your working directory (run `cat pr.diff`). It is a `git diff` of + base...head. Review ONLY those changes. Report high-signal findings only. + + Correctness & safety: + - logic errors, unhandled edge cases, broken assumptions + - security vulnerabilities + - data loss, concurrency hazards, resource leaks + + Design & code quality: + - the soundness of the overall approach, not just line-level bugs + - elegance: is there a simpler, cleaner way to achieve the same result? + - abstraction: prefer the most general clean abstraction that fits the problem, + without over-engineering for cases that don't exist + - redundancy: flag duplicated logic, dead code, and anything that violates DRY + + Skip pure formatting and style nits. + + Report at most the 5 most important findings. Consolidate an issue that + recurs in several places into one finding at the most representative location. + + Output JSON matching the provided schema: + - `summary`: one or two sentences on the PR overall. If there are no real + issues, set summary to "No issues found." and findings to []. + - `findings[].path`: repository-relative file path, exactly as it appears in + the diff (the path after `+++ b/`). + - `findings[].line`: the line number in the NEW (post-change) version of the + file. It MUST be a line the PR adds or modifies. + - `findings[].severity`: "blocking" or "consider". + - `findings[].comment`: markdown review comment with a short code snippet and a + concrete fix. + + - name: Post inline review + if: steps.pr.outputs.skip != 'true' && steps.run_codex.outputs.final-message != '' + uses: actions/github-script@v7 + env: + CODEX_RESULT: ${{ steps.run_codex.outputs.final-message }} + PR_NUMBER: ${{ steps.pr.outputs.number }} + with: + github-token: ${{ github.token }} + script: | + const { owner, repo } = context.repo; + const pull_number = Number(process.env.PR_NUMBER); + const SUMMARY_MARKER = ''; + const INLINE_MARKER = ''; + const MAX_COMMENTS = 5; + + // Parse Codex JSON. With --output-schema the result is already pure JSON, + // and its findings may contain fenced code blocks, so never grab an inner + // fence: parse the whole string first, then a fence wrapping the whole + // string, then fall back to the outermost braces. + function parseResult(raw) { + if (!raw) return null; + const tryParse = (s) => { try { return JSON.parse(s); } catch { return null; } }; + const trimmed = raw.trim(); + let out = tryParse(trimmed); + if (out) return out; + const fence = trimmed.match(/^```(?:json)?\s*\n?([\s\S]*?)\n?```$/); + if (fence) { out = tryParse(fence[1].trim()); if (out) return out; } + const a = trimmed.indexOf('{'), b = trimmed.lastIndexOf('}'); + if (a !== -1 && b > a) return tryParse(trimmed.slice(a, b + 1)); + return null; + } + const result = parseResult(process.env.CODEX_RESULT); + if (!result) { core.setFailed('Could not parse Codex output as JSON.'); return; } + + const summary = (result.summary || '').trim(); + const findings = (Array.isArray(result.findings) ? result.findings : []) + .slice(0, MAX_COMMENTS); + + // Build the set of (path -> commentable new-file line numbers) from the diff. + const pr = await github.rest.pulls.get({ owner, repo, pull_number }); + const headSha = pr.data.head.sha; + const files = await github.paginate(github.rest.pulls.listFiles, { + owner, repo, pull_number, per_page: 100, + }); + const commentable = new Map(); + for (const f of files) { + if (!f.patch) continue; + const lines = new Set(); + let newLine = 0; + for (const ln of f.patch.split('\n')) { + const h = ln.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/); + if (h) { newLine = parseInt(h[1], 10); continue; } + if (ln.startsWith('\\')) continue; // "\ No newline at end of file" + if (ln.startsWith('+')) { lines.add(newLine); newLine++; } + else if (ln.startsWith('-')) { /* removed line, no new-side number */ } + else { newLine++; } // context line + } + commentable.set(f.filename, lines); + } + + // Split findings into inline-able vs. orphans (lines not in the diff). + const inline = [], orphans = []; + for (const fnd of findings) { + const sev = (fnd.severity || 'consider').toUpperCase(); + const set = commentable.get(fnd.path); + if (set && set.has(fnd.line)) { + inline.push({ + path: fnd.path, line: fnd.line, side: 'RIGHT', + body: `${INLINE_MARKER}\n**${sev}** ${fnd.comment}`, + }); + } else { + orphans.push({ ...fnd, sev }); + } + } + + // Always clear our prior inline comments first, so findings resolved in a + // later push disappear even when this run produces no inline comments. + try { + const prior = await github.paginate(github.rest.pulls.listReviewComments, { + owner, repo, pull_number, per_page: 100, + }); + for (const c of prior) { + if (c.body && c.body.includes(INLINE_MARKER)) { + try { await github.rest.pulls.deleteReviewComment({ owner, repo, comment_id: c.id }); } + catch {} + } + } + } catch (e) { core.warning(`Could not clean prior inline comments: ${e.message}`); } + + // Post this run's inline comments. If it fails, fold them into the summary. + let inlinePosted = false; + if (inline.length) { + try { + await github.rest.pulls.createReview({ + owner, repo, pull_number, commit_id: headSha, + event: 'COMMENT', comments: inline, + }); + inlinePosted = true; + } catch (err) { + core.warning(`Inline review failed (${err.status || ''}); folding into the summary.`); + } + } + + // Build the rolling summary comment. + const leftover = inlinePosted + ? orphans + : findings.map(f => ({ ...f, sev: (f.severity || 'consider').toUpperCase() })); + let body = `${SUMMARY_MARKER}\n### Codex review\n\n` + + (summary || (findings.length ? 'See inline comments.' : 'No issues found.')); + if (leftover.length) { + body += `\n\n**${inlinePosted ? 'Findings not on changed lines' : 'Findings'}:**\n`; + for (const o of leftover) body += `\n- \`${o.path}:${o.line}\` **${o.sev}** ${o.comment}`; + } + + // Upsert one rolling summary comment instead of stacking on each push. + const comments = await github.paginate(github.rest.issues.listComments, { + owner, repo, issue_number: pull_number, per_page: 100, + }); + const existing = comments.find(c => c.body && c.body.includes(SUMMARY_MARKER)); + if (existing) { + await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body }); + } else { + await github.rest.issues.createComment({ owner, repo, issue_number: pull_number, body }); + } diff --git a/.github/workflows/codex-review.yml b/.github/workflows/codex-review.yml index 77957113..331226b8 100644 --- a/.github/workflows/codex-review.yml +++ b/.github/workflows/codex-review.yml @@ -1,259 +1,60 @@ -name: Codex PR Review +name: Codex Review (collect diff) + +# Stage 1 of the two-stage Codex PR review. +# +# Runs on EVERY pull request — including forks — but with NO secrets and a +# read-only token, so it is safe against untrusted code. It performs no build or +# install: it checks out the PR merge ref, computes the diff, and hands it to the +# privileged Stage 2 (codex-review-post.yml) via an artifact consumed through +# `workflow_run`. Because fork code never executes here and no secret is present, +# a malicious PR has nothing to steal or abuse. +# +# NOTE: `workflow_run` only fires for the copy of these workflows on the default +# branch, so Codex review activates once this pair is merged to main (the PR that +# introduces it will not review itself). on: pull_request: types: [opened, synchronize, reopened] - issue_comment: - types: [created] + +permissions: + contents: read concurrency: - group: codex-review-${{ github.event.pull_request.number || github.event.issue.number }} + group: codex-review-${{ github.event.pull_request.number }} cancel-in-progress: true jobs: - authorize: - # Trigger gate: PR events always, comment events only for "@codex review" from a - # trusted commenter. The step then restricts to same-repo (non-fork) PRs so a - # public fork can never reach the privileged checkout below. - if: > - github.event_name == 'pull_request' || - (github.event.issue.pull_request != null && - contains(github.event.comment.body, '@codex review') && - contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)) - runs-on: ubuntu-latest - permissions: - pull-requests: read - outputs: - ok: ${{ steps.gate.outputs.ok }} - pr: ${{ steps.gate.outputs.pr }} - steps: - - id: gate - uses: actions/github-script@v7 - with: - github-token: ${{ github.token }} - script: | - const prNum = context.payload.pull_request?.number ?? context.payload.issue?.number; - if (!prNum) { core.setOutput('ok', 'false'); return; } - const { data: pr } = await github.rest.pulls.get({ - owner: context.repo.owner, repo: context.repo.repo, pull_number: prNum, - }); - const sameRepo = !!pr.head.repo && pr.head.repo.full_name === pr.base.repo.full_name; - core.setOutput('pr', String(prNum)); - core.setOutput('ok', sameRepo ? 'true' : 'false'); - if (!sameRepo) { - core.notice(`Skipping Codex review: PR #${prNum} is from a fork; only same-repo branches are reviewed.`); - } - - review: - needs: authorize - if: needs.authorize.outputs.ok == 'true' + collect-diff: + # Skip drafts and bot-authored PRs; every other PR (incl. forks) is reviewed. + if: github.event.pull_request.draft == false && github.event.pull_request.user.type != 'Bot' runs-on: ubuntu-latest permissions: contents: read - outputs: - result: ${{ steps.run_codex.outputs.final-message }} steps: - - uses: actions/checkout@v4 + - name: Check out PR merge ref + uses: actions/checkout@v4 with: - ref: refs/pull/${{ needs.authorize.outputs.pr }}/merge + ref: refs/pull/${{ github.event.pull_request.number }}/merge fetch-depth: 0 persist-credentials: false - - name: Run Codex - id: run_codex - uses: openai/codex-action@v1 - with: - openai-api-key: ${{ secrets.OPENAI_API_KEY }} - model: gpt-5.5 - effort: xhigh - sandbox: read-only - output-schema: | - { - "type": "object", - "additionalProperties": false, - "properties": { - "summary": { "type": "string" }, - "findings": { - "type": "array", - "items": { - "type": "object", - "additionalProperties": false, - "properties": { - "path": { "type": "string" }, - "line": { "type": "integer" }, - "severity": { "type": "string", "enum": ["blocking", "consider"] }, - "comment": { "type": "string" } - }, - "required": ["path", "line", "severity", "comment"] - } - } - }, - "required": ["summary", "findings"] - } - prompt: | - You are reviewing pull request #${{ needs.authorize.outputs.pr }} in ${{ github.repository }}. - - The PR's changes are exactly the diff between the merge commit's two parents. - Run `git diff HEAD^1 HEAD^2` to see everything that changed, and - `git diff HEAD^1 HEAD^2 -- ` to focus on a single file. - - Review ONLY those changes. Report high-signal findings only. - - Correctness & safety: - - logic errors, unhandled edge cases, broken assumptions - - security vulnerabilities - - data loss, concurrency hazards, resource leaks - - Design & code quality: - - the soundness of the overall approach, not just line-level bugs - - elegance: is there a simpler, cleaner way to achieve the same result? - - abstraction: prefer the most general clean abstraction that fits the problem, - without over-engineering for cases that don't exist - - redundancy: flag duplicated logic, dead code, and anything that violates DRY - - Skip pure formatting and style nits. - - Report at most the 5 most important findings. Consolidate an issue that - recurs in several places into one finding at the most representative location. - - Output JSON matching the provided schema: - - `summary`: one or two sentences on the PR overall. If there are no real - issues, set summary to "No issues found." and findings to []. - - `findings[].path`: repository-relative file path, exactly as git reports it. - - `findings[].line`: the line number in the NEW (post-change) version of the - file. It MUST be a line the PR adds or modifies. - - `findings[].severity`: "blocking" or "consider". - - `findings[].comment`: markdown review comment with a short code snippet and a - concrete fix. - - post_review: - needs: [authorize, review] - if: needs.review.outputs.result != '' - runs-on: ubuntu-latest - permissions: - issues: write - pull-requests: write - steps: - - name: Post inline review - uses: actions/github-script@v7 + - name: Compute PR diff and metadata env: - CODEX_RESULT: ${{ needs.review.outputs.result }} - PR_NUMBER: ${{ needs.authorize.outputs.pr }} + PR_NUMBER: ${{ github.event.pull_request.number }} + run: | + set -euo pipefail + mkdir -p codex-payload + # On the PR merge ref, HEAD^1 is the base tip and HEAD^2 is the PR head; + # the diff between them is exactly the PR's changes. + git diff HEAD^1 HEAD^2 > codex-payload/pr.diff + printf '%s' "$PR_NUMBER" > codex-payload/pr-number.txt + echo "Collected $(wc -l < codex-payload/pr.diff) diff lines for PR #${PR_NUMBER}" + + - name: Upload review payload + uses: actions/upload-artifact@v4 with: - github-token: ${{ github.token }} - script: | - const { owner, repo } = context.repo; - const pull_number = Number(process.env.PR_NUMBER); - const SUMMARY_MARKER = ''; - const INLINE_MARKER = ''; - const MAX_COMMENTS = 5; - - // Parse Codex JSON. With --output-schema the result is already pure JSON, - // and its findings may contain fenced code blocks, so never grab an inner - // fence: parse the whole string first, then a fence wrapping the whole - // string, then fall back to the outermost braces. - function parseResult(raw) { - if (!raw) return null; - const tryParse = (s) => { try { return JSON.parse(s); } catch { return null; } }; - const trimmed = raw.trim(); - let out = tryParse(trimmed); - if (out) return out; - const fence = trimmed.match(/^```(?:json)?\s*\n?([\s\S]*?)\n?```$/); - if (fence) { out = tryParse(fence[1].trim()); if (out) return out; } - const a = trimmed.indexOf('{'), b = trimmed.lastIndexOf('}'); - if (a !== -1 && b > a) return tryParse(trimmed.slice(a, b + 1)); - return null; - } - const result = parseResult(process.env.CODEX_RESULT); - if (!result) { core.setFailed('Could not parse Codex output as JSON.'); return; } - - const summary = (result.summary || '').trim(); - const findings = (Array.isArray(result.findings) ? result.findings : []) - .slice(0, MAX_COMMENTS); - - // Build the set of (path -> commentable new-file line numbers) from the diff. - const pr = await github.rest.pulls.get({ owner, repo, pull_number }); - const headSha = pr.data.head.sha; - const files = await github.paginate(github.rest.pulls.listFiles, { - owner, repo, pull_number, per_page: 100, - }); - const commentable = new Map(); - for (const f of files) { - if (!f.patch) continue; - const lines = new Set(); - let newLine = 0; - for (const ln of f.patch.split('\n')) { - const h = ln.match(/^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/); - if (h) { newLine = parseInt(h[1], 10); continue; } - if (ln.startsWith('\\')) continue; // "\ No newline at end of file" - if (ln.startsWith('+')) { lines.add(newLine); newLine++; } - else if (ln.startsWith('-')) { /* removed line, no new-side number */ } - else { newLine++; } // context line - } - commentable.set(f.filename, lines); - } - - // Split findings into inline-able vs. orphans (lines not in the diff). - const inline = [], orphans = []; - for (const fnd of findings) { - const sev = (fnd.severity || 'consider').toUpperCase(); - const set = commentable.get(fnd.path); - if (set && set.has(fnd.line)) { - inline.push({ - path: fnd.path, line: fnd.line, side: 'RIGHT', - body: `${INLINE_MARKER}\n**${sev}** ${fnd.comment}`, - }); - } else { - orphans.push({ ...fnd, sev }); - } - } - - // Always clear our prior inline comments first, so findings resolved in a - // later push disappear even when this run produces no inline comments. - try { - const prior = await github.paginate(github.rest.pulls.listReviewComments, { - owner, repo, pull_number, per_page: 100, - }); - for (const c of prior) { - if (c.body && c.body.includes(INLINE_MARKER)) { - try { await github.rest.pulls.deleteReviewComment({ owner, repo, comment_id: c.id }); } - catch {} - } - } - } catch (e) { core.warning(`Could not clean prior inline comments: ${e.message}`); } - - // Post this run's inline comments. If it fails, fold them into the summary. - let inlinePosted = false; - if (inline.length) { - try { - await github.rest.pulls.createReview({ - owner, repo, pull_number, commit_id: headSha, - event: 'COMMENT', comments: inline, - }); - inlinePosted = true; - } catch (err) { - core.warning(`Inline review failed (${err.status || ''}); folding into the summary.`); - } - } - - // Build the rolling summary comment. - const leftover = inlinePosted - ? orphans - : findings.map(f => ({ ...f, sev: (f.severity || 'consider').toUpperCase() })); - let body = `${SUMMARY_MARKER}\n### Codex review\n\n` + - (summary || (findings.length ? 'See inline comments.' : 'No issues found.')); - if (leftover.length) { - body += `\n\n**${inlinePosted ? 'Findings not on changed lines' : 'Findings'}:**\n`; - for (const o of leftover) body += `\n- \`${o.path}:${o.line}\` **${o.sev}** ${o.comment}`; - } - - // Upsert one rolling summary comment instead of stacking on each push. - const comments = await github.paginate(github.rest.issues.listComments, { - owner, repo, issue_number: pull_number, per_page: 100, - }); - const existing = comments.find(c => c.body && c.body.includes(SUMMARY_MARKER)); - if (existing) { - await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body }); - } else { - await github.rest.issues.createComment({ owner, repo, issue_number: pull_number, body }); - } + name: codex-review-payload + path: codex-payload/ + retention-days: 1 + if-no-files-found: error From 696f685669567c52f6671eddad5964e8a999377b Mon Sep 17 00:00:00 2001 From: Harsha Nalluru Date: Sat, 18 Jul 2026 00:04:50 -0700 Subject: [PATCH 2/4] ci(codex-review): address Copilot review feedback - Trigger Stage 1 on ready_for_review so a draft marked ready is reviewed immediately, matching the non-draft gate. - Diff the PR head against the base merge-base instead of checking out the merge ref, so PRs with conflicts (no merge ref) are still reviewed. - Re-check state/draft/bot in Stage 2 before posting, in case the PR changed between diff collection and review. --- .github/workflows/codex-review-post.yml | 7 +++++-- .github/workflows/codex-review.yml | 19 +++++++++++++------ 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/.github/workflows/codex-review-post.yml b/.github/workflows/codex-review-post.yml index e22c410c..adf8b451 100644 --- a/.github/workflows/codex-review-post.yml +++ b/.github/workflows/codex-review-post.yml @@ -69,8 +69,11 @@ jobs: ); return; } - if (pr.state !== 'open') { - core.notice(`PR #${pull_number} is ${pr.state}; skipping review.`); + // Re-apply Stage 1's gate against the PR's *current* state: it may have + // been closed, converted to draft, or reassigned since the diff was + // collected. Keeps posting behavior consistent with what gets reviewed. + if (pr.state !== 'open' || pr.draft || pr.user?.type === 'Bot') { + core.notice(`PR #${pull_number} is not an open, ready, human PR; skipping review.`); core.setOutput('skip', 'true'); return; } diff --git a/.github/workflows/codex-review.yml b/.github/workflows/codex-review.yml index 331226b8..616dd44c 100644 --- a/.github/workflows/codex-review.yml +++ b/.github/workflows/codex-review.yml @@ -15,7 +15,9 @@ name: Codex Review (collect diff) on: pull_request: - types: [opened, synchronize, reopened] + # ready_for_review is included so a PR opened as a draft is reviewed the + # moment it becomes non-draft (the collect-diff job gates on draft == false). + types: [opened, synchronize, reopened, ready_for_review] permissions: contents: read @@ -32,22 +34,27 @@ jobs: permissions: contents: read steps: - - name: Check out PR merge ref + - name: Check out PR head uses: actions/checkout@v4 with: - ref: refs/pull/${{ github.event.pull_request.number }}/merge + # The head ref always exists; the merge ref does NOT when the PR has + # conflicts, which would silently skip the review. Diffing the head + # against the base's merge-base reproduces GitHub's PR diff regardless. + ref: refs/pull/${{ github.event.pull_request.number }}/head fetch-depth: 0 persist-credentials: false - name: Compute PR diff and metadata env: PR_NUMBER: ${{ github.event.pull_request.number }} + BASE_REF: ${{ github.event.pull_request.base.ref }} run: | set -euo pipefail mkdir -p codex-payload - # On the PR merge ref, HEAD^1 is the base tip and HEAD^2 is the PR head; - # the diff between them is exactly the PR's changes. - git diff HEAD^1 HEAD^2 > codex-payload/pr.diff + # Fetch the base branch, then take the merge-base ("...") diff — the exact + # change set GitHub shows, and robust to an un-mergeable PR. + git fetch --no-tags origin "$BASE_REF" + git diff FETCH_HEAD...HEAD > codex-payload/pr.diff printf '%s' "$PR_NUMBER" > codex-payload/pr-number.txt echo "Collected $(wc -l < codex-payload/pr.diff) diff lines for PR #${PR_NUMBER}" From 0721c41f862b3a63d0a5026e2f9dce9285f94a0e Mon Sep 17 00:00:00 2001 From: Harsha Nalluru Date: Sat, 18 Jul 2026 00:14:16 -0700 Subject: [PATCH 3/4] ci(codex-review): address cubic review (P0/P1/P2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - allow-users: '*' so codex-action's default write-access gate doesn't reject fork/non-collaborator PR authors (the whole point of the workflow). - Pin codex-action to the v1 commit SHA; it holds OPENAI_API_KEY. - Serialize Stage 2 per PR source branch (not per SHA) and skip posting if the PR head advanced past the reviewed commit — no stale overwrites. - Accept an empty pr.diff so stale findings still get cleared. - Only delete/upsert comments authored by github-actions[bot]. Rejected: cubic's 'head_sha is the merge SHA' P0 — verified against live GitHub data that workflow_run.head_sha == pr.head.sha for fork PRs. --- .github/workflows/codex-review-post.yml | 29 ++++++++++++++++++++----- 1 file changed, 24 insertions(+), 5 deletions(-) diff --git a/.github/workflows/codex-review-post.yml b/.github/workflows/codex-review-post.yml index adf8b451..e5b3bf50 100644 --- a/.github/workflows/codex-review-post.yml +++ b/.github/workflows/codex-review-post.yml @@ -22,7 +22,9 @@ permissions: contents: read concurrency: - group: codex-review-post-${{ github.event.workflow_run.head_sha }} + # Serialize per PR source (repo + branch), not per SHA, so a newer push cancels + # an in-flight older review instead of racing it to post stale findings. + group: codex-review-post-${{ github.event.workflow_run.head_repository.full_name }}-${{ github.event.workflow_run.head_branch }} cancel-in-progress: true jobs: @@ -87,15 +89,24 @@ jobs: # codex-action runs `codex exec --cd `; give it a valid # (empty) git repo containing ONLY the diff — never any PR code. cd codex-payload - test -s pr.diff || { echo "pr.diff missing or empty"; exit 1; } + # Allow an empty diff (e.g. a push reverted all changes) so the post step + # still clears stale prior findings; only a missing file is an error. + test -f pr.diff || { echo "pr.diff missing"; exit 1; } git init -q - name: Run Codex on the diff id: run_codex if: steps.pr.outputs.skip != 'true' - uses: openai/codex-action@v1 + # Pinned to the v1 commit SHA: this privileged job holds OPENAI_API_KEY, so a + # moved/compromised tag must not silently change what runs here. + uses: openai/codex-action@52fe01ec70a42f454c9d2ebd47598f9fd6893d56 # v1 with: openai-api-key: ${{ secrets.OPENAI_API_KEY }} + # This job reviews ALL PRs incl. forks; the action's default write-access + # gate would otherwise reject fork/non-collaborator authors. Safe to open + # here because security comes from this job never checking out PR code + # (only the diff artifact), not from the actor allowlist. + allow-users: "*" model: gpt-5.5 effort: xhigh sandbox: read-only @@ -164,6 +175,7 @@ jobs: env: CODEX_RESULT: ${{ steps.run_codex.outputs.final-message }} PR_NUMBER: ${{ steps.pr.outputs.number }} + REVIEWED_SHA: ${{ github.event.workflow_run.head_sha }} with: github-token: ${{ github.token }} script: | @@ -172,6 +184,7 @@ jobs: const SUMMARY_MARKER = ''; const INLINE_MARKER = ''; const MAX_COMMENTS = 5; + const BOT = 'github-actions[bot]'; // only ever touch comments we authored // Parse Codex JSON. With --output-schema the result is already pure JSON, // and its findings may contain fenced code blocks, so never grab an inner @@ -199,6 +212,12 @@ jobs: // Build the set of (path -> commentable new-file line numbers) from the diff. const pr = await github.rest.pulls.get({ owner, repo, pull_number }); const headSha = pr.data.head.sha; + // If the PR advanced past the commit we reviewed, a newer run is already + // handling the new head — don't post findings generated from a stale diff. + if (headSha !== process.env.REVIEWED_SHA) { + core.notice(`PR head moved ${process.env.REVIEWED_SHA} -> ${headSha}; skipping stale review.`); + return; + } const files = await github.paginate(github.rest.pulls.listFiles, { owner, repo, pull_number, per_page: 100, }); @@ -240,7 +259,7 @@ jobs: owner, repo, pull_number, per_page: 100, }); for (const c of prior) { - if (c.body && c.body.includes(INLINE_MARKER)) { + if (c.user?.login === BOT && c.body && c.body.includes(INLINE_MARKER)) { try { await github.rest.pulls.deleteReviewComment({ owner, repo, comment_id: c.id }); } catch {} } @@ -276,7 +295,7 @@ jobs: const comments = await github.paginate(github.rest.issues.listComments, { owner, repo, issue_number: pull_number, per_page: 100, }); - const existing = comments.find(c => c.body && c.body.includes(SUMMARY_MARKER)); + const existing = comments.find(c => c.user?.login === BOT && c.body && c.body.includes(SUMMARY_MARKER)); if (existing) { await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body }); } else { From 784be29529bc47a0050694b140839e7b7cd9e098 Mon Sep 17 00:00:00 2001 From: Harsha Nalluru Date: Sat, 18 Jul 2026 00:29:26 -0700 Subject: [PATCH 4/4] ci(codex-review): drop unused issues:write, fix stale comment wording - pull-requests: write already covers the PR conversation comment, so the issues: write scope was unnecessary (least privilege). - The state re-check inspects pr.user (author)/draft/state, not assignees; drop the misleading 'reassigned' wording. --- .github/workflows/codex-review-post.yml | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/.github/workflows/codex-review-post.yml b/.github/workflows/codex-review-post.yml index e5b3bf50..64999b28 100644 --- a/.github/workflows/codex-review-post.yml +++ b/.github/workflows/codex-review-post.yml @@ -34,8 +34,7 @@ jobs: permissions: contents: read actions: read # download-artifact needs this to read another run's artifact - pull-requests: write - issues: write + pull-requests: write # covers PR review comments AND the PR conversation comment steps: - name: Download Stage 1 diff artifact uses: actions/download-artifact@v4 @@ -72,8 +71,8 @@ jobs: return; } // Re-apply Stage 1's gate against the PR's *current* state: it may have - // been closed, converted to draft, or reassigned since the diff was - // collected. Keeps posting behavior consistent with what gets reviewed. + // been closed or converted to draft since the diff was collected. Keeps + // posting behavior consistent with what gets reviewed. if (pr.state !== 'open' || pr.draft || pr.user?.type === 'Bot') { core.notice(`PR #${pull_number} is not an open, ready, human PR; skipping review.`); core.setOutput('skip', 'true');