Repository navigation
[Phase 4] already-reviewed-at-head deadlocks on metadata-only findings — the demanded fix can never produce the new commit the re-review requires #1551
Description
Story
As a maintainer relying on pr-review's fix-requested → re-review loop to converge,
I want a metadata-only blocking finding (PR body, labels, linked issues) to be re-reviewable without a new commit,
so that the cascade cannot demand a fix that, once made, it is structurally unable to see.
Problem — a guaranteed deadlock, observed on PR #1531 (2026-08-19)
- 12:06 — pr-review posts
fix requested (cycle 1/3)at headf3f52b22. The sole blocking finding is metadata-only: the PR body'sCloses #1493would auto-close an issue whose ACs 10–12 are staged follow-ups. The prescribed fix: "edit the PR body to 'Refs [Phase 2 follow-up] Complete the #1184 legacy→major-scoped pin migration in the reusable-pin compliance check (split from #687 ACs 7–13) #1493' / 'Part of [Phase 2 follow-up] Complete the #1184 legacy→major-scoped pin migration in the reusable-pin compliance check (split from #687 ACs 7–13) #1493'" — explicitly "not a code change." - ~12:45 — the body is edited;
closingIssuesReferencesis confirmed empty. The demanded fix is complete. - 12:41 — a dispatched re-review no-ops. Run 32253896963 log:
{"pr":"…/pull/1531","sha":"f3f52b22…","decision":"noop","reason":"already-reviewed-at-head"}. Every subsequent dispatch does the same. - The review's own footer says "The review cascade will automatically re-review after new commits are pushed" — but the required fix produces no commit. Deadlock: the verdict blocks the PR, the fix is invisible to the reviewer, and the reviewer refuses to look again.
- Broken only by pushing an empty commit (
3bc26c32) to mint a new head SHA.
The skip logic lives in scripts/review-one-pr.sh (skip reasons: already-reviewed-at-head, ci-failing, ci-pending, changes-requested — see ~line 140) and is surfaced in scripts/review-batch.sh (~line 290). The "already reviewed" fingerprint is the head SHA alone, so PR-metadata changes never invalidate it.
Acceptance Criteria
- After a fix-requested verdict whose blocking findings are all metadata-level, a metadata change (PR body edit, label change, linked-issue change) makes the PR re-reviewable — no new commit and no human intervention required.
- Mechanism suggestion (not prescriptive): include a PR-metadata digest (body hash + closingIssuesReferences + labels) alongside the head SHA in the reviewed-state fingerprint, or treat
pull_request: [edited]as invalidatingalready-reviewed-at-headwhen the last verdict was fix-requested. - Same-SHA no-op behavior is unchanged for PRs whose last verdict was approval or whose findings required code changes — this must not create a re-review churn vector (an attacker or noisy bot editing the body must not burn review cycles; keep the churn breaker authoritative).
- The no-op log line states what would re-arm the review (new commit vs. metadata change), so a stranded PR is diagnosable from the run log.
- Regression test: fix-requested on metadata-only finding → body edit → next review run reviews instead of no-oping.
Tasks
- Extend the reviewed-state fingerprint (or the trigger surface) per AC 1–3.
- Update the verdict footer text to state the actual re-arm condition. (AC: 4)
- Tests. (AC: 5)
Dev Notes
- The empty-commit workaround costs a full CI cycle fleet-wide and pollutes history; it is documented here only as the incident's escape hatch, not a sanctioned path.
- Evidence: PR feat: implement issue #1493 — [Phase 2 follow-up] Complete the #1184 legacy→major-scoped pin migration in the reusable-pin compliance check (split from #687 ACs 7–13) #1531 comments at 12:06 and the run-32253896963 log (noop JSON quoted above).
References
- PR feat: implement issue #1493 — [Phase 2 follow-up] Complete the #1184 legacy→major-scoped pin migration in the reusable-pin compliance check (split from #687 ACs 7–13) #1531 (2026-08-19 incident); escape-hatch commit
3bc26c32 scripts/review-one-pr.sh,scripts/review-batch.sh- Sibling self-deadlocks: [Phase 4] pr-review defers indefinitely on another agent's own check run (dev-lead/dispatch) and on zombie checks — ci-pending has no timeout #1427 (ci-pending, closed), [Phase 4] Initiative driver counts human-gated and zombie sub-issues as in-flight — epic #1402 silently deadlocked at slots=0 for days #1494 (driver slots), and the rate-limit hold issue filed alongside this one
- Epic: Formalize the agentic PR-interaction model (event-first, timers-as-backstop) — clear, repeatable, enforceable #1402
Filed from steering review of epic #1402, 2026-08-19.
Activity
don-petry commentedon Aug 20, 2026
Implementation plan — #1551 (already-reviewed-at-head deadlock on metadata-only findings)
Root cause. The reviewed-state fingerprint in
scripts/review-one-pr.shis the head SHA alone (marker<!-- pr-review-agent v1 sha=<SHA> -->). A metadata-only fix-requested verdict demands a fix (PR body / labels / linked-issue edit) that produces no new commit, so the head SHA is unchanged and every re-review no-ops withalready-reviewed-at-head. Deadlock.Approach (AC2 option a). Add a PR-metadata digest to the fingerprint, but only for fix-requested verdicts that are classified metadata-only. Approval markers and code-change fix-requests carry no digest and keep identical same-SHA no-op behavior (AC3). The churn breaker (cycle cap + per-PR automation budget) stays authoritative — a re-armed review still flows through both gates, so a body-editing attacker/bot cannot burn cycles.
Mechanism (deterministic, unit-tested)
scripts/lib/pr-metadata-digest.shcompute_pr_metadata_digest <snapshot_json>→ stable short hash of{body, sorted(closingIssuesReferences[].number), sorted(labels[].name)}via jq → sha256 → 16 hex. Both writer and reader compute it identically from the same fields.marker_meta_digest <marker_body>→ extractsmeta=<hex>from a fix-request marker, else empty.scripts/post-pr-review.sh(writer)metadata_onlyfrom the verdict JSON (defaultfalse).metadata_only=true, compute the digest from the current PR (gh pr view --json body,closingIssuesReferences,labels) and stamp it into the marker's decision comment:<!-- pr-review-agent v1 sha=<SHA> --> <!-- decision=fix-requested risk=<R> meta=<digest> -->.scripts/review-one-pr.sh(reader)body,closingIssuesReferencesto thePR_SNAPSHOTgh pr view --jsonlist (line ~127).meta=<digest>, recompute the current digest; if it differs, re-arm (proceed to review) with an AC4 log line ("metadata changed since a metadata-only fix-request — re-reviewing"); if it matches, no-op.Classification (metadata_only)
metadata_onlyis set by the review prompts (prompts/single-review.md,prompts/cascade-action.md) — the only layer that can judge whether a finding's fix is a pure metadata edit. Additive optional boolean, defaults false (missing ⇒ current behavior, no digest, no churn). Kept out of the deterministic mechanism so the fingerprint/re-arm logic is fully unit-testable without an LLM.AC ↔ change map
meta=⇒ unchanged; cycle cap + budget remain authoritative.Tests (Phase 2, written first)
tests/test_pr_metadata_digest.bats— digest stability & sensitivity (body/label/closing-ref change flips it; unrelated fields don't);marker_meta_digestparsing.tests/test_review_one_pr_metadata_rearm.bats— AC5 regression: fix-request marker withmeta=<old>at head SHA + snapshot whose metadata yields a different digest ⇒ reviews (notalready-reviewed-at-head); same digest ⇒ no-op; an approval marker + body edit ⇒ still no-op (AC3).tests/test_post_pr_review_meta_stamp.bats— verdictmetadata_only=true⇒ marker carriesmeta=+ metadata-change footer;false/absent ⇒ neither.Verify (Phase 4)
bats tests/…(new + existing review suites) andbash .dev-lead/scripts/dev-lead-lint.sh.No git actions taken (CI owns commit/push).
don-petry commentedon Aug 20, 2026
Dev-Lead: Implementation Complete — PR #1559
The implementation for issue #1551 is durable: commits are pushed and a pull request is open. This record is backed by verifiable artifacts you can confirm in one click.
f3290d106da62c068fdc7f5cf9910536a433200dReview happens on the PR. Acceptance criteria and test results are evidenced by the PR's diff and CI checks — not asserted here. If a later stage fails, this claim is retracted in place.
feat: implement issue #1551 — [Phase 4] already-reviewed-at-head dead…
feat: implement issue #1551 — [Phase 4] already-reviewed-at-head dead…
chore: empty commit to refresh checks (no functional change)
feat: implement issue #1644 — [qa-lead S1] Provision <id>:hands-off l…
chore: empty commit to clear stale cancelled dev-lead checks
feat: implement issue #1700 — Sync PRs must expire and stay in scope;…
fix(pr-review): re-evaluate at the same head after its own gate dismi…