Skip to content

test(models): assert the hidden-modality premise on stored rows (#15306) - #16150

Closed
shipsfromrio wants to merge 3 commits into
cheaper-inference:release/v3.8.52from
shipsfromrio:fix/15306-br-hidden
Closed

shipsfromrio wants to merge 3 commits into
cheaper-inference:release/v3.8.52from
shipsfromrio:fix/15306-br-hidden

Conversation

@shipsfromrio

Copy link
Copy Markdown
Contributor

Problem

tests/unit/model-hidden-dashboard-modality-read.test.ts fails deterministically on release/v3.8.52 (one of the base-reds in #15306):

premise: the dashboard's chat-scoped PATCH stores hiddenModalities.chat, not isHidden
  true !== false — the scoped write must not set the legacy all-modalities flag

#15900 made GET /api/provider-models decorate every override row with a derived isHidden: isHiddenForModality(override, "chat") for legacy dashboard clients (src/app/api/provider-models/route.ts). The premise case read the row through that GET and asserted the key was absent, so it now always fails. Production is correct: the chat-scoped PATCH still stores only hiddenModalities.chat.

Fix (test only)

Tests

The file passes 8/8 (3 consecutive runs). Related suites pass: auto-combo-hidden-models-4558, db-models-split, model-hidden-modality-scope-12172, models-catalog-route, noauth-autocombo-hidden-7620, noauth-sibling-compat-override-7620, provider-models-management-route (73/73), and the vitest model-hidden-dashboard-visibility (4/4).

Refs #15306

⚠️ base-red inherited: #15306

…per-inference#15306)

The premise case in model-hidden-dashboard-modality-read read the override
through GET /api/provider-models and asserted the row carries no legacy
isHidden key. cheaper-inference#15900 (17b8c2b) made that GET decorate every override with
a derived isHidden = isHiddenForModality(override, "chat") for legacy
dashboard clients, so the key is always present in the projection and the
case failed deterministically.

The premise is about storage, so read the persisted row through
getModelCompatOverrides() and keep the no-legacy-flag guard there; also
assert the GET projection reports isHidden: true, pinning cheaper-inference#15900's contract.
@diegosouzapw

Copy link
Copy Markdown
Collaborator

Hi @shipsfromrio — closing this pull request to clear the open queue. The branch fix/15306-br-hidden is kept, so the commits stay. Comment here if it should be reopened.

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Hi @shipsfromrio — reopening this pull request. The close was a mistake, and it is back in the merge queue. The branch fix/15306-br-hidden was kept.

Merge the current release while preserving both the persisted-row premise and dashboard projection checks from PR cheaper-inference#16150.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
@diegosouzapw

Copy link
Copy Markdown
Collaborator

Thank you @shipsfromrio. I reconciled the conflict with release 94159e1b7536783aec456d4f1a1c02cf2b74403c directly on this PR's branch in 57ac0eaefeb813d54a4e1cbf370ced408dbd0545, preserving your authorship and adding the maintainer co-author trailer.

The overlapping #16420 repair had already separated the stored legacy flag from the API projection. This reconciliation retains your explicit stored-row existence and hiddenModalities.chat checks, and separately verifies the dashboard projection. No assertions or production behavior were removed.

  • Actual Node collector: 12/12 tests PASS across the dashboard premise and modality-scope regression suites; zero skipped/cancelled.
  • Scoped ESLint, Prettier and normal commit hooks PASS.
  • Post-hook diff against the pinned release: the intended test and original contributor-credit changelog only; clean worktree.
  • Push was a normal fast-forward to the original fork branch, not a replacement PR.

This resolves the local conflict, not full merge admission. Required current-candidate CI is still needed; no queue label, merge, administrative bypass or deploy was performed.

Preserve both stored modality flags and projected visibility assertions. Current focused suite on release ad37e8f: 8/8 pass, zero skips or cancellations.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
@diegosouzapw

Copy link
Copy Markdown
Collaborator

Relay continuation — scoped reconciliation, not merge acceptance

Thanks @shipsfromrio. Reconciled this original PR with pinned release ad37e8f8f51eb2d505f88a3139e76912cf9598e2 and published b6f441fe716af3f39dd59b4d27acb8215df2eab3 through a normal fast-forward push. Original commits and contributor attribution are preserved.

8/8 focused visibility tests PASS; both persisted modality flags and dashboard projections remain explicitly asserted.

Node 24.20.0 / npm 11.15.0; unchanged lockfile SHA-256 f2de65a4140bbe8f4f1ae78ea26e1ba06baa67850c297306d94e7b0793f7e3b4. Normal commit hooks ran. These are focused diagnostics, not the full current-candidate acceptance matrix. The release has advanced since the pinned comparison, and full mandatory checks must be refreshed before admission. No queue label, bypass, merge, closure or deployment was performed by this continuation.

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Closing: this only restates the hidden-modality premise already tracked on base-red #15306 and does not change production behavior. Thanks @shipsfromrio.

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Owner decision readback — 2026-10-16

After reviewing the conflicting closure, the owner explicitly selected option B: reopen #15945 and #16150 and retain them in the previously approved Relay integration group. Reopening this original PR follows that newer decision; it is not a merge or a waiver of CI.

The head remains b6f441fe716af3f39dd59b4d27acb8215df2eab3. The owner has chosen to retain the persisted-modality/dashboard regression checks even though this residual diff does not change production behavior.

Thanks @shipsfromrio. Original commits and attribution will be preserved. This PR is being coordinated by the Relay continuation; please do not re-close it based on the superseded closure decision. We will revalidate against the current release and will not enqueue or merge until all applicable current-candidate gates pass. No administrative bypass or direct release push is authorized.

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Closing as already on release/v3.8.52 via #16420. The same premise fix is in tests/unit/model-hidden-dashboard-modality-read.test.ts, and this branch conflicts on that file. Thanks @shipsfromrio — the diagnosis was right, and the tip already has it. This PR had been reopened after an earlier queue cleanup; closing it again now that the parent is on the release branch.

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Thanks @shipsfromrio. The hidden-modality premise test was already fixed on the tip by #16420 (it reads the stored override and asserts isHidden is absent from it); what remains here is a rename plus one extra assertion. Closing as covered. (/merge-batch 2026-10-17)

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