proxy/config: fix inconsistent replacement order for macros#335
proxy/config: fix inconsistent replacement order for macros#335brettp wants to merge 1 commit intomostlygeek:mainfrom
Conversation
WalkthroughCentralizes recursive macro expansion in proxy/config/config.go via a new replaceMacros helper (max 5 iterations) with per-model/field error context, retains port enforcement and metadata substitution, and adds tests in proxy/config/config_test.go to validate the 5-iteration cap and updated model fixtures. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App
participant Loader as ConfigLoader
participant Model as ModelEntry
participant Macro as replaceMacros
App->>Loader: Load config file
Loader->>Model: For each model -> build mergedMacros (global + model)
loop For each macro-enabled field (cmd, cmdStop, proxy, metadata, ...)
Loader->>Macro: replaceMacros(fieldValue, mergedMacros)
alt converges within 5 iterations
Macro-->>Loader: expandedValue
Loader->>Loader: apply port enforcement/validation
else exceeds 5 iterations
Macro-->>Loader: error (modelId, field)
Loader-->>App: fail load with contextual error
end
end
Loader-->>App: validated, sanitized config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proxy/config/config.go(1 hunks)proxy/config/config_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Fix all staticcheck-reported issues in Go code
Files:
proxy/config/config_test.goproxy/config/config.go
🧬 Code graph analysis (1)
proxy/config/config_test.go (1)
proxy/config/config.go (2)
LoadConfigFromReader(108-319)SanitizeCommand(366-399)
🪛 GitHub Actions: Linux CI
proxy/config/config_test.go
[error] 1-1: gofmt formatting check failed for Go files. Run 'gofmt -w' to fix code style issues.
proxy/config/config.go
[error] 1-1: gofmt formatting check failed for Go files. Run 'gofmt -w' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-tests
🔇 Additional comments (4)
proxy/config/config.go (2)
214-228: Well done centralizing macro expansion!The new
macrosFieldMapapproach provides a cleaner, more maintainable solution for macro expansion across multiple fields. The per-field error context (line 225) will help users quickly identify which field has problematic macros.
193-211: 5 iterations cap is sufficient for current macro usage
No nested macro definitions exceeding a single level were found in any YAML config; the 5-iteration limit safely handles all existing use cases.proxy/config/config_test.go (2)
764-779: Excellent test for the recursion limit!This test correctly validates that circular macro references are caught and reported with appropriate context. The circular dependency between
macro_aandmacro_bensures the 5-iteration limit is reached, and the error message assertion confirms proper error formatting.
499-522: Port assertions correctly updated for alphabetical ordering.The addition of the "abc" model and the updated port number assertions correctly reflect the alphabetical sorting of model IDs (as implemented in config.go line 164). The port assignments now follow the sequence: abc→9000, author/model:F16→9001, model1→9002, model2→9003.
proxy/config/config.go
Outdated
| return result, nil | ||
| } | ||
| } | ||
| return "", fmt.Errorf("macro expansion exceeded 5 iterations") |
There was a problem hiding this comment.
Fix the indentation to resolve gofmt failure.
Line 210 has inconsistent indentation (likely mixing spaces and tabs) which is causing the gofmt check to fail in the CI pipeline.
Run gofmt -w proxy/config/config.go to automatically fix the formatting, or manually ensure the line uses tabs consistently with the rest of the file.
🤖 Prompt for AI Agents
In proxy/config/config.go around line 210, the indentation on the return
statement is inconsistent (mix of spaces/tabs) causing gofmt to fail; fix by
normalizing indentation to use tabs consistent with the file (run `gofmt -w
proxy/config/config.go` or replace leading spaces with a tab on that line) so
the file passes the gofmt check.
brettp
force-pushed
the
macro-expansion
branch
from
d13944d to
4fc18de
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
proxy/config/config.go (1)
192-212: LGTM! Consider extracting the closure for clarity.The recursive macro expansion logic is correct and properly handles the 5-iteration limit. The early return when no changes are detected is efficient.
Consider extracting
replaceMacrosas a standalone function outside the loop to improve readability and avoid recreating it for each model:+// replaceMacros recursively replaces macros in the input string up to 5 iterations +func expandMacros(input string, mergedMacros MacroList) (string, error) { + result := input + for range 5 { + changed := false + for macroName, macroValue := range mergedMacros { + macroSlug := fmt.Sprintf("${%s}", macroName) + macroStr := fmt.Sprintf("%v", macroValue) + if strings.Contains(result, macroSlug) { + result = strings.ReplaceAll(result, macroSlug, macroStr) + changed = true + } + } + if !changed { + return result, nil + } + } + return "", fmt.Errorf("macro expansion exceeded 5 iterations") +}Then update the usage:
- replaceMacros := func(input string) (string, error) { - result := input - for range 5 { - changed := false - for macroName, macroValue := range mergedMacros { - macroSlug := fmt.Sprintf("${%s}", macroName) - // Convert macro value to string for command/string field substitution - macroStr := fmt.Sprintf("%v", macroValue) - if strings.Contains(result, macroSlug) { - result = strings.ReplaceAll(result, macroSlug, macroStr) - changed = true - } - } - if !changed { - return result, nil - } - } - - return "", fmt.Errorf("macro expansion exceeded 5 iterations") - } - // go through model config fields: cmd, cmdStop, proxy, checkEndPoint and replace macros with macro values macrosFieldMap := map[string]*string{ "cmd": &modelConfig.Cmd, "cmdStop": &modelConfig.CmdStop, "proxy": &modelConfig.Proxy, "checkEndpoint": &modelConfig.CheckEndpoint, "filters.stripParams": &modelConfig.Filters.StripParams, } for sectionName, ptr := range macrosFieldMap { - expanded, err := replaceMacros(*ptr) + expanded, err := expandMacros(*ptr, mergedMacros) if err != nil { return Config{}, fmt.Errorf("%s for %s.%s", err.Error(), modelId, sectionName) } *ptr = expanded }proxy/config/config_test.go (1)
498-501: LGTM! Port assignments correctly updated.The addition of the "abc" model and the updated port numbers correctly reflect the sorted model order:
abc(port 9000)author/model:F16(port 9001)model1(port 9002)model2(port 9003)Consider adding an assertion for the "abc" model's command to make the test more comprehensive:
sanitizedCmdAbc, err := SanitizeCommand(config.Models["abc"].Cmd) assert.NoError(t, err) assert.Equal(t, "docker run --name abc -p 9000:8080 docker_img", strings.Join(sanitizedCmdAbc, " "))Also applies to: 507-507, 513-513, 521-521
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proxy/config/config.go(1 hunks)proxy/config/config_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Fix all staticcheck-reported issues in Go code
Files:
proxy/config/config.goproxy/config/config_test.go
🧬 Code graph analysis (1)
proxy/config/config_test.go (1)
proxy/config/config.go (2)
LoadConfigFromReader(108-320)SanitizeCommand(367-400)
🪛 GitHub Actions: Linux CI
proxy/config/config_test.go
[error] 1-1: Go fmt check failed. Code is not properly formatted. Run 'gofmt -w' to format the code.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-tests
🔇 Additional comments (3)
proxy/config/config.go (2)
215-229: LGTM! Clean and maintainable approach.The use of
macrosFieldMapwith pointers effectively eliminates code duplication and ensures uniform macro expansion across all fields. The per-model and per-field error context (line 226) will be helpful for debugging.
1-502: Fix gofmt formatting in proxy/config/config.goCI is failing due to formatting issues. Run:
gofmt -s -w proxy/config/config.goto apply the required formatting changes, then commit the updated file.
proxy/config/config_test.go (1)
764-779: LGTM! Excellent test coverage for recursion limit.The test correctly validates that circular macro references trigger the 5-iteration limit. The error message assertion includes the model and field context, which matches the implementation in
config.go(line 226).
|
Hi, I’ve intended macro replacement be a single pass rather than a recursive one. If a macro contains a macro, llama-swap should exit with an error. The fact that it doesn’t error is an oversight / bug in the current code. |
|
I was thinking about this a bit more and it’d be nice to have both properties: single pass and embedded macros. What can be done is macros are processed in a first in, last out order. That would be: Reserved macros (PORT, MODEL_ID), Global macros then model macros. This would allow macros substitutions to be done in a single pass and allow embedding without circular references. |
|
replaced by PR #337. |
2 participants
This introduces recursive replacement for macros (max 5 iterations) to fix a race condition in v163 that can cause missed replacements due to the undefined order of
rangeover maps.The original tests I wrote for #226 seldom demonstrated this behavior, but adding a 4th model would error about 75% of the time.
Summary by CodeRabbit
Refactor
Bug Fixes
Tests