refactor: merge oauth_tokens/oauth_user_tokens into unified mcp_oauth_tokens table - #5708
Merged
Pratham-Mishra04 merged 1 commit intoAug 8, 2026
Conversation
|
|
This was referenced Jul 30, 2026
Collaborator
Author
This was referenced Jul 30, 2026
refactor: migrate OAuth flow state/PKCE fields from
oauth_configs to new mcp_oauth_flows table
#5709
Merged
Merged
Merged
Merged
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughOAuth token storage is consolidated into ChangesOAuth token storage consolidation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OAuthFlow
participant ConfigStore
participant mcp_oauth_tokens
participant MCPHTTP
OAuthFlow->>ConfigStore: create consolidated OAuth token
ConfigStore->>mcp_oauth_tokens: persist shared or per-user row
MCPHTTP->>ConfigStore: load authorized per-user token
ConfigStore->>mcp_oauth_tokens: filter by auth mode and identity
mcp_oauth_tokens-->>MCPHTTP: return authorized token row
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 |
This was referenced Jul 30, 2026
Merged
This was referenced Aug 6, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
framework/configstore/migrations_perf_test.go (1)
53-59: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose the SQLite pool if the migration chain fails.
runPreMergeMigrationChainasserts withrequireand can stop the goroutine.setupPreMergeSQLiteDBthen returns no*gorm.DB, and the caller never registers the cleanup at lines 344-351. The pool stays open. Register the close cleanup directly aftergorm.Opensucceeds, as the Postgres helper does.♻️ Proposed fix
require.NoError(t, err, "failed to open perf test sqlite db") + if sqlDB, dbErr := db.DB(); dbErr == nil { + t.Cleanup(func() { _ = sqlDB.Close() }) + } + runPreMergeMigrationChain(t, db) return db🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/configstore/migrations_perf_test.go` around lines 53 - 59, In setupPreMergeSQLiteDB, register the SQLite database cleanup immediately after the successful gorm.Open call and before runPreMergeMigrationChain executes. Reuse the existing cleanup pattern used by the Postgres helper so the pool closes even when runPreMergeMigrationChain aborts via require.
🤖 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.
Duplicate comments:
In `@framework/configstore/migrations_perf_test.go`:
- Around line 53-59: In setupPreMergeSQLiteDB, register the SQLite database
cleanup immediately after the successful gorm.Open call and before
runPreMergeMigrationChain executes. Reuse the existing cleanup pattern used by
the Postgres helper so the pool closes even when runPreMergeMigrationChain
aborts via require.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: af85c565-a4eb-4b09-9b49-be1b51b48a4a
📒 Files selected for processing (15)
framework/configstore/encryption.goframework/configstore/encryption_test.goframework/configstore/migrations.goframework/configstore/migrations_perf_test.goframework/configstore/rdb.goframework/configstore/rdb_mcp_sessions_test.goframework/configstore/rdb_oauth2_test.goframework/configstore/rdb_test.goframework/configstore/store.goframework/configstore/tables/mcpoauth2.goframework/oauth2/main.goframework/oauth2/sync_test.gotests/scripts/migration-checker/main.gotransports/bifrost-http/handlers/mcpsessions.gotransports/bifrost-http/lib/config_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- framework/oauth2/main.go
- tests/scripts/migration-checker/main.go
- framework/configstore/rdb_mcp_sessions_test.go
- framework/configstore/rdb_test.go
- framework/configstore/encryption.go
- transports/bifrost-http/lib/config_test.go
- transports/bifrost-http/handlers/mcpsessions.go
- framework/configstore/tables/mcpoauth2.go
- framework/oauth2/sync_test.go
- framework/configstore/rdb_oauth2_test.go
- framework/configstore/store.go
- framework/configstore/migrations.go
- framework/configstore/encryption_test.go
- framework/configstore/rdb.go
Pratham-Mishra04
force-pushed
the
07-28-feat_unify_shared_and_per-user_mcp_oauth_token_storage_into_tablemcpoauthtoken
branch
from
August 6, 2026 21:53
5025fd1 to
7949fe6
Compare
Pratham-Mishra04
force-pushed
the
05-29-feat_ui_support_for_config_json_per_user_headers_mcp
branch
from
August 6, 2026 21:53
f6e41a6 to
4f06391
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.
🧹 Nitpick comments (1)
framework/configstore/migrations_perf_test.go (1)
161-168: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove explicit GORM timestamp assignments.
GORM populates
CreatedAtandUpdatedAtfor these table models. Remove the manual assignments and removenowwhere it becomes unused.
framework/configstore/migrations_perf_test.go#L161-L168: removeCreatedAtandUpdatedAtfromTableMCPClient.framework/configstore/migrations_perf_test.go#L225-L255: removeCreatedAtandUpdatedAtfrom all seeded shared-token records.framework/configstore/migrations_perf_test.go#L280-L295: removeCreatedAtandUpdatedAtfromTableOauthUserToken.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/configstore/migrations_perf_test.go` around lines 161 - 168, Remove the explicit CreatedAt and UpdatedAt assignments from the TableMCPClient seed at framework/configstore/migrations_perf_test.go#L161-L168, all seeded shared-token records at framework/configstore/migrations_perf_test.go#L225-L255, and the TableOauthUserToken seed at framework/configstore/migrations_perf_test.go#L280-L295; then remove the now-unused now variable while preserving the existing seed data.Source: Learnings
🤖 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 `@framework/configstore/migrations_perf_test.go`:
- Around line 161-168: Remove the explicit CreatedAt and UpdatedAt assignments
from the TableMCPClient seed at
framework/configstore/migrations_perf_test.go#L161-L168, all seeded shared-token
records at framework/configstore/migrations_perf_test.go#L225-L255, and the
TableOauthUserToken seed at
framework/configstore/migrations_perf_test.go#L280-L295; then remove the
now-unused now variable while preserving the existing seed data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 38786e27-735f-42a0-9963-2f2b866ffd0e
📒 Files selected for processing (15)
framework/configstore/encryption.goframework/configstore/encryption_test.goframework/configstore/migrations.goframework/configstore/migrations_perf_test.goframework/configstore/rdb.goframework/configstore/rdb_mcp_sessions_test.goframework/configstore/rdb_oauth2_test.goframework/configstore/rdb_test.goframework/configstore/store.goframework/configstore/tables/mcpoauth2.goframework/oauth2/main.goframework/oauth2/sync_test.gotests/scripts/migration-checker/main.gotransports/bifrost-http/handlers/mcpsessions.gotransports/bifrost-http/lib/config_test.go
🚧 Files skipped from review as they are similar to previous changes (14)
- framework/configstore/rdb_test.go
- framework/configstore/rdb_oauth2_test.go
- tests/scripts/migration-checker/main.go
- framework/configstore/rdb_mcp_sessions_test.go
- transports/bifrost-http/handlers/mcpsessions.go
- framework/configstore/encryption_test.go
- framework/configstore/migrations.go
- transports/bifrost-http/lib/config_test.go
- framework/configstore/tables/mcpoauth2.go
- framework/configstore/encryption.go
- framework/oauth2/main.go
- framework/oauth2/sync_test.go
- framework/configstore/store.go
- framework/configstore/rdb.go
Pratham-Mishra04
force-pushed
the
07-28-feat_unify_shared_and_per-user_mcp_oauth_token_storage_into_tablemcpoauthtoken
branch
from
August 8, 2026 08:43
7949fe6 to
fb096cd
Compare
Collaborator
Author
Merge activity
|
Pratham-Mishra04
changed the base branch from
05-29-feat_ui_support_for_config_json_per_user_headers_mcp
to
graphite-base/5708
August 8, 2026 08:57
Pratham-Mishra04
dismissed
coderabbitai[bot]’s stale review
August 8, 2026 08:59
The base branch was changed.
…en perf fixtures so the migration's join is exercised, not just its miss path
Pratham-Mishra04
force-pushed
the
07-28-feat_unify_shared_and_per-user_mcp_oauth_token_storage_into_tablemcpoauthtoken
branch
from
August 8, 2026 09:00
fb096cd to
93937a5
Compare
Pratham-Mishra04
deleted the
07-28-feat_unify_shared_and_per-user_mcp_oauth_token_storage_into_tablemcpoauthtoken
branch
August 8, 2026 09:02
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
Merges the two legacy MCP OAuth token tables (
oauth_tokensfor shared client credentials andoauth_user_tokensfor per-identity credentials) into a single unified table,mcp_oauth_tokens, distinguished by anauth_modecolumn (shared|user|vk|session). This eliminates the split storage model, closes a long-standing orphan leak where deleting an MCP client never cleaned up its sharedoauth_tokensrow or itsoauth_configsrow, and adds defense-in-depth guards so per-user code paths can never read, mutate, or delete a shared credential and vice versa.Changes
TableMCPOauthTokenmapped tomcp_oauth_tokens, carrying every column both predecessor tables held plusauth_modeandstatus.TableOauthTokenandTableOauthUserTokenare retained as deprecated stubs so historical migrations keep compiling; both will be dropped in the next major version.merge_oauth_token_tablesas the final registered step. It createsmcp_oauth_tokensfresh, backfills shared rows fromoauth_tokens(derivingauth_mode='shared',mcp_client_id, andoauth_config_idinline via subselect), copies per-identity rows fromoauth_user_tokensfield-for-field, dedupes anyshared-modemcp_client_idcollisions introduced by the pre-existing orphan leak, then creates four partial unique indexes (one perauth_mode). Neither legacy table is dropped.ConfigStoreinterface methods and theirRDBConfigStoreimplementations are updated to operate onTableMCPOauthToken. Per-user-scoped methods (GetOauthUserTokenByID,UpdateOauthUserToken,DeleteOauthUserToken,MarkOauthUserTokenNeedsReauthByID,DeleteOrphanedOauthUserTokens,ListOauthUserTokens) now filter onauth_mode IN ('user','vk','session')so a shared row can never be reached through a per-user endpoint.GetExpiringOauthTokensgains an explicitauth_mode = 'shared'filter so theTokenRefreshWorkercontinues to handle only shared credentials; per-user proactive refresh remains a deliberate later change.DeleteMCPClientConfignow deletes allmcp_oauth_tokensrows for the client (shared and per-identity alike) in one pass and explicitly deletes the client'soauth_configsrow, closing the orphan leak that previously left both untouched.DeleteVirtualKeyandreconcileVKDirectTokensDBare updated to targetmcp_oauth_tokenswithauth_modescoping.CompleteOAuthFlowpopulatesAuthMode='shared',OauthConfigID,Status='active', and attempts to deriveMCPClientIDviaGetMCPClientByOauthConfigIDbefore creating the token record.migrations_perf_test.gowithTestMigrationMergeOauthTokenTablesPerf, gated behindBIFROST_RUN_MIGRATION_PERF_TESTS=1, which seeds ~1,000,000 rows across the pre-merge tables and measures the migration's wall-clock cost against both SQLite and Postgres.perUserOauthAuthModesis introduced as a package-level slice centralizing the set of non-shared auth modes used by all scoped queries.Type of change
Affected areas
How to test
Validate that:
mcp_oauth_tokensand leavesoauth_tokens/oauth_user_tokensempty but present.mcp_oauth_tokensrows and itsoauth_configsrow.auth_mode='shared'rows appear).Breaking changes
The
ConfigStoreinterface signatures for all OAuth token methods now use*tables.TableMCPOauthTokeninstead of*tables.TableOauthTokenor*tables.TableOauthUserToken. Any external implementation ofConfigStoremust be updated. The underlying storage moves fromoauth_tokens/oauth_user_tokenstomcp_oauth_tokens; the migration handles existing data automatically, but rollback dropsmcp_oauth_tokensentirely and restores reads/writes to the legacy tables.Security considerations
auth_mode IN ('user','vk','session'), preventing a caller-supplied token ID from resolving to a shared credential through endpoints intended only for per-identity credentials (e.g.POST /api/mcp/sessions/{id}/reauth,DELETE /api/mcp/sessions/{id}).UpdateOauthUserTokenrejects rows whoseauth_modeis not one of the per-identity modes, preventing silent rewrites of shared credentials through the per-user API.DeleteMCPClientConfig(sharedoauth_tokensrow andoauth_configsrow never cleaned up on client deletion) is closed.Checklist
docs/contributing/README.mdand followed the guidelines