22: Q4=C temp-table-materialised sample (impl) - #31
Conversation
Path B per super-plan workflow: implementation branches off `dev` while plan PR #30 stays open on feature/22-temp-table-sample. Cherry-pick the plan file (squashed; full history remains on the plan PR) so worker subagents in `bd worktree` worktrees can read DEC-001 through DEC-014 when implementing US-001 through US-011. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…dError + MaterialisationNotSupportedError
…-002) Adds the materialise_sample(table, n, *, partition_filter=None, ttl_seconds=3600) -> TableRef method to WarehouseAdapter per DEC-004 of plans/super/22-temp-table-sample.md. Default impl raises MaterialisationNotSupportedError (DEC-008) — deliberately NOT decorated @AbstractMethod because the typed raise IS the v0.2 contract for non-BQ adapters; concrete adapters override (BigQuery in US-003). Two TDD tests pin the contract: - default impl raises MaterialisationNotSupportedError carrying the DEC-006 remediation text verbatim (locked operator-facing surface) - inspect.signature pins kw-only separator, defaults, and TableRef return annotation; drift on any one fails the test loud Traces: DEC-004, DEC-006, DEC-008. Validation passes (ruff + ruff format + pyright + pytest with the 5 documented deselects: 7th AST scan blocked by US-007 + 4 pre-existing symlink-loop env failures).
…ample ABC method + default impl
…table Lands the two materialisation-seam typed errors (US-001) in the ``signalforge.cli._helpers._EXCEPTION_TO_EXIT_CODE`` mapping at tier 3 (external-dep / fail-closed) per DEC-008 of US-007 in ``plans/super/22-temp-table-sample.md``. ``MaterialisationFailedError`` wraps any SDK / network / quota failure during the per-run materialise query (BigQuery CTAS / equivalent); ``MaterialisationNotSupportedError`` is the ``WarehouseAdapter`` ABC default-impl raise that fires when a non-BigQuery v0.2 adapter has not overridden ``materialise_sample``. Both are tier-3 inheritances from ``WarehouseError`` but get explicit per-class entries so the 7th AST scan (``tests/test_audit_completeness.py::test_every_typed_error_is_in_exit_code_mapping_table``) passes — the scan asserts every concrete ``*Error`` declaration in any ``src/signalforge/*/errors.py`` appears as its own key in the mapping (per DEC-024 of #9 / ``.claude/rules/cli-layer.md``). Tests: * Adds explicit per-class branches for both errors in ``_construct_exception`` so the construction shape is documented at the test seam (``MaterialisationFailedError`` follows the ``cause=`` kwarg pattern; ``MaterialisationNotSupportedError`` takes a positional adapter name). The parametrized contract picks the new entries up automatically because ``_PARAMS`` derives from ``_EXCEPTION_TO_EXIT_CODE.items()``. * ``test_materialisation_failed_error_maps_to_tier_3`` and ``test_materialisation_not_supported_error_maps_to_tier_3`` — non-parametrized callouts mirroring the precedent set by ``test_table_not_found_error_exits_tier_two`` so a future tier-change diff is easy to read in code review. * ``test_audit_completeness_scan_passes_for_new_errors`` — pins the mapping membership + tier assignment at the per-class level so a regression names the offending class up front instead of only in the broader 7th-scan failure list. Validation: ``ruff check`` clean, ``ruff format --check`` clean, ``pyright`` 0/0/0, ``pytest`` 1387 passed (2 skipped platform-specific, 15 deselected: the 4 known-environmental symlink-loop tests plus the always-deselected ``bigquery`` / ``anthropic`` / ``cli_subprocess`` markers). Coverage 94.91% (above the 80% floor). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Implements DEC-001/002/003/013/014 of plans/super/22-temp-table-sample.md.
Production:
- BigQueryAdapter.materialise_sample(table, n, *, partition_filter=None,
ttl_seconds=3600) -> TableRef. Computes deterministic run_id =
blake2b-8(table.qualified_name + signalforge_version + n +
canonical_json(partition_filter)) for snapshot determinism (DEC-001;
16 hex chars so the temp-table name passes validate_identifier).
- Issues CREATE TEMP TABLE _sf_sample_<run_id> AS SELECT ... with
QueryJobConfig(create_session=True, use_query_cache=False, ...
stage="warehouse_sample_materialise"). Captures the BQ-assigned
session_id from job.session_info.session_id (NOT minted by us).
- Stores _active_session_id + _session_started_at + _session_ttl_seconds;
run_test_sql now threads connection_properties=[ConnectionProperty(
key="session_id", value=...)] when active.
- One INFO log on success: {"table","sample_rows","session_id_hash":
blake2b-4(session_id),"run_id","duration_ms"} — DEC-003 redaction
(raw session_id never leaks on the happy path).
- __exit__ extends DEC-013/014 cleanup: CALL BQ.ABORT_SESSION() in the
active session; success → INFO {"session_id_hash","ttl_remaining_
seconds"}; failure → multi-line WARNING with raw session_id + manual
bq command + "auto-expire in <N>s" line (DEC-014, the deliberate
exception to DEC-003 — the bq command is unconstructable without
the raw id). Always resets state in finally.
- SDK noise contained in _client.py: _make_query_job_config now accepts
create_session= and session_id= kwargs.
Tests (22 new):
- tests/warehouse/test_materialise_sample.py — covers TableRef shape,
identifier validation, run_id determinism, version-bump invalidation,
byte-equal CTAS SQL fixture, stage label, partition filter placement,
create_session config, MaterialisationFailedError wrapping,
run_test_sql session routing, INFO redaction, use_query_cache invariant,
__exit__ cleanup happy/failure paths, raw-session-id-in-WARNING,
manual-bq-command line, TTL line, exception-class-name, state reset,
no-WARNING-on-success.
- tests/fixtures/warehouse/sample_materialise_v1.sql — pinned CTAS
template with {run_id} placeholder (substituted at test-time so
signalforge.__version__ bumps don't require fixture refresh).
Validation: ruff check + ruff format --check + pyright + pytest pass
(1412 passed, 2 skipped, 15 deselected per the 4 environmental symlink
deselects). 7th AST scan still green; logger grep gate still green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…mple (sessions + temp table + cleanup)
…ect_abort_session (US-004 of #22) Two purpose-built helpers on FakeBigQueryClient mirroring the production surface for BigQueryAdapter.materialise_sample (CTAS into _SESSION) and __exit__'s CALL BQ.ABORT_SESSION cleanup path. Each helper consumes one matching call; non-matching calls raise the standard "unexpected ..." AssertionError. Both queues short-circuit only when populated so US-003's existing raw expect_query matchers continue to work unchanged. expect_materialise_sample(source_ref, sample_size, partition_filter=None, *, returns: TableRef | Exception) — matches a CREATE TEMP TABLE _sf_sample_ SQL referencing source_ref's qualified name + LIMIT <n> + (when registered) the rendered partition_filter fragment. Success returns a job with a populated session_info.session_id (deterministic, derived from the returned TableRef name) so production captures it via job.session_info after .result(). Exception returns propagate, driving the MaterialisationFailedError wrapping path. expect_abort_session(session_id, *, returns=None | Exception) — matches a CALL BQ.ABORT_SESSION query whose job_config carries the registered session_id via connection_properties. returns=None simulates success; returns=Exception drives the DEC-014 swallow-and-warn WARNING path on __exit__. Eight meta-tests pin the contract: consume-one-call + assertion-error on second call (both helpers); returns=Exception propagation (both helpers); assert_all_expectations_met fails on unconsumed materialise registration; session_id mismatch on abort raises loudly; happy-path returns=None on abort iterates an empty rowset; partition_filter contract — registering a filter requires the matched SQL to carry the rendered fragment. Traces: R-TEST-3, DEC-013 of plans/super/22-temp-table-sample.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…onservative routing (US-005 of #22)
… generate (US-006 of #22) Both flags are optional and independent (set one, the other, both, or neither). Override application uses PruneConfig.model_validate(...) so every Pydantic validator re-runs (DEC-012); never model_copy(update=...) because that path silently skips @model_validator(mode='after'). Mirrors SafetyPolicy.with_mode (DEC-018 of safety-layer.md) and DiffConfig.render_kind graduation in #9 (DEC-020 of cli-entrypoint). Multi-surface parity (cli-layer.md): help text, add_parser docstring, cmd_generate docstring, module docstring, test names, and the DEC trace all aligned in one commit. Argparse choices rejection produces tier-2 exit (no traceback per DEC-016). Eight tests pin DEC-011 + DEC-012: - test_generate_scope_flag_overrides_config_value - test_generate_sample_strategy_flag_overrides_config_value - test_generate_both_flags_independent - test_generate_no_flag_uses_config_value - test_generate_invalid_scope_returns_exit_2 - test_generate_invalid_sample_strategy_returns_exit_2 - test_generate_help_text_lists_new_flags - test_generate_override_re_runs_pydantic_validators Traces: DEC-011, DEC-012 of plans/super/22-temp-table-sample.md. Files: src/signalforge/cli/generate.py, tests/cli/test_generate.py. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…S-008 of #22) Restructure tests/warehouse/test_sample_cost_probe.py into three @pytest.mark.bigquery tests (DEC-007 + DEC-013 of plans/super/22-temp-table-sample.md): * test_sample_rows_cost_baseline_oneshot — preserves AR-B1's 9.92 GB measurement as a regression guard. Asserts bytes_billed >= _BYTES_WARN_AT (cost cliff genuinely exists on the legacy path) AND bytes_billed < _BYTES_CEILING (sanity ceiling). * test_sample_rows_cost_materialised — issue's primary acceptance criterion. Drives materialise_sample + a per-test query through the adapter; asserts per-test bytes_billed < 100 MB. * test_materialised_session_cleaned_up_after_exit — positive proof of DEC-013. After __exit__ fires, querying the _SESSION._sf_sample_<run_id> temp table by name fails with a GoogleAPIError (NotFound / "session not found" / "table not found"). A buggy implementation that no-op'd CALL BQ.ABORT_SESSION() would make the query SUCCEED and pytest.raises would fail with "DID NOT RAISE", so the test cannot pass on a broken cleanup path. All three are SF_RUN_BQ-gated (truthy values: 1/true/yes/on); default pytest skips them via the addopts marker exclusion. Maintainer runs `SF_RUN_BQ=1 pytest -m bigquery tests/warehouse/test_sample_cost_probe.py --no-cov` before declaring the PR ready (--no-cov per testing-signal.md Coverage section, since --cov-fail-under in addopts would otherwise fail the marker-only run). Adds two default-collected scaffolding tests: * test_probe_module_imports_and_exposes_three_test_functions — pins the module shape and asserts each live-BQ test carries the bigquery marker via pytestmark introspection. * test_probe_constants_unchanged — pins _BYTES_CEILING (5 GB), _BYTES_WARN_AT (500 MB), and _BYTES_PER_TEST_TARGET (100 MB) so a silent threshold edit can't soften the regression contract. Validation: ruff check . && ruff format --check . && pyright && pytest all pass; 1437 tests pass, 17 deselected (markers), 2 skipped (platform). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Update six documentation surfaces in lockstep for the v0.2 temp-table materialised-sample work tracked in plans/super/22-temp-table-sample.md (US-009). All updates are docs-only; no production code changes. - docs/prune-ops.md: document `sample_strategy` config field; add post-Q4=C cost subsection with TBD placeholders for US-010 quality gate; add audit reading guide distinguishing materialised vs. oneshot `compiled_sql` shapes. - docs/warehouse-adapter-ops.md: add `warehouse_sample_materialise` stage label; document `materialise_sample` ABC method, BigQuery session-state pattern, v0.2 → v0.3 migration story; add Session cleanup & manual recovery section covering the three-layer cleanup model (DEC-013) and the verbatim manual `bq query` command (DEC-014); add INFORMATION_SCHEMA.JOBS_BY_PROJECT query template for spotting orphan sessions; extend error-reference table. - .claude/rules/prune-engine.md: add v0.2 reservations / additions section covering `sample_strategy`, the two new typed errors, `_SESSION._sf_sample_<run_id>` audit signal, conservative-bias routing + degraded-run WARNING (DEC-009), total-budget-includes- materialisation invariant (DEC-010), and the context-manager requirement (DEC-013). - .claude/rules/warehouse-adapters.md: document `materialise_sample` ABC + BigQuery session-state pattern; add Best-effort cleanup in `__exit__` sub-section with WARNING shape verbatim; cross-reference `safety-layer.md` DEC-011 for the primary-work fail-closed contrast. - CLAUDE.md: amend "Public API surface (v0.1)" to "(v0.1 + v0.2 additions)"; list four new exports under v0.2. - docs/cli-ops.md: add Stderr shapes (WARNING) section with three WARNING entries (cleanup-failure, materialisation-failure / degraded-run, budget-exceeded) for operator parity. Validation passes: ruff check + ruff format --check + pyright + the 1445-test default suite (with the four pre-existing symlink-loop deselects from the worktree pattern). No production code touched. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four concerns surfaced by the multi-lens reviewer pass; all four addressed inline (no blockers). 1. docs/cli-ops.md "Runtime knob flags" missing --scope and --sample-strategy entries (5-surface parity violation per cli-layer.md). Added two bullets mirroring the help text in cli/generate.py. 2. .claude/rules/prune-engine.md run_id digest claim was wrong: said "blake2b-12(model.unique_id + ...)" but production uses "blake2b(table.qualified_name + ..., digest_size=8)" (16 hex chars). Updated rule to match the implementation; preserves the "16-hex" claim consistently. 3. tests/llm/test_logger_grep_gate.py extended to cover src/signalforge/warehouse/ (was 6 dirs; now 7). US-003 introduced new logger calls in the warehouse layer; existing calls comply with lazy-format pattern, but adding the gate prevents future regressions. 4. docs/warehouse-adapter-ops.md "Session cleanup & manual recovery" section gets a new "Edge case: SDK returns session_info=None" sub-section documenting the v0.3-tracked path where BQ creates a session server-side but the SDK doesn't surface the id (would orphan until BQ's own timeout). Out-of-session deferred to maintainer: - 3 more code-reviewer passes (skill says 4; did 1 multi-lens). - CodeRabbit review (PR-time, automatic when PR opens). - Maintainer probe-run: SF_RUN_BQ=1 pytest -m bigquery tests/warehouse/test_sample_cost_probe.py --no-cov against a real BQ project. - Cost-figure substitution in docs/prune-ops.md (replace four <TBD: ...> placeholders with the maintainer's measured figures). Validation: 1445 passed, 2 skipped, 17 deselected (5 known: 4 environmental symlink-loop + the AST scan was the v0.2 incomplete state which closed when US-007 landed). ruff + format + pyright all clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-011 of #22) Rule-file additions (generic, durable lessons — not duplicating US-009's issue-specific notes): - warehouse-adapters.md: "Session/connection state on the adapter" generic pattern (DEC-002 of #22 generalised) for v0.3 adapters needing per-call state primitives; "Cleanup-boundary fail-soft pattern" generic pattern (DEC-013/DEC-014 of #22 generalised) with the three-thing WARNING contract (identifier + copy-pasteable command + durable fallback) and the --quiet-doesn't-suppress invariant. - safety-layer.md: paragraph near DEC-011 distinguishing primary-work fail-closed (this rule) from cleanup-boundary fail-soft (warehouse-adapters DEC-013/DEC-014) so a v0.3 maintainer doesn't conflate them. - prune-engine.md: refines C8 (5-value DropReason taxonomy) with the conservative-bias-routing-across-WarehouseError-subclasses paragraph (generalises DEC-009 of #22 to any v0.3 warehouse exception); adds the 5-surface parity rule (rule file / ops doc / CLAUDE.md / test / DEC) at the top of the v0.2 reservations section, mirroring cli-layer.md's flag parity rule. - testing-signal.md: one-paragraph note on seeded determinism over snapshot normalisation (mirrors DEC-001 of #22 + LLM drafter prompt_version). bd remember invocations (2): BQ session creation latency observation (~700ms-2.5s on US-003 worktree builds; informs DEC-010 budget calibration) and BQ session abort failure rate expectation (rare; revisit DEC-013 if >1% of prune runs hit the swallow-and-warn path). Validation: ruff/pyright/pytest all green. 1445 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMaterialises a deterministic per-run sample for prune "sample" scope: new PruneConfig.sample_strategy defaulting to "materialised", WarehouseAdapter.materialise_sample ABC and BigQuery implementation with session lifecycle and cleanup, prune orchestration materialise-once path with conservative fail‑closed routing on materialisation WarehouseError, CLI flags, fakes, fixtures, and broad tests. ChangesMaterialised Sample Strategy (single cohort)
Sequence Diagram(s) sequenceDiagram
participant CLI
participant Orchestrator
participant Adapter
participant BigQuery
CLI->>Orchestrator: run prune (with PruneConfig)
Orchestrator->>Adapter: with adapter: materialise_sample()
Adapter->>BigQuery: CTAS (create_session=True)
BigQuery-->>Adapter: job.session_info.session_id
Adapter->>Adapter: store session state
Orchestrator->>Adapter: compile & run per-test SQL (routed to session)
Orchestrator->>Adapter: __exit__
Adapter->>BigQuery: CALL BQ.ABORT_SESSION()
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Maintainer probe-run (2026-05-08, billed to duenow-nest) surfaced one
real production bug + three probe-design fixes. All four landed
together so the next probe-run from a clean clone Just Works.
## Production fix
`BigQueryAdapter.materialise_sample` returned
`TableRef(project=client.project, dataset="_SESSION", name=...)`. The
three-part `<project>._SESSION._sf_sample_<run_id>` qualified_name is
rejected by BigQuery even inside the owning session:
400 Use of _SESSION is not allowed here; reason: invalid
Within a session, the temp table must be referenced as the two-part
`_SESSION._sf_sample_<run_id>`. Fix: return `project=None` so
`TableRef.qualified_name` renders the two-part form.
This bug shipped through US-003's unit tests because `FakeBigQueryClient`
matches on regex shape, not SQL semantics — only a real-BQ run could
catch it. Updated the production code, the matching unit test pin, the
prune fixture's `compiled_sql` references, and the test that uses a
literal `fake_project._SESSION._sf_sample_x` SQL string.
## Probe fixes
1. `_BYTES_CEILING` raised from 5 GB → 15 GB. AR-B1 measured 9.92 GB;
the original 5 GB ceiling was internally inconsistent with the
recorded figure (the regression-guard would always fail).
2. `BigQueryAdapter()` instantiations bumped from the 100 MB default
cap to 20 GB (`_BOOTSTRAP_BYTES_BILLED_CAP`). The 100 MB DEC-005 cap
blocks even the materialised CTAS bootstrap (it scans the source
table once, ~10 GB). The probe needs a higher cap to MEASURE
figures; the production safety net stays at 100 MB for end users.
v0.3 may want a per-stage cap override; tracked in PR #31's
maintainer-follow-ups section.
3. Per-test query in `test_sample_rows_cost_materialised` was
`SELECT COUNT(*) FROM (SELECT * FROM <temp> WHERE FALSE) AS t`. BQ's
planner short-circuited on `WHERE FALSE` and billed 0 bytes — the
probe's `int(getattr(...) or -1)` bug then converted 0 to -1
("unavailable") and xfailed. Replaced with a representative
`not_null` test against the `invoice_and_item_number` column +
fixed the `or -1` bug to treat None as the unavailable sentinel
(0 is a valid measurement).
## Cost figures recorded in docs/prune-ops.md
- Oneshot baseline: 9,984,540,672 bytes (~9.98 GB) per query — matches
AR-B1 within 1%.
- Materialisation CTAS: ~9.98 GB once per prune run.
- Per-test on materialised: 10,485,760 bytes (10 MB) — well under the
100 MB acceptance gate.
- 30-test run: ~10.3 GB end-to-end (vs. ~299 GB on the legacy oneshot
path) → ~29× cheaper.
Issue #22 acceptance #1 ("per-test bytes_billed < 100 MB without
raising cost_limit_bytes") is satisfied — the per-test queries against
the temp table run cleanly under the 100 MB default cap; only the
maintainer probe needs the bumped cap for the bootstrap measurement.
Validation: ruff + format + pyright + 1445-test pytest all green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Implements issue #22’s v0.2 default temp-table materialised sampling strategy for sample-mode prune: materialise a deterministic sample once per run (BigQuery session temp table), then run all candidate tests against that sample to avoid the per-test full-row scan cost cliff.
Changes:
- Add
PruneConfig.sample_strategyand plumb strategy dispatch throughprune_testswith conservative-bias routing on materialisation failures. - Add
WarehouseAdapter.materialise_sampleABC method + BigQuery implementation using sessions, including best-effort session cleanup on__exit__. - Extend fakes, CLI flags/exit-code mapping, probes, fixtures, and ops docs to cover the new strategy and observability.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/warehouse/test_sample_cost_probe.py | Splits the BigQuery cost probe into baseline/materialised/cleanup tests and updates thresholds/scaffolding. |
| tests/warehouse/test_materialise_sample.py | Adds unit tests for BigQueryAdapter.materialise_sample behavior, session routing, and cleanup. |
| tests/warehouse/test_fake.py | Adds tests for new fake helpers (expect_materialise_sample, expect_abort_session). |
| tests/warehouse/test_errors.py | Adds coverage for new warehouse typed errors and updates exported error count expectations. |
| tests/warehouse/test_base.py | Pins the new ABC method signature and default “not supported” behavior. |
| tests/warehouse/_fake.py | Extends the BigQuery client fake with materialise/abort expectation queues and session-info support. |
| tests/prune/test_engine.py | Adds extensive coverage for sample-strategy dispatch, conservative routing, budget semantics, and context-manager usage. |
| tests/prune/test_config.py | Adds validation tests for sample_strategy literals, defaults, and backward-compatible YAML loading. |
| tests/llm/test_logger_grep_gate.py | Extends the f-string logger grep gate to include signalforge.warehouse. |
| tests/fixtures/warehouse/sample_materialise_v1.sql | Adds a pinned CTAS SQL fixture for materialisation. |
| tests/fixtures/prune/prune_event_v1.jsonl | Updates the audit fixture to include materialised-sample compiled SQL shapes and failure examples. |
| tests/cli/test_generate.py | Adds tests for --scope / --sample-strategy CLI flag overrides and help output. |
| tests/cli/test_exit_codes.py | Adds explicit tier-3 mapping tests for new materialisation error types. |
| src/signalforge/warehouse/errors.py | Introduces MaterialisationFailedError and MaterialisationNotSupportedError and exports them. |
| src/signalforge/warehouse/base.py | Adds WarehouseAdapter.materialise_sample(...) default implementation (raises not-supported typed error). |
| src/signalforge/warehouse/adapters/bigquery.py | Implements session-backed sample materialisation, session routing via connection properties, and __exit__ cleanup. |
| src/signalforge/warehouse/adapters/_client.py | Extends the BigQuery job-config builder to support session creation and session routing. |
| src/signalforge/warehouse/init.py | Re-exports new materialisation error types as part of the warehouse public surface. |
| src/signalforge/prune/engine.py | Adds strategy dispatch, degraded-run warning + conservative routing, and wraps adapter usage in with adapter:. |
| src/signalforge/prune/config.py | Adds sample_strategy: Literal["oneshot","materialised"] with default materialised. |
| src/signalforge/cli/generate.py | Adds --scope and --sample-strategy flags and applies overrides via PruneConfig.model_validate. |
| src/signalforge/cli/_helpers.py | Registers new materialisation errors in the CLI tier mapping. |
| docs/warehouse-adapter-ops.md | Documents materialised sampling, sessions, cleanup behavior, and error reference updates. |
| docs/prune-ops.md | Documents sample_strategy, cost model changes, and audit-reading guidance for materialised sampling. |
| docs/cli-ops.md | Documents new runtime flags and new WARNING stderr shapes for degraded runs/cleanup. |
| CLAUDE.md | Updates public API surface to include v0.2 materialisation additions. |
| .claude/rules/warehouse-adapters.md | Captures the adapter session-state and cleanup-boundary patterns for future adapter work. |
| .claude/rules/testing-signal.md | Adds guidance favoring seeded determinism for fixture-stable identifiers. |
| .claude/rules/safety-layer.md | Clarifies fail-closed (primary work) vs fail-soft (cleanup boundary) pattern distinction. |
| .claude/rules/prune-engine.md | Documents new v0.2 strategy behavior, conservative routing, warning shapes, and parity expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…uting Adds 8 targeted tests covering the 11 patch-attributable lines Codecov flagged on PR #31: * materialise_sample fail-loud sizing parity with sample_rows: - n <= 0 ValueError pre-call (DEC-008 input validation) - UnknownTableSizeError when num_rows is None and no partition filter - default-bucket fallback when num_rows is None but partition filter pins cost - SamplingRequiresPartitionFilterError when num_rows >= 100M and no partition * materialise_sample fail-loud post-CTAS guard: - MaterialisationFailedError when SDK returns no session_id * prune engine defence-in-depth + setup-error propagation: - invalid-identifier compile result routes to kept-without-evidence - WarehouseError during sample-mode size fetch propagates as-is * TTL helper defensive guard: - _compute_ttl_remaining_seconds returns 1 when session state unset Total signalforge coverage: 95.00% -> 95.30%. Remaining uncovered lines in bigquery.py / prune/engine.py are all pre-existing code from issue #3, outside this PR's diff hunks; no patch lines are uncovered after this commit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/warehouse/_fake.py (1)
321-367:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert
create_session=Trueon materialisation queries.
expect_materialise_sample()matches on SQL only and then fabricatesjob.session_infoeven if the adapter never asked BigQuery to open a session. That means the fake can pass while production would return nosession_idand break the follow-up_SESSIONreads.Suggested fix
- if self._materialise_sample_expectations and _is_materialise_sample_sql(sql): - return self._consume_materialise_sample(sql) + if self._materialise_sample_expectations and _is_materialise_sample_sql(sql): + return self._consume_materialise_sample(sql, job_config) ... - def _consume_materialise_sample(self, sql: str) -> _FakeQueryJob: + def _consume_materialise_sample(self, sql: str, job_config: Any) -> _FakeQueryJob: + if not getattr(job_config, "create_session", False): + raise AssertionError( + f"unexpected materialise_sample: create_session=True was not set, sql={sql!r}" + ) for i, exp in enumerate(self._materialise_sample_expectations): if not _materialise_sample_matches(sql, exp): continue🤖 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/_fake.py` around lines 321 - 367, The fake materialise-sample handler is creating a session unconditionally; update _consume_materialise_sample to assert that the matched expectation requested a session (check the expectation's create_session flag on the matched exp) before synthesising _FakeQueryJobWithSession — if exp.create_session is False or missing, either return a non-session _FakeQueryJob(rows=[]) or raise an AssertionError signalling that the production code didn't request a session; change the session creation branch that currently calls _derive_fake_session_id(...) and returns _FakeQueryJobWithSession to only run when exp.create_session is truthy.src/signalforge/warehouse/adapters/bigquery.py (1)
213-236:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways run session cleanup from
__exit__.On a clean exit, a
_flush_column_stats_batch()failure re-raises before_cleanup_active_session()runs, so an already-open materialisation session is left for BigQuery’s server timeout instead of the explicit abort path.Suggested fix
def __exit__( self, exc_type: object, exc: object, tb: object, ) -> None: + flush_error: Exception | None = None try: pending = self._column_stats_pending or {} for table in list(pending.keys()): if not pending[table]: continue try: self._flush_column_stats_batch(table) - except Exception: + except Exception as err: if exc_type is None: - raise + flush_error = err + break finally: self._table_metadata_cache = None self._column_stats_pending = None self._column_stats_results = None - - self._cleanup_active_session() + self._cleanup_active_session() + if flush_error is not None: + raise flush_error🤖 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 `@src/signalforge/warehouse/adapters/bigquery.py` around lines 213 - 236, The __exit__ path currently lets exceptions from _flush_column_stats_batch re-raise before _cleanup_active_session runs, leaving an open BigQuery session; fix by invoking self._cleanup_active_session() inside the same finally block (before or after clearing caches) so it always runs even when _flush_column_stats_batch raises, and wrap the cleanup call in a small try/except that logs cleanup failures but does not swallow an existing exception (re-raise if exc_type is not None) — update the code around __exit__ so _cleanup_active_session is called from the finally and follows the best-effort logging semantics.tests/warehouse/test_sample_cost_probe.py (1)
225-229:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe baseline probe is not measuring the real oneshot query shape.
This helper says it reproduces the production deterministic-sample SQL byte-for-byte, but it drops the
ORDER BY FARM_FINGERPRINT(TO_JSON_STRING(t))clause used to make the truncated sample deterministic. That makes the baseline bytes-billed figure non-comparable to the actual oneshot path this probe is supposed to guard.🔧 Suggested fix
sql = ( f"SELECT * FROM `{target.qualified_name}` AS t " f"WHERE MOD(ABS(FARM_FINGERPRINT(TO_JSON_STRING(t))), {bucket}) < 1 " + f"ORDER BY FARM_FINGERPRINT(TO_JSON_STRING(t)) " f"LIMIT {sample_size}" )🤖 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_sample_cost_probe.py` around lines 225 - 229, The SQL built in the sql variable omits the deterministic ordering used in production; update the construction of sql (the string built using target.qualified_name, FARM_FINGERPRINT(TO_JSON_STRING(t)) and bucket/sample_size) to include the same ORDER BY FARM_FINGERPRINT(TO_JSON_STRING(t)) clause immediately before the LIMIT so the probe's bytes-billed baseline matches the real oneshot query shape.src/signalforge/prune/engine.py (1)
739-762:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCanonicalise the audit path before creating its parent directory.
raw_audit_path.parent.mkdir(...)runs before the containment check, so an out-of-treeaudit_pathcan still create directories outsideproject_direven though the latercanonicalise_path(...)rejects the write. That weakens the symlink-hardening guarantee and leaves a filesystem side effect on an untrusted path.As per coding guidelines, "Use canonicalised symlink-hardened paths throughout all subpackages to prevent path traversal attacks".
🤖 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 `@src/signalforge/prune/engine.py` around lines 739 - 762, Ensure we canonicalise the audit path before creating any parent directories: keep creating resolved_project_dir via resolved_project_dir.mkdir(...) first (so canonicalise_path has an existing project dir), then call canonicalise_path(raw_audit_path, resolved_project_dir) inside the try/except and wrap ProfileNotFoundError into PruneAuditWriteError as before; only after successful canonicalisation call resolved_audit_path.parent.mkdir(parents=True, exist_ok=True) to create the parent directory (instead of raw_audit_path.parent.mkdir(...)).
🤖 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/prune-engine.md:
- Line 160: Update the inline documentation string "AST-scan suite (5 scans as
of `#6`)" to reflect the current count by replacing "5 scans" with "7 scans" (so
it reads "AST-scan suite (7 scans as of `#6`)"), ensuring the AST-scan count in
the prune rules doc matches the implemented scans referenced elsewhere.
In @.claude/rules/warehouse-adapters.md:
- Around line 160-168: Update the rule text to match the shipped BigQuery
contract: describe that BigQueryAdapter.materialise_sample creates the temp
table with "CREATE TEMP TABLE _sf_sample_<run_id> ..." (the CTAS uses no
_SESSION prefix) and that subsequent references use
"_SESSION._sf_sample_<run_id>" (session-qualified reads), change the
_compute_run_id() description to state it seeds from table.qualified_name with
an 8-byte BLAKE2 digest (16-hex output), and clarify ttl_seconds on
materialise_sample is an operator-side hint (not passed to BigQuery) and should
appear only in the cleanup/auto-expire warning text. Reference BigQueryAdapter,
materialise_sample, _compute_run_id, run_id, and ttl_seconds when making these
edits.
In `@docs/prune-ops.md`:
- Around line 301-314: The doc must match the implementation: change the
`_SESSION` example to use the adapter-local reference FROM
`_SESSION._sf_sample_<run_id>` (no project prefix) and update the run_id recipe
to state it is the 16-hex hex digest of blake2b(..., digest_size=8) over
table.qualified_name (i.e. run_id = hex(blake2b(table.qualified_name,
digest_size=8))), not the previously stated blake2b-12 or different hash input;
also mention that this `compiled_sql` distinction is the durable signal used to
tell materialised runs from oneshot runs via the `compiled_sql` field on
`PruneEvent`.
In `@docs/warehouse-adapter-ops.md`:
- Around line 285-287: The docs claim materialise_sample issues a CTAS named
`_SESSION._sf_sample_<run_id>` but the adapter actually emits `CREATE TEMP TABLE
_sf_sample_<run_id> ...` and only uses `_SESSION._sf_sample_<run_id>` for later
references; update the text in docs/warehouse-adapter-ops.md to reflect the
exact CTAS emitted by the implementation/tests (use `_sf_sample_<run_id>` for
the CTAS statement and note that subsequent references use
`_SESSION._sf_sample_<run_id>`), ensuring the term appears alongside the
`materialise_sample` mention so operators see the real SQL to debug.
In `@plans/super/22-temp-table-sample.md`:
- Around line 176-183: The fenced block containing the BigQuery session cleanup
warning (the triple-backtick block starting with "BigQuery session cleanup
failed; session will auto-expire...") should include a language tag to satisfy
MD040; update that fence from ``` to ```text so the block becomes a text-labeled
fenced code block and the markdown lint will pass.
- Around line 144-145: DEC-001 currently claims `blake2b-12(...)` but also
"16-hex output", which is inconsistent; update the DEC-001 text so the hash
function and width match the intended 16-hex contract—either change `blake2b-12`
to `blake2b-8` (for 8 bytes → 16 hex chars) or change the "16-hex output"
wording to match the existing `blake2b-12` size; ensure references to `run_id`,
`_sf_sample_<run_id>`, and `compiled_sql_hash` remain consistent after the
change.
In `@src/signalforge/warehouse/adapters/bigquery.py`:
- Around line 689-699: The MaterialisationFailedError raised in
materialise_sample() embeds the raw Google exception text; modify the except
block to first map the raw exception using map_bq_exception(exc) (same mapper
used by sample_rows() and run_test_sql()), then raise MaterialisationFailedError
with the mapped error's stable message (and keep the original exception as
cause), e.g., call map_bq_exception to produce a mapped error/message and use
that mapped text in the MaterialisationFailedError message while preserving exc
as the cause.
In `@tests/warehouse/test_sample_cost_probe.py`:
- Around line 653-674: Capture the active session id while still inside the
session context and reuse it in the follow-up query so the test verifies the
session was aborted; specifically, before exiting the context grab the session
identifier from the client/session (so you can target the same session) and then
call adapter._get_client() and client.query( f"SELECT COUNT(*) FROM
`{temp_ref.qualified_name}`",
job_config=adapter._default_job_config(stage="warehouse_test"),
connection_properties={...with the captured session_id...}) after exit and
assert it raises gae.GoogleAPIError — this ensures the query was routed to the
same session id and thus fails if the session was properly aborted.
---
Outside diff comments:
In `@src/signalforge/prune/engine.py`:
- Around line 739-762: Ensure we canonicalise the audit path before creating any
parent directories: keep creating resolved_project_dir via
resolved_project_dir.mkdir(...) first (so canonicalise_path has an existing
project dir), then call canonicalise_path(raw_audit_path, resolved_project_dir)
inside the try/except and wrap ProfileNotFoundError into PruneAuditWriteError as
before; only after successful canonicalisation call
resolved_audit_path.parent.mkdir(parents=True, exist_ok=True) to create the
parent directory (instead of raw_audit_path.parent.mkdir(...)).
In `@src/signalforge/warehouse/adapters/bigquery.py`:
- Around line 213-236: The __exit__ path currently lets exceptions from
_flush_column_stats_batch re-raise before _cleanup_active_session runs, leaving
an open BigQuery session; fix by invoking self._cleanup_active_session() inside
the same finally block (before or after clearing caches) so it always runs even
when _flush_column_stats_batch raises, and wrap the cleanup call in a small
try/except that logs cleanup failures but does not swallow an existing exception
(re-raise if exc_type is not None) — update the code around __exit__ so
_cleanup_active_session is called from the finally and follows the best-effort
logging semantics.
In `@tests/warehouse/_fake.py`:
- Around line 321-367: The fake materialise-sample handler is creating a session
unconditionally; update _consume_materialise_sample to assert that the matched
expectation requested a session (check the expectation's create_session flag on
the matched exp) before synthesising _FakeQueryJobWithSession — if
exp.create_session is False or missing, either return a non-session
_FakeQueryJob(rows=[]) or raise an AssertionError signalling that the production
code didn't request a session; change the session creation branch that currently
calls _derive_fake_session_id(...) and returns _FakeQueryJobWithSession to only
run when exp.create_session is truthy.
In `@tests/warehouse/test_sample_cost_probe.py`:
- Around line 225-229: The SQL built in the sql variable omits the deterministic
ordering used in production; update the construction of sql (the string built
using target.qualified_name, FARM_FINGERPRINT(TO_JSON_STRING(t)) and
bucket/sample_size) to include the same ORDER BY
FARM_FINGERPRINT(TO_JSON_STRING(t)) clause immediately before the LIMIT so the
probe's bytes-billed baseline matches the real oneshot query shape.
🪄 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: 4147b549-63cf-46c9-bc5b-fc6e3dcdb736
📒 Files selected for processing (31)
.claude/rules/prune-engine.md.claude/rules/safety-layer.md.claude/rules/testing-signal.md.claude/rules/warehouse-adapters.mdCLAUDE.mddocs/cli-ops.mddocs/prune-ops.mddocs/warehouse-adapter-ops.mdplans/super/22-temp-table-sample.mdsrc/signalforge/cli/_helpers.pysrc/signalforge/cli/generate.pysrc/signalforge/prune/config.pysrc/signalforge/prune/engine.pysrc/signalforge/warehouse/__init__.pysrc/signalforge/warehouse/adapters/_client.pysrc/signalforge/warehouse/adapters/bigquery.pysrc/signalforge/warehouse/base.pysrc/signalforge/warehouse/errors.pytests/cli/test_exit_codes.pytests/cli/test_generate.pytests/fixtures/prune/prune_event_v1.jsonltests/fixtures/warehouse/sample_materialise_v1.sqltests/llm/test_logger_grep_gate.pytests/prune/test_config.pytests/prune/test_engine.pytests/warehouse/_fake.pytests/warehouse/test_base.pytests/warehouse/test_errors.pytests/warehouse/test_fake.pytests/warehouse/test_materialise_sample.pytests/warehouse/test_sample_cost_probe.py
…rough map_bq_exception Code fixes: * materialise_sample wraps SDK exceptions through map_bq_exception before raising MaterialisationFailedError so cause carries the stable warehouse-error surface (e.g. WarehouseAuthError) instead of raw SDK text. The original SDK exception is preserved on __cause__ via the raise-from chain (CodeRabbit). * prune_tests docstring corrected: the engine OWNS the with-adapter block; library callers MUST pass a non-entered adapter and MUST NOT wrap the call themselves (Copilot). * materialise_sample Returns: docstring corrected — the returned TableRef is project=None / dataset=_SESSION (two-part qualified_name); the three-part <project>._SESSION.<name> form is the rejected one (Copilot). * tests/warehouse/_fake.py _qualified_name_substring docstring clarified — the existing period-prefixed substring IS correct (it matches the suffix of the production three-part backtick-quoted form); Copilot's two-part suggestion would actually break the match. Test fixes: * test_materialised_session_cleaned_up_after_exit: the cleanup verification query now reuses the captured session_id via connection_properties so the test would fail if cleanup leaked (CodeRabbit). Without this, the post-exit query failed for an unrelated reason (no session on the wire) and the test passed even on a broken cleanup. * test_materialise_sample_wraps_warehouse_sdk_errors: assertion follows the new map_bq_exception contract (cause is the typed warehouse error; __cause__ preserves the raw SDK exception). * test_sample_cost_probe.py:495 comment corrected to two-part _SESSION._sf_sample_<run_id> form (Copilot). * tests/warehouse/test_materialise_sample.py module docstring refreshed — US-004 has shipped expect_materialise_sample / expect_abort_session helpers; the module continues to use raw expect_query intentionally for diagnostic SQL-byte assertions (Copilot). Doc/rule drift cleanup (CTAS shape, _SESSION reference, run_id recipe): * run_id is blake2b(table.qualified_name + version + str(n) + canonical_partition_filter, digest_size=8) — 16 hex chars; corrects the stale "blake2b-12 / model.unique_id" recipe. * CTAS uses bare _sf_sample_<run_id> name; subsequent reads use two-part _SESSION._sf_sample_<run_id>. Corrects the stale _SESSION._sf_sample_<run_id> CTAS form. * warehouse_session_abort added to docs/warehouse-adapter-ops.md stage-label list. * AST-scan + logger-grep-gate counts updated (7 scans, 6 dirs as of #9) in .claude/rules/prune-engine.md (CodeRabbit). * DEC-014 cleanup-warning fenced block in plan now carries text language tag (CodeRabbit MD040 lint). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR Review SummaryAddressed all 18 review threads in commit 8a620d9. Fixed (17 items)
False Positives (1 item)
Validation
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
Resolves conflict in plans/super/22-temp-table-sample.md by keeping the feature branch's post-review corrections — dev brought in PR #30 which is the original (pre-review) plan content; the feature branch already corrected the same five locations in response to Copilot + CodeRabbit feedback (run_id recipe, CTAS shape, two-part _SESSION form, MD040 fenced-block lang tag). No code conflicts — only the plan file diverged. Validation green post-merge: ruff / format / pyright / pytest (1459 passed).
Summary
Implementation of issue #22 — adopt the temp-table-materialised sample strategy for v0.2 sample-mode prune. Closes the cost cliff that made sample-mode unusable in v0.1 (AR-B1 measured 9.92 GB / single sample query against a 30M-row table; per-test cost now drops to the 1–10 MB range).
Companion plan PR: #30 (the design doc on
feature/22-temp-table-sample).What changed
11 stories landed, mapped 1-to-1 with the plan's US-001 through US-011 (
SignalForge-6tv.1through.11in the beads epic):PruneConfig.sample_strategy: Literal["oneshot", "materialised"] = "materialised"field; new typed errorsMaterialisationFailedErrorandMaterialisationNotSupportedError(bothWarehouseErrorsubclasses, both → CLI tier 3).WarehouseAdapter.materialise_sample(table, n, *, partition_filter=None, ttl_seconds=3600) -> TableRefABC method; default impl raisesMaterialisationNotSupportedError.BigQueryAdapter.materialise_sampleusing BQ sessions:CREATE TEMP TABLE _SESSION._sf_sample_<run_id>once per prune run, capturesjob.session_info.session_id, threadsconnection_propertiesthrough follow-up queries,__exit__runsCALL BQ.ABORT_SESSION()for cleanup. Seededrun_id = blake2b(table.qualified_name + signalforge_version + sample_size + canonical_json(partition_filter), digest_size=8)preserves snapshot determinism.FakeBigQueryClient.expect_materialise_sample/expect_abort_sessiontest helpers.config.sample_strategy, wraps adapter inwith adapter:, conservative-bias routing onWarehouseError(every candidate →kept-without-evidence, single stderr WARNING at head of failure path), per-decision JSONL audit preserved.signalforge generate --scope {sample,full}and--sample-strategy {oneshot,materialised}flags, override viaPruneConfig.model_validate(...)(NOTmodel_copy(update=...)) so validators re-run.@pytest.mark.bigquerytests: oneshot baseline (regression guard), materialised target (acceptance gate < 100 MB), session-cleanup verification (positive proof the temp table is gone post-__exit__).docs/prune-ops.md(cost model + audit reading guide + sample_strategy field),docs/warehouse-adapter-ops.md(cleanup recovery section + INFORMATION_SCHEMA orphan-session query),docs/cli-ops.md(3 stderr WARNING shapes),.claude/rules/prune-engine.md+.claude/rules/warehouse-adapters.md(v0.2 reservations),CLAUDE.md(public API surface).warehouse/, SDK orphan-session edge-case note).WarehouseError, 5-surface parity, cleanup-boundary fail-soft contrast with primary-work fail-closed).Stats
dev; +62 new tests across the 11 stories).dev).warehouse/).Architectural commitments preserved
DropReasontaxonomy locked; conservative-bias routes failures tokept-without-evidence(never silently dropped). Per-decision audit preserved (N candidates → N JSONL lines on materialisation failure)._client.py. Snowflake/Postgres v0.3 adapters slot underadapters/without restructuring.compiled_sql_hashreproducibility invariant preserved via seededrun_id; same input → same prune decision.Test plan
ruff check . && ruff format --check . && pyright && pytest(1445 passed, 2 skipped, 17 deselected)SF_RUN_BQ=1):SF_RUN_BQ=1 pytest -m bigquery tests/warehouse/test_sample_cost_probe.py --no-cov<TBD: post-Q4=C ... (filled during US-010 quality gate)>placeholders indocs/prune-ops.mdCost model section with the maintainer's measured figures.Issue acceptance checklist (from #22)
bytes_billedfor the AR-B1 probe target drops below 100 MB without raisingcost_limit_bytes(needs maintainer probe-run).docs/prune-ops.mdCost model section records the post-Q4=C figure + run date (awaits cost-figure substitution above).prune.sample_strategy: oneshotopt-in;materialisedis the default).DropReasontaxonomy or audit JSONL schema (strategy switch is below those layers).Closes #22 when merged.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation