From 7c55d05e643f41ae4b4aa9c7415676863a9059db Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sun, 1 Nov 2026 12:07:44 +0000 Subject: [PATCH 1/2] feat(cursor-review): carry an approval across base merges that only move a regenerated artifact carry_forward_unchanged_diff dismisses when the PR's own diff differs, which is right, but a PR rewriting a checked-in generated file the base also regenerates has other lines in it after every merge from the base. Each merge then costs a whole round on hand-written code that did not change. Add the opt-in carry_forward_regenerated_globs, empty by default. When the own diff differs it still carries only if the set of (name, previous, status) is the same, every differing file matches a glob, and every commit gained since the reviewed one is a merge with a parent on the live base. The carried approval, the log and the dismissal say so, and the approval names the files. Commits are read from the live base...head compare, not reviewed...head: the latter also lists every base commit a merge brought in, ordinary ones included, so requiring merges there would dismiss every real merge. A parent missing from the former is on the base, without a read per commit. A list the API cut short fails closed. A path outside a plain alphabet never matches a glob: its name lands in a body parsed back for its records, where a newline could forge a threshold line. --- .github/cursor-review/README.md | 1 + .github/cursor-review/auto-approve.py | 130 ++++++++- .../cursor-review/tests/test_auto_approve.py | 270 +++++++++++++++++- .github/workflows/cursor-review.yml | 23 +- docs/callers/cursor-review.md | 33 +++ 5 files changed, 440 insertions(+), 17 deletions(-) diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index cf9c3715..3d44317c 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -434,6 +434,7 @@ All optional except `workflows_ref` (required, no default) — pass them under | `approve_only_on_push` | `false` | On a push, the `approve-only` job (`auto-approve.py approve-only`, stages `check` and `start` either side of the `auto_rereview_delay_minutes` wait) decides whether the axes alone re-check the head: the card must carry `LANE_MARKER` (written only past a passing panel) and have no edit by any other login in its GraphQL `userContentEdits` (`card_foreign_editors`: write access can edit the card, and a later rewrite would carry a planted marker), its round must be numbered and below `max_rounds`, and the live PR must still qualify. `go` sets the `approve_gate` output to `pass` with no panel and stands `auto-rereview` down; without `go` (not eligible, or `start` stood down after the wait) `auto-rereview` still runs. The head is recorded on the card's re-check sentinel (`card.py`). With `auto_rereview_after_dismissal` too, `mark_card_stale` → `schedule_panel_rereview` handles the other state: a card carrying `PANEL_BLOCKED_MARKER` (only `render_round` writes it, for a `changes_requested` round) gets the stale-head marker and `auto_rereview`, and `cmd_auto_rereview` dates the in-flight check from the approver's last decision. | | `approve_only_max_runs` | `10` | Re-checks per panel round, read off the re-check sentinel, which names the round it counts for; clamped to `card.RECHECK_HEADS_KEPT` (50), past which the count could not grow. | | `carry_forward_unchanged_diff` | `false` | Opt-in: `dismiss-stale-approval` re-approves the live head before dismissing when the PR's own changes (compare API, merge base → head, hunk line numbers ignored, headings and inter-hunk distances kept) equal the ones the approving round reviewed under a threshold no looser than today's — the stacked-PR rebase, re-merge or retarget. What the round reviewed comes only from the approver's own records: the merge base the approval recorded (`approval_merge_base`, when the compare API and the round's sentinel agree — the sentinel can veto a record, never supply one) or what a carried approval copied forward (`carry_baseline`), and the compare API must show it as an ancestor of the reviewed commit; an approval with neither never carries. Only under `pull_request_target`, only for the approver's latest verdict, failing closed (dismissing as before) on anything it cannot compare (`try_carry_forward`); `mark_card_stale` leaves the card alone while such an approval stands. See [the setup guide](../../docs/callers/cursor-review.md#auto-approve). | +| `carry_forward_regenerated_globs` | `''` | With `carry_forward_unchanged_diff`, `regenerated_only` also carries when the set of (name, previous name, status) is unchanged, every file whose patch differs matches a glob (`matches_regenerated_glob`), and every commit the live compare lists beyond what the reviewed commit reaches is a merge with a parent outside that list, i.e. on the base (`only_merges_from_base`; a cut-short list, a retarget or a reviewed commit the head no longer contains fails closed). The carried body and the log name the files. Their content is not re-reviewed, so a glob must only name files a required CI check regenerates and verifies from code. | | `max_rounds` | `5` | Per-PR review-round cap; `0` → none. Counts the posting identity's consolidated reviews since `needs-human-review` was last removed, except a round whose own decision found the PR head had moved (a push superseded it), at most once per reviewed commit and at most `max_rounds` times; at or over the cap the panel is skipped, the PR is labelled `needs-human-review`, one comment lists the latest round's open findings above the threshold, and the `approve_gate` output is `capped`. Fails open on a read error. See [the setup guide](../../docs/callers/cursor-review.md#round-cap-and-the-approve_gate-output). | | `runs_on` | `'"ubuntu-latest"'` | JSON-encoded `runs-on` (same shape as `cursor-review-auto-label.yml`'s) for the four credential-free heavy jobs only: `diff-size`, `preflight`, the `review` panel and `consolidate`. The gate, ledger, Slack notices and every job holding the App token or `pull-requests: write` stay on GitHub-hosted `ubuntu-latest`. Those four run models with unsandboxed shell over PR code, so use a self-hosted pool only if each job gets a fresh pod (fresh filesystem and `$HOME`), holds no cloud identity or token, and can't reach private networks or the metadata server. The image must be linux/x64 with bash, git, curl, jq, python3, gh, tar and GNU coreutils. | | `approver_environment` | `''` | The caller-repo environment holding `APPROVER_TOKEN` (an environment secret, base-branch-only deployment policy), declared on `post-review`, `dismiss-stale-approval`, `auto-retry`, `auto-rereview`, `approve-only` and `axes-closeout` only (and passed to cursor-approve.yml from `approve-start-early`, which declares it there), and only under `pull_request_target`. Pair with a `pull_request_target` caller — see the [trust model](../../docs/callers/cursor-review.md#trust-model). | diff --git a/.github/cursor-review/auto-approve.py b/.github/cursor-review/auto-approve.py index a3ea3c76..8104acd4 100644 --- a/.github/cursor-review/auto-approve.py +++ b/.github/cursor-review/auto-approve.py @@ -53,7 +53,9 @@ lower stacked PR landed — the approval is re-posted on the live head, and only then is the stale one dismissed. Only under ``--event-name pull_request_target``; the carry-forward section below lists every - condition. + condition. ``--carry-forward-regenerated-globs`` (the workflow's + `carry_forward_regenerated_globs`, empty by default) lets a push of nothing + but merges from the base carry when only files those globs name differ. ``round-cap`` (in `round-cap`, before the panel starts) Count the consolidated reviews the posting identity has already left on @@ -2664,6 +2666,16 @@ def annotation_cause(error, fallback: str) -> str: # landed. A POST that failed ambiguously (a timeout, no review id) re-lists the # reviews and withdraws any carried approval of the live head it may have # landed anyway. A withdrawal that fails turns the job red. +# +# `--carry-forward-regenerated-globs` admits one difference: a checked-in +# generated file the PR rewrites shifts whenever the base regenerates it, so +# its patch differs though nothing hand-written did. That carries only when, on +# top of everything above, the set of (name, previous name, status) is the same, +# every file whose patch differs matches a glob, and everything the PR gained +# since the reviewed commit is a merge from the base (only_merges_from_base). +# Those files' content is not re-reviewed, so a glob may only name files a +# required CI check regenerates and verifies from code; this module cannot see +# whether one does. COMPARE_FILE_CAP = 300 COMPARE_HUNK_RE = re.compile(r"^@@ -(\d+)(?:,(\d+))? \+\d+(?:,\d+)? @@(.*)$") @@ -2674,10 +2686,16 @@ def annotation_cause(error, fallback: str) -> str: APPROVED_THRESHOLD_RE = re.compile(r"^_Threshold: `(" + "|".join(ALLOWED_THRESHOLDS) + r")` ", re.M) CARRIED_MESSAGE = ("Carried forward to `{head}`: this PR's own changes are identical to the ones reviewed, so the " "approval moved to the live head.") +CARRIED_REGENERATED_MESSAGE = ("Carried forward to `{head}`: this PR's own changes are identical to the ones reviewed " + "apart from regenerated artifacts, so the approval moved to the live head.") +# A path a regenerated glob may match. Its name lands in a signed review body, +# parsed back for its records, and in the log, so a name that could break a line +# or a code span (git allows both) is simply never a regenerated artifact. +REGENERATED_PATH_RE = re.compile(r"[A-Za-z0-9._/@+-]+") -def carried_message(head: str) -> str: - return CARRIED_MESSAGE.format(head=(head or "?")[:7]) +def carried_message(head: str, regenerated=()) -> str: + return (CARRIED_REGENERATED_MESSAGE if regenerated else CARRIED_MESSAGE).format(head=(head or "?")[:7]) def leading_markers(body) -> str: @@ -2771,6 +2789,75 @@ def normalize_patch(patch: str) -> str: return "\n".join(lines) +def glob_regex(glob: str) -> str: + """`glob` as a regex over a whole repo-relative path: `*` and `?` stop at a + `/`, `**/` spans directories, `**` spans anything, the rest is literal.""" + out, i = [], 0 + while i < len(glob): + if glob.startswith("**/", i): + out.append("(?:.*/)?") + i += 3 + elif glob.startswith("**", i): + out.append(".*") + i += 2 + else: + out.append({"*": "[^/]*", "?": "[^/]"}.get(glob[i], re.escape(glob[i]))) + i += 1 + return "".join(out) + + +def matches_regenerated_glob(path: str, globs: list) -> bool: + return bool(REGENERATED_PATH_RE.fullmatch(path)) and any(re.fullmatch(glob_regex(g), path) for g in globs) + + +def only_merges_from_base(live_read, reviewed: str) -> bool: + """Whether everything the PR gained since `reviewed` is a merge from its base. + + `live_read` is the compare of the live base with the live head, whose commits + are what the head has and the base does not: the merges, but not the commits + they brought in. So a parent missing from that list is on the base, and what + `reviewed` already held is what its parents reach inside the list. The rest + must each be a merge with a parent on the base, and there must be some: a + retarget added nothing, and `reviewed` missing from the list means the head + no longer contains it. A list the API cut short proves none of this.""" + commits = live_read.get("commits") if isinstance(live_read, dict) else None + if not isinstance(commits, list) or live_read.get("total_commits") != len(commits): + return False + parents = {} + for c in commits: + listed = c.get("parents") if isinstance(c, dict) else None + if not isinstance(listed, list) or not all(isinstance(p, dict) for p in listed): + return False + parents[c.get("sha")] = [p.get("sha") for p in listed] + if reviewed not in parents: + return False + held, walk = set(), [reviewed] + while walk: + sha = walk.pop() + if sha in parents and sha not in held: + held.add(sha) + walk.extend(parents[sha]) + added = [sha for sha in parents if sha not in held] + return bool(added) and all(len(parents[sha]) >= 2 and any(p not in parents for p in parents[sha]) + for sha in added) + + +def regenerated_only(before: list, after: list, globs: list, live_read, reviewed: str): + """(the files that differ, "") when `before` and `after` differ only in files + `globs` name, on a push of nothing but merges from the base; else ([], why).""" + if not globs: + return [], "" + if [change[:3] for change in before] != [change[:3] for change in after]: + return [], "the set of changed files is different" + differ = [a[0] for a, b in zip(before, after) if a != b] + others = [name for name in differ if not matches_regenerated_glob(name, globs)] + if others: + return [], f"{len(others)} changed file(s) match no `carry_forward_regenerated_globs` entry" + if not only_merges_from_base(live_read, reviewed): + return [], f"the commits since `{reviewed[:7]}` are not provably all merges from the base" + return differ, "" + + def round_merge_base(reviews: list, poster_login: str, head: str) -> str: """The merge base the latest round on `head` says it diffed against, or "". @@ -2936,10 +3023,11 @@ def carry_card_url(args) -> str: def render_carried_body(card_url: str, head: str, base_ref: str, reviewed: str, merge_base: str, from_sha: str, - threshold: str) -> str: + threshold: str, regenerated=()) -> str: """A marked approval of `head` carried forward from `from_sha`, whose own changes a round reviewed at `reviewed` (diffed from `merge_base`) and - approved under `threshold` — the original's, never loosened by a chain.""" + approved under `threshold` — the original's, never loosened by a chain. + `regenerated` names the files allowed to differ, which no round re-read.""" url = safe_card_url(card_url) lines = [ APPROVE_MARKER, @@ -2950,12 +3038,20 @@ def render_carried_body(card_url: str, head: str, base_ref: str, reviewed: str, f"Carried forward from `{from_sha[:7]}`: only what this PR is based on changed; its own changes are " f"identical to the ones reviewed at `{reviewed[:7]}`.", ] + if regenerated: + lines[-1] = (f"Carried forward from `{from_sha[:7]}`: only merges from the base landed; this PR's own changes " + f"are identical to the ones reviewed at `{reviewed[:7]}`, apart from the artifacts below.") + lines.append(f"Only regenerated artifacts differ: {regenerated_names(regenerated)}.") # Guarded as in render_body: this body is parsed back for its markers. if threshold in ALLOWED_THRESHOLDS: lines.append(f"\n_Threshold: `{threshold}` (set by this repo's `approve_max_severity`)._") return "\n".join(lines) +def regenerated_names(regenerated) -> str: + return ", ".join(f"`{name}`" for name in regenerated) + + def try_carry_forward(args, ctx: dict, approval): """Re-post `approval` on the live head when the PR's own changes are still the ones a round reviewed. (the new review, None) on success; (None, None) when @@ -2990,17 +3086,22 @@ def try_carry_forward(args, ctx: dict, approval): f"`{merge_base[:7]}`, is not an ancestor of the reviewed commit `{reviewed[:7]}`.") return None, None before = own_changes(reviewed_read) - after = own_changes(compare(args.repo, ctx["base_sha"], ctx["head"])) + live_read = compare(args.repo, ctx["base_sha"], ctx["head"]) + after = own_changes(live_read) if before is None or after is None: emit(f"Auto-approve: not carrying the approval of `{from_sha[:7]}` forward — the compare API could not " "vouch for every changed file (300+ files, a binary or oversized patch, or a read error).") return None, None + regenerated = [] if before != after: - emit(f"Auto-approve: not carrying the approval of `{from_sha[:7]}` forward — the PR's own changes differ " - f"from the ones reviewed at `{reviewed[:7]}`.") - return None, None + globs = (getattr(args, "carry_forward_regenerated_globs", "") or "").split() + regenerated, why = regenerated_only(before, after, globs, live_read, reviewed) + if not regenerated: + emit(f"Auto-approve: not carrying the approval of `{from_sha[:7]}` forward — the PR's own changes differ " + f"from the ones reviewed at `{reviewed[:7]}`{f': {why}' if why else ''}.") + return None, None new_body = render_carried_body(carry_card_url(args), ctx["head"], ctx["base"], reviewed, merge_base, from_sha, - granted.group(1)) + granted.group(1), regenerated) try: posted = json.loads(gh( ["api", "-X", "POST", f"repos/{args.repo}/pulls/{args.pr_number}/reviews", "--input", "-"], @@ -3051,9 +3152,10 @@ def try_carry_forward(args, ctx: dict, approval): emit("Auto-approve: the PR changed while the carried-forward approval was being posted — withdrawn again.") return None, None emit(f"Auto-approve: carried the approval of `{from_sha[:7]}` forward to `{ctx['head'][:7]}` — the PR's own " - f"changes are identical to the ones reviewed at `{reviewed[:7]}`.") + f"changes are identical to the ones reviewed at `{reviewed[:7]}`" + + (f"; only regenerated artifacts differ: {regenerated_names(regenerated)}." if regenerated else ".")) return {"id": posted["id"], "user": {"login": login}, "state": "APPROVED", "body": new_body, "edited": False, - "commit_id": ctx["head"]}, None + "commit_id": ctx["head"], "regenerated": regenerated}, None def withdraw_unconfirmed_carries(args, head: str, stale_id): @@ -3263,7 +3365,8 @@ def cmd_dismiss_stale(args) -> int: handed_off) for rid in ids: try: - dismiss(args.repo, args.pr_number, rid, carried_message(live_head) if rid in carried else message) + dismiss(args.repo, args.pr_number, rid, (carried_message(live_head, carried[rid].get("regenerated")) + if rid in carried else message)) except RuntimeError as e: # Collapsed per id: these are joined into an ::error:: annotation, and # each carries `gh`'s multi-line stderr. @@ -4755,6 +4858,7 @@ def main() -> int: # BE-20585: re-approve instead of dismissing when the push left the PR's # own changes identical; posts only when --event-name is pull_request_target. s.add_argument("--carry-forward", default="false") + s.add_argument("--carry-forward-regenerated-globs", default="") s.add_argument("--event-name", default="") # `approve_only_on_push`: only the stale card's next-step sentence. s.add_argument("--approve-only", default="false") diff --git a/.github/cursor-review/tests/test_auto_approve.py b/.github/cursor-review/tests/test_auto_approve.py index 20278bb2..c78f85c5 100644 --- a/.github/cursor-review/tests/test_auto_approve.py +++ b/.github/cursor-review/tests/test_auto_approve.py @@ -3980,7 +3980,7 @@ def test_the_default_label_matches_card_py(self): POST_REVIEW = AA._load_post_review() -class CarryForwardTest(unittest.TestCase): +class CarryForwardFixture(unittest.TestCase): """dismiss-stale's carry-forward (BE-20585). On a stack, a change to a lower PR re-merges or rebases every PR above it, @@ -4121,13 +4121,13 @@ def gh(self, args, payload=None): raise AssertionError(f"unexpected gh call {args}") def run_dismiss(self, carry="true", event="pull_request_target", threshold="low", authors="", - all_approvals=False, login=None, auto="false", env=None): + all_approvals=False, login=None, auto="false", env=None, globs=""): args = argparse.Namespace( repo="o/r", pr_number="1", head_sha=self.pr["head"]["sha"], approver_login=login or self.LOGIN, all_approvals=all_approvals, threshold=threshold, approve_authors=authors, max_rounds="5", poster_login=self.POSTER, review_label="cursor-review", auto_rereview=auto, auto_rereview_delay_minutes="10", can_relabel="true", run_without_label="false", - carry_forward=carry, event_name=event) + carry_forward=carry, event_name=event, carry_forward_regenerated_globs=globs) environ = dict({"GITHUB_OUTPUT": self.output_path}, **(env or {})) with mock.patch.object(AA, "gh", self.gh), mock.patch.object(AA, "emit", self.emitted.append), \ mock.patch.dict(os.environ, environ), \ @@ -4154,6 +4154,8 @@ def assert_dismissed_as_before(self, rc, message=AA.STALE_MESSAGE): self.assertEqual(self.posted, []) self.assertEqual(self.dismissed, {11: message}) + +class CarryForwardTest(CarryForwardFixture): # -- the carry --------------------------------------------------------- def test_an_unchanged_own_diff_moves_the_approval_to_the_live_head(self): @@ -4610,6 +4612,268 @@ def test_a_carried_approval_this_run_withdrew_no_longer_keeps_the_card(self): self.assertEqual(len(self.patches), 1) +class CarryForwardRegeneratedTest(CarryForwardFixture): + """`--carry-forward-regenerated-globs`, the opt-in extension of the carry. + + A PR that rewrites a checked-in generated file the base also regenerates has + other lines in it after every merge from the base. Only that exact push may + carry, and only when nothing but the named files differ. The PR, unless a case + says otherwise: REVIEWED changed svc/a.go and gen/quote.json, and LIVE merged + BASE_SHA into it. As GitHub answers, the live compare lists what the head has + and the base does not: the merge and REVIEWED, never the commits it brought in. + """ + + ARTIFACT, OTHER = "gen/quote.json", "9" * 40 + GLOBS = "gen/*.json" + + def setUp(self): + super().setUp() + self.compares = {(self.MERGE_BASE, self.REVIEWED): self.two_files(10, 20), + (self.BASE_SHA, self.LIVE): self.two_files(42, 50, artifact_added="+regenerated", + commits=self.merged())} + + def commit(self, sha, *parents): + return {"sha": sha, "parents": [{"sha": p} for p in parents]} + + def merged(self): + return [self.commit(self.REVIEWED, self.MERGE_BASE), self.commit(self.LIVE, self.REVIEWED, self.BASE_SHA)] + + def two_files(self, start, artifact_start, artifact_added="+new", added="+new", commits=()): + data = self.files(start, patch=self.patch(start, added=added), cmp_status="diverged", behind_by=3) + data["files"].append({"filename": self.ARTIFACT, "status": "modified", "changes": 2, + "patch": self.patch(artifact_start, added=artifact_added)}) + data["commits"], data["total_commits"] = list(commits), len(commits) + data["status"], data["behind_by"] = ("ahead", 0) if not commits else ("diverged", 3) + return data + + @property + def live(self): + return self.compares[(self.BASE_SHA, self.LIVE)] + + def assert_carried_naming_the_artifact(self, rc): + self.assertEqual(rc, 0, self.printed) + body = self.posted_body() + self.assertEqual(self.posted[0][1]["commit_id"], self.LIVE) + self.assertIn(f"Only regenerated artifacts differ: `{self.ARTIFACT}`.", body) + self.assertEqual(AA.CARRIED_MARKER_RE.search(body).groups(), (self.REVIEWED, self.MERGE_BASE)) + self.emitted_line(f"only regenerated artifacts differ: `{self.ARTIFACT}`") + + # -- what carries ------------------------------------------------------ + + def test_a_clean_merge_with_a_byte_identical_own_diff_still_carries(self): + # The base path, with and without the globs: nothing differs, so the + # commits are not read and the body claims nothing about artifacts. + for globs in ("", self.GLOBS): + with self.subTest(globs=globs): + self.setUp() + self.compares[(self.BASE_SHA, self.LIVE)] = self.two_files(42, 50) + self.compares[(self.BASE_SHA, self.LIVE)]["commits"] = [self.commit(self.LIVE, self.REVIEWED)] + self.assertEqual(self.run_dismiss(globs=globs), 0, self.printed) + self.assertNotIn("regenerated", self.posted_body()) + self.assertEqual(self.dismissed, {11: AA.carried_message(self.LIVE)}) + + def test_merges_where_only_a_matching_file_differs_carry(self): + self.assert_carried_naming_the_artifact(self.run_dismiss(globs=self.GLOBS)) + # Saying "identical" here would be false: it was not. + self.assertEqual(self.dismissed, {11: AA.carried_message(self.LIVE, [self.ARTIFACT])}) + self.assertIn("apart from regenerated artifacts", self.dismissed[11]) + + def test_globs_may_be_newline_or_space_separated(self): + for globs in (f"svc/none.go\n{self.GLOBS}\n", f"svc/none.go {self.GLOBS}"): + with self.subTest(globs=globs): + self.setUp() + self.assert_carried_naming_the_artifact(self.run_dismiss(globs=globs)) + + def test_several_merges_since_the_review_carry(self): + middle = "8" * 40 + self.live["commits"] = [self.commit(self.REVIEWED, self.MERGE_BASE), + self.commit(middle, self.REVIEWED, self.BASE_SHA2), + self.commit(self.LIVE, middle, self.BASE_SHA)] + self.live["total_commits"] = 3 + self.assert_carried_naming_the_artifact(self.run_dismiss(globs=self.GLOBS)) + + def test_a_chain_of_artifact_carries_keeps_comparing_against_the_round(self): + # Carried once already. The commits are counted from what a round read, + # not from the last carry, and the record stays the round's. + self.reviews = [self.approval(11, state="DISMISSED"), self.carried(50, head=self.OTHER)] + self.live["commits"] = [self.commit(self.REVIEWED, self.MERGE_BASE), + self.commit(self.OTHER, self.REVIEWED, self.BASE_SHA2), + self.commit(self.LIVE, self.OTHER, self.BASE_SHA)] + self.live["total_commits"] = 3 + self.assert_carried_naming_the_artifact(self.run_dismiss(globs=self.GLOBS)) + self.assertEqual(self.dismissed, {50: AA.carried_message(self.LIVE, [self.ARTIFACT])}) + + def test_the_carried_body_still_reads_back_as_a_carried_approval(self): + self.assertEqual(self.run_dismiss(globs=self.GLOBS), 0, self.printed) + body = self.posted_body() + posted = {"id": 100, "user": {"login": self.LOGIN}, "state": "APPROVED", "body": body} + self.assertEqual(AA.carry_baseline(posted), (self.REVIEWED, self.MERGE_BASE)) + self.assertEqual(AA.stale_reviews_to_dismiss([posted], self.LOGIN, self.LIVE, "main"), []) + self.assertEqual(AA.APPROVED_THRESHOLD_RE.search(body).group(1), "low") + + # -- what dismisses ---------------------------------------------------- + + def assert_dismissed(self, globs=None): + self.assert_dismissed_as_before(self.run_dismiss(globs=self.GLOBS if globs is None else globs)) + self.emitted_line("differ from the ones reviewed") + + def test_the_same_push_with_no_globs_dismisses(self): + for globs in ("", " \n "): + with self.subTest(globs=globs): + self.setUp() + self.assert_dismissed(globs) + + def test_globs_naming_another_file_dismiss(self): + self.assert_dismissed("gen/other.json") + + def test_a_conflict_resolution_changing_another_file_dismisses(self): + self.compares[(self.BASE_SHA, self.LIVE)] = self.two_files(42, 50, artifact_added="+regenerated", + added="+resolved", commits=self.merged()) + self.assert_dismissed() + self.emitted_line("match no `carry_forward_regenerated_globs` entry") + + def test_an_ordinary_commit_touching_only_the_artifact_dismisses(self): + self.live["commits"] = [self.commit(self.REVIEWED, self.MERGE_BASE), self.commit(self.LIVE, self.REVIEWED)] + self.assert_dismissed() + self.emitted_line("not provably all merges from the base") + + def test_an_ordinary_commit_under_a_later_merge_dismisses(self): + self.live["commits"] = [self.commit(self.REVIEWED, self.MERGE_BASE), self.commit(self.OTHER, self.REVIEWED), + self.commit(self.LIVE, self.OTHER, self.BASE_SHA)] + self.live["total_commits"] = 3 + self.assert_dismissed() + + def test_a_merge_that_adds_or_drops_a_file_dismisses(self): + def add(name): + self.live["files"].append({"filename": name, "status": "added", "changes": 1, "patch": self.patch(1)}) + + def drop(name): + self.live["files"][:] = [f for f in self.live["files"] if f["filename"] != name] + + for label, change in (("adds a file", lambda: add("svc/b.go")), ("adds an artifact", lambda: add("gen/b.json")), + ("drops a file", lambda: drop("svc/a.go")), ("drops the artifact", lambda: drop(self.ARTIFACT))): + with self.subTest(label): + self.setUp() + change() + self.assert_dismissed() + self.emitted_line("the set of changed files is different") + + def test_a_changed_status_or_previous_name_dismisses(self): + for field, value in (("status", "added"), ("previous_filename", "gen/old.json")): + with self.subTest(field): + self.setUp() + self.live["files"][1][field] = value + self.assert_dismissed() + + def test_a_merge_whose_other_parent_is_not_on_the_base_dismisses(self): + # The other branch's commit is in the list, so neither parent is on the + # base, and that commit is ordinary too. + self.live["commits"] = [self.commit(self.REVIEWED, self.MERGE_BASE), self.commit(self.OTHER, self.MERGE_BASE), + self.commit(self.LIVE, self.REVIEWED, self.OTHER)] + self.live["total_commits"] = 3 + self.assert_dismissed() + + def test_a_merge_of_a_branch_that_itself_merged_the_base_dismisses(self): + self.live["commits"] = [self.commit(self.REVIEWED, self.MERGE_BASE), + self.commit(self.OTHER, self.MERGE_BASE, self.BASE_SHA), + self.commit(self.LIVE, self.REVIEWED, self.OTHER)] + self.live["total_commits"] = 3 + self.assert_dismissed() + + def test_a_head_no_longer_holding_the_reviewed_commit_dismisses(self): + # Rewritten history: the reviewed commit is gone from the list, though + # every commit left in it is a merge from the base. + self.live["commits"] = [self.commit(self.OTHER, self.MERGE_BASE, self.BASE_SHA2), + self.commit(self.LIVE, self.OTHER, self.BASE_SHA)] + self.assert_dismissed() + + def test_a_commit_with_one_parent_is_no_merge_even_on_the_base(self): + data = {"commits": [self.commit(self.REVIEWED, self.MERGE_BASE), self.commit(self.LIVE, self.BASE_SHA)], + "total_commits": 2} + self.assertFalse(AA.only_merges_from_base(data, self.REVIEWED)) + + def test_a_retarget_adding_no_commit_dismisses(self): + self.pr["head"]["sha"] = self.REVIEWED + self.compares[(self.BASE_SHA, self.REVIEWED)] = self.two_files( + 42, 50, artifact_added="+regenerated", commits=[self.commit(self.REVIEWED, self.MERGE_BASE)]) + self.assert_dismissed_as_before(self.run_dismiss(globs=self.GLOBS, all_approvals=True), AA.BASE_CHANGED_MESSAGE) + + def test_a_commit_list_it_cannot_trust_dismisses(self): + # Cut short at the API's cap, a commit it holds back could be anything, + # and a parent it holds back would pass for one on the base. + for label, change in (("cut short", lambda: self.live.update(total_commits=251)), + ("no total", lambda: self.live.pop("total_commits")), + ("no list", lambda: self.live.pop("commits")), + ("no parents", lambda: self.live["commits"][1].pop("parents")), + ("bad parent", lambda: self.live["commits"][1].update(parents=["x", "y"])), + ("not a commit", lambda: self.live["commits"].__setitem__(1, "x"))): + with self.subTest(label): + self.setUp() + change() + self.assert_dismissed() + + def test_an_artifact_too_large_to_diff_dismisses(self): + # Nothing to compare: the rule never gets to weigh it. + self.live["files"][1].pop("patch") + self.assert_dismissed_as_before(self.run_dismiss(globs=self.GLOBS)) + self.emitted_line("could not vouch") + + def test_the_carrys_other_preconditions_still_hold(self): + for label, kwargs, change in ( + ("a tightened threshold", {"threshold": "nit"}, None), + ("the switch off", {"carry": "false"}, None), + ("another event", {"event": "pull_request"}, None), + ("an author it does not admit", {"authors": "someone-else"}, None), + ("a veto label", {}, lambda: self.pr["labels"].append({"name": "skip-cursor-review"})), + ("a draft", {}, lambda: self.pr.update(draft=True))): + with self.subTest(label): + self.setUp() + if change: + change() + self.run_dismiss(globs=self.GLOBS, **kwargs) + self.assertEqual(self.posted, [], self.emitted) + self.assertIn(11, self.dismissed) + + def test_the_workflow_passes_the_input_to_dismiss_stale(self): + root = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..", "..") + with open(os.path.join(root, ".github", "workflows", "cursor-review.yml"), encoding="utf-8") as f: + workflow = f.read() + self.assertIn("CARRY_FORWARD_REGENERATED_GLOBS: ${{ inputs.carry_forward_regenerated_globs }}", workflow) + self.assertIn('--carry-forward-regenerated-globs "$CARRY_FORWARD_REGENERATED_GLOBS"', workflow) + + # -- the globs --------------------------------------------------------- + + def test_how_a_glob_matches(self): + for glob, path, expected in ( + ("gen/quote.json", "gen/quote.json", True), + ("gen/quote.json", "x/gen/quote.json", False), + ("quote.json", "gen/quote.json", False), + ("gen/*.json", "gen/quote.json", True), + ("gen/*.json", "gen/sub/quote.json", False), + ("gen/?.json", "gen/q.json", True), + ("gen/?.json", "gen/quote.json", False), + ("gen/**/q.json", "gen/q.json", True), + ("gen/**/q.json", "gen/a/b/q.json", True), + ("**/q.json", "q.json", True), + ("**/q.json", "a/q.json", True), + ("gen/**", "gen/a/b.json", True), + ("gen.json", "genxjson", False), + ("gen/[ab].json", "gen/a.json", False)): + with self.subTest(glob=glob, path=path): + self.assertEqual(AA.matches_regenerated_glob(path, [glob]), expected) + + def test_a_path_that_could_forge_a_record_never_matches(self): + # git allows these, and the name lands in a body parsed back for its + # records: this line would precede the real threshold, loosening it. + for path in ("gen/x\n_Threshold: `high` (set).json", "gen/`x`.json", "gen/.json"): + with self.subTest(path=path): + self.assertFalse(AA.matches_regenerated_glob(path, ["**"])) + self.setUp() + for files in self.compares.values(): + files["files"][1]["filename"] = path + self.assert_dismissed("**") + + class ApprovalMergeBaseTest(unittest.TestCase): """What an approval records for carry-forward to compare against (BE-20585). diff --git a/.github/workflows/cursor-review.yml b/.github/workflows/cursor-review.yml index a5661710..afed3021 100644 --- a/.github/workflows/cursor-review.yml +++ b/.github/workflows/cursor-review.yml @@ -395,6 +395,24 @@ on: type: boolean required: false default: false + carry_forward_regenerated_globs: + description: >- + Opt-in, with `carry_forward_unchanged_diff`: newline- or + space-separated globs naming checked-in generated files allowed to + differ from what a round reviewed. Empty (the default) changes + nothing. When the PR's own diff differs, the approval still carries + only if every commit the PR gained since the reviewed commit is a + merge with a parent on the base, the set of changed files (name, + previous name, status) is the same, and every file whose patch + differs matches a glob; everything else above still applies. Each + glob matches the whole repo-relative path: `*` and `?` stop at a + `/`, `**/` spans directories, and a path holding anything but + letters, digits and `._/@+-` never matches. Their content is NOT + re-reviewed, so a glob must only name files a required CI check + regenerates and verifies from code. + type: string + required: false + default: '' approve_only_on_push: description: >- Opt-in: on a push (`synchronize`, or `reopened`) to a PR whose LAST @@ -6564,7 +6582,8 @@ jobs: # base the approval recorded, or what a carried approval copied forward), # never from a review the bot identity posted. Only under # `pull_request_target`; it compares compare-API diffs and never reads the - # PR's tree. + # PR's tree. `carry_forward_regenerated_globs` also lets a push of nothing + # but merges from the base carry when only files those globs name differ. # # NOT gated on `approve_max_severity`: unsetting that input is the kill # switch for NEW approvals, and an approval already on the PR must still be @@ -6719,6 +6738,7 @@ jobs: # under pull_request_target, and reads the compare API with # FALLBACK_TOKEN (this job's contents: read), not the approver token. CARRY_FORWARD: ${{ inputs.carry_forward_unchanged_diff }} + CARRY_FORWARD_REGENERATED_GLOBS: ${{ inputs.carry_forward_regenerated_globs }} # Only the stale card's next-step sentence: the `approve-only` job # decides the re-check itself. APPROVE_ONLY: ${{ inputs.approve_only_on_push }} @@ -6788,6 +6808,7 @@ jobs: --can-relabel "$CAN_RELABEL" \ --run-without-label "$RUN_WITHOUT_LABEL" \ --carry-forward "$CARRY_FORWARD" \ + --carry-forward-regenerated-globs "$CARRY_FORWARD_REGENERATED_GLOBS" \ --event-name "$EVENT_NAME" \ --approve-only "$APPROVE_ONLY" \ --approve-only-delay-minutes "$AUTO_REREVIEW_DELAY" \ diff --git a/docs/callers/cursor-review.md b/docs/callers/cursor-review.md index 317d30a3..6126235a 100644 --- a/docs/callers/cursor-review.md +++ b/docs/callers/cursor-review.md @@ -147,6 +147,7 @@ pull-requests: write # posting the consolidated review (and, at the round cap, | `approve_only_on_push` | `false` | Re-run only the **axes** on a push after a round whose panel passed. On `synchronize`/`reopened`, when the approver's [status card](cursor-approve.md#a-push-re-checked-by-the-axes-alone) shows the last round got past its panel (its axes ran, whatever they said), the reusable runs **no panel** and sets `approve_gate` to `pass`, so the caller's existing cursor-approve jobs judge the pushed head. After `auto_rereview_delay_minutes` (a burst of pushes yields one re-check, on the head still live), and only while the PR is open, not a draft, carries the review label and neither `skip-cursor-review` nor `needs-human-review`, has no approver approval already on that head, no open review thread above `approve_max_severity` (or unbadged), no review label applied since the approver's last decision (a full round in flight), and is under `approve_only_max_runs`. No consolidated review is posted, so it never counts toward `max_rounds`. Needs `max_rounds` > 0 (an uncapped round's card names no round to count re-checks under) and never follows the **last** round of `max_rounds`, which is a human's. A panel round that does not pass leaves the lane: the next push needs a full round, as before — and so, for good, does a status card anyone but the approver identity ever edited (read from the comment's edit history: anyone with write access can edit it). When it re-checks a push, `auto_rereview_after_dismissal` does not relabel for it. Relabelling still runs a full round. Needs `APPROVER_TOKEN`, `approve_max_severity` and a `pull_request_target` caller; no effect under `run_without_label`. With `auto_rereview_after_dismissal` on too, a push on a round whose **panel** blocked (cursor-review's own decide requested changes — not an untrusted or capped round) starts a **full** round by itself through the same debounced relabel, once per head, under the same conditions and `max_rounds`. | | `approve_only_max_runs` | `10` | Cap on `approve_only_on_push` re-checks per panel round, counted on the status card; a new panel round resets it. At the cap a push is left to a relabel (or `auto_rereview_after_dismissal`). Clamped to `50`, the heads the card's record keeps; unparsable re-checks nothing. | | `carry_forward_unchanged_diff` | `false` | Opt-in, for stacked PRs: when a push or a retarget would dismiss the approver's approval but the PR's **own** changes are identical to the ones a round reviewed (a rebase or re-merge onto a moved base, e.g. a lower PR of the stack changed or landed), re-approve the live head, then dismiss the old approval; the card keeps its approval and no round is scheduled. Identical is the compare API's diff from the merge base the approval recorded to the reviewed commit against the diff from the live base to the live head, file by file, context lines and hunk headings included, hunk line numbers ignored. Anything it cannot vouch for dismisses as before, and an approval that records no merge base (every one an earlier version posted, too) never carries. Only under `pull_request_target`, and only with `approve_max_severity` set and `bot_app_id` posting the rounds (with `defer_approval`, pass cursor-approve that App's `poster_login` too). See [Auto-approve](#auto-approve) for every condition and the [trust model](#trust-model) for what it trusts. | +| `carry_forward_regenerated_globs` | `''` | Opt-in, with `carry_forward_unchanged_diff`: newline- or space-separated globs naming checked-in generated files allowed to differ from what a round reviewed, when the push is nothing but merges from the base. Empty changes nothing. **Their content is not re-reviewed, so a glob must only name files a required CI check regenerates and verifies from code.** See [Auto-approve](#auto-approve) for the conditions and the glob syntax. | | `max_rounds` | `5` | Cap on review rounds per PR (`0` → no cap). At or over it, no panel runs: the PR is labelled `needs-human-review`, one comment lists the latest round's open findings above the threshold, and the `approve_gate` output is `capped`. A round whose own decision found the PR head had moved (a newer push superseded it) does not count, at most once per reviewed commit and at most `max_rounds` times. Removing the label resets the count. See [round cap](#round-cap-and-the-approve_gate-output). | | `runs_on` | `'"ubuntu-latest"'` | JSON-encoded `runs-on` for `diff-size`, `preflight`, the `review` panel and `consolidate` only — the jobs that hold no write credential. Every other job stays on GitHub-hosted `ubuntu-latest`. A self-hosted pool is fine only if it is one-job-per-fresh-pod, identity-free and private-network-isolated (those jobs run models with shell over PR code), on linux/x64 with bash, git, curl, jq, python3, gh, tar and GNU coreutils. Empty falls back to the default, so `${{ vars.CURSOR_REVIEW_RUNS_ON }}` is safe while the variable is unset; e.g. `'["self-hosted", "linux", "x64"]'`. | | `approver_environment` | `''` | The caller-repo environment that holds `APPROVER_TOKEN` (as an environment secret). Declared as `environment:` on the jobs that read the token, only under `pull_request_target`, and on none that checks out PR code. With a deployment-branch policy of the base branch only, the token is unreachable from `pull_request` runs. Use with a `pull_request_target` caller — see the [trust model](#trust-model). Empty = no environment. | @@ -1081,6 +1082,30 @@ has nothing left to carry. A run that reads the PR once both have landed carries it, and a merge-commit merge keeps those commits in the base and carries. +**Keeping it across a regenerated artifact (`carry_forward_regenerated_globs`).** +When a PR rewrites a checked-in generated file that the base also regenerates, +its lines in that file differ after every merge from the base, so the identity +check above correctly dismisses, though nothing hand-written changed. List such files here and, when the PR's own diff differs, the approval +still carries if all of this holds, on top of every condition above: + +- every commit the PR gained since the reviewed commit is a merge with a parent + on the live base, read from the compare of the live base with the live head — + an ordinary commit, a merge of another branch, a retarget with no new commit, + a reviewed commit the head no longer contains (a rebase), or more commits than + the API returns (250) dismisses; +- the set of changed files (name, previous name, status) is the same, so a merge + that adds or drops a file, artifact or not, dismisses; +- every file whose patch differs matches a glob, and every other file's patch is + identical. A matching file must still have a patch: one too large for the + compare API to return dismisses. + +Each glob matches the whole repo-relative path, so a bare `gen.json` means the +file at the root only: `*` and `?` stop at a `/`, `**/` spans directories, `**` +spans anything, and everything else is literal. This differs from +`extra_generated_globs`, where a bare name matches any directory. A path holding +anything but letters, digits and `._/@+-` never matches, since its name goes in +the review body. The carried approval and the log name the files that differed. + **Widened callers: keep `edited` from cancelling a panel.** The label-only caller above keys its group on `github.event.label.name`, so `edited` never shares a group with the labelled run. If you dropped the label from the group @@ -1184,6 +1209,14 @@ push, or re-applying the review label). by `github-actions[bot]`, and like every approval it posts only under `pull_request_target`. +- **`carry_forward_regenerated_globs` approves content no round read.** A merge + can put anything in a file a glob names, by hand as easily as by regenerating + it, and the carry cannot tell which. So a glob must only name files a + **required** CI check regenerates from code and fails on any difference; then + that check, not the approval, vouches for the content. Without one, a glob is a + path into the repo that needs push access and no review. Set the input in the + caller, on the base branch: a value a PR could influence defeats this. + So even in this shape the approval is no stronger than **push access** to the repo. Do not let it be the only review that satisfies a ruleset for code that needs a human's eyes — think twice before giving it an `APPROVER_TOKEN` that is From 904ed45549641ee0e80fdc2f04c051b396ad941b Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Sun, 1 Nov 2026 18:06:21 +0000 Subject: [PATCH 2/2] fix(cursor-review): match regenerated globs without backtracking The PR author picks the path and the regex backtracked through each `**`: one 4095-character path took 25s against two `**` and over 45s against three. Enough of those outlast dismiss-stale's 5 minutes, so the job dies and the stale approval stands. Walk the reachable path offsets once per glob character instead. Co-Authored-By: Claude Opus 5.5 --- .github/cursor-review/auto-approve.py | 29 ++++++++++++------- .../cursor-review/tests/test_auto_approve.py | 14 +++++++++ 2 files changed, 33 insertions(+), 10 deletions(-) diff --git a/.github/cursor-review/auto-approve.py b/.github/cursor-review/auto-approve.py index 8104acd4..b45a6179 100644 --- a/.github/cursor-review/auto-approve.py +++ b/.github/cursor-review/auto-approve.py @@ -2789,25 +2789,34 @@ def normalize_patch(patch: str) -> str: return "\n".join(lines) -def glob_regex(glob: str) -> str: - """`glob` as a regex over a whole repo-relative path: `*` and `?` stop at a - `/`, `**/` spans directories, `**` spans anything, the rest is literal.""" - out, i = [], 0 - while i < len(glob): +def glob_matches(glob: str, path: str) -> bool: + """Whether `glob` matches a whole repo-relative path: `*` and `?` stop at a + `/`, `**/` spans directories, `**` spans anything, the rest is literal. + + Not a regex: the PR author picks the path, and backtracking through a few + `**` on a long one would outlast the job, leaving the stale approval standing. + This walks the offsets the glob so far can reach, once per glob character.""" + at, i, end = {0}, 0, len(path) + while i < len(glob) and at: if glob.startswith("**/", i): - out.append("(?:.*/)?") + at |= {j + 1 for j in range(min(at), end) if path[j] == "/"} i += 3 elif glob.startswith("**", i): - out.append(".*") + at = set(range(min(at), end + 1)) i += 2 + elif glob[i] == "*": + for j in range(min(at), end): + if j in at and path[j] != "/": + at.add(j + 1) + i += 1 else: - out.append({"*": "[^/]*", "?": "[^/]"}.get(glob[i], re.escape(glob[i]))) + at = {j + 1 for j in at if j < end and (path[j] != "/" if glob[i] == "?" else path[j] == glob[i])} i += 1 - return "".join(out) + return end in at def matches_regenerated_glob(path: str, globs: list) -> bool: - return bool(REGENERATED_PATH_RE.fullmatch(path)) and any(re.fullmatch(glob_regex(g), path) for g in globs) + return bool(REGENERATED_PATH_RE.fullmatch(path)) and any(glob_matches(g, path) for g in globs) def only_merges_from_base(live_read, reviewed: str) -> bool: diff --git a/.github/cursor-review/tests/test_auto_approve.py b/.github/cursor-review/tests/test_auto_approve.py index c78f85c5..734abed9 100644 --- a/.github/cursor-review/tests/test_auto_approve.py +++ b/.github/cursor-review/tests/test_auto_approve.py @@ -30,6 +30,7 @@ import itertools import os import re +import signal import subprocess import tempfile import unittest @@ -4862,6 +4863,19 @@ def test_how_a_glob_matches(self): with self.subTest(glob=glob, path=path): self.assertEqual(AA.matches_regenerated_glob(path, [glob]), expected) + def test_a_long_path_cannot_stall_the_match(self): + # The PR author picks the path. A backtracking match spent over 45s on + # this one, and 299 paths at 25s each outlast the job's 5 minutes, which + # leaves the stale approval standing. + def stalled(*_): + raise TimeoutError("the match backtracked") + previous = signal.signal(signal.SIGALRM, stalled) + self.addCleanup(signal.signal, signal.SIGALRM, previous) + self.addCleanup(signal.alarm, 0) + signal.alarm(5) + self.assertFalse(AA.matches_regenerated_glob("a/" * 2047 + "x", ["**/a/**/a/**/a/**/b"])) + self.assertTrue(AA.matches_regenerated_glob("a/" * 2047 + "b", ["**/a/**/a/**/a/**/b"])) + def test_a_path_that_could_forge_a_record_never_matches(self): # git allows these, and the name lands in a body parsed back for its # records: this line would precede the real threshold, loosening it.