[PR #382] [MERGED] feat(cursor-review): optional direct-OpenAI review cell via a pinned Codex CLI (BE-19639) #383

New issue

Closed

opened 2026-10-26 22:39:14 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:39:14 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/382
Author: @mattmillerai
Created: 10/8/2026
Status: ✅ Merged
Merged: 10/8/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-19639-openai-direct-cell

📝 Commits (6)

6e7bfdf feat(cursor-review): optional direct-OpenAI review cell via a pinned Codex CLI (BE-19639)
ce1e2fd fix(cursor-review): keep the direct-OpenAI cell on hosted runners with the canonical ref guard
83123bd feat(cursor-review): the direct cell replaces the Cursor OpenAI cell by default (BE-19639)
1a237be fix(cursor-review): make the direct-OpenAI cell genuinely advisory and keep its key off disk (BE-19639)
c55df99 fix(cursor-review): splice the prior-review ledger into the direct cell's prompt; keep CODEX_HOME under RUNNER_TEMP (BE-19639)
b37c098 test(groom): bind the dependabot npm-entry check to the groom directory, and assert the cursor-review one too (BE-19639)

📊 Changes

8 files changed (+547 additions, -11 deletions)

View changed files

📝 .github/cursor-review/README.md (+3 -0)
➕ .github/cursor-review/package.json (+8 -0)
📝 .github/cursor-review/tests/test_workflow_ref_guard_parity.py (+1 -1)
📝 .github/dependabot.yml (+15 -0)
📝 .github/groom/tests/test_claude_code_pin.py (+14 -3)
📝 .github/workflow-pins/tests/test_check_workflow_pins.py (+6 -2)
📝 .github/workflows/cursor-review.yml (+497 -5)
📝 docs/callers/cursor-review.md (+3 -0)

📄 Description

ELI5

The review panel can only use models Cursor offers. This adds an optional extra
reviewer that talks to OpenAI directly, so the panel can include a model Cursor
does not carry. It is off unless a repo turns it on, and when it is off nothing
about today's reviews changes.

What this adds

Input openai_direct_model (default '') — an OpenAI model id reviewed as one
adversarial cell through the OpenAI API directly.
Input openai_direct_replaces_cursor_openai (default true) — when the direct
cell is in effect, preflight drops the Cursor gpt-* cell(s) so the OpenAI lane
is reviewed once; false runs both for a side-by-side comparison. Preflight
fails rather than leave the Cursor panel empty.
Optional secret OPENAI_API_KEY.
.github/cursor-review/package.json — pins @openai/codex to an exact version.
Job review-openai-direct.
A Dependabot npm entry for /.github/cursor-review, so the pin is actually watched.
Docs: openai_direct_model / openai_direct_replaces_cursor_openai / OPENAI_API_KEY rows in the cursor-review README and docs/callers/cursor-review.md.

Design notes

Separate job, not another review matrix entry. The matrix's model list is
validated against the live Cursor catalog in preflight, and a delisted id there
fails the run loud by design (BE-2627). A direct-API id is not in that catalog at
all, so adding it to panel_models would fail preflight for the right reason and
the wrong cause. Separation also means a failure here can neither cancel nor skip
a Cursor matrix leg.

Turned on only when it can run. The hosted gate job resolves whether
OPENAI_API_KEY reached the workflow, as a boolean. preflight follows runs_on
and must not reference the secret. preflight turns the cell on only for a plain
[A-Za-z0-9._-] id with the key present. Otherwise it warns, leaves the cell off
and leaves the Cursor panel unchanged, so a misconfigured opt-in never drops the
OpenAI lane.

Judge integration. The artifact is findings-direct-adversarial-<model>. It
still matches consolidate's findings-* download and can never collide with a
Cursor cell's name. consolidate lists the job in needs: (for ordering only; its
!cancelled() gate does not read the result). Aggregate panel findings sends the
cell's findings to the judge but keeps it out of the Cursor expected/present
sets, ok_count/total and the panel metadata auto-approve counts failures from.

Identical prompt path. Same prompt-adversarial.md with the same prior-review
ledger splice and new-hunks block, same marker stripped and
re-emitted by fence-diff.py, a fresh per-prompt nonce minted into a shell
variable rather than a step env:, and the same review-output-mcp.py submission
tool the Cursor cells use. So one implementation produces the findings schema for
both backends, and a finding difference is attributable to the model or the
backend rather than to prompt drift.

Advisory on purpose. continue-on-error: true at the job level, so no step in
the cell can turn the caller's run red while the backend is still being
characterised. Its record is not counted by Panel integrity or the auto-approve
gate.

CLI pinning follows the groom precedent. The pin lives in a real npm manifest
so Dependabot's npm ecosystem can see it and open a bump PR — an inline
npm install -g pkg@1.2.3 in a run: step is invisible to every ecosystem, which
is how the groom pin silently rotted. The manifest arrives with the assets
checkout, so it resolves at the same SHA as the prompts and scripts, and the job
rejects a non-exact pin at run time.

Credential posture unchanged. contents: read only, no PR checkout, no
write-scoped token (SECURITY.md). The job runs on GitHub-hosted ubuntu-latest,
never the caller's runs_on pool: it holds OPENAI_API_KEY, and only
CURSOR_API_KEY-only read-only jobs may follow runs_on. Its workflows_ref
guard is a byte-identical copy of the shared one (incl. the job.workflow_sha
cross-check). The only PR-authored content the agent sees is
the same nonce-fenced diff text the Cursor cells read. The key reaches
codex exec only as CODEX_API_KEY in that one step's env, with a fresh
CODEX_HOME under RUNNER_TEMP. It never goes in argv and never on disk: there is
no codex login, whose auth.json the read-only sandbox could still read. The
agent's shell gets shell_environment_policy.inherit=core, so it never sees the
key. The public log shows the cell's status and finding count, not the findings
verbatim. Fork PRs get no secret, as before.

Verification


`actionlint`	no new findings vs baseline (identical message classes and counts)
`bash -n` on every new run block	passes
YAML parse + job wiring	passes
heredoc terminators resolve at column 0 after block-scalar stripping	checked explicitly

The cell contract was exercised end to end against real PRs locally before this
was wired, using the same prompt, fencing and MCP tool: cells reached
status: ok, submitted findings in the judge's schema, and stayed well inside the
agent step's cap. That included a control case — a documentation-only diff
correctly produced no findings, which the adversarial prompt explicitly excludes.

Scope / follow-ups

v1 is adversarial only (not edge-case), one model, one round.
Reasoning effort is fixed at max; worth an input once there is a reason to vary it.
No early-stop equivalent to stop-on-submit.py — the CLI exits on submission
in practice, so this is unneeded today rather than deferred.
Characterising the backend against the Cursor cells on the same diffs is the
point of enabling it on one repo first.

Provenance

Authored by: agent-work loop
Verified:
- workflow-pins, cursor-review and groom unittest suites pass locally at the current head (incl. job-isolation, panel-integrity, guard-parity and inputs-docs-drift)
- check_workflow_pins.py, check_agents_md.py --root . and check-org-repo-literals.sh pass; actionlint reports no new message classes vs main
- Aggregate panel findings snippet exercised locally: a findings-direct-* cell stays out of ok_count/total/present, and a forged direct_api marker on a Cursor record is stripped
- Pinned Codex 0.162.0 checked locally: CODEX_API_KEY env auth reaches the API and writes no auth.json; model_reasoning_effort=max is in its enum
- Direct-API cell exercised end to end locally against real PRs before wiring (see Verification)
Deviations:
- Guard-count tests bumped (11→12 cursor-review guards, 22→23 guarded sites) for the new job's guard, as those tests direct
- groom's dependabot test assumed exactly ONE npm entry. It now binds every assertion to the single /.github/groom entry and asserts the same for /.github/cursor-review. It still fails if the groom block moves; that was checked by mutating the file.
- cursor-approve's test_cli_pin_matches_cursor_review fails here, as it does on main: cursor-axis-base.yml was not moved with #335's Cursor CLI bump. That is a base defect outside this PR.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/382 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/8/2026 **Status:** ✅ Merged **Merged:** 10/8/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-19639-openai-direct-cell` --- ### 📝 Commits (6) - [`6e7bfdf`](https://github.com/Comfy-Org/github-workflows/commit/6e7bfdf936ddc6dadbc7736bab9410177c625e6c) feat(cursor-review): optional direct-OpenAI review cell via a pinned Codex CLI (BE-19639) - [`ce1e2fd`](https://github.com/Comfy-Org/github-workflows/commit/ce1e2fd31a25beb15d658c371c798253ed1a6685) fix(cursor-review): keep the direct-OpenAI cell on hosted runners with the canonical ref guard - [`83123bd`](https://github.com/Comfy-Org/github-workflows/commit/83123bd52bc28ea33c083c00b7af5fc54c0a4312) feat(cursor-review): the direct cell replaces the Cursor OpenAI cell by default (BE-19639) - [`1a237be`](https://github.com/Comfy-Org/github-workflows/commit/1a237bec76ed29d206e202891030cd65a94ee7c6) fix(cursor-review): make the direct-OpenAI cell genuinely advisory and keep its key off disk (BE-19639) - [`c55df99`](https://github.com/Comfy-Org/github-workflows/commit/c55df992f6151499a8337cadef0f3206a136e168) fix(cursor-review): splice the prior-review ledger into the direct cell's prompt; keep CODEX_HOME under RUNNER_TEMP (BE-19639) - [`b37c098`](https://github.com/Comfy-Org/github-workflows/commit/b37c0988581c976eb87d8afcbfe7d89051546d02) test(groom): bind the dependabot npm-entry check to the groom directory, and assert the cursor-review one too (BE-19639) ### 📊 Changes **8 files changed** (+547 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+3 -0) ➕ `.github/cursor-review/package.json` (+8 -0) 📝 `.github/cursor-review/tests/test_workflow_ref_guard_parity.py` (+1 -1) 📝 `.github/dependabot.yml` (+15 -0) 📝 `.github/groom/tests/test_claude_code_pin.py` (+14 -3) 📝 `.github/workflow-pins/tests/test_check_workflow_pins.py` (+6 -2) 📝 `.github/workflows/cursor-review.yml` (+497 -5) 📝 `docs/callers/cursor-review.md` (+3 -0) </details> ### 📄 Description ## ELI5 The review panel can only use models Cursor offers. This adds an optional extra reviewer that talks to OpenAI directly, so the panel can include a model Cursor does not carry. It is off unless a repo turns it on, and when it is off nothing about today's reviews changes. ## What this adds - Input `openai_direct_model` (default `''`) — an OpenAI model id reviewed as one **adversarial** cell through the OpenAI API directly. - Input `openai_direct_replaces_cursor_openai` (default `true`) — when the direct cell is in effect, preflight drops the Cursor `gpt-*` cell(s) so the OpenAI lane is reviewed once; `false` runs both for a side-by-side comparison. Preflight fails rather than leave the Cursor panel empty. - Optional secret `OPENAI_API_KEY`. - `.github/cursor-review/package.json` — pins `@openai/codex` to an exact version. - Job `review-openai-direct`. - A Dependabot npm entry for `/.github/cursor-review`, so the pin is actually watched. - Docs: `openai_direct_model` / `openai_direct_replaces_cursor_openai` / `OPENAI_API_KEY` rows in the cursor-review README and `docs/callers/cursor-review.md`. ## Design notes **Separate job, not another `review` matrix entry.** The matrix's model list is validated against the live Cursor catalog in `preflight`, and a delisted id there fails the run loud by design (BE-2627). A direct-API id is not in that catalog at all, so adding it to `panel_models` would fail preflight for the right reason and the wrong cause. Separation also means a failure here can neither cancel nor skip a Cursor matrix leg. **Turned on only when it can run.** The hosted `gate` job resolves whether `OPENAI_API_KEY` reached the workflow, as a boolean. `preflight` follows `runs_on` and must not reference the secret. `preflight` turns the cell on only for a plain `[A-Za-z0-9._-]` id with the key present. Otherwise it warns, leaves the cell off and leaves the Cursor panel unchanged, so a misconfigured opt-in never drops the OpenAI lane. **Judge integration.** The artifact is `findings-direct-adversarial-<model>`. It still matches `consolidate`'s `findings-*` download and can never collide with a Cursor cell's name. `consolidate` lists the job in `needs:` (for ordering only; its `!cancelled()` gate does not read the result). `Aggregate panel findings` sends the cell's findings to the judge but keeps it out of the Cursor `expected`/`present` sets, `ok_count`/`total` and the `panel` metadata auto-approve counts failures from. **Identical prompt path.** Same `prompt-adversarial.md` with the same prior-review ledger splice and new-hunks block, same marker stripped and re-emitted by `fence-diff.py`, a fresh per-prompt nonce minted into a shell variable rather than a step `env:`, and the same `review-output-mcp.py` submission tool the Cursor cells use. So one implementation produces the findings schema for both backends, and a finding difference is attributable to the model or the backend rather than to prompt drift. **Advisory on purpose.** `continue-on-error: true` at the job level, so no step in the cell can turn the caller's run red while the backend is still being characterised. Its record is not counted by `Panel integrity` or the auto-approve gate. **CLI pinning follows the groom precedent.** The pin lives in a real npm manifest so Dependabot's npm ecosystem can see it and open a bump PR — an inline `npm install -g pkg@1.2.3` in a `run:` step is invisible to every ecosystem, which is how the groom pin silently rotted. The manifest arrives with the assets checkout, so it resolves at the same SHA as the prompts and scripts, and the job rejects a non-exact pin at run time. **Credential posture unchanged.** `contents: read` only, no PR checkout, no write-scoped token (SECURITY.md). The job runs on GitHub-hosted `ubuntu-latest`, never the caller's `runs_on` pool: it holds `OPENAI_API_KEY`, and only CURSOR_API_KEY-only read-only jobs may follow `runs_on`. Its `workflows_ref` guard is a byte-identical copy of the shared one (incl. the `job.workflow_sha` cross-check). The only PR-authored content the agent sees is the same nonce-fenced diff text the Cursor cells read. The key reaches `codex exec` only as `CODEX_API_KEY` in that one step's env, with a fresh `CODEX_HOME` under `RUNNER_TEMP`. It never goes in argv and never on disk: there is no `codex login`, whose `auth.json` the read-only sandbox could still read. The agent's shell gets `shell_environment_policy.inherit=core`, so it never sees the key. The public log shows the cell's status and finding count, not the findings verbatim. Fork PRs get no secret, as before. ## Verification | | | |---|---| | `actionlint` | no new findings vs baseline (identical message classes and counts) | | `bash -n` on every new run block | passes | | YAML parse + job wiring | passes | | heredoc terminators resolve at column 0 after block-scalar stripping | checked explicitly | The cell contract was exercised end to end against real PRs locally before this was wired, using the same prompt, fencing and MCP tool: cells reached `status: ok`, submitted findings in the judge's schema, and stayed well inside the agent step's cap. That included a control case — a documentation-only diff correctly produced no findings, which the adversarial prompt explicitly excludes. ## Scope / follow-ups 1. v1 is adversarial only (not `edge-case`), one model, one round. 2. Reasoning effort is fixed at `max`; worth an input once there is a reason to vary it. 3. No early-stop equivalent to `stop-on-submit.py` — the CLI exits on submission in practice, so this is unneeded today rather than deferred. 4. Characterising the backend against the Cursor cells on the same diffs is the point of enabling it on one repo first. ## Provenance - **Authored by:** agent-work loop - **Verified:** - `workflow-pins`, `cursor-review` and `groom` unittest suites pass locally at the current head (incl. job-isolation, panel-integrity, guard-parity and inputs-docs-drift) - `check_workflow_pins.py`, `check_agents_md.py --root .` and `check-org-repo-literals.sh` pass; `actionlint` reports no new message classes vs `main` - `Aggregate panel findings` snippet exercised locally: a `findings-direct-*` cell stays out of `ok_count`/`total`/`present`, and a forged `direct_api` marker on a Cursor record is stripped - Pinned Codex 0.162.0 checked locally: `CODEX_API_KEY` env auth reaches the API and writes no `auth.json`; `model_reasoning_effort=max` is in its enum - Direct-API cell exercised end to end locally against real PRs before wiring (see Verification) - **Deviations:** - Guard-count tests bumped (11→12 cursor-review guards, 22→23 guarded sites) for the new job's guard, as those tests direct - groom's dependabot test assumed exactly ONE npm entry. It now binds every assertion to the single `/.github/groom` entry and asserts the same for `/.github/cursor-review`. It still fails if the groom block moves; that was checked by mutating the file. - `cursor-approve`'s `test_cli_pin_matches_cursor_review` fails here, as it does on `main`: `cursor-axis-base.yml` was not moved with #335's Cursor CLI bump. That is a base defect outside this PR. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-10-26 22:39:14 +00:00

bsakaguchi referenced this issue

2026-10-26 22:39:14 +00:00

[PR #383] [MERGED] fix(cursor-axis-base): carry the Cursor CLI pin forward to match cursor-review #384

bsakaguchi closed this issue

2026-10-26 22:39:14 +00:00