feat: add rpc method to recompose feature flags - #2848
Closed
wilsonrivera wants to merge 28 commits into
Closed
Conversation
…lit config poller (#2844)
Contributor
WalkthroughAdds split-config CDN endpoints, router split-config poller with change diffs and mux reuse, centralized CompositionService for deploy/composition, feature-flag recompose RPC/CLI, schema/proto/type updates, repository/bufservice refactors, and extensive tests/docs. ChangesSplit-config loading across CDN, Router, and Controlplane
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
⚔️ Resolve merge conflicts
|
Router image scan passed✅ No security vulnerabilities found in image: |
wilsonrivera
marked this pull request as ready for review
May 8, 2026 23:48
wilsonrivera
requested review from
Aenimus,
JivusAyrus,
StarpTech,
endigma and
thisisnithin
as code owners
May 8, 2026 23:48
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
…onfigs Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
…ith transaction support Co-authored-by: Copilot <copilot@github.com>
…dd-rpc-method-to-recompose-a-specific-feature # Conflicts: # connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.go # connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.go # connect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.ts # connect/src/wg/cosmo/platform/v1/platform_connect.ts # connect/src/wg/cosmo/platform/v1/platform_pb.ts # controlplane/src/core/bufservices/PlatformService.ts # controlplane/src/core/services/CompositionService.ts # proto/wg/cosmo/platform/v1/platform.proto
wilsonrivera
requested review from
a team,
Noroth,
SkArchon,
devsergiy and
jensneuse
as code owners
May 11, 2026 17:08
![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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
router/core/router.go (1)
822-827:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate poller responses before using
response.Config.
cfg.Config/response.Configare used without nil checks. An empty/malformed poller response can break startup or hot-reload and crash the process path.💡 Suggested hardening
cfg, err := r.configPoller.GetRouterConfig(ctx) if err != nil { return nil, fmt.Errorf("failed to get initial execution config: %w", err) } +if cfg == nil || cfg.Config == nil { + return nil, errors.New("received empty execution config from config poller") +} // ... cfg, err := r.configPoller.GetRouterConfig(ctx) if err != nil { return fmt.Errorf("failed to get initial execution config: %w", err) } +if cfg == nil || cfg.Config == nil { + return errors.New("received empty execution config from config poller") +} r.configPoller.Subscribe(ctx, func(response *routerconfig.Response) error { + if response == nil || response.Config == nil { + return errors.New("received empty execution config update from config poller") + } if r.shutdown.Load() { r.logger.Warn("Router is in shutdown state. Skipping config update") return nil }Also applies to: 1567-1573, 1613-1621
🤖 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 `@router/core/router.go` around lines 822 - 827, The code uses the poller response (cfg) and its nested Config without validating them; update the startup and hot-reload call sites that call r.configPoller.GetRouterConfig(ctx) (e.g., the block that then calls r.newServer(ctx, cfg)) to check that cfg != nil and cfg.Config != nil before passing to r.newServer (and similarly where the poller response is used elsewhere), and if either is nil return a wrapped error like "invalid router config: missing response or Config" (or handle with a safe default) so the process cannot dereference a nil response.Config.controlplane/src/core/bufservices/graph/setGraphRouterCompatibilityVersion.ts (2)
115-147:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftDon’t commit the version bump before knowing recomposition succeeded.
The transaction updates
routerCompatibilityVersionand writes the audit log before composition/deployment, but failures are only returned as arrays. That means this transaction can still commit the new version while Lines 159-162 tell callers the version stayed unchanged. Please either compose against an in-memory graph snapshot first and persist on success, or explicitly abort the transaction when any composition/deployment error is produced.🤖 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 `@controlplane/src/core/bufservices/graph/setGraphRouterCompatibilityVersion.ts` around lines 115 - 147, The transaction currently calls FederatedGraphRepository.updateRouterCompatibilityVersion and AuditLogRepository.addAuditLog before running CompositionService.composeAndDeployFederatedGraph, which can commit a version bump even if composition fails; change the flow so composition succeeds before persisting the new version and audit log: either (A) run compositionService.composeAndDeployFederatedGraph outside the DB transaction against the in-memory/current federatedGraph snapshot and only open a transaction to call FederatedGraphRepository.updateRouterCompatibilityVersion(federatedGraph.id, version) and AuditLogRepository.addAuditLog when composition returns no deploymentErrors/compositionErrors, or (B) if you must run composition inside the same tx, inspect the returned deploymentErrors/compositionErrors and throw an error (or return a failure that aborts the tx) when any errors exist so the transaction rolls back instead of committing the version bump.
99-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the missing audit log on the empty-graph success path.
This branch persists a router-compatibility change and returns
OK, but it bypasses the audit log entirely. Version changes on graphs with zero subgraphs would be invisible in the audit trail.🤖 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 `@controlplane/src/core/bufservices/graph/setGraphRouterCompatibilityVersion.ts` around lines 99 - 112, The empty-subgraphs branch updates the router compatibility version but omits creating an audit entry; before returning, call the same audit logging routine used in the non-empty branch (the AuditLogRepository / audit logging method invoked elsewhere in this file) to record the change: pass federatedGraph.id, previous version federatedGraph.routerCompatibilityVersion, new version variable version (or req.version), and actor context from authContext/logger so the version change on graphs with zero subgraphs appears in the audit trail; insert this audit call right after FederatedGraphRepository.updateRouterCompatibilityVersion(...) and before the return.router/core/graph_server.go (1)
1387-1458:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReused muxes still depend on resources owned by the old
graphServer.
buildGraphMuxwires execution againsts.connector,s.pubSubProviders, ands.connectionMetrics, butShutdowntears those down even when a mux is marked reused. That means a reused mux can survive the swap with its plugin host / pubsub providers / shared metrics already stopped.Also applies to: 2041-2127
🤖 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 `@router/core/graph_server.go` around lines 1387 - 1458, Reused graph muxes keep references to s.connector, s.pubSubProviders, and s.connectionMetrics which Shutdown can tear down; update buildGraphMux and Shutdown to avoid dangling resources by transferring or isolating ownership for reused muxes: when buildGraphMux marks a mux as reused, ensure it receives independent copies or owned wrappers of s.connector, s.pubSubProviders and s.connectionMetrics (or increment a reference count) so Shutdown doesn't stop resources still used by that mux; alternatively, have shutdown check ownership/refcount before closing resources. Touch symbols: buildGraphMux, Shutdown, s.connector, s.pubSubProviders, s.connectionMetrics, startPubSubProviders, and ExecutorConfigurationBuilder/Build to implement ownership transfer or refcounting so reused muxes remain functional after a server swap.
🧹 Nitpick comments (3)
router/pkg/config/config.schema.json (1)
415-418: ⚡ Quick winTighten validation for
ignored_feature_flagsentries.Consider rejecting empty values and duplicates to prevent silent misconfiguration.
♻️ Proposed schema hardening
"ignored_feature_flags": { "type": "array", - "items": { "type": "string" }, + "uniqueItems": true, + "items": { + "type": "string", + "minLength": 1 + }, "description": "Feature flag names to skip entirely during config polling. Listed flags are not fetched even when present in the mapper." }🤖 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 `@router/pkg/config/config.schema.json` around lines 415 - 418, The ignored_feature_flags schema currently allows empty strings and duplicates; tighten validation by requiring non-empty entries and no duplicates: update the "ignored_feature_flags" entry to set "items": { "type": "string", "minLength": 1 } and add "uniqueItems": true at the array level so each flag is a non-empty string and duplicates are rejected (reference symbol: "ignored_feature_flags").docs-website/router/configuration.mdx (1)
1287-1287: ⚡ Quick winSplit the long behavior sentence into shorter declarative statements.
Line 1287 compresses multiple behaviors into one sentence, which hurts scanability in reference docs.
Proposed wording split
-The split-config polling strategy assembles the final router execution config by fetching the base graph and each feature flag config as separate files from the CDN. These rules govern its behavior when individual feature flag files are missing or should be excluded entirely. They are only applied when the router is enrolled in split-config loading; with a custom storage provider the router falls back to the regular polling strategy and these rules have no effect. +The split-config polling strategy assembles the final router execution config from separate CDN files. +It fetches the base graph and each feature flag config independently. +These rules define behavior when feature flag files are missing or excluded. +These rules apply only when split-config loading is enabled. +When a custom storage provider is configured, the router uses the regular polling strategy and these rules do not apply.As per coding guidelines, "Prefer short, declarative sentences. If a sentence has more than one comma-separated clause, consider splitting it."
🤖 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 `@docs-website/router/configuration.mdx` at line 1287, The long explanatory sentence about the split-config polling strategy should be broken into multiple short, declarative sentences for readability: describe that the split-config polling strategy assembles the final router execution config by fetching the base graph and each feature-flag config as separate CDN files; state that the rules govern behavior when individual feature-flag files are missing or should be excluded; and finally state that these rules only apply when the router is enrolled in split-config loading and that a custom storage provider causes the router to fall back to the regular polling strategy (making the rules a no-op). Use the terms "split-config polling strategy", "router", and "custom storage provider" so the sentences map to the original content.router-tests/protocol/config_hot_reload_test.go (1)
622-633: ⚡ Quick winUse
testenv.WSWriteJSONin the new helper.Line 625 still writes the subscription with
conn.WriteJSON, which bypasses the retry/deadline wrapper the rest of these router tests use and makes this reload test more timing-sensitive than it needs to be.As per coding guidelines "Use testenv.WSReadJSON and testenv.WSWriteJSON for WebSocket reads and writes in tests instead of conn.ReadJSON and conn.WriteJSON, as these helpers include retry logic with 2-second deadlines and exponential backoff".
🤖 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 `@router-tests/protocol/config_hot_reload_test.go` around lines 622 - 633, The helper function subscribe currently uses conn.WriteJSON to send the subscription message (inside the subscribe function that calls xEnv.InitGraphQLWebSocketConnection and constructs a testenv.WebSocketMessage); replace that conn.WriteJSON call with the test helper testenv.WSWriteJSON(t, conn, &testenv.WebSocketMessage{...}) so the write uses the 2s-deadline/retry wrapper used by other tests and remove the direct conn.WriteJSON invocation.
🤖 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 `@cli/src/commands/feature-flag/commands/recompose.ts`:
- Line 12: The CLI command in recompose.ts calls command.description() with no
text, leaving the help output empty; update the call to command.description(...)
to pass a short descriptive string explaining what the "recompose" command does
(e.g., its purpose and main effect) so users see meaningful help output—locate
the command.description() invocation in recompose.ts and replace the empty call
with an appropriate description for the command.
- Around line 44-55: The RPC call to opts.client.platform.recomposeFeatureFlag
can throw transport-level ConnectError and needs explicit handling: wrap the
await opts.client.platform.recomposeFeatureFlag(...) call in a try-catch, catch
errors of type ConnectError (import from the Connect library used in the
project) and handle them by stopping/failing the spinner (spinner.fail(...)) and
printing a user-friendly error message including error.message (or spinner.fail
with a descriptive string) and then exit/return; ensure normal non-ConnectError
exceptions are rethrown or handled the same way. Reference the spinner variable
and the recomposeFeatureFlag call (and keep the existing getBaseHeaders usage)
when adding the try-catch so the spinner is always stopped on error.
In `@cli/test/feature-flag/utils.ts`:
- Around line 23-48: runRecompose currently builds args as ['recompose',
'feature-flag'] but never supplies the required <name> positional argument (and
parsing will fail to match the RecomposeCommand), so change runRecompose to
accept a name (e.g., add a required name: string parameter or a name field on
opts) and push that name into args immediately after 'feature-flag'; keep the
existing flags handling and RecomposeCommand({ client: createClient(response) })
usage unchanged so the command parser receives ['recompose','feature-flag',
name, ...flags].
In `@controlplane/migrations/0138_lazy_speedball.sql`:
- Line 8: The migration uses PostgreSQL 15+ syntax with CONSTRAINT
"fed_graph_feature_flag_idx" UNIQUE NULLS NOT
DISTINCT("federated_graph_id","feature_flag_id") which will fail on PG14;
replace this single constraint with two partial unique indexes on the
federated_graph table to preserve semantics: create a unique index
"fed_graph_feature_flag_idx_not_null" on (federated_graph_id, feature_flag_id)
WHERE feature_flag_id IS NOT NULL and create a unique index
"fed_graph_feature_flag_idx_null" on (federated_graph_id) WHERE feature_flag_id
IS NULL, and remove the original CONSTRAINT "fed_graph_feature_flag_idx" so the
migration works on PostgreSQL versions <15.
In `@controlplane/src/core/bufservices/feature-flag/recomposeFeatureFlag.ts`:
- Around line 93-125: The audit log is created after the transaction that calls
compositionService.composeAndDeployFeatureFlag, causing a partial commit risk;
move the AuditLogRepository.addAuditLog call into the same opts.db.transaction
closure so it runs on the same tx and is committed/rolled back together.
Concretely: after composeAndDeployFeatureFlag returns inside the transaction,
instantiate AuditLogRepository with the same tx (instead of opts.db) and call
addAuditLog there before the transaction returns, ensuring the audit entry is
part of the same atomic transaction.
In `@controlplane/src/core/bufservices/feature-flag/updateFeatureFlag.ts`:
- Around line 115-143: The code currently returns a not-found result after
mutating state (AuditLogRepository.addAuditLog and the prior update), which
prevents the transaction from rolling back; instead, inside the same transaction
where txFeatureFlagRepo.getFeatureFlagById is called, throw an exception when
updatedFeatureFlag is null (e.g., throw new Error with a clear message
referencing featureFlagDTO.id/name) so the transaction aborts and rolls back the
prior changes; update any surrounding handler logic to translate this thrown
error into the appropriate ERR_NOT_FOUND or retry behavior as needed.
In
`@controlplane/src/core/bufservices/federated-graph/createFederatedGraphToken.ts`:
- Around line 75-94: The JWT issued by signJwtHS256 for GraphApiKeyJwtPayload
currently embeds the organization's feature flag (features /
'split-config-loading') with no expiration, so tokens remain valid forever;
update the token payload in createFederatedGraphToken to include an exp claim
(e.g., compute nowInSeconds() + 5*60 or a configured TTL) when calling
signJwtHS256 (using authContext, graph.id, opts.jwtSecret as before), or
alternatively implement runtime feature revalidation in the CDN gate that checks
OrganizationRepository.getFeature for organizationId before honoring the
feature-based JWT claims (ensure manifestMapperPath/manifestLatestPath gates
call the orgRepo instead of relying solely on the token).
In `@controlplane/src/core/composition/composer.ts`:
- Around line 225-246: The current check silently ignores partial pathOverride;
update the logic in composer.ts so that when pathOverride is provided you
require both pathOverride.ready and pathOverride.draft: if both exist, set
s3PathDraft and s3PathReady as now; if pathOverride is present but one is
missing, push a RouterConfigUploadError with a clear message (e.g. 'pathOverride
must include both "ready" and "draft"') onto errors and return { errors }; keep
the existing default path calculation only when pathOverride is not provided at
all and reference the symbols pathOverride, s3PathDraft, s3PathReady, and
RouterConfigUploadError to locate the change.
In `@controlplane/src/core/repositories/SubgraphRepository.ts`:
- Around line 563-568: The array passed into
compositionService.recomposeAndDeployAffected (updatedFederatedGraphs produced
by bySubgraphLabels(...)) can include contract graphs and must be filtered out
here just like in the update() path; modify the code before calling
recomposeAndDeployAffected to filter updatedFederatedGraphs =
updatedFederatedGraphs.filter(g => !g.contract) (or equivalent) so contract
graphs are excluded from the move-triggered recomposition and won't be
recomposed twice.
In `@controlplane/src/core/services/CompositionService.ts`:
- Around line 98-130: The current recomposition only builds the base variant
(subgraphsToCompose contains a single base entry), leaving feature-flag
manifests stale; change the flow to reuse the legacy getSubgraphsToCompose(...)
to produce the full array of variants and pass that into composeGraphsInWorker.
Specifically, replace the hardcoded subgraphsToCompose array in
composeAndDeployFederatedGraph (the call to composeGraphsInWorker) with the
result of getSubgraphsToCompose(federatedGraph, subgraphs, contracts, /* any
existing args */) so tagOptionsByContractName and compositionOptions are applied
for every variant, then continue to call `#handleCompositionResultsAndDeploy` with
graphAndCompositionResults built from those full results so feature-flag
manifests get refreshed too.
- Around line 988-1024: The code currently calls `#saveRouterConfigHash` and
advertises a new mapper before verifying composeAndUploadRouterConfig returned
no admission/upload errors; change the logic so you only call
`#saveRouterConfigHash` (and any subsequent call to updateMapperForFederatedGraph)
when uploadErrors contains no AdmissionError or RouterConfigUploadError.
Concretely, after calling composeAndUploadRouterConfig and collecting
uploadErrors, compute whether uploadErrors.filter(e => e instanceof
AdmissionError || e instanceof RouterConfigUploadError).length === 0, and only
then invoke this.#saveRouterConfigHash(graph.id, ...) and the mapper refresh in
both places where you currently persist the hash (the blocks around the
composeAndUploadRouterConfig call and the later similar block that updates the
mapper).
In `@controlplane/src/db/schema.ts`:
- Around line 2651-2652: The createdAt and updatedAt columns are currently
timezone-naive and nullable; change them to be timezone-aware and make createdAt
non-nullable to match the rest of the schema: use
timestamp('created_at').withTimezone(true).defaultNow().notNull() for createdAt
and timestamp('updated_at').withTimezone(true) (optionally .defaultNow() if
desired) for updatedAt; update the column definitions where createdAt and
updatedAt are declared to use withTimezone(true) and add notNull() to createdAt.
In `@controlplane/test/feature-flag/feature-flag-integration-v2.test.ts`:
- Around line 31-36: The mock registered for ClickHouseClient uses the wrong
module specifier; update the vi.mock call in feature-flag-integration-v2.test.ts
to use the exact import path '../../src/core/clickhouse/index.js' (matching the
import at line 25) so Vitest will intercept the module; ensure the mock still
returns { ClickHouseClient } and that ClickHouseClient.prototype.queryPromise is
mocked as before.
In `@controlplane/test/feature-flag/recompose-feature-flag.test.ts`:
- Around line 147-163: The test assumes blobStorage.keys() returns a stable
insertion order which can flap; update the assertions to avoid positional
indexing by either sorting the keys by name (e.g., alphabetically) before
asserting positions or, better, assert membership of the expected paths instead
of relying on indexes. Locate the checks around blobStorage.keys() and replace
usages that do keys()[0], keys()[1], or keys().at(-1) with a stable approach
(sort the array or use array.includes) when asserting the presence of
`${federatedGraphResponse.graph!.id}/manifest/latest.json`,
`${federatedGraphResponse.graph!.id}/manifest/mapper.json`, and the feature-flag
path
`${federatedGraphResponse.graph!.id}/manifest/feature-flags/${featureFlagName}.json`
(these appear alongside assertFeatureFlagExecutionConfig,
assertNumberOfCompositions, and createFeatureFlag).
- Around line 12-23: The test's vi.mock path doesn't match the actual import so
the real ClickHouseClient is used; update the vi.mock call to target the exact
same module string used by the import (the module that exports ClickHouseClient)
so the mock intercepts it, and ensure the mock provides ClickHouseClient with a
mocked prototype method queryPromise (i.e., keep ClickHouseClient and
ClickHouseClient.prototype.queryPromise in the mock).
In `@router/pkg/controlplane/configpoller/split_config_poller.go`:
- Around line 204-272: The polling path must apply the same ConfigRules
filtering as GetRouterConfig/Subscribe: before calling
computeCompositeVersion(activeGraphs) and before computing changes and building
toFetch, filter activeGraphs using p.configRules (honor ignored flags and
SkipMissingFeatureFlags behavior), and when FetchConfig(name) returns
ErrFileNotFound and SkipMissingFeatureFlags is set, skip that name instead of
returning early; ensure when applying fetchedConfig into
patched.FeatureFlagConfigs you only insert entries that pass the config rules.
Update logic around computeCompositeVersion, the changes loop, toFetch
population, and the FetchConfig error handling (references:
computeCompositeVersion(activeGraphs), p.knownHashes, p.currentConfig,
p.fetcher.FetchConfig, ErrFileNotFound, SkipMissingFeatureFlags) to perform this
filtering so ignored/missing feature-flag configs are not reintroduced during
poll reloads.
In `@router/pkg/routerconfig/cdn/split_fetcher.go`:
- Around line 85-86: The CDN base path prefix is being dropped because
ResolveReference is called with an absolute Path (starting with "/"), so update
the code paths built by FetchMapper/FetchConfig and the post function to
preserve the base path: use url.JoinPath to join f.cdnURL.Path with the request
path (e.g., joined := url.JoinPath(f.cdnURL.Path, path)) and then construct the
target URL from the base by setting its Path to the joined value (or
ResolveReference with a relative path) instead of passing the absolute path
directly to ResolveReference; adjust the code in post (and callers
FetchMapper/FetchConfig) to use f.cdnURL, url.JoinPath, and the new joined path
so the configured CDN prefix is retained.
---
Outside diff comments:
In
`@controlplane/src/core/bufservices/graph/setGraphRouterCompatibilityVersion.ts`:
- Around line 115-147: The transaction currently calls
FederatedGraphRepository.updateRouterCompatibilityVersion and
AuditLogRepository.addAuditLog before running
CompositionService.composeAndDeployFederatedGraph, which can commit a version
bump even if composition fails; change the flow so composition succeeds before
persisting the new version and audit log: either (A) run
compositionService.composeAndDeployFederatedGraph outside the DB transaction
against the in-memory/current federatedGraph snapshot and only open a
transaction to call
FederatedGraphRepository.updateRouterCompatibilityVersion(federatedGraph.id,
version) and AuditLogRepository.addAuditLog when composition returns no
deploymentErrors/compositionErrors, or (B) if you must run composition inside
the same tx, inspect the returned deploymentErrors/compositionErrors and throw
an error (or return a failure that aborts the tx) when any errors exist so the
transaction rolls back instead of committing the version bump.
- Around line 99-112: The empty-subgraphs branch updates the router
compatibility version but omits creating an audit entry; before returning, call
the same audit logging routine used in the non-empty branch (the
AuditLogRepository / audit logging method invoked elsewhere in this file) to
record the change: pass federatedGraph.id, previous version
federatedGraph.routerCompatibilityVersion, new version variable version (or
req.version), and actor context from authContext/logger so the version change on
graphs with zero subgraphs appears in the audit trail; insert this audit call
right after FederatedGraphRepository.updateRouterCompatibilityVersion(...) and
before the return.
In `@router/core/graph_server.go`:
- Around line 1387-1458: Reused graph muxes keep references to s.connector,
s.pubSubProviders, and s.connectionMetrics which Shutdown can tear down; update
buildGraphMux and Shutdown to avoid dangling resources by transferring or
isolating ownership for reused muxes: when buildGraphMux marks a mux as reused,
ensure it receives independent copies or owned wrappers of s.connector,
s.pubSubProviders and s.connectionMetrics (or increment a reference count) so
Shutdown doesn't stop resources still used by that mux; alternatively, have
shutdown check ownership/refcount before closing resources. Touch symbols:
buildGraphMux, Shutdown, s.connector, s.pubSubProviders, s.connectionMetrics,
startPubSubProviders, and ExecutorConfigurationBuilder/Build to implement
ownership transfer or refcounting so reused muxes remain functional after a
server swap.
In `@router/core/router.go`:
- Around line 822-827: The code uses the poller response (cfg) and its nested
Config without validating them; update the startup and hot-reload call sites
that call r.configPoller.GetRouterConfig(ctx) (e.g., the block that then calls
r.newServer(ctx, cfg)) to check that cfg != nil and cfg.Config != nil before
passing to r.newServer (and similarly where the poller response is used
elsewhere), and if either is nil return a wrapped error like "invalid router
config: missing response or Config" (or handle with a safe default) so the
process cannot dereference a nil response.Config.
---
Nitpick comments:
In `@docs-website/router/configuration.mdx`:
- Line 1287: The long explanatory sentence about the split-config polling
strategy should be broken into multiple short, declarative sentences for
readability: describe that the split-config polling strategy assembles the final
router execution config by fetching the base graph and each feature-flag config
as separate CDN files; state that the rules govern behavior when individual
feature-flag files are missing or should be excluded; and finally state that
these rules only apply when the router is enrolled in split-config loading and
that a custom storage provider causes the router to fall back to the regular
polling strategy (making the rules a no-op). Use the terms "split-config polling
strategy", "router", and "custom storage provider" so the sentences map to the
original content.
In `@router-tests/protocol/config_hot_reload_test.go`:
- Around line 622-633: The helper function subscribe currently uses
conn.WriteJSON to send the subscription message (inside the subscribe function
that calls xEnv.InitGraphQLWebSocketConnection and constructs a
testenv.WebSocketMessage); replace that conn.WriteJSON call with the test helper
testenv.WSWriteJSON(t, conn, &testenv.WebSocketMessage{...}) so the write uses
the 2s-deadline/retry wrapper used by other tests and remove the direct
conn.WriteJSON invocation.
In `@router/pkg/config/config.schema.json`:
- Around line 415-418: The ignored_feature_flags schema currently allows empty
strings and duplicates; tighten validation by requiring non-empty entries and no
duplicates: update the "ignored_feature_flags" entry to set "items": { "type":
"string", "minLength": 1 } and add "uniqueItems": true at the array level so
each flag is a non-empty string and duplicates are rejected (reference symbol:
"ignored_feature_flags").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 02c9adde-1616-4615-a0d8-6d26482f4df8
⛔ Files ignored due to path filters (2)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**connect-go/gen/proto/wg/cosmo/platform/v1/platformv1connect/platform.connect.gois excluded by!**/gen/**
📒 Files selected for processing (72)
cdn-server/cdn/src/index.tscdn-server/cdn/test/cdn.test.tscli/src/commands/feature-flag/commands/recompose.tscli/src/commands/feature-flag/index.tscli/test/feature-flag/recompose.test.tscli/test/feature-flag/utils.tsconnect/src/wg/cosmo/platform/v1/platform-PlatformService_connectquery.tsconnect/src/wg/cosmo/platform/v1/platform_connect.tsconnect/src/wg/cosmo/platform/v1/platform_pb.tscontrolplane/migrations/0138_lazy_speedball.sqlcontrolplane/migrations/meta/0138_snapshot.jsoncontrolplane/migrations/meta/_journal.jsoncontrolplane/src/core/bufservices/PlatformService.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/recomposeFeatureFlag.tscontrolplane/src/core/bufservices/feature-flag/updateFeatureFlag.tscontrolplane/src/core/bufservices/federated-graph/createFederatedGraph.tscontrolplane/src/core/bufservices/federated-graph/createFederatedGraphToken.tscontrolplane/src/core/bufservices/federated-graph/migrateFromApollo.tscontrolplane/src/core/bufservices/federated-graph/moveFederatedGraph.tscontrolplane/src/core/bufservices/federated-graph/updateFederatedGraph.tscontrolplane/src/core/bufservices/graph/recomposeGraph.tscontrolplane/src/core/bufservices/graph/setGraphRouterCompatibilityVersion.tscontrolplane/src/core/bufservices/monograph/publishMonograph.tscontrolplane/src/core/bufservices/monograph/updateMonograph.tscontrolplane/src/core/bufservices/subgraph/deleteFederatedSubgraph.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/repositories/FeatureFlagRepository.tscontrolplane/src/core/repositories/FederatedGraphRepository.tscontrolplane/src/core/repositories/OrganizationRepository.tscontrolplane/src/core/repositories/SubgraphRepository.tscontrolplane/src/core/services/CompositionService.tscontrolplane/src/db/models.tscontrolplane/src/db/schema.tscontrolplane/src/types/index.tscontrolplane/test/feature-flag/feature-flag-integration-v2.test.tscontrolplane/test/feature-flag/feature-flag-integration.test.tscontrolplane/test/feature-flag/recompose-feature-flag.test.tsdocs-website/router/configuration.mdxproto/wg/cosmo/platform/v1/platform.protorouter-tests/events/nats_events_test.gorouter-tests/lifecycle/graph_server_swap_test.gorouter-tests/operations/cache_warmup_test.gorouter-tests/operations/plan_fallback_cache_test.gorouter-tests/protocol/config_hot_reload_test.gorouter/core/graph_server.gorouter/core/graph_server_test.gorouter/core/init_config_poller.gorouter/core/router.gorouter/core/supervisor_instance.gorouter/internal/jwt/claims.gorouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/fixtures/full.yamlrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.jsonrouter/pkg/controlplane/configpoller/config_poller.gorouter/pkg/controlplane/configpoller/split_config_poller.gorouter/pkg/controlplane/configpoller/split_config_poller_test.gorouter/pkg/errs/errors.gorouter/pkg/routerconfig/cdn/client.gorouter/pkg/routerconfig/cdn/split_fetcher.gorouter/pkg/routerconfig/cdn/split_fetcher_test.gorouter/pkg/routerconfig/client.gorouter/pkg/routerconfig/s3/client.go
|
|
||
| export default (opts: BaseCommandOptions) => { | ||
| const command = new Command('recompose'); | ||
| command.description(); |
Contributor
There was a problem hiding this comment.
Missing description string for the command.
command.description() is called without an argument, resulting in an empty description. This should describe what the command does.
🔧 Proposed fix
- command.description();
+ command.description('Recompose a feature flag and deploy the updated composition.');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| command.description(); | |
| command.description('Recompose a feature flag and deploy the updated composition.'); |
🤖 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 `@cli/src/commands/feature-flag/commands/recompose.ts` at line 12, The CLI
command in recompose.ts calls command.description() with no text, leaving the
help output empty; update the call to command.description(...) to pass a short
descriptive string explaining what the "recompose" command does (e.g., its
purpose and main effect) so users see meaningful help output—locate the
command.description() invocation in recompose.ts and replace the empty call with
an appropriate description for the command.
Comment on lines
+44
to
+55
| const spinner = ora(`Recomposing feature flag "${name}"...`).start(); | ||
| const resp = await opts.client.platform.recomposeFeatureFlag( | ||
| { | ||
| disableResolvabilityValidation: options.disableResolvabilityValidation, | ||
| limit, | ||
| name, | ||
| namespace: options.namespace, | ||
| }, | ||
| { | ||
| headers: getBaseHeaders(), | ||
| }, | ||
| ); |
Contributor
There was a problem hiding this comment.
Missing error handling for ConnectRPC transport errors.
The RPC call opts.client.platform.recomposeFeatureFlag() can throw a ConnectError for transport-level failures (network errors, timeouts, protocol errors). Without a try-catch, these errors will propagate as unhandled rejections instead of being displayed as user-friendly error messages. Based on learnings, ConnectRPC calls in CLI commands should handle ConnectError rejections appropriately.
🛡️ Proposed fix
+import { ConnectError } from '@connectrpc/connect';
+
const spinner = ora(`Recomposing feature flag "${name}"...`).start();
- const resp = await opts.client.platform.recomposeFeatureFlag(
- {
- disableResolvabilityValidation: options.disableResolvabilityValidation,
- limit,
- name,
- namespace: options.namespace,
- },
- {
- headers: getBaseHeaders(),
- },
- );
+ let resp;
+ try {
+ resp = await opts.client.platform.recomposeFeatureFlag(
+ {
+ disableResolvabilityValidation: options.disableResolvabilityValidation,
+ limit,
+ name,
+ namespace: options.namespace,
+ },
+ {
+ headers: getBaseHeaders(),
+ },
+ );
+ } catch (e) {
+ spinner.fail(`Failed to recompose feature flag "${pc.bold(name)}".`);
+ if (e instanceof ConnectError) {
+ program.error(pc.red(e.message));
+ }
+ throw e;
+ }🤖 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 `@cli/src/commands/feature-flag/commands/recompose.ts` around lines 44 - 55,
The RPC call to opts.client.platform.recomposeFeatureFlag can throw
transport-level ConnectError and needs explicit handling: wrap the await
opts.client.platform.recomposeFeatureFlag(...) call in a try-catch, catch errors
of type ConnectError (import from the Connect library used in the project) and
handle them by stopping/failing the spinner (spinner.fail(...)) and printing a
user-friendly error message including error.message (or spinner.fail with a
descriptive string) and then exit/return; ensure normal non-ConnectError
exceptions are rethrown or handled the same way. Reference the spinner variable
and the recomposeFeatureFlag call (and keep the existing getBaseHeaders usage)
when adding the try-catch so the spinner is always stopped on error.
Comment on lines
+23
to
+48
| export async function runRecompose( | ||
| response: PartialMessage<RecomposeFeatureFlagResponse>, | ||
| opts: { | ||
| namespace?: string; | ||
| failOnCompositionError?: boolean; | ||
| failOnAdmissionWebhookError?: boolean; | ||
| suppressWarnings?: boolean; | ||
| } = {}, | ||
| ): Promise<void> { | ||
| const args = ['recompose', 'feature-flag']; | ||
| if (opts.namespace) { | ||
| args.push('--namespace', opts.namespace); | ||
| } | ||
| if (opts.failOnCompositionError) { | ||
| args.push('--fail-on-composition-error'); | ||
| } | ||
| if (opts.failOnAdmissionWebhookError) { | ||
| args.push('--fail-on-admission-webhook-error'); | ||
| } | ||
| if (opts.suppressWarnings) { | ||
| args.push('--suppress-warnings'); | ||
| } | ||
|
|
||
| const program = new Command(); | ||
| program.addCommand(RecomposeCommand({ client: createClient(response) })); | ||
| await program.parseAsync(args, { from: 'user' }); |
Contributor
There was a problem hiding this comment.
Missing required <name> argument for the recompose command.
The recompose command requires a <name> argument (as defined in recompose.ts line 13), but runRecompose never adds it to the args array. Also, the args array starts with ['recompose', 'feature-flag'] but the command being added is named 'recompose', so parsing will fail to match.
🐛 Proposed fix
export async function runRecompose(
response: PartialMessage<RecomposeFeatureFlagResponse>,
opts: {
+ name: string;
namespace?: string;
failOnCompositionError?: boolean;
failOnAdmissionWebhookError?: boolean;
suppressWarnings?: boolean;
- } = {},
+ },
): Promise<void> {
- const args = ['recompose', 'feature-flag'];
+ const args = ['recompose', opts.name];
if (opts.namespace) {
args.push('--namespace', opts.namespace);
}🤖 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 `@cli/test/feature-flag/utils.ts` around lines 23 - 48, runRecompose currently
builds args as ['recompose', 'feature-flag'] but never supplies the required
<name> positional argument (and parsing will fail to match the
RecomposeCommand), so change runRecompose to accept a name (e.g., add a required
name: string parameter or a name field on opts) and push that name into args
immediately after 'feature-flag'; keep the existing flags handling and
RecomposeCommand({ client: createClient(response) }) usage unchanged so the
command parser receives ['recompose','feature-flag', name, ...flags].
| "hash" text NOT NULL, | ||
| "created_at" timestamp DEFAULT now(), | ||
| "updated_at" timestamp, | ||
| CONSTRAINT "fed_graph_feature_flag_idx" UNIQUE NULLS NOT DISTINCT("federated_graph_id","feature_flag_id") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find declared PostgreSQL versions or constraints in infra/docs/config.
rg -n --iglob '*.yml' --iglob '*.yaml' --iglob '*.md' --iglob '*docker*' \
'POSTGRES_VERSION|postgresql?\s*(version)?\s*[:=]?\s*([0-9]{2}(\.[0-9]+)?)|postgres:[0-9]{2}'
# Check if PG15-only NULLS NOT DISTINCT syntax is already used elsewhere.
rg -n 'UNIQUE\s+NULLS\s+NOT\s+DISTINCT|NULLS\s+NOT\s+DISTINCT'Repository: wundergraph/cosmo
Length of output: 1066
🏁 Script executed:
# Get the actual postgres image version from docker-compose
grep -n "image.*postgres" docker-compose.full.yml
# Check for any version documentation in README or docs
find . -maxdepth 2 -type f \( -name "README.md" -o -name "*.md" \) -exec grep -l "postgres\|database.*version" {} \;
# Look for environment files or version specs
fd -e ".env*" -e "package.json" -e "*.toml" | head -20Repository: wundergraph/cosmo
Length of output: 170
🏁 Script executed:
# Check the actual docker-compose.full.yml around the postgres service definition
sed -n '1,350p' docker-compose.full.yml | grep -A 5 -B 5 "postgres"Repository: wundergraph/cosmo
Length of output: 2133
🏁 Script executed:
# Search for PostgreSQL version requirements in documentation
rg -n "postgres\|database" -i README.md
# Check for any version constraints or compatibility matrix
rg -n "version.*support|support.*version|compatible|requires" -i docs/ README.md --max-count=20
# Look for .env.example or similar to see documented versions
fd -e ".env*" -e "VERSION" | grep -v node_modules | head -20Repository: wundergraph/cosmo
Length of output: 325
🏁 Script executed:
# Check docker/README.md specifically since it was found
cat docker/README.md | head -100Repository: wundergraph/cosmo
Length of output: 3535
🏁 Script executed:
# Check for CI/CD configuration that might specify versions
fd -e ".yml" -e ".yaml" -e ".json" .github/ 2>/dev/null | head -20
# Look at what's in .github/workflows
ls -la .github/workflows/ 2>/dev/null || echo "No workflows found"
# Check if there are any version requirements in package.json or similar
cat package.json 2>/dev/null | grep -i "postgres\|version" | head -20Repository: wundergraph/cosmo
Length of output: 2634
🏁 Script executed:
# Search for minimum version requirements in docs or config files
rg -n "minimum|minimum.*version|15|require|support" -i docs/migrations.mdRepository: wundergraph/cosmo
Length of output: 372
🏁 Script executed:
# Search for any version support matrix or compatibility docs
rg -n "support\|compatible\|version.*requirement" -i . --type md | grep -i "postgres\|database\|version" | head -30
# Check the main README
head -200 README.mdRepository: wundergraph/cosmo
Length of output: 5898
🏁 Script executed:
# Check if there are any issues or comments about version support
rg -n "14|version.*require" controlplane/migrations/ | head -20Repository: wundergraph/cosmo
Length of output: 1759
🏁 Script executed:
# Check the controlplane Dockerfile and build configs
fd -e "Dockerfile" controlplane/ | xargs cat
# Check Helm values for any version constraints
rg -n "postgres" helm/ -i -A 3 -B 3 | head -50
# Look for any documentation about database setup/requirements
fd -e "*.md" docs/ | xargs grep -l "postgres\|database\|setup" -i | head -5Repository: wundergraph/cosmo
Length of output: 3568
🏁 Script executed:
# Check if there are any setup docs or prerequisites listed
cat docs/setup.md 2>/dev/null || cat docs/README.md 2>/dev/null | head -100Repository: wundergraph/cosmo
Length of output: 43
🏁 Script executed:
# Check if there are any version requirements mentioned in other migrations
rg -n "postgres" controlplane/migrations/ -i | grep -i "version\|require\|support\|15\|14" | head -20
# Look at the migration metadata to see when this was introduced
cat controlplane/migrations/meta/_journal.json | grep -A 20 "0138"Repository: wundergraph/cosmo
Length of output: 878
🏁 Script executed:
# Check for any environment configuration or deployment docs
fd . --type f --full-path "*/docs/*" -e "md" | xargs grep -l "postgres\|database.*requirement" -i 2>/dev/null | head -5Repository: wundergraph/cosmo
Length of output: 157
🏁 Script executed:
# Search for any explicit version requirements in docs directory
find docs -type f -name "*.md" 2>/dev/null | head -20Repository: wundergraph/cosmo
Length of output: 134
🏁 Script executed:
# Check CONTRIBUTING.md for development setup requirements
cat CONTRIBUTING.md 2>/dev/null | grep -A 10 -B 2 "postgres\|database\|version" -i | head -50Repository: wundergraph/cosmo
Length of output: 338
🏁 Script executed:
# Check if there are any .env.example or similar files
fd . --type f --full-path "*" -name ".env*" -o -name "*requirements*" -o -name "setup*" 2>/dev/null | head -20Repository: wundergraph/cosmo
Length of output: 43
🏁 Script executed:
# Check if there's any reference to version constraints in setup/deployment docs
cat docs/migrations.mdRepository: wundergraph/cosmo
Length of output: 1987
Confirm PostgreSQL 15+ requirement for UNIQUE NULLS NOT DISTINCT syntax.
This migration uses PostgreSQL 15+ syntax that will fail on PostgreSQL 14 or earlier. While the default docker-compose configuration uses PostgreSQL 15.3, the version is configurable via DC_POSTGRESQL_VERSION environment variable with no documented minimum version requirement. If any deployment environment uses PostgreSQL 14 or lower, this migration will fail during DDL execution.
Consider using two partial unique indexes instead to maintain backward compatibility:
CREATE UNIQUE INDEX "fed_graph_feature_flag_idx_not_null"
ON "federated_graph" ("federated_graph_id", "feature_flag_id")
WHERE "feature_flag_id" IS NOT NULL;
CREATE UNIQUE INDEX "fed_graph_feature_flag_idx_null"
ON "federated_graph" ("federated_graph_id")
WHERE "feature_flag_id" IS NULL;This preserves the same constraint semantics without requiring PostgreSQL 15+.
🤖 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 `@controlplane/migrations/0138_lazy_speedball.sql` at line 8, The migration
uses PostgreSQL 15+ syntax with CONSTRAINT "fed_graph_feature_flag_idx" UNIQUE
NULLS NOT DISTINCT("federated_graph_id","feature_flag_id") which will fail on
PG14; replace this single constraint with two partial unique indexes on the
federated_graph table to preserve semantics: create a unique index
"fed_graph_feature_flag_idx_not_null" on (federated_graph_id, feature_flag_id)
WHERE feature_flag_id IS NOT NULL and create a unique index
"fed_graph_feature_flag_idx_null" on (federated_graph_id) WHERE feature_flag_id
IS NULL, and remove the original CONSTRAINT "fed_graph_feature_flag_idx" so the
migration works on PostgreSQL versions <15.
Comment on lines
+93
to
+125
| const { deploymentErrors, compositionErrors, compositionWarnings } = await opts.db.transaction((tx) => { | ||
| const compositionService = new CompositionService( | ||
| tx, | ||
| authContext.organizationId, | ||
| logger, | ||
| { cdnBaseUrl: opts.cdnBaseUrl, webhookJWTSecret: opts.admissionWebhookJWTSecret }, | ||
| opts.blobStorage, | ||
| opts.chClient, | ||
| opts.webhookProxyUrl, | ||
| req.disableResolvabilityValidation, | ||
| ); | ||
|
|
||
| return compositionService.composeAndDeployFeatureFlag({ | ||
| actorId: authContext.userId, | ||
| featureFlag, | ||
| }); | ||
| }); | ||
|
|
||
| const auditLogRepo = new AuditLogRepository(opts.db); | ||
| await auditLogRepo.addAuditLog({ | ||
| organizationId: authContext.organizationId, | ||
| organizationSlug: authContext.organizationSlug, | ||
| auditAction: 'feature_flag.recomposed', | ||
| action: 'recomposed', | ||
| actorId: authContext.userId, | ||
| auditableType: 'feature_flag', | ||
| auditableDisplayName: featureFlag.name, | ||
| actorDisplayName: authContext.userDisplayName, | ||
| apiKeyName: authContext.apiKeyName, | ||
| actorType: authContext.auth === 'api_key' ? 'api_key' : 'user', | ||
| targetNamespaceId: featureFlag.namespaceId, | ||
| targetNamespaceDisplayName: featureFlag.namespace, | ||
| }); |
Contributor
There was a problem hiding this comment.
Keep the audit log inside the recomposition transaction.
The recomposition/deployment commits first, then the audit log runs separately on opts.db. If addAuditLog fails, this RPC returns an error even though the feature flag was already recomposed, and a retry will repeat the work.
🤖 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 `@controlplane/src/core/bufservices/feature-flag/recomposeFeatureFlag.ts`
around lines 93 - 125, The audit log is created after the transaction that calls
compositionService.composeAndDeployFeatureFlag, causing a partial commit risk;
move the AuditLogRepository.addAuditLog call into the same opts.db.transaction
closure so it runs on the same tx and is committed/rolled back together.
Concretely: after composeAndDeployFeatureFlag returns inside the transaction,
instantiate AuditLogRepository with the same tx (instead of opts.db) and call
addAuditLog there before the transaction returns, ensuring the audit entry is
part of the same atomic transaction.
Comment on lines
+31
to
+36
| vi.mock('../src/core/clickhouse/index.js', () => { | ||
| const ClickHouseClient = vi.fn(); | ||
| ClickHouseClient.prototype.queryPromise = vi.fn(); | ||
|
|
||
| return { ClickHouseClient }; | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'ClickHouseClient|vi\.mock' controlplane/test/feature-flag/feature-flag-integration-v2.test.tsRepository: wundergraph/cosmo
Length of output: 397
Fix the mock path to match the imported module specifier.
ClickHouseClient is imported from ../../src/core/clickhouse/index.js (line 25), but the mock is registered for ../src/core/clickhouse/index.js (line 31). Vitest requires the exact module specifier, so this mismatch prevents the mock from intercepting the import—the real client will be instantiated instead.
Suggested fix
-vi.mock('../src/core/clickhouse/index.js', () => {
+vi.mock('../../src/core/clickhouse/index.js', () => {🤖 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 `@controlplane/test/feature-flag/feature-flag-integration-v2.test.ts` around
lines 31 - 36, The mock registered for ClickHouseClient uses the wrong module
specifier; update the vi.mock call in feature-flag-integration-v2.test.ts to use
the exact import path '../../src/core/clickhouse/index.js' (matching the import
at line 25) so Vitest will intercept the module; ensure the mock still returns {
ClickHouseClient } and that ClickHouseClient.prototype.queryPromise is mocked as
before.
Comment on lines
+12
to
+23
| import { ClickHouseClient } from '../../src/core/clickhouse/index.js'; | ||
|
|
||
| // Change to true to enable a longer timeout | ||
| const isDebugMode = false; | ||
| let dbname = ''; | ||
|
|
||
| vi.mock('../src/core/clickhouse/index.js', () => { | ||
| const ClickHouseClient = vi.fn(); | ||
| ClickHouseClient.prototype.queryPromise = vi.fn(); | ||
|
|
||
| return { ClickHouseClient }; | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the test file
find . -name "recompose-feature-flag.test.ts" -type fRepository: wundergraph/cosmo
Length of output: 125
🏁 Script executed:
# Also check the repository structure
git ls-files | grep -E "(clickhouse|test)" | head -20Repository: wundergraph/cosmo
Length of output: 968
🏁 Script executed:
# Get the structure of the controlplane directory
find controlplane -type d -name "*clickhouse*" -o -type d -name "*test*" | head -20Repository: wundergraph/cosmo
Length of output: 164
🏁 Script executed:
# Read the test file, focusing on the imports and mocks
cat -n controlplane/test/feature-flag/recompose-feature-flag.test.ts | head -30Repository: wundergraph/cosmo
Length of output: 1217
🏁 Script executed:
# Verify the ClickHouse module exists at the import path
test -f controlplane/src/core/clickhouse/index.js && echo "FOUND: controlplane/src/core/clickhouse/index.js" || echo "NOT FOUND"
# Also check if the mock path exists
test -f controlplane/test/src/core/clickhouse/index.js && echo "FOUND: controlplane/test/src/core/clickhouse/index.js" || echo "NOT FOUND"
# List what's actually in the controlplane/test directory
ls -la controlplane/test/ | head -20Repository: wundergraph/cosmo
Length of output: 1491
🏁 Script executed:
# Let's understand how the paths resolve from the test file location
# Test file is at: controlplane/test/feature-flag/recompose-feature-flag.test.ts
# From controlplane/test/feature-flag/:
# ../../src/core/clickhouse/index.js goes up to controlplane/, then into src/core/clickhouse/index.js
test -f "controlplane/src/core/clickhouse/index.js" && echo "Import path resolves to: controlplane/src/core/clickhouse/index.js - EXISTS" || echo "Import path resolves to: controlplane/src/core/clickhouse/index.js - NOT FOUND"
# ../src/core/clickhouse/index.js from controlplane/test/feature-flag/ goes up to controlplane/test/, then into src/core/clickhouse/index.js
test -f "controlplane/test/src/core/clickhouse/index.js" && echo "Mock path resolves to: controlplane/test/src/core/clickhouse/index.js - EXISTS" || echo "Mock path resolves to: controlplane/test/src/core/clickhouse/index.js - NOT FOUND"
# Verify actual ClickHouseClient exists
ls -la controlplane/src/core/clickhouse/Repository: wundergraph/cosmo
Length of output: 447
🏁 Script executed:
# The file is actually .ts not .js - let's check the actual index.ts
cat -n controlplane/src/core/clickhouse/index.ts
# Also let's verify the complete working directory setup
pwdRepository: wundergraph/cosmo
Length of output: 234
🏁 Script executed:
# Let's check the vitest/tsconfig configuration to understand module resolution
cat -n controlplane/vitest.config.ts 2>/dev/null || cat -n controlplane/vitest.config.js 2>/dev/null || echo "No vitest config found"
# Also check tsconfig
cat -n controlplane/tsconfig.json 2>/dev/null | head -50Repository: wundergraph/cosmo
Length of output: 605
🏁 Script executed:
# Let's trace what module the import actually resolves to
# First, check if there's a vitest root config
cat -n vitest.config.ts 2>/dev/null || cat -n vitest.config.js 2>/dev/null || echo "Checking root tsconfig"
# Check root tsconfig for module resolution hints
cat -n tsconfig.base.json 2>/dev/null | head -50Repository: wundergraph/cosmo
Length of output: 653
🏁 Script executed:
# Verify the actual module resolution by checking how other test files mock ClickHouse
grep -r "vi.mock.*clickhouse" controlplane/test --include="*.ts" -A 2Repository: wundergraph/cosmo
Length of output: 13656
🏁 Script executed:
# Let's verify the exact file locations and their import/mock patterns
echo "=== Checking recompose-feature-flag.test.ts ==="
cat -n controlplane/test/feature-flag/recompose-feature-flag.test.ts | grep -E "(^[[:space:]]*12|^[[:space:]]*18)"
echo -e "\n=== File location context ==="
echo "Test file: controlplane/test/feature-flag/recompose-feature-flag.test.ts"
echo "Location depth: one level nested in test/"
echo ""
echo "From controlplane/test/feature-flag/:
- ../../src/core/clickhouse/index.js → controlplane/src/core/clickhouse/index.js ✓
- ../src/core/clickhouse/index.js → controlplane/test/src/core/clickhouse/index.js ✗"
# Check that other nested tests use the correct pattern
echo -e "\n=== Comparison with other nested tests ==="
grep -l "test/feature-flag" <(echo "controlplane/test/feature-flag/feature-flag-integration.test.ts") | while read f; do
grep "^import.*ClickHouseClient" "$f"
grep "^vi.mock" "$f" || echo "(checking feature-flag-integration.test.ts)"
doneRepository: wundergraph/cosmo
Length of output: 1688
🏁 Script executed:
# Final confirmation: list all feature-flag test files and their mock patterns
echo "=== All feature-flag test files and their ClickHouse mocks ==="
ls -1 controlplane/test/feature-flag/*.test.ts | while read f; do
echo "File: $(basename $f)"
grep "^import.*ClickHouseClient" "$f" || echo " (no ClickHouseClient import)"
grep "^vi.mock.*clickhouse" "$f" || grep "vi.mock.*clickhouse" "$f" || echo " (no ClickHouseClient mock)"
echo ""
doneRepository: wundergraph/cosmo
Length of output: 1451
🏁 Script executed:
# Let's verify if there's a pattern - checking the other feature-flag files with imports
cat -n controlplane/test/feature-flag/feature-flag-integration.test.ts | head -25Repository: wundergraph/cosmo
Length of output: 1192
🏁 Script executed:
# Let's check if vi.mock works with relative paths resolved from the config root
# Check if there's any vitest configuration at controlplane level
ls -la controlplane/vite.config.* controlplane/vitest.config.* 2>/dev/null || echo "No vitest config in controlplane"
# Check package.json for test script
cat controlplane/package.json | grep -A 5 '"test"' | head -10Repository: wundergraph/cosmo
Length of output: 608
🏁 Script executed:
# Check the vite.config.ts for vitest configuration
cat -n controlplane/vite.config.tsRepository: wundergraph/cosmo
Length of output: 540
Fix mock path to match the imported module.
Line 12 imports ../../src/core/clickhouse/index.js, but line 18 mocks ../src/core/clickhouse/index.js. These paths resolve to different locations, so the mock won't intercept the actual import and the suite will exercise the real ClickHouseClient.
Suggested fix
-vi.mock('../src/core/clickhouse/index.js', () => {
+vi.mock('../../src/core/clickhouse/index.js', () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { ClickHouseClient } from '../../src/core/clickhouse/index.js'; | |
| // Change to true to enable a longer timeout | |
| const isDebugMode = false; | |
| let dbname = ''; | |
| vi.mock('../src/core/clickhouse/index.js', () => { | |
| const ClickHouseClient = vi.fn(); | |
| ClickHouseClient.prototype.queryPromise = vi.fn(); | |
| return { ClickHouseClient }; | |
| }); | |
| import { ClickHouseClient } from '../../src/core/clickhouse/index.js'; | |
| // Change to true to enable a longer timeout | |
| const isDebugMode = false; | |
| let dbname = ''; | |
| vi.mock('../../src/core/clickhouse/index.js', () => { | |
| const ClickHouseClient = vi.fn(); | |
| ClickHouseClient.prototype.queryPromise = vi.fn(); | |
| return { ClickHouseClient }; | |
| }); |
🤖 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 `@controlplane/test/feature-flag/recompose-feature-flag.test.ts` around lines
12 - 23, The test's vi.mock path doesn't match the actual import so the real
ClickHouseClient is used; update the vi.mock call to target the exact same
module string used by the import (the module that exports ClickHouseClient) so
the mock intercepts it, and ensure the mock provides ClickHouseClient with a
mocked prototype method queryPromise (i.e., keep ClickHouseClient and
ClickHouseClient.prototype.queryPromise in the mock).
Comment on lines
+147
to
+163
| expect(blobStorage.keys()).toHaveLength(2); | ||
| const key = blobStorage.keys()[0]; | ||
| const mapperKey = blobStorage.keys()[1]; | ||
| expect(key).toContain(`${federatedGraphResponse.graph!.id}/manifest/latest.json`); | ||
| expect(mapperKey).toContain(`${federatedGraphResponse.graph!.id}/manifest/mapper.json`); | ||
|
|
||
| await assertFeatureFlagExecutionConfig(blobStorage, key, false); | ||
|
|
||
| // The base composition | ||
| await assertNumberOfCompositions(client, baseGraphName, 1); | ||
|
|
||
| const featureFlagName = genID('flag'); | ||
| await createFeatureFlag(client, featureFlagName, labels, ['users-feature', 'products-feature'], 'default', true); | ||
|
|
||
| expect(blobStorage.keys()).toHaveLength(3); | ||
| const ffKey = blobStorage.keys().at(-1); | ||
| expect(ffKey).toContain(`${federatedGraphResponse.graph!.id}/manifest/feature-flags/${featureFlagName}.json`); |
Contributor
There was a problem hiding this comment.
Avoid relying on blob key insertion order in these assertions.
These checks assume blobStorage.keys() always returns latest.json, mapper.json, and the feature-flag manifest in a stable order. If the underlying writes are reordered, these tests will flap. Sort the keys first or assert membership instead of indexing into the raw array.
As per coding guidelines, "Sort collections by a stable key before making positional assertions on non-deterministic order items (metrics, spans, etc.)".
Also applies to: 200-216
🤖 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 `@controlplane/test/feature-flag/recompose-feature-flag.test.ts` around lines
147 - 163, The test assumes blobStorage.keys() returns a stable insertion order
which can flap; update the assertions to avoid positional indexing by either
sorting the keys by name (e.g., alphabetically) before asserting positions or,
better, assert membership of the expected paths instead of relying on indexes.
Locate the checks around blobStorage.keys() and replace usages that do
keys()[0], keys()[1], or keys().at(-1) with a stable approach (sort the array or
use array.includes) when asserting the presence of
`${federatedGraphResponse.graph!.id}/manifest/latest.json`,
`${federatedGraphResponse.graph!.id}/manifest/mapper.json`, and the feature-flag
path
`${federatedGraphResponse.graph!.id}/manifest/feature-flags/${featureFlagName}.json`
(these appear alongside assertFeatureFlagExecutionConfig,
assertNumberOfCompositions, and createFeatureFlag).
Comment on lines
+204
to
+272
| newVersion := computeCompositeVersion(activeGraphs) | ||
| if newVersion == p.latestVersion { | ||
| p.logger.Debug("No changes detected in engine config, keeping existing config") | ||
| return | ||
| } | ||
|
|
||
| p.logger.Info("Router execution config has changed, hot reloading server", | ||
| zap.String("old_version", p.latestVersion), | ||
| zap.String("new_version", newVersion), | ||
| zap.String("fetch_time", time.Since(fetchStart).String()), | ||
| ) | ||
|
|
||
| // Determine what changed, was added, or was removed. | ||
| changes := routerconfig.Changes{ | ||
| AddedConfigs: make(map[string]struct{}), | ||
| RemovedConfigs: make(map[string]struct{}), | ||
| ChangedConfigs: make(map[string]struct{}), | ||
| } | ||
|
|
||
| for name, hash := range activeGraphs { | ||
| if oldHash, exists := p.knownHashes[name]; !exists { | ||
| changes.AddedConfigs[name] = struct{}{} | ||
| } else if oldHash != hash { | ||
| changes.ChangedConfigs[name] = struct{}{} | ||
| } | ||
| } | ||
| for name := range p.knownHashes { | ||
| if _, exists := activeGraphs[name]; !exists { | ||
| changes.RemovedConfigs[name] = struct{}{} | ||
| } | ||
| } | ||
|
|
||
| // Clone the in-use config before mutating. | ||
| patched := proto.Clone(p.currentConfig).(*nodev1.RouterConfig) | ||
|
|
||
| // Apply changes and additions. | ||
| toFetch := make(map[string]struct{}, len(changes.ChangedConfigs)+len(changes.AddedConfigs)) | ||
| maps.Copy(toFetch, changes.ChangedConfigs) | ||
| maps.Copy(toFetch, changes.AddedConfigs) | ||
|
|
||
| for name := range toFetch { | ||
| fetchedConfig, err := p.fetcher.FetchConfig(ctx, name) | ||
| if err != nil { | ||
| p.logger.Error("Failed to fetch config, skipping entire update", | ||
| zap.String("name", name), | ||
| zap.Error(err), | ||
| ) | ||
| return | ||
| } | ||
|
|
||
| if name == "" { | ||
| // Base graph update. | ||
| patched.EngineConfig = fetchedConfig.EngineConfig | ||
| patched.Version = fetchedConfig.Version | ||
| patched.Subgraphs = fetchedConfig.Subgraphs | ||
| patched.CompatibilityVersion = fetchedConfig.CompatibilityVersion | ||
| } else { | ||
| if patched.FeatureFlagConfigs == nil { | ||
| patched.FeatureFlagConfigs = &nodev1.FeatureFlagRouterExecutionConfigs{ | ||
| ConfigByFeatureFlagName: make(map[string]*nodev1.FeatureFlagRouterExecutionConfig), | ||
| } | ||
| } | ||
| patched.FeatureFlagConfigs.ConfigByFeatureFlagName[name] = &nodev1.FeatureFlagRouterExecutionConfig{ | ||
| EngineConfig: fetchedConfig.EngineConfig, | ||
| Version: fetchedConfig.Version, | ||
| Subgraphs: fetchedConfig.Subgraphs, | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Honor ConfigRules during polling too.
GetRouterConfig() filters ignored/missing feature-flag configs, but Subscribe() diffs against the raw mapper and blindly refetches every changed entry. After the first reload, an ignored flag can be inserted back into patched.FeatureFlagConfigs, and SkipMissingFeatureFlags stops working because ErrFileNotFound now aborts the whole update. Apply the same filtering/skip logic before computeCompositeVersion, changes, and FetchConfig in the polling path.
🤖 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 `@router/pkg/controlplane/configpoller/split_config_poller.go` around lines 204
- 272, The polling path must apply the same ConfigRules filtering as
GetRouterConfig/Subscribe: before calling computeCompositeVersion(activeGraphs)
and before computing changes and building toFetch, filter activeGraphs using
p.configRules (honor ignored flags and SkipMissingFeatureFlags behavior), and
when FetchConfig(name) returns ErrFileNotFound and SkipMissingFeatureFlags is
set, skip that name instead of returning early; ensure when applying
fetchedConfig into patched.FeatureFlagConfigs you only insert entries that pass
the config rules. Update logic around computeCompositeVersion, the changes loop,
toFetch population, and the FetchConfig error handling (references:
computeCompositeVersion(activeGraphs), p.knownHashes, p.currentConfig,
p.fetcher.FetchConfig, ErrFileNotFound, SkipMissingFeatureFlags) to perform this
filtering so ignored/missing feature-flag configs are not reintroduced during
poll reloads.
Comment on lines
+85
to
+86
| target := f.cdnURL.ResolveReference(&url.URL{Path: path}) | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go's net/url package, when ResolveReference is given a reference URL whose Path starts with "/", does it replace the base URL's existing path prefix?
💡 Result:
Yes, when ResolveReference is given a reference URL whose Path starts with "/", it replaces the base URL's existing path prefix. This follows RFC 3986 rules for absolute paths (abs_path case). In the net/url source code, for a reference without scheme/host/user (treated as abs_path or rel_path), it calls url.setPath(resolvePath(u.EscapedPath, ref.EscapedPath)) [1][2][3]. The resolvePath function explicitly checks: if ref == '/', then full = ref, using the reference's path directly and ignoring the base path [2]. Examples confirming this: - base "http://example.com/directory/", ref "../../..//search?q=dotnet" resolves to "http://example.com/search?q=dotnet", navigating up past the directory prefix [4]. - base "http://foo/a/b/c", ref "/d/e" resolves to "http://foo/d/e", replacing the entire /a/b/c prefix [5]. This behavior is consistent across Go versions, including recent ones like go1.26.0 [2], and matches standard URL resolution where an absolute path replaces the base path.
Citations:
- 1: https://github.com/golang/go/blob/master/src/net/url/url.go
- 2: https://github.com/golang/go/blob/go1.26.0/src/net/url/url.go
- 3: https://go.googlesource.com/go/+/go1.12.8/src/net/url/url.go
- 4: https://stackoverflow.com/questions/53692326/convert-relative-to-absolute-urls-in-go
- 5: https://stackoverflow.com/questions/34668012/combine-url-paths-with-path-join
🏁 Script executed:
find . -name "split_fetcher.go" -type fRepository: wundergraph/cosmo
Length of output: 108
🏁 Script executed:
cat -n ./router/pkg/routerconfig/cdn/split_fetcher.go | head -100Repository: wundergraph/cosmo
Length of output: 3765
🏁 Script executed:
cat -n ./router/pkg/routerconfig/cdn/split_fetcher.go | sed -n '75,230p'Repository: wundergraph/cosmo
Length of output: 5933
🏁 Script executed:
# Check if there are any tests or usage examples that might show expected behavior
rg "NewSplitFetcher" --context 3 -t goRepository: wundergraph/cosmo
Length of output: 5289
🏁 Script executed:
cat -n ./router/pkg/routerconfig/cdn/split_fetcher_test.go | head -150Repository: wundergraph/cosmo
Length of output: 5426
🏁 Script executed:
rg "FetchMapper\|FetchConfig" ./router/pkg/routerconfig/cdn/split_fetcher_test.go -A 15 | head -100Repository: wundergraph/cosmo
Length of output: 43
🏁 Script executed:
cat -n ./router/pkg/routerconfig/cdn/split_fetcher_test.go | sed -n '150,300p'Repository: wundergraph/cosmo
Length of output: 5992
🏁 Script executed:
cat -n ./router/pkg/routerconfig/cdn/split_fetcher_test.go | sed -n '275,290p'Repository: wundergraph/cosmo
Length of output: 675
🏁 Script executed:
rg "cdn\.example\.com" ./router/pkg/routerconfig/cdn/split_fetcher_test.go -B 2 -A 2Repository: wundergraph/cosmo
Length of output: 806
🏁 Script executed:
cat -n ./router/pkg/routerconfig/cdn/split_fetcher_test.go | sed -n '350,500p'Repository: wundergraph/cosmo
Length of output: 3377
Preserve any path prefix in the configured CDN endpoint.
When ResolveReference receives a Path starting with /, it treats it as root-relative per RFC 3986 and replaces the base URL's entire path. A deployment configured with https://cdn.example.com/router-config will request /org/... instead of /router-config/org/..., causing fetch failures.
Use url.JoinPath to combine the base URL's path with the request path, or switch to a different resolution method that preserves the base path prefix.
Related code (lines 182–219)
Both FetchMapper and FetchConfig use url.JoinPath("/", ...) to construct paths, then pass them to post, which calls ResolveReference with those absolute paths.
🤖 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 `@router/pkg/routerconfig/cdn/split_fetcher.go` around lines 85 - 86, The CDN
base path prefix is being dropped because ResolveReference is called with an
absolute Path (starting with "/"), so update the code paths built by
FetchMapper/FetchConfig and the post function to preserve the base path: use
url.JoinPath to join f.cdnURL.Path with the request path (e.g., joined :=
url.JoinPath(f.cdnURL.Path, path)) and then construct the target URL from the
base by setting its Path to the joined value (or ResolveReference with a
relative path) instead of passing the absolute path directly to
ResolveReference; adjust the code in post (and callers FetchMapper/FetchConfig)
to use f.cdnURL, url.JoinPath, and the new joined path so the configured CDN
prefix is retained.
Contributor
Author
|
Closed in favor of #2853 |
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
5 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
feature-flag recomposeCLI command for recomposing feature flags on demand.skip_missing_feature_flags,ignored_feature_flags) for controlling split-config polling behavior.Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.