dev-lead resolves review threads without addressing them — unfixed findings merge through the thread-resolution gate

[don-petry]

Summary

On a fix-reviews pass, dev-lead marked review threads resolved without making the change they asked for. Because required_review_thread_resolution is the org's real merge gate, resolving a thread is equivalent to clearing a merge block — so an unaddressed finding merged to main.

This is distinct from #1620 (zero-diff PRs). Here a real commit was pushed; it just did not contain the requested fixes, and the threads were resolved anyway.

Evidence — petry-projects/.github PR #1044

Three codeant-ai threads on scripts/apply-repo-settings.sh, each carrying an explicit maintainer reply marking it ACCEPTED — required before merge:

Thread	Finding
PRRT_kwDORyesfc6d7Zi2	apply_labels reports success after label API failures
PRRT_kwDORyesfc6d7Zi5	pp_apply_security_and_analysis returns success when verification finds settings non-compliant
PRRT_kwDORyesfc6d7Zi8	apply_codeql_default_setup returns success after a failed PATCH

Sequence:

1. fix-reviews dispatched at head ba3f93c5 — 3 of 6 threads unresolved.
2. dev-lead pushed b1f0405a and resolved all 6.
3. The three functions are byte-identical at b1f0405a to what they were at ba3f93c5.
4. PR merged.

Verified on main after merge — the guarded driver landed correctly, but only two of its five steps can ever record a failure:

apply_settings                   return-1 paths: 2
apply_labels                     return-1 paths: 0
pp_apply_security_and_analysis   return-1 paths: 0
apply_codeql_default_setup       return-1 paths: 0
apply_check_suite_prefs          return-1 paths: 1

apply_repo guards each step with step || FAILED_STEPS+=(...), so a step returning 0 on failure is invisible: FAILED_STEPS stays empty and the run exits 0 having applied nothing. That is precisely the class of bug #1038 was filed to eliminate, now shipped inside its own fix.

The unaddressed finding reached main only because the threads were resolved. With them open the PR was BLOCKED; the merge fired within the window between resolution and a maintainer unresolveReviewThread call.

AC1 — never resolve a thread that was not addressed

A thread may be resolved only when the pass produced a diff that touches the finding's file and region, or when it posts a reasoned refutation explaining why no change is warranted. Silent resolution with neither is not permitted.

AC2 — resolution must be justified in-thread

Every resolution posts a reply saying what changed (with the commit SHA) or why the finding does not hold. A thread resolved with no reply is the failure mode above and should be impossible.

AC3 — respect an explicit maintainer disposition

All three threads carried a maintainer reply stating ACCEPTED — required before merge and naming the exact change. A thread with a standing maintainer disposition must never be resolved without satisfying it; if the agent disagrees it must reply and leave the thread open for a human.

AC4 — do not resolve threads the agent did not act on in this pass

Track which threads the pass actually addressed and resolve only those. Bulk-resolving every open thread at the end of a run is what produced this.

AC5 — regression coverage

A test asserting that a fix-reviews pass which produces no diff for a given thread leaves that thread unresolved.

---

Follow-up: the three unfixed functions are tracked on .github#1038, which stayed open only because the PR's Closes #1038 had been manually changed to Part of #1038 before merge. Had that not been done, the finding would have closed with the defect shipped — the same closed-but-unfixed pattern as .github#1036 and .github-private#1620.

Filed from a manual compliance sweep, 2026-09-01.

---

Root cause (maintainer analysis, 2026-09-07) — finalized spec

The original ACs described the behaviour to prevent. This section names the mechanism that allowed it, because the fix follows from it directly.

There are two resolution paths, and only one is guarded

Path A — the harness (scripts/dev-lead-fix-reviews.sh). Deterministic and already well guarded. resolve_addressed_bot_threads() re-reads each thread immediately before mutating, fails closed on unknown state, requires the latest reply to be authored by our account (BOT_USER), and requires that reply to carry the addressed-marker via review_reply_is_addressed_marker. resolve_actor_outdated_threads() additionally consults review_thread_is_agent_authored (#1415) so a marker-less maintainer thread is never resolved.

Path B — the model (prompts/dev-lead/fix-reviews.md, "#### Resolving a thread"). The prompt hands the model the raw mutation:

gh api graphql -f query='mutation { resolveReviewThread(input: {threadId: "THREAD_NODE_ID"}) { thread { isResolved } } }'

The model has a shell and this snippet, so every guard in Path A is advisory. The rules that are supposed to constrain it — "Resolve a thread when you actually fixed it", "Never resolve a thread you did not fix" — are prose addressed to a model. On PR #1044 the model did not follow them. Prose is not a gate.

Even Path A cannot detect this defect

Path A verifies who claimed (our account) and that a claim was made (the marker is present). It never verifies whether the claim is true. The addressed-marker is written by the model; the harness trusts it. Three byte-identical functions carried addressed-markers and were resolved exactly as designed.

A second, narrower hole: resolve_addressed_bot_threads inspects only comments(last:1) — the latest reply. A standing maintainer disposition earlier in the thread (all three PR #1044 threads carried "ACCEPTED — required before merge") is invisible once the model appends its own marker reply after it. That is AC3's failure, mechanically.

Fix architecture

1. Make the harness the only resolver. Remove Path B so Path A's guards become authoritative rather than advisory.
2. Make the marker a verifiable claim, not an assertion. The reply must name the commit and the file(s) it changed; the harness verifies that commit exists on the head branch and that its diff touches those files. A claim that cannot be verified does not authorize resolution.
3. Read the whole thread, not the last reply, so a standing maintainer disposition cannot be buried.

Delivery — split for action-budget safety

Implemented as two sequenced stories rather than one. #1269 and #1609 both died at 2701s against the 2700s ACTION_TIMEOUT_SEC and lost all work unpushed, so a change touching a prompt, a 1400-line script, and two bats suites is deliberately not attempted in a single pass.

• Story 1 — close the bypass. Harness-only resolution. Delivers AC3/AC4 immediately, because Path A's existing guards already handle maintainer threads and per-thread scoping once the model can no longer route around them.
• Story 2 — verify the claim. Marker-to-diff verification and whole-thread disposition scan. Delivers AC1/AC2. Blocked by Story 1: verification is meaningless while the model can still resolve directly.

AC5 (regression coverage) is split across both — each story ships the test for its own guarantee.

---

AC REVIEW 2026-09-07 — a contrasting case that sharpens AC3

The behaviour is not uniform, and the counter-example points at which AC matters most.

PR #1048 (petry-projects/.github) — the same agent, same repo, correct outcome

Sequence after the #1044 failure documented above:

1. The three codeant-ai findings were re-raised on the successor PR.
2. A maintainer replied in each thread with an explicit disposition — "ACCEPTED — required before merge" — naming the exact change and quoting the offending lines.
3. A fresh dev-lead-reviews-retry / fix-reviews dispatch was sent after those replies landed.
4. dev-lead pushed b1f0405a→76b06707 and actually made the changes, then resolved.

Verified against the merged code on main, not the implementation report — all five apply_* steps gained real failure paths, and pp_apply_security_and_analysis correctly distinguishes a plan-unsupported skip (return 0) from a genuine failure (return 1).

What this implies for the ACs

AC3 (respect an explicit maintainer disposition) is the highest-value item here, not AC1. In the #1044 failure the threads carried maintainer dispositions and were resolved anyway; in #1048 the same dispositions were honoured. The difference was that #1048's dispatch came after the replies were visible, whereas #1044's pass had already loaded its context.

That suggests the failure is a staleness/ordering problem rather than a refusal to comply: a pass resolves the thread set it loaded at start, and any thread whose content changed mid-pass — including one that gained a maintainer disposition — is resolved on the stale read.

Add AC3b — a pass must re-read each thread's current state immediately before resolving it, and must not resolve a thread whose comments changed after the pass began. Re-queue it instead.

This also explains why AC4 (do not resolve threads the pass did not act on) is necessary but insufficient on its own: in #1044 the agent had acted on the PR, just not on those three threads.

Ordering caution for whoever implements this

Do not "fix" it by making the agent resolve fewer threads in general — #1048 shows it resolves correctly when it has current context. The target is precision, not timidity: resolve exactly the threads this pass addressed, verified against a fresh read.

[github-actions]

Thanks for the thorough write-up and the evidence trail — this is clearly documented and actionable. To help reproduce and scope the fix: can you confirm whether the fix-reviews pass has any existing mechanism to track which threads it acted on during a run, or does it currently treat thread resolution as a separate, unbounded step at the end?