feat(mcp): Add manifest_path parameter to local operations tools #800
Conversation
- Add manifest_path parameter to all local MCP operations tools - Enable testing of custom YAML connector manifests - Bridge PyAirbyte MCP with connector-builder-mcp functionality - Maintain backward compatibility with existing tools - Support declarative connectors with local manifest files Co-Authored-By: AJ Steers <[email protected]>
Original prompt from AJ Steers |
🤖 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. Testing 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/1758731973-add-manifest-path-to-mcp-tools' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1758731973-add-manifest-path-to-mcp-tools'Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
Community SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Ops as local_ops.<op>()
participant Resolver as _get_mcp_source
participant ExecUtil as get_connector_executor / get_source
participant Src as Source
User->>Ops: call op(..., manifest_path)
Ops->>Resolver: _get_mcp_source(..., manifest_path)
Resolver->>ExecUtil: get_source(..., source_manifest=manifest_path or True)
ExecUtil->>ExecUtil: normalize source_manifest -> Path if local string
ExecUtil-->>Resolver: returns Source instance
Resolver-->>Ops: Source
Ops->>Src: perform action (list/schema/read/preview/sync)
Src-->>Ops: result
Ops-->>User: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Would you like a short checklist of unit/integration tests to verify Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)airbyte/mcp/_local_ops.py (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (2)
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: 0
🧹 Nitpick comments (6)
airbyte/mcp/_local_ops.py (6)
46-49: Clarify manifest_path usage and auto-mode behavior in help textCould we add a short note that when a manifest_path is provided, YAML mode is generally preferred, and that docker mode may not honor a local manifest unless explicitly supported, to set user expectations, wdyt?
For declarative connectors, you can provide a `manifest_path` to specify a local YAML manifest file instead of using the registry version. This is useful for testing custom or locally-developed connector manifests. +Note: When providing a `manifest_path`, YAML execution is typically preferred. +If you leave `override_execution_mode="auto"`, consider that docker may be selected +if available, which may not honor local manifests. Explicitly set `override_execution_mode="yaml"` +to force local manifest usage.
89-91: Guard against empty-string manifest paths in YAML modeUsing
manifest_path or Truetreats empty string as True, silently ignoring the provided value. If you keep this pattern, should we validate non-empty strings before this point (as in the refactor above) to avoid surprising behavior, wdyt?
241-245: list_source_streams: manifest_path wiring LGTMLooks correct. Do we want to log/echo which execution mode was chosen when manifest_path is set and mode is auto, to aid debuggability, wdyt?
Also applies to: 253-254
463-471: get_stream_previews: manifest_path wiring LGTM (naming nit)Wiring looks good. Nit: for API consistency, would you consider aligning
source_namewithsource_connector_nameused elsewhere in this module in a future pass, wdyt?Also applies to: 481-482
559-566: sync_source_to_cache: manifest_path wiring LGTMLooks correct. Nice fallback to "*" when registry metadata isn't available (common for local YAML). Do we want to mention this in docs to set expectations for declarative-only sources, wdyt?
Also applies to: 572-573
58-91: Prefer YAML for local manifests, validate early, and DRY get_source kwargs
- When
manifest_pathis set andoverride_execution_mode=="auto", switch to"yaml"before checking Docker to improve UX and avoid confusing errors?- Validate
manifest_pathupfront (exists, is file,.yml/.yaml) for clearer errors?- Consolidate
get_sourcekeyword arguments into a single dict and call once to eliminate duplication? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/mcp/_local_ops.py(12 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (3)
airbyte/mcp/_local_ops.py (3)
139-146: validate_connector_config: manifest_path plumbed correctlySignature and propagation look good. Could we add a quick test covering (auto,yaml,python,docker) with and without manifest_path to validate error surfaces and success cases, wdyt?
Also applies to: 155-156
302-309: get_source_stream_json_schema: manifest_path wiring LGTMAll good. Any interest in a small smoke test with a local declarative YAML to ensure schema retrieval works in yaml mode, wdyt?
Also applies to: 315-316
372-379: read_source_stream_records: manifest_path wiring LGTMLooks good. Since yaml + manifest_path is a primary scenario, could we add a happy-path test that reads a handful of records from a tiny local manifest to catch regressions, wdyt?
Also applies to: 386-387
…hon defaults - Force execution mode to 'yaml' when manifest_path is provided - Remove Python-level default arguments from function signatures - Keep Field() defaults intact for MCP tool compatibility - Update parameter documentation to clarify override behavior Co-Authored-By: AJ Steers <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/_executors/util.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
There was a problem hiding this comment.
Pull Request Overview
This PR adds an optional manifest_path parameter to PyAirbyte MCP local operations tools, enabling users to specify custom YAML manifest files for declarative connectors instead of using registry versions. This creates a bridge between PyAirbyte MCP and connector-builder-mcp functionality for testing locally-developed connector manifests.
- Added
manifest_pathparameter to 6 MCP tools for connector operations - Updated internal
_get_mcp_sourcefunction to handle manifest paths across execution modes - Enhanced path handling logic to properly detect file paths vs inline YAML content
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| airbyte/mcp/_local_ops.py | Added manifest_path parameter to all MCP tools and updated _get_mcp_source function |
| airbyte/_executors/util.py | Enhanced path detection logic for plain string manifest paths |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Copilot <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/fix-pr
|
|
/fix-pr
|
aaronsteers
deleted the
devin/1758731973-add-manifest-path-to-mcp-tools
branch
1 participant
feat(mcp): Add manifest_path parameter to local operations tools
Summary
This PR adds a new optional
manifest_pathparameter to all PyAirbyte MCP local operations tools, enabling users to specify custom YAML manifest files for declarative connectors instead of using registry versions. This creates a bridge between PyAirbyte MCP and connector-builder-mcp functionality, allowing developers to test locally-developed YAML connector manifests using PyAirbyte's execution and testing capabilities.Key Changes:
manifest_pathparameter to 6 MCP tools:validate_connector_config,list_source_streams,get_source_stream_json_schema,read_source_stream_records,get_stream_previews, andsync_source_to_cache_get_mcp_sourcefunction to handle manifest paths across all execution modes (auto, docker, python, yaml)Review & Testing Checklist for Human
🟡 Medium Risk - 3 items to verify:
Notes
Requested by: @aaronsteers
Devin session: https://app.devin.ai/sessions/a27357ff3dbd42ce887412faf6b108fe
Summary by CodeRabbit
New Features
Bug Fixes
Important
Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.