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 #322] [MERGED] fix(cursor-review): record each round's merge base and pin the incremental block's OLD patch to it #322

New issue

Closed

opened 2026-10-26 22:38:28 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:38:28 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-15598-round-sentinel-merge-base

📝 Commits (6)

da6cbc8 fix(cursor-review): build the incremental hunks block as a subset of the reviewed diff
a2da829 fix(cursor-review): close seven gaps the panel found in the incremental block
74bbcff fix(cursor-review): record each round's merge base and pin the incremental block's OLD patch to it
65f04d9 fix(cursor-review): refuse a round sentinel from a round that reviewed nothing
c3a2721 Merge origin/main into matt/be-15598-round-sentinel-merge-base
cec0e33 fix(cursor-review): trim the superseded three-dot prose, and stop re-collecting the end-to-end suite

📊 Changes

9 files changed (+1088 additions, -35 deletions)

View changed files

📝 .github/cursor-review/README.md (+3 -1)
📝 .github/cursor-review/build-ledger.py (+137 -0)
📝 .github/cursor-review/incremental-diff.py (+14 -6)
📝 .github/cursor-review/post-review.py (+109 -4)
📝 .github/cursor-review/tests/test_build_ledger.py (+242 -0)
📝 .github/cursor-review/tests/test_incremental_diff.py (+174 -2)
📝 .github/cursor-review/tests/test_post_review.py (+282 -4)
📝 .github/workflows/cursor-review.yml (+124 -17)
📝 docs/callers/cursor-review.md (+3 -1)

📄 Description

No longer stacked — this targets main. #321 merged on 2026-09-20 and its branch was deleted, so GitHub retargeted this PR onto main; the earlier "do not merge before #321" banner no longer applies.

#321 squash-landed this branch's first two commits, so what remains here is the BE-15598 half: the round sentinel and the merge-base pin. origin/main has been merged in (c3a2721) and the conflicts — the overlap with #321 plus the prose and the OLD-patch command this branch supersedes — resolved in this branch's favour, since each is the BE-15598 rewrite of text #321 introduced. Main's independent work merged untouched: #328's workflows_ref-vs-uses: guard is intact, and #323's POST-failure handling composes with the sentinel (the sentinel rides review_header; post_error_review and the all-panel-cells-failed body keep the plain, sentinel-free header).

ELI-5

The review panel gets a "here's what's new since last time" block so it knows where to spend its budget. Building that block needs to know what the previous round already showed the panel, and until now we worked that out by re-deriving it from the PR's base branch today. If the PR gets retargeted — or its base branch is rewritten — "today" is a different fork point than the one the last round used, so code the panel has genuinely never seen gets subtracted as "already reviewed" and silently disappears from the block. So each review now writes down, in an invisible HTML comment under its header, exactly which commit it diffed against; the next round reads that back and uses it. If there's nothing written down, we show no block at all rather than guessing — an unprioritized full diff is always correct, a quietly shrunken one is not.

What changed

1. diff-size resolves and publishes this round's merge base. A new shell step right after the workflows_ref guard runs git merge-base "$BASE_SHA" "$HEAD_SHA" and publishes it as the job's merge_base_sha output. Shell rather than a check-pr-size output on purpose: the tool is skipped entirely on the degraded raw-numstat fallback path, and a round that records no merge base costs the next round its whole block. It never fails the job — an unresolvable merge base is recorded as empty and warned about.

2. post-review writes the round sentinel. post-review.py gains --base-sha / --merge-base-sha and render_round_sentinel(), which emits  (sorted keys, tight separators, every field validated ^[0-9a-f]{40}$ or written as ""). It goes on its own line directly under the header — above the notice and ledger-note banners — in the findings body, the "No high-signal findings" body, and the wholesale-422 fallback body, which derives from the same head. It is not emitted by the two bodies whose round reviewed nothing: post_error_review, and the "Panel did not produce any findings" body posted when every panel cell errored. defang_body_only_contract now breaks this prefix too, alongside the other two.

The job takes diff-size into needs: for that one output. It was already a transitive dependency (consolidate → review → diff-size), so this adds no ordering.

3. build-ledger.py reads it back. _ROUND_SENTINEL_RE is pinned to the single-spaced opener and anchored to a line start, _parse_round_sentinel is tolerant (None on missing / unknown version / bad JSON / wrong shape, never raises). It is read from consolidated[-1] only, refused outright if that body is the error-review shape (_ERROR_REVIEW_RE, the same refusal _body_only_entries applies and for the same reason), and otherwise accepted only when payload["head"] == review["commit_id"] (with a non-empty commit_id, so "" == "" is not a way in) and merge_base is full lowercase hex. The result reaches every ledger shape (ok / empty / unknown / disabled), _write_outputs, and the ledger job's new last_reviewed_merge_base output.

4. The incremental step pins OLD and fails closed. build_old_patch is now git -c core.quotePath=false diff "${LAST_REVIEWED_MERGE_BASE}" "${LAST_REVIEWED_SHA}" — two-dot, because the recorded merge base is the exact left tree round N diffed, and check-pr-size's writeReviewedDiff builds NEW as mergeBase...head, which resolves to the same two-tree diff (scripts/check-pr-size/main.go:166-175). Two new elif branches sit between the last-reviewed-SHA reachability check and the empty-FULL_PATCH check, each with a distinct sweepable log line, each leaving the block empty with incremental_subset=true: no recorded merge base, and a recorded merge base that is unreachable or no longer an ancestor of the last-reviewed commit. BASE_SHA is gone from the step's env: — nothing else in it used it — so the fallback that caused this cannot be reached by accident.

5. Docs. The ledger sections of .github/cursor-review/README.md and docs/callers/cursor-review.md each get a paragraph on the sentinel, the pinned OLD, and the one block-less round every open PR sees after rollout.

Round 2 — the panel's findings

The six-model panel raised nine. Four became code, two became docs, two became comments, one was answered on the thread.

🟡 The round sentinel was readable out of an ERROR review. _body_only_entries already refuses that shape; _parse_round_sentinel's caller did not, even though post_error_review is the one consolidated body that renders imported judge/CLI text inside a fence, at column 0 — where the sentinel's line anchor, its whole containment argument, buys nothing. The writer-side defang this PR added covers only bodies written by this version, while consumers stay pinned to older SHAs and every error review already sitting on their PRs is undefanged. The head == commit_id gate does not close it either: the reviewed head SHA is public to the PR author. Fixed at the reader, which is the half a slow fleet cannot outrun. A comment in post-review.py was already asserting this refusal existed.
🟢 The all-panel-cells-failed body recorded a merge base for a panel that never ran. It renders review_header while being posted with delivers=False, which contradicted the stated rationale for withholding the sentinel from post_error_review — and unlike the error review it carries no "Review failed" heading, so the reader-side refusal above does not reach it. It now takes the plain header. The next round therefore fails closed to "no block" instead of subtracting hunks nobody looked at.
🟢 ^[0-9a-f]{40}$ accepted a trailing newline. Python's $ matches before a final newline, so a 41-character value passed the gate whose adjacent comment calls it the single control keeping newlines out of $GITHUB_OUTPUT. Both the reader's _FULL_SHA_RE and the writer's _ROUND_SHA_RE now end \Z.
🟢 Neither forgery test could fail for the reason it claimed. search returns the first match and the genuine sentinel sits on line 2, ahead of the blockquoted forgery, so both passed with the line anchor removed entirely — nothing pinned the column-0 containment, which is the regression the module comments record happening once already. Added a case carrying only a quoted sentinel, across every indent a rendered finding can sit behind (>, > > , list items, tabs, four-space).
🟢 Both docs asserted the superseded formulation immediately above the paragraph reversing it, so a reader going top-down got the pre-BE-15598 behaviour first. Folded in — and both now also say which rounds record no sentinel at all.

Answered rather than changed, because neither is reachable and a guard that cannot fire is dead code:

⚪ MERGE_BASE has no line-break guard, unlike resolveMergeBase in the Go tool. git merge-base on two commits with no --all prints exactly one line and $( ) strips the trailing newline, so there is no value this step can emit that the Go check would refuse. Noted in the step, with why the two deliberately diverge.
⚪ LAST_REVIEWED_SHA is a positional git diff argument and is never shape-validated. True, and the reviewer's own note is the answer: the preceding git cat-file -e "${LAST_REVIEWED_SHA}^{commit}" already rejects anything option-shaped or unresolvable, and the value's provenance is GitHub's commit_id, not the PR. Left alone.

Deferred, with the reasoning recorded where the step explains itself rather than in a ticket:

🟢 On the retarget path, the block comes out close to the full diff and the prompt carries it twice. Real, and check cannot catch it (foreign == 0, new_lines <= full_lines — the block is a faithful subset, just a useless one). The cost is prompt budget on an uncommon path, never coverage. Declining a block whose length approaches full_lines needs a threshold nothing here has grounds to pick, so it is named as a known waste in the step's comments rather than guessed at.

Judgment calls

base is validated as a full hex SHA too, which the plan left as a bare payload.get("base",""). _write_outputs appends it to $GITHUB_OUTPUT, where a value carrying a newline is an output-injection vector, and the accept path is reachable from any Bot-authored consolidated review. The writer only ever emits hex-or-empty, so the check costs nothing real and keeps one control over the whole payload. test_a_non_hex_base_is_dropped_rather_than_written_through pins it.
The ancestor check on the recorded merge base is slightly beyond the plan's "unreachable" wording. A rewritten PR branch can leave the recorded commit reachable but no longer in the last-reviewed commit's history, in which case the diff succeeds and returns something that is not what round N saw.
The wholesale-422 fallback body carries the sentinel too. It falls out of sharing review_head, and it is a real review of that commit, so the next round should be able to read it.
The merge-base step sits after the workflows_ref guard rather than immediately after the checkout, to keep that guard's fail-fast position. It is still before every other step in the job, so both the classifier and the degraded fallback path are covered.
--base-sha / --merge-base-sha are passed to all three call sites including the error-review one, per the plan; post_error_review ignores them.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py': 720 tests, 0 failures, re-run on the merged tree. (Was 705 when this PR was opened, then 725 after merging main, then 720 once the duplicate collection CodeRabbit flagged was removed — the drop is exactly the five re-collected cases, no case lost.) Each of the four new tests was additionally confirmed to FAIL against the un-fixed code, by reverting all four fixes in a scratch copy and re-running — a test that passes either way pins nothing, which is precisely the defect finding 4 above reports. python3 .github/workflow-pins/check_workflow_pins.py: OK, 11 workflows, 156 uses: pinned; python3 -m unittest discover -s .github/workflow-pins/tests: 502 tests OK. bash .github/lint/check-org-repo-literals.sh: OK. python3 .github/agents-md-integrity/check_agents_md.py --root .: passed (2 pre-existing warnings, unrelated). shellcheck -x .github/cursor-review/install-cursor-cli.sh .github/cursor-review/slack-notify.sh: clean, plus shellcheck -S warning over the two run: blocks this PR edits, extracted from the YAML: clean. python3 -c yaml.safe_load over cursor-review.yml to confirm the new outputs and needs: parse.
Deviations: origin/main merged in and its conflicts resolved as described at the top, plus two CodeRabbit findings fixed in cec0e33 (the superseded three-dot prose in incremental-diff.py and cursor-review.yml; RoundSentinelTest re-collecting EndToEndPostTest's five cases, fixed by moving the shared harness to a PostHarness mixin). Earlier: the base-SHA validation and the ancestor check above; the two panel nits answered in comments rather than code, and the retarget doubled-prompt finding deferred to a code comment rather than a ticket (all three are listed with their reasoning under Round 2); the live two-round consumer run named under Verification is still not exercised — see Residual.

Residual

The ticket's second acceptance artifact — a real two-round run on a consumer PR — was not exercised, and cannot be from here. The ticket asks for three live observations after the unit suite: that round 2's consolidated review body carries the sentinel, that the run log shows Incremental diff since round 1 (…) with a non-empty block, and that a retargeted PR's next round logs the fail-closed line rather than a shrunken block. All three need a consumer repo to open a PR against and two rounds of a real panel run; this branch has neither, and the workflow only runs from a caller pinned to a merged SHA of this repo, so nothing here can trigger it. What is covered: TestRetargetedBase builds a real git repository with a real retarget and asserts both halves against the real incremental-diff.py — that the old formulation drops a lib.py hunk round 1 never showed, that check passes over that shrunken block (so no existing fail-safe would have caught it), and that pinning OLD to the recorded merge base keeps the hunk and still passes check. The workflow wiring itself is pinned by string assertions against cursor-review.yml, which is weaker than running it: the first consumer round after this merges is still the first time the shell actually executes. Worth watching that run's Diff size check log for Round merge base: and the fail-closed line, and its round-2 review body for the sentinel.

The rollout regression window is real and unbounded by this PR. Every PR with an open review round at merge time gets exactly one round with no incremental block, because the round it compares against predates the sentinel. That is the intended fail-closed behaviour and it is documented in both guides, but nothing measures how many PRs it touches or alerts if the block fails to come back on the round after — the only signal is the log line. A follow-up could surface "no recorded merge base" as a diff-size job output next to incremental_subset, the way the discard case already is, so the condition is visible to a consumer without reading step logs.

Not fixed, deliberately, and each named as a non-goal by the plan: no cross-run artifact lookup to recover a merge base for rounds written before the sentinel; no change to incremental-diff.py's check; nothing in scripts/check-pr-size. On the falsification question — this PR's user-facing outcome denies a capability ("no incremental block this round"), so the alternatives were checked rather than assumed: recomputing from the current base is the bug itself; the GitHub PR API exposes only the current base.sha, and the issue timeline's base_ref_changed event carries ref names, not the old SHAs; git merge-base --fork-point depends on a local reflog a fresh actions/checkout does not have. The prior round's merge base is genuinely unrecoverable after the fact without a written record, which is why the record exists. I did not run those API calls against a live retargeted PR — that is the same unexercised-artifact limit as above.

Unexercised artifacts I could not read: the spike this ticket was written from, and the review-thread discussion it was deferred from, are both internal-only links from my position; I worked from the plan as handed to me and did not confirm its measurements independently beyond reproducing the failure in TestRetargetedBase.

_{🔄 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/322 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/18/2026 **Status:** ✅ Merged **Merged:** 9/20/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-15598-round-sentinel-merge-base` --- ### 📝 Commits (6) - [`da6cbc8`](https://github.com/Comfy-Org/github-workflows/commit/da6cbc8c46eee15a26ba19f4d08465e84bc1fab5) fix(cursor-review): build the incremental hunks block as a subset of the reviewed diff - [`a2da829`](https://github.com/Comfy-Org/github-workflows/commit/a2da829779f99b2fdb0c67862a2737ad173004ac) fix(cursor-review): close seven gaps the panel found in the incremental block - [`74bbcff`](https://github.com/Comfy-Org/github-workflows/commit/74bbcff853483c935a3e477b98b8027812db3ee6) fix(cursor-review): record each round's merge base and pin the incremental block's OLD patch to it - [`65f04d9`](https://github.com/Comfy-Org/github-workflows/commit/65f04d933d4510c15c63dece3a71a072a896e4c5) fix(cursor-review): refuse a round sentinel from a round that reviewed nothing - [`c3a2721`](https://github.com/Comfy-Org/github-workflows/commit/c3a2721a44f3b43dee5d9c24a01ba732d3a7a840) Merge origin/main into matt/be-15598-round-sentinel-merge-base - [`cec0e33`](https://github.com/Comfy-Org/github-workflows/commit/cec0e332bc7c51448f737356fd7a3920da20e934) fix(cursor-review): trim the superseded three-dot prose, and stop re-collecting the end-to-end suite ### 📊 Changes **9 files changed** (+1088 additions, -35 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+3 -1) 📝 `.github/cursor-review/build-ledger.py` (+137 -0) 📝 `.github/cursor-review/incremental-diff.py` (+14 -6) 📝 `.github/cursor-review/post-review.py` (+109 -4) 📝 `.github/cursor-review/tests/test_build_ledger.py` (+242 -0) 📝 `.github/cursor-review/tests/test_incremental_diff.py` (+174 -2) 📝 `.github/cursor-review/tests/test_post_review.py` (+282 -4) 📝 `.github/workflows/cursor-review.yml` (+124 -17) 📝 `docs/callers/cursor-review.md` (+3 -1) </details> ### 📄 Description **No longer stacked — this targets `main`.** #321 merged on 2026-09-20 and its branch was deleted, so GitHub retargeted this PR onto `main`; the earlier "do not merge before #321" banner no longer applies. **#321 squash-landed this branch's first two commits**, so what remains here is the BE-15598 half: the round sentinel and the merge-base pin. `origin/main` has been merged in (`c3a2721`) and the conflicts — the overlap with #321 plus the prose and the OLD-patch command this branch supersedes — resolved in this branch's favour, since each is the BE-15598 rewrite of text #321 introduced. Main's independent work merged untouched: #328's `workflows_ref`-vs-`uses:` guard is intact, and #323's POST-failure handling composes with the sentinel (the sentinel rides `review_header`; `post_error_review` and the all-panel-cells-failed body keep the plain, sentinel-free `header`). ## ELI-5 The review panel gets a "here's what's new since last time" block so it knows where to spend its budget. Building that block needs to know what the *previous* round already showed the panel, and until now we worked that out by re-deriving it from the PR's base branch *today*. If the PR gets retargeted — or its base branch is rewritten — "today" is a different fork point than the one the last round used, so code the panel has genuinely never seen gets subtracted as "already reviewed" and silently disappears from the block. So each review now writes down, in an invisible HTML comment under its header, exactly which commit it diffed against; the next round reads that back and uses it. If there's nothing written down, we show no block at all rather than guessing — an unprioritized full diff is always correct, a quietly shrunken one is not. ## What changed **1. `diff-size` resolves and publishes this round's merge base.** A new shell step right after the `workflows_ref` guard runs `git merge-base "$BASE_SHA" "$HEAD_SHA"` and publishes it as the job's `merge_base_sha` output. Shell rather than a `check-pr-size` output on purpose: the tool is skipped entirely on the degraded raw-numstat fallback path, and a round that records no merge base costs the *next* round its whole block. It never fails the job — an unresolvable merge base is recorded as empty and warned about. **2. `post-review` writes the round sentinel.** `post-review.py` gains `--base-sha` / `--merge-base-sha` and `render_round_sentinel()`, which emits `` (sorted keys, tight separators, every field validated `^[0-9a-f]{40}$` or written as `""`). It goes on its own line directly under the header — above the notice and ledger-note banners — in the findings body, the "No high-signal findings" body, and the wholesale-422 fallback body, which derives from the same head. It is **not** emitted by the two bodies whose round reviewed nothing: `post_error_review`, and the "Panel did not produce any findings" body posted when every panel cell errored. `defang_body_only_contract` now breaks this prefix too, alongside the other two. The job takes `diff-size` into `needs:` for that one output. It was already a transitive dependency (`consolidate → review → diff-size`), so this adds no ordering. **3. `build-ledger.py` reads it back.** `_ROUND_SENTINEL_RE` is pinned to the single-spaced opener and anchored to a line start, `_parse_round_sentinel` is tolerant (`None` on missing / unknown version / bad JSON / wrong shape, never raises). It is read from `consolidated[-1]` only, refused outright if that body is the error-review shape (`_ERROR_REVIEW_RE`, the same refusal `_body_only_entries` applies and for the same reason), and otherwise accepted only when `payload["head"] == review["commit_id"]` (with a non-empty `commit_id`, so `"" == ""` is not a way in) and `merge_base` is full lowercase hex. The result reaches every ledger shape (`ok` / `empty` / `unknown` / `disabled`), `_write_outputs`, and the `ledger` job's new `last_reviewed_merge_base` output. **4. The `incremental` step pins OLD and fails closed.** `build_old_patch` is now `git -c core.quotePath=false diff "${LAST_REVIEWED_MERGE_BASE}" "${LAST_REVIEWED_SHA}"` — two-dot, because the recorded merge base *is* the exact left tree round N diffed, and `check-pr-size`'s `writeReviewedDiff` builds NEW as `mergeBase...head`, which resolves to the same two-tree diff (`scripts/check-pr-size/main.go:166-175`). Two new `elif` branches sit between the last-reviewed-SHA reachability check and the empty-FULL_PATCH check, each with a distinct sweepable log line, each leaving the block empty with `incremental_subset=true`: no recorded merge base, and a recorded merge base that is unreachable or no longer an ancestor of the last-reviewed commit. `BASE_SHA` is gone from the step's `env:` — nothing else in it used it — so the fallback that caused this cannot be reached by accident. **5. Docs.** The ledger sections of `.github/cursor-review/README.md` and `docs/callers/cursor-review.md` each get a paragraph on the sentinel, the pinned OLD, and the one block-less round every open PR sees after rollout. ## Round 2 — the panel's findings The six-model panel raised nine. Four became code, two became docs, two became comments, one was answered on the thread. - 🟡 **The round sentinel was readable out of an ERROR review.** `_body_only_entries` already refuses that shape; `_parse_round_sentinel`'s caller did not, even though `post_error_review` is the one consolidated body that renders imported judge/CLI text *inside a fence*, at column 0 — where the sentinel's line anchor, its whole containment argument, buys nothing. The writer-side defang this PR added covers only bodies written by *this* version, while consumers stay pinned to older SHAs and every error review already sitting on their PRs is undefanged. The `head == commit_id` gate does not close it either: the reviewed head SHA is public to the PR author. Fixed at the reader, which is the half a slow fleet cannot outrun. A comment in post-review.py was already *asserting* this refusal existed. - 🟢 **The all-panel-cells-failed body recorded a merge base for a panel that never ran.** It renders `review_header` while being posted with `delivers=False`, which contradicted the stated rationale for withholding the sentinel from `post_error_review` — and unlike the error review it carries no "Review failed" heading, so the reader-side refusal above does not reach it. It now takes the plain `header`. The next round therefore fails closed to "no block" instead of subtracting hunks nobody looked at. - 🟢 **`^[0-9a-f]{40}$` accepted a trailing newline.** Python's `$` matches before a final newline, so a 41-character value passed the gate whose adjacent comment calls it the single control keeping newlines out of `$GITHUB_OUTPUT`. Both the reader's `_FULL_SHA_RE` and the writer's `_ROUND_SHA_RE` now end `\Z`. - 🟢 **Neither forgery test could fail for the reason it claimed.** `search` returns the first match and the genuine sentinel sits on line 2, ahead of the blockquoted forgery, so both passed with the line anchor removed entirely — nothing pinned the column-0 containment, which is the regression the module comments record happening once already. Added a case carrying *only* a quoted sentinel, across every indent a rendered finding can sit behind (`>`, `> > `, list items, tabs, four-space). - 🟢 **Both docs asserted the superseded formulation immediately above the paragraph reversing it**, so a reader going top-down got the pre-BE-15598 behaviour first. Folded in — and both now also say which rounds record no sentinel at all. Answered rather than changed, because neither is reachable and a guard that cannot fire is dead code: - ⚪ **`MERGE_BASE` has no line-break guard**, unlike `resolveMergeBase` in the Go tool. `git merge-base` on two commits with no `--all` prints exactly one line and `$( )` strips the trailing newline, so there is no value this step can emit that the Go check would refuse. Noted in the step, with why the two deliberately diverge. - ⚪ **`LAST_REVIEWED_SHA` is a positional `git diff` argument and is never shape-validated.** True, and the reviewer's own note is the answer: the preceding `git cat-file -e "${LAST_REVIEWED_SHA}^{commit}"` already rejects anything option-shaped or unresolvable, and the value's provenance is GitHub's `commit_id`, not the PR. Left alone. Deferred, with the reasoning recorded where the step explains itself rather than in a ticket: - 🟢 **On the retarget path, the block comes out close to the full diff and the prompt carries it twice.** Real, and `check` cannot catch it (`foreign == 0`, `new_lines <= full_lines` — the block *is* a faithful subset, just a useless one). The cost is prompt budget on an uncommon path, never coverage. Declining a block whose length approaches `full_lines` needs a threshold nothing here has grounds to pick, so it is named as a known waste in the step's comments rather than guessed at. ## Judgment calls - **`base` is validated as a full hex SHA too, which the plan left as a bare `payload.get("base","")`.** `_write_outputs` appends it to `$GITHUB_OUTPUT`, where a value carrying a newline is an output-injection vector, and the accept path is reachable from any Bot-authored consolidated review. The writer only ever emits hex-or-empty, so the check costs nothing real and keeps one control over the whole payload. `test_a_non_hex_base_is_dropped_rather_than_written_through` pins it. - **The ancestor check on the recorded merge base** is slightly beyond the plan's "unreachable" wording. A rewritten PR branch can leave the recorded commit reachable but no longer in the last-reviewed commit's history, in which case the diff *succeeds* and returns something that is not what round N saw. - **The wholesale-422 fallback body carries the sentinel too.** It falls out of sharing `review_head`, and it is a real review of that commit, so the next round should be able to read it. - **The merge-base step sits after the `workflows_ref` guard rather than immediately after the checkout**, to keep that guard's fail-fast position. It is still before every other step in the job, so both the classifier and the degraded fallback path are covered. - **`--base-sha` / `--merge-base-sha` are passed to all three call sites including the error-review one**, per the plan; `post_error_review` ignores them. ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'`: **720 tests, 0 failures**, re-run on the merged tree. (Was 705 when this PR was opened, then 725 after merging main, then 720 once the duplicate collection CodeRabbit flagged was removed — the drop is exactly the five re-collected cases, no case lost.) Each of the four new tests was additionally confirmed to FAIL against the un-fixed code, by reverting all four fixes in a scratch copy and re-running — a test that passes either way pins nothing, which is precisely the defect finding 4 above reports. `python3 .github/workflow-pins/check_workflow_pins.py`: OK, 11 workflows, 156 `uses:` pinned; `python3 -m unittest discover -s .github/workflow-pins/tests`: 502 tests OK. `bash .github/lint/check-org-repo-literals.sh`: OK. `python3 .github/agents-md-integrity/check_agents_md.py --root .`: passed (2 pre-existing warnings, unrelated). `shellcheck -x .github/cursor-review/install-cursor-cli.sh .github/cursor-review/slack-notify.sh`: clean, plus `shellcheck -S warning` over the two `run:` blocks this PR edits, extracted from the YAML: clean. `python3 -c yaml.safe_load` over `cursor-review.yml` to confirm the new outputs and `needs:` parse. - **Deviations:** `origin/main` merged in and its conflicts resolved as described at the top, plus two CodeRabbit findings fixed in `cec0e33` (the superseded three-dot prose in `incremental-diff.py` and `cursor-review.yml`; `RoundSentinelTest` re-collecting `EndToEndPostTest`'s five cases, fixed by moving the shared harness to a `PostHarness` mixin). Earlier: the `base`-SHA validation and the ancestor check above; the two panel nits answered in comments rather than code, and the retarget doubled-prompt finding deferred to a code comment rather than a ticket (all three are listed with their reasoning under *Round 2*); the live two-round consumer run named under Verification is still not exercised — see Residual. ## Residual **The ticket's second acceptance artifact — a real two-round run on a consumer PR — was not exercised, and cannot be from here.** The ticket asks for three live observations after the unit suite: that round 2's consolidated review body carries the sentinel, that the run log shows `Incremental diff since round 1 (…)` with a non-empty block, and that a retargeted PR's next round logs the fail-closed line rather than a shrunken block. All three need a consumer repo to open a PR against and two rounds of a real panel run; this branch has neither, and the workflow only runs from a caller pinned to a merged SHA of this repo, so nothing here can trigger it. What *is* covered: `TestRetargetedBase` builds a real git repository with a real retarget and asserts both halves against the real `incremental-diff.py` — that the old formulation drops a `lib.py` hunk round 1 never showed, that `check` passes over that shrunken block (so no existing fail-safe would have caught it), and that pinning OLD to the recorded merge base keeps the hunk and still passes `check`. The workflow wiring itself is pinned by string assertions against `cursor-review.yml`, which is weaker than running it: **the first consumer round after this merges is still the first time the shell actually executes.** Worth watching that run's *Diff size check* log for `Round merge base:` and the fail-closed line, and its round-2 review body for the sentinel. **The rollout regression window is real and unbounded by this PR.** Every PR with an open review round at merge time gets exactly one round with no incremental block, because the round it compares against predates the sentinel. That is the intended fail-closed behaviour and it is documented in both guides, but nothing measures how many PRs it touches or alerts if the block fails to come back on the round after — the only signal is the log line. A follow-up could surface "no recorded merge base" as a `diff-size` job output next to `incremental_subset`, the way the discard case already is, so the condition is visible to a consumer without reading step logs. **Not fixed, deliberately, and each named as a non-goal by the plan:** no cross-run artifact lookup to recover a merge base for rounds written before the sentinel; no change to `incremental-diff.py`'s `check`; nothing in `scripts/check-pr-size`. On the falsification question — this PR's user-facing outcome *denies* a capability ("no incremental block this round"), so the alternatives were checked rather than assumed: recomputing from the current base is the bug itself; the GitHub PR API exposes only the *current* `base.sha`, and the issue timeline's `base_ref_changed` event carries ref names, not the old SHAs; `git merge-base --fork-point` depends on a local reflog a fresh `actions/checkout` does not have. The prior round's merge base is genuinely unrecoverable after the fact without a written record, which is why the record exists. I did not run those API calls against a live retargeted PR — that is the same unexercised-artifact limit as above. **Unexercised artifacts I could not read:** the spike this ticket was written from, and the review-thread discussion it was deferred from, are both internal-only links from my position; I worked from the plan as handed to me and did not confirm its measurements independently beyond reproducing the failure in `TestRetargetedBase`. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:38:28 +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#322

No description provided.

Rows
Columns