fix(models): tag privacy-filter as a non-completion (classification) model instead of hiding it - #872
fix(models): tag privacy-filter as a non-completion (classification) model instead of hiding it#872Evrard-Nil wants to merge 1 commit into
Conversation
GitHub Actions
Failure
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to filter out non-generative models (such as token-classification models) from the public model catalog endpoints. It adds a helper function is_listed_in_model_catalog to determine visibility based on output modalities, integrates this filter into the models list and completions endpoints, adds a database migration to update the openai/privacy-filter model's modalities, and includes corresponding tests. Feedback focuses on improving the Rust API design by passing Option<&[String]> instead of &Option<Vec<String>> to avoid unnecessary allocations and references, and refactoring the SQL migration to avoid an unused data-modifying CTE.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| WITH target_model AS ( | ||
| SELECT id | ||
| FROM models | ||
| WHERE model_name = 'openai/privacy-filter' | ||
| ), | ||
| updated_model AS ( | ||
| UPDATE models | ||
| SET | ||
| output_modalities = ARRAY['classification']::TEXT[], | ||
| updated_at = NOW() | ||
| WHERE model_name = 'openai/privacy-filter' | ||
| AND output_modalities IS DISTINCT FROM ARRAY['classification']::TEXT[] | ||
| RETURNING id | ||
| ) | ||
| -- Correct the open snapshot in place (a metadata fix, not a new state | ||
| -- transition), independent of the `models` UPDATE so a prior manual patch to | ||
| -- `models` still repairs the open history snapshot. | ||
| UPDATE model_history mh | ||
| SET output_modalities = ARRAY['classification']::TEXT[] | ||
| FROM target_model | ||
| WHERE mh.model_id = target_model.id | ||
| AND mh.effective_until IS NULL | ||
| AND mh.output_modalities IS DISTINCT FROM ARRAY['classification']::TEXT[]; |
There was a problem hiding this comment.
Defining updated_model as a data-modifying CTE but never referencing it in the main query is a code smell. While PostgreSQL executes data-modifying CTEs to completion even if they are not referenced, this behavior is non-standard and highly confusing to other developers. It is much cleaner and more idiomatic to split this into two separate, sequential SQL statements.
UPDATE models
SET
output_modalities = ARRAY['classification']::TEXT[],
updated_at = NOW()
WHERE model_name = 'openai/privacy-filter'
AND output_modalities IS DISTINCT FROM ARRAY['classification']::TEXT[];
UPDATE model_history mh
SET output_modalities = ARRAY['classification']::TEXT[]
FROM models m
WHERE mh.model_id = m.id
AND m.model_name = 'openai/privacy-filter'
AND mh.effective_until IS NULL
AND mh.output_modalities IS DISTINCT FROM ARRAY['classification']::TEXT[];| pub fn is_listed_in_model_catalog(output_modalities: &Option<Vec<String>>) -> bool { | ||
| match output_modalities { | ||
| Some(modalities) if !modalities.is_empty() => !modalities | ||
| .iter() | ||
| .all(|m| NON_GENERATIVE_OUTPUT_MODALITIES.contains(&m.as_str())), | ||
| _ => true, | ||
| } | ||
| } |
There was a problem hiding this comment.
Passing &Option<Vec<T>> as a function argument is a Rust anti-pattern. It unnecessarily restricts the caller to have an owned Option<Vec<T>> and forces a reference to it. Instead, use Option<&[T]> (or Option<&[String]> here) and let the caller use .as_deref() to convert their Option<Vec<T>> to Option<&[T]>.
| pub fn is_listed_in_model_catalog(output_modalities: &Option<Vec<String>>) -> bool { | |
| match output_modalities { | |
| Some(modalities) if !modalities.is_empty() => !modalities | |
| .iter() | |
| .all(|m| NON_GENERATIVE_OUTPUT_MODALITIES.contains(&m.as_str())), | |
| _ => true, | |
| } | |
| } | |
| pub fn is_listed_in_model_catalog(output_modalities: Option<&[String]>) -> bool { | |
| match output_modalities { | |
| Some(modalities) if !modalities.is_empty() => !modalities | |
| .iter() | |
| .all(|m| NON_GENERATIVE_OUTPUT_MODALITIES.contains(&m.as_str())), | |
| _ => true, | |
| } | |
| } |
| let listed = |m: &[&str]| { | ||
| is_listed_in_model_catalog(&Some(m.iter().map(|s| s.to_string()).collect())) | ||
| }; |
There was a problem hiding this comment.
Update the test helper to match the new signature of is_listed_in_model_catalog taking Option<&[String]>.
| let listed = |m: &[&str]| { | |
| is_listed_in_model_catalog(&Some(m.iter().map(|s| s.to_string()).collect())) | |
| }; | |
| let listed = |m: &[&str]| { | |
| let modalities: Vec<String> = m.iter().map(|s| s.to_string()).collect(); | |
| is_listed_in_model_catalog(Some(&modalities)) | |
| }; |
| assert!(is_listed_in_model_catalog(&None)); | ||
| assert!(is_listed_in_model_catalog(&Some(vec![]))); |
There was a problem hiding this comment.
Update the assertions to pass None and Some(&[]) directly instead of referencing them.
| assert!(is_listed_in_model_catalog(&None)); | |
| assert!(is_listed_in_model_catalog(&Some(vec![]))); | |
| assert!(is_listed_in_model_catalog(None)); | |
| assert!(is_listed_in_model_catalog(Some(&[]))); |
| // (issue #615). Chat/image/embedding models are unaffected. | ||
| data: models | ||
| .into_iter() | ||
| .filter(|m| crate::routes::common::is_listed_in_model_catalog(&m.output_modalities)) |
There was a problem hiding this comment.
Use .as_deref() to pass Option<&[String]> to is_listed_in_model_catalog.
| .filter(|m| crate::routes::common::is_listed_in_model_catalog(&m.output_modalities)) | |
| .filter(|m| crate::routes::common::is_listed_in_model_catalog(m.output_modalities.as_deref())) |
| // count stays consistent across pages. | ||
| let all_models: Vec<_> = all_models | ||
| .into_iter() | ||
| .filter(|m| crate::routes::common::is_listed_in_model_catalog(&m.output_modalities)) |
There was a problem hiding this comment.
Use .as_deref() to pass Option<&[String]> to is_listed_in_model_catalog.
| .filter(|m| crate::routes::common::is_listed_in_model_catalog(&m.output_modalities)) | |
| .filter(|m| crate::routes::common::is_listed_in_model_catalog(m.output_modalities.as_deref())) |
Review — PR #872 (exclude non-generative models from
|
There was a problem hiding this comment.
Pull request overview
This PR prevents non-generative models (specifically openai/privacy-filter, a token-classification/PII model) from being advertised as OpenAI-compatible generative models in the public model catalogs, by filtering listings based on output_modalities and repairing the catalog metadata via a migration.
Changes:
- Added
routes::common::is_listed_in_model_catalog+NON_GENERATIVE_OUTPUT_MODALITIESto hide models whose declared output modalities are entirely non-generative. - Applied the filter to both listing endpoints:
GET /v1/modelsandGET /v1/model/list(the latter before pagination). - Added migration
V0068to relabelopenai/privacy-filteroutput_modalitiesto["classification"], plus an e2e test asserting exclusion from/v1/models.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/database/src/migrations/sql/V0068__mark_privacy_filter_non_generative.sql | Repairs the seeded openai/privacy-filter output_modalities (and open history snapshot) to enable filtering it out of public catalogs. |
| crates/api/tests/e2e_all/general.rs | Adds e2e coverage ensuring classification-only models don’t appear in GET /v1/models. |
| crates/api/src/routes/models.rs | Filters non-generative models out of GET /v1/model/list prior to pagination for consistent total and pages. |
| crates/api/src/routes/completions.rs | Filters non-generative models out of GET /v1/models. |
| crates/api/src/routes/common.rs | Introduces the shared catalog-filter predicate and its unit test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| WHERE model_name = 'openai/privacy-filter' | ||
| AND output_modalities IS DISTINCT FROM ARRAY['classification']::TEXT[] | ||
| RETURNING id |
| WHERE mh.model_id = target_model.id | ||
| AND mh.effective_until IS NULL | ||
| AND mh.output_modalities IS DISTINCT FROM ARRAY['classification']::TEXT[]; |
ec1e98f to
cb464d7
Compare
GitHub Actions
Failure
PierreLeGuen
left a comment
PierreLeGuen
left a comment
There was a problem hiding this comment.
The change itself is well-scoped and correct — a denylist filter on output_modalities applied to both user-facing listing endpoints (GET /v1/models in completions.rs, list_models in models.rs), safe by construction for models with no declared modalities or mixed modalities, with admin listing unaffected. The blocker is the migration, which fails to apply against the actual column types.
- Blocking —
crates/database/src/migrations/sql/V0068__mark_privacy_filter_non_generative.sql:29: the migration writes/comparesARRAY['classification']::TEXT[]againstoutput_modalities, but V0043 defines bothmodels.output_modalitiesandmodel_history.output_modalitiesasJSONB. CI E2E fails on V68 withERROR: operator does not exist: jsonb = text[](run 28957601638), so merging breaks migrations at deploy startup. Use JSONB throughout on both themodelsandmodel_historyupdates, e.g. setoutput_modalities = '["classification"]'::jsonband guard withIS DISTINCT FROM '["classification"]'::jsonb. - Non-blocking — correct the catalog seed source for
openai/privacy-filtertooutputModalities: ["classification"]as well; a future re-upsert that explicitly re-sends["text"]would revert this migration's effect.
Checks run: cargo check -p api passed and LSP diagnostics were clean on all changed files; CI inspected via gh run view, where lint/unit/integration passed and E2E failed on the migration above. Full test suite / migration apply not run locally (requires PostgreSQL).
cb464d7 to
8911538
Compare
GitHub Actions
Inactive
|
Thanks @PierreLeGuen — I believe the blocking item was fixed in the current commit (
Re-requesting review — PTAL when you get a chance. |
PierreLeGuen
left a comment
PierreLeGuen
left a comment
There was a problem hiding this comment.
The change correctly excludes non-generative models (e.g. privacy-filter) from both public listing endpoints, and the earlier blocking migration issue is resolved. Approving.
- The filter in
is_listed_in_model_cataloghides a model only when every declared modality is non-generative;None/empty modalities default to listed (backfill-safe), and mixed lists like["classification","text"]stay listed. Both/v1/models(completions.rs) and/v1/model/list(models.rs, filtered before pagination sototalstays consistent) are covered, and the privacy/auto-redact resolution paths are unaffected. - Migration
V0068__mark_privacy_filter_non_generative.sql: now uses JSONB throughout ('["classification"]'::jsonb) matching the JSONBoutput_modalitiescolumns from V0043, is idempotent viaIS DISTINCT FROM, and is consecutively numbered after V0067 — the priorTEXT[]-vs-JSONB blocker is fixed.
Optional follow-up (non-blocking): the relabel in V0068 is durable only while the model-discovery/seed source doesn't re-upsert openai/privacy-filter with an explicit outputModalities: ["text"]. An omit-the-field re-upsert is safe (COALESCE(EXCLUDED...)), but a re-send of ["text"] would silently revert it. Already tracked in-thread; worth fixing at the discovery source to emit ["classification"].
Checks: cargo check --workspace --tests and cargo test -p api --lib test_is_listed_in_model_catalog passed; LSP diagnostics on common.rs/completions.rs/models.rs clean; CI (lint, unit, integration, e2e, security audit) green on 8911538d.
…model `openai/privacy-filter` is a token-classification (PII detection) model, not a generative chat model, but GET /v1/models and GET /v1/model/list advertised it as an ordinary chat model. Rather than hiding it, tag it with its true modality so clients can tell it apart, the same way image and embedding models are represented. Migration V0069 relabels the model row's output_modalities from the mislabeled ["text"] to ["classification"] (and mirrors the fix into the open model_history snapshot). Both listing endpoints already surface output_modalities / architecture.outputModalities to clients, so this tag alone achieves parity with how image models (["image"]) are listed. No new field is introduced and no catalog filtering is added. Replaces test_non_generative_models_excluded_from_models_list with test_classification_model_listed_and_tagged (asserts the model is listed AND carries the classification tag). test_models_api now locates the configured chat model by id instead of the first catalog entry, so it is robust to the shared test catalog containing a zero-priced classification model. Fixes #615.
8911538 to
64e5726
Compare
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Evrard-Nil
changed the title
GitHub Actions
Inactive
|
Revised per maintainer feedback: "Model should be there but tagged as not a completion model. Same as image models. Hiding it is not good." This supersedes the earlier hide-based version. What changed vs the hide approach
How it mirrors image models Migration renumbered + branch rebased Tests
Validation (rebased state = Note: the branch name ( |
PierreLeGuen
left a comment
PierreLeGuen
left a comment
There was a problem hiding this comment.
This PR relabels openai/privacy-filter as a non-completion (classification) model so it is listed and tagged by modality rather than hidden. The revised tag-based approach resolves the earlier blocking jsonb = text[] migration issue; the change (migration V0069 + a clarifying comment + e2e tests) is sound and safe to merge.
- Migration
V0069__mark_privacy_filter_non_generative.sqlcorrectly uses'["classification"]'::jsonbagainst the JSONBoutput_modalitiescolumns (defined in V0043) on bothmodelsand the openmodel_historysnapshot, withIS DISTINCT FROMidempotency guards; it is a valid no-op on DBs lacking the row. - Relabeling is safe:
output_modalitiesis used only in DB read/write plumbing and catalog-response mapping — the models listing (WHERE is_active = true) and chat completions routing (resolve_and_get_model) do not gate on it, so/v1/privacy/*, completions, and model lookup are unaffected.
Non-blocking follow-up:
V0069: the relabel is durable only until model-discovery/seed re-upsertsopenai/privacy-filter. An omit-the-field re-upsert is safe viaCOALESCE, but re-sending["text"]would silently revert it. Not introduced by this PR and already tracked as a follow-up at the discovery source — noted for visibility only.
Local checks: cargo fmt --check, cargo check/clippy -D warnings on the affected crates, and the general::test_models_api + general::test_classification_model_listed_and_tagged e2e tests all passed (isolated test DB, DEV=1).
Problem
GET /v1/models(andGET /v1/model/list) advertisedopenai/privacy-filteras if it were an ordinary chat model. It is a token-classification (PII detection) model: its only real endpoints are/v1/privacy/classifyand/v1/privacy/redact, and a/v1/chat/completionsrequest against it hits a non-existent upstream path and 404s. Because the catalog exposed no notion of its kind, a client had no way to tell it apart from a completion model.Fixes #615.
Approach (revised per maintainer feedback)
This PR supersedes its earlier hide-based version. Instead of filtering the model out of the catalog, it keeps the model listed and tags it with its true modality — exactly the way image and embedding models are represented ("list + tag, same as image models").
Why tagging alone is sufficient
The catalog already encodes model kind in
output_modalities, and both listing endpoints already surface that field to clients:GET /v1/models→ eachModelInfocarries both the OpenRouter-flatoutput_modalitiesand the nestedarchitecture.outputModalities.GET /v1/model/list→ each model'sarchitecture.outputModalities(built byModelArchitecture::from_options).An image model is listed with
outputModalities = ["image"]; an embedding model with["embedding"]. The privacy filter was simply mislabeled["text"], so it masqueraded as a chat model. Relabeling its output modality to["classification"]makes it self-describe as a classifier — a client distinguishes it by readingoutput_modalities, exactly as it would for an image model. No new field or scheme is introduced; this reuses the existing modality mechanism.Changes
V0069relabels theopenai/privacy-filterrow'soutput_modalitiesfrom the mislabeled["text"]to["classification"](and mirrors the fix into the currently-openmodel_historysnapshot). This is the whole fix. It is idempotent, a no-op on databases without the row (fresh/test DBs), and mirrors the in-place catalog-metadata repair pattern used byV0061. Input modality stays["text"]— it still consumes text. The migration comment block states its purpose is to correctly tag the model's modality, not to feed an api-layer hide filter.V0068→V0069: since this branch first opened,mainlanded its ownV0068(V0068__cache_read_cost_nullable.sql). Two migrations sharing version0068would make refinery reject startup, so this one moves to the next free version. The branch has also been rebased onto currentmainso it carries main'sV0068and thisV0069side by side..filter(...)inroutes::completions::models(/v1/models) androutes::models::list_models(/v1/model/list), plus the now-unusedis_listed_in_model_cataloghelper, theNON_GENERATIVE_OUTPUT_MODALITIESconst, and its unit test inroutes::common. (Net effect:common.rsandmodels.rsare unchanged frommain;completions.rsgains only an explanatory comment.)The privacy endpoints and direct
GET /v1/model/{name}lookups were never affected either way.Tests
test_non_generative_models_excluded_from_models_list(asserted exclusion) withtest_classification_model_listed_and_tagged: assertsopenai/privacy-filteris listed in/v1/modelsand reportsoutput_modalities/architecture.outputModalities=["classification"], alongside a chat model still reporting["text"].test_models_apito locate the configured chat model by id rather thandata.first(). With the classifier no longer hidden, the shared e2e catalog can now contain a zero-priced classification model that may sort first under some collations; keying off the first entry was order-dependent. Every other listing test already scopes by id.Validation (rebased state:
main+ this change)cargo fmt --all -- --check— cleancargo check --workspace --tests— cleancargo clippy --workspace --tests— cleanV0068and thisV0069) applies cleanly on a fresh bootstrap.test_classification_model_listed_and_taggedandtest_models_apiboth pass, including against a catalog already containing the zero-priced classifier.