fix(cloud): wrap public API response models#1034
fix(cloud): wrap public API response models#1034Aaron ("AJ") Steers (aaronsteers) merged 8 commits into
Conversation
🤖 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/1779843864-pyairbyte-api-import-guard' pyairbyte --help
# Install PyAirbyte from this branch for development:
pip install 'git+https://github.com/airbytehq/PyAirbyte.git@devin/1779843864-pyairbyte-api-import-guard'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. |
|
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:
📝 WalkthroughWalkthroughAdds typed Cloud* info models and exports; migrates CloudClient, CloudWorkspace, connectors, sync results, and MCP code to use these models; broadens get_job_logs job_type to accept strings and adds schedule-builder helper; introduces AST-based import-guard tests and CI job; updates unit tests for CloudWorkspaceInfo. ChangesCloud workspace abstraction and job-type standardization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Would you like me to point out specific ranges where enum-to-string normalization or from_mapping vs from_api_response differences could cause subtle compatibility issues, wdyt? 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit_tests/test_cloud_credentials.py (1)
195-197: ⚡ Quick winConsider verifying workspaceId values too, wdyt?
The mock now includes
workspaceIdfor each workspace, but the assertion on line 216 only verifies thenameattribute. Adding a check forworkspaceIdvalues would strengthen the test by ensuring CloudWorkspaceInfo correctly extracts both fields from the API response.💡 Proposed enhancement to verify workspaceId
assert all(isinstance(workspace, CloudWorkspaceInfo) for workspace in result) assert [workspace.name for workspace in result] == ["target-one"] + assert result[0].workspace_id == "workspace-target-one"Also applies to: 216-216
🤖 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_cloud_credentials.py` around lines 195 - 197, The test currently asserts only the workspace "name" values (e.g., "miss", "target-one", "target-two") but ignores the added "workspaceId" fields; update the test that builds/validates CloudWorkspaceInfo to also assert the extracted workspaceId values match the mocked ones (e.g., "workspace-miss", "workspace-target-one", "workspace-target-two") alongside the existing name assertions — locate the assertion block that checks names for the mock workspaces and add corresponding assertions for workspaceId on the CloudWorkspaceInfo instances.
🤖 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.
Nitpick comments:
In `@tests/unit_tests/test_cloud_credentials.py`:
- Around line 195-197: The test currently asserts only the workspace "name"
values (e.g., "miss", "target-one", "target-two") but ignores the added
"workspaceId" fields; update the test that builds/validates CloudWorkspaceInfo
to also assert the extracted workspaceId values match the mocked ones (e.g.,
"workspace-miss", "workspace-target-one", "workspace-target-two") alongside the
existing name assertions — locate the assertion block that checks names for the
mock workspaces and add corresponding assertions for workspaceId on the
CloudWorkspaceInfo instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 75633ae2-f0ec-4532-8b75-1e96e5547189
📒 Files selected for processing (4)
airbyte/_util/api_util.pyairbyte/cloud/models.pytests/unit_tests/test_cloud_credentials.pytests/unit_tests/test_public_api_imports.py
🚧 Files skipped from review as they are similar to previous changes (2)
- airbyte/_util/api_util.py
- airbyte/cloud/models.py
devin-ai-integration
Bot
changed the title
Co-Authored-By: AJ Steers <aj@airbyte.io>
Code Coverage OverviewLanguages: Python Python / code-coverage/pytest-fastThe overall coverage in the branch is 66%. The coverage in the branch is 65%. Show a code coverage summary of the most impacted files.
Python / code-coverage/pytest-no-credsThe overall coverage in the branch is 66%. The coverage in the branch is 65%. Show a code coverage summary of the most impacted files.
Python / code-coverage/pytestThe overall coverage in the branch remains at 71%, unchanged from the branch. Show a code coverage summary of the most impacted files.
Updated |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (6)
airbyte/_util/api_util.py (1)
1495-1503: ⚡ Quick winConsider adding input validation and expanding the docstring?
This helper doesn't validate
schedule_typebefore passing it tomodels.ScheduleTypeEnum(schedule_type), so users will get a genericValueErrorrather than a clearPyAirbyteInputErrorif they pass an invalid value. Theget_job_logsfunction above (lines 895-905) shows a nice pattern for this—wdyt about following the same approach here for consistency?Also, the docstring could document the expected
schedule_typevalues (looks like"cron"and"manual"from the usage context) and parameter descriptions.🔧 Suggested enhancement
def build_connection_schedule( schedule_type: str, cron_expression: str | None = None, ) -> models.AirbyteAPIConnectionSchedule: - """Build a connection schedule object.""" + """Build a connection schedule object. + + Args: + schedule_type: The schedule type (e.g., "cron", "manual"). + cron_expression: Optional cron expression for cron-based schedules. + + Returns: + AirbyteAPIConnectionSchedule object ready for API submission. + + Raises: + PyAirbyteInputError: If schedule_type is not a valid ScheduleTypeEnum value. + """ + try: + schedule_type_enum = models.ScheduleTypeEnum(schedule_type) + except ValueError: + valid_schedule_types = ", ".join(st.value for st in models.ScheduleTypeEnum) + raise PyAirbyteInputError( + message=f"`schedule_type` must be one of: {valid_schedule_types}.", + input_value=schedule_type, + ) from None + return models.AirbyteAPIConnectionSchedule( - schedule_type=models.ScheduleTypeEnum(schedule_type), + schedule_type=schedule_type_enum, cron_expression=cron_expression, )🤖 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 `@airbyte/_util/api_util.py` around lines 1495 - 1503, The build_connection_schedule helper lacks input validation and a descriptive docstring; update its docstring to list parameters and allowed schedule_type values (e.g., "cron", "manual") and validate schedule_type before calling models.ScheduleTypeEnum by checking it against allowed values (or catching ValueError) and raising a clear PyAirbyteInputError on invalid input; reference the build_connection_schedule function and models.ScheduleTypeEnum and mirror the validation pattern used in get_job_logs to ensure consistent error handling.airbyte/cloud/connections.py (1)
318-334: 💤 Low valueConsider clarifying the docstring to match the enum type annotation?
The
job_typeparameter is typed asJobTypeEnum | None, but the docstring examples (sync,refresh) look like lowercase string literals. This might lead users to try passing strings directly. Maybe update the docstring to show enum usage likeJobTypeEnum.SYNCorJobTypeEnum.REFRESH, wdyt?🤖 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 `@airbyte/cloud/connections.py` around lines 318 - 334, The docstring for the method that accepts job_type: JobTypeEnum | None is ambiguous about accepted values; update the docstring for the function (the one described as "Get previous sync jobs for a connection with pagination support") to show the enum usage explicitly (e.g., JobTypeEnum.SYNC, JobTypeEnum.REFRESH), note that passing lowercase strings like "sync" or "refresh" is not expected, and clarify the default behavior when job_type is None (API defaults to sync and reset jobs). Also ensure the parameter description references the JobTypeEnum type rather than string literals so callers know to use the enum constant.airbyte/mcp/cloud.py (1)
1365-1372: 💤 Low valueMinor question about the organization_id fallback, wdyt?
At line 1369, when
ws.organization_idisNone, we fall back to""(empty string). This is needed sinceCloudWorkspaceResult.organization_idis typed asstr(required), butCloudWorkspaceInfo.organization_idis optional.Would it be clearer to use a sentinel value like
"[unavailable]"or"[unknown]"instead of an empty string? An empty string could be mistaken for a valid (but empty) organization ID, whereas a bracketed sentinel makes it obvious that the value is missing.Alternatively, should
CloudWorkspaceResult.organization_idbe optional (str | None) to match reality?🤖 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 `@airbyte/mcp/cloud.py` around lines 1365 - 1372, The code currently coerces a possibly None ws.organization_id to an empty string when constructing CloudWorkspaceResult in the list comprehension; update either CloudWorkspaceResult.organization_id to be Optional[str] (or str | None) and pass ws.organization_id directly, or keep it required but use an explicit sentinel like "[unavailable]" (e.g., workspace.organization_id or "[unavailable]") so missing values are unambiguous; modify the CloudWorkspaceResult dataclass/type and this comprehension in cloud.py (the CloudWorkspaceResult constructor usage and the CloudWorkspaceResult definition) accordingly to keep types consistent.airbyte/cloud/__init__.py (1)
90-90: 💤 Low valueShould the other Cloud*Info models be exported too, wdyt?
Right now only
CloudWorkspaceInfoand the enums are re-exported in__all__, but the models module also definesCloudConnectionInfo,CloudJobInfo,CloudSourceInfo,CloudDestinationInfo, andCloudCustomSourceDefinitionInfo.If these models are returned by public methods (e.g.,
SyncResult._get_connection_info()returnsCloudConnectionInfo), users might want to import them for type hints. Should we add them to__all__now, or are they intentionally kept internal for this PR?Also applies to: 125-129
🤖 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 `@airbyte/cloud/__init__.py` at line 90, The package currently re-exports only CloudWorkspaceInfo and the enums; add the other public model classes to the module exports so callers can import them for typing. Update the import line that currently references CloudWorkspaceInfo, JobStatusEnum, JobTypeEnum to also import CloudConnectionInfo, CloudJobInfo, CloudSourceInfo, CloudDestinationInfo, and CloudCustomSourceDefinitionInfo, and add those names to __all__ (also apply the same change at the other export block around lines 125-129). Ensure functions that return these types (e.g., SyncResult._get_connection_info) will now reference the exported model names for consumers' type hints.tests/unit_tests/test_public_api_imports.py (2)
25-28: ⚡ Quick winFail fast if the guard root resolves to zero Python files (
_python_filessilent no-op)
_python_files()(tests/unit_tests/test_public_api_imports.py:25-28) will return an empty list if the configured root doesn’t exist or has no*.pyfiles, which would let the checks pass without actually validating anything. In the current repo,airbyte/cli,airbyte/mcp, andairbyte/cloudare directories with.pyfiles, so this won’t trigger today—would you add a smallpytest.failfor “path missing” / “no python files found” to make the guard more robust to future renames/moves?🛡️ Possible approach
def _python_files(path: Path) -> list[Path]: + if not path.exists(): + pytest.fail(f"Configured guard path does not exist: {path}") if path.is_file(): return [path] - return sorted(path.rglob("*.py")) + files = sorted(path.rglob("*.py")) + if not files: + pytest.fail(f"No Python files found under guard path: {path}") + return files🤖 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_public_api_imports.py` around lines 25 - 28, The helper _python_files currently returns an empty list when the given Path doesn't exist or contains no .py files, allowing tests to silently pass; update _python_files in tests/unit_tests/test_public_api_imports.py to fail fast by calling pytest.fail with a clear message when path does not exist or when the resulting python_files list is empty instead of returning an empty list; locate the function _python_files and add checks for path.exists() and len(python_files) == 0 (or similar) and invoke pytest.fail("...") with messages like "path missing" or "no python files found" so the guard no longer silently no-ops.
126-129: 💤 Low value
.valuematcher is quite broad — intentional?
reference.endswith(".value")will flag any attribute access ending in.valueinairbyte/cliandairbyte/mcp, not just generated-enum.valueaccess (e.g. a Pydantic field, dict-wrapper, or third-party object exposing.value). That could produce confusing false positives that block CI for unrelated code. Also note thatsomething().value(call in the chain) won't be caught since_attribute_referencedrops non-Namebases, so the matching is both over- and under-inclusive. Is the blunt suffix match the behavior you want here, wdyt?#!/bin/bash # Surface existing `.value` accesses under the guarded presentation modules rg -nP '\.value\b' airbyte/cli airbyte/mcp🤖 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_public_api_imports.py` around lines 126 - 129, The current suffix check reference.endswith(".value") is too broad; narrow it to only flag generated-enum accesses by making the check AST-aware: modify _attribute_reference to expose the attribute name and the base node (or return a tuple like (base_node, attr_name)), then replace the reference.endswith(".value") branch to 1) ensure attr_name == "value", 2) ensure base_node is an ast.Name (to exclude chained calls/expressions), and 3) verify the base identifier matches your generated-enum naming rule (e.g., endswith "Enum" or is in a maintained set of generated enum identifiers); only then append the violation to restricted_references/violations. Use the symbols _attribute_reference, restricted_references, and reference handling to locate and change the logic.
🤖 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.
Nitpick comments:
In `@airbyte/_util/api_util.py`:
- Around line 1495-1503: The build_connection_schedule helper lacks input
validation and a descriptive docstring; update its docstring to list parameters
and allowed schedule_type values (e.g., "cron", "manual") and validate
schedule_type before calling models.ScheduleTypeEnum by checking it against
allowed values (or catching ValueError) and raising a clear PyAirbyteInputError
on invalid input; reference the build_connection_schedule function and
models.ScheduleTypeEnum and mirror the validation pattern used in get_job_logs
to ensure consistent error handling.
In `@airbyte/cloud/__init__.py`:
- Line 90: The package currently re-exports only CloudWorkspaceInfo and the
enums; add the other public model classes to the module exports so callers can
import them for typing. Update the import line that currently references
CloudWorkspaceInfo, JobStatusEnum, JobTypeEnum to also import
CloudConnectionInfo, CloudJobInfo, CloudSourceInfo, CloudDestinationInfo, and
CloudCustomSourceDefinitionInfo, and add those names to __all__ (also apply the
same change at the other export block around lines 125-129). Ensure functions
that return these types (e.g., SyncResult._get_connection_info) will now
reference the exported model names for consumers' type hints.
In `@airbyte/cloud/connections.py`:
- Around line 318-334: The docstring for the method that accepts job_type:
JobTypeEnum | None is ambiguous about accepted values; update the docstring for
the function (the one described as "Get previous sync jobs for a connection with
pagination support") to show the enum usage explicitly (e.g., JobTypeEnum.SYNC,
JobTypeEnum.REFRESH), note that passing lowercase strings like "sync" or
"refresh" is not expected, and clarify the default behavior when job_type is
None (API defaults to sync and reset jobs). Also ensure the parameter
description references the JobTypeEnum type rather than string literals so
callers know to use the enum constant.
In `@airbyte/mcp/cloud.py`:
- Around line 1365-1372: The code currently coerces a possibly None
ws.organization_id to an empty string when constructing CloudWorkspaceResult in
the list comprehension; update either CloudWorkspaceResult.organization_id to be
Optional[str] (or str | None) and pass ws.organization_id directly, or keep it
required but use an explicit sentinel like "[unavailable]" (e.g.,
workspace.organization_id or "[unavailable]") so missing values are unambiguous;
modify the CloudWorkspaceResult dataclass/type and this comprehension in
cloud.py (the CloudWorkspaceResult constructor usage and the
CloudWorkspaceResult definition) accordingly to keep types consistent.
In `@tests/unit_tests/test_public_api_imports.py`:
- Around line 25-28: The helper _python_files currently returns an empty list
when the given Path doesn't exist or contains no .py files, allowing tests to
silently pass; update _python_files in
tests/unit_tests/test_public_api_imports.py to fail fast by calling pytest.fail
with a clear message when path does not exist or when the resulting python_files
list is empty instead of returning an empty list; locate the function
_python_files and add checks for path.exists() and len(python_files) == 0 (or
similar) and invoke pytest.fail("...") with messages like "path missing" or "no
python files found" so the guard no longer silently no-ops.
- Around line 126-129: The current suffix check reference.endswith(".value") is
too broad; narrow it to only flag generated-enum accesses by making the check
AST-aware: modify _attribute_reference to expose the attribute name and the base
node (or return a tuple like (base_node, attr_name)), then replace the
reference.endswith(".value") branch to 1) ensure attr_name == "value", 2) ensure
base_node is an ast.Name (to exclude chained calls/expressions), and 3) verify
the base identifier matches your generated-enum naming rule (e.g., endswith
"Enum" or is in a maintained set of generated enum identifiers); only then
append the violation to restricted_references/violations. Use the symbols
_attribute_reference, restricted_references, and reference handling to locate
and change the logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f5344726-9680-4805-99bf-d1852075f828
📒 Files selected for processing (11)
.github/workflows/python_lint.ymlairbyte/_util/api_util.pyairbyte/cloud/__init__.pyairbyte/cloud/connections.pyairbyte/cloud/connectors.pyairbyte/cloud/constants.pyairbyte/cloud/models.pyairbyte/cloud/sync_results.pyairbyte/mcp/cloud.pytests/unit_tests/test_mcp_cloud.pytests/unit_tests/test_public_api_imports.py
✅ Files skipped from review due to trivial changes (1)
- airbyte/cloud/constants.py
Co-Authored-By: AJ Steers <aj@airbyte.io>
|
CodeRabbit also flagged the outside-diff MCP status serialization issue. Confirmed it was real: Fixed in
Validated locally with:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit_tests/test_public_api_imports.py (1)
114-125:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCould we use prefix matching here to catch actual usage, wdyt?
The reference check at line 122 uses exact match (
if reference in restricted_references), but actual code that accesses generated models would create references like"api_util.models.JobTypeEnum"or"api_util.models.SomeClass.value", not the bare"api_util.models". These wouldn't match the tuple entry and would slip through.In contrast, the import check at line 93 correctly uses
_is_restricted_import, which does prefix matching. Should the reference check follow the same pattern?🔍 Proposed fix: prefix-match restricted references
violations: list[str] = [] for file_path in _python_files(REPO_ROOT / relative_path): for reference in _attribute_references(file_path): - if reference in restricted_references: - violations.append( - f"{file_path.relative_to(REPO_ROOT)} references {reference}" - ) + for restricted_reference in restricted_references: + if reference == restricted_reference or reference.startswith( + f"{restricted_reference}." + ): + violations.append( + f"{file_path.relative_to(REPO_ROOT)} references {reference}" + ) + break🤖 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_public_api_imports.py` around lines 114 - 125, The test test_public_modules_do_not_reference_generated_api_model_namespaces uses an exact membership check (if reference in restricted_references) which misses attribute usages like "api_util.models.JobTypeEnum"; update the check to perform prefix matching against restricted_references (e.g., use the existing helper _is_restricted_import or implement any(reference.startswith(prefix) for prefix in restricted_references)) when iterating _attribute_references(file_path) so violations captures any reference that begins with a restricted namespace; keep the rest of the loop (violations append and file iteration via _python_files and _attribute_references) unchanged.
🧹 Nitpick comments (1)
airbyte/mcp/cloud.py (1)
785-785: ⚡ Quick win
get_job_status()won’t returnNone, so.valueatairbyte/mcp/cloud.py(~785) shouldn’t raise anAttributeError.
SyncJobResult.get_job_status()is typed to returnJobStatusEnumand returnsCloudJobInfo.status, which is also typed as a non-optionalJobStatusEnum(constructed viaCloudJobInfo.from_api_response). Would you consider simplifying the redundantjob_status else Noneguard at ~1256?🤖 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 `@airbyte/mcp/cloud.py` at line 785, The code is guarding against a None job status even though SyncJobResult.get_job_status() is typed to return a non-optional JobStatusEnum (it returns CloudJobInfo.status built by CloudJobInfo.from_api_response), so remove the redundant None-guard and simplify usage: replace the conditional/ternary that does "job_status else None" with the direct JobStatusEnum and stop treating get_job_status() as optional (e.g., use sync_result.get_job_status().value directly), and remove the unnecessary None-handling in SyncJobResult construction or any code paths that create CloudJobInfo.from_api_response to ensure job_status is always a JobStatusEnum.
🤖 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.
Outside diff comments:
In `@tests/unit_tests/test_public_api_imports.py`:
- Around line 114-125: The test
test_public_modules_do_not_reference_generated_api_model_namespaces uses an
exact membership check (if reference in restricted_references) which misses
attribute usages like "api_util.models.JobTypeEnum"; update the check to perform
prefix matching against restricted_references (e.g., use the existing helper
_is_restricted_import or implement any(reference.startswith(prefix) for prefix
in restricted_references)) when iterating _attribute_references(file_path) so
violations captures any reference that begins with a restricted namespace; keep
the rest of the loop (violations append and file iteration via _python_files and
_attribute_references) unchanged.
---
Nitpick comments:
In `@airbyte/mcp/cloud.py`:
- Line 785: The code is guarding against a None job status even though
SyncJobResult.get_job_status() is typed to return a non-optional JobStatusEnum
(it returns CloudJobInfo.status built by CloudJobInfo.from_api_response), so
remove the redundant None-guard and simplify usage: replace the
conditional/ternary that does "job_status else None" with the direct
JobStatusEnum and stop treating get_job_status() as optional (e.g., use
sync_result.get_job_status().value directly), and remove the unnecessary
None-handling in SyncJobResult construction or any code paths that create
CloudJobInfo.from_api_response to ensure job_status is always a JobStatusEnum.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aea7226a-6fbf-4a20-9c06-606b2a6debe9
📒 Files selected for processing (3)
.github/workflows/python_lint.ymlairbyte/mcp/cloud.pytests/unit_tests/test_public_api_imports.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/python_lint.yml
Co-Authored-By: AJ Steers <aj@airbyte.io>
|
Confirmed the import-guard finding is real: exact matching would miss Fixed in Validated locally with:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces PyAirbyte-owned (public) Airbyte Cloud response models and enums to avoid leaking generated airbyte_api types through public Cloud/MCP/CLI surfaces, and adds CI enforcement to prevent regressions.
Changes:
- Added
airbyte.cloud.modelswith public wrapper models (workspace/connection/job/source/destination/custom definition) plusJobStatusEnum/JobTypeEnum. - Updated Cloud + MCP codepaths to return/use the new wrapper models and enums instead of generated Speakeasy response models.
- Added a linting test + CI job to guard against importing or referencing generated API model namespaces from public modules.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
airbyte/cloud/models.py |
Adds PyAirbyte-owned public Cloud API response models and enums. |
airbyte/cloud/client.py |
Returns CloudWorkspaceInfo from workspace endpoints instead of generated response models. |
airbyte/cloud/workspaces.py |
Wraps workspace listing responses into CloudWorkspaceInfo. |
airbyte/cloud/connections.py |
Wraps connection/job responses and routes schedule building through api_util helper. |
airbyte/cloud/connectors.py |
Wraps source/destination/custom-source-definition responses into public model objects. |
airbyte/cloud/sync_results.py |
Caches/wraps job + connection responses using public wrapper models. |
airbyte/cloud/constants.py |
Uses public JobStatusEnum for status sets. |
airbyte/cloud/__init__.py |
Re-exports new public models/enums at package top-level. |
airbyte/mcp/cloud.py |
Uses public JobTypeEnum and outputs job status via .value. |
airbyte/_util/api_util.py |
Allows job_type filter to be passed as string and adds build_connection_schedule(). |
tests/unit_tests/test_public_api_imports.py |
Adds AST-based guard to block generated model imports/references from public modules. |
tests/unit_tests/test_cloud_credentials.py |
Updates tests for new workspace return types (CloudWorkspaceInfo). |
tests/unit_tests/test_mcp_cloud.py |
Updates MCP tests to use public JobStatusEnum. |
.github/workflows/python_lint.yml |
Adds “Public API Import Guard” CI job running the new linting test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Co-Authored-By: AJ Steers <aj@airbyte.io>
Aaron ("AJ") Steers (aaronsteers)
merged commit 2170721
into
main
Aaron ("AJ") Steers (aaronsteers)
deleted the
devin/1779843864-pyairbyte-api-import-guard
branch
Summary
airbyte_apiresponse models.JobStatusEnumandJobTypeEnumvalues for public job status/type signatures; generated Speakeasy enums stay behindairbyte._util.api_utilconversion boundaries.public-api-import-guard) that blocksairbyte_apiandairbyte._util.api_importsimports plus generated model namespace references from public CLI/MCP/Cloud modules.patch_connection(), wideningCloudConnection.get_previous_sync_logs()to accept string job types, strengthening restricted-reference matching, hardening checkout credentials, and cleaning workflow comments.Local validation passed:
uv run ruff format --check airbyte/_util/api_util.py airbyte/cloud/connections.py tests/unit_tests/test_cloud_api_util.py tests/unit_tests/test_public_api_imports.pyuv run ruff check airbyte/_util/api_util.py airbyte/cloud/connections.py tests/unit_tests/test_cloud_api_util.py tests/unit_tests/test_public_api_imports.pyuv run pytest -q -m lintinguv run pytest -q tests/unit_tests/test_cloud_api_util.py tests/unit_tests/test_public_api_imports.py tests/unit_tests/test_cloud_credentials.pyuv run pyrefly checkLink to Devin session: https://app.devin.ai/sessions/a1ddc40130ad4cf09fcf4b8cd7b82354
Requested by: Aaron ("AJ") Steers (@aaronsteers)