diff --git a/apps/docs/openapi-v2-billing.json b/apps/docs/openapi-v2-billing.json index 5b513c1c03d..77de6a7d2ca 100644 --- a/apps/docs/openapi-v2-billing.json +++ b/apps/docs/openapi-v2-billing.json @@ -47,7 +47,8 @@ "schema": { "description": "Workspace whose payer should be resolved. Workspace API keys are pinned to their own workspace.", "type": "string", - "minLength": 1 + "minLength": 1, + "maxLength": 128 } } ], @@ -133,7 +134,8 @@ "schema": { "description": "Restrict results to one workspace whose payer the caller can inspect.", "type": "string", - "minLength": 1 + "minLength": 1, + "maxLength": 128 } }, { diff --git a/apps/docs/openapi-v2-files-audit.json b/apps/docs/openapi-v2-files-audit.json index e842423e106..045fd369674 100644 --- a/apps/docs/openapi-v2-files-audit.json +++ b/apps/docs/openapi-v2-files-audit.json @@ -51,6 +51,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose files should be listed." } }, @@ -343,6 +344,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the upload session." } }, @@ -433,6 +435,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the upload session." } }, @@ -537,6 +540,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the upload session." } }, @@ -629,6 +633,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the file." } } @@ -721,6 +726,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the file." } } @@ -965,8 +971,21 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the file." } + }, + { + "name": "scope", + "in": "query", + "required": false, + "description": "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.", + "schema": { + "default": "active", + "description": "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.", + "type": "string", + "enum": ["active", "archived"] + } } ], "responses": { @@ -1060,7 +1079,8 @@ "schema": { "description": "Filter to actions in one workspace.", "type": "string", - "minLength": 1 + "minLength": 1, + "maxLength": 128 } }, { @@ -1361,6 +1381,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the file." } } @@ -1655,6 +1676,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose folders should be listed." } }, @@ -1898,6 +1920,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the folder." } }, @@ -2437,6 +2460,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the file." }, "name": { @@ -2651,6 +2675,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which the file will be registered." }, "name": { @@ -2822,6 +2847,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the file." }, "name": { @@ -2877,6 +2903,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the archived file." } }, @@ -3352,6 +3379,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the files." }, "fileIds": { @@ -3446,6 +3474,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the file." }, "isActive": { @@ -3496,6 +3525,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the file." }, "content": { @@ -3572,6 +3602,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the files." }, "fileIds": { @@ -3683,6 +3714,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the folder." }, "path": { @@ -3701,6 +3733,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the folder." }, "path": { diff --git a/apps/docs/openapi-v2-knowledge.json b/apps/docs/openapi-v2-knowledge.json index 24157c46f4e..6f3da73eccb 100644 --- a/apps/docs/openapi-v2-knowledge.json +++ b/apps/docs/openapi-v2-knowledge.json @@ -47,6 +47,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose knowledge bases should be listed." } }, @@ -885,6 +886,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge base." } } @@ -1077,6 +1079,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge base." } }, @@ -1178,6 +1181,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge base." } }, @@ -1293,6 +1297,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge base." } }, @@ -1635,6 +1640,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose folders should be listed." } }, @@ -1881,6 +1887,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the folder." } }, @@ -2469,6 +2476,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the knowledge base." }, "name": { @@ -2509,6 +2517,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge base." }, "name": { @@ -2798,6 +2807,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge bases." }, "knowledgeBaseIds": { @@ -3116,6 +3126,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge base." }, "operation": { @@ -3467,6 +3478,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge base." }, "name": { @@ -3918,6 +3930,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the knowledge base." }, "filename": { @@ -4113,6 +4126,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the folder." }, "path": { @@ -4131,6 +4145,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the folder." }, "path": { diff --git a/apps/docs/openapi-v2-logs.json b/apps/docs/openapi-v2-logs.json index 0a64c9f581f..46f3e4fec47 100644 --- a/apps/docs/openapi-v2-logs.json +++ b/apps/docs/openapi-v2-logs.json @@ -47,6 +47,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose execution logs should be returned." } }, diff --git a/apps/docs/openapi-v2-resources.json b/apps/docs/openapi-v2-resources.json index 51e85c8125b..30d0b7a5ff5 100644 --- a/apps/docs/openapi-v2-resources.json +++ b/apps/docs/openapi-v2-resources.json @@ -67,6 +67,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace to retrieve." } } @@ -132,6 +133,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace to retrieve." } }, @@ -221,6 +223,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the MCP server." } }, @@ -424,6 +427,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the MCP server." } } @@ -575,6 +579,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the MCP server." } } @@ -651,6 +656,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the MCP server." } }, @@ -729,6 +735,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the skill." } }, @@ -932,6 +939,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the skill." } } @@ -1086,6 +1094,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the skill." } } @@ -1151,6 +1160,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the custom tool." } }, @@ -1354,6 +1364,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the custom tool." } } @@ -1508,6 +1519,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the custom tool." } } @@ -1573,6 +1585,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose credentials should be listed." } }, @@ -1720,6 +1733,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose secret metadata should be listed." } }, @@ -1969,6 +1983,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "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." } }, @@ -2291,6 +2306,7 @@ "id": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "name": { @@ -2668,6 +2684,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to register the server." }, "name": { @@ -2854,6 +2871,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the MCP server." }, "name": { @@ -3252,6 +3270,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the skill." }, "name": { @@ -3345,6 +3364,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the skill." }, "name": { @@ -3618,6 +3638,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the custom tool." }, "title": { @@ -3811,6 +3832,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the custom tool." }, "title": { @@ -4182,6 +4204,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "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": { diff --git a/apps/docs/openapi-v2-tables.json b/apps/docs/openapi-v2-tables.json index b1bff5d7222..aa729c5c616 100644 --- a/apps/docs/openapi-v2-tables.json +++ b/apps/docs/openapi-v2-tables.json @@ -47,6 +47,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose tables should be listed." } }, @@ -739,6 +740,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the table." } }, @@ -2648,6 +2650,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the transfer resource." } }, @@ -2733,6 +2736,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the transfer resource." } }, @@ -2823,6 +2827,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the transfer resource." } }, @@ -2927,6 +2932,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the transfer resource." } }, @@ -3102,6 +3108,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the transfer resource." } } @@ -3176,6 +3183,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the transfer resource." } } @@ -3255,6 +3263,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the transfer resource." } } @@ -3402,6 +3411,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose folders should be listed." } }, @@ -3648,6 +3658,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the folder." } }, @@ -4304,6 +4315,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "schema": { @@ -4455,6 +4467,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "name": { @@ -4588,6 +4601,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the table." }, "column": { @@ -4680,6 +4694,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace that owns the table." }, "columnName": { @@ -4768,6 +4783,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "columnName": { @@ -4929,6 +4945,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "rows": { @@ -4950,6 +4967,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "data": { @@ -5161,6 +5179,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "filter": { @@ -5232,6 +5251,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "filter": { @@ -5284,6 +5304,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "data": { @@ -5372,6 +5393,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "data": { @@ -5432,6 +5454,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "predicate": { @@ -5532,6 +5555,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "predicate": { @@ -6481,6 +6505,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "group": { @@ -6668,6 +6693,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "groupId": { @@ -6938,6 +6964,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "groupId": { @@ -6996,6 +7023,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "groupIds": { @@ -7084,6 +7112,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." } }, @@ -7158,6 +7187,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "q": { @@ -7676,6 +7706,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "source": { @@ -8159,6 +8190,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "format": { @@ -8274,6 +8306,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Unique workspace identifier." }, "scope": { @@ -8398,6 +8431,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the folder." }, "path": { @@ -8429,6 +8463,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the folder." }, "path": { diff --git a/apps/docs/openapi-v2-workflows.json b/apps/docs/openapi-v2-workflows.json index 6d9fdd9da01..e0b56db6970 100644 --- a/apps/docs/openapi-v2-workflows.json +++ b/apps/docs/openapi-v2-workflows.json @@ -51,6 +51,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose workflows should be listed." } }, @@ -1712,6 +1713,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace whose folders should be listed." } }, @@ -1964,6 +1966,7 @@ "schema": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the folder." } }, @@ -2499,6 +2502,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the workflow." }, "name": { @@ -3828,6 +3832,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to import the workflow." }, "workflow": { @@ -4660,7 +4665,7 @@ }, "durablyRecorded": { "type": "boolean", - "description": "Whether cancellation was recorded durably." + "description": "Whether this request durably recorded a cancellation. Always false for a run that was already terminal, where the request is satisfied but nothing was written." }, "locallyAborted": { "type": "boolean", @@ -4671,10 +4676,13 @@ "description": "Whether a paused execution was cancelled." }, "reason": { - "description": "Machine-readable cancellation outcome, present on every cancellation including full successes. `recorded` is the success value. `redis_unavailable` and `redis_write_failed` mean the distributed cancellation signal was not written, so an already-running execution may not observe the cancellation. `paused_event_publish_failed` and `paused_database_cancel_failed` name the failing step for a paused run.", + "description": "Machine-readable cancellation outcome, present on every cancellation including full successes. `recorded` is the success value. `already_cancelled`, `already_completed`, and `already_failed` mean the run had already reached that terminal state, so nothing was cancelled and `durablyRecorded` is false. `redis_unavailable` and `redis_write_failed` mean the distributed cancellation signal was not written, so an already-running execution may not observe the cancellation. `paused_event_publish_failed` and `paused_database_cancel_failed` name the failing step for a paused run.", "type": "string", "enum": [ "recorded", + "already_cancelled", + "already_completed", + "already_failed", "redis_unavailable", "redis_write_failed", "paused_event_publish_failed", @@ -4692,7 +4700,7 @@ ], "additionalProperties": false, "title": "Cancel workflow run result", - "description": "Outcome of a workflow run cancellation request. Cancellation is best-effort: a run already in a terminal state succeeds with no effect, so poll the run to observe its final state." + "description": "Outcome of a workflow run cancellation request. Cancellation is best-effort: a run already in a terminal state succeeds with no effect, reported as `durablyRecorded: false` with an `already_*` reason naming the state observed." }, "CancelWorkflowRunResponse": { "type": "object", @@ -4838,6 +4846,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace in which to create the folder." }, "path": { @@ -4881,6 +4890,7 @@ "workspaceId": { "type": "string", "minLength": 1, + "maxLength": 128, "description": "Workspace containing the folder." }, "path": { diff --git a/apps/sim/app/api/v2/custom-tools/[id]/route.test.ts b/apps/sim/app/api/v2/custom-tools/[id]/route.test.ts index ca4784712e4..11a5b6956c0 100644 --- a/apps/sim/app/api/v2/custom-tools/[id]/route.test.ts +++ b/apps/sim/app/api/v2/custom-tools/[id]/route.test.ts @@ -188,6 +188,56 @@ describe('/api/v2/custom-tools/[id]', () => { expect(mocks.update).not.toHaveBeenCalled() }) + /** + * The list omits a row it cannot project, so this surface must not answer the + * same row with a 500 — a caller who lists and sees nothing, then fetches by + * id and sees a server fault, can act on neither answer. Both surfaces say + * "not addressable here"; the recoveries (DELETE, or a PATCH carrying a valid + * schema) do not go through the projection and still work. + */ + describe('a stored row that cannot be projected onto the contract', () => { + const unrepairable = { ...tool, schema: 'this is not json' } + const repairable = { ...tool, schema: JSON.stringify(tool.schema) } + + it('answers a read with the same 404 the list implies by omitting it', async () => { + mocks.get.mockResolvedValue({ tool: unrepairable }) + + const response = await GET(request('GET'), context) + + expect(response.status).toBe(404) + expect((await response.json()).error).toMatchObject({ + code: 'NOT_FOUND', + message: 'Custom tool not found', + }) + }) + + it('answers a write with the same 404, leaving delete and a full-schema patch as the recoveries', async () => { + mocks.update.mockResolvedValue({ tool: unrepairable }) + expect( + (await PATCH(request('PATCH', { workspaceId: WORKSPACE_ID, code: 'return 2' }), context)) + .status + ).toBe(404) + + mocks.remove.mockResolvedValue({ tool: unrepairable }) + expect((await DELETE(request('DELETE'), context)).status).toBe(200) + }) + + it('serves a repairable row on both single-resource verbs', async () => { + mocks.get.mockResolvedValue({ tool: repairable }) + const read = await GET(request('GET'), context) + expect(read.status).toBe(200) + expect((await read.json()).data.schema).toEqual(tool.schema) + + mocks.update.mockResolvedValue({ tool: repairable }) + const written = await PATCH( + request('PATCH', { workspaceId: WORKSPACE_ID, code: 'return 2' }), + context + ) + expect(written.status).toBe(200) + expect((await written.json()).data.schema).toEqual(tool.schema) + }) + }) + it('conceals cross-tenant access while preserving same-workspace role denials', async () => { mocks.get.mockRejectedValueOnce(new NoWorkspaceAccessError()) expect((await GET(request('GET'), context)).status).toBe(404) diff --git a/apps/sim/app/api/v2/custom-tools/[id]/route.ts b/apps/sim/app/api/v2/custom-tools/[id]/route.ts index 199c2930a07..bd654423142 100644 --- a/apps/sim/app/api/v2/custom-tools/[id]/route.ts +++ b/apps/sim/app/api/v2/custom-tools/[id]/route.ts @@ -15,13 +15,25 @@ import { getWorkspaceCustomToolUseCase, updateWorkspaceCustomToolUseCase, } from '@/lib/custom-tools/application/use-cases' -import { toV2CustomTool } from '@/app/api/v2/custom-tools/utils' +import { MalformedCustomToolRowError, toV2CustomTool } from '@/app/api/v2/custom-tools/utils' +import { v2CaughtOrchestrationError, v2Error } from '@/app/api/v2/lib/response' export const dynamic = 'force-dynamic' export const revalidate = 0 +const NOT_FOUND_MESSAGE = 'Custom tool not found' + +/** + * Conceals cross-tenant denials, and answers a row that cannot be projected + * onto the contract with the same `404` — so this surface and the list, which + * omits such a row, tell one caller one story. See {@link toV2CustomTool}. + */ const customToolResourceErrorPolicy = createV2ResourceConcealmentPolicy({ - notFoundMessage: 'Custom tool not found', + notFoundMessage: NOT_FOUND_MESSAGE, + render: (error) => + error instanceof MalformedCustomToolRowError + ? v2Error('NOT_FOUND', NOT_FOUND_MESSAGE) + : v2CaughtOrchestrationError(error), }) /** GET /api/v2/custom-tools/[id] — Fetch a single custom tool. */ diff --git a/apps/sim/app/api/v2/custom-tools/route.test.ts b/apps/sim/app/api/v2/custom-tools/route.test.ts index 3b32d07ccab..bc43d671e36 100644 --- a/apps/sim/app/api/v2/custom-tools/route.test.ts +++ b/apps/sim/app/api/v2/custom-tools/route.test.ts @@ -4,8 +4,26 @@ import { NextRequest } from 'next/server' import { beforeEach, describe, expect, it, vi } from 'vitest' -const { mocks, MockV2ApiKeyUnauthenticatedError } = vi.hoisted(() => { +const { mocks, log, MockV2ApiKeyUnauthenticatedError } = vi.hoisted(() => { class MockV2ApiKeyUnauthenticatedError extends Error {} + /** + * The same surface `createMockLogger` provides, because this stub *replaces* + * the global `@sim/logger` mock for this file. A narrower one is not merely + * incomplete — the first module in this route's graph to call `logger.trace` + * or `logger.child` would throw `TypeError` here and nowhere else, which reads + * as a route bug rather than a missing mock method. `child`/`withMetadata` + * return the same instance so a chained call still records on `log`. + */ + const log: Record = { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + trace: vi.fn(), + fatal: vi.fn(), + } + log.child = vi.fn(() => log) + log.withMetadata = vi.fn(() => log) return { mocks: { authenticate: vi.fn(), @@ -15,10 +33,32 @@ const { mocks, MockV2ApiKeyUnauthenticatedError } = vi.hoisted(() => { list: vi.fn(), create: vi.fn(), }, + log: log as { + info: ReturnType + warn: ReturnType + error: ReturnType + debug: ReturnType + trace: ReturnType + fatal: ReturnType + child: ReturnType + withMetadata: ReturnType + }, MockV2ApiKeyUnauthenticatedError, } }) +/** + * Overrides the global logger mock with one stable instance so the malformed-row + * warnings can be asserted — `createLogger` is called at module load, before any + * `beforeEach` could capture the per-call mock the global stub returns. + */ +vi.mock('@sim/logger', () => ({ + createLogger: () => log, + logger: log, + runWithRequestContext: (_ctx: unknown, fn: () => T): T => fn(), + getRequestContext: () => undefined, +})) + vi.mock('@/lib/api/server/routes/v2-api-key-auth', () => ({ authenticateV2ApiKey: mocks.authenticate, V2ApiKeyUnauthenticatedError: MockV2ApiKeyUnauthenticatedError, @@ -230,6 +270,145 @@ describe('/api/v2/custom-tools', () => { expect(mocks.create).not.toHaveBeenCalled() }) + /** + * Both shapes below are real production rows. A single one of them used to + * throw out of the shared response validator and 500 the entire page, and + * because the list is keyset-paginated the caller could never page past it. + */ + describe('malformed stored rows', () => { + const malformed = (id: string, schema: unknown) => ({ ...tool, id, title: id, schema }) + + async function list() { + const response = await GET(request('GET', `/api/v2/custom-tools?workspaceId=${WORKSPACE_ID}`)) + return { status: response.status, body: await response.json() } + } + + it('recovers a schema stored as a stringified JSON object', async () => { + mocks.list.mockResolvedValue({ + tools: [malformed('stringified', JSON.stringify(TOOL_SCHEMA)), tool], + }) + + const { status, body } = await list() + + expect(status).toBe(200) + expect(body.data.map((t: { id: string }) => t.id)).toEqual(['stringified', 'tool-1']) + expect(body.data[0].schema).toEqual(TOOL_SCHEMA) + expect(log.warn).toHaveBeenCalledWith( + expect.stringContaining('Repaired'), + expect.objectContaining({ toolId: 'stringified', repairs: ['parsed-json-string'] }) + ) + }) + + it('recovers a declaration missing the `type` discriminator', async () => { + mocks.list.mockResolvedValue({ + tools: [malformed('no-type', { function: TOOL_SCHEMA.function }), tool], + }) + + const { status, body } = await list() + + expect(status).toBe(200) + expect(body.data.map((t: { id: string }) => t.id)).toEqual(['no-type', 'tool-1']) + expect(body.data[0].schema).toEqual(TOOL_SCHEMA) + expect(log.warn).toHaveBeenCalledWith( + expect.stringContaining('Repaired'), + expect.objectContaining({ + toolId: 'no-type', + repairs: ['filled-function-discriminator'], + }) + ) + }) + + it('serves the rest of the page when a row is not safely repairable', async () => { + mocks.list.mockResolvedValue({ + tools: [ + malformed('unparseable', 'this is not json'), + malformed('no-parameters-type', { + type: 'function', + function: { name: 'x', parameters: { properties: {} } }, + }), + tool, + ], + }) + + const { status, body } = await list() + + expect(status).toBe(200) + expect(body.data.map((t: { id: string }) => t.id)).toEqual(['tool-1']) + for (const toolId of ['unparseable', 'no-parameters-type']) { + expect(log.error).toHaveBeenCalledWith( + expect.stringContaining('cannot be projected'), + expect.objectContaining({ toolId, workspaceId: WORKSPACE_ID }) + ) + } + }) + + /** + * The repair guards must be unreachable for a row that already validates — + * otherwise the recovery path could rewrite rows it was never meant to + * touch. Pinned on a stored schema carrying an unrelated extension key, so + * a repair that rebuilt the object rather than leaving it alone would show + * up as a lost field rather than passing on a shallow shape check. + */ + it('emits a valid row exactly as stored, with no repair applied', async () => { + const stored = { + ...TOOL_SCHEMA, + 'x-vendor': { owner: 'billing' }, + function: { ...TOOL_SCHEMA.function, description: 'Look up an order' }, + } + mocks.list.mockResolvedValue({ tools: [{ ...tool, schema: stored }] }) + + const { status, body } = await list() + + expect(status).toBe(200) + expect(body.data[0].schema).toEqual(stored) + expect(log.warn).not.toHaveBeenCalledWith( + expect.stringContaining('Repaired'), + expect.anything() + ) + expect(log.error).not.toHaveBeenCalledWith( + expect.stringContaining('cannot be projected'), + expect.anything() + ) + }) + + /** + * A page whose rows all skip returns `data: []` with a non-null + * `nextCursor`, so `nextCursor` — never page length — is this list's + * completeness signal. Pins that the documented client loop terminates and + * observes every projectable row across an all-skipped page. + */ + it('lets a client following nextCursor terminate and see every projectable row', async () => { + const second = { ...tool, id: 'tool-2', title: 'refund_order' } + const pages = [ + { tools: [tool, malformed('bad-1', 'this is not json')], nextCursorKeys: ['a', 'bad-1'] }, + { tools: [malformed('bad-2', 'this is not json')], nextCursorKeys: ['b', 'bad-2'] }, + { tools: [second] }, + ] + mocks.list.mockImplementation(async ({ input }) => + input.cursorKeys === undefined ? pages[0] : pages[input.cursorKeys[0] === 'a' ? 1 : 2] + ) + + const seen: string[] = [] + const pageSizes: number[] = [] + let cursor: string | null = null + do { + expect(pageSizes.length).toBeLessThan(pages.length) + const query = cursor ? `&cursor=${encodeURIComponent(cursor)}` : '' + const response = await GET( + request('GET', `/api/v2/custom-tools?workspaceId=${WORKSPACE_ID}${query}`) + ) + expect(response.status).toBe(200) + const body = await response.json() + for (const t of body.data) seen.push(t.id) + pageSizes.push(body.data.length) + cursor = body.nextCursor + } while (cursor !== null) + + expect(pageSizes).toEqual([1, 0, 1]) + expect(seen).toEqual(['tool-1', 'tool-2']) + }) + }) + it('rejects invalid list sort fields before application execution', async () => { const response = await GET( request('GET', `/api/v2/custom-tools?workspaceId=${WORKSPACE_ID}&sortBy=invalid`) diff --git a/apps/sim/app/api/v2/custom-tools/route.ts b/apps/sim/app/api/v2/custom-tools/route.ts index 4cbfafb5492..8f0a9d2c65e 100644 --- a/apps/sim/app/api/v2/custom-tools/route.ts +++ b/apps/sim/app/api/v2/custom-tools/route.ts @@ -14,7 +14,7 @@ import { createWorkspaceCustomToolUseCase, listWorkspaceCustomToolsUseCase, } from '@/lib/custom-tools/application/use-cases' -import { toV2CustomTool } from '@/app/api/v2/custom-tools/utils' +import { toV2CustomTool, toV2CustomToolList } from '@/app/api/v2/custom-tools/utils' import { readSortedCursor, writeSortedCursor } from '@/app/api/v2/lib/response' export const dynamic = 'force-dynamic' @@ -46,7 +46,7 @@ export const GET = defineV2JsonRoute({ }), useCase: listWorkspaceCustomToolsUseCase, present: ({ tools, nextCursorKeys }, { query }) => ({ - data: tools.map(toV2CustomTool), + data: toV2CustomToolList(tools), nextCursor: writeSortedCursor( nextCursorKeys, query.sortBy, diff --git a/apps/sim/app/api/v2/custom-tools/utils.ts b/apps/sim/app/api/v2/custom-tools/utils.ts index d3f1181a10e..e70c0c879dc 100644 --- a/apps/sim/app/api/v2/custom-tools/utils.ts +++ b/apps/sim/app/api/v2/custom-tools/utils.ts @@ -1,21 +1,183 @@ import type { customTools } from '@sim/db/schema' -import type { V2CustomTool } from '@/lib/api/contracts/v2/custom-tools' +import { createLogger } from '@sim/logger' +import { isPlainRecord } from '@sim/utils/object' +import { type V2CustomTool, v2CustomToolSchema } from '@/lib/api/contracts/v2/custom-tools' /** Shared serialization + error mapping for the v2 custom tool surface. */ +const logger = createLogger('V2CustomToolsSerialization') + type CustomToolRow = typeof customTools.$inferSelect /** - * Public custom tool projection. `workspaceId` and `userId` are internal - * scoping columns and are not exposed. + * A stored row whose `schema` column cannot be projected onto the public + * contract even after the safe repairs in {@link repairStoredSchema}. + * + * Thrown by {@link toV2CustomTool} and consumed by the single-resource route's + * error policy, which renders it as the same `404 Custom tool not found` the + * list surface implies by omitting the row. The identifying detail lives in the + * message and in the structured `error` log at the throw site, so this carries + * no fields of its own. */ -export function toV2CustomTool(row: CustomToolRow): V2CustomTool { - return { +export class MalformedCustomToolRowError extends Error { + constructor(toolId: string, reason: string) { + super(`Custom tool ${toolId} has a malformed stored schema: ${reason}`) + this.name = 'MalformedCustomToolRowError' + } +} + +/** Identity fields safe to log for locating a bad row. Never includes `code`. */ +function rowIdentity(row: CustomToolRow) { + return { toolId: row.id, workspaceId: row.workspaceId, title: row.title } +} + +/** + * Safe, information-preserving normalizations for a `schema` column that drifted + * from the contract shape. Both are hypotheses — nothing here is trusted; the + * result is still validated against the response contract before it is emitted, + * so a wrong guess can only downgrade a row to "skipped", never emit bad data. + * + * 1. A `schema` persisted as a JSON *string* is parsed. This is a pure encoding + * fix: the stored bytes already describe the right object. + * 2. A declaration missing the top-level `type` discriminator gets `'function'`. + * The contract types that field as `z.literal('function')`, so there is + * exactly one legal value and filling it invents no information. + * + * Neither branch can fire on a row that was already contract-valid: a valid + * `schema` is an object, never a string, and the contract types its top-level + * `type` as a required `z.literal('function')`, so it is never `undefined`. + * Both guards therefore only see shapes that had already failed validation, and + * repair can only turn a rejection into an acceptance — never alter a row that + * would have been emitted as stored. + * + * Deliberately NOT repaired: `function.parameters.type`, which the contract + * types as an open `z.string()`. Substituting `'object'` there would be a guess + * about JSON-Schema semantics that changes how a model calls the tool. + */ +function repairStoredSchema(stored: unknown): { value: unknown; repairs: string[] } { + const repairs: string[] = [] + let value = stored + + if (typeof value === 'string') { + try { + value = JSON.parse(value) + repairs.push('parsed-json-string') + } catch { + return { value: stored, repairs } + } + } + + if (isPlainRecord(value) && value.type === undefined && isPlainRecord(value.function)) { + value = { ...value, type: 'function' } + repairs.push('filled-function-discriminator') + } + + return { value, repairs } +} + +/** + * Projects a stored row onto the public contract, repairing what is safely + * repairable. Reports a reason instead when the row cannot be made + * contract-valid, and logs that failure here — one signal per defective row, so + * both surfaces raise it identically rather than each describing the row in its + * own words. + * + * `workspaceId` and `userId` are internal scoping columns and are not exposed. + */ +function projectV2CustomTool(row: CustomToolRow): { tool: V2CustomTool } | { reason: string } { + const { value, repairs } = repairStoredSchema(row.schema) + + const parsed = v2CustomToolSchema.safeParse({ id: row.id, title: row.title, - schema: row.schema as V2CustomTool['schema'], + schema: value, code: row.code, createdAt: row.createdAt.toISOString(), updatedAt: row.updatedAt.toISOString(), + }) + + if (!parsed.success) { + const reason = parsed.error.issues + .map((issue) => `${issue.path.join('.') || ''}: ${issue.message}`) + .join('; ') + logger.error('Custom tool row cannot be projected onto the v2 contract', { + ...rowIdentity(row), + reason, + }) + return { reason } } + + if (repairs.length > 0) { + logger.warn('Repaired a malformed stored custom tool schema', { + ...rowIdentity(row), + repairs, + }) + } + + return { tool: parsed.data } +} + +/** + * Public custom tool projection for single-resource surfaces (read, create, + * update). + * + * @throws {MalformedCustomToolRowError} when the row is not contract-valid. + * The single-resource routes render that as `404 Custom tool not found`, which + * is the same answer {@link toV2CustomToolList} gives by omitting the row. A + * `500` there would leave the two surfaces contradicting each other about one + * row — listed as absent, fetched as a server fault — and a caller could act on + * neither. `404` states the one thing that is true of the row on this API: it + * cannot be addressed here. It also stays actionable, because the recoveries + * do not go through this projection — `DELETE` removes the row, and a `PATCH` + * supplying a contract-valid `schema` repairs it and returns `200`. + */ +export function toV2CustomTool(row: CustomToolRow): V2CustomTool { + const result = projectV2CustomTool(row) + if ('reason' in result) throw new MalformedCustomToolRowError(row.id, result.reason) + return result.tool +} + +/** + * Public custom tool projection for the keyset-paginated list. + * + * Rows that stay malformed after repair are omitted rather than thrown. + * Throwing here fails the whole page, and because the list is keyset-paginated + * the caller cannot page past the bad row — every page containing it becomes + * permanently unreachable. An incomplete page is a real cost, but it is + * strictly smaller than no page at all. + * + * The page is deliberately **not** drained back up to the requested limit. A + * page whose rows all skip therefore returns `data: []` with a non-null + * `nextCursor`, which is safe because `nextCursor` — not page length — is this + * list's completeness signal: + * + * - `listWorkspaceCustomTools` reads `limit + 1` rows and `keysetPage` mints + * `nextCursorKeys` from the extra row, so `nextCursor` is null exactly when + * the keyset is exhausted, independent of anything this projection does. + * - Each page resumes strictly after the last row the previous page *read*, not + * the last row it *emitted*, so a skipped row still advances the cursor past + * itself and is never revisited. + * - A caller looping `while (nextCursor !== null)` therefore terminates in + * `ceil(rows / limit)` requests over any workspace and observes every + * projectable row exactly once — including rows that follow an all-skipped + * page. + * + * Draining instead would mean re-entering the authorized list use case from a + * presenter, which is the surface adapter re-reading protected data, and it + * would still need a bound — so a workspace of mostly-defective rows would + * return a short page anyway, just less predictably. The trap it removes is a + * caller looping on `data.length`, which this list has never been able to + * promise: the response carries no total, and page length has always been an + * artifact of the read rather than a statement about the keyset. + */ +export function toV2CustomToolList(rows: CustomToolRow[]): V2CustomTool[] { + const tools: V2CustomTool[] = [] + + for (const row of rows) { + const result = projectV2CustomTool(row) + if ('reason' in result) continue + tools.push(result.tool) + } + + return tools } diff --git a/apps/sim/app/api/v2/files/[fileId]/metadata/route.test.ts b/apps/sim/app/api/v2/files/[fileId]/metadata/route.test.ts index c947dff15a9..82b8deb787f 100644 --- a/apps/sim/app/api/v2/files/[fileId]/metadata/route.test.ts +++ b/apps/sim/app/api/v2/files/[fileId]/metadata/route.test.ts @@ -35,6 +35,7 @@ vi.mock('@/lib/users/queries', () => ({ })) import { NoWorkspaceAccessError } from '@/lib/core/application' +import { OrchestrationError } from '@/lib/core/orchestration/types' import { GET } from '@/app/api/v2/files/[fileId]/metadata/route' const WORKSPACE_ID = 'workspace-1' @@ -79,6 +80,15 @@ function buildRecord() { const callGet = (query: string) => GET(new NextRequest(`http://localhost:3000/api/v2/files/${FILE_ID}/metadata?${query}`), context) +/** + * Stands in for the application use case's lifecycle predicate: a soft-deleted row only + * resolves when the caller opted into the archived set through `includeDeleted`. + */ +const archivedFileUseCase = async ({ input }: { input: { includeDeleted?: boolean } }) => { + if (!input.includeDeleted) throw new OrchestrationError('not_found', 'File not found') + return { file: { ...buildRecord(), deletedAt: new Date('2024-01-03T00:00:00Z') }, share: SHARE } +} + describe('GET /api/v2/files/[fileId]/metadata', () => { beforeEach(() => { vi.clearAllMocks() @@ -138,11 +148,96 @@ describe('GET /api/v2/files/[fileId]/metadata', () => { }) expect(mocks.readMetadata).toHaveBeenCalledWith({ principal: auth.principal, - input: { fileId: FILE_ID, assertedWorkspaceId: WORKSPACE_ID }, + input: { fileId: FILE_ID, assertedWorkspaceId: WORKSPACE_ID, includeDeleted: false }, request: expect.anything(), }) }) + it('leaves an archived file unreachable when scope is omitted', async () => { + mocks.readMetadata.mockImplementation(archivedFileUseCase) + + const response = await callGet(`workspaceId=${WORKSPACE_ID}`) + + expect(response.status).toBe(404) + expect((await response.json()).error.code).toBe('NOT_FOUND') + expect(mocks.readMetadata).toHaveBeenCalledWith( + expect.objectContaining({ + input: { fileId: FILE_ID, assertedWorkspaceId: WORKSPACE_ID, includeDeleted: false }, + }) + ) + }) + + it('leaves an archived file unreachable under an explicit scope=active', async () => { + mocks.readMetadata.mockImplementation(archivedFileUseCase) + + const response = await callGet(`workspaceId=${WORKSPACE_ID}&scope=active`) + + expect(response.status).toBe(404) + expect((await response.json()).error.code).toBe('NOT_FOUND') + expect(mocks.readMetadata).toHaveBeenCalledWith( + expect.objectContaining({ + input: { fileId: FILE_ID, assertedWorkspaceId: WORKSPACE_ID, includeDeleted: false }, + }) + ) + }) + + it('returns archived metadata when scope=archived opts into the archived set', async () => { + mocks.readMetadata.mockImplementation(archivedFileUseCase) + + const response = await callGet(`workspaceId=${WORKSPACE_ID}&scope=archived`) + + expect(response.status).toBe(200) + expect(await response.json()).toEqual({ + data: { + id: FILE_ID, + name: 'data.csv', + size: 1024, + type: 'text/csv', + key: 'workspace/ws/1-x-data.csv', + folderPath: '/', + uploadedByEmail: 'ada@example.com', + uploadedAt: '2024-01-01T00:00:00.000Z', + updatedAt: '2024-01-02T00:00:00.000Z', + deletedAt: '2024-01-03T00:00:00.000Z', + share: SHARE, + }, + }) + expect(mocks.readMetadata).toHaveBeenCalledWith( + expect.objectContaining({ + principal: auth.principal, + input: { fileId: FILE_ID, assertedWorkspaceId: WORKSPACE_ID, includeDeleted: true }, + }) + ) + }) + + it('still conceals an unauthorized archived read behind the same 404', async () => { + mocks.readMetadata.mockRejectedValue(new NoWorkspaceAccessError()) + + const response = await callGet(`workspaceId=${WORKSPACE_ID}&scope=archived`) + + expect(response.status).toBe(404) + expect((await response.json()).error.code).toBe('NOT_FOUND') + /** + * `includeDeleted: true` is what makes this the *archived* read being + * concealed rather than the plain cross-workspace 404 the suite already + * pins: without it the request never reaches the archived set and the test + * proves only `NoWorkspaceAccessError → 404`. + */ + expect(mocks.readMetadata).toHaveBeenCalledWith( + expect.objectContaining({ + principal: auth.principal, + input: { fileId: FILE_ID, assertedWorkspaceId: WORKSPACE_ID, includeDeleted: true }, + }) + ) + }) + + it('rejects an unrecognized scope before reaching the use case', async () => { + const response = await callGet(`workspaceId=${WORKSPACE_ID}&scope=all`) + + expect(response.status).toBe(400) + expect(mocks.readMetadata).not.toHaveBeenCalled() + }) + it('returns a null share when the file has no share configuration', async () => { mocks.readMetadata.mockResolvedValueOnce({ file: buildRecord(), share: null }) diff --git a/apps/sim/app/api/v2/files/[fileId]/metadata/route.ts b/apps/sim/app/api/v2/files/[fileId]/metadata/route.ts index 68d1420c541..722d125e4da 100644 --- a/apps/sim/app/api/v2/files/[fileId]/metadata/route.ts +++ b/apps/sim/app/api/v2/files/[fileId]/metadata/route.ts @@ -8,7 +8,15 @@ import { toV2File } from '@/app/api/v2/files/utils' export const dynamic = 'force-dynamic' export const revalidate = 0 -/** GET /api/v2/files/[fileId]/metadata — Return file metadata without downloading its bytes. */ +/** + * GET /api/v2/files/[fileId]/metadata — Return file metadata without downloading its bytes. + * + * `scope` mirrors the list endpoint's lifecycle selector: it defaults to `active`, and only an + * explicit `scope=archived` relaxes the soft-delete predicate on the row lookup so a caller can + * inspect an archived file before restoring it. Authorization is unaffected — the use case still + * resolves the canonical workspace context for the file and authorizes `files.read_metadata` + * against it either way. + */ export const GET = defineV2JsonRoute({ contract: v2GetFileContract, auth: v2ApiKeyAuth, @@ -18,6 +26,7 @@ export const GET = defineV2JsonRoute({ mapInput: ({ params, query }) => ({ fileId: params.fileId, assertedWorkspaceId: query.workspaceId, + includeDeleted: query.scope === 'archived', }), useCase: readWorkspaceFileMetadata, present: async ({ file, share }) => ({ data: { ...(await toV2File(file)), share } }), diff --git a/apps/sim/app/api/v2/lib/response.test.ts b/apps/sim/app/api/v2/lib/response.test.ts index f84dd84a203..b45aa898340 100644 --- a/apps/sim/app/api/v2/lib/response.test.ts +++ b/apps/sim/app/api/v2/lib/response.test.ts @@ -2,7 +2,8 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' -import { v2Error } from '@/app/api/v2/lib/response' +import { HttpError } from '@/lib/core/utils/http-error' +import { v2Error, v2HttpError, v2RateLimitError } from '@/app/api/v2/lib/response' describe('v2Error retry guidance', () => { it('sends Retry-After on 503 so a client does not retry a degraded dependency immediately', () => { @@ -46,3 +47,90 @@ describe('v2Error retry guidance', () => { expect(response.headers.get('Retry-After')).toBeNull() }) }) + +/** + * RFC 9110 §11.6.1 makes `WWW-Authenticate` a MUST on 401, and `v2Error` is the + * one funnel every v2 401 passes through — the missing key, the invalid key, an + * `unauthorized` orchestration failure, and the v1 middleware's auth result all + * render here. + */ +describe('v2 401 authentication challenge', () => { + /** + * Pinned exactly at the primary funnel rather than asserted as merely present. + * `toBeTruthy` accepts any string, so the scheme token, the realm, and the + * `header=` parameter that names the only channel v2 reads could all change + * without a test noticing. The reachability tests below stay loose on purpose + * — they pin that the header arrives down each path, not its value twice. + */ + const EXPECTED_CHALLENGE = 'SimApiKey realm="Sim API", header="x-api-key"' + + const challenge = () => + v2Error('UNAUTHORIZED', 'API key required').headers.get('WWW-Authenticate') + + it('sends a challenge on 401', () => { + const response = v2Error('UNAUTHORIZED', 'Invalid API key') + + expect(response.status).toBe(401) + expect(response.headers.get('WWW-Authenticate')).toBe(EXPECTED_CHALLENGE) + }) + + it('names the x-api-key header, the only channel v2 actually reads', () => { + expect(challenge()).toContain('x-api-key') + }) + + it('does not advertise a scheme v2 does not accept', () => { + const value = challenge() ?? '' + const scheme = value.split(' ')[0].toLowerCase() + + expect(scheme).not.toBe('bearer') + expect(scheme).not.toBe('basic') + expect(scheme).not.toBe('digest') + }) + + it('challenges on a 401 reached through the rate-limit auth result', () => { + const response = v2RateLimitError({ + allowed: false, + remaining: 0, + resetAt: new Date(), + limit: 0, + error: 'Invalid API key', + }) + + expect(response.status).toBe(401) + expect(response.headers.get('WWW-Authenticate')).toBeTruthy() + }) + + it('challenges on a 401 reached through a typed HTTP error', () => { + class UnauthorizedError extends HttpError { + readonly statusCode = 401 + } + + const response = v2HttpError(new UnauthorizedError('Invalid API key')) + + expect(response.status).toBe(401) + expect(response.headers.get('WWW-Authenticate')).toBeTruthy() + }) + + it('does not challenge on statuses that are not 401', () => { + for (const code of ['BAD_REQUEST', 'FORBIDDEN', 'NOT_FOUND', 'RATE_LIMITED'] as const) { + expect(v2Error(code, 'nope').headers.get('WWW-Authenticate')).toBeNull() + } + }) + + /** + * `options.headers` is spread *after* the challenge, so a caller-supplied + * `WWW-Authenticate` replaces the default rather than being ignored. That + * precedence is the whole reason the default is safe to install + * unconditionally on 401 — a route with a genuinely different challenge is + * not fighting it — but it also means an accidental override silently wins. + * Documented here so a reordering of the spread is a test failure either way. + */ + it('lets a caller-supplied challenge override the default', () => { + const response = v2Error('UNAUTHORIZED', 'Invalid token', { + headers: { 'WWW-Authenticate': 'Bearer realm="mcp"' }, + }) + + expect(response.status).toBe(401) + expect(response.headers.get('WWW-Authenticate')).toBe('Bearer realm="mcp"') + }) +}) diff --git a/apps/sim/app/api/v2/lib/response.ts b/apps/sim/app/api/v2/lib/response.ts index 8f5f61215b5..c96763ac4e9 100644 --- a/apps/sim/app/api/v2/lib/response.ts +++ b/apps/sim/app/api/v2/lib/response.ts @@ -100,6 +100,28 @@ const RETRY_AFTER_SECONDS_BY_STATUS: Partial> = { 503: ADMISSION_RETRY_AFTER_SECONDS, } +/** + * The challenge every v2 `401` carries, so a 401 is a complete one. + * + * RFC 9110 §11.6.1 makes `WWW-Authenticate` a MUST on 401 — a 401 without it is + * a refusal that never says what would have been accepted, and a generic HTTP + * client has nothing to react to. + * + * The scheme name is deliberately Sim-specific rather than a registered one. + * v2 authenticates from the `x-api-key` header and accepts no `Authorization` + * scheme at all — `Authorization: Bearer ` is not a channel here — so + * `Bearer` and `Basic` would both be false advertising. `Basic` is worse than + * false: a browser reacts to it by opening a native credential prompt that + * cannot produce an API key. An unregistered scheme is what remains, and it is + * legal: §11.6.1's grammar requires *an* `auth-scheme` token, not a registered + * one. Every challenge implies "retry via `Authorization: …`" by + * construction, so the token is chosen to be one no client has a built-in + * handler for — the challenge surfaces to a human instead of triggering an + * automatic retry down a channel v2 does not read — and the real channel is + * named outright in the `header` parameter beside it. + */ +const V2_AUTH_CHALLENGE = 'SimApiKey realm="Sim API", header="x-api-key"' + type RateLimitHeaderSource = Pick function rateLimitHeaders(rateLimit?: RateLimitHeaderSource): Record { @@ -183,6 +205,7 @@ export function v2Error( status, headers: { ...PRIVATE_NO_STORE, + ...(status === 401 ? { 'WWW-Authenticate': V2_AUTH_CHALLENGE } : {}), ...(retryAfterSeconds === undefined ? {} : { 'Retry-After': retryAfterSeconds.toString() }), ...options.headers, }, diff --git a/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/cancel/route.test.ts b/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/cancel/route.test.ts new file mode 100644 index 00000000000..12f0b1fbea5 --- /dev/null +++ b/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/cancel/route.test.ts @@ -0,0 +1,121 @@ +/** + * @vitest-environment node + */ + +import { + V2_OPERATION_RATE_LIMIT_ALLOWED, + V2_PREAUTH_RATE_LIMIT_ALLOWED, + v2ApiKeyAuthModuleMock, + v2GateModuleMock, + v2RateLimiterModuleMock, + v2RouteMocks, +} from '@sim/testing' +import { NextRequest } from 'next/server' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ + cancel: vi.fn(), + capture: vi.fn(), +})) + +vi.mock('@/lib/api/server/routes/v2-api-key-auth', () => v2ApiKeyAuthModuleMock) +vi.mock('@/lib/core/rate-limiter', () => v2RateLimiterModuleMock) +vi.mock('@/app/api/v2/lib/gate', () => v2GateModuleMock) +vi.mock('@/lib/posthog/server', () => ({ captureServerEvent: mocks.capture })) +vi.mock('@/lib/workflows/application/cancel-run', () => ({ + cancelWorkflowRun: { operation: { id: 'workflows.runs.cancel' }, execute: mocks.cancel }, +})) + +import { POST } from '@/app/api/v2/workflows/[id]/runs/[runId]/cancel/route' + +const WORKSPACE_ID = 'workspace-1' +const WORKFLOW_ID = 'workflow-1' +const RUN_ID = 'run-1' + +const principal = { + kind: 'personal_api_key' as const, + userId: 'user-1', + keyId: 'key-1', +} +const auth = { + principal, + rolloutUserId: 'user-1', + rateLimitSubjectIds: ['api-key:key-1'], + rateLimitSubscription: null, + keyType: 'personal' as const, +} + +const context = { params: Promise.resolve({ id: WORKFLOW_ID, runId: RUN_ID }) } + +function request() { + return new NextRequest( + `http://localhost:3000/api/v2/workflows/${WORKFLOW_ID}/runs/${RUN_ID}/cancel`, + { method: 'POST', headers: { 'x-api-key': 'secret' } } + ) +} + +/** The service result the use case hands back, minus the outcome under test. */ +function serviceResult(overrides: Record) { + return { + executionId: RUN_ID, + redisAvailable: true, + locallyAborted: false, + pausedCancelled: false, + workflowId: WORKFLOW_ID, + workspaceId: WORKSPACE_ID, + ...overrides, + } +} + +describe('POST /api/v2/workflows/[id]/runs/[runId]/cancel', () => { + beforeEach(() => { + vi.clearAllMocks() + v2RouteMocks.authenticate.mockResolvedValue(auth) + v2RouteMocks.gate.mockResolvedValue(null) + v2RouteMocks.preauthRate.mockResolvedValue(V2_PREAUTH_RATE_LIMIT_ALLOWED) + v2RouteMocks.operationRate.mockResolvedValue(V2_OPERATION_RATE_LIMIT_ALLOWED) + }) + + it('reports a durable write when an active run is cancelled', async () => { + mocks.cancel.mockResolvedValue( + serviceResult({ success: true, durablyRecorded: true, reason: 'recorded' }) + ) + + const response = await POST(request(), context) + + expect(response.status).toBe(200) + expect((await response.json()).data).toEqual({ + success: true, + runId: RUN_ID, + redisAvailable: true, + durablyRecorded: true, + locallyAborted: false, + pausedCancelled: false, + reason: 'recorded', + }) + }) + + /** + * The published outcome of a cancel against a run that had already finished. + * `durablyRecorded: true` here is the defect this suite pins: nothing was + * written, so a caller reconciling on that flag would trust a write that never + * happened. + */ + it.each([ + ['cancelled', 'already_cancelled'], + ['completed', 'already_completed'], + ['failed', 'already_failed'], + ])('reports a terminal %s run as a no-op the caller can tell apart', async (_status, reason) => { + mocks.cancel.mockResolvedValue(serviceResult({ success: true, durablyRecorded: false, reason })) + + const response = await POST(request(), context) + + expect(response.status).toBe(200) + expect((await response.json()).data).toMatchObject({ + success: true, + runId: RUN_ID, + durablyRecorded: false, + reason, + }) + }) +}) diff --git a/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/cancel/route.ts b/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/cancel/route.ts index 5418355124d..c492fe46957 100644 --- a/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/cancel/route.ts +++ b/apps/sim/app/api/v2/workflows/[id]/runs/[runId]/cancel/route.ts @@ -27,8 +27,16 @@ export const POST = defineV2JsonRoute({ reason: result.reason, }, }), + /** + * Reports a cancellation, so it needs the run to have actually been + * cancelled. `success` alone no longer implies that: a cancel against an + * already-terminal run satisfies the request without writing anything, and + * reports `success: true` with `durablyRecorded: false`. Requiring both also + * keeps the event off a cancellation that reached the row but failed its + * paused reconciliation, which reports the inverse pair. + */ onSuccess: ({ principal, result }) => { - if (!result.success || principal.kind !== 'personal_api_key') return + if (!result.success || !result.durablyRecorded || principal.kind !== 'personal_api_key') return captureServerEvent( principal.userId, 'workflow_execution_cancelled', diff --git a/apps/sim/app/api/workflows/[id]/executions/[executionId]/cancel/route.test.ts b/apps/sim/app/api/workflows/[id]/executions/[executionId]/cancel/route.test.ts index 84ba7ec5d3a..21c2fa13e9b 100644 --- a/apps/sim/app/api/workflows/[id]/executions/[executionId]/cancel/route.test.ts +++ b/apps/sim/app/api/workflows/[id]/executions/[executionId]/cancel/route.test.ts @@ -1010,6 +1010,7 @@ describe('POST /api/workflows/[id]/executions/[executionId]/cancel', () => { expect(response.status).toBe(200) await expect(response.json()).resolves.toMatchObject({ success: true, + durablyRecorded: false, reason: 'already_cancelled', }) expect(mockCancelByExecution).not.toHaveBeenCalled() diff --git a/apps/sim/lib/api/contracts/primitives.test.ts b/apps/sim/lib/api/contracts/primitives.test.ts index 4e8a605a98f..b9a2d791976 100644 --- a/apps/sim/lib/api/contracts/primitives.test.ts +++ b/apps/sim/lib/api/contracts/primitives.test.ts @@ -2,14 +2,17 @@ * @vitest-environment node */ import { describe, expect, it } from 'vitest' +import { z } from 'zod' import { customPatternSchema, isCanonicalBase64, + MAX_ID_LENGTH, organizationIdSchema, piiStagePolicySchema, piiStagesSchema, privateSecretProvenanceBundleSchema, resolvedSecretTraceProvenanceSchema, + withMissingFieldMessage, workflowIdSchema, workspaceFileIdSchema, workspaceFileNameSchema, @@ -257,4 +260,77 @@ describe('shared id schemas name the field when it is missing', () => { expect(result.error?.issues[0]?.message).not.toContain('received undefined') }) + + /** + * A schema-level `error` string replaces the message for *every* issue, so the + * required-field wording used to answer a wrong-typed value too: `{"name": 123}` + * came back "Name is required" when a name had in fact been supplied. Missing and + * wrong-typed are different mistakes and must read differently. + */ + for (const [name, schema, message] of cases) { + it(`${name}: a wrong-typed value reports the type, not "${message}"`, () => { + const result = schema.safeParse(123) + + expect(result.success).toBe(false) + expect(result.error?.issues[0]?.message).toBe( + 'Invalid input: expected string, received number' + ) + }) + } + + it('workspaceFileNameSchema separates a missing name from a wrong-typed one', () => { + expect(workspaceFileNameSchema.safeParse(undefined).error?.issues[0]?.message).toBe( + 'Name is required' + ) + expect(workspaceFileNameSchema.safeParse(123).error?.issues[0]?.message).toBe( + 'Invalid input: expected string, received number' + ) + }) +}) + +/** + * An unbounded `workspaceId` reached the workspace lookup at whatever length the + * caller chose. No workspace id this repo mints approaches the bound, so it only + * rejects values that could never have resolved. + */ +describe('workspaceIdSchema length bound', () => { + it('accepts an id at the bound', () => { + expect(workspaceIdSchema.safeParse('a'.repeat(MAX_ID_LENGTH)).success).toBe(true) + }) + + it('rejects an id one character past the bound, naming the field', () => { + const result = workspaceIdSchema.safeParse('a'.repeat(MAX_ID_LENGTH + 1)) + + expect(result.success).toBe(false) + expect(result.error?.issues[0]?.message).toBe('Workspace ID is too long') + }) + + it('still accepts a UUID workspace id', () => { + expect(workspaceIdSchema.safeParse('7a6cce2b-78b8-40bc-b8d3-0a2a6dfd9023').success).toBe(true) + }) +}) + +describe('withMissingFieldMessage', () => { + const base = z.string().min(1, 'Description is required').max(8, 'Description is too long') + const retrofitted = withMissingFieldMessage(base, 'Description is required') + + it('names the field when the value is omitted', () => { + expect(retrofitted.safeParse(undefined).error?.issues[0]?.message).toBe( + 'Description is required' + ) + }) + + it('keeps Zod default wording for a wrong-typed value', () => { + expect(retrofitted.safeParse(5).error?.issues[0]?.message).toBe( + 'Invalid input: expected string, received number' + ) + }) + + it('preserves the checks the source schema carried', () => { + expect(retrofitted.safeParse('').error?.issues[0]?.message).toBe('Description is required') + expect(retrofitted.safeParse('a'.repeat(9)).error?.issues[0]?.message).toBe( + 'Description is too long' + ) + expect(retrofitted.safeParse('ok').success).toBe(true) + }) }) diff --git a/apps/sim/lib/api/contracts/primitives.ts b/apps/sim/lib/api/contracts/primitives.ts index 39b7d6492a8..90306e53060 100644 --- a/apps/sim/lib/api/contracts/primitives.ts +++ b/apps/sim/lib/api/contracts/primitives.ts @@ -192,34 +192,74 @@ export const jobIdParamsSchema = z.object({ */ export const nonEmptyIdSchema = z.string().min(1) +/** + * Schema-level error customizer that applies a message **only when the value is + * absent**, and defers to Zod's default wording for everything else. + * + * A plain `z.string({ error: message })` replaces the message for *every* issue + * the schema raises, including `invalid_type`. A caller who sent `{"name": 123}` + * then reads `Name is required` — a name was supplied, it was the wrong type, and + * the message sends them looking for the wrong bug. Returning `undefined` for a + * present-but-wrong-typed value lets Zod render `Invalid input: expected string, + * received number` instead. + */ +export function missingFieldError(message: string) { + return (issue: z.core.$ZodRawIssue): string | undefined => + issue.input === undefined ? message : undefined +} + +/** + * Re-issues an existing string schema with a missing-value message, keeping every + * check (bounds, regex, trim) it already carries. + * + * Use this when the field's bounds are owned by a shared schema elsewhere and only + * the omitted-field wording needs to be added at this boundary — re-declaring the + * bounds locally would let the two copies drift. + */ +export function withMissingFieldMessage( + schema: TSchema, + message: string +): TSchema { + return schema.clone({ ...schema._zod.def, error: missingFieldError(message) }) +} + +/** + * Bound shared by the id primitives below. Every identifier this repo mints — + * UUID v4, `wf_`, and the legacy free-form `text` keys — is far shorter, + * so the bound rejects only values that were never going to resolve while keeping + * an unbounded string from reaching a lookup. + */ +export const MAX_ID_LENGTH = 128 + /** * Builds a required, non-empty string schema whose message covers **both** * failure modes. * * `.min(1, message)` alone only fires for a present-but-empty string; an omitted * field falls through to Zod's default `Invalid input: expected string, received - * undefined`, which never names the field the caller left out. Passing the same - * message to the `z.string({ error })` constructor closes that gap. + * undefined`, which never names the field the caller left out. + * {@link missingFieldError} closes that gap without also swallowing the + * wrong-type message. * * Prefer this over a bare `z.string().min(1, '...')` for any required request * field. When a named primitive below already carries the right wording, import * that instead of rebuilding it here. */ export function requiredFieldSchema(message: string) { - return z.string({ error: message }).min(1, message) + return z.string({ error: missingFieldError(message) }).min(1, message) } /** Non-empty `workspaceId` field with a stable, human-readable message. */ -export const workspaceIdSchema = requiredFieldSchema('Workspace ID is required').describe( - 'Unique workspace identifier.' -) +export const workspaceIdSchema = requiredFieldSchema('Workspace ID is required') + .max(MAX_ID_LENGTH, 'Workspace ID is too long') + .describe('Unique workspace identifier.') /** * A single workspace-file name, not a path. Folder placement is carried by a * separate folder id or path field, so separators and dot segments are invalid. */ export const workspaceFileNameSchema = z - .string({ error: 'Name is required' }) + .string({ error: missingFieldError('Name is required') }) .trim() .min(1, 'Name is required') .max(255, 'Name is too long') @@ -257,7 +297,7 @@ export const runIdSchema = z * two-state and three-state spellings stay explicit at each call site. */ export const folderIdSchema = requiredFieldSchema('Folder ID is required').max( - 128, + MAX_ID_LENGTH, 'Folder ID is too long' ) @@ -269,7 +309,7 @@ export const folderIdSchema = requiredFieldSchema('Folder ID is required').max( * UUID-only schema — a `.uuid()` constraint here silently 400s every `wf_` file. */ export const workspaceFileIdSchema = requiredFieldSchema('File ID is required') - .max(128, 'File ID is too long') + .max(MAX_ID_LENGTH, 'File ID is too long') .regex(/^[A-Za-z0-9_-]+$/, 'Invalid file id') /** diff --git a/apps/sim/lib/api/contracts/v2/files.ts b/apps/sim/lib/api/contracts/v2/files.ts index 30fdfcc42cc..b422d7792c0 100644 --- a/apps/sim/lib/api/contracts/v2/files.ts +++ b/apps/sim/lib/api/contracts/v2/files.ts @@ -334,6 +334,29 @@ export const v2FileWorkspaceQuerySchema = z export type V2FileWorkspaceQuery = z.output +/** + * Metadata read: the workspace scope plus the same `scope` lifecycle selector the + * list endpoint uses, so a caller that found a file under `GET /files?scope=archived` + * can read it back with the identical spelling. + * + * The default stays `active`, which keeps the read on the live set and continues to + * answer `404` for a soft-deleted file. `scope` only relaxes the `deleted_at` predicate + * on the row lookup — the workspace the file belongs to, the asserted-workspace check, + * and the operation's authorization are unchanged, so it cannot widen who may read. + */ +export const v2GetFileMetadataQuerySchema = z + .object({ + workspaceId: workspaceIdSchema.describe('Workspace that owns the file.'), + scope: v2FileScopeSchema + .default('active') + .describe( + '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.' + ), + }) + .strict() + +export type V2GetFileMetadataQuery = z.output + export const v2RenameFileBodySchema = z .object({ workspaceId: workspaceIdSchema.describe('Workspace that owns the file.'), @@ -610,7 +633,7 @@ export const v2GetFileContract = defineRouteContract({ method: 'GET', path: '/api/v2/files/[fileId]/metadata', params: v2FileParamsSchema, - query: v2FileWorkspaceQuerySchema, + query: v2GetFileMetadataQuerySchema, response: { mode: 'json', schema: v2DataResponse(v2FileMetadataSchema), diff --git a/apps/sim/lib/api/contracts/v2/knowledge.ts b/apps/sim/lib/api/contracts/v2/knowledge.ts index 52cdf284756..b30e5900a1a 100644 --- a/apps/sim/lib/api/contracts/v2/knowledge.ts +++ b/apps/sim/lib/api/contracts/v2/knowledge.ts @@ -836,8 +836,16 @@ export const MAX_V2_KNOWLEDGE_DOCUMENT_TAG_FILTERS = 10 */ export const MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH = 8192 * 4 -export const v2KnowledgeSearchBodySchema = v1KnowledgeSearchBodySchema - .safeExtend({ +/** + * Rebuilt from the v1 shape rather than extended from the v1 schema: v1 carries + * the "query or tagFilters" rule as a bare `.refine`, which reports at path `[]`, + * so no client could attach the failure to a field. The rule is restated below as + * a `superRefine` with a `path` — extending v1 would inherit the pathless issue + * alongside it and report the same violation twice. + */ +export const v2KnowledgeSearchBodySchema = z + .object({ + ...v1KnowledgeSearchBodySchema.shape, workspaceId: v1KnowledgeSearchBodySchema.shape.workspaceId.describe( 'Workspace that owns the knowledge bases.' ), @@ -855,9 +863,14 @@ export const v2KnowledgeSearchBodySchema = v1KnowledgeSearchBodySchema `Natural-language query; required when tag filters are omitted. At most ${MAX_V2_KNOWLEDGE_SEARCH_QUERY_LENGTH} characters — longer text exceeds the embedding model's per-input token ceiling and would be truncated before the billed search ran.` ) .meta({ examples: ['How do I reset my password?'] }), - topK: v1KnowledgeSearchBodySchema.shape.topK.describe( - 'Maximum number of search results to return. Must be a whole number between 1 and 100; the boundary schema only bounds the range, so a fractional value is admitted here and then rejected with 400 during search.' - ), + topK: z + .number() + .min(1, 'topK must be at least 1') + .max(100, 'topK cannot exceed 100') + .default(10) + .describe( + 'Maximum number of search results to return. Must be a whole number between 1 and 100; the boundary schema only bounds the range, so a fractional value is admitted here and then rejected with 400 during search.' + ), tagFilters: z .array(v2KnowledgeSearchTagFilterSchema) .max( @@ -909,6 +922,22 @@ export const v2KnowledgeSearchBodySchema = v1KnowledgeSearchBodySchema * parameters never arrived. */ .strict() + /** + * A search with neither a query nor a tag filter has nothing to retrieve on. + * Reported on `query`, the field a caller who sent neither is most likely to be + * missing, so the failure lands on an input instead of on the request as a whole. + */ + .superRefine((body, ctx) => { + const hasQuery = Boolean(body.query && body.query.trim().length > 0) + const hasTagFilters = Boolean(body.tagFilters && body.tagFilters.length > 0) + if (!hasQuery && !hasTagFilters) { + ctx.addIssue({ + code: 'custom', + path: ['query'], + message: 'Either query or tagFilters must be provided', + }) + } + }) export type V2KnowledgeSearchBody = z.input export const v2SearchKnowledgeContract = defineRouteContract({ diff --git a/apps/sim/lib/api/contracts/v2/required-field-messages.test.ts b/apps/sim/lib/api/contracts/v2/required-field-messages.test.ts new file mode 100644 index 00000000000..e72f4e2a57d --- /dev/null +++ b/apps/sim/lib/api/contracts/v2/required-field-messages.test.ts @@ -0,0 +1,175 @@ +/** + * @vitest-environment node + */ +import { describe, expect, it } from 'vitest' +import { v2KnowledgeSearchBodySchema } from '@/lib/api/contracts/v2/knowledge' +import { v2CreateSkillBodySchema } from '@/lib/api/contracts/v2/skills' +import { v2CreateWorkflowBodySchema } from '@/lib/api/contracts/v2/workflows' + +const workspaceId = '7a6cce2b-78b8-40bc-b8d3-0a2a6dfd9023' +const knowledgeBaseIds = ['ae814592-a730-4ad6-8741-b51e48843300'] + +function messageAt( + result: { success: boolean; error?: { issues: { path: PropertyKey[]; message: string }[] } }, + field: string +) { + return result.error?.issues.find((issue) => issue.path[0] === field)?.message +} + +/** + * A required field that is *omitted* and one that is *wrong-typed* are different + * mistakes. Both used to answer with wording that pointed at the other: the + * create bodies leaked Zod's default "expected string, received undefined" for an + * omitted field, and the shared string primitives answered "… is required" for a + * value that had in fact been supplied. + */ +describe('v2 create bodies name a missing required field', () => { + it('POST /v2/workflows names an omitted name and types a wrong-typed one', () => { + const missing = v2CreateWorkflowBodySchema.safeParse({ workspaceId }) + expect(messageAt(missing, 'name')).toBe('name is required') + + const wrongType = v2CreateWorkflowBodySchema.safeParse({ workspaceId, name: 123 }) + expect(messageAt(wrongType, 'name')).toBe('Invalid input: expected string, received number') + + const empty = v2CreateWorkflowBodySchema.safeParse({ workspaceId, name: ' ' }) + expect(messageAt(empty, 'name')).toBe('name is required') + }) + + it('POST /v2/workflows still names an omitted workspaceId', () => { + const result = v2CreateWorkflowBodySchema.safeParse({ name: 'Triage' }) + expect(messageAt(result, 'workspaceId')).toBe('Workspace ID is required') + }) + + it('POST /v2/skills names each omitted required field', () => { + const result = v2CreateSkillBodySchema.safeParse({ workspaceId }) + + expect(messageAt(result, 'name')).toBe('Skill name is required') + expect(messageAt(result, 'description')).toBe('Description is required') + expect(messageAt(result, 'content')).toBe('Content is required') + }) + + it('POST /v2/skills types a wrong-typed field instead of calling it missing', () => { + const result = v2CreateSkillBodySchema.safeParse({ + workspaceId, + name: 'my-skill', + description: 123, + content: 'Body', + }) + + expect(messageAt(result, 'description')).toBe('Invalid input: expected string, received number') + }) + + it('POST /v2/skills keeps the bounds the shared field schemas carry', () => { + const badName = v2CreateSkillBodySchema.safeParse({ + workspaceId, + name: 'Not Kebab', + description: 'A summary', + content: 'Body', + }) + expect(messageAt(badName, 'name')).toBe('Name must be kebab-case (e.g. my-skill)') + + const longContent = v2CreateSkillBodySchema.safeParse({ + workspaceId, + name: 'my-skill', + description: 'A summary', + content: 'a'.repeat(50_001), + }) + expect(messageAt(longContent, 'content')).toBe('Content is too large') + + expect( + v2CreateSkillBodySchema.safeParse({ + workspaceId, + name: 'my-skill', + description: 'A summary', + content: 'Body', + }).success + ).toBe(true) + }) +}) + +/** + * `topK` is the one search field whose range violations answered with Zod's + * default phrasing while every sibling — `limit`, `rerankerInputCount`, + * `tagFilters`, `query` — named itself. + */ +describe('v2 knowledge search topK messages name the field', () => { + it('names topK when it exceeds the maximum', () => { + const result = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + query: 'hello', + topK: 101, + }) + + expect(messageAt(result, 'topK')).toBe('topK cannot exceed 100') + }) + + it('names topK when it is below the minimum', () => { + const result = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + query: 'hello', + topK: 0, + }) + + expect(messageAt(result, 'topK')).toBe('topK must be at least 1') + }) + + it('keeps admitting a fractional topK for the use case to reject, and defaults to 10', () => { + const fractional = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + query: 'hello', + topK: 2.5, + }) + expect(fractional.success).toBe(true) + + const defaulted = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + query: 'hello', + }) + expect(defaulted.success && defaulted.data.topK).toBe(10) + }) +}) + +/** + * The "query or tagFilters" rule arrived from v1 as a bare `.refine`, which + * reports at path `[]` — a message no client could attach to a field. + */ +describe('v2 knowledge search reports the missing-input rule on a field', () => { + it('attaches the failure to query', () => { + const result = v2KnowledgeSearchBodySchema.safeParse({ workspaceId, knowledgeBaseIds }) + + expect(result.success).toBe(false) + expect(result.error?.issues).toHaveLength(1) + expect(result.error?.issues[0]?.path).toEqual(['query']) + expect(result.error?.issues[0]?.message).toBe('Either query or tagFilters must be provided') + }) + + it('still accepts a tag-only search and a query-only search', () => { + expect( + v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + tagFilters: [{ tagName: 'From', operator: 'contains', value: 'brex' }], + }).success + ).toBe(true) + + expect( + v2KnowledgeSearchBodySchema.safeParse({ workspaceId, knowledgeBaseIds, query: 'hello' }) + .success + ).toBe(true) + }) + + it('rejects a whitespace-only query with no tag filters, as v1 did', () => { + const result = v2KnowledgeSearchBodySchema.safeParse({ + workspaceId, + knowledgeBaseIds, + query: ' ', + }) + + expect(result.success).toBe(false) + expect(messageAt(result, 'query')).toBe('Either query or tagFilters must be provided') + }) +}) diff --git a/apps/sim/lib/api/contracts/v2/skills.ts b/apps/sim/lib/api/contracts/v2/skills.ts index fe2a8cd026d..03dd5a0f230 100644 --- a/apps/sim/lib/api/contracts/v2/skills.ts +++ b/apps/sim/lib/api/contracts/v2/skills.ts @@ -1,5 +1,10 @@ import { z } from 'zod' -import { noInputSchema, nonEmptyIdSchema, workspaceIdSchema } from '@/lib/api/contracts/primitives' +import { + noInputSchema, + nonEmptyIdSchema, + withMissingFieldMessage, + workspaceIdSchema, +} from '@/lib/api/contracts/primitives' import { skillContentSchema, skillDescriptionSchema, @@ -113,14 +118,23 @@ export const v2ListSkillsQuerySchema = v2SkillWorkspaceQuerySchema export type V2ListSkillsQuery = z.output +/** + * Create body. Every field is required, so each one carries the missing-value + * wording the shared field primitives cannot: those are also spelled `.optional()` + * on the update body, where an omitted field is legal, so the message belongs + * here rather than on the shared schema. + */ export const v2CreateSkillBodySchema = z .object({ workspaceId: workspaceIdSchema.describe('Workspace in which to create the skill.'), - name: skillNameSchema.describe( + name: withMissingFieldMessage(skillNameSchema, 'Skill name is required').describe( 'Kebab-case name, unique within the workspace and not reserved by a built-in skill.' ), - description: skillDescriptionSchema.describe('One-line summary of when the skill applies.'), - content: skillContentSchema.describe( + description: withMissingFieldMessage( + skillDescriptionSchema, + 'Description is required' + ).describe('One-line summary of when the skill applies.'), + content: withMissingFieldMessage(skillContentSchema, 'Content is required').describe( 'Skill body containing the instructions given to the agent.' ), }) diff --git a/apps/sim/lib/api/contracts/v2/workflows.ts b/apps/sim/lib/api/contracts/v2/workflows.ts index 8d0aa30a245..4432437b508 100644 --- a/apps/sim/lib/api/contracts/v2/workflows.ts +++ b/apps/sim/lib/api/contracts/v2/workflows.ts @@ -9,6 +9,7 @@ import { } from '@/lib/api/contracts/deployments' import { booleanQueryFlagSchema, + missingFieldError, noInputSchema, runIdSchema, workspaceIdSchema, @@ -398,7 +399,7 @@ export const v2CreateWorkflowBodySchema = z .object({ workspaceId: workspaceIdSchema.describe('Workspace in which to create the workflow.'), name: z - .string() + .string({ error: missingFieldError('name is required') }) .trim() .min(1, 'name is required') .max(255, 'name is too long') @@ -1348,25 +1349,29 @@ export const v2CancelWorkflowRunDataSchema = z redisAvailable: z .boolean() .describe('Whether the distributed cancellation channel was available.'), - durablyRecorded: z.boolean().describe('Whether cancellation was recorded durably.'), + durablyRecorded: z + .boolean() + .describe( + 'Whether this request durably recorded a cancellation. Always false for a run that was already terminal, where the request is satisfied but nothing was written.' + ), locallyAborted: z.boolean().describe('Whether an in-process execution was aborted.'), pausedCancelled: z.boolean().describe('Whether a paused execution was cancelled.'), /** * Always emitted by the cancellation service — it is not a partial-failure - * marker. `recorded` is the full-success value; the other four name the step - * that degraded. + * marker. `recorded` is the full-success value; the `already_*` values name + * a terminal no-op; the rest name the step that degraded. */ reason: cancelWorkflowExecutionReasonSchema .optional() .describe( - 'Machine-readable cancellation outcome, present on every cancellation including full successes. `recorded` is the success value. `redis_unavailable` and `redis_write_failed` mean the distributed cancellation signal was not written, so an already-running execution may not observe the cancellation. `paused_event_publish_failed` and `paused_database_cancel_failed` name the failing step for a paused run.' + 'Machine-readable cancellation outcome, present on every cancellation including full successes. `recorded` is the success value. `already_cancelled`, `already_completed`, and `already_failed` mean the run had already reached that terminal state, so nothing was cancelled and `durablyRecorded` is false. `redis_unavailable` and `redis_write_failed` mean the distributed cancellation signal was not written, so an already-running execution may not observe the cancellation. `paused_event_publish_failed` and `paused_database_cancel_failed` name the failing step for a paused run.' ), }) .meta({ id: 'CancelWorkflowRunResult', title: 'Cancel workflow run result', description: - 'Outcome of a workflow run cancellation request. Cancellation is best-effort: a run already in a terminal state succeeds with no effect, so poll the run to observe its final state.', + 'Outcome of a workflow run cancellation request. Cancellation is best-effort: a run already in a terminal state succeeds with no effect, reported as `durablyRecorded: false` with an `already_*` reason naming the state observed.', }) export type V2CancelWorkflowRunData = z.output diff --git a/apps/sim/lib/api/contracts/workflows.test.ts b/apps/sim/lib/api/contracts/workflows.test.ts index ad4a68c59da..5f026cfa9b6 100644 --- a/apps/sim/lib/api/contracts/workflows.test.ts +++ b/apps/sim/lib/api/contracts/workflows.test.ts @@ -135,13 +135,12 @@ describe('workflow contracts', () => { * The v2 cancel endpoint presents `cancelWorkflowRun`'s result unchanged, and * that use case delegates wholly to the cancellation service — so it cannot * emit the outcomes the internal route resolves for itself. Folding those into - * the service enum would publish four reasons v2 never returns, because the - * v2 contract documents this enum value by value. + * the service enum would publish reasons v2 never returns, because the v2 + * contract documents this enum value by value. */ it('keeps internal-only cancellation reasons out of the enum v2 publishes', () => { for (const reason of [ 'queue_cancelled', - 'already_cancelled', 'active_resume_signal_failed', 'cancellation_not_finalized', ]) { @@ -150,6 +149,19 @@ describe('workflow contracts', () => { } }) + /** + * Both surfaces answer a cancel against an already-terminal run, so both name + * it with the same member. The service observes the terminal status itself now + * — the internal route no longer owns `already_cancelled` privately — and + * without these the v2 contract rejects the very body v2 emits. + */ + it('shares the terminal no-op vocabulary between both cancel surfaces', () => { + for (const reason of ['already_cancelled', 'already_completed', 'already_failed']) { + expect(cancelWorkflowExecutionReasonSchema.options).toContain(reason) + expect(internalCancelWorkflowExecutionReasonSchema.options).toContain(reason) + } + }) + /** * `workflowStateSchema` is the PUT `/api/workflows/[id]/state` body and also * the `state` slot of the GET response. A stored value outside these bounds diff --git a/apps/sim/lib/api/contracts/workflows.ts b/apps/sim/lib/api/contracts/workflows.ts index 45a29933d25..25c2d286427 100644 --- a/apps/sim/lib/api/contracts/workflows.ts +++ b/apps/sim/lib/api/contracts/workflows.ts @@ -669,9 +669,16 @@ export const workflowExecutionStatusQuerySchema = z.object({ * `lib/execution/cancel-workflow-execution` (contracts stay import-clean of * server modules). Keeping the internal route's extra outcomes out of here is * what stops the published v2 schema advertising reasons v2 cannot emit. + * + * `already_cancelled`/`already_completed`/`already_failed` report a run that was + * already terminal when the request arrived: the request is satisfied, but no + * durable write happened, so they always pair with `durablyRecorded: false`. */ export const cancelWorkflowExecutionReasonSchema = z.enum([ 'recorded', + 'already_cancelled', + 'already_completed', + 'already_failed', 'redis_unavailable', 'redis_write_failed', 'paused_event_publish_failed', @@ -679,17 +686,20 @@ export const cancelWorkflowExecutionReasonSchema = z.enum([ ]) /** - * The internal route's vocabulary. It resolves four outcomes before the service - * is ever reached: `queue_cancelled` (the run was still queued, so no execution - * log row existed), `already_cancelled` (reconciling a run already cancelled), - * and the two stop-signal failures. Several ride on `success: true` responses, - * so validating them against the service enum makes `requestJson` reject - * cancellations that genuinely applied. + * The internal route's vocabulary. It reimplements cancellation rather than + * calling the service, so it resolves three further outcomes of its own: + * `queue_cancelled` (the run was still queued, so no execution log row existed), + * `active_resume_signal_failed`, and `cancellation_not_finalized`. Several ride + * on `success: true` responses, so validating them against the service enum + * makes `requestJson` reject cancellations that genuinely applied. + * + * The `already_*` outcomes are no longer route-local: the service now observes + * the run's terminal status itself, so both surfaces name a terminal no-op with + * the same member. */ export const internalCancelWorkflowExecutionReasonSchema = z.enum([ ...cancelWorkflowExecutionReasonSchema.options, 'queue_cancelled', - 'already_cancelled', 'active_resume_signal_failed', 'cancellation_not_finalized', ]) diff --git a/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.test.ts b/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.test.ts index 6885f3d1a4d..bfb078e8c2f 100644 --- a/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.test.ts +++ b/apps/sim/lib/api/server/routes/v2-body-lifecycle-route.test.ts @@ -190,6 +190,31 @@ describe('defineV2BodyLifecycleRoute', () => { expect(response.headers.get('x-request-id')).toBeTruthy() }) + /** + * `v2InvalidBodyResponse` answers `415` when an unreadable body declared a + * non-JSON media type, and `multipart/form-data` is exactly such a type. That + * change is argued safe for this builder in prose — its contract must omit the + * body schema, so `parseRequest` never attempts a JSON read and the + * classification is unreachable — but nothing executed it. A multipart upload + * is the shape this builder exists for, so it gets a test rather than a + * paragraph. + */ + it('accepts a multipart body, which the JSON builder would classify as 415', async () => { + const form = new FormData() + form.append('file', new Blob([new Uint8Array([1, 2, 3])]), 'data.bin') + const request = new NextRequest( + 'http://localhost/api/v2/body-lifecycle/item-1?workspaceId=workspace-1', + { method: 'POST', headers: { 'x-api-key': 'secret' }, body: form } + ) + expect(request.headers.get('content-type')).toContain('multipart/form-data') + + const response = await buildHandler()(request, context()) + + expect(response.status).toBe(201) + expect(await response.json()).toEqual({ data: { id: 'item-1' } }) + expect(mocks.order).toContain('body') + }) + it('rejects at the IP abuse limit before authentication', async () => { v2RouteMocks.preauthRate.mockImplementation(async () => { mocks.order.push('ip-limit') diff --git a/apps/sim/lib/api/server/routes/v2-json-route.test.ts b/apps/sim/lib/api/server/routes/v2-json-route.test.ts index 45dfd51000a..54755d94317 100644 --- a/apps/sim/lib/api/server/routes/v2-json-route.test.ts +++ b/apps/sim/lib/api/server/routes/v2-json-route.test.ts @@ -495,6 +495,123 @@ describe('defineV2JsonRoute', () => { }) }) +/** + * A body that cannot be read as JSON has two very different causes, and the + * single `400 "Request body must be valid JSON"` describes only one of them: a + * caller who sent a form-encoded body is told to go hunting for a syntax error + * in a body that has none. + * + * These pin the split to the *classification* of an already-failing read. The + * final two are the regression guard that keeps it from becoming a media-type + * gate: a body that parses as JSON still succeeds no matter what the caller + * declared, which is what keeps `curl -d '{…}'` (form-urlencoded by default) + * and a headerless browser `fetch` (`text/plain`) working. + */ +describe('defineV2JsonRoute unreadable body classification', () => { + beforeEach(() => { + vi.clearAllMocks() + v2RouteMocks.authenticate.mockResolvedValue(auth) + v2RouteMocks.gate.mockResolvedValue(null) + v2RouteMocks.preauthRate.mockResolvedValue({ allowed: true, remaining: 599, resetAt }) + v2RouteMocks.operationRate.mockResolvedValue(allowedRate) + }) + + /** + * A `string` body makes undici *derive* `content-type: text/plain;charset=UTF-8`, + * so omitting the header from `headers` is not enough to produce the + * absent-media-type request — the `null` case has to send pre-encoded bytes. + * The assertion is the guard that keeps that from silently drifting back: + * without it the two `contentType === null` cases secretly re-test `text/plain` + * and the `if (!header) return false` branch never runs. + */ + function bodyRequest(contentType: string | null, body: string): NextRequest { + const request = new NextRequest('http://localhost/api/v2/widgets', { + method: 'POST', + headers: { + 'x-api-key': 'secret', + ...(contentType === null ? {} : { 'content-type': contentType }), + }, + body: contentType === null ? new TextEncoder().encode(body) : body, + }) + if (contentType === null) expect(request.headers.get('content-type')).toBeNull() + return request + } + + it('answers 415 when an unreadable body declared a non-JSON media type', async () => { + const response = await createHandler()( + bodyRequest('application/x-www-form-urlencoded', 'value=ok') + ) + + expect(response.status).toBe(415) + await expect(response.json()).resolves.toEqual({ + error: { + code: 'UNSUPPORTED_MEDIA_TYPE', + message: 'Request body must be sent as application/json', + }, + }) + expect(response.headers.get('Cache-Control')).toBe('private, no-store') + }) + + it('keeps 400 for a truncated JSON body, whose media type was right', async () => { + const response = await createHandler()(bodyRequest('application/json', '{"value":')) + + expect(response.status).toBe(400) + await expect(response.json()).resolves.toEqual({ + error: { code: 'BAD_REQUEST', message: 'Request body must be valid JSON' }, + }) + }) + + it('keeps 400 when the media type is absent rather than wrong', async () => { + const response = await createHandler()(bodyRequest(null, '{"value":')) + + expect(response.status).toBe(400) + }) + + it('keeps 400 for text/plain, the default of a headerless browser fetch', async () => { + const response = await createHandler()(bodyRequest('text/plain;charset=UTF-8', '{"value":')) + + expect(response.status).toBe(400) + }) + + it('accepts a JSON body sent under a non-JSON media type, as it does today', async () => { + const response = await createHandler()( + bodyRequest('application/x-www-form-urlencoded', JSON.stringify({ value: 'ok' })) + ) + + expect(response.status).toBe(201) + await expect(response.json()).resolves.toEqual({ data: { value: 'ok' } }) + }) + + it('accepts a JSON body sent with no media type at all', async () => { + const response = await createHandler()(bodyRequest(null, JSON.stringify({ value: 'ok' }))) + + expect(response.status).toBe(201) + }) + + it('keeps 400 for a structured JSON suffix media type', async () => { + const response = await createHandler()(bodyRequest('application/merge-patch+json', '{"value":')) + + expect(response.status).toBe(400) + }) + + it('lets a route override the classification entirely', async () => { + const response = await createHandler({ + parseOptions: { + invalidJsonResponse: () => + NextResponse.json( + { error: { code: 'BAD_REQUEST', message: 'Import archive is not JSON' } }, + { status: 400 } + ), + }, + })(bodyRequest('application/x-www-form-urlencoded', 'value=ok')) + + expect(response.status).toBe(400) + await expect(response.json()).resolves.toEqual({ + error: { code: 'BAD_REQUEST', message: 'Import archive is not JSON' }, + }) + }) +}) + /** * A `HEAD` on a route whose `GET` is not safe must answer the question the `GET` * would answer, minus the effect — not merely the question admission can answer. diff --git a/apps/sim/lib/api/server/routes/v2-json-route.ts b/apps/sim/lib/api/server/routes/v2-json-route.ts index 1c284358dd1..f13eb0416b5 100644 --- a/apps/sim/lib/api/server/routes/v2-json-route.ts +++ b/apps/sim/lib/api/server/routes/v2-json-route.ts @@ -125,12 +125,75 @@ const v2PayloadTooLargeResponse = () => v2Error('PAYLOAD_TOO_LARGE', 'Request bo */ export const v2InvalidJsonResponse = () => v2Error('BAD_REQUEST', 'Request body must be valid JSON') +/** + * Whether the request declared a media type that is not a JSON body at all. + * + * Only consulted once a body has already **failed** to parse as JSON — see + * {@link v2InvalidBodyResponse} — so this decides how to describe a request + * that is failing either way, never whether one is accepted. + * + * An absent `Content-Type` is not a mismatch. A body sent without one is + * indistinguishable from a client that simply omits the header, and today's + * callers include ones that do; treating absence as a refusal is the likeliest + * way to turn a working client into a 415. + * + * `text/plain` is not a mismatch either, and that carve-out is load-bearing: + * `fetch(url, { method: 'POST', body: JSON.stringify(x) })` with no explicit + * headers sends `text/plain;charset=UTF-8`, so it is the default media type of + * a hand-written JSON body from a browser rather than a declaration that the + * body is not JSON. + * + * Anything else — `application/x-www-form-urlencoded`, `multipart/form-data`, + * `application/xml` — is a positive statement that the body is in some other + * format, which is exactly what 415 names. + */ +function declaresNonJsonBody(request: Request): boolean { + const header = request.headers.get('content-type') + if (!header) return false + const mediaType = header.split(';', 1)[0].trim().toLowerCase() + if (!mediaType || mediaType === 'text/plain') return false + const subtype = mediaType.slice(mediaType.indexOf('/') + 1) + return subtype !== 'json' && !subtype.endsWith('+json') +} + +/** + * The v2 answer to a body that could not be read as JSON: `415` when the caller + * declared a non-JSON media type, `400` otherwise. + * + * `400 "Request body must be valid JSON"` is the same answer for a truncated + * JSON body and for a form-encoded one, which leaves a caller who sent + * `application/x-www-form-urlencoded` hunting a syntax error in a body that has + * none. `UNSUPPORTED_MEDIA_TYPE` was already a declared `V2ErrorCode` with no + * path that reached it; this is that path. + * + * Deliberately a **re-classification of an existing failure**, not a new gate. + * It runs only after the JSON read has already failed, so no request that + * succeeds today can start failing: `curl -d '{"a":1}'` without `-H` sends + * form-urlencoded around a body that parses as JSON perfectly well, and that + * caller keeps working exactly as before. A pre-parse content-type gate would + * have broken them — and would also have to special-case the multipart bodies + * `defineV2BodyLifecycleRoute` legitimately accepts. Only the status and + * `error.code` of an already-4xx request change. + */ +export function v2InvalidBodyResponse(request: Request): NextResponse { + return declaresNonJsonBody(request) + ? v2Error('UNSUPPORTED_MEDIA_TYPE', 'Request body must be sent as application/json') + : v2InvalidJsonResponse() +} + /** * The parse failures every v2 route renders the same way. * * The builders spread this, and so must the handful of raw `withRouteHandler` * v2 routes that call `parseRequest` directly — they are exactly the routes a * builder default cannot reach. + * + * `invalidJsonResponse` is the request-unaware 400. `parseRequest` invokes it + * with no arguments, so the media-type-aware {@link v2InvalidBodyResponse} can + * only be installed by a caller that still holds the request — which + * {@link defineV2JsonRoute} does, overriding this entry. A raw route wanting the + * same 415 passes `invalidJsonResponse: () => v2InvalidBodyResponse(request)` + * after spreading this. */ export const V2_PARSE_DEFAULTS = { payloadTooLargeResponse: v2PayloadTooLargeResponse, @@ -360,6 +423,7 @@ export function defineV2JsonRoute< const parsed = await parseRequest(options.contract, request, context ?? {}, { ...V2_PARSE_DEFAULTS, + invalidJsonResponse: () => v2InvalidBodyResponse(request), ...options.parseOptions, validationErrorResponse: v2ValidationError, }) diff --git a/apps/sim/lib/execution/cancel-workflow-execution.test.ts b/apps/sim/lib/execution/cancel-workflow-execution.test.ts index 53621352a03..1f51da65463 100644 --- a/apps/sim/lib/execution/cancel-workflow-execution.test.ts +++ b/apps/sim/lib/execution/cancel-workflow-execution.test.ts @@ -17,6 +17,7 @@ const { mockPublishWorkflowGroupCancellationEvent, mockReleaseExecutionSlot, mockUpdateSet, + mockUpdateReturning, mockResolveWorkflowExecutionOwnership, } = vi.hoisted(() => ({ mockAbortManualExecution: vi.fn(), @@ -32,6 +33,7 @@ const { mockPublishWorkflowGroupCancellationEvent: vi.fn(), mockReleaseExecutionSlot: vi.fn(), mockUpdateSet: vi.fn(), + mockUpdateReturning: vi.fn(), mockResolveWorkflowExecutionOwnership: vi.fn(), })) @@ -40,7 +42,7 @@ vi.mock('@sim/db', () => ({ update: () => ({ set: (values: unknown) => { mockUpdateSet(values) - return { where: () => Promise.resolve(undefined) } + return { where: () => ({ returning: () => Promise.resolve(mockUpdateReturning()) }) } }, }), }, @@ -99,6 +101,16 @@ vi.mock('@/lib/workflows/executor/human-in-the-loop-manager', () => ({ import { cancelWorkflowExecution } from '@/lib/execution/cancel-workflow-execution' +/** + * The durable writes a workflow-group transition reports back. The transaction + * updates the workflow log only, the cell sidecar only, or both, so a single + * `kind` cannot answer whether this request wrote anything. + */ +const NO_WRITES = { workflowLogTerminalized: false, sidecarCancelled: false } as const +const LOG_WRITE = { workflowLogTerminalized: true, sidecarCancelled: false } as const +const SIDECAR_WRITE = { workflowLogTerminalized: false, sidecarCancelled: true } as const +const BOTH_WRITES = { workflowLogTerminalized: true, sidecarCancelled: true } as const + const INPUT = { executionId: 'execution-1', workflowId: 'workflow-1', @@ -112,7 +124,9 @@ describe('cancelWorkflowExecution', () => { mockResolveWorkflowExecutionOwnership.mockResolvedValue({ belongsToWorkflow: true, workflowGroupWorkspaceId: null, + priorStatus: 'running', }) + mockUpdateReturning.mockReturnValue([{ id: 'log-1' }]) mockBeginPausedCancellation.mockResolvedValue(false) mockGetPausedCancellationStatus.mockResolvedValue(null) mockMarkExecutionCancelled.mockResolvedValue({ durablyRecorded: true, reason: 'recorded' }) @@ -128,6 +142,147 @@ describe('cancelWorkflowExecution', () => { }) }) + /** + * The row reads `cancelled` after a successful cancel just as it does after + * someone else's, so a status re-read alone would report this run's own work + * as `already_cancelled`. Nothing is re-read once the claim moved a row. + */ + it('reports a durable write when an active run is cancelled', async () => { + mockResolveWorkflowExecutionOwnership + .mockResolvedValueOnce({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: 'running', + }) + .mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: 'cancelled', + }) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ success: true, durablyRecorded: true, reason: 'recorded' }) + expect(mockResolveWorkflowExecutionOwnership).toHaveBeenCalledTimes(1) + }) + + /** + * A cancel against a run that already reached a terminal state changes + * nothing: the log claim's `status = 'running'` predicate matches no row and + * no terminal metadata moves. Reporting `recorded`/`durablyRecorded: true` + * there tells a caller a durable write happened when none did, so the outcome + * names the state that was actually observed instead. + */ + it.each([ + ['cancelled', 'already_cancelled'], + ['completed', 'already_completed'], + ['failed', 'already_failed'], + ])('reports a run already %s as a no-op rather than a durable write', async (status, reason) => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: status, + }) + mockUpdateReturning.mockReturnValue([]) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ success: true, durablyRecorded: false, reason }) + }) + + /** + * Reclassification only ever applies to an otherwise-clean outcome. A run that + * reached any terminal status can still carry paused-HITL state — a + * force-failed run keeps whatever pause rows it had — and when reconciling + * that genuinely fails, the caller is owed the step that failed rather than a + * no-op that also flips `success` to `true`. + */ + it.each([['cancelled'], ['completed'], ['failed']])( + 'still reports the failing step when a %s run has paused work left over', + async (status) => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: status, + }) + mockBeginPausedCancellation.mockResolvedValue(true) + mockCompletePausedCancellation.mockResolvedValue(false) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ + success: false, + durablyRecorded: true, + reason: 'paused_database_cancel_failed', + }) + } + ) + + /** + * The status read at entry can be stale: a run that finishes after it and + * before the claim leaves a `running` snapshot on a cancel whose claim matched + * no row. The claim's own row count is what separates that from a cancel this + * request really performed. + */ + it.each([ + ['completed', 'already_completed'], + ['failed', 'already_failed'], + ])('reports a run that reached %s after the entry read as a no-op', async (status, reason) => { + mockResolveWorkflowExecutionOwnership + .mockResolvedValueOnce({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: 'running', + }) + .mockResolvedValueOnce({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: status, + }) + mockUpdateReturning.mockReturnValue([]) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ success: true, durablyRecorded: false, reason }) + }) + + it('reports an undifferentiated outcome when the claim finds no durable log row', async () => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: null, + }) + mockUpdateReturning.mockReturnValue([]) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ success: true, durablyRecorded: true, reason: 'recorded' }) + }) + + /** + * The re-read is purely observational — it only refines *which* no-op the + * caller is told about. A database that cannot answer it must not take the + * cancel down with it: the run has already been cancelled in Redis and its + * reservation still has to be released, so the failure degrades to the + * undifferentiated outcome rather than propagating. + */ + it('degrades to the undifferentiated outcome when the status re-read fails', async () => { + mockResolveWorkflowExecutionOwnership + .mockResolvedValueOnce({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: 'running', + }) + .mockRejectedValueOnce(new Error('connection terminated')) + mockUpdateReturning.mockReturnValue([]) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ success: true, durablyRecorded: true, reason: 'recorded' }) + expect(mockResolveWorkflowExecutionOwnership).toHaveBeenCalledTimes(2) + expect(mockReleaseExecutionSlot).toHaveBeenCalledWith('execution-1') + }) + it('releases the plan concurrency reservation after a successful cancellation', async () => { const result = await cancelWorkflowExecution(INPUT) @@ -151,12 +306,14 @@ describe('cancelWorkflowExecution', () => { mockResolveWorkflowExecutionOwnership.mockResolvedValue({ belongsToWorkflow: true, workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'running', }) const cancelled = { kind: 'cancelled' as const, tableId: 'table-1', rowId: 'row-1', groupId: 'group-1', + writes: BOTH_WRITES, } mockCancelWorkflowGroupExecution.mockResolvedValue(cancelled) @@ -173,9 +330,189 @@ describe('cancelWorkflowExecution', () => { expect(mockReleaseExecutionSlot).toHaveBeenCalledWith('execution-1') }) + /** + * A workflow-group log that is already `cancelled` can still own a cell + * sidecar left in `error`, and reconciling it to `cancelled` is a durable + * write this request performed. The terminal entry snapshot cannot see that + * work, so it must not reinterpret the outcome as a no-op — the API would + * otherwise tell the caller nothing changed and drop the cancellation event. + */ + it('reports a durable write when a cancelled group run still had its sidecar reconciled', async () => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'cancelled', + }) + const cancelled = { + kind: 'cancelled' as const, + tableId: 'table-1', + rowId: 'row-1', + groupId: 'group-1', + writes: SIDECAR_WRITE, + } + mockCancelWorkflowGroupExecution.mockResolvedValue(cancelled) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ success: true, durablyRecorded: true, reason: 'recorded' }) + expect(mockPublishWorkflowGroupCancellationEvent).toHaveBeenCalledWith(cancelled, 'execution-1') + }) + + /** + * The group path terminalizes the workflow log itself when the cell sidecar + * is already gone, so that outcome is a durable write too. + */ + it('reports a durable write when the group path cancels a run whose sidecar is gone', async () => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'running', + }) + mockCancelWorkflowGroupExecution.mockResolvedValue({ + kind: 'cancelled_without_sidecar', + writes: LOG_WRITE, + }) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ success: true, durablyRecorded: true, reason: 'recorded' }) + expect(mockUpdateSet).not.toHaveBeenCalled() + }) + + /** + * The mirror case: a group run that was already terminal and whose sidecar was + * already `cancelled` leaves both records untouched, so it still reports the + * state it observed rather than a durable write. + */ + it('reports a group run that changed nothing as a no-op', async () => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'cancelled', + }) + mockCancelWorkflowGroupExecution.mockResolvedValue({ + kind: 'already_cancelled', + tableId: 'table-1', + rowId: 'row-1', + groupId: 'group-1', + writes: NO_WRITES, + }) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ + success: true, + durablyRecorded: false, + reason: 'already_cancelled', + }) + }) + + /** + * The same `already_cancelled` kind covers a transition that left the sidecar + * alone but still terminalized an active workflow log. That log write is + * durable, so the outcome must stay `recorded` and must not re-read a state + * this request itself wrote. + */ + it('reports a durable write when a group run only repaired its workflow log', async () => { + mockResolveWorkflowExecutionOwnership.mockResolvedValue({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'running', + }) + mockCancelWorkflowGroupExecution.mockResolvedValue({ + kind: 'already_cancelled', + tableId: 'table-1', + rowId: 'row-1', + groupId: 'group-1', + writes: LOG_WRITE, + }) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ success: true, durablyRecorded: true, reason: 'recorded' }) + expect(mockResolveWorkflowExecutionOwnership).toHaveBeenCalledOnce() + }) + + /** + * The lost race the sidecar-bearing kind used to hide: a concurrent cancel + * terminalized both records between the entry snapshot and this transaction, + * which then found the sidecar already `cancelled` and the log already + * `cancelled` and wrote nothing. A non-terminal entry snapshot cannot catch + * that, so the transition's own report of having written nothing is what + * forces the re-read — otherwise the request would claim a durable write and + * fire the v2 cancel analytics gate on a no-op. + */ + it('reports a group run that lost the race with its sidecar already cancelled as a no-op', async () => { + mockResolveWorkflowExecutionOwnership + .mockResolvedValueOnce({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'running', + }) + .mockResolvedValueOnce({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'cancelled', + }) + mockCancelWorkflowGroupExecution.mockResolvedValue({ + kind: 'already_cancelled', + tableId: 'table-1', + rowId: 'row-1', + groupId: 'group-1', + writes: NO_WRITES, + }) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ + success: true, + durablyRecorded: false, + reason: 'already_cancelled', + }) + expect(mockResolveWorkflowExecutionOwnership).toHaveBeenCalledTimes(2) + }) + + /** + * The lost-race re-read applies to the group path as well: an entry snapshot + * can still read `running` when the sidecar-less transition finds the log + * already `cancelled` and writes nothing. + */ + it('reports a group run that lost the race to another cancel as a no-op', async () => { + mockResolveWorkflowExecutionOwnership + .mockResolvedValueOnce({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'running', + }) + .mockResolvedValueOnce({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'cancelled', + }) + mockCancelWorkflowGroupExecution.mockResolvedValue({ + kind: 'already_cancelled_without_sidecar', + writes: NO_WRITES, + }) + + const result = await cancelWorkflowExecution(INPUT) + + expect(result).toMatchObject({ + success: true, + durablyRecorded: false, + reason: 'already_cancelled', + }) + expect(mockResolveWorkflowExecutionOwnership).toHaveBeenCalledTimes(2) + }) + it.each([ - [{ kind: 'conflict' as const, status: 'completed' }, 'cannot be cancelled while completed'], - [{ kind: 'not_workflow_group' as const }, 'no longer the active table execution'], + [ + { kind: 'conflict' as const, status: 'completed', writes: NO_WRITES }, + 'cannot be cancelled while completed', + ], + [ + { kind: 'not_workflow_group' as const, writes: NO_WRITES }, + 'no longer the active table execution', + ], ])( 'releases the reservation before reporting a refused workflow-group cell claim as a conflict', async (outcome, message) => { @@ -195,8 +532,8 @@ describe('cancelWorkflowExecution', () => { ) it.each([ - [{ kind: 'conflict' as const, status: 'completed' }], - [{ kind: 'not_workflow_group' as const }], + [{ kind: 'conflict' as const, status: 'completed', writes: NO_WRITES }], + [{ kind: 'not_workflow_group' as const, writes: NO_WRITES }], ])( 'keeps the reservation held when a refused claim follows a paused cancellation', async (outcome) => { @@ -213,8 +550,8 @@ describe('cancelWorkflowExecution', () => { ) it.each([ - [{ kind: 'conflict' as const, status: 'completed' }], - [{ kind: 'not_workflow_group' as const }], + [{ kind: 'conflict' as const, status: 'completed', writes: NO_WRITES }], + [{ kind: 'not_workflow_group' as const, writes: NO_WRITES }], ])( 'keeps the reservation held when a refused claim follows a failed cancellation', async (outcome) => { @@ -237,6 +574,7 @@ describe('cancelWorkflowExecution', () => { mockResolveWorkflowExecutionOwnership.mockResolvedValue({ belongsToWorkflow: true, workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'running', }) const failure = new Error('Workflow-group cancellation lost its locked workflow-log claim') mockCancelWorkflowGroupExecution.mockRejectedValue(failure) @@ -251,6 +589,7 @@ describe('cancelWorkflowExecution', () => { mockResolveWorkflowExecutionOwnership.mockResolvedValue({ belongsToWorkflow: true, workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'running', }) mockBeginPausedCancellation.mockResolvedValue(true) mockCancelWorkflowGroupExecution.mockRejectedValue(new Error('serialization conflict')) @@ -263,6 +602,7 @@ describe('cancelWorkflowExecution', () => { mockResolveWorkflowExecutionOwnership.mockResolvedValue({ belongsToWorkflow: true, workflowGroupWorkspaceId: 'workspace-1', + priorStatus: 'running', }) mockMarkExecutionCancelled.mockResolvedValue({ durablyRecorded: false, diff --git a/apps/sim/lib/execution/cancel-workflow-execution.ts b/apps/sim/lib/execution/cancel-workflow-execution.ts index 8d542d3bb27..10e92517854 100644 --- a/apps/sim/lib/execution/cancel-workflow-execution.ts +++ b/apps/sim/lib/execution/cancel-workflow-execution.ts @@ -18,6 +18,7 @@ import { cancelWorkflowGroupExecution, type PublishableWorkflowGroupCancellation, publishWorkflowGroupCancellationEvent, + type WorkflowGroupCancellationWrites, } from '@/lib/table/workflow-group-cancellation' import { WORKFLOW_EXECUTION_JOB_ID_PREFIX } from '@/lib/workflows/executor/execution-job-ids' import { resolveWorkflowExecutionOwnership } from '@/lib/workflows/executor/execution-queries' @@ -45,17 +46,109 @@ async function cancelActiveWorkflowJob(executionId: string): Promise { * Cancellation outcome vocabulary produced by this service, and so the whole * vocabulary the public v2 endpoint can return. `recorded`/`redis_unavailable`/ * `redis_write_failed` come from the Redis record step; the two `paused_*` - * values from the paused-HITL path. The internal cancel route resolves further - * outcomes on top of these — see `internalCancelWorkflowExecutionReasonSchema` - * in `lib/api/contracts/workflows`. + * values from the paused-HITL path; the three `already_*` values report a run + * that was already terminal when the request arrived, where the cancel claim + * matched no row and nothing durable was written. The internal cancel route + * resolves further outcomes on top of these — see + * `internalCancelWorkflowExecutionReasonSchema` in `lib/api/contracts/workflows`. */ export type CancelWorkflowExecutionReason = | 'recorded' + | 'already_cancelled' + | 'already_completed' + | 'already_failed' | 'redis_unavailable' | 'redis_write_failed' | 'paused_event_publish_failed' | 'paused_database_cancel_failed' +/** Maps each log status a cancel claim can never move to the outcome that reports it. */ +const TERMINAL_NO_OP_REASONS = { + cancelled: 'already_cancelled', + completed: 'already_completed', + failed: 'already_failed', +} as const satisfies Record + +type TerminalExecutionStatus = keyof typeof TERMINAL_NO_OP_REASONS + +function toTerminalExecutionStatus( + status: string | null | undefined +): TerminalExecutionStatus | null { + return typeof status === 'string' && status in TERMINAL_NO_OP_REASONS + ? (status as TerminalExecutionStatus) + : null +} + +/** + * What this request's own terminal claim did: moved the run to `cancelled` here + * and now, provably matched no row, or ran on a path that cannot tell. Every + * path that can terminalize the run — the direct log claim and the + * workflow-group transition — answers in this one vocabulary, so the report can + * ask a single question: did this request durably write? + * + * Only the direct claim ever answers `unknown`, and only when it could not run + * or its statement failed. The workflow-group transition always knows: it + * reports the writes it performed. + */ +type TerminalWriteOutcome = 'applied' | 'no_row' | 'unknown' + +/** + * Reads a workflow-group transition's durability off the writes it reported + * rather than off its `kind`. Terminalizing the workflow log and cancelling the + * cell sidecar are each a durable write this request performed, and a single + * `kind` covers both a transition that did one of them and one that did + * neither: `already_cancelled` leaves a sidecar that was already `cancelled` + * alone, but may still have terminalized an active workflow log. + */ +function toTerminalWriteOutcome(writes: WorkflowGroupCancellationWrites): TerminalWriteOutcome { + return writes.workflowLogTerminalized || writes.sidecarCancelled ? 'applied' : 'no_row' +} + +/** + * Names the terminal state the cancel could not move, or `null` when it did + * real work or when this path cannot tell — in which case the caller keeps the + * undifferentiated report rather than guessing. + * + * A request that durably wrote is never a no-op, whatever the entry snapshot + * said. A run can be terminal at entry and still owe this request a real write: + * a workflow-group run whose log is already `cancelled` can carry a sidecar left + * in `error`, and reconciling it is a durable cancellation that the entry + * snapshot cannot see. + * + * The status read at entry is not enough on its own in the other direction + * either: a run that finishes between that read and the claim leaves a stale + * non-terminal snapshot behind a cancel that wrote nothing. The claim's own row + * count settles that, and a plain post-read cannot: after a successful cancel + * the row reads `cancelled` too, so the state has to be attributed to whoever + * wrote it. A claim that moved no row against a non-terminal snapshot re-reads + * the row it lost the race to, through the same ownership query the entry read + * came from. + * + * Purely observational — it gates no effect, and a read failure falls back to + * the undifferentiated report rather than failing the cancel. + */ +async function resolveTerminalNoOpReason( + executionId: string, + workflowId: string, + priorTerminalStatus: TerminalExecutionStatus | null, + terminalWrite: TerminalWriteOutcome +): Promise { + if (terminalWrite === 'applied') return null + if (priorTerminalStatus !== null) return TERMINAL_NO_OP_REASONS[priorTerminalStatus] + if (terminalWrite !== 'no_row') return null + try { + const { priorStatus } = await resolveWorkflowExecutionOwnership(executionId, workflowId) + const terminalStatus = toTerminalExecutionStatus(priorStatus) + return terminalStatus !== null ? TERMINAL_NO_OP_REASONS[terminalStatus] : null + } catch (error) { + logger.warn('Failed to re-read execution status after an unmatched cancel claim', { + executionId, + error, + }) + return null + } +} + export interface CancelWorkflowExecutionResult { success: boolean executionId: string @@ -173,11 +266,10 @@ export async function cancelWorkflowExecution( ): Promise { const { executionId, workflowId, userId, workspaceId } = input - const { belongsToWorkflow, workflowGroupWorkspaceId } = await resolveWorkflowExecutionOwnership( - executionId, - workflowId - ) + const { belongsToWorkflow, workflowGroupWorkspaceId, priorStatus } = + await resolveWorkflowExecutionOwnership(executionId, workflowId) if (!belongsToWorkflow) throw new WorkflowExecutionNotFoundError() + const priorTerminalStatus = toTerminalExecutionStatus(priorStatus) let pausedCancellationStarted = false let pausedCancelled = false @@ -372,14 +464,20 @@ export async function cancelWorkflowExecution( ? groupCancellation : null - if ( - groupCancellation === null && + /** + * The claim's row count is read back only to report it — `returning` changes + * what the statement returns, never the row it writes or the rows it matches. + */ + let terminalWrite: TerminalWriteOutcome = 'unknown' + if (groupCancellation !== null) { + terminalWrite = toTerminalWriteOutcome(groupCancellation.writes) + } else if ( (cancellation.durablyRecorded || queuedJobCancelled || locallyAborted) && !pausedCancelled ) { try { const cancelledAt = new Date() - await db + const claimedRows = await db .update(workflowExecutionLogs) .set(cancelledExecutionLogFields(cancelledAt)) .where( @@ -388,6 +486,8 @@ export async function cancelWorkflowExecution( eq(workflowExecutionLogs.status, 'running') ) ) + .returning({ id: workflowExecutionLogs.id }) + terminalWrite = claimedRows.length > 0 ? 'applied' : 'no_row' } catch (dbError) { logger.warn('Failed to update execution log status directly', { executionId, @@ -426,16 +526,36 @@ export async function cancelWorkflowExecution( ? 'recorded' : cancellation.reason + /** + * A run that was already terminal when the request arrived cannot be + * cancelled again: the claim's `status = 'running'` predicate matched no row + * and no terminal metadata moved, so `recorded`/`durablyRecorded: true` would + * claim a durable write that never happened. Every effect above still ran + * exactly as before — only the report changes. The request is still satisfied, + * because the run is not running, so `success` stays `true`. + * + * Reinterpreting is only ever right when nothing else went wrong. A terminal + * run — cancelled, or force-failed with paused state left behind — can still + * carry real paused-HITL reconciliation work, and a genuine failure there owes + * the caller the step that failed, not a no-op. So only an otherwise-clean + * `recorded` is a candidate, whatever the prior status was — and only when + * this request wrote nothing durable on any path. + */ + const terminalNoOpReason = + reason === 'recorded' && !pausedCancelled + ? await resolveTerminalNoOpReason(executionId, workflowId, priorTerminalStatus, terminalWrite) + : null + return { - success, + success: terminalNoOpReason ? true : success, executionId, redisAvailable: isPausedCancellationPath || pausedCancelled ? pausedCancellationPublished : cancellation.reason !== 'redis_unavailable', - durablyRecorded, + durablyRecorded: terminalNoOpReason ? false : durablyRecorded, locallyAborted, pausedCancelled, - reason, + reason: terminalNoOpReason ?? reason, } } diff --git a/apps/sim/lib/logs/execution/cancellation.test.ts b/apps/sim/lib/logs/execution/cancellation.test.ts index 4c2485cb08e..d37c5c28cf9 100644 --- a/apps/sim/lib/logs/execution/cancellation.test.ts +++ b/apps/sim/lib/logs/execution/cancellation.test.ts @@ -11,7 +11,9 @@ vi.unmock('@sim/db/schema') process.env.DATABASE_URL ??= 'postgresql://user:pass@localhost:5432/test' const { PgDialect } = await import('drizzle-orm/pg-core') -const { cancelledExecutionLogFields } = await import('@/lib/logs/execution/cancellation') +const { cancelledExecutionLogFields, terminalExecutionLogFields } = await import( + '@/lib/logs/execution/cancellation' +) describe('cancelledExecutionLogFields', () => { /** @@ -47,3 +49,40 @@ describe('cancelledExecutionLogFields', () => { expect(params).toContain(endedAt.toISOString()) }) }) + +describe('terminalExecutionLogFields', () => { + /** + * The force-fail boundaries — `LoggingSession.markExecutionAsFailed` and + * `PauseResumeManager.markResumeFailed` — leave the same row behind as a + * cancellation, only under a different status. Only the status may differ. + */ + it('writes the cancellation field set under the failed status', () => { + const endedAt = new Date('2026-08-13T12:00:05.000Z') + + const failed = terminalExecutionLogFields('failed', endedAt) + + expect(Object.keys(failed).sort()).toEqual( + Object.keys(cancelledExecutionLogFields(endedAt)).sort() + ) + expect(failed.status).toBe('failed') + expect(failed.endedAt).toBe(endedAt) + expect(failed.executionDeadlineAt).toBeNull() + + const { params } = new PgDialect().sqlToQuery(failed.totalDurationMs) + expect(params).toContain(endedAt.toISOString()) + }) + + /** The cancellation call sites must keep emitting exactly what they did. */ + it('is what the cancellation binding emits', () => { + const endedAt = new Date('2026-08-13T12:00:05.000Z') + const dialect = new PgDialect() + + const bound = cancelledExecutionLogFields(endedAt) + const direct = terminalExecutionLogFields('cancelled', endedAt) + + expect({ ...bound, totalDurationMs: dialect.sqlToQuery(bound.totalDurationMs) }).toEqual({ + ...direct, + totalDurationMs: dialect.sqlToQuery(direct.totalDurationMs), + }) + }) +}) diff --git a/apps/sim/lib/logs/execution/cancellation.ts b/apps/sim/lib/logs/execution/cancellation.ts index 09e68df1d12..6191fb18747 100644 --- a/apps/sim/lib/logs/execution/cancellation.ts +++ b/apps/sim/lib/logs/execution/cancellation.ts @@ -1,23 +1,46 @@ import { elapsedDurationMsSql } from '@/lib/logs/execution/duration' /** - * The fields every terminal cancellation sets on a `workflow_execution_logs` - * row, ready to spread into `.set()`. + * The statuses a terminal write outside `completeWorkflowExecution` can land a + * `workflow_execution_logs` row on: cancellation, and the force-fail boundaries + * that bypass the completion path. + */ +type TerminalExecutionLogStatus = 'cancelled' | 'failed' + +/** + * The fields every terminal write outside `completeWorkflowExecution` sets on a + * `workflow_execution_logs` row, ready to spread into `.set()`. + * + * The cancellation paths — direct, workflow-group with and without a sidecar, + * paused, and the async cancel route — and the two force-fail boundaries — + * `LoggingSession.markExecutionAsFailed` and `PauseResumeManager.markResumeFailed` + * — differ in their database handle, their claim predicate, whether they read + * the row back, and what they do when the claim is lost, so they remain separate + * statements. What they must not differ in is the row they leave behind, and + * hand-assembling this payload at each one had already dropped + * `executionDeadlineAt` at a single cancellation site and both the end timestamp + * and the duration at both force-fail sites, leaving a terminal run still + * carrying the deadline of an attempt that had stopped running and invisible to + * every `minDurationMs`/`maxDurationMs` query on `GET /api/v2/logs`. * - * The five cancellation paths — direct, workflow-group with and without a - * sidecar, paused, and the async cancel route — differ in their database - * handle, their claim predicate, whether they read the row back, and what they - * do when the claim is lost, so they remain separate statements. What they must - * not differ in is the row they leave behind, and hand-assembling this payload - * at each one had already dropped `executionDeadlineAt` at a single site, - * leaving a cancelled run still carrying the deadline of an attempt that had - * stopped running. + * A duration a paused run already recorded still wins — that guard lives in + * `elapsedDurationMsSql`, keyed on the row's own status, so a force-fail landing + * on a still-`pending` paused row keeps the checkpoint duration rather than + * redefining it to include the time the run sat waiting. */ -export function cancelledExecutionLogFields(endedAt: Date) { +export function terminalExecutionLogFields( + status: TStatus, + endedAt: Date +) { return { - status: 'cancelled' as const, + status, endedAt, totalDurationMs: elapsedDurationMsSql(endedAt), executionDeadlineAt: null, } } + +/** {@link terminalExecutionLogFields} bound to the cancellation status. */ +export function cancelledExecutionLogFields(endedAt: Date) { + return terminalExecutionLogFields('cancelled', endedAt) +} diff --git a/apps/sim/lib/logs/execution/logging-session.test.ts b/apps/sim/lib/logs/execution/logging-session.test.ts index 172c08c419c..3b11cbce4c7 100644 --- a/apps/sim/lib/logs/execution/logging-session.test.ts +++ b/apps/sim/lib/logs/execution/logging-session.test.ts @@ -1,10 +1,21 @@ +/** + * @vitest-environment node + */ + +import { workflowExecutionLogs } from '@sim/db/schema' import { dbChainMockFns, resetDbChainMock } from '@sim/testing' import { afterAll, beforeEach, describe, expect, it, vi } from 'vitest' const dbMocks = vi.hoisted(() => ({ eq: vi.fn(), and: vi.fn((...args: unknown[]) => ({ type: 'and', args })), - sql: vi.fn((strings: TemplateStringsArray, ...values: unknown[]) => ({ strings, values })), + sql: Object.assign( + vi.fn((strings: TemplateStringsArray, ...values: unknown[]) => ({ strings, values })), + { + /** `elapsedDurationMsSql` binds `ended_at` through the column's own mapper. */ + param: vi.fn((value: unknown, encoder?: unknown) => ({ value, encoder })), + } + ), })) const { @@ -1683,6 +1694,38 @@ describe('LoggingSession.markExecutionAsFailed workflowId scoping', () => { expect(statusGuards).toHaveLength(1) }) + it('terminalizes the row it force-fails: end timestamp, derived duration, deadline cleared', async () => { + await LoggingSession.markExecutionAsFailed('exec-terminal', 'boom', undefined, 'wf-1') + + const payload = dbChainMockFns.set.mock.calls[0]?.[0] as { + level: string + status: string + endedAt: Date + totalDurationMs: { strings: TemplateStringsArray; values: unknown[] } + executionDeadlineAt: Date | null + executionData: unknown + } + expect(payload.level).toBe('error') + expect(payload.status).toBe('failed') + expect(payload.endedAt).toBeInstanceOf(Date) + expect(payload.executionDeadlineAt).toBeNull() + + /** + * The duration is the derived SQL fragment, not a number the caller carried + * in — `elapsedDurationMsSql` measures against the row's own `started_at` + * and preserves what a paused row already banked. + */ + expect(String(Array.from(payload.totalDurationMs.strings))).toContain("= 'pending' THEN ") + expect(payload.totalDurationMs.values).toContain(workflowExecutionLogs.totalDurationMs) + + /** + * The end instant is bound through `started_at`'s encoder specifically. + * `endedAt`'s encoder renders identically and would silently subtract the + * timestamp from itself — a duration of zero on every force-failed run. + */ + expect(dbMocks.sql.param).toHaveBeenCalledWith(payload.endedAt, workflowExecutionLogs.startedAt) + }) + it('clears Redis markers when marking failed (terminal boundary outside completeWorkflowExecution)', async () => { await LoggingSession.markExecutionAsFailed('exec-3', 'boom', undefined, 'wf-3') expect(clearProgressMarkersMock).toHaveBeenCalledWith('exec-3') diff --git a/apps/sim/lib/logs/execution/logging-session.ts b/apps/sim/lib/logs/execution/logging-session.ts index 9102684ee6f..8536910302e 100644 --- a/apps/sim/lib/logs/execution/logging-session.ts +++ b/apps/sim/lib/logs/execution/logging-session.ts @@ -8,6 +8,7 @@ import type { BillingAttributionSnapshot } from '@/lib/billing/core/billing-attr import { isRetryableInfrastructureError } from '@/lib/core/errors/retryable-infrastructure' import { RESERVATION_TTL_BUFFER_MS } from '@/lib/core/execution-limits' import type { LargeValueStoreContext } from '@/lib/execution/payloads/store' +import { terminalExecutionLogFields } from '@/lib/logs/execution/cancellation' import type { SecretSafeBlockLog } from '@/lib/logs/execution/display-types' import { executionLogger } from '@/lib/logs/execution/logger' import { @@ -1545,7 +1546,11 @@ export class LoggingSession { await execDb .update(workflowExecutionLogs) - .set({ level: 'error', status: 'failed', executionDeadlineAt: null, executionData }) + .set({ + level: 'error', + ...terminalExecutionLogFields('failed', new Date()), + executionData, + }) .where( and( eq(workflowExecutionLogs.executionId, executionId), diff --git a/apps/sim/lib/table/__tests__/sql.test.ts b/apps/sim/lib/table/__tests__/sql.test.ts index 56a2118438c..8623b06f7cf 100644 --- a/apps/sim/lib/table/__tests__/sql.test.ts +++ b/apps/sim/lib/table/__tests__/sql.test.ts @@ -1038,3 +1038,292 @@ describe('legacy compiler rejects a v2 predicate (version-mismatch fail-fast)', ).not.toThrow() }) }) + +/** + * Equality/membership compiles to exact JSONB containment, which is untyped: + * `{"score":8} @> {"score":"8"}` is simply FALSE. Before the operand was read + * through the column type, a wrongly-typed `eq`/`ne`/`in`/`nin` returned an + * empty 200 that a caller could not tell apart from a genuinely empty table — + * while the range operators on the same column answered with a descriptive 400. + */ +describe('containment operators — operand is read through the column type', () => { + const NUM: ColumnDefinition[] = [{ id: 'score', name: 'score', type: 'number' }] + const BOOL: ColumnDefinition[] = [{ id: 'flag', name: 'flag', type: 'boolean' }] + const STR: ColumnDefinition[] = [{ id: 'title', name: 'title', type: 'string' }] + const DATE: ColumnDefinition[] = [{ id: 'due', name: 'due', type: 'date' }] + const MONEY: ColumnDefinition[] = [{ id: 'price', name: 'price', type: 'currency' }] + + describe('coerces an unambiguous operand the way a write would', () => { + it.each([ + ['eq', { all: [{ field: 'score', op: 'eq', value: '8' }] }, '"score":8'], + ['ne', { all: [{ field: 'score', op: 'ne', value: '8' }] }, '"score":8'], + ['in', { all: [{ field: 'score', op: 'in', value: ['8'] }] }, '"score":8'], + ['nin', { all: [{ field: 'score', op: 'nin', value: ['8'] }] }, '"score":8'], + ] as Array<[string, TablePredicate, string]>)('%s on a number column', (_op, p, expected) => { + const out = render(buildPredicateClause(p, TABLE, NUM)) + expect(out).toContain(expected) + expect(out).not.toContain('"score":"8"') + }) + + it('reads "false" as the boolean false', () => { + const p: TablePredicate = { all: [{ field: 'flag', op: 'eq', value: 'false' }] } + const out = render(buildPredicateClause(p, TABLE, BOOL)) + expect(out).toContain('"flag":false') + expect(out).not.toContain('"flag":"false"') + }) + + it('reads a number as text on a string column', () => { + const p: TablePredicate = { all: [{ field: 'title', op: 'eq', value: 8 }] } + const out = render(buildPredicateClause(p, TABLE, STR)) + expect(out).toContain('"title":"8"') + }) + + it('reads a formatted amount on a currency column', () => { + const p: TablePredicate = { all: [{ field: 'price', op: 'eq', value: '$1,234.56' }] } + const out = render(buildPredicateClause(p, TABLE, MONEY)) + expect(out).toContain('"price":1234.56') + }) + + it('applies to the legacy $-grammar too', () => { + const out = render(buildFilterClause({ score: { $eq: '8' } }, TABLE, NUM)) + expect(out).toContain('"score":8') + expect(out).not.toContain('"score":"8"') + }) + + it('applies to the legacy equality shorthand', () => { + const out = render(buildFilterClause({ score: '8' }, TABLE, NUM)) + expect(out).toContain('"score":8') + expect(out).not.toContain('"score":"8"') + }) + }) + + /** + * Coercion is best-effort, never fatal. The v2 predicate grammar is not + * operand-type-checked at the boundary (leaf `value` is `z.unknown()`), so a + * throw here would land inside the background runners that compile the same + * predicate later — including a filter-scoped cancel, which would leave those + * cells uncancellable. An operand the column type refuses therefore compiles + * exactly as it did before: byte-exact, matching nothing. + */ + describe('passes through an operand the column could never hold', () => { + it.each(['eq', 'ne'] as const)('%s with an unparseable number', (op) => { + const p = { all: [{ field: 'score', op, value: 'eight' }] } as TablePredicate + const out = render(buildPredicateClause(p, TABLE, NUM)) + expect(out).toContain('"score":"eight"') + }) + + it.each(['in', 'nin'] as const)('%s with a bad element', (op) => { + const p = { all: [{ field: 'score', op, value: ['eight'] }] } as TablePredicate + expect(render(buildPredicateClause(p, TABLE, NUM))).toContain('"score":"eight"') + }) + + it.each(['in', 'nin'] as const)('%s mixing coercible and uncoercible members', (op) => { + const p = { all: [{ field: 'score', op, value: ['8', 'eight'] }] } as TablePredicate + let out = '' + expect(() => { + out = render(buildPredicateClause(p, TABLE, NUM)) + }).not.toThrow() + expect(out).toContain('"score":8') + expect(out).toContain('"score":"eight"') + }) + + it('passes through a non-boolean on a boolean column', () => { + const p: TablePredicate = { all: [{ field: 'flag', op: 'eq', value: 'yes' }] } + expect(render(buildPredicateClause(p, TABLE, BOOL))).toContain('"flag":"yes"') + }) + + it('passes through an unparseable date', () => { + const p: TablePredicate = { all: [{ field: 'due', op: 'eq', value: 'not-a-date' }] } + expect(render(buildPredicateClause(p, TABLE, DATE))).toContain('"due":"not-a-date"') + }) + + it('passes through an object on a string column', () => { + const p = { + all: [{ field: 'title', op: 'eq', value: { a: 1 } }], + } as unknown as TablePredicate + expect(() => buildPredicateClause(p, TABLE, STR)).not.toThrow() + }) + + it('passes through on the legacy $-grammar too', () => { + expect(render(buildFilterClause({ score: { $eq: 'eight' } }, TABLE, NUM))).toContain( + '"score":"eight"' + ) + }) + }) + + /** + * `date` is excluded from containment coercion because `date.coerce` is not + * idempotent — `normalizeDateCellValue` drops the sub-second part, so the + * `.000Z` form the write path stores would be rewritten to a string that no + * longer matches the stored bytes. The same leaf compiles the unique and + * upsert probes, whose operands were already coerced once upstream, so a + * rewrite there would silently admit duplicates inside the write transaction. + */ + describe('never rewrites a date operand', () => { + it.each(['eq', 'ne'] as const)('%s keeps the stored .000Z form byte-exact', (op) => { + const p = { + all: [{ field: 'due', op, value: '2024-01-31T10:00:00.000Z' }], + } as TablePredicate + const out = render(buildPredicateClause(p, TABLE, DATE)) + expect(out).toContain('"due":"2024-01-31T10:00:00.000Z"') + expect(out).not.toContain('"due":"2024-01-31T10:00:00Z"') + }) + + it.each(['in', 'nin'] as const)('%s keeps every member byte-exact', (op) => { + const p = { + all: [{ field: 'due', op, value: ['2024-01-31T10:00:00.000Z', ' 2024-01-31 '] }], + } as TablePredicate + const out = render(buildPredicateClause(p, TABLE, DATE)) + expect(out).toContain('"due":"2024-01-31T10:00:00.000Z"') + expect(out).toContain('"due":" 2024-01-31 "') + }) + + it('does not trim or normalize a loose date operand', () => { + const p: TablePredicate = { all: [{ field: 'due', op: 'eq', value: ' 2024-01-31 ' }] } + const out = render(buildPredicateClause(p, TABLE, DATE)) + expect(out).toContain('"due":" 2024-01-31 "') + expect(out).not.toContain('"due":"2024-01-31"') + }) + + it('keeps the legacy $-grammar byte-exact too', () => { + const out = render( + buildFilterClause({ due: { $eq: '2024-01-31T10:00:00.000Z' } }, TABLE, DATE) + ) + expect(out).toContain('"due":"2024-01-31T10:00:00.000Z"') + }) + }) + + describe('leaves the operands that are not type assertions alone', () => { + it('keeps null — a real containment query for a JSON-null cell', () => { + const p: TablePredicate = { all: [{ field: 'score', op: 'eq', value: null }] } + expect(render(buildPredicateClause(p, TABLE, NUM))).toContain('"score":null') + }) + + it('keeps the empty string — the cleared-cell sentinel', () => { + const p: TablePredicate = { all: [{ field: 'score', op: 'eq', value: '' }] } + expect(render(buildPredicateClause(p, TABLE, NUM))).toContain('"score":""') + }) + + it('leaves a field with no schema entry untouched', () => { + const p: TablePredicate = { all: [{ field: 'adhoc', op: 'eq', value: '8' }] } + expect(render(buildPredicateClause(p, TABLE, NO_COLUMNS))).toContain('"adhoc":"8"') + }) + + /** + * `select` is excluded from containment coercion wholesale, and an operand + * that is already a declared option id cannot show that: `select.coerce` + * resolves it to itself, so the clause is identical with or without the + * exclusion. What discriminates is an operand `select.coerce` would + * *rewrite* — an option **name**, which `resolveSelectCellValue` turns into + * the option id. Names are already resolved upstream by + * `resolvePredicateSelectValues`, so a second resolution here is a rewrite + * of an operand that was deliberately left alone. + */ + describe('leaves a select column to its own name→id resolution', () => { + const statusCol: ColumnDefinition = { + id: 'col_status', + name: 'status', + type: 'select', + options: [{ id: 'opt_open', name: 'Open' }], + } + + it('keeps a declared option id', () => { + const p: TablePredicate = { all: [{ field: 'col_status', op: 'eq', value: 'opt_open' }] } + expect(render(buildPredicateClause(p, TABLE, [statusCol]))).toContain('"opt_open"') + }) + + it('does not re-resolve an option name into its id', () => { + const p: TablePredicate = { all: [{ field: 'col_status', op: 'eq', value: 'Open' }] } + const out = render(buildPredicateClause(p, TABLE, [statusCol])) + expect(out).toContain('"col_status":"Open"') + expect(out).not.toContain('"col_status":"opt_open"') + }) + + it('does not re-resolve names inside an $in list', () => { + const p: TablePredicate = { + all: [{ field: 'col_status', op: 'in', value: ['Open', 'opt_open'] }], + } + const out = render(buildPredicateClause(p, TABLE, [statusCol])) + expect(out).toContain('"col_status":"Open"') + }) + + /** + * A filter for an option deleted since the row was written must still + * compile — the row still stores the id, and the operand reaches the + * clause byte-exact rather than being dropped or refused. + */ + it('keeps an option id no longer in options', () => { + const p: TablePredicate = { all: [{ field: 'col_status', op: 'eq', value: 'opt_ghost' }] } + let out = '' + expect(() => { + out = render(buildPredicateClause(p, TABLE, [statusCol])) + }).not.toThrow() + expect(out).toContain('"col_status":"opt_ghost"') + }) + }) + }) +}) + +/** + * Filters reach the SQL builders storage-keyed — the boundaries translate + * column name → column id first — so a message interpolating the raw field + * reported a `col_…` id the caller never sent and cannot look up. + */ +describe('error messages name the caller-facing column, not the storage id', () => { + const multi: ColumnDefinition = { + id: 'col_934cea93275d46448b0d6c001554e146', + name: 'untitled_2', + type: 'select', + multiple: true, + options: [{ id: 'opt_a', name: 'Alpha' }], + } + const num: ColumnDefinition = { id: 'col_abc123', name: 'overall_score', type: 'number' } + const bool: ColumnDefinition = { id: 'col_def456', name: 'untitled', type: 'boolean' } + const str: ColumnDefinition = { id: 'col_ghi789', name: 'headline', type: 'string' } + + function expectNamed(fn: () => unknown, name: string, id: string) { + expect(fn).toThrow(new RegExp(`"${name}"`)) + expect(fn).not.toThrow(new RegExp(id)) + } + + it('names the column on an unsupported select operator (v2 grammar)', () => { + const p: TablePredicate = { all: [{ field: multi.id as string, op: 'eq', value: 'c' }] } + expectNamed(() => buildPredicateClause(p, TABLE, [multi]), 'untitled_2', 'col_934cea') + }) + + it('names the column on an unsupported select operator (legacy grammar)', () => { + expectNamed( + () => buildFilterClause({ [multi.id as string]: { $eq: 'c' } }, TABLE, [multi]), + 'untitled_2', + 'col_934cea' + ) + }) + + it('names the column on a range-operator type mismatch', () => { + const p: TablePredicate = { all: [{ field: 'col_abc123', op: 'gt', value: '7' }] } + expectNamed(() => buildPredicateClause(p, TABLE, [num]), 'overall_score', 'col_abc123') + }) + + it('names the column on an unorderable range operator', () => { + const p: TablePredicate = { all: [{ field: 'col_def456', op: 'gt', value: 1 }] } + expectNamed(() => buildPredicateClause(p, TABLE, [bool]), 'untitled', 'col_def456') + }) + + it('names the column on an empty pattern operand', () => { + const p: TablePredicate = { all: [{ field: 'col_ghi789', op: 'contains', value: '' }] } + expectNamed(() => buildPredicateClause(p, TABLE, [str]), 'headline', 'col_ghi789') + }) + + it('names the column on a bad $empty flag', () => { + expectNamed( + () => buildFilterClause({ col_ghi789: { $empty: 1 } } as unknown as Filter, TABLE, [str]), + 'headline', + 'col_ghi789' + ) + }) + + it('has no message to name on a containment type mismatch — it does not throw', () => { + const p: TablePredicate = { all: [{ field: 'col_abc123', op: 'eq', value: 'seven' }] } + expect(() => buildPredicateClause(p, TABLE, [num])).not.toThrow() + }) +}) diff --git a/apps/sim/lib/table/sql.ts b/apps/sim/lib/table/sql.ts index 6a90b5280db..4d5c6c9ab04 100644 --- a/apps/sim/lib/table/sql.ts +++ b/apps/sim/lib/table/sql.ts @@ -12,6 +12,7 @@ import { sql } from 'drizzle-orm' import { getColumnId } from '@/lib/table/column-keys' import { columnTypeById, + columnTypeOf, filterOperatorsFor, MULTI_SELECT_OPERATORS, SINGLE_SELECT_OPERATORS, @@ -350,6 +351,19 @@ function validateOperator(operator: string): void { } } +/** + * The caller-facing name for a field in an error message. + * + * Filters reach the SQL builders **storage-keyed** — the boundaries translate + * column name → column id first — so interpolating the raw `field` reports a + * `col_…` id the caller never sent and cannot look up. The definition already in + * hand carries the display name; fall back to `field` for a system column + * (`createdAt`), an unknown key, or a legacy column whose id IS its name. + */ +function columnLabel(field: string, column: ColumnDefinition | undefined): string { + return column?.name ?? field +} + /** * Validates that a range-operator value matches its column's expected JS type * before it reaches Postgres. Surfaces an actionable, column-named error at the @@ -357,31 +371,89 @@ function validateOperator(operator: string): void { * from the database. */ function validateComparisonValue( - field: string, + label: string, columnType: ColumnType | undefined, cast: 'numeric' | 'timestamptz', value: number | string ): void { if (cast === 'numeric' && typeof value !== 'number') { - const label = columnType ?? 'number' + const typeLabel = columnType ?? 'number' throw new TableQueryValidationError( - `Range operator on column "${field}" (${label}) requires a number, got ${typeof value}` + `Range operator on column "${label}" (${typeLabel}) requires a number, got ${typeof value}` ) } if (cast === 'timestamptz') { if (typeof value !== 'string') { throw new TableQueryValidationError( - `Range operator on column "${field}" (date) requires a date string, got ${typeof value}` + `Range operator on column "${label}" (date) requires a date string, got ${typeof value}` ) } if (normalizeDateCellValue(value) === null) { throw new TableQueryValidationError( - `Range operator on column "${field}" (date) requires a parseable date string, got "${truncate(value, 64)}"` + `Range operator on column "${label}" (date) requires a parseable date string, got "${truncate(value, 64)}"` ) } } } +/** + * Equality/membership operators. Their operand is compared by JSONB + * containment, which is exact and untyped: `{"score": 8} @> {"score": "8"}` is + * simply false, so a wrongly-typed operand never matches and the caller cannot + * tell that from a genuinely empty table. + */ +const CONTAINMENT_OPS = new Set(['eq', 'ne', 'in', 'nin']) + +/** + * Column types whose containment operand is left byte-exact. + * + * `select` — its operands are option **names**, already resolved to stored ids + * upstream by `resolvePredicateSelectValues` / `resolveFilterSelectValues`, and + * its `coerce` returns an array for a multi-select: the wrong shape for a + * membership clause. + * + * `date` — its `coerce` is **not idempotent**. `normalizeDateCellValue` rebuilds + * the string without a fractional part, so `"2024-01-31T10:00:00.000Z"` — the + * form the write path stores — comes back as `"2024-01-31T10:00:00Z"` and no + * longer matches the stored bytes. `fieldPredicate` is not only used for + * user-facing filters: it also compiles the unique-constraint probes + * (`checkUniqueConstraintsDb`, `checkBatchUniqueConstraintsDb`) and the upsert + * conflict probe, whose operands were **already** coerced by `coerceRowToSchema` + * earlier in the same request. Re-coercing them there would make a unique `date` + * probe stop matching, letting a duplicate row through inside the write + * transaction with no error. Fixing the stored date format is a far larger + * change than a read-path alignment should carry. + */ +const CONTAINMENT_COERCION_EXCLUDED_TYPES = new Set(['select', 'date']) + +/** + * Reads an equality/membership operand the way the **write path** reads a cell, + * so `eq` compares like against like — best-effort, never fatal. + * + * The column type's own `coerce` is the single definition of "what this column + * can hold": a write of `"8"` to a number column stores `8`, so a filter for + * `"8"` must look for `8` or it reports zero rows for a row that exists. + * + * When `coerce` refuses, the ORIGINAL operand is passed through unchanged and + * the clause compiles exactly as it always did — matching nothing, since JSONB + * containment is exact. Refusing loudly is not an option here: the v2 predicate + * grammar is not operand-type-checked at the boundary (leaf `value` is + * `z.unknown()`), so a throw would land not at submission but inside the + * background runners that compile the same predicate later — a filter-scoped + * cancel that can no longer compile would leave those cells uncancellable. + * + * `null` and `''` are passed through untouched. Neither is a typed operand: + * `null` is a real containment query for a JSON-null cell, and `''` is the + * cleared-cell sentinel the grid writes. Coercing either would change what an + * existing caller's filter means rather than fix it. `select` and `date` are + * excluded wholesale — see `CONTAINMENT_COERCION_EXCLUDED_TYPES`. + */ +function coerceContainmentOperand(column: ColumnDefinition, value: JsonValue): JsonValue { + if (value === null || value === '') return value + const result = columnTypeOf(column).coerce(value, column) + return result.ok ? (result.value as JsonValue) : value +} + /** * Guards a bound that is about to be bound into a `::timestamptz` cast on a * system timestamp column (`createdAt`/`updatedAt`). @@ -430,6 +502,7 @@ function buildFieldCondition( const columnType = column?.type const isSelect = columnType === 'select' const isMultiSelect = isSelect && column?.multiple === true + const label = columnLabel(field, column) // Types whose stored value is opaque (a select's option ids) restrict which // operators mean anything; `null` means the type accepts them all. const allowedOperators = column ? filterOperatorsFor(column) : null @@ -442,13 +515,13 @@ function buildFieldCondition( validateOperator(op) if (allowedOperators && !allowedOperators.has(op)) { throw new TableQueryValidationError( - `Operator "${op}" is not supported on ${isMultiSelect ? 'multi-select' : columnType} column "${field}". Allowed: ${Array.from(allowedOperators).join(', ')}` + `Operator "${op}" is not supported on ${isMultiSelect ? 'multi-select' : columnType} column "${label}". Allowed: ${Array.from(allowedOperators).join(', ')}` ) } if (op === '$empty') { // `$empty: true/false` maps onto the valueless v2 ops. - const filterOp: FilterOp = coerceEmptyFlag(field, value) ? 'isEmpty' : 'isNotEmpty' + const filterOp: FilterOp = coerceEmptyFlag(label, value) ? 'isEmpty' : 'isNotEmpty' const clause = fieldPredicate(tableName, field, filterOp, undefined, column) if (clause) conditions.push(clause) continue @@ -511,6 +584,10 @@ export function fieldPredicate( } const columnType = column?.type + // Messages must name what the CALLER sent. `field` is the storage key by the + // time it reaches here (the boundaries translate name → id before building + // SQL), so a raw `field` reports a `col_…` the caller never supplied. + const label = columnLabel(field, column) const isSelect = columnType === 'select' // A multi-select cell holds an ARRAY of option ids, so equality against a // scalar can never be true; the question is membership. Gating and clause @@ -522,7 +599,7 @@ export function fieldPredicate( const allowed = isMultiSelect ? MULTI_SELECT_OPS : SINGLE_SELECT_OPS if (!allowed.has(op)) { throw new TableQueryValidationError( - `Operator "${op}" is not supported on ${isMultiSelect ? 'multi-select' : 'select'} column "${field}". Allowed: ${Array.from(allowed).join(', ')}` + `Operator "${op}" is not supported on ${isMultiSelect ? 'multi-select' : 'select'} column "${label}". Allowed: ${Array.from(allowed).join(', ')}` ) } } @@ -542,45 +619,66 @@ export function fieldPredicate( } } + // Equality/membership compiles to exact JSONB containment, so an operand of + // the wrong JS type is not a narrower match — it is no match at all, reported + // as an empty 200 while the row it meant exists. Read the operand through the + // column type first, exactly as a write would. Best-effort only: an operand + // the type refuses passes through unchanged and the clause compiles as it + // always did. Skipped for a field with no schema entry (ad-hoc legacy keys), + // which has no declared type to read it with, and for the types in + // `CONTAINMENT_COERCION_EXCLUDED_TYPES`. + const coercesContainment = + column !== undefined && + !CONTAINMENT_COERCION_EXCLUDED_TYPES.has(column.type) && + CONTAINMENT_OPS.has(op) + const containmentValue: JsonValue | undefined = + coercesContainment && column + ? Array.isArray(value) + ? value.map((v) => coerceContainmentOperand(column, v as JsonValue)) + : coerceContainmentOperand(column, value as JsonValue) + : value + switch (op) { case 'eq': - return buildContainmentClause(tableName, field, value as JsonValue) + return buildContainmentClause(tableName, field, containmentValue as JsonValue) case 'ne': - return sql`NOT (${buildContainmentClause(tableName, field, value as JsonValue)})` + return sql`NOT (${buildContainmentClause(tableName, field, containmentValue as JsonValue)})` case 'gt': - return buildComparisonClause(tableName, field, '>', value as number | string, columnType) + return buildComparisonClause(tableName, field, column, '>', value as number | string) case 'gte': - return buildComparisonClause(tableName, field, '>=', value as number | string, columnType) + return buildComparisonClause(tableName, field, column, '>=', value as number | string) case 'lt': - return buildComparisonClause(tableName, field, '<', value as number | string, columnType) + return buildComparisonClause(tableName, field, column, '<', value as number | string) case 'lte': - return buildComparisonClause(tableName, field, '<=', value as number | string, columnType) + return buildComparisonClause(tableName, field, column, '<=', value as number | string) case 'in': { - if (!Array.isArray(value) || value.length === 0) return undefined - if (value.length === 1) return buildContainmentClause(tableName, field, value[0]) - const inConditions = value.map((v) => buildContainmentClause(tableName, field, v)) + const values = containmentValue + if (!Array.isArray(values) || values.length === 0) return undefined + if (values.length === 1) return buildContainmentClause(tableName, field, values[0]) + const inConditions = values.map((v) => buildContainmentClause(tableName, field, v)) return sql`(${sql.join(inConditions, sql.raw(' OR '))})` } case 'nin': { - if (!Array.isArray(value) || value.length === 0) return undefined - const ninConditions = value.map( + const values = containmentValue + if (!Array.isArray(values) || values.length === 0) return undefined + const ninConditions = values.map( (v) => sql`NOT (${buildContainmentClause(tableName, field, v)})` ) return sql`(${sql.join(ninConditions, sql.raw(' AND '))})` } case 'contains': - return buildLikeClause(tableName, field, value as string, 'contains') + return buildLikeClause(tableName, field, label, value as string, 'contains') case 'ncontains': - return buildLikeClause(tableName, field, value as string, 'contains', { negate: true }) + return buildLikeClause(tableName, field, label, value as string, 'contains', { negate: true }) case 'startsWith': - return buildLikeClause(tableName, field, value as string, 'startsWith') + return buildLikeClause(tableName, field, label, value as string, 'startsWith') case 'endsWith': - return buildLikeClause(tableName, field, value as string, 'endsWith') + return buildLikeClause(tableName, field, label, value as string, 'endsWith') case 'like': return buildPatternClause(tableName, field, value as string, { caseInsensitive: false }) @@ -813,15 +911,17 @@ function buildArrayMembershipClause(tableName: string, field: string, value: Jso function buildComparisonClause( tableName: string, field: string, + column: ColumnDefinition | undefined, operator: '>' | '>=' | '<' | '<=', - value: number | string, - columnType: ColumnType | undefined + value: number | string ): SQL { const escapedField = field.replace(/'/g, "''") + const label = columnLabel(field, column) + const columnType = column?.type if (columnType === 'boolean' || columnType === 'json') { throw new TableQueryValidationError( - `Range operator on column "${field}" (${columnType}) is not supported — ${columnType} values have no ordering.` + `Range operator on column "${label}" (${columnType}) is not supported — ${columnType} values have no ordering.` ) } @@ -831,7 +931,7 @@ function buildComparisonClause( } const cast = jsonbCastForType(columnType) ?? 'numeric' - validateComparisonValue(field, columnType, cast, value) + validateComparisonValue(label, columnType, cast, value) const cell = sql.raw(`(${tableName}.data->>'${escapedField}')::${cast}`) return cast === 'timestamptz' ? sql`${cell} ${sql.raw(operator)} ${value}::timestamptz` @@ -884,6 +984,7 @@ function buildPatternClause( function buildLikeClause( tableName: string, field: string, + label: string, value: string, position: 'contains' | 'startsWith' | 'endsWith', options?: { negate?: boolean } @@ -898,7 +999,7 @@ function buildLikeClause( if (text.length === 0) { const opName = position === 'contains' && options?.negate ? 'ncontains' : position throw new TableQueryValidationError( - `$${opName} on column "${field}" requires a non-empty value` + `$${opName} on column "${label}" requires a non-empty value` ) } const escaped = escapeLikePattern(text) @@ -920,12 +1021,12 @@ function buildLikeClause( * else throws rather than silently inverting the check — a 400 with a clear * message beats returning the opposite row set. */ -function coerceEmptyFlag(field: string, value: unknown): boolean { +function coerceEmptyFlag(label: string, value: unknown): boolean { if (typeof value === 'boolean') return value if (value === 'true') return true if (value === 'false') return false throw new TableQueryValidationError( - `$empty on column "${field}" requires a boolean, got ${typeof value}` + `$empty on column "${label}" requires a boolean, got ${typeof value}` ) } diff --git a/apps/sim/lib/table/workflow-group-cancellation.test.ts b/apps/sim/lib/table/workflow-group-cancellation.test.ts index 6514827e2ea..92e400d27e8 100644 --- a/apps/sim/lib/table/workflow-group-cancellation.test.ts +++ b/apps/sim/lib/table/workflow-group-cancellation.test.ts @@ -23,6 +23,8 @@ const OPTIONS = { executionId: 'execution-1', } +const NO_WRITES = { workflowLogTerminalized: false, sidecarCancelled: false } as const + const ACTIVE_TARGET = { tableId: 'table-1', rowId: 'row-1', @@ -64,6 +66,7 @@ describe('cancelWorkflowGroupExecution', () => { rowId: 'row-1', groupId: 'group-1', blockErrors: { 'block-1': 'Provider failed' }, + writes: { workflowLogTerminalized: true, sidecarCancelled: true }, }) expect(dbChainMockFns.transaction).toHaveBeenCalledOnce() @@ -165,6 +168,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'not_workflow_group', + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -178,6 +182,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'not_workflow_group', + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -197,6 +202,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'not_workflow_group', + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -216,6 +222,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'cancelled_without_sidecar', + writes: { workflowLogTerminalized: true, sidecarCancelled: false }, }) expect(dbChainMockFns.update).toHaveBeenCalledOnce() @@ -249,6 +256,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'already_cancelled_without_sidecar', + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -269,6 +277,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'not_workflow_group', + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -288,6 +297,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'conflict', status: 'completed', + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -318,6 +328,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'conflict', status: 'completed', + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -335,6 +346,7 @@ describe('cancelWorkflowGroupExecution', () => { rowId: 'row-1', groupId: 'group-1', blockErrors: { 'block-1': 'Provider failed' }, + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -353,6 +365,7 @@ describe('cancelWorkflowGroupExecution', () => { rowId: 'row-1', groupId: 'group-1', blockErrors: { 'block-1': 'Provider failed' }, + writes: { workflowLogTerminalized: false, sidecarCancelled: true }, }) expect(dbChainMockFns.update).toHaveBeenCalledOnce() @@ -371,6 +384,7 @@ describe('cancelWorkflowGroupExecution', () => { rowId: 'row-1', groupId: 'group-1', blockErrors: { 'block-1': 'Provider failed' }, + writes: { workflowLogTerminalized: false, sidecarCancelled: true }, }) const sidecarUpdateValues = collectConditionValues(dbChainMockFns.where.mock.calls[2]?.[0]) @@ -395,6 +409,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'conflict', status: 'error', + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() @@ -413,6 +428,7 @@ describe('cancelWorkflowGroupExecution', () => { rowId: 'row-1', groupId: 'group-1', blockErrors: { 'block-1': 'Provider failed' }, + writes: { workflowLogTerminalized: true, sidecarCancelled: false }, }) expect(dbChainMockFns.update).toHaveBeenCalledOnce() @@ -429,6 +445,7 @@ describe('cancelWorkflowGroupExecution', () => { await expect(cancelWorkflowGroupExecution(OPTIONS)).resolves.toEqual({ kind: 'conflict', status, + writes: NO_WRITES, }) expect(dbChainMockFns.update).not.toHaveBeenCalled() diff --git a/apps/sim/lib/table/workflow-group-cancellation.ts b/apps/sim/lib/table/workflow-group-cancellation.ts index 1231cc2b43a..fd739d3aefa 100644 --- a/apps/sim/lib/table/workflow-group-cancellation.ts +++ b/apps/sim/lib/table/workflow-group-cancellation.ts @@ -35,13 +35,28 @@ export type PublishableWorkflowGroupCancellation = | CancelledWorkflowGroupExecution | AlreadyCancelledWorkflowGroupExecution +/** + * The durable terminal writes this request's own transaction performed. Each + * flag is read off that statement's returned row, so it cannot drift from the + * write: the transaction updates the workflow log only, the cell sidecar only, + * or both, and `kind` alone collapses those cases. + */ +export interface WorkflowGroupCancellationWrites { + /** This transaction moved the workflow execution log to `cancelled`. */ + workflowLogTerminalized: boolean + /** This transaction moved the table cell sidecar to `cancelled`. */ + sidecarCancelled: boolean +} + +type WithCancellationWrites = TResult & { writes: WorkflowGroupCancellationWrites } + export type WorkflowGroupExecutionCancellationResult = - | { kind: 'not_workflow_group' } - | { kind: 'conflict'; status: string } - | { kind: 'cancelled_without_sidecar' } - | { kind: 'already_cancelled_without_sidecar' } - | CancelledWorkflowGroupExecution - | AlreadyCancelledWorkflowGroupExecution + | WithCancellationWrites<{ kind: 'not_workflow_group' }> + | WithCancellationWrites<{ kind: 'conflict'; status: string }> + | WithCancellationWrites<{ kind: 'cancelled_without_sidecar' }> + | WithCancellationWrites<{ kind: 'already_cancelled_without_sidecar' }> + | WithCancellationWrites + | WithCancellationWrites interface WorkflowGroupExecutionTarget { tableId: string @@ -90,11 +105,19 @@ function hasDurableWorkflowGroupOrigin(executionData: unknown): boolean { * * This helper only claims durable database state. Publish its `cancelled` result after * the exact execution has been signalled with `publishWorkflowGroupCancellationEvent`. + * + * Every result carries `writes`, the terminal writes this transaction actually + * performed, so a caller reporting durability never has to infer it from `kind`. */ export async function cancelWorkflowGroupExecution( options: CancelWorkflowGroupExecutionOptions ): Promise { const transition = await db.transaction(async (tx) => { + const writes: WorkflowGroupCancellationWrites = { + workflowLogTerminalized: false, + sidecarCancelled: false, + } + const workflowLog = await tx .select({ status: workflowExecutionLogs.status, @@ -134,20 +157,20 @@ export async function cancelWorkflowGroupExecution( .then((rows) => rows[0]) if (!workflowLog) { - return { result: { kind: 'conflict', status: 'no_longer_active' } as const } + return { result: { kind: 'conflict', status: 'no_longer_active' } as const, writes } } if (!target) { if (!hasDurableWorkflowGroupOrigin(workflowLog.executionData)) { - return { result: { kind: 'not_workflow_group' } as const } + return { result: { kind: 'not_workflow_group' } as const, writes } } const workflowLogActive = workflowLog.status === 'running' || workflowLog.status === 'pending' if (!workflowLogActive && workflowLog.status !== 'cancelled') { - return { result: { kind: 'conflict', status: workflowLog.status } as const } + return { result: { kind: 'conflict', status: workflowLog.status } as const, writes } } if (workflowLog.status === 'cancelled') { - return { result: { kind: 'already_cancelled_without_sidecar' } as const } + return { result: { kind: 'already_cancelled_without_sidecar' } as const, writes } } const cancelledAt = new Date() @@ -164,10 +187,11 @@ export async function cancelWorkflowGroupExecution( ) .returning({ status: workflowExecutionLogs.status }) - if (cancelledLog?.status !== 'cancelled') { + writes.workflowLogTerminalized = cancelledLog?.status === 'cancelled' + if (!writes.workflowLogTerminalized) { throw new Error('Workflow-group cancellation lost its locked workflow-log claim') } - return { result: { kind: 'cancelled_without_sidecar' } as const } + return { result: { kind: 'cancelled_without_sidecar' } as const, writes } } const workflowLogActive = workflowLog.status === 'running' || workflowLog.status === 'pending' @@ -179,10 +203,10 @@ export async function cancelWorkflowGroupExecution( const sidecarClaimable = sidecarActive || cancellationOwnedSidecarError if (!workflowLogActive && workflowLog.status !== 'cancelled') { - return { result: { kind: 'conflict', status: workflowLog.status } as const } + return { result: { kind: 'conflict', status: workflowLog.status } as const, writes } } if (!sidecarClaimable && target.status !== 'cancelled') { - return { result: { kind: 'conflict', status: target.status } as const } + return { result: { kind: 'conflict', status: target.status } as const, writes } } const now = new Date() @@ -200,7 +224,8 @@ export async function cancelWorkflowGroupExecution( ) .returning({ status: workflowExecutionLogs.status }) - if (cancelledLog?.status !== 'cancelled') { + writes.workflowLogTerminalized = cancelledLog?.status === 'cancelled' + if (!writes.workflowLogTerminalized) { throw new Error('Workflow-group cancellation lost its locked workflow-log claim') } } @@ -231,7 +256,8 @@ export async function cancelWorkflowGroupExecution( ) .returning({ status: tableRowExecutions.status }) - if (cancelledSidecar?.status !== 'cancelled') { + writes.sidecarCancelled = cancelledSidecar?.status === 'cancelled' + if (!writes.sidecarCancelled) { throw new Error('Workflow-group cancellation lost its locked table-sidecar claim') } } @@ -242,16 +268,17 @@ export async function cancelWorkflowGroupExecution( rowId: target.rowId, groupId: target.groupId, } - return { result, blockErrors: target.blockErrors } + return { result, writes, blockErrors: target.blockErrors } }) if (transition.result.kind !== 'cancelled' && transition.result.kind !== 'already_cancelled') { - return transition.result + return { ...transition.result, writes: transition.writes } } const blockErrors = normalizeBlockErrors(transition.blockErrors) return { ...transition.result, + writes: transition.writes, ...(blockErrors ? { blockErrors } : {}), } } diff --git a/apps/sim/lib/workflows/executor/execution-queries.test.ts b/apps/sim/lib/workflows/executor/execution-queries.test.ts index 0b01c5b6f15..52750473ef4 100644 --- a/apps/sim/lib/workflows/executor/execution-queries.test.ts +++ b/apps/sim/lib/workflows/executor/execution-queries.test.ts @@ -1,7 +1,7 @@ /** * @vitest-environment node */ -import { queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing/mocks' +import { dbChainMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing/mocks' import { beforeEach, describe, expect, it, vi } from 'vitest' const { mockGetJob, mockGetJobQueue } = vi.hoisted(() => ({ @@ -63,12 +63,87 @@ describe('resolveWorkflowExecutionOwnership', () => { ).resolves.toMatchObject({ workflowGroupWorkspaceId: null }) }) + /** + * A cancel has to tell a live run apart from one that had already finished, + * and the row it would ask for is the row this query already reads. + */ + it('projects the durable status from the same log row it already reads', async () => { + queueTableRows(schemaMock.workflowExecutionLogs, [ + { workflowId: 'workflow-1', status: 'completed' }, + ]) + + await expect( + resolveWorkflowExecutionOwnership('execution-1', 'workflow-1') + ).resolves.toMatchObject({ belongsToWorkflow: true, priorStatus: 'completed' }) + }) + + /** + * The row-queue mock returns whatever was queued for a table regardless of the + * predicate, so every other test here passes with the `WHERE` deleted. Execution + * ids are globally unique but nothing in the mock enforces that a lookup keyed + * on the wrong column — or on nothing — would fail, and this resolver is what + * every mutating caller trusts to say which workflow an execution belongs to. + */ + it('keys both durable reads on the execution id', async () => { + queueTableRows(schemaMock.workflowExecutionLogs, [{ workflowId: 'workflow-1' }]) + + await resolveWorkflowExecutionOwnership('execution-1', 'workflow-1') + + expect(dbChainMockFns.from).toHaveBeenNthCalledWith(1, schemaMock.workflowExecutionLogs) + expect(dbChainMockFns.from).toHaveBeenNthCalledWith(2, schemaMock.pausedExecutions) + expect(dbChainMockFns.where).toHaveBeenNthCalledWith(1, { + type: 'eq', + left: schemaMock.workflowExecutionLogs.executionId, + right: 'execution-1', + }) + expect(dbChainMockFns.where).toHaveBeenNthCalledWith(2, { + type: 'eq', + left: schemaMock.pausedExecutions.executionId, + right: 'execution-1', + }) + }) + + /** + * A run that paused before its log row landed — or whose log row is gone — is + * still durable, and the paused row's workflow id is the only thing standing + * between it and the queue fallback, which would answer `false` for a run the + * queue no longer holds. Nothing else here queues a `pausedExecutions` row, so + * dropping it from the ownership decision is otherwise invisible. + */ + it('resolves ownership from a paused-only execution', async () => { + queueTableRows(schemaMock.workflowExecutionLogs, []) + queueTableRows(schemaMock.pausedExecutions, [{ workflowId: 'workflow-1' }]) + + await expect( + resolveWorkflowExecutionOwnership('execution-1', 'workflow-1') + ).resolves.toMatchObject({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: null, + }) + expect(mockGetJobQueue).not.toHaveBeenCalled() + }) + + it('rejects a paused-only execution bound to another workflow', async () => { + queueTableRows(schemaMock.workflowExecutionLogs, []) + queueTableRows(schemaMock.pausedExecutions, [{ workflowId: 'workflow-2' }]) + + await expect( + resolveWorkflowExecutionOwnership('execution-1', 'workflow-1') + ).resolves.toMatchObject({ belongsToWorkflow: false }) + expect(mockGetJobQueue).not.toHaveBeenCalled() + }) + it('checks deterministic queue metadata before the durable log exists', async () => { mockGetJob.mockResolvedValue({ metadata: { workflowId: 'workflow-1' } }) await expect( resolveWorkflowExecutionOwnership('execution-1', 'workflow-1') - ).resolves.toMatchObject({ belongsToWorkflow: true, workflowGroupWorkspaceId: null }) + ).resolves.toMatchObject({ + belongsToWorkflow: true, + workflowGroupWorkspaceId: null, + priorStatus: null, + }) expect(mockGetJob).toHaveBeenCalledWith('workflow-execution:execution-1') }) }) diff --git a/apps/sim/lib/workflows/executor/execution-queries.ts b/apps/sim/lib/workflows/executor/execution-queries.ts index 9c221871f79..6c150192eae 100644 --- a/apps/sim/lib/workflows/executor/execution-queries.ts +++ b/apps/sim/lib/workflows/executor/execution-queries.ts @@ -104,6 +104,11 @@ export interface WorkflowExecutionOwnership { * that has no log row yet, and a paused-only run. */ workflowGroupWorkspaceId: string | null + /** + * Status the durable log row already carried. `null` when there is no log row + * — a queue-only run, or a paused-only run. + */ + priorStatus: string | null } /** @@ -112,10 +117,12 @@ export interface WorkflowExecutionOwnership { * operating on an execution id because execution ids are globally unique, not * nested DB keys under a workflow. * - * The workflow-group origin rides along on the same log row the ownership check - * already reads. A group run owns a table cell sidecar, so cancelling only the - * workflow log would leave the cell stuck as running — and resolving that from a - * second SELECT of the identical row would double the read on every cancel. + * The workflow-group origin and the row's current status both ride along on the + * same log row the ownership check already reads. A group run owns a table cell + * sidecar, so cancelling only the workflow log would leave the cell stuck as + * running; and a cancel has to tell a live run apart from one that had already + * finished. Resolving either from a second SELECT of the identical row would + * double the read on every cancel. */ export async function resolveWorkflowExecutionOwnership( executionId: string, @@ -126,6 +133,7 @@ export async function resolveWorkflowExecutionOwnership( .select({ workflowId: workflowExecutionLogs.workflowId, workspaceId: workflowExecutionLogs.workspaceId, + status: workflowExecutionLogs.status, executionOrigin: workflowExecutionOriginSql(), }) .from(workflowExecutionLogs) @@ -149,6 +157,7 @@ export async function resolveWorkflowExecutionOwnership( return { belongsToWorkflow: durableWorkflowIds.every((value) => value === workflowId), workflowGroupWorkspaceId, + priorStatus: logRow?.status ?? null, } } @@ -157,5 +166,6 @@ export async function resolveWorkflowExecutionOwnership( return { belongsToWorkflow: job?.metadata.workflowId === workflowId, workflowGroupWorkspaceId: null, + priorStatus: null, } } diff --git a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts index 5754349fc63..aaf109c3cc0 100644 --- a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts +++ b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.test.ts @@ -12,6 +12,7 @@ import { import { beforeEach, describe, expect, it, vi } from 'vitest' import { createTimeoutAbortController, getExecutionDeadlineAt } from '@/lib/core/execution-limits' import { abortManualExecution } from '@/lib/execution/manual-cancellation' +import { terminalExecutionLogFields } from '@/lib/logs/execution/cancellation' const { mockReleaseExecutionSlot, mockReplaceLargeValueReferenceKeysWithClient } = vi.hoisted( () => ({ @@ -1533,15 +1534,56 @@ describe('PauseResumeManager blocked resume readmission', () => { }) }) -describe('PauseResumeManager completed resume transitions', () => { +describe('PauseResumeManager terminal resume failure', () => { beforeEach(() => { vi.clearAllMocks() resetDbChainMock() }) - it('clears the active attempt deadline when sibling pause points remain', async () => { + async function markResumeFailed(): Promise { + const managerInternals = PauseResumeManager as unknown as PauseResumeManagerInternals + await managerInternals.markResumeFailed({ + resumeEntryId: 'resume-entry-1', + pausedExecutionId: 'paused-exec-1', + parentExecutionId: 'execution-1', + contextId: 'context-1', + failureReason: 'Resume execution failed', + }) + } + + it('terminalizes the parent log: end timestamp, derived duration, deadline cleared', async () => { queueTableRows(workflowExecutionLogs, [{ status: 'running' }]) queueTableRows(pausedExecutions, [{ status: 'paused' }]) + + await markResumeFailed() + + const logUpdate = dbChainMockFns.set.mock.calls.at(-1)?.[0] as { + status: string + endedAt: Date + totalDurationMs: unknown + executionDeadlineAt: Date | null + } + expect(logUpdate.status).toBe('failed') + expect(logUpdate.endedAt).toBeInstanceOf(Date) + expect(logUpdate.executionDeadlineAt).toBeNull() + expect(JSON.stringify(logUpdate.totalDurationMs)).toContain(logUpdate.endedAt.toISOString()) + }) +}) + +describe('PauseResumeManager completed resume transitions', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + }) + + interface MockSqlFragment { + values: unknown[] + toSQL: () => { sql: string } + } + + async function markPartialResumeCompleted(logStatus: string): Promise { + queueTableRows(workflowExecutionLogs, [{ status: logStatus }]) + queueTableRows(pausedExecutions, [{ status: 'paused' }]) queueTableRows(resumeQueue, [{ status: 'claimed' }]) queueTableRows(pausedExecutions, [{ remaining: 1 }]) const markResumeCompleted = Reflect.get(PauseResumeManager, 'markResumeCompleted') as (args: { @@ -1557,16 +1599,101 @@ describe('PauseResumeManager completed resume transitions', () => { parentExecutionId: 'execution-1', contextId: 'context-1', }) + } - expect(dbChainMockFns.set).toHaveBeenNthCalledWith(3, { - status: 'pending', - executionDeadlineAt: null, - }) + /** The revival is the third write: resume queue, paused execution, then the log. */ + function revivalPayload(): { + status: string + executionDeadlineAt: Date | null + endedAt: MockSqlFragment + totalDurationMs: MockSqlFragment + } { + return dbChainMockFns.set.mock.calls.at(-1)?.[0] + } + + it('clears the active attempt deadline when sibling pause points remain', async () => { + await markPartialResumeCompleted('running') + + expect(dbChainMockFns.set).toHaveBeenCalledTimes(3) + const revival = revivalPayload() + expect(revival.status).toBe('pending') + expect(revival.executionDeadlineAt).toBeNull() expect(dbChainMockFns.from).toHaveBeenNthCalledWith(1, workflowExecutionLogs) expect(dbChainMockFns.from).toHaveBeenNthCalledWith(2, pausedExecutions) expect(dbChainMockFns.from).toHaveBeenNthCalledWith(3, resumeQueue) }) + /** + * The revival claim excludes only `cancelled`, so it also matches a row + * `markResumeFailed` already ended: one context's resume fails, a sibling + * context resumes successfully afterwards, and the run goes live again. It + * must not go live still carrying the end timestamp and duration of the + * attempt that failed — a run waiting on its remaining pause points has not + * ended, and reporting that it has puts it in the `minDurationMs`/ + * `maxDurationMs` filters on `GET /api/v2/logs` with a duration measured at + * something other than its own end. + */ + it('clears the terminal stamp when a partial resume revives a force-failed row', async () => { + await markPartialResumeCompleted('failed') + + const revival = revivalPayload() + expect(Object.keys(revival).sort()).toEqual([ + 'endedAt', + 'executionDeadlineAt', + 'status', + 'totalDurationMs', + ]) + expect(revival.status).toBe('pending') + expect(revival.endedAt.toSQL().sql).toContain("IN ('failed', 'completed') THEN NULL") + expect(revival.totalDurationMs.toSQL().sql).toContain("IN ('failed', 'completed') THEN NULL") + expect(revival.endedAt.values[0]).toBe(workflowExecutionLogs.status) + expect(revival.totalDurationMs.values[0]).toBe(workflowExecutionLogs.status) + }) + + /** + * The opposite case, and the reason the clear is conditional rather than + * unconditional: a row revived from a non-terminal status carries the + * checkpoint `completeWithPause` banked, which is the active duration + * `elapsedDurationMsSql` deliberately preserves for a `pending` row. Nulling + * that would redefine a later terminal duration to include the time the run + * sat waiting. + */ + it('keeps the checkpoint a still-live row banked at its pause', async () => { + await markPartialResumeCompleted('running') + + const revival = revivalPayload() + expect(revival.endedAt.toSQL().sql).toContain('ELSE ?') + expect(revival.totalDurationMs.toSQL().sql).toContain('ELSE ?') + expect(revival.endedAt.values.at(-1)).toBe(workflowExecutionLogs.endedAt) + expect(revival.totalDurationMs.values.at(-1)).toBe(workflowExecutionLogs.totalDurationMs) + }) + + /** + * The compounding half. `elapsedDurationMsSql` preserves a `pending` row's + * `total_duration_ms` and recomputes otherwise, so whatever the revival leaves + * behind is what the next terminal write — a cancel, say — records as the run's + * duration. Leaving the failed resume's frozen value there would freeze the + * cancel at it; leaving `NULL` is what makes the `COALESCE` fall through to the + * elapsed computation. + * + * The link is asserted as a composition rather than executed: the repository + * has no in-memory Postgres, and under the drizzle mock a fragment renders its + * interpolations as `?` with the bound columns on `values`. + */ + it('lets the next terminal write recompute rather than preserve the failed resume duration', async () => { + await markPartialResumeCompleted('failed') + + const revived = revivalPayload().totalDurationMs + expect(revived.toSQL().sql).toContain("IN ('failed', 'completed') THEN NULL") + + const nextTerminalWrite = terminalExecutionLogFields( + 'cancelled', + new Date('2026-08-14T12:00:00.000Z') + ).totalDurationMs as unknown as MockSqlFragment + expect(nextTerminalWrite.toSQL().sql).toContain("= 'pending' THEN ?") + expect(nextTerminalWrite.values).toContain(workflowExecutionLogs.totalDurationMs) + }) + it('fails a claimed resume after cancellation wins the log lock', async () => { queueTableRows(workflowExecutionLogs, [{ status: 'cancelled' }]) queueTableRows(pausedExecutions, [{ status: 'cancelled' }]) diff --git a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts index 48029178fdd..d6c51041c17 100644 --- a/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts +++ b/apps/sim/lib/workflows/executor/human-in-the-loop-manager.ts @@ -33,7 +33,10 @@ import { } from '@/lib/execution/payloads/large-value-metadata' import { compactBlockLogs, compactExecutionPayload } from '@/lib/execution/payloads/serializer' import { preprocessExecution } from '@/lib/execution/preprocessing' -import { cancelledExecutionLogFields } from '@/lib/logs/execution/cancellation' +import { + cancelledExecutionLogFields, + terminalExecutionLogFields, +} from '@/lib/logs/execution/cancellation' import { LoggingSession } from '@/lib/logs/execution/logging-session' import { cleanupExecutionBase64Cache } from '@/lib/uploads/utils/user-file-base64.server' import { executeWorkflowCore } from '@/lib/workflows/executor/execution-core' @@ -249,6 +252,32 @@ function clearAutomaticResumeWaitingMetadataSql(contextId: string): SQL { END` } +/** + * The terminal columns a `workflow_execution_logs` row keeps when a partial + * resume moves it back to `pending`. + * + * The revival claim excludes only `cancelled`, so it also matches a row + * `markResumeFailed` already ended: one context's resume fails, a sibling + * context resumes successfully afterwards, and the run becomes live again + * carrying the end timestamp and duration of the attempt that failed. A live row + * must not carry a terminal stamp — and because `elapsedDurationMsSql` preserves + * a `pending` row's `total_duration_ms` as its pause checkpoint, leaving it + * there also hands the next terminal write a duration frozen at the failed + * resume rather than one it recomputes. + * + * A row revived from a non-terminal status is the opposite case: its columns are + * the checkpoint `completeWithPause` banked, which is precisely what that + * preservation rule exists to keep, so they survive untouched. The row's own + * status decides, read — like every expression in the same `SET` — against the + * pre-update row. + */ +const revivedExecutionLogStamp = { + endedAt: sql`CASE WHEN ${workflowExecutionLogs.status} IN ('failed', 'completed') THEN NULL ELSE ${workflowExecutionLogs.endedAt} END`, + totalDurationMs: sql< + number | null + >`CASE WHEN ${workflowExecutionLogs.status} IN ('failed', 'completed') THEN NULL ELSE ${workflowExecutionLogs.totalDurationMs} END`, +} + function withoutAutomaticResumeWaitingReason( point: Record ): Record { @@ -2104,7 +2133,7 @@ export class PauseResumeManager { } else { await tx .update(workflowExecutionLogs) - .set({ status: 'pending', executionDeadlineAt: null }) + .set({ status: 'pending', executionDeadlineAt: null, ...revivedExecutionLogStamp }) .where( and( eq(workflowExecutionLogs.executionId, targetParentExecutionId), @@ -2173,7 +2202,7 @@ export class PauseResumeManager { await tx .update(workflowExecutionLogs) - .set({ status: 'failed' }) + .set(terminalExecutionLogFields('failed', now)) .where( and( eq(workflowExecutionLogs.executionId, args.parentExecutionId), diff --git a/apps/sim/lib/workspace-files/application/read-workspace-file-metadata.test.ts b/apps/sim/lib/workspace-files/application/read-workspace-file-metadata.test.ts index 4d29c4211c5..3214cdf11f4 100644 --- a/apps/sim/lib/workspace-files/application/read-workspace-file-metadata.test.ts +++ b/apps/sim/lib/workspace-files/application/read-workspace-file-metadata.test.ts @@ -24,6 +24,7 @@ vi.mock('@/lib/public-shares/share-manager', () => ({ getShareForResource: mocks.getShareForResource, })) +import { NoWorkspaceAccessError } from '@/lib/core/application' import { readWorkspaceFileMetadata } from '@/lib/workspace-files/application/read-workspace-file-metadata' const canonical = { @@ -91,6 +92,61 @@ describe('readWorkspaceFileMetadata', () => { expect(mocks.getShareForResource).toHaveBeenCalledWith('file', 'file-1') }) + it('resolves a soft-deleted file when the caller opts into the archived lifecycle set', async () => { + const archived = { ...file, deletedAt: new Date('2026-01-03T00:00:00Z') } + mocks.getWorkspaceFile.mockResolvedValueOnce(archived) + + await expect( + readWorkspaceFileMetadata.execute({ + principal: { kind: 'session', userId: 'user-1', sessionId: 'session-1' }, + input: { + fileId: 'file-1', + assertedWorkspaceId: 'workspace-1', + includeDeleted: true, + }, + }) + ).resolves.toEqual({ file: archived, share }) + + expect(mocks.loadContext).toHaveBeenCalledWith('file-1', { includeDeleted: true }) + expect(mocks.getWorkspaceFile).toHaveBeenCalledWith('workspace-1', 'file-1', { + includeDeleted: true, + throwOnError: true, + }) + }) + + it('authorizes an archived read exactly like an active one', async () => { + mocks.resolvePermission.mockResolvedValueOnce(null) + + await expect( + readWorkspaceFileMetadata.execute({ + principal: { kind: 'session', userId: 'outsider', sessionId: 'session-2' }, + input: { + fileId: 'file-1', + assertedWorkspaceId: 'workspace-1', + includeDeleted: true, + }, + }) + ).rejects.toBeInstanceOf(NoWorkspaceAccessError) + + expect(mocks.getWorkspaceFile).not.toHaveBeenCalled() + expect(mocks.getShareForResource).not.toHaveBeenCalled() + }) + + it('still refuses an archived read that asserts the wrong workspace', async () => { + await expect( + readWorkspaceFileMetadata.execute({ + principal: { kind: 'session', userId: 'user-1', sessionId: 'session-1' }, + input: { + fileId: 'file-1', + assertedWorkspaceId: 'workspace-2', + includeDeleted: true, + }, + }) + ).rejects.toMatchObject({ code: 'not_found' }) + + expect(mocks.getWorkspaceFile).not.toHaveBeenCalled() + }) + it('fails fast if the authorized file disappears before projection', async () => { mocks.getWorkspaceFile.mockResolvedValueOnce(null) diff --git a/apps/sim/lib/workspace-files/application/read-workspace-file-metadata.ts b/apps/sim/lib/workspace-files/application/read-workspace-file-metadata.ts index 5be9e1c9c99..621d20f1324 100644 --- a/apps/sim/lib/workspace-files/application/read-workspace-file-metadata.ts +++ b/apps/sim/lib/workspace-files/application/read-workspace-file-metadata.ts @@ -14,6 +14,12 @@ import { resolveActiveWorkspaceFileContext } from '@/lib/workspace-files/applica export interface ReadWorkspaceFileMetadataInput { fileId: string assertedWorkspaceId?: string + /** + * Opt into the archived lifecycle set. It relaxes only the `deleted_at` predicate on the + * canonical row lookup — the workspace the file resolves to, the asserted-workspace check, + * and the `files.read_metadata` authorization that follows are identical either way, so it + * never widens who may read a file. + */ includeDeleted?: boolean }