feat(agent-bff): fetch and cache agent schema, capabilities and allow-list#1732
Conversation
2 new issues
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| this.ttlMs = ttlMs; | ||
| } | ||
|
|
||
| async get(): Promise<ForestSchemaCollection[]> { |
There was a problem hiding this comment.
🟡 Medium read-model/schema-cache.ts:46
When the TTL expires, get() calls refresh() and returns the awaited promise, so every concurrent reader blocks on the network fetch instead of immediately receiving the last-good schema. While this.inFlight is pending (e.g. Forest is slow or timing out), all requests stall despite usable cached data, breaking the intended "serve stale until refresh succeeds" behavior. Consider returning the stale this.entry.collections immediately when a warm cache exists and only triggering a background refresh.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/agent-bff/src/read-model/schema-cache.ts around line 46:
When the TTL expires, `get()` calls `refresh()` and returns the awaited promise, so every concurrent reader blocks on the network fetch instead of immediately receiving the last-good schema. While `this.inFlight` is pending (e.g. Forest is slow or timing out), all requests stall despite usable cached data, breaking the intended "serve stale until refresh succeeds" behavior. Consider returning the stale `this.entry.collections` immediately when a warm cache exists and only triggering a background refresh.
There was a problem hiding this comment.
Intentional — this is synchronous refresh-on-read, chosen over stale-while-revalidate during design.
The "serve last-good" contract applies on a refresh failure (warm cache keeps serving stale + emits schema_cache_refresh_error), not while a refresh is in flight. When the TTL expires we deliberately block on the refresh: concurrent readers share a single in-flight fetch (dedup, so one network call), the wait is bounded by the client's 10s timeout, and it only happens at the 24h expiry boundary (~once/day). Serving a stale schema when a fresh one is one await away was a conscious no (it also matches the MCP server precedent).
Leaving as-is.
|
Coverage Impact ⬆️ Merging this pull request will increase total coverage on Modified Files with Diff Coverage (10) 🛟 Help
|
| private buildRelations(collection: ForestSchemaCollection): void { | ||
| const relations = new Map<string, RelationTarget>(); | ||
|
|
||
| for (const field of collection.fields) { |
There was a problem hiding this comment.
This throws on a malformed payload where fields is absent: guard with collection.fields ?? [] like buildActionEndpoints does for actions. It runs outside SchemaCache try/catch, so a bad collection wedges the store for the whole 24h TTL with no SchemaUnavailableError and no metric.
There was a problem hiding this comment.
Fixed in 33c7bc9 — buildRelations now guards collection.fields ?? [], mirroring buildActionEndpoints, so a malformed collection can't wedge the store for the 24h TTL.
| private async doRefresh(): Promise<ForestSchemaCollection[]> { | ||
| try { | ||
| const collections = await this.fetcher.fetchSchema(); | ||
| this.entry = { collections, fetchedAt: this.now() }; |
There was a problem hiding this comment.
An empty collections from a transient 200 gets cached as last-good for 24h and silently denies every collection/relation/action. What about rejecting an empty result here so the cold-cache error path fires instead?
There was a problem hiding this comment.
Fixed in 33c7bc9 — an empty schema is now treated as a failed fetch (cold → SchemaUnavailableError + schema_cache_refresh_error, warm → keep serving last good), instead of being cached as an all-denying schema for 24h.
|
|
||
| const { generation } = this; | ||
| const fetch = fetcher(collection) | ||
| .then(result => { |
There was a problem hiding this comment.
Unlike SchemaCache.doRefresh, this has no .catch: a failing capabilities() propagates raw with no error counter and no last-good fallback. What about mirroring the schema cache here (emit a counter, serve stale)?
There was a problem hiding this comment.
Kept as-is, on purpose:
- The ticket's counter contract only names the schema + action-endpoint counters — a capabilities counter would be new contract.
- "Serve stale" is unsafe for capabilities specifically: stale operators would make Slice 4 validate filters against the wrong operator set. And there's no last-good on the first fetch anyway.
- The current behavior already avoids poisoning — the rejection propagates, the in-flight entry is cleared, and the next read re-attempts. The failure belongs to the request-scoped consumer (Slice 3/4), which has the context to map it to HTTP.
Happy to add a capabilities failure counter in Slice 4 if ops wants the signal.
| } | ||
|
|
||
| async get(collection: string, fetcher: CapabilitiesFetcher): Promise<CapabilitiesResult> { | ||
| const entry = this.entries.get(collection); |
There was a problem hiding this comment.
Cache is keyed by collection only while the token is bound in the fetcher, so the first caller result is served to every token for 24h. Can we confirm /capabilities is not token-scoped server-side? If it is, key by token.
There was a problem hiding this comment.
Confirmed not token-scoped. /forest/_internal/capabilities (packages/agent/src/routes/capabilities.ts, fetchCapabilities) derives operators from each field's filterOperators on the datasource schema — it does not read the caller / rendering / permissions. It's a PrivateRoute (needs a valid JWT) but the response is identical for every caller, so process-wide keying by collection is correct. Kept as-is.
| async getReadModel(): Promise<ReadModel> { | ||
| const collections = await this.schemaCache.get(); | ||
|
|
||
| if (collections !== this.lastCollections || !this.readModel) { |
There was a problem hiding this comment.
Refresh detection by array-reference identity is a contract only the doc-comment enforces, no test pins it: a future defensive copy in SchemaCache would rebuild and clear capabilities on every request. Returning a version counter from get() would make it testable.
There was a problem hiding this comment.
Fixed in 33c7bc9 — SchemaCache now exposes a monotonic revision (bumped only on a successful refresh), and the store rebuilds on a revision change instead of array-reference identity. Pinned by tests: revision increments on refresh but not on a cache hit or a warm failure, and the store doesn't rebuild/clear capabilities on a failed refresh.
| name: action.name, | ||
| endpoint: action.endpoint, | ||
| hooks: action.hooks, | ||
| fields: action.fields, |
There was a problem hiding this comment.
fields and hooks are stored by reference from the shared cached schema, so a future mutating consumer would corrupt the cache for all requests. What about freezing or shallow-cloning them here?
There was a problem hiding this comment.
Fixed in 33c7bc9 — action fields/hooks are now shallow-copied when building the endpoint map, so a mutating consumer can't corrupt the shared cached schema (consistent with the polymorphic-targets copy already done). Asserted by a test.
| it('should default to console metrics when none is provided', () => { | ||
| const bundle = createReadModel({ forestServerUrl: 'x', envSecret: 'y' }); | ||
|
|
||
| expect(bundle.store).toBeDefined(); |
There was a problem hiding this comment.
These assert only toBeDefined, not that console metrics were selected: the metrics ?? createConsoleMetrics fallback could be deleted and the test still passes. Assert an increment call routes through the console logger.
There was a problem hiding this comment.
Fixed in 33c7bc9 — the test now spies the console logger and asserts an increment is routed through it (deleting the ?? createConsoleMetrics fallback now fails the test), instead of only asserting toBeDefined.
| // Ensure any pending schema refresh (and its capabilities invalidation) runs first. | ||
| await this.getReadModel(); | ||
|
|
||
| return this.capabilitiesCache.get(collection, fetcher); |
There was a problem hiding this comment.
🟡 Medium read-model/read-model-store.ts:43
getCapabilities() can return capabilities from the wrong schema generation. await this.getReadModel() only checks the revision before the capabilities fetch starts — if another request triggers a schema refresh and capabilitiesCache.clear() runs while this.capabilitiesCache.get(collection, fetcher) is still in flight, the caller receives capabilities tied to the old schema alongside the new read-model/allow-list. Consider re-checking schemaCache.revision after the capabilities fetch resolves and retrying when it has changed, so capabilities and schema stay atomically coupled.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/agent-bff/src/read-model/read-model-store.ts around line 43:
`getCapabilities()` can return capabilities from the wrong schema generation. `await this.getReadModel()` only checks the revision before the capabilities fetch starts — if another request triggers a schema refresh and `capabilitiesCache.clear()` runs while `this.capabilitiesCache.get(collection, fetcher)` is still in flight, the caller receives capabilities tied to the old schema alongside the new read-model/allow-list. Consider re-checking `schemaCache.revision` after the capabilities fetch resolves and retrying when it has changed, so capabilities and schema stay atomically coupled.
There was a problem hiding this comment.
Valid TOCTOU. Deferring the actual fix to the data-endpoint wiring rather than adding it here, for three reasons:
- This method is not called from request handling yet (that's the data-endpoints slice).
- Cache poisoning is already prevented: a generation guard means a fetch that started before a
clear()never writes to the cache, so no stale entry is served from cache after a refresh (the ticket's coupling AC). - The residual window — a single caller receiving previous-generation capabilities while a refresh happens mid-flight — is near-nil impact: it needs a concurrent refresh exactly at the 24h boundary during an in-flight capabilities fetch AND the operators to actually differ between the two schema generations.
I've added a code note at the call site to re-check schemaCache.revision after the fetch and retry on a mismatch when the endpoints are wired, so the fix lands where the method is actually consumed.
There was a problem hiding this comment.
Thanks for the detailed reasoning—makes sense to defer given the method isn't called from production yet and the cache generation guard already prevents stale entries. Would you like me to implement the retry-on-revision-mismatch logic now in getCapabilities(), or are you handling it when you wire the endpoints?
…-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>
nbouliol
force-pushed
the
feature/prd-668-fetch-and-cache-the-agent-schema-capabilities-and-allow-list
branch
from
40bb4be to
0d226e2
Compare
| * Builds a capabilities fetcher bound to a request's agent token. The cache calls it only on a | ||
| * miss, so the token of whichever request first populates a collection is the one used. | ||
| */ | ||
| export default function createAgentCapabilitiesFetcher({ |
There was a problem hiding this comment.
🟡 Medium read-model/agent-capabilities-fetcher.ts:14
The fetcher is built once with a single request's token and the cache keys only by collection, so all subsequent requests for that collection reuse the first request's capabilities for up to 24h. Later requests with a different token see capabilities computed under the wrong permissions — wrong fields and operators for their own token.
This happens because createAgentCapabilitiesFetcher constructs one RemoteAgentClient with the initial token, and the cache never distinguishes which token fetched a collection's capabilities. Consider either including the token (or a token-derived identity) in the cache key, or creating a fresh client per request so each caller fetches with its own token.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @packages/agent-bff/src/read-model/agent-capabilities-fetcher.ts around line 14:
The fetcher is built once with a single request's `token` and the cache keys only by `collection`, so all subsequent requests for that collection reuse the first request's capabilities for up to 24h. Later requests with a different token see capabilities computed under the wrong permissions — wrong fields and operators for their own token.
This happens because `createAgentCapabilitiesFetcher` constructs one `RemoteAgentClient` with the initial `token`, and the cache never distinguishes which token fetched a collection's capabilities. Consider either including the token (or a token-derived identity) in the cache key, or creating a fresh client per request so each caller fetches with its own token.
There was a problem hiding this comment.
Kept as-is — the premise doesn't hold: /forest/_internal/capabilities is not permission- or token-scoped. fetchCapabilities (packages/agent/src/routes/capabilities.ts) derives each field's operators from filterOperators on the datasource schema; it doesn't read the caller, rendering, or permissions. It's a PrivateRoute (needs a valid JWT) but the response is identical for every valid token, so there's no "capabilities computed under the wrong permissions".
CRUD/action permissions are a separate concern (PermissionService → /liana/v1/permissions, a later slice), not capabilities. So process-wide keying by collection is correct and the cross-token reuse is intended. (Same as the earlier thread on this file.)
| hooks: { ...action.hooks, change: [...action.hooks.change] }, | ||
| fields: action.fields.map(field => ({ ...field })), |
There was a problem hiding this comment.
ForestSchemaAction declares hooks/fields as required, but that's only the declared type: ForestHttpApi.getSchema() deserializes JSON:API attributes typed Record<string, unknown> and casts the result, so nothing guarantees the runtime shape (the Forest server serves whatever apimap the customer's agent posted, across agent versions/lianas).
If a single action comes back without hooks or fields, the failure mode is nasty: the schema fetch succeeds → SchemaCache stores the entry → new ReadModel() throws a TypeError in ReadModelStore.getReadModel(). Since the cache is warm, no refresh is re-attempted: every read-model consumer throws for up to 24h (until restart or schema change), schema_cache_refresh_error never fires (it only covers the fetch), and one malformed action bricks the allow-list of all collections.
Suggestion — make the copy defensive:
| hooks: { ...action.hooks, change: [...action.hooks.change] }, | |
| fields: action.fields.map(field => ({ ...field })), | |
| hooks: { load: action.hooks?.load ?? false, change: [...(action.hooks?.change ?? [])] }, | |
| fields: (action.fields ?? []).map(field => ({ ...field })), |
Worth a companion test ("action with missing hooks/fields") next to the existing "collection with no actions/fields key" cases.
There was a problem hiding this comment.
Good catch — fixed in 6486f7e.
Same failure class as the collection-level fields/actions guards, and you're right that ForestHttpApi.getSchema() casts the JSON:API attributes so the declared required-ness of hooks/fields isn't guaranteed at runtime. Applied your suggestion:
hooks: { load: action.hooks?.load ?? false, change: [...(action.hooks?.change ?? [])] },
fields: (action.fields ?? []).map(field => ({ ...field })),and added a companion test ("should default hooks and fields when a malformed action omits them") next to the existing malformed-collection cases.
…mits them The schema is cast from Record<string, unknown>, so an action can lack hooks or fields at runtime despite the declared type. Building the endpoint map then threw in ReadModel, outside the schema-cache try/catch, wedging every collection's allow-list for the 24h TTL. Default them defensively. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Build the action-endpoint map with `Object.create(null)` so an action named like an Object.prototype member (`toString`, `constructor`, `__proto__`, …) can't falsely pass the allow-list. - Wrap the resolver's metric emissions so a throwing Metrics backend can't break its documented "never throws" contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
|
||
| async get(): Promise<ForestSchemaCollection[]> { | ||
| if (this.entry && this.now() - this.entry.fetchedAt < this.ttlMs) { | ||
| this.emitAge(); |
There was a problem hiding this comment.
2acfc83 hardened the resolver on the grounds that "Metrics is a user-supplied port; a throwing backend must not break requests" — but that invariant isn't applied here, and this call site is the worse exposure:
emitAge()sits on the cache-hit path: a throwingmetrics.gaugemakes everyget()throw even with a warm cache, so the whole read-model (store, resolver via its error path, capabilities) degrades continuously — exactly the failure classsafeIncrementwas added to prevent.- Same on the refresh success path (
doRefresh): the entry is stored, thenemitAge()can reject a refresh that actually succeeded.
Rather than spreading try/catch per call site, consider wrapping the port once in create-read-model.ts (a safeMetrics(resolvedMetrics) decorator that guards increment/gauge) and dropping the resolver-local safeIncrement — one place, whole bundle covered, consistent with the stated contract.
3 participants
Fixes PRD-668
What
Adds the BFF read-model layer (Slice 2): an in-memory, per-instance cache of the agent schema and the metadata every later BFF slice reads instead of re-fetching.
schema-cache.ts) — fetches viaSchemaService.getSchema()(MCP precedent), 24h TTL, in-flight dedup. The TTL only triggers a refresh; the last good schema is served until a refresh succeeds. Cold cache → typedSchemaUnavailableError; warm refresh failure → serves last-good +schema_cache_refresh_error. Exposesschema_cache_age_seconds.read-model.ts) — collection/relation/action allow-lists and relation targets, keyed(collection, name)(relation/action names are collection-scoped). Action allow-list = endpoint-map keys (endpoint-less actions are not exposed). Action-endpoint map typed as agent-clientActionEndpointsByCollection.action-endpoint-resolver.ts) — never throws; emitsmiss/errorcounters taggedrendering/collection/action.capabilities-cache.ts) — per-collection, 24h, fetcher passed per call (bound to the request token), invalidated with the schema. A generation guard prevents an in-flight fetch from repopulating stale data after a refresh.ReadModelStore— single owner of the coupled schema + capabilities lifecycle: a successful refresh rebuilds the read-model and clears capabilities atomically.Metricsport + console adapter (no metrics backend exists yet).Out of scope (later slices)
No operator/field validation (PRD-640), no data endpoints (PRD-639), no nested-relation guard (PRD-669), no permissions/OpenAPI, no active cache invalidation. Nothing is wired into the HTTP runtime yet — Slice 3 (PRD-671) constructs and injects it.
Tests
Build, lint, and the full suite pass (403 tests, incl. 42 new). Covers cache hit/miss, 24h boundary, cold/warm failure + re-attempt, schema-refresh invalidates capabilities, collection-scoped keying, and the miss/error counters.
🤖 Generated with Claude Code
Note
Add agent schema and capabilities read-model with TTL caching to agent-bff
ReadModelbuilt from Forest schema that answers allow-list queries for collections, relations, and actions, and supplies immutable action-endpoint mappings.SchemaCacheandCapabilitiesCachewith 24h TTL, deduped concurrent fetches, and monotonic revision tracking; schema revision changes trigger capabilities invalidation viaReadModelStore.ForestSchemaClient(wrappingSchemaService) andcreateAgentCapabilitiesFetcher(wrapping@forestadmin/agent-client) as the two remote data sources.createReadModel, which returns a store and anActionEndpointResolver; metrics default to a console logger sink if none are provided.SchemaUnavailableErrorwith no fallback; warm failures fall back to last known schema silently.Changes since #1732 opened
ReadModel.buildActionEndpointsmethod for malformed action schemas [6486f7e]ActionEndpointResolver.resolvewith a newActionEndpointResolver.safeIncrementmethod that swallows exceptions thrown by theMetricsbackend [2acfc83]ReadModelfor action endpoint storage [2acfc83]ActionEndpointResolverand prototype member collision scenarios inReadModel[2acfc83]Macroscope summarized 0d226e2.