build fix - #4583
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe ChangesVirtual Key IsActive nil-check fix
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
|
|
Merge activity
|
Confidence Score: 3/5The handler change introduces a logic inversion for the common DB-default case of nil IsActive, meaning provider filtering is silently skipped for the majority of active virtual keys. The nil-guard added to handle the *bool field has the wrong effect: nil IsActive means the key is active (per the DB default and the existing IsActiveValue() helper), but the new clause treats nil as inactive and bypasses ProviderConfigs filtering. This would expose all providers to any VK that was created without an explicit IsActive value, defeating the purpose of the filter. transports/bifrost-http/handlers/list_models_vk.go — the IsActive nil-check condition needs to be corrected before merging. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Request with VK header] --> B{VK header present?}
B -- No --> C[return true, no filter]
B -- Yes --> D{ConfigStore available?}
D -- No --> E[return false, 503]
D -- Yes --> F{GetVirtualKeyByValue}
F -- ErrNotFound --> C
F -- other error --> G[return false, 500]
F -- found --> H{vk == nil?}
H -- Yes --> C
H -- No --> I{vk.IsActive == nil?}
I -- Yes --> C["❌ BUG: return true (nil = active, should filter)"]
I -- No --> J{"!*vk.IsActive?"}
J -- true inactive --> C
J -- false active --> K[Set AvailableProviders in BifrostContext]
K --> L[return true]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Request with VK header] --> B{VK header present?}
B -- No --> C[return true, no filter]
B -- Yes --> D{ConfigStore available?}
D -- No --> E[return false, 503]
D -- Yes --> F{GetVirtualKeyByValue}
F -- ErrNotFound --> C
F -- other error --> G[return false, 500]
F -- found --> H{vk == nil?}
H -- Yes --> C
H -- No --> I{vk.IsActive == nil?}
I -- Yes --> C["❌ BUG: return true (nil = active, should filter)"]
I -- No --> J{"!*vk.IsActive?"}
J -- true inactive --> C
J -- false active --> K[Set AvailableProviders in BifrostContext]
K --> L[return true]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if vk == nil || vk.IsActive == nil || !*vk.IsActive { | ||
| return true | ||
| } |
There was a problem hiding this comment.
nil IsActive incorrectly bypasses provider filter
TableVirtualKey.IsActive is documented as "Nil means true (DB default); false means inactive" (see virtualkey.go:213 and the IsActiveValue() helper at line 247). When vk.IsActive == nil the VK is semantically active, but the new vk.IsActive == nil clause causes an early return true, silently skipping the ProviderConfigs filter. Any VK that was inserted with the DB default (nil IsActive) will expose all configured providers instead of only the ones in its ProviderConfigs.
The existing IsActiveValue() method already encodes the correct nil-as-true semantics. The condition should be if vk == nil || !vk.IsActiveValue() instead.
2 participants
Summary
Briefly explain the purpose of this PR and the problem it solves.
Changes
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelines