#170: unique_combination 7th test primitive - #180
Merged
Conversation
Phase 1 (discovery) — 3 parallel subagents mapped the 6 dispatch sites from the #169 precedent, surfaced the genuinely-new deltas (sample-mode semantics, columns canonical form, grade-side cache-stability gap). Phase 2 (architecture) — 5 parallel reviews (security, performance, data model + API, testing strategy, observability + ops). Cross-review consensus auto-decided sample-mode routing (engine source-override per #169 US-007a), rubric refinement (extend no-redundant, no 5th criterion), ingest strictness, mechanic exhaustiveness gate, and e2e-fixture seeding strategy. Phase 3 (refinement) — 17 DECs across all three earlier phases including three contested decisions resolved by the user: SORT the columns tuple in args_hash, establish the grade-side _PROMPT_VERSION surface, establish __repr__ redaction retroactively across CandidateTestRowCountBetween and CandidateTestCustomSQL. Phase 4 (detailing) — 16 implementation stories + Quality Gate + Patterns & Memory = 18 beads. US-005 split into compiler+snapshots (US-005a) and engine override+behavioural pin (US-005b) per user feedback. Plan doc is the tracking surface; beads land in Phase 7 after plan approval. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a new model-level test variant, ChangesCore unique_combination test feature
Comprehensive test and fixture coverage
🎯 3 (Moderate) | ⏱️ ~20–30 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
|
Devolved to 17 beads (1 epic + 14 implementation + Quality Gate + Patterns & Memory). User-approved on 2026-06-01. Ready-to-start beads: US-001 (variant model + plumbing) and US-011 (engineered fixture + manifest seed) — the two independent leaves of the dependency graph. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nt model + plumbing Add the 7th CandidateTest Pydantic variant + every supporting registration. Foundation for every downstream story in #170. Traces to #170 DEC-001 (variant name), DEC-002 (where shipped v1), DEC-014 (per-column identifier check at anchor-contract arm — NOT Pydantic), DEC-016 (len>=2 cardinality + no-duplicates invariants). Variant shape: type: Literal['unique_combination'] = 'unique_combination' column: None = None (model-level only — third variant after custom_sql and row_count_between to share this constraint) columns: tuple[str, ...] (len>=2, no duplicates — both invariants enforced at Pydantic; raw identifier strings carried through, shape check is US-004) where: str | None = None (non-empty after strip when set, matches the CandidateTestRowCountBetween precedent) rationale: str | None = None Registrations: - Added to CandidateTest discriminated union + __all__ export (src/signalforge/draft/models.py) - VALID_TEST_TYPES frozenset gains 'unique_combination' (src/signalforge/draft/config.py) - StrictCandidateTestUniqueCombination drift mirror + added to _StrictCandidateTest union (tests/draft/test_drift_detector.py) - New row in tests/fixtures/draft/candidate_schema_v1.json (after the row_count_between row, before the closing bracket) - test_valid_test_types_constant_matches_known_set updated to include the new variant (the load-bearing fail-loud gate) Deferred to downstream beads per the plan: - US-002: __repr__ redaction (security review gap) - US-003: drafter prompt catalogue + _PROMPT_VERSION rotation - US-004: anchor-contract arm (identifier shape + per-column check) - US-005a/b: prune compiler arm + sample-mode routing - US-006: artifact_id (SORTED) + diff emitter arm - US-007: ingest parser arm + anchor exemption - US-008: grade rubric no-redundant extension Validation: ruff check + ruff format + pyright + pytest all green (2860 passed, 75 deselected, coverage 97.53%).
…ombination variant model + plumbing
…ation_pairs.sql + manifest seed Add a new fixture model with a natural multi-column GROUP BY pattern on (start_station_id, end_station_id, subscriber_type) so the drafter's prompt example reliably steers claude-sonnet-4-6 toward proposing the structured `unique_combination` test (#170 AC-1) instead of freeform `custom_sql GROUP BY HAVING COUNT(*) > 1`. Source-as-model alias trick per .claude/rules/testing-signal.md § 'WHERE the always-pass column must live depends on whether the model is materialised' — the model's `alias` is overridden to `bikeshare_trips` so its relation_name resolves directly to the public source table, no `dbt run` materialisation needed. Real source columns only; no engineered literal/COALESCE columns. Hand-crafted manifest seed per testing-signal.md § 'Hand-crafted manifest seed when workers can't run live tooling': Ralph workers in worktrees can't reach live BigQuery. The committed manifest.json + catalog.json entries mirror the dbt-bigquery 1.8 shape used for `stg_bikeshare_trips`. A future maintainer with live credentials can re-run regenerate.sh to refresh both models in one shot. Loads-only tests in tests/manifest/test_austin_fixture_loads.py verify the seed survives Pydantic parsing through signalforge.manifest.load without env vars / live calls. The existing iter_models test graduated from 'exactly one model' to 'staging models include both'. Demo parity: src/signalforge/_demo/ is mirrored with the same SQL + manifest + catalog updates so tests/test_demo_fixture_parity.py stays green (DEC-015 of #47 — the regenerate.sh's rsync step naturally covers the new file on a real regen). Traces to plans/super/170-unique-combination.md DEC-005, DEC-010, US-011.
…stg_bikeshare_station_pairs.sql + hand-crafted manifest seed
…for grain-meaningfulness Extend the no-redundant criterion (rubric.py:189-198) with sibling calibration prose for unique_combination, naming the vacuously-unique tuple shape (primary_key, anything) — analogous to the row_count_between vacuous-bound extension from #169 DEC-009. Stays at 4 criteria per DEC-007 (no 5th criterion); same routing as the prior extension (low score → existing passed: bool threshold → flagged tier). Rotated three pinned hashes in lockstep (only the no-redundant criterion text changed; clarity/consistency/rationale hashes unchanged): - _canonical_rubric_hash: 22a0231690aca6ef → 30a9fda975b6d45c - prompt_version_template: 5a70561088930c97 → 4dae4421972e9c2d - criterion_prompt_hash[no-redundant]: 60690cb4ef9246ee → 7b96cfdfe63bc8bc Rotation-history comments updated in rubric.py + both pin sites with the #170 DEC-007 rationale. Grader's 3-trigger degrade taxonomy stays locked — a vacuous composite key is a low score, not a 4th degrade trigger.
…ndant extension for grain-meaningfulness
…ollect-all matrix Extend _validate_anchor_contract with a unique_combination arm (model-level only): per-column membership check on each entry of test.columns + sqlglot-driven column-existence + type-coherence validation on the optional where clause. Collect-all preserved: every violation surfaces in one LLMOutputAnchorContractError, never short-circuits (DEC-022 of #5; DEC-014/015/016 of #170). Generalised _check_row_count_between_where -> _check_where_clause with a test_type prefix parameter so the same sqlglot machinery serves both where-bearing variants (DEC-005 of #169 'reuse, don't fork'). Existing row_count_between violation messages preserved byte-equal via the test_type='row_count_between' call site. Tests (7 new, all under -k unique_combination): - valid pair (no where) - valid 3-column tuple + where on a coercible-type column - hallucinated column in the columns tuple - hallucinated column in the where clause - type-incoherent where (INT64 vs STRING comparison) - exclude_tests=('unique_combination',) backstop - collect-all multi-violation (CandidateColumn + tuple + where) Validation: ruff/ruff-format/pyright/pytest all green; 2867 passed, coverage 97.68%; the 6 pre-existing row_count_between parser tests still pass byte-equal against the renamed helper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mns) + diff emitter arm Add CandidateTestUniqueCombination arms in two related sites per #170 DEC-011 and DEC-002: - src/signalforge/_common/artifact_id.py: new isinstance arm in model_test_args_hash with sorted(test.columns) (load-bearing — DEC-011). Mirrors accepted_values.values precedent. Cross-stage parity holds by re-export identity through signalforge.diff._artifact_id and signalforge.grade.engine (no per-module change). - src/signalforge/diff/_emitter.py: new isinstance arm in _render_test emitting {dbt_utils.unique_combination_of_columns: {combination_of_columns: [...]}}. Pydantic field 'columns' maps to dbt-utils macro key 'combination_of_columns' on emission only (field-name mapping seam). Emission preserves the LLM's declared order — sorting is for the canonical-hash domain only. Optional 'where' rendered verbatim under 'where' key when set; omitted when None. Tests: - tests/diff/test_artifact_id.py: 7 new unique_combination tests covering sort invariance (a,b) == (b,a), 3-column permutation, distinct columns → distinct hash, distinct where → distinct hash, collision suffix via compute_args_hashes, exact-duplicate ordinal suffix, cross-stage parity. - tests/diff/test_emitter.py: 5 new unique_combination tests covering no-where YAML shape, with-where YAML shape, declared-order preservation (the sort/no-sort load-bearing distinction), dropped-decision filtering, and the contract that unique_combination does NOT flow to emit_proposed_test_files. Validation: uv run ruff check / format / pyright / pytest all green. All 2873 tests pass; coverage 97.54%. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…contract arm + collect-all matrix
…arm (SORTED columns) + diff emitter arm
…N snapshot surface Establishes the grade-side cache-stability surface that .claude/rules/business-rule-tests.md § "Lockstep _PROMPT_VERSION rotation when extending the catalogue (#169 DEC-012)" claims exists but actually doesn't pre-#170. Closes the overstated rule-file claim with reality (#170 DEC-012 / US-009). Changes: - src/signalforge/grade/prompts.py: add module-level _PROMPT_VERSION constant computed at import as prompt_version_template(DEFAULT_RUBRIC). Mirrors the drafter shape (signalforge.draft.prompts._PROMPT_VERSION). Recipe documented inline: blake2b-8 over _SYSTEM_PROMPT + render_rubric_block(DEFAULT_RUBRIC) + envelope tags. Rotates when system prompt, any of the 4 default criterion texts, or envelope tags change. Exported in __all__. - tests/grade/test_prompt_cache_stability.py (new): pin the constant + the rendered rubric-block bytes. Two tests: hash-pin assertion and difflib-diffing byte-equality against an inline golden. Mirrors tests/llm/test_prompt_cache_stability.py shape verbatim. Rotation history documents the current value 4dae4421972e9c2d as the #170 US-009 establishment. - pyproject.toml: per-file-ignore E501 on the new test (inline rubric-block golden carries long single-line criterion texts that render together on the wire; refactoring would change the bytes the test pins). Pinned values (current, also matching the live helpers after US-008): - _PROMPT_VERSION = "4dae4421972e9c2d" - _canonical_rubric_hash(DEFAULT_RUBRIC) = "30a9fda975b6d45c" (pinned elsewhere by US-008; this commit does not touch it). Validation: ruff check, ruff format --check, pyright, pytest all green (2863 passed, 97.54% coverage). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e _PROMPT_VERSION snapshot surface
…emption Adds the recognition arm + helper for the ``dbt_utils.unique_combination_of_columns`` macro in ``signalforge.ingest.parser`` (dispatch site #4 per ``.claude/rules/business-rule-tests.md`` § "The 6 production dispatch sites") and the model-level loop exemption in ``signalforge.ingest.anchor.validate_anchor_contract`` (dispatch site #6 — the third model-level-only variant after ``custom_sql`` and ``row_count_between``). Inbound mapping (DEC-002 / DEC-008): YAML ``combination_of_columns: list[str]`` ↔ Pydantic ``columns: tuple[str, ...]``; optional ``where: str`` carried verbatim. The mapping seams are exactly these two functions (inbound here, outbound in ``diff/_emitter.py``) — the dbt_utils macro-name field does NOT bleed into the internal model. ``_UNIQUE_COMBINATION_NAME`` mirrors the ``_ROW_COUNT_BETWEEN_NAME`` precedent. Skip routes (all ``reason="malformed-supported-test"`` — closed 3-value ``SkipReason`` enum stays locked per ``.claude/rules/ingest-layer.md``): column-scoped usage (variant is model-level only); non-dict body; missing ``combination_of_columns`` key; non-list value under the key; empty list; ``len < 2`` (single-column is just ``unique``); non-string items (incl. bool guard mirroring ``_parse_row_count_between``); duplicate items; non-empty non-string or whitespace-only ``where``. Different sibling ``dbt_utils.*`` macros stay ``custom-or-generic-test``. Tests (15 new parser + 1 anchor): * tests/ingest/test_parser.py — happy paths (inline 2-col, with where, 3-col, arguments:-nested), 9 malformed routes, sibling-macro custom-skip, config-keys-ignored. * tests/ingest/test_anchor.py — model-level + ``column=None`` does not raise (mirrors ``test_model_level_row_count_between_with_none_column_does_not_raise``). Fixture updates: ``schema_codegen_shaped.yml`` and ``schema_austin_bikeshare.yml`` (and their consumers ``test_reader.py``, ``test_prune_existing.py``, and the column-scoped custom-skip pin in ``test_parser.py``) switched the example namespaced/custom test from ``dbt_utils.unique_combination_of_columns`` to ``dbt_utils.not_null_proportion`` — the original macro is now a first-class variant and column-scoped usage now correctly routes to malformed-supported-test. Also: pre-existing format drift in ``tests/draft/test_parser.py`` fixed in-passing so the pipeline gate stays green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…combination + BQ/Snowflake snapshots Implements the 7th first-class CandidateTest variant's compiler arm (#170 US-005a, traces to DEC-014 / DEC-015). * _compile_unique_combination in src/signalforge/prune/compiler.py emits the composite-grain failing-rows SELECT shape 'SELECT <cols> FROM <table_ref> [WHERE <where>] GROUP BY <cols> HAVING COUNT(*) > 1'. Dialect-driven via the existing _fold_identifier + _quote + _qualified_table_name helpers (no dialect.name branching — load-bearing per .claude/rules/prune-engine.md § 'Compiler is dialect-driven'). * Per-column DEC-014 identifier shape gate (defence-in-depth on top of the anchor-contract arm). Identifier rejection routes via _InvalidIdentifier → kept-without-evidence (conservative-bias). * DEC-015 compose-then-validate: 'where' is interpolated into the full SELECT then routed through validate_test_sql, mirroring _compile_row_count_between (#169 DEC-005 reuse). Hostile 'where' (stray ';', '--', '/* */', unbalanced parens) returns _InvalidIdentifier rather than raising. * Dispatcher arm added at _compile_test after the CandidateTestRowCountBetween branch. 6 new snapshot fixtures (3 BigQuery + 3 Snowflake) pin the byte-exact emitted SQL across both dialects. The 3 Snowflake fixtures are added to the gated sqlglot Snowflake-dialect parse-guard (tests/prune/test_compiler_fakesnow.py::_ALL_SNOWFLAKE_FIXTURES) per the #121 lesson — snapshot equality certifies shape, not validity; a parser-in-the-loop is what catches reserved-keyword / quoting regressions. Sample-mode routing is out of scope here (US-005b). The engine's source-vs-temp override for unique_combination ships in the sibling bead; this arm consumes table_ref as-is. The dispatcher arm comment points at the load-bearing engine-level pin (test_prune_tests_unique_combination_under_*) that US-005b will add. 12 new compiler tests (6 snapshot equality + adversarial column + hostile where x3 + dispatch-arm uniqueness + safety round-trip). The full default suite (2880 tests) passes; the gated -m snowflake suite (36 offline fakesnow/sqlglot tests) passes; pyright clean; ruff check/format clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ingest anchor exemption
…_compile_unique_combination + BQ/Snowflake snapshots
…etroactive) Establishes redacted __repr__ on the three text-bearing candidate-test variants — CandidateTestCustomSQL, CandidateTestRowCountBetween, CandidateTestUniqueCombination — so the LLM-emitted free-text fields (sql / where / rationale) never reach log sinks via casual repr() or %s-interpolation. Chosen shape: per-class __repr__ overrides + a small shared helper (_scope_repr) for the column/model-level scope segment. Each variant has a distinct identifying surface (custom_sql's column-scope handling, row_count_between's numeric bounds, unique_combination's columns tuple), so per-class bodies stay clearer than a single mixin. The scope helper keeps the column-vs-model-level convention in one place for any future variant. Surface exposed in repr(): - type (the discriminator Literal) - scope (column=<name> for column-scoped, <model-level> otherwise) - per-variant constraint shape: custom_sql shows only scope; row_count_between shows minimum/maximum bounds; unique_combination shows the columns tuple (column NAMES are not value-bearing) Surface redacted: - sql (CandidateTestCustomSQL) - where (CandidateTestRowCountBetween, CandidateTestUniqueCombination) - rationale (all three) Per the rule (prune-engine.md / grade-layer.md § "Custom __repr__"): Pydantic __str__ is reserved for serialisation and stays untouched — only __repr__ is overridden. model_dump_json() round-trip continues to carry every field (3 positive tests pin this). The four non-text-bearing variants (NotNull, Unique, AcceptedValues, Relationships) are untouched — they carry only column names and structured args, no free-text leak surface. Closes the log-hygiene gap surfaced by Phase 2 Security review (#170 DEC-013). Drive-by: ruff format on tests/draft/test_parser.py (pre-existing format drift from US-004 merge that VALIDATE_CMD would otherwise reject). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ction (mixin + retroactive)
…ERSION rotation Add unique_combination as the 7th first-class test primitive in the drafter prompt catalogue (issue #170, DEC-002). Two illustrated forms (no-where and with-where) mirror the #169 row_count_between precedent. New _UNIQUE_COMBINATION_SCOPE_INSTRUCTION block carries cautionary prose steering the LLM away from vacuously-unique tuples like (pk, anything) — the prompt-level prevention complements the grade-side no-redundant criterion calibration (US-008). Catalogue + SCOPE wiring: - _TEST_CATALOGUE_LINES gains the unique_combination entry (after row_count_between), illustrating both no-where and with-where shapes with realistic 2-column examples. - _UNIQUE_COMBINATION_SCOPE_INSTRUCTION mirrors _CUSTOM_SQL_SCOPE_INSTRUCTION: emitted only when unique_combination is allowed, omitted when excluded so the prompt never asks for a type the parser would reject. - _render_system_prompt threads unique_combination_scope through the SCOPE template alongside custom_sql_scope. - _SYSTEM_PROMPT_TEMPLATE gets a {unique_combination_scope} format slot at the end of the SCOPE section. _PROMPT_VERSION rotation (77e9ee8a6ae7d875 → 389c8aa970df86cc): - Constant rotates automatically (computed from _SYSTEM_PROMPT bytes). - _EXPECTED_PROMPT_VERSION in tests/llm/test_prompt_cache_stability.py bumped to the new hex. - Rotation history extended with a #170 entry noting the new catalogue line + SCOPE instruction (cached-block golden unchanged — only the system prompt rotated). exclude_tests test fixes: - Four tests in tests/draft/test_exclude_tests.py added unique_combination to their exclusion tuples. They previously enumerated the standard set exhaustively; the 7th variant joining the catalogue means they need to exclude it too to preserve their original intent (testing how SCOPE renders with only some types remaining). Docstring updates name unique_combination + #170. "Five entries" → "six entries" in the _TEST_CATALOGUE_LINES docstring (custom_sql lives separately in _CUSTOM_SQL_CATALOGUE_LINE per the existing convention). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ogue + _PROMPT_VERSION rotation
…+ behavioural routing pin Extends the existing row_count_between source-vs-temp conditional in prune/engine.py to include CandidateTestUniqueCombination — composite uniqueness on a sample is semantically approximate (false-negative risk: a duplicate pair may straddle the sampled and unsampled rows), so always-route-to-source mirrors #169 US-007a. Bounded by maximum_bytes_billed. Both sites move in lockstep: * `all_bypass_to_source` short-circuit (CodeRabbit #176 fix) — when EVERY candidate is row_count_between OR unique_combination, skip materialise_sample AND _resolve_sample_bucket pre-work; otherwise a materialisation failure would spuriously route every test to kept-without-evidence. * Per-test `per_test_table_ref` override — when scope="sample" and candidates are mixed, the row_count_between / unique_combination ones still route to source while other variants consume the substituted compile_table_ref. Behavioural routing pin at tests/prune/test_engine.py — mirrors the #169 row_count_between precedent (test_prune_tests_row_count_between_under_materialised_references_source_not_temp_table): * Parametrised across sample_strategy="materialised" AND "oneshot" — the load-bearing pin (snapshot equality from US-005a certifies SQL shape but NOT engine routing per .claude/rules/business-rule-tests.md § "Pin the engine-routing test, not just the compiler snapshot"). * Asserts compiled_sql references the source qualified name AND never references `_SESSION._sf_sample_*`. * Companion scope="full" test as a no-regression belt-and-braces. Traces to #170 DEC-006 (Option (iii): engine override to source via per_test_table_ref). Done when the engine routes unique_combination to source under sample mode, pinned by behavioural test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…source override + behavioural routing pin
…e dispatch routing test) Adds a targeted dispatch-site routing test (NOT a full AST scan, per DEC-009 of #170) that constructs a minimal instance of every variant in the CandidateTest discriminated union and asserts each routes through every one of the 6 production dispatch sites without raising. The 6 sites (per .claude/rules/business-rule-tests.md § 'The 6 production dispatch sites'): 1. signalforge.prune.compiler._compile_test 2. signalforge._common.artifact_id.model_test_args_hash 3. signalforge.diff._emitter._render_test 4. signalforge.ingest.parser._parse_named_test (external macro recognition) 5. signalforge.draft.parser._validate_anchor_contract 6. signalforge.ingest.anchor.validate_anchor_contract Each variant × site combination is one parametrize iteration; site 4 skips for variants without an external dbt-macro form (only row_count_between and unique_combination have one). Self-checks pin that sites 5 and 6 raise on real violations so the routing arms can't silently mask dispatch bugs with a tautological green. Variant reflection via typing.get_args means an 8th variant added to the union auto-grows the parametrize without a test edit. A cardinality tripwire asserts the union currently holds exactly 7 variants; bumping the union forces the contributor to update _EXPECTED_VARIANT_COUNT, _make_instance, and _EXTERNAL_MACRO_YAML in lockstep — all gated by this test rather than discovered at runtime on the operator's machine. TDD verified: temporarily removing the unique_combination arm from _render_test (site 3) and the model-level exemption from validate_anchor_contract (site 6) each fail one parametrize iteration loudly with a remediation message pointing at the missing arm. 48 parametrize iterations (43 pass + 5 N/A site-4 skips); no production code change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ness gate (6-site dispatch routing test)
…s + CHANGELOG + mkdocs nav Bundles the documentation tail of #170 (unique_combination as 7th first-class CandidateTest variant). Per Phase 1 S2 + DEC-003, the SSOT lives in the operator-facing docs/ tier (not .claude/rules/). - docs/drafter-catalogue.md (new) — SSOT enumerating the seven first-class primitives (not_null, unique, accepted_values, relationships, custom_sql, row_count_between, unique_combination) with YAML examples, structural slots, scope, ingest signatures, semantics; a 'custom_sql is the catch-all' sub-section; a 'What we do NOT generate today' boundary section (column value range, conditional uniqueness beyond simple where, statistical/distributional anomalies, cross-table reconciliation, time-series anomaly detection). - README.md — new 'What tests SignalForge generates' section sitting between 'What it does' and 'How it works' (compact 7-row table + pointer to the SSOT). - docs/draft-ops.md — new 'Composite uniqueness (unique_combination)' section after the row_count_between block, mirroring its shape: what the variant is, when drafted, worked example, exclude_tests short-circuit. - docs/ingest-ops.md — new 'Recognition of dbt_utils.unique_combination_of_columns' subsection mirroring the expect_table_row_count_to_be_between precedent: inbound mapping, skip-recorded shapes, unchanged-other-dbt_utils-macros boundary. - docs/grade-ops.md — new 'Composite-key calibration (unique_combination)' subsection extending the no-redundant criterion narrative; rubric stays at four criteria. - docs/prune-ops.md — callout in the row-count cost model section for the unique_combination engine source-vs-temp routing override. - mkdocs.yml — adds 'Test Catalogue: drafter-catalogue.md' to the nav between 'Claude Code Skill' and 'Pipeline Stages'. - CHANGELOG.md [Unreleased] — Added (unique_combination variant + dbt_utils.unique_combination_of_columns ingest recognition), Docs (drafter-catalogue.md + README section + four ops doc paraphrases), Changed (drafter _PROMPT_VERSION rotation 77e9ee8a6ae7d875 → 389c8aa970df86cc, grade rubric no-redundant extension, grade-side _PROMPT_VERSION snapshot surface 4dae4421972e9c2d established). Validation: uv run ruff check/format/pyright + uv run pytest (2922 passed, 78 deselected, 97.70% coverage) + uv run --only-group docs mkdocs build green. Pre-existing pre-#170 INFO-level link warnings (e.g. draft-ops.md#row-count-tests-row_count_between anchor missing in rendered HTML — a #169 rendering bug from the BUSINESS RULES code fence) are unaffected. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…after-catalogue.md + README + ops paraphrases + CHANGELOG + mkdocs nav
… unique_combination) New @pytest.mark.e2e-gated test pinning the load-bearing behavioural claim of #170: when the drafter sees a model whose SELECT body advertises a natural composite GROUP BY shape (US-011's engineered fixture model stg_bikeshare_station_pairs), it proposes a structured CandidateTestUniqueCombination candidate — NOT a freeform custom_sql GROUP BY HAVING COUNT(*) > 1. Standard three-env-var gate (mirrors test_e2e_bigquery_smoke.py baseline): SF_RUN_BQ=1 + GOOGLE_CLOUD_PROJECT + ANTHROPIC_API_KEY. Drafter is Anthropic Sonnet 4.6; warehouse is BigQuery; no provider overlay. Engineered determinism via structural (not value-pinning) assertion: AT LEAST ONE PruneDecision must carry test.type == 'unique_combination' with len(test.columns) >= 2. The exact columns tuple is NOT pinned because Sonnet may legitimately propose two- or three-column variants of the fixture's GROUP BY shape; the prune verdict (kept vs. dropped) is NOT pinned because bikeshare data shape is orthogonal to the freeform→structured translation under test. Test reads PruneDecision.test from .signalforge/prune.jsonl via the existing read_prune_decisions helper — the typed CandidateTest discriminated union flows through prune intact (PruneEvent.test: CandidateTest, audit DEC-014). Validation green: ruff + format + pyright + pytest all clean. Test is correctly deselected by default (not e2e) addopts and skips with distinct reason for each missing env var when invoked with -m e2e. AC-1 (drafter proposes structured unique_combination), AC-8 (end-to-end pipeline shape). Maintainer-run command: SF_RUN_BQ=1 ANTHROPIC_API_KEY=sk-... GOOGLE_CLOUD_PROJECT=<project> \ uv run pytest -m e2e -k unique_combination --no-cov Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…emits structured unique_combination)
…r angles Triangulated findings across correctness / conventions / tests / docs+UX reviewers; same finding from 2+ angles upgraded to must-fix per memory qg-diverse-reviewer-angles-catch-cross-surface-drift. Fixes applied: 1. **F1 (Pass 4 must-fix)** — Pre-existing `## BUSINESS RULES` literal inside fenced code block in docs/draft-ops.md:288-300 was parsed as ATX heading by mkdocs anchor generator, breaking 14 of 20 H2 anchors downstream (covers row_count_between + unique_combination — all 8 inbound cross-doc links #170 added go through these). Switched to indented code block (4-space) which defeats the heading scan. 2. **C1 (Pass 1 concern, empirically verified)** — DEC-013 __repr__ redaction was leaking via Pydantic v2's __repr_args__ / __rich_repr__ / __pretty__ hooks (rich.print() / devtools / pprint). Added __repr_args__ overrides on the 3 redacting classes (CandidateTestCustomSQL, CandidateTestRowCountBetween, CandidateTestUniqueCombination). Filters out where / sql / rationale from the structured-debug surface. New test pins the closure across all 3 variants + asserts model_dump_json() still carries the secrets (serialisation contract unchanged). 3. **Pass 3 must-fix (load-bearing routing pin gap)** — US-005b per_test_table_ref override has two conditionals (all_bypass_to_source short-circuit AND per-test override). Single-variant tests only exercised the first; dropping unique_combination from the per-test arm while leaving it in the short-circuit would PASS silently. Added test_prune_tests_mixed_candidates_per_test_override_routes_unique_combination_to_source with not_null + unique_combination mix — exercises the per-test arm directly. Asserts not_null compiles against _SESSION sample, while unique_combination compiles against source — both routings pinned. 4. **F2 (Pass 4 must-fix)** — CHANGELOG Added entry was silent on the dbt-utils install constraint (parallel gap to #169's note on dbt-expectations). Appended the parallel clause. 5. **F3 (Pass 4 should-fix)** — CHANGELOG Changed entries on both _PROMPT_VERSION rotations now name the one-time Anthropic prompt-cache miss operators pay on upgrade. 6. **F4 (Pass 4 should-fix)** — docs/drafter-catalogue.md row_count_between subsection was missing the source-vs-temp routing callout that unique_combination has; added parallel one-paragraph callout naming both variants in lockstep. Deferred to US-015 / follow-up per Pass 2 informational findings: - US-004 #169 pre-existing format drift on tests/draft/test_parser.py (3 workers all "drive-by formatted" the same file; investigate why US-004's own ruff format --check passed) - custom_sql lacks model-level loop exemption in ingest.anchor (benign today, documented gap from US-010 worker) - E501 ignore asymmetry on drafter vs grade snapshot test - _PROMPT_VERSION in grade.prompts __all__ cosmetic - Missing "description" config-key in unique_combination ingest test Validation green across: - uv run ruff check . && uv run ruff format --check . - uv run pyright (0 errors) - uv run pytest (full suite + new tests) - uv run pytest -m cli_subprocess --no-cov (8 pass) - uv run pytest -m wheel_smoke --no-cov (5 pass) - uv run pytest -m snowflake --no-cov (36 pass + 5 live-only skips) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…memory writes Closes #170 Patterns & Memory tail. ## Rule file updates - **`.claude/rules/business-rule-tests.md`** — bumped "2-instance precedent" → "3-instance precedent" (custom_sql / row_count_between / unique_combination). Reference footnote extended with the #170 plan pointer + the durable conventions section "#170 lessons worth carrying forward" enumerating the 5 patterns the next variant-extension should pre-empt: 1. Two engine conditionals (`all_bypass_to_source` short-circuit AND per-test `per_test_table_ref` override) — both grow in lockstep; mixed-candidate test is load-bearing for the per-test arm. 2. Pydantic v2 `__repr_args__` / `__rich_repr__` / `__pretty__` hooks bypass `__repr__` — override `__repr_args__` for redaction parity across rich.print() / devtools / pprint. 3. SORT a tuple-shaped canonical hash arg (`(a,b) ≡ (b,a)` semantics); the diff EMITTER preserves declared order — split contract. 4. mkdocs ATX heading in fenced code block silently corrupts downstream H2 anchors; use indented (4-space) code blocks for examples that need a literal `##`. 5. Drive-by formatting across multiple workers reveals merge-induced format drift the original PR's gates can't see — add a post-merge `ruff format --check .` to the orchestrator's Step 5. - **`.claude/rules/grade-layer.md`** — documented the grade-side `_PROMPT_VERSION` snapshot surface established by #170 DEC-012, closing the asymmetry where `business-rule-tests.md` had historically claimed "two `_PROMPT_VERSION` constants" but only the drafter had one. Rotation policy mirrors the drafter contract — rotate when the grade `_SYSTEM_PROMPT` text changes OR any of the four DEFAULT_RUBRIC criterion texts changes; the new value is computed and pinned in the same commit (US-008 + US-009 paired). ## Memory writes (4 new files + MEMORY.md index) All in `~/.claude/projects/-home-wesd-Projects-SignalForge/memory/`: 1. `pydantic-v2-repr-args-redaction-required.md` — DEC-013 lesson; custom `__repr__` alone leaks via `__rich_repr__` / `__pretty__`. 2. `prune-engine-two-conditional-routing-pattern.md` — US-005b discovery; mixed-candidate test is load-bearing. 3. `mkdocs-atx-in-fenced-block-breaks-anchors.md` — QG Pass 4 finding; indented code blocks are the fix. 4. `drive-by-format-reveals-merge-induced-drift.md` — observation across US-005a / US-007 / US-002; orchestrator can pre-empt with a post-merge `ruff format --check`. ## Deferred to follow-up issues (not blocking #170) - `ingest.anchor.validate_anchor_contract` lacks model-level loop exemption for `custom_sql` (benign today; documented by US-010 worker in the dispatch-exhaustiveness test docstring). - US-004 of #169 pre-existing format drift on `tests/draft/test_parser.py` (each of US-005a / US-007 / US-002 fixed it independently; root cause is merge-time drift not worker carelessness). - E501 ignore asymmetry on grade vs drafter snapshot tests. - `_PROMPT_VERSION` in `signalforge.grade.prompts.__all__` (cosmetic inconsistency with drafter side). Validation green: ruff / pyright / pytest (2967 passed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
wjduenow
changed the title
wjduenow
marked this pull request as ready for review
June 1, 2026 14:27
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/fixtures/dbt_project_austin/regenerate.sh (1)
110-110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
mv -ffor non-interactive file operations.The
mvcommand should use the-fflag to force overwrite without prompting, preventing the script from hanging if the destination exists and requires confirmation.🔧 Proposed fix
- mv "$out.tmp" "$out" + mv -f "$out.tmp" "$out"As per coding guidelines, shell scripts must use non-interactive flags (cp -f, mv -f, rm -f) to avoid hanging on confirmation prompts.
🤖 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 `@tests/fixtures/dbt_project_austin/regenerate.sh` at line 110, The mv invocation that renames "$out.tmp" to "$out" should be made non-interactive to avoid prompts; update the mv command that operates on "$out.tmp" and "$out" to include the -f flag (use mv -f) so it force-overwrites the destination without prompting and prevents the script from hanging.
🤖 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 `@docs/draft-ops.md`:
- Around line 617-618: Update the link target so the prune routing reference
points to the specific prune subsection anchor: change the current link to
docs/prune-ops.md#unique_combination (the `unique_combination` anchor) where the
text mentions "for the engine routing (sample-mode bypassed to source —
composite" so readers go directly to the dedicated prune routing documentation.
In `@docs/drafter-catalogue.md`:
- Around line 211-213: Update the cross-reference in the drafter catalogue entry
that currently links to prune-ops.md#row-count-cost-model so it points to the
dedicated unique_combination prune section instead (use the
prune-ops.md#unique_combination anchor); change the link target used alongside
the engine routing mention (the same source-vs-temp pattern as
`row_count_between`) so the text points to the dedicated `unique_combination`
prune section to avoid misdirection/staleness.
In `@plans/super/170-unique-combination.md`:
- Line 173: The table row for DEC-002 contains the cell value `where: str |
None` which is being parsed as an extra column; update that cell (the DEC-002
row) to escape the pipe or wrap the entire value in code fencing so the `|` is
not treated as a column separator (e.g., replace `where: str | None` with an
escaped `where: str \| None` or wrap as `` `where: str | None` ``) to restore
proper table rendering.
- Line 573: The fenced code block at the triple-backtick fence near the noted
location is missing a language identifier, which triggers markdownlint MD040;
update the opening fence from ``` to include the appropriate language (e.g.,
```js, ```python, or the correct language for the snippet) so the block has a
language tag and passes MD040 linting.
In `@src/signalforge/ingest/anchor.py`:
- Around line 90-98: The current branch skips validation for test.type ==
"unique_combination", which bypasses checks on test.columns; change the branch
so that for unique_combination you iterate over test.columns and for each col
verify col in model_columns, appending the same violation message (e.g.
"model-level test references nonexistent column {col!r}") to violations when
missing; keep the existing behavior for other test types and mirror the logic
used in signalforge/draft/parser._validate_anchor_contract to ensure parity
between ingest and parser validation.
---
Outside diff comments:
In `@tests/fixtures/dbt_project_austin/regenerate.sh`:
- Line 110: The mv invocation that renames "$out.tmp" to "$out" should be made
non-interactive to avoid prompts; update the mv command that operates on
"$out.tmp" and "$out" to include the -f flag (use mv -f) so it force-overwrites
the destination without prompting and prevents the script from hanging.
🪄 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: 82cb3ae8-7da4-4559-ae9d-422691326ef9
📒 Files selected for processing (60)
.claude/rules/business-rule-tests.md.claude/rules/grade-layer.mdCHANGELOG.mdREADME.mddocs/draft-ops.mddocs/drafter-catalogue.mddocs/grade-ops.mddocs/ingest-ops.mddocs/prune-ops.mdmkdocs.ymlplans/super/170-unique-combination.mdpyproject.tomlsrc/signalforge/_common/artifact_id.pysrc/signalforge/_demo/models/staging/stg_bikeshare_station_pairs.sqlsrc/signalforge/_demo/target/catalog.jsonsrc/signalforge/_demo/target/manifest.jsonsrc/signalforge/diff/_emitter.pysrc/signalforge/draft/config.pysrc/signalforge/draft/models.pysrc/signalforge/draft/parser.pysrc/signalforge/draft/prompts.pysrc/signalforge/grade/prompts.pysrc/signalforge/grade/rubric.pysrc/signalforge/ingest/anchor.pysrc/signalforge/ingest/parser.pysrc/signalforge/prune/compiler.pysrc/signalforge/prune/engine.pytests/cli/test_e2e_unique_combination.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/fixtures/dbt_project_austin/models/staging/stg_bikeshare_station_pairs.sqltests/fixtures/dbt_project_austin/regenerate.shtests/fixtures/dbt_project_austin/target/catalog.jsontests/fixtures/dbt_project_austin/target/manifest.jsontests/fixtures/draft/candidate_schema_v1.jsontests/fixtures/ingest/schema_austin_bikeshare.ymltests/fixtures/ingest/schema_codegen_shaped.ymltests/fixtures/prune/compiled_sql/snowflake/unique_combination_pair.sqltests/fixtures/prune/compiled_sql/snowflake/unique_combination_three_columns.sqltests/fixtures/prune/compiled_sql/snowflake/unique_combination_with_where.sqltests/fixtures/prune/compiled_sql/unique_combination_pair.sqltests/fixtures/prune/compiled_sql/unique_combination_three_columns.sqltests/fixtures/prune/compiled_sql/unique_combination_with_where.sqltests/grade/test_prompt_cache_stability.pytests/grade/test_prompts.pytests/grade/test_rubric.pytests/ingest/test_anchor.pytests/ingest/test_parser.pytests/ingest/test_reader.pytests/llm/test_prompt_cache_stability.pytests/manifest/test_austin_fixture_loads.pytests/prune/test_compiler.pytests/prune/test_compiler_fakesnow.pytests/prune/test_engine.pytests/test_candidate_test_dispatch_exhaustiveness.py
All 5 review threads addressed: **Major (1):** - `src/signalforge/ingest/anchor.py` — the `unique_combination` model-level loop exemption was skipping `test.columns` validation entirely (CodeRabbit MAJOR; triangulated with QG Pass 1 C2 + Pass 2 informational #2 + US-010 worker docstring — four-way agreement). Extended the arm to iterate `test.columns` and surface per-column violations mirroring the draft-parser side. Pinned by new test `test_model_level_unique_combination_with_hallucinated_column_raises_per_column` asserting collect-all (two hallucinated columns → two distinct violations in one error). Used `isinstance(test, CandidateTestUniqueCombination)` for proper pyright narrowing of the discriminated union. **Minor (4):** - `docs/draft-ops.md` link target → direct `unique_combination` anchor in prune-ops.md (was pointing at `#row-count-cost-model`). - `docs/drafter-catalogue.md` link target → same direct anchor fix. - `plans/super/170-unique-combination.md` DEC-002 row — escaped `|` in `str \| None` so the markdown table parses correctly (markdownlint MD056). - `plans/super/170-unique-combination.md` story-dependency-graph fenced block — added `text` language identifier (markdownlint MD040). Validation green: ruff / pyright / pytest (2968 passed, +1 new test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
PR Review SummaryAddressed all 5 CodeRabbit review threads in commit Fixed (5 items)
False Positives (0 items)None — all 5 findings were valid. Triangulation notesThread 1 (ingest anchor) was triangulated across four independent angles:
Per memory The fix mirrors the draft-parser path at New test pinning the fix
ValidationFull local validation green after the fixes:
All threads resolved below. |
This was referenced Jun 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Super-plan for #170 — add
unique_combination(composite-key uniqueness, optionalwherefilter) as the 7th first-classCandidateTestvariant. Follows the 2-instance variant-extension precedent (custom_sqlfrom #116,row_count_betweenfrom #169); #170 makes it a 3-instance precedent.Phase: detailing → published (awaiting plan review)
Stories: 16 implementation + Quality Gate + Patterns & Memory = 18 beads
Decisions: 17 DECs captured across Phases 1–3
Parent epic: #179 — Test Generation Expansion (target +10 pp / 15-of-143 coverage on
intuit_airflow)Sequencing dependency: #169 shipped 2026-05-31 (commit
2fdb91e)Plan document
See
plans/super/170-unique-combination.mdfor the full 600-line plan.Highlights — the three contested decisions
columnstupleargs_hashcanonical form: SORT (DEC-011) — mirrorsaccepted_values.valuesprecedent at_common/artifact_id.py:79–84;(a, b)≡(b, a)(same GROUP BY result-row identity); single artifact_id → single warehouse call → cache stability._PROMPT_VERSIONsnapshot surface in Add unique_combination as a 7th first-class test primitive (composite-key uniqueness) #170 (DEC-012) — closes the overstated rule-file claim inbusiness-rule-tests.md§ "Lockstep_PROMPT_VERSIONrotation when extending the catalogue (Add row_count_between as a 6th first-class test primitive (drafter + prune + grade) #169 DEC-012)"; no such constant exists today, only the per-event dynamicrubric_hash.__repr__redaction established for Add unique_combination as a 7th first-class test primitive (composite-key uniqueness) #170 + retroactively on existing variants (DEC-013) — closes the log-hygiene gap surfaced by the security reviewer; coverswhere/sql/rationaleonCandidateTestUniqueCombinationANDCandidateTestRowCountBetween+CandidateTestCustomSQL.Load-bearing pins
sorted()invariant — pinned hash equality between(a,b)and(b,a).dbt parse; commit the parsed manifest alongside the new model SQL.Auto-decided architecture findings (cross-review consensus, no Phase 3 question)
per_test_table_ref(mirror Add row_count_between as a 6th first-class test primitive (drafter + prune + grade) #169 US-007a)no-redundantcriterion text, no 5th criteriondbt_utils.unique_combination_of_columns:SkippedTest(reason="malformed-supported-test")on missing / empty / len<2 / duplicates / column-scopedPhase 1 scoping answers
where: str | Noneshipped in v1 — holds the epic Epic: Test Generation Expansion (#169 + #170 + #171) #179 +10 pp coverage projection.docs/drafter-catalogue.md— matches.claude/rules/(architecture) vs.docs/*-ops.md(operator examples) split; keeps rule files context-light.stg_bikeshare_station_pairs.sql— pinnable e2e AC.Out of scope (deferrals)
intuit_airflow(lives in epic Epic: Test Generation Expansion (#169 + #170 + #171) #179, not in this child)bucket_subbucket_uniqueness/unique_if_not_nullcustom-macro ingest recognition (no public signature)expect_column_to_exist(schema-declaration, not behavioural — per ticket)Next steps
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Grading
Tests