feat: Add ConnectorBuilderMCPServer class for clean subclassing #192
Conversation
- Add ConnectorBuilderMCPServer base class with configuration support - Support checklist path override via constructor/class attr/env var - Enable plugin module registration for decorator-based tools - Refactor server.py to use DefaultConnectorBuilderServer subclass - Export ConnectorBuilderMCPServer from __init__.py for easy import - Update tests to work with new server architecture This enables external packages (like Sonar) to subclass the server, provide custom build strategies, and automatically register global tools without manual one-by-one setup. 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:
|
📝 WalkthroughWalkthroughThis PR introduces a configurable MCP server base class architecture for connector-builder-mcp. It adds Changes
Sequence Diagram(s)sequenceDiagram
participant main as main()
participant dbs as DefaultConnectorBuilderServer
participant base as ConnectorBuilderMCPServer
participant app as FastMCP app
participant env as Environment<br/>(CONNECTOR_BUILDER_STRATEGY)
main->>dbs: instantiate()
dbs->>base: __init__(app, overrides)
base->>base: configure_environment()
base->>app: register global tools
base->>dbs: get_strategy_class() [override point]
dbs->>env: read CONNECTOR_BUILDER_STRATEGY
env-->>dbs: strategy name
dbs->>dbs: validate & resolve class
dbs-->>base: BuildStrategy subclass
base->>base: register_strategy_tools()
base->>base: prepare_plugin_globals()
base->>base: register_plugins()
main->>base: run_stdio(show_banner=False)
base->>app: run via stdio transport
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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 |
👋 Welcome to the Airbyte Connector Builder MCP!Thank you for your contribution! Here are some helpful tips and reminders for your convenience. Testing This Branch via MCPTo test the changes in this specific branch with an MCP client like Claude Desktop, use the following configuration: {
"mcpServers": {
"connector-builder-mcp-dev": {
"command": "uvx",
"args": ["--from", "git+https://github.com/airbytehq/connector-builder-mcp.git@devin/1763760123-add-server-class", "connector-builder-mcp"]
}
}
}Testing This Branch via CLIYou can test this version of the MCP Server using the following CLI snippet: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/connector-builder-mcp.git@devin/1763760123-add-server-class#egg=airbyte-connector-builder-mcp' --helpPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
AI Builder EvaluationsAI builder evaluations run automatically under the following conditions:
A set of standardized evaluations also run on a schedule (Mon/Wed/Fri at midnight UTC) and can be manually triggered via workflow dispatch. Helpful ResourcesIf you have any questions, feel free to ask in the PR comments or join our Slack community. |
![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
🧹 Nitpick comments (9)
connector_builder_mcp/__init__.py (1)
18-27: Re‑exportingConnectorBuilderMCPServervia the package root looks goodMaking the base server available from
connector_builder_mcpdirectly is aligned with the subclassing use case and keeps the public surface clear. Optionally, you could also re‑exportDefaultConnectorBuilderServerhere for symmetry ("DefaultConnectorBuilderServer"in__all__), but it’s already accessible via theservermodule, so this is purely a convenience decision.tests/unit/test_server.py (4)
3-4: Consider importing the base class via the package root to exercise the new public APIThe imports are fine as‑is, but since
ConnectorBuilderMCPServeris now exported inconnector_builder_mcp.__init__, you could switch to:from connector_builder_mcp import ConnectorBuilderMCPServerThis would more directly validate the intended public import path.
27-29: App name assertion relies on a hard‑coded stringChecking that
server.app.name == "connector-builder-mcp"is useful, but it duplicates the value ofMCP_SERVER_NAME. To avoid drift if the constant ever changes, you might import and assert againstMCP_SERVER_NAMEinstead.
33-34:test_tools_registeredno longer verifies that tools are actually registeredThis test now only checks that
server.apphas a.toolattribute, which is a property ofFastMCPitself and doesn’t assert that any connector‑builder tools were registered by your server lifecycle.To better exercise the new server class, consider:
- Explicitly calling
server.register_all()or at leastserver.register_global_tools().- Asserting on a known tool or resource being present (e.g., that a specific checklist or server‑info tool was registered), or that registration functions were invoked.
That would keep coverage closer to the behavior the old tests were validating.
36-42: Add tests for strategy selection, including env‑var override and invalid valuesThe current test ensures that
get_strategy_class()returns a class withnameandversion, which is a good sanity check. Given the new behavior inDefaultConnectorBuilderServer.get_strategy_class, it would be valuable to:
- Add a test where
CONNECTOR_BUILDER_STRATEGYis set to a valid strategy name and verify that the corresponding class is returned.- Add a test that sets
CONNECTOR_BUILDER_STRATEGYto an invalid value and asserts that aSystemExitis raised with the expected error message.- Ensure tests explicitly clear or control this env var so a developer’s local environment cannot accidentally cause the whole test run to
sys.exit(1).This will make the strategy selection behavior deterministic and well‑covered.
connector_builder_mcp/server_class.py (3)
34-96: Base server class layout and configuration knobs look solidThe core design of
ConnectorBuilderMCPServeris coherent:
- Class attributes (
server_name,default_strategy_class,checklist_path) give subclass authors an easy way to set defaults.- The constructor’s keyword‑only overrides plus optional
FastMCPinjection cover the main configuration scenarios.self.app = app or FastMCP(self.server_name)ensures the FastMCP instance is always named consistently with the server.Two small nits:
- The
if TYPE_CHECKING: passblock is currently unused; you can drop it or use it for type‑only imports later.- If you expect subclasses to mutate
server_nameafter__init__, note that theFastMCPname will still be whatever it was at construction time—this is fine, but worth keeping in mind.Overall, this is a good foundation for subclassed servers.
177-205: Plugin registration API is flexible; decide whether to fail fast or degrade gracefully on import errorsThe plugin hook points (
prepare_plugin_globals,import_plugin_modules,register_plugins) give external integrators a clear way to wire globalmcpvariables and register tools via side‑effect imports, which fits the MCP tooling style.Right now, any exception during
__import__(module_path)will bubble up out ofregister_all()and causerun_stdio()to fail before the server starts. That’s a reasonable “fail fast” default for misconfigured plugins, but if you ever want a more resilient server you might:
- Catch
ImportError(and possibly other exceptions) per module, log an error, and continue with the remaining plugins.- Or add a configuration flag on the server to control whether plugin import failures are fatal or just logged.
No change is strictly required, but being explicit about the intended failure mode will help plugin authors know what to expect.
222-249:run_stdioencapsulates CLI semantics, includingsys.exit; consider documenting this behaviorThe
run_stdiomethod:
- Fully initializes the server via
register_all().- Prints a clear startup banner with strategy info.
- Runs
self.app.run_stdio_asyncusingasyncio.run.- Handles
KeyboardInterruptand other exceptions, printing messages and exiting with appropriate status codes.One thing to be aware of is that this method always calls
sys.exit(0 on normal completion, 1 on error). That’s perfect for CLI entry points (as used inserver.main()), but can surprise library consumers who expect a long‑running call to simply return.Two options:
- Keep the behavior as‑is but explicitly document in the docstring that
run_stdiowill terminate the process.- Or factor a lower‑level method (e.g.,
run_stdio_forever()that returns normally) and have the CLI entry point wrap it withsys.exit.Given current usage, this is more of an API‑design consideration than a correctness issue.
connector_builder_mcp/server.py (1)
25-80: Override bypasses base classstrategy_classparameter—valid architectural concernVerification confirms both suggestions in the original review are valid:
Strategy override concern is correct. The base
ConnectorBuilderMCPServer.__init__accepts astrategy_classparameter and stores it inself.default_strategy_class. The baseget_strategy_class()respects this instance attribute first. However,DefaultConnectorBuilderServer.get_strategy_class()completely overrides this behavior and ignoresself.default_strategy_class, meaning any programmatic configuration via the constructor parameter will be silently bypassed in favor of env var / default resolution.While no current code uses this pattern, adding the precedence check would maintain the base class contract:
def get_strategy_class(self): if self.default_strategy_class is not None: return self.default_strategy_class # existing CONNECTOR_BUILDER_STRATEGY and default resolution...Type hint is missing. The base class method signature is
get_strategy_class(self) -> type[BuildStrategy] | None. The override has no return annotation, reducing IDE clarity and consistency with the base contract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
connector_builder_mcp/__init__.py(1 hunks)connector_builder_mcp/server.py(2 hunks)connector_builder_mcp/server_class.py(1 hunks)tests/unit/test_server.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: aaronsteers
Repo: airbytehq/connector-builder-mcp PR: 116
File: .github/workflows/ci-tests.yml:110-113
Timestamp: 2025-10-10T17:09:02.151Z
Learning: The connector-builder-mcp repository (airbytehq/connector-builder-mcp) is an internal/private repository that doesn't accept external contributions, so there are no forked PRs to consider in workflow configurations.
Learnt from: aaronsteers
Repo: airbytehq/connector-builder-mcp PR: 116
File: .github/workflows/ci-tests.yml:110-113
Timestamp: 2025-10-10T17:09:05.986Z
Learning: The connector-builder-mcp repository at airbytehq/connector-builder-mcp is a private/internal repository that does not accept external contributions or forks.
Learnt from: aaronsteers
Repo: airbytehq/connector-builder-mcp PR: 116
File: connector_builder_agents/src/evals/evaluators.py:78-79
Timestamp: 2025-10-08T17:51:38.833Z
Learning: In the connector-builder-mcp repository, internal evaluation code (e.g., in connector_builder_agents/src/evals/) doesn't need to be bulletproof with extensive error handling, as the simpler approach is preferred for internal tooling.
📚 Learning: 2025-10-11T02:19:05.231Z
Learnt from: aaronsteers
Repo: airbytehq/connector-builder-mcp PR: 136
File: connector_builder_agents/src/evals/evaluators.py:233-245
Timestamp: 2025-10-11T02:19:05.231Z
Learning: In the connector-builder-mcp repository Python files, all imports must be placed at the module level (top of the file), never inside functions or methods. This is a recurring issue that should be flagged during code reviews.
Applied to files:
tests/unit/test_server.pyconnector_builder_mcp/__init__.pyconnector_builder_mcp/server_class.py
📚 Learning: 2025-10-10T17:09:05.986Z
Learnt from: aaronsteers
Repo: airbytehq/connector-builder-mcp PR: 116
File: .github/workflows/ci-tests.yml:110-113
Timestamp: 2025-10-10T17:09:05.986Z
Learning: The connector-builder-mcp repository at airbytehq/connector-builder-mcp is a private/internal repository that does not accept external contributions or forks.
Applied to files:
connector_builder_mcp/server.pyconnector_builder_mcp/__init__.py
📚 Learning: 2025-11-08T06:53:26.977Z
Learnt from: aaronsteers
Repo: airbytehq/connector-builder-mcp PR: 176
File: connector_builder_mcp/_manifest_history_utils.py:38-49
Timestamp: 2025-11-08T06:53:26.977Z
Learning: In the connector-builder-mcp repository, all imports must be at the module level (top of the file). Inline imports inside functions or methods are not allowed unless explicit permission is granted. This rule must never be violated.
Applied to files:
connector_builder_mcp/__init__.py
🧬 Code graph analysis (4)
tests/unit/test_server.py (2)
connector_builder_mcp/server.py (2)
DefaultConnectorBuilderServer(32-80)get_strategy_class(48-80)connector_builder_mcp/server_class.py (2)
ConnectorBuilderMCPServer(34-249)get_strategy_class(120-135)
connector_builder_mcp/server.py (3)
connector_builder_mcp/server_class.py (3)
ConnectorBuilderMCPServer(34-249)get_strategy_class(120-135)run_stdio(222-249)connector_builder_mcp/_util.py (1)
initialize_logging(17-23)connector_builder_mcp/build_strategies/declarative_yaml_v1/build_strategy.py (1)
DeclarativeYamlV1Strategy(22-61)
connector_builder_mcp/__init__.py (1)
connector_builder_mcp/server_class.py (1)
ConnectorBuilderMCPServer(34-249)
connector_builder_mcp/server_class.py (8)
connector_builder_mcp/build_strategies/base/build_strategy.py (2)
BuildStrategy(18-185)register_all_mcp_callables(107-120)connector_builder_mcp/build_strategies/declarative_yaml_v1/build_strategy.py (1)
DeclarativeYamlV1Strategy(22-61)connector_builder_mcp/mcp/checklist.py (1)
register_checklist_tools(214-218)connector_builder_mcp/mcp/manifest_edits.py (1)
register_manifest_edit_tools(445-452)connector_builder_mcp/mcp/manifest_history.py (1)
register_manifest_history_tools(739-745)connector_builder_mcp/mcp/secrets_config.py (1)
register_secrets_tools(765-772)connector_builder_mcp/mcp/server_info.py (1)
register_server_info_resources(81-87)connector_builder_mcp/server.py (1)
get_strategy_class(48-80)
⏰ 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). (3)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
🔇 Additional comments (3)
tests/unit/test_server.py (1)
10-24: Smoke tests for existence and basic instantiation look reasonableThese tests verify that both server classes are importable and that
DefaultConnectorBuilderServerexposes a non‑nullappwithrun_stdio_async, which is a good minimal contract check for the refactor.connector_builder_mcp/server.py (2)
1-8: Module docstring clearly points custom users to the new base classThe added note steering custom implementations toward
ConnectorBuilderMCPServerinstead of this module is helpful and matches the new architecture.
83-86: Module refactoring incomplete:appexport removed but still imported in other filesThe change to
main()is correct, but removing theappexport fromconnector_builder_mcp/server.pybreaks dependent code:
- poe_tasks.toml (lines 37–38): Task definitions call
app.run(transport='http'|'sse')and will fail withImportError- bin/test_mcp_tool.py (line 18): Imports
appfrom the server module and will fail withImportErrorEither restore the
appexport fromserver.py, or update these files to use the new class-based interface withDefaultConnectorBuilderServer.⛔ Skipped due to learnings
Learnt from: aaronsteers Repo: airbytehq/connector-builder-mcp PR: 136 File: connector_builder_agents/src/evals/evaluators.py:233-245 Timestamp: 2025-10-11T02:19:05.231Z Learning: In the connector-builder-mcp repository Python files, all imports must be placed at the module level (top of the file), never inside functions or methods. This is a recurring issue that should be flagged during code reviews.
| def configure_environment(self) -> None: | ||
| """Configure environment variables before tool registration. | ||
|
|
||
| Sets environment variables that tools may read during registration. | ||
| Subclasses can override to set additional environment variables. | ||
| """ | ||
| if self.checklist_path: | ||
| os.environ["CONNECTOR_BUILDER_MCP_CHECKLIST_PATH"] = str(self.checklist_path) | ||
| logger.info(f"Set CONNECTOR_BUILDER_MCP_CHECKLIST_PATH={self.checklist_path}") | ||
|
|
||
| def register_global_tools(self) -> None: | ||
| """Register global tools (checklist, secrets, etc.). | ||
|
|
||
| These tools are strategy-agnostic and available in all server instances. | ||
| Subclasses can override to customize global tool registration. | ||
| """ | ||
| logger.info("Registering global tools") | ||
| register_server_info_resources(self.app) | ||
| register_secrets_tools(self.app) | ||
| register_manifest_history_tools(self.app) | ||
| register_checklist_tools(self.app) | ||
| register_manifest_edit_tools(self.app) | ||
|
|
||
| def get_strategy_class(self) -> type[BuildStrategy] | None: | ||
| """Get the build strategy class. |
There was a problem hiding this comment.
🧩 Analysis chain
Registration flow and env configuration order are appropriate; consider adding tests for checklist env behavior
The register_all() orchestration:
configure_environment()(setsCONNECTOR_BUILDER_MCP_CHECKLIST_PATH),register_global_tools(),register_strategy_tools(),register_plugins(),
is well‑ordered for tools that depend on env vars at registration time. The logging around both env setup and strategy tool registration is also helpful for debugging.
Given the PR checklist mentions verifying the checklist path env‑var integration, I’d suggest adding a small unit test along these lines:
- Instantiate
ConnectorBuilderMCPServer(checklist_path="some/path.yaml"). - Call
configure_environment()(orregister_all()). - Assert that
os.environ["CONNECTOR_BUILDER_MCP_CHECKLIST_PATH"]is set to the expected value.
That would lock in the behavior and guard against regressions in the registration sequence.
Also applies to: 137-221
🏁 Script executed:
#!/bin/bash
# Optional: check for any existing references to CONNECTOR_BUILDER_MCP_CHECKLIST_PATH
# that might assume it is set externally rather than via this class.
rg -n "CONNECTOR_BUILDER_MCP_CHECKLIST_PATH" -C3Length of output: 740
I need to verify where this environment variable is actually consumed to ensure the integration is complete.
I need to search the codebase for where this environment variable is actually read, particularly in the checklist tools registration.
Let me search the codebase directly to find where the environment variable is actually consumed.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Search for where CONNECTOR_BUILDER_MCP_CHECKLIST_PATH is read/used
# This includes os.environ.get, os.getenv, or any attempt to read this var
rg -n "os\.environ\[.*CONNECTOR_BUILDER_MCP_CHECKLIST_PATH|os\.getenv.*CONNECTOR_BUILDER_MCP_CHECKLIST_PATH|os\.environ\.get.*CONNECTOR_BUILDER_MCP_CHECKLIST_PATH" -C3Length of output: 637
🏁 Script executed:
#!/bin/bash
# Also search for register_checklist_tools to see its implementation
rg -n "def register_checklist_tools" -A 30Length of output: 376
🏁 Script executed:
#!/bin/bash
# Search for any imports or references to checklist in the codebase that might read the env var
rg -n "checklist_path\|CHECKLIST_PATH" -C2 --type=py | head -50Length of output: 0
I found a critical issue: the environment variable is being SET but I cannot find where it's being READ. Let me search for the implementation that consumes this path.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Search for register_mcp_tools to understand how it works
rg -n "def register_mcp_tools" -A 20Length of output: 1647
🏁 Script executed:
#!/bin/bash
# Search more broadly for where checklist_path is used (case-insensitive to catch variations)
rg -in "checklist.*path|path.*checklist" --type=py -C2 | head -60Length of output: 4163
🏁 Script executed:
#!/bin/bash
# Search for os.environ reads to see if env vars are consumed dynamically
rg -n "os\.environ\.get|os\.getenv" --type=py | head -20Length of output: 1752
The environment variable is being SET but I need to find where it's actually being READ. Let me search the checklist module more thoroughly.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Look at the full checklist.py file to see the complete register_checklist_tools implementation
rg -n "." connector_builder_mcp/mcp/checklist.py | grep -A 50 "def register_checklist_tools"Length of output: 181
🏁 Script executed:
#!/bin/bash
# Search for where the checklist data is actually loaded/read in the codebase
rg -n "get_global_checklist_path|get_session_checklist_path" -B 2 -A 5Length of output: 2785
🏁 Script executed:
#!/bin/bash
# Search for any references to the environment variable by checking all variations
rg -n "CHECKLIST_PATH" --type=pyLength of output: 264
This is critical - the environment variable is being SET but NEVER READ anywhere in the codebase. Let me verify if there's module-level initialization or integration tests that might reveal the intended usage.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Search for any module-level or __init__ code that might read the env var
rg -n "^[^#]*CHECKLIST_PATH" connector_builder_mcp --type=pyLength of output: 264
🏁 Script executed:
#!/bin/bash
# Check tests to see if this integration is tested
rg -n "CONNECTOR_BUILDER_MCP_CHECKLIST_PATH|configure_environment" tests/ --type=py -B 2 -A 5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if get_global_checklist_path reads the env var
cat connector_builder_mcp/_paths.py | grep -A 10 "def get_global_checklist_path"Length of output: 174
Environment variable integration is incomplete—configured path is set but never consumed
The CONNECTOR_BUILDER_MCP_CHECKLIST_PATH environment variable is set at lines 104-105 but is never read anywhere in the codebase. The checklist system ignores it entirely:
get_global_checklist_path()returns a hardcoded path (_guidance/connector_build_checklist.yml)get_session_checklist_path(session_id)ignores the configured checklist path- No code path reads
os.environ["CONNECTOR_BUILDER_MCP_CHECKLIST_PATH"]
This breaks the documented feature: passing checklist_path to the server has no effect. Either:
- Add code to read the environment variable in the checklist loading functions, or
- Remove the unused
configure_environment()call andchecklist_pathparameter
feat: Add ConnectorBuilderMCPServer class for clean subclassing
Summary
This PR introduces a new
ConnectorBuilderMCPServerbase class that enables external packages (like Sonar) to create custom MCP servers by subclassing rather than duplicating registration logic. The refactoring maintains backward compatibility for the default server behavior while providing a clean extension point.Key Changes:
New
server_class.py: AddsConnectorBuilderMCPServerbase class with:register_global_tools(): Registers strategy-agnostic tools (checklist, secrets, manifest history, etc.)register_strategy_tools(): Registers build strategy-specific toolsregister_plugins(): Supports decorator-based tool registration viaprepare_plugin_globals()andimport_plugin_modules()hooksrun_stdio(): Orchestrates registration and runs the serverRefactored
server.py: Now usesDefaultConnectorBuilderServersubclass that:get_strategy_class()to select strategy based onCONNECTOR_BUILDER_STRATEGYenv varUpdated
__init__.py: ExportsConnectorBuilderMCPServerfor easy import by external packagesUpdated tests: Modified to work with new class-based architecture
Use Case (Sonar Integration):
Review & Testing Checklist for Human
@mcp.tooldecorators and verifyprepare_plugin_globals()+import_plugin_modules()correctly registers tools. This is the core feature for Sonar but has zero test coverage.CONNECTOR_BUILDER_MCP_CHECKLIST_PATHenv var and confirm checklist tools actually read it. The code sets the env var but doesn't verify the tools consume it.MEDIUM - Backward Compatibility
appdirectly fromconnector_builder_mcp.server(this would break). The globalappvariable was removed.MEDIUM - End-to-End Verification
connector-builder-mcpCLI and verify it starts correctly, registers all tools, and responds to MCP requestsCONNECTOR_BUILDER_STRATEGYvalues to ensure strategy selection logic works correctlyTest Plan
connector-builder-mcp(or viapoe server)poe inspectto verify all expected tools are registeredNotes
appvariable inserver.pywas removed. External code importing it will break, but this is unlikely since the intended usage is via the CLI entrypoint.prepare_plugin_globals()andimport_plugin_modules()hooks are designed for Sonar's decorator-based tool registration but are completely untested in this PR.CONNECTOR_BUILDER_MCP_CHECKLIST_PATH, but this integration wasn't verified.Requested by: AJ Steers (@aaronsteers, [email protected])
Devin Session: https://app.devin.ai/sessions/32a5956e7f1d4f70a3550357570a7855
Summary by CodeRabbit
Release Notes
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.