Comfy-Org/github-workflows

Fork 0

mirror of https://github.com/Comfy-Org/github-workflows.git synced 2026-11-05 00:17:37 +00:00

[PR #234] [MERGED] feat(cursor-review): carry body-only findings into the prior-review ledger via an HTML sentinel #243

New issue

Closed

opened 2026-10-26 22:37:43 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:37:43 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/234
Author: @mattmillerai
Created: 8/27/2026
Status: ✅ Merged
Merged: 8/27/2026
Merged by: @mattmillerai

Base: main ← Head: matt/be-9565-body-only-ledger

📝 Commits (3)

08f8cf3 feat(cursor-review): carry body-only findings into the prior-review ledger via an HTML sentinel (BE-9565)
96f2908 fix(cursor-review): close the ten review findings on the body-only ledger sentinel (BE-9565)
40f0de4 fix(cursor-review): close round-2 review findings on the body-only ledger sentinel (BE-9565)

📊 Changes

4 files changed (+1568 additions, -30 deletions)

View changed files

📝 .github/cursor-review/build-ledger.py (+376 -18)
📝 .github/cursor-review/post-review.py (+222 -12)
📝 .github/cursor-review/tests/test_build_ledger.py (+653 -0)
📝 .github/cursor-review/tests/test_post_review.py (+317 -0)

📄 Description

ELI-5

When the review panel finds a problem on a line the reviewed diff doesn't actually carry, that finding can't be pinned to a line — so it gets written into the review's summary text instead of as an inline comment. The bit of code that remembers "what did earlier rounds already say?" only ever looked at inline comments, so those demoted findings vanished from its memory: they'd come back verbatim next round, and a round where every finding got demoted looked like a round that found nothing at all. This adds a small invisible HTML comment at the top of that summary section carrying the demoted findings as JSON, and teaches the memory-builder to read it back.

Description

build_ledger() derives its entries exclusively from review-comment thread roots (the pull_request_review_id in round_by_review and not in_reply_to_id filter); review bodies were only ever read for marker/round identification. A demoted finding has no comment, so it had no ledger entry — no repeat_of link next round, no REPEAT_CAP coverage, and a fully-demoted round rendered as a review that found nothing.

post-review.py — render_body_only_findings() now opens its section with , a compact array of {path, line, severity, body} for each demoted finding. Three properties are load-bearing:

It goes before the prose. clamp_review_body cuts the tail, so the machine-readable copy stays recoverable for as long as any of the section survives at all. A test drives ten 19,000-char findings through main() and asserts all ten are still parseable out of the clamped body.
Every - is escaped to \u002d after encoding. Finding bodies are model output derived from PR content, so one can contain --> and close the comment early — which would both spill JSON into the rendered review and hand the reader a truncated payload. Removing the character entirely means no -- run can exist inside the comment; a test asserts that for adversarial bodies and that the escape round-trips losslessly. Post-encode is the only place this works — escaping first would have json.dumps escape the backslash.
The path is run through neutralize_mentions. The body was already neutralized upstream, but path is raw model output until it is rendered, and this render is still a POSTed review body — an HTML comment is not a reliable place to hide an @handle. This was caught by an existing test, not by me reading the diff.

Bodies are truncated to the ledger's own 600-char cap with its truncation marker, since the ledger truncates to the same number and would otherwise no-op, leaving a cut body indistinguishable from a whole one.

build-ledger.py — parses the sentinel out of each consolidated review. Those are already gated on Bot author + CONSOLIDATED_MARKER + not DISMISSED, so the sentinel inherits the ledger's existing provenance control. The regex is pinned to the exact v1 prefix and anchored to a line start: every line of a demoted finding's rendered prose is blockquote-prefixed, so a sentinel a model wrote inside a finding body never sits at column 0 and can never win the match. Any parse failure — malformed JSON, a clamped tail, an unknown version — falls back to a disclosed note when the prose marker says findings were demoted, and to nothing when it doesn't. Never a silent empty round.

Recovered entries carry the same shape as thread entries plus "anchored": false, with answered_count: 0, replies: [] and an empty discussion_url. Thread-derived entries get "anchored": true. Both kinds go through the existing sort, round-trimming and byte-cap logic unchanged and count toward entry_count / unanswered_count.

Rendering — an unanchorable entry renders [unanchorable] after the severity, omits the discussion_url: line rather than blanking it (an empty field reads as one the judge could fill in), and replaces the never-answered trailer with an explicit statement that no thread exists. One line is added to the judge's REPEAT POLICY.

Policy: body-only entries are permanently unanswered, and therefore cap-exempt

This matches the existing invariant that only an answered finding consumes a repeat slot. No disposition is invented, no anchor is retargeted onto another line, and the panel/judge are told plainly that nobody could have answered — so a bare answered_count=0 isn't misread as an author who ignored the finding.

One judgment call outside the plan

ledger_note() previously said "prior-review ledger dropped for size" whenever entries were empty and any note existed. A degradation note is a new source of notes, so that sentence could now be printed when nothing was dropped for size. It now distinguishes the two using a structural unrecovered_rounds count rather than matching prose, and the pre-existing cap wording is unchanged for every case that could already reach it.

Verification of the negative claim this diff states

The diff asserts a demoted finding "cannot be answered or resolved". Rather than assume it, I attempted that capability through every surface the code offers, and all four fail:

Inline review comment — the POST payload's comments is built only from inline_items; driving main() with one anchorable and one demoted finding posts [('app.py', 11)] and nothing for the demoted one.
Replying to the review body as a normal PR comment — an OWNER-authored reply on the PR leaves the recovered entry at answered_count: 0 with replies: [], because the roots filter requires a pull_request_review_id.
Resolving the thread — gate-unresolved reads the GraphQL reviewThreads connection; with no review comment there is no thread node to resolve.
File-level (subject_type: file) comments, the one alternative that would restore an answer surface — grep -rl subject_type .github scripts returns nothing. It is not implemented anywhere in this repo, which is why it is named under Residual rather than used.

How has this been tested?

python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' — 341 tests, 0 failures (was 305 on the base commit; 36 added — 22 with the feature, 14 more closing the review panel's findings). Two pre-existing tests failed against the first draft and both caught real defects — the unneutralized path above, and the blockquote-containment guard, which is now scoped to skip the sentinel line while asserting the sentinel's own containment (its JSON escaping keeps a hostile body on one line) rather than being loosened.

Also run green: shellcheck -x .github/cursor-review/{install-cursor-cli,slack-notify}.sh (what the same CI job runs), python3 .github/agents-md-integrity/check_agents_md.py --root . (passed, 1 pre-existing CODEOWNERS warning), python3 .github/workflow-pins/check_workflow_pins.py (11 workflows, 0 issues).

The first-round no-regression property is pinned by the existing test_first_round_prompts_are_byte_identical and is still green — the empty / disabled / unknown paths are untouched.

Residual

The wholesale-fallback review body carries no sentinel — but it is no longer silent. Scope measured: exactly 2 of post-review.py's post paths deliver findings to a PR — the success path (fixed here) and the 422 wholesale fallback at the end of main(), which still contributes zero ledger entries. The other three post paths (the error review, the no-findings review, the read-only/POST-failed step-summary degradations) carry no findings, so no ledger entry is possible or expected from them.

The review panel caught that this section originally understated itself: the fallback's note also lacked BODY_ONLY_PROSE_MARKER, so next round _body_only_entries returned ([], False) — not just zero entries but no degradation either, on the one path where every finding is body-only. ledger_note then returned empty and the round after it read as a genuine first round. The note now carries the marker, so that round degrades loudly.

Rendering the sentinel there is still deliberately not done, for two reasons worth a human's judgment: the fallback body is already strictly larger than the request that just 422'd, so adding bytes makes a size rejection worse; and in that body every finding is in the prose, including ones that anchored fine and lost their anchor only to the 422 — whether those should become anchored: false ledger entries is a disposition question this change was explicitly told not to invent. Proposed as a follow-up rather than settled here.

File-level (subject_type: file) comment demotion is not implemented. This is the one alternative that would restore a real answer surface for findings whose path is in the diff even when the line is not, and it would make the "cannot be answered or resolved" wording this PR introduces obsolete for that subset. It was evaluated and set aside upstream of this change on two grounds (it cannot be batched into the create-review call, and it does not cover paths absent from the diff entirely), so it does not replace this work — but it remains the better long-term fix for part of the same problem, and nothing here forecloses it. Verified above that no implementation of it exists in this repo today.

~~The panel prompt was not given the [unanchorable] explanation.~~ Resolved by review. _PANEL_STEERING now defines the token, and its opening line no longer claims every entry was "already raised AND answered" — which was false for every body-only entry, so the panel was being told to weigh a reply that does not exist.

Nothing here was exercised against a live workflow run. All verification is unit-level with gh stubbed. The round-trip is proven end-to-end in-process (a real render_body_only_findings() output parsed by a real build_ledger()), but no actual cursor-review round was posted to a PR and re-read on a subsequent round — I cannot trigger one, and doing so would be a mutating action on a live PR. The first real confirmation will be the second review round on some PR after this merges.

Artifacts I could not read. This change was specified from a read-only investigation whose findings live in the issue tracker, which I have no access to; I worked from the specification as handed to me and could not consult that source material or verify that the specification still matches it. If its conclusions have moved since, this PR would not know.

Review round 1 — all ten panel findings addressed

The cursor-review panel raised 10 findings (2 High, 2 Medium, 6 Low). All are fixed; each thread carries a reply and is resolved. Two changed the design rather than patching it:

The sentinel's position was wrong, and the test that guarded it was wrong too. clamp_review_body cuts the tail, so a clamp landing inside the sentinel's JSON took the closing --> — and, with the prose marker rendered below it, the evidence that anything had been demoted went with it. A fully-demoted round read as a review that found nothing: the exact failure this PR exists to prevent, reachable through the one cut that actually happens. The original test missed it by splicing the tail back on, a shape the clamp never produces. The marker now leads the section and the sentinel sits under it, and the new test drives the real clamp at a limit computed to land mid-payload.

The sentinel search was not scoped to the section. It ran over every consolidated review body, and post_error_review fences unbounded judge/CLI text rather than blockquoting it — the one body whose imported lines do sit at column 0, where a forged sentinel would fabricate ledger entries with fully attacker-chosen path, severity and body. Closed on both sides: the reader requires the marker line directly above the sentinel, and the writer defangs both literals out of the error text with a zero-width space.

The rest, briefly:

path and severity are flattened to one line on the way in and _defang_fences'd on the way out. They land on the entry's header line, and a path carrying \n=== END PRIOR REVIEW LEDGER === is guaranteed to be demoted, since it can never anchor.
The 422 fallback body carries the prose marker, so that round degrades loudly (see Residual).
Degradation notes are scoped to the rounds max_rounds keeps — a cap-excluded round was reported as a defect, and those notes bypassed max_bytes.
ledger_note branches on a structural cap_notes count, not on the note count as a proxy, and reports unrecovered_rounds rather than total_rounds.
A sentinel payload whose every item is the wrong shape returns None, not [] — [] read as "parsed, zero findings" and suppressed the note.
_body_only_line uses isdecimal() plus a guarded int() (isdigit() is True for '²'; a long decimal string still raises past sys.get_int_max_str_digits()), and rejects non-positive lines.
A demoted re-raise no longer carries ↩︎ re-raise of <url> inside the sentinel body — the entries most likely to carry a live thread URL were exactly the ones the ledger omits discussion_url: for.
_PANEL_STEERING explains [unanchorable] and no longer claims every entry was answered.

One reviewer sub-claim did not reproduce and was left unchanged, with a test pinning it instead: an unwrapped dict body was said to raise in _truncate, but _truncate already does str()-coerce, so a str() at the call site would be a no-op.

Review round 2 — all eight panel findings addressed

Two changed a control rather than patching it.

The fallback body's disclosure was in the wrong place, and the test that guarded it pinned the wrong shape. Round 1 added the prose marker to the 422 fallback but appended it at the TAIL — and clamp_review_body cuts the tail, so the marker was the FIRST thing any clamp took. On the one path where EVERY finding is body-only, over MAX_REVIEW_BODY_CHARS, the silent round came straight back. Reachable, too: that body carries every finding at full length with no count cap on the un-adjudicated panel path. The note now rides in review_head, which is finding-independent and bounded, so it sits within a few hundred chars of the top. The new test drives the real clamp and asserts the marker is near the HEAD, not merely present.

An unterminated , so a cut inside the sentinel's JSON did not merely lose the sentinel — GitHub swallowed the remainder of the posted body, including clamp_review_body's own "as much of it as fits is in the job summary" note, and the review rendered as a header with no findings and no explanation. The panel proposed a byte budget for the sentinel; that cannot actually promise this, because whether the sentinel is cut depends on how much body precedes it and the renderer does not know its own offset. drop_unterminated_comment runs inside the clamp instead — the one place that knows where the cut lands — so the swallow is impossible for any HTML comment in any posted body.

The rest, briefly:

The fence defang and the field flatten now share one line-separator set — the full str.splitlines() vocabulary. re.MULTILINE's ^ follows only \n, so a fence introduced by a bare CR (or U+2028/U+2029/U+0085/\v/\f/\x1c-\x1e) sat mid-"line" for the regex and at column 0 for everything downstream. This also fixed a second-order bug nobody named: . matches \r, so the old pattern swallowed the text after a bare CR into the fence line it rewrote.
The sentinel opener is one exact literal. Whitespace-run tolerance meant …body-only-findings\tv1 satisfied the reader while carrying none of the literal the writer defangs — the two-coupled-lines control was down to the prose-marker half alone.
The reader refuses the error-review shape. The writer-side defang only protects bodies this version wrote; consumers stay pinned to older SHAs, so their already-posted error reviews are undefanged and no writer-side change can reach them.
json.loads' RecursionError is caught — a RuntimeError, not a ValueError, so it escaped into cmd_build's blanket except and cost the whole ledger rather than one round.
"Never answered" now counts only anchored entries, with unanchorable_count reported separately, in both render sites.
A malformed sentinel line renders ? rather than None.

One fix of mine was wrong on the first attempt, and the diff records why. The error-review refusal started as a bare substring test. That silently drops a legitimate round whose demoted finding merely quotes the heading — a panel finding about post_error_review, which the reviews of this very PR produced — losing its real entries AND its degradation note. That is precisely the silent round this whole mechanism exists to remove. _ERROR_REVIEW_RE is anchored to a line start, reusing the blockquote-containment argument the sentinel already rests on, and test_a_finding_that_merely_quotes_the_error_heading_is_still_recovered pins it.

Every one of the 19 added tests was run against the pre-fix tree and confirmed to fail there. One of them did not, at first — the defang-invariant test put its whitespace after v1 rather than before it, where the defang does still bite; it passed pre-fix and was rewritten to probe the gap the reviewer actually named.

Provenance

Authored by: agent-work loop
Verified: python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py': 360 passed, 0 failed (was 341 before this round; 19 added, each confirmed failing against the pre-fix tree); shellcheck -x .github/cursor-review/{install-cursor-cli,slack-notify}.sh: clean — together these are exactly what test-cursor-review-scripts.yml runs; -s .github/agents-md-integrity/tests: 46 passed; check_agents_md.py --root .: passed (1 pre-existing CODEOWNERS warning); check_workflow_pins.py: 11 workflows, 0 issues
Deviations: one, stated in full above. The panel asked for a byte budget on the sentinel; a budget cannot promise the property it was asked to promise, since the renderer does not know its own offset in the final body, so the guard went into clamp_review_body instead — a strictly stronger fix covering every HTML comment in every posted body. Everything else follows the findings as raised. The error-review refusal is matched at a line start rather than as a substring, for the reason recorded above.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/234 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/27/2026 **Status:** ✅ Merged **Merged:** 8/27/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-9565-body-only-ledger` --- ### 📝 Commits (3) - [`08f8cf3`](https://github.com/Comfy-Org/github-workflows/commit/08f8cf38b0a3d9a5184fa37cc9bd0d3d866943c9) feat(cursor-review): carry body-only findings into the prior-review ledger via an HTML sentinel (BE-9565) - [`96f2908`](https://github.com/Comfy-Org/github-workflows/commit/96f2908caf2cdf450b5d17f8b8282e830151a5ac) fix(cursor-review): close the ten review findings on the body-only ledger sentinel (BE-9565) - [`40f0de4`](https://github.com/Comfy-Org/github-workflows/commit/40f0de4d8aaf360655e7380eb12155f92258edd6) fix(cursor-review): close round-2 review findings on the body-only ledger sentinel (BE-9565) ### 📊 Changes **4 files changed** (+1568 additions, -30 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/build-ledger.py` (+376 -18) 📝 `.github/cursor-review/post-review.py` (+222 -12) 📝 `.github/cursor-review/tests/test_build_ledger.py` (+653 -0) 📝 `.github/cursor-review/tests/test_post_review.py` (+317 -0) </details> ### 📄 Description ## ELI-5 When the review panel finds a problem on a line the reviewed diff doesn't actually carry, that finding can't be pinned to a line — so it gets written into the review's summary text instead of as an inline comment. The bit of code that remembers "what did earlier rounds already say?" only ever looked at inline comments, so those demoted findings vanished from its memory: they'd come back verbatim next round, and a round where *every* finding got demoted looked like a round that found nothing at all. This adds a small invisible HTML comment at the top of that summary section carrying the demoted findings as JSON, and teaches the memory-builder to read it back. ## Description `build_ledger()` derives its entries exclusively from review-**comment** thread roots (the `pull_request_review_id in round_by_review and not in_reply_to_id` filter); review bodies were only ever read for marker/round identification. A demoted finding has no comment, so it had no ledger entry — no `repeat_of` link next round, no `REPEAT_CAP` coverage, and a fully-demoted round rendered as a review that found nothing. **`post-review.py`** — `render_body_only_findings()` now opens its section with ``, a compact array of `{path, line, severity, body}` for each demoted finding. Three properties are load-bearing: - **It goes before the prose.** `clamp_review_body` cuts the *tail*, so the machine-readable copy stays recoverable for as long as any of the section survives at all. A test drives ten 19,000-char findings through `main()` and asserts all ten are still parseable out of the clamped body. - **Every `-` is escaped to `\u002d` after encoding.** Finding bodies are model output derived from PR content, so one can contain `-->` and close the comment early — which would both spill JSON into the rendered review and hand the reader a truncated payload. Removing the character entirely means no `--` run can exist inside the comment; a test asserts that for adversarial bodies and that the escape round-trips losslessly. Post-encode is the only place this works — escaping first would have `json.dumps` escape the backslash. - **The path is run through `neutralize_mentions`.** The body was already neutralized upstream, but `path` is raw model output until it is rendered, and this render is still a POSTed review body — an HTML comment is not a reliable place to hide an `@handle`. This was caught by an existing test, not by me reading the diff. Bodies are truncated to the ledger's own 600-char cap **with its truncation marker**, since the ledger truncates to the same number and would otherwise no-op, leaving a cut body indistinguishable from a whole one. **`build-ledger.py`** — parses the sentinel out of each consolidated review. Those are already gated on Bot author + `CONSOLIDATED_MARKER` + not `DISMISSED`, so the sentinel inherits the ledger's existing provenance control. The regex is pinned to the exact `v1` prefix and **anchored to a line start**: every line of a demoted finding's rendered prose is blockquote-prefixed, so a sentinel a model wrote *inside* a finding body never sits at column 0 and can never win the match. Any parse failure — malformed JSON, a clamped tail, an unknown version — falls back to a disclosed note when the prose marker says findings *were* demoted, and to nothing when it doesn't. Never a silent empty round. Recovered entries carry the same shape as thread entries plus `"anchored": false`, with `answered_count: 0`, `replies: []` and an empty `discussion_url`. Thread-derived entries get `"anchored": true`. Both kinds go through the existing sort, round-trimming and byte-cap logic unchanged and count toward `entry_count` / `unanswered_count`. **Rendering** — an unanchorable entry renders `[unanchorable]` after the severity, **omits** the `discussion_url:` line rather than blanking it (an empty field reads as one the judge could fill in), and replaces the never-answered trailer with an explicit statement that no thread exists. One line is added to the judge's REPEAT POLICY. ### Policy: body-only entries are permanently unanswered, and therefore cap-exempt This matches the existing invariant that only an **answered** finding consumes a repeat slot. No disposition is invented, no anchor is retargeted onto another line, and the panel/judge are told plainly that nobody *could* have answered — so a bare `answered_count=0` isn't misread as an author who ignored the finding. ### One judgment call outside the plan `ledger_note()` previously said "prior-review ledger dropped for size" whenever entries were empty and any note existed. A degradation note is a *new* source of notes, so that sentence could now be printed when nothing was dropped for size. It now distinguishes the two using a structural `unrecovered_rounds` count rather than matching prose, and **the pre-existing cap wording is unchanged for every case that could already reach it**. ### Verification of the negative claim this diff states The diff asserts a demoted finding "cannot be answered or resolved". Rather than assume it, I attempted that capability through every surface the code offers, and all four fail: 1. **Inline review comment** — the POST payload's `comments` is built only from `inline_items`; driving `main()` with one anchorable and one demoted finding posts `[('app.py', 11)]` and nothing for the demoted one. 2. **Replying to the review body as a normal PR comment** — an `OWNER`-authored reply on the PR leaves the recovered entry at `answered_count: 0` with `replies: []`, because the roots filter requires a `pull_request_review_id`. 3. **Resolving the thread** — `gate-unresolved` reads the GraphQL `reviewThreads` connection; with no review comment there is no thread node to resolve. 4. **File-level (`subject_type: file`) comments**, the one alternative that *would* restore an answer surface — `grep -rl subject_type .github scripts` returns nothing. It is not implemented anywhere in this repo, which is why it is named under Residual rather than used. ## How has this been tested? `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'` — **341 tests, 0 failures** (was 305 on the base commit; 36 added — 22 with the feature, 14 more closing the review panel's findings). Two pre-existing tests failed against the first draft and both caught real defects — the unneutralized path above, and the blockquote-containment guard, which is now scoped to skip the sentinel line while asserting the sentinel's own containment (its JSON escaping keeps a hostile body on one line) rather than being loosened. Also run green: `shellcheck -x .github/cursor-review/{install-cursor-cli,slack-notify}.sh` (what the same CI job runs), `python3 .github/agents-md-integrity/check_agents_md.py --root .` (passed, 1 pre-existing CODEOWNERS warning), `python3 .github/workflow-pins/check_workflow_pins.py` (11 workflows, 0 issues). The first-round no-regression property is pinned by the existing `test_first_round_prompts_are_byte_identical` and is still green — the empty / disabled / unknown paths are untouched. ## Residual **The wholesale-fallback review body carries no sentinel — but it is no longer silent.** Scope measured: exactly **2** of `post-review.py`'s post paths deliver findings to a PR — the success path (fixed here) and the 422 wholesale fallback at the end of `main()`, which still contributes zero ledger entries. The other three post paths (the error review, the no-findings review, the read-only/POST-failed step-summary degradations) carry no findings, so no ledger entry is possible or expected from them. The review panel caught that this section originally understated itself: the fallback's note also lacked `BODY_ONLY_PROSE_MARKER`, so next round `_body_only_entries` returned `([], False)` — not just zero entries but *no degradation either*, on the one path where every finding is body-only. `ledger_note` then returned empty and the round after it read as a genuine first round. The note now carries the marker, so that round degrades loudly. Rendering the sentinel there is still deliberately not done, for two reasons worth a human's judgment: the fallback body is already strictly larger than the request that just 422'd, so adding bytes makes a size rejection worse; and in that body *every* finding is in the prose, including ones that anchored fine and lost their anchor only to the 422 — whether those should become `anchored: false` ledger entries is a disposition question this change was explicitly told not to invent. Proposed as a follow-up rather than settled here. **File-level (`subject_type: file`) comment demotion is not implemented.** This is the one alternative that would restore a real answer surface for findings whose *path* is in the diff even when the *line* is not, and it would make the "cannot be answered or resolved" wording this PR introduces obsolete for that subset. It was evaluated and set aside upstream of this change on two grounds (it cannot be batched into the `create-review` call, and it does not cover paths absent from the diff entirely), so it does not replace this work — but it remains the better long-term fix for part of the same problem, and nothing here forecloses it. Verified above that no implementation of it exists in this repo today. ~~**The panel prompt was not given the `[unanchorable]` explanation.**~~ **Resolved by review.** `_PANEL_STEERING` now defines the token, and its opening line no longer claims every entry was "already raised AND answered" — which was false for every body-only entry, so the panel was being told to weigh a reply that does not exist. **Nothing here was exercised against a live workflow run.** All verification is unit-level with `gh` stubbed. The round-trip is proven end-to-end *in-process* (a real `render_body_only_findings()` output parsed by a real `build_ledger()`), but no actual cursor-review round was posted to a PR and re-read on a subsequent round — I cannot trigger one, and doing so would be a mutating action on a live PR. The first real confirmation will be the second review round on some PR after this merges. **Artifacts I could not read.** This change was specified from a read-only investigation whose findings live in the issue tracker, which I have no access to; I worked from the specification as handed to me and could not consult that source material or verify that the specification still matches it. If its conclusions have moved since, this PR would not know. ## Review round 1 — all ten panel findings addressed The cursor-review panel raised 10 findings (2 High, 2 Medium, 6 Low). All are fixed; each thread carries a reply and is resolved. Two changed the design rather than patching it: **The sentinel's position was wrong, and the test that guarded it was wrong too.** `clamp_review_body` cuts the *tail*, so a clamp landing inside the sentinel's JSON took the closing `-->` — and, with the prose marker rendered *below* it, the evidence that anything had been demoted went with it. A fully-demoted round read as a review that found nothing: the exact failure this PR exists to prevent, reachable through the one cut that actually happens. The original test missed it by splicing the tail back on, a shape the clamp never produces. **The marker now leads the section and the sentinel sits under it**, and the new test drives the real clamp at a limit computed to land mid-payload. **The sentinel search was not scoped to the section.** It ran over every consolidated review body, and `post_error_review` fences unbounded judge/CLI text rather than blockquoting it — the one body whose imported lines *do* sit at column 0, where a forged sentinel would fabricate ledger entries with fully attacker-chosen `path`, `severity` and `body`. Closed on both sides: the reader requires the marker line directly above the sentinel, and the writer defangs both literals out of the error text with a zero-width space. The rest, briefly: - `path` and `severity` are flattened to one line on the way in and `_defang_fences`'d on the way out. They land on the entry's *header* line, and a path carrying `\n=== END PRIOR REVIEW LEDGER ===` is guaranteed to be demoted, since it can never anchor. - The 422 fallback body carries the prose marker, so that round degrades loudly (see Residual). - Degradation notes are scoped to the rounds `max_rounds` keeps — a cap-excluded round was reported as a defect, and those notes bypassed `max_bytes`. - `ledger_note` branches on a structural `cap_notes` count, not on the note *count* as a proxy, and reports `unrecovered_rounds` rather than `total_rounds`. - A sentinel payload whose every item is the wrong shape returns `None`, not `[]` — `[]` read as "parsed, zero findings" and suppressed the note. - `_body_only_line` uses `isdecimal()` *plus* a guarded `int()` (`isdigit()` is True for `'²'`; a long decimal string still raises past `sys.get_int_max_str_digits()`), and rejects non-positive lines. - A demoted re-raise no longer carries `↩︎ re-raise of <url>` inside the sentinel body — the entries most likely to carry a live thread URL were exactly the ones the ledger omits `discussion_url:` for. - `_PANEL_STEERING` explains `[unanchorable]` and no longer claims every entry was answered. One reviewer sub-claim did not reproduce and was left unchanged, with a test pinning it instead: an unwrapped dict `body` was said to raise in `_truncate`, but `_truncate` already does `str()`-coerce, so a `str()` at the call site would be a no-op. ## Review round 2 — all eight panel findings addressed Two changed a control rather than patching it. **The fallback body's disclosure was in the wrong place, and the test that guarded it pinned the wrong shape.** Round 1 added the prose marker to the 422 fallback but appended it at the TAIL — and `clamp_review_body` cuts the tail, so the marker was the FIRST thing any clamp took. On the one path where EVERY finding is body-only, over `MAX_REVIEW_BODY_CHARS`, the silent round came straight back. Reachable, too: that body carries every finding at full length with no count cap on the un-adjudicated panel path. The note now rides in `review_head`, which is finding-independent and bounded, so it sits within a few hundred chars of the top. The new test drives the real clamp and asserts the marker is near the HEAD, not merely present. **An unterminated ``, so a cut inside the sentinel's JSON did not merely lose the sentinel — GitHub swallowed the remainder of the posted body, including `clamp_review_body`'s own "as much of it as fits is in the job summary" note, and the review rendered as a header with no findings and no explanation. The panel proposed a byte budget for the sentinel; that cannot actually promise this, because whether the sentinel is cut depends on how much body precedes it and the renderer does not know its own offset. `drop_unterminated_comment` runs inside the clamp instead — the one place that knows where the cut lands — so the swallow is impossible for any HTML comment in any posted body. The rest, briefly: - **The fence defang and the field flatten now share one line-separator set** — the full `str.splitlines()` vocabulary. `re.MULTILINE`'s `^` follows only `\n`, so a fence introduced by a bare CR (or U+2028/U+2029/U+0085/`\v`/`\f`/`\x1c`-`\x1e`) sat mid-"line" for the regex and at column 0 for everything downstream. This also fixed a second-order bug nobody named: `.` matches `\r`, so the old pattern swallowed the text *after* a bare CR into the fence line it rewrote. - **The sentinel opener is one exact literal.** Whitespace-run tolerance meant `…body-only-findings\tv1` satisfied the reader while carrying none of the literal the writer defangs — the two-coupled-lines control was down to the prose-marker half alone. - **The reader refuses the error-review shape.** The writer-side defang only protects bodies this version wrote; consumers stay pinned to older SHAs, so their already-posted error reviews are undefanged and no writer-side change can reach them. - `json.loads`' `RecursionError` is caught — a `RuntimeError`, not a `ValueError`, so it escaped into `cmd_build`'s blanket except and cost the whole ledger rather than one round. - "Never answered" now counts only anchored entries, with `unanchorable_count` reported separately, in both render sites. - A malformed sentinel `line` renders `?` rather than `None`. **One fix of mine was wrong on the first attempt, and the diff records why.** The error-review refusal started as a bare substring test. That silently drops a legitimate round whose demoted finding merely *quotes* the heading — a panel finding about `post_error_review`, which the reviews of this very PR produced — losing its real entries AND its degradation note. That is precisely the silent round this whole mechanism exists to remove. `_ERROR_REVIEW_RE` is anchored to a line start, reusing the blockquote-containment argument the sentinel already rests on, and `test_a_finding_that_merely_quotes_the_error_heading_is_still_recovered` pins it. Every one of the 19 added tests was run against the pre-fix tree and confirmed to fail there. One of them did not, at first — the defang-invariant test put its whitespace after `v1` rather than before it, where the defang does still bite; it passed pre-fix and was rewritten to probe the gap the reviewer actually named. ## Provenance - **Authored by:** agent-work loop - **Verified:** `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'`: 360 passed, 0 failed (was 341 before this round; 19 added, each confirmed failing against the pre-fix tree); `shellcheck -x .github/cursor-review/{install-cursor-cli,slack-notify}.sh`: clean — together these are exactly what `test-cursor-review-scripts.yml` runs; `-s .github/agents-md-integrity/tests`: 46 passed; `check_agents_md.py --root .`: passed (1 pre-existing CODEOWNERS warning); `check_workflow_pins.py`: 11 workflows, 0 issues - **Deviations:** one, stated in full above. The panel asked for a byte budget on the sentinel; a budget cannot promise the property it was asked to promise, since the renderer does not know its own offset in the final body, so the guard went into `clamp_review_body` instead — a strictly stronger fix covering every HTML comment in every posted body. Everything else follows the findings as raised. The error-review refusal is matched at a line start rather than as a substring, for the reason recorded above. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:37:43 +00:00