From 82d3097ee99d7db98e2b53801144db6caa548779 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 09:14:11 -0700 Subject: [PATCH 01/15] Add super plan for #234: Airflow SignalForgeHook --- plans/super/234-signalforge-hook.md | 176 ++++++++++++++++++++++++++++ 1 file changed, 176 insertions(+) create mode 100644 plans/super/234-signalforge-hook.md diff --git a/plans/super/234-signalforge-hook.md b/plans/super/234-signalforge-hook.md new file mode 100644 index 00000000..aa1d97b0 --- /dev/null +++ b/plans/super/234-signalforge-hook.md @@ -0,0 +1,176 @@ +# Super Plan — #234: Airflow `SignalForgeHook` (Connection/Variable → profiles.yml + LLM key) + +## Meta + +- **Ticket:** [#234](https://github.com/wjduenow/SignalForge/issues/234) +- **Epic:** [#228](https://github.com/wjduenow/SignalForge/issues/228) (v0.7 Airflow operator) +- **Depends on:** #230 (skeleton + shim), #231 (result→task-state), #232 (`SignalForgeGenerateOperator`) +- **Branch:** `feature/234-signalforge-hook` +- **Worktree:** `../worktrees/SignalForge/234-signalforge-hook` +- **Phase:** discovery +- **Sessions:** 1 (2026-06-16) + +--- + +## Phase 1: Discovery + +### Ticket summary + +Ship `SignalForgeHook(BaseHook)` keyed on a `signalforge_conn_id` so DAG authors configure SignalForge the Airflow-native way (one Connection + optional Variable) instead of hand-managing env vars on every task. The hook resolves three things: + +1. **Warehouse auth** — locate a dbt `profiles.yml` (the existing `WarehouseAdapter.from_profile` seam consumes it). Issue recommends **(a) on-disk profiles** via a `profiles_dir` for v0.7; **(b) synthesize a profiles.yml from an Airflow Connection** is flagged as a non-trivial follow-up (re-deriving the #120 per-type `DbtProfileTarget` validator from a Connection). +2. **LLM API key** — from an Airflow Variable or the Connection `extra`/`password`, injected into the task env for the in-process pipeline call. **Never logged / XCom'd / repr'd / rendered.** +3. **Connection `extra` schema** — `profiles_dir`, `provider`, optional cost ceilings, optional `cache_scope`. + +Acceptance (A8): a DAG configures SignalForge via Connection + Variable (no inline per-task env); credentials never leak; documented in `docs/airflow-ops.md`. + +### Key codebase findings (seam map) + +**Existing state.** `signalforge.airflow` ships: `__init__.py` (lazy `__getattr__` re-exports, incl. a `SignalForgeHook` → `hooks` mapping), `hooks.py` (a `NotImplementedError` **stub** — does NOT subclass `BaseHook` at module scope), `_airflow_compat.py` (the one shim: `make_base_operator` / `make_base_hook` lazy factories + `raise_for_outcome`), `operators.py` (`SignalForgeGenerateOperator` via deferred construction), `result.py` + `runner.py` (airflow-free core), `errors.py` (`AirflowIntegrationError` base + `AirflowConfigError` tier-2 concrete). + +- **`make_base_hook()` already exists** (`_airflow_compat.py:101`) — lazy `from airflow.hooks.base import BaseHook`. The real hook is built on it, exactly as `_make_generate_operator_class` builds on `make_base_operator()`. +- **Deferred-construction precedent** (`operators.py:624-651`): module `__getattr__` + `importlib.util.find_spec("airflow")` → real class (built in a `functools.cache`'d factory) when airflow present, else an airflow-free placeholder whose `__init__` raises `ModuleNotFoundError`. The hook mirrors this. +- **Operator `__init__`** (`operators.py:475-517`) already takes `profiles_dir`; **`template_fields`** = `("project_dir","select","model","profiles_dir","as_of")`. `execute()` calls `run_signalforge(argv, project_dir=…, invocation=…)` at `operators.py:546` (single) / `:586` (batch) — the slot where hook resolution + env injection lands. +- **`run_signalforge`** (`runner.py`): in_process reuses `cli.main(argv)`; **the LLM key is NOT injected by the runner** — the vendor SDK reads `ANTHROPIC_API_KEY`/`OPENAI_API_KEY`/`GOOGLE_API_KEY` from the ambient env (`llm/_anthropic_client.py:97`, `_openai_client.py`, `_gemini_client.py`). So the hook must set the env var **before** `run_signalforge`. **In-process isolation snapshots/restores only `("NO_COLOR","FORCE_COLOR","DBT_PROFILES_DIR")`** (`runner.py:63`) — the provider key env vars are NOT in that set today. +- **Profiles seam.** `load_profile(project_dir, target=None) -> DbtProfileTarget` resolves via `$DBT_PROFILES_DIR` → `/profiles.yml` → `~/.dbt/profiles.yml` (`profiles.py:361-464`). `from_profile(profile)` takes the typed `DbtProfileTarget` (`base.py:273`). The CLI's `--profiles-dir` sets `DBT_PROFILES_DIR`; the operator already emits `--profiles-dir` into argv (`_build_generate_argv`, `operators.py:122`). So **option (a) is "hook supplies `profiles_dir`, which becomes `--profiles-dir`"** — the path already works end-to-end. +- **No provider→env-var mapping table** exists; each SDK reads its own standard var. A small mapping (`{"anthropic":"ANTHROPIC_API_KEY", …}`) is new. +- **`__repr__` redaction precedent.** `DbtProfileTarget` uses `Field(repr=False)` on secrets (`profiles.py:164`); `SnowflakeAdapter.__repr__` shows only `account`+`warehouse` (test: `tests/warehouse/test_snowflake_stub.py:72-112` asserts secret substrings AND field-name labels absent). +- **`SignalForgePruneExistingOperator` DOES NOT EXIST.** Only `SignalForgeGenerateOperator` shipped (#232). The acceptance criterion names a prune-existing operator — scope decision required (see Q2). + +### Airflow 2.x API facts (research, gated to `apache-airflow>=2.8,<3`) + +- `BaseHook.get_connection(conn_id) -> Connection` (classmethod). `Connection` exposes `conn_type/host/login/password/schema/port/extra` + safe `extra_dejson` property (returns `{}` on null, never raises). Put the key in `password` (auto-masked) or in `extra` under a sensitive-keyword key. +- `Variable.get(key, default_var=…, deserialize_json=…)`. **Raises `KeyError` when key absent AND no `default_var`**; pass `default_var=None` for soft lookup. +- **Secrets masker**: `from airflow.utils.log.secrets_masker import mask_secret` — **stable across the entire 2.8–2.11 line** (the `airflow.sdk.*` path is 3.0 only). `mask_secret(value)` registers a process-global log filter. `password` + `extra` values whose **key** matches the sensitive list (`api_key`, `secret`, `token`, `password`, `private_key`, …) are auto-masked; `[core] sensitive_var_conn_names` extends the list. +- **`mask_secret` is log-only** — it does NOT scrub XCom or rendered templates. Keeping the key out of XCom (don't return it) and out of `template_fields` (structural) are separate, necessary disciplines. +- **Testing without a live DB**: inject `AIRFLOW_CONN_` env var (URI or JSON form); `get_connection` reads it before the metadata DB. Secret-absence: assert raw key substring absent from a captured log buffer (and assert the masker redacts to `***` via a buffer with the `SecretsMasker` filter attached). + +### Applicable convention constraints (from `.claude/rules/`) + +- **airflow-integration.md** — airflow-free core vs shim-confined translator; one-shim rule (`_airflow_compat` is the ONLY `from airflow` site); lazy `__getattr__` re-export; deferred construction via `find_spec`; fail-soft derived reads; `[airflow]` extra out of dev group; gated tests (`@pytest.mark.airflow` + in-test `importorskip`); certify against `.venv-airflow`. +- **warehouse-adapters.md** — `from_profile` single entry (don't reinvent warehouse auth); `__repr__` credential redaction; symlink-hardened `canonicalise_path` on every user path. +- **safety-layer.md** — secrets never leave without a receipt (audit records blake2b-8 hash only); `extra="forbid"` on user-input config models; `__repr__` omits credentials. +- **cli-layer.md** — four-tier exit codes; `errors.py` scan-7 (count must stay correct); logger grep-gate covers `signalforge.airflow`? (verify — see open item); `--profiles-dir`/`DBT_PROFILES_DIR` env-mutate-don't-restore pattern. +- **python-build.md / testing-signal.md** — `[airflow]` extra additively in `uv.lock`; no `assert True`; secret-absence assertions; planted-violation self-checks for any new AST scan. + +**No `workflow-project.md` found** — rules apply uniformly. + +### Open items surfaced (carry into refinement) + +1. **Logger grep-gate dir set** — confirm whether `tests/llm/test_logger_grep_gate.py` already scans `src/signalforge/airflow`; if the hook logs, it must use lazy-format JSON regardless. +2. **In-process env-restore set** — if the hook injects a provider key env var for `invocation="in_process"`, the `_ISOLATED_ENV_KEYS` set (or an equivalent restore) must cover the key so a long-lived worker doesn't retain it across tasks. This is a real secrets-hygiene + isolation interaction. +3. **`AirflowConfigError` sufficiency** — whether hook misconfig (missing conn, missing key, bad profiles_dir) reuses `AirflowConfigError` (tier 2) or needs a new concrete (`AirflowConnectionError`). Reuse preferred unless remediation differs materially. + +--- + +## Phase 2: Architecture Review + +Reviewed areas (Performance / Data-model omitted — N/A for a credential-resolving hook with no DB/queries). + +| Area | Rating | Headline finding | +|---|---|---| +| Security (secrets) | **concern** | Approach sound (key flows env → vendor SDK → audit-hash-only, never stored). 4 mandatory disciplines, all bakeable as ACs: `mask_secret` timing, `__repr__` redaction, `profiles_dir` canonicalisation, **closed provider allowlist**. 2 documented caveats: in-process concurrency window + hard-kill bypasses `finally`. | +| Testing | **pass** | Clean ungated/gated split; pure resolver 100% ungated (codecov patch gate); ~11 ungated + ~17 gated cases enumerated. **No new AST scan** — the existing `_airflow_compat` import-confinement scan already globs `hooks.py`. | +| API design | **concern** | Cost ceilings in `extra` are **undeliverable in v0.7** (no CLI landing strip; #232 DEC-002 deferred `config_overrides`) → trim them. Precedence rule needed; typed `HookResolution`; reuse `AirflowConfigError`; `PROVIDER_ENV_VAR_KEYS` home; `extra="forbid"` extra-model. | +| Observability + isolation | **pass** | Env-restore belongs at the **operator** seam, not the runner (`_ISOLATED_ENV_KEYS` stays unchanged); two-layer restore composes cleanly. Logger grep-gate does **not** yet scan `src/signalforge/airflow` → add it + update `cli-layer.md`. Log shape: conn_id / provider / key_source / `profiles_dir_set` bool — never the key. | + +**No blockers that change the approach.** Security "blockers" are mandatory ACs, not redesigns. The one real scope change is trimming cost ceilings from the v0.7 Connection `extra`. + +## Phase 3: Refinement Log + +### Decisions (from discovery answers + architecture review) + +- **DEC-001 — Warehouse auth = on-disk profiles only (option a).** Hook resolves `profiles_dir`; operator passes it as `--profiles-dir` (→ `DBT_PROFILES_DIR`), feeding the existing `load_profile` → `from_profile` seam. Connection-synthesis of a `profiles.yml`/`DbtProfileTarget` (option b) is explicitly deferred. _Rationale: the path already works end-to-end; re-deriving the #120 per-type validator from a Connection is its own ticket._ +- **DEC-002 — Prune-existing wiring deferred to #233.** #234 adds `signalforge_conn_id` to `SignalForgeGenerateOperator` only. `SignalForgePruneExistingOperator` already has an open issue (#233); its `conn_id` wiring rides there. _Action: leave a note on #233 recommending it adopt the same hook param when it lands._ +- **DEC-003 — API key source: `Connection.password` primary, Airflow `Variable` fallback.** `provider` + `profiles_dir` come from `extra_dejson`. Key-source recorded (for the log) as `password | variable | absent`; absent-from-both with a non-skippable run → `AirflowConfigError`. +- **DEC-004 — Pure resolver + typed result; operator injects env.** Airflow-free `resolve_connection(conn, variable_lookup) -> HookResolution(profiles_dir, provider, api_key)` at module scope (100% ungated). Gated `SignalForgeHook.get_conn()` delegates to it. Operator `execute()` snapshots → injects `os.environ[PROVIDER_ENV_VAR]` → `run_signalforge` → restores in `finally` (absent-before→delete-after; prior-value→restore-prior). +- **DEC-005 — Closed provider→env-var allowlist.** `PROVIDER_ENV_VAR_KEYS = {"anthropic":"ANTHROPIC_API_KEY","openai":"OPENAI_API_KEY","gemini":"GOOGLE_API_KEY"}` in `signalforge.llm.providers` (sibling to `PROVIDER_DEFAULT_MODELS`/`PROVIDER_SKU_PREFIXES`, reusable by the v0.8 GH Action). An unknown `provider` from `extra` raises `AirflowConfigError` — never derives an arbitrary env-var name. _Security: closes the arbitrary-env-var injection vector._ +- **DEC-006 — `mask_secret` confined to the shim, called at the operator seam.** New `_airflow_compat.register_secret(value)` (lazy `from airflow.utils.log.secrets_masker import mask_secret`, `# pragma: no cover`, `# type: ignore[import-not-found]`). Operator `execute()` calls it **immediately after resolution, before any logging or `run_signalforge`**. Belt-and-braces over Airflow's auto-masking of `password`/sensitive-`extra` keys. +- **DEC-007 — Four leak-surface disciplines are ACs, each pinned by a test.** (1) `signalforge_conn_id` NOT in `template_fields` (design-time assertion). (2) Never returned into XCom (`to_xcom()` already counts+paths only; key held in a local, never on `self`). (3) `SignalForgeHook.__repr__` + `HookResolution.__repr__` show only `conn_id`/`provider`, never the key or field-name labels (mirror `tests/warehouse/test_snowflake_stub.py`). (4) `mask_secret` for logs (DEC-006). +- **DEC-008 — `profiles_dir` from `extra` is symlink-hardened.** Route through `signalforge._common.path_safety.canonicalise_path`; `PathContainmentError` → `AirflowConfigError`. Containment anchor = `project_dir` when available (the operator has it), else suffix-only with the documented gap (mirrors the init-demo seam pattern). +- **DEC-009 — Reuse `AirflowConfigError` (tier 2); no new error class.** Missing conn / missing-key / unknown-provider / bad-`profiles_dir` are all input-validation. No `errors.py` scan-7 churn, no exit-code-table change. +- **DEC-010 — `extra` validated by an `extra="forbid"` Pydantic model.** `_ConnectionExtra(profiles_dir: str|None, provider: str, cache_scope: str|None)`. Typos (`cache_scop`) fail loud at resolution. _Mirrors safety-layer.md DEC-015._ +- **DEC-011 — Cost ceilings trimmed from the v0.7 `extra` schema.** No CLI flag delivers `max_grade_*` (#232 DEC-002 deferred `config_overrides`); storing them would be a dead affordance. Documented as a follow-up gated on an operator `--config` overlay landing. +- **DEC-012 — Precedence: explicit operator param > Connection `extra` > default.** Mirrors CLI `flag > YAML > default`. Applies to `profiles_dir` and `cache_scope` (the two knobs that exist on both surfaces). +- **DEC-013 — Logger grep-gate extended to `src/signalforge/airflow`.** Add `"airflow"` to `_SCAN_SUBPACKAGES` in `tests/llm/test_logger_grep_gate.py`; update the dir-set wording in `cli-layer.md`/`diff-renderer.md` (they list 6; the test already scans 10). Any hook `_LOGGER` call uses lazy-format `json.dumps`. + +- **DEC-014 — `signalforge_conn_id: str | None = None` (optional).** When `None`, the operator behaves exactly as #232 (ambient env / inline config) — byte-compatible, zero change for existing DAGs. When set, the hook resolves credentials. The Airflow-native path is opt-in. +- **DEC-015 — `invocation` default stays `in_process`; concurrency caveat documented.** No auto-promotion to subprocess. The per-task snapshot/restore `finally` scopes the key; docs state subprocess is the safe choice for concurrent multi-task workers (in-process shares `os.environ` + process-global `redirect_stdout`). Operator keeps explicit control via the existing `invocation` param. + +## Phase 4: Detailed Breakdown + +**Validation command (every story's AC):** `uv sync --dev && uv run ruff check . && uv run ruff format --check . && uv run pyright && uv run pytest` + +Architecture ordering: shared infra → grep-gate → pure resolver → gated hook → operator wiring → docs/example → Quality Gate → Patterns. + +### US-001 — `PROVIDER_ENV_VAR_KEYS` shared table +- **Traces to:** DEC-005. +- **Description:** Add the closed provider→env-var allowlist to `signalforge.llm.providers`, sibling to `PROVIDER_DEFAULT_MODELS`/`PROVIDER_SKU_PREFIXES`. The single source of truth for "which env var carries provider X's key" — reusable by the v0.8 GH Action. +- **Files:** `src/signalforge/llm/providers.py` (add `PROVIDER_ENV_VAR_KEYS = {"anthropic":"ANTHROPIC_API_KEY","openai":"OPENAI_API_KEY","gemini":"GOOGLE_API_KEY"}` + export); `tests/llm/test_providers.py` (or nearest) — test every key is a registered provider, and the table's keys match `PROVIDER_DEFAULT_MODELS`'s keys. +- **AC:** Table present + exported; test pins parity with the registered providers; validation passes. +- **Done when:** `from signalforge.llm.providers import PROVIDER_ENV_VAR_KEYS` resolves all three providers. +- **Depends on:** none. + +### US-002 — Extend logger grep-gate to `signalforge.airflow` +- **Traces to:** DEC-013. +- **Description:** The lazy-format logger gate does not yet scan `src/signalforge/airflow`; the hook + operator will log. Add `"airflow"` to the scan set and correct the dir-set wording in the rule files (they say 6; the test already scans 10). +- **Files:** `tests/llm/test_logger_grep_gate.py` (add `"airflow"` to `_SCAN_SUBPACKAGES`); `.claude/rules/cli-layer.md` + `.claude/rules/diff-renderer.md` (dir-set wording — **orchestrator-applied**, `.claude/` is not worker-writable). +- **AC:** Gate scans `src/signalforge/airflow` and passes against current tree; rule wording matches the actual scan set; validation passes. +- **Done when:** an `_LOGGER.x(f"…")` planted in an airflow module fails the gate. +- **Depends on:** none. + +### US-003 — Airflow-free pure resolver + typed models (100% ungated) +- **Traces to:** DEC-003, DEC-004 (pure half), DEC-005, DEC-008, DEC-009, DEC-010. +- **Description:** The airflow-free heart. `HookResolution` frozen dataclass `(profiles_dir: str|None, provider: str, api_key: str|None)` with redacting `__repr__`; `_ConnectionExtra` Pydantic model (`extra="forbid"`); `resolve_connection(conn_like, *, variable_lookup, project_dir=None) -> HookResolution` — reads `password`→Variable-fallback for the key, `provider`/`profiles_dir` from validated `extra`, enforces the `PROVIDER_ENV_VAR_KEYS` allowlist, canonicalises `profiles_dir`, raises `AirflowConfigError` on misconfig. Takes a duck-typed conn (`.password`, `.extra_dejson`) + an injected `variable_lookup` callable so it needs no airflow import. +- **Files:** new `src/signalforge/airflow/_resolve.py` (pure module); `src/signalforge/airflow/errors.py` (confirm `AirflowConfigError` remediation covers the new cases); new `tests/airflow/test_resolve.py` (ungated). +- **TDD cases:** provider+key from `password`; Variable fallback when no `password`; unknown provider → `AirflowConfigError`; missing key from both → `AirflowConfigError`; `extra` typo → `extra="forbid"` raise; `profiles_dir` symlink-escape → `AirflowConfigError`; `__repr__` omits key substring + field-name labels; frozen dataclass. +- **AC:** 100% ungated coverage of `_resolve.py` (codecov patch gate); no `from airflow` import in the module; validation passes. +- **Done when:** every TDD case passes ungated. +- **Depends on:** US-001. + +### US-004 — Real `SignalForgeHook(BaseHook)` + `register_secret` shim (gated) +- **Traces to:** DEC-004 (gated half), DEC-006, DEC-007 (repr/logs). +- **Description:** Replace the `hooks.py` stub with the real hook via the deferred-construction pattern (module `__getattr__` + `functools.cache` factory + `find_spec("airflow")` branch + airflow-free placeholder raising `ModuleNotFoundError`). `get_conn()` builds the `variable_lookup` from `Variable.get(k, default_var=None)` and delegates to `resolve_connection`. Add `_airflow_compat.register_secret(value)` (lazy `from airflow.utils.log.secrets_masker import mask_secret`, `# pragma: no cover`, `# type: ignore[import-not-found]`). Redacting `__repr__` on the hook. +- **Files:** `src/signalforge/airflow/hooks.py` (real hook); `src/signalforge/airflow/_airflow_compat.py` (+`register_secret`, +`__all__`); `src/signalforge/airflow/__init__.py` (lazy `__getattr__` already maps `SignalForgeHook` → confirm); new `tests/airflow/test_hooks.py` (gated `@pytest.mark.airflow` + in-test `importorskip`). +- **TDD cases (gated):** `get_conn()` happy path; Variable fallback; missing-conn / unknown-provider / missing-key → `AirflowConfigError`; hook never logs the raw key (caplog substring absent); masker redacts to `***` (SecretsMasker-filtered buffer); `__repr__` redaction; airflow-free access + construction-requires-airflow (ungated skeleton test in `test_skeleton.py`). +- **AC:** import-confinement scan still passes (no module-scope `from airflow` in `hooks.py`); gated tests pass under the airflow rig; validation passes (ungated portion). +- **Done when:** `SignalForgeHook(conn_id).get_conn()` returns a `HookResolution` against a fake connection. +- **Depends on:** US-003, US-002. + +### US-005 — Wire `signalforge_conn_id` through `SignalForgeGenerateOperator` +- **Traces to:** DEC-002, DEC-006, DEC-007 (template/XCom), DEC-012, DEC-014, DEC-015. +- **Description:** Add `signalforge_conn_id: str | None = None` to the operator `__init__` (NOT in `template_fields`). When set, `execute()`: resolve via the hook → `register_secret(api_key)` → apply precedence (param > extra > default) for `profiles_dir`/`cache_scope` → snapshot `os.environ[PROVIDER_ENV_VAR]` → inject → `run_signalforge(...)` → restore in `finally` (absent→delete, prior→restore). Single + batch paths. INFO log: conn_id/provider/key_source/`profiles_dir_set` (never the key). When `None`, behaviour is byte-identical to #232. +- **Files:** `src/signalforge/airflow/operators.py`; `tests/airflow/test_operators_helpers.py` (ungated — precedence resolution helper); `tests/airflow/test_operators.py` (gated — env inject/restore success+exception, XCom key-absence, `signalforge_conn_id` ∉ `template_fields`, single+batch with conn_id). +- **AC:** `conn_id=None` path unchanged from #232 (existing tests green); env restored on success AND exception; key absent from XCom + rendered fields; validation passes. +- **Done when:** an operator built with a `signalforge_conn_id` injects the right env var around `run_signalforge` and restores it. +- **Depends on:** US-004. + +### US-006 — `docs/airflow-ops.md` + example DAG (acceptance A8) +- **Traces to:** DEC-001, DEC-003, DEC-010, DEC-011, DEC-012, DEC-015. +- **Description:** Document the Connection `extra` schema (`profiles_dir`, `provider`, `cache_scope`; note cost-ceilings deferral), the `password`+Variable key precedence, secrets-hygiene guarantees (4 surfaces), the in-process concurrency caveat (prefer subprocess for concurrent workers), and the `.venv-airflow` certification command. Ship an example DAG configuring SignalForge via a Connection + Variable with no inline per-task env. +- **Files:** `docs/airflow-ops.md`; `examples/airflow/signalforge_hook_dag.py`; `tests/airflow/test_dag_parse.py` (gated DAG-parse of the new example). +- **AC:** A8 met (DAG configures via Connection + Variable, no inline env); example parses via `DagBag` (gated); docs cover the extra schema + hygiene + caveat + cert command; validation passes. +- **Done when:** the example DAG parses and the ops doc documents the full hook contract. +- **Depends on:** US-005. + +### US-007 — Quality Gate +- **Description:** Run the code reviewer 4× across the full changeset, fixing every real bug each pass; run CodeRabbit; certify the airflow-touching paths against the real `.venv-airflow` rig (`SF_RUN_AIRFLOW=1 PYTHONPATH="$PWD/src" /path/to/.venv-airflow/bin/python -m pytest tests/airflow -m airflow --no-cov`). Project validation passes after all fixes. +- **AC:** 4 review passes complete + fixes applied; CodeRabbit addressed; `.venv-airflow` certification green; full validation passes. +- **Depends on:** US-006 (all implementation complete). + +### US-008 — Patterns & Memory (priority 99) +- **Description:** Update `.claude/rules/airflow-integration.md` (hook landed: pure-resolver + shim-confined-`mask_secret` pattern, the four leak-surface disciplines, `PROVIDER_ENV_VAR_KEYS` home, on-disk-profiles DEC, cost-ceiling deferral) — **orchestrator-applied**. Add a memory for the secrets-hygiene-across-4-surfaces hook pattern. Leave the recommended note on #233. +- **AC:** rule file reflects the shipped hook; memory written; validation passes. +- **Depends on:** US-007. + +## Beads Manifest + +_(pending devolve)_ + +## Phase 4: Detailed Breakdown + +_(pending)_ + +## Beads Manifest + +_(pending)_ From 676ad81134d16b690e000d8977d3253296322283 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 09:35:57 -0700 Subject: [PATCH 02/15] #234: Address PR review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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). --- plans/super/234-signalforge-hook.md | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/plans/super/234-signalforge-hook.md b/plans/super/234-signalforge-hook.md index aa1d97b0..62f6056f 100644 --- a/plans/super/234-signalforge-hook.md +++ b/plans/super/234-signalforge-hook.md @@ -7,7 +7,7 @@ - **Depends on:** #230 (skeleton + shim), #231 (result→task-state), #232 (`SignalForgeGenerateOperator`) - **Branch:** `feature/234-signalforge-hook` - **Worktree:** `../worktrees/SignalForge/234-signalforge-hook` -- **Phase:** discovery +- **Phase:** detailing - **Sessions:** 1 (2026-06-16) --- @@ -20,7 +20,7 @@ Ship `SignalForgeHook(BaseHook)` keyed on a `signalforge_conn_id` so DAG authors 1. **Warehouse auth** — locate a dbt `profiles.yml` (the existing `WarehouseAdapter.from_profile` seam consumes it). Issue recommends **(a) on-disk profiles** via a `profiles_dir` for v0.7; **(b) synthesize a profiles.yml from an Airflow Connection** is flagged as a non-trivial follow-up (re-deriving the #120 per-type `DbtProfileTarget` validator from a Connection). 2. **LLM API key** — from an Airflow Variable or the Connection `extra`/`password`, injected into the task env for the in-process pipeline call. **Never logged / XCom'd / repr'd / rendered.** -3. **Connection `extra` schema** — `profiles_dir`, `provider`, optional cost ceilings, optional `cache_scope`. +3. **Connection `extra` schema** — `profiles_dir`, `provider`, optional `cache_scope`. (Cost ceilings are **not** in the v0.7 `extra` schema — they have no CLI landing strip and are trimmed per DEC-011.) Acceptance (A8): a DAG configures SignalForge via Connection + Variable (no inline per-task env); credentials never leak; documented in `docs/airflow-ops.md`. @@ -166,11 +166,3 @@ Architecture ordering: shared infra → grep-gate → pure resolver → gated ho ## Beads Manifest _(pending devolve)_ - -## Phase 4: Detailed Breakdown - -_(pending)_ - -## Beads Manifest - -_(pending)_ From b93c78a0e95b8f6ff89fa96a3e2774b2901369c9 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 09:42:37 -0700 Subject: [PATCH 03/15] Revise #234 plan after #233 merge: wire conn_id through both operators --- plans/super/234-signalforge-hook.md | 61 +++++++++++++++++------------ 1 file changed, 35 insertions(+), 26 deletions(-) diff --git a/plans/super/234-signalforge-hook.md b/plans/super/234-signalforge-hook.md index 62f6056f..0be26808 100644 --- a/plans/super/234-signalforge-hook.md +++ b/plans/super/234-signalforge-hook.md @@ -4,11 +4,11 @@ - **Ticket:** [#234](https://github.com/wjduenow/SignalForge/issues/234) - **Epic:** [#228](https://github.com/wjduenow/SignalForge/issues/228) (v0.7 Airflow operator) -- **Depends on:** #230 (skeleton + shim), #231 (result→task-state), #232 (`SignalForgeGenerateOperator`) +- **Depends on:** #230 (skeleton + shim), #231 (result→task-state), #232 (`SignalForgeGenerateOperator`), **#233 (`SignalForgePruneExistingOperator`) — MERGED to `dev` 2026-06-16** - **Branch:** `feature/234-signalforge-hook` - **Worktree:** `../worktrees/SignalForge/234-signalforge-hook` -- **Phase:** detailing -- **Sessions:** 1 (2026-06-16) +- **Phase:** detailing (revised after #233 merge) +- **Sessions:** 2 (2026-06-16) --- @@ -35,7 +35,7 @@ Acceptance (A8): a DAG configures SignalForge via Connection + Variable (no inli - **Profiles seam.** `load_profile(project_dir, target=None) -> DbtProfileTarget` resolves via `$DBT_PROFILES_DIR` → `/profiles.yml` → `~/.dbt/profiles.yml` (`profiles.py:361-464`). `from_profile(profile)` takes the typed `DbtProfileTarget` (`base.py:273`). The CLI's `--profiles-dir` sets `DBT_PROFILES_DIR`; the operator already emits `--profiles-dir` into argv (`_build_generate_argv`, `operators.py:122`). So **option (a) is "hook supplies `profiles_dir`, which becomes `--profiles-dir`"** — the path already works end-to-end. - **No provider→env-var mapping table** exists; each SDK reads its own standard var. A small mapping (`{"anthropic":"ANTHROPIC_API_KEY", …}`) is new. - **`__repr__` redaction precedent.** `DbtProfileTarget` uses `Field(repr=False)` on secrets (`profiles.py:164`); `SnowflakeAdapter.__repr__` shows only `account`+`warehouse` (test: `tests/warehouse/test_snowflake_stub.py:72-112` asserts secret substrings AND field-name labels absent). -- **`SignalForgePruneExistingOperator` DOES NOT EXIST.** Only `SignalForgeGenerateOperator` shipped (#232). The acceptance criterion names a prune-existing operator — scope decision required (see Q2). +- **`SignalForgePruneExistingOperator` now EXISTS** (merged #233 to `dev`, 2026-06-16). `__init__(*, task_id, project_dir, model, schema, profiles_dir=None, manifest=None, scope=None, sample_strategy=None, as_of=None, tests_dir=None, on_flagged="fail", invocation="in_process", **kwargs)`; `template_fields = ("project_dir","model","schema","profiles_dir","as_of","tests_dir")`; pure helpers `_build_prune_existing_argv` / `_validate_prune_existing_config`; `execute()` validates → builds argv → `run_signalforge` → `decide_task_outcome` → `raise_for_outcome` → `to_xcom`. **Read-only, makes NO LLM call** (#233 DEC-001) — so it needs **warehouse auth (`profiles_dir`) ONLY, no LLM key**; it has no `cache_scope` param. The #234 acceptance criterion names BOTH operators → `signalforge_conn_id` now wires through both (see DEC-002, DEC-016). ### Airflow 2.x API facts (research, gated to `apache-airflow>=2.8,<3`) @@ -81,21 +81,22 @@ Reviewed areas (Performance / Data-model omitted — N/A for a credential-resolv ### Decisions (from discovery answers + architecture review) - **DEC-001 — Warehouse auth = on-disk profiles only (option a).** Hook resolves `profiles_dir`; operator passes it as `--profiles-dir` (→ `DBT_PROFILES_DIR`), feeding the existing `load_profile` → `from_profile` seam. Connection-synthesis of a `profiles.yml`/`DbtProfileTarget` (option b) is explicitly deferred. _Rationale: the path already works end-to-end; re-deriving the #120 per-type validator from a Connection is its own ticket._ -- **DEC-002 — Prune-existing wiring deferred to #233.** #234 adds `signalforge_conn_id` to `SignalForgeGenerateOperator` only. `SignalForgePruneExistingOperator` already has an open issue (#233); its `conn_id` wiring rides there. _Action: leave a note on #233 recommending it adopt the same hook param when it lands._ -- **DEC-003 — API key source: `Connection.password` primary, Airflow `Variable` fallback.** `provider` + `profiles_dir` come from `extra_dejson`. Key-source recorded (for the log) as `password | variable | absent`; absent-from-both with a non-skippable run → `AirflowConfigError`. +- **DEC-002 — `signalforge_conn_id` wires through BOTH operators (REVISED after #233 merge).** #234 adds the optional param to `SignalForgeGenerateOperator` AND `SignalForgePruneExistingOperator` (now merged on `dev`). The two consumers differ: Generate needs `profiles_dir` + `provider` + `api_key`; PruneExisting needs `profiles_dir` ONLY (no LLM call → no key, no `provider`, no `cache_scope`). _Supersedes the session-1 deferral; the original "leave a note on #233" action is moot — #233 shipped and #234 owns the wiring for both._ +- **DEC-003 — API key source: `Connection.password` primary, Airflow `Variable` fallback.** `provider` + `profiles_dir` come from `extra_dejson`. Key-source recorded (for the log) as `password | variable | absent`. **The resolver is lenient** — it returns `api_key`/`provider` as `None` when absent; *requiredness is enforced by the consumer*: the Generate operator raises `AirflowConfigError` when `api_key`/`provider` is missing, PruneExisting never checks them (it needs neither). This is what lets one resolver serve both operators (see DEC-016). - **DEC-004 — Pure resolver + typed result; operator injects env.** Airflow-free `resolve_connection(conn, variable_lookup) -> HookResolution(profiles_dir, provider, api_key)` at module scope (100% ungated). Gated `SignalForgeHook.get_conn()` delegates to it. Operator `execute()` snapshots → injects `os.environ[PROVIDER_ENV_VAR]` → `run_signalforge` → restores in `finally` (absent-before→delete-after; prior-value→restore-prior). -- **DEC-005 — Closed provider→env-var allowlist.** `PROVIDER_ENV_VAR_KEYS = {"anthropic":"ANTHROPIC_API_KEY","openai":"OPENAI_API_KEY","gemini":"GOOGLE_API_KEY"}` in `signalforge.llm.providers` (sibling to `PROVIDER_DEFAULT_MODELS`/`PROVIDER_SKU_PREFIXES`, reusable by the v0.8 GH Action). An unknown `provider` from `extra` raises `AirflowConfigError` — never derives an arbitrary env-var name. _Security: closes the arbitrary-env-var injection vector._ +- **DEC-005 — Closed provider→env-var allowlist.** `PROVIDER_ENV_VAR_KEYS = {"anthropic":"ANTHROPIC_API_KEY","openai":"OPENAI_API_KEY","gemini":"GOOGLE_API_KEY"}` in `signalforge.llm.providers` (sibling to `PROVIDER_DEFAULT_MODELS`/`PROVIDER_SKU_PREFIXES`, reusable by the v0.8 GH Action). The allowlist is validated **whenever `provider` is present** (regardless of consumer): a non-`None` unknown `provider` raises `AirflowConfigError` — never derives an arbitrary env-var name. The env-var lookup itself is used only by the Generate operator's injection path. _Security: closes the arbitrary-env-var injection vector even for a prune-existing-only Connection that happens to set `provider`._ - **DEC-006 — `mask_secret` confined to the shim, called at the operator seam.** New `_airflow_compat.register_secret(value)` (lazy `from airflow.utils.log.secrets_masker import mask_secret`, `# pragma: no cover`, `# type: ignore[import-not-found]`). Operator `execute()` calls it **immediately after resolution, before any logging or `run_signalforge`**. Belt-and-braces over Airflow's auto-masking of `password`/sensitive-`extra` keys. - **DEC-007 — Four leak-surface disciplines are ACs, each pinned by a test.** (1) `signalforge_conn_id` NOT in `template_fields` (design-time assertion). (2) Never returned into XCom (`to_xcom()` already counts+paths only; key held in a local, never on `self`). (3) `SignalForgeHook.__repr__` + `HookResolution.__repr__` show only `conn_id`/`provider`, never the key or field-name labels (mirror `tests/warehouse/test_snowflake_stub.py`). (4) `mask_secret` for logs (DEC-006). - **DEC-008 — `profiles_dir` from `extra` is symlink-hardened.** Route through `signalforge._common.path_safety.canonicalise_path`; `PathContainmentError` → `AirflowConfigError`. Containment anchor = `project_dir` when available (the operator has it), else suffix-only with the documented gap (mirrors the init-demo seam pattern). -- **DEC-009 — Reuse `AirflowConfigError` (tier 2); no new error class.** Missing conn / missing-key / unknown-provider / bad-`profiles_dir` are all input-validation. No `errors.py` scan-7 churn, no exit-code-table change. -- **DEC-010 — `extra` validated by an `extra="forbid"` Pydantic model.** `_ConnectionExtra(profiles_dir: str|None, provider: str, cache_scope: str|None)`. Typos (`cache_scop`) fail loud at resolution. _Mirrors safety-layer.md DEC-015._ +- **DEC-009 — Reuse `AirflowConfigError` (tier 2); no new error class.** Missing conn / missing-key (Generate only) / unknown-provider / bad-`profiles_dir` are all input-validation. No `errors.py` scan-7 churn, no exit-code-table change. PruneExisting's own `_validate_prune_existing_config` already raises `AirflowConfigError` — same class, reused. +- **DEC-010 — `extra` validated by an `extra="forbid"` Pydantic model; all fields optional.** `_ConnectionExtra(profiles_dir: str|None = None, provider: str|None = None, cache_scope: str|None = None)`. `provider` is optional (a prune-existing-only Connection legitimately omits it); when present it's allowlist-checked (DEC-005). Typos (`cache_scop`) fail loud at resolution. _Mirrors safety-layer.md DEC-015._ - **DEC-011 — Cost ceilings trimmed from the v0.7 `extra` schema.** No CLI flag delivers `max_grade_*` (#232 DEC-002 deferred `config_overrides`); storing them would be a dead affordance. Documented as a follow-up gated on an operator `--config` overlay landing. - **DEC-012 — Precedence: explicit operator param > Connection `extra` > default.** Mirrors CLI `flag > YAML > default`. Applies to `profiles_dir` and `cache_scope` (the two knobs that exist on both surfaces). - **DEC-013 — Logger grep-gate extended to `src/signalforge/airflow`.** Add `"airflow"` to `_SCAN_SUBPACKAGES` in `tests/llm/test_logger_grep_gate.py`; update the dir-set wording in `cli-layer.md`/`diff-renderer.md` (they list 6; the test already scans 10). Any hook `_LOGGER` call uses lazy-format `json.dumps`. - **DEC-014 — `signalforge_conn_id: str | None = None` (optional).** When `None`, the operator behaves exactly as #232 (ambient env / inline config) — byte-compatible, zero change for existing DAGs. When set, the hook resolves credentials. The Airflow-native path is opt-in. - **DEC-015 — `invocation` default stays `in_process`; concurrency caveat documented.** No auto-promotion to subprocess. The per-task snapshot/restore `finally` scopes the key; docs state subprocess is the safe choice for concurrent multi-task workers (in-process shares `os.environ` + process-global `redirect_stdout`). Operator keeps explicit control via the existing `invocation` param. +- **DEC-016 — PruneExisting wiring resolves warehouse auth ONLY; no key injection (NEW, post-#233).** `SignalForgePruneExistingOperator` makes no LLM call, so its `signalforge_conn_id` path uses only the resolved `profiles_dir` (precedence: param > `extra` > default, per DEC-012). It does NOT call `register_secret`, does NOT inject any provider env var, and ignores `provider`/`api_key` on the resolution. A shared helper (extracted in US-005, reused in US-006) does the conn→`HookResolution` call + `profiles_dir` precedence; only the Generate path adds the key-injection + masking + env-restore wrapper. `signalforge_conn_id` must NOT enter either operator's `template_fields`. ## Phase 4: Detailed Breakdown @@ -137,31 +138,39 @@ Architecture ordering: shared infra → grep-gate → pure resolver → gated ho - **Done when:** `SignalForgeHook(conn_id).get_conn()` returns a `HookResolution` against a fake connection. - **Depends on:** US-003, US-002. -### US-005 — Wire `signalforge_conn_id` through `SignalForgeGenerateOperator` -- **Traces to:** DEC-002, DEC-006, DEC-007 (template/XCom), DEC-012, DEC-014, DEC-015. -- **Description:** Add `signalforge_conn_id: str | None = None` to the operator `__init__` (NOT in `template_fields`). When set, `execute()`: resolve via the hook → `register_secret(api_key)` → apply precedence (param > extra > default) for `profiles_dir`/`cache_scope` → snapshot `os.environ[PROVIDER_ENV_VAR]` → inject → `run_signalforge(...)` → restore in `finally` (absent→delete, prior→restore). Single + batch paths. INFO log: conn_id/provider/key_source/`profiles_dir_set` (never the key). When `None`, behaviour is byte-identical to #232. -- **Files:** `src/signalforge/airflow/operators.py`; `tests/airflow/test_operators_helpers.py` (ungated — precedence resolution helper); `tests/airflow/test_operators.py` (gated — env inject/restore success+exception, XCom key-absence, `signalforge_conn_id` ∉ `template_fields`, single+batch with conn_id). -- **AC:** `conn_id=None` path unchanged from #232 (existing tests green); env restored on success AND exception; key absent from XCom + rendered fields; validation passes. -- **Done when:** an operator built with a `signalforge_conn_id` injects the right env var around `run_signalforge` and restores it. +### US-005 — Wire `signalforge_conn_id` through `SignalForgeGenerateOperator` (+ shared helper) +- **Traces to:** DEC-002, DEC-006, DEC-007 (template/XCom), DEC-012, DEC-014, DEC-015, DEC-016. +- **Description:** Add `signalforge_conn_id: str | None = None` to the operator `__init__` (NOT in `template_fields`). Extract a **shared, reusable helper** (consumed again by US-006) that, given a `conn_id`, calls the hook → `HookResolution` and applies precedence (param > `extra` > default) for `profiles_dir`/`cache_scope` — e.g. `_apply_hook_resolution(...)` plus a key-injection context manager `_provider_key_env(provider, api_key)`. When `conn_id` is set, Generate's `execute()`: resolve → require `provider`+`api_key` (else `AirflowConfigError`) → `register_secret(api_key)` → precedence-merge profiles_dir/cache_scope → enter `_provider_key_env` (snapshot `os.environ[PROVIDER_ENV_VAR]`, inject, restore in `finally`: absent→delete, prior→restore) → `run_signalforge(...)`. Single + batch paths. INFO log: conn_id/provider/key_source/`profiles_dir_set` (never the key). When `None`, behaviour is byte-identical to #232. +- **Files:** `src/signalforge/airflow/operators.py` (+ shared helper module-level fns); `tests/airflow/test_operators_helpers.py` (ungated — precedence + helper logic); `tests/airflow/test_operators.py` (gated — env inject/restore success+exception, XCom key-absence, `signalforge_conn_id` ∉ `template_fields`, single+batch with conn_id, missing-key→`AirflowConfigError`). +- **AC:** `conn_id=None` path unchanged from #232 (existing tests green); env restored on success AND exception; key absent from XCom + rendered fields; the shared helper is module-level + ungated-testable; validation passes. +- **Done when:** a Generate operator built with a `signalforge_conn_id` injects the right env var around `run_signalforge` and restores it. - **Depends on:** US-004. -### US-006 — `docs/airflow-ops.md` + example DAG (acceptance A8) -- **Traces to:** DEC-001, DEC-003, DEC-010, DEC-011, DEC-012, DEC-015. -- **Description:** Document the Connection `extra` schema (`profiles_dir`, `provider`, `cache_scope`; note cost-ceilings deferral), the `password`+Variable key precedence, secrets-hygiene guarantees (4 surfaces), the in-process concurrency caveat (prefer subprocess for concurrent workers), and the `.venv-airflow` certification command. Ship an example DAG configuring SignalForge via a Connection + Variable with no inline per-task env. -- **Files:** `docs/airflow-ops.md`; `examples/airflow/signalforge_hook_dag.py`; `tests/airflow/test_dag_parse.py` (gated DAG-parse of the new example). -- **AC:** A8 met (DAG configures via Connection + Variable, no inline env); example parses via `DagBag` (gated); docs cover the extra schema + hygiene + caveat + cert command; validation passes. -- **Done when:** the example DAG parses and the ops doc documents the full hook contract. +### US-006 — Wire `signalforge_conn_id` through `SignalForgePruneExistingOperator` +- **Traces to:** DEC-002, DEC-007 (template/XCom), DEC-012, DEC-016. +- **Description:** Add `signalforge_conn_id: str | None = None` to the merged-#233 operator `__init__` (NOT in its `template_fields`). When set, `execute()` resolves via the hook and applies **only** the `profiles_dir` precedence (param > `extra` > default) using the US-005 shared helper. **No `register_secret`, no provider env-var injection** — prune-existing makes no LLM call (DEC-016); `provider`/`api_key`/`cache_scope` on the resolution are ignored (an allowlist-invalid `provider`, if present, still raises per DEC-005). INFO log: conn_id/`profiles_dir_set`. When `None`, behaviour is byte-identical to #233. +- **Files:** `src/signalforge/airflow/operators.py` (prune-existing `__init__` + `execute`); `tests/airflow/test_operators_helpers.py` (ungated — prune-existing profiles_dir precedence); `tests/airflow/test_operators.py` (gated — conn-resolved `profiles_dir` → argv, NO provider env var touched, `signalforge_conn_id` ∉ `template_fields`, `conn_id=None` unchanged). +- **AC:** `conn_id=None` path unchanged from #233; resolved `profiles_dir` reaches `--profiles-dir`; no provider env var is set/restored on this path; validation passes. +- **Done when:** a PruneExisting operator with a `signalforge_conn_id` runs with the conn-resolved `profiles_dir` and injects no LLM key. - **Depends on:** US-005. -### US-007 — Quality Gate +### US-007 — `docs/airflow-ops.md` + example DAG (acceptance A8) +- **Traces to:** DEC-001, DEC-003, DEC-010, DEC-011, DEC-012, DEC-015, DEC-016. +- **Description:** Document the Connection `extra` schema (`profiles_dir`, `provider`, `cache_scope`; note cost-ceilings deferral), the `password`+Variable key precedence, secrets-hygiene guarantees (4 surfaces), the in-process concurrency caveat (prefer subprocess for concurrent workers), the `.venv-airflow` certification command, and **the Generate-vs-PruneExisting credential difference** (PruneExisting needs only warehouse auth, no LLM key — a prune-existing-only Connection can omit `provider`/key). Ship an example DAG configuring **both** operators via a Connection + Variable with no inline per-task env. +- **Files:** `docs/airflow-ops.md`; `examples/airflow/signalforge_hook_dag.py`; `tests/airflow/test_dag_parse.py` (gated DAG-parse of the new example). +- **AC:** A8 met (a DAG configures both operators via Connection + Variable, no inline env); example parses via `DagBag` (gated); docs cover the extra schema + hygiene + caveat + cert command + the two-operator credential difference; validation passes. +- **Done when:** the example DAG parses and the ops doc documents the full hook contract for both operators. +- **Depends on:** US-006. + +### US-008 — Quality Gate - **Description:** Run the code reviewer 4× across the full changeset, fixing every real bug each pass; run CodeRabbit; certify the airflow-touching paths against the real `.venv-airflow` rig (`SF_RUN_AIRFLOW=1 PYTHONPATH="$PWD/src" /path/to/.venv-airflow/bin/python -m pytest tests/airflow -m airflow --no-cov`). Project validation passes after all fixes. - **AC:** 4 review passes complete + fixes applied; CodeRabbit addressed; `.venv-airflow` certification green; full validation passes. -- **Depends on:** US-006 (all implementation complete). +- **Depends on:** US-007 (all implementation complete). -### US-008 — Patterns & Memory (priority 99) -- **Description:** Update `.claude/rules/airflow-integration.md` (hook landed: pure-resolver + shim-confined-`mask_secret` pattern, the four leak-surface disciplines, `PROVIDER_ENV_VAR_KEYS` home, on-disk-profiles DEC, cost-ceiling deferral) — **orchestrator-applied**. Add a memory for the secrets-hygiene-across-4-surfaces hook pattern. Leave the recommended note on #233. +### US-009 — Patterns & Memory (priority 99) +- **Description:** Update `.claude/rules/airflow-integration.md` (hook landed: pure-resolver + shim-confined-`mask_secret` pattern, the four leak-surface disciplines, `PROVIDER_ENV_VAR_KEYS` home, on-disk-profiles DEC, cost-ceiling deferral, the two-consumer wiring where PruneExisting resolves warehouse-auth-only) — **orchestrator-applied**. Add a memory for the secrets-hygiene-across-4-surfaces hook pattern. - **AC:** rule file reflects the shipped hook; memory written; validation passes. -- **Depends on:** US-007. +- **Depends on:** US-008. ## Beads Manifest From 2ac9685139f6b8191e3b8e5cd03358f30f7c6093 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 10:02:48 -0700 Subject: [PATCH 04/15] #234: devolve plan to beads (epic bd_1-scaffolding-qhi, 9 tasks) --- plans/super/234-signalforge-hook.md | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/plans/super/234-signalforge-hook.md b/plans/super/234-signalforge-hook.md index 0be26808..a68ee36f 100644 --- a/plans/super/234-signalforge-hook.md +++ b/plans/super/234-signalforge-hook.md @@ -7,8 +7,9 @@ - **Depends on:** #230 (skeleton + shim), #231 (result→task-state), #232 (`SignalForgeGenerateOperator`), **#233 (`SignalForgePruneExistingOperator`) — MERGED to `dev` 2026-06-16** - **Branch:** `feature/234-signalforge-hook` - **Worktree:** `../worktrees/SignalForge/234-signalforge-hook` -- **Phase:** detailing (revised after #233 merge) +- **Phase:** devolved - **Sessions:** 2 (2026-06-16) +- **Epic bead:** `bd_1-scaffolding-qhi` (9 task beads `.1`–`.9`) --- @@ -174,4 +175,16 @@ Architecture ordering: shared infra → grep-gate → pure resolver → gated ho ## Beads Manifest -_(pending devolve)_ +- **Epic:** `bd_1-scaffolding-qhi` +- **Worktree:** `../worktrees/SignalForge/234-signalforge-hook` (branch `feature/234-signalforge-hook`, merged up to `dev`) +- **Tasks (dependency-ordered):** + - `.1` US-001 — `PROVIDER_ENV_VAR_KEYS` shared table — deps: none + - `.2` US-002 — logger grep-gate → `signalforge.airflow` — deps: none + - `.3` US-003 — pure resolver + typed models (100% ungated) — deps: `.1` + - `.4` US-004 — real `SignalForgeHook` + `register_secret` shim (gated) — deps: `.3`, `.2` + - `.5` US-005 — wire `signalforge_conn_id` → GenerateOperator (+ shared helper) — deps: `.4` + - `.6` US-006 — wire `signalforge_conn_id` → PruneExistingOperator — deps: `.5` + - `.7` US-007 — `docs/airflow-ops.md` + example DAG (A8) — deps: `.6` + - `.8` US-008 — Quality Gate — deps: `.7` + - `.9` US-009 — Patterns & Memory — deps: `.8` +- **Ready at devolve:** `.1`, `.2`. From d93743b2f02ac6905371062e0e9965f3e51e4803 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 10:07:14 -0700 Subject: [PATCH 05/15] bd_1-scaffolding-qhi.2: extend logger grep-gate to signalforge.airflow --- tests/llm/test_logger_grep_gate.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/llm/test_logger_grep_gate.py b/tests/llm/test_logger_grep_gate.py index 7d0b180d..eda405b9 100644 --- a/tests/llm/test_logger_grep_gate.py +++ b/tests/llm/test_logger_grep_gate.py @@ -43,6 +43,7 @@ # Subpackages covered by the gate. Order is alphabetical for ease of # diffing when a future stage extends the list. _SCAN_SUBPACKAGES: tuple[str, ...] = ( + "airflow", "cli", "demo", "diff", From 77e8c832bf57dc411a3325bfa9e740ea3c2ba730 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 10:08:26 -0700 Subject: [PATCH 06/15] bd_1-scaffolding-qhi.1: add PROVIDER_ENV_VAR_KEYS shared table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/signalforge/llm/providers.py | 24 +++++++++++-- tests/llm/test_providers.py | 62 ++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 3 deletions(-) diff --git a/src/signalforge/llm/providers.py b/src/signalforge/llm/providers.py index 2ef25f85..a6fef7b4 100644 --- a/src/signalforge/llm/providers.py +++ b/src/signalforge/llm/providers.py @@ -374,11 +374,11 @@ def provider_for(name: str) -> LLMProvider: # --------------------------------------------------------------------------- -# Provider -> string mappings (#187 US-001). +# Provider -> string mappings (#187 US-001; #234 US-001). # -# Two read-only constants keyed by the canonical provider names registered +# Read-only constants keyed by the canonical provider names registered # below (``anthropic`` / ``openai`` / ``gemini``). They are the single -# source of truth for two cross-cutting facts that previously lived as +# source of truth for cross-cutting facts that previously lived as # duplicated literals scattered across stages: # # * ``PROVIDER_DEFAULT_MODELS`` — the cheap/fast judge SKU per provider, used by @@ -389,6 +389,10 @@ def provider_for(name: str) -> LLMProvider: # the cost-rollup's prefix dispatch (``signalforge.llm.cost._rollup``) to map # a priced SKU back to its provider. Consumers that need the inverse # (prefix -> provider) iterate ``.items()`` and invert. +# * ``PROVIDER_ENV_VAR_KEYS`` — the environment-variable name carrying each +# provider's API key (#234 US-001). The single source of truth for "which env +# var holds provider X's credential," reused by the Airflow hook (#234) and the +# v0.8 GitHub Action so neither hard-codes the per-provider env-var name. # # These are plain ``dict`` literals (not ``MappingProxyType``) for the same # reason the cost-rollup's prefix table is a plain tuple — they are internal @@ -417,6 +421,19 @@ def provider_for(name: str) -> LLMProvider: "gemini": "gemini-", } +#: Environment-variable name carrying each provider's API key (#234 US-001). +#: The single source of truth for "which env var holds provider X's credential," +#: reused by the Airflow hook (#234) and the v0.8 GitHub Action. A closed +#: allowlist keyed by exactly the three registered provider names — stays in +#: lockstep with :data:`PROVIDER_DEFAULT_MODELS`. Note Gemini's key is +#: ``GOOGLE_API_KEY`` (the ``google-genai`` SDK's convention), not a +#: ``GEMINI_*`` name. +PROVIDER_ENV_VAR_KEYS: dict[str, str] = { + "anthropic": "ANTHROPIC_API_KEY", + "openai": "OPENAI_API_KEY", + "gemini": "GOOGLE_API_KEY", +} + class AnthropicProvider(LLMProvider): """Anthropic strategy behind the generic LLM orchestrator (DEC-002/003/004). @@ -1567,6 +1584,7 @@ def estimate_input_tokens( __all__ = ( "PROVIDER_DEFAULT_MODELS", + "PROVIDER_ENV_VAR_KEYS", "PROVIDER_SKU_PREFIXES", "AnthropicProvider", "ExceptionCategory", diff --git a/tests/llm/test_providers.py b/tests/llm/test_providers.py index 0cef467a..b0e4d899 100644 --- a/tests/llm/test_providers.py +++ b/tests/llm/test_providers.py @@ -11,6 +11,7 @@ from __future__ import annotations +import re from typing import Any import anthropic @@ -20,6 +21,7 @@ from signalforge.llm.errors import UnknownProviderError from signalforge.llm.providers import ( PROVIDER_DEFAULT_MODELS, + PROVIDER_ENV_VAR_KEYS, PROVIDER_SKU_PREFIXES, AnthropicProvider, ExceptionCategory, @@ -1517,3 +1519,63 @@ def test_both_constants_are_exported() -> None: assert "PROVIDER_DEFAULT_MODELS" in providers_module.__all__ assert "PROVIDER_SKU_PREFIXES" in providers_module.__all__ + + +# --------------------------------------------------------------------------- +# #234 US-001 — PROVIDER_ENV_VAR_KEYS shared table +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +@pytest.mark.llm +def test_provider_env_var_keys_keys_are_the_three_registered_providers() -> None: + """``PROVIDER_ENV_VAR_KEYS`` is keyed by exactly the three registered + provider names (#234 US-001). Cross-checked against the live registry so a + future registry change forces an env-var-table update in lockstep.""" + assert set(PROVIDER_ENV_VAR_KEYS) == _REGISTERED_PROVIDER_NAMES + for name in PROVIDER_ENV_VAR_KEYS: + assert provider_for(name).name == name + + +@pytest.mark.unit +@pytest.mark.llm +def test_provider_env_var_keys_match_default_models_keys() -> None: + """``PROVIDER_ENV_VAR_KEYS`` and ``PROVIDER_DEFAULT_MODELS`` cover the same + provider names — the three providers stay in lockstep (#234 US-001).""" + assert set(PROVIDER_ENV_VAR_KEYS) == set(PROVIDER_DEFAULT_MODELS) + + +@pytest.mark.unit +@pytest.mark.llm +def test_provider_env_var_keys_values_are_upper_snake_env_names() -> None: + """Each value is a non-empty UPPER_SNAKE environment-variable name + (#234 US-001) — a lower-case or empty value would silently fail to read + the credential from the process environment.""" + for provider, env_var in PROVIDER_ENV_VAR_KEYS.items(): + assert env_var, f"{provider} env-var name is empty" + assert env_var.isupper(), f"{provider} env-var {env_var!r} is not upper-case" + assert re.fullmatch(r"[A-Z][A-Z0-9_]*", env_var), ( + f"{provider} env-var {env_var!r} is not UPPER_SNAKE" + ) + + +@pytest.mark.unit +@pytest.mark.llm +def test_provider_env_var_keys_values_match_expected() -> None: + """Pin the exact env-var names the Airflow hook (#234) and GH Action depend + on (#234 US-001). Gemini's key is ``GOOGLE_API_KEY`` (the ``google-genai`` + SDK convention), not a ``GEMINI_*`` name.""" + assert PROVIDER_ENV_VAR_KEYS == { + "anthropic": "ANTHROPIC_API_KEY", + "openai": "OPENAI_API_KEY", + "gemini": "GOOGLE_API_KEY", + } + + +@pytest.mark.unit +@pytest.mark.llm +def test_provider_env_var_keys_is_exported() -> None: + """The constant is part of the module's public surface (#234 US-001).""" + from signalforge.llm import providers as providers_module + + assert "PROVIDER_ENV_VAR_KEYS" in providers_module.__all__ From 0aa5ef8dcef16ff297b10c4eae350186c22504fe Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 10:08:57 -0700 Subject: [PATCH 07/15] bd_1-scaffolding-qhi.2: update logger grep-gate dir-set wording (orchestrator .claude edit) --- .claude/rules/cli-layer.md | 4 ++-- .claude/rules/diff-renderer.md | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.claude/rules/cli-layer.md b/.claude/rules/cli-layer.md index 7d5fd8f0..321a36a9 100644 --- a/.claude/rules/cli-layer.md +++ b/.claude/rules/cli-layer.md @@ -86,11 +86,11 @@ Every user-supplied path flows through `canonicalise_user_path(raw, project_dir) When introducing a new flag that takes a path, route it through `canonicalise_user_path` from the orchestrator. Don't trust the writer / loader to derive its own `project_dir`. -## Logger grep gate covers 6 dirs (DEC-019) +## Logger grep gate covers 11 dirs (DEC-019; `airflow` added by #234) Every `_LOGGER.{info,warning,debug,error}` call in `signalforge.cli.*` uses lazy-format with `json.dumps()` for any user-controlled string. Never f-string-interpolate — ANSI escapes in a model id or path would inject into log viewers; JSON encoding handles this; f-string interpolation does not. -The grep gate at `tests/llm/test_logger_grep_gate.py` scans `src/signalforge/{llm, draft, prune, grade, diff, cli}` and rejects any `_LOGGER\.\w+\(f"` hit. Extend to a seventh dir only when a new pipeline package ships. +The grep gate at `tests/llm/test_logger_grep_gate.py` scans the `_SCAN_SUBPACKAGES` set — `src/signalforge/{airflow, cli, demo, diff, draft, grade, llm, manifest, prune, safety, warehouse}` (11 dirs as of #234) — and rejects any `_LOGGER\.\w+\(f"` hit. Add the subpackage to `_SCAN_SUBPACKAGES` when a new package that emits logs ships (`airflow` was added by #234 ahead of the hook's logging). The CLI is the orchestration layer (NOT a stage-0 reader) so it IS allowed to emit logs. `setup_logging(verbose, quiet)` is the single config site: INFO default, `--verbose` → DEBUG, `--quiet` → WARNING. diff --git a/.claude/rules/diff-renderer.md b/.claude/rules/diff-renderer.md index ba6f1746..6b33e668 100644 --- a/.claude/rules/diff-renderer.md +++ b/.claude/rules/diff-renderer.md @@ -157,7 +157,7 @@ No separate WARNING before raising typed errors — the exception IS the signal ## ANSI-safe lazy-format JSON logger + grep gate (DEC-019) -Same rule as the other four pipeline layers (`safety-layer.md` DEC-022 / `llm-drafter.md` DEC-011 / `prune-engine.md` DEC-017 / `grade-layer.md` DEC-029). The grep gate at `tests/llm/test_logger_grep_gate.py` scans `src/signalforge/{llm,draft,prune,grade,diff,cli}` (6 dirs as of #9) and rejects any `_LOGGER\.\w+\(f"` hit. +Same rule as the other four pipeline layers (`safety-layer.md` DEC-022 / `llm-drafter.md` DEC-011 / `prune-engine.md` DEC-017 / `grade-layer.md` DEC-029). The grep gate at `tests/llm/test_logger_grep_gate.py` scans the `_SCAN_SUBPACKAGES` set — `src/signalforge/{airflow, cli, demo, diff, draft, grade, llm, manifest, prune, safety, warehouse}` (11 dirs as of #234) — and rejects any `_LOGGER\.\w+\(f"` hit. ## Fail-closed writer AST defence (DEC-018) From 3eb71a5e2e8471f2e757c84eb65d8ddd0bf45e89 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 10:16:10 -0700 Subject: [PATCH 08/15] bd_1-scaffolding-qhi.3: airflow-free hook resolver + typed models --- src/signalforge/airflow/_resolve.py | 227 +++++++++++++++++++++++++++ tests/airflow/test_resolve.py | 231 ++++++++++++++++++++++++++++ 2 files changed, 458 insertions(+) create mode 100644 src/signalforge/airflow/_resolve.py create mode 100644 tests/airflow/test_resolve.py diff --git a/src/signalforge/airflow/_resolve.py b/src/signalforge/airflow/_resolve.py new file mode 100644 index 00000000..d378db48 --- /dev/null +++ b/src/signalforge/airflow/_resolve.py @@ -0,0 +1,227 @@ +"""Airflow-free pure resolver for the SignalForge Airflow hook (#234). + +US-003 of issue #234 (epic #228, v0.7 Airflow operator roadmap). This module is +the **airflow-free heart** of :class:`signalforge.airflow.hooks.SignalForgeHook`: +it turns an Airflow ``Connection`` (plus an injected ``Variable`` lookup) into a +typed :class:`HookResolution` — *without importing apache-airflow at all*. + +That airflow-freedom is load-bearing (mirrors ``result.py`` / ``runner.py``): + +* The gated hook (US-004) subclasses ``BaseHook`` and is therefore deselected + from the default coverage run; this pure resolver carries **all** the + decision logic so it can be unit-tested 100% in the **default** pytest suite + (the codecov patch gate), with a tiny fake ``conn`` object and a dict-backed + ``variable_lookup`` — no airflow rig required. +* It keeps the one-shim rule intact (``.claude/rules/airflow-integration.md``): + the sole ``from airflow ...`` site stays ``_airflow_compat``. This module has + **none** — verified by ``grep`` and by the import-confinement AST scan. + +Design decisions (``plans/super/234-signalforge-hook.md``): + +* **DEC-003 — lenient resolver.** ``api_key``/``provider`` come back as ``None`` + when absent; *requiredness is enforced by the consumer* (the Generate operator + raises when the key is missing; PruneExisting needs neither). One resolver + serves both operators. +* **DEC-004 — pure + typed result.** ``resolve_connection`` is a module-level + pure function returning :class:`HookResolution`. +* **DEC-005 — closed provider allowlist.** A non-``None`` ``provider`` is + validated against :data:`signalforge.llm.providers.PROVIDER_ENV_VAR_KEYS`; an + unknown provider raises :class:`AirflowConfigError` (closing the + arbitrary-env-var injection vector even for a Connection that only drives the + no-LLM prune-existing path). +* **DEC-008 — symlink-hardened ``profiles_dir``.** When a containment anchor + (``project_dir``) is available the path is routed through + :func:`signalforge._common.path_safety.canonicalise_path`; otherwise it is + accepted as-is (the documented bounded-defence gap — mirrors the init-demo + seam, which also has no natural anchor). +* **DEC-009 — reuse :class:`AirflowConfigError` (tier 2).** Every misconfig + (extra typo, unknown provider, escaping ``profiles_dir``) raises that one + class with a case-specific remediation; no new error class. +* **DEC-010 — ``extra`` validated by an ``extra="forbid"`` Pydantic model.** A + typo key (``cache_scop``) fails loud at resolution. +""" + +from __future__ import annotations + +from collections.abc import Callable +from dataclasses import dataclass +from pathlib import Path +from typing import Protocol + +from pydantic import BaseModel, ConfigDict, ValidationError + +from signalforge._common.path_safety import PathContainmentError, canonicalise_path +from signalforge.airflow.errors import AirflowConfigError +from signalforge.llm.providers import PROVIDER_ENV_VAR_KEYS + +#: The Airflow Variable key consulted as the API-key fallback when the +#: Connection carries no ``password``. A single fixed, provider-agnostic name +#: keeps the convention trivial to document and works even when ``provider`` is +#: omitted (a prune-existing-only Connection). The gated hook (US-004) wires the +#: ``variable_lookup`` callable to ``Variable.get(key, default_var=None)``, so an +#: absent Variable yields ``None`` and the resolver stays lenient (DEC-003). +API_KEY_VARIABLE_KEY = "signalforge_api_key" + + +class _ConnectionLike(Protocol): + """The duck-typed slice of an Airflow ``Connection`` the resolver reads. + + Declared as a :class:`typing.Protocol` so the resolver needs no airflow + import: any object exposing ``password`` and ``extra_dejson`` satisfies it + (the real ``airflow.models.Connection`` does; so does a one-line test fake). + Airflow's ``extra_dejson`` returns ``{}`` for a null/blank ``extra`` and + never raises, so the resolver can read it unconditionally. + """ + + @property + def password(self) -> str | None: ... + + @property + def extra_dejson(self) -> dict[str, object]: ... + + +@dataclass(frozen=True, repr=False) +class HookResolution: + """The typed result of resolving a SignalForge Airflow Connection. + + Frozen and airflow-free. Carries the three things the operators need: + ``profiles_dir`` (warehouse auth), ``provider`` (which LLM SKU family), and + ``api_key`` (the credential). All three are ``None``-able — the resolver is + lenient (DEC-003); the consuming operator enforces requiredness. + + The custom :meth:`__repr__` is a **leak-surface discipline** (DEC-007): it + renders ``profiles_dir`` and ``provider`` only, and NEVER the ``api_key`` + value *or* a field-name label that would reveal a credential is present — + mirroring ``SnowflakeAdapter.__repr__`` (which shows only ``account`` + + ``warehouse``) and ``DbtProfileTarget``'s ``Field(repr=False)`` secrets. A + debug-print / ``%r`` log line therefore cannot leak the key. + """ + + profiles_dir: str | None + provider: str | None + api_key: str | None + + def __repr__(self) -> str: + # Deliberately omits ``api_key`` entirely — no value, no label. + return f"HookResolution(profiles_dir={self.profiles_dir!r}, provider={self.provider!r})" + + +class _ConnectionExtra(BaseModel): + """Typed schema for an Airflow Connection's ``extra`` JSON (DEC-010). + + ``extra="forbid"`` so a typo key (``cache_scop`` for ``cache_scope``) fails + loud at resolution rather than silently no-op'ing — the same fail-loud + posture every other user-input config model in the project takes + (``.claude/rules/safety-layer.md`` § ``extra="forbid"``). ``frozen=True`` + because the parsed extra is read-only once validated. Every field is + optional: a prune-existing-only Connection legitimately omits ``provider``, + and a Connection relying on the ambient ``DBT_PROFILES_DIR`` omits + ``profiles_dir``. + """ + + model_config = ConfigDict(frozen=True, extra="forbid") + + profiles_dir: str | None = None + provider: str | None = None + cache_scope: str | None = None + + +def resolve_connection( + conn: _ConnectionLike, + *, + variable_lookup: Callable[[str], str | None], + project_dir: str | Path | None = None, +) -> HookResolution: + """Resolve an Airflow Connection (+ Variable lookup) to a :class:`HookResolution`. + + Pure and airflow-free. ``conn`` is duck-typed (``.password`` + + ``.extra_dejson``); ``variable_lookup`` is injected by the caller (the gated + hook wires it to ``Variable.get(key, default_var=None)``) so this function + never touches ``airflow.models.Variable`` directly. + + Resolution rules: + + * **extra** — parsed through :class:`_ConnectionExtra`; a typo / unknown key + raises :class:`AirflowConfigError` (DEC-010). + * **api_key** — primary source is ``conn.password``; when that is falsy the + fallback is ``variable_lookup(API_KEY_VARIABLE_KEY)``. The resolver is + **lenient**: if neither yields a key, ``api_key`` is ``None`` and no error + is raised (DEC-003) — the consumer enforces requiredness. + * **provider** — read from the validated ``extra``; may be ``None``. When + present it MUST be a key of + :data:`~signalforge.llm.providers.PROVIDER_ENV_VAR_KEYS`, else + :class:`AirflowConfigError` (the closed allowlist, DEC-005). The check + runs regardless of consumer, so an arbitrary env-var name can never be + derived from operator-supplied config. + * **profiles_dir** — read from the validated ``extra``; may be ``None``. When + present *and* a ``project_dir`` anchor is supplied, it is symlink-hardened + via :func:`canonicalise_path`; a containment escape (or a bare ``OSError`` + from resolution) raises :class:`AirflowConfigError` (DEC-008). When no + ``project_dir`` is given the path is accepted as-is (the documented + bounded-defence gap — there is no anchor to contain against). + + Raises: + AirflowConfigError: on any misconfiguration (DEC-009 — the one reused + tier-2 class; no new error type). + """ + extra_dict = conn.extra_dejson + try: + extra = _ConnectionExtra.model_validate(extra_dict) + except ValidationError as exc: + raise AirflowConfigError( + "The SignalForge Connection `extra` JSON is invalid.", + remediation=( + "The Connection `extra` accepts only `profiles_dir`, `provider`, " + "and `cache_scope` (all optional). Remove any unknown key (a typo " + "like `cache_scop` fails loud) and ensure each value is a string. " + f"Validation error: {exc}" + ), + ) from exc + + # provider — closed allowlist when present (DEC-005). + provider = extra.provider + if provider is not None and provider not in PROVIDER_ENV_VAR_KEYS: + allowed = ", ".join(sorted(PROVIDER_ENV_VAR_KEYS)) + raise AirflowConfigError( + f"The SignalForge Connection `extra.provider` is not a known provider: {provider!r}.", + remediation=( + f"Set `extra.provider` to one of: {allowed}. Leave it unset for a " + "prune-existing-only Connection that makes no LLM call." + ), + ) + + # api_key — password primary, Variable fallback; lenient (DEC-003). + api_key = conn.password or None + if api_key is None: + api_key = variable_lookup(API_KEY_VARIABLE_KEY) or None + + # profiles_dir — symlink-hardened when an anchor is available (DEC-008). + profiles_dir = extra.profiles_dir + if profiles_dir is not None and project_dir is not None: + try: + resolved = canonicalise_path(profiles_dir, Path(project_dir)) + except (PathContainmentError, OSError) as exc: + raise AirflowConfigError( + "The SignalForge Connection `extra.profiles_dir` could not be " + "resolved safely inside the project directory.", + remediation=( + "`extra.profiles_dir` must resolve to a path inside the " + "operator's `project_dir` (symlink-hardened). Point it at a " + "directory containing your dbt `profiles.yml`. " + f"Resolution error: {exc}" + ), + ) from exc + profiles_dir = str(resolved) + + return HookResolution( + profiles_dir=profiles_dir, + provider=provider, + api_key=api_key, + ) + + +__all__ = [ + "API_KEY_VARIABLE_KEY", + "HookResolution", + "resolve_connection", +] diff --git a/tests/airflow/test_resolve.py b/tests/airflow/test_resolve.py new file mode 100644 index 00000000..d6b095eb --- /dev/null +++ b/tests/airflow/test_resolve.py @@ -0,0 +1,231 @@ +"""Tests for ``signalforge.airflow._resolve`` (issue #234 / US-003). + +These tests import ONLY the airflow-free pure resolver — never the real +``apache-airflow`` package — so they run in the **default** pytest suite (NO +``airflow`` marker, NO ``importorskip``). They drive the resolver with a tiny +fake ``conn`` object (``.password`` + ``.extra_dejson``) and a dict-backed +``variable_lookup``, and cover EVERY branch of ``_resolve.py`` for the codecov +patch gate: + +* provider + key from ``password``; +* Variable fallback when ``password`` is absent / blank; +* both absent → ``api_key is None`` (lenient — no raise, DEC-003); +* unknown provider → :class:`AirflowConfigError` (closed allowlist, DEC-005); +* ``extra`` typo key → :class:`AirflowConfigError` (``extra="forbid"``, DEC-010); +* ``profiles_dir`` accepted when ``project_dir`` is ``None`` (bounded gap); +* ``profiles_dir`` canonicalised inside ``project_dir`` (happy path); +* ``profiles_dir`` escaping ``project_dir`` → :class:`AirflowConfigError` (DEC-008); +* ``__repr__`` omits the key value AND any field-name label revealing it; +* :class:`HookResolution` is frozen. +""" + +from __future__ import annotations + +import dataclasses +from pathlib import Path + +import pytest +from pydantic import ValidationError + +from signalforge.airflow._resolve import ( + API_KEY_VARIABLE_KEY, + HookResolution, + _ConnectionExtra, + resolve_connection, +) +from signalforge.airflow.errors import AirflowConfigError + + +class _FakeConn: + """Duck-typed Airflow ``Connection`` slice the resolver reads.""" + + def __init__( + self, *, password: str | None = None, extra_dejson: dict[str, object] | None = None + ) -> None: + self._password = password + self._extra_dejson = extra_dejson if extra_dejson is not None else {} + + @property + def password(self) -> str | None: + return self._password + + @property + def extra_dejson(self) -> dict[str, object]: + return self._extra_dejson + + +def _no_variable(_key: str) -> str | None: + """A ``variable_lookup`` that always reports the Variable absent.""" + return None + + +def _dict_variable(mapping: dict[str, str | None]): + """Build a dict-backed ``variable_lookup`` (mirrors ``Variable.get(default_var=None)``).""" + + def _lookup(key: str) -> str | None: + return mapping.get(key) + + return _lookup + + +# --------------------------------------------------------------------------- +# api_key resolution (DEC-003) +# --------------------------------------------------------------------------- + + +def test_provider_and_key_from_password() -> None: + """``password`` is the primary key source; ``provider`` from ``extra``.""" + conn = _FakeConn(password="sk-secret-123", extra_dejson={"provider": "anthropic"}) + res = resolve_connection(conn, variable_lookup=_no_variable) + assert res.api_key == "sk-secret-123" + assert res.provider == "anthropic" + assert res.profiles_dir is None + + +def test_variable_fallback_when_no_password() -> None: + """When ``password`` is absent, the Variable lookup supplies the key under + the documented :data:`API_KEY_VARIABLE_KEY`.""" + conn = _FakeConn(password=None, extra_dejson={"provider": "openai"}) + lookup = _dict_variable({API_KEY_VARIABLE_KEY: "sk-from-variable"}) + res = resolve_connection(conn, variable_lookup=lookup) + assert res.api_key == "sk-from-variable" + assert res.provider == "openai" + + +def test_blank_password_falls_back_to_variable() -> None: + """An empty-string ``password`` is treated as absent (falls back).""" + conn = _FakeConn(password="", extra_dejson={}) + lookup = _dict_variable({API_KEY_VARIABLE_KEY: "sk-fallback"}) + res = resolve_connection(conn, variable_lookup=lookup) + assert res.api_key == "sk-fallback" + + +def test_both_absent_yields_none_and_does_not_raise() -> None: + """Lenient (DEC-003): neither source has a key → ``api_key is None``, no raise.""" + conn = _FakeConn(password=None, extra_dejson={}) + res = resolve_connection(conn, variable_lookup=_no_variable) + assert res.api_key is None + assert res.provider is None + assert res.profiles_dir is None + + +def test_blank_variable_value_collapses_to_none() -> None: + """A blank Variable value collapses to ``None`` (not an empty string).""" + conn = _FakeConn(password=None, extra_dejson={}) + lookup = _dict_variable({API_KEY_VARIABLE_KEY: ""}) + res = resolve_connection(conn, variable_lookup=lookup) + assert res.api_key is None + + +# --------------------------------------------------------------------------- +# provider allowlist (DEC-005) +# --------------------------------------------------------------------------- + + +def test_unknown_provider_raises() -> None: + """A non-``None`` provider outside the closed allowlist fails loud.""" + conn = _FakeConn(password="k", extra_dejson={"provider": "definitely-not-a-provider"}) + with pytest.raises(AirflowConfigError) as excinfo: + resolve_connection(conn, variable_lookup=_no_variable) + rendered = str(excinfo.value) + assert "provider" in rendered + # Remediation lists the allowed providers. + assert "anthropic" in rendered + + +def test_absent_provider_is_left_none() -> None: + """When ``provider`` is omitted it stays ``None`` (prune-existing path).""" + conn = _FakeConn(password="k", extra_dejson={"profiles_dir": None}) + res = resolve_connection(conn, variable_lookup=_no_variable) + assert res.provider is None + + +# --------------------------------------------------------------------------- +# extra="forbid" (DEC-010) +# --------------------------------------------------------------------------- + + +def test_extra_typo_key_raises() -> None: + """A typo key (``cache_scop``) fails loud at validation.""" + conn = _FakeConn(password="k", extra_dejson={"cache_scop": "project"}) + with pytest.raises(AirflowConfigError) as excinfo: + resolve_connection(conn, variable_lookup=_no_variable) + assert "extra" in str(excinfo.value).lower() + + +def test_extra_accepts_all_three_known_keys() -> None: + """The full known-key set validates and round-trips.""" + extra = _ConnectionExtra.model_validate( + {"profiles_dir": "/p", "provider": "gemini", "cache_scope": "project"} + ) + assert extra.profiles_dir == "/p" + assert extra.provider == "gemini" + assert extra.cache_scope == "project" + + +def test_connection_extra_is_frozen() -> None: + """``_ConnectionExtra`` is frozen (read-only once validated).""" + extra = _ConnectionExtra() + with pytest.raises(ValidationError): # frozen mutation + extra.provider = "anthropic" # type: ignore[misc] + + +# --------------------------------------------------------------------------- +# profiles_dir (DEC-008) +# --------------------------------------------------------------------------- + + +def test_profiles_dir_accepted_when_project_dir_none() -> None: + """No anchor → path accepted as-is (the documented bounded-defence gap).""" + conn = _FakeConn(password="k", extra_dejson={"profiles_dir": "/some/profiles"}) + res = resolve_connection(conn, variable_lookup=_no_variable, project_dir=None) + assert res.profiles_dir == "/some/profiles" + + +def test_profiles_dir_canonicalised_inside_project_dir(tmp_path: Path) -> None: + """A path inside the project tree canonicalises to its absolute resolved form.""" + project = tmp_path / "proj" + profiles = project / "dbt" + profiles.mkdir(parents=True) + conn = _FakeConn(password="k", extra_dejson={"profiles_dir": "dbt"}) + res = resolve_connection(conn, variable_lookup=_no_variable, project_dir=project) + assert res.profiles_dir == str(profiles.resolve()) + + +def test_profiles_dir_escape_raises(tmp_path: Path) -> None: + """A ``profiles_dir`` escaping the project tree fails loud (DEC-008).""" + project = tmp_path / "proj" + project.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + conn = _FakeConn(password="k", extra_dejson={"profiles_dir": str(outside)}) + with pytest.raises(AirflowConfigError) as excinfo: + resolve_connection(conn, variable_lookup=_no_variable, project_dir=project) + assert "profiles_dir" in str(excinfo.value) + + +# --------------------------------------------------------------------------- +# HookResolution leak-surface discipline + immutability (DEC-007) +# --------------------------------------------------------------------------- + + +def test_repr_omits_api_key_value_and_label() -> None: + """``__repr__`` shows ``profiles_dir`` + ``provider`` ONLY — never the key + value, never a field-name label that would reveal a credential is present.""" + res = HookResolution(profiles_dir="/p", provider="anthropic", api_key="sk-super-secret-VALUE") + rendered = repr(res) + # Safe fields appear. + assert "/p" in rendered + assert "anthropic" in rendered + # The key value must NOT leak. + assert "sk-super-secret-VALUE" not in rendered + # No field-name label revealing a credential. + assert "api_key" not in rendered + assert "key" not in rendered + + +def test_hook_resolution_is_frozen() -> None: + """``HookResolution`` is a frozen dataclass.""" + res = HookResolution(profiles_dir=None, provider=None, api_key=None) + with pytest.raises(dataclasses.FrozenInstanceError): + res.api_key = "leak" # type: ignore[misc] From 4518f2ad9cff2aef6e2963e4544e88a5658eee3a Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 10:52:00 -0700 Subject: [PATCH 09/15] bd_1-scaffolding-qhi.4: real SignalForgeHook(BaseHook) + register_secret shim --- src/signalforge/airflow/_airflow_compat.py | 57 ++++++ src/signalforge/airflow/hooks.py | 175 ++++++++++++++--- tests/airflow/test_hooks.py | 216 +++++++++++++++++++++ tests/airflow/test_skeleton.py | 41 +++- 4 files changed, 459 insertions(+), 30 deletions(-) create mode 100644 tests/airflow/test_hooks.py diff --git a/src/signalforge/airflow/_airflow_compat.py b/src/signalforge/airflow/_airflow_compat.py index 3a68f113..1bf1bf18 100644 --- a/src/signalforge/airflow/_airflow_compat.py +++ b/src/signalforge/airflow/_airflow_compat.py @@ -36,6 +36,13 @@ success). Its ``from airflow.exceptions import ...`` is confined to the function body for the same reason as the factories' imports. +* :func:`register_secret` / :func:`airflow_variable_get` — the hook's two + airflow touch-points (#234, DEC-004 / DEC-006). ``register_secret`` registers a + resolved API key with Airflow's log secrets masker (so it redacts to ``***``); + ``airflow_variable_get`` reads an Airflow Variable as the API-key fallback, + coerced to ``str | None``. Both confine their ``from airflow ...`` import to the + function body, like the factories above. + Observability discipline (mirroring the warehouse/LLM shims): no logger calls in this shim. Logging lives in the implementing operator/hook where the task context is known. The shim itself is structural plumbing. @@ -163,13 +170,63 @@ def raise_for_outcome( # pragma: no cover - requires the [airflow] extra raise AirflowException(message) +def register_secret(value: str) -> None: # pragma: no cover - requires the [airflow] extra + """Register ``value`` with Apache Airflow's log secrets masker (#234, DEC-006). + + Belt-and-braces over Airflow's automatic masking of a Connection's + ``password`` / sensitive ``extra`` keys: the implementing operator calls this + immediately after resolving the SignalForge Connection — *before* any logging + or ``run_signalforge`` call — so the resolved LLM API key is redacted to + ``***`` everywhere Airflow's :class:`~airflow.utils.log.secrets_masker.SecretsMasker` + filter runs (task logs, tracebacks). One of the four leak-surface disciplines + (DEC-007); the others are ``signalforge_conn_id`` staying out of + ``template_fields`` / XCom and the redacting ``__repr__`` on the hook + + :class:`~signalforge.airflow._resolve.HookResolution`. + + The ``from airflow ... import mask_secret`` is confined to this function body + (DEC-007 — the one-shim-per-vendor rule); the single airflow ``# type: ignore`` + for the import lives here. ``mask_secret`` is the stable public entry point + across ``apache-airflow>=2.8,<3``. + """ + from airflow.utils.log.secrets_masker import mask_secret # type: ignore[import-not-found] + + mask_secret(value) + + +def airflow_variable_get(key: str) -> str | None: # pragma: no cover - requires the [airflow] extra + """Read an Apache Airflow Variable, returning ``None`` when absent (#234, DEC-004). + + The implementing hook wires this as the ``variable_lookup`` callable handed to + the airflow-free :func:`signalforge.airflow._resolve.resolve_connection`, so + the resolver never imports ``airflow.models.Variable`` directly (it stays + pure + unit-testable with a dict-backed lookup). ``Variable.get(key, + default_var=None)`` yields ``None`` for an absent Variable rather than + raising, keeping the resolver lenient (DEC-003). + + ``Variable.get`` is typed ``Any`` by Airflow, so the result is coerced to an + explicit ``str | None`` here (a non-string Variable — e.g. a JSON-deserialised + dict — resolves to ``None``) rather than letting the type widen to ``object`` + downstream. + + The ``from airflow.models import Variable`` is confined to this function body + (DEC-007 — the one-shim-per-vendor rule); the single airflow ``# type: ignore`` + for the import lives here. + """ + from airflow.models import Variable # type: ignore[import-not-found] + + value = Variable.get(key, default_var=None) + return value if isinstance(value, str) else None + + # Only the factory functions are public API. The ``_Base*Protocol`` types stay # ``_``-prefixed internals (still directly importable for tests / implementing # children) and are deliberately NOT listed in ``__all__`` — per the convention # that a subpackage's public contract is its ``__all__`` and ``_``-prefixed names # are internal. __all__ = [ + "airflow_variable_get", "make_base_hook", "make_base_operator", "raise_for_outcome", + "register_secret", ] diff --git a/src/signalforge/airflow/hooks.py b/src/signalforge/airflow/hooks.py index 81202b95..6dbdeebd 100644 --- a/src/signalforge/airflow/hooks.py +++ b/src/signalforge/airflow/hooks.py @@ -1,39 +1,164 @@ -"""Placeholder SignalForge Airflow hook — skeleton only (#230 US-002). - -This module defines the stub :class:`SignalForgeHook`. Like the operator stub -(:mod:`signalforge.airflow.operators`), it is a plain class that deliberately -does NOT subclass Apache Airflow's ``BaseHook`` at module scope — a module-scope -subclass would force an eager ``from airflow ...`` import and break the -no-eager-import gate (DEC-004 / DEC-006). Keeping the stub Airflow-free lets -``import signalforge.airflow.hooks`` (and the lazy -``signalforge.airflow.SignalForgeHook`` re-export) stay free of ``airflow`` in -``sys.modules``. - -The real hook — which DOES subclass ``BaseHook`` — lands with an implementing -child of epic #228, built at runtime via the lazy factory -:func:`signalforge.airflow._airflow_compat.make_base_hook`. +"""The SignalForge Apache Airflow hook (#234 US-004). + +:class:`SignalForgeHook` turns an Airflow Connection (plus an Airflow Variable +fallback) into a typed :class:`~signalforge.airflow._resolve.HookResolution` — +``profiles_dir`` (warehouse auth), ``provider`` (LLM SKU family), and +``api_key`` (the credential). It subclasses Apache Airflow's ``BaseHook`` so +``get_conn`` can call ``self.get_connection(conn_id)`` directly; all the actual +decision logic lives in the airflow-free pure resolver +:func:`signalforge.airflow._resolve.resolve_connection` (US-003), so it is +unit-tested 100% in the default suite while the airflow-touching wrapper here is +gated. + +**Deferred class construction (the load-bearing structural constraint).** The +real hook must subclass ``BaseHook``, which requires ``airflow`` at runtime — but +importing THIS module must stay Airflow-free so the ungated no-eager-import / +import-confinement gates keep passing with Airflow absent (DEC-004 / DEC-006 / +DEC-007). So there is NO module-scope ``class X(BaseHook)``. Mirroring +:mod:`signalforge.airflow.operators` exactly, a PEP 562 module-level +:func:`__getattr__` resolves the ``SignalForgeHook`` name lazily via +:func:`_get_signalforge_hook_class`: + +* When Apache Airflow is installed, the ``functools.cache``'d factory + :func:`_make_signalforge_hook_class` builds the real ``BaseHook`` subclass at + access time — the ``from airflow ...`` import stays confined to the one shim + (:func:`signalforge.airflow._airflow_compat.make_base_hook`), out of module + scope. +* When Airflow is NOT installed, the name resolves (without importing airflow — + via :func:`importlib.util.find_spec`, which does not execute the module) to the + airflow-free placeholder :class:`_SignalForgeHookAirflowMissing`, whose + construction raises :class:`ModuleNotFoundError`. Attribute ACCESS is + airflow-free; only CONSTRUCTION of the real hook needs Airflow. + +Secrets hygiene: the hook never stores the resolved ``api_key`` on ``self`` and +never logs it; the redacting :meth:`__repr__` shows only ``signalforge_conn_id`` +(one of the four leak-surface disciplines, DEC-007 — mirrors +:meth:`HookResolution.__repr__` and ``SnowflakeAdapter.__repr__``). """ from __future__ import annotations -from typing import Any +import functools +import importlib.util +from typing import TYPE_CHECKING, Any + +from signalforge.airflow import _airflow_compat +from signalforge.airflow._resolve import HookResolution, resolve_connection + +if TYPE_CHECKING: + # Type-checker-only declaration of the hook name. At runtime the class is + # built by the find_spec-guarded factory below (it subclasses Apache + # Airflow's ``BaseHook``, which is NOT a typecheck dependency), and the + # public name is resolved through the module-level :func:`__getattr__`. This + # block exists so ``signalforge.airflow.__init__``'s ``TYPE_CHECKING`` + # re-export of the name resolves under pyright. + class SignalForgeHook: # noqa: D401 - type stub only + def __init__(self, *args: Any, **kwargs: Any) -> None: ... + + def get_conn(self) -> HookResolution: ... -class SignalForgeHook: - """Stub for the future SignalForge Airflow hook. +class _SignalForgeHookAirflowMissing: + """Stand-in for :class:`SignalForgeHook` when Apache Airflow is absent. - Skeleton placeholder (#230). Constructing it raises - :class:`NotImplementedError` — the hook behaviour (a real ``BaseHook`` - subclass built via - :func:`signalforge.airflow._airflow_compat.make_base_hook`) lands with a - later epic-#228 child. + Resolving ``signalforge.airflow.SignalForgeHook`` without the ``[airflow]`` + optional extra installed returns THIS class (attribute access stays + airflow-free, keeping the no-eager-import gate green). Constructing it raises + :class:`ModuleNotFoundError` (an :class:`ImportError` subclass) naming the + remediation — the real hook subclasses ``BaseHook`` and so genuinely requires + Airflow at construction time. """ def __init__(self, *_args: Any, **_kwargs: Any) -> None: - raise NotImplementedError( - "SignalForgeHook is a skeleton placeholder; the hook lands in a later " - "epic #228 child. #230 ships the package skeleton only." + raise ModuleNotFoundError( + "SignalForgeHook requires Apache Airflow, which is not installed. " + "Install the optional extra: pip install 'signalforge-dbt[airflow]'." ) +def _make_signalforge_hook_class() -> type: # pragma: no cover - requires the [airflow] extra + """Build and return the real ``BaseHook``-subclassing hook class. + + Reached only when Apache Airflow is installed (guarded by + :func:`_get_signalforge_hook_class`'s ``find_spec`` check). The base class is + obtained via the one shim + :func:`signalforge.airflow._airflow_compat.make_base_hook`, so the + ``from airflow ...`` import stays confined there and out of this module's + scope (DEC-004 / DEC-007). Marked ``# pragma: no cover`` because its body — + the ``BaseHook`` subclass and its ``get_conn`` — runs only under the gated + ``[airflow]`` extra (the gated ``tests/airflow/test_hooks.py`` exercise it); + the default coverage env never installs Airflow. + """ + _Base = _airflow_compat.make_base_hook() + + class SignalForgeHook(_Base): # type: ignore[valid-type, misc] + """Resolve a SignalForge Airflow Connection to a :class:`HookResolution`. + + Subclasses Apache Airflow's ``BaseHook`` so :meth:`get_conn` can call + ``self.get_connection(conn_id)`` directly. The resolution itself is done + by the airflow-free pure + :func:`signalforge.airflow._resolve.resolve_connection` (US-003), which + reads the Connection's ``password`` (falling back to an Airflow Variable) + for the API key, and ``provider`` / ``profiles_dir`` from the Connection + ``extra`` JSON. + """ + + def __init__(self, signalforge_conn_id: str, **kwargs: Any) -> None: + # ``BaseHook`` owns the standard hook kwargs (e.g. ``logger_name``); + # pass them through. + super().__init__(**kwargs) + self.signalforge_conn_id = signalforge_conn_id + + def get_conn(self) -> HookResolution: + """Resolve the configured Connection to a :class:`HookResolution`. + + ``project_dir`` is ``None`` at the hook layer: the hook has no + project anchor to symlink-contain an ``extra.profiles_dir`` against + (DEC-008's bounded-defence gap). The consuming operator — which DOES + know its ``project_dir`` — supplies the containment anchor when it + calls the resolver itself (US-005/US-006). + """ + conn = self.get_connection(self.signalforge_conn_id) + variable_lookup = _airflow_compat.airflow_variable_get + return resolve_connection(conn, variable_lookup=variable_lookup, project_dir=None) + + def __repr__(self) -> str: + # Leak-surface discipline (DEC-007): show only the conn id, never the + # resolved key (which is not stored on ``self``) or any field-name + # label that would reveal a credential is present. + return f"SignalForgeHook(signalforge_conn_id={self.signalforge_conn_id!r})" + + return SignalForgeHook + + +@functools.cache +def _get_signalforge_hook_class() -> type: + """Resolve (and cache) the hook class without importing airflow eagerly. + + Mirrors :func:`signalforge.airflow.operators._get_generate_operator_class`. + Attribute ACCESS must stay airflow-free so the no-eager-import gate passes + with the ``[airflow]`` extra absent. :func:`importlib.util.find_spec` checks + Airflow availability WITHOUT executing/importing it (it does not add + ``airflow`` to ``sys.modules``). When Airflow is present, build the real + ``BaseHook`` subclass; when absent, return the airflow-free placeholder whose + construction raises :class:`ModuleNotFoundError`. + """ + if importlib.util.find_spec("airflow") is None: + return _SignalForgeHookAirflowMissing + return _make_signalforge_hook_class() # pragma: no cover - requires [airflow] + + +def __getattr__(name: str) -> object: + """PEP 562 lazy resolution of the public ``SignalForgeHook`` name. + + Building a real ``BaseHook`` subclass at module scope would force an eager + ``from airflow ...`` import; resolving the name here (via the find_spec-guarded + getter) keeps attribute access airflow-free while still yielding the real hook + when Airflow is installed. + """ + if name == "SignalForgeHook": + return _get_signalforge_hook_class() + raise AttributeError(f"module {__name__!r} has no attribute {name!r}") + + __all__ = ["SignalForgeHook"] diff --git a/tests/airflow/test_hooks.py b/tests/airflow/test_hooks.py new file mode 100644 index 00000000..47d92673 --- /dev/null +++ b/tests/airflow/test_hooks.py @@ -0,0 +1,216 @@ +"""Gated tests for :class:`SignalForgeHook.get_conn` (#234 US-004). + +Belt-and-suspenders gating per ``testing-signal.md`` and the +``tests/airflow/test_operators.py`` precedent: + +1. ``pytestmark = pytest.mark.airflow`` — every test is deselected by the default + ``addopts`` ``-m '... and not airflow'`` so a plain ``uv run pytest`` never + imports Apache Airflow. +2. A runtime ``pytest.importorskip("airflow")`` as the FIRST line of each test + (NOT a module-scope import — that would run at collection time even when + deselected, and this module IS collected before deselection). The skip carries + a clear reason when a maintainer runs ``-m airflow`` without Airflow installed. + +Run inside the constraints-pinned Airflow venv (see +docs/research/airflow-test-environment.md):: + + SF_RUN_AIRFLOW=1 PYTHONPATH="$PWD/src" \ + /path/to/.venv-airflow/bin/python -m pytest tests/airflow -m airflow --no-cov + +These pin the hook's ``get_conn`` contract: it delegates to the airflow-free +:func:`signalforge.airflow._resolve.resolve_connection` against a real +``airflow.models.Connection`` (the hook's ``get_connection`` is monkeypatched to +return it, so no Airflow metastore / DB is needed), the Airflow Variable +fallback, the ``AirflowConfigError`` misconfig paths, the secrets-hygiene +guarantees (no raw key logged; ``register_secret`` redacts to ``***``), and the +redacting ``__repr__``. +""" + +from __future__ import annotations + +import importlib +import json +import logging + +import pytest + +from signalforge.airflow import _airflow_compat +from signalforge.airflow._resolve import API_KEY_VARIABLE_KEY, HookResolution +from signalforge.airflow.errors import AirflowConfigError + +pytestmark = pytest.mark.airflow + +_AIRFLOW_SKIP = "Apache Airflow not installed (run inside the constraints-pinned airflow venv)" + +_SECRET = "sk-super-secret-value-42" + + +def _hook_class() -> type: + """Resolve the real hook class (airflow present — built by the factory).""" + hooks = importlib.import_module("signalforge.airflow.hooks") + return hooks.SignalForgeHook + + +def _make_connection( + *, password: str | None = None, extra: dict[str, object] | None = None +) -> object: + """Build a real ``airflow.models.Connection`` with the given password/extra.""" + from airflow.models import Connection + + return Connection( + conn_id="signalforge_default", + conn_type="generic", + password=password, + extra=json.dumps(extra) if extra is not None else None, + ) + + +def _hook_with_conn(monkeypatch: pytest.MonkeyPatch, conn: object) -> object: + """Build a hook whose ``get_connection`` returns ``conn`` (no metastore).""" + hook = _hook_class()(signalforge_conn_id="signalforge_default") + # Instance attribute shadows the inherited ``BaseHook.get_connection``; the + # hook calls ``self.get_connection(conn_id)``, so a plain ``lambda conn_id`` + # is the right shape (no bound ``self``). + monkeypatch.setattr(hook, "get_connection", lambda conn_id: conn) + return hook + + +# --------------------------------------------------------------------------- # +# get_conn resolution +# --------------------------------------------------------------------------- # + + +def test_get_conn_happy_provider_and_key_from_password(monkeypatch: pytest.MonkeyPatch) -> None: + """``password`` → ``api_key``; ``extra.provider`` → ``provider``.""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + + conn = _make_connection(password=_SECRET, extra={"provider": "anthropic"}) + hook = _hook_with_conn(monkeypatch, conn) + + res = hook.get_conn() + + assert isinstance(res, HookResolution) + assert res.provider == "anthropic" + assert res.api_key == _SECRET + + +def test_get_conn_variable_fallback_when_no_password(monkeypatch: pytest.MonkeyPatch) -> None: + """No ``password`` → the API key comes from the Airflow Variable lookup. + + The hook wires ``variable_lookup`` to ``_airflow_compat.airflow_variable_get`` + via dotted access at call time, so monkeypatching that shim attribute swaps + the Variable source without an Airflow metastore. + """ + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + + def _fake_variable_get(key: str) -> str | None: + return _SECRET if key == API_KEY_VARIABLE_KEY else None + + monkeypatch.setattr(_airflow_compat, "airflow_variable_get", _fake_variable_get) + + conn = _make_connection(password=None, extra={"provider": "openai"}) + hook = _hook_with_conn(monkeypatch, conn) + + res = hook.get_conn() + + assert res.api_key == _SECRET + assert res.provider == "openai" + + +def test_get_conn_unknown_provider_raises(monkeypatch: pytest.MonkeyPatch) -> None: + """An ``extra.provider`` outside the closed allowlist → ``AirflowConfigError``.""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + + conn = _make_connection(password=_SECRET, extra={"provider": "bogus-llm"}) + hook = _hook_with_conn(monkeypatch, conn) + + with pytest.raises(AirflowConfigError): + hook.get_conn() + + +def test_get_conn_invalid_extra_key_raises(monkeypatch: pytest.MonkeyPatch) -> None: + """A typo / unknown ``extra`` key → ``AirflowConfigError`` (``extra="forbid"``). + + Covers the "misconfigured Connection surfaces a tier-2 ``AirflowConfigError`` + at the hook seam" path — the resolver raises and ``get_conn`` propagates it + unchanged. + """ + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + + conn = _make_connection(password=_SECRET, extra={"cache_scop": "project"}) + hook = _hook_with_conn(monkeypatch, conn) + + with pytest.raises(AirflowConfigError): + hook.get_conn() + + +def test_get_conn_lenient_when_key_absent(monkeypatch: pytest.MonkeyPatch) -> None: + """Neither ``password`` nor a Variable → ``api_key is None`` (lenient, DEC-003). + + The hook does NOT enforce key-requiredness — the consuming operator does. So + a prune-existing-only Connection (no LLM key) resolves cleanly. + """ + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + + def _no_variable(_key: str) -> str | None: + return None + + monkeypatch.setattr(_airflow_compat, "airflow_variable_get", _no_variable) + + conn = _make_connection(password=None, extra={}) + hook = _hook_with_conn(monkeypatch, conn) + + res = hook.get_conn() + + assert res.api_key is None + assert res.provider is None + + +# --------------------------------------------------------------------------- # +# Secrets hygiene (DEC-006 / DEC-007) +# --------------------------------------------------------------------------- # + + +def test_get_conn_never_logs_raw_key( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """Resolving a Connection must not emit the raw API key into any log record.""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + + conn = _make_connection(password=_SECRET, extra={"provider": "anthropic"}) + hook = _hook_with_conn(monkeypatch, conn) + + with caplog.at_level(logging.DEBUG): + res = hook.get_conn() + + assert res.api_key == _SECRET # resolution succeeded … + assert _SECRET not in caplog.text # … but the key never hit the logs. + + +def test_register_secret_masks_value_in_logs() -> None: + """``_airflow_compat.register_secret`` registers the value with Airflow's + secrets masker so it redacts to ``***`` (belt-and-braces, DEC-006).""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + + from airflow.utils.log.secrets_masker import _secrets_masker + + _airflow_compat.register_secret(_SECRET) + + redacted = _secrets_masker().redact(f"the resolved key is {_SECRET} ok") + + assert _SECRET not in redacted + assert "***" in redacted + + +def test_repr_redacts_to_conn_id_only(monkeypatch: pytest.MonkeyPatch) -> None: + """``__repr__`` shows only ``signalforge_conn_id`` — never the resolved key + (which is not even stored on the hook) or a credential-revealing label.""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + + hook = _hook_class()(signalforge_conn_id="my_conn") + + rendered = repr(hook) + + assert rendered == "SignalForgeHook(signalforge_conn_id='my_conn')" + assert _SECRET not in rendered + assert "api_key" not in rendered diff --git a/tests/airflow/test_skeleton.py b/tests/airflow/test_skeleton.py index 2b907d07..de1e69d1 100644 --- a/tests/airflow/test_skeleton.py +++ b/tests/airflow/test_skeleton.py @@ -148,13 +148,44 @@ def _airflow_loaded() -> bool: pytest.skip("airflow installed; gated tests/airflow/test_operators.py cover construction") -def test_hook_stub_raises_not_implemented() -> None: - """The skeleton hook is construction-inert until an epic-#228 child - implements it.""" +def test_hook_access_is_airflow_free_and_construction_requires_airflow() -> None: + """#234 US-004: the hook name resolves WITHOUT importing airflow (the + no-eager-import contract), and — with the ``[airflow]`` extra absent — + CONSTRUCTING it raises an ``ImportError`` (the real hook subclasses + ``BaseHook`` and genuinely needs Airflow at construction time). + + Mirrors the operator skeleton tests above: attribute access goes through the + find_spec-guarded ``hooks._get_signalforge_hook_class`` (airflow-absent → the + airflow-free placeholder whose ``__init__`` raises ``ModuleNotFoundError``; + airflow-present → the real ``BaseHook`` subclass, whose construction is + covered by the gated ``tests/airflow/test_hooks.py``). This ungated test is + what exercises the airflow-absent placeholder/factory/``__getattr__`` lines in + the default (no-airflow) CI env — the codecov patch gate counts them.""" + + def _airflow_loaded() -> bool: + return any(m == "airflow" or m.startswith("airflow.") for m in sys.modules) + + had_airflow = _airflow_loaded() from signalforge.airflow import SignalForgeHook - with pytest.raises(NotImplementedError): - SignalForgeHook() + assert SignalForgeHook is not None + assert _airflow_loaded() == had_airflow, ( + "accessing SignalForgeHook must not import the real airflow package" + ) + + if importlib.util.find_spec("airflow") is None: + with pytest.raises((ImportError, ModuleNotFoundError)): + SignalForgeHook(signalforge_conn_id="signalforge_default") + else: # pragma: no cover - default CI env has no [airflow] extra + pytest.skip("airflow installed; gated tests/airflow/test_hooks.py cover construction") + + +def test_hooks_module_unknown_attr_raises() -> None: + """The hooks module's PEP 562 ``__getattr__`` fall-through raises + ``AttributeError`` for an unknown name (airflow-free — no class build).""" + hooks = importlib.import_module("signalforge.airflow.hooks") + with pytest.raises(AttributeError): + _ = hooks.does_not_exist # type: ignore[attr-defined] def test_package_dir_and_unknown_attr() -> None: From 019c799d7d3744fe06671f5fdcef64944f62b4d2 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 11:07:16 -0700 Subject: [PATCH 10/15] bd_1-scaffolding-qhi.5: wire signalforge_conn_id through SignalForgeGenerateOperator + shared helper --- src/signalforge/airflow/_resolve.py | 15 +- src/signalforge/airflow/operators.py | 185 +++++++++++++++-- tests/airflow/test_operators.py | 264 ++++++++++++++++++++++++ tests/airflow/test_operators_helpers.py | 80 +++++++ 4 files changed, 527 insertions(+), 17 deletions(-) diff --git a/src/signalforge/airflow/_resolve.py b/src/signalforge/airflow/_resolve.py index d378db48..84b67473 100644 --- a/src/signalforge/airflow/_resolve.py +++ b/src/signalforge/airflow/_resolve.py @@ -84,10 +84,12 @@ def extra_dejson(self) -> dict[str, object]: ... class HookResolution: """The typed result of resolving a SignalForge Airflow Connection. - Frozen and airflow-free. Carries the three things the operators need: - ``profiles_dir`` (warehouse auth), ``provider`` (which LLM SKU family), and - ``api_key`` (the credential). All three are ``None``-able — the resolver is - lenient (DEC-003); the consuming operator enforces requiredness. + Frozen and airflow-free. Carries what the operators need: + ``profiles_dir`` (warehouse auth), ``provider`` (which LLM SKU family), + ``api_key`` (the credential), and ``cache_scope`` (the Anthropic + cached-prefix knob — precedence-merged by the Generate operator, DEC-012). + All are ``None``-able — the resolver is lenient (DEC-003); the consuming + operator enforces requiredness. The custom :meth:`__repr__` is a **leak-surface discipline** (DEC-007): it renders ``profiles_dir`` and ``provider`` only, and NEVER the ``api_key`` @@ -100,6 +102,7 @@ class HookResolution: profiles_dir: str | None provider: str | None api_key: str | None + cache_scope: str | None = None def __repr__(self) -> str: # Deliberately omits ``api_key`` entirely — no value, no label. @@ -159,6 +162,9 @@ def resolve_connection( from resolution) raises :class:`AirflowConfigError` (DEC-008). When no ``project_dir`` is given the path is accepted as-is (the documented bounded-defence gap — there is no anchor to contain against). + * **cache_scope** — read from the validated ``extra`` and carried through + unchanged (may be ``None``). The Generate operator precedence-merges it + with its own ``cache_scope`` param (DEC-012); other consumers ignore it. Raises: AirflowConfigError: on any misconfiguration (DEC-009 — the one reused @@ -217,6 +223,7 @@ def resolve_connection( profiles_dir=profiles_dir, provider=provider, api_key=api_key, + cache_scope=extra.cache_scope, ) diff --git a/src/signalforge/airflow/operators.py b/src/signalforge/airflow/operators.py index f7d552a9..69933d7d 100644 --- a/src/signalforge/airflow/operators.py +++ b/src/signalforge/airflow/operators.py @@ -42,12 +42,21 @@ from __future__ import annotations +import contextlib import functools import importlib.util -from collections.abc import Sequence +import json +import logging +import os +from collections.abc import Iterator, Sequence from typing import TYPE_CHECKING, Any, Literal -from signalforge.airflow._airflow_compat import make_base_operator, raise_for_outcome +from signalforge.airflow._airflow_compat import ( + make_base_operator, + raise_for_outcome, + register_secret, +) +from signalforge.airflow._resolve import HookResolution from signalforge.airflow.errors import AirflowConfigError from signalforge.airflow.result import ( OnFlagged, @@ -55,6 +64,9 @@ decide_task_outcome, ) from signalforge.airflow.runner import run_signalforge +from signalforge.llm.providers import PROVIDER_ENV_VAR_KEYS + +_LOGGER = logging.getLogger("signalforge.airflow") if TYPE_CHECKING: # Type-checker-only declaration of the operator name. At runtime the class is @@ -77,6 +89,74 @@ def execute(self, context: Any) -> Any: ... _VALID_ON_FLAGGED: frozenset[str] = frozenset({"fail", "skip", "succeed"}) +# --------------------------------------------------------------------------- # +# Shared `signalforge_conn_id` wiring helpers (#234 US-005) # +# # +# Two of these are airflow-free + ungated-testable (the precedence merge and # +# the env-injection context manager); the third constructs the gated hook and # +# is reached only on the conn-id path inside ``execute``. US-006 reuses the # +# airflow-free merge for the prune-existing operator's profiles_dir-only # +# precedence (it needs NO key injection), so the two concerns are kept # +# deliberately decoupled (DEC-016). # +# --------------------------------------------------------------------------- # + + +def _merge_with_resolution(param_value: str | None, extra_value: str | None) -> str | None: + """Precedence merge for a value present on both the operator + Connection (DEC-012). + + Pure, airflow-free. Returns the explicit operator ``param_value`` when it is + truthy (an explicitly-set, non-empty operator param), else the + ``extra_value`` resolved from the Connection ``extra`` (itself possibly + ``None``). Mirrors the CLI's ``flag > YAML > default`` precedence; the final + fall-through to the tool default (when both are ``None``) is left to the + downstream CLI. Used for ``profiles_dir`` (both operators) and ``cache_scope`` + (Generate only). + """ + return param_value if param_value else extra_value + + +@contextlib.contextmanager +def _provider_key_env(env_var: str, api_key: str) -> Iterator[None]: + """Inject ``os.environ[env_var] = api_key`` for the block, restoring on exit. + + Pure (touches only ``os.environ``), airflow-free, ungated-testable. Snapshots + the prior value and, in a ``finally`` that fires on BOTH normal completion + and an exception raised inside the block, restores it: if the var was absent + before it is deleted; if it had a prior value (including an empty string) + that value is restored. This keeps a resolved LLM credential from lingering + in a long-lived Airflow worker's environment across tasks (DEC-015). + + ``os.environ`` values are always ``str``, so a ``None`` from ``.get`` means + the var was genuinely absent — distinguishing it cleanly from a present + empty-string value (which round-trips as ``""``). + """ + prior = os.environ.get(env_var) + os.environ[env_var] = api_key + try: + yield + finally: + if prior is None: + os.environ.pop(env_var, None) + else: + os.environ[env_var] = prior + + +def _resolve_hook(conn_id: str) -> HookResolution: # pragma: no cover - needs [airflow] + """Construct the SignalForge hook and resolve its Connection (gated helper). + + Reused by both operators' ``execute`` (US-005 / US-006). Constructing + :class:`signalforge.airflow.hooks.SignalForgeHook` requires Apache Airflow + (it subclasses ``BaseHook``), so this helper is reached only on the + ``signalforge_conn_id``-set path and is ``# pragma: no cover`` for the same + reason as the operator factories. The hook import is lazy — its + module-``__getattr__`` resolves the real class without eagerly importing + airflow — so ``import signalforge.airflow.operators`` stays airflow-free. + """ + from signalforge.airflow.hooks import SignalForgeHook + + return SignalForgeHook(conn_id).get_conn() + + def _build_generate_argv( *, model: str | None, @@ -496,6 +576,21 @@ class SignalForgeGenerateOperator(_Base): # type: ignore[valid-type, misc] operator's ``cache_scope`` is unset and the batch has ≥2 models, it is forced to ``"project"`` (DEC-007) so the Anthropic cached prefix amortises across the siblings. + + Airflow-native credentials (#234, optional ``signalforge_conn_id``): + + * When ``signalforge_conn_id`` is ``None`` (the default), the operator is + byte-identical to #232 — it relies on ambient env / inline config and + never touches the hook (DEC-014). + * When set, ``execute`` resolves the Connection via + :class:`signalforge.airflow.hooks.SignalForgeHook`, REQUIRES both an LLM + ``provider`` and an ``api_key`` (``generate`` calls the LLM; DEC-003), + masks the key (DEC-006), precedence-merges ``profiles_dir`` / + ``cache_scope`` (param > Connection ``extra``; DEC-012), and injects the + provider's API-key env var only for the duration of the run — restoring + it afterward so the secret never lingers across tasks (DEC-015). The + conn id is NOT a ``template_fields`` entry and never enters XCom + (DEC-007). """ # Airflow renders these fields from the task context before ``execute`` @@ -514,6 +609,7 @@ def __init__( no_grade: bool = False, cache_scope: str | None = None, as_of: str | None = None, + signalforge_conn_id: str | None = None, on_flagged: OnFlagged = "fail", invocation: Literal["in_process", "subprocess"] = "in_process", **kwargs: Any, @@ -529,6 +625,10 @@ def __init__( self.no_grade = no_grade self.cache_scope = cache_scope self.as_of = as_of + # A conn id is NOT a templated value and is deliberately kept OUT of + # ``template_fields`` (DEC-007): templating a credential reference is + # an avoidable leak surface. + self.signalforge_conn_id = signalforge_conn_id # Annotate the Literal-typed attrs explicitly so pyright does NOT # widen them to ``str`` on assignment (which would break the typed # ``decide_task_outcome`` / ``run_signalforge`` calls below). @@ -558,19 +658,75 @@ def execute(self, context: Any) -> dict[str, object]: on_flagged=self.on_flagged, ) - if self.select is None: - return self._execute_single() - return self._execute_batch() + # No ``signalforge_conn_id`` → byte-identical to #232 (DEC-014): no + # hook, no credential resolution, no env injection. + effective_profiles_dir = self.profiles_dir + effective_cache_scope = self.cache_scope + env_cm: contextlib.AbstractContextManager[None] = contextlib.nullcontext() + + if self.signalforge_conn_id is not None: + resolution = _resolve_hook(self.signalforge_conn_id) + # ``generate`` calls the LLM, so provider + key are REQUIRED here + # (DEC-003 — the resolver is lenient; the consumer enforces + # requiredness). PruneExisting, by contrast, needs neither. + if resolution.provider is None or resolution.api_key is None: + raise AirflowConfigError( + f"The SignalForge Airflow Connection {self.signalforge_conn_id!r} " + "did not supply both an LLM `provider` and an API key, which " + "`signalforge generate` requires (it calls the LLM).", + remediation=( + "Set the Connection `extra.provider` to one of " + "anthropic / openai / gemini and put the API key in the " + "Connection `password` (or the `signalforge_api_key` Airflow " + "Variable). For a credential-free task, use " + "SignalForgePruneExistingOperator instead." + ), + ) + # Mask the resolved key in Airflow's logs BEFORE any logging or + # run (DEC-006), belt-and-braces over Airflow's auto-masking. + register_secret(resolution.api_key) + # Precedence merge (DEC-012): explicit param > Connection extra. + effective_profiles_dir = _merge_with_resolution( + self.profiles_dir, resolution.profiles_dir + ) + effective_cache_scope = _merge_with_resolution( + self.cache_scope, resolution.cache_scope + ) + # NEVER log the api_key (DEC-007). ``key_source`` is not exposed + # by the resolution, so it is omitted. + _LOGGER.info( + "signalforge generate resolved airflow connection: %s", + json.dumps( + { + "conn_id": self.signalforge_conn_id, + "provider": resolution.provider, + "profiles_dir_set": effective_profiles_dir is not None, + } + ), + ) + env_cm = _provider_key_env( + PROVIDER_ENV_VAR_KEYS[resolution.provider], resolution.api_key + ) - def _execute_single(self) -> dict[str, object]: + # The provider env var is injected only for the duration of the run + # and restored in the context manager's ``finally`` (success OR + # exception) so the secret never lingers across tasks (DEC-015). + with env_cm: + if self.select is None: + return self._execute_single(effective_profiles_dir, effective_cache_scope) + return self._execute_batch(effective_profiles_dir, effective_cache_scope) + + def _execute_single( + self, profiles_dir: str | None, cache_scope: str | None + ) -> dict[str, object]: argv = _build_generate_argv( model=self.model, select=None, project_dir=self.project_dir, - profiles_dir=self.profiles_dir, + profiles_dir=profiles_dir, write=self.write, no_grade=self.no_grade, - cache_scope=self.cache_scope, + cache_scope=cache_scope, as_of=self.as_of, ) result = run_signalforge(argv, project_dir=self.project_dir, invocation=self.invocation) @@ -585,13 +741,16 @@ def _execute_single(self) -> dict[str, object]: ) return result.to_xcom() - def _execute_batch(self) -> dict[str, object]: + def _execute_batch( + self, profiles_dir: str | None, cache_scope: str | None + ) -> dict[str, object]: assert self.select is not None # narrowed by execute() model_ids = _resolve_select_models(self.project_dir, self.select) # DEC-007: force project-scope caching for a ≥2-model batch when the - # operator did not pin a scope, so the Anthropic cached prefix - # amortises across the siblings instead of paying creation per model. - resolved_cache_scope = self.cache_scope + # operator did not pin a scope (after the Connection-extra precedence + # merge), so the Anthropic cached prefix amortises across the siblings + # instead of paying creation per model. + resolved_cache_scope = cache_scope if resolved_cache_scope is None and len(model_ids) >= 2: resolved_cache_scope = "project" @@ -606,7 +765,7 @@ def _execute_batch(self) -> dict[str, object]: model=model_id, select=None, project_dir=self.project_dir, - profiles_dir=self.profiles_dir, + profiles_dir=profiles_dir, write=self.write, no_grade=self.no_grade, cache_scope=resolved_cache_scope, diff --git a/tests/airflow/test_operators.py b/tests/airflow/test_operators.py index 7da6f229..9010853a 100644 --- a/tests/airflow/test_operators.py +++ b/tests/airflow/test_operators.py @@ -27,9 +27,12 @@ from __future__ import annotations import importlib +import json +import os import pytest +from signalforge.airflow._resolve import HookResolution from signalforge.airflow.result import SignalForgeRunResult pytestmark = pytest.mark.airflow @@ -404,6 +407,267 @@ def test_construction_rejects_bad_on_flagged() -> None: _operator_class()(task_id="gen", project_dir="/proj", model="m", on_flagged="bogus") +# --------------------------------------------------------------------------- # +# signalforge_conn_id wiring (#234 US-005) # +# --------------------------------------------------------------------------- # + + +def _patch_run_capturing_env( + monkeypatch: pytest.MonkeyPatch, + results: list[SignalForgeRunResult], + env_var: str, +) -> tuple[list[list[str]], list[str | None]]: + """Patch ``run_signalforge`` recording (argv, ``os.environ[env_var]``) per call. + + The env value is sampled INSIDE the fake at call time, so a test can assert + the provider key was injected for the duration of the run (and later that it + was restored after ``execute`` returns / raises). + """ + captured_argv: list[list[str]] = [] + captured_env: list[str | None] = [] + queue = list(results) + + def _fake_run( + argv: list[str], + *, + project_dir: object, + invocation: object = "in_process", + timeout_seconds: object = None, + ) -> SignalForgeRunResult: + captured_argv.append(list(argv)) + captured_env.append(os.environ.get(env_var)) + return queue.pop(0) + + monkeypatch.setattr("signalforge.airflow.operators.run_signalforge", _fake_run) + return captured_argv, captured_env + + +def _patch_hook(monkeypatch: pytest.MonkeyPatch, resolution: HookResolution) -> list[str]: + """Patch ``_resolve_hook`` to return ``resolution`` + ``register_secret`` to record. + + Returns the list of values passed to ``register_secret`` (so a test can + assert the resolved key was masked before the run). + """ + masked: list[str] = [] + monkeypatch.setattr( + "signalforge.airflow.operators._resolve_hook", + lambda conn_id: resolution, + ) + monkeypatch.setattr( + "signalforge.airflow.operators.register_secret", + lambda value: masked.append(value), + ) + return masked + + +def test_single_model_conn_id_injects_env_masks_key_and_restores( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """conn_id set → env var injected DURING the run, masked, restored after.""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + + resolution = HookResolution( + profiles_dir="/conn/profiles", provider="anthropic", api_key="sk-conn-secret" + ) + masked = _patch_hook(monkeypatch, resolution) + argv_cap, env_cap = _patch_run_capturing_env( + monkeypatch, [_result(exit_code=0, flagged=0)], "ANTHROPIC_API_KEY" + ) + + op = _operator_class()( + task_id="gen", project_dir="/proj", model="m", signalforge_conn_id="sf_default" + ) + op.execute(context={}) + + # Injected for the run, then restored (absent before → absent after). + assert env_cap == ["sk-conn-secret"] + assert "ANTHROPIC_API_KEY" not in os.environ + # Masked before the run (DEC-006). + assert masked == ["sk-conn-secret"] + # The conn-resolved profiles_dir reaches the argv (no operator override). + argv = argv_cap[0] + assert argv[argv.index("--profiles-dir") + 1] == "/conn/profiles" + + +def test_single_model_conn_env_restored_on_exception( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """An exception inside the run (exit 3 → raise) still restores the env var.""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + from airflow.exceptions import AirflowException + + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + resolution = HookResolution(profiles_dir=None, provider="anthropic", api_key="sk-conn-secret") + _patch_hook(monkeypatch, resolution) + _patch_run_capturing_env(monkeypatch, [_result(exit_code=3, flagged=0)], "ANTHROPIC_API_KEY") + + op = _operator_class()(task_id="gen", project_dir="/proj", model="m", signalforge_conn_id="sf") + with pytest.raises(AirflowException): + op.execute(context={}) + # raise_for_outcome raises INSIDE the _provider_key_env block → finally restores. + assert "ANTHROPIC_API_KEY" not in os.environ + + +def test_single_model_conn_prior_env_value_restored( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A pre-existing ambient key is overlaid for the run, then restored.""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + monkeypatch.setenv("ANTHROPIC_API_KEY", "ambient-prior") + resolution = HookResolution(profiles_dir=None, provider="anthropic", api_key="sk-conn-secret") + _patch_hook(monkeypatch, resolution) + _, env_cap = _patch_run_capturing_env( + monkeypatch, [_result(exit_code=0, flagged=0)], "ANTHROPIC_API_KEY" + ) + + op = _operator_class()(task_id="gen", project_dir="/proj", model="m", signalforge_conn_id="sf") + op.execute(context={}) + assert env_cap == ["sk-conn-secret"] + assert os.environ["ANTHROPIC_API_KEY"] == "ambient-prior" + + +def test_conn_explicit_param_beats_extra_profiles_dir( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Explicit operator ``profiles_dir`` wins over the Connection extra (DEC-012).""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + resolution = HookResolution(profiles_dir="/conn/profiles", provider="anthropic", api_key="sk") + _patch_hook(monkeypatch, resolution) + argv_cap, _ = _patch_run_capturing_env( + monkeypatch, [_result(exit_code=0, flagged=0)], "ANTHROPIC_API_KEY" + ) + + op = _operator_class()( + task_id="gen", + project_dir="/proj", + model="m", + profiles_dir="/op/profiles", + signalforge_conn_id="sf", + ) + op.execute(context={}) + argv = argv_cap[0] + assert argv[argv.index("--profiles-dir") + 1] == "/op/profiles" + + +def test_conn_cache_scope_from_extra_reaches_argv( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A Connection-extra ``cache_scope`` is precedence-merged into the argv (DEC-012).""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + monkeypatch.delenv("GOOGLE_API_KEY", raising=False) + resolution = HookResolution( + profiles_dir=None, provider="gemini", api_key="sk", cache_scope="project" + ) + _patch_hook(monkeypatch, resolution) + argv_cap, env_cap = _patch_run_capturing_env( + monkeypatch, [_result(exit_code=0, flagged=0)], "GOOGLE_API_KEY" + ) + + op = _operator_class()(task_id="gen", project_dir="/proj", model="m", signalforge_conn_id="sf") + op.execute(context={}) + argv = argv_cap[0] + assert argv[argv.index("--cache-scope") + 1] == "project" + # The provider→env-var mapping picks GOOGLE_API_KEY for gemini (#234 US-001). + assert env_cap == ["sk"] + assert "GOOGLE_API_KEY" not in os.environ + + +def test_conn_api_key_absent_from_returned_xcom( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The resolved key never enters the returned XCom payload (DEC-007).""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + resolution = HookResolution(profiles_dir=None, provider="anthropic", api_key="sk-conn-secret") + _patch_hook(monkeypatch, resolution) + _patch_run_capturing_env(monkeypatch, [_result(exit_code=0, flagged=0)], "ANTHROPIC_API_KEY") + + op = _operator_class()(task_id="gen", project_dir="/proj", model="m", signalforge_conn_id="sf") + xcom = op.execute(context={}) + assert "sk-conn-secret" not in json.dumps(xcom) + + +def test_conn_missing_key_raises_config_error_and_skips_run( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """provider present but api_key absent → AirflowConfigError before any run.""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + from signalforge.airflow.errors import AirflowConfigError + + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + _patch_hook(monkeypatch, HookResolution(profiles_dir=None, provider="anthropic", api_key=None)) + argv_cap, _ = _patch_run_capturing_env(monkeypatch, [_result()], "ANTHROPIC_API_KEY") + + op = _operator_class()(task_id="gen", project_dir="/proj", model="m", signalforge_conn_id="sf") + with pytest.raises(AirflowConfigError): + op.execute(context={}) + # Fails before run_signalforge is reached, and leaks no env var. + assert argv_cap == [] + assert "ANTHROPIC_API_KEY" not in os.environ + + +def test_conn_missing_provider_raises_config_error( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """api_key present but provider absent → AirflowConfigError (generate needs both).""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + from signalforge.airflow.errors import AirflowConfigError + + _patch_hook(monkeypatch, HookResolution(profiles_dir=None, provider=None, api_key="sk")) + argv_cap, _ = _patch_run_capturing_env(monkeypatch, [_result()], "ANTHROPIC_API_KEY") + + op = _operator_class()(task_id="gen", project_dir="/proj", model="m", signalforge_conn_id="sf") + with pytest.raises(AirflowConfigError): + op.execute(context={}) + assert argv_cap == [] + + +def test_conn_id_not_in_template_fields() -> None: + """``signalforge_conn_id`` is NOT a templated field (DEC-007).""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + assert "signalforge_conn_id" not in _operator_class().template_fields + + +def test_batch_with_conn_id_injects_env_for_every_model_and_restores( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A --select batch injects the env var around EACH per-model run + restores it, + and the conn-resolved profiles_dir (+ forced project cache) reach each argv.""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + monkeypatch.setattr( + "signalforge.airflow.operators._resolve_select_models", + lambda project_dir, select: ("model.p.a", "model.p.b"), + ) + resolution = HookResolution( + profiles_dir="/conn/profiles", provider="anthropic", api_key="sk-conn-secret" + ) + _patch_hook(monkeypatch, resolution) + argv_cap, env_cap = _patch_run_capturing_env( + monkeypatch, + [ + _result(exit_code=0, flagged=0, model_unique_ids=("model.p.a",)), + _result(exit_code=0, flagged=0, model_unique_ids=("model.p.b",)), + ], + "ANTHROPIC_API_KEY", + ) + + op = _operator_class()( + task_id="gen", project_dir="/proj", select="tag:staging", signalforge_conn_id="sf" + ) + op.execute(context={}) + + # Env var present for BOTH per-model runs, restored afterward. + assert env_cap == ["sk-conn-secret", "sk-conn-secret"] + assert "ANTHROPIC_API_KEY" not in os.environ + # conn profiles_dir + the ≥2-model forced project cache reach each argv. + for argv in argv_cap: + assert argv[argv.index("--profiles-dir") + 1] == "/conn/profiles" + assert argv[argv.index("--cache-scope") + 1] == "project" + + # --------------------------------------------------------------------------- # # SignalForgePruneExistingOperator (#233) — single-model, no-LLM, read-only # --------------------------------------------------------------------------- # diff --git a/tests/airflow/test_operators_helpers.py b/tests/airflow/test_operators_helpers.py index f3767b00..a37588ae 100644 --- a/tests/airflow/test_operators_helpers.py +++ b/tests/airflow/test_operators_helpers.py @@ -23,6 +23,7 @@ from __future__ import annotations +import os from pathlib import Path import pytest @@ -32,6 +33,8 @@ _aggregate_batch_result, _build_generate_argv, _build_prune_existing_argv, + _merge_with_resolution, + _provider_key_env, _resolve_select_models, _validate_operator_config, _validate_prune_existing_config, @@ -647,3 +650,80 @@ def test_validate_prune_accepts_each_valid_on_flagged(on_flagged: str) -> None: _validate_prune_existing_config( project_dir="/proj", model="customers", schema="schema.yml", on_flagged=on_flagged ) + + +# --------------------------------------------------------------------------- # +# _merge_with_resolution (#234 US-005, DEC-012) # +# --------------------------------------------------------------------------- # + + +def test_merge_param_wins_over_extra() -> None: + """An explicit (truthy) operator param beats the Connection extra value.""" + assert _merge_with_resolution("/param/dbt", "/extra/dbt") == "/param/dbt" + + +def test_merge_falls_back_to_extra_when_param_none() -> None: + """A ``None`` operator param falls back to the Connection extra value.""" + assert _merge_with_resolution(None, "/extra/dbt") == "/extra/dbt" + + +def test_merge_falls_back_to_extra_when_param_empty() -> None: + """A blank operator param is treated as unset and falls back to extra.""" + assert _merge_with_resolution("", "/extra/dbt") == "/extra/dbt" + + +def test_merge_returns_none_when_both_absent() -> None: + """Both unset → ``None`` (the downstream CLI applies the tool default).""" + assert _merge_with_resolution(None, None) is None + + +def test_merge_param_set_extra_none() -> None: + """An explicit param with no extra value still wins (extra is ``None``).""" + assert _merge_with_resolution("project", None) == "project" + + +# --------------------------------------------------------------------------- # +# _provider_key_env (#234 US-005, DEC-015) # +# --------------------------------------------------------------------------- # + + +def test_provider_key_env_absent_injects_then_deletes(monkeypatch: pytest.MonkeyPatch) -> None: + """Var absent before → injected inside the block → deleted on exit.""" + monkeypatch.delenv("SF_TEST_KEY", raising=False) + assert "SF_TEST_KEY" not in os.environ + with _provider_key_env("SF_TEST_KEY", "sk-secret"): + assert os.environ["SF_TEST_KEY"] == "sk-secret" + assert "SF_TEST_KEY" not in os.environ + + +def test_provider_key_env_prior_value_restored(monkeypatch: pytest.MonkeyPatch) -> None: + """Var had a prior value → injected inside the block → prior restored on exit.""" + monkeypatch.setenv("SF_TEST_KEY", "prior-value") + with _provider_key_env("SF_TEST_KEY", "sk-secret"): + assert os.environ["SF_TEST_KEY"] == "sk-secret" + assert os.environ["SF_TEST_KEY"] == "prior-value" + + +def test_provider_key_env_empty_prior_value_restored(monkeypatch: pytest.MonkeyPatch) -> None: + """A present-but-empty prior value round-trips as ``""`` (not deleted).""" + monkeypatch.setenv("SF_TEST_KEY", "") + with _provider_key_env("SF_TEST_KEY", "sk-secret"): + assert os.environ["SF_TEST_KEY"] == "sk-secret" + assert os.environ.get("SF_TEST_KEY") == "" + + +def test_provider_key_env_restores_on_exception_absent(monkeypatch: pytest.MonkeyPatch) -> None: + """An exception inside the block still deletes a previously-absent var.""" + monkeypatch.delenv("SF_TEST_KEY", raising=False) + with pytest.raises(RuntimeError, match="boom"), _provider_key_env("SF_TEST_KEY", "sk-secret"): + assert os.environ["SF_TEST_KEY"] == "sk-secret" + raise RuntimeError("boom") + assert "SF_TEST_KEY" not in os.environ + + +def test_provider_key_env_restores_on_exception_prior(monkeypatch: pytest.MonkeyPatch) -> None: + """An exception inside the block still restores a prior value.""" + monkeypatch.setenv("SF_TEST_KEY", "prior-value") + with pytest.raises(RuntimeError, match="boom"), _provider_key_env("SF_TEST_KEY", "sk-secret"): + raise RuntimeError("boom") + assert os.environ["SF_TEST_KEY"] == "prior-value" From 5f9bd794602531e70c88dcbc3b220b6f47659ef8 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 11:13:26 -0700 Subject: [PATCH 11/15] bd_1-scaffolding-qhi.6: wire signalforge_conn_id through SignalForgePruneExistingOperator --- src/signalforge/airflow/operators.py | 51 ++++++++++++- tests/airflow/test_operators.py | 104 +++++++++++++++++++++++++++ 2 files changed, 154 insertions(+), 1 deletion(-) diff --git a/src/signalforge/airflow/operators.py b/src/signalforge/airflow/operators.py index 69933d7d..629127b0 100644 --- a/src/signalforge/airflow/operators.py +++ b/src/signalforge/airflow/operators.py @@ -864,6 +864,22 @@ class SignalForgePruneExistingOperator(_Base): # type: ignore[valid-type, misc] with the sibling operators but is inert today — ``prune-existing`` does no grading, so there is never a ``flagged`` tier and a clean (exit-0) run always yields ``SUCCESS`` regardless of ``on_flagged`` (#233 DEC-004). + + Airflow-native credentials (#234, optional ``signalforge_conn_id``): + + * When ``signalforge_conn_id`` is ``None`` (the default), the operator is + byte-identical to #233 — it relies on ambient env / inline config and + never touches the hook (DEC-014). + * When set, ``execute`` resolves the Connection via + :class:`signalforge.airflow.hooks.SignalForgeHook` and uses ONLY the + resolved ``profiles_dir`` (warehouse auth), precedence-merged + param > Connection ``extra`` (DEC-012). Because ``prune-existing`` makes + NO LLM call (DEC-016), this path does NOT call ``register_secret``, + injects NO provider env var, and ignores ``provider`` / ``api_key`` / + ``cache_scope`` on the resolution — though an allowlist-invalid + ``provider`` in the Connection ``extra`` still raises ``AirflowConfigError`` + (that check lives in the resolver, DEC-005). The conn id is NOT a + ``template_fields`` entry and never enters XCom (DEC-007). """ # Airflow renders these fields from the task context before ``execute`` @@ -891,6 +907,7 @@ def __init__( sample_strategy: str | None = None, as_of: str | None = None, tests_dir: str | None = None, + signalforge_conn_id: str | None = None, on_flagged: OnFlagged = "fail", invocation: Literal["in_process", "subprocess"] = "in_process", **kwargs: Any, @@ -907,6 +924,10 @@ def __init__( self.sample_strategy = sample_strategy self.as_of = as_of self.tests_dir = tests_dir + # A conn id is NOT a templated value and is deliberately kept OUT of + # ``template_fields`` (DEC-007): templating a credential reference is + # an avoidable leak surface. + self.signalforge_conn_id = signalforge_conn_id # Annotate the Literal-typed attrs explicitly so pyright does NOT # widen them to ``str`` on assignment (which would break the typed # ``decide_task_outcome`` / ``run_signalforge`` calls below). @@ -934,12 +955,40 @@ def execute(self, context: Any) -> dict[str, object]: schema=self.schema, on_flagged=self.on_flagged, ) + + # No ``signalforge_conn_id`` → byte-identical to #233 (DEC-014): no + # hook, no credential resolution. When set, resolve ONLY the + # warehouse-auth ``profiles_dir`` (DEC-016): prune-existing makes no + # LLM call, so there is NO register_secret, NO provider env injection, + # and ``provider`` / ``api_key`` / ``cache_scope`` on the resolution + # are ignored (an allowlist-invalid ``provider``, if present, still + # raises inside the resolver per DEC-005). + effective_profiles_dir = self.profiles_dir + if self.signalforge_conn_id is not None: + resolution = _resolve_hook(self.signalforge_conn_id) + # Precedence merge (DEC-012): explicit param > Connection extra. + effective_profiles_dir = _merge_with_resolution( + self.profiles_dir, resolution.profiles_dir + ) + # NEVER log a secret (DEC-007). prune-existing resolves no key, so + # there is none to leak; record only conn_id + whether a + # profiles_dir was resolved. + _LOGGER.info( + "signalforge prune-existing resolved airflow connection: %s", + json.dumps( + { + "conn_id": self.signalforge_conn_id, + "profiles_dir_set": effective_profiles_dir is not None, + } + ), + ) + # Single call — no batch (#233 DEC-003). argv = _build_prune_existing_argv( model=self.model, schema=self.schema, project_dir=self.project_dir, - profiles_dir=self.profiles_dir, + profiles_dir=effective_profiles_dir, manifest=self.manifest, scope=self.scope, sample_strategy=self.sample_strategy, diff --git a/tests/airflow/test_operators.py b/tests/airflow/test_operators.py index 9010853a..e8a21aff 100644 --- a/tests/airflow/test_operators.py +++ b/tests/airflow/test_operators.py @@ -835,3 +835,107 @@ def test_prune_existing_construction_rejects_bad_on_flagged() -> None: _prune_existing_operator_class()( task_id="prune", project_dir="/proj", model="m", schema="s.yml", on_flagged="bogus" ) + + +# --------------------------------------------------------------------------- # +# SignalForgePruneExistingOperator + signalforge_conn_id (#234 US-006) # +# Read-only path: resolves profiles_dir ONLY — NO key injection (DEC-016). # +# --------------------------------------------------------------------------- # + + +def test_prune_existing_conn_id_resolves_profiles_dir_and_injects_no_env( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """conn_id set → resolved profiles_dir reaches argv; NO provider env injected. + + The resolution deliberately carries a ``provider`` + ``api_key`` to prove the + prune-existing path IGNORES them: no ``register_secret`` call, no env var set + or restored, the credential never touches ``os.environ`` (DEC-016). + """ + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + + resolution = HookResolution( + profiles_dir="/conn/profiles", provider="anthropic", api_key="sk-conn-secret" + ) + masked = _patch_hook(monkeypatch, resolution) + argv_cap, env_cap = _patch_run_capturing_env( + monkeypatch, [_result(exit_code=0, flagged=0)], "ANTHROPIC_API_KEY" + ) + + op = _prune_existing_operator_class()( + task_id="prune", + project_dir="/proj", + model="m", + schema="s.yml", + signalforge_conn_id="sf_default", + ) + op.execute(context={}) + + # Conn-resolved profiles_dir reaches the argv (no operator override). + argv = argv_cap[0] + assert argv[argv.index("--profiles-dir") + 1] == "/conn/profiles" + # NO key injection on the read-only path: register_secret never called, the + # env var never set during the run, and untouched in os.environ afterward. + assert masked == [] + assert env_cap == [None] + assert "ANTHROPIC_API_KEY" not in os.environ + + +def test_prune_existing_conn_explicit_param_beats_extra_profiles_dir( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Explicit operator ``profiles_dir`` wins over the Connection extra (DEC-012).""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + + resolution = HookResolution(profiles_dir="/conn/profiles", provider=None, api_key=None) + _patch_hook(monkeypatch, resolution) + captured = _patch_run(monkeypatch, [_result(exit_code=0, flagged=0)]) + + op = _prune_existing_operator_class()( + task_id="prune", + project_dir="/proj", + model="m", + schema="s.yml", + profiles_dir="/op/profiles", + signalforge_conn_id="sf", + ) + op.execute(context={}) + argv = captured[0] + assert argv[argv.index("--profiles-dir") + 1] == "/op/profiles" + + +def test_prune_existing_conn_id_none_unchanged_no_hook( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """conn_id=None (#233 default): no hook touched, argv byte-identical to #233.""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + + def _boom(_conn_id: str) -> HookResolution: + raise AssertionError("_resolve_hook must NOT be called when conn_id is None") + + monkeypatch.setattr("signalforge.airflow.operators._resolve_hook", _boom) + captured = _patch_run(monkeypatch, [_result(exit_code=0, flagged=0)]) + + op = _prune_existing_operator_class()( + task_id="prune", project_dir="/proj", model="m", schema="s.yml" + ) + op.execute(context={}) + assert captured[0] == [ + "prune-existing", + "m", + "--schema", + "s.yml", + "--project-dir", + "/proj", + "--format", + "json", + "--dry-run", + ] + + +def test_prune_existing_conn_id_not_in_template_fields() -> None: + """``signalforge_conn_id`` is NOT a templated field on prune-existing (DEC-007).""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + assert "signalforge_conn_id" not in _prune_existing_operator_class().template_fields From ac487f38b15ca4da6537dd0c276f149614c757c2 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 11:19:03 -0700 Subject: [PATCH 12/15] bd_1-scaffolding-qhi.7: docs/airflow-ops.md hook section + example DAG --- docs/airflow-ops.md | 172 ++++++++++++++++++++++- examples/airflow/signalforge_hook_dag.py | 169 ++++++++++++++++++++++ tests/airflow/test_dag_parse.py | 26 +++- 3 files changed, 360 insertions(+), 7 deletions(-) create mode 100644 examples/airflow/signalforge_hook_dag.py diff --git a/docs/airflow-ops.md b/docs/airflow-ops.md index 45d7599a..89e3c55f 100644 --- a/docs/airflow-ops.md +++ b/docs/airflow-ops.md @@ -478,6 +478,171 @@ singular-test (`tests/*.sql`) files in that directory — the `custom_sql` busin test surface — alongside the `schema.yml` tests, pruning them in one run. Omitted when unset (the CLI defaults to `/tests`). +## Airflow-native credentials: `SignalForgeHook` + `signalforge_conn_id` + +Instead of managing `ANTHROPIC_API_KEY` / `DBT_PROFILES_DIR` as inline env on every +task, configure SignalForge the **Airflow-native** way: one Airflow **Connection** +(plus an optional **Variable** for the LLM key), referenced by both operators via the +optional `signalforge_conn_id` param. The hook (`SignalForgeHook`) resolves that +Connection into the three things a run needs — warehouse auth (`profiles_dir`), the LLM +`provider`, and the `api_key` — and the operator injects the key into the run's +environment only for the duration of that run. + +`signalforge_conn_id` is **opt-in**: when it is `None` (the default) both operators +behave exactly as before (ambient env / inline config), byte-identical to #232/#233. +Set it to switch to the Connection-driven path. + +### Setting up the Connection + +Create one Connection (Admin → Connections, or `airflow connections add`) and point +both operators at it via `signalforge_conn_id=""`: + +- **Conn Type** — any (e.g. `generic`); the hook reads only `password` + `extra`. +- **Password** — the **LLM API key** (the *primary* key source). Airflow auto-masks a + Connection `password` in task logs. +- **Extra (JSON)** — the validated schema below. + +```bash +airflow connections add signalforge_default \ + --conn-type generic \ + --conn-password "$ANTHROPIC_API_KEY" \ + --conn-extra '{"profiles_dir": "/opt/airflow/dbt_project", "provider": "anthropic", "cache_scope": "project"}' +``` + +### Connection `extra` schema (validated `extra="forbid"`) + +The `extra` JSON is parsed through a Pydantic model with `extra="forbid"`, so an +**unknown key fails loud** at resolution (a typo like `cache_scop` raises +`AirflowConfigError`, tier 2 — it never silently no-ops). Exactly three keys, all +optional: + +| `extra` key | Meaning | +|----------------|--------------------------------------------------------------------------------------| +| `profiles_dir` | Directory holding the dbt `profiles.yml` → mapped to `--profiles-dir` (warehouse auth). Omit to use the worker's ambient `DBT_PROFILES_DIR`. | +| `provider` | LLM SKU family — `anthropic` / `openai` / `gemini`. Selects the env var the resolved key is injected into (see allowlist below). Omit for a prune-existing-only Connection (no LLM call). | +| `cache_scope` | `per-model` / `project` — the Anthropic cached-prefix knob. **Generate only** (PruneExisting ignores it). | + +**Cost ceilings are deliberately NOT supported in the `extra` for v0.7** (there is no +CLI flag that delivers them — `config_overrides` is deferred, DEC-011). Bound a +scheduled DAG's spend through the committed `signalforge.yml grade:` block instead +(`max_grade_cost_usd` / `max_grade_calls` / `max_grade_tokens` / `total_budget_seconds` +— see the **Cost / time guardrails via `signalforge.yml`** section above). + +### API key: `password` primary, Variable fallback + +The key's primary source is the Connection `password`. When that is empty, the hook +falls back to an Airflow **Variable** named **`signalforge_api_key`** (the +`signalforge.airflow._resolve.API_KEY_VARIABLE_KEY` constant — a single fixed, +provider-agnostic name): + +```bash +airflow variables set signalforge_api_key "$ANTHROPIC_API_KEY" +``` + +The resolver is **lenient** — it returns `api_key` / `provider` as `None` when absent; +*requiredness is enforced by the consuming operator* (Generate raises when either is +missing; PruneExisting needs neither). Prefer the Connection `password` (it auto-masks); +the Variable is the fallback. + +### Provider allowlist (closed) + +`provider`, when present, must be one of a **closed allowlist** — the single source of +truth is `signalforge.llm.providers.PROVIDER_ENV_VAR_KEYS`: + +| `provider` | env var the key is injected into | +|-------------|----------------------------------| +| `anthropic` | `ANTHROPIC_API_KEY` | +| `openai` | `OPENAI_API_KEY` | +| `gemini` | `GOOGLE_API_KEY` (the `google-genai` SDK's convention, not a `GEMINI_*` name) | + +An **unknown provider fails loud** (`AirflowConfigError`) — the check runs whenever +`provider` is set, regardless of operator, so an arbitrary env-var name can never be +derived from a Connection (this closes the arbitrary-env-var injection vector even for a +prune-existing-only Connection that happens to set `provider`). + +### Precedence: explicit param > Connection `extra` > default (DEC-012) + +For the two knobs that exist on both surfaces — `profiles_dir` (both operators) and +`cache_scope` (Generate only) — an explicit, non-empty **operator param wins** over the +Connection `extra` value, which in turn wins over the downstream tool default. Mirrors +the CLI's `flag > YAML > default` precedence: set `profiles_dir=...` on the operator to +override the Connection's `extra.profiles_dir` for that one task. + +### Generate vs PruneExisting credentials (DEC-016) + +The two operators consume the **same** Connection differently: + +- **`SignalForgeGenerateOperator`** calls the LLM, so it **requires** both a `provider` + and an `api_key` (else `AirflowConfigError`), and uses `profiles_dir` for warehouse + auth. It masks the resolved key, precedence-merges `profiles_dir` / `cache_scope`, and + injects the provider's API-key env var around the run. +- **`SignalForgePruneExistingOperator`** is read-only and makes **no LLM call**, so it + uses **only** `profiles_dir` — `provider` / `api_key` / `cache_scope` on the resolution + are ignored. It calls **no** `register_secret`, injects **no** provider env var. A + prune-existing-only deployment can therefore use a Connection with just + `{"profiles_dir": "..."}` and no `password` / `provider` at all. (An allowlist-invalid + `provider`, if one *is* present, still raises in the resolver per the closed allowlist.) + +### Secrets hygiene — the four surfaces + +The resolved LLM API key never appears in any of these: + +1. **Task logs** — Airflow's secrets-masker (the operator calls `register_secret(key)` + before any logging or run, belt-and-braces over Airflow's auto-masking of a + Connection `password`). +2. **XCom** — `to_xcom()` carries tier **counts + sidecar paths only** (no secrets); the + key is held in a local, never stored on the operator/hook instance. +3. **Rendered templates** — `signalforge_conn_id` is **NOT** a `template_fields` entry + (templating a credential reference is an avoidable leak surface), so it never renders + into the task-instance rendered fields. +4. **`__repr__`** — `SignalForgeHook.__repr__` shows only the conn id; the resolution's + `HookResolution.__repr__` shows only `profiles_dir` + `provider` — never the key value + *or* a field-name label that would reveal a credential is present (mirrors + `SnowflakeAdapter.__repr__`). + +### In-process concurrency caveat (DEC-015) + +For `invocation="in_process"` (the default), the operator injects the provider's API-key +env var into `os.environ` only **around** the `run_signalforge` call and restores it in a +`finally` (absent-before → deleted; prior value → restored) — so the secret does not +linger in a long-lived worker across tasks. But because `os.environ` (and the in-process +stdout capture) is **process-global**, two concurrent in-process tasks in the *same* +worker can race on the injected key / captured output. **Prefer `invocation="subprocess"` +for concurrent multi-task workers** — it runs each task in a fresh interpreter with its +own environment. + +### Usage + +```python +from signalforge.airflow import ( + SignalForgeGenerateOperator, + SignalForgePruneExistingOperator, +) + +# Generate: needs provider + key + profiles_dir, all from the Connection. +generate = SignalForgeGenerateOperator( + task_id="drift_monitor", + project_dir="/opt/airflow/dbt_project", + signalforge_conn_id="signalforge_default", + select="tag:staging", + write=False, +) + +# PruneExisting: uses ONLY profiles_dir from the same Connection (no LLM key). +prune = SignalForgePruneExistingOperator( + task_id="signal_rot_monitor", + project_dir="/opt/airflow/dbt_project", + signalforge_conn_id="signalforge_default", + model="models/staging/stg_orders.sql", + schema="models/staging/schema.yml", +) +``` + +The shipped example DAG `examples/airflow/signalforge_hook_dag.py` +(`dag_id: signalforge_hook`) configures **both** operators via a single +`signalforge_conn_id` (+ the `signalforge_api_key` Variable) with **no inline per-task +env** — copy it as a starting point. + ## Scheduling for drift detection The example ships with `schedule=None` (manual trigger) so it never auto-spends on @@ -509,10 +674,11 @@ export AIRFLOW__CORE__DAGS_FOLDER="$(pwd)/examples/airflow" - **parse** — `DagBag` loads each shipped example with zero import errors and the expected tasks present: the two-`PythonOperator` pipeline (`signalforge_generate`), the - `SignalForgeGenerateOperator` drift monitor (`signalforge_generate_operator`), and the + `SignalForgeGenerateOperator` drift monitor (`signalforge_generate_operator`), the `SignalForgePruneExistingOperator` signal-rot monitor - (`signalforge_prune_existing_operator`). No credentials; runs in the gated CI `airflow` - job. + (`signalforge_prune_existing_operator`), and the Connection-configured both-operators + example (`signalforge_hook` — `drift_monitor` + `signal_rot_monitor` wired via + `signalforge_conn_id`). No credentials; runs in the gated CI `airflow` job. - **render** — each operator's `template_fields` Jinja-render from a synthetic task context (e.g. `{{ params.model }}` / `{{ ds }}`). - **live** — runs the `generate` task against an `init-demo` project; self-skips without diff --git a/examples/airflow/signalforge_hook_dag.py b/examples/airflow/signalforge_hook_dag.py new file mode 100644 index 00000000..19845eb5 --- /dev/null +++ b/examples/airflow/signalforge_hook_dag.py @@ -0,0 +1,169 @@ +"""Example Airflow DAG: Airflow-native credentials via ``signalforge_conn_id`` (#234). + +This is the **Connection-configured** integration shape (epic #228, US-007) — the +counterpart to the two single-operator examples +(``signalforge_generate_operator_dag.py`` / ``signalforge_prune_existing_operator_dag.py``), +which rely on ambient worker env. Here BOTH operators read their credentials from a +single Airflow **Connection** (plus an optional **Variable** for the LLM API key) +via the ``signalforge_conn_id`` param — so there is **NO inline ``ANTHROPIC_API_KEY`` +/ per-task env** on either task. This is the Airflow-native way to configure +SignalForge (Admin → Connections / Variables, or a secrets backend), keeping the +credential out of the DAG source and out of every task definition. + +## Connection + Variable setup this DAG assumes + +Create ONE Airflow Connection (Admin → Connections, or ``airflow connections add``) +with this id — both tasks point at it via ``signalforge_conn_id``: + +- **Conn Id:** ``signalforge_default`` (the ``_CONN_ID`` constant below). +- **Conn Type:** ``generic`` (any type works — the hook reads only ``password`` + + ``extra``). +- **Password:** the **LLM API key** (e.g. your Anthropic key). The Connection + ``password`` is auto-masked in Airflow logs, and the hook never logs / XCom's / + repr's it. The ``generate`` task needs this; the ``prune-existing`` task does NOT + (it makes no LLM call — see "Generate vs PruneExisting" below). +- **Extra (JSON):** the validated ``extra="forbid"`` schema — only these keys:: + + { + "profiles_dir": "/opt/airflow/dbt_project", + "provider": "anthropic", + "cache_scope": "project" + } + + ``profiles_dir`` (dir holding your dbt ``profiles.yml`` → ``--profiles-dir``), + ``provider`` (LLM SKU family — ``anthropic`` / ``openai`` / ``gemini``, picks the + env var the key is injected into), ``cache_scope`` (``per-model`` / ``project``, + Generate only). Any UNKNOWN key fails loud at resolution. Cost ceilings are + deliberately NOT in the ``extra`` (no CLI landing strip in v0.7) — set them in the + committed ``signalforge.yml grade:`` block instead. + +Optional Variable fallback for the API key (Admin → Variables, or +``airflow variables set``): key name **``signalforge_api_key``** (the +``API_KEY_VARIABLE_KEY`` constant). When the Connection ``password`` is empty the +hook falls back to this Variable. Use the Connection ``password`` as the primary +home for the key (it auto-masks); the Variable is the fallback. + +## Generate vs PruneExisting credentials + +Both tasks use the SAME ``signalforge_conn_id``, but they consume it differently +(DEC-016): + +- **``generate``** calls the LLM, so it REQUIRES ``provider`` + an API key + (``password`` or the Variable) AND uses ``profiles_dir`` for warehouse auth. +- **``prune-existing``** is read-only and makes **no LLM call**, so it uses **only** + ``profiles_dir`` from the Connection ``extra`` — ``provider`` / the key are + ignored on that path. A prune-existing-only deployment could use a Connection + with just ``{"profiles_dir": "..."}`` and no password/provider at all. + +## Secrets hygiene + +The resolved API key never appears in task logs (Airflow secrets-masker + +``password`` auto-masking), in XCom (counts + sidecar paths only), in rendered +templates (``signalforge_conn_id`` is NOT a ``template_fields`` entry), or in any +``__repr__``. For ``invocation="in_process"`` (the default) the key is injected +into ``os.environ`` only for the duration of the run and restored afterward; prefer +``invocation="subprocess"`` for concurrent multi-task workers (in-process shares +``os.environ`` + process-global stdout capture). Full walkthrough: docs/airflow-ops.md. +""" + +from __future__ import annotations + +from datetime import datetime + +from airflow import DAG + +from signalforge.airflow import ( + SignalForgeGenerateOperator, + SignalForgePruneExistingOperator, +) + +#: The Airflow Connection id BOTH operators resolve credentials from. Create a +#: Connection with this id (password = LLM API key; extra JSON = profiles_dir / +#: provider / cache_scope) — see the module docstring. +_CONN_ID = "signalforge_default" + +_VALID_ON_FLAGGED = ("fail", "skip", "succeed") + + +def _config(env_name: str, var_name: str, *, default: str) -> str: + """Resolve a config value: env override first, then Airflow Variable, then default. + + Env is read first so the value is available without a metadata-DB round-trip; + the Airflow ``Variable`` lookup is best-effort (skipped cleanly when no backend + is configured). A non-empty ``default`` guarantees both operators construct + validly at DAG-parse time even before an operator wires real config. NOTE: this + resolves NON-secret config only — the LLM API key is NOT read here; it comes + from the Connection (``password``) / the ``signalforge_api_key`` Variable via + the hook, never inline on a task. + """ + import os + + value = os.environ.get(env_name) + if not value: + try: + from airflow.models import Variable + + value = Variable.get(var_name, default_var=None) + except Exception: # noqa: BLE001 — no backend / not found → fall through + value = None + return value or default + + +_project_dir = _config( + "SF_PROJECT_DIR", "signalforge_project_dir", default="/opt/airflow/dbt_project" +) +_select = _config("SF_SELECT", "signalforge_select", default="tag:staging") +_model = _config("SF_MODEL", "signalforge_model", default="models/staging/stg_orders.sql") +_schema = _config("SF_SCHEMA", "signalforge_schema", default="models/staging/schema.yml") +_on_flagged = _config("SF_ON_FLAGGED", "signalforge_on_flagged", default="fail") +if _on_flagged not in _VALID_ON_FLAGGED: + # Fail loud at parse rather than silently defaulting — a typo'd policy is a + # config error the DAG author must see. + raise ValueError( + "signalforge_on_flagged / SF_ON_FLAGGED must be one of " + f"{'|'.join(_VALID_ON_FLAGGED)} (got {_on_flagged!r})" + ) + + +with DAG( + dag_id="signalforge_hook", + # Manual trigger by default so the example never auto-spends on credentials. + # For scheduled monitoring, set e.g. schedule="@daily". + schedule=None, + start_date=datetime(2026, 1, 1), + catchup=False, + params={"select": _select, "model": _model, "schema": _schema}, + tags=["signalforge", "dbt", "data-quality"], + doc_md=__doc__, +) as dag: + # `generate` — drift monitor. Credentials (LLM provider + key + profiles_dir) + # come entirely from the `signalforge_default` Connection (+ optional Variable) + # via signalforge_conn_id — NO inline ANTHROPIC_API_KEY / env on this task. + drift_monitor = SignalForgeGenerateOperator( + task_id="drift_monitor", + project_dir=_project_dir, + signalforge_conn_id=_CONN_ID, + # Templated: params-driven selector + the run's logical date as --as-of. + select="{{ params.select }}", + as_of="{{ ds }}", + # Read-only scheduled drift default: nothing is written; task state is the + # drift signal, governed by on_flagged. + write=False, + on_flagged=_on_flagged, # type: ignore[arg-type] + ) + + # `prune-existing` — no-LLM signal-rot monitor. Uses ONLY the profiles_dir from + # the SAME Connection (it makes no LLM call, so provider/key are ignored on this + # path) — likewise NO inline env on the task. + signal_rot_monitor = SignalForgePruneExistingOperator( + task_id="signal_rot_monitor", + project_dir=_project_dir, + signalforge_conn_id=_CONN_ID, + # Templated: params-driven model + schema path + the run's logical date. + model="{{ params.model }}", + schema="{{ params.schema }}", + as_of="{{ ds }}", + # Accepted for symmetry but inert: prune-existing does no grading, so a + # clean run is always SUCCESS regardless of on_flagged (#233 DEC-004). + on_flagged=_on_flagged, # type: ignore[arg-type] + ) diff --git a/tests/airflow/test_dag_parse.py b/tests/airflow/test_dag_parse.py index 3b43c3ab..9bf23315 100644 --- a/tests/airflow/test_dag_parse.py +++ b/tests/airflow/test_dag_parse.py @@ -34,10 +34,11 @@ def _load_example_dag(dag_id: str = "signalforge_generate"): from airflow.models.dagbag import DagBag - # The examples folder ships THREE DAGs (the two-PythonOperator pipeline - # example, the single-task generate drift monitor, and the no-LLM - # prune-existing signal-rot monitor); ALL must parse with no import errors - # regardless of which one the caller asked for. + # The examples folder ships FOUR DAGs (the two-PythonOperator pipeline + # example, the single-task generate drift monitor, the no-LLM prune-existing + # signal-rot monitor, and the Connection-configured both-operators hook + # example); ALL must parse with no import errors regardless of which one the + # caller asked for. bag = DagBag(dag_folder=str(_EXAMPLES_DIR), include_examples=False) assert bag.import_errors == {}, f"DAG import errors: {bag.import_errors}" # Read the in-memory parsed-DAG dict, NOT bag.get_dag(): get_dag() consults the @@ -173,6 +174,23 @@ def test_prune_existing_operator_renders_templated_fields() -> None: assert op.tests_dir is None +def test_hook_operator_example_dag_parses_without_import_errors() -> None: + """The Connection-configured both-operators example DAG parses cleanly via DagBag. + + Distinct ``dag_id`` from the single-operator examples; TWO tasks (the + ``SignalForgeGenerateOperator`` drift monitor + the + ``SignalForgePruneExistingOperator`` signal-rot monitor) both wired to ONE + ``signalforge_conn_id`` (+ the ``signalforge_api_key`` Variable) with no inline + per-task env — the #234 acceptance shape (A8). Parses with NO SignalForge config + in the env (the DAG's ``_config`` fallbacks keep both operators' construction-time + validation green at parse; the conn id is a literal constant, resolved only at + ``execute`` time, so parse needs no Connection backend). + """ + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + dag = _load_example_dag("signalforge_hook") + assert set(dag.task_ids) == {"drift_monitor", "signal_rot_monitor"} + + def _live_skip_reason() -> str | None: missing = [ v From bb3fa684c32103925976f784894a41badcefe96f Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 11:27:54 -0700 Subject: [PATCH 13/15] =?UTF-8?q?bd=5F1-scaffolding-qhi.8:=20Quality=20gat?= =?UTF-8?q?e=20=E2=80=94=20add=20signalforge=5Fconn=5Fid=20to=20operator?= =?UTF-8?q?=20param=20tables=20+=20explicit=20Generate=20conn=5Fid=3DNone?= =?UTF-8?q?=20no-hook=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/airflow-ops.md | 2 ++ tests/airflow/test_operators.py | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/docs/airflow-ops.md b/docs/airflow-ops.md index 89e3c55f..76312f40 100644 --- a/docs/airflow-ops.md +++ b/docs/airflow-ops.md @@ -260,6 +260,7 @@ and `--project-dir` are always injected by the runner. | `as_of` | `--as-of ` | reproducibility anchor for time-bound tests; `{{ ds }}` is a natural source. **`template_fields`** | | `on_flagged` | — (decision layer) | `fail` (default) / `skip` / `succeed`; see the **`on_flagged`** section below | | `invocation` | — (run mode) | `in_process` (default) / `subprocess`; see the **Invocation modes** section below | +| `signalforge_conn_id` | — (hook) | optional Airflow Connection id; resolves warehouse auth + LLM credentials via `SignalForgeHook` (see the **Airflow-native credentials** section). `None` (default) = ambient-env mode, byte-identical to prior behaviour. **Not** a `template_field` (secrets hygiene). | | `**kwargs` | — | passed to `BaseOperator` (`retries`, `retry_delay`, `depends_on_past`, …) | Exactly one of `model` or `select` must be set (a validation error fires at DAG-parse @@ -464,6 +465,7 @@ Every `__init__` kwarg and the `signalforge prune-existing` flag it maps to. | `tests_dir` | `--tests-dir ` | directory of singular-test `tests/*.sql` files to ingest too (DEC-006); omitted when unset. **`template_fields`** | | `on_flagged` | — (decision layer) | `fail` (default) / `skip` / `succeed`; **inert without grading** (DEC-004) | | `invocation` | — (run mode) | `in_process` (default) / `subprocess`; see the **Invocation modes** section above | +| `signalforge_conn_id` | — (hook) | optional Airflow Connection id; resolves **`profiles_dir` only** via `SignalForgeHook` (read-only — no LLM key; see the **Airflow-native credentials** section). `None` (default) = ambient-env mode, byte-identical to prior behaviour. **Not** a `template_field` (secrets hygiene). | | `**kwargs` | — | passed to `BaseOperator` (`retries`, `retry_delay`, `depends_on_past`, …) | The six `template_fields` (`project_dir`, `model`, `schema`, `profiles_dir`, `as_of`, diff --git a/tests/airflow/test_operators.py b/tests/airflow/test_operators.py index e8a21aff..8579a073 100644 --- a/tests/airflow/test_operators.py +++ b/tests/airflow/test_operators.py @@ -490,6 +490,27 @@ def test_single_model_conn_id_injects_env_masks_key_and_restores( assert argv[argv.index("--profiles-dir") + 1] == "/conn/profiles" +def test_single_model_conn_id_none_unchanged_no_hook( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """conn_id=None (#232 default): no hook touched, no conn-derived argv.""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + + def _boom(_conn_id: str) -> HookResolution: + raise AssertionError("_resolve_hook must NOT be called when conn_id is None") + + monkeypatch.setattr("signalforge.airflow.operators._resolve_hook", _boom) + captured = _patch_run(monkeypatch, [_result(exit_code=0, flagged=0)]) + + op = _operator_class()(task_id="gen", project_dir="/proj", model="m") + op.execute(context={}) + + # Hook never called (the _boom guard); argv carries no conn-derived profiles_dir. + argv = captured[0] + assert argv[:2] == ["generate", "m"] + assert "--profiles-dir" not in argv + + def test_single_model_conn_env_restored_on_exception( monkeypatch: pytest.MonkeyPatch, ) -> None: From 5eb6f953c5aa9818c8f63917ff2e247017a8fb13 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 11:30:53 -0700 Subject: [PATCH 14/15] bd_1-scaffolding-qhi.9: document SignalForgeHook in airflow-integration.md (Patterns & Memory) --- .claude/rules/airflow-integration.md | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/.claude/rules/airflow-integration.md b/.claude/rules/airflow-integration.md index 7089d7cc..e7b0c83b 100644 --- a/.claude/rules/airflow-integration.md +++ b/.claude/rules/airflow-integration.md @@ -13,7 +13,7 @@ The integration splits cleanly so the "act on the graded diff" logic is unit-tes - `run_signalforge(argv, *, project_dir, invocation="in_process"|"subprocess", timeout_seconds=None) -> SignalForgeRunResult` — builds/normalises argv, runs the pipeline, parses the result. Does NOT compute the outcome (the operator calls `decide_task_outcome`). - **Shim-confined translator** — `_airflow_compat.raise_for_outcome(outcome, *, message)`. The ONLY new `from airflow.exceptions import ...` site (lazy, inside the body, `# type: ignore[import-not-found]`, `# pragma: no cover`), per the one-shim-per-vendor rule (`llm-drafter.md` §"One SDK seam"). Maps `FAIL_NO_RETRY`→`AirflowFailException` (no retry), `SKIP`→`AirflowSkipException`, `FAIL_RETRYABLE`→`AirflowException` (retryable — Airflow's `retries`/`retry_delay` apply), `SUCCESS`→return. Deliberately NOT re-exported from the package top — the operator calls it via `_airflow_compat`. -**Rule for the remaining epic-#228 children (hook, drift — `SignalForgeGenerateOperator` landed in #232, `SignalForgePruneExistingOperator` in #233):** put the decision in the pure core, the airflow-exception raise in the shim. Never re-derive the outcome by string-matching; carry the typed `TaskOutcome`. Never grow a fifth exit tier. +**Rule for the remaining epic-#228 children (drift — `SignalForgeGenerateOperator` landed in #232, `SignalForgePruneExistingOperator` in #233, `SignalForgeHook` in #234):** put the decision in the pure core, the airflow-exception raise in the shim. Never re-derive the outcome by string-matching; carry the typed `TaskOutcome`. Never grow a fifth exit tier. ## Exit → TaskOutcome → Airflow (the #231 contract table) @@ -84,10 +84,25 @@ The second operator (sibling of #232), wrapping the no-LLM, read-only `signalfor Example DAG: `examples/airflow/signalforge_prune_existing_operator_dag.py` (`dag_id="signalforge_prune_existing_operator"`, single `signal_rot_monitor` task, templated `model`/`schema`/`as_of`, no Anthropic key referenced). Gated parse + `render_template_fields` tests in `tests/airflow/test_dag_parse.py`. +## `SignalForgeHook` (#234 DEC-001…016) + +The Airflow-native credential seam: a `SignalForgeHook(BaseHook)` keyed on a `signalforge_conn_id` so a DAG author configures SignalForge from one Airflow Connection (+ optional Variable) instead of inline per-task env. It **extends the two-layer split** (airflow-free core + shim-confined translator) to credential resolution, and threads `signalforge_conn_id` through BOTH operators. The durable patterns: + +- **Airflow-free pure resolver is the heart (`_resolve.py`).** `resolve_connection(conn, *, variable_lookup, project_dir=None) -> HookResolution` is duck-typed (`conn.password` + `conn.extra_dejson`) with an INJECTED `variable_lookup` callable — so it carries NO `from airflow` import and is 100%-covered UNGATED. The gated `SignalForgeHook.get_conn()` is a thin wrapper that calls `self.get_connection(...)` + wires `variable_lookup` to `_airflow_compat.airflow_variable_get`, then delegates. **Mirror this for any future credential/config resolution: put the logic in a duck-typed, callable-injected pure function; keep the airflow-touching wrapper a thin gated shim.** +- **The resolver is LENIENT; the consumer enforces requiredness (DEC-003).** `HookResolution(profiles_dir, provider, api_key, cache_scope)` (frozen, redacting `__repr__` — never the key or a field-name label) returns `None` for absent fields rather than raising. The **Generate** operator REQUIRES `provider`+`api_key` (raises `AirflowConfigError` else — it makes LLM calls); the **PruneExisting** operator requires NEITHER (DEC-016 — read-only, no LLM call, uses only `profiles_dir`). One lenient resolver serving two consumers with different needs is what avoids a second resolver. `_ConnectionExtra` is `extra="forbid"` + frozen, all-optional (`profiles_dir`/`provider`/`cache_scope`) — a typo key fails loud. +- **`mask_secret` + `Variable.get` join the one shim (`_airflow_compat`).** `register_secret(value)` (lazy `from airflow.utils.log.secrets_masker import mask_secret` — stable across `apache-airflow>=2.8,<3`) and `airflow_variable_get(key) -> str | None` (coerces `Variable.get`'s `Any` to `str | None` so pyright doesn't widen) are the only new `from airflow` sites — confined to the shim, `# pragma: no cover` + `# type: ignore[import-not-found]`, the import-confinement scan unchanged. `hooks.py` carries NO module-scope airflow import (deferred-class construction, verbatim from #232: `__getattr__` + `functools.cache` + `find_spec("airflow")` + airflow-free `_SignalForgeHookAirflowMissing` placeholder). +- **Closed `PROVIDER_ENV_VAR_KEYS` allowlist (DEC-005), home in `signalforge.llm.providers`.** `{anthropic: ANTHROPIC_API_KEY, openai: OPENAI_API_KEY, gemini: GOOGLE_API_KEY}` — sibling to `PROVIDER_DEFAULT_MODELS`/`PROVIDER_SKU_PREFIXES`, reusable by the v0.8 GH Action. Validated whenever `provider` is present (regardless of consumer): an unknown provider raises `AirflowConfigError` — the operator never derives an arbitrary env-var name from operator-supplied config (closes the arbitrary-env-var injection vector). `gemini → GOOGLE_API_KEY` (NOT `GEMINI_*`). +- **The four leak-surface disciplines are each test-pinned (DEC-006/007).** (1) **Logs** — lazy-format `json.dumps` (the grep-gate now scans `signalforge.airflow`, DEC-013) carrying conn_id/provider/`profiles_dir_set` bool, never the key; `register_secret` called BEFORE any log/run. (2) **XCom** — counts + sidecar paths only; key held in a local, never on `self`, never returned. (3) **Rendered templates** — `signalforge_conn_id` is deliberately NOT in either operator's `template_fields`. (4) **`__repr__`** — hook + `HookResolution` redact. `mask_secret` is log-only (does NOT scrub XCom/templates) — the structural disciplines (2)(3)(4) are independent and necessary. +- **Env injection at the operator seam, restored in `finally` (DEC-014/015).** Generate's `execute()` wraps `run_signalforge` in `_provider_key_env(env_var, api_key)` — a pure (airflow-free, ungated-testable) `os.environ` snapshot→inject→restore context manager (absent-before→delete-after; prior→restore, incl. empty-string; restores on exception). The runner's `_ISOLATED_ENV_KEYS` is UNCHANGED — the key restore belongs at the operator, not the runner. `invocation` default stays `in_process`; docs flag `subprocess` as the safe choice for concurrent multi-task workers (in-process shares `os.environ` + process-global stdout capture). `signalforge_conn_id=None` (default) ⇒ byte-identical to #232/#233. +- **Shared helpers, reused not duplicated.** `_merge_with_resolution(param, extra)` (precedence param > Connection extra > default, DEC-012), `_resolve_hook(conn_id)`, and `_provider_key_env` are module-level in `operators.py`; PruneExisting reuses the first two for its `profiles_dir`-only path (NOT `_provider_key_env` — no key injection). **Decouple the profiles_dir-precedence piece from the key-injection piece so a no-LLM consumer reuses only what it needs.** +- **On-disk profiles only for v0.7 (DEC-001); cost ceilings NOT in the Connection `extra` (DEC-011).** The hook resolves a `profiles_dir` → `--profiles-dir` (the existing `load_profile`/`from_profile` seam); synthesizing a `profiles.yml`/`DbtProfileTarget` from an Airflow Connection is deferred (re-deriving the #120 per-type validator is its own ticket). Cost ceilings have no CLI landing strip (#232 DEC-002 deferred `config_overrides`) so they are deliberately absent from `extra` — operators set them via the committed `signalforge.yml grade:` block. `AirflowConfigError` (tier 2) covers all hook misconfig — no new error class, no scan-7 churn. + +Example DAG: `examples/airflow/signalforge_hook_dag.py` (`dag_id="signalforge_hook"`, both operators via one `signalforge_conn_id`, no inline per-task env). Pure resolver tests ungated in `tests/airflow/test_resolve.py`; gated hook/operator/dag-parse tests certified against the real `.venv-airflow` rig (airflow 2.10.4). + ## v0.8 note `run_signalforge` is airflow-free and meant for the v0.8 GitHub Action too. If/when that lands, consider hoisting `result.py`/`runner.py` to a neutral package (e.g. `signalforge.automation`) so the GH Action doesn't import from a package named `airflow`. Out of scope for v0.7 — the modules are airflow-free so `import signalforge.airflow.runner` works without airflow today. ## Reference -`plans/super/233-prune-existing-operator.md` — DEC-001…DEC-008 (`SignalForgePruneExistingOperator`, the no-LLM sibling). `plans/super/232-generate-operator.md` — DEC-001…DEC-011 (`SignalForgeGenerateOperator`). `plans/super/231-result-task-state.md` — DEC-001…DEC-008. `plans/super/230-airflow-skeleton.md` — skeleton wiring. `docs/airflow-ops.md` — operator-facing contract + example DAGs. `src/signalforge/airflow/{operators,result,runner,_airflow_compat,__init__}.py`, `src/signalforge/__main__.py`. `examples/airflow/{signalforge_generate_operator_dag,signalforge_prune_existing_operator_dag}.py`. `tests/airflow/{test_operators,test_operators_helpers,test_dag_parse}.py`. See-Also: `cli-layer.md` (four-tier exit codes, the no-5th-tier rule, the `[airflow]` `errors.py`), `python-build.md` (`[airflow]` extra out of the dev group), `llm-drafter.md` (one-shim-per-vendor), `grade-layer.md`/`warehouse-adapters.md` (fail-soft vs fail-closed posture). +`plans/super/234-signalforge-hook.md` — DEC-001…DEC-016 (`SignalForgeHook`, Connection/Variable → profiles.yml + LLM key). `plans/super/233-prune-existing-operator.md` — DEC-001…DEC-008 (`SignalForgePruneExistingOperator`, the no-LLM sibling). `plans/super/232-generate-operator.md` — DEC-001…DEC-011 (`SignalForgeGenerateOperator`). `plans/super/231-result-task-state.md` — DEC-001…DEC-008. `plans/super/230-airflow-skeleton.md` — skeleton wiring. `docs/airflow-ops.md` — operator-facing contract + example DAGs (incl. the Airflow-native credentials section). `src/signalforge/airflow/{operators,hooks,result,runner,_resolve,_airflow_compat,__init__}.py`, `src/signalforge/__main__.py`, `src/signalforge/llm/providers.py` (`PROVIDER_ENV_VAR_KEYS`). `examples/airflow/{signalforge_generate_operator_dag,signalforge_prune_existing_operator_dag,signalforge_hook_dag}.py`. `tests/airflow/{test_operators,test_operators_helpers,test_hooks,test_resolve,test_dag_parse}.py`. See-Also: `cli-layer.md` (four-tier exit codes, the no-5th-tier rule, the `[airflow]` `errors.py`), `python-build.md` (`[airflow]` extra out of the dev group), `llm-drafter.md` (one-shim-per-vendor), `grade-layer.md`/`warehouse-adapters.md` (fail-soft vs fail-closed posture, `__repr__` redaction). From 5c21d0aebb222656f595c9ed065d75253a4e5e83 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 12:02:30 -0700 Subject: [PATCH 15/15] =?UTF-8?q?#234:=20Address=20PR=20review=20=E2=80=94?= =?UTF-8?q?=20thread=20operator=20project=5Fdir=20into=20hook=20resolution?= =?UTF-8?q?=20(DEC-008=20containment)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/signalforge/airflow/hooks.py | 20 +++++++++++------- src/signalforge/airflow/operators.py | 16 ++++++++++---- tests/airflow/test_operators.py | 31 +++++++++++++++++++++++++--- 3 files changed, 52 insertions(+), 15 deletions(-) diff --git a/src/signalforge/airflow/hooks.py b/src/signalforge/airflow/hooks.py index 6dbdeebd..8db06651 100644 --- a/src/signalforge/airflow/hooks.py +++ b/src/signalforge/airflow/hooks.py @@ -55,7 +55,7 @@ class SignalForgeHook: # noqa: D401 - type stub only def __init__(self, *args: Any, **kwargs: Any) -> None: ... - def get_conn(self) -> HookResolution: ... + def get_conn(self, *, project_dir: str | None = None) -> HookResolution: ... class _SignalForgeHookAirflowMissing: @@ -109,18 +109,22 @@ def __init__(self, signalforge_conn_id: str, **kwargs: Any) -> None: super().__init__(**kwargs) self.signalforge_conn_id = signalforge_conn_id - def get_conn(self) -> HookResolution: + def get_conn(self, *, project_dir: str | None = None) -> HookResolution: """Resolve the configured Connection to a :class:`HookResolution`. - ``project_dir`` is ``None`` at the hook layer: the hook has no - project anchor to symlink-contain an ``extra.profiles_dir`` against - (DEC-008's bounded-defence gap). The consuming operator — which DOES - know its ``project_dir`` — supplies the containment anchor when it - calls the resolver itself (US-005/US-006). + ``project_dir`` is the symlink-containment anchor for an + ``extra.profiles_dir`` (DEC-008). The consuming operator — which + knows its ``project_dir`` — passes it through (US-005/US-006) so a + Connection-supplied ``profiles_dir`` is canonicalised + contained + against the project tree. A standalone hook call without an anchor + (``project_dir=None``) accepts ``profiles_dir`` as-is (the documented + bounded-defence gap for direct library use). """ conn = self.get_connection(self.signalforge_conn_id) variable_lookup = _airflow_compat.airflow_variable_get - return resolve_connection(conn, variable_lookup=variable_lookup, project_dir=None) + return resolve_connection( + conn, variable_lookup=variable_lookup, project_dir=project_dir + ) def __repr__(self) -> str: # Leak-surface discipline (DEC-007): show only the conn id, never the diff --git a/src/signalforge/airflow/operators.py b/src/signalforge/airflow/operators.py index 629127b0..888022a2 100644 --- a/src/signalforge/airflow/operators.py +++ b/src/signalforge/airflow/operators.py @@ -141,7 +141,9 @@ def _provider_key_env(env_var: str, api_key: str) -> Iterator[None]: os.environ[env_var] = prior -def _resolve_hook(conn_id: str) -> HookResolution: # pragma: no cover - needs [airflow] +def _resolve_hook( # pragma: no cover - needs [airflow] + conn_id: str, project_dir: str | None = None +) -> HookResolution: """Construct the SignalForge hook and resolve its Connection (gated helper). Reused by both operators' ``execute`` (US-005 / US-006). Constructing @@ -151,10 +153,16 @@ def _resolve_hook(conn_id: str) -> HookResolution: # pragma: no cover - needs [ reason as the operator factories. The hook import is lazy — its module-``__getattr__`` resolves the real class without eagerly importing airflow — so ``import signalforge.airflow.operators`` stays airflow-free. + + ``project_dir`` is the operator's project root: it is threaded into + ``get_conn`` so a Connection-supplied ``extra.profiles_dir`` is + symlink-contained against the project tree (DEC-008). The operator always + knows its ``project_dir``, so the containment anchor is always supplied on + the operator path. """ from signalforge.airflow.hooks import SignalForgeHook - return SignalForgeHook(conn_id).get_conn() + return SignalForgeHook(conn_id).get_conn(project_dir=project_dir) def _build_generate_argv( @@ -665,7 +673,7 @@ def execute(self, context: Any) -> dict[str, object]: env_cm: contextlib.AbstractContextManager[None] = contextlib.nullcontext() if self.signalforge_conn_id is not None: - resolution = _resolve_hook(self.signalforge_conn_id) + resolution = _resolve_hook(self.signalforge_conn_id, self.project_dir) # ``generate`` calls the LLM, so provider + key are REQUIRED here # (DEC-003 — the resolver is lenient; the consumer enforces # requiredness). PruneExisting, by contrast, needs neither. @@ -965,7 +973,7 @@ def execute(self, context: Any) -> dict[str, object]: # raises inside the resolver per DEC-005). effective_profiles_dir = self.profiles_dir if self.signalforge_conn_id is not None: - resolution = _resolve_hook(self.signalforge_conn_id) + resolution = _resolve_hook(self.signalforge_conn_id, self.project_dir) # Precedence merge (DEC-012): explicit param > Connection extra. effective_profiles_dir = _merge_with_resolution( self.profiles_dir, resolution.profiles_dir diff --git a/tests/airflow/test_operators.py b/tests/airflow/test_operators.py index 8579a073..ed2e359e 100644 --- a/tests/airflow/test_operators.py +++ b/tests/airflow/test_operators.py @@ -451,7 +451,7 @@ def _patch_hook(monkeypatch: pytest.MonkeyPatch, resolution: HookResolution) -> masked: list[str] = [] monkeypatch.setattr( "signalforge.airflow.operators._resolve_hook", - lambda conn_id: resolution, + lambda conn_id, project_dir=None: resolution, ) monkeypatch.setattr( "signalforge.airflow.operators.register_secret", @@ -496,7 +496,7 @@ def test_single_model_conn_id_none_unchanged_no_hook( """conn_id=None (#232 default): no hook touched, no conn-derived argv.""" pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) - def _boom(_conn_id: str) -> HookResolution: + def _boom(_conn_id: str, _project_dir: str | None = None) -> HookResolution: raise AssertionError("_resolve_hook must NOT be called when conn_id is None") monkeypatch.setattr("signalforge.airflow.operators._resolve_hook", _boom) @@ -511,6 +511,31 @@ def _boom(_conn_id: str) -> HookResolution: assert "--profiles-dir" not in argv +def test_single_model_conn_id_threads_project_dir_anchor( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """The operator passes its ``project_dir`` to ``_resolve_hook`` so a + Connection ``extra.profiles_dir`` is symlink-contained (DEC-008).""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + + seen: list[str | None] = [] + + def _capture(_conn_id: str, project_dir: str | None = None) -> HookResolution: + seen.append(project_dir) + return HookResolution(profiles_dir=None, provider="anthropic", api_key="sk-x") + + monkeypatch.setattr("signalforge.airflow.operators._resolve_hook", _capture) + monkeypatch.setattr("signalforge.airflow.operators.register_secret", lambda value: None) + _patch_run(monkeypatch, [_result(exit_code=0, flagged=0)]) + + op = _operator_class()(task_id="gen", project_dir="/proj", model="m", signalforge_conn_id="sf") + op.execute(context={}) + + # The operator's project_dir reached the resolver as the containment anchor. + assert seen == ["/proj"] + + def test_single_model_conn_env_restored_on_exception( monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -933,7 +958,7 @@ def test_prune_existing_conn_id_none_unchanged_no_hook( """conn_id=None (#233 default): no hook touched, argv byte-identical to #233.""" pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) - def _boom(_conn_id: str) -> HookResolution: + def _boom(_conn_id: str, _project_dir: str | None = None) -> HookResolution: raise AssertionError("_resolve_hook must NOT be called when conn_id is None") monkeypatch.setattr("signalforge.airflow.operators._resolve_hook", _boom)