feat(mcp): allow MCP tools to specify a docker image / version for the connector#1020
feat(mcp): allow MCP tools to specify a docker image / version for the connector#1020Aaron ("AJ") Steers (aaronsteers) wants to merge 3 commits into
Conversation
🤖 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/1778018940-mcp-docker-image-override' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1778018940-mcp-docker-image-override'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 Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds explicit docker image override and version pinning support for MCP sources, introduces user-facing help constants, extends internal source resolution to handle docker images, updates multiple public endpoints to accept/forward ChangesDocker Image Override and Version Pinning
🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
airbyte/mcp/local.py (1)
803-813:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCould we resolve suggested streams with the normalized connector name instead, wdyt?
On Line 806 this still uses
source_connector_nameforget_connector_metadata(...). If the caller uses the new image-shaped path, registry lookup misses and we fall back to"*", sosync_source_to_cache()can sync every stream instead of the connector's suggested set.Proposed fix
- metadata = get_connector_metadata( - source_connector_name, - ) + metadata = get_connector_metadata(source.name)🤖 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/local.py` around lines 803 - 813, The call to get_connector_metadata currently passes source_connector_name which can be the image-shaped path and will miss registry metadata; change the call to use the normalized connector name used elsewhere (e.g., normalized_source_connector_name or the image reference variable source_connector_ref) so metadata lookup succeeds; if no normalized variable exists, call the existing normalization helper (e.g., normalize_connector_name(source_connector_name) or similar) and pass that result to get_connector_metadata, leaving the rest of the streams/metadata.suggested_streams logic unchanged.
🤖 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.
Inline comments:
In `@airbyte/mcp/local.py`:
- Around line 110-112: The parsing treats ":" too broadly and drops tags/digests
incorrectly; instead first strip any digest by splitting on "@" (e.g. base =
last_segment.split("@", 1)[0]) and then remove a tag by splitting on ":" only
from that digest-free base (name = base.split(":", 1)[0]); apply the same logic
when deriving the image/name from docker_image (the code paths around
last_segment, name and the branch that handles docker_image around lines
157-161) so digest-pinned refs (sha256:...) and registry:port prefixes are
preserved correctly.
---
Outside diff comments:
In `@airbyte/mcp/local.py`:
- Around line 803-813: The call to get_connector_metadata currently passes
source_connector_name which can be the image-shaped path and will miss registry
metadata; change the call to use the normalized connector name used elsewhere
(e.g., normalized_source_connector_name or the image reference variable
source_connector_ref) so metadata lookup succeeds; if no normalized variable
exists, call the existing normalization helper (e.g.,
normalize_connector_name(source_connector_name) or similar) and pass that result
to get_connector_metadata, leaving the rest of the
streams/metadata.suggested_streams logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 340d7b04-857c-4c60-99df-d78ede7431f5
📒 Files selected for processing (2)
airbyte/mcp/local.pytests/unit_tests/test_mcp_local.py
- Strip @sha256:... digest before stripping :tag in _resolve_docker_image_and_name, so refs like ghcr.io/org/source@sha256:abc resolve name='source'. - Inspect only the last path segment when deciding whether to drop the caller-provided version, so untagged registry-port refs like localhost:5000/airbyte/source-mssql keep version pinning. - Use source.name (the parsed registry name) for get_connector_metadata in sync_source_to_cache so suggested-stream resolution works when the caller passes an image-shaped connector_name. - Add regression tests for digest, registry-port (untagged), and registry-port + tag image references.
|
Both CodeRabbit findings addressed in 406516d:
Regression tests added for |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
airbyte/mcp/local.py (3)
812-820: 💤 Low valueSwitching to
source.namefor the registry lookup looks right — small follow-up question on the failure mode, wdyt?Using
source.namehere is the right move so that an image-shapedsource_connector_namelikeairbyte/source-mssql:4.4.2still resolves to the registry name (source-mssql) forget_connector_metadata. One tiny thing: theexcept Exception:branch at Line 816-817 setsstreams = "*", butstreamswas already set to"*"on Line 813, so this is a no-op. Worth dropping to reduce confusion, or replacing with apass/log so it's obvious the fallback is intentional?Optional simplification
try: metadata = get_connector_metadata(source.name) except Exception: - streams = "*" # Fallback to all streams if suggested streams fail. + pass # Fallback to all streams (already set above). else: if metadata is not None: streams = metadata.suggested_streams or "*"🤖 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/local.py` around lines 812 - 820, The code sets streams = "*" before calling get_connector_metadata(source.name) and then redundantly resets streams = "*" again in the except block; remove the no-op assignment in the except branch and either replace it with a pass or a brief log to make the fallback explicit. Update the try/except around get_connector_metadata(source.name) (referenced symbols: streams, get_connector_metadata, source.name) so the except block no longer reassigns streams to "*" but instead leaves streams as-is (or logs the error) to avoid confusion.
67-84: ⚡ Quick winHelp text says "mutually exclusive" but the implementation silently drops
version— should we soften the wording, wdyt?
_DOCKER_IMAGE_HELPclaimsdocker_imageis "Mutually exclusive with a:tagalready embedded in the image and an explicitversion", but_get_mcp_sourcejust silently setsdocker_version = Nonewhen the image already has a:tagor@digestrather than raising. From a tool-author UX angle, "mutually exclusive" usually implies a hard error. Could we either raise on conflict or reword to something like "If the image already includes a:tagor@digest, the explicitversionis ignored", to keep doc and behavior aligned?🤖 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/local.py` around lines 67 - 84, The help text for _DOCKER_IMAGE_HELP currently claims mutual exclusivity with an embedded tag/digest and an explicit version, but _get_mcp_source silently sets docker_version = None when the image already includes a :tag or `@digest`; update the phrasing in _DOCKER_IMAGE_HELP (instead of asserting “Mutually exclusive…”) to state that if the docker image already includes a :tag or `@digest`, any explicitly provided version will be ignored (or alternatively, change _get_mcp_source to raise on that conflict); reference the _DOCKER_IMAGE_HELP string and the docker_version handling in _get_mcp_source when making the change so documentation and behavior are aligned.
120-156: ⚡ Quick winQuick sanity check: when both
manifest_pathanddocker_imageare passed, manifest wins silently — intended, wdyt?The docstring on Line 138-139 says "when [
docker_image] is set,override_execution_modeis forced to'docker'", but at Line 153if manifest_path:later flips it to"yaml"and the resolved docker image is never used in the yaml branch. That means a caller pinning to a specific built image while also passing a manifest will silently get yaml mode and lose the image override. Is that the desired precedence, or would it be friendlier to either raise on conflict or flip the order so an explicitdocker_imagewins overmanifest_path?🤖 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/local.py` around lines 120 - 156, The function _get_mcp_source silently prefers manifest_path over docker_image (it forces override_execution_mode="yaml" after resolving a docker image), so fix by detecting the conflict between manifest_path and docker_image early in _get_mcp_source (after calling _resolve_docker_image_and_name) and either raise a clear ValueError when both are provided or explicitly document/implement the desired precedence; e.g., if you choose to make docker_image win, set override_execution_mode="docker" (and keep resolved_docker_image) before the manifest_path check, or if you choose to forbid the combination, raise ValueError mentioning manifest_path and docker_image to force the caller to disambiguate.
🤖 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/local.py`:
- Around line 812-820: The code sets streams = "*" before calling
get_connector_metadata(source.name) and then redundantly resets streams = "*"
again in the except block; remove the no-op assignment in the except branch and
either replace it with a pass or a brief log to make the fallback explicit.
Update the try/except around get_connector_metadata(source.name) (referenced
symbols: streams, get_connector_metadata, source.name) so the except block no
longer reassigns streams to "*" but instead leaves streams as-is (or logs the
error) to avoid confusion.
- Around line 67-84: The help text for _DOCKER_IMAGE_HELP currently claims
mutual exclusivity with an embedded tag/digest and an explicit version, but
_get_mcp_source silently sets docker_version = None when the image already
includes a :tag or `@digest`; update the phrasing in _DOCKER_IMAGE_HELP (instead
of asserting “Mutually exclusive…”) to state that if the docker image already
includes a :tag or `@digest`, any explicitly provided version will be ignored (or
alternatively, change _get_mcp_source to raise on that conflict); reference the
_DOCKER_IMAGE_HELP string and the docker_version handling in _get_mcp_source
when making the change so documentation and behavior are aligned.
- Around line 120-156: The function _get_mcp_source silently prefers
manifest_path over docker_image (it forces override_execution_mode="yaml" after
resolving a docker image), so fix by detecting the conflict between
manifest_path and docker_image early in _get_mcp_source (after calling
_resolve_docker_image_and_name) and either raise a clear ValueError when both
are provided or explicitly document/implement the desired precedence; e.g., if
you choose to make docker_image win, set override_execution_mode="docker" (and
keep resolved_docker_image) before the manifest_path check, or if you choose to
forbid the combination, raise ValueError mentioning manifest_path and
docker_image to force the caller to disambiguate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ef5ad448-f2e5-41c6-a8ed-41bdb8420087
📒 Files selected for processing (2)
airbyte/mcp/local.pytests/unit_tests/test_mcp_local.py
- _get_mcp_source now raises ValueError when both manifest_path and a docker image (via image-shaped connector_name or explicit docker_image) are supplied. Previously manifest_path silently won and the image override was dropped. - Update _DOCKER_IMAGE_HELP to match actual behavior: an embedded :tag or @digest causes any explicit version to be ignored (not raised), and docker_image cannot be combined with manifest_path.
1 participant
Summary
Closes #1018.
The local MCP tools in
airbyte/mcp/local.pypreviously accepted only a registry connector name (source-mssql) and silently resolved everything via the registry. As a result, MCP callers could not pin to a specific connector version, could not point at a custom-built image (e.g.airbyte/source-mssql:devafterdocker build), and would always pull whatever the registry resolved at the moment of the call.This PR exposes both compatible shapes from the issue, mirroring
get_source(name=..., docker_image=..., version=...):connector_name— values containing a/are treated as docker image identifiers (e.g.airbyte/source-mssql,airbyte/source-mssql:4.4.2,airbyte/source-mssql:dev,ghcr.io/airbytehq/source-mssql:4.4.2). Image-shaped values bypass registry lookup for the executable, infer the registry name from the last path segment (with any:tagstripped), and force docker execution.docker_image: str | Noneandversion: str | Nonekeyword args — these mirror theget_source(...)parameters. Whendocker_imageis set, docker execution is forced regardless ofoverride_execution_mode.versionpins the registry version when staying on the registry-name path and the docker image tag when the resolved image does not already include one.Updated tools:
validate_connector_configlist_source_streamsget_source_stream_json_schemaread_source_stream_recordsget_stream_previewssync_source_to_cacheThe existing
destination_smoke_testalready accepteddocker_image, so it is unchanged.The previous code path always passed
docker_image=Truetoget_source(...)in docker mode, which forced the registry-name lookup before the docker override could take effect. The new_get_mcp_sourceresolves an explicit docker image first, decouples the registry name from the executable image, and forwards bothdocker_imageandversionthrough toget_source(...).A small private helper
_resolve_docker_image_and_nameencapsulates the parsing rules and is unit-tested against the worked examples from the issue (registry passthrough, image identifiers with/without tags, multi-segment paths, explicit override precedence).Review & Testing Checklist for Human
connector_namevalues still work for connectors whose registry name does NOT match the last path segment (e.g. private registries with renamed images). The current behavior treats the last path segment with any:tagstripped as the registryname; if that segment is not a registered connector, the registry lookup is silently skipped (matchingget_source(docker_image=...)behavior).validate_connector_config(andread_source_stream_records) with a pinned image likeairbyte/source-mssql:4.4.2and verify the source executes against that exact image without permission/path errors.docker_image/versionkwargs surface correctly in coral-mcp / FastMCP schemas (the_DOCKER_IMAGE_HELP/_VERSION_HELPfield descriptions are what callers will see).Notes
ruff check,ruff format --check) and type check (pyrefly check) pass cleanly.pytest tests/unit_tests/ -m "not slow"— 219 passed, 1 skipped).tests/unit_tests/test_mcp_local.pycovers_resolve_docker_image_and_namewith parametrized cases mirroring the proposal in feat(mcp): allow MCP tools to specify a docker image / version for the connector (currently only registry-name lookup) #1018.Link to Devin session: https://app.devin.ai/sessions/a734523681944e4d9b1dfee5792cce69
Requested by: Aaron ("AJ") Steers (@aaronsteers)
Summary by CodeRabbit
New Features
Tests