Skip to content

fix(low-code): build only the checked static streams during check - #1247

Merged
Patrick Nilan (pnilan) merged 2 commits into
mainfrom
devin/1792687587-check-build-named-streams
Oct 23, 2026
Merged

Patrick Nilan (pnilan) merged 2 commits into
mainfrom
devin/1792687587-check-build-named-streams

Conversation

@devin-ai-integration

@devin-ai-integration devin-ai-integration Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Overview

👉 TL;DR: Connection checks now set up only the streams they actually test, instead of every stream in the connector. A stream the check doesn't read can no longer make the check fail, while discover and sync still set up every stream and still fail on the same problems.

Specifically, CheckStream builds only its named static streams via ConcurrentDeclarativeSource._streams_by_name, and CheckDynamicStream builds only its first stream_count streams via _first_streams. Both fall back to streams() when a subclass overrides it (_uses_default_streams()), or when a name is empty, unknown, or dynamic.

This PR is part of a stacked set:

Summary

Speeds up check for manifest-only sources: CheckStream now builds only the static streams it reads, instead of every stream in the manifest. This is PR 1 of the two stacked PRs planned in the issue (decisions D1, D5–D9). Dynamic streams are still resolved and built here; skipping them (D2) is PR 2.

Part of https://github.com/airbytehq/airbyte-internal-issues/issues/17934 (PR 1 of 2). This PR does not close the issue; #1248 (D2) is still needed.

Changes

  • ConcurrentDeclarativeSource (concurrent_declarative_source.py)
    • streams() now runs through _prepare_for_stream_build() (spec config_validations + API budget) and _create_streams(selected, all). Its result is unchanged.
    • New _streams_by_name(stream_names) -> Optional[List[AbstractStream]]: evaluates ConditionalStreams, builds only the named static configs plus every dynamic stream. Returns None when stream_names is empty or a name is not a static stream, so callers fall back to streams() (D7).
    • New _first_streams(count): builds the first count streams (count >= 1), static then dynamic, the same order as streams() (D9).
    • Both return None when a subclass overrides streams() (_uses_default_streams()), so connectors such as source-google-ads keep going through their override during check. Static names are worked out before setup, so the fallback runs setup only once.
    • _apply_stream_groups also walks the inlined copy of a top-level parent that was not built, so a conflict with a grandparent still fails check.
    • New read-only _parent_stream_names(). _initialize_cache_for_parent_streams takes an optional all_stream_configs, so a built stream that is the parent of an unbuilt stream still gets use_cache as today, and only the selected configs are deep-copied (D8).
  • CheckStream (check_stream.py)
    • Calls _streams_by_name when isinstance(source, ConcurrentDeclarativeSource) and there are no dynamic_streams_check_configs. Otherwise it calls streams() as today (legacy ManifestDeclarativeSource, MagicMock sources, dynamic or unknown names, __airbyte_check_stream_names naming a non-static stream).
    • should_check_dynamic_streams tests dynamic_streams_check_configs before hasattr(source, "dynamic_streams"), so a passing check no longer resolves dynamic streams a second time (D5).
  • CheckDynamicStream (check_dynamic_stream.py): uses _first_streams(stream_count) when use_check_availability is true and stream_count >= 1. Otherwise it keeps today's path.

Behaviour change (D1, decided)

An unread static stream that cannot be built no longer fails check. discover and read still build every stream and still raise that error. Config values used only by unread streams are no longer validated by check either. CheckDynamicStream no longer builds streams past stream_count.

Known affected connectors:

  • source-alpha-vantage, source-bigcommerce (archived), and source-mntn / source-strava when the config has a badly formatted date (from the issue);
  • source-paypal-transaction, source-plaid: an invalid start_date;
  • source-amazon-seller-partner, source-airtable: CheckDynamicStream no longer builds streams past stream_count.

Latent test break in source-linkedin-ads: its unit_tests/test_source.py patches _initialize_cache_for_parent_streams with a one-argument function. Every build path now passes all_stream_configs=, so 6 of its tests will fail with TypeError once its test lock moves past CDK 7.4.1. The mock needs to accept **kwargs then. No CDK change is needed.

Releaser: paste into the release draft

check builds only the streams it reads (#1247). check exists to verify that the credentials can reach the API, so it now builds only the streams it reads. CheckStream builds the static streams it names, and CheckDynamicStream builds its first stream_count streams. Dynamic streams are still resolved and built during check until #1248.

A problem in a stream that check doesn't read no longer fails check; discover and read still build every stream and still fail. This covers streams that can't be built, such as a broken custom component, and config values that only those streams use, such as an invalid start_date in source-paypal-transaction or source-plaid. To reject a bad config value during check, declare the rule in the spec: a pattern in connection_specification, or an entry in config_normalization_rules.validations. Both run before any stream is built. Connector Builder checks behave the same way.

Test Coverage

New tests in unit_tests/sources/declarative/checks/test_check_stream.py, test_check_dynamic_stream.py and test_concurrent_declarative_source.py cover:

  • only the named stream is built; a checked substream builds its parent inside its own definition;
  • ConditionalStreams true / false, StateDelegatingStream, __airbyte_check_stream_names, a misspelled name (same is not part of the catalog error), a dynamic stream name (fallback);
  • use_cache on a checked stream that is a parent of an unchecked stream, and no mutation of all_stream_configs;
  • D1 (an unread broken stream passes, a checked broken stream fails with today's message), D5 (dynamic_streams evaluated once);
  • a failing spec config_validations (config_overrides is covered by the existing test_concurrent_declarative_source_config_overrides.py);
  • CheckDynamicStream with stream_count below the stream count, stream_count: 0, use_check_availability: false;
  • a legacy ManifestDeclarativeSource and a MagicMock source still get streams().

Four of the new tests (only-named build, D1, D5, first_streams count) were run against unfixed main and fail there. unit_tests/sources/declarative, unit_tests/connector_builder and the entrypoint tests pass locally (2543 passed, 1 skipped), as do ruff and mypy on the changed files.

Declarative-First Evaluation

N/A — this is a CDK framework change, not a change to a connector manifest.

Not in this PR

  • Skipping dynamic stream resolution and building when every checked name is static (D2) — PR 2, stacked on this one.
  • The fleet benchmark row for the release that contains this PR (acceptance criterion in the issue).

Link to Devin session: https://app.devin.ai/sessions/328d83d551ec44219895b69f3d2f9bd3
Open in Devin Desktop: https://app.devin.ai/desktop/session/328d83d551ec44219895b69f3d2f9bd3?variant=devin

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

@github-actions

Copy link
Copy Markdown

👋 Greetings, Airbyte Team Member!

Here are some helpful tips and reminders for your convenience.

💡 Show Tips and Tricks

Testing This CDK Version

You can test this version of the CDK using the following:

# Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@devin/1792687587-check-build-named-streams#egg=airbyte-python-cdk[dev]' --help

# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch devin/1792687587-check-build-named-streams

PR Slash Commands

Airbyte Maintainers can execute the following slash commands on your PR:

  • /autofix - Fixes most formatting and linting issues
  • /poetry-lock - Updates poetry.lock file
  • /test - Runs connector tests with the updated CDK
  • /prerelease - Triggers a prerelease publish with default arguments
  • /poe build - Regenerate git-committed build artifacts, such as the pydantic models which are generated from the manifest JSON schema in YAML.
  • /poe <command> - Runs any poe command in the CDK environment
📚 Show Repo Guidance

Helpful Resources

📝 Edit this welcome message.

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

PyTest Results (Fast)

5 898 tests  +34   5 867 ✅ +34   5m 12s ⏱️ -39s
    1 suites ± 0      31 💤 ± 0 
    1 files   ± 0       0 ❌ ± 0 

Results for commit c6002a1. ± Comparison against base commit 41af85a.

This pull request removes 1 and adds 35 tests. Note that renamed tests count towards both.
unit_tests.sources.declarative.test_concurrent_declarative_source ‑ test_api_budget_is_set_before_dynamic_streams_evaluated
unit_tests.sources.declarative.checks.test_check_dynamic_stream ‑ test_check_dynamic_stream_builds_only_the_first_stream_count_in_order[one-stream]
unit_tests.sources.declarative.checks.test_check_dynamic_stream ‑ test_check_dynamic_stream_builds_only_the_first_stream_count_in_order[three-streams]
unit_tests.sources.declarative.checks.test_check_dynamic_stream ‑ test_check_dynamic_stream_uses_streams_for_legacy_manifest_source
unit_tests.sources.declarative.checks.test_check_dynamic_stream ‑ test_check_dynamic_stream_uses_streams_for_non_positive_count_or_disabled_availability[0-True]
unit_tests.sources.declarative.checks.test_check_dynamic_stream ‑ test_check_dynamic_stream_uses_streams_for_non_positive_count_or_disabled_availability[1-False]
unit_tests.sources.declarative.checks.test_check_dynamic_stream ‑ test_check_dynamic_stream_uses_streams_override_for_concurrent_source
unit_tests.sources.declarative.checks.test_check_stream ‑ test_check_builds_a_named_state_delegating_stream
unit_tests.sources.declarative.checks.test_check_stream ‑ test_check_builds_only_the_named_static_stream
unit_tests.sources.declarative.checks.test_check_stream ‑ test_check_detects_group_conflict_with_a_checked_streams_grandparent
unit_tests.sources.declarative.checks.test_check_stream ‑ test_check_does_not_build_an_unread_stream_that_fails_to_build
…

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

PyTest Results (Full)

5 901 tests   5 870 ✅  10m 40s ⏱️
    1 suites     31 💤
    1 files        0 ❌

Results for commit c6002a1.

♻️ This comment has been updated with latest results.

@pnilan

Copy link
Copy Markdown
Member

Does this PR handle if multiple static streams are defined? Do we build each?

@pnilan Patrick Nilan (pnilan) changed the title perf(low-code): build only the checked static streams during check fix(low-code check): build only the checked static streams during check Oct 22, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Yes, as long as the checker names every one of them. CheckStream passes its full stream_names list (or __airbyte_check_stream_names) to streams_by_name, which builds every static stream in that list in manifest order. check_connection then reads each one in turn, the same as before.

Two cases fall back to streams(), which builds everything:

  • an empty list;
  • any name that isn't a static stream, such as a misspelled or dynamic name.

Static streams the checker doesn't name are not built. That is D1 in the issue.

Our existing tests only name a single stream. If you'd like a test where the checker names two static streams out of three and asserts that both are built and read, I can add it.

Written by Devin

@pnilan

Copy link
Copy Markdown
Member

What happens if a name is incorrectly spelled when there is one static check stream? What if there are multiple and one is misspelled? How does this PR handle those two cases? What about if there are no defined check streams?

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

All three behave the same as on main. If any name is wrong, streams_by_name returns None and CheckStream._build_streams falls back to source.streams(), which is the old full build.

1. One check stream, misspelled. The full build runs, and then check_connection raises the same error as before:
frist is not part of the catalog. Expected one of ['first', 'second'].
test_check_with_misspelled_stream_name_uses_catalog_error covers this case. I also ran it ad hoc and saw streams() called once with that error.

2. Several check streams, one misspelled. The full build runs, because any unknown name makes streams_by_name return None. Then check_connection goes through the names in order, as it did before. Every correct name before the misspelled one is read. When the loop reaches the misspelled name, it raises ... is not part of the catalog. If one of the earlier streams fails its availability check first, check returns that failure and stops before it gets to the misspelled name.

I ran ["first", "secnd"] with the HTTP for first unmocked. Check returned the connection failure for first and never got to secnd. That matches the old ordering. We could check every name up front and raise the catalog error before any read, but that would change behaviour, so I left it out of this PR.

3. No check streams defined. There are three variants:

  • CheckStream with neither stream_names nor dynamic_streams_check_configs is rejected by the factory: Expected either stream_names or dynamic_streams_check_configs to be set for CheckStream. This is unchanged.
  • An empty list, either stream_names: [] or an empty __airbyte_check_stream_names that falls back to it, makes streams_by_name return None. Check then does the full build, as before. With no names to read, it returns success, or runs the dynamic checks if any are set. This is also unchanged.
  • An empty __airbyte_check_stream_names: [] with a non-empty manifest stream_names uses the manifest's names, as before (_get_stream_names). Those go through streams_by_name normally.

So in every case where a name is unknown or the list is empty, this PR changes nothing. Selective building only applies when every name is a known static stream.

Written by Devin

@pnilan Patrick Nilan (pnilan) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 This review was generated by an AI Agent.

Review: one blocker, then tests, release notes and cleanups

The refactor keeps discover and read identical, which an offline comparison confirmed:

  • Every manifest-only connector was built on v7.39.0 and on this head. The inputs to stream building matched for 516 of 516.
  • use_cache matched for all 5,513 static stream configs.

The fallbacks keep today's error messages, and D5 is a clean fix. One change blocks merge (#1). The rest are tests, release notes and cleanups.

Please address this review here first. Then rebase #1248 and apply the carry-over list in the comment on that PR.

# Priority Finding Where
1 P0 Check skips a subclass's streams() override checks/check_stream.py:119, checks/check_dynamic_stream.py:45
2 P1 The CheckDynamicStream fallback test never reaches the guards it tests test_check_dynamic_stream.py:249
3 P1 No tests for setup (validation, API budget) on the new paths, or for the dynamic_streams_check_configs fallback concurrent_declarative_source.py:486, :502; check_stream.py:121
4 P1 The D1 release note is incomplete and won't reach the release PR body
5 P2 Stream-group validation misses a checked stream's grandparent concurrent_declarative_source.py:499
6 P2 New public methods: wrong or missing docstrings, and #1248 changes the contract concurrent_declarative_source.py:484, :501
7 P2 The new all_stream_configs= keyword breaks 6 source-linkedin-ads unit tests (latent) concurrent_declarative_source.py:463
8 P2 Several new-path invariants have no test concurrent_declarative_source.py:463, :489, :498; check_dynamic_stream.py:48
9 P3 The fallback runs setup twice concurrent_declarative_source.py:486
10 P3 The spec-validation test isn't guarded against network access test_check_stream.py:1160
11 P3 The title's scope has a space; the commit uses perf PR title
12 P3 "Resolves" would auto-close #17934; the config_overrides test claim is wrong PR body

1. P0: check skips a subclass's streams() override

_build_streams sends every ConcurrentDeclarativeSource instance, subclasses included, to streams_by_name, which builds straight from the manifest. CheckDynamicStream does the same through first_streams. A Python connector that overrides streams() no longer runs that override during check.

Affected: source-google-ads.

  • It is certified and depends on airbyte-cdk ^7.33.0. An automated PR bumps its lock every week and merges itself.
  • SourceGoogleAds.streams() drops end_date: "" (its spec allows "") and derives customer_ids from customer_id (source.py:27-40).
  • A repro of that shape returns SUCCEEDED on v7.39.0. On this head it returns FAILED: … time data '' does not match format '%Y-%m-%d'.
  • Six more monorepo connectors override streams(), but they pin older CDK majors.

Fix (decided): take the fast path only when streams() is the CDK's own.

  • Don't use an exact-class check. Manifest-only connectors run as SourceLocalYaml, which is a subclass, so type(source) is ConcurrentDeclarativeSource would turn the speed-up off for all of them.
  • Put the guard inside the new methods, so that #1248 and any later caller inherit it:
# concurrent_declarative_source.py
def _uses_default_streams(self) -> bool:
    """A subclass that overrides streams() must keep going through it during check."""
    return type(self).streams is ConcurrentDeclarativeSource.streams

def streams_by_name(self, stream_names: List[str]) -> Optional[List[AbstractStream]]:
    if not self._uses_default_streams():
        return None
    ...

def first_streams(self, count: int) -> Optional[List[AbstractStream]]:
    if not self._uses_default_streams():
        return None
    ...
# checks/check_dynamic_stream.py: fall back to streams() when first_streams returns None
streams: Optional[List[Union[Stream, AbstractStream]]] = None
if (
    isinstance(source, ConcurrentDeclarativeSource)
    and self.use_check_availability
    and self.stream_count >= 1
):
    streams = cast(
        Optional[List[Union[Stream, AbstractStream]]], source.first_streams(self.stream_count)
    )
if streams is None:
    streams = source.streams(config=config)  # type: ignore  # keep the existing comment

Regression tests: add one each for CheckStream and CheckDynamicStream.

  • Arrange: a subclass that overrides streams() the way google-ads does: pop end_date: "", then call super().streams(config=config). Give the checked stream an end_datetime of {{ config.get('end_date', today_utc()) }}, and use config {"end_date": ""}.
  • Assert: check returns SUCCEEDED, and the override ran exactly once.

2. P1: the CheckDynamicStream fallback test is vacuous

test_check_dynamic_stream_uses_streams_for_non_positive_count_or_disabled_availability uses MagicMock(). That mock is not a ConcurrentDeclarativeSource, so the test never reaches the use_check_availability or stream_count >= 1 checks. Deleting both guards (check_dynamic_stream.py:47-48) leaves all 9 tests in the file green.

Without those guards:

  • stream_count: 0 would fail with "No streams to connect to".
  • use_check_availability: false would stop building every stream.

Fix: use MagicMock(spec=ConcurrentDeclarativeSource). Then assert source.first_streams.assert_not_called() and source.streams.assert_called_once_with(config={}).

3. P1: test the setup on the new paths

Each of these mutations passes the whole declarative and Connector Builder suites (2,489 tests):

  • removing self._prepare_for_stream_build() from first_streams;
  • keeping spec validation but dropping set_api_budget in streams_by_name;
  • removing and not self.dynamic_streams_check_configs from check_stream.py:121.

The regression test for the API-budget ordering bug (oncall #11954, fixed in #986) is test_api_budget_is_set_before_dynamic_streams_evaluated. It only calls streams().

Tests to add:

  • Parametrize that ordering test over streams(), streams_by_name([<static name>]) and first_streams(1).
  • Add a CheckDynamicStream version of test_check_still_fails_when_spec_config_validations_fail.
  • Add a CheckStream test with dynamic_streams_check_configs and an unread static stream that can't be built. Assert that check is still FAILED with "Encountered an error while discovering streams".

4. P1: release note

The repo squash-merges with the PR title and an empty commit message. The release drafter builds notes from commit titles only. So the note in the body reaches the release only if the releaser pastes it in by hand. The current note also covers only unread static streams under CheckStream.

Fix: add a "Releaser: paste into the release draft" section to the PR body with this text:

check builds only the streams it reads (#1247). check exists to verify that the credentials can reach the API, so it now builds only the streams it reads. CheckStream builds the static streams it names, and CheckDynamicStream builds its first stream_count streams. Dynamic streams are still resolved and built during check until #1248.

A problem in a stream that check doesn't read no longer fails check; discover and read still build every stream and still fail. This covers streams that can't be built, such as a broken custom component, and config values that only those streams use, such as an invalid start_date in source-paypal-transaction or source-plaid. To reject a bad config value during check, declare the rule in the spec: a pattern in connection_specification, or an entry in config_normalization_rules.validations. Both run before any stream is built. Connector Builder checks behave the same way.

Also add these to the body's list of known affected connectors:

  • source-paypal-transaction and source-plaid, because of an invalid start_date;
  • source-amazon-seller-partner and source-airtable, because CheckDynamicStream no longer builds streams past stream_count.

5. P2: stream-group validation (decided: fix it)

_create_streams passes only the built streams to _apply_stream_groups, which looks ancestors up by name among them.

  • In a chain A → B → C where check builds only C, the walk stops at B. A conflict between C and its grandparent A is missed, and check passes.
  • v7.39.0 fails the same check with "Stream 'C' and its parent stream 'A' are both in group 'g1'".
  • A conflict with a direct parent is still caught.

Fix: when a parent is a top-level stream that this call didn't build, walk the copy of it inlined in the child. Limit this to top-level streams. Falling back for every missing name also walks parents that exist only inline, and that makes streams() (so discover and read) raise for a manifest that passes today.

Patch, tested on a copy of this head (195 related tests and the 5 factory stream-group tests pass)
--- a/airbyte_cdk/sources/declarative/concurrent_declarative_source.py
+++ b/airbyte_cdk/sources/declarative/concurrent_declarative_source.py
@@ -477,7 +477,9 @@
             for stream_config in prepared_configs
         ]
 
-        self._apply_stream_groups(source_streams)
+        self._apply_stream_groups(
+            source_streams, {stream_config.get("name") for stream_config in all_configs}
+        )
 
         return source_streams
 
@@ -503,7 +505,11 @@
         stream_configs = self._stream_configs(self._source_config) + self.dynamic_streams
         return self._create_streams(stream_configs[:count], stream_configs)
 
-    def _apply_stream_groups(self, streams: List[AbstractStream]) -> None:
+    def _apply_stream_groups(
+        self,
+        streams: List[AbstractStream],
+        top_level_stream_names: Optional[Set[str]] = None,
+    ) -> None:
         """Set block_simultaneous_read on streams based on the manifest's stream_groups config.
 
         Iterates over the resolved manifest's stream_groups and matches group membership
@@ -526,10 +532,15 @@
         # Validate no stream shares a group with any of its ancestor streams
         stream_name_to_instance: Dict[str, AbstractStream] = {s.name: s for s in streams}
 
-        def _collect_all_ancestor_names(stream_name: str) -> Set[str]:
+        def _collect_all_ancestor_names(
+            stream_name: str, inline_instance: Optional[AbstractStream] = None
+        ) -> Set[str]:
             """Recursively collect all ancestor stream names."""
             ancestors: Set[str] = set()
             inst = stream_name_to_instance.get(stream_name)
+            if inst is None and stream_name in (top_level_stream_names or set()):
+                # A top-level stream that this call did not build: walk the copy inlined in the child
+                inst = inline_instance
             if not isinstance(inst, DefaultStream):
                 return ancestors
             partition_router = inst.get_partition_router()
@@ -546,7 +557,9 @@
                     for parent_config in router.parent_stream_configs:
                         parent_name = parent_config.stream.name
                         ancestors.add(parent_name)
-                        ancestors.update(_collect_all_ancestor_names(parent_name))
+                        ancestors.update(
+                            _collect_all_ancestor_names(parent_name, parent_config.stream)
+                        )
             return ancestors
 
         for stream in streams:

Tests to add. All three use incremental substreams, because DefaultStream.get_partition_router() only returns a router for a per-partition cursor:

  • A → B → C, all top-level, group [A, C], check names C: check is FAILED with today's message.
  • The same group, but B exists only inline in C's router: streams() returns ['A', 'C'] (unchanged), and streams_by_name(["C"]) returns ['C'].
  • Group [A, C], check names A: SUCCEEDED, because C is not read (D1).

6. P2: new public methods

Prefer renaming them _streams_by_name and _first_streams:

  • only the two checkers call them;
  • they would appear in the pdoc API reference;
  • #1248 changes what streams_by_name returns.

If they stay public:

  • Fix the streams_by_name docstring. In this PR it also returns every dynamic stream.
  • Document first_streams, including the None return from #1 and the valid range of count. A negative count silently drops the last streams.

7. P2: source-linkedin-ads unit tests

Certified source-linkedin-ads patches _initialize_cache_for_parent_streams with a one-argument function (unit_tests/test_source.py:77, :111, :334). Every build path now passes all_stream_configs=, so 6 of its 193 tests fail on this head with TypeError: … unexpected keyword argument 'all_stream_configs'.

The break is latent: its unit_tests/poetry.lock pins CDK 7.4.1. No CDK change is needed. Mention in the PR body that the connector's mock must accept **kwargs once its test lock moves.

8. P2: tests for the remaining invariants

These mutations also pass every test:

  • D8 wiring (:463, :499, :504): passing all_stream_configs=None, passing only the selected configs as all_configs, or dropping the deepcopy.
  • D7 (:489): dropping not stream_names or, or changing any to all.
  • Dropping + dynamic_configs (:498), which would ship D2 inside this PR.
  • Changing stream_count >= 1 to > 1. The build-count test uses 3, while both fleet users of CheckDynamicStream use 1.

Tests to add:

  • An end-to-end streams_by_name test that asserts use_cache on a parent stream and an unchanged resolved manifest.
  • D7 parametrized over [] and ["<static>", "<unknown>"].
  • A dynamic stream that can't be built still fails check. #1248 flips this one.
  • A stream_count: 1 case.

9–12. P3 cleanups

  • 9. In streams_by_name, work out the static names before _prepare_for_stream_build(). Today the fallback runs validation and set_api_budget twice. That logs validation messages twice and leaves an orphaned HTTPAPIBudget, whose MovingWindowCallRatePolicy threads never stop. If _stream_configs raises in that early step, return None, so that streams() reports the error exactly as today.
  • 10. Wrap test_check_still_fails_when_spec_config_validations_fail in with HttpMocker():, so a regression fails fast instead of reaching api.test.com.
  • 11. Retitle to fix(low-code): build only the checked static streams during check and squash-merge. None of the last 28 scoped titles has a space, and the branch commit's perf type is not allowed.
  • 12. Change "Resolves …17934" to "Part of #17934", so that merging doesn't close the issue before #1248 lands. Also correct the claim that new tests cover config_overrides; the existing test_concurrent_declarative_source_config_overrides.py covers it.

After this review

  • Rebase #1248 onto this branch and apply the carry-over comment there.
  • Before release, maintainers will run /prerelease and a live check on source-google-ads (with end_date: "") and on source-amazon-seller-partner or source-airtable. The PR author doesn't need to do anything for this step.

@devin-ai-integration devin-ai-integration Bot changed the title fix(low-code check): build only the checked static streams during check fix(low-code): build only the checked static streams during check Oct 23, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Addressed the review in 1101804:

  • P0: _uses_default_streams() (type(self).streams is ConcurrentDeclarativeSource.streams) is checked first in both _streams_by_name and _first_streams; they return None for an override and both checkers fall back to streams(). SourceLocalYaml keeps the fast path. New source-google-ads-style regression tests (override strips end_date: "") for CheckStream and CheckDynamicStream fail on the previous head and pass now.
  • P1: the non-positive/disabled fallback test now uses MagicMock(spec=ConcurrentDeclarativeSource) and asserts _first_streams isn't called. Added API-budget/spec-setup tests across all three build paths, a CheckDynamicStream spec-validation test, and the dynamic_streams_check_configs + unbuildable-static-stream test. Release note, Part of #17934, connector list and config_overrides claim fixed in the PR body.
  • P2: stream-group validation walks the inlined copy of an unbuilt top-level parent (your patch), with the three A→B→C tests. Helpers are private with docstrings. Added the cache/manifest invariant, D7, dynamic-build-failure and stream_count=1 tests. source-linkedin-ads mock note added to the body.
  • P3: static names are resolved before _prepare_for_stream_build() (fallback runs setup once, tested); the spec-validation test runs under HttpMocker; title is now fix(low-code): ....

All requested mutations were caught by the new tests. Next: rebase #1248 onto this and apply its carry-over list.

Written by Devin

@pnilan
Patrick Nilan (pnilan) marked this pull request as ready for review October 23, 2026 00:36
@pnilan
Patrick Nilan (pnilan) requested a review from a team as a code owner October 23, 2026 00:36
@pnilan
Patrick Nilan (pnilan) added this pull request to stack #1249 October 23, 2026 00:37
devin-ai-integration Bot and others added 2 commits October 22, 2026 17:37
Co-Authored-By: bot_apk <apk@cognition.ai>
Co-Authored-By: bot_apk <apk@cognition.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant