[codex] Rename MCP catalog to integrations#266
Conversation
all-hands-bot
left a comment
There was a problem hiding this comment.
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_CATALOG→INTEGRATION_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
| MarketplaceTemplate, | ||
| } from "./mcps/index.js"; | ||
| export { | ||
| INTEGRATION_CATALOG, |
There was a problem hiding this comment.
🟠 Important: Breaking change - MCP_CATALOG renamed to INTEGRATION_CATALOG.
Downstream consumers using import { MCP_CATALOG } from '@openhands/extensions' will break. Consider:
- Migration guide: Document the rename and type changes in a CHANGELOG or MIGRATION.md
- Deprecation period: If possible, export both names temporarily with a deprecation warning
- 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';There was a problem hiding this comment.
Uh oh! There was an unexpected error starting the job :(
| "import": "./mcps/index.js", | ||
| "default": "./mcps/index.js" | ||
| "./integrations": { | ||
| "types": "./integrations/index.d.ts", |
There was a problem hiding this comment.
🟡 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';There was a problem hiding this comment.
Uh oh! There was an unexpected error starting the job :(
a63dbe5 to
1c17ce4
Compare
all-hands-bot
left a comment
There was a problem hiding this comment.
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_CATALOG→INTEGRATION_CATALOG,MCP_LOGOS→INTEGRATION_LOGOS - Export paths changed:
/mcps→/integrations - Schema field changed in automations:
requiredMcpIds→requiredIntegrationIds - Catalog entry schema transformed:
template→connectionOptions
Required: Add a
CHANGELOG.mdorMIGRATION.mdwith:- Clear before/after import examples
- Schema migration examples for catalog consumers
- Version number for this breaking change (suggest major version bump)
- Timeline for deprecation if any backward compatibility is planned
- Export names changed:
[IMPROVEMENT OPPORTUNITIES]
- [index.js, index.d.ts] Backward Compatibility: Consider providing deprecated exports for one release cycle to give consumers time to migrate:
This would allow existing consumers to receive deprecation warnings instead of silent breakage.
// Deprecated: Use INTEGRATION_CATALOG instead. Will be removed in v2.0.0 export const MCP_CATALOG = INTEGRATION_CATALOG;
[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
/mcpssubpath
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
1c17ce4 to
4d68539
Compare
all-hands-bot
left a comment
There was a problem hiding this comment.
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
optionIdForDefaultsfunction 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
Summary
integrationsand remove the oldmcpspackage exports.connectionOptions.requiredMcpIdstorequiredIntegrationIds.Validation
uv run --group test pytest -q