Repository navigation
🤖 feat: sync CoderAIProvider drift, key rotation and kill switches - #265
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: ee660340d7
ℹ️ 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 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: 2c8585b811
ℹ️ 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 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: ef5e6bc715
ℹ️ 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 reads the Coder provider on every reconcile and sends one sparse PATCH with every field that differs from the spec. The base URL and the key travel in the same PATCH, so Coder never enables a provider at a stale URL. A changed key Secret value is resent behind a resend marker, and an in-memory SHA-256 skips resends for metadata-only Secret changes. After a full apply, a dashboard delete, disable or key change stalls the object until the spec changes. A 4xx create answer is now confirmed by a read before the create marker is cleared, and a nonce match without a recorded credential is recorded instead of failing an assertion. 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: Id83a2bc2b262042bfce91b4e712d60f1ec7094df
… key IDs A provider deleted between the create and the first sync no longer dereferences a nil step: the next reconcile looks up the name again. A key update answer with a zero key ID is a mismatch, and the resend marker stays. A dashboard disable during a pending key resend is still a kill switch, so a Secret rotation cannot undo 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: I3c31c5d8dcada80daf2f2b4a4dcbded1d576e0b9
…s stalled A 404 to a sync PATCH is confirmed by a read before the controller drops ownership, because a proxy can put a 404 in place of a committed PATCH that already replaced the nonce. A rejected create whose confirmation read fails is remembered, so a later read that confirms absence stalls instead of sending the create again. A create answer without a provider ID is an assertion failure again. 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: I20e41722d73b684b02a240971521de86ecee71ed
ef5e6bc to
6a7811e
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: 6a7811ea0d
ℹ️ 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 keeps Coder in line with the spec after the create. Each reconcile reads the provider and sends one sparse PATCH with every field that differs. The base URL and the key always travel in the same PATCH. A changed key is resent safely, and a dashboard delete, disable or key change stalls the object until the spec changes (kill switches). The controller is still not registered, so users see no change.This PR also fixes the two findings deferred from #253.
Background
#154 adds AI providers, chat models and AI policies as controller CRDs. This is the fifth
CoderAIProviderlayer, after the API types (#235), the fake Coder routes (#237), the skeleton (#245) and the create (#253). The plan's A5 is split in two to keep this PR reviewable.NameReservedByEnvand the narrowed-destination rule from the #253 final assessment go in the next PR (A5b), which lands before activation (A7). The narrowed-destination rule is now an A7 gate.Sync
Every reconcile of an object that has a Coder ID:
GET /api/v2/ai/providers/{id}. The answer must match the ID, the name and the type.Kill switches, only after a full apply of the current generation (
status.appliedGeneration == metadata.generation). The delete and the disable count also while a key resend is pending, so a Secret rotation never undoes a revocation. The key check waits for a pending resend, because the resend changes the key itself:DeletedExternallyStalled, zero writesDisabledExternallyStalled, zero writesKeyChangedExternallyStalled, zero writesA spec edit re-arms the object. The next sync then creates the provider again (only if the name is free), enables it, or sends its key again. A provider that is gone before a full apply is created again without a spec edit.
One sparse PATCH with every field that differs: display name (an empty spec name matches Coder showing the provider name), icon, enabled flag, base URL, and the key when it must be sent. No difference means no PATCH. The answer must show every requested value.
Key rules (plan 5.3):
status.credential, unless an in-memory SHA-256 of the last sent value shows that only the Secret's metadata changed. After a restart, that map is empty, so one resend can happen.status.credential.resendAttemptTime(the resend marker). A crash after the PATCH leaves the marker set, and the next reconcile sends the key once more. Coder ends with exactly that key.A 401, 403 or 429 is retried after 30 s. Another 4xx stalls with
CoderRejected. A 5xx or a transport error is retried.The base URL and the key in one PATCH means two things. Coder never pairs the old key with a new URL. And a provider is never enabled at a stale URL, because the enable and the URL go in the same request.
#253 follow-ups
A 4xx create answer is confirmed by a read. A proxy can put a 4xx in place of a committed create answer. The controller now reads the provider name after any 4xx, and it clears the create marker only when the read proves that nothing of this object exists (404, with a site-owner operator user):
NameConflict.CreateRetryingfor 401, 403, 409 and 429, andCoderRejectedfor any other status.Only a failed dial or TLS handshake still counts as "no effect" without a read. The shared
createHadNoEffect406 rule (fromCoderTemplateTest) no longer applies to providers.A nonce match without a recorded credential is accepted. It no longer fails an assertion. The object records the provider with an empty change token, so the sync sends the current key.
No enable at a stale base URL (see Sync).
Validation
New
internal/controller/coderaiprovider_sync_test.go(envtest plus the fake Coder). Most steps use a new reconciler, as a restart would.Readyprovider sends no write on later reconciles, with a display name and with an empty one.apiKeySecretRef.keyinside the same Secret resends.Pending, and the next reconcile resolves it by name.Ready, with the provider created again, enabled again, or holding its key again.Message,Detailand validations of a rejected create and a rejected key PATCH. The key appears in no status, annotation, log or error.Readywithout a second create.assertion failed:, and the next reconcile reads Coder and reportsReady.The A4 tests were updated where behavior intentionally changed. A
Readyobject now costs one GET per reconcile (the plan's resync read), and the old apply guards became the sync tests above.Mutation check: I broke the sync and create code in 19 ways (a 404 update not confirmed by a read, a rejected create not remembered, a zero ID handled softly, a provider gone right after the create not handled, a zero key ID accepted, a disable skipped during a resend, URL not sent with the key, enable sent before the URL, each of the three kill switches off, a deleted provider always created again, the in-memory hash ignored, the resend marker not written first, the status pointer not refreshed after the marker write, a PATCH without a difference, a kill switch that blocks a spec edit, a dashboard key not replaced on re-arm, a 4xx cleared without a read, a nil credential that fails an assertion). Each change made a test fail. One of them found a real bug while I wrote the tests: the status update after the resend marker replaces
provider.Status, so the new key ID was lost. That is fixed.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
ee660340left three P2 findings:2c8585b8. The nil step after a delete between the create and the first sync is gone. The reconcile reportsPending, and the next one looks up the name again.2c8585b8. A zero key ID is a mismatch, and the resend marker stays.2c8585b8makes the disable kill switch apply during a pending resend too, for the same reason. A spec edit re-arms the object as before.Review round 2
The requested reviews on
2c8585b8: the security review found no issues. The normal review left three findings, all fixed inef5e6bc7:ConfirmingRejectedCreateremembers the rejection. A later read that confirms absence then stalls withCoderRejectedinstead of sending the create again.assertion failed:error again. Only the recoverable case, a nonce match without a recorded credential, stays soft.Review round 3
The requested reviews on
ef5e6bc7: the security review found no issues. The normal review left one valid P2, "Validate the key when the PATCH omits it". If a spec edit re-arms the provider and someone rotates the key in Coder between the GET and a PATCH that sends no key, the next reconcile reportsKeyChangedExternally. The independent advisor agreed it is not a blocker. The outcome is fail-safe (Stalled, zero writes), the dashboard change is the kind that the kill switch respects (D11), and another spec edit recovers it. Under the review-round cap (from round 4, blockers only), it is deferred to A5b as #268, together with the advisor's note that a dashboard key change during a pending resend is overwritten by the Secret's value.Review round 4 (rebased head)
The requested reviews on
6a7811ea: the security review found no issues. The normal review left two findings. Neither is a blocker, so both are deferred to #268 (A5b, before activation):CoderAnswerMismatchstall is the plan's design (section 4.1 item 8) and matches the mergedCoderTemplateTesthandler (waitOnWrongAnswer). The condition keeps theassertion failed:text. The missing piece is an error log, which CoderAIProvider: check the key list on a key-less sync PATCH, and report key changes during a resend #268 adds.Shared test change
fakeFaultgets aMessagefield that replaces the error answer's message. The default stays the same, so other fake users do not change.Tracked for later layers
NameReservedByEnv, the narrowed-destination rule (CoderAIProvider: disable or flag a provider when its Secret stops allowing the provider's origin #267, an A7 gate), the resync jitter, and the two round-3 follow-ups above (CoderAIProvider: check the key list on a key-less sync PATCH, and report key changes during a resend #268).Rebase
The branch is rebased onto
mainat554c65a3, which includes #258 (the shared contract now also refuses a control plane's license and database Secrets). There were no conflicts. All gates passed again on the rebased tree. The review rounds above refer to the commits before this rebase. The same changes are nowbb1bc714-based commits ending in the current head.Size
266 production lines added and 58 removed. 543 test lines added and 100 removed. No generated files change.
Refs #154
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$40.56