Repository navigation
🤖 feat: add the CoderAIProvider reconciler skeleton - #245
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c825229b59
ℹ️ 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".
Add a CoderAIProviderReconciler that is not registered yet. It checks the base URL the way Coder does (including bad percent escapes), reads the key through the credential Secret contract with the base URL as the destination, gets Coder access through controlPlaneCoderClient, and looks up the provider name read-only: a provider that the object did not create gives NameConflict. It adds the finalizer by merge patch, writes kstatus conditions, and watches the key Secret and the control plane. It writes nothing to Coder. 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: I83ba9f01530f9358cb8df386607345b8640e37d1
…anges Retry control plane waits every 30 s, because no watch sees an operator token Secret appear. Map a control plane event also to providers whose key Secret it names as its operator token Secret. Reject UUID-shaped names, because Coder looks them up as provider IDs. 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: I652464b72098823d40ee3fa7bfaf958eca0fdb3c
c825229 to
649f650
Compare
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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. |
Summary
This PR adds
CoderAIProviderReconciler, the first reconciler for the dormantCoderAIProviderkind. It is not registered, so the operator does not run it, and users see no change. It checks the spec, the key Secret and the control plane, and it reports a Coder provider name that something else already uses. It writes nothing to Coder. Creating providers comes in a later layer.Background
#154 adds AI providers, chat models and AI policies as controller CRDs. This is the third
CoderAIProviderlayer, after the API types (#235) and the fake Coder routes (#237). Every check here runs before any Coder write that later layers add.Reconcile order
status.providerIDand no create marker. Otherwise it keeps the finalizer with reasonDeletePendingand deletes nothing.coder.com/ai-provider-cleanupis added with a JSON merge patch and an optimistic lock, never a full update.spec.baseURLis parsed exactly as written, with no trimming, the way Coder validatesbase_url. The check also requires a host name and refuses user info, as admission does. A bad percent escape, including one in the fragment, givesCredentialMalformed. Admission already rejects fragment escapes too:isURLusesurl.ParseRequestURI, which rejects#%zz(envtest-verified). My reply on 🤖 feat(api): add the dormant CoderAIProvider API types #235 said otherwise, and it was wrong. The controller check makes the rule hold without admission.internal/credentialsecret, withspec.baseURLas the destination. Every contract failure gives its own reason (CredentialRefused,CredentialNotOptedIn,CredentialDestinationNotAllowed,CredentialMalformed,CredentialSecretNotFound,CredentialKeyMissing).controlPlaneCoderClient(🤖 refactor: share CoderControlPlane access for controllers #231). A missing or unready control plane givesControlPlaneNotReadyorOperatorAccessNotReady, and the controller retries after 30 s, because no watch sees an operator token Secret appear.GET /api/v2/ai/providers/{name}:Pending, and this version creates nothing.NameConflict. Coder providers carry no owner, so the controller never changes it (no adoption).CoderUnavailablewith the status code and Coder's short message only, neverDetailor validations.CoderAnswerMismatch.Steps 3 to 5 send nothing to Coder. Every condition message is also scrubbed of the key value. Conditions follow kstatus (
Ready,Reconciling,Stalled). Credential, URL and name failures areStalled. Control plane and Coder waits areReconciling. Status is written only when it changes.SetupWithManagerindexes providers byspec.controlPlaneRef.nameandspec.apiKeySecretRef.name. It watches control planes, and maps each event to the providers that use it and to the providers whose key Secret it names as its operator token Secret (the contract refuses those). It watches Secrets withcredentialsecret.WatchPredicate(old-or-new label, every delete). No RBAC marker, no registration incontrollerapp, and no CRD move:config/rbac/role.yamlanddist/install.yamldo not change.Validation
internal/controller/coderaiprovider_controller_test.go(envtest plus the fake Coder from #237):OpaqueSecret and a Secret owned by acoder.comobject. Destination refusals cover another port, scheme, subdomain and a trailing dot. Malformed values are empty, oversized or whitespace-padded. Every case scans status, annotations and captured logs for the key.ControlPlaneNotReady. A ready control plane without its operator token Secret givesOperatorAccessNotReady. Both retry after 30 s with zero Coder requests, and the provider recovers once the Secret exists.NameConflictwith one GET and no write. A 500 whoseDetailholds the key givesCoderUnavailable, and the key appears nowhere. A rewritten answer givesCoderAnswerMismatch. A second reconcile in each case sends one GET, no write, and leaves the stored status unchanged.enabled: falsekept, released on delete when nothing reached Coder, and kept withDeletePendingwhenstatus.providerIDis set.coderaiprovider_internal_test.gochecks the URL rule, including#%zz,#a%2, a trailing space, user info andhttp://:8080.Mutation check: I broke the reconciler in 14 ways (no retry on a control plane wait, control plane events mapped only to its users, the UUID name rule off, credential errors ignored, Coder lookup before the credential check, full error text in status, an existing provider adopted, Secret watch mapping nothing, no control plane watch, finalizer released with a Coder ID, trimmed URL, user info allowed, answer not checked). Each change made a test fail. One more mutation, an unconditional status write, is not observable, because the API server does not change the resourceVersion for a no-op update.
Local gates:
make verify-vendor,make test,make test-integration,make build,make lint,GOFLAGS=-mod=vendor go tool govulncheck ./..., andgo test -race ./internal/controller/. Running the generators again leaves no diff.Known gap
The design plan's S1 also refuses a control plane's license Secret and database Secret.
internal/credentialsecretonmainrefuses only operator-generated Secrets. Today a license or database Secret is usable only if its writer labels it and lists the provider origin, which is explicit consent. Adding that refusal changes the shared contract forCoderWorkspaceProxyandCoderProvisionertoo, so it belongs in its own PR, not here.Review round 1
The automatic Codex review on
c825229bleft three P2 findings. All three are fixed in649f6508:The branch is rebased onto
mainatf9198ca1c. That brings in #238, #239 and #241. #239 already fixed the dormancy guard's grep exit-code handling.Size
298 production lines added and 1 removed (reconciler and API type). 406 test lines added and 0 removed. Generated CRD: 6 added and 1 removed. This is above the 500-line target. The S1, S2, S10 and S12 tests belong with the code they test, so I kept them together.
Refs #154
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$13.59