test(coderd/x/chatd): seed anthropic provider for computer_use tests#24611
test(coderd/x/chatd): seed anthropic provider for computer_use tests#24611ibetitsmike merged 2 commits intomainfrom
Conversation
|
/coder-agents-review |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, focused test helper with good scoping. The PR correctly limits the DB seed to the three tests that drive a computer_use spawn, leaves negative-availability tests untouched, and preserves the existing comment on seedInternalChatDeps. One P2, two P3s, one Nit.
The P2 is important: the three targeted tests pass at the base SHA without this fix. The resolveUserProviderAPIKeys path (where PruneDisabledProviderKeys clears the Anthropic key) runs in the background processing loop, which these tests do not exercise. The fixture is defensively correct and models production more accurately, but it is currently inert.
"Shall I show you? ♥" (Hisoka, after running the tests at the base SHA and finding them green)
coderd/x/chatd/subagent_internal_test.go:1012
P3 [DEREM-3] The static Anthropic: "test-anthropic-key" passed to newInternalTestServer is now redundant at the three call sites that also seed the DB provider. With seedEnabledAnthropicProvider in place, both isAnthropicConfigured (finds the DB provider at subagent.go:72-80) and resolveUserProviderAPIKeys (DB provider's central key populates ByProvider["anthropic"] in ResolveUserProviderKeys) work without the static fallback.
The concern is signaling: five other tests (lines 719, 765, 801, 885, 926) pass only the static key without a DB provider and work fine because they only exercise isAnthropicConfigured. A future test author who sees both patterns (static key + DB provider here, static key only elsewhere) may assume the static-key-only pattern is sufficient for spawning computer_use, copy it, and silently reproduce the original pruning bug. Removing the static key from the three fixed tests would clarify that the DB provider is the necessary part. (Meruem P3, Ryosuke Note, Chopper Note)
🤖
coderd/x/chatd/subagent_context_internal_test.go:470
P3 [DEREM-2] TestSpawnComputerUseAgentInheritsContext has the same missing DB provider seed as the three tests this PR fixes. It passes Anthropic: "test-anthropic-key" to newInternalTestServer (line 476) and spawns a computer_use child via tool.Run (line 486). Same class of bug: the background loop wakes via signalWake(), acquires the pending child chat, and reaches resolveUserProviderAPIKeys where PruneDisabledProviderKeys clears the Anthropic key.
The seedEnabledAnthropicProvider helper from this PR could be called here as well. (Kite)
🤖
🤖 This review was automatically generated with Coder Agents.
| // seedEnabledAnthropicProvider inserts an enabled Anthropic provider for | ||
| // the current test user so computer_use flows keep Anthropic credentials | ||
| // after provider-key pruning. | ||
| func seedEnabledAnthropicProvider( |
There was a problem hiding this comment.
P2 [DEREM-1] The DB fixture this helper inserts is currently inert. All three targeted tests pass at the base SHA without the fix.
The spawn tool path (tool.Run -> loadSubagentSpawnParentChat -> resolveSubagentDefinition -> buildOptions -> createChildSubagentChatWithOptions) never calls resolveUserProviderAPIKeys. The key-pruning path (PruneDisabledProviderKeys) lives in resolveChatModel inside the background processing loop (processChat), which these tests do not exercise. The isAnthropicConfigured availability check (subagent.go:69) short-circuits on the static providerAPIKeys key, so it never queries the DB either.
Verified empirically: reverted to base SHA e3a1fb0c89, ran all three tests (TestSubagentLifecycleToolsIncludePersistedSubagentTypeAcrossVariants/ComputerUse, TestSpawnAgent_ComputerUseUsesComputerUseModelNotParent, TestSpawnAgent_ComputerUseInheritsMCPServerIDs) under -count=1. All pass.
The mechanism described in the PR description is real: PruneDisabledProviderKeys does clear keys.Anthropic when no Anthropic provider row exists. But that code runs in the background processing loop, and the tests assert spawn-time state (child chat mode, model config, MCP server IDs), not post-processing state. The fixture is defensively correct and makes the test environment more production-realistic, but no assertion proves it works.
Could we add a targeted assertion that exercises the key-resolution path? Something like calling resolveUserProviderAPIKeys(ctx, user.ID) on the server and asserting the Anthropic key survives. That would make the fixture load-bearing. (Bisky P2, Hisoka P3, Mafu-san P2)
🤖
There was a problem hiding this comment.
Good catch. Added TestResolveUserProviderAPIKeys_PreservesAnthropicKeyFromDBProvider in commit 62336b5 to make the fixture load-bearing:
- Positive subtest: seeds the DB Anthropic provider via
insertEnabledAnthropicProvider, constructs the server with no static Anthropic key, and assertsresolveUserProviderAPIKeysreturns the Anthropic key from the DB-backed provider. - Negative subtest: no DB provider, static fallback Anthropic key on the server, and asserts
resolveUserProviderAPIKeysprunes it.
Together they prove the pruning behavior and that the helper actually prevents it.
This reply was posted by Mux working on Mike's behalf.
| // seedEnabledAnthropicProvider inserts an enabled Anthropic provider for | ||
| // the current test user so computer_use flows keep Anthropic credentials | ||
| // after provider-key pruning. | ||
| func seedEnabledAnthropicProvider( |
There was a problem hiding this comment.
Nit [DEREM-4] This file distinguishes seed* from insert*: seed* helpers create multi-entity fixtures (seedInternalChatDeps creates user + org + membership + provider + model; seedWorkspaceBinding creates org + template + workspace + build + resource + agent). insert* helpers do a single logical DB insert (insertInternalChatModelConfig, insertAssistantMessage, insertLinkedChatFile). This helper does one db.InsertChatProvider call, so insertEnabledAnthropicProvider would match the file convention. (Gon)
🤖
There was a problem hiding this comment.
Renamed to insertEnabledAnthropicProvider in commit 62336b5 to match the file convention.
This reply was posted by Mux working on Mike's behalf.
|
Addressed all review items in commit 62336b5:
Full chatd package passes: /coder-agents-review
|
There was a problem hiding this comment.
All four R1 findings verified as fixed in commit 62336b5. No new findings from the panel (Bisky, Mafuuu, Meruem, Knov).
The new TestResolveUserProviderAPIKeys_PreservesAnthropicKeyFromDBProvider makes the fixture load-bearing with clean positive/negative subtests that exercise the full resolveUserProviderAPIKeys -> PruneDisabledProviderKeys path. The redundant static fallback keys are gone from all DB-seeded call sites, and TestSpawnComputerUseAgentInheritsContext now has matching coverage. Helper renamed to insertEnabledAnthropicProvider per file convention.
R1: 1 P2, 2 P3, 1 Nit. R2: 0 new. All addressed.
"The helper's contract is honest and narrow." (Knov)
🤖 This review was automatically generated with Coder Agents.
3 participants
TestSubagentLifecycleToolsIncludePersistedSubagentTypeAcrossVariants/ComputerUseand two adjacent positive tests passed a static Anthropic key into
newInternalTestServer, butseedInternalChatDepsonly inserts an OpenAIprovider. At runtime,
Server.resolveUserProviderAPIKeyscallschatprovider.PruneDisabledProviderKeys, which clearskeys.Anthropicbecause Anthropic is not in the enabled DB provider set, so the
computer_useexecution path loses its key.Add a focused test helper
seedEnabledAnthropicProviderand use it only inthe positive tests that actually drive a
computer_usespawn through theruntime key-resolution path (the
computer_usebranch ofTestSubagentLifecycleToolsIncludePersistedSubagentTypeAcrossVariants,TestSpawnAgent_ComputerUseUsesComputerUseModelNotParent, andTestSpawnAgent_ComputerUseInheritsMCPServerIDs).seedInternalChatDepsstays unchanged, so the negative availability tests continue to model the
"Anthropic unavailable" fixture. No production code is modified.
Closes coder/internal#1486