fix(models): filter and control model visibility in models - #654
Merged
steebchen merged 1 commit intoAug 28, 2025
Merged
Conversation
Co-authored-by: contact <contact@polarlights.llc>
|
Cursor Agent can help with this pull request. Just |
Contributor
WalkthroughImplements query-parameter-driven filtering on /v1/models: include_deactivated and exclude_deprecated. Updates the list handler to parse and apply filters against deactivatedAt and deprecatedAt timestamps, and updates tests to cover default behavior, each flag individually, and both together. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant Gateway as Gateway /v1/models Handler
participant Store as Models Source
Client->>Gateway: GET /v1/models?include_deactivated&exclude_deprecated
Note over Gateway: Parse query params as booleans<br/>includeDeactivated, excludeDeprecated<br/>currentDate = now
Gateway->>Store: Fetch modelsList
Store-->>Gateway: modelsList
rect rgba(200,230,255,0.25)
Note right of Gateway: Filtering
Gateway->>Gateway: Remove deactivated if now >= deactivatedAt<br/>unless includeDeactivated==true
Gateway->>Gateway: Remove deprecated if now >= deprecatedAt<br/>when excludeDeprecated==true
end
Gateway-->>Client: 200 OK { data: filtered models with ISO dates }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
steebchen
marked this pull request as ready for review
August 28, 2025 16:23
steebchen
changed the title
steebchen
added this pull request to the merge queue
Aug 28, 2025
steebchen
deleted the
cursor/filter-and-control-model-visibility-in-v1-models-91aa
branch
August 28, 2025 16:26
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/gateway/src/models/models.ts (1)
103-107: Use nullish coalescing for defaultsKeeps intent explicit and avoids truthiness pitfalls.
-const includeDeactivated = query.include_deactivated || false; -const excludeDeprecated = query.exclude_deprecated || false; +const includeDeactivated = query.include_deactivated ?? false; +const excludeDeprecated = query.exclude_deprecated ?? false;apps/gateway/src/models/models.spec.ts (2)
92-108: Validate combined flags via ordering relations between result sets.
Assert that:
- both ≥ exclude_deprecated (adding include_deactivated cannot reduce), and
- both ≤ include_deactivated (excluding deprecated cannot increase),
plus keep the content check for deprecated models.Proposed diff:
- const res = await app.request( - "/v1/models?include_deactivated=true&exclude_deprecated=true", - ); - expect(res.status).toBe(200); - - const json = await res.json(); - const currentDate = new Date(); - - // Should include deactivated models but exclude deprecated ones - for (const model of json.data) { - if (model.deprecated_at) { - const deprecatedAt = new Date(model.deprecated_at); - expect(currentDate <= deprecatedAt).toBe(true); - } - } + const [resInclude, resExclude, resBoth] = await Promise.all([ + app.request("/v1/models?include_deactivated=true"), + app.request("/v1/models?exclude_deprecated=true"), + app.request("/v1/models?include_deactivated=true&exclude_deprecated=true"), + ]); + expect(resBoth.status).toBe(200); + + const include = await resInclude.json(); + const exclude = await resExclude.json(); + const both = await resBoth.json(); + expect(Array.isArray(include.data)).toBe(true); + expect(Array.isArray(exclude.data)).toBe(true); + expect(Array.isArray(both.data)).toBe(true); + + // Monotonicity across flags + expect(both.data.length).toBeLessThanOrEqual(include.data.length); + expect(both.data.length).toBeGreaterThanOrEqual(exclude.data.length); + + // Deprecated models still excluded under combined flags + const now = Date.now(); + for (const model of both.data) { + if (model.deprecated_at) { + const deprecatedAtMs = Date.parse(model.deprecated_at); + expect(Number.isFinite(deprecatedAtMs)).toBe(true); + expect(now <= deprecatedAtMs).toBe(true); + } + }
46-61: Tighten assertion and guard against shape/time parsing issues- const json = await res.json(); - const currentDate = new Date(); + const json = await res.json(); + expect(Array.isArray(json.data)).toBe(true); + const now = Date.now(); // Verify that no deactivated models are returned for (const model of json.data) { if (model.deactivated_at) { - const deactivatedAt = new Date(model.deactivated_at); - expect(currentDate <= deactivatedAt).toBe(true); + const deactivatedAtMs = Date.parse(model.deactivated_at); + expect(Number.isFinite(deactivatedAtMs)).toBe(true); + expect(now <= deactivatedAtMs).toBe(true); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/gateway/src/models/models.spec.ts(1 hunks)apps/gateway/src/models/models.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use localStorage instead of cookies for client-side data persistence
Files:
apps/gateway/src/models/models.tsapps/gateway/src/models/models.spec.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use drizzle with the latest object syntax for database operations
For read queries, always usedb().query.<table>.findMany()ordb().query.<table>.findFirst()
Files:
apps/gateway/src/models/models.tsapps/gateway/src/models/models.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use
as anyor: anyin TypeScript files.
Files:
apps/gateway/src/models/models.tsapps/gateway/src/models/models.spec.ts
apps/{api,gateway}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
.findMany() or db().query.
apps/{api,gateway}/**/*.{ts,tsx}: Use Drizzle ORM with the latest object syntax in backend services
For reads, use db().query..findFirst() Files:
apps/gateway/src/models/models.tsapps/gateway/src/models/models.spec.ts**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Name unit tests with the .spec.ts suffix
Files:
apps/gateway/src/models/models.spec.ts{**/*.spec.ts,**/*.e2e.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Write tests using Vitest
Files:
apps/gateway/src/models/models.spec.ts🧬 Code graph analysis (2)
apps/gateway/src/models/models.ts (1)
packages/models/src/models.ts (1)
ModelDefinition(79-116)apps/gateway/src/models/models.spec.ts (1)
apps/gateway/src/index.ts (1)
app(43-43)🔇 Additional comments (2)
apps/gateway/src/models/models.ts (2)
131-131: LGTM: mapping over filteredModels is correctEnsures filtering is applied before projection.
108-129: Retain existing>comparison. It aligns with the API e2e test’snew Date() <= model.deactivatedAtlogic and Date objects already compare by their numeric timestamp; switching to>=would alter behavior.Likely an incorrect or invalid review comment.
Comment on lines
+62
to
+75
| test("GET /v1/models?include_deactivated=true should include deactivated models", async () => { | ||
| const res = await app.request("/v1/models?include_deactivated=true"); | ||
| expect(res.status).toBe(200); | ||
|
|
||
| const json = await res.json(); | ||
| expect(json).toHaveProperty("data"); | ||
| expect(Array.isArray(json.data)).toBe(true); | ||
|
|
||
| // The response should include all models (including deactivated ones) | ||
| // We can't easily test this without knowing specific deactivated models, | ||
| // but we can at least verify the endpoint works with the parameter | ||
| expect(json.data.length).toBeGreaterThan(0); | ||
| }); | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make the test meaningful: assert monotonicity (include_deactivated cannot reduce results).
Currently it only checks non-emptiness; assert that enabling the flag yields a superset or equal set size vs default.
Proposed diff:
- const res = await app.request("/v1/models?include_deactivated=true");
- expect(res.status).toBe(200);
-
- const json = await res.json();
- expect(json).toHaveProperty("data");
- expect(Array.isArray(json.data)).toBe(true);
-
- // The response should include all models (including deactivated ones)
- // We can't easily test this without knowing specific deactivated models,
- // but we can at least verify the endpoint works with the parameter
- expect(json.data.length).toBeGreaterThan(0);
+ const [resDefault, res] = await Promise.all([
+ app.request("/v1/models"),
+ app.request("/v1/models?include_deactivated=true"),
+ ]);
+ expect(res.status).toBe(200);
+
+ const base = await resDefault.json();
+ const json = await res.json();
+ expect(Array.isArray(base.data)).toBe(true);
+ expect(Array.isArray(json.data)).toBe(true);
+ // Enabling include_deactivated should never reduce the number of models
+ expect(json.data.length).toBeGreaterThanOrEqual(base.data.length);📝 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.
Suggested change
| test("GET /v1/models?include_deactivated=true should include deactivated models", async () => { | |
| const res = await app.request("/v1/models?include_deactivated=true"); | |
| expect(res.status).toBe(200); | |
| const json = await res.json(); | |
| expect(json).toHaveProperty("data"); | |
| expect(Array.isArray(json.data)).toBe(true); | |
| // The response should include all models (including deactivated ones) | |
| // We can't easily test this without knowing specific deactivated models, | |
| // but we can at least verify the endpoint works with the parameter | |
| expect(json.data.length).toBeGreaterThan(0); | |
| }); | |
| test("GET /v1/models?include_deactivated=true should include deactivated models", async () => { | |
| const [resDefault, res] = await Promise.all([ | |
| app.request("/v1/models"), | |
| app.request("/v1/models?include_deactivated=true"), | |
| ]); | |
| expect(res.status).toBe(200); | |
| const base = await resDefault.json(); | |
| const json = await res.json(); | |
| expect(Array.isArray(base.data)).toBe(true); | |
| expect(Array.isArray(json.data)).toBe(true); | |
| // Enabling include_deactivated should never reduce the number of models | |
| expect(json.data.length).toBeGreaterThanOrEqual(base.data.length); | |
| }); |
🤖 Prompt for AI Agents
In apps/gateway/src/models/models.spec.ts around lines 62 to 75, the test only
checks non-emptiness; update it to request the endpoint twice (once without
params and once with ?include_deactivated=true) and assert that the response
with include_deactivated has a length greater than or equal to the default
response (and optionally that the set of IDs in the include_deactivated response
is a superset of the default IDs) to ensure enabling the flag cannot reduce
results.
Comment on lines
+76
to
+91
| test("GET /v1/models?exclude_deprecated=true should exclude deprecated models", async () => { | ||
| const res = await app.request("/v1/models?exclude_deprecated=true"); | ||
| expect(res.status).toBe(200); | ||
|
|
||
| const json = await res.json(); | ||
| const currentDate = new Date(); | ||
|
|
||
| // Verify that no deprecated models are returned | ||
| for (const model of json.data) { | ||
| if (model.deprecated_at) { | ||
| const deprecatedAt = new Date(model.deprecated_at); | ||
| expect(currentDate <= deprecatedAt).toBe(true); | ||
| } | ||
| } | ||
| }); | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen exclusion test: assert both content constraint and monotonicity (exclude_deprecated cannot increase results).
This makes the test robust regardless of fixture data.
Proposed diff:
- const res = await app.request("/v1/models?exclude_deprecated=true");
- expect(res.status).toBe(200);
-
- const json = await res.json();
- const currentDate = new Date();
-
- // Verify that no deprecated models are returned
- for (const model of json.data) {
- if (model.deprecated_at) {
- const deprecatedAt = new Date(model.deprecated_at);
- expect(currentDate <= deprecatedAt).toBe(true);
- }
- }
+ const [resDefault, res] = await Promise.all([
+ app.request("/v1/models"),
+ app.request("/v1/models?exclude_deprecated=true"),
+ ]);
+ expect(res.status).toBe(200);
+
+ const base = await resDefault.json();
+ const json = await res.json();
+ expect(Array.isArray(base.data)).toBe(true);
+ expect(Array.isArray(json.data)).toBe(true);
+ // Disallow deprecated models and ensure count is not larger than baseline
+ const now = Date.now();
+ expect(json.data.length).toBeLessThanOrEqual(base.data.length);
+ for (const model of json.data) {
+ if (model.deprecated_at) {
+ const deprecatedAtMs = Date.parse(model.deprecated_at);
+ expect(Number.isFinite(deprecatedAtMs)).toBe(true);
+ expect(now <= deprecatedAtMs).toBe(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.
Suggested change
| test("GET /v1/models?exclude_deprecated=true should exclude deprecated models", async () => { | |
| const res = await app.request("/v1/models?exclude_deprecated=true"); | |
| expect(res.status).toBe(200); | |
| const json = await res.json(); | |
| const currentDate = new Date(); | |
| // Verify that no deprecated models are returned | |
| for (const model of json.data) { | |
| if (model.deprecated_at) { | |
| const deprecatedAt = new Date(model.deprecated_at); | |
| expect(currentDate <= deprecatedAt).toBe(true); | |
| } | |
| } | |
| }); | |
| test("GET /v1/models?exclude_deprecated=true should exclude deprecated models", async () => { | |
| // Fetch both the default list and the filtered list in parallel | |
| const [resDefault, res] = await Promise.all([ | |
| app.request("/v1/models"), | |
| app.request("/v1/models?exclude_deprecated=true"), | |
| ]); | |
| expect(res.status).toBe(200); | |
| const base = await resDefault.json(); | |
| const json = await res.json(); | |
| expect(Array.isArray(base.data)).toBe(true); | |
| expect(Array.isArray(json.data)).toBe(true); | |
| // Ensure filtering never increases the total number of models | |
| expect(json.data.length).toBeLessThanOrEqual(base.data.length); | |
| // Disallow deprecated models and verify any deprecated_at is in the future | |
| const now = Date.now(); | |
| for (const model of json.data) { | |
| if (model.deprecated_at) { | |
| const deprecatedAtMs = Date.parse(model.deprecated_at); | |
| expect(Number.isFinite(deprecatedAtMs)).toBe(true); | |
| expect(now <= deprecatedAtMs).toBe(true); | |
| } | |
| } | |
| }); |
🤖 Prompt for AI Agents
In apps/gateway/src/models/models.spec.ts around lines 76 to 91, the test only
verifies that returned models are not already deprecated but doesn't assert
monotonicity (that excluding deprecated models never increases the result set);
update the test to first fetch the baseline list with no query params, store its
total count, then fetch with ?exclude_deprecated=true and assert (1) every
returned model has no deprecated_at or deprecated_at is in the future, and (2)
the count with exclude_deprecated is less than or equal to the baseline count to
enforce monotonicity; keep error messages or expect messages clear so failures
indicate whether the content constraint or the monotonicity check failed.
Comment on lines
+75
to
+88
| request: { | ||
| query: z.object({ | ||
| include_deactivated: z | ||
| .string() | ||
| .optional() | ||
| .transform((val) => val === "true") | ||
| .describe("Include deactivated models in the response"), | ||
| exclude_deprecated: z | ||
| .string() | ||
| .optional() | ||
| .transform((val) => val === "true") | ||
| .describe("Exclude deprecated models from the response"), | ||
| }), | ||
| }, |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parse booleans strictly and expose them as booleans in the OpenAPI spec
Current transform treats any non-"true" value (including typos) as false and will likely render as "string" in the schema. Tighten validation to accepted literals only.
request: {
query: z.object({
- include_deactivated: z
- .string()
- .optional()
- .transform((val) => val === "true")
- .describe("Include deactivated models in the response"),
- exclude_deprecated: z
- .string()
- .optional()
- .transform((val) => val === "true")
- .describe("Exclude deprecated models from the response"),
+ include_deactivated: z
+ .enum(["true", "false"])
+ .optional()
+ .transform((val) => val === "true")
+ .describe("Include deactivated models in the response"),
+ exclude_deprecated: z
+ .enum(["true", "false"])
+ .optional()
+ .transform((val) => val === "true")
+ .describe("Exclude deprecated models from the response"),
}),
},📝 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.
Suggested change
| request: { | |
| query: z.object({ | |
| include_deactivated: z | |
| .string() | |
| .optional() | |
| .transform((val) => val === "true") | |
| .describe("Include deactivated models in the response"), | |
| exclude_deprecated: z | |
| .string() | |
| .optional() | |
| .transform((val) => val === "true") | |
| .describe("Exclude deprecated models from the response"), | |
| }), | |
| }, | |
| request: { | |
| query: z.object({ | |
| - include_deactivated: z | |
| - .string() | |
| - .optional() | |
| - .transform((val) => val === "true") | |
| - .describe("Include deactivated models in the response"), | |
| - exclude_deprecated: z | |
| - .string() | |
| - .optional() | |
| - .transform((val) => val === "true") | |
| include_deactivated: z | |
| .enum(["true", "false"]) | |
| .optional() | |
| .transform((val) => val === "true") | |
| .describe("Include deactivated models in the response"), | |
| exclude_deprecated: z | |
| .enum(["true", "false"]) | |
| .optional() | |
| .transform((val) => val === "true") | |
| .describe("Exclude deprecated models from the response"), | |
| }), | |
| }, |
This was referenced Oct 24, 2025
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add optional filtering for deactivated and deprecated models to the
/v1/modelsendpoint to provide more control over the returned model list.Summary by CodeRabbit
New Features
Tests