Codex Review #217
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
| 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: | |
| # 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: | |
| 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 # covers PR review comments AND the PR conversation comment | |
| 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; | |
| } | |
| // Re-apply Stage 1's gate against the PR's *current* state: it may have | |
| // 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'); | |
| 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 <working-directory>`; give it a valid | |
| # (empty) git repo containing ONLY the diff — never any PR code. | |
| cd codex-payload | |
| # 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' | |
| # 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 | |
| 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 }} | |
| REVIEWED_SHA: ${{ github.event.workflow_run.head_sha }} | |
| with: | |
| github-token: ${{ github.token }} | |
| script: | | |
| const { owner, repo } = context.repo; | |
| const pull_number = Number(process.env.PR_NUMBER); | |
| const SUMMARY_MARKER = '<!-- codex-review-summary -->'; | |
| const INLINE_MARKER = '<!-- codex-review-inline -->'; | |
| 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 | |
| // 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; | |
| // 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, | |
| }); | |
| 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.user?.login === BOT && 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.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 { | |
| await github.rest.issues.createComment({ owner, repo, issue_number: pull_number, body }); | |
| } |