[Aaron ("AJ") Steers (aaronsteers)]

Overview

👉 TL;DR: Lets manifest-only connectors whose catalog is fully static return their catalog without credentials or any config. Specifically, discover accepts a missing --config when BaseConnector.check_config_during_discover is False. ConcurrentDeclarativeSource sets this flag from _requires_config_for_discover().

Changes

• BaseConnector.check_config_during_discover: bool = True is a new public flag. Non-declarative sources are unchanged.
• ConcurrentDeclarativeSource._requires_config_for_discover() returns True (config required) when the manifest contains any of the following:
  • ConditionalStreams, because the stream set depends on the config.
  • A DynamicSchemaLoader anywhere under streams or dynamic_streams, including list loaders and inactive conditional branches.
  • A dynamic_streams resolver other than ParametrizedComponentsResolver, since the HTTP and config resolvers need the API or the config.
• AirbyteEntrypoint changes:
  • discover --config is now optional.
  • With no config and a config-free source, _discover_without_config() calls source.discover(logger, {}) and skips spec validation.
  • With no config and a source that needs one, it raises a config_error trace telling the user to pass --config.
  • A crash during config-free discover is wrapped as a config_error with the same guidance.
  • Discover with an explicit --config still validates against the spec as before.
• source-declarative-manifest CLI: a missing --config becomes {} (previously None). If building the source fails during config-free discover, the trace is a config_error with the --config guidance instead of "Error starting the sync".
• Tests: parametrized detection tests against a real ConcurrentDeclarativeSource, and entrypoint tests for both branches, error wrapping, and validation when a config is provided.

$ source-declarative-manifest discover --manifest-path manifest.yaml   # no --config
static manifest                -> CATALOG
needs config (see above)       -> TRACE config_error: "This connector requires a config to run `discover`. Provide one with `--config`."
reads config while building    -> TRACE config_error: "This connector could not run `discover` without a config. ..."

Review Spotlight

Reviewers with limited time, please review first:

• concurrent_declarative_source.py#L984-L1014: which manifests count as config-free.
• entrypoint.py#L189-L208 and #L291-L306: CLI branching and the config-free discover path.

Risks

• A static manifest that reads config while building its streams (for example, an incremental cursor start_datetime from config['start_date']) still can't discover without a config. It now fails with a config_error asking for --config instead of a raw error. Discover still builds the full stream objects.
• The legacy ManifestDeclarativeSource is unchanged and keeps the default check_config_during_discover = True.

Evidence

Ran source-declarative-manifest discover --manifest-path <monorepo manifest> locally without --config:

Connector	Result
source-activecampaign	CATALOG, 6 streams
source-7shifts	config_error: start_date is read while streams are built
source-airtable	config_error: uses DynamicSchemaLoader
source-activecampaign + default_concurrency: "{{ config['num_workers'] }}"	config_error from source construction

Connector coverage

Each of the 516 manifest-only sources in airbytehq/airbyte was run with source-declarative-manifest discover --manifest-path manifest.yaml [--components-path components.py] against this branch, with no --config.

Result	Sources
Returns a catalog: works once the connector's base image is bumped to a CDK release with this PR	306
Needs streams built without config or auth (follow-up refactor)	200
Needs a config by design (DynamicSchemaLoader, ConditionalStreams, or API-resolved dynamic_streams)	10

Most of the refactor group (156 sources) fails because a datetime component parses config['start_date'] while streams are built. source-github fails because RateLimitedMultipleTokenAuthenticator raises when it has no tokens.

Works after version bump (306)

source-activecampaign, source-agilecrm, source-aha, source-airbyte, source-akeneo, source-alpha-vantage, source-amazon-ads, source-apify-dataset, source-app-store-connect, source-appfollow, source-apptivo, source-aws-cloudtrail, source-babelforce, source-bamboo-hr, source-bigmailer, source-blogger, source-box, source-breezometer, source-breezy-hr, source-bugsnag, source-bunny-inc, source-cal-com, source-calendly, source-campaign-monitor, source-campayn, source-canny, source-captain-data, source-care-quality-commission, source-chargedesk, source-chargify, source-chartmogul, source-chift, source-churnkey, source-cin7, source-cisco-meraki, source-claude, source-clazar, source-clickup-api, source-clockify, source-cloudbeds, source-coassemble, source-coda, source-coin-api, source-coinmarketcap, source-concord, source-configcat, source-confluence, source-copper, source-countercyclical, source-customer-io, source-customerly, source-datadog, source-dbt, source-defillama, source-deputy, source-devin-ai, source-discord, source-dockerhub, source-dremio, source-drift, source-drip, source-e-conomic, source-easypromos, source-emailoctopus, source-employment-hero, source-encharge, source-eventee, source-eventzilla, source-everhour, source-ezofficeinventory, source-fastbill, source-feishu, source-fillout, source-financial-modelling, source-finnhub, source-finnworlds, source-firehydrant, source-fleetio, source-flexmail, source-flowlu, source-formbricks, source-free-agent-connector, source-freightview, source-freshbooks, source-freshdesk, source-freshsales, source-fulcrum, source-fullstory, source-gainsight-px, source-getgist, source-getlago, source-gitbook, source-glassfrog, source-gnews, source-gocardless, source-goldcast, source-google-calendar, source-google-classroom, source-google-forms, source-google-pagespeed-insights, source-google-webfonts, source-grafana, source-granola, source-greythr, source-gutendex, source-harness, source-hellobaton, source-hibob, source-hoorayhr, source-hubplanner, source-hugging-face-datasets, source-humanitix, source-huntr, source-illumina-basespace, source-imagga, source-incident-io, source-inflowinventory, source-instagram, source-instatus, source-interzoid, source-intruder, source-invoiced, source-invoiceninja, source-ip2whois, source-jamf-pro, source-jobnimbus, source-just-sift, source-k6-cloud, source-keka, source-kisi, source-kissmetrics, source-klarna, source-launchdarkly, source-leadfeeder, source-lemlist, source-lightspeed-retail, source-linkrunner, source-lokalise, source-luma, source-mailerlite, source-mailgun, source-mailjet-mail, source-mailjet-sms, source-mailosaur, source-mailtrap, source-marketstack, source-mention, source-mercado-ads, source-merge, source-metabase, source-metricool, source-microsoft-entra-id, source-microsoft-lists, source-microsoft-teams, source-miro, source-mode, source-my-hours, source-n8n, source-news-api, source-newsdata, source-nexus-datasets, source-nocrm, source-northpass-lms, source-notion, source-nutshell, source-nylas, source-omnisend, source-onepagecrm, source-onfleet, source-open-data-dc, source-openai-ads, source-openaq, source-openfda, source-openweather, source-opinion-stage, source-opuswatch, source-oura, source-outlook, source-oveit, source-pabbly-subscriptions-billing, source-pagerduty, source-paperform, source-papersign, source-partnerize, source-payfit, source-pendo, source-persistiq, source-persona, source-pexels-api, source-pingdom, source-pipedrive, source-pipeliner, source-pivotal-tracker, source-piwik, source-planhat, source-plausible, source-pocket, source-pokeapi, source-polygon-stock-api, source-poplar, source-pretix, source-primetric, source-printify, source-productive, source-pylon, source-pypi, source-qonto, source-qualaroo, source-railz, source-ramp, source-recreation, source-recruitee, source-recurly, source-referralhero, source-rentcast, source-repairshopr, source-reply-io, source-reprise, source-ringcentral, source-rocket-chat, source-rocketlane, source-ruddr, source-safetyculture, source-sage-hr, source-salesflare, source-sap-fieldglass, source-savvycal, source-scryfall, source-secoda, source-sendinblue, source-sendpulse, source-sentry, source-serpstat, source-sigma-computing, source-simfin, source-simplecast, source-simplesat, source-smaily, source-smartengage, source-smartreach, source-smsmode-rcs, source-smsmode-sms, source-snapchat-marketing, source-sonar-cloud, source-spacex-api, source-split-io, source-spotify-ads, source-spotlercrm, source-statuspage, source-stigg, source-stockdata, source-stripe, source-survey-sparrow, source-systeme, source-taboola, source-teamtailor, source-tempo, source-testrail, source-thinkific-courses, source-thinkific, source-ticketmaster, source-tickettailor, source-tinyemail, source-tmdb, source-todoist, source-toggl, source-track-pms, source-trello, source-tremendous, source-twelve-data, source-twilio-taskrouter, source-twilio, source-twitter, source-tyntec-sms, source-ubidots, source-unleash, source-uptick, source-us-census, source-vantage, source-visma-economic, source-vitally, source-waiteraid, source-weatherstack, source-web-scrapper, source-when-i-work, source-whisky-hunter, source-workramp, source-wrike, source-wufoo, source-xkcd, source-xsolla, source-you-need-a-budget-ynab, source-younium, source-youtube-analytics, source-zapier-supported-storage, source-zendesk-sell, source-zenefits, source-zoho-analytics-metadata-api, source-zoho-bigin, source-zoho-billing, source-zoho-campaign, source-zoho-desk, source-zoho-expense, source-zoho-invoice, source-zonka-feedback, source-zoom

Needs the stream-construction refactor (200)

source-100ms, source-7shifts, source-acuity-scheduling, source-adobe-commerce-magento, source-aircall, source-algolia, source-alpaca-broker-api, source-amplitude, source-appcues, source-appfigures, source-apple-search-ads, source-ashby, source-assemblyai, source-auth0, source-aviationstack, source-awin-advertiser, source-basecamp, source-beamer, source-bigcommerce, source-bitly, source-bluetally, source-boldsign, source-braze, source-brevo, source-brex, source-buildkite, source-buzzsprout, source-callrail, source-capsule-crm, source-castor-edc, source-chameleon, source-chargebee, source-cimis, source-circa, source-circleci, source-clarif-ai, source-clockodo, source-close-com, source-codefresh, source-coingecko-coins, source-commission-junction, source-convertkit, source-criteo-marketing, source-cursor, source-datascope, source-delighted, source-ding-connect, source-dixa, source-docuseal, source-dolibarr, source-dropbox-sign, source-dust, source-dwolla, source-easypost, source-ebay-finance, source-ebay-fulfillment, source-elasticemail, source-eventbrite, source-exchange-rates, source-factorial, source-fastly, source-finage, source-flexport, source-float, source-freshchat, source-freshservice, source-front, source-giphy, source-github, source-gitlab, source-gmail, source-gologin, source-gong, source-google-tasks, source-gorgias, source-greenhouse, source-guru, source-harvest, source-height, source-help-scout, source-high-level, source-insightful, source-insightly, source-intercom, source-jira, source-jotform, source-judge-me-reviews, source-justcall, source-katana, source-klaus-api, source-klaviyo, source-less-annoying-crm, source-lever-hiring, source-linear, source-linkedin-pages, source-lob, source-mailchimp, source-mailersend, source-mantle, source-mendeley, source-missive, source-mixmax, source-mntn, source-monday, source-mux, source-nasa, source-navan, source-nebius-ai, source-newsdata-io, source-nexiopay, source-ninjaone-rmm, source-nytimes, source-oncehub, source-onesignal, source-open-exchange-rates, source-opsgenie, source-orb, source-outreach, source-paddle, source-pandadoc, source-pardot, source-partnerstack, source-paypal-transaction, source-paystack, source-pennylane, source-perigon, source-perk, source-phyllo, source-picqer, source-plaid, source-postmarkapp, source-prestashop, source-productboard, source-quickbooks, source-rd-station-marketing, source-reddit, source-reddit-ads, source-retailexpress-by-maropost, source-retently, source-revenuecat, source-revolut-merchant, source-rollbar, source-rootly, source-salesloft, source-segment, source-sendgrid, source-sendowl, source-senseforce, source-sharetribe, source-shiftbase, source-shippo, source-shipstation, source-shopwired, source-shortcut, source-shortio, source-shutterstock, source-signnow, source-slack, source-smartwaiver, source-solarwinds-service-desk, source-sparkpost, source-square, source-squarespace, source-statsig, source-strava, source-survicate, source-svix, source-tavus, source-teamwork, source-the-guardian-api, source-thrive-learning, source-ticktick, source-timely, source-trustpilot, source-tvmaze-schedule, source-typeform, source-uppromote, source-uservoice, source-veeqo, source-vercel, source-vwo, source-wasabi-stats-api, source-watchmode, source-wikipedia-pageviews, source-woocommerce, source-wordpress, source-workable, source-workflowmax, source-xero, source-yahoo-finance-price, source-yotpo, source-yousign, source-youtube-data, source-zapsign, source-zendesk-chat, source-zendesk-sunshine, source-zendesk-support, source-zendesk-talk, source-zoho-books, source-zoho-inventory

Needs a config by design (10)

source-airtable, source-amazon-seller-partner, source-bing-ads, source-google-analytics-data-api, source-google-search-console, source-google-sheets, source-hubspot, source-linkedin-ads, source-pinterest, source-tiktok-marketing

Related

• Replaces earlier attempts feat: skip config validation during discovery for declarative sources that don't use DynamicSchemaLoader #464, feat: skip config validation during discovery for sources with DynamicSchemaLoader #466, and feat: skip config validation during discovery for sources with DynamicSchemaLoader #467.
• Platform-side catalog caching for static schemas: https://github.com/airbytehq/airbyte-internal-issues/issues/402

Link to Devin session: https://app.devin.ai/sessions/ae9d8c8aceea4199ac2609308e973916
Open in Devin Desktop: https://app.devin.ai/desktop/session/ae9d8c8aceea4199ac2609308e973916?variant=devin
Requested by: Aaron ("AJ") Steers (@aaronsteers)

[coderabbitai]

📝 Walkthrough

Walkthrough

Allows discover to run without an explicit config when the source can load schemas dynamically. Adds a check_config_during_discover flag, detects DynamicSchemaLoader usage in declarative sources, updates CLI/entrypoint parsing/behavior for discover, and adds tests for the new flows.

Changes

Cohort / File(s)	Summary
CLI Config Parsing airbyte_cdk/cli/source_declarative_manifest/_run.py	_parse_inputs_into_config_catalog_state now assigns {} (empty dict) instead of None when config is missing or falsy.
Core Framework — Connector Flags airbyte_cdk/connector.py	Added check_config_during_discover: bool = True to BaseConnector with docstring; clarified check_config_against_spec docstring.
Entrypoint — Discovery Flow airbyte_cdk/entrypoint.py	Made --config optional for discover; use check_config_during_discover to decide validation during discover; added branch to run discover with empty config or raise a clear ValueError when unsupported.
Legacy Manifest Declarative Source airbyte_cdk/legacy/sources/declarative/manifest_declarative_source.py	Added check_config_during_discover: bool = False; added _uses_dynamic_schema_loader() to detect DynamicSchemaLoader usage and set the flag after post-processing.
Concurrent Declarative Source airbyte_cdk/sources/declarative/concurrent_declarative_source.py	Added _uses_dynamic_schema_loader() helper and initialize check_config_during_discover in __init__ based on detection.
Tests — New & Updated unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py, unit_tests/test_entrypoint.py, airbyte_cdk/test/standard_tests/source_base.py	New tests for dynamic vs inline schema discovery and check_config_during_discover; updated entrypoint tests to cover discover-without-config behavior; adjusted standard tests to add preliminary check step and update skip/expectation handling.
Formatting / Minor airbyte_cdk/test/entrypoint_wrapper.py	Removed an extra blank line (no behavioral change).

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Entrypoint
    participant Source

    rect rgb(220,240,255)
    Note over Entrypoint: Old flow — discover required config
    User->>Entrypoint: discover (no config)
    Entrypoint->>Entrypoint: error: config required
    end

    rect rgb(240,255,240)
    Note over Entrypoint,Source: New flow — conditional requirement
    User->>Entrypoint: discover (no config)
    Entrypoint->>Source: query check_config_during_discover?
    alt Source says config not required (False)
        Source-->>Entrypoint: False
        Entrypoint->>Source: discover(config = {})
        Source-->>Entrypoint: AirbyteCatalog
    else Source requires config (True)
        Source-->>Entrypoint: True
        Entrypoint->>Entrypoint: raise ValueError (helpful message)
    end
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Areas to inspect closely:

• airbyte_cdk/entrypoint.py — branching logic for discover (empty config path) and the raised ValueError messaging.
• airbyte_cdk/legacy/sources/declarative/manifest_declarative_source.py and airbyte_cdk/sources/declarative/concurrent_declarative_source.py — correctness and completeness of _uses_dynamic_schema_loader() traversal across streams, dynamic_streams, and loaders.
• unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py and unit_tests/test_entrypoint.py — ensure tests reflect intended behavior for both dynamic and inline schema loaders and for both paths (with/without config).

Possibly related PRs

• chore(concurrent_declarative_source): Integrate ManifestDeclarativeSource into ConcurrentDeclarativeSource #704 — related changes to declarative discovery behavior and use of check_config_during_discover.
• feat(low-code cdk): add dynamic schema loader #104 — introduces DynamicSchemaLoader types that the detection logic depends on.
• tests: add sdm unit tests from source-declarative-manifest connector directory #82 — overlaps CLI/entrypoint parsing/handling for --config and discover flows.

Suggested reviewers

• maxi297
• brianjlai
• aldogonzalez8

Question for the author: the BaseConnector default for check_config_during_discover is True while ManifestDeclarativeSource sets it to False until runtime detection — did you consider aligning the manifest default with the base class, or is the asymmetry intentional to reflect post-processing detection, wdyt?

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 61.90% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The PR title "feat: Add unprivileged and config-free discover for declarative static schemas" accurately and specifically describes the main change introduced in this pull request. The title directly reflects the primary feature being added—enabling discovery without configuration for declarative sources using static schemas—and appropriately scopes it to that specific use case. The title is concise, follows conventional commit format, and provides clear context that a developer scanning the PR history would understand the primary purpose of the changes.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch aj/feat/unprivileged-discover-for-declarative-static-schemas

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

airbyte_cdk/test/standard_tests/connector_base.py (1)

123-138: Good fixes addressing the previous review comments!

The null check for result._messages and the correct use of scenario.expected_outcome.expect_exception() both address the issues flagged in previous reviews.

However, there's a formatting issue that needs attention, wdyt?

         if (
-            scenario.expected_outcome.expect_exception()
-            and conn_status.status == Status.SUCCEEDED
+            scenario.expected_outcome.expect_exception()
+            and conn_status.status == Status.SUCCEEDED
             and not result.errors
         ):

🧹 Nitpick comments (1)

airbyte_cdk/test/standard_tests/connector_base.py (1)

153-193: Solid implementation of scenario loading with good error handling!

The method properly loads YAML config, validates scenarios, and handles path resolution. The filtering logic for "iam_role" tests and path conversion to absolute paths is well thought out.

One small suggestion: consider making the categories configurable rather than hardcoded, wdyt?

 @classmethod
 def get_scenarios(
     cls,
+    categories: list[str] | None = None,
 ) -> list[ConnectorTestScenario]:
     """Get acceptance tests for a given category."""
-    categories = ["connection", "spec"]
+    if categories is None:
+        categories = ["connection", "spec"]

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 451ce82 and ff14901.

⛔ Files ignored due to path filters (1)

• poetry.lock is excluded by !**/*.lock

📒 Files selected for processing (2)

• airbyte_cdk/test/standard_tests/connector_base.py (2 hunks)
• airbyte_cdk/test/standard_tests/source_base.py (4 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• airbyte_cdk/test/standard_tests/source_base.py

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.

Learnt from: pnilan
PR: airbytehq/airbyte-python-cdk#0
File: :0-0
Timestamp: 2024-12-11T16:34:46.319Z
Learning: In the airbytehq/airbyte-python-cdk repository, the `declarative_component_schema.py` file is auto-generated from `declarative_component_schema.yaml` and should be ignored in the recommended reviewing order.

airbyte_cdk/test/standard_tests/connector_base.py (4)

Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#174
File: unit_tests/source_declarative_manifest/resources/source_the_guardian_api/components.py:21-29
Timestamp: 2025-01-13T23:39:15.457Z
Learning: The CustomPageIncrement class in unit_tests/source_declarative_manifest/resources/source_the_guardian_api/components.py is imported from another connector definition and should not be modified in this context.

Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#90
File: Dockerfile:16-21
Timestamp: 2024-12-02T18:36:04.346Z
Learning: Copying files from `site-packages` in the Dockerfile maintains compatibility with both the old file structure that manifest-only connectors expect and the new package-based structure where SDM is part of the CDK.

Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/_run.py:62-65
Timestamp: 2024-11-15T01:04:21.272Z
Learning: The files in `airbyte_cdk/cli/source_declarative_manifest/`, including `_run.py`, are imported from another repository, and changes to these files should be minimized or avoided when possible to maintain consistency.

Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.

🪛 GitHub Actions: Linters

airbyte_cdk/test/standard_tests/connector_base.py

[error] 136-137: ruff formatting check failed. File requires reformatting as indicated by 'poetry run ruff format --diff'.

🔇 Additional comments (3)

airbyte_cdk/test/standard_tests/connector_base.py (3)

11-15: Import changes look good!

The addition of yaml and AirbyteConnectionStatus imports align well with the new YAML config loading functionality and explicit type casting in the test methods.

---

139-142: Clean implementation of connector root directory discovery!

The method properly leverages the existing find_connector_root utility and provides sensible search locations.

---

144-151: Well-implemented class property with proper error handling!

The implementation correctly uses the connector root directory and provides clear error messaging when the config file is missing.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

airbyte_cdk/test/standard_tests/connector_base.py (2)

154-194: Consider adding error handling for YAML parsing and path resolution.

The scenario loading logic is comprehensive, but there are a few areas where additional error handling might be helpful:

1. YAML parsing could fail with malformed files
2. Path resolution for config and catalog paths could fail if the files don't exist

Would you like to add try-catch blocks around the YAML loading and path resolution operations, wdyt?

-        all_tests_config = yaml.safe_load(cls.acceptance_test_config_path.read_text())
+        try:
+            all_tests_config = yaml.safe_load(cls.acceptance_test_config_path.read_text())
+        except yaml.YAMLError as e:
+            raise ValueError(f"Failed to parse YAML config at {cls.acceptance_test_config_path}: {e}") from e

Also, should we validate that the resolved paths actually exist before returning the scenarios, wdyt?

---

179-185: Consider extracting the filtering logic for better maintainability.

The list comprehension with multiple conditions is functional but could be more readable. Would you consider extracting this into a helper method or breaking it down for clarity, wdyt?

-            test_scenarios.extend(
-                [
-                    ConnectorTestScenario.model_validate(test)
-                    for test in all_tests_config["acceptance_tests"][category]["tests"]
-                    if "config_path" in test and "iam_role" not in test["config_path"]
-                ]
-            )
+            for test in all_tests_config["acceptance_tests"][category]["tests"]:
+                if "config_path" not in test:
+                    continue
+                if "iam_role" in test["config_path"]:
+                    # Skip iam_role tests as they are not supported in the test suite
+                    continue
+                test_scenarios.append(ConnectorTestScenario.model_validate(test))

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ff14901 and 2345164.

📒 Files selected for processing (3)

• airbyte_cdk/sources/declarative/manifest_declarative_source.py (4 hunks)
• airbyte_cdk/test/standard_tests/connector_base.py (2 hunks)
• airbyte_cdk/test/standard_tests/source_base.py (4 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• airbyte_cdk/test/standard_tests/source_base.py
• airbyte_cdk/sources/declarative/manifest_declarative_source.py

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.

Learnt from: pnilan
PR: airbytehq/airbyte-python-cdk#0
File: :0-0
Timestamp: 2024-12-11T16:34:46.319Z
Learning: In the airbytehq/airbyte-python-cdk repository, the `declarative_component_schema.py` file is auto-generated from `declarative_component_schema.yaml` and should be ignored in the recommended reviewing order.

Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/spec.json:9-15
Timestamp: 2024-11-15T00:59:08.154Z
Learning: When code in `airbyte_cdk/cli/source_declarative_manifest/` is being imported from another repository, avoid suggesting modifications to it during the import process.

Learnt from: pnilan
PR: airbytehq/airbyte-python-cdk#0
File: :0-0
Timestamp: 2024-12-11T16:34:46.319Z
Learning: In the airbytehq/airbyte-python-cdk repository, ignore all `__init__.py` files when providing a recommended reviewing order.

airbyte_cdk/test/standard_tests/connector_base.py (4)

Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#174
File: unit_tests/source_declarative_manifest/resources/source_the_guardian_api/components.py:21-29
Timestamp: 2025-01-13T23:39:15.457Z
Learning: The CustomPageIncrement class in unit_tests/source_declarative_manifest/resources/source_the_guardian_api/components.py is imported from another connector definition and should not be modified in this context.

Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.

Learnt from: ChristoGrab
PR: airbytehq/airbyte-python-cdk#90
File: Dockerfile:16-21
Timestamp: 2024-12-02T18:36:04.346Z
Learning: Copying files from `site-packages` in the Dockerfile maintains compatibility with both the old file structure that manifest-only connectors expect and the new package-based structure where SDM is part of the CDK.

Learnt from: aaronsteers
PR: airbytehq/airbyte-python-cdk#58
File: airbyte_cdk/cli/source_declarative_manifest/_run.py:62-65
Timestamp: 2024-11-15T01:04:21.272Z
Learning: The files in `airbyte_cdk/cli/source_declarative_manifest/`, including `_run.py`, are imported from another repository, and changes to these files should be minimized or avoided when possible to maintain consistency.

🧬 Code Graph Analysis (1)

airbyte_cdk/test/standard_tests/connector_base.py (5)

airbyte_cdk/test/entrypoint_wrapper.py (2)

• connection_status_messages (167-168)
• errors (200-201)

airbyte_cdk/test/models/scenario.py (2)

• expected_outcome (93-99)
• ConnectorTestScenario (27-179)

airbyte_cdk/test/models/outcome.py (1)

• expect_exception (55-57)

airbyte_cdk/test/standard_tests/docker_base.py (4)

• get_connector_root_dir (53-55)
• get_test_class_dir (46-50)
• acceptance_test_config_path (69-75)
• get_scenarios (78-132)

airbyte_cdk/utils/connector_paths.py (1)

• find_connector_root (180-198)

⏰ 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: Check: destination-motherduck
• GitHub Check: Check: source-shopify
• GitHub Check: SDM Docker Image Build
• GitHub Check: Analyze (python)

🔇 Additional comments (4)

airbyte_cdk/test/standard_tests/connector_base.py (4)

11-15: Import changes look good.

The addition of yaml and AirbyteConnectionStatus imports align with the new functionality, and the cleanup of unused imports (AirbyteMessage, Type) is appropriate.

---

123-139: Great improvements to error handling and validation!

The enhanced error message formatting and explicit connection status validation significantly improve debugging capabilities. I notice the past review comments have been addressed with the null check for result._messages and the proper attribute access for scenario.expected_outcome.expect_exception().

---

140-143: Clean delegation to utility function.

The get_connector_root_dir method properly delegates to find_connector_root with appropriate search paths. This aligns with the existing pattern in DockerConnectorTestSuite.

---

145-152: Solid implementation of the acceptance test config path property.

The classproperty decorator and error handling provide clear feedback when the config file is missing. Good use of the existing ACCEPTANCE_TEST_CONFIG constant.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a69c252 and 7048b30.

📒 Files selected for processing (5)

• airbyte_cdk/entrypoint.py (3 hunks)
• airbyte_cdk/legacy/sources/declarative/manifest_declarative_source.py (3 hunks)
• airbyte_cdk/sources/declarative/concurrent_declarative_source.py (2 hunks)
• unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py (1 hunks)
• unit_tests/test_entrypoint.py (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py

🧰 Additional context used

🧠 Learnings (5)

📓 Common learnings

Learnt from: ChristoGrab
Repo: airbytehq/airbyte-python-cdk PR: 58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.

📚 Learning: 2024-11-18T23:40:06.391Z

Learnt from: ChristoGrab
Repo: airbytehq/airbyte-python-cdk PR: 58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.

Applied to files:

• airbyte_cdk/legacy/sources/declarative/manifest_declarative_source.py
• airbyte_cdk/sources/declarative/concurrent_declarative_source.py

📚 Learning: 2024-11-15T01:04:21.272Z

Learnt from: aaronsteers
Repo: airbytehq/airbyte-python-cdk PR: 58
File: airbyte_cdk/cli/source_declarative_manifest/_run.py:62-65
Timestamp: 2024-11-15T01:04:21.272Z
Learning: The files in `airbyte_cdk/cli/source_declarative_manifest/`, including `_run.py`, are imported from another repository, and changes to these files should be minimized or avoided when possible to maintain consistency.

Applied to files:

• airbyte_cdk/legacy/sources/declarative/manifest_declarative_source.py
• airbyte_cdk/entrypoint.py

📚 Learning: 2024-11-15T00:59:08.154Z

Learnt from: aaronsteers
Repo: airbytehq/airbyte-python-cdk PR: 58
File: airbyte_cdk/cli/source_declarative_manifest/spec.json:9-15
Timestamp: 2024-11-15T00:59:08.154Z
Learning: When code in `airbyte_cdk/cli/source_declarative_manifest/` is being imported from another repository, avoid suggesting modifications to it during the import process.

Applied to files:

• airbyte_cdk/legacy/sources/declarative/manifest_declarative_source.py
• airbyte_cdk/entrypoint.py

📚 Learning: 2025-01-14T00:20:32.310Z

Learnt from: aaronsteers
Repo: airbytehq/airbyte-python-cdk PR: 174
File: airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py:1093-1102
Timestamp: 2025-01-14T00:20:32.310Z
Learning: In the `airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py` file, the strict module name checks in `_get_class_from_fully_qualified_class_name` (requiring `module_name` to be "components" and `module_name_full` to be "source_declarative_manifest.components") are intentionally designed to provide early, clear feedback when class declarations won't be found later in execution. These restrictions may be loosened in the future if the requirements for class definition locations change.

Applied to files:

• airbyte_cdk/legacy/sources/declarative/manifest_declarative_source.py

🧬 Code graph analysis (4)

airbyte_cdk/legacy/sources/declarative/manifest_declarative_source.py (1)

airbyte_cdk/sources/declarative/concurrent_declarative_source.py (3)

• _uses_dynamic_schema_loader (657-688)
• _stream_configs (519-538)
• dynamic_streams (513-517)

airbyte_cdk/sources/declarative/concurrent_declarative_source.py (1)

airbyte_cdk/legacy/sources/declarative/manifest_declarative_source.py (2)

• _uses_dynamic_schema_loader (622-654)
• _stream_configs (498-519)

airbyte_cdk/entrypoint.py (3)

airbyte_cdk/sources/abstract_source.py (2)

• _emit_queued_messages (291-294)
• discover (85-90)

airbyte_cdk/test/entrypoint_wrapper.py (1)

• discover (397-419)

airbyte_cdk/connector.py (3)

• read_config (51-58)
• configure (45-48)
• configure (122-127)

unit_tests/test_entrypoint.py (3)

unit_tests/sources/test_abstract_source.py (7)

• MockSource (62-99)
• message_repository (98-99)
• message_repository (150-155)
• streams (84-87)
• name (189-190)
• name (255-256)
• name (282-283)

airbyte_cdk/sources/message/repository.py (5)

• consume_queue (59-60)
• consume_queue (70-71)
• consume_queue (93-95)
• consume_queue (114-115)
• consume_queue (138-139)

airbyte_cdk/entrypoint.py (3)

• AirbyteEntrypoint (53-390)
• run (156-239)
• discover (282-291)

🪛 GitHub Actions: Linters

unit_tests/test_entrypoint.py

[error] 856-856: Ruff formatting: 2 files would be reformatted. Run 'poetry run ruff format' to fix formatting.

⏰ 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). (14)

• GitHub Check: Check: destination-motherduck
• GitHub Check: Check: source-shopify
• GitHub Check: Check: source-pokeapi
• GitHub Check: Check: source-intercom
• GitHub Check: Check: source-hardcoded-records
• GitHub Check: Manifest Server Docker Image Build
• GitHub Check: SDM Docker Image Build
• GitHub Check: Pytest (All, Python 3.11, Ubuntu)
• GitHub Check: Pytest (Fast)
• GitHub Check: Pytest (All, Python 3.10, Ubuntu)
• GitHub Check: Pytest (All, Python 3.12, Ubuntu)
• GitHub Check: Pytest (All, Python 3.13, Ubuntu)
• GitHub Check: Analyze (python)
• GitHub Check: Analyze (python)

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py (1)

9-9: Remove unused pytest import.

The pytest import isn't used anywhere in this test file, wdyt about removing it?

-import pytest
-

🧹 Nitpick comments (2)

unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py (2)

77-129: Consider testing actual behavior without mocking streams().

A few observations about this test:

1. Lines 120-121 duplicate the flag assertions from test_check_config_during_discover_with_dynamic_schema_loader – are these redundant?

2. By mocking streams(), the test bypasses the actual validation logic that would normally run in ManifestDeclarativeSource.streams(). According to the code in manifest_declarative_source.py, the streams() method validates config via self._spec_component.validate_config(config). This test doesn't verify whether that validation is correctly triggered or skipped based on the check_config_during_discover flag.

3. The test verifies that discover() can be called with an empty config when streams() is mocked, but doesn't confirm the real end-to-end flow works as intended.

Would it be more valuable to test the actual discover flow without mocking, perhaps in an integration test? Or is the mock intentional here to isolate the discover method from streams behavior, wdyt?

---

131-175: Misleading docstring and redundant assertions.

A few issues here:

1. The docstring (line 135) says "Test that discovery validates config when DynamicSchemaLoader is not used," but since streams() is mocked, no actual config validation occurs. The test just verifies that discover() returns a catalog when called with an empty config and mocked streams. Maybe update the docstring to reflect what's actually being tested?

2. Lines 164-165 and 174-175 repeat the flag assertions already covered in test_check_config_during_discover_without_dynamic_schema_loader – these seem redundant, wdyt?

3. Similar to my comment on the previous test, mocking streams() prevents testing the actual validation behavior. Is this intentional?

4. This test has significant overlap with test_discover_with_dynamic_schema_loader_no_config – could these be parameterized to reduce duplication (e.g., @pytest.mark.parametrize with schema_loader type and expected flag value)?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7048b30 and 711635f.

📒 Files selected for processing (1)

• unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py (1 hunks)

🧰 Additional context used

🧠 Learnings (5)

📓 Common learnings

Learnt from: ChristoGrab
Repo: airbytehq/airbyte-python-cdk PR: 58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.

📚 Learning: 2024-11-18T23:40:06.391Z

Learnt from: ChristoGrab
Repo: airbytehq/airbyte-python-cdk PR: 58
File: airbyte_cdk/sources/declarative/yaml_declarative_source.py:0-0
Timestamp: 2024-11-18T23:40:06.391Z
Learning: When modifying the `YamlDeclarativeSource` class in `airbyte_cdk/sources/declarative/yaml_declarative_source.py`, avoid introducing breaking changes like altering method signatures within the scope of unrelated PRs. Such changes should be addressed separately to minimize impact on existing implementations.

Applied to files:

• unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py

📚 Learning: 2024-11-15T01:04:21.272Z

Learnt from: aaronsteers
Repo: airbytehq/airbyte-python-cdk PR: 58
File: airbyte_cdk/cli/source_declarative_manifest/_run.py:62-65
Timestamp: 2024-11-15T01:04:21.272Z
Learning: The files in `airbyte_cdk/cli/source_declarative_manifest/`, including `_run.py`, are imported from another repository, and changes to these files should be minimized or avoided when possible to maintain consistency.

Applied to files:

• unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py

📚 Learning: 2024-11-10T04:50:11.914Z

Learnt from: aaronsteers
Repo: airbytehq/airbyte-python-cdk PR: 13
File: airbyte_cdk/connector.py:99-99
Timestamp: 2024-11-10T04:50:11.914Z
Learning: When a PR's goal is to run the autoformat task from `ruff`, avoid suggesting code changes beyond formatting to prevent potential negative side effects.

Applied to files:

• unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py

📚 Learning: 2024-11-15T00:59:08.154Z

Learnt from: aaronsteers
Repo: airbytehq/airbyte-python-cdk PR: 58
File: airbyte_cdk/cli/source_declarative_manifest/spec.json:9-15
Timestamp: 2024-11-15T00:59:08.154Z
Learning: When code in `airbyte_cdk/cli/source_declarative_manifest/` is being imported from another repository, avoid suggesting modifications to it during the import process.

Applied to files:

• unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py

🧬 Code graph analysis (1)

unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py (4)

airbyte_cdk/legacy/sources/declarative/manifest_declarative_source.py (2)

• ManifestDeclarativeSource (98-654)
• streams (305-342)

airbyte_cdk/entrypoint.py (1)

• discover (282-291)

airbyte_cdk/sources/declarative/concurrent_declarative_source.py (2)

• discover (385-388)
• streams (391-423)

airbyte_cdk/sources/abstract_source.py (2)

• discover (85-90)
• streams (74-79)

⏰ 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). (14)

• GitHub Check: Check: source-pokeapi
• GitHub Check: Check: destination-motherduck
• GitHub Check: Check: source-intercom
• GitHub Check: Check: source-hardcoded-records
• GitHub Check: Check: source-shopify
• GitHub Check: Analyze (python)
• GitHub Check: Analyze (python)
• GitHub Check: Pytest (All, Python 3.13, Ubuntu)
• GitHub Check: Pytest (All, Python 3.11, Ubuntu)
• GitHub Check: Pytest (All, Python 3.12, Ubuntu)
• GitHub Check: Pytest (All, Python 3.10, Ubuntu)
• GitHub Check: Manifest Server Docker Image Build
• GitHub Check: SDM Docker Image Build
• GitHub Check: Pytest (Fast)

🔇 Additional comments (2)

unit_tests/sources/declarative/test_manifest_declarative_source_dynamic_schema.py (2)

17-50: LGTM! Clean flag verification test.

This test clearly verifies that check_config_during_discover is correctly set to True when using DynamicSchemaLoader.

---

52-75: LGTM! Good coverage of the static schema case.

This test nicely complements the previous test by verifying that check_config_during_discover is False when using InlineSchemaLoader.

[devin-ai-integration]

Devin Review found 3 potential issues.

4 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)

feedback addressed