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 #225] [MERGED] fix(cursor-review): partition findings against the reviewed diff so one bad anchor stops costing every other (BE-9531) #231

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-9531-anchor-partition

📝 Commits (8)

92aafc2 fix(cursor-review): partition findings against the reviewed diff so one bad anchor stops costing every other (BE-9531)
151f2f2 fix(cursor-review): harden the anchor parser against spoofed headers and unbounded bodies (BE-9531)
81411d3 fix(cursor-review): keep the truncation promise, both quoting modes, and CR-safe diff reads (BE-9531)
016ce2c fix(cursor-review): contain a bare CR, budget the step summary, bound the error review (BE-9531)
9901bd8 fix(cursor-review): bound the fence, keep the delivery channel total, tighten the anchor gates (BE-9531)
e216a8b test(cursor-review): cover the truncated-summary run-log echo (BE-9531)
94bc001 fix(cursor-review): stop echoing the whole step summary to the run log (BE-9531)
e242ea8 fix(cursor-review): strip the header tab, refuse a swallowed header pair, make the surrogate scrub total (BE-9531)

📊 Changes

3 files changed (+2170 additions, -54 deletions)

View changed files

📝 .github/cursor-review/post-review.py (+775 -52)
➕ .github/cursor-review/tests/test_post_review.py (+1371 -0)
📝 .github/workflows/cursor-review.yml (+24 -2)

📄 Description

ELI-5

When the judge finishes, we post one PR review carrying every finding as a line-anchored inline comment. GitHub treats that comments array as a single unit: if one comment cites a line the diff doesn't carry, it rejects the whole request with HTTP 422, and post-review.py falls back to a body-only review. So one bad line number strips the anchors off every other finding in the round.

That happened on a real round: 10 findings, 9 on lines inside the hunks, 1 citing a real line in a touched file but outside every hunk in it → 0 anchored comments. The log carried only Review POST failed: gh: Unprocessable Entity (HTTP 422), and the review said "Inline comments could not be anchored to the diff" without naming the finding that sank the other nine. The previous round on that same PR anchored all ten, so nothing visible on the PR explained the difference.

normalize_comments checks that a finding has a path and a positive integer line — never that the line is in the diff — and the failure path degrades wholesale with no retry.

The reviewed diff is already sitting in the consolidate job (/tmp/pr-diff.patch, downloaded for the judge prompt), so the anchor set can be computed before posting instead of discovered from a 422.

What changed

post-review.py: anchorable_lines() parses the diff into per-file new-side line sets; partition_by_anchor() splits findings into inline vs body; load_anchors() reads the diff and fails open; render_body_only_findings() renders the unanchorable ones naming file:line. New optional --diff.
cursor-review.yml: passes --diff /tmp/pr-diff.patch at the two call sites that render findings. The --error-message site renders none, so it is untouched.

Four properties, each deliberate:

Fails open, but only for text that isn't a diff. No --diff, unreadable, empty, or no recognizable diff marker → every finding goes inline exactly as today. A diff we cannot read must never cost a finding an anchor that would have worked. Every such path says so on stderr. A diff that parses but has nowhere to anchor (delete-only, binary-only, mode-only) is a real answer, not an unusable one: those findings route to the body, because sending them inline is the 422.
The count stays honest. Found **N** is the total across both halves, and the severity table still counts every finding. A finding rendered in the body is still a finding; a headline that shrank because an anchor missed would misreport the review.
A file whose numbering becomes untrustworthy is dropped to the body, not guessed at. An unparseable hunk header, or content that doesn't fit the hunk's declared line budget, drops that file rather than numbering the remainder from a guess — a wrong anchor sends back the same 422 this exists to prevent.
The wholesale fallback stays. The reviewed diff excludes generated globs, so it is a subset of what GitHub will accept. The mismatch direction is conservative — an extra body-rendered finding, never a bad anchor — and the 422 path still catches anything that slips past the filter.

Review round 1 — parser hardening (`151f2f2`)

The panel found eight issues in the first cut. Seven fixed, one deferred:

Header spoofing inside hunk content (High). An added line whose text begins ++ is emitted in the diff as +++ ..., and the first cut parsed it as a new-file header — numbering the rest of the file under a spoofed path, which is the wrong-position 422 this feature exists to prevent. The scan now tracks each hunk's remaining line budget from its @@ counts, so the header test only runs outside hunk content; a +++ additionally needs its paired --- predecessor, and a diff --git line (which content can never impersonate) resynchronizes the scan after a lossy diff.
splitlines() over-splits. It breaks on \v, \f, \x1c–\x1e, U+0085 and U+2028/9, none of which advance git's line numbering — one form feed in a Python file shifted every later anchor by one. Now split("\n") with the trailing element popped.
"No anchors" was conflated with "not a diff". Split apart, per the first property above.
C-quoted header paths. core.quotePath is on by default, so a real git diff of café.py emits +++ "b/caf\303\251.py" — the first cut keyed it with the quotes and no finding in that file could ever anchor. Now unquoted; .strip() dropped so a legitimate trailing space survives.
Model-supplied path rendered raw into markdown. It can carry backticks, @ and newlines; demotion to the body turned that from a rare fallback render into a routine one. New render_code_ref() neutralizes mentions, strips CR/LF, and sizes the code fence past the longest backtick run.
Unbounded review body. Demoted findings render in the body, each up to the 20,000-char cap, against GitHub's 65,536-char limit — and the wholesale fallback is strictly larger than what just failed, so an oversize body lost the entire review. Now clamped; the step-summary copy stays whole.
False "could not be anchored" note when there were no inline comments to lose. Gated.
Deferred: body-only findings carry no thread, so they get no repeat_of link and no REPEAT_CAP cover. Documented in place as a # KNOWN GAP and filed as a follow-up — capping a finding that can never be answered is a policy call, and there are two designs that would make the ledger change unnecessary.

Review round 2 — the degradation paths (`81411d3`)

The panel found seven more, all in the paths this PR made routine. All fixed.

The truncation note promised a job summary nobody wrote (Medium). clamp_review_body tells the reader "the full text is in the job summary of this run", but write_step_summary was only reached on a read-only token or an outright POST failure — never after a clamped POST succeeded, which is the one case the clamp exists for. Everything past the cut was absent from both the PR and the summary while the header still reported the full Found **N**. A clamped post now writes the whole body to the summary, under a banner saying which degradation happened.
Universal-newline mode edited the diff before the parser saw it (Medium). open(..., encoding="utf-8") leaves newline=None, so Python rewrote every lone \r to \n. A content line carrying a bare CR — mixed endings, a minified asset — was split in two, and the prefix-less tail either tripped the desync arm or shifted every later anchor in that file into the wholesale 422. newline="" hands the splitting back to the parser; it also makes header_new_path's rstrip("\r") load-bearing in production rather than only under a direct-call test.
The fallback re-POST was byte-identical when there was no inline half (Low). render_findings_markdown(review_body, []) returns the body untouched and the suffix is gated off, so with every finding already demoted the "fallback" resent exactly the request that had just failed. It cannot fix a size or malformed-body rejection, and if GitHub committed the write before erroring it published a duplicate review. It now degrades to the job summary and lets the step go red. Trade-off taken knowingly: a genuinely transient 5xx no longer gets a second attempt, but the text is still delivered and the red check says so — versus a duplicate review that no one can un-post.
unquote_header_path only decoded one of git's two quoting modes (Low). The latin-1 round-trip is correct with core.quotePath ON. With it off, git still quotes a name containing ", \ or a control character but leaves its UTF-8 bytes verbatim — a real é came back as U+FFFD and an astral character raised UnicodeEncodeError, so the key stopped matching the path findings cite and every finding in that file demoted silently. Now only the backslash escapes are resolved and everything else is taken verbatim.
A finding body was interpolated as raw markdown (Low). normalize_comments applies neutralize_mentions, which touches mentions and nothing structural, so model output could open a heading, forge a **path:line** — … row, or leave a code fence open across the rest of the review. Demotion moved that from the rare 422 fallback into the primary body on every run. Each finding is now rendered as a blockquote, which confines the structure while leaving legitimate fenced code rendering normally.
The clamp cut the wrong end of the fallback body (Low). The fallback was review_body — which already ends with the demoted half — plus the inline half appended after it, so trimming from the end dropped the findings that had just lost their anchors while a demoted nit survived. The head is now kept separate from the demoted block, and every body-only render lists all findings once from the severity-sorted list. Severity order holds end to end, so the cut lands on the least urgent.
An unparseable @@ header left headers trusted (Nit). The suggestion was to keep path so the file's later well-formed hunks could still anchor. Gone the other way: without the hunk's counts the scan no longer knows where its content ends, and a -- x / ++ b/evil.py pair inside it is emitted as --- x / +++ b/evil.py — read as a header pair, numbering the rest under a path the diff never touched. That is the spoofing 422 this feature exists to prevent, and it was reachable before this change. An unparseable header now desyncs until a diff --git line, so the file goes anchorless (fail-safe) and the next file is unaffected.

Round 2 verification

12 new cases (58 in test_post_review.py, 252 in the cursor-review suite). Each of the five behavioural fixes fails a case when reverted individually.
Empirical, against real git output. Generated git diff for d/café"x.py, d/🎉"y.py and d/plain-café.py under core.quotePath both true and false. The shipped parser returned d/caf�"x.py in one mode and None (UnicodeEncodeError) for the astral name; the new one returns all three paths correctly under both modes.
agents-md-integrity, groom, workflow-pins and public-repo-hygiene suites re-run green; shellcheck -x clean.

Review round 3 — the degradation paths, again (`016ce2c`)

The panel found seven more. Six fixed, one declined with reasoning.

A bare \r escaped the blockquote (High). render_finding_entry split on "\n" alone, but CommonMark — and GitHub's cmark-gfm — ends a line on a lone \r too, so a body of safe\r## Forged stayed in ONE element, took the > prefix on the element, and rendered the heading outside the quote. That restores exactly the forged-heading and unterminated-fence escapes round 2 added this function to contain, and an ATX heading interrupts a paragraph, so lazy continuation does not absorb it. Nothing upstream strips control characters: review-output-mcp.py's validate_finding checks only type/non-empty/length, and neutralize_mentions touches @ alone. Now splits on \r\n|\r|\n.
The fallback's own failure lost the review entirely (Medium). When the wholesale re-POST also failed for a non-403 reason, main() raised SystemExit(1) without ever writing the job summary — contradicting the no-inline branch 25 lines up, which writes it precisely so the text is still delivered, and doing it on the branch carrying more content. Reachable on an API outage, a stale commit_id after a force-push, or a body-level rejection that dropping the anchors does not fix.
The step summary had no size budget (Low). Actions caps $GITHUB_STEP_SUMMARY at 1 MiB per step and discards an overflowing upload WHOLE rather than truncating, so the one delivery channel clamp_review_body's note promises could itself vanish — the review absent past the cut on the PR and absent from the summary, while the header still reported the full Found **N**. Now budgeted in bytes, not characters: the cap is on the file and a finding body is not ASCII-only (600k é is 1.2 MB), and the cut never severs a multi-byte sequence. Live on the un-adjudicated panel path this PR wires --diff into — the judge's 10-finding cap does not apply there and the degraded branch unions all eight cells.
post_error_review was the one unbounded body left (Low). --error-message is $JUDGE_ERROR, read straight out of judge-findings.json, i.e. unbounded CLI/model text; past 65,536 chars the POST 422s and the error review is lost — on the path whose whole job is to report why the review failed. The message is cut to its own budget rather than the assembled body, because clamp_review_body trims the end, which here is the closing fence and the re-trigger instruction. Two things the finding did not raise, fixed in the same expression: a ``` run in a stack trace closed the fence early and let the rest render as markdown (the fence now outgrows the longest run inside), and a genuine POST failure here now writes the summary like every other path.
The counts/content desync guard covered only one direction (Low). A declared +count too SMALL spent the budget while content kept coming; those lines fell through, matched nothing and set no desynced, putting the --- /+++ header test back in force inside hunk content. Closed symmetrically with the too-large arm. The seam that leaves is closed separately: if the miscount is exactly two, the overflow lines ARE the header pair and the desync never fires — so a header pair reached from inside a hunk region is refused, since git always emits diff --git before one. Gated on having seen a diff --git , so a prefix-less concatenated diff -u, whose files legitimately follow one another, still parses every file.
Reworded the no-inline-half comment (Nit). After round 2's review_head split the skipped body is measurably not byte-identical to the one that failed. The duplicate-review risk, not identity, is the reason to skip it.
Declined: feeding the unfiltered pr-diff-new.patch to --diff to keep anchors on generated files. The premise is right — that patch gets diff_excludes only, since the generated-file exclusions are computed inside check-pr-size — but it is LAST_REVIEWED...HEAD, not base...HEAD. A line changed since the last round but reverted relative to base is absent from GitHub's own diff, so unioning it breaks the deliberate subset invariant and sends back the very 422 this PR prevents. The real defect it exposes is upstream (the incremental block shows the panel generated-file churn the reviewed diff excludes); that belongs in the diff-building steps, needs a design call, and is proposed as a follow-up.

Round 3 verification

17 new cases (75 in test_post_review.py, 269 in the cursor-review suite). Each of the seven code changes fails at least one case when reverted individually.
Empirical, against real git output. The parser produces a byte-identical anchor map to round 2's on a real git diff covering a rename, a delete, a binary file, a mode change, CRLF content, a non-ASCII path and a new file, under core.quotePath both true and false — so the two new guards cost no anchor on real input. Also re-checked that a real diff whose content lines are -- x / ++ b/evil.py still parses to the correct single-file map.
agents-md-integrity, groom, public-repo-hygiene, refresh-reviewers and workflow-pins suites re-run green; shellcheck -x clean. (coderabbit-config errors locally on a missing jsonschema — pre-existing, reproduces on a clean tree, and CI installs the pinned requirements.)

Review round 4 — the budgets themselves (`9901bd8`, `e216a8b`, `94bc001`)

The panel found nine. Eight fixed, one declined with reasoning.

The fence outgrew the body it lived in (Medium). Round 3 made the fence outgrow the longest backtick run inside the message — but the fence is emitted twice, and a 40,000-backtick message is a perfectly legal capped message, so the delimiters alone came to 80,002 chars against a 60,000 body limit. clamp_review_body then cut the end: precisely the closing fence and the re-trigger line the message-side budget was added to preserve, leaving an unterminated fence that renders the clamp note as literal code. The trigger correlates with the path — a degenerate model/CLI repetition loop is both what fills error and what produces a run that long. Runs at or over MAX_FENCE_CHARS (64) are now broken up before the length budget, so the markers the substitution inserts stay inside the budget rather than being appended after it.
A lone surrogate killed the delivery channel (Medium). Python's JSON decoder admits "\ud800" and review-output-mcp.py's validate_finding checks type/non-empty/length only, so a finding body could carry one to write_step_summary, where .encode("utf-8") raises UnicodeEncodeError — taking down the channel that exists so nothing is lost (on the read-only path, the whole review with it). Fixed with one encodable() round-trip up front rather than per-encode: the size check was not the only hazard, the open(path, encoding="utf-8") write at the end of the same function would have raised too, and surrogatepass there would put invalid UTF-8 into the summary file.
The truncation note promised a run log nothing wrote to (Low). STEP_SUMMARY_TRUNCATED_NOTE sent the reader to the run log while the only prints on that path were status lines — so past the cut the text was gone from the PR (whose clamp note points at the summary) and gone from the summary. The panel offered two fixes; the first one was tried and reverted, on evidence. Echoing the whole payload to stdout made the promise true locally, and hung CI: the budget only fires near 900 KB, and pushing that into the Actions log took unittest + shellcheck past 13 minutes against the 8s it takes otherwise (suite output went from 3 KB to 4 MB). A delivery channel that stalls the run is not a delivery channel, and in production it would dump ~900 KB into a public Actions log on every truncated summary. Took the second option instead — the note now promises nothing rather than pointing somewhere the text isn't. Independently, the no-summary-file branch was hoisted above the budget, since the 1 MiB cap is a property of the file: a local run prints the full payload instead of a needlessly clamped one.
post_error_review re-opened the gap it had just closed elsewhere (Low). It omitted truncated=, so a clamped-but-successful POST wrote no summary and the clamp note pointed at a document that never existed — the same gap closed for main() and post_or_degrade, left open on the one caller behind. It also passed the already-cut body as summary_markdown, so even the read-only degradation, whose whole purpose is delivering the whole text, delivered the cut copy.
The fourth exit path delivered nothing (Nit). The no-findings / all-panel-failed POST failure raised SystemExit(1) with no write_step_summary. On an API outage that loses the one artifact explaining why no review happened — the panel summary naming which cells errored. All four exit paths now agree.
A header pair could still be honoured in git-output mode (Low). Round 3's saw_git_header and in_hunk_region gate only re-armed at the next @@, so an honoured +++ opened a window: a pair landing between it and the first hunk was honoured, merging spoofed anchors into the real file's set. Replaced with awaiting_header, the structural property that actually holds — git emits exactly one --- /+++ pair per diff --git , so the pair is trusted only while this file's own pair is outstanding. That strictly subsumes the hunk-region test.
The two desync arms disagreed on headers (Low). Inside the hunk budget the +/- tests ran before any header exclusion, and the branch is entered while either side has budget left — so on a too-large +count the next file's +++ b/y.py was counted as an added line, fabricating an anchor in the previous file and swallowing y.py entirely; and on @@ -1,2 +1,1 @@ a second + still recorded an anchor while pending_new went negative. Each prefix is now consumed against its own side's counter.
$JUDGE_ERROR never reached the budget meant for it (Low). MAX_ERROR_MESSAGE_CHARS can only run once Python has started, but the workflow passed the message as a single --error-message argv element and Linux rejects one argument over ~128 KiB at execve — so the largest errors, exactly the ones the budget was added for, produced no error review and no job summary at all, just a shell-level failure. Bounded in the shell to 100,000 UTF-8 bytes: under MAX_ARG_STRLEN with room for the prefix, and still above the char budget so post-review.py still emits its own "(truncated…)" note.
Declined: re-downloading or hashing /tmp/pr-diff.patch to stop an injected --trust judge from tampering with it. The judge step and this post-review.py invocation are steps in the same consolidate job, and CURSOR_REVIEW_ASSETS resolves under ${{ github.workspace }} — so an agent that can rewrite the patch can equally rewrite post-review.py itself, and a hash check inside that script is unenforceable against an attacker who can edit it. Both mitigations read as a boundary without being one. The fix that would help is structural (post from a job the agent-bearing job cannot write to, as groom.yml splits its patch and PR jobs) — a design call well beyond this PR, left as a thread comment rather than filed blind. Note this PR does not widen that surface: /tmp/pr-diff.patch was already the judge-visible input before it became a --diff argument.

Round 4 verification

15 new cases (90 in test_post_review.py, 284 in the cursor-review suite), all green.
Mutation-tested individually: each code change in post-review.py fails at least one case when reverted on its own (the plus- and minus-side counters, the awaiting_header gate, the fence break, the surrogate sanitize, the truncated= flag, the unclamped summary copy, the no-findings delivery, and the reworded cut note). The shell-side $JUDGE_ERROR bound is the one change with no harness — it is YAML-parsed and reasoned about, not executed.
One fix was reverted on CI evidence, not on review: the stdout echo above. Caught because the unittest + shellcheck job that normally finishes in 8s sat in_progress past 13 minutes. 94bc001 replaces it and the job is green in seconds again; two cases now pin that stdout stays empty on both the ordinary and the truncated path, so this cannot come back silently.
Regression-pinned, not just fixed. The two parser changes ship with the shapes that must keep working: a prefix-less concatenated diff -u (consecutive header pairs stay legitimate where no diff --git exists), ordinary multi-file git output, and a well-formed mixed add/remove/context hunk.
Merged result checked. main moved since this branch was cut (including the AGENTS.md trim). Merged origin/main into the branch locally and re-ran every suite on the merged tree — green, and check_agents_md.py passes there — so there is no semantic conflict waiting in the merge queue. The merge was then discarded; the branch still merges cleanly on its own.
agents-md-integrity (46) and groom (365) suites re-run green; shellcheck -x clean; cursor-review.yml re-parsed as YAML after the shell edit.

Review rounds 5–6 — real-world paths and the last of the budgets (`e242ea8`)

The panel re-raised nine findings across two rounds. Eight fixed, one taken in half, one declined with reasoning.

git's tab-terminated header path (High). git terminates the --- /+++ path with a TAB whenever the name contains a space. Confirmed against git itself before touching anything: --- a/my dir/app.py\t, --- "a/caf\303\251 x.py"\t when quoted (the tab lands after the closing quote), and --- a/trail \t for a name ending in a space. Stripping only \r keyed the file as my dir/app.py\t, which no finding's file can ever match — so every finding in a spacey-path file lost its anchor and demoted to the body, taking its thread, its repeat_of link and its REPEAT_CAP cover with it. A regression against pre---diff behaviour, where those comments anchored fine. Now split off one tab-delimited suffix, before the quoted-path test. Safe in both quoting modes: a name that really contains a tab is a control character, which git quotes as \t even under core.quotePath=false, so an unquoted target never carries a literal one. The legal trailing space still survives.
Both counts over-declared swallowed the next file's header pair (Medium). Round 4 gated each prefix on its own counter, which covers a one-sided over-declaration. When both sides are over, the pair is consumed as content — the --- on the - arm and the +++ on the + arm — so neither that gate nor the spent-budget arm fires: the +++ fabricates an anchor in the previous file and the following @@ renumbers the next file under the stale path ({"x.py": {1,2,3,500,501}}, y.py absent). The + arm now refuses an eaten header pair and desyncs until a diff --git . Detecting the pair rather than desyncing on either form alone is deliberate: a genuine diff of a patch file legitimately carries removed -- x / added ++ y content. It can still trip this, and demoting that one file is the fail-safe direction — a single wrong anchor costs every anchor in the request.
A --- on a spent budget was honoured unconditionally (Low). Round 3's exclusion of the header forms from the spent-budget desync arm left the --- branch ungated, because the saw_git_header guard sits only on the +++ side — so an overflow line rendering --- x fell through untouched and the next @@ fabricated anchors under the still-current path. Now gated on awaiting_header or not saw_git_header. awaiting_header is the precise condition rather than a bare not saw_git_header: git does emit a header pair on a zero budget, right after each diff --git , which is exactly when awaiting_header is True. A prefix-less concatenated diff -u is unaffected.
@@ -1,0 +0,3 @@ produced a shifted anchor set (Low). The path is not None and right truthiness test read the +0 start as "no hunk header yet", skipped the first added line and recorded the second and third as 1 and 2 — anchors on lines the diff never carried, where every other malformed-header case drops the file. Rejected at parse time on new_start == 0 and new_count > 0, which real diff output never emits; @@ -1,3 +0,0 @@ (a whole-file delete) still parses as the legal anchorless hunk it is.
The surrogate scrub was per-channel (Low). encodable() ran only in write_step_summary, so a lone surrogate in a finding body — which json.load produces from "\ud800" and validate_finding does not reject — rode into json.dumps and reached the API as a literal \ud800 escape. The wholesale fallback carried the same escape and failed the same way, leaving the review only in the job summary: the one copy that was sanitized, so the two channels disagreed on the finding's text. Now applied in normalize_comments (covering the body and the repeat_of line) and in clamp_review_body, the choke point every posted body passes through.
Two notes promised a full text the summary can cut (Low). TRUNCATED_SUMMARY_NOTE said "The full text is below" and clamp_review_body's said "in the job summary of this run", but write_step_summary has its own byte budget and cuts too — so both could be false at once, on the same oversize body, while the header still reported the full Found **N**. Both now say "as much of it as fits". Softening beat having write_step_summary report back: the PR body is posted before the summary is written, so a return value cannot retroactively correct the note the reader already holds.
The run-log line promised a payload nothing prints (Low). "The whole payload follows." was true until 94bc001 deleted the stdout echo in the same commit, and then sent an operator hunting the log for ~900 KB deliberately never written there. Now says the remainder is not delivered anywhere.
Dead $JUDGE_ERROR extraction on the panel-degraded branch (Medium). That branch passes --notice with $JUDGE_STATUS and never --error-message, so the value had nowhere to go — but under the step's bash -e a standalone assignment inherits its command substitution's exit status, so a UnicodeEncodeError from a lone surrogate in error would have exited the step before the raw un-adjudicated panel review was posted. Deleted, with a comment in its place recording why it must not come back.
A byte cut cannot guarantee a character-side note (Low). Round 4 bounded $JUDGE_ERROR to 100 KB on the claim that this "still exceeds" MAX_ERROR_MESSAGE_CHARS — true only at ≥2.5 bytes per character. 100 KB of emoji is 25,000 characters and CJK roughly 33,000, both under the 40,000 budget, so a severed message reached post_error_review looking complete. Raising the byte cut until that were impossible would breach MAX_ARG_STRLEN, so the marker is appended in the shell, only when the cut actually fired.
Taken in half: a hunk left incomplete at EOF. The silence was real, so a mid-hunk cut now says so on stderr, naming the file — otherwise that demotion is indistinguishable from a finding the model simply placed outside the diff. I did not drop the file or return None, as suggested. The other counts-disagree arms desync because the scan has lost track of the numbering and would record wrong lines from there on; here nothing follows the cut, so every line already recorded sits inside a real hunk and anchors correctly. {"x.py": {1}} is a correct map, merely incomplete, and it still honours the stated contract. Dropping the file would demote the line-1 finding too for no safety gained, and return None would send the unanchorable findings inline — the 422 this function exists to prevent. Reachability also argues against a behaviour change: the reviewed diff is written whole by check-pr-size --reviewed-diff-out or a plain git diff redirect, with no byte cap.
Declined: neutralizing a leading :: before an echo, to stop model text forging Actions workflow commands. The echo it cites was print(payload), deleted in 94bc001 — the same deletion the "whole payload follows" finding is about. What remains there is a fixed status string with no model text in it. The only surviving print(payload) is write_step_summary's fallback for an unset GITHUB_STEP_SUMMARY, a local-run path; on an Actions runner that variable is always set, so on every CI path the payload goes to the summary file, which the runner does not scan for workflow commands. Left as a thread reply rather than a ticket, since it cannot trigger today.

Rounds 5–6 verification

21 new cases (111 in test_post_review.py, 305 in the cursor-review suite), all green.
Mutation-tested as a batch: 12 of the 21 fail against the previous code; the other 9 pin behaviour that must not regress (an ordinary multi-file git diff, a prefix-less diff -u starting its next file, a well-formed mixed add/remove/context hunk, a whole-file delete's +0,0 header, a complete final hunk, and a legitimate removed/added pair adjacent to the new header-pair guard).
Empirical, against real git output. The tab finding was confirmed by generating git diff for my dir/app.py, café x.py, sp ace"q.py, we<TAB>ird.py and a name ending in a space, under core.quotePath both true and false — which is also where the "a tab-bearing name is always quoted" premise the fix rests on comes from. The reworked $JUDGE_ERROR expression was executed against a 200 KB message (marker present, ~100,070 bytes, inside MAX_ARG_STRLEN), a short one (no marker), a lone surrogate and a non-string error.
agents-md-integrity (46), groom (365) and bump-callers (437) suites re-run green; shellcheck -x clean; cursor-review.yml re-parsed as YAML and bash -n-checked after the shell edits; actionlint output byte-identical to the base (its one finding is pre-existing and on an untouched line).
Merged result re-checked. main's only movement since the merge-base is an AGENTS.md trim, a caller SHA bump and a pr-risk docs change — nothing under .github/cursor-review/ or in cursor-review.yml — so there is no semantic conflict waiting in the merge queue, and no merge-group run has ever been ejected for this PR.

Verification

tests/test_post_review.py, 42 cases: new-side numbering (added/context/removed, multi-hunk, multi-file, +++ /dev/null, unparseable header, \ No newline marker), header spoofing, line-splitting, C-quoted/CRLF/trailing-space paths, unusable-vs-anchorless, markdown escaping, the body budget, the fallback suffix, the partition, all fail-open paths, and end-to-end payload assertions driving main() against a stubbed gh — including the exact field regression (10 findings → 9 inline + 1 in the body, one POST, no fallback).
Full suite: 236 passed, python3 -m unittest discover -s .github/cursor-review/tests. Also re-ran agents-md-integrity, groom, workflow-pins and public-repo-hygiene: all green.
Mutation: removing the partition call fails 3 of the new cases.
Empirical, not just unit-tested. Generated a real git diff containing an added line ++ b/evil.py, a C-quoted café.py, a deleted file and a new file. The pre-hardening parser produced {"app.py": [1,2], "evil.py": [3,4,5,6]} and keyed café as "b/caf\303\251.py"; the hardened one produces {"added.py": [1,2], "app.py": [1..7], "café.py": [1,2]}, with the deleted file contributing nothing. That is the High finding reproduced and closed against real git output rather than a hand-written fixture.
post-review.py had no test file at all before this; these are its first coverage.

Judgment calls

Pre-filter rather than retry-on-422. A retry would need GitHub to tell us which position was rejected; the 422 body doesn't reliably say, so the retry would be a bisect (up to N extra POSTs) or a wholesale drop — which is what we already do. The diff is right there, so filtering is one read and zero extra API calls.
Body prose, not silent suppression. An unanchorable finding is still a finding; it is rendered with its file:line under a line saying why it isn't inline.
The a//b/ prefix strip was deliberately not generalized. The reviewed diff is generated by this workflow with a plain git diff, so diff.noprefix/diff.mnemonicPrefix are unreachable, and accepting a bare single-char prefix would mangle a real repo-root path like b/thing.py.
Untouched: the read-only-token degradation, the no-findings/error paths, and the repeat cap.
--diff is optional so a caller pinned to an older workflows_ref keeps working unchanged — the script and the workflow move together, but nothing breaks if only one does.

Residual

Not exercised against a live PR. Everything here is against the real code with a stubbed gh plus real git diff output; the end-to-end proof is the first round this posts after merge, where the expected signature is anchored comments plus a body block instead of an all-prose review.
Judge behaviour unchanged. The model can still cite a line outside the diff; this makes that cost one finding's anchor instead of all of them. Teaching the judge prompt to cite only diff-carried lines would reduce the body-rendered cases further and is not attempted here.
Body-only findings are absent from the prior-review ledger — documented in code, deferred to a follow-up (see round 1 above).
AGENTS.md not updated. No doc in this repo describes the posting path's anchor semantics today, so there was nothing to keep in step; if you'd rather this be written down, say where and I'll add it.

Provenance

Authored by: agent-work loop
Verified: 305 passed in the cursor-review suite (111 in test_post_review.py); agents-md-integrity, groom, public-repo-hygiene, refresh-reviewers and workflow-pins suites re-run green; shellcheck -x clean. coderabbit-config errors locally on a missing jsonschema and reproduces the same way on a clean tree — pre-existing, CI installs the pinned requirements. Round 1's header-spoofing and C-quoted-path fixes, round 2's both-quoting-modes fix and round 3's two new parser guards reproduced and closed against real git diff output, the last confirmed to produce a byte-identical anchor map to round 2's across a rename, delete, binary, mode change, CRLF, non-ASCII path and new file under core.quotePath both modes. Each round-2, round-3 and round-4 code change in post-review.py fails a test when reverted individually; round 4's shell-side $JUDGE_ERROR bound is the one change with no harness. Round 4 additionally merged origin/main into the branch locally and re-ran every suite on the merged tree (green, check_agents_md.py included) to rule out a semantic conflict in the merge queue, then discarded the merge. Rounds 5–6 add 21 cases, 12 of which fail against the previous code and 9 of which pin behaviour that must not regress; the tab-terminated-header fix and the premise it rests on (a tab-bearing name is always quoted) were confirmed by generating real git diff output for five awkward paths under core.quotePath both modes, and the reworked shell-side $JUDGE_ERROR expression was executed against oversize, short, lone-surrogate and non-string inputs — so it is no longer the one change without a harness. bump-callers (437) re-run green; cursor-review.yml bash -n-checked and actionlint output byte-identical to the base. main's movement since the merge-base touches no file this PR touches, and no merge-group run has ever been ejected for this PR.
Deviations: several panel findings addressed by something other than the suggestion, each with its reasoning on the thread and in the round section above. Round 1's ledger gap is deferred rather than fixed — documented in code, filed as a follow-up, and needing a policy decision on capping an unanswerable finding. Round 3's generated-file anchor suggestion is declined outright: unioning the incremental patch into --diff would break the reviewed-diff-is-a-subset invariant and reintroduce the 422 this PR prevents; the upstream inconsistency it exposes is proposed as a separate follow-up. Round 2's unparseable-hunk-header nit was also addressed in the opposite direction to the suggestion — reasoning in the round-2 section above. Round 4 declines the diff-tampering hardening outright — the proposed mitigations are no-ops given that the judge step and the posting step share a job and a writable workspace, and the structural fix is a human design call; addressed as a thread comment, not a ticket. Round 4 also addressed the too-large-+count finding by the counter half of the suggestion rather than excluding header forms inside the hunk budget, which would have desynced legitimate diffs of files containing ++ /-- lines — reasoning in the round-4 section above. Rounds 5–6 take one finding in half and decline one outright. The incomplete-hunk-at-EOF finding is answered with a stderr note rather than the suggested drop-the-file or fail-open: nothing follows an EOF cut, so the short map is correct rather than untrustworthy, and both suggested remedies would cost correct anchors — reasoning in the rounds-5–6 section above. The workflow-command-injection finding is declined as unreachable: the echo it cites was deleted earlier in this same PR, and the one surviving stdout print is a local-run path that no CI run takes. Both were answered on their threads rather than filed as tickets.

Closes BE-9531

_{🔄 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/225 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 8/25/2026 **Status:** ✅ Merged **Merged:** 8/26/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-9531-anchor-partition` --- ### 📝 Commits (8) - [`92aafc2`](https://github.com/Comfy-Org/github-workflows/commit/92aafc27b038990e24394f392fb89e04eeb59b76) fix(cursor-review): partition findings against the reviewed diff so one bad anchor stops costing every other (BE-9531) - [`151f2f2`](https://github.com/Comfy-Org/github-workflows/commit/151f2f2fe19474b154e437a6fb319282c04b78fb) fix(cursor-review): harden the anchor parser against spoofed headers and unbounded bodies (BE-9531) - [`81411d3`](https://github.com/Comfy-Org/github-workflows/commit/81411d392f006211494e680ffdd7d490444e1abb) fix(cursor-review): keep the truncation promise, both quoting modes, and CR-safe diff reads (BE-9531) - [`016ce2c`](https://github.com/Comfy-Org/github-workflows/commit/016ce2cf9caa3b147343c4710e42229f0f6a1900) fix(cursor-review): contain a bare CR, budget the step summary, bound the error review (BE-9531) - [`9901bd8`](https://github.com/Comfy-Org/github-workflows/commit/9901bd8812fb926c8c96e66225d03da61b421706) fix(cursor-review): bound the fence, keep the delivery channel total, tighten the anchor gates (BE-9531) - [`e216a8b`](https://github.com/Comfy-Org/github-workflows/commit/e216a8b79ce0c1e5f4989f5d81b650b2e807dcd4) test(cursor-review): cover the truncated-summary run-log echo (BE-9531) - [`94bc001`](https://github.com/Comfy-Org/github-workflows/commit/94bc0015e097da4d4046d7ec35ccbcd0ff556b48) fix(cursor-review): stop echoing the whole step summary to the run log (BE-9531) - [`e242ea8`](https://github.com/Comfy-Org/github-workflows/commit/e242ea80e3d0f9ea2536412688306cb148008427) fix(cursor-review): strip the header tab, refuse a swallowed header pair, make the surrogate scrub total (BE-9531) ### 📊 Changes **3 files changed** (+2170 additions, -54 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/post-review.py` (+775 -52) ➕ `.github/cursor-review/tests/test_post_review.py` (+1371 -0) 📝 `.github/workflows/cursor-review.yml` (+24 -2) </details> ### 📄 Description ## ELI-5 When the judge finishes, we post **one** PR review carrying every finding as a line-anchored inline comment. GitHub treats that `comments` array as a single unit: if **one** comment cites a line the diff doesn't carry, it rejects the whole request with HTTP 422, and `post-review.py` falls back to a body-only review. So one bad line number strips the anchors off every other finding in the round. That happened on a real round: **10 findings, 9 on lines inside the hunks, 1 citing a real line in a touched file but outside every hunk in it → 0 anchored comments.** The log carried only `Review POST failed: gh: Unprocessable Entity (HTTP 422)`, and the review said "Inline comments could not be anchored to the diff" without naming the finding that sank the other nine. The **previous** round on that same PR anchored all ten, so nothing visible on the PR explained the difference. `normalize_comments` checks that a finding has a path and a positive integer line — never that the line is in the diff — and the failure path degrades wholesale with no retry. The reviewed diff is already sitting in the `consolidate` job (`/tmp/pr-diff.patch`, downloaded for the judge prompt), so the anchor set can be computed **before** posting instead of discovered from a 422. ## What changed - **`post-review.py`**: `anchorable_lines()` parses the diff into per-file new-side line sets; `partition_by_anchor()` splits findings into inline vs body; `load_anchors()` reads the diff and fails open; `render_body_only_findings()` renders the unanchorable ones naming `file:line`. New optional `--diff`. - **`cursor-review.yml`**: passes `--diff /tmp/pr-diff.patch` at the two call sites that render findings. The `--error-message` site renders none, so it is untouched. Four properties, each deliberate: - **Fails open, but only for text that isn't a diff.** No `--diff`, unreadable, empty, or no recognizable diff marker → every finding goes inline exactly as today. A diff we cannot read must never cost a finding an anchor that would have worked. Every such path says so on stderr. A diff that parses but has *nowhere* to anchor (delete-only, binary-only, mode-only) is a real answer, not an unusable one: those findings route to the body, because sending them inline is the 422. - **The count stays honest.** `Found **N**` is the total across both halves, and the severity table still counts every finding. A finding rendered in the body is still a finding; a headline that shrank because an anchor missed would misreport the review. - **A file whose numbering becomes untrustworthy is dropped to the body, not guessed at.** An unparseable hunk header, or content that doesn't fit the hunk's declared line budget, drops that file rather than numbering the remainder from a guess — a *wrong* anchor sends back the same 422 this exists to prevent. - **The wholesale fallback stays.** The reviewed diff excludes generated globs, so it is a *subset* of what GitHub will accept. The mismatch direction is conservative — an extra body-rendered finding, never a bad anchor — and the 422 path still catches anything that slips past the filter. ## Review round 1 — parser hardening (151f2f2) The panel found eight issues in the first cut. Seven fixed, one deferred: - **Header spoofing inside hunk content (High).** An added line whose text begins `++ ` is emitted in the diff as `+++ ...`, and the first cut parsed it as a new-file header — numbering the rest of the file under a spoofed path, which is the wrong-position 422 this feature exists to prevent. The scan now tracks each hunk's remaining line budget from its `@@` counts, so the header test only runs **outside** hunk content; a `+++ ` additionally needs its paired `--- ` predecessor, and a `diff --git ` line (which content can never impersonate) resynchronizes the scan after a lossy diff. - **`splitlines()` over-splits.** It breaks on `\v`, `\f`, `\x1c`–`\x1e`, U+0085 and U+2028/9, none of which advance git's line numbering — one form feed in a Python file shifted every later anchor by one. Now `split("\n")` with the trailing element popped. - **"No anchors" was conflated with "not a diff".** Split apart, per the first property above. - **C-quoted header paths.** `core.quotePath` is on by default, so a real `git diff` of `café.py` emits `+++ "b/caf\303\251.py"` — the first cut keyed it with the quotes and no finding in that file could ever anchor. Now unquoted; `.strip()` dropped so a legitimate trailing space survives. - **Model-supplied path rendered raw into markdown.** It can carry backticks, `@` and newlines; demotion to the body turned that from a rare fallback render into a routine one. New `render_code_ref()` neutralizes mentions, strips CR/LF, and sizes the code fence past the longest backtick run. - **Unbounded review body.** Demoted findings render *in* the body, each up to the 20,000-char cap, against GitHub's 65,536-char limit — and the wholesale fallback is strictly larger than what just failed, so an oversize body lost the entire review. Now clamped; the step-summary copy stays whole. - **False "could not be anchored" note** when there were no inline comments to lose. Gated. - **Deferred:** body-only findings carry no thread, so they get no `repeat_of` link and no `REPEAT_CAP` cover. Documented in place as a `# KNOWN GAP` and filed as a follow-up — capping a finding that can never be *answered* is a policy call, and there are two designs that would make the ledger change unnecessary. ## Review round 2 — the degradation paths (81411d3) The panel found seven more, all in the paths this PR made routine. All fixed. - **The truncation note promised a job summary nobody wrote (Medium).** `clamp_review_body` tells the reader "the full text is in the job summary of this run", but `write_step_summary` was only reached on a read-only token or an outright POST failure — never after a *clamped POST succeeded*, which is the one case the clamp exists for. Everything past the cut was absent from both the PR and the summary while the header still reported the full `Found **N**`. A clamped post now writes the whole body to the summary, under a banner saying which degradation happened. - **Universal-newline mode edited the diff before the parser saw it (Medium).** `open(..., encoding="utf-8")` leaves `newline=None`, so Python rewrote every lone `\r` to `\n`. A content line carrying a bare CR — mixed endings, a minified asset — was split in two, and the prefix-less tail either tripped the desync arm or shifted every later anchor in that file into the wholesale 422. `newline=""` hands the splitting back to the parser; it also makes `header_new_path`'s `rstrip("\r")` load-bearing in production rather than only under a direct-call test. - **The fallback re-POST was byte-identical when there was no inline half (Low).** `render_findings_markdown(review_body, [])` returns the body untouched and the suffix is gated off, so with every finding already demoted the "fallback" resent exactly the request that had just failed. It cannot fix a size or malformed-body rejection, and if GitHub committed the write before erroring it published a duplicate review. It now degrades to the job summary and lets the step go red. Trade-off taken knowingly: a genuinely transient 5xx no longer gets a second attempt, but the text is still delivered and the red check says so — versus a duplicate review that no one can un-post. - **`unquote_header_path` only decoded one of git's two quoting modes (Low).** The latin-1 round-trip is correct with `core.quotePath` ON. With it off, git still quotes a name containing `"`, `\` or a control character but leaves its UTF-8 bytes verbatim — a real `é` came back as U+FFFD and an astral character raised `UnicodeEncodeError`, so the key stopped matching the path findings cite and every finding in that file demoted silently. Now only the backslash escapes are resolved and everything else is taken verbatim. - **A finding body was interpolated as raw markdown (Low).** `normalize_comments` applies `neutralize_mentions`, which touches mentions and nothing structural, so model output could open a heading, forge a `**path:line** — …` row, or leave a code fence open across the rest of the review. Demotion moved that from the rare 422 fallback into the primary body on every run. Each finding is now rendered as a blockquote, which confines the structure while leaving legitimate fenced code rendering normally. - **The clamp cut the wrong end of the fallback body (Low).** The fallback was `review_body` — which already ends with the *demoted* half — plus the inline half appended after it, so trimming from the end dropped the findings that had just lost their anchors while a demoted nit survived. The head is now kept separate from the demoted block, and every body-only render lists all findings once from the severity-sorted list. Severity order holds end to end, so the cut lands on the least urgent. - **An unparseable `@@` header left headers trusted (Nit).** The suggestion was to keep `path` so the file's later well-formed hunks could still anchor. Gone the other way: without the hunk's counts the scan no longer knows where its *content* ends, and a `-- x` / `++ b/evil.py` pair inside it is emitted as `--- x` / `+++ b/evil.py` — read as a header pair, numbering the rest under a path the diff never touched. That is the spoofing 422 this feature exists to prevent, and it was reachable before this change. An unparseable header now desyncs until a `diff --git` line, so the file goes anchorless (fail-safe) and the next file is unaffected. ### Round 2 verification - 12 new cases (58 in `test_post_review.py`, **252** in the cursor-review suite). Each of the five behavioural fixes fails a case when reverted individually. - **Empirical, against real git output.** Generated `git diff` for `d/café"x.py`, `d/🎉"y.py` and `d/plain-café.py` under `core.quotePath` both true and false. The shipped parser returned `d/caf�"x.py` in one mode and `None` (UnicodeEncodeError) for the astral name; the new one returns all three paths correctly under **both** modes. - agents-md-integrity, groom, workflow-pins and public-repo-hygiene suites re-run green; `shellcheck -x` clean. ## Review round 3 — the degradation paths, again (016ce2c) The panel found seven more. Six fixed, one declined with reasoning. - **A bare `\r` escaped the blockquote (High).** `render_finding_entry` split on `"\n"` alone, but CommonMark — and GitHub's cmark-gfm — ends a line on a lone `\r` too, so a body of `safe\r## Forged` stayed in ONE element, took the `> ` prefix on the element, and rendered the heading *outside* the quote. That restores exactly the forged-heading and unterminated-fence escapes round 2 added this function to contain, and an ATX heading interrupts a paragraph, so lazy continuation does not absorb it. Nothing upstream strips control characters: `review-output-mcp.py`'s `validate_finding` checks only type/non-empty/length, and `neutralize_mentions` touches `@` alone. Now splits on `\r\n|\r|\n`. - **The fallback's own failure lost the review entirely (Medium).** When the wholesale re-POST also failed for a non-403 reason, `main()` raised `SystemExit(1)` without ever writing the job summary — contradicting the no-inline branch 25 lines up, which writes it precisely so the text is still delivered, and doing it on the branch carrying *more* content. Reachable on an API outage, a stale `commit_id` after a force-push, or a body-level rejection that dropping the anchors does not fix. - **The step summary had no size budget (Low).** Actions caps `$GITHUB_STEP_SUMMARY` at 1 MiB per step and discards an overflowing upload WHOLE rather than truncating, so the one delivery channel `clamp_review_body`'s note promises could itself vanish — the review absent past the cut on the PR *and* absent from the summary, while the header still reported the full `Found **N**`. Now budgeted in **bytes**, not characters: the cap is on the file and a finding body is not ASCII-only (600k `é` is 1.2 MB), and the cut never severs a multi-byte sequence. Live on the un-adjudicated panel path this PR wires `--diff` into — the judge's 10-finding cap does not apply there and the degraded branch unions all eight cells. - **`post_error_review` was the one unbounded body left (Low).** `--error-message` is `$JUDGE_ERROR`, read straight out of `judge-findings.json`, i.e. unbounded CLI/model text; past 65,536 chars the POST 422s and the error review is lost — on the path whose whole job is to report why the review failed. The message is cut to its own budget rather than the assembled body, because `clamp_review_body` trims the *end*, which here is the closing fence and the re-trigger instruction. Two things the finding did not raise, fixed in the same expression: a ``` run in a stack trace closed the fence early and let the rest render as markdown (the fence now outgrows the longest run inside), and a genuine POST failure here now writes the summary like every other path. - **The counts/content desync guard covered only one direction (Low).** A declared `+count` too SMALL spent the budget while content kept coming; those lines fell through, matched nothing and set no `desynced`, putting the `--- `/`+++ ` header test back in force *inside* hunk content. Closed symmetrically with the too-large arm. The seam that leaves is closed separately: if the miscount is exactly two, the overflow lines ARE the header pair and the desync never fires — so a header pair reached from inside a hunk region is refused, since git always emits `diff --git ` before one. Gated on having seen a `diff --git `, so a prefix-less concatenated `diff -u`, whose files legitimately follow one another, still parses every file. - **Reworded the no-inline-half comment (Nit).** After round 2's `review_head` split the skipped body is measurably not byte-identical to the one that failed. The duplicate-review risk, not identity, is the reason to skip it. - **Declined:** feeding the unfiltered `pr-diff-new.patch` to `--diff` to keep anchors on generated files. The premise is right — that patch gets `diff_excludes` only, since the generated-file exclusions are computed inside `check-pr-size` — but it is `LAST_REVIEWED...HEAD`, not `base...HEAD`. A line changed since the last round but *reverted relative to base* is absent from GitHub's own diff, so unioning it breaks the deliberate subset invariant and sends back the very 422 this PR prevents. The real defect it exposes is upstream (the incremental block shows the panel generated-file churn the reviewed diff excludes); that belongs in the diff-building steps, needs a design call, and is proposed as a follow-up. ### Round 3 verification - 17 new cases (**75** in `test_post_review.py`, **269** in the cursor-review suite). Each of the seven code changes fails at least one case when reverted individually. - **Empirical, against real git output.** The parser produces a byte-identical anchor map to round 2's on a real `git diff` covering a rename, a delete, a binary file, a mode change, CRLF content, a non-ASCII path and a new file, under `core.quotePath` both true and false — so the two new guards cost no anchor on real input. Also re-checked that a real diff whose content lines are `-- x` / `++ b/evil.py` still parses to the correct single-file map. - agents-md-integrity, groom, public-repo-hygiene, refresh-reviewers and workflow-pins suites re-run green; `shellcheck -x` clean. (`coderabbit-config` errors locally on a missing `jsonschema` — pre-existing, reproduces on a clean tree, and CI installs the pinned requirements.) ## Review round 4 — the budgets themselves (9901bd8, e216a8b, 94bc001) The panel found nine. Eight fixed, one declined with reasoning. - **The fence outgrew the body it lived in (Medium).** Round 3 made the fence outgrow the longest backtick run inside the message — but the fence is emitted **twice**, and a 40,000-backtick message is a perfectly legal *capped* message, so the delimiters alone came to 80,002 chars against a 60,000 body limit. `clamp_review_body` then cut the **end**: precisely the closing fence and the re-trigger line the message-side budget was added to preserve, leaving an unterminated fence that renders the clamp note as literal code. The trigger correlates with the path — a degenerate model/CLI repetition loop is both what fills `error` and what produces a run that long. Runs at or over `MAX_FENCE_CHARS` (64) are now broken up *before* the length budget, so the markers the substitution inserts stay inside the budget rather than being appended after it. - **A lone surrogate killed the delivery channel (Medium).** Python's JSON decoder admits `"\ud800"` and `review-output-mcp.py`'s `validate_finding` checks type/non-empty/length only, so a finding body could carry one to `write_step_summary`, where `.encode("utf-8")` raises `UnicodeEncodeError` — taking down the channel that exists so nothing is lost (on the read-only path, the whole review with it). Fixed with one `encodable()` round-trip up front rather than per-encode: the size check was not the only hazard, the `open(path, encoding="utf-8")` write at the end of the same function would have raised too, and `surrogatepass` there would put invalid UTF-8 into the summary file. - **The truncation note promised a run log nothing wrote to (Low).** `STEP_SUMMARY_TRUNCATED_NOTE` sent the reader to the run log while the only prints on that path were status lines — so past the cut the text was gone from the PR (whose clamp note points at the summary) *and* gone from the summary. The panel offered two fixes; **the first one was tried and reverted, on evidence.** Echoing the whole payload to stdout made the promise true locally, and hung CI: the budget only fires near 900 KB, and pushing that into the Actions log took `unittest + shellcheck` past **13 minutes** against the 8s it takes otherwise (suite output went from 3 KB to 4 MB). A delivery channel that stalls the run is not a delivery channel, and in production it would dump ~900 KB into a public Actions log on every truncated summary. Took the second option instead — the note now promises nothing rather than pointing somewhere the text isn't. Independently, the no-summary-file branch was hoisted above the budget, since the 1 MiB cap is a property of the file: a local run prints the full payload instead of a needlessly clamped one. - **`post_error_review` re-opened the gap it had just closed elsewhere (Low).** It omitted `truncated=`, so a clamped-but-successful POST wrote no summary and the clamp note pointed at a document that never existed — the same gap closed for `main()` and `post_or_degrade`, left open on the one caller behind. It also passed the already-cut body as `summary_markdown`, so even the read-only degradation, whose whole purpose is delivering the whole text, delivered the cut copy. - **The fourth exit path delivered nothing (Nit).** The no-findings / all-panel-failed POST failure raised `SystemExit(1)` with no `write_step_summary`. On an API outage that loses the one artifact explaining why no review happened — the panel summary naming which cells errored. All four exit paths now agree. - **A header pair could still be honoured in git-output mode (Low).** Round 3's `saw_git_header and in_hunk_region` gate only re-armed at the next `@@`, so an honoured `+++` opened a window: a pair landing *between* it and the first hunk was honoured, merging spoofed anchors into the real file's set. Replaced with `awaiting_header`, the structural property that actually holds — git emits exactly **one** `--- `/`+++ ` pair per `diff --git `, so the pair is trusted only while this file's own pair is outstanding. That strictly subsumes the hunk-region test. - **The two desync arms disagreed on headers (Low).** Inside the hunk budget the `+`/`-` tests ran before any header exclusion, and the branch is entered while *either* side has budget left — so on a too-large `+count` the next file's `+++ b/y.py` was counted as an added line, **fabricating** an anchor in the previous file and swallowing y.py entirely; and on `@@ -1,2 +1,1 @@` a second `+` still recorded an anchor while `pending_new` went negative. Each prefix is now consumed against its own side's counter. - **`$JUDGE_ERROR` never reached the budget meant for it (Low).** `MAX_ERROR_MESSAGE_CHARS` can only run once Python has started, but the workflow passed the message as a single `--error-message` argv element and Linux rejects one argument over ~128 KiB at `execve` — so the largest errors, exactly the ones the budget was added for, produced no error review and no job summary at all, just a shell-level failure. Bounded in the shell to 100,000 UTF-8 bytes: under `MAX_ARG_STRLEN` with room for the prefix, and still above the char budget so `post-review.py` still emits its own "(truncated…)" note. - **Declined:** re-downloading or hashing `/tmp/pr-diff.patch` to stop an injected `--trust` judge from tampering with it. The judge step and this `post-review.py` invocation are steps in the **same** `consolidate` job, and `CURSOR_REVIEW_ASSETS` resolves under `${{ github.workspace }}` — so an agent that can rewrite the patch can equally rewrite `post-review.py` itself, and a hash check *inside* that script is unenforceable against an attacker who can edit it. Both mitigations read as a boundary without being one. The fix that would help is structural (post from a job the agent-bearing job cannot write to, as `groom.yml` splits its patch and PR jobs) — a design call well beyond this PR, left as a thread comment rather than filed blind. Note this PR does not widen that surface: `/tmp/pr-diff.patch` was already the judge-visible input before it became a `--diff` argument. ### Round 4 verification - 15 new cases (**90** in `test_post_review.py`, **284** in the cursor-review suite), all green. - **Mutation-tested individually:** each code change in `post-review.py` fails at least one case when reverted on its own (the plus- and minus-side counters, the `awaiting_header` gate, the fence break, the surrogate sanitize, the `truncated=` flag, the unclamped summary copy, the no-findings delivery, and the reworded cut note). The shell-side `$JUDGE_ERROR` bound is the one change with no harness — it is YAML-parsed and reasoned about, not executed. - **One fix was reverted on CI evidence,** not on review: the stdout echo above. Caught because the `unittest + shellcheck` job that normally finishes in 8s sat in_progress past 13 minutes. `94bc001` replaces it and the job is green in seconds again; two cases now pin that stdout stays empty on both the ordinary and the truncated path, so this cannot come back silently. - **Regression-pinned, not just fixed.** The two parser changes ship with the shapes that must keep working: a prefix-less concatenated `diff -u` (consecutive header pairs stay legitimate where no `diff --git` exists), ordinary multi-file git output, and a well-formed mixed add/remove/context hunk. - **Merged result checked.** `main` moved since this branch was cut (including the `AGENTS.md` trim). Merged `origin/main` into the branch locally and re-ran every suite on the merged tree — green, and `check_agents_md.py` passes there — so there is no semantic conflict waiting in the merge queue. The merge was then discarded; the branch still merges cleanly on its own. - agents-md-integrity (46) and groom (365) suites re-run green; `shellcheck -x` clean; `cursor-review.yml` re-parsed as YAML after the shell edit. ## Review rounds 5–6 — real-world paths and the last of the budgets (e242ea8) The panel re-raised nine findings across two rounds. Eight fixed, one taken in half, one declined with reasoning. - **git's tab-terminated header path (High).** git terminates the `--- `/`+++ ` path with a **TAB** whenever the name contains a space. Confirmed against git itself before touching anything: `--- a/my dir/app.py\t`, `--- "a/caf\303\251 x.py"\t` when quoted (the tab lands *after* the closing quote), and `--- a/trail \t` for a name ending in a space. Stripping only `\r` keyed the file as `my dir/app.py\t`, which no finding's `file` can ever match — so every finding in a spacey-path file lost its anchor and demoted to the body, taking its thread, its `repeat_of` link and its `REPEAT_CAP` cover with it. A regression against pre-`--diff` behaviour, where those comments anchored fine. Now split off one tab-delimited suffix, *before* the quoted-path test. Safe in both quoting modes: a name that really contains a tab is a control character, which git quotes as `\t` even under `core.quotePath=false`, so an unquoted target never carries a literal one. The legal trailing space still survives. - **Both counts over-declared swallowed the next file's header pair (Medium).** Round 4 gated each prefix on its own counter, which covers a one-sided over-declaration. When **both** sides are over, the pair is consumed as content — the `--- ` on the `-` arm and the `+++ ` on the `+` arm — so neither that gate nor the spent-budget arm fires: the `+++ ` fabricates an anchor in the previous file and the following `@@` renumbers the *next* file under the stale path (`{"x.py": {1,2,3,500,501}}`, y.py absent). The `+` arm now refuses an eaten header **pair** and desyncs until a `diff --git `. Detecting the pair rather than desyncing on either form alone is deliberate: a genuine diff *of* a patch file legitimately carries removed `-- x` / added `++ y` content. It can still trip this, and demoting that one file is the fail-safe direction — a single wrong anchor costs every anchor in the request. - **A `--- ` on a spent budget was honoured unconditionally (Low).** Round 3's exclusion of the header forms from the spent-budget desync arm left the `--- ` branch ungated, because the `saw_git_header` guard sits only on the `+++ ` side — so an overflow line rendering `--- x` fell through untouched and the next `@@` fabricated anchors under the still-current path. Now gated on `awaiting_header or not saw_git_header`. `awaiting_header` is the precise condition rather than a bare `not saw_git_header`: git *does* emit a header pair on a zero budget, right after each `diff --git `, which is exactly when `awaiting_header` is True. A prefix-less concatenated `diff -u` is unaffected. - **`@@ -1,0 +0,3 @@` produced a shifted anchor set (Low).** The `path is not None and right` truthiness test read the `+0` start as "no hunk header yet", skipped the first added line and recorded the second and third as 1 and 2 — anchors on lines the diff never carried, where every other malformed-header case drops the file. Rejected at parse time on `new_start == 0 and new_count > 0`, which real diff output never emits; `@@ -1,3 +0,0 @@` (a whole-file delete) still parses as the legal anchorless hunk it is. - **The surrogate scrub was per-channel (Low).** `encodable()` ran only in `write_step_summary`, so a lone surrogate in a finding body — which `json.load` produces from `"\ud800"` and `validate_finding` does not reject — rode into `json.dumps` and reached the API as a literal `\ud800` escape. The wholesale fallback carried the same escape and failed the same way, leaving the review only in the job summary: the one copy that *was* sanitized, so the two channels disagreed on the finding's text. Now applied in `normalize_comments` (covering the body and the `repeat_of` line) and in `clamp_review_body`, the choke point every posted body passes through. - **Two notes promised a full text the summary can cut (Low).** `TRUNCATED_SUMMARY_NOTE` said "The full text is below" and `clamp_review_body`'s said "in the job summary of this run", but `write_step_summary` has its own byte budget and cuts too — so both could be false at once, on the same oversize body, while the header still reported the full `Found **N**`. Both now say "as much of it as fits". Softening beat having `write_step_summary` report back: the PR body is posted *before* the summary is written, so a return value cannot retroactively correct the note the reader already holds. - **The run-log line promised a payload nothing prints (Low).** "The whole payload follows." was true until `94bc001` deleted the stdout echo in the same commit, and then sent an operator hunting the log for ~900 KB deliberately never written there. Now says the remainder is not delivered anywhere. - **Dead `$JUDGE_ERROR` extraction on the panel-degraded branch (Medium).** That branch passes `--notice` with `$JUDGE_STATUS` and never `--error-message`, so the value had nowhere to go — but under the step's `bash -e` a standalone assignment inherits its command substitution's exit status, so a `UnicodeEncodeError` from a lone surrogate in `error` would have exited the step before the raw un-adjudicated panel review was posted. Deleted, with a comment in its place recording why it must not come back. - **A byte cut cannot guarantee a character-side note (Low).** Round 4 bounded `$JUDGE_ERROR` to 100 KB on the claim that this "still exceeds" `MAX_ERROR_MESSAGE_CHARS` — true only at ≥2.5 bytes per character. 100 KB of emoji is 25,000 characters and CJK roughly 33,000, both under the 40,000 budget, so a severed message reached `post_error_review` looking complete. Raising the byte cut until that were impossible would breach `MAX_ARG_STRLEN`, so the marker is appended **in the shell**, only when the cut actually fired. - **Taken in half:** a hunk left incomplete at EOF. The silence was real, so a mid-hunk cut now says so on stderr, naming the file — otherwise that demotion is indistinguishable from a finding the model simply placed outside the diff. I did **not** drop the file or `return None`, as suggested. The other counts-disagree arms desync because the scan has lost track of the numbering and would record *wrong* lines from there on; here nothing follows the cut, so every line already recorded sits inside a real hunk and anchors correctly. `{"x.py": {1}}` is a correct map, merely incomplete, and it still honours the stated contract. Dropping the file would demote the line-1 finding too for no safety gained, and `return None` would send the unanchorable findings inline — the 422 this function exists to prevent. Reachability also argues against a behaviour change: the reviewed diff is written whole by `check-pr-size --reviewed-diff-out` or a plain `git diff` redirect, with no byte cap. - **Declined:** neutralizing a leading `::` before an echo, to stop model text forging Actions workflow commands. The echo it cites was `print(payload)`, deleted in `94bc001` — the same deletion the "whole payload follows" finding is about. What remains there is a fixed status string with no model text in it. The only surviving `print(payload)` is `write_step_summary`'s fallback for an unset `GITHUB_STEP_SUMMARY`, a local-run path; on an Actions runner that variable is always set, so on every CI path the payload goes to the summary file, which the runner does not scan for workflow commands. Left as a thread reply rather than a ticket, since it cannot trigger today. ### Rounds 5–6 verification - 21 new cases (**111** in `test_post_review.py`, **305** in the cursor-review suite), all green. - **Mutation-tested as a batch:** 12 of the 21 fail against the previous code; the other 9 pin behaviour that must not regress (an ordinary multi-file `git diff`, a prefix-less `diff -u` starting its next file, a well-formed mixed add/remove/context hunk, a whole-file delete's `+0,0` header, a complete final hunk, and a legitimate removed/added pair adjacent to the new header-pair guard). - **Empirical, against real git output.** The tab finding was confirmed by generating `git diff` for `my dir/app.py`, `café x.py`, `sp ace"q.py`, `we<TAB>ird.py` and a name ending in a space, under `core.quotePath` both true and false — which is also where the "a tab-bearing name is always quoted" premise the fix rests on comes from. The reworked `$JUDGE_ERROR` expression was executed against a 200 KB message (marker present, ~100,070 bytes, inside `MAX_ARG_STRLEN`), a short one (no marker), a lone surrogate and a non-string `error`. - agents-md-integrity (46), groom (365) and bump-callers (437) suites re-run green; `shellcheck -x` clean; `cursor-review.yml` re-parsed as YAML and `bash -n`-checked after the shell edits; `actionlint` output byte-identical to the base (its one finding is pre-existing and on an untouched line). - **Merged result re-checked.** `main`'s only movement since the merge-base is an `AGENTS.md` trim, a caller SHA bump and a `pr-risk` docs change — nothing under `.github/cursor-review/` or in `cursor-review.yml` — so there is no semantic conflict waiting in the merge queue, and no merge-group run has ever been ejected for this PR. ## Verification - `tests/test_post_review.py`, **42 cases**: new-side numbering (added/context/removed, multi-hunk, multi-file, `+++ /dev/null`, unparseable header, `\ No newline` marker), header spoofing, line-splitting, C-quoted/CRLF/trailing-space paths, unusable-vs-anchorless, markdown escaping, the body budget, the fallback suffix, the partition, all fail-open paths, and end-to-end payload assertions driving `main()` against a stubbed `gh` — including the exact field regression (10 findings → 9 inline + 1 in the body, one POST, no fallback). - Full suite: **236 passed**, `python3 -m unittest discover -s .github/cursor-review/tests`. Also re-ran agents-md-integrity, groom, workflow-pins and public-repo-hygiene: all green. - **Mutation**: removing the partition call fails 3 of the new cases. - **Empirical, not just unit-tested.** Generated a real `git diff` containing an added line `++ b/evil.py`, a C-quoted `café.py`, a deleted file and a new file. The pre-hardening parser produced `{"app.py": [1,2], "evil.py": [3,4,5,6]}` and keyed café as `"b/caf\303\251.py"`; the hardened one produces `{"added.py": [1,2], "app.py": [1..7], "café.py": [1,2]}`, with the deleted file contributing nothing. That is the High finding reproduced and closed against real git output rather than a hand-written fixture. - `post-review.py` had **no test file at all** before this; these are its first coverage. ## Judgment calls - **Pre-filter rather than retry-on-422.** A retry would need GitHub to tell us *which* position was rejected; the 422 body doesn't reliably say, so the retry would be a bisect (up to N extra POSTs) or a wholesale drop — which is what we already do. The diff is right there, so filtering is one read and zero extra API calls. - **Body prose, not silent suppression.** An unanchorable finding is still a finding; it is rendered with its `file:line` under a line saying why it isn't inline. - **The `a/`/`b/` prefix strip was deliberately *not* generalized.** The reviewed diff is generated by this workflow with a plain `git diff`, so `diff.noprefix`/`diff.mnemonicPrefix` are unreachable, and accepting a bare single-char prefix would mangle a real repo-root path like `b/thing.py`. - **Untouched:** the read-only-token degradation, the no-findings/error paths, and the repeat cap. - **`--diff` is optional** so a caller pinned to an older `workflows_ref` keeps working unchanged — the script and the workflow move together, but nothing breaks if only one does. ## Residual - **Not exercised against a live PR.** Everything here is against the real code with a stubbed `gh` plus real `git diff` output; the end-to-end proof is the first round this posts after merge, where the expected signature is anchored comments plus a body block instead of an all-prose review. - **Judge behaviour unchanged.** The model can still cite a line outside the diff; this makes that cost one finding's anchor instead of all of them. Teaching the judge prompt to cite only diff-carried lines would reduce the body-rendered cases further and is not attempted here. - **Body-only findings are absent from the prior-review ledger** — documented in code, deferred to a follow-up (see round 1 above). - **`AGENTS.md` not updated.** No doc in this repo describes the posting path's anchor semantics today, so there was nothing to keep in step; if you'd rather this be written down, say where and I'll add it. ## Provenance - **Authored by:** agent-work loop - **Verified:** 305 passed in the cursor-review suite (111 in test_post_review.py); agents-md-integrity, groom, public-repo-hygiene, refresh-reviewers and workflow-pins suites re-run green; shellcheck -x clean. coderabbit-config errors locally on a missing jsonschema and reproduces the same way on a clean tree — pre-existing, CI installs the pinned requirements. Round 1's header-spoofing and C-quoted-path fixes, round 2's both-quoting-modes fix and round 3's two new parser guards reproduced and closed against real `git diff` output, the last confirmed to produce a byte-identical anchor map to round 2's across a rename, delete, binary, mode change, CRLF, non-ASCII path and new file under `core.quotePath` both modes. Each round-2, round-3 and round-4 code change in `post-review.py` fails a test when reverted individually; round 4's shell-side `$JUDGE_ERROR` bound is the one change with no harness. Round 4 additionally merged `origin/main` into the branch locally and re-ran every suite on the merged tree (green, `check_agents_md.py` included) to rule out a semantic conflict in the merge queue, then discarded the merge. Rounds 5–6 add 21 cases, 12 of which fail against the previous code and 9 of which pin behaviour that must not regress; the tab-terminated-header fix and the premise it rests on (a tab-bearing name is always quoted) were confirmed by generating real `git diff` output for five awkward paths under `core.quotePath` both modes, and the reworked shell-side `$JUDGE_ERROR` expression was executed against oversize, short, lone-surrogate and non-string inputs — so it is no longer the one change without a harness. bump-callers (437) re-run green; `cursor-review.yml` `bash -n`-checked and `actionlint` output byte-identical to the base. `main`'s movement since the merge-base touches no file this PR touches, and no merge-group run has ever been ejected for this PR. - **Deviations:** several panel findings addressed by something other than the suggestion, each with its reasoning on the thread and in the round section above. Round 1's ledger gap is deferred rather than fixed — documented in code, filed as a follow-up, and needing a policy decision on capping an unanswerable finding. Round 3's generated-file anchor suggestion is declined outright: unioning the incremental patch into `--diff` would break the reviewed-diff-is-a-subset invariant and reintroduce the 422 this PR prevents; the upstream inconsistency it exposes is proposed as a separate follow-up. Round 2's unparseable-hunk-header nit was also addressed in the opposite direction to the suggestion — reasoning in the round-2 section above. Round 4 declines the diff-tampering hardening outright — the proposed mitigations are no-ops given that the judge step and the posting step share a job and a writable workspace, and the structural fix is a human design call; addressed as a thread comment, not a ticket. Round 4 also addressed the too-large-`+count` finding by the counter half of the suggestion rather than excluding header forms inside the hunk budget, which would have desynced legitimate diffs of files containing `++ `/`-- ` lines — reasoning in the round-4 section above. Rounds 5–6 take one finding in half and decline one outright. The incomplete-hunk-at-EOF finding is answered with a stderr note rather than the suggested drop-the-file or fail-open: nothing follows an EOF cut, so the short map is correct rather than untrustworthy, and both suggested remedies would cost correct anchors — reasoning in the rounds-5–6 section above. The workflow-command-injection finding is declined as unreachable: the echo it cites was deleted earlier in this same PR, and the one surviving stdout print is a local-run path that no CI run takes. Both were answered on their threads rather than filed as tickets. Closes BE-9531 --- <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:37 +00:00