fix(#827): dismiss stale bot approvals

[shairevivo]

Summary

Dismiss stale approvals from the review bot before processing a new verdict,
so GitHub does not retain approval for an older commit.

Related Issue

Fixes #827

Changes

• dismiss prior APPROVED reviews belonging to the authenticated bot
• apply the cleanup to comment-only verdicts as well as formal reviews
• preserve other users' reviews and the existing CHANGES_REQUESTED cleanup
• add regression coverage for filtering and dismissal errors

Testing

• make lint
• make go-vet
• go test ./internal/cli
• Focused review-flow tests
• Coverage check for changed production code (dismissStaleApprovals: 100%)

Checklist

• PR title follows Conventional Commits
• Commits are signed off (DCO)
• Codex co-author trailer included

[github-actions]

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

[qodo-code-review]

PR Summary by Qodo

Dismiss stale bot approvals before new review verdicts

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Dismiss the authenticated bot's prior approvals before processing each new verdict.
• Clean up comment-only verdicts while preserving reviews submitted by other users.
• Cover approval filtering, successful dismissal, and non-fatal API failures.

Diagram

sequenceDiagram
    actor Bot as Review Bot
    participant Flow as Review Flow
    participant Forge as Forge Client
    participant Cleanup as Approval Cleanup
    participant API as Forge API
    Bot->>Flow: Process verdict
    Flow->>Forge: Identify user and reviews
    Forge->>API: List reviews
    API-->>Forge: Existing reviews
    Forge-->>Flow: User and reviews
    Flow->>Cleanup: Remove bot approvals
    loop Matching APPROVED reviews
        Cleanup->>Forge: Dismiss review
        Forge->>API: Dismiss approval
    end
    alt Formal or inline verdict
        Flow->>Forge: Post new review
    else Comment-only verdict
        Flow-->>Bot: Skip formal review
    end

Loading

High-Level Assessment

Extending the existing pre-submission stale-review cleanup is the appropriate approach. Event-specific cleanup was considered, but unconditional filtering by authenticated user and APPROVED state also covers comment-only early returns without affecting other users' reviews.

Files changed (2) +55 / -1

Bug fix (1) +19 / -0

postreview.goDismiss stale authenticated-bot approvals before new verdicts +19/-0

Dismiss stale authenticated-bot approvals before new verdicts

• Adds stale approval cleanup to the existing pre-submission review workflow. Only APPROVED reviews from the authenticated user are dismissed, and API failures remain non-fatal.

internal/cli/postreview.go

Tests (1) +36 / -1

postreview_test.goCover stale approval cleanup and failure handling +36/-1

Cover stale approval cleanup and failure handling

• Updates review-flow expectations and adds regression tests for comment-only verdicts, user filtering, dismissal messages, and non-fatal dismissal errors.

internal/cli/postreview_test.go

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. A failed review erases prior approval ✓ Resolved 🐞 Bug ☼ Reliability

Description

submitFormalReview calls dismissStaleApprovals before attempting CreatePullRequestReview, and
no failure path restores successfully dismissed reviews. When an approve, request-changes, or
inline-comment submission fails, including its fallback, the previous bot approval is already gone
even though the intended replacement verdict was never created.

Code

internal/cli/postreview.go[327]

+		dismissStaleApprovals(ctx, client, owner, repo, pr, user, reviews, printer)

Relevance

●●● Strong

Accepted reliability findings address state loss and failure-path behavior;
cleanup-before-submission risks losing approvals on failed replacement.

PR-#6001
PR-#279

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The added call dismisses approvals during cleanup before any formal review is submitted. The later
creation path can return an error after both the original request and its fallback fail, while the
dismissal helper only logs dismissal failures and provides no rollback; the command has already
posted its sticky comment before entering this function. GitLab further confirms that dismissal
performs a real /unapprove, so this is destructive state rather than presentation-only cleanup.

internal/cli/postreview.go[320-329]
internal/cli/postreview.go[388-410]
internal/cli/postreview.go[694-705]
internal/cli/postreview.go[139-145]
internal/forge/gitlab/mr.go[523-538]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Stale approvals are dismissed before the replacement formal review is created. If review creation fails, the previous approval remains permanently removed without a replacement verdict.

## Issue Context
Comment-only verdicts without a formal review still need explicit cleanup, but formal verdict paths should not destructively remove approval state until the new verdict has succeeded. Preserve the intended GitHub and GitLab semantics, noting that GitLab models approval as the authenticated user's current approval rather than independent historical review objects.

## Fix Focus Areas
- internal/cli/postreview.go[320-329]
- internal/cli/postreview.go[371-412]
- internal/cli/postreview.go[690-706]
- internal/cli/postreview_test.go[286-317]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

✅ Compliance rules (platform): 67 rules

✅ Skills: writing-user-docs, writing-adrs

Review mode: ⚖️ Balanced: This modifies review-state handling in a GitHub workflow, affecting approval dismissal semantics and comment-only verdicts, so a careful full review is warranted despite the localized diff.

Tip of the day

💡 Did you know, you can copy the agent prompt from any finding and feed it to your IDE agent

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[rh-hemartin]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 1:12 PM UTC · Completed 1:27 PM UTC

Commit: 3975e49 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.68

[fullsend-ai-review]

Risk Assessment: moderate (2/5)

Details

A small, well-tested two-file change fixing a focused bug, but the touched files carry elevated risk signals from high multi-author contention and a heavy fix/revert history over the past 90 days.

[fullsend-ai-review]

Review

Findings

Low

• [code-organization] internal/cli/postreview.go:320 — In the var block added to submitFormalReview, user string is pre-declared unnecessarily. Unlike priorReviews, which is assigned inside the else block and genuinely needs a pre-declaration to remain in scope after the if/else chain, user is declared by the user, err := short-variable-declaration that immediately follows the var block. The pre-declaration of user is redundant and creates a slightly misleading symmetry — both variables appear to need escape-scoping, but only priorReviews actually does.
  Remediation: Keep only priorReviews []forge.PullRequestReview in the var block and let user, err := client.GetAuthenticatedUser(ctx) declare user directly.

---

Next steps:

• /fs-fix — agent addresses review findings automatically
• /fs-fix <your instruction> — agent fixes with your specific guidance
• Push commits directly — review re-runs automatically on push
• /fs-fix-stop — disable automatic fix runs for this PR

[fullsend-ai-review]

See the review comment for full details.