Comfy-Org/github-workflows

Fork 0

mirror of https://github.com/Comfy-Org/github-workflows.git synced 2026-11-05 00:17:37 +00:00

[PR #379] [MERGED] feat(cursor-review): end a panel cell's agent once findings.json reports status=ok #379

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: be-19660-cursor-review-early-stop

📝 Commits (2)

7630b11 feat(cursor-review): end a panel cell's agent once findings.json reports status=ok
593cef2 fix(cursor-review): forward the step's signals to stop-on-submit and reap the agent group on every exit

📊 Changes

4 files changed (+624 additions, -8 deletions)

View changed files

📝 .github/cursor-review/README.md (+1 -0)
➕ .github/cursor-review/stop-on-submit.py (+216 -0)
➕ .github/cursor-review/tests/test_stop_on_submit.py (+362 -0)
📝 .github/workflows/cursor-review.yml (+45 -8)

📄 Description

Summary

Some panel cells keep running after their agent has already finished the review. cursor_review_finish writes status: ok to findings.json, but cursor-agent sometimes doesn't exit, so the cell sits idle until the 15-minute Run cursor review step cap kills it.

stop-on-submit.py is a new stdlib-only wrapper around the reviewer's cursor-agent in Run cursor review. It works like this:

It starts the agent in its own process group and reads /tmp/findings-out/findings.json every 5s.
It acts only when the file is complete JSON, parses to an object, and has status equal to the exact string "ok". When that happens, the agent gets 15s to exit on its own. The wrapper then reads the file again. If it still says ok, the wrapper sends SIGTERM to the whole group, then SIGKILL to the group 10s later. Children such as the MCP server or a spawned shell can't outlive the step.
N = 30s. A cell that has submitted exits within about 30 seconds of submitting (5 + 15 + 10). This is documented in the step comment and in the cursor-review README.
An early stop exits 0. If the agent exits on its own, before or after submitting, its exit code passes through unchanged. A signal death is reported as 128+n, the way a shell reports it.
The wrapper only reads findings.json. The uploaded artifact, the leg check and the judge all get exactly the bytes the MCP tool wrote. The leg check and Aggregate panel findings read status, so an early stop counts as a submitted cell, not an error.
If the file is missing, half-written, unparseable, not an object, or has any status other than ok, the wrapper never stops the agent. A cell that never submits runs to the cap and fails its leg as it does today.
Signals and cleanup. The step cap or a run cancel signals the step shell. The shell runs the wrapper in the background and traps INT/TERM/HUP to forward them, so the wrapper gets the signal too. The wrapper installs its handlers before it spawns the agent and passes the signal to the agent's group. The group gets at most 5s, then SIGKILL, and the wrapper exits 128+signal even if the agent caught the signal and exited 0. The step also exits with that status. Every exit path, including an agent that exits on its own, ends with SIGKILL to the group, so nothing the agent left running can rewrite findings.json after the upload.
submitted() reads only a regular file (O_NONBLOCK|O_NOFOLLOW plus fstat), capped at 8 MiB. A FIFO, symlink, device or oversized file counts as not submitted. --poll/--linger/--grace reject nan, inf and values ≤ 0.

Test evidence

New tests/test_stop_on_submit.py (25 tests):
- Trigger cases for submitted(): ok, never written, error status, every possible truncation of an ok record, non-object or non-string status, "OK", invalid UTF-8, very deep nesting, a directory at the path, a FIFO (does not block), a symlink, an oversized file.
- End-to-end runs of the real wrapper against a stub agent:
  - An ok cell is stopped early, and its findings bytes are identical to what the agent wrote, odd formatting included.
  - A non-ok cell, a partial write and a never-written file each keep running past a stand-in cap.
  - The agent's exit code passes through.
  - An agent that exits during the 15s wait is never signalled.
  - A status that changes back from ok during the wait does not trigger a stop.
  - An agent and child that both ignore SIGTERM are SIGKILLed.
  - SIGTERM sent to the wrapper reaches the agent. An agent and grandchild that both ignore it are SIGKILLed within the bounded grace, and the wrapper exits 143.
  - A grandchild left behind by an agent that exits on its own is killed, and the agent's exit code still passes through.
  - nan, inf, 0 and -1 timings are usage errors.
- A structural check that Run cursor review routes cursor-agent through the wrapper, backgrounds it, and traps INT/TERM/HUP before the launch.

Dry run of the step's exact shell shape with default timings and stub agents. Each stub writes its status after 3s, then sleeps for 900s:

cell=ok exit=0 elapsed=20s findings={"status": "ok", "findings": []}
stop-on-submit: findings report status=ok; giving the agent 15s to exit on its own
stop-on-submit: agent still running after submitting; sending SIGTERM
stop-on-submit: agent stopped 0.0s after SIGTERM; findings left as written
cell=error exit=124 elapsed=75s findings={"status": "error", "findings": []}
no stub left

For the error cell, timeout 75 stood in for the 15-minute cap. Its SIGTERM was passed on, and no stub process was left over.

Provenance

Authored by: agent-work loop
Verified:
- Tests: cursor-review suite green (1172 tests, including 25 in test_stop_on_submit.py). Each new review-round test fails or hangs against the previous wrapper.
- Lint: check_workflow_pins.py OK, check_agents_md.py passed (2 warnings, none in a file this PR touches), org-repo-literal lint OK, shellcheck clean on the extracted Run cursor review script.
- Step simulation: the extracted Run cursor review script with stub agents. An ok cell records exit 0 and an exiting cell records 3. On SIGTERM to the step shell, a cell that ignores signals exits 143 in about 5s, with no agent or grandchild left.
- Not run: a live panel run on an Actions runner with the real cursor-agent, and actionlint, which is not installed locally.
Deviations: none from the acceptance criteria. N was chosen as 30s.

Residual

No live run yet. All the evidence comes from stub agents on a dev host. The first real panel run after this lands should show stop-on-submit: lines in the === Stderr === section of a cell that submitted, and that cell's Run cursor review step should finish well under 15 minutes. Check whether the sol-lane model cells that used to hit the cap after writing status: ok (the effort tier changes in #378) now finish early.
Runner signal order is simulated, not observed live. The design assumes the runner signals the step shell with SIGINT, then SIGTERM about 7.5s later, then kills it. The shell forwards both signals, and the wrapper SIGKILLs the agent's group within 5s of the first, before the runner's own kill. This was checked by signalling the extracted step script locally, not on a live runner.
The judge is not covered. The judge's Run judge step (30-minute cap, same cursor-agent pattern) can idle after cursor_review_submit_final in the same way. The ticket covers only panel cells, so it is not wrapped. The same wrapper would work there with --findings /tmp/judge-findings.json, but that step has its own fallback logic to check first.
The cap still decides the outcome for an agent that writes status: ok and then rewrites the file to non-ok. That case is handled (no stop), but such a cell runs to the cap exactly as it does today.

🤖 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/379 **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:** `be-19660-cursor-review-early-stop` --- ### 📝 Commits (2) - [`7630b11`](https://github.com/Comfy-Org/github-workflows/commit/7630b11da4cd3fc349ecc21b301f05bbf3efbbfc) feat(cursor-review): end a panel cell's agent once findings.json reports status=ok - [`593cef2`](https://github.com/Comfy-Org/github-workflows/commit/593cef287f5d8113807809277cf02071d64c04a0) fix(cursor-review): forward the step's signals to stop-on-submit and reap the agent group on every exit ### 📊 Changes **4 files changed** (+624 additions, -8 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+1 -0) ➕ `.github/cursor-review/stop-on-submit.py` (+216 -0) ➕ `.github/cursor-review/tests/test_stop_on_submit.py` (+362 -0) 📝 `.github/workflows/cursor-review.yml` (+45 -8) </details> ### 📄 Description ## Summary Some panel cells keep running after their agent has already finished the review. `cursor_review_finish` writes `status: ok` to `findings.json`, but `cursor-agent` sometimes doesn't exit, so the cell sits idle until the 15-minute `Run cursor review` step cap kills it. `stop-on-submit.py` is a new stdlib-only wrapper around the reviewer's `cursor-agent` in `Run cursor review`. It works like this: - It starts the agent in its own process group and reads `/tmp/findings-out/findings.json` every **5s**. - It acts only when the file is complete JSON, parses to an object, and has `status` equal to the exact string `"ok"`. When that happens, the agent gets **15s** to exit on its own. The wrapper then reads the file again. If it still says `ok`, the wrapper sends SIGTERM to the whole group, then SIGKILL to the group **10s** later. Children such as the MCP server or a spawned shell can't outlive the step. - **N = 30s.** A cell that has submitted exits within about 30 seconds of submitting (5 + 15 + 10). This is documented in the step comment and in the cursor-review README. - An early stop exits **0**. If the agent exits on its own, before or after submitting, its exit code passes through unchanged. A signal death is reported as 128+n, the way a shell reports it. - The wrapper **only reads** `findings.json`. The uploaded artifact, the leg check and the judge all get exactly the bytes the MCP tool wrote. The leg check and `Aggregate panel findings` read `status`, so an early stop counts as a submitted cell, not an error. - If the file is missing, half-written, unparseable, not an object, or has any status other than `ok`, the wrapper never stops the agent. A cell that never submits runs to the cap and fails its leg as it does today. - **Signals and cleanup.** The step cap or a run cancel signals the step shell. The shell runs the wrapper in the background and traps INT/TERM/HUP to forward them, so the wrapper gets the signal too. The wrapper installs its handlers before it spawns the agent and passes the signal to the agent's group. The group gets at most 5s, then SIGKILL, and the wrapper exits 128+signal even if the agent caught the signal and exited 0. The step also exits with that status. Every exit path, including an agent that exits on its own, ends with SIGKILL to the group, so nothing the agent left running can rewrite `findings.json` after the upload. - `submitted()` reads only a regular file (`O_NONBLOCK|O_NOFOLLOW` plus `fstat`), capped at 8 MiB. A FIFO, symlink, device or oversized file counts as not submitted. `--poll`/`--linger`/`--grace` reject nan, inf and values ≤ 0. ## Test evidence - New `tests/test_stop_on_submit.py` (25 tests): - Trigger cases for `submitted()`: ok, never written, error status, every possible truncation of an `ok` record, non-object or non-string status, `"OK"`, invalid UTF-8, very deep nesting, a directory at the path, a FIFO (does not block), a symlink, an oversized file. - End-to-end runs of the real wrapper against a stub agent: - An `ok` cell is stopped early, and its findings bytes are identical to what the agent wrote, odd formatting included. - A non-ok cell, a partial write and a never-written file each keep running past a stand-in cap. - The agent's exit code passes through. - An agent that exits during the 15s wait is never signalled. - A status that changes back from `ok` during the wait does not trigger a stop. - An agent and child that both ignore SIGTERM are SIGKILLed. - SIGTERM sent to the wrapper reaches the agent. An agent and grandchild that both ignore it are SIGKILLed within the bounded grace, and the wrapper exits 143. - A grandchild left behind by an agent that exits on its own is killed, and the agent's exit code still passes through. - nan, inf, 0 and -1 timings are usage errors. - A structural check that `Run cursor review` routes `cursor-agent` through the wrapper, backgrounds it, and traps INT/TERM/HUP before the launch. - Dry run of the step's exact shell shape with default timings and stub agents. Each stub writes its status after 3s, then sleeps for 900s: ``` cell=ok exit=0 elapsed=20s findings={"status": "ok", "findings": []} stop-on-submit: findings report status=ok; giving the agent 15s to exit on its own stop-on-submit: agent still running after submitting; sending SIGTERM stop-on-submit: agent stopped 0.0s after SIGTERM; findings left as written cell=error exit=124 elapsed=75s findings={"status": "error", "findings": []} no stub left ``` For the error cell, `timeout 75` stood in for the 15-minute cap. Its SIGTERM was passed on, and no stub process was left over. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests: cursor-review suite green (1172 tests, including 25 in `test_stop_on_submit.py`). Each new review-round test fails or hangs against the previous wrapper. - Lint: `check_workflow_pins.py` OK, `check_agents_md.py` passed (2 warnings, none in a file this PR touches), org-repo-literal lint OK, shellcheck clean on the extracted `Run cursor review` script. - Step simulation: the extracted `Run cursor review` script with stub agents. An `ok` cell records exit 0 and an exiting cell records 3. On SIGTERM to the step shell, a cell that ignores signals exits 143 in about 5s, with no agent or grandchild left. - Not run: a live panel run on an Actions runner with the real `cursor-agent`, and actionlint, which is not installed locally. - **Deviations:** none from the acceptance criteria. N was chosen as 30s. ## Residual - **No live run yet.** All the evidence comes from stub agents on a dev host. The first real panel run after this lands should show `stop-on-submit:` lines in the `=== Stderr ===` section of a cell that submitted, and that cell's `Run cursor review` step should finish well under 15 minutes. Check whether the sol-lane model cells that used to hit the cap after writing `status: ok` (the effort tier changes in #378) now finish early. - **Runner signal order is simulated, not observed live.** The design assumes the runner signals the step shell with SIGINT, then SIGTERM about 7.5s later, then kills it. The shell forwards both signals, and the wrapper SIGKILLs the agent's group within 5s of the first, before the runner's own kill. This was checked by signalling the extracted step script locally, not on a live runner. - **The judge is not covered.** The judge's `Run judge` step (30-minute cap, same `cursor-agent` pattern) can idle after `cursor_review_submit_final` in the same way. The ticket covers only panel cells, so it is not wrapped. The same wrapper would work there with `--findings /tmp/judge-findings.json`, but that step has its own fallback logic to check first. - **The cap still decides the outcome for an agent that writes `status: ok` and then rewrites the file to non-ok.** That case is handled (no stop), but such a cell runs to the cap exactly as it does today. 🤖 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:12 +00:00

closed this issue
added the
pull-request
label

No labels

cursor-review-catalog-drift

groom

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Comfy-Org/github-workflows#379

No description provided.

Rows
Columns