fix: properly disable prompt secret source in mcp mode #793
Conversation
👋 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@aj/fix/properly-disable-prompt-secret-source-in-mcp-mode' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@aj/fix/properly-disable-prompt-secret-source-in-mcp-mode'Helpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
Community SupportQuestions? Join the #pyairbyte channel in our Slack workspace. |
📝 WalkthroughWalkthroughMoves MCP initialization and secrets setup to module import time in the MCP server, adds versioned MCP mode stderr logging, adjusts SecretSourceEnum string representation and its use in config disabling, exposes SecretSourceEnum via config, and updates the CI Ruff format check to use --diff. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Import as Module Import
participant Meta as meta.set_mcp_mode
participant Secrets as initialize_secrets
participant App as MCP App
participant Tools as Tool Registrations
participant Main as main()
participant Runtime as app.run_stdio_async
rect rgb(245,245,255)
note over Import: Import-time initialization (moved)
Import->>Meta: set_mcp_mode()
Meta-->>Import: stderr log "MCP vX (Python vY)"
Import->>Secrets: initialize_secrets()
Secrets-->>Import: Secrets ready
Import->>App: construct app
Import->>Tools: register connector/local/cloud ops tools
Tools-->>Import: registrations complete
end
Main->>Runtime: run stdio async loop
Runtime-->>Main: exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 (3)
airbyte/secrets/config.py (2)
6-6: Remove unusedsysimport, wdyt?The linter correctly flags this unused import. Since no code uses
sysin this file, let's clean it up.-import sys
7-7: Remove unusedTYPE_CHECKINGimport, wdyt?Since you've moved
SecretSourceEnumto runtime import (Line 10), theTYPE_CHECKINGimport is no longer needed and should be removed.-from typing import TYPE_CHECKINGairbyte/mcp/_util.py (1)
5-5: Fix: Remove unusedsysimport.The linter caught that
sysis imported but never used in this file. Should we remove it to keep the imports clean, wdyt?-import sys
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
airbyte/_util/meta.py(2 hunks)airbyte/mcp/_util.py(1 hunks)airbyte/mcp/server.py(1 hunks)airbyte/secrets/base.py(1 hunks)airbyte/secrets/config.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
airbyte/secrets/config.py (1)
airbyte/secrets/base.py (2)
SecretManager(146-208)SecretSourceEnum(23-35)
airbyte/mcp/server.py (2)
airbyte/_util/meta.py (1)
set_mcp_mode(41-52)airbyte/mcp/_util.py (1)
initialize_secrets(37-70)
airbyte/_util/meta.py (1)
airbyte/version.py (1)
get_version(12-14)
🪛 GitHub Actions: Run Linters
airbyte/secrets/config.py
[error] 6-6: F401 [*] sys imported but unused
[error] 7-7: F401 [*] typing.TYPE_CHECKING imported but unused
airbyte/_util/meta.py
[error] 19-19: PLW0406 Module airbyte._util.meta imports itself
airbyte/mcp/_util.py
[error] 5-5: F401 [*] sys imported but unused
⏰ 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, Windows)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (No Creds)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (4)
airbyte/secrets/config.py (1)
10-10: LGTM: Improved string-based source comparison logic!The changes work well together:
- Moving
SecretSourceEnumto runtime import enables proper string comparisons- Using
str(existing_source) == str(source)leverages the new__str__method fromSecretSourceEnum- The
.copy()ensures we're iterating over a snapshot to avoid modification issuesThis approach is much cleaner than the previous
.nameattribute comparison and aligns with the new__str__behavior.Also applies to: 76-78
airbyte/mcp/server.py (2)
16-22: LGTM: Proper initialization order at import time!Moving the initialization to module import time is a good approach for MCP mode. This ensures:
- MCP mode detection is available immediately
- Secrets are initialized before any app functionality
- All tools (connector, local, cloud ops) are registered at startup
The sequence looks correct: set mode → initialize secrets → create app → register tools.
25-37: LGTM: Clean main function focused on execution!The main function is now properly focused on just running the server loop and handling errors, with all initialization moved to import time. The error handling and user feedback are appropriate for an MCP server.
airbyte/secrets/base.py (1)
33-35: LGTM! Nice implementation of the__str__method.The addition of
__str__method toSecretSourceEnumreturning the enum's underlying value makes perfect sense and aligns well with the changes in the secrets config handling. This enables cleaner string comparisons and is exactly what you need for the updated comparison logic indisable_secret_source, wdyt?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
.github/workflows/python_lint.yml (3)
59-59: Ensure format step fails CI when reformatting is needed
ruff format --diffshows the diff but may not fail the job. Do you want CI to fail on unformatted code? If yes, can we add--check(you can keep--difffor preview), wdyt?- run: poetry run ruff format --diff . + run: poetry run ruff format --check --diff .
20-31: Duplicate “Set up Poetry” step in the same jobThis block duplicates the earlier Poetry setup (Lines 21–24 vs 29–31). Can we drop one to shave runner time and noise, wdyt?
- - name: Set up Poetry - uses: Gr1N/setup-poetry@v9 - with: - poetry-version: "2.0.1"
69-79: Duplicate “Set up Poetry” step in mypy jobSame duplication here. Remove one to simplify the workflow, wdyt?
- - name: Set up Poetry - uses: Gr1N/setup-poetry@v9 - with: - poetry-version: "2.0.1"airbyte/_util/meta.py (2)
46-49: Make MCP-mode log idempotent and include implementationIf
set_mcp_mode()gets called twice, we’ll emit the banner twice. Also, would you prefer the richer Python string (e.g., “3.10.14 (CPython)”) viaget_python_version()and flush immediately, wdyt?-def set_mcp_mode() -> None: - """Set flag indicating we are running in MCP (Model Context Protocol) mode. +def set_mcp_mode() -> None: + """Set flag indicating we are running in MCP (Model Context Protocol) mode. @@ - print( - f"Running in MCP mode: PyAirbyte MCP v{get_version()} (Python v{python_version()})", - file=sys.stderr, - ) - global _MCP_MODE_ENABLED - _MCP_MODE_ENABLED = True + global _MCP_MODE_ENABLED + if _MCP_MODE_ENABLED: + return + print( + f"Running in MCP mode: PyAirbyte MCP v{get_version()} (Python {get_python_version()})", + file=sys.stderr, + flush=True, + ) + _MCP_MODE_ENABLED = True
124-126: Avoid potential hang when probing Colab session
requests.get()has no timeout and could block. Shall we add a small timeout (e.g., 0.5–1s) to keep imports snappy, wdyt?- response = requests.get(COLAB_SESSION_URL) + response = requests.get(COLAB_SESSION_URL, timeout=1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/python_lint.yml(1 hunks)airbyte/_util/meta.py(2 hunks)airbyte/secrets/config.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- airbyte/secrets/config.py
🧰 Additional context used
🧬 Code graph analysis (1)
airbyte/_util/meta.py (1)
airbyte/version.py (1)
get_version(12-14)
⏰ 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)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (No Creds)
🔇 Additional comments (1)
airbyte/_util/meta.py (1)
19-20: LGTM: import of get_versionImport looks clean and resolves the prior circular import concern noted in earlier CI. Nice fix.
aaronsteers
deleted the
aj/fix/properly-disable-prompt-secret-source-in-mcp-mode
branch
1 participant
Summary by CodeRabbit
Important
Auto-merge enabled.
This PR is set to merge automatically when all requirements are met.