[PR #279] [MERGED] fix(cursor-review): drop a judge repeat_of naming no ledger entry the judge was shown #281

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

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

Base: main ← Head: matt/be-12630-ledger-repeat-membership

📝 Commits (4)

6186fb0 fix(cursor-review): drop a judge repeat_of naming no ledger entry the judge was shown
72158a6 fix(cursor-review): shape-check the re-raise trailer, charge a rejected lineage a cap slot, gate membership on the JUDGE's ledger (BE-12630)
a27a695 docs(cursor-review): a refused repeat link can cost the finding past the cap (BE-12630)
d042380 Merge remote-tracking branch 'origin/main' into matt/be-12630-ledger-repeat-membership

📊 Changes

5 files changed (+807 additions, -17 deletions)

View changed files

📝 .github/cursor-review/README.md (+2 -0)
📝 .github/cursor-review/post-review.py (+246 -16)
📝 .github/cursor-review/tests/test_build_ledger.py (+1 -1)
📝 .github/cursor-review/tests/test_post_review.py (+536 -0)
📝 .github/workflows/cursor-review.yml (+22 -0)

📄 Description

ELI-5

The judge is shown a list of earlier findings on this PR (the "prior-review ledger") and may re-raise one of them by quoting that thread's link. Until now we only checked the link looked right. Now we also check it is actually one of the links the judge was shown. A link it was never shown gets dropped — no re-raise line, nothing carried forward — but the judge's claim that this is a re-raise still costs it one of its two re-raise slots, so citing a bogus link is not a cheaper way to re-litigate than citing a real one.

What changed

.github/cursor-review/post-review.py

New load_shown_repeat_urls(path) -> frozenset[str] | None. Reads the ledger job's ledger.json — the post-cap entry list the judge prompt was rendered from — and returns every thread URL it rendered, mirroring build-ledger.py's own render gates: discussion_url for an anchored entry, the repeat_of lineage key for a non-anchored (demoted re-raise) one, and nothing at all when status != "ok" (the renderer emits an empty block there). Non-string / empty values are skipped; values are .strip()ed.
- No path, missing, unreadable, not JSON, not an object, or no entries list → None (degrade to shape-only) with one stderr line saying why, plus a ::warning:: annotation so a silently disabled guard is visible on the run summary instead of buried in a step log. It never raises — an exception here would cost the whole review over a file that is deliberately optional.
- entries: [] → a real empty frozenset, not None. empty / disabled / unknown ledgers all carry entries: [], and prompt-judge.md permits repeat_of only inside a ledger block ("Emit those two fields on no other finding"), so on those runs the judge was shown no thread and every repeat_of is unfounded.
repeat_url_of(finding, shown_repeat_urls=None) now runs both halves of the writer's contract, in order: SHAPE (REPEAT_URL_MAX_CHARS then REPEAT_URL_RE.fullmatch) and then MEMBERSHIP. The shape pair used to be applied only in render_body_only_sentinel, so the rendered trailer — the prose in a bot-authored comment — carried whatever the judge wrote, unbounded and unvalidated, while the sentinel silently refused it. Both drops log one repr()'d line.
normalize_comments(...) threads the parameter through, keyword-defaulted so every existing caller is unchanged. Both repeat_of (the trailer) and repeat_url (the sentinel key) come off one repeat_url_of call via render_repeat_trailer, so a dropped URL produces neither.
A rejected lineage still costs a REPEAT_CAP slot. normalize_comments records repeat_dropped when the judge emitted a non-empty repeat_of that was refused, and enforce_repeat_cap counts repeat_of or repeat_dropped. Refunding the slot would have made a link the guard drops strictly cheaper than an honest one — a judge could then turn a whole round into uncapped re-litigation just by fabricating links. The finding itself is never deleted for a bad link; only its lineage is. The cap banner is reworded to match ("N re-raise(s) the judge declared"), since a rejected-lineage finding has no original thread to still be open on.
render_repeat_of is deleted — it lost its last caller in this change, and it is exactly the two-call shape render_repeat_trailer exists to prevent. Its rationale moved into that docstring.
main() gains --ledger (default None) and --no-judge-ledger.

.github/workflows/cursor-review.yml — the Post-review step builds LEDGER_FLAGS=(--ledger /tmp/ledger/ledger.json) once and passes "${LEDGER_FLAGS[@]}" to all three post-review.py invocations (judge-ok, raw-panel --notice, error). No new download step: ledger_download already places the file there.

The array exists so the step can append --no-judge-ledger when JUDGE_LEDGER_OK != true. The judge's and the poster's ledger downloads fail independently and the step already tracks them apart for the header banner; only the judge's says what the judge was shown. When it failed, the judge prompt was spliced an empty ledger block, so the shown set is empty however intact this job's own copy of ledger.json may be. --ledger itself stays ungated on the poster's download, which would only swap one graceful degradation for another.

Docs — module docstring and the README's repeat-policy section state the two-layer contract, that shape now covers the trailer as well as the field, and that the drop is whole while the declaration still spends its slot.

build-ledger.py is untouched.

Why membership, and why not a heuristic

A membership check cannot catch "a real root of ours belonging to a different finding" — that root is itself in the shown set. It is not meant to: it catches only resolvable-but-not-shown ids (aged past MAX_ROUNDS, dropped by the byte cap, or never rendered). A path/relation heuristic was deliberately not added — a false negative there would make the re-raise cap-exempt, which is the hole this closes.

Falsification of the capabilities this denies

A repeat_of naming an unshown thread. There are exactly two places build-ledger.py renders a permalink into the prompt block, and both are covered:

discussion_url: <url> (anchored entries) ← entry["discussion_url"]
re_raise_of: <url> and MUST carry repeat_of: <url> (demoted re-raise entries) ← entry["repeat_of"]

Verified by grepping every html_url / url / https:// occurrence in build-ledger.py — no third source exists. Two adjacent checks: the caps mutate entries before the dict is returned, so ledger.json is genuinely the post-cap list; and _defang_fences rewrites only lines opening with a run of =, so a rendered permalink is byte-identical to the string in ledger.json. The ledger the panel reads is rendered from the same entries list, which is what makes the flag correct on the raw-panel branch too.

A malformed repeat_of on the trailer. build-ledger.py:504 (_resolve_lineage) already applied the identical len(...) > REPEAT_URL_MAX_CHARS or not REPEAT_URL_RE.fullmatch(...) pair, and test_build_ledger.py pins the two modules' constants equal — so no URL the reader would have accepted is newly rejected by the writer. The .strip() still runs first, so a trailing newline remains slack around a well-shaped URL rather than a rejection.

A whole round of re-raises on a judge-ledger-less run. --no-judge-ledger is set only where the workflow already asserts the judge ran without prior-round context (LEDGER_DOWNLOAD_JUDGE != success, the same condition behind the "findings may repeat earlier rounds" banner), so the denial fires exactly where the judge provably saw an empty block.

Verification

python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py' — 487 passed, 0 failed.
Mutation-checked, eight guards, each restored afterwards. Removing the trailer shape check fails 7 tests; making the cap ignore repeat_dropped fails 2; dropping the loader's status gate fails 4; dropping its anchored gate, the ::warning::, the --no-judge-ledger short-circuit, the workflow's JUDGE_LEDGER_OK gate, and the flag array on the error branch each fail 1.
python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py' — 316 passed.
python3 -m unittest discover -s .github/agents-md-integrity/tests -p 'test_*.py' — 46 passed.
python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' — 374 passed (1 skipped).
python3 .github/workflow-pins/check_workflow_pins.py — OK, 11 workflows, 0 exempt.
python3 .github/agents-md-integrity/check_agents_md.py --root . — passed (2 pre-existing warnings: AGENTS.md 155 lines, no CODEOWNERS).
shellcheck -x .github/bump-callers/*.sh .github/bump-callers/tests/*.sh — clean.
actionlint .github/workflows/cursor-review.yml — one pre-existing job.workflow_sha property warning at line 548, untouched by this diff. No new findings on the LEDGER_FLAGS array.

Judgment calls

A rejected lineage keeps the finding but spends the slot. The alternative offered in review was to reject the finding outright; a hallucinated lineage field should not be able to delete a real severe finding. Consequence, stated plainly: with REPEAT_CAP = 2, three declared re-raises now cost one finding whatever their URLs' validity — which is what three valid re-raises have always cost, and it is disclosed in the review head.
The drop's stderr lines use {url!r}, truncated. This is relayed model text reaching a step log that GitHub parses for ::workflow-command:: at the start of a line, and that log is world-readable on a public repo — a newline in the value must not be able to start a line. This matters more now that the module writes real ::warning:: annotations to stdout.
The ::warning:: text is a fixed literal, no path and no exception string, for the same reason; the detail stays on stderr. The "no --ledger given" branch does not annotate — that is a caller's choice, not a failure.
The loader is called at the normalize_comments site, not the top of main(). The error-review and no-findings paths return earlier and adjudicate no repeat_of at all.
except (OSError, ValueError, RecursionError) rather than a bare except. JSONDecodeError and UnicodeDecodeError are both ValueError; RecursionError is the one non-ValueError json.load raises on input it cannot parse. A bare except would also swallow KeyboardInterrupt and genuine bugs in the module.

Residual

Verified against fixtures, not a live run. Every assertion here is unit-level plus a static grep of cursor-review.yml; no cursor-review workflow run was executed against a real PR with a multi-round ledger, and none of the fleet's consumer repos were exercised. The first live exercise is this PR's own review, whose ledger is a first round (entries: []) and therefore only exercises the empty-set branch.
REPEAT_URL_RE hardcodes github.com. Pre-existing in both modules and pinned by test_build_ledger.py; a GitHub Enterprise host's permalink was already unresolvable on the reader and already dropped from the sentinel, so applying the same pattern to the trailer aligns the two rather than narrowing support. Not addressed here.
The referenced spike's evidence was not readable from here. The plan cites an upstream investigation whose findings comment was not reachable from this environment; the implementation follows the plan text as given plus the falsification above, not that comment.
(Resolved in review: the split-download skew previously listed here is now covered by --no-judge-ledger.)

Provenance

Authored by: agent-work loop
Verified: unittest cursor-review suite: 487 passed, 0 failed; workflow-pins: 316 passed; agents-md-integrity: 46 passed; groom: 374 passed (1 skipped); eight-guard mutation check, all restored; check_workflow_pins.py: OK (11 workflows); check_agents_md.py: passed (2 pre-existing warnings); shellcheck: clean; actionlint: no new findings
Deviations: the drop's stderr lines print the URL repr()'d and truncated rather than bare (rationale above); the re-raise cap now counts a declaration whose URL was refused, which can cut a finding a fabricated link would previously have smuggled past the cap

_{🔄 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/279 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/9/2026 **Status:** ✅ Merged **Merged:** 9/12/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/be-12630-ledger-repeat-membership` --- ### 📝 Commits (4) - [`6186fb0`](https://github.com/Comfy-Org/github-workflows/commit/6186fb0fae8af02204e636cac226e3f0e1262474) fix(cursor-review): drop a judge repeat_of naming no ledger entry the judge was shown - [`72158a6`](https://github.com/Comfy-Org/github-workflows/commit/72158a6d69f9b3e7f971b7dd245fb559a655222c) fix(cursor-review): shape-check the re-raise trailer, charge a rejected lineage a cap slot, gate membership on the JUDGE's ledger (BE-12630) - [`a27a695`](https://github.com/Comfy-Org/github-workflows/commit/a27a6959810fa0af86c465e44a3d58e5e58789bf) docs(cursor-review): a refused repeat link can cost the finding past the cap (BE-12630) - [`d042380`](https://github.com/Comfy-Org/github-workflows/commit/d04238044fe5eda403735f706528c07e5d841ce5) Merge remote-tracking branch 'origin/main' into matt/be-12630-ledger-repeat-membership ### 📊 Changes **5 files changed** (+807 additions, -17 deletions) <details> <summary>View changed files</summary> 📝 `.github/cursor-review/README.md` (+2 -0) 📝 `.github/cursor-review/post-review.py` (+246 -16) 📝 `.github/cursor-review/tests/test_build_ledger.py` (+1 -1) 📝 `.github/cursor-review/tests/test_post_review.py` (+536 -0) 📝 `.github/workflows/cursor-review.yml` (+22 -0) </details> ### 📄 Description ## ELI-5 The judge is shown a list of earlier findings on this PR (the "prior-review ledger") and may re-raise one of them by quoting that thread's link. Until now we only checked the link *looked* right. Now we also check it is actually one of the links the judge was shown. A link it was never shown gets dropped — no re-raise line, nothing carried forward — but the judge's *claim* that this is a re-raise still costs it one of its two re-raise slots, so citing a bogus link is not a cheaper way to re-litigate than citing a real one. ## What changed **`.github/cursor-review/post-review.py`** - New `load_shown_repeat_urls(path) -> frozenset[str] | None`. Reads the ledger job's `ledger.json` — the **post-cap** entry list the judge prompt was rendered from — and returns every thread URL it *rendered*, mirroring `build-ledger.py`'s own render gates: `discussion_url` for an **anchored** entry, the `repeat_of` lineage key for a **non-anchored** (demoted re-raise) one, and nothing at all when `status != "ok"` (the renderer emits an empty block there). Non-string / empty values are skipped; values are `.strip()`ed. - No path, missing, unreadable, not JSON, not an object, or no `entries` list → `None` (degrade to shape-only) with one stderr line saying why, **plus a `::warning::` annotation** so a silently disabled guard is visible on the run summary instead of buried in a step log. It never raises — an exception here would cost the whole review over a file that is deliberately optional. - `entries: []` → a real **empty** frozenset, *not* `None`. `empty` / `disabled` / `unknown` ledgers all carry `entries: []`, and `prompt-judge.md` permits `repeat_of` only inside a ledger block ("Emit those two fields on no other finding"), so on those runs the judge was shown no thread and every `repeat_of` is unfounded. - `repeat_url_of(finding, shown_repeat_urls=None)` now runs **both** halves of the writer's contract, in order: SHAPE (`REPEAT_URL_MAX_CHARS` then `REPEAT_URL_RE.fullmatch`) and then MEMBERSHIP. The shape pair used to be applied only in `render_body_only_sentinel`, so the rendered **trailer** — the prose in a bot-authored comment — carried whatever the judge wrote, unbounded and unvalidated, while the sentinel silently refused it. Both drops log one `repr()`'d line. - `normalize_comments(...)` threads the parameter through, keyword-defaulted so every existing caller is unchanged. Both `repeat_of` (the trailer) and `repeat_url` (the sentinel key) come off **one** `repeat_url_of` call via `render_repeat_trailer`, so a dropped URL produces neither. - **A rejected lineage still costs a `REPEAT_CAP` slot.** `normalize_comments` records `repeat_dropped` when the judge emitted a non-empty `repeat_of` that was refused, and `enforce_repeat_cap` counts `repeat_of or repeat_dropped`. Refunding the slot would have made a link the guard drops strictly *cheaper* than an honest one — a judge could then turn a whole round into uncapped re-litigation just by fabricating links. The finding itself is never deleted for a bad link; only its lineage is. The cap banner is reworded to match ("N re-raise(s) the judge declared"), since a rejected-lineage finding has no original thread to still be open on. - `render_repeat_of` is **deleted** — it lost its last caller in this change, and it is exactly the two-call shape `render_repeat_trailer` exists to prevent. Its rationale moved into that docstring. - `main()` gains `--ledger` (default `None`) and `--no-judge-ledger`. **`.github/workflows/cursor-review.yml`** — the Post-review step builds `LEDGER_FLAGS=(--ledger /tmp/ledger/ledger.json)` once and passes `"${LEDGER_FLAGS[@]}"` to all three `post-review.py` invocations (judge-ok, raw-panel `--notice`, error). No new download step: `ledger_download` already places the file there. The array exists so the step can append **`--no-judge-ledger`** when `JUDGE_LEDGER_OK != true`. The judge's and the poster's ledger downloads fail independently and the step already tracks them apart for the header banner; only the **judge's** says what the judge was *shown*. When it failed, the judge prompt was spliced an empty ledger block, so the shown set is empty however intact this job's own copy of `ledger.json` may be. `--ledger` itself stays ungated on the *poster's* download, which would only swap one graceful degradation for another. **Docs** — module docstring and the README's repeat-policy section state the two-layer contract, that shape now covers the trailer as well as the field, and that the drop is whole *while the declaration still spends its slot*. `build-ledger.py` is untouched. ## Why membership, and why not a heuristic A membership check cannot catch "a real root of ours belonging to a *different* finding" — that root is itself in the shown set. It is not meant to: it catches only **resolvable-but-not-shown** ids (aged past `MAX_ROUNDS`, dropped by the byte cap, or never rendered). A path/relation heuristic was deliberately not added — a false negative there would make the re-raise cap-*exempt*, which is the hole this closes. ## Falsification of the capabilities this denies **A `repeat_of` naming an unshown thread.** There are exactly two places `build-ledger.py` renders a permalink into the prompt block, and both are covered: - `discussion_url: <url>` (anchored entries) ← `entry["discussion_url"]` - `re_raise_of: <url>` and `MUST carry repeat_of: <url>` (demoted re-raise entries) ← `entry["repeat_of"]` Verified by grepping every `html_url` / `url` / `https://` occurrence in `build-ledger.py` — no third source exists. Two adjacent checks: the caps mutate `entries` **before** the dict is returned, so `ledger.json` is genuinely the post-cap list; and `_defang_fences` rewrites only lines opening with a run of `=`, so a rendered permalink is byte-identical to the string in `ledger.json`. The ledger the *panel* reads is rendered from the same `entries` list, which is what makes the flag correct on the raw-panel branch too. **A malformed `repeat_of` on the trailer.** `build-ledger.py:504` (`_resolve_lineage`) already applied the identical `len(...) > REPEAT_URL_MAX_CHARS or not REPEAT_URL_RE.fullmatch(...)` pair, and `test_build_ledger.py` pins the two modules' constants equal — so no URL the reader would have accepted is newly rejected by the writer. The `.strip()` still runs first, so a *trailing* newline remains slack around a well-shaped URL rather than a rejection. **A whole round of re-raises on a judge-ledger-less run.** `--no-judge-ledger` is set only where the workflow already asserts the judge ran without prior-round context (`LEDGER_DOWNLOAD_JUDGE != success`, the same condition behind the "findings may repeat earlier rounds" banner), so the denial fires exactly where the judge provably saw an empty block. ## Verification - `python3 -m unittest discover -s .github/cursor-review/tests -p 'test_*.py'` — **487 passed, 0 failed**. - **Mutation-checked, eight guards, each restored afterwards.** Removing the trailer shape check fails 7 tests; making the cap ignore `repeat_dropped` fails 2; dropping the loader's `status` gate fails 4; dropping its `anchored` gate, the `::warning::`, the `--no-judge-ledger` short-circuit, the workflow's `JUDGE_LEDGER_OK` gate, and the flag array on the error branch each fail 1. - `python3 -m unittest discover -s .github/workflow-pins/tests -p 'test_*.py'` — 316 passed. - `python3 -m unittest discover -s .github/agents-md-integrity/tests -p 'test_*.py'` — 46 passed. - `python3 -m unittest discover -s .github/groom/tests -p 'test_*.py'` — 374 passed (1 skipped). - `python3 .github/workflow-pins/check_workflow_pins.py` — OK, 11 workflows, 0 exempt. - `python3 .github/agents-md-integrity/check_agents_md.py --root .` — passed (2 pre-existing warnings: AGENTS.md 155 lines, no CODEOWNERS). - `shellcheck -x .github/bump-callers/*.sh .github/bump-callers/tests/*.sh` — clean. - `actionlint .github/workflows/cursor-review.yml` — one pre-existing `job.workflow_sha` property warning at line 548, untouched by this diff. No new findings on the `LEDGER_FLAGS` array. ## Judgment calls - **A rejected lineage keeps the finding but spends the slot.** The alternative offered in review was to reject the finding outright; a hallucinated lineage field should not be able to delete a real severe finding. Consequence, stated plainly: with `REPEAT_CAP = 2`, three declared re-raises now cost one finding whatever their URLs' validity — which is what three *valid* re-raises have always cost, and it is disclosed in the review head. - **The drop's stderr lines use `{url!r}`, truncated.** This is relayed model text reaching a step log that GitHub parses for `::workflow-command::` at the start of a line, and that log is world-readable on a public repo — a newline in the value must not be able to start a line. This matters more now that the module writes real `::warning::` annotations to stdout. - **The `::warning::` text is a fixed literal**, no path and no exception string, for the same reason; the detail stays on stderr. The "no `--ledger` given" branch does not annotate — that is a caller's choice, not a failure. - **The loader is called at the `normalize_comments` site, not the top of `main()`.** The error-review and no-findings paths return earlier and adjudicate no `repeat_of` at all. - **`except (OSError, ValueError, RecursionError)` rather than a bare `except`.** `JSONDecodeError` and `UnicodeDecodeError` are both `ValueError`; `RecursionError` is the one non-`ValueError` `json.load` raises on input it cannot parse. A bare `except` would also swallow `KeyboardInterrupt` and genuine bugs in the module. ## Residual - **Verified against fixtures, not a live run.** Every assertion here is unit-level plus a static grep of `cursor-review.yml`; no `cursor-review` workflow run was executed against a real PR with a multi-round ledger, and none of the fleet's consumer repos were exercised. The first live exercise is this PR's own review, whose ledger is a first round (`entries: []`) and therefore only exercises the empty-set branch. - **`REPEAT_URL_RE` hardcodes `github.com`.** Pre-existing in both modules and pinned by `test_build_ledger.py`; a GitHub Enterprise host's permalink was already unresolvable on the reader and already dropped from the sentinel, so applying the same pattern to the trailer aligns the two rather than narrowing support. Not addressed here. - **The referenced spike's evidence was not readable from here.** The plan cites an upstream investigation whose findings comment was not reachable from this environment; the implementation follows the plan text as given plus the falsification above, not that comment. - *(Resolved in review: the split-download skew previously listed here is now covered by `--no-judge-ledger`.)* ## Provenance - **Authored by:** agent-work loop - **Verified:** `unittest` cursor-review suite: 487 passed, 0 failed; workflow-pins: 316 passed; agents-md-integrity: 46 passed; groom: 374 passed (1 skipped); eight-guard mutation check, all restored; `check_workflow_pins.py`: OK (11 workflows); `check_agents_md.py`: passed (2 pre-existing warnings); `shellcheck`: clean; `actionlint`: no new findings - **Deviations:** the drop's stderr lines print the URL `repr()`'d and truncated rather than bare (rationale above); the re-raise cap now counts a declaration whose URL was refused, which can cut a finding a fabricated link would previously have smuggled past the cap --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

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

bsakaguchi referenced this issue

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

[PR #281] [MERGED] ci: bump cursor-review to github-workflows@6fe8018 #282

bsakaguchi closed this issue

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