Repository navigation
🤖 tests: cover the CoderMCPServer disable window between the marker write and the PATCH - #367
Merged
Merged
Conversation
…rite and the PATCH Two regression tests for the opt-in disable marker that #360 writes before the disable PATCH: - The reconcile writes the marker and stops before the PATCH. While the opt-in stays withdrawn, the next reconcile still sends the enabled: false PATCH, because the decision comes from Coder's answer, not from appliedGeneration. - The PATCH fails without a commit, and the opt-in comes back while the config is still enabled. The generation counts as applied, but the drift sync still sends the spec edit made before the disable. The known limit of that window (review comment 4271304269 on #360) is documented next to sendMCPServerDisable. Fixes #365 Refs #155 Change-Id: I3f259f54aef05ffaf55024edc563504fbc473e0a 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
1 participant
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
Two regression tests for the disable marker that #360 writes before the fail-closed disable PATCH, and the documented limit of that window. Only tests and one comment change, and the kind is not registered, so users see no change.
Fixes #365
Background
#360 writes
status.appliedGenerationbefore the disable PATCH, so a lost answer cannot let a pending generation enable the config. Its round-4 review (comment 4271304269) found a window that the maintainer accepted as a known limit: the process stops after that status write and before the PATCH, and consent or the opt-in comes back before the next reconcile. #365 asks for a regression test of the stop, the limit sentence, and, from the final assessment of #363, an answer to one more case.The two cases
{"enabled":false}PATCH. Whether the config is disabled comes from Coder's answer, not fromappliedGeneration.{"description":"pending"}in the test) and reportsApplied.The comment next to
sendMCPServerDisablenow states the limit and the result of case 2. The plan's M9 row requires the how-to to repeat the limit. The how-to does not exist yet.Validation
TestCoderMCPServerOptInDisableAfterStopBeforePatch(case 1): an interceptor lets the status write succeed and then cancels the reconcile context, so the PATCH never goes out. The marker is in status, Coder is still enabled, and the next reconcile sends one{"enabled":false}PATCH.TestCoderMCPServerOptInFailedDisableKeepsPendingEdit(case 2): the disable PATCH answers 502 without a commit. The marker is in status and Coder is still enabled. After the opt-in comes back, the next reconcile sends exactly one write,{"description":"pending"}, and reportsApplied.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
96 lines added and 1 removed: 15 changed lines of comment and 82 lines of tests.
Refs #155
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$195.76