diff --git a/.github/workflows/codex-review-post.yml b/.github/workflows/codex-review-post.yml deleted file mode 100644 index 64999b28..00000000 --- a/.github/workflows/codex-review-post.yml +++ /dev/null @@ -1,302 +0,0 @@ -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 `; 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 = ''; - 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 - // 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 }); - } diff --git a/.github/workflows/codex-review.yml b/.github/workflows/codex-review.yml deleted file mode 100644 index 616dd44c..00000000 --- a/.github/workflows/codex-review.yml +++ /dev/null @@ -1,67 +0,0 @@ -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: - # 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 - -concurrency: - group: codex-review-${{ github.event.pull_request.number }} - cancel-in-progress: true - -jobs: - 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 - steps: - - name: Check out PR head - uses: actions/checkout@v4 - with: - # 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 - # 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}" - - - name: Upload review payload - uses: actions/upload-artifact@v4 - with: - name: codex-review-payload - path: codex-payload/ - retention-days: 1 - if-no-files-found: error