Skip to content

Tier-4 - #5

Merged
Hum2a merged 12 commits into
tier-3from
tier-4
Jul 3, 2026
Merged

Tier-4#5
Hum2a merged 12 commits into
tier-3from
tier-4

Conversation

@Hum2a

@Hum2a Hum2a commented Jul 3, 2026

Copy link
Copy Markdown
Owner
  • Added support for JWT authentication, allowing users to exchange API keys for JWTs via the new /auth/token endpoint.
  • Introduced API key management with routes for listing and creating API keys, enhancing security and user control.
  • Updated the OpenAPI specification to reflect new authentication methods and security schemes.
  • Enhanced the principal resolution logic to support JWTs and API keys, improving authorization handling across the application.
  • Added tests to ensure proper functionality of the new authentication features and their integration with existing components.

Summary by cubic

Adds JWT auth with an /auth/token exchange and hashed API keys, and updates principal resolution to accept JWTs or API keys. Also adds DO-backed rate limiting, KV caching for session lists, structured logging, and SPA upgrades (Owner switcher, API key UI, PWA/offline, i18n, scorecard export).

  • New Features

    • Auth: POST /auth/token (HS256, 1h TTL), API key list/create/revoke (hashed in DB), updated OpenAPI, and JWT/API key resolution across routes; web uses AuthProvider + OwnerSwitcher with demo keys still supported.
    • Scale: RateLimiter Durable Object per IP, KV cache for GET /sessions (60s KV / 15s HTTP, X-Cache), structured JSON logs with X-Trace-Id, and deep readiness GET /health?deep=1.
    • DX/Docs/Tests: OpenAPI exported to /openapi.json with Redoc page, contract + unit + visual tests added, and wrangler@4 upgrade.
    • UX: PWA via vite-plugin-pwa with offline read-only cache (React Query persistence), react-i18next (en), scorecard export, improved file input, and optional analytics.
  • Migration

    • Run DB migrations to create api_keys (e.g. npm run db:prepare).
    • Set OCHE_JWT_SECRET (Wrangler secret) and sync .dev.vars; keep demo keys for local use.
    • Bind RATE_LIMITER DO and CACHE KV in apps/api/wrangler.toml (apply migrations).
    • Rebuild web (OpenAPI export runs in apps/web build).

Written for commit 85c2198. Summary will update on new commits.

Review in cubic

Hum2a and others added 12 commits July 3, 2026 13:27
- Added support for JWT authentication, allowing users to exchange API keys for JWTs via the new `/auth/token` endpoint.
- Introduced API key management with routes for listing and creating API keys, enhancing security and user control.
- Updated the OpenAPI specification to reflect new authentication methods and security schemes.
- Enhanced the principal resolution logic to support JWTs and API keys, improving authorization handling across the application.
- Added tests to ensure proper functionality of the new authentication features and their integration with existing components.
…ures

- Introduced `RateLimiter` Durable Object for per-IP rate limiting, replacing in-memory token buckets.
- Added KV caching for session lists with a 15s TTL, including cache invalidation on session updates.
- Implemented structured JSON logging with `X-Trace-Id` for enhanced observability and error tracking.
- Updated health check endpoint to support deep readiness checks with database connectivity verification.
- Enhanced API routes and middleware to integrate new features, ensuring compliance with updated specifications.
- Added tests for health checks, rate limiting, and session caching functionalities to ensure reliability.
…ken setup

- Clarified the process for creating KV namespaces, specifying that commands should be run from the `apps/api` directory.
- Added instructions for copying KV namespace IDs into `wrangler.toml` for staging and production environments.
- Included guidance for local development without Cloudflare KV and troubleshooting authentication errors related to the Cloudflare API token.
- Added unit tests for the History and Overview components using Vitest and Testing Library, improving test coverage and reliability.
- Introduced contract tests to ensure OpenAPI paths align with expected schemas, enhancing API stability.
- Implemented visual regression tests with Playwright to capture UI changes and maintain visual consistency.
- Updated README and ROADMAP to reflect new testing capabilities and deployment instructions, including a full staging pipeline command.
- Upgraded Wrangler to version 4 across the project for improved development experience.
- Updated session list caching to use a 60s TTL in Cloudflare KV with a 15s HTTP max-age for responses, improving cache efficiency.
- Introduced a new utility function `isSessionId` for validating session IDs against a UUID format.
- Enhanced session retrieval routes to validate session IDs before processing requests, ensuring better error handling.
- Refactored cache-related functions to handle errors gracefully, maintaining API reliability.
- Updated documentation to reflect changes in caching strategy and session ID validation.
- Integrated PWA capabilities using vite-plugin-pwa, enabling offline access and caching for session data.
- Added i18n support with react-i18next, providing English translations for navigation and session details.
- Implemented a scorecard export feature, allowing users to generate print-ready scorecards for sessions.
- Introduced an OfflineBanner component to notify users when offline, enhancing user experience.
- Updated package dependencies to include necessary libraries for analytics and query persistence.
- Enhanced documentation to reflect new features and usage instructions.
… updates

- Updated the `FileField` component to include a visible button for file selection, improving user experience.
- Implemented state management to display the selected file name, enhancing feedback for users.
- Modified tooltip descriptions in the documentation to clarify the new file input pattern and usage guidelines.
- Ensured accessibility by maintaining proper aria attributes and labels for the file input.
@Hum2a
Hum2a merged commit 670ca34 into tier-3 Jul 3, 2026
1 check failed

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

24 issues found across 94 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/api/src/principal.ts">

<violation number="1" location="apps/api/src/principal.ts:27">
P2: JWT auth can be skipped for valid `Authorization` headers that use a different bearer-scheme case, because parsing currently requires `Bearer ` with exact casing. Since auth-scheme matching is case-insensitive, this can reject otherwise valid JWT requests depending on client behavior. A case-insensitive parse would make JWT auth robust across conforming clients.</violation>
</file>

<file name="packages/db/src/api-keys.ts">

<violation number="1" location="packages/db/src/api-keys.ts:20">
P1: API key exchange can resolve to an owner without proving the provided key when `app.current_owner` is already present in the transaction context. The lookup query in `resolveApiKeyOwner` uses `limit(1)` without filtering by `keyHash`, so it can return any row visible via the owner branch of the new `api_keys_select` policy. Adding an explicit `WHERE key_hash = ... AND revoked_at IS NULL` in the query keeps resolution tied to the submitted key regardless of surrounding session state.</violation>
</file>

<file name="apps/api/src/openapi/spec.ts">

<violation number="1" location="apps/api/src/openapi/spec.ts:170">
P1: Both `/auth/keys` operations are missing the required `responses` field. The OpenAPI 3.1 spec mandates every operation include a `responses` object with at least one response — a spec-compliant code generator or validator will reject these operations as invalid. Add response definitions covering the success and error cases (e.g., 200/201 + 401).</violation>
</file>

<file name="apps/api/src/lib/logger.ts">

<violation number="1" location="apps/api/src/lib/logger.ts:19">
P2: Info-level messages are written to stderr via `console.warn`, which conflates informational diagnostics with warnings. In production, stderr often drives alerting or error log aggregation, so info noise there reduces signal clarity. Recommend routing `'info'` to `console.log` (stdout) and `'warn'` to `console.warn` (stderr) to keep log streams semantically clean.</violation>
</file>

<file name="packages/db/migrations/meta/_journal.json">

<violation number="1" location="packages/db/migrations/meta/_journal.json:16">
P2: Missing snapshot file `meta/0001_snapshot.json` for migration `0001_api_keys`. Drizzle Kit uses the latest snapshot to compute schema diffs when generating the next migration. Without `0001_snapshot.json`, the next `drizzle-kit generate` won't know the current schema state and may produce incorrect or duplicate migrations. Commit the generated snapshot alongside the SQL file and journal entry.</violation>
</file>

<file name="scripts/env-setup.mjs">

<violation number="1" location="scripts/env-setup.mjs:25">
P2: The `.dev.vars.example` template doesn't include `OCHE_JWT_SECRET`, so `setup:dev-vars` (init) and `setup:dev-vars:merge` will both produce a `.dev.vars` file that's missing this key. Only `setup:dev-vars:sync` handles it (because `applyEnvValues` appends unseen keys), but new contributors who follow `init` first will have an incomplete file. Add `OCHE_JWT_SECRET` to `apps/api/.dev.vars.example` to keep the template consistent with the synced variables.</violation>
</file>

<file name="package.json">

<violation number="1" location="package.json:66">
P3: `deploy:staging:full` runs `db:migrate:staging` twice — once explicitly as the first step and then again as part of `deploy:staging`. Since migrations are idempotent this won't break anything, but it adds unnecessary time to the deploy pipeline and makes the dependency between steps harder to reason about. Recommend removing the explicit `db:migrate:staging` call from `deploy:staging:full` since `deploy:staging` already includes it.</violation>
</file>

<file name=".cursor/rules/60-realtime-do.mdc">

<violation number="1" location=".cursor/rules/60-realtime-do.mdc:5">
P2: This rule now documents that `RateLimiter` must be SQLite-backed, but the `globs` field still only targets `session-room.ts`. The rule won't activate when editing `rate-limiter.ts`, so future changes there can miss the SQLite constraint. Update the globs to include both files.</violation>
</file>

<file name="packages/shared/src/auth.ts">

<violation number="1" location="packages/shared/src/auth.ts:34">
P2: `createdAt` and `revokedAt` in `ApiKeySummary` and `CreateApiKeyResponse` use bare `z.string()` instead of `z.string().datetime()`. The project convention in `schema.ts` applies `.datetime()` to every timestamp field (`ScoreEventSchema.createdAt`, `SessionSchema.createdAt`, `SessionSchema.updatedAt`). Without format validation, invalid date strings (e.g. garbage text, ISO week dates, timestamps in wrong timezone) would pass validation silently, leading to inconsistent data downstream.</violation>
</file>

<file name="apps/web/src/routes/Overview.test.tsx">

<violation number="1" location="apps/web/src/routes/Overview.test.tsx:28">
P0: The test will crash at module evaluation time. `vi.mock` is hoisted and its factory runs before `const patchScores` is initialized, so the factory's reference to `patchScores` throws a `ReferenceError: Cannot access 'patchScores' before initialization`. Define `patchScores` with `vi.hoisted()` so it's available when the mock factory executes.</violation>
</file>

<file name="scripts/export-openapi.mjs">

<violation number="1" location="scripts/export-openapi.mjs:22">
P1: The `shell: true` option breaks the inline TypeScript code because it contains shell metacharacters (`;`, single quotes, newlines). The shell interprets `;` as a command separator and single quotes as shell quoting, so `tsx -e` only sees part of the code. The script will fail on any Unix shell. Remove `shell: true` — the command doesn't need shell features and works fine with direct argument passing.</violation>
</file>

<file name="docs/AUTH.md">

<violation number="1" location="docs/AUTH.md:7">
P2: The auth doc currently describes principal resolution in the opposite order from the API implementation. In practice, `resolvePrincipal` checks Bearer/`?token` first and only falls back to `x-oche-owner`/`?key` if JWT is absent, but this section lists API key first. Aligning this order will avoid incorrect integration assumptions when both credentials are present.</violation>

<violation number="2" location="docs/AUTH.md:10">
P3: Inconsistent path for the API key management UI. The doc first says `/auth/keys` (line 10) but the SPA flow section (line 22) and the cursor rule (`34-auth.mdc`) both say `/settings/keys`. Align the first reference to match the actual route.</violation>
</file>

<file name="apps/web/vite.config.js">

<violation number="1" location="apps/web/vite.config.js:57">
P1: Owner-scoped session data can be reused across users because the new service-worker cache stores `/sessions` responses without partitioning by authentication context. With `NetworkFirst`, a later user on the same browser profile can receive cached data from a previous principal during offline/timeout paths. Consider disabling runtime caching for authenticated `/sessions` responses (or explicitly keying cache entries by principal).</violation>

<violation number="2" location="apps/web/vite.config.js:59">
P1: The `apiOrigin()` helper referenced inside the `runtimeCaching.urlPattern` arrow function won't survive serialization into the generated service worker. workbox-build serializes function-based patterns via `.toString()`, which captures only the arrow function — the outer `apiOrigin` reference will be dangling. This causes a ReferenceError at runtime on every navigation that the sessions route pattern evaluates. Inline the origin logic directly into the arrow function instead, or compute the origin as a const outside the runtimeCaching config and reference that literal string.</violation>
</file>

<file name="apps/api/src/routes/auth.ts">

<violation number="1" location="apps/api/src/routes/auth.ts:39">
P2: Each protected route creates two separate postgres connection pools per request — one in the `auth.use('*')` middleware and another in the route handler. The middleware already has a `db` instance after resolving the principal but doesn't share it with downstream handlers, so `GET /keys`, `POST /keys`, and `DELETE /keys/:id` all call `getDb()` a second time. Store the `db` instance in the request context (e.g., extend `AuthVariables` with `db`) so handlers reuse it instead of creating a redundant pool.</violation>
</file>

<file name="apps/web/vite.config.ts">

<violation number="1" location="apps/web/vite.config.ts:60">
P0: The `urlPattern` callback in `runtimeCaching` calls `apiOrigin()`, but that function won't exist in the generated service worker's scope. Workbox-build serializes function-valued `urlPattern` entries via `.toString()`, which strips all closure and module-scope references — the resulting SW will throw a `ReferenceError` on every `/sessions/*` fetch, breaking the offline/network-first caching entirely. Compute the origin value at config-load time and use a RegExp pattern instead.</violation>
</file>

<file name="apps/web/src/routes/Overview.tsx">

<violation number="1" location="apps/web/src/routes/Overview.tsx:61">
P3: This extra guard is unreachable and adds dead branching to the render path. `panelId` is only `null` when `activeSessions.length === 0`, but that case already returns `QueryEmpty` above. Removing this branch keeps the flow simpler and avoids future confusion about whether `panelId` can be null here.</violation>
</file>

<file name="apps/web/src/components/ui/form-fields.tsx">

<violation number="1" location="apps/web/src/components/ui/form-fields.tsx:129">
P2: After this change, removing/reordering player rows can show the wrong selected filename beside a different player. The `FileField` display is now backed by internal `fileName` state, but this field is used inside an index-keyed list (`CreateSession`), so React instance reuse can carry stale filename UI across rows. Consider making the displayed filename controlled/resettable from parent data (or using stable row keys) so the visible file name stays aligned with the correct player.</violation>
</file>

<file name="apps/api/src/lib/jwt.ts">

<violation number="1" location="apps/api/src/lib/jwt.ts:51">
P2: Token expiration check is off by one second: tokens are still accepted when `exp === now` because verification only rejects `exp < now`. Tightening this to `exp <= now` avoids accepting already-expired JWTs at the boundary.</violation>
</file>

<file name="apps/web/src/main.tsx">

<violation number="1" location="apps/web/src/main.tsx:18">
P2: App startup can fail in storage-restricted environments because the persister reads `window.localStorage` eagerly at module scope. If that access throws, the SPA crashes before rendering. Wrapping storage acquisition in a safe try/catch fallback would keep the app usable even when persistent storage is unavailable.</violation>
</file>

<file name="apps/web/src/lib/query.ts">

<violation number="1" location="apps/web/src/lib/query.ts:18">
P1: Venue-scoped session data is now persisted under shared cache keys, so switching API key/venue can show another venue’s cached sessions/history until a network refetch succeeds. The new persistence filter stores `['sessions'...]` and `['session', id]` queries without any owner dimension, while auth switching only invalidates queries and does not clear the persisted store. Consider scoping persisted query identity by owner (or clearing persisted cache on venue switch) to avoid cross-tenant data exposure.</violation>
</file>

<file name="apps/api/src/middleware.ts">

<violation number="1" location="apps/api/src/middleware.ts:29">
P2: A transient Durable Object failure can currently turn normal requests into 500s because rate-limit checks parse the DO response without any error handling. Since this middleware runs on most routes, an intermittent DO issue can degrade API availability. Consider wrapping the DO fetch/JSON parse in a `try/catch` and falling back to the in-memory bucket (or another explicit fail-open/closed policy) when the DO path is unavailable.</violation>
</file>

<file name="apps/web/src/lib/auth-store.ts">

<violation number="1" location="apps/web/src/lib/auth-store.ts:48">
P2: After JWT expiry, the web client can get stuck sending only an expired bearer token, which causes API calls to fail until session reset. This happens because auth headers stop including `x-oche-owner` once `token` is set, and there is no automatic refresh path wired in. Including the API key header alongside bearer (or adding automatic refresh-on-401) would avoid this failure mode.</violation>
</file>

You're on the cubic free plan with 16 free PR reviews remaining this month. Upgrade for unlimited reviews.

Re-trigger cubic

scoreEvents: [],
};

const patchScores = vi.fn().mockResolvedValue({ ok: true });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0: The test will crash at module evaluation time. vi.mock is hoisted and its factory runs before const patchScores is initialized, so the factory's reference to patchScores throws a ReferenceError: Cannot access 'patchScores' before initialization. Define patchScores with vi.hoisted() so it's available when the mock factory executes.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/routes/Overview.test.tsx, line 28:

<comment>The test will crash at module evaluation time. `vi.mock` is hoisted and its factory runs before `const patchScores` is initialized, so the factory's reference to `patchScores` throws a `ReferenceError: Cannot access 'patchScores' before initialization`. Define `patchScores` with `vi.hoisted()` so it's available when the mock factory executes.</comment>

<file context>
@@ -0,0 +1,74 @@
+  scoreEvents: [],
+};
+
+const patchScores = vi.fn().mockResolvedValue({ ok: true });
+
+vi.mock('@/hooks/useLiveSession', () => ({
</file context>
Suggested change
const patchScores = vi.fn().mockResolvedValue({ ok: true });
const patchScores = vi.hoisted(() => vi.fn().mockResolvedValue({ ok: true }));

Comment thread apps/web/vite.config.ts
navigateFallbackDenylist: [/^\/docs\//],
runtimeCaching: [
{
urlPattern: ({ url }) => url.origin === apiOrigin() && /^\/sessions(\/|$|\?)/.test(url.pathname),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0: The urlPattern callback in runtimeCaching calls apiOrigin(), but that function won't exist in the generated service worker's scope. Workbox-build serializes function-valued urlPattern entries via .toString(), which strips all closure and module-scope references — the resulting SW will throw a ReferenceError on every /sessions/* fetch, breaking the offline/network-first caching entirely. Compute the origin value at config-load time and use a RegExp pattern instead.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/vite.config.ts, line 60:

<comment>The `urlPattern` callback in `runtimeCaching` calls `apiOrigin()`, but that function won't exist in the generated service worker's scope. Workbox-build serializes function-valued `urlPattern` entries via `.toString()`, which strips all closure and module-scope references — the resulting SW will throw a `ReferenceError` on every `/sessions/*` fetch, breaking the offline/network-first caching entirely. Compute the origin value at config-load time and use a RegExp pattern instead.</comment>

<file context>
@@ -21,7 +31,45 @@ function injectPreconnect(): PluginOption {
+        navigateFallbackDenylist: [/^\/docs\//],
+        runtimeCaching: [
+          {
+            urlPattern: ({ url }) => url.origin === apiOrigin() && /^\/sessions(\/|$|\?)/.test(url.pathname),
+            handler: 'NetworkFirst',
+            options: {
</file context>

return db.transaction(async (tx) => {
await tx.execute(sql`set local role oche_app`);
await tx.execute(sql`select set_config('app.api_key_hash', ${keyHash}, true)`);
const [row] = await tx.select({ ownerId: apiKeys.ownerId }).from(apiKeys).limit(1);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: API key exchange can resolve to an owner without proving the provided key when app.current_owner is already present in the transaction context. The lookup query in resolveApiKeyOwner uses limit(1) without filtering by keyHash, so it can return any row visible via the owner branch of the new api_keys_select policy. Adding an explicit WHERE key_hash = ... AND revoked_at IS NULL in the query keeps resolution tied to the submitted key regardless of surrounding session state.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/src/api-keys.ts, line 20:

<comment>API key exchange can resolve to an owner without proving the provided key when `app.current_owner` is already present in the transaction context. The lookup query in `resolveApiKeyOwner` uses `limit(1)` without filtering by `keyHash`, so it can return any row visible via the owner branch of the new `api_keys_select` policy. Adding an explicit `WHERE key_hash = ... AND revoked_at IS NULL` in the query keeps resolution tied to the submitted key regardless of surrounding session state.</comment>

<file context>
@@ -0,0 +1,29 @@
+  return db.transaction(async (tx) => {
+    await tx.execute(sql`set local role oche_app`);
+    await tx.execute(sql`select set_config('app.api_key_hash', ${keyHash}, true)`);
+    const [row] = await tx.select({ ownerId: apiKeys.ownerId }).from(apiKeys).limit(1);
+    return row?.ownerId ?? null;
+  });
</file context>

},
},
'/auth/keys': {
get: { tags: ['auth'], summary: 'List API keys for current owner' },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Both /auth/keys operations are missing the required responses field. The OpenAPI 3.1 spec mandates every operation include a responses object with at least one response — a spec-compliant code generator or validator will reject these operations as invalid. Add response definitions covering the success and error cases (e.g., 200/201 + 401).

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/openapi/spec.ts, line 170:

<comment>Both `/auth/keys` operations are missing the required `responses` field. The OpenAPI 3.1 spec mandates every operation include a `responses` object with at least one response — a spec-compliant code generator or validator will reject these operations as invalid. Add response definitions covering the success and error cases (e.g., 200/201 + 401).</comment>

<file context>
@@ -152,16 +153,40 @@ export const openApiDocument = {
+      },
+    },
+    '/auth/keys': {
+      get: { tags: ['auth'], summary: 'List API keys for current owner' },
+      post: { tags: ['auth'], summary: 'Create API key (plaintext returned once)' },
+    },
</file context>

const result = spawnSync('npx', ['tsx', '-e', inline], {
cwd: repoRoot,
encoding: 'utf8',
shell: true,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: The shell: true option breaks the inline TypeScript code because it contains shell metacharacters (;, single quotes, newlines). The shell interprets ; as a command separator and single quotes as shell quoting, so tsx -e only sees part of the code. The script will fail on any Unix shell. Remove shell: true — the command doesn't need shell features and works fine with direct argument passing.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/export-openapi.mjs, line 22:

<comment>The `shell: true` option breaks the inline TypeScript code because it contains shell metacharacters (`;`, single quotes, newlines). The shell interprets `;` as a command separator and single quotes as shell quoting, so `tsx -e` only sees part of the code. The script will fail on any Unix shell. Remove `shell: true` — the command doesn't need shell features and works fine with direct argument passing.</comment>

<file context>
@@ -0,0 +1,32 @@
+const result = spawnSync('npx', ['tsx', '-e', inline], {
+  cwd: repoRoot,
+  encoding: 'utf8',
+  shell: true,
+});
+
</file context>

Comment on lines +29 to +35
const res = await stub.fetch('https://rate-limiter/check', {
method: 'POST',
headers: { 'content-type': 'application/json' },
body: JSON.stringify({ limit, windowMs }),
});
const body = (await res.json()) as { allowed: boolean; remaining?: number };
return { allowed: body.allowed, remaining: body.remaining };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: A transient Durable Object failure can currently turn normal requests into 500s because rate-limit checks parse the DO response without any error handling. Since this middleware runs on most routes, an intermittent DO issue can degrade API availability. Consider wrapping the DO fetch/JSON parse in a try/catch and falling back to the in-memory bucket (or another explicit fail-open/closed policy) when the DO path is unavailable.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/middleware.ts, line 29:

<comment>A transient Durable Object failure can currently turn normal requests into 500s because rate-limit checks parse the DO response without any error handling. Since this middleware runs on most routes, an intermittent DO issue can degrade API availability. Consider wrapping the DO fetch/JSON parse in a `try/catch` and falling back to the in-memory bucket (or another explicit fail-open/closed policy) when the DO path is unavailable.</comment>

<file context>
@@ -1,4 +1,54 @@
+  if (!ns) return checkMemoryRateLimit(ip, limit, windowMs);
+
+  const stub = ns.get(ns.idFromName(ip));
+  const res = await stub.fetch('https://rate-limiter/check', {
+    method: 'POST',
+    headers: { 'content-type': 'application/json' },
</file context>
Suggested change
const res = await stub.fetch('https://rate-limiter/check', {
method: 'POST',
headers: { 'content-type': 'application/json' },
body: JSON.stringify({ limit, windowMs }),
});
const body = (await res.json()) as { allowed: boolean; remaining?: number };
return { allowed: body.allowed, remaining: body.remaining };
try {
const res = await stub.fetch('https://rate-limiter/check', {
method: 'POST',
headers: { 'content-type': 'application/json' },
body: JSON.stringify({ limit, windowMs }),
});
if (!res.ok) return checkMemoryRateLimit(ip, limit, windowMs);
const body = (await res.json()) as { allowed: boolean; remaining?: number };
return { allowed: body.allowed, remaining: body.remaining };
} catch {
return checkMemoryRateLimit(ip, limit, windowMs);
}

}

export function getAuthHeaders(): Record<string, string> {
if (token) return { Authorization: `Bearer ${token}` };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: After JWT expiry, the web client can get stuck sending only an expired bearer token, which causes API calls to fail until session reset. This happens because auth headers stop including x-oche-owner once token is set, and there is no automatic refresh path wired in. Including the API key header alongside bearer (or adding automatic refresh-on-401) would avoid this failure mode.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/lib/auth-store.ts, line 48:

<comment>After JWT expiry, the web client can get stuck sending only an expired bearer token, which causes API calls to fail until session reset. This happens because auth headers stop including `x-oche-owner` once `token` is set, and there is no automatic refresh path wired in. Including the API key header alongside bearer (or adding automatic refresh-on-401) would avoid this failure mode.</comment>

<file context>
@@ -0,0 +1,55 @@
+}
+
+export function getAuthHeaders(): Record<string, string> {
+  if (token) return { Authorization: `Bearer ${token}` };
+  return { 'x-oche-owner': apiKey };
+}
</file context>

Comment thread package.json
"db:rls:check": "node scripts/rls-check.mjs",
"db:rls:check:staging": "node scripts/with-env.mjs DATABASE_URL_STAGING node scripts/rls-check.mjs",
"deploy:staging": "node scripts/run-steps.mjs db:migrate:staging deploy:api:staging build:web:staging deploy:web:staging",
"deploy:staging:full": "node scripts/run-steps.mjs db:migrate:staging db:force-rls:staging db:rls:check:staging db:seed:staging deploy:staging",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: deploy:staging:full runs db:migrate:staging twice — once explicitly as the first step and then again as part of deploy:staging. Since migrations are idempotent this won't break anything, but it adds unnecessary time to the deploy pipeline and makes the dependency between steps harder to reason about. Recommend removing the explicit db:migrate:staging call from deploy:staging:full since deploy:staging already includes it.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At package.json, line 66:

<comment>`deploy:staging:full` runs `db:migrate:staging` twice — once explicitly as the first step and then again as part of `deploy:staging`. Since migrations are idempotent this won't break anything, but it adds unnecessary time to the deploy pipeline and makes the dependency between steps harder to reason about. Recommend removing the explicit `db:migrate:staging` call from `deploy:staging:full` since `deploy:staging` already includes it.</comment>

<file context>
@@ -60,7 +61,9 @@
     "db:rls:check": "node scripts/rls-check.mjs",
+    "db:rls:check:staging": "node scripts/with-env.mjs DATABASE_URL_STAGING node scripts/rls-check.mjs",
     "deploy:staging": "node scripts/run-steps.mjs db:migrate:staging deploy:api:staging build:web:staging deploy:web:staging",
+    "deploy:staging:full": "node scripts/run-steps.mjs db:migrate:staging db:force-rls:staging db:rls:check:staging db:seed:staging deploy:staging",
     "deploy:prod": "node scripts/run-steps.mjs db:migrate:prod deploy:api:prod build:web:prod deploy:web:prod",
     "deploy:all": "node scripts/run-steps.mjs deploy:staging deploy:prod",
</file context>

Comment thread docs/AUTH.md
1. **API key** — `x-oche-owner` header or `?key=` (WebSocket). Sources:
- Built-in demo keys (`demo-key-a`, `demo-key-b`) → seed owners A/B
- `OCHE_API_KEYS` JSON in Worker env (local: `.oche-keys.json`)
- Hashed rows in `api_keys` table (created via `/auth/keys` UI)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: Inconsistent path for the API key management UI. The doc first says /auth/keys (line 10) but the SPA flow section (line 22) and the cursor rule (34-auth.mdc) both say /settings/keys. Align the first reference to match the actual route.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/AUTH.md, line 10:

<comment>Inconsistent path for the API key management UI. The doc first says `/auth/keys` (line 10) but the SPA flow section (line 22) and the cursor rule (`34-auth.mdc`) both say `/settings/keys`. Align the first reference to match the actual route.</comment>

<file context>
@@ -0,0 +1,43 @@
+1. **API key** — `x-oche-owner` header or `?key=` (WebSocket). Sources:
+   - Built-in demo keys (`demo-key-a`, `demo-key-b`) → seed owners A/B
+   - `OCHE_API_KEYS` JSON in Worker env (local: `.oche-keys.json`)
+   - Hashed rows in `api_keys` table (created via `/auth/keys` UI)
+2. **JWT** — `Authorization: Bearer <token>` or `?token=` (WebSocket). Minted by `POST /auth/token` using `OCHE_JWT_SECRET` (HS256, 1h TTL).
+3. **RLS** — Worker still calls `withPrincipal()` → `SET LOCAL app.current_owner`. Policies unchanged; JWT/API key only affects which owner id is set.
</file context>

);
}

if (!panelId) return <SessionOverviewSkeleton />;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: This extra guard is unreachable and adds dead branching to the render path. panelId is only null when activeSessions.length === 0, but that case already returns QueryEmpty above. Removing this branch keeps the flow simpler and avoids future confusion about whether panelId can be null here.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/routes/Overview.tsx, line 61:

<comment>This extra guard is unreachable and adds dead branching to the render path. `panelId` is only `null` when `activeSessions.length === 0`, but that case already returns `QueryEmpty` above. Removing this branch keeps the flow simpler and avoids future confusion about whether `panelId` can be null here.</comment>

<file context>
@@ -62,13 +58,15 @@ export function Overview() {
     );
   }
 
+  if (!panelId) return <SessionOverviewSkeleton />;
+
   return (
</file context>

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.

1 participant