Skip to content

Add model_deny_list and provider_deny_list to organization settings, …#725

Merged
chrarnoldus merged 3 commits intomainfrom
christiaan/deny-lists
Mar 2, 2026
Merged

Add model_deny_list and provider_deny_list to organization settings, …#725
chrarnoldus merged 3 commits intomainfrom
christiaan/deny-lists

Conversation

@chrarnoldus
Copy link
Copy Markdown
Contributor

…set on update

Comment thread src/lib/model-allow.server.ts
Comment thread src/lib/model-allow.server.ts Outdated
Comment thread src/lib/model-allow.server.ts
Comment thread src/routers/organizations/organization-settings-router.ts
@kilo-code-bot
Copy link
Copy Markdown
Contributor

kilo-code-bot Bot commented Mar 2, 2026

Code Review Summary

Status: 4 Issues Found | Recommendation: Address before merge

Fix these issues in Kilo Cloud

Overview

Severity Count
CRITICAL 0
WARNING 3
SUGGESTION 1
Issue Details (click to expand)

WARNING

File Line Issue
src/lib/model-allow.server.ts 58 Unnecessary DB fetch when both allow lists are defined but empty — the early return only checks for undefined, not empty arrays
src/lib/model-allow.server.ts N/A Models belonging to denied providers are not added to model_deny_list — when only provider_allow_list is set, isAllowed is undefined so the inner loop never adds models to the deny list
src/routers/organizations/organization-settings-router.ts 230 When createDenyLists returns undefined (DB fetch failure), stale deny lists from currentSettings are preserved via the spread on line 216 rather than being cleared

SUGGESTION

File Line Issue
src/lib/model-allow.server.ts 72 Sequential await inside a nested loop — each model slug is checked one at a time; could batch or use Promise.all for better performance with large provider lists
Other Observations (not in diff)

Issues and considerations found outside the diff that cannot receive inline comments:

File Line Issue
N/A N/A No tests: createDenyLists is a new function with non-trivial logic (DB fetch, nested iteration, set operations) but no test file was added. Consider adding unit tests covering: both lists undefined, empty arrays, provider-only list, model-only list, and DB fetch failure.
N/A N/A Deny lists become stale over time: Deny lists are only recomputed when updateAllowLists is called. If new models/providers are added to OpenRouter, the deny lists won't reflect them until the next allow list update. This may be acceptable given the "under development" status noted in the schema comment.
Files Reviewed (4 files)
  • packages/db/src/schema-types.ts - 0 issues (schema addition is clean)
  • src/lib/model-allow.server.ts - 3 issues
  • src/lib/providers/openrouter/models-by-provider-index.server.ts - 0 issues (visibility change only)
  • src/routers/organizations/organization-settings-router.ts - 1 issue

@chrarnoldus chrarnoldus merged commit 1100791 into main Mar 2, 2026
12 checks passed
@chrarnoldus chrarnoldus deleted the christiaan/deny-lists branch March 2, 2026 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants