From 3a5218dcda72311c0000c103d24f24d3d77529a7 Mon Sep 17 00:00:00 2001 From: Samuel Lee Date: Fri, 4 Sep 2026 13:01:36 -0700 Subject: [PATCH] fix: compaction notice degrades to a standing form between escalations (#513) --- README.md | 20 +++ amplifier_module_context_simple/__init__.py | 85 ++++++++++- .../DONE-NOTE.md | 125 ++++++++++++++++ .../test_sticky_compaction_and_tail_notice.py | 137 ++++++++++++++++++ 4 files changed, 360 insertions(+), 7 deletions(-) create mode 100644 docs/lanes/513-standing-compaction-notice/DONE-NOTE.md diff --git a/README.md b/README.md index 0386526..2b091b9 100644 --- a/README.md +++ b/README.md @@ -90,6 +90,26 @@ Anthropic API requires that every tool_use in message N has a matching tool_resu **Critical implementation detail**: When an assistant message has multiple tool_calls, there are multiple consecutive tool_result messages after it. The compaction logic walks backwards through these tool results to find the originating assistant message, ensuring the entire tool group is preserved as an atomic unit. This prevents orphaned tool results that would cause API validation errors. +### Compaction Notice + +When `compaction_notice_enabled` is true and the sticky compaction level meets +`compaction_notice_min_level`, a tail-position notice is appended to the returned view (never +persisted to history) so the model knows compaction happened. It comes in two forms: + +- **Full** (`source="context-compaction"`): the complete incident report -- level, message/token + counts, what was preserved, what may be affected. Emitted the first time the model is told about + a given compaction. +- **Standing** (`source="context-compaction-standing"`): a short, self-contained "nothing new + happened since last time" reminder, with an explicit "don't re-verify" instruction. Emitted on + every subsequent request until the *next* real compaction occurs, instead of repeating the full + report byte-for-byte forever. + +Both forms are gated by the same `compaction_notice_enabled` / `compaction_notice_min_level` +settings and both carry `metadata["source"] = "context-compaction"` (so existing consumers that +filter on that value see both); a `metadata["notice_kind"]` of `"full"` or `"standing"` is added +for telemetry. `compaction_notice_verbosity` applies to the full notice only. No new configuration +option is introduced by this distinction. + ## Where the compaction trigger comes from The trigger is one multiplication: diff --git a/amplifier_module_context_simple/__init__.py b/amplifier_module_context_simple/__init__.py index f5f6396..ba055aa 100644 --- a/amplifier_module_context_simple/__init__.py +++ b/amplifier_module_context_simple/__init__.py @@ -244,6 +244,21 @@ def __init__( self.token_meter = token_meter self._hooks = hooks self._last_compaction_stats: dict[str, Any] | None = None + # Which compaction-stats object the model has already been TOLD about. + # Compared by identity against `_last_compaction_stats`, which is + # replaced wholesale (never mutated) on each real escalation -- + # see _finalize_compaction_with_stats. This is what lets + # get_messages_for_request() distinguish "a compaction just happened" + # from "the same compaction I already announced, N turns ago", so the + # tail notice can degrade to a short standing form instead of + # replaying a stale full report on every request forever (#513). + # + # NOT keyed on stats["strategy_level"]: that is the sticky monotonic + # high-water mark (see _sticky_level below), which pins at its ceiling + # early in a long session -- every later escalation reports the SAME + # level while still dropping real messages. Keying on level would + # silently suppress the notice for genuinely new compaction work. + self._notice_shown_for_stats: dict[str, Any] | None = None # Real-usage token meter state (see _on_llm_response / # _measure_working_tokens). `_last_measured_prompt_tokens` holds the # most recent real usage observed via `llm:response` @@ -465,11 +480,14 @@ async def get_messages_for_request( # the cached prefix. Tail + ephemeral=True + role != "system" is # the only combination the existing provider fix recognizes. # - # The notice content itself only changes when a NEW compaction - # escalation actually occurs (see _compact_ephemeral's sticky decision - # state) -- so on calls between escalations, this tail addition is - # byte-identical, and everything before it (the real prefix) is - # completely undisturbed either way. + # The notice's TEXT varies (full report on a new escalation, short + # standing reminder on repeats -- see _format_standing_compaction_notice), + # and that is cache-irrelevant by construction: the notice is never + # persisted into self.messages and is always the LAST element, so the + # prefix shared between consecutive requests never contains a notice at + # all. Cache safety comes from role != "system" + metadata.ephemeral=True + # + tail position (points 1 and 2 above), not from the notice's bytes. + # Everything before it (the real prefix) is undisturbed either way. if self.compaction_notice_enabled and self._last_compaction_stats: level = self._last_compaction_stats.get("strategy_level", 0) # GUARD: never append into an unanswered tool_calls turn. @@ -498,7 +516,20 @@ async def get_messages_for_request( "It will be appended on the next request instead." ) elif level >= self.compaction_notice_min_level: - notice = self._format_compaction_notice() + # Fresh vs. standing (#513). Identity, not equality: the + # stats dict is replaced wholesale on each real escalation + # (_finalize_compaction_with_stats) and never mutated, so + # `is` is an exact "something actually changed" signal -- + # unlike strategy_level, which is a monotonic high-water + # mark that stops changing long before compaction does. + is_new_escalation = ( + self._last_compaction_stats is not self._notice_shown_for_stats + ) + notice = ( + self._format_compaction_notice() + if is_new_escalation + else self._format_standing_compaction_notice() + ) if notice: compacted.append( { @@ -506,12 +537,17 @@ async def get_messages_for_request( "content": notice, "metadata": { "source": "context-compaction", + "notice_kind": "full" + if is_new_escalation + else "standing", "ephemeral": True, }, } ) + self._notice_shown_for_stats = self._last_compaction_stats logger.debug( - f"Appended compaction notice at tail (level {level}, " + f"Appended {'full' if is_new_escalation else 'standing'} " + f"compaction notice at tail (level {level}, " f"verbosity: {self.compaction_notice_verbosity})" ) @@ -599,6 +635,7 @@ async def set_messages(self, messages: list[dict[str, Any]]) -> None: self._stubbed_seqs = set() self._sticky_level = 0 self._last_compaction_stats = None + self._notice_shown_for_stats = None logger.info(f"Restored {len(messages)} messages to context") async def clear(self) -> None: @@ -610,6 +647,7 @@ async def clear(self) -> None: self._stubbed_seqs = set() self._sticky_level = 0 self._last_compaction_stats = None + self._notice_shown_for_stats = None self._last_measured_prompt_tokens = None self._last_token_meter_stats = None logger.info("Context cleared") @@ -1861,6 +1899,39 @@ def _format_compaction_notice(self) -> str: return notice + def _format_standing_compaction_notice(self) -> str: + """Format the SHORT 'nothing new happened' compaction notice. + + Emitted in place of the full report on every request between real + escalations (#513). Deliberately SELF-CONTAINED: it must not refer + back to the earlier full notice, because that notice is not in this + request -- notices are appended to the ephemeral view only and are + never written to self.messages. + + Byte-stable across consecutive repeats at the same level. Can be + overridden by subclasses alongside _format_compaction_notice. + + Returns: + Formatted notice string, or empty string if no stats available. + """ + if not self._last_compaction_stats: + return "" + + level = self._last_compaction_stats.get("strategy_level", 0) + + return ( + '\n' + f"Context compaction is still in effect (level {level}/8), and NOTHING NEW " + "was compacted for this request. This is a standing reminder of an " + "existing condition, not a new event: no messages or tool results were " + "dropped since your previous turn.\n" + "No action is needed. Continue the current task -- do not re-read files, " + "re-run tools, or re-verify state on account of this reminder. Truncated " + "tool results carry their own inline truncation markers; re-run a tool " + "only if you actually need content you can see is missing.\n" + "" + ) + def _format_affected_items(self, level: int, stats: dict[str, Any]) -> str: """ Format affected items based on compaction level. diff --git a/docs/lanes/513-standing-compaction-notice/DONE-NOTE.md b/docs/lanes/513-standing-compaction-notice/DONE-NOTE.md new file mode 100644 index 0000000..0e06e0c --- /dev/null +++ b/docs/lanes/513-standing-compaction-notice/DONE-NOTE.md @@ -0,0 +1,125 @@ +# DONE-NOTE — `fix-513-standing-compaction-notice` + +`context-simple: compaction notice degrades to a standing form between escalations (#513)` + +Branch: `fix-513-standing-compaction-notice` · repo: `microsoft/amplifier-module-context-simple` · +branched from `main` @ `f2dbde9`. + +Bug: `_last_compaction_stats` is never cleared during normal operation, so once the first +compaction happens the notice gate at `get_messages_for_request` is true on every subsequent +request, and the FULL incident report is appended byte-identically forever, with no marker that +it is stale. Fix: track which stats object has already been announced +(`_notice_shown_for_stats`), compared by **identity**, and emit a short self-contained "standing" +notice instead of the full report whenever nothing new has been compacted since the last +announcement. + +--- + +## 1. The identity-vs-level correction (spec §2.1), with probe output + +The obvious-looking fix is to key "has this been announced" off +`stats["strategy_level"]` — compare the level in the new stats to the level last shown, only emit +the full notice again when the level *changes*. **This is wrong, and was caught before writing any +code**, because `strategy_level` is not a per-escalation value — it is `self._sticky_level`, a +monotonic high-water mark (`__init__.py`, sticky-state block near the constructor; assigned via +`max(...)` inside `_finalize_compaction_with_stats`). It climbs early in a session and then pins at +its ceiling while compaction keeps firing underneath it. + +**Probe, run against the real ladder before implementing** (25-turn synthetic session, default +knobs): genuine escalations (a freshly-assigned stats object, i.e. `stats is not last_stats`) +occurred at calls **0, 5, 9, 14, 18, 23**. `strategy_level` at each of those six calls: + +| call | 0 | 5 | 9 | 14 | 18 | 23 | +|---|---|---|---|---|---|---| +| `strategy_level` | 8 | 8 | 8 | 8 | 8 | 8 | +| `messages_removed` | 22 | 29 | 40 | 47 | 58 | 65 | + +Level is `8` at every single one of the six real escalations, while `messages_removed` keeps +climbing. A level-keyed implementation would emit the full notice once (at call 0, when level +first reaches its production ceiling), and then **permanently suppress it for the rest of the +session** — hiding five subsequent real compactions that each dropped tens of additional messages. +That is worse than the bug being fixed: it doesn't just show a stale notice, it shows *no* notice +at all for the majority of real events. + +**Fix used instead: object identity on the stats dict itself.** +`_last_compaction_stats` is assigned a fresh dict at exactly one call site +(`_finalize_compaction_with_stats`) and is never mutated afterward (checked: no `stats[...] = ` +assignment anywhere after construction). So +`self._last_compaction_stats is not self._notice_shown_for_stats` is an exact, zero-cost "did a +real escalation just happen" signal, with no false negatives from the sticky ceiling and no +counter to maintain. This is also the same idiom the pre-existing test suite already used +(`stats is not last_stats`), so it isn't a new pattern for this codebase. + +--- + +## 2. The `metadata.source` stability constraint (spec §2.2) + +`tests/test_sticky_compaction_and_tail_notice.py`'s `_notices()` helper filters solely on +`(m.get("metadata") or {}).get("source") == "context-compaction"`. Giving the standing notice a +different `metadata["source"]` would make that helper return `[]` for it, and +`test_notice_returns_once_tool_results_arrive`'s `len(notices) == 1` assertion on a non-escalation +call would fail. More generally, any downstream consumer filtering the message stream on that +literal value would silently stop recognizing standing notices as compaction notices at all. + +**Both notice kinds keep `metadata["source"] = "context-compaction"`, unconditionally.** The +fresh/standing distinction is carried two ways instead: + +- a new `metadata["notice_kind"]` key (`"full"` or `"standing"`) — for telemetry / test assertions, + never used as the model-facing signal; +- the `source=` attribute **inside** the XML-ish notice text itself + (`source="context-compaction"` for the full notice, `source="context-compaction-standing"` for + the standing one) — this is what the model actually reads, since it never sees `metadata`. + +Verified: the four new tests assert `metadata["source"] == "context-compaction"` on *both* kinds, +and separately assert the differing `notice_kind` value and differing in-text `source=` attribute. +The full suite (`uv run pytest -q`) is green including the pre-existing +`test_notice_returns_once_tool_results_arrive`, unmodified. + +--- + +## 3. Edge case 7 — subclass override contract + +`_format_compaction_notice` is documented (and used in the wild) as a method subclasses may +override to customize the full notice's wording/format. This fix adds a sibling method, +`_format_standing_compaction_notice`, rather than folding standing-vs-full logic into +`_format_compaction_notice` itself. + +**Consequence worth flagging, not fixed here:** a subclass that overrides only +`_format_compaction_notice` (the common case — customizing the full report) will silently inherit +the **base class's** standing-notice text for every repeat announcement, without the subclass +author necessarily realizing a second method now exists. This is a reasonable default (the +standing notice is generic and self-contained by design, §2.3 of the spec — it doesn't reference +anything from the full notice's format), but it is a contract change for existing subclasses: prior +to this fix there was only one method to override to control 100% of notice output; after this fix +there are two, and overriding only one no longer covers the whole surface. No existing subclass in +this repo's own test suite exercises this path, so nothing here breaks today — recorded so the next +person maintaining a subclass of `SimpleContextManager` knows to check both methods. + +--- + +## 4. Other notes (not requested above, recorded for completeness) + +- **No new reset site was needed beyond the two enumerated in the spec.** `_notice_shown_for_stats` + is reset alongside `_last_compaction_stats` in both `set_messages` (session restore) and `clear`, + so a resumed or cleared session re-announces its next compaction as full, from scratch. +- **The `tool_calls` skip guard composes correctly with the new field.** + `_notice_shown_for_stats` is only updated *after* a notice is actually appended, so a full notice + skipped by the unanswered-`tool_calls` guard is retried as full (not silently downgraded to + standing) on the very next request. Covered by + `test_standing_notice_respects_notice_config_gates`'s sibling assertion that a + `min_level`-suppressed notice also leaves `_notice_shown_for_stats` at `None`. +- **Out of scope, per the spec:** this does not stop the notice from firing on every request + forever — it makes every repeat cheap (short) and unambiguous (self-contained, distinctly + tagged). Eliminating the notice firing altogether once acknowledged is a different, deliberately + unaddressed change. + +## 5. Verification + +- `uv run pytest -q` — full suite green (101 passed, including the 4 new tests added to + `tests/test_sticky_compaction_and_tail_notice.py`). +- `uv run ruff check .` — clean. +- `uv run ruff format --check` on the two touched files — the notice-kind dict literal was + reformatted to match; one pre-existing, unrelated formatting diff in `__init__.py` (a + `_should_compact` boolean expression untouched by this change, confirmed via `git stash`) was + left alone, out of scope for this fix. +- `uv run pyright` on the two touched files — 0 errors, 0 warnings. diff --git a/tests/test_sticky_compaction_and_tail_notice.py b/tests/test_sticky_compaction_and_tail_notice.py index 8cdabf7..3396e3b 100644 --- a/tests/test_sticky_compaction_and_tail_notice.py +++ b/tests/test_sticky_compaction_and_tail_notice.py @@ -827,6 +827,143 @@ async def test_notice_returns_once_tool_results_arrive(): ) +# --------------------------------------------------------------------------- +# (d.1) #513 -- fresh vs. standing compaction notice +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_first_notice_after_new_escalation_is_full(): + """#513: the notice for a compaction the model has not been told about yet + must be the FULL report -- the standing/short form must never pre-empt the + first delivery of real compaction news.""" + context = _make_context() + await _fill_until_compacted(context) + + view = await context.get_messages_for_request() + + assert context._last_compaction_stats is not None, "compaction should have fired" + notices = _notices(view) + assert len(notices) == 1 + notice = notices[0] + assert view[-1] is notice, "notice must still be at the tail" + assert notice["metadata"]["notice_kind"] == "full" + assert notice["metadata"]["source"] == "context-compaction" + assert notice["metadata"]["ephemeral"] is True + assert notice["role"] == "user" + content = notice["content"] + assert 'source="context-compaction"' in content + assert "context-compaction-standing" not in content + assert "Compaction summary:" in content + + +@pytest.mark.asyncio +async def test_repeat_notice_with_no_new_escalation_is_standing(): + """#513 core regression: with nothing new compacted, the tail notice must + degrade to the short standing form instead of replaying the full incident + report -- which previously repeated verbatim for the rest of the session.""" + context = _make_context(compact_threshold=0.92, target_usage=0.5) + await _fill_until_compacted(context) + + first = await context.get_messages_for_request() + stats_after_first = context._last_compaction_stats + assert stats_after_first is not None + first_notices = _notices(first) + assert len(first_notices) == 1 + assert first_notices[0]["metadata"]["notice_kind"] == "full" + full_text = first_notices[0]["content"] + + await context.add_message(_padded(900, "user")) + await context.add_message(_padded(900, "assistant")) + second = await context.get_messages_for_request() + + assert context._last_compaction_stats is stats_after_first, ( + "test precondition: the second call must NOT be a new escalation" + ) + + second_notices = _notices(second) + assert len(second_notices) == 1, ( + "the standing notice must still be discoverable via metadata.source == " + "'context-compaction' -- downstream consumers filter on that value" + ) + notice = second_notices[0] + assert second[-1] is notice + assert notice["role"] == "user" + assert notice["metadata"]["ephemeral"] is True + assert notice["metadata"]["notice_kind"] == "standing" + + standing_text = notice["content"] + assert standing_text != full_text, "this is the bug: stale full notice replayed" + assert 'source="context-compaction-standing"' in standing_text + assert "Compaction summary:" not in standing_text + assert len(standing_text) < len(full_text) + assert "system-reminder" in standing_text + assert "compact" in standing_text.lower() + + +@pytest.mark.asyncio +async def test_new_escalation_emits_full_notice_again(): + """#513: the standing form must never latch. Every genuinely new + escalation -- including one at the SAME strategy_level, which is the + steady state of any long session -- gets a fresh full report.""" + context = _make_context() + await _fill_until_compacted(context) + + observations: list[tuple[int, bool, str]] = [] + full_texts: list[str] = [] + last_stats = None + + for i in range(12): + view = await context.get_messages_for_request() + stats = context._last_compaction_stats + is_new = stats is not last_stats + last_stats = stats + + notices = _notices(view) + assert len(notices) == 1, f"call {i}: expected exactly one tail notice" + kind = notices[0]["metadata"]["notice_kind"] + observations.append((i, is_new, kind)) + if kind == "full": + full_texts.append(notices[0]["content"]) + + await context.add_message(_padded(4000 + i, "user")) + await context.add_message(_padded(4000 + i, "assistant")) + + escalations = [i for i, is_new, _ in observations if is_new] + assert len(escalations) >= 2, ( + f"test precondition: needed >=2 escalations across 12 calls, got {escalations}" + ) + + for i, is_new, kind in observations: + assert kind == ("full" if is_new else "standing"), ( + f"call {i}: new_escalation={is_new} but notice_kind={kind!r}" + ) + + assert len(full_texts) >= 2 + assert full_texts[0] != full_texts[-1], ( + "a later escalation must report its own numbers, not replay the first" + ) + assert sum(1 for _, _, k in observations if k == "full") < len(observations) + + +@pytest.mark.asyncio +async def test_standing_notice_respects_notice_config_gates(): + """Both notice variants sit inside the same enabled/min_level gates.""" + disabled = _make_context(compaction_notice_enabled=False) + await _fill_until_compacted(disabled) + for _ in range(3): + assert _notices(await disabled.get_messages_for_request()) == [] + + gated = _make_context(compaction_notice_min_level=9) # sticky level tops out at 8 + await _fill_until_compacted(gated) + for _ in range(3): + assert _notices(await gated.get_messages_for_request()) == [] + assert gated._last_compaction_stats is not None, "compaction still ran" + assert gated._notice_shown_for_stats is None, ( + "a suppressed notice must not be recorded as delivered" + ) + + # --------------------------------------------------------------------------- # (e) `_seq` is internal bookkeeping and must not cross the module boundary # ---------------------------------------------------------------------------