fix: surface duplicate AI provider hostname as warning, not error - #28208
Conversation
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
Two live ai_providers rows can share the same base_url hostname. This
is an intended configuration (AIGOV-97): the direct routing path
(/api/v2/aibridge/{name}/...) routes by provider name and fully
supports it. Only the MITM proxy path cannot disambiguate, because
its nameByHost map is 1:1.
classifyProviderRow marked the second provider as ProviderStatusError,
which surfaced as a misleading log warning and a status=error
Prometheus label. The provider is not actually broken; it is fully
routable via the direct path. The error was only visible in server
logs and Prometheus, not in the API or UI.
This change:
- Extracts BaseURLHostname into coderd/aibridged and eliminates the
three duplicate implementations (proxy classifier, chatprovider,
and the API layer now uses it for status checks).
- Downgrades the proxy classifier from ProviderStatusError to a new
ProviderStatusProxyExcluded status. The provider is not broken;
it is just not reachable via the MITM proxy path.
- Adds Status *AIProviderStatus to codersdk.AIProvider with
Warnings []string, matching the WorkspaceProxy precedent.
- Populates warnings on create, update, GET, and list when a
provider's hostname collides with another enabled provider.
- Adds an integration test proving that a duplicate provider is
routable via the direct path but not via the proxy path.
Refs AIGOV-596
0441874 to
b18a82e
Compare
Add a warning badge with tooltip to ProviderRow when the API returns status.warnings for a hostname collision. The badge is keyboard- focusable and includes the full warning text in its aria-label. Includes Storybook story with play function asserting the badge is visible.
- aiProviderHostnameWarnings returns *AIProviderStatus directly instead of requiring a 3-line if-block at every call site - Remove IncludeDisabled: true and dead !other.Enabled check; GetAIProviders already filters disabled rows by default - Collapse two adjacent if blocks in reload.go into a single switch - Drop redundant proxy routing assertion from DuplicateHostDirectPathRoutesBoth; DuplicateHostFirstWins already covers it
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Move proxy_excluded from aibridged provider_info help text to aibridgeproxyd provider_info help text; the aibridged metric never emits proxy_excluded, only the proxy classifier does - Log GetAIProviders errors in aiProviderHostnameWarnings instead of silently swallowing them - Update monitoring.md to enumerate proxy_excluded for the proxy provider_info metric Addresses CRF-1, CRF-2, CRF-3, CRF-4, CRF-5 from deep-review round 1.
This comment has been minimized.
This comment has been minimized.
Update scripts/metricsdocgen/metrics with the corrected help text for coder_ai_gateway_proxy_provider_info, then regenerate docs/admin/integrations/prometheus.md. Addresses CRF-5 from deep-review round 2.
This comment has been minimized.
This comment has been minimized.
- CRF-7: only warn the alphabetical loser, not the winner; the winner is reachable via the proxy - CRF-8: rebrand warning message to AI Gateway and /api/v2/ai-gateway/ path - CRF-9: eliminate N+1 query in list handler by building the hostname collision map once from the outer rows - CRF-10: explicitly filter disabled and deleted peers in the collision map builder - CRF-11: stop event propagation on warning badge so clicking it does not navigate to the edit page - CRF-13: document proxy_excluded status in monitoring.md - CRF-14: log proxy_excluded at Info level, not Warn
This comment has been minimized.
This comment has been minimized.
- CRF-16: BaseURLHostname docstring now says it IS the canonical normalization, not that it matches the classifier - CRF-18: story play function asserts aria-label contains the collision hostname - CRF-19: remove nested TooltipProvider so badge uses the app's default 100ms tooltip delay - CRF-20: ProviderOutcome doc enumerates proxy-excluded rows - CRF-21: restore already-logged-at-call-site invariant in ProviderOutcome doc - CRF-22: add Linear URL to AIGOV-596 test comment
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- CRF-24: buildHostnameCollisionMap preserves DB ORDER BY name ASC instead of re-sorting with Go byte order, matching the proxy classifier's winner selection on all collations - CRF-25: add onKeyUp stopPropagation to warning badge so Space does not navigate the row - CRF-26: warning message now says AI Gateway Proxy, not the umbrella AI Gateway - CRF-27: add proxy_excluded row to providers.md status table
This comment has been minimized.
This comment has been minimized.
- CRF-28: story play function now tests keyboard activation (Enter and Space) on the warning badge to prevent regression of the stopPropagation fix - CRF-29: aiProviderHostnameWarningFromMap docstring now says database order, not alphabetical - CRF-30: monitoring.md now says database name sort order, not alphabetically first - CRF-31: monitoring.md now advises excluding proxy_excluded from status-based alerts - CRF-33: warning badge uses named stopPropagation function instead of three inline closures
This comment has been minimized.
This comment has been minimized.
- CRF-29: inline comment now says database order, not alphabetical - CRF-32: declined; the API warning and classifier error serve different audiences (API consumer vs operator reading logs) and are maintained in separate packages with different imports. Extracting a shared string would couple the API layer to the proxy classifier's error format for no behavioral gain.
This reverts commit 9fdd080. The cleanup introduced regressions: - CRF-37: warnings?.length can leak 0 in React JSX - CRF-38: stopPropagation comment lost the three-handler invariant - CRF-39: buildHostnameCollisionMap doc lost the ORDER BY tie - CRF-22: AIGOV-596 reference lost its URL again The panel noted the cleanup was unsolicited on a converging PR.
- CRF-40: rename others to allProviders for clarity - CRF-41: fix aiProviderHostnameWarningFromDB doc to say enabled, non-deleted providers, not all providers
This comment has been minimized.
This comment has been minimized.
- CRF-40: rename allProviders to enabledProviders; the query filters disabled and soft-deleted rows - CRF-42: story now tests mouse click on the warning badge - CRF-43: add EmptyWarnings story to guard against the 0-leak JSX guard regression - CRF-44: ProviderOutcome doc now says 503 sentinel, matching the codebase vocabulary - CRF-45: providers.md now says database name sort order (ORDER BY name ASC), matching monitoring.md
This comment has been minimized.
This comment has been minimized.
CRF-43: the story asserts no bare "0" text appears in the row, which fails if the JSX guard is reverted from warnings && warnings.length > 0 to warnings?.length.
This comment has been minimized.
This comment has been minimized.
CRF-46: the block doc, mid-body invariant, and assert message already carry the claim; the loop-header comment restated it a third time.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Inline winner := names[0] into the Sprintf call - Remove default: continue from the Reload switch; falling through already continues the loop
There was a problem hiding this comment.
Pull request overview
This PR adjusts AI Gateway provider “duplicate hostname” handling so it’s treated as a non-fatal routing limitation (proxy-only) rather than a broken provider, and surfaces that nuance via API/UI warnings while adding a new proxy_excluded proxy status.
Changes:
- Centralizes base URL hostname normalization into
coderd/aibridged.BaseURLHostnameand removes duplicate implementations (proxy classifier + chatd). - Downgrades duplicate-hostname classification from error to
proxy_excluded, updates metrics/logging/docs accordingly, and adds a direct-routing integration test proving both providers remain reachable by name. - Extends
codersdk.AIProvider(and generated FE types/docs) with an optionalstatuscarrying warnings, and renders those warnings in the Providers table via a tooltip.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| site/src/pages/AISettingsPage/ProvidersPage/components/ProviderRow.tsx | Renders a Warning badge + tooltip when provider status warnings are present. |
| site/src/pages/AISettingsPage/ProvidersPage/components/ProviderRow.stories.tsx | Adds Storybook coverage for hostname-collision warning and empty warnings behavior. |
| site/src/api/typesGenerated.ts | Adds generated AIProvider.status and AIProviderStatus (warnings) to FE API types. |
| scripts/metricsdocgen/metrics | Updates metric help text to include proxy_excluded status label. |
| enterprise/cli/aibridgeproxyd.go | Uses shared hostname normalization and classifies duplicate hostnames as proxy_excluded with a guidance message. |
| enterprise/cli/aibridgeproxyd_internal_test.go | Updates classifier tests for new proxy_excluded behavior and messaging. |
| enterprise/aibridgeproxyd/reload.go | Logs proxy_excluded providers separately (info) and updates router comments. |
| enterprise/aibridgeproxyd/reload_test.go | Updates reload tests for proxy_excluded series and adds direct-path routing test for duplicate hostnames. |
| enterprise/aibridgeproxyd/reload_internal_test.go | Updates defensive-router test commentary to match new classification semantics. |
| enterprise/aibridgeproxyd/metrics.go | Extends provider info metric help text to include proxy_excluded. |
| docs/reference/api/schemas.md | Documents the new status field and AIProviderStatus schema. |
| docs/reference/api/aiproviders.md | Updates AI providers endpoint docs/examples to include status.warnings. |
| docs/ai-coder/ai-gateway/providers.md | Documents proxy_excluded status meaning and its effect on routing paths. |
| docs/ai-coder/ai-gateway/monitoring.md | Documents alerting guidance for excluding proxy_excluded statuses. |
| docs/admin/integrations/prometheus.md | Updates Prometheus metric doc entry to include proxy_excluded. |
| codersdk/aiproviders.go | Adds Status *AIProviderStatus with warnings to the SDK provider type. |
| coderd/x/chatd/model_routing_aibridge.go | Switches chatd hostname detection to the centralized hostname normalizer. |
| coderd/x/chatd/chatprovider/chatprovider.go | Removes the duplicated hostname-normalization helpers. |
| coderd/x/chatd/chatprovider/chatprovider_test.go | Removes now-obsolete tests for the deleted hostname-normalization helper. |
| coderd/apidoc/swagger.json | Regenerates Swagger to include codersdk.AIProvider.status and codersdk.AIProviderStatus. |
| coderd/apidoc/docs.go | Regenerates embedded Swagger doc template with the new schema. |
| coderd/aibridged/provider.go | Adds ProviderStatusProxyExcluded and the canonical BaseURLHostname helper. |
| coderd/aibridged/provider_test.go | Adds unit tests for BaseURLHostname normalization behavior. |
| coderd/ai_providers.go | Populates AIProvider.Status warnings for hostname collisions on list/get/create/update. |
| coderd/ai_providers_test.go | Adds integration tests asserting hostname collision warnings across create/get/list/update. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| When alerting on `coder_ai_gateway_proxy_provider_info`, exclude | ||
| `proxy_excluded` from the alert expression unless you want to be | ||
| notified of intended duplicate-hostname configurations: | ||
| `status!="enabled",status!="proxy_excluded"`. |
| play: async ({ canvasElement, args }) => { | ||
| const canvas = within(canvasElement); | ||
| const badge = canvas.getByText(/warning/i); | ||
| await expect(badge).toBeInTheDocument(); | ||
| await expect(badge).toHaveAttribute( | ||
| "aria-label", | ||
| expect.stringContaining("api.openai.com"), | ||
| ); | ||
| await expect(badge).toHaveAttribute("tabIndex", "0"); | ||
|
|
||
| // Keyboard and mouse activation must not navigate the row. | ||
| badge.focus(); | ||
| await userEvent.keyboard("{Enter}"); | ||
| await userEvent.keyboard(" "); | ||
| await userEvent.click(badge); | ||
| await expect(args.onClick).not.toHaveBeenCalled(); | ||
| }, |
- Fix PromQL example: status!~"enabled|proxy_excluded" instead of repeating the same label matcher key - Add tooltip hover assertion to warning badge story
ssncferreira
left a comment
There was a problem hiding this comment.
LGTM 👍 there are a few nits, but can be addressed in a follow-up PR
| } | ||
| require.Nil(t, firstListed.Status, "first provider in database order should not get a warning in list") | ||
| require.NotNil(t, secondListed.Status) | ||
| require.Len(t, secondListed.Status.Warnings, 1) |
There was a problem hiding this comment.
nit: we should assert the warning message here as well.
| require.Contains(t, second.Status.Warnings[0], `"first"`) | ||
| require.Contains(t, second.Status.Warnings[0], "AI Gateway Proxy") | ||
| require.Contains(t, second.Status.Warnings[0], "/api/v2/ai-gateway/second/...") |
There was a problem hiding this comment.
nit: this could probably be simplified to
require.Equal(t, []string{
`hostname "api.openai.com" is claimed by provider "first"; not reachable via the AI Gateway Proxy, use direct routing (/api/v2/ai-gateway/second/...) instead`,
}, updated.Status.Warnings)
| @@ -283,11 +283,12 @@ an API key. | |||
| Every provider carries an explicit status, surfaced through the | |||
| [`provider_info`](./monitoring.md#prometheus-metrics) metric and the API: | |||
There was a problem hiding this comment.
IIUC, this status table is only referencing the metrics, not the API, right?
| return nil | ||
| } | ||
| return &codersdk.AIProviderStatus{Warnings: []string{ | ||
| fmt.Sprintf("hostname %q is claimed by provider %q; not reachable via the AI Gateway Proxy, use direct routing (/api/v2/ai-gateway/%s/...) instead", host, names[0], provider.Name), |
There was a problem hiding this comment.
Should we send the warning even if AI Gateway proxy is disabled? We don't need to check if it is enabled, but at least we should add a note like "If AI Gateway Proxy is enabled, this provider will be excluded from proxy routing.”
Two live
ai_providersrows can share the samebase_urlhostname. This is an intended configuration (AIGOV-97): the direct routing path (/api/v2/ai-gateway/{name}/...) routes by provider name and fully supports it. Only the MITM proxy path (aibridgeproxyd) cannot disambiguate, because itsnameByHostmap is 1:1.classifyProviderRowmarked the second provider asProviderStatusError, which surfaced as a misleading log warning and astatus=errorPrometheus label. The provider is not actually broken; it is fully routable via the direct path. The error was only visible in server logs and Prometheus, not in the API or UI.This change:
BaseURLHostnameintocoderd/aibridgedand eliminates three duplicate implementations (proxy classifier, chatprovider, and the API layer now uses it for status checks).ProviderStatusErrorto a newProviderStatusProxyExcludedstatus. The provider is not broken; it is just not reachable via the MITM proxy path.Status *AIProviderStatustocodersdk.AIProviderwithWarnings []string, matching theWorkspaceProxyprecedent.Refs AIGOV-596
Generated by Coder Agents
recording-2026-08-19T12-07-30Z.mp4