refactor(providers): unify name separator and refresh docs#324
Conversation
Oracle, Azure, and Bedrock provider guides assumed Go was installed locally (go run ./cmd/gomodel), which kept first-time integrators out. Each "Start GoModel" step now offers three tabs — Docker with --env-file .env (recommended), Docker with inline -e flags, and a binary built via make build — so users can pick whichever fits their environment without leaving the page. The Prometheus metrics guide had the mirror problem: every snippet ran ./bin/gomodel without explaining where that binary comes from. It now leads with the same three-tab CodeGroup, and subsequent option/troubleshooting blocks show just the variable being set and link back to the Quick Start for the launch form. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Every page in docs/providers/ now follows the same shape: - one-line intro (drop trivial `Flow:` ASCII paths) - Configure: env vars in one .env-style block, optional YAML alt - Run GoModel: three-tab CodeGroup (Docker .env / Docker -e / binary) - Verify: single section, single curl, short note on what else works - Advanced (multi-instance, special endpoints, mode toggles) below - Troubleshooting and References preserved Cuts applied across pages: `Before you start` preambles that just restated later content, prose that re-described what the adjacent code block already showed, the .env <Tip> repeated on three pages (now lives once in overview), and verbose `Current status` lists kept only where they document a real limitation. The 1227-line provider section drops to 822 (~-33%) with no factual content removed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Vertex was the only provider (besides Bedrock) that set `Discovery.NameSeparator: "_"`, registering `vertex_us` and `vertex_eu` from suffixed env vars while every other provider produced `name-suffix`. The override was undocumented, no reviewer questioned it on the original Vertex PR, and Google Cloud's own RFC-1034 / AIP-122 naming convention prefers hyphens. Drop the override so `VERTEX_US_*` registers `vertex-us` like `OPENAI_EAST_*` registers `openai-east`. Test, README, and provider/advanced docs updated. Tests pass. BREAKING CHANGE: operators with `VERTEX_<SUFFIX>_*` env vars or config keyed on `vertex_us` / `vertex_eu` will need to switch to `vertex-us` / `vertex-eu` (provider name only — the env-var names themselves are unchanged). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bedrock was the last provider carrying the legacy `NameSeparator: "_"` override (likely copy-pasted from the Vertex registration added one day earlier). Same situation as Vertex: the override was undocumented, no reviewer flagged it on PR #316, and AWS resource IDs follow RFC-1034 hyphens — Bedrock model IDs use dots and hyphens, never underscores. Drop the override so `BEDROCK_<SUFFIX>_*` env vars register `bedrock-<suffix>` like every other provider. No tests or docs reference `bedrock_<suffix>` form today, so this only affects operators who were relying on the previously-undocumented underscored shape. BREAKING CHANGE: operators with `BEDROCK_<SUFFIX>_*` env vars or config keyed on `bedrock_<suffix>` will need to switch to `bedrock-<suffix>` (env-var names themselves are unchanged). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous Vertex-specific wording in README.md, docs/advanced/config-yaml.mdx, and docs/advanced/configuration.mdx singled Vertex out as the exception that "keeps underscores in the provider name". After unifying Vertex and Bedrock on the default `-` separator, there is no exception anymore: every provider follows `<PROVIDER>_<SUFFIX>_*` → `<provider>-<suffix>`. Drop the special-case framing and keep only the actual Vertex requirements (VERTEX_PROJECT + VERTEX_LOCATION, VERTEX_AUTH_TYPE default). The general rule documented earlier in each file now correctly covers Vertex and Bedrock. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughStandardizes provider discovery by removing custom underscore NameSeparator for Vertex and Bedrock so suffixed env vars normalize to hyphenated provider names (e.g., VERTEX_US → vertex-us), updates discovery tests, and restructures many provider and guide docs into consistent Configure / Run GoModel / Verify flows. ChangesProvider Configuration and Documentation Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR removes the
Confidence Score: 4/5Safe to merge; the Go changes are minimal two-line removals whose correctness is confirmed by an updated test on the vertex side and by the live smoke test described in the PR. The bedrock naming change is symmetric with vertex but has no dedicated test — the behavior of internal/providers/config_test.go (missing bedrock suffix test) and docs/providers/bedrock.mdx (no multi-instance naming example) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Env var VERTEX_US_PROJECT\nBEDROCK_US_BASE_URL"] --> B{"DiscoveryConfig\nNameSeparator"}
B -- "Before PR\nNameSeparator = '_'" --> C["Provider name:\nvertex_us / bedrock_us"]
B -- "After PR\nNameSeparator = '' (default)" --> D["Provider name:\nvertex-us / bedrock-us"]
D --> E["Matches all other providers\ne.g. openai-east, ollama-a, vllm-test"]
C --> F["Was inconsistent\nwith general suffix rule"]
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/providers/azure.mdx (1)
57-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerification request is incomplete when master-key auth is enabled.
Because setup uses
GOMODEL_MASTER_KEY, the verify curl needs a bearer token header.Suggested fix
curl -s http://localhost:8080/v1/chat/completions \ + -H "Authorization: Bearer change-me" \ -H "Content-Type: application/json" \ -d '{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/providers/azure.mdx` around lines 57 - 63, The verification curl example in docs/providers/azure.mdx is missing the Authorization header required when master-key auth is enabled; update the curl command used for the verify flow to include a Bearer token header using the GOMODEL_MASTER_KEY (e.g., add -H "Authorization: Bearer $GOMODEL_MASTER_KEY") so the request authenticates against the server when GOMODEL_MASTER_KEY is set.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/advanced/config-yaml.mdx`:
- Around line 28-30: The wording about Vertex's required environment variables
is ambiguous; update the text to tie required vars to the same suffix rather
than globals — clarify that for a suffixed provider like VERTEX_US_PROJECT
(which registers as vertex-us) you must also provide the matching suffixed
LOCATION var (e.g., VERTEX_US_LOCATION) and any other suffixed settings, rather
than the generic VERTEX_PROJECT or VERTEX_LOCATION; keep the note that
VERTEX_AUTH_TYPE defaults to gcp_adc. Use the existing example names
VERTEX_US_PROJECT and add VERTEX_US_LOCATION to show the intended pattern.
In `@docs/guides/prometheus-metrics.mdx`:
- Around line 80-82: Add the missing flag so the snippet is runnable: update the
quick snippet that currently only sets METRICS_ENDPOINT to also set
METRICS_ENABLED=true (or add an explicit prerequisite line) so the example
enables metrics before specifying METRICS_ENDPOINT; look for the snippet
containing METRICS_ENDPOINT=/internal/prometheus and prepend or include
METRICS_ENABLED=true next to it.
In `@docs/providers/bedrock.mdx`:
- Around line 56-63: The curl example in the Bedrock docs omits the required
Authorization header even though the page configures GOMODEL_MASTER_KEY; update
the example curl (the snippet that posts to /v1/chat/completions) to include the
Authorization header using the GOMODEL_MASTER_KEY (e.g., add an Authorization:
Bearer $GOMODEL_MASTER_KEY header) so the verification request succeeds with the
documented setup.
In `@docs/providers/deepseek.mdx`:
- Around line 44-46: The sentence "To skip reasoning entirely, omit the
`reasoning` field; don't pass `low`." is misleading because the mapping converts
`"reasoning": {"effort":"low"}` into DeepSeek's top-level `reasoning_effort` and
`low` maps to `high` (which enables reasoning). Update the text around the
GoModel/`reasoning` mapping to explicitly state that omitting `reasoning`
disables reasoning, whereas providing `reasoning.effort` — including `low` —
will be mapped and will enable reasoning (note that `low` maps to `high`);
replace "don't pass `low`" with a clear instruction like "omit the `reasoning`
field to disable reasoning; any provided `reasoning.effort` (including `low`)
will be mapped and enable reasoning."
- Around line 36-42: The docs mapping for the reasoning_effort parameter is
incorrect: DeepSeek V4 only accepts "high" and "max", so "anything else → passed
through" is misleading and will cause API errors; update the docs table to
either (A) state that unknown client values will be rejected by DeepSeek (and
recommend mapping/normalizing them to "high" or "max"), or (B) show that the
client must validate/normalize the input (e.g., via a normalizeReasoningEffort
or mapReasoningEffort routine) and reject invalid values before calling the API;
ensure the table and any descriptive text mention that only "high" and "max" are
permitted and that other values will fail.
In `@docs/providers/oracle.mdx`:
- Around line 77-83: The example curl request in the docs omits the required
Authorization header despite the environment key being set; update the curl
example that posts to /v1/chat/completions to include the Authorization header
(e.g., add -H "Authorization: Bearer change-me") so the request uses the
GOMODEL_MASTER_KEY; ensure the header appears alongside the existing
Content-Type header in the sample.
In `@internal/providers/config_test.go`:
- Around line 551-562: Add a parallel assertion in
TestApplyProviderEnvVars_DiscoversSuffixedVertexProvider to also verify Bedrock
suffixed discovery: set the analogous BEDROCK_US_* env vars (e.g.,
BEDROCK_US_PROJECT, BEDROCK_US_LOCATION, BEDROCK_US_AUTH_TYPE,
BEDROCK_US_SERVICE_ACCOUNT_FILE) before calling applyProviderEnvVars and then
assert that got["bedrock-us"] exists (mirroring the existing vertex-us check) so
the test covers the Bedrock separatator/name path as well.
In `@README.md`:
- Around line 128-131: Reword the README sentence about Vertex AI to clarify
that project and location env vars must match the instance prefix: for a
suffixed instance like VERTEX_US_PROJECT you must also set the corresponding
VERTEX_US_LOCATION (and similarly for any other suffix), rather than implying
the unsuffixed VERTEX_PROJECT/VERTEX_LOCATION are always required; retain
mention that VERTEX_US_PROJECT registers provider `vertex-us` and that
VERTEX_AUTH_TYPE defaults to `gcp_adc`.
---
Outside diff comments:
In `@docs/providers/azure.mdx`:
- Around line 57-63: The verification curl example in docs/providers/azure.mdx
is missing the Authorization header required when master-key auth is enabled;
update the curl command used for the verify flow to include a Bearer token
header using the GOMODEL_MASTER_KEY (e.g., add -H "Authorization: Bearer
$GOMODEL_MASTER_KEY") so the request authenticates against the server when
GOMODEL_MASTER_KEY is set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b232491c-14c8-4868-aa88-bcadcc70144a
📒 Files selected for processing (16)
README.mddocs/advanced/config-yaml.mdxdocs/advanced/configuration.mdxdocs/guides/prometheus-metrics.mdxdocs/providers/azure.mdxdocs/providers/bedrock.mdxdocs/providers/deepseek.mdxdocs/providers/gemini.mdxdocs/providers/multiple-ollama.mdxdocs/providers/oracle.mdxdocs/providers/overview.mdxdocs/providers/vertex.mdxdocs/providers/vllm.mdxinternal/providers/bedrock/bedrock.gointernal/providers/config_test.gointernal/providers/vertex/vertex.go
💤 Files with no reviewable changes (2)
- internal/providers/vertex/vertex.go
- internal/providers/bedrock/bedrock.go
…time status (#326) When CONFIGURED_PROVIDER_MODELS_MODE=allowlist applies a configured model list and intentionally skips the upstream `/models` call, the registry left LastModelFetchSuccessAt unset. The admin status classifier interprets that combination (Registered + DiscoveredModelCount>0 + no fetch error + no success timestamp) as "still serving cached inventory while live refresh finishes" and reports `status: degraded, label: Starting`. The result was a fully functional allowlist-mode provider — serving real traffic against AWS Bedrock during smoke-testing of #324 — appearing unhealthy on dashboards and to any health-keyed monitoring. Set lastModelFetchSuccessAt for the allowlist-applied case too, because in that mode the allowlist IS the authoritative inventory: there is no pending refresh to wait for. Upstream-failure fallbacks (configured_models_upstream_ error/nil/empty) still leave SuccessAt unset, so health correctly surfaces "live refresh failed, serving configured fallback" for that distinct scenario. Tests: - Existing TestModelRegistry/ConfiguredModelsAllowlistModeSkipsUpstreamAndUses ConfiguredModels updated: it now asserts LastModelFetchSuccessAt is set, UsingCachedModels is false, and DiscoveredModelCount reflects the allowlist. The previous nil assertion was codifying the bug. - New TestClassifyProviderStatus_HealthyForAllowlistInventory in internal/admin/ pins the end-to-end classifier outcome: an allowlist provider with one model and a SuccessAt timestamp is healthy, not degraded. - All other registry / admin tests pass under -race. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
go run ./cmd/gomodeland bare./bin/gomodelsnippets with a three-tab<CodeGroup>(Docker.env, Docker-e, binary viamake build) on the Oracle, Azure, Bedrock provider pages and the Prometheus metrics guide.docs/providers/*.mdx(1227 → 822 lines, ~-33%) onto a single value-first shape: one-line intro → Configure → Run GoModel → Verify → advanced bits → Troubleshooting → References. No factual content removed.Discovery.NameSeparator: "_"override from Vertex and Bedrock soVERTEX_<SUFFIX>_*/BEDROCK_<SUFFIX>_*registervertex-<suffix>/bedrock-<suffix>, matching every other provider and Google/AWS's own RFC-1034 conventions.BREAKING CHANGE
Operators with
VERTEX_<SUFFIX>_*orBEDROCK_<SUFFIX>_*env vars (or config keyed onvertex_us/bedrock_us) need to switch the provider-name references tovertex-<suffix>/bedrock-<suffix>. Env-var names themselves are unchanged.Why
The
_override was introduced in PR #302 (Vertex) and copy-pasted into PR #316 (Bedrock) one day later. Neither PR justifies the choice in body or inline comments, no code comment explains it, and the AWS/GCP naming conventions both prefer hyphens. Removing the override removes a special case from the codebase, aligns the documentation with reality (the general "underscores become hyphens" rule), and matches what every other provider already does.Test plan
make test-race,make lint,go mod tidy,mint validate— all green via pre-commit hooks on each commit.provider registered name=bedrock-us type=bedrockandname=vertex-us type=vertex(post-fix; pre-fix wasbedrock_us/vertex_us)./providers/statuslists both withregistered: true, separate fetch timestamps./v1/modelsreturnsbedrock/us.amazon.nova-lite-v1:0andbedrock-us/us.amazon.nova-lite-v1:0.Follow-up worth a separate issue/PR
internal/providers/googleauth/googleauth.gousesgoogle.DefaultTokenSource, which doesn't readquota_project_idfromapplication_default_credentials.json. As a result, any ADC-based Vertex deployment hitsauthentication_error: ... API requires a quota projectonaiplatform.googleapis.com. The fix is to switch togoogle.FindDefaultCredentialsand propagate the project as theX-Goog-User-Projectheader on outbound Vertex requests. Unrelated to this PR; surfaced during smoke testing.🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Bug Fixes
Clarification