#171: row_count_anomaly_by_period - #181
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds an end-to-end time-bound row-count anomaly test variant (row_count_anomaly_by_period): draft model + prompts, compiler producing stats+violation SQL, prune engine two-query execution and as_of threading, AnomalyTestStats types, warehouse stats seam, diff emission of singular SQL, audit schema v3, CLI flags, dialect templates, docs, fixtures, and extensive tests. ChangesRow-count anomaly-by-period feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
|
…e arithmetic + percentile)
…e + drift mirror + fixture
…_PROMPT_VERSION rotation Extend DEFAULT_RUBRIC no-redundant criterion with calibration prose for per-period anomaly tests (row_count_anomaly_by_period) per #171 DEC-004 — mirrors #169 DEC-009 + #170 DEC-007 verbatim shape (Option A: extend the existing criterion, no 5th, same +25% LLM-cost reason). The prose teaches the judge to score CALIBRATION of the (method, seasonality, threshold) combination: is it tight enough to catch the failure mode (anomalously small/empty period) but loose enough not to fire on legitimate weekday/weekend or seasonal swings? A worked example pins zscore+threshold=3.0 without seasonality='dow' on a weekday-heavy model as a calibration failure (fires every Sat). Rotation lockstep: - src/signalforge/grade/rubric.py — DEFAULT_RUBRIC no-redundant text + rotation-history comment. - tests/grade/test_rubric.py — verbatim-match test updated; _DEFAULT_RUBRIC_GOLDEN_HASH 30a9fda975b6d45c → a4e3ee92cf9ec36f; new test_no_redundant_criterion_carries_row_count_anomaly_ calibration_prose pins the new prose elements; existing other- criteria preservation tests unchanged. - tests/grade/test_prompts.py — prompt_version_template golden 4dae4421972e9c2d → b1e609fae240ac1c; per-criterion no-redundant hash 7b96cfdfe63bc8bc → b24ff0014a5dcb86 (other 3 unchanged). - tests/grade/test_prompt_cache_stability.py — _EXPECTED_PROMPT_VERSION 4dae4421972e9c2d → b1e609fae240ac1c; _RUBRIC_BLOCK_GOLDEN refreshed with the new no-redundant line. The 3-trigger grade degrade taxonomy (DEC-011 of #169) stays locked — weak anomaly calibration routes through a low criterion score → passed: bool threshold → flagged tier, NOT a 4th degrade trigger. The drafter _PROMPT_VERSION is untouched (US-005 territory). Canonical validation passes: ruff check + ruff format + pyright + 2969 pytest passing.
…xtension + _PROMPT_VERSION rotation
…ariant class + union + drift + fixture + VALID_TEST_TYPES
…yByPeriod variant class
…hema v2→v3 + serializer + fixture update + drift detector
… + audit schema v2→v3
…og + variant detection
…RSION rotation + cache-stability snapshot
Extend _TEST_CATALOGUE_LINES with an 8th entry for
row_count_anomaly_by_period illustrating three JSON shapes (bare default
call; seasonality="dow" for business-calendar grain; explicit method +
threshold override). Add _ROW_COUNT_ANOMALY_SCOPE_INSTRUCTION block to
the SCOPE section teaching:
- propose when projection includes loaded_at / created_at / event_date /
partition_date (incremental fact tables)
- propose seasonality="dow" for business-calendar grain
- default method=mad (median absolute deviation; robust to outliers),
default threshold=3.0, lookback_periods=28, min_samples_per_bucket=3
- method-by-method calibration (zscore for outlier sensitivity,
percentile for Tukey-style IQR, min_max for zero-margin envelope)
Wire _row_count_anomaly_allowed through _render_system_prompt so
exclude_tests=("row_count_anomaly_by_period",) drops both the catalogue
line AND the SCOPE-instruction block (mirrors #163 / #169 / #170
exclude_tests pattern).
_PROMPT_VERSION rotates 389c8aa970df86cc -> a4fea640b3b60f24. Update
_EXPECTED_PROMPT_VERSION + the rotation log in
tests/llm/test_prompt_cache_stability.py. The cached-block golden
(manifest summary) is unchanged — only the system prompt rotated.
Update three pre-existing exclude_tests test cases in
tests/draft/test_exclude_tests.py to include the new variant in their
exclusion tuples so the SCOPE phrase assertions stay valid.
Grade-side _PROMPT_VERSION (US-015) untouched.
… _PROMPT_VERSION rotation
Add row_count_anomaly_by_period arm to model_test_args_hash in src/signalforge/_common/artifact_id.py. Identifying args are the eight scalar/literal fields (method, seasonality, period, lookback_periods, threshold, min_samples_per_bucket, date_column, where) plus column (always None) for shape-parity. rationale is NOT in the hash domain — it is drafter-emitted prose, not identifying (mirrors precedent on every other variant). Per the code comment: all scalar args, no tuples — no sort needed. The #170 unique_combination sort of its columns tuple addresses a semantic order-invariance specific to composite GROUP BY identity; this variant has only scalars + literals, so sorting is N/A. Collision rule: two anomaly tests on the same model differing only by method (or any identifying arg) get distinct artifact_id suffixes via the existing compute_args_hashes disambiguator — verified by 10 new tests in tests/diff/test_artifact_id.py: * 7 distinct-arg rotations (method/seasonality/period/lookback_periods/ threshold/min_samples_per_bucket/date_column/where) * 1 identical-args same-hash regression (guards against spurious hash inputs) * 1 rationale-not-in-hash regression * 1 collision-disambiguator test exercising compute_args_hashes * 1 cross-stage parity (defence-in-depth alongside function identity) Cross-stage parity (function identity across signalforge._common.artifact_id, signalforge.diff._artifact_id, signalforge.grade.engine) is preserved by construction — diff and grade re-export from _common, no edits needed. Scaffolding update: _VARIANTS_PENDING_DISPATCH_ARMS in tests/test_candidate_test_dispatch_exhaustiveness.py converted from a single frozenset to a per-site dict (site number -> frozenset). Site 2 (_common.artifact_id) entry is now empty — exercising CandidateTestRowCountAnomalyByPeriod through the artifact_id hash parametrize. Sites 1/3/5/6 still pending in sibling beads (US-008/US-014/US-006/US-007); site 4 is N/A (no external dbt-macro form). Per-site granularity is load-bearing: it lets a single dispatch-arm bead land without blocking sibling beads on the same variant. Validation: ruff check, ruff format --check, pyright, and the full pytest suite all green (3064 passed, 6 skipped, 79 deselected; 97.73% coverage).
…er-site pending dict
…_compile_violation_query + 8 SQL shapes + partition filter
Adds the row_count_anomaly_by_period compile arm to signalforge.prune.compiler:
- _compile_anomaly_stats_query + _compile_anomaly_violation_query helpers
per DEC-008. Stats: 4 methods (mad/zscore/percentile/min_max) × 2
seasonality (none/dow) = 8 distinct CTE shapes. Violation: one row per
row in today's period (the adapter's COUNT-wrap yields today's count;
US-011 wires the engine-side band comparison).
- Dispatcher arm in _compile_test returns a (stats_sql, violation_sql)
tuple ONLY for this variant; signature broadens to
str | _RequiresFutureData | _InvalidIdentifier | tuple[str, str].
Other variants still return a single string.
- as_of: date | None kwarg threads through _compile_test (keyword-only,
default None; non-anomaly variants ignore). Engine (US-009) resolves
to date.today() before the compile call.
- DEC-011 dialect-driven: every SQL fragment (date_trunc_expr_template,
interval_expr_template, extract_dow_expr_template,
percentile_cont_expr_template, date_literal_template) is read from
Dialect. NEVER branches on dialect.name; the import-guard at
tests/prune/test_compiler_import_guard.py stays green.
- DEC-012 partition filter present in EVERY emitted shape: stats has
>= as_of - INTERVAL <lookback> <unit> AND < as_of (history-only);
violation has >= as_of AND < as_of + INTERVAL 1 <unit> (today-only).
Pinned via byte-exact snapshot fixtures + a shape-pinning regex test.
- DEC-005 compose-then-validate: a hostile where (stray ; / -- / comment
/ unbalanced parens) routes via _InvalidIdentifier (mirrors
row_count_between / unique_combination). date_column safety-checked
via validate_identifier as DEC-013 defence-in-depth.
Fixtures: 16 BigQuery + 16 Snowflake under tests/fixtures/prune/
compiled_sql/anomaly/{bigquery,snowflake}/ (one file per query per
shape). Snowflake fixtures parse cleanly via sqlglot.parse_one(dialect=
'snowflake') under the gated @pytest.mark.snowflake suite.
Cross-story scaffolding (#171 US-003 → US-008):
- tests/test_candidate_test_dispatch_exhaustiveness.py:
_VARIANTS_PENDING_DISPATCH_ARMS frozenset graduates to a PER-SITE
dict (_VARIANTS_PENDING_DISPATCH_ARMS_PER_SITE) keyed by site number
so each bead can mark its own site as landed without waiting for
siblings. site_1 (prune compiler dispatch) is empty post this bead;
site_4 is empty (no external macro for this variant); sites 2/3/5/6
still pending US-004/006/007/014.
Defensive engine arm: prune.engine grows an isinstance(compile_result,
tuple) branch that routes to kept-without-evidence with reason
'row_count_anomaly_by_period two-query split not yet wired in the
engine (#171 US-011 pending)'. This keeps the engine type-correct
under the broadened compiler return type until US-011 lands the
real two-query handling. Mirrors the compiler-side conservative-bias
routing pattern.
Canonical validation passes: ruff check + format, pyright, full
pytest suite (3110 passed, 6 expected skips). Gated Snowflake suite
(uv run pytest -m snowflake --no-cov) passes 52 tests including
all 16 anomaly Snowflake-fixture parse-guards.
…arly-out) Adds the row_count_anomaly_by_period early-continue arm to the model-level loop in signalforge.ingest.anchor.validate_anchor_contract, mirroring the row_count_between and unique_combination exemptions. Without it, the generic 'test.column not in model_columns' check would fire a spurious 'references nonexistent column None' violation on the variant's hardcoded column=None shape. The drafter parser (US-006, sibling bead) remains the anchor-contract authority for date_column shape validation; the ingest anchor's job is column-of-test enforcement only — a defensive test pins this contract boundary so a future refactor cannot inadvertently introduce a cross-layer ingest -> draft coupling. Cross-story scaffolding: removes CandidateTestRowCountAnomalyByPeriod from site 6's _VARIANTS_PENDING_DISPATCH_ARMS entry in tests/test_candidate_test_dispatch_exhaustiveness.py — site 6 now exercises the variant through the routing test.
… + partition filter # Conflicts: # src/signalforge/prune/compiler.py # tests/test_candidate_test_dispatch_exhaustiveness.py
# Conflicts: # tests/test_candidate_test_dispatch_exhaustiveness.py
…te_column + where type-coherence)
…act arm # Conflicts: # tests/test_candidate_test_dispatch_exhaustiveness.py
…ble helper + tighter bypass + DEC-010 behaviour change
…e helper + DEC-010 behaviour change
…gular SQL emission Adds the 3rd of 6 dispatch arms for CandidateTestRowCountAnomalyByPeriod (diff emitter site, per .claude/rules/business-rule-tests.md § 'The 6 production dispatch sites'). Mirrors the custom_sql arm: _render_test returns _SKIP, emit_proposed_test_files surfaces the kept test as a standalone tests/*.sql singular test file. Key design: - _render_test arm in src/signalforge/diff/_emitter.py returns _SKIP for the anomaly variant (between the custom_sql and row_count_between arms). - emit_proposed_test_files extended with keyword-only model / dialect / as_of kwargs. The emitter recompiles the violation query via signalforge.prune.compiler._compile_anomaly_violation_query (US-008 helper) against TableRef.from_model(model) + BIGQUERY_DIALECT default + as_of (defaults: kwarg > decision.as_of > date.today()). - render_diff threads as_of through to the emitter; defaults to date.today() via the per-decision fallback. - Filename uses anchor_to_filename with descriptor 'row_count_anomaly_by_period' (model-level only — no column prefix), hashed via the shared _common.artifact_id seam. - The fail-closed _test_file_writer.write_test_file is variant-agnostic; no changes there. - Pinned by snapshot fixture tests/fixtures/diff/proposed_test_files/anomaly/. Cross-story scaffolding (per US-004 convention): removes CandidateTestRowCountAnomalyByPeriod from site 3 in _VARIANTS_PENDING_DISPATCH_ARMS — the routing test now exercises the variant on the diff emitter site. Site 5 (drafter anchor) remains pending in US-006. Plan: plans/super/171-row-count-anomaly.md § US-014 + Phase 1 B.7 (locked: singular SQL only — no dbt-ext macro form for this primitive). Tests: - tests/diff/test_emitter.py: 11 new tests covering _SKIP routing, basic path/marker, body == violation_query, args_hash filename, dropped-decision exclusion, as_of resolution (decision vs kwarg vs today), fail-loud-without-model, custom_sql co-emission, snapshot fixture pin. - tests/test_candidate_test_dispatch_exhaustiveness.py: site 3 pending set emptied; 53 dispatch tests pass. Validation: ruff check / ruff format --check / pyright / pytest all green (3159 passed; coverage 97.72%).
…ute for singular SQL # Conflicts: # tests/test_candidate_test_dispatch_exhaustiveness.py
… + prune-existing + 5-surface parity Add --as-of YYYY-MM-DD flag to both 'signalforge generate' and 'signalforge prune-existing' per #171 DEC-001. type=date.fromisoformat parses strict ISO; bad format raises argparse SystemExit(2) which maps cleanly to tier-2 input-validation. Default None lets prune_tests resolve to date.today() at prune time (the resolution belongs in the engine, NOT the CLI, so PruneEvent.as_of records the same resolved value across callers). Thread args.as_of -> prune_tests(as_of=...) in both cmd_generate's _run_single_model AND cmd_prune_existing. In multi-model batch (--select), the same as_of value flows to every per-model call (_run_batch invokes _run_single_model per match with the shared args namespace, so the operator's single --as-of applies uniformly). 5-surface parity per cli-layer.md: 1. argparse help= string (both subcommands) 2. cmd_generate / cmd_prune_existing handler docstrings + add_parser docstrings 3. docs/cli-ops.md Flag reference: bullet under generate, table row under prune-existing 4. test names (per-subcommand) 5. DEC reference: #171 DEC-001 (already in plan) Tests pin: parses cleanly, default None, bad format -> exit 2 + no-traceback floor, threads to engine kwarg, multi-model batch uses one as_of value across all models. test_flag_defaults parametrize grows an entry; help-lists-every-flag grows --as-of. US-010 (engine helper) is in flight in a sibling worktree -- this slice is purely additive on the CLI side; engine.py untouched.
…+ 5-surface parity
…tart routing + AnomalyTestStats wiring + DOW degrade Replaces the defensive 'two-query split not yet wired' arm in signalforge.prune.engine with real handling of the (stats_sql, violation_sql) tuple compiler arm landed by US-008 for the row_count_anomaly_by_period variant. Engine wiring per #171 DEC-001 / DEC-003 / DEC-005 / DEC-006 / DEC-008: 1. Stats query (Query 1) runs via adapter.run_stats_query — a new vendor-neutral seam on WarehouseAdapter (ABC default raises StatsQueryNotSupportedError for adapters that have not grown the primitive; BigQuery overrides). Result rows parse into the typed AnomalyTestStats discriminated-union member matching test.method via _parse_anomaly_stats (handles all four methods + seasonal per-DOW shape). 2. Cold-start gate: stats.n_periods < min_samples_per_bucket routes to kept-without-evidence with structured why ('insufficient history: N/M periods'). Query 2 (violation) is SKIPPED — no warehouse call. stats populated on decision + audit. 3. DOW degrade: when seasonality=dow AND any per-DOW bucket below floor, recompile the stats query with seasonality=none, emit ONE operator-actionable WARNING (lazy-format JSON), proceed with the non-seasonal stats. Violation SQL is unchanged (today's bucket is today's bucket regardless of seasonality). 4. Violation query (Query 2) runs via standard adapter.run_test_sql; PruneDecision carries stats + failures per the standard routing matrix (always-passes, failed-on-known-clean-data, kept). All three anomaly routing paths populate PruneDecision.stats and (via _build_prune_event) PruneEvent.stats — audit-of-record per DEC-006 + DEC-013. Conservative-bias contract preserved: a WarehouseError from either query (incl. StatsQueryNotSupportedError from a no-primitive adapter) routes to kept-without-evidence; the DropReason Literal stays 5-valued (never grow). New seams: - WarehouseAdapter.run_stats_query(sql) -> tuple[dict[str, object], ...] + StatsQueryNotSupportedError (tier 3, ABC default raise pattern) - BigQueryAdapter.run_stats_query override (client.query verbatim with the same map_bq_exception path used by run_test_sql). - _parse_anomaly_stats / _decide_anomaly_cold_start / _any_dow_bucket_thin helpers in signalforge.prune.engine. - _decide_from_test_result + _decide_kept_without_evidence_warehouse_error thread stats + as_of through to PruneDecision. Tests (tests/prune/test_engine.py § '#171 US-011' block): - Happy path: both queries run, stats populated, decision routes. - Cold-start: violation query SKIPPED (pinned by fake adapter's assert_all_expectations_met — no unmet violation expectation). - DOW degrade: thin per-DOW bucket triggers recompile + WARNING + non-seasonal proceed; healthy DOW path skips degrade entirely. - stats populated on PruneEvent audit (happy-path drop + cold-start kept-without-evidence both pinned via JSONL readback). - StatsQueryNotSupportedError routes through standard WarehouseError catch surface. - Defensive 'US-011 pending' arm verified replaced (never fires on the happy path). Two pre-existing US-010 routing tests updated to queue stats + violation expectations now that the engine actually issues them (previously they pinned the defensive arm). Standard validation green (3177 tests, 97.35% coverage). Refs #171 US-011, DEC-001, DEC-003, DEC-005, DEC-006, DEC-008, DEC-010, DEC-013.
…old-start + DOW degrade + AnomalyTestStats wiring
…ue + prune-ops + cli-ops + SKILL.md + CHANGELOG
…nject_model_anomaly_rules helper US-017 adds two test surfaces for the row_count_anomaly_by_period variant and the inject_model_anomaly_rules e2e helper: 1. Unit determinism (tests/prune/test_engine.py): runs prune_tests twice with as_of=date(2026, 5, 1) and asserts PruneEvent.compiled_sql is byte-equal across runs; a third run with as_of=date(2026, 5, 2) asserts the compiled SQL differs (proves the as_of threading from engine to compiler is real). Fresh fake adapter + audit path per run per the expect_query consumption model. Pinned via the typed PruneEvent.model_validate path off the audit JSONL. Traces DEC-001. 2. inject_model_anomaly_rules (tests/cli/_e2e_helpers.py): thin specialisation of inject_model_business_rules — same on-disk mutation surface (config.meta.signalforge.business_rules + meta.signalforge.business_rules in lockstep), distinct name so the anomaly e2e reads self-documentingly. Mirrors the precedent the #169 / #170 e2e helpers set for variant steering via prose rules. 3. E2E gated (tests/cli/test_e2e_row_count_anomaly.py): runs signalforge generate --as-of 2023-04-16 against the Austin bikeshare fixture (the documented ~50% volume drop date per #171 plan §Refinement Q9). Belt-and-suspenders gating per testing-signal.md — @pytest.mark.e2e + @pytest.mark.anthropic + @pytest.mark.bigquery + runtime _skip_reason() for the standard three env vars (SF_RUN_BQ, ANTHROPIC_API_KEY, GOOGLE_CLOUD_PROJECT). tmp_path isolation per testing-signal.md — committed fixture untouched; manifest injection writes only to the per-run copy. Asserts: exit 0; diff sidecar present; structured row_count_anomaly_by_period PruneDecision exists; PruneEvent.as_of == 2023-04-16; AnomalyTestStats populated with valid method + n_periods >= 1; decision='kept' (real anomaly caught); no traceback in stderr. Traces DEC-001, DEC-003, DEC-008, DEC-013. Validation: full canonical command passes (ruff check / ruff format check / pyright / pytest); 3205 passed, 6 intentional skips, 96 gated-deselected; coverage 97.36%. Per-task constraints honoured: tests only — no src/ changes; no dispatch-arm work (all six sites already wired); engineered determinism (unit fake stats + hand-picked anomaly date) per testing-signal.md.
…rminism + inject_model_anomaly_rules helper
wjduenow
marked this pull request as ready for review
June 1, 2026 18:59
wjduenow
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
docs/prune-ops.md (1)
414-415: 💤 Low valueUse fenced code blocks for consistency.
The partition-filter WHERE clause example uses indentation instead of fenced code blocks (triple backticks). For consistency with the rest of the document and to satisfy the markdownlint rule, please fence this code block.
📝 Proposed formatting fix
- <date_column> >= <as_of> - INTERVAL <lookback_periods> <period> - AND <date_column> < <as_of> + INTERVAL 1 <period> +``` +<date_column> >= <as_of> - INTERVAL <lookback_periods> <period> +AND <date_column> < <as_of> + INTERVAL 1 <period> +```As per coding guidelines, markdownlint flags indented code blocks (MD046).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/prune-ops.md` around lines 414 - 415, Replace the indented SQL snippet using <date_column>, <as_of>, <lookback_periods>, and <period> with a fenced code block: surround the two lines " <date_column> >= <as_of> - INTERVAL <lookback_periods> <period>" and "AND <date_column> < <as_of> + INTERVAL 1 <period>" with triple backticks (```) above and below, remove the leading indentation/plus signs, and ensure no extra characters inside the fenced block so it conforms to markdownlint MD046.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/rules/business-rule-tests.md:
- Line 142: The line starting with "`#171` (`row_count_anomaly_by_period`)" is
being parsed as an ATX heading and triggers MD018; edit that sentence to avoid
an initial lone '#' by changing the token (for example, replace "`#171`" with
"Issue `#171`" or "PR `#171`") so the text in the file business-rule-tests.md no
longer begins with an unspaced ATX marker; update the same line containing
`row_count_anomaly_by_period` accordingly to preserve the meaning.
In @.claude/rules/prune-engine.md:
- Line 180: The line beginning with "`#171 introduced
_test_requires_source_table(...)" is being parsed as a markdown ATX heading;
update the sentence in .claude/rules/prune-engine.md so it does not start with
an unescaped hash — e.g., prefix with "Issue " or write "Issue `#171` introduced
..." or escape the hash ("\`#171`") — leaving the rest of the description intact
and still referencing the helper function _test_requires_source_table in
signalforge.prune.engine.
In `@src/signalforge/ingest/anchor.py`:
- Around line 91-100: The current early "continue" for tests with test.type ==
"row_count_anomaly_by_period" skips validation of test.date_column, allowing an
invalid/stale date_column to slip through during re-ingest (e.g.,
prune-existing); update the anchor ingest loop in
src/signalforge/ingest/anchor.py so that after detecting
row_count_anomaly_by_period you still validate date_column against the model's
columns (model_columns) if date_column is not None — mirror the column-of-test
enforcement used elsewhere (e.g., unique_combination’s iteration over
test.columns) and, where possible, reuse or invoke the existing
validate_anchor_contract/date_column validation logic (or call the same helper
used by validate_anchor_contract) to produce the proper per-column error rather
than silently continuing.
In `@src/signalforge/prune/compiler.py`:
- Around line 1056-1062: The window predicate builder uses the raw as_of value
which can be misaligned for perioded windows; update the logic in the function
that builds the predicate (the code that computes as_of_literal, unit via
_period_unit_keyword(period), lookback_interval via
dialect.interval_expr_template, and returns the date range using
date_column_quoted) to first normalize or validate as_of to the period boundary
(e.g., compute an anchor using DATE_TRUNC/period-truncation or raise if as_of is
not aligned) and then produce as_of_literal and lookback_interval from that
normalized anchor; apply the same normalization/validation to the equivalent
window-building site that mirrors this logic so both history and violation
predicates use the same anchored as_of.
In `@tests/prune/test_engine.py`:
- Around line 3972-4043: The test is flaky because it calls _date.today() twice
after the call to prune_tests (for today_iso and captured["as_of"]) which can
cross midnight; fix by capturing the resolved date once before invoking
prune_tests (e.g. today = _date.today()) and then use that single today variable
for today_iso, for comparing the log payload, and for asserting
captured["as_of"] inside
test_as_of_none_with_anomaly_candidate_resolves_to_today_and_logs so all
comparisons use the same stable date value.
---
Nitpick comments:
In `@docs/prune-ops.md`:
- Around line 414-415: Replace the indented SQL snippet using <date_column>,
<as_of>, <lookback_periods>, and <period> with a fenced code block: surround the
two lines " <date_column> >= <as_of> - INTERVAL <lookback_periods> <period>" and
"AND <date_column> < <as_of> + INTERVAL 1 <period>" with triple backticks (```)
above and below, remove the leading indentation/plus signs, and ensure no extra
characters inside the fenced block so it conforms to markdownlint MD046.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4ce79f5-b270-4577-9920-c24c23510955
📒 Files selected for processing (96)
.claude/rules/business-rule-tests.md.claude/rules/prune-engine.mdCHANGELOG.mdREADME.mddocs/cli-ops.mddocs/drafter-catalogue.mddocs/prune-ops.mdplans/super/171-row-count-anomaly.mdsrc/signalforge/_common/artifact_id.pysrc/signalforge/cli/_helpers.pysrc/signalforge/cli/generate.pysrc/signalforge/cli/prune_existing.pysrc/signalforge/diff/_emitter.pysrc/signalforge/diff/engine.pysrc/signalforge/draft/config.pysrc/signalforge/draft/models.pysrc/signalforge/draft/parser.pysrc/signalforge/draft/prompts.pysrc/signalforge/grade/rubric.pysrc/signalforge/ingest/anchor.pysrc/signalforge/prune/__init__.pysrc/signalforge/prune/audit.pysrc/signalforge/prune/compiler.pysrc/signalforge/prune/engine.pysrc/signalforge/prune/models.pysrc/signalforge/prune/stats.pysrc/signalforge/skills/signalforge/SKILL.mdsrc/signalforge/warehouse/__init__.pysrc/signalforge/warehouse/adapters/bigquery.pysrc/signalforge/warehouse/base.pysrc/signalforge/warehouse/errors.pysrc/signalforge/warehouse/models.pytests/cli/_e2e_helpers.pytests/cli/test_e2e_row_count_anomaly.pytests/cli/test_generate.pytests/cli/test_generate_batch.pytests/cli/test_prune_existing.pytests/diff/test_artifact_id.pytests/diff/test_emitter.pytests/draft/test_drift_detector.pytests/draft/test_exclude_tests.pytests/draft/test_models.pytests/draft/test_parser.pytests/draft/test_prompts.pytests/fixtures/diff/proposed_test_files/anomaly/orders__row_count_anomaly_by_period_8e4d6245.sqltests/fixtures/draft/candidate_schema_v1.jsontests/fixtures/prune/anomaly_stats_v1.jsontests/fixtures/prune/compiled_sql/anomaly/bigquery/mad_dow_stats.sqltests/fixtures/prune/compiled_sql/anomaly/bigquery/mad_dow_violation.sqltests/fixtures/prune/compiled_sql/anomaly/bigquery/mad_none_stats.sqltests/fixtures/prune/compiled_sql/anomaly/bigquery/mad_none_violation.sqltests/fixtures/prune/compiled_sql/anomaly/bigquery/min_max_dow_stats.sqltests/fixtures/prune/compiled_sql/anomaly/bigquery/min_max_dow_violation.sqltests/fixtures/prune/compiled_sql/anomaly/bigquery/min_max_none_stats.sqltests/fixtures/prune/compiled_sql/anomaly/bigquery/min_max_none_violation.sqltests/fixtures/prune/compiled_sql/anomaly/bigquery/percentile_dow_stats.sqltests/fixtures/prune/compiled_sql/anomaly/bigquery/percentile_dow_violation.sqltests/fixtures/prune/compiled_sql/anomaly/bigquery/percentile_none_stats.sqltests/fixtures/prune/compiled_sql/anomaly/bigquery/percentile_none_violation.sqltests/fixtures/prune/compiled_sql/anomaly/bigquery/zscore_dow_stats.sqltests/fixtures/prune/compiled_sql/anomaly/bigquery/zscore_dow_violation.sqltests/fixtures/prune/compiled_sql/anomaly/bigquery/zscore_none_stats.sqltests/fixtures/prune/compiled_sql/anomaly/bigquery/zscore_none_violation.sqltests/fixtures/prune/compiled_sql/anomaly/snowflake/mad_dow_stats.sqltests/fixtures/prune/compiled_sql/anomaly/snowflake/mad_dow_violation.sqltests/fixtures/prune/compiled_sql/anomaly/snowflake/mad_none_stats.sqltests/fixtures/prune/compiled_sql/anomaly/snowflake/mad_none_violation.sqltests/fixtures/prune/compiled_sql/anomaly/snowflake/min_max_dow_stats.sqltests/fixtures/prune/compiled_sql/anomaly/snowflake/min_max_dow_violation.sqltests/fixtures/prune/compiled_sql/anomaly/snowflake/min_max_none_stats.sqltests/fixtures/prune/compiled_sql/anomaly/snowflake/min_max_none_violation.sqltests/fixtures/prune/compiled_sql/anomaly/snowflake/percentile_dow_stats.sqltests/fixtures/prune/compiled_sql/anomaly/snowflake/percentile_dow_violation.sqltests/fixtures/prune/compiled_sql/anomaly/snowflake/percentile_none_stats.sqltests/fixtures/prune/compiled_sql/anomaly/snowflake/percentile_none_violation.sqltests/fixtures/prune/compiled_sql/anomaly/snowflake/zscore_dow_stats.sqltests/fixtures/prune/compiled_sql/anomaly/snowflake/zscore_dow_violation.sqltests/fixtures/prune/compiled_sql/anomaly/snowflake/zscore_none_stats.sqltests/fixtures/prune/compiled_sql/anomaly/snowflake/zscore_none_violation.sqltests/fixtures/prune/prune_event_v1.jsonltests/grade/test_prompt_cache_stability.pytests/grade/test_prompts.pytests/grade/test_rubric.pytests/ingest/test_anchor.pytests/llm/test_prompt_cache_stability.pytests/prune/test_audit.pytests/prune/test_compiler.pytests/prune/test_compiler_fakesnow.pytests/prune/test_drift_detector.pytests/prune/test_engine.pytests/prune/test_models.pytests/prune/test_smoke.pytests/prune/test_stats.pytests/test_candidate_test_dispatch_exhaustiveness.pytests/warehouse/test_errors.pytests/warehouse/test_models.py
There was a problem hiding this comment.
Pull request overview
Adds the planned row_count_anomaly_by_period primitive and its supporting plumbing across draft → prune → diff → grade, including a new time-bound --as-of evaluation date and typed anomaly-stats handoff for audit/grade/diff.
Changes:
- Introduces
AnomalyTestStats(discriminated union) + audit/prune model fields (as_of,stats) and bumps prune audit schema to v3. - Extends warehouse dialect + adapter surface to support anomaly stats queries (
Dialectdate/percentile templates,WarehouseAdapter.run_stats_query, BigQuery implementation, new typed error). - Updates drafter prompts/parsing, diff emission for singular anomaly SQL files, rubric calibration prose, CLI/docs/fixtures/tests.
Reviewed changes
Copilot reviewed 96 out of 96 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/warehouse/test_models.py | Adds coverage for new Dialect date/percentile fields and POSTGRES inheritance behavior. |
| tests/warehouse/test_errors.py | Updates exported error count + fixture map for StatsQueryNotSupportedError. |
| tests/prune/test_stats.py | New unit tests for AnomalyTestStats union and per-DOW serialization behavior. |
| tests/prune/test_smoke.py | Ensures new anomaly stats types are part of the prune public API. |
| tests/prune/test_models.py | Verifies PruneDecision repr / repr_args redact as_of and stats. |
| tests/prune/test_drift_detector.py | Extends strict drift mirrors to include as_of/stats and anomaly-stats strict unions. |
| tests/prune/test_compiler_fakesnow.py | Adds sqlglot parse-guards for Snowflake anomaly SQL fixtures. |
| tests/prune/test_audit.py | Bumps expected audit schema to v3 and adds serialization/replay tests for as_of + stats. |
| tests/llm/test_prompt_cache_stability.py | Rotates drafter _PROMPT_VERSION for new catalogue entry + scope instruction. |
| tests/ingest/test_anchor.py | Pins ingest anchor contract exemption for model-level row_count_anomaly_by_period. |
| tests/grade/test_rubric.py | Extends no-redundant criterion prose for anomaly calibration and updates golden hashes. |
| tests/grade/test_prompts.py | Updates grade prompt-version expectations + criterion hash for updated rubric text. |
| tests/grade/test_prompt_cache_stability.py | Updates grade _PROMPT_VERSION pin + rubric-block golden text. |
| tests/fixtures/prune/prune_event_v1.jsonl | Updates prune audit fixture rows to schema v3 (adds as_of, stats). |
| tests/fixtures/prune/compiled_sql/anomaly/snowflake/zscore_none_violation.sql | Adds Snowflake anomaly “violation” SQL snapshot fixture. |
| tests/fixtures/prune/compiled_sql/anomaly/snowflake/zscore_none_stats.sql | Adds Snowflake anomaly “stats” SQL snapshot fixture. |
| tests/fixtures/prune/compiled_sql/anomaly/snowflake/zscore_dow_violation.sql | Adds Snowflake anomaly “violation” SQL snapshot fixture (DOW). |
| tests/fixtures/prune/compiled_sql/anomaly/snowflake/zscore_dow_stats.sql | Adds Snowflake anomaly “stats” SQL snapshot fixture (DOW). |
| tests/fixtures/prune/compiled_sql/anomaly/snowflake/percentile_none_violation.sql | Adds Snowflake anomaly “violation” SQL snapshot fixture. |
| tests/fixtures/prune/compiled_sql/anomaly/snowflake/percentile_none_stats.sql | Adds Snowflake anomaly “stats” SQL snapshot fixture. |
| tests/fixtures/prune/compiled_sql/anomaly/snowflake/percentile_dow_violation.sql | Adds Snowflake anomaly “violation” SQL snapshot fixture (DOW). |
| tests/fixtures/prune/compiled_sql/anomaly/snowflake/percentile_dow_stats.sql | Adds Snowflake anomaly “stats” SQL snapshot fixture (DOW). |
| tests/fixtures/prune/compiled_sql/anomaly/snowflake/min_max_none_violation.sql | Adds Snowflake anomaly “violation” SQL snapshot fixture. |
| tests/fixtures/prune/compiled_sql/anomaly/snowflake/min_max_none_stats.sql | Adds Snowflake anomaly “stats” SQL snapshot fixture. |
| tests/fixtures/prune/compiled_sql/anomaly/snowflake/min_max_dow_violation.sql | Adds Snowflake anomaly “violation” SQL snapshot fixture (DOW). |
| tests/fixtures/prune/compiled_sql/anomaly/snowflake/min_max_dow_stats.sql | Adds Snowflake anomaly “stats” SQL snapshot fixture (DOW). |
| tests/fixtures/prune/compiled_sql/anomaly/snowflake/mad_none_violation.sql | Adds Snowflake anomaly “violation” SQL snapshot fixture. |
| tests/fixtures/prune/compiled_sql/anomaly/snowflake/mad_none_stats.sql | Adds Snowflake anomaly “stats” SQL snapshot fixture. |
| tests/fixtures/prune/compiled_sql/anomaly/snowflake/mad_dow_violation.sql | Adds Snowflake anomaly “violation” SQL snapshot fixture (DOW). |
| tests/fixtures/prune/compiled_sql/anomaly/snowflake/mad_dow_stats.sql | Adds Snowflake anomaly “stats” SQL snapshot fixture (DOW). |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/zscore_none_violation.sql | Adds BigQuery anomaly “violation” SQL snapshot fixture. |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/zscore_none_stats.sql | Adds BigQuery anomaly “stats” SQL snapshot fixture. |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/zscore_dow_violation.sql | Adds BigQuery anomaly “violation” SQL snapshot fixture (DOW). |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/zscore_dow_stats.sql | Adds BigQuery anomaly “stats” SQL snapshot fixture (DOW). |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/percentile_none_violation.sql | Adds BigQuery anomaly “violation” SQL snapshot fixture. |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/percentile_none_stats.sql | Adds BigQuery anomaly “stats” SQL snapshot fixture. |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/percentile_dow_violation.sql | Adds BigQuery anomaly “violation” SQL snapshot fixture (DOW). |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/percentile_dow_stats.sql | Adds BigQuery anomaly “stats” SQL snapshot fixture (DOW). |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/min_max_none_violation.sql | Adds BigQuery anomaly “violation” SQL snapshot fixture. |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/min_max_none_stats.sql | Adds BigQuery anomaly “stats” SQL snapshot fixture. |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/min_max_dow_violation.sql | Adds BigQuery anomaly “violation” SQL snapshot fixture (DOW). |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/min_max_dow_stats.sql | Adds BigQuery anomaly “stats” SQL snapshot fixture (DOW). |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/mad_none_violation.sql | Adds BigQuery anomaly “violation” SQL snapshot fixture. |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/mad_none_stats.sql | Adds BigQuery anomaly “stats” SQL snapshot fixture. |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/mad_dow_violation.sql | Adds BigQuery anomaly “violation” SQL snapshot fixture (DOW). |
| tests/fixtures/prune/compiled_sql/anomaly/bigquery/mad_dow_stats.sql | Adds BigQuery anomaly “stats” SQL snapshot fixture (DOW). |
| tests/fixtures/prune/anomaly_stats_v1.json | Adds canonical JSON fixture rows for all anomaly methods + a seasonal entry. |
| tests/fixtures/draft/candidate_schema_v1.json | Extends candidate schema fixture with a row_count_anomaly_by_period entry. |
| tests/fixtures/diff/proposed_test_files/anomaly/orders__row_count_anomaly_by_period_8e4d6245.sql | Adds proposed singular test-file fixture for anomaly variant. |
| tests/draft/test_prompts.py | Adds tests asserting new system-prompt catalogue + scope instruction behavior. |
| tests/draft/test_parser.py | Adds anchor-contract validation tests for row_count_anomaly_by_period. |
| tests/draft/test_exclude_tests.py | Updates valid test type set + scope-line behavior for exclude-tests feature. |
| tests/draft/test_drift_detector.py | Adds strict drift mirror model for CandidateTestRowCountAnomalyByPeriod. |
| tests/diff/test_artifact_id.py | Adds args-hash domain tests and cross-stage parity for anomaly variant IDs. |
| tests/cli/test_prune_existing.py | Adds --as-of parsing/help/forwarding tests for prune-existing. |
| tests/cli/test_generate.py | Adds --as-of parsing/help/forwarding tests for generate. |
| tests/cli/test_generate_batch.py | Pins --as-of behavior across multi-model --select batches. |
| tests/cli/_e2e_helpers.py | Adds helper to inject anomaly-focused business rules into manifest fixtures. |
| src/signalforge/warehouse/models.py | Adds 5 new date/percentile SQL templates to Dialect and Snowflake overrides. |
| src/signalforge/warehouse/errors.py | Introduces StatsQueryNotSupportedError and exports it. |
| src/signalforge/warehouse/base.py | Adds WarehouseAdapter.run_stats_query default (typed degrade). |
| src/signalforge/warehouse/adapters/bigquery.py | Implements run_stats_query for BigQuery with safety checks and error mapping. |
| src/signalforge/warehouse/init.py | Re-exports StatsQueryNotSupportedError. |
| src/signalforge/skills/signalforge/SKILL.md | Updates documented test catalogue (7→8) and adds --as-of flag note. |
| src/signalforge/prune/stats.py | Adds typed anomaly-stats value objects + discriminated union for cross-stage stats. |
| src/signalforge/prune/models.py | Adds as_of + stats to PruneDecision with serializers and repr redaction. |
| src/signalforge/prune/audit.py | Bumps audit schema v2→v3; adds as_of + stats fields + serializers to PruneEvent. |
| src/signalforge/prune/init.py | Exposes anomaly-stats types in prune package public API. |
| src/signalforge/ingest/anchor.py | Exempts row_count_anomaly_by_period from model-level column membership enforcement. |
| src/signalforge/grade/rubric.py | Extends no-redundant criterion prose to include anomaly calibration guidance. |
| src/signalforge/draft/prompts.py | Adds prompt catalogue example lines + SCOPE instruction block for anomaly variant. |
| src/signalforge/draft/parser.py | Adds anchor-contract validation for date_column and where on anomaly variant. |
| src/signalforge/draft/models.py | Adds CandidateTestRowCountAnomalyByPeriod Pydantic model + validation + redacted repr. |
| src/signalforge/draft/config.py | Adds anomaly test type to VALID_TEST_TYPES and config documentation. |
| src/signalforge/diff/engine.py | Threads as_of into proposed test-file emission for anomaly variant. |
| src/signalforge/diff/_emitter.py | Emits anomaly tests as singular SQL files (skips YAML) and recompiles SQL with as_of. |
| src/signalforge/cli/prune_existing.py | Adds --as-of CLI flag and forwards it to prune_tests. |
| src/signalforge/cli/generate.py | Adds --as-of CLI flag and forwards it to prune_tests (single + batch). |
| src/signalforge/cli/_helpers.py | Maps StatsQueryNotSupportedError to exit code tier 3. |
| src/signalforge/_common/artifact_id.py | Adds anomaly variant identifying args to args-hash computation. |
| README.md | Updates “What tests SignalForge generates” to include the 8th variant + time-bound note. |
| docs/prune-ops.md | Documents anomaly variant evaluation contract (two-query split, as-of, partition filter, degrade). |
| docs/drafter-catalogue.md | Adds anomaly variant to catalogue and updates “out of scope” boundaries. |
| docs/cli-ops.md | Documents --as-of semantics and adds it to the flag reference table. |
| CHANGELOG.md | Adds release notes for anomaly variant + --as-of and notes sampling behavior change. |
| .claude/rules/prune-engine.md | Updates internal prune-engine rules for anomaly routing, audit schema bump, and new seam. |
| .claude/rules/business-rule-tests.md | Updates variant-extension precedent docs to include anomaly primitive and as-of carve-out. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- #1, #2 (CodeRabbit MD018): prefix #171 → 'Issue #171' in rule files so markdownlint doesn't parse the bare hash as an ATX heading - #5 (CodeRabbit flake): bracket date.today() resolution within [today_before, today_after] in the as_of-resolves-to-today test to survive midnight crossings - #6, #7 (Copilot doc lies): correct 'IQR multiplier' / 'Tukey-style IQR band' wording for percentile method — actual implementation is half-band width in percentile points (p_lo = threshold/100) - #10 (Copilot CRITICAL): _parse_anomaly_stats now normalises raw dialect-emitted DOW integers to POSIX (Mon=0..Sun=6) via the new _normalize_dow_to_posix(raw, dialect) helper. Without this, BQ's DAYOFWEEK (1..7, Sun=1) and Snowflake's DOW (0..6, Sun=0) would produce mutually-incompatible per_dow dict keys, breaking any consumer that does dict.get(date.weekday()) Drafter _PROMPT_VERSION rotated to c11a73cc95b31614 (the percentile wording fix touched _SYSTEM_PROMPT). Validation: 3205 passed, 0 ruff / pyright errors. Pending fixes from same review pass (next commit): - #3 (CodeRabbit confirmation reply on date_column validation) - #4 (period boundary for week/hour as_of) - #8, #9 (Copilot CRITICAL: emitted singular test SQL needs full band-check shape, not just violation query)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/llm/test_prompt_cache_stability.py`:
- Line 97: The test's expected prompt version constant _EXPECTED_PROMPT_VERSION
was updated to "c11a73cc95b31614" but the rotation history above still marks
"a4fea640b3b60f24" as current; update the rotation list to add a new entry for
"c11a73cc95b31614" as the current/pinned hash and demote "a4fea640b3b60f24" to a
prior rotation so the rotation history aligns with the _EXPECTED_PROMPT_VERSION
constant in test_prompt_cache_stability.py.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5e09c3ed-e83f-43c0-8318-3b9470370a97
📒 Files selected for processing (7)
.claude/rules/business-rule-tests.md.claude/rules/prune-engine.mdsrc/signalforge/draft/models.pysrc/signalforge/draft/prompts.pysrc/signalforge/prune/engine.pytests/llm/test_prompt_cache_stability.pytests/prune/test_engine.py
✅ Files skipped from review due to trivial changes (2)
- .claude/rules/prune-engine.md
- .claude/rules/business-rule-tests.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/signalforge/draft/models.py
- src/signalforge/draft/prompts.py
- src/signalforge/prune/engine.py
- tests/prune/test_engine.py
…iod boundary) - #4 (CodeRabbit): _render_as_of_literal wraps the as_of date literal in DATE_TRUNC(<lit>, <unit>) for period in {week, hour} so the today-window aligns to the natural period boundary. period=day is a no-op (date literal is already at day-boundary 00:00:00); existing day-period snapshots stay byte-equal. Documented hour-period limit: DATE_TRUNC of a DATE is dialect-divergent for HOUR. - #8, #9 (Copilot CRITICAL): new _compile_anomaly_singular_test_sql helper emits the FULL band-check SQL — history + per-method stats CTEs + today CTE + band-violation WHERE predicate. Returns 0 rows when in-band and >=1 row only when out-of-band (the correct dbt singular-test contract). The prior emitted SQL was just _compile_anomaly_violation_query, which returns ALL rows in the as_of period and fails the dbt test on every non-empty day. Per-method band predicates (all 4 covered): * mad: ABS(0.6745 * (today.cnt - stats.median)) > k * NULLIF(stats.mad, 0) * zscore: ABS(today.cnt - stats.mean) > k * NULLIF(stats.stddev, 0) * percentile: today.cnt < stats.p_lo OR today.cnt > stats.p_hi * min_max: today.cnt < stats.min_cnt OR today.cnt > stats.max_cnt Seasonal (seasonality=dow): today CTE projects MAX(EXTRACT(DOW)) too; JOIN against per-DOW stats so band check fires only for today's DOW. Diff emitter wired to call the new helper instead of the engine-side violation query. Snapshot fixture updated. 13 new tests covering the per-method predicates, seasonal JOIN, period truncation, where-clause symmetry, min_max threshold-ignored, percentile half-band-width semantics, Snowflake dialect fragment usage. Validation: 3218 passed, 0 ruff / pyright errors. Pending: #3 (CodeRabbit confirmation reply — just a comment), summary comment, resolve all 10 review threads.
Owner
Author
PR Review SummaryAddressed all 10 review comments from CodeRabbit + Copilot. Two commits: Fixed (9 items)
Documented as intentional design (1 item)
QG gap acknowledgedMy pre-review Quality Gate (4 reviewer-angle passes per memory Validation
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/signalforge/diff/_emitter.py`:
- Around line 452-463: The emission path directly calls
_compile_anomaly_singular_test_sql (skipping date_column validation and
validate_test_sql) which allows malformed/kept-without-evidence anomalies to be
emitted; before emitting the SQL from _compile_anomaly_singular_test_sql, invoke
the same safety checks used by _compile_row_count_anomaly_by_period — validate
the date_column and run validate_test_sql (or call the shared pre-compile
validation routine if one exists) for the given test/model/resolved_as_of, and
only proceed to call _compile_anomaly_singular_test_sql when those checks pass,
failing fast and logging/raising if validation fails.
In `@src/signalforge/prune/compiler.py`:
- Around line 1320-1325: The today CTE currently computes dow using the filtered
date column (date_column_quoted) which yields NULL when no rows exist, causing
stats.dow = today.dow to drop all rows; change the include_dow branch so the dow
expression is derived from the anchored as_of value instead of the table's date
column: use
dialect.extract_dow_expr_template.format(date=as_of_quoted_or_literal) (or an
as_of reference used elsewhere) to build dow_expr and keep select_list as
"COUNT(*) AS cnt, MAX({dow_expr}) AS dow" so the seasonal key is stable even
when COUNT(*) == 0; ensure the identifier you use matches the existing as_of
symbol in this module and that the today CTE still returns cnt and dow.
- Around line 1029-1053: In _render_as_of_literal: detect period == "hour" and
raise/return a clear compilation-time error (or otherwise reject hour) instead
of emitting DATE_TRUNC on a date literal (since BigQuery rejects
DATE_TRUNC(DATE, HOUR)); keep returning the bare literal for "day" and the
DATE_TRUNC wrap for supported units. In _render_anomaly_today_cte: stop
computing today.dow from data (MAX(EXTRACT(...))) and instead derive the dow
from the as_of period boundary literal produced by _render_as_of_literal (i.e.
EXTRACT(DOW/DAYOFWEEK from the truncated/as_of literal) or compute it from the
same date-trunc expression) so today.dow is non-NULL even when the today slice
is empty. Ensure references to _render_as_of_literal and
_render_anomaly_today_cte are used so the as_of-derived dow matches the period
alignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c86f0e4-920b-4982-8eea-4704fe5348cb
📒 Files selected for processing (5)
src/signalforge/diff/_emitter.pysrc/signalforge/prune/compiler.pytests/diff/test_emitter.pytests/fixtures/diff/proposed_test_files/anomaly/orders__row_count_anomaly_by_period_8e4d6245.sqltests/prune/test_compiler.py
CodeRabbit re-reviewed my first closeout commits and caught 3 real issues with the new singular-test SQL helper + emitter wiring. - #13 (CRITICAL: zero-row seasonal silent-pass): the today CTE computed dow via MAX(EXTRACT(... FROM date_column)) which is NULL on an empty period; the downstream stats.dow = today.dow JOIN then drops every row and the test passes silently — even though a zero-count period IS itself a meaningful anomaly (catastrophic load failure). Fix: derive dow from the anchored as_of LITERAL (compile-time constant per emitted SQL), so the band check fires correctly even when COUNT(*) = 0. Pinned by new regression test test_singular_test_sql_seasonal_dow_is_stable_for_empty_today_period. - #12 (CRITICAL: invalid SQL for period=hour): DATE_TRUNC(DATE, HOUR) is rejected by BigQuery (DATE_TRUNC of DATE only accepts year/month/week/day; HOUR requires DATETIME/TIMESTAMP). With a date-typed as_of the period=hour case cannot emit valid SQL. Fix: return _InvalidIdentifier at compile entry → engine routes to kept-without-evidence per conservative-bias. v0.x ships day/week only; hour-period support is deferred until as_of becomes a datetime. Pinned by test_compile_row_count_anomaly_period_hour_returns_invalid_identifier. - #11 (emitter bypassed compiler safety checks): the diff emitter called _compile_anomaly_singular_test_sql() directly, skipping the validate_identifier(date_column) + validate_test_sql(composed) gates that the engine-side _compile_row_count_anomaly_by_period runs. A kept anomaly with a hostile where clause could land in operator- shipped dbt SQL. Fix: re-run both safety gates in the emitter before writing; on failure, skip emission silently (the engine separately routes the case to kept-without-evidence). Pinned by test_emit_proposed_test_files_anomaly_skips_hostile_where_clause. Validation: 3221 passed, 0 ruff / pyright errors.
Owner
Author
PR Review Round 2 — CodeRabbit follow-upAddressed all 3 new findings from CR's re-review of the first closeout commits ( Fixed (3 items)
Validation
Closeout meta-observationCR's iterative re-review caught what my snapshot-based QG missed:
All 3 are the same shape of bug: "defensive helper exists; new call site bypassed it." Going to update |
This was referenced Jun 3, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Super plan for #171 —
row_count_anomaly_by_periodas the 8th first-class test primitive. First time-bound primitive: predicts a per-period row-count band from history and flags the most-recent period when it falls outside the band.Phase: detailing (awaiting approval to devolve)
Stories: 17 implementation + Quality Gate + Patterns & Memory = 19 stories (~20 beads incl. epic)
Decisions: 13 locked DECs
Locked decisions
--as-ofdefaults todate.today(), one INFO log line; carried onPruneEvent.as_of.AnomalyTestStatsdiscriminated union (MadStats | ZscoreStats | PercentileStats | MinMaxStats) onPruneDecision+PruneEvent; grader-visible.no-redundanttext with anomaly calibration prose.as_of+statson BOTHPruneEventandPruneDecision; redacted from__repr__.period: Literal["hour","day","week"]+lookback_periods: int = 28+ 4-method × 2-seasonality knobs.n < min→ skip Query 2 +kept-without-evidence. DOW + thin per-DOW → degrade-to-non-seasonal + WARNING._test_requires_source_table(test, sample_strategy)helper centralises bypass routing.row_count_between+unique_combinationALSO bypass undersample_strategy="oneshot"(was: materialised only). CHANGELOG entry obligated.Dialectgraduates 5 new SQL-fragment fields for date arithmetic._PRUNE_AUDIT_SCHEMA_VERSION: 2 → 3;as_ofserialises via.isoformat(); v2 records replay clean.Architectural commitments respected
no-redundantcriterion (extended).Dialect; never branches ondialect.name. AST import-guard remains green.--as-of; same(model, as_of)= same decision.Story sequence
17 implementation stories + Quality Gate + Patterns & Memory. Architecture ordering: typed shapes → dialect → drafter → prune → CLI → diff → grade → docs + e2e.
See
plans/super/171-row-count-anomaly.mdfor the full breakdown.Ralph concurrency hazards (flagged for devolve)
_PROMPT_VERSIONsnapshots → serializesignalforge.prune.engine→ serializeCHANGELOG.md→ schedule last before US-018Next steps
Summary by CodeRabbit
New Features
Behavior Changes
Docs
Tests