Repository navigation
Fix account lifecycle export and deletion safety - #890
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds account-deletion markers and write leases, makes deletion inventory-driven and atomic, adds strict R2 cleanup, introduces bounded chunked R2 exports, narrows exported community data, and removes legacy package-retriever keys. ChangesAccount lifecycle enforcement
Account export and storage contracts
Package retriever cache cleanup
Migration registration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
cursor
Bot
force-pushed
the
cursor/review-fix-account-lifecycle-aeba
branch
from
July 22, 2026 23:47
7dbf633 to
9a6b2e7
Compare
kody-bot
marked this pull request as ready for review
July 22, 2026 23:49
Contributor
|
🔎 Preview deployed: https://kody-pr-890.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/community/avatar.ts (1)
223-251: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
deleteUserAvatarisn't lease-protected — TOCTOU with account deletion.Unlike
saveUserAvatar, this function checksassertAccountWritableDbonce and then performs an unguardedUPDATE(nodeleting_at IS NULLclause, no.meta.changescheck) without holdingwithAccountWriteLease. Between the check and the update,markAccountDeletingcan start (and, since this write never incrementedactive_write_count, the deletion won't detect it as an active writer), letting deletion cleanup race with this in-flight delete.🔒 Proposed fix to lease-protect the delete path
export async function deleteUserAvatar(input: { env: Pick<Env, 'APP_DB' | 'COMMUNITY_ASSETS'> numericUserId: number stableUserId: string }): Promise<void> { - await assertAccountWritableDb(input.env.APP_DB, input.stableUserId) - const existing = await input.env.APP_DB.prepare( - `SELECT avatar_key FROM users WHERE id = ?`, - ) - .bind(input.numericUserId) - .first<{ avatar_key: string | null }>() - const previousKey = - existing?.avatar_key == null ? null : String(existing.avatar_key) - - await input.env.APP_DB.prepare( - `UPDATE users - SET avatar_key = NULL, updated_at = ? - WHERE id = ?`, - ) - .bind(utcSqliteTimestamp(), input.numericUserId) - .run() - - if (!previousKey) return - try { - await input.env.COMMUNITY_ASSETS.delete(previousKey) - } catch (error) { - console.error('user-avatar-delete-failed', previousKey, error) - } + await withAccountWriteLease({ + db: input.env.APP_DB, + stableUserId: input.stableUserId, + async write() { + await assertAccountWritableDb(input.env.APP_DB, input.stableUserId) + const existing = await input.env.APP_DB.prepare( + `SELECT avatar_key FROM users WHERE id = ?`, + ) + .bind(input.numericUserId) + .first<{ avatar_key: string | null }>() + const previousKey = + existing?.avatar_key == null ? null : String(existing.avatar_key) + + const update = await input.env.APP_DB.prepare( + `UPDATE users + SET avatar_key = NULL, updated_at = ? + WHERE id = ? AND deleting_at IS NULL`, + ) + .bind(utcSqliteTimestamp(), input.numericUserId) + .run() + if ((update.meta.changes ?? 0) !== 1) { + throw new Error( + 'Avatar delete was rejected because account state changed.', + ) + } + + if (!previousKey) return + try { + await input.env.COMMUNITY_ASSETS.delete(previousKey) + } catch (error) { + console.error('user-avatar-delete-failed', previousKey, error) + } + }, + }) }Would you like a matching regression test added, mirroring the existing
saveUserAvatar removes an in-flight upload when deletion startscase inavatar.node.test.ts?🤖 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 `@packages/worker/src/community/avatar.ts` around lines 223 - 251, Update deleteUserAvatar to perform the account validation, avatar lookup, database update, and asset deletion within withAccountWriteLease, matching saveUserAvatar’s lease-protected flow. Guard the UPDATE with deleting_at IS NULL and verify its meta.changes result so deletion cannot race with account cleanup or proceed after the account enters deletion.
🤖 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 `@packages/worker/src/app/account-deletion.ts`:
- Around line 1029-1044: Avoid adding the EMAIL_BLOBS inventory deletion count
to result.deletedEmailBlobs after deleteAccountEmailBlobPrefixes already removes
those canonical prefixes. Remove the redundant deleteR2Objects count update, or
filter its keys to exclude the deleted email and attachment prefixes while
preserving safe cleanup.
In `@packages/worker/src/email/service.ts`:
- Around line 162-167: Update the catch block around the email insertion flow to
make the blobs.delete cleanup best-effort: handle or suppress any rejection from
deleting rawMimeKey, then always rethrow the original caught error. Preserve the
existing null check and cleanup attempt.
In `@packages/worker/src/mcp-auth-user-context.ts`:
- Around line 105-108: Update the comment immediately above the catch in the MCP
user-context role-loading flow to describe the current fail-closed behavior:
transient D1 failures are rethrown and the request does not fall back to
baseUser. Keep the existing rethrow implementation unchanged.
In `@packages/worker/src/package-retrievers/manifest-cache.ts`:
- Around line 124-141: Update deleteLegacyPackageRetrieverScopeIndexes to return
the actual number of legacy keys deleted rather than
packageRetrieverScopes.length; use the KV delete results to count successful
deletions. Ensure deleteAllPackageRetrieverCacheEntriesForUser adds this actual
count to keys.size so already-clean accounts report zero deleted keys.
---
Outside diff comments:
In `@packages/worker/src/community/avatar.ts`:
- Around line 223-251: Update deleteUserAvatar to perform the account
validation, avatar lookup, database update, and asset deletion within
withAccountWriteLease, matching saveUserAvatar’s lease-protected flow. Guard the
UPDATE with deleting_at IS NULL and verify its meta.changes result so deletion
cannot race with account cleanup or proceed after the account enters deletion.
🪄 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: a70ae9f3-4259-424f-add6-a8bb676dde03
📒 Files selected for processing (41)
docs/contributing/architecture/data-storage.mddocs/use/privacy.mdpackages/worker/migrations/0084-account-deleting-state.sqlpackages/worker/migrations/0085-account-write-leases.sqlpackages/worker/src/app/account-data-targets.node.test.tspackages/worker/src/app/account-data-targets.tspackages/worker/src/app/account-deletion-state.tspackages/worker/src/app/account-deletion.node.test.tspackages/worker/src/app/account-deletion.tspackages/worker/src/app/account-export.node.test.tspackages/worker/src/app/account-export.tspackages/worker/src/app/account-r2-export.tspackages/worker/src/app/account-r2-inventory.tspackages/worker/src/app/account-r2-prefix-cleanup.node.test.tspackages/worker/src/app/account-r2-prefix-cleanup.tspackages/worker/src/app/account-user-owned-surfaces.node.test.tspackages/worker/src/app/account-user-owned-surfaces.tspackages/worker/src/app/authenticated-user.node.test.tspackages/worker/src/app/authenticated-user.tspackages/worker/src/app/handlers/account-delete.tspackages/worker/src/app/request-auth-cache.tspackages/worker/src/community/avatar.node.test.tspackages/worker/src/community/avatar.tspackages/worker/src/community/community-flow-test-schema.tspackages/worker/src/community/community-icon.node.test.tspackages/worker/src/community/community-icon.tspackages/worker/src/db.tspackages/worker/src/email/inbound.workers.test.tspackages/worker/src/email/outbound.tspackages/worker/src/email/service.tspackages/worker/src/entitlements/test-schema.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp-auth-user-context.node.test.tspackages/worker/src/mcp-auth-user-context.tspackages/worker/src/package-invocations/http.tspackages/worker/src/package-invocations/http.workers.test.tspackages/worker/src/package-invocations/idempotent-module-invocation.tspackages/worker/src/package-invocations/service.node.test.tspackages/worker/src/package-registry/test-schema.tspackages/worker/src/package-retrievers/manifest-cache.node.test.tspackages/worker/src/package-retrievers/manifest-cache.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/community/avatar.ts (1)
167-200: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winScope the avatar row reads and writes by
stableUserId.Both paths lease and namespace R2 data using
input.stableUserId, but select/updateusersusing onlyinput.numericUserId. If those IDs ever disagree, this can overwrite and delete another user’s avatar while holding a lease for the wrong account. Addstable_user_id = ?to bothSELECTandUPDATEpredicates and bindinput.stableUserId.Proposed fix
- `SELECT avatar_key FROM users WHERE id = ?` + `SELECT avatar_key FROM users WHERE id = ? AND stable_user_id = ?` - `WHERE id = ? AND deleting_at IS NULL` + `WHERE id = ? AND stable_user_id = ? AND deleting_at IS NULL`As per coding guidelines, “every read/write path must be scoped by
userId” and “Cross-user data sharing is a bug.”Also applies to: 233-251
🤖 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 `@packages/worker/src/community/avatar.ts` around lines 167 - 200, Scope the avatar database operations in the avatar update flow by stableUserId: update the existing avatar_key SELECT and the users UPDATE predicate to require stable_user_id = ?, and bind input.stableUserId alongside the existing numeric ID. Apply the same stable-user constraint to the later cleanup-related users read/write path around the corresponding avatar handling logic.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.
Outside diff comments:
In `@packages/worker/src/community/avatar.ts`:
- Around line 167-200: Scope the avatar database operations in the avatar update
flow by stableUserId: update the existing avatar_key SELECT and the users UPDATE
predicate to require stable_user_id = ?, and bind input.stableUserId alongside
the existing numeric ID. Apply the same stable-user constraint to the later
cleanup-related users read/write path around the corresponding avatar handling
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 04b3b82f-0da3-4871-9f34-112a345f11a2
📒 Files selected for processing (6)
packages/worker/src/app/account-deletion.node.test.tspackages/worker/src/app/account-deletion.tspackages/worker/src/community/avatar.tspackages/worker/src/email/service.tspackages/worker/src/mcp-auth-user-context.tspackages/worker/src/package-retrievers/manifest-cache.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/worker/src/mcp-auth-user-context.ts
- packages/worker/src/package-retrievers/manifest-cache.ts
- packages/worker/src/app/account-deletion.ts
- packages/worker/src/app/account-deletion.node.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/worker/src/jobs/service.ts (1)
1271-1273: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid re-acquiring the write lease inside the leased scheduler paths.
executeJobOncecallswithAccountWriteLease, but it is only invoked fromrunJobNowandrunDueJobsForUser, both of which already acquire a write lease before calling it. Add an internal un-leased executor helper for these callers and keep the leased wrapper only for standalone job execution to avoid an extra lease acquire/heartbeat/release round-trip per due job.🤖 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 `@packages/worker/src/jobs/service.ts` around lines 1271 - 1273, Refactor executeJobOnce to separate the job execution logic into an internal un-leased helper, then have runJobNow and runDueJobsForUser call that helper while retaining their existing leases. Keep executeJobOnce as the leased wrapper for standalone execution, acquiring the lease before delegating to the helper.
🤖 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.
Nitpick comments:
In `@packages/worker/src/jobs/service.ts`:
- Around line 1271-1273: Refactor executeJobOnce to separate the job execution
logic into an internal un-leased helper, then have runJobNow and
runDueJobsForUser call that helper while retaining their existing leases. Keep
executeJobOnce as the leased wrapper for standalone execution, acquiring the
lease before delegating to the helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: edc5982e-abb3-407d-8ed3-a1168954a534
📒 Files selected for processing (15)
packages/worker/src/app/account-deletion-state.tspackages/worker/src/app/account-deletion.node.test.tspackages/worker/src/app/account-deletion.tspackages/worker/src/app/account-export.node.test.tspackages/worker/src/app/account-export.tspackages/worker/src/community/avatar.tspackages/worker/src/jobs/service.node.test.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp/memory/service.node.test.tspackages/worker/src/mcp/memory/service.tspackages/worker/src/mcp/run-kody-registry.node.test.tspackages/worker/src/package-registry/service.node.test.tspackages/worker/src/package-registry/service.tspackages/worker/src/package-retrievers/manifest-cache.node.test.tspackages/worker/src/package-retrievers/manifest-cache.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/worker/src/app/account-deletion-state.ts
- packages/worker/src/package-retrievers/manifest-cache.ts
- packages/worker/src/community/avatar.ts
- packages/worker/src/app/account-deletion.ts
- packages/worker/src/app/account-export.node.test.ts
- packages/worker/src/app/account-export.ts
- packages/worker/src/app/account-deletion.node.test.ts
cursor
Bot
force-pushed
the
cursor/review-fix-account-lifecycle-aeba
branch
from
July 23, 2026 04:02
40e22ca to
e78e4cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/worker/src/package-retrievers/manifest-cache.ts (2)
461-496: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
deletePrefixper-key deletes could approach the KV subrequest ceiling for large accounts.Each page from
kv.list(up to ~1000 keys) is deleted viaPromise.allof individualkv.delete()calls, plus thelist()call itself, all counting toward the platform's ~1,000 external-service-operations-per-invocation limit. Workers KV bindings don't support bulk delete (only the REST API does), so this is the best available approach with the binding — but for a user with many packages × retrievers × 2 scopes, a single invocation could exceed the cap and throw mid-sweep.The caller (
deleteRetrieverCacheinaccount-deletion.ts) catches this and logs a warning, returning0, so it won't crash account deletion — but it will leave that user's retriever KV entries orphaned instead of cleaned up, which cuts against this PR's stated goal of atomic/complete cleanup.Given this is a platform-level constraint and only bites at scale, consider capping
kv.list({ prefix, cursor, limit })to a safer batch size relative to remaining subrequest budget, or explicitly documenting this as a known best-effort limit.🤖 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 `@packages/worker/src/package-retrievers/manifest-cache.ts` around lines 461 - 496, Update deletePrefix within deleteAllPackageRetrieverCacheEntriesForUser to use a conservative kv.list page limit so each sweep stays within the Workers KV subrequest budget, while preserving cursor-based pagination and deletion counts. Document that cleanup is best-effort and may stop or require retry for users whose key volume exceeds the invocation limit.
22-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
packageRetrieverScopesfrom the shared schema.
packageRetrieverScopeValuesis already exported from#worker/package-registry/types.tswith the same['search', 'context']values. Having this module duplicate the full scope list creates drift risk; new scopes added to the shared source won’t automatically be included in cleanup/refresh loops.♻️ Suggested fix
-const packageRetrieverScopes = [ - 'search', - 'context', -] satisfies Array<PackageRetrieverScope> +const packageRetrieverScopes = packageRetrieverScopeValuesImport
packageRetrieverScopeValuesfrom../package-registry/types.ts;PackageRetrieverScopealready maps to this source.🤖 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 `@packages/worker/src/package-retrievers/manifest-cache.ts` around lines 22 - 26, Update packageRetrieverScopes in manifest-cache.ts to derive its values from the shared packageRetrieverScopeValues export in package-registry/types.ts, importing that symbol and removing the duplicated literal array. Preserve the existing PackageRetrieverScope typing and ensure cleanup/refresh loops use the shared source.
🤖 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 `@packages/worker/src/mcp-auth.workers.test.ts`:
- Around line 118-123: Update the mock database client’s async run() method to
accept the lease-update query only when it includes the stable_user_id-scoped
WHERE predicate and the bound user ID matches the expected test user. Reject
unscoped or mismatched updates instead of accepting every query beginning with
“update users”.
In `@packages/worker/src/mcp/session-registry.ts`:
- Around line 12-17: Update the session insert/upsert flow in the session
registry to reject an existing do_id when its stored user_id differs from
input.userId, rather than overwriting ownership; preserve idempotent behavior
for the same owner. Add a regression test covering a conflicting do_id from
another user and assert the original owner remains unchanged.
---
Nitpick comments:
In `@packages/worker/src/package-retrievers/manifest-cache.ts`:
- Around line 461-496: Update deletePrefix within
deleteAllPackageRetrieverCacheEntriesForUser to use a conservative kv.list page
limit so each sweep stays within the Workers KV subrequest budget, while
preserving cursor-based pagination and deletion counts. Document that cleanup is
best-effort and may stop or require retry for users whose key volume exceeds the
invocation limit.
- Around line 22-26: Update packageRetrieverScopes in manifest-cache.ts to
derive its values from the shared packageRetrieverScopeValues export in
package-registry/types.ts, importing that symbol and removing the duplicated
literal array. Preserve the existing PackageRetrieverScope typing and ensure
cleanup/refresh loops use the shared source.
🪄 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: a2390574-f3db-4aa6-b828-925c9cac5bef
📒 Files selected for processing (56)
docs/contributing/architecture/data-storage.mddocs/use/privacy.mdpackages/worker/migrations/0084-account-deleting-state.sqlpackages/worker/migrations/0085-account-write-leases.sqlpackages/worker/migrations/0086-mcp-agent-sessions.sqlpackages/worker/src/app/account-data-targets.node.test.tspackages/worker/src/app/account-data-targets.tspackages/worker/src/app/account-deletion-state.tspackages/worker/src/app/account-deletion.node.test.tspackages/worker/src/app/account-deletion.tspackages/worker/src/app/account-export.node.test.tspackages/worker/src/app/account-export.tspackages/worker/src/app/account-r2-export.tspackages/worker/src/app/account-r2-inventory.tspackages/worker/src/app/account-r2-prefix-cleanup.node.test.tspackages/worker/src/app/account-r2-prefix-cleanup.tspackages/worker/src/app/account-user-owned-surfaces.node.test.tspackages/worker/src/app/account-user-owned-surfaces.tspackages/worker/src/app/authenticated-user.node.test.tspackages/worker/src/app/authenticated-user.tspackages/worker/src/app/handlers/account-delete.tspackages/worker/src/app/handlers/account-export.tspackages/worker/src/app/request-auth-cache.tspackages/worker/src/community/avatar.node.test.tspackages/worker/src/community/avatar.tspackages/worker/src/community/community-flow-test-schema.tspackages/worker/src/community/community-icon.node.test.tspackages/worker/src/community/community-icon.tspackages/worker/src/db.tspackages/worker/src/email/inbound.workers.test.tspackages/worker/src/email/outbound.tspackages/worker/src/email/service.tspackages/worker/src/entitlements/test-schema.tspackages/worker/src/jobs/service.node.test.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp-auth-user-context.node.test.tspackages/worker/src/mcp-auth-user-context.tspackages/worker/src/mcp-auth.tspackages/worker/src/mcp-auth.workers.test.tspackages/worker/src/mcp/index.tspackages/worker/src/mcp/memory/service.node.test.tspackages/worker/src/mcp/memory/service.tspackages/worker/src/mcp/run-kody-registry.node.test.tspackages/worker/src/mcp/session-registry.node.test.tspackages/worker/src/mcp/session-registry.tspackages/worker/src/package-invocations/http.tspackages/worker/src/package-invocations/http.workers.test.tspackages/worker/src/package-invocations/idempotent-module-invocation.tspackages/worker/src/package-invocations/service.node.test.tspackages/worker/src/package-registry/service.node.test.tspackages/worker/src/package-registry/service.tspackages/worker/src/package-registry/test-schema.tspackages/worker/src/package-retrievers/manifest-cache.node.test.tspackages/worker/src/package-retrievers/manifest-cache.tstools/check-migrations.node.test.tstools/migration-ledger.json
🚧 Files skipped from review as they are similar to previous changes (35)
- packages/worker/migrations/0085-account-write-leases.sql
- packages/worker/src/entitlements/test-schema.ts
- packages/worker/src/db.ts
- packages/worker/src/mcp/memory/service.node.test.ts
- packages/worker/src/app/account-r2-prefix-cleanup.ts
- packages/worker/src/package-invocations/service.node.test.ts
- packages/worker/src/app/account-data-targets.node.test.ts
- packages/worker/src/mcp/run-kody-registry.node.test.ts
- packages/worker/src/package-registry/test-schema.ts
- packages/worker/src/app/handlers/account-delete.ts
- packages/worker/src/app/authenticated-user.ts
- packages/worker/src/jobs/service.node.test.ts
- packages/worker/src/package-invocations/http.workers.test.ts
- packages/worker/src/community/community-icon.ts
- packages/worker/src/email/service.ts
- packages/worker/src/app/request-auth-cache.ts
- packages/worker/src/app/account-r2-prefix-cleanup.node.test.ts
- packages/worker/src/package-invocations/http.ts
- packages/worker/src/mcp-auth-user-context.node.test.ts
- packages/worker/src/app/authenticated-user.node.test.ts
- packages/worker/src/app/account-r2-inventory.ts
- packages/worker/src/email/inbound.workers.test.ts
- packages/worker/src/app/account-deletion-state.ts
- packages/worker/src/app/account-user-owned-surfaces.node.test.ts
- packages/worker/src/community/community-icon.node.test.ts
- packages/worker/src/package-retrievers/manifest-cache.node.test.ts
- packages/worker/src/app/account-r2-export.ts
- packages/worker/src/jobs/service.ts
- packages/worker/src/app/account-data-targets.ts
- packages/worker/src/mcp/memory/service.ts
- packages/worker/src/package-invocations/idempotent-module-invocation.ts
- packages/worker/src/app/account-export.node.test.ts
- packages/worker/src/app/account-deletion.ts
- packages/worker/src/app/account-deletion.node.test.ts
- packages/worker/src/community/community-flow-test-schema.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/worker/src/mcp/session-registry.ts`:
- Around line 18-21: Update the ownership lookup in the session-registry flow to
filter by both doId and input.userId, binding both values in the query. Treat a
missing matching row as a failed ownership check and preserve the existing
fail-closed behavior; do not read or return another user’s user_id.
🪄 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: 12ed85f0-3765-467e-bc69-695d5826598a
📒 Files selected for processing (3)
packages/worker/src/mcp-auth.workers.test.tspackages/worker/src/mcp/session-registry.node.test.tspackages/worker/src/mcp/session-registry.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/worker/src/mcp/session-registry.node.test.ts
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f927e13. Configure here.
3 of 4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
propskey directly; stored-data objects without proven owners block completionOperator rollout
npm run backfill:mcp-agent-sessions -- --origin https://heykody.dev --worker-script kody --audit-out /tmp/mcp-agent-session-backfill-dry-run.json--execute; anyno_owner, conflict, or failure prevents completionCLOUDFLARE_ACCOUNT_ID,CLOUDFLARE_API_TOKEN,CAPABILITY_REINDEX_SECRETValidation
npm run validatepassed on final revisiona9e9360dSystem recap — extends existing primitives (medium risk)
Mode: recap · Base:
main@a6f027da· Head:a9e9360dInvariants
Summary by CodeRabbit
New Features
r2_objectsection for user-owned media/attachments, with resumable, chunked downloads and bounded metadata.Bug Fixes
Documentation
Chores