Repository navigation
Conversation
952120f to
c4efb77
Compare
877f496 to
590bdfe
Compare
…irect routing Deprecate the AIGatewayRoutingEnabled deployment option as a no-op instead of hard-deleting it, so deployments with the flag in their YAML config or CLI flags don't break on upgrade. Delete the direct routing implementation, collapse the resolvedModelRoute discriminated union into aiGatewayModelRoute, remove the dead providerKeys cascade, delete the preferredShortTextCandidates quickgen function, and simplify the advisor override error handling. The option will be fully removed in a future release.
…itle path gofmt advisor_internal_test.go and drop the dead chatprovider import in title_override_internal_test.go. Remove the providerKeys parameter from generateManualTitleCandidate, proposeChatTitleWithStore, and regenerateChatTitleWithStore, plus the upstream resolveUserProviderAPIKeys calls in RegenerateChatTitle and ProposeChatTitle that fed them. The manual title path stopped reading keys when title_override.go dropped its keys parameter.
- coderd: chat daemon only constructs when BridgeConfig.Enabled. Chat HTTP handlers return 503 with a clear remediation message via a new requireChatDaemon guard. - codersdk: ChatModelsResponse gains ai_gateway_disabled. listChatModels short-circuits with the signal so the UI can render a banner. - site: AgentsPageView renders a warning banner with copy: "AI Gateway must be enabled for Coder Agents functionality." Plumbed from AgentsPage via the new SDK field. Storybook AIGatewayDisabled story added. - cli: warnDeprecatedChatAIGatewayRoutingFlag logs once at startup when the operator explicitly set the deprecated flag. - chatd/advisor: restore SuccessReturnsOverrideModelAndConfig and AIProviderIDResolvesOverrideProviderKeys to test the actual success path by wiring a mock transport factory. - chatd/quickgen: inline the single-element candidates loop in maybeGenerateChatTitle and generateTurnStatusLabel; the multi-candidate retry was specific to direct routing.
590bdfe to
c0cd83f
Compare
Docs preview📖 View docs preview for |
…re-chatd tests PR #26658 only migrated full-server tests to AI Gateway routing; this migrates the bare-chatd tests that build *chatd.Server directly via chatd.New/newActiveTestServer/newTestServer/newInternalTestServer, which PR #26862 left without a transport factory once direct routing was deleted. - 78 tests wired to an existing mock provider URL via chattest.NewMockAIBridgeTransport, mirroring the pattern already used in 10 passing tests. - TestProviderSwitchSanitizesAndRestoresPEToolHistory and TestComputerUseSubagentToolsAndModel each drive two providers in one chat; added a small providerRoutedTransportFactory that dispatches by provider name instead of extending chattest's single-target mock. - 6 TestSpawnAgent_* tests had an unrelated bug: they call runSubagentTool directly and never set aibridge.WithDelegatedAPIKeyID on ctx, so they failed before transport wiring was ever reached. Added the missing ctx line, copying the established sibling pattern. - newInternalTestServer gained one new withInternalTestServerTransportFactory option, mirroring its existing With* options, for the two internal-package tests that needed it. - TestRegenerateChatTitle_PersistsAndBroadcasts and its idle variant exposed a real call-sequence change: PR #26862 deleted the pre-lock resolveUserProviderAPIKeys call these tests' gomock expectations still modeled, and title-generation routing now goes through GetAIProviderByID instead of GetAIProviders/GetAIProviderKeysByProviderIDs. Rewrote the mock expectations and AIProviderID wiring to match the current code path; chatd.go itself is untouched. No production code changed.
…iring sweep - Restore the outgoing-request model assertion that was lost when TestRegenerateChatTitle_PersistsAndBroadcasts and its idle variant were rewritten to use a synthesized RoundTripper response instead of a real chattest.NewOpenAI server. The previous rewrite returned a canned response without checking what model was actually requested. - Drop the PR-number reference from the explanatory comment in both tests; state the current routing invariant instead. - Use the existing withSubagentDelegatedKey helper consistently at the two TestSpawnAgent_ComputerUse* call sites instead of spelling out aibridge.WithDelegatedAPIKeyID inline, matching the other four call sites added in the same change.
This comment was marked as resolved.
This comment was marked as resolved.
…ta, not chat models response Replace codersdk.ChatModelsResponse.AIGatewayDisabled with a new ai-gateway-enabled meta tag on the SPA shell HTML, mirroring the existing HideAITasks/tasks-tab-visible mechanism exactly. This is a deployment config boolean gating an AI feature for all users, not just deployment admins, so /api/v2/deployment/config (which requires ResourceDeploymentConfig read) isn't a usable home for it; the embedded-metadata mechanism already serves this exact purpose. - listChatModels drops its AI-Gateway-disabled special case and falls through to its normal catalog logic unconditionally, same as before this branch added the special case. - AgentsPage reads the new metadata key, defaulting to enabled (not disabled) when the tag is unavailable in dev/Storybook, so the banner doesn't show by default outside production. - AgentsPageView's own prop contract and its AIGatewayDisabled story are unchanged; only the data source moves. - Removed now-dead ai_gateway_disabled fixture lines forced by the type change across 5 story/test files.
…isabled derivation Compute aiGatewayDisabled directly from the metadata instead of deriving an intermediate aiGatewayEnabled local and inverting it at the prop site, per deep-review feedback.
This comment was marked as resolved.
This comment was marked as resolved.
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 2 findings (1 P0, 1 P3), COMMENT. Review Finding inventoryFinding inventoryFindings
Round logRound 1About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
First-pass review (Netero). The full review panel has not yet reviewed this PR; it will run after these findings are addressed.
The deprecation approach is sound: keeping the option in the OptionSet for YAML upgrade safety while removing the underlying routing path is the right call. The test sweep across 81 bare-chatd tests is thorough, and the embedded-metadata pattern for surfacing gateway state to the frontend mirrors the existing tasks-tab-visible pattern cleanly.
1 P0, 1 P3.
"This crashes the process on shutdown."
coderd/coderd.go:2344-2351
P0 [CRF-1] chatDaemon is now conditionally initialized (line 832, guarded by BridgeConfig.Enabled), but gitSyncWorker at line 869 is created unconditionally within the outer block. When AI Gateway is disabled, chatDaemon is nil, so api.chatDaemon.PublishDiffStatusChange (line 871) captures a nil receiver. When the worker processes a diff status change, calling the stored method value panics.
The same nil receiver causes a second crash here: api.chatDaemon.Close() (line 2351) is called unconditionally during shutdown, and Close() immediately calls p.closeInflightAdmission(), which dereferences the nil receiver.
Before this PR, chatd.New() was always called, so chatDaemon was never nil.
Fix: move the gitSyncWorker creation (lines 862-876) inside the if BridgeConfig.Enabled block, and add if api.chatDaemon != nil guards on api.chatDaemon.Close() (line 2351) and api.gitSyncWorker.Done() (line 2346). (Netero)
🤖
🤖 This review was automatically generated with Coder Agents.
…extual notice when AI Gateway is disabled Previously the top-of-page banner in AgentsPageView was the only signal that AI Gateway was disabled deployment-wide, and it didn't disable anything. Since this session's listChatModels simplification made /models reflect DB config regardless of runtime gateway state, a deployment with providers/models configured but AI Gateway off would show a chat input that looks usable but cannot actually work. - AgentSetupNotice gains an aiGatewayDisabled branch, checked before its existing provider/model checks (it can be true even when the catalog has entries), reusing the existing admin-aware messaging convention instead of a disconnected page banner. - AgentChatInput forwards the flag into its existing setup-notice logic. - Both composer entry points (AgentChatPage for existing chats, AgentCreateForm for new chats, reached via AgentsPage and AgentCreatePage) now fold aiGatewayDisabled into their existing disabled-state checks and read the ai-gateway-enabled embedded metadata added earlier this session. - Removed the now-redundant top-of-page banner and its story from AgentsPageView; AgentsPage no longer needs the metadata read it only fed.
…teway-disabled derivation - AgentChatInput picked the isAdmin/non-admin AgentSetupNotice branch based on providerCount/modelCount being defined, but aiGatewayDisabled resolves synchronously from embedded metadata while those counts are still loading. An admin would briefly see 'Contact your admin to enable it' before it flipped to the correct admin copy. The branch now short-circuits on aiGatewayDisabled directly, independent of the counts. - Extracted the repeated ai-gateway-enabled metadata read/inversion from AgentChatPage.tsx and AgentCreatePage.tsx into a single useAIGatewayDisabled() hook. - Tightened AgentChatInput's AIGatewayDisabledShowsSetupNotice story assertion, which matched a substring common to both the admin and non-admin copy and couldn't catch a wiring regression between the two; added a non-admin counterpart story. Per deep review.
This comment was marked as resolved.
This comment was marked as resolved.
…isabled Two crashes when AI.BridgeConfig.Enabled is false (chatDaemon stays nil): - The git sync worker captured api.chatDaemon.PublishDiffStatusChange as a method value unconditionally. A method value on a nil pointer receiver is itself non-nil, so the worker's own nil check on its callback never caught it; calling it later dereferenced the nil receiver and panicked. - api.chatDaemon.Close() was called unconditionally during shutdown, immediately dereferencing the nil receiver in closeInflightAdmission(). Only bind the git sync worker's publish callback when chatDaemon exists, and guard the shutdown Close() call. Added TestAIGatewayDisabledStartupAndShutdown, which panics without this fix (verified by reverting it locally) and passes with it. Also adds Test_warnDeprecatedChatAIGatewayRoutingFlag, covering the three explicit ValueSource cases plus the default/absent cases, per review feedback. Addresses coder-agents-review P0 and P3 findings on this PR.
… fix - Found and fixed a third, previously-unreported crash of the same class: workspaceAgentRPC assigned api.chatDaemon (a *chatd.Server, nil when AI Gateway is disabled) directly into the interface-typed ContextDirtyMarker field. A nil pointer assigned to an interface produces a non-nil interface value (a typed nil), so agentapi's own 'if DirtyMarker != nil' check didn't catch it, and calling HydrateAndMarkChatsDirty on the nil receiver would panic. The removed comment claiming chatDaemon is 'always constructed' was stale; extracted a small nil-safe constructor instead. - Extracted the git-sync-worker callback guard into chatDaemonPublishDiffStatusChangeFunc so it's directly unit testable (TestChatDaemonPublishDiffStatusChangeFunc) without needing to drive the worker's real-time tick loop. Verified this test fails against the naive unconditional-method-value version of the guard, and passes with the fix, per two reviewers converging on the same coverage gap in TestAIGatewayDisabledStartupAndShutdown.
This comment was marked as resolved.
This comment was marked as resolved.
…scan warnDeprecatedChatAIGatewayRoutingFlag looped over the whole option set with a continue to find one flag by name; serpent.OptionSet already ships ByFlag for exactly this lookup. Also drops one of two test cases (Default, None) that exercised the identical no-op branch of the deprecation switch. Per ponytail review.
- Drop the redundant NonNilChatDaemon subtest; a real non-nil chatDaemon's PublishDiffStatusChange is already exercised in coderd/exp_chats_test.go. Keep only the NilChatDaemon case, which is the actual non-obvious behavior this guard protects. - Drop the deprecated-option existence/description assertions added to TestChatAIGatewayRoutingEnabledDefault; overkill for an upgrade- safety no-op contract. - Collapse AgentSetupNotice's aiGatewayDisabled admin/non-admin branches into one message. Unlike the provider/model branches, there's no in-app settings page for this deployment-level flag for either audience, so the isAdmin distinction didn't gate any real capability difference. - Rename useAIGatewayDisabled to useAIGatewayEnabled, returning the metadata value directly instead of inverting it internally, so the hook name matches the ai-gateway-enabled metadata key it reads. Callers invert locally (!useAIGatewayEnabled()) where they need the disabled polarity. Also drops a redundant .available check in the hook itself, since registerValue only ever sets available:true alongside a defined value.
…n AI Gateway is disabled getChat is a pure read; gating it behind requireChatDaemon made every existing chat look not-found once the gateway was disabled. The FE chat WebSocket also kept reconnecting to the still-gated stream endpoint forever with no way to know it could never succeed.
Regression test for the chatDaemon nil-guard fix in getChat; every existing chat test starts the daemon, so nothing exercised this path.
…ed-flag-and-dead-direct-routing
|
Generated by Coder Agents on behalf of @johnstcn. Verified the change using CDP/Playwright using an authenticated admin session:
Existing chat detail (
|
There was a problem hiding this comment.
Pull request overview
This PR completes the migration to mandatory AI Gateway routing for chat by deprecating (as a no-op) the AIGatewayRoutingEnabled deployment option, removing the direct-routing implementation in chatd, and wiring an ai-gateway-enabled signal through server-rendered embedded metadata so the frontend can disable chat UX when the gateway is off.
Changes:
- Deprecate
chat-ai-gateway-routing-enabled(kept for upgrade safety) and warn when it is explicitly set. - Remove direct chat routing code paths in
chatd, collapsing routing types to an AI-Gateway-only model route and updating tests accordingly. - Gate chat functionality when the AI Gateway is disabled: backend returns clear 503s for chat operations needing
chatd, and frontend disables composer/UX via embedded metadata.
Reviewed changes
Copilot reviewed 49 out of 49 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| site/src/testHelpers/entities.ts | Add mock value for ai-gateway-enabled embedded metadata tests. |
| site/src/pages/AgentsPage/components/ChatPageContent.tsx | Thread aiGatewayDisabled prop into chat input. |
| site/src/pages/AgentsPage/components/ChatConversation/useChatStore.ts | Prevent chat WebSocket subscription when AI Gateway is disabled. |
| site/src/pages/AgentsPage/components/ChatConversation/chatStore.test.tsx | Add coverage for “no WebSocket when gateway disabled”. |
| site/src/pages/AgentsPage/components/AgentSetupNotice.tsx | Show a dedicated notice when AI Gateway is disabled, independent of model/provider counts. |
| site/src/pages/AgentsPage/components/AgentSetupNotice.stories.tsx | Story to validate the “AI Gateway disabled” notice rendering/copy. |
| site/src/pages/AgentsPage/components/AgentCreateForm.tsx | Disable creation inputs and surface setup notice when AI Gateway is disabled. |
| site/src/pages/AgentsPage/components/AgentCreateForm.stories.tsx | Story asserting create-form input is disabled when gateway is off. |
| site/src/pages/AgentsPage/components/AgentChatInput.tsx | Force setup notice path and pass aiGatewayDisabled through to the notice. |
| site/src/pages/AgentsPage/components/AgentChatInput.stories.tsx | Story to ensure notice appears even while catalog counts are loading. |
| site/src/pages/AgentsPage/AgentsPageView.tsx | Layout refactor (structural wrapper) to support updated Agents page composition. |
| site/src/pages/AgentsPage/AgentCreatePage.tsx | Derive aiGatewayDisabled from embedded metadata and pass to create form. |
| site/src/pages/AgentsPage/AgentChatPageView.tsx | Thread aiGatewayDisabled into chat view composition. |
| site/src/pages/AgentsPage/AgentChatPage.tsx | Disable chat composer and prevent live behavior when AI Gateway is disabled. |
| site/src/hooks/useEmbeddedMetadata.ts | Add ai-gateway-enabled metadata key and useAIGatewayEnabled() helper. |
| site/src/hooks/useEmbeddedMetadata.test.ts | Extend embedded metadata tests to include ai-gateway-enabled. |
| site/src/api/typesGenerated.ts | Mark ai_gateway_routing_enabled as deprecated in generated types. |
| site/site.go | Inject ai-gateway-enabled into HTML embedded metadata state. |
| site/index.html | Emit <meta property="ai-gateway-enabled" ...> for runtime metadata parsing. |
| codersdk/deployment.go | Deprecate AIGatewayRoutingEnabled option description; add deprecation comment to ChatConfig. |
| coderd/x/chatd/title_override.go | Remove direct-routing key cascade from title override resolution; route via AI Gateway only. |
| coderd/x/chatd/title_override_internal_test.go | Update title override tests to use AI Gateway transport factory and provider IDs. |
| coderd/x/chatd/subagent_internal_test.go | Add transport factory override wiring and ensure delegated key is present for gateway routing. |
| coderd/x/chatd/quickgen.go | Remove preferred “cheap model” candidate path and collapse to single AI Gateway route usage. |
| coderd/x/chatd/quickgen_internal_test.go | Remove tests tied to preferred candidate generation under direct routing. |
| coderd/x/chatd/model_routing.go | Remove resolvedModelRoute discriminated union and direct-routing selection logic. |
| coderd/x/chatd/model_routing_internal_test.go | Update routing tests for AI Gateway-only route shape and behavior. |
| coderd/x/chatd/model_routing_direct.go | Delete the direct routing implementation. |
| coderd/x/chatd/model_routing_aibridge.go | Return aiGatewayModelRoute directly (no wrapper kind) and update return types. |
| coderd/x/chatd/integration_responses_test.go | Ensure integration tests provide an AI Bridge transport factory. |
| coderd/x/chatd/generation.go | Remove provider-keys field from generation preparation state. |
| coderd/x/chatd/generation_preparer.go | Resolve chat model via AI Gateway route only; remove provider-keys plumbing. |
| coderd/x/chatd/generation_preparer_internal_test.go | Update tests to provide transport factory and align assertions with removed provider keys. |
| coderd/x/chatd/computer_use.go | Update computer-use model resolution signature to AI Gateway route type. |
| coderd/x/chatd/chatd.go | Remove aiGatewayRoutingEnabled server field and simplify advisor override routing error behavior. |
| coderd/x/chatd/chatd_test.go | Update extensive chatd tests to inject AI Bridge transport factory and remove routing flag usage. |
| coderd/x/chatd/chatd_retry_test.go | Wire mock transport factory for retry tests. |
| coderd/x/chatd/chatd_internal_test.go | Update title regeneration tests to use AI Gateway transport; add request-model assertion helper. |
| coderd/x/chatd/chatd_debug.go | Simplify provider hint handling for debug-aware model creation under AI Gateway routing. |
| coderd/x/chatd/chatd_chainmode_test.go | Inject AI Bridge transport factory into chain-mode tests. |
| coderd/x/chatd/advisor_internal_test.go | Update advisor override tests for AI Gateway-only routing and required delegated API key IDs. |
| coderd/workspaceagentsrpc.go | Fix typed-nil interface assignment when chatDaemon is nil (gateway disabled). |
| coderd/exp_chats.go | Add requireChatDaemon and gate chat operations with 503 when gateway disabled. |
| coderd/exp_chats_test.go | Add regression test ensuring getChat still works with AI Gateway disabled. |
| coderd/coderd.go | Only construct chat daemon when AI Gateway enabled; fix nil method-value callback and guarded shutdown. |
| coderd/coderd_test.go | Regression test for clean startup/shutdown with AI Gateway disabled. |
| coderd/coderd_internal_test.go | Unit test proving chatDaemonPublishDiffStatusChangeFunc(nil) returns a true nil func. |
| cli/testdata/server-config.yaml.golden | Update golden config docs for deprecated routing flag description. |
| cli/server.go | Warn when deprecated chat-ai-gateway-routing-enabled is explicitly set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
DanielleMaywood
left a comment
There was a problem hiding this comment.
Just giving a review of the frontend for now
- Collapse a vestigial flex-row wrapper div in AgentsPageView left over from a removed banner; restore sm:flex-row on the outer layout div. - Drop three comments that only restated the code they annotated. - Tighten requireChatDaemon's doc comment: it no longer covers all chat HTTP operations now that pure reads (getChat) are exempt.
mafredri
left a comment
There was a problem hiding this comment.
Writing critique of this PR against the clear-writing standard, scoped to the PR description and the net-diff comments (superseded intermediate-commit comments excluded). Comment-level findings are inline; description findings are below. Each finding includes full replacement text.
Description
-
Process theater. The closing line
All changes reviewed via multi-reviewer deep-review passes; findings addressed inline in review threads.
announces thoroughness without telling a reader anything about the change or how to act on it. Remove it; the description already states the scope. Nothing replaces it.
-
Unresolvable shorthand.
Depends on #26658 (PR 2 test migration). All tests already run through AI Gateway routing from PR 2."PR 2" is an internal label a reader of this PR cannot resolve. Replacement:Depends on #26658, which migrated the chat tests onto AI Gateway routing. With that merged, every test already exercises the AI Gateway path.
-
Reader load in the lead paragraph. The headline (deprecate the option as a no-op) and five separate cleanups are chained into one sentence, so the lead does not stand alone and the cleanups are not a scannable set. Replacement for the first paragraph:
Deprecate the
AIGatewayRoutingEnableddeployment option as a no-op instead of hard-deleting it, so deployments that still carry the flag in YAML or CLI args do not break on upgrade.This also removes the now-dead direct-routing code:
- Delete the direct routing implementation.
- Collapse the
resolvedModelRoutediscriminated union intoaiGatewayModelRoute. - Remove the dead
providerKeyscascade. - Delete the
preferredShortTextCandidatesquickgen function. - Simplify the advisor override error handling.
🤖 This review was automatically generated with Coder Agents.
…ed-flag-and-dead-direct-routing Resolves conflicts with #26877 (drop chat_model_configs provider column): - Delete model_routing_direct.go entirely (dead code path this PR removes). - Drop `Provider` field from database.ChatModelConfig literals (column dropped). - Replace `route.providerHint()` calls with `string(route.Provider.Type)` (method removed alongside the resolvedModelRoute union we collapsed). - Keep our single-`candidate` shape in quickgen; drop the multi-candidate fallback fragments from main. - Fold main's added `GetAIProvider*` mock expectations into our test wiring where needed; preserve our aibridgeTestAIProvider helper (which sets Provider.Name, required by newAIGatewayModel validation).


Closes https://linear.app/codercom/issue/CODAGT-682
Closes https://linear.app/codercom/issue/CODAGT-598
This PR removes the now-dead direct-routing code:
It also deprecates the AIGatewayRoutingEnabled deployment option as a no-op instead of hard-deleting it, so deployments that still carry the flag in YAML or CLI args do not break on upgrade.
Once direct routing was gone, the AI Gateway became mandatory for chat, which surfaced gaps in how the product behaves with the gateway disabled:
ai-gateway-enabledto the frontend via embedded page metadata (same mechanism astasks-tab-visible) instead of a field on the models response.AgentSetupNoticewhen the gateway is disabled, for both new and existing chats.chatDaemonpanics on startup and shutdown (git sync worker method value,Close(),ContextDirtyMarkerinterface assignment) when the gateway is disabled.getChatincorrectly 503ing (making existing chats look not-found in the UI) even though it's a pure DB read; stop the chat WebSocket from retrying the still-gated stream endpoint forever when the gateway is disabled.