#234: Airflow SignalForgeHook — Connection/Variable → profiles.yml + LLM key - #243
Conversation
- Align Phase metadata (discovery → detailing) with the PR description; the plan is fully detailed through Phase 4 with DECs + US-001..008 (Copilot). - Fix cost-ceilings contradiction: ticket summary listed them in the Connection extra schema, but DEC-011 trims them from v0.7 — note the deferral (Copilot). - Remove duplicate footer scaffolding (second Phase 4 + second Beads Manifest) (Copilot + CodeRabbit).
…e-hook # Conflicts: # plans/super/234-signalforge-hook.md
📝 WalkthroughWalkthroughImplements ChangesSignalForgeHook credential seam
Sequence Diagram(s)sequenceDiagram
participant DAG as Airflow DAG
participant Op as SignalForgeGenerateOperator.execute
participant ResolveHook as _resolve_hook
participant Hook as SignalForgeHook.get_conn
participant Resolver as resolve_connection
participant Compat as _airflow_compat
participant EnvCtx as _provider_key_env
participant Run as run_signalforge
DAG->>Op: execute(context)
Op->>ResolveHook: signalforge_conn_id set?
ResolveHook->>Hook: SignalForgeHook(conn_id).get_conn()
Hook->>Resolver: resolve_connection(conn, variable_lookup)
Resolver-->>Hook: HookResolution(provider, api_key, profiles_dir)
Hook-->>ResolveHook: HookResolution
ResolveHook-->>Op: HookResolution
Op->>Op: validate provider + api_key present
Op->>Compat: register_secret(api_key)
Op->>Op: _merge_with_resolution(profiles_dir, cache_scope)
Op->>EnvCtx: inject PROVIDER_ENV_VAR=api_key
EnvCtx->>Run: run_signalforge(argv)
Run-->>EnvCtx: TaskOutcome
EnvCtx->>EnvCtx: restore prior env value
EnvCtx-->>Op: TaskOutcome
Op-->>DAG: XCom payload (no api_key)
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 |
…signalforge.airflow
Add a closed provider->env-var allowlist to signalforge.llm.providers as a module-level sibling to PROVIDER_DEFAULT_MODELS / PROVIDER_SKU_PREFIXES — the single source of truth for which env var carries each provider's API key, reusable by the Airflow hook (#234) and the v0.8 GitHub Action. Traces: #234 US-001, DEC-005.
…estrator .claude edit)
…ok) + register_secret shim
…enerateOperator + shared helper
…rough SignalForgeGenerateOperator + shared helper
…runeExistingOperator
…rough SignalForgePruneExistingOperator
…ction + example DAG
…rator param tables + explicit Generate conn_id=None no-hook test
…on.md (Patterns & Memory)
There was a problem hiding this comment.
Pull request overview
Implements issue #234 by adding an Airflow-native credential seam (SignalForgeHook) that resolves an Airflow Connection (+ optional Variable fallback) into (profiles_dir, provider, api_key), and wiring signalforge_conn_id through both SignalForgeGenerateOperator (LLM + env-key injection) and SignalForgePruneExistingOperator (profiles-dir only, no key injection). This extends the existing Airflow integration (epic #228) while preserving the “no eager Airflow import” confinement pattern.
Changes:
- Added
PROVIDER_ENV_VAR_KEYSas the shared, closed allowlist mapping provider → API-key env var. - Introduced an Airflow-free pure resolver (
resolve_connection) + typedHookResolution, plus the gatedSignalForgeHook(BaseHook)wrapper. - Wired
signalforge_conn_idthrough both operators (including masking + scoped env injection for generate only), and updated docs + examples + tests accordingly.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/llm/test_providers.py | Adds unit tests pinning PROVIDER_ENV_VAR_KEYS and its export/shape. |
| tests/llm/test_logger_grep_gate.py | Extends logger grep gate scan to include signalforge.airflow. |
| tests/airflow/test_skeleton.py | Updates ungated skeleton tests to validate Airflow-free attribute access + construction-time ImportError. |
| tests/airflow/test_resolve.py | Adds ungated tests covering all branches of the pure Connection/Variable resolver. |
| tests/airflow/test_operators.py | Adds gated tests for signalforge_conn_id wiring, masking, env injection/restore, and no-leak surfaces. |
| tests/airflow/test_operators_helpers.py | Adds ungated tests for _merge_with_resolution and _provider_key_env. |
| tests/airflow/test_hooks.py | Adds gated tests for real hook behavior against Airflow Connection + Variable shim + masking. |
| tests/airflow/test_dag_parse.py | Ensures the new example DAG parses cleanly and asserts expected tasks. |
| src/signalforge/llm/providers.py | Introduces PROVIDER_ENV_VAR_KEYS and exports it via __all__. |
| src/signalforge/airflow/operators.py | Adds shared conn-id helpers, masks secrets, injects provider env var for generate, and wires conn-id through both operators. |
| src/signalforge/airflow/hooks.py | Implements deferred-construction SignalForgeHook with a gated BaseHook subclass and an airflow-missing placeholder. |
| src/signalforge/airflow/_resolve.py | Adds the airflow-free resolver + Pydantic-validated extra schema and redacting HookResolution. |
| src/signalforge/airflow/_airflow_compat.py | Adds shim-confined register_secret and airflow_variable_get. |
| plans/super/234-signalforge-hook.md | Updates implementation plan/DEC log reflecting the final shipped shape (incl. prune-existing wiring). |
| examples/airflow/signalforge_hook_dag.py | Adds an example DAG using a single Connection + Variable to configure both operators. |
| docs/airflow-ops.md | Documents signalforge_conn_id, Connection/Variable precedence, allowlist, hygiene guarantees, and example usage. |
| .claude/rules/diff-renderer.md | Updates logger-gate documentation to reflect the expanded scan set. |
| .claude/rules/cli-layer.md | Updates logger-gate documentation to reflect the expanded scan set and process. |
| .claude/rules/airflow-integration.md | Documents the hook/resolver patterns, leak-surface disciplines, and provider env-var allowlist. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ution (DEC-008 containment)
PR Review SummaryFixed (1 item)
False Positives (0 items)None. CodeRabbit reported no actionable comments. 🎉 Validation
|
Summary
Implements #234 (epic #228) —
SignalForgeHook(BaseHook)keyed onsignalforge_conn_id, mapping an Airflow Connection (+ optional Variable) →(profiles_dir, provider, LLM API key), wired through bothSignalForgeGenerateOperatorandSignalForgePruneExistingOperator.Ralph-executed from
plans/super/234-signalforge-hook.md(16 DECs). This PR began as the plan-revision PR and now carries the full implementation.Stories landed
PROVIDER_ENV_VAR_KEYSclosed allowlist insignalforge.llm.providerssignalforge.airflowresolve_connection+HookResolution+_ConnectionExtra(100% ungated)SignalForgeHook(BaseHook)(deferred construction) +register_secret/airflow_variable_getin the one shimsignalforge_conn_id→ GenerateOperator + shared_merge_with_resolution/_provider_key_envhelperssignalforge_conn_id→ PruneExistingOperator (profiles_dir only — read-only, no key injection)docs/airflow-ops.mdhook section +examples/airflow/signalforge_hook_dag.py(A8)conn_id=Noneno-hook test)airflow-integration.md§ SignalForgeHook)Secrets hygiene (4 surfaces, each test-pinned)
Key never in: task logs (
mask_secret+ lazy-format JSON), XCom (counts+paths only), rendered templates (signalforge_conn_id∉template_fields), or__repr__.Validation
ruff+ruff format+pyright(0 errors) +pytest(3987 passed) green. Gated airflow tests certified against the real.venv-airflowrig (airflow 2.10.4): 58 passed.Summary by CodeRabbit
Release Notes
signalforge_conn_idsupport to Airflow operators for connection-driven credentials and precedence with operator params.SignalForgeHookfor resolving connection-based credentials with Airflow Variable fallback and redacted output.SignalForgeHook+signalforge_conn_idsetup, validation, and generate vs prune credential behavior.