- Notifications
You must be signed in to change notification settings - Fork474
Fix(seed): Handle missing permissions during database seeding#910
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:dev
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@ArvindParekh is attempting to deploy a commit to theStack Team onVercel. A member of the Team first needs toauthorize it. |
coderabbitaibot commentedSep 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaced calls to updatePermissionDefinition with ensurePermissionDefinition in the seed script, adjusted payload shapes (use Changes
Sequence Diagram(s)sequenceDiagram autonumber participant Seed as Seed Script participant PermLib as permissions.tsx participant DB as Database rect rgb(240,250,240) Note over Seed,PermLib: Seed now calls ensurePermissionDefinition Seed->>PermLib: ensurePermissionDefinition(scope, tenancy, id, data) end PermLib->>DB: getPermissionDefinition(id) alt exists PermLib->>DB: updatePermissionDefinition(id, data) DB-->>PermLib: updated else not found PermLib->>DB: createPermissionDefinition(id, data) DB-->>PermLib: created end PermLib-->>Seed: result Note over PermLib: Implementation duplicated in fileEstimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR fixes a database seeding issue where missing permissions during seeding caused errors. The problem occurred when the internal project existed butteam_member andteam_admin permissions were missing from the configuration.
- Added
ensurePermissionDefinition()function that safely handles both creating and updating permissions - Updated seed script to use the new function instead of always attempting updates
- Simplified parameter structure by removing redundant
oldIdand nestedidfields
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/backend/src/lib/permissions.tsx | Added newensurePermissionDefinition() function that creates or updates permissions safely |
| apps/backend/prisma/seed.ts | Updated to useensurePermissionDefinition() instead ofupdatePermissionDefinition() for team permissions |
Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn how to get started.
| }); | ||
| }else{ | ||
| // permission doesn't exist, create it | ||
| returnawaitcreatePermissionDefinition(globalTx,{ |
CopilotAISep 22, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ThecreatePermissionDefinition function expects asourceOfTruthTx parameter based on theupdatePermissionDefinition call above, but onlyglobalTx is being passed. This inconsistency could cause issues ifcreatePermissionDefinition requires the second transaction parameter.
| returnawaitcreatePermissionDefinition(globalTx,{ | |
| returnawaitcreatePermissionDefinition(globalTx,sourceOfTruthTx,{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
createPermissionDefinition actually doesn't require thesourceOfTruthTx parameter.
| }); | ||
| awaitupdatePermissionDefinition( | ||
| awaitensurePermissionDefinition( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This code violates the rule to properly handle asynchronous operations. TheensurePermissionDefinition function is async but the call is not wrapped withrunAsynchronously() as required by the code patterns rule. This can lead to unhandled promise rejections if the function fails.
🔍 This comment matches yourcode_patterns.mdc rule.
| awaitensurePermissionDefinition( | |
| awaitrunAsynchronously(ensurePermissionDefinition( |
React with 👍 to tell me that this comment was useful, or 👎 if not (and I'll stop posting more comments like this in the future)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Greptile Summary
This PR introduces a robust solution to fix database seeding failures that occur when permissions are missing from the project configuration. The core change adds a newensurePermissionDefinition() function inpermissions.tsx that handles both creation and update scenarios for permission definitions.
TheensurePermissionDefinition() function first checks if a permission exists in the tenancy configuration usinglistPermissionDefinitionsFromConfig(). If the permission exists, it calls the existingupdatePermissionDefinition() function. If the permission doesn't exist, it creates it usingcreatePermissionDefinition(). This defensive programming approach prevents the "Permission not found" errors that previously occurred when the internal project record existed but essential permissions liketeam_member andteam_admin were missing.
The seed script (seed.ts) is updated to use this new function instead of directly callingupdatePermissionDefinition(). The import statement is modified, and the function calls for bothteam_member andteam_admin permissions are updated to use the new ensure-style approach. The parameter structure is also adjusted - theoldId parameter is removed and the permissionid becomes a top-level parameter rather than being nested in thedata object.
This change addresses edge cases that can occur during Docker re-deployments with persistent databases, interrupted setup processes, or database corruption scenarios where the project exists but its permission definitions are incomplete.
Confidence score: 4/5
- This PR is safe to merge with minimal risk of breaking existing functionality
- Score reflects well-structured defensive programming and proper use of existing functions
- Pay close attention to the parameter handling differences between create and update scenarios
2 files reviewed, 1 comment
| returnawaitcreatePermissionDefinition(globalTx,{ | ||
| scope:options.scope, | ||
| tenancy:options.tenancy, | ||
| data:{ | ||
| id:options.id, | ||
| description:options.data.description, | ||
| contained_permission_ids:options.data.contained_permission_ids, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
logic: Inconsistent transaction parameter usage between create and update operations.createPermissionDefinition only receivesglobalTx, whileupdatePermissionDefinition receives bothglobalTx andsourceOfTruthTx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/prisma/seed.ts (1)
361-366:Bug: stale tenancy used when granting team_admin.grantTeamPermission validates existence against tenancy.config. After creating permissions, you reloaded updatedInternalTenancy, but this call still uses internalTenancy, which may not include the new definition and can throw PermissionNotFound.
Apply:
- await grantTeamPermission(internalPrisma, {- tenancy: internalTenancy,+ await grantTeamPermission(internalPrisma, {+ tenancy: updatedInternalTenancy, teamId: internalTeamId, userId: defaultUserId, permissionId: "team_admin", });
🧹 Nitpick comments (3)
apps/backend/src/lib/permissions.tsx (1)
337-376:Tidy: avoid rename path and redundant awaits.Passing data.id equals oldId needlessly activates rename logic; also return await is unnecessary here.
- Drop data.id in the update branch (already covered in the diff above).
- Keep returns without await (already covered above).
apps/backend/prisma/seed.ts (2)
200-211:Good switch to ensurePermissionDefinition; improve description text.Logic is correct. Consider replacing placeholder descriptions ("1") with meaningful, user-facing text.
Suggested tweak:
data: {- description: "1",+ description: "Team member role", contained_permission_ids: ["$read_members"], }
214-226:Same here: swap placeholder description.Use a clear description for team_admin.
data: {- description: "2",+ description: "Team admin role", contained_permission_ids: ["$read_members", "$remove_members", "$update_team"], }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/backend/prisma/seed.ts(2 hunks)apps/backend/src/lib/permissions.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer ES6 Map over Record when representing key–value collections
Files:
apps/backend/src/lib/permissions.tsxapps/backend/prisma/seed.ts
🧬 Code graph analysis (2)
apps/backend/src/lib/permissions.tsx (2)
apps/backend/src/lib/types.tsx (1)
PrismaTransaction(3-3)apps/backend/src/lib/tenancies.tsx (1)
Tenancy(47-47)
apps/backend/prisma/seed.ts (3)
apps/backend/src/lib/permissions.tsx (1)
ensurePermissionDefinition(337-376)apps/backend/src/prisma-client.tsx (1)
globalPrismaClient(33-33)apps/backend/src/lib/tenancies.tsx (2)
getSoleTenancyFromProjectBranch(59-66)DEFAULT_BRANCH_ID(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Security Check
🔇 Additional comments (1)
apps/backend/prisma/seed.ts (1)
4-4:Import change looks good.Switching to ensurePermissionDefinition aligns seed with upsert behavior.
| exportasyncfunctionensurePermissionDefinition( | ||
| globalTx:PrismaTransaction, | ||
| sourceOfTruthTx:PrismaTransaction, | ||
| options:{ | ||
| scope:"team"|"project", | ||
| tenancy:Tenancy, | ||
| id:string, | ||
| data:{ | ||
| description?:string, | ||
| contained_permission_ids?:string[], | ||
| }, | ||
| } | ||
| ){ | ||
| constexistingPermission=options.tenancy.config.rbac.permissions[options.id]; | ||
| if(existingPermission){ | ||
| // permission already exists, update it | ||
| returnawaitupdatePermissionDefinition(globalTx,sourceOfTruthTx,{ | ||
| scope:options.scope, | ||
| tenancy:options.tenancy, | ||
| oldId:options.id, | ||
| data:{ | ||
| id:options.id, | ||
| description:options.data.description, | ||
| contained_permission_ids:options.data.contained_permission_ids, | ||
| }, | ||
| }); | ||
| }else{ | ||
| // permission doesn't exist, create it | ||
| returnawaitcreatePermissionDefinition(globalTx,{ | ||
| scope:options.scope, | ||
| tenancy:options.tenancy, | ||
| data:{ | ||
| id:options.id, | ||
| description:options.data.description, | ||
| contained_permission_ids:options.data.contained_permission_ids, | ||
| }, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Prevent cross-scope updates in ensurePermissionDefinition.
If an existing permission has a different scope than options.scope, this path will silently flip its scope via updatePermissionDefinition. That’s risky for RBAC integrity. Add an explicit scope guard and fail fast.
Apply this diff:
@@ export async function ensurePermissionDefinition( globalTx: PrismaTransaction, sourceOfTruthTx: PrismaTransaction, options: { scope: "team" | "project", tenancy: Tenancy, id: string, data: { description?: string, contained_permission_ids?: string[], }, } ) {- const existingPermission = options.tenancy.config.rbac.permissions[options.id];+ const existingPermission = options.tenancy.config.rbac.permissions[options.id] as CompleteConfig['rbac']['permissions'][string] | undefined;++ // Do not allow cross-scope edits.+ if (existingPermission && (existingPermission.scope ?? null) !== options.scope) {+ throw new KnownErrors.PermissionScopeMismatch(options.id, options.scope, existingPermission.scope ?? null);+ }@@- return await updatePermissionDefinition(globalTx, sourceOfTruthTx, {+ return updatePermissionDefinition(globalTx, sourceOfTruthTx, { scope: options.scope, tenancy: options.tenancy, oldId: options.id, data: {- id: options.id, description: options.data.description, contained_permission_ids: options.data.contained_permission_ids, }, }); } else { // permission doesn't exist, create it- return await createPermissionDefinition(globalTx, {+ return createPermissionDefinition(globalTx, { scope: options.scope, tenancy: options.tenancy, data: { id: options.id, description: options.data.description, contained_permission_ids: options.data.contained_permission_ids, }, }); } }📝 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.
| exportasyncfunctionensurePermissionDefinition( | |
| globalTx:PrismaTransaction, | |
| sourceOfTruthTx:PrismaTransaction, | |
| options:{ | |
| scope:"team"|"project", | |
| tenancy:Tenancy, | |
| id:string, | |
| data:{ | |
| description?:string, | |
| contained_permission_ids?:string[], | |
| }, | |
| } | |
| ){ | |
| constexistingPermission=options.tenancy.config.rbac.permissions[options.id]; | |
| if(existingPermission){ | |
| // permission already exists, update it | |
| returnawaitupdatePermissionDefinition(globalTx,sourceOfTruthTx,{ | |
| scope:options.scope, | |
| tenancy:options.tenancy, | |
| oldId:options.id, | |
| data:{ | |
| id:options.id, | |
| description:options.data.description, | |
| contained_permission_ids:options.data.contained_permission_ids, | |
| }, | |
| }); | |
| }else{ | |
| // permission doesn't exist, create it | |
| returnawaitcreatePermissionDefinition(globalTx,{ | |
| scope:options.scope, | |
| tenancy:options.tenancy, | |
| data:{ | |
| id:options.id, | |
| description:options.data.description, | |
| contained_permission_ids:options.data.contained_permission_ids, | |
| }, | |
| }); | |
| } | |
| } | |
| exportasyncfunctionensurePermissionDefinition( | |
| globalTx:PrismaTransaction, | |
| sourceOfTruthTx:PrismaTransaction, | |
| options:{ | |
| scope:"team"|"project", | |
| tenancy:Tenancy, | |
| id:string, | |
| data:{ | |
| description?:string, | |
| contained_permission_ids?:string[], | |
| }, | |
| } | |
| ){ | |
| constexistingPermission=options.tenancy.config.rbac.permissions[options.id]asCompleteConfig['rbac']['permissions'][string]|undefined; | |
| // Do not allow cross-scope edits. | |
| if(existingPermission&&(existingPermission.scope??null)!==options.scope){ | |
| thrownewKnownErrors.PermissionScopeMismatch(options.id,options.scope,existingPermission.scope??null); | |
| } | |
| if(existingPermission){ | |
| // permission already exists, update it | |
| returnupdatePermissionDefinition(globalTx,sourceOfTruthTx,{ | |
| scope:options.scope, | |
| tenancy:options.tenancy, | |
| oldId:options.id, | |
| data:{ | |
| description:options.data.description, | |
| contained_permission_ids:options.data.contained_permission_ids, | |
| }, | |
| }); | |
| }else{ | |
| // permission doesn't exist, create it | |
| returncreatePermissionDefinition(globalTx,{ | |
| scope:options.scope, | |
| tenancy:options.tenancy, | |
| data:{ | |
| id:options.id, | |
| description:options.data.description, | |
| contained_permission_ids:options.data.contained_permission_ids, | |
| }, | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/backend/src/lib/permissions.tsx around lines 337 to 376, the currentlogic will allow an existing permission with a different scope to be updated tooptions.scope, which can silently flip scopes; add an explicit guard thatcompares existingPermission.scope to options.scope and throw a clear error (orreturn a failed Promise) if they differ so the function fails fast instead ofcalling updatePermissionDefinition, ensuring no cross-scope updates occur.
Uh oh!
There was an error while loading.Please reload this page.
Problem
Fixes#868. The problem was that database seeding fails with
Permission "team_member" not founderror in certain edge cases. This happens when the internal project record exists in the database but theteam_memberandteam_adminpermissions are missing from the project configuration. The seed script then skips permission creation (since the project exists) but still attempts to update the missing permissions, causing the error.I'm not sure, but I guess this edge case can occur during Docker re-deployments with persistent databases, interrupted setup processes, or maybe database corruption/version mismatches.
Solution
I added a new
ensurePermissionDefinition()function inpermissions.tsxthat safely handles both scenarios. If the permission exists, it updates it using the existingupdatePermissionDefinition()function. If the permission doesn't exist, it creates it using the existingcreatePermissionDefinition()function.The seed script
seed.tsnow uses this new function for bothteam_memberandteam_adminpermissions instead of always trying to update them.I thought of adding
ensurePermissionDefinition()as a helper function inseed.tsitself, since it has no usage anywhere else, but ultimately put it inpermissions.tsx, to keep all permission handling functions in one place. (Let me know if you'd like it otherwise)High-level PR Summary
This PR fixes issue#868 where database seeding fails with a "Permission not found" error when the internal project exists but required permissions are missing. It introduces a new
ensurePermissionDefinition()function that safely handles both creating and updating permissions as needed. The seed script now uses this function instead of always attempting to update permissions, preventing failures in edge cases where permissions don't exist yet.⏱️ Estimated Review Time: 5-15 minutes
💡 Review Order Suggestion
apps/backend/src/lib/permissions.tsxapps/backend/prisma/seed.tsImportant
Fixes database seeding error by adding
ensurePermissionDefinition()to handle missing permissions inseed.ts.seed.ts.ensurePermissionDefinition()inpermissions.tsxto create or update permissions as needed.updatePermissionDefinition()withensurePermissionDefinition()forteam_memberandteam_adminpermissions inseed.ts.ensurePermissionDefinition()inpermissions.tsxto check and handle permission existence.updatePermissionDefinition()andcreatePermissionDefinition()withinensurePermissionDefinition().seed.tsto use the new function for permission handling.This description was created by
for8d6b9fa. You cancustomize this summary. It will automatically update as commits are pushed.
Review by RecurseML
🔍 Review performed on7a0bf86..8d6b9fa
✅ Files analyzed, no issues (1)
•
apps/backend/src/lib/permissions.tsxSummary by CodeRabbit