Skip to content

Commit 7133845

Browse files
committed
fix(v2): say what an error means in terms the caller can act on
A size-limit refusal collapsed every value under a kilobyte to "0 Bytes", so a 28-byte file over a 27-byte ceiling read "is 0 Bytes, above the 0 Bytes limit" - self-contradictory, and useless for choosing a value that would work. Errors and field descriptions also told callers to invoke raw HTTP endpoints. These strings serve the REST reference and the CLI's own help equally, so they now name the operation and its object rather than a method and a path. A sweep test walks every v2 schema description and holds the line, with the remaining offenders in files this change does not own recorded explicitly rather than left to be rediscovered. Listing the editors of a built-in skill claimed the skill did not exist, while reading the same id succeeded - a well-formed request for a real resource is not malformed, so the list answers an empty roster and only the mutations refuse. Bulk folder deletion recorded only the leaf name in its audit trail while the single delete recorded the full path, leaving two same-named folders under different parents indistinguishable after the fact. Bulk chunk enable, disable and delete each treated an unmatched id differently behind one sentence of documentation. They now follow one rule. A workspace-scoped list refused with the name of a resource the caller never addressed, which reads as an empty workspace rather than an unreachable one.
1 parent 5396edb commit 7133845

21 files changed

Lines changed: 584 additions & 67 deletions

apps/sim/app/api/v2/chat-deployments/route.test.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -295,12 +295,21 @@ describe('/api/v2/chat-deployments', () => {
295295
expect(response.status).toBe(200)
296296
})
297297

298-
it('conceals a workspace the caller cannot reach as 404', async () => {
298+
/**
299+
* The list addresses a workspace, so the concealed denial must name the
300+
* workspace. Naming a chat deployment reported a resource the caller never
301+
* asked for. Concealment itself is unchanged: still 404, still no signal
302+
* about whether the workspace holds any deployment.
303+
*/
304+
it('conceals a workspace the caller cannot reach as a missing workspace', async () => {
299305
mocks.resolvePermission.mockResolvedValue(null)
300306

301307
const response = await get()
302308

303309
expect(response.status).toBe(404)
310+
const body = await response.json()
311+
expect(body.error.code).toBe('NOT_FOUND')
312+
expect(body.error.message).toBe('Workspace not found')
304313
expect(mocks.listDeployments).not.toHaveBeenCalled()
305314
})
306315

apps/sim/app/api/v2/chat-deployments/route.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { cursorRoute, cursorScopeKey } from '@/lib/api/cursor-binding'
33
import { defineV2JsonRoute, v2ApiKeyAuth, v2RateLimits } from '@/lib/api/server/routes'
44
import { chatDeploymentOperations, listChatDeployments } from '@/lib/chat-deployments/application'
55
import {
6-
chatDeploymentErrorPolicy,
6+
chatDeploymentWorkspaceErrorPolicy,
77
toV2ChatDeploymentListItem,
88
} from '@/app/api/v2/chat-deployments/utils'
99
import { readSortedCursor, writeSortedCursor } from '@/app/api/v2/lib/response'
@@ -48,7 +48,7 @@ export const GET = defineV2JsonRoute({
4848
auth: v2ApiKeyAuth,
4949
operation: chatDeploymentOperations.list,
5050
rateLimit: v2RateLimits.publicApi,
51-
errorPolicy: chatDeploymentErrorPolicy,
51+
errorPolicy: chatDeploymentWorkspaceErrorPolicy,
5252
mapInput: ({ query }) => ({
5353
workspaceId: query.workspaceId,
5454
workflowId: query.workflowId,

apps/sim/app/api/v2/chat-deployments/utils.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,3 +135,13 @@ export function toV2ChatDeploymentListItem(
135135
export const chatDeploymentErrorPolicy = createV2ResourceConcealmentPolicy({
136136
notFoundMessage: 'Chat deployment not found',
137137
})
138+
139+
/**
140+
* The list is addressed by workspace, not by deployment, so a concealed
141+
* cross-tenant denial must name the workspace the caller asked for. Concealment
142+
* itself is unchanged — an unreachable workspace still answers 404 whether or
143+
* not it holds any deployment.
144+
*/
145+
export const chatDeploymentWorkspaceErrorPolicy = createV2ResourceConcealmentPolicy({
146+
notFoundMessage: 'Workspace not found',
147+
})
Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,183 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { describe, expect, it } from 'vitest'
5+
import { listContractFiles } from '@/lib/api/contracts/v2/__tests__/contract-sweep'
6+
7+
/**
8+
* Every v2 schema description is user-facing prose on two surfaces at once: the
9+
* published API reference and `sim <command> --help`, which is generated from
10+
* these exact strings. A description spelling an HTTP method and path tells a
11+
* CLI caller to do something the CLI cannot do, so descriptions name the
12+
* operation and its object rather than the transport.
13+
*
14+
* This is a sweep rather than a handful of per-file assertions because the
15+
* strings that regressed last time sat a few lines from ones already fixed by
16+
* hand. Anything deliberately left alone goes in ALLOWED below with its reason,
17+
* and the sweep fails when an allowlisted description no longer appears, so the
18+
* list cannot rot.
19+
*
20+
* Allowlisting is keyed by the description text, not by schema path: these
21+
* schemas are shared between contracts, so one sentence surfaces under many
22+
* paths and fixing it must clear every one of them at once.
23+
*/
24+
25+
const ENDPOINT_SPELLING = /\b(GET|POST|PATCH|PUT|DELETE)\s+\//
26+
27+
/** Depth cap so a self-referential `lazy` schema cannot spin the walk. */
28+
const MAX_DEPTH = 12
29+
30+
/**
31+
* Descriptions still naming a transport. Every entry is a contract file owned by
32+
* another change in flight — none is a judgment that the spelling is correct.
33+
*/
34+
const ALLOWED = new Map<string, string>([
35+
[
36+
'Tag definition identifier. Published because `PATCH` and `DELETE /knowledge/{knowledgeBaseId}/tags/{tagId}` address a definition by it; without it those operations are unreachable from a list read.',
37+
'knowledge tag contracts owned elsewhere',
38+
],
39+
[
40+
'Tag definition identifier. Published for the same reason the vocabulary read publishes it: `PATCH` and `DELETE /knowledge/{knowledgeBaseId}/tags/{tagId}` address a definition by id, so without it a usage row cannot be acted on without a second read and a slot join.',
41+
'knowledge tag contracts owned elsewhere',
42+
],
43+
[
44+
'Document tag values keyed by tag display name. Writes address the same tags by slot (`tag1`..`tag7`); resolve names to slots with GET /api/v2/knowledge/{knowledgeBaseId}/tags.',
45+
'knowledge.ts owned elsewhere',
46+
],
47+
[
48+
'ISO 8601 timestamp when the knowledge base was archived by `DELETE /knowledge/{knowledgeBaseId}`, or null while the knowledge base is active. Only `GET /knowledge?scope=archived` returns knowledge bases with a non-null value.',
49+
'knowledge.ts owned elsewhere',
50+
],
51+
[
52+
'Which lifecycle set to list: `active` (default) for live knowledge bases, `archived` for knowledge bases a `DELETE` archived and `POST /knowledge/{knowledgeBaseId}/restore` can bring back. `folderPath` resolves against active folders only, so pairing it with `scope=archived` returns an empty page when the containing folder was archived too.',
53+
'knowledge.ts owned elsewhere',
54+
],
55+
[
56+
'Structured tag filters, at most 10 of them. Every filter must hold, including two that name the same tag: repeating one tag narrows the result rather than widening it, matching `GET /api/v2/knowledge/{knowledgeBaseId}/documents`. To match either of two values for one tag, issue a search per value. Each filtered tag must resolve to the same slot and field type in every knowledge base selected; one missing from any of them, or defined inconsistently across them, is rejected rather than ignored, and those knowledge bases must be searched separately. List the available names with `GET /api/v2/knowledge/{knowledgeBaseId}/tags`.',
57+
'knowledge.ts owned elsewhere',
58+
],
59+
[
60+
'Runs that finished successfully. Failed, cancelled, and paused runs are not counted, and the counter is never reduced when a run ages out of log retention — so it does not match the size of `GET /api/v2/workflows/{workflowId}/runs`, in either direction.',
61+
'workflows.ts owned elsewhere',
62+
],
63+
[
64+
'The workflow was archived, not erased. Its schedules, webhooks, MCP tools, and chats were archived with it, and `POST /workflows/{workflowId}/restore` brings all of them back.',
65+
'workflows.ts owned elsewhere',
66+
],
67+
[
68+
'Whether the deployed workflow accepts unauthenticated public API execution. While true, anyone holding the execution URL can run the workflow — and be billed for it — without an API key, so this is the field an audit of what a deployment exposes reads. Changed with `PATCH /workflows/{workflowId}/deployment`.',
69+
'workflows.ts owned elsewhere',
70+
],
71+
[
72+
'Operation id from `GET /api/v2/blocks/{blockId}`. Required when the block exposes multiple operations; it may differ from the underlying tool id.',
73+
'workflows.ts owned elsewhere',
74+
],
75+
['Custom tool id returned by `GET /api/v2/custom-tools`.', 'workflows.ts owned elsewhere'],
76+
[
77+
'Deployment attempt accepted for processing. Activation is asynchronous, and `latestDeploymentAttempt` is the attempt handle — returned by every deployment mutation as well as this read. Poll activation with `isDeployed` and `deployedAt` on the workflow, or `isActive` on `GET /workflows/{workflowId}/versions`.',
78+
'workflows.ts owned elsewhere',
79+
],
80+
])
81+
82+
interface Described {
83+
/** `file.ts#exportName.field`, so a failure names the symbol to edit. */
84+
key: string
85+
description: string
86+
}
87+
88+
function describedOf(node: unknown): string | undefined {
89+
const described = node as { description?: unknown; meta?: () => { description?: unknown } }
90+
if (typeof described?.description === 'string') return described.description
91+
const meta = typeof described?.meta === 'function' ? described.meta() : undefined
92+
return typeof meta?.description === 'string' ? meta.description : undefined
93+
}
94+
95+
function collect(node: unknown, key: string, seen: Set<unknown>, out: Described[], depth: number) {
96+
if (!node || typeof node !== 'object' || depth <= 0 || seen.has(node)) return
97+
seen.add(node)
98+
const def = (node as { def?: Record<string, unknown> }).def
99+
if (!def) return
100+
101+
const description = describedOf(node)
102+
if (description) out.push({ key, description })
103+
104+
for (const wrapper of ['innerType', 'in', 'out', 'schema', 'element', 'valueType', 'keyType']) {
105+
if (def[wrapper]) collect(def[wrapper], key, seen, out, depth - 1)
106+
}
107+
for (const option of (def.options as unknown[] | undefined) ?? []) {
108+
collect(option, key, seen, out, depth - 1)
109+
}
110+
for (const [field, child] of Object.entries(
111+
(def.shape as Record<string, unknown> | undefined) ?? {}
112+
)) {
113+
collect(child, `${key}.${field}`, seen, out, depth - 1)
114+
}
115+
}
116+
117+
/** Every description reachable from an exported schema or route contract. */
118+
async function sweepDescriptions(): Promise<Described[]> {
119+
const out: Described[] = []
120+
for (const file of listContractFiles().filter((path) => path.includes('/contracts/v2/'))) {
121+
const name = file.split('/contracts/v2/')[1]
122+
const module = (await import(file)) as Record<string, unknown>
123+
for (const [exported, value] of Object.entries(module)) {
124+
if (!value || typeof value !== 'object') continue
125+
/**
126+
* A fresh visited set per export: schemas are shared between contracts, and
127+
* deduplicating across them would report a shared field under whichever
128+
* export reached it first and hide the rest.
129+
*/
130+
const seen = new Set<unknown>()
131+
const key = `${name}#${exported}`
132+
if ('def' in value) {
133+
collect(value, key, seen, out, MAX_DEPTH)
134+
continue
135+
}
136+
const contract = value as {
137+
params?: unknown
138+
query?: unknown
139+
body?: unknown
140+
headers?: unknown
141+
response?: { schema?: unknown }
142+
}
143+
for (const slot of ['params', 'query', 'body', 'headers'] as const) {
144+
if (contract[slot]) collect(contract[slot], `${key}.${slot}`, seen, out, MAX_DEPTH)
145+
}
146+
if (contract.response?.schema) {
147+
collect(contract.response.schema, `${key}.response`, seen, out, MAX_DEPTH)
148+
}
149+
}
150+
}
151+
return out
152+
}
153+
154+
function offendingDescriptions(described: Described[]): Map<string, string[]> {
155+
const byDescription = new Map<string, string[]>()
156+
for (const { key, description } of described) {
157+
if (!ENDPOINT_SPELLING.test(description)) continue
158+
const keys = byDescription.get(description)
159+
if (keys) keys.push(key)
160+
else byDescription.set(description, [key])
161+
}
162+
return byDescription
163+
}
164+
165+
describe('v2 schema descriptions', () => {
166+
it('name the operation rather than an HTTP method and path', async () => {
167+
const described = await sweepDescriptions()
168+
expect(described.length).toBeGreaterThan(1000)
169+
170+
const unexpected = [...offendingDescriptions(described)]
171+
.filter(([description]) => !ALLOWED.has(description))
172+
.map(([description, keys]) => `${keys[0]} :: ${description}`)
173+
174+
expect(unexpected).toEqual([])
175+
})
176+
177+
it('keeps the allowlist honest', async () => {
178+
const offending = offendingDescriptions(await sweepDescriptions())
179+
const stale = [...ALLOWED.keys()].filter((description) => !offending.has(description))
180+
181+
expect(stale).toEqual([])
182+
})
183+
})

apps/sim/lib/api/contracts/v2/catalog.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -227,13 +227,11 @@ export const v2BlockSummarySchema = z
227227
toolIds: z
228228
.array(z.string())
229229
.describe(
230-
'Built-in tools this block can run. Resolve one with `GET /api/v2/tools/{toolId}`.'
230+
'Built-in tools this block can run. Read a tool by its id for the full definition.'
231231
),
232232
operationIds: z
233233
.array(z.string())
234-
.describe(
235-
'Operations this block exposes. Their fields and tools are on `GET /api/v2/blocks/{blockId}`.'
236-
),
234+
.describe('Operations this block exposes. Their fields and tools are on the block read.'),
237235
preview: z
238236
.boolean()
239237
.describe('Whether the block is unreleased and revealed only to this caller.'),

apps/sim/lib/api/contracts/v2/files.ts

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -72,13 +72,13 @@ export const v2FileSchema = z
7272
.number()
7373
.nonnegative()
7474
.describe(
75-
'Size in bytes of the stored file. For a generated document (docx, pptx, pdf, xlsx) this is the generation source, not the rendered document, so it does not predict how many bytes `GET /files/{fileId}` returns.'
75+
'Size in bytes of the stored file. For a generated document (docx, pptx, pdf, xlsx) this is the generation source, not the rendered document, so it does not predict how many bytes downloading the file returns.'
7676
)
7777
.meta({ examples: [1024] }),
7878
type: z
7979
.string()
8080
.describe(
81-
'MIME type of the stored file. For a generated document (docx, pptx, pdf, xlsx) this is the generation source type, not the rendered document type `GET /files/{fileId}` serves.'
81+
'MIME type of the stored file. For a generated document (docx, pptx, pdf, xlsx) this is the generation source type, not the rendered document type a download serves.'
8282
)
8383
.meta({ examples: ['text/csv'] }),
8484
key: z
@@ -108,7 +108,7 @@ export const v2FileSchema = z
108108
.string()
109109
.nullable()
110110
.describe(
111-
'ISO 8601 timestamp when the file was archived by `DELETE /files/{fileId}`, or null while the file is active. Only `GET /files?scope=archived` returns files with a non-null value.'
111+
'ISO 8601 timestamp when the file was archived by deleting it, or null while the file is active. Only an archived-scope file list returns files with a non-null value.'
112112
)
113113
.meta({ format: 'date-time', examples: ['2026-01-16T09:00:00Z'] }),
114114
})
@@ -396,7 +396,7 @@ export const v2GetFileMetadataQuerySchema = z
396396
scope: v2FileScopeSchema
397397
.default('active')
398398
.describe(
399-
'Which lifecycle set to read from: `active` (default) resolves live files only and returns `404` for a file a `DELETE` soft-deleted; `archived` also resolves soft-deleted files, so metadata stays readable before `POST /files/{fileId}/restore`. Authorization is identical for both.'
399+
'Which lifecycle set to read from: `active` (default) resolves live files only and returns `404` for a file a delete soft-deleted; `archived` also resolves soft-deleted files, so metadata stays readable before the file is restored. Authorization is identical for both.'
400400
),
401401
})
402402
.strict()
@@ -510,7 +510,7 @@ export const v2ListFileFoldersQuerySchema = v2ListFoldersQuerySchema.extend({
510510
scope: v2FileScopeSchema
511511
.default('active')
512512
.describe(
513-
'Which lifecycle set to list: `active` (default) returns live folders only; `archived` returns folders a recursive `DELETE` soft-deleted, which is how a caller finds a path to hand to `POST /api/v2/files/folders/restore`. Authorization is identical for both.'
513+
'Which lifecycle set to list: `active` (default) returns live folders only; `archived` returns folders a recursive delete soft-deleted, which is how a caller finds a path to hand to the folder restore. Authorization is identical for both.'
514514
),
515515
})
516516
export type V2ListFileFoldersQuery = z.output<typeof v2ListFileFoldersQuerySchema>
@@ -526,7 +526,7 @@ export const v2RestoreFileFolderBodySchema = z
526526
.object({
527527
workspaceId: workspaceIdSchema.describe('Workspace that owns the archived folder.'),
528528
path: v2NonRootFolderPathInputSchema.describe(
529-
'Path of the archived folder to restore, as reported by `GET /api/v2/files/folders?scope=archived`.'
529+
'Path of the archived folder to restore, as reported by an archived-scope folder list.'
530530
),
531531
})
532532
.strict()
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
import { describe, expect, it } from 'vitest'
2+
import { v2BulkKnowledgeChunksBodySchema } from '@/lib/api/contracts/v2/knowledge-chunks'
3+
4+
/**
5+
* This description is the only source of the `--chunk` flag help the CLI
6+
* generates, so it and the implementation must state the same rule. It used to
7+
* say unmatched ids were "ignored" while the response returned them in
8+
* `errors[]` — for two of the three operations.
9+
*/
10+
describe('v2 bulk knowledge chunk contract', () => {
11+
const description = v2BulkKnowledgeChunksBodySchema.shape.chunkIds.description ?? ''
12+
13+
it('does not promise unmatched ids are ignored', () => {
14+
expect(description).not.toMatch(/ignored/i)
15+
})
16+
17+
it('points at the field that reports unmatched ids', () => {
18+
expect(description).toMatch(/errors/)
19+
})
20+
})

apps/sim/lib/api/contracts/v2/knowledge-chunks.ts

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -202,15 +202,18 @@ export const v2BulkKnowledgeChunksBodySchema = z
202202
MAX_V2_BULK_KNOWLEDGE_CHUNKS,
203203
`chunkIds cannot contain more than ${MAX_V2_BULK_KNOWLEDGE_CHUNKS} chunks`
204204
)
205-
.describe('Chunks to operate on, by identifier. Ids outside the document are ignored.'),
205+
.describe(
206+
'Chunks to operate on, by identifier. An id naming no chunk in the document is reported in errors and does not fail the request.'
207+
),
206208
})
207209
.strict()
208210
export type V2BulkKnowledgeChunksBody = z.input<typeof v2BulkKnowledgeChunksBodySchema>
209211

210212
/**
211213
* Bulk chunk outcome. Unlike the per-chunk operations this is best-effort: an
212-
* identifier naming no chunk in the document is skipped rather than failing the
213-
* request, so `processed` is the authoritative count.
214+
* identifier naming no chunk in the document is reported in `errors` rather
215+
* than failing the request, and the same rule holds for all three operations.
216+
* `processed` counts only the chunks that actually changed.
214217
*/
215218
export const v2BulkKnowledgeChunksDataSchema = z
216219
.object({
@@ -223,7 +226,9 @@ export const v2BulkKnowledgeChunksDataSchema = z
223226
.meta({ examples: [12] }),
224227
errors: z
225228
.array(z.string())
226-
.describe('Per-chunk failures. A populated array still answers 200.'),
229+
.describe(
230+
'Per-chunk failures, including any identifier that named no chunk in the document. A populated array still answers 200.'
231+
),
227232
})
228233
.strict()
229234
.meta({

apps/sim/lib/api/contracts/v2/tables.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -436,7 +436,7 @@ export const v2ListTablesQuerySchema = z
436436
scope: v2TableScopeSchema
437437
.default('active')
438438
.describe(
439-
'Which lifecycle set to list: `active` (default) for live tables, `archived` for tables a `DELETE` archived and `POST /tables/{tableId}/restore` can bring back. `folderPath` resolves against active folders only, so pairing it with `scope=archived` returns an empty page when the containing folder was archived too.'
439+
'Which lifecycle set to list: `active` (default) for live tables, `archived` for tables a delete archived and a table restore can bring back. `folderPath` resolves against active folders only, so pairing it with `scope=archived` returns an empty page when the containing folder was archived too.'
440440
),
441441
folderPath: v2FolderPathInputSchema
442442
.optional()
@@ -664,7 +664,7 @@ export const v2RestoreTableFolderBodySchema = z
664664
.object({
665665
workspaceId: workspaceIdSchema.describe('Workspace that owns the archived folder.'),
666666
path: v2NonRootFolderPathInputSchema.describe(
667-
'Path the folder held when `DELETE /api/v2/tables/folders` archived it.'
667+
'Path the folder held when a folder delete archived it.'
668668
),
669669
})
670670
.strict()
@@ -1185,7 +1185,7 @@ export const v2UpsertTableRowBodySchema = upsertTableRowBodySchema
11851185
.omit(OMIT_PRIVATE_PROVENANCE)
11861186
.extend({
11871187
data: v2RowDataSchema.describe(
1188-
'Complete set of row cells keyed by column name. On the update branch this REPLACES the matched row: any column not present here is cleared, unlike the merging `PATCH /api/v2/tables/{tableId}/rows/{rowId}`.'
1188+
'Complete set of row cells keyed by column name. On the update branch this REPLACES the matched row: any column not present here is cleared, unlike a single-row update, which merges.'
11891189
),
11901190
})
11911191
.strict()

0 commit comments

Comments
 (0)