feat: implemented ollama provider#47
Merged
SantiagoDePolonia merged 3 commits intomainfrom Jan 25, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds an Ollama provider: config and .env entries, provider registration in main, a new AvailabilityChecker interface and pre-registration availability check, and a full Ollama provider implementation (chat, streaming, models) with tests. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Factory as Provider Factory
participant Provider as Ollama Provider
participant Backend as Ollama Backend
App->>Factory: factory.Register(ollama.Registration)
Factory->>Provider: New(apiKey, hooks)
Provider-->>Factory: Provider instance
Note over Factory,Provider: Availability check before final registration
Factory->>Provider: CheckAvailability(ctx, 5s)
Provider->>Backend: GET /models (timeout 5s)
alt backend responds
Backend-->>Provider: model list (200)
Provider-->>Factory: nil (available)
Factory->>Factory: register provider
else timeout/error
Backend-->>Provider: error/timeout
Provider-->>Factory: error (unavailable)
Factory->>Factory: skip registration (log)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @.env.template:
- Around line 119-122: Add an optional OLLAMA_API_KEY entry to the .env.template
to mirror the optional key exposed in config/config.yaml; update the Ollama
block near the OLLAMA_BASE_URL comment to include a commented example line
(e.g., OLLAMA_API_KEY=your_api_key_here) and a short note that the key is
optional and only needed when Ollama is behind an auth proxy so users are not
confused.
In `@cmd/gomodel/main.go`:
- Around line 21-22: Switch the named imports for the provider packages to blank
imports (use _ "gomodel/internal/providers/ollama" and _
"gomodel/internal/providers/openai") and remove the explicit provider
registration calls in main.go (e.g., any ollama.Register(...) or
openai.Register(...) invocation) so provider init() functions handle
registration per project policy; also verify other providers follow the same
blank-import pattern.
In `@config/config.go`:
- Around line 341-348: The provider pruning logic currently removes entries with
empty APIKey, which strips the intentionally keyless Ollama config; update the
removeEmptyProviders function to preserve providers whose ProviderConfig.Type ==
"ollama" when ProviderConfig.BaseURL is non-empty (i.e., do not delete entries
that are type "ollama" and have a resolved BaseURL), leaving all other
empty-APIKey providers unchanged; reference ProviderConfig and
removeEmptyProviders so you locate and adjust the deletion condition
accordingly.
In `@config/config.yaml`:
- Around line 69-72: Update the top-level note that currently says “at least one
provider with a valid API key is required” to explicitly call out the Ollama
exception: clarify that the requirement applies to cloud/remote providers that
use API keys, but the ollama provider (the "ollama" block using base_url and
api_key) can operate without an API key when running a local Ollama instance;
reference the "ollama" config block and ensure the note explains that a local
Ollama instance without an API key still satisfies running the system but may
not count as a cloud provider for external features.
In `@internal/providers/init.go`:
- Around line 14-15: The availability probe currently calls registerProviders
using context.Background(), so cancellations from the caller don't stop the
probe; change the call sites (including Init/registerProviders and the other
call around the block referenced 203-215) to pass the incoming caller context
instead of context.Background(), and inside registerProviders derive a
timeout/context with context.WithTimeout(ctx, timeout) from that passed context
(so cancellations propagate) rather than creating a fresh Background context;
update function signatures if needed (registerProviders(ctx context.Context,
...)) and ensure callers forward their ctx.
In `@internal/providers/ollama/ollama.go`:
- Around line 227-239: The Responses method must guard against streaming
requests: in Provider.Responses, check req.Stream on the incoming
*core.ResponsesRequest and if true return a clear error (e.g., instructing
callers to use StreamResponses) instead of routing to ChatCompletion; do not
call ChatCompletion when req.Stream is true. Reference: Provider.Responses,
core.ResponsesRequest (req.Stream), ChatCompletion, and StreamResponses.
- Around line 124-188: The code in convertResponsesRequestToChat and
extractContentFromInput relies on untyped interface{} and map[string]interface{}
parsing which must be replaced with strongly-typed payloads; change the
ResponsesRequest.Input to a typed form (e.g., []core.Message or a new
core.ResponsesInputMessage struct) and update convertResponsesRequestToChat to
accept and iterate that typed slice instead of doing runtime type assertions,
and remove extractContentFromInput or rewrite it to operate on the typed content
field(s) (e.g., Content string or []core.ContentPart) so all parsing is static
and uses fields like Message.Role and Message.Content rather than map lookups.
Ensure you update any callers to construct the new typed input structs and
adjust MaxTokens/system instruction handling remains the same in
convertResponsesRequestToChat.
Comment on lines
+119
to
+122
| # Ollama (local LLM server) | ||
| # Note: Ollama doesn't require an API key | ||
| # Set base URL to enable (default: http://localhost:11434/v1) | ||
| # OLLAMA_BASE_URL=http://localhost:11434/v1 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting OLLAMA_API_KEY for parity.
config/config.yaml exposes an optional OLLAMA_API_KEY; adding it here reduces confusion for users running Ollama behind an auth proxy.
💡 Suggested tweak
# OLLAMA_BASE_URL=http://localhost:11434/v1
+# OLLAMA_API_KEY=📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Ollama (local LLM server) | |
| # Note: Ollama doesn't require an API key | |
| # Set base URL to enable (default: http://localhost:11434/v1) | |
| # OLLAMA_BASE_URL=http://localhost:11434/v1 | |
| # Ollama (local LLM server) | |
| # Note: Ollama doesn't require an API key | |
| # Set base URL to enable (default: http://localhost:11434/v1) | |
| # OLLAMA_BASE_URL=http://localhost:11434/v1 | |
| # OLLAMA_API_KEY= |
🤖 Prompt for AI Agents
In @.env.template around lines 119 - 122, Add an optional OLLAMA_API_KEY entry
to the .env.template to mirror the optional key exposed in config/config.yaml;
update the Ollama block near the OLLAMA_BASE_URL comment to include a commented
example line (e.g., OLLAMA_API_KEY=your_api_key_here) and a short note that the
key is optional and only needed when Ollama is behind an auth proxy so users are
not confused.
Comment on lines
+21
to
22
| "gomodel/internal/providers/ollama" | ||
| "gomodel/internal/providers/openai" |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Align Ollama import/registration with blank‑import provider policy.
This change adds a named import and explicit registration, but the project guideline requires blank imports in cmd/gomodel/main.go to trigger provider init() registration. Consider switching to blank imports and removing explicit registration (and aligning other providers accordingly).
📌 Suggested direction
- "gomodel/internal/providers/ollama"
+ _ "gomodel/internal/providers/ollama"- factory.Register(ollama.Registration)Also applies to: 74-74
🤖 Prompt for AI Agents
In `@cmd/gomodel/main.go` around lines 21 - 22, Switch the named imports for the
provider packages to blank imports (use _ "gomodel/internal/providers/ollama"
and _ "gomodel/internal/providers/openai") and remove the explicit provider
registration calls in main.go (e.g., any ollama.Register(...) or
openai.Register(...) invocation) so provider init() functions handle
registration per project policy; also verify other providers follow the same
blank-import pattern.
Comment on lines
+69
to
+72
| ollama: | ||
| type: "ollama" | ||
| base_url: "${OLLAMA_BASE_URL:-http://localhost:11434/v1}" | ||
| api_key: "${OLLAMA_API_KEY:-}" # Optional, not required by Ollama |
Contributor
There was a problem hiding this comment.
Clarify the no‑API‑key exception for Ollama.
The header note states “at least one provider with a valid API key is required,” but the Ollama block documents a no‑key provider. Please update the note to avoid confusion.
✏️ Suggested doc fix
-# - At least one provider with a valid API key is required
+# - At least one provider must be configured; some providers (e.g., Ollama) do not require an API key🤖 Prompt for AI Agents
In `@config/config.yaml` around lines 69 - 72, Update the top-level note that
currently says “at least one provider with a valid API key is required” to
explicitly call out the Ollama exception: clarify that the requirement applies
to cloud/remote providers that use API keys, but the ollama provider (the
"ollama" block using base_url and api_key) can operate without an API key when
running a local Ollama instance; reference the "ollama" config block and ensure
the note explains that a local Ollama instance without an API key still
satisfies running the system but may not count as a cloud provider for external
features.
Comment on lines
+124
to
+188
| // convertResponsesRequestToChat converts a ResponsesRequest to ChatRequest for Ollama | ||
| func convertResponsesRequestToChat(req *core.ResponsesRequest) *core.ChatRequest { | ||
| chatReq := &core.ChatRequest{ | ||
| Model: req.Model, | ||
| Messages: make([]core.Message, 0), | ||
| Temperature: req.Temperature, | ||
| Stream: req.Stream, | ||
| } | ||
|
|
||
| if req.MaxOutputTokens != nil { | ||
| chatReq.MaxTokens = req.MaxOutputTokens | ||
| } | ||
|
|
||
| // Add system instruction if provided | ||
| if req.Instructions != "" { | ||
| chatReq.Messages = append(chatReq.Messages, core.Message{ | ||
| Role: "system", | ||
| Content: req.Instructions, | ||
| }) | ||
| } | ||
|
|
||
| // Convert input to messages | ||
| switch input := req.Input.(type) { | ||
| case string: | ||
| chatReq.Messages = append(chatReq.Messages, core.Message{ | ||
| Role: "user", | ||
| Content: input, | ||
| }) | ||
| case []interface{}: | ||
| for _, item := range input { | ||
| if msgMap, ok := item.(map[string]interface{}); ok { | ||
| role, _ := msgMap["role"].(string) | ||
| content := extractContentFromInput(msgMap["content"]) | ||
| if role != "" && content != "" { | ||
| chatReq.Messages = append(chatReq.Messages, core.Message{ | ||
| Role: role, | ||
| Content: content, | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return chatReq | ||
| } | ||
|
|
||
| // extractContentFromInput extracts text content from responses input | ||
| func extractContentFromInput(content interface{}) string { | ||
| switch c := content.(type) { | ||
| case string: | ||
| return c | ||
| case []interface{}: | ||
| // Array of content parts - extract text | ||
| var texts []string | ||
| for _, part := range c { | ||
| if partMap, ok := part.(map[string]interface{}); ok { | ||
| if text, ok := partMap["text"].(string); ok { | ||
| texts = append(texts, text) | ||
| } | ||
| } | ||
| } | ||
| return strings.Join(texts, " ") | ||
| } | ||
| return "" | ||
| } |
Contributor
There was a problem hiding this comment.
Replace interface{}/map[string]interface{} parsing with typed input structs.
convertResponsesRequestToChat / extractContentFromInput currently do per-request type assertions on untyped maps/slices, which violates the internal payload-typing guideline and pushes type errors to runtime. Prefer strongly-typed input message/content structs and accept typed slices (e.g., []core.Message or a dedicated core.ResponsesInputMessage), then convert without maps.
Based on learnings, please avoid interface{} / map[string]interface{} for payload types.
🤖 Prompt for AI Agents
In `@internal/providers/ollama/ollama.go` around lines 124 - 188, The code in
convertResponsesRequestToChat and extractContentFromInput relies on untyped
interface{} and map[string]interface{} parsing which must be replaced with
strongly-typed payloads; change the ResponsesRequest.Input to a typed form
(e.g., []core.Message or a new core.ResponsesInputMessage struct) and update
convertResponsesRequestToChat to accept and iterate that typed slice instead of
doing runtime type assertions, and remove extractContentFromInput or rewrite it
to operate on the typed content field(s) (e.g., Content string or
[]core.ContentPart) so all parsing is static and uses fields like Message.Role
and Message.Content rather than map lookups. Ensure you update any callers to
construct the new typed input structs and adjust MaxTokens/system instruction
handling remains the same in convertResponsesRequestToChat.
Comment on lines
+227
to
+239
| // Responses sends a Responses API request to Ollama (converted to chat format) | ||
| func (p *Provider) Responses(ctx context.Context, req *core.ResponsesRequest) (*core.ResponsesResponse, error) { | ||
| // Convert ResponsesRequest to ChatRequest | ||
| chatReq := convertResponsesRequestToChat(req) | ||
|
|
||
| // Use the existing ChatCompletion method | ||
| chatResp, err := p.ChatCompletion(ctx, chatReq) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return convertChatResponseToResponses(chatResp), nil | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against stream=true in Responses().
If req.Stream is true, the call will hit the non-streaming ChatCompletion path and likely fail decoding SSE. Reject stream requests here (or force Stream=false) and direct callers to StreamResponses.
🐛 Proposed fix
import (
"context"
+ "errors"
"io"
"net/http"
@@
func (p *Provider) Responses(ctx context.Context, req *core.ResponsesRequest) (*core.ResponsesResponse, error) {
+ if req.Stream {
+ return nil, errors.New("use StreamResponses when stream=true")
+ }
// Convert ResponsesRequest to ChatRequest
chatReq := convertResponsesRequestToChat(req)
+ chatReq.Stream = false
@@
}🤖 Prompt for AI Agents
In `@internal/providers/ollama/ollama.go` around lines 227 - 239, The Responses
method must guard against streaming requests: in Provider.Responses, check
req.Stream on the incoming *core.ResponsesRequest and if true return a clear
error (e.g., instructing callers to use StreamResponses) instead of routing to
ChatCompletion; do not call ChatCompletion when req.Stream is true. Reference:
Provider.Responses, core.ResponsesRequest (req.Stream), ChatCompletion, and
StreamResponses.
Update removeEmptyProviders to keep Ollama providers that have a non-empty BaseURL, even without an API key. This prevents the intentionally keyless Ollama configuration from being stripped. Also update TestLoad_EmptyAPIKey to expect ollama to be present since it has a default base_url in config.yaml. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@config/config.go`:
- Around line 341-348: The current Ollama provider entry creates a keyless
ProviderConfig (Type "ollama") which allows an env-only startup without any
provider API key; after the provider collection is assembled (where
cfg.Providers is populated) add a validation step that iterates cfg.Providers
and ensures at least one ProviderConfig has a non-empty APIKey (or otherwise a
provider that is explicitly allowed to be keyless), and return an error from the
config loader/init function if none are found so startup fails; update the
relevant function in config.go that builds cfg (check where cfg.Providers is
populated) to perform this check and return a clear error message when no API
key is present.
- Around line 493-497: The Ollama provider branch in removeEmptyProviders
currently keeps entries when pCfg.Type == "ollama" and pCfg.BaseURL != "";
update this to also reject unresolved env placeholders by treating values like
"${...}" as empty: when handling pCfg.Type == "ollama" check that pCfg.BaseURL
is non-empty and does not contain an unresolved placeholder (e.g., starts with
"${" and ends with "}" or matches the placeholder regex) before adding to
filteredProviders[name] so unresolved ${OLLAMA_BASE_URL} entries are filtered
out just like missing API keys.
Comment on lines
+341
to
+348
| // Ollama (no API key required, enabled via base URL) | ||
| if baseURL := viper.GetString("OLLAMA_BASE_URL"); baseURL != "" { | ||
| cfg.Providers["ollama"] = ProviderConfig{ | ||
| Type: "ollama", | ||
| APIKey: "", // Not required | ||
| BaseURL: baseURL, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Enforce the API‑key requirement for env‑only config.
Adding a keyless Ollama provider here allows an environment‑only setup to pass without any API‑keyed provider, which violates the requirement to have at least one provider API key before startup. Consider validating after provider collection and returning an error when no API key is present.
🛠️ Suggested guard
// Ollama (no API key required, enabled via base URL)
if baseURL := viper.GetString("OLLAMA_BASE_URL"); baseURL != "" {
cfg.Providers["ollama"] = ProviderConfig{
Type: "ollama",
APIKey: "", // Not required
BaseURL: baseURL,
}
}
+ hasAPIKey := false
+ for _, pCfg := range cfg.Providers {
+ if pCfg.APIKey != "" && !strings.Contains(pCfg.APIKey, "${") {
+ hasAPIKey = true
+ break
+ }
+ }
+ if !hasAPIKey {
+ return nil, fmt.Errorf("at least one provider API key is required when using environment configuration")
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Ollama (no API key required, enabled via base URL) | |
| if baseURL := viper.GetString("OLLAMA_BASE_URL"); baseURL != "" { | |
| cfg.Providers["ollama"] = ProviderConfig{ | |
| Type: "ollama", | |
| APIKey: "", // Not required | |
| BaseURL: baseURL, | |
| } | |
| } | |
| // Ollama (no API key required, enabled via base URL) | |
| if baseURL := viper.GetString("OLLAMA_BASE_URL"); baseURL != "" { | |
| cfg.Providers["ollama"] = ProviderConfig{ | |
| Type: "ollama", | |
| APIKey: "", // Not required | |
| BaseURL: baseURL, | |
| } | |
| } | |
| hasAPIKey := false | |
| for _, pCfg := range cfg.Providers { | |
| if pCfg.APIKey != "" && !strings.Contains(pCfg.APIKey, "${") { | |
| hasAPIKey = true | |
| break | |
| } | |
| } | |
| if !hasAPIKey { | |
| return nil, fmt.Errorf("at least one provider API key is required when using environment configuration") | |
| } |
🤖 Prompt for AI Agents
In `@config/config.go` around lines 341 - 348, The current Ollama provider entry
creates a keyless ProviderConfig (Type "ollama") which allows an env-only
startup without any provider API key; after the provider collection is assembled
(where cfg.Providers is populated) add a validation step that iterates
cfg.Providers and ensures at least one ProviderConfig has a non-empty APIKey (or
otherwise a provider that is explicitly allowed to be keyless), and return an
error from the config loader/init function if none are found so startup fails;
update the relevant function in config.go that builds cfg (check where
cfg.Providers is populated) to perform this check and return a clear error
message when no API key is present.
Comment on lines
+493
to
+497
| // Preserve Ollama providers with a non-empty BaseURL (no API key required) | ||
| if pCfg.Type == "ollama" && pCfg.BaseURL != "" { | ||
| filteredProviders[name] = pCfg | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
Filter out unresolved Ollama base_url placeholders.
removeEmptyProviders now keeps Ollama entries solely on non‑empty BaseURL; if the YAML uses ${OLLAMA_BASE_URL} and the env var is unset, the placeholder survives and the provider is kept with an invalid URL. Treat unresolved placeholders the same way as API keys.
🛠️ Suggested fix
- // Preserve Ollama providers with a non-empty BaseURL (no API key required)
- if pCfg.Type == "ollama" && pCfg.BaseURL != "" {
- filteredProviders[name] = pCfg
- continue
- }
+ // Preserve Ollama providers with a resolved BaseURL (no API key required)
+ if pCfg.Type == "ollama" {
+ baseURLSet := pCfg.BaseURL != "" && !strings.Contains(pCfg.BaseURL, "${")
+ if baseURLSet {
+ filteredProviders[name] = pCfg
+ }
+ continue
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Preserve Ollama providers with a non-empty BaseURL (no API key required) | |
| if pCfg.Type == "ollama" && pCfg.BaseURL != "" { | |
| filteredProviders[name] = pCfg | |
| continue | |
| } | |
| // Preserve Ollama providers with a resolved BaseURL (no API key required) | |
| if pCfg.Type == "ollama" { | |
| baseURLSet := pCfg.BaseURL != "" && !strings.Contains(pCfg.BaseURL, "${") | |
| if baseURLSet { | |
| filteredProviders[name] = pCfg | |
| } | |
| continue | |
| } |
🤖 Prompt for AI Agents
In `@config/config.go` around lines 493 - 497, The Ollama provider branch in
removeEmptyProviders currently keeps entries when pCfg.Type == "ollama" and
pCfg.BaseURL != ""; update this to also reject unresolved env placeholders by
treating values like "${...}" as empty: when handling pCfg.Type == "ollama"
check that pCfg.BaseURL is non-empty and does not contain an unresolved
placeholder (e.g., starts with "${" and ends with "}" or matches the placeholder
regex) before adding to filteredProviders[name] so unresolved ${OLLAMA_BASE_URL}
entries are filtered out just like missing API keys.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/adr/0001-explicit-provider-registration.md`:
- Around line 15-28: Add a migration section to the ADR that explains how to
move providers from init()-based automatic registration to explicit Register
calls in main.go: describe a step-by-step migration procedure (e.g., remove
init() registration, add ProviderName.Register(factory) in the application
startup), state whether both patterns are supported concurrently during
transition and how to handle duplicates, and provide a suggested
timeline/checklist for completing the migration and tests; reference the
explicit Register calls in main.go and the prior init() registration pattern so
readers can locate the affected providers and update them accordingly.
- Around line 1-2: Add standard ADR metadata to the top of "ADR-0001: Explicit
Provider Registration" by inserting fields like Date, Status (e.g.,
Proposed/Accepted/Deprecated/Superseded), and Authors (with contact) plus
optional Tags or Supersedes; update the document header to include these
metadata entries so the ADR can be tracked over time (e.g., beneath the title
add a small metadata block containing Date, Status, Authors, and Tags).
Comment on lines
+1
to
+2
| # ADR-0001: Explicit Provider Registration | ||
|
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding ADR metadata for better tracking.
ADRs typically include metadata such as date, status (Proposed/Accepted/Deprecated/Superseded), and authors. This helps track the evolution of architectural decisions over time.
📝 Suggested metadata addition
# ADR-0001: Explicit Provider Registration
+**Date**: 2026-01-22
+**Status**: Accepted
+**Authors**: [Author names]
+
## Context📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # ADR-0001: Explicit Provider Registration | |
| # ADR-0001: Explicit Provider Registration | |
| **Date**: 2026-01-22 | |
| **Status**: Accepted | |
| **Authors**: [Author names] | |
🤖 Prompt for AI Agents
In `@docs/adr/0001-explicit-provider-registration.md` around lines 1 - 2, Add
standard ADR metadata to the top of "ADR-0001: Explicit Provider Registration"
by inserting fields like Date, Status (e.g.,
Proposed/Accepted/Deprecated/Superseded), and Authors (with contact) plus
optional Tags or Supersedes; update the document header to include these
metadata entries so the ADR can be tracked over time (e.g., beneath the title
add a small metadata block containing Date, Status, Authors, and Tags).
Comment on lines
+15
to
+28
| ## Consequences | ||
|
|
||
| ### Positive | ||
|
|
||
| - **Explicit control flow**: Registration order is visible and controllable | ||
| - **No global state**: Factory is created and passed explicitly | ||
| - **Better testability**: Tests create isolated factories without workarounds | ||
| - **IDE navigation**: Click through to Registration instead of dead-end blank imports | ||
| - **Conditional registration**: Easy to add feature flags for experimental providers | ||
| - **Go best practices**: Avoids init() side effects | ||
|
|
||
| ### Negative | ||
|
|
||
| - Slightly more boilerplate in main.go (6 explicit Register calls) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding migration guidance.
Since this ADR introduces a new registration pattern that differs from the previous approach (based on learnings), it would be helpful to document:
- Steps to migrate existing providers from
init()-based registration to explicit registration - Whether both patterns are supported during the transition period
- Timeline or checklist for completing the migration
📝 Suggested migration section addition
### Negative
- Slightly more boilerplate in main.go (6 explicit Register calls)
+
+## Migration Path
+
+To migrate from init()-based registration to explicit registration:
+
+1. Remove blank imports from `cmd/gomodel/main.go`
+2. Remove `init()` function from each provider package
+3. Export a `Registration` variable in each provider package
+4. Add explicit `factory.Register(provider.Registration)` calls in `main.go`
+
+All providers should be migrated to maintain consistency.🤖 Prompt for AI Agents
In `@docs/adr/0001-explicit-provider-registration.md` around lines 15 - 28, Add a
migration section to the ADR that explains how to move providers from
init()-based automatic registration to explicit Register calls in main.go:
describe a step-by-step migration procedure (e.g., remove init() registration,
add ProviderName.Register(factory) in the application startup), state whether
both patterns are supported concurrently during transition and how to handle
duplicates, and provide a suggested timeline/checklist for completing the
migration and tests; reference the explicit Register calls in main.go and the
prior init() registration pattern so readers can locate the affected providers
and update them accordingly.
This was referenced Feb 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Behavior Changes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.