Skip to content

feat(cache): generalize breakpoint eligibility to unstable-suffix, not trailing-only - #109

Merged
Brian Krabach (bkrabach) merged 2 commits into
mainfrom
feat/reminder-redesign-w5
Aug 31, 2026
Merged

feat(cache): generalize breakpoint eligibility to unstable-suffix, not trailing-only#109
Brian Krabach (bkrabach) merged 2 commits into
mainfrom
feat/reminder-redesign-w5

Conversation

@bkrabach

@bkrabach Brian Krabach (bkrabach) commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

What

_count_trailing_ephemeral_messages walked backward from the end and
stopped at the first message that was either role="tool" or NOT marked
ephemeral -- so it could only ever exclude a TRAILING run of ephemeral
messages from cache-breakpoint eligibility. The in-flight system-reminder
redesign (amplifier-module-loop-streaming) introduces a LEADING pre-user
reminder block: [..., block(ephemeral, ...), user(real), ...]. Under
ephemeral_injection_mode="tail" + reminder_placement="pre_user", that
block is regenerated per request (never persisted) but is no longer
trailing -- the old walk's count stayed 0, so a cache breakpoint could land
at or after the block, guaranteeing a cache miss on the very next request.

Fix: replaces _count_trailing_ephemeral_messages with
_unstable_suffix_length, keyed on the metadata signal already present in
the data: ephemeral=True, persisted=True = STABLE (frozen canonical
history); ephemeral=True alone = UNSTABLE (regenerated per request).
Everything from the last unstable message through the end is excluded,
whether or not that message is itself last.

This is a strict generalization -- verified against every shape in the
spec's own table (see commit message for the full comparison table): today's
tail-mode shape is byte-identical; the redesign's new pre-user shape is
fixed; persist-mode shapes are unaffected or improved (a stable trailing
message is no longer over-conservatively excluded).

_find_rolling_secondary_index is deliberately left unchanged, per the
spec's own instruction -- but its docstring is corrected here to describe
the actually-verified behavior (empirically probed, not just derived on
paper): the secondary breakpoint's real benefit for the pre-user-block shape
is a WITHIN-A-TURN property (stable across a turn's own tool-loop
iterations), not the cross-turn property the spec's hand-derived trace
assumed. See the updated docstring and the corrected T-W5-04 test for the
full derivation.

Spec

Workstream W5 of the system-reminder redesign
(reminder-redesign-spec.md, section W5). Per the spec's shipping order
(section 13), W5 must land before W1 -- it's correct for today's shapes
too, so it can ship ahead safely, and W1 would introduce a tail-mode cache
regression without it.

Testing

  • 754 passed (747 baseline + 7 net new/changed). uv run pytest -q.
  • Fail-before proof (git stash against unmodified 833403b, testing
    only the new/changed tests): T-W5-01 and T-W5-03 genuinely fail on old
    code (2 failed) -- confirming both directions of the bug (a breakpoint
    landing directly on the unstable block AND on a persisted-but-excluded
    trailing message). The other 5 new/changed tests pass on both old and new
    code -- documented explicitly as pins, not fail-before proofs, in each
    docstring (see "spec ambiguity resolved" note below).
  • python_check: 16 errors / 19 warnings across both touched files,
    confirmed via git stash compare to be identical to baseline and
    unrelated to this diff -- zero new issues introduced.

Spec ambiguity resolved (T-W5-04)

The spec's own table marks T-W5-04 ("secondary lands on turn N's last
message = turn N's primary") as a fail-before proof. I could not reproduce
that claim: empirically probing the actual algorithm shows the cross-turn
overlap the spec describes does not hold for the pre-user-block shape
(both the "prior" and "current" hypothetical requests independently
self-match on their own just-arrived trailing user message, so neither
reaches back into the other's history). What does hold, verified by
direct probe, is a within-turn property: the secondary breakpoint stays on
the SAME pre-user block across a turn's own tool-loop iterations (iteration
1, before any reply, and iteration 2+, after a tool call, both resolve to
the same secondary index). I corrected both the _find_rolling_secondary_ index docstring and the T-W5-04 test to describe this verified property
instead of the spec's unverified one -- flagging this explicitly rather than
either silently deviating or shipping a test asserting something false.

Merge note

This PR is intentionally left OPEN, not merged -- pending DTU validation
(the 2x2 ephemeral_injection_mode x reminder_placement matrix plus the
0629f373 reproduction leg, spec section 12), per the task's DTU-gated-merge
policy. Per the spec's shipping order, W5 should merge before W1 lands, but
after DTU validation confirms the design.

🤖 Generated with Amplifier

Co-Authored-By: Amplifier 240397093+microsoft-amplifier@users.noreply.github.com

Update: verified against the 20260831-rr validation wave's findings

A DTU validation wave (20260831-rr) ran this PR (plus W1/W2/W3) end-to-end
and returned DO-NOT-MERGE on two regressions -- an anthropic
prompt-cache collapse (D1, fixed in hooks-status-context and
routing-matrix's own PRs) and envelope-header accumulation (D2, fixed in
loop-streaming's PR). This PR (W5) was called out as a suspect (the
report asked "why is ~22K/turn being appended to system? ... does W5's
own cache-breakpoint walk preserve caching under this?").

Verified: no code change needed here for either defect.

  • D1 is entirely about the system_prompt_factory wrapping
    mechanism (hooks-status-context / routing-matrix / tool-skills each
    wrapping context.set_system_prompt_factory) -- this provider's
    cache-breakpoint walk operates on the message list, which is
    completely orthogonal to how the system prompt string is assembled.
    No change needed.
  • D2's fix (in loop-streaming) changes ONLY the literal TEXT of a
    subsequent same-turn persisted reminder message (strips the repeated
    descriptive boilerplate header), leaving metadata
    (ephemeral/persisted/reminder_placement) and message
    role/position completely unchanged. _unstable_suffix_length keys
    ONLY off msg.role and msg.metadata -- never msg.content -- so a
    header-less persisted reminder message is treated identically to a
    full-header one.

Added test_persisted_reminder_eligibility_unaffected_by_d2_header_suppression
to tests/test_prompt_cache_breakpoints.py to pin this invariant
explicitly (constructs both a full-header turn-start block and a
header-less mid-loop block per D2's exact shape, asserts breakpoint
eligibility holds for both) rather than leaving it as an inference from
reading the source. 35 -> 36 passed in this file; 754 -> 755 passed in
the full suite. No regressions, no functional changes.

…t trailing-only

`_count_trailing_ephemeral_messages` walked backward from the end and
stopped at the first message that was either role="tool" or NOT marked
ephemeral -- so it could only ever exclude a TRAILING run of ephemeral
messages from cache-breakpoint eligibility. The system-reminder redesign
(in-flight, amplifier-module-loop-streaming) introduces a LEADING
pre-user reminder block: `[..., block(ephemeral, ...), user(real), ...]`.
Under `ephemeral_injection_mode="tail"` + `reminder_placement="pre_user"`,
that block is regenerated per request (never persisted) but is no longer
trailing -- the old walk's count stayed 0, so a cache breakpoint could
land at or after the block, guaranteeing a cache miss on the very next
request (the exact "write every turn, read never" failure class this
provider's cache-breakpoint logic exists to prevent).

Replaces `_count_trailing_ephemeral_messages` with
`_unstable_suffix_length`, keyed on the metadata signal already present
in the data: `ephemeral=True, persisted=True` = STABLE (frozen canonical
history, written via `context.add_message`, safe to cache past);
`ephemeral=True` alone (no `persisted`) = UNSTABLE (regenerated per
request). Everything from the last unstable message through the end of
the conversation is excluded from eligibility, whether or not that
message is itself last.

This is a STRICT GENERALIZATION, verified against every shape in the
spec's own table plus a from-scratch stash-compare of the full suite:
- Tail mode today (`[..., asst, block]`): old excludes 1, new excludes 1.
  Identical.
- Tail mode + the redesign's pre-user placement (`[..., asst, block,
  user]`): old excludes 0 (bug), new excludes 2 (fixed).
- Persist mode (`[..., block(persisted), user]`): old excludes 0, new
  excludes 0. Identical (the block is genuinely frozen history).
- Persist mode, mid-loop trailing block (`[..., block(persisted)]`): old
  excludes 1 (over-conservative -- the old walk never checked
  `persisted`), new excludes 0 (improvement: a stable tail can now take a
  breakpoint).
- All-ephemeral request: old excludes len, new excludes len. Identical
  (the `eligible_upper <= 0` guard still fires).
- No metadata anywhere: unaffected either way (the loud-skip path is
  untouched).

`_find_rolling_secondary_index` (the secondary/rolling cache breakpoint)
is DELIBERATELY LEFT UNCHANGED, per the spec's own instruction -- but its
docstring is corrected here to describe the ACTUALLY VERIFIED behavior
under the new pre-user-block shape (empirically probed, not just derived
on paper): the secondary breakpoint's real benefit for this shape is a
WITHIN-A-TURN property (stable across a turn's own tool-loop iterations),
not a cross-turn one as the spec's hand-derived trace assumed -- see the
updated docstring and T-W5-04's test for the full derivation and the
concrete counter-example that led to the correction.

Tests (tests/test_prompt_cache_breakpoints.py, 28 -> 35):
- Updated: the docstring on the existing trailing-tail test now states
  explicitly that it stays a genuinely UNSTABLE shape (no `persisted`
  key), and a new persisted-metadata variant of the existing
  multi-message regression guard is added alongside it.
- New (6 tests, T-W5-01..06): unstable block before the trailing user
  excludes both from eligibility (FAILS on 833403b -- confirmed via `git
  stash`: old code stamped BOTH the block and the trailing user);
  persisted block before the user does not reduce eligibility (passes
  both before and after -- documented pin, not a fail-before proof, see
  PR description); a persisted TRAILING message may now take a
  breakpoint (FAILS on 833403b -- confirmed via `git stash`: old code
  always excluded it); the rolling secondary breakpoint's real
  within-turn-iteration behavior under the pre-user-block shape (passes
  both before and after -- `_find_rolling_secondary_index` is untouched);
  an unstable message buried behind a tool-role batch degrades safely,
  no crash; the pre-user-block shape still respects the 4-breakpoint
  hard limit.

Suite: 754 passed (747 baseline + 7 net new/changed).
`python_check`: 16 errors / 19 warnings across both touched files,
confirmed via `git stash` compare against unmodified `833403b` to be
identical, pre-existing, and unrelated to this diff (mock `.get()` typed
as list-index access, blind-except lint warnings elsewhere in the file,
etc.) -- zero new issues introduced.

Spec: reminder-redesign-spec.md, section W5. Per the spec's
dependency-ordered shipping order (section 13), W5 must land before W1
(amplifier-module-loop-streaming) -- it is a strict generalization
correct for today's shapes too, so it ships ahead safely and prevents W1
from introducing a tail-mode cache regression when it lands.

Merge note: this PR is intentionally left OPEN, not merged, pending DTU
validation per the task's DTU-gated-merge policy (see PR description).

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
… fix

The 20260831-rr treatment-validation wave's D2 finding (envelope
accumulation) was fixed in amplifier-module-loop-streaming by writing
subsequent same-turn persisted reminder messages WITHOUT the repeated
descriptive boilerplate header -- a change to message CONTENT only,
never to metadata or role/position.

_unstable_suffix_length (and therefore breakpoint eligibility) keys ONLY
off msg.role and msg.metadata -- never msg.content -- so this provider
needs no code change for D2. This test pins that invariant explicitly:
a header-less persisted reminder message must be treated identically to
a full-header one (both STABLE, both breakpoint-eligible), rather than
leaving it as an inference from reading the source.

No functional change to this provider. D1 (the anthropic prompt-cache
collapse, fixed in hooks-status-context and routing-matrix) is also
independently confirmed to require no change here: the cache-breakpoint
walk operates on the MESSAGE list, entirely orthogonal to the
system-prompt-factory-wrapping mechanism D1 fixes.

35 -> 36 passed in this file; 754 -> 755 passed in the full suite. No
regressions.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@bkrabach

Copy link
Copy Markdown
Collaborator Author

Maintainer merge — system-reminder redesign (W5: provider-anthropic)

Merging via --admin to bypass the review-approval gate only (mergeStateStatus: BLOCKED, reviewDecision: REVIEW_REQUIRED). No CI/status check is being bypassed: full CI matrix is green — pytest on ubuntu-latest/macos-latest/windows-latest × py3.11/py3.12 (6/6 SUCCESS) plus license/cla (SUCCESS), all at HEAD d5330dbb57d070cfa529bb463f0ca71ad1683920.

Validation evidence: two DTU validation waves cleared this redesign end-to-end. Second wave (20260831-rr2) passed all pre-registered gates — Anthropic cache parity restored (94%/92%), system prompt byte-stable (+0B both providers), exactly one envelope header per turn, structural assertions clean. Capture roots: 20260831-rr, 20260831-rr2.

Merge order: this PR (W5) is merge 2 of 4 in the mandated dependency-ordered sequence (W3 → W5 → W1 → W2). This generalization (unstable-suffix cache-breakpoint walk) is a spec-enforced prerequisite for W1's placement change (loop-streaming #46) landing next.

@bkrabach
Brian Krabach (bkrabach) merged commit 8a1f837 into main Aug 31, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants