Skip to content

ci(codex-review): review all PRs incl forks via two-stage split#437

Merged
HarshaNalluru merged 4 commits into
mainfrom
ci/codex-review-all-prs
Jul 23, 2026
Merged

ci(codex-review): review all PRs incl forks via two-stage split#437
HarshaNalluru merged 4 commits into
mainfrom
ci/codex-review-all-prs

Conversation

@HarshaNalluru

@HarshaNalluru HarshaNalluru commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

What

Makes the Codex PR reviewer run on all pull requests, including forks/contributors — today fork PRs are silently skipped.

Why forks were skipped

A fork's pull_request run gets a read-only token and no secrets (GitHub's fork-safety default), so the reviewer can't get OPENAI_API_KEY. The old workflow acknowledged this by gating itself to same-repo branches (sameRepo check), so every contributor PR got Skipping Codex review: … is from a fork.

You can't fix this by dropping the gate — the secret still isn't there on fork runs. You need a trigger that has secrets, without ever running fork code next to them.

How — two-stage split

Stage File Trigger Secrets? Touches PR code?
1. Collect diff codex-review.yml pull_request (all PRs) ❌ none checkout + git diff only (no build/install)
2. Review + post codex-review-post.yml workflow_run (base context) ❌ — reads the diff artifact only

Stage 1 runs on every PR with no secrets, computes the diff, and uploads it as an artifact. Stage 2 is triggered by Stage 1's completion, runs in the base-repo context (so it has secrets even for forks), downloads the diff, runs Codex over the diff text only, and posts the inline review.

Fork code never executes in a job that holds the key — that's the structural guarantee that makes reviewing fork PRs safe, and it's more robust than pull_request_target (which would put fork code on disk beside the secret and rely on "no one ever adds a build step").

Security notes

  • Anti-spoofing: the artifact carries the PR number; Stage 2 requires pr.head.sha === workflow_run.head_sha (a value GitHub sets, not the fork) before posting, so a tampered artifact can't redirect the review onto another PR. PR number is validated as digits-only.
  • Least privilege: Stage 1 is contents: read. Stage 2 is contents: read, actions: read (to fetch the artifact), pull-requests/issues: write (to post).
  • Residual risk — please use a scoped key. Codex still reads the untrusted diff as data, and per the action's docs the API key "flows through the proxy, so Codex could read it if it can reach process memory." A prompt-injection payload in a diff could in principle try to coax the key into a review comment. The two-stage split does not remove this (it removes code execution), so use a low-limit / restricted OPENAI_API_KEY for this workflow.

Behavior changes

  • Auto-reviews all non-draft, non-bot PRs (previously same-repo only).
  • Drops the @codex review comment trigger and the trusted-commenter gate (no longer needed to keep forks out). Easy to re-add as an opt-in gate if you'd prefer maintainer-triggered reviews on forks to control cost.

Activation caveat

workflow_run only fires for the copy of these workflows on the default branch, so Codex review activates once this is merged to mainthis PR will not review itself. After merge, opening any fork PR (or re-syncing an existing one like #391/#393) will exercise it end-to-end.

Not tested in CI here

The openai/codex-action step reviews a pre-computed pr.diff in an empty git init'd working dir (rather than a full checkout). YAML + embedded JS are validated locally; the first live run after merge is the real end-to-end test.

Review in cubic

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.
Copilot AI review requested due to automatic review settings July 18, 2026 06:51
@HarshaNalluru
HarshaNalluru requested a review from r4ghu as a code owner July 18, 2026 06:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR restructures the Codex PR reviewer workflow into a two-stage pipeline so reviews can run on all PRs (including forks) while keeping OPENAI_API_KEY only in a base-repo workflow_run job that never checks out PR code.

Changes:

  • Replace the existing single-stage Codex review workflow with a Stage 1 pull_request workflow that collects a PR diff and uploads it as an artifact.
  • Add a Stage 2 workflow_run workflow that downloads the diff artifact, runs openai/codex-action on the diff text only, and posts inline review comments + a rolling summary.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
.github/workflows/codex-review.yml Stage 1: run on PR events, compute diff, upload artifact for Stage 2 consumption.
.github/workflows/codex-review-post.yml Stage 2: run with secrets in base context, verify PR/head SHA, run Codex on diff artifact, post review comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/codex-review.yml Outdated
Comment thread .github/workflows/codex-review.yml Outdated
Comment thread .github/workflows/codex-review-post.yml Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files

Tip: instead of fixing issues one by one fix them all with cubic

Re-trigger cubic

Comment thread .github/workflows/codex-review.yml Outdated
Comment thread .github/workflows/codex-review-post.yml
Comment thread .github/workflows/codex-review.yml Outdated
Comment thread .github/workflows/codex-review-post.yml Outdated
Comment thread .github/workflows/codex-review-post.yml
Comment thread .github/workflows/codex-review.yml
Comment thread .github/workflows/codex-review-post.yml Outdated
Comment thread .github/workflows/codex-review-post.yml Outdated
Comment thread .github/workflows/codex-review-post.yml Outdated
Comment thread .github/workflows/codex-review-post.yml Outdated
- 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.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread .github/workflows/codex-review-post.yml Outdated
- 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.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 1 file (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Fix all with cubic | Re-trigger cubic

Comment thread .github/workflows/codex-review-post.yml
- 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.
@HarshaNalluru
HarshaNalluru merged commit c3e6ddb into main Jul 23, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants