fix: remove resolution parameter from image generation and add MCP client config fields#2041
Merged
Pratham-Mishra04 merged 1 commit intomainfrom Mar 13, 2026
Merged
Conversation
18 tasks
|
|
12 tasks
18 tasks
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRemoved the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP_Handler as "Bifrost HTTP Handler"
participant ConfigStore
participant MCP_Runtime as "mcp Runtime"
Client->>HTTP_Handler: Create MCP client request (OAuth or direct)
alt OAuth flow
HTTP_Handler->>ConfigStore: create pending MCPClientConfig (ToolSyncInterval, IsPingAvailable)
HTTP_Handler-->>Client: return OAuth redirect
Client->>HTTP_Handler: OAuth callback
HTTP_Handler->>ConfigStore: finalize pending -> store final MCPClientConfig
HTTP_Handler->>MCP_Runtime: start/connect with final MCPClientConfig
else Non-OAuth flow
HTTP_Handler->>ConfigStore: derive ToolSyncInterval (request or stored or mcp.DefaultToolSyncInterval)
HTTP_Handler->>ConfigStore: store MCPClientConfig (ToolSyncInterval, IsPingAvailable)
HTTP_Handler->>MCP_Runtime: start/connect with MCPClientConfig
end
MCP_Runtime->>HTTP_Handler: optional ping/health checks (if IsPingAvailable)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Pratham-Mishra04
force-pushed
the
02-26-refactor_pricing_module_refactor
branch
from
5e6a6b8 to
c017629
Compare
Pratham-Mishra04
force-pushed
the
03-12-fix_minor_fixes
branch
from
683eaa2 to
bb7602f
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 243-255: The default for IsPingAvailable in the OAuth branch is
currently set to false, causing pending OAuth-created clients to skip pings;
change the default to true and only override it when req.IsPingAvailable is
non-nil (i.e., set isPingAvailable := true then if req.IsPingAvailable != nil {
isPingAvailable = *req.IsPingAvailable }), so the pendingConfig
(schemas.MCPClientConfig with field IsPingAvailable) matches the direct-create
default behavior.
- Around line 238-241: The OAuth create path currently sets toolSyncInterval to
mcp.DefaultToolSyncInterval whenever req.ToolSyncInterval == 0, ignoring any
instance override; change the logic in the block that assigns toolSyncInterval
so that if req.ToolSyncInterval == 0 you first check
instance.MCPToolSyncInterval (and use it if non-zero, converting to
time.Duration * time.Minute), otherwise fall back to
mcp.DefaultToolSyncInterval; update the assignment that uses
req.ToolSyncInterval to only multiply by time.Minute when req.ToolSyncInterval
is non-zero and ensure the final toolSyncInterval variable reflects the instance
override when 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: 6406fcd8-2dc1-4b9c-ab65-e6d39f3d147b
📒 Files selected for processing (4)
core/providers/replicate/images.gocore/providers/replicate/types.gocore/schemas/images.gotransports/bifrost-http/handlers/mcp.go
💤 Files with no reviewable changes (3)
- core/providers/replicate/images.go
- core/schemas/images.go
- core/providers/replicate/types.go
Pratham-Mishra04
force-pushed
the
02-26-refactor_pricing_module_refactor
branch
from
c017629 to
dbf2248
Compare
Pratham-Mishra04
force-pushed
the
03-12-fix_minor_fixes
branch
from
bb7602f to
546b43d
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
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)
423-441:⚠️ Potential issue | 🟠 MajorResolve
toolSyncIntervalbefore the DB write, or rollback on lookup failure.
UpdateMCPClientConfignow commits the DB change before the newGetClientConfigcall. If that lookup fails on Line 435, the handler returns with the DB updated and the in-memory MCP client still on the old config; the existing rollback only coversh.mcpManager.UpdateMCPClientfailures. This new error path can leave storage and runtime out of sync.🤖 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 423 - 441, The handler currently updates the DB (h.store.ConfigStore.UpdateMCPClientConfig) before resolving toolSyncInterval via h.store.ConfigStore.GetClientConfig, which can leave DB and in-memory MCP out of sync if GetClientConfig fails; either compute toolSyncInterval first (use req.ToolSyncInterval if set, otherwise call GetClientConfig before calling UpdateMCPClientConfig) or, if you must keep the DB-write-first order, perform a compensating rollback on lookup failure by re-writing the previous client config back to the store (capture the old config via GetClientConfig at the start) and return the error; update the flow around UpdateMCPClientConfig, GetClientConfig, toolSyncInterval and h.mcpManager.UpdateMCPClient to ensure the DB write and in-memory update remain consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 238-250: The handler currently replaces a missing
req.ToolSyncInterval (0) with the resolved instance default and writes that
concrete value into the stored client config, which pins the client; instead,
preserve 0 in stored config while still resolving a runtime duration for
in-memory use. Concretely: keep the stored field (e.g.,
schemas.MCPClientConfig.ToolSyncInterval / pendingConfig) set to
req.ToolSyncInterval (leave as 0 when omitted), and introduce a separate runtime
variable (e.g., toolSyncIntervalResolved or reuse local toolSyncInterval only
for runtime) that, when req.ToolSyncInterval == 0, fetches
h.store.ConfigStore.GetClientConfig(ctx) and sets the runtime duration from
config.MCPToolSyncInterval; only use the resolved duration for runtime logic but
do not write it back into pendingConfig/ schemasConfig. Apply this change in the
tool_sync_interval handling blocks referenced (around the toolSyncInterval,
req.ToolSyncInterval, h.store.ConfigStore.GetClientConfig usage at the shown
ranges).
---
Outside diff comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 423-441: The handler currently updates the DB
(h.store.ConfigStore.UpdateMCPClientConfig) before resolving toolSyncInterval
via h.store.ConfigStore.GetClientConfig, which can leave DB and in-memory MCP
out of sync if GetClientConfig fails; either compute toolSyncInterval first (use
req.ToolSyncInterval if set, otherwise call GetClientConfig before calling
UpdateMCPClientConfig) or, if you must keep the DB-write-first order, perform a
compensating rollback on lookup failure by re-writing the previous client config
back to the store (capture the old config via GetClientConfig at the start) and
return the error; update the flow around UpdateMCPClientConfig, GetClientConfig,
toolSyncInterval and h.mcpManager.UpdateMCPClient to ensure the DB write and
in-memory update remain consistent.
🪄 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: 9b1adbda-d7cb-44e5-8810-4416d18789f3
📒 Files selected for processing (4)
core/providers/replicate/images.gocore/providers/replicate/types.gocore/schemas/images.gotransports/bifrost-http/handlers/mcp.go
💤 Files with no reviewable changes (3)
- core/providers/replicate/images.go
- core/providers/replicate/types.go
- core/schemas/images.go
TejasGhatte
changed the base branch from
02-26-refactor_pricing_module_refactor
to
graphite-base/2041
Pratham-Mishra04
force-pushed
the
graphite-base/2041
branch
from
dbf2248 to
76d7e6d
Compare
Pratham-Mishra04
force-pushed
the
03-12-fix_minor_fixes
branch
from
546b43d to
3ce8729
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 the current code and only fix it if needed.
Inline comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 238-250: The code currently overwrites toolSyncInterval with zero
when config.MCPToolSyncInterval is unset; change the assignment logic in the
handler so you only set toolSyncInterval =
time.Duration(config.MCPToolSyncInterval) * time.Minute when config != nil &&
config.MCPToolSyncInterval != 0, keeping the default
(mcp.DefaultToolSyncInterval) otherwise; apply this same non-zero check pattern
wherever toolSyncInterval is computed from config (the branch that checks
req.ToolSyncInterval and the non-OAuth and update paths) so that a zero config
value does not replace the default.
🪄 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: 35e0ff3f-8a36-4f6a-b3af-145d37549c13
📒 Files selected for processing (4)
core/providers/replicate/images.gocore/providers/replicate/types.gocore/schemas/images.gotransports/bifrost-http/handlers/mcp.go
💤 Files with no reviewable changes (3)
- core/providers/replicate/types.go
- core/providers/replicate/images.go
- core/schemas/images.go
Pratham-Mishra04
force-pushed
the
03-12-fix_minor_fixes
branch
from
3ce8729 to
5120bd4
Compare
Pratham-Mishra04
force-pushed
the
graphite-base/2041
branch
from
76d7e6d to
1df9e09
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
transports/bifrost-http/handlers/mcp.go (1)
238-249:⚠️ Potential issue | 🟠 MajorKeep
tool_sync_intervalsplit into stored override vs runtime-resolved value (and guard zero global defaults).Create flows still persist the resolved interval into config (
pendingConfig/schemasConfig), which pins clients to today’s global default instead of preserving0as “inherit global default.” Also,config.MCPToolSyncInterval == 0currently overwritesmcp.DefaultToolSyncIntervalto0.Proposed fix pattern
- toolSyncInterval := mcp.DefaultToolSyncInterval + toolSyncIntervalRuntime := mcp.DefaultToolSyncInterval + storedToolSyncInterval := time.Duration(req.ToolSyncInterval) * time.Minute if req.ToolSyncInterval != 0 { - toolSyncInterval = time.Duration(req.ToolSyncInterval) * time.Minute + toolSyncIntervalRuntime = storedToolSyncInterval } else { config, err := h.store.ConfigStore.GetClientConfig(ctx) if err != nil { ... } - if config != nil { - toolSyncInterval = time.Duration(config.MCPToolSyncInterval) * time.Minute + if config != nil && config.MCPToolSyncInterval != 0 { + toolSyncIntervalRuntime = time.Duration(config.MCPToolSyncInterval) * time.Minute } } // persisted config (DB/pending): preserve override semantics - ToolSyncInterval: toolSyncInterval, + ToolSyncInterval: storedToolSyncInterval, // runtime add/update call: use resolved runtime value - ToolSyncInterval: toolSyncInterval, + ToolSyncInterval: toolSyncIntervalRuntime,Also applies to: 259-265, 294-327, 429-441
🤖 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 238 - 249, The code is resolving a runtime toolSyncInterval and then writing that resolved value into stored configs (pendingConfig/schemasConfig), which loses the distinction between an explicit stored override and "inherit global default" (0) and also allows config.MCPToolSyncInterval == 0 to overwrite mcp.DefaultToolSyncInterval; fix by keeping two values: (1) the stored override field (leave req.ToolSyncInterval or config.MCPToolSyncInterval as-is, preserving 0 to mean inherit) and (2) a separately computed runtimeToolSyncInterval used for behavior (compute runtimeToolSyncInterval = req.ToolSyncInterval != 0 ? req.ToolSyncInterval : (config != nil && config.MCPToolSyncInterval != 0 ? config.MCPToolSyncInterval : mcp.DefaultToolSyncInterval)); update code paths that persist pendingConfig/schemasConfig to write the stored override variable (not runtimeToolSyncInterval) and ensure comparisons/guards check for zero before substituting DefaultToolSyncInterval; refer to toolSyncInterval, req.ToolSyncInterval, config.MCPToolSyncInterval, pendingConfig, schemasConfig, and mcp.DefaultToolSyncInterval when applying this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@transports/bifrost-http/handlers/mcp.go`:
- Around line 238-249: The code is resolving a runtime toolSyncInterval and then
writing that resolved value into stored configs (pendingConfig/schemasConfig),
which loses the distinction between an explicit stored override and "inherit
global default" (0) and also allows config.MCPToolSyncInterval == 0 to overwrite
mcp.DefaultToolSyncInterval; fix by keeping two values: (1) the stored override
field (leave req.ToolSyncInterval or config.MCPToolSyncInterval as-is,
preserving 0 to mean inherit) and (2) a separately computed
runtimeToolSyncInterval used for behavior (compute runtimeToolSyncInterval =
req.ToolSyncInterval != 0 ? req.ToolSyncInterval : (config != nil &&
config.MCPToolSyncInterval != 0 ? config.MCPToolSyncInterval :
mcp.DefaultToolSyncInterval)); update code paths that persist
pendingConfig/schemasConfig to write the stored override variable (not
runtimeToolSyncInterval) and ensure comparisons/guards check for zero before
substituting DefaultToolSyncInterval; refer to toolSyncInterval,
req.ToolSyncInterval, config.MCPToolSyncInterval, pendingConfig, schemasConfig,
and mcp.DefaultToolSyncInterval when applying this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 548ef5a0-0a98-4736-84fb-686dc3e80dd4
📒 Files selected for processing (4)
core/providers/replicate/images.gocore/providers/replicate/types.gocore/schemas/images.gotransports/bifrost-http/handlers/mcp.go
💤 Files with no reviewable changes (3)
- core/providers/replicate/types.go
- core/schemas/images.go
- core/providers/replicate/images.go
danpiths
approved these changes
Mar 13, 2026
Collaborator
Author
Merge activity
|
Pratham-Mishra04
force-pushed
the
03-12-fix_minor_fixes
branch
from
5120bd4 to
e6da1e0
Compare
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
3 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
Removes the
Resolutionparameter from image generation functionality across the Replicate provider and core schemas, while adding support for ping availability and tool sync interval configuration in MCP client setup.Changes
Resolutionfield fromImageGenerationParametersin core schemasResolutionfield fromReplicatePredictionRequestInputstruct and its JSON unmarshaling logicToReplicateImageGenerationInputfunctionIsPingAvailableandToolSyncIntervalconfiguration support in MCP client handler with proper defaultsType of change
Affected areas
How to test
Validate image generation still works without resolution parameter and MCP client configuration accepts new parameters:
Test image generation requests to ensure they work without the resolution parameter. Test MCP client creation with and without the new
IsPingAvailableandToolSyncIntervalparameters to verify proper defaults are applied.Screenshots/Recordings
N/A
Breaking changes
The removal of the
Resolutionparameter from image generation requests is a breaking change. Any clients currently sending this parameter will need to remove it from their requests. The parameter will be ignored if sent.Related issues
N/A
Security considerations
No security implications identified. The changes involve parameter removal and configuration additions without affecting authentication or sensitive data handling.
Checklist
docs/contributing/README.mdand followed the guidelines