feat(agent-bff): expose collection list and count with flat mapping#1742
feat(agent-bff): expose collection list and count with flat mapping#1742nbouliol wants to merge 15 commits into
Conversation
… and count Add a pure syntactic guard that rejects any top-level field path carrying the relation separator ":" with 422 relation_field_not_supported. Closes an agent authority gap where a relation-target field could be projected without the target collection browse/scope check. Guard + tests only; wiring into live list/count handlers belongs to the data-endpoints slice. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013GqCtMftj4gwCo2AwicMNL
…-list Add a read-model layer that caches the agent schema (24h), derives collection/relation/action allow-list metadata with relation targets and an action-endpoint map, and caches per-collection capabilities coupled to the schema refresh. Adds a Metrics port emitting schema-cache and action-endpoint counters. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…argets
A reference is `${foreignCollection}.${key}`; the collection name may itself
contain dots (mongoose nested collections like `User.address`). Split on the
last dot only instead of taking the first segment.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… store Add tests for createReadModel wiring, ForestSchemaClient (mocked SchemaService), ReadModelStore.ageSeconds delegation, and the read-model defensive branches (no-reference relation, actions-less collection, unknown collection). read-model files at 100% coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… mutation Address review feedback: - guard `collection.fields ?? []` so a malformed collection cannot wedge the store for the 24h TTL - treat an empty schema as a failed fetch (cold -> SchemaUnavailableError, warm -> serve last good) instead of caching an all-denying schema - detect a schema refresh via an explicit revision counter instead of array reference identity - defensively copy action fields/hooks so a consumer cannot corrupt the shared cached schema - strengthen the console-metrics fallback test to assert routing, not presence Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…cache Deep-freeze the action-endpoint map and relation targets at construction (after the field/hook clone that detaches them from the source schema), so the getters can return live references safely. Add a note on the getCapabilities TOCTOU to resolve when the data endpoints wire it in. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add mapAgentError() translating agent-client AgentHttpError failures into the BFF error envelope, plus the complete registry of BFF-local error factories the Slice-3 endpoints emit. Fixes PRD-670 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- unmapped agent error name now returns 500 mapping_error instead of a silent status fallback - extract the unmapped-name warn out of fallbackTypeByStatus (single responsibility) - use a named constant for the validation_error type - make the name-to-type test use an explicit table instead of deriving expectations from the map under test Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…mapping - fall back to the outer AgentHttpError status when the JSON:API error object omits its own status, instead of hardcoding 400 - fall back to errors[0].message when detail is absent, before the generic default message Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…dies When the agent body is not a JSON error object, use AgentHttpError.responseText as the message source before the generic default, so plain-text or empty bodies still surface the agent-supplied text. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
JSON:API expresses errors[0].status as a string; a string status reached BffHttpError and was rejected by isSerializableError, downgrading a mapped 4xx to a generic 500. Coerce it to a number before constructing the error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lities-and-allow-list' into feature/prd-671-expose-collection-list-and-count-with-flat-mapping
…op-level-list-and' into feature/prd-671-expose-collection-list-and-count-with-flat-mapping
…act' into feature/prd-671-expose-collection-list-and-count-with-flat-mapping
Add POST /v1/:collection/list and /count as flat REST over the agent:
- list returns flat records with __forest { collection, primaryKey } and
meta.countStatus "not_requested"; primaryKey is the opaque id unpacked to a
typed { field: value } map via read-model key metadata.
- count returns { count, countStatus }, deriving "deactivated" from the raw
agent payload (meta.count) instead of the lossy Number() helper; an active
empty collection is 0/available, a missing signal is a mapping_error.
- Wire the nested-relation guard on list projection/filter/sort and count
filter (422 relation_field_not_supported).
- Resolve the collection via the read-model allow-list. Absent name maps to
404 unknown_collection; 403 collection_not_allowed has no local trigger yet
(single allow-list) — see TODO and the ticket escalation.
- All agent calls go through the exported HttpRequester so the BFF controls
the query params (notably the resolved timezone) for both list and count.
- Add schema_unavailable (503) local error and expose ReadModel.getPrimaryKeys.
Fixes PRD-671
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3 new issues
|
|
Coverage Impact ⬆️ Merging this pull request will increase total coverage on Modified Files with Diff Coverage (20) 🤖 Increase coverage with AI coding...🚦 See full report on Qlty Cloud » 🛟 Help
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| if (agentError.name !== undefined) { | ||
| const type = AGENT_ERROR_TYPE_MAP[agentError.name]; | ||
|
|
||
| if (type === undefined) { | ||
| logger('Error', 'Unmapped agent error name', { name: agentError.name, status }); | ||
|
|
||
| return mappingError(`Unmapped agent error name: ${agentError.name}`); | ||
| } | ||
|
|
||
| return new BffHttpError(status, type, message, agentError.data); | ||
| } |
There was a problem hiding this comment.
🟡 Medium http/agent-error-mapper.ts:85
When a JSON:API error from the agent has a name not present in AGENT_ERROR_TYPE_MAP, mapJsonApiError returns a 500 mapping_error instead of preserving the agent's original status and type. For example, a custom agent error subclass named CustomNotFoundError that carries status: 404 gets remapped to a 500 internal BFF failure, turning a legitimate 4xx client error into a server error.
This happens in the branch where agentError.name !== undefined and AGENT_ERROR_TYPE_MAP[agentError.name] returns undefined. Consider falling back to fallbackTypeByStatus(status) (the same path used when name is absent) rather than calling mappingError(), so unmapped error names still preserve their original HTTP status and a sensible error type.
if (agentError.name !== undefined) {
- const type = AGENT_ERROR_TYPE_MAP[agentError.name];
-
- if (type === undefined) {
- logger('Error', 'Unmapped agent error name', { name: agentError.name, status });
-
- return mappingError(`Unmapped agent error name: ${agentError.name}`);
- }
-
- return new BffHttpError(status, type, message, agentError.data);
+ const type = AGENT_ERROR_TYPE_MAP[agentError.name] ?? fallbackTypeByStatus(status);
+
+ return new BffHttpError(status, type, message, agentError.data);Also found in 1 other location(s)
packages/agent-bff/src/read-model/action-endpoint-resolver.ts:39
resolve()swallowsSchemaUnavailableErrorfromgetReadModel()and returnsundefinedas if the action mapping were simply missing. On a cold schema cache while Forest is unavailable, callers cannot distinguish "schema unavailable" from "unknown action" and will mis-handle the request (typically turning a transient503condition into a false miss/404).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/agent-bff/src/http/agent-error-mapper.ts around lines 85-95:
When a JSON:API error from the agent has a `name` not present in `AGENT_ERROR_TYPE_MAP`, `mapJsonApiError` returns a 500 `mapping_error` instead of preserving the agent's original status and type. For example, a custom agent error subclass named `CustomNotFoundError` that carries `status: 404` gets remapped to a 500 internal BFF failure, turning a legitimate 4xx client error into a server error.
This happens in the branch where `agentError.name !== undefined` and `AGENT_ERROR_TYPE_MAP[agentError.name]` returns `undefined`. Consider falling back to `fallbackTypeByStatus(status)` (the same path used when `name` is absent) rather than calling `mappingError()`, so unmapped error names still preserve their original HTTP status and a sensible error type.
Also found in 1 other location(s):
- packages/agent-bff/src/read-model/action-endpoint-resolver.ts:39 -- `resolve()` swallows `SchemaUnavailableError` from `getReadModel()` and returns `undefined` as if the action mapping were simply missing. On a cold schema cache while Forest is unavailable, callers cannot distinguish "schema unavailable" from "unknown action" and will mis-handle the request (typically turning a transient `503` condition into a false miss/`404`).
| if (field.reference) { | ||
| // reference is `${foreignCollection}.${key}`; the collection name may itself contain dots | ||
| // (e.g. mongoose nested collections like `User.address`), so drop only the trailing key. | ||
| return { type, polymorphic: false, target: field.reference.split('.').slice(0, -1).join('.') }; |
There was a problem hiding this comment.
🟡 Medium read-model/read-model.ts:24
toRelationTarget returns an empty string as the target collection for relations whose field.reference is a bare collection name (e.g. 'orders') with no trailing .<key> segment. split('.').slice(0, -1).join('.') strips the only token, so getRelationTarget() records target: '' for a valid HasMany/BelongsToMany relation, and callers that rely on the resolved target mis-handle or reject the relation. The code assumes every reference is ${foreignCollection}.${key}, but schemas also use bare collection names. Consider falling back to the full field.reference when no . is present.
- return { type, polymorphic: false, target: field.reference.split('.').slice(0, -1).join('.') };
+ const target = field.reference.includes('.')
+ ? field.reference.split('.').slice(0, -1).join('.')
+ : field.reference;
+ return { type, polymorphic: false, target };🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/agent-bff/src/read-model/read-model.ts around line 24:
`toRelationTarget` returns an empty string as the target collection for relations whose `field.reference` is a bare collection name (e.g. `'orders'`) with no trailing `.<key>` segment. `split('.').slice(0, -1).join('.')` strips the only token, so `getRelationTarget()` records `target: ''` for a valid `HasMany`/`BelongsToMany` relation, and callers that rely on the resolved target mis-handle or reject the relation. The code assumes every `reference` is `${foreignCollection}.${key}`, but schemas also use bare collection names. Consider falling back to the full `field.reference` when no `.` is present.
| primaryKeys.forEach(({ name, type }, index) => { | ||
| const value = values[index]; | ||
| result[name] = type === 'Number' ? Number(value) : value; | ||
| }); |
There was a problem hiding this comment.
🟡 Medium data/pack-id.ts:31
unpackPrimaryKey casts Number-typed key segments with Number(value) but never validates the result. When the agent returns an invalid packed numeric id such as 'abc' for a Number primary key, this produces { id: NaN } in the returned primary key instead of throwing a mapping error, so callers receive a malformed key silently. Consider checking Number.isNaN(result[name]) for Number fields and throwing mappingError when the cast fails, mirroring the agent's IdUtils.unpackId validation.
| primaryKeys.forEach(({ name, type }, index) => { | |
| const value = values[index]; | |
| result[name] = type === 'Number' ? Number(value) : value; | |
| }); | |
| primaryKeys.forEach(({ name, type }, index) => { | |
| const value = values[index]; | |
| if (type === 'Number') { | |
| const numericValue = Number(value); | |
| if (Number.isNaN(numericValue)) { | |
| throw mappingError( | |
| `Cannot build primary key: invalid numeric value "${value}" for field "${name}"`, | |
| ); | |
| } | |
| result[name] = numericValue; | |
| } else { | |
| result[name] = value; | |
| } | |
| }); |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/agent-bff/src/data/pack-id.ts around lines 31-34:
`unpackPrimaryKey` casts `Number`-typed key segments with `Number(value)` but never validates the result. When the agent returns an invalid packed numeric id such as `'abc'` for a `Number` primary key, this produces `{ id: NaN }` in the returned primary key instead of throwing a mapping error, so callers receive a malformed key silently. Consider checking `Number.isNaN(result[name])` for `Number` fields and throwing `mappingError` when the cast fails, mirroring the agent's `IdUtils.unpackId` validation.
| createPerKeyOriginMiddleware(), | ||
| createTimezoneMiddleware({ defaultTimezone }), | ||
| createAgentStubMiddleware(), | ||
| buildDataRoutesMiddleware(config, logger), |
There was a problem hiding this comment.
🟠 High src/cli-core.ts:191
In OAuth mode, requests to /agent/v1/:collection/list|count fail because buildDataRoutesMiddleware is mounted for every authenticated /agent request but only the API-key branch populates ctx.state.agentToken. When the OAuth path runs, createAuthModeMiddleware sets only ctx.state.principal, leaving ctx.state.agentToken undefined, so the data routes call the agent with Authorization: Bearer undefined. Either pass the OAuth bearer token into ctx.state.agentToken for the data routes or gate buildDataRoutesMiddleware so it only runs in API-key mode.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/agent-bff/src/cli-core.ts around line 191:
In OAuth mode, requests to `/agent/v1/:collection/list|count` fail because `buildDataRoutesMiddleware` is mounted for every authenticated `/agent` request but only the API-key branch populates `ctx.state.agentToken`. When the OAuth path runs, `createAuthModeMiddleware` sets only `ctx.state.principal`, leaving `ctx.state.agentToken` undefined, so the data routes call the agent with `Authorization: Bearer undefined`. Either pass the OAuth bearer token into `ctx.state.agentToken` for the data routes or gate `buildDataRoutesMiddleware` so it only runs in API-key mode.
1 participant
What
Slice 3 of the Forest BFF: expose
POST /v1/:collection/listand/countas flat REST over the agent.{ data: [{ ...directFields, id, __forest: { collection, primaryKey } }], meta: { countStatus: "not_requested" } }.primaryKeyis the opaque packed id unpacked into a typed{ field: value }map using the read-model's key metadata (composite ids supported; a composite id without key metadata is surfaced as a mapping error, never guessed).{ count, countStatus }. Deactivation is derived from the raw agent payload (meta.count === "deactivated"), not the lossyNumber()helper. Active empty collection →0/available; no usable signal →mapping_error(never a silent0).projection/filter/sortand countfilter→422 relation_field_not_supported.mapAgentErrorfor agent failures; local envelope for BFF-origin errors, incl. a new503 schema_unavailable).HttpRequester, so the BFF controls the query params — notably the resolvedtimezone(spec: never lean on agent-client'sEurope/Parisdefault).Stacking / dependencies
This work depends on two PRs still in review, whose branches are merged into this one. Until they land on
main, this PR's diff temporarily includes their code:PRD-669 (#1736) is already merged to
main; its content here is identical. Rebase ontomainonce #1732 and #1738 land, then this diff narrows to thedata/additions +read-model/cli-core/bff-local-errorschanges.Open item — 403 not satisfiable in Slice 3
The read-model exposes a single allow-list (the exposed collections), so it cannot distinguish an unknown collection from a known-but-disallowed one. Every absent name maps to
404 unknown_collection;collection_not_allowed(403) has no local trigger. Documented with aTODOindata-routes-middleware.tsand escalated on the ticket.Tests
Focused tests with a stubbed agent client + read-model: flat mapping +
__forest+meta.countStatus; packed/composite id roundtrip; count deactivated vs active-zero vs mapping-error fallback; guard 422 across all four surfaces; 404 resolution;503 schema_unavailable; agent failure via the error contract; timezone propagated to the agent query. Full package: build + lint + 499 tests green.Fixes PRD-671
🤖 Generated with Claude Code
Note
Expose collection list and count endpoints in agent-bff with flat record mapping
/agent/v1/:collection/listand/agent/v1/:collection/countendpoints indata-routes-middleware.tsthat forward requests to the underlying agent and return flat-mapped records with__forestmetadata.ReadModellayer (read-model.ts,read-model-store.ts) built from Forest schema, exposing allowed collections, relations, actions, primary keys, and action endpoints.SchemaCacheandCapabilitiesCachefor TTL-based caching with single-flight refresh, stale-on-error semantics, and monotonic revision tracking.cli-core.tswhen agent URL and API key are configured; falls back to the stub middleware otherwise.assertNoRelationFieldPathsto reject requests containing nested relation field paths with a 422 error, andmapAgentErrorto normalize all agent errors into consistent BFF error shapes including 503 for agent 5xx responses.schema_unavailablewhen the schema cannot be loaded, which is a new error response previously not possible with the stub.📊 Macroscope summarized aefa0cd. 21 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.