Add Cloudflare Durable Object subrouter#50
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
12 issues found across 19 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="cloudflare/packages/worker/src/contract.ts">
<violation number="1" location="cloudflare/packages/worker/src/contract.ts:247">
P3: `findJsonString` recursively walks every nested value of the request body with no depth cap. For JSON payloads with extreme nesting this could cause a stack overflow. Consider adding a max depth parameter (e.g. 10) to bound recursion.</violation>
<violation number="2" location="cloudflare/packages/worker/src/contract.ts:318">
P2: `accountStatus` assigns both `auth_checked` and `auth_valid` to `account.hasCredentials`, which conflates "credentials exist" with "credentials are valid". This can mislead admin/status consumers into believing the account's auth was actively verified when only the presence of stored credential fields was checked. Consider renaming the field to `has_credentials` or performing an actual validation check before reporting `auth_valid: true`.</violation>
</file>
<file name="cloudflare/packages/core/package.json">
<violation number="1" location="cloudflare/packages/core/package.json:3">
P2: Missing `"private": true` on this internal workspace package. The root workspace is already marked private, but this package lacks its own guard. If someone runs `npm publish` or `bun publish` from inside the package directory, it could accidentally publish to the npm registry. Add `"private": true` to prevent accidental publication.</violation>
</file>
<file name="cloudflare/packages/core/src/index.ts">
<violation number="1" location="cloudflare/packages/core/src/index.ts:54">
P3: `quotaKeyForModel` uses `String.includes()` to match pool keywords, so model names containing "spark", "opus", or "sonnet" as substrings would silently route to the wrong quota pool. Consider word-boundary matching or an explicit allowlist if model names evolve beyond current conventions.</violation>
<violation number="2" location="cloudflare/packages/core/src/index.ts:117">
P2: The round-robin cursor is shared across all orgs and quota keys, so distribution for one tenant or quota pool is skewed by requests from other tenants/pools. In a multi-tenant reference implementation this can produce confusing test behavior. Consider maintaining a Map<(orgId, resolvedQuotaKey) => number> for per-org-per-pool cursors.</violation>
<violation number="3" location="cloudflare/packages/core/src/index.ts:131">
P2: Pinned-account requests can be ignored for existing sticky sessions. Because sticky lookup runs before `preferAccountId`, a request that tries to force a specific account may still route to the old sticky account. Reordering this flow so explicit pins are evaluated before sticky lookup would align behavior with the documented `X-Subrouter-Account-ID` force semantics.</violation>
</file>
<file name="cloudflare/packages/worker/test/worker-transcripts.test.ts">
<violation number="1" location="cloudflare/packages/worker/test/worker-transcripts.test.ts:386">
P2: wrangler dev stdout/stderr pipes are created but never consumed. If wrangler writes enough output (e.g., startup logs, warnings) to fill the pipe buffer (~64KB on Linux), the child process will block on write and the test will hang. With `--log-level error` this is unlikely but not guaranteed over a 60s test window. Either consume the streams (pipe to parent process or drain to /dev/null) or use `"inherit"` / `"ignore"` since the output is not asserted.</violation>
</file>
<file name="cloudflare/packages/worker/src/index.ts">
<violation number="1">
P3: `listTranscriptSummaries` and `transcriptDashboardData` duplicate the full transcript-summary-building loop — iterating rows, constructing compound keys with `baseSessionId`, building `MutableTranscriptSummary` via `applyTranscriptEvent`, and sorting with the same comparator. Extraction into a shared `buildTranscriptSummaries(sql, orgId)` helper would eliminate the duplication and keep the two endpoints consistent.</violation>
<violation number="2">
P2: Unmatched `/admin/*` sub-routes fall through to the proxy handler instead of returning 404. For example, a `GET` to `/admin/accounts/foo/totp` passes admin auth, matches no sub-handler (TOTP only handles POST), then bypasses the `/_subrouter/` catch-all and reaches the proxy handler. If `PROXY_TOKEN` is unset the proxy auth reuses `ADMIN_TOKEN`, so the request gets proxied upstream with an unrecognized path. This is a routing gap: admin sub-routes that don't match a defined method/path should produce 404, not fall through to the proxy pipeline.</violation>
<violation number="3">
P1: `parseRouteInput` silently falls back to demo defaults when the JSON body is malformed, empty, or an array. A POST to `/route` with a malformed body or `application/json` but `[]` gets routed to `demo-org` / `default` session with zero error feedback to the caller. The caller gets a successful HTTP 200 route response when the API contract was violated — this silently breaks the routing contract and makes integration bugs very hard to debug. Recommend returning `json({ error: \"Expected a JSON object body\" }, { status: 400 })` when the body is missing, invalid, or an array.</violation>
<violation number="4">
P2: Token auth currently treats the `Bearer` scheme as case-sensitive, so requests using a lowercase (but valid) scheme like `authorization: bearer ...` are rejected as unauthorized. Using case-insensitive parsing for the scheme would make auth handling standards-compliant and avoid avoidable client compatibility failures.</violation>
<violation number="5">
P1: WebSocket clients can currently call routing and usage operations without presenting `PROXY_TOKEN`/`ADMIN_TOKEN`, because the `/ws` branch forwards upgrades directly to the Durable Object before any auth gate. That exposes state-changing subrouter actions to unauthenticated callers. It would be safer to apply the same proxy-token check used for `/v1/*` proxy traffic before accepting the websocket.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| @@ -0,0 +1,3022 @@ | |||
| import { DurableObject } from "cloudflare:workers" | |||
There was a problem hiding this comment.
P1: parseRouteInput silently falls back to demo defaults when the JSON body is malformed, empty, or an array. A POST to /route with a malformed body or application/json but [] gets routed to demo-org / default session with zero error feedback to the caller. The caller gets a successful HTTP 200 route response when the API contract was violated — this silently breaks the routing contract and makes integration bugs very hard to debug. Recommend returning json({ error: \"Expected a JSON object body\" }, { status: 400 }) when the body is missing, invalid, or an array.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cloudflare/packages/worker/src/index.ts:
<comment>`parseRouteInput` silently falls back to demo defaults when the JSON body is malformed, empty, or an array. A POST to `/route` with a malformed body or `application/json` but `[]` gets routed to `demo-org` / `default` session with zero error feedback to the caller. The caller gets a successful HTTP 200 route response when the API contract was violated — this silently breaks the routing contract and makes integration bugs very hard to debug. Recommend returning `json({ error: \"Expected a JSON object body\" }, { status: 400 })` when the body is missing, invalid, or an array.</comment>
| @@ -0,0 +1,3022 @@ | |||
| import { DurableObject } from "cloudflare:workers" | |||
There was a problem hiding this comment.
P1: WebSocket clients can currently call routing and usage operations without presenting PROXY_TOKEN/ADMIN_TOKEN, because the /ws branch forwards upgrades directly to the Durable Object before any auth gate. That exposes state-changing subrouter actions to unauthenticated callers. It would be safer to apply the same proxy-token check used for /v1/* proxy traffic before accepting the websocket.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cloudflare/packages/worker/src/index.ts:
<comment>WebSocket clients can currently call routing and usage operations without presenting `PROXY_TOKEN`/`ADMIN_TOKEN`, because the `/ws` branch forwards upgrades directly to the Durable Object before any auth gate. That exposes state-changing subrouter actions to unauthenticated callers. It would be safer to apply the same proxy-token check used for `/v1/*` proxy traffic before accepting the websocket.</comment>
| export const accountStatus = (account: StoredAccountContract) => ({ | ||
| ...safeGoAccount(account), | ||
| auth_checked: account.hasCredentials, | ||
| auth_valid: account.hasCredentials, |
There was a problem hiding this comment.
P2: accountStatus assigns both auth_checked and auth_valid to account.hasCredentials, which conflates "credentials exist" with "credentials are valid". This can mislead admin/status consumers into believing the account's auth was actively verified when only the presence of stored credential fields was checked. Consider renaming the field to has_credentials or performing an actual validation check before reporting auth_valid: true.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cloudflare/packages/worker/src/contract.ts, line 318:
<comment>`accountStatus` assigns both `auth_checked` and `auth_valid` to `account.hasCredentials`, which conflates "credentials exist" with "credentials are valid". This can mislead admin/status consumers into believing the account's auth was actively verified when only the presence of stored credential fields was checked. Consider renaming the field to `has_credentials` or performing an actual validation check before reporting `auth_valid: true`.</comment>
<file context>
@@ -0,0 +1,792 @@
+export const accountStatus = (account: StoredAccountContract) => ({
+ ...safeGoAccount(account),
+ auth_checked: account.hasCredentials,
+ auth_valid: account.hasCredentials,
+})
+
</file context>
| @@ -0,0 +1,16 @@ | |||
| { | |||
| "name": "@subrouter/core", | |||
| "version": "0.0.0", | |||
There was a problem hiding this comment.
P2: Missing "private": true on this internal workspace package. The root workspace is already marked private, but this package lacks its own guard. If someone runs npm publish or bun publish from inside the package directory, it could accidentally publish to the npm registry. Add "private": true to prevent accidental publication.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cloudflare/packages/core/package.json, line 3:
<comment>Missing `"private": true` on this internal workspace package. The root workspace is already marked private, but this package lacks its own guard. If someone runs `npm publish` or `bun publish` from inside the package directory, it could accidentally publish to the npm registry. Add `"private": true` to prevent accidental publication.</comment>
<file context>
@@ -0,0 +1,16 @@
+{
+ "name": "@subrouter/core",
+ "version": "0.0.0",
+ "type": "module",
+ "main": "./src/index.ts",
</file context>
| export const makeInMemoryAccountStoreLayer = (initial: ReadonlyArray<Account>) => { | ||
| const accounts = [...initial] | ||
| const sticky = makeInMemoryStickyStore() | ||
| let cursor = 0 |
There was a problem hiding this comment.
P2: The round-robin cursor is shared across all orgs and quota keys, so distribution for one tenant or quota pool is skewed by requests from other tenants/pools. In a multi-tenant reference implementation this can produce confusing test behavior. Consider maintaining a Map<(orgId, resolvedQuotaKey) => number> for per-org-per-pool cursors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cloudflare/packages/core/src/index.ts, line 117:
<comment>The round-robin cursor is shared across all orgs and quota keys, so distribution for one tenant or quota pool is skewed by requests from other tenants/pools. In a multi-tenant reference implementation this can produce confusing test behavior. Consider maintaining a Map<(orgId, resolvedQuotaKey) => number> for per-org-per-pool cursors.</comment>
<file context>
@@ -0,0 +1,163 @@
+export const makeInMemoryAccountStoreLayer = (initial: ReadonlyArray<Account>) => {
+ const accounts = [...initial]
+ const sticky = makeInMemoryStickyStore()
+ let cursor = 0
+
+ return Layer.succeed(AccountStoreTag, {
</file context>
| @@ -0,0 +1,3022 @@ | |||
| import { DurableObject } from "cloudflare:workers" | |||
There was a problem hiding this comment.
P2: Token auth currently treats the Bearer scheme as case-sensitive, so requests using a lowercase (but valid) scheme like authorization: bearer ... are rejected as unauthorized. Using case-insensitive parsing for the scheme would make auth handling standards-compliant and avoid avoidable client compatibility failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cloudflare/packages/worker/src/index.ts:
<comment>Token auth currently treats the `Bearer` scheme as case-sensitive, so requests using a lowercase (but valid) scheme like `authorization: bearer ...` are rejected as unauthorized. Using case-insensitive parsing for the scheme would make auth handling standards-compliant and avoid avoidable client compatibility failures.</comment>
| accountHasQuotaForModel(account, resolvedQuotaKey) | ||
|
|
||
| // 1. sticky table first | ||
| const stickyId = sticky.get(orgId, sessionId, resolvedQuotaKey) |
There was a problem hiding this comment.
P2: Pinned-account requests can be ignored for existing sticky sessions. Because sticky lookup runs before preferAccountId, a request that tries to force a specific account may still route to the old sticky account. Reordering this flow so explicit pins are evaluated before sticky lookup would align behavior with the documented X-Subrouter-Account-ID force semantics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cloudflare/packages/core/src/index.ts, line 131:
<comment>Pinned-account requests can be ignored for existing sticky sessions. Because sticky lookup runs before `preferAccountId`, a request that tries to force a specific account may still route to the old sticky account. Reordering this flow so explicit pins are evaluated before sticky lookup would align behavior with the documented `X-Subrouter-Account-ID` force semantics.</comment>
<file context>
@@ -0,0 +1,163 @@
+ accountHasQuotaForModel(account, resolvedQuotaKey)
+
+ // 1. sticky table first
+ const stickyId = sticky.get(orgId, sessionId, resolvedQuotaKey)
+ if (stickyId) {
+ const found = accounts.find((a) => a.id === stickyId && isEligible(a))
</file context>
| @@ -0,0 +1,792 @@ | |||
| import type { Account } from "@subrouter/core" | |||
There was a problem hiding this comment.
P3: findJsonString recursively walks every nested value of the request body with no depth cap. For JSON payloads with extreme nesting this could cause a stack overflow. Consider adding a max depth parameter (e.g. 10) to bound recursion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cloudflare/packages/worker/src/contract.ts, line 247:
<comment>`findJsonString` recursively walks every nested value of the request body with no depth cap. For JSON payloads with extreme nesting this could cause a stack overflow. Consider adding a max depth parameter (e.g. 10) to bound recursion.</comment>
<file context>
@@ -0,0 +1,792 @@
+ }
+}
+
+const findJsonString = (
+ value: unknown,
+ keys: ReadonlyArray<string>
</file context>
| const normalized = model?.trim().toLowerCase() | ||
| if (!normalized) return "default" | ||
| for (const pool of MODEL_QUOTA_POOLS) { | ||
| if (normalized.includes(pool)) return pool |
There was a problem hiding this comment.
P3: quotaKeyForModel uses String.includes() to match pool keywords, so model names containing "spark", "opus", or "sonnet" as substrings would silently route to the wrong quota pool. Consider word-boundary matching or an explicit allowlist if model names evolve beyond current conventions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cloudflare/packages/core/src/index.ts, line 54:
<comment>`quotaKeyForModel` uses `String.includes()` to match pool keywords, so model names containing "spark", "opus", or "sonnet" as substrings would silently route to the wrong quota pool. Consider word-boundary matching or an explicit allowlist if model names evolve beyond current conventions.</comment>
<file context>
@@ -0,0 +1,163 @@
+ const normalized = model?.trim().toLowerCase()
+ if (!normalized) return "default"
+ for (const pool of MODEL_QUOTA_POOLS) {
+ if (normalized.includes(pool)) return pool
+ }
+ return "default"
</file context>
| @@ -0,0 +1,3022 @@ | |||
| import { DurableObject } from "cloudflare:workers" | |||
There was a problem hiding this comment.
P3: listTranscriptSummaries and transcriptDashboardData duplicate the full transcript-summary-building loop — iterating rows, constructing compound keys with baseSessionId, building MutableTranscriptSummary via applyTranscriptEvent, and sorting with the same comparator. Extraction into a shared buildTranscriptSummaries(sql, orgId) helper would eliminate the duplication and keep the two endpoints consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cloudflare/packages/worker/src/index.ts:
<comment>`listTranscriptSummaries` and `transcriptDashboardData` duplicate the full transcript-summary-building loop — iterating rows, constructing compound keys with `baseSessionId`, building `MutableTranscriptSummary` via `applyTranscriptEvent`, and sorting with the same comparator. Extraction into a shared `buildTranscriptSummaries(sql, orgId)` helper would eliminate the duplication and keep the two endpoints consistent.</comment>
1 participant
Summary
cloudflare/Bun workspace with an Effect-based subrouter core and Cloudflare Durable Object workerDeploy evidence
regatta-subrouter-do-stagingversiond7812871-226d-46e2-a55f-566b2fac2bdfregatta-subrouter-do-productionversion983335bb-bfe0-4a7b-96c4-123313d82e6chttps://subrouter.cmux.dev/healthzreturns{"ok":true,"service":"subrouter-do"}Verification
cd cloudflare && bun install --frozen-lockfilecd cloudflare && bun run typecheckcd cloudflare && bun run testcd cloudflare/packages/worker && bunx wrangler deploy --env staging --dry-run --outdir dist-dry-rungo build ./...go vet ./...go test ./... -count=1actionlint .github/workflows/cloudflare-do.ymlNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds a Cloudflare Durable Object-based subrouter with sticky session routing and Go-compatible admin/proxy endpoints. Includes a new
cloudflare/workspace and CI/CD for verify, staging onmain, and manual production deploys.New Features
/_subrouter/*admin, session, transcript, and proxy endpoints; proxies Codex/OpenAI/Claude.cloudflare/workspace with@subrouter/core(Effect-based routing) and@subrouter/cloudflare-worker; CI/CD for verify + staged/prod deploys.Migration
ADMIN_TOKENandPROXY_TOKEN.@subrouter/cloudflare-workerscripts (deploy:staging/deploy:production)./healthz.Written for commit 324c515. Summary will update on new commits.