complexity router: remove config.json as authoritative as default and merge the keywords + move to hash based reconcilation#4293
Closed
Madhuvod wants to merge 1 commit into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a non-JSON ConfigHash to ComplexityAnalyzerConfig, deterministic hash generation and merge utilities, persists and reads the hash in the config store, implements reconciliation helpers for split vs config.json source_of_truth modes, and updates transport logic, tests, and docs. ChangesComplexity Analyzer Config Hash & Split-Mode Reconciliation
Sequence Diagram(s)sequenceDiagram
participant File as config.json (file)
participant Loader as loadGovernanceConfig
participant Reconciler as reconcileComplexityAnalyzerConfig
participant Runtime as in-memory GovernanceConfig
participant Store as ConfigStore (governance_config)
File->>Loader: provide Governance ComplexityAnalyzerConfig
Loader->>Reconciler: complexityAnalyzerConfigFromFile(file)
Reconciler->>Store: GetComplexityAnalyzerConfig() (reads typed config + stored hash)
Reconciler->>Reconciler: compare fileHash vs stored.ConfigHash
alt source_of_truth == "config.json" or hash differs
Reconciler->>Runtime: merge or replace config (file authoritative)
Reconciler->>Store: UpdateComplexityAnalyzerConfig(merged, write hash)
else hash matches (split mode)
Reconciler->>Runtime: preserve stored/runtime config
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (1 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 |
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/features/governance/complexity-router.mdx`:
- Around line 184-186: Add documentation for the governance.source_of_truth
config: update the config.json example to include the governance.source_of_truth
field (showing valid values "split" and "config.json" and the default "split")
and update the field table to document source_of_truth as a string, optional,
default "split", with a brief description ("Reconciliation mode: 'split'
(default) merges file changes with runtime edits; 'config.json' makes the file
authoritative on every restart"). Reference the governance.source_of_truth key
in the example and the table so users editing config.json directly can see the
JSON path, allowed values, and default.
🪄 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: b1dea81a-03fa-4ac4-9def-0e504a54bef9
📒 Files selected for processing (8)
docs/deployment-guides/helm/governance.mdxdocs/features/governance/complexity-router.mdxframework/configstore/complexityconfig.goframework/configstore/rdb.goframework/configstore/rdb_test.goframework/configstore/tables/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.go
Contributor
Confidence Score: 5/5Safe to merge — the core concurrency fix and transactional two-row write are both implemented correctly, and the new reconciliation logic is well-covered by tests. The two previously-flagged issues are both properly resolved. The remaining comments are about implicit API contracts and monotonic keyword accumulation semantics, neither of which causes incorrect behavior under current usage. No files require special attention, though the keyword-growth semantics in transports/bifrost-http/lib/config.go could use a clarifying comment. Important Files Changed
Reviews (3): Last reviewed commit: "complexity router: remove config.json as..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Madhuvod
force-pushed
the
06-11-complexity_router_remove_config.json_as_authoritative_always_and_merge_the_keywords_move_to_hash_based_reconcilation
branch
from
8502674 to
8c263b3
Compare
Madhuvod
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/deployment-guides/helm/governance.mdx`:
- Line 409: Update the wording to clarify split-mode merge/persistence: state
that source_of_truth: "split" causes Helm-configured keywords to be merged
(unioned) with stored lists rather than replacing them, and replace ambiguous
phrases like "while the rendered file value is unchanged"/"while the file value
is unchanged" with an explicit statement such as "the config.json file on disk
does not reflect runtime UI/API edits; those edits are preserved in runtime
state but not written back to config.json." Also correct the Helm guide setting
name from bifrost.sourceOfTruth to bifrost.governance.sourceOfTruth to match the
governance.* configuration structure.
In `@framework/configstore/complexityconfig.go`:
- Around line 143-151: The merged ComplexityAnalyzerConfig being constructed in
the merged variable omits the file's ConfigHash so callers (e.g.,
UpdateComplexityAnalyzerConfig) never persist
ConfigComplexityAnalyzerConfigHashKey; fix by carrying the
normalizedFile.ConfigHash into the merged struct (assign ConfigHash from
normalizedFile to the new ComplexityAnalyzerConfig) so the merged result
preserves the file hash used by reconciliation.
In `@transports/bifrost-http/lib/config.go`:
- Line 2366: The initial-creation path fails to set
ComplexityAnalyzerConfig.ConfigHash causing the first persisted governance row
to be hashless; update createGovernanceConfigInStore (the code path exercised by
loadGovernanceConfig) to compute the same normalized+hashed value used by
reconcileComplexityAnalyzerConfig and assign it to
ComplexityAnalyzerConfig.ConfigHash before persisting so startup reconciliation
treats the freshly-created config the same as subsequent reconciled rows.
- Around line 2458-2466: The function syncComplexityAnalyzerConfig currently
assigns config.GovernanceConfig.ComplexityAnalyzerConfig = next before
persisting via config.ConfigStore.UpdateComplexityAnalyzerConfig, risking
in-memory/durable inconsistency on write failure; change the flow in
syncComplexityAnalyzerConfig to first check reflect.DeepEqual(current, next),
then call config.ConfigStore.UpdateComplexityAnalyzerConfig(ctx, next) and only
upon a successful return assign config.GovernanceConfig.ComplexityAnalyzerConfig
= next (and propagate or handle the error instead of only logging) so the
durable store is updated before swapping the live config.
🪄 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: 9c5cff17-3730-41a3-af8c-e9aaed27bc2c
📒 Files selected for processing (8)
docs/deployment-guides/helm/governance.mdxdocs/features/governance/complexity-router.mdxframework/configstore/complexityconfig.goframework/configstore/rdb.goframework/configstore/rdb_test.goframework/configstore/tables/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.go
…ge the keywords + move to hash based reconcilation
Madhuvod
force-pushed
the
06-11-complexity_router_remove_config.json_as_authoritative_always_and_merge_the_keywords_move_to_hash_based_reconcilation
branch
from
8c263b3 to
99974c2
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/features/governance/complexity-router.mdx (1)
150-178:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
source_of_truthfield documentation to the config.json tab.The Note at lines 184–186 references
source_of_truth: "split"andsource_of_truth: "config.json"modes, but the config.json example (lines 150–168) and field table (lines 170–178) don't show this field. Readers editing config.json directly won't know the JSON path, valid values, or default.Based on the schema and Helm guide, add:
- In the config.json example (after line 152):
"source_of_truth": "split",- In the field table (after line 178): a row documenting
source_of_truthas string, optional, default"split", with valid values"split"(merge file changes with runtime edits) and"config.json"(file authoritative on restart)Note: This was acknowledged in a previous review comment thread.
🤖 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/features/governance/complexity-router.mdx` around lines 150 - 178, The config.json example is missing the source_of_truth field referenced later; add a "source_of_truth": "split" entry to the JSON example (adjacent to the existing governance.complexity_analyzer_config block) and update the field table to include a row for source_of_truth (type: string, required: No, default: "split", valid values: "split" — merge file changes with runtime edits; "config.json" — file authoritative on restart) so readers editing config.json know the path, valid values, and default.
♻️ Duplicate comments (1)
docs/deployment-guides/helm/governance.mdx (1)
409-409:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCorrect the Helm values path to match the governance namespace.
The instruction to set
bifrost.sourceOfTruth: "config.json"should bebifrost.governance.sourceOfTruth: "config.json"to match thegovernance.*structure used throughout this guide (lines 432–465 show all other governance settings nested underbifrost.governance).🤖 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/deployment-guides/helm/governance.mdx` at line 409, Update the Helm values key to the governance-namespaced setting: replace any use of bifrost.sourceOfTruth with bifrost.governance.sourceOfTruth in the documentation text so it matches the governance.* structure used elsewhere; ensure the example and explanatory sentence use bifrost.governance.sourceOfTruth: "config.json" and adjust any neighboring references in the same paragraph to maintain consistency with other governance settings.
🤖 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.
Outside diff comments:
In `@docs/features/governance/complexity-router.mdx`:
- Around line 150-178: The config.json example is missing the source_of_truth
field referenced later; add a "source_of_truth": "split" entry to the JSON
example (adjacent to the existing governance.complexity_analyzer_config block)
and update the field table to include a row for source_of_truth (type: string,
required: No, default: "split", valid values: "split" — merge file changes with
runtime edits; "config.json" — file authoritative on restart) so readers editing
config.json know the path, valid values, and default.
---
Duplicate comments:
In `@docs/deployment-guides/helm/governance.mdx`:
- Line 409: Update the Helm values key to the governance-namespaced setting:
replace any use of bifrost.sourceOfTruth with bifrost.governance.sourceOfTruth
in the documentation text so it matches the governance.* structure used
elsewhere; ensure the example and explanatory sentence use
bifrost.governance.sourceOfTruth: "config.json" and adjust any neighboring
references in the same paragraph to maintain consistency with other governance
settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4cea6b8c-3004-4d7b-b947-351505d75347
📒 Files selected for processing (8)
docs/deployment-guides/helm/governance.mdxdocs/features/governance/complexity-router.mdxframework/configstore/complexityconfig.goframework/configstore/rdb.goframework/configstore/rdb_test.goframework/configstore/tables/config.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.go
Closed
18 tasks
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
Documentation
New Features
Tests