Sync Codex review workflow to latest #37
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 PR Review | |
| on: | |
| pull_request: | |
| branches: [main] | |
| types: [opened, synchronize, reopened] | |
| issue_comment: | |
| types: [created] | |
| # Only collide for runs that review the same PR (a push or a trusted @codex review | |
| # on PR #N share group codex-review-N). The comment branch mirrors the full authorize | |
| # gate, including author_association, so an untrusted comment can never land in the | |
| # shared group and cancel an in-flight review; it falls back to the unique run id. | |
| concurrency: | |
| group: >- | |
| codex-review-${{ | |
| (github.event_name == 'pull_request' && github.event.pull_request.number) || | |
| (github.event_name == 'issue_comment' && | |
| github.event.issue.pull_request != null && | |
| contains(github.event.comment.body, '@codex review') && | |
| contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association) && | |
| github.event.issue.number) || | |
| github.run_id }} | |
| cancel-in-progress: true | |
| jobs: | |
| authorize: | |
| # Trigger gate: PR events and "@codex review" from a trusted commenter. The | |
| # step then restricts to same-repo (non-fork) PRs so fork-controlled content | |
| # can never reach the secret-backed checkout/review 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' | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: read | |
| outputs: | |
| result: ${{ steps.run_codex.outputs.final-message }} | |
| steps: | |
| - uses: actions/checkout@v4 | |
| with: | |
| ref: refs/pull/${{ needs.authorize.outputs.pr }}/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 a senior staff engineer doing a deep, context-aware review of pull | |
| request #${{ needs.authorize.outputs.pr }} in ${{ github.repository }}. Do not | |
| skim the diff: investigate the whole repository to judge each change in the | |
| context of the code around it. The full repo is checked out at the PR merge | |
| commit and you have a read-only shell, so actually explore. | |
| Step 1 - establish what changed. The checkout is the PR merge commit, whose | |
| parents are the base tip (HEAD^1) and the PR head (HEAD^2). Diff from the | |
| MERGE BASE so you see only this PR's changes, never commits that landed on the | |
| base branch after the branch was cut (a plain `HEAD^1 HEAD^2` diff is wrong for | |
| stale branches). This also matches the line numbers GitHub maps comments to: | |
| - `BASE=$(git merge-base HEAD^1 HEAD^2)` | |
| - `git diff "$BASE" HEAD^2 --stat` for the shape, then | |
| `git diff "$BASE" HEAD^2 -- <path>` per file for the exact changes. | |
| Step 2 - crawl the repository for context. Do not review a hunk in isolation: | |
| - Open each changed file IN FULL (not just the diff window) to understand | |
| surrounding logic, invariants, and intent. | |
| - Trace every symbol the change touches outward through the repo. For each | |
| changed function/class/constant/export/route/env var, find its definition | |
| and ALL usages with `git grep -n` / ripgrep, and read those call sites. | |
| - Follow imports both directions: what this code depends on, and what depends | |
| on it. Read the real implementations being called, never assume behavior. | |
| - Pull in the related files the diff did NOT touch but should be checked | |
| against: callers, tests, type definitions, schemas/migrations, configs, | |
| API contracts, fixtures, and docs. | |
| Step 3 - assess impact across the whole repo, not just the changed lines: | |
| - Ripple effects: does this change break or silently require updates in | |
| callers, tests, types, serialization, DB schema, or public contracts | |
| elsewhere in the repo? Did the PR update everything it needed to? | |
| - Consistency: does it match this repo's established conventions and patterns | |
| (naming, error handling, logging, layering, auth)? Cite the existing pattern. | |
| - Correctness & safety: logic errors, unhandled edge cases, broken | |
| assumptions, security holes, data loss, concurrency hazards, resource leaks. | |
| - Design: soundness of the overall approach; a simpler/cleaner way; the right | |
| abstraction without over-engineering; duplicated logic / dead code / DRY. | |
| Report high-signal findings only; skip pure formatting and style nits. Report | |
| at most the 5 most important findings, consolidating a recurring issue into a | |
| single finding at the most representative location. A finding may be rooted in | |
| an untouched file (e.g. a caller this PR breaks): anchor it to the changed line | |
| that causes the problem and name the affected file(s) in the comment. | |
| Output JSON matching the provided schema: | |
| - `summary`: one or two sentences on the PR overall, reflecting repo-wide | |
| impact. 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 that explains the repo-wide | |
| impact (reference the specific other files/call sites you inspected), 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 | |
| env: | |
| CODEX_RESULT: ${{ needs.review.outputs.result }} | |
| PR_NUMBER: ${{ needs.authorize.outputs.pr }} | |
| 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; | |
| // Only ever mutate comments this workflow itself authored, identified by | |
| // the GITHUB_TOKEN bot actor plus our hidden marker. Never touch a human's | |
| // (or another bot's) comment even if it happens to quote a marker. | |
| const isOurComment = (c, marker) => | |
| c.user?.type === 'Bot' && | |
| c.user?.login === 'github-actions[bot]' && | |
| c.body?.includes(marker); | |
| // 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 }); | |
| } | |
| } | |
| // Clear our own prior inline comments so resolved findings disappear on a | |
| // later push, but never delete a thread that has replies (someone engaged | |
| // with it) or a comment we did not author. | |
| try { | |
| const prior = await github.paginate(github.rest.pulls.listReviewComments, { | |
| owner, repo, pull_number, per_page: 100, | |
| }); | |
| const repliedTo = new Set(prior.filter(c => c.in_reply_to_id).map(c => c.in_reply_to_id)); | |
| for (const c of prior) { | |
| if (isOurComment(c, INLINE_MARKER) && !c.in_reply_to_id && !repliedTo.has(c.id)) { | |
| 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', | |
| body: `${SUMMARY_MARKER.replace('summary','review-header')}\nCodex flagged ${inline.length} item(s) inline below.`, | |
| 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 our own rolling summary comment (never overwrite a human's). | |
| const comments = await github.paginate(github.rest.issues.listComments, { | |
| owner, repo, issue_number: pull_number, per_page: 100, | |
| }); | |
| const existing = comments.find(c => isOurComment(c, 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 }); | |
| } |