internal/config,watcher: add -config-dir#873
Conversation
Over time the llama-swap configuration file can get really long and challenging to work with. The -config-dir flag is used for a directory of configuration YAML fragments. These fragements are merged together and into a full configuration and tested for validity. All previous configuration functionality remains unchanged.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
|
| Filename | Overview |
|---|---|
| internal/config/merge.go | New file implementing multi-source config merging; overlap detection uses filepath.Abs which does not resolve symlinks, and sequence fields like apiKeys are concatenated without deduplication. |
| internal/config/merge_test.go | Comprehensive test suite covering most merge scenarios including duplicate keys, scalar conflicts, empty dirs, and macro propagation; no coverage for the symlink overlap edge case. |
| internal/watcher/dirwatcher.go | New poll-based directory watcher that detects YAML file add/remove/modify; correctly silences directory-disappearance events and fires on recovery; design mirrors the existing single-file Watcher. |
| internal/watcher/dirwatcher_test.go | Well-structured tests covering baseline no-fire, add, remove, mtime change, non-YAML ignore, missing-dir recovery, and context cancellation. |
| llama-swap.go | Adds -config-dir flag, wires LoadConfigSources for both initial load and hot-reload, and launches DirWatcher alongside the existing single-file Watcher when -watch-config is set. |
| internal/config/load.go | LoadConfigFromReader extracted from config.go; no functional changes, only file reorganisation. |
| internal/config/commands.go | SanitizeCommand and StripComments extracted from config.go; no functional changes. |
| internal/config/macros.go | Macro-related regex vars and helper functions extracted from config.go; no functional changes. |
| internal/config/config.go | Imports and code blocks moved to load.go, commands.go, and macros.go; struct definitions and LoadConfig unchanged. |
Reviews (1): Last reviewed commit: "internal/config,watcher: add -config-dir" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| absConfig, err := filepath.Abs(configPath) | ||
| if err != nil { | ||
| return Config{}, fmt.Errorf("failed to resolve -config path: %w", err) | ||
| } | ||
| for _, f := range dirFiles { | ||
| absF, err := filepath.Abs(f) | ||
| if err != nil { | ||
| return Config{}, fmt.Errorf("failed to resolve config dir file %s: %w", f, err) | ||
| } | ||
| if absConfig == absF { | ||
| return Config{}, fmt.Errorf("-config path %s is also present in -config-dir %s; remove it from one", configPath, configDir) | ||
| } | ||
| } |
There was a problem hiding this comment.
filepath.Abs only cleans the path (resolves .., prepends cwd); it does not follow symlinks. If -config is a symlink pointing to a file inside -config-dir, the comparison absConfig == absF will always be false because the two absolute paths differ even though they resolve to the same inode. The file will be parsed twice: once as the -config source and once as a member of the directory scan. The merge will then fail with a generic "duplicate models" or "conflict at" error rather than the precise "is also present in -config-dir" message.
Using filepath.EvalSymlinks on both paths before comparing would catch this case correctly.
| case dstVal.Kind == yaml.SequenceNode && srcVal.Kind == yaml.SequenceNode: | ||
| dstVal.Content = append(dstVal.Content, srcVal.Content...) | ||
| return nil |
There was a problem hiding this comment.
Sequence-typed fields like apiKeys and hooks.on_startup.preload are concatenated verbatim. If the same value appears in two fragment files (e.g. apiKeys: [shared-key] in both a.yaml and b.yaml), the merged config will contain duplicates. For apiKeys this is harmless at auth-check time, but it silently inflates the list and produces no diagnostic. A user who accidentally copies an apiKeys entry into a second fragment will see no warning.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
internal/watcher/dirwatcher_test.go (1)
133-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a present→empty (all files removed) test.
TestDirWatcher_MissingDirRecoverscovers directory removal, but not the case where the directory remains but becomes empty. Given theRundoc claims empty is treated as transient, a test asserting the intended behavior would lock in whichever policy is chosen (see the related comment indirwatcher.go).🤖 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 `@internal/watcher/dirwatcher_test.go` around lines 133 - 155, Add a test for the present→empty transition in DirWatcher so the behavior is locked in when a watched directory stays present but all YAML files are removed. Extend the coverage in dirwatcher_test.go alongside TestDirWatcher_MissingDirRecovers by asserting the callback behavior after deleting the last file from an existing directory, using startDirWatcher, DirWatcher.Run semantics, and waitForCount/atomic counter checks to verify the intended empty-directory policy.
🤖 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 `@internal/config/load.go`:
- Around line 388-397: The API key validation in the RequiredAPIKeys loop is
leaking secrets by including the full key in the error message. Update the
validation in load.go so the `strings.Contains(apikey, " ")` failure reports the
key index from the `for i, apikey := range config.RequiredAPIKeys` loop instead
of echoing `apikey`, and keep the rest of the `Config` validation logic
unchanged.
In `@internal/config/merge_test.go`:
- Around line 204-214: The test currently only checks PORT substitution, so it
does not actually cover env macro expansion. Update
TestLoadConfigSources_EnvMacrosSubstituted to add a real ${env.*} case using
t.Setenv and verify it survives multi-source loading, ideally through a
flow-style YAML field such as apiKeys in writeYAML/LoadConfigSources. Keep the
existing PORT assertions, but also assert the env macro is substituted in the
merged config object to preserve the current env substitution behavior.
- Around line 216-233: The test in
TestLoadConfigSources_SortedOrderDeterministic only verifies that both models
are present, so it won’t fail if the merge order changes; update it to assert an
order-sensitive merged outcome from LoadConfigSources, such as the deterministic
startPort-based result or another concatenated field like apiKeys, and compare
the exact merged value with assert.Equal using the existing FindConfig checks as
needed to locate the merged models.
In `@internal/config/merge.go`:
- Around line 72-85: LoadConfigSources is parsing raw source files in
parseSource before env macro substitution, so valid fragments like flow-style
lists with ${env.*} are rejected by yaml.Unmarshal. Update parseSource to read
the file content, run substituteEnvMacros on it first, then unmarshal the
substituted text, and keep the existing mergeNodes flow in LoadConfigSources
unchanged. Add a regression test covering LoadConfigSources with a fragment such
as apiKeys: [${env.API_KEY}] to verify it now parses correctly.
In `@internal/watcher/dirwatcher.go`:
- Around line 65-99: DirWatcher.Run currently only treats a missing directory as
transient, but the documented policy also says an empty directory should stay
quiet. Update the transition logic in Run (using scanDir and prev.equal) so that
present→empty is suppressed the same way as present→missing, or adjust the doc
comment if the empty-directory case is intentionally observable; keep the
behavior aligned with the policy around OnChange firing.
---
Nitpick comments:
In `@internal/watcher/dirwatcher_test.go`:
- Around line 133-155: Add a test for the present→empty transition in DirWatcher
so the behavior is locked in when a watched directory stays present but all YAML
files are removed. Extend the coverage in dirwatcher_test.go alongside
TestDirWatcher_MissingDirRecovers by asserting the callback behavior after
deleting the last file from an existing directory, using startDirWatcher,
DirWatcher.Run semantics, and waitForCount/atomic counter checks to verify the
intended empty-directory policy.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20397283-9635-45ee-8447-40eb4f7c899b
📒 Files selected for processing (9)
internal/config/commands.gointernal/config/config.gointernal/config/load.gointernal/config/macros.gointernal/config/merge.gointernal/config/merge_test.gointernal/watcher/dirwatcher.gointernal/watcher/dirwatcher_test.gollama-swap.go
💤 Files with no reviewable changes (1)
- internal/config/config.go
| func TestLoadConfigSources_EnvMacrosSubstituted(t *testing.T) { | ||
| dir := t.TempDir() | ||
| // Use ${PORT} in cmd so the pipeline allocates a port and substitutes it; | ||
| // verifies env/macro substitution runs on the merged document. | ||
| writeYAML(t, dir, "a.yaml", "models:\n m1:\n cmd: serve --port ${PORT}\n proxy: \"http://localhost:${PORT}\"\n") | ||
| cfg, err := LoadConfigSources("", dir) | ||
| require.NoError(t, err) | ||
| m := cfg.Models["m1"] | ||
| assert.NotContains(t, m.Cmd, "${PORT}", "PORT macro should have been substituted") | ||
| assert.NotContains(t, m.Proxy, "${PORT}", "PORT macro should have been substituted in proxy") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make this cover an actual env macro.
This test name says env macros, but it only verifies ${PORT} allocation. Add a ${env.*} case via t.Setenv, especially through flow-style YAML such as apiKeys: [${env.LLAMA_SWAP_TEST_API_KEY}], so the multi-source loader preserves existing env substitution behavior.
🤖 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 `@internal/config/merge_test.go` around lines 204 - 214, The test currently
only checks PORT substitution, so it does not actually cover env macro
expansion. Update TestLoadConfigSources_EnvMacrosSubstituted to add a real
${env.*} case using t.Setenv and verify it survives multi-source loading,
ideally through a flow-style YAML field such as apiKeys in
writeYAML/LoadConfigSources. Keep the existing PORT assertions, but also assert
the env macro is substituted in the merged config object to preserve the current
env substitution behavior.
| func TestLoadConfigSources_SortedOrderDeterministic(t *testing.T) { | ||
| // Two files defining distinct models, scanned in z..a order by filename. | ||
| // Determine merged result is the same regardless of how the FS returns them. | ||
| dir := t.TempDir() | ||
| writeYAML(t, dir, "z.yaml", "models:\n"+modelCfg("zmodel", "echo z")) | ||
| writeYAML(t, dir, "a.yaml", "models:\n"+modelCfg("amodel", "echo a")) | ||
|
|
||
| const runs = 3 | ||
| for i := 0; i < runs; i++ { | ||
| cfg, err := LoadConfigSources("", dir) | ||
| require.NoError(t, err) | ||
| // startPort-based allocation: first allocated model gets 5800. | ||
| // Sorted order means amodel gets 5800, zmodel gets 5801. | ||
| _, _, ok := cfg.FindConfig("amodel") | ||
| assert.True(t, ok) | ||
| _, _, ok = cfg.FindConfig("zmodel") | ||
| assert.True(t, ok) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert an order-sensitive merged result.
This test only checks that both models exist, so it would pass even if listYAMLFiles stopped sorting. Assert something order-sensitive, such as concatenated apiKeys, with assert.Equal instead of presence checks.
Suggested direction
- writeYAML(t, dir, "z.yaml", "models:\n"+modelCfg("zmodel", "echo z"))
- writeYAML(t, dir, "a.yaml", "models:\n"+modelCfg("amodel", "echo a"))
+ writeYAML(t, dir, "z.yaml", "models:\n"+modelCfg("zmodel", "echo z")+"\napiKeys: [key-z]\n")
+ writeYAML(t, dir, "a.yaml", "models:\n"+modelCfg("amodel", "echo a")+"\napiKeys: [key-a]\n")
- const runs = 3
- for i := 0; i < runs; i++ {
- cfg, err := LoadConfigSources("", dir)
- require.NoError(t, err)
- // startPort-based allocation: first allocated model gets 5800.
- // Sorted order means amodel gets 5800, zmodel gets 5801.
- _, _, ok := cfg.FindConfig("amodel")
- assert.True(t, ok)
- _, _, ok = cfg.FindConfig("zmodel")
- assert.True(t, ok)
- }
+ cfg, err := LoadConfigSources("", dir)
+ require.NoError(t, err)
+ assert.Equal(t, []string{"key-a", "key-z"}, cfg.RequiredAPIKeys)📝 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.
| func TestLoadConfigSources_SortedOrderDeterministic(t *testing.T) { | |
| // Two files defining distinct models, scanned in z..a order by filename. | |
| // Determine merged result is the same regardless of how the FS returns them. | |
| dir := t.TempDir() | |
| writeYAML(t, dir, "z.yaml", "models:\n"+modelCfg("zmodel", "echo z")) | |
| writeYAML(t, dir, "a.yaml", "models:\n"+modelCfg("amodel", "echo a")) | |
| const runs = 3 | |
| for i := 0; i < runs; i++ { | |
| cfg, err := LoadConfigSources("", dir) | |
| require.NoError(t, err) | |
| // startPort-based allocation: first allocated model gets 5800. | |
| // Sorted order means amodel gets 5800, zmodel gets 5801. | |
| _, _, ok := cfg.FindConfig("amodel") | |
| assert.True(t, ok) | |
| _, _, ok = cfg.FindConfig("zmodel") | |
| assert.True(t, ok) | |
| } | |
| func TestLoadConfigSources_SortedOrderDeterministic(t *testing.T) { | |
| // Two files defining distinct models, scanned in z..a order by filename. | |
| // Determine merged result is the same regardless of how the FS returns them. | |
| dir := t.TempDir() | |
| writeYAML(t, dir, "z.yaml", "models:\n"+modelCfg("zmodel", "echo z")+"\napiKeys: [key-z]\n") | |
| writeYAML(t, dir, "a.yaml", "models:\n"+modelCfg("amodel", "echo a")+"\napiKeys: [key-a]\n") | |
| cfg, err := LoadConfigSources("", dir) | |
| require.NoError(t, err) | |
| assert.Equal(t, []string{"key-a", "key-z"}, cfg.RequiredAPIKeys) |
🤖 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 `@internal/config/merge_test.go` around lines 216 - 233, The test in
TestLoadConfigSources_SortedOrderDeterministic only verifies that both models
are present, so it won’t fail if the merge order changes; update it to assert an
order-sensitive merged outcome from LoadConfigSources, such as the deterministic
startPort-based result or another concatenated field like apiKeys, and compare
the exact merged value with assert.Equal using the existing FindConfig checks as
needed to locate the merged models.
- load.go: stop leaking API keys in error messages; report index instead
- merge.go: substitute env macros before YAML parse so flow-style lists
like [${env.API_KEY}] parse correctly
- dirwatcher.go: suppress present to empty transitions per documented
policy; fix TOCTOU race in scanDir between ReadDir and Stat
1 participant
Over time the llama-swap configuration file can get really long and challenging to work with. The -config-dir flag is used for a directory of configuration YAML fragments.
These fragements are merged together and into a full configuration and tested for validity. All previous configuration functionality remains unchanged.