[PR #360] [MERGED] fix(cursor-review): gate a delta re-raise on an earlier round's open thread even without repeat_of #362

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: be-19480-gate-unmarked-reraises

📝 Commits (3)

29b44a3 fix(cursor-review): gate a delta re-raise on an earlier round's open thread even without repeat_of
06d8514 Merge origin/main into be-19480-gate-unmarked-reraises
b4491ab fix(cursor-review): gate delta re-raises on post-review's one pre-post thread snapshot

📊 Changes

7 files changed (+291 additions, -10 deletions)

View changed files

📝 .github/cursor-review/README.md (+1 -1)
📝 .github/cursor-review/auto-approve.py (+110 -2)
📝 .github/cursor-review/gate-unresolved.py (+6 -0)
📝 .github/cursor-review/post-review.py (+16 -1)
📝 .github/cursor-review/tests/test_auto_approve.py (+145 -0)
📝 .github/workflows/cursor-review.yml (+12 -5)
📝 docs/callers/cursor-review.md (+1 -1)

📄 Description

ELI-5

On review rounds after the first, approve_scope: delta lets the bot approve even when an old finding outside the new changes is still open. Re-raising a finding that is still open was supposed to block. It only did so when the judge linked the old thread (repeat_of), and the judge only links threads someone has already answered. An unanswered finding could therefore come back every round and the bot kept approving anyway. Now the bot checks where the finding sits: if it lands on the same file and line as an earlier round's thread that is still open, it blocks.

What changed

Of the three options, this PR takes option 2 (structural match, no model trust).

gate-unresolved.py: the shared reviewThreads query now also fetches each thread's path, line and startLine. These are additive fields.
auto-approve.py:
- earlier_thread_anchors() is pure. It collects {path: [(start, line)]} for every open (unresolved, non-outdated) cursor-review thread in the list it is given.
- with_open_anchors() reads the threads (only under delta) with a 90s wall-clock budget and adds the anchors to the scope.
- gating_reason() returns "on an unresolved earlier thread's line" for a finding inside one of those anchors.
- cmd_decide no longer reads threads for this. It gates on post-review's snapshot (--open-anchors), so marking and gating come from one read. A snapshot that is missing, malformed or for another commit makes the round fail closed to full.
post-review.py load_non_gating(): it reads the anchors before posting, so every thread in the snapshot is from an earlier round and no head-SHA filter is needed. A gating re-raise is posted without the non-gating marker, and the snapshot is saved for decide (--open-anchors-out). A failed or over-budget read (including the SystemExit(2) from run_graphql) marks nothing and leaves no snapshot, so decide uses full too.
cursor-review.yml: passes /tmp/open-anchors.json from Post review to Auto-approve decision. These are steps in the same job.
Docs: the approve_scope row in docs/callers/cursor-review.md, the inputs table in .github/cursor-review/README.md, and the workflow input description now say that a re-raise gates either by repeat_of or by landing on the path and line of an earlier round's still-open thread, and that an unreadable list of earlier threads fails closed to full.
Merged main, which brings in approve_authors (#358) and the approve_scope review fixes (#359). Main's rule that an empty or unparseable incremental block fails closed to full takes precedence.

Judgment call: the match covers any open earlier-round cursor-review thread, not only threads carrying the non-gating marker. That is a superset of what the ticket asked for, and it is what the docs already promised ("re-raises an earlier finding whose thread is still unresolved"). For an unmarked thread above the threshold, the open-thread check already blocks, so nothing changes there. The only new case is a below-threshold earlier thread re-raised above the threshold outside the delta, which now gates. That is the fail-closed direction.

Provenance

Authored by: agent-work loop
Verified:
- Tests: .github/cursor-review/tests passes (1023 tests), .github/cursor-approve/tests passes (149), and .github/workflow-pins/tests passes, all on the current head
- Regression: an open, marked-non-gating Medium thread whose finding is re-raised outside the delta with no repeat_of APPROVEs without the anchors and gets REQUEST_CHANGES with them. It also gates when the thread was opened on the reviewed head itself (head reset) or has a null originalCommit
- Snapshot hand-off: post-review's saved snapshot gives decide the same verdicts. A missing, foreign-commit or malformed snapshot fails decide closed to full, and a failed post-review read removes any stale snapshot. The walk budget is tested
- Negative cases: resolved, outdated, another line, another file, non-cursor thread, no line, no path, and multi-line bounds
- Lints: check_workflow_pins.py (including the input-drift harness), check_agents_md.py and check-org-repo-literals.sh all pass
- Not run: a live delta round against a real PR
Deviations: the anchor match covers all open earlier-round cursor threads, not only marked ones (a superset; see the judgment call above)

Residual

Exact-anchor match only. A re-raise anchored to a different line than the earlier thread (the model picks a neighbouring line, or the code moved and GitHub did not carry the thread's line along) still has no repeat_of and is not matched, so it stays non-gating under delta. Closing that would need either a line-proximity window or option 1 (the judge emits repeat_of for unanswered re-raises too). Both are design choices left for a follow-up.
Not exercised live. No real delta round was run against a GitHub PR. The new GraphQL fields (path, line, startLine on PullRequestReviewThread) are verified only by mocked tests. If the read fails on the first real delta round, the decision note will show "post-review's earlier open-thread snapshot is unavailable, so the round fails closed to full".
Extra read cost: under delta, post-review makes one additional paginated reviewThreads walk per round, capped at 90s. decide makes none.

🤖 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/360 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 10/7/2026 **Status:** ✅ Merged **Merged:** 10/7/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `be-19480-gate-unmarked-reraises` --- ### 📝 Commits (3) - [`29b44a3`](https://github.com/Comfy-Org/github-workflows/commit/29b44a39ca581c8175797274641039e4aea1e7db) fix(cursor-review): gate a delta re-raise on an earlier round's open thread even without repeat_of - [`06d8514`](https://github.com/Comfy-Org/github-workflows/commit/06d851472d82878da2b8995940eadfb5db7477db) Merge origin/main into be-19480-gate-unmarked-reraises - [`b4491ab`](https://github.com/Comfy-Org/github-workflows/commit/b4491abd24d3276711ebed4660eed0d07906a987) fix(cursor-review): gate delta re-raises on post-review's one pre-post thread snapshot ### 📊 Changes **7 files changed** (+291 additions, -10 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+1 -1) 📝 `.github/cursor-review/auto-approve.py` (+110 -2) 📝 `.github/cursor-review/gate-unresolved.py` (+6 -0) 📝 `.github/cursor-review/post-review.py` (+16 -1) 📝 `.github/cursor-review/tests/test_auto_approve.py` (+145 -0) 📝 `.github/workflows/cursor-review.yml` (+12 -5) 📝 `docs/callers/cursor-review.md` (+1 -1) </details> ### 📄 Description ## ELI-5 On review rounds after the first, `approve_scope: delta` lets the bot approve even when an old finding outside the new changes is still open. Re-raising a finding that is still open was supposed to block. It only did so when the judge linked the old thread (`repeat_of`), and the judge only links threads someone has already answered. An unanswered finding could therefore come back every round and the bot kept approving anyway. Now the bot checks where the finding sits: if it lands on the same file and line as an earlier round's thread that is still open, it blocks. ## What changed Of the three options, this PR takes **option 2 (structural match, no model trust)**. - `gate-unresolved.py`: the shared `reviewThreads` query now also fetches each thread's `path`, `line` and `startLine`. These are additive fields. - `auto-approve.py`: - `earlier_thread_anchors()` is pure. It collects `{path: [(start, line)]}` for every open (unresolved, non-outdated) cursor-review thread in the list it is given. - `with_open_anchors()` reads the threads (only under `delta`) with a 90s wall-clock budget and adds the anchors to the scope. - `gating_reason()` returns "on an unresolved earlier thread's line" for a finding inside one of those anchors. - `cmd_decide` no longer reads threads for this. It gates on post-review's snapshot (`--open-anchors`), so marking and gating come from one read. A snapshot that is missing, malformed or for another commit makes the round fail closed to `full`. - `post-review.py` `load_non_gating()`: it reads the anchors **before** posting, so every thread in the snapshot is from an earlier round and no head-SHA filter is needed. A gating re-raise is posted without the non-gating marker, and the snapshot is saved for decide (`--open-anchors-out`). A failed or over-budget read (including the `SystemExit(2)` from `run_graphql`) marks nothing and leaves no snapshot, so decide uses `full` too. - `cursor-review.yml`: passes `/tmp/open-anchors.json` from `Post review` to `Auto-approve decision`. These are steps in the same job. - Docs: the `approve_scope` row in `docs/callers/cursor-review.md`, the inputs table in `.github/cursor-review/README.md`, and the workflow input description now say that a re-raise gates either by `repeat_of` or by landing on the path and line of an earlier round's still-open thread, and that an unreadable list of earlier threads fails closed to `full`. - Merged `main`, which brings in `approve_authors` (#358) and the approve_scope review fixes (#359). Main's rule that an empty or unparseable incremental block fails closed to `full` takes precedence. **Judgment call:** the match covers *any* open earlier-round cursor-review thread, not only threads carrying the non-gating marker. That is a superset of what the ticket asked for, and it is what the docs already promised ("re-raises an earlier finding whose thread is still unresolved"). For an unmarked thread above the threshold, the open-thread check already blocks, so nothing changes there. The only new case is a below-threshold earlier thread re-raised above the threshold outside the delta, which now gates. That is the fail-closed direction. ## Provenance - **Authored by:** agent-work loop - **Verified:** - Tests: `.github/cursor-review/tests` passes (1023 tests), `.github/cursor-approve/tests` passes (149), and `.github/workflow-pins/tests` passes, all on the current head - Regression: an open, marked-non-gating Medium thread whose finding is re-raised outside the delta with no `repeat_of` APPROVEs without the anchors and gets REQUEST_CHANGES with them. It also gates when the thread was opened on the reviewed head itself (head reset) or has a null `originalCommit` - Snapshot hand-off: post-review's saved snapshot gives decide the same verdicts. A missing, foreign-commit or malformed snapshot fails decide closed to `full`, and a failed post-review read removes any stale snapshot. The walk budget is tested - Negative cases: resolved, outdated, another line, another file, non-cursor thread, no line, no path, and multi-line bounds - Lints: `check_workflow_pins.py` (including the input-drift harness), `check_agents_md.py` and `check-org-repo-literals.sh` all pass - Not run: a live `delta` round against a real PR - **Deviations:** the anchor match covers all open earlier-round cursor threads, not only marked ones (a superset; see the judgment call above) ## Residual - **Exact-anchor match only.** A re-raise anchored to a *different* line than the earlier thread (the model picks a neighbouring line, or the code moved and GitHub did not carry the thread's `line` along) still has no `repeat_of` and is not matched, so it stays non-gating under `delta`. Closing that would need either a line-proximity window or option 1 (the judge emits `repeat_of` for unanswered re-raises too). Both are design choices left for a follow-up. - **Not exercised live.** No real `delta` round was run against a GitHub PR. The new GraphQL fields (`path`, `line`, `startLine` on `PullRequestReviewThread`) are verified only by mocked tests. If the read fails on the first real `delta` round, the decision note will show "post-review's earlier open-thread snapshot is unavailable, so the round fails closed to `full`". - **Extra read cost:** under `delta`, post-review makes one additional paginated `reviewThreads` walk per round, capped at 90s. decide makes none. 🤖 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:02 +00:00