[MM-69266] Fix false "No webhook was found" warning when only an org-…

[nang2049]

Summary

When a channel is subscribed to a specific repository via /github subscribe owner/repo and webhook delivery is handled by an org level webhook the plugin incorrectly posts:

No webhook was found for this repository or organization. To create one, enter the following slash command /github setup webhook
and this message is a false negative.
This change makes the verification consistent with the rest of the plugin. No change to webhook delivery, subscription creation or command parsing

Ticket Link

https://mattermost.atlassian.net/browse/MM-69266

QA

• In GitHub: <org> - Settings - Webhooks add a webhook with Payload URL https://<mattermost>/plugins/github/webhook.
• Make sure the target repo has no repo level webhook with that URL.
• In Mattermost: /github subscribe <org>/<repo>.
• Expected: subscription confirmation no "No webhook was found" note.

Change Impact: 🟡 Medium

Reasoning: This PR modifies webhook verification logic in a critical user-facing flow (subscription creation), changing the behavior for organization-level webhook detection and error handling. While the changes are isolated to the command.go file and fix a legitimate false-negative issue, the lack of dedicated test coverage for the modified functions and the introduction of special error handling logic (treating 403/404 API errors as success) creates moderate regression risk.

Regression Risk: The changes affect checkIfConfiguredWebhookExists, which is called during repository subscriptions in handleSubscribesAdd and repository unsubscriptions. The refactored logic introduces a fallback pattern where if repository-level webhook listing fails with 403/404 errors, the function returns success rather than failure, and then attempts organization-level webhook checking. This behavioral change could mask genuine permission issues and result in users seeing subscription success messages when they should be warned about missing webhooks. The addition of isWebhookListAccessError centralizes error detection, but the interpretation of these errors as a sign that webhooks "effectively exist" may not account for all failure scenarios. No specific test cases for these functions were found in command_test.go despite 24 existing test functions in that file.

QA Recommendation: Comprehensive manual QA is recommended before production release. Test scenarios should include: (1) repositories with both repository-level and organization-level webhooks configured; (2) repositories with only organization-level webhooks; (3) users with insufficient permissions to list repository webhooks (403 errors); (4) repositories where webhook access is completely denied (404 errors); (5) edge cases where organization webhook listing fails after successful repository webhook check. At minimum, verify that the warning message appears/disappears correctly in all webhook configuration scenarios described in the QA steps. Automated test coverage should be added for checkIfConfiguredWebhookExists and anyHookMatchesSiteURL functions before merging, particularly for the new fallback and error handling logic.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: a621e46f-a77e-40f8-8818-88f226617147

📥 Commits

Reviewing files that changed from the base of the PR and between 6b4dab0 and 8a96b1b.

📒 Files selected for processing (1)

• server/plugin/command.go

🚧 Files skipped from review as they are similar to previous changes (1)

• server/plugin/command.go

---

📝 Walkthrough

Walkthrough

checkIfConfiguredWebhookExists is refactored to compute siteURL once, check repo hooks first (treating 403/404 listing errors as implicit success), and fall back to org hooks. anyHookMatchesSiteURL is rewritten to accept a hook-listing callback and paginate results. A new isWebhookListAccessError helper centralizes 403/404 error classification.

Changes

Webhook Existence Check Refactor

Layer / File(s)	Summary
checkIfConfiguredWebhookExists orchestration and fallback server/plugin/command.go	Computes siteURL up-front; for repo-scoped subscriptions, lists repo hooks first and returns success when a 403/404 access error is detected; otherwise falls back to org-scoped hook listing.
anyHookMatchesSiteURL callback redesign and isWebhookListAccessError server/plugin/command.go	anyHookMatchesSiteURL now accepts a list callback, paginates via ListOptions/NextPage, logs and returns errors on listing failure, and returns true on any URL match. isWebhookListAccessError classifies errors by checking for "404 Not Found" or "403 Forbidden" in the message.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 A webhook once lost in the 403 mist,
Now gracefully handled — it won't be dismissed.
The rabbit refactored with callbacks in tow,
Paginating through hooks, watching siteURLs flow.
On 404, we shrug and say "present enough!" 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The PR title is truncated and incomplete ('Fix false "No webhook was found" warning when only an org-…'), making it unclear what the complete change is about.	Complete the title to fully describe the fix, e.g., 'Fix false webhook warning when repository uses organization-level webhooks' or similar.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch MM-69266-org-webhook-check

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@server/plugin/command.go`:
- Around line 455-458: The error handling block checks for cErr but then uses
listErr in both the log message and return statement, which could be nil and
cause a panic. In the condition where cErr is non-nil, replace all references to
listErr with cErr in the processLogger.Warn call and the return statement to log
and return the actual error from the useGitHubClient operation.
- Around line 466-468: The pagination logic at line 466-468 checks resp.NextPage
without first verifying that resp itself is not nil, which can cause a nil
pointer dereference when the GitHub API returns a nil response in edge cases.
Update the condition to include a nil check for resp before accessing its
NextPage property, following the pattern already established in sla_digest.go
where both conditions are checked together: first verify resp is not nil, then
check if resp.NextPage equals zero.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 8c7c4ebc-4d4e-4db0-9a05-566a3b613784

📥 Commits

Reviewing files that changed from the base of the PR and between 5cd47fb and 6b4dab0.

📒 Files selected for processing (1)

• server/plugin/command.go