feat(plan): federation correctness and parity fixes (paired with cosmo)

[jensneuse]

Five federation query-planner fixes (paired with composition changes in the cosmo
PR), each strictly gated for zero plan-snapshot regression (0 diffs across 220
real federated graphs / 13,143 operations):

• honor @provides over union-typed fields — read the provided union-member field
  inline instead of an entity fetch to the owner.
• resolve abstract field whose union members are split across datasources — was an
  empty selection set → HTTP 500; now resolves.
• split entity fetch for @requires with conflicting field arguments — same field
  required with different args collapsed to one value (silently wrong data); now
  spec-correct (matches an alternative federation router).
• honor @provides over interface-typed fields — match interface-keyed provides
  against the rewritten concrete fragment paths.
• synthesize multi-hop compound @key — gather a missing compound-key member from a
  third subgraph (two-stage entity fetch), gated as a strict fallback (the un-gated
  synthesis drifts ~114 ops; gated → 0).

Each fix is validated against the federation-gateway-audit and has a
full-plan unit test. Based on v2.5.0.

See the cosmo PR for the decision records (adr/), the reviewer guide
(docs/planner-improvements.md), and the router e2e tests.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3f2b784c-68f2-424f-9edc-d315ffe465e9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The PR updates GraphQL federation planning to carry fallback source paths, compare required-field argument signatures, skip union rewrites for selected fields, and select abstract-field routes with additional cleanup. It also adds tests for interface provides, union provides, shareable unions, fallback routes, and multi-hop compound-key planning.

Changes

Federation planner routing

Layer / File(s)	Summary
Downstream argument import v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go	addFieldArguments now imports downstream arguments when no explicit argument configuration exists upstream.
Fallback source connections v2/pkg/engine/plan/source_connection_graph.go, v2/pkg/engine/plan/source_connection_graph_test.go	KeyJump and path lookup distinguish fallback routes, and tests cover subset-key fallback lookup and exact-route precedence.
Required-argument conflicts v2/pkg/engine/plan/federation_metadata.go, v2/pkg/engine/plan/path_builder_visitor.go, v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_requires_arguments_test.go	Required-field argument signatures are compared for conflicts, planner reuse skips mismatched requires configs, and the new test covers different currency arguments on the same field.
Abstract-field selection v2/pkg/engine/plan/abstract_selection_rewriter.go, v2/pkg/engine/plan/datasource_filter_*.go, v2/pkg/engine/plan/path_builder.go	Abstract-field rewrite skipping, provided-field collection, datasource selection, node suggestion lookup, and duplicate-leaf pruning now account for fragment context, union/interface membership, and fallback-key-aware routing.
Abstract-field federation tests v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_interface_provides_test.go, v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_provides_test.go, v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go	New federation tests cover interface-provided fields, union @provides, and shareable unions split across subgraphs.
Fallback-key node selection v2/pkg/engine/plan/node_selection_*.go, v2/pkg/engine/plan/multihop_compound_key_test.go, v2/pkg/engine/datasource/graphql_datasource/multihop_compound_key_test.go	Node selection now refilters with fallback key jumps, prunes stale requirements, and records jump source paths; the new multi-hop tests assert the resulting planner route.

Review effort

🎯 5 (Critical) | ⏱️ ~90+ minutes

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: federation correctness and parity fixes in the plan layer.
Description check	✅ Passed	The description clearly matches the changeset, listing the five federation planner fixes and their gated validation.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch planner-improvements

---

_{Comment @coderabbitai help to get the list of available commands.}

[jensneuse]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (1)

v2/pkg/engine/plan/federation_metadata.go (1)

199-235: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Passing FederationFieldConfiguration by value defeats the parseSelectionSet cache.

parseSelectionSet has a pointer receiver and memoizes the parsed document into f.parsedSelectionSet. Since both requiredFieldArgumentConflict and requiredFieldArgumentsByPath take the config by value, the cache is written to a local copy and discarded, so the selection set is re-parsed on every invocation. HasArgumentConflictWith calls requiredFieldArgumentConflict in an i*j loop, so each requires selection set is parsed repeatedly on the planning path.

Passing pointers (the loop elements (*f)[i] and configs[j] are addressable) lets the memoization take effect.

♻️ Proposed change

-func (f *FederationFieldConfigurations) HasArgumentConflictWith(configs []FederationFieldConfiguration) bool {
-	for i := range *f {
-		for j := range configs {
-			if requiredFieldArgumentConflict((*f)[i], configs[j]) {
+func (f *FederationFieldConfigurations) HasArgumentConflictWith(configs []FederationFieldConfiguration) bool {
+	for i := range *f {
+		for j := range configs {
+			if requiredFieldArgumentConflict(&(*f)[i], &configs[j]) {
 				return true
 			}
 		}
 	}

 	return false
 }

-func requiredFieldArgumentConflict(left, right FederationFieldConfiguration) bool {
+func requiredFieldArgumentConflict(left, right *FederationFieldConfiguration) bool {
 	if left.TypeName != right.TypeName {
 		return false
 	}
@@
-func requiredFieldArgumentsByPath(config FederationFieldConfiguration) (map[string]string, bool) {
+func requiredFieldArgumentsByPath(config *FederationFieldConfiguration) (map[string]string, bool) {
 	if err := config.parseSelectionSet(); err != nil {

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@v2/pkg/engine/plan/federation_metadata.go` around lines 199 - 235, The
parsing cache is being lost because requiredFieldArgumentConflict and
requiredFieldArgumentsByPath take FederationFieldConfiguration by value, so
parseSelectionSet() memoizes only on a copy. Update these helpers to work with
pointers and call them from HasArgumentConflictWith using the addressable loop
elements (for example, the configs accessed in the i/j loop) so
parseSelectionSet() can reuse the cached parsedSelectionSet instead of reparsing
on every conflict check.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go`:
- Around line 747-755: The raw argument-copy branch in the GraphQL datasource
path is adding arguments via ImportArguments and AddArgumentToField without
bringing over matching variable definitions, which can leave dangling $variable
references in the upstream operation. Update this branch in the field-argument
handling logic to also import any referenced variable definitions into the
upstream operation before or while copying the arguments, using the same
variable-import behavior as the configured-argument paths. Also validate the
copied arguments against the upstream field before adding them so only arguments
valid for that field are forwarded.

In `@v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go`:
- Around line 575-581: The fragment-provides lookup in
datasource_filter_collect_nodes_visitor.go should handle interface
type-condition fragments before falling back to the object-only check. Update
the logic around the `onInterfaceFragment` handling in the visitor so
`providedFieldKey(...)` is consulted for interface fragments as well, instead of
returning false when `info.enclosingTypeDefinition.Kind` is not an object. Keep
the existing `onUnionFragment` path and reuse the same fragment-stripped lookup
flow for `onInterfaceFragment` to ensure `SomeInterface` fragments resolve
correctly.

In `@v2/pkg/engine/plan/datasource_filter_visitor.go`:
- Around line 223-230: In the abstract-field member merge logic inside the
datasource filter visitor, the cache for item.Path is currently choosing
whichever requestedMembers slice is longer, which can lose union members from
separate selections. Update the merge behavior to union members for the same
path by appending only unique entries from requestedMembers into existingMembers
in the abstractFieldRequestedMembers handling, rather than comparing slice
lengths; this will keep missingMembers computation complete.
- Around line 823-829: The fallback-path selection in the selectedParentHashes
loop can assign a path from hasPathBetweenDs directly to currentNode.requiresKey
without rechecking whether the source can provide the missing fallback key
field. Update the path acceptance logic in datasource_filter_visitor.go so that
when allowFallbackKeyJumps is true, any fallback path is only accepted after
validating sourceConnectionRequiresMissingFallbackKeyField for currentNode and
the chosen path; otherwise keep searching or reject that path before setting
currentNode.requiresKey and currentNode.requiresFallbackKey.

In `@v2/pkg/engine/plan/path_builder.go`:
- Around line 217-223: The PathBuilder.hasProvidedSuggestionForPath check is too
broad because it treats inactive orphaned/unselected suggestions as active
provided routes. Update the matching logic to ignore suggestions that are not
part of the selected plan, so removeDuplicateLeafAbstractFieldPaths only prunes
paths when a provided suggestion is actually active. Use the existing
PathBuilder, hasProvidedSuggestionForPath, and visitor.nodeSuggestions.items
flow to narrow the condition to selected/active suggestions only.

---

Nitpick comments:
In `@v2/pkg/engine/plan/federation_metadata.go`:
- Around line 199-235: The parsing cache is being lost because
requiredFieldArgumentConflict and requiredFieldArgumentsByPath take
FederationFieldConfiguration by value, so parseSelectionSet() memoizes only on a
copy. Update these helpers to work with pointers and call them from
HasArgumentConflictWith using the addressable loop elements (for example, the
configs accessed in the i/j loop) so parseSelectionSet() can reuse the cached
parsedSelectionSet instead of reparsing on every conflict check.

🪄 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: ac1f5c79-91f9-4764-bc86-168b49fd79fb

📥 Commits

Reviewing files that changed from the base of the PR and between 478f3c0 and 7f2e9a9.

📒 Files selected for processing (19)

• v2/pkg/engine/datasource/graphql_datasource/graphql_datasource.go
• v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_interface_provides_test.go
• v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_provides_test.go
• v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_requires_arguments_test.go
• v2/pkg/engine/datasource/graphql_datasource/graphql_datasource_federation_test.go
• v2/pkg/engine/datasource/graphql_datasource/multihop_compound_key_test.go
• v2/pkg/engine/plan/abstract_selection_rewriter.go
• v2/pkg/engine/plan/datasource_filter_collect_nodes_visitor.go
• v2/pkg/engine/plan/datasource_filter_node_suggestions.go
• v2/pkg/engine/plan/datasource_filter_resolvable_visitor.go
• v2/pkg/engine/plan/datasource_filter_visitor.go
• v2/pkg/engine/plan/federation_metadata.go
• v2/pkg/engine/plan/multihop_compound_key_test.go
• v2/pkg/engine/plan/node_selection_builder.go
• v2/pkg/engine/plan/node_selection_visitor.go
• v2/pkg/engine/plan/path_builder.go
• v2/pkg/engine/plan/path_builder_visitor.go
• v2/pkg/engine/plan/source_connection_graph.go
• v2/pkg/engine/plan/source_connection_graph_test.go