From 99280ce170ac99e9eed3d7d116c7c5169eabf4a9 Mon Sep 17 00:00:00 2001 From: Anthony Guimard Date: Mon, 14 Sep 2026 11:33:17 +0200 Subject: [PATCH 1/7] fix(agent-bff): order composite key segments by the record's own values Claude-Session: https://claude.ai/code/session_01TrJS9jCkAfmyWQc1PdSEA2 --- packages/agent-bff/src/data/pack-id.ts | 63 ++++++++- .../agent-bff/src/data/response-mappers.ts | 2 +- packages/agent-bff/test/data/pack-id.test.ts | 127 ++++++++++++++++++ 3 files changed, 190 insertions(+), 2 deletions(-) diff --git a/packages/agent-bff/src/data/pack-id.ts b/packages/agent-bff/src/data/pack-id.ts index d7b47960a4..7e50e2e84c 100644 --- a/packages/agent-bff/src/data/pack-id.ts +++ b/packages/agent-bff/src/data/pack-id.ts @@ -1,5 +1,6 @@ import type { PrimaryKeyField } from '../read-model/read-model'; +import recordKey from './record-key'; import { mappingError } from '../http/bff-local-errors'; export const PACKED_ID_SEPARATOR = '|'; @@ -20,6 +21,64 @@ function toNumberIfLossless(value: string): string | number { return Number.isSafeInteger(numeric) && String(numeric) === value ? numeric : value; } +/** + * The value a record carries for a key field, when it can stand for a packed segment. The field is + * read under its own name first, then under the camelCase key the deserializer emits + * (`agent-client/src/http-requester.ts`). Anything that is not a string or a finite number is + * ignored: a record attribute can also be `null`, a boolean, a relation object written over the + * same key, or the JSON of a Buffer, and none of those names a segment. + */ +function comparableValue(record: Record, name: string): string | null { + const declared = record[name]; + const value = declared === undefined || declared === null ? record[recordKey(name)] : declared; + + if (typeof value === 'string') return value; + + return typeof value === 'number' && Number.isFinite(value) ? String(value) : null; +} + +/** + * The packed segments, reordered onto the keys they belong to. + * + * The pairing cannot come from the order the keys arrive in: the apimap sorts its fields + * alphabetically (`agent/src/utils/forest-schema/generator-collection.ts`) while the agent packs in + * declaration order (`agent/src/utils/id.ts`), and the two agree only by accident — a + * `tenant_id`/`seq` collection packs `acme|42` and gets `"acme"` cast as `seq`, so every list of it + * answers 500. + * + * The record settles it. Its attributes hold the key values, so a segment equal to one of them + * belongs to that key whatever the published order. The record is used for THAT and nothing else: + * the value emitted is always the packed segment, never the record's own, because the two forms + * differ on a Date (ISO in the attributes, `String(date)` in the id) and on a Buffer, and because a + * key named `id` reads the whole packed id — `jsonapi-serializer` overwrites that attribute with + * the resource id. Such a key simply matches nothing and takes the segment its siblings left. + * + * A key that matches nothing keeps its positional segment, which is what this function returns + * whole when no record is given. + */ +function segmentsByKey( + values: string[], + primaryKeys: PrimaryKeyField[], + record?: Record, +): string[] { + if (!record) return values; + + const claimed = values.map(() => false); + const matched = primaryKeys.map(({ name }) => { + const wanted = comparableValue(record, name); + const index = wanted === null ? -1 : values.findIndex((v, i) => !claimed[i] && v === wanted); + + if (index === -1) return null; + claimed[index] = true; + + return values[index]; + }); + + const leftovers = values.filter((_, index) => !claimed[index]); + + return matched.map(value => value ?? (leftovers.shift() as string)); +} + /** * Rebuild the structured primary key of a record from its opaque packed id, mirroring the agent's * `IdUtils.packId`/`unpackId` (`|`-joined values, `Number` columns cast back to numbers). Returns a @@ -34,6 +93,7 @@ function toNumberIfLossless(value: string): string | number { export default function unpackPrimaryKey( packedId: string, primaryKeys: PrimaryKeyField[], + record?: Record, ): Record { if (primaryKeys.length === 0) { throw mappingError('Cannot build primary key: the collection exposes no key metadata'); @@ -55,10 +115,11 @@ export default function unpackPrimaryKey( ); } + const segments = segmentsByKey(values, primaryKeys, record); const result: Record = {}; primaryKeys.forEach(({ name, type }, index) => { - const value = values[index]; + const value = segments[index]; if (type !== NUMBER_COLUMN_TYPE) { result[name] = value; diff --git a/packages/agent-bff/src/data/response-mappers.ts b/packages/agent-bff/src/data/response-mappers.ts index f4f48c6e00..2e6b147a83 100644 --- a/packages/agent-bff/src/data/response-mappers.ts +++ b/packages/agent-bff/src/data/response-mappers.ts @@ -34,7 +34,7 @@ export function mapListResponse( ...record, __forest: { collection, - primaryKey: unpackPrimaryKey(String(record.id), primaryKeys), + primaryKey: unpackPrimaryKey(String(record.id), primaryKeys, record), }, }; }); diff --git a/packages/agent-bff/test/data/pack-id.test.ts b/packages/agent-bff/test/data/pack-id.test.ts index 590dc2805b..c707e815da 100644 --- a/packages/agent-bff/test/data/pack-id.test.ts +++ b/packages/agent-bff/test/data/pack-id.test.ts @@ -39,6 +39,133 @@ describe('unpackPrimaryKey', () => { ).toThrow(expect.objectContaining({ type: 'mapping_error', status: 500 })); }); + describe('when the record can say which segment belongs to which key', () => { + it('should place the segments by value rather than by the order the apimap published', () => { + expect( + unpackPrimaryKey( + 'acme|42', + [ + { name: 'seq', type: 'Number' }, + { name: 'tenant_id', type: 'String' }, + ], + { tenantId: 'acme', seq: 42, id: 'acme|42' }, + ), + ).toEqual({ seq: 42, tenant_id: 'acme' }); + }); + + it('should read a key under its exact name when the record carries it unchanged', () => { + expect( + unpackPrimaryKey( + 'ab|7', + [ + { name: 'orderId', type: 'Number' }, + { name: 'sku', type: 'String' }, + ], + { orderId: 7, sku: 'ab' }, + ), + ).toEqual({ orderId: 7, sku: 'ab' }); + }); + + it('should give a key named id the leftover segment, since its attribute holds the packed id', () => { + expect( + unpackPrimaryKey( + 'acme|42', + [ + { name: 'id', type: 'Number' }, + { name: 'tenant', type: 'String' }, + ], + { tenant: 'acme', id: 'acme|42' }, + ), + ).toEqual({ id: 42, tenant: 'acme' }); + }); + + it('should keep the packed value, not the record value, once the segment is placed', () => { + expect( + unpackPrimaryKey( + '042|acme', + [ + { name: 'ref', type: 'String' }, + { name: 'tenant', type: 'String' }, + ], + { ref: '042', tenant: 'acme' }, + ), + ).toEqual({ ref: '042', tenant: 'acme' }); + }); + + it.each([ + ['null', null], + ['a boolean', false], + ['a relation object', { id: 3 }], + ['a buffer payload', { type: 'Buffer', data: [1, 2] }], + ['an array', [1, 2]], + ])('should ignore %s and fall back to the positional segment', (_, value) => { + expect( + unpackPrimaryKey( + '7|ab', + [ + { name: 'orderId', type: 'Number' }, + { name: 'sku', type: 'String' }, + ], + { orderId: value, sku: 'ab' }, + ), + ).toEqual({ orderId: 7, sku: 'ab' }); + }); + + it('should fall back to the positional segments when no key matches', () => { + expect( + unpackPrimaryKey( + '7|ab', + [ + { name: 'orderId', type: 'Number' }, + { name: 'sku', type: 'String' }, + ], + { createdAt: '2026-09-14T00:00:00.000Z' }, + ), + ).toEqual({ orderId: 7, sku: 'ab' }); + }); + + it('should still throw when a positionally assigned Number segment is not numeric', () => { + expect(() => + unpackPrimaryKey( + 'acme|42', + [ + { name: 'seq', type: 'Number' }, + { name: 'tenant_id', type: 'String' }, + ], + {}, + ), + ).toThrow(expect.objectContaining({ type: 'mapping_error', status: 500 })); + }); + + it('should leave a derived key whole, since its column was never declared', () => { + expect( + unpackPrimaryKey('tenant|42', [{ name: 'id', type: 'String', derived: true }], { + id: 'tenant|42', + tenant: 'tenant', + }), + ).toEqual({ id: 'tenant|42' }); + }); + + it('should keep a single numeric key typed', () => { + expect(unpackPrimaryKey('42', [{ name: 'id', type: 'Number' }], { id: '42' })).toEqual({ + id: 42, + }); + }); + + it('should assign one segment to a single key even when two keys share a value', () => { + expect( + unpackPrimaryKey( + 'acme|acme', + [ + { name: 'tenant_id', type: 'String' }, + { name: 'owner_id', type: 'String' }, + ], + { tenantId: 'acme', ownerId: 'acme' }, + ), + ).toEqual({ tenant_id: 'acme', owner_id: 'acme' }); + }); + }); + describe('when the key was derived, so its arity is a guess', () => { it('should keep a packed composite id whole rather than 500 on the segment count', () => { expect( From 5dd7d29cfbb02d4a16a05790c0ea8b6ba9742222 Mon Sep 17 00:00:00 2001 From: Anthony Guimard Date: Mon, 14 Sep 2026 11:42:50 +0200 Subject: [PATCH 2/7] fix(agent-bff): keep an ambiguous record key out of the segment matching Claude-Session: https://claude.ai/code/session_01TrJS9jCkAfmyWQc1PdSEA2 --- packages/agent-bff/src/data/pack-id.ts | 25 ++++++++---- .../agent-bff/src/read-model/read-model.ts | 29 ++++++++++++-- packages/agent-bff/test/data/pack-id.test.ts | 40 +++++++++---------- .../test/data/response-mappers.test.ts | 20 ++++++++++ .../test/read-model/read-model.test.ts | 15 +++++++ 5 files changed, 98 insertions(+), 31 deletions(-) diff --git a/packages/agent-bff/src/data/pack-id.ts b/packages/agent-bff/src/data/pack-id.ts index 7e50e2e84c..7fcb2daf8b 100644 --- a/packages/agent-bff/src/data/pack-id.ts +++ b/packages/agent-bff/src/data/pack-id.ts @@ -27,8 +27,15 @@ function toNumberIfLossless(value: string): string | number { * (`agent-client/src/http-requester.ts`). Anything that is not a string or a finite number is * ignored: a record attribute can also be `null`, a boolean, a relation object written over the * same key, or the JSON of a Buffer, and none of those names a segment. + * + * A key whose response key is shared with another field is refused outright. The record then holds + * a single value under that key and which field wrote it is not knowable here, so reading it could + * claim the segment of a sibling key — worse than not matching at all. */ -function comparableValue(record: Record, name: string): string | null { +function comparableValue(record: Record, key: PrimaryKeyField): string | null { + if (key.ambiguousRecordKey) return null; + + const { name } = key; const declared = record[name]; const value = declared === undefined || declared === null ? record[recordKey(name)] : declared; @@ -47,11 +54,13 @@ function comparableValue(record: Record, name: string): string * answers 500. * * The record settles it. Its attributes hold the key values, so a segment equal to one of them - * belongs to that key whatever the published order. The record is used for THAT and nothing else: - * the value emitted is always the packed segment, never the record's own, because the two forms - * differ on a Date (ISO in the attributes, `String(date)` in the id) and on a Buffer, and because a - * key named `id` reads the whole packed id — `jsonapi-serializer` overwrites that attribute with - * the resource id. Such a key simply matches nothing and takes the segment its siblings left. + * belongs to that key whatever the published order. The record is used for THAT and nothing else — + * the value emitted is always the packed segment, never the record's own. The case that forces it + * is a key named `id`: `jsonapi-serializer` overwrites that attribute with the resource id + * (`deserializer-utils.js`), so the record would hand back `acme|42` for it. Reading the segment + * instead keeps that key correct, and it matches nothing, so it takes what its siblings left. A + * `Date` or a Buffer key is the same story from the other side: their attribute form differs from + * the packed one (ISO versus `String(date)`), they match nothing, and they keep their position. * * A key that matches nothing keeps its positional segment, which is what this function returns * whole when no record is given. @@ -64,8 +73,8 @@ function segmentsByKey( if (!record) return values; const claimed = values.map(() => false); - const matched = primaryKeys.map(({ name }) => { - const wanted = comparableValue(record, name); + const matched = primaryKeys.map(key => { + const wanted = comparableValue(record, key); const index = wanted === null ? -1 : values.findIndex((v, i) => !claimed[i] && v === wanted); if (index === -1) return null; diff --git a/packages/agent-bff/src/read-model/read-model.ts b/packages/agent-bff/src/read-model/read-model.ts index 9d47226ecf..5fc800625f 100644 --- a/packages/agent-bff/src/read-model/read-model.ts +++ b/packages/agent-bff/src/read-model/read-model.ts @@ -1,6 +1,8 @@ import type { ActionEndpointsByCollection } from '@forestadmin/agent-client'; import type { ForestSchemaCollection, ForestSchemaField } from '@forestadmin/forestadmin-client'; +import recordKey, { groupByRecordKey } from '../data/record-key'; + export const RELATIONSHIP_TYPES = [ 'BelongsTo', 'HasOne', @@ -14,7 +16,17 @@ export type RelationTarget = | { type: RelationshipType; polymorphic: false; target: string } | { type: RelationshipType; polymorphic: true; targets: string[] }; -export type PrimaryKeyField = { name: string; type: string; derived?: true }; +export type PrimaryKeyField = { + name: string; + type: string; + derived?: true; + /** + * Set when this key shares its response key with another field of the collection. The record then + * holds one value under that key and there is no telling whose, so the key cannot be read back + * from a record — see `unpackPrimaryKey`. + */ + ambiguousRecordKey?: true; +}; export type ListableRelation = { name: string; foreignCollection: string }; @@ -160,9 +172,20 @@ export default class ReadModel { */ private buildPrimaryKeys(collection: ForestSchemaCollection): void { const keys: PrimaryKeyField[] = []; + const fields = collection.fields ?? []; + const ambiguous = new Set(); - for (const field of collection.fields ?? []) { - if (field.isPrimaryKey) keys.push({ name: field.field, type: field.type }); + groupByRecordKey(fields, field => field.field).forEach((group, key) => { + if (group.length > 1) ambiguous.add(key); + }); + + for (const field of fields) { + if (field.isPrimaryKey) { + const key: PrimaryKeyField = { name: field.field, type: field.type }; + if (ambiguous.has(recordKey(field.field))) key.ambiguousRecordKey = true; + + keys.push(key); + } } if (keys.length === 0) { diff --git a/packages/agent-bff/test/data/pack-id.test.ts b/packages/agent-bff/test/data/pack-id.test.ts index c707e815da..f5daed7cde 100644 --- a/packages/agent-bff/test/data/pack-id.test.ts +++ b/packages/agent-bff/test/data/pack-id.test.ts @@ -79,17 +79,17 @@ describe('unpackPrimaryKey', () => { ).toEqual({ id: 42, tenant: 'acme' }); }); - it('should keep the packed value, not the record value, once the segment is placed', () => { + it('should keep a date key positional, its attribute being ISO where the id is not', () => { expect( unpackPrimaryKey( - '042|acme', + 'Thu Jan 01 2026 00:00:00 GMT+0100|acme', [ - { name: 'ref', type: 'String' }, + { name: 'day', type: 'Date' }, { name: 'tenant', type: 'String' }, ], - { ref: '042', tenant: 'acme' }, + { day: '2026-01-01T00:00:00.000Z', tenant: 'acme' }, ), - ).toEqual({ ref: '042', tenant: 'acme' }); + ).toEqual({ day: 'Thu Jan 01 2026 00:00:00 GMT+0100', tenant: 'acme' }); }); it.each([ @@ -97,8 +97,7 @@ describe('unpackPrimaryKey', () => { ['a boolean', false], ['a relation object', { id: 3 }], ['a buffer payload', { type: 'Buffer', data: [1, 2] }], - ['an array', [1, 2]], - ])('should ignore %s and fall back to the positional segment', (_, value) => { + ])('should not read %s as a key value, leaving that key its position', (_, value) => { expect( unpackPrimaryKey( '7|ab', @@ -111,6 +110,20 @@ describe('unpackPrimaryKey', () => { ).toEqual({ orderId: 7, sku: 'ab' }); }); + it('should refuse to read a key whose response key another field shares', () => { + expect( + unpackPrimaryKey( + 'A|B|C', + [ + { name: 'ownerId', type: 'String', ambiguousRecordKey: true }, + { name: 'owner_id', type: 'String', ambiguousRecordKey: true }, + { name: 'sku', type: 'String' }, + ], + { ownerId: 'B', sku: 'C' }, + ), + ).toEqual({ ownerId: 'A', owner_id: 'B', sku: 'C' }); + }); + it('should fall back to the positional segments when no key matches', () => { expect( unpackPrimaryKey( @@ -151,19 +164,6 @@ describe('unpackPrimaryKey', () => { id: 42, }); }); - - it('should assign one segment to a single key even when two keys share a value', () => { - expect( - unpackPrimaryKey( - 'acme|acme', - [ - { name: 'tenant_id', type: 'String' }, - { name: 'owner_id', type: 'String' }, - ], - { tenantId: 'acme', ownerId: 'acme' }, - ), - ).toEqual({ tenant_id: 'acme', owner_id: 'acme' }); - }); }); describe('when the key was derived, so its arity is a guess', () => { diff --git a/packages/agent-bff/test/data/response-mappers.test.ts b/packages/agent-bff/test/data/response-mappers.test.ts index 8c437b926e..b0bba6a9bf 100644 --- a/packages/agent-bff/test/data/response-mappers.test.ts +++ b/packages/agent-bff/test/data/response-mappers.test.ts @@ -38,6 +38,26 @@ describe('mapListResponse', () => { ); }); + it('should order the composite key from the record rather than from the key order', () => { + const result = mapListResponse( + 'edgeCompositePk', + [{ id: 'acme|42', tenantId: 'acme', seq: 42, payload: 'x' }], + [ + { name: 'seq', type: 'Number' }, + { name: 'tenant_id', type: 'String' }, + ], + ); + + expect(result.data[0]).toEqual( + expect.objectContaining({ + __forest: { + collection: 'edgeCompositePk', + primaryKey: { seq: 42, tenant_id: 'acme' }, + }, + }), + ); + }); + it('should throw a mapping error when a record has no id', () => { expect(() => mapListResponse('users', [{ email: 'x' }], [{ name: 'id', type: 'Number' }]), diff --git a/packages/agent-bff/test/read-model/read-model.test.ts b/packages/agent-bff/test/read-model/read-model.test.ts index 55fcb50714..1e8ae7da43 100644 --- a/packages/agent-bff/test/read-model/read-model.test.ts +++ b/packages/agent-bff/test/read-model/read-model.test.ts @@ -314,6 +314,21 @@ describe('ReadModel', () => { expect(model.getPrimaryKeys('ghost')).toEqual([]); }); + it('should flag a key whose response key another field of the collection shares', () => { + const model = new ReadModel([ + collection('memberships', [ + { ...column('owner_id'), isPrimaryKey: true }, + { ...column('sku'), isPrimaryKey: true }, + { ...column('ownerId'), isPrimaryKey: false }, + ]), + ]); + + expect(model.getPrimaryKeys('memberships')).toEqual([ + { name: 'owner_id', type: 'String', ambiguousRecordKey: true }, + { name: 'sku', type: 'String' }, + ]); + }); + // A forest_liana older than 9.17.6 publishes no isPrimaryKey anywhere, which used to leave the // collection keyless and make every list answer 500 mapping_error. describe('when the schema declares no key', () => { From 61233ec649195ad0558966ca49faf8a3dfa90929 Mon Sep 17 00:00:00 2001 From: Anthony Guimard Date: Mon, 14 Sep 2026 11:54:56 +0200 Subject: [PATCH 3/7] fix(agent-bff): keep an unmatched key on its own segment unless a match took it Claude-Session: https://claude.ai/code/session_01TrJS9jCkAfmyWQc1PdSEA2 --- packages/agent-bff/src/data/pack-id.ts | 18 ++++++++++--- packages/agent-bff/test/data/pack-id.test.ts | 28 ++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/packages/agent-bff/src/data/pack-id.ts b/packages/agent-bff/src/data/pack-id.ts index 7fcb2daf8b..2ee8a6dec7 100644 --- a/packages/agent-bff/src/data/pack-id.ts +++ b/packages/agent-bff/src/data/pack-id.ts @@ -62,8 +62,13 @@ function comparableValue(record: Record, key: PrimaryKeyField): * `Date` or a Buffer key is the same story from the other side: their attribute form differs from * the packed one (ISO versus `String(date)`), they match nothing, and they keep their position. * - * A key that matches nothing keeps its positional segment, which is what this function returns - * whole when no record is given. + * A key that matches nothing keeps its own positional segment whenever no match claimed it, and + * only moves when a match did. That is what makes this a strict improvement over pairing by + * position: a key the published order happened to get right can only lose its segment to a key + * that proved the segment is its own, and a key that lost it was wrong by definition. Reordering + * every unmatched key instead would trade a lucky-but-right key for an unproven guess. + * + * With no record, the values are returned whole and the pairing is the positional one. */ function segmentsByKey( values: string[], @@ -83,9 +88,16 @@ function segmentsByKey( return values[index]; }); + const placed = matched.map((value, index) => { + if (value !== null || claimed[index]) return value; + claimed[index] = true; + + return values[index]; + }); + const leftovers = values.filter((_, index) => !claimed[index]); - return matched.map(value => value ?? (leftovers.shift() as string)); + return placed.map(value => value ?? (leftovers.shift() as string)); } /** diff --git a/packages/agent-bff/test/data/pack-id.test.ts b/packages/agent-bff/test/data/pack-id.test.ts index f5daed7cde..a465dbf006 100644 --- a/packages/agent-bff/test/data/pack-id.test.ts +++ b/packages/agent-bff/test/data/pack-id.test.ts @@ -124,6 +124,34 @@ describe('unpackPrimaryKey', () => { ).toEqual({ ownerId: 'A', owner_id: 'B', sku: 'C' }); }); + it('should leave an unmatched key on its own segment when no match claimed it', () => { + expect( + unpackPrimaryKey( + '42|Thu Jan 01 2026 00:00:00 GMT+0100|7', + [ + { name: 'ref', type: 'String' }, + { name: 'day', type: 'String' }, + { name: 'seq', type: 'Number' }, + ], + { seq: 7, day: '2026-01-01T00:00:00.000Z', id: '42|Thu Jan 01 2026 00:00:00 GMT+0100|7' }, + ), + ).toEqual({ ref: '42', day: 'Thu Jan 01 2026 00:00:00 GMT+0100', seq: 7 }); + }); + + it('should move an unmatched key only when a matched key took its segment', () => { + expect( + unpackPrimaryKey( + 'g|b|a', + [ + { name: 'alpha', type: 'String' }, + { name: 'beta', type: 'String' }, + { name: 'gamma', type: 'String' }, + ], + { alpha: 'a', beta: 'b' }, + ), + ).toEqual({ alpha: 'a', beta: 'b', gamma: 'g' }); + }); + it('should fall back to the positional segments when no key matches', () => { expect( unpackPrimaryKey( From a5f32c20290dfeb0048c85ce6e4a22683272f0d6 Mon Sep 17 00:00:00 2001 From: Anthony Guimard Date: Mon, 14 Sep 2026 12:24:13 +0200 Subject: [PATCH 4/7] fix(agent-bff): drop the reordering when several unread keys lost their segment Claude-Session: https://claude.ai/code/session_01TrJS9jCkAfmyWQc1PdSEA2 --- packages/agent-bff/src/data/pack-id.ts | 12 ++++++++---- packages/agent-bff/test/data/pack-id.test.ts | 14 ++++++++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/packages/agent-bff/src/data/pack-id.ts b/packages/agent-bff/src/data/pack-id.ts index 2ee8a6dec7..e5a52a0175 100644 --- a/packages/agent-bff/src/data/pack-id.ts +++ b/packages/agent-bff/src/data/pack-id.ts @@ -63,10 +63,11 @@ function comparableValue(record: Record, key: PrimaryKeyField): * the packed one (ISO versus `String(date)`), they match nothing, and they keep their position. * * A key that matches nothing keeps its own positional segment whenever no match claimed it, and - * only moves when a match did. That is what makes this a strict improvement over pairing by - * position: a key the published order happened to get right can only lose its segment to a key - * that proved the segment is its own, and a key that lost it was wrong by definition. Reordering - * every unmatched key instead would trade a lucky-but-right key for an unproven guess. + * moves only when a match did. A single unread key can be placed safely, since the one segment left + * over is necessarily its own. Several cannot: as soon as one of them loses its segment to a match, + * which of the remaining segments is whose is exactly the question the record failed to answer, and + * placing them anyway hands back a wrong key under a 200 where the positional pairing would have + * raised its numeric-cast error. The whole reordering is dropped there, errors included. * * With no record, the values are returned whole and the pairing is the positional one. */ @@ -88,6 +89,7 @@ function segmentsByKey( return values[index]; }); + const unread = matched.filter(value => value === null).length; const placed = matched.map((value, index) => { if (value !== null || claimed[index]) return value; claimed[index] = true; @@ -95,6 +97,8 @@ function segmentsByKey( return values[index]; }); + if (unread > 1 && placed.some(value => value === null)) return values; + const leftovers = values.filter((_, index) => !claimed[index]); return placed.map(value => value ?? (leftovers.shift() as string)); diff --git a/packages/agent-bff/test/data/pack-id.test.ts b/packages/agent-bff/test/data/pack-id.test.ts index a465dbf006..e0dfa1d64e 100644 --- a/packages/agent-bff/test/data/pack-id.test.ts +++ b/packages/agent-bff/test/data/pack-id.test.ts @@ -152,6 +152,20 @@ describe('unpackPrimaryKey', () => { ).toEqual({ alpha: 'a', beta: 'b', gamma: 'g' }); }); + it('should keep the positional pairing, error included, when two displaced keys stay unread', () => { + expect(() => + unpackPrimaryKey( + 'ab|9|5', + [ + { name: 'id', type: 'Number' }, + { name: 'owner_id', type: 'String', ambiguousRecordKey: true }, + { name: 'sku', type: 'String' }, + ], + { sku: 'ab', ownerId: '9', id: 'ab|9|5' }, + ), + ).toThrow(expect.objectContaining({ type: 'mapping_error', status: 500 })); + }); + it('should fall back to the positional segments when no key matches', () => { expect( unpackPrimaryKey( From 02c8a68af924c54ec3d26b037ca3e31ca4ac9c33 Mon Sep 17 00:00:00 2001 From: Anthony Guimard Date: Mon, 14 Sep 2026 12:24:15 +0200 Subject: [PATCH 5/7] docs(agent-bff): stop telling clients to assemble a composite parentId Claude-Session: https://claude.ai/code/session_01TrJS9jCkAfmyWQc1PdSEA2 --- packages/agent-bff/src/openapi/unfolded-paths.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/packages/agent-bff/src/openapi/unfolded-paths.ts b/packages/agent-bff/src/openapi/unfolded-paths.ts index 78be1045bf..b46371cc6d 100644 --- a/packages/agent-bff/src/openapi/unfolded-paths.ts +++ b/packages/agent-bff/src/openapi/unfolded-paths.ts @@ -457,10 +457,15 @@ function parentIdSchema( // A composite id only works as its packed string: a number could never carry the separator. type: 'string', pattern: NON_BLANK_PATTERN, + // The order the keys are published in is not the order the agent packs them in, so naming one + // would send a client to build an id the agent unpacks onto the wrong columns. description: - `The composite id of the parent ${quoted(parent)} record: the values of ` + - `${primaryKeys.map(key => key.name).join(', ')} joined by ` + - `${quoted(PACKED_ID_SEPARATOR)}, in that order.`, + `The composite id of the parent ${quoted(parent)} record, taken verbatim from that ` + + `record's own id: the values of ${primaryKeys.map(key => key.name).join(', ')} joined by ` + + `${quoted( + PACKED_ID_SEPARATOR, + )}, in the order the agent packs them. Copy it from a listed ` + + `record rather than assembling it.`, }; } From cc29a2b0e36f117dc5627c1bc0ed20524715b9b2 Mon Sep 17 00:00:00 2001 From: Anthony Guimard Date: Mon, 14 Sep 2026 12:36:58 +0200 Subject: [PATCH 6/7] fix(agent-bff): drop the reordering as soon as a second key goes unread Claude-Session: https://claude.ai/code/session_01TrJS9jCkAfmyWQc1PdSEA2 --- packages/agent-bff/src/data/pack-id.ts | 25 +++++++------------- packages/agent-bff/test/data/pack-id.test.ts | 15 ++++++++++++ 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/packages/agent-bff/src/data/pack-id.ts b/packages/agent-bff/src/data/pack-id.ts index e5a52a0175..afd3bb88b9 100644 --- a/packages/agent-bff/src/data/pack-id.ts +++ b/packages/agent-bff/src/data/pack-id.ts @@ -62,12 +62,13 @@ function comparableValue(record: Record, key: PrimaryKeyField): * `Date` or a Buffer key is the same story from the other side: their attribute form differs from * the packed one (ISO versus `String(date)`), they match nothing, and they keep their position. * - * A key that matches nothing keeps its own positional segment whenever no match claimed it, and - * moves only when a match did. A single unread key can be placed safely, since the one segment left - * over is necessarily its own. Several cannot: as soon as one of them loses its segment to a match, - * which of the remaining segments is whose is exactly the question the record failed to answer, and - * placing them anyway hands back a wrong key under a 200 where the positional pairing would have - * raised its numeric-cast error. The whole reordering is dropped there, errors included. + * One key the record cannot answer for is still placed: the single segment left over is necessarily + * its own, whatever the published order. Two are not. Which of the remaining segments is whose is + * exactly the question the record failed to answer, and placing them anyway hands back a wrong key + * under a 200 where the positional pairing would have raised its numeric-cast error. So the whole + * reordering is dropped as soon as a second key goes unread, errors included. Keeping an unread key + * on its own segment instead is not enough: with four keys, two read and two unread, both unread + * ones can sit on their published segment and both still be wrong. * * With no record, the values are returned whole and the pairing is the positional one. */ @@ -89,19 +90,11 @@ function segmentsByKey( return values[index]; }); - const unread = matched.filter(value => value === null).length; - const placed = matched.map((value, index) => { - if (value !== null || claimed[index]) return value; - claimed[index] = true; - - return values[index]; - }); - - if (unread > 1 && placed.some(value => value === null)) return values; + if (matched.filter(value => value === null).length > 1) return values; const leftovers = values.filter((_, index) => !claimed[index]); - return placed.map(value => value ?? (leftovers.shift() as string)); + return matched.map(value => value ?? (leftovers.shift() as string)); } /** diff --git a/packages/agent-bff/test/data/pack-id.test.ts b/packages/agent-bff/test/data/pack-id.test.ts index e0dfa1d64e..beb18753f3 100644 --- a/packages/agent-bff/test/data/pack-id.test.ts +++ b/packages/agent-bff/test/data/pack-id.test.ts @@ -152,6 +152,21 @@ describe('unpackPrimaryKey', () => { ).toEqual({ alpha: 'a', beta: 'b', gamma: 'g' }); }); + it('should keep the positional pairing when two unread keys each sit on a published segment', () => { + expect(() => + unpackPrimaryKey( + '9|5|ab|7', + [ + { name: 'id', type: 'Number' }, + { name: 'owner_id', type: 'String', ambiguousRecordKey: true }, + { name: 'seq', type: 'Number' }, + { name: 'sku', type: 'String' }, + ], + { seq: 7, sku: 'ab', ownerId: '5', id: '9|5|ab|7' }, + ), + ).toThrow(expect.objectContaining({ type: 'mapping_error', status: 500 })); + }); + it('should keep the positional pairing, error included, when two displaced keys stay unread', () => { expect(() => unpackPrimaryKey( From 5d702a509535e34d11ff3daa74f6e162d625581a Mon Sep 17 00:00:00 2001 From: Anthony Guimard Date: Mon, 14 Sep 2026 12:36:59 +0200 Subject: [PATCH 7/7] test(agent-bff): pin the composite parentId description to its new wording Claude-Session: https://claude.ai/code/session_01TrJS9jCkAfmyWQc1PdSEA2 --- packages/agent-bff/test/openapi/openapi-unfolded.test.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/agent-bff/test/openapi/openapi-unfolded.test.ts b/packages/agent-bff/test/openapi/openapi-unfolded.test.ts index 84fea615d5..198ad038c8 100644 --- a/packages/agent-bff/test/openapi/openapi-unfolded.test.ts +++ b/packages/agent-bff/test/openapi/openapi-unfolded.test.ts @@ -482,6 +482,11 @@ describe('the unfolded document', () => { expect(request.properties.parentId.type).toBe('string'); expect(request.properties.parentId.description).toContain('shop, number joined by "|"'); + expect(request.properties.parentId.description).toContain('in the order the agent packs them'); + expect(request.properties.parentId.description).toContain( + 'Copy it from a listed record rather than assembling it', + ); + expect(request.properties.parentId.description).not.toContain('in that order'); }); it('should fall back to the opaque parent id when the parent exposes no key metadata', () => {