Repository navigation
Migrate legacy tenants to hosted Subrouter safely - #138
Conversation
📝 WalkthroughWalkthroughHosted tenant migration now validates source and destination bindings, stages supported accounts, persists recoverable migration state, controls source mutations, and supports activation, rollback, restoration, and finalization. Account IDs and labels remain available across hosted storage and listing paths. ChangesHosted tenant migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AdminClient
participant AdminMigrationRoute
participant SubrouterDurableObject
participant HostedDestination
AdminClient->>AdminMigrationRoute: POST /admin/tenants/:tenantId/migrate-hosted
AdminMigrationRoute->>SubrouterDurableObject: Prepare and quiesce source
SubrouterDurableObject-->>AdminMigrationRoute: Migration snapshot
AdminMigrationRoute->>HostedDestination: Stage and activate accounts
HostedDestination-->>AdminMigrationRoute: Destination status
AdminMigrationRoute->>SubrouterDurableObject: Finalize or restore source
SubrouterDurableObject-->>AdminMigrationRoute: Migration count or recovery result
AdminMigrationRoute-->>AdminClient: Structured migration response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/proxy/multitenant.go (1)
744-753: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDelegate credential mutation to the upstream account manager.
The tenant upload route persists OAuth tokens and API keys directly. This violates the credential-handling rule. Keep this route read-only unless it explicitly delegates the mutation to the upstream account manager.
internal/proxy/multitenant.go#L744-L753: replace direct Codex token persistence with an explicit upstream account-manager delegation.internal/proxy/multitenant.go#L775-L778: replace direct API-key persistence with an explicit upstream account-manager delegation.internal/proxy/multitenant.go#L792-L792: replace direct Claude credential persistence with an explicit upstream account-manager delegation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/proxy/multitenant.go` around lines 744 - 753, Make the tenant upload route read-only by replacing direct credential persistence in internal/proxy/multitenant.go at lines 744-753, 775-778, and 792 with explicit delegation to the upstream account manager; update the Codex token, API-key, and Claude credential handling in the relevant upload flow without changing unrelated account storage behavior.Source: Coding guidelines
🧹 Nitpick comments (7)
cloudflare/packages/worker/src/index.ts (3)
5014-5016: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the new route to the telemetry route classifier.
routePatternForRequestincloudflare/packages/worker/src/telemetry.ts(Lines 96-132) matches/admin/tenants/:id/revokeand/admin/tenants/:id/rotate, but it has no branch formigrate-hosted. The new route therefore reports an unclassified pattern. Line 5067 setstelemetry.errorTypefor a failed migration, and that signal is hard to use without a route label.Add a branch that returns
/admin/tenants/:id/migrate-hosted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudflare/packages/worker/src/index.ts` around lines 5014 - 5016, Update routePatternForRequest in telemetry.ts to recognize the /admin/tenants/:id/migrate-hosted path and return /admin/tenants/:id/migrate-hosted, alongside the existing tenant revoke and rotate route classifications.
490-498: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn distinct error text for malformed JSON.
Both the parse failure at Line 494 and the non-object body at Line 497 return
"Missing JSON body". The body is present in both cases. The message misleads the caller.♻️ Proposed change
let decoded: unknown try { decoded = JSON.parse(new TextDecoder().decode(body)) } catch { - return json({ error: "Missing JSON body" }, { status: 400 }) + return json({ error: "Invalid JSON body" }, { status: 400 }) } if (!decoded || typeof decoded !== "object" || Array.isArray(decoded)) { - return json({ error: "Missing JSON body" }, { status: 400 }) + return json({ error: "JSON body must be an object" }, { status: 400 }) }Confirm that no existing test asserts the
"Missing JSON body"text for these two cases before you change it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudflare/packages/worker/src/index.ts` around lines 490 - 498, Update the JSON parsing validation around decoded so malformed JSON caught by JSON.parse returns a distinct error message from the non-object body case, while preserving the existing 400 status. Before changing the text, verify that no existing tests assert "Missing JSON body" for either scenario.
2611-2622: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPrefer a generation check over JSON string equality for the integrity guard.
Line 2618 compares
row.credentials_jsonagainstJSON.stringify(account.credentials). That test depends on byte-identical serialization. It holds today only because every writer in this file serializes withJSON.stringifyand the snapshot round-trips throughJSON.parse. It breaks if any writer stores formatted JSON, if a key order changes, or if a numeric value is stored in a different textual form.The file already tracks
credentials.credentialGenerationfor exactly this purpose.updateAccountCredentialsIfGeneration(Line 3419) uses it to detect a concurrent credential change. Compare the generation andenabledinstead.♻️ Proposed change
for (const account of input.accounts) { const row = this.getAccountRow(sql, input.orgId, account.id, false) + const rowCredentials = row?.credentials_json + ? (JSON.parse(row.credentials_json) as AccountCredentials) + : undefined if ( !row || row.enabled !== 0 || - row.credentials_json !== JSON.stringify(account.credentials) + (rowCredentials?.credentialGeneration ?? 0) !== + (account.credentials?.credentialGeneration ?? 0) ) { throw new Error("migration source changed during hosted upload") } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudflare/packages/worker/src/index.ts` around lines 2611 - 2622, Update the integrity guard in the transaction within the hosted upload flow to compare the stored credential generation and enabled state against the corresponding values from account, instead of comparing credentials_json with JSON.stringify(account.credentials). Reuse the credentialGeneration field and preserve the existing failure behavior when the account is missing, disabled, or its generation differs.cloudflare/packages/worker/test/tenant-migration.test.ts (3)
208-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the remaining rejection branches.
The suite covers the untrusted-destination branch.
migrationUploadandmigrateLegacyAccountsToHostedincloudflare/packages/worker/src/legacy-migration.tsreject several other inputs, and none of them has a test:
- a disabled account (
!account.enabled, Line 161)- an account with a TOTP seed (
account.hasTotp, Line 161)anthropic_oauth(Line 196)- an invalid
tenantKey(Line 80)- more than
maxMigrationAccountsaccounts (Line 83)Each of these guards protects a credential path. Add a test per branch and assert that no secret appears in the error text.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudflare/packages/worker/test/tenant-migration.test.ts` around lines 208 - 250, Add tests in the tenant migration suite covering the remaining guards in migrationUpload and migrateLegacyAccountsToHosted: disabled accounts, accounts with hasTotp, anthropic_oauth accounts, invalid tenantKey values, and exceeding maxMigrationAccounts. For each case, exercise the corresponding admin migration request or helper path, assert the expected rejection, and verify the response or error text does not contain any uploaded credential secret.
183-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the exact-string body assertion and assert the
Cache-Controlheader instead.Lines 183-190 compare the raw response text against
JSON.stringify({ ok, migrated, sourceFinalized }). That assertion depends on the key order the route emits. Lines 191-195 already verify the same payload semantically. The route also setsCache-Control: no-store(cloudflare/packages/worker/src/index.ts, Line 5064), and no test covers that header.♻️ Proposed change
const responseText = await response.text() - expect({ status: response.status, body: responseText }).toEqual({ - status: 200, - body: JSON.stringify({ - ok: true, - migrated: 1, - sourceFinalized: true, - }), - }) + expect(response.status).toBe(200) + expect(response.headers.get("cache-control")).toBe("no-store") expect(JSON.parse(responseText)).toEqual({ ok: true, migrated: 1, sourceFinalized: true, }) expect(responseText).not.toContain(secret)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudflare/packages/worker/test/tenant-migration.test.ts` around lines 183 - 196, In the tenant migration test, remove the exact JSON-string body comparison from the first response assertion and retain the existing parsed-body assertion for semantic payload validation. Add an assertion that the response’s Cache-Control header is set to no-store, using the response object and existing header access conventions.
15-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAwait
server.stop()inafterEach.
Bun.serve().stop()returns a promise, so the loop should await it to avoid leaving servers listening while later tests start. Update theserverselement type to allowstop()returningvoid | Promise<void>.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudflare/packages/worker/test/tenant-migration.test.ts` around lines 15 - 18, Update the afterEach cleanup hook to await every server.stop() call, ensuring all servers finish shutting down before subsequent tests run. Adjust the servers collection element type so stop() is declared as returning void or Promise<void>, while preserving the existing splice-and-cleanup flow.cloudflare/packages/worker/src/legacy-migration.ts (1)
53-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAttach the original failure as
causewhen restoration also fails.When
restorethrows, the thrown error hides the upload failure reason. The operator then cannot tell why the migration failed. Preserve both causes.♻️ Proposed change
} catch (error) { if (options.finalizeSource) { try { await options.source.restore(accounts) - } catch { - throw new Error("hosted migration failed and source restoration failed") + } catch (restoreError) { + throw new Error( + "hosted migration failed and source restoration failed", + { cause: { migration: error, restore: restoreError } } + ) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudflare/packages/worker/src/legacy-migration.ts` around lines 53 - 62, Update the outer catch around the migration flow to retain the original upload failure when options.source.restore(accounts) also throws: throw the restoration error with the original caught error attached as its cause, while preserving the existing failure message and behavior for successful restoration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cloudflare/packages/worker/src/index.ts`:
- Around line 5035-5044: Update the migration flow around migrateLegacyTenant so
allowLoopback is derived from the worker’s trusted configuration rather than new
URL(request.url).hostname or any request-controlled header. Preserve loopback
support for the local worker fixture by defining and supplying the corresponding
configuration variable in the tenant migration test setup.
- Around line 5017-5020: Validate the decoded tenantId in the tenant migration
POST handler before parsing the request or calling adminActor. Use
normalizeTenantId, return HTTP 400 for an invalid value consistent with
/_subrouter/ready, and pass the normalized tenant ID to the Durable Object.
- Around line 2573-2586: Update the migration flow around the
blockConcurrencyWhile callback to await the current refreshInFlight promises
before entering the barrier, then re-check refreshInFlight inside the barrier
and reject the migration if any refresh started in the interim. Keep
snapshotting, disabling account rows, scheduling the alarm, and returning the
snapshot inside the barrier, but do not await network-backed refreshes there.
Verify that refreshOAuthCredentials uses a bounded timeout.
In `@cloudflare/packages/worker/src/legacy-migration.ts`:
- Around line 168-197: Reject unknown legacy account kinds before migration
uploads: add a default-throwing branch to migrationUpload’s switch in
cloudflare/packages/worker/src/legacy-migration.ts (lines 168-197), and replace
the unchecked row.kind as AccountKind cast with runtime validation against all
known AccountKind values in cloudflare/packages/worker/src/index.ts (lines
2557-2559), rejecting invalid rows.
- Around line 88-121: Ensure the migration started by begin() cannot leave
source accounts disabled when the serial uploads loop is interrupted or exceeds
the Worker lifetime. Add durable unfinished-migration tracking with an admin
recovery route or Durable Object alarm that invokes restore() for stale
migrations, and clear that state only after complete() succeeds; keep recovery
idempotent and bound upload work where needed.
---
Outside diff comments:
In `@internal/proxy/multitenant.go`:
- Around line 744-753: Make the tenant upload route read-only by replacing
direct credential persistence in internal/proxy/multitenant.go at lines 744-753,
775-778, and 792 with explicit delegation to the upstream account manager;
update the Codex token, API-key, and Claude credential handling in the relevant
upload flow without changing unrelated account storage behavior.
---
Nitpick comments:
In `@cloudflare/packages/worker/src/index.ts`:
- Around line 5014-5016: Update routePatternForRequest in telemetry.ts to
recognize the /admin/tenants/:id/migrate-hosted path and return
/admin/tenants/:id/migrate-hosted, alongside the existing tenant revoke and
rotate route classifications.
- Around line 490-498: Update the JSON parsing validation around decoded so
malformed JSON caught by JSON.parse returns a distinct error message from the
non-object body case, while preserving the existing 400 status. Before changing
the text, verify that no existing tests assert "Missing JSON body" for either
scenario.
- Around line 2611-2622: Update the integrity guard in the transaction within
the hosted upload flow to compare the stored credential generation and enabled
state against the corresponding values from account, instead of comparing
credentials_json with JSON.stringify(account.credentials). Reuse the
credentialGeneration field and preserve the existing failure behavior when the
account is missing, disabled, or its generation differs.
In `@cloudflare/packages/worker/src/legacy-migration.ts`:
- Around line 53-62: Update the outer catch around the migration flow to retain
the original upload failure when options.source.restore(accounts) also throws:
throw the restoration error with the original caught error attached as its
cause, while preserving the existing failure message and behavior for successful
restoration.
In `@cloudflare/packages/worker/test/tenant-migration.test.ts`:
- Around line 208-250: Add tests in the tenant migration suite covering the
remaining guards in migrationUpload and migrateLegacyAccountsToHosted: disabled
accounts, accounts with hasTotp, anthropic_oauth accounts, invalid tenantKey
values, and exceeding maxMigrationAccounts. For each case, exercise the
corresponding admin migration request or helper path, assert the expected
rejection, and verify the response or error text does not contain any uploaded
credential secret.
- Around line 183-196: In the tenant migration test, remove the exact
JSON-string body comparison from the first response assertion and retain the
existing parsed-body assertion for semantic payload validation. Add an assertion
that the response’s Cache-Control header is set to no-store, using the response
object and existing header access conventions.
- Around line 15-18: Update the afterEach cleanup hook to await every
server.stop() call, ensuring all servers finish shutting down before subsequent
tests run. Adjust the servers collection element type so stop() is declared as
returning void or Promise<void>, while preserving the existing
splice-and-cleanup flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 24ac4c91-bbf8-4841-9f31-738443c8995b
📒 Files selected for processing (9)
cloudflare/packages/worker/src/index.tscloudflare/packages/worker/src/legacy-migration.tscloudflare/packages/worker/test/tenant-migration.test.tsinternal/accounts/codex_store.gointernal/broker/client.gointernal/broker/hosted_client_test.gointernal/proxy/multitenant.gointernal/proxy/multitenant_test.gointernal/proxy/proxy.go
| if (tenantMigrationMatch && request.method === "POST") { | ||
| const tenantId = decodeURIComponent(tenantMigrationMatch[1]!) | ||
| const record = await parseBoundedJsonRecord(request, 4 * 1024) | ||
| if (record instanceof Response) return record |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate tenantId before you address the Durable Object.
Line 5018 passes the decoded path segment straight to adminActor at Line 5031. Other routes in this file normalize it first. /_subrouter/ready calls normalizeTenantId and returns 400 for an invalid value (Lines 4658-4661).
Without that check, a malformed tenant id addresses a new empty Durable Object. The migration then returns 200 with migrated: 0, and the operator believes an empty tenant was migrated.
🐛 Proposed fix
if (tenantMigrationMatch && request.method === "POST") {
- const tenantId = decodeURIComponent(tenantMigrationMatch[1]!)
+ const tenantId = normalizeTenantId(
+ decodeURIComponent(tenantMigrationMatch[1]!)
+ )
+ if (!tenantId) {
+ return json({ error: "Invalid tenant" }, { status: 400 })
+ }
const record = await parseBoundedJsonRecord(request, 4 * 1024)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (tenantMigrationMatch && request.method === "POST") { | |
| const tenantId = decodeURIComponent(tenantMigrationMatch[1]!) | |
| const record = await parseBoundedJsonRecord(request, 4 * 1024) | |
| if (record instanceof Response) return record | |
| if (tenantMigrationMatch && request.method === "POST") { | |
| const tenantId = normalizeTenantId( | |
| decodeURIComponent(tenantMigrationMatch[1]!) | |
| ) | |
| if (!tenantId) { | |
| return json({ error: "Invalid tenant" }, { status: 400 }) | |
| } | |
| const record = await parseBoundedJsonRecord(request, 4 * 1024) | |
| if (record instanceof Response) return record |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloudflare/packages/worker/src/index.ts` around lines 5017 - 5020, Validate
the decoded tenantId in the tenant migration POST handler before parsing the
request or calling adminActor. Use normalizeTenantId, return HTTP 400 for an
invalid value consistent with /_subrouter/ready, and pass the normalized tenant
ID to the Durable Object.
| switch (account.kind) { | ||
| case "codex_oauth": { | ||
| const accessToken = requiredSecret(credentials.accessToken) | ||
| const refreshToken = requiredSecret(credentials.refreshToken) | ||
| const idToken = requiredSecret(credentials.idToken) | ||
| const accountID = requiredSecret(credentials.accountId) | ||
| return { | ||
| provider: "codex", | ||
| accountId: id, | ||
| label, | ||
| tokens: { accessToken, refreshToken, idToken, accountID }, | ||
| } | ||
| } | ||
| case "openai_apikey": | ||
| return { | ||
| provider: "openai-apikey", | ||
| accountId: id, | ||
| label, | ||
| apiKey: requiredSecret(credentials.apiKey), | ||
| } | ||
| case "anthropic_apikey": | ||
| return { | ||
| provider: "anthropic-apikey", | ||
| accountId: id, | ||
| label, | ||
| apiKey: requiredSecret(credentials.apiKey), | ||
| } | ||
| case "anthropic_oauth": | ||
| throw new Error("legacy Claude OAuth migration is not supported") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
An unvalidated kind value produces an undefined upload. The Durable Object reads the kind TEXT column and asserts the type with a cast, and migrationUpload then branches on that value without a fallback. A row holding any other value returns undefined from migrationUpload. The failure surfaces late and after an outbound request: Line 101 posts the body "undefined" to the destination, and Line 117 throws a TypeError on upload.accountId.
cloudflare/packages/worker/src/legacy-migration.ts#L168-L197: add adefaultbranch to theswitchthat throws, so an unknown kind is rejected before the first upload.cloudflare/packages/worker/src/index.ts#L2557-L2559: replacerow.kind as AccountKindwith a runtime check against the knownAccountKindvalues, and reject the row when it does not match.
📍 Affects 2 files
cloudflare/packages/worker/src/legacy-migration.ts#L168-L197(this comment)cloudflare/packages/worker/src/index.ts#L2557-L2559
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloudflare/packages/worker/src/legacy-migration.ts` around lines 168 - 197,
Reject unknown legacy account kinds before migration uploads: add a
default-throwing branch to migrationUpload’s switch in
cloudflare/packages/worker/src/legacy-migration.ts (lines 168-197), and replace
the unchecked row.kind as AccountKind cast with runtime validation against all
known AccountKind values in cloudflare/packages/worker/src/index.ts (lines
2557-2559), rejecting invalid rows.
04aed6b to
63bbc58
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/proxy/multitenant.go (1)
736-739: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the staging failure cause.
This code discards the
StageMigrationBatcherror and always answers 409. An operator then cannot tell a validation conflict from a disk failure, and no log line exists. Log the failure throughserver.Loggerwith the migration id only, and map non-conflict errors to 500. Do not include account identifiers or credentials in the log.As per coding guidelines: "Never log access tokens, refresh tokens, API keys, request bodies, or complete
Authorizationheaders."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/proxy/multitenant.go` around lines 736 - 739, Update the StageMigrationBatch error handling in the migration staging handler to log the failure through server.Logger with only the migration ID, excluding account identifiers and credentials. Preserve the 409 response for conflict errors, but return HTTP 500 for non-conflict errors instead of mapping every failure to 409.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cloudflare/packages/worker/src/index.ts`:
- Around line 2639-2642: Update the migration flow around blockConcurrencyWhile
so outbound migrateLegacyAccountsToHosted and activateLegacyMigrationDestination
requests cannot make the barrier exceed Cloudflare’s limit: either shorten their
AbortSignal.timeout deadlines to fit the sequential stage/activate/rollback
flow, or move those requests outside the barrier while retaining only local
transactions inside it. Ensure rollback follows the same bounded handling and
revise the nearby comment to describe the actual sequential execution rather
than parallel uploads.
In `@internal/proxy/multitenant.go`:
- Around line 757-762: Update the compensation path in the migration activation
handler around server.reloadAccounts: use a context detached from r.Context()
for both RollbackMigrationBatch and the compensating reloadAccounts call, so
cleanup runs after request cancellation. Capture and report the
RollbackMigrationBatch error instead of discarding it, while preserving the
existing internal-server-error response.
---
Nitpick comments:
In `@internal/proxy/multitenant.go`:
- Around line 736-739: Update the StageMigrationBatch error handling in the
migration staging handler to log the failure through server.Logger with only the
migration ID, excluding account identifiers and credentials. Preserve the 409
response for conflict errors, but return HTTP 500 for non-conflict errors
instead of mapping every failure to 409.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 60ee725a-adef-41d7-b0d8-a46eb74e5483
📒 Files selected for processing (6)
cloudflare/packages/worker/src/index.tscloudflare/packages/worker/src/legacy-migration.tscloudflare/packages/worker/test/tenant-migration.test.tsinternal/accounts/codex_store.gointernal/proxy/multitenant.gointernal/proxy/multitenant_test.go
| if _, _, err := server.reloadAccounts(r.Context()); err != nil { | ||
| _ = server.AccountRef.store.RollbackMigrationBatch(batchID) | ||
| _, _, _ = server.reloadAccounts(r.Context()) | ||
| http.Error(w, "activate migration accounts", http.StatusInternalServerError) | ||
| return | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not run the compensating rollback on the request context.
Both compensation steps use r.Context(). The caller applies a 10-second deadline to this request. If the deadline expires or the client disconnects, the context is already canceled, so reloadAccounts cannot restore the in-memory account set after RollbackMigrationBatch removed the activation marker. The process then serves accounts that no longer exist on disk until the next successful reload. The discarded RollbackMigrationBatch error hides the same failure.
Use a context that is detached from the request for the compensation, and report the rollback error.
🛠️ Proposed change
if _, _, err := server.reloadAccounts(r.Context()); err != nil {
- _ = server.AccountRef.store.RollbackMigrationBatch(batchID)
- _, _, _ = server.reloadAccounts(r.Context())
+ compensation := context.WithoutCancel(r.Context())
+ if rollbackErr := server.AccountRef.store.RollbackMigrationBatch(batchID); rollbackErr != nil && server.Logger != nil {
+ server.Logger.Error("migration rollback failed", "migrationId", batchID)
+ }
+ _, _, _ = server.reloadAccounts(compensation)
http.Error(w, "activate migration accounts", http.StatusInternalServerError)
return
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/proxy/multitenant.go` around lines 757 - 762, Update the
compensation path in the migration activation handler around
server.reloadAccounts: use a context detached from r.Context() for both
RollbackMigrationBatch and the compensating reloadAccounts call, so cleanup runs
after request cancellation. Capture and report the RollbackMigrationBatch error
instead of discarding it, while preserving the existing internal-server-error
response.
f5be62e to
21d0127
Compare
21d0127 to
487e000
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
cloudflare/packages/worker/test/tenant-migration.test.ts (4)
855-876: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRead the request body only for the paths that need it.
Line 860 parses JSON before the path check. The fallback at line 875 returns 404 for an unknown path, but a request without a JSON body throws at line 860 first. Inside a
Bun.servehandler that surfaces as a 500, which hides the intended 404 and makes a routing regression harder to diagnose.Move the body read into each branch.
♻️ Proposed refactor
const path = new URL(request.url).pathname - const body = (await request.json()) as Record<string, any> if (path.endsWith("/stage")) { + const body = (await request.json()) as Record<string, any> const accounts = body.accounts as Array<Record<string, any>> options.uploads?.push(...accounts) return Response.json({ ok: true, accountIds: accounts.map((account) => account.accountId), }) } if (path.endsWith("/activate")) { + const body = (await request.json()) as Record<string, any> return Response.json({ ok: true, activated: body.accountIds }) } if (path.endsWith("/rollback")) { return Response.json({ ok: true, rolledBack: true }) } return new Response("not found", { status: 404 })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudflare/packages/worker/test/tenant-migration.test.ts` around lines 855 - 876, Update hostedMigrationResponse to determine the pathname before parsing the request body, then read JSON only inside the /stage and /activate branches that consume it. Keep /rollback body-independent and ensure unknown paths reach the existing 404 response without attempting to parse a body.
823-853: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
rejectAuthorizationor make it check the header.The option name states that the fixture rejects the authorization. The implementation returns 401 for every request without reading the
authorizationheader. A future test that passes a wrong tenant key would still see the same 401 and would not prove the header is validated. Rename the option torespondUnauthorized, or compare the bearer token against the expected tenant key.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudflare/packages/worker/test/tenant-migration.test.ts` around lines 823 - 853, The hostedMigrationFetch fixture’s rejectAuthorization option does not validate authorization headers and misleadingly implies header checking. Rename rejectAuthorization to respondUnauthorized throughout this fixture and its callers, preserving the existing behavior of returning 401 for every request when enabled.
132-164: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueAssert the redaction intent explicitly.
hostedMigrationFetch({ failStage: true })throws"network body with secret", and line 161 asserts the surfaced message is"destination is unavailable". That check depends on an exact-match rejection, so a future change to a message that both contains the secret text and the phrase would still pass. Add a negative assertion that the rejection message does not contain"secret".This makes the guideline "Never log access tokens, refresh tokens, API keys, request bodies, or complete
Authorizationheaders" verifiable from the test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudflare/packages/worker/test/tenant-migration.test.ts` around lines 132 - 164, Update the rejection assertion in the test around migrateLegacyTenant to also verify that the surfaced error message does not contain “secret”. Preserve the existing “destination is unavailable” expectation while explicitly validating that sensitive body content is redacted.Source: Coding guidelines
380-397: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAttach a rejection handler to the deferred
preflightpromise.Line 380 starts
preflightand line 397 awaits it. Between those lines the test awaitswaitForConditionandmigrateTenant. IfwaitForConditiontimes out and throws,preflightbecomes an unhandled rejection and the failure output shows the rejection instead of the timeout message. The tests at lines 619 and 677 already use.catch(() => null)for the same pattern.Apply the same handling here for consistent failure output.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudflare/packages/worker/test/tenant-migration.test.ts` around lines 380 - 397, The deferred preflight promise in the migration test can reject before it is awaited, causing an unhandled rejection to mask timeout failures. Attach a rejection handler to the promise created by migrateTenant in the preflight setup, matching the existing .catch(() => null) pattern used by the tests around lines 619 and 677, while preserving the later preflightResponse await.cloudflare/packages/worker/src/legacy-migration.ts (1)
110-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePreserve the rollback failure for diagnosis.
Line 124 discards the rollback error when
activationAttemptedis false. The recovery state is preserved throughpreserveRecovery, so behavior is correct. However, an operator retrying later has no record of why the rollback failed. Attach the rollback failure to the rethrown error withcause, or record it in the migration state.Keep the message free of the tenant key and of any credential text, as required by the guideline "Never log access tokens, refresh tokens, API keys, request bodies, or complete
Authorizationheaders".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cloudflare/packages/worker/src/legacy-migration.ts` around lines 110 - 130, Preserve the rollback exception in the inner catch around rollbackLegacyMigrationDestination instead of discarding it when activationAttempted is false. Attach it as the cause of the rethrown migration error or store it through preserveRecovery, while keeping the error message free of tenant keys and credential or request data.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cloudflare/packages/worker/src/legacy-migration.ts`:
- Around line 386-401: Update terminalControlPattern used by
normalizedMigrationText to reject only characters that break terminal output,
removing format characters such as U+200D and U+200C from this label validation
path. Preserve strict rejection for account.id through its separate validation,
and keep the existing trimming and byte-limit checks unchanged.
- Around line 82-105: Update migrateLegacyAccountsToHosted to return both the
migration count and the normalized staged accountIds produced by
migrationUpload, then use migrated.accountIds in
activateLegacyMigrationDestination instead of deriving IDs from accounts.
Preserve the existing activation flow while ensuring migration and activation
use the same exact ID set.
- Around line 404-409: Update validatedMigrationUploads to handle an empty
accounts array separately from the maxMigrationAccounts check, returning an
error message that clearly identifies the absence of migratable accounts while
preserving the existing limit error for oversized batches.
In `@internal/accounts/codex_store_test.go`:
- Around line 74-107: The rollback path in RollbackMigrationBatch must preserve
membership for accounts activated by a batch even after SaveStored clears or
changes MigrationBatchID. Update activation and rollback to use the stored
activation-time account set or another immutable batch membership record, while
retaining existing rollback behavior for accounts still carrying the batch ID.
---
Nitpick comments:
In `@cloudflare/packages/worker/src/legacy-migration.ts`:
- Around line 110-130: Preserve the rollback exception in the inner catch around
rollbackLegacyMigrationDestination instead of discarding it when
activationAttempted is false. Attach it as the cause of the rethrown migration
error or store it through preserveRecovery, while keeping the error message free
of tenant keys and credential or request data.
In `@cloudflare/packages/worker/test/tenant-migration.test.ts`:
- Around line 855-876: Update hostedMigrationResponse to determine the pathname
before parsing the request body, then read JSON only inside the /stage and
/activate branches that consume it. Keep /rollback body-independent and ensure
unknown paths reach the existing 404 response without attempting to parse a
body.
- Around line 823-853: The hostedMigrationFetch fixture’s rejectAuthorization
option does not validate authorization headers and misleadingly implies header
checking. Rename rejectAuthorization to respondUnauthorized throughout this
fixture and its callers, preserving the existing behavior of returning 401 for
every request when enabled.
- Around line 132-164: Update the rejection assertion in the test around
migrateLegacyTenant to also verify that the surfaced error message does not
contain “secret”. Preserve the existing “destination is unavailable” expectation
while explicitly validating that sensitive body content is redacted.
- Around line 380-397: The deferred preflight promise in the migration test can
reject before it is awaited, causing an unhandled rejection to mask timeout
failures. Attach a rejection handler to the promise created by migrateTenant in
the preflight setup, matching the existing .catch(() => null) pattern used by
the tests around lines 619 and 677, while preserving the later preflightResponse
await.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 89a1e20f-8e62-4cca-b1ce-0fb14bfbb9e9
📒 Files selected for processing (8)
cloudflare/packages/worker/src/index.tscloudflare/packages/worker/src/legacy-migration.tscloudflare/packages/worker/test/tenant-migration.test.tsinternal/accounts/codex_auth.gointernal/accounts/codex_store.gointernal/accounts/codex_store_test.gointernal/proxy/multitenant.gointernal/proxy/multitenant_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/proxy/multitenant.go
- cloudflare/packages/worker/src/index.ts
- internal/accounts/codex_store.go
| const migrated = await migrateLegacyAccountsToHosted({ | ||
| destinationUrl, | ||
| tenantKey, | ||
| migrationId, | ||
| accounts, | ||
| allowLoopback: options.allowLoopback, | ||
| fetch: options.fetch, | ||
| onRequestStart: () => { | ||
| destinationAttempted = true | ||
| }, | ||
| }) | ||
| if (sourceBegan) { | ||
| await options.source.markActivating?.() | ||
| await activateLegacyMigrationDestination({ | ||
| destinationUrl, | ||
| tenantKey, | ||
| migrationId, | ||
| accountIds: accounts.map((account) => account.id), | ||
| allowLoopback: options.allowLoopback, | ||
| fetch: options.fetch, | ||
| onRequestStart: () => { | ||
| activationAttempted = true | ||
| }, | ||
| }) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Derive activation account IDs from the staged uploads, not from the raw accounts.
migrationUpload trims each id through normalizedMigrationText before staging. Line 99 sends the untrimmed account.id values to /activate. If any source id carries leading or trailing whitespace, the staged id set and the activation id set differ. The destination activates an exact account set, so activation then fails after begin() already quiesced the source.
Return the staged ids from migrateLegacyAccountsToHosted and pass those to activateLegacyMigrationDestination, so both requests use one derivation.
🛠️ Sketch of the change
- const migrated = await migrateLegacyAccountsToHosted({
+ const staged = await migrateLegacyAccountsToHosted({
destinationUrl,
tenantKey,
migrationId,
accounts,
allowLoopback: options.allowLoopback,
fetch: options.fetch,
onRequestStart: () => {
destinationAttempted = true
},
})
if (sourceBegan) {
await options.source.markActivating?.()
await activateLegacyMigrationDestination({
destinationUrl,
tenantKey,
migrationId,
- accountIds: accounts.map((account) => account.id),
+ accountIds: staged.accountIds,migrateLegacyAccountsToHosted then returns { count, accountIds } instead of a bare number.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloudflare/packages/worker/src/legacy-migration.ts` around lines 82 - 105,
Update migrateLegacyAccountsToHosted to return both the migration count and the
normalized staged accountIds produced by migrationUpload, then use
migrated.accountIds in activateLegacyMigrationDestination instead of deriving
IDs from accounts. Preserve the existing activation flow while ensuring
migration and activation use the same exact ID set.
| const terminalControlPattern = /[\p{Cc}\p{Cf}\p{Zl}\p{Zp}]/u | ||
|
|
||
| function normalizedMigrationText( | ||
| value: unknown, | ||
| maxBytes: number | ||
| ): string | null { | ||
| if (typeof value !== "string") return null | ||
| const normalized = value.trim() | ||
| if ( | ||
| !normalized || | ||
| terminalControlPattern.test(normalized) || | ||
| new TextEncoder().encode(normalized).byteLength > maxBytes | ||
| ) { | ||
| return null | ||
| } | ||
| return normalized |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
\p{Cf} rejection can block migration permanently for a valid label.
terminalControlPattern includes \p{Cf}, which matches U+200D ZERO WIDTH JOINER and U+200C ZWNJ. Those code points appear in legitimate emoji sequences and in Indic scripts. An existing account label that contains one fails migrationUpload with "legacy account cannot be migrated safely". The source is quiesced or read-only during migration, so the operator has no path to edit the label and retry.
Restrict the pattern to the characters that break terminal output, or strip the disallowed code points from the label instead of rejecting the account. Keep the strict rejection for account.id, since the id must match the destination exactly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloudflare/packages/worker/src/legacy-migration.ts` around lines 386 - 401,
Update terminalControlPattern used by normalizedMigrationText to reject only
characters that break terminal output, removing format characters such as U+200D
and U+200C from this label validation path. Preserve strict rejection for
account.id through its separate validation, and keep the existing trimming and
byte-limit checks unchanged.
| function validatedMigrationUploads( | ||
| accounts: ReadonlyArray<LegacyMigrationAccount> | ||
| ): ReadonlyArray<Record<string, unknown>> { | ||
| if (accounts.length === 0 || accounts.length > maxMigrationAccounts) { | ||
| throw new Error("hosted migration account limit exceeded") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Separate the empty-batch error from the limit error.
An empty account list raises "hosted migration account limit exceeded". The operator reads that message and looks for too many accounts. The real cause is a tenant with no migratable accounts.
✏️ Proposed fix
- if (accounts.length === 0 || accounts.length > maxMigrationAccounts) {
+ if (accounts.length === 0) {
+ throw new Error("hosted migration has no migratable accounts")
+ }
+ if (accounts.length > maxMigrationAccounts) {
throw new Error("hosted migration account limit exceeded")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function validatedMigrationUploads( | |
| accounts: ReadonlyArray<LegacyMigrationAccount> | |
| ): ReadonlyArray<Record<string, unknown>> { | |
| if (accounts.length === 0 || accounts.length > maxMigrationAccounts) { | |
| throw new Error("hosted migration account limit exceeded") | |
| } | |
| function validatedMigrationUploads( | |
| accounts: ReadonlyArray<LegacyMigrationAccount> | |
| ): ReadonlyArray<Record<string, unknown>> { | |
| if (accounts.length === 0) { | |
| throw new Error("hosted migration has no migratable accounts") | |
| } | |
| if (accounts.length > maxMigrationAccounts) { | |
| throw new Error("hosted migration account limit exceeded") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cloudflare/packages/worker/src/legacy-migration.ts` around lines 404 - 409,
Update validatedMigrationUploads to handle an empty accounts array separately
from the maxMigrationAccounts check, returning an error message that clearly
identifies the absence of migratable accounts while preserving the existing
limit error for oversized batches.
| func TestCodexStoreRollbackOwnsAnOrdinaryReplacementOfAnActiveBatchAccount(t *testing.T) { | ||
| store := CodexStore{Dir: t.TempDir()} | ||
| staged := StoredCodexAccount{ | ||
| Email: "migrated@example.com", | ||
| Provider: ProviderCodex, | ||
| Auth: CodexAuthFile{ | ||
| AuthMode: "apikey", | ||
| OpenAIAPIKey: "sk-migrated", | ||
| }, | ||
| } | ||
| const batchID = "replacement-ownership" | ||
| if err := store.StageMigrationBatch(batchID, []StoredCodexAccount{staged}); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := store.ActivateMigrationBatch(batchID, []string{staged.Email}); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| replacement := staged | ||
| replacement.MigrationBatchID = "" | ||
| replacement.Auth.OpenAIAPIKey = "sk-repaired" | ||
| if err := store.SaveStored(replacement); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := store.RollbackMigrationBatch(batchID); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| accounts, err := store.ListStored() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if len(accounts) != 0 { | ||
| t.Fatalf("rollback left ordinary replacement active: %+v", accounts) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect RollbackMigrationBatch and SaveStored to confirm the account-matching gap.
rg -n -B2 -A 30 'func \(s CodexStore\) RollbackMigrationBatch' internal/accounts/codex_store.go
rg -n -B2 -A 20 'func \(s CodexStore\) SaveStored' internal/accounts/codex_store.go
rg -n -A 25 'func \(s CodexStore\) listStored' internal/accounts/codex_store.goRepository: manaflow-ai/subrouter
Length of output: 2809
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== codex_store.go relevant implementation =="
sed -n '130,230p;530,580p' internal/accounts/codex_store.go
echo "== migration batch marker helpers =="
rg -n -B3 -A20 'migrationBatchMarker|ActivateMigrationBatch|StageMigrationBatch|activeMigrationBatchSnapshot|lockStoredAcc' internal/accounts/codex_store.go
echo "== remaining RollbackMigrationBatch scan =="
sed -n '580,615p' internal/accounts/codex_store.go
echo "== account identifier/filepath helpers =="
rg -n -A25 'func .*account.*Filename|filenameFor|loadStored|saveStoredUnlocked' internal/accounts/codex_store.go | sed -n '1,220p'Repository: manaflow-ai/subrouter
Length of output: 18890
Preserve batch membership when deleting an active migration after SaveStored clears the tag.
RollbackMigrationBatch reads accounts only by matching account.MigrationBatchID == batchID, so replacing an active batch account after activation with an ordinary account whose MigrationBatchID is "" makes the rollback skip it. Use the account set captured at activation time, or another immutable batch membership track, instead of depending on the live mutable field.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/accounts/codex_store_test.go` around lines 74 - 107, The rollback
path in RollbackMigrationBatch must preserve membership for accounts activated
by a batch even after SaveStored clears or changes MigrationBatchID. Update
activation and rollback to use the stored activation-time account set or another
immutable batch membership record, while retaining existing rollback behavior
for accounts still carrying the batch ID.
1 participant
Copies legacy credentials directly from the Cloudflare Durable Object into an inactive Hosted migration batch without returning secrets to the operator.
Pre-copy never routes or refreshes cloned OAuth tokens. Finalization quiesces legacy refreshes, atomically activates the complete Hosted batch, then replaces the legacy credential snapshot with a completed receipt. Recovery is bound to the original Hosted origin, migration ID, and tenant-key hash. Confirmed rollback restores legacy routing; ambiguous activation stays quiesced until an idempotent retry resolves it.
Tests:
go test ./...; Cloudflarebun run typecheck; Cloudflarebun test(73 pass).Summary by CodeRabbit
New Features
Bug Fixes