fix: compaction must not drop messages carrying loaded-tool state (break 5) — ported to today's head, re-measured

amplifier_module_context_simple/__init__.py

@@ -56,6 +56,42 @@
 TOKEN_METER_ACTUAL = "actual"
 _VALID_TOKEN_METERS = (TOKEN_METER_ESTIMATE, TOKEN_METER_ACTUAL)
 
+# BREAK 5 -- messages carrying LOADED-TOOL STATE are un-droppable.
+#
+# A provider may record, on a message's metadata, which tools the model
+# dynamically loaded during that turn (OpenAI's hosted tool search does this via
+# `openai:tool_search_items`). That record is not decoration -- per the vendored
+# tool-search guide `TS:854`:
+#
+#   "Tools that were not listed as part of this array will not be available to
+#    the model ... changing the loaded tool set will break the model's cache
+#    from that point forward."
+#
+# So dropping such a message during compaction costs TWICE: (a) every tool it
+# loaded silently ceases to exist for the model, which it discovers by failing
+# to call one, and (b) the prompt cache breaks forward from that point -- and
+# OpenAI's cache is grow-only, so a break is a full cold rebuild, not a dip.
+#
+# The ladder has no other concept of an un-droppable item, so this is a named
+# protected set rather than a role check. It is deliberately keyed on METADATA,
+# not on a provider name: a second provider adding the same kind of state adds
+# a key here and needs no other change. Truncation and stubbing already rebuild
+# messages with `{**msg, ...}`, so they preserve metadata and need no change --
+# only REMOVAL had to learn this.
+LOADED_TOOL_STATE_METADATA_KEYS: frozenset[str] = frozenset(
+    {
+        "openai:tool_search_items",
+    }
+)
+
+
+def _carries_loaded_tool_state(msg: dict[str, Any]) -> bool:
+    """True if removing this message would silently unload tools (TS:854)."""
+    meta = msg.get("metadata") or {}
+    if not isinstance(meta, dict):
+        return False
+    return any(meta.get(key) for key in LOADED_TOOL_STATE_METADATA_KEYS)
+
 
 async def mount(coordinator: ModuleCoordinator, config: dict[str, Any] | None = None):
     """
@@ -1420,6 +1456,21 @@ def _remove_messages_with_protection(
             if msg.get("role") == "system":
                 protected_indices.add(i)
 
+        # BREAK 5 -- never remove a message carrying loaded-tool state.
+        # Protecting it here also protects it from the tool-pair cascade below,
+        # which consults this same set. See LOADED_TOOL_STATE_METADATA_KEYS.
+        loaded_tool_state_indices = {
+            i for i, msg in enumerate(messages) if _carries_loaded_tool_state(msg)
+        }
+        if loaded_tool_state_indices:
+            protected_indices |= loaded_tool_state_indices
+            logger.debug(
+                "[CONTEXT] protecting %d message(s) carrying loaded-tool state "
+                "from removal (TS:854: dropping them unloads tools and breaks "
+                "the prompt cache forward)",
+                len(loaded_tool_state_indices),
+            )
+
         # First user message is stubbable at extreme pressure (Level 8), but never fully removed
         # (It's excluded from removal_candidates via user_message_indices, but can be stubbed)
         # We don't add it to protected_indices so it can be stubbed at Level 8

docs/lanes/l4s1-context-simple-break5/DONE-NOTE.md

@@ -0,0 +1,180 @@
+# l4s1 — Land context-simple break 5 (homeless across three lanes)
+
+**Item:** `model_performance-l4s1` · **Outcome branch: A (RESOLVED)** · **Spend: $0.00 of $0.00**
+
+Break 5 — *compaction must not drop the message carrying loaded-tool state* — is
+**applied, re-measured at today's head, and shipped as a draft PR.** Every deliverable
+is DONE. Nothing was NOT-POSSIBLE; the $0 authority was correct and sufficient
+(the whole lane is `git`, `pytest` and `ruff` — no API calls, no DTU, no containers).
+
+**The one thing that is NOT as previously reported: the patch did not apply cleanly.**
+It applied *with fuzz* at today's head, so per the goal it was **ported by hand, not
+forced**. The divergence is named in §3.
+
+---
+
+## 1. Deliverables
+
+| # | Deliverable | State |
+|---|---|---|
+| 1 | Patch applied to current `context-simple` main, sourced from provider-openai main | **DONE** — ported (§3), not force-applied |
+| 2 | FAIL-BEFORE re-run at today's head, counts from **this** run | **DONE** — §2, head `dd9b9c3` |
+| 3 | Full suite + ruff clean | **DONE** — §2 |
+| 4 | Explicit statement: does G13 become measurable? | **DONE** — §4 |
+| 5 | DRAFT PR, not merged | **DONE** — see `DONE.json` `publication` block |
+| 6 | DONE-NOTE.md at lane artifact root (never repo root) | **DONE** — this file |
+
+---
+
+## 2. The measurement, at today's head
+
+**Head measured: `dd9b9c3f042fc70588f0afbd6a81f4ac20337ee7`** (`ci: add GitHub Actions
+CI …`, PR #36). `v5co` measured against **`a2a098b`**; the baseline moved by 4 merged
+PRs (#30, #32, #34, #36) touching `__init__.py` by 290 lines.
+
+| step | `v5co` @ `a2a098b` | **this lane @ `dd9b9c3`** |
+|---|---|---|
+| patch applies | clean, no fuzz | **NOT clean — hunk 1 fuzz 2** → ported (§3) |
+| FAIL-BEFORE (test, unpatched) | 1 failed, 2 passed | **1 failed, 2 passed** — identical |
+| failure mode | `len([]) == 0`, carrier removed | **`assert 0 == 1`, `where 0 = len([])`** — identical |
+| PASS-AFTER (test, patched) | 3 passed | **3 passed** |
+| full suite, unpatched | 58 passed | **102 passed, 1 xfailed** |
+| full suite, patched | 61 passed | **105 passed, 1 xfailed** |
+| regressions | 0 | **0** |
+| ruff | clean | **`uvx ruff@0.14.10 check .` → All checks passed** |
+
+**The fail-before counts are unchanged from `v5co`'s (1 failed / 2 passed → 3 passed)
+and so is the failure message.** The *suite* counts differ only because the suite grew
+from 58 to 102 tests between the two heads; the delta is the same **+3**.
+
+Raw logs: `evidence/fail-before.txt`, `evidence/pass-after.txt`.
+
+**CI is green, and this is the first of the break-5 attempts where that means
+something.** `context-simple` got its first workflow in this program (PR #36, `dd9b9c3`),
+proven red-then-green before merging. Run
+[`34081821041`](https://github.com/microsoft/amplifier-module-context-simple/actions/runs/34081821041)
+on commit `1a83f17`: **`ruff check` success · `pytest (py3.11)` success ·
+`pytest (py3.12)` success**. Captured at `evidence/ci-green-run.json`. (That run predates
+this paragraph by one commit; the run for the final head is recorded in the lane's
+`DONE.json`, so neither citation chases its own tail.)
+
+**Byte-identity of default mode.** The protection fires only when a message's
+`metadata` carries a key in `LOADED_TOOL_STATE_METADATA_KEYS`. With no such key
+present, `loaded_tool_state_indices` is empty, `protected_indices` is untouched and no
+branch changes — which is why all 102 pre-existing tests pass byte-for-byte unchanged
+(stash-compare in `evidence/`: the same 102 passed / 1 xfailed on both sides).
+
+**`ruff format --check` reports 5 files would be reformatted — this is pre-existing on
+clean main and is not caused by this change.** The CI workflow itself documents this at
+`.github/workflows/*.yml:19–29`: it deliberately runs `ruff check` only, because no repo
+in this module family carries a `[tool.ruff]` section, so `format --check` would enforce
+ruff's default line-length-88 and be red on clean main on day one. The 5 files it names
+are exactly the 5 seen here, including `__init__.py` **before** this patch.
+
+---
+
+## 3. What diverged, and why the patch was ported rather than forced
+
+`patch -p1 --dry-run` at `dd9b9c3`:
+
+```
+Hunk #1 succeeded at 56 with fuzz 2 (offset 27 lines).
+Hunk #2 succeeded at 1456 (offset 254 lines).
+```
+
+- **Hunk 2 — clean.** Offset 254 is line drift only; zero fuzz, context matched exactly.
+  Applied verbatim into `_remove_messages_with_protection`, immediately after the
+  system-message protection loop, exactly as `v5co` wrote it.
+- **Hunk 1 — NOT clean, fuzz 2.** `v5co` anchored the new module-level block on
+  `logger = logging.getLogger(__name__)` followed directly by a blank line and
+  `async def mount(`. Since `a2a098b`, PR #32/#34's **token-meter config block**
+  (`TOKEN_METER_ESTIMATE` / `TOKEN_METER_ACTUAL` / `_VALID_TOKEN_METERS`, with its
+  five-line comment) was inserted **between** those two anchors. `patch` resolved the
+  mismatch by discarding 2 context lines — a silent placement decision.
+
+  **Ported instead:** the block was placed by hand **after** the token-meter constants
+  and immediately before `async def mount(`, which keeps all module-level constants
+  together and leaves the token-meter block undisturbed. Semantically identical to
+  `v5co`'s intent; the only difference is where in the constants region it sits.
+
+Net diff: **+51 lines, one file** (`amplifier_module_context_simple/__init__.py`),
+plus the 112-line test at `tests/test_loaded_tool_state_protection.py`.
+
+Both patches are kept side by side for audit:
+`evidence/break5-v5co-original.patch` (as shipped from provider-openai main) and
+`evidence/break5-ported-at-dd9b9c3.patch` (what actually landed here).
+
+---
+
+## 4. Does G13 (compaction survival) become measurable once this lands?
+
+**Yes — with this merged, G13 is measurable, and no other code prerequisite remains.
+What remains is spend and a scoped caveat, not a blocker.**
+
+The chain is now complete and was **verified across both repos in this lane, not
+assumed**:
+
+1. **Producer.** `amplifier-module-provider-openai` @ `702b361` (main) defines
+   `METADATA_TOOL_SEARCH_ITEMS = "openai:tool_search_items"` (`_constants.py:19`) and
+   writes it onto response metadata in two paths
+   (`__init__.py:4752`, `_response_handling.py:590`).
+2. **Consumer.** The same provider replays it **off each message's own `metadata`**
+   while rebuilding the request (`__init__.py:3975`,
+   `for _ts_item in metadata.get(METADATA_TOOL_SEARCH_ITEMS) or []`) — so the state
+   genuinely lives on messages in the context manager's list.
+3. **Protector.** This PR. The key string in
+   `LOADED_TOOL_STATE_METADATA_KEYS` is **byte-identical** to the provider's constant,
+   so the protection actually fires on real traffic and not only in the unit fixture.
+   That equality was checked, because a near-miss here would have protected nothing
+   while looking green.
+
+**Two caveats a future eval must carry, both stated rather than discovered later:**
+
+- **Retention is fixed; *ordering* is out of scope.** `TS:893`'s positional contract for
+  an `additional_tools` input item is untouched, because this provider never persists
+  that item into history — it rebuilds it at the input tail every request from
+  session-scoped provider state. If a future implementation persists it into the message
+  list, break 5 acquires a second half (ordering, not just retention) that this patch
+  does **not** cover. (Carried forward verbatim from `v5co`'s scope-honesty section and
+  re-confirmed against provider main.)
+- **G13 still costs money to *run*.** This removes the code blocker. Actually grading
+  compaction survival needs live OpenAI traffic with `tool_search.mode` on and enough
+  context to force a boundary. That is a spend authority for a future lane, not a
+  missing piece here.
+
+**Consequence for `webu`'s record:** `webu` recorded G13 **UNINFORMATIVE with the
+dependency named**. That call was correct at the time. Once this PR merges, the reason
+it gave no longer holds, and a re-run of G13 should be expected to be *informative* —
+pass or fail on its merits.
+
+---
+
+## 5. Spend
+
+**$0.00 spent against a $0.00 authority** (`0 runs × 0 arms × $0 / 1.00 = $0.00`,
+slack $0.00). No API calls, no DTU, no containers, no infrastructure registered and
+therefore none to tear down. The arithmetic closed on first read: applying an existing
+patch and running an existing suite costs nothing, and nothing in the deliverable list
+required a purchase. Total wall time ≈ 6 minutes, all local.
+
+---
+
+## 6. Deviations from the goal
+
+None substantive. One judgment call, recorded per procedure:
+
+- **The patch was ported, not applied.** The goal explicitly forbids force-applying and
+  requires the divergence be explained; §3 is that explanation. `patch` would have
+  succeeded with fuzz, which is exactly the silent-placement case the instruction
+  exists to prevent.
+
+## 7. Files
+
+| path | what |
+|---|---|
+| `DONE-NOTE.md` | this file |
+| `evidence/fail-before.txt` | FAIL-BEFORE + unpatched full suite at `dd9b9c3` |
+| `evidence/pass-after.txt` | PASS-AFTER + patched full suite + `ruff check` |
+| `evidence/break5-v5co-original.patch` | the artifact as shipped on provider-openai main |
+| `evidence/break5-ported-at-dd9b9c3.patch` | what actually landed here (+51 lines) |
+| `evidence/BREAK5-PATCH-v5co.md` | `v5co`'s original write-up, kept for provenance |

docs/lanes/l4s1-context-simple-break5/evidence/BREAK5-PATCH-v5co.md

@@ -0,0 +1,148 @@
+# Break 5 — `context-simple` compaction must not drop loaded-tool state
+
+**Status: DONE as a verified patch artifact, NOT as a commit in that repo — because
+this lane cannot reach that repo. That is a GOAL DEFECT, reported below, not a gap
+this lane absorbed.**
+
+---
+
+## 1. The goal defect, stated plainly
+
+`GOAL.md` requires, as a deliverable:
+
+> **`context-simple` break 5 fixed** (G13 compaction survival).
+
+and simultaneously constrains:
+
+> Do not merge anything to main; **do not edit files outside the paths this lane
+> owns**. … **Never touch other repos.**
+
+This lane's worktree contains **exactly one repo**:
+
+```
+/home/bkrabach/dev/hw-model-performance/lanes/v5co-tool-search-provider-build/
+└── amplifier-module-provider-openai        <- the only checkout
+```
+
+`amplifier-module-context-simple` is not in it and has no lane branch. So the only
+way to satisfy that deliverable as literally written is to write into a repo the
+goal forbids touching. `GOAL.md` anticipates precisely this and names the remedy:
+
+> **AND: every option this goal offers you must have at least one target inside the
+> paths it says you own.** If the only way to satisfy a deliverable is to write a
+> file outside your worktree (another repo, or the manager's unversioned `goals/`
+> directory), that is a **DEFECT IN THIS GOAL**, not a task. Report it against the
+> goal, ship the patch as an artifact under your ARTIFACT ROOT, and resolve — do not
+> edit another repo, and do not invent a fourth outcome branch.
+
+That is what this directory is. **The remedy for the manager is one line: give the
+next lane a `context-simple` worktree, or apply this patch directly** (§4).
+
+Note this is a *packaging* defect, not a *scoping* one: the work item is right that
+break 5 belongs with breaks 1/3/6 — they are one feature. Only the lane's checkout
+was sized for one repo.
+
+---
+
+## 2. What break 5 actually is
+
+`DESIGN.md` w3 §4, break 5 (severity **high**):
+
+> Compaction removes input items. Remove a `tool_search_output` / `additional_tools`
+> item and (a) those tools cease to exist (`TS:854`) and (b) the cache breaks
+> forward. … `context-simple`'s ladder has no concept of an un-droppable,
+> position-pinned item.
+
+`TS:854`, normative:
+
+> "Tools that were not listed as part of this array will not be available to the
+> model … **changing the loaded tool set will break the model's cache from that
+> point forward.**"
+
+Two failures from one action, and **both are silent**. The model discovers (a) by
+calling a tool that no longer exists; nobody discovers (b) at all, except as cost —
+and OpenAI's cache is **grow-only** (`00-what-we-know.md` §2a: strict truncation of
+a cached prefix returns `0`, MISS), so "breaks forward" means a full cold rebuild,
+not a dip.
+
+**Where the state lives in this implementation.** The provider carries the hosted
+items on `Message.metadata["openai:tool_search_items"]` (the existing provider-state
+channel, same as encrypted reasoning) rather than as free-standing input items. So
+"drop the item" is, concretely, "drop the message that carries the metadata" — which
+is exactly what `_remove_messages_with_protection` does at ladder levels 3/5/7.
+
+---
+
+## 3. The fix
+
+`break5-loaded-tool-state-protection.patch` (67 lines) against
+`amplifier-module-context-simple` @ `a2a098b`.
+
+1. A named, provider-neutral protected set:
+   ```python
+   LOADED_TOOL_STATE_METADATA_KEYS: frozenset[str] = frozenset({"openai:tool_search_items"})
+   ```
+   Keyed on **metadata, not on a provider name** — a second provider adding the same
+   kind of state adds a key and needs no other change.
+2. `_remove_messages_with_protection` adds every carrying message's index to
+   `protected_indices`. Because the tool-pair cascade
+   (`_try_remove_tool_pair_from_result` / `_from_assistant`) already consults that
+   same set, one insertion protects the message from the cascade too.
+
+**Deliberately NOT changed:** `_truncate_tool_result` and `_stub_user_message`
+already rebuild with `{**msg, ...}`, so they preserve `metadata` — a truncated
+message keeps its loaded-tool record. Only **removal** had to learn this. Verified,
+not assumed (§4 shows the whole suite still green).
+
+**Scope honesty — what this does NOT do.** `TS:893`'s *positional* contract for an
+`additional_tools` input item is untouched, because this provider never emits that
+item into history: it is rebuilt at the input tail on every request from
+session-scoped provider state. If a future implementation persists it into the
+message list, break 5 acquires a second half (ordering, not just retention) that
+this patch does not cover. Said here rather than discovered later.
+
+---
+
+## 4. Verification — run it yourself, ~2 minutes, $0
+
+```bash
+git clone https://github.com/microsoft/amplifier-module-context-simple /tmp/cs && cd /tmp/cs
+git checkout a2a098b
+cp <this-dir>/test_loaded_tool_state_protection.py tests/
+
+# FAIL-BEFORE
+uv run pytest -q tests/test_loaded_tool_state_protection.py     # 1 failed, 2 passed
+
+patch -p1 < <this-dir>/break5-loaded-tool-state-protection.patch
+
+# PASS-AFTER
+uv run pytest -q tests/test_loaded_tool_state_protection.py     # 3 passed
+uv run pytest -q -m "not live"                                  # 61 passed
+```
+
+Measured in this lane on 2026-09-06, against `a2a098b`:
+
+| step | result |
+|---|---|
+| patch applies | clean, `patch -p1`, no fuzz |
+| FAIL-BEFORE (unpatched + test) | **1 failed, 2 passed** — `len([]) == 0`: the carrier was removed |
+| PASS-AFTER (patched + test) | **3 passed** |
+| full suite, unpatched | 58 passed |
+| full suite, patched | **61 passed** (58 + 3), 0 regressions |
+
+**The test is guarded against being vacuous**, because the first version of it
+*was*: `add_message` is a coroutine, and an un-awaited call left the manager with
+zero messages while two of three assertions still "passed". It now asserts
+`len(mgr.messages) == 26` after the fixture, asserts that compaction actually
+reduced the list, and asserts that ordinary messages are still removable — so the
+protection cannot pass by being a blanket do-not-compact.
+
+---
+
+## 5. Files here
+
+| file | what it is |
+|---|---|
+| `break5-loaded-tool-state-protection.patch` | the fix, `patch -p1` against `a2a098b` |
+| `test_loaded_tool_state_protection.py` | the FAIL-BEFORE/PASS-AFTER guard; belongs at `tests/` |
+| `BREAK5-PATCH.md` | this file |