feat: implement issue #1415 — [Phase 4] Close the maintainer review-gate gap — dev-lead PRs cannot be blocked by the owner (shared identity + agent-resolved threads)

.github/workflows/lint.yml

@@ -128,7 +128,8 @@
             tests/test_dev_lead_concurrency_guard.bats \
             tests/test_validate_workflow_schedules.bats \
             tests/caller_stub_freeze.bats \
-            tests/pr_review_canary.bats
+            tests/pr_review_canary.bats \
+            tests/dev-lead/unit/test_maintainer_review_thread_gate.bats
 
   actionlint:
     # General workflow linting (#1256, epic #1052 Part D): actionlint validates
@@ -196,7 +197,7 @@
     timeout-minutes: 5
     steps:
       - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1  # v7.0.1
         # NOTE: persist-credentials is intentionally left at the default (true) —
         # vci_resolve_reusable needs `git fetch origin` to authenticate against this
         # private repo when resolving same-repo channel tags. Setting
         # persist-credentials: false would silently push every such caller into the

docs/agentic-interaction-model.md

@@ -574,3 +574,83 @@ which is the human checkpoint. During the 14-day window, treat any stall-candida
 rollback decision per §11.2 rather than a routine triage item. After the window settles, the stall
 scan remains as a **permanent** detector (it is cheap and detect-only) — settling ends the
 *heightened* watch, not the instrument.
+
+---
+
+## 12. §7 rule 2 applies to review and merge gates, not just automation budgets (#1415)
+
+**Normative rule.** §7 rule 2 — *"a safety cap whose reset is reachable by the runaway is not a
+cap; resets must be human-gated"* — is **not limited to numeric budgets and circuit breakers.** It
+governs **every gate an agent's merge must pass**, including **review gates** and the **merge gate
+itself**. Restated for this class:
+
+> **A review or merge gate whose reset is reachable by the agent is not a gate.** If an agent can
+> clear the very signal that is supposed to block its own PR — by resolving the blocking review
+> thread, dismissing the blocking review, or otherwise satisfying the check itself — the gate holds
+> only by the agent's goodwill, not by control. The reset must be reachable **only by a human**.
+
+This is the review-path counterpart of two existing applications of the same rule: the per-PR
+automation budget (§9), whose re-engagement is human-gated; and the timer stop-condition (§6.2.4),
+which forbids a timer from re-arming a runaway. The maintainer **issue-comment** gate (#1290) and
+the maintainer **review-thread** gate (#1415) are the same rule applied to the two ways a
+maintainer finding reaches a PR.
+
+### 12.1 Why the shared-identity case makes this acute
+
+On a **dev-lead-authored PR**, dev-lead commits, reviews, and resolves as the owner account
+`don-petry` (AGENTS.md "Agent identity & credential secrets"; #1316 set this deliberately — dev-lead
+acts as the owner, `pr-review` as the machine user `donpetry-bot`). That shared identity removes
+**both** of the owner's mechanically-blocking review paths:
+
+1. GitHub refuses `CHANGES_REQUESTED` on your own PR ("Can not request changes on your own pull
+   request"), so the ergonomic blocking review is unavailable to the owner.
+2. The inline-review-thread fallback (`required_review_thread_resolution`) is defeated because the
+   agent, running as `don-petry`, can **resolve the maintainer's own threads** — observed on PR
+   #1413, where four maintainer threads ended `resolvedBy: don-petry` and auto-merge proceeded.
+
+Because **login cannot separate the agent from the maintainer** in this case, the discriminator is
+the **automation marker** in a thread's originating comment (`<!-- pr-review-agent … -->`,
+`<!-- persona:… -->`, `<!-- dev-lead … -->`, `<!-- dependency-advisory -->`), exactly as the
+issue-comment gate discriminates. The rule is enforced by two **coupled** halves — a resolve-guard
+that stops the agent clearing a marker-less (maintainer) thread, and an approval gate that withholds
+pr-review's approval while an unresolved maintainer thread postdates the last push. The gate can
+safely treat *resolution* as a human clear **only because** the guard makes resolution unreachable by
+the agent. See `docs/pr-review-agent/maintainer-comment-gate.md` ("The review-thread sibling") and
+`scripts/lib/maintainer-review-thread-gate.sh`.
+
+### 12.2 Decision: keep the shared identity + add the gate — the distinct-identity alternative is rejected (AC #7)
+
+Two ways to close the #1413 gap were evaluated:
+
+- **(a) Give dev-lead a distinct commit/PR identity** (e.g. its own `dev-lead-bot` account) so a PR
+  it authors is authored by a *different* account than the owner. GitHub would then permit the owner
+  to `CHANGES_REQUESTED` it, and login alone would separate the agent's thread resolutions from the
+  maintainer's — no marker discriminator needed.
+- **(b) Keep the shared `don-petry` identity and add the marker-keyed gate + resolve-guard** (this
+  implementation).
+
+**Decision: option (b).** Option (a) **reverses the deliberate #1316 choice** (dev-lead acts as the
+owner `don-petry`; `pr-review` acts as `donpetry-bot`) and carries consequences that are out of scope
+for a gate-integrity fix and are not clearly net-positive:
+
+- **CODEOWNERS.** dev-lead PRs are authored-as-owner today; a distinct bot identity would need to be
+  added to CODEOWNERS / review-requirement rulesets to retain the same approval semantics, and would
+  change who is requested for review across the fleet.
+- **PAT / credential surface.** A new account means a new PAT secret
+  (`GH_PAT_<ACCOUNT>` per AGENTS.md), new `runtime.identity` manifest wiring, and a new machine user
+  to provision, secure, and rotate — widening the credential surface the #1316 model deliberately
+  kept to two accounts.
+- **Persona-manifest / identity-resolution.** `runtime.identity.account`, `resolve-persona-identity.sh`,
+  and the `verify-persona-identity` lint job all encode the current account mapping; a re-identity is a
+  cross-cutting change to that machinery, not a localized fix.
+- **It does not generalize.** Even with distinct identities, the *dependency-advisory* pass and other
+  automation still post **as `don-petry`**, so login-based discrimination would remain insufficient on
+  the issue-comment path (#1290) — the marker discriminator is needed regardless. Option (b) reuses one
+  mechanism for both paths; option (a) would still need the marker for #1290 while adding an identity
+  migration for #1415.
+
+The marker-keyed gate closes the gap **without** touching the identity model, reuses the proven #1290
+shape, and keeps a single discriminator across both the comment and review paths. If a distinct
+dev-lead identity is later adopted for *other* reasons, this gate remains correct (a marker-less
+maintainer thread is still a maintainer finding) and the login signal simply becomes a redundant
+confirmation — so option (b) does not foreclose option (a).

docs/pr-review-agent/maintainer-comment-gate.md

@@ -92,6 +92,82 @@ dispatch) bypasses the gate — a human `@mention` *is* the human-in-the-loop th
 exists to obtain, and the same comment that triggers a re-review would otherwise
 deadlock the gate. Required-check enforcement at the ruleset still applies.
 
+## The review-thread sibling (issue #1415)
+
+The issue-comment gate above closes the *comment* path. Its sibling — the
+**maintainer review-thread gate**
+([`scripts/lib/maintainer-review-thread-gate.sh`](../../scripts/lib/maintainer-review-thread-gate.sh))
+— closes the *review* path, for the specific case that makes a dev-lead-authored
+PR different from any other PR: **dev-lead acts as the owner `don-petry`**
+(AGENTS.md "Agent identity & credential secrets"; #1316 set this on purpose). That
+shared identity removes *both* of the owner's mechanically-blocking review paths:
+
+1. **`CHANGES_REQUESTED` is unavailable** — GitHub refuses "Can not request changes
+   on your own pull request" when the owner reviews a PR authored by the owner
+   account.
+2. **The inline-thread fallback is defeated** — inline review threads normally block
+   via `required_review_thread_resolution`, but the agent (running as `don-petry`)
+   can *resolve the maintainer's own threads*. Observed on PR #1413: four maintainer
+   threads, all `resolvedBy: don-petry`, gate cleared, auto-merge enabled.
+
+This is [#860](https://github.com/petry-projects/.github-private/issues/860)
+normative rule 2 applied to review gates: **a gate whose reset is reachable by the
+agent is not a gate** (`docs/agentic-interaction-model.md` §7 rule 2, extended in
+§12 to review/merge gates).
+
+### Two coupled halves
+
+Because login cannot separate the agent from the maintainer here (both are
+`don-petry`), the fix keys on the **automation marker** in a thread's originating
+comment — the *same* discriminator this issue-comment gate uses — and has two
+coupled halves:
+
+- **The resolve-guard (dev-lead side).** `review_thread_is_agent_authored` classifies
+  a thread by its originating comment's marker. dev-lead's resolve call-site
+  (`resolve_actor_outdated_threads` in
+  [`scripts/dev-lead-fix-reviews.sh`](../../scripts/dev-lead-fix-reviews.sh)) and its
+  prompt ([`prompts/dev-lead/fix-reviews.md`](../../prompts/dev-lead/fix-reviews.md))
+  **skip** any marker-less thread — a maintainer finding — logging a visible
+  `::warning::` rather than silently resolving it. The agent may only resolve threads
+  carrying one of our markers (`<!-- pr-review-agent … -->`, `<!-- persona:… -->`,
+  `<!-- dev-lead … -->`, `<!-- dependency-advisory -->`).
+- **The approval gate (pr-review side).** `check_maintainer_review_threads` withholds
+  pr-review's automated approval while an **unresolved maintainer review thread
+  postdates the last push**, wired into
+  [`scripts/review-one-pr.sh`](../../scripts/review-one-pr.sh) right after the
+  issue-comment gate. Withholding the code-owner approval means the PR does not
+  auto-merge — the same approval-boundary lever the issue-comment gate pulls.
+
+The two halves are **coupled by design**: the gate treats a *resolved* maintainer
+thread as cleared (AC #4: "a human resolving the thread also clears it"), and that is
+safe **only because** the resolve-guard prevents the agent from resolving marker-less
+threads. Resolution therefore becomes a human-only signal — a reset the runaway
+cannot reach.
+
+### Clears the #1290 way, fails closed the #1290 way
+
+- **Addressed by a push.** A commit pushed at/after the finding (head commit
+  `pushedDate` ≥ the thread's originating `createdAt`) marks it addressed, so the
+  normal fix-then-continue flow is not deadlocked. `pushedDate` is a server-recorded
+  timestamp written by GitHub on receipt of the push — unlike `committer.date` (which
+  is commit metadata the author controls), it cannot be set to an arbitrary past or
+  future value. When `pushedDate` is unavailable, the gate fails closed (finding
+  treated as unresolved).
+- **Addressed by a human resolving the thread** (safe per the coupling above).
+- **Fails closed.** Undeterminable authorship, `createdAt`, or push-time → **block**
+  (return 1); a malformed threads snapshot → **fail the PR** (return 2 → exit 1). An
+  inability to confirm a finding was addressed must never read as "no findings".
+- **`FORCE_REVIEW` bypasses** — a human `@mention` *is* the human-in-the-loop the gate
+  exists to obtain, and the same comment that triggers a re-review would otherwise
+  deadlock the gate.
+
+Tests:
+[`tests/dev-lead/unit/test_maintainer_review_thread_gate.bats`](../../tests/dev-lead/unit/test_maintainer_review_thread_gate.bats),
+including a #1413 regression fixture
+([`tests/dev-lead/fixtures/review-threads/pr-1413-maintainer-threads.json`](../../tests/dev-lead/fixtures/review-threads/pr-1413-maintainer-threads.json))
+that reproduces the four marker-less `don-petry` threads and asserts the gate blocks
+and the classifier refuses to resolve each one.
+
 ## Interim workaround (still valid)
 
 Posting findings as **reviews with `CHANGES_REQUESTED`** (or inline review comments)

prompts/dev-lead/fix-reviews.md

@@ -56,7 +56,7 @@ For each open review thread:
 2. Understand the reviewer's concern
 3. Apply the appropriate fix using Edit/Write tools
 4. **Reply to the thread with the specific fix** — see below
-5. **Resolve the thread** — do this for every thread you fix *and* for every thread with `isOutdated: true` (the code it referenced no longer exists)
+5. **Resolve the thread according to the scope below** — never resolve a marker-less human thread, including one with `isOutdated: true` (outdated status never overrides marker ownership).
 
 #### Replying to a thread
 
@@ -81,12 +81,14 @@ After replying, resolve the thread using its `id` from the JSON above:
 gh api graphql -f query='mutation { resolveReviewThread(input: {threadId: "THREAD_NODE_ID"}) { thread { isResolved } } }'
 ```
 
-Resolve a thread when **you actually fixed it** (or it is `isOutdated: true`), per this scope:
+Resolve a thread when **you actually fixed it**, per this scope (outdated status never overrides marker ownership for human threads):
 
-- **Bot threads** (`comments.nodes[0].author.__typename` is `"Bot"` — the GitHub GraphQL API sets this for all bot accounts; note that GraphQL omits the `[bot]` suffix from `comments.nodes[0].author.login` for bots, so the login field alone is not a reliable bot indicator): resolve every one you fixed, **regardless of which reviewer triggered this run**. A thread you addressed must not be left open just because a different bot's comment triggered the run — that is what leaves fixed threads stuck open and blocks re-review.
-- **Human threads** (`comments.nodes[0].author.__typename` is `"User"`): resolve only when `comments.nodes[0].author.login` matches `${TRIGGERING_REVIEWER}`. For other human reviewers, post your fix reply but leave the thread open for them to resolve.
+- **Bot threads** (`comments.nodes[0].author.__typename` is `"Bot"` — the GitHub GraphQL API sets this for all bot accounts; note that GraphQL omits the `[bot]` suffix from `comments.nodes[0].author.login` for bots, so the login field alone is not a reliable bot indicator): resolve every one you fixed **and** every thread with `isOutdated: true`, **regardless of which reviewer triggered this run**. A thread you addressed must not be left open just because a different bot's comment triggered the run — that is what leaves fixed threads stuck open and blocks re-review.
+- **Human threads** (`comments.nodes[0].author.__typename` is `"User"`): **never resolve a maintainer's review thread.** You run as the owner account `don-petry` — the *same* account a human maintainer uses — so `comments.nodes[0].author.login` (even when it matches `${TRIGGERING_REVIEWER}`) **cannot** tell your own thread apart from the maintainer's. Discriminate by the **automation marker** in the thread's originating comment (`comments.nodes[0].body`) instead:
+  - If the originating comment carries one of **our** markers — `<!-- pr-review-agent … -->`, `<!-- persona:… -->`, `<!-- dev-lead … -->`, `<!-- dependency-advisory -->` — the thread is ours and you may resolve it once fixed.
+  - If it carries **no** marker, it is a **maintainer finding**: post your fix reply, **but leave the thread open** for the maintainer to resolve. Resolving it yourself would clear the maintainer's own review gate — exactly the PR #1413 defect this rule closes (#1415). If you cannot determine the marker, **treat it as a maintainer finding and leave it open** (fail closed).
 
-Never resolve a thread you did not fix (except `isOutdated: true` ones). Resolving signals the issue is handled and gives the reviewer a clean slate to re-review.
+Never resolve a thread you did not fix, except outdated bot threads. Never resolve a marker-less human thread — even when you fixed it, and even when it is `isOutdated: true` — reply and leave it for the maintainer to resolve. Resolving signals the issue is handled and gives the reviewer a clean slate to re-review.
 
 ### Phase 2 — Test Verification
 
@@ -132,7 +134,7 @@ Read every changed line as if you are the reviewer seeing the response:
 
 - Address each open thread individually
 - For every thread you fix, post a reply naming the specific change before resolving — never resolve silently
-- Resolve every bot thread you fix (regardless of which reviewer triggered this run) and outdated threads; for human threads, resolve only the triggering reviewer's and leave other humans' open (replied)
+- Resolve every bot thread you fix and every outdated bot thread (regardless of which reviewer triggered this run); for human threads, resolve **only** those whose originating comment carries one of our automation markers, and leave every marker-less (maintainer) thread open with a fix reply — never clear a maintainer's review gate, including when that thread is `isOutdated: true` (#1415)
 - Do not resolve threads you are skipping due to ambiguity — leave those open and note them in your output
 - Do not make changes beyond what the review threads request, except that fixing Tier-1 blockers (failure/timed_out/cancelled/action_required/stale/startup_failure CI checks and CHANGES_REQUESTED reviews) is always in-scope
 - Never revert or undo the PR's own committed changes to satisfy a neutral `COMMENTED`/overview review — that produces a net-zero diff that silently cancels the fix (#1340)