Skip to content

Implement canonical PR ledger and fail-closed PR routing for review and merge integrity #244

Description

@fujiwaranosai850

From research #243

Overview

DevClaw currently re-discovers “the PR for this issue” from provider lookups (getPrStatus(issueId), mergePr(issueId), PR diff/review-comment helpers). That works only while one issue has exactly one linked PR. Once repeated retry cycles create sibling PRs for the same issue, routing becomes heuristic and can drift onto a stale PR.

Implement a durable canonical PR model so one issue has one authoritative active PR, reviewer dispatch and heartbeat review consume that exact PR identity, merge can only merge the reviewed canonical PR, and duplicate sibling PR creation is either prevented or handled through explicit supersession.

Implementation Checklist

Phase 1: Canonical PR state model and ingestion (~1.5 days)

  • Add an issue-scoped canonical PR state module (for example under lib/services/ or lib/state/) that can persist and load canonical PR records keyed by project + issue id, including number, url, sourceBranch, repo, status, updatedAt, and supersededPrs metadata.
  • Extend provider abstractions in lib/providers/provider.ts and GitHub implementation in lib/providers/github.ts with PR-specific lookup helpers (for example getPrByUrl / getPrByNumber / getPrStatusByUrl) so later routing does not have to rediscover PR identity from issueId alone.
  • Update developer completion ingestion in lib/tools/worker/work-finish.ts and/or lib/services/pipeline.ts so developer:done resolves a concrete PR identity, validates it references the issue, and atomically records it as the canonical PR.
  • Fail closed when completion cannot resolve exactly one eligible PR, or when explicit prUrl, provider lookup, current branch, and worker-reported identity disagree in a way that would make canonical routing ambiguous.

Phase 2: Review dispatch and merge routing integrity (~1.5 days)

  • Refactor lib/dispatch/pr-context.ts and lib/dispatch/index.ts to load PR diff, URL, feedback, and branch name from the stored canonical PR record first, with provider discovery used only for validation/reconciliation.
  • Refactor lib/services/heartbeat/review.ts so review checks, feedback transitions, and approval handling operate on the canonical PR record instead of provider.getPrStatus(issueId) alone.
  • Change merge execution in lib/services/pipeline.ts and lib/providers/github.ts to merge by canonical PR identity (URL/number), and reject merge if the approved PR and canonical PR differ or if the canonical PR changed after approval.
  • Surface an explicit routing-integrity hold/error path when multiple linked PRs exist but canonical PR state is missing, stale, or mismatched, instead of silently guessing.

Phase 3: Duplicate PR prevention, supersession, and UX (~1 day)

  • Add explicit supersession handling so when a new PR intentionally replaces the canonical PR, the previous canonical PR is recorded as superseded and no longer eligible for review or merge routing.
  • Update worker dispatch messaging in lib/dispatch/message-builder.ts and related PR-context helpers so all feedback/retry cycles, not just merge-conflict flows, instruct developers to reuse the canonical branch/PR unless the task explicitly requires replacement.
  • Add optional GitHub-side hygiene in lib/providers/github.ts or routing logic, such as a comment/reaction/label on superseded sibling PRs, so operators can see which PR is canonical and which are stale.

Phase 4: Regression tests and safety coverage (~1 day)

  • Add provider/unit tests covering multiple linked PRs for one issue, canonical PR selection, supersession, and mismatch detection in lib/providers/ tests and/or new focused tests.
  • Add pipeline/review e2e coverage in lib/services/pipeline.e2e.test.ts for: newer replacement PR becomes canonical, reviewer dispatch uses canonical PR, approval of PR X cannot merge PR Y, and ambiguous multi-PR state fails closed.
  • Add work-finish tests covering divergence cases where summary or explicit prUrl conflicts with provider-discovered PR state, ensuring completion rejects ambiguous routing instead of proceeding.

Dependencies & Blockers

  • Need a small durable state design that fits existing DevClaw project state without making provider portability worse.
  • Be careful with migration/backfill behavior for issues already in review when canonical PR state is absent.
  • Review logic must preserve current human-review and agent-review behavior while tightening safety around merge identity.

Estimated Total: 4-5 days

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions