Skip to content

fix(agent-bff): order composite key segments by the record, not by the apimap - #1898

Merged
Tonours merged 7 commits into
fix/agent-bff-4xx-body-leakfrom
fix/agent-bff-composite-pk-order
Sep 14, 2026
Merged

fix(agent-bff): order composite key segments by the record, not by the apimap#1898
Tonours merged 7 commits into
fix/agent-bff-4xx-body-leakfrom
fix/agent-bff-composite-pk-order

Conversation

@Tonours

@Tonours Tonours commented Sep 14, 2026

Copy link
Copy Markdown
Member

Stacked on #1894 (fix/agent-bff-4xx-body-leak), which merges first. This PR targets that branch, so the diff shown here is this change alone.

What changed

__forest.primaryKey paired the |-split segments of a record's packed id with the collection's primary keys by index. That index came from the apimap, which sorts its fields alphabetically (generator-collection.ts), while the agent packs in declaration order (agent/src/utils/id.ts). Neither order is contractual and they agree only by accident.

In front of forest-express-sequelize 9.3.8, with a tenant_id + seq table: the apimap publishes seq first, the agent packs acme|42, and every list of that collection answered

500 mapping_error: Cannot build primary key: invalid numeric value "acme" for "seq"

The record's own attributes now decide which segment belongs to which key. A segment equal to a key's attribute value belongs to that key, whatever order the schema published.

Why the record gives the order and not the value

The record decides the ordering and nothing else. Every emitted value still comes from the packed id, cast exactly as before. Reading the value from the record instead looks simpler and is wrong:

  • jsonapi-serializer overwrites the id attribute with the resource id (deserializer-utils.js:90). A key named id would read back acme|42, and a single Number id key would read '42' where the contract promises 42.
  • an attribute can be null, a boolean, a relation object written over the same key, or the JSON of a Buffer, and none of those fit Record<string, string | number>.
  • a Date key serializes as ISO in the attributes but as String(date) in the packed id, and only the packed form round-trips through the agent's unpackId.

A key that matches nothing, id included, keeps its positional segment. That is the behavior that shipped before.

Scope and safety

The matcher skips any key whose response key another field of the collection shares. The record then holds a single value under that key and nothing here can tell whose it is, so reading it could claim a sibling's segment. ReadModel flags those keys with ambiguousRecordKey, reusing groupByRecordKey, the collision detector the context builder already relies on.

What does not change: single-key collections, derived keys (#1895), the arity and numeric-cast errors, and every call that takes an id opaque (parentId, recordIds). A request whose record cannot answer falls back to the positional split, errors included.

Out of scope: a liana that publishes a single isPrimaryKey on a composite-key table. forest-rails flags only tenant_id and its JSON:API id carries no second component, and no BFF code recovers a value the agent never sent.

How to test

Against a v1 agent with a composite-key collection whose fields do not sort in declaration order:

POST /agent/v1/<collection>/list   {"page": {"limit": 5, "offset": 0}}

Before: 500 mapping_error. After: 200, with __forest.primaryKey naming both columns.

Measured on the conformance bench in front of forest-express-sequelize 9.3.8: pk-composite goes from fail 500 to pass 200, and that agent's column from 23 pass / 2 fail to 24 pass / 1 fail. The one failure left is caps-route, which is structural, since no v1 liana serves the capabilities route.

packages/agent-bff: 95 suites, 1888 tests, 0 failures. tsc --noEmit clean, eslint src test reports no error on the diff.

Definition of Done

General

  • Write an explicit title for the Pull Request, following Conventional Commits specification
  • Test manually the implemented changes
  • Validate the code quality (indentation, syntax, style, simplicity, readability)

Security

  • Consider the security impact of the changes made

https://claude.ai/code/session_01TrJS9jCkAfmyWQc1PdSEA2

@qltysh

qltysh Bot commented Sep 14, 2026

Copy link
Copy Markdown

All good ✅

Comment thread packages/agent-bff/src/data/pack-id.ts
@qltysh

qltysh Bot commented Sep 14, 2026

Copy link
Copy Markdown

Qlty


Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (3)

RatingFile% DiffUncovered Line #s
Coverage rating: A Coverage rating: A
packages/agent-bff/src/read-model/read-model.ts100.0%
Coverage rating: A Coverage rating: A
packages/agent-bff/src/openapi/unfolded-paths.ts100.0%
Coverage rating: A Coverage rating: A
packages/agent-bff/src/data/pack-id.ts100.0%
Total100.0%
🚦 See full report on Qlty Cloud »

🛟 Help
  • Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

  • Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

  • File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

    • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

@nbouliol nbouliol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spec: no Linear ticket is linked, so the functional check was limited to the PR description, which the diff satisfies.

Applies to: packages/agent-bff/src/openapi/unfolded-paths.ts:463 (not in this diff)
Claude Fable 5.1 (claude-fable-5-1): Should fix: the OpenAPI document tells a client to build a composite parentId in apimap order, which the agent does not unpack.

Code

unfolded-paths.ts:460 describes the composite id as the read-model keys joined by | in that order, the apimap order the agent does not pack in.

A way out: drop the in that order claim and state the id is taken opaque from a record's id.

Reproduction
  1. Generate the unfolded document for a collection declared tenant_id, seq, with a to-many relation.
  2. Read the relation route's parentId description, which says the values of seq, tenant_id joined by | in that order.
  3. Send 42|acme as parentId, and the agent unpacks tenant_id=42, seq=acme, so the relation list fails.

Comment thread packages/agent-bff/src/data/pack-id.ts Outdated
@Tonours

Tonours commented Sep 14, 2026

Copy link
Copy Markdown
Member Author

On the second finding, the parentId description in unfolded-paths.ts:463: fixed in 02c8a68, even though that line is not in this PR's diff. It is the same defect as the one this PR is about, stated in prose instead of code, and leaving it would have kept telling clients to assemble an id the agent unpacks onto the wrong columns.

The description no longer claims an order a client can rely on. It names the keys, says the id is taken verbatim from a record's own id, and says to copy it from a listed record rather than assemble it.

Both findings are in. packages/agent-bff: 95 suites, 1891 tests, 0 failures, tsc --noEmit clean, eslint src test no error.

@nbouliol

Copy link
Copy Markdown
Member

Claude Fable 5.1 (claude-fable-5-1): Closed: the composite parentId description no longer names an order a client could assemble.

How it was checked

Predicate: the document states a key order for a composite parentId that differs from the order the agent packs in.
Read unfolded-paths.ts:460 at the head: it names the agent's own order and tells the client to copy the id from a listed record.
Not pinned: openapi-unfolded.test.ts:484 matches a substring the pre-fix text also contained, so the missing assertion is filed as PRD-1221.

Comment thread packages/agent-bff/src/data/pack-id.ts
@nbouliol

Copy link
Copy Markdown
Member

Claude Fable 5.1 (claude-fable-5-1): Closed: the composite parentId description is now pinned to its new wording, so PRD-1221 is done.

How it was checked

Predicate: the document states a key order for a composite parentId that differs from the order the agent packs in.
unfolded-paths.ts:460 is unchanged since the last pass, and openapi-unfolded.test.ts:485 now asserts the new sentences and the absence of in that order.
Pinned: those three assertions fail on the pre-fix wording, so the ticket filed for the gap is closed.

@nbouliol nbouliol left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Both validator findings verified closed at 5d702a5 (guard unread > 1, brute-forced over 1230 declaration orders; parentId description pinned by test). Merge after #1894, which this PR is stacked on.

@Tonours
Tonours merged commit 5ff78e5 into fix/agent-bff-4xx-body-leak Sep 14, 2026
32 of 33 checks passed
@Tonours
Tonours deleted the fix/agent-bff-composite-pk-order branch September 14, 2026 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants