[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 changes the logic that determines whether to show a user-facing “No webhook was found” warning during /github subscribe, specifically treating certain GitHub webhook-list access failures (403/404) during org-hook checks as “configured” so organization-level webhooks are recognized. While it does not alter webhook delivery, it does change the plugin’s behavioral contract for warning/UX decisions in permission-related scenarios.

Regression Risk: Moderate—changes are localized to command.go, but the new fallback/error-handling behavior can affect multiple subscription outcomes (org-owned vs user-owned repos) and the decision to suppress warnings when webhook listing is blocked; while unit tests cover the main org-owned/403 scenario, they don’t exhaustively cover all permission/error variants (e.g., 404 access error vs other errors, pagination edge cases).

QA Recommendation: Perform targeted manual QA focused on the subscription warning behavior: (1) repo-level webhook configured only, (2) org-level webhook configured only, (3) org-owned repo where listing org webhooks returns 403/404, (4) confirm no false warning appears in the described reproduction flow. Automated tests added for the core logic; manual QA should be limited to these scenarios due to moderate risk.
Generated by CodeRabbitAI

[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: 5bf61226-da7f-4fac-ade3-728b21087546

📥 Commits

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

📒 Files selected for processing (2)

• server/plugin/command.go
• server/plugin/command_test.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), verify organization ownership, and fall back to org hooks. anyHookMatchesSiteURL now accepts a hook-listing callback and paginates results. A new isWebhookListAccessError helper centralizes 403/404 error classification. Comprehensive tests validate the complete flow.

Changes

Webhook Existence Check Refactor

Layer / File(s)	Summary
checkIfConfiguredWebhookExists refactor and organization ownership check server/plugin/command.go	Precomputes siteURL once. 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 listing only after verifying the owner is an organization.
anyHookMatchesSiteURL callback redesign and isWebhookListAccessError server/plugin/command.go	anyHookMatchesSiteURL now accepts a list callback, paginates via PerPageValue/NextPage, logs and returns errors on listing failure, and returns true on any URL substring match. isWebhookListAccessError classifies errors by checking for "404 Not Found" or "403 Forbidden" in the message.
Webhook check test suite server/plugin/command_test.go	Introduces TestCheckIfConfiguredWebhookExists with mocked Site URL and httptest server, validating user-repo fallback skip, repo-level webhook matching, org-owned repo fallback success, and 403 access errors treated as configured.

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
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly references the specific issue being fixed (false 'No webhook was found' warning with org-level webhooks) and aligns with the main code changes that refactor webhook verification logic.
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

[avasconcelos114]

Changes are looking good! I just have one possible edge-case that might need to be confirmed

[avasconcelos114]

We may have to skip this org fallback if the repository's owner is a user instead of an org, otherwise we may get a false-positive where the 404 response from the listOrgHooks request cascades into isWebhookListAccessError being true and returning true instead of leaving it to the final return found, nil which was going to be correctly marked as false since the user hadn't set up webhooks for their personal repo

[nang2049]

@avasconcelos114 added

[avasconcelos114]

LGTM!