fix: composite mcp servers #5317
Open
+633
−517
Conversation
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
87ae71c to
a865690
Compare
Keep component MCPServers and MCPServerInstances up-to-date with the composite MCPServer's manifest in a controller handler instead of API handlers. Addresses: - obot-platform#5127 - obot-platform#5084 Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
e803f49 to
b145bb4
Compare
Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
Addresses obot-platform#4929 Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
entries This change allows composite component servers to be launched after their original CatalogEntry has been deleted. Addresses obot-platform#5272 Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
Addresses obot-platform#4975 Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
84392d7 to
fafeda9
Compare
Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
thedadams
reviewed
Dec 22, 2025
| URLTemplate string `json:"urlTemplate,omitempty"` // URL template for user URLs | ||
| Hostname string `json:"hostname,omitempty"` // Optional: Hostname constraint the URL conforms to | ||
| Headers []MCPHeader `json:"headers,omitempty"` // Optional | ||
|
|
| golang.org/x/net v0.47.0 // indirect | ||
| golang.org/x/oauth2 v0.30.0 | ||
| golang.org/x/sync v0.19.0 // indirect | ||
| golang.org/x/sync v0.19.0 |
Contributor
There was a problem hiding this comment.
I'm surprised that go mod tidy doesn't do this, but if these are direct dependencies, then they should be in the first require block.
Comment on lines
+187
to
+190
| // Remove and recreate the credential to update it | ||
| if err := gptClient.DeleteCredential(ctx, existing.Context, cred.ToolName); err != nil && !errors.As(err, &gptscript.ErrNotFound{}) { | ||
| return false, fmt.Errorf("failed to remove existing credential: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
nit:
Suggested change
| // Remove and recreate the credential to update it | |
| if err := gptClient.DeleteCredential(ctx, existing.Context, cred.ToolName); err != nil && !errors.As(err, &gptscript.ErrNotFound{}) { | |
| return false, fmt.Errorf("failed to remove existing credential: %w", err) | |
| } |
Comment on lines
+529
to
+540
| if err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { | ||
| var latestServer v1.MCPServer | ||
| if err := req.Get(&latestServer, compositeServer.Namespace, existingServer.Name); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| latestServer.Spec.Manifest = component.Manifest | ||
| latestServer = withNeedsURL(latestServer) | ||
| return req.Client.Update(req.Ctx, &latestServer) | ||
| }); err != nil { | ||
| return fmt.Errorf("failed to update existing component server: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
If the updating of component servers was done in a different handler (that operated on the component itself), then we wouldn't need to retry here. We could just return the error and have the controller framework handle the retrying.
|
|
||
| // listCompositeDeletionDependencies lists the composite MCP servers and catalog entries that depend on the given multi-user server. | ||
| func (m *MCPHandler) listCompositeDeletionDependencies(req api.Context, server v1.MCPServer) ([]compositeDeletionDependency, error) { | ||
| func (*MCPHandler) listCompositeDeletionDependencies(req api.Context, server v1.MCPServer) ([]compositeDeletionDependency, error) { |
Contributor
There was a problem hiding this comment.
Suggested change
| func (*MCPHandler) listCompositeDeletionDependencies(req api.Context, server v1.MCPServer) ([]compositeDeletionDependency, error) { | |
| func listCompositeDeletionDependencies(req api.Context, server v1.MCPServer) ([]compositeDeletionDependency, error) { |
| parentComps[comp.ComponentID()] = i | ||
| var ( | ||
| manifestChanged bool | ||
| oldCompositeServer = compositeServer.DeepCopy() |
Contributor
There was a problem hiding this comment.
If there is a reason why we are deep copying here, then can we add a comment about that? If there isn't, then can we remove it?
| // triggerCompositeUpdate upgrades a composite server and all its component servers from the latest catalog entry | ||
| func (m *MCPHandler) triggerCompositeUpdate(req api.Context, server v1.MCPServer, entry v1.MCPServerCatalogEntry) error { | ||
| func (m *MCPHandler) triggerCompositeUpdate(req api.Context, compositeServer v1.MCPServer, entry v1.MCPServerCatalogEntry) error { | ||
| oldServer := compositeServer.DeepCopy() |
Contributor
There was a problem hiding this comment.
Same comment here about deep copying.
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.
Addresses:
update requiredcontinues to be present after save is done (until manual refresh) #4929Update Serveroption presented for connected MCP server instance after upgrading composite MCP server for changes made to remote MCP server. #5084GithubMCP server is added to it because of validating errors relating to hostname. #5214