22: Q4=C temp-table-materialised sample (plan) - #30
Merged
Conversation
Super-plan for adopting the temp-table-materialised sample strategy for v0.2 sample-mode prune. 11 decisions captured, 10 stories ready for devolve, awaiting review. 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:
Comment |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
Reverses DEC-011's prior "config-only" position based on user feedback (2026-05-05): operators want to flip between thorough (full-scan) and cheap (materialised sample) modes per-run without editing signalforge.yml. Adds DEC-012 documenting the override-via-model_validate pattern (mirrors safety-layer.md DEC-018 / cli-layer.md graduated DiffConfig .render_kind in #9). Inserts new US-006 story for the CLI flag work; renumbers US-006 through US-010 forward by one to US-007 through US-011. Plan now ships 9 implementation stories + Quality Gate + Patterns & Memory (11 total). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User feedback (2026-05-05): cleanup-failure path should still message the user with the raw session_id, the manual kill command, and the TTL fallback note. Don't silently swallow. Changes: - DEC-002 revised: __exit__ now belt-and-braces closes the BQ session (graduates v0.3 deferral forward; cheap to add now). - DEC-003 revised: narrow exception to session_id redaction — the raw id appears in the cleanup-failure WARNING (single user-facing surface). - DEC-013 (new): explicit cleanup mechanism via CALL BQ.ABORT_SESSION(); best-effort with three-layer defence (explicit close + TTL fallback + swallow-on-failure). - DEC-014 (new): cleanup-failure WARNING contract — multi-line shape with raw session_id, manual `bq query` command, and "auto-expire in Ns" reassurance. - US-003: 12 new tests covering __exit__ cleanup, success-path INFO, failure-path WARNING shape, session_id redaction on happy path, state reset in finally. Total 22 tests now (was 12). - US-004: adds expect_abort_session helper to FakeBigQueryClient (4 new tests). Total 7 tests now (was 3). - US-008 probe: third test verifies temp table is gone post-__exit__ (positive proof of DEC-013). - US-009 docs: warehouse-adapter-ops.md gets a "Session cleanup & manual recovery" section with the manual command template + an INFORMATION_SCHEMA query for ops. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User audit (2026-05-05) caught three places where the cleanup amendment didn't fully land: 1. US-005 had no test pinning the orchestrator's use of `with adapter:`. Without that, BigQueryAdapter.__exit__ never fires and DEC-013 cleanup is unreachable from the orchestrator. Added three new tests: test_prune_tests_uses_adapter_as_context_manager, test_prune_tests_adapter_exit_fires_after_normal_completion, test_prune_tests_adapter_exit_fires_after_materialisation_failure. 2. US-009 surface 4 (.claude/rules/warehouse-adapters.md) only mentioned the session-state pattern. Extended to call out a "Best-effort cleanup in __exit__ with user-actionable failure WARNING" sub-section covering DEC-013/DEC-014 verbatim, so v0.3 stateful-adapter work inherits the canonical reference. 3. US-011 Patterns & Memory had no entry for the cleanup pattern. Added entry #5: best-effort cleanup with user-actionable WARNING as a reusable project pattern, contrasted with safety-layer.md DEC-011's fail-closed-on-primary-work pattern (cleanup boundary vs primary boundary). Renumbered the bd remember entry to #6. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User audit asked us to double-check how cleanup works and what we tell users. Six fixes: G1 — Factual error: BQ assigns the session_id server-side; we capture it from job.session_info.session_id after .result(). Plan previously said we mint via uuid4().hex. DEC-002 + US-003 description corrected; note added on run_id vs session_id (distinct identifiers). G2 — Missing degraded-run WARNING from orchestrator. When materialise fails and all tests route to kept-without-evidence, the only signal was N identical `why` fields buried in the diff. DEC-009 now requires a single stderr WARNING (lazy-format JSON) at the head of the conservative-bias routing path. US-005 adds two pinning tests. G3 — TTL semantics overstated. BigQuery sessions have a server-managed max lifetime (~24h); ttl_seconds is OUR-side hint to the WARNING text, not a BQ knob. DEC-013 clarified. G4 — Pinned that --quiet does NOT suppress the cleanup-failure WARNING (operator-actionable; deliberate non-suppressible). DEC-014 extended. G5 — prune_tests docstring requirement: callers MUST use the adapter inside `with adapter:`. Without it, no explicit BQ.ABORT_SESSION() call fires; cleanup falls back to BigQuery's server-side timeout. CLI's cmd_generate already complies; notebook/script callers responsible. US-005 "Done when" extended. G6 — docs/cli-ops.md 5-surface parity: added three WARNING surfaces (cleanup-failure, materialisation-failure, budget-exceeded) to a "Stderr shapes" section so CI parsers and downstream tooling have one stable reference. US-009 surface count: 5 -> 6. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Created bd epic SignalForge-6tv (external-ref gh-22) with 11 child tasks SignalForge-6tv.1 through .11. Wired 26 dependency edges per the plan's "Depends on" graph. US-001 is the sole ready task on devolve; all others blocked until their predecessors close. Plan phase advanced from approved -> devolved. Beads manifest table appended to plan document. Note: bd dolt auto-push warnings throughout creation are non-fatal (dolt remote sync issue, not a local-write failure). bd dolt push will be run separately to reconcile. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a super-plan for Issue #22 describing the v0.2 shift to a temp-table-materialised sampling strategy (Q4=C) to amortise BigQuery sampling costs across candidate tests, including proposed API seams, error handling, testing, and documentation updates.
Changes:
- Introduces a detailed planning document covering architecture decisions (DECs), implementation stories, and acceptance criteria for materialised sampling.
- Specifies BigQuery session-based temp table materialisation mechanics, conservative-bias failure routing, and CLI/config surface changes.
- Defines a test and docs rollout plan (including probe split and ops documentation updates) to validate the cost reduction and operational behavior.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+9
to
+13
| - **Phase:** devolved | ||
| - **PR:** <https://github.com/wjduenow/SignalForge/pull/30> | ||
| - **Approved:** 2026-05-07 by wjduenow | ||
| - **Devolved:** 2026-05-07 | ||
| - **Beads epic:** `SignalForge-6tv` |
| - **Approved:** 2026-05-07 by wjduenow | ||
| - **Devolved:** 2026-05-07 | ||
| - **Beads epic:** `SignalForge-6tv` | ||
| - **Worktree:** `/Users/wesduenow/Projects/worktrees/SignalForge/22-temp-table-sample` |
Comment on lines
+237
to
+239
| ### US-002 — `WarehouseAdapter.materialise_sample` ABC method + default impl | ||
|
|
||
| **Description.** Add abstract method `materialise_sample(table, n, *, partition_filter=None, ttl_seconds=3600) -> TableRef` to `WarehouseAdapter`. The default impl raises `MaterialisationNotSupportedError`; subclasses override (BigQuery in US-003). Method is NOT `@abstractmethod` because the default impl IS the v0.2 behaviour for non-BQ adapters. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
plans/super/22-temp-table-sample.md (3)
158-159: 💤 Low valueUpdate DEC-007 to account for three probe tests.
DEC-007 states "Probe ships as two
@pytest.mark.bigquerytests", but US-008 (line 446) correctly describes three tests: the two probe tests (oneshot baseline + materialised target) plus a third cleanup-verification test per DEC-013.Consider amending DEC-007 to say "Probe ships as two cost-measurement tests plus one cleanup-verification test" for alignment with the final US-008 scope.
🤖 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 `@plans/super/22-temp-table-sample.md` around lines 158 - 159, Update DEC-007 to reflect three probe tests instead of two: change the wording to state that Probe ships as two cost-measurement tests plus one cleanup-verification test so it aligns with US-008 and DEC-013; ensure the description mentions the existing test names (test_sample_rows_cost_baseline_oneshot, test_sample_rows_cost_materialised) and the additional cleanup-verification test to avoid ambiguity.
176-182: 💤 Low valueAdd language specifier to fenced code block.
The DEC-014 WARNING template block (lines 176-182) lacks a language specifier, triggering markdownlint warning MD040. Add
```textor```plaintextto the opening fence for proper syntax highlighting and markdown compliance.🤖 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 `@plans/super/22-temp-table-sample.md` around lines 176 - 182, The fenced code block in the DEC-014 WARNING template block is missing a language specifier and triggers markdownlint MD040; update the opening fence of that block (the triple-backtick that precedes "BigQuery session cleanup failed...") to include a plain text language such as "text" or "plaintext" (i.e., change ``` to ```text) so the block is properly tagged and the MD040 warning is resolved.
263-263: 💤 Low valueOptional: Consider tightening inline code spans to reduce markdown lint warnings.
Line 263 triggers nine markdownlint MD038 warnings for spaces inside code spans (e.g.,
<captured>instead of<captured>). While the spaces improve readability for placeholder values, removing them would align with strict markdown conventions.This is cosmetic and not blocking.
🤖 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 `@plans/super/22-temp-table-sample.md` at line 263, Summary: The markdown has MD038 lint warnings due to spaces inside inline code spans; remove the extra spaces. Fix: edit the doc text around the BigQuery sample materialisation description and remove leading/trailing spaces inside inline code spans (e.g., change backticks wrapping ` <captured> ` to `<captured>`, ` _sf_sample_<run_id> ` to `_sf_sample_<run_id>`, ` job.session_info.session_id ` to `job.session_info.session_id`, ` self._active_session_id ` to `self._active_session_id`, ` _SESSION._sf_sample_<run_id> ` to `_SESSION._sf_sample_<run_id>`, and any JSON/code examples like ` {"model": ..., "sample_rows": ..., "session_id_hash": blake2b-4(...), "run_id": ..., "duration_ms": ...} ` to remove surrounding space) so all inline code spans have no interior leading/trailing spaces and the MD038 warnings are resolved.
🤖 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 `@plans/super/22-temp-table-sample.md`:
- Line 144: DEC-001's blake2b digest size is misstated: update the spec so the
hash length and description match the project's convention used elsewhere;
either change `blake2b-12(...)` to `blake2b-8(...)` with "(16-hex output)"
(recommended) or keep `blake2b-12(...)` and correct the note to "(24-hex
output)"; ensure the `run_id` formula and the temp-table name template
`_sf_sample_<run_id>` reflect the chosen digest size so downstream references
(US-003, snapshot fixtures, and `compiled_sql_hash` expectations) stay
consistent.
---
Nitpick comments:
In `@plans/super/22-temp-table-sample.md`:
- Around line 158-159: Update DEC-007 to reflect three probe tests instead of
two: change the wording to state that Probe ships as two cost-measurement tests
plus one cleanup-verification test so it aligns with US-008 and DEC-013; ensure
the description mentions the existing test names
(test_sample_rows_cost_baseline_oneshot, test_sample_rows_cost_materialised) and
the additional cleanup-verification test to avoid ambiguity.
- Around line 176-182: The fenced code block in the DEC-014 WARNING template
block is missing a language specifier and triggers markdownlint MD040; update
the opening fence of that block (the triple-backtick that precedes "BigQuery
session cleanup failed...") to include a plain text language such as "text" or
"plaintext" (i.e., change ``` to ```text) so the block is properly tagged and
the MD040 warning is resolved.
- Line 263: Summary: The markdown has MD038 lint warnings due to spaces inside
inline code spans; remove the extra spaces. Fix: edit the doc text around the
BigQuery sample materialisation description and remove leading/trailing spaces
inside inline code spans (e.g., change backticks wrapping ` <captured> ` to
`<captured>`, ` _sf_sample_<run_id> ` to `_sf_sample_<run_id>`, `
job.session_info.session_id ` to `job.session_info.session_id`, `
self._active_session_id ` to `self._active_session_id`, `
_SESSION._sf_sample_<run_id> ` to `_SESSION._sf_sample_<run_id>`, and any
JSON/code examples like ` {"model": ..., "sample_rows": ..., "session_id_hash":
blake2b-4(...), "run_id": ..., "duration_ms": ...} ` to remove surrounding
space) so all inline code spans have no interior leading/trailing spaces and the
MD038 warnings are resolved.
🪄 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: 156d7575-4661-4f57-908f-fc75285aab19
📒 Files selected for processing (1)
plans/super/22-temp-table-sample.md
|
|
||
| ### Decisions | ||
|
|
||
| - **DEC-001 — Compiled-SQL determinism via seeded run_id (R1=B).** `run_id = blake2b-12(model.unique_id + signalforge_version + sample_size + canonical_json(partition_filter))` (16-hex output). Temp-table name = `_sf_sample_<run_id>`. Same input → byte-equal compiled SQL across runs; `compiled_sql_hash` invariant preserved unchanged. Rationale: simpler than snapshot normalisation; sessions provide namespace isolation so two concurrent runs on the same model don't collide on the temp-table identifier. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for existing blake2b usage patterns to determine the project's digest-size convention
rg -nP 'blake2b' --type=py --type=md -C2Repository: wjduenow/SignalForge
Length of output: 50378
🏁 Script executed:
#!/bin/bash
# Read the target file to verify line 144 and context
sed -n '140,160p' plans/super/22-temp-table-sample.mdRepository: wjduenow/SignalForge
Length of output: 4270
Fix blake2b digest size specification in DEC-001.
Line 144 specifies blake2b-12(...) with "(16-hex output)", but the project's established convention is blake2b-N → 2N hex characters (confirmed by DEC-003's blake2b-4 → 8 hex chars). Thus blake2b-12 produces 24 hex chars, not 16.
Correct to either:
blake2b-8(...)with "(16-hex output)" — recommended, matches the codebase pattern for deterministic hashes, orblake2b-12(...)with "(24-hex output)"
This specification drives the temp-table naming in US-003 and snapshot fixtures; the mismatch will cause implementation misalignment.
🤖 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 `@plans/super/22-temp-table-sample.md` at line 144, DEC-001's blake2b digest
size is misstated: update the spec so the hash length and description match the
project's convention used elsewhere; either change `blake2b-12(...)` to
`blake2b-8(...)` with "(16-hex output)" (recommended) or keep `blake2b-12(...)`
and correct the note to "(24-hex output)"; ensure the `run_id` formula and the
temp-table name template `_sf_sample_<run_id>` reflect the chosen digest size so
downstream references (US-003, snapshot fixtures, and `compiled_sql_hash`
expectations) stay consistent.
wjduenow
added a commit
that referenced
this pull request
May 9, 2026
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).
wjduenow
added a commit
that referenced
this pull request
May 9, 2026
* 22: import plan from feature/22-temp-table-sample for impl branch 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> * SignalForge-6tv.1: PruneConfig.sample_strategy + MaterialisationFailedError + MaterialisationNotSupportedError * SignalForge-6tv.2: WarehouseAdapter.materialise_sample ABC method (US-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). * SignalForge-6tv.7: register Materialisation* errors in CLI exit-code 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> * SignalForge-6tv.3: BigQueryAdapter.materialise_sample (US-003 of #22) 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> * SignalForge-6tv.4: FakeBigQueryClient.expect_materialise_sample + expect_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> * SignalForge-6tv.5: prune orchestrator dispatch on sample_strategy + conservative routing (US-005 of #22) * SignalForge-6tv.6: --scope and --sample-strategy flags on signalforge 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> * SignalForge-6tv.8: probe re-run scaffolding + cleanup verification (US-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> * SignalForge-6tv.9: documentation surfaces (6-surface parity) 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> * SignalForge-6tv.10: Quality gate — fix bugs from code review 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> * SignalForge-6tv.11: distil patterns into rule files + bd remember (US-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> * 22: fix _SESSION qualified-name + record post-Q4=C cost figures 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> * 22: close Codecov patch-coverage gap on materialise_sample + prune routing 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> * 22: address PR review — fix drift, route materialise_sample errors through 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> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
This was referenced May 11, 2026
This was referenced Jun 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Super-plan for issue #22 — adopt the temp-table-materialised sample strategy for v0.2 sample-mode prune.
Phase: detailing (awaiting review).
Stories: 8 implementation stories + Quality Gate + Patterns & Memory.
Decisions: 11 (DEC-001 through DEC-011) covering compiled-SQL determinism, BigQuery session lifecycle,
session_idredaction, conservative-bias failure routing, and total-budget semantics.What this plan ships (when devolved)
WarehouseAdapter.materialise_sample(...)ABC method; default impl raisesMaterialisationNotSupportedError.CREATE TEMP TABLE _sf_sample_<seeded_run_id>perprune_testscall; per-test queries reuse the session viaconnection_propertiesso they can read_SESSION._sf_sample_<run_id>.PruneConfig.sample_strategy: Literal["oneshot", "materialised"]field, defaultmaterialised.WarehouseErrorsubclasses (MaterialisationFailedError,MaterialisationNotSupportedError), both → CLI exit-code tier 3.WarehouseErrorduring materialisation → every candidate test routes tokept-without-evidencewith a class+messagewhyfield; onePruneEventper candidate written to JSONL (fail-closed audit preserved).test_sample_rows_cost_baseline_oneshot(regression guard for the 9.92 GB AR-B1 measurement) andtest_sample_rows_cost_materialised(asserts < 100 MB per test).prune-ops.md,warehouse-adapter-ops.md, two rule files,CLAUDE.mdv0.2 surface).Plan document
See
plans/super/22-temp-table-sample.mdfor:Architectural commitments preserved
DropReasontaxonomy locked; no schema change toPruneEvent.adapters/without restructuring.run_idpreserves "same input → same prune decision" + thecompiled_sql_hashreproducibility invariant.Next steps
Closes #22 (after implementation lands; this PR is the plan only).
🤖 Generated with Claude Code
Summary by CodeRabbit