Skip to content

[codex] Rename MCP catalog to integrations#266

Merged
neubig merged 1 commit into
mainfrom
codex/unified-integrations-catalog
May 25, 2026
Merged

[codex] Rename MCP catalog to integrations#266
neubig merged 1 commit into
mainfrom
codex/unified-integrations-catalog

Conversation

@neubig
Copy link
Copy Markdown
Member

@neubig neubig commented May 25, 2026

Summary

  • Rename the MCP catalog surface to integrations and remove the old mcps package exports.
  • Convert direct MCP catalog entries to canonical connectionOptions.
  • Move Integrations Hub provider/default metadata into the extensions package so downstream apps can pin one source of truth.
  • Rename automation catalog references from requiredMcpIds to requiredIntegrationIds.

Validation

  • uv run --group test pytest -q

Copy link
Copy Markdown
Contributor

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taste Rating: 🟡 Acceptable - Well-executed refactoring with proper schema evolution, but lacks migration documentation for breaking changes.

The core refactoring is solid: the schema transformation from template to connectionOptions is consistent across all 40 integrations, tests pass, and the new structure is more extensible. However, this introduces significant breaking changes to the public API without a migration guide.

[RISK ASSESSMENT]

  • [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM
    This PR introduces intentional breaking changes to the public API (MCP_CATALOGINTEGRATION_CATALOG, type renames, package export path changes). While the refactoring is well-executed and complete, downstream consumers will break on upgrade without a migration guide. The risk is medium rather than high because: (1) the package version is 0.0.0, indicating pre-release/unstable API, and (2) the tests demonstrate the migration path clearly.

VERDICT:
Worth merging: Schema evolution is sound and complete. Add migration documentation before publishing to npm.

KEY INSIGHT:
The new connectionOptions array structure is a significant improvement - it separates auth strategy from transport mechanism and enables multi-option integrations (e.g., OAuth + API key fallback). This lays the groundwork for HTTP/OpenAPI integrations alongside MCP.


Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/extensions/actions/runs/26411134242

Comment thread index.d.ts
MarketplaceTemplate,
} from "./mcps/index.js";
export {
INTEGRATION_CATALOG,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 Important: Breaking change - MCP_CATALOG renamed to INTEGRATION_CATALOG.

Downstream consumers using import { MCP_CATALOG } from '@openhands/extensions' will break. Consider:

  1. Migration guide: Document the rename and type changes in a CHANGELOG or MIGRATION.md
  2. Deprecation period: If possible, export both names temporarily with a deprecation warning
  3. Version bump: Since package.json shows version 0.0.0, ensure the next publish is clearly marked as breaking (e.g., 0.1.0 or 1.0.0)

Example migration:

// Before
import { MCP_CATALOG, McpCatalogEntry } from '@openhands/extensions';

// After  
import { INTEGRATION_CATALOG, IntegrationCatalogEntry } from '@openhands/extensions';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh oh! There was an unexpected error starting the job :(

Comment thread package.json
"import": "./mcps/index.js",
"default": "./mcps/index.js"
"./integrations": {
"types": "./integrations/index.d.ts",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Consider adding a brief migration note in the package description or README.

Since the export path changed from /mcps to /integrations, consumers importing from the subpath will also break:

// Old (broken after this PR)
import { MCP_CATALOG } from '@openhands/extensions/mcps';

// New
import { INTEGRATION_CATALOG } from '@openhands/extensions/integrations';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh oh! There was an unexpected error starting the job :(

@neubig neubig marked this pull request as ready for review May 25, 2026 17:29
@neubig neubig force-pushed the codex/unified-integrations-catalog branch from a63dbe5 to 1c17ce4 Compare May 25, 2026 17:31
Copy link
Copy Markdown
Contributor

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taste Rating: 🟡 Acceptable - Well-executed refactoring with proper schema evolution, but lacks migration documentation for breaking changes.

The core refactoring is solid: the schema transformation from template to connectionOptions is consistent across all 40 integrations, tests pass, and the new structure is more extensible. However, this introduces significant breaking changes to the public API without a migration guide.

[CRITICAL ISSUES]

  • [Repository root] Breaking Change - No Migration Documentation: This PR introduces multiple breaking changes without any migration guide:

    • Export names changed: MCP_CATALOGINTEGRATION_CATALOG, MCP_LOGOSINTEGRATION_LOGOS
    • Export paths changed: /mcps/integrations
    • Schema field changed in automations: requiredMcpIdsrequiredIntegrationIds
    • Catalog entry schema transformed: templateconnectionOptions

    Required: Add a CHANGELOG.md or MIGRATION.md with:

    1. Clear before/after import examples
    2. Schema migration examples for catalog consumers
    3. Version number for this breaking change (suggest major version bump)
    4. Timeline for deprecation if any backward compatibility is planned

[IMPROVEMENT OPPORTUNITIES]

  • [index.js, index.d.ts] Backward Compatibility: Consider providing deprecated exports for one release cycle to give consumers time to migrate:
    // Deprecated: Use INTEGRATION_CATALOG instead. Will be removed in v2.0.0
    export const MCP_CATALOG = INTEGRATION_CATALOG;
    This would allow existing consumers to receive deprecation warnings instead of silent breakage.

[RISK ASSESSMENT]

  • [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM

The code quality is excellent and the refactoring is well-executed with comprehensive test coverage (319 tests passing). The new connectionOptions schema is more extensible and properly separates transport, authentication, and HTTP configuration concerns. However, this introduces breaking changes to a public npm package that downstream consumers (agent-canvas, integrations-hub, and potentially external consumers) import directly. Without migration documentation, consumers will experience:

  • Import errors from renamed exports
  • Runtime type errors from schema changes
  • Silent failures if they rely on the old /mcps subpath

Recommendation: Add migration documentation before merging. The technical implementation is sound, but the lack of consumer-facing guidance makes this risky to ship.

VERDICT:
Worth merging after: Adding migration documentation

KEY INSIGHT:
The schema evolution from flat template to structured connectionOptions is a solid design improvement that enables better separation of concerns and easier extension for future auth/transport types. The merging logic elegantly handles both direct integrations and OAuth provider catalog entries. Just needs proper communication to consumers.


Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/extensions/actions/runs/26412412527

@neubig neubig force-pushed the codex/unified-integrations-catalog branch from 1c17ce4 to 4d68539 Compare May 25, 2026 17:42
Copy link
Copy Markdown
Contributor

@all-hands-bot all-hands-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taste Rating: 🟡 Acceptable - Well-executed refactoring with proper schema evolution and migration documentation. The breaking change is acceptable per repo guidelines for pre-release surfaces.

The core refactoring is solid: the schema transformation from template to connectionOptions is consistent across all 40+ integrations, tests pass, and the new structure is more extensible. The MIGRATION.md guide clearly documents the breaking changes and provides concrete examples.

[IMPROVEMENT OPPORTUNITIES]
While not blocking, these could improve maintainability:

  • [integrations/index.js, Lines 95-100] Simplification: The optionIdForDefaults function has redundant logic - the ternary defaults to "oauth2" in both the true and false branches. Consider simplifying to make the intent clearer.
  • [integrations/index.js, Lines 194-205] Complexity: The merging logic works but adds cognitive overhead. Consider adding inline comments explaining when OAuth providers override direct integrations vs. when they merge.

[RISK ASSESSMENT]

  • [Overall PR] ⚠️ Risk Assessment: 🟡 MEDIUM

This is a significant breaking change affecting 104 files, but the risk is mitigated by:

  • Pre-release status (version 0.0.0)
  • Comprehensive migration guide (MIGRATION.md)
  • All validation tests passing
  • Repo-specific guidelines explicitly allow clean breaking changes for this pre-release surface
  • Coordinated deployment with downstream consumers

The main risk is the hard cutover with no backward compatibility, but this is intentional and documented.

VERDICT:
Worth merging: Solid architectural improvement with proper migration documentation. The breaking change is acceptable per repo policy.

KEY INSIGHT:
The new connectionOptions array architecture enables future multi-auth support (OAuth + API key fallbacks) while maintaining the MCP-first focus, making this a good foundation for broader integration types.


Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/OpenHands/extensions/actions/runs/26412939616

@neubig neubig merged commit c4d8d97 into main May 25, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants