fix: close the gaps a red-team pass found in spend exclusions

Four of these are real defects, two of them regressions from the two
commits above.

Quota sweep no longer cancels excluded trials. `_active_trial_predicates`
lost the key-exclusion filter when that feature was deleted and gained
nothing in its place, so `cancel_trials_if_quota_reached` would kill every
active trial in an excluded experiment — trials that contributed nothing
to the number that tripped the cap and reserve nothing against it, so
killing them frees no headroom. Adds both filters and a test, since the
test that used to cover this was rewritten around the deleted feature.

Model exclusions no longer silently no-op. `trials.model` is written by
the agent-aware `normalize_trial_model`, which re-routes ids — `kimi-k2`
is stored as `moonshot/kimi-k2`, Claude models as Bedrock ids — so storing
what the operator typed registered a row that matched nothing, with no
feedback. The endpoint now resolves the typed spelling against the values
trials actually store, registers every match (one model can have two
stored spellings, and excluding one would leave half the spend counting),
and 404s when nothing matches. The SQL predicate and its Python twin now
both compare `trials.model` verbatim, so they can no longer disagree about
which rows are excluded.

The CLI could never write. Mutations gated on `can_manage_api_keys`, which
rejects API-key auth by design because it guards key management itself —
so every `oddish cost-exclusions add` was a guaranteed 403 telling the
operator to use the credential they were already using. Operator-org
membership is the gate; `require_admin` already counts a full-scope key as
admin.

The experiment Cost tile claimed gathered spend was comped. It covers
member trials including gathered ones, but was passed the whole-experiment
excluded flag, which by design only excludes homed trials. Excluding a
collection is now rejected outright for the same reason: it homes no
trials, so it would exclude nothing while claiming everything was free.

Also: migration resets `SET LOCAL lock_timeout` (all pending revisions
share one transaction, so it leaked into later ones) and touches `trials`
last on downgrade as it already did on upgrade; restores the `llm_key_hash`
entry in Carl's SQL deny-list, which is inert once the column is gone but
is the only guard where code runs ahead of schema; CLI emits JSON on every
error branch and resolves non-canonical model spellings on remove.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zmdrcCqnkVM6hWjhPu2T9

Author: claude

backend/api/routers/cost_excluded_experiments.py

@@ -15,7 +15,7 @@
 from sqlalchemy.exc import IntegrityError, ProgrammingError
 from sqlalchemy.ext.asyncio import AsyncSession
 
-from auth import AuthContext, can_manage_api_keys, require_admin
+from auth import AuthContext, require_admin
 from auth.permissions import require_operator_org
 from oddish.db import (
     CostExcludedExperimentModel,
@@ -55,11 +55,6 @@ def _response(row: CostExcludedExperimentModel) -> CostExcludedExperimentRespons
 
 def _require_manage(auth: AuthContext) -> None:
     require_operator_org(auth)
-    if not can_manage_api_keys(auth):
-        raise HTTPException(
-            status_code=403,
-            detail="Only organization admins may edit the cost-exclusion list",
-        )
 
 
 def _unavailable(exc: ProgrammingError) -> HTTPException:
@@ -102,6 +97,18 @@ async def _resolve_experiment(session: AsyncSession, ref: str) -> ExperimentMode
     return matches[0]
 
 
+def _reject_collection(experiment: ExperimentModel) -> None:
+    if getattr(experiment, "is_collection", False):
+        raise HTTPException(
+            status_code=400,
+            detail=(
+                "that experiment is a collection: it homes no trials of its "
+                "own, so excluding it would exclude nothing. Exclude the "
+                "experiments that actually ran the work."
+            ),
+        )
+
+
 @router.get("", response_model=list[CostExcludedExperimentResponse])
 async def list_cost_excluded_experiments(
     auth: Annotated[AuthContext, Depends(require_admin)],
@@ -133,6 +140,7 @@ async def add_cost_excluded_experiment(
     try:
         async with get_session() as session:
             experiment = await _resolve_experiment(session, ref)
+            _reject_collection(experiment)
             existing = await session.scalars(
                 select(CostExcludedExperimentModel).where(
                     CostExcludedExperimentModel.experiment_id == experiment.id

backend/api/routers/cost_excluded_models.py

@@ -1,24 +1,18 @@
-"""Admin API for excluding whole models from cost accounting.
-
-Operator-only and deployment-wide: a model on this list stops counting on the
-admin cost dashboards and against quotas everywhere, for every org, and
-retroactively -- the reason its spend isn't real (sponsored capacity, a free
-preview tier) is a property of the model, not of when it ran.
-"""
-
 from __future__ import annotations
 
 from typing import Annotated
 
 from fastapi import APIRouter, Depends, HTTPException
 from pydantic import BaseModel
-from sqlalchemy import select
+from sqlalchemy import distinct, or_, select
 from sqlalchemy.exc import IntegrityError, ProgrammingError
+from sqlalchemy.ext.asyncio import AsyncSession
 
-from auth import AuthContext, can_manage_api_keys, require_admin
+from auth import AuthContext, require_admin
 from auth.permissions import require_operator_org
+from oddish.config import normalize_model_id
 from oddish.core.cost_exclusions import canonical_excluded_model
-from oddish.db import CostExcludedModelModel, get_session, utcnow
+from oddish.db import CostExcludedModelModel, TrialModel, get_session, utcnow
 from pg_errors import is_undefined_table_error
 
 router = APIRouter(prefix="/admin/cost-excluded-models", tags=["Admin"])
@@ -47,23 +41,52 @@ def _response(row: CostExcludedModelModel) -> CostExcludedModelResponse:
     )
 
 
-def _require_manage(auth: AuthContext) -> None:
-    require_operator_org(auth)
-    if not can_manage_api_keys(auth):
-        raise HTTPException(
-            status_code=403,
-            detail="Only organization admins may edit the cost-exclusion list",
-        )
+def _unavailable(exc: ProgrammingError) -> HTTPException:
+    if not is_undefined_table_error(exc):
+        raise exc
+    return HTTPException(
+        503,
+        "Cost exclusions are not available yet (schema is still migrating). "
+        "Try again shortly.",
+    )
 
 
-def _unavailable(exc: ProgrammingError) -> HTTPException:
-    if is_undefined_table_error(exc):
-        return HTTPException(
-            503,
-            "Cost exclusions are not available yet (schema is still "
-            "migrating). Try again shortly.",
+async def _resolve_models(session: AsyncSession, ref: str) -> list[str]:
+    """Every spelling of ``ref`` that ``trials.model`` actually stores.
+
+    Exclusion matches ``trials.model`` verbatim, but that value is written by
+    the agent-aware ``normalize_trial_model``, which re-routes ids: an operator
+    who types ``kimi-k2`` has trials stored under ``moonshot/kimi-k2``, and
+    ``claude-sonnet-4-5`` lands as a Bedrock id. Storing what they typed would
+    register a row that silently matches nothing. So resolve against the values
+    on disk and store those, and return every match -- one model can legitimately
+    have two stored spellings (bare ``grok-free-preview`` and the routed
+    ``xai/grok-free-preview``), and excluding only one would leave half the
+    spend counting.
+    """
+    canonical = canonical_excluded_model(ref)
+    rows = await session.scalars(
+        select(distinct(TrialModel.model)).where(
+            TrialModel.model.isnot(None),
+            or_(
+                TrialModel.model == ref,
+                TrialModel.model == canonical,
+                TrialModel.model.ilike(f"%/{canonical}"),
+            ),
         )
-    raise exc
+    )
+    stored = [m for m in rows if m]
+    # The ILIKE can over-match on a suffix (``a/b-c`` for ``b-c``); confirm in
+    # Python against the same normalization the writer used.
+    return sorted(
+        {
+            m
+            for m in stored
+            if m == ref
+            or normalize_model_id(m) == canonical
+            or (normalize_model_id(m) or "").endswith(f"/{canonical}")
+        }
+    )
 
 
 @router.get("", response_model=list[CostExcludedModelResponse])
@@ -83,43 +106,52 @@ async def list_cost_excluded_models(
         raise _unavailable(exc)
 
 
-@router.post("", response_model=CostExcludedModelResponse)
+@router.post("", response_model=list[CostExcludedModelResponse])
 async def add_cost_excluded_model(
     request: CreateCostExcludedModelRequest,
     auth: Annotated[AuthContext, Depends(require_admin)],
-) -> CostExcludedModelResponse:
-    _require_manage(auth)
-
-    # Store the canonical spelling so the live UNIQUE index collapses case and
-    # whitespace variants, and so the stored value matches ``trials.model``
-    # (normalized by the same function on write).
-    model_name = canonical_excluded_model(request.model_name)
-    if not model_name:
+) -> list[CostExcludedModelResponse]:
+    require_operator_org(auth)
+
+    if not canonical_excluded_model(request.model_name):
         raise HTTPException(status_code=400, detail="model must not be empty")
 
     try:
         async with get_session() as session:
+            names = await _resolve_models(session, request.model_name.strip())
+            if not names:
+                raise HTTPException(
+                    status_code=404,
+                    detail=(
+                        "no trials have run on that model, so excluding it "
+                        "would have no effect; check the spelling against the "
+                        "model shown on a trial"
+                    ),
+                )
+
             existing = await session.scalars(
-                select(CostExcludedModelModel).where(
-                    CostExcludedModelModel.model_name == model_name
+                select(CostExcludedModelModel.model_name).where(
+                    CostExcludedModelModel.model_name.in_(names)
                 )
             )
-            if existing.first() is not None:
+            fresh = [name for name in names if name not in set(existing)]
+            if not fresh:
                 raise HTTPException(status_code=409, detail="model is already excluded")
 
-            row = CostExcludedModelModel(
-                model_name=model_name,
-                label=request.label.strip(),
-                created_by_user_id=auth.user_id,
-            )
-            session.add(row)
+            rows = [
+                CostExcludedModelModel(
+                    model_name=name,
+                    label=request.label.strip(),
+                    created_by_user_id=auth.user_id,
+                )
+                for name in fresh
+            ]
+            session.add_all(rows)
             try:
                 await session.commit()
             except IntegrityError:
-                raise HTTPException(
-                    status_code=409, detail="model is already excluded"
-                )
-            return _response(row)
+                raise HTTPException(status_code=409, detail="model is already excluded")
+            return [_response(row) for row in rows]
     except ProgrammingError as exc:
         raise _unavailable(exc)
 
@@ -129,7 +161,7 @@ async def remove_cost_excluded_model(
     row_id: str,
     auth: Annotated[AuthContext, Depends(require_admin)],
 ) -> dict:
-    _require_manage(auth)
+    require_operator_org(auth)
     try:
         async with get_session() as session:
             result = await session.scalars(

backend/carl_tools.py

@@ -36,6 +36,7 @@
     "RangeTableFunc",
 }
 _SQL_FORBIDDEN_COLUMNS = {
+    "llm_key_hash",
     "harbor_config",
     "idempotency_key",
     "result",

backend/tests/test_cost_excluded_experiments_router.py

@@ -262,14 +262,15 @@ async def test_member_jwt_cannot_add(app, monkeypatch):
         app.dependency_overrides.pop(require_auth, None)
 
 
-async def test_full_api_key_cannot_add(app, monkeypatch):
-    _install_fake_get_session(monkeypatch, FakeSession())
+async def test_operator_full_api_key_can_add(app, monkeypatch):
+    # The CLI's only credential; operator-org membership is the real gate.
+    _install_fake_get_session(monkeypatch, FakeSession(results=[[_experiment()], []]))
     client = _client(app, _full_api_key())
     try:
         resp = await client.post(
             "/admin/cost-excluded-experiments", json={"experiment": "exp_1"}
         )
-        assert resp.status_code == 403
+        assert resp.status_code == 200, resp.text
     finally:
         await client.aclose()
         app.dependency_overrides.pop(require_auth, None)

backend/tests/test_cost_excluded_models_router.py

@@ -46,6 +46,9 @@ async def scalars(self, _stmt):
     def add(self, obj):
         self.added.append(obj)
 
+    def add_all(self, objs):
+        self.added.extend(objs)
+
     async def commit(self):
         # Simulate the Python-side column defaults a real flush would apply.
         from oddish.db import generate_id
@@ -132,27 +135,57 @@ async def admin_client(app):
         app.dependency_overrides.pop(require_auth, None)
 
 
-async def test_add_canonicalizes_model(admin_client, monkeypatch):
-    # One query: the duplicate check (miss).
-    session = FakeSession(results=[[]])
+async def test_add_stores_the_spelling_trials_actually_use(admin_client, monkeypatch):
+    # Queries: resolve against trials.model (hit), duplicate check (miss).
+    # The operator typed a bare id; trials store the agent-routed one, and that
+    # is what has to be stored or the exclusion matches nothing.
+    session = FakeSession(results=[["moonshot/kimi-k2"], []])
     _install_fake_get_session(monkeypatch, session)
 
     resp = await admin_client.post(
         "/admin/cost-excluded-models",
-        json={"model_name": "  XAI/Grok-4  ", "label": "sponsored"},
+        json={"model_name": "  Kimi-K2  ", "label": "sponsored"},
     )
     assert resp.status_code == 200, resp.text
     body = resp.json()
-    # Stored canonicalized so it matches trials.model, which is normalized by
-    # the same function on write.
-    assert body["model_name"] == "xai/grok-4"
-    assert body["label"] == "sponsored"
-    assert session.added[0].model_name == "xai/grok-4"
+    assert [r["model_name"] for r in body] == ["moonshot/kimi-k2"]
+    assert body[0]["label"] == "sponsored"
+    assert session.added[0].model_name == "moonshot/kimi-k2"
     assert session.committed
 
 
+async def test_add_covers_every_stored_spelling(admin_client, monkeypatch):
+    # One model can be stored two ways; excluding one would leave half the
+    # spend counting.
+    session = FakeSession(
+        results=[["grok-free-preview", "xai/grok-free-preview"], []]
+    )
+    _install_fake_get_session(monkeypatch, session)
+
+    resp = await admin_client.post(
+        "/admin/cost-excluded-models", json={"model_name": "grok-free-preview"}
+    )
+    assert resp.status_code == 200, resp.text
+    assert sorted(r["model_name"] for r in resp.json()) == [
+        "grok-free-preview",
+        "xai/grok-free-preview",
+    ]
+
+
+async def test_add_unknown_model_is_404(admin_client, monkeypatch):
+    # Silently registering a row that matches nothing is the failure mode this
+    # endpoint exists to prevent.
+    _install_fake_get_session(monkeypatch, FakeSession(results=[[]]))
+    resp = await admin_client.post(
+        "/admin/cost-excluded-models", json={"model_name": "never/ran"}
+    )
+    assert resp.status_code == 404
+
+
 async def test_add_duplicate_is_409(admin_client, monkeypatch):
-    _install_fake_get_session(monkeypatch, FakeSession(results=[[object()]]))
+    _install_fake_get_session(
+        monkeypatch, FakeSession(results=[["xai/grok-4"], ["xai/grok-4"]])
+    )
     resp = await admin_client.post(
         "/admin/cost-excluded-models", json={"model_name": "xai/grok-4"}
     )
@@ -166,7 +199,7 @@ class RacingSession(FakeSession):
         async def commit(self):
             raise IntegrityError("INSERT", {}, Exception("duplicate key"))
 
-    _install_fake_get_session(monkeypatch, RacingSession(results=[[]]))
+    _install_fake_get_session(monkeypatch, RacingSession(results=[["xai/grok-4"], []]))
     resp = await admin_client.post(
         "/admin/cost-excluded-models", json={"model_name": "xai/grok-4"}
     )
@@ -221,16 +254,16 @@ async def test_member_jwt_cannot_add(app, monkeypatch):
         app.dependency_overrides.pop(require_auth, None)
 
 
-async def test_full_api_key_cannot_add(app, monkeypatch):
-    # Mutations are Clerk-admin only: a full-scope API key must not be able to
-    # silently zero out a deployment's spend reporting.
-    _install_fake_get_session(monkeypatch, FakeSession())
+async def test_operator_full_api_key_can_add(app, monkeypatch):
+    # The CLI's only credential. `require_admin` already counts a full-scope
+    # key as admin; the operator-org check is what keeps this privileged.
+    _install_fake_get_session(monkeypatch, FakeSession(results=[["xai/grok-4"], []]))
     client = _client(app, _full_api_key())
     try:
         resp = await client.post(
             "/admin/cost-excluded-models", json={"model_name": "xai/grok-4"}
         )
-        assert resp.status_code == 403
+        assert resp.status_code == 200, resp.text
     finally:
         await client.aclose()
         app.dependency_overrides.pop(require_auth, None)

frontend/src/components/experiment-detail-view.tsx

@@ -777,7 +777,6 @@ function ExperimentSummaryBar({
             <NotRealSpendBadge
               excludedCostUsd={summary.excludedCostUsd}
               totalCostUsd={summary.costUsd}
-              wholeSubjectExcluded={summary.experimentCostExcluded}
             />
           )}
         </span>

oddish/alembic/versions/costexcl02_model_and_experiment_exclusions.py

@@ -77,13 +77,14 @@ def upgrade() -> None:
     op.execute("DROP TABLE IF EXISTS cost_excluded_llm_keys")
     op.execute("SET LOCAL lock_timeout = '5s'")
     op.execute("ALTER TABLE trials DROP COLUMN IF EXISTS llm_key_hash")
+    # All pending revisions share one transaction, so an un-reset SET LOCAL
+    # would impose this timeout on every later migration in the same run.
+    op.execute("SET LOCAL lock_timeout = DEFAULT")
 
 
 def downgrade() -> None:
     # Recreates the key feature's shape only. Neither the exclusion rows nor
     # the per-trial hashes are recoverable -- nothing else stored either.
-    op.execute("SET LOCAL lock_timeout = '5s'")
-    op.execute("ALTER TABLE trials ADD COLUMN IF NOT EXISTS llm_key_hash VARCHAR(64)")
     op.execute(
         """
         CREATE TABLE IF NOT EXISTS cost_excluded_llm_keys (
@@ -106,3 +107,6 @@ def downgrade() -> None:
     )
     op.execute("DROP TABLE IF EXISTS cost_excluded_experiments")
     op.execute("DROP TABLE IF EXISTS cost_excluded_models")
+    op.execute("SET LOCAL lock_timeout = '5s'")
+    op.execute("ALTER TABLE trials ADD COLUMN IF NOT EXISTS llm_key_hash VARCHAR(64)")
+    op.execute("SET LOCAL lock_timeout = DEFAULT")