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.
1 Skipped Deployment
|
|
|
TL;DR — Data component JSON Schemas are now normalized before being sent to LLMs, stripping numeric constraints that Anthropic rejects ( Key changes
Summary ｜ 8 files ｜ 4 commits ｜ base: Shared normalization pipeline in
|
![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.
| 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.
agents-manage-ui/src/app/api/data-components/[dataComponentId]/generate-render/route.ts
Outdated
Show resolved
Hide resolved
![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 |
| 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:
sarah-inkeep
left a comment
sarah-inkeep
left a comment
There was a problem hiding this comment.
Could you look through the pullfrog / claude code comments, I think this one in particular seems relevant https://github.com/inkeep/agents/pull/2764/changes#r2956834716?
Could you also apply this fix to artifacts as well? I think the issue exists there as well.
…emas - SchemaProcessor.makeAllPropertiesRequired now delegates to agents-core (same pattern as stripUnsupportedConstraints) - artifact-component-schema.ts now uses normalizeDataComponentSchema (adds missing stripUnsupportedConstraints for Anthropic compatibility)
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
✅ Prior Issues Resolved
The key issues from the prior reviews have been addressed in this latest commit:
-
✅ SchemaProcessor duplication fixed —
makeAllPropertiesRequirednow delegates tomakeAllPropertiesRequiredCorefromagents-core(lines 235-238), following the same pattern asstripUnsupportedConstraints. The ~40-line duplicate implementation has been removed. -
✅ Artifacts now use full normalization — Both
ArtifactCreateSchema.getSchemas()(line 106) andArtifactCreateSchema.getDataComponents()(line 160) now callnormalizeDataComponentSchema()instead of justSchemaProcessor.makeAllPropertiesRequired(). This ensures Anthropic compatibility (constraint stripping) is applied to artifact schemas, addressing sarah-inkeep's feedback.
🕐 Pending Recommendations (2)
These items were flagged in prior automated reviews but were not specifically requested by the human reviewer. They remain as considerations for future work:
- 🟠
generate.ts(lines 228-238) Silent fallback when buildDataComponentsSchema fails — logs error but continues without user visibility - 🟡
generate-render/route.ts(lines 58-61) Schema normalization errors caught by outer handler with generic message — could benefit from specific error handling
💭 Consider (3)
Test coverage enhancements (all MINOR, optional):
- Add tests for
oneOf/allOfrecursion instripUnsupportedConstraints(currently onlyanyOfis tested) - Add tests for
makeAllPropertiesRequiredrecursion through nested arrays - Add null/undefined input test for the composed
normalizeDataComponentSchemapipeline
⚠️ Changeset Reminder
The changeset-bot flagged this PR has no changeset. Consider adding:
pnpm bump patch --pkg agents-core --pkg agents-api "Fix data component schema normalization for cross-provider LLM compatibility"✅ APPROVE
Summary: The requested fixes have been implemented correctly. SchemaProcessor now properly delegates both makeAllPropertiesRequired and stripUnsupportedConstraints to the shared utilities in agents-core, eliminating the code duplication. Artifact schemas now receive full cross-provider normalization (constraint stripping + required promotion), which resolves the Anthropic compatibility issue sarah-inkeep flagged. The implementation is clean and the test coverage validates the core functionality. 🎉
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | Pending Recs | Discarded |
|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 |
pr-review-tests |
3 | 0 | 3 | 0 | 0 |
pr-review-consistency |
0 | 0 | 0 | 0 | 0 |
pr-review-errors |
2 | 0 | 0 | 2 | 0 |
| Total | 5 | 0 | 3 | 2 | 0 |
Note: pr-review-standards and pr-review-consistency returned zero findings because all prior issues have been addressed in this commit.
The artifact generate-render route was passing raw schema to z.fromJSONSchema without normalization, causing OpenAI strict-mode rejection when properties exist but aren't all in required.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
✅ Delta Review — Clean
This is a re-review scoped to 1 commit since the last automated review.
What changed:
agents-manage-ui/src/app/api/artifact-components/[artifactComponentId]/generate-render/route.ts: AddednormalizeDataComponentSchema()call before passingartifactComponent.propstoz.fromJSONSchema()— this mirrors the fix already applied to the data-component route.
Assessment: The change correctly addresses sarah-inkeep's request to apply the normalization fix to artifact components. The implementation is consistent with the data-component route (generate-render/route.ts lines 58-61) and properly handles the optional props field.
✅ Prior Issues Resolved
All Main issues from prior reviews have been addressed:
- ✅ SchemaProcessor duplication —
makeAllPropertiesRequirednow delegates tomakeAllPropertiesRequiredCorefromagents-core(commitaf5a9524b) - ✅ Artifact schemas use full normalization — Both
ArtifactCreateSchema.getSchemas()andArtifactCreateSchema.getDataComponents()callnormalizeDataComponentSchema()(commitaf5a9524b) - ✅ Artifact generate-render route normalized — This delta adds the missing normalization to the UI preview route (commit
fcb5952ce)
🕐 Pending Recommendations (2)
These items were flagged in prior reviews but not specifically requested by the human reviewer. They remain as considerations for future work:
- 🟠
generate.ts(lines 228-238) — Silent fallback whenbuildDataComponentsSchema()fails logs error but continues without user visibility - 🟡
generate-render/route.ts(lines 58-61) — Schema normalization errors caught by outer handler with generic message; could benefit from specific error handling
⚠️ Changeset Reminder
The changeset-bot flagged this PR has no changeset. This PR modifies agents-core, agents-api, and agents-manage-ui with runtime behavior changes. Consider adding:
pnpm bump patch --pkg agents-core --pkg agents-api "Fix data component schema normalization for cross-provider LLM compatibility"✅ APPROVE
Summary: The human-requested fix has been implemented correctly. Artifact component schemas in the generate-render route now receive the same cross-provider normalization (constraint stripping + required promotion) as data components. All prior blocking issues have been addressed. Ship it! 🚀
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator |
0 | 0 | 0 | 0 | 0 | 2 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 2 | 0 |
Note: This is a delta review scoped to changes since the last automated review. No new issues were identified in the delta.
Ito Test Report ❌15 test cases ran. 3 failed, 12 passed. Overall, 15 test cases ran with 12 passing and 3 failing, showing that core data/artifact generate and regenerate flows, runtime SSE compatibility, resilience under interruption/rage-click/cancel stress, prompt-injection and malicious-schema handling, and mobile viewport usability all worked as expected. The key confirmed issues are three pre-existing API defects in generate-render routes: unauthenticated access is not blocked (High), unknown component IDs return 500 instead of 404, and cross-tenant/not-found upstream errors are incorrectly collapsed into generic 500 responses (while missing tenantId/projectId validation correctly returns 400). ❌ Failed (3)
|
| Category | Summary | Screenshot |
|---|---|---|
| Adversarial | Verified rage-click throttling behavior is enforced by UI generation state, with no app defect indicated. |  |
| Adversarial | Rapid regenerate/cancel interactions returned to a stable final state with coherent output and enabled controls. |  |
| Adversarial | Prompt-injection-like regenerate instructions completed without NDJSON parser crash or UI failure. |  |
| Adversarial | Malicious schema/sample strings did not trigger script execution or console-error instability. |  |
| Adversarial | Mobile viewport generate/regenerate flows, tab navigation, and sample-data editing were usable across data and artifact editors. |  |
| Edge | Data and artifact generate-render endpoints correctly return HTTP 400 when tenantId or projectId is missing. |  |
| Edge | Executed interruption/back-forward/rage-click/regenerate-cancel resilience checks; editors recovered cleanly and stayed interactive. |  |
| Happy-path | Data component generation completed with Render/Code/Sample Data populated under verified run conditions. |  |
| Happy-path | Artifact component generation completed with Render/Code/Sample Data populated. |  |
| Happy-path | /run/v1/chat/completions returned HTTP 200 SSE with terminal completion, confirming runtime compatibility for the normalized-schema flow in this run. |  |
| Happy-path | Not a real application bug; code-level verification confirms regenerate-with-instructions preserves existing mockData when model output omits it. |  |
| Happy-path | Artifact component generate/regenerate flow completed and remained coherent after refresh. |  |
Commit: fcb5952
Tell us how we did: Give Ito Feedback
2 participants
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.