#163: drafter business-rules fidelity (plan) - #164
Merged
Conversation
Add super-plan for #163 covering: - DEC-001: Lever B + C1 (dynamic-block envelopes + parser cardinality gate) - DEC-002: at-least-one-per-rule cardinality - DEC-004: thread business_rules through parse_draft_response (mirror #159) - DEC-005: reuse PromptEnvelopeBreachError with parameterised envelope - DEC-008: exclude_tests=('custom_sql',) short-circuits both surfaces - DEC-009: <BUSINESS_RULE id="N">...</BUSINESS_RULE> envelope format Four stories: dynamic-block hardening, parser gate, Quality Gate, Patterns & Memory. ~100 LOC code + ~30 LOC prose. No _PROMPT_VERSION rotation; pinned by US-001 AC #7 as the load-bearing cache-stability gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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 (3)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughThis PR renders declared business rules as numbered <BUSINESS_RULE id="N"> prompt envelopes with a pre-render closing-tag breach guard, generalizes PromptEnvelopeBreachError for multiple envelopes, threads business_rules through the draft pipeline, and adds a parser cardinality gate requiring at-least-one custom_sql per declared rule. ChangesBusiness-rule envelope hardening and cardinality gating
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Phase 7 — populate Beads manifest with epic + 4 task IDs and update phase marker to 'devolved'. Epic: bd_1-scaffolding-74b .1 US-001 — Dynamic-block envelope hardening + parameterised breach guard .2 US-002 — Parser cardinality gate + business_rules threading (blocked on .1) .3 US-003 — Quality Gate (blocked on .1, .2) .4 US-004 — Patterns & Memory (blocked on .3) Ready next: bd_1-scaffolding-74b.1. Run /ralph-run from this worktree to execute the chain. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sed breach guard (US-001 of #163) Replace the bulleted `## BUSINESS RULES` list in `_render_business_rules_section` with numbered `<BUSINESS_RULE id="N">…</BUSINESS_RULE>` envelopes (DEC-009). Each rule body indented exactly 2 spaces, preserving the existing `(model) `/`(column X) ` scope prefix verbatim. Extend `PromptEnvelopeBreachError` to be envelope-parameterised (DEC-005): new keyword-only `envelope: str = "MODEL_SQL"` and `rule_index: int | None` kwargs. The default rendering is byte-equal to the pre-#163 message so the existing call site in `prompts.py` keeps working unchanged. When `envelope="BUSINESS_RULE"` with `rule_index` set, the message names the 1-indexed offending rule. Add a pre-render breach guard: `_render_business_rules_section` scans each rule for the literal `</BUSINESS_RULE>` substring (boring substring match per the `</MODEL_SQL>` precedent — no whitespace/case normalisation) and raises `PromptEnvelopeBreachError` with the 1-indexed rule index. Short-circuit the section to `""` when `"custom_sql"` is in `DraftConfig.exclude_tests` (DEC-008) — don't tell the LLM to draft rules it can't emit. Thread `exclude_tests` through `_render_dynamic_block` and `render_prompt` so the caller's already-known exclusion set reaches the business-rules renderer without re-reading config. `_PROMPT_VERSION` is unchanged (`c9e7ee1f6f465933`) — all changes scope to the dynamic block and the errors module; the cached system prompt templates are untouched. `tests/llm/test_prompt_cache_stability.py` stays green. Traces to: DEC-001, DEC-005, DEC-008, DEC-009 of plans/super/163-drafter-business-rules-fidelity.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ening + parameterised breach guard
…ading (US-002 of #163) Adds keyword-only `business_rules: tuple[str, ...] = ()` to `parse_draft_response` and `_validate_anchor_contract`. Threads it from `draft_from_request` (in `signalforge.draft.schema`) using the existing `_read_business_rules(model)` helper from `prompts.py` as the single source of truth — mirrors the `model_columns_by_type` threading style from issue #159. Gate (DEC-002, DEC-008): when `business_rules` is non-empty AND `"custom_sql"` is NOT in `exclude_tests`, count `custom_sql` tests across both `candidate.tests` (model-level) and every `column.tests` (column-level), and append one collect-all violation when the count falls below `len(business_rules)`. At-least-one-per-rule semantics: excess is allowed (legitimate multi-test decomposition of a complex rule). Violation message (DEC-006) names every declared rule verbatim via `repr()` so the operator sees exactly which rules they declared: "Expected ≥N custom_sql test(s) (one per declared business rule), got M. Declared rules: '(model) rule one', '(column X) rule two'." 11 new TDD tests in `tests/draft/test_parser.py` cover under-coverage rejection, coverage match, over-coverage allowed, both no-op paths (empty rules with/without custom_sql), exclude_tests short-circuit, model-level / column-level / mixed counting, the pinned violation message shape, and collect-all parity with other violations. The gate is a no-op when `business_rules=()` so all 36 existing parser-test call sites continue to work unchanged. No new error class, no new exit-code-table entry, no prompt rendering touched, no `_PROMPT_VERSION` rotation — cache-stability test still passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…usiness_rules threading
…deRabbit US-003 (#163) inline Quality Gate. 4 code-review passes ran in parallel on the US-001+US-002 diff; this commit lands the real signal from those passes as 5 invariant tests (no production-code changes — the diff itself was found contract-compliant and correctness-clean). Findings landed: 1. Pass 3 (coverage) finding 1 — pin the constructor invariant for the state-mismatched 'envelope="BUSINESS_RULE", rule_index=None' call so a future refactor (e.g. silent default to rule_index=1) can't regress the BUSINESS_RULE-side rendering. Production call sites always pair the two; this is the latent-invariant gate. 2. Pass 3 (coverage) side observation — pin the rotated default_remediation text. AC #5's byte-equality applies to .message (already pinned); the accompanying remediation hint was deliberately rotated to cover BOTH envelopes ('</MODEL_SQL>' and '</BUSINESS_RULE>') so operators of the new envelope see an actionable steer. Pinning the new text prevents a silent re-rotation back to MODEL_SQL-only wording. 3. Pass 3 (coverage) finding 2a — boring substring match invariant: a rule containing the bare opening tag '<BUSINESS_RULE>' (no slash) does NOT terminate the envelope and must render. Mirrors the safety / grade envelope contract. 4. Pass 3 (coverage) finding 2b — truncated closing fragment '</BUSINESS_RUL' is NOT a breach. Pins the exact-substring contract so a future 'be helpful' regex / case-normalisation refactor can't silently widen the match. 5. Pass 3 (coverage) finding 3 — branch-order invariant: the exclude_tests short-circuit beats the breach scan. An adversarial rule containing '</BUSINESS_RULE>' does NOT raise when 'custom_sql' is excluded because the section never renders. Pins the order. Pass 4 (cross-cutting) finding — 'import pytest as _pytest' alias dropped in favour of plain 'import pytest' to match the in-file precedent at test_render_dynamic_block_rejects_closing_tag_in_raw_code (line 515). Pass 1 (correctness): no P0/P1 bugs. Pass 2 (contract): no DEC / rule-file deviations. Validation: 2662 passed (+5 new), 97.72% coverage, _PROMPT_VERSION unchanged at c9e7ee1f6f465933 (cache-stability gate green). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lan-doc closeout US-004 (#163) inline Patterns & Memory. Updates the durable conventions established by #163: - docs/draft-ops.md — three new subsections: 1. 'Numbered envelope shape (<BUSINESS_RULE id="N">...</BUSINESS_RULE>)' documenting the envelope format + breach guard contract (boring substring match, opening tag / truncated fragments allowed) 2. 'Cardinality contract (at-least-one-per-rule)' documenting the parser-side gate, the verbatim violation message shape, and the collect-all preservation 3. ''exclude_tests' short-circuit' documenting the no-op on both surfaces when 'custom_sql' is in DraftConfig.exclude_tests - .claude/rules/business-rule-tests.md — 'Two input paths' section extended with the #163 'gate-over-prompt' subsection covering: * Dynamic-block envelope (DEC-009 of #163) * Parser cardinality gate (DEC-002 / DEC-006 of #163) * exclude_tests short-circuit (DEC-008 of #163) * Load-bearing 'no _PROMPT_VERSION rotation' note pointing at tests/llm/test_prompt_cache_stability.py as the regression gate - .claude/rules/llm-drafter.md — '<MODEL_SQL> prompt-injection envelope' section heading updated to call out the #163 parameterisation; a new paragraph documents the envelope-parameterised PromptEnvelopeBreachError pattern (one class, N raise sites; never a new error subclass). Future envelopes follow this shape. - plans/super/163-drafter-business-rules-fidelity.md — Beads manifest updated with final statuses (all 4 stories closed) + commit hashes; phase marker advanced to 'complete'. Validation: 2662 passed, 97.72% coverage. _PROMPT_VERSION unchanged at c9e7ee1f6f465933. Note: workers cannot write to .claude/ in worktrees (orchestrator-only per the ralph-worker-claude-dir-perms convention), so US-004's bead description flagged that the .claude/rules/*.md edits would be applied inline by the orchestrator. That's what happened here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
wjduenow
marked this pull request as ready for review
May 30, 2026 02:30
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Implements a Lever B+C1 fix for issue #163, where the drafter was ignoring meta.signalforge.business_rules and hallucinating unrelated rules. The fix combines dynamic-block prompt hardening (numbered XML envelopes per rule) with a parser-side cardinality gate (at-least-one custom_sql per declared rule). No _PROMPT_VERSION rotation, no new error class, no new exit-code entry — mirrors the #159 model_columns_by_type threading precedent and the <MODEL_SQL> envelope-breach precedent.
Changes:
- Render each declared rule inside a numbered
<BUSINESS_RULE id="N">…</BUSINESS_RULE>envelope with an exact-substring breach guard, and short-circuit whencustom_sqlis excluded. - Parameterize
PromptEnvelopeBreachErrorwithenvelope=/rule_index=kwargs (one class, two raise sites), preserving byte-equalMODEL_SQLmessage. - Thread
business_rules: tuple[str, ...]fromdraft_from_request→parse_draft_response→_validate_anchor_contract, which appends a collect-all violation whencustom_sql_count < len(business_rules).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/signalforge/draft/prompts.py | Switch business-rules section to numbered XML envelopes; add closing-tag breach guard; thread exclude_tests short-circuit. |
| src/signalforge/draft/errors.py | Add envelope / rule_index kwargs to PromptEnvelopeBreachError; generalize default_remediation to cover both envelopes. |
| src/signalforge/draft/parser.py | Add business_rules kwarg to parse_draft_response and _validate_anchor_contract; implement at-least-one-per-rule cardinality gate. |
| src/signalforge/draft/schema.py | Build rules tuple via _read_business_rules(model) and pass to parser. |
| tests/draft/test_prompts.py | Cover new envelope render, exclude-tests short-circuit, breach guard (closing tag, opening tag, truncated fragment). |
| tests/draft/test_parser.py | 9 new tests for cardinality gate (under/exact/over coverage, exclusions, mixed scopes, message shape, collect-all). |
| tests/draft/test_errors.py | Pin default/parameterized message shapes and shared remediation content. |
| docs/draft-ops.md | Document numbered envelope, cardinality contract, exclude_tests short-circuit. |
| .claude/rules/llm-drafter.md | Note PromptEnvelopeBreachError is parameterized, not subclassed; future envelopes follow the same shape. |
| .claude/rules/business-rule-tests.md | Capture #163 conventions (envelope, parser gate, no PROMPT_VERSION rotation). |
| plans/super/163-drafter-business-rules-fidelity.md | New plan/decisions/stories doc. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/business-rule-tests.md:
- Line 19: The markdown contains inline code spans with trailing spaces (e.g.,
backticked examples like `</BUSINESS_RULE> `) that trigger MD038; edit the text
around the DEC-009 description and the examples so all inline code spans remove
any leading/trailing spaces inside the backticks (preserve the exact literal
closing tag `</BUSINESS_RULE>` and other examples), keeping the envelope names
and the referenced PromptEnvelopeBreachError and DEC-009 wording intact.
In `@docs/draft-ops.md`:
- Around line 302-304: The inline code spans contain extra spaces which trigger
MD038; update the sentence to remove spaces inside the backticks so the code
spans are normalized (e.g., change `(model) ` to `(model)`, `(column X) ` to
`(column X)`, and ensure `<MODEL_SQL>` has no inner leading/trailing spaces),
preserving the existing scope prefixes and the 2-space indentation for bodies;
locate these spans in the sentence that references `(model) `, `(column X) ` and
`<MODEL_SQL>` and edit them to remove the stray spaces inside the backticks.
In `@plans/super/163-drafter-business-rules-fidelity.md`:
- Around line 30-37: Add a language tag to the fenced code block (e.g., change
``` to ```text) around the "## BUSINESS RULES" section to satisfy MD040, and
remove the internal spacing inside the inline code span so the tag reads as a
contiguous code span `<BUSINESS_RULE id="N">…</BUSINESS_RULE>` (no extra spaces
before/after the backticks) to satisfy MD038; locate the fenced block and the
inline span in the BUSINESS RULES section and update them accordingly.
🪄 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: 1eda6f97-1a2b-477a-9991-b43b02ec6dd7
📒 Files selected for processing (11)
.claude/rules/business-rule-tests.md.claude/rules/llm-drafter.mddocs/draft-ops.mdplans/super/163-drafter-business-rules-fidelity.mdsrc/signalforge/draft/errors.pysrc/signalforge/draft/parser.pysrc/signalforge/draft/prompts.pysrc/signalforge/draft/schema.pytests/draft/test_errors.pytests/draft/test_parser.pytests/draft/test_prompts.py
CodeRabbit found 3 markdownlint warnings on the docs/rules added by US-004: 1. .claude/rules/business-rule-tests.md:19 — MD038 (spaces inside inline code spans). Dropped trailing spaces inside backticks: '(model) ' -> '(model)', '(column X) ' -> '(column X)'. 2. docs/draft-ops.md:304 — same MD038 fix. 3. plans/super/163-drafter-business-rules-fidelity.md:30 — MD040 (fenced code block missing language tag). Added 'text' language to the 'BUSINESS RULES' rendered-output illustration. Also fixed the line-122 MD038 same as (1)/(2). All three are valid markdownlint findings; fixed inline per the closeout rule 'NEVER SILENTLY DEFER ANYTHING.' Validation: 2662 passed, 97.72% coverage. _PROMPT_VERSION unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
PR Review SummaryCodeRabbit posted 3 actionable findings — all valid markdownlint warnings (MD038 inline-code spaces + MD040 missing fence language). All fixed inline in Fixed (3 items)
False Positives (0 items)None — all findings were valid markdownlint warnings. Validation post-fix
CI re-run will pick up |
This was referenced May 30, 2026
Merged
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 #163 —
test_e2e_business_rulesdrafter ignoresmeta.signalforge.business_rulesand hallucinates an unrelated rule.Phase: detailing (awaiting approval)
Stories: 2 implementation + Quality Gate + Patterns & Memory
Decisions: 9 (DEC-001..DEC-009)
Fix shape
Lever B + C1 — dynamic-block envelope hardening + parser cardinality gate:
_PROMPT_VERSIONrotation):count >= len(rules)) enforced in_validate_anchor_contract, surfaced as aLLMOutputAnchorContractErrorviolation.business_rulesthreaded keyword-only fromdraft_from_request→parse_draft_response(mirrors test_e2e_business_rules flake: drafter emits custom_sql with column-type mismatch; rule routes to kept-uncertain instead of kept (BQ rejects with type error) #159'smodel_columns_by_typeprecedent).PromptEnvelopeBreachError—envelope: str = "MODEL_SQL"kwarg. One class, two raise sites. No taxonomy growth.System-prompt restructure (Lever A) is held in reserve if Sonnet 4.6 compliance stays low.
Key invariants preserved (load-bearing)
_PROMPT_VERSIONunchanged (pinned by US-001 AC Quality grader: rubric scoring of surviving artifacts #7 viatests/llm/test_prompt_cache_stability.py)DropReason/ no new error class / no exit-code-table entrybusiness_rulesstay in dynamic block (business-rule-tests.mdDEC-001)testing-signal.md§ "Gate-over-prompt")exclude_tests=("custom_sql",)short-circuits both surfaces (DEC-008)Plan document
See
plans/super/163-drafter-business-rules-fidelity.mdfor full architecture review, decisions log, and story breakdown.Next steps
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests