#270: comment-bearing / CTE-reprojected / cross-dialect count-scalar prune gaps (plan)

[wjduenow]

Summary

Super plan for #270 — the four follow-up gaps from #267's Quality Gate in the prune-dbt-compiled-tests path.

Phase: detailing (awaiting approval)
Base: dev (at f0bb1a6, after #154 / #267 / #268)
Stories: 6 implementation + Quality Gate + Patterns & Memory
Decisions: 6 (DEC-001…006)

What the plan resolves

• G3 (comment-bearing SQL never prunes) — strip comments in the compiler on complete strings only, leaving the warehouse adapters and Follow-up (#154): scope=sample for ingested dbt-expectations tests via sqlglot AST relation-rewriting #268's span-splice machinery/fixtures byte-untouched. Also disarms a fifth, unfiled bug: Follow-up (#154): aggregate/scalar dbt-expectations macros — source-table routing (Direction 2) #267's single-line count-restructure f-string, where a trailing -- comment eats the closing WHERE (masked today only by the adapter's -- reject; unmasked by fixing G3).
• G1 (cross-dialect divergence) — compiler refuses an unparseable-under-live-dialect body → kept-without-evidence (correctness); CLI threads the live adapter.dialect().name into ingest (signal).
• G2 (CTE-reprojected count) — additive is_row_returning branch skip-records provable one-row bodies, bailing conservatively to row-returning on any filter/join so dbt-expectations' validation_errors shell (which always has a WHERE) is never misclassified.
• G4 (--tests-dir asymmetry) — documented as deliberate; from_manifest=False hand-authored tests keep dbt semantics.

Standing locks held

No new DropReason (5), SkipReason (3), error class, CLI flag, or CandidateTest variant; sqlglot importers stay 2; audit schema stays 4; live BigQuery e2e is the merge gate.

Plan document

See plans/super/270-count-scalar-prune-gaps.md.

Next steps

• Review the plan in this PR
• Approve in Claude Code to devolve to beads

🤖 Generated with Claude Code

https://claude.ai/code/session_01N21jLMPyeo31JDtmXgkDQ7

Summary by CodeRabbit

• Bug Fixes

  • Manifest-ingested SQL containing comments now executes successfully, with comments safely removed before validation.
  • SQL is now checked against the active warehouse dialect to prevent misleading results from incompatible syntax.
  • Improved handling of count-based tests wrapped in CTEs or derived queries.
  • Fixed SQL wrapping when trailing comments could interfere with generated queries.
  • prune-existing now consistently uses the active warehouse dialect throughout processing.

• Documentation

  • Updated pruning, ingestion, adapter, and changelog documentation to describe these behaviors and edge cases.

[coderabbitai]

📝 Walkthrough

Walkthrough

Manifest-ingested custom_sql now strips comments, checks the live SQL dialect, handles re-projected count scalars conservatively, and propagates the active adapter dialect through prune-existing. Tests, documentation, release notes, and the implementation plan cover these behaviors.

Changes

Manifest custom SQL pruning

Layer / File(s)	Summary
SQL analysis and cardinality gates src/signalforge/ingest/_compiled_sql.py, tests/ingest/*	Adds public comment-stripping and dialect-parse helpers, expands one-row CTE/derived-table classification, and adds regression coverage for conservative parsing and cardinality decisions.
Compiler normalization and verdict routing src/signalforge/prune/compiler.py, tests/prune/test_compiler.py, tests/prune/test_engine.py, tests/cli/test_e2e_bigquery_ingested_sample.py	Normalizes ingested SQL before validation and routing, refuses live-dialect parse failures, fixes count-scalar wrapping, and verifies compiled and audit SQL behavior.
Live dialect and adapter reuse src/signalforge/cli/prune_existing.py, tests/cli/test_prune_existing.py	Passes the active warehouse dialect into manifest ingestion and reuses the adapter instance for pruning.
Contracts, release notes, and implementation plan .claude/rules/*, docs/*, CHANGELOG.md, plans/super/270-count-scalar-prune-gaps.md	Documents the compiler, adapter, count-scalar, dialect, validation, testing, and scope changes.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

• SignalForge issue 270 — Directly covers the comment stripping, dialect gating, and CTE count classification implemented here.

Possibly related PRs

• wjduenow/SignalForge#266 — Introduced the manifest-ingested custom_sql determinism and validation flow extended by this change.
• wjduenow/SignalForge#269 — Added the count-scalar classification and rewrite path modified here.
• wjduenow/SignalForge#271 — Added the dialect-aware read_manifest_tests interface used by this change.

Poem

I’m a rabbit with SQL in my den,
Stripping comments again and again.
Dialects now agree,
Counts hop safely free,
While clean audit bytes mark the end.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is specific and matches the PR’s main theme, even though it frames the work as a plan.
Docstring Coverage	✅ Passed	Docstring coverage is 85.71% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

---

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Pull request overview

This PR closes the remaining #270 follow-up gaps from #267’s Quality Gate by making manifest-ingested compiled SQL more robust across comments, dialect parsing, and scalar/CTE edge cases, while preserving the existing adapter/engine contracts (no new enums/flags/audit schema changes).

Changes:

• Strip -- / /* */ comments in the prune compiler for from_manifest custom SQL (including verified sample rewrites) so execution-time strict validation can succeed and audit records reflect what actually ran.
• Add compiler-side live-dialect parse refusal (parses_under_dialect) and thread adapter.dialect().name into manifest ingest to eliminate ingest/compile dialect drift.
• Extend is_row_returning to detect provably one-row CTE/derived-table re-projections of collapsing aggregates (depth-capped, conservative bailouts), and adjust count-scalar compose to prevent trailing-comment suffix swallowing.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/signalforge/prune/compiler.py	Strips comments for manifest-ingested custom SQL, refuses bodies unparseable under the live dialect, and hardens count-scalar compose with newlines.
src/signalforge/ingest/_compiled_sql.py	Exposes strip_sql_comments, adds parses_under_dialect, and implements conservative CTE/derived-table one-row detection via build_scope.
src/signalforge/cli/prune_existing.py	Builds an un-entered adapter earlier to thread the live dialect into read_manifest_tests, reusing the same adapter instance for pruning.
tests/prune/test_engine.py	Asserts audit + in-memory compiled_sql record the stripped bytes for comment-bearing manifest bodies.
tests/prune/test_compiler.py	Pins comment stripping, dialect refusal ordering, and the count-scalar newline compose behavior.
tests/prune/test_compiler_fakesnow.py	Adds Snowflake-specific pin for live-dialect parse refusal behavior.
tests/prune/test_compiler_databricks.py	Adds Databricks-specific pin for live-dialect parse refusal behavior.
tests/ingest/test_manifest_tests.py	Verifies CTE-reprojected count scalars are skip-recorded (existing skip reason).
tests/ingest/test_compiled_sql.py	Adds a broad must-fire/must-not-fire matrix for the CTE one-row classifier; tests parses_under_dialect + strip_sql_comments.
tests/cli/test_prune_existing.py	Validates live-dialect threading into ingest and ensures adapter is entered exactly once and is the same instance passed to pruning.
tests/cli/test_e2e_bigquery_ingested_sample.py	Adds gated live BigQuery e2e certs for comment-bearing execution and count-scalar restructure execution.
plans/super/270-count-scalar-prune-gaps.md	Adds/updates the detailed plan and decisions for #270 implementation and testing.
docs/prune-ops.md	Documents comment-bearing execution and live-dialect refusal behavior.
docs/ingest-ops.md	Documents CTE-reprojected count handling, live dialect threading, and tests-dir count semantics.
CHANGELOG.md	Records the user-visible behavior changes introduced by #270.
.claude/rules/*.md	Updates repository rules/memory to reflect #270’s landed behavior and constraints.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 5

🤖 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/ingest-layer.md:
- Line 141: Fix the invalid Markdown paragraph starts by rewording the
issue-reference sentences: in .claude/rules/ingest-layer.md lines 141-141 and
.claude/rules/prune-engine.md lines 272-272, prefix the sentence with “Issue” so
it begins “Issue `#270` closed...”; in docs/prune-ops.md lines 355-356, keep the
sentence as a normal paragraph, such as “Before `#270`, the story above stopped at
ingest...”.

In `@src/signalforge/ingest/_compiled_sql.py`:
- Around line 289-306: Update _scope_produces_exactly_one_row to reject
row-generating projection and lateral-view shapes before recursing into child
scopes. Detect generator expressions such as explode(...) and LATERAL VIEW
constructs, return False for those cases, and preserve the existing reducer,
join, source, pivot, sample, and recursive checks.

In `@tests/cli/test_e2e_bigquery_ingested_sample.py`:
- Around line 467-550: Update the injected test setup and verdict assertions in
the scenario using always_pass_ids and the corresponding later test block:
capture the unique ID returned for every injected candidate, index prune
decisions by test_anchor, and assert each candidate’s exact expected decision,
reason, and failure count instead of aggregate >= 1 checks. Preserve the
existing comment-stripping and clean-exit assertions while validating the
always-pass candidates as dropped with zero failures and the engineered
violation as kept with at least one failure.

In `@tests/cli/test_prune_existing.py`:
- Around line 1521-1528: Extend the assertions in the test covering the wired
unknown dialect so every test entry’s explanation includes the live-dialect
parse-refusal text. Keep the existing test_entries, tier, and flagged_count
assertions, and use the explanation field from each entry to verify the compiler
gate specifically caused the kept-uncertain result.
- Around line 1417-1466: The test’s factory currently records only the most
recently created adapter, so duplicate constructions can go undetected. Update
the factory and assertions around _make_warehouse_adapter to retain every
BigQueryAdapter instance, then assert exactly one was created while preserving
the existing identity and single-enter checks.

🪄 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: b2ccb51f-72fd-4103-be0a-9f656100f516

📥 Commits

Reviewing files that changed from the base of the PR and between ce3986c and cc93c41.

📒 Files selected for processing (19)

• .claude/rules/business-rule-tests.md
• .claude/rules/ingest-layer.md
• .claude/rules/prune-engine.md
• .claude/rules/warehouse-adapters.md
• CHANGELOG.md
• docs/ingest-ops.md
• docs/prune-ops.md
• plans/super/270-count-scalar-prune-gaps.md
• src/signalforge/cli/prune_existing.py
• src/signalforge/ingest/_compiled_sql.py
• src/signalforge/prune/compiler.py
• tests/cli/test_e2e_bigquery_ingested_sample.py
• tests/cli/test_prune_existing.py
• tests/ingest/test_compiled_sql.py
• tests/ingest/test_manifest_tests.py
• tests/prune/test_compiler.py
• tests/prune/test_compiler_databricks.py
• tests/prune/test_compiler_fakesnow.py
• tests/prune/test_engine.py

[wjduenow]

PR Review Summary (CodeRabbit)

All 5 comments were real issues — fixed inline (no false positives, no deferrals). Fixes in b1bc5d0; the two live merge-gate tests were re-certified against real BigQuery after strengthening.

Fixed (5 items)

File	Issue	Fix
src/signalforge/ingest/_compiled_sql.py	🟠 Major — the CTE one-row classifier didn't reject row-generating projections (explode/posexplode/inline/unnest) or LATERAL VIEW before recursing, so a one-row CTE under a generator could be misclassified as scalar	Added _ROW_GENERATOR_EXPRS + a laterals/generator-projection guard before the pass-through recursion; +2 matrix cases (explode-proj, LATERAL VIEW). Degraded only to skip-record (never a wrong verdict), but it broke the "provably one row" proof — now closed.
tests/cli/test_e2e_bigquery_ingested_sample.py	🟠 Major — >= 1 dropped AND >= 1 kept would pass even if the always-pass and violation verdicts were swapped (these are the merge gate)	Added _index_ingested_by_predicate (1:1 map by the body's WHERE predicate — the model-level test_anchor isn't unique) and assert each candidate's exact verdict. Zero-count → dropped/always-passes, non-zero → kept/failures≥1. Re-run live: 3 passed.
tests/cli/test_prune_existing.py (no-double-enter)	🟡 Minor — created["adapter"] overwrite hid a possible double-construction	Track every construction in a list, assert len(created) == 1.
tests/cli/test_prune_existing.py (dialect refusal)	🟡 Minor — asserted only the kept-uncertain tier, which another uncertainty path could satisfy	Also assert each entry's why contains "does not parse under the live warehouse dialect".
.claude/rules/ingest-layer.md, .claude/rules/prune-engine.md, docs/prune-ops.md	🟡 Minor — MD018 (#270 starting a line parses as a malformed heading)	Reworded the three line-starts.

False Positives (0 items)

Invariants re-verified after the fixes: #268 fixtures byte-unchanged (d1713ba6…), parse-guard + import-guard green, sqlglot importers still 2, full suite green (4821 passed), and the live BigQuery e2e re-certified (3 passed).