Skip to content

Commit eb50a58

Browse files
waleedlatif1waleedlatif1
committed
fix(v2): correct three regressions this branch introduced, and harden its tests
Adversarial review of the previous commit found that three of its "behavior preserving" claims were wrong. Each is corrected here at the layer that owns it. Table filters no longer coerce a `date` operand, and no longer throw. `date` is the one column type whose registry `coerce` is not idempotent — it drops sub-second precision — and the leaf that compiles a filter also builds the unique-constraint and upsert-conflict probes, so re-reading an already-coerced operand could stop it matching the row it was written from and admit a duplicate inside the write transaction with no error. Throwing was the second mistake: the v2 predicate grammar type-checks structure but not operand values, so a rejected operand no longer failed at submission but inside the delete, update, dispatch and cancel runners, where a filter that cannot compile means the cells it started can no longer be cancelled. Coercion is now total — it rewrites what the registry accepts and passes everything else through unchanged, exactly as before. Reviving a force-failed run no longer inherits its terminal duration. Writing `ended_at` and `total_duration_ms` on the force-fail boundary was correct in isolation, but a partial resume flips that row back to `pending` and those columns survived. The preserved value is meant to be the pause checkpoint — the run's active time — and it had become wall clock measured at the failed resume, which the checkpoint rule then faithfully carried into the next terminal write. The revival clears them only for a row that was terminal, so an ordinary paused row keeps the checkpoint it is supposed to keep. Cancelling reports the terminal state it actually observed. Reclassification now requires that nothing else went wrong, so a genuine paused-reconciliation failure survives instead of being rewritten as an already-terminal no-op, and the claim's own row count — not a snapshot read before it — decides whether this cancel terminalized the run or lost a race to something else. The status the snapshot needed rides along on the ownership query that already reads the row, rather than the second read that query's own contract warns against. A custom tool that cannot be projected now answers the same way everywhere: the list omits it, and reading or patching it by id reports it as absent rather than as a server fault. Analytics stops reporting a cancellation for a request that cancelled nothing. The tests around all of this were audited by mutating each fix and checking the suite noticed. Where it did not, the assertion is stronger now: the absent content-type branch is genuinely exercised rather than relying on a header the client library supplies, the duration encoder is pinned to the column it must measure from, execution ownership is pinned to both ids it must match, and the archived-file concealment test proves it conceals the archived read specifically. Two tests that asserted a paused branch they could not observe are gone; the rendered-SQL test that can decide it already covers them.
1 parent 31631f9 commit eb50a58

18 files changed

Lines changed: 893 additions & 230 deletions

File tree

‎apps/sim/app/api/v2/custom-tools/[id]/route.test.ts‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,56 @@ describe('/api/v2/custom-tools/[id]', () => {
188188
expect(mocks.update).not.toHaveBeenCalled()
189189
})
190190

191+
/**
192+
* The list omits a row it cannot project, so this surface must not answer the
193+
* same row with a 500 — a caller who lists and sees nothing, then fetches by
194+
* id and sees a server fault, can act on neither answer. Both surfaces say
195+
* "not addressable here"; the recoveries (DELETE, or a PATCH carrying a valid
196+
* schema) do not go through the projection and still work.
197+
*/
198+
describe('a stored row that cannot be projected onto the contract', () => {
199+
const unrepairable = { ...tool, schema: 'this is not json' }
200+
const repairable = { ...tool, schema: JSON.stringify(tool.schema) }
201+
202+
it('answers a read with the same 404 the list implies by omitting it', async () => {
203+
mocks.get.mockResolvedValue({ tool: unrepairable })
204+
205+
const response = await GET(request('GET'), context)
206+
207+
expect(response.status).toBe(404)
208+
expect((await response.json()).error).toMatchObject({
209+
code: 'NOT_FOUND',
210+
message: 'Custom tool not found',
211+
})
212+
})
213+
214+
it('answers a write with the same 404, leaving delete and a full-schema patch as the recoveries', async () => {
215+
mocks.update.mockResolvedValue({ tool: unrepairable })
216+
expect(
217+
(await PATCH(request('PATCH', { workspaceId: WORKSPACE_ID, code: 'return 2' }), context))
218+
.status
219+
).toBe(404)
220+
221+
mocks.remove.mockResolvedValue({ tool: unrepairable })
222+
expect((await DELETE(request('DELETE'), context)).status).toBe(200)
223+
})
224+
225+
it('serves a repairable row on both single-resource verbs', async () => {
226+
mocks.get.mockResolvedValue({ tool: repairable })
227+
const read = await GET(request('GET'), context)
228+
expect(read.status).toBe(200)
229+
expect((await read.json()).data.schema).toEqual(tool.schema)
230+
231+
mocks.update.mockResolvedValue({ tool: repairable })
232+
const written = await PATCH(
233+
request('PATCH', { workspaceId: WORKSPACE_ID, code: 'return 2' }),
234+
context
235+
)
236+
expect(written.status).toBe(200)
237+
expect((await written.json()).data.schema).toEqual(tool.schema)
238+
})
239+
})
240+
191241
it('conceals cross-tenant access while preserving same-workspace role denials', async () => {
192242
mocks.get.mockRejectedValueOnce(new NoWorkspaceAccessError())
193243
expect((await GET(request('GET'), context)).status).toBe(404)

‎apps/sim/app/api/v2/custom-tools/[id]/route.ts‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,25 @@ import {
1515
getWorkspaceCustomToolUseCase,
1616
updateWorkspaceCustomToolUseCase,
1717
} from '@/lib/custom-tools/application/use-cases'
18-
import { toV2CustomTool } from '@/app/api/v2/custom-tools/utils'
18+
import { MalformedCustomToolRowError, toV2CustomTool } from '@/app/api/v2/custom-tools/utils'
19+
import { v2CaughtOrchestrationError, v2Error } from '@/app/api/v2/lib/response'
1920

2021
export const dynamic = 'force-dynamic'
2122
export const revalidate = 0
2223

24+
const NOT_FOUND_MESSAGE = 'Custom tool not found'
25+
26+
/**
27+
* Conceals cross-tenant denials, and answers a row that cannot be projected
28+
* onto the contract with the same `404` — so this surface and the list, which
29+
* omits such a row, tell one caller one story. See {@link toV2CustomTool}.
30+
*/
2331
const customToolResourceErrorPolicy = createV2ResourceConcealmentPolicy({
24-
notFoundMessage: 'Custom tool not found',
32+
notFoundMessage: NOT_FOUND_MESSAGE,
33+
render: (error) =>
34+
error instanceof MalformedCustomToolRowError
35+
? v2Error('NOT_FOUND', NOT_FOUND_MESSAGE)
36+
: v2CaughtOrchestrationError(error),
2537
})
2638

2739
/** GET /api/v2/custom-tools/[id] — Fetch a single custom tool. */

‎apps/sim/app/api/v2/custom-tools/route.test.ts‎

Lines changed: 90 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,24 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'
66

77
const { mocks, log, MockV2ApiKeyUnauthenticatedError } = vi.hoisted(() => {
88
class MockV2ApiKeyUnauthenticatedError extends Error {}
9+
/**
10+
* The same surface `createMockLogger` provides, because this stub *replaces*
11+
* the global `@sim/logger` mock for this file. A narrower one is not merely
12+
* incomplete — the first module in this route's graph to call `logger.trace`
13+
* or `logger.child` would throw `TypeError` here and nowhere else, which reads
14+
* as a route bug rather than a missing mock method. `child`/`withMetadata`
15+
* return the same instance so a chained call still records on `log`.
16+
*/
17+
const log: Record<string, unknown> = {
18+
info: vi.fn(),
19+
warn: vi.fn(),
20+
error: vi.fn(),
21+
debug: vi.fn(),
22+
trace: vi.fn(),
23+
fatal: vi.fn(),
24+
}
25+
log.child = vi.fn(() => log)
26+
log.withMetadata = vi.fn(() => log)
927
return {
1028
mocks: {
1129
authenticate: vi.fn(),
@@ -15,7 +33,16 @@ const { mocks, log, MockV2ApiKeyUnauthenticatedError } = vi.hoisted(() => {
1533
list: vi.fn(),
1634
create: vi.fn(),
1735
},
18-
log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() },
36+
log: log as {
37+
info: ReturnType<typeof vi.fn>
38+
warn: ReturnType<typeof vi.fn>
39+
error: ReturnType<typeof vi.fn>
40+
debug: ReturnType<typeof vi.fn>
41+
trace: ReturnType<typeof vi.fn>
42+
fatal: ReturnType<typeof vi.fn>
43+
child: ReturnType<typeof vi.fn>
44+
withMetadata: ReturnType<typeof vi.fn>
45+
},
1946
MockV2ApiKeyUnauthenticatedError,
2047
}
2148
})
@@ -308,24 +335,77 @@ describe('/api/v2/custom-tools', () => {
308335
expect(status).toBe(200)
309336
expect(body.data.map((t: { id: string }) => t.id)).toEqual(['tool-1'])
310337
for (const toolId of ['unparseable', 'no-parameters-type']) {
311-
expect(log.warn).toHaveBeenCalledWith(
312-
expect.stringContaining('Omitted'),
338+
expect(log.error).toHaveBeenCalledWith(
339+
expect.stringContaining('cannot be projected'),
313340
expect.objectContaining({ toolId, workspaceId: WORKSPACE_ID })
314341
)
315342
}
316343
})
317344

318-
it('still mints a next cursor when the page contained a skipped row', async () => {
319-
mocks.list.mockResolvedValue({
320-
tools: [malformed('unparseable', 'this is not json')],
321-
nextCursorKeys: ['2026-01-01T00:00:00.000Z', 'unparseable'],
322-
})
345+
/**
346+
* The repair guards must be unreachable for a row that already validates —
347+
* otherwise the recovery path could rewrite rows it was never meant to
348+
* touch. Pinned on a stored schema carrying an unrelated extension key, so
349+
* a repair that rebuilt the object rather than leaving it alone would show
350+
* up as a lost field rather than passing on a shallow shape check.
351+
*/
352+
it('emits a valid row exactly as stored, with no repair applied', async () => {
353+
const stored = {
354+
...TOOL_SCHEMA,
355+
'x-vendor': { owner: 'billing' },
356+
function: { ...TOOL_SCHEMA.function, description: 'Look up an order' },
357+
}
358+
mocks.list.mockResolvedValue({ tools: [{ ...tool, schema: stored }] })
323359

324360
const { status, body } = await list()
325361

326362
expect(status).toBe(200)
327-
expect(body.data).toEqual([])
328-
expect(body.nextCursor).toEqual(expect.any(String))
363+
expect(body.data[0].schema).toEqual(stored)
364+
expect(log.warn).not.toHaveBeenCalledWith(
365+
expect.stringContaining('Repaired'),
366+
expect.anything()
367+
)
368+
expect(log.error).not.toHaveBeenCalledWith(
369+
expect.stringContaining('cannot be projected'),
370+
expect.anything()
371+
)
372+
})
373+
374+
/**
375+
* A page whose rows all skip returns `data: []` with a non-null
376+
* `nextCursor`, so `nextCursor` — never page length — is this list's
377+
* completeness signal. Pins that the documented client loop terminates and
378+
* observes every projectable row across an all-skipped page.
379+
*/
380+
it('lets a client following nextCursor terminate and see every projectable row', async () => {
381+
const second = { ...tool, id: 'tool-2', title: 'refund_order' }
382+
const pages = [
383+
{ tools: [tool, malformed('bad-1', 'this is not json')], nextCursorKeys: ['a', 'bad-1'] },
384+
{ tools: [malformed('bad-2', 'this is not json')], nextCursorKeys: ['b', 'bad-2'] },
385+
{ tools: [second] },
386+
]
387+
mocks.list.mockImplementation(async ({ input }) =>
388+
input.cursorKeys === undefined ? pages[0] : pages[input.cursorKeys[0] === 'a' ? 1 : 2]
389+
)
390+
391+
const seen: string[] = []
392+
const pageSizes: number[] = []
393+
let cursor: string | null = null
394+
do {
395+
expect(pageSizes.length).toBeLessThan(pages.length)
396+
const query = cursor ? `&cursor=${encodeURIComponent(cursor)}` : ''
397+
const response = await GET(
398+
request('GET', `/api/v2/custom-tools?workspaceId=${WORKSPACE_ID}${query}`)
399+
)
400+
expect(response.status).toBe(200)
401+
const body = await response.json()
402+
for (const t of body.data) seen.push(t.id)
403+
pageSizes.push(body.data.length)
404+
cursor = body.nextCursor
405+
} while (cursor !== null)
406+
407+
expect(pageSizes).toEqual([1, 0, 1])
408+
expect(seen).toEqual(['tool-1', 'tool-2'])
329409
})
330410
})
331411

‎apps/sim/app/api/v2/custom-tools/utils.ts‎

Lines changed: 63 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,14 @@ type CustomToolRow = typeof customTools.$inferSelect
1313
* A stored row whose `schema` column cannot be projected onto the public
1414
* contract even after the safe repairs in {@link repairStoredSchema}.
1515
*
16-
* Single-resource surfaces throw this; the list surface skips the row instead
17-
* so one corrupt row cannot make a whole page unreachable.
16+
* Thrown by {@link toV2CustomTool} and consumed by the single-resource route's
17+
* error policy, which renders it as the same `404 Custom tool not found` the
18+
* list surface implies by omitting the row. The identifying detail lives in the
19+
* message and in the structured `error` log at the throw site, so this carries
20+
* no fields of its own.
1821
*/
1922
export class MalformedCustomToolRowError extends Error {
20-
constructor(
21-
readonly toolId: string,
22-
readonly reason: string
23-
) {
23+
constructor(toolId: string, reason: string) {
2424
super(`Custom tool ${toolId} has a malformed stored schema: ${reason}`)
2525
this.name = 'MalformedCustomToolRowError'
2626
}
@@ -43,6 +43,13 @@ function rowIdentity(row: CustomToolRow) {
4343
* The contract types that field as `z.literal('function')`, so there is
4444
* exactly one legal value and filling it invents no information.
4545
*
46+
* Neither branch can fire on a row that was already contract-valid: a valid
47+
* `schema` is an object, never a string, and the contract types its top-level
48+
* `type` as a required `z.literal('function')`, so it is never `undefined`.
49+
* Both guards therefore only see shapes that had already failed validation, and
50+
* repair can only turn a rejection into an acceptance — never alter a row that
51+
* would have been emitted as stored.
52+
*
4653
* Deliberately NOT repaired: `function.parameters.type`, which the contract
4754
* types as an open `z.string()`. Substituting `'object'` there would be a guess
4855
* about JSON-Schema semantics that changes how a model calls the tool.
@@ -71,7 +78,9 @@ function repairStoredSchema(stored: unknown): { value: unknown; repairs: string[
7178
/**
7279
* Projects a stored row onto the public contract, repairing what is safely
7380
* repairable. Reports a reason instead when the row cannot be made
74-
* contract-valid.
81+
* contract-valid, and logs that failure here — one signal per defective row, so
82+
* both surfaces raise it identically rather than each describing the row in its
83+
* own words.
7584
*
7685
* `workspaceId` and `userId` are internal scoping columns and are not exposed.
7786
*/
@@ -88,11 +97,14 @@ function projectV2CustomTool(row: CustomToolRow): { tool: V2CustomTool } | { rea
8897
})
8998

9099
if (!parsed.success) {
91-
return {
92-
reason: parsed.error.issues
93-
.map((issue) => `${issue.path.join('.') || '<root>'}: ${issue.message}`)
94-
.join('; '),
95-
}
100+
const reason = parsed.error.issues
101+
.map((issue) => `${issue.path.join('.') || '<root>'}: ${issue.message}`)
102+
.join('; ')
103+
logger.error('Custom tool row cannot be projected onto the v2 contract', {
104+
...rowIdentity(row),
105+
reason,
106+
})
107+
return { reason }
96108
}
97109

98110
if (repairs.length > 0) {
@@ -107,51 +119,63 @@ function projectV2CustomTool(row: CustomToolRow): { tool: V2CustomTool } | { rea
107119

108120
/**
109121
* Public custom tool projection for single-resource surfaces (read, create,
110-
* update), where there is no other row to serve and failing loudly is the
111-
* honest outcome.
122+
* update).
112123
*
113124
* @throws {MalformedCustomToolRowError} when the row is not contract-valid.
125+
* The single-resource routes render that as `404 Custom tool not found`, which
126+
* is the same answer {@link toV2CustomToolList} gives by omitting the row. A
127+
* `500` there would leave the two surfaces contradicting each other about one
128+
* row — listed as absent, fetched as a server fault — and a caller could act on
129+
* neither. `404` states the one thing that is true of the row on this API: it
130+
* cannot be addressed here. It also stays actionable, because the recoveries
131+
* do not go through this projection — `DELETE` removes the row, and a `PATCH`
132+
* supplying a contract-valid `schema` repairs it and returns `200`.
114133
*/
115134
export function toV2CustomTool(row: CustomToolRow): V2CustomTool {
116135
const result = projectV2CustomTool(row)
117-
if ('reason' in result) {
118-
logger.error('Custom tool row cannot be projected onto the v2 contract', {
119-
...rowIdentity(row),
120-
reason: result.reason,
121-
})
122-
throw new MalformedCustomToolRowError(row.id, result.reason)
123-
}
136+
if ('reason' in result) throw new MalformedCustomToolRowError(row.id, result.reason)
124137
return result.tool
125138
}
126139

127140
/**
128141
* Public custom tool projection for the keyset-paginated list.
129142
*
130-
* Rows that stay malformed after repair are omitted and logged at `warn` rather
131-
* than thrown. Throwing here fails the whole page, and because the list is
132-
* keyset-paginated the caller cannot page past the bad row — every page
133-
* containing it becomes permanently unreachable. An incomplete page is a real
134-
* cost, but it is strictly smaller than no page at all, and the omission is
135-
* recorded server-side with enough identity to find and fix the row.
143+
* Rows that stay malformed after repair are omitted rather than thrown.
144+
* Throwing here fails the whole page, and because the list is keyset-paginated
145+
* the caller cannot page past the bad row — every page containing it becomes
146+
* permanently unreachable. An incomplete page is a real cost, but it is
147+
* strictly smaller than no page at all.
136148
*
137-
* Pagination stays coherent: `nextCursor` is minted from the keys the use case
138-
* read out of the database, not from this projection, so a skipped row still
139-
* advances the cursor past itself. The list response carries no total, so no
140-
* count metadata contradicts a short page — a caller must follow `nextCursor`
141-
* rather than infer completeness from a page's length.
149+
* The page is deliberately **not** drained back up to the requested limit. A
150+
* page whose rows all skip therefore returns `data: []` with a non-null
151+
* `nextCursor`, which is safe because `nextCursor` — not page length — is this
152+
* list's completeness signal:
153+
*
154+
* - `listWorkspaceCustomTools` reads `limit + 1` rows and `keysetPage` mints
155+
* `nextCursorKeys` from the extra row, so `nextCursor` is null exactly when
156+
* the keyset is exhausted, independent of anything this projection does.
157+
* - Each page resumes strictly after the last row the previous page *read*, not
158+
* the last row it *emitted*, so a skipped row still advances the cursor past
159+
* itself and is never revisited.
160+
* - A caller looping `while (nextCursor !== null)` therefore terminates in
161+
* `ceil(rows / limit)` requests over any workspace and observes every
162+
* projectable row exactly once — including rows that follow an all-skipped
163+
* page.
164+
*
165+
* Draining instead would mean re-entering the authorized list use case from a
166+
* presenter, which is the surface adapter re-reading protected data, and it
167+
* would still need a bound — so a workspace of mostly-defective rows would
168+
* return a short page anyway, just less predictably. The trap it removes is a
169+
* caller looping on `data.length`, which this list has never been able to
170+
* promise: the response carries no total, and page length has always been an
171+
* artifact of the read rather than a statement about the keyset.
142172
*/
143173
export function toV2CustomToolList(rows: CustomToolRow[]): V2CustomTool[] {
144174
const tools: V2CustomTool[] = []
145175

146176
for (const row of rows) {
147177
const result = projectV2CustomTool(row)
148-
if ('reason' in result) {
149-
logger.warn('Omitted a malformed custom tool row from the v2 list response', {
150-
...rowIdentity(row),
151-
reason: result.reason,
152-
})
153-
continue
154-
}
178+
if ('reason' in result) continue
155179
tools.push(result.tool)
156180
}
157181

0 commit comments

Comments
 (0)