fix(cursor-review): carry re-raise lineage through the body-only sentinel by mattmillerai · Pull Request #273 · Comfy-Org/github-workflows

mattmillerai · 2026-09-08T18:32:38Z

ELI-5

When the reviewer raises a finding it already raised before, it links back to the old conversation ("re-raise of …, round 2"). That link is what stops the reviewer from re-litigating the same point forever — only two such re-raises are allowed per round, and only findings the author actually answered cost one of those two slots.

But some findings can't be attached to a line of the diff, so they get written into the review body instead. Those have no conversation of their own, so the link was deliberately scrubbed out of them — and it was scrubbed without being saved anywhere. The next round then saw a brand-new, never-answered finding and let it be raised again for free. One such hop broke the budget for that finding forever after.

This keeps the link. It travels as a hidden field rather than as visible text, and the next round looks it up against the PR's real comments to find the original conversation, check whether the author actually answered it, and quote what they said. If they did, raising it again costs a slot, exactly like it always should have.

What changed

The policy is unchanged — only an ANSWERED finding costs a repeat slot. What changed is which finding that is read off: the lineage ancestor of a body-only entry, using the ancestor's real answer state, instead of the thread-less copy that is unanswered by construction.

Writer — .github/cursor-review/post-review.py

normalize_comments still stores the rendered ↩︎ re-raise of … trailer in item["repeat_of"], so enforce_repeat_cap's count and strip_repeat_line's reconstruction are byte-for-byte untouched. It now also stores the raw URL as item["repeat_url"].
render_body_only_sentinel emits one optional key, repeat_of, following the exact lost_to_fallback discipline: emitted only for an item that carries it, so a payload with no re-raise in it renders byte-identically to before. The ancestor's round is deliberately not carried — the reader derives it from the resolved ancestor's own review, which is truthful by construction and available whenever the URL resolves at all, so a repeat_round field would be payload nothing reads inside a body under a hard size cap that can drop a real finding to make room for it.
The URL must be one anchored GitHub discussion permalink of at most 512 chars. Matched with fullmatch, not match: $ also matches just before a trailing newline, and JSON round-trips a line break losslessly — it would arrive on the ledger's own re_raise_of: line at column 0 of the prompt, which is the forged-fence shape the field flattener already exists for.
strip_repeat_line still clears the trailer from the body. The URL never travels in prose. The prose trailer still shows the round, and coerce_repeat_round still guards that render.

Reader — .github/cursor-review/build-ledger.py

New _resolve_lineage parses the trailing comment id, looks it up in the PR's own fetched comments, and requires a ROOT comment belonging to one of our consolidated reviews. That is the integrity check; a hallucinated id, a reply, a human's review comment and a foreign host all resolve to nothing.
Resolution runs over round_by_review, which covers every consolidated review and is built before the MAX_ROUNDS age filter — so the ancestor is still found after its own round has aged out of the ledger. That is the spike's scenario.
The rendered permalink is always the ancestor's own html_url, never the relayed string, and an ancestor without one yields no lineage at all. REPEAT_URL_RE shape-checks the owner/repo/PR-number half without comparing it to anything, and only the trailing id is read — so a URL naming another repo does resolve against a genuine root of ours; refusing to relay it is what keeps that from mattering.
A resolved entry gains repeat_of / repeat_round / repeat_answered_count, plus a bounded repeat_answer excerpt of the ancestor's most recent answering reply, and renders re_raise_of: <url> (round N; ancestor_answers=k) with a re_raise_answer: line beneath it. Its own thread.answered_count stays 0 and discussion_url stays "" — it still has no thread of its own.
ancestor_answers=, not a second answers_from_author_or_maintainer=: the entry's own thread: line already uses that token with a contradictory value, and the judge's first REPEAT POLICY bullet keys on it to demand a repeat_of equal to the entry's own discussion_url, which a demoted entry has none of.
With k >= 1 the closing parenthetical and both steering blocks say a further re-raise must carry repeat_of and costs a slot; with k == 0 the existing "needs no repeat_of" wording is kept verbatim. The same rule covers a [post-failed] entry, which has the identical lineage problem.
A claim that fails to resolve is its own third state (repeat_unresolved): the entry says the lineage is UNVERIFIED and withholds the cap exemption, without asserting the converse. Dismissing a review or deleting the ancestor comment are ordinary maintainer actions that otherwise rebuilt the entry as a plain cap-exempt finding and silently reopened the chain; asserting the opposite would instead let a forged unresolvable URL spend a repeat slot.
unanswered_count / unanchorable_count / post_failed_count are unchanged, and the entry is not added to unanswered.

Verification against the artifact the ticket names

The ticket cites Comfy-Org/github-workflows#230 (a 4-round chain) as where the judge's repeat_of behaviour was observed. It is public, so I exercised the reader against its real payloads rather than only against fixtures — read-only gh api reads, no writes:

That PR carries 4 consolidated rounds and 2 live re-raise trailers, both pointing at round-1 roots.
_resolve_lineage resolves both real trailers to their real ancestors: discussion_r3859833104 → round 1, ancestor_answers=1, and discussion_r3859833108 → round 1, ancestor_answers=1. Under the old behaviour a demoted hop of either would have been rebuilt as cap-free. Both now also return the ancestor's real reply text for the re_raise_answer: excerpt, so that field is populated from production payloads and not only from fixtures.
The integrity gates reject correctly against that same real data: a reply id → None, an id not on the PR → None, another host → None. A foreign repo slug with a real id does resolve — as documented — and renders our own permalink, with attacker/evil never reaching the block.
A full build_ledger over ci(rosters): add a leak-safe roster shape probe (TEMPORARY) #230 runs clean (17 entries, 2 rounds of 4, both existing truncation notes intact, 28.6 KB rendered) and 0 entries carry lineage — every finding there anchored, so the anchored path is provably untouched on production data.
html_url presence was checked rather than assumed: 81/81 review comments on ci(rosters): add a leak-safe roster shape probe (TEMPORARY) #230 and 8/8 on this PR carry a non-empty one, so dropping the relayed-string fallback removes a path unreachable in practice.

Judgment calls

A latent crash fixed as a side effect. Sharing the round coercion required guarding int(): str.isdigit() is true for characters int() rejects, so repeat_round: "²" raised an unhandled ValueError straight out of normalize_comments and would have taken down the entire review post. Confirmed reproducible on the base branch before the change; build-ledger.py's _body_only_line already documents "degrade rather than raise" as the rule for exactly this parser. coerce_repeat_round still owns that guard for the prose trailer even though the sentinel no longer carries the round. Pinned by a test.
The relayed URL is never a fallback. An earlier revision used it when GitHub's payload omitted html_url, on the reasoning that a resolved id means "the same link". That reasoning was wrong — the shape gate never compares the owner/repo half — and the fallback would have rendered a model-chosen foreign link that the steering then tells the judge to emit verbatim as repeat_of. It is gone; no permalink now means no lineage.
Only the trailing id is read from the URL; the owner/repo/PR-number half is shape only. This is stated plainly rather than defended: such a URL does resolve. What bounds it is that the lookup table holds this PR's comments alone and that the rendered link is always ours.
The re_raise_of: / re_raise_answer: render lines are gated on not anchored even though only _body_only_entries writes the keys, so "anchored entries are unchanged" is a property of the renderer itself rather than an invariant a reader has to confirm elsewhere.
The quoted answer is flattened and bounded. It is a reply body — the same untrusted prose every other imported field is — landing on a metadata line, so it goes through _body_only_text as well as _defang_fences, and is capped at MAX_LINEAGE_ANSWER_CHARS (300, tighter than MAX_BODY_CHARS) because every demoted re-raise pays for it out of MAX_LEDGER_BYTES.
Negative-claim falsification: applied to the one change that removes a path. Dropping the html_url fallback is the only user-facing denial in the diff, so it was tested empirically against every real payload available (see above: 89/89 comments carry a non-empty html_url) rather than argued. The UNVERIFIED parenthetical is not a dead-end — it names the working path ("judge the finding on its merits"). Nothing else here adds a "not supported"/"unavailable"/"STOP" string, a throw/deny dead-end, or a test asserting one; the change strictly adds a capability.

Residual

The spike ticket's findings comment was not read. The change is built on evidence quoted second-hand in the ticket body from a spike whose comment thread lives in Linear, which this run has no access to. Everything in it that is checkable against code or against #230 was checked and held; anything in that comment that the ticket body did not quote is unexercised.
The ticket's own duplicate check is recorded as INCOMPLETE — one search token was dropped by a tool cap and never ran, so a pre-existing ticket describing this same fix may exist. That check cannot be completed from this run and should be redone before this is treated as the only work on the subject.
The demoted-re-raise path is not observed in production, only in tests. #230's re-raises all anchored, so its entries exercise the resolver against real data but not the demotion that triggers the bug. Nothing available to this run has a body-only re-raise on it; the end-to-end path is pinned by tests that render through the real writer and read through the real reader, which is the strongest available substitute but is not a production observation.
Writer/reader version skew degrades silently. A consumer pinned to an older SHA posting a v1 sentinel with no lineage key simply produces no lineage and the old cap-free behaviour for that round. That is consistent with every other optional key here, and is not disclosed in the ledger's truncation notes.
Prose continuation lines can still forge a metadata line — a property of the ledger's line-oriented format that predates this change (discussion_url: and thread: are equally exposed) and whose fix reshapes every entry the panel and judge read. Deferred to a follow-up with a concrete approach; the re_raise_answer: line this PR adds is flattened, so it cannot itself forge one.
The judge's repeat_of is still bounded by shape on the writing side, not against the ledger it was shown. The reader's ROOT check excludes arbitrary links, so the residual is a valid-but-wrong ancestor draining a repeat slot. Closing it needs the ledger's entry URLs plumbed into post-review.py, which is a data-flow change; deferred to a follow-up. This one predates the PR — enforce_repeat_cap has always counted judge-supplied re-raise URLs unvalidated.
Not measured: how many existing open PRs across the fleet currently carry a demoted re-raise whose lineage was already lost. This fix is forward-only — it reads lineage out of sentinels written after consumers bump their SHA.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/cursor-review/tests: 444 passed, 0 failed (422 on base, +22). Also green, unchanged by this diff: agents-md-integrity 46, public-repo-hygiene 204, workflow-pins 316, refresh-reviewers 48, groom 374. Lints: check_workflow_pins.py OK (11 workflows, 0 exempt); check_agents_md.py --root . passed with 2 pre-existing warnings (AGENTS.md 155 lines, no CODEOWNERS — both present on base); shellcheck -x bump-callers.sh clean. Live read-only checks against #230 and this PR as described above.
Deviations: none against the source request — every acceptance criterion is met, enforce_repeat_cap and review-output-mcp.py are untouched (confirmed by diff), and a sentinel with no re-raise items is byte-identical to before (pinned by test). One deviation from the first revision of this PR, made in response to the review panel: the sentinel no longer carries repeat_round, since nothing read it back and it cost bytes under a hard cap. Two panel findings were deferred rather than fixed (prose-line forging; writer-side repeat_of validation) — both predate this change and are recorded under Residual.

…inel A re-raise demoted to the review body lost its repeat lineage. The writer strips the `↩︎ re-raise of <url> (round N)` trailer out of the sentinel body (a live thread URL must never sit in the prose the judge reads) and carried the URL in no field, so the reader rebuilt the entry as a fresh unanchorable finding — answered_count 0, no discussion_url, and steering that told the judge re-raising it needs no repeat_of. One demoted hop therefore made every later re-raise of that finding cap-free. The policy is unchanged: only an ANSWERED finding costs a repeat slot. It is now applied to the lineage ANCESTOR of a body-only entry, using the ancestor's real answer state, instead of to the thread-less copy that is unanswered by construction. Writer (post-review.py): normalize_comments keeps the rendered trailer in `repeat_of` — enforce_repeat_cap and strip_repeat_line are untouched — and stores the raw URL and coerced round beside it. render_body_only_sentinel emits them as optional `repeat_of` / `repeat_round` keys, same discipline as `lost_to_fallback`, so a payload with no re-raise is byte-identical to before. The URL must be one anchored GitHub discussion permalink within 512 chars. Reader (build-ledger.py): _resolve_lineage looks the trailing comment id up in the PR's own comments and requires a ROOT of one of our consolidated reviews — the integrity check a hallucinated or foreign URL fails. Resolution runs over every consolidated review before the MAX_ROUNDS age filter, so the ancestor is still found after its own round has aged out. A resolved entry renders a `re_raise_of: <url> (round N; answers_from_author_or_maintainer=k)` line; with k >= 1 both steering blocks require a further re-raise to carry repeat_of and cost a slot, and with k == 0 nothing changes. The same rule covers a [post-failed] entry. Counts and anchored entries are unchanged. Also shares the repeat_round coercion between the trailer render and the sentinel field, which fixes a latent crash: `isdigit()` is true for characters `int()` rejects ('²'), so that pair raised out of normalize_comments and would have taken the whole review post down.

coderabbitai · 2026-09-08T18:32:58Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Team

Run ID: 036112df-24f2-4270-a3c5-00101a8d9d87

📥 Commits

Reviewing files that changed from the base of the PR and between 52b4fe6 and d79bc06.

📒 Files selected for processing (5)

.github/cursor-review/README.md
.github/cursor-review/build-ledger.py
.github/cursor-review/post-review.py
.github/cursor-review/tests/test_build_ledger.py
.github/cursor-review/tests/test_post_review.py

Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

📝 Walkthrough

Walkthrough

Changes

Demoted finding lineage tracking

Layer / File(s)	Summary
Sentinel lineage transport `.github/cursor-review/post-review.py`, `.github/cursor-review/tests/test_post_review.py`	The review writer validates `repeat_of` URLs, preserves them in sentinel data, and safely handles repeat rounds. Tests cover malformed URLs, Unicode digits, escaping, and optional fields.
Ledger lineage resolution and policy `.github/cursor-review/build-ledger.py`, `.github/cursor-review/tests/test_build_ledger.py`, `.github/cursor-review/README.md`	The ledger resolves trusted ancestor threads, records answer state and excerpts, applies repeat-cap rules, renders verified or unresolved lineage, and documents the policy. Tests cover cross-round recovery, replies, validation, rendering, and compatibility.

Sequence Diagram(s)

sequenceDiagram
  participant Finding
  participant post_review.py
  participant Sentinel
  participant build-ledger.py
  Finding->>post_review.py: provide repeat URL and round
  post_review.py->>Sentinel: serialize validated repeat_of
  Sentinel->>build-ledger.py: provide demoted finding lineage
  build-ledger.py->>build-ledger.py: resolve ancestor and answer state
  build-ledger.py-->>Finding: render lineage and repeat policy

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to d79bc

Demoted findings now retain validated re-raise lineage and remain subject to repeat limits without exposing unverified links. The implementation is covered by passing regression checks and is ready to merge.

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch matt/be-12534-repeat-lineage

✨ Simplify code

Create PR with simplified code
Commit simplified code in branch matt/be-12534-repeat-lineage

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

github-actions

🔍 Cursor Review — Consolidated panel

Triggered by @mattmillerai.

Found 8 finding(s).

Severity	Count
🟡 Medium	3
🟢 Low	4
⚪ Nit	1

Panel: 5/6 reviewers contributed findings.

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

Review-panel findings on the lineage change: - The ancestor's own `html_url` is now the ONLY permalink rendered. The `html_url or url` fallback relayed the judge's own string, and REPEAT_URL_RE shape-checks the owner/repo/PR-number half without comparing it to anything — so a `repeat_of` naming another repo resolves against a genuine root of ours and that foreign link was rendered on `re_raise_of:`, which the steering then tells the judge to emit verbatim as `repeat_of`. No permalink now means no lineage, the same way the anchored branch degrades. Verified against real data: all 81 review comments on #230 and all 8 on #273 carry a non-empty `html_url`, so the removed fallback was unreachable in practice. - A lineage CLAIM that fails to resolve is no longer rebuilt as a plain cap-EXEMPT finding. Dismissing a review or deleting the ancestor comment are ordinary maintainer actions, and either one silently reopened the cap-free chain this change exists to close. The entry now says the claim is UNVERIFIED and withholds the exemption — without asserting the converse, so a forged unresolvable URL cannot spend a repeat slot either. - The lineage line reports `ancestor_answers=` instead of reusing `answers_from_author_or_maintainer=`. One entry emitted that token twice with contradictory values, and the judge's first REPEAT POLICY bullet keys on it to demand a `repeat_of` equal to the entry's own discussion_url — which a demoted entry has none of. - The ancestor's most recent answering reply is carried as a bounded, flattened `re_raise_answer:` excerpt. The steering tells the model to engage the reason that reply gives, but in the case this whole change exists for the ancestor's round has aged out of the ledger, so that text was nowhere in the prompt. - The sentinel no longer carries `repeat_round`. Nothing read it back — the reader derives the round from the resolved ancestor's own review — so it was dead payload in a body under a hard size cap that can drop a real finding to make room for it. The prose trailer still shows the round, and `coerce_repeat_round` still guards the render. - README: the round is not "resolved back" from the payload, and a foreign link does not resolve to nothing. Both claims corrected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

mattmillerai · 2026-09-08T22:15:57Z

🤖 The reviews loop filed Linear follow-up ticket(s) for review thread(s) deferred as out of scope for this PR:

BE-12621 — Make prose continuation lines unambiguous in the prior-review ledger's line-oriented format — filed as agent-spike (premise unverified)
BE-12622 — Validate the judge's repeat_of against the ledger entries the judge was actually shown — filed as agent-spike (premise unverified)

The following carry agent-spike instead of agent-ok because their reachability claim was not backed by evidence (BE-5378) — the claim is investigated before any code is written, and "the premise does not hold" is a valid, successful outcome:

Make prose continuation lines unambiguous in the prior-review ledger's line-oriented format — no reachability block in the proposal
Validate the judge's repeat_of against the ledger entries the judge was actually shown — no reachability block in the proposal

mattmillerai added the agent-coded Authored by the agent-work loop label Sep 8, 2026

mattmillerai marked this pull request as ready for review September 8, 2026 18:32

cloud-code-bot Bot assigned sundar-svg Sep 8, 2026

mattmillerai added the cursor-review Multi-model cursor review label Sep 8, 2026

github-actions Bot reviewed Sep 8, 2026

View reviewed changes

mattmillerai merged commit 1d37fe8 into main Sep 8, 2026
5 checks passed

mattmillerai deleted the matt/be-12534-repeat-lineage branch September 8, 2026 22:28

Navigation Menu

Uh oh!

fix(cursor-review): carry re-raise lineage through the body-only sentinel - #273

fix(cursor-review): carry re-raise lineage through the body-only sentinel#273
mattmillerai merged 2 commits into
mainfrom
matt/be-12534-repeat-lineage

mattmillerai commented Sep 8, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Sep 8, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

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!

mattmillerai commented Sep 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

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

Uh oh!

ELI-5

What changed

Verification against the artifact the ticket names

Judgment calls

Residual

Provenance

Uh oh!

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

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

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!

mattmillerai commented Sep 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mattmillerai commented Sep 8, 2026 •

edited

Loading

coderabbitai Bot commented Sep 8, 2026 •

edited

Loading