fix(v2): stop telling callers something the server did not do (#6676)

A works-as-advertised sweep of the v2 surface found one defect class in five
places: input is validated for shape, then its meaning is re-derived
independently by each consumer — so a filter compiles differently than it
validated, or a write commits and the response then reports failure.

Knowledge tag filters were validated once and re-parsed three times. The
document list read `Number()`, search read `parseFloat()`; the list matched
booleans case-insensitively, search compared against the literal `'true'`; the
list escaped LIKE metacharacters, search did not; and the date pattern was
tested against the untrimmed string the validator had already trimmed. Two of
those paths dropped the predicate entirely and answered 200 with the whole
knowledge base — on a billed endpoint. Values are now coerced once, where the
resolved field type is known, and both builders consume the result. A builder
that cannot compile an already-validated filter now raises instead of silently
widening the result set.

`PUT /api/v2/secrets/{name}` with `scope: personal` committed the secret and
then answered 500, because a user-global write was reported through a
workspace-scoped mirror lookup, and an org admin's inherited access has no
`permissions` row for the fan-out to find. The personal path no longer decides
success from a per-workspace mirror. The `workspaceId` descriptions said a
personal secret lives in one workspace; it does not, and they now say so.

A custom tool could be stored with a schema the read path cannot serialize —
`POST /workflows/import` and Copilot both wrote through name-only checks — so
one row made the whole workspace list 500, and a title-only PATCH committed,
audited, then reported failure. Every write now passes the same guard the
response schema is derived from.

`POST /api/v2/tables` accepted `workflowGroupId` on an initial column. Nothing
can populate it legitimately, and it made every later column-add and group-add
fail with no way to clear it. The key is refused at the boundary, and
`createTable` now runs the invariant every later mutation already runs, closing
the internal and v1 ingresses too. Those invariants moved to a leaf module:
reaching them through `workflow-columns` pulled the executable tool registry
into the tables page graph, taking it from 1,767 modules to 6,999.

An out-of-range upload part number answered 500 rather than the 400 its
published contract promises, because the throw happened above the route's
try/catch and was not an `HttpError`.
This commit is contained in:
Waleed
2026-08-13 14:19:41 -07:00
committed by GitHub
parent 0758df3682
commit e2b7335644
33 changed files with 1523 additions and 349 deletions
+8 -8
View File
@@ -1965,22 +1965,22 @@
"name": "workspaceId",
"in": "query",
"required": true,
"description": "Workspace in which the secret is available.",
"description": "Workspace the request is authorized against. A workspace secret is deleted from it; a personal secret is deleted for the caller in all of their workspaces.",
"schema": {
"type": "string",
"minLength": 1,
"description": "Workspace in which the secret is available."
"description": "Workspace the request is authorized against. A workspace secret is deleted from it; a personal secret is deleted for the caller in all of their workspaces."
}
},
{
"name": "scope",
"in": "query",
"required": true,
"description": "Whether the secret belongs to the workspace or the caller.",
"description": "Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace.",
"schema": {
"type": "string",
"enum": ["workspace", "personal"],
"description": "Whether the secret belongs to the workspace or the caller."
"description": "Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace."
}
}
],
@@ -4088,7 +4088,7 @@
"scope": {
"type": "string",
"enum": ["workspace", "personal"],
"description": "Whether the secret belongs to the workspace or the caller."
"description": "Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace."
},
"role": {
"type": "string",
@@ -4184,12 +4184,12 @@
"workspaceId": {
"type": "string",
"minLength": 1,
"description": "Workspace in which the secret is available."
"description": "Workspace the request is authorized against. A workspace secret is written to it; a personal secret is written to the caller and is available in all of their workspaces."
},
"scope": {
"type": "string",
"enum": ["workspace", "personal"],
"description": "Whether the secret belongs to the workspace or the caller."
"description": "Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace."
},
"value": {
"type": "string",
@@ -4224,7 +4224,7 @@
"scope": {
"type": "string",
"enum": ["workspace", "personal"],
"description": "Whether the secret belongs to the workspace or the caller."
"description": "Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace."
},
"deleted": {
"type": "boolean",
-4
View File
@@ -4370,10 +4370,6 @@
"description": "ISO 4217 code for currency columns.",
"type": "string",
"pattern": "^[A-Za-z]{3}$"
},
"workflowGroupId": {
"description": "Workflow group initially associated with the column.",
"type": "string"
}
},
"required": ["name", "type"],
@@ -29,6 +29,7 @@ vi.mock('@/lib/uploads/upload-session/service', () => ({
verifyUploadSessionToken: mockVerifyUploadSessionToken,
}))
import { OrchestrationError } from '@/lib/core/orchestration/types'
import { PUT } from '@/app/api/v2/uploads/[uploadId]/parts/[partNumber]/route'
const SESSION = {
@@ -37,6 +38,8 @@ const SESSION = {
method: 'multipart',
status: 'uploading',
expiresAt: new Date('2999-01-01T00:00:00.000Z'),
partSize: 3,
partCount: 2,
} as const
describe('PUT /api/v2/uploads/[uploadId]/parts/[partNumber]', () => {
@@ -84,6 +87,41 @@ describe('PUT /api/v2/uploads/[uploadId]/parts/[partNumber]', () => {
expect(mockWriteLocalMultipartPart).not.toHaveBeenCalled()
})
/**
* The part number is a path segment of a session-scoped signed URL, so any
* holder of a legitimate part URL can address a part the session does not
* have. `expectedUploadPartSize` classifies that as a validation failure;
* `service.test.ts` pins that classification on the real implementation,
* which this suite mocks away.
*/
it('maps an out-of-range part number to the documented 400', async () => {
mockExpectedUploadPartSize.mockImplementation(() => {
throw new OrchestrationError('validation', 'partNumber must be between 1 and 2')
})
const response = await request({ partNumber: '99' })
expect(response.status).toBe(400)
await expect(response.json()).resolves.toEqual({
error: { code: 'BAD_REQUEST', message: 'partNumber must be between 1 and 2' },
})
expect(mockWriteLocalMultipartPart).not.toHaveBeenCalled()
})
it('still renders an unclassified part-size failure as a generic 500', async () => {
mockExpectedUploadPartSize.mockImplementation(() => {
throw new Error('unexpected')
})
const response = await request()
expect(response.status).toBe(500)
await expect(response.json()).resolves.toEqual({
error: { code: 'INTERNAL_ERROR', message: 'Internal server error' },
})
expect(mockWriteLocalMultipartPart).not.toHaveBeenCalled()
})
it('rejects expired upload sessions before writing the part', async () => {
mockVerifyUploadSessionToken.mockReturnValue({
...SESSION,
@@ -101,17 +139,21 @@ describe('PUT /api/v2/uploads/[uploadId]/parts/[partNumber]', () => {
})
})
function request(options?: { contentLength?: string | null }) {
function request(options?: { contentLength?: string | null; partNumber?: string }) {
const headers = new Headers({ 'Content-Type': 'application/octet-stream' })
if (options?.contentLength !== null) {
headers.set('Content-Length', options?.contentLength ?? '3')
}
const partNumber = options?.partNumber ?? '1'
return PUT(
new NextRequest('http://localhost:3000/api/v2/uploads/upload-1/parts/1?token=signed-token', {
method: 'PUT',
headers,
body: new Uint8Array([1, 2, 3]),
}),
{ params: Promise.resolve({ uploadId: 'upload-1', partNumber: '1' }) }
new NextRequest(
`http://localhost:3000/api/v2/uploads/upload-1/parts/${partNumber}?token=signed-token`,
{
method: 'PUT',
headers,
body: new Uint8Array([1, 2, 3]),
}
),
{ params: Promise.resolve({ uploadId: 'upload-1', partNumber }) }
)
}
@@ -12,7 +12,12 @@ import {
type UploadSessionRecord,
verifyUploadSessionToken,
} from '@/lib/uploads/upload-session/service'
import { v2Error, v2HttpError, v2UploadDataPlaneError } from '@/app/api/v2/lib/response'
import {
v2CaughtOrchestrationError,
v2Error,
v2HttpError,
v2UploadDataPlaneError,
} from '@/app/api/v2/lib/response'
interface LocalPartRouteParams {
params: Promise<{ uploadId: string; partNumber: string }>
@@ -60,7 +65,18 @@ export const PUT = withRouteHandler(
}
const { partNumber } = parsed.data.params
const expectedSize = expectedUploadPartSize(session, partNumber)
let expectedSize: number
try {
expectedSize = expectedUploadPartSize(session, partNumber)
} catch (error) {
// The part number is a path segment of a session-scoped signed URL, so a
// caller can address a part this session does not have. That refusal is a
// classified domain failure, and the data plane's generic 500 tail would
// otherwise render it as an internal error.
const classified = v2CaughtOrchestrationError(error)
if (classified) return classified
throw error
}
const contentLength = request.headers.get('content-length')
if (contentLength !== null && Number(contentLength) !== expectedSize) {
return v2Error('BAD_REQUEST', `Part ${partNumber} must contain exactly ${expectedSize} bytes`)
@@ -59,6 +59,27 @@ describe('v2 table column contracts', () => {
).toMatchObject({ success: true, data: { updates: { required: true } } })
})
/**
* v2 mints workflow group ids server-side and has no way to declare a group
* on the create body, so any id a caller supplied would name a group that
* does not exist. `createTable` does not check that, but every later schema
* mutation does accepting the field made the created table's columns and
* groups permanently unaddable, with nothing on the update body able to clear
* it.
*/
it('refuses a workflow group id on an initial column', () => {
const result = v2CreateTableBodySchema.safeParse({
workspaceId: WORKSPACE_ID,
name: 'contacts',
schema: {
columns: [{ name: 'email', type: 'string', workflowGroupId: 'wfg_does_not_exist' }],
},
})
expect(result.success).toBe(false)
expect(issueCodes(result.error?.issues ?? [])).toContain('unrecognized_keys')
})
it('keeps required in table responses for existing stored schemas', () => {
expect(
v2ApiTableSchema.safeParse({
+9 -3
View File
@@ -15,7 +15,9 @@ const SECRET_NAME_REGEX = /^[A-Za-z0-9_]+$/
export const v2SecretScopeSchema = z
.enum(['workspace', 'personal'])
.describe('Whether the secret belongs to the workspace or the caller.')
.describe(
'Whether the secret belongs to the workspace or to the caller. A personal secret belongs to the caller across every workspace, not to one workspace.'
)
export type V2SecretScope = z.output<typeof v2SecretScopeSchema>
export const v2SecretNameSchema = z
@@ -76,7 +78,9 @@ export type V2SecretParams = z.output<typeof v2SecretParamsSchema>
export const v2SetSecretBodySchema = z
.object({
workspaceId: workspaceIdSchema.describe('Workspace in which the secret is available.'),
workspaceId: workspaceIdSchema.describe(
'Workspace the request is authorized against. A workspace secret is written to it; a personal secret is written to the caller and is available in all of their workspaces.'
),
scope: v2SecretScopeSchema,
value: z
.string()
@@ -90,7 +94,9 @@ export type V2SetSecretBody = z.input<typeof v2SetSecretBodySchema>
export const v2DeleteSecretQuerySchema = z
.object({
workspaceId: workspaceIdSchema.describe('Workspace in which the secret is available.'),
workspaceId: workspaceIdSchema.describe(
'Workspace the request is authorized against. A workspace secret is deleted from it; a personal secret is deleted for the caller in all of their workspaces.'
),
scope: v2SecretScopeSchema,
})
.strict()
+11 -12
View File
@@ -390,24 +390,23 @@ export const v2TableColumnInputSchema = z
.strict()
.superRefine(refineColumnOptions)
const v2InitialTableColumnInputSchema = z
.object({
...v2TableColumnInputShape,
workflowGroupId: z
.string()
.optional()
.describe('Workflow group initially associated with the column.'),
})
.strict()
.superRefine(refineColumnOptions)
/**
* Initial columns take the same shape as every other v2 column input.
*
* They deliberately cannot name a workflow group: v2 has no way to declare one
* on this body and mints group ids server-side, so any id a caller supplied
* would necessarily dangle. A dangling `workflowGroupId` is a schema invariant
* violation, and `createTable` does not check it while every later schema
* mutation does so accepting the field made the table's own columns and
* groups permanently unaddable, with no update body field able to clear it.
*/
export const v2CreateTableBodySchema = v1CreateTableBodySchema
.omit({ folderId: true, schema: true })
.extend({
schema: z
.object({
columns: z
.array(v2InitialTableColumnInputSchema)
.array(v2TableColumnInputSchema)
.min(1, 'Table must have at least one column')
.max(
TABLE_LIMITS.MAX_COLUMNS_PER_TABLE,
+48 -2
View File
@@ -3,7 +3,7 @@ import { credential, credentialMember, permissions, workspace } from '@sim/db/sc
import { permissionSatisfies } from '@sim/platform-authz/workspace'
import { chunkArray } from '@sim/utils/helpers'
import { generateId } from '@sim/utils/id'
import { and, eq, inArray, isNotNull, isNull, notInArray, or, sql } from 'drizzle-orm'
import { and, asc, eq, inArray, isNotNull, isNull, notInArray, or, sql } from 'drizzle-orm'
import { acquireUserBillingIdentityLock } from '@/lib/billing/organizations/billing-identity-lock'
import type { DbOrTx } from '@/lib/db/types'
import {
@@ -552,7 +552,53 @@ export async function upsertPersonalEnvCredentialForUser(params: {
await db.transaction(upsert)
}
/** Deletes one caller-owned personal secret's credential metadata in every workspace. */
export interface PersonalEnvCredentialMetadata {
id: string
createdAt: Date
updatedAt: Date
}
/**
* Reads one caller-owned personal secret's credential metadata without scoping to
* a workspace.
*
* A personal secret is stored once per user; the `env_personal` credential rows
* are per-workspace mirrors, so a reader that needs the secret's own timestamps
* must not require a mirror in one particular workspace. The earliest mirror is
* the authoritative creation time later ones are written when the caller joins
* another workspace, long after the secret itself was created.
*/
export async function getPersonalEnvCredentialMetadata(params: {
userId: string
envKey: string
}): Promise<PersonalEnvCredentialMetadata | null> {
const [row] = await db
.select({
id: credential.id,
createdAt: credential.createdAt,
updatedAt: credential.updatedAt,
})
.from(credential)
.where(
and(
eq(credential.type, 'env_personal'),
eq(credential.envOwnerUserId, params.userId),
eq(credential.envKey, params.envKey)
)
)
.orderBy(asc(credential.createdAt))
.limit(1)
return row ?? null
}
/**
* Deletes one caller-owned personal secret's credential metadata in every workspace.
*
* Deliberately unscoped by workspace: the value being removed alongside it lives
* in the user-global `environment` row, so leaving mirrors behind in other
* workspaces would advertise a secret that no longer exists.
*/
export async function deletePersonalEnvCredentialForUser(params: {
userId: string
envKey: string
@@ -8,6 +8,8 @@ const { mocks } = vi.hoisted(() => ({
loadContext: vi.fn(),
resolvePermission: vi.fn(),
getByTitle: vi.fn(),
getWorkspaceTool: vi.fn(),
updateWorkspaceTool: vi.fn(),
upsert: vi.fn(),
audit: vi.fn(),
},
@@ -34,12 +36,12 @@ vi.mock('@/lib/workflows/custom-tools/operations', () => ({
deleteCustomTool: vi.fn(),
deleteWorkspaceCustomTool: vi.fn(),
getCustomToolById: vi.fn(),
getWorkspaceCustomTool: vi.fn(),
getWorkspaceCustomTool: mocks.getWorkspaceTool,
getWorkspaceCustomToolByTitle: mocks.getByTitle,
listCustomTools: vi.fn(),
listWorkspaceCustomTools: vi.fn(),
updateCustomTool: vi.fn(),
updateWorkspaceCustomTool: vi.fn(),
updateWorkspaceCustomTool: mocks.updateWorkspaceTool,
upsertCustomTools: mocks.upsert,
}))
@@ -47,6 +49,7 @@ import { CUSTOM_TOOL_DELEGATION_AUDIENCE } from '@/lib/custom-tools/application/
import {
createWorkspaceCustomToolUseCase,
saveWorkspaceCustomToolUseCase,
updateWorkspaceCustomToolUseCase,
} from '@/lib/custom-tools/application/use-cases'
const workspace = {
@@ -184,4 +187,66 @@ describe('custom tool application use cases', () => {
expect(mocks.audit).not.toHaveBeenCalled()
})
describe('public update against the shape the response publishes', () => {
const storableSchema = {
type: 'function',
function: {
name: 'lookup_order',
parameters: { type: 'object', properties: { id: { type: 'string' } } },
},
}
const session = { kind: 'session' as const, userId: 'user-1', sessionId: 'session-1' }
it('renames a tool whose stored schema can be published', async () => {
const stored = { ...tool, schema: storableSchema }
mocks.getWorkspaceTool.mockResolvedValueOnce(stored)
mocks.updateWorkspaceTool.mockResolvedValueOnce({ ...stored, title: 'renamed' })
const result = await updateWorkspaceCustomToolUseCase.execute({
principal: session,
input: { workspaceId: workspace.workspaceId, toolId: tool.id, title: 'renamed' },
})
expect(result.tool.title).toBe('renamed')
expect(mocks.updateWorkspaceTool).toHaveBeenCalledWith(
expect.objectContaining({ schema: storableSchema, code: tool.code })
)
})
it('refuses before committing when the stored schema cannot be published', async () => {
mocks.getWorkspaceTool.mockResolvedValueOnce({
...tool,
schema: { function: storableSchema.function },
})
await expect(
updateWorkspaceCustomToolUseCase.execute({
principal: session,
input: { workspaceId: workspace.workspaceId, toolId: tool.id, title: 'renamed' },
})
).rejects.toMatchObject({ code: 'validation' })
expect(mocks.updateWorkspaceTool).not.toHaveBeenCalled()
expect(mocks.audit).not.toHaveBeenCalled()
})
it('refuses a supplied schema that cannot be published', async () => {
mocks.getWorkspaceTool.mockResolvedValueOnce({ ...tool, schema: storableSchema })
await expect(
updateWorkspaceCustomToolUseCase.execute({
principal: session,
input: {
workspaceId: workspace.workspaceId,
toolId: tool.id,
schema: { ...storableSchema, type: 'object' },
},
})
).rejects.toMatchObject({ code: 'validation' })
expect(mocks.updateWorkspaceTool).not.toHaveBeenCalled()
expect(mocks.audit).not.toHaveBeenCalled()
})
})
})
@@ -11,6 +11,7 @@ import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application'
import { OrchestrationError } from '@/lib/core/orchestration/types'
import { customToolDelegationPolicy } from '@/lib/custom-tools/application/authorization'
import { customToolOperations } from '@/lib/custom-tools/application/operations'
import { assertStorableCustomToolSchema } from '@/lib/custom-tools/schema'
import { loadActiveWorkspaceContext } from '@/lib/uploads/contexts/workspace'
import {
type CustomToolSortBy,
@@ -261,12 +262,21 @@ export const updateWorkspaceCustomToolUseCase = defineAuthorizedWorkspaceUseCase
async execute({ input, context }) {
const title = input.title ?? context.tool.title
await ensureTitleAvailable(context, title)
const schema = input.schema ?? context.tool.schema
/**
* The merged schema, not only a supplied one: this surface parses the tool
* it returns, so an update that falls back to a stored schema the response
* cannot serialize used to commit and audit and only then fail telling
* the caller the write failed after it had succeeded. Refusing before the
* write makes that error true.
*/
assertStorableCustomToolSchema(schema)
try {
const tool = await updateWorkspaceCustomTool({
workspaceId: context.workspaceId,
toolId: context.tool.id,
title,
schema: input.schema ?? context.tool.schema,
schema,
code: input.code ?? context.tool.code,
})
if (!tool) throw new OrchestrationError('not_found', 'Custom tool not found')
@@ -303,6 +313,13 @@ export const updateAvailableCustomToolUseCase = defineAuthorizedWorkspaceUseCase
async execute({ principal, input, context }) {
const title = input.title ?? context.tool.title
await ensureTitleAvailable(context, title)
/**
* Only a supplied schema, unlike the public update above: this surface does
* not parse what it returns, so an edit that merely keeps a legacy stored
* schema still succeeds, while a caller-supplied one is held to the shape
* the public API has to publish.
*/
if (input.schema !== undefined) assertStorableCustomToolSchema(input.schema)
try {
const tool = await updateCustomTool({
workspaceId: context.workspaceId,
+32
View File
@@ -0,0 +1,32 @@
import { customToolSchemaSchema } from '@/lib/api/contracts/tools/custom'
import { OrchestrationError } from '@/lib/core/orchestration/types'
/**
* Storage invariant for the `custom_tools.schema` column.
*
* The column is published verbatim by the v2 custom tool surface, whose
* response declaration extends {@link customToolSchemaSchema} and is parsed on
* the way out. A stored schema that does not satisfy it cannot be serialized
* back at all: one such row fails the whole `GET /api/v2/custom-tools` page,
* and an update commits and audits before its own response parse throws, so the
* caller is told a write failed after it succeeded.
*
* Every writer is therefore held to the read shape here, against the same
* schema the response is built from, rather than each write path restating the
* check which is how the two drifted apart in the first place.
*/
export function isStorableCustomToolSchema(schema: unknown): boolean {
return customToolSchemaSchema.safeParse(schema).success
}
/** {@link isStorableCustomToolSchema} as a caller-fixable validation failure. */
export function assertStorableCustomToolSchema(schema: unknown): void {
const parsed = customToolSchemaSchema.safeParse(schema)
if (parsed.success) return
const issue = parsed.error.issues[0]
const path = issue?.path.join('.')
throw new OrchestrationError(
'validation',
`Invalid custom tool schema${path ? ` at ${path}` : ''}: ${issue?.message ?? 'does not match the published function declaration'}`
)
}
+3 -3
View File
@@ -99,6 +99,7 @@ import {
parseBooleanValue,
parseDateValue,
parseNumberValue,
uncompilableTagFilterError,
validateTagValue,
} from '@/lib/knowledge/tags/utils'
import type { ProcessedDocumentTags } from '@/lib/knowledge/types'
@@ -1690,9 +1691,8 @@ export async function getDocuments(
if (tagFilters && tagFilters.length > 0) {
for (const filter of tagFilters) {
const condition = buildTagFilterCondition(filter)
if (condition) {
whereConditions.push(condition)
}
if (!condition) throw uncompilableTagFilterError(filter)
whereConditions.push(condition)
}
}
@@ -2,7 +2,9 @@
* @vitest-environment node
*/
import { describe, expect, it } from 'vitest'
import { getDocuments } from '@/lib/knowledge/documents/service'
import { buildTagFilterCondition } from '@/lib/knowledge/documents/tag-filter'
import { validateTagValue } from '@/lib/knowledge/tags/utils'
/**
* The global `drizzle-orm` mock renders `sql` fragments to a `?`-placeholder
@@ -174,4 +176,62 @@ describe('buildTagFilterCondition', () => {
).toBeUndefined()
})
})
describe('agreement with the value the tag-value gate validated', () => {
it('compiles a date the gate trimmed rather than dropping the filter', () => {
expect(validateTagValue('due', ' 2026-04-21', 'date')).toBeNull()
const { sql, params } = rendered(
buildTagFilterCondition({
tagSlot: 'date1',
fieldType: 'date',
operator: 'eq',
value: ' 2026-04-21',
})
)
expect(sql).toBe('?::date = ?::date')
expect(params).toEqual(['date1', '2026-04-21'])
})
it('compiles a trimmed between bound too', () => {
const condition = buildTagFilterCondition({
tagSlot: 'date1',
fieldType: 'date',
operator: 'between',
value: '2026-04-01',
valueTo: ' 2026-04-30 ',
}) as unknown as { type: string; conditions: unknown[] }
expect(condition.type).toBe('and')
expect(rendered(condition.conditions[1] as never).params).toEqual(['date1', '2026-04-30'])
})
it('reads a boolean case-insensitively', () => {
expect(validateTagValue('flag', 'TRUE', 'boolean')).toBeNull()
expect(
buildTagFilterCondition({
tagSlot: 'boolean1',
fieldType: 'boolean',
operator: 'eq',
value: 'TRUE',
})
).toEqual({ type: 'eq', left: 'boolean1', right: true })
})
})
})
describe('getDocuments tag filters', () => {
it('raises rather than dropping a filter it cannot compile', async () => {
await expect(
getDocuments(
'kb-1',
{
limit: 10,
offset: 0,
tagFilters: [
{ tagSlot: 'not_a_real_slot', fieldType: 'text', operator: 'eq', value: 'x' },
],
},
'req-1'
)
).rejects.toThrow(/Tag filter on slot "not_a_real_slot" could not be applied/)
})
})
+20 -21
View File
@@ -1,6 +1,6 @@
import { document } from '@sim/db/schema'
import { and, eq, gt, gte, lt, lte, ne, type SQL, sql } from 'drizzle-orm'
import { parseBooleanValue } from '@/lib/knowledge/tags/utils'
import { coerceTagFilterValue, escapeLikePattern } from '@/lib/knowledge/tags/utils'
/**
* A single tag filter applied to a document list query.
@@ -33,27 +33,27 @@ const ALLOWED_TAG_SLOTS = new Set([
'boolean3',
])
const DATE_ONLY_PATTERN = /^\d{4}-\d{2}-\d{2}$/
function escapeLikePattern(s: string): string {
return s.replace(/\\/g, '\\\\').replace(/%/g, '\\%').replace(/_/g, '\\_')
}
/**
* Builds a SQL predicate for a single tag filter against the document table.
*
* Text comparisons are case-insensitive and date comparisons are evaluated on
* the calendar day, matching the semantics of the knowledge base search filter
* (`lib/knowledge/search/queries.ts`). Returns `undefined` when the slot,
* operator, or value is not usable so the caller can skip the condition.
* (`lib/knowledge/search/queries.ts`). The value is coerced by the same
* function the tag-value gate validates with, so a value that passed validation
* always compiles. Returns `undefined` only when the slot, operator, or value is
* genuinely unusable which, after validation, is a bug the caller reports
* rather than a predicate it may skip.
*/
export function buildTagFilterCondition(filter: TagFilterCondition): SQL | undefined {
if (!ALLOWED_TAG_SLOTS.has(filter.tagSlot)) return undefined
const col = document[filter.tagSlot as keyof typeof document]
const coerced = coerceTagFilterValue(filter.value, filter.fieldType)
if (!coerced.ok) return undefined
if (filter.fieldType === 'text') {
const v = String(filter.value ?? '')
const v = coerced.value as string
switch (filter.operator) {
case 'eq':
return sql`LOWER(${col}) = LOWER(${v})`
@@ -81,8 +81,7 @@ export function buildTagFilterCondition(filter: TagFilterCondition): SQL | undef
}
if (filter.fieldType === 'number') {
const num = Number(filter.value)
if (Number.isNaN(num)) return undefined
const num = coerced.value as number
switch (filter.operator) {
case 'eq':
return eq(col as typeof document.number1, num)
@@ -97,8 +96,10 @@ export function buildTagFilterCondition(filter: TagFilterCondition): SQL | undef
case 'lte':
return lte(col as typeof document.number1, num)
case 'between': {
const numTo = Number(filter.valueTo)
if (Number.isNaN(numTo)) return undefined
if (filter.valueTo === undefined || filter.valueTo === null) return undefined
const coercedTo = coerceTagFilterValue(filter.valueTo, 'number')
if (!coercedTo.ok) return undefined
const numTo = coercedTo.value as number
return and(
gte(col as typeof document.number1, num),
lte(col as typeof document.number1, numTo)
@@ -110,8 +111,7 @@ export function buildTagFilterCondition(filter: TagFilterCondition): SQL | undef
}
if (filter.fieldType === 'date') {
const v = String(filter.value ?? '')
if (!DATE_ONLY_PATTERN.test(v)) return undefined
const v = coerced.value as string
switch (filter.operator) {
case 'eq':
return sql`${col}::date = ${v}::date`
@@ -126,8 +126,9 @@ export function buildTagFilterCondition(filter: TagFilterCondition): SQL | undef
case 'lte':
return sql`${col}::date <= ${v}::date`
case 'between': {
const valueTo = String(filter.valueTo ?? '')
if (!DATE_ONLY_PATTERN.test(valueTo)) return undefined
const coercedTo = coerceTagFilterValue(filter.valueTo, 'date')
if (!coercedTo.ok) return undefined
const valueTo = coercedTo.value as string
return and(sql`${col}::date >= ${v}::date`, sql`${col}::date <= ${valueTo}::date`)
}
default:
@@ -136,9 +137,7 @@ export function buildTagFilterCondition(filter: TagFilterCondition): SQL | undef
}
if (filter.fieldType === 'boolean') {
const boolVal =
typeof filter.value === 'boolean' ? filter.value : parseBooleanValue(String(filter.value))
if (boolVal === null) return undefined
const boolVal = coerced.value as boolean
switch (filter.operator) {
case 'eq':
return eq(col as typeof document.boolean1, boolVal)
@@ -0,0 +1,171 @@
/**
* @vitest-environment node
*/
import { describe, expect, it } from 'vitest'
import { getStructuredTagFilters } from '@/lib/knowledge/search/queries'
import type { StructuredFilter } from '@/lib/knowledge/types'
/**
* The builder only reads `embeddingTable[tagSlot]`, so a slot-to-name map stands
* in for the real table and makes each rendered parameter readable.
*/
const embeddingTable = {
tag1: 'tag1',
number1: 'number1',
date1: 'date1',
boolean1: 'boolean1',
}
/**
* The global `drizzle-orm` mock renders `sql` fragments to a `?`-placeholder
* string via `toSQL()`, so we can assert the exact predicate each filter builds.
*/
function renderOne(filters: StructuredFilter[]) {
const conditions = getStructuredTagFilters(filters, embeddingTable)
expect(conditions).toHaveLength(1)
return (conditions[0] as unknown as { toSQL: () => { sql: string; params: unknown[] } }).toSQL()
}
describe('getStructuredTagFilters', () => {
describe('agreement with the value the gate validated', () => {
it('compiles a number the gate read as 0 rather than dropping the filter', () => {
const { sql, params } = renderOne([
{ tagSlot: 'number1', fieldType: 'number', operator: 'eq', value: '' },
])
expect(sql).toBe('? = ?')
expect(params).toEqual(['number1', 0])
})
it('reads a number in the same base the gate validated', () => {
const { params } = renderOne([
{ tagSlot: 'number1', fieldType: 'number', operator: 'eq', value: '0x10' },
])
expect(params).toEqual(['number1', 16])
})
it('reads a boolean case-insensitively instead of inverting it', () => {
const { params } = renderOne([
{ tagSlot: 'boolean1', fieldType: 'boolean', operator: 'eq', value: 'TRUE' },
])
expect(params).toEqual(['boolean1', true])
})
it('trims a date the gate trimmed rather than dropping the filter', () => {
const { sql, params } = renderOne([
{ tagSlot: 'date1', fieldType: 'date', operator: 'eq', value: ' 2026-08-13' },
])
expect(sql).toBe('?::date = ?::date')
expect(params).toEqual(['date1', '2026-08-13'])
})
it('escapes LIKE metacharacters so a typed % is not a wildcard', () => {
const { sql, params } = renderOne([
{ tagSlot: 'tag1', fieldType: 'text', operator: 'contains', value: '50%off' },
])
expect(sql).toBe("LOWER(?) LIKE LOWER(?) ESCAPE '\\'")
expect(params).toEqual(['tag1', '%50\\%off%'])
})
it('escapes LIKE metacharacters for every text operator that uses LIKE', () => {
for (const operator of ['not_contains', 'starts_with', 'ends_with']) {
const { sql, params } = renderOne([
{ tagSlot: 'tag1', fieldType: 'text', operator, value: 'a_b' },
])
expect(sql).toContain("ESCAPE '\\'")
expect(params[1]).toContain('a\\_b')
}
})
})
describe('a filter that cannot compile is reported, never skipped', () => {
it('raises instead of returning no predicate at all', () => {
expect(() =>
getStructuredTagFilters(
[{ tagSlot: 'not_a_slot', fieldType: 'text', operator: 'eq', value: 'x' }],
embeddingTable
)
).toThrow(/Tag filter on slot "not_a_slot" could not be applied/)
})
it('raises rather than silently widening a multi-filter search', () => {
expect(() =>
getStructuredTagFilters(
[
{ tagSlot: 'tag1', fieldType: 'text', operator: 'eq', value: 'ok' },
{ tagSlot: 'not_a_slot', fieldType: 'text', operator: 'eq', value: 'x' },
],
embeddingTable
)
).toThrow(/could not be applied/)
})
})
describe('a correct filter still compiles to the predicate it always did', () => {
it('text eq', () => {
const { sql, params } = renderOne([
{ tagSlot: 'tag1', fieldType: 'text', operator: 'eq', value: 'Billing' },
])
expect(sql).toBe('LOWER(?) = LOWER(?)')
expect(params).toEqual(['tag1', 'Billing'])
})
it('number gte', () => {
const { sql, params } = renderOne([
{ tagSlot: 'number1', fieldType: 'number', operator: 'gte', value: '42' },
])
expect(sql).toBe('? >= ?')
expect(params).toEqual(['number1', 42])
})
it('number between', () => {
const { sql, params } = renderOne([
{
tagSlot: 'number1',
fieldType: 'number',
operator: 'between',
value: '1',
valueTo: '9',
},
])
expect(sql).toBe('? >= ? AND ? <= ?')
expect(params).toEqual(['number1', 1, 'number1', 9])
})
it('date between', () => {
const { sql, params } = renderOne([
{
tagSlot: 'date1',
fieldType: 'date',
operator: 'between',
value: '2026-01-01',
valueTo: '2026-12-31',
},
])
expect(sql).toBe('?::date >= ?::date AND ?::date <= ?::date')
expect(params).toEqual(['date1', '2026-01-01', 'date1', '2026-12-31'])
})
it('boolean neq', () => {
const { sql, params } = renderOne([
{ tagSlot: 'boolean1', fieldType: 'boolean', operator: 'neq', value: 'false' },
])
expect(sql).toBe('? != ?')
expect(params).toEqual(['boolean1', false])
})
it('ORs two filters on the same slot and keeps them one condition', () => {
const conditions = getStructuredTagFilters(
[
{ tagSlot: 'tag1', fieldType: 'text', operator: 'eq', value: 'a' },
{ tagSlot: 'tag1', fieldType: 'text', operator: 'eq', value: 'b' },
],
embeddingTable
)
expect(conditions).toHaveLength(1)
const joined = (conditions[0] as unknown as { values: unknown[] }).values[0] as {
toSQL: () => { sql: string; params: unknown[] }
}
expect(joined.toSQL().params).toEqual(['tag1', 'a', 'tag1', 'b'])
})
})
})
+41 -26
View File
@@ -3,6 +3,11 @@ import { document, embedding } from '@sim/db/schema'
import { createLogger } from '@sim/logger'
import { getErrorMessage } from '@sim/utils/errors'
import { and, eq, inArray, isNull, type SQL, sql } from 'drizzle-orm'
import {
coerceTagFilterValue,
escapeLikePattern,
uncompilableTagFilterError,
} from '@/lib/knowledge/tags/utils'
import type { StructuredFilter } from '@/lib/knowledge/types'
const logger = createLogger('KnowledgeSearchQueries')
@@ -170,20 +175,23 @@ function buildFilterCondition(filter: StructuredFilter, embeddingTable: any) {
// Handle text operators
if (fieldType === 'text') {
const stringValue = String(value)
const coerced = coerceTagFilterValue(value, 'text')
if (!coerced.ok) return null
const stringValue = coerced.value as string
const escaped = escapeLikePattern(stringValue)
switch (operator) {
case 'eq':
return sql`LOWER(${column}) = LOWER(${stringValue})`
case 'neq':
return sql`LOWER(${column}) != LOWER(${stringValue})`
case 'contains':
return sql`LOWER(${column}) LIKE LOWER(${`%${stringValue}%`})`
return sql`LOWER(${column}) LIKE LOWER(${`%${escaped}%`}) ESCAPE '\\'`
case 'not_contains':
return sql`LOWER(${column}) NOT LIKE LOWER(${`%${stringValue}%`})`
return sql`LOWER(${column}) NOT LIKE LOWER(${`%${escaped}%`}) ESCAPE '\\'`
case 'starts_with':
return sql`LOWER(${column}) LIKE LOWER(${`${stringValue}%`})`
return sql`LOWER(${column}) LIKE LOWER(${`${escaped}%`}) ESCAPE '\\'`
case 'ends_with':
return sql`LOWER(${column}) LIKE LOWER(${`%${stringValue}`})`
return sql`LOWER(${column}) LIKE LOWER(${`%${escaped}`}) ESCAPE '\\'`
default:
return sql`LOWER(${column}) = LOWER(${stringValue})`
}
@@ -191,8 +199,9 @@ function buildFilterCondition(filter: StructuredFilter, embeddingTable: any) {
// Handle number operators
if (fieldType === 'number') {
const numValue = typeof value === 'number' ? value : Number.parseFloat(String(value))
if (Number.isNaN(numValue)) return null
const coerced = coerceTagFilterValue(value, 'number')
if (!coerced.ok) return null
const numValue = coerced.value as number
switch (operator) {
case 'eq':
@@ -209,10 +218,9 @@ function buildFilterCondition(filter: StructuredFilter, embeddingTable: any) {
return sql`${column} <= ${numValue}`
case 'between':
if (valueTo !== undefined) {
const numValueTo =
typeof valueTo === 'number' ? valueTo : Number.parseFloat(String(valueTo))
if (Number.isNaN(numValueTo)) return sql`${column} = ${numValue}`
return sql`${column} >= ${numValue} AND ${column} <= ${numValueTo}`
const coercedTo = coerceTagFilterValue(valueTo, 'number')
if (!coercedTo.ok) return sql`${column} = ${numValue}`
return sql`${column} >= ${numValue} AND ${column} <= ${coercedTo.value as number}`
}
return sql`${column} = ${numValue}`
default:
@@ -222,11 +230,9 @@ function buildFilterCondition(filter: StructuredFilter, embeddingTable: any) {
// Handle date operators - expects YYYY-MM-DD format from frontend
if (fieldType === 'date') {
const dateStr = String(value)
// Validate YYYY-MM-DD format
if (!/^\d{4}-\d{2}-\d{2}$/.test(dateStr)) {
return null
}
const coerced = coerceTagFilterValue(value, 'date')
if (!coerced.ok) return null
const dateStr = coerced.value as string
switch (operator) {
case 'eq':
@@ -243,10 +249,11 @@ function buildFilterCondition(filter: StructuredFilter, embeddingTable: any) {
return sql`${column}::date <= ${dateStr}::date`
case 'between':
if (valueTo !== undefined) {
const dateStrTo = String(valueTo)
if (!/^\d{4}-\d{2}-\d{2}$/.test(dateStrTo)) {
const coercedTo = coerceTagFilterValue(valueTo, 'date')
if (!coercedTo.ok) {
return sql`${column}::date = ${dateStr}::date`
}
const dateStrTo = coercedTo.value as string
return sql`${column}::date >= ${dateStr}::date AND ${column}::date <= ${dateStrTo}::date`
}
return sql`${column}::date = ${dateStr}::date`
@@ -257,7 +264,9 @@ function buildFilterCondition(filter: StructuredFilter, embeddingTable: any) {
// Handle boolean operators
if (fieldType === 'boolean') {
const boolValue = value === true || value === 'true'
const coerced = coerceTagFilterValue(value, 'boolean')
if (!coerced.ok) return null
const boolValue = coerced.value as boolean
switch (operator) {
case 'eq':
return sql`${column} = ${boolValue}`
@@ -276,8 +285,14 @@ function buildFilterCondition(filter: StructuredFilter, embeddingTable: any) {
* Build SQL conditions from structured filters with operator support
* - Same tag multiple times: OR logic
* - Different tags: AND logic
*
* Every filter reaching here has already been validated, so one that fails to
* compile is a defect rather than a predicate to skip. Skipping it dropped the
* tag term from the WHERE clause entirely and answered a filtered search with
* the whole knowledge base under a 200 and search is billed, so the caller
* paid for the widened scan. It is reported as a validation failure instead.
*/
function getStructuredTagFilters(filters: StructuredFilter[], embeddingTable: any) {
export function getStructuredTagFilters(filters: StructuredFilter[], embeddingTable: any) {
// Group filters by tagSlot
const filtersBySlot = new Map<string, StructuredFilter[]>()
for (const filter of filters) {
@@ -291,12 +306,12 @@ function getStructuredTagFilters(filters: StructuredFilter[], embeddingTable: an
// Build conditions: OR within same slot, AND across different slots
const conditions: ReturnType<typeof sql>[] = []
for (const [slot, slotFilters] of filtersBySlot) {
const slotConditions = slotFilters
.map((f) => buildFilterCondition(f, embeddingTable))
.filter((c): c is ReturnType<typeof sql> => c !== null)
if (slotConditions.length === 0) continue
for (const [, slotFilters] of filtersBySlot) {
const slotConditions = slotFilters.map((f) => {
const condition = buildFilterCondition(f, embeddingTable)
if (condition === null) throw uncompilableTagFilterError(f)
return condition
})
if (slotConditions.length === 1) {
// Single condition for this slot
+70
View File
@@ -0,0 +1,70 @@
/**
* @vitest-environment node
*/
import { describe, expect, it } from 'vitest'
import { coerceTagFilterValue, validateTagValue } from '@/lib/knowledge/tags/utils'
describe('coerceTagFilterValue', () => {
it('accepts exactly what validateTagValue accepts', () => {
const cases: Array<[string, 'number' | 'date' | 'boolean']> = [
['', 'number'],
['0x10', 'number'],
['12.5', 'number'],
['abc', 'number'],
['TRUE', 'boolean'],
['False', 'boolean'],
['yes', 'boolean'],
[' 2026-08-13', 'date'],
['2026-08-13', 'date'],
['2026-02-31', 'date'],
['13-08-2026', 'date'],
]
for (const [value, fieldType] of cases) {
expect(coerceTagFilterValue(value, fieldType).ok, `${fieldType} "${value}"`).toBe(
validateTagValue('tag', value, fieldType) === null
)
}
})
it('trims a date the same way the gate does', () => {
expect(coerceTagFilterValue(' 2026-08-13 ', 'date')).toEqual({ ok: true, value: '2026-08-13' })
})
it('reads a boolean case-insensitively', () => {
expect(coerceTagFilterValue('TRUE', 'boolean')).toEqual({ ok: true, value: true })
expect(coerceTagFilterValue('False', 'boolean')).toEqual({ ok: true, value: false })
expect(coerceTagFilterValue(true, 'boolean')).toEqual({ ok: true, value: true })
})
it('reads a number with the same base the gate validates with', () => {
expect(coerceTagFilterValue('0x10', 'number')).toEqual({ ok: true, value: 16 })
expect(coerceTagFilterValue('', 'number')).toEqual({ ok: true, value: 0 })
expect(coerceTagFilterValue(12.5, 'number')).toEqual({ ok: true, value: 12.5 })
})
it('leaves a text value untouched so a search for padded text still matches', () => {
expect(coerceTagFilterValue(' padded ', 'text')).toEqual({ ok: true, value: ' padded ' })
})
})
describe('validateTagValue', () => {
it('keeps its distinct messages per failure', () => {
expect(validateTagValue('flag', 'yes', 'boolean')).toBe(
'Tag "flag" expects a boolean value (true/false), but received "yes"'
)
expect(validateTagValue('score', 'abc', 'number')).toBe(
'Tag "score" expects a number value, but received "abc"'
)
expect(validateTagValue('due', '13-08-2026', 'date')).toBe(
'Tag "due" expects a date in YYYY-MM-DD format, but received "13-08-2026"'
)
expect(validateTagValue('due', '2026-02-31', 'date')).toBe(
'Tag "due" has an invalid date: "2026-02-31"'
)
})
it('does not constrain text or unknown field types', () => {
expect(validateTagValue('name', 'anything', 'text')).toBeNull()
expect(validateTagValue('name', 'anything', 'json')).toBeNull()
})
})
+105 -31
View File
@@ -1,40 +1,114 @@
import { OrchestrationError } from '@/lib/core/orchestration/types'
const DATE_ONLY_PATTERN = /^\d{4}-\d{2}-\d{2}$/
/** Field types a tag filter value can be compiled for. */
export type TagFilterFieldType = 'text' | 'number' | 'date' | 'boolean'
export type TagFilterValueCoercion =
| { ok: true; value: string | number | boolean }
| { ok: false; reason: 'number' | 'date-format' | 'date-calendar' | 'boolean' }
/**
* Coerces a raw tag filter value into the typed value its SQL predicate
* compares against.
*
* This is the only place a filter value is parsed. `validateTagValue` reports
* the failures it returns, and both filter builders the document list
* (`lib/knowledge/documents/tag-filter.ts`) and search
* (`lib/knowledge/search/queries.ts`) consume the value it produces. Sharing
* one parse is what makes "the gate accepted this value" and "the predicate
* compiled that same value" a single fact. Three independent parses used to
* disagree: a boolean `"TRUE"` passed the gate, matched `= true` on the
* document list and `= false` on search, and a date carrying a leading space
* passed the gate (which trimmed) but compiled to no predicate at all on the
* document list (which did not), returning the whole knowledge base under a
* 200.
*/
export function coerceTagFilterValue(
value: unknown,
fieldType: TagFilterFieldType
): TagFilterValueCoercion {
switch (fieldType) {
case 'text':
return { ok: true, value: String(value ?? '') }
case 'number': {
const numValue = typeof value === 'number' ? value : Number(String(value ?? '').trim())
if (Number.isNaN(numValue)) return { ok: false, reason: 'number' }
return { ok: true, value: numValue }
}
case 'date': {
const stringValue = String(value ?? '').trim()
if (!DATE_ONLY_PATTERN.test(stringValue)) return { ok: false, reason: 'date-format' }
const [year, month, day] = stringValue.split('-').map(Number)
const date = new Date(year, month - 1, day)
if (date.getFullYear() !== year || date.getMonth() !== month - 1 || date.getDate() !== day) {
return { ok: false, reason: 'date-calendar' }
}
return { ok: true, value: stringValue }
}
case 'boolean': {
if (typeof value === 'boolean') return { ok: true, value }
const lowerValue = String(value ?? '')
.trim()
.toLowerCase()
if (lowerValue === 'true') return { ok: true, value: true }
if (lowerValue === 'false') return { ok: true, value: false }
return { ok: false, reason: 'boolean' }
}
}
}
/**
* Escapes the LIKE metacharacters in a tag filter value so a `%` or `_` a
* caller typed matches itself instead of acting as a wildcard. Both filter
* builders pair this with `ESCAPE '\'`.
*/
export function escapeLikePattern(value: string): string {
return value.replace(/\\/g, '\\\\').replace(/%/g, '\\%').replace(/_/g, '\\_')
}
/**
* The failure raised when a validated tag filter still fails to compile to a
* SQL predicate.
*
* Both filter builders return "no predicate" for a filter they cannot compile,
* and both consumers used to skip it. A skipped predicate does not narrow
* anything, so a filtered read answered 200 with every row the caller could
* see. Once validation has passed, that state is a defect in this code not an
* input a surface may quietly ignore so it is surfaced as a validation
* failure the caller can act on.
*/
export function uncompilableTagFilterError(filter: {
tagSlot: string
fieldType: string
operator: string
}): OrchestrationError {
return new OrchestrationError(
'validation',
`Tag filter on slot "${filter.tagSlot}" could not be applied (field type "${filter.fieldType}", operator "${filter.operator}")`
)
}
/**
* Validate a tag value against its expected field type
* Returns an error message if invalid, or null if valid
*/
export function validateTagValue(tagName: string, value: string, fieldType: string): string | null {
const stringValue = String(value).trim()
if (fieldType !== 'boolean' && fieldType !== 'number' && fieldType !== 'date') return null
switch (fieldType) {
case 'boolean': {
const lowerValue = stringValue.toLowerCase()
if (lowerValue !== 'true' && lowerValue !== 'false') {
return `Tag "${tagName}" expects a boolean value (true/false), but received "${value}"`
}
return null
}
case 'number': {
const numValue = Number(stringValue)
if (Number.isNaN(numValue)) {
return `Tag "${tagName}" expects a number value, but received "${value}"`
}
return null
}
case 'date': {
// Check format first
if (!/^\d{4}-\d{2}-\d{2}$/.test(stringValue)) {
return `Tag "${tagName}" expects a date in YYYY-MM-DD format, but received "${value}"`
}
// Validate the date is actually valid (e.g., reject 2024-02-31)
const [year, month, day] = stringValue.split('-').map(Number)
const date = new Date(year, month - 1, day)
if (date.getFullYear() !== year || date.getMonth() !== month - 1 || date.getDate() !== day) {
return `Tag "${tagName}" has an invalid date: "${value}"`
}
return null
}
default:
return null
const coerced = coerceTagFilterValue(value, fieldType)
if (coerced.ok) return null
switch (coerced.reason) {
case 'boolean':
return `Tag "${tagName}" expects a boolean value (true/false), but received "${value}"`
case 'number':
return `Tag "${tagName}" expects a number value, but received "${value}"`
case 'date-format':
return `Tag "${tagName}" expects a date in YYYY-MM-DD format, but received "${value}"`
case 'date-calendar':
return `Tag "${tagName}" has an invalid date: "${value}"`
}
}
@@ -3,7 +3,7 @@
*/
import type { Principal } from '@sim/auth/principal'
import { beforeEach, describe, expect, it, vi } from 'vitest'
import type { SetSecretInput } from '@/lib/secrets/application/use-cases'
import type { DeleteSecretInput, SetSecretInput } from '@/lib/secrets/application/use-cases'
const { mocks } = vi.hoisted(() => ({
mocks: {
@@ -11,7 +11,10 @@ const { mocks } = vi.hoisted(() => ({
resolvePermission: vi.fn(),
workspaceAccess: vi.fn(),
keyAccess: vi.fn(),
personalMetadata: vi.fn(),
setWorkspace: vi.fn(),
setPersonal: vi.fn(),
deletePersonal: vi.fn(),
listCredentials: vi.fn(),
audit: vi.fn(),
},
@@ -38,18 +41,19 @@ vi.mock('@/lib/workspaces/permissions/utils', () => ({
}))
vi.mock('@/lib/credentials/environment', () => ({
getWorkspaceEnvKeyAdminAccess: mocks.keyAccess,
getPersonalEnvCredentialMetadata: mocks.personalMetadata,
}))
vi.mock('@/lib/credentials/queries', () => ({
listVisibleWorkspaceCredentials: mocks.listCredentials,
}))
vi.mock('@/lib/credentials/secret-values', () => ({
deletePersonalSecret: vi.fn(),
deletePersonalSecret: mocks.deletePersonal,
deleteWorkspaceSecret: vi.fn(),
setPersonalSecret: vi.fn(),
setPersonalSecret: mocks.setPersonal,
setWorkspaceSecret: mocks.setWorkspace,
}))
import { setSecretUseCase } from '@/lib/secrets/application/use-cases'
import { deleteSecretUseCase, setSecretUseCase } from '@/lib/secrets/application/use-cases'
const workspace = {
workspaceId: 'workspace-1',
@@ -74,6 +78,18 @@ const secret = {
role: 'admin' as const,
}
const personalUpdatedAt = new Date('2026-02-01T00:00:00Z')
const personalSecret = {
...secret,
id: 'secret-2',
type: 'env_personal' as const,
displayName: 'OPENAI_API_KEY',
envKey: 'OPENAI_API_KEY',
envOwnerUserId: 'user-1',
}
const session = { kind: 'session', userId: 'user-1', sessionId: 'session-1' } as const
describe('secret application use cases', () => {
beforeEach(() => {
vi.clearAllMocks()
@@ -81,7 +97,10 @@ describe('secret application use cases', () => {
mocks.resolvePermission.mockResolvedValue('write')
mocks.workspaceAccess.mockResolvedValue({ hasAccess: true, canWrite: true, canAdmin: false })
mocks.keyAccess.mockResolvedValue({ knownKeys: new Set(), adminKeys: new Set() })
mocks.setWorkspace.mockResolvedValue({ created: true })
mocks.setWorkspace.mockResolvedValue({ created: true, updatedAt: personalUpdatedAt })
mocks.setPersonal.mockResolvedValue({ created: true, updatedAt: personalUpdatedAt })
mocks.personalMetadata.mockResolvedValue(null)
mocks.deletePersonal.mockResolvedValue(true)
mocks.listCredentials.mockResolvedValue({ data: [secret], nextCursorKeys: null })
})
@@ -143,4 +162,122 @@ describe('secret application use cases', () => {
)
expect(JSON.stringify(mocks.audit.mock.calls)).not.toContain('secret-value')
})
it('still fails a workspace write whose metadata never materialized', async () => {
mocks.listCredentials.mockResolvedValue({ data: [], nextCursorKeys: null })
await expect(
setSecretUseCase.execute({
principal: session,
input: {
workspaceId: workspace.workspaceId,
name: secret.envKey,
scope: 'workspace',
value: 'secret-value',
},
})
).rejects.toThrow(/workspace:STRIPE_API_KEY/)
})
it('reports a committed personal write when this workspace holds no mirror', async () => {
mocks.resolvePermission.mockResolvedValue('admin')
mocks.workspaceAccess.mockResolvedValue({ hasAccess: true, canWrite: true, canAdmin: true })
mocks.listCredentials.mockResolvedValue({ data: [], nextCursorKeys: null })
const result = await setSecretUseCase.execute({
principal: session,
input: {
workspaceId: workspace.workspaceId,
name: personalSecret.envKey,
scope: 'personal',
value: 'secret-value',
},
})
expect(mocks.setPersonal).toHaveBeenCalledWith({
userId: 'user-1',
name: personalSecret.envKey,
value: 'secret-value',
})
expect(result.created).toBe(true)
expect(result.secret).toMatchObject({
type: 'env_personal',
envKey: personalSecret.envKey,
envOwnerUserId: 'user-1',
role: 'admin',
createdAt: personalUpdatedAt,
updatedAt: personalUpdatedAt,
})
})
it('dates a mirrorless personal write from the secret the caller already owns', async () => {
const createdAt = new Date('2025-06-01T00:00:00Z')
mocks.listCredentials.mockResolvedValue({ data: [], nextCursorKeys: null })
mocks.personalMetadata.mockResolvedValue({
id: 'secret-2',
createdAt,
updatedAt: createdAt,
})
mocks.setPersonal.mockResolvedValue({ created: false, updatedAt: personalUpdatedAt })
const result = await setSecretUseCase.execute({
principal: session,
input: {
workspaceId: workspace.workspaceId,
name: personalSecret.envKey,
scope: 'personal',
value: 'secret-value',
},
})
expect(mocks.personalMetadata).toHaveBeenCalledWith({
userId: 'user-1',
envKey: personalSecret.envKey,
})
expect(result.created).toBe(false)
expect(result.secret).toMatchObject({
id: 'secret-2',
createdAt,
updatedAt: personalUpdatedAt,
})
})
it('prefers this workspace mirror for a personal write when one exists', async () => {
mocks.listCredentials.mockResolvedValue({ data: [personalSecret], nextCursorKeys: null })
const result = await setSecretUseCase.execute({
principal: session,
input: {
workspaceId: workspace.workspaceId,
name: personalSecret.envKey,
scope: 'personal',
value: 'secret-value',
},
})
expect(result.secret).toBe(personalSecret)
expect(mocks.personalMetadata).not.toHaveBeenCalled()
})
it('deletes a personal secret for the caller rather than for one workspace', async () => {
const execute = deleteSecretUseCase.execute as (args: {
principal: Principal
input: DeleteSecretInput
}) => Promise<{ name: string; scope: string }>
const result = await execute({
principal: session,
input: {
workspaceId: workspace.workspaceId,
name: personalSecret.envKey,
scope: 'personal',
},
})
expect(mocks.deletePersonal).toHaveBeenCalledWith({
userId: 'user-1',
name: personalSecret.envKey,
})
expect(result).toEqual({ name: personalSecret.envKey, scope: 'personal' })
})
})
+92 -21
View File
@@ -4,7 +4,10 @@ import type { CursorKey, ListSortOrder } from '@/lib/api/list-query'
import { defineAuthorizedWorkspaceUseCase } from '@/lib/core/application'
import { ForbiddenOperationError } from '@/lib/core/application/forbidden'
import { OrchestrationError } from '@/lib/core/orchestration/types'
import { getWorkspaceEnvKeyAdminAccess } from '@/lib/credentials/environment'
import {
getPersonalEnvCredentialMetadata,
getWorkspaceEnvKeyAdminAccess,
} from '@/lib/credentials/environment'
import {
listVisibleWorkspaceCredentials,
type VisibleWorkspaceCredential,
@@ -115,29 +118,90 @@ async function requireWorkspaceSecretMutationAccess(params: {
}
}
async function getSecretMetadata(params: {
async function findSecretMetadata(params: {
workspaceId: string
userId: string
scope: SecretScope
name: string
}): Promise<VisibleWorkspaceCredential> {
}): Promise<VisibleWorkspaceCredential | null> {
const { data } = await listSecretMetadata({
...params,
search: params.name,
sortBy: 'name',
sortOrder: 'asc',
})
const row = data.find(
(candidate) =>
candidate.envKey === params.name &&
(params.scope === 'workspace'
? candidate.type === 'env_workspace'
: candidate.type === 'env_personal' && candidate.envOwnerUserId === params.userId)
return (
data.find(
(candidate) =>
candidate.envKey === params.name &&
(params.scope === 'workspace'
? candidate.type === 'env_workspace'
: candidate.type === 'env_personal' && candidate.envOwnerUserId === params.userId)
) ?? null
)
if (!row) throw new Error(`Secret metadata was not created for ${params.scope}:${params.name}`)
}
async function getWorkspaceSecretMetadata(params: {
workspaceId: string
userId: string
name: string
}): Promise<VisibleWorkspaceCredential> {
const row = await findSecretMetadata({ ...params, scope: 'workspace' })
if (!row) throw new Error(`Secret metadata was not created for workspace:${params.name}`)
return row
}
/**
* Resolves the metadata a personal-scope write reports back.
*
* A personal secret is stored once per user, and its `env_personal` credential
* rows are per-workspace mirrors written only for the workspaces the caller holds
* an explicit grant on. A caller whose access to this workspace is inherited an
* organization admin with no `permissions` row authorizes fine and commits the
* value, but has no mirror here; deciding the response from that mirror would
* report a committed write as a failure. The workspace's own mirror still answers
* when it exists because it carries the real per-workspace metadata; otherwise the
* secret's earliest mirror does, and a secret with no mirror anywhere was created
* by this very write.
*/
async function getPersonalSecretMetadata(params: {
workspaceId: string
userId: string
name: string
updatedAt: Date
}): Promise<VisibleWorkspaceCredential> {
const mirror = await findSecretMetadata({
workspaceId: params.workspaceId,
userId: params.userId,
scope: 'personal',
name: params.name,
})
if (mirror) return mirror
const stored = await getPersonalEnvCredentialMetadata({
userId: params.userId,
envKey: params.name,
})
return {
/** No credential row backs this projection; the id is never presented. */
id: stored?.id ?? `env_personal:${params.userId}:${params.name}`,
workspaceId: params.workspaceId,
type: 'env_personal',
displayName: params.name,
description: null,
providerId: null,
accountId: null,
envKey: params.name,
envOwnerUserId: params.userId,
createdBy: params.userId,
createdAt: stored?.createdAt ?? params.updatedAt,
updatedAt: params.updatedAt,
hasServiceAccountKey: false,
role: 'admin',
}
}
const authorizationOptions = {}
export interface ListSecretsInput {
@@ -203,20 +267,27 @@ export const setSecretUseCase = defineAuthorizedWorkspaceUseCase({
})
}
const mutation =
input.scope === 'workspace'
? await setWorkspaceSecret({
workspaceId: context.workspaceId,
name: input.name,
value: input.value,
userId,
})
: await setPersonalSecret({ userId, name: input.name, value: input.value })
const secret = await getSecretMetadata({
if (input.scope === 'workspace') {
const mutation = await setWorkspaceSecret({
workspaceId: context.workspaceId,
name: input.name,
value: input.value,
userId,
})
const secret = await getWorkspaceSecretMetadata({
workspaceId: context.workspaceId,
userId,
name: input.name,
})
return { secret, userId, created: mutation.created }
}
const mutation = await setPersonalSecret({ userId, name: input.name, value: input.value })
const secret = await getPersonalSecretMetadata({
workspaceId: context.workspaceId,
userId,
scope: input.scope,
name: input.name,
updatedAt: mutation.updatedAt,
})
return { secret, userId, created: mutation.created }
},
+1 -1
View File
@@ -39,6 +39,7 @@ import { assertColumnDestructive, assertSchemaMutable } from '@/lib/table/mutati
import type { DbTransaction } from '@/lib/table/planner'
import { stripGroupExecutions } from '@/lib/table/rows/executions'
import { updateTableRowsWithDerivedSecretProvenance } from '@/lib/table/rows/secret-provenance'
import { assertValidSchema } from '@/lib/table/schema-invariants'
import { selectValueToNames } from '@/lib/table/select-values'
import { withLockedTable } from '@/lib/table/service'
import { scaledStatementTimeoutMs, setTableTxTimeouts } from '@/lib/table/tx'
@@ -57,7 +58,6 @@ import type {
UpdateColumnTypeData,
} from '@/lib/table/types'
import { validateColumnDefinition } from '@/lib/table/validation'
import { assertValidSchema } from '@/lib/table/workflow-columns'
import { stripGroupDeps } from '@/lib/table/workflow-group-deps'
const logger = createLogger('TableColumnService')
+210
View File
@@ -0,0 +1,210 @@
/**
* Schema invariants for workflow-group columns, kept in a leaf module.
*
* Pure functions over a `TableSchema`. They lived in `workflow-columns.ts`,
* which transitively reaches the executable tool registry, so importing them
* from `service.ts` to validate a create pulled ~5,200 modules into every page
* graph that renders a table. Splitting them out keeps one source of truth for
* what a valid schema is reachable from the create path and from every later
* mutation without the weight.
*/
import { OrchestrationError } from '@/lib/core/orchestration/types'
import { getColumnId } from '@/lib/table/column-keys'
import type { TableSchema, WorkflowGroup } from '@/lib/table/types'
/**
* Validates schema-level invariants. Run on every `addTableColumn`,
* `addWorkflowGroup`, `updateWorkflowGroup`, `renameColumn`, `reorderColumns`,
* etc. Returns a list of human-readable errors (empty if valid).
*/
export function validateSchema(schema: TableSchema, columnOrder: string[] | undefined): string[] {
const errors: string[] = []
// Group refs and columnOrder hold stable column ids (not display names).
const columnsById = new Map(schema.columns.map((c) => [getColumnId(c), c]))
const groups = schema.workflowGroups ?? []
const groupsById = new Map(groups.map((g) => [g.id, g]))
// Reference integrity for group outputs.
const claimedColumns = new Map<string, string>() // columnId → groupId
for (const group of groups) {
if (group.outputs.length === 0) {
errors.push(`Workflow group "${group.name ?? group.id}" has no outputs.`)
}
for (const out of group.outputs) {
const col = columnsById.get(out.columnName)
if (!col) {
errors.push(
`Workflow group "${group.name ?? group.id}" references missing column "${out.columnName}".`
)
continue
}
if (col.workflowGroupId !== group.id) {
errors.push(
`Column "${col.name}" is referenced by group "${group.id}" but its workflowGroupId is "${col.workflowGroupId ?? '(unset)'}".`
)
}
const claimer = claimedColumns.get(out.columnName)
if (claimer && claimer !== group.id) {
errors.push(
`Column "${out.columnName}" is claimed by both groups "${claimer}" and "${group.id}".`
)
} else {
claimedColumns.set(out.columnName, group.id)
}
}
}
// Every column flagged with a workflowGroupId must appear in exactly one group's outputs.
for (const col of schema.columns) {
if (!col.workflowGroupId) continue
if (!groupsById.has(col.workflowGroupId)) {
errors.push(
`Column "${col.name}" references missing workflow group "${col.workflowGroupId}".`
)
continue
}
if (claimedColumns.get(getColumnId(col)) !== col.workflowGroupId) {
errors.push(
`Column "${col.name}" has workflowGroupId "${col.workflowGroupId}" but isn't in that group's outputs.`
)
}
if (col.required) {
errors.push(`Workflow-output column "${col.name}" cannot be required.`)
}
if (col.unique) {
errors.push(`Workflow-output column "${col.name}" cannot be unique.`)
}
}
// Dependency integrity. Deps are columns only — workflow output columns are
// valid deps too (the upstream group fills them, downstream becomes eligible
// when filled). A group can't depend on its own outputs.
for (const group of groups) {
const ownOutputs = new Set(group.outputs.map((o) => o.columnName))
for (const depCol of group.dependencies?.columns ?? []) {
const col = columnsById.get(depCol)
if (!col) {
errors.push(`Group "${group.name ?? group.id}" depends on missing column "${depCol}".`)
continue
}
if (ownOutputs.has(depCol)) {
errors.push(
`Group "${group.name ?? group.id}" depends on its own output column "${depCol}".`
)
}
}
}
// Cycle detection on the column-induced group graph. An edge A → B exists
// when B depends on a column that A produces.
const cycle = findGroupCycle(groups)
if (cycle) {
errors.push(
`Workflow groups form a dependency cycle: ${cycle.map((id) => groupsById.get(id)?.name ?? id).join(' → ')}.`
)
}
// Layout: every group's outputs must be contiguous in columnOrder (when set).
if (columnOrder && columnOrder.length > 0) {
for (const split of findSplitGroups(columnOrder, groups)) {
errors.push(
`Workflow group "${split.groupName}" output columns must be contiguous; got order [${split.actual.join(', ')}].`
)
}
}
return errors
}
/**
* Returns the cycle as an ordered list of group ids, or null if acyclic. Edges
* are induced by columns: an edge A B exists iff B depends on a column that
* A produces.
*/
function findGroupCycle(groups: WorkflowGroup[]): string[] | null {
// Map each output column → the group that produces it.
const producerByColumn = new Map<string, string>()
for (const g of groups) {
for (const o of g.outputs) producerByColumn.set(o.columnName, g.id)
}
const adjacency = new Map<string, string[]>()
for (const g of groups) {
const upstream = new Set<string>()
for (const depCol of g.dependencies?.columns ?? []) {
const producer = producerByColumn.get(depCol)
if (producer && producer !== g.id) upstream.add(producer)
}
adjacency.set(g.id, [...upstream])
}
const VISITING = 1
const VISITED = 2
const state = new Map<string, number>()
const stack: string[] = []
const dfs = (id: string): string[] | null => {
if (state.get(id) === VISITED) return null
if (state.get(id) === VISITING) {
const cycleStart = stack.indexOf(id)
return cycleStart >= 0 ? [...stack.slice(cycleStart), id] : [id]
}
state.set(id, VISITING)
stack.push(id)
for (const next of adjacency.get(id) ?? []) {
const found = dfs(next)
if (found) return found
}
stack.pop()
state.set(id, VISITED)
return null
}
for (const g of groups) {
const cycle = dfs(g.id)
if (cycle) return cycle
}
return null
}
interface SplitGroupReport {
groupId: string
groupName: string
actual: number[]
}
/**
* Returns groups whose output columns occupy non-contiguous positions in the
* given columnOrder. Empty array means all groups are cohesive.
*/
export function findSplitGroups(
columnOrder: string[],
groups: WorkflowGroup[]
): SplitGroupReport[] {
const positions = new Map<string, number>()
columnOrder.forEach((name, idx) => positions.set(name, idx))
const reports: SplitGroupReport[] = []
for (const group of groups) {
const indices = group.outputs
.map((o) => positions.get(o.columnName))
.filter((i): i is number => i !== undefined)
.sort((a, b) => a - b)
if (indices.length < 2) continue
const min = indices[0]
const max = indices[indices.length - 1]
if (max - min + 1 !== indices.length) {
reports.push({
groupId: group.id,
groupName: group.name ?? group.id,
actual: indices,
})
}
}
return reports
}
export function assertValidSchema(schema: TableSchema, columnOrder: string[] | undefined): void {
const errs = validateSchema(schema, columnOrder)
if (errs.length > 0) {
throw new OrchestrationError('validation', `Schema validation failed: ${errs.join('; ')}`)
}
}
+98
View File
@@ -0,0 +1,98 @@
/**
* @vitest-environment node
*/
import { dbChainMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing'
import { beforeEach, describe, expect, it, vi } from 'vitest'
import type { TableSchema } from '@/lib/table/types'
vi.mock('@/lib/realtime/notify', () => ({
notifyWorkspaceTablesChanged: vi.fn().mockResolvedValue(undefined),
}))
vi.mock('@/lib/table/billing', () => ({
assertRowCapacity: vi.fn().mockResolvedValue(undefined),
notifyTableRowUsage: vi.fn(),
}))
import { createTable } from '@/lib/table/service'
const WORKSPACE_ID = '6fc7631d-88cd-46f8-9f0a-d4764daef7f8'
/** A column produced by a workflow group, and the group that declares it. */
function groupedSchema(overrides: { columnGroupId: string; groupId: string }): TableSchema {
return {
columns: [
{ id: 'col_email', name: 'email', type: 'string' },
{
id: 'col_summary',
name: 'summary',
type: 'string',
workflowGroupId: overrides.columnGroupId,
},
],
workflowGroups: [
{
id: overrides.groupId,
workflowId: 'workflow-1',
outputs: [{ blockId: 'block-1', path: 'out', columnName: 'col_summary' }],
},
],
} as TableSchema
}
function create(schema: TableSchema) {
return createTable(
{ name: 'contacts', schema, workspaceId: WORKSPACE_ID, userId: 'user-1' },
'request-1'
)
}
describe('createTable schema invariants', () => {
beforeEach(() => {
vi.clearAllMocks()
resetDbChainMock()
})
/**
* `POST /api/table` and `POST /api/v1/tables` both forward caller-supplied
* columns into this function, and their bodies carry no `workflowGroups`, so
* any group id they carry names a group that cannot exist. Stored, it fails
* every later add-column and add-group with a 400 that nothing can clear.
*/
it('rejects a column naming a workflow group the schema does not declare', async () => {
await expect(
create({
columns: [
{ id: 'col_email', name: 'email', type: 'string', workflowGroupId: 'wfg_missing' },
],
} as TableSchema)
).rejects.toMatchObject({
code: 'validation',
message: expect.stringContaining('references missing workflow group "wfg_missing"'),
})
expect(dbChainMockFns.insert).not.toHaveBeenCalled()
})
it('still creates a table whose columns name a group the same schema declares', async () => {
queueTableRows(schemaMock.userTableDefinitions, [{ count: 0 }])
const table = await create(groupedSchema({ columnGroupId: 'group-1', groupId: 'group-1' }))
expect(table.schema.columns.map((column) => column.workflowGroupId)).toEqual([
undefined,
'group-1',
])
expect(dbChainMockFns.insert).toHaveBeenCalled()
})
it('creates an ordinary group-free table unchanged', async () => {
queueTableRows(schemaMock.userTableDefinitions, [{ count: 0 }])
const table = await create({ columns: [{ name: 'email', type: 'string' }] } as TableSchema)
expect(table.name).toBe('contacts')
expect(table.schema.columns[0].id).toEqual(expect.any(String))
expect(dbChainMockFns.insert).toHaveBeenCalled()
})
})
+11
View File
@@ -46,6 +46,7 @@ import {
createExactEmptyTableRowSecretProvenance,
mutateTableRowsWithSecretProvenance,
} from '@/lib/table/rows/secret-provenance'
import { assertValidSchema } from '@/lib/table/schema-invariants'
import { setTableTxTimeouts } from '@/lib/table/tx'
import {
type CreateTableData,
@@ -548,6 +549,16 @@ export async function createTable(
// Stamp stable ids so the table is id-keyed from its first row write.
const schema = withGeneratedColumnIds(data.schema)
// The same invariants every later schema mutation enforces, run over what is
// about to be persisted. `validateTableSchema` above only checks columns in
// isolation, so a create could store a column naming a workflow group the
// schema does not declare — which no update path can clear, and which then
// fails every subsequent add-column and add-group with a 400. Imported lazily
// because `workflow-columns` transitively reaches the executable tool
// registry, which a static edge would pull into every page graph that renders
// a table.
assertValidSchema(schema, undefined)
// Row limits are enforced per-write against the current plan (see assertRowCapacity); the stored
// column is vestigial, so it just takes the caller's value (if any) or the default.
const maxRows = data.maxRows ?? TABLE_LIMITS.MAX_ROWS_PER_TABLE
-197
View File
@@ -38,7 +38,6 @@ import type {
RowExecutions,
TableDefinition,
TableRow,
TableSchema,
WorkflowGroup,
} from '@/lib/table/types'
@@ -51,7 +50,6 @@ const TABLE_TRIGGER_CANCELLATION_MAX_RUNS = 5_000
const TABLE_TRIGGER_CANCELLATION_RETENTION_MS = 14 * 24 * 60 * 60_000
const TABLE_ROW_EXECUTIONS_ROW_FK = 'table_row_executions_row_id_user_table_rows_id_fk'
import { getColumnId } from '@/lib/table/column-keys'
import { USER_TABLE_ROWS_SQL_NAME } from '@/lib/table/constants'
import { areGroupDepsSatisfied, areOutputsFilled, isExecInFlight } from '@/lib/table/deps'
import { resolveTableDispatchConcurrency } from '@/lib/table/dispatch-concurrency'
@@ -1072,165 +1070,6 @@ export async function runWorkflowColumn(opts: {
return { dispatchId, shouldSignalRowsChanged: true }
}
/**
* Validates schema-level invariants. Run on every `addTableColumn`,
* `addWorkflowGroup`, `updateWorkflowGroup`, `renameColumn`, `reorderColumns`,
* etc. Returns a list of human-readable errors (empty if valid).
*/
export function validateSchema(schema: TableSchema, columnOrder: string[] | undefined): string[] {
const errors: string[] = []
// Group refs and columnOrder hold stable column ids (not display names).
const columnsById = new Map(schema.columns.map((c) => [getColumnId(c), c]))
const groups = schema.workflowGroups ?? []
const groupsById = new Map(groups.map((g) => [g.id, g]))
// Reference integrity for group outputs.
const claimedColumns = new Map<string, string>() // columnId → groupId
for (const group of groups) {
if (group.outputs.length === 0) {
errors.push(`Workflow group "${group.name ?? group.id}" has no outputs.`)
}
for (const out of group.outputs) {
const col = columnsById.get(out.columnName)
if (!col) {
errors.push(
`Workflow group "${group.name ?? group.id}" references missing column "${out.columnName}".`
)
continue
}
if (col.workflowGroupId !== group.id) {
errors.push(
`Column "${col.name}" is referenced by group "${group.id}" but its workflowGroupId is "${col.workflowGroupId ?? '(unset)'}".`
)
}
const claimer = claimedColumns.get(out.columnName)
if (claimer && claimer !== group.id) {
errors.push(
`Column "${out.columnName}" is claimed by both groups "${claimer}" and "${group.id}".`
)
} else {
claimedColumns.set(out.columnName, group.id)
}
}
}
// Every column flagged with a workflowGroupId must appear in exactly one group's outputs.
for (const col of schema.columns) {
if (!col.workflowGroupId) continue
if (!groupsById.has(col.workflowGroupId)) {
errors.push(
`Column "${col.name}" references missing workflow group "${col.workflowGroupId}".`
)
continue
}
if (claimedColumns.get(getColumnId(col)) !== col.workflowGroupId) {
errors.push(
`Column "${col.name}" has workflowGroupId "${col.workflowGroupId}" but isn't in that group's outputs.`
)
}
if (col.required) {
errors.push(`Workflow-output column "${col.name}" cannot be required.`)
}
if (col.unique) {
errors.push(`Workflow-output column "${col.name}" cannot be unique.`)
}
}
// Dependency integrity. Deps are columns only — workflow output columns are
// valid deps too (the upstream group fills them, downstream becomes eligible
// when filled). A group can't depend on its own outputs.
for (const group of groups) {
const ownOutputs = new Set(group.outputs.map((o) => o.columnName))
for (const depCol of group.dependencies?.columns ?? []) {
const col = columnsById.get(depCol)
if (!col) {
errors.push(`Group "${group.name ?? group.id}" depends on missing column "${depCol}".`)
continue
}
if (ownOutputs.has(depCol)) {
errors.push(
`Group "${group.name ?? group.id}" depends on its own output column "${depCol}".`
)
}
}
}
// Cycle detection on the column-induced group graph. An edge A → B exists
// when B depends on a column that A produces.
const cycle = findGroupCycle(groups)
if (cycle) {
errors.push(
`Workflow groups form a dependency cycle: ${cycle.map((id) => groupsById.get(id)?.name ?? id).join(' → ')}.`
)
}
// Layout: every group's outputs must be contiguous in columnOrder (when set).
if (columnOrder && columnOrder.length > 0) {
for (const split of findSplitGroups(columnOrder, groups)) {
errors.push(
`Workflow group "${split.groupName}" output columns must be contiguous; got order [${split.actual.join(', ')}].`
)
}
}
return errors
}
/**
* Returns the cycle as an ordered list of group ids, or null if acyclic. Edges
* are induced by columns: an edge A B exists iff B depends on a column that
* A produces.
*/
function findGroupCycle(groups: WorkflowGroup[]): string[] | null {
// Map each output column → the group that produces it.
const producerByColumn = new Map<string, string>()
for (const g of groups) {
for (const o of g.outputs) producerByColumn.set(o.columnName, g.id)
}
const adjacency = new Map<string, string[]>()
for (const g of groups) {
const upstream = new Set<string>()
for (const depCol of g.dependencies?.columns ?? []) {
const producer = producerByColumn.get(depCol)
if (producer && producer !== g.id) upstream.add(producer)
}
adjacency.set(g.id, [...upstream])
}
const VISITING = 1
const VISITED = 2
const state = new Map<string, number>()
const stack: string[] = []
const dfs = (id: string): string[] | null => {
if (state.get(id) === VISITED) return null
if (state.get(id) === VISITING) {
const cycleStart = stack.indexOf(id)
return cycleStart >= 0 ? [...stack.slice(cycleStart), id] : [id]
}
state.set(id, VISITING)
stack.push(id)
for (const next of adjacency.get(id) ?? []) {
const found = dfs(next)
if (found) return found
}
stack.pop()
state.set(id, VISITED)
return null
}
for (const g of groups) {
const cycle = dfs(g.id)
if (cycle) return cycle
}
return null
}
interface SplitGroupReport {
groupId: string
groupName: string
actual: number[]
}
/**
* Cell context stored on `paused_executions.metadata` so the resume worker
* can route post-resume block outputs back to the same `(tableId, rowId,
@@ -1300,40 +1139,4 @@ export async function findCellContextByExecutionId(
}
}
/**
* Returns groups whose output columns occupy non-contiguous positions in the
* given columnOrder. Empty array means all groups are cohesive.
*/
export function findSplitGroups(
columnOrder: string[],
groups: WorkflowGroup[]
): SplitGroupReport[] {
const positions = new Map<string, number>()
columnOrder.forEach((name, idx) => positions.set(name, idx))
const reports: SplitGroupReport[] = []
for (const group of groups) {
const indices = group.outputs
.map((o) => positions.get(o.columnName))
.filter((i): i is number => i !== undefined)
.sort((a, b) => a - b)
if (indices.length < 2) continue
const min = indices[0]
const max = indices[indices.length - 1]
if (max - min + 1 !== indices.length) {
reports.push({
groupId: group.id,
groupName: group.name ?? group.id,
actual: indices,
})
}
}
return reports
}
/** Throws if the schema has any invariant violations. Convenience for callers. */
export function assertValidSchema(schema: TableSchema, columnOrder: string[] | undefined): void {
const errs = validateSchema(schema, columnOrder)
if (errs.length > 0) {
throw new OrchestrationError('validation', `Schema validation failed: ${errs.join('; ')}`)
}
}
@@ -21,10 +21,17 @@ vi.mock('@/lib/table/rows/secret-provenance', () => ({
updateTableRowsWithDerivedSecretProvenance: vi.fn(),
}))
vi.mock('@/lib/table/workflow-columns', () => ({
assertValidSchema: vi.fn(),
runWorkflowColumn: vi.fn().mockResolvedValue(undefined),
stripGroupDeps: (schema: unknown) => schema,
}))
/**
* These ceiling fixtures declare groups whose output columns are not in the
* schema, so the invariant check has to stay stubbed for them to exercise the
* count limit. It moved to its own leaf module, so the stub follows it.
*/
vi.mock('@/lib/table/schema-invariants', () => ({
assertValidSchema: vi.fn(),
}))
import { TABLE_LIMITS } from '@/lib/table/constants'
import { addWorkflowGroup } from '@/lib/table/workflow-groups/service'
@@ -24,6 +24,7 @@ import { NAME_PATTERN, TABLE_LIMITS } from '@/lib/table/constants'
import { assertColumnDestructive, assertSchemaMutable } from '@/lib/table/mutation-locks'
import { stripGroupExecutions } from '@/lib/table/rows/executions'
import { updateTableRowsWithDerivedSecretProvenance } from '@/lib/table/rows/secret-provenance'
import { assertValidSchema } from '@/lib/table/schema-invariants'
import { getTableById, withLockedTable } from '@/lib/table/service'
import { setTableTxTimeouts } from '@/lib/table/tx'
import type {
@@ -37,7 +38,7 @@ import type {
WorkflowGroup,
WorkflowGroupOutput,
} from '@/lib/table/types'
import { assertValidSchema, runWorkflowColumn } from '@/lib/table/workflow-columns'
import { runWorkflowColumn } from '@/lib/table/workflow-columns'
import { stripGroupDeps } from '@/lib/table/workflow-group-deps'
const logger = createLogger('TableWorkflowGroupsService')
@@ -66,6 +66,7 @@ vi.mock('@/lib/uploads/upload-session/provider', () => ({
uploadStorageProvider: vi.fn(() => 's3'),
}))
import { OrchestrationError } from '@/lib/core/orchestration/types'
import { LOCAL_UPLOAD_METADATA_SUFFIX } from '@/lib/uploads/core/storage-key'
import {
abortUploadSession,
@@ -75,6 +76,7 @@ import {
createUploadPartUrls,
createUploadSession,
createUploadSessionAuthBinding,
expectedUploadPartSize,
getOwnedUploadSession,
getPrincipalKnowledgeDocumentUploadSession,
UPLOAD_SESSION_PART_SIZE,
@@ -654,6 +656,32 @@ describe('upload sessions', () => {
).rejects.toMatchObject({ code: 'validation' })
})
/**
* The part-number path segment of a signed part URL is caller-editable, so
* the size lookup is a request boundary. It has to classify an out-of-range
* part as a validation failure for the data-plane route to answer 400 rather
* than falling through to its generic 500.
*/
it('classifies an out-of-range part number as a validation failure', () => {
const multipart = sessionRecord({
method: 'multipart',
partSize: UPLOAD_SESSION_PART_SIZE,
partCount: 2,
fileSize: UPLOAD_SESSION_PART_SIZE + 1,
})
expect(expectedUploadPartSize(multipart, 1)).toBe(UPLOAD_SESSION_PART_SIZE)
expect(expectedUploadPartSize(multipart, 2)).toBe(1)
expect(() => expectedUploadPartSize(multipart, 3)).toThrow(OrchestrationError)
expect(() => expectedUploadPartSize(multipart, 3)).toThrow('partNumber must be between 1 and 2')
try {
expectedUploadPartSize(multipart, 3)
expect.unreachable('out-of-range part number must throw')
} catch (error) {
expect(error).toMatchObject({ code: 'validation' })
}
})
it('loads ownership from PostgreSQL and rejects a mismatched token', async () => {
const token = 'upload-secret'
const row = uploadRow({ tokenHash: sha256Hex(token) })
@@ -928,7 +928,10 @@ export function expectedUploadPartSize(session: UploadSessionRecord, partNumber:
throw new UploadSessionError('conflict', 'PUT upload sessions do not have multipart parts')
}
if (!Number.isInteger(partNumber) || partNumber < 1 || partNumber > session.partCount) {
throw new UploadSessionError('validation', 'Invalid upload part number')
throw new UploadSessionError(
'validation',
`partNumber must be between 1 and ${session.partCount}`
)
}
if (partNumber < session.partCount) return session.partSize
return session.fileSize - session.partSize * (session.partCount - 1)
@@ -0,0 +1,62 @@
/**
* @vitest-environment node
*/
import { db } from '@sim/db'
import { beforeEach, describe, expect, it, vi } from 'vitest'
import { upsertCustomTools } from '@/lib/workflows/custom-tools/operations'
const WORKSPACE_ID = 'workspace-1'
const USER_ID = 'user-1'
const storableSchema = {
type: 'function',
function: {
name: 'lookup_order',
parameters: { type: 'object', properties: { id: { type: 'string' } } },
},
}
describe('upsertCustomTools schema invariant', () => {
beforeEach(() => {
vi.clearAllMocks()
})
it('refuses a declaration missing the function discriminator before opening a transaction', async () => {
await expect(
upsertCustomTools({
tools: [
{ title: 'No discriminator', schema: { function: storableSchema.function }, code: '' },
],
workspaceId: WORKSPACE_ID,
userId: USER_ID,
})
).rejects.toMatchObject({ code: 'validation' })
expect(db.transaction).not.toHaveBeenCalled()
})
it('refuses the whole batch when any declaration is unstorable', async () => {
await expect(
upsertCustomTools({
tools: [
{ title: 'Good', schema: storableSchema, code: '' },
{ title: 'Bad', schema: { ...storableSchema, type: 'object' }, code: '' },
],
workspaceId: WORKSPACE_ID,
userId: USER_ID,
})
).rejects.toMatchObject({ code: 'validation' })
expect(db.transaction).not.toHaveBeenCalled()
})
it('opens the transaction for a storable declaration', async () => {
await upsertCustomTools({
tools: [{ title: 'Good', schema: storableSchema, code: '' }],
workspaceId: WORKSPACE_ID,
userId: USER_ID,
})
expect(db.transaction).toHaveBeenCalled()
})
})
@@ -16,6 +16,7 @@ import {
timestampKey,
} from '@/lib/api/list-query'
import { generateRequestId } from '@/lib/core/utils/request'
import { assertStorableCustomToolSchema } from '@/lib/custom-tools/schema'
const logger = createLogger('CustomToolsOperations')
@@ -38,6 +39,14 @@ export async function upsertCustomTools(params: {
}) {
const { tools, workspaceId, userId, requestId = generateRequestId() } = params
/**
* Ahead of the transaction so a batch is rejected whole rather than landing
* the tools that preceded the unstorable one.
*/
for (const tool of tools) {
assertStorableCustomToolSchema(tool.schema)
}
return await db.transaction(async (tx) => {
for (const tool of tools) {
const nowTime = new Date()
@@ -0,0 +1,93 @@
/**
* @vitest-environment node
*/
import { beforeEach, describe, expect, it, vi } from 'vitest'
const { mockUpsertCustomTools } = vi.hoisted(() => ({
mockUpsertCustomTools: vi.fn(),
}))
vi.mock('@/lib/workflows/custom-tools/operations', () => ({
upsertCustomTools: mockUpsertCustomTools,
}))
import { persistCustomToolsToDatabase } from '@/lib/workflows/persistence/custom-tools-persistence'
const WORKSPACE_ID = 'workspace-1'
const USER_ID = 'user-1'
const storableSchema = {
type: 'function' as const,
function: {
name: 'lookup_order',
description: 'Looks up an order',
parameters: { type: 'object', properties: { id: { type: 'string' } }, required: ['id'] },
},
}
function customTool(overrides: Record<string, unknown> = {}) {
return {
type: 'custom-tool' as const,
title: 'Lookup order',
schema: storableSchema,
code: 'return 1',
...overrides,
} as Parameters<typeof persistCustomToolsToDatabase>[0][number]
}
describe('persistCustomToolsToDatabase', () => {
beforeEach(() => {
vi.clearAllMocks()
mockUpsertCustomTools.mockResolvedValue([])
})
it('persists a storable declaration unchanged', async () => {
const result = await persistCustomToolsToDatabase([customTool()], WORKSPACE_ID, USER_ID)
expect(result).toEqual({ saved: 1, errors: [] })
expect(mockUpsertCustomTools).toHaveBeenCalledWith({
tools: [{ id: undefined, title: 'Lookup order', schema: storableSchema, code: 'return 1' }],
workspaceId: WORKSPACE_ID,
userId: USER_ID,
})
})
it('skips a declaration missing the function discriminator the public API republishes', async () => {
const withoutType = customTool({
title: 'No discriminator',
schema: { function: storableSchema.function },
})
const result = await persistCustomToolsToDatabase([withoutType], WORKSPACE_ID, USER_ID)
expect(result).toEqual({ saved: 0, errors: [] })
expect(mockUpsertCustomTools).not.toHaveBeenCalled()
})
it('skips a declaration whose discriminator is not `function`', async () => {
const wrongType = customTool({
title: 'Wrong discriminator',
schema: { ...storableSchema, type: 'object' },
})
const result = await persistCustomToolsToDatabase([wrongType], WORKSPACE_ID, USER_ID)
expect(result).toEqual({ saved: 0, errors: [] })
expect(mockUpsertCustomTools).not.toHaveBeenCalled()
})
it('keeps the rest of an import when one declaration is unstorable', async () => {
const result = await persistCustomToolsToDatabase(
[customTool({ title: 'Bad', schema: { function: storableSchema.function } }), customTool()],
WORKSPACE_ID,
USER_ID
)
expect(result).toEqual({ saved: 1, errors: [] })
expect(mockUpsertCustomTools).toHaveBeenCalledWith(
expect.objectContaining({
tools: [{ id: undefined, title: 'Lookup order', schema: storableSchema, code: 'return 1' }],
})
)
})
})
@@ -1,5 +1,6 @@
import { createLogger } from '@sim/logger'
import { toError } from '@sim/utils/errors'
import { isStorableCustomToolSchema } from '@/lib/custom-tools/schema'
import { upsertCustomTools } from '@/lib/workflows/custom-tools/operations'
const logger = createLogger('CustomToolsPersistence')
@@ -138,6 +139,17 @@ export async function persistCustomToolsToDatabase(
logger.warn(`Skipping custom tool without function name: ${tool.title}`)
return false
}
/**
* An imported graph can carry any inline declaration, and a stored tool
* whose schema the public API cannot serialize back breaks every read of
* the workspace's tool list. Skipped per tool, like the check above, so one
* unstorable declaration does not cost the rest of the import its tools
* the tool itself still runs from the inline definition on the block.
*/
if (!isStorableCustomToolSchema(tool.schema)) {
logger.warn(`Skipping custom tool with an unstorable schema: ${tool.title}`)
return false
}
return true
})