Open
Conversation
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:
📝 WalkthroughWalkthroughAdds a two-level federation entity caching system (request-scoped L1 and external L2), planner support for argument-aware cache-key templates, runtime ExecutionOptions and cache analytics wiring, gateway/HTTP/WS plumbing for caching and headers, and extensive tests, docs, and federation test-subgraph updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Gateway as Gateway
participant Engine as Engine
participant Planner as Planner
participant Subgraph as Subgraph
participant LoaderCache as LoaderCache
Client->>Gateway: POST GraphQL (query, variables, extensions, headers)
Gateway->>Engine: Execute(request, ExecutionOptions...)
Engine->>Planner: Plan request (build cache templates)
Planner-->>Engine: Plan w/ FetchCacheConfiguration
Engine->>LoaderCache: Check L1 (per-request)
LoaderCache-->>Engine: L1 hit/miss
alt L1 hit
Engine-->>Gateway: Return response (skip subgraph)
else L1 miss
Engine->>LoaderCache: L2 Get(s)
LoaderCache-->>Engine: L2 hit/miss
alt L2 hit
Engine-->>Gateway: Return response (skip subgraph)
else L2 miss
Engine->>Subgraph: Fetch (_entities or root-field)
Subgraph-->>Engine: Response (+ possible extensions.cacheInvalidation)
Engine->>LoaderCache: Set L2 (and update L1) as configured
Engine-->>Gateway: Return response
end
end
Gateway-->>Client: HTTP response [ + X-Cache-Analytics header if enabled ]
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Member
Author
|
TODO:
|
# Conflicts: # v2/pkg/engine/resolve/fetch.go # v2/pkg/engine/resolve/resolve.go
…to resolve Context
# Conflicts: # go.work.sum # v2/go.mod # v2/go.sum # v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_test.go
Snapshot returned slice headers aliasing the collector's internal backing arrays. Because GetCacheStats releases the collector to the pool immediately after snapshotting, the next request's ResetForReuse + Record* calls would overwrite the caller's snapshot — surfaced as a WARNING: DATA RACE in cosmo router-tests/entity_caching under -race. Clone FieldHashes, FetchTimings, ErrorEvents, MutationEvents, and CacheOpErrors via slices.Clone so each snapshot owns its data. The other fields already allocate via deduplicate* helpers or append/make. Adds TestSnapshotIndependentOfPooledCollector (race detector) and TestSnapshotSlicesAreIndependent (functional, deterministic) to lock the invariant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Acquire + Release with RecordMutationEvent / RecordCacheOperationError leaves the pool holding a collector whose mutationEvents and cacheOpErrors slices are non-nil empty (NewCacheAnalyticsCollector leaves them nil; only ResetForReuse'd writes make them non-nil). Downstream tests that assert.Equal a full CacheAnalyticsSnapshot with those fields set to nil then pick up the polluted collector and fail. Use NewCacheAnalyticsCollector directly — the test's purpose is to prove Snapshot's slice independence, which does not require exercising the sync.Pool path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
7 tasks
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@execution/engine/request_scoped_widening_e2e_test.go`:
- Around line 39-71: The httptest.NewServer handler uses require.NoError(t, err)
in a non-test goroutine (in the handler closure at the body read,
json.Unmarshal, and response write points), which can panic; change those
require.NoError calls to assert.NoError(t, err) or handle the error explicitly
(e.g., write a 500 response and return) inside the handler. Locate the anonymous
http.HandlerFunc passed to httptest.NewServer (which builds
requestScopedE2ERequest using normalizeRequestScopedVariables and appends to
s.requests) and replace the three require.NoError calls with assert.NoError(t,
err) or error-handling logic that sets an error response and exits the handler
gracefully.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af68dba2-0940-40ce-81f0-22f83396d5b5
📒 Files selected for processing (17)
execution/engine/request_scoped_widening_e2e_test.gov2/pkg/engine/datasource/graphql_datasource/graphql_datasource.gov2/pkg/engine/datasource/graphql_datasource/request_scoped_widening_test.gov2/pkg/engine/plan/node_selection_builder.gov2/pkg/engine/plan/node_selection_visitor.gov2/pkg/engine/plan/node_selection_visitor_request_scoped.gov2/pkg/engine/plan/node_selection_visitor_request_scoped_test.gov2/pkg/engine/plan/planner.gov2/pkg/engine/plan/required_fields_provided_visitor.gov2/pkg/engine/plan/required_fields_provided_visitor_test.gov2/pkg/engine/plan/required_fields_visitor.gov2/pkg/engine/plan/visitor.gov2/pkg/engine/resolve/cache_analytics.gov2/pkg/engine/resolve/cache_analytics_test.gov2/pkg/engine/resolve/loader_cache.gov2/pkg/engine/resolve/mutation_cache_test.gov2/pkg/engine/resolve/request_scoped_test.go
The httptest.NewServer handler in newRequestScopedE2EServer runs on a non-test goroutine, but used require.NoError and called through to compactJSONForAssert (also require-based) transitively via normalizeRequestScopedVariables and inline responder closures. require.NoError calls FailNow -> runtime.Goexit, which per Go testing docs must only run on the test goroutine; on other goroutines behavior is undefined and can leave the test running with partial state. Fix the class at the goroutine boundary: - Handler: require.NoError -> assert.NoError with http.Error bail on body read and JSON decode; bare assert.NoError after Write. - normalizeRequestScopedVariables: inline the compact-JSON logic with non-fatal assert.NoError instead of calling require-based compactJSONForAssert. - Hoist two compactJSONForAssert calls out of responder closures into local vars on the test goroutine, captured by value. compactJSONForAssert itself is unchanged - it is still correct for all its test-goroutine callers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SkArchon
reviewed
Apr 23, 2026
| // The cache key construction pipeline mirrors the storage pipeline: | ||
| // | ||
| // typename + key fields → build JSON → apply header prefix → apply interceptor → cache.Delete() | ||
| func (l *Loader) processExtensionsCacheInvalidation(res *result, cacheInvalidation *astjson.Value, deletedKeys map[string]struct{}) { |
Contributor
There was a problem hiding this comment.
We don't seem to record events for these type of invalidations, I think we also would need to call
l.ctx.cacheAnalytics.RecordMutationEvent(MutationEvent{
With proper information
processExtensionsCacheInvalidation deleted L2 keys without recording analytics, leaving extension-driven invalidation invisible in cache analytics. Add a RecordMutationEvent call after the dedupe / about-to-be-set skip checks, gated on cacheAnalyticsEnabled. Source is derived from operation type so the event correctly tags Query/Mutation/Subscription origins. CachedHash, FreshHash, CachedBytes, FreshBytes are intentionally left zero (no extra L2 Get). Addresses unresolved PR #1259 review thread (SkArchon). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AllowVisitor appends to fieldPlanners[ref] slices. When a Planner is
reused across operations (planner cache, tests), AST refs can repeat
and the second walk appends to leftover slices, leaking stale planner
IDs into cost-tree metadata, fetch reasons, and dependency tracking.
Use clear() rather than reallocating so the cost visitor's captured
map pointer stays valid within a Plan() call. The previous comment
("intentionally NOT reset") conflated reset with reallocate; clear()
satisfies the captured-pointer invariant while restoring per-operation
isolation.
No behavior change for the single-operation path. The new regression
test reuses a single Planner across two operations and asserts plan2's
cost-tree datasource hashes contain no leak from plan1.
Addresses ysmolski review on PR #1259 (visitor.go:1177).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Existing AC-linked tests cover slices of the cache-key contract
(global prefix, header prefix, args-derived keys, response-derived
keys, extension invalidation) in isolation, but no single test
verifies that the SAME logical entity produces an identical L2 key
across all three operations: read, write, and extension-driven delete.
This test runs two loader executions to capture each observation:
PHASE 1 + 2 share one execution to observe the read and write keys
for the same entity fetch; PHASE 3 uses a separate newExtInvEnv run
because processExtensionsCacheInvalidation skips a delete when the
active fetch is about to write the same key, which would hide the
invalidation key in a single-execution observation.
Result: read == write == invalidation == schema-v42:33333:{...}.
No parity divergence found; the contract holds.
AC doc updated with test links to AC-L2-04, AC-KEY-03, AC-KEY-07
(partial), AC-EXT-02, AC-EXT-03 (partial — does not exercise
L2CacheKeyInterceptor).
Addresses ysmolski review concern on PR #1259 about cache-key
parity brittleness across the prefix + args + response-derived
key combination.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lf-check Adds an explicit "Universal rules / E2E rules / LLM agent self-check" structure so future agents do not extract shared helpers in execution/engine/ or write cramped multi-key cache log assertions. Cross-references the package-specific conventions in execution/engine/CLAUDE.md and v2/pkg/engine/resolve/CLAUDE.md, and states that package conventions override the universal defaults. The self-check table lists concrete STOP triggers (helper extraction, top-level config vars, single-line multi-event struct literals, partial assertions) so agents can self-correct without a human review round trip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…shape
Three intertwined changes shipped together because they touch the same
cache fixture and call sites.
1. Per-entry TTL (interface change)
- CacheEntry.TTL field added.
- LoaderCache.Set drops its trailing ttl parameter.
- Each CacheEntry now carries its own TTL, enabling mixed-TTL bulk
writes in a single backend call.
2. Bulk L2 Set per cache instance
- writeL2CacheSetContributors groups all per-fetch L2 writes by
LoaderCache identity and issues one Set per instance.
- Combined regular + negative entries (was 2 Sets per fetch, now 1).
- Cross-fetch grouping in Phase 4 collapses N root-field fetches
against the same cache into a single bulk Set.
- Failure semantics preserved: bulk Set error records one
CacheOperationError per contributing fetch, matching the
bulkL2Lookup pattern.
3. CacheLogEntry.Items shape (test fixture)
- Replaces parallel Keys/Hits/TTL/TTLs slices with a single
Items []CacheLogItem slice. Each item bundles {Key, Hit, TTL}
so the relationship between key and per-operation field is
explicit instead of positional.
- Single sortCacheLogEntries helper (was two).
Also folds Task 4a in for the same files: TestRootFieldSplitByDatasource
moved to its own sibling file with self-contained subtests per
execution/engine/CLAUDE.md (no shared helpers, inline setup).
Public API break: cosmo router and any downstream LoaderCache impl
must update to the new Set signature. The feature has not been
released yet, so no compat shim.
Tests: full v2/pkg/* and execution/* suites green.
Addresses the engineering question raised on PR #1259 about whether
multiple root-field cache writes could be a single Redis round trip:
yes, and now they are.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ysmolski flagged three exported symbols as suspicious because their only callers in this repository are tests. The author's intent was to expose them for external consumers (wundergraph/cosmo router). Add minimal godoc comments to make the contract explicit: - MergeRepresentationVariableNodes also has one in-repo production caller (visitor.go:1812), confirming public-API status. - RecordL2KeyEvent and RecordFetchTiming have zero in-repo production callers; doc notes they are kept exported for cosmo and should be internalized in the next breaking window if cosmo stops using them. No signature changes, no internalization. Addresses ysmolski review on PR #1259 (representation_variable.go:21, cache_analytics.go:278). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nerState
ysmolski flagged that the caching feature added ~900 LOC of two new
responsibilities on top of the planner visitor: a parallel ProvidesData
tree for caching, and per-fetch caching configuration. Visitor was
already 1600+ LOC before; this PR added another 900+ on top.
Extracts those two responsibilities into a sub-struct on Visitor:
type Visitor struct {
...
caching *cachingPlannerState
}
Sub-struct (Option B) chosen over a sibling visitor (Option A) because
the highest-risk part of this code is walker callback ordering. A
sibling visitor adds ordering risk with AllowVisitor, datasource
planner visitors, DefferOnEnterField, and the cost visitor. A sub-struct
preserves callback ordering exactly, with only Visitor registered on
the walker.
Moved to caching_planner_state.go (936 LOC):
- Fields: entityAnalyticsCache, requestScopedVisibleResponseKeys,
requestScopedFetchAliases, plannerObjects, plannerCurrentFields,
plannerResponsePaths, plannerEntityBoundaryPaths.
- Methods: 27 caching-specific methods including configureFetchCaching,
trackFieldForPlanner, popFieldsForPlanner,
configureSubscriptionEntityCachePopulation, entityCacheAnalytics,
polymorphicEntityCacheAnalytics, configureMutationEntityImpact, etc.
- Helper: extractKeyFields.
Preserved on Visitor (load-bearing):
- fieldPlanners — cost visitor captures by pointer.
- plannerFields — fetch deps/reasons read it.
- fieldEnclosingTypeNames — caching state reads through parent.
- Public RequestScopedFetchAlias — kept for external API stability,
delegates to v.caching.fetchAlias.
Net diff: visitor.go shrunk from 2646 to 1766 LOC (-880, -33%).
New caching_planner_state.go: 936 LOC.
Three test files updated for moved helpers (call-site rewrites only,
no logic change):
- visitor_path_normalization_test.go
- request_scoped_provides_data_test.go
- visitor_subscription_entity_population_test.go
Also adds a "Code Comment Conventions" section to CLAUDE.md banning
PR/issue/reviewer references in code comments. Two such references
that crept in during this refactor have been removed from
caching_planner_state.go and visitor_path_normalization_test.go.
Tests: full plan, resolve, execution suites green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three CI fixes:
1. extensions_cache_invalidation_test.go: TestExtensionsCacheInvalidation
Analytics/records_no_MutationEvent_when_extension_delete_is_skipped
asserted assert.Equal([]MutationEvent{}, stats.MutationEvents). The
snapshot's slices.Clone returns nil when the underlying slice is
never appended to, so DeepEqual mismatched the empty-slice expected
value against the nil actual. Switch to len() == 0, which is exact
and tolerates the nil/empty implementation detail.
2. cache_key_parity_test.go: gci import grouping fix per .golangci.yml
sections (standard / default / wundergraph / wundergraph/graphql-go-tools).
3. planner_test.go: gofmt -s fix for redundant []int{0} slice literals
in the reused-planner regression test.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (1)
execution/engine/federation_subscription_caching_test.go (1)
21-76:⚠️ Potential issue | 🟠 MajorAvoid introducing shared helpers in
execution/engine/tests without explicit approval.Lines 21-76 add shared helpers and centralize subtest setup flow. This package’s convention is inline, self-contained subtests; new shared helpers are disallowed unless explicitly approved.
As per coding guidelines:
execution/engine/**/*.go: “No new shared test helpers in execution/engine/ without explicit approval.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/engine/federation_subscription_caching_test.go` around lines 21 - 76, The new shared test helper functions (toWSAddr, mustRecvMessage, boolToInt, collectSubscriptionMessages) violate the rule forbidding shared helpers in execution/engine/ tests; remove these helpers from this file and instead inline their logic into the individual subtests that use them (or, if you have explicit approval, move them to an approved shared test utility package). Specifically, replace calls to collectSubscriptionMessages (and its use of GraphqlClient.Subscription and federationtesting.FederationSetup.NextProductSubscription) with the equivalent inline subscription setup and message loop in each test, and inline the small utilities toWSAddr, mustRecvMessage, and boolToInt where needed within those tests so no new package-level helpers remain.
🧹 Nitpick comments (2)
execution/engine/federation_caching_root_split_test.go (2)
281-291: Inline the GraphQL query string at each call site.Using a named
queryvariable here conflicts with the execution/engine test convention requiring inline literals directly where they’re used.As per coding guidelines: "
execution/engine/**/*_test.go: Inline all literal values (cache keys, hashes, byte sizes, query strings, expected responses) directly in assertions and setup code—no const blocks or named variables for expected values."Also applies to: 297-297, 310-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/engine/federation_caching_root_split_test.go` around lines 281 - 291, Remove the named variable query and inline the GraphQL query string literal at each call site where query is used (the occurrences flagged around lines 281, 297, and 310); locate the test function that declares query and replace references to query with the exact multiline string literal shown in the diff so the test adheres to the execution/engine convention of inlining all literal values.
59-59: UseQueryStringWithHeadersin these execution/engine requests.This file consistently uses
QueryString, but the package rule requiresQueryStringWithHeaderswith inline query strings.As per coding guidelines: "
execution/engine/**/*_test.go: Use QueryStringWithHeaders with inline GraphQL query strings—do not load queries from external files."Also applies to: 86-86, 137-137, 190-190, 212-212, 297-297, 310-310, 352-352, 363-363
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/engine/federation_caching_root_split_test.go` at line 59, Replace the inline GraphQL test calls that use gqlClient.QueryString with gqlClient.QueryStringWithHeaders in execution/engine/federation_caching_root_split_test.go; specifically update each occurrence of gqlClient.QueryString(ctx, setup.GatewayServer.URL, `{ ... }`, nil, t) to gqlClient.QueryStringWithHeaders(ctx, setup.GatewayServer.URL, `{ ... }`, nil /* headers */, nil, t) or the correct overload in your test helpers (pass an empty/nil headers map as required) for the lines mentioned (including the instances at the reported locations such as the call shown and the other occurrences around lines 86, 137, 190, 212, 297, 310, 352, 363) so all inline query strings use QueryStringWithHeaders per the package rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@execution/engine/federation_caching_entity_field_args_test.go`:
- Around line 49-127: Shared test setup (entityFieldArgsSetup,
newEntityFieldArgsSetup and the parent-level peekCache) creates cross-subtest
shared state which violates this package's self-contained subtest convention;
fix by inlining the setup into each t.Run body: remove or stop using
entityFieldArgsSetup and newEntityFieldArgsSetup, move their initialization
logic (cache creation, tracker/http client, federationtesting.NewFederationSetup
with addCachingGateway/options, gqlClient, ctx/cancel, and host URL parsing)
directly inside each subtest, and convert peekCache into a subtest-local helper
or inline its logic so no state is shared across tests (search for
entityFieldArgsSetup, newEntityFieldArgsSetup, and peekCache to update all
usages).
- Around line 164-165: Replace calls to s.gqlClient.QueryString(...) with
s.gqlClient.QueryStringWithHeaders(...), passing an explicit headers argument
(e.g., nil or the required map) as the additional parameter and keep the GraphQL
queries inline in the call; update every usage in this test file (e.g., the call
site s.gqlClient.QueryString(s.ctx, s.setup.GatewayServer.URL, query, nil, t)
should become s.gqlClient.QueryStringWithHeaders(s.ctx,
s.setup.GatewayServer.URL, query, headers, t)) so that all execution/engine
GraphQL test calls follow the required QueryStringWithHeaders convention.
In `@execution/engine/federation_caching_root_split_test.go`:
- Around line 295-318: Two calls to defaultCache.ClearLog() in this subtest are
not followed by assertions on the cache log; update the test to assert
defaultCache.GetLog() immediately after each ClearLog() (before tracker.Reset()
/ the next ClearLog()) to satisfy the cache-log rule. Specifically, after the
first defaultCache.ClearLog() (before tracker.Reset()/resp :=
gqlClient.QueryString(...)) add an assertion checking defaultCache.GetLog()
equals the expected initial state (e.g., empty or the known entries), and after
the second defaultCache.ClearLog() (before the warm-path query) assert
defaultCache.GetLog() again; use defaultCache.GetLog() and assert.Equal to
validate the log and keep the existing checks for
tracker.GetCount(accountsHost/productsHost/reviewsHost).
In `@execution/engine/federation_caching_source_test.go`:
- Around line 181-182: Replace the file-path indirection via
cachingTestQueryPath in the failing subtests with an inline GraphQL string and
use the test helper QueryStringWithHeaders instead of loading from file;
specifically, remove calls to
cachingTestQueryPath("subscriptions/subscription_product_only.query") (and the
similar call at the other location) and pass the literal subscription query
string directly to QueryStringWithHeaders along with the existing queryVariables
and expected count so the subtests follow the execution/engine guideline
requiring inline queries.
- Around line 264-266: Replace the piecemeal assertions on invalidateCalls with
a single structural equality check: construct the expected slice value (matching
the struct type stored in invalidateCalls) with one element having entityType
"Product" and keys []string{`{"__typename":"Product","key":{"upc":"top-4"}}`}
and assert assert.Equal(t, expectedInvalidateCalls, invalidateCalls); this edits
the assertions around invalidateCalls/OnSubscriptionCacheInvalidate to compare
the full struct/slice in one call rather than checking len, entityType, and keys
separately.
In `@execution/engine/federation_caching_test.go`:
- Around line 478-497: The parent-level variables subgraphCachingConfigs and
mutationVars are shared across multiple t.Run subtests; move their definitions
into each t.Run so every subtest is self-contained and does not rely on
parent-scoped state. Specifically, copy the current subgraphCachingConfigs
(including EntityCaching and MutationFieldCaching entries) and the mutationVars
(the queryVariables map) into the setup block of each t.Run that uses them,
ensuring any modifications or assertions operate on local variables unique to
that subtest and referencing the same symbols (subgraphCachingConfigs,
mutationVars, plan.EntityCacheConfigurations,
plan.MutationFieldCacheConfigurations, queryVariables) when relocating them.
- Around line 199-204: Remove the named local query variable firstQuery and pass
the query string literal directly into the gqlClient.QueryString call; i.e.,
replace the use of firstQuery in the call to gqlClient.QueryString(ctx,
setup.GatewayServer.URL, firstQuery, nil, t) with the inline query literal
(`query { topProducts { name } }`) so the GraphQL query is expressed at the
invocation site per the test-file guideline.
- Line 75: The test is loading the GraphQL operation from a file via
cachingTestQueryPath, but this package requires inline query strings; replace
the call to gqlClient.Query(...,
cachingTestQueryPath("queries/multiple_upstream_without_provides.query"), nil,
t) with the appropriate inline-call form using QueryStringWithHeaders and an
inline GraphQL string (i.e., provide the full query text directly in the call),
keep the same ctx, setup.GatewayServer.URL, nil headers and t arguments, and
remove use of cachingTestQueryPath to make the subtest self-contained.
In `@execution/engine/federation_subscription_caching_test.go`:
- Around line 298-303: Replace the non-deterministic assert.Eventually usage
with a single exact-state assertion: stop using assert.Eventually around
defaultCache.Peek and instead deterministically drive TTL expiry (e.g., advance
the test clock or call the cache expiry/flush helper) so you can immediately
assert the expected state with a single assertion like require.False/
assert.False on defaultCache.Peek for the two keys; update the test to remove
the Eventually block and use deterministic expiry via the test clock or cache
expiration API before calling trigger.Emit and asserting that both Peek calls
return false.
- Around line 594-603: The test calls defaultCache.ClearLog() in several
subtests (e.g., the branch with productQuery and tracker.Reset()) but never
follows the ClearLog() contract by asserting the cache entries via
defaultCache.GetLog(); add explicit calls to defaultCache.GetLog() and full
assertions of expected log contents immediately before each subtest ends (before
any subsequent ClearLog() or return), for the instances around the productQuery
branch (the block using defaultCache.ClearLog(), tracker.Reset(), productQuery,
resp, and productsCallsQuery via tracker.GetCount(productsHost)) and the other
listed ranges (652-681, 1151-1165) so every ClearLog() is paired with a GetLog()
+ assertions per the test guidelines.
- Around line 1078-1082: The test currently uses assert.NotContains to check
that item.Key (iterating cacheLog -> entry.Items -> item.Key) does not include
the substring `"fieldName":"updateProductPrice"`, which violates the "exact
assertions only" rule; replace this with an exact-structure assertion: either
build the expected cacheLog structure and use assert.Equal(t, expectedCacheLog,
cacheLog) to compare full structures, or for a smaller change replace the
NotContains check with an exact inequality assertion like assert.NotEqual(t,
`"fieldName":"updateProductPrice"`, item.Key, "subscription root field must not
be cached") so the test uses an exact assertion against the full value of
item.Key instead of a Contains check.
- Around line 132-133: Replace the file-path indirection call
cachingTestQueryPath("subscriptions/subscription_product_with_reviews.query")
with an inline GraphQL document string and pass that inline string into the test
helper that expects a query document (use QueryStringWithHeaders where
appropriate); update the subtest invocation that currently passes
cachingTestQueryPath to instead provide the raw query string literal directly so
the test uses an inline GraphQL query rather than loading from a file.
In `@execution/engine/partial_cache_test.go`:
- Around line 83-85: The test currently loads GraphQL operations from files via
partialCacheTestQueryPath and calls like gqlClient.Query(...); instead, inline
the GraphQL documents directly in the test using QueryStringWithHeaders so the
operation is visible in the test. Replace uses of partialCacheTestQueryPath(...)
and gqlClient.Query(...) with gqlClient.QueryStringWithHeaders(ctx,
operationName, `...graphql query...`, headers) (or the equivalent
QueryStringWithHeaders signature used in this package), remove the
partialCacheTestQueryPath helper, and update the three occurrences noted (around
lines 83, 156, 228, 307) to embed the exact GraphQL strings inline. Ensure
operationName and any headers/variables are preserved when switching to
QueryStringWithHeaders.
- Around line 152-153: Several test locations call defaultCache.ClearLog()
without subsequently asserting the log; either remove the ClearLog() calls or
capture and assert the log before clearing. Locate each use of
defaultCache.ClearLog() in the test (and any following subtest blocks), and if
the cache activity is not part of the test assertions delete the ClearLog()
call; otherwise call defaultCache.GetLog(), assert the expected entries, then
call defaultCache.ClearLog() only after those assertions. Ensure every
ClearLog() is paired with a preceding GetLog() + assertions or removed.
---
Duplicate comments:
In `@execution/engine/federation_subscription_caching_test.go`:
- Around line 21-76: The new shared test helper functions (toWSAddr,
mustRecvMessage, boolToInt, collectSubscriptionMessages) violate the rule
forbidding shared helpers in execution/engine/ tests; remove these helpers from
this file and instead inline their logic into the individual subtests that use
them (or, if you have explicit approval, move them to an approved shared test
utility package). Specifically, replace calls to collectSubscriptionMessages
(and its use of GraphqlClient.Subscription and
federationtesting.FederationSetup.NextProductSubscription) with the equivalent
inline subscription setup and message loop in each test, and inline the small
utilities toWSAddr, mustRecvMessage, and boolToInt where needed within those
tests so no new package-level helpers remain.
---
Nitpick comments:
In `@execution/engine/federation_caching_root_split_test.go`:
- Around line 281-291: Remove the named variable query and inline the GraphQL
query string literal at each call site where query is used (the occurrences
flagged around lines 281, 297, and 310); locate the test function that declares
query and replace references to query with the exact multiline string literal
shown in the diff so the test adheres to the execution/engine convention of
inlining all literal values.
- Line 59: Replace the inline GraphQL test calls that use gqlClient.QueryString
with gqlClient.QueryStringWithHeaders in
execution/engine/federation_caching_root_split_test.go; specifically update each
occurrence of gqlClient.QueryString(ctx, setup.GatewayServer.URL, `{ ... }`,
nil, t) to gqlClient.QueryStringWithHeaders(ctx, setup.GatewayServer.URL, `{ ...
}`, nil /* headers */, nil, t) or the correct overload in your test helpers
(pass an empty/nil headers map as required) for the lines mentioned (including
the instances at the reported locations such as the call shown and the other
occurrences around lines 86, 137, 190, 212, 297, 310, 352, 363) so all inline
query strings use QueryStringWithHeaders per the package rule.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 980530d8-3915-4843-9e99-f597aa6afcbc
📒 Files selected for processing (42)
CLAUDE.mddocs/entity-caching/ENTITY_CACHING_ACCEPTANCE_CRITERIA.mdexecution/engine/federation_caching_batch_test.goexecution/engine/federation_caching_entity_field_args_test.goexecution/engine/federation_caching_ext_invalidation_test.goexecution/engine/federation_caching_helpers_test.goexecution/engine/federation_caching_l2_test.goexecution/engine/federation_caching_remap_variables_test.goexecution/engine/federation_caching_root_args_test.goexecution/engine/federation_caching_root_entity_test.goexecution/engine/federation_caching_root_split_test.goexecution/engine/federation_caching_source_test.goexecution/engine/federation_caching_test.goexecution/engine/federation_subscription_caching_test.goexecution/engine/partial_cache_test.gov2/pkg/engine/plan/caching_planner_state.gov2/pkg/engine/plan/planner.gov2/pkg/engine/plan/planner_test.gov2/pkg/engine/plan/representation_variable.gov2/pkg/engine/plan/request_scoped_provides_data_test.gov2/pkg/engine/plan/visitor.gov2/pkg/engine/plan/visitor_path_normalization_test.gov2/pkg/engine/plan/visitor_subscription_entity_population_test.gov2/pkg/engine/resolve/batch_entity_cache_test.gov2/pkg/engine/resolve/cache_analytics.gov2/pkg/engine/resolve/cache_key_parity_test.gov2/pkg/engine/resolve/cache_load_test.gov2/pkg/engine/resolve/caching_overhead_bench_test.gov2/pkg/engine/resolve/circuit_breaker.gov2/pkg/engine/resolve/circuit_breaker_test.gov2/pkg/engine/resolve/entity_cache_partial_writeback_regression_test.gov2/pkg/engine/resolve/entity_merge_path_test.gov2/pkg/engine/resolve/extensions_cache_invalidation_test.gov2/pkg/engine/resolve/l1_cache_test.gov2/pkg/engine/resolve/l1_l2_cache_e2e_test.gov2/pkg/engine/resolve/l2_cache_key_interceptor_test.gov2/pkg/engine/resolve/loader.gov2/pkg/engine/resolve/loader_cache.gov2/pkg/engine/resolve/mutation_cache_test.gov2/pkg/engine/resolve/negative_cache_test.gov2/pkg/engine/resolve/resolve.gov2/pkg/engine/resolve/trigger_cache_test.go
✅ Files skipped from review due to trivial changes (2)
- execution/engine/federation_caching_root_entity_test.go
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/entity-caching/ENTITY_CACHING_ACCEPTANCE_CRITERIA.md
- execution/engine/federation_caching_ext_invalidation_test.go
- execution/engine/federation_caching_batch_test.go
- execution/engine/federation_caching_helpers_test.go
- execution/engine/federation_caching_l2_test.go
Comment on lines
+49
to
+127
| // entityFieldArgsSetup holds common test infrastructure for entity field args caching tests. | ||
| type entityFieldArgsSetup struct { | ||
| setup *federationtesting.FederationSetup | ||
| gqlClient *GraphqlClient | ||
| ctx context.Context | ||
| cancel context.CancelFunc | ||
| defaultCache *FakeLoaderCache | ||
| tracker *subgraphCallTracker | ||
| accountsHost string | ||
| productsHost string | ||
| reviewsHost string | ||
| } | ||
|
|
||
| func newEntityFieldArgsSetup(t *testing.T) *entityFieldArgsSetup { | ||
| t.Helper() | ||
|
|
||
| defaultCache := NewFakeLoaderCache() | ||
| caches := map[string]resolve.LoaderCache{ | ||
| "default": defaultCache, | ||
| } | ||
|
|
||
| tracker := newSubgraphCallTracker(http.DefaultTransport) | ||
| trackingClient := &http.Client{ | ||
| Transport: tracker, | ||
| } | ||
|
|
||
| subgraphCachingConfigs := engine.SubgraphCachingConfigs{ | ||
| { | ||
| SubgraphName: "products", | ||
| RootFieldCaching: plan.RootFieldCacheConfigurations{ | ||
| {TypeName: "Query", FieldName: "topProducts", CacheName: "default", TTL: 30 * time.Second, IncludeSubgraphHeaderPrefix: false}, | ||
| }, | ||
| }, | ||
| { | ||
| SubgraphName: "reviews", | ||
| EntityCaching: plan.EntityCacheConfigurations{ | ||
| {TypeName: "Product", CacheName: "default", TTL: 30 * time.Second, IncludeSubgraphHeaderPrefix: false}, | ||
| }, | ||
| }, | ||
| { | ||
| SubgraphName: "accounts", | ||
| EntityCaching: plan.EntityCacheConfigurations{ | ||
| {TypeName: "User", CacheName: "default", TTL: 30 * time.Second, IncludeSubgraphHeaderPrefix: false}, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| setup := federationtesting.NewFederationSetup(addCachingGateway( | ||
| withCachingEnableART(false), | ||
| withCachingLoaderCache(caches), | ||
| withHTTPClient(trackingClient), | ||
| withCachingOptionsFunc(resolve.CachingOptions{EnableL2Cache: true}), | ||
| withSubgraphEntityCachingConfigs(subgraphCachingConfigs), | ||
| )) | ||
| t.Cleanup(setup.Close) | ||
|
|
||
| gqlClient := NewGraphqlClient(http.DefaultClient) | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| t.Cleanup(cancel) | ||
|
|
||
| accountsURLParsed, err := url.Parse(setup.AccountsUpstreamServer.URL) | ||
| require.NoError(t, err) | ||
| productsURLParsed, err := url.Parse(setup.ProductsUpstreamServer.URL) | ||
| require.NoError(t, err) | ||
| reviewsURLParsed, err := url.Parse(setup.ReviewsUpstreamServer.URL) | ||
| require.NoError(t, err) | ||
|
|
||
| return &entityFieldArgsSetup{ | ||
| setup: setup, | ||
| gqlClient: gqlClient, | ||
| ctx: ctx, | ||
| cancel: cancel, | ||
| defaultCache: defaultCache, | ||
| tracker: tracker, | ||
| accountsHost: accountsURLParsed.Host, | ||
| productsHost: productsURLParsed.Host, | ||
| reviewsHost: reviewsURLParsed.Host, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Inline setup per t.Run; shared helpers break execution/engine test conventions.
entityFieldArgsSetup, newEntityFieldArgsSetup, and the parent-level peekCache helper make subtests depend on shared setup/state, which conflicts with the required self-contained subtest style in this package.
As per coding guidelines, "execution/engine/**/*_test.go: Self-contained subtests ... do NOT extract setup into shared helpers like newXxxFederationTestEnv(...)" and "Inline setup ... belongs inside each subtest body`."
Also applies to: 133-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@execution/engine/federation_caching_entity_field_args_test.go` around lines
49 - 127, Shared test setup (entityFieldArgsSetup, newEntityFieldArgsSetup and
the parent-level peekCache) creates cross-subtest shared state which violates
this package's self-contained subtest convention; fix by inlining the setup into
each t.Run body: remove or stop using entityFieldArgsSetup and
newEntityFieldArgsSetup, move their initialization logic (cache creation,
tracker/http client, federationtesting.NewFederationSetup with
addCachingGateway/options, gqlClient, ctx/cancel, and host URL parsing) directly
inside each subtest, and convert peekCache into a subtest-local helper or inline
its logic so no state is shared across tests (search for entityFieldArgsSetup,
newEntityFieldArgsSetup, and peekCache to update all usages).
Comment on lines
+164
to
+165
| resp := s.gqlClient.QueryString(s.ctx, s.setup.GatewayServer.URL, query, nil, t) | ||
|
|
Contributor
There was a problem hiding this comment.
Use QueryStringWithHeaders for execution/engine GraphQL calls (applies file-wide).
At Line 164 (and similarly throughout this file), calls use QueryString(...). The package rule requires QueryStringWithHeaders with inline query strings for these tests.
As per coding guidelines, "execution/engine/**/*_test.go: Use QueryStringWithHeaders with inline GraphQL query strings—do not load queries from files`."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@execution/engine/federation_caching_entity_field_args_test.go` around lines
164 - 165, Replace calls to s.gqlClient.QueryString(...) with
s.gqlClient.QueryStringWithHeaders(...), passing an explicit headers argument
(e.g., nil or the required map) as the additional parameter and keep the GraphQL
queries inline in the call; update every usage in this test file (e.g., the call
site s.gqlClient.QueryString(s.ctx, s.setup.GatewayServer.URL, query, nil, t)
should become s.gqlClient.QueryStringWithHeaders(s.ctx,
s.setup.GatewayServer.URL, query, headers, t)) so that all execution/engine
GraphQL test calls follow the required QueryStringWithHeaders convention.
Four findings from coderabbit, all in execution/engine/ tests, all
applying the package conventions documented in CLAUDE.md.
federation_caching_root_split_test.go (the only finding we introduced):
- Removed a pointless initial ClearLog and added explicit GetLog()
assertions for both cold-path (6 cache operations across accounts /
products / reviews) and warm-path (all hits, no further set) per the
cache log rule.
federation_caching_entity_field_args_test.go (pre-existing patterns
flagged retroactively by the new self-contained-subtest rule):
- Removed shared entityFieldArgsSetup struct, newEntityFieldArgsSetup
constructor, and parent-level peekCache helper. Each subtest now
inlines its own cache, tracker, gateway setup, ctx, and gqlClient.
- Switched all gqlClient.QueryString(...) calls to QueryStringWithHeaders.
federation_caching_source_test.go (pre-existing pattern):
- Replaced the two cachingTestQueryPath("subscriptions/...") calls with
inline subscription documents. graphql_client_test.go's subscription
helper accepts inline operation strings while preserving the file-path
compatibility used by other suites.
Tests: full resolve + execution suites pass; resolve passes under -race;
gofmt + gci clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
execution/engine/federation_caching_source_test.go (1)
277-282:⚠️ Potential issue | 🟡 MinorAssert
invalidateCallsas one exact slice value.The current len-plus-field checks can miss shape and ordering regressions. Compare the full slice in one
assert.Equal.Suggested change
// Assert entire callback data — exactly 1 invalidation call mu.Lock() defer mu.Unlock() - require.Equal(t, 1, len(invalidateCalls), "OnSubscriptionCacheInvalidate should be called exactly once") - assert.Equal(t, "Product", invalidateCalls[0].entityType) - assert.Equal(t, []string{`{"__typename":"Product","key":{"upc":"top-4"}}`}, invalidateCalls[0].keys) + assert.Equal(t, []struct { + entityType string + keys []string + }{ + { + entityType: "Product", + keys: []string{ + `{"__typename":"Product","key":{"upc":"top-4"}}`, + }, + }, + }, invalidateCalls) }) }As per coding guidelines:
**/*_test.go: “Always assert entire structs with assert.Equal on the complete struct, never iterate over fields with individual assertions.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/engine/federation_caching_source_test.go` around lines 277 - 282, Replace the separate length and field assertions with a single equality check that asserts the entire slice value at once: lock the mutex as currently done (mu.Lock()/defer mu.Unlock()), then call assert.Equal(t, []<type>{expectedValue}, invalidateCalls) (using the actual test's Invalidate call struct type and build the expected element with entityType "Product" and keys []string{`{"__typename":"Product","key":{"upc":"top-4"}}`}) so the test compares the whole invalidateCalls slice in one shot instead of checking len and individual fields (referencing invalidateCalls and the current per-element fields invalidateCalls[0].entityType and invalidateCalls[0].keys to construct the expected value).
🧹 Nitpick comments (2)
execution/engine/federation_caching_source_test.go (1)
140-141: Rename the callbacks in the doc comment.The tests cover
OnSubscriptionCacheWriteandOnSubscriptionCacheInvalidate, notOnSubscriptionCacheHit/OnSubscriptionCacheSet. The current comment points readers at the wrong API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/engine/federation_caching_source_test.go` around lines 140 - 141, Update the doc comment for TestOnSubscriptionCacheCallbacks to reference the correct callback names: replace mentions of OnSubscriptionCacheHit and OnSubscriptionCacheSet with OnSubscriptionCacheWrite and OnSubscriptionCacheInvalidate so the comment matches the actual tested API (OnSubscriptionCacheWrite, OnSubscriptionCacheInvalidate) and avoids pointing readers to the wrong callbacks.execution/engine/graphql_client_test.go (1)
132-140: Prefix-based query/path detection is unreliable.This will misclassify valid GraphQL documents that start with comments or
fragment, and it can also misclassify relative file names likequery_product.graphqlas inline documents. Please make the mode explicit instead of guessing from prefixes.Possible direction
-func (g *GraphqlClient) Subscription(ctx context.Context, addr, queryOrFilePath string, variables queryVariables, t *testing.T) (chan []byte, func()) { +func (g *GraphqlClient) SubscriptionString(ctx context.Context, addr, query string, variables queryVariables, t *testing.T) (chan []byte, func()) { + payload := requestBody(t, query, variables) + // ... +} + +func (g *GraphqlClient) SubscriptionFile(ctx context.Context, addr, queryFilePath string, variables queryVariables, t *testing.T) (chan []byte, func()) { + payload := loadQuery(t, queryFilePath, variables) + // ... +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/engine/graphql_client_test.go` around lines 132 - 140, The current prefix-based detection using trimmedQuery and the conditional that chooses between requestBody(...) and loadQuery(...) is unreliable (comments, fragments, filenames with prefixes); change the API to require an explicit mode/flag (e.g., a boolean isFile or enum mode) instead of guessing and update the logic in the function that currently trims queryOrFilePath to consult that explicit flag to call either requestBody(t, ...) or loadQuery(t, ...); update any callers to pass the new mode parameter and adjust tests to exercise both explicit inline-document and file-path flows (refer to trimmedQuery, requestBody, loadQuery, and variables to locate the code to change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@execution/engine/federation_caching_source_test.go`:
- Line 255: The test currently calls defaultCache.ClearLog() which discards
evidence of the pre-population Set; instead, before calling
defaultCache.ClearLog() assert the seed write by calling defaultCache.GetLog()
and validating it contains the expected seed entry (e.g., a Set/Put record for
the seed key or operation inserted during pre-population) and any expected
metadata, then only call defaultCache.ClearLog() afterwards; update the test
around defaultCache.ClearLog() to perform these GetLog() assertions (referencing
defaultCache.GetLog() and the seed key used in the pre-population Set) so the
seed write is verified before clearing.
---
Duplicate comments:
In `@execution/engine/federation_caching_source_test.go`:
- Around line 277-282: Replace the separate length and field assertions with a
single equality check that asserts the entire slice value at once: lock the
mutex as currently done (mu.Lock()/defer mu.Unlock()), then call assert.Equal(t,
[]<type>{expectedValue}, invalidateCalls) (using the actual test's Invalidate
call struct type and build the expected element with entityType "Product" and
keys []string{`{"__typename":"Product","key":{"upc":"top-4"}}`}) so the test
compares the whole invalidateCalls slice in one shot instead of checking len and
individual fields (referencing invalidateCalls and the current per-element
fields invalidateCalls[0].entityType and invalidateCalls[0].keys to construct
the expected value).
---
Nitpick comments:
In `@execution/engine/federation_caching_source_test.go`:
- Around line 140-141: Update the doc comment for
TestOnSubscriptionCacheCallbacks to reference the correct callback names:
replace mentions of OnSubscriptionCacheHit and OnSubscriptionCacheSet with
OnSubscriptionCacheWrite and OnSubscriptionCacheInvalidate so the comment
matches the actual tested API (OnSubscriptionCacheWrite,
OnSubscriptionCacheInvalidate) and avoids pointing readers to the wrong
callbacks.
In `@execution/engine/graphql_client_test.go`:
- Around line 132-140: The current prefix-based detection using trimmedQuery and
the conditional that chooses between requestBody(...) and loadQuery(...) is
unreliable (comments, fragments, filenames with prefixes); change the API to
require an explicit mode/flag (e.g., a boolean isFile or enum mode) instead of
guessing and update the logic in the function that currently trims
queryOrFilePath to consult that explicit flag to call either requestBody(t, ...)
or loadQuery(t, ...); update any callers to pass the new mode parameter and
adjust tests to exercise both explicit inline-document and file-path flows
(refer to trimmedQuery, requestBody, loadQuery, and variables to locate the code
to change).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf55a187-132d-4e2d-8e81-4434df4d1339
📒 Files selected for processing (7)
execution/engine/federation_caching_entity_field_args_test.goexecution/engine/federation_caching_root_split_test.goexecution/engine/federation_caching_source_test.goexecution/engine/graphql_client_test.gov2/pkg/engine/plan/planner_test.gov2/pkg/engine/resolve/cache_key_parity_test.gov2/pkg/engine/resolve/extensions_cache_invalidation_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- execution/engine/federation_caching_root_split_test.go
coderabbit flagged one seed-Set-then-ClearLog pattern in federation_caching_source_test.go that drops the only proof the seed write happened. A focused scan found the same pattern at 8 other sites across partial_cache_test.go and federation_subscription_caching_test.go. Adds an inline GetLog() + assert.Equal for the expected seed state immediately before each ClearLog, per the cache log rule: > every defaultCache.ClearLog() must be followed by GetLog() + > assertions before the next ClearLog() or end of test The special site at federation_subscription_caching_test.go:804 also performs a Get between Set and ClearLog; the assertion captures both operations in order. Test fixture only; no production change. Full execution + resolve suites pass; resolve passes under -race. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
execution/engine/partial_cache_test.go (1)
83-85:⚠️ Potential issue | 🟠 MajorInline the GraphQL operation in each subtest instead of loading from
testdata.The test still depends on
partialCacheTestQueryPath(...)+gqlClient.Query(...), so the operation is not self-contained in eacht.Run. Please switch these calls to inline query strings (e.g.,QueryStringWithHeaders) and remove the path helper.As per coding guidelines:
execution/engine/**/*_test.go: useQueryStringWithHeaderswith the query string inline, and do not load queries from external files.Also applies to: 160-160, 236-236, 319-319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@execution/engine/partial_cache_test.go` around lines 83 - 85, The tests currently read GraphQL operations from files via partialCacheTestQueryPath and gqlClient.Query; instead, inline each operation string directly in the subtests and call QueryStringWithHeaders (or the equivalent inline-query helper) so each t.Run is self-contained; replace uses of partialCacheTestQueryPath(...) + gqlClient.Query(...) with QueryStringWithHeaders("<GRAPHQL QUERY HERE>", headers) and remove the now-unused partialCacheTestQueryPath helper and any file-loading logic, and update the other similar occurrences in this test file to match.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@execution/engine/partial_cache_test.go`:
- Around line 336-395: The new shared E2E test helpers
(partialCacheGatewayOptions, withPartialCacheLoaderCache,
withPartialCacheHTTPClient, withPartialCacheCachingOptions,
withPartialCacheSubgraphCachingConfigs, partialCacheGatewayOptionsToFunc and
addPartialCacheGateway) violate the package convention of self-contained t.Run
tests; remove these helpers and inline their logic into each relevant t.Run
instead: copy the addPartialCacheGateway implementation (creating httpClient
fallback, building gateway.NewDatasource with the three ServiceConfig entries,
calling gateway.HandlerWithCaching with abstractlogger.NoopLogger, poller,
httpClient, opts.withLoaderCache, nil, opts.cachingOptions,
opts.subgraphEntityCachingConfigs, false, creating ctx with context.WithTimeout,
calling poller.Run(ctx) and httptest.NewServer(gtw)) directly into each subtest
that used addPartialCacheGateway, and replace references to the
withPartialCache* option helpers with local variables in the test body so each
t.Run is self-contained and readable.
---
Duplicate comments:
In `@execution/engine/partial_cache_test.go`:
- Around line 83-85: The tests currently read GraphQL operations from files via
partialCacheTestQueryPath and gqlClient.Query; instead, inline each operation
string directly in the subtests and call QueryStringWithHeaders (or the
equivalent inline-query helper) so each t.Run is self-contained; replace uses of
partialCacheTestQueryPath(...) + gqlClient.Query(...) with
QueryStringWithHeaders("<GRAPHQL QUERY HERE>", headers) and remove the
now-unused partialCacheTestQueryPath helper and any file-loading logic, and
update the other similar occurrences in this test file to match.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 56d64eca-9a38-49e4-9498-5e0b5f0c1c4b
📒 Files selected for processing (5)
execution/engine/execution_engine_cost_test.goexecution/engine/execution_engine_test.goexecution/engine/federation_caching_source_test.goexecution/engine/federation_subscription_caching_test.goexecution/engine/partial_cache_test.go
✅ Files skipped from review due to trivial changes (1)
- execution/engine/federation_caching_source_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- execution/engine/execution_engine_test.go
Address 8 unresolved CodeRabbit review threads on execution/engine/ test files, all violations of execution/engine/CLAUDE.md test conventions: - federation_caching_source_test.go: assert full invalidateCalls slice as one struct literal instead of len + per-field checks. - federation_caching_test.go: inline all cachingTestQueryPath(...) calls as raw-string GraphQL literals; remove firstQuery named local; make each mutation subtest own its subgraphCachingConfigs and mutationVars instead of sharing them at parent scope. - federation_subscription_caching_test.go: inline all subscription cachingTestQueryPath(...) calls; remove queryPath := named locals; replace assert.Eventually for the User L2 TTL expiry test with a deterministic fake-clock approach and exact Peek assertions; replace assert.NotContains with the existing full cache-log assertion; drop unasserted defaultCache.ClearLog() calls. - partial_cache_test.go: remove partialCacheTestQueryPath, the partialCacheGatewayOptions/withPartialCache*/addPartialCacheGateway helper pattern, and inline gateway construction inside each t.Run. Inline subgraphCachingConfigs, expectedResponse, expectedReviewsRequest per subtest. Drop unasserted defaultCache.ClearLog() calls. - federation_caching_helpers_test.go: add nil-safe currentTime() / setCurrentTime() fake-clock to FakeLoaderCache (used by the subscription TTL expiry test). Backwards compatible: existing tests that don't call setCurrentTime fall back to wall clock. The remaining unresolved thread (SkArchon @ loader_cache.go:2813, RecordMutationEvent for extension-driven invalidation) was already addressed in commit 0f049c3. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
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
4 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 by CodeRabbit
New Features
onErrorextension (PROPAGATE,NULL,HALTmodes).Documentation