[PR #387] [MERGED] feat(cursor-review): gate on the direct-OpenAI cells when they replace the Cursor OpenAI lane #390

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-19850-direct-openai-gating

📝 Commits (3)

106fc0e feat(cursor-review): gate on the direct-OpenAI cells when they replace the Cursor OpenAI lane
c30c68b test(cursor-review): pin the direct-cell gating wiring
36ae64c docs(cursor-review): move the aggregate-panel.py row away from the stop-on-submit.py row

📊 Changes

7 files changed (+844 additions, -133 deletions)

View changed files

📝 .github/cursor-review/README.md (+4 -3)
➕ .github/cursor-review/aggregate-panel.py (+132 -0)
📝 .github/cursor-review/post-review.py (+5 -1)
➕ .github/cursor-review/tests/test_aggregate_panel.py (+239 -0)
📝 .github/cursor-review/tests/test_workflow_panel_integrity.py (+331 -24)
📝 .github/workflows/cursor-review.yml (+131 -103)
📝 docs/callers/cursor-review.md (+2 -2)

📄 Description

ELI-5

When a repo sends its OpenAI review straight to OpenAI instead of through Cursor, that direct reviewer used to be a "guest": its notes reached the judge, but it was never counted when deciding whether the panel was complete enough to auto-approve. It also only did one of the two passes. So turning it on quietly shrank the voting panel from three labs to two. Now the direct reviewer does both passes and, when it replaces the Cursor OpenAI reviewer, it votes like the one it replaced.

What changes

review-openai-direct is a 2-entry matrix (adversarial, edge-case). Each leg builds its prompt from the same prompt-<review_type>.md the Cursor cells use. Artifacts are findings-direct-<review_type>-<model>. Check names are <review_type> direct (<model>), so the adversarial leg keeps its existing context name.
preflight emits openai_direct_counts, set when the direct cell is in effect and openai_direct_replaces_cursor_openai is true. When it is set:
- the job's continue-on-error evaluates to false;
- a new Fail the leg when the cell did not submit step runs after the upload (same contract as the Cursor step of that name), so a failed direct cell is red in the rollup;
- Aggregate panel findings puts both direct cells in expected, present, ok_count and total. A missing artifact is filled in as status=error, just as it is for a Cursor cell. The panel line reads e.g. 6/6.
Side-by-side mode (replace false) is unchanged. The direct cells stay advisory: they are never counted, never red, and kept out of the panel metadata auto-approve reads.
Aggregation moved from inline Python into .github/cursor-review/aggregate-panel.py so it can be unit-tested. Both markers (direct_api, advisory) are popped from every cell's own record and set only by the script, so a cell still cannot excuse itself from the count.
Counted direct cells enter the panel metadata tagged direct. post-review.py names a failed one in the "Reviewers that did not contribute" line as gpt-5.6-sol:edge-case (direct, error). The judge input carries each direct cell's own review_type.
Docs updated: the input descriptions (openai_direct_model, openai_direct_replaces_cursor_openai), .github/cursor-review/README.md and docs/callers/cursor-review.md describe the new gating.

Judgment call: "replace is true" is read as "openai_direct is in effect AND the replace input is true". That includes the edge case where the panel list had no gpt-* id to drop. In that case the direct cells are still the run's only OpenAI lane, so they count.

Self-review notes

Riskiest line: the job-level continue-on-error: ${{ needs.preflight.outputs.openai_direct_counts != 'true' }}. GitHub allows needs and matrix in that expression, and actionlint reports nothing new on this file versus main (only the existing job.workflow_sha / SC2018-19 notices). If the output is ever empty, the expression evaluates to true, which is today's advisory behaviour. So the failure direction is "does not gate", never "spuriously red".
A red direct leg cannot block consolidation. consolidate still gates on !cancelled() and reads the direct job only for ordering. A failed or absent direct cell is counted from its artifact, or from the missing artifact.
Cost: with replace on, each run makes one extra Codex call (the edge-case pass), which the ticket asks for.

Provenance

Authored by: agent-work loop
Verified:
- Tests: cursor-review script suite all green (1184 tests). This includes the new test_aggregate_panel.py, which covers replace=true with both direct cells ok (6/6), one direct error (5/6), a missing direct artifact (synthesised as an error), replace=false (advisory, 4/4), marker forgery, the (direct, error) summary line, and panel_gate counting a direct failure.
- Updated test_workflow_panel_integrity.py's aggregate-step pin to follow the counting into the script.
- workflow-pins suite + check_workflow_pins.py, check_agents_md.py, org-repo-literal lint: pass
- actionlint on cursor-review.yml: no new findings vs main
- Not run: an end-to-end Actions run of the reusable workflow (it only executes from a caller after merge + bump)
Deviations: none against the acceptance criteria

Residual

No live run yet. None of the new behaviour has been exercised in a real Actions run: the 2-leg direct matrix, the expression-valued job continue-on-error, and the red leg on a failed direct cell. The first consumer run after the SHA bump will show it. Check that the review posts Panel: 6/6 and shows two direct (gpt-…) legs. Also check that a direct error turns its leg red and appears as (direct, error).
Ticket evidence not reproduced from here. The ~220 / 18 cloud-run sample (consolidate logs showing Panel: 2/4 alongside Direct-API cell (advisory, not counted): status=ok) was not re-read. I have no access to those run logs, so the fix is checked against the aggregation code path and unit tests, not those runs.
Cost not measured. The extra edge-case Codex call per run doubles direct-API spend on the OpenAI lane. Callers on a tight OPENAI_API_KEY spend limit may need to raise it.
Out of scope per the ticket: retrying the direct cell on transient OpenAI errors, more than one direct model, changing the Kimi model or any caller's approve_max_failed_reviewers, and moving another lab to a direct-API path.

Review follow-ups

A failed direct cell now counts. With replace on, a failed direct cell counts against approve_max_failed_reviewers; before this PR it was free. Callers with a 1-failure allowance therefore gate on the full panel again: a failed direct cell uses up that allowance, just as a failed Cursor cell does. A whole-lab outage is two failed cells, so it withholds approval under that allowance both before and after this change.
Not exercised here. This repo's own panel run has not taken the new path: its caller (ci-cursor-review.yml) does not set openai_direct_model, and it runs the reusable workflow pinned to a main SHA.
Wiring tests added (DirectCellsGateWhenTheyReplaceTest in test_workflow_panel_integrity.py). They execute the real step scripts, from preflight's Define panel models through Aggregate panel findings to the direct leg check. Four one-line edits used to pass every suite while making the cells advisory again: deleting preflight's openai_direct_counts=true, hard-coding --direct-counts "false", blanking DIRECT_MODEL, and changing the leg check's exit 1 to exit 0. Each was applied to a scratch copy, and each now fails these tests.
Also: corrected the job comment's lab-outage rationale, made the input descriptions plural, and listed aggregate-panel.py in the README file table. Verified: the cursor-review suite (1194 tests), the cursor-approve suite, the workflow-pins suite with check_workflow_pins.py, and the org-repo-literal lint all pass; actionlint shows no new findings.

_{🔄 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/387 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/9/2026 **Status:** ✅ Merged **Merged:** 10/9/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-19850-direct-openai-gating` --- ### 📝 Commits (3) - [`106fc0e`](https://github.com/Comfy-Org/github-workflows/commit/106fc0e999d523e2f23b1e25b9db670258529581) feat(cursor-review): gate on the direct-OpenAI cells when they replace the Cursor OpenAI lane - [`c30c68b`](https://github.com/Comfy-Org/github-workflows/commit/c30c68b59411d88a94dce5309ab9aa19f77dfc9f) test(cursor-review): pin the direct-cell gating wiring - [`36ae64c`](https://github.com/Comfy-Org/github-workflows/commit/36ae64c7cb56a578658c7a68da26eff586109e54) docs(cursor-review): move the aggregate-panel.py row away from the stop-on-submit.py row ### 📊 Changes **7 files changed** (+844 additions, -133 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+4 -3) ➕ `.github/cursor-review/aggregate-panel.py` (+132 -0) 📝 `.github/cursor-review/post-review.py` (+5 -1) ➕ `.github/cursor-review/tests/test_aggregate_panel.py` (+239 -0) 📝 `.github/cursor-review/tests/test_workflow_panel_integrity.py` (+331 -24) 📝 `.github/workflows/cursor-review.yml` (+131 -103) 📝 `docs/callers/cursor-review.md` (+2 -2) </details> ### 📄 Description ## ELI-5 When a repo sends its OpenAI review straight to OpenAI instead of through Cursor, that direct reviewer used to be a "guest": its notes reached the judge, but it was never counted when deciding whether the panel was complete enough to auto-approve. It also only did one of the two passes. So turning it on quietly shrank the voting panel from three labs to two. Now the direct reviewer does both passes and, when it *replaces* the Cursor OpenAI reviewer, it votes like the one it replaced. ## What changes - **`review-openai-direct` is a 2-entry matrix** (`adversarial`, `edge-case`). Each leg builds its prompt from the same `prompt-<review_type>.md` the Cursor cells use. Artifacts are `findings-direct-<review_type>-<model>`. Check names are `<review_type> direct (<model>)`, so the adversarial leg keeps its existing context name. - **preflight emits `openai_direct_counts`**, set when the direct cell is in effect and `openai_direct_replaces_cursor_openai` is true. When it is set: - the job's `continue-on-error` evaluates to `false`; - a new `Fail the leg when the cell did not submit` step runs after the upload (same contract as the Cursor step of that name), so a failed direct cell is red in the rollup; - `Aggregate panel findings` puts both direct cells in `expected`, `present`, `ok_count` and `total`. A missing artifact is filled in as `status=error`, just as it is for a Cursor cell. The panel line reads e.g. `6/6`. - **Side-by-side mode (replace `false`) is unchanged.** The direct cells stay advisory: they are never counted, never red, and kept out of the panel metadata auto-approve reads. - **Aggregation moved from inline Python into `.github/cursor-review/aggregate-panel.py`** so it can be unit-tested. Both markers (`direct_api`, `advisory`) are popped from every cell's own record and set only by the script, so a cell still cannot excuse itself from the count. - **Counted direct cells enter the `panel` metadata tagged `direct`.** `post-review.py` names a failed one in the "Reviewers that did not contribute" line as `gpt-5.6-sol:edge-case (direct, error)`. The judge input carries each direct cell's own `review_type`. - **Docs updated:** the input descriptions (`openai_direct_model`, `openai_direct_replaces_cursor_openai`), `.github/cursor-review/README.md` and `docs/callers/cursor-review.md` describe the new gating. Judgment call: "replace is true" is read as "`openai_direct` is in effect AND the replace input is true". That includes the edge case where the panel list had no `gpt-*` id to drop. In that case the direct cells are still the run's only OpenAI lane, so they count. ## Self-review notes - **Riskiest line:** the job-level `continue-on-error: ${{ needs.preflight.outputs.openai_direct_counts != 'true' }}`. GitHub allows `needs` and `matrix` in that expression, and actionlint reports nothing new on this file versus `main` (only the existing `job.workflow_sha` / SC2018-19 notices). If the output is ever empty, the expression evaluates to `true`, which is today's advisory behaviour. So the failure direction is "does not gate", never "spuriously red". - **A red direct leg cannot block consolidation.** `consolidate` still gates on `!cancelled()` and reads the direct job only for ordering. A failed or absent direct cell is counted from its artifact, or from the missing artifact. - **Cost:** with replace on, each run makes one extra Codex call (the edge-case pass), which the ticket asks for. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests: cursor-review script suite all green (1184 tests). This includes the new `test_aggregate_panel.py`, which covers replace=true with both direct cells ok (6/6), one direct error (5/6), a missing direct artifact (synthesised as an error), replace=false (advisory, 4/4), marker forgery, the `(direct, error)` summary line, and `panel_gate` counting a direct failure. - Updated `test_workflow_panel_integrity.py`'s aggregate-step pin to follow the counting into the script. - workflow-pins suite + `check_workflow_pins.py`, `check_agents_md.py`, org-repo-literal lint: pass - actionlint on `cursor-review.yml`: no new findings vs `main` - Not run: an end-to-end Actions run of the reusable workflow (it only executes from a caller after merge + bump) - **Deviations:** none against the acceptance criteria ## Residual - **No live run yet.** None of the new behaviour has been exercised in a real Actions run: the 2-leg direct matrix, the expression-valued job `continue-on-error`, and the red leg on a failed direct cell. The first consumer run after the SHA bump will show it. Check that the review posts `Panel: 6/6` and shows two `direct (gpt-…)` legs. Also check that a direct error turns its leg red and appears as `(direct, error)`. - **Ticket evidence not reproduced from here.** The ~220 / 18 cloud-run sample (consolidate logs showing `Panel: 2/4` alongside `Direct-API cell (advisory, not counted): status=ok`) was not re-read. I have no access to those run logs, so the fix is checked against the aggregation code path and unit tests, not those runs. - **Cost not measured.** The extra edge-case Codex call per run doubles direct-API spend on the OpenAI lane. Callers on a tight `OPENAI_API_KEY` spend limit may need to raise it. - **Out of scope per the ticket:** retrying the direct cell on transient OpenAI errors, more than one direct model, changing the Kimi model or any caller's `approve_max_failed_reviewers`, and moving another lab to a direct-API path. ## Review follow-ups - **A failed direct cell now counts.** With replace on, a failed direct cell counts against `approve_max_failed_reviewers`; before this PR it was free. Callers with a 1-failure allowance therefore gate on the full panel again: a failed direct cell uses up that allowance, just as a failed Cursor cell does. A whole-lab outage is two failed cells, so it withholds approval under that allowance both before and after this change. - **Not exercised here.** This repo's own panel run has not taken the new path: its caller (`ci-cursor-review.yml`) does not set `openai_direct_model`, and it runs the reusable workflow pinned to a main SHA. - **Wiring tests added** (`DirectCellsGateWhenTheyReplaceTest` in `test_workflow_panel_integrity.py`). They execute the real step scripts, from preflight's `Define panel models` through `Aggregate panel findings` to the direct leg check. Four one-line edits used to pass every suite while making the cells advisory again: deleting preflight's `openai_direct_counts=true`, hard-coding `--direct-counts "false"`, blanking `DIRECT_MODEL`, and changing the leg check's `exit 1` to `exit 0`. Each was applied to a scratch copy, and each now fails these tests. - **Also:** corrected the job comment's lab-outage rationale, made the input descriptions plural, and listed `aggregate-panel.py` in the README file table. Verified: the cursor-review suite (1194 tests), the cursor-approve suite, the workflow-pins suite with `check_workflow_pins.py`, and the org-repo-literal lint all pass; actionlint shows no new findings. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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