Conversation
Add ability to set/override parameters in requests to peers via filters.setParams configuration. - Add PeerFilters struct with SetParams map - Protected params (model) cannot be overridden - Apply setParams when proxying to peers - Add tests for PeerFilters fixes #453
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a unified Filters type used by models and peers (stripParams and setParams), propagates filters into peer config and model config, provides sanitization helpers, validates peer YAML, adds PeerProxy accessor, and applies filters to request JSON before local or peer forwarding. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
proxy/config/peer.go (1)
1-8: Fix gofmt formatting issues.The pipeline reports formatting issues. Run
gofmt -w .to fix.proxy/config/peer_test.go (1)
1-8: Fix gofmt formatting issues.The pipeline reports formatting issues. Run
gofmt -w .to fix.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
config.example.yamlproxy/config/peer.goproxy/config/peer_test.goproxy/peerproxy.goproxy/proxymanager.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Follow test naming conventions like
TestProxyManager_<test name>,TestProcessGroup_<test name>, etc.
Files:
proxy/config/peer_test.go
🧠 Learnings (3)
📓 Common learnings
Learnt from: mostlygeek
Repo: mostlygeek/llama-swap PR: 438
File: proxy/proxymanager.go:483-498
Timestamp: 2025-12-27T18:51:17.405Z
Learning: In proxy/proxymanager.go, peer models are intentionally added to the models list without checking for an unlisted status. Unlike local models which use ModelConfig with an Unlisted field, peer models use simple string lists in PeerConfig and do not support per-model unlisted filtering by design.
📚 Learning: 2025-12-27T18:51:17.405Z
Learnt from: mostlygeek
Repo: mostlygeek/llama-swap PR: 438
File: proxy/proxymanager.go:483-498
Timestamp: 2025-12-27T18:51:17.405Z
Learning: In proxy/proxymanager.go, peer models are intentionally added to the models list without checking for an unlisted status. Unlike local models which use ModelConfig with an Unlisted field, peer models use simple string lists in PeerConfig and do not support per-model unlisted filtering by design.
Applied to files:
proxy/peerproxy.goproxy/config/peer_test.goproxy/proxymanager.goproxy/config/peer.go
📚 Learning: 2026-01-11T06:14:48.645Z
Learnt from: CR
Repo: mostlygeek/llama-swap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T06:14:48.645Z
Learning: Applies to **/*_test.go : Follow test naming conventions like `TestProxyManager_<test name>`, `TestProcessGroup_<test name>`, etc.
Applied to files:
proxy/config/peer_test.go
🧬 Code graph analysis (2)
proxy/peerproxy.go (1)
proxy/config/peer.go (1)
PeerFilters(24-28)
proxy/config/peer_test.go (1)
proxy/config/peer.go (2)
PeerFilters(24-28)PeerConfig(15-21)
🪛 GitHub Actions: Linux CI
proxy/config/peer_test.go
[error] 1-1: gofmt formatting issues detected. Run 'gofmt -w .' to format the file.
proxy/config/peer.go
[error] 1-1: gofmt formatting issues detected. Run 'gofmt -w .' to format the file.
⏰ 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 (8)
config.example.yaml (1)
376-388: LGTM!The example configuration clearly demonstrates the new
filters.setParamsfeature with appropriate comments explaining usage, optionality, and value types. The example with OpenRouter'sprovidersettings (data_collection, allow_fallbacks) is practical and illustrative.proxy/config/peer.go (3)
10-28: LGTM!The
ProtectedPeerParamsslice andPeerFiltersstruct are well-designed. The protection of the "model" parameter is essential to prevent breaking the proxy's routing logic.
30-57: LGTM!The
SanitizedSetParamsmethod correctly:
- Handles nil/empty inputs gracefully
- Filters out protected parameters
- Returns sorted keys for deterministic iteration
- Returns nil when no params remain after filtering
59-91: LGTM!The
UnmarshalYAMLmethod properly initializesFilterswith an emptyPeerFilters{}struct, ensuring safe access toFilters.SetParamseven when not specified in the configuration.proxy/peerproxy.go (1)
109-121: LGTM!The
GetPeerFiltersmethod correctly performs a two-step lookup to retrieve peer filters for a given model ID. Returning an emptyPeerFilters{}struct on failure is a safe default that prevents nil-related issues downstream.proxy/proxymanager.go (1)
659-671: LGTM!The integration correctly applies per-peer
setParamsfilters:
- Retrieves filters only when routing to a peer
- Uses
SanitizedSetParams()to get filtered parameters and deterministic key order- Applies each parameter using
sjson.SetByteswhich handles nested structures- Proper error handling with descriptive messages
This mirrors the existing
stripParamspattern used for local models, maintaining consistency.proxy/config/peer_test.go (2)
141-254: LGTM!Excellent test coverage for
SanitizedSetParams:
- Empty/nil inputs
- Normal parameter passing
- Protected parameter filtering
- Edge case where all params are protected
- Complex nested structures
The test naming follows the
TestPeerFilters_<test name>convention as per coding guidelines.
256-289: LGTM!The test properly validates YAML unmarshalling of the new
filters.setParamsconfiguration, including:
- Non-nil
SetParamsafter loading- Primitive value extraction (temperature)
- Nested map typing and value extraction (provider.data_collection)
The test naming follows the
TestPeerConfig_<test name>convention as per coding guidelines.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Add stripParams filter for peers to match model filter capabilities. Peers now support the same filters as models (stripParams, setParams). - Add StripParams field to PeerFilters - Add SanitizedStripParams method - Apply stripParams in proxymanager for peer requests - Add tests for stripParams
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@proxy/config/peer_test.go`:
- Line 140: Fix the formatting failure by running the formatter on the test
file: run `gofmt -w proxy/config/peer_test.go` (or `go fmt ./...` from the repo
root) to rewrite the file with proper Go formatting, then stage and commit the
updated proxy/config/peer_test.go so the CI go fmt check passes.
🧹 Nitpick comments (2)
proxy/config/peer.go (1)
11-13: Consider makingProtectedPeerParamsimmutable.The exported
varcan be modified at runtime, which could lead to accidental changes. If immutability is desired, consider using a function that returns a copy instead.♻️ Optional: Return a copy to prevent mutation
-// ProtectedPeerParams is a list of parameters that cannot be set via filters.setParams -// These are protected to prevent breaking the proxy's ability to route requests correctly -var ProtectedPeerParams = []string{"model"} +// protectedPeerParams is a list of parameters that cannot be set via filters.setParams +// These are protected to prevent breaking the proxy's ability to route requests correctly +var protectedPeerParams = []string{"model"} + +// ProtectedPeerParams returns a copy of the protected peer parameters list +func ProtectedPeerParams() []string { + return append([]string{}, protectedPeerParams...) +}Note: This would require updating usages in
SanitizedSetParamsto call the function.proxy/config/peer_test.go (1)
243-251: Test doesn't deeply compare nested structures.The switch statement only compares basic types, so the "complex nested values" test case doesn't verify that nested maps and slices are correctly preserved. Consider using
reflect.DeepEqualfor comprehensive comparison.♻️ Suggested improvement for deep comparison
+import "reflect" + // Simple comparison for basic types switch v := wantValue.(type) { case string, int, float64, bool: if gotValue != v { t.Errorf("value mismatch for key %s: got %v, want %v", key, gotValue, v) } +default: + if !reflect.DeepEqual(gotValue, wantValue) { + t.Errorf("value mismatch for key %s: got %v, want %v", key, gotValue, wantValue) + } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
config.example.yamlproxy/config/peer.goproxy/config/peer_test.goproxy/proxymanager.go
🚧 Files skipped from review as they are similar to previous changes (1)
- proxy/proxymanager.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Follow test naming conventions like
TestProxyManager_<test name>,TestProcessGroup_<test name>, etc.
Files:
proxy/config/peer_test.go
🧠 Learnings (1)
📚 Learning: 2025-12-27T18:51:17.405Z
Learnt from: mostlygeek
Repo: mostlygeek/llama-swap PR: 438
File: proxy/proxymanager.go:483-498
Timestamp: 2025-12-27T18:51:17.405Z
Learning: In proxy/proxymanager.go, peer models are intentionally added to the models list without checking for an unlisted status. Unlike local models which use ModelConfig with an Unlisted field, peer models use simple string lists in PeerConfig and do not support per-model unlisted filtering by design.
Applied to files:
proxy/config/peer_test.goproxy/config/peer.go
🧬 Code graph analysis (1)
proxy/config/peer_test.go (1)
proxy/config/peer.go (2)
PeerFilters(25-33)PeerConfig(16-22)
🪛 GitHub Actions: Linux CI
proxy/config/peer_test.go
[error] 1-1: go fmt check failed. The file is not properly formatted. Run 'gofmt -w' or 'go fmt ./...' to fix formatting.
⏰ 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 (7)
config.example.yaml (1)
376-395: LGTM! Clear and comprehensive documentation.The example configuration effectively demonstrates the new peer filter capabilities, including
stripParamsfor removing parameters andsetParamsfor injecting provider-specific settings like zero-data-retention. The comments clearly document optionality, protected parameters, and allowed value types.proxy/config/peer.go (3)
15-33: LGTM!The struct definitions are well-organized with clear documentation. The
PeerFiltersstruct appropriately separates concerns and the YAML tags are correctly configured.
35-58: LGTM!The
SanitizedStripParamsmethod correctly handles all edge cases: empty strings, whitespace, duplicates, and protected "model" parameter. The sorting ensures deterministic output.
60-87: LGTM!The
SanitizedSetParamsmethod correctly filters protected parameters and returns sorted keys for deterministic iteration. The implementation is clean and handles edge cases properly.Note: The method performs a shallow copy, so nested maps/slices share references with the original. This is acceptable for read-only config usage but worth keeping in mind if the result is ever mutated.
proxy/config/peer_test.go (3)
256-289: LGTM!Good integration test verifying YAML parsing of filters with nested
setParams. The test properly validates both primitive values and nested map structures.
291-356: LGTM!Excellent test coverage for
SanitizedStripParamscovering all edge cases including empty strings, model filtering, duplicates, whitespace handling, and sorted output verification.
358-391: LGTM!Good integration test ensuring both
stripParamsandsetParamscan be used together and are parsed correctly from YAML.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| } | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix formatting issue flagged by CI.
The pipeline reports a go fmt check failure. Run gofmt -w proxy/config/peer_test.go or go fmt ./... to fix the formatting.
🤖 Prompt for AI Agents
In `@proxy/config/peer_test.go` at line 140, Fix the formatting failure by running
the formatter on the test file: run `gofmt -w proxy/config/peer_test.go` (or `go
fmt ./...` from the repo root) to rewrite the file with proper Go formatting,
then stage and commit the updated proxy/config/peer_test.go so the CI go fmt
check passes.
Add setParams filter for models to match peer filter capabilities. Models and peers now share the same filter capabilities (stripParams, setParams). - Add SetParams field to ModelFilters - Add SanitizedSetParams method - Apply setParams in proxymanager for local model requests - Add tests for setParams
Refactor to eliminate duplicated code between ModelFilters and PeerFilters: - Create shared Filters type with StripParams and SetParams - ModelFilters embeds Filters with yaml inline for backwards compatibility - PeerConfig uses Filters directly - Shared ProtectedParams prevents modification of "model" parameter - Consolidated tests in filters_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
proxy/config/filters.go (1)
9-11: Consider making ProtectedParams immutable.
ProtectedParamsis a package-levelvar, which means it can be modified at runtime. While this is likely fine for the current use case, consider if this is intentional (e.g., for testing) or if it should be more restrictive to prevent accidental modification.proxy/config/model_config.go (1)
104-108: Consider documenting that the error return is for API compatibility only.The
SanitizedStripParamsmethod always returnsnilfor the error since the underlyingFilters.SanitizedStripParams()doesn't return an error. The comment mentions "backwards compatibility" but could be clearer that the error is vestigial.proxy/config/model_config_test.go (1)
76-106: Consider asserting thestoparray value.The test configures a
stoparray with two elements but doesn't verify its content is correctly parsed. Adding an assertion would ensure array values insetParamswork correctly.Proposed assertion for stop array
assert.Equal(t, 0.7, setParams["temperature"]) assert.Equal(t, 0.9, setParams["top_p"]) + assert.Equal(t, []interface{}{"<|end|>", "<|stop|>"}, setParams["stop"]) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
config.example.yamlproxy/config/filters.goproxy/config/filters_test.goproxy/config/model_config.goproxy/config/model_config_test.goproxy/config/peer.goproxy/config/peer_test.goproxy/peerproxy.goproxy/proxymanager.go
🚧 Files skipped from review as they are similar to previous changes (3)
- proxy/peerproxy.go
- config.example.yaml
- proxy/config/peer_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
Follow test naming conventions like
TestProxyManager_<test name>,TestProcessGroup_<test name>, etc.
Files:
proxy/config/model_config_test.goproxy/config/filters_test.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: mostlygeek
Repo: mostlygeek/llama-swap PR: 438
File: proxy/proxymanager.go:483-498
Timestamp: 2025-12-27T18:51:17.405Z
Learning: In proxy/proxymanager.go, peer models are intentionally added to the models list without checking for an unlisted status. Unlike local models which use ModelConfig with an Unlisted field, peer models use simple string lists in PeerConfig and do not support per-model unlisted filtering by design.
📚 Learning: 2025-12-27T18:51:17.405Z
Learnt from: mostlygeek
Repo: mostlygeek/llama-swap PR: 438
File: proxy/proxymanager.go:483-498
Timestamp: 2025-12-27T18:51:17.405Z
Learning: In proxy/proxymanager.go, peer models are intentionally added to the models list without checking for an unlisted status. Unlike local models which use ModelConfig with an Unlisted field, peer models use simple string lists in PeerConfig and do not support per-model unlisted filtering by design.
Applied to files:
proxy/config/filters.goproxy/config/model_config.goproxy/proxymanager.goproxy/config/peer.go
📚 Learning: 2026-01-11T06:14:48.645Z
Learnt from: CR
Repo: mostlygeek/llama-swap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T06:14:48.645Z
Learning: Applies to **/*_test.go : Follow test naming conventions like `TestProxyManager_<test name>`, `TestProcessGroup_<test name>`, etc.
Applied to files:
proxy/config/filters_test.go
📚 Learning: 2025-10-29T05:26:34.964Z
Learnt from: mostlygeek
Repo: mostlygeek/llama-swap PR: 371
File: proxy/process.go:0-0
Timestamp: 2025-10-29T05:26:34.964Z
Learning: In proxy/process.go, the loading message "llama-swap loading model: {name}" intentionally uses p.ID (Process.ID) rather than the realModelName from the request context. This is the correct design choice.
Applied to files:
proxy/proxymanager.go
🧬 Code graph analysis (4)
proxy/config/model_config_test.go (2)
proxy/config/config.go (1)
LoadConfigFromReader(181-439)proxy/config/filters.go (1)
Filters(15-23)
proxy/config/filters_test.go (1)
proxy/config/filters.go (2)
Filters(15-23)ProtectedParams(11-11)
proxy/proxymanager.go (1)
proxy/config/filters.go (1)
Filters(15-23)
proxy/config/peer.go (1)
proxy/config/filters.go (1)
Filters(15-23)
🔇 Additional comments (10)
proxy/config/filters.go (2)
25-52: LGTM!The
SanitizedStripParamsimplementation correctly handles edge cases: empty input, whitespace trimming, duplicate removal, protected param filtering, and returns a sorted result.
54-81: LGTM!The
SanitizedSetParamsimplementation correctly filters protected params from the map, returns sorted keys for deterministic iteration, and handles nil/empty cases properly.proxy/config/filters_test.go (2)
9-64: LGTM!Comprehensive test coverage for
SanitizedStripParamsincluding edge cases for empty input, protected params, duplicates, whitespace handling, and empty values.
165-168: LGTM!Simple but effective test to ensure "model" remains in the protected params list.
proxy/config/peer.go (2)
14-14: LGTM!Adding the
Filtersfield toPeerConfigaligns peer configuration with model configuration, enabling consistent parameter filtering for peer requests.
17-48: LGTM!The
UnmarshalYAMLimplementation properly initializesFilterswith a zero value and maintains existing validation logic. Sanitization of filter content is correctly deferred to theSanitizedStripParamsandSanitizedSetParamsmethods at usage time.proxy/config/model_config.go (2)
75-79: LGTM!Embedding
Filterswith theyaml:",inline"tag is a clean approach that provides backwards compatibility while reusing the shared filter logic.
81-102: LGTM!The legacy
strip_paramsfield handling maintains backwards compatibility. The conditional check ensures the newstripParamsfield takes precedence when present.proxy/proxymanager.go (2)
653-662: LGTM!The setParams implementation for local models is well-structured: uses deterministic key ordering from
SanitizedSetParams(), includes proper error handling for eachsjson.SetBytescall, and returns appropriate 500 error responses on failure.
670-695: No action required. The code is correct as written.The apparent inconsistency is based on two different method signatures:
ModelFilters.SanitizedStripParams()returns([]string, error)with a backwards-compatibility wrapper (line 106 inproxy/config/model_config.go), whileFilters.SanitizedStripParams()returns only[]string(line 27 inproxy/config/filters.go). Line 639 correctly handles the error because it calls theModelFiltersmethod, while line 674 correctly omits error handling becausepeerFiltersis of typeFiltersand that method does not return an error.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| { | ||
| name: "complex nested values", | ||
| setParams: map[string]any{ | ||
| "provider": map[string]any{ | ||
| "data_collection": "deny", | ||
| "allow_fallbacks": false, | ||
| }, | ||
| "transforms": []string{"middle-out"}, | ||
| }, | ||
| wantParams: map[string]any{ | ||
| "provider": map[string]any{ | ||
| "data_collection": "deny", | ||
| "allow_fallbacks": false, | ||
| }, | ||
| "transforms": []string{"middle-out"}, | ||
| }, | ||
| wantKeys: []string{"provider", "transforms"}, | ||
| }, |
There was a problem hiding this comment.
Complex nested values are not fully asserted.
The "complex nested values" test case at lines 117-133 includes a nested map and slice, but the assertion logic at lines 156-159 only compares basic types (string, int, float64, bool). The nested provider map and transforms slice are not actually verified for correctness.
Proposed fix to assert nested values
for key, wantValue := range tt.wantParams {
gotValue, exists := gotParams[key]
assert.True(t, exists, "missing key: %s", key)
- // Simple comparison for basic types
- switch v := wantValue.(type) {
- case string, int, float64, bool:
- assert.Equal(t, v, gotValue, "value mismatch for key %s", key)
- }
+ assert.Equal(t, wantValue, gotValue, "value mismatch for key %s", key)
}Also applies to: 151-160
Update struct literal to properly initialize embedded Filters field.
2 participants
Add ability to set/override parameters in requests to peers via
filters.setParams configuration.
fixes #453
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.