fix(pr): refresh re-fetches the PR description under a digest guard (#85) - #131
Conversation
) `refresh` replaced `## Diff` every round but never `## PR description`, so from round 2 the scratch described the design the PR had at ingest beside a diff showing what it has now. Secondaries read both, noticed the disagreement, and spent findings on it (2 of 18 on #84, one per vendor, same round). The description now follows the PR the way the diff does: seed and the new `replace-desc` record the SHA-256 of the body they composed, and a body that no longer matches — the one section a primary may legitimately hand-edit — is left alone with exit 3 and a reason, never clobbered. `refresh` treats that skip as non-fatal, prints `PR description not refreshed`, and the command doc tells the primary to relay it and reconcile by hand before seeding copies. The window is not a heading-to-heading candidate like the diff's: PR bodies routinely carry `## ` headings of their own, so it is bounded by the first `## PR description` heading and the digest-verified diff heading instead. Tests: 12 assertions in pr.test.sh, 1 packaging assertion; 9 mutation entries (one SURVIVES-BY-DESIGN) plus the re-pointed seed-desc-propagation entry. Version 1.32.2 → 1.32.3. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
kevin-agrology
left a comment
There was a problem hiding this comment.
Commented inline (4)
Multi-review
Agreed findings (4)
🟠 med — refresh can still pair a pre-push diff with a post-push description because it confirms head before fetching the body. — risk: copies can again describe a different revision than their diff
🟡 low — The SURVIVES-BY-DESIGN reason on pr/desc-window-needs-diff misstates its cover: no test drives replace-desc with an unverifiable diff, so the mutation survives because the path is never exercised, not because a covered outer layer masks it — and losing the "no heading" refusal would still not surface it, since hstart="" then yields an empty window whose digest the record check refuses anyway. — risk: a table entry whose recorded reason is false is the "reads correctly but cannot fail" shape the table exists to catch; a test that strips the diff record and asserts exit 3/untouched would make the reason true
🟡 low — Once the primary reconciles the description by hand, as the round-N step instructs, no later refresh can ever write it again: the body matches no record, there is no record-desc to re-arm the guard, and only ingest --fresh re-seeds — so every subsequent round skips and warns, and the description is only as current as the primary's last manual pass. Neither the command doc nor the PR's assumptions say this. — risk: a one-time hand edit silently turns the fix off for the rest of the review, reintroducing the #85 drift the primary now has to close by hand each round
🟡 low — cmd_replace_desc can die (mktemp, record write, splice, mv) after cmd_replace_diff has swapped the diff and before cmd_record_head writes the head record; the same-round retry refusal keys on that head record, so a re-run is accepted and record-anchors reads the NEW diff under the old keys. The text-mismatch poison then degrades those findings to the summary rather than misplacing them, but silently. This widens a window that previously held only cmd_record_head itself; I could not trigger it without an infra failure. — risk: after an infra failure mid-refresh, anchored findings quietly lose their inline placement on the retry
Disagreements (1)
🟠 med — The description guard accepts every historical digest, so a hand edit that restores a prior body is treated as unedited and overwritten. — risk: intentional primary text can be silently lost — flagged by gpt-5.6; claude-opus-5 disputes: The guard's predicate is "is this body one the tool composed", not "is it the latest"; every recorded digest is a body seed/replace-desc wrote, so a body matching one is by construction not hand-authored prose — accepting the full history implements that predicate correctly, and the record-before-rename crash window requires accepting more than the newest in any case.
Quarantine events (secondary did not review that round; its findings for that round are excluded)
- gemini · dispatch exited 41; see pr-131.md.gemini.multi-review.log · round 1
———
🤖 Posted by AI agents (claude-opus-5 + gpt-5.6 + claude-fable-5-1 + gemini (quarantined)) via multi-review star review.
| || die "could not re-confirm the PR head after fetching the diff — re-run refresh for round ${round}" 1 | ||
| [[ "$head_after" == "$head" ]] \ | ||
| || die "the PR moved during refresh (${head} -> ${head_after}) — re-run refresh for round ${round}" 1 | ||
| gh pr view "$n" --repo "${o}/${r}" --json body --jq '.body' > "${tmpd}/desc" \ |
There was a problem hiding this comment.
🟠 med — refresh can still pair a pre-push diff with a post-push description because it confirms head before fetching the body. — risk: copies can again describe a different revision than their diff — 🤖 multi-review star review (gpt-5.6 + claude-opus-5)
| # The description window is bounded BELOW by the verified diff window, so an unverifiable diff | ||
| # must refuse here too. Deliberately redundant: with no diff span, `bstart` is empty, the heading | ||
| # scan's `NR < lim` is a string compare against "" that never holds, and the "no heading" refusal | ||
| # fires — covered by the no-record and hand-edited assertions above. Recorded so that losing that | ||
| # outer layer surfaces here rather than as a silent gap. | ||
| mutate 'pr/desc-window-needs-diff' 'scripts/multi-review-pr.sh' replace \ | ||
| 'SURVIVES-BY-DESIGN' 'multi-review-pr.test.sh' \ | ||
| ' span="$(_locate_diff "$scratch")" || return 3' \ | ||
| ' span="$(_locate_diff "$scratch")" || span=""' |
There was a problem hiding this comment.
🟡 low — The SURVIVES-BY-DESIGN reason on pr/desc-window-needs-diff misstates its cover: no test drives replace-desc with an unverifiable diff, so the mutation survives because the path is never exercised, not because a covered outer layer masks it — and losing the "no heading" refusal would still not surface it, since hstart="" then yields an empty window whose digest the record check refuses anyway. — risk: a table entry whose recorded reason is false is the "reads correctly but cannot fail" shape the table exists to catch; a test that strips the diff record and asserts exit 3/untouched would make the reason true — 🤖 multi-review star review (claude-fable-5-1 + claude-opus-5)
| under the same digest guard as the diff, so one that was hand-edited since it was last | ||
| written no longer matches and is LEFT AS-IS: `refresh` still exits 0 and prints | ||
| `PR description not refreshed` on stderr. Relay that line, and reconcile the description | ||
| against the PR by hand before seeding the copies — do not fan out a description that | ||
| contradicts the diff. |
There was a problem hiding this comment.
🟡 low — Once the primary reconciles the description by hand, as the round-N step instructs, no later refresh can ever write it again: the body matches no record, there is no record-desc to re-arm the guard, and only ingest --fresh re-seeds — so every subsequent round skips and warns, and the description is only as current as the primary's last manual pass. Neither the command doc nor the PR's assumptions say this. — risk: a one-time hand edit silently turns the fix off for the rest of the review, reintroducing the #85 drift the primary now has to close by hand each round — 🤖 multi-review star review (claude-fable-5-1 + claude-opus-5)
| if ! cmd_replace_desc "$scratch" "${tmpd}/desc"; then | ||
| echo "multi-review-pr: PR description not refreshed for round ${round} — reconcile it against the PR by hand before seeding the copies" >&2 | ||
| fi | ||
| cmd_record_head "$scratch" "$round" "$head" "$mb" |
There was a problem hiding this comment.
🟡 low — cmd_replace_desc can die (mktemp, record write, splice, mv) after cmd_replace_diff has swapped the diff and before cmd_record_head writes the head record; the same-round retry refusal keys on that head record, so a re-run is accepted and record-anchors reads the NEW diff under the old keys. The text-mismatch poison then degrades those findings to the summary rather than misplacing them, but silently. This widens a window that previously held only cmd_record_head itself; I could not trigger it without an infra failure. — risk: after an infra failure mid-refresh, anchored findings quietly lose their inline placement on the retry — 🤖 multi-review star review (claude-fable-5-1 + claude-opus-5)
`refresh` confirmed the head after fetching the diff but before fetching the description, leaving the body outside the confirmation window. A push landing in that gap paired a pre-push diff with a post-push description — the #85 drift this command exists to close, reopened for the description half. Raised as codex-rd1-r1 in the multi-review of PR #131. Fetch first, confirm last: the body read now sits between the diff read and the confirmation, so every content read is covered by it. Also pins the confirmation guard itself. It had no covering assertion and no mutation table entry — deleted, the whole suite stayed green, which is the "reads correctly but cannot fail" shape the table exists to catch. It is now load-bearing for the description too, so it gets a named assertion and an entry rather than being left uncoverable in a block being edited. Both tests were watched failing first: the reproduction at rc=0 with the post-push description spliced beside the pre-push diff, and the head-moved test with the guard removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fable-rd1-r1 — pr/desc-window-needs-diff was recorded SURVIVES-BY-DESIGN behind the "no heading" refusal, and that reason was false twice over: nothing drove replace-desc through a failing _locate_diff, and deleting the named outer layer as well still refuses at the record check, so no assertion could ever have surfaced its loss. What the fall-through really costs is the diagnosis — the reader is told the document lacks a '## PR description' heading it plainly has. Pinned by an assertion on that diagnosis; the entry is a real caught entry now, not a survivor. fable-rd1-r2 — a hand reconcile of the description is one-way: the edited body matches no record, nothing re-arms the guard, and every later round skips it too. The command doc said none of this, so the primary could believe a later round would pick it back up. Documented where the reconcile is instructed, with a packaging assertion and an entry. fable-rd1-r3 — cmd_replace_desc's die paths ran after the diff swap and before the head record. Since die exits, an infra failure there left the round retryable, and a re-run's record-anchors read the new diff under the old keys and poisoned them, silently degrading anchored findings to the summary. The description swap is already the optional half of refresh, so its fatal paths are now contained in a subshell too, not just its exit 3. Each test was watched failing first: the diagnosis assertion under the mutation it pins, the doc assertion before the text existed, and the retryable-window assertion at rc=1 with the diff swapped and no head record written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TL;DR: In a multi-round PR review,
refreshupdated the diff every round but never the PR description, so from round 2 the scratch described the design the PR had when it was ingested next to a diff showing what it has now. Reviewers noticed the disagreement and spent findings on it.refreshnow re-fetches the description too, and refuses to overwrite one that a person edited by hand.Closes #85.
What changed
seedrecords a digest of the description body it composed, alongside the diff digest it already records, as amulti-review-pr-desc:line in the.recordssidecar.replace-desc <scratch> <desc-file>subcommand swaps## PR descriptionunder that digest guard. The current body must match a recorded digest; otherwise it exits 3 with a reason and writes nothing. An identical body is a no-op (no write, no new record). The splice followsreplace-diff's discipline: record before rename, temp beside the scratch, component-by-component propagation.refreshfetches the PR body and callsreplace-descafter the diff swap. A refused swap is not fatal: it means someone edited the description on purpose, so the round proceeds on the diff. The skip is printed (PR description not refreshed for round N) and the command doc tells the primary to relay it and reconcile by hand before seeding copies.refreshresolves the head, fetches the diff and the body, then re-confirms the head and refuses if it moved. Confirming between the two reads left the body outside the window, so a push in that gap paired a pre-push diff with a post-push description.cmd_replace_descruns in a subshell insiderefresh:diecallsexit, so an infra failure there used to killrefreshafter the diff swap and before the head record, and the same-round retry refusal keys on that record.commands/multi-review.mdand thepr.shline in the README.Design note: how the description window is located
The diff window is located as a
## Diff-to-next-heading candidate matched by digest. That does not work for the description: PR bodies routinely carry column-0##headings of their own (hunk lines are prefixed; prose is not), so such a candidate would end at the body's first heading and never verify. The description window is instead bounded by two trusted lines: the first## PR descriptionheading (above it seed writes only single-line header fields, and the title is single-line on GitHub, so nothing author-written can forge one earlier) and the heading of the digest-verified diff window. The digest then decides whether what lies between is still a body a writer composed. A decoy## PR descriptionplanted inside the body cannot move the window; a test covers that.Assumptions and scope
refreshwith a digest guard). Option 3 (refuse the whole refresh) was rejected as too strict for prose: the round can proceed on the diff, and the skip is loud.refreshleaves its description alone and says so;ingest --freshre-seeds with the record. No migration code.replace-diff's; no new mechanism.record-descsubcommand to re-arm the guard deliberately is the obvious follow-up; it is new behaviour, not a fix, so it is not in this PR.Verification
Tests written first and watched fail for the expected reasons, then pass. That includes the review fixes below: the confirmation-window test went red at
rc=0with the post-push description spliced beside the pre-push diff, the retryable-window test atrc=1with the diff swapped and no head record written, and the diagnosis test under the exact mutation it pins.multi-review-pr.test.sh, 2 inmulti-review-packaging.test.sh./bin/bash3.2; shellcheck clean; version check 1.32.2 → 1.32.4.pr/seed-desc-propagationandpr/refresh-fetches-desc, each probed with--only. All caught by a named assertion except one recordedSURVIVES-BY-DESIGNwith its covered outer layer named (pr/seed-desc-cat-propagation).--verify-tablematches at 371 entries.Review
Multi-reviewed as a star review (primary
claude-opus-5; secondariescodex/gpt-5.6 andclaude-fable-5-1admitted,geminiquarantined on an auth failure). Converged in one round: 5 findings, 4 agreed and fixed here, 1 disputed.The dispute is recorded on the PR: that the guard accepting every historical digest lets a hand edit restoring a prior body be overwritten. Refuted — the guard's predicate is "is this a body the tool composed", and every recorded digest is tool output, so a match is by construction not hand-authored prose; the record-before-rename crash window requires accepting more than the newest in any case.
One agreed finding was about this repo's own mutation table rather than the shipped code:
pr/desc-window-needs-diffwas recordedSURVIVES-BY-DESIGNbehind a cover that does not hold — nothing drovereplace-descthrough a failing_locate_diff, and deleting the named outer layer as well still refuses at the record check, so no assertion could ever have surfaced its loss. What the fall-through actually costs is the diagnosis: the reader is told the document lacks a## PR descriptionheading it plainly has. That is now pinned by an assertion and the entry is a real caught entry.Security
Not security-relevant. The description remains untrusted author text, as before; the new window is bounded by two lines the author cannot forge, and a body that does not match a recorded digest is never written over. No new dependencies, no new network calls beyond the existing
gh pr view --json bodythatingestalready makes.🤖 Opened by an AI agent — Claude Fable 5.1 (claude-fable-5-1); review fixes and this description by Claude Opus 5 (claude-opus-5). Both run by the maintainer, who remains accountable for the contents.