Repository navigation
refactor(coderd/x/chatd/chatloop): resolve tool name aliases in one toolBatch method - #30939
ibetitsmike wants to merge 3 commits into
Conversation
…oolBatch method runOne, notifyStepToolCallObservers, notifyStepToolResultObservers, and isSerialToolCall each repeated the ToolNameAliases lookup. They now call (*toolBatch).resolve, and the observers and isSerialToolCall become toolBatch methods instead of taking the tool map and alias map. runOne also looks up result provider metadata by the resolved name, like its other lookups. No production alias maps to a provider tool with result metadata, so this changes nothing in production. TestExecuteToolsSerialToolCallOrder calls one serial tool by an alias, so dropping the alias lookup from isSerialToolCall now fails a test.
|
@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". |
TestExecuteLocalTools_ProviderToolRunner now also calls the runner by an alias and checks that the result keeps the runner's provider metadata. This pins the resolved-name metadata lookup in runOne: with the raw name from the model, a runner called by an alias loses its metadata, and the test fails on the layer below.
|
@codex review
|
|
Codex Review: Didn't find any major issues. Delightful! 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". |
Adds
(*toolBatch).resolvefor theToolNameAliaseslookup thatrunOne,notifyStepToolCallObservers,notifyStepToolResultObservers, andisSerialToolCalleach repeated. The last three becometoolBatchmethods instead of taking the tool map and the alias map. Where they looked up a tool and then type-asserted it, they now assert on the map value: a missing name gives a nil interface, so the assertion fails as the missing-key check did.It also fixes a latent bug:
runOnelooked up result provider metadata by the name the model sent, while its other lookups use the resolved name, so a runner called by an alias lost its metadata. This cannot happen in production today: the only alias isclose_agenttointerrupt_agent(subagent.go:45), and onlycomputer_use.go:112setsResultProviderMetadata.Why it is safe
Each mutant below puts the raw name back at one call site, and each fails a test:
runOnetool lookupTestExecuteSingleTool_ResolvesToolNameAliasand 5 morerunOnemetadata lookupTestExecuteLocalTools_ProviderToolRunnernotifyStepToolCallObserversTestExecuteToolsNotifiesStepToolCallObserversnotifyStepToolResultObservers(either loop)TestExecuteToolsNotifiesStepToolResultObserversisSerialToolCallTestExecuteToolsSerialToolCallOrderNo test called a serial tool or a provider runner by an alias, so this PR adds one aliased call to
TestExecuteToolsSerialToolCallOrderand one toTestExecuteLocalTools_ProviderToolRunner. With thechatloop.goof #30938, the first test still passes, and the second fails because the aliased result has no metadata.Validation
go test -race -count=1 ./coderd/x/chatd/chatloop/passes on refactor(coderd/x/chatd): replace executeTools and executeSingleTool with toolBatch #30938 and here.go test -count=1 -p 4 -parallel 4 ./coderd/x/chatd/...passes here (20 packages).golangci-lint run ./coderd/x/chatd/chatloop/...reports 0 issues, and the pre-commit hook passed.Stack
This stack replaces the 20-parameter
executeToolsand the 15-parameterexecuteSingleToolinchatloopwith methods on onetoolBatchtype, and resolves tool name aliases in one place. Production behavior does not change.ExecuteLocalToolsexecuteToolsandexecuteSingleToolwithtoolBatchtoolBatchmethod