fix: extend pending_verification bootstrap path to cover per_user_oauth clients - #3873
Merged
Pratham-Mishra04 merged 1 commit intoAug 8, 2026
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR extends OAuth pending-verification handling to shared and ChangesPer-user OAuth pending verification
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ConfigBootstrap
participant ClientManager
participant MCPRegistry
participant HTTPHandlers
ConfigBootstrap->>ClientManager: load OAuth client with pending configuration
ClientManager->>ClientManager: set PendingVerification state
MCPRegistry->>HTTPHandlers: initiate verification
HTTPHandlers-->>MCPRegistry: return authorization and completion URLs
MCPRegistry->>HTTPHandlers: complete OAuth verification
HTTPHandlers->>ClientManager: update verified connection
HTTPHandlers->>ConfigBootstrap: clear pending OAuth configuration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Collaborator
Author
|
|
This was referenced May 29, 2026
Contributor
Confidence Score: 4/5The cleanup error path should be fixed before merging.
transports/bifrost-http/handlers/mcp.go Important Files Changed
Reviews (6): Last reviewed commit: "feat: add per user oauth mcp support for..." | Re-trigger Greptile |
Pratham-Mishra04
force-pushed
the
05-29-feat_add_per_user_oauth_mcp_support_for_config.json
branch
from
July 1, 2026 10:15
ff11dc3 to
4d85b9c
Compare
Pratham-Mishra04
force-pushed
the
05-29-feat_ui_support_for_pending_auth_mcps
branch
from
July 1, 2026 10:15
2e6de8c to
41d8011
Compare
![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)
core/mcp/clientmanager.go (1)
358-359: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd table-driven coverage for the new
per_user_oauthbranch.The pending-verification gate now branches on two auth types, but no test changes are visible for this behavior change. Since
core/mcp/clientmanager_test.goexists in this package, consider adding a case assertingper_user_oauthclients withPendingOAuthConfigset land inMCPConnectionStatePendingVerification, alongside the existingoauthcase.As per path instructions, "review agent loops for bounded depth, deterministic tool execution behavior" and general Go guideline to apply "table-driven coverage for behavior changes."
🤖 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 358 - 359, Add table-driven test coverage in clientmanager_test for the pending-verification branch in ClientManager behavior. Extend the existing auth-state tests to include a per_user_oauth case with PendingOAuthConfig set and assert it resolves to MCPConnectionStatePendingVerification, alongside the existing oauth coverage. Use the same test setup/helpers already used for the oauth branch so the new case exercises the config.PendingOAuthConfig gate and the AuthType check in the client manager logic.Source: Path instructions
🤖 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 1620-1631: The OAuth bootstrap fallback in the config loader is
unreachable from the config-file path because the schema currently requires
oauth_config_id for oauth and per_user_oauth. Update the MCP schema validation
to allow the bootstrap-pending shape, or move the PendingOAuthConfig synthesis
in config.go behind a non-config.json source path so the runtime gate and
initiate-verification flow can actually reach it.
---
Nitpick comments:
In `@core/mcp/clientmanager.go`:
- Around line 358-359: Add table-driven test coverage in clientmanager_test for
the pending-verification branch in ClientManager behavior. Extend the existing
auth-state tests to include a per_user_oauth case with PendingOAuthConfig set
and assert it resolves to MCPConnectionStatePendingVerification, alongside the
existing oauth coverage. Use the same test setup/helpers already used for the
oauth branch so the new case exercises the config.PendingOAuthConfig gate and
the AuthType check in the client manager logic.
🪄 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: b70191eb-7cca-4fd2-8560-0df58a74ac26
📒 Files selected for processing (4)
core/mcp/clientmanager.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
Pratham-Mishra04
force-pushed
the
05-29-feat_add_per_user_oauth_mcp_support_for_config.json
branch
from
July 2, 2026 05:29
4d85b9c to
fd86d73
Compare
Pratham-Mishra04
force-pushed
the
05-29-feat_ui_support_for_pending_auth_mcps
branch
from
July 2, 2026 05:29
41d8011 to
dc2f30a
Compare
Pratham-Mishra04
force-pushed
the
05-29-feat_add_per_user_oauth_mcp_support_for_config.json
branch
from
July 2, 2026 13:53
fd86d73 to
c26897d
Compare
Pratham-Mishra04
force-pushed
the
05-29-feat_ui_support_for_pending_auth_mcps
branch
from
July 2, 2026 13:53
dc2f30a to
ae90a7d
Compare
This was referenced Jul 2, 2026
Pratham-Mishra04
force-pushed
the
05-29-feat_add_per_user_oauth_mcp_support_for_config.json
branch
from
July 3, 2026 06:17
c26897d to
77e5f5f
Compare
Pratham-Mishra04
force-pushed
the
05-29-feat_ui_support_for_pending_auth_mcps
branch
from
July 3, 2026 06:17
ae90a7d to
32cf960
Compare
Pratham-Mishra04
force-pushed
the
05-29-feat_add_per_user_oauth_mcp_support_for_config.json
branch
from
July 21, 2026 09:16
77e5f5f to
a5d0538
Compare
Pratham-Mishra04
force-pushed
the
05-29-feat_ui_support_for_pending_auth_mcps
branch
from
July 21, 2026 09:16
32cf960 to
8e33d96
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
2 tasks
This was referenced Aug 6, 2026
Pratham-Mishra04
force-pushed
the
05-29-feat_ui_support_for_pending_auth_mcps
branch
from
August 6, 2026 21:53
5aa467f to
fd4d929
Compare
Pratham-Mishra04
force-pushed
the
05-29-feat_add_per_user_oauth_mcp_support_for_config.json
branch
from
August 6, 2026 21:53
84d0fa9 to
b9a6764
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 `@ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx`:
- Around line 226-242: Serialize concurrent bootstrap flows in the client
verification action so only one bootstrap can run at a time, preventing shared
oauthConfigId, mcpClientId, and "oauth_popup" state from being overwritten.
Update the relevant bootstrap/verification handler around the popup
initialization to disable or reject other bootstrap actions until the active
flow completes, then restore availability on success or failure. Add an E2E test
covering two initiation requests resolving out of order.
🪄 Autofix
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: b194710c-5aa4-49f9-b82a-74c0ae7cb9f3
📒 Files selected for processing (6)
core/mcp/clientmanager.gotransports/bifrost-http/handlers/mcp.gotransports/bifrost-http/lib/config.gotransports/bifrost-http/lib/config_test.goui/app/workspace/mcp-registry/views/mcpClientSheet.tsxui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- ui/app/workspace/mcp-registry/views/mcpClientSheet.tsx
- transports/bifrost-http/lib/config.go
- core/mcp/clientmanager.go
- transports/bifrost-http/handlers/mcp.go
- transports/bifrost-http/lib/config_test.go
Comment on lines
+226
to
+242
| const isPerUserOauth = client.config.auth_type === "per_user_oauth"; | ||
| // Open a blank popup synchronously, before the initiateVerification | ||
| // await, so the click's transient user-activation is captured here | ||
| // rather than consumed by the network round-trip — otherwise the | ||
| // browser can block OAuth2Authorizer's later window.open entirely. | ||
| // OAuth2Authorizer navigates this handle once authorize_url is known. | ||
| const width = 600; | ||
| const height = 700; | ||
| const left = window.screen.width / 2 - width / 2; | ||
| const top = window.screen.height / 2 - height / 2; | ||
| const popup = window.open("", "oauth_popup", `width=${width},height=${height},left=${left},top=${top},resizable=yes,scrollbars=yes`); | ||
| // Not needed for per_user_oauth: that flow shows a confirm step first | ||
| // and opens its own popup synchronously from that step's own button | ||
| // click, so pre-opening one here would just leak an unused window. | ||
| let popup: Window | null = null; | ||
| if (!isPerUserOauth) { | ||
| const width = 600; | ||
| const height = 700; | ||
| const left = window.screen.width / 2 - width / 2; | ||
| const top = window.screen.height / 2 - height / 2; | ||
| popup = window.open("", "oauth_popup", `width=${width},height=${height},left=${left},top=${top},resizable=yes,scrollbars=yes`); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target file outline ---'
ast-grep outline ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
printf '%s\n' '--- relevant target sections ---'
sed -n '160,275p' ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
printf '%s\n' '--- all relevant symbols and popup usage ---'
rg -n -C 4 'bootstrapAuthorize|authorizingClients|oauth_popup|initialPopup|OAuth2Authorizer|initiateVerification' ui
printf '%s\n' '--- candidate OAuth2Authorizer files ---'
fd -i 'OAuth2Authorizer|oauth.*author' uiRepository: maximhq/bifrost
Length of output: 31198
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- row action guards and handler wiring ---'
sed -n '1,145p' ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
sed -n '620,665p' ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx
printf '%s\n' '--- OAuth2Authorizer implementation ---'
sed -n '1,330p' ui/app/workspace/mcp-registry/views/oauth2Authorizer.tsx
printf '%s\n' '--- API mutation definition ---'
sed -n '245,275p' ui/lib/store/apis/mcpApi.tsRepository: maximhq/bifrost
Length of output: 18207
Serialize concurrent bootstrap flows or key state and popups per client.
Different rows can start verification at the same time. Both shared OAuth flows use the "oauth_popup" target, and bootstrapAuthorize stores only one flow. An out-of-order response can replace the active oauthConfigId and mcpClientId, leaving the other flow stranded.
Disable all bootstrap actions while one flow is active, or store flow state per client_id and use a unique popup name for each flow. Add an E2E test for two initiation requests that resolve out of order.
🤖 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 `@ui/app/workspace/mcp-registry/views/mcpClientsTable.tsx` around lines 226 -
242, Serialize concurrent bootstrap flows in the client verification action so
only one bootstrap can run at a time, preventing shared oauthConfigId,
mcpClientId, and "oauth_popup" state from being overwritten. Update the relevant
bootstrap/verification handler around the popup initialization to disable or
reject other bootstrap actions until the active flow completes, then restore
availability on success or failure. Add an E2E test covering two initiation
requests resolving out of order.
Pratham-Mishra04
force-pushed
the
05-29-feat_add_per_user_oauth_mcp_support_for_config.json
branch
from
August 8, 2026 08:43
b9a6764 to
9129d60
Compare
Pratham-Mishra04
force-pushed
the
05-29-feat_ui_support_for_pending_auth_mcps
branch
from
August 8, 2026 08:43
fd4d929 to
561a8a5
Compare
Collaborator
Author
Merge activity
|
Pratham-Mishra04
changed the base branch from
05-29-feat_ui_support_for_pending_auth_mcps
to
graphite-base/3873
August 8, 2026 08:49
Pratham-Mishra04
force-pushed
the
05-29-feat_add_per_user_oauth_mcp_support_for_config.json
branch
from
August 8, 2026 08:52
9129d60 to
a866cd8
Compare
Pratham-Mishra04
deleted the
05-29-feat_add_per_user_oauth_mcp_support_for_config.json
branch
August 8, 2026 08:53
This was referenced Aug 8, 2026
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
Extends the
pending_verificationbootstrap flow to coverper_user_oauthclients in addition to sharedoauthclients. Previously, only shared OAuth clients declared inconfig.jsonwere parked inpending_verificationawaiting admin authorization. Per-user OAuth clients loaded from config were not handled the same way, meaning they could end up in an inconsistent state before an admin completed the verification step.Changes
pending_verificationgate inAddClientto apply to bothoauthandper_user_oauthauth types whenPendingOAuthConfigis set, replacing the previous shared-OAuth-only check.initiate-verificationendpoint now accepts bothoauthandper_user_oauthauth types instead of rejecting the latter.complete-oauthcallback handler now permits both OAuth-based auth types to complete through the endpoint, with downstream branching handling the per-user vs. shared distinction.config.json, OAuth-based clients (oauthorper_user_oauth) that have no inlineoauth_configblock and no existingoauth_config_idnow have an emptyOAuth2Configsynthesized so they consistently land inpending_verificationand theinitiate-verificationendpoint can run discovery and dynamic client registration at admin-click time.per_user_oauthclients inpending_verificationshow a "Verify" button and a description clarifying that the admin test login is a one-time setup step and each user will authenticate individually afterward. The success toast message is similarly differentiated.Type of change
Affected areas
How to test
per_user_oauthMCP client inconfig.jsonwith aconnection_stringbut nooauth_configblock.pending_verificationstate.pending_verificationand tools are discovered.oauthclient to confirm existing behavior is unchanged.Breaking changes
Security considerations
The admin verification flow for
per_user_oauthuses the upstream token only as a temporary credential to validate the OAuth configuration and discover tools, then revokes it. The token is not persisted or reused for end-user requests. No end-user credentials are stored as a result of the admin verification step.Checklist
docs/contributing/README.mdand followed the guidelines