feat: Add SpiceDB authorization sync for project lifecycle#1916
feat: Add SpiceDB authorization sync for project lifecycle#1916omar-inkeep merged 3 commits intomainfrom
Conversation
Wrap project create operations in two-phase commit with SpiceDB sync so authorization relationships are created atomically with DB records. On delete, clean up SpiceDB relationships with graceful fallback. Mock SpiceDB functions in integration tests. Co-authored-by: Cursor <cursoragent@cursor.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: 8c2eeb6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
6 Key Findings | Risk: Medium
🟠⚠️ Major (3) 🟠⚠️
🟠 1) projectFull.ts:132-189 "Two-phase commit" naming overstates consistency guarantees
Issue: The code comments claim "two-phase commit" where "If SpiceDB sync fails, both DB transactions will rollback automatically." This is architecturally misleading. Calling an external gRPC service (SpiceDB) within a database transaction does not make the external call part of that transaction. The pattern provides fail-fast semantics (SpiceDB failure before commit prevents DB commit), but not true atomicity.
Why: If SpiceDB sync succeeds but a subsequent operation fails (or the DB commit itself fails for any reason), the SpiceDB relationships remain — they cannot be rolled back. This could leave orphaned authorization state. Additionally, there's no compensation mechanism to clean up SpiceDB if something goes wrong after the syncProjectToSpiceDb call returns successfully.
Fix: Consider either:
- Rename the pattern in comments to clarify actual guarantees (e.g., "fail-fast consistency: SpiceDB failure prevents DB commit")
- Move SpiceDB sync after the transaction commits, with explicit rollback logic (delete the project) if SpiceDB fails
- Accept eventual consistency with a background reconciliation job
Refs:
🟠 2) projectFull.ts:204-206 gRPC error detection heuristic may misclassify errors
Issue: The check error?.metadata !== undefined && typeof error?.code === 'number' is intended to detect gRPC errors from SpiceDB, but this heuristic is fragile. Many JavaScript errors have numeric code properties, and various libraries attach metadata properties. If a non-SpiceDB error matches this pattern, users receive the misleading message about SpiceDB authorization failure.
Why: False positives would show "Failed to set up project authorization" when the actual error is unrelated to SpiceDB. False negatives would let SpiceDB errors slip through to generic error handling, potentially exposing internal error details.
Fix: Consider more specific detection:
// Check for gRPC status code range (0-16) and required properties
const isGrpcError = (
error?.metadata !== undefined &&
typeof error?.code === 'number' &&
error?.code >= 0 &&
error?.code <= 16 &&
typeof error?.details === 'string'
);Or wrap syncProjectToSpiceDb in its own try-catch to distinguish SpiceDB errors explicitly.
Refs:
🟠 3) projectFull.ts vs projects.ts Inconsistent SpiceDB error handling between sibling routes
Issue: This PR introduces a stricter consistency model (rollback on SpiceDB failure) for projectFull.ts, while the sibling projects.ts route uses graceful fallback (log warning, continue). Both endpoints handle project creation but with different failure semantics.
Why: A customer using POST /manage/tenants/{id}/projects gets different behavior than one using POST /manage/tenants/{id}/project-full. With the former, SpiceDB failures are logged but creation succeeds. With the latter, SpiceDB failures roll back the entire operation. This behavioral divergence creates confusing API semantics.
Fix: Either:
- Align
projectFull.tsto use the same graceful fallback asprojects.ts - Update
projects.tsto use the new stricter pattern - If divergence is intentional, document why the two routes have different failure semantics
Refs:
// Findings posted as inline comments:
- 🟠 Major:
projectFull.ts:473Variable shadowing — isCreate redeclared
🟡 Minor (2) 🟡
🟡 1) projectFull.test.ts:9-19 No test coverage for SpiceDB failure scenarios
Issue: The integration tests mock all SpiceDB functions to return success. The critical error handling logic (gRPC error detection, transaction rollback) has no test coverage.
Why: If the rollback-on-failure logic has bugs (e.g., SpiceDB call executes outside transaction scope), it won't be caught by tests. The PR's core consistency guarantee is untested.
Fix: Add at least one test that configures the mock to throw and verifies DB rollback:
it('should rollback DB when SpiceDB sync fails during create', async () => {
vi.mocked(syncProjectToSpiceDb).mockRejectedValueOnce(
Object.assign(new Error('SpiceDB connection failed'), { metadata: {}, code: 14 })
);
// ... verify 500 response and project NOT in DB
});Refs:
🟡 2) projectFull.ts:176-184 External service call inside open transaction extends lock duration
Issue: The SpiceDB gRPC call occurs while two database transactions are held open. SpiceDB involves network I/O with variable latency (DNS, TLS, potential retries). During this time, both databases hold locks, increasing contention under load.
Why: If SpiceDB is slow or times out, both databases are blocked. This is the "pessimistic lock held across async boundary" anti-pattern that can cause cascading contention.
Fix: Consider moving SpiceDB sync after the transactions commit. Pattern:
const project = await runDbClient.transaction(...);
await syncProjectToSpiceDb(...); // After commit
return project;If SpiceDB then fails, implement explicit rollback logic (delete the just-created project).
Refs:
// Findings posted as inline comments:
- 🟡 Minor:
projectFull.ts:178Silent skip when userId is falsy - 🟡 Minor:
projectFull.ts:582-593Standardize error key in log
💭 Consider (2) 💭
💭 1) projectFull.ts:179-183 No timeout or retry for SpiceDB calls
Issue: SpiceDB gRPC calls lack explicit timeout configuration and retry logic for transient failures.
Why: Could hang indefinitely or fail on brief network blips.
Fix: Configure gRPC deadline (5-10s) and consider 2-3 retries with exponential backoff for creates.
💭 2) sync.ts SpiceDB sync for API key/system users may create orphaned relationships
Issue: When userId is apikey:... or system, the sync creates relationships for non-existent SpiceDB users.
Why: Could pollute SpiceDB with invalid relationships.
Fix: Add explicit handling: skip SpiceDB sync entirely for non-user principals, or only create the project-to-org link.
💡 APPROVE WITH SUGGESTIONS
Summary: This PR adds valuable SpiceDB authorization sync for project lifecycle, addressing a real gap in the authorization system. The core implementation is solid. The main concerns are: (1) the "two-phase commit" naming overstates the consistency guarantees — clarifying the actual semantics would help future maintainers, (2) the gRPC error detection heuristic could be more robust, and (3) the behavioral divergence from the sibling projects.ts route should be intentional and documented. None of these are blocking, but addressing them would strengthen the implementation.
Discarded (5)
| Location | Issue | Reason Discarded |
|---|---|---|
projectFull.ts:574-593 |
Asymmetric consistency (DELETE graceful, CREATE strict) | Intentional design choice documented in code comments |
client.ts:21-33 |
SpiceDB client lacks health monitoring | Pre-existing — not introduced by this PR |
projectFull.ts:207-215 |
Missing correlation ID in logs | Valid but very minor — logger may already include request context |
projectFull.test.ts |
Test mock completely bypasses SpiceDB | Expected behavior for API/DB layer tests; documented in PR |
projects.test.ts |
Sibling test doesn't mock SpiceDB | Pre-existing — consequence of existing graceful fallback pattern |
Reviewers (7)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-architecture |
5 | 2 | 0 | 0 | 0 | 0 | 3 |
pr-review-security-iam |
4 | 0 | 1 | 0 | 1 | 0 | 2 |
pr-review-sre |
6 | 0 | 1 | 0 | 0 | 0 | 5 |
pr-review-errors |
5 | 1 | 0 | 0 | 1 | 0 | 3 |
pr-review-standards |
3 | 1 | 0 | 0 | 1 | 0 | 1 |
pr-review-tests |
3 | 1 | 0 | 0 | 0 | 0 | 2 |
pr-review-consistency |
4 | 1 | 0 | 0 | 0 | 0 | 3 |
| Total | 30 | 6 | 2 | 0 | 3 | 0 | 19 |
| } | ||
|
|
||
| // Handle SpiceDB sync failures for creates - transactions already rolled back | ||
| const isCreate = c.get('isProjectCreate') ?? false; |
There was a problem hiding this comment.
🟠 MAJOR Variable shadowing: isCreate redeclared in catch block
Issue: The variable isCreate is already declared at line 405 and is redeclared identically here in the catch block.
Why: While functionally harmless (same value), this duplication is unnecessary and could confuse maintainers if the logic diverges in the future.
Fix: (1-click apply)
| const isCreate = c.get('isProjectCreate') ?? false; | |
| // Handle SpiceDB sync failures for creates - transactions already rolled back | |
| if (c.get('isProjectCreate')) { |
Refs:
|
|
||
| // Phase 2: Sync to SpiceDB (still within transaction scope) | ||
| // If this fails, both transactions will rollback automatically | ||
| if (userId) { |
There was a problem hiding this comment.
🟡 MINOR Silent skip when userId is falsy — no logging
Issue: When userId is not set, the SpiceDB sync is silently skipped without any log entry.
Why: If a project is created in a code path where userId is undefined (e.g., certain test scenarios or future code changes), the project will exist but have no authorization relationships. The silent skip makes this difficult to debug.
Fix: Add explicit logging:
if (userId) {
await syncProjectToSpiceDb({
tenantId,
projectId: validatedProjectData.id,
creatorUserId: userId,
});
} else {
logger.warn(
{ tenantId, projectId: validatedProjectData.id },
'Skipping SpiceDB sync: no userId available - project may have no explicit owner'
);
}Refs:
- Same pattern at line 440 in PUT handler
| } catch (spiceDbError) { | ||
| // Log but don't fail - the project data is already deleted | ||
| // This could leave orphaned auth relationships, but won't affect functionality | ||
| logger.warn( | ||
| { | ||
| spiceDbError, | ||
| tenantId, | ||
| projectId, | ||
| }, | ||
| 'Failed to remove project from SpiceDB - orphaned auth relationships may remain' | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 MINOR Standardize error key and add remediation guidance
Issue: The SpiceDB error is logged as spiceDbError instead of the conventional error key, which may cause inconsistent log parsing.
Why: Log aggregation tools that key on error may not detect this failure. Operations teams also need guidance on remediation.
Fix:
| } catch (spiceDbError) { | |
| // Log but don't fail - the project data is already deleted | |
| // This could leave orphaned auth relationships, but won't affect functionality | |
| logger.warn( | |
| { | |
| spiceDbError, | |
| tenantId, | |
| projectId, | |
| }, | |
| 'Failed to remove project from SpiceDB - orphaned auth relationships may remain' | |
| ); | |
| } | |
| } catch (spiceDbError) { | |
| // Log but don't fail - the project data is already deleted | |
| // This could leave orphaned auth relationships, but won't affect functionality | |
| logger.warn( | |
| { | |
| error: spiceDbError, | |
| tenantId, | |
| projectId, | |
| }, | |
| 'Failed to remove project from SpiceDB - orphaned auth relationships may remain. Manual cleanup may be required if this persists.' | |
| ); | |
| } |
Refs:
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: High
🔴❗ Critical (1) ❗🔴
Inline Comments:
- 🔴 Critical:
projectFull.ts:415-431DuplicatecreateProjectMetadataAndBranchcall — merge artifact causing authorization gap
🟡 Minor (0) 🟡
No new Minor findings in this delta review.
💭 Consider (3) 💭
💭 1) projectFull.ts:542-547, 565-571, 588-593 Promise.allSettled results not aggregated for observability
Issue: The scheduled trigger reconciliation uses Promise.allSettled but doesn't examine or log the aggregate results. Individual failures are logged via .catch() handlers, but there's no summary of how many operations succeeded/failed.
Why: In high-failure scenarios, operators would need to correlate multiple log entries to understand the scope of the problem.
Fix: Consider logging an aggregate summary:
const results = await Promise.allSettled(workflowOperations);
const failures = results.filter(r => r.status === 'rejected');
if (failures.length > 0) {
logger.warn(
{ tenantId, projectId, agentId, failedCount: failures.length, totalCount: results.length },
'Some scheduled trigger workflow operations failed during reconciliation'
);
}💭 2) projectFull.ts:606-611 Outer try-catch lacks agent context in error log
Issue: The catch block logs err, tenantId, and projectId but not which agent was being processed when the failure occurred.
Why: Debugging multi-agent project updates is difficult when failures occur at the agent iteration level.
Fix: Add agent context: agentIds: Object.keys(validatedProjectData.agents || {})
💭 3) projectFull.ts:503-599 Trigger reconciliation pattern diverges from agentFull.ts sibling
Issue: This code uses Promise.all with .map() and Promise.allSettled, while agentFull.ts uses sequential for...of loops with try/catch per operation.
Why: The divergence creates maintenance burden and inconsistent error semantics.
Fix: Consider aligning patterns or documenting the intentional divergence (parallel execution for performance).
🕐 Pending Recommendations (3)
Prior feedback from automated review — still unresolved:
- 🟠
projectFull.ts:630Variable shadowing:isCreateredeclared in catch block - 🟡
projectFull.ts:185Silent skip whenuserIdis falsy — no logging - 🟡
projectFull.ts:739-750SpiceDB error logged asspiceDbErrorinstead oferror
🚫 REQUEST CHANGES
Summary: The merge from main introduced a critical bug: duplicate createProjectMetadataAndBranch calls in the PUT handler that cause project creation to fail with an authorization gap (project exists in DB but has no SpiceDB permissions). This must be fixed before merge. The duplicate block at lines 415-431 should be removed entirely — the transactional path at lines 447-488 is the correct implementation. Additionally, the 3 prior review items remain unaddressed.
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
client.ts:21-33 |
SpiceDB client lacks timeout/retry configuration | Pre-existing — not introduced by this PR |
projectFull.ts:141-196 |
External gRPC call inside transaction extends lock duration | Already raised in prior review (Major #3) |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-security-iam |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-sre |
4 | 0 | 0 | 0 | 1 | 0 | 2 |
pr-review-errors |
2 | 0 | 2 | 0 | 0 | 0 | 0 |
pr-review-consistency |
2 | 0 | 1 | 0 | 1 | 0 | 0 |
| Total | 10 | 0 | 3 | 0 | 1 | 3 | 2 |
Note: All 5 reviewers independently identified the duplicate createProjectMetadataAndBranch call as a critical issue — merged into single inline comment.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This is a delta review following the previous REQUEST_CHANGES review. The commit 8c2eeb63 ("address comments") correctly addresses all prior feedback.
✅ Prior Feedback Verified as Addressed
| Prior Issue | Status |
|---|---|
🔴 Critical: Duplicate createProjectMetadataAndBranch call causing authorization gap |
✅ Fixed — duplicate block removed, single call inside transaction |
🟠 Major: Variable shadowing — isCreate redeclared in catch block |
✅ Fixed — now declared once at line 404 |
| 🟠 Major: "Two-phase commit" naming overstated consistency guarantees | ✅ Fixed — comments now accurately describe fail-fast semantics |
🟡 Minor: Silent skip when userId is falsy — no logging |
✅ Fixed — logger.warn added at lines 192-196 and 470-471 |
🟡 Minor: SpiceDB error logged as spiceDbError instead of error |
✅ Fixed — standardized to error at line 726 |
Security Assessment
The userId skip behavior (when falsy) was verified as correct:
systemandapikey:...principals bypass SpiceDB permission checks viarequirePermission.ts- Skipping SpiceDB sync for these principals is semantically correct
- The added warning log provides appropriate observability
✅ APPROVE
Summary: All critical and major issues from the prior review have been correctly addressed. The delta is clean — no new issues introduced. The SpiceDB authorization sync for project lifecycle is ready for merge. 🎉
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Both reviewers verified all prior feedback was correctly addressed; no new findings.
3 participants
Summary
Split from #1836 — this PR contains only the SpiceDB/authz synchronization work.
Test plan
Made with Cursor