fix: deprecate ai provider seeding env config#25854
Conversation
Docs preview📖 View docs preview for |
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.6.0 | Round 3 | Last posted: Round 3, 9 findings (1 P0, 1 P2, 4 P3, 2 P4, 1 Nit), COMMENT. Review Finding inventoryFinding Inventory - PR #25854Findings
Contested and acknowledged(none) Round logRound 1Netero-only. 1 P0. Reviewed against d0a51da..8496aad. Panel review blocked pending fix. Round 2BLOCKED. CRF-1 (P0) silent: author replied "Fixed" and resolved thread but no commit pushed. Head SHA unchanged. No review. Round 3Panel. CRF-1 addressed. 1 P2, 4 P3, 2 P4, 1 Nit new. Reviewed against d0a51da..8f5635b. 13-reviewer panel (Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Ging-Go, Ryosuke, Gon, Leorio, Chopper, Kite, Robin, Meruem). About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
First-pass review (Netero). One P0 finding: the xerrors.Errorf call in the mixed-prefix guard passes aiBridgeProviderEnvPrefix for all three format args, producing a nonsensical error message that names the same prefix twice and directs users to consolidate onto the deprecated prefix. TestReadAIProvidersFromEnv/MixedPrefixesAreNotAllowed fails on this commit.
This is a mechanical first-pass review only. The full review panel has not yet reviewed this PR and will review after this finding is addressed.
"I was a simple test with a simple assertion, and the code disrespected me." - Netero
🤖 This review was automatically generated with Coder Agents.
Signed-off-by: Danny Kopping <danny@coder.com>
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2 blocked. CRF-1 (P0) remains unresolved: the thread was resolved and the author replied "Fixed," but no commit has been pushed. Head SHA is unchanged from round 1. The bug at cli/server.go:3003 (all three Errorf args are aiBridgeProviderEnvPrefix) is still present in the code, and CI test failures confirm it.
Further review is blocked until the fix is pushed. The full review panel will run once the P0 is addressed.
🤖 This review was automatically generated with Coder Agents.
8496aad to
8f5635b
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Panel review (13 reviewers). CRF-1 (P0) verified fixed. Nice work on the constant extraction; it eliminates the class of copy-paste bugs, not just the instance.
The PR is well-scoped: only provider-seeding options are deprecated, infrastructure options (enabled, rate-limit, retention, circuit breaker) are correctly left alone. Test coverage is solid at 1:1.4 production-to-test ratio, with all four quadrants of the warning function exercised.
Severity count: 1 P2, 4 P3, 2 P4, 1 Nit.
The P2 is about erasing operational information from user-facing surfaces. The codebase already has a deprecation pattern (lines 4101-4246 in deployment.go) that prepends "Deprecated: use X instead." and keeps the original description. This PR replaces the entire description with identical text for all 10 options. The information loss matters because these options still function (they seed), so operators using them during migration have less guidance.
Process note: the commit type fix: signals a defect correction. Deprecating user-facing options is a deliberate behavior change, closer to feat: or chore:. The commit body has no explanation of why these options are being deprecated or what the migration path is. (Leorio)
"The log now reads: deprecation warning, typo warning, fatal error. The deprecation warning is premature noise for a config that never would have worked." - Hisoka
🤖 This review was automatically generated with Coder Agents.
| } | ||
|
|
||
| // AI Gateway options | ||
| aiGatewayProviderSeedingDeprecated := "Deprecated: manage AI Providers from the Coder UI or HTTP API. This option only seeds provider configuration at startup once-off, if configured." |
There was a problem hiding this comment.
P2 [CRF-2] All 10 deprecated option descriptions are replaced with identical text, erasing operational information from every user-facing surface (--help, YAML config, generated docs).
The codebase's existing deprecation pattern preserves the original description after the notice. For example, the older CODER_AIBRIDGE_* options at line ~4114:
"Deprecated: use --ai-gateway-openai-base-url or CODER_AI_GATEWAY_OPENAI_BASE_URL instead. The base URL of the OpenAI API."
This PR replaces the description entirely. Operational details like Bedrock base URL taking precedence over region, or Bedrock small-fast-model being for Claude Code background tasks, are gone from every surface an operator would consult. These options still function (they seed), so operators using them during migration lose their documentation.
Consistent approach: "Deprecated: manage AI Providers from the Coder UI or HTTP API. This option only seeds provider configuration at startup if set. <original description>" (Pariston P3, Leorio P2, Hisoka Note, Mafu-san Note, Ryosuke Nit)
🤖
| } | ||
|
|
||
| // AI Gateway options | ||
| aiGatewayProviderSeedingDeprecated := "Deprecated: manage AI Providers from the Coder UI or HTTP API. This option only seeds provider configuration at startup once-off, if configured." |
There was a problem hiding this comment.
P3 [CRF-3] Two wording issues in the deprecation string that propagates to all 10 options, --help, YAML, and docs.
-
"once-off" is not used anywhere else in the codebase. The established terms are "one-off" (cli/organizationsettings.go:107) and "one-time" (20+ uses). "once-off" is a regional variant that may confuse some operators.
-
"if configured" at the end has an ambiguous antecedent. It could mean "if this env var is set" or "if the provider is already configured." The intended meaning is the former; "if set" is unambiguous.
Suggested: "...This option only seeds provider configuration at startup one-off, if set." (Gon Nit, Leorio P3, Chopper Nit, Kite Nit)
🤖
| Group: &deploymentGroupAIGateway, | ||
| YAML: "enabled", | ||
| } | ||
| // The base URL of the OpenAI API. |
There was a problem hiding this comment.
P3 [CRF-4] Ten new comments above deprecated options mechanically copy the old Description text into Go comments. Seven of ten restate what the variable name and Option.Name already say (e.g., // The base URL of the OpenAI API. above aiGatewayOpenAIBaseURL with Name: "AI Gateway OpenAI Base URL"). Three carry genuine substance: Bedrock base URL's precedence over region, Bedrock region's URL format, and Claude Code's Haiku-class model usage.
Per AGENTS.md: "Comments MUST be substantive and concise."
This finding interacts with CRF-2: if the original descriptions are restored in the Description field (matching the codebase convention), these comments become fully redundant and should be deleted. If CRF-2 is not accepted, the three substantive comments should be kept (trimmed to their non-obvious content) and the seven that restate the identifier should be removed. (Gon P2, Leorio Nit, Ryosuke Nit)
🤖
| } | ||
|
|
||
| logger.Warn(ctx, | ||
| "ai provider environment variables are deprecated for provider management and only seed provider rows at startup", |
There was a problem hiding this comment.
P3 [CRF-5] The log message says "provider rows", which is database implementation language. The Description field in deployment.go uses "provider configuration" for the same concept. Two messages about the same deprecation use different terms, making them harder to connect.
An operator at 2 AM sees "provider rows" in logs and has to mentally map it to the "provider configuration" they saw in --help. Fix: change "provider rows" to "provider configuration" to match the Description wording. (Leorio)
🤖
| ) | ||
| } | ||
|
|
||
| func aiProviderEnvPrefix(environ []string) string { |
There was a problem hiding this comment.
P4 [CRF-6] aiProviderEnvPrefix re-derives which prefix is active by scanning environ, but the caller (ReadAIProvidersFromEnv) already knows: providers came from aiBridgeProviderEnvPrefix, gatewayProviders came from aiGatewayProviderEnvPrefix. After the append at line 3005, the distinction is lost, then warnIfAIProvidersConfiguredFromEnv re-discovers it.
The two derivations also use different algorithms: one parses indexed env vars via serpent.ParseEnviron, the other does a raw prefix scan. They agree in all realistic scenarios, but the coupling is unnecessary. Passing the prefix string from the caller would eliminate aiProviderEnvPrefix entirely. Not urgent given the deprecation context. (Robin P3, Meruem Note)
🤖
| return nil, xerrors.Errorf("cannot mix %s* and %s* environment variables, please consolidate onto %s*", aiBridgeProviderEnvPrefix, aiGatewayProviderEnvPrefix, aiGatewayProviderEnvPrefix) | ||
| } | ||
| providers = append(providers, gatewayProviders...) | ||
| warnIfAIProvidersConfiguredFromEnv(context.Background(), logger, environ, providers) |
There was a problem hiding this comment.
P4 [CRF-7] warnIfAIProvidersConfiguredFromEnv is called before the post-parse validation loop starting at line 3008. If an operator sets CODER_AI_GATEWAY_PROVIDER_0_HELLO=world (unknown field, no TYPE), readAIProvidersForPrefix creates an empty provider struct, the deprecation warning fires, then validation fails with "provider 0: TYPE is required." The deprecation warning is premature noise for a config that would never have worked.
Moving the call after the validation loop (between line 3086 and the return) would warn only on configs that survive validation. (Hisoka)
🤖
| }) | ||
| require.Len(t, entries, 1) | ||
| require.Len(t, entries[0].Fields, 2) | ||
| assert.Equal(t, aiGatewayProviderEnvPrefix, entries[0].Fields[0].Value) |
There was a problem hiding this comment.
P3 [CRF-8] The test asserts log fields by positional index (entries[0].Fields[0].Value, entries[0].Fields[1].Value), coupling to the slog.F call order in warnIfAIProvidersConfiguredFromEnv. If the two slog.F fields are reordered, the test fails with a misleading "wrong value" error.
The blast radius is small (the function is 5 lines and the test is in the same package), but a field-by-name lookup would be more resilient. require.Len(t, entries[0].Fields, 2) already catches structural expansion. (Kite)
🤖
| "CODER_AI_GATEWAY_PROVIDER_0_TYPE=openai", | ||
| }, nil) | ||
|
|
||
| require.Empty(t, sink.Entries(nil)) |
There was a problem hiding this comment.
Nit [CRF-9] sink.Entries(nil) passes a nil filter, which is skipped internally. The positive-path subtests pass a real filter function. Using sink.Entries() (no args) in the negative-path subtests would make the "no filter, give me everything" intent clearer. (Bisky)
🤖
No description provided.