Repository navigation
feat(mcp): add async source catalog discovery and comparison (discover_cloud_source_catalog) - #1392
Conversation
…loud connections Co-Authored-By: AJ Steers <aj@airbyte.io>
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
👋 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/1792117913-rc-stream-sync-mode-refresh-catalog' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1792117913-rc-stream-sync-mode-refresh-catalog'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-creds
View coverage of the 10 most impacted files.
Python / code-coverage/pytest
View coverage of the 10 most impacted files.
Python / code-coverage/pytest-unit
Line coverage data for the release-candidate/v1 branch is not yet available. View coverage of the 10 most covered files.
ℹ️ Calculations represent line coverage. Results may change as calculations are updated. Updated |
There was a problem hiding this comment.
🟡 Changes recommended
Default cursor persistence, null-namespace selection, and the trace-inventory fixture need correction.
3 open findings
What changed in this PR
Adds Cloud catalog refresh and per-stream sync configuration to the Python API and MCP, porting #994 to the RC trunk.
Changes:
- Adds catalog rediscovery and targeted stream configuration methods.
- Exposes guarded, destructive MCP tools.
- Adds mocked tests for validation and catalog preservation.
| File | Description |
|---|---|
| tests/unit_tests/test_cloud_connection_catalog_edits.py | Tests catalog refresh, validation, and stream preservation. |
| airbyte/mcp/cloud.py | Adds two guarded catalog-editing tools. |
| airbyte/cloud/connections.py | Implements refresh and per-stream sync configuration. |
| airbyte/_util/api_util.py | Adds the refreshed-catalog API request. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-Authored-By: AJ Steers <aj@airbyte.io>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 3 potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
Aaron ("AJ") Steers (aaronsteers)
left a comment
Aaron ("AJ") Steers (aaronsteers)
left a comment
There was a problem hiding this comment.
Devin, let's split this into two PRs. One for catalog refresh and one for set stream sync mode. This one can stay the refresh catalog feature, and the new one can tackle set stream sync mode. If helpful stack the new one on top of this one.
Co-Authored-By: AJ Steers <aj@airbyte.io>
…_catalog_refresh Co-Authored-By: AJ Steers <aj@airbyte.io>
…/1792117913-rc-stream-sync-mode-refresh-catalog
devin-ai-integration
Bot
changed the title
force_catalog_refresh)
|
Split done. This PR now covers only catalog refresh, exposed as |
Co-Authored-By: AJ Steers <aj@airbyte.io>
Co-Authored-By: AJ Steers <aj@airbyte.io>
devin-ai-integration
Bot
changed the title
force_catalog_refresh)discover_cloud_source_catalog)
Co-Authored-By: AJ Steers <aj@airbyte.io>
|
/evals full=true
|
|
Aaron ("AJ") Steers (@aaronsteers) sorry for the comment, just testing |
There was a problem hiding this comment.
What is this file and how is it updated?
There was a problem hiding this comment.
It's a snapshot of how every MCP tool argument is recorded in OpenTelemetry spans. Each entry is one of the TraceArg classes: omit, hash, fingerprint, presence, or value allowed=[...].
How it's checked: test_inventory_matches_fixture in tests/unit_tests/test_mcp_arg_trace_parity.py builds the live plan with fastmcp_extensions.otel.trace_plan(server_app) and asserts it equals this file. Adding, removing or reclassifying a tool arg fails the test until the fixture is updated, so every telemetry classification change shows up in review.
How it's updated: regenerate it, then review and commit the diff:
UPDATE_ARG_TRACE_FIXTURE=1 AIRBYTE_MCP_TRUSTED_EXECUTION=1 \
uv run pytest tests/unit_tests/test_mcp_arg_trace_parity.py::test_inventory_matches_fixtureAIRBYTE_MCP_TRUSTED_EXECUTION=1 is required because the test refuses to write the file unless the trusted-only manifest_yaml_file args are visible.
In this PR, the only change is the 9-line discover_cloud_source_catalog entry. That entry has an inconsistency I'd fix: all five ID args (source_id, discover_request_id, source_catalog_id, compare_to_*) are fingerprint, but every existing ID arg in the fixture is hash. FINGERPRINT adds a similarity bitset, which is meant for short text and is meaningless for UUIDs. Switching them to TraceArg.HASH is a small change: the markers plus the regenerated fixture.
There was a problem hiding this comment.
I'm concerned this is a wasteful process with no guidance and no safeguards, and only adds friction and confusion to new PRs against this repo. It has the signs of both an auto-generated file that should be ignored via gitproperties - while also showing signs of needing careful review by each reviwerer, which apparently no guidance about how to handle - and apparently not enough AGENTS.md guidance that your suggestions here were found by you earlier not by any of the bot reviewers.
There was a problem hiding this comment.
Agreed. Here's what I found:
- It came in with feat(mcp): adopt
fastmcp-extensionstool-call tracing with built-in Otel handling (fastmcp-extensions#143) #1339 (adoptingfastmcp-extensionstool-call tracing) and has changed in 6 PRs since. No AGENTS.md, CONTRIBUTING or docs file mentions it, and.gitattributesdoesn't mark it as generated. - The only safeguard is the exact-match snapshot. Nothing checks that a classification is right. With no marker, a plain
strarg defaults tohash, so thefingerprintIDs in this PR come from the explicitTraceArg.FINGERPRINTmarkers I copied from sibling text args. The test passed because I regenerated the fixture, which is exactly the gap you're describing.
Proposal, as a separate small PR on release-candidate/v1 (it's repo-wide, not specific to this PR):
- Rule test instead of eyeball review: assert that every
*_idarg classifies ashashoromit, and thatfingerprintappears only on free-text or name args. That catches this PR's mistake automatically. - Collapse the snapshot: mark
tests/unit_tests/fixtures/arg_trace_plan.jsonaslinguist-generated=true. Once the rule test guards the meaning, reviewers don't need to read it. - Two lines in AGENTS.md: the regen command, plus "IDs: no marker (defaults to
hash); free text:FINGERPRINT; workspace:OMIT".
Alternative: delete the snapshot entirely and keep only the rule test plus the existing record tests. That's less friction, but we'd lose the full inventory diff when an arg gets reclassified.
Separately, in this PR I'd drop the FINGERPRINT markers from the five ID args so they default to hash. No changes until you choose.
There was a problem hiding this comment.
I think we probably need a DRY artifact per input arg name or with arg name pattern-based rules. Not in this PR but if the managed artifact is a data classification rules file that only updates when a new arg name shows up which doesn't match rules.
Then the json file is purely a generated build artifact which can be gitignored. And then the CI failure is to modify the rules file not the json.
There was a problem hiding this comment.
Makes sense. That's a better shape than my proposal. Here's how I'd sketch it for the follow-up:
- Managed artifact: a small rules file (for example
airbyte/mcp/arg_trace_rules.yaml) of arg-name patterns mapped to a class, such as*_id: hash,workspace_id: omit,*_name/prompt/name_contains: fingerprint, with exact names overriding patterns. - Generated artifact:
arg_trace_plan.jsongets gitignored and built from the live plan on demand. - CI failure: an arg whose name matches no rule, or whose live class contradicts its rule, fails with "add or adjust a rule in
arg_trace_rules.yaml". It no longer says "regenerate the JSON".
One design choice to settle: today the class comes from the TraceArg markers in the tool signatures. fastmcp-extensions only exposes a single global arg_default, with no name-based rules hook. So either:
- Rules validate the markers: this lives entirely in PyAirbyte. It's still two places to touch when a new arg pattern appears, but CI points you to both.
- Rules drive classification: fully DRY, because the markers go away except for real exceptions. This needs a small
fastmcp-extensionschange: anarg_ruleshook next toarg_default.
I'd lean toward 2 for DRY, landing 1 first if the upstream change is slow. Not touching this in #1392.
There was a problem hiding this comment.
Option 2. New PR in parallel.
There was a problem hiding this comment.
On it: option 2, in a separate parallel PR on release-candidate/v1, plus the upstream arg_rules hook in fastmcp-extensions. I'll link both here once they're open.
There was a problem hiding this comment.
Both are open as drafts:
- feat(otel): add name-pattern
arg_rulesfor argument tracing fastmcp-extensions#151 addsarg_rulesandtrace_plan(include_sources=True). - refactor(mcp): classify traced tool args from a rules file #1412 adds
airbyte/mcp/arg_trace_rules.yamland removes allTraceArgmarkers. It gitignores the JSON, and CI fails with "add a rule" for unclassified args.
After #1412 lands, this PR drops its markers and its IDs pick up *_id: hash.
Overview
👉 TL;DR: Agents can start a source schema discovery in Cloud without waiting for it, check on it later, and compare the resulting catalog with another catalog or with a connection's current one. Nothing is applied to the connection yet.
Specifically, this adds the MCP tool
discover_cloud_source_catalog, which is backed byCloudSource.start_catalog_discover,CloudSource.get_catalog_discover_result,CloudSource.get_latest_saved_catalog, andCloudWorkspace.compare_source_catalogs.Depends on #1397:
update_cloud_connection_unsafe#1397Replaces (with #1397):
Changes
discover_cloud_source_catalog(source_id, discover_request_id, no_refresh, source_catalog_id, compare_to_source_catalog_id, compare_to_connection_id, workspace_id), markeddestructive=False, pipeline_change=False. It supports five modes:source_id: starts an async discover via/v1/commands/run/discoverand returns a generateddiscover_request_id.source_id+no_refresh=True: returns the latest saved catalog's timestamp and stream count, read from/v1/sources/most_recent_source_actor_catalog.discover_request_id: returns the status from/v1/commands/status. Once the discover completes, it also returns the outcome,source_catalog_id, stream count, and failure reason from/v1/commands/output/discover.source_catalog_id+compare_to_source_catalog_id: returns a bounded diff summary andschema_changefrom/v1/catalogs/diff(persist_changes=false).source_catalog_id+compare_to_connection_id: the same diff, against the connection's currentsourceCatalogId.source_idanddiscover_request_idare mutually exclusive.no_refreshrequiressource_id.source_catalog_idrequires exactly onecompare_to_*argument.api_utilwrappers:run_source_discover_command,get_command_status,get_discover_command_output,get_most_recent_source_catalog, andget_catalog_diff.CloudSourceCatalogDiscoverResultandCloudSourceCatalogDiffStreamSummary._otel.py: the new endpoints are added to the safe-URL list.CloudConnection.import_raw_catalog()now clears the cached_connection_info, sostream_namesand the other cached properties no longer go stale after a catalog import. This bug predates the PR.Review Spotlight
Reviewers with limited time, please review first:
mcp/cloud.py#L4136:discover_cloud_source_catalog, its mode routing and input validationworkspaces.py#L528:compare_source_catalogsconnectors.py#L1185: starting and polling a discoverRisks
/v1/catalogs/diffbehavior come from reading the platform code.source_catalog_id, becausemost_recent_source_actor_catalogdoesn't return one.Follow-ups
apply_source_catalog_idtoupdate_cloud_connection/update_cloud_connection_unsafe, in this PR, after feat(mcp): add batch stream config overrides andupdate_cloud_connection_unsafe#1397 merges.Test plan
AIRBYTE_MCP_TRUSTED_EXECUTION=1 uv run pytest tests/unit_tests/test_cloud_source_catalog_discovery.py tests/unit_tests/test_cloud_api_util.py tests/unit_tests/test_cloud_connection_catalog_edits.py tests/unit_tests/test_mcp_otel.py tests/unit_tests/test_mcp_arg_trace_parity.pyruff check,ruff format --check,pyrefly checkLink to Devin session: https://app.devin.ai/sessions/4d1ece6885c44c52a07034e025db11e6
Open in Devin Desktop: https://app.devin.ai/desktop/session/4d1ece6885c44c52a07034e025db11e6?variant=devin
Requested by: Aaron ("AJ") Steers (@aaronsteers)