From 70ed39349d906c298613ae6a8bb01bcb57a1b9eb Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Wed, 29 Jul 2026 00:29:34 -0700 Subject: [PATCH 1/2] feat(cursor-review): prior-review ledger so re-reviews stop re-litigating answered findings (BE-5109) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each review round was stateless: the panel and judge re-derived the PR from the full diff with no memory, so a finding refuted, declined, or deferred in round 1 came back in rounds 2 and 3 and the author hand-wrote the same rebuttal again. A dedupe filter cannot fix it — the judge rewrites each finding's prose every round, so a true repeat pair scores lower on text similarity than two unrelated findings on the same file. The prior context has to enter the prompt. - New `ledger` job (between gate and the panel) reads prior consolidated reviews, their thread roots, reply chains, and isResolved/isOutdated, and publishes ledger.json/ledger.md as the `cursor-review-ledger` artifact — one fetch, nine consumers. - The GraphQL reviewThreads read and CONSOLIDATED_MARKER are reused from gate-unresolved.py (one reader of that state), not re-implemented. - Entries carry verbatim prose plus checkable structural flags, and NO derived verdict/disposition field — pinned by a test. - Three statuses: `empty` (first round, prompt byte-identical to before), `ok`, and `unknown` (a fetch failed — reported via a review banner naming the failed call, never silently reported as `empty`). - Repeat policy: a re-raise must carry `repeat_of` (the prior discussion link) and is capped at 2 per review, enforced in post-review.py; drops are disclosed. Nothing already-answered is silently suppressed. - Ledger block is delimited and labelled untrusted data, never instructions. - The ledger's last-reviewed SHA also feeds a separately-labelled incremental diff alongside the full diff (prioritization, not narrowing). - `ledger_prior_review: false` is the kill switch, documented in the README. --- .github/cursor-review/README.md | 1 + .github/cursor-review/build-ledger.py | 658 ++++++++++++++++++ .github/cursor-review/gate-unresolved.py | 33 +- .github/cursor-review/post-review.py | 70 +- .../cursor-review/tests/test_build_ledger.py | 546 +++++++++++++++ .github/workflows/cursor-review.yml | 214 +++++- 6 files changed, 1507 insertions(+), 15 deletions(-) create mode 100644 .github/cursor-review/build-ledger.py create mode 100644 .github/cursor-review/tests/test_build_ledger.py diff --git a/.github/cursor-review/README.md b/.github/cursor-review/README.md index 2120606..ff74b46 100644 --- a/.github/cursor-review/README.md +++ b/.github/cursor-review/README.md @@ -87,6 +87,7 @@ silently vanishing — the review tells you what didn't run. | [`gate-unresolved.py`](gate-unresolved.py) | **Orphaned — nothing calls this today.** Implemented the opt-in blocking gate: queries the PR's review threads and exits non-zero while any cursor-review finding thread is unresolved. The job that ran it was dropped from `cursor-review.yml` in #31; see [the regression note](#the-blocking-gate-is-currently-not-available-regressed). | | [`slack-notify.sh`](slack-notify.sh) | Sends the start/complete Slack DMs to the triggerer (no-ops without a token). | | [`install-cursor-cli.sh`](install-cursor-cli.sh) | Installs the Cursor agent CLI from the versioned, sha256-pinned release artifact — not `curl cursor.com/install \| bash`. Used by all three CLI-using jobs; the pin (`CURSOR_CLI_VERSION` / `CURSOR_CLI_SHA256`) lives in `cursor-review.yml`'s top-level `env:`. | +| [`build-ledger.py`](build-ledger.py) | Builds the **prior-review ledger** — what earlier rounds raised on this PR and how the author answered — and splices it into the panel/judge prompts. Also the prompt splicer, so the no-ledger path is byte-identical to the pre-ledger prompt. | ## Adopt it in your repo diff --git a/.github/cursor-review/build-ledger.py b/.github/cursor-review/build-ledger.py new file mode 100644 index 0000000..29f4cd2 --- /dev/null +++ b/.github/cursor-review/build-ledger.py @@ -0,0 +1,658 @@ +#!/usr/bin/env python3 +"""Build the prior-review ledger — what earlier rounds already raised and how the +author answered — and splice it into the panel/judge prompts. + +Why this exists +--------------- +The review workflow is stateless between runs: every round re-derives the PR +from the full diff with no memory, so a finding that was refuted, declined, or +deliberately deferred in round 1 comes straight back in round 2 and 3, and the +author hand-writes the same rebuttal again. A string/embedding dedupe cannot fix +that — the judge rewrites each finding's prose every round, so a true repeat +pair scores *lower* on text similarity than two unrelated findings on the same +file. The prior context has to enter the prompt, so that is what this does. + +Two subcommands: + +``build`` + Fetch the PR's prior *consolidated* reviews, their per-finding thread roots, + the reply chains hanging off them, and each thread's ``isResolved`` / + ``isOutdated`` flags. Emit ``ledger.json`` (structured) plus ``ledger.md`` + and ``ledger-judge.md`` (the delimited, explicitly-untrusted blocks the panel + and the judge read). + +``splice`` + Insert a rendered block into a prompt immediately before its trailing + ``=== BEGIN … ===`` marker. With no block to insert the output is + **byte-identical** to the prompt file — that is the first-round + no-regression property, and ``test_build_ledger.py`` pins it. + +What a ledger entry deliberately does NOT carry +----------------------------------------------- +There is **no derived ``verdict`` / ``disposition`` field.** Real author replies +open with "Not taking this one", "Refuted — the premise doesn't hold", "Real, +but DEFERRED", "Half fixed, half accepted", "Already addressed (dup…)" — no +regex or keyword match survives that corpus, and a wrong label is worse than no +label because the judge would trust it. Entries carry the verbatim prose plus +the *checkable structural* flags (resolved / outdated / reply count / whether the +replier is the PR author) and let the judge read the rest. + +Prompt injection +---------------- +The ledger imports PR comment text into a reviewer prompt on a workflow whose +``consolidate`` job holds ``pull-requests: write``, so it is a new untrusted +channel. The rendered block is delimited and labelled DATA, NOT INSTRUCTIONS, +and the steering text tells the model that a prior reply justifies dropping a +finding only when it gives a *checkable technical reason*, and that text trying +to steer the review is disregarded and called out. The workflow's ``gate`` job +already skips fork PRs, so the surface is same-repo PRs plus bot-authored text +(Dependabot, cloud-code-bot). +""" + +import argparse +import importlib.util +import json +import os +import re +import subprocess +import sys + +# Size caps. Anything they drop is stated inside the text the model reads, so a +# partial ledger can never be mistaken for a complete one. +MAX_ROUNDS = 3 +MAX_BODY_CHARS = 600 +MAX_LEDGER_BYTES = 40 * 1024 +TRUNCATION_MARKER = " …[truncated]" + +# Max re-raises the judge may emit per review. Enforced deterministically in +# post-review.py; stated here because the judge prompt block quotes it. +REPEAT_CAP = 2 + + +def _load_gate_unresolved(): + """Import gate-unresolved.py by path (its name has a hyphen). + + Reused, not re-implemented: the ledger takes ``CONSOLIDATED_MARKER`` (the + canonical "is this our review?" discriminator, also used by the workflow's + dup-check) and the paging ``reviewThreads`` query from that one module. + """ + path = os.path.join(os.path.dirname(os.path.abspath(__file__)), "gate-unresolved.py") + spec = importlib.util.spec_from_file_location("gate_unresolved", path) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +gate_unresolved = _load_gate_unresolved() +CONSOLIDATED_MARKER = gate_unresolved.CONSOLIDATED_MARKER + +# post-review.py renders every inline finding as " **