fix(complexity):complexity router keyword removal via config.json (split + source_of_truth) - #4303
Conversation
Contributor
📝 WalkthroughWalkthroughAdds snapshot-aware merging for ComplexityAnalyzerConfig, persists reconciled configs with JSON snapshots in the config store, refactors reconciliation to use snapshots for merge/prune decisions, and updates mocks and tests to validate pruning and persistence behavior. ChangesGovernance Config Snapshot-Aware Reconciliation
Sequence Diagram(s)sequenceDiagram
participant Reconciler as reconcileComplexityAnalyzerConfig
participant Merge as MergeComplexityAnalyzerConfigWithFileSnapshot
participant Store as ConfigStore / RDBConfigStore
participant DB as Database
Reconciler->>Merge: provide base/current + previousFile snapshot
Merge-->>Reconciler: merged config
Reconciler->>Store: ApplyComplexityAnalyzerConfigFromFile(merged, fileSnapshot)
Store->>Store: validate & normalize, ensure ConfigHash
Store->>DB: update typed config (updateComplexityAnalyzerConfigTx)
Store->>DB: write snapshot JSON (ConfigComplexityAnalyzerConfigSnapshotKey)
DB-->>Store: persisted
Store-->>Reconciler: apply success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
Madhuvod
marked this pull request as ready for review
June 11, 2026 16:20
Contributor
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
18 tasks
![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: 5
🤖 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 `@framework/configstore/rdb_test.go`:
- Around line 190-203: Replace the single-case test
TestMergeComplexityAnalyzerConfigWithFileSnapshotRemovesFileOwnedKeywords with a
table-driven test: define a slice of cases (name, currentOverrides,
previousFileOverrides, fileOverrides, expectedCodeKeywords) and for each case
create current/previous/file by calling testComplexityAnalyzerConfig() and
applying the overrides to Keywords.CodeKeywords, then call
MergeComplexityAnalyzerConfigWithFileSnapshot(current, previousFile, file),
require no error, and assert that merged.Keywords.CodeKeywords equals
expectedCodeKeywords; use t.Run(case.name) for isolation and clear names
referencing scenarios (e.g., "removes-file-owned", "keeps-current-only",
"adds-new-file-keywords") to cover multiple input combinations.
- Around line 129-133: The test currently asserts snapshot.Value contains the
substring `"simple_medium":0.1`, which can yield false positives; instead decode
snapshot.Value as JSON and assert the numeric field equals the expected value.
Update the test around the call to store.GetConfig (using
tables.ConfigComplexityAnalyzerConfigSnapshotKey) to json.Unmarshal
snapshot.Value into an appropriate struct or map and assert that the
parsed["simple_medium"] (or struct.SimpleMedium) is exactly 0.1 using require/
assert to fail the test deterministically.
In `@framework/configstore/rdb.go`:
- Around line 2303-2311: The DeleteConfig and similar methods in RDBConfigStore
(e.g., UpdateConfig) currently only check len(tx) > 0 and will dereference a nil
*gorm.DB if caller passes an explicit nil; update these functions to guard the
variadic transaction by checking both len(tx) > 0 and tx[0] != nil, and if tx[0]
is nil fall back to s.DB(); ensure all usages that call txDB.WithContext(ctx)...
(references: DeleteConfig, UpdateConfig, RDBConfigStore.DB(), and any other
methods at the referenced ranges) use the resulting non-nil txDB to avoid
panics.
In `@transports/bifrost-http/lib/config_test.go`:
- Around line 1009-1030: The mock ApplyComplexityAnalyzerConfigFromFile
implementation can panic or skip validations; update
MockConfigStore.ApplyComplexityAnalyzerConfigFromFile to mirror RDBConfigStore
behavior by: (1) validating inputs and returning an error if config or
fileSnapshot is nil, (2) invoking the same validation/hash logic used by
RDBConfigStore (e.g., call UpdateComplexityAnalyzerConfig or the underlying
validation routine and verify config.ConfigHash is computed/consistent), (3)
ensure fileSnapshot is normalized safely (handle nil) and that ConfigHash
comparisons/assignments mimic the real store before writing entries into
m.configEntries, and (4) preserve the existing call to
UpdateComplexityAnalyzerConfig(ctx, config, tx...) while returning the
appropriate errors when validation/hash checks fail so tests behave like
production paths.
In `@transports/bifrost-http/lib/config.go`:
- Around line 2519-2528: The current helper
hasComplexityAnalyzerConfigFileMetadata() treats any single metadata key
presence as evidence the analyzer was previously applied from config.json, which
leads the sync logic to delete persisted analyzer when the section is omitted;
change the function to only report "file-backed" when the stored metadata is
complete—i.e., both configstoreTables.ConfigComplexityAnalyzerConfigHashKey and
configstoreTables.ConfigComplexityAnalyzerConfigSnapshotKey exist and have
non-empty values—so omission in config.json won't be treated as a delete; apply
the same change to the analogous check around lines 2545-2549 (the other
helper/call site) so both places require both keys to be present before deciding
the config came from config.json.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d0b3d0c1-8e9e-4da4-beea-6b75a7792646
📒 Files selected for processing (7)
framework/configstore/complexityconfig.goframework/configstore/rdb.goframework/configstore/rdb_test.goframework/configstore/store.goframework/configstore/tables/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.go
Contributor
Confidence Score: 3/5The reconciliation logic itself is correct, but the pruning path sets in-memory state to nil inside a multi-operation transaction before it commits, which can leave the runtime config and DB out of sync if any later operation in that same transaction fails. The three-way merge, snapshot persistence, and source-of-truth prune are all well-structured and tested. The divergence risk arises specifically in pruneComplexityAnalyzerConfig: it nulls the in-memory field as part of its own execution, yet it runs as the first step of a larger transaction that also handles virtual keys, routing rules, pricing overrides, and other governance collections. A transient failure anywhere after the complexity-config deletes causes a rollback that restores the DB rows but leaves the in-memory pointer at nil. transports/bifrost-http/lib/config.go — specifically pruneComplexityAnalyzerConfig and its call site inside pruneGovernanceConfigToFile's transaction lambda. Important Files Changed
Reviews (1): Last reviewed commit: "fix(complexity):complexity router keywor..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Madhuvod
force-pushed
the
06-11-fix_complexity_complexity_router_keyword_removal_via_config.json_split_source_of_truth_
branch
from
June 11, 2026 16:57
65a24d9 to
c66d703
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
transports/bifrost-http/lib/config.go (1)
2547-2552:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't turn omission of
complexity_analyzer_configinto a delete.This still deletes the stored analyzer when the section is absent but metadata exists. Under
source_of_truth=config.json, omission should leave the persisted analyzer untouched; only sections that are actually present inconfig.jsonare authoritative.
As per coding guidelines,transports/config.schema.jsonis the source of truth andsource_of_truth=config.jsonmakes present governance sections authoritative while missing sections leave DB rows untouched.🤖 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 `@transports/bifrost-http/lib/config.go` around lines 2547 - 2552, The current block treats absence of "complexity_analyzer_config" as a deletion when hasComplexityAnalyzerConfigFileMetadata returns true; instead, do not prune or clear config when the section is missing under source_of_truth=config.json. Remove or change this branch so pruneComplexityAnalyzerConfig(ctx, config, tx) and setting config.GovernanceConfig.ComplexityAnalyzerConfig = nil only run for authoritative removal (i.e., when governanceSectionPresent("complexity_analyzer_config") is true and the config file explicitly indicates deletion), leaving the stored analyzer untouched when the section is merely omitted; locate the logic around governanceSectionPresent, hasComplexityAnalyzerConfigFileMetadata, and pruneComplexityAnalyzerConfig to implement this change.Source: Coding guidelines
🤖 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 `@transports/bifrost-http/lib/config.go`:
- Around line 2409-2431: The file-snapshot write is only forced in one branch,
so change the post-merge apply to force metadata persistence whenever the file
snapshot changed; after computing merged via
mergeComplexityAnalyzerConfigFromFile(current, previousFileConfig, fileConfig)
set a boolean forceMetadataSync := !snapshotFound || (current != nil &&
current.ConfigHash != fileHash) and pass that boolean as the last argument to
applyComplexityAnalyzerConfigFromFile(ctx, config, current, merged, fileConfig,
forceMetadataSync) so a changed config.json always updates the stored snapshot
(use the existing symbols fileHash, current, previousFileConfig, fileConfig,
merged, snapshotFound, mergeComplexityAnalyzerConfigFromFile, and
applyComplexityAnalyzerConfigFromFile).
---
Duplicate comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 2547-2552: The current block treats absence of
"complexity_analyzer_config" as a deletion when
hasComplexityAnalyzerConfigFileMetadata returns true; instead, do not prune or
clear config when the section is missing under source_of_truth=config.json.
Remove or change this branch so pruneComplexityAnalyzerConfig(ctx, config, tx)
and setting config.GovernanceConfig.ComplexityAnalyzerConfig = nil only run for
authoritative removal (i.e., when
governanceSectionPresent("complexity_analyzer_config") is true and the config
file explicitly indicates deletion), leaving the stored analyzer untouched when
the section is merely omitted; locate the logic around governanceSectionPresent,
hasComplexityAnalyzerConfigFileMetadata, and pruneComplexityAnalyzerConfig to
implement this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a4f7263d-75ca-42f9-8bca-86384faa4db1
📒 Files selected for processing (7)
framework/configstore/complexityconfig.goframework/configstore/rdb.goframework/configstore/rdb_test.goframework/configstore/store.goframework/configstore/tables/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.go
…lit + source_of_truth)
Madhuvod
force-pushed
the
06-11-fix_complexity_complexity_router_keyword_removal_via_config.json_split_source_of_truth_
branch
from
June 11, 2026 17:11
c66d703 to
22af525
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
transports/bifrost-http/lib/config.go (2)
2409-2420:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBackfill analyzer snapshot metadata even when the file already matches the DB.
In
source_of_truth=config.json, this still callsapplyComplexityAnalyzerConfigFromFile(..., false). On an upgrade wherecurrentalready equals the normalized file config,applyComplexityAnalyzerConfigFromFileshort-circuits onreflect.DeepEqualand never writes the new snapshot/hash metadata. That leaves later snapshot-aware merges without a previous file snapshot, so removals fromconfig.jsonwill not prune correctly.Suggested fix
if configData.isConfigJSONSourceOfTruth() { fileConfig.ConfigHash = fileHash - applyComplexityAnalyzerConfigFromFile(ctx, config, current, fileConfig, fileConfig, false) + forceMetadataSync := !hasComplexityAnalyzerConfigFileMetadata(ctx, config.ConfigStore) + applyComplexityAnalyzerConfigFromFile(ctx, config, current, fileConfig, fileConfig, forceMetadataSync) return } previousFileConfig, snapshotFound := complexityAnalyzerConfigFileSnapshot(ctx, config.ConfigStore) if current != nil && current.ConfigHash == fileHash { - if !snapshotFound { + if !snapshotFound || !hasComplexityAnalyzerConfigFileMetadata(ctx, config.ConfigStore) { applyComplexityAnalyzerConfigFromFile(ctx, config, current, current, fileConfig, true) }🤖 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 `@transports/bifrost-http/lib/config.go` around lines 2409 - 2420, When source_of_truth is config.json, ensure we backfill snapshot/metadata even if the on-disk config already matches DB: call applyComplexityAnalyzerConfigFromFile with the "force backfill" snapshot flag (the same call shape used when current.ConfigHash == fileHash and !snapshotFound) instead of the current applyComplexityAnalyzerConfigFromFile(..., false) short-circuit. In practice, inside the isConfigJSONSourceOfTruth() branch replace the applyComplexityAnalyzerConfigFromFile(ctx, config, current, fileConfig, fileConfig, false) call with a call that passes current as the "existing" config and sets the final boolean to true (e.g., applyComplexityAnalyzerConfigFromFile(ctx, config, current, current, fileConfig, true)) so the function will write the snapshot/hash metadata even when reflect.DeepEqual would otherwise skip it.
2546-2553:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t delete
complexity_analyzer_configjust because the section is omitted.This still prunes the persisted analyzer when the section is absent and metadata exists. Under the documented
source_of_truth=config.jsoncontract, omitted governance sections must leave existing DB rows untouched, so restarting after removing this section will unexpectedly erase the runtime analyzer state.Suggested fix
- shouldPruneComplexityAnalyzerConfig := !configData.governanceSectionPresent("complexity_analyzer_config") && - hasComplexityAnalyzerConfigFileMetadata(ctx, config.ConfigStore) err := config.ConfigStore.ExecuteTransaction(ctx, func(tx *gorm.DB) error { - if shouldPruneComplexityAnalyzerConfig { - if err := pruneComplexityAnalyzerConfig(ctx, config, tx); err != nil { - return err - } - } if configData.governanceSectionPresent("virtual_keys") { keep := make(map[string]bool, len(configData.Governance.VirtualKeys)) @@ - if shouldPruneComplexityAnalyzerConfig { - config.GovernanceConfig.ComplexityAnalyzerConfig = nil - }As per coding guidelines,
source_of_truth="config.json"makes present governance sections authoritative during startup sync, while missing sections leave existing DB rows untouched.Also applies to: 2699-2700
🤖 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 `@transports/bifrost-http/lib/config.go` around lines 2546 - 2553, The current logic sets shouldPruneComplexityAnalyzerConfig when the governance section is absent, which causes persisted analyzer rows to be deleted; change the condition so pruning only occurs when the governance section is present and there is file metadata. Specifically, update the computation of shouldPruneComplexityAnalyzerConfig to use configData.governanceSectionPresent("complexity_analyzer_config") && hasComplexityAnalyzerConfigFileMetadata(ctx, config.ConfigStore) (and make the same change for the analogous check at the other occurrence around lines 2699-2700) so omitted sections do not trigger pruning; keep the existing call to pruneComplexityAnalyzerConfig inside config.ConfigStore.ExecuteTransaction.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@transports/bifrost-http/lib/config.go`:
- Around line 2409-2420: When source_of_truth is config.json, ensure we backfill
snapshot/metadata even if the on-disk config already matches DB: call
applyComplexityAnalyzerConfigFromFile with the "force backfill" snapshot flag
(the same call shape used when current.ConfigHash == fileHash and
!snapshotFound) instead of the current
applyComplexityAnalyzerConfigFromFile(..., false) short-circuit. In practice,
inside the isConfigJSONSourceOfTruth() branch replace the
applyComplexityAnalyzerConfigFromFile(ctx, config, current, fileConfig,
fileConfig, false) call with a call that passes current as the "existing" config
and sets the final boolean to true (e.g.,
applyComplexityAnalyzerConfigFromFile(ctx, config, current, current, fileConfig,
true)) so the function will write the snapshot/hash metadata even when
reflect.DeepEqual would otherwise skip it.
- Around line 2546-2553: The current logic sets
shouldPruneComplexityAnalyzerConfig when the governance section is absent, which
causes persisted analyzer rows to be deleted; change the condition so pruning
only occurs when the governance section is present and there is file metadata.
Specifically, update the computation of shouldPruneComplexityAnalyzerConfig to
use configData.governanceSectionPresent("complexity_analyzer_config") &&
hasComplexityAnalyzerConfigFileMetadata(ctx, config.ConfigStore) (and make the
same change for the analogous check at the other occurrence around lines
2699-2700) so omitted sections do not trigger pruning; keep the existing call to
pruneComplexityAnalyzerConfig inside config.ConfigStore.ExecuteTransaction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 339256b7-2462-40f0-aced-9725b6cd649d
📒 Files selected for processing (7)
framework/configstore/complexityconfig.goframework/configstore/rdb.goframework/configstore/rdb_test.goframework/configstore/store.goframework/configstore/tables/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.go
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
1 participant
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
Briefly explain the purpose of this PR and the problem it solves.
Changes
Type of change
Affected areas
How to test
Describe the steps to validate this change. Include commands and expected outcomes.
If adding new configs or environment variables, document them here.
Screenshots/Recordings
If UI changes, add before/after screenshots or short clips.
Breaking changes
If yes, describe impact and migration instructions.
Related issues
Link related issues and discussions. Example: Closes #123
Security considerations
Note any security implications (auth, secrets, PII, sandboxing, etc.).
Checklist
docs/contributing/README.mdand followed the guidelinesSummary by CodeRabbit