Repository navigation
🤖 feat: create the Coder provider for a CoderAIProvider - #253
Conversation
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. |
![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: b2778fba3e
ℹ️ 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".
b2778fb to
9cff0b8
Compare
|
@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: 9cff0b8897
ℹ️ 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. |
The CoderAIProvider reconciler (still not registered) now creates the Coder provider when the name is free: it writes the create marker and the key's change token first, then POSTs the provider disabled with the nonce display name coder-k8s:<UID>. An uncertain answer keeps the marker, and the next reconcile reads Coder before it sends anything again. The nonce and the type prove ownership. Then one PATCH per generation sets the display name, icon and enabled flag. The status message for a bad baseURL no longer repeats the URL. Refs #154 Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_ Change-Id: I504b0bb461fb75ecba41154e9d34e01e642af890
Store the provider ID before the apply PATCH replaces the nonce display name, so a crash or a failed status write after the PATCH cannot turn the provider into a NameConflict. Check the applied display name, icon and enabled flag in the update answer before reporting Ready. Treat a create answer without the expected key as uncertain, so a read confirms it. Refs #154 Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_ Change-Id: Ie28921f3eaf3a24af2a6d7b4cf47211b0652758f
A rotated key Secret does not change the generation, so check the change token before the ready shortcut. Retry a 401 or 403 on the apply PATCH, and treat a 403 on the create as a create without effect, because no Kubernetes event follows a repaired operator token or owner role. Refs #154 Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_ Change-Id: I0eedef2da992751b7089adbc82b42b90572f97f4
9cff0b8 to
877bfed
Compare
|
@codex review |
|
@codex security review |
🛡️ 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 877bfedef1
ℹ️ 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".
1 participant
Summary
The dormant
CoderAIProviderreconciler now creates the Coder provider and applies the spec to it. It is still not registered, so the operator does not run it, and users see no change. The create follows the crash-safe rules thatCoderTemplateTestuses: a marker before every create, a nonce that proves ownership, and a read of Coder before any second create.Background
#154 adds AI providers, chat models and AI policies as controller CRDs. This is the fourth
CoderAIProviderlayer, after the API types (#235), the fake Coder routes (#237) and the reconciler skeleton (#245). Drift handling, key resends and kill switches come in the next layer. Deletion comes after that.Create
When the object has no Coder ID and no create marker, the reconciler looks up the name:
NameConflict, and the controller never changes that provider (no adoption).status.createAttemptTime(the marker) and the key's change token (status.credential: Secret name, key, UID, resourceVersion) in one status update. A conflict on that update ends the reconcile before any request.POST /api/v2/ai/providerswithenabled: false,display_name: "coder-k8s:<object UID>"(the nonce), the type, the base URL and the key.The answer decides the next step:
status.providerIDand the Coder key ID, clear the marker, then applyNameConflictCoderRejected. No retry until the spec changesConfirmingCreate. The marker staysWith a marker set, the next reconcile reads the name before it sends anything:
KeyChangedExternally.NameConflict, and the marker is cleared.Apply
Once per generation, one
PATCHsetsdisplay_name,iconandenabledto the spec values. The provider ID is already stored, so ownership does not depend on the nonce once the PATCH replaces it. The answer must show the requested display name (or the provider name for an empty one), icon and enabled flag. Then the reconcile setsstatus.appliedGenerationandReady=Truewith reasonApplied. A ready object needs no Coder call.These cases do not count as applied:
Pending, also when the provider wasReadybefore, because a Secret change does not change the generation. The next layer sends changed keys.Pending. The next layer sends changed URLs.DeletedExternally, and the controller creates nothing new.CoderUnavailable, retried after 30 s, because no Kubernetes event follows a repaired operator token or owner role.Other changes
spec.baseURLthat does not parse is now fixed text. The old message embedded the parse error, which repeats the URL, user info included.Validation
internal/controller/coderaiprovider_create_test.go(envtest plus the fake Coder from #237). Every test uses a new reconciler for each reconcile, as a restart would.Ready.NameConflict, zero PATCH and DELETE requests, and no second create.Ready), a changed base URL, an update answer without the requested enabled flag, a provider deleted in Coder before the apply, and a 401 and a 403 on the apply, which recover on the next reconcile.Mutation check: I broke the create flow in 18 ways (ready shortcut before the key check, 401 or 403 on the apply stalled, 403 on the create rejected, ID not stored before the apply, applied fields not checked, a create answer without the key treated as a dashboard change, uncertain answer clears the marker, no nonce check, provider created enabled, no site-owner check, rejected create retried, 429 keeps the marker, wrong answer recorded, key change ignored, applied without the URL check, 404 on apply retried, PATCH on every reconcile, 409 recorded as ours). Each change made a test fail.
Local gates on the pushed tree:
make verify-vendor,make test,make test-integration,make build,make lint,GOFLAGS=-mod=vendor go tool govulncheck ./...,go test -race ./internal/controller/, and regeneration with no diff.Review round 1
The automatic Codex review on
b2778fbaleft three findings. All are fixed ind514eb4e(rebased from9cff0b88):Review round 2
The requested Codex review on
9cff0b88left two P2 findings. Both are fixed in877bfede:Readytest.CoderUnavailablewith a 30 s retry. A 403 on the create is a create without effect. Tests cover both.The security review on
9cff0b88found no issues. The branch is rebased ontomainat116cefe8.Review round 3
The requested Codex reviews on
877bfede: security found no issues. The normal review left two valid P2 findings. Under the review-round cap (from round 4, blockers only), I deferred both to A5, with the independent advisor's agreement. Neither is a blocker: the controller is not registered, nothing leaks, and no data is lost.spec.baseURLchanges. The old URL already passed the destination check of the current Secret.Both are in the #154 plan's A5 row, and the A7 activation gate requires them.
Tracked for later layers
Size
256 production lines added and 36 removed. 423 test lines added and 4 removed. No generated files change. The plan's A4 row targets about 510 lines. Most of the extra lines are the outcome-class tests.
Refs #154
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$23.36