fix(mcp): resolve get_connector_info spec from registry instead of installing#1077
Conversation
…out Docker Co-Authored-By: AJ Steers <aj@airbyte.io>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. 💡 Show Tips and TricksTesting This PyAirbyte VersionYou can test this version of PyAirbyte using the following: # Run PyAirbyte CLI from this branch:
uvx --from 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1784058706-gate-connector-info-install' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1784058706-gate-connector-info-install'PR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
📚 Show Repo GuidanceHelpful ResourcesCommunity SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesConnector info resolution
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant get_connector_info
participant get_connector_spec_from_registry
participant PublicConnectorRegistry
get_connector_info->>get_connector_spec_from_registry: Request cloud spec
get_connector_spec_from_registry->>PublicConnectorRegistry: Fetch connector spec
PublicConnectorRegistry-->>get_connector_spec_from_registry: Cloud spec or None
get_connector_info->>get_connector_spec_from_registry: Request OSS spec when cloud is unavailable
get_connector_spec_from_registry->>PublicConnectorRegistry: Fetch OSS connector spec
PublicConnectorRegistry-->>get_connector_info: Selected spec or None
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
airbyte/mcp/registry.py (1)
171-178: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider evaluating
is_docker_installed()once.
is_docker_installed()relies on a filesystem lookup (which("docker")), and we are currently evaluating it twice in this function (here and previously in theget_sourcecall). Could we assign the result to a local variable to avoid redundant checks and clean up the redundantor Falsefrom theget_sourcecall, wdyt?♻️ Proposed refactor
Apply the following changes outside the current line range, right before the
get_sourcecall:if connector_name not in get_available_connectors(): return "Connector not found." docker_available = is_docker_installed() connector = get_source( connector_name, docker_image=docker_available, install_if_missing=False, # Defer to avoid failing entirely if it can't be installed. )Then, update this conditional check:
- if is_docker_installed(): + if docker_available: # Populating `config_spec` requires installing and running the connector. # Only attempt it when Docker is available (a fast image pull). In a hosted,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@airbyte/mcp/registry.py` around lines 171 - 178, Evaluate is_docker_installed() once before the get_source call and store the result in a local docker_available variable. Pass that variable directly as docker_image without the redundant “or False”, and reuse docker_available for the later conditional guarding connector installation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@airbyte/mcp/registry.py`:
- Around line 171-178: Evaluate is_docker_installed() once before the get_source
call and store the result in a local docker_available variable. Pass that
variable directly as docker_image without the redundant “or False”, and reuse
docker_available for the later conditional guarding connector installation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d8c5dfa5-e4dc-4864-b703-7ede96b763d8
📒 Files selected for processing (2)
airbyte/mcp/registry.pytests/unit_tests/test_mcp_connector_registry.py
Co-Authored-By: AJ Steers <aj@airbyte.io>
Code Coverage OverviewLanguages: Python Python / code-coverage/pytest-fastThe overall coverage in the branch is 67%. The coverage in the branch is 65%. Show a code coverage summary of the most impacted files.
Python / code-coverage/pytest-no-credsThe overall coverage in the branch is 67%. The coverage in the branch is 65%. Show a code coverage summary of the most impacted files.
Python / code-coverage/pytestThe overall coverage in the branch is 72%. The coverage in the branch is 71%. Show a code coverage summary of the most impacted files.
Updated |
|
Is there a different approach we can take? Now that we have, I think, other code in production that is able to get config information from GCS-hosted public HTTP endpoints, only needing to have the version string to calculate the spec based on a stored OSS and cloud spec file hosted in GCP and available via public endpoint. You can check this repo, and you can check the ops repo for the working pattern, but I think we can just always avoid calling docker here if all we need docker for is to get the spec.json |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Fixes a performance/hanging issue in the MCP connector registry tool by preventing get_connector_info from triggering an unbounded connector install path in no-Docker (hosted) environments, while still returning fast registry metadata.
Changes:
- Gate
connector.install()/config_spec_jsonschemapopulation behindis_docker_installed()to avoid slow pip/venv installs on hosted runtimes. - Reuse a single
docker_availablecheck for bothget_source(..., docker_image=...)and the config-spec resolution path. - Add a parametrized unit regression test ensuring install is attempted only when Docker is available.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
airbyte/mcp/registry.py |
Skips config-spec installation step when Docker isn’t available to prevent long hangs in hosted MCP. |
tests/unit_tests/test_mcp_connector_registry.py |
Adds regression coverage for Docker-vs-no-Docker behavior of get_connector_info. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…point Fetch the config spec from the public connector registry (cloud then oss fallback) keyed by version instead of installing/running the connector. Works in any runtime without Docker, so the spec is populated in hosted no-Docker mode too, eliminating the unbounded install path entirely. Co-Authored-By: AJ Steers <aj@airbyte.io>
devin-ai-integration
Bot
changed the title
|
☑️ Done — switched to the registry HTTP approach in a016541. You're right, and this is cleaner than the Docker gate. Instead of installing/running the connector to compute the spec, config_spec_jsonschema = get_connector_spec_from_registry(
connector_name, platform="cloud"
) or get_connector_spec_from_registry(connector_name, platform="oss")Net effect vs. the earlier commit: no Docker anywhere in this path, and the spec is now populated in all runtimes (hosted included), not just where Docker exists — it's only |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
airbyte/mcp/registry.py (1)
173-179: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider passing the version and avoiding
orfor dict fallbacks.Since we already fetch
connector_metadataabove, we could pass the explicit version toget_connector_spec_from_registryhere to avoid redundantly looking up the metadata inside that helper.Additionally, while Airbyte config specs are never empty dictionaries in practice, an empty dictionary evaluates to
Falsein Python. Usingorwould cause an unintended fallback to the OSS spec if the cloud spec was ever technically valid but empty{}. Using an explicitis Nonecheck is a bit safer.Would you be open to a small refactor like this? wdyt?
💡 Proposed refactor
- config_spec_jsonschema: dict[str, Any] | None = get_connector_spec_from_registry( - connector_name, - platform="cloud", - ) or get_connector_spec_from_registry( - connector_name, - platform="oss", - ) + version = connector_metadata.latest_available_version if connector_metadata else None + config_spec_jsonschema: dict[str, Any] | None = get_connector_spec_from_registry( + connector_name, + version=version, + platform="cloud", + ) + if config_spec_jsonschema is None: + config_spec_jsonschema = get_connector_spec_from_registry( + connector_name, + version=version, + platform="oss", + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@airbyte/mcp/registry.py` around lines 173 - 179, Update the config spec lookup in the surrounding registry flow to pass the already-fetched connector_metadata version to each get_connector_spec_from_registry call, avoiding redundant metadata lookup. Replace the truthiness-based cloud/OSS fallback with an explicit None check so a valid empty cloud dictionary is retained.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@airbyte/mcp/registry.py`:
- Around line 173-179: Update the config spec lookup in the surrounding registry
flow to pass the already-fetched connector_metadata version to each
get_connector_spec_from_registry call, avoiding redundant metadata lookup.
Replace the truthiness-based cloud/OSS fallback with an explicit None check so a
valid empty cloud dictionary is retained.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e8dd1947-c4c1-459d-a677-37bf04046070
📒 Files selected for processing (2)
airbyte/mcp/registry.pytests/unit_tests/test_mcp_connector_registry.py
Pass the already-fetched metadata version to get_connector_spec_from_registry to avoid a redundant lookup and version drift between the cloud/oss calls, and use an explicit is-None check so a valid empty cloud spec is not skipped. Co-Authored-By: AJ Steers <aj@airbyte.io>
|
☑️ Addressed CodeRabbit's nitpick on
Tests updated to assert the |
Aaron ("AJ") Steers (aaronsteers)
deleted the
devin/1784058706-gate-connector-info-install
branch
Aaron ("AJ") Steers (aaronsteers)
changed the title
get_connector_info spec from registry instead of installing
Summary
get_connector_infocould hang for minutes on the hosted Cloud MCP server. On ahosted, no-Docker runtime, populating
config_spec_jsonschemafell through to afresh pip/venv install of the connector on every call — an unbounded operation
that has stalled requests for over four minutes.
PyAirbyte #1052 gated the
local.pyfilesystem/config tools withrequires_client_filesystem, butget_connector_infoinregistry.pywas nevergated, so it stayed exposed on hosted Cloud MCP and kept running the install path.
This gates only the expensive step on Docker availability, so the tool still returns
the fast registry metadata (docs URL, manifest URL, connector metadata) everywhere,
and simply omits
config_spec_jsonschemawhen it can't be resolved cheaply:Behavior after the fix:
config_spec_jsonschema.config_spec_jsonschema=None, no hang.config_spec_jsonschemais alreadyOptional, so returningNoneis within theexisting contract.
is_docker_installed()is already imported and used two linesabove to choose
docker_image=.Added a parametrized regression test asserting
connector.install()is called onlywhen Docker is available and
config_spec_jsonschemaisNoneotherwise.Link to Devin session: https://app.devin.ai/sessions/9274bd4f3e0a4b0880795fc1ef9c806b
Requested by: Aaron ("AJ") Steers (@aaronsteers)
Summary by CodeRabbit
config_spec_jsonschemais now returned asNonerather than relying on Docker-based execution.Important
Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.