[#1621 S2] Verify the addressed-marker against the pushed diff + honour maintainer dispositions #1692

Story

As a maintainer relying on required_review_thread_resolution as the real merge gate,
I want the addressed-marker to be a verifiable claim rather than an assertion, and standing maintainer dispositions to be honoured,
so that a thread can only be resolved when a change plausibly addressing it actually exists in the pushed diff.

Parent: #1621. Story 2 of 2. Blocked by #1691 — verification is meaningless while the model can still resolve threads directly.

What is wrong today

resolve_addressed_bot_threads() verifies who claimed (latest reply authored by BOT_USER) and that a claim was made (review_reply_is_addressed_marker). It never verifies whether the claim is true. On petry-projects/.github PR #1044, three functions were byte-identical between ba3f93c5 and b1f0405a, carried addressed-markers, and were resolved exactly as designed.

Second hole: the function reads comments(last:1) — only the latest reply. All three PR #1044 threads carried an earlier maintainer reply reading "ACCEPTED — required before merge", which became invisible the moment the model appended its own marker reply after it.

Acceptance Criteria

1. The marker carries a verifiable claim. The addressed-reply format is extended so it names the commit SHA and the file path(s) the fix touches, in a machine-readable form (e.g. an HTML-comment payload alongside <!-- dev-lead:addressed -->). The prompt is updated to emit it; the format is documented in one place both the prompt and the harness reference.
2. The harness verifies the claim before resolving. For a candidate thread, resolution requires: the named commit exists on the PR head branch, and its diff is non-empty, and the diff touches at least one file named in the claim. Verification failure leaves the thread unresolved and logs why.
3. File-level is the hard gate; region proximity is advisory. Do not gate on the thread's line — review lines shift under edits and a legitimate fix may land in a helper elsewhere, which is why the claim declares its own files. If the claim's files do not include the thread's path, still allow resolution but emit a ::warning:: naming both, so the mismatch is visible without producing false blocks.
4. Standing maintainer dispositions are honoured. Scan all comments in the thread, not just the last. If any comment authored by a human maintainer (marker-less, per review_thread_is_agent_authored's discriminator) asserts a required disposition, the thread may only be resolved if the verified diff satisfies AC2 and postdates that comment. Otherwise reply and leave it open. If the disposition cannot be parsed, fail closed — leave it open.
5. Fail closed throughout. Any inability to fetch the commit, read the diff, or classify a comment leaves the thread unresolved. Never treat an error as "verified".
6. Regression coverage (parent AC5): a fix-reviews pass that pushes a commit not touching a thread's claimed files leaves that thread unresolved — the exact PR feat(canary-rings): onboard the 6 #482 reusables into the registry (#1036) #1044 shape. Plus cases for: empty diff; commit absent from head; a maintainer disposition postdating the fix; a malformed claim.
7. shellcheck --severity=warning -x clean; new bats registered in the existing suites.

Tasks / Subtasks

• Define + document the machine-readable claim payload; update the prompt to emit it (AC: test issue from agent #1)
• Add a pure, sourceable verifier (commit-on-head, non-empty diff, file intersection) with unit tests (AC: Go-live improvements for PR review agent #2, Add @claude delegation, auto-merge, and rebase handling #3, feat: add Copilot engine support via REVIEW_ENGINE toggle #5)
• Wire it into resolve_addressed_bot_threads() ahead of the mutation (AC: Go-live improvements for PR review agent #2, feat: add Copilot engine support via REVIEW_ENGINE toggle #5)
• Replace the comments(last:1) read with a full-thread fetch; add the maintainer-disposition scan (AC: Optimize review: small-PR and incremental fast paths #4)
• Add the PR feat(canary-rings): onboard the 6 #482 reusables into the registry (#1036) #1044-shaped regression case and the fail-closed cases (AC: feat: feature ideation framework — adversarial proposer/challenger/synthesizer #6)
• shellcheck + registration (AC: Workflow should skip no-op PRs in MAX_PRS count #7)

Dev Notes

• Keep the verifier pure and sourceable (inputs: claim payload, commit list, diff file list, thread comments) with the impure gh gathering isolated in one function — the repo's established pattern and what makes it bats-testable without network. Mirror scripts/lib/agent-rate-limit.sh / canary-rollout.sh's split.
• Reuse review_thread_is_agent_authored from scripts/lib/maintainer-review-thread-gate.sh as the human-vs-ours discriminator for AC4; do not invent a second classifier.
• The existing pre-mutation re-read (fresh node(id) fetch, fail-closed on isResolved == null) is correct and must be preserved — add verification alongside it, do not replace it.
• Backward compatibility: a marker with no claim payload (pre-migration replies) must be treated as unverifiable → do not resolve, not as legacy-allowed. Note this in the PR: it may leave some existing threads open until re-run, which is the safe direction.
• Watch the action budget (Measure and tune the dev-lead action budget: #1609 died 1s over ACTION_TIMEOUT_SEC and lost all its work — near-miss alerting + checkpoint push before tuning the number #1619): if this is running long, land the verifier + wiring (AC1–3) and split AC4 into a follow-up rather than losing the pass at 2701s.

References

• scripts/dev-lead-fix-reviews.sh → resolve_addressed_bot_threads() (~L624), the comments(last:1) read (~L700)
• scripts/lib/maintainer-review-thread-gate.sh → review_reply_is_addressed_marker, review_thread_is_agent_authored
• tests/dev-lead/unit/test_fix_reviews.bats
• Evidence: petry-projects/.github PR feat(canary-rings): onboard the 6 #482 reusables into the registry (#1036) #1044; parent dev-lead resolves review threads without addressing them — unfixed findings merge through the thread-resolution gate #1621; blocked by [#1621 S1] Harness-only thread resolution: remove the model's resolveReviewThread path #1691

Maintainer answer — the claim payload schema is PINNED (2026-09-07)

Answering the open question on this issue in the body, because dev-lead does not read issue comments (#1566). This schema is normative: the prompt emitter, the harness parser, and the bats fixtures must all use exactly this.

Format

The addressed reply carries two HTML comments. The existing marker is unchanged so review_reply_is_addressed_marker keeps working exactly as today — the claim is additive, never a replacement:

Fixed in scripts/lib/auto-merge.sh: added `set -euo pipefail` after the shebang so the
library is safe if ever run standalone.

<!-- dev-lead:addressed -->
<!-- dev-lead:claim {"v":1,"sha":"3cc4132fd4b4692aa20865f8b68ea8e21de604b8","files":["scripts/lib/auto-merge.sh","tests/auto_merge.bats"]} -->

Field contract

Delimiters — to answer the question directly: neither newline- nor comma-separated. files is a JSON array, so paths containing spaces, commas, or unicode are handled by the JSON parser rather than by string splitting. Parse with jq, never with IFS/cut/tr.

Extraction

One claim comment per reply. Extract the JSON between the literal <!-- dev-lead:claim prefix and the trailing -->, then parse with jq -e. Rules:

• Zero claim comments → unverifiable → do not resolve (this is the pre-migration case in the Dev Notes above).
• More than one claim comment in a single reply → malformed → do not resolve. Do not "take the last one."
• JSON that does not parse, or fails the field contract → do not resolve.

Every rejection emits a ::notice:: naming the thread id and the reason, so a stuck thread is diagnosable from the run log rather than silently sticky.

Multi-commit fixes

sha names the commit the model believes carries the fix, and it must be reachable from the PR head. For the file intersection (AC2), check in this order:

1. The named commit's own diff. If it touches at least one path in files, verified.
2. Otherwise the cumulative diff <sha>^..HEAD. If that touches a claimed path, verified, and emit a ::notice:: that verification came from the cumulative range — this covers a fix later amended or split across commits.
3. Otherwise not verified → leave the thread unresolved.

Both steps are deterministic and offline once the diff is fetched; keep them in the pure verifier per the Dev Notes.

Not in scope for v1

No line/hunk numbers in the payload. AC3 already settles this: review lines shift under edits and a legitimate fix may land in a helper elsewhere, so region matching stays advisory and is derived from the thread's own path, not from the claim.