#188: bulk-mode shared cached prefix for --select - #195
Merged
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 (10)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds a project-scoped cached prompt prefix for multi-model ChangesProject-scope prompt caching
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
Add cache_scope: Literal["per-model", "project"] = "per-model" to DraftConfig under the llm: namespace (DEC-001 of #188). DraftConfig stays extra="forbid" and _DraftConfigFile stays extra="ignore". The docstring names issue #188 and the auto-promote semantics (--select >= 2 models promotes to project) consumed by later stories (US-002..US-008). Establish the StrictDraftConfig drift pair in tests/draft/test_drift_detector.py plus a draft_config_v1.json fixture: field-set parity with production, fixture validation, and a cache_scop typo failing loud. Add config round-trip / Literal-rejection / unknown-llm-sibling-key tests to tests/draft/test_config.py. Foundation field only; renderer / prompt-version / CLI flag / drafter wiring ship in separate beads.
…lope + project rule aggregation
US-002 building blocks for the bulk-mode shared cached prefix:
- _render_project_summary(manifest): compressed 'name (N cols)' line per
model in sorted(unique_id) order; byte-identical regardless of which
model is under draft (the cache-hit precondition, DEC-007). Wrapped in
a <PROJECT_MANIFEST> envelope (DEC-005, envelope only).
- _PROJECT_SUMMARY_TEMPLATE constant for the project block (DEC-004).
- _read_project_business_rules(manifest): deterministic project-wide
aggregation (sorted unique_id -> model-level -> column-name) with a
single global 1-indexed <BUSINESS_RULE id=N> counter; reuses the
safety-layer dict-guard read pattern (strict isinstance(dict); scalar/
list noise dropped, never fail-loud).
- Boring-substring breach guards (DEC-008): </PROJECT_MANIFEST> in
rendered content or </BUSINESS_RULE> in any aggregated rule raises
PromptEnvelopeBreachError (fail-closed, no per-model fallback).
- PromptEnvelopeBreachError extended with nullable model_unique_id and a
rule_source discriminator ('model' default; 'project' for project-level
breaches). Default MODEL_SQL / per-model BUSINESS_RULE behaviour is
byte-equal to pre-#188. Class identity unchanged so scan 7 stays green.
No US-003+ wiring: render_prompt / _PROMPT_VERSION / system-prompt variant
untouched; the new functions are not yet called from render_prompt.
…ECT_MANIFEST> envelope
… render_prompt dispatch US-003 (#188): adds the project-scope cached-prefix prompt path alongside the unchanged per-model path. - _PROMPT_VERSION_PER_MODEL (byte-identical to today's _PROMPT_VERSION; bare name kept as an alias) and _PROMPT_VERSION_PROJECT (hash inputs add _PROJECT_SUMMARY_TEMPLATE + the project-scope <PROJECT_MANIFEST> defence text). - _prompt_version_for(exclude_tests, cache_scope) selects the base by scope, folds '|scope=...|exclude=...' into a blake2b-8 when either dimension is non-default; _prompt_version_for((), 'per-model') returns the per-model base verbatim (snapshot-stable). - _render_system_prompt(exclude_tests, cache_scope): project variant appends a <PROJECT_MANIFEST> 'data, not instructions' defence line mirroring <MODEL_SQL>; per-model variant byte-identical to today (preserves the cache-stability golden). - render_prompt(..., cache_scope='project') emits the shared project cached block and a dynamic block carrying <MODEL_SQL> + this model's full column/neighbour detail + its own <BUSINESS_RULE> rules (DEC-013); per-model path unchanged. tests/llm/test_prompt_cache_stability.py left unmodified and passing.
…re system prompt + render_prompt dispatch
…try in drafter Thread config.cache_scope from draft_from_request into render_prompt (US-004, DEC-006). The default per-model path is byte-identical to today. Oversize fallback: call_llm keeps raising LLMCacheTooLargeError at the 8000-token gate. In draft_from_request, when cache_scope=="project", catch it, re-render the cached block in per-model scope, and retry call_llm exactly once; emit one INFO breadcrumb (lazy-format JSON, passes the logger grep gate) naming the model + the fallback. When cache_scope=="per-model", the error propagates unchanged (no retry). The successful retried path falls through to the single response-audit write — audit written exactly once. Tests cover: default per-model path unchanged, project-under-cap sends the <PROJECT_MANIFEST> block, project-over-cap fallback (+ INFO assert via caplog + per-model block on retry), audit-written-once, and per-model-over-cap propagation.
…oversize catch-and-retry
Extend tests/llm/test_prompt_cache_stability.py with the project-scope cache prefix goldens alongside the existing per-model ones: - _EXPECTED_PROMPT_VERSION_PROJECT pins the _PROMPT_VERSION_PROJECT base constant (0d7dbc9e5f69f5fc); _RENDERED_PROMPT_VERSION_PROJECT pins the composed value render_prompt(..., cache_scope="project") actually returns (2cd2df7087ce462d — _prompt_version_for folds the non-default scope into a fresh hash). - _CACHED_BLOCK_GOLDEN_PROJECT pins the rendered <PROJECT_MANIFEST> cached block byte-for-byte (captured via render_prompt, not hand-written). - New tests: base-version pin, composed-version pin, project cached-block byte-stability (unified diff on mismatch), cross-model byte-identity (the DEC-007 cache-hit precondition), and project != per-model version. - Docstring documents the disjoint lockstep rotation policy (per-model rotates on _SYSTEM_PROMPT/_MANIFEST_SUMMARY_TEMPLATE/_DATA_SECTION_TEMPLATES; project on the project _SYSTEM_PROMPT variant/_PROJECT_SUMMARY_TEMPLATE/ _DATA_SECTION_TEMPLATES). Existing per-model golden + version assertions are untouched and green.
…lay (US-005)
Add the --cache-scope {per-model,project} flag to `signalforge generate`,
mirroring the --mode / --scope / --sample-strategy / --format overlay shape.
- argparse: --cache-scope, default=None sentinel, choices-validated (exit 2
on a bad value), full help text.
- _run_single_model gains a draft_overrides kwarg applied via the canonical
DraftConfig.model_validate({**dump, **overrides}) re-validation overlay
(DEC-010) — None/empty leaves the loaded config untouched so the
single-model positional path stays byte-identical to v0.1.
- Single-model dispatch reflects ONLY an explicit --cache-scope flag; never
auto-promotes.
- _run_batch resolves the overlay once via _resolve_batch_draft_overrides
with precedence: explicit flag > YAML llm.cache_scope (non-default) >
auto-promote to project when >= 2 models matched (DEC-002 / DEC-003).
- Tests across the full matrix (batch>=2 promotes, single-match no-promote,
single-model never-promote, flag-wins, force-project-on-single, YAML
honoured, overlay re-validates via model_validate, invalid value exit 2),
all asserting the no-traceback floor.
…e caveat + 5-surface parity test (US-007)
Add the --cache-scope {per-model,project} flag reference to the
signalforge generate flag list in docs/cli-ops.md, covering default
per-model, auto-promotion to project on --select >= 2 models, and the
precedence (explicit flag > YAML llm.cache_scope > auto-promote).
Correct the stale Anthropic prompt-cache caveat in the 'Running across
many models' section: under cache_scope=project the shared project
prefix is cached once and read across the batch, so the marked cache
now DOES amortise across siblings. Per-model scope retains the prior
no-amortise behaviour; document the oversize fallback.
Ship tests/cli/test_5_surface_parity_cache_scope.py mirroring the
--select parity test: hard asserts that --cache-scope, per-model, and
project appear in argparse --help, docs/cli-ops.md, and the plan.
Add a 'Bulk-mode shared cache' section to docs/draft-ops.md covering what the feature is (drafter --select cache hit 0% -> ~95%), how the shared project prefix works, automatic activation on --select >= 2 models plus the --cache-scope override + precedence, the oversize auto-fallback to per-model, and a cost model derived from the verified Sonnet pricing multipliers (write 1.25x input, read 0.1x input). Add a provider & model applicability subsection verified against the code: AnthropicProvider.supports_prompt_caching=True (Sonnet/Opus min 1024 tokens, Haiku 2048 per _MIN_CACHEABLE_TOKENS in llm/client.py); OpenAI and Gemini supports_prompt_caching=False -> no benefit and no penalty (the restructure is a no-op for the cost lever on those providers). Cross-link the new section from README's Supported LLM providers area. SKILL.md left unchanged: it documents single-model generate only, not --select batch runs.
…puts (DEC-014) Code review (4 passes: correctness/security/CLI/tests) found one real MEDIUM: _PROMPT_VERSION_PROJECT folded _MANIFEST_SUMMARY_TEMPLATE into its hash, so the project version rotated on per-model template changes — violating DEC-014 rotation independence. Removed that term; project base version 0d7dbc9e->49e58185, composed 2cd2df70->e3ec5997. Updated the two pinned constants in test_prompt_cache_stability.py + the stale docstring in lockstep. CodeRabbit unavailable in this environment. ruff/format/pyright clean; 3435 passed.
US-010 (Patterns & Memory). Document the project-scope shared cached prefix conventions from #188 across four rule files + plan references: - llm-drafter.md: new "Project-scope cached prefix for --select batches" section — DraftConfig.cache_scope, _render_project_summary + <PROJECT_MANIFEST> envelope, deterministic total order, dual _PROMPT_VERSION + _prompt_version_for dispatch, oversize catch-and-retry vs breach fail-closed, and the DEC-014 rotation-independence lesson (project version hashes _PROJECT_SUMMARY_TEMPLATE, not the per-model one). - cli-layer.md: correct the DEC-15 sibling-cache caveat (now amortises under project scope); document the draft_overrides overlay + --cache-scope flag + auto-promote precedence. - business-rule-tests.md: project-wide rule aggregation (_read_project_business_rules, global 1-indexed counter) + DEC-013 dual-placement rule (cardinality gate reads the Model object). - testing-signal.md: second cache-stability golden + cross-model byte-identity test pattern. Docs-only; no production/test code changed. Validation green (ruff, format, pyright, 3435 pytest passed).
wjduenow
marked this pull request as ready for review
June 3, 2026 14:41
wjduenow
changed the title
There was a problem hiding this comment.
Pull request overview
This PR implements issue #188’s “bulk-mode shared cached prefix” for signalforge generate --select <expr> runs by introducing a project-scoped cached prompt prefix that is byte-identical across models in a batch, substantially increasing Anthropic prompt-cache hit rates and reducing input-token costs. It adds configuration + CLI controls for cache scope, a safe oversize fallback, and pins the new behavior with expanded prompt stability tests and docs.
Changes:
- Add
DraftConfig.cache_scope(per-modeldefault,projectfor shared batch prefix) and thread it through prompt rendering, drafter execution, and CLI overlay/auto-promote logic. - Implement project-scope cached prefix rendering (
<PROJECT_MANIFEST>…</PROJECT_MANIFEST>), dual prompt-version bases, and an oversize catch-and-retry fallback to per-model scope. - Expand tests (goldens + cross-model byte identity + CLI precedence + drift detection) and update operator docs / parity surfaces.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/signalforge/draft/config.py |
Adds cache_scope config field and documents semantics. |
src/signalforge/draft/prompts.py |
Adds project-scope cached prefix rendering, scope-aware system prompt, and dual _PROMPT_VERSION scheme. |
src/signalforge/draft/schema.py |
Threads cache_scope into prompt render + adds oversize fallback (project→per-model retry). |
src/signalforge/draft/errors.py |
Extends PromptEnvelopeBreachError for project-scope envelope breaches. |
src/signalforge/cli/generate.py |
Adds --cache-scope, implements batch auto-promote + precedence and applies overrides via model_validate. |
tests/draft/test_config.py |
Validates cache_scope default/validation and YAML round-trip / typo failure behavior. |
tests/draft/test_drift_detector.py |
Adds strict DraftConfig drift detection + fixture validation for field-set changes. |
tests/fixtures/draft/draft_config_v1.json |
New DraftConfig fixture used by drift detector tests. |
tests/draft/test_prompts.py |
Adds comprehensive unit tests for project-scope rendering, breach guards, and version composition. |
tests/draft/test_schema.py |
Adds integration tests ensuring correct cached block is sent + oversize fallback behavior + audit write count. |
tests/llm/test_prompt_cache_stability.py |
Adds project-scope golden + version pins and cross-model cached-prefix byte-identity assertions. |
tests/cli/test_generate_cache_scope.py |
Pins CLI precedence/auto-promote behavior by asserting forwarded DraftConfig.cache_scope. |
tests/cli/test_5_surface_parity_cache_scope.py |
Adds 5-surface parity enforcement for --cache-scope token consistency. |
docs/draft-ops.md |
Documents bulk-mode shared cache behavior, fallback, and provider applicability. |
docs/cli-ops.md |
Documents --cache-scope and updates multi-model Anthropic caching guidance. |
README.md |
Adds high-level pointer to bulk-mode shared cache documentation. |
plans/super/188-bulk-cache-prefix.md |
Adds implementation plan / DEC log to anchor the feature contract. |
.claude/rules/llm-drafter.md |
Captures new drafter caching patterns, invariants, and fallback semantics. |
.claude/rules/cli-layer.md |
Updates CLI conventions and corrects the sibling-cache caveat in light of project scope. |
.claude/rules/testing-signal.md |
Documents the “second golden + cross-model byte identity” testing pattern for shared cached prefixes. |
.claude/rules/business-rule-tests.md |
Documents project-scope aggregated business-rule placement + determinism requirements. |
💡 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.
🧹 Nitpick comments (1)
src/signalforge/draft/errors.py (1)
445-493: 💤 Low valueConsider adding parameter combination validation.
The error message construction branches on
rule_sourcebut doesn't validate that parameter combinations are sensible. For example, calling withrule_source="model"(the default) plusenvelope="PROJECT_MANIFEST"would fall through to theelseblock and produce a message about</MODEL_SQL>, which is misleading.All current call sites in
prompts.pypass correct combinations, so this is a low-priority defensive suggestion rather than an active bug.🛡️ Optional validation guard
) -> None: self.model_unique_id = model_unique_id self.envelope = envelope self.rule_index = rule_index self.rule_source = rule_source + # Validate sensible parameter combinations + if rule_source == "model" and envelope == "PROJECT_MANIFEST": + raise ValueError( + "rule_source='model' is incompatible with envelope='PROJECT_MANIFEST'; " + "use rule_source='project' for project-level envelopes." + ) if rule_source == "project":🤖 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/draft/errors.py` around lines 445 - 493, The constructor builds misleading messages for invalid parameter combos (e.g., rule_source="model" with envelope="PROJECT_MANIFEST"); add an early validation guard in the __init__ method that checks envelope and rule_source consistency (and rule_index presence only when envelope == "BUSINESS_RULE") and raise a clear ValueError if combinations are unsupported; reference the local symbols envelope, rule_source, rule_index, model_unique_id and place the check before the existing branching so the later message construction (which uses _format_value and super().__init__) only runs for valid combinations.
🤖 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.
Nitpick comments:
In `@src/signalforge/draft/errors.py`:
- Around line 445-493: The constructor builds misleading messages for invalid
parameter combos (e.g., rule_source="model" with envelope="PROJECT_MANIFEST");
add an early validation guard in the __init__ method that checks envelope and
rule_source consistency (and rule_index presence only when envelope ==
"BUSINESS_RULE") and raise a clear ValueError if combinations are unsupported;
reference the local symbols envelope, rule_source, rule_index, model_unique_id
and place the check before the existing branching so the later message
construction (which uses _format_value and super().__init__) only runs for valid
combinations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f724b4cd-7d88-44ca-8e98-e8da16bdbe5b
📒 Files selected for processing (21)
.claude/rules/business-rule-tests.md.claude/rules/cli-layer.md.claude/rules/llm-drafter.md.claude/rules/testing-signal.mdREADME.mddocs/cli-ops.mddocs/draft-ops.mdplans/super/188-bulk-cache-prefix.mdsrc/signalforge/cli/generate.pysrc/signalforge/draft/config.pysrc/signalforge/draft/errors.pysrc/signalforge/draft/prompts.pysrc/signalforge/draft/schema.pytests/cli/test_5_surface_parity_cache_scope.pytests/cli/test_generate_cache_scope.pytests/draft/test_config.pytests/draft/test_drift_detector.pytests/draft/test_prompts.pytests/draft/test_schema.pytests/fixtures/draft/draft_config_v1.jsontests/llm/test_prompt_cache_stability.py
- Resolve tests/draft/test_drift_detector.py conflict (union: #188 draft-config drift tests + dev #184 reshape-record tests). - dev #184 rotated the per-model drafter system prompt; project-scope version pins recomputed in lockstep (base 49e58185->1bfec838, composed e3ec5997->77edde8a). Per-model pin 32d33f14 (from dev) intact. - PR review (Copilot, 4 threads): the <PROJECT_MANIFEST> prefix carries model names + column counts (+ aggregated business rules), NOT model descriptions. Corrected the defence line, PromptEnvelopeBreachError remediation + docstring, and docs/draft-ops.md to match the real prompt/injection surface. - Fixed redundant 'The project summary (the project summary)' breach message when the offending model is unknown; pinned the corrected message + a regression guard in tests.
Owner
Author
PR Review SummaryAll four Copilot threads were real (no false positives) and clustered on one theme: prose claimed the Fixed (4 items)
Lockstep follow-through
False positivesNone. Validation green after the fixes: |
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
Lifts the LLM drafter's Anthropic prompt-cache hit rate across a multi-model
signalforge generate --select <expr>batch from 0% → ~95%. Previously each model paidcache_creationfrom scratch because the cached block was per-model; this restructures it into a project-level shared prefix that is byte-identical across the batch, so models 2..N hitcache_read(~12× cheaper input-side).Ticket: #188 · Plan:
plans/super/188-bulk-cache-prefix.md· surfaced by the #179 retest (PR #182).Changes
DraftConfig.cache_scope: Literal["per-model","project"] = "per-model"(llm:namespace) + drift detection.name (N cols)summary + project-wide business rules, wrapped in a<PROJECT_MANIFEST>envelope with a boring-substring breach guard. Deterministic total order (sorted(unique_id)→ column → global rule counter) — the cache-hit precondition. Per-model detail moves to the uncached dynamic block._PROMPT_VERSION(_PER_MODEL== historic value,_PROJECT) +_prompt_version_for(exclude_tests, cache_scope)dispatch; scope-aware system-prompt defence line (project variant only, preserving the per-model golden byte-for-byte).LLMCacheTooLargeErrorand retries per-model once when project-scope; breach fails closed (security) — deliberately distinct.--cache-scope {per-model,project}+ auto-promote on--select≥ 2 models; precedence flag > non-default YAML > auto-promote;draft_overridesoverlay viamodel_validate.docs/draft-ops.mdbulk-mode section + provider/model applicability matrix (Anthropic-only; Sonnet/Opus min 1024, Haiku 2048; OpenAI/Gemini no-op);docs/cli-ops.mdflag reference + corrected sibling-cache caveat; 4 rule files updated.10 stories (8 implementation + Quality Gate + Patterns & Memory), executed via Ralph in isolated worktrees.
Testing
uv sync --dev && uv run ruff check . && uv run ruff format --check . && uv run pyright && uv run pytest— green at every merge (pyright 0 errors; 3,435 passed, 8 skipped).--select— cache hit rate from 0% to ~95% across N models #188 (existing cache-stability golden unchanged); new project golden + cross-model byte-identity test pinned._PROMPT_VERSION_PROJECTrotation independence (DEC-014); pins updated in lockstep.Compounding Update
.claude/rules/llm-drafter.md,cli-layer.md,business-rule-tests.md,testing-signal.mdupdated with the project-scope cache patterns (US-010).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Behavior
Documentation
Tests