feat(router): entity caching with L1/L2, shadow mode, and analytics#2777
Open
feat(router): entity caching with L1/L2, shadow mode, and analytics#2777
Conversation
Squashed snapshot of jensneuse/entity-caching (e5ac2b9) onto main. The original branch's history was rooted at unsigned "Initial commit" 4d73e22... while wundergraph/cosmo:main is rooted at the GPG-signed equivalent — every commit SHA differs and GitHub treated the two as unrelated histories, which is why PR #2677 was closed and could not be reopened. This commit re-bases the entire feature as a single diff on top of current main so it can be reviewed and merged. Original branch is preserved at jensneuse/entity-caching for history. Supersedes #2677.
jensneuse
requested review from
a team,
Aenimus,
JivusAyrus,
Noroth,
SkArchon,
StarpTech,
devsergiy,
endigma,
thisisnithin and
wilsonrivera
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2777 +/- ##
===========================================
- Coverage 59.41% 47.58% -11.84%
===========================================
Files 238 1092 +854
Lines 25867 150755 +124888
Branches 0 10132 +10132
===========================================
+ Hits 15370 71734 +56364
- Misses 8994 77145 +68151
- Partials 1503 1876 +373
🚀 New features to boost your workflow:
|
❌ Internal Query Planner CI checks failedThe Internal Query Planner CI checks failed in the celestial repository, and this is going to stop the merge of this PR. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds end-to-end entity and query caching: new OpenFed directives and composition extraction, router protobuf/config extensions, demo cached subgraphs and router configs, comprehensive benchmark harness (k6, scenarios, fixtures, runner), playground cache‑explorer UI, many tests, scripts, and documentation. All changes are additive. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
demo/go.mod (1)
20-20:⚠️ Potential issue | 🔴 CriticalCritical: Upgrade vulnerable OpenTelemetry SDK to latest secure version.
go.opentelemetry.io/otel/sdk v1.36.0is vulnerable to multiple PATH hijacking vulnerabilities:
- GHSA-9h8m-3fm2-qjrq (CVE-2026-24051): affects v1.21.0 – v1.39.x, fixed in v1.40.0
- GHSA-hfvc-g4fc-pqhx: affects v1.15.0 – v1.42.0, fixed in v1.43.0
Upgrade to
v1.43.0to address both vulnerabilities.🔒 Proposed fix
- go.opentelemetry.io/otel/sdk v1.36.0 + go.opentelemetry.io/otel/sdk v1.43.0After applying, run
go mod tidyto update the lock file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/go.mod` at line 20, Update the OpenTelemetry SDK dependency from v1.36.0 to v1.43.0 by changing the module version for go.opentelemetry.io/otel/sdk to v1.43.0 in the go.mod entry, then run "go mod tidy" to refresh the module graph and lockfile so the vulnerable versions are removed.composition/src/v1/constants/constants.ts (1)
49-87:⚠️ Potential issue | 🟠 MajorRegister
@openfed__requestScopedin the directive registry map.
REQUEST_SCOPED_DEFINITIONis declared indirective-definitions.ts, but there is no corresponding import/entry in this file’sDIRECTIVE_DEFINITION_BY_NAME(see Line 89 map construction). That leaves this directive unwired compared to the other new cache directives.Suggested fix
import { AUTHENTICATED, BOOLEAN_SCALAR, CACHE_INVALIDATE, CACHE_POPULATE, @@ PROVIDES, QUERY_CACHE, + REQUEST_SCOPED, REQUIRE_FETCH_REASONS, REQUIRES, @@ import { AUTHENTICATED_DEFINITION, CACHE_INVALIDATE_DEFINITION, CACHE_POPULATE_DEFINITION, @@ PROVIDES_DEFINITION, QUERY_CACHE_DEFINITION, + REQUEST_SCOPED_DEFINITION, REQUIRE_FETCH_REASONS_DEFINITION, @@ [PROVIDES, PROVIDES_DEFINITION], [QUERY_CACHE, QUERY_CACHE_DEFINITION], + [REQUEST_SCOPED, REQUEST_SCOPED_DEFINITION], [REQUIRE_FETCH_REASONS, REQUIRE_FETCH_REASONS_DEFINITION],Also applies to: 89-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/constants/constants.ts` around lines 49 - 87, The DIRECTIVE_DEFINITION_BY_NAME map is missing the REQUEST_SCOPED_DEFINITION registration; import REQUEST_SCOPED_DEFINITION from directive-definitions (similar to the other imports) and add an entry mapping the directive name (the string used for other keys, e.g., '@openfed__requestScoped') to REQUEST_SCOPED_DEFINITION inside DIRECTIVE_DEFINITION_BY_NAME so the request-scoped directive is wired up like the other directives.
🟠 Major comments (25)
composition/src/v1/warnings/params.ts-14-53 (1)
14-53: 🛠️ Refactor suggestion | 🟠 MajorUse
interfacefor these object-shape warning params.These new object-shape declarations should be interfaces to match repo TypeScript conventions.
As per coding guidelines `**/*.{ts,tsx}`: Prefer interfaces over type aliases for object shapes in TypeScript.🛠️ Proposed refactor
-export type IncompleteQueryCacheKeyMappingWarningParams = { +export interface IncompleteQueryCacheKeyMappingWarningParams { subgraphName: SubgraphName; fieldCoords: FieldName; entityType: TypeName; unmappedKeyField: FieldName; -}; +} -export type AutoMappingTypeMismatchWarningParams = { +export interface AutoMappingTypeMismatchWarningParams { subgraphName: SubgraphName; argumentName: string; fieldCoords: FieldName; argumentType: string; keyField: FieldName; entityType: TypeName; keyFieldType: string; -}; +} -export type AutoMappingAdditionalNonKeyArgumentWarningParams = { +export interface AutoMappingAdditionalNonKeyArgumentWarningParams { subgraphName: SubgraphName; argumentName: string; fieldCoords: FieldName; keyField: FieldName; entityType: TypeName; extraArgument: string; -}; +} -export type AutoBatchAdditionalNonKeyArgumentWarningParams = { +export interface AutoBatchAdditionalNonKeyArgumentWarningParams { subgraphName: SubgraphName; fieldCoords: FieldName; argumentName: string; keyField: FieldName; entityType: TypeName; extraArgument: string; -}; +} -export type RequestScopedSingleFieldWarningParams = { +export interface RequestScopedSingleFieldWarningParams { subgraphName: SubgraphName; key: string; fieldCoords: FieldName; -}; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/warnings/params.ts` around lines 14 - 53, Replace the object-shape type aliases with exported interfaces: change each "export type <Name> = { ... }" to "export interface <Name> { ... }" for IncompleteQueryCacheKeyMappingWarningParams, AutoMappingTypeMismatchWarningParams, AutoMappingAdditionalNonKeyArgumentWarningParams, AutoBatchAdditionalNonKeyArgumentWarningParams, and RequestScopedSingleFieldWarningParams, preserving all property names and their types (SubgraphName, FieldName, TypeName, string, etc.) exactly as declared so the shape and exports remain identical but follow the repository convention of using interfaces for object shapes.benchmark/queries/request_scoped_viewer_articles.graphql-2-13 (1)
2-13:⚠️ Potential issue | 🟠 MajorAdd
@requestScopedto bothcurrentViewerparticipants.
currentVieweris selected twice in one request (Line 2 and Line 10), but neither field is marked request-scoped, so this scenario won’t benchmark coordinate-level L1 behavior as intended.Suggested patch
query RequestScopedViewerArticles { - currentViewer { + currentViewer `@requestScoped` { id name email } articles { id title - currentViewer { + currentViewer `@requestScoped` { id name } viewCount rating } }As per coding guidelines
**/*.graphql: Use the@requestScopeddirective for per-request coordinate L1 cache on fields that resolve to the same value within a request; every participating field is both a reader and a writer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/queries/request_scoped_viewer_articles.graphql` around lines 2 - 13, Add the `@requestScoped` directive to both occurrences of the currentViewer field in the query so that both participants (the top‑level currentViewer and the nested currentViewer under articles) are marked as request-scoped readers/writers; update the GraphQL selections for currentViewer (the one with id,name,email and the nested one with id,name) to include `@requestScoped` to enable per-request coordinate L1 caching behavior.cli/cachegraph-cachedemo/cosmo-composition.yaml-7-7 (1)
7-7:⚠️ Potential issue | 🟠 MajorUse repository-relative schema paths in committed composition config.
schema.fileis machine-specific. Line 7, Line 12, and Line 17 will fail on other environments.Suggested fix
- file: >- - /Users/jens/conductor/workspaces/cosmo/tripoli/cli/cachegraph-cachedemo/subgraphs/cachegraph.graphql + file: ./subgraphs/cachegraph.graphql @@ - file: >- - /Users/jens/conductor/workspaces/cosmo/tripoli/cli/cachegraph-cachedemo/subgraphs/cachegraph-ext.graphql + file: ./subgraphs/cachegraph-ext.graphql @@ - file: >- - /Users/jens/conductor/workspaces/cosmo/tripoli/cli/cachegraph-cachedemo/subgraphs/viewer.graphql + file: ./subgraphs/viewer.graphqlAlso applies to: 12-12, 17-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/cachegraph-cachedemo/cosmo-composition.yaml` at line 7, The composition config in cosmo-composition.yaml uses machine-specific absolute paths for schema.file (e.g., /Users/jens/.../subgraphs/cachegraph.graphql on lines referenced), which will break on other machines; update each schema.file entry to use repository-relative paths (for example use subgraphs/cachegraph.graphql or ./subgraphs/cachegraph.graphql) so the composition is portable across environments and commit the updated cosmo-composition.yaml replacing the absolute paths on the affected entries.benchmark/scripts/stop_stack.sh-14-17 (1)
14-17:⚠️ Potential issue | 🟠 MajorProcess shutdown not confirmed before pidfile removal.
The
spawn_detachedfunction usessubprocess.Popen(start_new_session=True), creating processes that are not children of the bash shell. Whenstop_stack.shruns separately,wait "${pid}"(line 15) has no effect on these non-child processes and returns immediately. The pidfile is then removed (line 17) while the process may still be running.Safer stop logic
if kill -0 "${pid}" >/dev/null 2>&1; then kill "${pid}" >/dev/null 2>&1 || true - wait "${pid}" 2>/dev/null || true + for _ in {1..20}; do + kill -0 "${pid}" >/dev/null 2>&1 || break + sleep 0.25 + done + if kill -0 "${pid}" >/dev/null 2>&1; then + kill -9 "${pid}" >/dev/null 2>&1 || true + fi fi rm -f "${pid_file}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/scripts/stop_stack.sh` around lines 14 - 17, stop_stack.sh currently kills the PID written by spawn_detached (which uses subprocess.Popen(start_new_session=True)) then immediately removes the pid file; because wait "${pid}" does nothing for non-child sessions the process may still be alive when rm -f "${pid_file}" runs. Fix: after kill "${pid}" return, poll the pid using kill -0 "${pid}" (or pgrep -P/ps to confirm process identity) in a short loop with a configurable timeout to ensure the PID exits before removing the pid file; reference the existing kill/wait calls in stop_stack.sh and the spawn_detached/start_new_session behavior to implement a safe shutdown wait-and-timeout mechanism before deleting "${pid_file}".benchmark/scripts/capture_pprof.sh-9-12 (1)
9-12:⚠️ Potential issue | 🟠 MajorAdd explicit timeouts to profile capture calls.
These
curlcalls can block much longer than expected when the endpoint is unhealthy, which can stall automation.💡 Suggested fix
-curl -sf "http://127.0.0.1:6060/debug/pprof/profile?seconds=${PPROF_SECONDS}" \ +CURL_CONNECT_TIMEOUT="${CURL_CONNECT_TIMEOUT:-2}" +CURL_MAX_TIME="${CURL_MAX_TIME:-20}" + +curl -fsS --connect-timeout "${CURL_CONNECT_TIMEOUT}" --max-time "${CURL_MAX_TIME}" \ + "http://127.0.0.1:6060/debug/pprof/profile?seconds=${PPROF_SECONDS}" \ -o "${OUTPUT_DIR}/router_cpu.pb.gz" -curl -sf "http://127.0.0.1:6060/debug/pprof/heap" \ +curl -fsS --connect-timeout "${CURL_CONNECT_TIMEOUT}" --max-time "${CURL_MAX_TIME}" \ + "http://127.0.0.1:6060/debug/pprof/heap" \ -o "${OUTPUT_DIR}/router_heap.pb.gz"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/scripts/capture_pprof.sh` around lines 9 - 12, The two curl invocations in capture_pprof.sh that download router CPU and heap profiles (using PPROF_SECONDS and OUTPUT_DIR) can hang; update those curl calls to include explicit timeouts (e.g. --connect-timeout and --max-time) and keep -f/-s behavior so failures are surfaced quickly — modify the lines that call curl for "/debug/pprof/profile" and "/debug/pprof/heap" to add suitable timeout flags (adjust values as appropriate for PPROF_SECONDS) so the script won't block indefinitely when the endpoint is unhealthy.benchmark/scripts/wait_ready.sh-7-13 (1)
7-13:⚠️ Potential issue | 🟠 MajorBound each readiness probe with timeouts.
Right now a hanging endpoint can block the script beyond the intended 60 attempts. Add connect/overall timeouts per probe to keep readiness bounded.
💡 Suggested fix
check_ready() { + local ct="${CURL_CONNECT_TIMEOUT:-1}" + local mt="${CURL_MAX_TIME:-2}" docker exec "${REDIS_CONTAINER}" redis-cli ping | rg -q '^PONG$' && - curl -sf http://127.0.0.1:8088/metrics >/dev/null && - curl -sf http://127.0.0.1:6060/debug/pprof/heap >/dev/null && - curl -sf http://127.0.0.1:3002/ >/dev/null && - curl -sf http://127.0.0.1:4012/ >/dev/null && - curl -sf http://127.0.0.1:4013/ >/dev/null && - curl -sf http://127.0.0.1:4014/ >/dev/null + curl -fsS --connect-timeout "$ct" --max-time "$mt" http://127.0.0.1:8088/metrics >/dev/null && + curl -fsS --connect-timeout "$ct" --max-time "$mt" http://127.0.0.1:6060/debug/pprof/heap >/dev/null && + curl -fsS --connect-timeout "$ct" --max-time "$mt" http://127.0.0.1:3002/ >/dev/null && + curl -fsS --connect-timeout "$ct" --max-time "$mt" http://127.0.0.1:4012/ >/dev/null && + curl -fsS --connect-timeout "$ct" --max-time "$mt" http://127.0.0.1:4013/ >/dev/null && + curl -fsS --connect-timeout "$ct" --max-time "$mt" http://127.0.0.1:4014/ >/dev/null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/scripts/wait_ready.sh` around lines 7 - 13, The readiness probes can hang indefinitely; bound each probe with timeouts by wrapping the redis check and each curl call with a short overall timeout. For the Redis check (using REDIS_CONTAINER and redis-cli ping) run it under the system timeout utility (e.g., timeout 2s) so a stuck docker exec cannot block, and for every curl invocation add both a connect timeout and a max overall timeout (e.g., --connect-timeout 2 --max-time 5) so the metrics and service endpoints (http://127.0.0.1:8088/metrics, /debug/pprof/heap, and the ports 3002, 4012, 4013, 4014) fail fast instead of hanging.Makefile-190-197 (1)
190-197:⚠️ Potential issue | 🟠 MajorPropagate subgraph startup failure before launching the router.
Line 195 backgrounds
go run cmd/all/main.go, but the target never verifies that process is still alive. If startup fails, the router still boots and the failure is masked.💡 Suggested fix
demo: `@echo` "Composing subgraph schemas..." cd demo && $(MAKE) compose-cache `@echo` "Starting subgraphs and router..." `@echo` "Playground will be at http://localhost:3002/" - cd demo && go run cmd/all/main.go & \ - sleep 2 && cd router && go run cmd/router/main.go --config ../demo/router-cache.yaml + `@set` -e; \ + cd demo && go run cmd/all/main.go & DEMO_PID=$$!; \ + trap 'kill $$DEMO_PID >/dev/null 2>&1 || true' EXIT INT TERM; \ + sleep 2; \ + kill -0 $$DEMO_PID 2>/dev/null || { echo "demo subgraphs failed to start" >&2; exit 1; }; \ + cd router && go run cmd/router/main.go --config ../demo/router-cache.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 190 - 197, The demo Makefile target currently backgrounds the subgraph start (go run cmd/all/main.go) and immediately proceeds to start the router, masking startup failures; update the recipe so it launches the subgraph process, captures its PID (or run it in a subshell), then verify it started successfully before starting the router (e.g., wait for the process to be alive or poll its health/port), and only after successful verification run go run cmd/router/main.go --config ../demo/router-cache.yaml; reference the demo target and the commands go run cmd/all/main.go and go run cmd/router/main.go --config ../demo/router-cache.yaml when making the change.benchmark/scripts/scrape_metrics.ts-54-57 (1)
54-57:⚠️ Potential issue | 🟠 MajorAdd an explicit timeout to metrics scraping fetch.
A hung metrics endpoint can stall the benchmark suite indefinitely because this request has no timeout.
Proposed fix
- const response = await fetch("http://127.0.0.1:8088/metrics"); + const controller = new AbortController(); + const timeout = setTimeout(() => controller.abort(), 5_000); + const response = await fetch("http://127.0.0.1:8088/metrics", { + signal: controller.signal, + }); + clearTimeout(timeout);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/scripts/scrape_metrics.ts` around lines 54 - 57, The fetch call that assigns to response lacks a timeout and can hang; wrap the fetch in an AbortController: create an AbortController, start a timer (e.g., 5s) that calls controller.abort(), pass controller.signal to fetch, and clear the timer after fetch completes; handle the abort case (detect AbortError or signal.aborted) and throw a clear error like "metrics fetch timed out"; update the existing fetch/response logic in scrape_metrics.ts to use this AbortController pattern so the benchmark doesn't stall.playground/src/components/playground/cache-explorer-controller.ts-32-47 (1)
32-47:⚠️ Potential issue | 🟠 MajorPrevent stale-run cleanup from clobbering the active run state.
On overlapping
start()calls, the earlier run can reachfinally(Line 46) and clearcurrentControllerfor the newer run, and abort handling can emitidlewhile a newer run is active.Proposed fix
start: async (config: CacheExplorerConfig): Promise<void> => { - if (currentController) { - currentController.abort(); - } - currentController = new AbortController(); + if (currentController) currentController.abort(); + const controller = new AbortController(); + currentController = controller; try { - await runCacheExplorer(config, emit, currentController.signal); - } catch (err: any) { - if (err?.message === 'aborted' || err?.name === 'AbortError') { - emit({ status: 'idle' }); + await runCacheExplorer(config, emit, controller.signal); + } catch (err: unknown) { + const isAbort = + (err instanceof DOMException && err.name === 'AbortError') || + (err instanceof Error && err.message === 'aborted'); + if (currentController !== controller) return; + if (isAbort) { + emit({ status: 'idle' }); } else { - emit({ status: 'error', message: err?.message || 'Cache explorer failed' }); + emit({ + status: 'error', + message: err instanceof Error ? err.message : 'Cache explorer failed', + }); } } finally { - currentController = null; + if (currentController === controller) { + currentController = null; + } } },As per coding guidelines:
Avoid any type in TypeScript; use specific types or generics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playground/src/components/playground/cache-explorer-controller.ts` around lines 32 - 47, The start function can clear currentController from a previous run's finally, clobbering a newer run; fix by creating a local controller (e.g., const controller = new AbortController()), assign currentController = controller, pass controller.signal to runCacheExplorer, and in finally only set currentController = null if currentController === controller so you don't clear a newer controller; also replace the typed catch signature catch (err: any) with an untyped catch (err) and narrow the error using safe type guards (e.g., check typeof err === 'object' && err !== null and read err.message or err.name) or an isAbortError helper to decide between emitting idle vs error while avoiding the any type.demo/pkg/subgraphs/subgraphs.go-227-229 (1)
227-229:⚠️ Potential issue | 🟠 MajorViewer subgraph bypasses the shared middleware chain.
ViewerHandlerand the viewer server registration skipinjector.Latency(injector.HTTP(...)), so viewer behavior diverges from other subgraphs.Proposed fix
func ViewerHandler() http.Handler { - return viewer.NewHandler() + return injector.Latency(injector.HTTP(viewer.NewHandler())) } @@ if config.Ports.Viewer != 0 { servers = append(servers, &http.Server{ Addr: ":" + strconv.Itoa(config.Ports.Viewer), - Handler: viewer.NewHandler(), + Handler: ViewerHandler(), }) }Also applies to: 312-316
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/pkg/subgraphs/subgraphs.go` around lines 227 - 229, ViewerHandler currently returns viewer.NewHandler() directly and bypasses the shared middleware; change ViewerHandler to wrap the viewer.NewHandler() with the same middleware chain used by other subgraphs (e.g., return injector.Latency(injector.HTTP(viewer.NewHandler())) or the equivalent call sequence used elsewhere) and update the other place where the viewer handler is registered (the separate viewer server registration) to also use the wrapped/middleware-wrapped handler so the viewer subgraph passes through injector.HTTP and injector.Latency like the rest.benchmark/k6/cache_demo.js-70-72 (1)
70-72:⚠️ Potential issue | 🟠 MajorMerge custom headers instead of replacing the JSON content type.
Every scenario in
benchmark/scenarios/cache-demo.jsonsendsheaders, so this branch dropscontent-type: application/jsonon every POST. That makes the benchmark depend on the router accepting a JSON body without a JSON content type.Suggested fix
{ - headers: payload.headers || { "content-type": "application/json" }, + headers: { + "content-type": "application/json", + ...(payload.headers ?? {}), + }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/k6/cache_demo.js` around lines 70 - 72, The current branch replaces the default JSON content-type when payload.headers is present; change the assignment so headers are merged instead of replaced: build the headers object by starting with the default { "content-type": "application/json" } and then spreading/merging payload.headers on top (so payload.headers can override the default if it provides its own content-type) and use that merged object for the headers property (reference payload.headers and the headers property in this request object).composition/src/router-configuration/types.ts-127-139 (1)
127-139:⚠️ Potential issue | 🟠 Major
EntityCacheConfigdrops the new not-found TTL.
connect/src/wg/cosmo/node/v1/node_pb.tsalready exposesEntityCacheConfiguration.notFoundCacheTtlSeconds, but this hand-written config type has no slot for it. As a result, composition cannot carry that value intoConfigurationData, so the router never sees the negative-cache TTL even though the wire format supports it.Suggested fix
export type EntityCacheConfig = { typeName: TypeName; maxAgeSeconds: number; // When true, request headers are included in the cache key (useful for user-specific entities) includeHeaders: boolean; // When true, allows partial cache hits — the router fetches only missing entities from the subgraph partialCacheLoad: boolean; // When true, the cache runs in shadow mode — cache reads/writes happen but responses always come from the subgraph. // Useful for warming caches or validating cache correctness without affecting production traffic. shadowMode: boolean; + // Optional TTL for caching "not found" entity responses. Omit or 0 disables negative caching. + notFoundCacheTtlSeconds?: number; };Based on learnings, "When adding fields to caching config types, wire the change through the entire stack in order: TypeScript type → composition extraction logic → proto message → Go proto generation → TS proto class → proto serialization → proto to planner mapping → composition-go bundle → integration test config recomposition".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/router-configuration/types.ts` around lines 127 - 139, EntityCacheConfig currently lacks a field for negative-cache TTL so the notFoundCacheTtlSeconds from EntityCacheConfiguration never flows into Composition/ConfigurationData; add a notFoundCacheTtlSeconds?: number (or similar name matching proto: notFoundCacheTtlSeconds) to the EntityCacheConfig type, update any composition extraction logic that builds ConfigurationData to copy EntityCacheConfiguration.notFoundCacheTtlSeconds into the new field, and ensure the ConfigurationData mapping and any proto serialization/mapping layers that consume EntityCacheConfig are updated to propagate this value to the wire (so ConfigurationData and downstream code see the negative-cache TTL).benchmark/k6/cache_demo.js-85-105 (1)
85-105:⚠️ Potential issue | 🟠 MajorHandle response parsing failures before updating metrics.
response.json()and the normalization/comparison path can throw on malformed upstream responses. Right now that aborts the iteration beforeresponse_mismatch_rateandgraphql_error_rateare updated, so the benchmark under-reports failures.Suggested fix
- const body = response.json(); - const hasGraphqlErrors = Array.isArray(body?.errors) && body.errors.length > 0; - graphqlErrorRate.add(hasGraphqlErrors); - - const sameBody = - JSON.stringify(normalizeResponseForComparison(body)) === expectedBody; + let body; + let hasGraphqlErrors = true; + let sameBody = false; + try { + body = response.json(); + hasGraphqlErrors = Array.isArray(body?.errors) && body.errors.length > 0; + sameBody = + JSON.stringify(normalizeResponseForComparison(body)) === expectedBody; + } catch (_error) { + graphqlErrorRate.add(true); + mismatchRate.add(true); + mismatchCount.add(1); + return; + } + graphqlErrorRate.add(hasGraphqlErrors);As per coding guidelines, "Add proper error handling with try-catch blocks".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/k6/cache_demo.js` around lines 85 - 105, The parsing and comparison logic (response.json(), normalizeResponseForComparison, expectedBody) can throw and currently prevents updating metrics (graphqlErrorRate, mismatchRate, mismatchCount); wrap the parse/normalize/check block in a try-catch around response.json() and subsequent JSON handling so exceptions are caught, and in the catch handler increment mismatchRate.add(true) and mismatchCount.add(1) and also record graphqlErrorRate.add(true) (or a suitable error indicator) before returning/continuing the iteration; keep the normal path to compute hasGraphqlErrors, sameBody, run check(...) and update graphqlErrorRate, mismatchRate, mismatchCount as before.composition/tests/v1/directives/entity-cache-mapping-rules.test.ts-1065-1102 (1)
1065-1102:⚠️ Potential issue | 🟠 MajorRedundant
@openfed__isshould warn, not pass silently.This assertion locks in the opposite of the mapping rule. When the argument name already matches the key field, composition should still surface the redundancy warning so authors can remove the no-op directive. As per coding guidelines, "Warn when
@openfed__isdirective's argument name matches the key field name (redundant mapping)".composition/tests/v1/directives/entity-cache-fuzz.test.ts-1169-1189 (1)
1169-1189:⚠️ Potential issue | 🟠 MajorThis scenario should not pass without
@openfed__entityCacheon the return entity.Expecting a
rootFieldCacheConfigurationhere contradicts the@openfed__queryCachecontract. The test should assert the validation failure instead of empty mappings. As per coding guidelines, "@openfed__queryCachereturn type must have@openfed__entityCachedirective".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/tests/v1/directives/entity-cache-fuzz.test.ts` around lines 1169 - 1189, The test incorrectly expects a rootFieldCacheConfiguration when a field uses `@openfed__queryCache` but the return type Product lacks `@openfed__entityCache`; update the test "45. `@openfed__queryCache` on field returning entity that exists only in another subgraph" to assert a validation failure instead of a defined config: call getConfigForType(sg, QUERY) and expect it to either throw a validation error or return undefined/null, and replace the subsequent expects (config!.rootFieldCacheConfigurations, entityKeyMappings) with an assertion that a validation error referencing the `@openfed__queryCache/`@openfed__entityCache contract is raised (or that config is falsy), so the test enforces "return type must have `@openfed__entityCache`" rather than allowing empty mappings.composition/tests/v1/directives/entity-cache-mapping-rules.test.ts-364-387 (1)
364-387:⚠️ Potential issue | 🟠 MajorThis test codifies an invalid
@openfed__queryCacheshape as valid.
@openfed__queryCacheis supposed to require the return entity type to carry@openfed__entityCache. Keeping a passing path with emptyentityKeyMappingswill preserve the wrong behavior in normalization. As per coding guidelines, "@openfed__queryCachereturn type must have@openfed__entityCachedirective".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/tests/v1/directives/entity-cache-mapping-rules.test.ts` around lines 364 - 387, The test "rule 0b" accepts a Query root field with `@openfed__queryCache` even though the return type Product lacks `@openfed__entityCache`; update the test and code path checked by getSingleQueryRootFieldConfig to enforce that `@openfed__queryCache` requires the return type to have `@openfed__entityCache`: modify the test to expect a validation/failure (or non-empty entityKeyMappings error) instead of an empty entityKeyMappings array, and ensure the logic in getSingleQueryRootFieldConfig (and the RootFieldCacheConfig construction) validates presence of `@openfed__entityCache` on the entity type and either throws or returns a config that signals invalid shape rather than producing an empty entityKeyMappings.composition/tests/v1/directives/entity-cache-fuzz.test.ts-160-197 (1)
160-197:⚠️ Potential issue | 🟠 MajorThese cases don't assert a single expected outcome, so they won't catch regressions.
The repeated
if (result.success) { ... } // if it errored, that's also correctpattern makes the suite green for both permissive and rejecting behavior. That defeats the point of adding coverage around these edge cases.Also applies to: 262-288, 412-488
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/tests/v1/directives/entity-cache-fuzz.test.ts` around lines 160 - 197, The tests currently tolerate either success or failure by wrapping assertions in "if (result.success) { ... }" which masks regressions; update each test (e.g., the "6. `@openfed__is`(fields: \"id id\")" test and the other cases at the noted ranges) to assert a single deterministic outcome: replace the conditional with an explicit expectation for result.success (either expect(result.success).toBe(true) or expect(result.success).toBe(false) depending on the intended behavior), and if expecting success, then cast to BatchNormalizationSuccess and perform the uniqueness assertions on internalSubgraphBySubgraphName / configurationDataByTypeName / rootFieldCacheConfigurations / entityKeyMappings (verify keyFields has no duplicates) so the test fails on any change in behavior.demo/pkg/subgraphs/cachegraph/subgraph/schema.resolvers.go-79-86 (1)
79-86:⚠️ Potential issue | 🟠 Major
articleBySlugis still an ID lookup.This compares
slugagainstArticle.ID, and the model/schema do not define a slug field. The demo therefore isn't exercising a real remapped slug lookup, and actual slug values will always miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/pkg/subgraphs/cachegraph/subgraph/schema.resolvers.go` around lines 79 - 86, The resolver ArticleBySlug is incorrectly comparing the incoming slug against Article.ID in articlesData, so lookups always miss; update the lookup to compare against a real slug property (e.g., Article.Slug) or compute a slug from an existing field (e.g., Title) before matching. Add the slug field to the model.Article and populate it in articlesData (or implement a deterministic slugify function and use it in ArticleBySlug) and then change the loop in ArticleBySlug to compare slug == a.Slug (or slug == slugify(a.Title)) and return the matched article.playground/src/components/playground/cache-explorer-runner.ts-161-196 (1)
161-196:⚠️ Potential issue | 🟠 MajorCount pass-through fetches in
sourceBreakdown.This bookkeeping only runs inside
if (ct), so fetches that have nocache_tracenever incrementtotalFetchesorhttpCalls. That underreports subgraph work for non-cached/mixed plans even though the comment says the 0/0/0/0 case should count as HTTP.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playground/src/components/playground/cache-explorer-runner.ts` around lines 161 - 196, The current logic updates sourceMap counts only when ct (cache_trace) exists, so fetch nodes with no cache_trace are omitted; update the code that computes the source breakdown (using node, ct, sourceMap, sourceName, and entry.totalFetches/httpCalls) so that you always increment entry.totalFetches for each fetch node and treat missing ct as a pass-through HTTP call (increment entry.httpCalls and leave l1Cached/l2Cached unchanged), keeping the existing classification logic for when ct is present to decide l1Cached/l2Cached/httpCalls.playground/src/components/playground/cache-explorer-runner.ts-300-311 (1)
300-311:⚠️ Potential issue | 🟠 MajorMeasure client latency after the body is consumed.
fetch()resolves when headers arrive, not after the response body is read. Stopping the timer beforeresp.json()makesclientDurationMstoo low, and the no-trace fallback reuses that understated value.Suggested fix
const start = performance.now(); const resp = await fetch(config.url, { method: 'POST', headers: { 'Content-Type': 'application/json', ...headers, }, body: JSON.stringify(body), signal, }); - const clientDurationMs = performance.now() - start; const parsed = await resp.json(); + const clientDurationMs = performance.now() - start;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playground/src/components/playground/cache-explorer-runner.ts` around lines 300 - 311, The clientDurationMs is measured too early because fetch() resolves on headers; move the timing so it measures after the response body is consumed: await resp.json() into parsed first, then compute clientDurationMs = performance.now() - start (or compute after any body-read used instead of json), ensuring the fetch call/response variable names (start, resp, parsed, clientDurationMs) are updated in the same scope so the no-trace fallback uses the correct, post-body-consumption duration.playground/src/components/playground/cache-explorer-runner.ts-311-336 (1)
311-336:⚠️ Potential issue | 🟠 MajorAbort the run on HTTP or GraphQL errors.
Right now a 4xx/5xx or a GraphQL
errorspayload is still recorded as a successful iteration, so the controller can report a completed benchmark with meaningless cache metrics. Throw once the response is parsed so the UI shows an error state instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playground/src/components/playground/cache-explorer-runner.ts` around lines 311 - 336, After parsing the HTTP response (the parsed = await resp.json() line) abort the iteration by throwing when the HTTP status is not ok or when the GraphQL payload contains errors: check resp.ok (or resp.status) and if false throw an Error including status/text and parsed body, and also if parsed.errors exists throw an Error with the errors; keep these checks before using trace/extractMetricsFromTrace, extractServerDurationMs, or buildFetchPlan so failed requests are not recorded as successful iterations.playground/src/components/playground/cache-explorer-runner.ts-292-297 (1)
292-297:⚠️ Potential issue | 🟠 MajorDon't silently drop malformed
variablesJSON.If parsing fails here, the request goes out without
variables, so the explorer benchmarks a different operation than the user configured. Fail fast and surface the parse error instead of continuing.Suggested fix
if (config.variables) { try { body.variables = JSON.parse(config.variables); - } catch { - // ignore parse errors; send as-is if it's already an object-ish string + } catch (error) { + throw new Error( + `Variables must be valid JSON: ${error instanceof Error ? error.message : 'parse error'}`, + ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playground/src/components/playground/cache-explorer-runner.ts` around lines 292 - 297, The current try/catch around parsing config.variables silently swallows JSON parse errors so body.variables is omitted; change the catch to surface the error instead of ignoring it — in the block that handles config.variables and assigns body.variables, catch the parse exception and throw or propagate a new Error (including the original error message and the offending config.variables value) so the caller fails fast and the UI/logs can show the parse problem rather than sending the request without variables.composition/src/v1/normalization/normalization-factory.ts-3794-3829 (1)
3794-3829:⚠️ Potential issue | 🟠 MajorRegister renamed root operations before extracting cache directives.
processRootFieldCacheDirectives()resolves the operation fromgetOperationTypeNodeForRootTypeName(parentTypeName), butnormalize()calls it before explicitschema { query: MyQuery }/mutation/subscriptionroots are inserted intooperationTypeNodeByTypeName. On renamed roots,@openfed__queryCache,@openfed__cacheInvalidate, and@openfed__cachePopulatecan therefore be skipped or written under the raw SDL type key instead of the normalized root config entry.Also applies to: 5640-5643
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 3794 - 3829, processRootFieldCacheDirectives is calling getOperationTypeNodeForRootTypeName before renamed root types are registered in operationTypeNodeByTypeName, causing cache directives on renamed root types to be skipped; fix by ensuring renamed root operation registration runs before directive extraction — move or invoke the logic that registers schema root mappings (the code that populates operationTypeNodeByTypeName used by normalize()) prior to calling processRootFieldCacheDirectives, so getOperationTypeNodeForRootTypeName sees the normalized root entries; apply the same ordering change where cache directive extraction is called elsewhere (e.g., the other occurrence around extract cache handling).composition/src/v1/normalization/normalization-factory.ts-4903-5047 (1)
4903-5047:⚠️ Potential issue | 🟠 MajorKeep evaluating alternative keys after one auto-mapping candidate fails.
Several branches inside this per-key loop
return []on the first type mismatch or extra-argument warning. That aborts later@keys as well, so a badidmatch suppresses an otherwise validskumapping instead of discarding only the failing key.As per coding guidelines, "Key mapping via
buildArgumentKeyMappingsmust evaluate each@keydirective independently, attempt mapping against all keys, emit separateEntityKeyMappingConfigentries for all fully-satisfiable keys, compare named types while unwrapping NonNull for auto-mapping, require strict type matching for explicit@openfed__is, and ignore nullability differences."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 4903 - 5047, The loop over keyFieldSets in buildArgumentKeyMappings returns [] inside the per-key processing (on type mismatch and extra-argument cases), which aborts evaluating remaining `@key` directives; change those early return [] statements to continue so a failing key only skips that key but allows the loop to evaluate other keys (refer to the branches around the typeMismatchWarningEmitted block where it currently does "return []", the block that emits autoBatchAdditionalNonKeyArgumentWarning for isListReturn + scalarMatchedOnListReturn, and the branch that emits autoMappingAdditionalNonKeyArgumentWarning/autoBatchAdditionalNonKeyArgumentWarning when keyFullyMapped but extraArgs exist). Ensure the function still returns the accumulated results array after the loop.composition/src/v1/normalization/normalization-factory.ts-4187-4247 (1)
4187-4247:⚠️ Potential issue | 🟠 MajorValidate list shape for nested composite-key leaves.
This branch only uses
namedTypesMatch(), which strips list wrappers. A nested input field liketags: String!will currently be accepted for a key leaftags: [String!]!, and the resultingFieldMappingConfigis not satisfiable at runtime.Possible fix
- if (!this.namedTypesMatch(inputFieldData.type, keyTypeNode)) { + if ( + !this.namedTypesMatch(inputFieldData.type, keyTypeNode) || + !this.listStructureMatches(inputFieldData.type, keyTypeNode) + ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 4187 - 4247, The code currently uses namedTypesMatch(inputFieldData.type, keyTypeNode) which ignores list wrappers, allowing mismatched list shapes (e.g., input leaf String vs key [String]) to pass; update the check to validate full type shape (including list and non-null wrappers) between inputFieldData.node.type and keyTypeNode before accepting the mapping: either replace namedTypesMatch(...) with a stricter comparator that compares list/non-null structure or add an extra check after namedTypesMatch that walks both TypeNode trees to ensure their list nesting and NonNullType wrappers match; if the shapes differ, push the same invalidDirectiveError path (using nestedInputObjectTypeMismatchErrorMessage/inputObjectCompositeTypeMismatchErrorMessage) and return null instead of creating the FieldMappingConfig (refer to namedTypesMatch, keyTypeNode, inputFieldData.node.type, FieldMappingConfig, fullEntityKeyPath).
🟡 Minor comments (10)
composition/AGENTS.md-60-68 (1)
60-68:⚠️ Potential issue | 🟡 MinorFix malformed directive names in the validation rules table.
At Line 60, Line 63, and Line 68,
@openfed**...appears to be unintended markdown formatting and should be escaped as@openfed\_\_...for accuracy/readability.Doc text correction
-| 6 | `@openfed`**queryCache return type must have `@openfed`**entityCache | Phase 2 | +| 6 | `@openfed`\_\_queryCache return type must have `@openfed`\_\_entityCache | Phase 2 | ... -| 10 | `@openfed`**is only with `@openfed`**queryCache | Phase 2 | +| 10 | `@openfed`\_\_is only with `@openfed`\_\_queryCache | Phase 2 | ... -| 16 | `@openfed`**cacheInvalidate and `@openfed`**cachePopulate are mutually exclusive | Phase 2 | +| 16 | `@openfed`\_\_cacheInvalidate and `@openfed`\_\_cachePopulate are mutually exclusive | Phase 2 |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/AGENTS.md` around lines 60 - 68, The table contains malformed directive names using '@openfed**...' instead of the intended escaped double-underscore form; search for occurrences like '@openfed**queryCache', '@openfed**is', '@openfed**cacheInvalidate', and '@openfed**cachePopulate' in the AGENTS.md rules table and replace them with the correct escaped forms '@openfed\_\_queryCache', '@openfed\_\_is', '@openfed\_\_cacheInvalidate', and '@openfed\_\_cachePopulate' so the directives render correctly and match the rest of the doc.docs/entity-caching/ENTITY_CACHING_DEMO.md-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorAdd fence languages to satisfy markdownlint MD040.
These fenced blocks should declare a language (use
textfor diagrams/tables) to avoid lint warnings.Suggested fix
-``` +```text ... -``` +```text ... -``` +```text ... -``` +```text ... -``` +```text ... -``` +```textAlso applies to: 90-90, 128-128, 159-159, 229-229, 260-260
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/entity-caching/ENTITY_CACHING_DEMO.md` at line 5, Several fenced code blocks in ENTITY_CACHING_DEMO.md are missing language identifiers and trigger markdownlint MD040; update each triple-backtick fence (``` ) used for diagrams/tables to include the language tag "text" so they read ```text instead of ``` — apply this to all occurrences of bare code fences (the repeated diagram/table blocks shown in the diff).CLAUDE.md-111-111 (1)
111-111:⚠️ Potential issue | 🟡 MinorUse a hyphenated compound modifier.
Line 111 should read
proto-generated TS classfor grammatical correctness.Suggested fix
-3. Check if the proto generated TS class has the field (`connect/src/.../node_pb.ts`) +3. Check if the proto-generated TS class has the field (`connect/src/.../node_pb.ts`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 111, Update the wording at the noted line so the compound modifier is hyphenated: replace the phrase "proto generated TS class" with "proto-generated TS class" (the string to change appears in the CLAUDE.md content that currently reads "3. Check if the proto generated TS class has the field (`connect/src/.../node_pb.ts`)").demo/pkg/subgraphs/cachegraph/subgraph/data.go-146-152 (1)
146-152:⚠️ Potential issue | 🟡 MinorMinor: Comment placement mismatch.
The comment on Line 146 says "Metric data — for shadow mode testing" but is immediately followed by
recommendedArticlesByViewer(Line 148), not metrics data. ThemetricsDatais on Line 154.📝 Proposed fix
-// Metric data — for shadow mode testing // Recommended articles per viewer — different users get different recommendations var recommendedArticlesByViewer = map[string][]string{ "v1": {"2", "3"}, // Alice → Advanced Federation + Cache Invalidation "v2": {"1", "4"}, // Bob → Intro to Caching + Performance Tuning "v3": {"1", "2", "3"}, // Charlie → all except Performance Tuning } +// Metric data — for shadow mode testing var metricsData = map[string]*model.Metric{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/pkg/subgraphs/cachegraph/subgraph/data.go` around lines 146 - 152, The comment "Metric data — for shadow mode testing" is placed before recommendedArticlesByViewer but describes metricsData; update the comments so they match their variables: either move the "Metric data — for shadow mode testing" comment to directly precede metricsData, or change that comment to describe recommendedArticlesByViewer (e.g., "Recommended articles per viewer"); locate the symbols recommendedArticlesByViewer and metricsData to adjust or relocate the comments accordingly so each comment accurately describes the following variable.docs/entity-caching/directives.md-215-243 (1)
215-243:⚠️ Potential issue | 🟡 Minor
@openfed__isis documented with the wrong argument name.These examples use
field:, but the rest of the PR surface usesfields:. As written, the copy/paste examples here won't match the implemented directive shape.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/entity-caching/directives.md` around lines 215 - 243, The docs for the `@openfed__is` directive wrongly use the argument name "field" while the implementation uses "fields"; update the directive signature, arguments table, and all examples to use "fields:" (plural) instead of "field:" so the examples match the actual directive shape (e.g., change the directive block, the Arguments table entry, and the example argument annotation on productUpc to `@openfed__is`(fields: "upc")).docs/entity-caching/engineering-brief.md-121-127 (1)
121-127:⚠️ Potential issue | 🟡 MinorThe brief still documents
@openfed__is(field: ...)instead offields:.These examples and reference rows no longer match the directive shape exercised throughout the tests, so readers copying them will get invalid SDL.
Also applies to: 143-147, 166-172, 344-350
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/entity-caching/engineering-brief.md` around lines 121 - 127, The docs show the obsolete directive usage `@openfed__is`(field: "...") instead of the current shape using fields:, so update every example and reference (e.g., the GraphQL snippet and the rows around the examples at the noted sections) to use `@openfed__is`(fields: ["id"]) (or the appropriate array of field names) so the SDL matches the directive shape used in tests and runtime; search for occurrences of `@openfed__is`(field: and replace with `@openfed__is`(fields: [...]) preserving the same field names and surrounding text.docs/entity-caching/engineering-brief.md-352-352 (1)
352-352:⚠️ Potential issue | 🟡 MinorThe list-return mapping description is stale.
This line says list-returning fields skip key mapping entirely, but this PR adds batch mappings for list returns (
isBatch, list-of-input-object composite mappings, etc.). The brief should describe that behavior instead of "population only."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/entity-caching/engineering-brief.md` at line 352, The sentence stating "List-returning fields skip key mapping entirely (no key-based lookups, only cache population)" is stale—update it to state that list-returning fields can participate in key-based lookups via batch mappings: support for isBatch, list-of-input-object composite mappings, and batched argument mapping has been added; keep note that incomplete mappings still produce warnings (not errors) and that nested `@key` fields (e.g., store { id }) remain filtered out because they cannot map to flat arguments.docs/entity-caching/directives.md-13-15 (1)
13-15:⚠️ Potential issue | 🟡 MinorThe naming principle here contradicts the actual directive surface.
This says entity caching uses unprefixed directives, but the entire document defines
@openfed__...directives. That will send readers to the wrong SDL immediately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/entity-caching/directives.md` around lines 13 - 15, The text incorrectly states "No prefix" while the document defines prefixed directives; update the wording to match the actual SDL by replacing the "No prefix" rule with a clear statement that entity caching directives use the `@openfed__` prefix (e.g., refer to directives like `@openfed__cacheInvalidate` and `@openfed__cachePopulate`) or otherwise align the examples (such as `@authenticated` and `@requiresScopes`) to the prefixed naming convention used throughout the document so readers aren't misdirected.docs/entity-caching/directives.md-124-129 (1)
124-129:⚠️ Potential issue | 🟡 MinorIncomplete single-entity key mappings are documented as hard errors, but the feature treats them as warnings.
For non-list
@openfed__queryCache, the intended behavior is to disable cache reads for that field and emit a warning, not fail composition. The docs should match that contract consistently in both sections. As per coding guidelines, "Warn when there is incomplete key mapping (for non-list return types only) on@openfed__queryCache".Also applies to: 339-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/entity-caching/directives.md` around lines 124 - 129, The docs incorrectly state that unmapped `@key` fields cause a composition error for `@openfed__queryCache`; update the wording to reflect the actual behavior: for non-list `@openfed__queryCache` fields with incomplete key mapping, composition should not fail but instead disable cache reads for that field and emit a warning; keep the descriptions of auto-mapping (argument name match) and explicit mapping with `@openfed__is` but replace the “Composition error” paragraph with text explaining the warning behavior and disabled cache reads for single-entity returns. Apply the same replacement to the other occurrence of the composition-error wording elsewhere in this document so both sections consistently describe the warning/disable behavior.benchmark/scripts/run_suite.ts-65-87 (1)
65-87:⚠️ Potential issue | 🟡 MinorValidate CLI option values before starting a run.
--vuscan becomeNaN, and missing values for--duration/--ramp-up/--ramp-downcurrently fail much later in the flow. Rejecting bad inputs here would make the benchmark runner a lot easier to diagnose.
Comment on lines
+17
to
+46
| func (r *mutationResolver) UpdateArticle(ctx context.Context, id string, title string) (*model.Article, error) { | ||
| for _, a := range articlesData { | ||
| if a.ID == id { | ||
| a.Title = title | ||
| return a, nil | ||
| } | ||
| } | ||
| return nil, nil | ||
| } | ||
|
|
||
| // CreateArticle is the resolver for the createArticle field. | ||
| func (r *mutationResolver) CreateArticle(ctx context.Context, title string, body string, authorName string) (*model.Article, error) { | ||
| a := &model.Article{ | ||
| ID: fmt.Sprintf("%d", len(articlesData)+1), | ||
| Title: title, | ||
| Body: body, | ||
| AuthorName: authorName, | ||
| PublishedAt: time.Now().Format(time.RFC3339), | ||
| Tags: []string{}, | ||
| } | ||
| articlesData = append(articlesData, a) | ||
| return a, nil | ||
| } | ||
|
|
||
| // DeleteListing is the resolver for the deleteListing field. | ||
| func (r *mutationResolver) DeleteListing(ctx context.Context, key model.ListingKey) (*model.Listing, error) { | ||
| k := listingKey{SellerID: key.SellerID, SKU: key.Sku} | ||
| l := listingsData[k] | ||
| delete(listingsData, k) | ||
| return l, nil |
Contributor
There was a problem hiding this comment.
Guard the shared demo stores.
These resolvers mutate package-level slices/maps with no synchronization while the query/entity resolvers read them concurrently. Under parallel demo traffic this can race, and listingsData is especially risky because concurrent map read/write will panic the process.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@demo/pkg/subgraphs/cachegraph/subgraph/schema.resolvers.go` around lines 17 -
46, The mutation resolvers (UpdateArticle, CreateArticle, DeleteListing) modify
shared package-level stores without synchronization; add package-level mutexes
(e.g., articlesMu for articlesData and listingsMu for listingsData) and wrap
writes with articlesMu.Lock()/Unlock() and listingsMu.Lock()/Unlock()
respectively in UpdateArticle, CreateArticle and DeleteListing; also update any
query/entity resolvers that read articlesData or listingsData to use the
corresponding mutex RLock()/RUnlock() to prevent concurrent map read/write
panics (ensure to reference the same mutex names when protecting reads).
SkArchon
reviewed
Apr 22, 2026
Contributor
SkArchon
left a comment
SkArchon
left a comment
There was a problem hiding this comment.
Added a few comments of what I saw while testing this PR while trying out the feature.
| // so additional HTTP calls are made even though @requestScoped would serve them. | ||
| // Reproduced from the cache explorer playground tool — the demo showed 3 viewer | ||
| // fetches for a query with 2 article nesting levels plus the root currentViewer. | ||
| t.Run("L1/request-scoped nested dedup", func(t *testing.T) { |
Contributor
There was a problem hiding this comment.
This test uses
RouterOptions: entityCachingOptions(cache),
Which enables L1 AND L2 caches. I would expect this test only to rely on L1 only being used for this test. Would be good if we either pass params to entityCachingOptions with what caches we wanted enabled. This would apply to other L1 / L2 tests respectively.
| }) | ||
|
|
||
| t.Run("L1/deduplicates repeated entity loads", func(t *testing.T) { | ||
| // This test was originally `a: item(id:"1") b: item(id:"1")` asserting |
Contributor
There was a problem hiding this comment.
Nit: I think this comment could be simplified.
| }) | ||
| }) | ||
|
|
||
| t.Run("L1/deduplicates repeated entity loads", func(t *testing.T) { |
Contributor
There was a problem hiding this comment.
Unsure if this tests adds any value, even without L1 enabled, the subgraph gets the exact same request (when I tested).
| // Query.currentViewer fetch populates the L1 coordinate cache under | ||
| // key "currentViewer", and every subsequent read at any nesting depth | ||
| // must inject from L1 without launching a new subgraph fetch. | ||
| require.Equal(t, int64(1), counters.viewer.Load(), |
Contributor
There was a problem hiding this comment.
Also might be good to add an inverse test where L1 disabled results in 3 calls (so there is no false positives). Ignore if this already exists.
|
|
||
| testenv.Run(t, &testenv.Config{ | ||
| RouterConfigJSONTemplate: configJSON, | ||
| RouterOptions: entityCachingOptions(cache), |
Contributor
There was a problem hiding this comment.
Another place where do we want L1 enabled for this test?
…, router, playground, demo
Composition
- Register @openfed__requestScoped in DIRECTIVE_DEFINITION_BY_NAME and add a
normalization-output regression test that fails if the entry is removed.
- Add renamed-root-operation regression tests for @openfed__queryCache and
@openfed__cacheInvalidate on types declared via `schema { query: MyQuery }`.
- Four normalization bug fixes in normalization-factory.ts:
(a) @openfed__queryCache on object return types now requires the return
entity to declare @openfed__entityCache; emits
queryCacheReturnEntityMissingEntityCacheWarning otherwise. Interface /
union return types are unaffected.
(b) Redundant @openfed__is(fields: "x") on an argument named "x" now emits
redundantIsDirectiveWarning, gated on whether auto-mapping would have
produced the same mapping (does not fire on list-valued keys where @is
is required).
(c) Nested composite-key leaves now match list/NonNull wrapping via new
typesMatchIncludingListShape helper, not just the named type.
(d) Per-@key failures in buildAutoMappings now `continue` instead of
`return []`, preserving OR semantics across @key alternatives. Global
invalidation (extra non-key arg) still clears accumulated results.
Negative-cache TTL (composition -> proto -> shared -> router)
- New directive argument @openfed__entityCache(negativeCacheTTL: Int = 0).
- TS config field notFoundCacheTtlSeconds, proto not_found_cache_ttl_seconds.
- Router maps to plan.EntityCacheConfiguration.NegativeCacheTTL.
- Three new router-test subtests under L2/ covering window, expiry, and
unset-fallback behaviors.
Demo (cachegraph)
- Replace package-level articlesData/listingsData with mutex-guarded
articleStore / listingStore. New data_race_test.go exercises both store
APIs and the generated resolver layer under -race.
- Add real slug field to Article, change articleBySlug to
@openfed__is(fields: "slug") backed by findBySlug.
- UserProfile resolver now honors Authorization header via injector.Header,
so @openfed__queryCache(includeHeaders: true) has a detectable signal in
benchmarks. cmd/cache-demo main now wraps handlers with injector.HTTP so
headers reach the context.
- Makefile demo target captures subgraph PID, installs trap for EXIT/INT/
TERM/HUP, and asserts ports 4012-4014 are live before launching router.
- subgraphs.go viewer registration now matches sibling middleware chain
(injector.Latency + injector.HTTP).
Playground (Cache Explorer)
- Guard controller cleanup against stale runs via a monotonically-increasing
run id.
- Fail fast on malformed `variables` JSON instead of silently dropping it.
- Count pass-through fetches (no cache_trace) as sourceBreakdown.httpCalls.
- Move clientDurationMs measurement after await resp.json() completes.
- Throw on HTTP non-2xx and GraphQL errors[] before metric extraction.
Benchmark
- fetchJsonWithRetry now takes a per-attempt timeout via
AbortSignal.any([callerSig, AbortSignal.timeout(ms)]); retries on
AbortError and TimeoutError, bubbles caller-initiated aborts.
- scrape_metrics.ts wraps fetch with AbortSignal.timeout.
- k6/cache_demo.js merges user headers with the default content-type and
records mismatch + graphql-error metrics on parse failure or non-2xx.
Router tests
- Retry HTTP 501 in the testenv retryablehttp client. The router never emits
501 for GraphQL requests; any 501 is a transient stdlib TE parse race on
heavy parallel subtest load. 2000x -race on
L1/deduplicates_with_warm_L2 is stable.
Docs / hygiene
- docs/entity-caching: @openfed__is(fields: "...") not field: "...",
warnings vs composition errors, batch key mapping supported on list
returns.
- cli/cachegraph-cachedemo/cosmo-composition.yaml: repo-relative paths.
- graphqlmetrics/README.md: add pointer to `make generate-go` (also refreshes
the graphqlmetrics-ci dirty-files sticky).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ing-v2 # Conflicts: # router-tests/go.mod # router-tests/go.sum # router/go.sum
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
demo/cmd/cache-demo/main.go (2)
47-47: Set explicit HTTP timeouts on demo serversLines 47 and 54 build
http.Serverwithout timeout guards. Setting at leastReadHeaderTimeout,ReadTimeout,WriteTimeout, andIdleTimeoutprevents slow-client resource exhaustion.Suggested hardening
+const ( + readHeaderTimeout = 5 * time.Second + readTimeout = 15 * time.Second + writeTimeout = 15 * time.Second + idleTimeout = 60 * time.Second +) @@ - return &http.Server{Addr: ":" + strconv.Itoa(port), Handler: injector.Latency(injector.HTTP(mux))} + return &http.Server{ + Addr: ":" + strconv.Itoa(port), + Handler: injector.Latency(injector.HTTP(mux)), + ReadHeaderTimeout: readHeaderTimeout, + ReadTimeout: readTimeout, + WriteTimeout: writeTimeout, + IdleTimeout: idleTimeout, + } @@ - return &http.Server{Addr: ":" + strconv.Itoa(port), Handler: injector.Latency(injector.HTTP(mux))} + return &http.Server{ + Addr: ":" + strconv.Itoa(port), + Handler: injector.Latency(injector.HTTP(mux)), + ReadHeaderTimeout: readHeaderTimeout, + ReadTimeout: readTimeout, + WriteTimeout: writeTimeout, + IdleTimeout: idleTimeout, + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/cmd/cache-demo/main.go` at line 47, The server is created without timeouts which can allow slow-client resource exhaustion; update the http.Server returned in main (the &http.Server{Addr: ":" + strconv.Itoa(port), Handler: injector.Latency(injector.HTTP(mux))}) to include explicit timeouts such as ReadHeaderTimeout, ReadTimeout, WriteTimeout, and IdleTimeout (set to reasonable durations like a few seconds for reads/writes and a longer value for idle) so the demo servers are hardened against slow or malicious clients.
33-37: Useerrgroup.WithContextto align with established patterns in the codebase and enable graceful cancellationLines 33-37 use
new(errgroup.Group)without context propagation. While this doesn't causeWait()to hang (errors return immediately),demo/pkg/subgraphs/subgraphs.godemonstrates the preferred pattern witherrgroup.WithContext(), allowing other servers to shut down cleanly if one fails. Align this file with that convention.Additionally, the
http.Serverinstances (lines 47 and 54) lack timeout configuration (ReadTimeout,WriteTimeout,IdleTimeout), which should be set for production resilience.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/cmd/cache-demo/main.go` around lines 33 - 37, Replace new(errgroup.Group) with errgroup.WithContext to create a context-aware group (use the returned ctx and group variables), then start each server with g.Go(func() error { return srv.ListenAndServe() }) so that cancellation propagates and you can use ctx to trigger graceful shutdown of other servers when one fails; update places that call g.Wait() to use the new group variable. Also configure the http.Server instances (the Server constructors used to build elements of the servers slice) to set production timeouts: set ReadTimeout, WriteTimeout and IdleTimeout on those http.Server objects to avoid hanging connections and improve resiliency.benchmark/scripts/lib.ts (1)
13-103: Prefer interfaces for these exported object shapes.
Scenario,Manifest,RequestVariant,K6Stage,ComparableModeSummary,ModeComparison,ScenarioVariantSummary, andSuiteSummaryare all DTO-style object shapes. Converting them tointerfacewould match the repo’s TypeScript convention and make future extension/merging cleaner. As per coding guidelines, "Prefer interfaces over type aliases for object shapes in TypeScript".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/scripts/lib.ts` around lines 13 - 103, Replace the exported object-shaped type aliases with interfaces: change Scenario, Manifest, RequestVariant, K6Stage, ComparableModeSummary, ModeComparison, ScenarioVariantSummary, and SuiteSummary from "export type ..." to "export interface ..." preserving all properties and their types (including nested objects like cache/router/redis in ComparableModeSummary); update any imports/exports if needed and ensure no usage relies on union/tuple-specific type-alias features so the new interfaces remain structurally identical and extendable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmark/scripts/lib.ts`:
- Around line 229-231: The "cache_disabled" branch only sets
headers["X-WG-Disable-Entity-Cache"] but leaves root-field query caching active,
so update the branch that checks mode === "cache_disabled" to also disable
query-level caching (e.g., add headers["X-WG-Disable-Query-Cache"] = "true") so
the benchmark truly measures with all caches off; alternatively, if you prefer
semantics, rename the mode to "entity_cache_disabled" across the code where mode
is used to avoid misleading results—refer to the mode variable and the existing
headers["X-WG-Disable-Entity-Cache"] assignment to locate where to change.
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 4418-4420: The current early return inside buildExplicitMappings
(the block that calls buildCompositeIsMapping when isCompositeIsSpec is true)
stops processing other `@openfed__is/`@key directives and skips downstream
duplicate and non-key-arg checks; change the logic to not return immediately —
instead call buildCompositeIsMapping, push/append its resulting
EntityKeyMappingConfig(s) into the collection used by buildExplicitMappings, and
continue iterating so each `@key` is evaluated independently; ensure downstream
duplicate/non-key-argument validation still runs against the full list of
emitted mappings (functions/classes to touch: buildExplicitMappings,
buildCompositeIsMapping, and the aggregation/validation code that produces
EntityKeyMappingConfig).
- Around line 4939-4993: The nested-key pre-pass (loop over keyFieldSets)
currently pushes mappings produced by validateNestedInputObjectMapping directly
via results.push({ entityTypeName, fieldMappings }) and thus bypasses the
extra-argument invalidation logic applied later to flat-key mappings; update the
nested pre-pass so that after obtaining fieldMappings from
validateNestedInputObjectMapping you run the same extra-argument checks used for
flat keys (the logic around argumentInfos and the extra-argument handling at the
later block) — emitting errors for explicit `@openfed__is` uses and warnings for
auto-mapping, and if any unmapped arguments are found discard (do not push) the
mapping for that normalizedFieldSet; you can either extract the extra-argument
validation into a helper and call it here or inline the same checks before
calling results.push to ensure nested mappings are invalidated identically to
flat-key mappings.
- Around line 3902-3913: The current check in normalization-factory.ts only
enforces entity-cache when returnTypeData.kind === Kind.OBJECT_TYPE_DEFINITION,
allowing a field with openfed__queryCache returning an interface (or other
non-object kinds) to bypass the entity-cache requirement; update the logic in
the block that references returnTypeData, Kind.OBJECT_TYPE_DEFINITION and
entityCacheConfigByTypeName so that you validate presence of
this.entityCacheConfigByTypeName.has(returnTypeName) for the return type
regardless of it being an object (or explicitly include
INTERFACE_TYPE_DEFINITION/UNION as needed), and if the entity cache is missing
call queryCacheReturnEntityMissingEntityCacheWarning with the same args and
return early—i.e., remove the object-only gate and ensure the
openfed__queryCache return type always requires a corresponding
openfed__entityCache entry.
In `@composition/tests/v1/directives/entity-caching.test.ts`:
- Around line 226-238: The test calls normalizeSubgraphFailure without the
required version flag; update the call in the test (the normalizeSubgraphFailure
invocation inside the 'error: `@openfed__entityCache` negativeCacheTTL must be
non-negative' test) to pass version: ROUTER_COMPATIBILITY_VERSION_ONE (the same
pattern used by other tests) so batchNormalize receives the correct version
parameter.
In `@demo/config-cache-only.json`:
- Line 1: The composed config currently has conflicting Article entity TTLs
(Article `@openfed__entityCache`(maxAge: 120) in cachegraph vs maxAge: 90 in
cachegraph-ext) but the generated entityCacheConfigurations lists Article with
maxAgeSeconds "120"; update the composition output so the Article TTL uses the
minimum declared value (90) to avoid serving stale fields: locate the Article
`@openfed__entityCache` declarations in the cachegraph and cachegraph-ext SDLs and
either align both SDLs to maxAge 90 or change the composed
entityCacheConfigurations entry for "Article" to maxAgeSeconds "90"; then
rebuild the generated config so the serialized config, keys, and any
cache-related mappings reflect the chosen TTL consistently.
In `@demo/pkg/subgraphs/cachegraph/subgraph/data_race_test.go`:
- Line 21: Move the deadline initialization so the race timer starts after
worker goroutines are scheduled and ready: replace the existing early "deadline
:= time.Now().Add(100 * time.Millisecond)" (the three occurrences) with
assigning deadline only after all workers signal readiness (e.g., after a ready
channel is closed or after wg.Wait that confirms workers started); ensure the
worker loop uses that deadline variable unchanged, and keep the same timeout
value (100ms) so tests run the intended number of iterations under -race.
In `@demo/pkg/subgraphs/cachegraph/subgraph/schema.resolvers.go`:
- Around line 70-71: Guard against a nil location.Address before dereferencing
location.Address.ID in the queryResolver.Venue function: check if
location.Address == nil and handle that case (e.g., return nil with a
descriptive error or nil,nil as appropriate for your API) instead of directly
indexing venuesData[location.Address.ID]; update the Venue resolver to branch on
location.Address being nil and reference the same symbols (queryResolver.Venue,
location.Address.ID, venuesData) when implementing the fix.
In `@Makefile`:
- Around line 195-205: The Makefile demo startup loop gives go run
cmd/all/main.go too little time (~10s) to compile and bind ports 4012/4013/4014,
making make demo brittle; update the demo startup logic (the block that runs "go
run cmd/all/main.go", captures pid in variable pid, and loops nc -z against
ports 4012 4013 4014) to poll for readiness with a longer, configurable timeout
or more retries (or exponential backoff) instead of the current 20*0.5s
loop—e.g., increase iterations or implement a timeout loop that waits up to a
minute (or use a helper wait-for approach) before failing so the subgraphs have
enough time to compile and bind.
---
Nitpick comments:
In `@benchmark/scripts/lib.ts`:
- Around line 13-103: Replace the exported object-shaped type aliases with
interfaces: change Scenario, Manifest, RequestVariant, K6Stage,
ComparableModeSummary, ModeComparison, ScenarioVariantSummary, and SuiteSummary
from "export type ..." to "export interface ..." preserving all properties and
their types (including nested objects like cache/router/redis in
ComparableModeSummary); update any imports/exports if needed and ensure no usage
relies on union/tuple-specific type-alias features so the new interfaces remain
structurally identical and extendable.
In `@demo/cmd/cache-demo/main.go`:
- Line 47: The server is created without timeouts which can allow slow-client
resource exhaustion; update the http.Server returned in main (the
&http.Server{Addr: ":" + strconv.Itoa(port), Handler:
injector.Latency(injector.HTTP(mux))}) to include explicit timeouts such as
ReadHeaderTimeout, ReadTimeout, WriteTimeout, and IdleTimeout (set to reasonable
durations like a few seconds for reads/writes and a longer value for idle) so
the demo servers are hardened against slow or malicious clients.
- Around line 33-37: Replace new(errgroup.Group) with errgroup.WithContext to
create a context-aware group (use the returned ctx and group variables), then
start each server with g.Go(func() error { return srv.ListenAndServe() }) so
that cancellation propagates and you can use ctx to trigger graceful shutdown of
other servers when one fails; update places that call g.Wait() to use the new
group variable. Also configure the http.Server instances (the Server
constructors used to build elements of the servers slice) to set production
timeouts: set ReadTimeout, WriteTimeout and IdleTimeout on those http.Server
objects to avoid hanging connections and improve resiliency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 94c4a941-0add-4691-afcc-61253ad9c722
⛔ Files ignored due to path filters (2)
demo/pkg/subgraphs/cachegraph/subgraph/generated/federation.gois excluded by!**/generated/**demo/pkg/subgraphs/cachegraph/subgraph/generated/generated.gois excluded by!**/generated/**
📒 Files selected for processing (42)
Makefilebenchmark/fixtures/user_profile_header_sensitive.bob.response.jsonbenchmark/k6/cache_demo.jsbenchmark/scripts/lib.test.tsbenchmark/scripts/lib.tsbenchmark/scripts/scrape_metrics.tscli/cachegraph-cachedemo/cosmo-composition.yamlcomposition-go/index.global.jscomposition/src/errors/errors.tscomposition/src/router-configuration/types.tscomposition/src/utils/string-constants.tscomposition/src/v1/constants/constants.tscomposition/src/v1/constants/directive-definitions.tscomposition/src/v1/normalization/directive-definition-data.tscomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/warnings/params.tscomposition/src/v1/warnings/warnings.tscomposition/tests/v1/directives/entity-cache-fuzz.test.tscomposition/tests/v1/directives/entity-cache-mapping-rules.test.tscomposition/tests/v1/directives/entity-caching.test.tsdemo/cmd/cache-demo/main.godemo/config-cache-only.jsondemo/pkg/subgraphs/cachegraph/cachegraph.godemo/pkg/subgraphs/cachegraph/subgraph/data.godemo/pkg/subgraphs/cachegraph/subgraph/data_race_test.godemo/pkg/subgraphs/cachegraph/subgraph/entity.resolvers.godemo/pkg/subgraphs/cachegraph/subgraph/model/models_gen.godemo/pkg/subgraphs/cachegraph/subgraph/resolver.godemo/pkg/subgraphs/cachegraph/subgraph/schema.graphqlsdemo/pkg/subgraphs/cachegraph/subgraph/schema.resolvers.godemo/pkg/subgraphs/subgraphs.godocs/entity-caching/directives.mddocs/entity-caching/engineering-brief.mdgraphqlmetrics/README.mdplayground/src/components/playground/cache-explorer-controller.tsplayground/src/components/playground/cache-explorer-runner.tsrouter-tests/entity_caching/entity_caching_test.gorouter-tests/entity_caching/subgraphs/items/subgraph/schema.graphqlsrouter-tests/entity_caching/testdata/config.jsonrouter-tests/testenv/testenv.gorouter/internal/graphiql/graphiql.htmlshared/src/router-config/graphql-configuration.ts
✅ Files skipped from review due to trivial changes (4)
- benchmark/fixtures/user_profile_header_sensitive.bob.response.json
- benchmark/scripts/scrape_metrics.ts
- demo/pkg/subgraphs/cachegraph/subgraph/entity.resolvers.go
- benchmark/scripts/lib.test.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- cli/cachegraph-cachedemo/cosmo-composition.yaml
- demo/pkg/subgraphs/cachegraph/cachegraph.go
- demo/pkg/subgraphs/cachegraph/subgraph/resolver.go
- composition/src/v1/constants/constants.ts
- composition/src/v1/warnings/params.ts
- benchmark/k6/cache_demo.js
- composition/src/router-configuration/types.ts
- composition/src/v1/constants/directive-definitions.ts
- demo/pkg/subgraphs/cachegraph/subgraph/data.go
- composition/src/utils/string-constants.ts
- composition/tests/v1/directives/entity-cache-fuzz.test.ts
- demo/pkg/subgraphs/cachegraph/subgraph/schema.graphqls
- composition/src/errors/errors.ts
Comment on lines
+3902
to
+3913
| const returnTypeData = this.parentDefinitionDataByTypeName.get(returnTypeName); | ||
| const isObjectReturn = returnTypeData?.kind === Kind.OBJECT_TYPE_DEFINITION; | ||
| if (isObjectReturn && !this.entityCacheConfigByTypeName.has(returnTypeName)) { | ||
| this.warnings.push( | ||
| queryCacheReturnEntityMissingEntityCacheWarning({ | ||
| subgraphName: this.subgraphName, | ||
| fieldCoords, | ||
| entityType: returnTypeName, | ||
| }), | ||
| ); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Enforce the @entityCache prerequisite for every @queryCache entity return.
Lines 3902-3913 only gate on object returns, so a @openfed__queryCache field returning a keyed interface can still serialize a root-field cache config even though @openfed__entityCache is never present for that return type. That breaks the “query cache requires entity cache” rule and leaves the router with a query-cache config that has no guaranteed backing entity cache.
Based on learnings: The return type of a field with openfed__queryCache must have openfed__entityCache defined (Validation Rule 6).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composition/src/v1/normalization/normalization-factory.ts` around lines 3902
- 3913, The current check in normalization-factory.ts only enforces entity-cache
when returnTypeData.kind === Kind.OBJECT_TYPE_DEFINITION, allowing a field with
openfed__queryCache returning an interface (or other non-object kinds) to bypass
the entity-cache requirement; update the logic in the block that references
returnTypeData, Kind.OBJECT_TYPE_DEFINITION and entityCacheConfigByTypeName so
that you validate presence of
this.entityCacheConfigByTypeName.has(returnTypeName) for the return type
regardless of it being an object (or explicitly include
INTERFACE_TYPE_DEFINITION/UNION as needed), and if the entity cache is missing
call queryCacheReturnEntityMissingEntityCacheWarning with the same args and
return early—i.e., remove the object-only gate and ensure the
openfed__queryCache return type always requires a corresponding
openfed__entityCache entry.
| @@ -0,0 +1 @@ | |||
| {"engineConfig":{"defaultFlushInterval":"500","datasourceConfigurations":[{"kind":"GRAPHQL","rootNodes":[{"typeName":"Query","fieldNames":["article","articles","articlesByIds","articleBySlug","listing","listings","venue","venues","userProfile","catalog","catalogs","metric"]},{"typeName":"Mutation","fieldNames":["updateArticle","createArticle","deleteListing"]},{"typeName":"UserProfile","fieldNames":["id","username","email","role"]},{"typeName":"Catalog","fieldNames":["id","name","category","itemCount"]},{"typeName":"Metric","fieldNames":["id","name","value","unit"]},{"typeName":"Personalized","fieldNames":["id"]},{"typeName":"Viewer","fieldNames":["id","recommendedArticles"]},{"typeName":"Article","fieldNames":["id","slug","title","body","authorName","publishedAt","tags"]},{"typeName":"Listing","fieldNames":["sellerId","sku","title","price","currency","inStock"]},{"typeName":"Venue","fieldNames":["address","name","capacity","city"]}],"childNodes":[{"typeName":"Address","fieldNames":["id"]}],"overrideFieldPathFromAlias":true,"customGraphql":{"fetch":{"url":{"staticVariableContent":"http://localhost:4012/graphql"},"method":"POST","body":{},"baseUrl":{},"path":{}},"subscription":{"enabled":true,"url":{"staticVariableContent":"http://localhost:4012/graphql"},"protocol":"GRAPHQL_SUBSCRIPTION_PROTOCOL_WS","websocketSubprotocol":"GRAPHQL_WEBSOCKET_SUBPROTOCOL_AUTO"},"federation":{"enabled":true,"serviceSdl":"extend schema\n @link(\n url: \"https://specs.apollo.dev/federation/v2.5\"\n import: [\"@key\"]\n )\n\ndirective @openfed__entityCache(\n maxAge: Int!\n includeHeaders: Boolean = false\n partialCacheLoad: Boolean = false\n shadowMode: Boolean = false\n) on OBJECT\n\ndirective @openfed__queryCache(\n maxAge: Int!\n includeHeaders: Boolean = false\n shadowMode: Boolean = false\n) on FIELD_DEFINITION\n\ndirective @openfed__cacheInvalidate on FIELD_DEFINITION\n\ndirective @openfed__cachePopulate(maxAge: Int) on FIELD_DEFINITION\n\ndirective @openfed__is(fields: String!) on ARGUMENT_DEFINITION\n\ntype Query {\n \"\"\"Simple key lookup\"\"\"\n article(id: ID!): Article @openfed__queryCache(maxAge: 120)\n \"\"\"List query\"\"\"\n articles: [Article!]! @openfed__queryCache(maxAge: 120)\n \"\"\"Batch lookup with @openfed__is\"\"\"\n articlesByIds(ids: [ID!]! @openfed__is(fields: \"id\")): [Article!]! @openfed__queryCache(maxAge: 120)\n \"\"\"Argument remapping via @openfed__is\"\"\"\n articleBySlug(slug: String! @openfed__is(fields: \"slug\")): Article @openfed__queryCache(maxAge: 120)\n\n \"\"\"Composite key lookup via input object with @openfed__is\"\"\"\n listing(key: ListingKey! @openfed__is(fields: \"sellerId sku\")): Listing @openfed__queryCache(maxAge: 60)\n \"\"\"List of all listings\"\"\"\n listings: [Listing!]! @openfed__queryCache(maxAge: 60)\n\n \"\"\"Nested key lookup with @openfed__is and input object\"\"\"\n venue(location: VenueLocationKey! @openfed__is(fields: \"address { id }\")): Venue @openfed__queryCache(maxAge: 180)\n \"\"\"List of all venues\"\"\"\n venues: [Venue!]! @openfed__queryCache(maxAge: 180)\n\n \"\"\"Per-user profile (cache varies by Authorization header)\"\"\"\n userProfile(id: ID!): UserProfile @openfed__queryCache(maxAge: 60, includeHeaders: true)\n\n \"\"\"Single catalog entry\"\"\"\n catalog(id: ID!): Catalog @openfed__queryCache(maxAge: 120)\n \"\"\"All catalog entries (for partial cache load testing)\"\"\"\n catalogs: [Catalog!]! @openfed__queryCache(maxAge: 120)\n\n \"\"\"Single metric (shadow mode - always fetches from subgraph, compares with cache)\"\"\"\n metric(id: ID!): Metric @openfed__queryCache(maxAge: 300, shadowMode: true)\n}\n\ntype Mutation {\n updateArticle(id: ID!, title: String!): Article @openfed__cacheInvalidate\n createArticle(title: String!, body: String!, authorName: String!): Article! @openfed__cachePopulate(maxAge: 30)\n deleteListing(key: ListingKey!): Listing @openfed__cacheInvalidate\n}\n\n\"\"\"Per-user caching: includeHeaders makes the cache key include a hash of forwarded headers (e.g. Authorization)\"\"\"\ntype UserProfile @key(fields: \"id\") @openfed__entityCache(maxAge: 60, includeHeaders: true) {\n id: ID!\n username: String!\n email: String!\n role: String!\n}\n\n\"\"\"Partial cache load: when some entities are cached and others aren't, only the missing ones are fetched\"\"\"\ntype Catalog @key(fields: \"id\") @openfed__entityCache(maxAge: 120, partialCacheLoad: true) {\n id: ID!\n name: String!\n category: String!\n itemCount: Int!\n}\n\n\"\"\"Shadow mode: always fetches from subgraph but compares with cache for staleness detection\"\"\"\ntype Metric @key(fields: \"id\") @openfed__entityCache(maxAge: 300, shadowMode: true) {\n id: ID!\n name: String!\n value: Float!\n unit: String!\n}\n\ninput ListingKey {\n sellerId: ID!\n sku: String!\n}\n\ninput VenueLocationKey {\n address: VenueAddressKey!\n}\n\ninput VenueAddressKey {\n id: ID!\n}\n\ninterface Personalized @key(fields: \"id\") {\n id: ID!\n}\n\ntype Viewer @key(fields: \"id\") {\n id: ID!\n recommendedArticles: [Article!]!\n}\n\ntype Article implements Personalized @key(fields: \"id\") @key(fields: \"slug\") @openfed__entityCache(maxAge: 120) {\n id: ID!\n slug: String!\n title: String!\n body: String!\n authorName: String!\n publishedAt: String!\n tags: [String!]!\n}\n\ntype Listing @key(fields: \"sellerId sku\") @openfed__entityCache(maxAge: 60) {\n sellerId: ID!\n sku: String!\n title: String!\n price: Float!\n currency: String!\n inStock: Boolean!\n}\n\ntype Address {\n id: ID!\n}\n\ntype Venue @key(fields: \"address { id }\") @openfed__entityCache(maxAge: 180) {\n address: Address!\n name: String!\n capacity: Int!\n city: String!\n}\n"},"upstreamSchema":{"key":"fabc39bc398d33faa5e52491a3a5aa91a8058d1f"}},"requestTimeoutSeconds":"10","id":"0","keys":[{"typeName":"UserProfile","selectionSet":"id"},{"typeName":"Catalog","selectionSet":"id"},{"typeName":"Metric","selectionSet":"id"},{"typeName":"Personalized","selectionSet":"id"},{"typeName":"Viewer","selectionSet":"id"},{"typeName":"Article","selectionSet":"id"},{"typeName":"Article","selectionSet":"slug"},{"typeName":"Listing","selectionSet":"sellerId sku"},{"typeName":"Venue","selectionSet":"address { id }"}],"entityInterfaces":[{"interfaceTypeName":"Personalized","concreteTypeNames":["Article"]}],"entityCacheConfigurations":[{"typeName":"UserProfile","maxAgeSeconds":"60","includeHeaders":true},{"typeName":"Catalog","maxAgeSeconds":"120","partialCacheLoad":true},{"typeName":"Metric","maxAgeSeconds":"300","shadowMode":true},{"typeName":"Article","maxAgeSeconds":"120"},{"typeName":"Listing","maxAgeSeconds":"60"},{"typeName":"Venue","maxAgeSeconds":"180"}],"rootFieldCacheConfigurations":[{"fieldName":"article","maxAgeSeconds":"120","entityTypeName":"Article","entityKeyMappings":[{"entityTypeName":"Article","fieldMappings":[{"entityKeyField":"id","argumentPath":["id"]}]}]},{"fieldName":"articles","maxAgeSeconds":"120","entityTypeName":"Article"},{"fieldName":"articlesByIds","maxAgeSeconds":"120","entityTypeName":"Article","entityKeyMappings":[{"entityTypeName":"Article","fieldMappings":[{"entityKeyField":"id","argumentPath":["ids"],"isBatch":true}]}]},{"fieldName":"articleBySlug","maxAgeSeconds":"120","entityTypeName":"Article","entityKeyMappings":[{"entityTypeName":"Article","fieldMappings":[{"entityKeyField":"slug","argumentPath":["slug"]}]}]},{"fieldName":"listing","maxAgeSeconds":"60","entityTypeName":"Listing","entityKeyMappings":[{"entityTypeName":"Listing","fieldMappings":[{"entityKeyField":"sellerId","argumentPath":["key","sellerId"]},{"entityKeyField":"sku","argumentPath":["key","sku"]}]}]},{"fieldName":"listings","maxAgeSeconds":"60","entityTypeName":"Listing"},{"fieldName":"venue","maxAgeSeconds":"180","entityTypeName":"Venue","entityKeyMappings":[{"entityTypeName":"Venue","fieldMappings":[{"entityKeyField":"address.id","argumentPath":["location","address","id"]}]}]},{"fieldName":"venues","maxAgeSeconds":"180","entityTypeName":"Venue"},{"fieldName":"userProfile","maxAgeSeconds":"60","includeHeaders":true,"entityTypeName":"UserProfile","entityKeyMappings":[{"entityTypeName":"UserProfile","fieldMappings":[{"entityKeyField":"id","argumentPath":["id"]}]}]},{"fieldName":"catalog","maxAgeSeconds":"120","entityTypeName":"Catalog","entityKeyMappings":[{"entityTypeName":"Catalog","fieldMappings":[{"entityKeyField":"id","argumentPath":["id"]}]}]},{"fieldName":"catalogs","maxAgeSeconds":"120","entityTypeName":"Catalog"},{"fieldName":"metric","maxAgeSeconds":"300","shadowMode":true,"entityTypeName":"Metric","entityKeyMappings":[{"entityTypeName":"Metric","fieldMappings":[{"entityKeyField":"id","argumentPath":["id"]}]}]}],"cachePopulateConfigurations":[{"fieldName":"createArticle","operationType":"Mutation","maxAgeSeconds":"30","entityTypeName":"Article"}],"cacheInvalidateConfigurations":[{"fieldName":"updateArticle","operationType":"Mutation","entityTypeName":"Article"},{"fieldName":"deleteListing","operationType":"Mutation","entityTypeName":"Listing"}]},{"kind":"GRAPHQL","rootNodes":[{"typeName":"Article","fieldNames":["id","viewCount","rating","reviewSummary","relatedArticles","personalizedRecommendation"],"externalFieldNames":["currentViewer"]},{"typeName":"Viewer","fieldNames":["id"],"externalFieldNames":["name"]},{"typeName":"Catalog","fieldNames":["id","description","lastUpdated"]}],"overrideFieldPathFromAlias":true,"customGraphql":{"fetch":{"url":{"staticVariableContent":"http://localhost:4013/graphql"},"method":"POST","body":{},"baseUrl":{},"path":{}},"subscription":{"enabled":true,"url":{"staticVariableContent":"http://localhost:4013/graphql"},"protocol":"GRAPHQL_SUBSCRIPTION_PROTOCOL_WS","websocketSubprotocol":"GRAPHQL_WEBSOCKET_SUBPROTOCOL_AUTO"},"federation":{"enabled":true,"serviceSdl":"extend schema\n @link(\n url: \"https://specs.apollo.dev/federation/v2.5\"\n import: [\"@key\", \"@external\", \"@requires\"]\n )\n\ndirective @openfed__entityCache(\n maxAge: Int!\n includeHeaders: Boolean = false\n partialCacheLoad: Boolean = false\n shadowMode: Boolean = false\n) on OBJECT\n\ntype Article @key(fields: \"id\") @openfed__entityCache(maxAge: 90) {\n id: ID!\n currentViewer: Viewer @external\n viewCount: Int!\n rating: Float!\n reviewSummary: String!\n relatedArticles: [Article!]!\n personalizedRecommendation: String! @requires(fields: \"currentViewer { id name }\")\n}\n\ntype Viewer @key(fields: \"id\") {\n id: ID!\n name: String! @external\n}\n\n\"\"\"Extends Catalog with description from a second subgraph (for partial cache load testing)\"\"\"\ntype Catalog @key(fields: \"id\") @openfed__entityCache(maxAge: 120, partialCacheLoad: true) {\n id: ID!\n description: String!\n lastUpdated: String!\n}\n"},"upstreamSchema":{"key":"a2cd2a1b7cb5ebae92f1c56df2d8c7d3a56d19e1"}},"requestTimeoutSeconds":"10","id":"1","keys":[{"typeName":"Article","selectionSet":"id"},{"typeName":"Viewer","selectionSet":"id"},{"typeName":"Catalog","selectionSet":"id"}],"requires":[{"typeName":"Article","fieldName":"personalizedRecommendation","selectionSet":"currentViewer { id name }"}],"entityCacheConfigurations":[{"typeName":"Article","maxAgeSeconds":"90"},{"typeName":"Catalog","maxAgeSeconds":"120","partialCacheLoad":true}]},{"kind":"GRAPHQL","rootNodes":[{"typeName":"Personalized","fieldNames":["id","currentViewer"]},{"typeName":"Viewer","fieldNames":["id","name","email"]},{"typeName":"Query","fieldNames":["currentViewer"]},{"typeName":"Article","fieldNames":["id","currentViewer"]}],"overrideFieldPathFromAlias":true,"customGraphql":{"fetch":{"url":{"staticVariableContent":"http://localhost:4014/graphql"},"method":"POST","body":{},"baseUrl":{},"path":{}},"subscription":{"enabled":true,"url":{"staticVariableContent":"http://localhost:4014/graphql"},"protocol":"GRAPHQL_SUBSCRIPTION_PROTOCOL_WS","websocketSubprotocol":"GRAPHQL_WEBSOCKET_SUBPROTOCOL_AUTO"},"federation":{"enabled":true,"serviceSdl":"extend schema\n @link(\n url: \"https://specs.apollo.dev/federation/v2.5\"\n import: [\"@key\", \"@interfaceObject\", \"@inaccessible\"]\n )\n\ndirective @openfed__requestScoped(key: String!) on FIELD_DEFINITION\n\n# Symmetric @openfed__requestScoped: both Query.currentViewer and Personalized.currentViewer\n# declare key: \"currentViewer\". Their L1 cache entry is \"viewer.currentViewer\".\n# Whichever is resolved first populates L1; subsequent fields with the same key\n# inject from L1 and skip the fetch.\ntype Personalized @key(fields: \"id\") @interfaceObject {\n id: ID!\n currentViewer: Viewer @inaccessible @openfed__requestScoped(key: \"currentViewer\")\n}\n\ntype Viewer @key(fields: \"id\") {\n id: ID!\n name: String!\n email: String!\n}\n\ntype Query {\n currentViewer: Viewer @openfed__requestScoped(key: \"currentViewer\")\n}\n"},"upstreamSchema":{"key":"9ee951b49b83d66e073d83402d78cd8078181571"}},"requestTimeoutSeconds":"10","id":"2","keys":[{"typeName":"Personalized","selectionSet":"id"},{"typeName":"Viewer","selectionSet":"id"},{"typeName":"Article","selectionSet":"id"}],"interfaceObjects":[{"interfaceTypeName":"Personalized","concreteTypeNames":["Article"]}],"requestScopedFields":[{"fieldName":"currentViewer","typeName":"Personalized","l1Key":"viewer.currentViewer"},{"fieldName":"currentViewer","typeName":"Query","l1Key":"viewer.currentViewer"}]}],"fieldConfigurations":[{"typeName":"Query","fieldName":"article","argumentsConfiguration":[{"name":"id","sourceType":"FIELD_ARGUMENT"}]},{"typeName":"Query","fieldName":"articlesByIds","argumentsConfiguration":[{"name":"ids","sourceType":"FIELD_ARGUMENT"}]},{"typeName":"Query","fieldName":"articleBySlug","argumentsConfiguration":[{"name":"slug","sourceType":"FIELD_ARGUMENT"}]},{"typeName":"Query","fieldName":"listing","argumentsConfiguration":[{"name":"key","sourceType":"FIELD_ARGUMENT"}]},{"typeName":"Query","fieldName":"venue","argumentsConfiguration":[{"name":"location","sourceType":"FIELD_ARGUMENT"}]},{"typeName":"Query","fieldName":"userProfile","argumentsConfiguration":[{"name":"id","sourceType":"FIELD_ARGUMENT"}]},{"typeName":"Query","fieldName":"catalog","argumentsConfiguration":[{"name":"id","sourceType":"FIELD_ARGUMENT"}]},{"typeName":"Query","fieldName":"metric","argumentsConfiguration":[{"name":"id","sourceType":"FIELD_ARGUMENT"}]},{"typeName":"Mutation","fieldName":"updateArticle","argumentsConfiguration":[{"name":"id","sourceType":"FIELD_ARGUMENT"},{"name":"title","sourceType":"FIELD_ARGUMENT"}]},{"typeName":"Mutation","fieldName":"createArticle","argumentsConfiguration":[{"name":"title","sourceType":"FIELD_ARGUMENT"},{"name":"body","sourceType":"FIELD_ARGUMENT"},{"name":"authorName","sourceType":"FIELD_ARGUMENT"}]},{"typeName":"Mutation","fieldName":"deleteListing","argumentsConfiguration":[{"name":"key","sourceType":"FIELD_ARGUMENT"}]}],"graphqlSchema":"schema {\n query: Query\n mutation: Mutation\n}\n\ndirective @inaccessible on ARGUMENT_DEFINITION | ENUM | ENUM_VALUE | FIELD_DEFINITION | INPUT_FIELD_DEFINITION | INPUT_OBJECT | INTERFACE | OBJECT | SCALAR | UNION\n\ntype Query {\n \"\"\"Simple key lookup\"\"\"\n article(id: ID!): Article\n \"\"\"List query\"\"\"\n articles: [Article!]!\n \"\"\"Batch lookup with @openfed__is\"\"\"\n articlesByIds(ids: [ID!]!): [Article!]!\n \"\"\"Argument remapping via @openfed__is\"\"\"\n articleBySlug(slug: String!): Article\n \"\"\"Composite key lookup via input object with @openfed__is\"\"\"\n listing(key: ListingKey!): Listing\n \"\"\"List of all listings\"\"\"\n listings: [Listing!]!\n \"\"\"Nested key lookup with @openfed__is and input object\"\"\"\n venue(location: VenueLocationKey!): Venue\n \"\"\"List of all venues\"\"\"\n venues: [Venue!]!\n \"\"\"Per-user profile (cache varies by Authorization header)\"\"\"\n userProfile(id: ID!): UserProfile\n \"\"\"Single catalog entry\"\"\"\n catalog(id: ID!): Catalog\n \"\"\"All catalog entries (for partial cache load testing)\"\"\"\n catalogs: [Catalog!]!\n \"\"\"\n Single metric (shadow mode - always fetches from subgraph, compares with cache)\n \"\"\"\n metric(id: ID!): Metric\n currentViewer: Viewer\n}\n\ntype Mutation {\n updateArticle(id: ID!, title: String!): Article\n createArticle(title: String!, body: String!, authorName: String!): Article!\n deleteListing(key: ListingKey!): Listing\n}\n\n\"\"\"\nPer-user caching: includeHeaders makes the cache key include a hash of forwarded headers (e.g. Authorization)\n\"\"\"\ntype UserProfile {\n id: ID!\n username: String!\n email: String!\n role: String!\n}\n\n\"\"\"\nPartial cache load: when some entities are cached and others aren't, only the missing ones are fetched\n\"\"\"\ntype Catalog {\n id: ID!\n name: String!\n category: String!\n itemCount: Int!\n description: String!\n lastUpdated: String!\n}\n\n\"\"\"\nShadow mode: always fetches from subgraph but compares with cache for staleness detection\n\"\"\"\ntype Metric {\n id: ID!\n name: String!\n value: Float!\n unit: String!\n}\n\ninput ListingKey {\n sellerId: ID!\n sku: String!\n}\n\ninput VenueLocationKey {\n address: VenueAddressKey!\n}\n\ninput VenueAddressKey {\n id: ID!\n}\n\ninterface Personalized {\n id: ID!\n currentViewer: Viewer @inaccessible\n}\n\ntype Viewer {\n id: ID!\n recommendedArticles: [Article!]!\n name: String!\n email: String!\n}\n\ntype Listing {\n sellerId: ID!\n sku: String!\n title: String!\n price: Float!\n currency: String!\n inStock: Boolean!\n}\n\ntype Address {\n id: ID!\n}\n\ntype Venue {\n address: Address!\n name: String!\n capacity: Int!\n city: String!\n}\n\ntype Article implements Personalized {\n id: ID!\n slug: String!\n title: String!\n body: String!\n authorName: String!\n publishedAt: String!\n tags: [String!]!\n currentViewer: Viewer\n viewCount: Int!\n rating: Float!\n reviewSummary: String!\n relatedArticles: [Article!]!\n personalizedRecommendation: String!\n}","stringStorage":{"fabc39bc398d33faa5e52491a3a5aa91a8058d1f":"schema {\n query: Query\n mutation: Mutation\n}\n\ndirective @key(fields: openfed__FieldSet!, resolvable: Boolean = true) repeatable on INTERFACE | OBJECT\n\ndirective @link(as: String, for: link__Purpose, import: [link__Import], url: String!) repeatable on SCHEMA\n\ndirective @openfed__cacheInvalidate on FIELD_DEFINITION\n\ndirective @openfed__cachePopulate(maxAge: Int) on FIELD_DEFINITION\n\ndirective @openfed__entityCache(includeHeaders: Boolean = false, maxAge: Int!, negativeCacheTTL: Int = 0, partialCacheLoad: Boolean = false, shadowMode: Boolean = false) on OBJECT\n\ndirective @openfed__is(fields: String!) on ARGUMENT_DEFINITION\n\ndirective @openfed__queryCache(includeHeaders: Boolean = false, maxAge: Int!, shadowMode: Boolean = false) on FIELD_DEFINITION\n\ntype Address {\n id: ID!\n}\n\ntype Article implements Personalized @key(fields: \"id\") @key(fields: \"slug\") @openfed__entityCache(maxAge: 120) {\n authorName: String!\n body: String!\n id: ID!\n publishedAt: String!\n slug: String!\n tags: [String!]!\n title: String!\n}\n\n\"\"\"\nPartial cache load: when some entities are cached and others aren't, only the missing ones are fetched\n\"\"\"\ntype Catalog @key(fields: \"id\") @openfed__entityCache(maxAge: 120, partialCacheLoad: true) {\n category: String!\n id: ID!\n itemCount: Int!\n name: String!\n}\n\ntype Listing @key(fields: \"sellerId sku\") @openfed__entityCache(maxAge: 60) {\n currency: String!\n inStock: Boolean!\n price: Float!\n sellerId: ID!\n sku: String!\n title: String!\n}\n\ninput ListingKey {\n sellerId: ID!\n sku: String!\n}\n\n\"\"\"\nShadow mode: always fetches from subgraph but compares with cache for staleness detection\n\"\"\"\ntype Metric @key(fields: \"id\") @openfed__entityCache(maxAge: 300, shadowMode: true) {\n id: ID!\n name: String!\n unit: String!\n value: Float!\n}\n\ntype Mutation {\n createArticle(authorName: String!, body: String!, title: String!): Article! @openfed__cachePopulate(maxAge: 30)\n deleteListing(key: ListingKey!): Listing @openfed__cacheInvalidate\n updateArticle(id: ID!, title: String!): Article @openfed__cacheInvalidate\n}\n\ninterface Personalized @key(fields: \"id\") {\n id: ID!\n}\n\ntype Query {\n \"\"\"Simple key lookup\"\"\"\n article(id: ID!): Article @openfed__queryCache(maxAge: 120)\n \"\"\"Argument remapping via @openfed__is\"\"\"\n articleBySlug(slug: String! @openfed__is(fields: \"slug\")): Article @openfed__queryCache(maxAge: 120)\n \"\"\"List query\"\"\"\n articles: [Article!]! @openfed__queryCache(maxAge: 120)\n \"\"\"Batch lookup with @openfed__is\"\"\"\n articlesByIds(ids: [ID!]! @openfed__is(fields: \"id\")): [Article!]! @openfed__queryCache(maxAge: 120)\n \"\"\"Single catalog entry\"\"\"\n catalog(id: ID!): Catalog @openfed__queryCache(maxAge: 120)\n \"\"\"All catalog entries (for partial cache load testing)\"\"\"\n catalogs: [Catalog!]! @openfed__queryCache(maxAge: 120)\n \"\"\"Composite key lookup via input object with @openfed__is\"\"\"\n listing(key: ListingKey! @openfed__is(fields: \"sellerId sku\")): Listing @openfed__queryCache(maxAge: 60)\n \"\"\"List of all listings\"\"\"\n listings: [Listing!]! @openfed__queryCache(maxAge: 60)\n \"\"\"\n Single metric (shadow mode - always fetches from subgraph, compares with cache)\n \"\"\"\n metric(id: ID!): Metric @openfed__queryCache(maxAge: 300, shadowMode: true)\n \"\"\"Per-user profile (cache varies by Authorization header)\"\"\"\n userProfile(id: ID!): UserProfile @openfed__queryCache(maxAge: 60, includeHeaders: true)\n \"\"\"Nested key lookup with @openfed__is and input object\"\"\"\n venue(location: VenueLocationKey! @openfed__is(fields: \"address { id }\")): Venue @openfed__queryCache(maxAge: 180)\n \"\"\"List of all venues\"\"\"\n venues: [Venue!]! @openfed__queryCache(maxAge: 180)\n}\n\n\"\"\"\nPer-user caching: includeHeaders makes the cache key include a hash of forwarded headers (e.g. Authorization)\n\"\"\"\ntype UserProfile @key(fields: \"id\") @openfed__entityCache(maxAge: 60, includeHeaders: true) {\n email: String!\n id: ID!\n role: String!\n username: String!\n}\n\ntype Venue @key(fields: \"address { id }\") @openfed__entityCache(maxAge: 180) {\n address: Address!\n capacity: Int!\n city: String!\n name: String!\n}\n\ninput VenueAddressKey {\n id: ID!\n}\n\ninput VenueLocationKey {\n address: VenueAddressKey!\n}\n\ntype Viewer @key(fields: \"id\") {\n id: ID!\n recommendedArticles: [Article!]!\n}\n\nscalar link__Import\n\nenum link__Purpose {\n EXECUTION\n SECURITY\n}\n\nscalar openfed__FieldSet","a2cd2a1b7cb5ebae92f1c56df2d8c7d3a56d19e1":"directive @external on FIELD_DEFINITION | OBJECT\n\ndirective @key(fields: openfed__FieldSet!, resolvable: Boolean = true) repeatable on INTERFACE | OBJECT\n\ndirective @link(as: String, for: link__Purpose, import: [link__Import], url: String!) repeatable on SCHEMA\n\ndirective @openfed__entityCache(includeHeaders: Boolean = false, maxAge: Int!, negativeCacheTTL: Int = 0, partialCacheLoad: Boolean = false, shadowMode: Boolean = false) on OBJECT\n\ndirective @requires(fields: openfed__FieldSet!) on FIELD_DEFINITION\n\ntype Article @key(fields: \"id\") @openfed__entityCache(maxAge: 90) {\n currentViewer: Viewer @external\n id: ID!\n personalizedRecommendation: String! @requires(fields: \"currentViewer { id name }\")\n rating: Float!\n relatedArticles: [Article!]!\n reviewSummary: String!\n viewCount: Int!\n}\n\n\"\"\"\nExtends Catalog with description from a second subgraph (for partial cache load testing)\n\"\"\"\ntype Catalog @key(fields: \"id\") @openfed__entityCache(maxAge: 120, partialCacheLoad: true) {\n description: String!\n id: ID!\n lastUpdated: String!\n}\n\ntype Viewer @key(fields: \"id\") {\n id: ID!\n name: String! @external\n}\n\nscalar link__Import\n\nenum link__Purpose {\n EXECUTION\n SECURITY\n}\n\nscalar openfed__FieldSet","9ee951b49b83d66e073d83402d78cd8078181571":"schema {\n query: Query\n}\n\ndirective @inaccessible on ARGUMENT_DEFINITION | ENUM | ENUM_VALUE | FIELD_DEFINITION | INPUT_FIELD_DEFINITION | INPUT_OBJECT | INTERFACE | OBJECT | SCALAR | UNION\n\ndirective @interfaceObject on OBJECT\n\ndirective @key(fields: openfed__FieldSet!, resolvable: Boolean = true) repeatable on INTERFACE | OBJECT\n\ndirective @link(as: String, for: link__Purpose, import: [link__Import], url: String!) repeatable on SCHEMA\n\ndirective @openfed__requestScoped(key: String!) on FIELD_DEFINITION\n\ntype Personalized @key(fields: \"id\") @interfaceObject {\n currentViewer: Viewer @inaccessible @openfed__requestScoped(key: \"currentViewer\")\n id: ID!\n}\n\ntype Query {\n currentViewer: Viewer @openfed__requestScoped(key: \"currentViewer\")\n}\n\ntype Viewer @key(fields: \"id\") {\n email: String!\n id: ID!\n name: String!\n}\n\nscalar link__Import\n\nenum link__Purpose {\n EXECUTION\n SECURITY\n}\n\nscalar openfed__FieldSet"},"graphqlClientSchema":"schema {\n query: Query\n mutation: Mutation\n}\n\ntype Query {\n \"\"\"Simple key lookup\"\"\"\n article(id: ID!): Article\n \"\"\"List query\"\"\"\n articles: [Article!]!\n \"\"\"Batch lookup with @openfed__is\"\"\"\n articlesByIds(ids: [ID!]!): [Article!]!\n \"\"\"Argument remapping via @openfed__is\"\"\"\n articleBySlug(slug: String!): Article\n \"\"\"Composite key lookup via input object with @openfed__is\"\"\"\n listing(key: ListingKey!): Listing\n \"\"\"List of all listings\"\"\"\n listings: [Listing!]!\n \"\"\"Nested key lookup with @openfed__is and input object\"\"\"\n venue(location: VenueLocationKey!): Venue\n \"\"\"List of all venues\"\"\"\n venues: [Venue!]!\n \"\"\"Per-user profile (cache varies by Authorization header)\"\"\"\n userProfile(id: ID!): UserProfile\n \"\"\"Single catalog entry\"\"\"\n catalog(id: ID!): Catalog\n \"\"\"All catalog entries (for partial cache load testing)\"\"\"\n catalogs: [Catalog!]!\n \"\"\"\n Single metric (shadow mode - always fetches from subgraph, compares with cache)\n \"\"\"\n metric(id: ID!): Metric\n currentViewer: Viewer\n}\n\ntype Mutation {\n updateArticle(id: ID!, title: String!): Article\n createArticle(title: String!, body: String!, authorName: String!): Article!\n deleteListing(key: ListingKey!): Listing\n}\n\n\"\"\"\nPer-user caching: includeHeaders makes the cache key include a hash of forwarded headers (e.g. Authorization)\n\"\"\"\ntype UserProfile {\n id: ID!\n username: String!\n email: String!\n role: String!\n}\n\n\"\"\"\nPartial cache load: when some entities are cached and others aren't, only the missing ones are fetched\n\"\"\"\ntype Catalog {\n id: ID!\n name: String!\n category: String!\n itemCount: Int!\n description: String!\n lastUpdated: String!\n}\n\n\"\"\"\nShadow mode: always fetches from subgraph but compares with cache for staleness detection\n\"\"\"\ntype Metric {\n id: ID!\n name: String!\n value: Float!\n unit: String!\n}\n\ninput ListingKey {\n sellerId: ID!\n sku: String!\n}\n\ninput VenueLocationKey {\n address: VenueAddressKey!\n}\n\ninput VenueAddressKey {\n id: ID!\n}\n\ninterface Personalized {\n id: ID!\n}\n\ntype Viewer {\n id: ID!\n recommendedArticles: [Article!]!\n name: String!\n email: String!\n}\n\ntype Listing {\n sellerId: ID!\n sku: String!\n title: String!\n price: Float!\n currency: String!\n inStock: Boolean!\n}\n\ntype Address {\n id: ID!\n}\n\ntype Venue {\n address: Address!\n name: String!\n capacity: Int!\n city: String!\n}\n\ntype Article implements Personalized {\n id: ID!\n slug: String!\n title: String!\n body: String!\n authorName: String!\n publishedAt: String!\n tags: [String!]!\n currentViewer: Viewer\n viewCount: Int!\n rating: Float!\n reviewSummary: String!\n relatedArticles: [Article!]!\n personalizedRecommendation: String!\n}"},"version":"00000000-0000-0000-0000-000000000000","subgraphs":[{"id":"0","name":"cachegraph","routingUrl":"http://localhost:4012/graphql"},{"id":"1","name":"cachegraph-ext","routingUrl":"http://localhost:4013/graphql"},{"id":"2","name":"viewer","routingUrl":"http://localhost:4014/graphql"}],"compatibilityVersion":"1:{{$COMPOSITION__VERSION}}"} No newline at end of file | |||
Contributor
There was a problem hiding this comment.
Resolve the conflicting Article entity TTL before shipping this demo config.
This config serializes Article @openfed__entityCache(maxAge: 120) in the cachegraph SDL and Article @openfed__entityCache(maxAge: 90) in the cachegraph-ext SDL, but collapses them into one entityCacheConfigurations entry of 120. If the router shares one TTL per entity type, fields coming from cachegraph-ext can now be served 30s past their declared freshness. Composition should either reject divergent entity TTLs or serialize the minimum TTL instead. Based on learnings, "When modifying entity caching logic, understand the key mapping pipeline..." and rebuild the generated config after composition/shared changes.
🧰 Tools
🪛 Betterleaks (1.1.2)
[high] 1-1: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 1-1: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 1-1: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@demo/config-cache-only.json` at line 1, The composed config currently has
conflicting Article entity TTLs (Article `@openfed__entityCache`(maxAge: 120) in
cachegraph vs maxAge: 90 in cachegraph-ext) but the generated
entityCacheConfigurations lists Article with maxAgeSeconds "120"; update the
composition output so the Article TTL uses the minimum declared value (90) to
avoid serving stale fields: locate the Article `@openfed__entityCache`
declarations in the cachegraph and cachegraph-ext SDLs and either align both
SDLs to maxAge 90 or change the composed entityCacheConfigurations entry for
"Article" to maxAgeSeconds "90"; then rebuild the generated config so the
serialized config, keys, and any cache-related mappings reflect the chosen TTL
consistently.
…ment - Run prettier on composition/src/v1/normalization/normalization-factory.ts. - Move userProfileIDByAuthorizationToken out of schema.resolvers.go (which gqlgen regenerates) into a sibling auth_profiles.go so it survives `go generate`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
demo/pkg/subgraphs/cachegraph/subgraph/schema.resolvers.go (1)
61-62:⚠️ Potential issue | 🔴 CriticalGuard nullable
location.Addressbefore dereferencing.Line 62 can panic when
location.Addressis nil.💡 Proposed fix
import ( "context" + "fmt" "github.com/wundergraph/cosmo/demo/pkg/injector" "github.com/wundergraph/cosmo/demo/pkg/subgraphs/cachegraph/subgraph/generated" "github.com/wundergraph/cosmo/demo/pkg/subgraphs/cachegraph/subgraph/model" ) @@ func (r *queryResolver) Venue(ctx context.Context, location model.VenueLocationKey) (*model.Venue, error) { + if location.Address == nil { + return nil, fmt.Errorf("location.address is required") + } return venuesData[location.Address.ID], nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demo/pkg/subgraphs/cachegraph/subgraph/schema.resolvers.go` around lines 61 - 62, In Venue, guard against a nil location.Address before accessing location.Address.ID to avoid a panic: inside func (r *queryResolver) Venue(ctx context.Context, location model.VenueLocationKey) (*model.Venue, error) check if location.Address == nil and return an appropriate result (e.g., nil, nil or nil, fmt.Errorf("missing address in location")) instead of dereferencing; then proceed to look up venuesData[location.Address.ID] only when Address is non-nil.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 3845-3854: The issue is that extractQueryCacheConfig /
extractCacheInvalidateConfig / extractCachePopulateConfig are storing configs
under the schema-local parentTypeName (e.g. MyQuery) but the rest of
normalization expects the normalized root key (e.g. Query); change these calls
to use the normalized root key instead of raw parentTypeName: compute a
normalizedRootKey for the root field (e.g. via a helper like
getNormalizedTypeName(parentTypeName) or by mapping schema-local root names to
the normalized names used elsewhere) and pass that normalizedRootKey into
extractQueryCacheConfig, extractCacheInvalidateConfig, and
extractCachePopulateConfig (and ensure rootFieldCacheConfigurations /
cacheInvalidateConfigurations / cachePopulateConfigurations are indexed by that
normalized key); keep the validateIsDirectivePlacement call using
fieldCoords/hasQueryCache as-is.
- Around line 4122-4131: typesMatchIncludingListShape currently requires
NON_NULL_TYPE parity and rejects equivalent types that differ only by
nullability; change it to transparently unwrap any NON_NULL_TYPE wrappers on
either side before comparing (i.e., if expected.kind === Kind.NON_NULL_TYPE or
got.kind === Kind.NON_NULL_TYPE then recurse into expected.type or got.type
accordingly) so named/list comparisons ignore NonNull wrappers; apply the same
fix to the analogous block around lines 4242-4245 and ensure the function still
recurses for LIST_TYPE and handles NAMED_TYPE equality by name.
- Around line 4848-4854: The lookup in buildCompositeIsMapping is comparing the
raw argInfo.isFieldValue string to the normalized map keys, so normalize the
composite `@openfed__is(fields:)` value before doing the keyFieldSets loop:
parse argInfo.isFieldValue into a KeyFieldSetData/canonical string by trimming
and collapsing whitespace, splitting into field tokens, sorting tokens
lexicographically, and joining with a single space (the same normalization used
when creating keyFieldSets), then compare that canonical string (instead of raw
isFieldValue) against each normalizedFieldSet; update references in
buildCompositeIsMapping to use that normalized variable.
---
Duplicate comments:
In `@demo/pkg/subgraphs/cachegraph/subgraph/schema.resolvers.go`:
- Around line 61-62: In Venue, guard against a nil location.Address before
accessing location.Address.ID to avoid a panic: inside func (r *queryResolver)
Venue(ctx context.Context, location model.VenueLocationKey) (*model.Venue,
error) check if location.Address == nil and return an appropriate result (e.g.,
nil, nil or nil, fmt.Errorf("missing address in location")) instead of
dereferencing; then proceed to look up venuesData[location.Address.ID] only when
Address is non-nil.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33dad95f-a85e-4447-972f-7bcf5a47ef50
⛔ Files ignored due to path filters (2)
router-tests/go.sumis excluded by!**/*.sumrouter/go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
composition/src/v1/normalization/normalization-factory.tsdemo/go.moddemo/pkg/subgraphs/cachegraph/subgraph/auth_profiles.godemo/pkg/subgraphs/cachegraph/subgraph/schema.resolvers.goplayground/package.jsonrouter-tests/go.modrouter-tests/testenv/testenv.gorouter/core/graph_server.gorouter/core/router.gorouter/core/supervisor_instance.gorouter/go.modrouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/config_test.gorouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.json
✅ Files skipped from review due to trivial changes (1)
- demo/go.mod
Comment on lines
+3845
to
+3854
| if (hasQueryCache) { | ||
| this.extractQueryCacheConfig(parentTypeName, fieldName, fieldData, operationType); | ||
| } | ||
| if (hasCacheInvalidate) { | ||
| this.extractCacheInvalidateConfig(parentTypeName, fieldName, fieldData, operationType); | ||
| } | ||
| if (hasCachePopulate) { | ||
| this.extractCachePopulateConfig(parentTypeName, fieldName, fieldData, operationType); | ||
| } | ||
| this.validateIsDirectivePlacement(fieldCoords, fieldData, hasQueryCache); |
Contributor
There was a problem hiding this comment.
Key root-field cache configs by the normalized root type.
These extractors store rootFieldCacheConfigurations / cacheInvalidateConfigurations / cachePopulateConfigurations under parentTypeName, but this loop iterates schema-local root names. With schema { query: MyQuery }, the configs end up on MyQuery while the rest of normalization uses the normalized root key (Query), so the router config silently misses them for renamed roots.
Suggested fix
- processRootFieldCacheDirectives() {
+ processRootFieldCacheDirectives() {
for (const [parentTypeName, parentData] of this.parentDefinitionDataByTypeName) {
if (parentData.kind !== Kind.OBJECT_TYPE_DEFINITION) {
continue;
}
+ const configTypeName = getParentTypeName(parentData);
const operationType = this.getOperationTypeNodeForRootTypeName(parentTypeName);
if (!operationType) {
continue;
}
@@
if (hasQueryCache) {
- this.extractQueryCacheConfig(parentTypeName, fieldName, fieldData, operationType);
+ this.extractQueryCacheConfig(parentTypeName, configTypeName, fieldName, fieldData, operationType);
}
if (hasCacheInvalidate) {
- this.extractCacheInvalidateConfig(parentTypeName, fieldName, fieldData, operationType);
+ this.extractCacheInvalidateConfig(parentTypeName, configTypeName, fieldName, fieldData, operationType);
}
if (hasCachePopulate) {
- this.extractCachePopulateConfig(parentTypeName, fieldName, fieldData, operationType);
+ this.extractCachePopulateConfig(parentTypeName, configTypeName, fieldName, fieldData, operationType);
} extractQueryCacheConfig(
parentTypeName: string,
+ configTypeName: string,
fieldName: string,
fieldData: FieldData,
operationType: OperationTypeNode,
) {
@@
- const configData = this.getCacheConfigurationData(parentTypeName, false);
+ const configData = this.getCacheConfigurationData(configTypeName, false);Also applies to: 3919-3930, 3957-3965, 4008-4017
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composition/src/v1/normalization/normalization-factory.ts` around lines 3845
- 3854, The issue is that extractQueryCacheConfig / extractCacheInvalidateConfig
/ extractCachePopulateConfig are storing configs under the schema-local
parentTypeName (e.g. MyQuery) but the rest of normalization expects the
normalized root key (e.g. Query); change these calls to use the normalized root
key instead of raw parentTypeName: compute a normalizedRootKey for the root
field (e.g. via a helper like getNormalizedTypeName(parentTypeName) or by
mapping schema-local root names to the normalized names used elsewhere) and pass
that normalizedRootKey into extractQueryCacheConfig,
extractCacheInvalidateConfig, and extractCachePopulateConfig (and ensure
rootFieldCacheConfigurations / cacheInvalidateConfigurations /
cachePopulateConfigurations are indexed by that normalized key); keep the
validateIsDirectivePlacement call using fieldCoords/hasQueryCache as-is.
Comment on lines
+4122
to
+4131
| typesMatchIncludingListShape(expected: TypeNode, got: TypeNode): boolean { | ||
| if (expected.kind !== got.kind) { | ||
| return false; | ||
| } | ||
| if (expected.kind === Kind.NAMED_TYPE) { | ||
| return expected.name.value === (got as typeof expected).name.value; | ||
| } | ||
| if (expected.kind === Kind.NON_NULL_TYPE || expected.kind === Kind.LIST_TYPE) { | ||
| return this.typesMatchIncludingListShape(expected.type, (got as typeof expected).type); | ||
| } |
Contributor
There was a problem hiding this comment.
Ignore NonNull wrappers when validating nested key leaf types.
typesMatchIncludingListShape() currently requires exact NON_NULL_TYPE parity, so nested mappings reject valid cases like entity key store.id: ID! vs input field store.id: ID. This helper is used by both composite @openfed__is and nested auto-mapping, so both paths become stricter than the repo rules.
Suggested fix
+ private stripNonNull(typeNode: TypeNode): TypeNode {
+ return typeNode.kind === Kind.NON_NULL_TYPE ? this.stripNonNull(typeNode.type) : typeNode;
+ }
+
typesMatchIncludingListShape(expected: TypeNode, got: TypeNode): boolean {
- if (expected.kind !== got.kind) {
+ const normalizedExpected = this.stripNonNull(expected);
+ const normalizedGot = this.stripNonNull(got);
+
+ if (normalizedExpected.kind !== normalizedGot.kind) {
return false;
}
- if (expected.kind === Kind.NAMED_TYPE) {
- return expected.name.value === (got as typeof expected).name.value;
+ if (normalizedExpected.kind === Kind.NAMED_TYPE) {
+ return normalizedExpected.name.value === (normalizedGot as typeof normalizedExpected).name.value;
}
- if (expected.kind === Kind.NON_NULL_TYPE || expected.kind === Kind.LIST_TYPE) {
- return this.typesMatchIncludingListShape(expected.type, (got as typeof expected).type);
+ if (normalizedExpected.kind === Kind.LIST_TYPE) {
+ return this.typesMatchIncludingListShape(
+ normalizedExpected.type,
+ (normalizedGot as typeof normalizedExpected).type,
+ );
}
return false;
}As per coding guidelines: composition/src/v1/normalization/**/*.ts: Type checking in key mappings must unwrap NonNull for auto-mapping comparison but compare strictly for explicit @openfed__is directives, with nullability differences ignored.
Also applies to: 4242-4245
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composition/src/v1/normalization/normalization-factory.ts` around lines 4122
- 4131, typesMatchIncludingListShape currently requires NON_NULL_TYPE parity and
rejects equivalent types that differ only by nullability; change it to
transparently unwrap any NON_NULL_TYPE wrappers on either side before comparing
(i.e., if expected.kind === Kind.NON_NULL_TYPE or got.kind ===
Kind.NON_NULL_TYPE then recurse into expected.type or got.type accordingly) so
named/list comparisons ignore NonNull wrappers; apply the same fix to the
analogous block around lines 4242-4245 and ensure the function still recurses
for LIST_TYPE and handles NAMED_TYPE equality by name.
Test isolation and determinism (codex-escalated blockers) - Add entityCachingL1OnlyOptions / entityCachingL2OnlyOptions / entityCachingDisabledOptions helpers in router-tests harness. - Switch every L2/... subtest in entity_caching_test.go (41 sites) to the L2-only helper so an L1 layer can no longer accidentally satisfy an L2 assertion. Switch L1/request-scoped nested dedup and the skipped L1/request-scoped widening refetch to the L1-only helper. Rename two misnamed L1/... tests (list batch caches across requests, field widening across requests) to L2/... and switch them too. - Delete L1/deduplicates repeated entity loads — its own header comment admitted the planner-merge made the assertion vacuous regardless of L1 state; the truthful coordinate-L1 dedup assertion lives in request_scoped_nested_dedup. - Add inverse L1-disabled/request-scoped nested no-dedup baseline test: with the coordinate cache disabled the viewer subgraph is hit three times (root + two BatchEntity sites), pinning the dedup test against silent regressions if the planner ever started merging on its own. - Lock single expected outcome on four entity-cache fuzz tests (cases 6, 10, 15, 16) that previously accepted both success and failure — they now assert specific composition errors or specific RootFieldCacheConfig output as observed. Demo otel alignment with router - demo/go.mod: bump otel require entries to v1.39.0 and add the same replace block router/go.mod uses to pin the full otel set to v1.28.0. Removes the v1.36.0 drift CodeRabbit flagged without doing a repo-wide jump that would change router behavior. Benchmark harness robustness - stop_stack.sh: poll kill -0 for up to 5s before SIGKILL, then remove the pid file. Previous wait was a no-op for non-child PIDs created via start_new_session. - capture_pprof.sh and wait_ready.sh: explicit --connect-timeout / --max-time on every curl probe and timeout(1) around docker exec, so a stuck endpoint can't hang the harness past the intended deadline. - run_suite.ts parseArgs: validate --vus (positive int), --duration / --ramp-up / --ramp-down (e.g. 30s / 2m / 1h / 500ms or raw ms), and reject missing values / unknown flags. Gate the CLI entry block on isMainModule() so the parser is unit-testable. New run_suite.test.ts covers the validation paths. Docs - composition/AGENTS.md: escape @openfed\_\_… on three rows that were rendering as @openfed**…. - docs/entity-caching/ENTITY_CACHING_DEMO.md: declare ```text on six fenced ASCII-diagram blocks (markdownlint MD040). - docs/entity-caching/directives.md and engineering-brief.md: rename the @openfed__is argument from `field:` to `fields:` to match the implemented directive shape. PLAN.md - Capture the triage that produced this commit (CodeRabbit + SkArchon feedback, codex second opinion, fix/ignore/discuss decisions, and the suggested commit ordering this commit collapses). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Dependency ReviewThe following issues were found:
Vulnerabilitiesdemo/go.modOnly included vulnerabilities with severity high or higher. OpenSSF ScorecardScorecard details
Scanned Files
|
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
benchmark/scripts/run_suite.ts (2)
174-182: Consider adding a timeout to prevent indefinite hangs.
execFileSyncwithout atimeoutoption can block forever if the subprocess hangs (e.g., waiting on a stuck Docker container or network). For benchmark reliability, consider adding a generous timeout.💡 Suggested improvement
function run(command: string, args: string[], env?: Record<string, string>): void { execFileSync(command, args, { stdio: "inherit", + timeout: 10 * 60 * 1000, // 10 minutes env: { ...process.env, ...env, }, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/scripts/run_suite.ts` around lines 174 - 182, The run function uses execFileSync without a timeout which can hang indefinitely; update run (the function named "run" that calls execFileSync) to pass a timeout option (e.g., a generous milliseconds value) into the execFileSync options object and handle the thrown TimeoutError accordingly (bubble up or catch and log/exit) so benchmark runs cannot block forever; ensure the env merge and stdio remain unchanged when adding the timeout option.
516-529: UsepathToFileURLfromnode:urlto properly resolve relative paths in ESM fallback.When
process.argv[1]contains a relative path (e.g.,tsx ./scripts/run_suite.ts), the currentnew URL(entry, "file:").hrefproduces an incomplete file URL likefile:///scripts/run_suite.tsinstead of the absolute path required for comparison withimport.meta.url. This causes the ESM fallback to fail when matching against the actual module URL.💡 Suggested improvement
+import { pathToFileURL } from "node:url"; + function isMainModule(): boolean { const entry = process.argv[1]; if (!entry) return false; try { return path.resolve(entry) === path.resolve(__filename); } catch { - return import.meta.url === new URL(entry, "file:").href; + return import.meta.url === pathToFileURL(path.resolve(entry)).href; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/scripts/run_suite.ts` around lines 516 - 529, The ESM fallback in isMainModule() can mis-handle relative argv paths; import pathToFileURL from 'node:url' and in the catch block convert the resolved entry to a file URL using pathToFileURL(path.resolve(entry)).href (instead of new URL(entry, "file:").href), and add the pathToFileURL import at the top of the module so import.meta.url compares correctly for relative invocations; update the catch block in isMainModule() to use that conversion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composition/AGENTS.md`:
- Line 76: The markdown has an unresolved reference label "8" caused by the
token `FieldMappingConfig` → `EntityCacheFieldMapping`; fix by either replacing
the arrowed token with plain text or inline code (e.g., `FieldMappingConfig` →
`EntityCacheFieldMapping`) or by adding a matching reference definition for
`[8]: ...` below the document; update the occurrence of
FieldMappingConfig/EntityCacheFieldMapping in AGENTS.md so it no longer relies
on a missing reference label.
In `@demo/go.mod`:
- Around line 191-210: The replace block pins multiple OpenTelemetry modules too
low (notably go.opentelemetry.io/otel/sdk at v1.28.0); update
go.opentelemetry.io/otel/sdk to at least v1.40.0 and align related otel modules
in the replace block (go.opentelemetry.io/otel, go.opentelemetry.io/otel/metric,
go.opentelemetry.io/otel/trace, and any otlp/exporters) to compatible versions
meeting the security minimum, then run `go mod tidy` to ensure transitive
versions are resolved consistently; target the same or newer versions used by
router/go.mod but ensure go.opentelemetry.io/otel/sdk is >= v1.40.0.
---
Nitpick comments:
In `@benchmark/scripts/run_suite.ts`:
- Around line 174-182: The run function uses execFileSync without a timeout
which can hang indefinitely; update run (the function named "run" that calls
execFileSync) to pass a timeout option (e.g., a generous milliseconds value)
into the execFileSync options object and handle the thrown TimeoutError
accordingly (bubble up or catch and log/exit) so benchmark runs cannot block
forever; ensure the env merge and stdio remain unchanged when adding the timeout
option.
- Around line 516-529: The ESM fallback in isMainModule() can mis-handle
relative argv paths; import pathToFileURL from 'node:url' and in the catch block
convert the resolved entry to a file URL using
pathToFileURL(path.resolve(entry)).href (instead of new URL(entry,
"file:").href), and add the pathToFileURL import at the top of the module so
import.meta.url compares correctly for relative invocations; update the catch
block in isMainModule() to use that conversion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3df25b59-db0a-4886-9515-4f2e218a0a37
⛔ Files ignored due to path filters (1)
demo/go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
PLAN.mdbenchmark/scripts/capture_pprof.shbenchmark/scripts/run_suite.test.tsbenchmark/scripts/run_suite.tsbenchmark/scripts/stop_stack.shbenchmark/scripts/wait_ready.shcomposition/AGENTS.mdcomposition/tests/v1/directives/entity-cache-fuzz.test.tsdemo/go.moddocs/entity-caching/ENTITY_CACHING_DEMO.mddocs/entity-caching/directives.mddocs/entity-caching/engineering-brief.mdrouter-tests/entity_caching/entity_caching_test.gorouter-tests/entity_caching/harness_test.go
✅ Files skipped from review due to trivial changes (1)
- benchmark/scripts/stop_stack.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- benchmark/scripts/wait_ready.sh
- benchmark/scripts/capture_pprof.sh
- composition/tests/v1/directives/entity-cache-fuzz.test.ts
|
|
||
| - `EntityCacheConfig` → `EntityCacheConfiguration` | ||
| - `RootFieldCacheConfig` → `RootFieldCacheConfiguration` | ||
| - `FieldMappingConfig` → `EntityCacheFieldMapping` |
Contributor
There was a problem hiding this comment.
Fix unresolved Markdown reference label (MD052).
markdownlint-cli2 reports a missing reference definition ("8"). Please either define the missing reference label or rewrite that token as plain text/inline code so the doc passes lint consistently.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 76-76: Reference links and images should use a label that is defined
Missing link or image reference definition: "8"
(MD052, reference-links-images)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composition/AGENTS.md` at line 76, The markdown has an unresolved reference
label "8" caused by the token `FieldMappingConfig` → `EntityCacheFieldMapping`;
fix by either replacing the arrowed token with plain text or inline code (e.g.,
`FieldMappingConfig` → `EntityCacheFieldMapping`) or by adding a matching
reference definition for `[8]: ...` below the document; update the occurrence of
FieldMappingConfig/EntityCacheFieldMapping in AGENTS.md so it no longer relies
on a missing reference label.
Comment on lines
+191
to
+210
| // Keep otel pinned to the same versions as router/go.mod. Upstream v1.40+ renames | ||
| // attributes we rely on, so the require line picks v1.39.0 but every transitive | ||
| // is pinned to v1.28.0 via the replace block below (mirrors router/go.mod:178-194). | ||
| replace ( | ||
| go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp => go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.46.1 | ||
| go.opentelemetry.io/contrib/propagators/b3 => go.opentelemetry.io/contrib/propagators/b3 v1.23.0 | ||
| go.opentelemetry.io/contrib/propagators/jaeger => go.opentelemetry.io/contrib/propagators/jaeger v1.23.0 | ||
| go.opentelemetry.io/otel => go.opentelemetry.io/otel v1.28.0 | ||
| go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc => go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc v0.44.0 | ||
| go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp => go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp v0.44.0 | ||
| go.opentelemetry.io/otel/exporters/otlp/otlptrace => go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.23.1 | ||
| go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc => go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.23.1 | ||
| go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp => go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.23.1 | ||
| go.opentelemetry.io/otel/exporters/prometheus => go.opentelemetry.io/otel/exporters/prometheus v0.50.0 | ||
| go.opentelemetry.io/otel/metric => go.opentelemetry.io/otel/metric v1.28.0 | ||
| go.opentelemetry.io/otel/sdk => go.opentelemetry.io/otel/sdk v1.28.0 | ||
| go.opentelemetry.io/otel/sdk/metric => go.opentelemetry.io/otel/sdk/metric v1.28.0 | ||
| go.opentelemetry.io/otel/trace => go.opentelemetry.io/otel/trace v1.28.0 | ||
| go.opentelemetry.io/proto/otlp => go.opentelemetry.io/proto/otlp v1.1.0 | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking effective otel/sdk pin in demo/go.mod..."
awk '
BEGIN { in_replace=0; found=0 }
/^replace[[:space:]]*\(/ { in_replace=1; next }
in_replace && /^\)/ { in_replace=0 }
in_replace && $1=="go.opentelemetry.io/otel/sdk" && $2=="=>" {
found=1
print "replace target:", $0
ver=$4
gsub(/^v/, "", ver)
split(ver, a, ".")
if ((a[1] < 1) || (a[1]==1 && a[2] < 40)) {
print "FAIL: otel/sdk is pinned below v1.40.0 ->", $4
exit 2
} else {
print "PASS: otel/sdk meets minimum floor ->", $4
exit 0
}
}
END {
if (!found) {
print "No otel/sdk replace entry found; check resolved version via require/graph separately."
}
}
' demo/go.modRepository: wundergraph/cosmo
Length of output: 246
Pin otel/sdk to minimum v1.40.0 in the replace block.
The go.opentelemetry.io/otel/sdk replace directive at line 206 pins the version to v1.28.0, which is below the required minimum v1.40.0 (GHSA-9h8m-3fm2-qjrq / CVE-2026-24051). Update this and any other otel module versions in the replace block to meet the security requirement.
Problematic replace block (lines 191–210)
replace (
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp => go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.46.1
go.opentelemetry.io/contrib/propagators/b3 => go.opentelemetry.io/contrib/propagators/b3 v1.23.0
go.opentelemetry.io/contrib/propagators/jaeger => go.opentelemetry.io/contrib/propagators/jaeger v1.23.0
go.opentelemetry.io/otel => go.opentelemetry.io/otel v1.28.0
go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc => go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc v0.44.0
go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp => go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp v0.44.0
go.opentelemetry.io/otel/exporters/otlp/otlptrace => go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.23.1
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc => go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.23.1
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp => go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.23.1
go.opentelemetry.io/otel/exporters/prometheus => go.opentelemetry.io/otel/exporters/prometheus v0.50.0
go.opentelemetry.io/otel/metric => go.opentelemetry.io/otel/metric v1.28.0
go.opentelemetry.io/otel/sdk => go.opentelemetry.io/otel/sdk v1.28.0
go.opentelemetry.io/otel/sdk/metric => go.opentelemetry.io/otel/sdk/metric v1.28.0
go.opentelemetry.io/otel/trace => go.opentelemetry.io/otel/trace v1.28.0
go.opentelemetry.io/proto/otlp => go.opentelemetry.io/proto/otlp v1.1.0
)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@demo/go.mod` around lines 191 - 210, The replace block pins multiple
OpenTelemetry modules too low (notably go.opentelemetry.io/otel/sdk at v1.28.0);
update go.opentelemetry.io/otel/sdk to at least v1.40.0 and align related otel
modules in the replace block (go.opentelemetry.io/otel,
go.opentelemetry.io/otel/metric, go.opentelemetry.io/otel/trace, and any
otlp/exporters) to compatible versions meeting the security minimum, then run
`go mod tidy` to ensure transitive versions are resolved consistently; target
the same or newer versions used by router/go.mod but ensure
go.opentelemetry.io/otel/sdk is >= v1.40.0.
SkArchon
reviewed
Apr 23, 2026
| @@ -311,6 +334,136 @@ | |||
| } | |||
| } | |||
| }, | |||
| "entity_caching": { | |||
Contributor
There was a problem hiding this comment.
You would need to add options for telemetry.entity_caching, I noticed they were enabled by default.
(to follow the same pattern)
telemetry:
metrics:
otlp:
connection_stats: true
entity_caching_stats: true
prometheus:
connection_stats: true
entity_caching_stats: true
SkArchon
reviewed
Apr 23, 2026
| │ | ||
| ▼ | ||
| ┌─────────────────────────────────────────────────────────┐ | ||
| │ Coordinate L1 (requestScopedL1, per-request sync.Map) │ |
Contributor
There was a problem hiding this comment.
What is the difference between Coordinate L1, Entity L1, I looked at the tests but didn't help me understand (atleast without going through this doc in full detail).
…18) - #9: buildExplicitMappings accumulates composite @openfed__is mappings instead of returning on the first composite, so alternative @key directives on the same field still emit their own EntityKeyMappingConfig - #10: nested auto-mapping runs the same extra-argument invalidation check as flat-key mappings via new shared helper invalidateAutoMappingWithExtraArgument - #11: add missing `version` argument to the negativeCacheTTL failure test - #18: buildCompositeIsMapping normalizes argInfo.isFieldValue before key lookup so "sku id" matches an "id sku" key Thread #8 (coderabbit: error on @queryCache interface return) investigated and refuted — rules 37 / 37b in entity-cache-mapping-rules.test.ts assert this case must succeed with no warning because concrete implementations carry @entityCache. Thread #17 (coderabbit: unwrap NonNull in typesMatchIncludingListShape) investigated and refuted — rule 15b, 40, 40b, 40c encode the opposite semantics: explicit @openfed__is is lenient about NonNull when named type matches, nested auto-mapping is strict. composition/CLAUDE.md lines 142-146 were stale and misled review tooling; corrected in this commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Entity cache metrics were created whenever entity caching was enabled, ignoring the opt-in pattern used by connection_stats. Mirror the connection_stats convention: - Add entity_caching_stats bool (default false) to telemetry.metrics.otlp and telemetry.metrics.prometheus in config.schema.json - Matching Go struct fields with env bindings METRICS_OTLP_ENTITY_CACHING_STATS and PROMETHEUS_ENTITY_CACHING_STATS - Propagate through MetricConfigFromTelemetry into rmetric.Config - Gate OTLPEntityCache / PromEntityCache collector creation in setupEntityCacheMetrics on the per-backend flag - Parallel coverage in config_test.go, config_defaults.json, config_full.json, fixtures/full.yaml, and graph_server_cache_metrics_test.go Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
) - #3: shrink oversized multi-line comment on L1/request-scoped nested dedup to three lines that explain the invariant instead of reading like scratch debugging notes - #4: verified the L1 test has a meaningful assertion — counters.viewer == 1 with L1 on, == 3 with L1 off (inverse baseline exists); keep the test - #6: audit layer-named tests; L2/... subtests that used the combined entityCachingOptionsWithCircuitBreakerRef and entityCachingOptionsWithSubgraphConfig helpers are now L2-only. The one intentional hybrid is renamed L1/deduplicates with warm L2 → Combined/L1 deduplicates with warm L2 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- #13: start the race-test deadline after workers are scheduled so -race + parallel tests don't execute near-zero iterations. Applied in all three race tests (TestArticleStoreNoRace, TestListingStoreNoRace, TestResolverPathNoRace) - #14: nil-guard location.Address in the cachegraph Venue query resolver; returns "location.address is required" instead of panicking on nil deref - #15: parameterize `make demo` startup wait via DEMO_STARTUP_ATTEMPTS (default 60) and DEMO_STARTUP_SLEEP (default 0.5). Previous hard-coded 20 × 0.5s was too short on cold checkouts Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…review #7, #12) - #7: rename benchmark mode cache_disabled -> entity_cache_disabled so the label reflects what is actually disabled (X-WG-Disable-Entity-Cache only; there is no router header for query-cache). Updated in lib.ts, lib.test.ts, and scenarios/cache-demo.json. - #12: document cross-subgraph TTL divergence in docs/entity-caching/directives.md. The composed router config keeps per-datasource entityCacheConfigurations entries (demo/cachegraph Article maxAge: 120, demo/cachegraph-ext Article maxAge: 90 are preserved separately). Coderabbit's claim that composition collapses them was incorrect; the section now explicitly states the real behavior and recommends aligning maxAge values for shared entity types. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…view #22) Add a short up-front section at the top of docs/REQUEST_SCOPED.md that distinguishes the two per-request L1 caches: coordinate L1 (this document, keyed by {subgraph}.{@requestScoped key}) vs entity L1 (keyed by {subgraph}.{type}.{@key values}). Reader no longer has to read the full doc to tell them apart. #19 was investigated and is effectively refuted — composition/AGENTS.md has no `[N]` or `][` reference-link syntax for markdownlint MD052 to fire on, so the bot's missing-reference claim does not match the file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| let node = root; | ||
| let accumulated = ''; | ||
| for (const seg of segments) { | ||
| accumulated = accumulated === '' ? seg.replace(/^@\./, '@.') : accumulated + '.' + seg.replace(/^@\./, '@.'); |
| let node = root; | ||
| let accumulated = ''; | ||
| for (const seg of segments) { | ||
| accumulated = accumulated === '' ? seg.replace(/^@\./, '@.') : accumulated + '.' + seg.replace(/^@\./, '@.'); |
| let node = root; | ||
| let accumulated = ''; | ||
| for (const seg of segments) { | ||
| accumulated = accumulated === '' ? seg.replace(/^@\./, '@.') : accumulated + '.' + seg.replace(/^@\./, '@.'); |
| let node = root; | ||
| let accumulated = ''; | ||
| for (const seg of segments) { | ||
| accumulated = accumulated === '' ? seg.replace(/^@\./, '@.') : accumulated + '.' + seg.replace(/^@\./, '@.'); |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
composition/src/v1/normalization/normalization-factory.ts (3)
4122-4131:⚠️ Potential issue | 🟠 MajorDon’t enforce
NON_NULLparity on explicit composite@openfed__ismappings.
buildCompositeIsMapping()reusesvalidateNestedInputObjectMapping(), and that path currently callstypesMatchIncludingListShape()which requires exact nullability parity. That makes explicit composite mappings reject cases likeID→ID!, even though explicit@openfed__isis supposed to be strict on named type/list shape but tolerant of nullability differences.As per coding guidelines:
composition/src/v1/normalization/**/*.ts: Entity type argument-to-key mapping must compare types strictly: named type, list shape, and NonNull parity must all match for auto-mapping; explicit@openfed__istolerates nullability differences but requires strict named type and list shape matching.Also applies to: 4221-4245, 4894-4904
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 4122 - 4131, typesMatchIncludingListShape currently enforces exact NonNull parity; change its signature to accept an optional flag (e.g., ignoreNullability: boolean = false) and, when true, treat NON_NULL wrappers as transparent (unwrap and compare inner types without requiring got.kind === NON_NULL_TYPE) while still enforcing named type identity and list shape; then update validateNestedInputObjectMapping / buildCompositeIsMapping so explicit `@openfed__is` mapping flows call typesMatchIncludingListShape(..., true) (other callers keep the default false) to allow nullability differences but still require strict named type and list shape matching.
3896-3913:⚠️ Potential issue | 🟠 MajorKeep the entity-cache prerequisite for every
@openfed__queryCachereturn type.The guard only warns/returns for object returns. A keyed interface return still produces a
rootFieldCacheConfigurationeven thoughextractEntityCacheDirectives()never registers interface types inentityCacheConfigByTypeName, so the router gets query-cache metadata without a guaranteed backing entity cache.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 3896 - 3913, The current guard only checks object returns before warning and returning, which lets keyed interface/union returns produce rootFieldCacheConfiguration without an entity-cache backing; remove the isObjectReturn conditional so the code always verifies entityCacheConfigByTypeName.has(returnTypeName) and, if missing, pushes queryCacheReturnEntityMissingEntityCacheWarning and returns; update the check around returnTypeName/entityCacheConfigByTypeName in normalization-factory.ts (the block using returnTypeName, isObjectReturn, this.entityCacheConfigByTypeName, this.warnings.push, and queryCacheReturnEntityMissingEntityCacheWarning) so every `@openfed__queryCache` return type requires an entity-cache entry.
3820-3853:⚠️ Potential issue | 🟠 MajorStore root-field cache configs under the normalized root config key.
This loop iterates schema-local root names, but the rest of normalization keys root
ConfigurationDataoff the normalized root type. Withschema { query: MyQuery }, these cache configs land on a separateMyQueryentry and never get attached to theQueryroot node the router consumes.Also applies to: 3919-4017
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/normalization-factory.ts` around lines 3820 - 3853, The loop currently stores cache configs under the schema-local root type (parentTypeName) which causes configs for schema-root aliases (e.g. schema { query: MyQuery }) to be attached to MyQuery instead of the normalized root; fix by deriving the canonical/normalized root type from the operationType node returned by getOperationTypeNodeForRootTypeName(parentTypeName) (e.g. operationType.name.value) and pass that canonical root name into extractQueryCacheConfig, extractCacheInvalidateConfig and extractCachePopulateConfig instead of parentTypeName so all root-field cache configs are stored under the normalized root config key.
🧹 Nitpick comments (2)
benchmark/scripts/lib.ts (2)
13-103: Prefer interfaces for the exported object shapes.The new exported record-like types (
Scenario,Manifest,RequestVariant,K6Stage,ComparableModeSummary,ModeComparison,ScenarioVariantSummary,SuiteSummary) are all object shapes, so these should be interfaces instead of type aliases to match the repo’s TypeScript rule. As per coding guidelines, "Prefer interfaces over type aliases for object shapes in TypeScript".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/scripts/lib.ts` around lines 13 - 103, Replace the exported object-shaped type aliases with exported interfaces: change export type Scenario, Manifest, RequestVariant, K6Stage, ComparableModeSummary, ModeComparison, ScenarioVariantSummary, and SuiteSummary into export interface declarations preserving all property names and types; ensure optional properties (e.g., responseFixture?, headers?) remain optional and any index/record signatures (e.g., Partial<Record<AuthProfile, string>>) are preserved, and keep all references/usages unchanged so consumers compile against the new interfaces.
166-175: Add source context around JSON parse failures.These
JSON.parse()calls currently throw a bare syntax error with no file/input context, which makes a broken manifest, fixture, or docker snapshot unnecessarily hard to diagnose in the benchmark flow. Wrapping them and rethrowing with the source path would make failures actionable. As per coding guidelines, "Add proper error handling with try-catch blocks".Also applies to: 240-241, 299-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/scripts/lib.ts` around lines 166 - 175, The JSON.parse in loadManifest currently throws raw syntax errors without source context; wrap the fs.readFileSync + JSON.parse in a try-catch inside loadManifest, catch any error, and rethrow a new Error (or augment the original) that includes the manifestPath and a clear message like "Failed to parse JSON at <manifestPath>: <original error message>" so callers get file context. Apply the same pattern to the other JSON.parse sites in this file that read fixtures and docker snapshots (the JSON.parse calls that follow the manifest reader) so all parse failures include the source path and original error details.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmark/scripts/lib.ts`:
- Around line 337-349: readMetricNumber currently only reads a top-level field
and misses k6's aggregated formats; update the function readMetricNumber to
handle both legacy top-level fields and the nested metric.values object,
preferring the "rate" aggregate then falling back to "value" (and handling
either a top-level or metric.values structure), so metrics like http_req_failed,
graphql_error_rate, and response_mismatch_rate correctly return their numeric
aggregates instead of 0; keep the existing type checks (object/array) and return
0 if no numeric value is found.
In `@composition/CLAUDE.md`:
- Around line 105-111: The fenced code blocks that list the schema directive
lines (e.g., the block containing `@openfed__entityCache`, `@openfed__queryCache`,
`@openfed__cachePopulate`, `@openfed__cacheInvalidate` and `@openfed__is`) are missing
a language tag; update those fences to use an appropriate language for syntax
highlighting (for example ```graphql) and apply the same change to the other
similar blocks that list directives (the other blocks showing the same
`@openfed__`* directives further down). This will satisfy markdownlint and improve
rendering/tooling.
---
Duplicate comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 4122-4131: typesMatchIncludingListShape currently enforces exact
NonNull parity; change its signature to accept an optional flag (e.g.,
ignoreNullability: boolean = false) and, when true, treat NON_NULL wrappers as
transparent (unwrap and compare inner types without requiring got.kind ===
NON_NULL_TYPE) while still enforcing named type identity and list shape; then
update validateNestedInputObjectMapping / buildCompositeIsMapping so explicit
`@openfed__is` mapping flows call typesMatchIncludingListShape(..., true) (other
callers keep the default false) to allow nullability differences but still
require strict named type and list shape matching.
- Around line 3896-3913: The current guard only checks object returns before
warning and returning, which lets keyed interface/union returns produce
rootFieldCacheConfiguration without an entity-cache backing; remove the
isObjectReturn conditional so the code always verifies
entityCacheConfigByTypeName.has(returnTypeName) and, if missing, pushes
queryCacheReturnEntityMissingEntityCacheWarning and returns; update the check
around returnTypeName/entityCacheConfigByTypeName in normalization-factory.ts
(the block using returnTypeName, isObjectReturn,
this.entityCacheConfigByTypeName, this.warnings.push, and
queryCacheReturnEntityMissingEntityCacheWarning) so every `@openfed__queryCache`
return type requires an entity-cache entry.
- Around line 3820-3853: The loop currently stores cache configs under the
schema-local root type (parentTypeName) which causes configs for schema-root
aliases (e.g. schema { query: MyQuery }) to be attached to MyQuery instead of
the normalized root; fix by deriving the canonical/normalized root type from the
operationType node returned by
getOperationTypeNodeForRootTypeName(parentTypeName) (e.g.
operationType.name.value) and pass that canonical root name into
extractQueryCacheConfig, extractCacheInvalidateConfig and
extractCachePopulateConfig instead of parentTypeName so all root-field cache
configs are stored under the normalized root config key.
---
Nitpick comments:
In `@benchmark/scripts/lib.ts`:
- Around line 13-103: Replace the exported object-shaped type aliases with
exported interfaces: change export type Scenario, Manifest, RequestVariant,
K6Stage, ComparableModeSummary, ModeComparison, ScenarioVariantSummary, and
SuiteSummary into export interface declarations preserving all property names
and types; ensure optional properties (e.g., responseFixture?, headers?) remain
optional and any index/record signatures (e.g., Partial<Record<AuthProfile,
string>>) are preserved, and keep all references/usages unchanged so consumers
compile against the new interfaces.
- Around line 166-175: The JSON.parse in loadManifest currently throws raw
syntax errors without source context; wrap the fs.readFileSync + JSON.parse in a
try-catch inside loadManifest, catch any error, and rethrow a new Error (or
augment the original) that includes the manifestPath and a clear message like
"Failed to parse JSON at <manifestPath>: <original error message>" so callers
get file context. Apply the same pattern to the other JSON.parse sites in this
file that read fixtures and docker snapshots (the JSON.parse calls that follow
the manifest reader) so all parse failures include the source path and original
error details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5b4389db-8064-493a-849c-0ffcec36403b
⛔ Files ignored due to path filters (1)
docs/entity-caching/pre-release-testing/generated/.gitignoreis excluded by!**/generated/**
📒 Files selected for processing (38)
Makefilebenchmark/scenarios/cache-demo.jsonbenchmark/scripts/lib.test.tsbenchmark/scripts/lib.tscomposition-go/index.global.jscomposition/CLAUDE.mdcomposition/src/v1/normalization/normalization-factory.tscomposition/tests/v1/directives/entity-caching.test.tsdemo/pkg/subgraphs/cachegraph/subgraph/data_race_test.godemo/pkg/subgraphs/cachegraph/subgraph/schema.resolvers.godocs/REQUEST_SCOPED.mddocs/entity-caching/directives.mddocs/entity-caching/pre-release-testing/.gitignoredocs/entity-caching/pre-release-testing/Makefiledocs/entity-caching/pre-release-testing/README.mddocs/entity-caching/pre-release-testing/config/router.redis.yamldocs/entity-caching/pre-release-testing/docker-compose.ymldocs/entity-caching/pre-release-testing/example/graph.yamldocs/entity-caching/pre-release-testing/example/subgraphs/products/schema.graphqlsdocs/entity-caching/pre-release-testing/example/subgraphs/products/server.tsdocs/entity-caching/pre-release-testing/package.jsondocs/entity-caching/pre-release-testing/scripts/check-config.shdocs/entity-caching/pre-release-testing/scripts/compose.shdocs/entity-caching/pre-release-testing/scripts/setup-pr.shdocs/entity-caching/pre-release-testing/scripts/smoke-test.shdocs/entity-caching/pre-release-testing/scripts/wait-router.shrouter-tests/entity_caching/entity_caching_test.gorouter-tests/entity_caching/harness_test.gorouter/core/graph_server.gorouter/core/graph_server_cache_metrics_test.gorouter/core/router.gorouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/config_test.gorouter/pkg/config/fixtures/full.yamlrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.jsonrouter/pkg/metric/config.go
✅ Files skipped from review due to trivial changes (2)
- benchmark/scenarios/cache-demo.json
- demo/pkg/subgraphs/cachegraph/subgraph/schema.resolvers.go
🚧 Files skipped from review as they are similar to previous changes (3)
- demo/pkg/subgraphs/cachegraph/subgraph/data_race_test.go
- composition/tests/v1/directives/entity-caching.test.ts
- benchmark/scripts/lib.test.ts
Comment on lines
+337
to
+349
| function readMetricNumber( | ||
| metrics: Record<string, unknown>, | ||
| metricName: string, | ||
| field: string, | ||
| ): number { | ||
| const metric = metrics[metricName]; | ||
| if (!metric || typeof metric !== "object" || Array.isArray(metric)) { | ||
| return 0; | ||
| } | ||
|
|
||
| const value = (metric as Record<string, unknown>)[field]; | ||
| return typeof value === "number" ? value : 0; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and get its size
wc -l benchmark/scripts/lib.tsRepository: wundergraph/cosmo
Length of output: 90
🏁 Script executed:
# Read the specific lines mentioned in the review: 337-349
sed -n '337,349p' benchmark/scripts/lib.tsRepository: wundergraph/cosmo
Length of output: 414
🏁 Script executed:
# Also check the "Also applies to" section: 431-443
sed -n '431,443p' benchmark/scripts/lib.tsRepository: wundergraph/cosmo
Length of output: 744
🏁 Script executed:
# Get some context around these sections to understand the overall structure
sed -n '330,360p' benchmark/scripts/lib.tsRepository: wundergraph/cosmo
Length of output: 708
🏁 Script executed:
# Look for k6 related files or documentation
find . -type f -name "*.ts" -o -name "*.js" -o -name "*.md" | xargs grep -l "k6\|metric" | head -20Repository: wundergraph/cosmo
Length of output: 560
🏁 Script executed:
# Check if there are any tests or examples showing the metric structure
rg "values|http_req_failed|graphql_error_rate" -B 3 -A 3 --type ts --type jsonRepository: wundergraph/cosmo
Length of output: 50375
🏁 Script executed:
# Also check the imports and context around the readMetricNumber function usage
sed -n '1,50p' benchmark/scripts/lib.tsRepository: wundergraph/cosmo
Length of output: 1159
🏁 Script executed:
# Look at where metrics are being read from/passed to readMetricNumber
sed -n '400,450p' benchmark/scripts/lib.tsRepository: wundergraph/cosmo
Length of output: 2122
🏁 Script executed:
# Look at the test file for lib.ts
cat -n benchmark/scripts/lib.test.ts | head -100Repository: wundergraph/cosmo
Length of output: 2858
🏁 Script executed:
# Search for k6 metric examples or references
rg "http_req_failed|graphql_error_rate|response_mismatch_rate" -B 5 -A 5Repository: wundergraph/cosmo
Length of output: 4121
🏁 Script executed:
# Check if there are any k6 output examples or documentation
find . -name "*.json" -o -name "*.md" | xargs grep -l "http_req_failed\|k6.*metric" 2>/dev/null | head -10Repository: wundergraph/cosmo
Length of output: 88
🏁 Script executed:
# Look for test fixtures or example k6 output
find . -type f \( -name "*.json" -o -name "*.test.ts" \) | xargs grep -l "http_req_failed\|graphql_error_rate" 2>/dev/nullRepository: wundergraph/cosmo
Length of output: 93
🏁 Script executed:
# Check more of the test file to see the full metric structure
sed -n '100,200p' benchmark/scripts/lib.test.tsRepository: wundergraph/cosmo
Length of output: 2535
🏁 Script executed:
# Look for any comments or documentation about k6 metric structure
rg "k6.*metric\|metric.*structure\|values.*rate" -B 2 -A 2Repository: wundergraph/cosmo
Length of output: 43
🏁 Script executed:
# Check if there's documentation about the Rate type in k6
rg "Rate\(" benchmark/k6/cache_demo.js -B 2 -A 2Repository: wundergraph/cosmo
Length of output: 241
🏁 Script executed:
# Look for the test that shows metric structure
sed -n '150,250p' benchmark/scripts/lib.test.tsRepository: wundergraph/cosmo
Length of output: 2505
🏁 Script executed:
# Also look for extractComparableModeSummary test which uses readMetricNumber
rg "extractComparableModeSummary" benchmark/scripts/lib.test.ts -A 50Repository: wundergraph/cosmo
Length of output: 2437
🏁 Script executed:
# Look for any reference to the grafana documentation link or k6 metric format
rg "grafana.com\|metric.*values\|values.*rate" benchmark/scripts/lib.ts -B 3 -A 3Repository: wundergraph/cosmo
Length of output: 43
🏁 Script executed:
# Check if there are any comments explaining the metric structure
sed -n '320,360p' benchmark/scripts/lib.tsRepository: wundergraph/cosmo
Length of output: 1101
🏁 Script executed:
# Look at the complete context where readMetricNumber is called
sed -n '425,450p' benchmark/scripts/lib.tsRepository: wundergraph/cosmo
Length of output: 1279
🏁 Script executed:
# Check if the test passes with the current metric structure
# Also look for any k6 actual output examples or related issues
rg "k6.*output\|end-of-test\|summary.*metric" benchmark/ -B 2 -A 2Repository: wundergraph/cosmo
Length of output: 43
🏁 Script executed:
# Look for any real k6 test runs or CI configuration
find . -name "*.yml" -o -name "*.yaml" | xargs grep -l "k6\|benchmark" 2>/dev/null | head -5Repository: wundergraph/cosmo
Length of output: 204
Use rate field for rate metrics; handle both metric value formats.
k6's end-of-test summary stores aggregated numbers under metric.values, and rate metrics expose their aggregate as rate, not value. The current code uses "value" for http_req_failed, graphql_error_rate, and response_mismatch_rate, while other metrics correctly use "rate" (e.g., requestRate). This inconsistency causes these fields to collapse to 0 even when real data exists. (grafana.com)
🩹 Proposed fix
function readMetricNumber(
metrics: Record<string, unknown>,
metricName: string,
field: string,
): number {
const metric = metrics[metricName];
if (!metric || typeof metric !== "object" || Array.isArray(metric)) {
return 0;
}
- const value = (metric as Record<string, unknown>)[field];
+ const values =
+ "values" in metric &&
+ (metric as { values?: unknown }).values &&
+ typeof (metric as { values?: unknown }).values === "object" &&
+ !Array.isArray((metric as { values?: unknown }).values)
+ ? ((metric as { values: Record<string, unknown> }).values)
+ : (metric as Record<string, unknown>);
+
+ const value = values[field];
return typeof value === "number" ? value : 0;
}- httpFailureRate: readMetricNumber(metrics, "http_req_failed", "value"),
- graphqlErrorRate: readMetricNumber(metrics, "graphql_error_rate", "value"),
+ httpFailureRate: readMetricNumber(metrics, "http_req_failed", "rate"),
+ graphqlErrorRate: readMetricNumber(metrics, "graphql_error_rate", "rate"),
responseMismatchRate: readMetricNumber(
metrics,
"response_mismatch_rate",
- "value",
+ "rate",
),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmark/scripts/lib.ts` around lines 337 - 349, readMetricNumber currently
only reads a top-level field and misses k6's aggregated formats; update the
function readMetricNumber to handle both legacy top-level fields and the nested
metric.values object, preferring the "rate" aggregate then falling back to
"value" (and handling either a top-level or metric.values structure), so metrics
like http_req_failed, graphql_error_rate, and response_mismatch_rate correctly
return their numeric aggregates instead of 0; keep the existing type checks
(object/array) and return 0 if no numeric value is found.
Comment on lines
+105
to
+111
| ``` | ||
| @openfed__entityCache(maxAge: Int!) → on OBJECT types (entities with @key) | ||
| @openfed__queryCache(maxAge: Int!) → on Query fields returning cached entities | ||
| @openfed__cachePopulate(maxAge?: Int) → on Mutation/Subscription fields | ||
| @openfed__cacheInvalidate → on Mutation/Subscription fields | ||
| @openfed__is(fields: String!) → on argument definitions (maps arg → key field) | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add languages to these fenced code blocks.
markdownlint is already flagging these fences. Leaving them untyped keeps docs lint noisy and hurts rendering/tooling.
Also applies to: 218-220, 235-237
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 105-105: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@composition/CLAUDE.md` around lines 105 - 111, The fenced code blocks that
list the schema directive lines (e.g., the block containing
`@openfed__entityCache`, `@openfed__queryCache`, `@openfed__cachePopulate`,
`@openfed__cacheInvalidate` and `@openfed__is`) are missing a language tag; update
those fences to use an appropriate language for syntax highlighting (for example
```graphql) and apply the same change to the other similar blocks that list
directives (the other blocks showing the same `@openfed__`* directives further
down). This will satisfy markdownlint and improve rendering/tooling.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
@keyfields at the router level, so aProductcached by one query is reused by every query that needs it — regardless of query shape or subgraphsync.Map) eliminates redundant entity fetches within a single request; L2 (Redis or in-memory ristretto) shares cached entities across requests with per-entity TTLs via@entityCache(maxAge)@entityCacheand@entityCacheTTLdirectives validated and propagated through the composition layerCompanion PR: graphql-go-tools #1259
About this PR
Supersedes #2677 (closed by GitHub as "no shared history with main"). The original
jensneuse/entity-cachingbranch was rooted at an unsignedInitial commitwhilewundergraph/cosmo:mainis rooted at the GPG-signed equivalent — same content at the root, but every commit SHA differs all the way down, so GitHub refused to compare or merge them.This branch is a single squashed commit containing the full diff from
origin/main(d86216fc0) tojensneuse/entity-caching(e5ac2b97), applied on top of current main. The original feature branch is preserved atjensneuse/entity-cachingfor full per-commit history if needed during review.The squash was verified to revert nothing:
entity-caching's most recent main-equivalent commit matches the currentorigin/maintip exactly, so no recent main work is lost.Documentation
All entity caching docs live in
docs/entity-caching/:engineering-brief.mddirectives.md@entityCacheand@entityCacheTTLdirective definitions and usageconfiguration.mdanalytics.mdanalytics-rfc.mdStart with the engineering brief for a full overview of the design and review focus areas.
Test plan
composition/tests/v1/directives/entity-caching.test.ts)router/core/executor_entity_cache_test.go)router-tests/entity_caching/)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores