version cut - #4571
Merged
Merged
Conversation
|
|
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (34)
📝 WalkthroughSummary by CodeRabbit
WalkthroughDefers MCP client dialing from ChangesMCP Deferred Client Connection
Canonical Model Migration Rename
v1.5.22 Release — Version Bumps and Changelogs
Sequence Diagram(s)sequenceDiagram
participant Bootstrap
participant Bifrost
participant MCPManager
Bootstrap->>Bifrost: Init(config) — plugins registered
Note over Bifrost,MCPManager: NewMCPManager stores bootClientConfigs, does NOT dial
Bootstrap->>Bootstrap: Register inference routes & plugins
Bootstrap->>Bifrost: ConnectConfiguredMCPClients(ctx)
Bifrost->>MCPManager: ConnectConfiguredClients(ctx)
par for each ClientConfig
MCPManager->>MCPManager: AddClient(cfg)
alt success
MCPManager-->>MCPManager: client Connected
else failure
MCPManager-->>MCPManager: client Disconnected + start ClientHealthMonitor
end
end
MCPManager-->>Bifrost: all goroutines complete
Bifrost-->>Bootstrap: return
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
akshaydeo
marked this pull request as ready for review
June 20, 2026 07:35
Contributor
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (1)
framework/logstore/migrations.go (1)
2941-2978: ⚡ Quick winAdd a comment documenting the migration ID version change.
The migration name was updated to
logs_add_canonical_model_columns_v2, but there's no explanation for why the migration was versioned. Future maintainers reviewing this code won't understand whether:
- The original migration had a bug that required re-running
- The schema change was modified between versions
- The versioning is purely for release tracking
📝 Suggested documentation addition
func migrationAddCanonicalModelColumns(ctx context.Context, db *gorm.DB, logger schemas.Logger) error { + // Migration ID versioned to v2 in release 1.5.22 to [explain reason here, e.g., + // "force re-application after fixing a bug in the original migration" or + // "track which release version applied this schema change"] migrationName := "logs_add_canonical_model_columns_v2"🤖 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 `@framework/logstore/migrations.go` around lines 2941 - 2978, The migration ID in the migrationAddCanonicalModelColumns function was changed to include a v2 version suffix, but there is no explanatory comment documenting why this versioning change was necessary. Add a comment above the migrationName variable assignment that explains the reason for the version change, such as whether the original migration had a bug that required re-running, the schema change was modified between versions, or the versioning is for release tracking purposes. This will help future maintainers understand the migration history and intent.
🤖 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/logstore/migrations.go`:
- Line 262: Add a comment above the migration registration line (where the IDs
array contains "logs_add_canonical_model_columns_v2") explaining that the v2
suffix indicates this is a bugfix re-run for the canonical_model_view migration
and reference the related fix commit or bug description. Additionally, update
the documentation comment for the migrationAddCanonicalModelColumns function
(currently at lines 2937-2940) to clarify that this is a bugfix migration that
forces re-execution on existing databases due to a bug in the original
migration, and explain that the column-existence guards prevent errors on
systems that already have the columns.
---
Nitpick comments:
In `@framework/logstore/migrations.go`:
- Around line 2941-2978: The migration ID in the
migrationAddCanonicalModelColumns function was changed to include a v2 version
suffix, but there is no explanatory comment documenting why this versioning
change was necessary. Add a comment above the migrationName variable assignment
that explains the reason for the version change, such as whether the original
migration had a bug that required re-running, the schema change was modified
between versions, or the versioning is for release tracking purposes. This will
help future maintainers understand the migration history and intent.
🪄 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: CHILL
Plan: Pro Plus
Run ID: b2d9823e-d09b-4a8d-8539-a914a6fe30bd
📒 Files selected for processing (34)
core/bifrost.gocore/changelog.mdcore/internal/mcptests/fixtures.gocore/mcp/interface.gocore/mcp/mcp.gocore/versionframework/changelog.mdframework/logstore/migrations.goframework/versionplugins/compat/changelog.mdplugins/compat/versionplugins/governance/changelog.mdplugins/governance/versionplugins/jsonparser/changelog.mdplugins/jsonparser/versionplugins/logging/changelog.mdplugins/logging/versionplugins/maxim/changelog.mdplugins/maxim/versionplugins/mocker/changelog.mdplugins/mocker/versionplugins/modelcatalogresolver/changelog.mdplugins/modelcatalogresolver/versionplugins/otel/changelog.mdplugins/otel/versionplugins/prompts/changelog.mdplugins/prompts/versionplugins/semanticcache/changelog.mdplugins/semanticcache/versionplugins/telemetry/changelog.mdplugins/telemetry/versiontransports/bifrost-http/server/server.gotransports/changelog.mdtransports/version
akshaydeo
force-pushed
the
06-20-version_cut
branch
from
June 20, 2026 07:53
1f5aea6 to
51780e5
Compare
Contributor
Author
Merge activity
|
akshaydeo
added a commit
that referenced
this pull request
Jun 21, 2026
## Summary MCP client connections are now deferred until after all plugins are registered, ensuring `PreMCPConnectionHook` runs against the complete plugin set. Previously, `NewMCPManager` dialed clients immediately during construction, which meant plugins registered after `Init` (e.g. enterprise plugins) were silently excluded from the hook and the client would only recover on a later health-monitor reconnect cycle. ## Changes - Extracted the parallel client-dialing logic from `NewMCPManager` into a new `ConnectConfiguredClients` method on `MCPManager`. Construction now only stores the boot configs; callers must explicitly invoke `ConnectConfiguredClients` when ready. - Added `ConnectConfiguredMCPClients` on `Bifrost` as the public entry point, delegating to `MCPManager.ConnectConfiguredClients` when MCP is configured. - Added `ConnectConfiguredClients` to `MCPManagerInterface` to keep the interface consistent. - In the HTTP server's `Bootstrap`, `ConnectConfiguredMCPClients` is called after all inference routes (and therefore all plugins) are registered. - Updated the MCP test fixture helper `setupMCPManager` to call `ConnectConfiguredClients` explicitly after construction. - Renamed the `logs_add_canonical_model_columns` migration to `logs_add_canonical_model_columns_v2` to fix a previously broken migration. ## Type of change - [ ] Bug fix - [ ] Feature - [x] Refactor - [ ] Documentation - [ ] Chore/CI ## Affected areas - [x] Core (Go) - [x] Transports (HTTP) - [ ] Providers/Integrations - [x] Plugins - [ ] UI (React) - [ ] Docs ## How to test ```sh # Core/Transports go version go test ./... go test ./core/internal/mcptests/... ``` 1. Configure one or more MCP clients in `MCPConfig.ClientConfigs` alongside at least one plugin that implements `PreMCPConnectionHook`. 2. Start the HTTP server and confirm the hook is invoked for each configured client during `Bootstrap` rather than during `Init`. 3. Simulate a connection failure for a boot client and verify the client is retained in `Disconnected` state and the health monitor recovers it automatically. ## Breaking changes - [x] Yes - [ ] No Callers that construct `MCPManager` directly via `NewMCPManager` must now call `manager.ConnectConfiguredClients(ctx)` explicitly after construction. The HTTP server transport handles this automatically. Any custom transport or embedding that relied on auto-connect during `NewMCPManager` will need to add this call. ## Related issues Closes #4556 (Anthropic duplicate `message_start` stream event, included in changelog) ## Security considerations No new auth surfaces or PII handling introduced. The change only affects the timing of MCP client connection establishment. ## Checklist - [x] 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) - [x] I verified the CI pipeline passes locally if applicable
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
2 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
MCP client connections are now deferred until after all plugins are registered, ensuring
PreMCPConnectionHookruns against the complete plugin set. Previously,NewMCPManagerdialed clients immediately during construction, which meant plugins registered afterInit(e.g. enterprise plugins) were silently excluded from the hook and the client would only recover on a later health-monitor reconnect cycle.Changes
NewMCPManagerinto a newConnectConfiguredClientsmethod onMCPManager. Construction now only stores the boot configs; callers must explicitly invokeConnectConfiguredClientswhen ready.ConnectConfiguredMCPClientsonBifrostas the public entry point, delegating toMCPManager.ConnectConfiguredClientswhen MCP is configured.ConnectConfiguredClientstoMCPManagerInterfaceto keep the interface consistent.Bootstrap,ConnectConfiguredMCPClientsis called after all inference routes (and therefore all plugins) are registered.setupMCPManagerto callConnectConfiguredClientsexplicitly after construction.logs_add_canonical_model_columnsmigration tologs_add_canonical_model_columns_v2to fix a previously broken migration.Type of change
Affected areas
How to test
MCPConfig.ClientConfigsalongside at least one plugin that implementsPreMCPConnectionHook.Bootstraprather than duringInit.Disconnectedstate and the health monitor recovers it automatically.Breaking changes
Callers that construct
MCPManagerdirectly viaNewMCPManagermust now callmanager.ConnectConfiguredClients(ctx)explicitly after construction. The HTTP server transport handles this automatically. Any custom transport or embedding that relied on auto-connect duringNewMCPManagerwill need to add this call.Related issues
Closes #4556 (Anthropic duplicate
message_startstream event, included in changelog)Security considerations
No new auth surfaces or PII handling introduced. The change only affects the timing of MCP client connection establishment.
Checklist
docs/contributing/README.mdand followed the guidelines