#130: Snowflake estimate_query_bytes — EXPLAIN-based estimation (plan)

[wjduenow]

Summary

Super plan for #130 — replace the Snowflake --estimate degrade (#123) with a real EXPLAIN USING JSON-based estimate.

Phase: detailing (awaiting approval)
Stories: 6 implementation stories + Quality Gate + Patterns & Memory
Decisions: 9 (DEC-001 … DEC-009)

Key findings

• Dependency resolved: feat: Snowflake deterministic sampling (sample_rows HASH-mod) + materialise_sample (TEMP TABLE) #122 (connection seam) merged into dev. estimate_query_bytes is now the only unimplemented SnowflakeAdapter method — a clean single-method override, fully testable via the existing FakeSnowflakeConnection.expect_execute API.
• EXPLAIN format: EXPLAIN USING JSON → parse GlobalStats.bytesAssigned (DEC-001).
• Missing-stat fallback: raise typed EstimateUnavailableError → graceful degrade via the cli: add 'signalforge generate --estimate' for pre-flight cost preview #36 supplementary-failure path (DEC-002/003).
• Downstream churn: three feat: Snowflake estimate_query_bytes — degrade-first (EstimateNotSupportedError), EXPLAIN follow-up #123 degrade tests assert SnowflakeAdapter() raises EstimateNotSupportedError — they break under feat: Snowflake estimate_query_bytes — EXPLAIN-based estimation (Phase 2 of #123) #130 and are rewritten (DEC-007), not deleted.

Plan document

See plans/super/130-snowflake-estimate-explain.md.

Next steps

• Review the plan in this PR
• Approve in Claude Code, then say "devolve" to create beads

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Snowflake adapters now support query byte estimation using EXPLAIN USING JSON.
  • CLI now displays the source of warehouse estimates (e.g., "Snowflake EXPLAIN" or "BigQuery dryRun").

• Improvements

  • New error handling for estimation failures when query plans lack parseable byte figures.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (4)

• feature/.*
• bug/.*
• hotfix/.*
• feat/.*

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 42e689df-5327-4d53-85b9-1488ef1ed215

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR implements Snowflake's estimate_query_bytes using EXPLAIN USING JSON to extract byte counts from GlobalStats.bytesAssigned, introduces a new EstimateUnavailableError for parsing failures, and integrates the feature through CLI reporting, exit codes, and comprehensive test coverage including live certification support.

Changes

Snowflake EXPLAIN-based estimate_query_bytes implementation

Layer / File(s)	Summary
Error type definition and wiring src/signalforge/warehouse/errors.py, src/signalforge/warehouse/__init__.py, src/signalforge/cli/_helpers.py, tests/warehouse/test_errors.py, tests/cli/test_exit_codes.py	New EstimateUnavailableError is defined as a WarehouseError subclass with a detail field and locked remediation message. The error is exported through signalforge.warehouse, mapped to exit-code tier 3 in the CLI exception handler, and covered by unit tests asserting inheritance, rendering, and exit-code mapping.
Core adapter implementation: EXPLAIN parser and estimate_query_bytes src/signalforge/warehouse/adapters/snowflake.py	New _parse_explain_json_bytes(cell) helper extracts and validates GlobalStats.bytesAssigned from JSON EXPLAIN output, raising EstimateUnavailableError for missing/invalid fields or non-integer values. New _execute_scalar(sql) helper runs SQL and returns the first result cell, routing Snowflake SDK errors through the exception mapper. estimate_query_bytes(sql) validates input, executes EXPLAIN USING JSON <sql>, and parses the result or raises EstimateUnavailableError on empty results.
Unit tests for parser and adapter method tests/warehouse/test_snowflake_estimate.py	Comprehensive unit tests for _parse_explain_json_bytes covering happy paths (string JSON, dict inputs, minimal structures, zero bytes), failure modes (missing GlobalStats, non-mapping types, missing/invalid bytesAssigned field, malformed JSON, bool/float/negative values), and SnowflakeAdapter.estimate_query_bytes covering happy path, SQL validation, SQL embedding, exception mapping, empty results, and unmapped exception passthrough.
CLI estimate reporting and source labeling src/signalforge/cli/_estimate.py, tests/cli/test_estimate_render.py	New warehouse_estimate_source: str | None field added to EstimateReport to track which adapter produced the byte estimate. Populated on successful estimation using an internal adapter-class-to-label mapping (e.g., "Snowflake EXPLAIN", "BigQuery dryRun"). Render output includes the source label in the bytes-per-row line instead of hardcoding "BigQuery dryRun".
Estimate engine tests for Snowflake happy and degrade paths tests/cli/test_estimate_engine.py	Fixture-loaded EXPLAIN JSON samples and tests for estimate() with SnowflakeAdapter on FakeSnowflakeConnection. Happy-path test verifies real warehouse bytes with correct "Snowflake EXPLAIN" source label. Degrade-path test verifies missing GlobalStats reports EstimateUnavailableError, warehouse bytes become None, but LLM cost computation succeeds.
Test fixtures, README, and Snowflake stub tests tests/fixtures/warehouse/snowflake/*.json, tests/fixtures/warehouse/snowflake/README.md, tests/warehouse/test_snowflake_stub.py	Hand-crafted EXPLAIN USING JSON fixtures (with and without GlobalStats) pinned for deterministic test behavior. README documents fixture maintenance and gated live-test procedure. Snowflake stub test suite replaces prior EstimateNotSupportedError expectation with real EXPLAIN-based byte extraction tests and graceful degradation on missing stats.
Integration tests: FakeSnowflakeConnection and live certification tests/warehouse/test_snowflake_estimate_live.py, tests/cli/test_generate_estimate.py	Gated live Snowflake tests (@pytest.mark.snowflake) run only when env vars are set, exercising real EXPLAIN USING JSON execution and validating result shape against committed fixtures. End-to-end CLI tests verify generate --estimate produces real warehouse bytes and graceful degradation with correct error output.
Rules, operations docs, and planning documentation .claude/rules/warehouse-adapters.md, docs/warehouse-adapter-ops.md, plans/super/130-snowflake-estimate-explain.md, pyproject.toml	Updated warehouse-adapters.md to document Snowflake skeleton dialect contracts and graduation to EXPLAIN-based estimation. Expanded warehouse-adapter-ops.md to document BigQuery and Snowflake estimate_query_bytes behavior, error conditions, remediation, and migration paths. Comprehensive planning document (plans/super/130) details implementation strategy, testing approach, and graduation recipe. Pytest marker updated to reflect gated live EXPLAIN-estimate certification.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

• #130: Snowflake estimate_query_bytes(EXPLAIN-based estimation) — the exact feature implemented in this PR (SnowflakeAdapter.estimate_query_bytes, _parse_explain_json_bytes, EstimateUnavailableError, and comprehensive test/docs coverage).
• #118: Snowflake adapter epic — this PR advances the v0.2 skeleton by implementing the #ESTIMATE follow-up surface and corresponding test/documentation parity.

Possibly related PRs

• wjduenow/SignalForge#128: Replaces Snowflake's earlier degrade-first EstimateNotSupportedError behavior (from PR #123) with the new EXPLAIN-based implementation and updates CLI/engine/stub test expectations accordingly.
• wjduenow/SignalForge#125: Introduces the SnowflakeAdapter skeleton (#119) that left estimate_query_bytes to the ABC typed "not supported" degrade; this PR implements that method.

Poem

🐰 A rabbit hops through EXPLAIN trees,

Extracting GlobalStats with ease,

When bytes can't parse, no sorrow—

A graceful degrade saves the day!

Snowflake soars in every way. ❄️

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR implements issue #130 (Snowflake estimate_query_bytes via EXPLAIN) comprehensively, but the linked issue #122 (Snowflake deterministic sampling) is unrelated—this PR contains no sampling implementation whatsoever.	The PR fully addresses #130 but does not implement #122 sampling surfaces (sample_rows, materialise_sample). Clarify whether #122 is a dependency (should be listed as 'depends on' rather than 'linked') or remove it from linked issues.
Docstring Coverage	⚠️ Warning	Docstring coverage is 75.93% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely identifies the main change: implementing EXPLAIN-based estimation for Snowflake's estimate_query_bytes method (issue #130), accurately reflecting the PR's primary objective.
Out of Scope Changes check	✅ Passed	All changes directly support issue #130: new EstimateUnavailableError exception, Snowflake EXPLAIN-based estimation implementation, fixture-based tests, documentation, and CLI integration. No unrelated refactoring or scope creep detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

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

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[wjduenow]

@CodeRabbit review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[Copilot]

Pull request overview

This PR implements Snowflake query-bytes estimation for signalforge generate --estimate by overriding SnowflakeAdapter.estimate_query_bytes to run EXPLAIN USING JSON and parse GlobalStats.bytesAssigned, replacing the prior “not supported” degrade behavior. It also introduces a new typed EstimateUnavailableError for the “supported but no parseable stat” case, updates CLI labeling for the warehouse estimate source, and adds fixture-based + gated-live tests to pin behavior without requiring CI Snowflake access.

Changes:

• Add SnowflakeAdapter.estimate_query_bytes implementation + pure JSON-plan parser.
• Introduce EstimateUnavailableError (tier-3) and update engine/CLI/reporting to degrade gracefully when EXPLAIN lacks a byte stat.
• Add offline fixtures + unit tests + @pytest.mark.snowflake gated live certification; update docs/rules/plan accordingly.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/signalforge/warehouse/adapters/snowflake.py	Adds EXPLAIN-based estimation, scalar-exec helper, and pure bytesAssigned parser.
src/signalforge/warehouse/errors.py	Adds EstimateUnavailableError and exports it via __all__.
src/signalforge/warehouse/__init__.py	Re-exports EstimateUnavailableError at package level.
src/signalforge/cli/_helpers.py	Registers EstimateUnavailableError in exit-code mapping (tier 3).
src/signalforge/cli/_estimate.py	Adds warehouse_estimate_source to report + renders estimate source label (BQ vs Snowflake).
tests/warehouse/test_snowflake_stub.py	Updates Snowflake stub contract tests to cover real EXPLAIN estimation + missing-stat degrade.
tests/warehouse/test_snowflake_estimate.py	New: pins pure parser behavior and adapter override behavior using fixtures + fakes.
tests/warehouse/test_snowflake_estimate_live.py	New: gated live Snowflake tests validating EXPLAIN shape and non-negative parsed int.
tests/warehouse/test_errors.py	Adds coverage for EstimateUnavailableError inheritance, rendering, remediation lock, and distinction from NotSupported.
tests/cli/test_estimate_engine.py	Updates estimate-engine tests for Snowflake happy-path bytes + missing-stat degrade and source labeling.
tests/cli/test_generate_estimate.py	Updates CLI --estimate tests for Snowflake happy-path bytes + missing-stat degrade.
tests/cli/test_exit_codes.py	Adds probe construction and explicit mapping assertion for EstimateUnavailableError.
tests/cli/test_estimate_render.py	Updates renderer test report setup to include the new source label field.
tests/fixtures/warehouse/snowflake/README.md	New: documents fixture purpose and maintainer regeneration instructions.
tests/fixtures/warehouse/snowflake/explain_using_json_sample.json	New: sample EXPLAIN JSON fixture containing GlobalStats.bytesAssigned.
tests/fixtures/warehouse/snowflake/explain_using_json_no_stats.json	New: EXPLAIN JSON fixture missing GlobalStats for degrade-path tests.
docs/warehouse-adapter-ops.md	Documents Snowflake EXPLAIN estimation + accuracy caveat + new degrade error.
.claude/rules/warehouse-adapters.md	Updates adapter conventions/rules to reflect Snowflake estimation “graduation” pattern.
pyproject.toml	Updates snowflake marker description to include the gated live certification.
plans/super/130-snowflake-estimate-explain.md	Adds/updates the “super plan” document for #130.

---

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

[coderabbitai]

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tests/warehouse/test_errors.py (1)

14-35: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use warehouse public exports in tests, not private errors module paths.

At Line 14 and Line 407, import from signalforge.warehouse instead of signalforge.warehouse.errors to keep tests aligned with the package contract.

Proposed fix

-from signalforge.warehouse.errors import (
+from signalforge.warehouse import (
     BytesBilledExceededError,
     ColumnNotFoundError,
     EstimateUnavailableError,
@@
     WarehouseError,
 )
@@
-    from signalforge.warehouse.errors import EstimateNotSupportedError
+    from signalforge.warehouse import EstimateNotSupportedError

As per coding guidelines, "Package imports: import from the public API surface (re-exported names from subpackage __init__.py files) rather than private submodule paths."

Also applies to: 407-407

🤖 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/warehouse/test_errors.py` around lines 14 - 35, Replace direct imports
from the private submodule signalforge.warehouse.errors with imports from the
package public surface signalforge.warehouse; specifically update the import
statement that currently brings in BytesBilledExceededError,
ColumnNotFoundError, EstimateUnavailableError, IncompleteProfileError,
InvalidIdentifierError, ManifestProjectNotFoundError,
ManifestSchemaNotFoundError, MaterialisationFailedError,
MaterialisationNotSupportedError, ProfileNotFoundError,
ProfileTargetNotFoundError, QuerySyntaxError, SamplingError,
SamplingRequiresPartitionFilterError, TableNotFoundError, UnknownTableSizeError,
UnsupportedAuthMethodError, UnsupportedProfileTypeError, WarehouseAuthError, and
WarehouseError so they are imported from signalforge.warehouse (do the same
replacement for the second occurrence that imports the same error symbols).

🧹 Nitpick comments (1)

tests/warehouse/test_snowflake_estimate.py (1)

159-163: ⚡ Quick win

Type _record explicitly instead of suppressing with type: ignore.

This helper can be fully typed and avoid no-untyped-def suppression, which keeps pyright strictness meaningful.

As per coding guidelines, "Type annotations: Use full type annotations on all function signatures, class attributes, and module-level variables... Use # pyright: ignore sparingly with an inline comment explaining the exception."

🤖 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/warehouse/test_snowflake_estimate.py` around lines 159 - 163, Add
explicit typing to the helper: change def _record(sql: str):  # type:
ignore[no-untyped-def] to include a return annotation (e.g., def _record(sql:
str) -> Any:) and import Any from typing, and remove the no-untyped-def
suppression; then assign the function to fake._consume_execute without
suppressing method-assign by using a proper Callable type or cast (e.g.,
annotate/declare fake._consume_execute as Callable[[str], Any] or use
typing.cast when doing fake._consume_execute = _record) so the assignment and
function are fully typed; references: _record, original, fake._consume_execute.

🤖 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/warehouse-adapter-ops.md`:
- Around line 370-374: The document contains conflicting statements about
adapter overrides: the paragraph describing Snowflake running `EXPLAIN USING
JSON` and parsing `GlobalStats.bytesAssigned` (and inheriting
`EstimateNotSupportedError`) contradicts the later sentence that "v0.2 ships the
BigQuery override only"; update the latter to reflect the Snowflake behavior (or
vice versa) so both places are consistent—specifically edit the sentence
mentioning "v0.2 ships the BigQuery override only" to either include Snowflake
as also shipping an override or change the earlier Snowflake description to
indicate it is not shipped in v0.2; ensure references to "EXPLAIN USING JSON",
"GlobalStats.bytesAssigned", and "EstimateNotSupportedError" remain accurate
after the change.
- Around line 401-404: The markdown block in the Snowflake section contains
inline code spans with extra inner/trailing spaces (e.g., the literal `EXPLAIN
USING JSON`) that trigger MD038; edit the paragraph referencing
`_sql_safety.validate_test_sql` and the Snowflake override mechanism to remove
any spaces inside backticks so inline code spans are `EXPLAIN USING JSON` (no
leading/trailing spaces) and similarly fix any other inline code spans in that
block to have no inner/trailing spaces.

In `@src/signalforge/warehouse/adapters/snowflake.py`:
- Around line 840-841: In _execute_scalar, the current return logic returns a
mapping row object as-is; update it to normalize mapping rows by extracting and
returning the first cell value (not the full row). Specifically, after assigning
first = rows[0], if first is a Mapping (dict-like), return its first value
(e.g., the first value from first.values()); otherwise keep the existing
behavior that returns first[0] for sequence rows or first for scalar rows.
Ensure you reference the existing symbols rows and first inside _execute_scalar.

In `@tests/cli/test_estimate_engine.py`:
- Line 36: Replace the direct submodule import of SnowflakeAdapter with the
public package re-export: update the import statement that currently imports
SnowflakeAdapter from signalforge.warehouse.adapters.snowflake to import
SnowflakeAdapter from signalforge.warehouse so the test uses the package's
public API surface.

In `@tests/cli/test_exit_codes.py`:
- Line 589: The test imports EstimateUnavailableError from a private submodule;
change the import to use the package public surface by replacing the current
import "from signalforge.warehouse.errors import EstimateUnavailableError" with
"from signalforge.warehouse import EstimateUnavailableError" so the test
references the re-exported symbol from the warehouse package (update the import
in tests/cli/test_exit_codes.py where EstimateUnavailableError is referenced).

In `@tests/cli/test_generate_estimate.py`:
- Line 371: The test uses SnowflakeAdapter via a private import; update the test
to import SnowflakeAdapter from the package's public API (the re-export on
signalforge.warehouse) and keep the call site unchanged
(adapter=SnowflakeAdapter(connection=fake_conn)) so _install_estimate_patches
still receives the adapter instance; also update the other occurrence around the
second mention (line ~410) to use the public import as well.
- Line 40: Add an explicit module-level type annotation for the fixture path by
changing the declaration of _SNOWFLAKE_FIXTURES to include its type (e.g.,
_SNOWFLAKE_FIXTURES: Path = Path(__file__).resolve().parents[1] / "fixtures" /
"warehouse" / "snowflake"); ensure Path is imported from pathlib at the top of
the module if not already.

In `@tests/warehouse/test_snowflake_estimate_live.py`:
- Around line 66-69: The test imports private symbols directly (SnowflakeAdapter
and _parse_explain_json_bytes) from the snowflake submodule; change the imports
to use the public package re-exports instead (import SnowflakeAdapter from the
warehouse package public API) and remove/replace any usage of the private helper
_parse_explain_json_bytes with public-facing behavior or a public helper
exported by the package so the test asserts against the adapter's public API
rather than internal module symbols.

In `@tests/warehouse/test_snowflake_estimate.py`:
- Around line 143-147: The test currently uses pytest.raises(Exception) which is
too broad; change it to assert the specific validation exception thrown by the
SQL-safety check (e.g., pytest.raises(ValidationError) or the concrete
QueryValidationError used in your codebase) when calling
adapter.estimate_query_bytes("SELECT 1; DROP TABLE x"), import that exception at
top of the test, keep the assert that "unexpected query" not in
str(excinfo.value), and leave fake.assert_all_expectations_met() unchanged so
the test only passes for the intended validation reject path.

In `@tests/warehouse/test_snowflake_stub.py`:
- Around line 34-37: Replace direct imports from internal modules by importing
re-exported symbols from the package public API: stop importing SnowflakeAdapter
from signalforge.warehouse.adapters.snowflake and EstimateUnavailableError from
signalforge.warehouse.errors; instead import SnowflakeAdapter and
EstimateUnavailableError from signalforge.warehouse while leaving other imports
(e.g., WarehouseAdapter, SNOWFLAKE_DIALECT, Dialect, TableRef) unchanged or also
moved to the public surface if available; update the import statements in
tests/warehouse/test_snowflake_stub.py to reference SnowflakeAdapter and
EstimateUnavailableError via signalforge.warehouse.

---

Outside diff comments:
In `@tests/warehouse/test_errors.py`:
- Around line 14-35: Replace direct imports from the private submodule
signalforge.warehouse.errors with imports from the package public surface
signalforge.warehouse; specifically update the import statement that currently
brings in BytesBilledExceededError, ColumnNotFoundError,
EstimateUnavailableError, IncompleteProfileError, InvalidIdentifierError,
ManifestProjectNotFoundError, ManifestSchemaNotFoundError,
MaterialisationFailedError, MaterialisationNotSupportedError,
ProfileNotFoundError, ProfileTargetNotFoundError, QuerySyntaxError,
SamplingError, SamplingRequiresPartitionFilterError, TableNotFoundError,
UnknownTableSizeError, UnsupportedAuthMethodError, UnsupportedProfileTypeError,
WarehouseAuthError, and WarehouseError so they are imported from
signalforge.warehouse (do the same replacement for the second occurrence that
imports the same error symbols).

---

Nitpick comments:
In `@tests/warehouse/test_snowflake_estimate.py`:
- Around line 159-163: Add explicit typing to the helper: change def
_record(sql: str):  # type: ignore[no-untyped-def] to include a return
annotation (e.g., def _record(sql: str) -> Any:) and import Any from typing, and
remove the no-untyped-def suppression; then assign the function to
fake._consume_execute without suppressing method-assign by using a proper
Callable type or cast (e.g., annotate/declare fake._consume_execute as
Callable[[str], Any] or use typing.cast when doing fake._consume_execute =
_record) so the assignment and function are fully typed; references: _record,
original, fake._consume_execute.

🪄 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: 3882ca6d-300b-4f62-bb27-5304c6194726

📥 Commits

Reviewing files that changed from the base of the PR and between f903570 and a1688b7.

📒 Files selected for processing (20)

• .claude/rules/warehouse-adapters.md
• docs/warehouse-adapter-ops.md
• plans/super/130-snowflake-estimate-explain.md
• pyproject.toml
• src/signalforge/cli/_estimate.py
• src/signalforge/cli/_helpers.py
• src/signalforge/warehouse/__init__.py
• src/signalforge/warehouse/adapters/snowflake.py
• src/signalforge/warehouse/errors.py
• tests/cli/test_estimate_engine.py
• tests/cli/test_estimate_render.py
• tests/cli/test_exit_codes.py
• tests/cli/test_generate_estimate.py
• tests/fixtures/warehouse/snowflake/README.md
• tests/fixtures/warehouse/snowflake/explain_using_json_no_stats.json
• tests/fixtures/warehouse/snowflake/explain_using_json_sample.json
• tests/warehouse/test_errors.py
• tests/warehouse/test_snowflake_estimate.py
• tests/warehouse/test_snowflake_estimate_live.py
• tests/warehouse/test_snowflake_stub.py

[wjduenow]

PR Review Summary

All 5 review comments addressed (commit d02b050 + plan/PR-body alignment). No false positives — every comment was a real fix.

Fixed (5 items)

File	Line	Issue	Resolution
src/signalforge/warehouse/adapters/snowflake.py	837	_execute_scalar cursor never closed	Close cursor in a finally (success + both raise paths)
src/signalforge/warehouse/adapters/snowflake.py	145	Docstring "NEVER 0" inconsistent with tests treating bytesAssigned=0 as valid	Reworded: explicit 0 is valid; "never 0" forbids only a fabricated fallback
tests/warehouse/test_snowflake_estimate.py	143	pytest.raises(Exception) too broad	Narrowed to QuerySyntaxError (what validate_test_sql raises on ;)
tests/fixtures/warehouse/snowflake/README.md	6	Inline code span split across newline broke Markdown	Fully-qualified parser path kept on one line
plans/super/130-...md / PR body	9	PR body said "detailing (awaiting approval)" vs plan "devolved"	PR body updated to "implemented / devolved" to match

False Positives (0 items)

None.

All review threads resolved. Validation green: 2369 passed, snowflake.py 100%.

[wjduenow]

PR Review Summary (CodeRabbit batch)

All 10 CodeRabbit threads addressed in commit e80c1a1. No false positives.

Fixed (10 items)

File	Issue	Resolution
src/.../adapters/snowflake.py	_execute_scalar returned a whole DictCursor row instead of the first cell	Normalise mapping rows to next(iter(first.values())) + regression test
tests/cli/test_estimate_engine.py	SnowflakeAdapter imported from private submodule	Promoted SnowflakeAdapter to the public signalforge.warehouse surface (matches BigQueryAdapter); import migrated
tests/cli/test_generate_estimate.py (import)	same	migrated to public surface
tests/cli/test_generate_estimate.py:40	module-level var lacked annotation	_SNOWFLAKE_FIXTURES: Path
tests/cli/test_exit_codes.py:589	EstimateUnavailableError from private submodule	from signalforge.warehouse
tests/warehouse/test_snowflake_stub.py	SnowflakeAdapter + EstimateUnavailableError private imports	public surface
tests/warehouse/test_snowflake_estimate_live.py	SnowflakeAdapter private import	public; _parse_explain_json_bytes kept as a documented white-box import (no public seam exists for a private pure helper)
tests/warehouse/test_snowflake_estimate.py	over-broad pytest.raises(Exception)	already narrowed to QuerySyntaxError in d02b050
docs/warehouse-adapter-ops.md (override status)	"v0.2 ships the BigQuery override only" contradicted the Snowflake-override paragraph	reworded: BigQuery + Snowflake override, Postgres inherits
docs/warehouse-adapter-ops.md (MD038)	trailing space inside `EXPLAIN USING JSON ` code span	removed in-span space; trailing space described in prose

Consistency bonus: migrated all 8 Snowflake test files (incl. pre-existing #119/#122 ones) to the public SnowflakeAdapter import so the convention is uniform.

False Positives (0)

None.

Validation green: 2370 passed, snowflake.py 100%, ruff/pyright/mkdocs clean.