[Pratham-Mishra04]

Summary

Briefly explain the purpose of this PR and the problem it solves.

Changes

• What was changed and why
• Any notable design decisions or trade-offs

Type of change

• Bug fix
• Feature
• Refactor
• Documentation
• Chore/CI

Affected areas

• Core (Go)
• Transports (HTTP)
• Providers/Integrations
• Plugins
• UI (React)
• Docs

How to test

Describe the steps to validate this change. Include commands and expected outcomes.

# Core/Transports
go version
go test ./...

# UI
cd ui
pnpm i || npm i
pnpm test || npm test
pnpm build || npm run build

If adding new configs or environment variables, document them here.

Screenshots/Recordings

If UI changes, add before/after screenshots or short clips.

Breaking changes

• Yes
• No

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

• I read docs/contributing/README.md and followed the guidelines
• I added/updated tests where appropriate
• I updated documentation where needed
• I verified builds succeed (Go and UI)
• I verified the CI pipeline passes locally if applicable

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: maximhq/bifrost
• Branch: 10-26-fix&#95;keep&#95;a&#95;config.json&#95;mcp&#95;client&#95;s&#95;stored&#95;id&#95;when&#95;its&#95;block&#95;declares&#95;another&#95;client&#95;s
• File: .coderabbit.yaml
• Commit: 96626a4
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Team
• Run ID: 6795e852-ae9b-42a4-99df-e7b99ea4bdf8

Commits

Reviewing files that changed from the base of the PR and between 7c61a8e and 96626a4.

Files selected for processing: 1

• transports/bifrost-http/lib/config.go

Limit details: You’ve used all 8 included reviews currently available.

Summary

Summary by CodeRabbit

• Bug Fixes
  • When loading MCP client configuration, a client matched by name retains the ID of the stored client, even if the configuration file specifies a different ID.
  • A file-declared client without a matching stored client receives a generated ID when its configured ID is empty.
  • In configuration-file source-of-truth mode, clients not declared in the file are removed from the active configuration, including when a declared client’s ID conflicts with another stored client.

The MCP config loader now uses the stored ID for a matched client. It warns when the file declares a different non-empty ID. A SQLite-backed regression test checks the retained ID and pruning of another stored client.

Changes: MCP config ID matching

Layer / File(s)	Summary
Preserve the stored ID for name matches transports/bifrost-http/lib/config.go, transports/bifrost-http/lib/config_test.go	For a matched client, the loader uses the stored ID and warns if the file declares a different non-empty ID. The regression test checks that the name-matched client retains its stored ID and that the other stored client is pruned.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: Moderate · up to 96626

Some configurations can lose an MCP client at startup. Resolve the ID collision before merging.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Description check		The description contains only the repository template and placeholder text. It does not explain the bug, implementation, affected area, testing performed, breaking changes, security considerations, or…	Replace the placeholders with completed content. Describe the stored-ID fix, list the changed HTTP transport area, identify the change as a bug fix, document test commands and results such as the relevant Go tests, state whether the change …

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: preserving the stored MCP client ID when config.json declares another client ID.
Linked Issues check		The only directly linked issue, #123 ("Files API Support"), is closed and completed. It supplies background only. No active directly linked target remains, so no linked-issue coding requirements apply…
Out of Scope Changes check		The whole-PR diff (merge base to reviewed head) has two files. transports/bifrost-http/lib/config.go changes only the ID-assignment block in syncMCPConfigFromFile. The change keeps the stored ID for a…
Docstring Coverage		Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…

Full details: Linked Issues check

Explanation

The only directly linked issue, #123 ("Files API Support"), is closed and completed. It supplies background only. No active directly linked target remains, so no linked-issue coding requirements apply. The PR objective is a PR-level fix: a config.json MCP client keeps its stored ID when its block declares another client's ID. That objective is not an issue requirement. The whole-PR diff in transports/bifrost-http/lib/config.go implements the fix. A name-matched client with a stored non-empty ID now takes that stored ID. A conflicting declared ID triggers a warning. A UUID is generated only when no match exists and the declared ID is empty.

Full details: Out of Scope Changes check

Explanation

The whole-PR diff (merge base to reviewed head) has two files. transports/bifrost-http/lib/config.go changes only the ID-assignment block in syncMCPConfigFromFile. The change keeps the stored ID for a name-matched client and adds a warning for a conflicting declared ID. transports/bifrost-http/lib/config_test.go adds one regression test, TestLoadMCPConfig_ConfigJSONKeepsTheStoredIDOfANameMatch. The test loads alpha and beta, then reloads with a block named alpha that declares beta's ID. It asserts that the runtime client keeps id-alpha and its URL, and that id-beta is pruned. The test directly exercises the config.go change. No unrelated change is found in either file.

Full details: Docstring Coverage

Explanation

Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 1 files.

Full details: Description check

Explanation

The description contains only the repository template and placeholder text. It does not explain the bug, implementation, affected area, testing performed, breaking changes, security considerations, or checklist status.

Resolution

Replace the placeholders with completed content. Describe the stored-ID fix, list the changed HTTP transport area, identify the change as a bug fix, document test commands and results such as the relevant Go tests, state whether the change is breaking, address security considerations, and complete the checklist or mark items accurately.

Finishing Touches

Generate docstrings

• Commit to this branch
• Create a new PR

Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Usage-based review receipt

• Mode: Continue automatically
• Reviewed files: 1
• Charged: $0.25
• View usage details

Note

This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. View usage-based billing.

[Pratham-Mishra04]

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.
Learn more

• fix(mcp): refresh a per-call credential once when the upstream rejects it at the handshake #9415
• test(mcp): add e2e harness tests for per-user discovery #9414
• feat(mcp): add prompts, resources, per-identity faults and gates to chaosmock #9413
• feat(mcp): add a Refresh tools action to re-discover a caller's tools from the sessions page #9412
• chore(ui): remove the unused MCP tool selector #9133
• docs(openapi): document the credential block on listed MCP clients #9132
• fix: keep a config.json MCP client's stored id when its block declares another client's #9131 👈 (View in Graphite)
• feat: let a deployment gate the per-user OAuth and headers sweeps #9130
• fix: keep a revoke from being undone by a per-user sign-in that finishes during it #9129
• fix: skip the per-user auth retry when its client was replaced during the call #9128
• feat(mcp): add per-user discovery test fixture MCP server example #9310
• feat(ui/mcp): add registry for extra access grants in MCP server view sheet #9313
• feat(mcp): show per-caller discovery counts in the sessions table #9312
• feat(ui): split the MCP server sheet into a read-only view and a settings editor #9311
• perf(mcp): share one cached per-user discovery copy across callers with identical discovery #9309
• perf(mcp): read each client's discovery once per request for its tools, connect tools and instructions #9134
• fix(logging): skip MCP log entries for sign-in prompts #9051
• feat(mcp): serve per-user discovery clients' tools, prompts and resources per caller on the MCP gateway #9341
• feat(mcp): add per-user tool discovery option for per-user auth clients #9050
• feat(ui/mcp): add per-caller tool discovery option for per-user auth MCP clients #9049
• feat(mcp): list tool names and caller counts for per-user discovery clients in the registry #9048
• feat(mcp): enable per_user_discovery and handle turning it on or off #9047
• refactor(mcp): record pending admin verification explicitly with a stored pending_verification flag #9046
• fix(mcp): delete per-user discoveries along with the credentials they were made with #9045
• feat(mcp): store per-user tool discovery at sign-in and warn in the UI when tools fail to load #9044
• feat(mcp): add per-caller connect tool for clients with per-user discovery #9043
• feat(mcp): add database-backed shared and per-user discovery stores #9042
• feat(mcp): add per-user discovery storage and config flag, rejected until lifecycle lands #9041
• refactor(mcp): move discovered tools and instructions into a pluggable discovery store #9040
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

[greptile-apps]

Confidence Score: 5/5

[Medium impact] This PR appears safe to merge.

_{Reviews (4) · Last reviewed commit: "fix: keep a config.json MCP client's sto..." · Reviewed by Greptile}