refactor(config): unify LLM config under global namespace#44
Merged
Conversation
- Move [llm] section to [global.llm] in configuration files - Migrate provider definitions from [llm.providers.*] to [global.llm.providers.*] - Update Go config to use c.Global.LLM.Providers instead of c.LLM.Providers - Remove legacy LLMConfig struct and GetActiveProvider() fallback logic - Update Go LLM client to reference c.Config.Global.LLM - Simplify Rust config by removing LlmConfig, ProviderConfig, AgentConfig structs - Add default_embedding_db_uri() and default_graph_db_uri() functions - Update documentation (README, ARCHITECTURE) to reflect new config structure
Reviewer's GuideRefactors LLM configuration to live exclusively under the [global.llm] namespace, removes legacy/duplicate LLM config structures and fallbacks in Go and Rust, updates resolution logic and tooling to use the unified config, adds a new tool override, and refreshes defaults and documentation to match. Class diagram for unified Go LLM config under global.llmclassDiagram
class ProviderConfig {
+string Model
+float64 Temperature
+int MaxTokens
+string APIBaseURL
+string APIKey
+string AWSRegion
+string ModelProvider
}
class GlobalLLMConfig {
+string UseProvider
+map[string]ProviderConfig Providers
}
class ToolLLMOverride {
+string UseProvider
}
class ToolsLLMConfig {
+string UseProvider
+ToolLLMOverride* MicroAgent
+ToolLLMOverride* Thinking
+ToolLLMOverride* ImplPlan
}
class AgentLLMOverride {
+string UseProvider
}
class AgentsLLMConfig {
+string UseProvider
+map[string]AgentLLMOverride Agents
}
class TopLevelConfig {
+GlobalLLMConfig* LLM
}
class AppConfig {
+bool EnableStreaming
}
class AgentConfig {
+int ConductorMaxSteps
+int CodingMaxSteps
+int RepoMaxSteps
+string Lang
}
class Config {
+TopLevelConfig Global
+AgentsLLMConfig Agents
+ToolsLLMConfig Tools
+AppConfig App
+AgentConfig Agent
+ProviderConfig getProvider(name string)
+ToolLLMOverride* getToolOverride(toolName string)
+ProviderConfig ResolveProvider(agentName string, toolName string)
+[]string GetProviderNames()
+string resolveEffectiveProviderName()
+error validate()
}
class Client {
+Config* Config
+ProviderConfig* ResolveProvider(agentName string, toolName string)
+string ResolveProviderName(agentName string, toolName string)
+string resolveProviderName(provider* ProviderConfig)
+string GenerateCompletionWithMemory(ctx context.Context, memory []Message, prompt string)
}
Config --> TopLevelConfig : Global
Config --> AgentsLLMConfig : Agents
Config --> ToolsLLMConfig : Tools
Config --> AppConfig : App
Config --> AgentConfig : Agent
TopLevelConfig --> GlobalLLMConfig : LLM
GlobalLLMConfig --> ProviderConfig : Providers
AgentsLLMConfig --> AgentLLMOverride : Agents
ToolsLLMConfig --> ToolLLMOverride : MicroAgent
ToolsLLMConfig --> ToolLLMOverride : Thinking
ToolsLLMConfig --> ToolLLMOverride : ImplPlan
Client --> Config
Client --> ProviderConfig
Class diagram for simplified Rust config with codebase defaultsclassDiagram
class Config {
+CodeBaseConfig codebase
+Config load()
}
class CodeBaseConfig {
+bool enable_embedding
+string embedding_db_uri
+string graph_db_uri
+EmbeddingConfig embedding
}
class EmbeddingConfig {
+string model
+Option~usize~ dimensions
}
class default_embedding_db_uri {
+string default_embedding_db_uri()
}
class default_graph_db_uri {
+string default_graph_db_uri()
}
Config --> CodeBaseConfig : codebase
CodeBaseConfig --> EmbeddingConfig : embedding
default_embedding_db_uri ..> CodeBaseConfig : serde_default_embedding_db_uri
default_graph_db_uri ..> CodeBaseConfig : serde_default_graph_db_uri
Flow diagram for Go provider resolution order without legacy llm fallbackflowchart TD
A["ResolveProvider(agentName, toolName)"] --> B{"toolName not empty?"}
B -->|yes| C["toolOverride = getToolOverride(toolName)"]
C --> D{"toolOverride not nil and toolOverride.UseProvider not empty?"}
D -->|yes| E["return getProvider(toolOverride.UseProvider)"]
D -->|no| F{"agentName not empty?"}
B -->|no| F
F --> G["agentOverride = Agents.Agents[agentName]"]
G --> H{"agentOverride.UseProvider not empty?"}
H -->|yes| I["return getProvider(agentOverride.UseProvider)"]
H -->|no| J{"Agents.UseProvider not empty?"}
J -->|yes| K["return getProvider(Agents.UseProvider)"]
J -->|no| L{"Global.LLM not nil and Global.LLM.UseProvider not empty?"}
L -->|yes| M["return getProvider(Global.LLM.UseProvider)"]
L -->|no| N["error: no LLM provider configured"]
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 1 issue, and left some high level feedback:
- Several log lines in
internal/llm/llm.gostill logc.Config.Global.LLM.UseProvideras the model/provider, which will be misleading when an agent/tool override is used; consider logging the resolved provider name (e.g., viaResolveProviderNameor by wiring through the name used inResolveProvider). - The validation error
"no providers configured in LLM section"inconfig.validate()no longer matches the refactored structure (providers are now underglobal.llm.providers); updating this message to reference the new namespace will make configuration issues clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several log lines in `internal/llm/llm.go` still log `c.Config.Global.LLM.UseProvider` as the model/provider, which will be misleading when an agent/tool override is used; consider logging the resolved provider name (e.g., via `ResolveProviderName` or by wiring through the name used in `ResolveProvider`).
- The validation error `"no providers configured in LLM section"` in `config.validate()` no longer matches the refactored structure (providers are now under `global.llm.providers`); updating this message to reference the new namespace will make configuration issues clearer.
## Individual Comments
### Comment 1
<location path="codebase/src/config.rs" line_range="5" />
<code_context>
- pub app: AppConfig,
- pub agent: AgentConfig,
- pub codebase: CodeBaseConfig,
+fn default_embedding_db_uri() -> String {
+ let home = dirs::home_dir().unwrap_or_default();
+ home.join(".codeactor/data/embedding")
</code_context>
<issue_to_address>
**suggestion:** default_*_db_uri uses unwrap_or_default while Config::load errors on missing home_dir, leading to inconsistent behavior.
Here a missing home directory quietly falls back to an empty `PathBuf`, yielding a relative `.codeactor/...` path. Please either share the same `home_dir` resolution (and error behavior) as `Config::load`, or intentionally support this relative-path fallback and document it so default URIs and the loader behave consistently.
Suggested implementation:
```rust
use std::fs;
use std::path::PathBuf;
use tracing::info;
/// Returns the base data directory used by default DB URIs.
///
/// This intentionally falls back to a *relative* `.codeactor/data` directory
/// when no home directory can be resolved (e.g. in some containerized or
/// restricted environments). Callers that require a hard failure on missing
/// home directories should not use this helper and instead mirror the
/// `Config::load` behavior.
fn default_data_dir() -> PathBuf {
dirs::home_dir()
.map(|home| home.join(".codeactor/data"))
.unwrap_or_else(|| PathBuf::from(".codeactor/data"))
}
/// Default embedding DB URI.
///
/// If a home directory is available, this will be
/// `$HOME/.codeactor/data/embedding`. Otherwise it falls back to the relative
/// path `.codeactor/data/embedding` from the current working directory.
fn default_embedding_db_uri() -> String {
default_data_dir()
.join("embedding")
.to_string_lossy()
.to_string()
}
/// Default graph DB URI.
///
/// If a home directory is available, this will be
/// `$HOME/.codeactor/data/graph`. Otherwise it falls back to the relative
/// path `.codeactor/data/graph` from the current working directory.
fn default_graph_db_uri() -> String {
default_data_dir()
.join("graph")
.to_string_lossy()
.to_string()
```
To make the behavior fully consistent across the codebase (as mentioned in your review comment), you should also:
1. Refactor `Config::load` to use the new `default_data_dir()` helper (or a variant of it) instead of resolving `dirs::home_dir()` independently.
2. If `Config::load` must continue to *error* when `home_dir` is missing, introduce a separate helper (e.g. `fn resolve_home_dir_or_error() -> Result<PathBuf, ConfigError>`) that encapsulates that behavior, and document the distinction between:
- "strict" home-dir resolution used by `Config::load`, and
- the "lenient" fallback used by `default_*_db_uri`.
3. Update any documentation or config help text to mention that default DB URIs may fall back to a relative `.codeactor/data/...` path when no home directory is available, so users are not surprised by the behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| pub app: AppConfig, | ||
| pub agent: AgentConfig, | ||
| pub codebase: CodeBaseConfig, | ||
| fn default_embedding_db_uri() -> String { |
There was a problem hiding this comment.
suggestion: default_*_db_uri uses unwrap_or_default while Config::load errors on missing home_dir, leading to inconsistent behavior.
Here a missing home directory quietly falls back to an empty PathBuf, yielding a relative .codeactor/... path. Please either share the same home_dir resolution (and error behavior) as Config::load, or intentionally support this relative-path fallback and document it so default URIs and the loader behave consistently.
Suggested implementation:
use std::fs;
use std::path::PathBuf;
use tracing::info;
/// Returns the base data directory used by default DB URIs.
///
/// This intentionally falls back to a *relative* `.codeactor/data` directory
/// when no home directory can be resolved (e.g. in some containerized or
/// restricted environments). Callers that require a hard failure on missing
/// home directories should not use this helper and instead mirror the
/// `Config::load` behavior.
fn default_data_dir() -> PathBuf {
dirs::home_dir()
.map(|home| home.join(".codeactor/data"))
.unwrap_or_else(|| PathBuf::from(".codeactor/data"))
}
/// Default embedding DB URI.
///
/// If a home directory is available, this will be
/// `$HOME/.codeactor/data/embedding`. Otherwise it falls back to the relative
/// path `.codeactor/data/embedding` from the current working directory.
fn default_embedding_db_uri() -> String {
default_data_dir()
.join("embedding")
.to_string_lossy()
.to_string()
}
/// Default graph DB URI.
///
/// If a home directory is available, this will be
/// `$HOME/.codeactor/data/graph`. Otherwise it falls back to the relative
/// path `.codeactor/data/graph` from the current working directory.
fn default_graph_db_uri() -> String {
default_data_dir()
.join("graph")
.to_string_lossy()
.to_string()To make the behavior fully consistent across the codebase (as mentioned in your review comment), you should also:
- Refactor
Config::loadto use the newdefault_data_dir()helper (or a variant of it) instead of resolvingdirs::home_dir()independently. - If
Config::loadmust continue to error whenhome_diris missing, introduce a separate helper (e.g.fn resolve_home_dir_or_error() -> Result<PathBuf, ConfigError>) that encapsulates that behavior, and document the distinction between:- "strict" home-dir resolution used by
Config::load, and - the "lenient" fallback used by
default_*_db_uri.
- "strict" home-dir resolution used by
- Update any documentation or config help text to mention that default DB URIs may fall back to a relative
.codeactor/data/...path when no home directory is available, so users are not surprised by the behavior.
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
Unify LLM configuration under the [global.llm] namespace and remove legacy per-root [llm] structures in both Go and Rust components.
New Features:
Enhancements:
Documentation: