Normalize JSON schemas for OpenAI structured output compatibility #1836
Conversation
🦋 Changeset detectedLatest commit: 6a1b44d 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
tim-inkeep
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
3 Key Findings | Risk: Medium
🔴❗ Critical (0) ❗🔴
None identified.
🟠⚠️ Major (2) ⚠️ 🟠
🟠 1) SchemaProcessor.ts:231-270 Missing unit tests for new normalizeForStructuredOutput method
Issue: The new normalizeForStructuredOutput() method contains non-trivial recursive logic that transforms JSON schemas by: (1) making all properties required, (2) recursively normalizing nested object properties, (3) handling array items, and (4) handling union types (anyOf, oneOf, allOf). Per AGENTS.md, all new code paths must have test coverage.
Why: Without unit tests, regressions in schema normalization could cause production failures across all LLM providers. Specific failure modes that would go undetected:
- Deeply nested schemas not being normalized, causing Azure OpenAI structured output API to reject requests with "required is required to be supplied" errors
- Union types (
anyOf/oneOf/allOf) not being properly processed, leaving some branches unnormalized - Array items with object schemas not being normalized
- Null/undefined schema inputs causing runtime exceptions
Fix: Add a dedicated test file agents-api/src/domains/run/utils/__tests__/SchemaProcessor.test.ts with tests covering:
- Basic object normalization
- Nested object properties
- Array items with object schemas
anyOf/oneOf/allOfhandling- Edge cases (null/undefined inputs, schemas without properties)
describe('SchemaProcessor.normalizeForStructuredOutput', () => {
it('should make all properties required in a flat schema', () => {
const schema = {
type: 'object',
properties: { name: { type: 'string' }, age: { type: 'number' } },
};
const result = SchemaProcessor.normalizeForStructuredOutput(schema);
expect(result.required).toEqual(['name', 'age']);
});
it('should recursively normalize nested object properties', () => {
const schema = {
type: 'object',
properties: {
user: {
type: 'object',
properties: { email: { type: 'string' } },
},
},
};
const result = SchemaProcessor.normalizeForStructuredOutput(schema);
expect(result.properties.user.required).toEqual(['email']);
});
it('should handle null/undefined inputs gracefully', () => {
expect(SchemaProcessor.normalizeForStructuredOutput(null)).toBeNull();
expect(SchemaProcessor.normalizeForStructuredOutput(undefined)).toBeUndefined();
});
});Refs: SchemaProcessor.ts:231-270 · AGENTS.md - Testing Requirements
🟠 2) projectFull.ts:168-176, 404-415 SpiceDB sync failure silently swallowed, leaving project in inconsistent authorization state
Issue: The catch blocks catch ALL errors from syncProjectToSpiceDb (network errors, gRPC failures, SpiceDB unavailability, validation errors) and log only a warning without surfacing the failure to the user or API response. This pattern appears in both the POST route (lines 168-176) and PUT route (lines 404-415).
Why: When SpiceDB sync fails, the project is created in the database but lacks proper authorization relationships:
- The project creator may lose admin access to their own project since the
project_adminrole was not granted - The project-to-org relationship is not established, breaking inherited permission checks
- Users will see confusing 403 errors on a project they just created with no indication of why
- Debugging requires correlating warning logs with user-reported permission issues, often hours or days later
Fix: Consider one of these approaches depending on business requirements:
Option A: Make sync failure fail the request (recommended for authorization-critical systems):
if (userId) {
try {
await syncProjectToSpiceDb({
tenantId,
projectId: validatedProjectData.id,
creatorUserId: userId,
});
} catch (syncError) {
logger.error({ syncError, tenantId, projectId: validatedProjectData.id, userId },
'Failed to sync project to SpiceDB - rolling back project creation');
throw createApiError({
code: 'internal_server_error',
message: 'Project created but authorization setup failed. Please try again or contact support.',
});
}
}Option B: Return success with warning if degraded operation is acceptable:
let authzWarning: string | undefined;
if (userId) {
try {
await syncProjectToSpiceDb({ ... });
} catch (syncError) {
logger.error({ syncError, tenantId, projectId, userId },
'SpiceDB sync failed - project created without authorization relationships');
authzWarning = 'Project created but authorization setup failed.';
}
}
return c.json({ data: createdProject, warning: authzWarning }, 201);Refs: projectFull.ts:168-176 · projectFull.ts:404-415
🟡 Minor (1) 🟡
🟡 1) model-context-utils.test.ts:5-10 Test file change unrelated to PR's main functionality
Issue: The test file adds a mock for execution-limits constants, but this appears to be a fix for a pre-existing test issue (likely making tests pass locally) rather than testing the new normalizeForStructuredOutput functionality.
Why: This conflates an unrelated test fix with the main PR changes, making it harder to understand and review the PR scope.
Fix: Consider separating test fixes into their own commit with a clear commit message explaining why the mock is needed.
Refs: model-context-utils.test.ts:5-10
💭 Consider (2) 💭
💭 1) .changeset/twenty-regions-stare.md Changeset does not mention SpiceDB sync changes
Issue + Why: The changeset only mentions "Normalize JSON schemas for OpenAI structured output compatibility" but the PR also includes SpiceDB sync changes for project creation which affect authorization behavior.
Suggestion: Consider updating the changeset to mention both changes, or if SpiceDB sync is internal infrastructure, confirm this is intentional.
💭 2) SchemaProcessor.ts:231 Method uses any types throughout
Issue + Why: The normalizeForStructuredOutput method uses any for both input and output types, bypassing TypeScript's type checking. While this provides flexibility, it allows passing non-schema objects without compile-time errors.
Suggestion: Consider typing with JSONSchema.BaseSchema from zod/v4/core (already imported elsewhere in the codebase) for compile-time validation of schema inputs.
📌 Inline Comments (1)
- 🟡
projectFull.ts:510Missing SpiceDB cleanup on project deletion
🕐 Pending Recommendations (0)
No prior unresolved feedback.
💡 APPROVE WITH SUGGESTIONS
Summary: The schema normalization fix for Azure OpenAI structured output compatibility looks correct and well-implemented. However, two key issues should be addressed:
-
Missing tests — The new
normalizeForStructuredOutputmethod needs unit tests per AGENTS.md requirements. This is critical business logic affecting all structured output operations. -
Silent SpiceDB failures — The error handling pattern of logging-and-continuing for authorization sync failures could lead to confusing user experiences where newly created projects have broken permissions. This warrants discussion on the intended behavior.
Additionally, the DELETE endpoint should include SpiceDB cleanup to match the pattern in projects.ts.
Discarded (6)
| Location | Issue | Reason Discarded |
|---|---|---|
SchemaProcessor.ts:240 |
Normalization overwrites existing required array | Intentional behavior - this is the fix for Azure OpenAI compatibility |
SchemaProcessor.ts:231-271 |
Missing $defs handling |
Follows existing codebase patterns (e.g., makeSchemaPermissive) |
PromptConfig.ts |
Naming similarity with existing normalizeSchema |
Different purposes, not a concern |
SchemaProcessor.ts:254-267 |
Map callbacks use any types |
LOW confidence, runtime handles primitives correctly |
| Breaking changes INFO items | Various | These are informational observations, not actual issues |
artifact-component-schema.ts |
Missing integration tests | SchemaProcessor unit tests would provide adequate coverage |
Reviewers (7)
| Reviewer | Returned | Main Findings | Consider | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|
pr-review-tests |
2 | 1 | 0 | 0 | 0 | 1 |
pr-review-errors |
2 | 1 | 0 | 0 | 0 | 0 |
pr-review-standards |
2 | 0 | 0 | 0 | 0 | 2 |
pr-review-consistency |
5 | 0 | 1 | 0 | 0 | 3 |
pr-review-types |
2 | 0 | 1 | 0 | 0 | 1 |
pr-review-security-iam |
1 | 0 | 0 | 1 | 0 | 0 |
pr-review-breaking-changes |
5 | 0 | 0 | 0 | 0 | 5 |
| Total | 19 | 2 | 2 | 1 | 0 | 12 |
There was a problem hiding this comment.
PR Review Summary
0 Key Findings | Risk: Low
🔴❗ Critical (0) ❗🔴
None identified.
🟠⚠️ Major (0) 🟠⚠️
None identified.
🟡 Minor (0) 🟡
None identified.
💭 Consider (1) 💭
Inline comments:
- 💭 Consider:
SchemaProcessor.test.ts:304Edge case coverage foradditionalProperties/$ref/patternProperties
🕐 Pending Recommendations (1)
From previous reviews:
- 🟠
projectFull.ts:168-176, 404-415SpiceDB sync failure silently swallowed, leaving project in inconsistent authorization state
💡 APPROVE WITH SUGGESTIONS
Summary: The core schema normalization fix for Azure OpenAI structured output compatibility is well-implemented and now has comprehensive test coverage. The previous review's main concern (missing tests) has been fully addressed with 304 lines of thorough tests covering:
- Basic object normalization
- Nested object recursion (multiple levels deep)
- Array items with object schemas
- Union types (
anyOf,oneOf,allOf) - Edge cases (null, undefined, empty properties, primitive schemas)
- Real-world schema examples (fact data components, artifact components)
- Non-mutation verification
The SpiceDB sync pattern follows the established convention in projects.ts and was already flagged in the previous review. Since it's consistent with existing codebase patterns and represents a separate concern from the main fix, it's noted as a pending recommendation rather than a blocker.
Great job addressing the feedback! 🎉
Discarded (8)
| Location | Issue | Reason Discarded |
|---|---|---|
projectFull.ts:174 |
Logger uses logger.warn vs console.warn in projects.ts |
INFO severity; logger.warn is actually the preferred pattern (structured logging) |
SchemaProcessor.ts:235-237 |
Type signature more precise than sibling methods | Not an issue - more precise types are an improvement |
SchemaProcessor.ts:244-253 |
Missing type === 'object' check before accessing properties |
Edge case for malformed schemas; valid JSON schemas always have type with properties |
SchemaProcessor.test.ts:22 |
Test name says "preserve" but implementation replaces | INFO severity; test correctly verifies behavior, naming is cosmetic |
SchemaProcessor.test.ts:227 |
Non-mutation test only verifies shallow mutations | LOW confidence; implementation correctly uses spread at each level |
SchemaProcessor.ts:236-237 |
Return type is overly broad union | LOW confidence; pragmatic for JSON schema manipulation |
SchemaProcessor.ts:249 |
Internal any type for normalizedProperties |
INFO severity; pragmatic for recursive JSON schema processing |
projectFull.ts:404-415 |
Duplicate of POST route SpiceDB issue | Same as pending recommendation - already raised in previous review |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
3 | 0 | 0 | 0 | 0 | 3 |
pr-review-tests |
3 | 0 | 0 | 1 | 0 | 2 |
pr-review-security-iam |
1 | 0 | 0 | 0 | 1 | 0 |
pr-review-errors |
2 | 0 | 0 | 0 | 0 | 2 |
pr-review-types |
2 | 0 | 0 | 0 | 0 | 2 |
| Total | 11 | 0 | 0 | 1 | 1 | 9 |
| ]); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
💭 Consider Edge case coverage for additionalProperties, $ref, and patternProperties
Issue: The test suite comprehensively covers properties, items, anyOf, oneOf, and allOf, but doesn't document behavior for JSON Schema keywords that may also contain nested object schemas: additionalProperties, $ref, not, and patternProperties.
Why: If a data component schema uses additionalProperties: { type: 'object', properties: { ... } }, those nested properties would not be normalized by the current implementation—and the tests don't verify this is intentional. This could cause the same Azure OpenAI "required is required" error for edge-case schemas.
Fix: Consider adding a test case to document expected behavior (either normalization or explicit non-normalization):
describe('unsupported schema keywords (documents limitations)', () => {
it('does not normalize schemas nested in additionalProperties', () => {
const schema = {
type: 'object',
properties: { name: { type: 'string' } },
additionalProperties: {
type: 'object',
properties: { extra: { type: 'string' } },
},
};
const result = SchemaProcessor.normalizeForStructuredOutput(schema);
// Documents current behavior: additionalProperties is NOT normalized
expect(result.additionalProperties.required).toBeUndefined();
});
});Refs:
There was a problem hiding this comment.
PR Review Summary
0 Key Findings | Risk: Low
🔴❗ Critical (0) ❗🔴
None identified.
🟠⚠️ Major (0) 🟠⚠️
None identified.
🟡 Minor (0) 🟡
None identified.
💭 Consider (2) 💭
💭 1) SchemaProcessor.ts:242 Shallow clone may share nested object references
Issue: The normalizeForStructuredOutput implementation uses shallow spread ({ ...schema }) at each level. While the test at line 227-239 verifies that the top-level original schema is not mutated, deeply nested objects share references between the original and normalized schemas.
Why: If code downstream modifies the normalized schema's nested properties, it could inadvertently affect the original schema object. In the current usage (schemas are typically not mutated after normalization), this is unlikely to cause issues.
Suggestion: Either strengthen the non-mutation test to verify deep immutability (and potentially use structuredClone() or similar), or document this as a known limitation. Low priority given current usage patterns.
💭 2) projectFull.ts:184-189 Orphaned project on SpiceDB sync failure
Issue: When SpiceDB sync fails during project creation, the error is thrown (good), but the project already exists in the database. The error message tells users to "delete and try again, or contact support" — but the creator cannot delete a project they cannot access.
Why: This creates an orphaned project that requires admin intervention to clean up. The alternative (the old projects.ts pattern of logging and continuing) leaves the project accessible but without proper authorization relationships.
Suggestion: The stricter error handling is the right choice. Consider adding a future enhancement: (1) an admin endpoint to list/delete orphaned projects, (2) a background job to retry SpiceDB sync with a pending_authorization flag, or (3) compensating cleanup logic that deletes the project if SpiceDB sync fails.
🕐 Pending Recommendations (1)
From previous reviews:
- 💭
SchemaProcessor.test.ts:304Consider documenting edge case behavior foradditionalProperties,$ref, andpatternProperties(these JSON Schema keywords are not normalized by the current implementation)
✅ APPROVE
Summary: This PR successfully addresses the Azure OpenAI structured output compatibility issue with a well-implemented normalizeForStructuredOutput method. The previous review's main concerns have been fully addressed:
✅ Tests added — 304 lines of comprehensive test coverage including basic normalization, nested objects, arrays, union types (anyOf/oneOf/allOf), edge cases, and real-world schema examples.
✅ SpiceDB error handling improved — The new pattern in projectFull.ts correctly fails the request when SpiceDB sync fails on create (preventing inaccessible projects), while appropriately logging warnings on delete (orphaned relationships are harmless). This is an improvement over the existing projects.ts pattern.
The schema normalization implementation is clean, follows existing patterns in the codebase, and provides the cross-provider compatibility needed for Azure OpenAI's strict structured output requirements. 🎉
Discarded (6)
| Location | Issue | Reason Discarded |
|---|---|---|
projects.ts:263-266 vs projectFull.ts:168-194 |
SpiceDB sync error handling inconsistency | The new pattern is an improvement over the existing code, not a regression. The old projects.ts pattern (log and continue) is the problem. |
projects.ts:265,418 |
Uses console.warn vs logger.warn |
Pre-existing issue in projects.ts, not introduced by this PR |
SchemaProcessor.test.ts |
No test for non-object primitive inputs | INFO severity, LOW confidence, edge case unlikely in practice |
projectFull.ts:546-562 |
DELETE route SpiceDB cleanup uses warn vs error | The reviewer explicitly states "The current behavior is acceptable from a security standpoint" |
projectFull.ts:169,423 |
Conditional SpiceDB sync skipped when userId is absent | INFO severity, may be intentional for service accounts |
projectFull.ts:552-562 |
SpiceDb error object logging could be more detailed | LOW confidence, minor improvement for a cleanup operation |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 |
pr-review-consistency |
2 | 0 | 0 | 0 | 0 | 2 |
pr-review-tests |
2 | 0 | 1 | 0 | 0 | 1 |
pr-review-types |
0 | 0 | 0 | 0 | 0 | 0 |
pr-review-security-iam |
3 | 0 | 1 | 0 | 0 | 2 |
pr-review-errors |
0 | 0 | 0 | 0 | 0 | 0 |
| Total | 7 | 0 | 2 | 0 | 1 | 5 |
There was a problem hiding this comment.
PR Review Summary
0 Key Findings | Risk: Low
🔴❗ Critical (0) ❗🔴
None identified.
🟠⚠️ Major (0) 🟠⚠️
None identified.
🟡 Minor (0) 🟡
None identified.
💭 Consider (1) 💭
💭 1) SchemaProcessor.test.ts:22 Misleading test name
Issue: The test at line 22 is named "should preserve existing required array while making all properties required" but the implementation at SchemaProcessor.ts:247 completely replaces required with Object.keys(properties). The test assertion is correct — all properties become required — but the name suggests preservation/merge semantics that don't exist.
Why: The misleading name could cause confusion during maintenance. If someone later needs to actually preserve existing required values (e.g., for backwards compatibility), they might assume this test validates that behavior.
Suggestion: Consider renaming to accurately describe the behavior: "should overwrite existing required array to include all properties" or "should make all properties required regardless of existing required array". The test body can remain unchanged.
Refs: SchemaProcessor.test.ts:22-35
🕐 Pending Recommendations (2)
From previous reviews:
- 💭
SchemaProcessor.test.ts:304Consider documenting edge case behavior foradditionalProperties,$ref, andpatternProperties(these JSON Schema keywords are not normalized by the current implementation) - 💭
projectFull.ts:169-194The stricter SpiceDB sync error handling (fail on create, warn on delete) is an intentional improvement. Consider migratingprojects.tsto the same pattern for consistency in a follow-up.
✅ APPROVE
Summary: The schema normalization fix for Azure OpenAI structured output compatibility is well-implemented with comprehensive test coverage. The normalizeForStructuredOutput method correctly handles:
- Basic and deeply nested object normalization
- Array items with object schemas
- Union types (
anyOf,oneOf,allOf) - Edge cases (null, undefined, empty properties, primitive schemas)
- Non-mutation of original schema
The SpiceDB authorization sync additions to projectFull.ts follow a stricter error handling pattern than the existing projects.ts — failing on create (preventing inaccessible orphaned projects) while warning on delete (cleanup failures are harmless). This is the correct approach and represents an improvement over the existing pattern.
All prior major feedback (missing tests, SpiceDB error handling) has been addressed. Great work! 🎉
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
projects.ts vs projectFull.ts |
Logging approach mismatch (console.warn vs structured logger) |
Pre-existing issue in projects.ts, not introduced by this PR. The new code correctly uses structured logging. |
SchemaProcessor.test.ts:227 |
Shallow non-mutation test only verifies top-level | LOW confidence; implementation correctly creates new objects at each level, current usage patterns are safe |
artifact-component-schema.ts:109,166 |
Type assertions (as JSONSchema.BaseSchema, as any) |
LOW confidence; assertions are safe given preceding code constructs valid schemas, pragmatic for JSON schema manipulation |
SchemaProcessor.ts:235-237 |
Broad return type requires assertions by callers | LOW confidence; design consideration for a function handling loosely-typed external data, optional enhancement |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|
pr-review-consistency |
2 | 0 | 0 | 0 | 1 | 1 |
pr-review-tests |
2 | 0 | 1 | 0 | 0 | 1 |
pr-review-types |
3 | 0 | 0 | 0 | 0 | 3 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 |
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 |
pr-review-errors |
0 | 0 | 0 | 0 | 0 | 0 |
| Total | 7 | 0 | 1 | 0 | 1 | 5 |
There was a problem hiding this comment.
PR Review Summary
0 Key Findings | Risk: Low
🔴❗ Critical (0) ❗🔴
None identified.
🟠⚠️ Major (0) 🟠⚠️
None identified.
🟡 Minor (0) 🟡
None identified.
💭 Consider (0) 💭
No new suggestions.
🕐 Pending Recommendations (3)
From previous reviews:
- 💭
SchemaProcessor.test.ts:304Consider documenting edge case behavior foradditionalProperties,$ref, andpatternProperties(these JSON Schema keywords are not normalized by the current implementation) - 💭
SchemaProcessor.test.ts:22Misleading test name — "should preserve existing required array while making all properties required" describes preservation semantics that don't exist; the implementation overwrites therequiredarray
New human feedback:
- 💬
projectFull.ts:168@amikofalvy raised a valid architectural point about using transaction-like semantics for the database + SpiceDB operations. This is a reasonable enhancement to consider — wrapping both operations in a compensating transaction pattern (create project → sync to SpiceDB → on failure, delete project) would prevent orphaned projects.
✅ APPROVE
Summary: This is a delta re-review triggered by pull_request:synchronize. The delta since the last review (1429cca3) contains:
| Commit | Description |
|---|---|
a5b019c3 |
updated |
PR-specific delta: The only change is in projectFull.test.ts (+6/-7 lines) — updating the test should reject projects with empty IDs to correctly expect a 500 error with "authorization setup failed" message instead of a successful 201 response. This accurately reflects the new behavior where SpiceDB validation rejects empty project IDs.
Addressing @amikofalvy's feedback: The suggestion to wrap the database + SpiceDB operations in a transaction-like pattern with compensating rollback is architecturally sound. The current implementation creates the project first, then syncs to SpiceDB — if SpiceDB fails, an orphaned project exists in the database that the creator cannot access or delete. A compensating transaction pattern (delete the project on SpiceDB sync failure) would be a good follow-up enhancement. This is orthogonal to the main PR's schema normalization fix and represents a reasonable future improvement.
No PR-specific code changes requiring specialized review. The core implementation (SchemaProcessor.ts, Agent.ts, artifact-component-schema.ts, projectFull.ts, and comprehensive tests) remains unchanged from the previous review which found no blocking issues.
The schema normalization fix for Azure OpenAI structured output compatibility is well-implemented with comprehensive test coverage. Ship it! 🚀
Discarded (0)
No new findings to discard.
Reviewers (0)
Delta review — no sub-reviewers dispatched. The delta contains only a test expectation update that correctly documents the new SpiceDB validation behavior.
| * @param schema - JSON schema to normalize (supports nested objects, arrays, unions) | ||
| * @returns Normalized schema with all properties marked as required | ||
| */ | ||
| static normalizeForStructuredOutput( |
There was a problem hiding this comment.
I find that normalizeForStructuredOutput name hides the behavior of the function which is to make all properties required. I suggest renaming the function for clarity. makeAllPropertiesRequired( which leaves no shadow of doubt. Then the jsdoc can be removed and the function signature is enough to describe the entire function behavior.
There was a problem hiding this comment.
PR Review Summary
2 Key Findings | Risk: Low
🔴❗ Critical (0) ❗🔴
None identified.
🟠⚠️ Major (2) ⚠️ 🟠
Inline comments:
- 🟠 Major:
projectFull.ts:134Comments overstate atomicity guarantee — SpiceDB success + DB commit failure leaves orphaned SpiceDB relationships - 🟠 Major:
projectFull.ts:203SpiceDB error detection heuristic is overly broad —error?.detailsmatches many non-SpiceDB errors
🟡 Minor (0) 🟡
None identified.
💭 Consider (2) 💭
💭 1) projectFull.ts:166 Middleware state mutation inside transaction scope
Issue: c.set('resolvedRef', newResolvedRef) is called inside the transaction callback. If the transaction later fails, this context state has already been mutated.
Why: Depending on how middleware reads resolvedRef on error paths, this could cause unexpected behavior. In practice, error responses likely don't use this value.
Suggestion: Low priority — monitor for issues. If problems arise, defer the c.set() call until after transaction commit.
💭 2) projectFull.test.ts:11-19 Consider adding a SpiceDB failure test case
Issue: The SpiceDB mocks always return success, so the new transaction rollback behavior is not directly tested.
Why: A regression where SpiceDB sync is accidentally moved outside the transaction wouldn't be caught by existing tests.
Suggestion: Consider adding a test that mocks syncProjectToSpiceDb to reject, then verifies the project does NOT exist in the database afterward.
🕐 Pending Recommendations (2)
From previous reviews:
- 💭
SchemaProcessor.ts:235@amikofalvy suggests renamingnormalizeForStructuredOutputtomakeAllPropertiesRequiredfor clarity - 💭
SchemaProcessor.test.ts:304Consider documenting edge case behavior foradditionalProperties,$ref, andpatternProperties
💡 APPROVE WITH SUGGESTIONS
Summary: This delta directly addresses @amikofalvy's feedback by wrapping database operations and SpiceDB sync in nested transactions. The implementation is a clear improvement over the previous pattern — if SpiceDB fails, database changes now rollback automatically.
Two observations worth addressing:
- Comment accuracy — The comments claim full atomicity, but SpiceDB success + DB commit failure is not handled (orphaned SpiceDB relationships). This is rare and arguably better than the previous failure mode, but the comments should reflect reality.
- Error detection — The
error?.detailscheck is too broad and could misclassify non-SpiceDB errors. Consider tightening the heuristic.
These are refinements, not blockers. The core transaction pattern is correct and addresses the original concern. 🎉
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
projectFull.test.ts |
Empty ID test removed | Test was testing SpiceDB behavior which is now mocked; appropriate for integration test scope |
projectFull.ts:406-447 |
PUT path has same issues as POST | Same patterns apply; not a separate issue |
projectFull.ts |
Full Saga/compensating transaction pattern | Enhancement suggestion, not a defect in current implementation |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|
pr-review-sre |
4 | 0 | 1 | 2 | 0 | 1 |
pr-review-errors |
1 | 0 | 0 | 0 | 0 | 1 |
pr-review-tests |
2 | 0 | 1 | 0 | 0 | 1 |
| Total | 7 | 0 | 2 | 2 | 0 | 3 |
Note: SRE findings #2 and Errors finding #1 were merged (same issue). SRE finding #3 and Tests finding #1 were merged (related coverage gap).
| }); | ||
| // Two-phase commit: Wrap all database operations in transactions | ||
| // If SpiceDB sync fails, both DB transactions will rollback automatically | ||
| const createdProject = await runDbClient.transaction(async (runTx) => { |
There was a problem hiding this comment.
🟠 MAJOR: Comments overstate atomicity guarantee
Issue: The comment at lines 132-133 claims "If SpiceDB sync fails, both DB transactions will rollback automatically" — this is true. However, the converse case is not handled: if SpiceDB succeeds but the DB transaction commit subsequently fails, SpiceDB state cannot be rolled back because syncProjectToSpiceDb has no rollback mechanism.
Why: The comments set an expectation of full two-phase commit atomicity that doesn't exist. SpiceDB uses RelationshipOperation.CREATE which is not idempotent. If SpiceDB succeeds but DB commit fails, orphaned SpiceDB relationships will exist pointing to a project that doesn't exist in the database. This is arguably better than the previous pattern (orphaned DB project with no SpiceDB access), but the guarantee is one-directional.
Fix: Update comments to accurately describe the actual failure modes:
| const createdProject = await runDbClient.transaction(async (runTx) => { | |
| try { | |
| // Wrap database operations in transactions. If SpiceDB sync fails, DB changes rollback. | |
| // NOTE: SpiceDB is NOT transactional with the DB. If SpiceDB succeeds but DB commit fails, | |
| // orphaned SpiceDB relationships may remain. This is rare and preferable to orphaned DB projects. | |
| const createdProject = await runDbClient.transaction(async (runTx) => { |
Refs:
| } | ||
|
|
||
| // Handle SpiceDB sync failures - transactions already rolled back | ||
| if (error?.message?.includes('SpiceDB') || error?.code === 3 || error?.details) { |
There was a problem hiding this comment.
🟠 MAJOR: SpiceDB error detection heuristic is overly broad
Issue: The condition error?.message?.includes('SpiceDB') || error?.code === 3 || error?.details has problematic checks:
error?.code === 3matches gRPCINVALID_ARGUMENT, but could also match errors from other gRPC services (OTLP exporters, etc.)error?.detailsmatches any error with adetailsproperty, including Zod validation errors, database errors, and many JS/Node errors
Why: A database constraint error with .details, or a telemetry export failure with code 3, would be incorrectly handled as a SpiceDB failure. The user would see "Failed to set up project authorization. No changes were made to the database." when the actual failure could be unrelated to SpiceDB.
Fix: Make the detection more specific. The @authzed/authzed-node library throws gRPC errors with metadata:
| if (error?.message?.includes('SpiceDB') || error?.code === 3 || error?.details) { | |
| // Handle SpiceDB sync failures - transactions already rolled back | |
| // Check for gRPC error characteristics (SpiceDB uses gRPC) | |
| const isGrpcError = error?.metadata !== undefined && typeof error?.code === 'number'; | |
| const mentionsSpiceDb = error?.message?.includes('SpiceDB'); | |
| if (mentionsSpiceDb || isGrpcError) { |
Or minimally, remove the overly broad error?.details check:
if (error?.message?.includes('SpiceDB') || (error?.code === 3 && error?.metadata)) {Refs:
There was a problem hiding this comment.
PR Review Summary
0 Key Findings | Risk: Low
🔴❗ Critical (0) ❗🔴
None identified.
🟠⚠️ Major (0) 🟠⚠️
None identified.
🟡 Minor (0) 🟡
None identified.
💭 Consider (0) 💭
No new suggestions.
🕐 Pending Recommendations (3)
From previous reviews:
- 🟠
projectFull.ts:134Comments overstate atomicity guarantee — SpiceDB success + DB commit failure leaves orphaned SpiceDB relationships - 🟠
projectFull.ts:203SpiceDB error detection heuristic is overly broad —error?.detailsmatches many non-SpiceDB errors - 💭
SchemaProcessor.test.ts:304Consider documenting edge case behavior foradditionalProperties,$ref, andpatternProperties
✅ APPROVE
Summary: This is a delta re-review triggered by pull_request:synchronize. The delta since the last review (6911b96c) contains a single commit:
| Commit | Description |
|---|---|
d5443b4d |
renamed function |
Human feedback addressed: ✅ @amikofalvy's suggestion to rename normalizeForStructuredOutput to makeAllPropertiesRequired has been implemented. The new name accurately describes the function's behavior without requiring JSDoc to understand it.
Changes in delta:
SchemaProcessor.ts: Method renamed fromnormalizeForStructuredOutput→makeAllPropertiesRequired, JSDoc simplifiedAgent.ts: Call site updatedartifact-component-schema.ts: Call sites updated (×2)SchemaProcessor.test.ts: Test describe block and all test calls updated
No behavioral changes. The core schema normalization fix for Azure OpenAI structured output compatibility remains well-implemented with comprehensive test coverage. The prior review's pending recommendations (comment accuracy, error detection heuristic, edge case documentation) remain valid suggestions for future improvement but are not blockers.
Ship it! 🚀
Discarded (0)
No new findings to discard.
Reviewers (0)
Delta review — no sub-reviewers dispatched. The rename-only delta contains no behavioral changes requiring specialized review.
There was a problem hiding this comment.
PR Review Summary
0 Key Findings | Risk: Low
🔴❗ Critical (0) ❗🔴
None identified.
🟠⚠️ Major (0) 🟠⚠️
None identified.
🟡 Minor (0) 🟡
None identified.
💭 Consider (0) 💭
No new suggestions.
🕐 Pending Recommendations (2)
From previous reviews:
- 🟠
projectFull.ts:134Comments overstate atomicity guarantee — the comment claims "If SpiceDB sync fails, both DB transactions will rollback automatically" but doesn't mention the converse case (SpiceDB succeeds, DB commit fails → orphaned SpiceDB relationships) - 💭
SchemaProcessor.test.ts:304Consider documenting edge case behavior foradditionalProperties,$ref, andpatternProperties
✅ APPROVE
Summary: This is a delta re-review triggered by pull_request:synchronize. The delta since the last review (d5443b4da602) contains:
| Commit | Description |
|---|---|
6a1b44d5 |
Merge branch 'main' into bugfix/schema-validation-user |
b7187e5e |
updated (projectFull.ts changes) |
5b0b1f15 |
fix max height for card content on traces conversation page (#1905) — from main |
Prior feedback addressed: ✅ The SpiceDB error detection heuristic feedback has been implemented. The previous code used:
if (error?.message?.includes('SpiceDB') || error?.code === 3 || error?.details)The new implementation is more specific to gRPC errors:
const isGrpcError = error?.metadata !== undefined && typeof error?.code === 'number';
const mentionsSpiceDb = error?.message?.includes('SpiceDB');
if (mentionsSpiceDb || isGrpcError)This correctly:
- Removes the overly broad
error?.detailscheck that could match Zod validation errors, database errors, etc. - Removes the standalone
error?.code === 3check that could match non-SpiceDB gRPC services - Adds a gRPC-specific check (
error?.metadata !== undefined) combined with numeric code verification
The remaining pending recommendations (comment accuracy about atomicity, edge case test coverage) are minor improvements — not blockers for this fix.
The core schema normalization fix for Azure OpenAI structured output compatibility remains well-implemented with comprehensive test coverage. Ship it! 🚀
Discarded (0)
No new findings to discard.
Reviewers (0)
Delta review — no sub-reviewers dispatched. The delta contains a focused fix addressing prior feedback and merge commits from main (unrelated UI changes).
|
this PR appears to combine multiple separate issues in a single batch:
Can you explain why these are both in here? Is the spice db change related to the json schema change in some way I don't know about? |
@amikofalvy i noticed both of these issues at the same time when i was testing an agent for the first time and both were causing me issues with the agent i tested so both were fixed at the same time |
4 participants
Azure OpenAI's structured output API requires ALL properties to be in the required array, even nullable/optional ones. When data components with optional properties (like fact: z.string().nullable()) were converted to JSON schemas, they failed with:
Invalid schema for response_format 'response': 'required' is required to be supplied and to be an array including every key in properties. Missing 'fact'.Solution:
Added SchemaProcessor.normalizeForStructuredOutput() that recursively ensures all properties in JSON schemas are marked as required before conversion to Zod schemas via z.fromJSONSchema().
Changes: