feat(mcp): Add test-my-tools prompt with optional scope parameter#893
Conversation
Co-Authored-By: AJ Steers <aj@airbyte.io>
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/1764959378-add-test-tools-prompt' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1764959378-add-test-tools-prompt'Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
Community SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
📝 WalkthroughWalkthroughAdds a new MCP prompts module defining long-form testing guidance and a scope-aware prompt generator, and registers a "test-my-tools" prompt with the FastMCP MCP server during MCP initialization. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Want a minimal unit test sketch for prompt registration and scope behavior? wdyt? Pre-merge checks and finishing touches✅ Passed checks (3 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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
airbyte/mcp/prompts.py (2)
108-125: Consider normalizing and validatingscopea bit moreRight now you only do
scope.lower()and silently ignore invalid values, which means a value like" registry "or"REGISTRY "works only partially (second does, first doesn’t) and totally invalid values just fall back to the generic guidance with no hint. Would it be worth doing something likenormalized = scope.strip().lower()plus an explicit check/optional warning or error so callers get clearer feedback when they pass a bad scope, wdyt?
63-93: Tighten up scope guidance string formatting to avoid extra blank linesBecause the scope texts are triple‑quoted without
.strip(), they’ll likely include a leading newline and any trailing whitespace when appended tocontent, which can make the rendered prompt a bit uneven. Would you consider either calling.strip()onscope_textwhen you pull it fromSCOPE_GUIDANCE, or defining these strings with.strip()at declaration time to keep formatting predictable, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte/mcp/prompts.py(1 hunks)airbyte/mcp/server.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
airbyte/mcp/server.py (1)
airbyte/mcp/prompts.py (1)
register_prompts(96-131)
⏰ 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 (No Creds)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (1)
airbyte/mcp/server.py (1)
14-26: Registering prompts alongside other MCP tools looks goodImporting
register_promptsand wiring it in next to the otherregister_*_toolscalls keeps server setup cohesive and discoverable; I don’t see any issues here. 👍 Wdyt?
…ule level Co-Authored-By: AJ Steers <aj@airbyte.io>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
airbyte/mcp/prompts.py (2)
19-44: Guidance content is clear; do you want to make it more tool‑specific?The general instructions and scope notes read well. Given the PR checklist mentions validating specific tool names, would it be worth explicitly calling out a few canonical tools in the guidance (e.g., “start by calling the connector registry listing tools, then local, then cloud ones”), so agents are nudged toward the right entrypoints, wdyt? You could also consider slightly expanding the scope strings to mirror the actual domains (e.g., “Connector Registry tools”, “Local PyAirbyte tools”, etc.) for extra clarity.
47-65: Scope parameter: tighten typing and clarify behavior for invalid values?The optional
scopeparameter and case‑insensitive lookup look good. Two small improvements you might consider:
- Would switching the annotation to
Literal["registry", "local", "cloud"] | Nonehelp make the valid values self‑documenting to type checkers and clients?- Right now, an unrecognized scope silently falls back to the “all tools” behavior. Would you prefer to either (a) raise a clear error, or (b) append a short note to the content like “Unknown scope 'foo'; testing all tools instead” so callers get feedback, wdyt?
Neither is strictly required, but they might make the prompt behavior more predictable when mis‑configured.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/mcp/prompts.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 (No Creds)
- 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 (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (1)
airbyte/mcp/prompts.py (1)
74-79: Prompt registration helper looks clean and consistentRegistering
test-my-toolsvia a dedicatedregister_prompts(app)keeps server setup modular and mirrors the usual FastMCP pattern. I don’t see any issues here from a correctness or design standpoint.
…ned values Co-Authored-By: AJ Steers <aj@airbyte.io>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte/mcp/prompts.py (1)
41-52: Consider hinting at common scope examples in the parameter descriptionThe
scopeparameter is nicely modeled as free‑form text and injected verbatim, which lines up with the earlier requirement. To improve discoverability for common use cases, would it be worth mentioning a few canonical examples (like “registry”, “local”, or “cloud”) directly in the description string so MCP clients surface that guidance in UI tooltips, while still allowing arbitrary text? Something like “…for example: ‘registry’, ‘local’, ‘cloud’, or any other custom scope.” Wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte/mcp/prompts.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: yohannj
Repo: airbytehq/PyAirbyte PR: 716
File: airbyte/logs.py:384-402
Timestamp: 2025-07-11T19:53:44.427Z
Learning: In the PyAirbyte project, when reviewing PRs, maintain clear separation of concerns. Don't suggest changes that are outside the scope of the PR's main objective, even if they would improve consistency or fix other issues. This helps with reviewing changes and potential reverts.
⏰ 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 (No Creds)
- GitHub Check: Pytest (All, Python 3.11, Windows)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Windows)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (2)
airbyte/mcp/prompts.py (2)
19-38: Guidance text looks clear and safely genericThe guidance here is concise, avoids tool‑specific details, and emphasizes safe, low‑impact testing, which seems exactly right for a shared “test-my-tools” prompt. I don’t see anything I’d tweak in this block.
67-72: Prompt registration pattern looks correct and consistentRegistering
test_my_tools_promptviaapp.prompt(...)(test_my_tools_prompt)with a clear name and description matches the FastMCP pattern and keeps the wiring nicely centralized inregister_prompts. Unless you plan on adding aliases (e.g., a shorter “test-tools” name) I don’t see any changes needed here. Wdyt about leaving it as-is for now?
Aaron ("AJ") Steers (aaronsteers)
deleted the
devin/1764959378-add-test-tools-prompt
branch
1 participant
Summary
Adds a new
test-my-toolsMCP prompt that instructs an agent to systematically test all available tools in the PyAirbyte MCP server. This replicates the pattern from Airbyte Ops MCP with a PyAirbyte-specific enhancement: an optionalscopeparameter that allows focusing tests on specific tool domains.Changes:
airbyte/mcp/prompts.pymodule withTEST_MY_TOOLS_GUIDANCEand scope-specific guidancetest_my_tools_prompt()function with optionalscopeparameter accepting 'registry', 'local', or 'cloud'server.pyUpdates since last revision
Per reviewer feedback:
test_my_tools_prompt()function is now defined at module level instead of nested insideregister_prompts(), following the pattern used elsewhere in the codebase.Review & Testing Checklist for Human
scopeparameter works correctly with values 'registry', 'local', and 'cloud'app.prompt(...)(function)) works correctly with FastMCPRecommended test plan: Connect to the PyAirbyte MCP server via Claude Desktop or another MCP client, invoke the "test-my-tools" prompt with and without the scope parameter, and verify it returns the expected guidance text.
Notes
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.
Important
Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.