proxy/config: add model level macros #330
Conversation
Add macros to model configuration. Model macros override macros that are defined at the global configuration level. They follow the same naming and value rules as the global macros.
WalkthroughAdds a validated global and per-model macro system, merges/expands macros into model fields, relocates model-specific configuration into new ModelConfig/ModelFilters types with YAML unmarshalling and sanitizers, updates example config and tests for per-model overrides and reserved-name validations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User
participant Loader as LoadConfigFromReader
participant YAML as YAML Unmarshal
participant GVal as validateMacro (global)
participant MVal as validateMacro (per-model)
participant Merge as Merge Macros
participant Expand as Macro Expansion
participant Sanitize as SanitizedCommand / SanitizedStripParams
User->>Loader: provide config reader
Loader->>YAML: unmarshal Config (globals + models)
YAML->>GVal: validate global macros
GVal-->>YAML: ok / error
loop per model
YAML->>MVal: validate model macros
MVal-->>YAML: ok / error
YAML->>Merge: merge macros (model overrides global)
Merge-->>Expand: merged macro set
Expand->>Sanitize: expand macros in Cmd/CmdStop/Proxy/CheckEndpoint/Filters
Sanitize-->>YAML: sanitized values or error
end
YAML-->>Loader: return validated, expanded Config
Loader-->>User: Config or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
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 (5)
config.example.yaml(3 hunks)proxy/config/config.go(5 hunks)proxy/config/config_test.go(2 hunks)proxy/config/model_config.go(1 hunks)proxy/config/model_config_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-07T04:50:07.200Z
Learnt from: brettp
PR: mostlygeek/llama-swap#226
File: proxy/config.go:305-313
Timestamp: 2025-08-07T04:50:07.200Z
Learning: In the llama-swap project, model IDs are treated as trusted input from YAML configuration files. Additional validation for MODEL_ID macro expansion is not needed because if someone can control model IDs in the config, they can already inject malicious content in other fields like cmd. The configuration file is the trusted source.
Applied to files:
config.example.yaml
🧬 Code graph analysis (2)
proxy/config/model_config.go (1)
proxy/config/config.go (2)
MacroList(19-19)SanitizeCommand(323-356)
proxy/config/model_config_test.go (2)
proxy/config/model_config.go (1)
ModelConfig(9-33)proxy/config/config.go (1)
LoadConfigFromReader(103-276)
⏰ 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_test.go (1)
202-211: Per-model macro precedence exercised effectivelyNice job extending the fixture to prove model-level macros win over global ones; this guards the new merge logic and catches regressions around override order.
Also applies to: 219-219
proxy/config/model_config_test.go (2)
10-20: Targeted coverage forSanitizedCommandExercising
ModelConfig.SanitizedCommandin isolation is a solid addition—it keeps the parser behaviour verified even when config loading isn’t involved.
22-44: Filter macro expansion and cleanup verifiedThanks for asserting both the raw, macro-expanded
StripParamsvalue and the sanitized slice; this protects the macro merge and de-duplication path end to end.
The PORT reserved name was not properly checked
mostlygeek
force-pushed
the
model-macros
branch
from
1ff5079 to
2793ea7
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- add check that model.filters.stripParams has no invalid macros - renamed strip_params to stripParams for camel case consistency - add legacy code to strip_params continues to work
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
proxy/config/model_config.go (1)
100-121: Dedup + sanitize fixed; prefer sort.Strings for broader Go compatibility.To avoid a Go 1.21+ stdlib requirement on slices.Sort, switch to sort.Strings. Optional: use map[string]struct{} to shave a few allocs; optional: treat "model" case‑insensitively if the spec allows.
Apply this diff to replace slices.Sort with sort.Strings:
@@ - // sort cleaned - slices.Sort(cleaned) + // sort cleaned + sort.Strings(cleaned) return cleaned, nil }Also update imports accordingly:
@@ -import ( - "errors" - "runtime" - "slices" - "strings" -) +import ( + "errors" + "runtime" + "sort" + "strings" +)Optional micro-tweak (if desired):
- seen := make(map[string]bool) + seen := make(map[string]struct{}, len(params)) @@ - if trimmed == "model" || trimmed == "" || seen[trimmed] { + // block the "model" param; make case-insensitive if required by spec + if trimmed == "model" || trimmed == "" { continue } - seen[trimmed] = true + if _, ok := seen[trimmed]; ok { + continue + } + seen[trimmed] = struct{}{}Please confirm the repository targets Go ≥ 1.21 in go.mod; if not, the switch to sort.Strings is required.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
proxy/config/model_config.go(1 hunks)proxy/config/model_config_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- proxy/config/model_config_test.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: brettp
PR: mostlygeek/llama-swap#226
File: proxy/config.go:305-313
Timestamp: 2025-08-07T04:50:07.200Z
Learning: In the llama-swap project, model IDs are treated as trusted input from YAML configuration files. Additional validation for MODEL_ID macro expansion is not needed because if someone can control model IDs in the config, they can already inject malicious content in other fields like cmd. The configuration file is the trusted source.
🧬 Code graph analysis (1)
proxy/config/model_config.go (1)
proxy/config/config.go (2)
MacroList(19-19)SanitizeCommand(324-357)
🔇 Additional comments (5)
proxy/config/model_config.go (5)
10-34: Per‑model macros field looks good; please verify override/validation path.Confirm that config.go merges global + per‑model MacroList with model-level taking precedence and that reserved-name validation is applied consistently. No extra validation needed for MODEL_ID expansions per prior guidance.
Based on learnings
36-64: YAML defaulting behavior LGTM.OS-specific CmdStop default before unmarshal is sane; user values still override it.
66-69: Command sanitizer delegation LGTM.Forwarding to SanitizeCommand keeps logic centralized.
71-74: Filters struct LGTM.Field naming matches current schema; legacy support handled below.
75-99: Legacy strip_params fallback is clear and safe.Good precedence: prefer stripParams, else read strip_params; bubbling type errors from legacy is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config.example.yaml (2)
106-106: Proxy port likely mismatches ${PORT} used in cmdThis model’s cmd uses ${PORT} (auto‑assigned from startPort), but proxy is hardcoded to 8999. That will route requests to the wrong port.
Apply one of the following:
- proxy: http://127.0.0.1:8999 + proxy: http://127.0.0.1:${PORT}or remove proxy entirely so the default http://localhost:${PORT} applies.
199-201: Minor grammar fixes in comments
- “to run a time” → “to run at a time”
- “prevents over groups” → “prevents other groups”
- # to run a time across the whole llama-swap instance + # to run at a time across the whole llama-swap instance ... - # persistent: prevents over groups from unloading the models in this group + # persistent: prevents other groups from unloading the models in this groupAlso applies to: 236-238
🧹 Nitpick comments (1)
config.example.yaml (1)
41-49: Reserve PID and clarify single‑pass expansion scope
- Add PID to reserved macro names (it’s documented as available for cmdStop).
- Clarify whether macros used inside macro values (e.g., a macro value containing ${MODEL_ID}) will be expanded in the same single pass; if not, remove the “also available in macros defined above” implication to avoid confusion.
Proposed doc tweak:
-# - macro names must not be a reserved name: PORT or MODEL_ID +# - macro names must not be a reserved name: PORT, MODEL_ID, or PID # - macro values must be less than 1024 characters # -# Important: do not nest macros inside other macros; expansion is single-pass +# Important: expansion is single-pass across target fields (cmd, cmdStop, proxy, checkEndpoint, filters.stripParams). +# Do not rely on recursive expansion; reserved macros (PORT, MODEL_ID, PID) are injected and cannot be overridden.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config.example.yaml(4 hunks)proxy/config/model_config_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- proxy/config/model_config_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-07T04:50:07.200Z
Learnt from: brettp
PR: mostlygeek/llama-swap#226
File: proxy/config.go:305-313
Timestamp: 2025-08-07T04:50:07.200Z
Learning: In the llama-swap project, model IDs are treated as trusted input from YAML configuration files. Additional validation for MODEL_ID macro expansion is not needed because if someone can control model IDs in the config, they can already inject malicious content in other fields like cmd. The configuration file is the trusted source.
Applied to files:
config.example.yaml
🔇 Additional comments (4)
config.example.yaml (4)
53-53: Global macro example looks goodUsing a string for numeric ctx is correct and aligns with macro string substitution.
65-71: Per‑model macro override is clear and correctGood example demonstrating precedence of model macros over global ones.
81-81: Good illustrative usage of a model‑level macro in cmdNo issues.
137-146: stripParams parsing and trimming correct
ModelFiltersunmarshal handles bothstripParams(camelCase) and legacystrip_params.SanitizedStripParamssplits on commas, trims whitespace, deduplicates, and drops empty entries (verified by model_config_test.go).
* proxy/config: add model level macros Add macros to model configuration. Model macros override macros that are defined at the global configuration level. They follow the same naming and value rules as the global macros. * proxy/config: fix bug with macro reserved name checking The PORT reserved name was not properly checked * proxy/config: add tests around model.filters.stripParams - add check that model.filters.stripParams has no invalid macros - renamed strip_params to stripParams for camel case consistency - add legacy code compatibility so model.filters.strip_params continues to work * proxy/config: add duplicate removal to model.filters.stripParams * clean up some doc nits
Add macros to model configuration. Model macros override macros that are defined at the global configuration level. They follow the same naming and value rules as the global macros.
Updates: #264
Summary by CodeRabbit
New Features
Bug Fixes
Tests