peer: Support SetParamsByID virtual sub-IDs#843
Conversation
Extend the peer routing and filter middleware to support virtual model sub-IDs via SetParamsByID, matching the existing model-level behaviour. - Register SetParamsByID keys in the peer routing map so requests to virtual sub-IDs (e.g. `model:thinking`) are routed to the correct peer - Add ResolveVirtualSubID to lookup a SetParamsByID key across peers and resolve its base model name (first-wins, peers sorted by ID) - Update resolveFilters to recognise virtual sub-ID requests and return the peer's filters together with the resolved base model name for UseModelName rewriting - Include virtual sub-IDs in /v1/models listings (api.go, apigroup.go) - Document setParamsByID in the peer section of config-schema.json and config.example.yaml
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds ChangesPeer
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
usrlocalben
changed the title
|
| Filename | Overview |
|---|---|
| internal/router/router.go | Adds ResolveVirtualSubID (used by filters.go) and resolvePeerBaseModel (unused, will fail staticcheck U1000) |
| internal/router/peer.go | Registers SetParamsByID keys in the peer routing map; collision detection mirrors existing model-registration logic |
| internal/server/filters.go | Adds virtual sub-ID fallback in resolveFilters; peer iteration is now sorted for determinism; applyFilters correctly uses original requested ID for SetParamsByID lookup |
| internal/server/api.go | Includes virtual sub-IDs in /v1/models with dedup via known map; sorted afterward so listing order is deterministic |
| internal/server/apigroup.go | Same virtual sub-ID inclusion as api.go for model status listing; logic is symmetric and correct |
| config-schema.json | Adds setParamsByID schema to peer filters, matching model-level structure |
| config.example.yaml | Documents setParamsByID in the peer filters section with a clear commented example |
Reviews (1): Last reviewed commit: "peer: support SetParamsByID virtual sub-..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // resolvePeerBaseModel checks whether search is a virtual sub-ID (a key in any | ||
| // peer's SetParamsByID) and returns the peer's base model name. Peers are | ||
| // visited in sorted order (first-wins for duplicates). Returns the raw search | ||
| // string when no match is found. | ||
| func resolvePeerBaseModel(cfg config.Config, search string) string { | ||
| if _, baseModel, found := ResolveVirtualSubID(cfg, search); found && baseModel != "" { | ||
| return baseModel | ||
| } | ||
| return search | ||
| } |
There was a problem hiding this comment.
resolvePeerBaseModel is never called
resolvePeerBaseModel is unexported and has no callers anywhere in the codebase (verified by search). AGENTS.md requires that all staticcheck errors be fixed before committing, and staticcheck will flag this as U1000 func resolvePeerBaseModel is unused. The PR should either call the function or remove it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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)
540-540:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate peer filter description to include
setParamsByID.Line 540 still documents only
stripParamsandsetParams, but Lines 528-536 addsetParamsByID. This creates schema-doc drift.✏️ Suggested fix
- "description": "Dictionary of filter settings for peer requests. Supports stripParams and setParams." + "description": "Dictionary of filter settings for peer requests. Supports stripParams, setParams, and setParamsByID."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config-schema.json` at line 540, Update the description text at line 540 in the config-schema.json file to include `setParamsByID` alongside `stripParams` and `setParams`. The description currently reads "Dictionary of filter settings for peer requests. Supports stripParams and setParams." but should be updated to mention all three supported options that are documented in lines 528-536, specifically adding `setParamsByID` to the list of supported filter settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/router/router.go`:
- Around line 90-99: The function resolvePeerBaseModel is flagged as unused by
golangci-lint. Either remove the entire resolvePeerBaseModel function definition
(lines 90-99) if it is not needed, or identify where this function should be
called within the codebase to resolve virtual sub-IDs and integrate it into the
appropriate call sites. Check related code that handles sub-ID resolution to
determine if this function was intended to be used but accidentally left
unwired.
In `@internal/server/api.go`:
- Around line 174-185: In internal/server/api.go lines 174-185 (anchor): Move
the declaration of the `known` map outside the peer loop so it is shared across
all peers, and sort the peers by ID before iterating them in the for loop. This
ensures each routable model/virtual ID is emitted only once in the order
determined by peer ID ordering. In internal/server/apigroup.go lines 60-71
(sibling): Apply the same global deduplication pattern by declaring a `known`
map before the peer iteration and iterating peers in sorted ID order so that
modelStatus assigns ownership to the first peer that owns each model/virtual ID,
keeping the status aligned with the routing behavior.
In `@internal/server/filters.go`:
- Around line 183-194: The resolveFilters function currently checks all
direct-model matches across all peers first, then checks virtual-ID matches, but
the peer router in internal/router/peer.go claims keys per peer in order: direct
models first, then virtual IDs for that peer. To align the filter resolution
with the router's precedence and avoid applying filters from the wrong peer in
collision cases, restructure the loop in resolveFilters to iterate through each
peer and check that peer's direct models from peer.Models first, then check that
same peer's virtual sub-IDs via ResolveVirtualSubID before moving to the next
peer in the peerIDs list. This ensures the filter returned matches the peer that
the router would actually route the request to.
---
Outside diff comments:
In `@config-schema.json`:
- Line 540: Update the description text at line 540 in the config-schema.json
file to include `setParamsByID` alongside `stripParams` and `setParams`. The
description currently reads "Dictionary of filter settings for peer requests.
Supports stripParams and setParams." but should be updated to mention all three
supported options that are documented in lines 528-536, specifically adding
`setParamsByID` to the list of supported filter settings.
🪄 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: efb8a1e1-4b26-4621-bdc5-cf3bfd7dff20
📒 Files selected for processing (7)
config-schema.jsonconfig.example.yamlinternal/router/peer.gointernal/router/router.gointernal/server/api.gointernal/server/apigroup.gointernal/server/filters.go
| // resolvePeerBaseModel checks whether search is a virtual sub-ID (a key in any | ||
| // peer's SetParamsByID) and returns the peer's base model name. Peers are | ||
| // visited in sorted order (first-wins for duplicates). Returns the raw search | ||
| // string when no match is found. | ||
| func resolvePeerBaseModel(cfg config.Config, search string) string { | ||
| if _, baseModel, found := ResolveVirtualSubID(cfg, search); found && baseModel != "" { | ||
| return baseModel | ||
| } | ||
| return search | ||
| } |
There was a problem hiding this comment.
Remove or wire resolvePeerBaseModel to avoid lint breakage.
Line 94 introduces resolvePeerBaseModel, and the current static analysis flags it as unused. If golangci-lint is enforced, this will fail CI.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 94-94: func resolvePeerBaseModel is unused
(unused)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/router/router.go` around lines 90 - 99, The function
resolvePeerBaseModel is flagged as unused by golangci-lint. Either remove the
entire resolvePeerBaseModel function definition (lines 90-99) if it is not
needed, or identify where this function should be called within the codebase to
resolve virtual sub-IDs and integrate it into the appropriate call sites. Check
related code that handles sub-ID resolution to determine if this function was
intended to be used but accidentally left unwired.
Source: Linters/SAST tools
| for peerID, peer := range s.cfg.Peers { | ||
| known := make(map[string]bool, len(peer.Models)) | ||
| for _, modelID := range peer.Models { | ||
| known[modelID] = true | ||
| data = append(data, newRecord(modelID, peerID+": "+modelID, "", map[string]any{"peerID": peerID}, config.ModelCapConfig{})) | ||
| } | ||
| for virtualID := range peer.Filters.SetParamsByID { | ||
| if known[virtualID] { | ||
| continue | ||
| } | ||
| data = append(data, newRecord(virtualID, peerID+": "+virtualID, "", map[string]any{"peerID": peerID}, config.ModelCapConfig{})) | ||
| } |
There was a problem hiding this comment.
Use global peer-ID deduping to keep listings/status aligned with routable IDs.
Both sites scope known per peer, so the same model/virtual ID can still be emitted multiple times across peers even though routing is first-wins.
internal/server/api.go#L174-L185: keep a singleknownmap across all peers and iterate peers in sorted ID order so/v1/modelsemits each routable ID once.internal/server/apigroup.go#L60-L71: apply the same global dedupe + sorted peer iteration somodelStatusmirrors routing ownership consistently.
📍 Affects 2 files
internal/server/api.go#L174-L185(this comment)internal/server/apigroup.go#L60-L71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/server/api.go` around lines 174 - 185, In internal/server/api.go
lines 174-185 (anchor): Move the declaration of the `known` map outside the peer
loop so it is shared across all peers, and sort the peers by ID before iterating
them in the for loop. This ensures each routable model/virtual ID is emitted
only once in the order determined by peer ID ordering. In
internal/server/apigroup.go lines 60-71 (sibling): Apply the same global
deduplication pattern by declaring a `known` map before the peer iteration and
iterating peers in sorted ID order so that modelStatus assigns ownership to the
first peer that owns each model/virtual ID, keeping the status aligned with the
routing behavior.
| for _, peerID := range peerIDs { | ||
| peer := cfg.Peers[peerID] | ||
| for _, m := range peer.Models { | ||
| if m == requested { | ||
| return "", peer.Filters, true | ||
| } | ||
| } | ||
| } | ||
| // Also check setParamsByID keys for virtual sub-ID requests | ||
| if p, baseModel, found := router.ResolveVirtualSubID(cfg, requested); found { | ||
| return baseModel, p.Filters, true | ||
| } |
There was a problem hiding this comment.
Align resolveFilters precedence with peer router key-claim order.
Lines 183-194 resolve all direct-model matches before virtual-ID matches, but internal/router/peer.go claims keys per peer in this order: direct models first, then setParamsByID virtual IDs. In collision cases, this can apply filters for a different peer than the one that actually receives the request.
💡 Suggested direction
- for _, peerID := range peerIDs {
- peer := cfg.Peers[peerID]
- for _, m := range peer.Models {
- if m == requested {
- return "", peer.Filters, true
- }
- }
- }
- // Also check setParamsByID keys for virtual sub-ID requests
- if p, baseModel, found := router.ResolveVirtualSubID(cfg, requested); found {
- return baseModel, p.Filters, true
- }
+ for _, peerID := range peerIDs {
+ peer := cfg.Peers[peerID]
+
+ // Match router.NewPeer precedence: direct IDs first for this peer.
+ for _, m := range peer.Models {
+ if m == requested {
+ return "", peer.Filters, true
+ }
+ }
+
+ // Then virtual IDs for this peer.
+ if _, ok := peer.Filters.SetParamsByID[requested]; ok {
+ if p, baseModel, found := router.ResolveVirtualSubID(cfg, requested); found && p != nil && p.Proxy == peer.Proxy {
+ return baseModel, peer.Filters, true
+ }
+ return "", peer.Filters, true
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, peerID := range peerIDs { | |
| peer := cfg.Peers[peerID] | |
| for _, m := range peer.Models { | |
| if m == requested { | |
| return "", peer.Filters, true | |
| } | |
| } | |
| } | |
| // Also check setParamsByID keys for virtual sub-ID requests | |
| if p, baseModel, found := router.ResolveVirtualSubID(cfg, requested); found { | |
| return baseModel, p.Filters, true | |
| } | |
| for _, peerID := range peerIDs { | |
| peer := cfg.Peers[peerID] | |
| // Match router.NewPeer precedence: direct IDs first for this peer. | |
| for _, m := range peer.Models { | |
| if m == requested { | |
| return "", peer.Filters, true | |
| } | |
| } | |
| // Then virtual IDs for this peer. | |
| if _, ok := peer.Filters.SetParamsByID[requested]; ok { | |
| if p, baseModel, found := router.ResolveVirtualSubID(cfg, requested); found && p != nil && p.Proxy == peer.Proxy { | |
| return baseModel, peer.Filters, true | |
| } | |
| return "", peer.Filters, true | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/server/filters.go` around lines 183 - 194, The resolveFilters
function currently checks all direct-model matches across all peers first, then
checks virtual-ID matches, but the peer router in internal/router/peer.go claims
keys per peer in order: direct models first, then virtual IDs for that peer. To
align the filter resolution with the router's precedence and avoid applying
filters from the wrong peer in collision cases, restructure the loop in
resolveFilters to iterate through each peer and check that peer's direct models
from peer.Models first, then check that same peer's virtual sub-IDs via
ResolveVirtualSubID before moving to the next peer in the peerIDs list. This
ensures the filter returned matches the peer that the router would actually
route the request to.
1 participant
Extend the peer routing and filter middleware to support virtual model sub-IDs via SetParamsByID, matching the existing model-level behaviour.
model:thinking) are routed to the correct peerCloses #697