Repository navigation
chore!: remove the deprecated advisor config request fields - #30982
Draft
ibetitsmike wants to merge 1 commit into
Draft
ibetitsmike wants to merge 1 commit into
ibetitsmike wants to merge 1 commit into
Conversation
Remove DeprecatedModelConfigID and DeprecatedReasoningEffort from UpdateAdvisorConfigRequest, and the 400 that PUT /api/experimental/chats/config/advisor returned when a client still sent them. Such a request now succeeds and the two fields are ignored. The advisor model is set through the organization model override endpoint. Also drop the clamp of negative stored limits in the GET handler. The PUT handler is the only writer of agents_advisor_config and has rejected negative limits since the endpoint was added, so a stored limit is never negative.
Collaborator
Author
|
@codex 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 Review: Didn't find any major issues. Bravo. 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". |
This branch has not been deployed
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.
Removes the two deprecated advisor model fields,
model_config_idandreasoning_effort, fromUpdateAdvisorConfigRequest. It also removes the400thatPUT /api/experimental/chats/config/advisorreturned when a client still sent them, and the clamp of negative stored limits in the matchingGET.make gendrops the fields fromsite/src/api/typesGenerated.ts.Behavior change, approved by Mike: a
PUTthat still sendsmodel_config_idorreasoning_effortnow succeeds, and the endpoint ignores both fields, becausehttpapi.Readignores unknown JSON fields. Before, it failed with400and namedPUT /api/v2/organizations/{organization}/chats/model-overrides/advisor, which has set the advisor model since #28442. Nothing in this repository sends the fields: the settings page sends only the two limits, and no CLI command uses the request type.Why removing the clamp is safe:
putChatAdvisorConfigis the only production writer ofagents_advisor_config, and it has rejected negative limits since #24621 added the endpoint, in the same commit as the clamp. No migration writes this key. So a stored limit is never negative.TestChatAdvisorConfig_StaleModelWriteRejected(the removed400) andTestChatAdvisorConfig_ClampsNegativeStoredValues(the removed clamp).TestChatAdvisorConfig_IgnoresLegacyReasoningEffortstays. Migration 000583 left old advisor blobs as they were, so upgraded databases can still hold the legacy key, and the test covers howGETreads them.Review focus: the behavior change, for any client outside this repository that still sends the deprecated fields to this experimental endpoint.
Validation:
go test ./coderd -run '^TestChatAdvisorConfig' -count=1passes (10 tests),go vet ./coderd ./codersdkis clean, and the pre-commit hook passed (make gen, formatting, golangci-lint over every package, and the slim build).Stack
This stack removes the deprecated advisor config request fields, then runs the advisor config tests on one server.