#230: Airflow packaging + signalforge.airflow skeleton - #239
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughIntroduces the Changessignalforge.airflow Skeleton and Integration
Sequence Diagram(s)sequenceDiagram
participant Consumer
participant "signalforge.airflow.__init__"
participant "_airflow_compat"
participant airflow
Consumer->>signalforge.airflow.__init__: import AirflowConfigError
Note over signalforge.airflow.__init__: Eagerly available — no airflow import triggered
Consumer->>signalforge.airflow.__init__: access SignalForgeGenerateOperator
signalforge.airflow.__init__->>operators: importlib.import_module(".operators")
operators-->>signalforge.airflow.__init__: SignalForgeGenerateOperator (raises NotImplementedError on init)
Consumer->>_airflow_compat: make_base_operator()
_airflow_compat->>airflow: from airflow.models import BaseOperator
airflow-->>_airflow_compat: BaseOperator class
_airflow_compat-->>Consumer: returns BaseOperator
Consumer->>signalforge.airflow.__init__: access unknown_attr
signalforge.airflow.__init__-->>Consumer: raises AttributeError
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
…flip CI install to .[airflow]
…him + lazy __init__ + stubs)
…egistration + scan-7 bump
…ger-import + wheel-deps)
… + regenerate uv.lock - regenerate uv.lock for the [airflow] extra (purely additive; no downgrades; default uv sync --dev stays airflow-free) — fixes stale-lock drift - add tests/airflow/test_skeleton.py: base-env coverage of the shim protocols, both stubs raising NotImplementedError, __getattr__/__dir__, and _format_value - mark make_base_operator/make_base_hook bodies '# pragma: no cover' (require the [airflow] extra; mirrors _snowflake_client.make_real_client) - isolate the AST-import branch in the confinement planted-violation self-check (drop the type-ignore so it can't be caught by the comment branch) - drop 'Async/' from make_base_hook docstring
…+ docs) - python-build.md: document the [airflow]-extra deviation (heavy/constraints-only extra deliberately NOT mirrored into [dependency-groups].dev; backed by the no-eager-import gate + wheel-deps assertion) - cli-layer.md: scan-7 count twelve -> fourteen; add SkillError + AirflowIntegrationError to the excluded-bases list; note airflow's excluded-only (no dual-registration) posture - CLAUDE.md: add signalforge.airflow row to the architecture map (integration skeleton, v0.7 epic #228)
wjduenow
changed the title
wjduenow
marked this pull request as ready for review
June 15, 2026 21:03
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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.
Inline comments:
In @.claude/rules/cli-layer.md:
- Around line 101-103: The abstract-base counts in this documentation are
inconsistent and need correction. The text states "ten of the twelve abstract
bases" but should reflect that there are 14 total abstract bases (as defined on
line 99), of which 10 have dual registration in _EXCEPTION_TO_EXIT_CODE and 4
are excluded-only exceptions (DemoError, IngestError, SkillError, and
AirflowIntegrationError). Update the opening statement of the Dual registration
section to change "twelve" to "fourteen" and adjust the supporting text to
accurately reflect that there are four deliberate exceptions rather than
implying only two, ensuring the policy contract language remains internally
consistent with the actual counts described in the paragraph.
In `@src/signalforge/airflow/_airflow_compat.py`:
- Around line 101-106: The __all__ list in the _airflow_compat.py module is
currently exporting underscore-prefixed protocol names (_BaseHookProtocol and
_BaseOperatorProtocol), which violates the convention that underscore-prefixed
symbols are internal and should not be part of the public API surface. Remove
the two underscore-prefixed protocol names from the __all__ list, keeping only
the factory functions make_base_hook and make_base_operator as the public
exports. The protocols will remain directly importable for internal and test
use, but will no longer be part of the official public contract.
In `@src/signalforge/airflow/hooks.py`:
- Line 32: The `__init__` method signature contains unused variadic parameters
that should be marked as intentionally unused according to the repo's coding
guidelines. Rename the `*args` parameter to `*_args` and the `**kwargs`
parameter to `**_kwargs` in the `__init__` method to prefix them with an
underscore, indicating they are internal placeholder parameters that are not
part of the public API and are intentionally unused.
In `@src/signalforge/airflow/operators.py`:
- Line 33: The `__init__` constructor method declares variadic parameters
`*args` and `**kwargs` that are not used within the method body. According to
the coding guidelines, unused or internal-only parameters should be prefixed
with an underscore to signal they are intentional placeholders. Change the
parameter names from `*args, **kwargs` to `*_args, **_kwargs` in the method
signature to mark these as internal implementation details.
In `@tests/airflow/test_airflow_import_confinement.py`:
- Around line 75-83: The false-positive occurs because the code checks if
"airflow" is present anywhere in the full line text, including in import paths
like signalforge.airflow. To fix this, modify the airflow detection to only
check the code portion of the line (before the comment starts), not the comment
itself. Use the comment token's start column position (tok.start[1]) to slice
the line and check only the code portion: examine line_text[:tok.start[1]]
instead of the full line_text when checking if "airflow" is present. This
ensures that "airflow" appearing in a package name or import path is not
mistakenly flagged as a violation.
In `@tests/airflow/test_airflow_no_eager_import.py`:
- Around line 79-84: The test cleanup loop only removes airflow and airflow.*
modules from sys.modules, but prior imports of signalforge.airflow* modules can
remain cached and cause the subsequent import statement to skip the lazy-name
resolution path you intend to test. Extend the cleanup condition in the loop
that checks for name == "airflow" or name.startswith("airflow.") to also check
for name == "signalforge.airflow" or name.startswith("signalforge.airflow.") so
that all previously cached signalforge.airflow modules are also deleted before
the test import runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f376254-52a2-4ff3-b628-c749cbac6fd5
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.claude/rules/cli-layer.md.claude/rules/python-build.md.github/workflows/ci.ymlCLAUDE.mdplans/super/230-airflow-skeleton.mdpyproject.tomlsrc/signalforge/airflow/__init__.pysrc/signalforge/airflow/_airflow_compat.pysrc/signalforge/airflow/errors.pysrc/signalforge/airflow/hooks.pysrc/signalforge/airflow/operators.pysrc/signalforge/cli/_helpers.pytests/airflow/test_airflow_import_confinement.pytests/airflow/test_airflow_no_eager_import.pytests/airflow/test_errors.pytests/airflow/test_skeleton.pytests/test_audit_completeness.pytests/test_wheel_packaging.py
There was a problem hiding this comment.
Pull request overview
This PR introduces the initial signalforge.airflow integration skeleton and an [airflow] optional extra that keeps Apache Airflow completely out of the base install/import path, backed by ungated tests that enforce “core stays lean” invariants.
Changes:
- Add
signalforge.airflowskeleton package (lazy public re-exports, one-shim Airflow import seam, stub operator/hook, typed errors). - Add ungated guardrail tests (no-eager-import, import/type-ignore confinement) plus wheel-level assertions that
apache-airflowis not vendored and not a core dependency. - Wire packaging/CI/docs/scans to recognize the new stage (
airflow/errors.py) and install.[airflow]in the gated workflow job.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Adds [project.optional-dependencies].airflow with apache-airflow>=2.8,<3. |
src/signalforge/airflow/__init__.py |
Establishes lazy public surface via PEP 562 __getattr__ while eagerly re-exporting Airflow-free errors. |
src/signalforge/airflow/_airflow_compat.py |
Single confined shim for all from airflow ... imports (lazy factories + Protocols). |
src/signalforge/airflow/errors.py |
Adds AirflowIntegrationError + AirflowConfigError remediation-rendering seam. |
src/signalforge/airflow/operators.py |
Adds Airflow-free stub operator raising NotImplementedError. |
src/signalforge/airflow/hooks.py |
Adds Airflow-free stub hook raising NotImplementedError. |
src/signalforge/cli/_helpers.py |
Registers AirflowConfigError in _EXCEPTION_TO_EXIT_CODE (tier 2). |
tests/test_audit_completeness.py |
Updates Scan 7 exclusions + expected errors.py modules list to include airflow/errors.py. |
tests/test_wheel_packaging.py |
Adds wheel METADATA/core-deps negative assertions for apache-airflow; refactors wheel build fixture. |
tests/airflow/test_airflow_import_confinement.py |
Enforces all Airflow imports/type-ignores confined to _airflow_compat.py (with planted-violation checks). |
tests/airflow/test_airflow_no_eager_import.py |
Ungated in-process + subprocess checks that importing signalforge.airflow never imports airflow. |
tests/airflow/test_skeleton.py |
Default-suite tests for the Airflow-free skeleton surface (protocols/stubs/error helpers). |
tests/airflow/test_errors.py |
Pins remediation rendering and exit-code mapping for AirflowConfigError. |
.github/workflows/ci.yml |
Updates gated airflow job to install -e '.[airflow]' under constraints. |
CLAUDE.md |
Adds signalforge.airflow to the architecture map summary table. |
plans/super/230-airflow-skeleton.md |
Adds the plan document detailing decisions and deliverables for #230. |
.claude/rules/python-build.md |
Documents the deliberate exception: [airflow] extra is not mirrored into the dev dependency group. |
.claude/rules/cli-layer.md |
Updates Scan 7 documentation for new bases/modules (but contains a count mismatch noted in comments). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- _airflow_compat __all__: drop underscore-prefixed protocols (internal, not public API) - operators/hooks stubs: *_args/**_kwargs for intentionally-unused variadics - confinement test: comment-branch gates on the comment token, not the full line (no false-positive on 'from signalforge.airflow ... # type: ignore') - no-eager + skeleton tests: scrub signalforge.airflow* / fresh-import so the no-import assertions are test-order-independent - cli-layer.md: ten-of-fourteen + four excluded-only bases (count consistency) - plan Meta: phase -> complete (in review)
Owner
Author
PR Review SummaryAll review comments addressed in Fixed (9 threads)
Validationpyright 0 errors · 3831 passed · wheel_smoke 7 passed · |
This was referenced Jun 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Skeleton + packaging for the SignalForge Airflow integration (epic #228, v0.7). Stands up
signalforge.airflow(one-shim seam, lazy__getattr__re-exports, stub operators/hooks) + the[airflow]optional extra — zero Airflow weight in the base install. Builds on the #229 test-environment contract.Status: implemented, in review. Full plan:
plans/super/230-airflow-skeleton.md.Key decisions
[airflow]extra is NOT mirrored into[dependency-groups].dev(documented deviation; default env stays Airflow-free).BaseOperatorat module scope (no-eager-import gate); real subclassing defers to children via the lazy shim factory.Testing
signalforge.airflowpackage 100% covered..venv-airflow(Airflow 2.10.4 / py3.11):.[airflow]resolves, lazy seam holds, DAG parses.Compounding update
python-build.md: documents the deliberate not-mirrored-into-dev-group extra pattern.cli-layer.md: scan-7 count → 14; airflow excluded-base posture.CLAUDE.md: architecture-map row forsignalforge.airflow.Summary by CodeRabbit
Release Notes
New Features
[airflow]extra (kept lightweight for base installs).2).Documentation
Tests
[airflow]extra.