config: add macro support for name and description fields#578
config: add macro support for name and description fields#578mostlygeek merged 1 commit intomainfrom
Conversation
Extend macro substitution to the name and description fields of ModelConfig, matching the behavior already present for cmd, proxy, checkEndpoint, and filters. - substitute global/model macros (including MODEL_ID) in name and description - substitute PORT macro in name and description when allocated - validate no unknown macros remain in name and description after substitution - add tests for macro substitution, MODEL_ID, and unknown macro error
WalkthroughThe PR adds Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
proxy/config/config.go (1)
343-358: Consider adding a clearer error message for${PORT}usage in name/description without cmd.If a user specifies
${PORT}innameordescriptionbut not incmd, the PORT macro won't be substituted (this block is skipped), and validation will produce "macro '${PORT}' should have been substituted" which is slightly confusing.Consider checking for this edge case explicitly for a more helpful error message:
💡 Optional improvement for clearer error messaging
// Handle PORT macro - only allocate if cmd uses it cmdHasPort := strings.Contains(modelConfig.Cmd, "${PORT}") proxyHasPort := strings.Contains(modelConfig.Proxy, "${PORT}") + nameHasPort := strings.Contains(modelConfig.Name, "${PORT}") + descHasPort := strings.Contains(modelConfig.Description, "${PORT}") + + if (nameHasPort || descHasPort) && !cmdHasPort { + return Config{}, fmt.Errorf("model %s: name/description uses ${PORT} but cmd does not - ${PORT} is only available when used in cmd", modelId) + } + if cmdHasPort || proxyHasPort {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@proxy/config/config.go` around lines 343 - 358, Detect when the ${PORT} macro appears in modelConfig.Name or modelConfig.Description while modelConfig.Cmd does not contain ${PORT} (same situation already checked for modelConfig.Proxy) and return a clear error (e.g., "model %s: ${PORT} found in name/description but cmd does not contain ${PORT} — ${PORT} is only substituted when used in cmd"). Update the existing macro handling block that checks cmdHasPort/proxyHasPort to also inspect Name and Description for "${PORT}" and produce this explicit error referencing modelId, modelConfig.Name, modelConfig.Description, and modelConfig.Cmd so it's easy to locate and fix in the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@proxy/config/config.go`:
- Around line 343-358: Detect when the ${PORT} macro appears in modelConfig.Name
or modelConfig.Description while modelConfig.Cmd does not contain ${PORT} (same
situation already checked for modelConfig.Proxy) and return a clear error (e.g.,
"model %s: ${PORT} found in name/description but cmd does not contain ${PORT} —
${PORT} is only substituted when used in cmd"). Update the existing macro
handling block that checks cmdHasPort/proxyHasPort to also inspect Name and
Description for "${PORT}" and produce this explicit error referencing modelId,
modelConfig.Name, modelConfig.Description, and modelConfig.Cmd so it's easy to
locate and fix in the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 58b553d5-a21f-4869-97af-c78921bffc53
📒 Files selected for processing (2)
proxy/config/config.goproxy/config/macro_in_macro_test.go
Extend macro substitution to the name and description fields of
ModelConfig, matching the behavior already present for cmd, proxy,
checkEndpoint, and filters.