Allow regular model IDs in Matrix DSL#715
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughMatrix Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config-schema.json (1)
335-344:⚠️ Potential issue | 🟠 MajorLoosen the
matrix.varsschema; it still enforces the old 1-8 alnum rule.The description and
propertyNames.patternstill reject keys likelarge_llms, so schema-backed validation in editors/CI will keep flagging the exact configs that issue#699is trying to allow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config-schema.json` around lines 335 - 344, The schema for matrix.vars currently enforces the old alphanumeric 1-8 rule via propertyNames.pattern; update that pattern to allow underscores and a longer name length (e.g. change propertyNames.pattern to "^[a-zA-Z0-9_]{1,64}$") and update the description to reflect that keys may include underscores and be longer (e.g. "Keys must be alphanumeric or underscore, 1-64 characters"). Keep the rest of the object (type, minProperties, additionalProperties) unchanged so sets and evict_costs continue to accept vars or real model IDs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@proxy/config/matrix.go`:
- Around line 168-175: The code currently allows matrix.Var keys to shadow real
model IDs causing ambiguous resolution in resolveMatrixIdent; instead add a
validation step that rejects collisions: in the matrix config validation routine
(the function that validates MatrixConfig against the models map) iterate over
each key in MatrixConfig.Var and if that key exists in the models map (the
models map of ModelConfig), return a clear validation error stating the
conflicting identifier so configs like vars.g and model id g are disallowed;
keep resolveMatrixIdent behavior unchanged once collision checks are in place.
- Around line 144-152: validateMatrixVars currently enforces the old short
alphanumeric-only rule via varKeyPattern and rejects names like "large_llms";
update the validator to accept longer, underscore-containing keys by loosening
varKeyPattern (e.g., allow underscores and increase max length) and adjust the
error message accordingly, ensuring you update uses of matrix.Var and any
related schema/tests/docs to reflect the new allowed pattern; keep the model
existence check (models map lookup) unchanged.
---
Outside diff comments:
In `@config-schema.json`:
- Around line 335-344: The schema for matrix.vars currently enforces the old
alphanumeric 1-8 rule via propertyNames.pattern; update that pattern to allow
underscores and a longer name length (e.g. change propertyNames.pattern to
"^[a-zA-Z0-9_]{1,64}$") and update the description to reflect that keys may
include underscores and be longer (e.g. "Keys must be alphanumeric or
underscore, 1-64 characters"). Keep the rest of the object (type, minProperties,
additionalProperties) unchanged so sets and evict_costs continue to accept vars
or real model IDs.
🪄 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: 0bc5caac-5f7b-443e-8589-f72339b24b2a
📒 Files selected for processing (7)
config-schema.jsonconfig.example.yamldocs/configuration.mdproxy/config/matrix.goproxy/config/matrix_dsl.goproxy/config/matrix_dsl_test.goproxy/config/matrix_test.go
| func validateMatrixVars(matrix MatrixConfig, models map[string]ModelConfig) error { | ||
| for id, modelName := range matrix.Var { | ||
| if !varKeyPattern.MatchString(id) { | ||
| return fmt.Errorf("var key %q must be alphanumeric and 1-8 characters", id) | ||
| } | ||
| if _, exists := models[modelName]; !exists { | ||
| return fmt.Errorf("var key %q references unknown model %q", id, modelName) | ||
| } | ||
| } |
There was a problem hiding this comment.
This still preserves the legacy vars restriction from issue #699.
validateMatrixVars continues to reject names like large_llms, so the linked objective of allowing longer descriptive keys with underscores is still unmet at runtime. Please loosen this validator and keep the schema/tests/docs in sync.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@proxy/config/matrix.go` around lines 144 - 152, validateMatrixVars currently
enforces the old short alphanumeric-only rule via varKeyPattern and rejects
names like "large_llms"; update the validator to accept longer,
underscore-containing keys by loosening varKeyPattern (e.g., allow underscores
and increase max length) and adjust the error message accordingly, ensuring you
update uses of matrix.Var and any related schema/tests/docs to reflect the new
allowed pattern; keep the model existence check (models map lookup) unchanged.
There was a problem hiding this comment.
Goal of this PR is to add support for model IDs, not change existing restrictions on vars. These are two separate code paths. To clarify, the relaxed validation was referring to:
There was a problem hiding this comment.
@spicyneuron, thank you for the clarification! You're right — the "relaxed validation" in the PR description refers to the changes in matrix_dsl.go (allowing / and : in DSL identifiers so real model IDs can be used directly), not to the varKeyPattern governing vars keys. These are indeed separate code paths. My comment was incorrect, and I apologize for the noise.
✏️ Learnings added
Learnt from: spicyneuron
Repo: mostlygeek/llama-swap PR: 715
File: proxy/config/matrix.go:144-152
Timestamp: 2026-04-28T01:43:14.083Z
Learning: In the llama-swap project, PR `#715` relaxes matrix DSL tokenization (in `proxy/config/matrix_dsl.go`) to accept `/` and `:` in identifiers so real model IDs can be used directly in DSL expressions. The `varKeyPattern` in `proxy/config/matrix.go` (which enforces the 1–8 alphanumeric constraint on `vars` keys) is intentionally left unchanged by this PR — changing `vars` key restrictions is a separate concern tracked by issue `#699`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: brettp
Repo: mostlygeek/llama-swap PR: 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.
|
Hi, This isn't the design I want to maintain in llama-swap. I prefer the shorter var name design. |
|
@mostlygeek Just in case something was lost in translation — the Am I missing something about how this would create maintenance burden? Happy to adjust if so. |
2 participants
While short vars for matrix logic seemed convenient at first, I found that maintaining another set of model ID to be more cumbersome than expected in practice. Especially with long, frequently updated model lists.
This PR makes
varsoptional and lets matrix sets use eithervarsor normal model IDs. Using YAML's multiline syntax, I think this is just as readable as the var-based one-liners:Also:
evict_costsworks with eithervarsor model IDs./and:.References