Skip to content

fix(pr-followup): filter merged/closed PRs in ingestMergeStateEvent and ingestReviewEvent#377

Merged
joryirving merged 1 commit into
mainfrom
saffron/fix-364-filter-merged-closed-prs
Jun 15, 2026
Merged

fix(pr-followup): filter merged/closed PRs in ingestMergeStateEvent and ingestReviewEvent#377
joryirving merged 1 commit into
mainfrom
saffron/fix-364-filter-merged-closed-prs

Conversation

@itsmiso-ai

Copy link
Copy Markdown
Contributor

Fixes #364

Add early return checks in both ingestMergeStateEvent and ingestReviewEvent to skip enqueuing work for PRs that are already merged (prMergedAt set) or closed (prState=closed). This prevents stale items from entering the PR-fix queue for terminal PRs.

Changes

  • Add optional prState/prMergedAt to ingestReviewEvent and ingestMergeStateEvent opts
  • Add early return: if (opts.prMergedAt || opts.prState === "closed") return null
  • Pass prState/prMergedAt from sync route (pr.state, pr.merged_at)
  • Pass prState/prMergedAt from webhook route (pr.state, pr.merged_at)
  • Pass through PrFollowupEvent interface and processPrFollowupEvents
  • Add tests for merged/closed PR filtering in both functions

…nd ingestReviewEvent

Add early return checks in both ingestMergeStateEvent and ingestReviewEvent
to skip enqueuing work for PRs that are already merged (prMergedAt set) or
closed (prState=closed). This prevents stale items from entering the PR-fix
queue for terminal PRs.

Changes:
- Add optional prState/prMergedAt to ingestReviewEvent and ingestMergeStateEvent opts
- Add early return: if prMergedAt || prState === 'closed', return null
- Pass prState/prMergedAt from sync route (pr.state, pr.merged_at)
- Pass prState/prMergedAt from webhook route (pr.state, pr.merged_at)
- Pass through PrFollowupEvent interface and processPrFollowupEvents
- Add tests for merged/closed PR filtering in both functions

@its-saffron its-saffron Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Automated Review

Full PR review.

Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic) — routed smart (risk match: public_route_changes)

PR PR 377 Review: Filter merged/closed PRs in ingestMergeStateEvent and ingestReviewEvent

Recommendation: Approve

This PR correctly implements the guard described in issue PR 364, preventing merged/closed PRs from entering the PR-fix queue at ingest time. The fix is targeted, well-tested, and preserves existing auth controls.


Change-by-Change Findings

src/lib/pr-followup-ingestion.ts (core logic)

  • Added prState?: string | null and prMergedAt?: string | null optional parameters to ingestReviewEvent and ingestMergeStateEvent
  • Added early return guard if (opts.prMergedAt || opts.prState === "closed") return null; in both functions, placed before the bot-author check as expected per issue PR 364's suggested fix
  • Extended PrFollowupEvent interface and processPrFollowupEvents to propagate these fields through the event pipeline
  • The guard is positioned correctly: it short-circuits before any database write or queue enqueue operation, which is the intended behavior

src/app/api/pr-followup/sync/route.ts (pull-based ingestion)

  • Passes pr.state and pr.merged_at as prState and prMergedAt in both review and merge-state event branches
  • These values are sourced from the already-cached GithubPR object populated during sync, satisfying the issue's guidance to use cached state rather than making extra GitHub API calls

src/app/api/pr-followup/webhook/route.ts (webhook-based ingestion)

  • Passes pr.state and pr.merged_at from the webhook payload in both pull_request_review and pull_request event handlers
  • Signature verification remains intact (see below)

src/lib/pr-followup-ingestion.test.ts (new tests)

  • Four new test cases covering merged (via prMergedAt) and closed (via prState) PRs for both ingestReviewEvent and ingestMergeStateEvent
  • All tests assert result is null and client.items has length 0, confirming no queue entries are created for terminal PRs

Required Checks

1. Verify route access controls are in place

  • Sync route (src/app/api/pr-followup/sync/route.ts): Calls authorizeRequest(request) and returns 401 if unauthorized. The pull-based path requires valid auth.
  • Webhook route (src/app/api/pr-followup/webhook/route.ts): Uses HMAC-SHA256 signature verification via verifyWebhookSignature with timingSafeEqual (constant-time comparison, preventing timing attacks). Fail-closed by default when WEBHOOK_SECRET is not configured. Auth controls are unchanged and remain in place.

2. Check for unintended public endpoints

  • No new endpoints are introduced in this PR. Only the existing POST /api/pr-followup/sync and webhook handler are modified to pass additional PR state fields. The ingestion guard reduces queue noise; it does not open any new access surface.

Standards Compliance

  • TypeScript types: Optional prState/prMergedAt fields are typed as string | null | undefined, consistent with the existing PrFollowupEvent interface
  • Error handling: Uses the standard error instanceof Error pattern where applicable; this PR adds filtering logic rather than error paths
  • No secrets logged: No token or secret values are involved in this change
  • PR label conventions: N/A for this PR (no labels involved)
  • API route conventions: HTTP status codes are unchanged; sync route returns 401 for unauthorized, 500 for missing GITHUB_TOKEN

Linked Issue Fit

Issue PR 364 explicitly requested filtering at ingest to prevent merged/closed PRs from entering the queue. This PR:

  • Adds the guard at the top of both ingestMergeStateEvent and ingestReviewEvent
  • Uses cached state/merged_at from the sync run (avoids extra API calls) ✅
  • Complements PR PR 362's reconcileStalePrFixItems() cleanup pass with an upstream guard ✅
  • All four new test cases verify the guard works for both terminal states (prMergedAt set, prState=closed) ✅

The issue's acceptance criteria are fully met.


Evidence Provider Findings

No evidence providers configured for this PR.


Tool Harness Findings

  • git_grep for DISPATCH_AGENT_TOKEN|authorization|bearer returned no matches in the diff, confirming no secrets are being logged or mishandled in this change

CI Check Results

  • Validate: completed (success)
  • Docker Build: completed (success)

Both CI checks passed before this review began.


Unknowns / Needs Verification

None. The implementation is complete, tested, and the auth controls are verified.

@joryirving joryirving merged commit b9a8397 into main Jun 15, 2026
3 checks passed
@joryirving joryirving deleted the saffron/fix-364-filter-merged-closed-prs branch June 15, 2026 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(pr-followup): filter merged/closed PRs in ingestMergeStateEvent and ingestReviewEvent

2 participants