Repository navigation
refactor(coderd): share the deployment chat setting PUT handler - #31018
Open
ibetitsmike wants to merge 1 commit into
Open
ibetitsmike wants to merge 1 commit into
ibetitsmike wants to merge 1 commit into
Conversation
Seven deployment chat setting PUT handlers repeated the same audit, authorization, decode, audited write, and response code. Move that into putChatOperationalSetting so each handler keeps only its validation and upsert, and share the 0 to maximum day check of the three day settings.
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. Keep them coming! 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". |
ibetitsmike
marked this pull request as ready for review
October 19, 2026 22:48
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
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.
Seven deployment chat setting
PUThandlers repeated the same steps: start the audit entry, check the deployment configupdatepermission, decode the body, run the audited write, and write the500or204. This PR moves those steps into one genericputChatOperationalSetting, so each handler keeps only its validation and upsert. The three day settings sharevalidateChatSettingDays, andinitChatOperationalSettingsAuditis inlined into the helper because only these handlers used it. The PR adds 105 lines and removes 239.The seven handlers set personal model overrides, the computer use provider, debug logging, the workspace TTL, chat retention days, debug retention days, and auto-archive days.
What changes: the workspace TTL handler loses a branch that turned a not-found or not-authorized write error into a
404. The branch cannot run:GetChatSiteConfigValueis an aggregate that always returns a row, and it andUpsertChatWorkspaceTTLcheck the same deployment configupdatepermission that the handler checks first. The other six handlers never had the branch.Why it is safe: each handler has its own coderd test, and
TestChatOperationalSettingsAuditwrites all seven settings and checks their audit entries. I also ran a throwaway test, not part of this PR, that sends 98 requests to the seven endpoints as an admin and as a member, with valid, unchanged, out-of-range, wrongly typed, and malformed bodies, and reads each setting back after every write. The status codes, response bodies, and audit entries are identical onmainand on this branch.Review focus: the
preparecallback, which returns the audited value and the write, or a400response.#31036 fixes an overflow in the workspace TTL handler on
mainby checking the 30-day maximum before the duration conversion. It conflicts with this PR in that handler, so whichever merges second will mergemainand keep that check.Validation: on
mainand on this branch,go test ./coderd -run '^(TestChatPersonalModelOverridesAdminSettings|TestChatComputerUseProvider|TestChatDebugLoggingSettings|TestChatWorkspaceTTL|TestChatRetentionDays.*|TestChatDebugRetentionDays|TestChatAutoArchiveDays|TestAuditedChatOperationalSettingWrite.*)$' -count=1passes (11 tests, 27 with subtests) andgo test ./enterprise/coderd -run '^TestChatOperationalSettingsAudit$' -count=1passes (17 with subtests). The pre-commit hook passed (make gen, formatting, golangci-lint, slim build).