refactor: per user oauth cleanups

[Pratham-Mishra04]

Summary

Consolidates the persistence of discovered MCP tools into the initial CreateMCPClientConfig call during the OAuth completion flow, eliminating the separate UpdateMCPClientDiscoveredTools method and its redundant second database write.

Changes

• Removed UpdateMCPClientDiscoveredTools from the ConfigStore interface and its RDBConfigStore implementation.
• Added GetMCPClientConfigByID to the ConfigStore interface and implemented it in RDBConfigStore, returning a fully populated schemas.MCPClientConfig including DiscoveredTools and DiscoveredToolNameMapping.
• In completeMCPClientOAuth, discovered tools are now attached to mcpClientConfig before calling CreateMCPClientConfig, so the DB row is written with tools in a single operation rather than two sequential writes.
• CreateMCPClientConfig now explicitly carries DiscoveredTools and DiscoveredToolNameMapping from the original (pre-deep-copy) config to avoid losing those fields, which are tagged json:"-" and therefore dropped during deep copy.
• Updated MockConfigStore in tests to reflect the interface changes.

Type of change

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

Affected areas

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

How to test

Complete a per-user OAuth flow for an MCP client and verify:

1. The MCP client row in the database contains the correct discovered_tools_json and tool_name_mapping_json immediately after creation.
2. Restarting the service still loads the discovered tools correctly from the persisted row.
3. No second DB update for discovered tools is issued after client creation.

go test ./...

Breaking changes

• Yes
• No

UpdateMCPClientDiscoveredTools has been removed from the ConfigStore interface. Any custom implementations of ConfigStore must remove this method and add GetMCPClientConfigByID.

Related issues

Security considerations

No new auth, secrets, or PII handling introduced. The change reduces the window where a restart between the two previous DB writes could leave a client row without its discovered tools.

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

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a78a62b2-b322-4aa3-a7c2-c29e2e0ff0e3

📥 Commits

Reviewing files that changed from the base of the PR and between 98f47a8 and 3c87ff9.

📒 Files selected for processing (5)

• core/schemas/mcp.go
• framework/configstore/rdb.go
• framework/configstore/store.go
• transports/bifrost-http/handlers/mcp.go
• transports/bifrost-http/lib/config_test.go

---

📝 Walkthrough

Summary by CodeRabbit

• Refactor
  • MCP client configuration now persists discovered tools as part of initial creation instead of via a separate update, and the config store API was adjusted to support retrieving full client configs by ID.
• Tests
  • Test mocks updated to reflect the new retrieval path and removed the separate discovered-tools update mock.

Walkthrough

Discovered tools are now attached to MCP client configs before the initial DB persistence. The ConfigStore interface adds GetMCPClientConfigByID and removes UpdateMCPClientDiscoveredTools. RDB store and tests updated accordingly; OAuth handler persists tools on create and no longer performs a separate tools update.

Changes

Cohort / File(s)	Summary
Schema formatting core/schemas/mcp.go	Identifier and alignment/whitespace adjustments for OAuth error vars and MCPClientConfig field alignment only — no API, tag, type, or behavior changes.
Config store interface framework/configstore/store.go	Interface updated: added GetMCPClientConfigByID(ctx, id); removed UpdateMCPClientDiscoveredTools(...).
Config store implementation framework/configstore/rdb.go	Added GetMCPClientConfigByID implementation; CreateMCPClientConfig now persists DiscoveredTools and DiscoveredToolNameMapping at creation; removed targeted UpdateMCPClientDiscoveredTools method.
HTTP handler & tests transports/bifrost-http/handlers/mcp.go, transports/bifrost-http/lib/config_test.go	OAuth completion attaches discovered tools/mapping to mcpClientConfig before CreateMCPClientConfig; mock updated to provide GetMCPClientConfigByID and to remove UpdateMCPClientDiscoveredTools.

Sequence Diagram(s)

sequenceDiagram
    participant Handler as HTTP Handler
    participant ConfigStore as Config Store
    participant MCPMgr as MCP Manager

    rect rgba(100, 150, 255, 0.5)
    Note over Handler,ConfigStore: Old Flow: create then update tools
    Handler->>ConfigStore: CreateMCPClientConfig(configWithoutTools)
    ConfigStore->>ConfigStore: Persist config row
    Handler->>ConfigStore: UpdateMCPClientDiscoveredTools(id, tools)
    ConfigStore->>ConfigStore: Persist discovered tools fields
    Handler->>MCPMgr: SetClientTools(id, tools)
    end

    rect rgba(100, 255, 150, 0.5)
    Note over Handler,ConfigStore: New Flow: include tools on create
    Handler->>Handler: Attach DiscoveredTools to config
    Handler->>ConfigStore: CreateMCPClientConfig(configWithTools)
    ConfigStore->>ConfigStore: Persist config including tools
    Handler->>MCPMgr: SetClientTools(id, tools)
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• refactor: per user oauth cleanups #2842: Mirrors the same changes—schema identifier alignment, add GetMCPClientConfigByID, remove UpdateMCPClientDiscoveredTools, persist discovered tools at creation, and corresponding handler/test updates.

Suggested reviewers

• akshaydeo
• danpiths

Poem

🐰 I hopped through configs, found tools in bloom,

Tucked them in early, saved a later gloom.
One tidy save, no extra chase,
Now DB rows wake with tools in place. 🥕

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'refactor: per user oauth cleanups' accurately summarizes the main change—consolidating MCP tool persistence into the initial config creation call and removing the redundant UpdateMCPClientDiscoveredTools method.
Description check	✅ Passed	The description is well-structured, covering summary, detailed changes, type of change, affected areas, testing instructions, breaking changes, and security considerations. All major template sections are completed.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 04-20-fix_mcp_per_oauth_clients_gossip_fixes

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.11.4)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Pratham-Mishra04]

• feat: store VK at OAuth client registration to skip identity selection in consent flow #2843
• refactor: per user oauth cleanups #2842 👈 (View in Graphite)
• adds concurrent map fixes for plugin timer #2835 : 1 other dependent PR (#2836 )
• separate streaming clients for avoiding read timeouts #2832
• separate conenction for migrations #2831
• compilation fixes #2830
• main

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

[greptile-apps]

Confidence Score: 5/5

Safe to merge; all findings are minor style issues that do not affect correctness.

The core fix (carrying json:"-" fields around the deep-copy and consolidating into one DB write) is correct. No data-integrity, security, or reliability concerns were found. Remaining comments are P2: redundant nil guards and an unused interface method.

No files require special attention.

Important Files Changed

Filename	Overview
transports/bifrost-http/handlers/mcp.go	Consolidates per-user OAuth tool discovery into a single CreateMCPClientConfig call; logic is correct but contains redundant ConfigStore nil-guards already covered by the early-return at function entry.
framework/configstore/rdb.go	CreateMCPClientConfig now explicitly copies DiscoveredTools/DiscoveredToolNameMapping from the pre-deep-copy config to avoid losing json:"-" fields; new GetMCPClientConfigByID returns a fully-populated MCPClientConfig but is currently unused.
framework/configstore/store.go	UpdateMCPClientDiscoveredTools removed and GetMCPClientConfigByID added to ConfigStore interface; breaking change documented in PR description.
transports/bifrost-http/lib/config_test.go	MockConfigStore updated to replace UpdateMCPClientDiscoveredTools with GetMCPClientConfigByID stub, keeping the mock in sync with the interface.
core/schemas/mcp.go	No functional changes; DiscoveredTools/DiscoveredToolNameMapping are tagged json:"-" which is the root cause fixed in CreateMCPClientConfig.

_{Reviews (2): Last reviewed commit: "fix: mcp per oauth clients gossip fixes" | Re-trigger Greptile}

[coderabbitai]

🧹 Nitpick comments (1)

framework/configstore/rdb.go (1)

1261-1285: Consider extracting a shared TableMCPClient → schemas.MCPClientConfig mapper.

This field list is now duplicated in GetMCPConfig and here, so future MCP fields can drift between code paths again.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@framework/configstore/rdb.go` around lines 1261 - 1285, The mapping from the
DB client type to schemas.MCPClientConfig is duplicated between
GetMCPClientConfigByID and GetMCPConfig; extract a single helper like
mapTableMCPClientToMCPClientConfig(dbClient *TableMCPClient)
*schemas.MCPClientConfig (or similar) that constructs and returns the
MCPClientConfig (mapping fields such as ClientID→ID, Name, IsCodeModeClient,
ConnectionType, ConnectionString, StdioConfig, AuthType, OauthConfigID,
ToolsToExecute, ToolsToAutoExecute, Headers, AllowedExtraHeaders,
IsPingAvailable, ToolSyncInterval, AllowOnAllVirtualKeys, ToolPricing,
DiscoveredTools, DiscoveredToolNameMapping) and replace the in-place
construction in GetMCPClientConfigByID and GetMCPConfig with a call to that
helper to avoid duplication and drift.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@framework/configstore/rdb.go`:
- Around line 1261-1285: The mapping from the DB client type to
schemas.MCPClientConfig is duplicated between GetMCPClientConfigByID and
GetMCPConfig; extract a single helper like
mapTableMCPClientToMCPClientConfig(dbClient *TableMCPClient)
*schemas.MCPClientConfig (or similar) that constructs and returns the
MCPClientConfig (mapping fields such as ClientID→ID, Name, IsCodeModeClient,
ConnectionType, ConnectionString, StdioConfig, AuthType, OauthConfigID,
ToolsToExecute, ToolsToAutoExecute, Headers, AllowedExtraHeaders,
IsPingAvailable, ToolSyncInterval, AllowOnAllVirtualKeys, ToolPricing,
DiscoveredTools, DiscoveredToolNameMapping) and replace the in-place
construction in GetMCPClientConfigByID and GetMCPConfig with a call to that
helper to avoid duplication and drift.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d954b580-f68a-4823-8288-5a37cc8406d8

📥 Commits

Reviewing files that changed from the base of the PR and between fe043ae and 273d4d1.

📒 Files selected for processing (5)

• core/schemas/mcp.go
• framework/configstore/rdb.go
• framework/configstore/store.go
• transports/bifrost-http/handlers/mcp.go
• transports/bifrost-http/lib/config_test.go

The base branch was changed.

[akshaydeo]

Merge activity

• Apr 20, 6:52 AM UTC: A user started a stack merge that includes this pull request via Graphite.
• Apr 20, 6:52 AM UTC: @akshaydeo merged this pull request with Graphite.