feat/one-api

persist() has no observable signal on persistent write failure

apps/platform/src/api/utils/public-v1-telemetry.ts

All errors are intentionally swallowed — the right call for not blocking requests. But a persistent DB write failure (connection pool exhaustion, schema drift after a migration) is completely invisible: no counter, no log line, no metric. The test proved two silent bugs already existed. Consider emitting a single `console.warn(JSON.stringify({ event: 'api.telemetry.write_failed' }))` in the outer catch — no payload, just a detectable signal.

durationMs stored with floor-truncation — integer column, no Math.round()

apps/platform/src/api/utils/public-v1-telemetry.ts

The schema column is `integer`. If `durationMs` is derived from `performance.now()` subtraction, the float is floor-truncated by Postgres — not rounded. Explicit `Math.round(durationMs)` at the call site would make the intent clear and avoid systematic under-reporting by up to ~1 ms per row.

Proposed get_user_org_ids() cast raises exception on malformed claims

docs/design/machine-caller-isolation.md

The proposed function casts `current_setting('request.jwt.claims', true)::json ->> 'batu_org_id')::uuid` without guarding against non-UUID strings. A valid-JSON but non-UUID claim value (e.g. `{"batu_org_id": "not-a-uuid"}`) raises `invalid_text_representation` rather than silently yielding NULL. Wrap in `BEGIN ... EXCEPTION WHEN others THEN RETURN; END` or use `pg_try_cast` equivalent before the migration is written.

rehearse-upgrade.sh localhost guard is string-based, not IP-resolved

scripts/db/rehearse-upgrade.sh

The guard checks if POSTGRES_URL contains '127.0.0.1' or 'localhost'. A hostname that resolves to loopback via /etc/hosts would bypass it. For a script that drops the database, consider also requiring `--confirm` as a secondary flag.

get_user_admin_org_ids() not-widened boundary should be structurally enforced in future migration

docs/design/machine-caller-isolation.md

The design correctly identifies that the admin helper must not be widened (so machine callers can't read api_keys). When the migration is written, add a regression test that specifically asserts a machine credential cannot SELECT from api_keys — documenting this as a test invariant prevents a future 'cleanup' from silently removing the protection.

Silent skip via `if (!available) return;` passes with 0 assertions

apps/platform/src/__tests__/integration/api-telemetry-persistence.test.ts:65

When the database is unavailable, each test body returns undefined and Vitest marks it PASSED — not SKIPPED. Zero assertions ran, so a broken schema that happens to pass `LIMIT 1` would make all three tests green. Use `it.skipIf(!available)(...)` or call `ctx.skip()` / `vi.skip()` inside the body so the runner surfaces them as SKIPPED.

afterAll cleanup misses `_refused` principal row on test failure

apps/platform/src/__tests__/integration/api-telemetry-persistence.test.ts:114

The second test deletes the `${PRINCIPAL}_refused` row inline at the end of its body. If `waitForRow` times out or an assertion fails first, that row is orphaned. Move cleanup into `afterAll` using a LIKE prefix match (`WHERE principal LIKE '${PRINCIPAL}%'`) so it runs unconditionally.

No test for `client_error` outcome branch (4xx non-auth statuses)

apps/platform/src/__tests__/integration/api-telemetry-persistence.test.ts

The suite covers `ok` (status 200) and `refused` (status 401) but not `client_error` (e.g. 400, 422, 404) or `server_error` (5xx). `outcomeOf` has four branches; only two are exercised end-to-end.

No assertion that `null` org round-trips as SQL NULL (not string 'null')

apps/platform/src/__tests__/integration/api-telemetry-persistence.test.ts

The second test passes `org: null` but never asserts `row.orgPublicId === null`. Given the history of the column-name spread bug, confirming the nullable column round-trips correctly is a meaningful regression guard.

waitForRow masks DB connection errors with misleading timeout failure

apps/platform/src/__tests__/integration/api-telemetry-persistence.test.ts

If the DB connection drops mid-poll, the `while` loop swallows the query error and spins for the full 15 s before returning null. CI failures slow and the final error message ('no telemetry row was persisted') is misleading. Let unexpected errors propagate so the failure is immediate and diagnostic.

The `after()` production path has no integration coverage — only fallback is tested

apps/platform/src/__tests__/integration/api-telemetry-persistence.test.ts

`next/server` is unavailable in Vitest, so `persist()` always takes the inline fallback path. The `after()` scheduling branch — the production code path — is untested. This is acceptable now but worth an e2e smoke test or a manual confirmation after deploy.

Inline comments in persist() describe WHAT, not WHY — violates convention

apps/platform/src/api/utils/public-v1-telemetry.ts

Several inline comments annotate code mechanics (e.g. 'Mapped FIELD BY FIELD, never spread', 'No request context — fall through and write inline', 'Module resolution or connection failure'). The convention is WHY only; WHAT comments are noise to readers who can read the code. The field-mapping risk is better addressed by a compile-time assertion or a narrow type than by a prose warning inside a catch-all sink.

byTime solo index is likely redundant given both composites cover time

packages/database/src/schema/api-request-events.ts

Every real query in api-usage.sql filters by org or route in addition to time, so Postgres will prefer `byOrg` or `byRoute` over the solo `byTime` index. The only pure-time scan is the retention DELETE, which can use either composite index by ignoring the leading column. Dropping `byTime` removes write amplification on every insert with no practical read cost.

Inline cleanup in second test case is asymmetric with afterAll pattern

apps/platform/src/__tests__/integration/api-telemetry-persistence.test.ts:114

First test case relies on afterAll for cleanup; second test cleans up inline. Consolidating into afterAll with a prefix-LIKE match would be consistent and failure-safe.