Repository navigation
Conversation
Change-Id: I6a1b09dd1e5811eaa57977b70199a075cc4b2b35 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…config Change-Id: I51f32054c2be80a04ccb71cd66155c257d695749 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (2)
ℹ️ 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 review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d83d2f30b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🛡️ 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. |
…heck every answered field Change-Id: I33c42b0d71af811d67f331d775c0a0ecb171b35d Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dc8a590f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🛡️ 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. |
…eared the old credential Change-Id: Ia71b65e0fb3a49afd17495c7d8bd91edf329a119 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db5b3e0d9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🛡️ Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: db5b3e0d9e
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
Change-Id: Ia2a200bdcfb547524c4b82def948c23d45c311d6 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ 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. |
1 participant
Summary
The dormant
CoderMCPServerreconciler now applies the spec to the config that it created. It removes the access list entries that the spec does not list, enables the config with every spec value, then adds the missing groups, and reportsReady. The kind is not registered, so users see no change.Base branch:
mcp-create(#304). Review only the last commit. After #304 lands, I retarget this PR tomain.Background
#155 adds
CoderMCPServer. #304 creates the config disabled, with the nonce as its display name, and records its ID. This PR is plan step 4.5 create steps 5 and 6. It makes the ACL helper from #294 live, and adds a remove-only pass to it.How an apply works
An object with
status.configIDruns these steps after the checks of #274, unless its generation is applied and every credential's change token equals the one instatus.credentials. A Secret change moves the token but not the generation, so a rotated credential is sent again. The in-memory hash that skips the resend after a metadata-only Secret change comes with M6.DeletedExternallywith no write. Creating it again after a spec change comes with the update path (M6). An answer for another ID, organization or slug givesCoderAnswerMismatch.spec.accessdoes not list, and add nothing yet (narrowMCPServerACL, a remove-only pass of the 🤖 feat: add the CoderMCPServer ACL helper #294 helper). The helper resolves every name first, soGroupNotFoundstops the reconcile before any write, and a new config stays disabled with the nonce.enabled(default true), the three flags, and the fields and credentials of the auth type.url, so it always carries every credential of the auth type (plan 4.6 item 5). Each credential was read in this reconcile for every origin that receives it. It is revealed only into this request body.CoderRejected. Other errors wait 30 s, because the PATCH sets absolute values and sending it again is safe. That includes a 404: a proxy can put one in place of a committed answer, so the GET of the next reconcile decides whether the config is gone.api_key_header, the OAuth2 fields,enabled, the three flags, and thehas_*flag of each credential sent. When the PATCH changes the auth type, Coder must also report the other types' credentials as cleared (plan F9). Anything else givesCoderAnswerMismatch.spec.accesslists and the ACL lacks (the full 🤖 feat: add the CoderMCPServer ACL helper #294 helper).status.appliedGeneration, and reportReadywith reasonApplied. Later reconciles of the same generation send no MCP request. Reading Coder for drift, a sparse PATCH, and the kill switches come with M6.Why this order: while the PATCH changes or enables the config, its ACL holds only groups that both the old and the new access list allow. A new config is therefore never enabled with a group that the spec does not list (plan S20). A PATCH that fails leaves the ACL narrower than both lists, never broader. A group added in a new generation never gets the old configuration, for example when the same generation disables the server.
The ID is written to status before step 1. The PATCH replaces the nonce, so after it only the stored ID proves that this object created the config. If the status write after the PATCH is lost, the next reconcile applies again instead of reporting its own config as a
SlugConflict.Access list doc (follow-up from #294)
The
spec.accessfield doc now says that every sync removes each user entry, for example a user that an admin added in the Coder dashboard, becauseaccesslists groups only. It also says that Coder does not show the entry of a deleted group, so the sync cannot remove it, but it grants nothing. Coder deletes groups for real (coderd/database/queries/groups.sql:190-194). Deleted users stay in the user table (users.sql:422-428) and still appear in the ACL, so the sync removes their entries. The dormant CRD carries the new text. The kind has no user docs yet. The plan's M9 row now requires the same statement indocs/how-to/manage-mcp-servers.md.Validation
internal/controller/codermcpserver_apply_test.go(envtest and the fake Coder, with the leak scan on every reconcile):TestCoderMCPServerApplyOrder(plan S20): withaccess.groups: [data], the writes are the create, an ACL PATCH that only removes Everyone, the enabling PATCH, and an ACL PATCH that only addsdata. Coder holds the config enabled with the spec display name and an ACL of exactlydata, and the PATCH carries the API key. The next reconcile sends nothing.TestCoderMCPServerApplyNeverBroadensBeforeTheChange: an applied config readable by Everyone anddatagets a new generation withdataandops, plusenabled: false(rejected with a 400) or a URL change (failing with a 502). After the failure the ACL holds onlydata: Everyone is gone andopsis not added. The next reconcile applies.TestCoderMCPServerApplyAccess(plan S15):access: {groups: []}withenabled: falsegives an empty ACL on a disabled config. Withoutaccess, no ACL route is called, and Coder's Everyone entry stays.TestCoderMCPServerApplyGroupNotFound: the config stays disabled with the nonce, with no PATCH. Once the group exists, the next reconcile applies.TestCoderMCPServerApplyFailures: a 404 or 503 on the read, an answer for another slug, a 500 or 400 on the PATCH whose detail holds the key, a 404 on the PATCH (a wait, then applied), and PATCH answers with anotherenabled, description, availability, flag, API key header, or no key. Each leaves the generation unapplied. The retryable ones apply on the next reconcile.TestCoderMCPServerApplySpecChange: a URL and description change sends one PATCH with the new values and the key.TestCoderMCPServerApplySecretRotation: a new Secret value with no spec change gives one PATCH with the new value and the new change token. The next reconcile sends nothing.TestCoderMCPServerApplyAuthTypeChange:api_keytononeapplies when Coder clears the key, and stalls withCoderAnswerMismatchwhen the answer still hashas_api_key: true.TestCoderMCPServerApplyLostStatusWrite: the status write after the PATCH fails. The next reconcile applies again with no second create.Appliedwhere they expectedPending.enabled: falseignoredReadyfor any applied generation instead of the current oneenabled, or ofhas_api_keyReadywithout comparing the change tokenshas_*flag after an auth type changemake verify-vendor,make test,make test-integration,make build,make lint,make docs-check,GOFLAGS=-mod=vendor go tool govulncheck ./...andgo test -race ./internal/controller/.make codegen,make manifests,make build-installerandmake docs-referenceleave no diff.Size
646 lines added and 44 removed, 631 of them hand-written: 181 in
codermcpserver_apply.go, 23 changed lines in the #294 ACL helper (the remove-only pass), 21 in the reconciler, 382 in the apply tests, 65 changed lines in the existing tests, and 9 lines of field doc. The regenerated dormant CRD has 9 changed lines. The two Codex review rounds added 233 of these lines. Before them this PR was 413 lines. It stays one PR because the review fixes change the apply order and the Ready rule that every apply test checks.Review record and deferred findings
Codex reviewed three heads. Two fix rounds (the cap) fixed five findings. The third review (normal and security, on
db5b3e0) found three more, each deferred with an issue, after an independent advisor recommended "ready with tracked follow-ups":has_*flags on every apply, not only on the auth type transition (M6).The kind is not registered, so none of them can affect an install before M9.
Refs #155
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$83.70