Repository navigation
🤖 fix: trust only Coder's own 404 before a CoderMCPServer config counts as gone - #356
Merged
Merged
Conversation
…ts as gone The sync read and the disable read treated any 404 as a deleted config, including a proxy HTML page and Coder's "Route not found." for a wrong URL. After a spec edit that cleared status.configID, so the create path then reported SlugConflict and the config became unmanaged. The disable read skipped the fail-closed PATCH. Both reads now use isCoderResourceNotFound, and any other 404 waits and changes nothing. Fixes #337 Refs #155 Change-Id: I734fed2d39abb89dbbb95d055b489f9a65d6b803 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Member
Author
|
@codex security review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
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
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
The dormant
CoderMCPServerreconciler now treats only Coder's own 404 as proof that a config is gone. A proxy HTML 404 or Coder'sRoute not found.(a wrong base URL) waits and changes nothing. The kind is not registered, so users see no change.Fixes #337
Background
#328 to #330 read the config by ID in two places and treated any 404 as "gone":
syncMCPServer,codermcpserver_apply.go). After a spec edit, a 404 clearedstatus.configID, and the create path then found the slug with a non-nonce display name. It reportedSlugConflictand cleared the marker, so the config became unmanaged in Coder, possibly enabled and holding credentials. After a full apply, the same 404 gave theDeletedExternallykill switch, which also stops all writes.codermcpserver_disable.go, review thread 4269665474 on 🤖 feat: disable the CoderMCPServer config when a credential Secret withdraws consent #330). A false 404 skipped the fail-closed PATCH, so the config stayed enabled while its Secret had withdrawn consent.This is the MCP version of #327 for
CoderAIProvider. Plan gate G12, which must land before activation (M9).Implementation
All three cases use
isCoderResourceNotFoundfrom #325: a 404 whose message is Coder's fixed resource message.DeletedExternally, no writeCoderUnavailablewait, 30 sCoderUnavailablewait, 30 s, ID keptThe disable read keeps its credential reason for a failed read, as
CoderAIProviderdoes in its disable path. The organization lookup (resolveMCPServerOrganization) still treats any 404 as "organization not found". That is a wait with no state change, so it is out of scope.Validation
New tests in
internal/controller/codermcpserver_notfound_test.go, each for an HTML 404 page and forRoute not found.:TestCoderMCPServerSyncNotCoder404: for an applied generation and after a spec edit, the reconcile waits withCoderUnavailableand a 30 s requeue, keepsstatus.configIDand the change tokens, and sends no write. The next reconcile applies, with no second create.TestCoderMCPServerDisableNotCoder404: after the Secret is deleted, the disable read waits, keeps the ID, sends no write, and Coder stays enabled. The next reconcile disables.TestCoderMCPServerDisableCoder404: Coder's own 404 in the disable read still stalls with "does not exist in Coder" and no write. The sync read with Coder's own 404 is covered by the existingTestCoderMCPServerSyncDeletedConfig.isCoderNotFoundin the applied sync case, in the unapplied sync case, or in the disable read, and a disable read that never counts a 404 as gone.make verify-vendor,make build,make lint,GOFLAGS=-mod=vendor go tool govulncheck ./...,make test,make test-integration,go test -race ./internal/controller/, andmake docs-checkpass.make codegen,make manifests,make build-installerandmake docs-referenceleave no diff.Size
104 lines added and 3 removed, all hand-written: 6 changed lines in
codermcpserver_apply.go, 3 incodermcpserver_disable.go, and 98 in the new tests.Refs #155
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$165.80