Bugfix/artifact saving tool ids #1909
Conversation
* few fixes for GenericPromptEditor/GenericJsonEditor * Add changeset for GenericPromptEditor/GenericJsonEditor layout fixes Co-authored-by: Dimitri POSTOLOV <[email protected]> * Update generic-prompt-editor.tsx --------- Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Dimitri POSTOLOV <[email protected]>
…aces (#1903) * local time for traces, end date when agent is done running * changeset * lint and claude comment * remove unneeded state
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 259a9ad 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
9 Key Findings | Risk: High
🔴❗ Critical (1) ❗🔴
🔴 1) agents-api/src/domains/manage/routes/projectFull.ts:131-189 SpiceDB transaction pattern doesn't achieve true distributed atomicity
Issue: The code places SpiceDB sync inside nested database transactions with comments claiming "If this fails, both transactions will rollback automatically." This is architecturally misleading. SpiceDB is an external gRPC service that does not participate in PostgreSQL/Doltgres transactions. The transactions only roll back if an exception is thrown before the transaction callbacks complete—not if commit itself fails.
Why: If SpiceDB succeeds but then the outer transaction commit fails (e.g., network timeout during commit), SpiceDB will have authorization relationships for a project that doesn't exist in the database. This creates a permanent inconsistent state that's difficult to detect and reconcile. The reverse (DB commits, SpiceDB fails) is handled, but this failure mode is not.
Additionally, the nested configDb.transaction() inside runDbClient.transaction() commits independently—if the inner Doltgres commit succeeds but the outer Postgres commit fails, you have config without metadata.
Fix: Consider one of these approaches:
- Move SpiceDB sync outside transactions with best-effort semantics (matching existing
projects.tspattern at lines 257-266), accepting eventual consistency - Implement compensating actions — if DB commit fails after SpiceDB sync succeeds, attempt SpiceDB cleanup before re-throwing
- Use an outbox pattern where SpiceDB sync is driven by a separate worker reading committed database events
At minimum, update the misleading comments to accurately describe the actual guarantees and failure modes.
Refs:
🟠⚠️ Major (4) 🟠⚠️
🟠 1) agents-api/src/domains/manage/routes/projectFull.ts:203-206 Fragile gRPC error detection using duck typing
Issue: The error detection logic (error?.metadata !== undefined && typeof error?.code === 'number') relies on the internal structure of gRPC errors which is not part of the public API contract. The fallback error?.message?.includes('SpiceDB') is also unreliable since SpiceDB errors typically contain gRPC status messages like "UNAVAILABLE" or "PERMISSION_DENIED", not the string "SpiceDB".
Why: This heuristic could miss real SpiceDB failures (false negatives) or incorrectly classify unrelated errors (false positives). If the grpc-js library changes its error structure, this could silently break.
Fix: Consider extracting to a shared utility with more robust detection:
// In agents-core/src/auth/authz/errors.ts
export function isSpiceDbError(error: unknown): boolean {
if (!error || typeof error !== 'object') return false;
const err = error as Record<string, unknown>;
// Check for gRPC error shape with common status codes
const GRPC_UNAVAILABLE = 14;
const GRPC_INTERNAL = 13;
return (err.metadata !== undefined &&
typeof err.code === 'number' &&
[GRPC_UNAVAILABLE, GRPC_INTERNAL].includes(err.code as number)) ||
(typeof err.message === 'string' && err.message.includes('SpiceDB'));
}Refs:
🟠 2) agents-api/src/domains/manage/routes/projectFull.ts Inconsistent SpiceDB sync patterns across project creation routes
Issue: This PR introduces a new pattern where SpiceDB failures cause the entire operation to fail with a 500 error. However, the existing projects.ts route (lines 257-266) uses fire-and-forget: SpiceDB failures are logged as warnings but don't fail the request.
Why: Same conceptual operation (project creation) has different failure modes depending on which endpoint is used. This creates unpredictable behavior for API consumers and split-brain risk where one route can leave orphaned projects without SpiceDB relationships while the other claims to roll back.
Fix: Establish a single pattern for SpiceDB sync during project creation. Either update projects.ts to match the new stricter approach, or revert projectFull.ts to use the existing fire-and-forget pattern. Document the chosen consistency model.
Refs:
🟠 3) agents-api/src/domains/manage/routes/projectFull.ts:574-593 Delete operation silently swallows SpiceDB cleanup failures
Issue: After successfully deleting project data from both databases, if removeProjectFromSpiceDb fails, the operation returns 204 success while authorization relationships remain orphaned in SpiceDB. The comment acknowledges this but dismisses it as not affecting functionality.
Why: This asymmetry with create (which rolls back on SpiceDB failure) is inconsistent. Orphaned auth data accumulates over time, and users have no indication of partial failure. If a project is recreated with the same ID, it may inherit unexpected permission states.
Fix: At minimum, upgrade the log level to error to trigger operational alerts:
logger.error(
{
spiceDbError,
tenantId,
projectId,
orphanedRelationships: true, // Structured field for alerting
},
'CRITICAL: Failed to remove project from SpiceDB - orphaned auth relationships require manual cleanup'
);Consider implementing a background reconciliation job to clean up orphaned SpiceDB relationships.
Refs:
🟠 4) agents-api/src/domains/run/agents/Agent.ts:2437 Missing test coverage for tool call ID propagation
Issue: The enhanceToolResultWithStructureHints method was modified to accept and include toolCallId, but there are no unit tests for this critical functionality. The method conditionally adds _toolCallId to enhanced results when artifact components are available.
Why: If the toolCallId parameter is not correctly propagated, artifacts will fail to be created properly since they require the exact tool call ID. A regression could cause silent failures in the artifact creation workflow.
Fix: Add tests covering:
- When
toolCallIdis provided and artifact components exist,_toolCallIdappears in enhanced result - When
toolCallIdis undefined,_toolCallIdis not added - When no artifact components exist, neither
_toolCallIdnor_structureHintsare added
Refs:
// Findings posted as inline comments:
- 🟠 Major:
artifact-component-schema.ts:166Overly broadas anytype assertion
🟡 Minor (3) 🟡
🟡 1) agents-api/src/__tests__/manage/integration/projectFull.test.ts:588 Test removed without explanation
Issue: The test "should handle projects with empty IDs" was removed. This test documented previous behavior (empty IDs were accepted). The removal suggests this is now disallowed, but there's no corresponding validation change visible in the PR.
Why: Without clarification, it's unclear whether empty project IDs are now rejected or if this was just test cleanup. If the behavior changed, a test for the rejection case should be added.
Fix: Clarify whether empty project IDs are now rejected. If so, add a test for the rejection case. If not, restore the test documenting the accepted behavior.
Refs:
🟡 2) agents-api/src/domains/run/agents/Agent.ts:2648 Naming inconsistency between tool result and artifact schema
Issue: The PR introduces _toolCallId (underscore prefix, camelCase) in enhanced tool results, but artifact component schemas use tool_call_id (snake_case). The underscore prefix convention typically signals "internal/don't touch" but the guidance text explicitly tells LLMs to use this field.
Why: This vocabulary drift between the tool result surface and the artifact creation surface may confuse developers and LLMs who need to connect the two.
Fix: Consider aligning on tool_call_id (matching artifact schemas and OpenAI convention) or document why the naming difference exists.
Refs:
// Findings posted as inline comments:
- 🟡 Minor:
projectFull.test.ts:18UnusedsyncOrgMemberToSpiceDbmock - 🟡 Minor:
SchemaProcessor.ts:240-241Missing documentation for intentional required array overwrite
💭 Consider (3) 💭
💭 1) agents-api/src/domains/run/utils/SchemaProcessor.ts:231 Schema normalization doesn't handle all JSON Schema constructs
Issue: makeAllPropertiesRequired doesn't handle $ref, additionalProperties, or definitions/$defs blocks.
Why: Future schemas using these constructs would not be fully normalized.
Fix: Document that these constructs are not supported, or add handling if needed.
💭 2) agents-api/src/domains/run/agents/Agent.ts:2649-2670 Verbose artifact guidance may consume excessive context tokens
Issue: The _structureHints.artifactGuidance object is included in every tool result and contains extensive guidance text.
Why: This accumulates across multiple tool calls in a conversation.
Fix: Consider moving artifact guidance to the system prompt where it would be stated once.
💭 3) .changeset/afraid-crabs-cheat.md Changeset message could be more descriptive
Issue: "Added tool call id to tool call results" is accurate but doesn't explain when or why.
Fix: Consider: "Add _toolCallId field to tool results when artifact components are enabled, providing the exact tool call ID for artifact creation"
🚫 REQUEST CHANGES
Summary: The core functionality (schema normalization for cross-provider compatibility, tool call ID propagation) appears sound, but the SpiceDB transaction pattern has a critical architectural flaw—it claims atomicity guarantees it cannot actually provide. This could lead to inconsistent state between the database and authorization system that's difficult to detect and reconcile. I recommend either fixing the transaction pattern to accurately reflect its guarantees, or adopting the existing fire-and-forget pattern from projects.ts with explicit documentation of the consistency model.
Discarded (12)
| Location | Issue | Reason Discarded |
|---|---|---|
Agent.ts:2648 |
Tool result object shape extended | Additive change, non-breaking |
SchemaProcessor.ts |
Schema normalization changes behavior | Intentional for cross-provider compatibility, documented |
projectFull.ts:178 |
SpiceDB sync skipped without userId | Matches existing pattern, would need broader auth refactor |
projectFull.ts:496-499 |
Error messages may expose details | Generic error handling, not specific to this PR |
Agent.ts:2659 |
Duplicate toolCallId guidance | Low confidence, may be helpful redundancy |
SchemaProcessor.ts |
Return type preserves nullable | Pragmatic approach, assertions are localized |
artifact-component-schema.ts:109 |
Type assertion without validation | Safe in context, input is known JSONSchema |
Agent.ts:3505 |
Database props trusted as JSONSchema | Boundary where runtime data is trusted, not new |
SchemaProcessor.test.ts:22 |
Test name slightly misleading | Test passes, behavior is correct |
| Docs | Documentation updates needed | No customer-facing contract changes requiring docs |
projectFull.ts:134-189 |
Cross-database transaction isolation | Covered by Critical finding #1 |
Agent.ts:1009 |
MCP tool toolCallId propagation | Implementation is correct, informational only |
Reviewers (12)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
4 | 1 | 0 | 0 | 0 | 0 | 3 |
pr-review-architecture |
4 | 1 | 0 | 0 | 0 | 0 | 3 |
pr-review-sre |
5 | 2 | 0 | 0 | 0 | 0 | 3 |
pr-review-security-iam |
4 | 1 | 0 | 0 | 0 | 0 | 3 |
pr-review-consistency |
7 | 1 | 0 | 0 | 0 | 0 | 6 |
pr-review-errors |
4 | 1 | 0 | 0 | 0 | 0 | 3 |
pr-review-tests |
4 | 1 | 0 | 0 | 1 | 0 | 2 |
pr-review-types |
4 | 0 | 0 | 0 | 1 | 0 | 3 |
pr-review-product |
4 | 1 | 1 | 0 | 0 | 0 | 2 |
pr-review-llm |
4 | 0 | 2 | 0 | 0 | 0 | 2 |
pr-review-breaking-changes |
5 | 0 | 0 | 0 | 0 | 0 | 5 |
pr-review-docs |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 49 | 9 | 3 | 0 | 3 | 0 | 35 |
| name: `ArtifactCreate_${component.name}`, | ||
| description: `Create ${component.name} artifacts from tool results by extracting structured data using selectors.`, | ||
| props: propsSchema, | ||
| props: normalizedPropsSchema as any, |
There was a problem hiding this comment.
🟠 MAJOR: Overly broad type assertion
Issue: Using as any completely bypasses TypeScript's type checking. The DataComponentInsert.props field expects Record<string, unknown> | null | undefined, and normalizedPropsSchema is JSONSchema.BaseSchema | Record<string, unknown> | null | undefined. The as any hides this type relationship.
Why: If DataComponentInsert.props changes to a more specific type in the future, this assertion would silently suppress the error rather than catching the incompatibility at compile time.
Fix: Use a narrower assertion that documents the actual type contract:
| props: normalizedPropsSchema as any, | |
| props: normalizedPropsSchema as Record<string, unknown>, |
Refs:
| syncProjectToSpiceDb: vi.fn().mockResolvedValue(undefined), | ||
| removeProjectFromSpiceDb: vi.fn().mockResolvedValue(undefined), | ||
| syncOrgMemberToSpiceDb: vi.fn().mockResolvedValue(undefined), | ||
| }; |
There was a problem hiding this comment.
🟡 MINOR: Unused mock function
Issue: syncOrgMemberToSpiceDb is mocked but not used in the projectFull routes being tested. This appears to be copy-pasted from another test file.
Why: Unused mocks add confusion about what the test is actually covering and may mislead future maintainers.
Fix:
| }; | |
| vi.mock('@inkeep/agents-core', async (importOriginal) => { | |
| const actual = await importOriginal<typeof import('@inkeep/agents-core')>(); | |
| return { | |
| ...actual, | |
| syncProjectToSpiceDb: vi.fn().mockResolvedValue(undefined), | |
| removeProjectFromSpiceDb: vi.fn().mockResolvedValue(undefined), | |
| }; | |
| }); |
Refs:
| if (normalized.properties && typeof normalized.properties === 'object') { | ||
| normalized.required = Object.keys(normalized.properties); |
There was a problem hiding this comment.
🟡 MINOR: Missing documentation for intentional behavior
Issue: This line overwrites any existing required array with all property keys. The test at line 22-35 is named "should preserve existing required array" but the actual behavior is to replace it entirely. This could be confusing for future maintainers.
Why: JSON Schema allows required to list properties not in properties for inheritance scenarios. If this intentional overwrite is the desired behavior, it should be documented.
Fix: Add a clarifying comment:
| if (normalized.properties && typeof normalized.properties === 'object') { | |
| normalized.required = Object.keys(normalized.properties); | |
| if (normalized.properties && typeof normalized.properties === 'object') { | |
| // Intentionally overwrites existing required array - all properties become required | |
| // for OpenAI/Azure structured output compatibility | |
| normalized.required = Object.keys(normalized.properties); |
Refs:
There was a problem hiding this comment.
PR Review Summary
0 Key Findings | Risk: Low | Delta Review
This is a re-review scoped to the 4 commits since the last automated review. The delta consists entirely of a merge from main that brought in PR #1910's MCP node UI improvements—these changes are unrelated to the core artifact/tool ID bugfix being addressed in this PR.
Delta Changes Reviewed
The delta includes:
- MCP duplicate connection validation — Prevents users from connecting multiple agents to the same MCP node, with clear toast error feedback
- "No tools" badge UX — Changed from displaying "0" to "No tools" for better clarity
- Parameter cleanup — Removed unused
_edgesparameters fromorphaned-tools-detector.tsutility functions - Type safety improvements — Changed from
(node.data as any).relationshipIdto destructuring
All delta changes are clean and follow good patterns. The MCP connection validation correctly enforces the 1:1 relationship between MCP nodes and sub-agents.
💭 Consider (1) 💭
- 💭 Consider:
orphaned-tools-detector.ts:33Inconsistent type access pattern (direct check vsas anyreturn)
🕐 Pending Recommendations (P)
The following issues from the prior review remain unresolved and are not addressed by this delta:
- 🔴
projectFull.ts:131-189SpiceDB transaction pattern doesn't achieve true distributed atomicity - 🟠
projectFull.ts:203-206Fragile gRPC error detection using duck typing - 🟠
projectFull.tsInconsistent SpiceDB sync patterns across project creation routes - 🟠
projectFull.ts:574-593Delete operation silently swallows SpiceDB cleanup failures - 🟠
Agent.ts:2437Missing test coverage for tool call ID propagation - 🟠
artifact-component-schema.ts:166Overly broadas anytype assertion - 🟡
projectFull.test.ts:588Test removed without explanation - 🟡
Agent.ts:2648Naming inconsistency between tool result and artifact schema - 🟡
projectFull.test.ts:18UnusedsyncOrgMemberToSpiceDbmock - 🟡
SchemaProcessor.ts:240-241Missing documentation for intentional required array overwrite
💡 APPROVE WITH SUGGESTIONS
Summary: The delta changes (PR #1910 merge) are clean UX improvements and code cleanup. No issues introduced. However, the prior review's critical finding about SpiceDB transaction semantics and other major/minor issues remain unaddressed in the core PR code. Recommend addressing the pending recommendations before merging.
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
orphaned-tools-detector.ts |
_edges not removed from sibling utilities |
Pre-existing inconsistency not introduced by this PR; separate cleanup scope |
page.client.tsx:567-570 |
Duplicate connection check prevents reconnection | Working as intended; 1:1 relationship correctly enforced |
mcp-node.tsx |
"No tools" text ambiguity | LOW confidence; loading/unavailable states handled separately |
mcp-node.tsx:63 |
'use memo' directive |
Positive observation, not an issue |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-frontend |
4 | 0 | 0 | 0 | 0 | 0 | 4 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
2 | 0 | 1 | 0 | 1 | 0 | 0 |
| Total | 6 | 0 | 1 | 0 | 1 | 0 | 4 |
Note: Delta scope limited review to 3 frontend-focused reviewers. Core reviewers from prior review remain applicable to unchanged code.
There was a problem hiding this comment.
PR Review Summary
0 Key Findings | Risk: Low | Delta Review
This re-review was triggered by a pull_request:synchronize event. The delta since the last automated review consists entirely of merges from main:
| Commit | PR | Summary |
|---|---|---|
615713fb |
#1921 | Editor fullscreen styles |
c0c922b5 |
#1902 | Azure model selector fixes |
f504ff12 |
#1920 | MCP toast error messages |
5ec6c797 |
#1919 | PR review agent updates |
7cb0d548 |
#1915 | JSON schema normalization |
eaa346bc |
#1914 | create-agents-template upgrades |
9ac403a4 |
#1913 | Pre-push operation reduction |
No code changes to the core PR files (projectFull.ts, Agent.ts, projectFull.test.ts) since the last review. The merged changes do not interact with this PR's functionality.
🕐 Pending Recommendations (P)
The following issues from prior reviews remain unresolved:
- 🔴
projectFull.ts:131-189SpiceDB transaction pattern doesn't achieve true distributed atomicity - 🟠
projectFull.ts:203-206Fragile gRPC error detection using duck typing - 🟠
projectFull.tsInconsistent SpiceDB sync patterns across project creation routes - 🟠
projectFull.ts:574-593Delete operation silently swallows SpiceDB cleanup failures - 🟠
Agent.ts:2437Missing test coverage for tool call ID propagation - 🟠
artifact-component-schema.ts:166Overly broadas anytype assertion (Outdated — file modified in #1915) - 🟡
projectFull.test.ts:588Test removed without explanation - 🟡
Agent.ts:2648Naming inconsistency between tool result and artifact schema - 🟡
projectFull.test.ts:18UnusedsyncOrgMemberToSpiceDbmock
💡 APPROVE WITH SUGGESTIONS
Summary: No new issues introduced by the merge commits. The prior review's critical finding about SpiceDB transaction semantics and other recommendations remain applicable. Recommend addressing the pending recommendations—particularly the Critical item regarding transaction atomicity guarantees—before merging.
Discarded (0)
No new findings to discard.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| None dispatched | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: No reviewers dispatched — delta contained only merge commits with no changes to PR's core files.
There was a problem hiding this comment.
PR Review Summary
2 Key Findings | Risk: Low | Delta Review
This re-review is scoped to commit 259a9adfa ("address spicedb comments") which addresses significant feedback from the initial review. The delta successfully resolves the Critical and several Major findings.
Prior Feedback Resolution
| Prior Finding | Status |
|---|---|
| 🔴 Critical: SpiceDB transaction atomicity | ✅ RESOLVED — SpiceDB sync moved outside transactions with compensating deletes |
| 🟠 Major: gRPC error detection duck typing | ✅ RESOLVED — New SpiceDbError class provides typed error handling |
| 🟠 Major: Inconsistent SpiceDB patterns | ✅ RESOLVED — projects.ts now uses same compensating action pattern |
| 🟠 Major: Delete swallows failures | warn (not error) for cleanup failures |
🟠⚠️ Major (1) 🟠⚠️
- 🟠 Major:
projectFull.ts:201-211Error message claims rollback succeeded when cleanup may fail
🟡 Minor (1) 🟡
- 🟡 Minor:
projects.ts:441-443Inconsistent logging (console.warnvslogger.warn)
💭 Consider (2) 💭
💭 1) packages/agents-core/src/auth/authz/errors.ts SpiceDbError has redundant originalError property
Issue: The class stores the cause in both error.cause (standard ES2022) and error.originalError.
Why: This could lead to inconsistent error inspection patterns.
Fix: Consider removing originalError and documenting to use standard cause, or add a comment explaining why both are needed.
💭 2) packages/agents-core/src/auth/authz/sync.ts SpiceDbError could capture more gRPC context
Issue: Only grpcCode is extracted; gRPC errors often include useful details and metadata fields.
Why: During incidents, distinguishing 'ALREADY_EXISTS' from 'UNAVAILABLE' helps triage.
Fix: Consider extracting grpcDetails in the SpiceDbError constructor.
🕐 Pending Recommendations (P)
Issues from prior reviews that remain applicable:
- 🟠
Agent.ts:2437Missing test coverage for tool call ID propagation (unchanged code) - 🟡
projectFull.test.ts:18Test mock comment clarified but mock retained — acceptable ifsyncOrgMemberToSpiceDbis called via better-auth hooks - 🟡
Agent.ts:2648Naming inconsistency (_toolCallIdvstool_call_id) — unchanged code
💡 APPROVE WITH SUGGESTIONS
Summary: This commit substantially improves the SpiceDB integration by addressing the Critical atomicity issue and Major error handling concerns. The new SpiceDbError class and compensating action pattern are well-designed. The remaining issues are minor: (1) the error message could better distinguish between successful and failed cleanup, and (2) one route still uses console.warn instead of structured logging. These are polish items that don't block the PR. The prior test coverage and naming concerns remain but are outside this delta's scope.
Discarded (6)
| Location | Issue | Reason Discarded |
|---|---|---|
sync.ts:21-44 |
syncOrgMemberToSpiceDb not wrapped in SpiceDbError |
Outside delta scope — function not modified by this commit |
projectFull.ts:193-206 |
Compensating deletes not in transaction | Acceptable given deny-by-default — cleanup failures are logged and don't grant unauthorized access |
projectFull.ts:178-179 |
SpiceDB deny-by-default assumption | Positive validation — assumption is correct per schema analysis |
projects.ts:280-285 |
Compensating delete cleanup failure | Merged with Major finding about misleading error messages |
projectFull.ts:474-479 |
PUT route same misleading error | Merged with Major finding — same pattern, noted in inline comment |
sync.ts:185-190 |
SpiceDbError error context | Covered in Consider section |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-sre |
4 | 0 | 1 | 0 | 1 | 0 | 2 |
pr-review-errors |
6 | 1 | 1 | 0 | 1 | 0 | 3 |
pr-review-security-iam |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
| Total | 13 | 1 | 2 | 0 | 2 | 0 | 8 |
Note: Most SRE and security-iam findings were merged with pr-review-errors findings covering the same issues.
| } catch (cleanupError) { | ||
| logger.error( | ||
| { error: cleanupError, tenantId, projectId: validatedProjectData.id }, | ||
| 'Failed to clean up project after SpiceDB sync failure - manual intervention required' | ||
| ); | ||
| } | ||
| throw createApiError({ | ||
| code: 'internal_server_error', | ||
| message: | ||
| 'Failed to set up project authorization. Project creation has been rolled back.', | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR: Error message claims rollback succeeded when cleanup may have failed
Issue: When SpiceDB sync fails and the compensating delete also fails (lines 201-206), the code still throws an error claiming "Project creation has been rolled back" (line 209-211). This is misleading — the project remains in the database but is inaccessible due to SpiceDB's deny-by-default.
Why: Users receive feedback that doesn't match system state. The project exists but cannot be accessed, edited, or deleted by any user. Only operators reviewing logs would know manual intervention is required. Debugging this situation months later requires correlating logs with database state.
Fix: Consider distinguishing between successful and failed cleanup in the error message:
} catch (cleanupError) {
logger.error(
{ error: cleanupError, tenantId, projectId: validatedProjectData.id },
'Failed to clean up project after SpiceDB sync failure - manual intervention required'
);
// Throw a more accurate error
throw createApiError({
code: 'internal_server_error',
message: `Failed to set up project authorization. Cleanup also failed - please contact support. Project ID: ${validatedProjectData.id}`,
});
}Refs:
- Same pattern at projectFull.ts:474-479 (PUT route)
- Same pattern at projects.ts:280-285
|
I addressed the suggestions for the spicedb-related changes. I think we can merge this |
5 participants
No description provided.