#184: drafter row_count_anomaly_by_period scope fix - #194
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR rewrites prompt guidance and adds parser recovery that detects model-only tests emitted under column scope, emits a structured warning, re-attaches them to model scope (with argument revalidation), records ReshapeRecord(s) in a v2 audit event, and updates tests/docs end-to-end. ChangesModel-only variant scope correction
Sequence Diagram (high-level) sequenceDiagram
participant Client as draft_from_request
participant Parser as parse_draft_response
participant Anchor as _validate_anchor_contract
participant Args as _validate_model_only_test_args
participant Logger as _LOGGER
participant Rebuilder as _apply_reattach_actions
participant Audit as _build_response_event
Client->>Parser: parse candidate (pass reshapes_collected, model_unique_id)
Parser->>Anchor: validate columns/tests (collect reattach actions)
Anchor->>Logger: warning("parser re-attach: %s", payload_json)
Anchor->>Args: re-validate test args at model scope
Anchor-->>Parser: violations + reattach actions + reshape records
Parser->>Rebuilder: apply reattach actions (immutable rebuild)
Rebuilder-->>Parser: rebuilt CandidateSchema
Parser->>Audit: build event with parser_reshaped
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
…del-level scope
…mp audit_schema_version 1->2
… WARNING + ReshapeRecord audit
…+ ReshapeRecord audit
…est to LLMResponseEvent audit
…h draft_from_request
Pass 1 (Correctness) fixes: - New _validate_model_only_test_args helper called from the re-attach branch — re-attached tests' date_column / where / unique_combination.columns args are now re-validated against model_columns, preserving the 'fail loud on hallucinations' contract. Without it a re-attached row_count_anomaly_by_period(date_column='phantom') silently degraded to kept-without-evidence at prune instead of surfacing a typed parser violation. (Pass 1 Finding 1, promoted to must-fix per qg-pass-3-defer-defensive-tests-fails-codecov.) - 4 new tests in test_parser.py pinning the new validation path: test_reattach_validates_anomaly_date_column_against_model_columns, test_reattach_validates_row_count_between_where_against_model_columns, test_reattach_validates_unique_combination_columns_against_model_columns, test_reattach_preserves_sibling_tests_on_same_column (Pass 1 Finding 2 — sibling-test preservation in _apply_reattach_actions was previously unpinned). Pass 4 (Docs+UX) fixes: - BLOCKER: docs/draft-ops.md audit-fields table updated audit_schema_version from 'Currently 1' to 'Currently 2' (was stale post-US-002 bump). - MAJOR: Added a row to the audit-fields table for the new parser_reshaped field, with the back-compat default explanation. - MINOR: Added operator-workflow cleanup paragraph to docs/draft-ops.md parser-re-attach section + CHANGELOG Fixed entry instructing operators to remove the pre-fix llm.exclude_tests workaround. - TRIVIAL: Added sort_keys=True to the parser re-attach WARNING json.dumps so the doc sample and the actual emitted bytes share key ordering (alphabetical is also diff-friendly across runs). Pass 2 (Conventions) and Pass 3 (Tests) returned zero findings. Validation: 3368 passed, 6 skipped, 99 deselected; ruff / format / pyright all 0 errors.
…-fix # Conflicts: # CHANGELOG.md
|
@CodeRabbit review |
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #184 where the LLM drafter incorrectly emits the model-only row_count_anomaly_by_period test at column scope (often on audit timestamp columns), causing the parser’s anchor-contract validation to fail. The change tightens prompt guidance to explicitly teach model-level placement, and adds a parser-side “belt-and-braces” re-attach mechanism with warning + durable audit trail so runs don’t fail when the LLM still mis-scopes.
Changes:
- Update
_ROW_COUNT_ANOMALY_SCOPE_INSTRUCTIONto explicitly state model-level placement and include a worked YAML example; rotate prompt version and update cache-stability pin. - Add parser support to re-attach column-scoped emissions of model-only variants (
row_count_anomaly_by_period,row_count_between,unique_combination) to model scope, emitting a warning and recording reshapes. - Bump
LLMResponseEvent.audit_schema_versionfrom 1 → 2 and addparser_reshapedaudit field, with fixtures + strict drift-detector coverage and end-to-end threading fromdraft_from_request.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/signalforge/draft/prompts.py |
Clarifies anomaly test model-level scope and adds worked example; preserves calibration guidance. |
tests/draft/test_prompts.py |
Pins the new scope wording and worked example so prompt regressions fail loudly. |
tests/llm/test_prompt_cache_stability.py |
Updates _EXPECTED_PROMPT_VERSION to match the rotated prompt hash and documents the rotation. |
src/signalforge/draft/parser.py |
Implements column→model re-attach for model-only variants, warning emission, and reshape collection + rebuild. |
tests/draft/test_parser.py |
Adds comprehensive re-attach behavior tests (all 3 variants, warning shape, exclude gate, collect-all, arg validation, sibling preservation). |
src/signalforge/draft/audit.py |
Introduces ReshapeRecord, adds parser_reshaped, and bumps audit_schema_version default to 2. |
src/signalforge/draft/schema.py |
Threads reshapes_collected into parsing and forwards it into the response audit event. |
tests/draft/test_schema.py |
Verifies reshape threading into the audit JSONL and v2 defaults on the no-reshape path. |
tests/draft/test_audit.py |
Updates audit schema default expectations and asserts the new field default. |
tests/draft/test_drift_detector.py |
Adds strict mirrors for ReshapeRecord + v2 fixture validation while keeping v1 fixture valid. |
tests/fixtures/draft/llm_response_with_reshape_v2.jsonl |
New v2 audit fixture exercising the reshape path. |
docs/draft-ops.md |
Documents the new audit field and the parser re-attach behavior (including exclude_tests precedence). |
docs/research/179-test-primitive-expansion-retest.md |
Adds follow-up writeup section describing the #184 resolution and partial validation results. |
CHANGELOG.md |
Adds Fixed + Changed entries for the scope fix and audit schema bump. |
.claude/rules/llm-drafter.md |
Documents the parser re-attach carve-out and invariants (exclude gate, collect-all, warning shape, no dedupe). |
.claude/rules/business-rule-tests.md |
Captures #184 as a durable pattern for dual-defence prompt+parser fixes with audit visibility. |
plans/super/184-anomaly-scope-fix.md |
Adds the detailed plan/DEC log and acceptance criteria for the change set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot flagged that the writeup overclaimed DEC-008's acceptance bar. The plan's DEC-008 explicitly committed to all 15 Phase B candidates; the partial 3-of-15 run was an opt-in budget concession, not the bar DEC-008 set. Adjusted to honestly mark the remaining 12 candidates as PENDING and to scope the 3-candidate evidence to 'the bug is fixed on the load-bearing failure shape' (necessary but not sufficient for the full DEC-008 close). No code changes — research-doc clarification only.
PR Review SummaryFixed (1 item)
The writeup now honestly marks Status: PARTIAL — 3 of 15 candidates run, names the remaining 12 candidates as PENDING, and scopes the 3-candidate evidence to "the bug is fixed on the load-bearing failure shape" (necessary but not sufficient for the full DEC-008 close). A future maintainer-side pass with a fresh API window (or chunked across sessions to avoid the Anthropic rate-limit pressure that capped this run) will close the gap. False Positives (0 items)No false positives in this review pass. |
Summary
Super plan for #184 — drafter mis-scopes
row_count_anomaly_by_periodto date columns on models with audit timestamps (reproduced 3/3 on Phase B of the #179 retest).Phase: detailing (awaiting approval)
Stories: 8 implementation + Quality Gate + Patterns & Memory = 10 total
Decisions: 11 captured (DEC-001 … DEC-011)
What the plan ships
_ROW_COUNT_ANOMALY_SCOPE_INSTRUCTIONto explicitly teach model-level scope (closes the LLM-steering gap).row_count_anomaly_by_period/row_count_between/unique_combination), with an always-emit_LOGGER.warningAND a durable audit record (parser_reshaped: tuple[ReshapeRecord, ...]onLLMResponseEvent).LLMResponseEvent.audit_schema_version: 1 → 2— new fixture row + drift-detector strict mirror added; v1 fixture stays valid (extra="ignore").exclude_testskill-switch beats re-attach — operator opt-out is always honoured.intuit_airflowsubstrate; results land as a "Followup #171 follow-on: Drafter mis-scopesrow_count_anomaly_by_periodto date columns on models with audit timestamps #184 resolution" section indocs/research/179-test-primitive-expansion-retest.md.Eight-surface lockstep change (extends #183's six-surface template)
prompts.py+parser.py+audit.py+test_prompts.py+test_prompt_cache_stability.py+test_parser.py+ drift detector / fixture + four doc surfaces (draft-ops.md, two.claude/rules/files,CHANGELOG.md).Plan document
See
plans/super/184-anomaly-scope-fix.mdfor the full plan — decisions, story breakdown, dependency ordering, and Quality Gate focus areas.Next steps
Related
_ROW_COUNT_BETWEEN_SCOPE_INSTRUCTION(DEC-012 worked example never landed) #183 / PR #183: Add _ROW_COUNT_BETWEEN_SCOPE_INSTRUCTION #191 (parallelrow_count_betweenscope-instruction pattern; merged 2026-06-02)Summary by CodeRabbit
Bug Fixes
Documentation
Audit