From 9ef11a307d3da5f9f90910f9809a682f4f101404 Mon Sep 17 00:00:00 2001 From: Matt Date: Mon, 8 Jun 2026 16:43:38 +0200 Subject: [PATCH 1/2] refactor(workflow-executor): own record not-found handling in the port [PRD-450] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keep @forestadmin/agent-client's Collection#getOne a thin by-id passthrough and move the not-found semantics into AgentClientAgentPort, which owns RecordNotFoundError: - getOne no longer swallows errors nor normalizes empty bodies — it just GETs by id. - getRecord converts a 404 to RecordNotFoundError, treats a 200 + empty body as not-found, and rethrows any other error (no masking of real failures as "record not found"). Follow-up refactor on top of #1629. Relates to PRD-450. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../agent-client/src/domains/collection.ts | 23 ++++-------- .../src/adapters/agent-client-agent-port.ts | 23 ++++++++---- .../adapters/agent-client-agent-port.test.ts | 35 +++++++++++++++++-- 3 files changed, 56 insertions(+), 25 deletions(-) diff --git a/packages/agent-client/src/domains/collection.ts b/packages/agent-client/src/domains/collection.ts index 8e95c0a1d1..7438caafb0 100644 --- a/packages/agent-client/src/domains/collection.ts +++ b/packages/agent-client/src/domains/collection.ts @@ -171,23 +171,12 @@ export default class Collection extends CollectionChart { }); } - // Fetch a single record by its (possibly composite) id via the agent's by-id route. The id is - // serialized opaquely (serializeRecordId), so the agent — the only party that knows the primary - // key column order — does the matching. Returns null when the record does not exist (404, or a - // 200 with an empty payload, which some agents return for a missing composite-key record). - async getOne(id: RecordId, options?: SelectOptions): Promise { - try { - const record = await this.httpRequester.query({ - method: 'get', - path: `/forest/${this.name}/${serializeRecordId(id)}`, - query: QuerySerializer.serialize(options, this.name), - }); - - return record && Object.keys(record as object).length > 0 ? record : null; - } catch (error) { - if ((this.httpRequester.constructor as typeof HttpRequester).is404Error(error)) return null; - throw error; - } + async getOne(id: RecordId, options?: SelectOptions): Promise { + return this.httpRequester.query({ + method: 'get', + path: `/forest/${this.name}/${serializeRecordId(id)}`, + query: QuerySerializer.serialize(options, this.name), + }); } private getActionInfo( diff --git a/packages/workflow-executor/src/adapters/agent-client-agent-port.ts b/packages/workflow-executor/src/adapters/agent-client-agent-port.ts index 820d32e17e..1da997d4cc 100644 --- a/packages/workflow-executor/src/adapters/agent-client-agent-port.ts +++ b/packages/workflow-executor/src/adapters/agent-client-agent-port.ts @@ -12,7 +12,7 @@ import type { StepUser } from '../types/execution-context'; import type { RecordData } from '../types/validated/collection'; import type { ActionEndpointsByCollection } from '@forestadmin/agent-client'; -import { createRemoteAgentClient } from '@forestadmin/agent-client'; +import { HttpRequester, createRemoteAgentClient } from '@forestadmin/agent-client'; import jsonwebtoken from 'jsonwebtoken'; import { @@ -61,12 +61,23 @@ export default class AgentClientAgentPort implements AgentPort { const client = this.createClient(user); // Fetch by id through the agent's by-id route (like update/delete): the recordId is an // opaque ordered token and the agent — the only party that knows the primary key column - // order — does the matching. No buildPkFilter / primaryKeyFields ordering assumption here. - const record = await client - .collection(collection) - .getOne>(id, { ...(fields?.length && { fields }) }); + // order — does the matching. No primaryKeyFields ordering assumption here. + let record: Record | null; + + try { + record = await client + .collection(collection) + .getOne>(id, { ...(fields?.length && { fields }) }); + } catch (error) { + if (HttpRequester.is404Error(error)) { + throw new RecordNotFoundError(collection, id.join('|')); + } + + throw error; + } - if (!record) { + // Some agents answer a missing composite-key record with a 200 + empty body instead of 404. + if (!record || Object.keys(record).length === 0) { throw new RecordNotFoundError(collection, id.join('|')); } diff --git a/packages/workflow-executor/test/adapters/agent-client-agent-port.test.ts b/packages/workflow-executor/test/adapters/agent-client-agent-port.test.ts index 7066aefc0e..28768f4c3d 100644 --- a/packages/workflow-executor/test/adapters/agent-client-agent-port.test.ts +++ b/packages/workflow-executor/test/adapters/agent-client-agent-port.test.ts @@ -1,19 +1,23 @@ import type { StepUser } from '../../src/types/execution-context'; -import { createRemoteAgentClient } from '@forestadmin/agent-client'; +import { HttpRequester, createRemoteAgentClient } from '@forestadmin/agent-client'; import jsonwebtoken from 'jsonwebtoken'; import AgentClientAgentPort from '../../src/adapters/agent-client-agent-port'; -import { AgentProbeError, RecordNotFoundError } from '../../src/errors'; +import { AgentPortError, AgentProbeError, RecordNotFoundError } from '../../src/errors'; import SchemaCache from '../../src/schema-cache'; jest.mock('@forestadmin/agent-client', () => ({ createRemoteAgentClient: jest.fn(), + HttpRequester: { is404Error: jest.fn() }, })); const mockedCreateRemoteAgentClient = createRemoteAgentClient as jest.MockedFunction< typeof createRemoteAgentClient >; +const mockedIs404Error = HttpRequester.is404Error as jest.MockedFunction< + typeof HttpRequester.is404Error +>; function createMockClient() { const mockAction = { execute: jest.fn(), getFields: jest.fn().mockReturnValue([]) }; @@ -132,6 +136,33 @@ describe('AgentClientAgentPort', () => { ); }); + it('maps a 404 from the agent to RecordNotFoundError', async () => { + mockedIs404Error.mockReturnValue(true); + mockCollection.getOne.mockRejectedValue(new Error('not found')); + + await expect(port.getRecord({ collection: 'users', id: [999] }, user)).rejects.toThrow( + RecordNotFoundError, + ); + }); + + it('rethrows non-404 errors instead of masking them as RecordNotFoundError', async () => { + // A 500 / auth / network failure must surface as a real error, not "record not found". + mockedIs404Error.mockReturnValue(false); + mockCollection.getOne.mockRejectedValue(new Error('boom')); + + await expect(port.getRecord({ collection: 'users', id: [1] }, user)).rejects.toThrow( + AgentPortError, + ); + }); + + it('treats a 200 with an empty body as RecordNotFoundError (missing composite record)', async () => { + mockCollection.getOne.mockResolvedValue({}); + + await expect(port.getRecord({ collection: 'orders', id: [1, 2] }, user)).rejects.toThrow( + RecordNotFoundError, + ); + }); + it('should pass fields to getOne when fields is provided', async () => { mockCollection.getOne.mockResolvedValue({ id: 42, name: 'Alice' }); From ce724f4ca1e427d6fd3e6f02506540b3a2530083 Mon Sep 17 00:00:00 2001 From: Matt Date: Mon, 8 Jun 2026 16:57:26 +0200 Subject: [PATCH 2/2] refactor(workflow-executor): format composite id inside RecordNotFoundError Move the `id.join('|')` formatting into RecordNotFoundError so call sites pass the recordId as-is. The constructor accepts a string or a (composite) id array and joins it, removing the repeated join at every throw site. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/adapters/agent-client-agent-port.ts | 4 ++-- packages/workflow-executor/src/errors.ts | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/workflow-executor/src/adapters/agent-client-agent-port.ts b/packages/workflow-executor/src/adapters/agent-client-agent-port.ts index 1da997d4cc..c478813986 100644 --- a/packages/workflow-executor/src/adapters/agent-client-agent-port.ts +++ b/packages/workflow-executor/src/adapters/agent-client-agent-port.ts @@ -70,7 +70,7 @@ export default class AgentClientAgentPort implements AgentPort { .getOne>(id, { ...(fields?.length && { fields }) }); } catch (error) { if (HttpRequester.is404Error(error)) { - throw new RecordNotFoundError(collection, id.join('|')); + throw new RecordNotFoundError(collection, id); } throw error; @@ -78,7 +78,7 @@ export default class AgentClientAgentPort implements AgentPort { // Some agents answer a missing composite-key record with a 200 + empty body instead of 404. if (!record || Object.keys(record).length === 0) { - throw new RecordNotFoundError(collection, id.join('|')); + throw new RecordNotFoundError(collection, id); } return { diff --git a/packages/workflow-executor/src/errors.ts b/packages/workflow-executor/src/errors.ts index 70af3e8abe..4d2119b0bd 100644 --- a/packages/workflow-executor/src/errors.ts +++ b/packages/workflow-executor/src/errors.ts @@ -57,9 +57,11 @@ export class MalformedToolCallError extends WorkflowExecutorError { } export class RecordNotFoundError extends WorkflowExecutorError { - constructor(collectionName: string, recordId: string) { + constructor(collectionName: string, recordId: string | Array) { + const id = Array.isArray(recordId) ? recordId.join('|') : recordId; + super( - `Record not found: collection "${collectionName}", id "${recordId}"`, + `Record not found: collection "${collectionName}", id "${id}"`, 'The record no longer exists. It may have been deleted.', ); }