Use unified extensions integrations catalog#757
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean architectural refactoring with proper abstraction layers
[ARCHITECTURAL IMPROVEMENTS]
- Elegant migration from single
templateto flexibleconnectionOptionsmodel - Well-designed utility functions (
getMcpConnectionOptions,getDefaultMcpConnectionOption,getDefaultMcpTransport) provide clear abstractions - UUID generation improved from
Date.now()touuidv4()for proper uniqueness findInstalledEntryMatchcorrectly checks all connection options, not just the default
[TESTING]
- Comprehensive test coverage across 6 test files
- All call sites updated consistently (20 files)
- Tests properly exercise the new
connectionOptionsstructure
[MINOR OBSERVATIONS]
- PR depends on upstream OpenHands/extensions#266 (already noted in description)
- The
isCredentialOptionalfunction has slight duplication checkingapiKeyOptionalin two places, but this is acceptable given it handles different transport types (stdio vs http)
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This is a significant refactor touching 20 files and changing core data structures, but risk is mitigated by:
- Comprehensive test coverage with all tests updated
- Consistent migration across all call sites
- Clear utility functions maintaining backward-compatible behavior
- Documentation kept in sync
- Coordinated with upstream dependency change
Main risk: Depends on upstream PR being merged first. Recommend verifying OpenHands/extensions#266 is merged before merging this.
VERDICT:
✅ Worth merging: Solid architectural improvement with excellent test coverage
KEY INSIGHT:
The migration from a single-template model to multiple connection options per integration is a clean abstraction that improves flexibility while maintaining backward compatibility through well-designed utility functions.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/agent-canvas/actions/runs/26412412504
neubig
force-pushed
the
codex/use-integrations-catalog
branch
from
1615be9 to
a59f098
Compare
neubig
added
the
update-snapshots
neubig
force-pushed
the
codex/use-integrations-catalog
branch
from
a59f098 to
0c396fb
Compare
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean architectural refactoring with proper abstraction layers
[ARCHITECTURAL IMPROVEMENTS]
- Elegant migration from single
templateto flexibleconnectionOptionsmodel - Well-designed utility functions (
getMcpConnectionOptions,getDefaultMcpConnectionOption,getDefaultMcpTransport) provide clear abstractions - UUID generation improved from
Date.now()touuidv4()for proper uniqueness findInstalledEntryMatchcorrectly checks all connection options, not just the default — handles cases where services offer multiple protocols (OAuth-hosted MCP + API/stdio)- Auth strategy properly separated from transport type via
optionNeedsCredentialFieldandisCredentialOptionalhelpers
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
This is an internal architectural refactoring that improves maintainability without breaking external contracts. The migration is well-tested with comprehensive unit and snapshot coverage. The connectionOptions model eliminates special cases and provides a cleaner foundation for supporting integrations with multiple connection methods.
VERDICT:
✅ Worth merging: Core logic is sound, architecture is improved, comprehensive test coverage
KEY INSIGHT:
The unified connectionOptions model is the right abstraction — it eliminates the need for special-case logic when an integration supports multiple protocols, making the system more maintainable as the marketplace grows.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/agent-canvas/actions/runs/26412940610
neubig
force-pushed
the
codex/use-integrations-catalog
branch
from
0c396fb to
dd9f9b3
Compare
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Clean architectural refactoring with proper abstractions
[IMPROVEMENT OPPORTUNITIES]
- [tests/routes/mcp-page.test.tsx, Lines 91-95] Test Inconsistency: This test expects Slack to render a URL field with the OAuth endpoint, but
getInstallableMcpConnectionOption()should select the stdio "api" fallback (SLACK_BOT_TOKEN/SLACK_TEAM_ID fields). The snapshot test and install-server-modal test both expect the stdio form fields. This test's expectations were incorrectly updated during the refactoring.
[ARCHITECTURAL IMPROVEMENTS]
- Elegant migration from single
templateto flexibleconnectionOptionsmodel - Well-designed utility functions (
getMcpConnectionOptions,getDefaultMcpConnectionOption,getInstallableMcpConnectionOption,getDefaultMcpTransport) provide clear abstractions - UUID generation improved from
Date.now()touuidv4()for proper uniqueness findInstalledEntryMatchcorrectly checks all connection options, not just the default- Smart fallback logic: OAuth-first entries like Slack correctly fall back to API/stdio for local installation
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This is a significant refactoring of marketplace data structures that changes how MCP servers are matched and installed. Well-tested and architecturally sound, but depends on an external PR (OpenHands/extensions#266) and touches many files. The test inconsistency should be fixed to ensure correctness.
VERDICT:
✅ Worth merging after fixing the test inconsistency - The core architectural direction is sound and the implementation is clean
KEY INSIGHT:
The connectionOptions model is the right abstraction for supporting multiple authentication methods per integration, but test expectations must remain consistent across unit and snapshot tests.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/agent-canvas/actions/runs/26415389948
| expect(screen.getByTestId("mcp-install-field-url")).toHaveValue( | ||
| "https://mcp.slack.com/mcp", | ||
| ); | ||
| expect(screen.queryByTestId("mcp-install-field-api_key")).toBeNull(); |
There was a problem hiding this comment.
🟠 Important: Test expectations are inconsistent with actual behavior.
This test expects Slack to render a URL field (https://mcp.slack.com/mcp - the OAuth endpoint), but InstallServerModal uses getInstallableMcpConnectionOption() which intelligently prefers the stdio "api" fallback for local installation (see __tests__/utils/mcp-marketplace-utils.test.ts:94-99).
The modal should render SLACK_BOT_TOKEN and SLACK_TEAM_ID fields, as verified by:
- The snapshot test (
tests/e2e/snapshots/mcp-page.snapshot.spec.ts:217-223) - The install modal unit test (
__tests__/components/features/mcp-page/install-server-modal.test.tsx:35- "uses Slack's API fallback when the default option is OAuth")
These expectations were incorrectly updated during the refactoring. The test should expect:
expect(screen.getByTestId("mcp-install-field-SLACK_BOT_TOKEN")).toBeInTheDocument();
expect(screen.getByTestId("mcp-install-field-SLACK_TEAM_ID")).toBeInTheDocument();
expect(screen.queryByTestId("mcp-install-field-url")).toBeNull();
neubig
force-pushed
the
codex/use-integrations-catalog
branch
from
dd9f9b3 to
24e1f13
Compare
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean architectural refactoring with proper abstraction layers
Key Improvements
- Flexible connection model: Elegant migration from single
templateto multi-optionconnectionOptionsarchitecture enables services like Slack to offer both OAuth (default for cloud) and API/stdio (for local installs) - Smart fallback logic:
getInstallableMcpConnectionOption()intelligently prefers locally-installable options (non-OAuth) when the default requires hosted auth - Comprehensive matching:
findInstalledEntryMatch()now checks ALL connection options instead of just the default, fixing edge cases where installed servers wouldn't match their catalog entry - Proper UUID generation: Replaced
Date.now()withuuidv4()for collision-free server IDs - Test coverage: Tests verify the Slack OAuth→stdio fallback scenario, multi-option matching, and catalog migration
Review Notes
Unresolved thread from previous review: The test expectation issue at __tests__/routes/mcp-page.test.tsx:101 appears to be resolved. The test now correctly expects stdio form fields (SLACK_BOT_TOKEN, SLACK_TEAM_ID) with no URL/api_key fields, which aligns with getInstallableMcpConnectionOption() selecting the API fallback over the OAuth default.
Snapshot updates: The update-snapshots label is appropriate - the marketplace content changed intentionally due to the catalog migration.
Documentation: AGENTS.md correctly documents the migration from @openhands/extensions/mcps to @openhands/extensions/integrations.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
This is a well-structured refactoring with comprehensive test coverage. The changes are contained to the MCP marketplace integration layer and don't affect agent behavior, core conversation logic, or external APIs. Test coverage verifies the new multi-option logic and fallback scenarios. No breaking changes to existing functionality.
VERDICT:
✅ Worth merging - Solid architectural improvement with proper tests and documentation
KEY INSIGHT:
The multi-option architecture elegantly solves the "cloud-first, local-fallback" integration pattern - services can default to OAuth while still offering API/stdio alternatives for self-hosted deployments.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/agent-canvas/actions/runs/26415749360
| function isLocallyInstallableMcpOption( | ||
| option: McpMarketplaceConnectionOption, | ||
| ): boolean { | ||
| return option.auth.strategy !== "oauth2"; |
There was a problem hiding this comment.
Could you please explain this? Why is OAuth not locally installable?
There was a problem hiding this comment.
Added a code comment at the decision point in 71bda6c.
The distinction I intended is: this local install path only writes static MCP server config into the user settings. OAuth catalog options describe hosted/redirect flows, which need an OAuth callback handler and provider coordination rather than a static raw MCP config form. So when an entry defaults to OAuth but also exposes a direct API/stdio MCP option, the local install modal chooses that static option; OAuth remains the default connection option for hosted/managed integration flows.
The integrations naming is meant as the public extensions catalog category for external capabilities, not as a dependency on the separate Integrations Hub product/repo. Integrations Hub and agent-canvas now consume the same public catalog data for their respective surfaces.
There was a problem hiding this comment.
Thank you.
The integrations naming is meant as the public extensions catalog category for external capabilities, not as a dependency on the separate Integrations Hub product/repo.
I think maybe this is confusing; will people make the difference if we name different things with the same name?
Related Slack conversation
So this used to be named “MCP” -> then “Integrations” -> then “MCP” was restored.
Could we please pause and reconsider this? Why would it be named “Integrations” if what is contains is only MCPs?
There was a problem hiding this comment.
What are “hosted integrations” in the comment above, could you give an example?
Are we talking here about connecting to Notion MCP which, I think, recommends only OAuth?
There was a problem hiding this comment.
Thank you for this PR, it's interesting. I left a quick question inline.
Is this documented somewhere? How is this supposed to work?
What worries me a bit is that Integrations Hub is a proprietary/closed repo, while this PR appears to reuse the name Integrations, which may be confusing people; in addition, it seems to suggest that MCPs that need OAuth are not installable locally? Sorry I'm confused too.
I'd love some help to understand the full picture we're looking towards.
It may be possible to combine, but I worry a lot about details if so.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean architectural refactoring with well-designed abstractions
Addressing Unresolved Questions
Re: OAuth not locally installable (line 37)
The logic at line 37 (option.auth.strategy !== "oauth2") is correct. OAuth is excluded from "locally installable" options because:
- Requires browser redirect flow and user interaction
- Needs a web server to receive callbacks
- Depends on network access to OAuth providers
For local installations, stdio/API options that accept direct credentials are more practical. The code elegantly handles this by preferring locally-installable fallbacks when the default option is OAuth (see getInstallableMcpConnectionOption).
Re: Test inconsistency (line 101)
This appears resolved. The test now correctly expects Slack's stdio fields (SLACK_BOT_TOKEN, SLACK_TEAM_ID) and explicitly verifies OAuth fields (url, api_key) are NOT present. This matches the actual behavior of getInstallableMcpConnectionOption.
Key Improvements
- Flexible connection model: Services like Slack can now offer both OAuth (default for cloud) and API/stdio (for local installs)
- Smart fallback logic:
getInstallableMcpConnectionOption()intelligently prefers locally-installable options - Backward compatibility: Existing installed servers matched via
findInstalledEntryMatchchecking all connection options - Clean utilities: Well-named helper functions with clear responsibilities
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Internal architectural refactoring with comprehensive test coverage. Changes are isolated to MCP catalog handling with backward compatibility for existing installations. No user-facing breaking changes.
VERDICT:
✅ Approved - Solid refactoring that adds flexibility without breaking existing functionality.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/agent-canvas/actions/runs/26418977998
📸 Snapshot Test ReportWarning Snapshot comparison step crashed (timeout, OOM, or runner error) — diff results below may be incomplete or absent. ✅ 8 snapshots changed — acknowledged via the
🔴 Changed snapshots (8)
|
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
changes-tab
changes-diff-viewer
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
mcp-page — 5 snapshots
mcp-custom-server-1-editor-open
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
mcp-custom-server-editor
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
mcp-empty-installed
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
mcp-search-filtered
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
mcp-slack-install-1-marketplace
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
onboarding
onboarding-step-3-say-hello
| Expected (main) | Actual (PR) | Diff |
|---|---|---|
 |
 |
 |
✅ Unchanged snapshots (65)
archived-conversation
- conversation-panel-with-archived-badges
- conversation-view-archived
- conversation-view-sandbox-error
automations
- automations-delete-modal
- automations-list-active-inactive
- automations-search-no-results
backends-extended
- backend-add-blank-disabled
- backend-add-cloud-advanced-open
- backend-add-cloud-no-key-disabled
- backend-add-cloud-with-key-enabled
- backend-add-form-partially-filled
- backend-add-invalid-url-disabled
- backend-add-local-ready
- backend-add-name-only-disabled
- backend-add-two-column-layout
- backend-add-whitespace-host-disabled
- backend-after-switch
- backend-cancel-nothing-saved
- backend-dropdown-two-backends
- backend-edit-prefilled
- backend-manage-after-removal
- backend-manage-two-listed
- backend-remove-cancelled
- backend-remove-confirmation
- backend-switch-overlay
backends
- backend-add-modal
- backend-manage-modal
- backend-selector-open
changes-tab
- changes-deleted-file
- changes-empty
collapsible-thinking
- reasoning-content-collapsed
- reasoning-content-expanded
- think-action-collapsed
- think-action-expanded
mcp-page
- mcp-custom-server-2-url-filled
- mcp-custom-server-3-all-filled
- mcp-custom-server-4-installed
- mcp-slack-install-2-modal
- mcp-slack-install-3-filled
- mcp-slack-install-4-installed
onboarding
- onboarding-step-0-choose-agent
- onboarding-step-1-check-backend
- onboarding-step-2-setup-llm
projects-workspace-browser
- projects-workspace-browser
settings-page
- add-backend-modal
- analytics-consent-modal
- home-screen
- settings-app-page
- settings-page
settings-secrets
- secrets-add-form-filled
- secrets-add-form
- secrets-after-save
- secrets-delete-confirm
- secrets-list
settings-verification
- condenser-settings
- verification-settings-off
- verification-settings-on
sidebar
- sidebar-collapsed
- sidebar-conversation-panel
- sidebar-filter-menu
skills-page
- skills-empty
- skills-loaded
- skills-no-match
- skills-search-filtered
- skills-type-filter
Generated by the Snapshot Tests workflow. This comment was created by an AI agent (OpenHands) on behalf of the repo maintainers.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste - Clean architectural refactoring with well-designed abstractions
Summary
This PR successfully migrates from the old single-template MCP structure to a flexible connectionOptions model. The new architecture elegantly handles multiple connection methods per integration (OAuth, API, stdio) and intelligently selects locally-installable options when available.
Key Improvements:
- Flexible
connectionOptionsmodel supports multiple auth/transport combinations per integration - Smart fallback logic:
getInstallableMcpConnectionOption()prefers non-OAuth options for local installs - Well-designed utility functions with clear single responsibilities
- Comprehensive test coverage for common scenarios
- UUID generation improved from
Date.now()touuidv4() - All catalog usages updated consistently across the codebase
Unresolved Threads Addressed:
- OAuth not locally installable: Code comment added explaining browser-redirect limitation
- Test expectations: Fixed - tests now correctly expect stdio fallback fields
Agent Behavior Impact: This is primarily a frontend/data-structure refactor. The actual MCP runtime integration and agent tool execution paths are unchanged. Only the UI for installing and matching MCP servers is affected.
[IMPROVEMENT OPPORTUNITIES]
I've identified a few edge cases worth hardening (see inline comments). These are defensive improvements rather than blocking issues - the current catalog data doesn't trigger them, but defensive handling would make the code more robust.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Well-executed frontend refactor with no changes to agent reasoning, tool execution, or runtime behavior. The migration is internally consistent and comprehensively tested. No breaking changes to public APIs or agent capabilities.
VERDICT:
✅ Worth merging - Clean architecture with excellent test coverage for common scenarios. Optional edge-case hardening suggested inline.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/agent-canvas/actions/runs/26419689745
| if (defaultOption && isLocallyInstallableMcpOption(defaultOption)) { | ||
| return defaultOption; | ||
| } | ||
| return options.find(isLocallyInstallableMcpOption) ?? defaultOption; |
There was a problem hiding this comment.
🟡 Suggestion: Fallback behavior may return OAuth option
getInstallableMcpConnectionOption returns defaultOption as final fallback even when it's OAuth (not locally installable). While this shouldn't occur with current catalog data (services like Slack provide API/stdio fallbacks), it's worth defensive handling.
Consider:
return options.find(isLocallyInstallableMcpOption);Or document this behavior explicitly if the fallback is intentional.
| makeInitialState(entry), | ||
| ); | ||
| const [globalError, setGlobalError] = React.useState<string | null>(null); | ||
| const option = getInstallableMcpConnectionOption(entry); |
There was a problem hiding this comment.
🟡 Suggestion: Add defensive guard for undefined option
When getInstallableMcpConnectionOption returns undefined (e.g., entry has no MCP options), the modal silently fails with no fields rendered. While this shouldn't occur with catalog data filtered by getMcpMarketplaceCatalog, adding explicit feedback would improve debugging:
if (!option) {
return (
<ModalBackdrop>
<div className="p-4 text-danger">
Error: No installable configuration available for {entry.name}
</div>
</ModalBackdrop>
);
}| expect(option?.auth.strategy).toBe("api_key"); | ||
| expect(option?.transport.kind).toBe("stdio"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟡 Suggestion: Add edge case test coverage
Consider adding tests for defensive edge cases:
- OAuth-only entry: When an entry has only OAuth options, verify
getInstallableMcpConnectionOptionbehavior - Undefined return: When an entry has no MCP connection options, verify graceful handling
- Empty transport URL: Verify transport matching handles malformed template data
Example:
it("returns undefined for entry with no MCP options", () => {
const entry = { ...githubEntry, connectionOptions: [] };
expect(getInstallableMcpConnectionOption(entry)).toBeUndefined();
});These shouldn't occur with real catalog data, but explicit test coverage makes the contract clear.
| type McpCatalogEntry as MarketplaceEntry, | ||
| } from "@openhands/extensions/mcps"; | ||
| INTEGRATION_CATALOG as MCP_MARKETPLACE, | ||
| type IntegrationCatalogEntry as MarketplaceEntry, |
There was a problem hiding this comment.
Could you explain the purpose of this rename? I see no problem if “Integrations” means “integrations”; but if in one app “Integrations” means MCPs, while on the Saas “Integrations Hub” means a different thing, then I’m afraid this will cause a lot of confusion.
enyst
dismissed
their stale review
I still think this is not clear. Removing the request for changes though, I think maybe things come in the wrong order 😂
3 participants
Summary
@openhands/extensions/mcpsto MCP-capable entries in@openhands/extensions/integrationsconnectionOptions, including OAuth/default hosted MCP options and API/stdio fallback optionsrequiredIntegrationIdsand match installed servers against any MCP connection option@openhands/extensionsto [codex] Rename MCP catalog to integrations extensions#266 commitc4d8d9727405d26fe9013f7ac4ca477cb61a37f6Uses merged OpenHands/extensions#266.
Tests
Notes
update-snapshotsbecause the MCP marketplace content changed intentionally./api/settingsso MCP install flows do not hit a real local backend during visual tests.🐳 Docker images for this PR
• GHCR package: https://github.com/OpenHands/agent-canvas/pkgs/container/agent-canvas
ghcr.io/openhands/agent-canvasghcr.io/openhands/agent-server:1.23.0-pythonopenhands-automation==1.0.0a371bda6cf358c6cd11b666416457d9f8e0625c724Pull (multi-arch manifest)
# Multi-arch manifest — Docker automatically pulls the correct architecture docker pull ghcr.io/openhands/agent-canvas:sha-71bda6cRun
All tags pushed for this build
About Multi-Architecture Support
sha-71bda6c) is a multi-arch manifest supporting both amd64 and arm64sha-71bda6c-amd64) are also available if needed