Merged
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:
WalkthroughThis PR refactors composition and federation parameter passing by introducing a unified Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Rationale: Large-scope refactoring affecting 200+ files across composition, controlplane, and CLI with heterogeneous changes: (1) significant parameter-passing restructuring requiring verification across federation/normalization call chains; (2) high-volume repetitive type-only import changes across 110+ files with lower complexity but requiring spot-checking; (3) feature flag integration across 15+ service handlers; (4) test infrastructure overhaul affecting 60+ test files. Estimated breakdown: ~30 min for parameter threading logic verification, ~20 min for feature flag correctness in controlplane, ~15 min for test coverage validation, ~10 min for edge cases and integration points. Possibly related PRs
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2595 +/- ##
===========================================
- Coverage 62.37% 44.27% -18.11%
===========================================
Files 244 1042 +798
Lines 25775 146582 +120807
Branches 0 9455 +9455
===========================================
+ Hits 16078 64900 +48822
- Misses 8322 79951 +71629
- Partials 1375 1731 +356
🚀 New features to boost your workflow:
|
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
![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: 2
🧹 Nitpick comments (8)
controlplane/src/core/bufservices/contract/updateContract.ts (1)
143-146: DeduplicatecompositionOptionsconstruction to prevent drift.The same options object is built twice. A single local constant keeps future flag additions (like
ignoreExternalKeys) consistent.♻️ Suggested diff
+ const compositionOptions = { + disableResolvabilityValidation: req.disableResolvabilityValidation, + // ignoreExternalKeys: req.ignoreExternalKeys, + }; + await fedGraphRepo.update({ targetId: graph.targetId, @@ - compositionOptions: { - // `@TODO` ignoreExternalKeys: ?, - disableResolvabilityValidation: req.disableResolvabilityValidation, - }, + compositionOptions, }); @@ const composition = await fedGraphRepo.composeAndDeployGraphs({ @@ - compositionOptions: { - // `@TODO` ignoreExternalKeys: ?, - disableResolvabilityValidation: req.disableResolvabilityValidation, - }, + compositionOptions, federatedGraphs: [Also applies to: 161-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/contract/updateContract.ts` around lines 143 - 146, The code builds the same compositionOptions object twice (used in updateContract) which can drift — create a single local constant named compositionOptions (containing disableResolvabilityValidation and any future flags like ignoreExternalKeys) near the top of the function and reuse that constant wherever compositionOptions is currently constructed (the two duplicate sites), replacing the duplicate inline objects with references to the new constant so flag additions stay consistent.controlplane/src/core/bufservices/feature-flag/createFeatureFlag.ts (1)
208-211: Consider centralizing composition-options construction.Line 208 repeats the same inline options shape used in multiple handlers (including the same
ignoreExternalKeysTODO), which can drift over time.♻️ Suggested direction
- compositionOptions: { - // `@TODO` ignoreExternalKeys: ?, - disableResolvabilityValidation: req.disableResolvabilityValidation, - }, + compositionOptions: buildCompositionOptions(req),// e.g. shared helper colocated with request mappers function buildCompositionOptions( req: { disableResolvabilityValidation: boolean }, ) { return { // TODO: wire ignoreExternalKeys once exposed in request contract disableResolvabilityValidation: req.disableResolvabilityValidation, }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/feature-flag/createFeatureFlag.ts` around lines 208 - 211, The inline compositionOptions object is duplicated across handlers; extract construction into a single helper (e.g., buildCompositionOptions) and replace inline uses (including the object passed as compositionOptions in createFeatureFlag) with a call like buildCompositionOptions(req). The helper should accept the request shape (at minimum req.disableResolvabilityValidation), return the options object and include the existing ignoreExternalKeys TODO comment so it’s centralized for future wiring. Update all other handlers that create the same inline shape to use this helper to avoid drift.composition/src/v1/normalization/params.ts (1)
4-7: Useimport typeand canonical GraphQL entrypoint.All four imports on lines 4–7 are used exclusively in type annotations and should be declared as type-only imports. Additionally,
DocumentNodeshould be imported from the canonical'graphql'entrypoint rather than'graphql/index'.♻️ Suggested import adjustment
-import { Subgraph } from '../../subgraph/types'; -import { CompositionOptions } from '../../types/params'; -import { Graph } from '../../resolvability-graph/graph'; -import { DocumentNode } from 'graphql/index'; +import type { Subgraph } from '../../subgraph/types'; +import type { CompositionOptions } from '../../types/params'; +import type { Graph } from '../../resolvability-graph/graph'; +import type { DocumentNode } from 'graphql';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/params.ts` around lines 4 - 7, Change the four type-only imports to use type-only import syntax: import type { Subgraph } from '...'; import type { CompositionOptions } from '...'; import type { Graph } from '...'; and import type { DocumentNode } from 'graphql' (use the canonical 'graphql' entrypoint instead of 'graphql/index'); update the import statements that reference Subgraph, CompositionOptions, Graph, and DocumentNode in composition/src/v1/normalization/params.ts accordingly.composition/src/federation/types.ts (1)
72-76: Avoid dual config inputs inFederateSubgraphsParams.Keeping both
disableResolvabilityValidationandoptionson the same params type allows conflicting values and unclear precedence. Prefer a single source of truth or an exclusive union.♻️ Suggested type-shape cleanup
export type FederateSubgraphsParams = { subgraphs: Array<Subgraph>; - disableResolvabilityValidation?: boolean; - options?: CompositionOptions; + options?: CompositionOptions; version?: SupportedRouterCompatibilityVersion; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/federation/types.ts` around lines 72 - 76, FederateSubgraphsParams currently allows both disableResolvabilityValidation and options (CompositionOptions) which can conflict; change the type to use a single source of truth by replacing the current shape with an exclusive union or by removing the standalone flag—e.g., keep FederateSubgraphsParams as either { subgraphs: Array<Subgraph>; options?: CompositionOptions; version?: SupportedRouterCompatibilityVersion } and remove disableResolvabilityValidation, or define FederateSubgraphsParams as a union like { subgraphs: Array<Subgraph>; options?: CompositionOptions & { disableResolvabilityValidation?: never }; version?: SupportedRouterCompatibilityVersion } | { subgraphs: Array<Subgraph>; disableResolvabilityValidation?: boolean; options?: never; version?: SupportedRouterCompatibilityVersion } so callers must supply one source of config, and update all call sites that construct FederateSubgraphsParams accordingly (look for usages of FederateSubgraphsParams, disableResolvabilityValidation and CompositionOptions).composition/tests/v1/router-configuration.test.ts (1)
678-678: Keep router compatibility version explicit in this test.Relying on the default version can make this assertion drift when defaults change; keeping it explicit makes intent stable.
🧪 Suggested test hardening
- const result = batchNormalize({ subgraphs: [monolith, reviews, users] }) as BatchNormalizationSuccess; + const result = batchNormalize({ + subgraphs: [monolith, reviews, users], + version: ROUTER_COMPATIBILITY_VERSION_ONE, + }) as BatchNormalizationSuccess;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/tests/v1/router-configuration.test.ts` at line 678, The test relies on batchNormalize(...) but doesn't pin the router compatibility version; update the batchNormalize call that produces result (the invocation using batchNormalize and its expected type BatchNormalizationSuccess) to pass an explicit options object that sets the router compatibility/version (e.g., include a routerConfig or routerCompositionCompatibilityVersion field with the intended version string) so the test no longer depends on the default.composition/tests/v1/directives/directives.test.ts (1)
108-111: Drop the redundantNormalizationFailurecast.
normalizeSubgraphFailure(...)already returnsNormalizationFailure, so the cast adds noise.♻️ Suggested cleanup
- const { errors, warnings } = normalizeSubgraphFailure( - nf, - ROUTER_COMPATIBILITY_VERSION_ONE, - ) as NormalizationFailure; + const { errors, warnings } = normalizeSubgraphFailure( + nf, + ROUTER_COMPATIBILITY_VERSION_ONE, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/tests/v1/directives/directives.test.ts` around lines 108 - 111, The test currently casts the result of normalizeSubgraphFailure(...) to NormalizationFailure unnecessarily; remove the redundant cast and use const { errors, warnings } = normalizeSubgraphFailure(nf, ROUTER_COMPATIBILITY_VERSION_ONE) to rely on the function's return type directly (update any affected imports or type annotations if needed).controlplane/src/core/bufservices/federated-graph/checkFederatedGraph.ts (1)
111-114: Resolve or track the inline TODO explicitly.Leaving
ignoreExternalKeysas an inline TODO in this hot path makes the migration easy to forget. Prefer either wiring it now (when request shape supports it) or removing the TODO and tracking it in a dedicated issue.I can draft the follow-up issue template (scope, protobuf/API changes, and test updates) if you want.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/federated-graph/checkFederatedGraph.ts` around lines 111 - 114, The inline TODO for ignoreExternalKeys in the options object inside checkFederatedGraph must be resolved: if the request type already exposes this flag, wire it by adding ignoreExternalKeys: req.ignoreExternalKeys to the options object alongside disableResolvabilityValidation; if the request does not yet support it, remove the inline TODO and add a tracked follow-up (create an issue and reference its ID in the code comment) describing required protobuf/API changes and tests so it doesn't get forgotten. Ensure you update the call site that assembles these options (the object containing disableResolvabilityValidation) and any relevant request type handling to reflect the chosen approach.composition/src/v1/federation/federation-factory.ts (1)
2877-2877: Optional chaining is redundant here.Since
this.optionsis initialized withoptions ?? {}in the constructor (line 313), it can never be nullish. The?.operator is unnecessary, though functionally harmless.♻️ Suggested simplification
- if (!this.options?.disableResolvabilityValidation && this.internalSubgraphBySubgraphName.size > 1) { + if (!this.options.disableResolvabilityValidation && this.internalSubgraphBySubgraphName.size > 1) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/federation/federation-factory.ts` at line 2877, The optional chaining on this.options is unnecessary because the constructor initializes this.options = options ?? {}, so change the condition in the if that checks disableResolvabilityValidation to use direct property access (this.options.disableResolvabilityValidation) instead of this.options?.disableResolvabilityValidation; keep the rest of the condition (this.internalSubgraphBySubgraphName.size > 1) unchanged and ensure no runtime-null checks are added elsewhere for this.options.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composition/tests/v1/directives/directives.test.ts`:
- Line 162: The test title is misleading: the test declared in the test(...)
call reads "that a float can be coerced into a list of Int type" but the
assertions validate Float behavior; update the test title string in the test
declaration to correctly reference Float (e.g., change "Int" to "Float") so the
description matches the validation inside the test function (locate the
test(...) with that title in directives.test.ts and edit the title accordingly).
In `@controlplane/src/core/repositories/SubgraphRepository.ts`:
- Line 18: Change the runtime import of CompositionOptions to a type-only import
to avoid pulling it into emitted JS: replace the current import of
CompositionOptions with a type import (i.e., use "import type {
CompositionOptions } from '@wundergraph/composition'") and keep all usages in
SubgraphRepository (references at the CompositionOptions parameter/annotation
sites around the functions/lines where CompositionOptions is used) as type-only
annotations so the symbol is erased from runtime output.
---
Nitpick comments:
In `@composition/src/federation/types.ts`:
- Around line 72-76: FederateSubgraphsParams currently allows both
disableResolvabilityValidation and options (CompositionOptions) which can
conflict; change the type to use a single source of truth by replacing the
current shape with an exclusive union or by removing the standalone flag—e.g.,
keep FederateSubgraphsParams as either { subgraphs: Array<Subgraph>; options?:
CompositionOptions; version?: SupportedRouterCompatibilityVersion } and remove
disableResolvabilityValidation, or define FederateSubgraphsParams as a union
like { subgraphs: Array<Subgraph>; options?: CompositionOptions & {
disableResolvabilityValidation?: never }; version?:
SupportedRouterCompatibilityVersion } | { subgraphs: Array<Subgraph>;
disableResolvabilityValidation?: boolean; options?: never; version?:
SupportedRouterCompatibilityVersion } so callers must supply one source of
config, and update all call sites that construct FederateSubgraphsParams
accordingly (look for usages of FederateSubgraphsParams,
disableResolvabilityValidation and CompositionOptions).
In `@composition/src/v1/federation/federation-factory.ts`:
- Line 2877: The optional chaining on this.options is unnecessary because the
constructor initializes this.options = options ?? {}, so change the condition in
the if that checks disableResolvabilityValidation to use direct property access
(this.options.disableResolvabilityValidation) instead of
this.options?.disableResolvabilityValidation; keep the rest of the condition
(this.internalSubgraphBySubgraphName.size > 1) unchanged and ensure no
runtime-null checks are added elsewhere for this.options.
In `@composition/src/v1/normalization/params.ts`:
- Around line 4-7: Change the four type-only imports to use type-only import
syntax: import type { Subgraph } from '...'; import type { CompositionOptions }
from '...'; import type { Graph } from '...'; and import type { DocumentNode }
from 'graphql' (use the canonical 'graphql' entrypoint instead of
'graphql/index'); update the import statements that reference Subgraph,
CompositionOptions, Graph, and DocumentNode in
composition/src/v1/normalization/params.ts accordingly.
In `@composition/tests/v1/directives/directives.test.ts`:
- Around line 108-111: The test currently casts the result of
normalizeSubgraphFailure(...) to NormalizationFailure unnecessarily; remove the
redundant cast and use const { errors, warnings } = normalizeSubgraphFailure(nf,
ROUTER_COMPATIBILITY_VERSION_ONE) to rely on the function's return type directly
(update any affected imports or type annotations if needed).
In `@composition/tests/v1/router-configuration.test.ts`:
- Line 678: The test relies on batchNormalize(...) but doesn't pin the router
compatibility version; update the batchNormalize call that produces result (the
invocation using batchNormalize and its expected type BatchNormalizationSuccess)
to pass an explicit options object that sets the router compatibility/version
(e.g., include a routerConfig or routerCompositionCompatibilityVersion field
with the intended version string) so the test no longer depends on the default.
In `@controlplane/src/core/bufservices/contract/updateContract.ts`:
- Around line 143-146: The code builds the same compositionOptions object twice
(used in updateContract) which can drift — create a single local constant named
compositionOptions (containing disableResolvabilityValidation and any future
flags like ignoreExternalKeys) near the top of the function and reuse that
constant wherever compositionOptions is currently constructed (the two duplicate
sites), replacing the duplicate inline objects with references to the new
constant so flag additions stay consistent.
In `@controlplane/src/core/bufservices/feature-flag/createFeatureFlag.ts`:
- Around line 208-211: The inline compositionOptions object is duplicated across
handlers; extract construction into a single helper (e.g.,
buildCompositionOptions) and replace inline uses (including the object passed as
compositionOptions in createFeatureFlag) with a call like
buildCompositionOptions(req). The helper should accept the request shape (at
minimum req.disableResolvabilityValidation), return the options object and
include the existing ignoreExternalKeys TODO comment so it’s centralized for
future wiring. Update all other handlers that create the same inline shape to
use this helper to avoid drift.
In `@controlplane/src/core/bufservices/federated-graph/checkFederatedGraph.ts`:
- Around line 111-114: The inline TODO for ignoreExternalKeys in the options
object inside checkFederatedGraph must be resolved: if the request type already
exposes this flag, wire it by adding ignoreExternalKeys: req.ignoreExternalKeys
to the options object alongside disableResolvabilityValidation; if the request
does not yet support it, remove the inline TODO and add a tracked follow-up
(create an issue and reference its ID in the code comment) describing required
protobuf/API changes and tests so it doesn't get forgotten. Ensure you update
the call site that assembles these options (the object containing
disableResolvabilityValidation) and any relevant request type handling to
reflect the chosen approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30bddf1a-21a1-405d-90f1-94218d31fcce
📒 Files selected for processing (44)
cli/src/commands/router/commands/compose.tscli/src/utils.tscomposition-go/index.global.jscomposition/src/federation/federation.tscomposition/src/federation/types.tscomposition/src/index.tscomposition/src/normalization/normalization.tscomposition/src/normalization/params.tscomposition/src/types/params.tscomposition/src/v1/federation/federation-factory.tscomposition/src/v1/federation/params.tscomposition/src/v1/federation/types.tscomposition/src/v1/federation/utils.tscomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/normalization/params.tscomposition/src/v1/normalization/utils.tscomposition/tests/utils/utils.tscomposition/tests/v1/directives/directives.test.tscomposition/tests/v1/directives/fieldset-directives.test.tscomposition/tests/v1/directives/override.test.tscomposition/tests/v1/normalization.test.tscomposition/tests/v1/resolvability.test.tscomposition/tests/v1/router-configuration.test.tscontrolplane/src/core/bufservices/contract/createContract.tscontrolplane/src/core/bufservices/contract/updateContract.tscontrolplane/src/core/bufservices/feature-flag/createFeatureFlag.tscontrolplane/src/core/bufservices/feature-flag/deleteFeatureFlag.tscontrolplane/src/core/bufservices/feature-flag/enableFeatureFlag.tscontrolplane/src/core/bufservices/feature-flag/updateFeatureFlag.tscontrolplane/src/core/bufservices/federated-graph/checkFederatedGraph.tscontrolplane/src/core/bufservices/federated-graph/createFederatedGraph.tscontrolplane/src/core/bufservices/federated-graph/updateFederatedGraph.tscontrolplane/src/core/bufservices/graph/setGraphRouterCompatibilityVersion.tscontrolplane/src/core/bufservices/subgraph/deleteFederatedSubgraph.tscontrolplane/src/core/bufservices/subgraph/fixSubgraphSchema.tscontrolplane/src/core/bufservices/subgraph/moveSubgraph.tscontrolplane/src/core/bufservices/subgraph/publishFederatedSubgraph.tscontrolplane/src/core/bufservices/subgraph/updateSubgraph.tscontrolplane/src/core/composition/composer.tscontrolplane/src/core/composition/composition.tscontrolplane/src/core/repositories/FederatedGraphRepository.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/util.tscontrolplane/src/types/index.ts
💤 Files with no reviewable changes (2)
- controlplane/src/types/index.ts
- composition/src/v1/federation/types.ts
Aenimus
commented
Mar 6, 2026
Aenimus
commented
Mar 6, 2026
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Line 433: NormalizationFactory declares and assigns the options field
(this.options / CompositionOptions) in the constructor but never reads it;
either remove the unused property and any constructor parameter/storage of
CompositionOptions from NormalizationFactory, or if it’s intended for future use
add a clear explanatory comment above the field and constructor parameter
stating its purpose (mirroring how FederationFactory uses this.options, e.g.,
this.options?.disableResolvabilityValidation) so the intent is explicit; update
the NormalizationFactory constructor and class fields accordingly to avoid the
unused-field lint warning.
In `@controlplane/emails/package.json`:
- Line 11: The package.json script "lint:fix" currently runs only "pnpm format"
which is misleading; either rename the script to "format" or make it actually
perform linting like other packages. Fix option A: rename the script key from
"lint:fix" to "format" and update any callers to use "format". Fix option B:
install/configure ESLint for the emails package and change the "lint:fix" script
to run both formatter and ESLint auto-fix (e.g., run the formatter and then
"eslint --fix" against the TypeScript files) so the "lint:fix" name matches
behavior; update any parent invocation if necessary. Ensure you update
package.json's scripts entry containing "lint:fix" accordingly and keep naming
consistent with other packages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56446123-ef31-416d-86d3-fefc70fc1e8e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
cli/src/commands/router/commands/compose.tscli/vite.config.tscomposition-go/index.global.jscomposition/src/v1/normalization/normalization-factory.tscontrolplane/emails/package.jsoncontrolplane/package.jsoncontrolplane/src/core/util.tscontrolplane/tsconfig.json
✅ Files skipped from review due to trivial changes (1)
- controlplane/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/src/commands/router/commands/compose.ts
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
composition/src/v1/federation/federation-factory.ts (1)
2877-2877: Add direct regression tests fordisableResolvabilityValidationon each public path.Line 2877 is now option-gated; consider dedicated tests for
federateSubgraphs,federateSubgraphsWithContracts, andfederateSubgraphsContractto prevent future wiring regressions.Also applies to: 3375-3427
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/federation/federation-factory.ts` at line 2877, Add direct regression tests to cover the option-gated path where disableResolvabilityValidation is used: create unit tests that call federateSubgraphs, federateSubgraphsWithContracts, and federateSubgraphsContract with a setup of multiple subgraphs (so internalSubgraphBySubgraphName.size > 1) and assert behavior when options.disableResolvabilityValidation is true vs false. Ensure each public function (federateSubgraphs, federateSubgraphsWithContracts, federateSubgraphsContract) is exercised independently so the conditional at the start of the block referencing this.options.disableResolvabilityValidation is hit, and verify that resolvability validation is skipped when true and performed when false to catch future wiring regressions.composition/src/federation/params.ts (1)
1-5: Convert value imports toimport typefor type-only symbols.Lines 1, 3, and Line 5 import type-only symbols as value imports. Although the project does not currently enforce strict type-only import settings, using
import typeis a best practice that clarifies intent and improves code maintainability.♻️ Proposed change
-import { Subgraph } from '../subgraph/types'; +import type { Subgraph } from '../subgraph/types'; import type { CompositionOptions } from '../types/params'; -import { SupportedRouterCompatibilityVersion } from '../router-compatibility-version/router-compatibility-version'; +import type { SupportedRouterCompatibilityVersion } from '../router-compatibility-version/router-compatibility-version'; import type { ContractName } from '../types/types'; -import { ContractTagOptions } from './types'; +import type { ContractTagOptions } from './types';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/federation/params.ts` around lines 1 - 5, The file imports Subgraph, SupportedRouterCompatibilityVersion, and ContractTagOptions as value imports but they are type-only; change their imports to use "import type" (i.e., replace "import { Subgraph } from '../subgraph/types';", "import { SupportedRouterCompatibilityVersion } from '../router-compatibility-version/router-compatibility-version';", and "import { ContractTagOptions } from './types';" with type-only imports) so the intent is explicit and tree-shaking/type-only checks are clearer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@composition/src/federation/params.ts`:
- Around line 1-5: The file imports Subgraph,
SupportedRouterCompatibilityVersion, and ContractTagOptions as value imports but
they are type-only; change their imports to use "import type" (i.e., replace
"import { Subgraph } from '../subgraph/types';", "import {
SupportedRouterCompatibilityVersion } from
'../router-compatibility-version/router-compatibility-version';", and "import {
ContractTagOptions } from './types';" with type-only imports) so the intent is
explicit and tree-shaking/type-only checks are clearer.
In `@composition/src/v1/federation/federation-factory.ts`:
- Line 2877: Add direct regression tests to cover the option-gated path where
disableResolvabilityValidation is used: create unit tests that call
federateSubgraphs, federateSubgraphsWithContracts, and federateSubgraphsContract
with a setup of multiple subgraphs (so internalSubgraphBySubgraphName.size > 1)
and assert behavior when options.disableResolvabilityValidation is true vs
false. Ensure each public function (federateSubgraphs,
federateSubgraphsWithContracts, federateSubgraphsContract) is exercised
independently so the conditional at the start of the block referencing
this.options.disableResolvabilityValidation is hit, and verify that
resolvability validation is skipped when true and performed when false to catch
future wiring regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bdb523ad-edb3-413a-92b6-da8e0c44e50c
📒 Files selected for processing (13)
composition-go/index.global.jscomposition/src/federation/federation.tscomposition/src/federation/params.tscomposition/src/federation/types.tscomposition/src/index.tscomposition/src/normalization/params.tscomposition/src/v1/federation/federation-factory.tscomposition/src/v1/federation/params.tscomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/normalization/params.tscomposition/tests/v1/directives/directives.test.tscomposition/tests/v1/directives/fieldset-directives.test.tscomposition/tsconfig.json
✅ Files skipped from review due to trivial changes (1)
- composition/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (2)
- composition/src/normalization/params.ts
- composition/src/federation/federation.ts
This comment has been minimized.
This comment has been minimized.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
composition/src/v1/normalization/params.ts (1)
45-49: Consider makingnoLocationoptional for consistency.Line 46 currently requires
noLocation, which is stricter than the adjacent normalization params surface and can create avoidable call-site friction.♻️ Suggested adjustment
export type NormalizeSubgraphFromStringParams = { - noLocation: boolean; + noLocation?: boolean; sdlString: string; options?: CompositionOptions; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/params.ts` around lines 45 - 49, The type NormalizeSubgraphFromStringParams currently requires noLocation which is inconsistent with nearby param types and causes unnecessary friction; update the type declaration for NormalizeSubgraphFromStringParams so that noLocation is optional (change noLocation to noLocation?: boolean) and then search for usages of NormalizeSubgraphFromStringParams/any calls that construct this object to ensure they handle an undefined noLocation (add defaulting where necessary, e.g., treat undefined as false) so callers no longer must pass the flag.composition/src/v1/normalization/utils.ts (1)
237-237: Add a focused regression test forignoreExternalKeys.Line 237 changes external-key classification semantics; please ensure there is an explicit test for both
ignoreExternalKeys: trueandfalseon this path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@composition/src/v1/normalization/utils.ts` at line 237, Add focused regression tests covering the conditional at the path where conditionalData is checked against nf.options.ignoreExternalKeys: create two tests that exercise the code path in composition/src/v1/normalization/utils.ts where "if (!conditionalData && !nf.options.ignoreExternalKeys)" is evaluated — one with nf.options.ignoreExternalKeys set to true and one set to false — asserting that external-key classification behavior matches the intended semantics (external keys ignored when true, classified when false); locate the normalization helper that references conditionalData and nf (search for nf.options.ignoreExternalKeys or the surrounding normalize/classify function) and call it with inputs that produce conditionalData = falsy to explicitly cover the changed branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@composition/package.json`:
- Line 23: The "lint:fix" npm script in composition's package.json references
eslint but the package doesn't declare it; either add eslint to composition's
devDependencies (install a matching eslint version) so the "lint:fix" script can
call eslint directly, or update the "lint:fix" script to use pnpm exec (e.g.
change the command to use pnpm exec eslint ...) so eslint is resolved from the
workspace, and ensure the script name "lint:fix" in package.json is updated
accordingly.
In `@composition/src/v1/normalization/normalization-factory.ts`:
- Around line 3909-3914: The call to normalizeSubgraph(...) passes subgraph.name
which is undefined for unnamed entries; change it to pass the previously
computed per-subgraph fallback name (the variable assigned just above the call)
so normalizeSubgraph receives the actual fallback identifier instead of
undefined; update the argument for subgraphName in the normalizeSubgraph
invocation to use that computed fallback variable rather than subgraph.name.
---
Nitpick comments:
In `@composition/src/v1/normalization/params.ts`:
- Around line 45-49: The type NormalizeSubgraphFromStringParams currently
requires noLocation which is inconsistent with nearby param types and causes
unnecessary friction; update the type declaration for
NormalizeSubgraphFromStringParams so that noLocation is optional (change
noLocation to noLocation?: boolean) and then search for usages of
NormalizeSubgraphFromStringParams/any calls that construct this object to ensure
they handle an undefined noLocation (add defaulting where necessary, e.g., treat
undefined as false) so callers no longer must pass the flag.
In `@composition/src/v1/normalization/utils.ts`:
- Line 237: Add focused regression tests covering the conditional at the path
where conditionalData is checked against nf.options.ignoreExternalKeys: create
two tests that exercise the code path in
composition/src/v1/normalization/utils.ts where "if (!conditionalData &&
!nf.options.ignoreExternalKeys)" is evaluated — one with
nf.options.ignoreExternalKeys set to true and one set to false — asserting that
external-key classification behavior matches the intended semantics (external
keys ignored when true, classified when false); locate the normalization helper
that references conditionalData and nf (search for nf.options.ignoreExternalKeys
or the surrounding normalize/classify function) and call it with inputs that
produce conditionalData = falsy to explicitly cover the changed branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d52f911f-2485-4816-8638-481e18ec2d0a
📒 Files selected for processing (86)
composition-go/index.global.jscomposition/.eslintrc.jsoncomposition/package.jsoncomposition/src/ast/utils.tscomposition/src/buildASTSchema/buildASTSchema.tscomposition/src/buildASTSchema/extendSchema.tscomposition/src/errors/errors.tscomposition/src/errors/types.tscomposition/src/federation/federation.tscomposition/src/federation/params.tscomposition/src/federation/types.tscomposition/src/normalization/normalization.tscomposition/src/normalization/types.tscomposition/src/resolvability-graph/graph-nodes.tscomposition/src/resolvability-graph/graph.tscomposition/src/resolvability-graph/node-resolution-data/node-resolution-data.tscomposition/src/resolvability-graph/node-resolution-data/types/params.tscomposition/src/resolvability-graph/types/params.tscomposition/src/resolvability-graph/utils/types/params.tscomposition/src/resolvability-graph/utils/types/types.tscomposition/src/resolvability-graph/utils/utils.tscomposition/src/resolvability-graph/walker/entity-walker/entity-walker.tscomposition/src/resolvability-graph/walker/entity-walker/types/params.tscomposition/src/resolvability-graph/walker/root-field-walkers/root-field-walker.tscomposition/src/resolvability-graph/walker/root-field-walkers/types/params.tscomposition/src/router-configuration/types.tscomposition/src/router-configuration/utils.tscomposition/src/schema-building/ast.tscomposition/src/schema-building/types.tscomposition/src/schema-building/utils.tscomposition/src/subgraph/types.tscomposition/src/utils/string-constants.tscomposition/src/utils/types.tscomposition/src/utils/utils.tscomposition/src/v1/constants/constants.tscomposition/src/v1/constants/directive-definitions.tscomposition/src/v1/constants/non-directive-definitions.tscomposition/src/v1/constants/strings.tscomposition/src/v1/constants/type-nodes.tscomposition/src/v1/federation/federation-factory.tscomposition/src/v1/federation/utils.tscomposition/src/v1/federation/walkers.tscomposition/src/v1/normalization/directive-definition-data.tscomposition/src/v1/normalization/normalization-factory.tscomposition/src/v1/normalization/params.tscomposition/src/v1/normalization/types.tscomposition/src/v1/normalization/utils.tscomposition/src/v1/normalization/walkers.tscomposition/src/v1/schema-building/type-merging.tscomposition/src/v1/utils/utils.tscomposition/src/v1/warnings/params.tscomposition/src/v1/warnings/warnings.tscomposition/tests/utils/utils.tscomposition/tests/v1/contracts.test.tscomposition/tests/v1/directives/authorization-directives.test.tscomposition/tests/v1/directives/configure-description.test.tscomposition/tests/v1/directives/connect-configure-resolver.test.tscomposition/tests/v1/directives/directives.test.tscomposition/tests/v1/directives/external.test.tscomposition/tests/v1/directives/fieldset-directives.test.tscomposition/tests/v1/directives/inaccessible.test.tscomposition/tests/v1/directives/interface-object.test.tscomposition/tests/v1/directives/one-of.test.tscomposition/tests/v1/directives/override.test.tscomposition/tests/v1/directives/provides.test.tscomposition/tests/v1/directives/require-fetch-reasons.test.tscomposition/tests/v1/directives/semantic-non-null.test.tscomposition/tests/v1/directives/shareable.test.tscomposition/tests/v1/directives/subscription-filter.test.tscomposition/tests/v1/directives/v2-directives.test.tscomposition/tests/v1/entities.test.tscomposition/tests/v1/entity-interface.test.tscomposition/tests/v1/events.test.tscomposition/tests/v1/federation-factory.test.tscomposition/tests/v1/normalization.test.tscomposition/tests/v1/resolvability.test.tscomposition/tests/v1/router-configuration.test.tscomposition/tests/v1/type-merging.test.tscomposition/tests/v1/types/arguments.test.tscomposition/tests/v1/types/enums.test.tscomposition/tests/v1/types/inputs.test.tscomposition/tests/v1/types/interfaces.test.tscomposition/tests/v1/types/objects.test.tscomposition/tests/v1/types/scalars.test.tscomposition/tests/v1/types/unions.test.tscomposition/tests/v1/utils/utils.ts
✅ Files skipped from review due to trivial changes (8)
- composition/src/v1/constants/directive-definitions.ts
- composition/tests/v1/entity-interface.test.ts
- composition/src/v1/schema-building/type-merging.ts
- composition/tests/v1/types/objects.test.ts
- composition/src/resolvability-graph/utils/utils.ts
- composition/tests/v1/directives/configure-description.test.ts
- composition/src/schema-building/types.ts
- composition/tests/v1/directives/subscription-filter.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- composition/src/federation/federation.ts
- composition/tests/v1/directives/override.test.ts
- composition/tests/v1/router-configuration.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controlplane/src/core/bufservices/graph/setGraphRouterCompatibilityVersion.ts (1)
121-185:⚠️ Potential issue | 🔴 CriticalPropagate failure out of the transaction instead of always returning OK.
The error objects returned on Line 159 and Line 172 never leave the transaction callback, so this handler still falls through to the success response on Line 185. Since the version update happens on Line 124 before those checks, a failed composition/deployment can also leave the new compatibility version persisted unless the transaction is explicitly aborted.
💡 One way to fix this
- await opts.db.transaction(async (tx) => { + const txResult = await opts.db.transaction(async (tx) => { const fedGraphRepo = new FederatedGraphRepository(logger, tx, authContext.organizationId); - - await fedGraphRepo.updateRouterCompatibilityVersion(federatedGraph.id, version); - - const auditLogRepo = new AuditLogRepository(opts.db); - - await auditLogRepo.addAuditLog({ - organizationId: authContext.organizationId, - organizationSlug: authContext.organizationSlug, - auditAction: `${federatedGraph.supportsFederation ? 'federated_graph' : 'monograph'}.updated`, - action: 'updated', - actorId: authContext.userId, - auditableType: `${federatedGraph.supportsFederation ? 'federated_graph' : 'monograph'}`, - auditableDisplayName: federatedGraph.name, - actorDisplayName: authContext.userDisplayName, - apiKeyName: authContext.apiKeyName, - actorType: authContext.auth === 'api_key' ? 'api_key' : 'user', - targetNamespaceId: federatedGraph.namespaceId, - targetNamespaceDisplayName: federatedGraph.namespace, - }); - const composition = await fedGraphRepo.composeAndDeployGraphs({ actorId: authContext.userId, admissionConfig: { cdnBaseUrl: opts.cdnBaseUrl, webhookJWTSecret: opts.admissionWebhookJWTSecret, @@ if (composition.compositionErrors.length > 0) { return { response: { code: EnumStatusCode.ERR_SUBGRAPH_COMPOSITION_FAILED, }, @@ if (composition.deploymentErrors.length > 0) { return { response: { code: EnumStatusCode.ERR_DEPLOYMENT_FAILED, }, @@ } + + await fedGraphRepo.updateRouterCompatibilityVersion(federatedGraph.id, version); + + const auditLogRepo = new AuditLogRepository(opts.db); + await auditLogRepo.addAuditLog({ + organizationId: authContext.organizationId, + organizationSlug: authContext.organizationSlug, + auditAction: `${federatedGraph.supportsFederation ? 'federated_graph' : 'monograph'}.updated`, + action: 'updated', + actorId: authContext.userId, + auditableType: `${federatedGraph.supportsFederation ? 'federated_graph' : 'monograph'}`, + auditableDisplayName: federatedGraph.name, + actorDisplayName: authContext.userDisplayName, + apiKeyName: authContext.apiKeyName, + actorType: authContext.auth === 'api_key' ? 'api_key' : 'user', + targetNamespaceId: federatedGraph.namespaceId, + targetNamespaceDisplayName: federatedGraph.namespace, + }); + + return null; }); + + if (txResult) { + return txResult; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/src/core/bufservices/graph/setGraphRouterCompatibilityVersion.ts` around lines 121 - 185, The transaction commits the version update before composition/deployment checks and the early "return" inside the transaction callback (after fedGraphRepo.updateRouterCompatibilityVersion and composeAndDeployGraphs) doesn't escape the transaction, so failures still commit; modify the flow so composition/deployment failures abort the transaction and propagate the error to the outer handler — e.g., when composeAndDeployGraphs returns compositionErrors or deploymentErrors, throw a specific Error (or reject) carrying the intended error response instead of returning from inside the tx callback, then catch that Error around opts.db.transaction(...) and return its payload; reference fedGraphRepo.updateRouterCompatibilityVersion, composeAndDeployGraphs, and the transaction callback to locate where to throw and where to catch.
🧹 Nitpick comments (1)
controlplane/test/integrations.test.ts (1)
324-360: This only covers greenfield enablement.Lines 324-360 republish everything after
enabledFeaturesis set, so this test will not catch rollout bugs where already-published inputs stay blocked because their cached composability was computed before the org feature changed. Please add one regression that flips the feature for existing subgraphs/graphs too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controlplane/test/integrations.test.ts` around lines 324 - 360, Test currently only validates greenfield enablement; add a regression that verifies flipping the feature for already-published inputs works. Specifically: use SetupTest without enabledFeatures to create namespace via createNamespace, publish the externalKey and keySource subgraphs with publishFederatedSubgraph and create the federated graph with createFederatedGraph, then simulate enabling the COMPOSITION_IGNORE_EXTERNAL_KEYS_FEATURE_ID (e.g., re-run or reconfigure SetupTest with enabledFeatures including that feature or restart the server/client with the flag) and re-attempt composition/republishing of the graph/subgraphs; assert response codes are OK and compositionErrors length is 0 to ensure previously-published inputs are no longer blocked (reference SetupTest, enabledFeatures, publishFederatedSubgraph, createFederatedGraph, createNamespace).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controlplane/src/core/bufservices/contract/createContract.ts`:
- Around line 108-111: The early rejection uses cached sourceGraph.isComposable
without considering the org feature ignoreExternalKeys (fetched via
orgRepo.getFeature and COMPOSITION_IGNORE_EXTERNAL_KEYS_FEATURE_ID); update
createContract to re-evaluate composability under the effective composition
options by invoking the composition routine (the same composition used later)
with ignoreExternalKeys set according to ignoreExternalKeysFeature before
returning the early error (i.e., compute a fresh composition result and check
its isComposable instead of relying solely on the stored
sourceGraph.isComposable); apply the same change to the later composition path
so both checks use the effective composition options.
---
Outside diff comments:
In
`@controlplane/src/core/bufservices/graph/setGraphRouterCompatibilityVersion.ts`:
- Around line 121-185: The transaction commits the version update before
composition/deployment checks and the early "return" inside the transaction
callback (after fedGraphRepo.updateRouterCompatibilityVersion and
composeAndDeployGraphs) doesn't escape the transaction, so failures still
commit; modify the flow so composition/deployment failures abort the transaction
and propagate the error to the outer handler — e.g., when composeAndDeployGraphs
returns compositionErrors or deploymentErrors, throw a specific Error (or
reject) carrying the intended error response instead of returning from inside
the tx callback, then catch that Error around opts.db.transaction(...) and
return its payload; reference fedGraphRepo.updateRouterCompatibilityVersion,
composeAndDeployGraphs, and the transaction callback to locate where to throw
and where to catch.
---
Nitpick comments:
In `@controlplane/test/integrations.test.ts`:
- Around line 324-360: Test currently only validates greenfield enablement; add
a regression that verifies flipping the feature for already-published inputs
works. Specifically: use SetupTest without enabledFeatures to create namespace
via createNamespace, publish the externalKey and keySource subgraphs with
publishFederatedSubgraph and create the federated graph with
createFederatedGraph, then simulate enabling the
COMPOSITION_IGNORE_EXTERNAL_KEYS_FEATURE_ID (e.g., re-run or reconfigure
SetupTest with enabledFeatures including that feature or restart the
server/client with the flag) and re-attempt composition/republishing of the
graph/subgraphs; assert response codes are OK and compositionErrors length is 0
to ensure previously-published inputs are no longer blocked (reference
SetupTest, enabledFeatures, publishFederatedSubgraph, createFederatedGraph,
createNamespace).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a396958c-81fa-4ee9-9b6e-c294be6f17e3
📒 Files selected for processing (24)
cli/src/commands/router/commands/compose.tscontrolplane/src/core/bufservices/contract/createContract.tscontrolplane/src/core/bufservices/contract/updateContract.tscontrolplane/src/core/bufservices/feature-flag/createFeatureFlag.tscontrolplane/src/core/bufservices/feature-flag/deleteFeatureFlag.tscontrolplane/src/core/bufservices/feature-flag/enableFeatureFlag.tscontrolplane/src/core/bufservices/feature-flag/updateFeatureFlag.tscontrolplane/src/core/bufservices/federated-graph/checkFederatedGraph.tscontrolplane/src/core/bufservices/federated-graph/createFederatedGraph.tscontrolplane/src/core/bufservices/federated-graph/migrateFromApollo.tscontrolplane/src/core/bufservices/federated-graph/updateFederatedGraph.tscontrolplane/src/core/bufservices/graph/setGraphRouterCompatibilityVersion.tscontrolplane/src/core/bufservices/monograph/publishMonograph.tscontrolplane/src/core/bufservices/subgraph/checkSubgraphSchema.tscontrolplane/src/core/bufservices/subgraph/deleteFederatedSubgraph.tscontrolplane/src/core/bufservices/subgraph/fixSubgraphSchema.tscontrolplane/src/core/bufservices/subgraph/moveSubgraph.tscontrolplane/src/core/bufservices/subgraph/publishFederatedSubgraph.tscontrolplane/src/core/bufservices/subgraph/updateSubgraph.tscontrolplane/src/core/repositories/OrganizationRepository.tscontrolplane/src/core/repositories/SchemaCheckRepository.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/types/index.tscontrolplane/test/integrations.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- controlplane/src/core/bufservices/feature-flag/createFeatureFlag.ts
- controlplane/src/core/bufservices/subgraph/updateSubgraph.ts
- controlplane/src/core/bufservices/subgraph/moveSubgraph.ts
- cli/src/commands/router/commands/compose.ts
- controlplane/src/core/repositories/SubgraphRepository.ts
- controlplane/src/core/bufservices/subgraph/deleteFederatedSubgraph.ts
devsergiy
approved these changes
Mar 9, 2026
wilsonrivera
reviewed
Mar 9, 2026
Aenimus
requested review from
a team,
JivusAyrus,
Noroth,
StarpTech,
endigma,
jensneuse and
thisisnithin
as code owners
JivusAyrus
approved these changes
Mar 10, 2026
5 tasks
This was referenced Mar 30, 2026
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
6 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
Release Notes
New Features
--ignore-external-keysCLI flag to allow ignoring external entity keys during composition.Bug Fixes
Checklist