[PR #392] [MERGED] feat(cursor-review): make the per-cell agent cap an input (cell_timeout_minutes) #393

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/cell-timeout-input

📝 Commits (3)

3476cd7 feat(cursor-review): make the per-cell agent cap an input (cell_timeout_minutes)
c8474ac fix(cursor-review): compute derived timeout caps in preflight, not in expressions
9288f87 fix(cursor-review): floor cell_timeout_minutes at 10, keep the judge step at 30+, reject overlong values

📊 Changes

5 files changed (+484 additions, -30 deletions)

View changed files

📝 .github/cursor-review/README.md (+1 -0)
📝 .github/cursor-review/tests/test_stop_on_submit.py (+6 -1)
➕ .github/cursor-review/tests/test_workflow_cell_timeout.py (+349 -0)
📝 .github/workflows/cursor-review.yml (+127 -29)
📝 docs/callers/cursor-review.md (+1 -0)

📄 Description

The per-cell agent cap is a hardcoded timeout-minutes: 15 with no way for a caller to raise it, and it is now the dominant cause of red panels on the two busiest consumers.

Evidence

Every panel cell from 56 completed panels on one consumer — 372 cells, 43 red, classified from run annotations:

cause	reds
15-minute per-cell cap	22
Cursor `resource_exhausted`	21 (fixed separately by #388)

It is a clipped tail, not a hang catcher. Of the 329 cells that did submit:

 0-5  m  104 cells  ####################################################
 5-8  m   96        ################################################
 8-10 m   44        ######################
10-12 m   25        ############
12-14 m   19        #########
14-16 m   27        #############
16+   m   14        #######

18% finished within three minutes of the cap.

Caveat on the 22/43 split, added after review. That window mixes panel configurations — 372 cells over 56 panels is ~6.6 per panel, more than the current panel runs, and the 10 gpt-5.6-sol-max timeouts predate #378 notching that model down. So the split describes an older panel than the one running today, and should not be quoted as a forecast. What is not config-mixed, and is what this PR actually rests on: the duration histogram above (one metric, every submitting cell, panel-size independent), and grok-4.7-high on the current config — every cell that reached ~17 minutes died, 11 of 20, while the lanes with a 4-6 minute median never failed once.

Also corrected: #388 retries resource_exhausted only twice (30s, then 90s), so it does not "clear the capacity half" — a model that stays capacity-bound still fails its leg, as both kimi-k3-high cells did on one consumer PR. The timeout cap and the capacity errors are independent failure modes; this PR addresses only the first. The other busy consumer shows the same shape on a different lane: every grok-4.7-high cell that reached ~17 min died (11 of 20), while the lanes with a 4–6 min median never failed once. A cell that honestly needs 20 minutes has no way to ask.

The change

cell_timeout_minutes (number, default 15). The default reproduces today's four numbers exactly, so an unset input changes nothing.

The coupling is now in the workflow. Three further caps have to move with the cell cap, and until now a comment asked the next editor to do it by hand — "COUPLED to consolidate below — bump this and you must bump those too". That is the shape of every drift this repo has had. They are derived instead — computed in preflight's shell and exposed as job outputs (cell_job_timeout, judge_step_timeout, judge_job_timeout), which the three jobs read via fromJSON(needs.preflight.outputs.*), because Actions expressions have no arithmetic operators (${{ x + 15 }} fails the workflow at load):

cap	derivation	at the default
cell agent step	`cell_timeout_minutes`	15
cell job	`+ 15` (checkout, CLI install, artifacts)	30
judge step	`× 2`, never below 30 (reads every cell's findings at the same tier; its workload scales with the panel, not the cell cap)	30
judge job	`+ 10` (its own overhead)	40

Why each ordering matters rather than being tidiness:

A job cap at or under its own step cap turns a dead step into a cancelled job. For a cell that means Fail the leg when the cell did not submit never reads the artifact back, so a red leg becomes a cancelled run. For the judge it means every panel cell is discarded instead of falling back to the degraded union-of-findings review.
A cell cap raised past the judge's leaves the judge adjudicating a panel it cannot finish reading — the exact silent skew the 40/30 split was introduced to fix.

Bounded, not clamped

New preflight step, running before the panel fans out. type: number buys only numeric-ness — 0, -5, 1.5 and 600 all parse, and the judge's job cap doubles whatever gets through, so a typo'd 600 would mint a 20-hour job.

Bounded rather than clamped on purpose: silently correcting a caller's 600 to 45 would review a long diff on a budget nobody chose and report success. The step also prints every derived cap, since they are no longer visible in a diff of this file.

Floor 10 because the stop-on-submit wrapper may start its last resource_exhausted retry as late as 300s in (--retry-window 300), and that attempt still needs room for a healthy 3–7 minute review (at 5 a retried cell was all but certain to hit the cap); values longer than two digits are rejected before the numeric test, so an all-digit value past 64 bits cannot slip through an erroring [ -lt ]; ceiling 45 because that is already a 100-minute judge job, and anything past it is a diff-size problem, not a timeout problem.

Tests

test_workflow_cell_timeout.py pins it from four directions:

the exact expression at each of the six sites, plus the preflight outputs feeding them — a re-hardcoded literal at one site while the others stay derived is the regression;
no + - * / inside any ${{ }} in cursor-review.yml (with a self-test proving the check catches the original + 15 text) — a text-only pin of an invalid expression stays green while the workflow fails to load;
the ordering invariants, evaluated across the whole admitted range rather than at the default, because a derivation that holds at 15 and inverts at 45 is precisely what this guards;
the bound check by running its shell against real values, including the ones type: number still admits, and reading the derived caps back from a real $GITHUB_OUTPUT.

test_stop_on_submit.py is updated too — it pinned the literal.

Docs

Both places — docs/callers/cursor-review.md and .github/cursor-review/README.md. test_inputs_docs_drift.py checks that table two-way for this workflow, so an undocumented input fails CI. The caller guide spells out the "is it a tail or a hang" test, so the knob gets raised on evidence rather than by reflex.

Not in this PR

Nothing changes for any caller until one passes the input. The two consumers that need it get it in their own caller PRs once this lands and the fleet bump rolls.

Provenance

Authored by: agent-work loop
Verified:

python3 -m unittest discover -s .github/cursor-review/tests: 1280 tests OK at 9288f87
.github/cursor-approve/tests and .github/workflow-pins/tests OK; check_workflow_pins.py OK
check_agents_md.py --root . passed; check-org-repo-literals.sh OK
actionlint on cursor-review.yml at c8474ac reported no timeout-minutes expression errors; not re-run at 9288f87, which changes only the preflight shell and comments
Deviations:
Derived caps are computed in preflight and passed as job outputs, not written as inline expressions, because Actions cannot evaluate arithmetic
Review follow-up: the floor is 10, not the planned 5, and the judge step is max(30, 2 × cell), not a plain 2 × cell

🤖 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/392 **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/cell-timeout-input` --- ### 📝 Commits (3) - [`3476cd7`](https://github.com/Comfy-Org/github-workflows/commit/3476cd7195ad52630461460a75d12fb55a56cf77) feat(cursor-review): make the per-cell agent cap an input (cell_timeout_minutes) - [`c8474ac`](https://github.com/Comfy-Org/github-workflows/commit/c8474ac709752d617341a56cc729532a2d4c50a2) fix(cursor-review): compute derived timeout caps in preflight, not in expressions - [`9288f87`](https://github.com/Comfy-Org/github-workflows/commit/9288f873be7bb658c744d8b3c7e858c61115b161) fix(cursor-review): floor cell_timeout_minutes at 10, keep the judge step at 30+, reject overlong values ### 📊 Changes **5 files changed** (+484 additions, -30 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+1 -0) 📝 `.github/cursor-review/tests/test_stop_on_submit.py` (+6 -1) ➕ `.github/cursor-review/tests/test_workflow_cell_timeout.py` (+349 -0) 📝 `.github/workflows/cursor-review.yml` (+127 -29) 📝 `docs/callers/cursor-review.md` (+1 -0) </details> ### 📄 Description The per-cell agent cap is a hardcoded `timeout-minutes: 15` with no way for a caller to raise it, and it is now the dominant cause of red panels on the two busiest consumers. ## Evidence Every panel cell from 56 completed panels on one consumer — **372 cells, 43 red**, classified from run annotations: | cause | reds | |---|---| | **15-minute per-cell cap** | **22** | | Cursor `resource_exhausted` | 21 (fixed separately by #388) | It is a **clipped tail, not a hang catcher**. Of the 329 cells that *did* submit: ``` 0-5 m 104 cells #################################################### 5-8 m 96 ################################################ 8-10 m 44 ###################### 10-12 m 25 ############ 12-14 m 19 ######### 14-16 m 27 ############# 16+ m 14 ####### ``` **18% finished within three minutes of the cap.** **Caveat on the 22/43 split, added after review.** That window **mixes panel configurations** — 372 cells over 56 panels is ~6.6 per panel, more than the current panel runs, and the 10 `gpt-5.6-sol-max` timeouts predate #378 notching that model down. So the split describes an older panel than the one running today, and should not be quoted as a forecast. What is *not* config-mixed, and is what this PR actually rests on: the duration histogram above (one metric, every submitting cell, panel-size independent), and `grok-4.7-high` on the current config — every cell that reached ~17 minutes died, 11 of 20, while the lanes with a 4-6 minute median never failed once. Also corrected: #388 retries `resource_exhausted` only twice (30s, then 90s), so it does not "clear the capacity half" — a model that stays capacity-bound still fails its leg, as both `kimi-k3-high` cells did on one consumer PR. The timeout cap and the capacity errors are independent failure modes; this PR addresses only the first. The other busy consumer shows the same shape on a different lane: every `grok-4.7-high` cell that reached ~17 min died (11 of 20), while the lanes with a 4–6 min median never failed once. A cell that honestly needs 20 minutes has no way to ask. ## The change `cell_timeout_minutes` (number, default 15). The default reproduces today's four numbers exactly, so an unset input changes nothing. **The coupling is now in the workflow.** Three further caps have to move with the cell cap, and until now a comment asked the next editor to do it by hand — *"COUPLED to `consolidate` below — bump this and you must bump those too"*. That is the shape of every drift this repo has had. They are derived instead — computed in preflight's shell and exposed as job outputs (`cell_job_timeout`, `judge_step_timeout`, `judge_job_timeout`), which the three jobs read via `fromJSON(needs.preflight.outputs.*)`, because Actions expressions have no arithmetic operators (`${{ x + 15 }}` fails the workflow at load): | cap | derivation | at the default | |---|---|---| | cell agent step | `cell_timeout_minutes` | 15 | | cell job | `+ 15` (checkout, CLI install, artifacts) | 30 | | judge step | `× 2`, never below 30 (reads **every** cell's findings at the same tier; its workload scales with the panel, not the cell cap) | 30 | | judge job | `+ 10` (its own overhead) | 40 | Why each ordering matters rather than being tidiness: - A **job** cap at or under its own **step** cap turns a dead step into a *cancelled job*. For a cell that means `Fail the leg when the cell did not submit` never reads the artifact back, so a red leg becomes a cancelled run. For the judge it means every panel cell is discarded instead of falling back to the degraded union-of-findings review. - A cell cap raised past the judge's leaves the judge adjudicating a panel it cannot finish reading — the exact silent skew the 40/30 split was introduced to fix. ## Bounded, not clamped New preflight step, running **before** the panel fans out. `type: number` buys only numeric-ness — `0`, `-5`, `1.5` and `600` all parse, and the judge's job cap *doubles* whatever gets through, so a typo'd 600 would mint a 20-hour job. Bounded rather than clamped on purpose: silently correcting a caller's 600 to 45 would review a long diff on a budget nobody chose and report success. The step also prints every derived cap, since they are no longer visible in a diff of this file. Floor 10 because the stop-on-submit wrapper may start its last `resource_exhausted` retry as late as 300s in (`--retry-window 300`), and that attempt still needs room for a healthy 3–7 minute review (at 5 a retried cell was all but certain to hit the cap); values longer than two digits are rejected before the numeric test, so an all-digit value past 64 bits cannot slip through an erroring `[ -lt ]`; ceiling 45 because that is already a 100-minute judge job, and anything past it is a diff-size problem, not a timeout problem. ## Tests `test_workflow_cell_timeout.py` pins it from four directions: 1. the exact expression at each of the six sites, plus the preflight outputs feeding them — a re-hardcoded literal at one site while the others stay derived is the regression; 2. no `+ - * /` inside any `${{ }}` in `cursor-review.yml` (with a self-test proving the check catches the original `+ 15` text) — a text-only pin of an invalid expression stays green while the workflow fails to load; 3. the ordering invariants, evaluated across the **whole admitted range** rather than at the default, because a derivation that holds at 15 and inverts at 45 is precisely what this guards; 4. the bound check by **running its shell** against real values, including the ones `type: number` still admits, and reading the derived caps back from a real `$GITHUB_OUTPUT`. `test_stop_on_submit.py` is updated too — it pinned the literal. ## Docs Both places — `docs/callers/cursor-review.md` and `.github/cursor-review/README.md`. `test_inputs_docs_drift.py` checks that table two-way for this workflow, so an undocumented input fails CI. The caller guide spells out the "is it a tail or a hang" test, so the knob gets raised on evidence rather than by reflex. ## Not in this PR Nothing changes for any caller until one passes the input. The two consumers that need it get it in their own caller PRs once this lands and the fleet bump rolls. ## Provenance **Authored by:** agent-work loop **Verified:** - `python3 -m unittest discover -s .github/cursor-review/tests`: 1280 tests OK at 9288f87 - `.github/cursor-approve/tests` and `.github/workflow-pins/tests` OK; `check_workflow_pins.py` OK - `check_agents_md.py --root .` passed; `check-org-repo-literals.sh` OK - `actionlint` on `cursor-review.yml` at c8474ac reported no `timeout-minutes` expression errors; not re-run at 9288f87, which changes only the preflight shell and comments **Deviations:** - Derived caps are computed in preflight and passed as job outputs, not written as inline expressions, because Actions cannot evaluate arithmetic - Review follow-up: the floor is 10, not the planned 5, and the judge step is `max(30, 2 × cell)`, not a plain `2 × cell` 🤖 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:39:19 +00:00