From d21d6883c57d7e0b23af2723a12ef318fd909f57 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 12:22:17 -0700 Subject: [PATCH 01/13] Add super plan for #235: Airflow drift / signal-rot detection (plan) --- plans/super/235-drift-detection.md | 392 +++++++++++++++++++++++++++++ 1 file changed, 392 insertions(+) create mode 100644 plans/super/235-drift-detection.md diff --git a/plans/super/235-drift-detection.md b/plans/super/235-drift-detection.md new file mode 100644 index 00000000..b0be6d09 --- /dev/null +++ b/plans/super/235-drift-detection.md @@ -0,0 +1,392 @@ +# Super Plan — #235: Airflow drift / signal-rot detection (run-over-run) + +## Meta +- **Ticket:** #235 — Airflow: drift / signal-rot detection mode (run-over-run). Part of epic #228 (v0.7 Airflow). Depends on #231 (result/XCom contract — landed) and #232 (`SignalForgeGenerateOperator` — landed). +- **Phase:** detailing (awaiting approval to devolve) +- **Branch:** `feature/235-drift-detection` (worktree `/home/wesd/Projects/worktrees/SignalForge/235-drift-detection`, base `dev`). +- **Sessions:** 1 (2026-06-16). + +--- + +## Discovery + +### What +Add **run-over-run drift detection** to the Airflow integration: compare a SignalForge run (N) +against its prior run (N-1) and emit a structured, JSON-serialisable **drift report** delta. The +headline signal is **signal rot** — a test that *used to* catch failing rows (`kept`) but now +**always-passes** (`dropped: always-passes`), i.e. the column it guarded went all-clean / the rule +went vacuous. That transition is a schema-drift alarm worth paging on (Architectural Commitment #1: +an always-pass test is noise; a test that *rotted into* always-pass is a drift signal). + +The report also surfaces: tier transitions either direction (`newly_dropped` / `newly_kept`), +**grade regressions** (mean grade fell beyond a threshold), and **schema-shape changes** (columns +added/removed/retyped). It is pushed to XCom and can `fail` / `skip` / `succeed` the Airflow task +via an `on_drift` policy — the run-over-run analogue of #231's `on_flagged`. + +### Why +Running `generate` nightly produces a graded diff each night, but the *interesting* signal is +**change over time** — a single run's sidecar can't see it. This is the feature that turns a +SignalForge DAG from "run generate nightly" into a **schema-drift / signal-rot monitor**. It is the +explicit headline scheduled value-add of epic #228. + +### Key codebase findings (from research) + +**The two input sidecars (what we read back & diff):** +- `DiffReport` (`src/signalforge/diff/models.py:206`) — `model_dump_json(by_alias=True)` lands at + `.signalforge/diff.json`. Fields we consume: `entries: tuple[DiffEntry, ...]`, the four count + fields, `model_unique_id`, `audit_schema_version: Literal[3]`, the three `blake2b-8` reproducibility + hashes (`candidate_hash` / `prune_result_hash` / `grading_report_hash`). +- `DiffEntry` (`:142`) — `artifact_id: str`, `test_type: str | None`, `tier: Tier`, + `drop_reason: DropReason | None`, `why: str`, `score: float | None`, `passed: bool | None`. +- `Tier = Literal["kept","kept-uncertain","dropped","flagged"]` (`diff/models.py:62`). +- `DropReason = Literal["always-passes","requires-future-data","failed-on-known-clean-data","kept","kept-without-evidence"]` + (`prune/models.py:55`). +- `GradingReport` (`grade/models.py:182`) — `.signalforge/grade.json`; `mean_score` and `pass_rate` + are `@computed_field` properties; carries `model_unique_id`, `rubric_hash`, `results`. +- **`artifact_id` encodes the column** (`column..description`, `test.column..`, …) so + the **column SET** (add/remove) is derivable from the union of artifact_ids across the two reports. + **Column TYPES are NOT in the sidecar** — a retype can't be computed from `diff.json` alone. + +**The #231 airflow-free seam we extend:** +- `SignalForgeRunResult` (frozen dataclass, `airflow/result.py:55`) — counts + `mean_grade` + + `diff_sidecar_path` / `grade_sidecar_path` + `to_xcom()` (counts + paths only). `below_threshold` + is a read-only property `= flagged > 0`. +- `TaskOutcome(StrEnum)` = `SUCCESS / SKIP / FAIL_NO_RETRY / FAIL_RETRYABLE` (a SEPARATE axis from + the 4-tier exit code — NOT a 5th tier). +- `decide_task_outcome(result, *, on_flagged: OnFlagged = "fail") -> TaskOutcome` (`:110`) — pure, + no I/O, no airflow. `OnFlagged = Literal["fail","skip","succeed"]` (`:37`). +- `run_signalforge(argv, *, project_dir, invocation="in_process", timeout_seconds=None)` (`runner.py:324`) + — reads diff JSON off **stdout**, `grade.json` from disk when present; sidecar reads are fail-soft. +- `raise_for_outcome(outcome, *, message)` — the ONLY `from airflow ...` site, in `_airflow_compat`. + +**The #232 operator pattern we mirror:** +- Pure helpers (`_build_generate_argv` / `_validate_operator_config` / `_resolve_select_models` / + `_aggregate_batch_result`) tested UNGATED; deferred class via module `__getattr__` + + `find_spec("airflow")` branch + `functools.cache`d factory + airflow-free placeholder; `execute()` + is a thin `# pragma: no cover` wire-up. `template_fields = ("project_dir","select","model","profiles_dir","as_of")`. + `__init__` already takes `as_of`, `on_flagged`, `invocation`. +- `--as-of` (`cli/generate.py:459`, `type=date.fromisoformat`) is threaded as a stringified argv token + (`_build_generate_argv(..., as_of=...)`). It is the **reproducibility carve-out** (#171) the ticket + cites for time-bound determinism. + +**Errors / tests:** +- `AirflowConfigError(AirflowIntegrationError)` — tier 2; base renders `↳ Remediation:`. No new error + class needed unless a genuinely new failure mode appears. +- Airflow tests: `pytestmark = pytest.mark.airflow` + in-test `pytest.importorskip("airflow")`; pure + helpers tested ungated; committed sidecar fixtures under `tests/fixtures/diff/` and `tests/fixtures/grade/`. + +### Rules constraints in force (from `.claude/rules/`; no `workflow-project.md` exists) + +**airflow-integration.md (primary):** +- Pure airflow-free core decides; shim raises. The drift comparison is a pure function; the + task-state raise stays in `_airflow_compat.raise_for_outcome`. +- `on_drift` is the run-over-run analogue of `on_flagged` — key it on a **drift property of an exit-0 + run**, NOT on the exit code. **Never grow a 5th exit tier.** `TaskOutcome` stays 4-valued. +- XCom hygiene: counts/paths/summary only — never bulk sidecar text, never secrets. +- A dedicated operator (if chosen) follows the deferred-class + pure/gated split verbatim, and needs + an UNGATED skeleton test (placeholder `__init__` raises, `find_spec is None` factory branch, + `__getattr__` arm) or the codecov patch gate fails. +- Certify airflow-touching paths against `.venv-airflow` (#229) before closing. + +**diff-renderer.md / prune-engine.md:** +- Reuse the existing 4 `tier` literals + 5 `DropReason` literals — the drift report is computed + *from* that taxonomy, not a new classification. The drift report is a NEW model, not a tier-shaped + thing. +- Reproducibility: same two sidecars → byte-identical drift report. Carry the two input + `blake2b-8` hashes so the report is self-describing. +- If a `drift.json` sidecar is written: it's a fail-closed writer (6th) — pre-size-check, `O_TRUNC`, + single `os.write`, `os.fsync`, no `except` around write/fsync; AST scan 5 pins the `try/finally` + shape; symlink-hardened canonicalisation at the orchestrator. + +**business-rule-tests.md:** `--as-of` reproducibility carve-out is the precedent for time-bound +determinism — surface it so a drift comparison is reproducible at `(model, as_of)`. + +**cli-layer.md:** four-tier taxonomy; any new typed error registers in `_EXCEPTION_TO_EXIT_CODE` +(scan-7) — or inherit via MRO if it subclasses `AirflowIntegrationError`. 5-surface parity for any +new flag/param. `errors.py` count is 14 (scan-7). + +**testing-signal.md:** determinism via committed fixture PAIRS (run N-1, run N) engineered so each +transition class is mathematically guaranteed; drift-detector (`extra="forbid"` strict mirror + +fixture) for any read-back model; planted-violation self-check for any AST/source-scan gate; no +`assert True` tests. + +**python-build.md:** `[airflow]` extra stays OUT of the dev group; base install airflow-free; the +comparison core carries no `from airflow` import. + +**docs-publishing.md:** document in `docs/airflow-ops.md` (ticket __A8__); new top-level section may +need a `nav:` entry in `mkdocs.yml`. + +### Hard technical constraint surfaced in discovery +**`schema_shape_changes` cannot fully come from the two diff sidecars.** Column SET (add/remove) is +derivable from `artifact_id` prefixes; column TYPE changes are not (sidecars carry no types). Options +are folded into the scoping questions (Q4): derive add/remove only (defer retype), persist a small +shape snapshot alongside the sidecar, or defer schema-shape entirely for v0.7. + +--- + +## Scoping questions (Phase 1) — ANSWERED 2026-06-16 +- **Q1 Surface form → BOTH.** `detect_drift_against` flag on `SignalForgeGenerateOperator` (ergonomic + default) PLUS a dedicated `SignalForgeDriftOperator` (explicit/branchable, downstream). Pure + compare-logic shared between them. +- **Q2 Prior-run source → TEMPLATED PATH.** Operator takes `detect_drift_against: str` = path to the + prior `diff.json` (Jinja-templatable, e.g. `{{ prev_ds }}`); grade.json sibling auto-located. Each + run persists its current sidecar to a per-run path so tomorrow's run finds it. +- **Q3 on_drift trigger → ALARM SUBSET.** Trip only on `newly_always_passes` (signal rot) + + `grade_regressions`. All categories reported; `newly_kept` / `newly_dropped` / `schema_shape_changes` + are informational and don't page. +- **Q4 schema_shape scope → ADD/REMOVE ONLY.** Derive column add/remove from `artifact_id` prefixes + across the two sidecars. Defer retype detection (sidecars carry no types). Zero extra persisted state. + +### Discovery-confirmed mechanics (architecture-critical) +- **`artifact_id` is a stable cross-run join key** — pure dotted paths, no `run_id` embedded + (`_common/artifact_id.py`). Column name embedded ⇒ column SET derivable from prefixes (validates Q4). +- **Current DiffReport is always on `result.stdout`** — `run_signalforge` parses it internally + (`runner.py:64`) but returns only counts. Under the default `write=False`→`--dry-run` there is NO + on-disk current sidecar, but the JSON is on stdout (#231 DEC-005). ⇒ the drift core takes parsed + `DiffReport` objects; the current one is re-parsed from stdout, the prior loaded from the path. +- **Persisting "today for tomorrow" reuses the existing fail-closed `diff._sidecar.write_sidecar`** — + no new fail-closed writer, no new AST scan (honors "no new audit-event class"). +- **Logger grep gate does NOT cover `airflow/`** — drift logging follows lazy-format convention for + hygiene but is not gated. + +--- + +## Architecture Review (Phase 2) + +| Area | Rating | Finding | +|---|---|---| +| Security | **pass** | No network/DB/secrets. Reads two local JSON files. The operator-supplied `detect_drift_against` path needs symlink-hardened `canonicalise_path` + a size cap on read (mirror diff's 10 MB / ingest's 5 MB). **Containment anchor is a concern, not a blocker** — the prior path may legitimately sit OUTSIDE `project_dir` (a history dir / object-store mount), so the project-dir containment used elsewhere can't apply verbatim → see DEC. | +| Performance | **pass** | Two JSON parses + O(artifacts) dict join. Trivial. Size cap on the prior-file read guards a hostile/huge file. | +| Data Model | **concern** | New read-back-able `DriftReport` Pydantic model ⇒ needs a `Strict(extra="forbid")` drift detector + committed fixture. Open: field set, transition-category representation, grade-regression shape + threshold, schema-shape (add/remove) shape, baseline (empty) shape, the two input `blake2b-8` hashes. → refinement DECs. | +| API Design | **concern** | `decide_task_outcome` gains `on_drift`; an exit-0 run can be BOTH flagged AND drifted → precedence rule needed. `compute_drift(...)` signature. Operator params + a SECOND deferred operator class. These are operator KWARGS (not CLI flags) → "5-surface parity" reduces to operator docstring + `airflow-ops.md` + tests + plan DEC (no argparse/SKILL surface). → refinement DECs. | +| Observability | **pass (minor)** | INFO "baseline established" on no-prior; INFO/structured log on drift detected (categories + counts) via lazy-format JSON. Airflow pkg not under the grep gate; follow convention anyway. | +| Testing Strategy | **concern** | `compute_drift` + `DriftReport` + the `decide_task_outcome` extension are airflow-free ⇒ 100% ungated coverage (codecov patch gate). The deferred `SignalForgeDriftOperator` needs an UNGATED skeleton test (placeholder `__init__` raises, `find_spec is None` factory branch, `__getattr__` arm) + a no-eager-import pin. Engineered fixture PAIRS (run N-1, N) — one per transition class. → story design. | +| Drift correctness (project-specific) | **concern** | Join/transition semantics: flagged↔kept transitions, kept-uncertain handling, artifacts present in only one run (test added/removed), model-set mismatch (`model_unique_id` differs), `--no-grade` ⇒ no grade.json ⇒ `grade_regressions` unavailable (degrade, don't fail). These become DECs. | + +**No blockers.** Six concerns, all resolvable in refinement (they become DEC-### below). The design rides +the established #231/#232 airflow-free-core + deferred-operator pattern; nothing here requires inventing +a new architectural seam. + +--- + +## Refinement Log (Phase 3) — ANSWERED 2026-06-16 +- **on_flagged vs on_drift → MOST-SEVERE WINS.** Independent knobs; the worse verdict governs the + single task state. Severity rank `FAIL_NO_RETRY (3) > SKIP (2) > SUCCESS (1)` (combination only ever + happens among these three on an exit-0 run; the 1/2/3 exit tiers short-circuit before either policy). +- **Grade-regression threshold → 0.05 default, operator-tunable** (`grade_regression_threshold`). +- **Comparison-failure → DEGRADE, NEVER FAIL** (baseline / model-mismatch / corrupt prior / `--no-grade`). +- **Delivery → ONE plan, sequenced Ralph stories, merged to `dev`** (epic-#228 convention, no GitHub PR). + +--- + +## Decisions + +- **DEC-001 — Surface form: BOTH.** A `detect_drift_against` flag on `SignalForgeGenerateOperator` + (run + compare in one task — the ergonomic default) AND a dedicated `SignalForgeDriftOperator` + (reads two sidecars downstream, explicit/branchable). Both wrap the same airflow-free pure core. +- **DEC-002 — Airflow-free pure core in `signalforge/airflow/drift.py`.** Carries NO `from airflow` + import; eagerly importable; eager-re-exported from `__init__.py` (alongside `result`/`runner`, not + the lazy `__getattr__`). Per the v0.8 note (`airflow-integration.md`), it's a candidate to hoist to + a neutral `signalforge.automation` package later; out of scope here. +- **DEC-003 — `compute_drift` pure signature:** + `compute_drift(*, previous_diff: DiffReport, current_diff: DiffReport, previous_grade: GradingReport | None = None, current_grade: GradingReport | None = None, as_of: date | None = None, grade_regression_threshold: float = 0.05) -> DriftReport`. + No I/O, no airflow, deterministic. +- **DEC-004 — `DriftReport` model (frozen, `extra="ignore"`, read-back-able).** Fields: + `schema_version: Literal[1] = 1`, `signalforge_version: str`, `model_unique_id: str`, + `as_of: date | None`, `grade_regression_threshold: float`, `baseline: bool = False`, + `previous_diff_hash: str`, `current_diff_hash: str`, + `newly_always_passes: tuple[DriftArtifact, ...]`, `newly_dropped: tuple[DriftArtifact, ...]`, + `newly_kept: tuple[DriftArtifact, ...]`, `added_artifacts: tuple[str, ...]`, + `removed_artifacts: tuple[str, ...]`, `grade_regressions: tuple[GradeRegression, ...]`, + `schema_shape_changes: SchemaShapeDelta`, `degrade_reason: str | None = None`. + Computed property `alarming: bool = bool(newly_always_passes) or bool(grade_regressions)` (drives + `on_drift`). Custom `__repr__` omitting the long lists (mirrors result-model repr-redaction). + Sub-types: `DriftArtifact{artifact_id, previous_tier: str|None, current_tier: str|None, + previous_drop_reason: str|None, current_drop_reason: str|None, why: str}` (why truncated); + `GradeRegression{model_unique_id, previous_mean: float, current_mean: float, delta: float}`; + `SchemaShapeDelta{columns_added: tuple[str,...], columns_removed: tuple[str,...]}`. +- **DEC-005 — Transition classification (the join).** Over the UNION of `artifact_id`s in both + reports, classify by `(prior_tier, current_tier)`; "dropped:always-passes" keyed on + `drop_reason == "always-passes"`: + - `newly_always_passes` — in BOTH; prior ∈ {kept, kept-uncertain, flagged}; current `dropped`/`always-passes`. **The signal-rot alarm.** + - `newly_dropped` — in BOTH; prior ∈ {kept, kept-uncertain, flagged}; current `dropped` with a drop_reason ≠ `always-passes`. (Disjoint from `newly_always_passes`.) + - `newly_kept` — in BOTH; prior `dropped`; current ∈ {kept, kept-uncertain, flagged}. + - `added_artifacts` / `removed_artifacts` — `artifact_id`s present in only the current / only the prior report (informational; not alarming). + `flagged` is treated as a kept-ish tier for transition purposes (it ships). Same-tier pairs are no-ops. +- **DEC-006 — `decide_task_outcome` gains `on_drift` + optional `drift`.** + `decide_task_outcome(result, *, on_flagged: OnFlagged = "fail", on_drift: OnDrift = "fail", drift: DriftReport | None = None) -> TaskOutcome`. + `OnDrift = Literal["fail","skip","succeed"]`. When `drift is None` → byte-identical to today (every + #232/#233 caller unchanged; pinned by existing tests). When `drift is not None and drift.alarming` → + fold `on_drift` and return the MOST-SEVERE of the flagged-outcome and the drift-outcome. **No field + added to `SignalForgeRunResult`** (its #231 frozen contract + drift detector stay untouched). +- **DEC-007 — Fail-soft airflow-free loaders.** `load_diff_report(path) -> DiffReport | None`, + `load_grade_report(path) -> GradingReport | None`, `parse_diff_report(stdout) -> DiffReport | None` + (current report off `run_signalforge` stdout). All fail-soft: absent / corrupt / oversize → `None` + (degrade), never raise. Symlink-loop-hardened (`_common.path_safety`) + size-capped (10 MB, mirrors + diff's `existing_schema` cap). +- **DEC-008 — Prior-read path is operator-trusted; symlink-loop-hardened + size-capped, NOT + project-contained.** The `detect_drift_against` path may legitimately sit outside `project_dir` + (a history dir / object-store mount). Resolve + loop-guard + size-cap; do NOT enforce project + containment on the read (it's operator-supplied config, not attacker input, and read is fail-soft). +- **DEC-009 — Persist current run for the next comparison via the EXISTING writer.** When a + templatable `drift_history_dir` is set on the generate operator, write the current `DiffReport` + (parsed from stdout) — and the grade.json sibling if present — to + `//diff.json` via the existing fail-closed + `diff._sidecar.write_sidecar` (containment anchored to `drift_history_dir`). Default + `drift_history_dir = /.signalforge/history`. **No new fail-closed writer.** +- **DEC-010 — Dedicated `SignalForgeDriftOperator` follows the #232/#233 deferred-class pattern + verbatim.** Module `__getattr__` + `find_spec("airflow")` branch + `functools.cache` factory + + airflow-free placeholder (`__init__` raises `ModuleNotFoundError`). Params: `task_id`, + `previous_diff_path` (templatable, required), `current_diff_path` (templatable, required), + `previous_grade_path` / `current_grade_path` (optional; auto-sibling), `on_drift`, `as_of`, + `grade_regression_threshold`. Pure ungated helpers `_build_drift_inputs` / `_validate_drift_config`; + `execute()` is the thin `# pragma: no cover` wire-up (load both → `compute_drift` → XCom → + `raise_for_outcome`). `template_fields` = the path params + `as_of`. Runs downstream of a generate + task configured to persist sidecars. +- **DEC-011 — No new audit-event class, no new fail-closed writer.** `DriftReport` → XCom only; + current-run persistence reuses `write_sidecar`. Honors the ticket guardrail. +- **DEC-012 — No new error class.** Reuse `AirflowConfigError` (tier 2) for config faults (bad + `on_drift`/`on_flagged` value, missing required path on the dedicated operator, leading-dash argv + injection). Comparison degrades do NOT raise (DEC-013). ⇒ no `_EXCEPTION_TO_EXIT_CODE` / scan-7 / + errors.py-count churn. +- **DEC-013 — Degrade taxonomy (never fail).** No prior file → `baseline=True`, empty report, INFO + "baseline established", SUCCESS. `model_unique_id` mismatch → empty report + + `degrade_reason="model mismatch: prior=… current=…"` + WARNING + SUCCESS. Corrupt/unreadable prior → + empty report + `degrade_reason="prior sidecar unreadable: "` + WARNING + SUCCESS. + `--no-grade` (a grade.json absent) → tier-transition drift still computed; `grade_regressions=()`; + one INFO "grade comparison skipped (no grade sidecar)". A degraded report is never `alarming`. +- **DEC-014 — `as_of` threading + reproducibility.** Surface `as_of` on both operators; thread to + `compute_drift`; carry on `DriftReport`. Same two sidecars + same `as_of` → byte-identical report. +- **DEC-015 — XCom hygiene.** `DriftReport.to_xcom()` = per-category counts + `alarming` + `as_of` + (iso) + the two input hashes + the artifact_id lists (truncated `why`) + `degrade_reason`. No bulk + sidecar text, no secrets. +- **DEC-016 — Determinism.** `compute_drift` iterates `sorted` artifact_ids; transition tuples sorted + by `artifact_id`; `columns_added/removed` sorted. Two input hashes use the project's `blake2b-8` + recipe (`model_dump_json(by_alias=True)` → `json.dumps(sort_keys=True, separators=(",",":"))`). +- **DEC-017 — `DriftReport` read-back drift detector.** `StrictDriftReport(extra="forbid")` mirror + + committed fixture `tests/fixtures/airflow/drift_report_v1.json`. Ungated (the model is airflow-free). +- **DEC-018 — Docs.** `docs/airflow-ops.md` gains a "Drift / signal-rot detection" section (ticket + __A8__) with the worked nightly-drift-monitor DAG (both surfaces). `airflow-ops.md` is already in the + mkdocs nav → no nav change. +- **DEC-019 — Example DAG.** `examples/airflow/signalforge_drift_monitor_dag.py` — the generate-flag + form + the dedicated-operator branchable form. Gated DagBag-parse + `render_template_fields` test in + `tests/airflow/test_dag_parse.py`. + +--- + +## Detailed Breakdown + +> Natural ordering: airflow-free pure core → read-back gate → loaders + task-state extension → +> generate-operator flag → dedicated operator → gated tests/DAGs/docs → Quality Gate → Patterns. +> Validation command for every story: `uv sync --dev && uv run ruff check . && uv run ruff format --check . && uv run pyright && uv run pytest`. + +### US-001 — `DriftReport` model + `compute_drift` pure core (airflow-free, ungated, TDD) +- **Description:** Add `signalforge/airflow/drift.py` with `DriftReport` (+ `DriftArtifact`, + `GradeRegression`, `SchemaShapeDelta`) and the pure `compute_drift(...)`. Eager-re-export from + `signalforge/airflow/__init__.py`. Commit the engineered sidecar fixture PAIRS used by the tests. +- **Traces to:** DEC-002, 003, 004, 005, 013 (compute side), 014, 015 (`to_xcom`), 016. +- **TDD (tests first):** each transition category (`newly_always_passes` signal-rot; `newly_dropped` + non-always-passes; `newly_kept`; `added`/`removed`); grade-regression at/above/below 0.05; + schema add/remove derived from `artifact_id` prefixes; baseline (empty current vs prior) shape; + `--no-grade` (grades None) → `grade_regressions=()`; model-mismatch `degrade_reason`; determinism + (sorted, byte-identical on re-run); `alarming` property truth table; `to_xcom` shape + no bulk text. +- **Files:** `src/signalforge/airflow/drift.py` (new), `src/signalforge/airflow/__init__.py` (re-export), + `tests/airflow/test_drift_core.py` (ungated), `tests/fixtures/airflow/drift_pairs/*.json` (engineered pairs). +- **Done when:** all transition + degrade + determinism tests pass ungated; 100% line coverage of + `drift.py`; validation green. +- **Depends on:** none. + +### US-002 — `DriftReport` schema-stability drift detector + fixture (ungated) +- **Description:** Pin the read-back shape with a `StrictDriftReport(extra="forbid")` mirror validated + against a committed fixture. +- **Traces to:** DEC-017. +- **Files:** `tests/airflow/test_drift_report_schema.py`, `tests/fixtures/airflow/drift_report_v1.json`. +- **Done when:** adding a field to `DriftReport` without updating the strict mirror OR the fixture fails + the test loudly; validation green. +- **Depends on:** US-001. + +### US-003 — Fail-soft loaders + `decide_task_outcome` `on_drift` extension (airflow-free, ungated, TDD) +- **Description:** Add `load_diff_report` / `load_grade_report` / `parse_diff_report` (fail-soft, + symlink-loop-hardened, size-capped). Extend `decide_task_outcome` with `on_drift` + optional `drift` + + the most-severe combine + `OnDrift` literal — byte-identical when `drift is None`. +- **Traces to:** DEC-006, 007, 008, 013 (load side). +- **TDD:** loaders return `None` on absent/corrupt/oversize; symlink-loop guard; `decide_task_outcome` + flagged×drift most-severe matrix (incl. `drift=None` identity vs every existing case); `on_drift` + fail/skip/succeed; degraded (`alarming=False`) report never trips. +- **Files:** `src/signalforge/airflow/drift.py` (loaders), `src/signalforge/airflow/result.py` + (`decide_task_outcome` + `OnDrift`), `tests/airflow/test_drift_loaders.py`, + `tests/airflow/test_decide_task_outcome.py` (extend existing). +- **Done when:** existing #231/#232/#233 `decide_task_outcome` tests still pass unchanged; new matrix + green; loaders 100% covered ungated; validation green. +- **Depends on:** US-001. + +### US-004 — `detect_drift_against` on `SignalForgeGenerateOperator` + current-run persistence (gated execute, ungated helpers) +- **Description:** Add operator params `detect_drift_against`, `drift_history_dir`, `on_drift`, + `grade_regression_threshold`; ungated `_build_*`/`_validate_*` helper updates; `execute()` (gated) + wires: parse current report from stdout → load prior via templated path → `compute_drift` → persist + current via `write_sidecar` → `decide_task_outcome(..., drift=...)` → `raise_for_outcome`. Push + `drift_report.to_xcom()` under a `"drift"` key in the operator's XCom. Extend `template_fields`. +- **Traces to:** DEC-001, 009, 011, 012, 015. +- **Files:** `src/signalforge/airflow/operators.py`, `tests/airflow/test_operators_helpers.py` (ungated + helper tests). +- **Done when:** helper validation + argv tests pass ungated; `detect_drift_against` unset ⇒ generate + behavior byte-identical to #232 (pinned); validation green. (Gated execute() tests in US-006.) +- **Depends on:** US-001, US-003. + +### US-005 — Dedicated `SignalForgeDriftOperator` (deferred class + ungated helpers + UNGATED skeleton test) +- **Description:** Second deferred operator class (factory + `__getattr__` arm + airflow-free + placeholder), ungated `_build_drift_inputs`/`_validate_drift_config`, gated `# pragma: no cover` + `execute()`. Lazy re-export + name pin. UNGATED skeleton test (placeholder raises, `find_spec is None` + factory branch, `__getattr__` arm) — required for the codecov patch gate. +- **Traces to:** DEC-010, 012, 015. +- **Files:** `src/signalforge/airflow/operators.py`, `src/signalforge/airflow/__init__.py`, + `tests/airflow/test_skeleton.py` (UNGATED, add `SignalForgeDriftOperator` case), + `tests/airflow/test_airflow_no_eager_import.py` (pin new name). +- **Done when:** `from signalforge.airflow import SignalForgeDriftOperator` is airflow-free; construction + without airflow raises; skeleton + no-eager-import pins green; helper tests ungated; validation green. +- **Depends on:** US-001, US-003. + +### US-006 — Gated execute() tests (both surfaces) + example DAG + docs (gated + docs) +- **Description:** Gated `@pytest.mark.airflow` execute() tests for the generate-flag path and the + dedicated operator (fake-backed, using the US-001 fixture pairs): assert drift on XCom, `on_drift`→ + task-state via `raise_for_outcome`, baseline path succeeds, degrade paths succeed + WARNING. Add the + example DAG + DagBag-parse/render_template_fields gated test. Write the `airflow-ops.md` A8 section. +- **Traces to:** DEC-018, 019 (+ exercises DEC-001/010/013/015 end-to-end). +- **Files:** `tests/airflow/test_drift_operators.py` (gated), `tests/airflow/test_dag_parse.py` (extend), + `examples/airflow/signalforge_drift_monitor_dag.py`, `docs/airflow-ops.md`. +- **Done when:** gated tests pass under `.venv-airflow`; example DAG parses; docs section renders; + default-suite validation green (gated tests deselected). +- **Depends on:** US-004, US-005. + +### US-007 — Quality Gate (code review ×4 + CodeRabbit + airflow certification) +- **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** (#229): + `SF_RUN_AIRFLOW=1 PYTHONPATH="$PWD/src" /path/to/.venv-airflow/bin/python -m pytest tests/airflow -m airflow --no-cov`. +- **Traces to:** all DECs. +- **Done when:** all four passes clean; CodeRabbit addressed; gated airflow suite green vs Airflow 2.10.4; + full validation green. +- **Depends on:** US-001…US-006. + +### US-008 — Patterns & Memory (priority 99) +- **Description:** Update `.claude/rules/airflow-integration.md` with the drift section (on_drift + parallels on_flagged via most-severe combine; `compute_drift` airflow-free core; history-persist + reuses `write_sidecar`; `DriftReport` read-back drift detector; degrade taxonomy). Cross-ref from + `diff-renderer.md`/`cli-layer.md` as needed. Save a memory note for the run-over-run pattern. +- **Traces to:** all DECs. +- **Files:** `.claude/rules/airflow-integration.md` (orchestrator-edited per worker `.claude/` perms), + memory. +- **Depends on:** US-007. + +### Rules-compliance gate (validated against Discovery constraints) +- airflow-free pure core, no `from airflow` in `drift.py` ✓ (DEC-002); shim-confined raise via + `raise_for_outcome` ✓; `TaskOutcome` stays 4-valued, no 5th exit tier ✓ (DEC-006); XCom = counts + + paths + summary, no secrets ✓ (DEC-015); deferred-operator + UNGATED skeleton test ✓ (US-005); + `.venv-airflow` certification ✓ (US-007); reuse tiers/DropReason, new model not a tier ✓ (DEC-004/005); + reproducibility via two `blake2b-8` input hashes + sorted iteration ✓ (DEC-016); no new fail-closed + writer / audit class ✓ (DEC-011); no new error class / scan-7 untouched ✓ (DEC-012); read-back drift + detector + fixture ✓ (DEC-017); `--as-of` carve-out surfaced ✓ (DEC-014); docs in airflow-ops.md ✓ + (DEC-018); `[airflow]` extra stays out of dev group (unchanged) ✓. + +--- + +## Beads Manifest (Phase 7) — pending devolve From a57bf0349fb7638e9a1805c60d6053d52822c6e3 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 12:25:01 -0700 Subject: [PATCH 02/13] #235: devolve plan to beads (epic bd_1-scaffolding-v42, 8 tasks) --- plans/super/235-drift-detection.md | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/plans/super/235-drift-detection.md b/plans/super/235-drift-detection.md index b0be6d09..c2298d00 100644 --- a/plans/super/235-drift-detection.md +++ b/plans/super/235-drift-detection.md @@ -2,7 +2,7 @@ ## Meta - **Ticket:** #235 — Airflow: drift / signal-rot detection mode (run-over-run). Part of epic #228 (v0.7 Airflow). Depends on #231 (result/XCom contract — landed) and #232 (`SignalForgeGenerateOperator` — landed). -- **Phase:** detailing (awaiting approval to devolve) +- **Phase:** devolved (2026-06-16) — beads created, ready for Ralph. - **Branch:** `feature/235-drift-detection` (worktree `/home/wesd/Projects/worktrees/SignalForge/235-drift-detection`, base `dev`). - **Sessions:** 1 (2026-06-16). @@ -389,4 +389,16 @@ a new architectural seam. --- -## Beads Manifest (Phase 7) — pending devolve +## Beads Manifest (Phase 7) — devolved 2026-06-16 +- **Epic:** `bd_1-scaffolding-v42` — Airflow: drift / signal-rot detection (run-over-run) — epic #235 +- **Worktree:** `/home/wesd/Projects/worktrees/SignalForge/235-drift-detection` (branch `feature/235-drift-detection`, base `dev`). +- **Tasks (chain):** + - `…v42.1` US-001 DriftReport + compute_drift (ungated, TDD) — **ready** + - `…v42.2` US-002 StrictDriftReport drift detector ← .1 + - `…v42.3` US-003 loaders + decide_task_outcome on_drift ← .1 + - `…v42.4` US-004 generate-operator flag + persistence ← .1, .3 + - `…v42.5` US-005 dedicated SignalForgeDriftOperator + skeleton ← .1, .3 + - `…v42.6` US-006 gated execute tests + DAG + docs ← .4, .5 + - `…v42.7` US-007 Quality Gate ← .6, .2 + - `…v42.8` US-008 Patterns & Memory (P4) ← .7 +- **Ready frontier:** US-001 only (verified `bd ready`); 14 dependency edges; rest blocked until their parents close. From d9a8355a10764d1043cb1f0f8cbe3e0913a8515c Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 12:41:37 -0700 Subject: [PATCH 03/13] bd_1-scaffolding-v42.1: DriftReport + compute_drift airflow-free pure core (#235 US-001) --- src/signalforge/airflow/__init__.py | 20 +- src/signalforge/airflow/drift.py | 470 +++++++++++ tests/airflow/test_drift_core.py | 732 ++++++++++++++++++ .../drift_pairs/signal_rot_curr_diff.json | 40 + .../drift_pairs/signal_rot_curr_grade.json | 20 + .../drift_pairs/signal_rot_prev_diff.json | 40 + .../drift_pairs/signal_rot_prev_grade.json | 20 + 7 files changed, 1339 insertions(+), 3 deletions(-) create mode 100644 src/signalforge/airflow/drift.py create mode 100644 tests/airflow/test_drift_core.py create mode 100644 tests/fixtures/airflow/drift_pairs/signal_rot_curr_diff.json create mode 100644 tests/fixtures/airflow/drift_pairs/signal_rot_curr_grade.json create mode 100644 tests/fixtures/airflow/drift_pairs/signal_rot_prev_diff.json create mode 100644 tests/fixtures/airflow/drift_pairs/signal_rot_prev_grade.json diff --git a/src/signalforge/airflow/__init__.py b/src/signalforge/airflow/__init__.py index 2f5b9fd6..34553918 100644 --- a/src/signalforge/airflow/__init__.py +++ b/src/signalforge/airflow/__init__.py @@ -32,9 +32,11 @@ Error classes (``AirflowIntegrationError`` / ``AirflowConfigError``), the result core (``SignalForgeRunResult`` / ``TaskOutcome`` / ``decide_task_outcome`` / -``OnFlagged``), and the runner (``run_signalforge``) are pure-Python and -Airflow-free (none of ``signalforge.airflow.errors`` / ``.result`` / ``.runner`` -carries a ``from airflow ...`` import — the runner's only heavy import, +``OnFlagged``), the runner (``run_signalforge``), and the drift core +(``compute_drift`` / ``DriftReport`` / ``DriftArtifact`` / ``GradeRegression`` / +``SchemaShapeDelta`` — issue #235) are pure-Python and Airflow-free (none of +``signalforge.airflow.errors`` / ``.result`` / ``.runner`` / ``.drift`` carries +a ``from airflow ...`` import — the runner's only heavy import, :func:`signalforge.cli.main`, is done lazily inside the function body), so they are **eager** re-exports here (only the operator/hook names stay lazy — DEC-003 / DEC-006). Importing them does not violate the no-eager-import gate. @@ -45,6 +47,13 @@ from importlib import import_module from typing import TYPE_CHECKING +from signalforge.airflow.drift import ( + DriftArtifact, + DriftReport, + GradeRegression, + SchemaShapeDelta, + compute_drift, +) from signalforge.airflow.errors import AirflowConfigError, AirflowIntegrationError from signalforge.airflow.result import ( OnFlagged, @@ -77,12 +86,17 @@ __all__ = [ "AirflowConfigError", "AirflowIntegrationError", + "DriftArtifact", + "DriftReport", + "GradeRegression", "OnFlagged", + "SchemaShapeDelta", "SignalForgeGenerateOperator", "SignalForgeHook", "SignalForgePruneExistingOperator", "SignalForgeRunResult", "TaskOutcome", + "compute_drift", "decide_task_outcome", "run_signalforge", ] diff --git a/src/signalforge/airflow/drift.py b/src/signalforge/airflow/drift.py new file mode 100644 index 00000000..6fffe986 --- /dev/null +++ b/src/signalforge/airflow/drift.py @@ -0,0 +1,470 @@ +"""Airflow-free run-over-run drift / signal-rot detection (pure core). + +US-001 of issue #235 (epic #228, v0.7 Airflow). This module is the *pure +core* of the drift-detection feature: it compares a SignalForge run (N) +against its prior run (N-1) — two :class:`signalforge.diff.models.DiffReport` +sidecars, plus optional :class:`signalforge.grade.models.GradingReport` +sidecars — and emits a structured, JSON-serialisable :class:`DriftReport` +delta. + +The headline signal is **signal rot** — a test that *used to* catch failing +rows (a ``kept`` / ``kept-uncertain`` / ``flagged`` tier) but now +**always-passes** (``dropped`` with ``drop_reason == "always-passes"``). That +transition is a schema-drift alarm worth paging on (Architectural Commitment +#1: an always-pass test is noise; a test that *rotted into* always-pass is a +drift signal). + +**This module imports NO airflow.** That is load-bearing (DEC-002): it lets +``signalforge.airflow.__init__`` re-export :func:`compute_drift` and +:class:`DriftReport` **eagerly** (alongside ``result`` / ``runner`` — only the +operator/hook names stay lazy), and it keeps the comparison logic +unit-testable in the default pytest suite without the heavy, version-pinned +Apache Airflow dependency installed. Per the v0.8 note in +``.claude/rules/airflow-integration.md``, this pure core is a candidate to +hoist to a neutral ``signalforge.automation`` package later; out of scope here. + +:func:`compute_drift` is pure, deterministic, and does NO I/O — it takes parsed +:class:`DiffReport` / :class:`GradingReport` objects (the loaders that read +them off disk / stdout land in US-003). The comparison **degrades, never +raises** (DEC-013): a model-set mismatch yields an empty report with a +``degrade_reason`` rather than an exception, and a missing grade sidecar simply +leaves ``grade_regressions`` empty. +""" + +from __future__ import annotations + +import hashlib +import json +from collections.abc import Iterable +from datetime import date +from typing import Literal + +from pydantic import BaseModel, ConfigDict, field_serializer, field_validator + +import signalforge +from signalforge.diff.models import DiffEntry, DiffReport +from signalforge.grade.models import GradingReport + +_BASE_CONFIG = ConfigDict(frozen=True, extra="ignore", populate_by_name=True) + +# Truncation budget for the per-artifact ``why`` string carried on a +# :class:`DriftArtifact` (mirrors the diff layer's ``_truncate_why`` idea — a +# one-line operator-readable explanation, not a multi-line dump). +_WHY_MAX_CHARS = 200 + +# Tiers that mean "the artifact ships in the proposed schema.yml". ``flagged`` +# is kept-ish for transition purposes (DEC-005) — it still ships, it just +# graded below threshold. ``dropped`` is the only non-kept-ish tier. +_KEPT_ISH_TIERS: frozenset[str] = frozenset({"kept", "kept-uncertain", "flagged"}) + + +def _truncate_why(text: str, max_chars: int = _WHY_MAX_CHARS) -> str: + """Truncate ``text`` to ``max_chars`` with a U+2026 ellipsis tail. + + Mirrors :func:`signalforge.diff.engine._truncate_why` byte-for-byte: + empty / whitespace-only input returns the empty string; a non-positive + budget returns the empty string; an over-budget string is hard-cut at + ``max_chars - 1`` (after :meth:`str.rstrip`) with ``"…"`` appended; + otherwise the string is returned with a trailing :meth:`str.rstrip` only. + """ + if not text or not text.strip(): + return "" + if max_chars <= 0: + return "" + if len(text) > max_chars: + return text[: max_chars - 1].rstrip() + "…" + return text.rstrip() + + +def _hash_report(report: BaseModel) -> str: + """Return the project's canonical ``blake2b-8`` fingerprint of a model. + + DEC-016 recipe (inlined to keep this module airflow-free and free of a + cross-stage import of the diff layer's private helper): serialise via + ``model_dump_json(by_alias=True)`` and re-encode through + :func:`json.dumps` with ``sort_keys=True`` + ``separators=(",", ":")`` so + equivalent inputs produce identical 16-hex digests regardless of + field-construction order. + """ + raw_json = report.model_dump_json(by_alias=True) + parsed = json.loads(raw_json) + canonical = json.dumps(parsed, sort_keys=True, separators=(",", ":")) + return hashlib.blake2b(canonical.encode("utf-8"), digest_size=8).hexdigest() + + +def _columns_from_artifact_ids(artifact_ids: Iterable[str]) -> set[str]: + """Derive the column SET from ``artifact_id`` dotted-path prefixes. + + Column names are embedded in two of the six dotted-path shapes + (``.claude/rules`` + :mod:`signalforge._common.artifact_id`): + + * ``column..`` → column is the 2nd component. + * ``test.column..[.]`` → column is the 3rd component. + + ``model.`` and ``test.model.`` carry no column. Column names + pass the strict ``^[A-Za-z_][A-Za-z0-9_]*$`` identifier regex upstream, so + they never contain a dot — a plain ``str.split(".")`` is unambiguous. + """ + columns: set[str] = set() + for artifact_id in artifact_ids: + parts = artifact_id.split(".") + if len(parts) >= 2 and parts[0] == "column": + columns.add(parts[1]) + elif len(parts) >= 3 and parts[0] == "test" and parts[1] == "column": + columns.add(parts[2]) + return columns + + +class DriftArtifact(BaseModel): + """One artifact whose tier transitioned between two runs. + + Carried on the three transition tuples of :class:`DriftReport` + (``newly_always_passes`` / ``newly_dropped`` / ``newly_kept``). Records the + artifact's identity plus its before/after tier and drop_reason, and a + truncated one-line ``why`` (the current run's explanation for the new + tier). + + Read-back-stable: ``frozen=True, extra="ignore"`` per the manifest-readers + convention. + """ + + model_config = _BASE_CONFIG + + artifact_id: str + previous_tier: str | None + current_tier: str | None + previous_drop_reason: str | None + current_drop_reason: str | None + why: str = "" + + @field_validator("why") + @classmethod + def _truncate_why_field(cls, value: str) -> str: + """Cap ``why`` at :data:`_WHY_MAX_CHARS` regardless of caller.""" + return _truncate_why(value) + + +class GradeRegression(BaseModel): + """A run-over-run grade regression for one model. + + Emitted when ``current_mean <= previous_mean - grade_regression_threshold`` + (DEC-005). ``delta = previous_mean - current_mean`` — positive means the + grade fell (a regression). + + Read-back-stable: ``frozen=True, extra="ignore"``. + """ + + model_config = _BASE_CONFIG + + model_unique_id: str + previous_mean: float + current_mean: float + delta: float + + +class SchemaShapeDelta(BaseModel): + """Column SET add/remove between two runs (DEC-005, Q4). + + Derived from the union of ``artifact_id`` prefixes across the two reports. + Column TYPE changes (retype) are out of scope — the sidecars carry no + types. Both tuples are sorted for determinism. + + Read-back-stable: ``frozen=True, extra="ignore"``. + """ + + model_config = _BASE_CONFIG + + columns_added: tuple[str, ...] = () + columns_removed: tuple[str, ...] = () + + +class DriftReport(BaseModel): + """Run-over-run drift delta — the public output of :func:`compute_drift`. + + Pushed to XCom (via :meth:`to_xcom`) and optionally compared by the + ``on_drift`` policy (US-003). Read-back-stable (``frozen=True, + extra="ignore"``) and self-describing: it carries the two input + ``blake2b-8`` hashes so the same two sidecars + same ``as_of`` reproduce a + byte-identical report (DEC-014 / DEC-016). + + The headline :attr:`alarming` property is ``True`` iff there is signal rot + (``newly_always_passes``) or a grade regression (``grade_regressions``) — + these are the categories that page (DEC-005, Q3). ``newly_kept`` / + ``newly_dropped`` / ``added_artifacts`` / ``removed_artifacts`` / + ``schema_shape_changes`` are informational and never alarm. A degraded + report (``degrade_reason`` set) is never alarming — the alarm lists are + empty by construction (DEC-013). + + DEC-015 — minimal :meth:`__repr__` (and Pydantic-v2 :meth:`__repr_args__`) + omit the long transition lists so an accidental ``_LOGGER.warning("drift: + %s", report)`` doesn't dump every transitioned artifact's prose ``why``. + """ + + model_config = _BASE_CONFIG + + schema_version: Literal[1] = 1 + signalforge_version: str + model_unique_id: str + as_of: date | None + grade_regression_threshold: float + baseline: bool = False + previous_diff_hash: str + current_diff_hash: str + newly_always_passes: tuple[DriftArtifact, ...] = () + newly_dropped: tuple[DriftArtifact, ...] = () + newly_kept: tuple[DriftArtifact, ...] = () + added_artifacts: tuple[str, ...] = () + removed_artifacts: tuple[str, ...] = () + grade_regressions: tuple[GradeRegression, ...] = () + schema_shape_changes: SchemaShapeDelta = SchemaShapeDelta() + degrade_reason: str | None = None + + @field_serializer("as_of") + def _serialize_as_of(self, value: date | None) -> str | None: + """Render ``as_of`` as a bare ``YYYY-MM-DD`` ISO string or ``null``. + + Uses :meth:`date.isoformat` directly — NOT + :func:`signalforge._common.timestamp.iso8601_z`, which is + ``datetime``-only and would append a spurious ``T00:00:00Z`` suffix + (mirrors :class:`signalforge.prune.audit.PruneEvent`'s ``as_of`` + serializer, issue #171). + """ + return value.isoformat() if value is not None else None + + @property + def alarming(self) -> bool: + """Whether this drift trips the ``on_drift`` policy (DEC-005). + + ``True`` iff there is signal rot (:attr:`newly_always_passes`) or a + grade regression (:attr:`grade_regressions`). A degraded report returns + ``False`` because both lists are empty by construction (DEC-013). + """ + return bool(self.newly_always_passes) or bool(self.grade_regressions) + + def to_xcom(self) -> dict[str, object]: + """Return a JSON-serialisable summary for XCom (DEC-015). + + Per-category counts + the transition artifact lists (with the already + truncated ``why``) + ``alarming`` + ``as_of`` (iso str or ``None``) + + the two input hashes + ``grade_regressions`` / ``schema_shape_changes`` + as dicts + ``degrade_reason``. Carries NO bulk sidecar text and NO + secrets — every value round-trips through ``json.dumps`` / + ``json.loads``. + """ + return { + "schema_version": self.schema_version, + "model_unique_id": self.model_unique_id, + "as_of": self.as_of.isoformat() if self.as_of is not None else None, + "baseline": self.baseline, + "alarming": self.alarming, + "grade_regression_threshold": self.grade_regression_threshold, + "previous_diff_hash": self.previous_diff_hash, + "current_diff_hash": self.current_diff_hash, + "degrade_reason": self.degrade_reason, + "counts": { + "newly_always_passes": len(self.newly_always_passes), + "newly_dropped": len(self.newly_dropped), + "newly_kept": len(self.newly_kept), + "added_artifacts": len(self.added_artifacts), + "removed_artifacts": len(self.removed_artifacts), + "grade_regressions": len(self.grade_regressions), + "columns_added": len(self.schema_shape_changes.columns_added), + "columns_removed": len(self.schema_shape_changes.columns_removed), + }, + "newly_always_passes": [a.model_dump() for a in self.newly_always_passes], + "newly_dropped": [a.model_dump() for a in self.newly_dropped], + "newly_kept": [a.model_dump() for a in self.newly_kept], + "added_artifacts": list(self.added_artifacts), + "removed_artifacts": list(self.removed_artifacts), + "grade_regressions": [g.model_dump() for g in self.grade_regressions], + "schema_shape_changes": { + "columns_added": list(self.schema_shape_changes.columns_added), + "columns_removed": list(self.schema_shape_changes.columns_removed), + }, + } + + def __repr__(self) -> str: + """Minimal repr — omits the long transition lists (DEC-015).""" + return ( + f"DriftReport(model_unique_id={self.model_unique_id!r}, " + f"as_of={self.as_of!r}, " + f"newly_always_passes={len(self.newly_always_passes)!r}, " + f"newly_dropped={len(self.newly_dropped)!r}, " + f"newly_kept={len(self.newly_kept)!r}, " + f"grade_regressions={len(self.grade_regressions)!r}, " + f"alarming={self.alarming!r}, " + f"baseline={self.baseline!r})" + ) + + def __repr_args__(self) -> list[tuple[str | None, object]]: + """Pydantic v2 structured-repr hook mirroring :meth:`__repr__`. + + Keeps ``rich.print()`` / ``devtools.pretty()`` / ``pprint`` from + dumping the long transition lists too (see memory + ``pydantic-v2-repr-args-redaction-required``). + """ + return [ + ("model_unique_id", self.model_unique_id), + ("as_of", self.as_of), + ("newly_always_passes", len(self.newly_always_passes)), + ("newly_dropped", len(self.newly_dropped)), + ("newly_kept", len(self.newly_kept)), + ("grade_regressions", len(self.grade_regressions)), + ("alarming", self.alarming), + ("baseline", self.baseline), + ] + + +def _build_drift_artifact( + artifact_id: str, previous: DiffEntry, current: DiffEntry +) -> DriftArtifact: + """Build a :class:`DriftArtifact` from a matched (prev, curr) entry pair. + + The ``why`` is taken from the *current* entry (it describes the new tier) + and is truncated by the :class:`DriftArtifact` field validator. + """ + return DriftArtifact( + artifact_id=artifact_id, + previous_tier=previous.tier, + current_tier=current.tier, + previous_drop_reason=previous.drop_reason, + current_drop_reason=current.drop_reason, + why=current.why, + ) + + +def compute_drift( + *, + previous_diff: DiffReport, + current_diff: DiffReport, + previous_grade: GradingReport | None = None, + current_grade: GradingReport | None = None, + as_of: date | None = None, + grade_regression_threshold: float = 0.05, +) -> DriftReport: + """Compare two SignalForge runs and return a :class:`DriftReport` delta. + + Pure, deterministic, no I/O, no airflow (DEC-003). Iterates ``sorted`` + ``artifact_id``s so the output is reproducible: the same two sidecars + + same ``as_of`` produce a byte-identical report (DEC-016). + + Transition classification (DEC-005), over the UNION of ``artifact_id``s, + with ``flagged`` treated as a kept-ish (shipped) tier: + + * ``newly_always_passes`` — in BOTH; prior kept-ish; current ``dropped`` + with ``drop_reason == "always-passes"``. **The signal-rot alarm.** + * ``newly_dropped`` — in BOTH; prior kept-ish; current ``dropped`` with a + drop_reason ≠ ``always-passes``. (Disjoint from ``newly_always_passes``.) + * ``newly_kept`` — in BOTH; prior ``dropped``; current kept-ish. + * ``added_artifacts`` / ``removed_artifacts`` — present in only the current + / only the prior report (informational). + + ``schema_shape_changes`` derives the column add/remove SET from + ``artifact_id`` prefixes. ``grade_regressions`` is emitted only when BOTH + grades are present and the mean fell beyond ``grade_regression_threshold``; + a missing grade (``--no-grade``) leaves it empty (degrade, don't fail — + DEC-013). + + Degrade (never raise — DEC-013): a ``model_unique_id`` mismatch between the + two diffs returns an empty report with ``degrade_reason`` set and + ``alarming`` ``False``. ``compute_drift`` does NOT handle the "no prior at + all" baseline case — that is a loader concern (US-003); ``baseline`` + defaults to ``False`` here. + """ + version = signalforge.__version__ + previous_diff_hash = _hash_report(previous_diff) + current_diff_hash = _hash_report(current_diff) + + # Degrade: model-set mismatch. Empty report, never alarming (DEC-013). + if previous_diff.model_unique_id != current_diff.model_unique_id: + return DriftReport( + signalforge_version=version, + model_unique_id=current_diff.model_unique_id, + as_of=as_of, + grade_regression_threshold=grade_regression_threshold, + previous_diff_hash=previous_diff_hash, + current_diff_hash=current_diff_hash, + degrade_reason=( + f"model mismatch: prior={previous_diff.model_unique_id} " + f"current={current_diff.model_unique_id}" + ), + ) + + previous_by_id: dict[str, DiffEntry] = {e.artifact_id: e for e in previous_diff.entries} + current_by_id: dict[str, DiffEntry] = {e.artifact_id: e for e in current_diff.entries} + + newly_always_passes: list[DriftArtifact] = [] + newly_dropped: list[DriftArtifact] = [] + newly_kept: list[DriftArtifact] = [] + added_artifacts: list[str] = [] + removed_artifacts: list[str] = [] + + all_ids = sorted(set(previous_by_id) | set(current_by_id)) + for artifact_id in all_ids: + prev = previous_by_id.get(artifact_id) + curr = current_by_id.get(artifact_id) + if prev is None: + # Present only in the current run → an added artifact. + added_artifacts.append(artifact_id) + continue + if curr is None: + # Present only in the prior run → a removed artifact. + removed_artifacts.append(artifact_id) + continue + prev_kept_ish = prev.tier in _KEPT_ISH_TIERS + curr_kept_ish = curr.tier in _KEPT_ISH_TIERS + if prev_kept_ish and curr.tier == "dropped": + artifact = _build_drift_artifact(artifact_id, prev, curr) + if curr.drop_reason == "always-passes": + newly_always_passes.append(artifact) + else: + newly_dropped.append(artifact) + elif prev.tier == "dropped" and curr_kept_ish: + newly_kept.append(_build_drift_artifact(artifact_id, prev, curr)) + # Same-tier (or any other kept-ish ↔ kept-ish) pair: no-op. + + previous_columns = _columns_from_artifact_ids(previous_by_id) + current_columns = _columns_from_artifact_ids(current_by_id) + schema_shape_changes = SchemaShapeDelta( + columns_added=tuple(sorted(current_columns - previous_columns)), + columns_removed=tuple(sorted(previous_columns - current_columns)), + ) + + grade_regressions: list[GradeRegression] = [] + if previous_grade is not None and current_grade is not None: + previous_mean = previous_grade.mean_score + current_mean = current_grade.mean_score + if current_mean <= previous_mean - grade_regression_threshold: + grade_regressions.append( + GradeRegression( + model_unique_id=current_diff.model_unique_id, + previous_mean=previous_mean, + current_mean=current_mean, + delta=previous_mean - current_mean, + ) + ) + + return DriftReport( + signalforge_version=version, + model_unique_id=current_diff.model_unique_id, + as_of=as_of, + grade_regression_threshold=grade_regression_threshold, + previous_diff_hash=previous_diff_hash, + current_diff_hash=current_diff_hash, + newly_always_passes=tuple(newly_always_passes), + newly_dropped=tuple(newly_dropped), + newly_kept=tuple(newly_kept), + added_artifacts=tuple(added_artifacts), + removed_artifacts=tuple(removed_artifacts), + grade_regressions=tuple(grade_regressions), + schema_shape_changes=schema_shape_changes, + ) + + +__all__ = [ + "DriftArtifact", + "DriftReport", + "GradeRegression", + "SchemaShapeDelta", + "compute_drift", +] diff --git a/tests/airflow/test_drift_core.py b/tests/airflow/test_drift_core.py new file mode 100644 index 00000000..ce6d86b5 --- /dev/null +++ b/tests/airflow/test_drift_core.py @@ -0,0 +1,732 @@ +"""Ungated tests for the airflow-free drift core (US-001 of #235). + +These tests run in the DEFAULT pytest suite — NO ``@pytest.mark.airflow`` +marker — because :mod:`signalforge.airflow.drift` carries no ``from airflow`` +import (DEC-002). They pin the transition classification (DEC-005), the +degrade taxonomy (DEC-013, compute side), the schema-shape derivation, +determinism (DEC-016), the ``alarming`` truth table, and the ``to_xcom`` shape +(DEC-015). + +Each transition is *engineered* so the expected outcome is mathematically +guaranteed (``.claude/rules/testing-signal.md``): the two :class:`DiffReport` +inputs are built explicitly with the exact ``(prev_tier, curr_tier, +drop_reason)`` triples each classification arm keys on. +""" + +from __future__ import annotations + +import json +from datetime import date +from pathlib import Path + +import signalforge +from signalforge.airflow import ( + DriftArtifact, + GradeRegression, + SchemaShapeDelta, + compute_drift, +) +from signalforge.diff.models import DiffEntry, Tier +from signalforge.diff.models import DiffReport as SfDiffReport +from signalforge.grade.models import GradingReport, GradingResult + +_FIXTURE_DIR = Path(__file__).parent.parent / "fixtures" / "airflow" / "drift_pairs" + + +# --------------------------------------------------------------------------- +# Engineered builders — explicit DiffReport / GradingReport construction. +# --------------------------------------------------------------------------- + + +def _entry( + artifact_id: str, + tier: Tier, + *, + test_type: str | None = None, + drop_reason: str | None = None, + why: str = "", +) -> DiffEntry: + return DiffEntry( + artifact_id=artifact_id, + test_type=test_type, + tier=tier, + drop_reason=drop_reason, # type: ignore[arg-type] + why=why, + ) + + +def _diff( + *, + model_unique_id: str = "model.shop.fct_orders", + entries: tuple[DiffEntry, ...], +) -> SfDiffReport: + """Build a minimal valid :class:`DiffReport` for drift comparison. + + The count fields / hashes are not consumed by :func:`compute_drift`; they + are filled with consistent placeholders so the model validates. + """ + kept = sum(1 for e in entries if e.tier in ("kept", "kept-uncertain", "flagged")) + dropped = sum(1 for e in entries if e.tier == "dropped") + return SfDiffReport( + signalforge_version=signalforge.__version__, + model_unique_id=model_unique_id, + run_id="r" * 32, + duration_seconds=1.0, + proposed_yaml="version: 2\n", + existing_yaml=None, + unified_diff="", + entries=entries, + kept_count=kept, + kept_uncertain_count=sum(1 for e in entries if e.tier == "kept-uncertain"), + dropped_count=dropped, + flagged_count=sum(1 for e in entries if e.tier == "flagged"), + has_existing_schema=False, + candidate_hash="0" * 16, + prune_result_hash="0" * 16, + grading_report_hash=None, + ) + + +def _grade(mean: float, *, model_unique_id: str = "model.shop.fct_orders") -> GradingReport: + """Build a :class:`GradingReport` whose ``mean_score`` equals ``mean``. + + A single scored result with ``score=mean`` makes the computed + ``mean_score`` exactly ``mean``. + """ + return GradingReport( + signalforge_version=signalforge.__version__, + run_id="g" * 32, + timestamp="2026-06-16T00:00:00.000000Z", # type: ignore[arg-type] + duration_seconds=2.0, + model_unique_id=model_unique_id, + rubric_hash="1" * 16, + thresholds=(0.7, 0.7), + results=( + GradingResult( + artifact_id="column.amount.description", + criterion_id="clarity", + score=mean, + passed=True, + ), + ), + ) + + +# --------------------------------------------------------------------------- +# Transition classification (DEC-005). +# --------------------------------------------------------------------------- + + +def test_newly_always_passes_is_the_signal_rot_alarm() -> None: + """kept → dropped/always-passes is the headline signal-rot transition.""" + prev = _diff(entries=(_entry("test.column.amount.not_null", "kept", why="caught rows"),)) + curr = _diff( + entries=( + _entry( + "test.column.amount.not_null", + "dropped", + drop_reason="always-passes", + why="always passes on the sample", + ), + ) + ) + report = compute_drift(previous_diff=prev, current_diff=curr) + assert len(report.newly_always_passes) == 1 + artifact = report.newly_always_passes[0] + assert artifact.artifact_id == "test.column.amount.not_null" + assert artifact.previous_tier == "kept" + assert artifact.current_tier == "dropped" + assert artifact.current_drop_reason == "always-passes" + assert report.newly_dropped == () + assert report.alarming is True + + +def test_flagged_to_always_passes_also_counts_as_signal_rot() -> None: + """flagged is a kept-ish (shipped) tier — its rot to always-passes alarms.""" + prev = _diff(entries=(_entry("test.column.amount.not_null", "flagged"),)) + curr = _diff( + entries=(_entry("test.column.amount.not_null", "dropped", drop_reason="always-passes"),) + ) + report = compute_drift(previous_diff=prev, current_diff=curr) + assert len(report.newly_always_passes) == 1 + assert report.newly_always_passes[0].previous_tier == "flagged" + + +def test_newly_dropped_non_always_passes_is_disjoint_from_signal_rot() -> None: + """kept → dropped with a non-always-passes reason is informational only.""" + prev = _diff(entries=(_entry("test.column.user_id.relationships", "kept"),)) + curr = _diff( + entries=( + _entry( + "test.column.user_id.relationships", + "dropped", + drop_reason="requires-future-data", + why="ref target absent", + ), + ) + ) + report = compute_drift(previous_diff=prev, current_diff=curr) + assert len(report.newly_dropped) == 1 + assert report.newly_dropped[0].current_drop_reason == "requires-future-data" + assert report.newly_always_passes == () + assert report.alarming is False + + +def test_newly_kept_dropped_to_kept_ish() -> None: + """dropped → kept (test started catching rows again) is informational.""" + prev = _diff( + entries=(_entry("test.column.amount.unique", "dropped", drop_reason="always-passes"),) + ) + curr = _diff(entries=(_entry("test.column.amount.unique", "kept", why="caught dupes"),)) + report = compute_drift(previous_diff=prev, current_diff=curr) + assert len(report.newly_kept) == 1 + assert report.newly_kept[0].previous_tier == "dropped" + assert report.newly_kept[0].current_tier == "kept" + assert report.alarming is False + + +def test_dropped_to_kept_uncertain_is_newly_kept() -> None: + """kept-uncertain is a kept-ish tier — dropped → kept-uncertain is newly_kept.""" + prev = _diff(entries=(_entry("test.model.row_count_between", "dropped"),)) + curr = _diff(entries=(_entry("test.model.row_count_between", "kept-uncertain"),)) + report = compute_drift(previous_diff=prev, current_diff=curr) + assert len(report.newly_kept) == 1 + + +def test_added_and_removed_artifacts() -> None: + """artifact_ids present in only one report are added / removed.""" + prev = _diff(entries=(_entry("test.column.amount.not_null", "kept"),)) + curr = _diff(entries=(_entry("test.column.region.not_null", "kept"),)) + report = compute_drift(previous_diff=prev, current_diff=curr) + assert report.added_artifacts == ("test.column.region.not_null",) + assert report.removed_artifacts == ("test.column.amount.not_null",) + # Neither is a tier transition. + assert report.newly_always_passes == () + assert report.newly_dropped == () + assert report.newly_kept == () + + +def test_same_tier_pair_is_a_noop() -> None: + """An artifact whose tier is unchanged produces no transition entry.""" + prev = _diff(entries=(_entry("test.column.amount.not_null", "kept"),)) + curr = _diff(entries=(_entry("test.column.amount.not_null", "kept"),)) + report = compute_drift(previous_diff=prev, current_diff=curr) + assert report.newly_always_passes == () + assert report.newly_dropped == () + assert report.newly_kept == () + assert report.added_artifacts == () + assert report.removed_artifacts == () + assert report.alarming is False + + +def test_kept_ish_to_kept_ish_transition_is_not_flagged_as_drift() -> None: + """kept → flagged (graded below threshold) is NOT a drift transition. + + Both are kept-ish (shipped) tiers, so the artifact stays out of the + three transition lists — grade movement is captured by grade_regressions, + not by tier transitions. + """ + prev = _diff(entries=(_entry("test.column.amount.not_null", "kept"),)) + curr = _diff(entries=(_entry("test.column.amount.not_null", "flagged"),)) + report = compute_drift(previous_diff=prev, current_diff=curr) + assert report.newly_always_passes == () + assert report.newly_dropped == () + assert report.newly_kept == () + + +# --------------------------------------------------------------------------- +# Schema-shape derivation from artifact_id prefixes. +# --------------------------------------------------------------------------- + + +def test_schema_shape_add_and_remove_from_artifact_ids() -> None: + """Column SET add/remove derives from column.* and test.column.* prefixes.""" + prev = _diff( + entries=( + _entry("column.amount.description", "kept"), + _entry("test.column.amount.not_null", "kept"), + ) + ) + curr = _diff( + entries=( + _entry("column.region.description", "kept"), + _entry("test.column.region.not_null", "kept"), + ) + ) + report = compute_drift(previous_diff=prev, current_diff=curr) + assert report.schema_shape_changes.columns_added == ("region",) + assert report.schema_shape_changes.columns_removed == ("amount",) + + +def test_schema_shape_ignores_model_level_artifacts() -> None: + """model.* and test.model.* carry no column → no shape change from them.""" + prev = _diff( + entries=( + _entry("model.description", "kept"), + _entry("test.model.row_count_between", "kept"), + ) + ) + curr = _diff( + entries=( + _entry("model.description", "kept"), + _entry("test.model.unique_combination", "kept"), + ) + ) + report = compute_drift(previous_diff=prev, current_diff=curr) + assert report.schema_shape_changes == SchemaShapeDelta() + + +def test_schema_shape_test_with_args_hash_suffix_parses_column() -> None: + """A test.column... 5-part form still yields the column.""" + prev = _diff(entries=()) + curr = _diff(entries=(_entry("test.column.amount.accepted_values.abcd1234", "kept"),)) + report = compute_drift(previous_diff=prev, current_diff=curr) + assert report.schema_shape_changes.columns_added == ("amount",) + + +# --------------------------------------------------------------------------- +# Grade regression (DEC-005) at / above / below threshold. +# --------------------------------------------------------------------------- + + +def test_grade_regression_at_exactly_threshold_trips() -> None: + """current_mean <= previous_mean - threshold; 0.85 <= 0.90 - 0.05 is True.""" + diff = _diff(entries=(_entry("column.amount.description", "kept"),)) + report = compute_drift( + previous_diff=diff, + current_diff=diff, + previous_grade=_grade(0.90), + current_grade=_grade(0.85), + grade_regression_threshold=0.05, + ) + assert len(report.grade_regressions) == 1 + reg = report.grade_regressions[0] + assert reg.previous_mean == 0.90 + assert reg.current_mean == 0.85 + assert abs(reg.delta - 0.05) < 1e-9 + assert report.alarming is True + + +def test_grade_regression_above_threshold_trips() -> None: + diff = _diff(entries=(_entry("column.amount.description", "kept"),)) + report = compute_drift( + previous_diff=diff, + current_diff=diff, + previous_grade=_grade(0.90), + current_grade=_grade(0.70), + ) + assert len(report.grade_regressions) == 1 + + +def test_grade_regression_below_threshold_does_not_trip() -> None: + """A drop smaller than the threshold (0.04 < 0.05) is not a regression.""" + diff = _diff(entries=(_entry("column.amount.description", "kept"),)) + report = compute_drift( + previous_diff=diff, + current_diff=diff, + previous_grade=_grade(0.90), + current_grade=_grade(0.86), + ) + assert report.grade_regressions == () + assert report.alarming is False + + +def test_grade_improvement_is_not_a_regression() -> None: + diff = _diff(entries=(_entry("column.amount.description", "kept"),)) + report = compute_drift( + previous_diff=diff, + current_diff=diff, + previous_grade=_grade(0.70), + current_grade=_grade(0.95), + ) + assert report.grade_regressions == () + + +def test_no_grade_means_no_regression_and_no_failure() -> None: + """--no-grade (grades None) → grade_regressions empty, never raises.""" + diff = _diff(entries=(_entry("column.amount.description", "kept"),)) + # Both None. + assert compute_drift(previous_diff=diff, current_diff=diff).grade_regressions == () + # Only one present → still empty (need both to compare). + assert ( + compute_drift( + previous_diff=diff, current_diff=diff, current_grade=_grade(0.5) + ).grade_regressions + == () + ) + assert ( + compute_drift( + previous_diff=diff, current_diff=diff, previous_grade=_grade(0.5) + ).grade_regressions + == () + ) + + +# --------------------------------------------------------------------------- +# Degrade: model mismatch (DEC-013). +# --------------------------------------------------------------------------- + + +def test_model_mismatch_degrades_and_is_never_alarming() -> None: + prev = _diff( + model_unique_id="model.shop.fct_orders", + entries=(_entry("test.column.amount.not_null", "dropped", drop_reason="always-passes"),), + ) + curr = _diff( + model_unique_id="model.shop.dim_users", + entries=(_entry("test.column.amount.not_null", "dropped", drop_reason="always-passes"),), + ) + report = compute_drift(previous_diff=prev, current_diff=curr) + assert report.degrade_reason is not None + assert "model mismatch" in report.degrade_reason + assert "model.shop.fct_orders" in report.degrade_reason + assert "model.shop.dim_users" in report.degrade_reason + # Empty transition lists by construction → never alarming. + assert report.newly_always_passes == () + assert report.newly_dropped == () + assert report.newly_kept == () + assert report.grade_regressions == () + assert report.schema_shape_changes == SchemaShapeDelta() + assert report.alarming is False + # Model id reported is the current run's. + assert report.model_unique_id == "model.shop.dim_users" + + +# --------------------------------------------------------------------------- +# Determinism (DEC-016) + report metadata. +# --------------------------------------------------------------------------- + + +def test_compute_drift_is_deterministic() -> None: + """Same inputs → byte-identical model_dump_json on re-run.""" + prev = _diff( + entries=( + _entry("test.column.amount.not_null", "kept"), + _entry("test.column.region.not_null", "kept"), + _entry("column.amount.description", "kept"), + ) + ) + curr = _diff( + entries=( + _entry("test.column.amount.not_null", "dropped", drop_reason="always-passes"), + _entry( + "test.column.region.not_null", + "dropped", + drop_reason="failed-on-known-clean-data", + ), + _entry("column.region.description", "kept"), + ) + ) + a = compute_drift(previous_diff=prev, current_diff=curr, as_of=date(2026, 6, 16)) + b = compute_drift(previous_diff=prev, current_diff=curr, as_of=date(2026, 6, 16)) + assert a.model_dump_json() == b.model_dump_json() + + +def test_transition_lists_and_added_removed_are_sorted() -> None: + """Iteration over sorted artifact_ids → sorted output tuples.""" + prev = _diff( + entries=( + _entry("test.column.zeta.not_null", "kept"), + _entry("test.column.alpha.not_null", "kept"), + ) + ) + curr = _diff( + entries=( + _entry("test.column.zeta.not_null", "dropped", drop_reason="always-passes"), + _entry("test.column.alpha.not_null", "dropped", drop_reason="always-passes"), + ) + ) + report = compute_drift(previous_diff=prev, current_diff=curr) + ids = [a.artifact_id for a in report.newly_always_passes] + assert ids == sorted(ids) + + +def test_report_carries_input_hashes_and_metadata() -> None: + diff = _diff(entries=(_entry("column.amount.description", "kept"),)) + report = compute_drift( + previous_diff=diff, + current_diff=diff, + as_of=date(2026, 6, 16), + grade_regression_threshold=0.1, + ) + assert report.signalforge_version == signalforge.__version__ + assert report.model_unique_id == "model.shop.fct_orders" + assert report.as_of == date(2026, 6, 16) + assert report.grade_regression_threshold == 0.1 + assert report.baseline is False + assert len(report.previous_diff_hash) == 16 + assert len(report.current_diff_hash) == 16 + assert report.degrade_reason is None + + +def test_as_of_serializes_to_bare_iso_date_or_null() -> None: + diff = _diff(entries=()) + with_date = compute_drift(previous_diff=diff, current_diff=diff, as_of=date(2026, 6, 16)) + assert json.loads(with_date.model_dump_json())["as_of"] == "2026-06-16" + without = compute_drift(previous_diff=diff, current_diff=diff) + assert json.loads(without.model_dump_json())["as_of"] is None + + +# --------------------------------------------------------------------------- +# alarming truth table. +# --------------------------------------------------------------------------- + + +def test_alarming_truth_table() -> None: + base = _diff(entries=(_entry("column.amount.description", "kept"),)) + + # No transitions, no regressions → not alarming. + assert compute_drift(previous_diff=base, current_diff=base).alarming is False + + # Signal rot only → alarming. + rot_prev = _diff(entries=(_entry("test.column.amount.not_null", "kept"),)) + rot_curr = _diff( + entries=(_entry("test.column.amount.not_null", "dropped", drop_reason="always-passes"),) + ) + assert compute_drift(previous_diff=rot_prev, current_diff=rot_curr).alarming is True + + # Grade regression only → alarming. + assert ( + compute_drift( + previous_diff=base, + current_diff=base, + previous_grade=_grade(0.9), + current_grade=_grade(0.5), + ).alarming + is True + ) + + # newly_dropped / newly_kept / schema changes only → NOT alarming. + drop_prev = _diff(entries=(_entry("test.column.amount.unique", "kept"),)) + drop_curr = _diff( + entries=( + _entry("test.column.amount.unique", "dropped", drop_reason="requires-future-data"), + ) + ) + assert compute_drift(previous_diff=drop_prev, current_diff=drop_curr).alarming is False + + +# --------------------------------------------------------------------------- +# to_xcom shape (DEC-015) + no bulk text. +# --------------------------------------------------------------------------- + + +def test_to_xcom_shape_and_round_trips_through_json() -> None: + # Engineered so every category is populated: the model-level test rots to + # always-passes (signal rot), the amount column's doc is removed, the + # region column's doc is added, and the grade drops 0.9 → 0.5. + prev = _diff( + entries=( + _entry("test.model.row_count_between", "kept", why="caught row-count drift"), + _entry("column.amount.description", "kept"), + ) + ) + curr = _diff( + entries=( + _entry( + "test.model.row_count_between", + "dropped", + drop_reason="always-passes", + why="always passes", + ), + _entry("column.region.description", "kept"), + ) + ) + report = compute_drift( + previous_diff=prev, + current_diff=curr, + previous_grade=_grade(0.9), + current_grade=_grade(0.5), + as_of=date(2026, 6, 16), + ) + xcom = report.to_xcom() + # JSON-serialisable. + round_tripped = json.loads(json.dumps(xcom)) + assert round_tripped == xcom + + assert xcom["alarming"] is True + assert xcom["as_of"] == "2026-06-16" + assert xcom["baseline"] is False + assert xcom["model_unique_id"] == "model.shop.fct_orders" + assert xcom["degrade_reason"] is None + assert isinstance(xcom["previous_diff_hash"], str) + assert isinstance(xcom["current_diff_hash"], str) + + counts = xcom["counts"] + assert isinstance(counts, dict) + assert counts["newly_always_passes"] == 1 + assert counts["grade_regressions"] == 1 + assert counts["columns_added"] == 1 + assert counts["columns_removed"] == 1 + assert counts["added_artifacts"] == 1 + assert counts["removed_artifacts"] == 1 + + assert len(xcom["newly_always_passes"]) == 1 + assert xcom["newly_always_passes"][0]["artifact_id"] == "test.model.row_count_between" + assert len(xcom["grade_regressions"]) == 1 + assert xcom["schema_shape_changes"]["columns_added"] == ["region"] + assert xcom["schema_shape_changes"]["columns_removed"] == ["amount"] + + +def test_to_xcom_carries_no_bulk_sidecar_text() -> None: + """to_xcom keys never include raw YAML / unified diff / stdout.""" + diff = _diff(entries=(_entry("column.amount.description", "kept"),)) + xcom = compute_drift(previous_diff=diff, current_diff=diff).to_xcom() + forbidden = {"proposed_yaml", "existing_yaml", "unified_diff", "stdout", "stderr"} + assert forbidden.isdisjoint(xcom.keys()) + + +# --------------------------------------------------------------------------- +# DriftArtifact why truncation. +# --------------------------------------------------------------------------- + + +def test_drift_artifact_truncates_long_why() -> None: + long_why = "x" * 500 + artifact = DriftArtifact( + artifact_id="test.column.amount.not_null", + previous_tier="kept", + current_tier="dropped", + previous_drop_reason=None, + current_drop_reason="always-passes", + why=long_why, + ) + assert len(artifact.why) <= 200 + assert artifact.why.endswith("…") + + +def test_truncate_why_helper_edge_cases() -> None: + """Direct coverage of the shared truncation helper's branches.""" + from signalforge.airflow.drift import _truncate_why + + assert _truncate_why("") == "" + assert _truncate_why(" ") == "" + # Non-positive budget has no room even for the ellipsis. + assert _truncate_why("anything", 0) == "" + assert _truncate_why("anything", -5) == "" + # At/below budget → rstrip only, no ellipsis. + assert _truncate_why("short ") == "short" + # Over budget → hard cut + ellipsis. + truncated = _truncate_why("y" * 10, 4) + assert truncated == "yyy…" + + +def test_drift_artifact_blank_why_is_empty() -> None: + artifact = DriftArtifact( + artifact_id="x", + previous_tier="kept", + current_tier="dropped", + previous_drop_reason=None, + current_drop_reason="always-passes", + why=" ", + ) + assert artifact.why == "" + + +def test_grade_regression_is_a_frozen_model() -> None: + reg = GradeRegression( + model_unique_id="model.shop.fct_orders", + previous_mean=0.9, + current_mean=0.5, + delta=0.4, + ) + assert reg.model_dump() == { + "model_unique_id": "model.shop.fct_orders", + "previous_mean": 0.9, + "current_mean": 0.5, + "delta": 0.4, + } + + +def test_repr_omits_long_lists() -> None: + prev = _diff(entries=(_entry("test.column.amount.not_null", "kept", why="x" * 300),)) + curr = _diff( + entries=( + _entry( + "test.column.amount.not_null", + "dropped", + drop_reason="always-passes", + why="x" * 300, + ), + ) + ) + report = compute_drift(previous_diff=prev, current_diff=curr) + text = repr(report) + assert "DriftReport(" in text + assert "newly_always_passes=1" in text + assert "alarming=True" in text + # The long why is not dumped into the repr. + assert "x" * 300 not in text + # __repr_args__ mirrors the redacted field set. + keys = [k for k, _ in report.__repr_args__()] + assert "model_unique_id" in keys + assert "alarming" in keys + + +# --------------------------------------------------------------------------- +# Committed fixture pair (real serialized DiffReport / GradingReport JSON). +# --------------------------------------------------------------------------- + + +def test_signal_rot_from_committed_fixture_pair() -> None: + """The engineered signal-rot fixture pair drives a real cross-run compare. + + Validates that genuine ``DiffReport.model_dump_json`` / ``GradingReport`` + JSON (round-tripped through ``model_validate_json``) flows through + ``compute_drift`` and yields the headline signal-rot + grade-regression + alarm. + """ + prev_diff = SfDiffReport.model_validate_json( + (_FIXTURE_DIR / "signal_rot_prev_diff.json").read_text() + ) + curr_diff = SfDiffReport.model_validate_json( + (_FIXTURE_DIR / "signal_rot_curr_diff.json").read_text() + ) + prev_grade = GradingReport.model_validate_json( + (_FIXTURE_DIR / "signal_rot_prev_grade.json").read_text() + ) + curr_grade = GradingReport.model_validate_json( + (_FIXTURE_DIR / "signal_rot_curr_grade.json").read_text() + ) + report = compute_drift( + previous_diff=prev_diff, + current_diff=curr_diff, + previous_grade=prev_grade, + current_grade=curr_grade, + as_of=date(2026, 6, 16), + ) + assert report.alarming is True + assert len(report.newly_always_passes) == 1 + assert report.newly_always_passes[0].artifact_id == "test.column.amount.not_null" + # 0.9 → 0.8 is a 0.10 drop ≥ 0.05 threshold. + assert len(report.grade_regressions) == 1 + assert report.grade_regressions[0].model_unique_id == "model.shop.fct_orders" + # The kept doc column is unchanged → no transition for it. + assert report.newly_dropped == () + assert report.newly_kept == () + + +def test_eager_reexport_is_in_public_api() -> None: + """compute_drift / DriftReport are eager-importable from the package root. + + Asserts the names are in ``signalforge.airflow.__all__`` and resolve to a + usable callable / model — NOT object identity against this module's + top-level binding, which a sibling test + (``test_airflow_no_eager_import.py``) deliberately invalidates by purging + ``signalforge.airflow.*`` from ``sys.modules``. (The no-airflow-import + guarantee itself is pinned, with proper cleanup, in that sibling test.) + """ + import signalforge.airflow as sf_airflow + + for name in ( + "compute_drift", + "DriftReport", + "DriftArtifact", + "GradeRegression", + "SchemaShapeDelta", + ): + assert name in sf_airflow.__all__ + + diff = _diff(entries=(_entry("column.amount.description", "kept"),)) + report = sf_airflow.compute_drift(previous_diff=diff, current_diff=diff) + assert isinstance(report, sf_airflow.DriftReport) diff --git a/tests/fixtures/airflow/drift_pairs/signal_rot_curr_diff.json b/tests/fixtures/airflow/drift_pairs/signal_rot_curr_diff.json new file mode 100644 index 00000000..9c676fc7 --- /dev/null +++ b/tests/fixtures/airflow/drift_pairs/signal_rot_curr_diff.json @@ -0,0 +1,40 @@ +{ + "schema_version": 1, + "audit_schema_version": 3, + "signalforge_version": "0.7.0.dev0", + "model_unique_id": "model.shop.fct_orders", + "run_id": "0000000000000000000000000000curr", + "duration_seconds": 1.6, + "proposed_yaml": "version: 2\n", + "existing_yaml": null, + "unified_diff": "", + "entries": [ + { + "artifact_id": "test.column.amount.not_null", + "test_type": "not_null", + "tier": "dropped", + "drop_reason": "always-passes", + "why": "always passes on the warehouse sample (0 failing rows)", + "score": null, + "passed": null + }, + { + "artifact_id": "column.amount.description", + "test_type": null, + "tier": "kept", + "drop_reason": null, + "why": "documents the order amount column", + "score": null, + "passed": null + } + ], + "proposed_test_files": [], + "kept_count": 1, + "kept_uncertain_count": 0, + "dropped_count": 1, + "flagged_count": 0, + "has_existing_schema": false, + "candidate_hash": "dddddddddddddddd", + "prune_result_hash": "eeeeeeeeeeeeeeee", + "grading_report_hash": "ffffffffffffffff" +} diff --git a/tests/fixtures/airflow/drift_pairs/signal_rot_curr_grade.json b/tests/fixtures/airflow/drift_pairs/signal_rot_curr_grade.json new file mode 100644 index 00000000..cefdd77b --- /dev/null +++ b/tests/fixtures/airflow/drift_pairs/signal_rot_curr_grade.json @@ -0,0 +1,20 @@ +{ + "grade_schema_version": 1, + "signalforge_version": "0.7.0.dev0", + "run_id": "0000000000000000000000000000curr", + "timestamp": "2026-06-16T00:00:00.000000Z", + "duration_seconds": 2.1, + "model_unique_id": "model.shop.fct_orders", + "rubric_hash": "1111111111111111", + "thresholds": [0.7, 0.7], + "results": [ + { + "artifact_id": "column.amount.description", + "criterion_id": "clarity", + "score": 0.8, + "passed": true, + "evidence": "", + "reasoning": "" + } + ] +} diff --git a/tests/fixtures/airflow/drift_pairs/signal_rot_prev_diff.json b/tests/fixtures/airflow/drift_pairs/signal_rot_prev_diff.json new file mode 100644 index 00000000..74eb2b09 --- /dev/null +++ b/tests/fixtures/airflow/drift_pairs/signal_rot_prev_diff.json @@ -0,0 +1,40 @@ +{ + "schema_version": 1, + "audit_schema_version": 3, + "signalforge_version": "0.7.0.dev0", + "model_unique_id": "model.shop.fct_orders", + "run_id": "0000000000000000000000000000prev", + "duration_seconds": 1.5, + "proposed_yaml": "version: 2\n", + "existing_yaml": null, + "unified_diff": "", + "entries": [ + { + "artifact_id": "test.column.amount.not_null", + "test_type": "not_null", + "tier": "kept", + "drop_reason": null, + "why": "caught 4 failing rows on the warehouse sample", + "score": null, + "passed": null + }, + { + "artifact_id": "column.amount.description", + "test_type": null, + "tier": "kept", + "drop_reason": null, + "why": "documents the order amount column", + "score": null, + "passed": null + } + ], + "proposed_test_files": [], + "kept_count": 2, + "kept_uncertain_count": 0, + "dropped_count": 0, + "flagged_count": 0, + "has_existing_schema": false, + "candidate_hash": "aaaaaaaaaaaaaaaa", + "prune_result_hash": "bbbbbbbbbbbbbbbb", + "grading_report_hash": "cccccccccccccccc" +} diff --git a/tests/fixtures/airflow/drift_pairs/signal_rot_prev_grade.json b/tests/fixtures/airflow/drift_pairs/signal_rot_prev_grade.json new file mode 100644 index 00000000..3645b142 --- /dev/null +++ b/tests/fixtures/airflow/drift_pairs/signal_rot_prev_grade.json @@ -0,0 +1,20 @@ +{ + "grade_schema_version": 1, + "signalforge_version": "0.7.0.dev0", + "run_id": "0000000000000000000000000000prev", + "timestamp": "2026-06-15T00:00:00.000000Z", + "duration_seconds": 2.0, + "model_unique_id": "model.shop.fct_orders", + "rubric_hash": "1111111111111111", + "thresholds": [0.7, 0.7], + "results": [ + { + "artifact_id": "column.amount.description", + "criterion_id": "clarity", + "score": 0.9, + "passed": true, + "evidence": "", + "reasoning": "" + } + ] +} From b736b9999f601677d5f85d5a78ff190db653147b Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 12:48:10 -0700 Subject: [PATCH 04/13] bd_1-scaffolding-v42.2: StrictDriftReport schema-stability drift detector + fixture (#235 US-002) --- tests/airflow/test_drift_report_schema.py | 359 ++++++++++++++++++++ tests/fixtures/airflow/drift_report_v1.json | 64 ++++ 2 files changed, 423 insertions(+) create mode 100644 tests/airflow/test_drift_report_schema.py create mode 100644 tests/fixtures/airflow/drift_report_v1.json diff --git a/tests/airflow/test_drift_report_schema.py b/tests/airflow/test_drift_report_schema.py new file mode 100644 index 00000000..b08a3e3c --- /dev/null +++ b/tests/airflow/test_drift_report_schema.py @@ -0,0 +1,359 @@ +"""Schema-stability drift detector for the Airflow drift core (US-002, DEC-017 of #235). + +Pairs the production ``extra="ignore"`` read-back models from +:mod:`signalforge.airflow.drift` with ``extra="forbid"`` ``Strict`` +mirrors validated against the committed fixture +:file:`tests/fixtures/airflow/drift_report_v1.json`. Adding a field to a +production model without updating the strict mirror OR the fixture breaks +this test loudly. + +**Ungated** (NO ``@pytest.mark.airflow``): :class:`DriftReport` and its +sub-models are airflow-free (DEC-002 — the pure core imports no airflow), so +the drift detector runs in the default pytest suite without an Apache Airflow +install. + +Mirrors :mod:`tests.grade.test_drift_detector` and +:mod:`tests.diff.test_drift_detector` shape verbatim. The four +airflow-drift read-back models covered here: + +* :class:`signalforge.airflow.drift.DriftArtifact` +* :class:`signalforge.airflow.drift.GradeRegression` +* :class:`signalforge.airflow.drift.SchemaShapeDelta` +* :class:`signalforge.airflow.drift.DriftReport` + +Reference: ``.claude/rules/manifest-readers.md`` (drift detectors +mandatory for ``extra="ignore"`` reader-shaped models), +``.claude/rules/diff-renderer.md`` DEC-003 (pair every read-back model with +a one-off ``extra="forbid"`` mirror + committed fixture), +``.claude/rules/testing-signal.md`` (the drift-detector mandate), +``plans/super/235-drift-detection.md`` DEC-004 (the field set) + DEC-017 +(this task). +""" + +from __future__ import annotations + +import json +from datetime import date +from pathlib import Path +from typing import Literal + +import pytest +from pydantic import BaseModel, ConfigDict, ValidationError + +from signalforge.airflow.drift import ( + DriftArtifact, + DriftReport, + GradeRegression, + SchemaShapeDelta, +) + +_STRICT = ConfigDict(frozen=True, extra="forbid", populate_by_name=True) +_FIXTURES_DIR = Path(__file__).resolve().parent.parent / "fixtures" / "airflow" + + +# --------------------------------------------------------------------------- +# Strict drift mirrors. +# --------------------------------------------------------------------------- + + +class StrictDriftArtifact(BaseModel): + """One-off ``extra="forbid"`` mirror of :class:`DriftArtifact`. + + If you add a field to :class:`DriftArtifact`, you MUST: + + 1. Add it here, and + 2. Update :file:`tests/fixtures/airflow/drift_report_v1.json` (each of + the three transition lists carries a populated artifact). + """ + + model_config = _STRICT + + artifact_id: str + previous_tier: str | None + current_tier: str | None + previous_drop_reason: str | None + current_drop_reason: str | None + why: str = "" + + +class StrictGradeRegression(BaseModel): + """One-off ``extra="forbid"`` mirror of :class:`GradeRegression`.""" + + model_config = _STRICT + + model_unique_id: str + previous_mean: float + current_mean: float + delta: float + + +class StrictSchemaShapeDelta(BaseModel): + """One-off ``extra="forbid"`` mirror of :class:`SchemaShapeDelta`.""" + + model_config = _STRICT + + columns_added: tuple[str, ...] = () + columns_removed: tuple[str, ...] = () + + +class StrictDriftReport(BaseModel): + """One-off ``extra="forbid"`` mirror of :class:`DriftReport`. + + Stamps every field type :class:`DriftReport` declares with the same + typing — including the ``Literal[1]`` schema sentinel, the ``date | None`` + ``as_of``, and the nested ``StrictDriftArtifact`` / ``StrictGradeRegression`` + tuples + the nested ``StrictSchemaShapeDelta``. + + The computed ``alarming`` property lives on production :class:`DriftReport` + as a plain ``@property`` (NOT a Pydantic ``computed_field``), so it is + absent from ``model_fields`` and is not part of the field-set parity below. + """ + + model_config = _STRICT + + schema_version: Literal[1] = 1 + signalforge_version: str + model_unique_id: str + as_of: date | None + grade_regression_threshold: float + baseline: bool = False + previous_diff_hash: str + current_diff_hash: str + newly_always_passes: tuple[StrictDriftArtifact, ...] = () + newly_dropped: tuple[StrictDriftArtifact, ...] = () + newly_kept: tuple[StrictDriftArtifact, ...] = () + added_artifacts: tuple[str, ...] = () + removed_artifacts: tuple[str, ...] = () + grade_regressions: tuple[StrictGradeRegression, ...] = () + schema_shape_changes: StrictSchemaShapeDelta = StrictSchemaShapeDelta() + degrade_reason: str | None = None + + +# --------------------------------------------------------------------------- +# Fixture validation. +# --------------------------------------------------------------------------- + + +def test_strict_drift_report_validates_fixture() -> None: + """The :file:`drift_report_v1.json` fixture validates against + :class:`StrictDriftReport`. + + If this raises, an unknown field was introduced in the fixture without + being mirrored on :class:`StrictDriftReport` (or vice versa). Update + production :class:`DriftReport`, :class:`StrictDriftReport`, and the + fixture together. + """ + fixture_path = _FIXTURES_DIR / "drift_report_v1.json" + payload = json.loads(fixture_path.read_text(encoding="utf-8")) + StrictDriftReport.model_validate(payload) + + +def test_fixture_exercises_every_drift_report_field() -> None: + """The committed fixture populates EVERY :class:`DriftReport` field — + including at least one entry in each transition list, one grade + regression, and a non-empty schema-shape delta. + + Without this, a fixture edit could quietly drop a transition list to + empty and the strict mirror would still validate, silently weakening the + typing coverage on the nested ``Strict*`` tuples. + """ + fixture_path = _FIXTURES_DIR / "drift_report_v1.json" + payload = json.loads(fixture_path.read_text(encoding="utf-8")) + + # Every top-level field name from production is present as a JSON key. + prod_fields = set(DriftReport.model_fields.keys()) + fixture_keys = set(payload.keys()) + assert prod_fields <= fixture_keys, ( + f"drift_report_v1.json is missing DriftReport fields: {prod_fields - fixture_keys}" + ) + + # The three transition lists each carry at least one populated artifact. + assert payload["newly_always_passes"], "fixture must populate newly_always_passes" + assert payload["newly_dropped"], "fixture must populate newly_dropped" + assert payload["newly_kept"], "fixture must populate newly_kept" + # One grade regression + a non-empty schema-shape delta. + assert payload["grade_regressions"], "fixture must populate grade_regressions" + assert payload["schema_shape_changes"]["columns_added"], ( + "fixture must populate schema_shape_changes.columns_added" + ) + assert payload["schema_shape_changes"]["columns_removed"], ( + "fixture must populate schema_shape_changes.columns_removed" + ) + # Informational artifact lists are populated too. + assert payload["added_artifacts"], "fixture must populate added_artifacts" + assert payload["removed_artifacts"], "fixture must populate removed_artifacts" + + +def test_strict_drift_artifact_validates_each_transition_entry() -> None: + """Each artifact in every transition list of :file:`drift_report_v1.json` + validates against :class:`StrictDriftArtifact` (``extra="forbid"``). + + Exercises the ``previous_drop_reason``/``current_drop_reason`` + ``str | None`` typing end-to-end: across the three lists the fixture + covers both a populated ``current_drop_reason`` (signal rot → + ``always-passes``) and a null one (newly kept). + """ + fixture_path = _FIXTURES_DIR / "drift_report_v1.json" + payload = json.loads(fixture_path.read_text(encoding="utf-8")) + artifacts = ( + payload["newly_always_passes"] + payload["newly_dropped"] + payload["newly_kept"] + ) + assert artifacts, "expected at least one transition artifact in the fixture" + saw_drop_reason = False + saw_null_drop_reason = False + for entry in artifacts: + StrictDriftArtifact.model_validate(entry) + if entry["current_drop_reason"] is None: + saw_null_drop_reason = True + else: + saw_drop_reason = True + assert saw_drop_reason, ( + "fixture must include a transition with a populated current_drop_reason" + ) + assert saw_null_drop_reason, ( + "fixture must include a transition with a null current_drop_reason" + ) + + +# --------------------------------------------------------------------------- +# Field-set parity. +# --------------------------------------------------------------------------- + + +def test_drift_artifact_field_set_parity() -> None: + """:class:`StrictDriftArtifact` ``model_fields`` exactly match + :class:`DriftArtifact` ``model_fields``. + """ + strict_fields = set(StrictDriftArtifact.model_fields.keys()) + prod_fields = set(DriftArtifact.model_fields.keys()) + missing_in_strict = prod_fields - strict_fields + extra_in_strict = strict_fields - prod_fields + assert not missing_in_strict, ( + f"StrictDriftArtifact is missing fields present in DriftArtifact: " + f"{missing_in_strict}. Update StrictDriftArtifact to match." + ) + assert not extra_in_strict, ( + f"StrictDriftArtifact has fields absent from DriftArtifact: " + f"{extra_in_strict}. Remove from StrictDriftArtifact or add to DriftArtifact." + ) + + +def test_grade_regression_field_set_parity() -> None: + """:class:`StrictGradeRegression` ``model_fields`` exactly match + :class:`GradeRegression` ``model_fields``. + """ + strict_fields = set(StrictGradeRegression.model_fields.keys()) + prod_fields = set(GradeRegression.model_fields.keys()) + missing_in_strict = prod_fields - strict_fields + extra_in_strict = strict_fields - prod_fields + assert not missing_in_strict, ( + f"StrictGradeRegression is missing fields present in GradeRegression: " + f"{missing_in_strict}. Update StrictGradeRegression to match." + ) + assert not extra_in_strict, ( + f"StrictGradeRegression has fields absent from GradeRegression: " + f"{extra_in_strict}. Remove from StrictGradeRegression or add to GradeRegression." + ) + + +def test_schema_shape_delta_field_set_parity() -> None: + """:class:`StrictSchemaShapeDelta` ``model_fields`` exactly match + :class:`SchemaShapeDelta` ``model_fields``. + """ + strict_fields = set(StrictSchemaShapeDelta.model_fields.keys()) + prod_fields = set(SchemaShapeDelta.model_fields.keys()) + missing_in_strict = prod_fields - strict_fields + extra_in_strict = strict_fields - prod_fields + assert not missing_in_strict, ( + f"StrictSchemaShapeDelta is missing fields present in SchemaShapeDelta: " + f"{missing_in_strict}. Update StrictSchemaShapeDelta to match." + ) + assert not extra_in_strict, ( + f"StrictSchemaShapeDelta has fields absent from SchemaShapeDelta: " + f"{extra_in_strict}. Remove from StrictSchemaShapeDelta or add to SchemaShapeDelta." + ) + + +def test_drift_report_field_set_parity() -> None: + """:class:`StrictDriftReport` ``model_fields`` exactly match + :class:`DriftReport` ``model_fields``. + + ``alarming`` is a plain ``@property`` on production :class:`DriftReport` + (NOT a Pydantic ``computed_field``), so it lives on neither + ``model_fields`` nor ``model_computed_fields`` — it is not part of this + comparison. + """ + strict_fields = set(StrictDriftReport.model_fields.keys()) + prod_fields = set(DriftReport.model_fields.keys()) + missing_in_strict = prod_fields - strict_fields + extra_in_strict = strict_fields - prod_fields + assert not missing_in_strict, ( + f"StrictDriftReport is missing fields present in DriftReport: " + f"{missing_in_strict}. Update StrictDriftReport to match." + ) + assert not extra_in_strict, ( + f"StrictDriftReport has fields absent from DriftReport: " + f"{extra_in_strict}. Remove from StrictDriftReport or add to DriftReport." + ) + + +# --------------------------------------------------------------------------- +# The standard pair: production extra="ignore", strict mirror extra="forbid". +# --------------------------------------------------------------------------- + + +def test_production_models_are_extra_ignore_and_strict_mirrors_are_extra_forbid() -> None: + """Production read-back models use ``extra="ignore"`` (forward-compat); + their strict mirrors use ``extra="forbid"`` (the drift gate). + + This is the standard drift-detector pair (``.claude/rules/diff-renderer.md`` + DEC-003 / ``manifest-readers.md``): production tolerates an upstream field + addition silently, while the strict mirror fails loudly so the addition is + caught and mirrored in the same change. + """ + production = (DriftArtifact, GradeRegression, SchemaShapeDelta, DriftReport) + for model in production: + assert model.model_config.get("extra") == "ignore", ( + f"{model.__name__} must use extra='ignore' for forward-compat" + ) + strict = ( + StrictDriftArtifact, + StrictGradeRegression, + StrictSchemaShapeDelta, + StrictDriftReport, + ) + for model in strict: + assert model.model_config.get("extra") == "forbid", ( + f"{model.__name__} must use extra='forbid' to gate schema drift" + ) + + +# --------------------------------------------------------------------------- +# Sanity floor — extra="forbid" actually fires. +# --------------------------------------------------------------------------- + + +def test_strict_drift_report_rejects_unknown_field() -> None: + """Sanity floor: a fixture with an extra unknown field raises + :class:`ValidationError`. + + Confirms ``extra="forbid"`` is wired up — a silently-accepted unknown + field would defeat the entire drift gate. Mirrors + ``test_strict_diff_report_rejects_unknown_field`` and + ``test_strict_grade_event_rejects_unknown_field``. + """ + fixture_path = _FIXTURES_DIR / "drift_report_v1.json" + payload = json.loads(fixture_path.read_text(encoding="utf-8")) + payload["future_field_that_should_not_exist"] = "boom" + with pytest.raises(ValidationError): + StrictDriftReport.model_validate(payload) + + +def test_strict_drift_artifact_rejects_unknown_field() -> None: + """Same sanity floor for :class:`StrictDriftArtifact`.""" + fixture_path = _FIXTURES_DIR / "drift_report_v1.json" + payload = json.loads(fixture_path.read_text(encoding="utf-8")) + entry = dict(payload["newly_always_passes"][0]) + entry["future_field_that_should_not_exist"] = "boom" + with pytest.raises(ValidationError): + StrictDriftArtifact.model_validate(entry) diff --git a/tests/fixtures/airflow/drift_report_v1.json b/tests/fixtures/airflow/drift_report_v1.json new file mode 100644 index 00000000..379db1ee --- /dev/null +++ b/tests/fixtures/airflow/drift_report_v1.json @@ -0,0 +1,64 @@ +{ + "schema_version": 1, + "signalforge_version": "0.7.0.dev0", + "model_unique_id": "model.jaffle_shop.customers", + "as_of": "2026-06-16", + "grade_regression_threshold": 0.05, + "baseline": false, + "previous_diff_hash": "0123456789abcdef", + "current_diff_hash": "fedcba9876543210", + "newly_always_passes": [ + { + "artifact_id": "test.column.customer_id.not_null", + "previous_tier": "kept", + "current_tier": "dropped", + "previous_drop_reason": null, + "current_drop_reason": "always-passes", + "why": "ran clean on warehouse sample \u2014 rotted into always-passes" + } + ], + "newly_dropped": [ + { + "artifact_id": "test.column.order_total.accepted_values", + "previous_tier": "flagged", + "current_tier": "dropped", + "previous_drop_reason": null, + "current_drop_reason": "failed-on-known-clean-data", + "why": "failed on a trusted-model clean sample" + } + ], + "newly_kept": [ + { + "artifact_id": "test.column.email.unique", + "previous_tier": "dropped", + "current_tier": "kept", + "previous_drop_reason": "always-passes", + "current_drop_reason": null, + "why": "now catches duplicate emails" + } + ], + "added_artifacts": [ + "column.created_at.description", + "test.column.created_at.not_null" + ], + "removed_artifacts": [ + "column.legacy_flag.description" + ], + "grade_regressions": [ + { + "model_unique_id": "model.jaffle_shop.customers", + "previous_mean": 0.92, + "current_mean": 0.71, + "delta": 0.21 + } + ], + "schema_shape_changes": { + "columns_added": [ + "created_at" + ], + "columns_removed": [ + "legacy_flag" + ] + }, + "degrade_reason": null +} From 3426ab3f4eee8f0e22527ed92482aa5a882587dd Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 12:49:35 -0700 Subject: [PATCH 05/13] bd_1-scaffolding-v42.2: ruff-format test_drift_report_schema.py (merge hygiene) --- tests/airflow/test_drift_report_schema.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/tests/airflow/test_drift_report_schema.py b/tests/airflow/test_drift_report_schema.py index b08a3e3c..ddd0a574 100644 --- a/tests/airflow/test_drift_report_schema.py +++ b/tests/airflow/test_drift_report_schema.py @@ -195,9 +195,7 @@ def test_strict_drift_artifact_validates_each_transition_entry() -> None: """ fixture_path = _FIXTURES_DIR / "drift_report_v1.json" payload = json.loads(fixture_path.read_text(encoding="utf-8")) - artifacts = ( - payload["newly_always_passes"] + payload["newly_dropped"] + payload["newly_kept"] - ) + artifacts = payload["newly_always_passes"] + payload["newly_dropped"] + payload["newly_kept"] assert artifacts, "expected at least one transition artifact in the fixture" saw_drop_reason = False saw_null_drop_reason = False @@ -207,12 +205,8 @@ def test_strict_drift_artifact_validates_each_transition_entry() -> None: saw_null_drop_reason = True else: saw_drop_reason = True - assert saw_drop_reason, ( - "fixture must include a transition with a populated current_drop_reason" - ) - assert saw_null_drop_reason, ( - "fixture must include a transition with a null current_drop_reason" - ) + assert saw_drop_reason, "fixture must include a transition with a populated current_drop_reason" + assert saw_null_drop_reason, "fixture must include a transition with a null current_drop_reason" # --------------------------------------------------------------------------- From 38bd9d0cdee0e97655b36221992ed0fd66e36af0 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 12:53:18 -0700 Subject: [PATCH 06/13] bd_1-scaffolding-v42.3: fail-soft loaders + decide_task_outcome on_drift (#235 US-003) --- src/signalforge/airflow/drift.py | 102 ++++++++++- src/signalforge/airflow/result.py | 101 +++++++++-- tests/airflow/test_drift_loaders.py | 259 ++++++++++++++++++++++++++++ tests/airflow/test_result.py | 138 +++++++++++++++ 4 files changed, 583 insertions(+), 17 deletions(-) create mode 100644 tests/airflow/test_drift_loaders.py diff --git a/src/signalforge/airflow/drift.py b/src/signalforge/airflow/drift.py index 6fffe986..de7b87ad 100644 --- a/src/signalforge/airflow/drift.py +++ b/src/signalforge/airflow/drift.py @@ -37,16 +37,27 @@ import json from collections.abc import Iterable from datetime import date +from pathlib import Path from typing import Literal -from pydantic import BaseModel, ConfigDict, field_serializer, field_validator +from pydantic import BaseModel, ConfigDict, ValidationError, field_serializer, field_validator import signalforge +from signalforge._common.path_safety import PathContainmentError from signalforge.diff.models import DiffEntry, DiffReport from signalforge.grade.models import GradingReport _BASE_CONFIG = ConfigDict(frozen=True, extra="ignore", populate_by_name=True) +# Byte cap for a sidecar read by the fail-soft loaders (DEC-007). Mirrors the +# diff layer's sidecar cap (``signalforge.diff._sidecar`` — +# ``_DIFF_SIDECAR_RECORD_LIMIT_BYTES = 10_000_000``): a drift comparison reads a +# prior ``diff.json`` / ``grade.json`` written by that same writer, so the read +# cap is sized to the write cap. An over-cap file degrades to ``None`` (the run +# becomes a baseline / skips the comparison) rather than loading a hostile or +# runaway payload into memory. +_DRIFT_SIDECAR_SIZE_LIMIT_BYTES = 10_000_000 + # Truncation budget for the per-artifact ``why`` string carried on a # :class:`DriftArtifact` (mirrors the diff layer's ``_truncate_why`` idea — a # one-line operator-readable explanation, not a multi-line dump). @@ -461,10 +472,99 @@ def compute_drift( ) +def _load_report_json( + path: str | Path, model_cls: type[DiffReport] | type[GradingReport] +) -> DiffReport | GradingReport | None: + """Read + validate a sidecar JSON file into ``model_cls``, FAIL-SOFT. + + Shared body for :func:`load_diff_report` / :func:`load_grade_report`. Returns + ``None`` (never raises) on every recoverable fault — absent file, symlink + cycle, oversize, unreadable, malformed JSON, or wrong shape — so a drift + comparison degrades to a baseline / skipped-grade run rather than crashing + the Airflow task (DEC-007 / DEC-013). + + **No project-dir containment (DEC-008).** The prior-read path is + operator-TRUSTED config (``detect_drift_against`` / a history-dir mount) and + may legitimately live OUTSIDE any project tree, so the project-anchored + :func:`signalforge._common.path_safety.canonicalise_path` gate cannot apply. + The path is still **symlink-loop-hardened**: it is resolved ``strict=True`` + first so a cycle surfaces (``OSError(errno.ELOOP)`` on Python >= 3.13, + ``RuntimeError`` on <= 3.12) instead of slipping silently through + ``strict=False``; either signal is caught and degraded to ``None``. A + genuinely missing target falls back to ``strict=False`` and degrades on the + subsequent stat/read. + """ + raw = Path(path) + try: + try: + resolved = raw.resolve(strict=True) + except (FileNotFoundError, NotADirectoryError): + # Target does not exist yet — best-effort lexical resolution; the + # subsequent stat()/read() raises FileNotFoundError → ``None``. + resolved = raw.resolve(strict=False) + if resolved.stat().st_size > _DRIFT_SIDECAR_SIZE_LIMIT_BYTES: + return None + text = resolved.read_text(encoding="utf-8") + return model_cls.model_validate_json(text) + except (OSError, RuntimeError, json.JSONDecodeError, ValidationError, PathContainmentError): + # OSError covers ELOOP (>=3.13 cycle), missing/unreadable file, and any + # other resolve/stat/read failure; RuntimeError covers the <=3.12 cycle + # signal; (JSONDecodeError, ValidationError) cover malformed JSON / wrong + # shape. PathContainmentError is defensive — this loader does not enforce + # containment (DEC-008), but catching it keeps the fail-soft contract + # total if a future refactor routes resolution through a containment gate. + return None + + +def load_diff_report(path: str | Path) -> DiffReport | None: + """Load a prior ``diff.json`` sidecar into a :class:`DiffReport`, FAIL-SOFT. + + Returns ``None`` (never raises) on absent / corrupt / oversize / wrong-shape + input, or on a symlink cycle in ``path``. The path is operator-trusted and + NOT project-contained (DEC-008); see :func:`_load_report_json`. + """ + report = _load_report_json(path, DiffReport) + # ``isinstance`` narrows the union return for the type-checker; a non-None + # result of ``_load_report_json(path, DiffReport)`` is always a DiffReport. + return report if isinstance(report, DiffReport) else None + + +def load_grade_report(path: str | Path) -> GradingReport | None: + """Load a prior ``grade.json`` sidecar into a :class:`GradingReport`, FAIL-SOFT. + + Returns ``None`` (never raises) on absent / corrupt / oversize / wrong-shape + input, or on a symlink cycle in ``path``. The path is operator-trusted and + NOT project-contained (DEC-008); see :func:`_load_report_json`. + """ + report = _load_report_json(path, GradingReport) + return report if isinstance(report, GradingReport) else None + + +def parse_diff_report(stdout: str) -> DiffReport | None: + """Parse the CURRENT :class:`DiffReport` from ``run_signalforge`` stdout, FAIL-SOFT. + + Under the operator's default ``write=False`` → ``--dry-run`` there is no + on-disk current sidecar, but the diff JSON is on stdout (#231 DEC-005). This + mirrors :func:`signalforge.airflow.runner._parse_diff_stdout`'s empty-guard, + then validates the text straight into a :class:`DiffReport`. Returns ``None`` + (never raises) on empty / non-JSON / wrong-shape stdout. + """ + text = stdout.strip() + if not text: + return None + try: + return DiffReport.model_validate_json(text) + except (json.JSONDecodeError, ValidationError): + return None + + __all__ = [ "DriftArtifact", "DriftReport", "GradeRegression", "SchemaShapeDelta", "compute_drift", + "load_diff_report", + "load_grade_report", + "parse_diff_report", ] diff --git a/src/signalforge/airflow/result.py b/src/signalforge/airflow/result.py index 7d7620ed..ecb40f3f 100644 --- a/src/signalforge/airflow/result.py +++ b/src/signalforge/airflow/result.py @@ -29,13 +29,27 @@ from dataclasses import dataclass from enum import StrEnum -from typing import Literal +from typing import TYPE_CHECKING, Literal + +if TYPE_CHECKING: + # Type-checker-only import: keeps ``result.py`` airflow-free AND avoids any + # runtime import edge (``drift.py`` does not import ``result.py``, so there + # is no cycle today — but the annotation is a string under + # ``from __future__ import annotations`` and ``decide_task_outcome`` only + # duck-types ``drift.alarming`` at runtime, so no runtime import is needed). + from signalforge.airflow.drift import DriftReport # How an operator should treat an exit-0 run that flagged below-threshold # artifacts. Operator-supplied policy; the default ("fail") makes a flagged run a # hard task failure so a reviewer sees it. OnFlagged = Literal["fail", "skip", "succeed"] +# How an operator should treat an exit-0 run whose run-over-run drift is +# *alarming* (signal rot or a grade regression — :attr:`DriftReport.alarming`). +# The run-over-run analogue of :data:`OnFlagged` (issue #235, DEC-006); same +# three policies, same default ("fail"). +OnDrift = Literal["fail", "skip", "succeed"] + class TaskOutcome(StrEnum): """Neutral discriminator for what an Airflow task should do. @@ -107,12 +121,57 @@ def to_xcom(self) -> dict[str, object]: } +# Severity ordering for combining the flagged-outcome and the drift-outcome on +# an exit-0 run (DEC-006 — "most-severe wins"). Only the three exit-0-reachable +# outcomes appear: the 1/2/3 exit tiers short-circuit BEFORE either policy, so +# ``FAIL_RETRYABLE`` never enters the combine. +_OUTCOME_SEVERITY: dict[TaskOutcome, int] = { + TaskOutcome.SUCCESS: 1, + TaskOutcome.SKIP: 2, + TaskOutcome.FAIL_NO_RETRY: 3, +} + + +def _policy_outcome(triggered: bool, policy: OnFlagged | OnDrift) -> TaskOutcome: + """Map a (triggered?, policy) pair to an exit-0 :class:`TaskOutcome`. + + Shared by both the ``on_flagged`` and ``on_drift`` axes (their literals are + identical). ``triggered=False`` is always ``SUCCESS``; a triggered condition + maps ``"skip"`` → ``SKIP``, ``"succeed"`` → ``SUCCESS``, ``"fail"`` (the + default) → ``FAIL_NO_RETRY``. Always returns one of the three exit-0-reachable + outcomes, so its result is safe to look up in :data:`_OUTCOME_SEVERITY`. + """ + if not triggered: + return TaskOutcome.SUCCESS + if policy == "skip": + return TaskOutcome.SKIP + if policy == "succeed": + return TaskOutcome.SUCCESS + # policy == "fail" (the default) + return TaskOutcome.FAIL_NO_RETRY + + +def _most_severe(left: TaskOutcome, right: TaskOutcome) -> TaskOutcome: + """Return the more-severe of two exit-0 outcomes (DEC-006 most-severe wins). + + Severity rank ``FAIL_NO_RETRY (3) > SKIP (2) > SUCCESS (1)``. Both arguments + must be exit-0-reachable outcomes (the only ones :func:`_policy_outcome` + produces). + """ + return left if _OUTCOME_SEVERITY[left] >= _OUTCOME_SEVERITY[right] else right + + def decide_task_outcome( - result: SignalForgeRunResult, *, on_flagged: OnFlagged = "fail" + result: SignalForgeRunResult, + *, + on_flagged: OnFlagged = "fail", + on_drift: OnDrift = "fail", + drift: DriftReport | None = None, ) -> TaskOutcome: - """Map a run result + ``on_flagged`` policy to a :class:`TaskOutcome`. + """Map a run result + ``on_flagged`` / ``on_drift`` policies to a :class:`TaskOutcome`. - Pure — no I/O, no airflow. Decision table: + Pure — no I/O, no airflow. Decision table (exit-code tiers short-circuit + BEFORE either policy, so ``drift`` is consulted only on an exit-0 run): * exit 0, no flagged → ``SUCCESS`` * exit 0, flagged, ``on_flagged="fail"`` → ``FAIL_NO_RETRY`` @@ -127,23 +186,33 @@ def decide_task_outcome( Any unexpected ``exit_code`` (negative, or > 3) defaults conservatively to ``FAIL_NO_RETRY`` — an unknown failure should fail the task, not retry forever. + + **Drift (issue #235, DEC-006).** When ``drift is None`` the behaviour is + byte-identical to the pre-#235 table above (every #231/#232/#233 caller is + unchanged). When ``drift is not None and drift.alarming`` (signal rot or a + grade regression) on an exit-0 run, the ``on_drift`` policy is folded in and + the function returns the MOST-SEVERE of the flagged-outcome and the + drift-outcome (rank ``FAIL_NO_RETRY > SKIP > SUCCESS``). A non-alarming or + degraded ``DriftReport`` (``drift.alarming`` ``False``) never changes the + outcome. ``drift`` is ignored entirely on a non-exit-0 run. """ - if result.exit_code == 0: - if not result.below_threshold: - return TaskOutcome.SUCCESS - if on_flagged == "skip": - return TaskOutcome.SKIP - if on_flagged == "succeed": - return TaskOutcome.SUCCESS - # on_flagged == "fail" (the default) + if result.exit_code != 0: + if result.exit_code == 3: + return TaskOutcome.FAIL_RETRYABLE + # exit 1, exit 2, and any unexpected code (negative / > 3): fail, no retry. return TaskOutcome.FAIL_NO_RETRY - if result.exit_code == 3: - return TaskOutcome.FAIL_RETRYABLE - # exit 1, exit 2, and any unexpected code (negative / > 3): fail, no retry. - return TaskOutcome.FAIL_NO_RETRY + + flagged_outcome = _policy_outcome(result.below_threshold, on_flagged) + if drift is None or not drift.alarming: + # No drift supplied, or drift is non-alarming/degraded → flagged axis + # alone governs. Byte-identical to the pre-#235 exit-0 decision. + return flagged_outcome + drift_outcome = _policy_outcome(True, on_drift) + return _most_severe(flagged_outcome, drift_outcome) __all__ = [ + "OnDrift", "OnFlagged", "SignalForgeRunResult", "TaskOutcome", diff --git a/tests/airflow/test_drift_loaders.py b/tests/airflow/test_drift_loaders.py new file mode 100644 index 00000000..10870369 --- /dev/null +++ b/tests/airflow/test_drift_loaders.py @@ -0,0 +1,259 @@ +"""Tests for the fail-soft drift sidecar loaders (issue #235 / US-003). + +These tests import ONLY the airflow-free drift core — never the real +``apache-airflow`` package — so they run in the **default** pytest suite (NO +``airflow`` marker). They pin the FAIL-SOFT contract of +:func:`signalforge.airflow.drift.load_diff_report` / +:func:`~signalforge.airflow.drift.load_grade_report` / +:func:`~signalforge.airflow.drift.parse_diff_report`: + +* happy path returns the right type; +* absent / corrupt-JSON / wrong-shape / oversize input → ``None`` (never raise); +* a symlink cycle in the path → ``None`` (never raise) — DEC-007 / DEC-008. + +The prior-read path is operator-TRUSTED and may live OUTSIDE any project dir, so +the loaders are symlink-loop-hardened but NOT project-contained (DEC-008). +""" + +from __future__ import annotations + +import sys +from pathlib import Path + +import pytest + +import signalforge +from signalforge.airflow.drift import ( + _DRIFT_SIDECAR_SIZE_LIMIT_BYTES, + load_diff_report, + load_grade_report, + parse_diff_report, +) +from signalforge.diff.models import DiffEntry, DiffReport +from signalforge.grade.models import GradingReport, GradingResult + +_FIXTURE_DIR = Path(__file__).parent.parent / "fixtures" / "airflow" / "drift_pairs" + + +# --------------------------------------------------------------------------- +# Engineered builders — minimal valid models serialised to disk for the loaders. +# --------------------------------------------------------------------------- + + +def _diff_report() -> DiffReport: + """A minimal valid :class:`DiffReport` (one kept test).""" + return DiffReport( + signalforge_version=signalforge.__version__, + model_unique_id="model.shop.fct_orders", + run_id="r" * 32, + duration_seconds=1.0, + proposed_yaml="version: 2\n", + existing_yaml=None, + unified_diff="", + entries=(DiffEntry(artifact_id="test.column.amount.not_null", tier="kept"),), + kept_count=1, + kept_uncertain_count=0, + dropped_count=0, + flagged_count=0, + has_existing_schema=False, + candidate_hash="0" * 16, + prune_result_hash="0" * 16, + grading_report_hash=None, + ) + + +def _grade_report() -> GradingReport: + """A minimal valid :class:`GradingReport` (one scored result).""" + return GradingReport( + signalforge_version=signalforge.__version__, + run_id="g" * 32, + timestamp="2026-06-16T00:00:00.000000Z", # type: ignore[arg-type] + duration_seconds=2.0, + model_unique_id="model.shop.fct_orders", + rubric_hash="1" * 16, + thresholds=(0.7, 0.7), + results=( + GradingResult( + artifact_id="column.amount.description", + criterion_id="clarity", + score=0.9, + passed=True, + ), + ), + ) + + +# --------------------------------------------------------------------------- +# load_diff_report +# --------------------------------------------------------------------------- + + +def test_load_diff_report_happy_path(tmp_path: Path) -> None: + """A round-tripped DiffReport JSON loads back into an equal DiffReport.""" + report = _diff_report() + path = tmp_path / "diff.json" + path.write_text(report.model_dump_json(by_alias=True), encoding="utf-8") + + loaded = load_diff_report(path) + assert loaded is not None + assert isinstance(loaded, DiffReport) + assert loaded.model_unique_id == "model.shop.fct_orders" + assert loaded.model_dump_json() == report.model_dump_json() + + +def test_load_diff_report_from_committed_fixture() -> None: + """The committed signal-rot fixture loads as a DiffReport (str path form).""" + loaded = load_diff_report(str(_FIXTURE_DIR / "signal_rot_curr_diff.json")) + assert isinstance(loaded, DiffReport) + assert loaded.model_unique_id == "model.shop.fct_orders" + + +def test_load_diff_report_absent_returns_none(tmp_path: Path) -> None: + """A path that does not exist degrades to ``None`` (never raises).""" + assert load_diff_report(tmp_path / "nope.json") is None + + +def test_load_diff_report_corrupt_json_returns_none(tmp_path: Path) -> None: + """Non-JSON content degrades to ``None``.""" + path = tmp_path / "diff.json" + path.write_text("this is not json {", encoding="utf-8") + assert load_diff_report(path) is None + + +def test_load_diff_report_wrong_shape_returns_none(tmp_path: Path) -> None: + """Valid JSON of the WRONG shape (missing required fields) → ``None``.""" + path = tmp_path / "diff.json" + path.write_text('{"model_unique_id": "model.x.y"}', encoding="utf-8") + assert load_diff_report(path) is None + + +def test_load_diff_report_grade_json_is_wrong_shape(tmp_path: Path) -> None: + """A grade.json (missing DiffReport's required fields) → ``None``.""" + path = tmp_path / "grade.json" + path.write_text(_grade_report().model_dump_json(by_alias=True), encoding="utf-8") + assert load_diff_report(path) is None + + +def test_load_diff_report_oversize_returns_none(tmp_path: Path) -> None: + """A file over the size cap degrades to ``None`` BEFORE any JSON parse.""" + path = tmp_path / "diff.json" + # One byte over the cap — content need not be valid JSON; the cap fires first. + path.write_text("x" * (_DRIFT_SIDECAR_SIZE_LIMIT_BYTES + 1), encoding="utf-8") + assert load_diff_report(path) is None + + +def test_load_diff_report_at_size_cap_is_read(tmp_path: Path) -> None: + """A file EXACTLY at the cap is read (the cap is a strict ``>`` over-limit).""" + report = _diff_report() + body = report.model_dump_json(by_alias=True) + assert len(body.encode("utf-8")) <= _DRIFT_SIDECAR_SIZE_LIMIT_BYTES + path = tmp_path / "diff.json" + path.write_text(body, encoding="utf-8") + loaded = load_diff_report(path) + assert isinstance(loaded, DiffReport) + + +# --------------------------------------------------------------------------- +# load_grade_report +# --------------------------------------------------------------------------- + + +def test_load_grade_report_happy_path(tmp_path: Path) -> None: + """A round-tripped GradingReport JSON loads back into an equal report.""" + report = _grade_report() + path = tmp_path / "grade.json" + path.write_text(report.model_dump_json(by_alias=True), encoding="utf-8") + + loaded = load_grade_report(path) + assert isinstance(loaded, GradingReport) + assert loaded.model_unique_id == "model.shop.fct_orders" + assert loaded.mean_score == pytest.approx(0.9) + + +def test_load_grade_report_from_committed_fixture() -> None: + """The committed grade fixture loads as a GradingReport.""" + loaded = load_grade_report(_FIXTURE_DIR / "signal_rot_curr_grade.json") + assert isinstance(loaded, GradingReport) + + +def test_load_grade_report_absent_returns_none(tmp_path: Path) -> None: + """An absent grade sidecar (e.g. ``--no-grade``) degrades to ``None``.""" + assert load_grade_report(tmp_path / "nope.json") is None + + +def test_load_grade_report_corrupt_returns_none(tmp_path: Path) -> None: + """Malformed grade JSON degrades to ``None``.""" + path = tmp_path / "grade.json" + path.write_text("{ broken", encoding="utf-8") + assert load_grade_report(path) is None + + +def test_load_grade_report_diff_json_is_wrong_shape(tmp_path: Path) -> None: + """A diff.json (missing GradingReport's required fields) → ``None``.""" + path = tmp_path / "diff.json" + path.write_text(_diff_report().model_dump_json(by_alias=True), encoding="utf-8") + assert load_grade_report(path) is None + + +# --------------------------------------------------------------------------- +# Symlink-loop hardening (DEC-007 / DEC-008) — fail-soft, never raise. +# --------------------------------------------------------------------------- + + +@pytest.mark.skipif(sys.platform.startswith("win"), reason="POSIX symlink-loop semantics required") +def test_load_diff_report_symlink_loop_returns_none(tmp_path: Path) -> None: + """A symlink cycle in the path degrades to ``None`` rather than raising. + + Resolving ``strict=True`` surfaces the loop (``OSError(errno.ELOOP)`` on + Python >= 3.13, ``RuntimeError`` on <= 3.12); both are caught and degraded. + """ + link = tmp_path / "loop.json" + link.symlink_to(link) # self-referential symlink — a one-node cycle + assert load_diff_report(link) is None + + +@pytest.mark.skipif(sys.platform.startswith("win"), reason="POSIX symlink-loop semantics required") +def test_load_grade_report_symlink_loop_returns_none(tmp_path: Path) -> None: + """The grade loader degrades symmetrically on a symlink cycle.""" + a = tmp_path / "a.json" + b = tmp_path / "b.json" + a.symlink_to(b) + b.symlink_to(a) # two-node cycle + assert load_grade_report(a) is None + + +# --------------------------------------------------------------------------- +# parse_diff_report (current report off run_signalforge stdout) +# --------------------------------------------------------------------------- + + +def test_parse_diff_report_happy_path() -> None: + """Valid DiffReport JSON on stdout parses into a DiffReport.""" + report = _diff_report() + stdout = report.model_dump_json(by_alias=True) + parsed = parse_diff_report(stdout) + assert isinstance(parsed, DiffReport) + assert parsed.model_unique_id == "model.shop.fct_orders" + + +def test_parse_diff_report_strips_surrounding_whitespace() -> None: + """Leading/trailing whitespace around the JSON is tolerated (stripped).""" + report = _diff_report() + stdout = "\n\n" + report.model_dump_json(by_alias=True) + "\n " + assert isinstance(parse_diff_report(stdout), DiffReport) + + +@pytest.mark.parametrize("stdout", ["", " ", "\n\t "]) +def test_parse_diff_report_empty_returns_none(stdout: str) -> None: + """Empty / whitespace-only stdout (a non-zero exit produced no diff) → ``None``.""" + assert parse_diff_report(stdout) is None + + +def test_parse_diff_report_non_json_returns_none() -> None: + """Non-JSON stdout (a non-json ``--format``) degrades to ``None``.""" + assert parse_diff_report("Rendered an ANSI table here, not JSON.") is None + + +def test_parse_diff_report_wrong_shape_returns_none() -> None: + """Valid JSON of the wrong shape (missing required fields) → ``None``.""" + assert parse_diff_report('{"model_unique_id": "model.x.y"}') is None diff --git a/tests/airflow/test_result.py b/tests/airflow/test_result.py index 0c180d8d..6bffa1d8 100644 --- a/tests/airflow/test_result.py +++ b/tests/airflow/test_result.py @@ -23,11 +23,14 @@ import pytest from signalforge.airflow import ( + DriftArtifact, + DriftReport, OnFlagged, SignalForgeRunResult, TaskOutcome, decide_task_outcome, ) +from signalforge.airflow.result import OnDrift def _make_result(*, exit_code: int = 0, flagged: int = 0) -> SignalForgeRunResult: @@ -156,6 +159,141 @@ def test_to_xcom_round_trips_with_none_fields() -> None: assert json.loads(json.dumps(payload)) == payload +def _drift(*, alarming: bool, degraded: bool = False) -> DriftReport: + """Build a :class:`DriftReport` whose :attr:`alarming` property is as asked. + + * ``alarming=True`` → one ``newly_always_passes`` artifact (signal rot). + * ``alarming=False`` → all transition lists empty. + * ``degraded=True`` → a ``degrade_reason`` is set; the alarm lists stay empty + so ``alarming`` is ``False`` by construction (DEC-013). + """ + rot = ( + ( + DriftArtifact( + artifact_id="test.column.amount.not_null", + previous_tier="kept", + current_tier="dropped", + previous_drop_reason=None, + current_drop_reason="always-passes", + why="always passes on the sample", + ), + ) + if alarming + else () + ) + return DriftReport( + signalforge_version="0.7.0.dev0", + model_unique_id="model.shop.fct_orders", + as_of=None, + grade_regression_threshold=0.05, + previous_diff_hash="0" * 16, + current_diff_hash="1" * 16, + newly_always_passes=rot, + degrade_reason="model mismatch: prior=a current=b" if degraded else None, + ) + + +def test_drift_helper_alarming_property_truth_table() -> None: + """Sanity-pin the helper: it produces the alarming state each case asks for.""" + assert _drift(alarming=True).alarming is True + assert _drift(alarming=False).alarming is False + assert _drift(alarming=False, degraded=True).alarming is False + + +def test_decide_task_outcome_drift_none_is_byte_identical_to_pre_235() -> None: + """Passing ``drift=None`` reproduces EVERY pre-#235 row exactly. + + The ``on_drift`` value is irrelevant when ``drift is None`` — pinned by + varying it across all three policies and asserting the result still equals + the drift-free decision. + """ + cases: list[tuple[int, int, OnFlagged, TaskOutcome]] = [ + (0, 0, "fail", TaskOutcome.SUCCESS), + (0, 3, "fail", TaskOutcome.FAIL_NO_RETRY), + (0, 3, "skip", TaskOutcome.SKIP), + (0, 3, "succeed", TaskOutcome.SUCCESS), + (1, 0, "fail", TaskOutcome.FAIL_NO_RETRY), + (2, 0, "fail", TaskOutcome.FAIL_NO_RETRY), + (3, 0, "fail", TaskOutcome.FAIL_RETRYABLE), + ] + for exit_code, flagged, on_flagged, expected in cases: + result = _make_result(exit_code=exit_code, flagged=flagged) + for on_drift in ("fail", "skip", "succeed"): + got = decide_task_outcome(result, on_flagged=on_flagged, on_drift=on_drift, drift=None) + assert got == expected, (exit_code, flagged, on_flagged, on_drift) + # And explicitly equal to the drift-free single-arg form. + assert decide_task_outcome(result, on_flagged=on_flagged) == expected + + +@pytest.mark.parametrize( + ("flagged", "on_flagged", "alarming", "on_drift", "expected"), + [ + # --- drift drives (not flagged): on_drift maps straight through --- + (0, "fail", True, "fail", TaskOutcome.FAIL_NO_RETRY), + (0, "fail", True, "skip", TaskOutcome.SKIP), + (0, "fail", True, "succeed", TaskOutcome.SUCCESS), + # --- non-alarming drift never changes the flagged-only outcome --- + (0, "fail", False, "fail", TaskOutcome.SUCCESS), + (2, "fail", False, "fail", TaskOutcome.FAIL_NO_RETRY), + (2, "succeed", False, "fail", TaskOutcome.SUCCESS), + # --- both trip: MOST-SEVERE wins (FAIL_NO_RETRY > SKIP > SUCCESS) --- + (2, "fail", True, "succeed", TaskOutcome.FAIL_NO_RETRY), # flagged worse + (2, "succeed", True, "fail", TaskOutcome.FAIL_NO_RETRY), # drift worse + (2, "skip", True, "fail", TaskOutcome.FAIL_NO_RETRY), # drift worse + (2, "fail", True, "skip", TaskOutcome.FAIL_NO_RETRY), # flagged worse + (2, "skip", True, "succeed", TaskOutcome.SKIP), # skip > success + (2, "succeed", True, "skip", TaskOutcome.SKIP), # skip > success + (2, "succeed", True, "succeed", TaskOutcome.SUCCESS), # both benign + (2, "skip", True, "skip", TaskOutcome.SKIP), # tie + ], +) +def test_decide_task_outcome_flagged_x_drift_most_severe( + flagged: int, + on_flagged: OnFlagged, + alarming: bool, + on_drift: OnDrift, + expected: TaskOutcome, +) -> None: + """On an exit-0 run, the flagged-outcome and drift-outcome combine to the + MOST-SEVERE verdict (DEC-006).""" + result = _make_result(exit_code=0, flagged=flagged) + got = decide_task_outcome( + result, on_flagged=on_flagged, on_drift=on_drift, drift=_drift(alarming=alarming) + ) + assert got == expected + + +def test_decide_task_outcome_degraded_drift_never_trips() -> None: + """A degraded DriftReport (``alarming=False``) never changes the outcome, + even with ``on_drift="fail"`` — degrade, don't page (DEC-013).""" + result = _make_result(exit_code=0, flagged=0) + degraded = _drift(alarming=False, degraded=True) + assert decide_task_outcome(result, on_drift="fail", drift=degraded) == TaskOutcome.SUCCESS + + +@pytest.mark.parametrize( + ("exit_code", "expected"), + [ + (1, TaskOutcome.FAIL_NO_RETRY), + (2, TaskOutcome.FAIL_NO_RETRY), + (3, TaskOutcome.FAIL_RETRYABLE), + (99, TaskOutcome.FAIL_NO_RETRY), + ], +) +def test_decide_task_outcome_exit_tiers_ignore_alarming_drift( + exit_code: int, expected: TaskOutcome +) -> None: + """The 1/2/3 exit tiers short-circuit BEFORE the drift policy. + + The load-bearing case is exit 3 + alarming drift + ``on_drift="fail"``: it + stays ``FAIL_RETRYABLE`` (the retryable external-dependency verdict), NOT + downgraded to ``FAIL_NO_RETRY`` by the drift combine — drift is consulted + only on an exit-0 run (DEC-006).""" + result = _make_result(exit_code=exit_code, flagged=0) + got = decide_task_outcome(result, on_drift="fail", drift=_drift(alarming=True)) + assert got == expected + + def test_task_outcome_has_exactly_four_members() -> None: """Guard against silent fifth-tier creep — ``TaskOutcome`` is a SEPARATE axis from the CLI exit-code taxonomy and must stay four-valued.""" From 9512789d7951955263f16378178edb53f35e6170 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 13:04:56 -0700 Subject: [PATCH 07/13] bd_1-scaffolding-v42.4: detect_drift_against on SignalForgeGenerateOperator + persistence (#235 US-004) --- src/signalforge/airflow/operators.py | 308 +++++++++++++++++++++++- tests/airflow/test_operators_helpers.py | 209 ++++++++++++++++ 2 files changed, 508 insertions(+), 9 deletions(-) diff --git a/src/signalforge/airflow/operators.py b/src/signalforge/airflow/operators.py index f7d552a9..fafa61ba 100644 --- a/src/signalforge/airflow/operators.py +++ b/src/signalforge/airflow/operators.py @@ -44,12 +44,25 @@ import functools import importlib.util +import json +import logging from collections.abc import Sequence +from datetime import date +from pathlib import Path from typing import TYPE_CHECKING, Any, Literal +import signalforge from signalforge.airflow._airflow_compat import make_base_operator, raise_for_outcome +from signalforge.airflow.drift import ( + DriftReport, + compute_drift, + load_diff_report, + load_grade_report, + parse_diff_report, +) from signalforge.airflow.errors import AirflowConfigError from signalforge.airflow.result import ( + OnDrift, OnFlagged, SignalForgeRunResult, decide_task_outcome, @@ -57,6 +70,9 @@ from signalforge.airflow.runner import run_signalforge if TYPE_CHECKING: + from signalforge.diff.models import DiffReport + from signalforge.grade.models import GradingReport + # Type-checker-only declaration of the operator name. At runtime the class is # built by the find_spec-guarded factory below (it subclasses Apache # Airflow's ``BaseOperator``, which is NOT a typecheck dependency), and the @@ -74,8 +90,17 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: ... def execute(self, context: Any) -> Any: ... +_LOGGER = logging.getLogger(__name__) + _VALID_ON_FLAGGED: frozenset[str] = frozenset({"fail", "skip", "succeed"}) +# Conventional sidecar filenames under a drift-history directory (mirror the +# runner's ``/.signalforge/.json`` names). The generate operator +# persists THIS run's ``diff.json`` (+ ``grade.json`` sibling) here so a later +# run can pass ``detect_drift_against=/diff.json`` (#235 DEC-009). +_DIFF_SIDECAR_NAME = "diff.json" +_GRADE_SIDECAR_NAME = "grade.json" + def _build_generate_argv( *, @@ -141,8 +166,11 @@ def _validate_operator_config( model: str | None, select: str | None, on_flagged: str, + on_drift: str = "fail", + detect_drift_against: str | None = None, + drift_history_dir: str | None = None, ) -> None: - """Validate operator params, raising :class:`AirflowConfigError` (DEC-009). + """Validate operator params, raising :class:`AirflowConfigError` (DEC-009 / #235 DEC-012). Pure, airflow-free, no I/O. Runs BEFORE any ``run_signalforge`` call. Raises on: @@ -152,7 +180,12 @@ def _validate_operator_config( value must NOT satisfy the mutex — ``model=""`` is "unset", not "set"); * ``model`` and ``select`` both set OR both unset (mutex — exactly one); * a ``model`` / ``select`` value beginning with ``-`` (argv-injection guard); - * ``on_flagged`` outside ``{"fail", "skip", "succeed"}``. + * ``on_flagged`` / ``on_drift`` outside ``{"fail", "skip", "succeed"}``; + * a ``detect_drift_against`` / ``drift_history_dir`` path that is set (and + non-blank) but not a ``str`` or begins with ``-`` (argv-injection guard). + A ``None`` or blank value means "drift detection / persistence off" for + that param (the feature is opt-in, #235 DEC-001/009), so it is NOT an + error — mirrors the truthiness gate ``execute`` keys on. """ if not project_dir: raise AirflowConfigError("`project_dir` must be set (non-empty).") @@ -184,6 +217,29 @@ def _validate_operator_config( f"`on_flagged` must be one of {{fail, skip, succeed}} (got {on_flagged!r})." ) + if on_drift not in _VALID_ON_FLAGGED: + raise AirflowConfigError( + f"`on_drift` must be one of {{fail, skip, succeed}} (got {on_drift!r})." + ) + + # Drift paths are opt-in: ``None`` / blank means "off" (mirrors ``execute``'s + # truthiness gate), so only a set, non-blank value is validated. A set value + # must be a ``str`` and must not begin with ``-`` (argv-injection guard, + # mirrors the model/select guard above). + for label, value in ( + ("detect_drift_against", detect_drift_against), + ("drift_history_dir", drift_history_dir), + ): + if value is None or (isinstance(value, str) and not value.strip()): + continue + if not isinstance(value, str): + raise AirflowConfigError(f"`{label}` must be a string when set (got {value!r}).") + if value.startswith("-"): + raise AirflowConfigError( + f"`{label}` must not begin with '-' (got {value!r}); refusing as an " + "argv-injection guard." + ) + def _resolve_select_models(project_dir: str, select: str) -> tuple[str, ...]: """Resolve a ``--select`` expression to a sorted tuple of model unique_ids. @@ -299,6 +355,59 @@ def _aggregate_batch_result( ) +def build_drift_report( + *, + current_diff: DiffReport, + prior_diff: DiffReport | None, + current_grade: GradingReport | None, + prior_grade: GradingReport | None, + as_of: date | None, + grade_regression_threshold: float, +) -> DriftReport: + """Orchestrate run-over-run drift detection into a :class:`DriftReport` (pure). + + Airflow-free, no I/O — the decision-logic seam the (gated) generate-operator + ``execute`` drives, factored out so the codecov patch gate covers it (#235 + US-004). The loaders that read the prior sidecars off disk + (:func:`load_diff_report` / :func:`load_grade_report`) and the stdout parse + (:func:`parse_diff_report`) live in :mod:`signalforge.airflow.drift`; this + function takes the already-parsed objects. + + Two cases (DEC-013 — the comparison degrades, never raises): + + * ``prior_diff is None`` — there is NO prior run to compare against, so this + run establishes the **baseline**: a non-alarming :class:`DriftReport` with + ``baseline=True``, empty transition / regression lists, an empty schema + delta, and ``degrade_reason=None``. The two input hashes are left empty + (``""``) — a baseline performs NO comparison, so neither hash is + meaningful. :func:`compute_drift` is NOT called. + * ``prior_diff is not None`` — delegate to the pure + :func:`signalforge.airflow.drift.compute_drift`, which classifies the + tier transitions (incl. the signal-rot ``newly_always_passes`` alarm), + folds in any grade regression beyond ``grade_regression_threshold`` (only + when BOTH grades are present), and itself degrades (never raises) on a + ``model_unique_id`` mismatch. + """ + if prior_diff is None: + return DriftReport( + signalforge_version=signalforge.__version__, + model_unique_id=current_diff.model_unique_id, + as_of=as_of, + grade_regression_threshold=grade_regression_threshold, + baseline=True, + previous_diff_hash="", + current_diff_hash="", + ) + return compute_drift( + previous_diff=prior_diff, + current_diff=current_diff, + previous_grade=prior_grade, + current_grade=current_grade, + as_of=as_of, + grade_regression_threshold=grade_regression_threshold, + ) + + def _build_prune_existing_argv( *, model: str, @@ -499,8 +608,18 @@ class SignalForgeGenerateOperator(_Base): # type: ignore[valid-type, misc] """ # Airflow renders these fields from the task context before ``execute`` - # (DEC-005), so a DAG author can template e.g. ``as_of="{{ ds }}"``. - template_fields = ("project_dir", "select", "model", "profiles_dir", "as_of") + # (DEC-005), so a DAG author can template e.g. ``as_of="{{ ds }}"``, + # ``detect_drift_against="…/{{ prev_ds }}/diff.json"`` (#235 DEC-001), + # ``drift_history_dir="…/{{ ds }}"`` (#235 DEC-009). + template_fields = ( + "project_dir", + "select", + "model", + "profiles_dir", + "as_of", + "detect_drift_against", + "drift_history_dir", + ) def __init__( self, @@ -515,6 +634,10 @@ def __init__( cache_scope: str | None = None, as_of: str | None = None, on_flagged: OnFlagged = "fail", + detect_drift_against: str | None = None, + drift_history_dir: str | None = None, + on_drift: OnDrift = "fail", + grade_regression_threshold: float = 0.05, invocation: Literal["in_process", "subprocess"] = "in_process", **kwargs: Any, ) -> None: @@ -529,10 +652,21 @@ def __init__( self.no_grade = no_grade self.cache_scope = cache_scope self.as_of = as_of + # Run-over-run drift detection (#235 US-004), all opt-in: + # ``detect_drift_against`` is the prior run's diff.json path, + # ``drift_history_dir`` is where THIS run's diff.json is persisted + # for the next run, ``on_drift`` is the alarm policy, and + # ``grade_regression_threshold`` is the mean-grade drop that counts + # as a regression. When ``detect_drift_against`` is unset, behaviour + # is byte-identical to the pre-#235 generate operator. + self.detect_drift_against = detect_drift_against + self.drift_history_dir = drift_history_dir + self.grade_regression_threshold = grade_regression_threshold # 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). self.on_flagged: OnFlagged = on_flagged + self.on_drift: OnDrift = on_drift self.invocation: Literal["in_process", "subprocess"] = invocation # Fail fast at DAG-parse / instantiation time. The leading-dash # argv-injection guard is harmless here: an un-rendered Jinja @@ -544,18 +678,24 @@ def __init__( model=model, select=select, on_flagged=on_flagged, + on_drift=on_drift, + detect_drift_against=detect_drift_against, + drift_history_dir=drift_history_dir, ) def execute(self, context: Any) -> dict[str, object]: # Re-validate the now-rendered template_fields (DEC-005): the # leading-dash argv-injection guard is meaningful only once Jinja has - # rendered ``model`` / ``select`` / ``project_dir`` to their final - # values. + # rendered ``model`` / ``select`` / ``project_dir`` (and the drift + # paths) to their final values. _validate_operator_config( project_dir=self.project_dir, model=self.model, select=self.select, on_flagged=self.on_flagged, + on_drift=self.on_drift, + detect_drift_against=self.detect_drift_against, + drift_history_dir=self.drift_history_dir, ) if self.select is None: @@ -574,19 +714,169 @@ def _execute_single(self) -> dict[str, object]: as_of=self.as_of, ) result = run_signalforge(argv, project_dir=self.project_dir, invocation=self.invocation) - outcome = decide_task_outcome(result, on_flagged=self.on_flagged) + + # Run-over-run drift detection (#235 US-004). Opt-in via + # ``detect_drift_against``; degrades to ``None`` (no drift key, no + # outcome change) when the current diff JSON is unparseable, so the + # generate run's own exit_code / flagged verdict still governs. + drift: DriftReport | None = None + drift_xcom: dict[str, object] | None = None + if self.detect_drift_against: + drift = self._maybe_compute_and_persist_drift(result) + if drift is not None: + drift_xcom = drift.to_xcom() + + outcome = decide_task_outcome( + result, + on_flagged=self.on_flagged, + on_drift=self.on_drift, + drift=drift, + ) raise_for_outcome( outcome, message=( f"signalforge generate for {self.model!r} produced " f"task outcome {outcome.value} " - f"(exit_code={result.exit_code}, flagged={result.flagged})." + f"(exit_code={result.exit_code}, flagged={result.flagged}" + + (f", drift_alarming={drift.alarming}" if drift is not None else "") + + ")." ), ) - return result.to_xcom() + xcom = result.to_xcom() + if drift_xcom is not None: + xcom = {**xcom, "drift": drift_xcom} + return xcom + + def _maybe_compute_and_persist_drift( + self, result: SignalForgeRunResult + ) -> DriftReport | None: + """Compute drift for THIS run and persist it for the next (gated wiring). + + Returns the :class:`DriftReport` (baseline or comparison), or + ``None`` when the current diff JSON is unparseable (degrade — no + drift verdict, so it cannot alarm). Everything here is fail-soft on + the read side (DEC-013): a missing / corrupt prior sidecar yields a + baseline, a missing grade leaves ``grade_regressions`` empty. + """ + current_diff = parse_diff_report(result.stdout) + if current_diff is None: + _LOGGER.warning( + "signalforge drift: current diff JSON unparseable; skipping drift: %s", + json.dumps( + { + "model": self.model, + "detect_drift_against": self.detect_drift_against, + } + ), + ) + return None + + prior_diff = load_diff_report(self.detect_drift_against) # type: ignore[arg-type] + current_grade = ( + load_grade_report(result.grade_sidecar_path) if result.grade_sidecar_path else None + ) + prior_grade = self._load_prior_grade_sibling() + + as_of_date: date | None = None + if self.as_of: + try: + as_of_date = date.fromisoformat(self.as_of) + except ValueError: + as_of_date = None + + drift = build_drift_report( + current_diff=current_diff, + prior_diff=prior_diff, + current_grade=current_grade, + prior_grade=prior_grade, + as_of=as_of_date, + grade_regression_threshold=self.grade_regression_threshold, + ) + + if drift.baseline: + _LOGGER.info( + "signalforge drift: baseline established: %s", + json.dumps({"model": drift.model_unique_id}), + ) + else: + _LOGGER.info( + "signalforge drift: %s", + json.dumps( + { + "model": drift.model_unique_id, + "alarming": drift.alarming, + "degrade_reason": drift.degrade_reason, + "counts": drift.to_xcom()["counts"], + } + ), + ) + + self._persist_current_run(current_diff, result) + return drift + + def _load_prior_grade_sibling(self) -> GradingReport | None: + """Best-effort load of the ``grade.json`` next to ``detect_drift_against``. + + The prior grade sidecar conventionally sits beside the prior + diff.json (``/diff.json`` ↔ ``/grade.json``). Fail-soft — + an absent / corrupt sibling yields ``None`` (DEC-013): drift's tier + transitions are still computed, only the grade-regression axis is + skipped. + """ + prior_path = self.detect_drift_against + if not prior_path: + return None + sibling = Path(prior_path).parent / _GRADE_SIDECAR_NAME + return load_grade_report(sibling) + + def _persist_current_run( + self, current_diff: DiffReport, result: SignalForgeRunResult + ) -> None: + """Persist THIS run's diff.json (+ grade.json sibling) for the next comparison. + + Reuses the existing fail-closed + :func:`signalforge.diff._sidecar.write_sidecar` (#235 DEC-009/011 — + NO new writer); containment is anchored to the resolved + ``drift_history_dir`` (the directory the run is persisting under, not + the project dir). The grade sidecar — supplementary — is copied + best-effort. A no-op when ``drift_history_dir`` is unset. + """ + if not self.drift_history_dir: + return + import shutil + + from signalforge.diff._sidecar import write_sidecar + + history_dir = Path(self.drift_history_dir) + history_dir.mkdir(parents=True, exist_ok=True) + write_sidecar( + current_diff, + sidecar_path=history_dir / _DIFF_SIDECAR_NAME, + project_dir=history_dir, + ) + # Persist the grade sidecar if this run produced one, so the next + # run's prior-grade sibling load finds it. Supplementary → best + # effort; a copy failure must not fail the (already-succeeded) run. + if result.grade_sidecar_path: + try: + shutil.copyfile(result.grade_sidecar_path, history_dir / _GRADE_SIDECAR_NAME) + except OSError: + _LOGGER.warning( + "signalforge drift: failed to persist grade sidecar: %s", + json.dumps({"src": result.grade_sidecar_path}), + ) def _execute_batch(self) -> dict[str, object]: assert self.select is not None # narrowed by execute() + if self.detect_drift_against: + # Drift detection is single-model only in v0.7 (#235 DEC-001): + # a ``--select`` batch's stdout diff is the LAST model's render + # (runner DEC-002), so there is no faithful per-model current + # diff to compare. Skip drift for the batch and proceed. + _LOGGER.info( + "signalforge drift: detection skipped for --select batch: %s", + json.dumps({"select": self.select}), + ) 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 diff --git a/tests/airflow/test_operators_helpers.py b/tests/airflow/test_operators_helpers.py index f3767b00..02ee102b 100644 --- a/tests/airflow/test_operators_helpers.py +++ b/tests/airflow/test_operators_helpers.py @@ -23,6 +23,7 @@ from __future__ import annotations +from datetime import date from pathlib import Path import pytest @@ -36,8 +37,26 @@ _validate_operator_config, _validate_prune_existing_config, _without_sidecar_paths, + build_drift_report, ) from signalforge.airflow.result import SignalForgeRunResult +from signalforge.diff.models import DiffReport as SfDiffReport +from signalforge.grade.models import GradingReport + +# Engineered drift sidecar PAIR committed by #235 US-001: the +# ``test.column.amount.not_null`` artifact goes kept → dropped/always-passes +# between the prev and curr diff (signal rot), and the grade mean falls +# 0.9 → 0.8 (a regression beyond the 0.05 default threshold). +_DRIFT_PAIRS = Path(__file__).resolve().parents[1] / "fixtures" / "airflow" / "drift_pairs" + + +def _load_diff(name: str) -> SfDiffReport: + return SfDiffReport.model_validate_json((_DRIFT_PAIRS / f"{name}.json").read_text()) + + +def _load_grade(name: str) -> GradingReport: + return GradingReport.model_validate_json((_DRIFT_PAIRS / f"{name}.json").read_text()) + # A committed multi-model dbt fixture: tags `staging` (stg_a, stg_b) + `marts` # (fct_x). Resolves through the real `signalforge.manifest.load` + selector. @@ -647,3 +666,193 @@ 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 ) + + +# --------------------------------------------------------------------------- # +# _validate_operator_config — drift params (#235 US-004) # +# --------------------------------------------------------------------------- # + + +@pytest.mark.parametrize("on_drift", ["fail", "skip", "succeed"]) +def test_validate_accepts_each_valid_on_drift(on_drift: str) -> None: + _validate_operator_config( + project_dir="/proj", model="m.sql", select=None, on_flagged="fail", on_drift=on_drift + ) + + +def test_validate_rejects_bogus_on_drift() -> None: + with pytest.raises(AirflowConfigError, match="on_drift"): + _validate_operator_config( + project_dir="/proj", model="m.sql", select=None, on_flagged="fail", on_drift="bogus" + ) + + +def test_validate_accepts_set_drift_paths() -> None: + """A valid prior-diff path + history dir do not raise.""" + _validate_operator_config( + project_dir="/proj", + model="m.sql", + select=None, + on_flagged="fail", + detect_drift_against="/history/2026-06-14/diff.json", + drift_history_dir="/history/2026-06-15", + ) + + +def test_validate_accepts_templated_drift_paths() -> None: + """An un-rendered Jinja template (begins with '{') is not a leading-dash hit.""" + _validate_operator_config( + project_dir="/proj", + model="m.sql", + select=None, + on_flagged="fail", + detect_drift_against="/history/{{ prev_ds }}/diff.json", + drift_history_dir="/history/{{ ds }}", + ) + + +@pytest.mark.parametrize("blank", [None, "", " "]) +def test_validate_treats_blank_drift_paths_as_off(blank: str | None) -> None: + """``None`` / blank drift paths mean 'feature off' — not an error (opt-in).""" + _validate_operator_config( + project_dir="/proj", + model="m.sql", + select=None, + on_flagged="fail", + detect_drift_against=blank, + drift_history_dir=blank, + ) + + +def test_validate_rejects_leading_dash_detect_drift_against() -> None: + with pytest.raises(AirflowConfigError, match="argv-injection guard"): + _validate_operator_config( + project_dir="/proj", + model="m.sql", + select=None, + on_flagged="fail", + detect_drift_against="--evil", + ) + + +def test_validate_rejects_leading_dash_drift_history_dir() -> None: + with pytest.raises(AirflowConfigError, match="argv-injection guard"): + _validate_operator_config( + project_dir="/proj", + model="m.sql", + select=None, + on_flagged="fail", + drift_history_dir="-x", + ) + + +def test_validate_rejects_non_str_drift_path() -> None: + with pytest.raises(AirflowConfigError, match="must be a string"): + _validate_operator_config( + project_dir="/proj", + model="m.sql", + select=None, + on_flagged="fail", + detect_drift_against=123, # type: ignore[arg-type] + ) + + +def test_drift_params_do_not_leak_into_generate_argv() -> None: + """Drift is operator-internal: it never becomes a ``signalforge generate`` flag. + + Guards against a future change wiring drift into the argv builder — there is + no ``--detect-drift-against`` / ``--drift-history-dir`` / ``--on-drift`` CLI + flag, so none must appear in the generate argv. + """ + argv = _argv() + for tok in ("--detect-drift-against", "--drift-history-dir", "--on-drift"): + assert tok not in argv + + +# --------------------------------------------------------------------------- # +# build_drift_report (#235 US-004) — pure drift orchestration # +# --------------------------------------------------------------------------- # + + +def test_build_drift_report_baseline_when_no_prior() -> None: + """``prior_diff=None`` → a non-alarming baseline report; compute_drift NOT called.""" + curr = _load_diff("signal_rot_curr_diff") + report = build_drift_report( + current_diff=curr, + prior_diff=None, + current_grade=None, + prior_grade=None, + as_of=None, + grade_regression_threshold=0.05, + ) + assert report.baseline is True + assert report.alarming is False + assert report.model_unique_id == "model.shop.fct_orders" + assert report.newly_always_passes == () + assert report.newly_dropped == () + assert report.newly_kept == () + assert report.grade_regressions == () + assert report.degrade_reason is None + # No comparison performed → no prior hash; as_of threads through. + assert report.previous_diff_hash == "" + + +def test_build_drift_report_with_prior_delegates_to_compute_drift() -> None: + """``prior_diff`` set → delegates to compute_drift; signal rot ⇒ alarming.""" + prev = _load_diff("signal_rot_prev_diff") + curr = _load_diff("signal_rot_curr_diff") + report = build_drift_report( + current_diff=curr, + prior_diff=prev, + current_grade=None, + prior_grade=None, + as_of=date(2026, 6, 15), + grade_regression_threshold=0.05, + ) + assert report.baseline is False + assert report.alarming is True + assert len(report.newly_always_passes) == 1 + assert report.newly_always_passes[0].artifact_id == "test.column.amount.not_null" + assert report.as_of == date(2026, 6, 15) + # Real comparison → real input hashes (not the baseline empty-string sentinel). + assert report.previous_diff_hash != "" + assert report.current_diff_hash != "" + + +def test_build_drift_report_threads_grades_to_compute_drift() -> None: + """Both grades present → grade-regression axis reaches compute_drift. + + The fixture grades fall 0.9 → 0.8 (delta 0.1 ≥ the 0.05 threshold), so a + regression is emitted — proving the helper passes the grades through rather + than dropping them. + """ + prev = _load_diff("signal_rot_prev_diff") + curr = _load_diff("signal_rot_curr_diff") + report = build_drift_report( + current_diff=curr, + prior_diff=prev, + current_grade=_load_grade("signal_rot_curr_grade"), + prior_grade=_load_grade("signal_rot_prev_grade"), + as_of=None, + grade_regression_threshold=0.05, + ) + assert len(report.grade_regressions) == 1 + regression = report.grade_regressions[0] + assert regression.previous_mean == pytest.approx(0.9) + assert regression.current_mean == pytest.approx(0.8) + assert report.alarming is True + + +def test_build_drift_report_no_grades_leaves_regressions_empty() -> None: + """A missing grade (``--no-grade``) leaves ``grade_regressions`` empty (degrade).""" + prev = _load_diff("signal_rot_prev_diff") + curr = _load_diff("signal_rot_curr_diff") + report = build_drift_report( + current_diff=curr, + prior_diff=prev, + current_grade=None, + prior_grade=None, + as_of=None, + grade_regression_threshold=0.05, + ) + assert report.grade_regressions == () From 4a4646c56d93b9a65c1e23574a5438486bfcb7db Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 13:15:09 -0700 Subject: [PATCH 08/13] bd_1-scaffolding-v42.5: dedicated SignalForgeDriftOperator (deferred class + skeleton) (#235 US-005) --- src/signalforge/airflow/__init__.py | 3 + src/signalforge/airflow/operators.py | 345 +++++++++++++++++- tests/airflow/test_airflow_no_eager_import.py | 4 + tests/airflow/test_operators_helpers.py | 200 +++++++++- tests/airflow/test_skeleton.py | 37 ++ 5 files changed, 585 insertions(+), 4 deletions(-) diff --git a/src/signalforge/airflow/__init__.py b/src/signalforge/airflow/__init__.py index 34553918..62bc2326 100644 --- a/src/signalforge/airflow/__init__.py +++ b/src/signalforge/airflow/__init__.py @@ -70,6 +70,7 @@ # gate). The modules themselves are Airflow-free per DEC-004. from signalforge.airflow.hooks import SignalForgeHook from signalforge.airflow.operators import ( + SignalForgeDriftOperator, SignalForgeGenerateOperator, SignalForgePruneExistingOperator, ) @@ -80,6 +81,7 @@ _LAZY_NAMES: dict[str, str] = { "SignalForgeGenerateOperator": "operators", "SignalForgePruneExistingOperator": "operators", + "SignalForgeDriftOperator": "operators", "SignalForgeHook": "hooks", } @@ -91,6 +93,7 @@ "GradeRegression", "OnFlagged", "SchemaShapeDelta", + "SignalForgeDriftOperator", "SignalForgeGenerateOperator", "SignalForgeHook", "SignalForgePruneExistingOperator", diff --git a/src/signalforge/airflow/operators.py b/src/signalforge/airflow/operators.py index fafa61ba..001f9c38 100644 --- a/src/signalforge/airflow/operators.py +++ b/src/signalforge/airflow/operators.py @@ -1,6 +1,6 @@ -"""The SignalForge Apache Airflow operators (#232 US-003, #233 US-002). +"""The SignalForge Apache Airflow operators (#232 US-003, #233 US-002, #235 US-005). -This module ships two operators a DAG author wires as one Airflow task each: +This module ships three operators a DAG author wires as one Airflow task each: * :class:`SignalForgeGenerateOperator` — runs ``signalforge generate`` (single model or a ``--select`` batch). The four pure helpers below @@ -11,6 +11,15 @@ (ingest -> prune -> diff, **no LLM call**, read-only; #233). Its pure helpers are :func:`_build_prune_existing_argv` + :func:`_validate_prune_existing_config`. Single-model only — no batch apparatus (#233 DEC-003). +* :class:`SignalForgeDriftOperator` — run-over-run drift / signal-rot detection + (#235 DEC-010). Runs NO ``signalforge`` CLI invocation: it reads a prior + a + current ``diff.json`` sidecar (plus optional ``grade.json`` siblings) off disk, + computes a :class:`~signalforge.airflow.drift.DriftReport`, and maps the + ``on_drift`` policy + the drift verdict to a task outcome. Its pure helpers are + :func:`_validate_drift_config` + :func:`_build_drift_inputs` + + :func:`_drift_task_outcome`. Single-model only — drift is single-model in v0.7 + (#235 DEC-001). Reuses the existing :func:`build_drift_report` orchestration + seam (no new error class — :class:`AirflowConfigError`; #235 DEC-012). **Deferred class construction (the load-bearing structural constraint).** The real operator must subclass Apache Airflow's ``BaseOperator``, which requires @@ -65,6 +74,7 @@ OnDrift, OnFlagged, SignalForgeRunResult, + TaskOutcome, decide_task_outcome, ) from signalforge.airflow.runner import run_signalforge @@ -89,6 +99,11 @@ def __init__(self, *args: Any, **kwargs: Any) -> None: ... def execute(self, context: Any) -> Any: ... + class SignalForgeDriftOperator: # noqa: D401 - type stub only + def __init__(self, *args: Any, **kwargs: Any) -> None: ... + + def execute(self, context: Any) -> Any: ... + _LOGGER = logging.getLogger(__name__) @@ -523,6 +538,108 @@ def _validate_prune_existing_config( ) +def _validate_drift_config( + *, + previous_diff_path: str | None, + current_diff_path: str | None, + on_drift: str, +) -> None: + """Validate dedicated-drift-operator params (raise :class:`AirflowConfigError`; #235 DEC-010). + + Pure, airflow-free, no I/O. Runs BEFORE any sidecar read. The dedicated + :class:`SignalForgeDriftOperator` runs no ``signalforge`` CLI invocation — it + reads the prior + current ``diff.json`` sidecars directly — so BOTH diff + paths are REQUIRED operator params. Raises on: + + * empty / ``None`` (or non-``str`` / blank) ``previous_diff_path`` — required + (a missing prior *file* at this path degrades to a baseline at read time, + DEC-013; but the path itself must be configured); + * empty / ``None`` (or non-``str`` / blank) ``current_diff_path`` — required; + * a ``previous_diff_path`` / ``current_diff_path`` value beginning with ``-`` + (defensive path-shape guard, mirrors the sibling operators); + * ``on_drift`` outside ``{"fail", "skip", "succeed"}``. + + The optional ``previous_grade_path`` / ``current_grade_path`` are NOT + validated here: they feed the fail-soft :func:`load_grade_report` (an absent / + malformed grade simply leaves the grade-regression axis empty), so a bad + value degrades rather than fails (DEC-013). + """ + for label, value in ( + ("previous_diff_path", previous_diff_path), + ("current_diff_path", current_diff_path), + ): + if not isinstance(value, str) or not value.strip(): + raise AirflowConfigError(f"`{label}` must be a non-empty string (got {value!r}).") + if value.startswith("-"): + raise AirflowConfigError( + f"`{label}` must not begin with '-' (got {value!r}); refusing as a " + "defensive path-shape guard." + ) + + if on_drift not in _VALID_ON_FLAGGED: + raise AirflowConfigError( + f"`on_drift` must be one of {{fail, skip, succeed}} (got {on_drift!r})." + ) + + +def _build_drift_inputs( + *, + previous_diff_path: str, + current_diff_path: str, + previous_grade_path: str | None, + current_grade_path: str | None, +) -> tuple[str, str, str, str]: + """Resolve the four sidecar read paths, auto-siblinging the grades (pure, #235 DEC-010). + + Airflow-free, no I/O — pure path arithmetic. A grade sidecar conventionally + sits beside its diff (``/diff.json`` ↔ ``/grade.json``); when a + grade path is not given explicitly it is resolved to the ``grade.json`` + sibling of the corresponding diff path. Returns + ``(previous_diff_path, current_diff_path, previous_grade_path, + current_grade_path)`` with the grade entries resolved. The diff paths pass + through unchanged. + """ + resolved_previous_grade = previous_grade_path or str( + Path(previous_diff_path).parent / _GRADE_SIDECAR_NAME + ) + resolved_current_grade = current_grade_path or str( + Path(current_diff_path).parent / _GRADE_SIDECAR_NAME + ) + return ( + previous_diff_path, + current_diff_path, + resolved_previous_grade, + resolved_current_grade, + ) + + +def _drift_task_outcome(drift: DriftReport, on_drift: OnDrift) -> TaskOutcome: + """Map a ``(drift, on_drift)`` pair to a :class:`TaskOutcome` (pure, #235 DEC-010). + + The dedicated :class:`SignalForgeDriftOperator` runs no ``signalforge`` CLI + invocation, so it has NO :class:`SignalForgeRunResult` to feed + :func:`~signalforge.airflow.result.decide_task_outcome`; its single Airflow + task state is driven directly off the drift verdict. A non-alarming (or + degraded) report → ``SUCCESS``; an alarming report (signal rot or a grade + regression — :attr:`DriftReport.alarming`) maps ``on_drift`` → ``"skip"`` + :attr:`~TaskOutcome.SKIP` / ``"succeed"`` :attr:`~TaskOutcome.SUCCESS` / + ``"fail"`` (the default) :attr:`~TaskOutcome.FAIL_NO_RETRY`. Mirrors the + exit-0 half of :func:`decide_task_outcome`'s ``on_flagged`` axis. Airflow-free, + no I/O — the airflow raise stays confined to + :func:`~signalforge.airflow._airflow_compat.raise_for_outcome`. + """ + if not drift.alarming: + return TaskOutcome.SUCCESS + if on_drift == "skip": + return TaskOutcome.SKIP + if on_drift == "succeed": + return TaskOutcome.SUCCESS + # on_drift == "fail" (the default): an alarming drift is a hard failure that + # bypasses the task's retry policy (signal rot is deterministic — retrying + # cannot un-rot it; it is a reviewer signal, not a transient). + return TaskOutcome.FAIL_NO_RETRY + + class _GenerateOperatorAirflowMissing: """Stand-in for :class:`SignalForgeGenerateOperator` when Airflow is absent. @@ -562,6 +679,27 @@ def __init__(self, *_args: Any, **_kwargs: Any) -> None: ) +class _DriftOperatorAirflowMissing: + """Stand-in for :class:`SignalForgeDriftOperator` when Airflow is absent. + + Mirrors :class:`_GenerateOperatorAirflowMissing` / + :class:`_PruneExistingOperatorAirflowMissing` exactly. Resolving + ``signalforge.airflow.SignalForgeDriftOperator`` 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 operator subclasses ``BaseOperator`` and so genuinely + requires Airflow at construction time. + """ + + def __init__(self, *_args: Any, **_kwargs: Any) -> None: + raise ModuleNotFoundError( + "SignalForgeDriftOperator requires Apache Airflow, which is not " + "installed. Install the optional extra: " + "pip install 'signalforge-dbt[airflow]'." + ) + + # Cache for the resolved operator class — the real ``BaseOperator`` subclass when # Airflow is installed, else the airflow-free placeholder above. Built once on # first attribute access and reused (a process either has Airflow or it does not, @@ -1108,6 +1246,201 @@ def _get_prune_existing_operator_class() -> type: return _make_prune_existing_operator_class() # pragma: no cover - requires [airflow] +def _make_drift_operator_class() -> type: # pragma: no cover - requires [airflow] + """Build and return the real ``BaseOperator``-subclassing drift operator (#235 DEC-010). + + Sibling of :func:`_make_generate_operator_class` / + :func:`_make_prune_existing_operator_class`. Reached only when Apache Airflow + is installed (guarded by :func:`_get_drift_operator_class`'s ``find_spec`` + check). The base class comes from the one shim + :func:`signalforge.airflow._airflow_compat.make_base_operator`, so the + ``from airflow ...`` import stays confined there and out of this module's + scope. Marked ``# pragma: no cover`` because its body — the ``BaseOperator`` + subclass and its ``execute`` — runs only under the gated ``[airflow]`` extra + (the gated ``tests/airflow/test_operators.py`` exercise it, #235 US-006); the + default coverage env never installs Airflow. + """ + _Base = make_base_operator() + + class SignalForgeDriftOperator(_Base): # type: ignore[valid-type, misc] + """Run run-over-run drift / signal-rot detection as one Apache Airflow task (#235). + + Runs NO ``signalforge`` CLI invocation: it reads a prior + a current + ``diff.json`` sidecar (plus optional ``grade.json`` siblings, auto-resolved + next to each diff when not given) off disk, computes a + :class:`~signalforge.airflow.drift.DriftReport` via the pure + :func:`build_drift_report`, and drives the single task state off the drift + verdict — :func:`_drift_task_outcome` maps the ``on_drift`` policy + + :attr:`DriftReport.alarming` to a + :class:`~signalforge.airflow.result.TaskOutcome`, which + :func:`~signalforge.airflow._airflow_compat.raise_for_outcome` translates + into the matching Airflow signal. Returns the drift XCom payload + (:meth:`DriftReport.to_xcom` — per-category counts + the transition + lists + the two input hashes; no bulk text, no secrets; #235 DEC-015). + + Wired downstream of a :class:`SignalForgeGenerateOperator` configured to + persist its sidecars (its ``drift_history_dir``); this operator points + ``previous_diff_path`` / ``current_diff_path`` at two such persisted + sidecars. Single-model only — drift is single-model in v0.7 (#235 + DEC-001). The comparison degrades, never raises (DEC-013): a missing + prior ``diff.json`` makes this run a baseline; a ``model_unique_id`` + mismatch yields a degraded (non-alarming) report. + """ + + # Airflow renders these fields from the task context before ``execute`` + # (#235 DEC-010), so a DAG author can template e.g. + # ``previous_diff_path="…/{{ prev_ds }}/diff.json"``, + # ``current_diff_path="…/{{ ds }}/diff.json"``, ``as_of="{{ ds }}"``. + template_fields = ( + "previous_diff_path", + "current_diff_path", + "previous_grade_path", + "current_grade_path", + "as_of", + ) + + def __init__( + self, + *, + task_id: str, + previous_diff_path: str, + current_diff_path: str, + previous_grade_path: str | None = None, + current_grade_path: str | None = None, + on_drift: OnDrift = "fail", + as_of: str | None = None, + grade_regression_threshold: float = 0.05, + **kwargs: Any, + ) -> None: + # BaseOperator owns ``task_id`` + the standard Airflow kwargs + # (``retries`` / ``retry_delay`` / ``depends_on_past`` / ...). + super().__init__(task_id=task_id, **kwargs) + self.previous_diff_path = previous_diff_path + self.current_diff_path = current_diff_path + self.previous_grade_path = previous_grade_path + self.current_grade_path = current_grade_path + self.as_of = as_of + self.grade_regression_threshold = grade_regression_threshold + # Annotate the Literal-typed attr explicitly so pyright does NOT widen + # it to ``str`` on assignment (which would break the typed + # ``_drift_task_outcome`` call below). + self.on_drift: OnDrift = on_drift + # Fail fast at DAG-parse / instantiation time. The leading-dash guard + # is harmless here: an un-rendered Jinja template (e.g. ``"{{ ds }}"``) + # never begins with ``-``, so it cannot false-trip. ``execute`` + # re-validates the RENDERED values before any read. + _validate_drift_config( + previous_diff_path=previous_diff_path, + current_diff_path=current_diff_path, + on_drift=on_drift, + ) + + def execute(self, context: Any) -> dict[str, object]: + # Re-validate the now-rendered template_fields (#235 DEC-010): the + # leading-dash guard is meaningful only once Jinja has rendered the + # paths to their final values. + _validate_drift_config( + previous_diff_path=self.previous_diff_path, + current_diff_path=self.current_diff_path, + on_drift=self.on_drift, + ) + ( + previous_diff_path, + current_diff_path, + previous_grade_path, + current_grade_path, + ) = _build_drift_inputs( + previous_diff_path=self.previous_diff_path, + current_diff_path=self.current_diff_path, + previous_grade_path=self.previous_grade_path, + current_grade_path=self.current_grade_path, + ) + + # The CURRENT diff is required: this operator reads it from a path + # (not stdout — it runs no CLI). A None load means the path is wrong / + # the file is absent or malformed → a hard config error (NOT a + # baseline; baseline is the PRIOR-absent case below). + current_diff = load_diff_report(current_diff_path) + if current_diff is None: + raise AirflowConfigError( + f"The current diff sidecar at {current_diff_path!r} could not " + "be read as a DiffReport (absent, unreadable, oversize, or " + "malformed). The dedicated drift operator runs downstream of a " + "generate task configured to persist its diff.json sidecar; " + "point `current_diff_path` at that persisted file." + ) + + # The PRIOR diff is fail-soft (DEC-013): None → this run is a baseline. + prior_diff = load_diff_report(previous_diff_path) + current_grade = load_grade_report(current_grade_path) + prior_grade = load_grade_report(previous_grade_path) + + as_of_date: date | None = None + if self.as_of: + try: + as_of_date = date.fromisoformat(self.as_of) + except ValueError: + as_of_date = None + + drift = build_drift_report( + current_diff=current_diff, + prior_diff=prior_diff, + current_grade=current_grade, + prior_grade=prior_grade, + as_of=as_of_date, + grade_regression_threshold=self.grade_regression_threshold, + ) + + if drift.baseline: + _LOGGER.info( + "signalforge drift: baseline established: %s", + json.dumps({"model": drift.model_unique_id}), + ) + else: + _LOGGER.info( + "signalforge drift: %s", + json.dumps( + { + "model": drift.model_unique_id, + "alarming": drift.alarming, + "degrade_reason": drift.degrade_reason, + "counts": drift.to_xcom()["counts"], + } + ), + ) + + outcome = _drift_task_outcome(drift, self.on_drift) + raise_for_outcome( + outcome, + message=( + f"signalforge drift for {drift.model_unique_id!r} produced " + f"task outcome {outcome.value} (alarming={drift.alarming}, " + f"baseline={drift.baseline}, " + f"degrade_reason={drift.degrade_reason!r})." + ), + ) + return drift.to_xcom() + + return SignalForgeDriftOperator + + +@functools.cache +def _get_drift_operator_class() -> type: + """Resolve (and cache) the drift operator class without importing airflow eagerly. + + Sibling of :func:`_get_generate_operator_class` / + :func:`_get_prune_existing_operator_class` (#235 DEC-010). 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. When Airflow is present, build + the real ``BaseOperator`` subclass; when absent, return the airflow-free + placeholder whose construction raises :class:`ModuleNotFoundError`. + """ + if importlib.util.find_spec("airflow") is None: + return _DriftOperatorAirflowMissing + return _make_drift_operator_class() # pragma: no cover - requires [airflow] + + def __getattr__(name: str) -> object: """PEP 562 lazy resolution of the public operator names. @@ -1120,7 +1453,13 @@ def __getattr__(name: str) -> object: return _get_generate_operator_class() if name == "SignalForgePruneExistingOperator": return _get_prune_existing_operator_class() + if name == "SignalForgeDriftOperator": + return _get_drift_operator_class() raise AttributeError(f"module {__name__!r} has no attribute {name!r}") -__all__ = ["SignalForgeGenerateOperator", "SignalForgePruneExistingOperator"] +__all__ = [ + "SignalForgeDriftOperator", + "SignalForgeGenerateOperator", + "SignalForgePruneExistingOperator", +] diff --git a/tests/airflow/test_airflow_no_eager_import.py b/tests/airflow/test_airflow_no_eager_import.py index db92cf30..d24d410f 100644 --- a/tests/airflow/test_airflow_no_eager_import.py +++ b/tests/airflow/test_airflow_no_eager_import.py @@ -39,6 +39,7 @@ # Resolving the lazy public names imports the airflow-free stub modules only. from signalforge.airflow import ( + SignalForgeDriftOperator, SignalForgeGenerateOperator, SignalForgeHook, SignalForgePruneExistingOperator, @@ -47,6 +48,7 @@ assert SignalForgeGenerateOperator is not None assert SignalForgeHook is not None assert SignalForgePruneExistingOperator is not None +assert SignalForgeDriftOperator is not None leaked = sorted( name for name in sys.modules if name == "airflow" or name.startswith("airflow.") @@ -93,6 +95,7 @@ def test_resolving_lazy_names_does_not_import_airflow_in_process() -> None: del sys.modules[name] from signalforge.airflow import ( + SignalForgeDriftOperator, SignalForgeGenerateOperator, SignalForgeHook, SignalForgePruneExistingOperator, @@ -101,6 +104,7 @@ def test_resolving_lazy_names_does_not_import_airflow_in_process() -> None: assert SignalForgeGenerateOperator is not None assert SignalForgeHook is not None assert SignalForgePruneExistingOperator is not None + assert SignalForgeDriftOperator is not None assert not _airflow_modules_in_sys_modules(), ( "resolving the lazy operator/hook names must not import airflow — the " "stubs deliberately do not subclass BaseOperator/BaseHook at module scope" diff --git a/tests/airflow/test_operators_helpers.py b/tests/airflow/test_operators_helpers.py index 02ee102b..d44e1785 100644 --- a/tests/airflow/test_operators_helpers.py +++ b/tests/airflow/test_operators_helpers.py @@ -28,18 +28,22 @@ import pytest +from signalforge.airflow.drift import DriftArtifact, DriftReport from signalforge.airflow.errors import AirflowConfigError from signalforge.airflow.operators import ( _aggregate_batch_result, + _build_drift_inputs, _build_generate_argv, _build_prune_existing_argv, + _drift_task_outcome, _resolve_select_models, + _validate_drift_config, _validate_operator_config, _validate_prune_existing_config, _without_sidecar_paths, build_drift_report, ) -from signalforge.airflow.result import SignalForgeRunResult +from signalforge.airflow.result import SignalForgeRunResult, TaskOutcome from signalforge.diff.models import DiffReport as SfDiffReport from signalforge.grade.models import GradingReport @@ -856,3 +860,197 @@ def test_build_drift_report_no_grades_leaves_regressions_empty() -> None: grade_regression_threshold=0.05, ) assert report.grade_regressions == () + + +# --------------------------------------------------------------------------- # +# _validate_drift_config (#235 US-005) # +# --------------------------------------------------------------------------- # + + +def test_validate_drift_accepts_valid_config() -> None: + """A valid dedicated-drift config does not raise.""" + _validate_drift_config( + previous_diff_path="/history/2026-06-14/diff.json", + current_diff_path="/history/2026-06-15/diff.json", + on_drift="fail", + ) + + +def test_validate_drift_accepts_templated_paths() -> None: + """An un-rendered Jinja template (begins with '{') is not a leading-dash hit.""" + _validate_drift_config( + previous_diff_path="/history/{{ prev_ds }}/diff.json", + current_diff_path="/history/{{ ds }}/diff.json", + on_drift="skip", + ) + + +@pytest.mark.parametrize("blank", [None, "", " ", "\t"]) +def test_validate_drift_rejects_blank_previous_diff_path(blank: str | None) -> None: + with pytest.raises(AirflowConfigError, match="non-empty string"): + _validate_drift_config( + previous_diff_path=blank, + current_diff_path="/c/diff.json", + on_drift="fail", + ) + + +@pytest.mark.parametrize("blank", [None, "", " "]) +def test_validate_drift_rejects_blank_current_diff_path(blank: str | None) -> None: + with pytest.raises(AirflowConfigError, match="non-empty string"): + _validate_drift_config( + previous_diff_path="/p/diff.json", + current_diff_path=blank, + on_drift="fail", + ) + + +def test_validate_drift_rejects_non_str_path() -> None: + with pytest.raises(AirflowConfigError, match="non-empty string"): + _validate_drift_config( + previous_diff_path=123, # type: ignore[arg-type] + current_diff_path="/c/diff.json", + on_drift="fail", + ) + + +def test_validate_drift_rejects_leading_dash_previous() -> None: + with pytest.raises(AirflowConfigError, match="must not begin with"): + _validate_drift_config( + previous_diff_path="-evil", + current_diff_path="/c/diff.json", + on_drift="fail", + ) + + +def test_validate_drift_rejects_leading_dash_current() -> None: + with pytest.raises(AirflowConfigError, match="must not begin with"): + _validate_drift_config( + previous_diff_path="/p/diff.json", + current_diff_path="-evil", + on_drift="fail", + ) + + +def test_validate_drift_rejects_bogus_on_drift() -> None: + with pytest.raises(AirflowConfigError, match="on_drift"): + _validate_drift_config( + previous_diff_path="/p/diff.json", + current_diff_path="/c/diff.json", + on_drift="bogus", + ) + + +@pytest.mark.parametrize("on_drift", ["fail", "skip", "succeed"]) +def test_validate_drift_accepts_each_valid_on_drift(on_drift: str) -> None: + _validate_drift_config( + previous_diff_path="/p/diff.json", + current_diff_path="/c/diff.json", + on_drift=on_drift, + ) + + +# --------------------------------------------------------------------------- # +# _build_drift_inputs (#235 US-005) # +# --------------------------------------------------------------------------- # + + +def test_build_drift_inputs_auto_siblings_grades_when_none() -> None: + """Unset grade paths resolve to the ``grade.json`` sibling of each diff path.""" + prev_diff, curr_diff, prev_grade, curr_grade = _build_drift_inputs( + previous_diff_path="/history/2026-06-14/diff.json", + current_diff_path="/history/2026-06-15/diff.json", + previous_grade_path=None, + current_grade_path=None, + ) + # Diff paths pass through unchanged. + assert prev_diff == "/history/2026-06-14/diff.json" + assert curr_diff == "/history/2026-06-15/diff.json" + # Grades auto-sibling next to each diff. + assert prev_grade == str(Path("/history/2026-06-14/grade.json")) + assert curr_grade == str(Path("/history/2026-06-15/grade.json")) + + +def test_build_drift_inputs_uses_explicit_grade_paths_when_set() -> None: + """Explicit grade paths win over the auto-sibling resolution.""" + _, _, prev_grade, curr_grade = _build_drift_inputs( + previous_diff_path="/history/prev/diff.json", + current_diff_path="/history/curr/diff.json", + previous_grade_path="/custom/prev_grade.json", + current_grade_path="/custom/curr_grade.json", + ) + assert prev_grade == "/custom/prev_grade.json" + assert curr_grade == "/custom/curr_grade.json" + + +def test_build_drift_inputs_mixes_explicit_and_auto() -> None: + """A set previous grade + unset current grade resolve independently.""" + _, _, prev_grade, curr_grade = _build_drift_inputs( + previous_diff_path="/history/prev/diff.json", + current_diff_path="/history/curr/diff.json", + previous_grade_path="/custom/prev_grade.json", + current_grade_path=None, + ) + assert prev_grade == "/custom/prev_grade.json" + assert curr_grade == str(Path("/history/curr/grade.json")) + + +# --------------------------------------------------------------------------- # +# _drift_task_outcome (#235 US-005) # +# --------------------------------------------------------------------------- # + + +def _non_alarming_drift() -> DriftReport: + """A baseline (no comparison) → non-alarming DriftReport.""" + return DriftReport( + signalforge_version="0", + model_unique_id="model.shop.fct_orders", + as_of=None, + grade_regression_threshold=0.05, + baseline=True, + previous_diff_hash="", + current_diff_hash="", + ) + + +def _alarming_drift() -> DriftReport: + """A DriftReport carrying a signal-rot transition → alarming.""" + return DriftReport( + signalforge_version="0", + model_unique_id="model.shop.fct_orders", + as_of=None, + grade_regression_threshold=0.05, + previous_diff_hash="aa", + current_diff_hash="bb", + newly_always_passes=( + DriftArtifact( + artifact_id="test.column.amount.not_null", + previous_tier="kept", + current_tier="dropped", + previous_drop_reason=None, + current_drop_reason="always-passes", + ), + ), + ) + + +def test_drift_outcome_non_alarming_is_success_regardless_of_policy() -> None: + """A non-alarming report → SUCCESS for every ``on_drift`` policy.""" + drift = _non_alarming_drift() + assert drift.alarming is False + assert _drift_task_outcome(drift, "fail") == TaskOutcome.SUCCESS + assert _drift_task_outcome(drift, "skip") == TaskOutcome.SUCCESS + assert _drift_task_outcome(drift, "succeed") == TaskOutcome.SUCCESS + + +def test_drift_outcome_alarming_fail_is_fail_no_retry() -> None: + """An alarming report under the default ``on_drift="fail"`` → FAIL_NO_RETRY.""" + assert _drift_task_outcome(_alarming_drift(), "fail") == TaskOutcome.FAIL_NO_RETRY + + +def test_drift_outcome_alarming_skip_is_skip() -> None: + assert _drift_task_outcome(_alarming_drift(), "skip") == TaskOutcome.SKIP + + +def test_drift_outcome_alarming_succeed_is_success() -> None: + assert _drift_task_outcome(_alarming_drift(), "succeed") == TaskOutcome.SUCCESS diff --git a/tests/airflow/test_skeleton.py b/tests/airflow/test_skeleton.py index 2b907d07..d6f07e7f 100644 --- a/tests/airflow/test_skeleton.py +++ b/tests/airflow/test_skeleton.py @@ -148,6 +148,43 @@ def _airflow_loaded() -> bool: pytest.skip("airflow installed; gated tests/airflow/test_operators.py cover construction") +def test_drift_operator_access_is_airflow_free_and_construction_requires_airflow() -> None: + """#235 US-005: the dedicated drift operator name resolves WITHOUT importing + airflow (the no-eager-import contract), and — with the ``[airflow]`` extra + absent — CONSTRUCTING it raises an ``ImportError`` (the real operator + subclasses ``BaseOperator`` and genuinely needs Airflow at construction time). + + Mirrors the generate / prune-existing sibling tests above: attribute access + goes through the find_spec-guarded ``operators._get_drift_operator_class`` + (airflow-absent → the airflow-free placeholder whose ``__init__`` raises + ``ModuleNotFoundError``; airflow-present → the real ``BaseOperator`` subclass, + whose construction is covered by the gated ``tests/airflow/test_operators.py``, + #235 US-006). 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 SignalForgeDriftOperator + + assert SignalForgeDriftOperator is not None + assert _airflow_loaded() == had_airflow, ( + "accessing SignalForgeDriftOperator must not import the real airflow package" + ) + + if importlib.util.find_spec("airflow") is None: + with pytest.raises((ImportError, ModuleNotFoundError)): + SignalForgeDriftOperator( + task_id="t", + previous_diff_path="/history/prev/diff.json", + current_diff_path="/history/curr/diff.json", + ) + else: # pragma: no cover - default CI env has no [airflow] extra + 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.""" From 6ff8a839fc05fd885d1cea528b8fd1fae1cc2af2 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 13:26:44 -0700 Subject: [PATCH 09/13] bd_1-scaffolding-v42.6: gated drift execute tests + example DAG + airflow-ops docs (#235 US-006) --- docs/airflow-ops.md | 180 +++++- .../airflow/signalforge_drift_monitor_dag.py | 208 +++++++ tests/airflow/test_dag_parse.py | 61 +- tests/airflow/test_drift_operators.py | 529 ++++++++++++++++++ 4 files changed, 967 insertions(+), 11 deletions(-) create mode 100644 examples/airflow/signalforge_drift_monitor_dag.py create mode 100644 tests/airflow/test_drift_operators.py diff --git a/docs/airflow-ops.md b/docs/airflow-ops.md index 45d7599a..51eb247b 100644 --- a/docs/airflow-ops.md +++ b/docs/airflow-ops.md @@ -478,6 +478,164 @@ 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`). +## Drift / signal-rot detection + +SignalForge's prune step drops a test that *always-passes* on warehouse samples +(Architectural Commitment #1 — an always-pass test is noise). **Signal rot** is the +run-over-run version of that: a test that *used to* catch failing rows (a `kept` / +`kept-uncertain` / `flagged` tier) but **now always-passes** (`dropped` with +`drop_reason == "always-passes"`). The data changed underneath a test that silently +stopped doing anything — a schema-drift alarm worth paging on. A run-over-run **grade +regression** (the mean rubric score fell beyond a threshold) is the second alarming +signal. + +Drift detection is **single-model** in v0.7, opt-in, and comes in **two operator +surfaces** — both backed by the same pure, Airflow-free comparison core +(`signalforge.airflow.compute_drift`). + +### Form 1 — ergonomic: drift folded into the generate task + +The `SignalForgeGenerateOperator` detects drift *as part of* its run when +`detect_drift_against` is set: it parses the current diff off its own stdout, loads the +prior run's `diff.json` from the templated path, computes a `DriftReport`, optionally +persists this run's `diff.json` (+ `grade.json` sibling) for the next run +(`drift_history_dir`), and folds the drift verdict into the task state via `on_drift`. +One task does run + compare + persist + decide. The drift summary rides on the task's +XCom under the `"drift"` key. + +```python +from signalforge.airflow import SignalForgeGenerateOperator + +monitor = SignalForgeGenerateOperator( + task_id="drift_monitor", + project_dir="/opt/dbt/my_project", + model="models/staging/stg_orders.sql", + write=False, # read-only scheduled default + as_of="{{ ds }}", # reproducibility anchor + # Compare against yesterday's persisted diff; persist today's for tomorrow. + detect_drift_against="/opt/airflow/sf_history/{{ macros.ds_add(ds, -1) }}/diff.json", + drift_history_dir="/opt/airflow/sf_history/{{ ds }}", + on_drift="fail", # alarming drift → hard, no-retry failure +) +``` + +When `detect_drift_against` is unset the operator behaves byte-identically to the +pre-drift generate operator — no drift key, no behaviour change. + +### Form 2 — branchable: generate persists, a dedicated operator gates + +When you want the run and the alarm to be **separate tasks** — the generate should +always succeed-and-persist, and a downstream task is the pageable gate you branch / +alert on independently — use the dedicated `SignalForgeDriftOperator`. It runs **no +`signalforge` CLI invocation**: it reads two persisted `diff.json` sidecars +(yesterday + today, plus auto-resolved `grade.json` siblings) off disk, computes the +`DriftReport`, and drives its single task state off the drift verdict. Because it +touches no LLM and no warehouse, the worker needs **no credentials** for this task. + +```python +from signalforge.airflow import SignalForgeDriftOperator, SignalForgeGenerateOperator + +generate = SignalForgeGenerateOperator( + task_id="generate", + project_dir="/opt/dbt/my_project", + model="models/staging/stg_orders.sql", + detect_drift_against="/opt/airflow/sf_history/{{ macros.ds_add(ds, -1) }}/diff.json", + drift_history_dir="/opt/airflow/sf_history/{{ ds }}", # persists today's diff.json + on_drift="succeed", # record-only: the generate task never fails on drift +) + +drift_check = SignalForgeDriftOperator( + task_id="drift_check", + previous_diff_path="/opt/airflow/sf_history/{{ macros.ds_add(ds, -1) }}/diff.json", + current_diff_path="/opt/airflow/sf_history/{{ ds }}/diff.json", + on_drift="fail", # the pageable gate +) + +generate >> drift_check +``` + +`previous_diff_path` and `current_diff_path` are **required** params (validated at +DAG-parse). The dedicated operator's XCom **is** the drift payload (not nested under a +`"drift"` key — that nesting is the generate operator's shape). + +### `detect_drift_against` + `drift_history_dir` — the date-stamped history pattern + +Each run persists a date-stamped `diff.json` (`drift_history_dir="…/{{ ds }}"`); the +next run compares against the prior date's copy (`detect_drift_against="…/{{ macros.ds_add(ds, -1) }}/diff.json"`). +Both params are in `template_fields`, so the logical date stamps the path. Mount the +history base on durable storage so a run can find the prior run's sidecar. Persistence +reuses the fail-closed `write_sidecar` writer (no new writer); the `grade.json` sibling +is copied best-effort. Persistence is triggered by `detect_drift_against` being set. + +### `on_drift` (fail / skip / succeed) — most-severe-wins with `on_flagged` + +`on_drift` is the run-over-run analogue of `on_flagged`: + +- `fail` (default) — an alarming drift is a hard, **no-retry** `AirflowFailException`. + Signal rot is deterministic, so retrying cannot un-rot it; it is a reviewer signal, + not a transient. +- `skip` — `AirflowSkipException` (mark the task skipped — route to a review branch). +- `succeed` — pass through (record-only; the drift still rides on XCom). + +On the generate operator the flagged-axis (`on_flagged`) and the drift-axis (`on_drift`) +**combine most-severe-wins** on an exit-0 run (`FAIL_NO_RETRY > SKIP > SUCCESS`). A hard +load/parse/input/external error (exit 1/2/3) short-circuits both axes per the +[Exit → TaskOutcome → Airflow](#exit--taskoutcome--airflow) table. + +### Degrade, never fail (DEC-013) + +The comparison is fail-soft, so a drift monitor never breaks the DAG over its own +bookkeeping: + +- **Missing prior `diff.json`** (the first run) → a non-alarming **baseline** + (`baseline=True`); the task SUCCEEDs and persists today's diff for next time. +- **`model_unique_id` mismatch** or a **corrupt / unreadable / oversize** prior sidecar + → a non-alarming **degraded** report with `degrade_reason` set; the task SUCCEEDs. +- **Missing `grade.json`** (`--no-grade`) → tier-transition drift is still computed; the + grade-regression axis is simply empty. +- **Unparseable current diff** (on the generate path) → drift degrades to *no verdict* + (no `"drift"` key); the generate run's own exit-code / flagged verdict still governs. + +A degraded or baseline report is **never** `alarming`, so `on_drift` cannot trip on it. + +On the **dedicated** operator only, a missing / unreadable **current** diff is the one +hard error: it reads the current diff from a path (it runs no CLI), so an absent current +file means the operator is misconfigured → `AirflowConfigError` (CLI tier 2), not a +baseline. + +### `--as-of` reproducibility + +`as_of="{{ ds }}"` flows the logical date into `--as-of` so time-bound primitives (e.g. +`row_count_anomaly_by_period`) are pinned to the scheduled date — see +`docs/prune-ops.md`. The same two sidecars + same `as_of` reproduce a byte-identical +`DriftReport` (the report carries the two input `blake2b-8` hashes). + +### Drift XCom payload (`DriftReport.to_xcom`) + +The drift summary is JSON-serialisable, carries **no bulk sidecar text and no secrets**, +and on the generate operator is nested under `"drift"` (the dedicated operator returns it +directly). Shape: + +- `model_unique_id`, `as_of` (iso or `null`), `baseline`, `alarming`, `degrade_reason`, + `grade_regression_threshold`, `previous_diff_hash`, `current_diff_hash`; +- `counts` — per-category counts: `newly_always_passes` (the signal-rot tally), + `newly_dropped`, `newly_kept`, `added_artifacts`, `removed_artifacts`, + `grade_regressions`, `columns_added`, `columns_removed`; +- the transition lists (`newly_always_passes` / `newly_dropped` / `newly_kept` as + artifact dicts with a truncated one-line `why`), `added_artifacts` / + `removed_artifacts` (artifact-id lists), `grade_regressions`, `schema_shape_changes`. + +`alarming` is `True` iff there is signal rot (`newly_always_passes`) or a grade +regression (`grade_regressions`) — those are the categories that page; `newly_kept` / +`newly_dropped` / added / removed / schema-shape are informational. + +### Worked example DAG + +`examples/airflow/signalforge_drift_monitor_dag.py` (`dag_id="signalforge_drift_monitor"`) +ships both surfaces side by side: a `drift_monitor_ergonomic` task (Form 1) and a +`generate` → `drift_check` pair (Form 2). It ships `schedule=None`; set +`schedule="@daily"` for a real nightly monitor. + ## Scheduling for drift detection The example ships with `schedule=None` (manual trigger) so it never auto-spends on @@ -509,12 +667,18 @@ 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 run-over-run drift monitor + (`signalforge_drift_monitor`). 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 }}`). +- **execute** — `tests/airflow/test_drift_operators.py` and + `tests/airflow/test_operators.py` drive each operator's `execute` against a fake + `run_signalforge` (and, for the dedicated drift operator, the committed + `tests/fixtures/airflow/drift_pairs/*.json` sidecars): the result/drift → `TaskOutcome` + → Airflow-signal translation, the XCom shape, and the baseline / degrade / unparseable + paths. No credentials; runs in the gated CI `airflow` job. - **live** — runs the `generate` task against an `init-demo` project; self-skips without `SF_RUN_AIRFLOW=1` + `ANTHROPIC_API_KEY` + `GOOGLE_CLOUD_PROJECT` + `SF_RUN_BQ`. @@ -524,10 +688,12 @@ uv run --no-sync pytest -m airflow --no-cov # inside the constraints-pinned ai ## Caveats -- **Both operators have landed (epic #228):** the `SignalForgeGenerateOperator` (#232, - [section](#signalforgegenerateoperator)) and the no-LLM `SignalForgePruneExistingOperator` - (#233, [section](#signalforgepruneexistingoperator)). The `PythonOperator` example DAG - remains a from-scratch reference. +- **Three operators have landed (epic #228):** the `SignalForgeGenerateOperator` (#232, + [section](#signalforgegenerateoperator)), the no-LLM `SignalForgePruneExistingOperator` + (#233, [section](#signalforgepruneexistingoperator)), and the run-over-run drift surface + (#235) — `detect_drift_against` on the generate operator plus the dedicated + `SignalForgeDriftOperator` ([section](#drift--signal-rot-detection)). The `PythonOperator` + example DAG remains a from-scratch reference. - **Time-bound tests + reproducibility.** If your draft includes the time-bound `row_count_anomaly_by_period` variant, pass `--as-of YYYY-MM-DD` (the DAG's logical date is a natural source) so a re-run is reproducible — see `docs/prune-ops.md`. diff --git a/examples/airflow/signalforge_drift_monitor_dag.py b/examples/airflow/signalforge_drift_monitor_dag.py new file mode 100644 index 00000000..a8c4a419 --- /dev/null +++ b/examples/airflow/signalforge_drift_monitor_dag.py @@ -0,0 +1,208 @@ +"""Example Airflow DAG: nightly run-over-run drift / signal-rot monitor (epic #228, issue #235). + +SignalForge's prune step drops a test that *always-passes* on warehouse samples +(Architectural Commitment #1 — an always-pass test is noise). **Signal rot** is +the run-over-run version of that: a test that *used to* catch failing rows (a +``kept`` / ``kept-uncertain`` / ``flagged`` tier) but **now always-passes** +(``dropped`` with ``drop_reason == "always-passes"``). That transition is a +schema-drift alarm worth paging on — the data changed underneath a test that +silently stopped doing anything. A run-over-run **grade regression** (the mean +rubric score fell beyond a threshold) is the second alarming signal. + +This DAG shows the **two operator surfaces** for detecting it, side by side: + +## Form 1 — ergonomic: drift folded into the generate task + +``drift_monitor_ergonomic`` is a single :class:`SignalForgeGenerateOperator` that +detects drift *as part of* its run: it parses the current diff off its own +stdout, compares it against the prior run's persisted ``diff.json`` +(``detect_drift_against``), persists this run's ``diff.json`` for the next run +(``drift_history_dir``), and folds the drift verdict into its task state via +``on_drift``. One task does run + compare + persist + decide. The drift summary +rides on the task's XCom under the ``"drift"`` key. + +## Form 2 — branchable: generate persists, a dedicated operator gates + +When you want the run and the alarm to be **separate tasks** (e.g. the generate +should always succeed-and-persist, and a downstream task is the pageable gate you +can branch / alert on independently): + +- ``generate`` runs :class:`SignalForgeGenerateOperator` with + ``drift_history_dir`` (so it persists today's ``diff.json``) and + ``on_drift="succeed"`` (so it never fails the run on drift — it just records); +- ``drift_check`` runs the dedicated :class:`SignalForgeDriftOperator` + **downstream**, pointing ``previous_diff_path`` / ``current_diff_path`` at two + persisted date-stamped ``diff.json`` sidecars (yesterday + today). It runs NO + ``signalforge`` CLI invocation — it just reads the two sidecars, computes the + :class:`~signalforge.airflow.drift.DriftReport`, and is the task whose + ``on_drift="fail"`` pages. Its XCom IS the drift payload (counts + transition + lists + the two input hashes; no bulk text, no secrets). + +## ``on_drift`` (fail / skip / succeed) — most-severe-wins with ``on_flagged`` + +``on_drift`` is the run-over-run analogue of ``on_flagged``: ``fail`` (default — +an alarming drift is a hard, **no-retry** ``AirflowFailException``; signal rot is +deterministic, so retrying cannot un-rot it), ``skip`` (mark the task skipped — +route to a review branch via ``AirflowSkipException``), or ``succeed`` (pass +through). On the generate operator the flagged-axis and the drift-axis combine +**most-severe-wins** on an exit-0 run; a hard load/parse/input/external error +(exit 1/2/3) short-circuits both. + +## Degrade, never fail (DEC-013) + +The comparison is fail-soft: a **missing** prior ``diff.json`` (the first run) +makes this run a non-alarming **baseline**; a ``model_unique_id`` mismatch or a +corrupt/unreadable prior sidecar yields a non-alarming **degraded** report +(``degrade_reason`` set) — never an exception. A missing ``grade.json`` +(``--no-grade``) simply leaves the grade-regression axis empty. So a drift +monitor never breaks the DAG over its own bookkeeping; only a *genuine* alarm +trips ``on_drift``. + +## Reproducibility — ``as_of`` + +``as_of="{{ ds }}"`` flows the run's logical date into ``--as-of`` so time-bound +primitives (e.g. ``row_count_anomaly_by_period``) are pinned to the scheduled +date and the run is reproducible at ``(model, as_of)`` granularity. The same two +sidecars + same ``as_of`` reproduce a byte-identical ``DriftReport``. + +## Templated paths + +Both operators declare their path params in ``template_fields``, so the +prior/current sidecar locations are date-stamped from the task context: +``detect_drift_against`` uses yesterday's history dir +(``{{ macros.ds_add(ds, -1) }}``), ``drift_history_dir`` / ``current_diff_path`` +use today's (``{{ ds }}``). + +## Configuration (Airflow Variable / env override, with fallbacks) + +Resolved at DAG-parse time (env-override-first, then Airflow ``Variable``) WITH +safe fallbacks so the DAG always parses cleanly even before an operator wires +real config: + +- ``signalforge_project_dir`` / ``SF_PROJECT_DIR`` — dbt project root (must + contain ``target/manifest.json``). Falls back to ``/opt/airflow/dbt_project``. +- ``signalforge_model`` / ``SF_MODEL`` — the model to monitor (**file-path or + unique_id**; a bare name fails). Falls back to + ``models/staging/stg_orders.sql``. Surfaced as the ``model`` DAG param. +- ``signalforge_history_dir`` / ``SF_HISTORY_DIR`` — base directory under which + each run persists a date-stamped ``diff.json`` (+ ``grade.json``). Falls back + to ``/opt/airflow/signalforge_history``. Mount this on durable storage so a + run can compare against the prior run. + +Live runs need ``ANTHROPIC_API_KEY`` + the warehouse env (e.g. +``GOOGLE_CLOUD_PROJECT`` + gcloud ADC for BigQuery) available to the Airflow +worker (Form 1 + the ``generate`` half of Form 2 run the full pipeline; the +dedicated ``drift_check`` reads sidecars only and needs neither). It ships +``schedule=None`` so the example never auto-spends on credentials; set +``schedule="@daily"`` to turn it into a real nightly monitor. Full walkthrough: +docs/airflow-ops.md. +""" + +from __future__ import annotations + +from datetime import datetime + +from airflow import DAG + +from signalforge.airflow import SignalForgeDriftOperator, SignalForgeGenerateOperator + + +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 the operators construct + validly at DAG-parse time even before an operator wires real config. + """ + 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" +) +_model = _config("SF_MODEL", "signalforge_model", default="models/staging/stg_orders.sql") +_history_dir = _config( + "SF_HISTORY_DIR", "signalforge_history_dir", default="/opt/airflow/signalforge_history" +) + +# Date-stamped history paths (rendered from the task context before execute()). +# Yesterday's diff is the comparison baseline; today's dir is where this run +# persists its diff.json for tomorrow's run. +_today_dir = f"{_history_dir}/{{{{ ds }}}}" +_yesterday_dir = f"{_history_dir}/{{{{ macros.ds_add(ds, -1) }}}}" + + +with DAG( + dag_id="signalforge_drift_monitor", + # Manual trigger by default so the example never auto-spends on credentials. + # For a real nightly drift monitor, set schedule="@daily". + schedule=None, + start_date=datetime(2026, 1, 1), + catchup=False, + params={"model": _model}, + tags=["signalforge", "dbt", "data-quality", "drift"], + doc_md=__doc__, +) as dag: + # ----------------------------------------------------------------------- # + # Form 1 — ergonomic: one task runs + compares + persists + decides. + # ----------------------------------------------------------------------- # + drift_monitor_ergonomic = SignalForgeGenerateOperator( + task_id="drift_monitor_ergonomic", + project_dir=_project_dir, + model="{{ params.model }}", + # Read-only scheduled default: nothing is written to the dbt project. + write=False, + as_of="{{ ds }}", + # Compare this run's diff (parsed off stdout) against yesterday's + # persisted diff.json, and persist this run's diff.json (+ grade.json) + # under today's dir for tomorrow. A missing prior → baseline (no alarm). + detect_drift_against=f"{_yesterday_dir}/diff.json", + drift_history_dir=_today_dir, + # An alarming drift (signal rot or grade regression) fails the task, + # no retry. Use "skip" to route to a review branch, "succeed" to record-only. + on_drift="fail", + ) + + # ----------------------------------------------------------------------- # + # Form 2 — branchable: generate persists (and never fails on drift), a + # dedicated downstream operator is the pageable gate. + # ----------------------------------------------------------------------- # + generate = SignalForgeGenerateOperator( + task_id="generate", + project_dir=_project_dir, + model="{{ params.model }}", + write=False, + as_of="{{ ds }}", + # Persist today's diff.json (+ grade.json) for the dedicated check below. + # detect_drift_against is required to trigger persistence; point it at + # yesterday so the generate task also records a drift summary on its XCom. + detect_drift_against=f"{_yesterday_dir}/diff.json", + drift_history_dir=_today_dir, + # Record-only here: the generate task always succeeds; the gate is the + # downstream dedicated operator. + on_drift="succeed", + ) + + drift_check = SignalForgeDriftOperator( + task_id="drift_check", + # Two persisted, date-stamped diff.json sidecars: yesterday vs today. + # The grade.json siblings are auto-resolved next to each diff. + previous_diff_path=f"{_yesterday_dir}/diff.json", + current_diff_path=f"{_today_dir}/diff.json", + as_of="{{ ds }}", + # This is the pageable gate: an alarming drift fails the task (no retry). + on_drift="fail", + ) + + generate >> drift_check diff --git a/tests/airflow/test_dag_parse.py b/tests/airflow/test_dag_parse.py index 3b43c3ab..1d4a6cac 100644 --- a/tests/airflow/test_dag_parse.py +++ b/tests/airflow/test_dag_parse.py @@ -34,10 +34,10 @@ 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 run-over-run drift monitor); 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 +173,59 @@ def test_prune_existing_operator_renders_templated_fields() -> None: assert op.tests_dir is None +def test_drift_monitor_operator_example_dag_parses_without_import_errors() -> None: + """The run-over-run drift-monitor example DAG parses cleanly via DagBag (#235 DEC-019). + + Distinct ``dag_id`` from the other examples; THREE tasks demonstrating the + two drift surfaces side by side: the ergonomic ``SignalForgeGenerateOperator`` + with ``detect_drift_against`` (Form 1), and the branchable + ``generate`` → dedicated ``SignalForgeDriftOperator`` pair (Form 2). Parses + with NO SignalForge config in the env (the DAG's ``_config`` fallbacks keep + every operator's construction-time validation green at parse). + """ + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + dag = _load_example_dag("signalforge_drift_monitor") + assert set(dag.task_ids) == {"drift_monitor_ergonomic", "generate", "drift_check"} + # The branchable form wires the dedicated drift check downstream of generate. + assert dag.get_task("drift_check").upstream_task_ids == {"generate"} + + +def test_drift_operator_renders_templated_fields() -> None: + """The dedicated drift operator's ``template_fields`` render from the task context. + + Constructs the operator inside a DAG context and calls + ``render_template_fields`` with a synthetic context carrying ``ds``; asserts + the templated ``current_diff_path`` (``{{ ds }}/diff.json``) and ``as_of`` + (``{{ ds }}``) render to the injected value — the date-stamped sidecar-path + pattern the example DAG demonstrates (#235 DEC-010/DEC-019). + """ + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + import importlib + from datetime import datetime + + from airflow import DAG + + operators = importlib.import_module("signalforge.airflow.operators") + operator_cls = operators.SignalForgeDriftOperator + + with DAG(dag_id="render_test_drift", start_date=datetime(2026, 1, 1), schedule=None): + op = operator_cls( + task_id="drift_check", + previous_diff_path="/history/2026-06-14/diff.json", + current_diff_path="/history/{{ ds }}/diff.json", + as_of="{{ ds }}", + ) + + # BaseOperator.render_template_fields(context, jinja_env=None) renders every + # template_fields attr IN PLACE from the context (jinja_env built from the DAG). + op.render_template_fields({"ds": "2026-06-15"}) + + assert op.current_diff_path == "/history/2026-06-15/diff.json" + assert op.as_of == "2026-06-15" + # Non-templated literal field is untouched by the render pass. + assert op.previous_diff_path == "/history/2026-06-14/diff.json" + + def _live_skip_reason() -> str | None: missing = [ v diff --git a/tests/airflow/test_drift_operators.py b/tests/airflow/test_drift_operators.py new file mode 100644 index 00000000..66b8798d --- /dev/null +++ b/tests/airflow/test_drift_operators.py @@ -0,0 +1,529 @@ +"""Gated tests for the two drift surfaces' ``execute`` (#235 US-006). + +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). 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): +``uv run --no-sync pytest -m airflow --no-cov``. + +These pin BOTH drift surfaces end-to-end (exercising DEC-001/010/013/015): + +* the ``SignalForgeGenerateOperator`` drift path (``detect_drift_against`` + + ``on_drift`` + ``drift_history_dir`` persistence): the ``"drift"`` key on the + XCom; the signal-rot fixture pair driving ``on_drift`` fail/skip/succeed → + the matching Airflow signal; the baseline (prior missing) + degrade (model + mismatch) + unparseable-current degrade paths all succeeding; +* the dedicated ``SignalForgeDriftOperator``: prev+curr fixture pair → drift on + XCom; ``on_drift`` mapping; baseline (prior None) → success; a missing CURRENT + diff → hard ``AirflowConfigError``. + +``run_signalforge`` is monkeypatched to canned :class:`SignalForgeRunResult`s +(carrying the committed US-001 fixture diff JSON on stdout) so no real +``signalforge`` run happens; the dedicated operator reads the fixtures off disk +(it runs no CLI), so it is exercised against tmp-copied fixtures directly. +""" + +from __future__ import annotations + +import importlib +import json +from pathlib import Path + +import pytest + +from signalforge.airflow.result import SignalForgeRunResult + +pytestmark = pytest.mark.airflow + +_AIRFLOW_SKIP = "Apache Airflow not installed (run inside the constraints-pinned airflow venv)" + +_FIXTURE_DIR = Path(__file__).parent.parent / "fixtures" / "airflow" / "drift_pairs" + +_MODEL = "model.shop.fct_orders" + + +# --------------------------------------------------------------------------- # +# Fixture helpers +# --------------------------------------------------------------------------- # + + +def _fixture_text(name: str) -> str: + """Read a committed drift_pairs fixture's JSON text.""" + return (_FIXTURE_DIR / name).read_text(encoding="utf-8") + + +def _layout_prior( + tmp_path: Path, + *, + prev_diff: str | None = "signal_rot_prev_diff.json", + prev_grade: str | None = "signal_rot_prev_grade.json", + curr_grade: str | None = "signal_rot_curr_grade.json", +) -> tuple[str, str | None]: + """Lay out a prior ``/diff.json`` (+ ``grade.json`` sibling) for the generate path. + + Mirrors the on-disk convention the generate operator's drift wiring reads: + the prior diff sits at ``detect_drift_against`` and its grade sibling at + ``/grade.json``. The current run's grade sidecar is written + separately (the generate operator reads it off ``result.grade_sidecar_path``). + + Returns ``(detect_drift_against_path, current_grade_sidecar_path | None)``. + ``prev_diff=None`` lays out NO prior diff (the path still points into the + empty dir → a missing-prior baseline). + """ + prev = tmp_path / "prev" + prev.mkdir(exist_ok=True) + if prev_diff is not None: + (prev / "diff.json").write_text(_fixture_text(prev_diff), encoding="utf-8") + if prev_grade is not None: + (prev / "grade.json").write_text(_fixture_text(prev_grade), encoding="utf-8") + + current_grade_path: str | None = None + if curr_grade is not None: + curr = tmp_path / "curr" + curr.mkdir(exist_ok=True) + (curr / "grade.json").write_text(_fixture_text(curr_grade), encoding="utf-8") + current_grade_path = str(curr / "grade.json") + + return str(prev / "diff.json"), current_grade_path + + +def _layout_pair(tmp_path: Path, *, prior: bool = True) -> tuple[str, str]: + """Lay out a prev + curr diff/grade quartet for the dedicated drift operator. + + The dedicated operator reads BOTH diffs off disk and auto-siblings the grades + (``/diff.json`` ↔ ``/grade.json``). Returns + ``(previous_diff_path, current_diff_path)``. ``prior=False`` omits the prior + diff so the operator establishes a baseline. + """ + prev = tmp_path / "prev" + prev.mkdir(exist_ok=True) + if prior: + (prev / "diff.json").write_text( + _fixture_text("signal_rot_prev_diff.json"), encoding="utf-8" + ) + (prev / "grade.json").write_text(_fixture_text("signal_rot_prev_grade.json"), encoding="utf-8") + + curr = tmp_path / "curr" + curr.mkdir(exist_ok=True) + (curr / "diff.json").write_text(_fixture_text("signal_rot_curr_diff.json"), encoding="utf-8") + (curr / "grade.json").write_text(_fixture_text("signal_rot_curr_grade.json"), encoding="utf-8") + + return str(prev / "diff.json"), str(curr / "diff.json") + + +def _generate_result( + *, + stdout_fixture: str | None = "signal_rot_curr_diff.json", + stdout: str | None = None, + grade_sidecar_path: str | None = None, + exit_code: int = 0, + flagged: int = 0, +) -> SignalForgeRunResult: + """Build a canned generate result carrying the CURRENT diff JSON on stdout. + + The generate operator's drift wiring parses the current :class:`DiffReport` + off ``result.stdout`` (#231 DEC-005), so the canned result carries the + committed current-run fixture there. ``stdout`` overrides ``stdout_fixture`` + (use it for an unparseable-current degrade test). + """ + body = stdout if stdout is not None else _fixture_text(stdout_fixture) if stdout_fixture else "" + return SignalForgeRunResult( + exit_code=exit_code, + model_unique_ids=(_MODEL,), + kept=1, + kept_uncertain=0, + dropped=1, + flagged=flagged, + mean_grade=0.8, + diff_sidecar_path=None, + grade_sidecar_path=grade_sidecar_path, + duration_seconds=1.0, + stdout=body, + stderr="", + ) + + +def _generate_operator_class() -> type: + operators = importlib.import_module("signalforge.airflow.operators") + return operators.SignalForgeGenerateOperator + + +def _drift_operator_class() -> type: + operators = importlib.import_module("signalforge.airflow.operators") + return operators.SignalForgeDriftOperator + + +def _patch_run(monkeypatch: pytest.MonkeyPatch, result: SignalForgeRunResult) -> None: + """Monkeypatch ``operators.run_signalforge`` to return ``result``.""" + + def _fake_run( + argv: list[str], + *, + project_dir: object, + invocation: object = "in_process", + timeout_seconds: object = None, + ) -> SignalForgeRunResult: + return result + + monkeypatch.setattr("signalforge.airflow.operators.run_signalforge", _fake_run) + + +# --------------------------------------------------------------------------- # +# SignalForgeGenerateOperator — drift path (detect_drift_against) +# --------------------------------------------------------------------------- # + + +def test_generate_drift_signal_rot_on_fail_raises_airflow_fail_exception( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Signal rot (kept→always-passes) with ``on_drift='fail'`` → AirflowFailException. + + The exit-0 generate run flagged nothing, but the prior→current diff shows the + ``not_null`` test rotted from ``kept`` to ``dropped(always-passes)`` → the + drift report is alarming → most-severe(SUCCESS, FAIL_NO_RETRY) = FAIL_NO_RETRY. + """ + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + from airflow.exceptions import AirflowFailException + + detect_against, curr_grade = _layout_prior(tmp_path) + _patch_run(monkeypatch, _generate_result(grade_sidecar_path=curr_grade)) + + op = _generate_operator_class()( + task_id="gen", + project_dir="/proj", + model=_MODEL, + detect_drift_against=detect_against, + on_drift="fail", + ) + with pytest.raises(AirflowFailException): + op.execute(context={}) + + +def test_generate_drift_signal_rot_on_skip_raises_airflow_skip_exception( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Alarming drift with ``on_drift='skip'`` → AirflowSkipException (route to review).""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + from airflow.exceptions import AirflowSkipException + + detect_against, curr_grade = _layout_prior(tmp_path) + _patch_run(monkeypatch, _generate_result(grade_sidecar_path=curr_grade)) + + op = _generate_operator_class()( + task_id="gen", + project_dir="/proj", + model=_MODEL, + detect_drift_against=detect_against, + on_drift="skip", + ) + with pytest.raises(AirflowSkipException): + op.execute(context={}) + + +def test_generate_drift_signal_rot_on_succeed_returns_xcom_with_drift_key( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Alarming drift with ``on_drift='succeed'`` → no raise; XCom carries ``"drift"`` (DEC-015). + + The drift summary rides under the ``"drift"`` key alongside the run's own + counts; the signal-rot transition (1 newly-always-passes) and the grade + regression (prev 0.9 → curr 0.8, delta 0.1 > 0.05) both surface. + """ + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + + detect_against, curr_grade = _layout_prior(tmp_path) + _patch_run(monkeypatch, _generate_result(grade_sidecar_path=curr_grade)) + + op = _generate_operator_class()( + task_id="gen", + project_dir="/proj", + model=_MODEL, + detect_drift_against=detect_against, + on_drift="succeed", + ) + xcom = op.execute(context={}) + + # Base run keys still present (the run succeeded, exit 0, flagged 0). + assert xcom["exit_code"] == 0 + assert xcom["below_threshold"] is False + # The drift summary is nested under "drift". + assert "drift" in xcom + drift = xcom["drift"] + assert isinstance(drift, dict) + assert drift["model_unique_id"] == _MODEL + assert drift["alarming"] is True + assert drift["baseline"] is False + assert drift["counts"]["newly_always_passes"] == 1 + assert drift["counts"]["grade_regressions"] == 1 + # JSON-serialisable (XCom hygiene): round-trips with no bulk text / secrets. + json.dumps(xcom) + + +def test_generate_drift_baseline_when_prior_missing_succeeds( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """A missing prior diff (first run) → baseline DriftReport → SUCCESS (DEC-013). + + ``on_drift='fail'`` does NOT trip because a baseline is never alarming. + """ + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + + # Point detect_drift_against at a path with NO prior diff file present. + detect_against, curr_grade = _layout_prior(tmp_path, prev_diff=None, prev_grade=None) + _patch_run(monkeypatch, _generate_result(grade_sidecar_path=curr_grade)) + + op = _generate_operator_class()( + task_id="gen", + project_dir="/proj", + model=_MODEL, + detect_drift_against=detect_against, + on_drift="fail", + ) + xcom = op.execute(context={}) + + assert "drift" in xcom + drift = xcom["drift"] + assert isinstance(drift, dict) + assert drift["baseline"] is True + assert drift["alarming"] is False + + +def test_generate_drift_degrade_on_model_mismatch_succeeds( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """A prior diff for a DIFFERENT model → degraded (non-alarming) report → SUCCESS (DEC-013). + + ``compute_drift`` degrades on a ``model_unique_id`` mismatch: it sets + ``degrade_reason`` and an empty (never-alarming) report rather than raising, + so ``on_drift='fail'`` does not trip. + """ + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + + # Write a prior diff whose model_unique_id differs from the current run's. + prev = tmp_path / "prev" + prev.mkdir() + prior_obj = json.loads(_fixture_text("signal_rot_prev_diff.json")) + prior_obj["model_unique_id"] = "model.shop.some_other_model" + (prev / "diff.json").write_text(json.dumps(prior_obj), encoding="utf-8") + detect_against = str(prev / "diff.json") + + _patch_run(monkeypatch, _generate_result()) + + op = _generate_operator_class()( + task_id="gen", + project_dir="/proj", + model=_MODEL, + detect_drift_against=detect_against, + on_drift="fail", + ) + xcom = op.execute(context={}) + + assert "drift" in xcom + drift = xcom["drift"] + assert isinstance(drift, dict) + assert drift["degrade_reason"] is not None + assert drift["alarming"] is False + + +def test_generate_drift_unparseable_current_skips_drift( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """An unparseable current diff (no JSON on stdout) → no ``"drift"`` key, run governs (DEC-013). + + Drift degrades to ``None`` (no verdict, cannot alarm); the generate run's own + exit-0 / flagged-0 verdict still governs the task → SUCCESS, no drift key. + """ + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + + detect_against, _ = _layout_prior(tmp_path) + _patch_run(monkeypatch, _generate_result(stdout="not json at all", stdout_fixture=None)) + + op = _generate_operator_class()( + task_id="gen", + project_dir="/proj", + model=_MODEL, + detect_drift_against=detect_against, + on_drift="fail", + ) + xcom = op.execute(context={}) + + assert "drift" not in xcom + assert xcom["exit_code"] == 0 + + +def test_generate_drift_persists_current_run_to_history_dir( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """``drift_history_dir`` persists this run's diff.json (+ grade.json) for next time (DEC-009). + + Reuses the fail-closed ``write_sidecar`` writer; the grade sidecar is copied + best-effort. After ``execute`` the history dir holds a valid prior pair the + next run can point ``detect_drift_against`` at. + """ + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + + from signalforge.airflow.drift import load_diff_report + + detect_against, curr_grade = _layout_prior(tmp_path) + history_dir = tmp_path / "history" + _patch_run(monkeypatch, _generate_result(grade_sidecar_path=curr_grade)) + + op = _generate_operator_class()( + task_id="gen", + project_dir="/proj", + model=_MODEL, + detect_drift_against=detect_against, + drift_history_dir=str(history_dir), + on_drift="succeed", + ) + op.execute(context={}) + + persisted_diff = history_dir / "diff.json" + assert persisted_diff.exists() + loaded = load_diff_report(persisted_diff) + assert loaded is not None + assert loaded.model_unique_id == _MODEL + # The grade sidecar was copied alongside it. + assert (history_dir / "grade.json").exists() + + +# --------------------------------------------------------------------------- # +# SignalForgeDriftOperator — dedicated, reads two diff.json sidecars +# --------------------------------------------------------------------------- # + + +def test_drift_operator_signal_rot_on_fail_raises_airflow_fail_exception(tmp_path: Path) -> None: + """Signal-rot pair with ``on_drift='fail'`` → AirflowFailException (no retry).""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + from airflow.exceptions import AirflowFailException + + prev_diff, curr_diff = _layout_pair(tmp_path) + op = _drift_operator_class()( + task_id="drift", + previous_diff_path=prev_diff, + current_diff_path=curr_diff, + on_drift="fail", + ) + with pytest.raises(AirflowFailException): + op.execute(context={}) + + +def test_drift_operator_signal_rot_on_skip_raises_airflow_skip_exception(tmp_path: Path) -> None: + """Signal-rot pair with ``on_drift='skip'`` → AirflowSkipException.""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + from airflow.exceptions import AirflowSkipException + + prev_diff, curr_diff = _layout_pair(tmp_path) + op = _drift_operator_class()( + task_id="drift", + previous_diff_path=prev_diff, + current_diff_path=curr_diff, + on_drift="skip", + ) + with pytest.raises(AirflowSkipException): + op.execute(context={}) + + +def test_drift_operator_signal_rot_on_succeed_returns_drift_xcom(tmp_path: Path) -> None: + """Signal-rot pair with ``on_drift='succeed'`` → no raise; returns ``drift.to_xcom()`` directly. + + The dedicated operator's XCom IS the drift payload (NOT nested under a + ``"drift"`` key — that nesting is the generate operator's shape). + """ + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + + prev_diff, curr_diff = _layout_pair(tmp_path) + op = _drift_operator_class()( + task_id="drift", + previous_diff_path=prev_diff, + current_diff_path=curr_diff, + on_drift="succeed", + ) + xcom = op.execute(context={}) + + assert xcom["model_unique_id"] == _MODEL + assert xcom["alarming"] is True + assert xcom["baseline"] is False + assert xcom["counts"]["newly_always_passes"] == 1 + assert xcom["counts"]["grade_regressions"] == 1 + json.dumps(xcom) + + +def test_drift_operator_baseline_when_prior_missing_succeeds(tmp_path: Path) -> None: + """A missing PRIOR diff → baseline DriftReport → SUCCESS, returns the baseline XCom (DEC-013). + + ``on_drift='fail'`` does not trip because a baseline is never alarming. + """ + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + + prev_diff, curr_diff = _layout_pair(tmp_path, prior=False) + op = _drift_operator_class()( + task_id="drift", + previous_diff_path=prev_diff, # configured path, but no file present + current_diff_path=curr_diff, + on_drift="fail", + ) + xcom = op.execute(context={}) + + assert xcom["baseline"] is True + assert xcom["alarming"] is False + assert xcom["model_unique_id"] == _MODEL + + +def test_drift_operator_missing_current_diff_raises_config_error(tmp_path: Path) -> None: + """A missing/unreadable CURRENT diff is a hard config error, NOT a baseline (DEC-010). + + The dedicated operator reads the current diff from a path (it runs no CLI), so + an absent current file means the operator is misconfigured — it raises + ``AirflowConfigError`` rather than silently degrading. + """ + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + from signalforge.airflow.errors import AirflowConfigError + + prev_diff, curr_diff = _layout_pair(tmp_path) + # Remove the current diff so the load returns None. + Path(curr_diff).unlink() + op = _drift_operator_class()( + task_id="drift", + previous_diff_path=prev_diff, + current_diff_path=curr_diff, + on_drift="fail", + ) + with pytest.raises(AirflowConfigError): + op.execute(context={}) + + +def test_drift_operator_construction_rejects_empty_current_path() -> None: + """``current_diff_path`` is required — empty fails fast at __init__ (DEC-010/DEC-012).""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + from signalforge.airflow.errors import AirflowConfigError + + with pytest.raises(AirflowConfigError): + _drift_operator_class()( + task_id="drift", + previous_diff_path="/prev/diff.json", + current_diff_path="", + ) + + +def test_drift_operator_construction_rejects_bad_on_drift() -> None: + """An invalid ``on_drift`` fails fast at __init__ (DEC-010/DEC-012).""" + pytest.importorskip("airflow", reason=_AIRFLOW_SKIP) + from signalforge.airflow.errors import AirflowConfigError + + with pytest.raises(AirflowConfigError): + _drift_operator_class()( + task_id="drift", + previous_diff_path="/prev/diff.json", + current_diff_path="/curr/diff.json", + on_drift="bogus", + ) From b241ebe9024912592b49773a21bb5a48ee104121 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 13:35:19 -0700 Subject: [PATCH 10/13] =?UTF-8?q?bd=5F1-scaffolding-v42.7:=20Quality=20gat?= =?UTF-8?q?e=20=E2=80=94=20tighten=20schema-shape=20artifact-id=20arity=20?= =?UTF-8?q?+=20docs=20accuracy=20(#235=20US-007)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/airflow-ops.md | 8 +++++--- src/signalforge/airflow/drift.py | 4 ++-- tests/airflow/test_drift_core.py | 19 +++++++++++++++++++ 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/docs/airflow-ops.md b/docs/airflow-ops.md index 51eb247b..6e0fd402 100644 --- a/docs/airflow-ops.md +++ b/docs/airflow-ops.md @@ -565,7 +565,9 @@ next run compares against the prior date's copy (`detect_drift_against="…/{{ m Both params are in `template_fields`, so the logical date stamps the path. Mount the history base on durable storage so a run can find the prior run's sidecar. Persistence reuses the fail-closed `write_sidecar` writer (no new writer); the `grade.json` sibling -is copied best-effort. Persistence is triggered by `detect_drift_against` being set. +is copied best-effort. Persistence requires **both** `detect_drift_against` (which enables +the drift step) **and** `drift_history_dir` (the destination): with `drift_history_dir` +unset the run still computes drift but writes nothing. ### `on_drift` (fail / skip / succeed) — most-severe-wins with `on_flagged` @@ -616,8 +618,8 @@ The drift summary is JSON-serialisable, carries **no bulk sidecar text and no se and on the generate operator is nested under `"drift"` (the dedicated operator returns it directly). Shape: -- `model_unique_id`, `as_of` (iso or `null`), `baseline`, `alarming`, `degrade_reason`, - `grade_regression_threshold`, `previous_diff_hash`, `current_diff_hash`; +- `schema_version`, `model_unique_id`, `as_of` (iso or `null`), `baseline`, `alarming`, + `degrade_reason`, `grade_regression_threshold`, `previous_diff_hash`, `current_diff_hash`; - `counts` — per-category counts: `newly_always_passes` (the signal-rot tally), `newly_dropped`, `newly_kept`, `added_artifacts`, `removed_artifacts`, `grade_regressions`, `columns_added`, `columns_removed`; diff --git a/src/signalforge/airflow/drift.py b/src/signalforge/airflow/drift.py index de7b87ad..36798146 100644 --- a/src/signalforge/airflow/drift.py +++ b/src/signalforge/airflow/drift.py @@ -119,9 +119,9 @@ def _columns_from_artifact_ids(artifact_ids: Iterable[str]) -> set[str]: columns: set[str] = set() for artifact_id in artifact_ids: parts = artifact_id.split(".") - if len(parts) >= 2 and parts[0] == "column": + if len(parts) >= 3 and parts[0] == "column": columns.add(parts[1]) - elif len(parts) >= 3 and parts[0] == "test" and parts[1] == "column": + elif len(parts) >= 4 and parts[0] == "test" and parts[1] == "column": columns.add(parts[2]) return columns diff --git a/tests/airflow/test_drift_core.py b/tests/airflow/test_drift_core.py index ce6d86b5..e0f3c2f2 100644 --- a/tests/airflow/test_drift_core.py +++ b/tests/airflow/test_drift_core.py @@ -284,6 +284,25 @@ def test_schema_shape_test_with_args_hash_suffix_parses_column() -> None: assert report.schema_shape_changes.columns_added == ("amount",) +def test_schema_shape_ignores_too_short_malformed_artifact_ids() -> None: + """Short/malformed dotted forms below the spec arity contribute no column. + + The canonical shapes are ``column..`` (3 parts) and + ``test.column..`` (4 parts). A 2-part ``column.amount`` or a + 3-part ``test.column.amount`` is malformed and must NOT be mined as a + column — otherwise a corrupt sidecar would pollute schema_shape_changes. + """ + prev = _diff(entries=()) + curr = _diff( + entries=( + _entry("column.amount", "kept"), + _entry("test.column.region", "kept"), + ) + ) + report = compute_drift(previous_diff=prev, current_diff=curr) + assert report.schema_shape_changes == SchemaShapeDelta() + + # --------------------------------------------------------------------------- # Grade regression (DEC-005) at / above / below threshold. # --------------------------------------------------------------------------- From d477c4abc019bf05fa8f23909fd16033ae39165b Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 13:37:37 -0700 Subject: [PATCH 11/13] bd_1-scaffolding-v42.8: document drift detection in airflow-integration.md (#235 US-008) --- .claude/rules/airflow-integration.md | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/.claude/rules/airflow-integration.md b/.claude/rules/airflow-integration.md index 7089d7cc..d623cfd0 100644 --- a/.claude/rules/airflow-integration.md +++ b/.claude/rules/airflow-integration.md @@ -84,10 +84,28 @@ 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`. +## Drift / signal-rot detection (#235 DEC-001…019) + +Run-over-run drift detection — compare run N vs N-1 and page on **signal rot** (a test that was `kept` and is now `dropped:always-passes`) or a **grade regression**. The headline scheduled value-add of epic #228. Two surfaces, one shared airflow-free pure core. The durable patterns: + +- **Airflow-free pure core in `signalforge/airflow/drift.py`** (DEC-002) — eagerly importable, NO `from airflow`, eager-re-exported from `__init__.py` (alongside `result`/`runner`, NOT the lazy `__getattr__`). Carries `DriftReport` (+ `DriftArtifact`/`GradeRegression`/`SchemaShapeDelta`), the pure `compute_drift(*, previous_diff, current_diff, previous_grade=None, current_grade=None, as_of=None, grade_regression_threshold=0.05) -> DriftReport`, and the fail-soft loaders `load_diff_report`/`load_grade_report`/`parse_diff_report`. Reusable by the v0.8 GH Action (hoist candidate — see v0.8 note). +- **`on_drift` is the run-over-run analogue of `on_flagged` — most-severe-wins, NOT a 5th axis precedent (DEC-006).** `decide_task_outcome(result, *, on_flagged="fail", on_drift="fail", drift: DriftReport | None = None)`. **Byte-identical when `drift is None`** (every #232/#233 caller + test unchanged — pinned). When `drift.alarming` on an exit-0 run, fold `on_drift` and return the MOST-SEVERE of {flagged-outcome, drift-outcome} (rank `FAIL_NO_RETRY > SKIP > SUCCESS`). Exit tiers 1/2/3 short-circuit before either policy. **`on_drift` keys on `DriftReport.alarming` (a property of an exit-0 run), never on the exit code** — same "decision layered on a successful run" rule as `on_flagged`. `TaskOutcome` stays 4-valued; no new exit tier; no new error class (config faults reuse `AirflowConfigError` tier 2). +- **`alarming` = `newly_always_passes` OR `grade_regressions` only (DEC-005).** The OTHER categories (`newly_dropped`/`newly_kept`/`added_artifacts`/`removed_artifacts`/`schema_shape_changes`) are reported but informational — they don't page. A degraded or baseline report is **never** alarming by construction. +- **Degrade-never-fail (DEC-013).** No prior file → `baseline=True` empty report + SUCCESS; `model_unique_id` mismatch / corrupt prior → `degrade_reason` + WARNING + SUCCESS; `--no-grade` → `grade_regressions=()`, tier-transition drift still computed. The dedicated operator's ONE hard error is a missing/unreadable CURRENT diff (`AirflowConfigError`). Mirrors the conservative-bias routing posture. +- **Two surfaces (DEC-001).** (1) `detect_drift_against` flag on `SignalForgeGenerateOperator` (ergonomic: run + compare in one task; drift on the single-model path only — `--select` batch logs an INFO and skips drift); (2) dedicated `SignalForgeDriftOperator` (reads two `diff.json` sidecars downstream, branchable). The dedicated operator has NO `SignalForgeRunResult`, so its task state is driven by a pure `_drift_task_outcome(drift, on_drift)` that mirrors `decide_task_outcome`'s on_drift semantics; the airflow raise stays confined to `raise_for_outcome`. +- **Templated-path history, persistence reuses the EXISTING writer (DEC-009).** `detect_drift_against="…/{{ macros.ds_add(ds,-1) }}/diff.json"` (prior) + `drift_history_dir="…/{{ ds }}"` (where THIS run persists its diff.json for tomorrow). **Persistence requires BOTH params** — `detect_drift_against` enables the drift step, `drift_history_dir` is the destination; with the latter unset the run computes drift but writes nothing. Persistence reuses `diff._sidecar.write_sidecar` (containment anchored to `drift_history_dir`, which may sit outside `project_dir`) — NO new fail-closed writer, NO new audit class. +- **The prior-read path is operator-TRUSTED (DEC-008).** `load_diff_report` is symlink-loop-hardened + size-capped (10 MB, mirrors diff's `existing_schema` cap) but NOT project-contained — the prior sidecar legitimately lives outside `project_dir`. Reads are fail-soft (absent/corrupt/oversize → `None`). +- **Schema-shape = column ADD/REMOVE only (DEC-004 Q4).** Derived from `artifact_id` prefixes — column SET mined from `column..` (≥3 dotted parts) and `test.column..` (≥4 parts); **enforce the arity** (a 2-/3-part malformed id must NOT be mined — a corrupt sidecar would otherwise pollute `schema_shape_changes`). Retype is OUT OF SCOPE (sidecars carry no types). +- **Determinism (DEC-016).** `compute_drift` iterates `sorted` artifact_ids; transition tuples + column lists sorted; carries `previous_diff_hash`/`current_diff_hash` (project `blake2b-8` recipe over `model_dump_json(by_alias=True)` → canonical `json.dumps`). Same two sidecars + same `as_of` → byte-identical report. `DriftReport` is read-back-able → paired `StrictDriftReport(extra="forbid")` drift detector + committed fixture (`tests/fixtures/airflow/drift_report_v1.json`). +- **XCom hygiene (DEC-015).** `DriftReport.to_xcom()` = per-category counts + transition lists (truncated `why`) + `alarming` + `as_of` + the two input hashes + `schema_version` + `degrade_reason`. No bulk sidecar text, no secrets. On the generate operator it nests under a `"drift"` key; the dedicated operator returns it directly. +- **`--as-of` reproducibility carve-out** surfaced on both operators (the #171 precedent) so a comparison is reproducible at `(model, as_of)`. + +The `SignalForgeDriftOperator` follows the #232/#233 deferred-class pattern verbatim (module `__getattr__` + `find_spec("airflow")` + `functools.cache` factory + airflow-free placeholder + UNGATED skeleton test — required for the codecov patch gate). Example DAG `examples/airflow/signalforge_drift_monitor_dag.py` (`dag_id="signalforge_drift_monitor"`, both forms). Certified vs Airflow 2.10.4 (50 gated tests). + ## 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/235-drift-detection.md` — DEC-001…DEC-019 (run-over-run drift / signal-rot detection: airflow-free `compute_drift` + `DriftReport`, `on_drift` most-severe-wins, both surfaces, templated history). `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,drift,_airflow_compat,__init__}.py`, `src/signalforge/__main__.py`. `examples/airflow/{signalforge_generate_operator_dag,signalforge_prune_existing_operator_dag,signalforge_drift_monitor_dag}.py`. `tests/airflow/{test_operators,test_operators_helpers,test_dag_parse,test_drift_core,test_drift_loaders,test_drift_report_schema,test_drift_operators,test_result,test_skeleton}.py`. `tests/fixtures/airflow/{drift_report_v1.json,drift_pairs/}`. 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). From 17e2adc4c13250157509c143546cef18665cbcc7 Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 13:38:00 -0700 Subject: [PATCH 12/13] #235: mark plan Complete --- plans/super/235-drift-detection.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plans/super/235-drift-detection.md b/plans/super/235-drift-detection.md index c2298d00..ee445b05 100644 --- a/plans/super/235-drift-detection.md +++ b/plans/super/235-drift-detection.md @@ -2,7 +2,7 @@ ## Meta - **Ticket:** #235 — Airflow: drift / signal-rot detection mode (run-over-run). Part of epic #228 (v0.7 Airflow). Depends on #231 (result/XCom contract — landed) and #232 (`SignalForgeGenerateOperator` — landed). -- **Phase:** devolved (2026-06-16) — beads created, ready for Ralph. +- **Phase:** Complete (2026-06-16) — all 8 stories built + merged on `feature/235-drift-detection`; Quality Gate (4 diverse-angle reviews + 2 fixes) passed; certified vs Airflow 2.10.4 (50 gated tests). Epic `bd_1-scaffolding-v42` auto-closed. Pending: merge `feature/235-drift-detection` → `dev`. - **Branch:** `feature/235-drift-detection` (worktree `/home/wesd/Projects/worktrees/SignalForge/235-drift-detection`, base `dev`). - **Sessions:** 1 (2026-06-16). From 4345f04593cf7dfbec613f876d0bc0fe3e00105b Mon Sep 17 00:00:00 2001 From: Wes Duenow Date: Tue, 16 Jun 2026 14:53:05 -0700 Subject: [PATCH 13/13] #235: Address PR review feedback (threshold validation, blank-path normalization, degrade docs, branchable example on_flagged) --- .claude/rules/airflow-integration.md | 2 +- docs/airflow-ops.md | 12 +++-- .../airflow/signalforge_drift_monitor_dag.py | 22 +++++---- src/signalforge/airflow/operators.py | 40 ++++++++++++++++- tests/airflow/test_operators_helpers.py | 45 +++++++++++++++++++ 5 files changed, 106 insertions(+), 15 deletions(-) diff --git a/.claude/rules/airflow-integration.md b/.claude/rules/airflow-integration.md index d59a30ee..8ae7644f 100644 --- a/.claude/rules/airflow-integration.md +++ b/.claude/rules/airflow-integration.md @@ -91,7 +91,7 @@ Run-over-run drift detection — compare run N vs N-1 and page on **signal rot** - **Airflow-free pure core in `signalforge/airflow/drift.py`** (DEC-002) — eagerly importable, NO `from airflow`, eager-re-exported from `__init__.py` (alongside `result`/`runner`, NOT the lazy `__getattr__`). Carries `DriftReport` (+ `DriftArtifact`/`GradeRegression`/`SchemaShapeDelta`), the pure `compute_drift(*, previous_diff, current_diff, previous_grade=None, current_grade=None, as_of=None, grade_regression_threshold=0.05) -> DriftReport`, and the fail-soft loaders `load_diff_report`/`load_grade_report`/`parse_diff_report`. Reusable by the v0.8 GH Action (hoist candidate — see v0.8 note). - **`on_drift` is the run-over-run analogue of `on_flagged` — most-severe-wins, NOT a 5th axis precedent (DEC-006).** `decide_task_outcome(result, *, on_flagged="fail", on_drift="fail", drift: DriftReport | None = None)`. **Byte-identical when `drift is None`** (every #232/#233 caller + test unchanged — pinned). When `drift.alarming` on an exit-0 run, fold `on_drift` and return the MOST-SEVERE of {flagged-outcome, drift-outcome} (rank `FAIL_NO_RETRY > SKIP > SUCCESS`). Exit tiers 1/2/3 short-circuit before either policy. **`on_drift` keys on `DriftReport.alarming` (a property of an exit-0 run), never on the exit code** — same "decision layered on a successful run" rule as `on_flagged`. `TaskOutcome` stays 4-valued; no new exit tier; no new error class (config faults reuse `AirflowConfigError` tier 2). - **`alarming` = `newly_always_passes` OR `grade_regressions` only (DEC-005).** The OTHER categories (`newly_dropped`/`newly_kept`/`added_artifacts`/`removed_artifacts`/`schema_shape_changes`) are reported but informational — they don't page. A degraded or baseline report is **never** alarming by construction. -- **Degrade-never-fail (DEC-013).** No prior file → `baseline=True` empty report + SUCCESS; `model_unique_id` mismatch / corrupt prior → `degrade_reason` + WARNING + SUCCESS; `--no-grade` → `grade_regressions=()`, tier-transition drift still computed. The dedicated operator's ONE hard error is a missing/unreadable CURRENT diff (`AirflowConfigError`). Mirrors the conservative-bias routing posture. +- **Degrade-never-fail (DEC-013).** No prior file **OR a corrupt/unreadable/oversize prior** → `baseline=True` empty report + SUCCESS (the fail-soft `load_diff_report` returns `None` for absent AND malformed alike — a corrupt prior is NOT distinguished from a missing one, so it does NOT set `degrade_reason`); `model_unique_id` mismatch → `degrade_reason` + WARNING + SUCCESS (the ONE `degrade_reason` path, fired inside `compute_drift` after both diffs load); `--no-grade` → `grade_regressions=()`, tier-transition drift still computed. The dedicated operator's ONE hard error is a missing/unreadable CURRENT diff (`AirflowConfigError`). Mirrors the conservative-bias routing posture. - **Two surfaces (DEC-001).** (1) `detect_drift_against` flag on `SignalForgeGenerateOperator` (ergonomic: run + compare in one task; drift on the single-model path only — `--select` batch logs an INFO and skips drift); (2) dedicated `SignalForgeDriftOperator` (reads two `diff.json` sidecars downstream, branchable). The dedicated operator has NO `SignalForgeRunResult`, so its task state is driven by a pure `_drift_task_outcome(drift, on_drift)` that mirrors `decide_task_outcome`'s on_drift semantics; the airflow raise stays confined to `raise_for_outcome`. - **Templated-path history, persistence reuses the EXISTING writer (DEC-009).** `detect_drift_against="…/{{ macros.ds_add(ds,-1) }}/diff.json"` (prior) + `drift_history_dir="…/{{ ds }}"` (where THIS run persists its diff.json for tomorrow). **Persistence requires BOTH params** — `detect_drift_against` enables the drift step, `drift_history_dir` is the destination; with the latter unset the run computes drift but writes nothing. Persistence reuses `diff._sidecar.write_sidecar` (containment anchored to `drift_history_dir`, which may sit outside `project_dir`) — NO new fail-closed writer, NO new audit class. - **The prior-read path is operator-TRUSTED (DEC-008).** `load_diff_report` is symlink-loop-hardened + size-capped (10 MB, mirrors diff's `existing_schema` cap) but NOT project-contained — the prior sidecar legitimately lives outside `project_dir`. Reads are fail-soft (absent/corrupt/oversize → `None`). diff --git a/docs/airflow-ops.md b/docs/airflow-ops.md index 36616d99..ed66d387 100644 --- a/docs/airflow-ops.md +++ b/docs/airflow-ops.md @@ -591,10 +591,14 @@ load/parse/input/external error (exit 1/2/3) short-circuits both axes per the The comparison is fail-soft, so a drift monitor never breaks the DAG over its own bookkeeping: -- **Missing prior `diff.json`** (the first run) → a non-alarming **baseline** - (`baseline=True`); the task SUCCEEDs and persists today's diff for next time. -- **`model_unique_id` mismatch** or a **corrupt / unreadable / oversize** prior sidecar - → a non-alarming **degraded** report with `degrade_reason` set; the task SUCCEEDs. +- **Missing prior `diff.json`** (the first run), **or a corrupt / unreadable / oversize** + prior sidecar → a non-alarming **baseline** (`baseline=True`); the task SUCCEEDs and + persists today's diff for next time. The prior loader (`load_diff_report`) is fail-soft + and returns `None` for absent *and* malformed input alike, so both collapse to the same + baseline — a corrupt prior is **not** distinguished from a missing one. +- **`model_unique_id` mismatch** between the two diffs → a non-alarming **degraded** report + with `degrade_reason` set; the task SUCCEEDs. (This is the one degrade path that sets + `degrade_reason`; it fires inside `compute_drift`, after both diffs load.) - **Missing `grade.json`** (`--no-grade`) → tier-transition drift is still computed; the grade-regression axis is simply empty. - **Unparseable current diff** (on the generate path) → drift degrades to *no verdict* diff --git a/examples/airflow/signalforge_drift_monitor_dag.py b/examples/airflow/signalforge_drift_monitor_dag.py index a8c4a419..f5bae9a4 100644 --- a/examples/airflow/signalforge_drift_monitor_dag.py +++ b/examples/airflow/signalforge_drift_monitor_dag.py @@ -51,12 +51,14 @@ ## Degrade, never fail (DEC-013) The comparison is fail-soft: a **missing** prior ``diff.json`` (the first run) -makes this run a non-alarming **baseline**; a ``model_unique_id`` mismatch or a -corrupt/unreadable prior sidecar yields a non-alarming **degraded** report -(``degrade_reason`` set) — never an exception. A missing ``grade.json`` -(``--no-grade``) simply leaves the grade-regression axis empty. So a drift -monitor never breaks the DAG over its own bookkeeping; only a *genuine* alarm -trips ``on_drift``. +**or a corrupt/unreadable prior sidecar** makes this run a non-alarming +**baseline** — the prior loader returns ``None`` for absent and malformed input +alike, so a corrupt prior is treated exactly like a missing one (no +``degrade_reason``). A ``model_unique_id`` mismatch yields a non-alarming +**degraded** report (``degrade_reason`` set) — never an exception. A missing +``grade.json`` (``--no-grade``) simply leaves the grade-regression axis empty. +So a drift monitor never breaks the DAG over its own bookkeeping; only a +*genuine* alarm trips ``on_drift``. ## Reproducibility — ``as_of`` @@ -189,8 +191,12 @@ def _config(env_name: str, var_name: str, *, default: str) -> str: # yesterday so the generate task also records a drift summary on its XCom. detect_drift_against=f"{_yesterday_dir}/diff.json", drift_history_dir=_today_dir, - # Record-only here: the generate task always succeeds; the gate is the - # downstream dedicated operator. + # Record-only here: the generate task always succeeds so the downstream + # dedicated operator is the single pageable gate. BOTH policies must be + # `succeed`: `on_drift` for a drift alarm AND `on_flagged` for a flagged + # (below-threshold) generate run — otherwise the default `on_flagged="fail"` + # could fail this task and prevent `drift_check` from ever running. + on_flagged="succeed", on_drift="succeed", ) diff --git a/src/signalforge/airflow/operators.py b/src/signalforge/airflow/operators.py index c9872ec0..e4d5e50c 100644 --- a/src/signalforge/airflow/operators.py +++ b/src/signalforge/airflow/operators.py @@ -261,6 +261,36 @@ def _build_generate_argv( return argv +def _blank_str_to_none(value: str | None) -> str | None: + """Collapse a blank / whitespace-only path to ``None`` (#235 PR review). + + The opt-in drift paths use truthiness as their on/off gate. A + whitespace-only value (``" "``) validates as "off" (blank) but a raw + ``if self.detect_drift_against:`` reads it as "on" at runtime — triggering an + accidental read/write to an unintended path. Normalising blank → ``None`` + keeps construction-time validation and the runtime gate consistent. + """ + if isinstance(value, str): + return value.strip() or None + return value + + +def _validate_grade_regression_threshold(value: object) -> None: + """A non-numeric or negative ``grade_regression_threshold`` is a config error. + + Airflow params / Variables frequently arrive as strings; a string or a + negative value would raise a runtime ``TypeError`` inside ``compute_drift`` + or silently mis-behave ("always regresses"). Fail fast with + :class:`AirflowConfigError` at construction time (#235 PR review). ``bool`` is + rejected explicitly — ``True`` / ``False`` are ``int`` subclasses but never a + valid threshold. + """ + if isinstance(value, bool) or not isinstance(value, (int, float)): + raise AirflowConfigError(f"`grade_regression_threshold` must be a number (got {value!r}).") + if value < 0: + raise AirflowConfigError(f"`grade_regression_threshold` must be >= 0 (got {value!r}).") + + def _validate_operator_config( *, project_dir: str | None, @@ -899,8 +929,9 @@ def __init__( # ``grade_regression_threshold`` is the mean-grade drop that counts # as a regression. When ``detect_drift_against`` is unset, behaviour # is byte-identical to the pre-#235 generate operator. - self.detect_drift_against = detect_drift_against - self.drift_history_dir = drift_history_dir + self.detect_drift_against = _blank_str_to_none(detect_drift_against) + self.drift_history_dir = _blank_str_to_none(drift_history_dir) + _validate_grade_regression_threshold(grade_regression_threshold) self.grade_regression_threshold = grade_regression_threshold # A conn id is NOT a templated value and is deliberately kept OUT of # ``template_fields`` (DEC-007): templating a credential reference is @@ -941,6 +972,10 @@ def execute(self, context: Any) -> dict[str, object]: detect_drift_against=self.detect_drift_against, drift_history_dir=self.drift_history_dir, ) + # A template_field may render to a blank/whitespace value; collapse to + # ``None`` so the truthiness gates below agree with validation (#235 PR review). + self.detect_drift_against = _blank_str_to_none(self.detect_drift_against) + self.drift_history_dir = _blank_str_to_none(self.drift_history_dir) # No ``signalforge_conn_id`` → byte-identical to #232 (DEC-014): no # hook, no credential resolution, no env injection. @@ -1534,6 +1569,7 @@ def __init__( self.previous_grade_path = previous_grade_path self.current_grade_path = current_grade_path self.as_of = as_of + _validate_grade_regression_threshold(grade_regression_threshold) self.grade_regression_threshold = grade_regression_threshold # Annotate the Literal-typed attr explicitly so pyright does NOT widen # it to ``str`` on assignment (which would break the typed diff --git a/tests/airflow/test_operators_helpers.py b/tests/airflow/test_operators_helpers.py index ca19bf7b..f9311d87 100644 --- a/tests/airflow/test_operators_helpers.py +++ b/tests/airflow/test_operators_helpers.py @@ -33,6 +33,7 @@ from signalforge.airflow.errors import AirflowConfigError from signalforge.airflow.operators import ( _aggregate_batch_result, + _blank_str_to_none, _build_drift_inputs, _build_generate_argv, _build_prune_existing_argv, @@ -41,6 +42,7 @@ _provider_key_env, _resolve_select_models, _validate_drift_config, + _validate_grade_regression_threshold, _validate_operator_config, _validate_prune_existing_config, _without_sidecar_paths, @@ -1133,3 +1135,46 @@ def test_provider_key_env_restores_on_exception_prior(monkeypatch: pytest.Monkey 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" + + +# --------------------------------------------------------------------------- # +# _blank_str_to_none + _validate_grade_regression_threshold (#235 PR review) # +# --------------------------------------------------------------------------- # + + +def test_blank_str_to_none_collapses_whitespace_only() -> None: + """A whitespace-only drift path collapses to None (validation/runtime parity).""" + assert _blank_str_to_none(" ") is None + assert _blank_str_to_none("") is None + assert _blank_str_to_none("\t\n") is None + + +def test_blank_str_to_none_preserves_real_values_and_none() -> None: + assert _blank_str_to_none(None) is None + assert _blank_str_to_none("/history/diff.json") == "/history/diff.json" + # Surrounding whitespace is stripped but the value is preserved. + assert _blank_str_to_none(" /history/diff.json ") == "/history/diff.json" + # An un-rendered Jinja template is non-blank and passes through. + assert _blank_str_to_none("{{ ds }}/diff.json") == "{{ ds }}/diff.json" + + +def test_validate_grade_regression_threshold_accepts_valid_numbers() -> None: + for value in (0, 0.0, 0.05, 1, 0.5): + _validate_grade_regression_threshold(value) # must not raise + + +def test_validate_grade_regression_threshold_rejects_non_numeric() -> None: + for bad in ("0.05", None, [0.05]): + with pytest.raises(AirflowConfigError, match="must be a number"): + _validate_grade_regression_threshold(bad) + + +def test_validate_grade_regression_threshold_rejects_bool() -> None: + # bool is an int subclass but is never a valid threshold. + with pytest.raises(AirflowConfigError, match="must be a number"): + _validate_grade_regression_threshold(True) + + +def test_validate_grade_regression_threshold_rejects_negative() -> None: + with pytest.raises(AirflowConfigError, match="must be >= 0"): + _validate_grade_regression_threshold(-0.01)