Skip to content

Commit eed0997

Browse files
committed
refactor: strip comments and compress the spend-exclusion code
Removes comments and docstrings from the files this PR introduces, and takes out what the red-team pass found was dead or duplicated. Net -613 lines across 27 files. Dead code removed: the `byok_env` plumbing through `_settle_trial_metering` and `store_trial_results`, whose only reader was the deleted key-hash stamp; the unused `exclusions` parameter on `build_task_status_response`, which no caller passed because `task_detail` rebuilds the trial list itself; and `CostExclusions.__bool__`. Deduplicated: the four Next.js proxy routes now use the app's shared `proxyBackendJson` / `proxyJsonRequest` instead of reimplementing them — 83 lines down to 10 — which also restores request-id propagation, upstream server-timing, and abort-signal forwarding, and drops the bespoke `{error, details}` envelope in favour of the backend payload the rest of the app already forwards. The two routers share `unavailable` and `soft_delete`; their genuinely different halves (model resolution against `trials.model` returning a list, experiment id-or-name resolution rejecting collections) stay separate. Pre-existing prose in shared modules is untouched — the accounting rationale in `cost_basis`, `experiment_cost`, `quotas` and `helpers` documents other features and this PR has no business deleting it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017zmdrcCqnkVM6hWjhPu2T9
1 parent d147ed8 commit eed0997

27 files changed

Lines changed: 132 additions & 715 deletions
Lines changed: 21 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,3 @@
1-
"""Admin API for excluding an experiment's spend from cost accounting.
2-
3-
Operator-only and deployment-wide, like the model list. Scoped to trials
4-
**homed** in the experiment, so a collection that merely gathers other
5-
experiments' trials cannot launder their cost.
6-
"""
7-
81
from __future__ import annotations
92

103
from typing import Annotated
@@ -15,15 +8,10 @@
158
from sqlalchemy.exc import IntegrityError, ProgrammingError
169
from sqlalchemy.ext.asyncio import AsyncSession
1710

11+
from api.routers.cost_exclusions_shared import soft_delete, unavailable
1812
from auth import AuthContext, require_admin
1913
from auth.permissions import require_operator_org
20-
from oddish.db import (
21-
CostExcludedExperimentModel,
22-
ExperimentModel,
23-
get_session,
24-
utcnow,
25-
)
26-
from pg_errors import is_undefined_table_error
14+
from oddish.db import CostExcludedExperimentModel, ExperimentModel, get_session
2715

2816
router = APIRouter(prefix="/admin/cost-excluded-experiments", tags=["Admin"])
2917

@@ -53,28 +41,7 @@ def _response(row: CostExcludedExperimentModel) -> CostExcludedExperimentRespons
5341
)
5442

5543

56-
def _require_manage(auth: AuthContext) -> None:
57-
require_operator_org(auth)
58-
59-
60-
def _unavailable(exc: ProgrammingError) -> HTTPException:
61-
if is_undefined_table_error(exc):
62-
return HTTPException(
63-
503,
64-
"Cost exclusions are not available yet (schema is still "
65-
"migrating). Try again shortly.",
66-
)
67-
raise exc
68-
69-
7044
async def _resolve_experiment(session: AsyncSession, ref: str) -> ExperimentModel:
71-
"""The experiment an operator meant, by id or by name.
72-
73-
An exact id wins, and resolves with ``include_deleted``: spend from a
74-
soft-deleted experiment still shows on cost surfaces, so it must stay
75-
excludable. Names resolve among live experiments only and must be
76-
unambiguous -- experiment names are not unique.
77-
"""
7845
by_id = await session.scalars(
7946
select(ExperimentModel)
8047
.where(ExperimentModel.id == ref)
@@ -97,18 +64,6 @@ async def _resolve_experiment(session: AsyncSession, ref: str) -> ExperimentMode
9764
return matches[0]
9865

9966

100-
def _reject_collection(experiment: ExperimentModel) -> None:
101-
if getattr(experiment, "is_collection", False):
102-
raise HTTPException(
103-
status_code=400,
104-
detail=(
105-
"that experiment is a collection: it homes no trials of its "
106-
"own, so excluding it would exclude nothing. Exclude the "
107-
"experiments that actually ran the work."
108-
),
109-
)
110-
111-
11267
@router.get("", response_model=list[CostExcludedExperimentResponse])
11368
async def list_cost_excluded_experiments(
11469
auth: Annotated[AuthContext, Depends(require_admin)],
@@ -123,15 +78,15 @@ async def list_cost_excluded_experiments(
12378
)
12479
return [_response(row) for row in rows]
12580
except ProgrammingError as exc:
126-
raise _unavailable(exc)
81+
raise unavailable(exc)
12782

12883

12984
@router.post("", response_model=CostExcludedExperimentResponse)
13085
async def add_cost_excluded_experiment(
13186
request: CreateCostExcludedExperimentRequest,
13287
auth: Annotated[AuthContext, Depends(require_admin)],
13388
) -> CostExcludedExperimentResponse:
134-
_require_manage(auth)
89+
require_operator_org(auth)
13590

13691
ref = request.experiment.strip()
13792
if not ref:
@@ -140,7 +95,15 @@ async def add_cost_excluded_experiment(
14095
try:
14196
async with get_session() as session:
14297
experiment = await _resolve_experiment(session, ref)
143-
_reject_collection(experiment)
98+
if getattr(experiment, "is_collection", False):
99+
raise HTTPException(
100+
status_code=400,
101+
detail=(
102+
"that experiment is a collection: it homes no trials of "
103+
"its own, so excluding it would exclude nothing. Exclude "
104+
"the experiments that actually ran the work."
105+
),
106+
)
144107
existing = await session.scalars(
145108
select(CostExcludedExperimentModel).where(
146109
CostExcludedExperimentModel.experiment_id == experiment.id
@@ -153,7 +116,6 @@ async def add_cost_excluded_experiment(
153116

154117
row = CostExcludedExperimentModel(
155118
experiment_id=experiment.id,
156-
# A display snapshot: the row outlives the experiment.
157119
experiment_name=experiment.name,
158120
label=request.label.strip(),
159121
created_by_user_id=auth.user_id,
@@ -167,29 +129,23 @@ async def add_cost_excluded_experiment(
167129
)
168130
return _response(row)
169131
except ProgrammingError as exc:
170-
raise _unavailable(exc)
132+
raise unavailable(exc)
171133

172134

173135
@router.delete("/{row_id}")
174136
async def remove_cost_excluded_experiment(
175137
row_id: str,
176138
auth: Annotated[AuthContext, Depends(require_admin)],
177139
) -> dict:
178-
_require_manage(auth)
140+
require_operator_org(auth)
179141
try:
180142
async with get_session() as session:
181-
result = await session.scalars(
182-
select(CostExcludedExperimentModel).where(
183-
CostExcludedExperimentModel.id == row_id
184-
)
143+
await soft_delete(
144+
session,
145+
CostExcludedExperimentModel,
146+
row_id,
147+
"cost-excluded experiment not found",
185148
)
186-
row = result.first()
187-
if row is None:
188-
raise HTTPException(
189-
status_code=404, detail="cost-excluded experiment not found"
190-
)
191-
row.deleted_at = utcnow()
192-
await session.commit()
193149
except ProgrammingError as exc:
194-
raise _unavailable(exc)
150+
raise unavailable(exc)
195151
return {"deleted": row_id}

backend/api/routers/cost_excluded_models.py

Lines changed: 14 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,12 @@
88
from sqlalchemy.exc import IntegrityError, ProgrammingError
99
from sqlalchemy.ext.asyncio import AsyncSession
1010

11+
from api.routers.cost_exclusions_shared import soft_delete, unavailable
1112
from auth import AuthContext, require_admin
1213
from auth.permissions import require_operator_org
1314
from oddish.config import normalize_model_id
1415
from oddish.core.cost_exclusions import canonical_excluded_model
15-
from oddish.db import CostExcludedModelModel, TrialModel, get_session, utcnow
16-
from pg_errors import is_undefined_table_error
16+
from oddish.db import CostExcludedModelModel, TrialModel, get_session
1717

1818
router = APIRouter(prefix="/admin/cost-excluded-models", tags=["Admin"])
1919

@@ -41,29 +41,7 @@ def _response(row: CostExcludedModelModel) -> CostExcludedModelResponse:
4141
)
4242

4343

44-
def _unavailable(exc: ProgrammingError) -> HTTPException:
45-
if not is_undefined_table_error(exc):
46-
raise exc
47-
return HTTPException(
48-
503,
49-
"Cost exclusions are not available yet (schema is still migrating). "
50-
"Try again shortly.",
51-
)
52-
53-
5444
async def _resolve_models(session: AsyncSession, ref: str) -> list[str]:
55-
"""Every spelling of ``ref`` that ``trials.model`` actually stores.
56-
57-
Exclusion matches ``trials.model`` verbatim, but that value is written by
58-
the agent-aware ``normalize_trial_model``, which re-routes ids: an operator
59-
who types ``kimi-k2`` has trials stored under ``moonshot/kimi-k2``, and
60-
``claude-sonnet-4-5`` lands as a Bedrock id. Storing what they typed would
61-
register a row that silently matches nothing. So resolve against the values
62-
on disk and store those, and return every match -- one model can legitimately
63-
have two stored spellings (bare ``grok-free-preview`` and the routed
64-
``xai/grok-free-preview``), and excluding only one would leave half the
65-
spend counting.
66-
"""
6745
canonical = canonical_excluded_model(ref)
6846
rows = await session.scalars(
6947
select(distinct(TrialModel.model)).where(
@@ -75,16 +53,16 @@ async def _resolve_models(session: AsyncSession, ref: str) -> list[str]:
7553
),
7654
)
7755
)
78-
stored = [m for m in rows if m]
79-
# The ILIKE can over-match on a suffix (``a/b-c`` for ``b-c``); confirm in
80-
# Python against the same normalization the writer used.
8156
return sorted(
8257
{
8358
m
84-
for m in stored
85-
if m == ref
86-
or normalize_model_id(m) == canonical
87-
or (normalize_model_id(m) or "").endswith(f"/{canonical}")
59+
for m in rows
60+
if m
61+
and (
62+
m == ref
63+
or normalize_model_id(m) == canonical
64+
or (normalize_model_id(m) or "").endswith(f"/{canonical}")
65+
)
8866
}
8967
)
9068

@@ -103,7 +81,7 @@ async def list_cost_excluded_models(
10381
)
10482
return [_response(row) for row in rows]
10583
except ProgrammingError as exc:
106-
raise _unavailable(exc)
84+
raise unavailable(exc)
10785

10886

10987
@router.post("", response_model=list[CostExcludedModelResponse])
@@ -153,7 +131,7 @@ async def add_cost_excluded_model(
153131
raise HTTPException(status_code=409, detail="model is already excluded")
154132
return [_response(row) for row in rows]
155133
except ProgrammingError as exc:
156-
raise _unavailable(exc)
134+
raise unavailable(exc)
157135

158136

159137
@router.delete("/{row_id}")
@@ -164,18 +142,9 @@ async def remove_cost_excluded_model(
164142
require_operator_org(auth)
165143
try:
166144
async with get_session() as session:
167-
result = await session.scalars(
168-
select(CostExcludedModelModel).where(
169-
CostExcludedModelModel.id == row_id
170-
)
145+
await soft_delete(
146+
session, CostExcludedModelModel, row_id, "cost-excluded model not found"
171147
)
172-
row = result.first()
173-
if row is None:
174-
raise HTTPException(
175-
status_code=404, detail="cost-excluded model not found"
176-
)
177-
row.deleted_at = utcnow()
178-
await session.commit()
179148
except ProgrammingError as exc:
180-
raise _unavailable(exc)
149+
raise unavailable(exc)
181150
return {"deleted": row_id}
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
from __future__ import annotations
2+
3+
from fastapi import HTTPException
4+
from sqlalchemy import select
5+
from sqlalchemy.exc import ProgrammingError
6+
from sqlalchemy.ext.asyncio import AsyncSession
7+
8+
from oddish.db import utcnow
9+
from pg_errors import is_undefined_table_error
10+
11+
12+
def unavailable(exc: ProgrammingError) -> HTTPException:
13+
if not is_undefined_table_error(exc):
14+
raise exc
15+
return HTTPException(
16+
503,
17+
"Cost exclusions are not available yet (schema is still migrating). "
18+
"Try again shortly.",
19+
)
20+
21+
22+
async def soft_delete(
23+
session: AsyncSession, model, row_id: str, not_found_detail: str
24+
) -> None:
25+
result = await session.scalars(select(model).where(model.id == row_id))
26+
row = result.first()
27+
if row is None:
28+
raise HTTPException(status_code=404, detail=not_found_detail)
29+
row.deleted_at = utcnow()
30+
await session.commit()

backend/tests/test_cost_excluded_experiments_router.py

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,6 @@ def __iter__(self):
3333

3434

3535
class FakeSession:
36-
"""Returns one queued result list per scalars() call, in order."""
37-
3836
def __init__(self, results=()):
3937
self.results = [list(rows) for rows in results]
4038
self.added: list[object] = []
@@ -136,7 +134,6 @@ async def admin_client(app):
136134

137135

138136
async def test_add_by_id_snapshots_name(admin_client, monkeypatch):
139-
# Queries: resolve by id (hit), duplicate check (miss).
140137
session = FakeSession(results=[[_experiment()], []])
141138
_install_fake_get_session(monkeypatch, session)
142139

@@ -147,14 +144,12 @@ async def test_add_by_id_snapshots_name(admin_client, monkeypatch):
147144
assert resp.status_code == 200, resp.text
148145
body = resp.json()
149146
assert body["experiment_id"] == "exp_1"
150-
# Name is a display snapshot: the row outlives the experiment.
151147
assert body["experiment_name"] == "glm sweep"
152148
assert session.added[0].experiment_id == "exp_1"
153149
assert session.committed
154150

155151

156152
async def test_add_by_name_resolves(admin_client, monkeypatch):
157-
# Queries: resolve by id (miss), by name (hit), duplicate check (miss).
158153
session = FakeSession(results=[[], [_experiment()], []])
159154
_install_fake_get_session(monkeypatch, session)
160155

@@ -166,8 +161,6 @@ async def test_add_by_name_resolves(admin_client, monkeypatch):
166161

167162

168163
async def test_add_ambiguous_name_is_409(admin_client, monkeypatch):
169-
# Experiment names are not unique, so an ambiguous one must not silently
170-
# exclude the wrong experiment's spend.
171164
session = FakeSession(results=[[], [_experiment("exp_1"), _experiment("exp_2")]])
172165
_install_fake_get_session(monkeypatch, session)
173166

@@ -263,7 +256,6 @@ async def test_member_jwt_cannot_add(app, monkeypatch):
263256

264257

265258
async def test_operator_full_api_key_can_add(app, monkeypatch):
266-
# The CLI's only credential; operator-org membership is the real gate.
267259
_install_fake_get_session(monkeypatch, FakeSession(results=[[_experiment()], []]))
268260
client = _client(app, _full_api_key())
269261
try:

0 commit comments

Comments
 (0)