E2E: Mattermost breadcrumbs for main-push + stop silent webhook drops - #96
Closed
yasserfaraazkhan wants to merge 2 commits into
Closed
E2E: Mattermost breadcrumbs for main-push + stop silent webhook drops#96yasserfaraazkhan wants to merge 2 commits into
yasserfaraazkhan wants to merge 2 commits into
Conversation
|
@yasserfaraazkhan: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughWebhook ingestion no longer uses GitHub API rate limits to reject deliveries. It now limits request bodies to 10 MiB and handles read errors. Push E2E handling logs commit SHAs and sends Mattermost updates for skips, provisioning, and workflow dispatch. Webhook requests now use a timeout and close response bodies. ChangesWebhook ingestion and E2E reporting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
server/e2e_watchdog_test.go (1)
91-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse context-aware test requests.
The
httptest.NewRequestcalls lack context. Passcontext.Background()to provide context to the request, following Go testing conventions. This is a best practice for consistency with context-aware HTTP operations.Proposed fix
- req := httptest.NewRequest(http.MethodPost, "/github_event", bytes.NewReader(pingBody)) + req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/github_event", bytes.NewReader(pingBody))- req := httptest.NewRequest(http.MethodPost, "/github_event", bytes.NewReader([]byte(`{}`))) + req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/github_event", bytes.NewReader([]byte(`{}`)))🤖 Prompt for 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. In `@server/e2e_watchdog_test.go` at line 91, Update the httptest.NewRequest call in the watchdog test to use the context-aware request constructor, passing context.Background() while preserving the existing POST method, endpoint, and pingBody.Source: Linters/SAST tools
🤖 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/e2e_watchdog.go`:
- Around line 150-156: Remove the unconditional len(instances) fallback in the
watchdog logic so an active E2E run only suppresses the alert when
strings.HasSuffix(key, suffix) matches the current branch SHA. Add a test
covering an active earlier SHA alongside an uncovered current SHA, asserting
that the watchdog still alerts.
- Around line 202-207: Update the commit-time resolution near commitTime so it
no longer defaults to time.Now when both committer and author dates are absent.
Return or propagate an error from the surrounding reconciliation flow when
neither date is available, while preserving the committer-date preference and
author-date fallback when present.
In `@server/provision_errors.go`:
- Around line 31-39: The isCancellationError function incorrectly relies on
substring matching for "cancelled" and "canceled" to classify errors, which
causes wrapped provider errors containing those strings to be misclassified as
cancellation errors. Remove the substring matching logic that uses
strings.ToLower and strings.Contains in the isCancellationError function,
keeping only the identity-based check using errors.Is(err, context.Canceled).
Also add a regression test that verifies a provider error wrapped with format
string error wrapping containing the word "cancelled" is not misclassified as a
cancellation error.
In `@server/push_events.go`:
- Line 46: The worker goroutines created by
createMultipleE2EInstancesForPushEvent lack panic recovery, allowing panics to
leave results[idx] as a false success. Add deferred recovery in each worker that
records the recovered panic as results[idx].err and invokes cancel(), while
preserving wg.Done() execution.
In `@server/server.go`:
- Around line 203-208: Update the POST /github_event handler around io.ReadAll
to wrap r.Body with http.MaxBytesReader before reading, using the existing
request context and an appropriate body-size limit. Handle *http.MaxBytesError
with http.StatusRequestEntityTooLarge, while retaining http.StatusBadRequest for
all other read errors and preserving the current error logging and return flow.
---
Nitpick comments:
In `@server/e2e_watchdog_test.go`:
- Line 91: Update the httptest.NewRequest call in the watchdog test to use the
context-aware request constructor, passing context.Background() while preserving
the existing POST method, endpoint, and pingBody.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7e789cd-047d-41e9-ac88-7853ab3d32bc
📒 Files selected for processing (9)
server/e2e_tests.goserver/e2e_watchdog.goserver/e2e_watchdog_test.goserver/limit_rate_gh.goserver/provision_errors.goserver/pull_request.goserver/push_events.goserver/server.goserver/workflow_run.go
mm-cloud-bot
added
release-note-none
Stop silently dropping GitHub webhooks when API rate reserve is low, and post Mattermost breadcrumbs for main/master push E2E (received → provisioned → dispatched, or why it stopped). Co-authored-by: Cursor <cursoragent@cursor.com>
yasserfaraazkhan
force-pushed
the
e2e/webhook-ingest-and-main-watchdog
branch
from
August 4, 2026 16:24
b269fba to
89bdb3f
Compare
yasserfaraazkhan
changed the title
|
@yasserfaraazkhan: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
mm-cloud-bot
added
do-not-merge/release-note-label-needed
and removed
release-note-none
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/server.go`:
- Around line 180-182: Limit the request body in the GitHub webhook handler
before its io.ReadAll call by wrapping r.Body with http.MaxBytesReader. Handle
read failures by returning HTTP 413 for *http.MaxBytesError and HTTP 400 for all
other errors, while preserving the existing signature-validation flow.
- Around line 180-182: Add the repository-compliant release-note block for the
webhook ingestion and E2E status reporting change, placing it with the project’s
existing release-note metadata before merge. Keep the note focused on preventing
webhook deliveries from being dropped by GitHub API rate-reserve gating and
preserving E2E status reporting.
In `@server/utils.go`:
- Around line 16-17: Update sendToWebhook to use a finite request timeout so
slow Mattermost calls cannot block handlePushEventE2E during any lifecycle
stage. Apply the timeout to the HTTP client or request used by notifyMattermost,
preserving the existing webhook behavior while ensuring the call fails promptly.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9f578fba-3ddc-4567-80d4-30020d3445ef
📒 Files selected for processing (4)
server/limit_rate_gh.goserver/push_events.goserver/server.goserver/utils.go
💤 Files with no reviewable changes (1)
- server/limit_rate_gh.go
Cap GitHub webhook body reads with MaxBytesReader and give Mattermost webhook posts a finite HTTP timeout so notifications cannot hang E2E. Co-authored-by: Cursor <cursoragent@cursor.com>
mm-cloud-bot
added
release-note-none
Contributor
Author
|
closing for now |
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
2 participants
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
Minimal observability for “what happens in Matterwick after a mobile/desktop main merge?”
GitHubTokenReserve. That abort returned without processing the push, so main E2E never started and nothing was logged/alerted.MaxBytesReader) and Mattermost webhook HTTP timeout (CodeRabbit).No watchdog, self-heal, goSafe, quorum changes, or product-repo contract changes.
Test plan
mattermost-mobilemainpush and confirm Mattermost shows the receive → provision → dispatch sequence