Assume 404 is not healthy #72
Conversation
🦋 Changeset detectedLatest commit: 1144e55 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
This PR modifies the health check logic to treat HTTP 404 responses as unhealthy/offline rather than healthy. Previously, any response with a status code below 500 (including 404) was considered "online". The change recognizes that a 404 response indicates the MCP endpoint doesn't exist, which should be classified as offline.
- Updated health check condition to exclude 404 from healthy status codes
- Added changeset documenting the behavioral change
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/core/src/health.ts | Modified status check to treat 404 responses as offline |
| .changeset/treat-404-as-offline.md | Added changeset entry documenting the health check behavior change |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| if (response.status < 500) { | ||
| // 2xx, 3xx, and 4xx (except 404) mean server is responding | ||
| // 404, 5xx, or network errors mean "offline" | ||
| if (response.status < 500 && response.status !== 404) { |
There was a problem hiding this comment.
Bug: Test expects 404 to be healthy but code now treats it as offline
The health check now treats HTTP 404 as offline, but an existing test in packages/core/src/health.test.ts (the "should handle server returning 404 as up" test) explicitly expects 404 responses to result in health: "up". The test comment states "404 means server is responding, so it's 'up'" which contradicts the new behavior. The test will fail because checkServerHealth now returns status: "offline" for 404, which maps to "down" in the gateway, but the test asserts expect(results[0]?.health).toBe("up").
given that our test suite was not run on CI we had some failing tests
Bun's mock.module() is global and persists across test files, causing flaky tests when execution order varies. Remove mockFilterAutocomplete and mockFilterBadge from tests that don't need them. - command-filter-input.test.tsx: only keep mockUseAvailableFilters (API hooks) - filter-bar.test.tsx: remove mockFilterBadge, use aria-label queries - filter-autocomplete.test.tsx: revert to normal import Verified fix by running tests 10x with --randomize, all passing.
Remove mockFilterBadge, mockFilterAutocomplete, mockCommandFilterInput, and mockNuqs - these were causing global mock pollution and are no longer used after the previous fix.
Right now the health check assumes any status code < 500 is ok but a 404 shouldn't be considered ok/healthy
Note
Treat HTTP 404 responses as offline in health checks instead of healthy.
packages/core/src/health.ts):checkServerHealthto consider404as offline; only non-404<500statuses are online.HTTP_ERRORwithHTTP {status}: {statusText}for offline responses.@fiberplane/mcp-gatewaydocumenting the 404-as-offline behavior.Written by Cursor Bugbot for commit 7768adc. This will update automatically on new commits. Configure here.