Conversation
…ility Adds stripUnsupportedConstraints + makeAllPropertiesRequired to agents-core as a shared normalizeDataComponentSchema utility, fixing two provider errors: - Anthropic: rejects minimum/maximum on number types in structured output - OpenAI: requires all properties to appear in the required array Applies normalization in both affected code paths: - Runtime agent generation (schema-builder.ts) - Component generation UI (generate-render/route.ts, previously had no normalization) Also wraps buildDataComponentsSchema in a try/catch in generate.ts so an invalid schema no longer crashes the server — falls back to generation without structured output instead.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
TL;DR — Data component JSON Schemas are now normalized before being sent to LLMs, stripping numeric constraints that Anthropic rejects ( Key changes
Summary ｜ 6 files ｜ 1 commit ｜ base:
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good fix — the new shared normalization utilities are well-structured, the try/catch fallback in generate.ts prevents server crashes, and the test coverage is solid. Two items worth addressing: a duplication concern and a gap in the artifact-component-schema.ts call sites.
| T extends JSONSchema.BaseSchema | Record<string, unknown> | null | undefined, | ||
| >(schema: T): T { | ||
| return stripUnsupportedConstraints(schema as Record<string, unknown>) as T; | ||
| } |
There was a problem hiding this comment.
SchemaProcessor.makeAllPropertiesRequired (line 232) is now a near-identical duplicate of the standalone makeAllPropertiesRequired in agents-core/schema-conversion.ts. Unlike stripUnsupportedConstraints which already delegates here, makeAllPropertiesRequired does not — it still carries its own recursive implementation.
Consider having the static method delegate to the shared function (same pattern as stripUnsupportedConstraints on line 289) or removing it entirely and updating callers in artifact-component-schema.ts to import from agents-core directly. Keeping two independent copies is a maintenance hazard.
| /** | ||
| * Strips JSON Schema constraints that are not supported by all LLM providers. | ||
| * Delegates to the shared `stripUnsupportedConstraints` utility from agents-core. | ||
| */ | ||
| static stripUnsupportedConstraints< |
There was a problem hiding this comment.
This delegates stripUnsupportedConstraints to the shared util — good. But makeAllPropertiesRequired right above (line 232) does not delegate; it has its own full copy of the logic. These two methods should follow the same pattern for consistency.
| export function stripUnsupportedConstraints<T extends Record<string, unknown> | null | undefined>( | ||
| schema: T | ||
| ): T { | ||
| if (!schema || typeof schema !== 'object') { | ||
| return schema; | ||
| } | ||
|
|
||
| const stripped: any = { ...schema }; | ||
|
|
||
| if (stripped.type === 'number' || stripped.type === 'integer') { | ||
| delete stripped.minimum; | ||
| delete stripped.maximum; | ||
| delete stripped.exclusiveMinimum; | ||
| delete stripped.exclusiveMaximum; | ||
| delete stripped.multipleOf; | ||
| } | ||
|
|
||
| if (stripped.properties && typeof stripped.properties === 'object') { | ||
| const strippedProperties: any = {}; | ||
| for (const [key, value] of Object.entries(stripped.properties)) { | ||
| strippedProperties[key] = stripUnsupportedConstraints(value as Record<string, unknown>); | ||
| } | ||
| stripped.properties = strippedProperties; | ||
| } | ||
|
|
||
| if (stripped.items) { | ||
| stripped.items = stripUnsupportedConstraints(stripped.items as Record<string, unknown>); | ||
| } | ||
| if (Array.isArray(stripped.anyOf)) { | ||
| stripped.anyOf = stripped.anyOf.map((s: any) => | ||
| stripUnsupportedConstraints(s as Record<string, unknown>) | ||
| ); | ||
| } | ||
| if (Array.isArray(stripped.oneOf)) { | ||
| stripped.oneOf = stripped.oneOf.map((s: any) => | ||
| stripUnsupportedConstraints(s as Record<string, unknown>) | ||
| ); | ||
| } | ||
| if (Array.isArray(stripped.allOf)) { | ||
| stripped.allOf = stripped.allOf.map((s: any) => | ||
| stripUnsupportedConstraints(s as Record<string, unknown>) | ||
| ); | ||
| } | ||
|
|
||
| return stripped; |
There was a problem hiding this comment.
Neither stripUnsupportedConstraints nor makeAllPropertiesRequired recurse into $defs / definitions or follow $ref pointers. If a user defines a data component schema that uses JSON Schema references (e.g., shared types via $defs), constraints inside those definitions will survive stripping.
This is likely fine for current data component schemas (they tend to be flat), but worth noting as a known limitation — or adding a $defs/definitions traversal for completeness.
| const shouldStream = ctx.isDelegatedAgent ? undefined : ctx.streamHelper; | ||
|
|
||
| const dataComponentsSchema = hasStructuredOutput ? buildDataComponentsSchema(ctx) : null; | ||
| let dataComponentsSchema: ReturnType<typeof buildDataComponentsSchema> | null = null; |
There was a problem hiding this comment.
Nit: ReturnType<typeof buildDataComponentsSchema> resolves to z.ZodType<any>, so let dataComponentsSchema: z.ZodType<any> | null = null would be more readable and avoids coupling to the function name.
| // Normalize for cross-provider compatibility: | ||
| // - strips Anthropic-unsupported constraints (minimum/maximum on numbers) | ||
| // - ensures all properties are in required (OpenAI strict-mode requirement) | ||
| const normalizedProps = normalizeDataComponentSchema( |
There was a problem hiding this comment.
These three comment lines restate what the code does and what the JSDoc on normalizeDataComponentSchema already documents. Per codebase conventions, consider removing them.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: Medium
🟠⚠️ Major (3) 🟠⚠️
🟠 1) SchemaProcessor.ts Duplicate makeAllPropertiesRequired should delegate to agents-core
Issue: The PR correctly established a delegation pattern for stripUnsupportedConstraints (lines 289-293), but makeAllPropertiesRequired (lines 232-283) remains a full 50-line duplicate of the identical implementation in agents-core/src/utils/schema-conversion.ts:145-196.
Why: Code duplication creates maintenance burden — if a bug is found in one implementation, it must be fixed in both. The delegation pattern is already established in this file; makeAllPropertiesRequired should follow the same pattern for consistency. This is especially concerning since both implementations handle the same edge cases (nullable detection, anyOf handling).
Fix: Replace the full implementation with a delegation wrapper (same pattern as stripUnsupportedConstraints):
import { makeAllPropertiesRequired as makeAllPropertiesRequiredCore } from '@inkeep/agents-core';
static makeAllPropertiesRequired<T extends JSONSchema.BaseSchema | Record<string, unknown> | null | undefined>(schema: T): T {
return makeAllPropertiesRequiredCore(schema as Record<string, unknown>) as T;
}Then remove lines 232-283.
Refs:
🟠 2) generate.ts:228-238 Silent fallback masks failures from users
Issue: When buildDataComponentsSchema() fails, the error is logged but generation silently continues without structured output. Users who configured data components expecting JSON responses will receive unstructured text instead, with no visible indication that anything went wrong.
Why: This creates a debugging nightmare. Users see their agent "working" but not producing the expected data components. The catch block swallows ALL exceptions indiscriminately — schema validation errors, type errors, or unexpected runtime bugs are all hidden. The only diagnostic path is server log correlation, which most users cannot access.
Fix: Consider surfacing the failure to users. Options:
- Fail fast — throw a user-facing error explaining the schema issue
- Graceful with warning — add
span.setAttribute('generation.structured_output_fallback', true)and send a warning viactx.streamHelperif available - At minimum — include
dataComponentIdsin the error log for debugging
Refs:
🟠 3) generate-render/route.ts:63-66 Missing error handling for schema normalization
Issue: normalizeDataComponentSchema() and z.fromJSONSchema() are called without specific error handling. If these fail, the outer try/catch returns a generic HTTP 500 with no context about which data component failed or why.
Why: When "Generate Component" fails in the Agent Builder UI, users cannot self-diagnose. They see a generic error toast with no actionable information. This generates support tickets that could be avoided with better error messaging.
Fix: Add specific error handling that returns HTTP 400 with actionable feedback:
try {
normalizedProps = normalizeDataComponentSchema(dataComponent.props as Record<string, unknown>);
mockDataSchema = z.fromJSONSchema(normalizedProps);
} catch (error) {
console.error('Schema normalization failed:', { dataComponentId, dataComponentName: dataComponent.name, error });
return new Response(JSON.stringify({
error: 'Invalid data component schema',
message: 'Please check that all field types are valid.',
}), { status: 400, headers: { 'Content-Type': 'application/json' } });
}Refs:
Inline Comments:
- 🟠 Major:
SchemaProcessor.ts:293Duplicate makeAllPropertiesRequired should delegate - 🟠 Major:
generate.ts:228-238Silent fallback from structured to unstructured output - 🟠 Major:
generate-render/route.ts:63-66Missing error handling for schema normalization
🟡 Minor (1) 🟡
🟡 1) schema-normalization.test.ts Additional edge case coverage recommended
Issue: The test suite covers core functionality well but misses some edge cases handled by the implementation: oneOf/allOf recursive stripping (only anyOf tested), nullable: true property handling, and deeply nested schemas (3+ levels).
Why: These code paths exist in the implementation but aren't validated by tests. Future refactoring could silently break them.
Fix: Consider adding tests for oneOf/allOf stripping and nullable: true handling. See inline comment for examples.
Refs:
Inline Comments:
- 🟡 Minor:
schema-normalization.test.ts:175Consider adding edge case coverage
💭 Consider (2) 💭
💭 1) artifact-component-schema.ts:105,159 Partial normalization inconsistency
Issue: This file calls SchemaProcessor.makeAllPropertiesRequired but NOT stripUnsupportedConstraints, meaning artifact component schemas only get OpenAI compatibility fixes but NOT Anthropic compatibility fixes. By contrast, schema-builder.ts and generate-render/route.ts now use the full normalizeDataComponentSchema helper.
Why: Split-world scenario where artifact components behave differently from data components. If artifact schemas include numeric constraints, they'll fail with Anthropic models.
Fix: Consider using normalizeDataComponentSchema here as well for consistency, or document why partial normalization is intentional for artifact schemas.
💭 2) Missing changeset
Issue: The changeset-bot flagged this PR has no changeset. This PR modifies agents-core, agents-api, and agents-manage-ui with bug fixes that affect runtime behavior.
Fix: Add a changeset: pnpm bump patch --pkg agents-core --pkg agents-api "Fix data component schema normalization for cross-provider LLM compatibility"
💡 APPROVE WITH SUGGESTIONS
Summary: The core fix is solid — the new normalizeDataComponentSchema utility correctly addresses the Anthropic/OpenAI compatibility issues. However, the PR introduces code duplication (SchemaProcessor.makeAllPropertiesRequired should delegate to agents-core like stripUnsupportedConstraints does) and the silent fallback behavior in generate.ts could mask configuration issues from users. Address the duplication and consider improving error visibility.
Discarded (3)
| Location | Issue | Reason Discarded |
|---|---|---|
schema-conversion.ts:91-136 |
Unsafe any type casting in recursive schema processing |
Pragmatic choice for JSON Schema processing; type guards at entry points provide sufficient safety |
schema-conversion.ts:1 |
File should be named schema-normalization.ts |
Cosmetic naming suggestion; file already exports schema conversion utilities so current name is acceptable |
generate.ts:303 |
hasStructuredOutput variable is misleading after fallback |
Minor observability concern; the variable reflects config intent, not runtime state — acceptable semantics |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
4 | 1 | 0 | 0 | 1 | 0 | 2 |
pr-review-tests |
8 | 0 | 0 | 0 | 1 | 0 | 7 |
pr-review-errors |
3 | 1 | 0 | 0 | 2 | 0 | 0 |
pr-review-consistency |
3 | 1 | 2 | 0 | 0 | 0 | 0 |
| Total | 18 | 3 | 2 | 0 | 4 | 0 | 9 |
| T extends JSONSchema.BaseSchema | Record<string, unknown> | null | undefined, | ||
| >(schema: T): T { | ||
| return stripUnsupportedConstraints(schema as Record<string, unknown>) as T; | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR: makeAllPropertiesRequired should also delegate to agents-core
Issue: stripUnsupportedConstraints was correctly refactored to delegate to the shared utility (lines 289-293), but makeAllPropertiesRequired (lines 232-283) remains a full duplicate of the identical implementation in agents-core/src/utils/schema-conversion.ts:145-196.
Why: Code duplication creates maintenance burden — bug fixes must be applied in two places. The delegation pattern is already established here for stripUnsupportedConstraints, so makeAllPropertiesRequired should follow the same pattern for consistency.
Fix:
| } | |
| static stripUnsupportedConstraints< | |
| T extends JSONSchema.BaseSchema | Record<string, unknown> | null | undefined, | |
| >(schema: T): T { | |
| return stripUnsupportedConstraints(schema as Record<string, unknown>) as T; | |
| } | |
| /** | |
| * Makes all properties required recursively throughout the schema. | |
| * Delegates to the shared `makeAllPropertiesRequired` utility from agents-core. | |
| */ | |
| static makeAllPropertiesRequired< | |
| T extends JSONSchema.BaseSchema | Record<string, unknown> | null | undefined, | |
| >(schema: T): T { | |
| return makeAllPropertiesRequiredCore(schema as Record<string, unknown>) as T; | |
| } |
Then update the import at the top of the file:
import { stripUnsupportedConstraints, makeAllPropertiesRequired as makeAllPropertiesRequiredCore } from '@inkeep/agents-core';And remove the duplicate implementation at lines 232-283.
Refs:
| let dataComponentsSchema: ReturnType<typeof buildDataComponentsSchema> | null = null; | ||
| if (hasStructuredOutput) { | ||
| try { | ||
| dataComponentsSchema = buildDataComponentsSchema(ctx); | ||
| } catch (err) { | ||
| logger.error( | ||
| { agentId: ctx.config.id, err }, | ||
| 'Failed to build data components schema — continuing without structured output' | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟠 MAJOR: Silent fallback from structured to unstructured output
Issue: When buildDataComponentsSchema() fails, the error is logged but generation silently continues without structured output. Users configured data components expecting JSON output will receive unstructured text instead, with no visible indication of the failure.
Why: This degrades user experience silently. The user configured data components, expects structured output, but receives plain text. The catch block swallows ALL exception types — it could hide schema validation errors, type errors, or unexpected runtime bugs. Debugging requires server log correlation.
Fix: Consider one of these approaches:
Option A — Fail fast (recommended for data integrity):
let dataComponentsSchema: ReturnType<typeof buildDataComponentsSchema> | null = null;
if (hasStructuredOutput) {
try {
dataComponentsSchema = buildDataComponentsSchema(ctx);
} catch (err) {
logger.error(
{ agentId: ctx.config.id, dataComponentIds: ctx.config.dataComponents?.map(dc => dc.id), err },
'Data component schema build failed'
);
throw new Error(
'Failed to build structured output schema from data components. ' +
'Please review your data component schemas in the agent builder.'
);
}
}Option B — Surface warning to user (graceful with awareness):
// Add span attribute for observability
span.setAttribute('generation.structured_output_fallback', true);
// Send warning via stream helper if available
if (ctx.streamHelper) {
ctx.streamHelper.sendWarning('Structured output unavailable due to schema issues. Returning text response.');
}Refs:
- PR description — mentions "graceful fallback" but doesn't address user visibility
| const normalizedProps = normalizeDataComponentSchema( | ||
| dataComponent.props as Record<string, unknown> | ||
| ); | ||
| const mockDataSchema = z.fromJSONSchema(normalizedProps); |
There was a problem hiding this comment.
🟠 MAJOR: Missing error handling for schema normalization
Issue: normalizeDataComponentSchema() and z.fromJSONSchema() are called without specific error handling. If normalization fails, the outer try/catch (line 124) catches it and returns a generic HTTP 500 with error.message. Users see "Internal server error" with no context about which data component failed or why.
Why: When clicking "Generate Component" in the Agent Builder UI fails, users cannot self-diagnose. They see a generic error toast with no actionable information. Every schema issue becomes a support ticket.
Fix: Add specific error handling with actionable feedback:
let normalizedProps: Record<string, unknown>;
let mockDataSchema: z.ZodType<any>;
try {
normalizedProps = normalizeDataComponentSchema(
dataComponent.props as Record<string, unknown>
);
mockDataSchema = z.fromJSONSchema(normalizedProps);
} catch (error) {
console.error('Schema normalization failed:', {
dataComponentId,
dataComponentName: dataComponent.name,
error: error instanceof Error ? error.message : String(error),
});
return new Response(
JSON.stringify({
error: 'Invalid data component schema',
message: 'The data component schema could not be processed. Please check that all field types are valid.',
details: error instanceof Error ? error.message : undefined,
}),
{
status: 400,
headers: { 'Content-Type': 'application/json' },
}
);
}Refs:
- generate.ts error handling — has intermediate error handling with agentId logging
| expect(result.properties.b).not.toHaveProperty('minimum'); | ||
| expect(result.properties.b).not.toHaveProperty('maximum'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟡 Minor: Consider adding test coverage for edge cases
Issue: The test suite covers the core functionality well, but could benefit from additional edge cases:
oneOf/allOfrecursive stripping — onlyanyOfis tested (line 76-82)nullable: trueproperty handling — the code checks for this (schema-conversion.ts:167) but no test validates it- Deeply nested schemas (3+ levels) — only one level of nesting is tested
Why: These edge cases are handled in the implementation but not validated by tests. Future refactoring could silently break them.
Fix: Consider adding these test cases:
it('strips recursively in oneOf', () => {
const schema = {
oneOf: [{ type: 'number', minimum: 0 }, { type: 'string' }]
};
const result = stripUnsupportedConstraints(schema) as any;
expect(result.oneOf[0]).not.toHaveProperty('minimum');
});
it('does not double-wrap properties already marked as nullable', () => {
const schema = {
type: 'object',
properties: { field: { type: 'string', nullable: true } },
required: []
};
const result = makeAllPropertiesRequired(schema) as any;
expect(result.properties.field).not.toHaveProperty('anyOf');
});Refs:
1 participant
Problem
Data component JSON schemas pass our validation but fail at runtime when passed to OpenAI and Anthropic structured output:
minimum/maximum(andexclusiveMinimum,exclusiveMaximum,multipleOf) onnumber/integertypespropertiesto also appear inrequired— optional fields likelimitationscaused rejectionsThis affected both component generation in the UI and runtime agent generation when a data component is attached to a subagent.