fix: markProcessed on chatops auth-fail / exception paths (Bug #6)#51
Open
avrabe wants to merge 1 commit into
Open
fix: markProcessed on chatops auth-fail / exception paths (Bug #6)#51avrabe wants to merge 1 commit into
avrabe wants to merge 1 commit into
Conversation
Why: QA bug-hunter flagged that auth-failure early returns and
exceptions in `handleChatopsIssue` skipped `markProcessed`. Probot
retries the webhook ~5 times on 5xx/timeout, so the user got duplicate
"❌ unauthorised" comments and duplicate analysis-report issues.
What:
- issues.opened: wrap the body in try/finally so the delivery is
marked even when handleChatopsIssue or any octokit call throws.
Logger captures the error so we don't lose visibility, but the
handler swallows it — the alternative is Probot re-running a
handler that already posted comments / created issues / enqueued
work.
- issue_comment.created: surgical fix — every auth-failure return
(10 sites: requireOrgMember/requireAllowedUser, /allow-merge-commit
admin check, /review-pr not-a-PR check) now marks before returning.
Free-text comments that don't match any slash command stay
unmarked (preserves existing dedup-skip behaviour for common case).
Test plan:
- 4 new tests:
- issues.opened marks even when octokit.request throws
- /sync-all-repos marks on non-membership rejection
- /sync-all-repos marks on not-allowed-user rejection
- /review-pr marks on not-a-PR rejection
- npm test → 837 pass (was 834)
- npm run lint → clean
Risk: low — behaviour change is narrow (paths that already had visible
side effects now also mark dedup state). No paths that previously
marked stop marking. The handler now swallows exceptions in
issues.opened instead of letting them propagate to Probot — but the
old behaviour was Probot logging + retrying, which IS the bug.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
issues.openednow wraps in try/finally so the delivery is marked even whenhandleChatopsIssueor downstream octokit calls throw.issue_comment.createdauth-failure early returns (10 sites) now mark before returning. Free-text comments unchanged (still unmarked, preserving the dedup-skip path for common case).Why
Probot retries webhooks ~5 times on 5xx/timeout. The previous code skipped
markProcessedon auth-failure replies and exceptions during command bodies, producing duplicate "❌ unauthorised" comments and duplicate analysis-report issues on retry.Behaviour change matrix
Test plan
Risk
Low. Behaviour change is narrow — paths that already had visible side effects now also persist their dedup state. No paths that previously marked stop marking.
🤖 Generated with Claude Code