Skip to content

fix: surface duplicate AI provider hostname as warning, not error - #28208

Merged
ssncferreira merged 22 commits into
mainfrom
aigov-596-hostname-collision-warnings
Aug 24, 2026
Merged

fix: surface duplicate AI provider hostname as warning, not error#28208
ssncferreira merged 22 commits into
mainfrom
aigov-596-hostname-collision-warnings

Conversation

@johnstcn

@johnstcn johnstcn commented Aug 17, 2026

Copy link
Copy Markdown
Member

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/ai-gateway/{name}/...) routes by provider name and fully supports it. Only the MITM proxy path (aibridgeproxyd) 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 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

Generated by Coder Agents

recording-2026-08-19T12-07-30Z.mp4

@linear-code

linear-code Bot commented Aug 17, 2026

Copy link
Copy Markdown

AIGOV-596

@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Docs preview

Check 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
@johnstcn
johnstcn force-pushed the aigov-596-hostname-collision-warnings branch from 0441874 to b18a82e Compare August 18, 2026 13:05
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.

@coder-agents-review

This comment has been minimized.

coder-agents-review[bot]

This comment was marked as resolved.

- 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.

coder-agents-review[bot]

This comment was marked as resolved.

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.

coder-agents-review[bot]

This comment was marked as resolved.

- 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.

coder-agents-review[bot]

This comment was marked as resolved.

- 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.

coder-agents-review[bot]

This comment was marked as resolved.

- 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.

coder-agents-review[bot]

This comment was marked as resolved.

- 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.

coder-agents-review[bot]

This comment was marked as resolved.

- 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.

coder-agents-review[bot]

This comment was marked as resolved.

- 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.

coder-agents-review[bot]

This comment was marked as resolved.

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.

coder-agents-review[bot]

This comment was marked as resolved.

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.

coder-agents-review[bot]

This comment was marked as resolved.

This comment has been minimized.

coder-agents-review[bot]

This comment was marked as outdated.

- Inline winner := names[0] into the Sprintf call
- Remove default: continue from the Reload switch; falling
  through already continues the loop
@johnstcn
johnstcn marked this pull request as ready for review August 22, 2026 13:27
@johnstcn
johnstcn requested a review from ibetitsmike as a code owner August 22, 2026 13:27
Copilot AI lite review requested due to automatic review settings August 22, 2026 13:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.BaseURLHostname and 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 optional status carrying 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.

Comment thread docs/ai-coder/ai-gateway/monitoring.md Outdated
Comment on lines +73 to +76
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"`.
Comment on lines +118 to +134
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
@mtojek
mtojek requested a review from ssncferreira August 24, 2026 07:15

@ssncferreira ssncferreira left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we should assert the warning message here as well.

Comment on lines +1921 to +1923
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/...")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, this status table is only referencing the metrics, not the API, right?

Comment thread coderd/ai_providers.go
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),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.”

@ssncferreira
ssncferreira merged commit 0424216 into main Aug 24, 2026
31 checks passed
@ssncferreira
ssncferreira deleted the aigov-596-hostname-collision-warnings branch August 24, 2026 12:27
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 24, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants