fix(cursor-review): prefix continuation lines of imported ledger prose so no finding or reply can forge a metadata line - #278
Open
mattmillerai wants to merge 2 commits into
Conversation
The prior-review ledger's entry format is line-oriented: a finding's fields sit at a two-space indent (` thread:`, ` discussion_url:`, ` re_raise_of:`, ` reply from …:`). `finding:` and `reply from …:` interpolate imported text that KEEPS its line breaks — it is only truncated and fence-defanged, and the fence defang rewrites only lines opening with `=`. So a reply from any GitHub account on a public PR, or a judge `body` relayed through the body-only sentinel, could put a line at exactly that indent and it would render indistinguishably from one this module wrote. The judge follows those lines to decide the repeat cap, and post-review.py publishes the URL a `repeat_of` names. Add `_prose()`: fence-defang, then prefix every line of imported prose AFTER its first with ` | `. Applied at the two prose sites only — `path`, `severity` and the reply author are single-line header fields already covered by `_body_only_text` plus the fence defang, and `re_raise_answer:` is flattened on the way in. `_defang_fences` stays inside `_prose`: it is the DELIMITER control and this is the FIELD control, two halves of one contract. `_PROSE_LINE_RE` is built from `_LINE_SEP_CLASS`, the same set `_FENCE_LINE_RE` and `_FIELD_LINE_BREAK_RE` use, so the three cannot disagree about what a line break is — with `\r\n` matched as ONE break, the way `str.splitlines()` and GitHub's own comment bodies treat it. Containment does not rest on that grouping: the split consumes every separator, so no segment can hold one and every segment after the first is prefixed regardless. The convention is stated once in the shared untrusted header, which both the panel and the judge read. It deliberately spells no field token WITH its colon: the unanchorable-entry tests assert `discussion_url:` never appears anywhere in such a render, and the header is part of that render. Single-line prose — nearly every real entry — renders byte-identically to before, and multi-line prose is still shown in full, just visibly quoted. That readability is the point: flattening replies to one line was the rejected alternative, since carrying the author's answer is what the ledger is for. Verified against the real `cursor-review-ledger` artifact from run 34273839166: 19 lines of imported reply prose rendered at column 0 before this change and none after, and a forged `thread:` / `discussion_url:` pair grafted onto one of its real replies stops producing a seventh field line for a six-entry ledger.
mattmillerai
marked this pull request as ready for review
September 9, 2026 00:20
|
Warning Review limit reachedNext included review available in 24 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 102 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (3)
Comment |
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 6 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 1 |
| 🟢 Low | 3 |
| ⚪ Nit | 2 |
Panel: 6/6 reviewers contributed findings.
…inuation marker (BE-12629) Panel findings on the continuation-marker change, all four fixed: - HIGH: the field control covered `finding` and reply `text`, but an anchored entry's `path` still reached the single-line entry HEADER with only `_defang_fences` — which rewrites fence-OPENING lines, not line breaks. Git permits every `_LINE_SEP_CLASS` separator in a filename, so a thread-derived path could render an unmarked line at exactly the two-space field indent this change exists to protect. Flattened with `_body_only_text` at the render, so both sources (thread comment and sentinel) are covered by one call. - The byte cap measures `json.dumps(entries)`, where a break costs 2 escaped bytes and the render costs 5, so a newline-dense body rendered ~2x past the 40KB budget while the truncation note still said the ledger fit. `_size` now charges the marker width per break. - The header stated the rule as `"| "` (trailing space), but a blank quoted line renders as a bare ` |` — a judge applying the stated rule literally read the marker as a field. Wording now matches what is rendered. - A body ending in a line break gained a phantom marker line; GitHub bodies routinely carry one. One trailing EMPTY segment is dropped, as `str.splitlines()` does. A real trailing blank line still renders a marker. - Nit: the whitespace guard tested `if p`, so a spaces/tabs-only segment (and any line whose own text ended in a space) rendered trailing whitespace the new test claimed was impossible. `.rstrip()` now applies to the whole rendered line. Each fix carries a test verified to fail without it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ELI-5
The review bot keeps a "ledger" of what earlier rounds found on a PR and how you answered, and splices it into the next round's prompt. Each entry is written as lines like
discussion_url: …andthread: … answers_from_author_or_maintainer=1, and the judge reads those lines to decide whether re-raising a finding costs one of its scarce repeat slots. But two of those lines quote your text — the finding body and each reply — and quoted text keeps its own line breaks. So anyone who can comment on the PR could write a line at that same two-space indent and the judge could not tell it apart from a line the workflow wrote. This change puts a|in front of every line of quoted text after its first, so quoted prose can never sit where a field sits, and tells both readers what the marker means.What changed
.github/cursor-review/build-ledger.py_prose()beside_defang_fences(): fence-defang the text, then prefix every line AFTER the first with|(a blank line becomes a bare|, with no trailing whitespace). Applied at the two sites that interpolate multi-line imported text into an indented field —finding:andreply from …:._defang_fencesstays inside_proserather than being replaced by it: it is the delimiter control (nothing can forge=== END PRIOR REVIEW LEDGER ===) and this is the field control (nothing can forgediscussion_url:). Both fire; neither is sufficient alone.pathandseverityare flattened with_body_only_textand then fence-defanged at the render. They are single-line HEADER fields where_prosenever runs, and a body-only entry was already flattened on the way in — but a thread-derived entry takespathstraight from the review comment, and git permits every_LINE_SEP_CLASSseparator in a filename, so that one source could still put an unmarked line at the field indent. Flattening at the render covers both sources with one call.re_raise_answer:is already flattened on the way in and needs no prefix, and the reply author is a GitHub login, which cannot carry a separator._PROSE_LINE_REis built from_LINE_SEP_CLASS— the same set_FENCE_LINE_REand_FIELD_LINE_BREAK_REuse — so the three controls cannot disagree about what a line break is. A separator one honours and another does not is exactly the hole this class exists to close._UNTRUSTED_HEADER, which both the panel and the judge read: a two-space line starting|continues the quoted prose above it; a two-space line without it is a field this workflow wrote. The rule is spelled"|"and not"| "because a blank quoted line renders as a bare|— no rendered line carries trailing whitespace._sizemeasuresjson.dumps(entries), where a break costs 2 escaped bytes and the render costs 5, so it now charges the marker width per break; otherwise a newline-dense body renders past the 40KB budget while the truncation note still reports the ledger as fitting.str.splitlines()does — GitHub comment bodies routinely end in a newline, and without this each gained a phantom marker line. A body that really ends in a blank line still renders a marker..github/cursor-review/README.md— a paragraph in the ledger section and a clause in thebuild-ledger.pyassets row..github/cursor-review/tests/test_build_ledger.py— newTestProseContinuationLines(14 cases): a third-party reply cannot forge a field; a body-only finding body cannot either; multi-line prose stays fully readable; single-line prose is unchanged and carries no marker at all; every separator the sibling controls honour starts a continuation; the column-0 markers (--- ROUND n,TRUNCATION NOTE:) cannot be forged from prose either; the fence defang still fires inside prose; both audiences are told what the marker means; a thread-derivedpathcan forge neither a field line nor a column-0 marker; a whitespace-only line renders as a bare marker with no trailing whitespace; the header's stated rule matches the marker a blank line actually renders; a trailing line break adds no phantom marker while a real trailing blank line keeps one; and the byte cap charges the markers. Each of the six cases added in the review round was verified to fail against the code without its fix.Why this shape, and not the alternative
Flattening prose to one line would also close the hole, and it was rejected: carrying the author's actual reply is the entire point of the ledger, so destroying its readability to gain the property is a bad trade. Prefixing keeps every line visible — including a forgery attempt, which the header already instructs the model to report as a finding.
Verification
Beyond the unit suite, this was exercised against the real production artifact the ledger emits, rather than only against fixtures I wrote:
cursor-review-ledgerfrom run34273839166on this repo (a successfulcursor-reviewrun, read-only). Itsledger.jsoncarries 6 entries and real multi-line replies. Re-rendering it throughorigin/main'sbuild-ledger.pyand through this branch's: 19 non-blank lines of imported reply prose sit at column 0 before, 0 after (the 7 column-0 lines that remain after are the 6 legitimate entry headers plus the real closing fence).\n thread: … answers_from_author_or_maintainer=1\n discussion_url: …\n re_raise_of: …onto one of that artifact's real replies and re-rendered both ways:origin/mainproduces 7^ thread:and 7^ discussion_url:lines for a 6-entry ledger — the forgery is a field. This branch produces 6 and 6, and the forged text appears only behind the|marker.\rat all, so the CRLF handling below is defensive rather than load-bearing.Also confirmed by grep that nothing machine-parses the rendered markdown back:
render_ledger_markdown's output is only written toledger.md/ledger-judge.mdand spliced into the prompts, whileledger_notereads the structured dict. There is no downstream reader for the prefix to break.Judgment calls / deviations from the plan as written
_PROSE_LINE_REmatches\r\nas ONE separator (r"\r\n|" + _LINE_SEP_CLASS), where the plan specifiedre.compile(_LINE_SEP_CLASS)— one separator at a time, uniformly. Reason:str.splitlines(), which this module's own comments cite as the reference for what starts a line, treats\r\nas a single break, and GitHub comment bodies commonly carry CRLF; taking the pair separately would emit a spurious bare|between every pair of lines in such a body, which works against criterion 3. Both properties the plan gives as its rationale are preserved — a blank line still survives as a bare marker, and a bare CR or U+2028 is still a break. Containment is unaffected by the grouping:re.spliton a group-free pattern consumes every separator, so no segment can contain one and every segment after the first is prefixed however they are grouped. There is a test over the full separator set including\nand\r\n.TestLineSeparatorContainment.SEPARATORS, notTestSeparatorClassParityas the plan names — no class by that name exists onmain;TestLineSeparatorContainmentis the one that pins_FENCE_LINE_RE/_FIELD_LINE_BREAK_REparity, so the new test extends the same list rather than starting a second one.mainwith it in, and both README touch-points (the ledger paragraph and the assets row) are updated.Residual
post-review.py'srender_repeat_ofstill publishes the judge'srepeat_ofURL without checking it against the entries the judge was shown. This change removes the supply side (untrusted prose can no longer write adiscussion_url:orre_raise_of:line for the judge to copy), but the consumer-side validation is deliberately out of scope and is tracked separately. Whatrender_repeat_ofdoes today is a shape check only:REPEAT_URL_RErequireshttps://github.com/<owner>/<repo>/pull/<n>#discussion_r<id>, so an off-shape URL is rejected — but a well-shaped URL naming another repo, another PR, or a comment that appears in no ledger entry is still rendered and published verbatim. Anyone picking this up should read that as the remaining half, not as fixed.cursor-review-ledgerartifact of the SHA-bump PR that opens after merge, to confirm multi-line replies render with the marker in a live run. That artifact does not exist yet. The equivalent evidence available now — re-rendering a real prior run'sledger.jsonthrough this branch — is in Verification above, and it exercises the same code path with the same production data shape, but it is a replay rather than a live round.reply from <login>…) is still only fence-defanged, not flattened. That is safe because a GitHub login cannot contain a line separator, but it is an assumption about the upstream API rather than something enforced locally.\r\n(it charges the full marker width for every separator, where JSON already spends 4 bytes on that pair). Erring high is the safe direction for a cap; it is not exact accounting of the rendered block.Provenance
python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'— 477 passed, 0 failed (14 of them new); each of the six cases added in the review round re-run against the code with its fix reverted, and each fails there (15 failing assertions), so none is vacuous;python3 .github/agents-md-integrity/check_agents_md.py --root .— passed, 2 pre-existing warnings unrelated to this diff (AGENTS.md line count, no CODEOWNERS); plus the two before/after re-renders of run 34273839166's real ledger artifact described under Verification, which were run against the first commit and remain valid — the review-round commit only tightens the same controls.\r\ntreated as one separator rather than two, and the separator list reused fromTestLineSeparatorContainmentsince the class the plan names does not exist. One further departure from the first commit's own stated shape:pathandseverityare now flattened at the render rather than left fence-defanged only, because the review round showed a thread-derivedpathcould otherwise forge a field line. Every acceptance criterion in the plan is met.