[PR #274] [MERGED] fix(cursor-review): budget the success path's demoted-findings sentinel so the clamp never cuts inside it #277

New issue

Closed

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

bsakaguchi commented

2026-10-26 22:38:00 +00:00

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-12535-success-path-sentinel-budget

📝 Commits (6)

7b5579b fix(cursor-review): budget the success path's demoted-findings sentinel (BE-12535)
4787286 fix(cursor-review): make a budget-truncated sentinel disclose its own loss (BE-12535)
2d9eac1 fix(cursor-review): neutralize planted comment openers and stop the ceiling overcharging (BE-12535)
b570aab Merge remote-tracking branch 'origin/main' into matt/be-12535-success-path-sentinel-budget
b1fdb78 test(cursor-review): pin the companion's line anchor at the reader, not via the writer's defang (BE-12535)
bc133ee fix(cursor-review): spell the defang's zero-width space as an escape (BE-12535)

📊 Changes

4 files changed (+944 additions, -55 deletions)

View changed files

📝 .github/cursor-review/build-ledger.py (+66 -3)
📝 .github/cursor-review/post-review.py (+320 -50)
📝 .github/cursor-review/tests/test_build_ledger.py (+98 -0)
📝 .github/cursor-review/tests/test_post_review.py (+460 -2)

📄 Description

ELI-5

When a review round has too many findings to anchor to lines in the diff, post-review.py lists them in the review body and hides a machine-readable copy of them in an HTML comment (the "sentinel") so the next round's ledger can read them back. That sentinel is nearly as long as the prose it duplicates, so it competes with the findings a human actually sees — and on the success path it was allowed to take the whole body. Past a certain number of findings it overran the size limit, GitHub's clamp cut it mid-JSON, the unterminated-comment backstop rewound it to its opener and swallowed every finding below it, and the review posted a 420-character header with nothing in it. The 422 fallback path already had a guard for exactly this; this gives the success path the same one.

What changed

FALLBACK_SENTINEL_MAX_CHARS → SENTINEL_MAX_CHARS (same value, MAX_REVIEW_BODY_CHARS // 2), now documented as the prose floor on both finding-carrying paths rather than the fallback's alone. git grep found no reader outside this repo, so no back-compat alias was added; the one stale mention was a test docstring, updated in place.
render_body_only_findings(items, budget=None). budget is how many characters the whole section may occupy before the clamp's cut point. None keeps today's behaviour byte-identical (the round-trip tests in test_post_review.py / test_build_ledger.py all call it unbudgeted and pin that). With a budget it first checks whether the whole section fits — if it does, nothing is charged, because the clamp will not cut it — and otherwise hands sentinel_share(available, len(prose)) to the existing fit_sentinel_items, keeping the most-urgent prefix that fits — or dropping the sentinel entirely and posting the marker alone. Both outcomes print the same kind of stderr disclosure the fallback path already prints. The prose loop still renders every item either way: the budget governs what the ledger recovers, never what a reader is shown.
main() passes budget=MAX_REVIEW_BODY_CHARS - len(CLAMP_TRUNCATION_NOTE) - len(review_head) - len(FINDINGS_SEPARATOR), computed where review_head has actually been measured. The posted body is review_head + FINDINGS_SEPARATOR + marker + sentinel + "\n\n" + prose and the clamp cuts at limit - len(note), so the sentinel's closing --> now always sits ahead of the cut.
drop_unterminated_comment is scoped to CommonMark's HTML-block start condition, and its docstring no longer claims a render-time budget "cannot promise this" — both paths now derive one from the measured head, so it is reworded as the backstop for any HTML comment in any posted body (a miscounted head, a comment some future path adds) rather than as the primary guard.
.github/cursor-review/README.md needed no change: grep -ni "sentinel\|size guard\|body-only" returns nothing there, so the size guard is not described at that level.

Evidence

Reproduced on the base commit through the repo's stubbed-gh harness — one anchorable finding plus n demoted findings of ~700 chars, reading the success-path payload (posted[0]):

n	before (len / ledger entries / visible findings)	after
60	60,000 / 45 / 27	60,000 / 45 / 40
90	60,000 / 45 / 1	60,000 / 45 / 40
91	420 / 0 / 0 (`unrecovered_rounds=1`)	60,000 / 45 / 40
100	422 / 0 / 0	60,000 / 45 / 40
130	422 / 0 / 0	60,000 / 45 / 40

Then swept the shape space rather than only the shapes being fixed: 42 combinations of body length (100–3,000 chars) × finding count (20–200), before vs after. 19 unchanged, 23 changed, 0 regressed on either ledger entries or visible findings. Ten of the changed cells were the total collapse above (0 entries, 0 visible, a 422-char body), now carrying 45–73 ledger entries and 10–68 visible findings; the other 13 keep the same entry count and show a reader more findings (e.g. 700-char bodies at n=80: 45 entries either way, visible findings 10 → 41). The only shapes where the new cap could cost ledger entries are ones where the sentinel exceeds half the limit while the prose does not, and none exist in the swept range because a sentinel entry is always shorter than the prose entry it duplicates.

Edge cases checked by hand beyond the suite: a review head larger than the whole limit drives budget negative — fit_sentinel_items already guards budget <= 0, so it degrades to the marker-alone branch and the clamp takes it from there, with no traceback.

This change denies no capability and adds no dead-end path: at every measured shape the new code posts strictly more recoverable findings and strictly more visible findings than the old code, never fewer. The two new stderr messages are degradation disclosures on a path that previously degraded silently.

Tests

Five added next to the fallback's equivalents in BodyBudgetTest: the success-path prose floor across n ∈ {60, 90, 91, 100, 130}; the clamp-boundary sweep across a padded path; the most-urgent-prefix assertion (reading the sentinel payload, not the ledger — build_ledger re-sorts its entries, so the write order the test is about only survives in the payload); budget=None equivalence; and the marker-alone floor. Every existing test named in the plan stays green unchanged, including test_the_sentinel_survives_the_clamp_that_cuts_the_prose, test_the_marker_still_outlives_a_cut_that_takes_the_whole_sentinel, test_the_sentinel_follows_the_marker_precedes_the_findings_and_round_trips, and the fallback prose-floor / reserve tests.

Mutation check as specified: removing budget= from the main() call fails all 11 subtests of the two new success-path tests (including every n ≥ 91) plus the prefix test, then restored to green.

Judgment calls

No FALLBACK_SENTINEL_MAX_CHARS alias. git grep across the repo found the old name only in one test docstring; nothing outside this repo imports post-review.py (the scripts are loaded at run time by workflows in this repo, never vendored), so an alias would be dead code. Renaming it outright is the reversible choice.
budget: int | None is the file's first PEP 604 union. Valid on 3.10+; CI pins 3.12 and cursor-review.yml runs on ubuntu-latest's system python3 with no setup-python, so there is no interpreter here older than that.
budget=None default rather than making the argument required. Three test modules and the round-trip suite call this function unbudgeted and pin its exact output; a required argument would have forced churn in tests that are deliberately pinning the unbudgeted render.

Residual

The panel-size interaction is now tuned rather than merely bounded — see the review round below — but nothing caps review_head itself on either path. Past ~59,660 characters of head the clamp cuts inside the head, and the whole findings section (marker, sentinel, companion and prose alike) lands past the cut no matter what this code emits. Not reachable from today's inputs: every head contributor is a short workflow-generated string, and it took a synthetic 54,000-character model name to reach that regime in the new sweep test. Worth a ticket only if a caller ever ships a head that large.
A related in-progress change adds a repeat_of key to the sentinel payload. It does not conflict — fit_sentinel_items measures the real render, so the budget stays correct whichever lands first — but if that merges first this branch should be rebased rather than re-derived, since the payload's per-entry length changes.
Duplicate check incomplete. The originating investigation flagged that a residual ticket covering this same item may already exist, filed minutes apart from the change that introduced the fallback's guard. I hold no issue-tracker access from this environment and could not read either side to confirm, so I could not rule out a duplicate; if one exists, one should be closed against the other rather than both being built. The change itself is verified against the code on main regardless.
Unexercised artifacts. The originating investigation's transcript log and its findings comment are named in the source material but live outside this repo and were not readable from here; everything acted on is the reproduction re-derived independently against main at 52b4fe6 and reported above. The cursor-review.yml judge-failed raw-panel branch — the only route that produces an uncapped finding union in production — was exercised only through post-review.py's own harness, not by running the workflow end to end, which would require a live PR and the review panel's model spend.

Review round (`4787286`)

The panel raised five findings on 7b5579b. Four were valid and are fixed; the fifth is answered on its thread. Every fix is pinned by a test verified to fail against the pre-fix code and pass after.

🟡 A budget-truncated sentinel silently lost findings (4/6 reviewers). A prefix payload is still valid JSON, so build-ledger.py read 12-of-89 as a complete recovery — strictly worse than the all-or-nothing rule the budget replaced, which degraded loudly because a dropped sentinel does not parse. The writer now emits  on a real cut, and _body_only_entries returns entries and degraded independently, so a truncated round both contributes what it recovered and discloses what it did not. A second comment rather than a payload key, because the reader pins the sentinel to a single-spaced opener directly below the prose marker: anything inserted between them breaks the recovery it annotates, and an older pinned reader ignores an unknown trailing comment instead of failing to parse. Applied on both budgeted paths, defanged in quoted text alongside the other two literals, and the suffix is deliberately unpinned so an unexpected count shape still discloses.
🟡 The prose floor did not hold at every head size (2/6). min(SENTINEL_MAX_CHARS, available) bought the ceiling only while the first term won; past a head of roughly half the limit the min stopped binding. Confirmed by reverting the one expression: the new sweep fails at head shares 0.5/0.6/0.75/0.9 with one visible finding. Now sentinel_share(available, prose_len) = min(SENTINEL_MAX_CHARS, max(available - prose_len, available // 2)) — one step past the reviewer's flat half, so a big-head round with small prose is not charged a floor nothing was going to use. Line 1681 got the same treatment, as flagged.
🟢 The 30k cap bound unconditionally (2/6). A section that fits its budget now pays no budget at all. Reachable exactly through the escaping the reviewer identified: 400 findings on a hyphen-rich path push the render past SENTINEL_MAX_CHARS on - → \u002d alone while the whole body stays well under the limit. The 422 fallback gets the same skip.
🟢 drop_unterminated_comment rewound too far (1/6). Scoped the rewind to CommonMark's HTML-block start condition, (?:\A|(?<=\n)|(?<=\r)) {0,3}<!-- (bare \r explicitly, since cmark-gfm ends a line on one and re.MULTILINE does not). The reasoning behind this one was wrong and round 2 overturned it — I argued a <!-- inside a finding's blockquote could not swallow anything because the > prefix indents it out of the start condition. cmark-gfm strips the blockquote marker before parsing the contents, so it does. Superseded below; the note is kept rather than edited away because the round-2 fix only makes sense against it.
🟢 The marker is emitted without checking it fits (1/6) — answered, not changed. For budget to fall under len(marker), review_head must exceed ~59,660 of 60,000 characters, at which point the clamp is already cutting inside the head and every byte of this section is past the cut regardless. Only bounding the head would help, and the head is not this function to bound. Recorded as an in-code concern rather than a follow-up ticket, since it is not reachable under current code.

Review round 2 (`b1fdb78`)

Five more findings on 4787286. All five were valid; all five are fixed, plus one bug the fuzz written for the third turned up. Each fix is pinned by a test verified to fail against the pre-fix code and pass after.

Two of them turned on how GitHub actually renders a body, so I stopped reasoning about CommonMark and measured it — POST /markdown with mode: gfm, which is the same renderer the PR page uses.

🟡 A blockquoted  does not help either — that one is escaped to --> and closes nothing — so the opener has to die at the writer. render_finding_entry now neutralizes every <!-- in a rendered entry with a zero-width space (DEFANGED_COMMENT_OPENER), the same trick and house style as defang_body_only_contract; the defanged form renders as <!-- and swallows nothing, and mid-line openers were already escaped by cmark-gfm, so nothing a human reads changes.
🟡 The truncation companion had no line anchor (6/6). Unanimous, and correct in its diagnosis of why the comment defending it did not hold: defang_body_only_contract is called only from post_error_review, which _body_only_entries refuses outright, so the writer-side defang gave this line zero coverage on the success and 422-fallback bodies where it is actually read. Unanchored, the literal was plantable from the PR under review and flipped a fully recovered round to degraded, fabricating an unrecovered_rounds entry and a "may repeat" warning in the next prompt. _BODY_ONLY_TRUNCATED_RE is now line-anchored with the same \r-aware lookbehind _ERROR_REVIEW_RE uses, and _body_only_truncated additionally requires it to sit below the parsed sentinel — taking the reviewer's "and ideally". Both are properties of what the writer emits byte-for-byte, so neither can turn a disclosed loss back into a silent one.
🟢 A <!-- inside a fence is not an opener (2/6). Confirmed against the same endpoint: prose after the fence and the clamp note both survive it. It lands on the error path, where the fenced judge/CLI text is unbounded, so the over-deletion would have been large. html_block_openers now walks the cut a line at a time tracking fence state and returns only openers outside a fence, with character- and length-correct fence matching. On the parenthetical: a fence the cut left open is now detected but deliberately still not closed — closing it means appending characters past a cut point whose slack is already spent on CLAMP_TRUNCATION_NOTE, trading a cosmetic problem (the note renders as a last line of code rather than prose) for a 422.
🟢 The fits-check measured against the cut point, not the limit (4/6, a re-raise). Right: budget has already subtracted CLAMP_TRUNCATION_NOTE, while clamp_review_body leaves any body up to MAX_REVIEW_BODY_CHARS untouched, so a section in the ~140-char window between them was truncated and flagged degraded with no clamp behind it. Now len(whole) <= budget + len(CLAMP_TRUNCATION_NOTE), which is exactly the fallback's whole_fallback_len <= MAX_REVIEW_BODY_CHARS. Checked at the edge: sized so len(whole) equals budget + len(note) to the character, the section is posted whole and the body lands on 60,000 exactly, so the widened check cannot itself produce a 422.
🟢 The ceiling was charged where the prose wanted no room (same thread). Also right, and the reviewer's reasoning is the fix: available // 2 alone already guarantees the prose min(prose_len, ceil(available/2)) — everything it wants, up to half — at every head size, so capping the whole max only ever reserved space nothing would use. SENTINEL_MAX_CHARS now bounds the floor, not the leftover: max(available - prose_len, min(SENTINEL_MAX_CHARS, available // 2)). Their numbers reproduce — sentinel_share(59_000, 17_000) was 30,000 and is now 42,000, roughly a hundred ledger entries recovered, with the prose still getting all 17,000 it asked for.
🟢 Found while fuzzing the above: the rewind went to the last opener, so a body with two block-level openers where the first was the dangling one had the wrong one removed and the real one left standing — the same misdirection as the round-1 blockquote case, in a container no scoping covers. An HTML block runs to its first -->, so a later <!-- is already inside the earlier comment and is not an opener at all. It now rewinds to the first unterminated opener.

Round 1's fifth finding (the marker-fits check) stands answered on its thread and is unchanged: it is still unreachable without a ~59,660-character head, at which point the clamp is cutting inside the head and this section is past the cut whatever it emits.

Provenance

Authored by: agent-work loop
Verified: at b1fdb78 (which merges origin/main at 1d37fe8 into the branch — see below): python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py': 484 passed, 0 failed; .github/groom/tests: 374 passed, 1 skipped; .github/agents-md-integrity/tests: 46 passed; shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh: clean; check_agents_md.py --root .: passed with the same 2 pre-existing warnings (AGENTS.md 155 lines vs the 150 target, no CODEOWNERS), neither touched here. Falsification: each of the six round-2 fixes was individually reverted and the test pinning it confirmed to fail, then restored to green. That pass is why b1fdb78 exists — reverting the companion's line anchor left every test green, because the end-to-end fixture reached the reader through render_finding_entry and so was covered by the new writer-side defang rather than by the anchor it claimed to pin. The reader-side control is now pinned directly, which is the half that covers consumer repos still on older SHAs. Two rendering claims were checked empirically against POST /markdown (mode: gfm) rather than argued from the spec, and drop_unterminated_comment was fuzzed over 20,000 mixed fence/blockquote/terminated/dangling bodies: no dangling opener survives it and it never grows the cut. No workflow or shell files were edited, so the pin-validation surface is unchanged.
Deviations: the branch now carries a merge of origin/main (1d37fe8), which had moved substantially under this PR in the same two files. It conflicted textually in build-ledger.py and test_post_review.py — both pure adjacency (each side inserted a new function or class at the same point), resolved by keeping both sides, with _body_only_entries taking main's newer resolve_lineage= signature. The merged tree runs 484 tests green, so the merge-queue result is exercised rather than assumed. Otherwise as planned, with one deliberate step past a reviewer's suggestion carried over from round 1 (the prose floor is max(available - prose_len, …) rather than a flat half). One round-1 finding remains answered rather than fixed, with the reasoning on its thread; the round-1 rationale that round 2 overturned is marked as such above rather than quietly rewritten.

_{🔄 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/274 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/8/2026 **Status:** ✅ Merged **Merged:** 9/9/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-12535-success-path-sentinel-budget` --- ### 📝 Commits (6) - [`7b5579b`](https://github.com/Comfy-Org/github-workflows/commit/7b5579bc12f96651bedcc43899dd004b29bce680) fix(cursor-review): budget the success path's demoted-findings sentinel (BE-12535) - [`4787286`](https://github.com/Comfy-Org/github-workflows/commit/47872860c1661c0b2d0d0e2bd4704056d25fb41c) fix(cursor-review): make a budget-truncated sentinel disclose its own loss (BE-12535) - [`2d9eac1`](https://github.com/Comfy-Org/github-workflows/commit/2d9eac13f9a648583d51501d77cf360591c6568b) fix(cursor-review): neutralize planted comment openers and stop the ceiling overcharging (BE-12535) - [`b570aab`](https://github.com/Comfy-Org/github-workflows/commit/b570aabaefb8868e509771cd241fcf7f2a7e67a5) Merge remote-tracking branch 'origin/main' into matt/be-12535-success-path-sentinel-budget - [`b1fdb78`](https://github.com/Comfy-Org/github-workflows/commit/b1fdb78ad5479702785742fe7a50ee9273e9517e) test(cursor-review): pin the companion's line anchor at the reader, not via the writer's defang (BE-12535) - [`bc133ee`](https://github.com/Comfy-Org/github-workflows/commit/bc133eefa45efa5544c55539beeaccae99d37c71) fix(cursor-review): spell the defang's zero-width space as an escape (BE-12535) ### 📊 Changes **4 files changed** (+944 additions, -55 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/build-ledger.py` (+66 -3) 📝 `.github/cursor-review/post-review.py` (+320 -50) 📝 `.github/cursor-review/tests/test_build_ledger.py` (+98 -0) 📝 `.github/cursor-review/tests/test_post_review.py` (+460 -2) </details> ### 📄 Description ## ELI-5 When a review round has too many findings to anchor to lines in the diff, `post-review.py` lists them in the review body and hides a machine-readable copy of them in an HTML comment (the "sentinel") so the *next* round's ledger can read them back. That sentinel is nearly as long as the prose it duplicates, so it competes with the findings a human actually sees — and on the success path it was allowed to take the whole body. Past a certain number of findings it overran the size limit, GitHub's clamp cut it mid-JSON, the unterminated-comment backstop rewound it to its opener and swallowed every finding below it, and the review posted a 420-character header with nothing in it. The 422 fallback path already had a guard for exactly this; this gives the success path the same one. ## What changed - `FALLBACK_SENTINEL_MAX_CHARS` → `SENTINEL_MAX_CHARS` (same value, `MAX_REVIEW_BODY_CHARS // 2`), now documented as the prose floor on **both** finding-carrying paths rather than the fallback's alone. `git grep` found no reader outside this repo, so no back-compat alias was added; the one stale mention was a test docstring, updated in place. - `render_body_only_findings(items, budget=None)`. `budget` is how many characters the whole section may occupy before the clamp's cut point. `None` keeps today's behaviour byte-identical (the round-trip tests in `test_post_review.py` / `test_build_ledger.py` all call it unbudgeted and pin that). With a budget it first checks whether the whole section fits — if it does, nothing is charged, because the clamp will not cut it — and otherwise hands `sentinel_share(available, len(prose))` to the existing `fit_sentinel_items`, keeping the most-urgent prefix that fits — or dropping the sentinel entirely and posting the marker alone. Both outcomes print the same kind of stderr disclosure the fallback path already prints. The prose loop still renders **every** item either way: the budget governs what the ledger recovers, never what a reader is shown. - `main()` passes `budget=MAX_REVIEW_BODY_CHARS - len(CLAMP_TRUNCATION_NOTE) - len(review_head) - len(FINDINGS_SEPARATOR)`, computed where `review_head` has actually been measured. The posted body is `review_head + FINDINGS_SEPARATOR + marker + sentinel + "\n\n" + prose` and the clamp cuts at `limit - len(note)`, so the sentinel's closing `-->` now always sits ahead of the cut. - `drop_unterminated_comment` is scoped to CommonMark's HTML-block start condition, and its docstring no longer claims a render-time budget "cannot promise this" — both paths now derive one from the measured head, so it is reworded as the backstop for any HTML comment in any posted body (a miscounted head, a comment some future path adds) rather than as the primary guard. - `.github/cursor-review/README.md` needed no change: `grep -ni "sentinel\|size guard\|body-only"` returns nothing there, so the size guard is not described at that level. ## Evidence Reproduced on the base commit through the repo's stubbed-`gh` harness — one anchorable finding plus n demoted findings of ~700 chars, reading the success-path payload (`posted[0]`): | n | before (len / ledger entries / visible findings) | after | |---|---|---| | 60 | 60,000 / 45 / 27 | 60,000 / 45 / 40 | | 90 | 60,000 / 45 / 1 | 60,000 / 45 / 40 | | 91 | **420 / 0 / 0** (`unrecovered_rounds=1`) | 60,000 / 45 / 40 | | 100 | **422 / 0 / 0** | 60,000 / 45 / 40 | | 130 | **422 / 0 / 0** | 60,000 / 45 / 40 | Then swept the shape space rather than only the shapes being fixed: 42 combinations of body length (100–3,000 chars) × finding count (20–200), before vs after. 19 unchanged, 23 changed, **0 regressed** on either ledger entries or visible findings. Ten of the changed cells were the total collapse above (0 entries, 0 visible, a 422-char body), now carrying 45–73 ledger entries and 10–68 visible findings; the other 13 keep the same entry count and show a reader more findings (e.g. 700-char bodies at n=80: 45 entries either way, visible findings 10 → 41). The only shapes where the new cap could cost ledger entries are ones where the sentinel exceeds half the limit while the prose does not, and none exist in the swept range because a sentinel entry is always shorter than the prose entry it duplicates. Edge cases checked by hand beyond the suite: a review head larger than the whole limit drives `budget` negative — `fit_sentinel_items` already guards `budget <= 0`, so it degrades to the marker-alone branch and the clamp takes it from there, with no traceback. This change denies no capability and adds no dead-end path: at every measured shape the new code posts strictly more recoverable findings and strictly more visible findings than the old code, never fewer. The two new stderr messages are degradation disclosures on a path that previously degraded *silently*. ## Tests Five added next to the fallback's equivalents in `BodyBudgetTest`: the success-path prose floor across n ∈ {60, 90, 91, 100, 130}; the clamp-boundary sweep across a padded `path`; the most-urgent-prefix assertion (reading the sentinel **payload**, not the ledger — `build_ledger` re-sorts its entries, so the write order the test is about only survives in the payload); `budget=None` equivalence; and the marker-alone floor. Every existing test named in the plan stays green unchanged, including `test_the_sentinel_survives_the_clamp_that_cuts_the_prose`, `test_the_marker_still_outlives_a_cut_that_takes_the_whole_sentinel`, `test_the_sentinel_follows_the_marker_precedes_the_findings_and_round_trips`, and the fallback prose-floor / reserve tests. Mutation check as specified: removing `budget=` from the `main()` call fails all 11 subtests of the two new success-path tests (including every n ≥ 91) plus the prefix test, then restored to green. ## Judgment calls - **No `FALLBACK_SENTINEL_MAX_CHARS` alias.** `git grep` across the repo found the old name only in one test docstring; nothing outside this repo imports `post-review.py` (the scripts are loaded at run time by workflows in this repo, never vendored), so an alias would be dead code. Renaming it outright is the reversible choice. - **`budget: int | None` is the file's first PEP 604 union.** Valid on 3.10+; CI pins 3.12 and `cursor-review.yml` runs on `ubuntu-latest`'s system `python3` with no `setup-python`, so there is no interpreter here older than that. - **`budget=None` default rather than making the argument required.** Three test modules and the round-trip suite call this function unbudgeted and pin its exact output; a required argument would have forced churn in tests that are deliberately pinning the unbudgeted render. ## Residual - The panel-size interaction is now *tuned* rather than merely bounded — see the review round below — but nothing caps `review_head` itself on either path. Past ~59,660 characters of head the clamp cuts inside the head, and the whole findings section (marker, sentinel, companion and prose alike) lands past the cut no matter what this code emits. Not reachable from today's inputs: every head contributor is a short workflow-generated string, and it took a synthetic 54,000-character model name to reach that regime in the new sweep test. Worth a ticket only if a caller ever ships a head that large. - A related in-progress change adds a `repeat_of` key to the sentinel payload. It does not conflict — `fit_sentinel_items` measures the real render, so the budget stays correct whichever lands first — but if that merges first this branch should be rebased rather than re-derived, since the payload's per-entry length changes. - **Duplicate check incomplete.** The originating investigation flagged that a residual ticket covering this same item may already exist, filed minutes apart from the change that introduced the fallback's guard. I hold no issue-tracker access from this environment and could not read either side to confirm, so I could not rule out a duplicate; if one exists, one should be closed against the other rather than both being built. The change itself is verified against the code on `main` regardless. - **Unexercised artifacts.** The originating investigation's transcript log and its findings comment are named in the source material but live outside this repo and were not readable from here; everything acted on is the reproduction re-derived independently against `main` at `52b4fe6` and reported above. The `cursor-review.yml` judge-failed raw-panel branch — the only route that produces an uncapped finding union in production — was exercised only through `post-review.py`'s own harness, not by running the workflow end to end, which would require a live PR and the review panel's model spend. ## Review round (4787286) The panel raised five findings on `7b5579b`. Four were valid and are fixed; the fifth is answered on its thread. Every fix is pinned by a test verified to fail against the pre-fix code and pass after. - 🟡 **A budget-truncated sentinel silently lost findings** (4/6 reviewers). A prefix payload is still valid JSON, so `build-ledger.py` read 12-of-89 as a *complete* recovery — strictly worse than the all-or-nothing rule the budget replaced, which degraded loudly because a dropped sentinel does not parse. The writer now emits `` on a real cut, and `_body_only_entries` returns entries and `degraded` **independently**, so a truncated round both contributes what it recovered and discloses what it did not. A second comment rather than a payload key, because the reader pins the sentinel to a single-spaced opener directly below the prose marker: anything inserted between them breaks the recovery it annotates, and an older pinned reader ignores an unknown trailing comment instead of failing to parse. Applied on both budgeted paths, defanged in quoted text alongside the other two literals, and the suffix is deliberately unpinned so an unexpected count shape still discloses. - 🟡 **The prose floor did not hold at every head size** (2/6). `min(SENTINEL_MAX_CHARS, available)` bought the ceiling only while the first term won; past a head of roughly half the limit the `min` stopped binding. Confirmed by reverting the one expression: the new sweep fails at head shares 0.5/0.6/0.75/0.9 with **one** visible finding. Now `sentinel_share(available, prose_len) = min(SENTINEL_MAX_CHARS, max(available - prose_len, available // 2))` — one step past the reviewer's flat half, so a big-head round with *small* prose is not charged a floor nothing was going to use. Line 1681 got the same treatment, as flagged. - 🟢 **The 30k cap bound unconditionally** (2/6). A section that fits its budget now pays no budget at all. Reachable exactly through the escaping the reviewer identified: 400 findings on a hyphen-rich path push the render past `SENTINEL_MAX_CHARS` on `-` → `\u002d` alone while the whole body stays well under the limit. The 422 fallback gets the same skip. - 🟢 **`drop_unterminated_comment` rewound too far** (1/6). Scoped the rewind to CommonMark's HTML-block start condition, `(?:\A|(?<=\n)|(?<=\r)) {0,3}` does not help either — that one is escaped to `-->` and closes nothing — so the opener has to die at the **writer**. `render_finding_entry` now neutralizes every ``, so a later `<!--` is already inside the earlier comment and is not an opener at all. It now rewinds to the first unterminated opener. Round 1's fifth finding (the marker-fits check) stands answered on its thread and is unchanged: it is still unreachable without a ~59,660-character head, at which point the clamp is cutting inside the head and this section is past the cut whatever it emits. ## Provenance - **Authored by:** agent-work loop - **Verified:** at `b1fdb78` (which merges `origin/main` at `1d37fe8` into the branch — see below): `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'`: 484 passed, 0 failed; `.github/groom/tests`: 374 passed, 1 skipped; `.github/agents-md-integrity/tests`: 46 passed; `shellcheck -x .github/bump-callers/bump-callers.sh .github/bump-callers/tests/test_bump_callers.sh`: clean; `check_agents_md.py --root .`: passed with the same 2 pre-existing warnings (AGENTS.md 155 lines vs the 150 target, no CODEOWNERS), neither touched here. Falsification: each of the six round-2 fixes was individually reverted and the test pinning it confirmed to fail, then restored to green. That pass is why `b1fdb78` exists — reverting the companion's line anchor left every test green, because the end-to-end fixture reached the reader through `render_finding_entry` and so was covered by the *new* writer-side defang rather than by the anchor it claimed to pin. The reader-side control is now pinned directly, which is the half that covers consumer repos still on older SHAs. Two rendering claims were checked empirically against `POST /markdown` (`mode: gfm`) rather than argued from the spec, and `drop_unterminated_comment` was fuzzed over 20,000 mixed fence/blockquote/terminated/dangling bodies: no dangling opener survives it and it never grows the cut. No workflow or shell files were edited, so the pin-validation surface is unchanged. - **Deviations:** the branch now carries a merge of `origin/main` (`1d37fe8`), which had moved substantially under this PR in the same two files. It conflicted textually in `build-ledger.py` and `test_post_review.py` — both pure adjacency (each side inserted a new function or class at the same point), resolved by keeping both sides, with `_body_only_entries` taking main's newer `resolve_lineage=` signature. The merged tree runs 484 tests green, so the merge-queue result is exercised rather than assumed. Otherwise as planned, with one deliberate step past a reviewer's suggestion carried over from round 1 (the prose floor is `max(available - prose_len, …)` rather than a flat half). One round-1 finding remains answered rather than fixed, with the reasoning on its thread; the round-1 rationale that round 2 overturned is marked as such above rather than quietly rewritten. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:38:00 +00:00