mcp auth server setup#1429
Closed
akshaydeo wants to merge 4 commits intographite-base/1429from
Closed
Conversation
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a pluggable CodeMode (Starlark) and per-client tool sync subsystem, converts MCP client configs to pointer-based schemas, moves OAuth pending MCP state to DB, reworks migrations and DB columns, broad refactors across MCP manager/interface and related APIs, and introduces multiple frontend MCP UI pages/components and OAuth flows. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Contributor
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
akshaydeo
force-pushed
the
01-24-mcp_auth_server_setup
branch
from
9bba38f to
c464a72
Compare
akshaydeo
changed the base branch from
graphite-base/1429
to
01-24-feat_add_oauth_support_to_mcp
This was referenced Jan 26, 2026
Closed
akshaydeo
force-pushed
the
01-24-mcp_auth_server_setup
branch
from
c464a72 to
3ab9e96
Compare
akshaydeo
force-pushed
the
01-24-feat_add_oauth_support_to_mcp
branch
from
ddbaed6 to
30d19ca
Compare
akshaydeo
force-pushed
the
01-24-mcp_auth_server_setup
branch
from
3ab9e96 to
bdfabe5
Compare
akshaydeo
force-pushed
the
01-24-feat_add_oauth_support_to_mcp
branch
from
30d19ca to
0fcfd40
Compare
akshaydeo
force-pushed
the
01-24-mcp_auth_server_setup
branch
from
bdfabe5 to
33569ef
Compare
akshaydeo
force-pushed
the
01-24-feat_add_oauth_support_to_mcp
branch
from
0fcfd40 to
ec62573
Compare
akshaydeo
force-pushed
the
01-24-mcp_auth_server_setup
branch
from
33569ef to
7570b34
Compare
akshaydeo
force-pushed
the
01-24-feat_add_oauth_support_to_mcp
branch
from
ec62573 to
4df30b5
Compare
akshaydeo
force-pushed
the
01-24-mcp_auth_server_setup
branch
from
7570b34 to
903979e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@framework/configstore/migrations.go`:
- Around line 3001-3060: The rollback for migrationAddOAuthTables currently
drops only oauth tables but leaves the MCP client columns (auth_type,
oauth_config_id and any oauth_config column) added in the Migrate block; update
the Rollback function to also check for and remove those columns from
tables.TableMCPClient (use migrator.HasColumn + migrator.DropColumn for
"auth_type", "oauth_config_id" and "oauth_config" if present) so the schema is
fully reverted and older binaries can operate against the pre-migration schema.
- Around line 155-168: The rollback for migrationAddOAuthTables is incomplete:
update its down/rollback path to also remove the auth_type and oauth_config_id
columns from TableMCPClient in addition to dropping TableOauthToken and
TableOauthConfig; locate the migrationAddOAuthTables function and modify its
rollback branch to drop those two columns (auth_type, oauth_config_id) from
TableMCPClient so the migration is symmetric.
In `@framework/oauth2/main.go`:
- Line 371: Replace the printf-style logger.Debug calls that use "%s" and "%t"
with structured logging by removing format verbs and passing key/value pairs;
e.g. update the Dynamic client registration logger (the logger.Debug call that
references clientID and clientSecret) to logger.Debug("Dynamic client
registration successful", "client_id", clientID, "has_secret", clientSecret !=
"") and make the analogous change for the other logger.Debug at line 420 so both
use message plus "client_id"/client_secret boolean key/value fields instead of
printf formatting.
Comment on lines
+3001
to
3060
| // migrationAddOAuthTables creates the oauth_configs and oauth_tokens tables | ||
| func migrationAddOAuthTables(ctx context.Context, db *gorm.DB) error { | ||
| m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ | ||
| ID: "add_disable_db_pings_in_health_column", | ||
| ID: "add_oauth_tables", | ||
| Migrate: func(tx *gorm.DB) error { | ||
| tx = tx.WithContext(ctx) | ||
| migrator := tx.Migrator() | ||
| if !migrator.HasColumn(&tables.TableClientConfig{}, "disable_db_pings_in_health") { | ||
| if err := migrator.AddColumn(&tables.TableClientConfig{}, "disable_db_pings_in_health"); err != nil { | ||
| return err | ||
| // Updating MCPClient table to add auth_type, oauth_config_id, and oauth_config columns | ||
| if !migrator.HasColumn(&tables.TableMCPClient{}, "auth_type") { | ||
| if err := migrator.AddColumn(&tables.TableMCPClient{}, "auth_type"); err != nil { | ||
| return fmt.Errorf("failed to add auth_type column: %w", err) | ||
| } | ||
| } | ||
| return nil | ||
| }, | ||
| Rollback: func(tx *gorm.DB) error { | ||
| tx = tx.WithContext(ctx) | ||
| migrator := tx.Migrator() | ||
| if migrator.HasColumn(&tables.TableClientConfig{}, "disable_db_pings_in_health") { | ||
| if err := migrator.DropColumn(&tables.TableClientConfig{}, "disable_db_pings_in_health"); err != nil { | ||
| return err | ||
| if !migrator.HasColumn(&tables.TableMCPClient{}, "oauth_config_id") { | ||
| if err := migrator.AddColumn(&tables.TableMCPClient{}, "oauth_config_id"); err != nil { | ||
| return fmt.Errorf("failed to add oauth_config_id column: %w", err) | ||
| } | ||
| } | ||
| return nil | ||
| }, | ||
| }}) | ||
| err := m.Migrate() | ||
| if err != nil { | ||
| return fmt.Errorf("error while running db migration: %s", err.Error()) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // migrationAddIsPingAvailableColumn adds the is_ping_available column to the config_mcp_clients table | ||
| func migrationAddIsPingAvailableColumn(ctx context.Context, db *gorm.DB) error { | ||
| m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ | ||
| ID: "add_is_ping_available_column", | ||
| Migrate: func(tx *gorm.DB) error { | ||
| tx = tx.WithContext(ctx) | ||
| migrator := tx.Migrator() | ||
| if !migrator.HasColumn(&tables.TableMCPClient{}, "is_ping_available") { | ||
| if err := migrator.AddColumn(&tables.TableMCPClient{}, "is_ping_available"); err != nil { | ||
| return err | ||
| // Set default value for auth_type column | ||
| if err := tx.Model(&tables.TableMCPClient{}).Where("auth_type IS NULL").Update("auth_type", "headers").Error; err != nil { | ||
| return err | ||
| } | ||
| // Create oauth_configs table | ||
| if !migrator.HasTable(&tables.TableOauthConfig{}) { | ||
| if err := migrator.CreateTable(&tables.TableOauthConfig{}); err != nil { | ||
| return fmt.Errorf("failed to create oauth_configs table: %w", err) | ||
| } | ||
| // Set default value for existing rows | ||
| if err := tx.Model(&tables.TableMCPClient{}).Where("is_ping_available IS NULL").Update("is_ping_available", true).Error; err != nil { | ||
| return err | ||
| } | ||
| // Create oauth_tokens table | ||
| if !migrator.HasTable(&tables.TableOauthToken{}) { | ||
| if err := migrator.CreateTable(&tables.TableOauthToken{}); err != nil { | ||
| return fmt.Errorf("failed to create oauth_tokens table: %w", err) | ||
| } | ||
| } | ||
| return nil | ||
| }, | ||
| Rollback: func(tx *gorm.DB) error { | ||
| tx = tx.WithContext(ctx) | ||
| migrator := tx.Migrator() | ||
| if migrator.HasColumn(&tables.TableMCPClient{}, "is_ping_available") { | ||
| if err := migrator.DropColumn(&tables.TableMCPClient{}, "is_ping_available"); err != nil { | ||
| return err | ||
|
|
||
| // Drop tables in reverse order | ||
| if migrator.HasTable(&tables.TableOauthToken{}) { | ||
| if err := migrator.DropTable(&tables.TableOauthToken{}); err != nil { | ||
| return fmt.Errorf("failed to drop oauth_tokens table: %w", err) | ||
| } | ||
| } | ||
|
|
||
| if migrator.HasTable(&tables.TableOauthConfig{}) { | ||
| if err := migrator.DropTable(&tables.TableOauthConfig{}); err != nil { | ||
| return fmt.Errorf("failed to drop oauth_configs table: %w", err) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| }, | ||
| }}) | ||
| err := m.Migrate() | ||
| if err != nil { | ||
| return fmt.Errorf("error while running is_ping_available migration: %s", err.Error()) | ||
| return fmt.Errorf("error while running oauth tables migration: %s", err.Error()) | ||
| } |
Contributor
There was a problem hiding this comment.
Rollback leaves MCP auth columns behind.
Line 3008–3021 add auth_type and oauth_config_id, but rollback only drops OAuth tables. A rollback will leave extra columns on config_mcp_clients, which can break down-migrations or older binaries expecting the pre-migration schema.
✅ Suggested rollback completion
Rollback: func(tx *gorm.DB) error {
tx = tx.WithContext(ctx)
migrator := tx.Migrator()
// Drop tables in reverse order
if migrator.HasTable(&tables.TableOauthToken{}) {
if err := migrator.DropTable(&tables.TableOauthToken{}); err != nil {
return fmt.Errorf("failed to drop oauth_tokens table: %w", err)
}
}
if migrator.HasTable(&tables.TableOauthConfig{}) {
if err := migrator.DropTable(&tables.TableOauthConfig{}); err != nil {
return fmt.Errorf("failed to drop oauth_configs table: %w", err)
}
}
+
+ // Drop MCP client columns added in this migration
+ if migrator.HasColumn(&tables.TableMCPClient{}, "oauth_config_id") {
+ if err := migrator.DropColumn(&tables.TableMCPClient{}, "oauth_config_id"); err != nil {
+ return fmt.Errorf("failed to drop oauth_config_id column: %w", err)
+ }
+ }
+ if migrator.HasColumn(&tables.TableMCPClient{}, "auth_type") {
+ if err := migrator.DropColumn(&tables.TableMCPClient{}, "auth_type"); err != nil {
+ return fmt.Errorf("failed to drop auth_type column: %w", err)
+ }
+ }
return nil
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // migrationAddOAuthTables creates the oauth_configs and oauth_tokens tables | |
| func migrationAddOAuthTables(ctx context.Context, db *gorm.DB) error { | |
| m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ | |
| ID: "add_disable_db_pings_in_health_column", | |
| ID: "add_oauth_tables", | |
| Migrate: func(tx *gorm.DB) error { | |
| tx = tx.WithContext(ctx) | |
| migrator := tx.Migrator() | |
| if !migrator.HasColumn(&tables.TableClientConfig{}, "disable_db_pings_in_health") { | |
| if err := migrator.AddColumn(&tables.TableClientConfig{}, "disable_db_pings_in_health"); err != nil { | |
| return err | |
| // Updating MCPClient table to add auth_type, oauth_config_id, and oauth_config columns | |
| if !migrator.HasColumn(&tables.TableMCPClient{}, "auth_type") { | |
| if err := migrator.AddColumn(&tables.TableMCPClient{}, "auth_type"); err != nil { | |
| return fmt.Errorf("failed to add auth_type column: %w", err) | |
| } | |
| } | |
| return nil | |
| }, | |
| Rollback: func(tx *gorm.DB) error { | |
| tx = tx.WithContext(ctx) | |
| migrator := tx.Migrator() | |
| if migrator.HasColumn(&tables.TableClientConfig{}, "disable_db_pings_in_health") { | |
| if err := migrator.DropColumn(&tables.TableClientConfig{}, "disable_db_pings_in_health"); err != nil { | |
| return err | |
| if !migrator.HasColumn(&tables.TableMCPClient{}, "oauth_config_id") { | |
| if err := migrator.AddColumn(&tables.TableMCPClient{}, "oauth_config_id"); err != nil { | |
| return fmt.Errorf("failed to add oauth_config_id column: %w", err) | |
| } | |
| } | |
| return nil | |
| }, | |
| }}) | |
| err := m.Migrate() | |
| if err != nil { | |
| return fmt.Errorf("error while running db migration: %s", err.Error()) | |
| } | |
| return nil | |
| } | |
| // migrationAddIsPingAvailableColumn adds the is_ping_available column to the config_mcp_clients table | |
| func migrationAddIsPingAvailableColumn(ctx context.Context, db *gorm.DB) error { | |
| m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ | |
| ID: "add_is_ping_available_column", | |
| Migrate: func(tx *gorm.DB) error { | |
| tx = tx.WithContext(ctx) | |
| migrator := tx.Migrator() | |
| if !migrator.HasColumn(&tables.TableMCPClient{}, "is_ping_available") { | |
| if err := migrator.AddColumn(&tables.TableMCPClient{}, "is_ping_available"); err != nil { | |
| return err | |
| // Set default value for auth_type column | |
| if err := tx.Model(&tables.TableMCPClient{}).Where("auth_type IS NULL").Update("auth_type", "headers").Error; err != nil { | |
| return err | |
| } | |
| // Create oauth_configs table | |
| if !migrator.HasTable(&tables.TableOauthConfig{}) { | |
| if err := migrator.CreateTable(&tables.TableOauthConfig{}); err != nil { | |
| return fmt.Errorf("failed to create oauth_configs table: %w", err) | |
| } | |
| // Set default value for existing rows | |
| if err := tx.Model(&tables.TableMCPClient{}).Where("is_ping_available IS NULL").Update("is_ping_available", true).Error; err != nil { | |
| return err | |
| } | |
| // Create oauth_tokens table | |
| if !migrator.HasTable(&tables.TableOauthToken{}) { | |
| if err := migrator.CreateTable(&tables.TableOauthToken{}); err != nil { | |
| return fmt.Errorf("failed to create oauth_tokens table: %w", err) | |
| } | |
| } | |
| return nil | |
| }, | |
| Rollback: func(tx *gorm.DB) error { | |
| tx = tx.WithContext(ctx) | |
| migrator := tx.Migrator() | |
| if migrator.HasColumn(&tables.TableMCPClient{}, "is_ping_available") { | |
| if err := migrator.DropColumn(&tables.TableMCPClient{}, "is_ping_available"); err != nil { | |
| return err | |
| // Drop tables in reverse order | |
| if migrator.HasTable(&tables.TableOauthToken{}) { | |
| if err := migrator.DropTable(&tables.TableOauthToken{}); err != nil { | |
| return fmt.Errorf("failed to drop oauth_tokens table: %w", err) | |
| } | |
| } | |
| if migrator.HasTable(&tables.TableOauthConfig{}) { | |
| if err := migrator.DropTable(&tables.TableOauthConfig{}); err != nil { | |
| return fmt.Errorf("failed to drop oauth_configs table: %w", err) | |
| } | |
| } | |
| return nil | |
| }, | |
| }}) | |
| err := m.Migrate() | |
| if err != nil { | |
| return fmt.Errorf("error while running is_ping_available migration: %s", err.Error()) | |
| return fmt.Errorf("error while running oauth tables migration: %s", err.Error()) | |
| } | |
| // migrationAddOAuthTables creates the oauth_configs and oauth_tokens tables | |
| func migrationAddOAuthTables(ctx context.Context, db *gorm.DB) error { | |
| m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ | |
| ID: "add_oauth_tables", | |
| Migrate: func(tx *gorm.DB) error { | |
| tx = tx.WithContext(ctx) | |
| migrator := tx.Migrator() | |
| // Updating MCPClient table to add auth_type, oauth_config_id, and oauth_config columns | |
| if !migrator.HasColumn(&tables.TableMCPClient{}, "auth_type") { | |
| if err := migrator.AddColumn(&tables.TableMCPClient{}, "auth_type"); err != nil { | |
| return fmt.Errorf("failed to add auth_type column: %w", err) | |
| } | |
| } | |
| if !migrator.HasColumn(&tables.TableMCPClient{}, "oauth_config_id") { | |
| if err := migrator.AddColumn(&tables.TableMCPClient{}, "oauth_config_id"); err != nil { | |
| return fmt.Errorf("failed to add oauth_config_id column: %w", err) | |
| } | |
| } | |
| // Set default value for auth_type column | |
| if err := tx.Model(&tables.TableMCPClient{}).Where("auth_type IS NULL").Update("auth_type", "headers").Error; err != nil { | |
| return err | |
| } | |
| // Create oauth_configs table | |
| if !migrator.HasTable(&tables.TableOauthConfig{}) { | |
| if err := migrator.CreateTable(&tables.TableOauthConfig{}); err != nil { | |
| return fmt.Errorf("failed to create oauth_configs table: %w", err) | |
| } | |
| } | |
| // Create oauth_tokens table | |
| if !migrator.HasTable(&tables.TableOauthToken{}) { | |
| if err := migrator.CreateTable(&tables.TableOauthToken{}); err != nil { | |
| return fmt.Errorf("failed to create oauth_tokens table: %w", err) | |
| } | |
| } | |
| return nil | |
| }, | |
| Rollback: func(tx *gorm.DB) error { | |
| tx = tx.WithContext(ctx) | |
| migrator := tx.Migrator() | |
| // Drop tables in reverse order | |
| if migrator.HasTable(&tables.TableOauthToken{}) { | |
| if err := migrator.DropTable(&tables.TableOauthToken{}); err != nil { | |
| return fmt.Errorf("failed to drop oauth_tokens table: %w", err) | |
| } | |
| } | |
| if migrator.HasTable(&tables.TableOauthConfig{}) { | |
| if err := migrator.DropTable(&tables.TableOauthConfig{}); err != nil { | |
| return fmt.Errorf("failed to drop oauth_configs table: %w", err) | |
| } | |
| } | |
| // Drop MCP client columns added in this migration | |
| if migrator.HasColumn(&tables.TableMCPClient{}, "oauth_config_id") { | |
| if err := migrator.DropColumn(&tables.TableMCPClient{}, "oauth_config_id"); err != nil { | |
| return fmt.Errorf("failed to drop oauth_config_id column: %w", err) | |
| } | |
| } | |
| if migrator.HasColumn(&tables.TableMCPClient{}, "auth_type") { | |
| if err := migrator.DropColumn(&tables.TableMCPClient{}, "auth_type"); err != nil { | |
| return fmt.Errorf("failed to drop auth_type column: %w", err) | |
| } | |
| } | |
| return nil | |
| }, | |
| }}) | |
| err := m.Migrate() | |
| if err != nil { | |
| return fmt.Errorf("error while running oauth tables migration: %s", err.Error()) | |
| } |
🤖 Prompt for AI Agents
In `@framework/configstore/migrations.go` around lines 3001 - 3060, The rollback
for migrationAddOAuthTables currently drops only oauth tables but leaves the MCP
client columns (auth_type, oauth_config_id and any oauth_config column) added in
the Migrate block; update the Rollback function to also check for and remove
those columns from tables.TableMCPClient (use migrator.HasColumn +
migrator.DropColumn for "auth_type", "oauth_config_id" and "oauth_config" if
present) so the schema is fully reverted and older binaries can operate against
the pre-migration schema.
| clientSecret = regResp.ClientSecret // May be empty for public clients | ||
|
|
||
| logger.Debug("Dynamic client registration successful", "client_id", clientID, "has_secret", clientSecret != "") | ||
| logger.Debug("Dynamic client registration successful: client_id: %s, has_secret: %t", clientID, clientSecret != "") |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the overall structure of the main.go file and locate the logger
cd framework/oauth2 && head -50 main.go && echo "..." && wc -l main.goRepository: maximhq/bifrost
Length of output: 1456
🏁 Script executed:
# Find and examine lines around 371 and 420
sed -n '365,375p' framework/oauth2/main.go && echo "---" && sed -n '415,425p' framework/oauth2/main.goRepository: maximhq/bifrost
Length of output: 700
🏁 Script executed:
# Search for logger usage patterns to understand the pattern throughout the file
rg -A 2 'logger\.(Debug|Info|Warn|Error)' framework/oauth2/main.go | head -100Repository: maximhq/bifrost
Length of output: 1742
🏁 Script executed:
# Find and examine the logger interface definition
find . -name "*.go" -type f | xargs rg -l "type Logger" | head -5Repository: maximhq/bifrost
Length of output: 109
🏁 Script executed:
# Also check if there's any SetLogger function that could shed light on logger type
rg "func SetLogger" framework/oauth2/main.go -A 5Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Examine the logger interface definition
cat core/schemas/logger.goRepository: maximhq/bifrost
Length of output: 1993
Fix structured logging format at lines 371 and 420.
Lines 371 and 420 incorrectly use printf-style format strings with the logger, which implements structured logging (message + variadic args). The %s and %t tokens will not be interpreted as format directives; instead, the arguments will be logged as raw values, creating malformed output. All other logger calls in this file use the correct structured format (message followed by key/value pairs).
Suggested fix (structured fields)
- logger.Debug("Dynamic client registration successful: client_id: %s, has_secret: %t", clientID, clientSecret != "")
+ logger.Debug("Dynamic client registration successful", "client_id", clientID, "has_secret", clientSecret != "")
- logger.Debug("OAuth flow initiated successfully: oauth_config_id: %s, client_id: %s", oauthConfigID, clientID)
+ logger.Debug("OAuth flow initiated successfully", "oauth_config_id", oauthConfigID, "client_id", clientID)🤖 Prompt for AI Agents
In `@framework/oauth2/main.go` at line 371, Replace the printf-style logger.Debug
calls that use "%s" and "%t" with structured logging by removing format verbs
and passing key/value pairs; e.g. update the Dynamic client registration logger
(the logger.Debug call that references clientID and clientSecret) to
logger.Debug("Dynamic client registration successful", "client_id", clientID,
"has_secret", clientSecret != "") and make the analogous change for the other
logger.Debug at line 420 so both use message plus "client_id"/client_secret
boolean key/value fields instead of printf formatting.
4df30b5 to
1800642
Compare
akshaydeo
force-pushed
the
01-24-mcp_auth_server_setup
branch
from
903979e to
15cee1d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@framework/configstore/tables/mcp.go`:
- Line 117: The `return nil` is over-indented; locate the `return nil` inside
the function that contains the preceding if/else block and dedent it one tab so
it aligns with the function body (same indentation level as the if/else), then
run gofmt/clang-format to ensure consistent formatting; verify there are no
leftover tabs/spaces before the statement.
♻️ Duplicate comments (1)
framework/configstore/migrations.go (1)
3037-3055: Rollback is incomplete: MCP client columns not removed.The migration adds
auth_typeandoauth_config_idcolumns toTableMCPClient, but the rollback only drops the OAuth tables without removing these columns. A rollback will leave orphan columns onconfig_mcp_clients, breaking schema consistency.✅ Suggested rollback completion
Rollback: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) migrator := tx.Migrator() // Drop tables in reverse order if migrator.HasTable(&tables.TableOauthToken{}) { if err := migrator.DropTable(&tables.TableOauthToken{}); err != nil { return fmt.Errorf("failed to drop oauth_tokens table: %w", err) } } if migrator.HasTable(&tables.TableOauthConfig{}) { if err := migrator.DropTable(&tables.TableOauthConfig{}); err != nil { return fmt.Errorf("failed to drop oauth_configs table: %w", err) } } + + // Drop MCP client columns added in this migration + if migrator.HasColumn(&tables.TableMCPClient{}, "oauth_config_id") { + if err := migrator.DropColumn(&tables.TableMCPClient{}, "oauth_config_id"); err != nil { + return fmt.Errorf("failed to drop oauth_config_id column: %w", err) + } + } + if migrator.HasColumn(&tables.TableMCPClient{}, "auth_type") { + if err := migrator.DropColumn(&tables.TableMCPClient{}, "auth_type"); err != nil { + return fmt.Errorf("failed to drop auth_type column: %w", err) + } + } return nil },
| c.ToolPricingJSON = "{}" | ||
| } | ||
| return nil | ||
| return nil |
Contributor
There was a problem hiding this comment.
Fix incorrect indentation for return nil.
The return nil statement has an extra tab/indentation level. It should be at the same level as the preceding if/else blocks (function body level).
🔧 Proposed fix
- return nil
+ return nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return nil | |
| return nil |
🤖 Prompt for AI Agents
In `@framework/configstore/tables/mcp.go` at line 117, The `return nil` is
over-indented; locate the `return nil` inside the function that contains the
preceding if/else block and dedent it one tab so it aligns with the function
body (same indentation level as the if/else), then run gofmt/clang-format to
ensure consistent formatting; verify there are no leftover tabs/spaces before
the statement.
Pratham-Mishra04
changed the base branch from
01-24-feat_add_oauth_support_to_mcp
to
graphite-base/1429
akshaydeo
force-pushed
the
graphite-base/1429
branch
from
1800642 to
b31b58f
Compare
akshaydeo
force-pushed
the
01-24-mcp_auth_server_setup
branch
from
15cee1d to
44c643f
Compare
akshaydeo
changed the base branch from
graphite-base/1429
to
01-24-feat_add_oauth_support_to_mcp
16 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@framework/configstore/migrations.go`:
- Around line 3005-3034: The migration adds the oauth_config_id column
(migrator.AddColumn on tables.TableMCPClient with column "oauth_config_id")
before creating the referenced tables, which can fail due to the foreign key;
move the block that creates the referenced tables (migrator.CreateTable for
tables.TableOauthConfig and tables.TableOauthToken) to run before attempting
migrator.AddColumn for "oauth_config_id" (and "auth_type" can remain as-is),
keeping the same tx context (tx.WithContext(ctx)) and error handling.
♻️ Duplicate comments (1)
framework/configstore/migrations.go (1)
3037-3055: Rollback leavesauth_typeandoauth_config_idcolumns behind.The migrate function adds
auth_typeandoauth_config_idcolumns toTableMCPClient, but the rollback only drops the OAuth tables. After rollback, these columns will remain onconfig_mcp_clients, causing schema inconsistency with older code versions.✅ Suggested rollback completion
Rollback: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) migrator := tx.Migrator() // Drop tables in reverse order if migrator.HasTable(&tables.TableOauthToken{}) { if err := migrator.DropTable(&tables.TableOauthToken{}); err != nil { return fmt.Errorf("failed to drop oauth_tokens table: %w", err) } } if migrator.HasTable(&tables.TableOauthConfig{}) { if err := migrator.DropTable(&tables.TableOauthConfig{}); err != nil { return fmt.Errorf("failed to drop oauth_configs table: %w", err) } } + // Drop MCP client columns added in this migration + if migrator.HasColumn(&tables.TableMCPClient{}, "oauth_config_id") { + if err := migrator.DropColumn(&tables.TableMCPClient{}, "oauth_config_id"); err != nil { + return fmt.Errorf("failed to drop oauth_config_id column: %w", err) + } + } + if migrator.HasColumn(&tables.TableMCPClient{}, "auth_type") { + if err := migrator.DropColumn(&tables.TableMCPClient{}, "auth_type"); err != nil { + return fmt.Errorf("failed to drop auth_type column: %w", err) + } + } + return nil },
Comment on lines
3005
to
3034
| Migrate: func(tx *gorm.DB) error { | ||
| tx = tx.WithContext(ctx) | ||
| migrator := tx.Migrator() | ||
| if !migrator.HasColumn(&tables.TableClientConfig{}, "disable_db_pings_in_health") { | ||
| if err := migrator.AddColumn(&tables.TableClientConfig{}, "disable_db_pings_in_health"); err != nil { | ||
| return err | ||
| // Updating MCPClient table to add auth_type, oauth_config_id, and oauth_config columns | ||
| if !migrator.HasColumn(&tables.TableMCPClient{}, "auth_type") { | ||
| if err := migrator.AddColumn(&tables.TableMCPClient{}, "auth_type"); err != nil { | ||
| return fmt.Errorf("failed to add auth_type column: %w", err) | ||
| } | ||
| } | ||
| return nil | ||
| }, | ||
| Rollback: func(tx *gorm.DB) error { | ||
| tx = tx.WithContext(ctx) | ||
| migrator := tx.Migrator() | ||
| if migrator.HasColumn(&tables.TableClientConfig{}, "disable_db_pings_in_health") { | ||
| if err := migrator.DropColumn(&tables.TableClientConfig{}, "disable_db_pings_in_health"); err != nil { | ||
| return err | ||
| if !migrator.HasColumn(&tables.TableMCPClient{}, "oauth_config_id") { | ||
| if err := migrator.AddColumn(&tables.TableMCPClient{}, "oauth_config_id"); err != nil { | ||
| return fmt.Errorf("failed to add oauth_config_id column: %w", err) | ||
| } | ||
| } | ||
| return nil | ||
| }, | ||
| }}) | ||
| err := m.Migrate() | ||
| if err != nil { | ||
| return fmt.Errorf("error while running db migration: %s", err.Error()) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // migrationAddIsPingAvailableColumn adds the is_ping_available column to the config_mcp_clients table | ||
| func migrationAddIsPingAvailableColumn(ctx context.Context, db *gorm.DB) error { | ||
| m := migrator.New(db, migrator.DefaultOptions, []*migrator.Migration{{ | ||
| ID: "add_is_ping_available_column", | ||
| Migrate: func(tx *gorm.DB) error { | ||
| tx = tx.WithContext(ctx) | ||
| migrator := tx.Migrator() | ||
| if !migrator.HasColumn(&tables.TableMCPClient{}, "is_ping_available") { | ||
| if err := migrator.AddColumn(&tables.TableMCPClient{}, "is_ping_available"); err != nil { | ||
| return err | ||
| // Set default value for auth_type column | ||
| if err := tx.Model(&tables.TableMCPClient{}).Where("auth_type IS NULL").Update("auth_type", "headers").Error; err != nil { | ||
| return err | ||
| } | ||
| // Create oauth_configs table | ||
| if !migrator.HasTable(&tables.TableOauthConfig{}) { | ||
| if err := migrator.CreateTable(&tables.TableOauthConfig{}); err != nil { | ||
| return fmt.Errorf("failed to create oauth_configs table: %w", err) | ||
| } | ||
| // Set default value for existing rows | ||
| if err := tx.Model(&tables.TableMCPClient{}).Where("is_ping_available IS NULL").Update("is_ping_available", true).Error; err != nil { | ||
| return err | ||
| } | ||
| // Create oauth_tokens table | ||
| if !migrator.HasTable(&tables.TableOauthToken{}) { | ||
| if err := migrator.CreateTable(&tables.TableOauthToken{}); err != nil { | ||
| return fmt.Errorf("failed to create oauth_tokens table: %w", err) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual migration code context
cd framework/configstore
sed -n '3005,3040p' migrations.goRepository: maximhq/bifrost
Length of output: 1519
🏁 Script executed:
# Find the table structure definitions
find . -name "*.go" -type f | xargs grep -l "TableMCPClient\|TableOauthConfig\|TableOauthToken" | head -10Repository: maximhq/bifrost
Length of output: 429
🏁 Script executed:
# Check the table struct definitions to see FK constraint declarations
rg -n -A20 "type TableMCPClient struct" framework/configstore/tables/Repository: maximhq/bifrost
Length of output: 3061
🏁 Script executed:
# Check how oauth_config_id is defined in the TableMCPClient struct
rg -n "oauth_config_id" framework/configstore/tables/ -B2 -A2Repository: maximhq/bifrost
Length of output: 746
🏁 Script executed:
# Look for other examples of AddColumn with FK constraints in migrations
rg -n "AddColumn.*constraint" framework/configstore/migrations.go -B3 -A3Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Check if GORM defers FK constraint creation or fails immediately
rg -n "AddColumn" framework/configstore/migrations.go -B2 -A2 | head -50Repository: maximhq/bifrost
Length of output: 2165
🏁 Script executed:
# Check the TableOauthConfig definition to confirm it exists and its structure
rg -n "type TableOauthConfig struct" framework/configstore/tables/ -A15Repository: maximhq/bifrost
Length of output: 2716
🏁 Script executed:
# Look for any error handling or special patterns around this specific migration
sed -n '2990,3050p' framework/configstore/migrations.goRepository: maximhq/bifrost
Length of output: 2379
🏁 Script executed:
# Check if there are any tests for this migration that might reveal if it fails
rg -n "migrationAddOAuthTables\|oauth_config_id" framework/configstore/migrations_test.go -B3 -A10Repository: maximhq/bifrost
Length of output: 41
🏁 Script executed:
# Check GORM documentation behavior by looking at similar FK patterns in other migrations
rg -n "foreignKey.*constraint:OnDelete" framework/configstore/tables/ -B2 -A2Repository: maximhq/bifrost
Length of output: 2507
🏁 Script executed:
# Verify if the constraint tag on the column field itself would cause issues
# by checking how GORM handles this in AddColumn operations
rg -n "AddColumn.*oauth_config_id" framework/configstore/migrations.go -B5 -A5Repository: maximhq/bifrost
Length of output: 695
🏁 Script executed:
# Check if there's any database migration framework documentation or patterns
# showing how FK constraints are handled when added vs created
find . -name "*.md" -o -name "*.txt" | xargs grep -l "migration\|constraint" 2>/dev/null | head -5Repository: maximhq/bifrost
Length of output: 93
Reorder migration to create tables before adding constrained columns.
The oauth_config_id column addition (line 3015) includes a foreign key constraint from the struct tag that references the oauth_configs table. However, that table is not created until line 3026. Most databases will reject creating a foreign key constraint to a non-existent table. Move the CreateTable calls for oauth_configs and oauth_tokens before the AddColumn calls.
🤖 Prompt for AI Agents
In `@framework/configstore/migrations.go` around lines 3005 - 3034, The migration
adds the oauth_config_id column (migrator.AddColumn on tables.TableMCPClient
with column "oauth_config_id") before creating the referenced tables, which can
fail due to the foreign key; move the block that creates the referenced tables
(migrator.CreateTable for tables.TableOauthConfig and tables.TableOauthToken) to
run before attempting migrator.AddColumn for "oauth_config_id" (and "auth_type"
can remain as-is), keeping the same tx context (tx.WithContext(ctx)) and error
handling.
44c643f to
9d20b32
Compare
akshaydeo
force-pushed
the
01-24-feat_add_oauth_support_to_mcp
branch
from
b31b58f to
595b1ed
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@framework/configstore/migrations.go`:
- Around line 155-168: The rollback for migrationAddOAuthTables is missing
removal of the two columns it adds; update the rollback path to drop auth_type
and oauth_config_id from TableMCPClient (or add a companion migration that
removes those columns) so Rollback mirrors Migrate exactly—modify
migrationAddOAuthTables (or its rollback helper) to execute ALTER TABLE
TableMCPClient DROP COLUMN auth_type and ALTER TABLE TableMCPClient DROP COLUMN
oauth_config_id and ensure the operations run in the correct order alongside
dropping oauth_tokens and oauth_configs.
Comment on lines
+155
to
168
| if err := migrationAddDisableDBPingsInHealthColumn(ctx, db); err != nil { | ||
| return err | ||
| } | ||
| if err := migrationAddOAuthProxyFields(ctx, db); err != nil { | ||
| if err := migrationAddIsPingAvailableColumnToMCPClientTable(ctx, db); err != nil { | ||
| return err | ||
| } | ||
| if err := migrationAddDisableDBPingsInHealthColumn(ctx, db); err != nil { | ||
| if err := migrationAddToolPricingJSONColumn(ctx, db); err != nil { | ||
| return err | ||
| } | ||
| if err := migrationAddIsPingAvailableColumn(ctx, db); err != nil { | ||
| if err := migrationRemoveServerPrefixFromMCPTools(ctx, db); err != nil { | ||
| return err | ||
| } | ||
| if err := migrationAddOAuthTables(ctx, db); err != nil { | ||
| return err |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Show the current triggerMigrations ordering for the new steps and surrounding context
rg -n -B 2 -A 2 "migrationAddDisableDBPingsInHealthColumn|migrationAddIsPingAvailableColumnToMCPClientTable|migrationAddToolPricingJSONColumn|migrationRemoveServerPrefixFromMCPTools|migrationAddOAuthTables" framework/configstore/migrations.goRepository: maximhq/bifrost
Length of output: 2537
🏁 Script executed:
# Get more context: show the full triggerMigrations function to understand the overall ordering
sed -n '140,180p' framework/configstore/migrations.goRepository: maximhq/bifrost
Length of output: 1294
🏁 Script executed:
# Check if this is part of a stack or has related PR information
git log --oneline -20 --allRepository: maximhq/bifrost
Length of output: 1414
🏁 Script executed:
# Look for any OAuth migration implementation to understand the rollback concern mentioned
rg -n "func migrationAddOAuthTables" framework/configstore/migrations.go | head -1 | cut -d: -f1 | xargs -I {} sed -n '{},$p' framework/configstore/migrations.go | head -50Repository: maximhq/bifrost
Length of output: 2136
Fix OAuth migration rollback asymmetry: add column removal to match migrate function.
The migration ordering at lines 155–168 is logically sound for the column and table additions. However, migrationAddOAuthTables has a critical asymmetry: the Migrate function adds auth_type and oauth_config_id columns to TableMCPClient, but the Rollback function only drops the oauth_configs and oauth_tokens tables without removing those columns. The rollback must remove the columns it added to ensure idempotency.
Additionally, stack context could not be verified from the available git metadata. If this PR is part of a dependent set, confirm that downstream PRs do not assume the final migration order differs from what is committed here.
🤖 Prompt for AI Agents
In `@framework/configstore/migrations.go` around lines 155 - 168, The rollback for
migrationAddOAuthTables is missing removal of the two columns it adds; update
the rollback path to drop auth_type and oauth_config_id from TableMCPClient (or
add a companion migration that removes those columns) so Rollback mirrors
Migrate exactly—modify migrationAddOAuthTables (or its rollback helper) to
execute ALTER TABLE TableMCPClient DROP COLUMN auth_type and ALTER TABLE
TableMCPClient DROP COLUMN oauth_config_id and ensure the operations run in the
correct order alongside dropping oauth_tokens and oauth_configs.
akshaydeo
force-pushed
the
01-24-mcp_auth_server_setup
branch
from
9d20b32 to
cec3f0d
Compare
akshaydeo
force-pushed
the
01-24-feat_add_oauth_support_to_mcp
branch
from
595b1ed to
e3b6ca5
Compare
akshaydeo
changed the base branch from
01-24-feat_add_oauth_support_to_mcp
to
graphite-base/1429
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
1 participant
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.
MCP Gateway Reorganization and Auth Config Feature
This PR reorganizes the MCP Gateway section into a hierarchical structure with two sub-features: MCP Registry and Auth Config. It also adds a new enterprise feature for MCP authentication configuration.
Changes
mcp-gatewaydirectory tomcp-registryto better reflect its specific functionalitymcp-auth-configpage for configuring authentication for MCP serversType of change
Affected areas
How to test
Screenshots/Recordings
N/A
Breaking changes
Related issues
N/A
Security considerations
The new Auth Config feature will help improve security for MCP connections when implemented in enterprise environments.
Checklist