add source of truth flow for config.json - #3968
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a configurable ChangesSource-of-Truth Config Reconciliation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Confidence Score: 4/5Safe to merge for the default split path; the new config.json SOT mode works correctly for the tested scenarios but has a nuanced interaction in the governance pruning transaction that is worth validating under more complex DB states before broad rollout. The governance pruning transaction in pruneGovernanceConfigToFile deletes teams and customers before it processes budgets and rate-limits. Because DeleteTeam cascades owned budgets via the governance_budgets.team_id FK, and DeleteCustomer explicitly deletes its owned budget/rate-limit rows, those rows can be removed by cascade before the budgets/rate-limits prune block attempts to delete them again — which would return not-found and cause the transaction to fail with logger.Fatal on startup. The bulk-entity integration test avoids this scenario by creating teams/customers with no owned budgets, so the test passes but the failure path remains uncovered. transports/bifrost-http/lib/config.go — specifically pruneGovernanceConfigToFile and the ordering of the teams/customers/budgets/rate-limits prune blocks within its transaction Important Files Changed
Reviews (7): Last reviewed commit: "add source of truth flow for config.json" | Re-trigger Greptile |
23c58ea to
66cf60c
Compare
66cf60c to
e80068e
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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 `@framework/configstore/rdb.go`:
- Around line 3471-3479: The functions (e.g., DeleteTeam, and the similar
DeleteCustomerConfig/DeleteModelConfig blocks flagged) currently dereference
tx[0] after only checking length, which will panic if someone calls the function
with a nil transaction (e.g., DeleteTeam(ctx,id,nil)); modify each function to
explicitly check that when len(tx) > 0 the provided tx[0] is not nil before
using it and return a clear error if it is nil (or alternatively treat nil as
"no transaction" and start a new transaction); ensure txDB := tx[0] is only
assigned after the nil check and update all call sites in those functions (e.g.,
the code paths using txDB.WithContext(ctx) or transaction) accordingly.
In `@transports/bifrost-http/lib/config.go`:
- Around line 1121-1129: The current authoritative sync loop using
isConfigJSONSourceOfTruth() calls processAuthoritativeProvider and, on error,
skips adding that provider into authoritativeProviders which causes the existing
DB provider to be pruned; change the error path so that when
processAuthoritativeProvider returns an error you preserve the existing provider
config by inserting existingCfg (from providersInConfigStore[provider]) into
authoritativeProviders if exists, or alternatively abort the authoritative sync
by returning the error; update the loop around processAuthoritativeProvider (and
the similar block at the later location mentioned) to ensure
authoritativeProviders always contains either the new validated config or the
existingCfg for that provider to avoid accidental deletion of DB-only keys.
- Around line 1717-1761: The delete loop is mistakenly pruning existing MCP
client rows when a corresponding file entry fails validation because keepIDs is
only set after mcpClientConfigToTable and configstore.GenerateMCPClientHash
succeed; fix by marking the matched existing client as kept up-front and/or
skipping the prune pass if any file entry fails: in the file iteration over
fileMCPConfig.ClientConfigs, when you compute existing := existingByName[...] /
existingByID[...] immediately set keepIDs[existing.ID] = true (if existing !=
nil && existing.ID != "") before calling mcpClientConfigToTable or
GenerateMCPClientHash, and additionally track a boolean like hadValidationError
which you set when you hit an error and then guard the later
config.ConfigStore.DeleteMCPClientConfig loop with if !hadValidationError { ...
} to avoid accidental deletions.
- Around line 2333-2340: The unchanged virtual keys path is passing
vk.MCPConfigs with unresolved mcp_client_name entries (MCPClientID==0) into
reconcileVirtualKeyAssociations, which can create or delete wrong associations;
before calling reconcileVirtualKeyAssociations for any virtual key (including
those in the unchanged branch that iterate configData.Governance.VirtualKeys),
ensure you run the same resolution step used for new/changed keys — call
resolveMCPConfigClientIDs (or the equivalent resolution routine) to populate
MCPClientID values on vk.MCPConfigs (using mergeGovernanceConfig’s logic) so
reconcileVirtualKeyAssociations receives fully-resolved MCPClientIDs and does
not create/delete incorrect client-id 0 associations.
🪄 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
Run ID: b7047f89-0de7-4432-b025-a83bbe93fae7
📒 Files selected for processing (6)
framework/configstore/rdb.goframework/configstore/store.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.gotransports/bifrost-http/lib/validator.gotransports/config.schema.json
👮 Files not reviewed due to content moderation or server errors (1)
- transports/bifrost-http/lib/config_test.go
66d1cee to
9c33870
Compare
d7a666c to
52bcb42
Compare
52bcb42 to
4a1a240
Compare
9c33870 to
7d7f118
Compare
7d7f118 to
5236f8f
Compare
4a1a240 to
10c220c
Compare
Merge activity
|
The base branch was changed.
## Summary
Introduces a `source_of_truth` field to `config.json` that allows operators to make config.json sections authoritative during startup reconciliation. When set to `"config.json"`, any section explicitly present in the file becomes the single source of truth — database-only rows for that section are pruned. When omitted or set to `"split"` (the default), existing merge behavior is preserved.
## Changes
- Added `source_of_truth` field to `ConfigData` with two modes: `"split"` (default, existing behavior) and `"config.json"` (file-authoritative).
- Added `presentSections` and `presentGovernanceSections` tracking maps populated during `UnmarshalJSON` so that explicitly-present-but-empty sections (e.g., `"providers": {}`) can be distinguished from absent sections.
- Added `sectionPresent` and `governanceSectionPresent` helpers on `ConfigData` to query section presence.
- Introduced `syncAuthoritativeProvidersInStore` which, under `config.json` mode, deletes DB-only providers and DB-only keys within kept providers inside a single transaction.
- Introduced `processAuthoritativeProvider` as the authoritative counterpart to `processProvider`, preserving runtime-only fields (status, description) from the DB while making the file's key list canonical.
- Introduced `syncMCPConfigFromFile` which replaces stored MCP clients with exactly those declared in config.json, deleting DB-only clients and upserting file clients.
- Introduced `syncPluginsFromFile` which replaces stored plugins with exactly those declared in config.json, deleting DB-only plugins and upserting file plugins within a transaction.
- Introduced `pruneGovernanceConfigToFile` which, after the normal governance merge, removes DB-only rows for each governance collection that was explicitly present in the file (virtual keys, routing rules, pricing overrides, model configs, teams, customers, providers, budgets, rate limits).
- Added `source_of_truth` to `config.schema.json` as an enum of `["split", "config.json"]` with schema validation.
- Added a schema candidate path for tests running from `transports/bifrost-http/lib/`.
- Fixed `MockConfigStore.DeleteMCPClientConfig` and `DeletePlugin` to actually remove entries so sync tests can assert on store state.
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [ ] UI (React)
- [ ] Docs
## How to test
```sh
go test ./transports/bifrost-http/lib/... -run TestConfigDataSourceOfTruth
go test ./transports/bifrost-http/lib/... -run TestSourceOfTruthConfigJSON
go test ./transports/bifrost-http/lib/... -run TestSQLite_SourceOfTruthConfigJSON
go test ./transports/bifrost-http/lib/... -run TestConfigSchemaSourceOfTruthValidation
go test ./transports/bifrost-http/lib/...
```
**New `config.json` field:**
| Field | Type | Values | Default |
|---|---|---|---|
| `source_of_truth` | `string` | `"split"`, `"config.json"` | `"split"` |
To enable authoritative mode, add to `config.json`:
```json
{
"source_of_truth": "config.json",
"providers": { ... },
"governance": { "budgets": [...] }
}
```
Only sections explicitly present in the file will be pruned in the database. Sections omitted from the file leave database rows untouched regardless of mode.
## Breaking changes
- [ ] Yes
- [x] No
Default behavior (`"split"`) is unchanged. Operators must explicitly opt in to `"config.json"` mode.
## Related issues
## Security considerations
Provider API keys that exist only in the database will be permanently deleted when `source_of_truth: "config.json"` is set and the `providers` section is present in the file. Operators should ensure all required keys are declared in config.json before enabling this mode.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
## Summary
Introduces a `source_of_truth` field to `config.json` that allows operators to make config.json sections authoritative during startup reconciliation. When set to `"config.json"`, any section explicitly present in the file becomes the single source of truth — database-only rows for that section are pruned. When omitted or set to `"split"` (the default), existing merge behavior is preserved.
## Changes
- Added `source_of_truth` field to `ConfigData` with two modes: `"split"` (default, existing behavior) and `"config.json"` (file-authoritative).
- Added `presentSections` and `presentGovernanceSections` tracking maps populated during `UnmarshalJSON` so that explicitly-present-but-empty sections (e.g., `"providers": {}`) can be distinguished from absent sections.
- Added `sectionPresent` and `governanceSectionPresent` helpers on `ConfigData` to query section presence.
- Introduced `syncAuthoritativeProvidersInStore` which, under `config.json` mode, deletes DB-only providers and DB-only keys within kept providers inside a single transaction.
- Introduced `processAuthoritativeProvider` as the authoritative counterpart to `processProvider`, preserving runtime-only fields (status, description) from the DB while making the file's key list canonical.
- Introduced `syncMCPConfigFromFile` which replaces stored MCP clients with exactly those declared in config.json, deleting DB-only clients and upserting file clients.
- Introduced `syncPluginsFromFile` which replaces stored plugins with exactly those declared in config.json, deleting DB-only plugins and upserting file plugins within a transaction.
- Introduced `pruneGovernanceConfigToFile` which, after the normal governance merge, removes DB-only rows for each governance collection that was explicitly present in the file (virtual keys, routing rules, pricing overrides, model configs, teams, customers, providers, budgets, rate limits).
- Added `source_of_truth` to `config.schema.json` as an enum of `["split", "config.json"]` with schema validation.
- Added a schema candidate path for tests running from `transports/bifrost-http/lib/`.
- Fixed `MockConfigStore.DeleteMCPClientConfig` and `DeletePlugin` to actually remove entries so sync tests can assert on store state.
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [ ] UI (React)
- [ ] Docs
## How to test
```sh
go test ./transports/bifrost-http/lib/... -run TestConfigDataSourceOfTruth
go test ./transports/bifrost-http/lib/... -run TestSourceOfTruthConfigJSON
go test ./transports/bifrost-http/lib/... -run TestSQLite_SourceOfTruthConfigJSON
go test ./transports/bifrost-http/lib/... -run TestConfigSchemaSourceOfTruthValidation
go test ./transports/bifrost-http/lib/...
```
**New `config.json` field:**
| Field | Type | Values | Default |
|---|---|---|---|
| `source_of_truth` | `string` | `"split"`, `"config.json"` | `"split"` |
To enable authoritative mode, add to `config.json`:
```json
{
"source_of_truth": "config.json",
"providers": { ... },
"governance": { "budgets": [...] }
}
```
Only sections explicitly present in the file will be pruned in the database. Sections omitted from the file leave database rows untouched regardless of mode.
## Breaking changes
- [ ] Yes
- [x] No
Default behavior (`"split"`) is unchanged. Operators must explicitly opt in to `"config.json"` mode.
## Related issues
## Security considerations
Provider API keys that exist only in the database will be permanently deleted when `source_of_truth: "config.json"` is set and the `providers` section is present in the file. Operators should ensure all required keys are declared in config.json before enabling this mode.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable
## Summary
Introduces a `source_of_truth` field to `config.json` that allows operators to make config.json sections authoritative during startup reconciliation. When set to `"config.json"`, any section explicitly present in the file becomes the single source of truth — database-only rows for that section are pruned. When omitted or set to `"split"` (the default), existing merge behavior is preserved.
## Changes
- Added `source_of_truth` field to `ConfigData` with two modes: `"split"` (default, existing behavior) and `"config.json"` (file-authoritative).
- Added `presentSections` and `presentGovernanceSections` tracking maps populated during `UnmarshalJSON` so that explicitly-present-but-empty sections (e.g., `"providers": {}`) can be distinguished from absent sections.
- Added `sectionPresent` and `governanceSectionPresent` helpers on `ConfigData` to query section presence.
- Introduced `syncAuthoritativeProvidersInStore` which, under `config.json` mode, deletes DB-only providers and DB-only keys within kept providers inside a single transaction.
- Introduced `processAuthoritativeProvider` as the authoritative counterpart to `processProvider`, preserving runtime-only fields (status, description) from the DB while making the file's key list canonical.
- Introduced `syncMCPConfigFromFile` which replaces stored MCP clients with exactly those declared in config.json, deleting DB-only clients and upserting file clients.
- Introduced `syncPluginsFromFile` which replaces stored plugins with exactly those declared in config.json, deleting DB-only plugins and upserting file plugins within a transaction.
- Introduced `pruneGovernanceConfigToFile` which, after the normal governance merge, removes DB-only rows for each governance collection that was explicitly present in the file (virtual keys, routing rules, pricing overrides, model configs, teams, customers, providers, budgets, rate limits).
- Added `source_of_truth` to `config.schema.json` as an enum of `["split", "config.json"]` with schema validation.
- Added a schema candidate path for tests running from `transports/bifrost-http/lib/`.
- Fixed `MockConfigStore.DeleteMCPClientConfig` and `DeletePlugin` to actually remove entries so sync tests can assert on store state.
## Type of change
- [ ] Bug fix
- [x] Feature
- [ ] Refactor
- [ ] Documentation
- [ ] Chore/CI
## Affected areas
- [x] Core (Go)
- [x] Transports (HTTP)
- [ ] Providers/Integrations
- [x] Plugins
- [ ] UI (React)
- [ ] Docs
## How to test
```sh
go test ./transports/bifrost-http/lib/... -run TestConfigDataSourceOfTruth
go test ./transports/bifrost-http/lib/... -run TestSourceOfTruthConfigJSON
go test ./transports/bifrost-http/lib/... -run TestSQLite_SourceOfTruthConfigJSON
go test ./transports/bifrost-http/lib/... -run TestConfigSchemaSourceOfTruthValidation
go test ./transports/bifrost-http/lib/...
```
**New `config.json` field:**
| Field | Type | Values | Default |
|---|---|---|---|
| `source_of_truth` | `string` | `"split"`, `"config.json"` | `"split"` |
To enable authoritative mode, add to `config.json`:
```json
{
"source_of_truth": "config.json",
"providers": { ... },
"governance": { "budgets": [...] }
}
```
Only sections explicitly present in the file will be pruned in the database. Sections omitted from the file leave database rows untouched regardless of mode.
## Breaking changes
- [ ] Yes
- [x] No
Default behavior (`"split"`) is unchanged. Operators must explicitly opt in to `"config.json"` mode.
## Related issues
## Security considerations
Provider API keys that exist only in the database will be permanently deleted when `source_of_truth: "config.json"` is set and the `providers` section is present in the file. Operators should ensure all required keys are declared in config.json before enabling this mode.
## Checklist
- [ ] I read `docs/contributing/README.md` and followed the guidelines
- [x] I added/updated tests where appropriate
- [ ] I updated documentation where needed
- [x] I verified builds succeed (Go and UI)
- [ ] I verified the CI pipeline passes locally if applicable

Summary
Introduces a
source_of_truthfield toconfig.jsonthat allows operators to make config.json sections authoritative during startup reconciliation. When set to"config.json", any section explicitly present in the file becomes the single source of truth — database-only rows for that section are pruned. When omitted or set to"split"(the default), existing merge behavior is preserved.Changes
source_of_truthfield toConfigDatawith two modes:"split"(default, existing behavior) and"config.json"(file-authoritative).presentSectionsandpresentGovernanceSectionstracking maps populated duringUnmarshalJSONso that explicitly-present-but-empty sections (e.g.,"providers": {}) can be distinguished from absent sections.sectionPresentandgovernanceSectionPresenthelpers onConfigDatato query section presence.syncAuthoritativeProvidersInStorewhich, underconfig.jsonmode, deletes DB-only providers and DB-only keys within kept providers inside a single transaction.processAuthoritativeProvideras the authoritative counterpart toprocessProvider, preserving runtime-only fields (status, description) from the DB while making the file's key list canonical.syncMCPConfigFromFilewhich replaces stored MCP clients with exactly those declared in config.json, deleting DB-only clients and upserting file clients.syncPluginsFromFilewhich replaces stored plugins with exactly those declared in config.json, deleting DB-only plugins and upserting file plugins within a transaction.pruneGovernanceConfigToFilewhich, after the normal governance merge, removes DB-only rows for each governance collection that was explicitly present in the file (virtual keys, routing rules, pricing overrides, model configs, teams, customers, providers, budgets, rate limits).source_of_truthtoconfig.schema.jsonas an enum of["split", "config.json"]with schema validation.transports/bifrost-http/lib/.MockConfigStore.DeleteMCPClientConfigandDeletePluginto actually remove entries so sync tests can assert on store state.Type of change
Affected areas
How to test
New
config.jsonfield:source_of_truthstring"split","config.json""split"To enable authoritative mode, add to
config.json:{ "source_of_truth": "config.json", "providers": { ... }, "governance": { "budgets": [...] } }Only sections explicitly present in the file will be pruned in the database. Sections omitted from the file leave database rows untouched regardless of mode.
Breaking changes
Default behavior (
"split") is unchanged. Operators must explicitly opt in to"config.json"mode.Related issues
Security considerations
Provider API keys that exist only in the database will be permanently deleted when
source_of_truth: "config.json"is set and theproviderssection is present in the file. Operators should ensure all required keys are declared in config.json before enabling this mode.Checklist
docs/contributing/README.mdand followed the guidelines