fix(cursor-review): record each round's merge base and pin the incremental block's OLD patch to it by mattmillerai · Pull Request #322 · Comfy-Org/github-workflows

mattmillerai · 2026-09-18T21:37:23Z

STACKED — merging lands on be-15558-incremental-diff-subset (owned by the author of #321), NOT main. Do not merge this before #321.

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': 705 tests, 0 failures (48 new). 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, 155 uses: pinned. 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: 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.

…ental block's OLD patch to it The incremental "hunks new since the last reviewed round" block built its OLD side as `git diff BASE...LAST_REVIEWED`, which is right only while BASE still resolves to the merge base that round actually used. Retarget the PR, or rewrite its base branch, and it does not: the three-dot form silently re-resolves to a different merge base, everything the branch inherited from the old one lands on both sides, and hunks the panel has never seen are subtracted as "already reviewed". Nothing downstream catches it — the block is merely smaller, and a smaller block still passes the subset fail-safe. Every consolidated review now carries a round sentinel directly under its header recording the commit it reviewed and the merge base it was diffed against. The next round's ledger reads the merge base back out of the LAST round's sentinel — accepting it only when its `head` is that review's own `commit_id` and its SHAs are full lowercase hex — and the incremental step builds OLD as a two-dot diff against exactly that commit. With no usable recorded merge base it emits no block at all and logs which case it hit, rather than falling back to the current base, which is the bug. Every live PR sees one block-less round after this ships.

coderabbitai · 2026-09-18T21:37:29Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Team

Run ID: cc73f9ee-7351-4b23-97d9-11727b33325b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

_{Comment @coderabbitai help to get the list of available commands.}

github-actions

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 9 finding(s).

Severity	Count
🟡 Medium	1
🟢 Low	6
⚪ Nit	2

Panel: 5/6 reviewers contributed findings.

Reviewers that did not contribute: gpt-5.6-sol-max:adversarial (error)

…d nothing Panel review of BE-15598. Four fixes, one documentation pass. 1. `build_ledger` now refuses the ERROR-REVIEW shape before parsing the round sentinel, exactly as `_body_only_entries` already does and for the identical reason: `post_error_review` renders imported judge/CLI text inside a FENCE, so it is the one consolidated body whose foreign lines sit at column 0 and can satisfy the line anchor the sentinel's containment argument rests on. The writer-side defang added alongside it only reaches bodies THIS version wrote, while consumers stay pinned to older SHAs and every error review they have already posted sits on their PRs undefanged. The `head == commit_id` gate does not close that window either — the reviewed head SHA is public to the PR author — and the workflow's `cat-file -e` / `merge-base --is-ancestor` checks prove ancestry, not that a commit was the merge base round N used. A comment in post-review.py already asserted this reader-side refusal existed; now it does. 2. The all-panel-cells-failed body takes the PLAIN header, so it writes no sentinel. It carries no "Review failed" heading, so (1) does not reach it, and it is posted with `delivers=False` precisely because nothing was reviewed — yet it was recording a merge base the next round would have subtracted hunks against. Same rule as the error review, enforced at the writer because that is the only place it can be. 3. `_FULL_SHA_RE` / `_ROUND_SHA_RE` terminate with `\Z`, not `$`. Python's `$` also matches just before a final newline, so `^[0-9a-f]{40}$` accepted a 41-character value ending in one — and that gate is the single control keeping a line break out of the `key=value` `_write_outputs` appends to $GITHUB_OUTPUT. The invariant the surrounding code relies on is now the one the regex provides. 4. A test that actually pins the column-0 containment. The two existing forgery tests could not fail for the reason they claim: `search` returns the FIRST match and a genuine sentinel sits ahead of the forgery, so both pass with the line anchor removed entirely — the same regression the module comments record happening once already on the body-only sentinel. The new case carries no real sentinel at all, across every indent a rendered finding can sit behind. Docs: both ledger sections stated the superseded `git diff BASE...LAST_REVIEWED` formulation in a paragraph directly above the one that reverses it, so a reader going top-down got the pre-BE-15598 behaviour first. Folded into the new formulation, and both now say which rounds record no sentinel at all. Two panel nits are answered in comments rather than code, since neither is reachable: the `diff-size` merge-base step needs no line-break guard (`git merge-base` on two commits prints one line and `$( )` strips the newline), and the doubled-prompt shape on the retarget path is a known prompt-budget waste whose fix needs a threshold nothing here has grounds to pick. Verified: 705 tests, 0 failures; each new test confirmed to fail against the un-fixed code. check_workflow_pins.py OK; check_agents_md.py passed; shellcheck -S warning clean over both edited run blocks. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

mattmillerai marked this pull request as ready for review September 18, 2026 21:37

mattmillerai added agent-coded Authored by the agent-work loop cursor-review Multi-model cursor review labels Sep 18, 2026

cloud-code-bot Bot assigned huntcsg Sep 18, 2026

github-actions Bot reviewed Sep 18, 2026

View reviewed changes

mattmillerai mentioned this pull request Sep 19, 2026

feat(cursor-review): fail the review when workflows_ref differs from the uses: pin #328

Open

Navigation Menu

fix(cursor-review): record each round's merge base and pin the incremental block's OLD patch to it - #322

fix(cursor-review): record each round's merge base and pin the incremental block's OLD patch to it#322
mattmillerai wants to merge 2 commits into
be-15558-incremental-diff-subsetfrom
matt/be-15598-round-sentinel-merge-base

mattmillerai commented Sep 18, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Sep 18, 2026 •

edited

Loading

Review skipped

Uh oh!

github-actions Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mattmillerai commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ELI-5

What changed

Round 2 — the panel's findings

Judgment calls

Provenance

Residual

Uh oh!

coderabbitai Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

🔍 Cursor Review — Consolidated panel

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mattmillerai commented Sep 18, 2026 •

edited

Loading

coderabbitai Bot commented Sep 18, 2026 •

edited

Loading