Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/twenty-regions-stare.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@inkeep/agents-api": patch
---

Normalize JSON schemas for OpenAI structured output compatibility
32 changes: 13 additions & 19 deletions agents-api/src/__tests__/manage/integration/projectFull.test.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,23 @@
import { generateId } from '@inkeep/agents-core';
import { afterEach, describe, expect, it } from 'vitest';
import { afterEach, describe, expect, it, vi } from 'vitest';
import { cleanupTenants } from '../../utils/cleanup';
import { createTestToolData } from '../../utils/testHelpers';
import { makeRequest } from '../../utils/testRequest';
import { createTestSubAgentData } from '../../utils/testSubAgent';
import { createTestTenantWithOrg } from '../../utils/testTenant';

// Mock SpiceDB sync functions for integration tests
// These are not needed for integration tests as they test the API/DB layer only
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),
syncOrgMemberToSpiceDb: vi.fn().mockResolvedValue(undefined),
};
});

describe('Project Full CRUD Routes - Integration Tests', () => {
// Track tenants created during tests for cleanup
const createdTenants = new Set<string>();
Expand Down Expand Up @@ -576,23 +588,5 @@ describe('Project Full CRUD Routes - Integration Tests', () => {

expect(response.status).toBe(400);
});

it('should handle projects with empty IDs', async () => {
const tenantId = await createTrackedTenant();
const projectDefinition = createTestProjectDefinition(''); // Empty ID

const response = await makeRequest(`/manage/tenants/${tenantId}/project-full`, {
method: 'POST',
body: JSON.stringify(projectDefinition),
expectError: false,
});

// The API currently accepts empty IDs (might be used for special cases)
// This behavior could be changed if empty IDs should be rejected
expect(response.status).toBe(201);
const body = await response.json();
expect(body.data).toBeDefined();
expect(body.data.id).toBe(''); // Empty ID is preserved
});
});
});
221 changes: 166 additions & 55 deletions agents-api/src/domains/manage/routes/projectFull.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import {
getProjectMainBranchName,
getProjectMetadata,
type ResolvedRef,
removeProjectFromSpiceDb,
syncProjectToSpiceDb,
TenantParamsSchema,
TenantProjectParamsSchema,
updateFullProjectServerSide,
Expand Down Expand Up @@ -127,39 +129,63 @@ app.openapi(
const validatedProjectData = FullProjectDefinitionSchema.parse(projectData);

try {
// 1. Create project in runtime DB and create project main branch
await createProjectMetadataAndBranch(
runDbClient,
configDb
)({
tenantId,
projectId: validatedProjectData.id,
createdBy: userId,
});
// 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) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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:

Suggested change
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:

return await configDb.transaction(async (configTx) => {
// Phase 1: Database operations (within transactions - not committed yet)

// 1. Create project in runtime DB and create project main branch
await createProjectMetadataAndBranch(
runTx,
configTx
)({
tenantId,
projectId: validatedProjectData.id,
createdBy: userId,
});

logger.info(
{ tenantId, projectId: validatedProjectData.id },
'Created project with branch, now populating config'
);

logger.info(
{ tenantId, projectId: validatedProjectData.id },
'Created project with branch, now populating config'
);

// Checkout the project main branch
const projectMainBranch = getProjectMainBranchName(tenantId, validatedProjectData.id);
await checkoutBranch(configDb)({ branchName: projectMainBranch, autoCommitPending: true });

// Update resolvedRef so the middleware commits to the correct branch
const newResolvedRef: ResolvedRef = {
type: 'branch',
name: projectMainBranch,
hash: '', // Hash will be determined at commit time
};
c.set('resolvedRef', newResolvedRef);

logger.debug({ projectMainBranch }, 'Checked out project branch for config writes');

// 3. Create full project config in the project branch
const createdProject = await createFullProjectServerSide(configDb)({
scopes: { tenantId, projectId: validatedProjectData.id },
projectData: validatedProjectData,
// Checkout the project main branch
const projectMainBranch = getProjectMainBranchName(tenantId, validatedProjectData.id);
await checkoutBranch(configTx)({
branchName: projectMainBranch,
autoCommitPending: true,
});

// Update resolvedRef so the middleware commits to the correct branch
const newResolvedRef: ResolvedRef = {
type: 'branch',
name: projectMainBranch,
hash: '', // Hash will be determined at commit time
};
c.set('resolvedRef', newResolvedRef);

logger.debug({ projectMainBranch }, 'Checked out project branch for config writes');

// 2. Create full project config in the project branch
const project = await createFullProjectServerSide(configTx)({
scopes: { tenantId, projectId: validatedProjectData.id },
projectData: validatedProjectData,
});

// Phase 2: Sync to SpiceDB (still within transaction scope)
// If this fails, both transactions will rollback automatically
if (userId) {
await syncProjectToSpiceDb({
tenantId,
projectId: validatedProjectData.id,
creatorUserId: userId,
});
}

// If we reach here, both transactions will commit
return project;
});
});

return c.json({ data: createdProject }, 201);
Expand All @@ -173,6 +199,26 @@ app.openapi(
});
}

// Handle SpiceDB sync failures - transactions already rolled back
// Check for gRPC error characteristics (SpiceDB uses gRPC via @authzed/authzed-node)
const isGrpcError = error?.metadata !== undefined && typeof error?.code === 'number';
const mentionsSpiceDb = error?.message?.includes('SpiceDB');
if (mentionsSpiceDb || isGrpcError) {
logger.error(
{
error,
tenantId,
projectId: validatedProjectData.id,
userId,
},
'Failed to sync project to SpiceDB - database transactions rolled back'
);
throw createApiError({
code: 'internal_server_error',
message: 'Failed to set up project authorization. No changes were made to the database.',
});
}

// Re-throw other errors to be handled by the global error handler
throw error;
}
Expand Down Expand Up @@ -358,37 +404,57 @@ app.openapi(
// Use cached result from middleware (permission already checked there)
const isCreate = c.get('isProjectCreate') ?? false;

if (isCreate) {
// Project doesn't exist - create it with branch first
await createProjectMetadataAndBranch(
runDbClient,
configDb
)({
tenantId,
projectId,
createdBy: userId,
});

logger.info({ tenantId, projectId }, 'Created project with branch for PUT (upsert)');

// Checkout the project main branch
const projectMainBranch = getProjectMainBranchName(tenantId, projectId);
await checkoutBranch(configDb)({ branchName: projectMainBranch, autoCommitPending: true });
}

// Update/create the full project using server-side data layer operations
// Two-phase commit for creates, regular update for existing projects
const updatedProject: FullProjectSelect = isCreate
? await createFullProjectServerSide(configDb)({
scopes: { tenantId, projectId },
projectData: validatedProjectData,
? await runDbClient.transaction(async (runTx) => {
return await configDb.transaction(async (configTx) => {
// Phase 1: Database operations (within transactions)

// Create project with branch first
await createProjectMetadataAndBranch(
runTx,
configTx
)({
tenantId,
projectId,
createdBy: userId,
});

logger.info({ tenantId, projectId }, 'Created project with branch for PUT (upsert)');

// Checkout the project main branch
const projectMainBranch = getProjectMainBranchName(tenantId, projectId);
await checkoutBranch(configTx)({
branchName: projectMainBranch,
autoCommitPending: true,
});

// Create the full project config
const project = await createFullProjectServerSide(configTx)({
scopes: { tenantId, projectId },
projectData: validatedProjectData,
});

// Phase 2: Sync to SpiceDB (within transaction scope)
// If this fails, both transactions will rollback automatically
if (userId) {
await syncProjectToSpiceDb({
tenantId,
projectId,
creatorUserId: userId,
});
}

return project;
});
})
: await updateFullProjectServerSide(configDb)({
scopes: { tenantId, projectId },
projectData: validatedProjectData,
});

return c.json({ data: updatedProject }, isCreate ? 201 : 200);
} catch (error) {
} catch (error: any) {
if (error instanceof z.ZodError) {
throw createApiError({
code: 'bad_request',
Expand All @@ -403,6 +469,30 @@ app.openapi(
});
}

// Handle SpiceDB sync failures for creates - transactions already rolled back
const isCreate = c.get('isProjectCreate') ?? false;
if (isCreate) {
// Check for gRPC error characteristics (SpiceDB uses gRPC via @authzed/authzed-node)
const isGrpcError = error?.metadata !== undefined && typeof error?.code === 'number';
const mentionsSpiceDb = error?.message?.includes('SpiceDB');
if (mentionsSpiceDb || isGrpcError) {
logger.error(
{
error,
tenantId,
projectId,
userId,
},
'Failed to sync project to SpiceDB - database transactions rolled back'
);
throw createApiError({
code: 'internal_server_error',
message:
'Failed to set up project authorization. No changes were made to the database.',
});
}
}

throw createApiError({
code: 'internal_server_error',
message: error instanceof Error ? error.message : 'Failed to update project',
Expand Down Expand Up @@ -481,6 +571,27 @@ app.openapi(
});
}

// 4. Clean up SpiceDB relationships
// This removes all authorization relationships for the project
try {
await removeProjectFromSpiceDb({
tenantId,
projectId,
});
logger.info({ tenantId, projectId }, 'Removed project from SpiceDB');
} catch (spiceDbError) {
// Log but don't fail - the project data is already deleted
// This could leave orphaned auth relationships, but won't affect functionality
logger.warn(
{
spiceDbError,
tenantId,
projectId,
},
'Failed to remove project from SpiceDB - orphaned auth relationships may remain'
);
}

return c.body(null, 204);
} catch (error) {
if (error instanceof Error && error.message.includes('not found')) {
Expand Down
5 changes: 4 additions & 1 deletion agents-api/src/domains/run/agents/Agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ import { generateToolId } from '../utils/agent-operations';
import { ArtifactCreateSchema, ArtifactReferenceSchema } from '../utils/artifact-component-schema';
import { withJsonPostProcessing } from '../utils/json-postprocessor';
import { getCompressionConfigForModel } from '../utils/model-context-utils';
import { SchemaProcessor } from '../utils/SchemaProcessor';
import type { StreamHelper } from '../utils/stream-helpers';
import { getStreamHelper } from '../utils/stream-registry';
import {
Expand Down Expand Up @@ -3493,7 +3494,9 @@ ${output}`;
const componentSchemas: z.ZodType<any>[] = [];

this.config.dataComponents?.forEach((dc) => {
const propsSchema = dc.props ? z.fromJSONSchema(dc.props) : z.string();
// Normalize schema to ensure all properties are required (cross-provider compatibility)
const normalizedProps = dc.props ? SchemaProcessor.makeAllPropertiesRequired(dc.props) : null;
const propsSchema = normalizedProps ? z.fromJSONSchema(normalizedProps) : z.string();
componentSchemas.push(
z.object({
id: z.string(),
Expand Down
Loading