Repository navigation
fix(cloud): preserve guidance warnings and tolerate malformed docs - #1240
Piotr Kozikowski (pkozikow) wants to merge 10 commits into
Conversation
👋 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@piotr/fusion-mcp-api' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@piotr/fusion-mcp-api'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. |
Code Coverage OverviewLanguages: Python Python / code-coverage/pytest-no-credsThe overall line coverage in commit 97996d9 in the Show a line coverage summary of the most impacted files.
Python / code-coverage/pytestThe overall line coverage in commit 97996d9 in the Show a line coverage summary of the most impacted files.
Python / code-coverage/pytest-unitThe overall line coverage in commit 97996d9 in the Show a line coverage summary of the most covered files.
Updated |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Four moderate issues remain in capability state handling, workspace lookup, warning duplication, and warning type safety.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This PR normalizes Cloud execution results and improves connector capability guidance and unsupported-write handling.
Changes:
- Maps Cloud
data/metaresponses toExternalApiExecuteResult. - Preserves warnings and supports recoverable capability checks.
- Rejects unsupported Cloud writes with actionable guidance.
| File | Summary |
|---|---|
tests/unit_tests/test_mcp_cloud.py |
Tests MCP write-action errors and structured warnings. |
tests/unit_tests/test_cloud_direct_execute.py |
Tests execution normalization and error propagation. |
tests/unit_tests/test_cloud_describe.py |
Tests capability recovery and documentation warnings. |
airbyte/mcp/cloud.py |
Reports unknown capabilities and unsupported writes. Two moderate issues remain around unavailable probe state and pre-rejection workspace resolution. |
airbyte/cloud/workspaces.py |
Adjusts connector feature resolution and warning propagation. |
airbyte/cloud/connectors.py |
Updates feature caching, warning handling, and write rejection. Two moderate warning-handling issues remain. |
airbyte/_direct_connectors/connector_docs.py |
Stores fallback notices in metadata. |
airbyte/_direct_connectors/api_util.py |
Normalizes Cloud execution envelopes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0449d48b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
|
Piotr Kozikowski (@pkozikow) - I've cherry-picked the data shape fix here #1248, and added strong typing: But it looks like we're blocked by: Before we can confirm the fix in 1248, I think we need to resolve that upstream issue. |
Piotr Kozikowski (pkozikow)
changed the title
|
Agreed—the unit tests validate response handling, but don’t establish that execution works end to end. #1248 is now included in release-candidate/v1 and not redone here; live validation remains outstanding pending the upstream issue. |
2ed0bcd to
6591ca5
Compare
|
|
||
|
|
||
| class CloudConnector: | ||
| class CloudConnector: # noqa: PLR0904 - public connector management and execution API |
There was a problem hiding this comment.
Nit - Normally would expect text after the noqa to briefly hint at the rule being ignored and/or reason to ignore. Can we update for clarity? The post-noqa comment doesn't explain what is being ignored.
There was a problem hiding this comment.
I looked it up: PLR0904 - too many public methods
| class CloudConnector: # noqa: PLR0904 - public connector management and execution API | |
| class CloudConnector: # noqa: PLR0904 # too many public methods |
But this probably goes away if we revert _get_enabled_features to private, so maybe not needed.
There was a problem hiding this comment.
Making _get_enabled_features private removes the need for the PLR0904 suppression, so I removed it. Ruff passes without it.
| """Return the enabled features, resolving them through the workspace on first use.""" | ||
| if self._enabled_features is None: | ||
| self._enabled_features = self.workspace._get_connector_features(self) # noqa: SLF001 | ||
| def get_enabled_features( |
There was a problem hiding this comment.
I think this should remain private since we already have the same as a public property.
There was a problem hiding this comment.
Agreed—restored the private helper and kept enabled_features as the public property. Internal callers retain the warning and unknown-state handling.
| @@ -396,15 +409,13 @@ def execute_api_action( | |||
| context={"entity_type": entity_type, "action": action}, | |||
| ) from None | |||
|
|
|||
| return self._execute_direct_action( | |||
| entity_type=entity_type, | |||
| action=resolved_action.value, | |||
| api_args=api_args, | |||
| select_fields=select_fields, | |||
| exclude_fields=exclude_fields, | |||
| skip_truncation=skip_truncation, | |||
| intent=intent, | |||
| read_only=False, | |||
| _ = api_args, select_fields, exclude_fields, skip_truncation, intent | |||
| raise exc.PyAirbyteInputError( | |||
| message="Cloud connector write actions are not supported yet.", | |||
| guidance=( | |||
| "Use execute_api_query (Python) or execute_external_api_query (MCP) for reads." | |||
| ), | |||
| context={"entity_type": entity_type, "action": resolved_action.value}, | |||
| ) | |||
There was a problem hiding this comment.
Let's not break the implementation please. Better to just hide it by suppressing/disabling the tool.
There was a problem hiding this comment.
Probably best for this to be in a dedicated PR, so it doesn't slow other needed changes. As-is today, I think this is working as designed. It properly advertises itself as not supported/enabled and is basically inert, while still being ready to handle workloads when enabled/unblocked.
There was a problem hiding this comment.
Confirmed. This fails gracefully as designed.
Tested against live
mcp.airbyte.com(RC4) with theairbyte-support-botcreds:
execute_external_api_action(action="create", entity_type="issues")on the Jira source in Devin's Sandbox → "AirbyteExternalAccessNotEnabledError: The connector is not enabled for external access… Checkenabled_featureson the connector and organization."Clean, accurate, agent-friendly — no write was attempted (backend 403/404 → the base branch's relabel path fired, and it was correct here). So for non-enabled connectors the status quo degrades gracefully...
My preference is to just let it stay. I'm happy to hide it in a separate PR if that helps, but let's not break the current implementation.
There was a problem hiding this comment.
Agreed—removed the write-rejection changes and restored the existing implementation. Any change to tool visibility can go in a separate PR.
0238f61 to
9e15721
Compare
Piotr Kozikowski (pkozikow)
changed the title
|
Closing. Resolved by:
Piotr Kozikowski (@pkozikow) - In future, please:
Thanks.
|
AGENTIC-2244
Malformed optional skill docs can fail an otherwise valid connector description, and local destination guidance warnings are lost from structured MCP results. Catch validation failures when describing connectors, preserve independently confirmed enabled features, and store local notices in
metadata.warningsso both MCP renderers retain them. Docs failure warnings avoid embedding raw error details.Destination fallback guidance keeps the existing not-enabled notice, with a code comment documenting the rare missing-docs 404 ambiguity. Regression tests cover malformed docs, unavailable docs, structured warnings on fresh and cached reads, and warning preservation through both renderers.
Rebased onto
release-candidate/v1atb256ec9f. The base now provides authoritative connector enablement, confirmed-disabled versus failed-lookup error handling, search, typed execution results, and the hidden MCP write tool. Those implementations are preserved; superseded docs-based feature inference has been removed from this PR. The existing Python write execution path is unchanged.Validation: