Repository navigation
dev-lead: resolve a bot review thread on a maintainer "no change needed" disposition (rebuild of #1799 on #2049's resolver) #2079
Description
Problem
A bot review thread whose correct answer is "no change needed / false positive" can never be resolved by automation. Under required_review_thread_resolution it blocks merge until a human resolves it by hand (#1743). PR #1799 tried to fix this but was closed as superseded:
- It went 10 commits behind main and conflicted with feat: implement issue #2045 — dev-lead: no deferred/out-of-scope outcome for review threads — a valid finding dev-lead defers stays unresolved forever and blocks merge #2049.
- It has a production bug: dev-lead's
BOT_USERisdon-petry, the same account the maintainer uses.acv_latest_nochange_dispositionskips comments by that login, so the maintainer's own "false positive" reply was always ignored.
#2049 (#2045) adds resolve_deferred_bot_threads. It is a fail-closed resolver: shared listing, a fresh re-read before resolving, marker-based authorship, a retry marker on failure, and it runs before the terminal marker. This issue adds the maintainer no-change case on top of that resolver.
Acceptance criteria
-
In
resolve_deferred_bot_threads, accept a second case: the latest maintainer comment in a bot thread is a "no change needed" disposition (false positive / no change needed / won't fix / not applicable, with negation and question forms rejected).- A maintainer is OWNER, MEMBER or COLLABORATOR and the comment carries no
<!-- dev-lead/<!-- pr-review-agentmarker. Tell maintainer comments apart from dev-lead's by marker, never by login. - No newer "required" disposition and no newer bot finding may follow it.
- That case skips the tracker check. Everything else (fresh re-read, unchanged-thread check, retry marker on failure) is the same as for a deferral.
- A maintainer is OWNER, MEMBER or COLLABORATOR and the comment carries no
-
Carry over from feat: implement issue #1743 — Harness cannot resolve a bot review thread whose correct response is 'no change needed' — false positives block merge permanently #1799's branch (
dev-lead/issue-1743-20260913-0436): the pureacv_latest_nochange_disposition, its negation and question regexes, and their bats tests, adapted to the marker-based authorship rule. -
acv_latest_maintainer_dispositiontreats an unreadable timestamp as blocking even when another valid one exists (the small fix from feat: implement issue #1743 — Harness cannot resolve a bot review thread whose correct response is 'no change needed' — false positives block merge permanently #1799). -
An agent can never dismiss a finding on its own: dev-lead writing "no change needed" must not resolve a thread. Only the maintainer's statement does (Harness cannot resolve a bot review thread whose correct response is 'no change needed' — false positives block merge permanently #1743 AC2/AC4). Add a test for this.
-
AC4 must hold for the replies dev-lead really posts (added 2026-10-09 after reviewing PR feat: implement issue #2079 — dev-lead: resolve a bot review thread on a maintainer "no change needed" disposition (rebuild of #1799 on #2049's resolver) #2084). dev-lead's skip notes are free-form replies written by the agent, and most carry no
<!-- dev-leadmarker (11 of 29 on PR feat: implement issue #2079 — dev-lead: resolve a bot review thread on a maintainer "no change needed" disposition (rebuild of #1799 on #2049's resolver) #2084). A marker-less skip note that says "no change needed" or "false positive" isUser+MEMBER+ marker-less, soacv_latest_nochange_dispositionaccepts it as the maintainer's verdict.- Enforce authorship in shell, not in the prompt: before either resolver runs, every thread reply the pass created must be attributable to dev-lead (for example, record the reply IDs the pass adds and append a
<!-- dev-lead:reply -->marker to any without one). - Fail closed on marker-less dev-lead replies posted before that enforcement existed.
- Tests: a marker-less
don-petry/MEMBERreply "No change needed in this pass: …" and "Skipped: this is a false positive …" must return rc 1; a thread whose only no-change comment was posted by the pass itself must stay unresolved; a marker-lessdon-petrycomment the pass did not post must still return rc 0. - Details and a reproduction are in the review thread on PR feat: implement issue #2079 — dev-lead: resolve a bot review thread on a maintainer "no change needed" disposition (rebuild of #1799 on #2049's resolver) #2084 at
scripts/lib/addressed-claim-verify.shline 484.
- Enforce authorship in shell, not in the prompt: before either resolver runs, every thread reply the pass created must be attributable to dev-lead (for example, record the reply IDs the pass adds and append a
-
Attribution must be by identity, not by time window (added 2026-10-09 after reviewing
47fac647on PR feat: implement issue #2079 — dev-lead: resolve a bot review thread on a maintainer "no change needed" disposition (rebuild of #1799 on #2049's resolver) #2084). Stamping every marker-less comment our account created since the pass started also rewrites comments the human maintainer posts during a pass (BOT_USERis the maintainer's account), and a stamped "must be fixed" comment stops counting as a REQUIRED disposition.- Stamp only comments the pass itself posted, identified by reply ID recorded at post time. Never edit any other comment. If attribution is unavailable, stamp nothing and disable the no-change path for that pass.
- The legacy cutoff must come from configuration, with unset meaning "no-change path disabled", so it is switched on only after the release has reached every consumer's channel.
- Tests: a
don-petrycomment created inside the pass window that the pass did not post stays byte-identical and still counts as REQUIRED; a top-level comment is never stamped; an unset cutoff resolves nothing on a no-change verdict. - Details and a reproduction are in the review thread on PR feat: implement issue #2079 — dev-lead: resolve a bot review thread on a maintainer "no change needed" disposition (rebuild of #1799 on #2049's resolver) #2084 at
scripts/dev-lead-fix-reviews.shline 2997.
Budget guard
Do AC1, AC2 and AC4 first, in one commit with tests. AC3 is a separate small commit. If time runs short, push AC1+AC2+AC4 and file AC3 as a follow-up.
Depends on #2049 being merged.
Status (2026-10-09): PR #2084 implements AC1–AC6 with the no-change path switched off (DEV_LEAD_NOCHANGE_EPOCH is unset everywhere). Switching it on, and closing #1743, is tracked in #2161. Merging PR #2084 does not close #1743.
Activity
don-petry commentedon Oct 9, 2026
Dev-Lead Implementation Plan
Issue: #2079 — dev-lead: resolve a bot review thread on a maintainer "no change needed" disposition (rebuild of #1799 on #2049's resolver)
Dependency status
#2049 (which adds
resolve_deferred_bot_threadsandscripts/lib/deferred-thread-verify.sh) is still open, not merged.mainhas noresolve_deferred_bot_threads, so the AC1 wiring can't be built on this branch without pulling #2049's whole diff into this PR. This pass ships only the parts that don't depend on #2049. They live inscripts/lib/addressed-claim-verify.sh, which #2049 doesn't touch, so they won't conflict with it.Scope
Add the pure maintainer no-change classifier (AC2) and its agent-cannot-self-dismiss guarantee (AC4, at the classifier level). Also include the unreadable-timestamp fix (AC3). Defer the AC1 wiring into
resolve_deferred_bot_threadsuntil #2049 merges.Implementation Checklist
scripts/lib/addressed-claim-verify.sh: add_ACV_NOCHANGE_RE_UPPERand_ACV_NOCHANGE_NEGATION_RE_UPPER, carried over from feat: implement issue #1743 — Harness cannot resolve a bot review thread whose correct response is 'no change needed' — false positives block merge permanently #1799.acv_latest_nochange_disposition. It identifies a maintainer by marker + authorAssociation (OWNER/MEMBER/COLLABORATOR) + User type. It does not skip by login, which fixes feat: implement issue #1743 — Harness cannot resolve a bot review thread whose correct response is 'no change needed' — false positives block merge permanently #1799'sdon-petrybug.acv_latest_maintainer_disposition, an unparseable timestamp returns rc2 even when a valid one also exists.resolve_deferred_bot_threads(no newer REQUIRED disposition or bot finding, skip the tracker check). This needs a follow-up once feat: implement issue #2045 — dev-lead: no deferred/out-of-scope outcome for review threads — a valid finding dev-lead defers stays unresolved forever and blocks merge #2049 lands.Tests to Write (
tests/dev-lead/unit/test_addressed_claim_verify.bats)acv_latest_nochange_dispositiontests: phrases, negation, questions, a bot author, unparseable date, latest wins.don-petry) with no marker is accepted. This is the feat: implement issue #1743 — Harness cannot resolve a bot review thread whose correct response is 'no change needed' — false positives block merge permanently #1799 production bug.<!-- dev-leadmarker is never a disposition. Same for a<!-- pr-review-agentmarker.unparseable.Test Command
bats tests/dev-lead/unit/test_addressed_claim_verify.bats(plus the fullbats tests/dev-lead/unit/) andbash .dev-lead/scripts/dev-lead-lint.shdon-petry commentedon Oct 9, 2026
Dev-Lead: Progress (in progress — not yet durable)
Plan Execution (intended)
acv_latest_nochange_dispositionand its no-change and negation regexes intoscripts/lib/addressed-claim-verify.sh. Adapted it to the marker-based authorship rule: no login skip, and thebot_userargument is dropped. A maintainer is aUserwith association OWNER, MEMBER or COLLABORATOR whose comment carries no agent marker. AddedNOT APPLICABLEto both regexes, since the issue lists it.don-petry), the same OWNER association and the same "False positive — no change needed." text. Without a marker it is accepted; with<!-- dev-lead:reply -->it is rejected. A<!-- pr-review-agentmarker is rejected too.acv_latest_maintainer_dispositionnow returns rc2unparseablewhenever any matching disposition's timestamp can't be read, even if a valid one also exists.resolve_deferred_bot_threadsdoesn't exist onmain. Wiring the classifier into it needs a follow-up once feat: implement issue #2045 — dev-lead: no deferred/out-of-scope outcome for review threads — a valid finding dev-lead defers stays unresolved forever and blocks merge #2049 merges. That work: skip the tracker check for this case; require no neweracv_latest_maintainer_dispositionand no newer bot finding; keep the fresh re-read, unchanged-thread check and retry marker. Also, "no change required" matches the REQUIRED regex, so the caller must not count the same comment as a newer REQUIRED disposition.Local check output (provisional — not the pushed run)
test_addressed_claim_verify.batspasses locally. The 20 failures in the wholetests/dev-lead/unit/suite (gemini/copilot provider and sidecar tests) are identical on unmodifiedmain.dev-lead-lint.shpasses.Files touched (this workspace, not yet pushed)
scripts/lib/addressed-claim-verify.sh: no-change regexes,acv_latest_nochange_disposition, AC3 ordering fix.tests/dev-lead/unit/test_addressed_claim_verify.bats: carried-over feat: implement issue #1743 — Harness cannot resolve a bot review thread whose correct response is 'no change needed' — false positives block merge permanently #1799 tests, plus tests for the same-login maintainer, AC4 self-dismissal, association, extra negation forms, mixed valid/unparseable dates, and the AC3 regression.Notes
acv_latest_maintainer_disposition(the REQUIRED gate) still skips comments bybot_userlogin. Since that login isdon-petry, the maintainer's own REQUIRED replies are ignored there too. That's the same class of bug as #1799's, but outside this issue's ACs; it's worth a follow-up.don-petry commentedon Oct 9, 2026
Dev-Lead: Implementation Complete — PR #2084
The implementation for issue #2079 is durable: commits are pushed and a pull request is open. This record is backed by verifiable artifacts you can confirm in one click.
a876d5c40d108858fdafbe8ebdcc14a0c77f4495main: 2 file(s), +347/-0 linesReview 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.
fix(#2079 AC1): integrate no-change disposition into resolve_deferred…
fix(reviews): stamp pass replies so dev-lead skip notes never read as…
fix(reviews): attribute pass replies by recorded id and gate no-chang…
feat: implement issue #2161 — dev-lead: switch on the maintainer no-c…
feat: implement issue #2161 — dev-lead: switch on the maintainer no-c…