fix(mcp): make Cloud credential guidance execution-mode-aware#1089
Conversation
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/1784914782-mcp-mode-aware-cred-guidance' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1784914782-mcp-mode-aware-cred-guidance'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. |
![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
This PR updates PyAirbyte’s MCP Cloud credential guidance so it’s execution-mode-aware: hosted (HTTP) MCP requests should instruct users to provide credentials/workspace via request headers, while local/stdio usage should continue to reference AIRBYTE_CLOUD_* environment variables. It also adjusts static help text that cannot vary per-request, and adds unit tests covering the new behavior.
Changes:
- Added hosted-request detection + centralized, mode-aware credential guidance helpers in
airbyte/mcp/_tool_utils.py. - Updated MCP Cloud tool tips/instructions and improved runtime error guidance to reference the correct credential path (headers vs env vars).
- Added/updated unit tests for hosted detection and updated credential guidance wording.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
airbyte/mcp/_tool_utils.py |
Adds is_hosted_mcp_request() and get_mcp_credential_guidance() to produce transport-aware guidance. |
airbyte/mcp/cloud.py |
Updates Cloud tool help text and uses the new guidance helper in the missing-workspace error. |
airbyte/mcp/server.py |
Updates the server “Cloud operations” instructions to mention hosted-header auth. |
airbyte/cloud/_credentials.py |
Makes generic credential guidance non-prescriptive about env vars only (mentions hosted headers too). |
tests/unit_tests/test_mcp_tool_utils.py |
Adds tests for hosted detection and for both hosted/local guidance strings. |
tests/unit_tests/test_cloud_credentials.py |
Updates expected guidance string to match new wording. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…dance Co-Authored-By: AJ Steers <aj@airbyte.io>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCloud credential and workspace failures now use dedicated exceptions with guidance that varies between hosted MCP HTTP and local/stdio execution. Hosted mode is set during HTTP startup, and MCP cloud instructions and tests reflect header- and environment-based configuration. ChangesHosted MCP credential guidance
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HTTPMain
participant MCPCloud
participant CloudExceptions
participant MCPClient
HTTPMain->>MCPCloud: initialize hosted MCP mode
MCPClient->>MCPCloud: request cloud operation
MCPCloud->>CloudExceptions: raise credential or workspace exception
CloudExceptions-->>MCPClient: return mode-specific guidance
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 1
🧹 Nitpick comments (1)
tests/unit_tests/test_mcp_tool_utils.py (1)
92-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCould we cover the
RuntimeErrorfallback?
is_hosted_mcp_request()explicitly converts request-contextRuntimeErrors toFalse, but this parametrization only covers token/header values. Adding aside_effect=RuntimeErrorcase would protect the intended local/stdio fallback.🤖 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 `@tests/unit_tests/test_mcp_tool_utils.py` around lines 92 - 112, Add a parametrized case to test_is_hosted_mcp_request where the mocked request-context accessor raises RuntimeError, and assert is_hosted_mcp_request() returns False. Use side_effect=RuntimeError on the appropriate get_access_token or get_http_headers patch while preserving the existing token/header cases.
🤖 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/cloud.py`:
- Around line 46-56: Update CLOUD_AUTH_TIP_TEXT and WORKSPACE_ID_TIP_TEXT in
airbyte/mcp/cloud.py (lines 46-56) to consistently document the exact hosted
authentication headers and local alternatives: AIRBYTE_CLOUD_BEARER_TOKEN or the
client-credential pair with workspace ID. Update the server guidance in
airbyte/mcp/server.py (lines 113-116) to use the same wording, including
AIRBYTE_CLOUD_BEARER_TOKEN, so it matches get_mcp_credential_guidance().
---
Nitpick comments:
In `@tests/unit_tests/test_mcp_tool_utils.py`:
- Around line 92-112: Add a parametrized case to test_is_hosted_mcp_request
where the mocked request-context accessor raises RuntimeError, and assert
is_hosted_mcp_request() returns False. Use side_effect=RuntimeError on the
appropriate get_access_token or get_http_headers patch while preserving the
existing token/header cases.
🪄 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 Plus
Run ID: 44d402c1-3c8f-4509-9a06-5c1f4915b136
📒 Files selected for processing (6)
airbyte/cloud/_credentials.pyairbyte/mcp/_tool_utils.pyairbyte/mcp/cloud.pyairbyte/mcp/server.pytests/unit_tests/test_cloud_credentials.pytests/unit_tests/test_mcp_tool_utils.py
Code Coverage OverviewLanguages: Python Python / code-coverage/pytest-fastThe overall coverage in commit 622c268 in the Show a code coverage summary of the most impacted files.
Python / code-coverage/pytest-no-credsThe overall coverage in commit 622c268 in the Show a code coverage summary of the most impacted files.
Python / code-coverage/pytestThe overall coverage in commit 622c268 in the Show a code coverage summary of the most impacted files.
Updated |
Co-Authored-By: AJ Steers <aj@airbyte.io>
Co-Authored-By: AJ Steers <aj@airbyte.io>
Co-Authored-By: AJ Steers <aj@airbyte.io>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tests/unit_tests/test_mcp_tool_utils.py:108
- Same as above: prefer patching the
airbyte.mcp._tool_utils.is_hosted_mcp_modecallable over mutating the privatemeta._HOSTED_MCP_MODE_ENABLEDvariable (and remove the now-unusedmetaimport if you make this change).
with patch.object(meta, "_HOSTED_MCP_MODE_ENABLED", False):
Co-Authored-By: AJ Steers <aj@airbyte.io>
Co-Authored-By: AJ Steers <aj@airbyte.io>
Co-Authored-By: AJ Steers <aj@airbyte.io>
organizations.py and workspaces.py imported the _AirbyteCredentials name directly, which CodeQL flags as py/unsafe-cyclic-import (the name may be undefined mid-cycle). Import the _credentials module instead so the class is resolved lazily at use time, downgrading the alert to a benign module-level cyclic import. Co-Authored-By: AJ Steers <aj@airbyte.io>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
… import" This reverts commit 9ca4420.
Co-Authored-By: AJ Steers <aj@airbyte.io>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
airbyte/exceptions.py:541
- The
workspace_urlproperty assumesworkspacehas a.workspace_urlattribute; typing it asAnyremoves useful type checking. Consider annotating it with the local_WorkspaceWithUrlprotocol (added above) so type checkers can validate callers.
workspace: Any | None = None
AirbyteError.workspace was downgraded to Any when its CloudWorkspace import was removed, losing static checking of the workspace_url property. Declare a local _WorkspaceWithUrl Protocol that structurally describes the one attribute exceptions needs, so the field is properly typed without importing airbyte.cloud (duck typing, no shared ancestor required). Co-Authored-By: AJ Steers <aj@airbyte.io>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Co-Authored-By: AJ Steers <aj@airbyte.io>
Summary
Cloud MCP tools told users to "set the
AIRBYTE_CLOUD_*environment variables" to authenticate. That guidance is wrong under the hosted (HTTP) MCP transport, where credentials arrive as request headers, not server-side env vars. This makes the credential/workspace guidance execution-mode-aware and moves it into self-rendering exception classes that both PyAirbyte and the Ops MCP can share.Mode detection is a dependency-light process-global flag — no FastMCP in the credential/guidance path, since FastMCP is becoming an optional extra and core code must not import it there:
airbyte/constants.py:set_hosted_mcp_mode()/is_hosted_mcp_mode(), defaultFalse. They're inconstants.py(a stdlib-only leaf module) deliberately —exceptions.pyalready imports fromconstants, so reading the flag there adds no new import edge and avoids the module-import cycle that a dedicated_utilsubmodule introduced.airbyte/mcp/http_main.pycallsset_hosted_mcp_mode()at startup. The stdio/local path never sets it, so an unset flag ⇒ not hosted. No token/header/request inspection is used to infer mode.Guidance lives in two new self-rendering exception classes in
airbyte/exceptions.py(call sites justraise— no inline message/guidance strings). Each computes mode-awareguidancein__post_init__, reading onlyis_hosted_mcp_mode()plus the header/env-var constants (no FastMCP, no MCP-layer import):The underscore-prefixed control fields are excluded from the rendered error text, and the explicit
guidance: str | None = Nonefield makes it unambiguous that mode-aware guidance is applied by default (the inheritedPyAirbyteInputError.guidanceis an unannotated class attr, not a dataclass field default)._credentials.pyraisesAirbyteNoCloudCredentialsError(_env_vars=env_vars);mcp/cloud.pyraisesAirbyteMissingWorkspaceContextError. The oldget_mcp_credential_guidance()helper in_tool_utils.pyis removed.Static tool help / field descriptions (baked at import, can't vary per request) are reworded to mention both paths and name the exact headers/env vars via constants:
CLOUD_AUTH_TIP_TEXT,WORKSPACE_ID_TIP_TEXT(airbyte/mcp/cloud.py), and the "Cloud operations" line inMCP_SERVER_INSTRUCTIONS(airbyte/mcp/server.py).Import cycle / CodeQL:
exceptions.pyused to typeAirbyteError.workspacevia aTYPE_CHECKINGimport ofairbyte.cloud.workspaces.CloudWorkspace, which placedexceptionson the_credentials→exceptions→cloud→_credentialsmodule cycle that CodeQL surfaces once this PR edits_credentials.py. That import is now replaced with a local_WorkspaceWithUrlProtocol— structural typing for the singleworkspace_urlattribute the error needs, so any object exposing it (e.g.CloudWorkspace) satisfies it without an ancestor or import.exceptionstherefore no longer importsairbyte.cloudat all, which removes the cycle edge; CodeQL is clean.Out of scope (intentionally unchanged):
CloudClientConfig.from_env/CloudWorkspace.from_envdocstrings (env-var by definition), and all non-credential env-var text.Test plan
ruff format --check .andruff check .clean;import airbyteclean.tests/unit_tests/test_exceptions.pycovers both classes in hosted + local modes (by toggling the shared flag), asserts_env_vars=Falsedrops env-var guidance, and that control fields don't leak into rendered text;test_cloud_credentials.pyupdated for the new wording. The Ops MCP consumes the same shared exceptions and flag.Link to Devin session: https://app.devin.ai/sessions/3ef501472b3d49ca831f588f36f95b29
Requested by: Aaron ("AJ") Steers (@aaronsteers)