feat: support OAuth credential rotation (client_id/client_secret) for existing MCP clients via edit flow#3209
Merged
akshaydeo merged 1 commit intoMay 5, 2026
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds edit-time OAuth credential rotation and live header reconnects for MCP clients, enforces immutability of connection_type/auth_type/connection_string/stdio_config, persists pending OAuth updates for interactive completion, and wires manager, config, server, handler, UI, store, DB, and docs to support these flows. ChangesOAuth credential rotation & header reconnect
sequenceDiagram
participant UI as User/UI
participant Backend as MCP Backend
participant Store as Config Store
participant Provider as OAuth Provider
rect rgba(100,150,200,0.5)
Note over UI,Provider: OAuth credential rotation flow
UI->>Backend: Submit update with oauth_config
Backend->>Store: Fetch existing MCP client
Backend->>Backend: Validate immutable fields unchanged
Backend->>Backend: Backfill/validate oauth_config, persist pending update
Backend->>UI: Respond with pending_oauth + authorize_url
UI->>Provider: Open authorize_url (user consents)
Provider->>Backend: Callback with code
Backend->>Provider: Exchange code, obtain tokens / discovered tools
Backend->>Store: Update client row (or rollback on in-memory failure)
Backend->>UI: Return success (credential update complete)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
BearTS
changed the title
client_id/client_secret) for existing MCP clients via edit flow
Merged
18 tasks
Contributor
Author
|
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.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
Author
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
Confidence Score: 3/5Not safe to merge — the new Score capped below 4 by the P1 false-error-return in
Important Files Changed
Reviews (15): Last reviewed commit: "feat: ability to edit prexisting mcp oau..." | Re-trigger Greptile |
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/mcp/connecting-to-servers.mdx (1)
937-937:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix broken image link blocking docs CI.
Line 937 points to
/images/placeholder-mcp-disable-toggle.png, and docs validation is failing on that exact path. Please replace it with an existing asset path or remove the placeholder image reference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/mcp/connecting-to-servers.mdx` at line 937, Replace the broken image reference to "placeholder-mcp-disable-toggle.png" used in the markdown image tag: either swap the source to an existing asset path (e.g., one from the /images/ assets that passes docs validation) or remove the entire image tag if no suitable asset exists; ensure the alt text remains meaningful and run the docs validator to confirm the CI error is resolved.transports/bifrost-http/handlers/mcp.go (1)
825-833:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't return an error after already committing the edit.
UpdateMCPClientConfig,mcpManager.UpdateMCPClient, and the VK assignment writes all run beforeInitiateOAuthFlow/StorePendingMCPClient. If the OAuth setup fails, this handler returns 500 even though the non-OAuth edits were already persisted. The UI only invalidates/refetches MCP clients onpending_oauthorsuccessinui/lib/store/apis/mcpApi.ts:59-95, so this leaves a hidden partial update until a manual refresh. Either initialize/store the OAuth flow first, or roll back the earlier mutations when the OAuth setup fails.Also applies to: 868-1008
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@transports/bifrost-http/handlers/mcp.go` around lines 825 - 833, The handler currently persists req.TableMCPClient (via h.store.ConfigStore.UpdateMCPClientConfig and mcpManager.UpdateMCPClient / VK assignment) before running InitiateOAuthFlow/StorePendingMCPClient, causing partial commits if OAuth fails; change the flow so OAuth setup is completed first (call InitiateOAuthFlow and StorePendingMCPClient and persist the pending_oauth state) and only after those succeed update the final MCP client config (UpdateMCPClientConfig and mcpManager.UpdateMCPClient), or if you prefer to keep the current ordering implement a rollback path: capture the original req.TableMCPClient state, and if InitiateOAuthFlow/StorePendingMCPClient fails call h.store.ConfigStore.UpdateMCPClientConfig to restore the original object and call mcpManager.UpdateMCPClient / clear the VK assignment to revert side effects; reference symbols: UpdateMCPClientConfig, mcpManager.UpdateMCPClient, InitiateOAuthFlow, StorePendingMCPClient, req.TableMCPClient, h.store.ConfigStore.
🧹 Nitpick comments (1)
ui/lib/types/mcp.ts (1)
111-111: ⚡ Quick winNarrow
UpdateMCPClientRequest.oauth_configto rotation fields only.Using full
OAuthConfigon Line 111 permits update payload fields the API doesn’t accept in this flow. Prefer a dedicated type with onlyclient_idand optionalclient_secret.Suggested type narrowing
export interface OAuthConfig { client_id: string; client_secret?: string; // Optional for public clients using PKCE authorize_url?: string; // Optional, will be discovered from server_url if not provided token_url?: string; // Optional, will be discovered from server_url if not provided registration_url?: string; // Optional, for dynamic client registration scopes?: string[]; // Optional, can be discovered server_url?: string; // MCP server URL for OAuth discovery (automatically set from connection_string) } + +export type OAuthRotationConfig = Pick<OAuthConfig, "client_id" | "client_secret">; ... export interface UpdateMCPClientRequest { ... - oauth_config?: OAuthConfig; // Only supported for existing oauth/per_user_oauth clients (client_id/client_secret rotation) + oauth_config?: OAuthRotationConfig; // Only supported for existing oauth/per_user_oauth clients (client_id/client_secret rotation) vk_configs?: MCPVKConfig[]; // When provided, replaces all VK assignments for this MCP client }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/lib/types/mcp.ts` at line 111, Update the UpdateMCPClientRequest type so oauth_config is narrowed to a rotation-only shape instead of the full OAuthConfig: create a new type (e.g., OAuthRotationConfig) containing required client_id and optional client_secret, replace the oauth_config?: OAuthConfig on UpdateMCPClientRequest with oauth_config?: OAuthRotationConfig, and adjust any consumers/serializers that build update payloads to only accept/emit those two fields (client_id, client_secret) to match the API's accepted rotation fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/mcp/connecting-to-servers.mdx`:
- Line 116: Update the sentence that currently states "rotating OAuth
credentials (`client_id` and `client_secret`) while preserving the same client
ID" to explicitly state that the preserved identifier is the MCP client record
ID (the internal MCP client resource identifier), not the OAuth `client_id`;
e.g., replace or append wording so it reads that the OAuth credentials can be
rotated while preserving the MCP client record ID (not the OAuth `client_id`) to
remove ambiguity.
In `@framework/configstore/rdb.go`:
- Around line 1512-1527: The code is marshaling the virtual discovery fields
from clientConfigCopy (which deepCopy removed) so discoveredToolsJSON and
toolNameMappingJSON end up empty; change the serialization to read from
clientConfig.DiscoveredTools and clientConfig.DiscoveredToolNameMapping (not
clientConfigCopy) before assigning discoveredToolsJSON and toolNameMappingJSON
so the refreshed discovery payload is persisted into the updates map/columns
(also apply the same change at the other occurrence around the block referenced
at lines ~1560-1565).
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 784-811: The secret-only OAuth rotation path rejects clients that
lack a persisted connection_string and also fails to supply omitted OAuth
metadata to InitiateOAuthFlow; update the shouldRotateOAuthConfig branch in
addMCPClient (and the similar block near lines handling rotations) to backfill
any missing authorize_url, token_url, registration_url and scopes from the
stored existingOauthConfig (loaded via h.store.ConfigStore.GetOauthConfigByID
when existingConfig.OauthConfigID is present) before calling InitiateOAuthFlow,
and only require existingConfig.ConnectionString when discovery/registration is
actually needed (e.g., when req.OauthConfig requires dynamic registration or
discovery), leaving secret-only rotations valid for manually configured
providers.
- Around line 1329-1341: The code persists mcpClientConfig via
h.store.ConfigStore.CreateMCPClientConfig then calls h.mcpManager.AddMCPClient
but does not remove the DB row if AddMCPClient fails; update this branch so that
on error from h.mcpManager.AddMCPClient you call the config-store delete method
(the same one used in the other create branches, e.g.
ConfigStore.DeleteMCPClientConfig or equivalent) with the created
mcpClientConfig identifier (e.g. mcpClientConfig.Id) before returning, and log a
warning if the delete itself fails; keep the existing error responses to the
caller.
In `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 160-163: The form's initial values in the MCPClientSheet are
creating an optional oauth_config with empty strings which fails schema
validation; change the initial state so oauth_config is omitted or set to
undefined unless the OAuth section is active, or set oauth_config.client_id and
client_secret to undefined (not ""), and ensure the same change is applied where
oauth_config is currently initialized later (the other identical block). Locate
the oauth_config object in this file (the initialValues / form defaults inside
the MCPClientSheet component) and remove the empty-string defaults or
conditionally add oauth_config only when needed so the validator doesn't see
empty strings.
In `@ui/lib/types/schemas.ts`:
- Around line 1117-1122: The oauth_config schema currently allows an empty
string for client_secret; change the validation for the client_secret property
in the oauth_config z.object (referencing oauth_config and client_secret) so
that when a client_secret is provided it must be non-empty (e.g., make
client_secret use a non-empty string validator combined with optionality) to
reject "" while still allowing the field to be omitted.
---
Outside diff comments:
In `@docs/mcp/connecting-to-servers.mdx`:
- Line 937: Replace the broken image reference to
"placeholder-mcp-disable-toggle.png" used in the markdown image tag: either swap
the source to an existing asset path (e.g., one from the /images/ assets that
passes docs validation) or remove the entire image tag if no suitable asset
exists; ensure the alt text remains meaningful and run the docs validator to
confirm the CI error is resolved.
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 825-833: The handler currently persists req.TableMCPClient (via
h.store.ConfigStore.UpdateMCPClientConfig and mcpManager.UpdateMCPClient / VK
assignment) before running InitiateOAuthFlow/StorePendingMCPClient, causing
partial commits if OAuth fails; change the flow so OAuth setup is completed
first (call InitiateOAuthFlow and StorePendingMCPClient and persist the
pending_oauth state) and only after those succeed update the final MCP client
config (UpdateMCPClientConfig and mcpManager.UpdateMCPClient), or if you prefer
to keep the current ordering implement a rollback path: capture the original
req.TableMCPClient state, and if InitiateOAuthFlow/StorePendingMCPClient fails
call h.store.ConfigStore.UpdateMCPClientConfig to restore the original object
and call mcpManager.UpdateMCPClient / clear the VK assignment to revert side
effects; reference symbols: UpdateMCPClientConfig, mcpManager.UpdateMCPClient,
InitiateOAuthFlow, StorePendingMCPClient, req.TableMCPClient,
h.store.ConfigStore.
---
Nitpick comments:
In `@ui/lib/types/mcp.ts`:
- Line 111: Update the UpdateMCPClientRequest type so oauth_config is narrowed
to a rotation-only shape instead of the full OAuthConfig: create a new type
(e.g., OAuthRotationConfig) containing required client_id and optional
client_secret, replace the oauth_config?: OAuthConfig on UpdateMCPClientRequest
with oauth_config?: OAuthRotationConfig, and adjust any consumers/serializers
that build update payloads to only accept/emit those two fields (client_id,
client_secret) to match the API's accepted rotation fields.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6c6c3f6-22b3-45b6-b242-1dc36e2602d7
📒 Files selected for processing (8)
docs/mcp/connecting-to-servers.mdxframework/configstore/rdb.gotransports/bifrost-http/handlers/mcp.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/oauth2Authorizer.tsxui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
BearTS
force-pushed
the
05-04-feat_ability_to_edit_prexisting_mcp_oauth_details
branch
from
0c0da14 to
21faa25
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@transports/bifrost-http/handlers/mcp.go`:
- Around line 727-742: The handler currently rejects any presence of
req.ConnectionString or req.StdioConfig rather than only when they differ from
existingConfig; change the logic in the MCP update block to first perform the
redacted-value/merge equality check (i.e., compare the provided value against
existingConfig.ConnectionString and existingConfig.StdioConfig after applying
the same redaction/placeholder rules used elsewhere) and only call SendError
with the appropriate message when the provided value actually differs; keep the
existing checks for ConnectionType and AuthType as-is and reference the same
symbols (req.ConnectionString, req.StdioConfig, existingConfig.ConnectionString,
existingConfig.StdioConfig) when implementing the conditional.
- Around line 1005-1041: The current branch that handles shouldRotateOAuthConfig
calls InitiateOAuthFlow and StorePendingMCPClient after live mutations, risking
partial updates on failure; move the OAuth initiation and persisting of the
pending config (calls to h.oauthHandler.InitiateOAuthFlow and
h.oauthHandler.StorePendingMCPClient that create flowInitiation and
pendingConfig) to occur before mutating the live MCP client/DB/in-memory
structures (the code that updates existingConfig, in-memory client and VK
assignments), or alternatively implement a rollback path that undoes those live
mutations if InitiateOAuthFlow or StorePendingMCPClient returns an error; ensure
the same flowInitiation.OauthConfigID is used when you later apply the
pendingConfig and that SendJSON still returns the pending_oauth response only
after the pending config is safely stored.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ee0f763-445b-41c5-b928-1fa8d593d9df
📒 Files selected for processing (8)
docs/mcp/connecting-to-servers.mdxframework/configstore/rdb.gotransports/bifrost-http/handlers/mcp.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/oauth2Authorizer.tsxui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (4)
- ui/lib/types/mcp.ts
- docs/mcp/connecting-to-servers.mdx
- ui/lib/store/apis/mcpApi.ts
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/lib/types/schemas.ts
- ui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx
BearTS
force-pushed
the
05-04-feat_ability_to_edit_prexisting_mcp_oauth_details
branch
from
21faa25 to
81283af
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@core/mcp/clientmanager.go`:
- Around line 595-621: Guard against nil newConfig and perform the copy of the
client's ExecutionConfig while still holding the read lock: inside the block
that looks up the client in m.clientMap (under m.mu.RLock()), check if newConfig
== nil and return an error if so, then make a defensive copy (e.g., copy the
ExecutionConfig value into oldConfig/mergedConfig) while the lock is held rather
than after m.mu.RUnlock(); afterwards you can release the lock and proceed to
merge headers (using maps.Clone) and the reconnectingClients logic as before.
In `@transports/bifrost-http/lib/config.go`:
- Around line 4633-4641: The loop over c.MCPConfig.ClientConfigs dereferences cc
without checking for nil and the function currently returns success even if no
client with the given id is found; update the code in the method handling
c.MCPConfig to (1) skip nil entries by checking if cc == nil before accessing
cc.ID or cc.Headers, and (2) detect whether a matching client was found (e.g.,
set a found flag when cc.ID == id) and return an error if no match exists
instead of silently succeeding; ensure you still clone newConfig.Headers into
cc.Headers only after the nil check and when a match is confirmed.
In `@ui/lib/types/schemas.ts`:
- Around line 1119-1120: Tighten the optional OAuth credential schema by
ensuring any present value is trimmed and non-empty: update the client_id and
client_secret schema entries so they use trimmed, non-empty string validation
(e.g., change them to use z.string().trim().min(1, "...").optional() or
equivalent) so that undefined is allowed but "" or whitespace-only values fail
validation; reference the client_id and client_secret fields in
ui/lib/types/schemas.ts when making this change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e85136cb-9b2a-485a-b343-38ab909d7d22
📒 Files selected for processing (13)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/interface.godocs/mcp/connecting-to-servers.mdxframework/configstore/rdb.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/oauth2Authorizer.tsxui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (3)
- docs/mcp/connecting-to-servers.mdx
- framework/configstore/rdb.go
- ui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/lib/types/mcp.ts
- ui/lib/store/apis/mcpApi.ts
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
BearTS
force-pushed
the
05-04-feat_ability_to_edit_prexisting_mcp_oauth_details
branch
from
81283af to
c9358bd
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx`:
- Around line 202-206: The oauth_config currently always emits client_id as a
string, causing an empty "" to be sent during secret-only rotations; change the
construction of oauth_config so client_id is omitted when oauthClientID is falsy
— e.g., only include client_id when oauthClientID is non-empty (or set client_id
to undefined instead of ""), keeping client_secret behavior the same; update the
code around shouldRotateOAuthCredentials / oauth_config / oauthClientID /
oauthClientSecret to build the object conditionally so empty client_id is not
sent.
- Around line 486-493: The handler that clears OAuth credentials currently sets
oauth_config.client_id and oauth_config.client_secret to empty strings (in
onCheckedChange where field.onChange is called), which breaks schema validation;
instead remove those keys from the oauth_config object (or set oauth_config to
undefined if no other fields remain) before calling form.setValue. Concretely:
inside the onCheckedChange checked branch, read the current
form.getValues("oauth_config"), create a new object that omits client_id and
client_secret (or set oauth_config to undefined/null when empty), and call
form.setValue("oauth_config", newObjectOrUndefined) rather than setting the
individual keys to "". Ensure you handle the case where oauth_config was
previously undefined.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a3e0253-1e51-48db-99a4-669181d53688
📒 Files selected for processing (13)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/interface.godocs/mcp/connecting-to-servers.mdxframework/configstore/rdb.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/oauth2Authorizer.tsxui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (2)
- docs/mcp/connecting-to-servers.mdx
- ui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- ui/lib/types/mcp.ts
- ui/lib/store/apis/mcpApi.ts
- transports/bifrost-http/lib/config.go
- core/mcp/clientmanager.go
- framework/configstore/rdb.go
- core/bifrost.go
- transports/bifrost-http/handlers/mcp.go
BearTS
force-pushed
the
05-04-feat_ability_to_edit_prexisting_mcp_oauth_details
branch
from
c9358bd to
f26fbce
Compare
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 `@transports/bifrost-http/lib/config.go`:
- Around line 4623-4650: The method currently succeeds when c.MCPConfig is nil,
leaving the in-memory mirror stale; before or after calling
c.client.UpdateMCPClientConnection ensure c.MCPConfig is present and return an
error if it's nil. Specifically, check c.MCPConfig (protected by c.muMCP) and if
nil return a clear error like "in-memory MCPConfig absent" instead of proceeding
to treat the rotate as successful; keep the existing flow that updates
cc.Headers for the matching entry (loop over c.MCPConfig.ClientConfigs) only
when MCPConfig is non-nil, and retain the found/not-found error path for missing
client IDs (using UpdateMCPClientConnection and muMCP as reference points).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 93829858-dfbe-469f-ae79-2ac03a6d01fb
📒 Files selected for processing (13)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/interface.godocs/mcp/connecting-to-servers.mdxframework/configstore/rdb.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/oauth2Authorizer.tsxui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (3)
- core/mcp/interface.go
- ui/lib/types/schemas.ts
- transports/bifrost-http/handlers/mcp.go
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/mcp/connecting-to-servers.mdx
- transports/bifrost-http/server/server.go
- core/mcp/clientmanager.go
- framework/configstore/rdb.go
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- ui/lib/types/mcp.ts
BearTS
force-pushed
the
05-04-feat_ability_to_edit_prexisting_mcp_oauth_details
branch
from
f26fbce to
7892e73
Compare
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 `@core/mcp/clientmanager.go`:
- Around line 603-644: The snapshot/restore race is caused by releasing m.mu
before the long reconnect; either serialize all per-client mutations with the
same in-flight guard or use a generation/CAS check when restoring. Fix: before
taking oldConfig in UpdateClientConnection, grab the per-client in-flight lock
(use m.reconnectingClients.LoadOrStore(id, true)) and defer Delete(id) so no
other UpdateClient/DisableClient/EnableClient can mutate ExecutionConfig while
reconnect runs; alternatively add a generation integer on the client struct,
copy generation with oldConfig, release m.mu, perform connectToMCPClient, and on
failure re-acquire m.mu and only restore oldConfig if the current client
generation matches the saved generation (increment generation on successful
external mutators). Update other mutators (UpdateClient, DisableClient,
EnableClient) to either honor the in-flight guard (fail/queue if Load shows
in-flight) or bump the generation/CAS so the restore will not overwrite newer
changes.
In `@transports/bifrost-http/lib/config.go`:
- Around line 4623-4656: Preflight the presence of the target client ID in the
in-memory config before attempting the live reconnect: under c.muMCP.RLock()
inspect c.MCPConfig.ClientConfigs for a matching cc.ID == id (skipping nil
entries) and return a not-found error if absent; only if found proceed to call
c.client.UpdateMCPClientConnection(id, newConfig). This ensures
UpdateMCPClientConnection is only attempted when the in-memory MCPConfig
contains the client, avoiding the case where a successful live reconnect later
fails to mirror into memory (symbols: c.MCPConfig, c.muMCP, ClientConfigs,
UpdateMCPClientConnection).
In `@ui/lib/types/mcp.ts`:
- Line 111: The exported field oauth_config?: OAuthConfig on the update payload
is wrong because OAuthConfig requires client_id while update-only secret
rotations should allow client_secret-only shapes; change the type used for
oauth_config to a new, more permissive update-specific type (e.g.,
OAuthConfigUpdate or PartialOAuthConfig) defined in ui/lib/types/ (not reusing
OAuthConfig from ui/lib/types/schemas.ts), and update any callers to use that
type so the frontend contract matches the backend update handler that permits
secret-only rotations.
In `@ui/lib/types/schemas.ts`:
- Around line 1117-1122: The oauth_config object currently allows an empty
object and must be updated to require at least one credential when present:
modify the oauth_config Zod schema (the object with client_id and client_secret)
to use refine or superRefine on that object to check that at least one of
client_id or client_secret is a non-empty string (after trim) and add a
field-level error message (e.g., referencing client_id/client_secret) when both
are missing; keep the existing .optional() behavior so oauth_config may be
absent but, if present, fails validation unless one credential is provided.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c210ae5-9139-4f9d-9711-b0a59704c7f5
📒 Files selected for processing (13)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/interface.godocs/mcp/connecting-to-servers.mdxframework/configstore/rdb.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/oauth2Authorizer.tsxui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (2)
- docs/mcp/connecting-to-servers.mdx
- core/bifrost.go
🚧 Files skipped from review as they are similar to previous changes (6)
- ui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx
- ui/lib/store/apis/mcpApi.ts
- transports/bifrost-http/server/server.go
- framework/configstore/rdb.go
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- transports/bifrost-http/handlers/mcp.go
BearTS
force-pushed
the
05-04-feat_ability_to_edit_prexisting_mcp_oauth_details
branch
from
7892e73 to
3c9428a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/mcp.go (1)
919-937:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve
toolSyncIntervalbefore writing Track 1.Line 920 commits the DB update, but Lines 930-933 can still fail while reading the default interval. That returns 500 after the row has already been mutated, leaving DB and in-memory state diverged without hitting the rollback path. Compute
toolSyncIntervalfirst, or rollbackUpdateMCPClientConfigon that read failure.🤖 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 `@transports/bifrost-http/handlers/mcp.go` around lines 919 - 937, The code updates the DB via h.store.ConfigStore.UpdateMCPClientConfig before resolving toolSyncInterval, so a subsequent GetClientConfig failure can leave DB and memory out of sync; fix by computing toolSyncInterval first (use mcp.DefaultToolSyncInterval, override with req.ToolSyncInterval if set, otherwise call h.store.ConfigStore.GetClientConfig and derive config.MCPToolSyncInterval) and only then call UpdateMCPClientConfig with the fully-resolved value, or alternatively, if you prefer to keep the update-first flow, call UpdateMCPClientConfig inside a transaction or call a compensating rollback of UpdateMCPClientConfig when GetClientConfig returns an error and return that error via SendError to avoid leaving inconsistent state.
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/mcp.go (1)
758-775:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun redacted-value normalization before the immutable-field checks.
Line 768 still compares
req.ConnectionStringagainst the raw stored value, so a caller that echoes the redacted placeholder fromGET /api/mcp/clientswill be rejected beforemergeMCPRedactedValuescan restore the original secret. The same ordering issue exists forstdio_configon Line 772 when it contains redacted env values. Compare the merged/normalized payload instead of the pre-merge request.🤖 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 `@transports/bifrost-http/handlers/mcp.go` around lines 758 - 775, Run mergeMCPRedactedValues on the incoming request before performing immutable-field checks so comparisons use the normalized/merged payload rather than the raw request; specifically call mergeMCPRedactedValues to produce a mergedReq and then compare mergedReq.ConnectionString to existingConfig.ConnectionString and mergedReq.StdioConfig (using stdioConfigsEqual) to existingConfig.StdioConfig (and likewise use mergedReq.ConnectionType/AuthType for those checks) before calling SendError on immutability violations.
🤖 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 `@transports/bifrost-http/handlers/mcp.go`:
- Around line 1103-1114: When UpdateMCPClientConfig(ctx, id, &dbUpdateRecord)
returns an error, do not silently continue; either return a partial-error
response to the caller or rollback the live client to the previous headers
before signaling success. Specifically, in the block around
h.store.ConfigStore.UpdateMCPClientConfig (and the logger.Error call), capture
the DB update error, restore the prior header state on the live MCP client (or
reconnect it using the old headers held before you set dbUpdateRecord) and then
return an appropriate error/partial-success to the API flow instead of falling
through to "success"/"pending_oauth", ensuring the response reflects the
persistence failure.
- Around line 889-895: willAttemptReconnect currently only checks disabled state
and causes header edits for per_user_oauth clients to go through Track 2
(partial_success) even though UpdateMCPClientConnection rejects per-user OAuth
clients; add a guard that disables Track 2 for per-user OAuth clients. Compute
an isPerUserOAuth boolean using the client's auth method (e.g., check
existingConfig.AuthMethod or req.AuthMethod for the value "per_user_oauth") and
change the willAttemptReconnect assignment to: willAttemptReconnect :=
hasHeadersChange && !clientWillBeDisabled && !isPerUserOAuth so headers are
persisted via Track 1 for per-user OAuth clients.
---
Outside diff comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 919-937: The code updates the DB via
h.store.ConfigStore.UpdateMCPClientConfig before resolving toolSyncInterval, so
a subsequent GetClientConfig failure can leave DB and memory out of sync; fix by
computing toolSyncInterval first (use mcp.DefaultToolSyncInterval, override with
req.ToolSyncInterval if set, otherwise call h.store.ConfigStore.GetClientConfig
and derive config.MCPToolSyncInterval) and only then call UpdateMCPClientConfig
with the fully-resolved value, or alternatively, if you prefer to keep the
update-first flow, call UpdateMCPClientConfig inside a transaction or call a
compensating rollback of UpdateMCPClientConfig when GetClientConfig returns an
error and return that error via SendError to avoid leaving inconsistent state.
---
Duplicate comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 758-775: Run mergeMCPRedactedValues on the incoming request before
performing immutable-field checks so comparisons use the normalized/merged
payload rather than the raw request; specifically call mergeMCPRedactedValues to
produce a mergedReq and then compare mergedReq.ConnectionString to
existingConfig.ConnectionString and mergedReq.StdioConfig (using
stdioConfigsEqual) to existingConfig.StdioConfig (and likewise use
mergedReq.ConnectionType/AuthType for those checks) before calling SendError on
immutability violations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15d2947a-e305-46cc-9d6d-b6ae37f33d6f
📒 Files selected for processing (13)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/interface.godocs/mcp/connecting-to-servers.mdxframework/configstore/rdb.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/oauth2Authorizer.tsxui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (4)
- ui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx
- docs/mcp/connecting-to-servers.mdx
- framework/configstore/rdb.go
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- ui/lib/types/mcp.ts
- core/mcp/interface.go
- core/mcp/clientmanager.go
- transports/bifrost-http/server/server.go
BearTS
force-pushed
the
05-04-feat_ability_to_edit_prexisting_mcp_oauth_details
branch
from
3c9428a to
05b1afe
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
transports/bifrost-http/lib/config.go (1)
4653-4671:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn an error when in-memory MCP config is missing after reconnect.
At Line [4653], the function can still return success if
c.MCPConfigbecomes nil in the mirror phase. That can report a successful credential rotation while skipping in-memory synchronization.Suggested patch
c.muMCP.Lock() defer c.muMCP.Unlock() - if c.MCPConfig != nil { - found := false - for _, cc := range c.MCPConfig.ClientConfigs { - if cc == nil { - continue - } - if cc.ID == id { - found = true - if newConfig.Headers != nil { - cc.Headers = maps.Clone(newConfig.Headers) - } - break - } - } - if !found { - return fmt.Errorf("MCP client %s not found in in-memory config after successful reconnect", id) - } + if c.MCPConfig == nil { + return fmt.Errorf("in-memory MCPConfig absent after successful reconnect") + } + found := false + for _, cc := range c.MCPConfig.ClientConfigs { + if cc == nil { + continue + } + if cc.ID == id { + found = true + if newConfig.Headers != nil { + cc.Headers = maps.Clone(newConfig.Headers) + } + break + } + } + if !found { + return fmt.Errorf("MCP client %s not found in in-memory config after successful reconnect", id) } return nil }🤖 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 `@transports/bifrost-http/lib/config.go` around lines 4653 - 4671, The function currently allows a nil c.MCPConfig to slip through the mirror phase and return success; ensure we return an error if the in-memory MCP config is missing after reconnect by adding an explicit nil check after reconnect and before treating the rotation as successful. Specifically, after the reconnect/mirror step, if c.MCPConfig == nil return a descriptive error (e.g., "MCPConfig missing after reconnect for client %s") instead of proceeding to the loop that updates MCPConfig.ClientConfigs; reference the existing symbols c.MCPConfig, MCPConfig.ClientConfigs, cc.ID, id and newConfig.Headers (and use maps.Clone when copying headers) so the update logic only runs when c.MCPConfig is present.core/mcp/clientmanager.go (1)
350-352:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
LoadOrStore(notLoad) for mutator gating to close the TOCTOU window.
DisableClientandUpdateClientcurrently do a pre-check withLoad, but that check is not atomic with the mutation. If reconnect starts immediately after the check, the mutator can still proceed and be overwritten by reconnect rollback state.Suggested patch
func (m *MCPManager) DisableClient(id string) error { // The internal in-process client must never be disabled: if id == BifrostMCPClientKey { return fmt.Errorf("cannot disable internal bifrost client") } - if _, inFlight := m.reconnectingClients.Load(id); inFlight { + if _, inFlight := m.reconnectingClients.LoadOrStore(id, true); inFlight { return fmt.Errorf("reconnect or connection credential update already in progress for MCP client %s", id) } + defer m.reconnectingClients.Delete(id) m.mu.Lock() defer m.mu.Unlock() @@ func (m *MCPManager) UpdateClient(id string, updatedConfig *schemas.MCPClientConfig) error { - if _, inFlight := m.reconnectingClients.Load(id); inFlight { + if _, inFlight := m.reconnectingClients.LoadOrStore(id, true); inFlight { return fmt.Errorf("reconnect or connection credential update already in progress for MCP client %s", id) } + defer m.reconnectingClients.Delete(id) m.mu.Lock() defer m.mu.Unlock()Also applies to: 488-490
🤖 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 `@core/mcp/clientmanager.go` around lines 350 - 352, The pre-check using m.reconnectingClients.Load(...) is racy; replace the Load call with LoadOrStore to atomically detect and claim the id before performing a mutating operation (e.g., in DisableClient and UpdateClient and the other similar block around the reconnect logic), so you both test and set the in-flight marker in one step; if LoadOrStore reports the key was already present, return the same error ("reconnect or connection credential update already in progress for MCP client %s"), otherwise proceed and ensure you delete the marker from m.reconnectingClients when the operation completes or rolls back.
🤖 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 `@ui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx`:
- Around line 228-243: The dialog cancel handlers (onOpenChange, DialogContent
onPointerDownOutside/onEscapeKeyDown) call handleCancel which only clears the
interval but does not stop in-flight async status requests that may later call
onSuccess/onError; update handleCancel to also abort pending requests by
creating and storing an AbortController (or a boolean cancelled flag) when
starting the polling/status fetch, call controller.abort() (or set
cancelled=true) inside handleCancel, and modify the polling function / status
request (the routine that currently triggers onSuccess/onError) to accept an
AbortSignal or check the cancelled flag before invoking onSuccess/onError so no
callbacks run after the dialog is dismissed.
---
Duplicate comments:
In `@core/mcp/clientmanager.go`:
- Around line 350-352: The pre-check using m.reconnectingClients.Load(...) is
racy; replace the Load call with LoadOrStore to atomically detect and claim the
id before performing a mutating operation (e.g., in DisableClient and
UpdateClient and the other similar block around the reconnect logic), so you
both test and set the in-flight marker in one step; if LoadOrStore reports the
key was already present, return the same error ("reconnect or connection
credential update already in progress for MCP client %s"), otherwise proceed and
ensure you delete the marker from m.reconnectingClients when the operation
completes or rolls back.
In `@transports/bifrost-http/lib/config.go`:
- Around line 4653-4671: The function currently allows a nil c.MCPConfig to slip
through the mirror phase and return success; ensure we return an error if the
in-memory MCP config is missing after reconnect by adding an explicit nil check
after reconnect and before treating the rotation as successful. Specifically,
after the reconnect/mirror step, if c.MCPConfig == nil return a descriptive
error (e.g., "MCPConfig missing after reconnect for client %s") instead of
proceeding to the loop that updates MCPConfig.ClientConfigs; reference the
existing symbols c.MCPConfig, MCPConfig.ClientConfigs, cc.ID, id and
newConfig.Headers (and use maps.Clone when copying headers) so the update logic
only runs when c.MCPConfig is present.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 927f823a-a0f1-4afe-8d89-3c4354b6f9b0
📒 Files selected for processing (13)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/interface.godocs/mcp/connecting-to-servers.mdxframework/configstore/rdb.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/oauth2Authorizer.tsxui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (3)
- ui/lib/types/mcp.ts
- docs/mcp/connecting-to-servers.mdx
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- ui/lib/store/apis/mcpApi.ts
- transports/bifrost-http/server/server.go
- transports/bifrost-http/handlers/mcp.go
BearTS
force-pushed
the
05-04-feat_ability_to_edit_prexisting_mcp_oauth_details
branch
from
05b1afe to
a0fd1fb
Compare
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@core/mcp/clientmanager.go`:
- Around line 650-656: connectToMCPClient currently tears down and replaces
clientMap[id] before validating new credentials, so when it fails the old live
connection and state are lost; change the flow to validate the new
credentials/connection first and only swap in the new client on success.
Specifically, modify the logic around connectToMCPClient (and the callers that
set clientMap[id] and ExecutionConfig) to: 1) create/test a new client instance
using mergedConfig without mutating clientMap[id]; 2) on successful auth/health
checks, acquire m.mu and atomically replace clientMap[id] and ExecutionConfig;
3) on failure, leave the existing clientMap[id] and ExecutionConfig untouched
and return the error. Ensure mutex usage remains correct when swapping and when
rolling back.
- Around line 517-526: The auth-type immutability check compares raw in-memory
values and rejects updates that are semantically identical ("" vs "headers");
normalize both sides to treat empty as "headers" before enforcing immutability:
create local variables (e.g., oldAuthType from client.ExecutionConfig.AuthType
and newAuthType from updatedConfig.AuthType), set each to "headers" if empty,
then compare newAuthType != oldAuthType and return the existing error if they
differ; apply this change around the existing updatedConfig.AuthType /
client.ExecutionConfig.AuthType check in clientmanager.go.
In `@transports/bifrost-http/lib/config.go`:
- Around line 4657-4661: The code mirrors only newConfig.Headers into the live
connection config (inside the if cc.ID == id block) but omits mirroring
newConfig.OauthConfigID, which leaves c.MCPConfig pointing at stale OAuth
credentials after a rotation; update that same block to also set
cc.OauthConfigID = newConfig.OauthConfigID (assign/clone as appropriate) so the
in-memory connection config is updated alongside Headers during the live
reconnect.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d157382a-941e-47b1-bba0-d927a0085799
📒 Files selected for processing (13)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/interface.godocs/mcp/connecting-to-servers.mdxframework/configstore/rdb.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/oauth2Authorizer.tsxui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (5)
- docs/mcp/connecting-to-servers.mdx
- core/bifrost.go
- framework/configstore/rdb.go
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- ui/lib/types/mcp.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx
BearTS
force-pushed
the
05-04-feat_ability_to_edit_prexisting_mcp_oauth_details
branch
from
0332ad2 to
6fae3b9
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/mcp/interface.go (1)
62-63: ⚡ Quick winUpdate the interface contract comment to include OAuth rotation.
The comment currently says this path is for “updated headers”, but the new flow also uses it for OAuth credential/config rotation. Narrow wording here can mislead future implementations across the stack.
Suggested patch
- // UpdateClientConnection reconnects an existing MCP client using updated headers + // UpdateClientConnection reconnects an existing MCP client using updated + // connection auth config (e.g., headers and OAuth config references). UpdateClientConnection(id string, newConfig *schemas.MCPClientConfig) error🤖 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 `@core/mcp/interface.go` around lines 62 - 63, Update the comment for the UpdateClientConnection method in the MCP interface to mention that this call is used not only to reconnect with updated headers but also to rotate/update OAuth credentials/configuration; edit the comment above UpdateClientConnection(id string, newConfig *schemas.MCPClientConfig) error to explicitly reference OAuth credential/config rotation so implementers know this path must handle header updates and OAuth token/credential refreshes.
🤖 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.
Nitpick comments:
In `@core/mcp/interface.go`:
- Around line 62-63: Update the comment for the UpdateClientConnection method in
the MCP interface to mention that this call is used not only to reconnect with
updated headers but also to rotate/update OAuth credentials/configuration; edit
the comment above UpdateClientConnection(id string, newConfig
*schemas.MCPClientConfig) error to explicitly reference OAuth credential/config
rotation so implementers know this path must handle header updates and OAuth
token/credential refreshes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ffa78c4-c97f-44de-8a34-9d43e8e326aa
📒 Files selected for processing (13)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/interface.godocs/mcp/connecting-to-servers.mdxframework/configstore/rdb.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/oauth2Authorizer.tsxui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (4)
- ui/lib/types/mcp.ts
- framework/configstore/rdb.go
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- ui/lib/types/schemas.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- core/bifrost.go
- docs/mcp/connecting-to-servers.mdx
- ui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx
- transports/bifrost-http/server/server.go
- ui/lib/store/apis/mcpApi.ts
- core/mcp/clientmanager.go
BearTS
force-pushed
the
05-04-feat_ability_to_edit_prexisting_mcp_oauth_details
branch
from
6fae3b9 to
c69990f
Compare
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 `@core/mcp/interface.go`:
- Around line 62-63: Update the comment for UpdateClientConnection to state it
reconnects an existing MCP client using updated headers and/or refreshed OAuth
credentials as represented in the new MCPClientConfig (including
OauthConfigID-driven rotations); mention that the method handles both simple
header updates and OAuth credential rotation flows so callers know it may
trigger an OAuth token refresh/rebind when OauthConfigID changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a767ae69-44f1-4f51-a712-8e7605e87e63
📒 Files selected for processing (13)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/interface.godocs/mcp/connecting-to-servers.mdxframework/configstore/rdb.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/oauth2Authorizer.tsxui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (2)
- transports/bifrost-http/lib/config.go
- docs/mcp/connecting-to-servers.mdx
🚧 Files skipped from review as they are similar to previous changes (9)
- ui/lib/types/mcp.ts
- ui/lib/types/schemas.ts
- ui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx
- transports/bifrost-http/server/server.go
- framework/configstore/rdb.go
- core/bifrost.go
- core/mcp/clientmanager.go
- ui/lib/store/apis/mcpApi.ts
- transports/bifrost-http/handlers/mcp.go
BearTS
force-pushed
the
05-04-feat_ability_to_edit_prexisting_mcp_oauth_details
branch
from
c69990f to
8ee2760
Compare
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 `@transports/bifrost-http/handlers/mcp.go`:
- Around line 1450-1505: The control flow is wrong: the else currently pairs
with the UpdateMCPClient error check instead of the isUpdateFlow branch, causing
updates to also run the create path and creates to skip persistence/connection;
fix by restructuring the isUpdateFlow branch so it only handles update logic
(call UpdateMCPClient, on error call updateMCPClientWithRetry and rollback via
UpdateMCPClientConfig using the saved oldDBConfig), and add a separate else
branch for the create flow that calls CreateMCPClientConfig; after a successful
create or successful update, call h.mcpManager.AddMCPClient and on failure
rollback (DeleteMCPClientConfig for create, UpdateMCPClientConfig rollback for
update) and return; use the existing functions UpdateMCPClient,
updateMCPClientWithRetry, UpdateMCPClientConfig, CreateMCPClientConfig,
AddMCPClient, and DeleteMCPClientConfig to implement this.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e121c827-f760-49b0-a782-48a56e11b614
📒 Files selected for processing (13)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/interface.godocs/mcp/connecting-to-servers.mdxframework/configstore/rdb.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/oauth2Authorizer.tsxui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (3)
- ui/lib/types/schemas.ts
- core/bifrost.go
- transports/bifrost-http/server/server.go
🚧 Files skipped from review as they are similar to previous changes (8)
- core/mcp/interface.go
- docs/mcp/connecting-to-servers.mdx
- framework/configstore/rdb.go
- ui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx
- ui/lib/store/apis/mcpApi.ts
- core/mcp/clientmanager.go
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- transports/bifrost-http/lib/config.go
BearTS
force-pushed
the
05-04-feat_ability_to_edit_prexisting_mcp_oauth_details
branch
from
8ee2760 to
93ab57e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/mcp.go (1)
729-745:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun redacted-value merging before enforcing
connection_stringimmutability.
req.ConnectionStringis compared against the stored raw value beforemergeMCPRedactedValues()runs. A caller that round-trips the redacted placeholder fromGET /api/mcp/clientscan still get a 400 even when the connection string is unchanged, which is broader than the new PUT contract.🤖 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 `@transports/bifrost-http/handlers/mcp.go` around lines 729 - 745, Run mergeMCPRedactedValues before enforcing immutability checks so redacted placeholders are expanded prior to comparison: call mergeMCPRedactedValues(&req, existingConfig) (or the existing helper used for this purpose) immediately after loading existingConfig and before comparing req.ConnectionString with existingConfig.ConnectionString; then keep the current comparisons (req.ConnectionType, req.AuthType, req.ConnectionString.Equals, stdioConfigsEqual) as-is so the connection_string check uses the merged value rather than the raw redacted placeholder.
🤖 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 `@transports/bifrost-http/handlers/mcp.go`:
- Around line 1450-1483: The update branch updates DB via
h.store.ConfigStore.UpdateMCPClientConfig and then calls
h.updateMCPClientWithRetry, but that call only swaps ExecutionConfig and does
not recreate the active transport so an OAuth credential change won't be
applied; after the DB update succeeds you must call the method that
rebuilds/reconnects the live client (e.g., UpdateMCPClientConnection or an
explicit reconnect routine used elsewhere) and if that reconnect fails, rollback
the DB update using the saved oldDBConfig (the same rollback pattern used now);
modify the flow around UpdateMCPClientConfig -> reconnect
(UpdateMCPClientConnection) -> on reconnect error call
h.store.ConfigStore.UpdateMCPClientConfig(ctx, mcpClientConfig.ID, &oldDBConfig)
and log/return the failure.
---
Duplicate comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 729-745: Run mergeMCPRedactedValues before enforcing immutability
checks so redacted placeholders are expanded prior to comparison: call
mergeMCPRedactedValues(&req, existingConfig) (or the existing helper used for
this purpose) immediately after loading existingConfig and before comparing
req.ConnectionString with existingConfig.ConnectionString; then keep the current
comparisons (req.ConnectionType, req.AuthType, req.ConnectionString.Equals,
stdioConfigsEqual) as-is so the connection_string check uses the merged value
rather than the raw redacted placeholder.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 277f5ac3-993c-4a50-b8e5-b20aedfbc227
📒 Files selected for processing (13)
core/bifrost.gocore/mcp/clientmanager.gocore/mcp/interface.godocs/mcp/connecting-to-servers.mdxframework/configstore/rdb.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/server/server.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/oauth2Authorizer.tsxui/lib/store/apis/mcpApi.tsui/lib/types/mcp.tsui/lib/types/schemas.ts
✅ Files skipped from review due to trivial changes (6)
- docs/mcp/connecting-to-servers.mdx
- ui/lib/types/schemas.ts
- core/bifrost.go
- transports/bifrost-http/server/server.go
- transports/bifrost-http/lib/config.go
- framework/configstore/rdb.go
🚧 Files skipped from review as they are similar to previous changes (4)
- core/mcp/interface.go
- ui/lib/store/apis/mcpApi.ts
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- ui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx
BearTS
force-pushed
the
05-04-feat_ability_to_edit_prexisting_mcp_oauth_details
branch
from
93ab57e to
31eec60
Compare
akshaydeo
previously approved these changes
May 5, 2026
Contributor
Merge activity
|
akshaydeo
changed the base branch from
04-27-feat_adds_option_to_disable_mcp_clients
to
graphite-base/3209
akshaydeo
dismissed stale reviews from coderabbitai[bot] and themself
The base branch was changed.
akshaydeo
force-pushed
the
05-04-feat_ability_to_edit_prexisting_mcp_oauth_details
branch
from
31eec60 to
61d9ae2
Compare
akshaydeo
deleted the
05-04-feat_ability_to_edit_prexisting_mcp_oauth_details
branch
18 tasks
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
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
Adds support for rotating OAuth credentials (
client_idandclient_secret) on existing MCP clients without requiring the client to be deleted and recreated. The edit flow now initiates a new OAuth authorization flow when updated credentials are provided, preserving the existing connection type, auth type, and connection URL as immutable fields.Changes
MCPClientUpdateRequestnow accepts an optionaloauth_configfield containingclient_idandclient_secretfor credential rotation, restricted to clients withauth_typeofoauthorper_user_oauth.updateMCPClienthandler enforces immutability ofconnection_type,auth_type,connection_string, andstdio_configon PUT requests, returning a 400 if any of these are supplied with a differing value.pending_oauthstatus with anauthorize_url, mirroring the creation flow.completeMCPClientOAuthnow detects whether the pending config maps to an existing client (isUpdateFlow) and branches into update vs. create paths for both standard and per-user OAuth, including rollback on manager update failure.UpdateMCPClientConfigin the RDB config store now persistsoauth_config_id,discovered_tools_json, andtool_name_mapping_jsonwhen provided.client_id,client_secret) for OAuth-type clients, triggers theOAuth2Authorizerdialog on apending_oauthresponse, and handles success/error callbacks.updateMCPClientAPI mutation is typed asUpdateMCPClientResponse(union of success andOAuthFlowResponse) and skips the optimistic cache patch when the response ispending_oauth.OAuth2Authorizerdialog now responds toonOpenChangeso clicking outside or pressing Escape triggers the cancel handler.client_idandclient_secretcan be rotated via the edit flow while connection metadata remains immutable.Type of change
Affected areas
How to test
auth_type: oauthorauth_type: per_user_oauth.Client IDandClient Secretinputs.client_id(and optionally a newclient_secret) and save.OAuth2Authorizerdialog with the new authorization URL.client_secret(noclient_id) and confirm the backend resolves the existingclient_idfrom the stored OAuth config.connection_type,auth_type,connection_string, orstdio_configvia the API and confirm a 400 is returned.Breaking changes
PUT
/api/mcp/client/:idnow returns a 400 ifconnection_type,auth_type,connection_string, orstdio_configare included in the request body with values that differ from the stored config. Callers that previously sent these fields expecting them to be silently ignored must remove them.Security considerations
OAuth
client_secretvalues are handled as sensitive inputs (password-type field in the UI, trimmed and never logged). The rotation flow reuses the existing OAuth config encryption path. Only clients whose storedauth_typeisoauthorper_user_oauthare permitted to submit credential updates, preventing credential injection on non-OAuth clients.Checklist
docs/contributing/README.mdand followed the guidelines