feat: refactor config and LLM module, support OpenAI Go SDK#42
Merged
Conversation
- Add new config parsing with toml support - Refactor LLM module with OpenAI API integration - Update app initialization to use new config structure - Add comprehensive config validation and loading
Reviewer's GuideRefactors configuration and LLM client to support a multi-tier provider selection (global/agent/tool) while keeping legacy [llm] compatible, adds per-agent/per-tool engine resolution and caching in the LLM client and app initialization, updates the sample TOML config to the new structure, and tweaks HTTP server startup to auto-select a port when not explicitly provided. Sequence diagram for CodingAssistant initialization with per-agent and per-tool enginessequenceDiagram
actor User
participant Main as main
participant LLMClient as Client
participant Config as Config
participant Assistant as CodingAssistant
participant Tools as Tools
participant Agents as Agents
User->>Main: start application
Main->>Config: LoadConfig(path)
Config-->>Main: *config.Config
Main->>LLMClient: NewClient(config)
LLMClient->>Config: ResolveProvider("", "")
Config-->>LLMClient: *ProviderConfig (default)
LLMClient->>LLMClient: create default Engine and cache map
LLMClient-->>Main: *Client
Main->>Assistant: NewCodingAssistant(client)
Assistant-->>Main: *CodingAssistant
Main->>Assistant: Init(client.Engine, workDir)
Assistant->>Assistant: set default engine
Assistant->>LLMClient: GetToolEngine("micro_agent")
LLMClient->>Config: ResolveProvider("", "micro_agent")
Config-->>LLMClient: *ProviderConfig (tool or fallback)
LLMClient->>LLMClient: getOrCreateEngine(provider, name)
LLMClient-->>Assistant: Engine for micro_agent
Assistant->>Tools: NewMicroAgentTool(microAgentEngine)
loop for each agent
Assistant->>LLMClient: GetAgentEngine(agentName)
LLMClient->>Config: ResolveProvider(agentName, "")
Config-->>LLMClient: *ProviderConfig
LLMClient->>LLMClient: getOrCreateEngine(provider, name)
LLMClient-->>Assistant: Engine for agent
Assistant->>Agents: New<Agent>(agentEngine)
end
Assistant->>Agents: NewConductorAgent(conductorEngine, repoAgent, codingAgent, chatAgent, metaAgent, devopsAgent)
Class diagram for updated LLM configuration hierarchyclassDiagram
class Config {
+LLMConfig LLM
+TopLevelConfig Global
+AgentsLLMConfig Agents
+ToolsLLMConfig Tools
+AppConfig App
+AgentConfig Agent
+GetActiveProvider() ProviderConfig
+getProvider(name string) ProviderConfig
+resolveAgentProvider(agentName string) string
+getAgentOverride(agentName string) AgentLLMOverride
+resolveToolProvider(toolName string) string
+getToolOverride(toolName string) ToolLLMOverride
+ResolveProvider(agentName string, toolName string) ProviderConfig
+GetProviderNames() []string
+resolveEffectiveProviderName() string
+validate() error
}
class TopLevelConfig {
+GlobalLLMConfig LLM
}
class GlobalLLMConfig {
+string UseProvider
}
class AgentsLLMConfig {
+string UseProvider
+AgentLLMOverride Conductor
+AgentLLMOverride Coding
+AgentLLMOverride Repo
+AgentLLMOverride Chat
+AgentLLMOverride Meta
+AgentLLMOverride DevOps
}
class AgentLLMOverride {
+string UseProvider
}
class ToolsLLMConfig {
+string UseProvider
+ToolLLMOverride MicroAgent
+ToolLLMOverride Thinking
}
class ToolLLMOverride {
+string UseProvider
}
class LLMConfig {
+string UseProvider
+map~string,ProviderConfig~ Providers
}
class ProviderConfig {
+string Model
+string APIKey
+string APIBaseURL
+string AWSRegion
}
Config --> LLMConfig : has
Config --> TopLevelConfig : has
Config --> AgentsLLMConfig : has
Config --> ToolsLLMConfig : has
TopLevelConfig --> GlobalLLMConfig : has
AgentsLLMConfig --> AgentLLMOverride : overrides
ToolsLLMConfig --> ToolLLMOverride : overrides
LLMConfig --> ProviderConfig : providers
Class diagram for updated LLM client and engine resolutionclassDiagram
class Engine {
<<interface>>
+GenerateContent(ctx context.Context, messages []Message, params GenerationParams) (Response, error)
}
class LoggingEngine {
-Engine inner
+GenerateContent(ctx context.Context, messages []Message, params GenerationParams) (Response, error)
}
class Client {
+Engine Engine
+Config *config.Config
-map~string,Engine~ engines
-sync.RWMutex mu
+NewClient(config *config.Config) Client
+ResolveProviderName(agentName string, toolName string) string
+getOrCreateEngine(provider *config.ProviderConfig, providerName string) Engine
+GetEngine() Engine
+GetAgentEngine(agentName string) Engine
+GetToolEngine(toolName string) Engine
-resolveProviderName(provider *config.ProviderConfig) string
}
class Config {
+ResolveProvider(agentName string, toolName string) ProviderConfig
+LLMConfig LLM
}
class ProviderConfig {
+string Model
+string APIKey
+string APIBaseURL
}
class CodingAssistant {
-llm.Engine engine
-llm.Client client
-config.Config config
+NewCodingAssistant(client *llm.Client) CodingAssistant
+Init(engine llm.Engine, workDir string)
}
class ToolsNamespace {
<<namespace>>
+NewMicroAgentTool(engine llm.Engine)
+NewThinkingTool()
}
Engine <|.. LoggingEngine : implements
Client --> Engine : uses
Client --> Config : holds
Client --> ProviderConfig : resolves
LoggingEngine --> Engine : wraps
CodingAssistant --> Client : uses
CodingAssistant --> Engine : uses
CodingAssistant ..> ToolsNamespace : constructs tools
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The documented priority chain for tools (
tools.<tool> > tools.default > agent > global > legacy) isn’t actually implemented inGetToolEngine/ResolveProvider(you always callResolveProvider("", toolName)), so either the implementation should accept an agent context or the comment/expected behavior should be adjusted for consistency. - Current config validation only checks the single
effectiveProviderfrom defaults (tools.llm,agents.llm,global.llm,llm) and doesn’t verify that per-agent and per-tooluse_provideroverrides refer to existing providers, which could lead to runtime failures; consider iterating all override fields inAgentsLLMConfig/ToolsLLMConfigduringvalidate()to ensure they point to valid providers. Client.ResolveProviderNameandresolveProviderNamerely on matchingModelandAPIBaseURLto infer the provider key, which can be ambiguous if multiple providers share these values and is also O(n) each call; consider tracking the provider name alongside the config inResolveProvideror returning the name directly to avoid guesswork and improve performance.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The documented priority chain for tools (`tools.<tool> > tools.default > agent > global > legacy`) isn’t actually implemented in `GetToolEngine`/`ResolveProvider` (you always call `ResolveProvider("", toolName)`), so either the implementation should accept an agent context or the comment/expected behavior should be adjusted for consistency.
- Current config validation only checks the single `effectiveProvider` from defaults (`tools.llm`, `agents.llm`, `global.llm`, `llm`) and doesn’t verify that per-agent and per-tool `use_provider` overrides refer to existing providers, which could lead to runtime failures; consider iterating all override fields in `AgentsLLMConfig`/`ToolsLLMConfig` during `validate()` to ensure they point to valid providers.
- `Client.ResolveProviderName` and `resolveProviderName` rely on matching `Model` and `APIBaseURL` to infer the provider key, which can be ambiguous if multiple providers share these values and is also O(n) each call; consider tracking the provider name alongside the config in `ResolveProvider` or returning the name directly to avoid guesswork and improve performance.
## Individual Comments
### Comment 1
<location path="internal/config/config.go" line_range="280-289" />
<code_context>
return config, nil
}
+// resolveEffectiveProviderName returns the effective provider name using the full
+// priority chain. Used during validation to find the fallback provider to validate.
+func (c *Config) resolveEffectiveProviderName() string {
+ // tools default
+ if c.Tools.UseProvider != "" {
+ return c.Tools.UseProvider
+ }
+ // agents default
+ if c.Agents.UseProvider != "" {
+ return c.Agents.UseProvider
+ }
+ // global
+ if c.Global.LLM != nil && c.Global.LLM.UseProvider != "" {
+ return c.Global.LLM.UseProvider
+ }
+ // legacy
+ return c.LLM.UseProvider
+}
+
</code_context>
<issue_to_address>
**issue:** Validation only checks default-level use_provider and can miss invalid per-agent/per-tool overrides
`resolveEffectiveProviderName` only uses the default-level `use_provider` fields (`tools.llm.use_provider`, `agents.llm.use_provider`, `global.llm.use_provider`, legacy `llm.use_provider`) when validating, so per-agent/per-tool overrides like `[agents.llm.coding]` or `[tools.llm.micro_agent]` aren’t checked. Typos in those override provider names will only fail at runtime when `ResolveProvider` is called. Please extend `validate` to walk all agent/tool overrides and confirm each referenced provider exists so configuration errors are caught early.
</issue_to_address>
### Comment 2
<location path="internal/llm/llm.go" line_range="174-183" />
<code_context>
+// ResolveProviderName resolves the provider name using the full priority chain.
+// Exported for logging convenience.
+func (c *Client) ResolveProviderName(agentName, toolName string) string {
+ provider, err := c.Config.ResolveProvider(agentName, toolName)
+ if err != nil {
+ return "unknown"
+ }
+ // Find the provider key from the config
+ for name, p := range c.Config.LLM.Providers {
+ if p.Model == provider.Model && p.APIBaseURL == provider.APIBaseURL {
+ return name
+ }
+ }
+ return "unknown"
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Provider-name resolution by matching only model/APIBaseURL can be ambiguous across providers
Both `ResolveProviderName` and `resolveProviderName` infer the provider name by scanning `Config.LLM.Providers` and matching only `Model` + `APIBaseURL`. If multiple providers share those values (e.g., different keys/environments), the chosen name is effectively arbitrary. Since this is used for logging and as the cache key in `getOrCreateEngine`, that ambiguity can lead to misleading logs or engines cached under the wrong name. If overlapping model/baseURL is allowed, consider including a more unique field (e.g., `APIKey` or explicit `Name`), or have `ResolveProvider` return the provider name directly instead of re-deriving it.
Suggested implementation:
```golang
// ResolveProviderName resolves the provider name using the full priority chain.
// Exported for logging convenience.
func (c *Client) ResolveProviderName(agentName, toolName string) string {
providerName, _, err := c.Config.ResolveProvider(agentName, toolName)
if err != nil || providerName == "" {
return "unknown"
}
return providerName
}
```
To fully implement the suggestion and remove the ambiguous provider-name inference, you’ll also need to:
1. Update the `ResolveProvider` method in the `config` package (likely in `internal/config`), so that it returns the provider name along with the provider configuration, e.g.:
- Change its signature from something like:
- `ResolveProvider(agentName, toolName string) (ProviderConfig, error)`
- to:
- `ResolveProvider(agentName, toolName string) (string, ProviderConfig, error)`
2. Adjust all existing call sites of `Config.ResolveProvider` to handle the extra returned `providerName` value.
3. Remove any other code that tries to infer the provider name by scanning `Config.LLM.Providers` and matching only `Model` + `APIBaseURL` (e.g., a lowercase `resolveProviderName` helper if it exists) and instead use the returned `providerName` from `ResolveProvider`.
</issue_to_address>
### Comment 3
<location path="internal/llm/llm.go" line_range="218-227" />
<code_context>
+
+// GetAgentEngine resolves and returns the engine for a specific agent.
+// Uses the priority chain: agents.<agent> > agents.default > global > legacy.
+func (c *Client) GetAgentEngine(agentName string) Engine {
+ provider, err := c.Config.ResolveProvider(agentName, "")
+ if err != nil {
+ slog.Warn("Failed to resolve agent provider, falling back to default", "agent", agentName, "error", err)
+ return c.Engine
+ }
+
+ providerName := c.resolveProviderName(provider)
+ if providerName == "" {
+ return c.Engine
+ }
+
+ // If resolved provider matches the default engine's provider, reuse default
+ defaultProvider, defaultErr := c.Config.ResolveProvider("", "")
+ if defaultErr == nil && providersEqual(provider, defaultProvider) {
+ return c.Engine
+ }
+
+ return c.getOrCreateEngine(provider, providerName)
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Silent fallback to default engine when provider name cannot be resolved may hide configuration issues
When `resolveProviderName` returns an empty string in `GetAgentEngine`/`GetToolEngine`, we silently fall back to the default engine. This can mask misconfigured or unmapped providers where resolution succeeds but name mapping fails. Consider logging a warning in this branch (including agent/tool name and basic provider info) so these configuration problems are visible while keeping the fallback behavior.
Suggested implementation:
```golang
// GetAgentEngine resolves and returns the engine for a specific agent.
// Uses the priority chain: agents.<agent> > agents.default > global > legacy.
func (c *Client) GetAgentEngine(agentName string) Engine {
provider, err := c.Config.ResolveProvider(agentName, "")
if err != nil {
slog.Warn("Failed to resolve agent provider, falling back to default", "agent", agentName, "error", err)
return c.Engine
}
providerName := c.resolveProviderName(provider)
if providerName == "" {
slog.Warn(
"Resolved agent provider has no name mapping, falling back to default engine",
"agent", agentName,
)
return c.Engine
}
// If resolved provider matches the default engine's provider, reuse default
defaultProvider, defaultErr := c.Config.ResolveProvider("", "")
if defaultErr == nil && providersEqual(provider, defaultProvider) {
return c.Engine
}
return c.getOrCreateEngine(provider, providerName)
}
```
To fully implement your suggestion consistently:
1. Apply the same pattern to `GetToolEngine`:
- After `providerName := c.resolveProviderName(provider)`, when `providerName == ""`, log a similar `slog.Warn` including the tool name (and possibly agent name if available) before returning `c.Engine`.
2. Ensure the warning message for tools clearly distinguishes between agent and tool contexts, e.g. `"Resolved tool provider has no name mapping, falling back to default engine"`, with keys like `"tool", toolName`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+280
to
+289
| // resolveEffectiveProviderName returns the effective provider name using the full | ||
| // priority chain. Used during validation to find the fallback provider to validate. | ||
| func (c *Config) resolveEffectiveProviderName() string { | ||
| // tools default | ||
| if c.Tools.UseProvider != "" { | ||
| return c.Tools.UseProvider | ||
| } | ||
| // agents default | ||
| if c.Agents.UseProvider != "" { | ||
| return c.Agents.UseProvider |
There was a problem hiding this comment.
issue: Validation only checks default-level use_provider and can miss invalid per-agent/per-tool overrides
resolveEffectiveProviderName only uses the default-level use_provider fields (tools.llm.use_provider, agents.llm.use_provider, global.llm.use_provider, legacy llm.use_provider) when validating, so per-agent/per-tool overrides like [agents.llm.coding] or [tools.llm.micro_agent] aren’t checked. Typos in those override provider names will only fail at runtime when ResolveProvider is called. Please extend validate to walk all agent/tool overrides and confirm each referenced provider exists so configuration errors are caught early.
Comment on lines
+174
to
+183
| func (c *Client) ResolveProviderName(agentName, toolName string) string { | ||
| provider, err := c.Config.ResolveProvider(agentName, toolName) | ||
| if err != nil { | ||
| return "unknown" | ||
| } | ||
| // Find the provider key from the config | ||
| for name, p := range c.Config.LLM.Providers { | ||
| if p.Model == provider.Model && p.APIBaseURL == provider.APIBaseURL { | ||
| return name | ||
| } |
There was a problem hiding this comment.
suggestion (bug_risk): Provider-name resolution by matching only model/APIBaseURL can be ambiguous across providers
Both ResolveProviderName and resolveProviderName infer the provider name by scanning Config.LLM.Providers and matching only Model + APIBaseURL. If multiple providers share those values (e.g., different keys/environments), the chosen name is effectively arbitrary. Since this is used for logging and as the cache key in getOrCreateEngine, that ambiguity can lead to misleading logs or engines cached under the wrong name. If overlapping model/baseURL is allowed, consider including a more unique field (e.g., APIKey or explicit Name), or have ResolveProvider return the provider name directly instead of re-deriving it.
Suggested implementation:
// ResolveProviderName resolves the provider name using the full priority chain.
// Exported for logging convenience.
func (c *Client) ResolveProviderName(agentName, toolName string) string {
providerName, _, err := c.Config.ResolveProvider(agentName, toolName)
if err != nil || providerName == "" {
return "unknown"
}
return providerName
}To fully implement the suggestion and remove the ambiguous provider-name inference, you’ll also need to:
- Update the
ResolveProvidermethod in theconfigpackage (likely ininternal/config), so that it returns the provider name along with the provider configuration, e.g.:- Change its signature from something like:
ResolveProvider(agentName, toolName string) (ProviderConfig, error)- to:
ResolveProvider(agentName, toolName string) (string, ProviderConfig, error)
- Change its signature from something like:
- Adjust all existing call sites of
Config.ResolveProviderto handle the extra returnedproviderNamevalue. - Remove any other code that tries to infer the provider name by scanning
Config.LLM.Providersand matching onlyModel+APIBaseURL(e.g., a lowercaseresolveProviderNamehelper if it exists) and instead use the returnedproviderNamefromResolveProvider.
Comment on lines
+218
to
+227
| func (c *Client) GetAgentEngine(agentName string) Engine { | ||
| provider, err := c.Config.ResolveProvider(agentName, "") | ||
| if err != nil { | ||
| slog.Warn("Failed to resolve agent provider, falling back to default", "agent", agentName, "error", err) | ||
| return c.Engine | ||
| } | ||
|
|
||
| providerName := c.resolveProviderName(provider) | ||
| if providerName == "" { | ||
| return c.Engine |
There was a problem hiding this comment.
suggestion (bug_risk): Silent fallback to default engine when provider name cannot be resolved may hide configuration issues
When resolveProviderName returns an empty string in GetAgentEngine/GetToolEngine, we silently fall back to the default engine. This can mask misconfigured or unmapped providers where resolution succeeds but name mapping fails. Consider logging a warning in this branch (including agent/tool name and basic provider info) so these configuration problems are visible while keeping the fallback behavior.
Suggested implementation:
// GetAgentEngine resolves and returns the engine for a specific agent.
// Uses the priority chain: agents.<agent> > agents.default > global > legacy.
func (c *Client) GetAgentEngine(agentName string) Engine {
provider, err := c.Config.ResolveProvider(agentName, "")
if err != nil {
slog.Warn("Failed to resolve agent provider, falling back to default", "agent", agentName, "error", err)
return c.Engine
}
providerName := c.resolveProviderName(provider)
if providerName == "" {
slog.Warn(
"Resolved agent provider has no name mapping, falling back to default engine",
"agent", agentName,
)
return c.Engine
}
// If resolved provider matches the default engine's provider, reuse default
defaultProvider, defaultErr := c.Config.ResolveProvider("", "")
if defaultErr == nil && providersEqual(provider, defaultProvider) {
return c.Engine
}
return c.getOrCreateEngine(provider, providerName)
}To fully implement your suggestion consistently:
- Apply the same pattern to
GetToolEngine:- After
providerName := c.resolveProviderName(provider), whenproviderName == "", log a similarslog.Warnincluding the tool name (and possibly agent name if available) before returningc.Engine.
- After
- Ensure the warning message for tools clearly distinguishes between agent and tool contexts, e.g.
"Resolved tool provider has no name mapping, falling back to default engine", with keys like"tool", toolName.
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 Sourcery
Introduce hierarchical LLM provider configuration and per-agent/tool engine routing while preserving legacy defaults.
New Features:
Enhancements: