[PR #219] [MERGED] fix(cursor-review): give the judge the same timeout as the panel it adjudicates #228

New issue

Closed

opened 2026-10-26 22:37:34 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:37:34 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/cursor-review-judge-timeout

📝 Commits (2)

791af41 fix(cursor-review): give the judge the same timeout as the panel it adjudicates
598cc56 fix(cursor-review): time-box the judge STEP, and stop naming a consumer repo

📊 Changes

1 file changed (+42 additions, -1 deletions)

View changed files

📝 .github/workflows/cursor-review.yml (+42 -1)

📄 Description

ELI5

The Cursor review runs 8 model calls in parallel (the panel), then one more call (the judge) reads all 8 and writes the single review that lands on your PR. The panel was allowed 30 minutes; the judge only 15 — even though the judge has to read everything the panel produced.

So on a big PR the panel would finish fine after ~25 minutes, and then the judge would get killed right at the end. All that work, no review posted.

What was happening

consolidate had timeout-minutes: 15; review (one panel cell) has 30. The judge calls cursor-agent once more with prompt-judge.md against all 8 cell artifacts, using the same top reasoning tier a single cell uses — the strictly harder call, on half the budget.

A diff slow enough to stretch the panel stretches the judge with it, so the two go long together and only the judge has a tight cap.

It also hid itself. GitHub reports a timeout-minutes kill as cancelled, not failed, so the run page reads as though someone stopped it by hand rather than a cap firing.

Evidence

Two occurrences on one consumer repo on 2026-08-24, both landing exactly on the cap:

Judge started	Killed	Elapsed
19:49:57	20:05:10	15m13s
17:55:11	18:10:16	15m05s

In the first, all 8 panel cells succeeded between 19:24 and 19:49 — the slowest (adversarial (claude-opus-5-thinking-max)) took 25 minutes of review's 30. Then the judge burned its full 15 and was killed. Nothing posted.

Across the last 60 Cursor Review runs on that repo, 5 were cancelled: 2 were this judge timeout, the other 3 were ordinary concurrency cancels (every job killed within seconds of start when a new push superseded the run). So ~3% of reviews were running a full panel and discarding the result.

notify-complete already branches on needs.consolidate.result, so the DM correctly degraded to a warning — the loss was the review itself, not a false success signal.

The change

Raising the consolidate job cap alone would only move the cliff. A job-level timeout-minutes kill takes Build consolidated findings file and Post review with it, and that is the one failure mode that can never reach the degraded panel-union fallback the workflow already has — so a hung judge would still burn the budget and post nothing, just 30 minutes later.

So the cap goes on the step, in the shape the ledger job already uses:

Run judge gets timeout-minutes: 30 — the review panel cap, so the judge is never given less model time than one of the cells it reads — plus continue-on-error: true, which is what makes the timeout absorbable. On a hung judge the steps below then read the --init seed (status=error), take the existing panel-union path and post what the 8 cells found. A hung judge now costs the adjudication, not the panel.
The consolidate job cap goes 15 → 40, deliberately above the step so the step trips first. The 10m of slack covers this job's own overhead — fetch-depth: 0 checkout, CLI install, three artifact downloads, the post — which a panel cell does not all pay, so the judge's effective model budget is the step cap, not the job cap.
A cross-reference comment at the review cap records the coupling, so a future bump there does not silently recreate the same skew.

continue-on-error hides no real signal: cursor-agent's exit code is already captured in-script, and a judge that did not submit is reported to the PR as judge_status by Post review.

Verification

python3 -m unittest discover -s .github/workflow-pins/tests — 216 passed
python3 .github/workflow-pins/check_workflow_pins.py — OK, 11 workflows, none with a default ref
python3 -m unittest discover -s .github/cursor-review/tests — 194 passed
YAML parses; review = 30, consolidate job = 40, Run judge step = 30 with continue-on-error: true
check_public_repo_hygiene.py over the edited workflow: 13 findings, all pre-existing (BE-####) commit-convention refs, zero repo-name refs

Note for the merger

This touches .github/workflows/cursor-review.yml, so merging trips bump-cursor-review-callers.yml's path filter and rolls the cursor-review fleet. That is expected for any change to this file.

Provenance

Authored by: agent-work loop

Verified: workflow-pins (216) and cursor-review (194) unittest suites pass; check_workflow_pins.py clean; the workflow YAML parses and the three caps resolve as stated; check_public_repo_hygiene.py over the edited file reports no repo-name reference. Not verified end-to-end: the step-timeout → degraded-fallback path has not been exercised against a genuinely hung judge in CI — it is reasoned from the --init seed's status=error and the existing degraded branch in Build consolidated findings file.

Deviations: The job cap is 40, not the 30 the first commit used. The review panel asked for headroom above the panel cap, and the step-under-job idiom requires the job to outlive the step; 40 satisfies both. The panel also suggested asserting the review/consolidate coupling mechanically in check_workflow_pins.py — not done: that checker is scoped to workflows_ref pinning, and a timeout-coupling rule is unrelated machinery. A comment at the review cap records the coupling instead.

🤖 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/219 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/24/2026 **Status:** ✅ Merged **Merged:** 8/24/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/cursor-review-judge-timeout` --- ### 📝 Commits (2) - [`791af41`](https://github.com/Comfy-Org/github-workflows/commit/791af4124f9754ae40aecca2b22b00b613dafb23) fix(cursor-review): give the judge the same timeout as the panel it adjudicates - [`598cc56`](https://github.com/Comfy-Org/github-workflows/commit/598cc565e2a2a040c432b4711a651d96a6425e64) fix(cursor-review): time-box the judge STEP, and stop naming a consumer repo ### 📊 Changes **1 file changed** (+42 additions, -1 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/cursor-review.yml` (+42 -1) </details> ### 📄 Description ## ELI5 The Cursor review runs 8 model calls in parallel (the *panel*), then one more call (the *judge*) reads all 8 and writes the single review that lands on your PR. The panel was allowed 30 minutes; the judge only 15 — even though the judge has to read everything the panel produced. So on a big PR the panel would finish fine after ~25 minutes, and then the judge would get killed right at the end. All that work, no review posted. ## What was happening `consolidate` had `timeout-minutes: 15`; `review` (one panel cell) has 30. The judge calls `cursor-agent` once more with `prompt-judge.md` against all 8 cell artifacts, using the same top reasoning tier a single cell uses — the strictly harder call, on half the budget. A diff slow enough to stretch the panel stretches the judge with it, so the two go long together and only the judge has a tight cap. It also hid itself. GitHub reports a `timeout-minutes` kill as **`cancelled`**, not `failed`, so the run page reads as though someone stopped it by hand rather than a cap firing. ## Evidence Two occurrences on one consumer repo on 2026-08-24, both landing exactly on the cap: | Judge started | Killed | Elapsed | |---|---|---| | 19:49:57 | 20:05:10 | 15m13s | | 17:55:11 | 18:10:16 | 15m05s | In the first, all 8 panel cells succeeded between 19:24 and 19:49 — the slowest (`adversarial (claude-opus-5-thinking-max)`) took 25 minutes of `review`'s 30. Then the judge burned its full 15 and was killed. Nothing posted. Across the last 60 Cursor Review runs on that repo, 5 were `cancelled`: **2 were this judge timeout**, the other 3 were ordinary concurrency cancels (every job killed within seconds of start when a new push superseded the run). So ~3% of reviews were running a full panel and discarding the result. `notify-complete` already branches on `needs.consolidate.result`, so the DM correctly degraded to a warning — the loss was the review itself, not a false success signal. ## The change Raising the `consolidate` **job** cap alone would only move the cliff. A job-level `timeout-minutes` kill takes `Build consolidated findings file` and `Post review` with it, and that is the one failure mode that can never reach the degraded panel-union fallback the workflow already has — so a hung judge would still burn the budget and post nothing, just 30 minutes later. So the cap goes on the **step**, in the shape the `ledger` job already uses: - `Run judge` gets `timeout-minutes: 30` — the `review` panel cap, so the judge is never given less model time than one of the cells it reads — plus `continue-on-error: true`, which is what makes the timeout absorbable. On a hung judge the steps below then read the `--init` seed (`status=error`), take the existing panel-union path and **post what the 8 cells found**. A hung judge now costs the adjudication, not the panel. - The `consolidate` **job** cap goes `15` → `40`, deliberately above the step so the step trips first. The 10m of slack covers this job's own overhead — `fetch-depth: 0` checkout, CLI install, three artifact downloads, the post — which a panel cell does not all pay, so the judge's effective *model* budget is the step cap, not the job cap. - A cross-reference comment at the `review` cap records the coupling, so a future bump there does not silently recreate the same skew. `continue-on-error` hides no real signal: `cursor-agent`'s exit code is already captured in-script, and a judge that did not submit is reported to the PR as `judge_status` by `Post review`. ## Verification - `python3 -m unittest discover -s .github/workflow-pins/tests` — 216 passed - `python3 .github/workflow-pins/check_workflow_pins.py` — OK, 11 workflows, none with a default ref - `python3 -m unittest discover -s .github/cursor-review/tests` — 194 passed - YAML parses; `review` = 30, `consolidate` job = 40, `Run judge` step = 30 with `continue-on-error: true` - `check_public_repo_hygiene.py` over the edited workflow: 13 findings, all pre-existing `(BE-####)` commit-convention refs, zero repo-name refs ## Note for the merger This touches `.github/workflows/cursor-review.yml`, so merging trips `bump-cursor-review-callers.yml`'s path filter and rolls the cursor-review fleet. That is expected for any change to this file. ## Provenance **Authored by:** agent-work loop **Verified:** `workflow-pins` (216) and `cursor-review` (194) unittest suites pass; `check_workflow_pins.py` clean; the workflow YAML parses and the three caps resolve as stated; `check_public_repo_hygiene.py` over the edited file reports no repo-name reference. Not verified end-to-end: the step-timeout → degraded-fallback path has not been exercised against a genuinely hung judge in CI — it is reasoned from the `--init` seed's `status=error` and the existing `degraded` branch in `Build consolidated findings file`. **Deviations:** The job cap is 40, not the 30 the first commit used. The review panel asked for headroom above the panel cap, and the step-under-job idiom requires the job to outlive the step; 40 satisfies both. The panel also suggested asserting the review/consolidate coupling mechanically in `check_workflow_pins.py` — not done: that checker is scoped to `workflows_ref` pinning, and a timeout-coupling rule is unrelated machinery. A comment at the `review` cap records the coupling instead. 🤖 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

2026-10-26 22:37:34 +00:00