Repository navigation
test(coderd/x/chatd): fold the duplicate advisor scenarios - #30993
ibetitsmike wants to merge 4 commits into
Conversation
TestAdvisorGating_RootChat replayed the mixed advisor and read_file batch of TestActiveServer_ExclusiveToolPolicy. Move its exact model call count into that subtest and delete it; the Root table row checks the advisor tool and guidance. Check the persisted advisor result in TestAdvisorHappyPath_RootChat and delete the solo exclusive subtest, which it now covers.
|
@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. Swish! 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". |
TestAdvisorGating_RootChat was the only test that read the advisor guidance in the first model request of a root chat; the Root table row reads the prompt before generation. TestAdvisorHappyPath_RootChat now records the parent's first request and checks that it carries ParentGuidanceBlock, so a mutant that drops the guidance from the request fails it.
|
@codex review
|
|
Codex Review: Didn't find any major issues. 👍 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". |
Two end-to-end advisor scenarios repeated other tests. This PR moves their unique checks into tests that stay and deletes them.
TestAdvisorGating_RootChatreplayed the advisor andread_filebatch of themixed exclusive and local tools commit policy errorssubtest. Its exact two-call count moves into that subtest, which already checks thatread_filenever runs and both stored errors; theRootrow (test(coderd/x/chatd): cover advisor eligibility in the tool policy table #30990) checks the advisor tool and guidance.solo exclusive tool executessubtest is a subset ofTestAdvisorHappyPath_RootChat, which now also checks the stored advisor result.Why it is safe
>= 2became== 2RootrowTestExclusiveToolPolicy_MixedBatchErrors(text)RootChatRootChatRootChatonly{}Only an exact count sees the nested advisor call of the third mutant, so the count moves instead of going away.
Coverage with
-coverpkg=./coderd/x/chatd/...: the tests left at the top of the stack reach every block the deleted tests reached, except an emptycase <-ctx.Done():inrunnerManager.requestCleanupthat only shutdown timing reaches.Review focus
The subtest reads the count after the chat reaches
waiting, and its fake model server counts only streamed requests, so title requests do not change it.Timing
From 10 runs, the two deleted scenarios took 0.83 s summed (median per test).
Validation
go test ./coderd/x/chatd/ -run '^(TestActiveServer_ExclusiveToolPolicy|TestAdvisorHappyPath_RootChat)$' -count=10, and with-race -count=3: passgo test ./coderd/x/chatd/...: passgo test -overlaymutant runs before and after this PR: results aboveStack
The stack moves advisor eligibility and the root versus child tool split into
TestBuildTurnEnvironmentToolPolicyand deletes the end-to-end tests that only checked them, finishing #30868. It is based on #30911.