diff --git a/amplifier_module_context_simple/__init__.py b/amplifier_module_context_simple/__init__.py index f5f6396..7c36e79 100644 --- a/amplifier_module_context_simple/__init__.py +++ b/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 diff --git a/docs/lanes/l4s1-context-simple-break5/DONE-NOTE.md b/docs/lanes/l4s1-context-simple-break5/DONE-NOTE.md new file mode 100644 index 0000000..e733fff --- /dev/null +++ b/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 | diff --git a/docs/lanes/l4s1-context-simple-break5/evidence/BREAK5-PATCH-v5co.md b/docs/lanes/l4s1-context-simple-break5/evidence/BREAK5-PATCH-v5co.md new file mode 100644 index 0000000..cb43b78 --- /dev/null +++ b/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 /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 < /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 | diff --git a/docs/lanes/l4s1-context-simple-break5/evidence/break5-ported-at-dd9b9c3.patch b/docs/lanes/l4s1-context-simple-break5/evidence/break5-ported-at-dd9b9c3.patch new file mode 100644 index 0000000..a82d1e9 --- /dev/null +++ b/docs/lanes/l4s1-context-simple-break5/evidence/break5-ported-at-dd9b9c3.patch @@ -0,0 +1,69 @@ +diff --git a/amplifier_module_context_simple/__init__.py b/amplifier_module_context_simple/__init__.py +index f5f6396..7c36e79 100644 +--- a/amplifier_module_context_simple/__init__.py ++++ b/amplifier_module_context_simple/__init__.py +@@ -56,6 +56,42 @@ TOKEN_METER_ESTIMATE = "estimate" + 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 @@ class SimpleContextManager: + 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 diff --git a/docs/lanes/l4s1-context-simple-break5/evidence/break5-v5co-original.patch b/docs/lanes/l4s1-context-simple-break5/evidence/break5-v5co-original.patch new file mode 100644 index 0000000..248c595 --- /dev/null +++ b/docs/lanes/l4s1-context-simple-break5/evidence/break5-v5co-original.patch @@ -0,0 +1,67 @@ +--- a/amplifier_module_context_simple/__init__.py ++++ b/amplifier_module_context_simple/__init__.py +@@ -29,6 +29,42 @@ + + logger = logging.getLogger(__name__) + ++# 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): + """ +@@ -1166,6 +1202,21 @@ + 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 diff --git a/docs/lanes/l4s1-context-simple-break5/evidence/ci-green-run.json b/docs/lanes/l4s1-context-simple-break5/evidence/ci-green-run.json new file mode 100644 index 0000000..49e5c97 --- /dev/null +++ b/docs/lanes/l4s1-context-simple-break5/evidence/ci-green-run.json @@ -0,0 +1 @@ +{"jobs":[{"completedAt":"2026-09-07T04:05:46Z","conclusion":"success","databaseId":101618406049,"name":"ruff check","startedAt":"2026-09-07T04:05:37Z","status":"completed","steps":[{"conclusion":"success","name":"Set up job","number":1,"status":"completed"},{"conclusion":"success","name":"Run actions/checkout@v4","number":2,"status":"completed"},{"conclusion":"success","name":"Install uv","number":3,"status":"completed"},{"conclusion":"success","name":"ruff check","number":4,"status":"completed"},{"conclusion":"success","name":"Post Install uv","number":7,"status":"completed"},{"conclusion":"success","name":"Post Run actions/checkout@v4","number":8,"status":"completed"},{"conclusion":"success","name":"Complete job","number":9,"status":"completed"}],"url":"https://github.com/microsoft/amplifier-module-context-simple/actions/runs/34081821041/job/101618406049"},{"completedAt":"2026-09-07T04:05:40Z","conclusion":"success","databaseId":101618406229,"name":"pytest (py3.11)","startedAt":"2026-09-07T04:05:25Z","status":"completed","steps":[{"conclusion":"success","name":"Set up job","number":1,"status":"completed"},{"conclusion":"success","name":"Run actions/checkout@v4","number":2,"status":"completed"},{"conclusion":"success","name":"Install uv","number":3,"status":"completed"},{"conclusion":"success","name":"Install dependencies","number":4,"status":"completed"},{"conclusion":"success","name":"Run test suite","number":5,"status":"completed"},{"conclusion":"success","name":"Post Install uv","number":9,"status":"completed"},{"conclusion":"success","name":"Post Run actions/checkout@v4","number":10,"status":"completed"},{"conclusion":"success","name":"Complete job","number":11,"status":"completed"}],"url":"https://github.com/microsoft/amplifier-module-context-simple/actions/runs/34081821041/job/101618406229"},{"completedAt":"2026-09-07T04:05:45Z","conclusion":"success","databaseId":101618406238,"name":"pytest (py3.12)","startedAt":"2026-09-07T04:05:26Z","status":"completed","steps":[{"conclusion":"success","name":"Set up job","number":1,"status":"completed"},{"conclusion":"success","name":"Run actions/checkout@v4","number":2,"status":"completed"},{"conclusion":"success","name":"Install uv","number":3,"status":"completed"},{"conclusion":"success","name":"Install dependencies","number":4,"status":"completed"},{"conclusion":"success","name":"Run test suite","number":5,"status":"completed"},{"conclusion":"success","name":"Post Install uv","number":9,"status":"completed"},{"conclusion":"success","name":"Post Run actions/checkout@v4","number":10,"status":"completed"},{"conclusion":"success","name":"Complete job","number":11,"status":"completed"}],"url":"https://github.com/microsoft/amplifier-module-context-simple/actions/runs/34081821041/job/101618406238"}]} diff --git a/docs/lanes/l4s1-context-simple-break5/evidence/fail-before.txt b/docs/lanes/l4s1-context-simple-break5/evidence/fail-before.txt new file mode 100644 index 0000000..4547f16 --- /dev/null +++ b/docs/lanes/l4s1-context-simple-break5/evidence/fail-before.txt @@ -0,0 +1,24 @@ +HEAD=dd9b9c3f042fc70588f0afbd6a81f4ac20337ee7 +F.. [100%] +=================================== FAILURES =================================== +__________________ test_loaded_tool_state_survives_compaction __________________ + + def test_loaded_tool_state_survives_compaction(): + mgr = _manager() + out = _build(mgr) + carriers = _carriers(out) +> assert len(carriers) == 1, ( + "the message recording which tools were dynamically loaded was dropped " + "by compaction -- per TS:854 those tools now cease to exist for the " + "model AND the prompt cache breaks forward, silently (break 5)." + ) +E AssertionError: the message recording which tools were dynamically loaded was dropped by compaction -- per TS:854 those tools now cease to exist for the model AND the prompt cache breaks forward, silently (break 5). +E assert 0 == 1 +E + where 0 = len([]) + +tests/test_loaded_tool_state_protection.py:90: AssertionError +=========================== short test summary info ============================ +FAILED tests/test_loaded_tool_state_protection.py::test_loaded_tool_state_survives_compaction +1 failed, 2 passed in 0.03s +............................... [100%] +102 passed, 1 xfailed in 4.93s diff --git a/docs/lanes/l4s1-context-simple-break5/evidence/pass-after.txt b/docs/lanes/l4s1-context-simple-break5/evidence/pass-after.txt new file mode 100644 index 0000000..e31110a --- /dev/null +++ b/docs/lanes/l4s1-context-simple-break5/evidence/pass-after.txt @@ -0,0 +1,6 @@ +HEAD=dd9b9c3f042fc70588f0afbd6a81f4ac20337ee7 + ported patch +... [100%] +3 passed in 0.02s +.................................. [100%] +105 passed, 1 xfailed in 4.96s +All checks passed! diff --git a/tests/test_loaded_tool_state_protection.py b/tests/test_loaded_tool_state_protection.py new file mode 100644 index 0000000..ba93b0c --- /dev/null +++ b/tests/test_loaded_tool_state_protection.py @@ -0,0 +1,112 @@ +"""BREAK 5 -- a message carrying loaded-tool state must survive compaction. + +FAIL-BEFORE / PASS-AFTER guard for the `context-simple` half of +`model_performance-v5co`. Belongs at `tests/test_loaded_tool_state_protection.py` +in `amplifier-module-context-simple`; it ships here because this lane's worktree +does not contain that repo (see BREAK5-PATCH.md). + +`TS:854` makes dropping a `tool_search_output` record cost twice: every tool it +loaded silently ceases to exist for the model, AND the prompt cache breaks +forward from that point. OpenAI's cache is grow-only (`00-what-we-know.md` §2a), +so "breaks forward" means a full cold rebuild, not a dip. + +The ladder had no concept of an un-droppable item, so at the unpatched tree the +message is removed like any other and the failure is silent. +""" + +from __future__ import annotations + +import asyncio +from typing import Any + +from amplifier_module_context_simple import SimpleContextManager + +TOOL_SEARCH_ITEMS_KEY = "openai:tool_search_items" + +HOSTED_ITEMS = [ + { + "type": "tool_search_call", + "execution": "server", + "call_id": None, + "arguments": {"paths": ["files"]}, + }, + { + "type": "tool_search_output", + "execution": "server", + "call_id": None, + "tools": [{"type": "function", "name": "glob", "namespace": "files"}], + }, +] + +FILLER = "x" * 4000 + + +def _manager() -> SimpleContextManager: + return SimpleContextManager( + max_tokens=2_000, + compact_threshold=0.5, + protected_recent=0.10, + protected_tool_results=1, + ) + + +async def _build_async(mgr: SimpleContextManager) -> list[dict[str, Any]]: + """A long conversation whose SECOND turn carries the loaded-tool state. + + Deliberately early in the history and outside the protected tail, so an + unpatched ladder removes it. `add_message` is a coroutine -- awaiting it is + what makes this test non-vacuous. + """ + await mgr.add_message({"role": "user", "content": "start " + FILLER}) + await mgr.add_message( + { + "role": "assistant", + "content": "searching for file tools " + FILLER, + "metadata": {TOOL_SEARCH_ITEMS_KEY: HOSTED_ITEMS}, + } + ) + for n in range(12): + await mgr.add_message({"role": "assistant", "content": f"step {n} " + FILLER}) + await mgr.add_message({"role": "user", "content": f"go on {n} " + FILLER}) + # get_messages() is the FULL history and is never compacted; the request + # path is what runs the ladder. + return await mgr.get_messages_for_request(token_budget=2_000) + + +def _build(mgr: SimpleContextManager) -> list[dict[str, Any]]: + out = asyncio.run(_build_async(mgr)) + assert len(mgr.messages) == 26, "fixture did not land; nothing to compact" + return out + + +def _carriers(messages: list[dict[str, Any]]) -> list[dict[str, Any]]: + return [m for m in messages if (m.get("metadata") or {}).get(TOOL_SEARCH_ITEMS_KEY)] + + +def test_loaded_tool_state_survives_compaction(): + mgr = _manager() + out = _build(mgr) + carriers = _carriers(out) + assert len(carriers) == 1, ( + "the message recording which tools were dynamically loaded was dropped " + "by compaction -- per TS:854 those tools now cease to exist for the " + "model AND the prompt cache breaks forward, silently (break 5)." + ) + assert carriers[0]["metadata"][TOOL_SEARCH_ITEMS_KEY] == HOSTED_ITEMS + + +def test_compaction_still_happened(): + """Guard against a vacuous pass: worthless if nothing was compacted.""" + mgr = _manager() + out = _build(mgr) + assert len(out) < 26, "nothing was compacted; the protection was never exercised" + + +def test_ordinary_messages_are_still_removable(): + """The protection must be narrow -- it is not a blanket do-not-compact.""" + mgr = _manager() + out = _build(mgr) + contents = " ".join( + str(m.get("content") or "") for m in out if m.get("role") == "assistant" + ) + assert "step 0" not in contents or "step 1" not in contents