Repository navigation
feat(cloud): name conflicting destination streams and point to table_prefix on connection create - #1406
Conversation
Co-Authored-By: AJ Steers <[email protected]>
Co-Authored-By: AJ Steers <[email protected]>
Co-Authored-By: AJ Steers <[email protected]>
…200415-conflicting-stream-guidance
…200415-conflicting-stream-guidance
Co-Authored-By: AJ Steers <[email protected]>
Co-Authored-By: AJ Steers <[email protected]>
|
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/1792200415-conflicting-stream-guidance' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1792200415-conflicting-stream-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. |
There was a problem hiding this comment.
🟡 Changes recommended
Malformed Unicode in conflict fields can produce guidance that fails UTF-8 encoding.
1 open issue
What changed in this PR
Improves Cloud connection-creation errors by identifying destination table conflicts and explaining how to resolve them with table_prefix.
Changes:
- Adds capped conflict details with stream names and connection IDs.
- Extracts shared bounded JSON parsing.
- Updates parameter documentation and error-formatting tests.
| File | Description |
|---|---|
tests/unit_tests/test_cloud_api_util.py |
Tests conflict guidance, malformed entries, and agent-facing output. |
airbyte/mcp/cloud.py |
Documents table conflicts and table_prefix. |
airbyte/cloud/workspaces.py |
Clarifies prefix usage for deployed connections. |
airbyte/_util/cloud_errors.py |
Extracts shared problem-payload parsing. |
airbyte/_util/api_util.py |
Adds conflict details and prefix guidance. |
Review effort: Balanced
|
|
||
| stream_name = f"{namespace}.{name}" if namespace else name | ||
| rendered_ids = ", ".join(f"`{connection_id}`" for connection_id in connection_ids) | ||
| return f"`{stream_name}` (connection {rendered_ids})" |
There was a problem hiding this comment.
Not changing this. data.streams comes from Airbyte Cloud's own validation response, which is built from stream names and connection UUIDs that Cloud has already stored. We have no evidence of lone surrogates turning up there, so a guard would be speculative. The detail/title pre-check exists because those are free-text fields that our general Cloud error path renders for every tool. If this ever happens, the error will surface loudly instead of being silently skipped.
There was a problem hiding this comment.
👍 On it. I'm reversing my earlier "not changing this". The guard is small and handles the nested fields the same way parse_problem_payload already handles detail/title. Each rendered entry gets a UTF-8 encode check, and an entry that fails is skipped. If no entries are left, the generic table_prefix guidance is used. I'll add a test. This ships with the conflict-guidance rework.
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 devin/1792200236-run... 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if e.status_code == HTTPStatus.BAD_REQUEST: | ||
| raise AirbyteLibInputError( | ||
| message=sdk_error_message(e), | ||
| guidance=_conflicting_destination_stream_guidance(e), |
There was a problem hiding this comment.
🟡 Conflict errors expose prohibited extra guidance
For a conflicting-stream 400, create_connection adds table names and connection IDs to agent-facing errors. The Cloud error policy permits added guidance only for actor-not-ready and 403 failures.
Learn more
Cloud problem responses supply the error text through detail, or title when detail is empty. Both AGENTS.md and CONTRIBUTING.md limit PyAirbyte-added instructions to actor-not-ready setup and 403 access errors. The new conflict guidance adds an additional category and displays fields from data.streams, so it violates that restriction even when the Cloud message is preserved.
Example: A 400 conflict response with detail: "Conflicting stream" and a users stream owned by connection-1 now produces an agent error that also names users, connection-1, and table_prefix; previously it returned only the Cloud detail.
Recommended fix: Remove _conflicting_destination_stream_guidance from the create error path under the current policy. If this new exception to the policy is intended, update the mandatory Cloud error rules before adding it.
| guidance=_conflicting_destination_stream_guidance(e), | |
| guidance=None, |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Agreed: as written, this breaks the "Agent-facing Cloud Error Messages" rule. I've proposed adding this case to cloud_error_guidance() and updating the rule docs to list it, and I'm waiting on the maintainer's decision before reworking it.
There was a problem hiding this comment.
🙋 Human Input Needed: may this become a third allowed guidance case? Agreed that, as written, it breaks the rule. My proposal is to add a connection-conflicting-destination-stream case to cloud_error_guidance(), driven by data.streams, and list it in the rule in AGENTS.md and CONTRIBUTING.md. The alternative is to drop the added guidance and keep only the table_prefix docstring changes. This reply replaces my earlier one, which didn't follow the reply format.
| stream_name = f"{namespace}.{name}" if namespace else name | ||
| rendered_ids = ", ".join(f"`{connection_id}`" for connection_id in connection_ids) | ||
| return f"`{stream_name}` (connection {rendered_ids})" |
There was a problem hiding this comment.
🟡 Malformed stream names break conflict errors
When conflict metadata contains an escaped lone surrogate, _format_conflicting_destination_stream includes it in guidance. parse_problem_payload validates only the top-level message, so UTF-8 encoding of that guidance fails.
Learn more
A Cloud error body can be valid UTF-8 JSON while containing an escaped lone surrogate inside data.streams. parse_problem_payload checks encoding of the raw body and the selected top-level message, but does not validate nested strings. Rendering one of those strings into guidance produces text that cannot be encoded to UTF-8 for an agent response.
Example: The body {"type":"error:connection-conflicting-destination-stream","detail":"Conflict","data":{"streams":[{"streamName":"users\\ud800","connectionIds":["c1"]}]}} parses successfully. The guidance contains users\ud800, and encoding it as UTF-8 raises UnicodeEncodeError instead of returning the conflict message.
Recommended fix: Validate streamName, streamNamespace, and every connectionIds entry with UTF-8 encoding before formatting; skip malformed entries or fall back to a generic message. Test escaped lone surrogates in each field.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Same point as the Copilot thread on this function. data.streams comes from Cloud's own validation response, which is built from stored stream names and connection UUIDs, and we have no evidence of lone surrogates turning up there. Not adding a guard for now; this code is being reworked anyway (see the policy thread above).
There was a problem hiding this comment.
👍 On it. I'm reversing my earlier decline here too, with the same fix as the Copilot thread on this function: each rendered entry gets a UTF-8 encode check, an entry that fails is skipped, the generic table_prefix guidance is the fallback, and there's a test.
Overview
👉 TL;DR: When a new connection would write to tables another connection on the same destination already uses, the error now names the clashing tables and the connections that own them, and says to set a table prefix.
Specifically,
api_util.create_connectionaddsguidancebuilt from the Cloudconnection-conflicting-destination-streamproblem'sdata.streams, and thecreate_connection_on_cloud/deploy_connectiondocs nametable_prefixas the fix.Stacked on #1405:
run_cloud_sync409s and report the running job ID #1405Closes #1404
Changes
api_util.create_connection: on a 400error:connection-conflicting-destination-stream, adds guidance listing the clashing`namespace.stream` (connection `id`, ...)entries (first 10, then "and N more") and telling the caller to settable_prefix. Malformed entries are skipped. If no entry is usable, the guidance namestable_prefixwithout the list. Other 400s are unchanged.cloud_errors.parse_problem_payload: the existing bounded JSON parsing, pulled out ofparse_cloud_errorso both callers share it. No behavior change.create_connection_on_clouddocstring, itstable_prefixfield description, and theCloudWorkspace.deploy_connectiontable_prefixarg now state the one-table-per-destination rule.Review Spotlight
Reviewers with limited time, please review first:
api_util.py#L1996-L2067:data.streamsparsing + guidance textcloud_errors.py#L86-L105: extractedparse_problem_payloadRisks
run_cloud_sync409s and report the running job ID #1405. This PR must merge after it.Test plan
format_user_facing_erroroutput.ruff, docstring lint,pyrefly, and MCP arg-trace parity tests: all pass.Link to Devin session: https://app.devin.ai/sessions/1fd07730c0494565ba33fec3854b26d2
Open in Devin Desktop: https://app.devin.ai/desktop/session/1fd07730c0494565ba33fec3854b26d2?variant=devin
Requested by: Aaron ("AJ") Steers (@aaronsteers)