review: address ysmolski + CodeRabbit feedback on #1259

Applies the accepted set of review items from docs/pr1259-review/TODO.md.

ysmolski (planner / visitor.go):
- A25: drop the redundant (v.planners == nil || plannerID >= len) guard in
  trackFieldForPlanner — shouldPlannerHandleField already checks bounds.
- A33: comment the EnterDocument reset block so the per-walk reset lifecycle
  is explicit next to the fieldPlanners-not-reset note.
- A34: trim the backwards "non-key fields on concrete entity types" comment
  into a single, forward-reading line.
- A35: remove the vestigial `typeName != ""` guard inside the object-type
  branch of the CacheAnalytics switch.
- A36: extract polymorphicEntityCacheAnalytics so the union/interface arm
  no longer needs a hasEntity flag and duplicate inner switch.
- A37: tighten the "Initialize per-planner structures" comment.
- A39: drop the unused nil-check at the top of initializePlannerStructures.
- A40: document why createFieldValueForPlanner doesn't reuse resolveFieldValue
  (no walker-state mutation so it is callable from EnterField).
- A41: note that plannerResponsePaths / plannerEntityBoundaryPaths are stored
  normalized (fragment markers stripped).
- A42: normalize fullFieldPath in isEntityRootField before the HasPrefix
  comparison and extract isEntityRootPath as a pure helper.
  Inline-fragment queries (e.g. `... on User { reviews }`) previously broke
  entity-root detection because the walker path still carried `$N<TypeName>`
  markers while the boundary path was already normalized.
  Adds visitor_path_normalization_test.go with TestIsEntityRootPath covering
  fragment-wrapped boundary paths and TestNormalizePathRemovingFragments
  locking the regex invariant.
- A44: rewrite the entityKeyFieldNames doc without double-negation; flag the
  compound-key limitation (whitespace splitting produces a superset that
  widens key-field detection, falling into the safer no-op branch rather
  than over-invalidating).
- A45: iterate fieldEnclosingTypeNames in subscriptionSelectsNonKeyFields
  instead of every operation field ref.
- A46: merge resolveUnionEntityPopulation and resolveInterfaceEntityPopulation
  into a single resolveAbstractEntityPopulation helper that handles both
  union members and interface implementors.
- A47: drop the unused fieldRef parameter from createFieldValueForPlanner.
- A48: in configureMutationEntityImpact, merge keys from ALL @key configs via
  extractKeyFields instead of reading only keyConfigs[0] — entities with
  multiple @key directives no longer lose invalidation-relevant fields.
- A28 / A29 / A32: delete the unused relatedUsers field (regenerate gqlgen
  output), drop redundant `IncludeSubgraphHeaderPrefix: false` lines in
  federation_caching_test.go, and remove the unused
  WithRootFieldEntityCacheKeyTemplates testing option.

A49 (reverse-index optimization for trackFieldForPlanner) was attempted and
reverted — planningVisitor is registered on the walker before individual
planner visitors, so AllowVisitor fires for planners after planningVisitor.EnterField
runs. fieldPlanners[ref] is still empty at that point, which broke datasource
tests (TestGraphQLDataSource/simple_named_Query_with_field_info and
composite_keys_with_info/run) with missing ProvidesData fields. The O(planners)
loop remains, with shouldPlannerHandleField short-circuiting non-owning
planners. A49 is now tracked as a follow-up in TODO.md.

CodeRabbit (test robustness and fixtures):
- B02: close the original req.Body and surface the io.ReadAll error in
  partial_cache_test.go subgraphRequestTracker.RoundTrip.
- B03: route MeInterface / MeUnion / Identifiable resolvers through
  GetUsername(id) so UpdateUsername is reflected in those paths too.
- B05: confirmed all subscription channel sends in products/schema.resolvers.go
  are already wrapped in `select { <-ctx.Done() / ch <- p }` — no change
  needed.
- B11+C07: broaden the CacheEntry.Value description to opaque bytes
  (entity JSON or root-field response bytes).
- B13: populate Nickname and RealName on the User returned by
  UpdateUsername (match the Me/User resolver pattern).
- B14: accept first=0 in products/UpdatedPrices (guard changed from `> 0`
  to `>= 0`).
- B15+C08: document the full L2 key transformation pipeline (GlobalCacheKeyPrefix
  → subgraph header prefix → L2CacheKeyInterceptor) in both the main
  "Key Transformations" section and the extension-invalidation step list.
- B17: create a fresh FakeLoaderCache inside each parallel subtest in
  TestFakeLoaderCache to eliminate TTL cross-contamination.
- B18: bound raw `<-messages` receives in federation_subscription_caching_test.go
  via a mustRecvMessage(t, ch, 5s) helper.
- B19: configure a matching Subscription.updateProductPrice root-field cache
  in the negative test so it actually exercises the "subscription roots
  ignore matching root-field cache" path, and assert the subscription-root
  cache key never appears.
- B20: harden shared-trigger receive loops with readOrFail semantics
  (`m, ok := <-ch`) and mirror the warm-up pattern to both 2-client blocks.
- C01: drop `.serena` from .gitignore (personal tool artifact).
- C02: extract executeQuery helper in graphql_client_test.go to de-duplicate
  the four Query/QueryWithHeaders/QueryString/QueryStringWithHeaders flows.
- C03: normalize indentation in multiple_upstream_without_provides.query.
- C04: rename the inconsistent gatewayOptions.withLoaderCache field to
  loaderCache to match sibling naming and avoid the option-builder collision.
- C05 already in tree: each gzip/deflate subtest already uses its own local
  headers map; no change needed.
- C11: require.NoError every ignored url.Parse in
  federation_caching_root_entity_test.go (5 sites).
- C13: wrap the two `<-message` receives in federation_integration_static_test.go
  with the mustRecvMessage helper.

Docs / review artifacts:
- docs/pr1259-review/ — FEEDBACK.md (raw PR comments, H2 per item),
  CLAUDE_EVAL.md, CODEX_EVAL.md, and TODO.md (reconciled joint decisions).
  Joint evaluations reached agreement before code changes; re-review after
  applying changes surfaced three follow-ups (A42 test coverage, B15+C08
  second doc reference, C11 remaining sites) which are also addressed in
  this commit.

Validation:
- `go test ./...` green for both the v2/ and execution/ modules.
- New tests: TestIsEntityRootPath, TestNormalizePathRemovingFragments,
  TestVisitorEntityKeyFieldNames.
- Codex code-review sign-off on the final diff.

Also bundled:
- v2/pkg/engine/resolve/cache_analytics.go / context.go — sync.Pool for
  CacheAnalyticsCollector (pre-existing branch-local perf work;
  AcquireCacheAnalyticsCollector / ReleaseCacheAnalyticsCollector / ResetForReuse).
- v2/pkg/engine/plan/visitor_subscription_entity_population_test.go —
  entityKeyFieldNames unit tests (pre-existing branch-local).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: jensneuse

.gitignore

@@ -6,5 +6,4 @@
 pkg/parser/testdata/lotto.graphql
 *node_modules*
 *vendor*
-.serena
 docs/superpowers/

docs/entity-caching/ENTITY_CACHING_INTEGRATION.md

@@ -39,7 +39,7 @@ type LoaderCache interface {
 
 type CacheEntry struct {
     Key          string           // Cache key string (JSON format)
-    Value        []byte           // JSON-encoded entity data
+    Value        []byte           // Opaque cached payload bytes (e.g., entity JSON or root-field response bytes); callers interpret
     RemainingTTL time.Duration    // Remaining TTL from cache (0 = unknown/not supported)
     WriteReason  CacheWriteReason // Why this entry was written (set by the engine, not by backends)
 }
@@ -325,14 +325,19 @@ Arguments are sorted alphabetically for stable key generation.
 
 ### Key Transformations (applied in order)
 
-1. **Subgraph header hash prefix** (when `IncludeSubgraphHeaderPrefix = true`):
+1. **Global cache key prefix** (when `GlobalCacheKeyPrefix` is set on the request's `CachingOptions`):
    ```text
-   {headerHash}:{"__typename":"User","key":{"id":"123"}}
+   v42:{"__typename":"User","key":{"id":"123"}}
    ```
 
-2. **L2CacheKeyInterceptor** (when set):
+2. **Subgraph header hash prefix** (when `IncludeSubgraphHeaderPrefix = true`):
    ```text
-   tenant-X:{headerHash}:{"__typename":"User","key":{"id":"123"}}
+   v42:{headerHash}:{"__typename":"User","key":{"id":"123"}}
+   ```
+
+3. **L2CacheKeyInterceptor** (when set):
+   ```text
+   tenant-X:v42:{headerHash}:{"__typename":"User","key":{"id":"123"}}
    ```
 
 ### Entity Field Argument-Aware Keys
@@ -553,7 +558,7 @@ Subgraphs can signal cache invalidation through GraphQL response extensions:
 The engine automatically:
 1. Parses `extensions.cacheInvalidation.keys` from each subgraph response
 2. Builds L2 cache keys matching entity type and key fields
-3. Applies subgraph header prefix and `L2CacheKeyInterceptor` transformations
+3. Applies the full L2 key-transformation pipeline in order: `GlobalCacheKeyPrefix` → subgraph header prefix → `L2CacheKeyInterceptor` (same ordering as cache writes)
 4. Calls `LoaderCache.Delete()` for each key
 5. **Optimization**: skips delete if the same key is being written in the same fetch (no unnecessary round-trip)
 

docs/pr1259-review/CLAUDE_EVAL.md

@@ -0,0 +1,114 @@
+# Claude's per-item evaluation — PR #1259
+
+My read of each feedback item before asking codex. Tags:
+- **ACCEPT** — do what the reviewer asks
+- **ACCEPT-MINOR** — trivial cleanup, accept but low priority
+- **REJECT** — disagree, rationale given
+- **VERIFY** — likely a real bug; must check the current code first
+- **DEFER** — out of scope for this PR, track separately
+- **DONE** — reviewer/author already closed it; we believe the current code reflects the fix
+
+---
+
+## A. ysmolski
+
+- **A01** `configuration.go:54` Caching default-enabled flag — **DONE.** Author inverted to `DisableCaching` default-false; reviewer intent satisfied.
+- **A02** `federation_metadata.go:302` wording "should be" → "is" — **DONE.** Author fixed wording.
+- **A03** `loader.go:184` extra fields mixed into loader — **DONE.** Author extracted `cacheState`, full refactor deferred (reasonable).
+- **A04** `fetch.go:278` `Caching` missing from `Equals` — **DONE.** Author added and guarded with regression test.
+- **A05** `caching.go:84` `field` not used in loop body, repeats N times — **DONE.**
+- **A06** `caching.go:331` handle `*Enum` / `*BigInt` — **DONE.** Author added both cases + tests.
+- **A07** `caching.go:347` redundant comments / collapse identical cases — **DONE.**
+- **A08** `resolvable.go:1408` field path changed "query.user.name" → "User.name" — **DONE.** Author reverted to master behavior.
+- **A09** `cache_analytics.go:688` misleading function name — **DONE.** Renamed to `CacheHitCount()`.
+- **A10** `cache_analytics.go:928` unsafe byte slicing of strings — **DONE.** Author fixed.
+- **A11** `loader_cache.go:81` last-match-wins loop missing `break` — **DONE.** Fixed.
+- **A12** `loader_cache.go:1315` reuse `buildMutationEntityDisplayKey` — **DONE.** Refactored.
+- **A13** `loader_cache.go:61` avoid throw-away arena `key` — **DONE.** Refactored to heap-allocated slice.
+- **A14** `representation_variable.go:21` why exported? — **DONE (keep exported).** Decision: router consumes them; keep public.
+- **A15** `loader_cache.go:707` unused `items` arg — **DONE.** Removed.
+- **A16/A17/A18** `loader_cache.go:{43,67,89}` stale comments — **DONE.**
+- **A19** `cache_analytics.go:182` unused fields — **DONE.** Cleaned up.
+- **A20** `cache_analytics.go:58` `CacheKeyEvent` usage unclear — **DONE (decision).** Author explained usage.
+- **A21** `cache_analytics.go:228` `RecordFetchTiming` etc. only called by tests — **DONE (decision).** Needed by cosmo router.
+- **A22** `loader.go:1665` unneeded defer — **DONE.**
+- **A23** `visitor.go:1246` redundant comments — **DONE.**
+- **A24** `visitor.go:1235` 115-LOC function needs doc — **DONE.** Reviewer explicitly accepted ("Thank you. This comment is enough").
+- **A25** `visitor.go:1236` redundant check — **REOPENED — needs real fix.** ysmolski: "I still see those two redundant checks." **ACCEPT, must address.**
+- **A26** `circuit_breaker.go:115` XCHGL writes on hot path — **DONE.** Author added fast-path.
+- **A27** `variables_renderer.go:295` stale comment — **DONE.**
+
+### Open (post-fix-pass, never replied to):
+
+- **A28** `schema.graphqls:62` test-only field comment — **ACCEPT-MINOR.** Drop comment or delete the field; it's in federationtesting schema. Cheap cleanup.
+- **A29** `federation_caching_test.go:205` `IncludeSubgraphHeaderPrefix: false` noise — **ACCEPT-MINOR.** `false` is the zero value; remove the redundant field everywhere. Mechanical sed.
+- **A30** `federation_caching_test.go:596` verbose assertion messages — **REJECT (or DEFER).** Opinion-driven; the messages do help when tests fail in CI. Not worth churn.
+- **A31** `federation_caching_test.go:23` 4k LOC file, naming + helpers — **DEFER.** Legitimate concern but a proper refactor of a 4k-line test file is a separate PR. File a follow-up issue.
+- **A32** `datasourcetesting.go:121` unused `WithRootFieldEntityCacheKeyTemplates` — **VERIFY.** grep — if truly unused, delete; if tests use it, keep.
+- **A33** `visitor.go:1157` re-init maps in EnterDocument vs constructor — **ACCEPT.** Author should either move the resets to `NewVisitor` (matching ysmolski's earlier refactor) or justify the divergence in a comment. Consistency matters.
+- **A34** `visitor.go:527` backwards / obvious comment — **ACCEPT-MINOR.** Reword or delete L2.
+- **A35** `visitor.go:922` "could not be non-empty here" — **VERIFY.** If reviewer is right, remove the `typeName != ""` guard. Safe cleanup, but worth a 2-minute check.
+- **A36** `visitor.go:940` duplicate switch + complexity — **ACCEPT.** Reviewer provided a clean extraction (`polymorphicEntityCacheAnalytics`). Low risk, improves readability.
+- **A37** `visitor.go:1085` ambiguous ProvidesData comment — **ACCEPT-MINOR.** Reword with reviewer's suggested text.
+- **A38** `visitor.go:1231` init in EnterOperationDefinition — **ACCEPT (explain or move).** Either add a comment explaining why init happens in Enter, or hoist into `initializePlannerStructures` called from one canonical place.
+- **A39** `visitor.go:1215` nil-check before range over slice — **ACCEPT-MINOR.** Drop the guard; `range` over nil is zero-iteration.
+- **A40** `visitor.go:1388` "simplified field value" rationale — **ACCEPT-MINOR.** Add a one-line "why we don't reuse `resolveFieldValue`" comment.
+- **A41** `visitor.go:78` mention paths are normalized — **ACCEPT-MINOR.** One-line doc fix.
+- **A42** `visitor.go:1512` non-normalized vs normalized path compare — **VERIFY — potential BUG.** If the current `fullFieldPath` still contains `$0User` fragment markers but boundary paths don't, `HasPrefix` silently fails. Needs a real check + test.
+- **A43** `visitor.go:77` `plannerEntityBoundaryPaths` unused — **VERIFY.** If genuinely unused, delete the field and its initializer.
+- **A44** `visitor.go:1922` `entityKeyFieldNames` wording + behavior concern — **ACCEPT.** Double-negation confuses; reviewer is right that incorrectly expanding key fields skews invalidation. Rewrite doc + confirm compound-key handling with a test (or add a TODO with an issue link).
+- **A45** `visitor.go:1944` iterate all fields vs `fieldEnclosingTypeNames` — **ACCEPT.** Restrict iteration to the known subset. Fixes both complexity and correctness clarity.
+- **A46** `visitor.go:1882` merge `resolveUnion*` / `resolveInterface*` — **ACCEPT.** Two helpers differ by a single predicate (UnionType vs InterfaceType); fold into one, call once.
+- **A47** `visitor.go:1393` `fieldRef` dead param — **ACCEPT-MINOR.** Drop it.
+- **A48** `visitor.go:2522` single key fields vs multiple `keyConfigs` — **VERIFY — potential BUG.** `ParseKeyFields(keyConfigs[0].SelectionSet)` silently ignores secondary keys. Either pick deterministically or merge all — needs decision + doc.
+- **A49** `visitor.go:387` O(fields × planners) on `EnterField` hot path — **ACCEPT-PARTIAL.** Legit concern. Either (a) skip planners that can't own this field (cheap filter), or (b) benchmark and document acceptable overhead. Not a blocker but worth a follow-up.
+- **A50** `visitor.go:74` split caching concerns into separate visitor — **DEFER.** Legit architectural feedback that ysmolski himself framed as "food for thought" before going on vacation. Track as a follow-up refactor; don't block this PR, but file an issue referencing his cost-visitor pattern as the model.
+
+---
+
+## B. CodeRabbit inline
+
+- **B01** `federation_subscription_caching_test.go:1590` subscription test timeouts/cleanup — **VERIFY.** Bot was mid-analysis; check current code uses `collectSubscriptionMessages` with timeout everywhere.
+- **B02** `partial_cache_test.go:53` close original req.Body + surface read error — **ACCEPT.** Standard RoundTrip correctness; 4-line fix.
+- **B03** `accounts/schema.resolvers.go:45` test resolver hardcoded "User <id>" — **ACCEPT.** Since the PR added `UpdateUsername`, the query resolvers must use `GetUsername` for consistency across tests.
+- **B04** `accounts/schema.resolvers.go:322` Greeting obj.Username fallback — **DONE.** CodeRabbit marks "✅ Confirmed as addressed".
+- **B05** `products/schema.resolvers.go:260` goroutine leak (unbuffered send after `time.After`) — **ACCEPT.** Wrap sends with `select { case <-ctx.Done(): ... case ch <- p: }`. Prevents test hangs.
+- **B06** `reviews/schema.resolvers.go:25` synthesized usernames in AddReview — **DONE.** CodeRabbit confirmed addressed.
+- **B07** `graphql_datasource.go:915` root-field L1 keyed by entity type alone — **VERIFY — likely real BUG.** Aliased root fields (`viewer`, `user`) both return `User`; current keying collides. Needs composite key.
+- **B08** `graphql_datasource.go:957` `(TypeName,FieldName)` dedup collapses aliased selections — **VERIFY — likely real BUG.** Same root cause as B07. `a:user(id:$a) b:user(id:$b)` would merge args.
+- **B09** `graphql_datasource.go:1400` `trackFieldWithArgument` picks up nested fields — **VERIFY — likely real BUG.** Fix: gate with `p.isRootField()` before tracking.
+- **B10** `ENTITY_CACHING_INTEGRATION.md:15` L1 scope wording — **DONE.** CodeRabbit confirmed addressed.
+- **B11** `ENTITY_CACHING_INTEGRATION.md:43` broaden `CacheEntry.Value` doc — **ACCEPT.** 1-line wording fix.
+- **B12** `federation_caching_l1_test.go:37` comment says both fetch, assertion says 1 — **VERIFY.** Either the comment lies or the assertion is wrong; the diff mismatch masks a regression.
+- **B13** `accounts/schema.resolvers.go:23` UpdateUsername returns incomplete User — **ACCEPT.** Populate Nickname/RealName so subsequent selections don't return "".
+- **B14** `products/schema.resolvers.go:200` `first: 0` ignored — **REJECT-OR-ACCEPT-MINOR.** Test-fixture only. Fine either way; if you want boundary symmetry with `TopProducts`, accept; otherwise leave.
+- **B15** `ENTITY_CACHING_INTEGRATION.md:298-317` L2 key order docs — **ACCEPT.** Already tracked in CLAUDE.md that the three-stage pipeline exists; doc must reflect it.
+- **B16** `federation_caching_helpers_test.go:182` `defer cancel()` kills poller immediately — **ACCEPT.** Use `context.Background()` or return cleanup fn. Otherwise poller loses context at function return.
+- **B17** `federation_caching_helpers_test.go:653` parallel subtests share one `FakeLoaderCache` — **ACCEPT.** Move `NewFakeLoaderCache()` inside each subtest. Cheap isolation.
+- **B18** `federation_subscription_caching_test.go:303` `<-messages` no timeout — **ACCEPT.** Adopt `collectSubscriptionMessages` or `mustRecvMessage` helper. Prevents package-level hangs.
+- **B19** `federation_subscription_caching_test.go:1067` negative case configs wrong root — **ACCEPT.** The test supposed to prove subscription roots don't get root-field cache; configure the matching root entry to make the negative proof real.
+- **B20** `federation_subscription_caching_test.go:1562` shared-trigger receive loops treat closed chan as ready — **ACCEPT.** Use `m, ok := <-ch` + fail on `!ok`. Add warm-up.
+
+---
+
+## C. CodeRabbit review-summary (nitpicks / outside-diff)
+
+- **C01** `.gitignore` drop `.serena` — **ACCEPT.** Personal tool; belongs in `~/.gitignore_global`.
+- **C02** `graphql_client_test.go` extract `executeQuery` helper — **ACCEPT-MINOR.** DRY; small helper, low churn.
+- **C03** `multiple_upstream_without_provides.query` indent mixed — **ACCEPT-MINOR.**
+- **C04** `federation_integration_test.go:25` naming — **ACCEPT-MINOR.**
+- **C05** `execution_engine_test.go:198` header clone per parallel subtest — **ACCEPT.** Real race condition.
+- **C06** `execution_engine_test.go:5879` shared executionPlanCache + t.Parallel — **ACCEPT.** Real flake source.
+- **C07** `ENTITY_CACHING_INTEGRATION.md:42` CacheEntry.Value doc — **ACCEPT.** Same as B11 (duplicate).
+- **C08** `ENTITY_CACHING_INTEGRATION.md:283-317` L2 key order — **ACCEPT.** Same as B15 (duplicate).
+- **C09** `federation_caching_helpers_test.go:266-430` merge sort helpers — **DEFER.** Low signal, current is explicit. Leave as-is unless it bloats further.
+- **C10** `federation_caching_helpers_test.go:601-619` `GetLog` vs `GetLogWithCaller` — **ACCEPT-MINOR.** Either delete the alias or make `GetLog` strip Caller to match its name.
+- **C11** `federation_caching_root_entity_test.go:82` check url.Parse + query errors — **ACCEPT-MINOR.** `require.NoError` on both.
+- **C12** `execution_engine_test.go:4614` blank line — **REJECT.** Cosmetic, not worth a commit.
+- **C13** `federation_integration_static_test.go:123` wrap `<-message` in timeout — **ACCEPT.** Matches B18/B20 pattern. Fix once, reuse helper.
+
+---
+
+## D. Author TODO (jensneuse 2025-08-05)
+
+- **D01** "ability for Router to provide a key prefix, e.g. from authorization header" — **VERIFY-DONE.** Per `CLAUDE.md` the project now has `GlobalCacheKeyPrefix` + `IncludeSubgraphHeaderPrefix`. Confirm the header-derived hash prefix is wired through, then close the TODO.