[PR #278] [MERGED] fix(cursor-review): prefix continuation lines of imported ledger prose so no finding or reply can forge a metadata line #280

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-12629-prose-continuation-marker

📝 Commits (2)

2790739 fix(cursor-review): prefix continuation lines of imported ledger prose
363f48a fix(cursor-review): flatten thread-derived paths and tighten the continuation marker (BE-12629)

📊 Changes

3 files changed (+453 additions, -10 deletions)

View changed files

📝 .github/cursor-review/README.md (+3 -1)
📝 .github/cursor-review/build-ledger.py (+82 -9)
📝 .github/cursor-review/tests/test_build_ledger.py (+368 -0)

📄 Description

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: … and thread: … 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

New _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: and reply from …:.
_defang_fences stays inside _prose rather 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 forge discussion_url:). Both fire; neither is sufficient alone.
path and severity are flattened with _body_only_text and then fence-defanged at the render. They are single-line HEADER fields where _prose never runs, and a body-only entry was already flattened on the way in — but a thread-derived entry takes path straight from the review comment, and git permits every _LINE_SEP_CLASS separator 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_RE is built from _LINE_SEP_CLASS — the same set _FENCE_LINE_RE and _FIELD_LINE_BREAK_RE use — 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.
The convention is stated once in _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.
The byte cap accounts for the marker. _size measures json.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.
One trailing EMPTY segment is dropped, as 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 the build-ledger.py assets row.

.github/cursor-review/tests/test_build_ledger.py — new TestProseContinuationLines (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-derived path can 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:

Downloaded cursor-review-ledger from run 34273839166 on this repo (a successful cursor-review run, read-only). Its ledger.json carries 6 entries and real multi-line replies. Re-rendering it through origin/main's build-ledger.py and 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).
Grafted a forged \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/main produces 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.
The artifact contains no \r at 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 to ledger.md / ledger-judge.md and spliced into the prompts, while ledger_note reads the structured dict. There is no downstream reader for the prefix to break.

Judgment calls / deviations from the plan as written

_PROSE_LINE_RE matches \r\n as ONE separator (r"\r\n|" + _LINE_SEP_CLASS), where the plan specified re.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\n as 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.split on 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 \n and \r\n.
The separator list is reused from TestLineSeparatorContainment.SEPARATORS, not TestSeparatorClassParity as the plan names — no class by that name exists on main; TestLineSeparatorContainment is the one that pins _FENCE_LINE_RE / _FIELD_LINE_BREAK_RE parity, so the new test extends the same list rather than starting a second one.
The plan sequenced this after the re-raise-lineage PR; that has merged, so this branches from main with it in, and both README touch-points (the ledger paragraph and the assets row) are updated.
No capability-denial falsification was required: the change adds no deny/dead-end path, no "not supported"/"unavailable" string, and flips no test to assert a dead-end. It is additive and preserves the full text of every quoted line — the trigger for that check does not fire here.

Residual

post-review.py's render_repeat_of still publishes the judge's repeat_of URL without checking it against the entries the judge was shown. This change removes the supply side (untrusted prose can no longer write a discussion_url: or re_raise_of: line for the judge to copy), but the consumer-side validation is deliberately out of scope and is tracked separately. What render_repeat_of does today is a shape check only: REPEAT_URL_RE requires https://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.
The originating read-only investigation's findings comment lives on the tracker and is not reachable from here, so the line/column citations in the plan were re-derived against the current file rather than taken on trust — they had moved (the referenced line numbers predate the re-raise-lineage change). The two prose sites, the fence defang and the truncation points were each re-located and confirmed by reading the file; nothing else from that comment could be verified directly.
The plan's post-merge check is not something this PR can perform: it asks for a glance at the round-2 cursor-review-ledger artifact 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's ledger.json through 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.
The reply author field ( 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.
The byte cap's charge is an over-estimate for \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

Authored by: agent-work loop
Verified: 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.
Deviations: the two listed under "Judgment calls" above — \r\n treated as one separator rather than two, and the separator list reused from TestLineSeparatorContainment since the class the plan names does not exist. One further departure from the first commit's own stated shape: path and severity are now flattened at the render rather than left fence-defanged only, because the review round showed a thread-derived path could otherwise forge a field line. Every acceptance criterion in the plan is met.

_{🔄 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/278 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/9/2026 **Status:** ✅ Merged **Merged:** 9/9/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-12629-prose-continuation-marker` --- ### 📝 Commits (2) - [`2790739`](https://github.com/Comfy-Org/github-workflows/commit/27907397b2415e33c337fcc3e5c52e7d7c7c36f0) fix(cursor-review): prefix continuation lines of imported ledger prose - [`363f48a`](https://github.com/Comfy-Org/github-workflows/commit/363f48a5174d66381f3be1cd6e6d1d9316255aec) fix(cursor-review): flatten thread-derived paths and tighten the continuation marker (BE-12629) ### 📊 Changes **3 files changed** (+453 additions, -10 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+3 -1) 📝 `.github/cursor-review/build-ledger.py` (+82 -9) 📝 `.github/cursor-review/tests/test_build_ledger.py` (+368 -0) </details> ### 📄 Description ## 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: …` and ` thread: … 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` - New `_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:` and ` reply from …:`. - `_defang_fences` stays *inside* `_prose` rather 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 forge ` discussion_url:`). Both fire; neither is sufficient alone. - `path` and `severity` are **flattened** with `_body_only_text` and then fence-defanged at the render. They are single-line HEADER fields where `_prose` never runs, and a body-only entry was already flattened on the way in — but a thread-derived entry takes `path` straight from the review comment, and git permits every `_LINE_SEP_CLASS` separator 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_RE` is built from `_LINE_SEP_CLASS` — the same set `_FENCE_LINE_RE` and `_FIELD_LINE_BREAK_RE` use — 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. - The convention is stated once in `_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. - The byte cap accounts for the marker. `_size` measures `json.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. - One trailing EMPTY segment is dropped, as `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 the `build-ledger.py` assets row. `.github/cursor-review/tests/test_build_ledger.py` — new `TestProseContinuationLines` (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-derived `path` can 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: - Downloaded `cursor-review-ledger` from run `34273839166` on this repo (a successful `cursor-review` run, read-only). Its `ledger.json` carries 6 entries and real multi-line replies. Re-rendering it through `origin/main`'s `build-ledger.py` and 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). - Grafted a forged `\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/main` produces **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. - The artifact contains no `\r` at 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 to `ledger.md` / `ledger-judge.md` and spliced into the prompts, while `ledger_note` reads the structured dict. There is no downstream reader for the prefix to break. ## Judgment calls / deviations from the plan as written - **`_PROSE_LINE_RE` matches `\r\n` as ONE separator** (`r"\r\n|" + _LINE_SEP_CLASS`), where the plan specified `re.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\n` as 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.split` on 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 `\n` and `\r\n`. - **The separator list is reused from `TestLineSeparatorContainment.SEPARATORS`**, not `TestSeparatorClassParity` as the plan names — no class by that name exists on `main`; `TestLineSeparatorContainment` is the one that pins `_FENCE_LINE_RE` / `_FIELD_LINE_BREAK_RE` parity, so the new test extends the same list rather than starting a second one. - The plan sequenced this after the re-raise-lineage PR; that has merged, so this branches from `main` with it in, and both README touch-points (the ledger paragraph and the assets row) are updated. - No capability-denial falsification was required: the change adds no deny/dead-end path, no "not supported"/"unavailable" string, and flips no test to assert a dead-end. It is additive and preserves the full text of every quoted line — the trigger for that check does not fire here. ## Residual - **`post-review.py`'s `render_repeat_of` still publishes the judge's `repeat_of` URL without checking it against the entries the judge was shown.** This change removes the *supply* side (untrusted prose can no longer write a ` discussion_url:` or ` re_raise_of:` line for the judge to copy), but the consumer-side validation is deliberately out of scope and is tracked separately. What `render_repeat_of` does today is a *shape* check only: `REPEAT_URL_RE` requires `https://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. - **The originating read-only investigation's findings comment lives on the tracker and is not reachable from here**, so the line/column citations in the plan were re-derived against the current file rather than taken on trust — they had moved (the referenced line numbers predate the re-raise-lineage change). The two prose sites, the fence defang and the truncation points were each re-located and confirmed by reading the file; nothing else from that comment could be verified directly. - **The plan's post-merge check is not something this PR can perform:** it asks for a glance at the round-2 `cursor-review-ledger` artifact 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's `ledger.json` through 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. - The reply *author* field (` 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. - The byte cap's charge is an over-estimate for `\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 - **Authored by:** agent-work loop - **Verified:** `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. - **Deviations:** the two listed under "Judgment calls" above — `\r\n` treated as one separator rather than two, and the separator list reused from `TestLineSeparatorContainment` since the class the plan names does not exist. One further departure from the first commit's own stated shape: `path` and `severity` are now flattened at the render rather than left fence-defanged only, because the review round showed a thread-derived `path` could otherwise forge a field line. Every acceptance criterion in the plan is met. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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