fix: deprecate ai provider seeding env config - #25854
Conversation
Docs preview📖 View docs preview for |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
/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)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
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.
|
/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.
Documentation CheckUpdates Needed
No Changes Needed
Automated review via Coder Agents |
|
Docs will be updated upstack. |
bc492a7 to
5af0214
Compare
5af0214 to
70e8be7
Compare
Signed-off-by: Danny Kopping <danny@coder.com>
- CRF-2: Restore original descriptions after deprecation notice prefix - CRF-3: Change 'once-off' to 'one-off' and 'if configured' to 'if set' - CRF-4: Remove redundant Go comments (descriptions now in Description field) - CRF-5: Change 'provider rows' to 'provider configuration' in log message - CRF-6: Pass env prefix from caller instead of re-deriving it - CRF-7: Move deprecation warning after validation loop - CRF-8: Use field-by-name lookup in tests instead of positional index - CRF-9: Remove nil arg from sink.Entries() calls 🤖 Generated by Coder Agents
70e8be7 to
1e0ff3a
Compare
Signed-off-by: Danny Kopping <danny@coder.com>
1e0ff3a to
72fc38b
Compare
Merge activity
|
|
Cherry-pick PR created: #25900 |
Cherry-pick of #25854 Original PR: #25854 — fix: deprecate ai provider seeding env config Merge commit: c8555e2 Requested by: @dannykopping Co-authored-by: Danny Kopping <danny@coder.com>
2 participants
Environment variables used to configure AI Gateway providers are now deprecated, and we need to reflect this as such.