Skip to content

pr-review can't approve ANY open PR: maintainer-comment gate needs pushedDate (always null) and treats codeant/qodo/auto-rebase comments as maintainer findings — 18/18 PRs stranded #1813

Closed

Description

@don-petry

Summary

pr-review can no longer auto-approve any open PR in this repo. The maintainer issue-comment gate (scripts/lib/maintainer-comment-gate.sh, #1290) fails closed on every PR:

  1. It depends on pushedDate, which GitHub now always returns as null. maintainer_gate_head_committer_date() returns empty without it, and check_maintainer_comments fails closed on an empty push time whenever any unanswered comment exists. So a push can never clear the gate, even though the skip verdict claims one would.
  2. It judges a comment "addressed" by comparing timestamps, not by whether anyone answered it. A push that happens after a comment is only a proxy. It says nothing about whether the comment's finding was read, checked, or acted on. And every open PR today carries comments that nobody has answered: codeant-ai review-status comments, qodo-code-review's trial-ended notice, and auto-rebase's merge-conflict reports.

The fix is NOT to exclude those authors from the gate. Excluding comments because a bot wrote them makes ignoring them the official process. Every comment must be researched, analyzed, answered with evidence, and resolved. The gate withholds approval until that has happened, for bots and humans alike.

This also regresses a known failure class: #577 removed the pushedDate dependency from advisory-review-gate.sh in June, and #1290 reintroduced it here.

Revision — 2026-09-13 (supersedes the original "exclude automation authors" approach)

The first version of this issue said to add codeant-ai / qodo-code-review to MAINTAINER_GATE_EXCLUDED_BOTS and add auto-rebase-conflict to _MAINTAINER_GATE_AGENT_MARKERS, so the gate would ignore their comments. The maintainer rejected that direction: comments must not be ignored; their findings must be considered, answered, and resolved.

The dev-lead implementation plan posted on this issue at 2026-09-13T13:43Z is superseded and must not be implemented. That plan's excluded-bots registry derivation, the auto-rebase-conflict agent-marker addition, and tests asserting "a codeant-ai / qodo comment → gate passes" are the rejected design. The run executing it was cancelled.

Evidence (executed 2026-09-13 ~13:20Z)

pushedDate is null on PR heads, via the gate's own GraphQL query:

#1789 head=13482110 pushedDate=NULL committer.date=2026-09-13T07:08:46-05:00
#1772 head=f5cd9dc1 pushedDate=NULL committer.date=2026-09-12T23:08:11-05:00

The real gate, run against PR #1789's live snapshot (sourced from origin/main):

helper head date=[]
[maintainer-gate] WARNING: maintainer issue comment present but head-commit push time is unknown — failing closed (blocking approval)
gate rc=1

What it counted as maintainer findings on #1789, all automation:

04:23:06Z qodo-code-review   <!-- qodo:billing-blocked --> Qodo reviews are paused because your trial has ended…
04:23:06Z codeant-ai         ## 🤖 CodeAnt AI — Review Status …
04:23:07Z codeant-ai         Thanks for using CodeAnt! …
04:28:56Z don-petry          <!-- auto-rebase-conflict:56d8ccc7… --> Auto-rebase failed — merge conflict …
05:06:24Z don-petry          <!-- auto-rebase-conflict:675cb4aa… --> Auto-rebase failed — merge conflict …

pr-review's verdict on a targeted run (34759124454):

{"pr":".../pull/1789","sha":"13482110…","decision":"skip","reason":"unaddressed-maintainer-comment",
 "would_change":"the maintainer comment is addressed by a new push, or an @mention (FORCE_REVIEW) overrides the gate"}

Scope: 18 of 18 open PRs trip the gate (same filter, same exclusion list and marker regex):

PR#1804 PR#1802 PR#1801 PR#1800 PR#1799 PR#1798 PR#1796 PR#1794 PR#1793
PR#1792 PR#1790 PR#1789 PR#1788 PR#1783 PR#1782 PR#1770 PR#1742 PR#1730
— every one carries codeant-ai ×2–3 and qodo-code-review ×1; most also carry auto-rebase-conflict comments

Every one is BLOCKED / REVIEW_REQUIRED. The last PR to merge on an automated approval before this was #1772, at 2026-09-13T04:23:27Z, just before the first Qodo trial-ended notice (04:23:06Z on #1789) appeared.

The fix PR for this issue will hit the same gate. It needs a human approval or the @donpetry-bot FORCE_REVIEW break-glass.

Design

Addressed means a verified disposition on the comment, not a timestamp.

Answer to the triage question (committer.date vs a tamper-resistant server-side time): once "addressed" means a verified disposition, head push time is no longer what decides whether a comment is answered. That removes the tamper-resistance concern at the root, because a spoofed commit date cannot manufacture a verified disposition. So: remove the pushedDate dependency entirely. Where a head time is still needed (for example, noticing new commits after a disposition), use the head commit's committer.date, matching advisory-review-gate.sh.

Acceptance criteria

  1. No pushedDate dependency. Nothing in the gate or its helpers reads pushedDate. Any head time still needed comes from committer.date, as in advisory-bot gate strands PRs forever: pushedDate=null disables both absent-bot timeout fallbacks #577.
  2. The gate withholds approval while any non-agent PR issue comment lacks a verified disposition. This applies to bots and humans alike. Do not add codeant-ai or qodo-code-review to an excluded-authors list, and do not add auto-rebase-conflict to the ignorable agent markers. A conflict report is a finding, and it is fixed by the rebase commit. The only exclusion is our own disposition and ack replies.
  3. dev-lead handles PR issue comments, not only review threads. Today fix-reviews walks reviewThreads alone, which is the gap PR comments from maintainers are mechanically inert — they neither block merge nor reach dev-lead #1290 describes. For each undispositioned comment it researches, analyzes, and posts exactly one evidence-backed reply carrying <!-- dev-lead:comment-disposition id=<comment_id> disposition=<fixed|invalid|out-of-scope|answered|informational> [sha=<sha>] [ref=#<n>] -->.
  4. The harness resolves after verifying: it checks the disposition as described in Design, then minimizes the original comment as RESOLVED. It never resolves an unverified disposition, and the model never minimizes anything itself.
  5. Human maintainer comments are always answered, auto-resolved only on a verified fixed, and otherwise left open for the human. The gate keeps blocking until they are resolved.
  6. Informational comments with consequences are routed, not merely acknowledged. From the evidence above: Qodo's qodo:billing-blocked notice means Qodo reviews have stopped. The response states that, checks whether the advisory gate's required-bot set still expects Qodo, and raises the billing decision for a human, with one tracking issue per repo linked from each PR's reply rather than a new issue per PR.
  7. Loop safety (feat: implement issue #846 — [Phase 2] Cache the LSP index and enforce the 30s cold-start SLA with auto-skip #860): our disposition and ack replies never count as comments needing a response. A bot's reply to our disposition is answered only if it raises a new finding, so boilerplate never starts a reply chain. One disposition per comment id (idempotency key = comment id). Everything stays within the existing per-PR automation budget (MAX_PR_AUTOMATION_CYCLES).
  8. An undeterminable gate state is diagnosable: a distinct verdict reason, never disguised as unaddressed-maintainer-comment, so this outage cannot recur looking like a legitimate hold.
  9. Regression bats:
    • (a) pushedDate null, every comment dispositioned and verified → passes.
    • (b) An undispositioned codeant-ai, qodo-code-review, or auto-rebase-conflict comment → blocks.
    • (c) A fixed disposition citing a sha that is not on the head → still blocks.
    • (d) A human comment with a non-fixed disposition → blocks until the human resolves it.
    • (e) Our own disposition reply is never treated as needing a response.
    • (f) A bot's boilerplate reply to our disposition does not trigger another reply.
  10. After merge, the stranded PRs are processed: dev-lead dispositions the existing codeant-ai, qodo and auto-rebase comments on the open PRs, then pr-review re-reviews them (see pr-review sweep reports success while silently omitting an eligible open PR from the candidate pool #1744 for the sweep caveat).

Notes for implementation

  • This fix's own PR will be blocked by the currently merged, broken gate, so it needs a human approval or the @donpetry-bot FORCE_REVIEW break-glass. That PR's own bot comments must also be dispositioned under the new rule.
  • If this is too large for one PR, split it into (i) the gate plus harness verification and resolution (AC1, 2, 4, 5, 8, 9) and (ii) the dev-lead comment-disposition pass (AC3, 6, 7, 10). Land (ii) first, or together with (i). Landing the gate alone would block every PR until comments are dispositioned.

Architect review — folded in 2026-09-13 (these are acceptance criteria)

The Solution Architect's advisory on this issue found the design mostly aligned with ADR-0004 (the harness verifies, then does the write) and ADR-0008 (personas post as the owner account). It escalated one gap and flagged that no ADR records this contract. Its escalation premise was verified against main:

  1. Dispositions are workflow-enforced, not prompt-enforced. The model must not post disposition replies itself. It emits each disposition as structured output (a sentinel block, as persona-runner-reusable.yml uses for advisories). The harness validates the structure, runs the verification in AC4, and only then posts the reply with the disposition marker. The marker carries provenance the gate can check: at minimum the Actions run_id of the dev-lead run that posted it. The gate accepts a disposition only if that run exists, is a dev-lead-reusable.yml run on this repo, and completed. A marker typed by the model outside that path, or pasted by a human or any holder of the owner token, must not clear the gate. For invalid / answered / informational there is no diff to verify, so provenance is the only thing standing between a model's assertion and a merge.
  2. Record the contract as ADR-0009, alongside this change (same PR, or merged before it), not after. It must cover what "addressed" means for the merge gate (verified disposition), the harness-posted provenance requirement (AC11) and why marker-only trust is insufficient under ADR-0008, the human-comment rule (auto-resolve only on verified fixed), and the minimizeComment auditability trade-off (AC14). No existing ADR governs this gate's contract.
  3. Retire blanket bot-resolve commands. Remove the @coderabbitai resolve post from dev-lead-fix-reviews.sh, and any equivalent "resolve all" instruction to another review bot. Resolution happens only through verified per-finding dispositions. A bot resolving its own threads wholesale is exactly the "ignore the comment" behaviour this issue exists to remove.
  4. Minimizing never erases the audit trail. A resolved comment stays discoverable: the harness-posted disposition reply quotes or links the original finding and records the verification evidence (commit sha and verified files for fixed, the referenced issue for out-of-scope, the evidence summary otherwise). A minimized finding is always reachable from a visible reply, and a count of minimized-as-resolved findings is available to the reviewer report, so nobody has to un-hide comments to audit what was dismissed.

Note on the in-flight dev-lead pass (run 34760944872): it started before this section was added. If its PR does not meet ACs 11–14, those gaps will be raised as review findings on the PR rather than by restarting the pass.

Added 2026-09-13 — review of dev-lead's 13:58 plan (acceptance criterion)

  1. A disposition that needs no commit must still be resolvable. The 13:58 plan wires resolve_dispositioned_comments() into the existing fix-reviews resolution-gate blocks, which "never resolve without a commit" (the Follow-up to #1567: threads resolved with NO commit at all — the shipped evidence bar gates status=applied, not thread resolution (near-miss: fleet-breaking merge on .github#1024) #1609 / Follow-up to #1609 (slice 2): gate dev-lead thread resolution on the pass having advanced the PR head #1617 no-commit resolution gate). Most comments this issue targets produce no commit by nature: Qodo's qodo:billing-blocked notice (informational), CodeAnt's review-status tables (informational), a wrong finding (invalid), a question (answered), work tracked elsewhere (out-of-scope). If minimizing is gated on a commit, those comments are never minimized. The new gate blocks on every unminimized comment, so every open PR would be stranded again, recreating this outage in a new form. It is the Harness cannot resolve a bot review thread whose correct response is 'no change needed' — false positives block merge permanently #1743 deadlock ("a bot finding whose correct response is no change can never be resolved") generalized to issue comments. Required: minimizing a verified invalid / answered / informational / out-of-scope disposition does not require a commit-producing pass. Only fixed is tied to a commit, verified via the cited sha (AC4). A bats case covers it: a verified informational disposition on a qodo:billing-blocked comment, from a pass with no commit, is minimized and the gate passes.

Note on the in-flight pass: its 13:58 plan covers ACs 1–10 but none of 11–15 (workflow-enforced disposition provenance, ADR-0009, retiring the blanket @coderabbitai resolve, the minimize audit trail, and no-commit resolution). Those will be raised as review findings on the PR.

Added 2026-09-13 — found while verifying PR #1816 (acceptance criterion)

  1. Nothing dev-lead posts may be marker-less. On PR feat: implement issue #1813 — pr-review can't approve ANY open PR: maintainer-comment gate needs pushedDate (always null) and treats codeant/qodo/auto-rebase comments as maintainer findings — 18/18 PRs stranded #1816, dev-lead's model posted a free-form "Phase 0 note — template-drift failure is the pre-existing [bug] seed-repo-template fetches standards unpinned — pin to published stable, compliance accepts N-1 (continues #1436) #1448 known-red…" comment (2026-09-13T14:21:26Z) carrying no agent marker. It comes from no harness template: nothing in scripts/ or prompts/ contains that text, so the model wrote and posted it on its own. It is authored by don-petry, and author.__typename is "User", so both the current gate and feat: implement issue #1813 — pr-review can't approve ANY open PR: maintainer-comment gate needs pushedDate (always null) and treats codeant/qodo/auto-rebase comments as maintainer findings — 18/18 PRs stranded #1816's new gate read it as a human maintainer's finding. Consequences: dev-lead blocks its own PR; under AC5 a "human" comment auto-resolves only on a verified fixed, which a note can never have; and the disposition pass would be asked to research and answer dev-lead's own note, the self-reply loop AC7 forbids. Required: every comment the model or harness posts carries an agent marker, enforced by the harness rather than the prompt (the same mechanism as AC11: the model emits content, the harness posts it with the marker). A lint or bats check fails if any dev-lead prompt instructs posting a comment by any path other than that one.

Activity

  1. github-actions commented on Sep 13, 2026

    @github-actions
    Contributor

    Thanks for the thorough write-up — the evidence trail, live gate output, and acceptance criteria make the scope very clear. Before diving in: since AC #1 calls out a tamper-resistance trade-off between using committer.date (simple, already proven in the advisory gate) versus a server-side source like the PR timeline's head_ref_force_pushed event or a check-suite created_at (harder to spoof but more complex to fetch), do you have a preference, or should the fix default to committer.date parity with advisory-review-gate.sh and leave the tamper-resistant path as a follow-up?

  2. added
    dev-leadFor dev-lead agent pickup
    and removed
    needs-human-reviewFlagged by automated PR review agent
    on Sep 13, 2026
  3. don-petry commented on Sep 13, 2026

    @don-petry
    CollaboratorAuthor

    Dev-Lead Implementation Plan

    Issue: #1813 — pr-review can't approve ANY open PR: maintainer-comment gate needs pushedDate (always null) and treats codeant/qodo/auto-rebase comments as maintainer findings

    Scope

    Repair the #1290 maintainer issue-comment gate so it (a) derives head push-time from the head commit's committer.date instead of the always-null pushedDate, (b) never classifies org automation (advisory bots + auto-rebase-conflict) as a human maintainer, and (c) gives an undeterminable push-time a distinct, diagnosable verdict — while still blocking a genuine post-head human maintainer comment.

    Implementation Checklist

    • scripts/lib/maintainer-comment-gate.sh — AC1: rewrite maintainer_gate_head_committer_date() to return the head commit's committer.date (drop pushedDate); record the tamper-resistance trade-off in the header.
    • scripts/lib/maintainer-comment-gate.sh — AC2: derive MAINTAINER_GATE_EXCLUDED_BOTS from the reviewer-source registry (reviewer-sources.sh / reviewer-sources.tsv, so codeant-ai/qodo-code-review/graphite-app and any future bot are excluded automatically) unioned with the generic automation accounts; keep an in-sync built-in fallback.
    • scripts/lib/maintainer-comment-gate.sh — AC2: add auto-rebase-conflict to _MAINTAINER_GATE_AGENT_MARKERS (posted as don-petry); dev-lead*/dependency-advisory already covered by the existing alternation.
    • scripts/lib/maintainer-comment-gate.sh — AC4: return a distinct rc=3 when a maintainer comment exists but the head push-time is undeterminable (was folded into rc=1).
    • scripts/review-one-pr.sh — AC4: map rc=3 to a distinct verdict reason (maintainer-comment-gate-indeterminate-pushtime), not unaddressed-maintainer-comment.

    Tests to Write (tests/dev-lead/unit/test_maintainer_comment_gate.bats)

    • AC5(a): pushedDate-null world (empty pushedDate no longer consulted) + only automation comments → 0
    • AC5(b): human comment OLDER than head commit → 0 (addressed)
    • AC5(c): human comment NEWER than head commit → 1 (block) — AC3 genuine case
    • AC5(d): qodo:billing-blocked comment (author qodo-code-review) → 0; auto-rebase-conflict marker (author don-petry) → 0
    • AC2: codeant-ai issue comment → 0
    • AC4: maintainer comment + empty head date → rc=3 (distinct), not 1
    • AC1: maintainer_gate_head_committer_date reads committer.date (does not depend on pushedDate)
    • Wiring: review-one-pr.sh maps rc=3 to maintainer-comment-gate-indeterminate-pushtime

    Test Command

    bats tests/dev-lead/unit/test_maintainer_comment_gate.bats (full suite via the repo bats job); lint via bash .dev-lead/scripts/dev-lead-lint.sh

  4. don-petry commented on Sep 13, 2026

    @don-petry
    CollaboratorAuthor

    Two responses: an answer to the triage question from 13:18, and a note that the dev-lead plan from 13:43 is superseded.

    Triage question: committer.date parity vs. a tamper-resistant server-side time. The scope has changed, and that changes the answer. The issue body is revised: a comment now counts as "addressed" only once it has a verified disposition, meaning an evidence-backed reply that the harness checks before resolving the comment. Whether a push came after the comment no longer decides it. Under that model the head push time is not what judges a comment answered, so the tamper-resistance concern goes away at the root: a spoofed commit date cannot produce a verified disposition. The decision: remove pushedDate entirely, and wherever a head time is still needed, use committer.date for parity with advisory-review-gate.sh. No separate tamper-resistant follow-up is needed for this gate.

    The 13:43 implementation plan is superseded. Do not implement it. It excluded codeant-ai, qodo-code-review and graphite through the reviewer-sources registry, added auto-rebase-conflict to the ignorable agent markers, and tested that those comments pass the gate. The maintainer has directed the opposite: comments are not to be ignored. Every comment, from a bot or a human, must be researched, analyzed, answered, and resolved. The run implementing the old plan was cancelled, and dev-lead will be re-dispatched against the revised body. The Design and Acceptance criteria sections of the body are now the source of truth.

  5. added
    dev-leadFor dev-lead agent pickup
    and removed
    dev-leadFor dev-lead agent pickup
    on Sep 13, 2026
  6. don-petry commented on Sep 13, 2026

    @don-petry
    CollaboratorAuthor

    @petry-projects/solution-architect please measure the revised design in this issue's body against our recorded ADRs before it lands. It changes the contract of the merge gate that every PR in this repo passes through.

    What changed (see the body's Revision, Design and Acceptance criteria sections): the maintainer-comment gate (#1290) currently treats a comment as "addressed" when a push lands after it. That depends on GitHub's pushedDate, which is always null, so the gate strands every open PR. The maintainer rejected the obvious fix, excluding bot authors, because comments must not be ignored. The revised design:

    Where your lens matters most:

    1. Governing decision. Is this consistent with the decided architecture, and which ADR governs it? If none does, and the merge gate's "addressed" contract has never been recorded, say so, and say whether this change should be recorded as a new ADR before or alongside implementation.
    2. ADR-0008 (personas post as the owner account). Agents and the human maintainer all post as don-petry, so the gate can tell an agent's disposition from a human's comment only by HTML markers. Is a marker-only trust boundary structurally sound for a merge gate? Does the harness minimizing comments authored by the owner account introduce an identity ambiguity ADR-0008 did not anticipate?
    3. Resolution via minimizeComment. Is hiding the original comment the right structural representation of "resolved" for issue comments, or does it trade away auditability (a minimized finding is easy to miss later) that the decision record would care about?
    4. Sequencing. The body says the dev-lead comment-disposition pass must land before or with the gate, otherwise every PR blocks. Is that the right split, or is there a safer landing order?

    Time-sensitive: dev-lead is already implementing this issue (run 34760944872), so structural concerns are cheapest to act on now.

  7. don-petry commented on Sep 13, 2026

    @don-petry
    CollaboratorAuthor

    Solution Architect — structural advisory

    Governing ADR: ADR-0008 (personas post as the owner account) for the identity question raised in point 2; ADR-0004 (pure-logic classifiers + bats) for the verify/resolve split. No ADR governs the merge gate's "addressed" contract itself.

    Alignment: mostly aligned, with one gap ADR-0008 didn't anticipate.

    • Governing decision: ADR-0004 fits — "the model never resolves, the harness verifies then acts" (AC4) is exactly its I/O-in-main(), judgment-only-where-needed split, and AC1/AC9's pure-classifier-plus-bats requirement is its stated boundary. But no ADR records what "addressed" means for this gate; record one alongside this change, not after.
    • ADR-0008's marker axis, extended to a new user: ADR-0008 accepted a single mechanical guard (the marker) because persona-runner-reusable.yml's post step enforces it workflow-side, not prompt-side. This design reuses the same marker-as-trust-boundary pattern for dev-lead's disposition comments, but dev-lead runs through dev-lead-reusable.yml (ADR-0001), not the persona runner. Confirm the disposition marker is equally workflow-enforced there before the gate trusts it to distinguish agent from human — otherwise ADR-0008's residual-risk warning applies unverified.
    • minimizeComment as "resolved": consistent with ADR-0004's harness-does-the-write pattern; the auditability trade-off (a minimized finding is easy to miss) isn't covered by any ADR — worth a line in the new record.
    • Sequencing: the design's own answer (land the disposition pass first or together) is the safer order; no ADR conflict.

    Escalate? yes — confirm dev-lead's disposition-marker enforcement is workflow-side before this becomes a merge-gate trust boundary.

    Advisory only — I comment, I do not change code. Ground: BMAD Method architect (bmad-agent-architect v6.8.0). Opt out on this item with the solution-architect:hands-off label.

  8. don-petry commented on Sep 13, 2026

    @don-petry
    CollaboratorAuthor

    Responding to the Solution Architect's advisory above. All four points are accepted and folded into this issue's body as ACs 11–14 (section "Architect review — folded in").

    The escalation is confirmed, not hypothetical. I checked it against main:

    • prompts/dev-lead/fix-reviews.md lines 63–73 instruct the model to post its own thread replies ending in <!-- dev-lead:addressed --> and <!-- dev-lead:claim {…} --> via gh api graphql.
    • The model step runs with GH_TOKEN = GH_PAT_DON_PETRY (dev-lead-reusable.yml:141).
    • The harness only verifies the claim against the pushed diff ([#1621 S2] Verify the addressed-marker against the pushed diff + honour maintainer dispositions #1692) and resolves. So dev-lead's markers are prompt-enforced, the opposite of the persona runner's split that ADR-0008 relies on.

    For fixed, the diff verification limits the risk. For invalid, answered and informational there is nothing to verify against, so the gate would be trusting a model-authored marker outright. AC11 therefore requires dispositions to be emitted as structured output and posted by the harness, carrying the dev-lead run_id as provenance the gate checks.

    A related finding the advisory did not have: scripts/dev-lead-fix-reviews.sh:324 posts @coderabbitai resolve, which makes CodeRabbit resolve all of its threads wholesale with no per-finding verification. This is how a half-applied finding on PR #1789 (#1727 AC4 parity) closed. AC13 retires it.

    On the missing ADR: agreed, and AC12 requires ADR-0009 to land alongside the change, not after, covering the "addressed" contract, the provenance requirement and why marker-only trust is insufficient under ADR-0008, the human-comment rule, and the minimizeComment auditability trade-off. AC14 makes that trade-off concrete: a minimized finding is always reachable from a visible, harness-posted reply that records the verification evidence.

    On sequencing: kept as advised. The comment-disposition pass lands first or together with the gate.

  9. coderabbitai commented on Sep 13, 2026

    @coderabbitai

    @don-petry, the revised acceptance criteria address the structural risks.

    AC11 removes the marker-only trust boundary. The harness becomes the disposition author and binds each disposition to the dev-lead run_id.

    AC12 correctly requires ADR-0009 to land with the implementation. The ADR should define the verified-disposition contract and the audit trail for minimized comments.

    AC13 removes an unsafe bulk-resolution path. Per-finding verification must precede every resolution action.

    AC14 preserves auditability. Each minimized original comment has a visible, harness-posted reply with the disposition and verification evidence.

    The required landing order remains correct: land the disposition pass before, or together with, the merge-gate change.

    🐇


    ✏️ Learnings added
    Learnt from: don-petry
    URL: https://github.com/petry-projects/.github-private/issues/1813
    
    Timestamp: 2026-09-13T13:56:45.323Z
    Learning: For GitHub issue `#1813`, dev-lead disposition markers must not be a merge-gate trust boundary because `prompts/dev-lead/fix-reviews.md` instructs the model to post them using the owner `GH_PAT_DON_PETRY`. The harness must post structured per-comment dispositions and include the dev-lead `run_id` as provenance that the gate verifies.
    

    Learnt from: don-petry
    URL: https://github.com/petry-projects/.github-private/issues/1813
    
    Timestamp: 2026-09-13T13:56:45.324Z
    Learning: For GitHub issue `#1813`, `scripts/dev-lead-fix-reviews.sh` must retire the wholesale `coderabbitai resolve` action. The automation must resolve comments only after per-finding verification.
    

    Learnt from: don-petry
    URL: https://github.com/petry-projects/.github-private/issues/1813
    
    Timestamp: 2026-09-13T13:56:45.324Z
    Learning: For GitHub issue `#1813`, ADR-0009 must land alongside the verified-disposition merge-gate implementation. It must define the addressed contract, run provenance requirement, limitations of marker-only identity under ADR-0008, the human-comment disposition rule, and the auditability trade-off of GraphQL `minimizeComment`.
    

    Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

    You are interacting with an AI system.

  10. don-petry commented on Sep 13, 2026

    @don-petry
    CollaboratorAuthor

    Dev-Lead Implementation Plan

    Issue: #1813 — pr-review can't approve ANY open PR: maintainer-comment gate needs pushedDate (always null) and treats codeant/qodo/auto-rebase comments as maintainer findings

    Scope

    Replace the maintainer issue-comment gate's pushedDate/timestamp "addressed" proxy with a verified-disposition model: "addressed" is a comment minimized server-side with classifier RESOLVED (mirrors review-thread isResolved); dev-lead posts one evidence-backed disposition reply per undispositioned non-agent comment, and the harness verifies it and minimizes the original.

    Implementation Checklist

    • scripts/lib/maintainer-comment-gate.sh — remove all pushedDate reads (AC1); maintainer_gate_head_committer_date returns committer.date (kept for the review-thread gate); rewrite check_maintainer_comments <snapshot> [bot_user] (drop head_date) to block any non-agent issue comment not minimized as RESOLVED (AC2) — the only exclusions are our bot login and our own agent-marked disposition/ack/note replies; no bot-author exclusion list. Distinct verdict path so an undeterminable snapshot (rc=2) is never disguised as the legitimate hold (AC8).
    • scripts/lib/comment-disposition-verify.sh — new pure lib: parse <!-- dev-lead:comment-disposition id=<id> disposition=<fixed|invalid|out-of-scope|answered|informational> [sha=<sha>] [ref=#<n>] -->; cdv_authorize encodes AC4/AC5 (human comment auto-resolves only on a verified fixed; bots on any verified disposition). Reuses addressed-claim-verify.sh for the fixed sha-on-head check.
    • scripts/review-one-pr.sh — call the new gate signature; emit a distinct reason (undispositioned-pr-comment for the hold, maintainer-comment-gate-error for undeterminable, AC8); keep FORCE_REVIEW bypass + prior-approval dismissal.
    • scripts/dev-lead-fix-reviews.sh — new resolve_dispositioned_comments(): for each undispositioned non-agent issue comment, find our disposition reply (idempotent per comment id, AC7), verify per disposition type, and minimizeComment classifier RESOLVED (AC4/AC5). Wire it into the fix-reviews / fix-bot-comment resolution-gate blocks (never resolves without a commit).
    • prompts/dev-lead/fix-reviews.md — instruct dev-lead to research/analyze/respond-once to every undispositioned PR issue comment (not just review threads), emit the disposition marker, route informational comments with one tracking issue per repo (AC3/AC6), and never answer its own replies (AC7).
    • scripts/lib/consumer-manifest.json + .github/workflows/lint.yml — register the new lib and bats files (also adds the pre-existing test_maintainer_comment_gate.bats, currently unregistered).

    Tests to Write

    • tests/dev-lead/unit/test_maintainer_comment_gate.bats (rewrite): no pushedDate; undispositioned codeant-ai/qodo-code-review/auto-rebase-conflict comment → blocks (AC9b); all comments minimized RESOLVED → passes (AC9a); our disposition reply never blocks (AC9e); malformed → rc=2 (AC8).
    • tests/dev-lead/unit/test_comment_disposition_verify.bats (new): marker parse (valid/missing/multiple/bad); fixed citing a sha not on head → not authorized (AC9c); human comment + non-fixed disposition → not authorized/blocks (AC9d); a bot boilerplate reply to our disposition is not a new finding (AC9f).

    Test Command

    scripts/run-bats.sh tests/dev-lead/unit/test_maintainer_comment_gate.bats tests/dev-lead/unit/test_comment_disposition_verify.bats and bash .dev-lead/scripts/dev-lead-lint.sh

    Note: AC10 (sweeping the already-stranded open PRs) is an operational post-merge step, not a code change, and is out of this PR's diff.

  11. don-petry commented on Sep 13, 2026

    @don-petry
    CollaboratorAuthor

    Dev-Lead: Implementation Complete — PR #1816

    The implementation for issue #1813 is durable: commits are pushed and a pull request is open. This record is backed by verifiable artifacts you can confirm in one click.

    Review 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.

  12. added a commit that references this issue on Sep 13, 2026
  13. added 3 commits that reference this issue on Oct 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugBug reportsdev-leadFor dev-lead agent pickup

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions