Skip to content

fix(models): filter and control model visibility in models #654

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
steebchen merged 1 commit into from
Aug 28, 2025
Merged

fix(models): filter and control model visibility in models #654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 64 additions & 0 deletions apps/gateway/src/models/models.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,68 @@ describe("Models API", () => {

expect(firstModel).toHaveProperty("family");
});

test("GET /v1/models should exclude deactivated models by default", async () => {
const res = await app.request("/v1/models");
expect(res.status).toBe(200);

const json = await res.json();
const currentDate = new Date();

// 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);
}
}
});

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);
});

Comment on lines +62 to +75

@coderabbitai coderabbitai Bot Aug 28, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

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);
}
}
});

Comment on lines +76 to +91

@coderabbitai coderabbitai Bot Aug 28, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

test("GET /v1/models should handle both parameters together", async () => {
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);
}
}
});
});
45 changes: 43 additions & 2 deletions apps/gateway/src/models/models.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,20 @@ const listModels = createRoute({
description: "List all available models",
method: "get",
path: "/",
request: {},
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"),
}),
},
Comment on lines +75 to +88

@coderabbitai coderabbitai Bot Aug 28, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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"),
}),
},

responses: {
200: {
content: {
Expand All @@ -87,7 +100,35 @@ const listModels = createRoute({

modelsApi.openapi(listModels, async (c) => {
try {
const modelData = modelsList.map((model: ModelDefinition) => {
const query = c.req.valid("query");
const includeDeactivated = query.include_deactivated || false;
const excludeDeprecated = query.exclude_deprecated || false;
const currentDate = new Date();

// Filter models based on deactivation and deprecation status
const filteredModels = modelsList.filter((model: ModelDefinition) => {
// Filter out deactivated models by default (unless explicitly included)
if (
!includeDeactivated &&
model.deactivatedAt &&
currentDate > model.deactivatedAt
) {
return false;
}

// Filter out deprecated models if requested
if (
excludeDeprecated &&
model.deprecatedAt &&
currentDate > model.deprecatedAt
) {
return false;
}

return true;
});

const modelData = filteredModels.map((model: ModelDefinition) => {
// Determine input modalities (if model supports images)
const inputModalities: ("text" | "image")[] = ["text"];

Expand Down
Loading