Repository navigation
refactor(coderd/x/chatd): delete the write-only generatedChatTitle - #30926
ibetitsmike wants to merge 3 commits into
Conversation
maybeGenerateChatTitle stored each generated title in a generatedChatTitle so that tests could read it back. Production passed a fresh one and never read it. The tests already check the stored title and the title_change event. The cases that write no title now use a pubsub mock that fails on any Publish. Without that, dropping the return after a refused title write would go unnoticed: it publishes an empty chat on the nil owner's channel, which the owner subscription never sees.
|
@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. Hooray! 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 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". |
Deletes
generatedChatTitle, a mutex-guarded cell in whichmaybeGenerateChatTitlestored each generated title so that tests could read it. Production passed a fresh one and never read it. The PR removes the type, itsStoreandLoadmethods, the parameter, and the fiveLoadchecks in the title tests.Why it is safe
git grep generatedChatTitlefinds nothing. Each removedLoadcheck has a remaining check on the stored chat row or the published event:WritesGeneratedTitle: title storedtitle_changeeventKeepsCurrentTitleWhenNoTitleIsWritten, both cases: nothing storedTestQuickgenFollowsConfiguredRetries/Title: nothing storedmodel call failedandTitleGenerationOverrideCallFailurecover the same returnTitleGenerationOverrideSetUsable: title storedUpdateChatTitleByIDTitleGenerationOverrideCallFailure: nothing storedUpdateChatTitleByIDOne gap needed a test change. If the
returnafter a refused title write is dropped, the code publishes the zeroupdatedChaton the nil owner's channel, which the test's owner subscription never sees. Onmainonly the observer caught that (quickgen_internal_test.go:688). The two no-write cases now use apsmockpubsub with no expectations, so any publish fails them.Mutants of
maybeGenerateChatTitle(go test -overlay) and the tests they fail:returnafter a refused writewrite refused by a rename during the callreturnafter a failed model callmodel call failed,TitleGenerationOverrideCallFailuretitle + "!"WritesGeneratedTitle,TitleGenerationOverrideSetUsable,TestMaybeGenerateChatTitleAppliesModelConfigReasoningEffortpublishChatPubsubEventWritesGeneratedTitleReview focus
The mock in the two no-write cases is stricter than the old owner subscription: a publish on any channel fails.
Validation
go test ./coderd/x/chatd/ -count=1 -race -run 'Title|Quickgen': pass.make pre-commit: gen, fmt,lint/go,lint/go/new-code, slim build) passed.