Skip to content

feat: add Codex PR review workflow#2

Open
falkoro wants to merge 1 commit into
mainfrom
feat/codex-review
Open

feat: add Codex PR review workflow#2
falkoro wants to merge 1 commit into
mainfrom
feat/codex-review

Conversation

@falkoro

@falkoro falkoro commented Jun 7, 2026

Copy link
Copy Markdown
Owner

Codex PR review + graceful Dario quota skip on Claude review.

Add Codex review workflow and script (Headroom-specific prompt), mirror
shelldeck merge-gate pattern on logan-gl502vs. Update Dario proxy
resolution to skip on 402/429/unreachable instead of failing, and gate
Claude review on proxy availability with continue-on-error.
Copilot AI review requested due to automatic review settings June 7, 2026 17:44
@github-actions

github-actions Bot commented Jun 7, 2026

Copy link
Copy Markdown

🔎 Grok review (Hermes · grok-4.3)

scripts/codex-review.py:78 and .github/workflows/claude-review.yml:30 duplicate the Dario probe logic that now lives in resolve-dario-proxy.sh; the workflow should invoke the script instead. No correctness, security or public-API problems otherwise.

Verdict: Clean enough to merge; only minor duplication in the new CI paths.

@falkoro

falkoro commented Jun 7, 2026

Copy link
Copy Markdown
Owner Author

🧠 Grok Composer review (grok-composer-2.5)

Review

1. Brand consistency

No issues. This diff is CI-only (GitHub Actions + a review script). No logos, favicons, og:image, schema assets, or UI mockups.

2. Spot Suite architecture

No issues against the listed architecture rules. No tenant/session handling, D1, SECRETS KV, OAuth flows, cron handlers, migrations, or Pages deploys.

3. Correctness + security

Wrong product context in Codex promptscripts/codex-review.py:2, scripts/codex-review.py:19
The module docstring and PROMPT say Headroom, not Spot Suite. If this lands in spot-techno/spot-suite (or a sister product repo), reviews will use the wrong scope and checklist. The prompt should name the actual repo/product (or be parameterized).

Merge gate does not gate on outcome.github/workflows/codex-review.yml:104-106
Once the “Codex review” job has any conclusion (success, failure, cancelled, etc.), the merge gate exits 0 and allows merge. Combined with continue-on-error: true on the review job (.github/workflows/codex-review.yml:39), failed or skipped reviews never block merge. Fine if the intent is “wait, then proceed”; misleading if “merge gate” is meant to enforce review quality.

Duplicated Dario probe logic.github/workflows/claude-review.yml:29-68 vs .github/scripts/resolve-dario-proxy.sh:6-54
The workflow inlines the resolver instead of calling the script. Behavior is mostly aligned (402/429/unreachable → skip), but the script still emits api_key=dario on success (resolve-dario-proxy.sh:40-42) while the workflow does not (claude-review.yml:51-54). Probably harmless today (only ANTHROPIC_BASE_URL is passed at claude-review.yml:70-71), but two paths will drift.

Sandbox bypass in CIscripts/codex-review.py:81
--dangerously-bypass-approvals-and-sandbox is expected for unattended codex exec on a trusted self-hosted runner; worth keeping the runner locked down. Not a Spot Suite app security violation.

Resilient skip pathsresolve-dario-proxy.sh:45-52, claude-review.yml:20-21, codex-review.py:117-120
Quota/unreachable proxy, missing Codex CLI/auth, and review failures degrade to ::warning:: + exit 0 rather than failing the workflow. Consistent with optional review automation.

Otherwise: error handling in codex-review.py is reasonable (timeouts and non-zero exits surfaced; GitHub API/post failures warned, not thrown). No committed secrets, auth bypass, injection, or “workspace” copy.


Verdict: CI-only change is structurally sound and sensibly fail-open, but fix the Headroom-specific Codex prompt before merging into Spot Suite and confirm the merge gate is meant to wait only, not block on failed review.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Adds an automated “Codex Review” GitHub Actions workflow that runs Codex CLI on a self-hosted runner to generate a single PR review comment, and adjusts the existing Claude review workflow to gracefully skip when the Dario proxy is unreachable or quota-limited.

Changes:

  • Introduces scripts/codex-review.py to run codex exec against origin/<base>...HEAD and upsert a marker-based PR comment.
  • Adds .github/workflows/codex-review.yml with an enablement gate and a “merge gate” job that waits briefly for the review job to finish.
  • Updates Claude review/Dario proxy resolution to skip (not fail) on proxy/quota issues, and updates the helper script accordingly.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
scripts/codex-review.py New Python helper to run Codex review and post/update a PR comment.
.gitignore Allowlists the new scripts/codex-review.py under the existing scripts/ ignore rules.
.github/workflows/codex-review.yml New workflow to run Codex review (gated) and provide a merge-wait “gate” job.
.github/workflows/claude-review.yml Makes Claude review non-blocking and skips cleanly on Dario proxy quota/unreachability (now inline).
.github/scripts/resolve-dario-proxy.sh Updates the Dario proxy resolver to treat quota/unreachability as warnings and exit successfully.

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

Comment on lines +61 to +65
{
echo "available=true"
echo "bin=$(command -v "$CODEX_BIN")"
} >> "$GITHUB_OUTPUT"
echo "Codex CLI ready at $(command -v "$CODEX_BIN")"
Comment thread scripts/codex-review.py
Comment on lines +15 to +17
MODEL = os.environ.get("CODEX_MODEL", "gpt-5.5")
CODEX_BIN = os.environ.get("CODEX_BIN", "codex")
TIMEOUT_SEC = int(os.environ.get("CODEX_TIMEOUT_SEC", "900"))
Comment thread scripts/codex-review.py


def run_codex_review(base_ref: str, repo_root: Path) -> str:
out_path = Path(tempfile.mkstemp(suffix=".md")[1])
Comment on lines 27 to +31
- name: Resolve Dario proxy
id: dario
run: bash .github/scripts/resolve-dario-proxy.sh
run: |
set -euo pipefail
GW=""
Comment on lines 22 to 25
GW=""
if command -v ip >/dev/null 2>&1; then
GW="$(ip route 2>/dev/null | awk '/default/{print $3; exit}')"
fi
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.

2 participants