feat/one-api

CI widening gates api-key unit tests (decisions, shells, service-lib) behind the DB-change condition

.github/workflows/pr-checks.yml:276

Widening from src/api-key/__tests__/api-key.integration.test.ts to src/api-key means decisions, shells, and service-lib test files — pure unit tests — are now gated on steps.db-changes.outputs.changed == 'true'. They cannot collect in the validate job (setup.ts throws without POSTGRES_URL) and are gated in the DB job. A pure decision-logic bug introduced in a non-schema file would skip all three suites. The widening was correct in intent (the new shells.test.ts needs to run) but the gate applies to unit tests that have no DB dependency.

Route-meta guard second for-loop is dead code — admitted is always empty when the first assertion passes

apps/platform/src/api/contracts/__tests__/route-meta.guard.test.ts:105

expect(admitted).toEqual([]) at line 101 proves no publicV1Contract leaf has 'service' in auth. The subsequent for-loop checking if (meta.auth.includes('service')) → expect visibility 'infra' can never execute — the condition is never true. The comment 'Durable form of the same rule, for when infra routes join this tree' is circular: infra routes cannot join publicV1Contract without the 'public visibility throughout' test also failing first. Remove the loop or move it to a test over the internal contract tree where service-auth infra routes actually live.

domain-patterns.md rule 'returning err() COMMITS' is imprecise — only commits when no query error was recorded on the handle

.claude/rules/domain-patterns.md:98

The full postgres.js mechanic: scope() attaches q.catch(e => uncaughtError ||= e) to every query; after callback resolves it re-throws if uncaughtError is set. So returning err() commits ONLY when no in-flight query error was recorded. If a write query inside the callback raises a DB error, uncaughtError is set, the Result is discarded, and the transaction rejects regardless. The documented safe case ('nothing before this point writes') is correct for createServiceKeyShell specifically, but the general statement 'return err() COMMITS' can mislead authors of more complex shells where a prior write failed.

5 confirmed postgres.js in-callback catch sites left unfixed — updateColumnConfigShell can COMMIT without outbox event

domains/core/src/column-configuration/column-configuration.shells.ts:130

domain-patterns.md documents the bug but does not fix it: column-configuration.shells.ts ~lines 130/215/367 and secret-configuration.shells.ts ~lines 296/825 all use try/catch inside db.transaction() callback returning err(...). postgres.js records the error on the handle and re-throws after the callback resolves — the Result is discarded and the shell throws a raw PostgresError. updateColumnConfigShell (~215) is highest risk: can COMMIT an entity update while the outbox event is never written, breaking the FCIS delivery guarantee. In secret-configuration, AWS compensation logic must stay inside the callback; only the error mapping moves to .catch().

withInternalAuth static-key comparison uses === not timingSafeEqual — timing oracle during W2 dual-accept window

packages/api/src/middleware/internal-auth.ts:78

The legacy x-api-key path still uses apiKey === expected — a non-constant-time JS equality that can leak the comparison position under JIT optimization. The new bearer-token path uses verifySecret (timingSafeEqual under the hood). During W2 dual-accept, both paths are live simultaneously. While the internal API is not publicly reachable (Vercel functions behind Step Functions ACLs), the transitional window leaves a timing oracle on the most privileged credential class. Fix: crypto.timingSafeEqual(Buffer.from(apiKey), Buffer.from(expected)) with a length pre-check.

scopesSatisfied(['*'] required) correctly rejects empty and narrowed grants — verified as fail-closed

packages/api/src/middleware/internal-auth.ts:86

INFO level: scopesSatisfied(auth.scopes, ['*']) is correctly fail-closed. empty [] → scopeSatisfied fails at !op guard ('*'.split(':') yields ['*', undefined]). ['bills:read'] → fails same guard. The two new tests (narrowed and empty) correctly pin this. No bypass found.

findByOrg unchecked OrgScopedApiKey cast relies on SQL NULL semantics — integration test is the only runtime guard

domains/core/src/api-key/api-key.queries.ts:41

eq(apiKeys.orgId, orgId) compiles to col = value; NULL = value → NULL, so service keys are correctly excluded. Cast to OrgScopedApiKey[] is unchecked at runtime. The integration isolation test (added in this diff) directly verifies the invariant against a real DB — that's the correct guard. Optional hardening: add isNotNull(apiKeys.orgId) as an explicit filter or a runtime assertion rows.every(r => r.orgId !== null) before the cast.

route-meta.guard.test.ts logical self-contradiction: 'public visibility throughout' will fail the moment the first infra route is added

apps/platform/src/api/contracts/__tests__/route-meta.guard.test.ts:84

The existing 'public visibility throughout' assertion (line 86: every leaf has visibility === 'public') is mutually exclusive with the new 'infra routes must be infra' comment (line 109). When W4 brings infra routes into the contract tree, the blanket public-only assertion will need to be narrowed (e.g. allow 'infra' too) before that migration can land. Flag this for update before W4.

makeRethrowingDb comment overstates accuracy — throws unconditionally, not only when a query error was recorded

domains/core/src/api-key/__tests__/api-key.shells.test.ts:480

The helper runs fn({}) then unconditionally throws raise. In real postgres.js, the throw fires only when a query inside the callback stored an error on uncaughtError. The regression tests happen to be correct (mocked insert returns a valid row, so the shape matches real postgres.js behavior), but the model could mislead a future author who tests a path where the callback itself could raise. Add a comment: 'simulates the driver's post-callback rethrow of a query error, not an arbitrary throw; in this mock the callback runs fully'.

Shell comment 'only a throw rolls back' ambiguous — applies inside callback, not at .catch() level

domains/core/src/api-key/api-key.shells.ts:471

A future author might add a throw inside .catch() under the mistaken belief that's needed. Clarify: 'only a throw from inside the callback triggers rollback — the .catch() runs after postgres.js has already rolled back'.

REGRESSION tests assert error code but not absence of throw — .resolves.toMatchObject() would make the invariant explicit

domains/core/src/api-key/__tests__/api-key.shells.test.ts:554

The three REGRESSION tests (lines 554, 567, 582) use const result = await createServiceKeyShell(...); expect(result.ok).toBe(false). An uncaught rejection from the shell IS surfaced as a test failure by vitest, so the tests are protective in practice. But await expect(createServiceKeyShell(...)).resolves.toMatchObject({ ok: false, error: { _tag: '...' } }) would make the intent explicit — 'resolves' asserts it did NOT throw.

buildKey fixture missing default kind: 'machine' — all non-service fixtures have kind: undefined at runtime

domains/core/src/api-key/__tests__/api-key.shells.test.ts:84

buildKey() doesn't set kind in its defaults. TypeScript compiles because overrides spread provides it for buildServiceKey, but other callers get kind: undefined. A test asserting result.value.apiKey.kind === 'machine' for a customer path would silently fail/pass depending on what the shell sets. Fix: add kind: 'machine' as the default.

.strict() applied only to eventData shape — missing top-level outbox schema fields would not be caught

domains/core/src/api-key/__tests__/api-key.shells.test.ts:139

ApiKeyCreatedEventSchema.shape.eventData.strict().safeParse(...) guards eventData fields but does not validate eventType, schemaVersion, aggregateId, aggregateType, eventMetadata on the top-level outbox entry. A dropped schemaVersion in the outbox write would be missed.

Second for-loop in route-meta guard is structurally dead code — gives false impression of a two-layer guard

apps/platform/src/api/contracts/__tests__/route-meta.guard.test.ts:105

The loop body can never execute when the first assertion has already passed. The comment 'Durable form of the same rule' creates false confidence. The first assertion is the sole load-bearing check. Remove the loop, or add a NOTE comment explaining it is intentionally inactive until infra routes exist in this tree.

internal-auth.ts retirement is untracked — mount could coexist with narrowed keys indefinitely

packages/api/src/middleware/internal-auth.ts:33

The comment says the mount is 'scheduled for retirement, not migration' but references no Linear issue or wave. W4 (28-call-site convergence) is the highest-risk milestone. Cross-reference to a Linear issue so the scope check doesn't become permanent scaffolding.