Support Macro-In-Macro #337
Conversation
WalkthroughReplaces map-based macros with an ordered Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant YAML as YAML input
participant Loader as LoadConfigFromReader
participant Parser as MacroList.UnmarshalYAML
participant Merger as MergeMacros
participant Validator as ValidateMacros
participant Substitutor as LIFO Substitutor
note right of Parser #E8F4FF: preserve declaration order
YAML->>Loader: provide config (globals, models, macros)
Loader->>Parser: parse macros into ordered []MacroEntry
Parser-->>Loader: ordered MacroList
Loader->>Merger: build merged list (global → model → reserved MODEL_ID)
Merger-->>Loader: merged ordered macros
Loader->>Validator: check lengths, reserved usage, self-references
Validator-->>Loader: ok / error
Loader->>Substitutor: reverse-iterate merged macros (LIFO)
Substitutor->>Loader: recursively substitute into Cmd/Proxy/metadata/etc.
alt unknown macro found
Substitutor-->>Loader: return error (unknown macro)
end
Loader-->>Caller: final config or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 2
🧹 Nitpick comments (1)
ai-plans/issue-336-macro-in-macro.md (1)
81-85: Add a language hint to this fenced blockMarkdown lint (MD040) is flagging this snippet because it lacks a language identifier. Tag it with something like ```text to clear the warning.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ai-plans/issue-336-macro-in-macro.md(1 hunks)proxy/config/config.go(7 hunks)proxy/config/config_posix_test.go(1 hunks)proxy/config/config_test.go(3 hunks)proxy/config/config_windows_test.go(1 hunks)proxy/config/macro_in_macro_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Fix all staticcheck-reported issues in Go code
Files:
proxy/config/config_windows_test.goproxy/config/macro_in_macro_test.goproxy/config/config_test.goproxy/config/config_posix_test.goproxy/config/config.go
ai-plans/**
📄 CodeRabbit inference engine (CLAUDE.md)
ai-plans/**: Plans live in ai-plans/ and, when improving a plan, identify any inconsistencies
Expand user-written plans into detailed specifications of requirements and changes
Ensure every plan includes sections: Title, Overview, Design Requirements, Testing Plan, Checklist
While implementing a plan, update the plan’s Checklist as items are completed
Files:
ai-plans/issue-336-macro-in-macro.md
🧬 Code graph analysis (1)
proxy/config/macro_in_macro_test.go (1)
proxy/config/config.go (1)
LoadConfigFromReader(172-414)
🪛 markdownlint-cli2 (0.18.1)
ai-plans/issue-336-macro-in-macro.md
3-3: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
81-81: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
117-117: Hard tabs
Column: 1
(MD010, no-hard-tabs)
122-122: Hard tabs
Column: 1
(MD010, no-hard-tabs)
123-123: Hard tabs
Column: 1
(MD010, no-hard-tabs)
124-124: Hard tabs
Column: 1
(MD010, no-hard-tabs)
125-125: Hard tabs
Column: 1
(MD010, no-hard-tabs)
126-126: Hard tabs
Column: 1
(MD010, no-hard-tabs)
127-127: Hard tabs
Column: 1
(MD010, no-hard-tabs)
128-128: Hard tabs
Column: 1
(MD010, no-hard-tabs)
129-129: Hard tabs
Column: 1
(MD010, no-hard-tabs)
130-130: Hard tabs
Column: 1
(MD010, no-hard-tabs)
131-131: Hard tabs
Column: 1
(MD010, no-hard-tabs)
132-132: Hard tabs
Column: 1
(MD010, no-hard-tabs)
133-133: Hard tabs
Column: 1
(MD010, no-hard-tabs)
141-141: Hard tabs
Column: 1
(MD010, no-hard-tabs)
142-142: Hard tabs
Column: 1
(MD010, no-hard-tabs)
143-143: Hard tabs
Column: 1
(MD010, no-hard-tabs)
144-144: Hard tabs
Column: 1
(MD010, no-hard-tabs)
146-146: Hard tabs
Column: 1
(MD010, no-hard-tabs)
147-147: Hard tabs
Column: 1
(MD010, no-hard-tabs)
148-148: Hard tabs
Column: 1
(MD010, no-hard-tabs)
154-154: Hard tabs
Column: 1
(MD010, no-hard-tabs)
155-155: Hard tabs
Column: 1
(MD010, no-hard-tabs)
156-156: Hard tabs
Column: 1
(MD010, no-hard-tabs)
158-158: Hard tabs
Column: 1
(MD010, no-hard-tabs)
159-159: Hard tabs
Column: 1
(MD010, no-hard-tabs)
160-160: Hard tabs
Column: 1
(MD010, no-hard-tabs)
161-161: Hard tabs
Column: 1
(MD010, no-hard-tabs)
162-162: Hard tabs
Column: 1
(MD010, no-hard-tabs)
163-163: Hard tabs
Column: 1
(MD010, no-hard-tabs)
165-165: Hard tabs
Column: 1
(MD010, no-hard-tabs)
166-166: Hard tabs
Column: 1
(MD010, no-hard-tabs)
167-167: Hard tabs
Column: 1
(MD010, no-hard-tabs)
168-168: Hard tabs
Column: 1
(MD010, no-hard-tabs)
169-169: Hard tabs
Column: 1
(MD010, no-hard-tabs)
170-170: Hard tabs
Column: 1
(MD010, no-hard-tabs)
171-171: Hard tabs
Column: 1
(MD010, no-hard-tabs)
172-172: Hard tabs
Column: 1
(MD010, no-hard-tabs)
182-182: Hard tabs
Column: 1
(MD010, no-hard-tabs)
183-183: Hard tabs
Column: 1
(MD010, no-hard-tabs)
185-185: Hard tabs
Column: 1
(MD010, no-hard-tabs)
186-186: Hard tabs
Column: 1
(MD010, no-hard-tabs)
187-187: Hard tabs
Column: 1
(MD010, no-hard-tabs)
188-188: Hard tabs
Column: 1
(MD010, no-hard-tabs)
189-189: Hard tabs
Column: 1
(MD010, no-hard-tabs)
190-190: Hard tabs
Column: 1
(MD010, no-hard-tabs)
191-191: Hard tabs
Column: 1
(MD010, no-hard-tabs)
192-192: Hard tabs
Column: 1
(MD010, no-hard-tabs)
193-193: Hard tabs
Column: 1
(MD010, no-hard-tabs)
194-194: Hard tabs
Column: 1
(MD010, no-hard-tabs)
195-195: Hard tabs
Column: 1
(MD010, no-hard-tabs)
197-197: Hard tabs
Column: 1
(MD010, no-hard-tabs)
198-198: Hard tabs
Column: 1
(MD010, no-hard-tabs)
199-199: Hard tabs
Column: 1
(MD010, no-hard-tabs)
200-200: Hard tabs
Column: 1
(MD010, no-hard-tabs)
201-201: Hard tabs
Column: 1
(MD010, no-hard-tabs)
202-202: Hard tabs
Column: 1
(MD010, no-hard-tabs)
203-203: Hard tabs
Column: 1
(MD010, no-hard-tabs)
204-204: Hard tabs
Column: 1
(MD010, no-hard-tabs)
205-205: Hard tabs
Column: 1
(MD010, no-hard-tabs)
206-206: Hard tabs
Column: 1
(MD010, no-hard-tabs)
207-207: Hard tabs
Column: 1
(MD010, no-hard-tabs)
209-209: Hard tabs
Column: 1
(MD010, no-hard-tabs)
210-210: Hard tabs
Column: 1
(MD010, no-hard-tabs)
211-211: Hard tabs
Column: 1
(MD010, no-hard-tabs)
212-212: Hard tabs
Column: 1
(MD010, no-hard-tabs)
213-213: Hard tabs
Column: 1
(MD010, no-hard-tabs)
214-214: Hard tabs
Column: 1
(MD010, no-hard-tabs)
215-215: Hard tabs
Column: 1
(MD010, no-hard-tabs)
216-216: Hard tabs
Column: 1
(MD010, no-hard-tabs)
217-217: Hard tabs
Column: 1
(MD010, no-hard-tabs)
218-218: Hard tabs
Column: 1
(MD010, no-hard-tabs)
219-219: Hard tabs
Column: 1
(MD010, no-hard-tabs)
221-221: Hard tabs
Column: 1
(MD010, no-hard-tabs)
222-222: Hard tabs
Column: 1
(MD010, no-hard-tabs)
223-223: Hard tabs
Column: 1
(MD010, no-hard-tabs)
224-224: Hard tabs
Column: 1
(MD010, no-hard-tabs)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
proxy/config/config.go (1)
334-348: Error messages should distinguish truly unknown macros from unresolved ones.Currently, line 346 reports "unknown macro" for any macro that wasn't substituted. However, this doesn't distinguish between:
- A macro that was never declared (truly unknown)
- A macro that was declared but couldn't be resolved (e.g., due to ordering issues)
After fixing the LIFO substitution bug (see previous comment), consider tracking which macros exist in
mergedMacrosand providing more specific errors:macroName := match[1] + // Check if macro exists in our merged list + _, macroExists := mergedMacros.Get(macroName) + if macroName == "PID" && fieldName == "cmdStop" { continue // this is ok, has to be replaced by process later } // Reserved macros are always valid (they should have been substituted already) if macroName == "PORT" || macroName == "MODEL_ID" { return Config{}, fmt.Errorf("macro '${%s}' should have been substituted in %s.%s", macroName, modelId, fieldName) } - // Any other macro is unknown - return Config{}, fmt.Errorf("unknown macro '${%s}' found in %s.%s", macroName, modelId, fieldName) + if macroExists { + return Config{}, fmt.Errorf("macro '${%s}' could not be resolved in %s.%s; it may reference a later-declared macro or have circular dependencies", macroName, modelId, fieldName) + } else { + return Config{}, fmt.Errorf("unknown macro '${%s}' found in %s.%s", macroName, modelId, fieldName) + }This provides clearer guidance to users debugging their configurations.
🧹 Nitpick comments (2)
proxy/config/config.go (2)
507-511: Self-reference check prevents direct cycles.The validation correctly prevents
foo: ${foo}bar. Indirect circular references likea: ${b}andb: ${a}will be caught during the resolution phase when macros remain unresolved.For more user-friendly errors, consider adding explicit circular dependency detection:
// After validation, before substitution: visited := make(map[string]bool) stack := make(map[string]bool) func checkCycles(macroName string) error { if stack[macroName] { return fmt.Errorf("circular dependency detected involving macro '%s'", macroName) } if visited[macroName] { return nil } visited[macroName] = true stack[macroName] = true defer func() { stack[macroName] = false }() // Check dependencies... return nil }However, this adds complexity and the current approach of catching unresolved macros may be sufficient.
246-266: Consider simplifying the macro merging logic.The nested loop (lines 254-265) correctly implements override semantics but is verbose and has O(n*m) complexity.
After fixing the
Setmethod to have consistent behavior (see earlier comment), you could simplify this to:// Merge global config and model macros. Model macros take precedence - mergedMacros := make(MacroList, 0, len(config.Macros)+len(modelConfig.Macros)) - mergedMacros = append(mergedMacros, MacroEntry{Name: "MODEL_ID", Value: modelId}) - - // Add global macros first - mergedMacros = append(mergedMacros, config.Macros...) - - // Add model macros (can override global) - for _, entry := range modelConfig.Macros { - // Remove any existing global macro with same name - found := false - for i, existing := range mergedMacros { - if existing.Name == entry.Name { - mergedMacros[i] = entry // Override - found = true - break - } - } - if !found { - mergedMacros = append(mergedMacros, entry) - } - } + mergedMacros := MacroList{MacroEntry{Name: "MODEL_ID", Value: modelId}} + mergedMacros = append(mergedMacros, config.Macros...) + + // Model macros override globals + for _, entry := range modelConfig.Macros { + mergedMacros = mergedMacros.Set(entry.Name, entry.Value) + }This is clearer and more maintainable, though it has the same time complexity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
proxy/config/config.go(7 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.go
🔇 Additional comments (9)
proxy/config/config.go (9)
18-24: LGTM! Clear type definitions for ordered macro storage.The
MacroEntryandMacroListtypes provide a clean foundation for preserving YAML declaration order, which is essential for the macro-in-macro feature.
26-52: LGTM! Correct YAML unmarshaling implementation.The custom unmarshaling correctly preserves macro declaration order by processing YAML's alternating key/value node structure.
54-62: LGTM!The
Getmethod provides a straightforward lookup with appropriate return semantics.
76-83: LGTM!The
ToMapconversion is straightforward and correct for backward compatibility scenarios.
293-323: PORT macro handling looks correct.The logic correctly:
- Defers PORT allocation until after user macro substitution
- Detects PORT usage in both cmd and proxy
- Enforces that proxy can only use PORT if cmd also does
- Substitutes PORT in command fields and metadata
This allows users to reference
${PORT}within their own macros, which will be preserved during user macro substitution and then resolved here.
526-557: Recursive metadata validation is correct.The function properly recurses through nested structures (maps, slices, strings) to detect any remaining macro references.
Note: The error message at line 533 says "unknown macro" but has the same limitation as the field validation (doesn't distinguish unknown from unresolved). This should be addressed consistently with the fix suggested for lines 334-348.
559-605: LGTM! Recursive substitution handles complex metadata correctly.The function correctly handles:
- Direct macro replacement while preserving type (lines 568-570)
- String interpolation (lines 572-574)
- Recursive processing of nested maps and slices
This enables macros to be used in complex metadata structures.
217-221: LGTM!The validation loop correctly iterates over the ordered
MacroList.
239-243: LGTM!Model macro validation correctly iterates over
MacroListand provides good error context with the model ID.
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)
config.example.yaml(1 hunks)proxy/config/config.go(7 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.go
🧠 Learnings (2)
📓 Common learnings
Learnt from: mostlygeek
PR: mostlygeek/llama-swap#337
File: proxy/config/config.go:268-291
Timestamp: 2025-10-07T05:41:52.692Z
Learning: In the mostlygeek/llama-swap repository's proxy/config/config.go, macro-in-macro substitution requires that referenced macros be declared earlier in the YAML file (declaration order matters). A macro can only reference macros that appear before it in the configuration. The YAML order is preserved by the UnmarshalYAML implementation for MacroList, and the LIFO substitution approach is intentional.
📚 Learning: 2025-10-07T05:41:52.692Z
Learnt from: mostlygeek
PR: mostlygeek/llama-swap#337
File: proxy/config/config.go:268-291
Timestamp: 2025-10-07T05:41:52.692Z
Learning: In the mostlygeek/llama-swap repository's proxy/config/config.go, macro-in-macro substitution requires that referenced macros be declared earlier in the YAML file (declaration order matters). A macro can only reference macros that appear before it in the configuration. The YAML order is preserved by the UnmarshalYAML implementation for MacroList, and the LIFO substitution approach is intentional.
Applied to files:
proxy/config/config.go
This refactors macros to better support macro when macros contain macros. Now all macros can contain another macro as long as that macro has been already declared.
Fixes #336
Notes about this AI coding session
This one took a lot longer and more manual effort than anticipated:
gopkg.in/yaml.v3' was attempted to be replaced withgoccy/go-yaml`. This was too complex of a change and the result was:yaml.v3specific default values did not cleanly port over togo-yaml. This really confused claude and it wrote and changed a lot of tests. Keepingyaml.v3and using theyaml.Nodeapproach for created an ordered map was much simpler${PORT}macro is special. I wanted macro substitution to be done in entirely in a single pass. This was mostly possible except for${PORT}because of the additional port allocation logic around it. Claude actually understood this and did a second macro pass for${PORT}. I thought it misunderstood the plan and wasted a bunch of time refactoring. The lesson here is sometimes as the human I forget the requirements and jumping to conclusion that the AI was wrong was the mistake.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests