dispatcher-conflict-pr-reaper-plan

Dispatcher stale/conflict PR reaper — design plan (#6555)

Status: DESIGN / PLAN — not yet implemented. This document is the deliverable of the plan-labelled issue #6555. It must pass /review-plan and a human sign-off before any code lands against the live dispatcher loop (scripts/queue-dispatcher-loop.py). Do not auto-implement.

1. Problem

#6551 fixed the source of the auto-rotate PR pile-up: PR #6552 added _close_superseded_rotation_prs() to scripts/sops-sync-receiver.py, so each new rotation run now closes earlier open chore/auto-rotate-<cred>-* PRs for the same credential. That converges future duplicate rotations at the moment a new rotation runs.

It does not close the general gap (Q2 in #6551): there is no give-up path for a PR that stays permanently CONFLICTING/DIRTY.

• A SOPS re-encryption rewrites the entire ciphertext. Once any sibling PR touching that SOPS file lands on main, every other open PR touching the same file is permanently conflicting and cannot be auto-rebased (there is no clean 3-way merge of two full-file ciphertext rewrites).
• scan_prs_for_review() (scripts/queue-dispatcher-loop.py:2111, called once per cycle at :3204) has no notion of “give up”. Post-#6387 the review pipeline removes ai-review-queued on every review outcome, so a permanently-conflicting / merge-failed PR is re-reviewed every cycle forever — not stuck, but futile, and it burns review tokens every ~2-minute cycle.
• The 2026-08-16 stuck PRs (#6337/#6336/#6335/#6333) were the concrete symptom. Those specific PRs were reconciled by secret-manager under #6553, but the dispatcher still has no mechanism to prevent the next occurrence.

2. Goal

Add a dispatcher-side reaper pass that, for any open PR from a worker/automation branch whose merge state has been CONFLICTING/DIRTY for N consecutive dispatcher cycles, does exactly one of two things:

1. Close it (with a “superseded, permanently conflicting” comment + delete branch) — when a sibling / superseding PR for the same file has already merged.
2. Escalate it — otherwise: apply human-action, post a one-time escalation comment, and stop re-enqueuing it for review.

Both outcomes stop the futile re-review loop. Closing is only taken when it is provably safe; everything else escalates to a human. When in doubt, escalate — never close.

3. Key technical facts (verified against the current code)

• scan_prs_for_review() already skips any PR labelled human-action (queue-dispatcher-loop.py:2179). So the “stop re-enqueuing for review” requirement is satisfied simply by applying human-action — no change to the review-scan filter logic is needed.
• The dispatcher talks to GitHub over urllib REST against GH_API (:82), gated on GH_TOKEN (:2124). It already lists open PRs per repo at :2164 (GET /repos/{repo}/pulls?state=open&base={base_branch}&per_page=50&sort=updated&direction=desc). That list endpoint does not return mergeable/mergeable_state. Conflict state requires a per-PR GET /repos/{repo}/pulls/{n}, which returns mergeable (bool|null) and mergeable_state (dirty|blocked|behind|clean|unstable|draft|has_hooks|unknown).
• GitHub computes mergeability asynchronously. The first read after a base change returns mergeable=null, mergeable_state="unknown". unknown must be treated as “not yet determined” → do not increment the counter, do not act this cycle. Only mergeable_state=="dirty" (the gh pr view --json mergeable equivalent value CONFLICTING) is an authoritative conflict.

  Note: the issue conflated two GitHub fields. gh’s mergeable enum uses CONFLICTING; GraphQL/gh’s mergeStateStatus uses DIRTY; REST mergeable_state uses dirty. They refer to the same condition. This plan standardises on REST mergeable_state=="dirty" for consistency with the existing urllib-based scan_prs_for_review().

• Supabase REST helpers already exist in the dispatcher: supa_get(path) (:961), supa_post(path, body) (:884), supa_patch(path, body) (:995), supa_delete(path) (:1008).
• Existing “close a superseded PR” reference implementation: _close_superseded_rotation_prs(credential_name, keep_branch, gh_env) in scripts/sops-sync-receiver.py:463 — gh pr close <n> --comment ... --delete-branch. The reaper is the cycle-time backstop for the PRs that slipped past that rotation-time cleanup.
• close_shipped_issues() (:2252) is the canonical pattern to copy for: per-cycle window, per-cycle cap, fail-closed GitHub reads (a None/error read defers, never acts blind), and idempotency.

4. Proposed design

4.1 New persistent state (counter must survive restarts — issue requirement)

The N-consecutive-cycle counter must not live in process memory (the dispatcher loop restarts, and a restart must not reset a PR’s conflict age to zero). Add one small Supabase table, dispatcher-owned, following the dev_r_* convention:

-- supabase/migrations/<UTC-ts>_dispatcher_pr_conflict_state_6555.sql
CREATE TABLE IF NOT EXISTS public.dev_r_dispatcher_pr_conflict (
    repo                        text        NOT NULL,
    pr_number                   integer     NOT NULL,
    first_conflicting_at        timestamptz NOT NULL DEFAULT now(),
    consecutive_conflict_cycles integer     NOT NULL DEFAULT 0,
    last_state                  text,
    last_checked_at             timestamptz NOT NULL DEFAULT now(),
    escalated                   boolean     NOT NULL DEFAULT false,
    reaped_action               text,        -- 'closed' | 'escalated' | NULL (still tracking)
    reaped_at                   timestamptz,
    PRIMARY KEY (repo, pr_number)
);

• Applied via the per-repo ledger runner (public._p24_applied_migrations + execute_sql / Management API /database/query) per docs/playbooks/supabase-per-repo-migration-ledger.md. Never mcp__claude_ai_Supabase__apply_migration (banned 5645).
• State transitions per cycle, per open worker PR observed:
  • mergeable_state=="dirty" → upsert row, consecutive_conflict_cycles += 1, set last_state.
  • mergeable_state in ("clean","unstable","behind","blocked") (i.e. not dirty, mergeability known) → reset: delete the row (or set counter 0, escalated=false). A PR that recovered must not carry stale conflict age.
  • mergeable_state=="unknown" / read error → leave row untouched (no increment). Fail-closed.
  • PR no longer open (merged/closed) → delete the row.

4.2 New function reap_conflicting_prs() (in queue-dispatcher-loop.py)

Runs once per cycle, immediately before scan_prs_for_review() (so a PR escalated this cycle is already human-action-labelled before the review scan runs). Guarded by GH_TOKEN and REVIEW_REPOS exactly like the review scan; wrapped in the same try/except at the call site (:3202).

Per repo, per open PR from the existing list call:

1. Scope filter — only worker/automation PRs. Act only when the PR author login is in an allowlist of the automation identities (AI-Dev-BMS4-1, ai-dev-bms4-2, the App-2109526 p24-infra-admin bot) or the head branch matches a worker-branch prefix (chore/auto-rotate-, feat/, fix/, bug/, plan/, docs). Never reap a human’s PR.
2. Determine merge state via per-PR GET /repos/{repo}/pulls/{n} → mergeable_state. unknown/error → skip (fail-closed). Not dirty → reset state, skip.
3. Increment / persist the counter in dev_r_dispatcher_pr_conflict.
4. Threshold gate. If consecutive_conflict_cycles < N → done for this PR this cycle.
5. At / past threshold — decide close vs escalate:
   • Fetch the PR’s changed files: GET /repos/{repo}/pulls/{n}/files.
   • Superseded check: is there a merged PR into the same base that touched any of the same files and merged after this PR was created? (For chore/auto-rotate-<cred>-* branches, the stronger, cheaper signal is a merged sibling chore/auto-rotate-<cred>-* — reuse the credential-name parse from _close_superseded_rotation_prs.) Query merged PRs via GET /repos/{repo}/pulls?state=closed&base={base}&sort=updated&direction=desc filtered to merged_at != null, or the search API.
     • Superseded confirmed → CLOSE: gh pr close <n> --comment "<superseded/permanently conflicting, #6555>" --delete-branch (mirror _close_superseded_rotation_prs). Record reaped_action='closed', reaped_at=now().
     • Not confirmed (orphan, or superseded-check errored) → ESCALATE: apply human-action (_gh_add_label), post a one-time escalation comment (guard on the escalated flag), set escalated=true, reaped_action='escalated'. scan_prs_for_review() then skips it forever (:2179).
6. Bounds & safety (copy close_shipped_issues): at most DISPATCHER_PR_REAP_MAX_PER_CYCLE destructive actions per cycle; every GitHub read fails closed; all failures logged and swallowed — the reaper must never break the dispatch loop.

4.3 Config / feature flags (env, read at module load like the other dispatcher settings)

Env var	Default	Meaning
DISPATCHER_PR_REAP_ENABLED	false at first deploy	Master on/off. Ship OFF; enable after observation.
DISPATCHER_PR_REAP_DRY_RUN	true at first deploy	When true, log the close/escalate decision but take no destructive action. Flip to false after validating logs.
DISPATCHER_PR_REAP_CONFLICT_CYCLES	5	N. ~2-min cycles → ~10 min of stable conflict before acting. Avoids acting on a transient conflict during an in-flight rebase.
DISPATCHER_PR_REAP_MAX_PER_CYCLE	10	Per-cycle destructive-action cap.
DISPATCHER_PR_REAP_AUTHOR_ALLOWLIST	bot logins	Comma-separated automation author logins eligible for reaping.

Dry-run + disabled-by-default is the concrete mechanism that satisfies the issue’s “human-supervised before deploy” constraint: the first deploy only logs what it would do.

5. Files to change (implementation issue — NOT this plan PR)

File	Change
scripts/queue-dispatcher-loop.py	Add reap_conflicting_prs(); call it before scan_prs_for_review() at :3202; add the 5 env-var reads near the other review settings.
supabase/migrations/<ts>_dispatcher_pr_conflict_state_6555.sql	Create dev_r_dispatcher_pr_conflict; apply via per-repo ledger runner.
scripts/tests/test_dispatcher_pr_reaper.py (new)	Unit tests (see §6).
docs/dispatcher-conflict-pr-reaper-plan.md	This doc → mark implemented + link the impl PR.
dispatcher .env (secrets-sync, sys-admin)	Add the 5 env vars (start disabled + dry-run).

6. Test plan (all in scripts/tests/, stdlib unittest + unittest.mock, no network)

• test_close_on_superseded — dirty ≥ N + a merged sibling touching the same file → gh pr close invoked with --delete-branch; state row reaped_action='closed'.
• test_escalate_on_orphan — dirty ≥ N + no superseding merge → human-action added + one escalation comment; no close; scan_prs_for_review then skips it.
• test_threshold_not_reached — dirty for N-1 cycles → no action; counter persists.
• test_counter_reset_on_recovery — PR returns to mergeable_state="clean" → state row cleared, counter back to 0.
• test_noop_on_mergeable — clean PR is never touched.
• test_unknown_state_fail_closed — mergeable_state="unknown" / API error → no increment, no action.
• test_one_time_escalation_comment — second cycle past threshold with escalated=true → no second comment.
• test_per_cycle_cap — more than MAX_PER_CYCLE reapable PRs → at most the cap acted on.
• test_dry_run — DRY_RUN=true → decisions logged, no gh pr close / label / comment calls.
• test_scope_filter_skips_human_prs — a non-allowlisted author’s dirty PR is never reaped.

7. Regression risks

• Closing a PR that is not actually superseded (data-loss-ish — a human would have to reopen). Mitigated by: close only on a confirmed merged sibling touching the same file; every uncertainty path escalates instead of closing; dry-run + disabled-by-default first deploy.
• Reaping a legitimate long-lived human PR. Mitigated by the author/branch scope filter (§4.2.1).
• Extra GitHub API calls (1 per-PR merge-state GET + 1 files GET + 1 merged-PR list only for PRs at threshold). Bounded by per_page=50 on the existing list, the threshold gate (files/merged lookups happen only for PRs that reached N dirty cycles), and MAX_PER_CYCLE.
• Interaction with _close_superseded_rotation_prs — both may close the same rotation PR. Both are idempotent/best-effort on gh pr close; a second close on an already-closed PR is a harmless no-op.
• Interaction with close_shipped_issues — different objects (issues vs PRs); no overlap.

8. Compliance

• No new service (a function inside the existing queue-dispatcher — no new dev_r_services row). No new AI system (deterministic housekeeping, no model call) — no dev_r_ai_systems entry.
• Update the dispatcher ops doc (docs/ai-agent-operations.md / dispatcher section) when implemented.
• New Supabase table via the per-repo ledger runner only; apply_migration is banned (#5653/#5645).

9. Rollout (human-supervised, per the issue’s constraint)

1. Merge this plan (docs-only). Run /review-plan on #6555; iterate.
2. Open a separate implementation issue for the code once the plan is human-approved.
3. Deploy the code with DISPATCHER_PR_REAP_ENABLED=false.
4. Enable with ENABLED=true + DRY_RUN=true; watch dispatcher logs for the close/escalate decisions it would make over several cycles / a real rotation event.
5. Flip DRY_RUN=false once the logged decisions look correct.

10. Related

• #6551 (root cause) · #6552 (source fix, merged: _close_superseded_rotation_prs) · #6553 (secret-manager reconciliation of the then-stuck PRs 6333) · #6387 (ai-review-queued removed on every review outcome — the reason dead PRs are re-reviewed forever).