From 638ea55648d7d00b7f9280099a5a13b5405ede08 Mon Sep 17 00:00:00 2001 From: lane/sqh Date: Wed, 2 Sep 2026 18:58:28 -0700 Subject: [PATCH 1/2] docs: recover 6 lane DONE-NOTEs from git history into docs/lanes// (#sqh) --- docs/lanes/2o9-clear-at-least/DONE-NOTE.md | 263 ++++++++++++++ docs/lanes/7k2-summary-call-fork/DONE-NOTE.md | 214 ++++++++++++ docs/lanes/README.md | 42 +++ .../jnt-fork-prefix-capture/DONE-NOTE.md | 171 +++++++++ .../rb1-rebase-conflicted-prs/DONE-NOTE.md | 193 +++++++++++ .../sqh-context-simple-note-loss/AUDIT.md | 225 ++++++++++++ .../RECOVERED-index-header.md | 17 + .../recover_root_done_notes.py | 208 +++++++++++ .../lanes/x1r-tool-result-budget/DONE-NOTE.md | 326 ++++++++++++++++++ .../DONE-NOTE.md | 1 + tests/test_done_note_placement.py | 150 ++++++++ tools/check_done_note_placement.py | 163 +++++++++ 12 files changed, 1973 insertions(+) create mode 100644 docs/lanes/2o9-clear-at-least/DONE-NOTE.md create mode 100644 docs/lanes/7k2-summary-call-fork/DONE-NOTE.md create mode 100644 docs/lanes/README.md create mode 100644 docs/lanes/jnt-fork-prefix-capture/DONE-NOTE.md create mode 100644 docs/lanes/rb1-rebase-conflicted-prs/DONE-NOTE.md create mode 100644 docs/lanes/sqh-context-simple-note-loss/AUDIT.md create mode 100644 docs/lanes/sqh-context-simple-note-loss/RECOVERED-index-header.md create mode 100644 docs/lanes/sqh-context-simple-note-loss/recover_root_done_notes.py create mode 100644 docs/lanes/x1r-tool-result-budget/DONE-NOTE.md rename DONE-NOTE.md => docs/lanes/x7p-protected-tool-results-bug/DONE-NOTE.md (99%) create mode 100644 tests/test_done_note_placement.py create mode 100644 tools/check_done_note_placement.py diff --git a/docs/lanes/2o9-clear-at-least/DONE-NOTE.md b/docs/lanes/2o9-clear-at-least/DONE-NOTE.md new file mode 100644 index 0000000..aa6c5f8 --- /dev/null +++ b/docs/lanes/2o9-clear-at-least/DONE-NOTE.md @@ -0,0 +1,263 @@ +# DONE-NOTE — W3-4 / `model_performance-2o9` + +`clear_at_least` — a worth-the-rebuild predicate in front of compaction +(context-simple), plus deepseek's summary shrink guard in the same PR. + +**Spend: $0.00.** No API calls, no eval runs, no DTU, no containers, no +infrastructure created or registered (and therefore none to tear down). +Everything below is local `pytest` / `python` on this host. The lane's spend +authority was $0 and none of it was used. + +--- + +## 1. Deliverables + +| deliverable | status | +|---|---| +| DRAFT PR on origin, branch `lane/2o9-clear-at-least`, default byte-identical, full suite green | **DONE** — [microsoft/amplifier-module-context-simple#26](https://github.com/microsoft/amplifier-module-context-simple/pull/26) (draft, rebased on `3972070`) | +| Tests for block / allow / default byte-identity / prefix stability | **DONE** — 41 new tests, 249 passed + 1 skipped total | +| Follow-up eval item filed with a Given/When/Then gate | **DONE** — `model_performance-wxs`, filed `discovered-from` this item | +| DONE-NOTE.md in the PR body | **DONE** — this note | + +--- + +## 2. The one design question the item asked me to answer honestly + +The spec (`POC-SPECS/04-clear-at-least-predicate.md`) assumes a plan/apply +split: + +``` +plan = build_compaction_plan(messages, target) +freed = tokens(messages) - tokens(apply(plan, messages)) +``` + +**That split does not exist in this module.** `_compact_ephemeral` has no plan +object: it mutates an ephemeral copy level by level (levels 1–8), threading a +running `current_tokens` through every rung, and returns through +`_finalize_compaction_with_stats` from eight different call sites. There is no +point at which a projection is available *before* the work is done. + +The GOAL says: *"if the projection is not available before the decision, say so +and implement the cheapest honest alternative, documenting the choice."* So, +stated plainly: + +**Chosen: judge the ladder's actual result, not a projection of it.** The +predicate runs at `_finalize_compaction_with_stats` — the single terminal choke +point every level returns through — and *before* that method writes any durable +effect (sticky level, stats, `context:compaction` event). If the reclaim is +below the floor, the escalation is rolled back and the baseline view is +returned. + +Three things make this the honest choice rather than a workaround: + +1. **It is exact, not estimated.** The ladder runs on an ephemeral copy that + never touches `self.messages` (`_truncate_tool_wave` does + `messages[i] = self._truncate_tool_result(msg)` — a *new* dict; + `_remove_messages_with_protection` returns a *new* list). So running it is a + simulation whose only durable output is three sticky-decision sets, which are + snapshot-and-restored. The number the predicate acts on is what the + compaction *did*, not what it might do. +2. **No second estimator was introduced.** Both sides of the comparison go + through the same `_estimate_tokens` the rest of the ladder runs on — which is + exactly what the item required ("use the existing projection the ladder + already computes; do not add a second estimator"). +3. **The cost of a refusal is CPU, not tokens.** A refused boundary costs one + wasted ladder pass over an in-memory list. The thing being avoided is a full + cold prompt-cache rebuild. The asymmetry is not close. + +**`freed` is the MARGINAL reclaim**, not the distance from raw history: + +``` +freed = tokens(view this call would have returned had it decided nothing new) + - tokens(view the ladder actually produced) +``` + +This is deliberate and load-bearing. What breaks the provider's cache is *this +view differing from the last one*, not its distance from raw history. Measured +against raw history, an already-compacted session would report a large stale +"freed" on every single call and the predicate would wave through boundaries +that free nothing — the exact failure it exists to prevent. +`test_freed_is_marginal_not_measured_against_raw_history` pins this. + +--- + +## 3. Decisions taken without waiting (per SCOPE-OUTS), and why + +| # | decision | reasoning | +|---|---|---| +| 1 | **Fail-loud = `raise RuntimeError`**, not log-and-compact-anyway | The POC's own Risks §1 says the gate is *"written so the failure mode fails the run rather than degrading quietly"*, and G-CAL-NOSKIPHANG requires the run to FAIL. A silent override would let an eval pass while the predicate had stopped applying — the one outcome the gate exists to catch. Local precedent (`_provenance_overrides`) points the other way, but that escape exists to avoid a *guaranteed* provider hard-failure; here the operator has a knob to move, and the error names it. Unreachable while the predicate is disabled. | +| 2 | **The summary swap is NOT gated by the predicate** | By the time `_swap_in_pending_summary` returns it has already appended to `self.messages` and consumed a `_seq` — irreversible. Rather than build a fragile rollback for `self.messages`, that path is governed by the shrink guard (below), which checks *before* mutating. The predicate governs the progressive ladder, which is where the boundary-refire cost actually lives. | +| 3 | **Fraction form added** (`0 < v < 1` = fraction of budget) | The GOAL says "at least N tokens (or a fraction of the window)"; the POC spec said `int` only. A hardcoded token floor silently means something different on a 200k window than a 45k one. `1.0` is read as **absolute (1 token)**, not "100% of budget" — a floor of one whole budget can never be met, so the fraction reading would turn a plausible config into a guaranteed fail-loud. | +| 4 | **`compact_max_consecutive_skips <= 0` clamps UP to 1** | Read literally, 0 means "tolerate unlimited refusals" — precisely the silent hang the cap exists to prevent. | +| 5 | **A malformed value disables the predicate with a warning** | Consistent with how this module already handles `token_meter` / `compaction_strategy` / `tool_result_shape`. Never take a session down on a config typo. | +| 6 | Config names `compact_clear_at_least` / `compact_max_consecutive_skips` | Namespaced with the existing `compact_threshold`; the spec's bare `max_consecutive_skips` would have read as unrelated to compaction. | +| 7 | **No knob for the shrink guard** | There is no defensible reason to want a summary that makes the context bigger. | +| 8 | Predicate state does **not** survive `clear()` / `set_messages()` | A skip streak accumulated against one message set says nothing about a different one; inheriting it would fail loud on the first refusal after a resume. | + +--- + +## 4. An interaction bug this lane introduced and fixed inside itself + +The branch was cut from `f47c894`, but `origin/main` had advanced two commits +(`49e2799` tool-result budget, `3972070` last-user replay). **Rebased onto +`origin/main` (3972070)**; three conflicts, all additive, resolved by keeping +both sides. + +The replay feature then created a real interaction: +`replay_last_user_on_compaction` fires once per compaction *boundary*, where a +boundary is `(_sticky_level, _summary_absorbed_count)`. **A refusal leaves both +unchanged by design** — the verdict runs before `_sticky_level` is bumped — so +on the first-ever refusal that identity still differs from the initial `None` +and would have looked like a fresh boundary. A verbatim user replay would have +been appended after a compaction that did not happen: misleading to the model, +and spending the tokens the refusal exists to save. + +Fixed with an explicit call-scoped flag (`_clear_at_least_last_refused`) rather +than by changing the replay's own boundary logic, so the fix **cannot alter +main's behaviour** — the flag is always `False` while the predicate is +disabled. Pinned by `test_a_refusal_is_not_a_boundary_for_the_last_user_replay` +and its positive counterpart. + +This is worth naming as evidence for the parallel-lane process: the bug did not +exist when this lane started and would not have been found by either lane +alone. + +--- + +## 5. Evidence + +### 5.1 Test suite + +``` +249 passed, 1 skipped in 5.57s +``` + +**41 new tests** in `tests/test_clear_at_least_predicate.py`; the 208 tests +already on `origin/main` are unmodified and green. The new tests are written +adversarially against the predicate's *own* failure mode (starvation), not just +its happy path: + +- **Blocks** — an unsatisfiable floor refuses the boundary; the returned view is + the untouched history; `context:compaction` is **not** emitted; + `context:compaction-skipped` carries freed/required/level/skips. +- **Allows** — a floor of 1 token produces a view **byte-identical** to running + with the predicate off (`test_allowed_boundary_is_identical_to_the_disabled_path`). +- **Default byte-identity** — `None`, `0`, and a negative value produce + identical views *and* identical event streams; `_clear_at_least_pending` stays + `None`, proving the guarded path is never *entered*, not merely that it agreed. +- **Rollback** — all three sticky sets, `_sticky_level`, and + `_last_compaction_stats` are unchanged after a refusal; no compaction notice + appears. +- **Prefix / `_seq` stability** — two consecutive refused calls with one turn of + growth in between share a byte-identical prefix (exact comparison, no + normalisation), and `_seq` identity is untouched. +- **Tool-pair integrity** — every `tool_calls` id in a refused view is answered + by a `tool_call_id`. +- **Starvation** — no-op calls never count as skips; the cap raises with a + message naming freed/required/protected set; state is rolled back *before* the + raise; the streak resets on an accepted compaction and on clear/resume. +- **Floor resolution** — 12 parametrised cases covering None/0/negative/int/ + fraction/1.0-boundary/zero-budget/malformed. +- **Shrink guard** — larger summary refused (history untouched, span not + recorded as removed, **not** counted as a summarizer failure); *equal-sized* + summary refused (the `>=` boundary exercised directly by searching for a + text that prices exactly at the span's estimate); genuinely smaller summary + still swaps. + +### 5.2 Default byte-identity vs `origin/main` (the honest stash-compare) + +Captures: +`/home/bkrabach/dev/openai-evals-team-ci/.amplifier/evaluation/treatment-validation/20260902-2o9-clear-at-least/` +(`byte_identity_check.py`, `baseline_origin_main__init__.py`, +`treatment__init__.py`, `byte_identity_result.txt`, +`negctl_predicate_on_by_default__init__.py`, `negative_control_result.txt`). + +Both module versions are loaded **in one process** and driven through identical +scenarios with identical inputs. Non-determinism is *eliminated*, not normalised +away: message timestamps are frozen to a fixed string after history is built, so +the two runs differ in nothing but the module source. Compared per call: the +returned view, every emitted hook event (name + payload), all three sticky +decision sets, `_sticky_level`, and `_last_compaction_stats`. + +**11 of 11 scenarios byte-identical:** + +``` +IDENTICAL default IDENTICAL tool_result_head_tail +IDENTICAL token_meter_actual IDENTICAL replay_last_user +IDENTICAL token_meter_hybrid IDENTICAL notice_disabled +IDENTICAL summary_strategy_no_provider IDENTICAL protected_tool_results_zero +IDENTICAL tool_result_budget IDENTICAL larger_budget + IDENTICAL system_prompt_factory +ALL SCENARIOS BYTE-IDENTICAL +``` + +The scenario set deliberately covers **every other opt-in feature**, not just +the default path: "default byte-identical" must also mean "does not perturb a +feature that was already opt-in". + +**The check is not vacuous.** A negative control that changes exactly one line — +`DEFAULT_CLEAR_AT_LEAST = None` → `20_000` — diverges immediately on the same +harness, and does so by raising the fail-loud error, which doubles as a live +demonstration of its content: + +``` +RuntimeError: context-simple: compact_clear_at_least=20000 could not be +satisfied 3 consecutive times (cap: 3). The compaction ladder reached level 8 +and freed only 7,698 tokens against a required floor of 20,000. Protected set +holding the floor: protected_tool_results=1 (of 30 tool results in the view), +protected_recent=20% of 124 messages. Baseline view is 7,852 tokens against a +budget of 1,200. Lower compact_clear_at_least, lower +protected_recent/protected_tool_results, or raise the budget -- compacting +harder will not help. +``` + +(That fixture's budget is 1,200 tokens, so a 20k floor is unsatisfiable by +construction — i.e. exactly the misconfiguration the fail-loud path exists for, +and the message names the right knobs.) + +--- + +## 6. What is NOT claimed + +**No workload measurement exists. No performance claim rides into source, the +PR body, or the README** (§5 rule 6). + +- The mechanism is implemented and unit-tested. Its effect on boundary count, + cost, waste, or latency on a real workload is **unmeasured**. +- The eval is filed as **`model_performance-wxs`** with gates pre-registered + *before* any spend: `G-CAL-BOUNDARIES` (monotonic dose-response across + `{null, 10k, 20k, 40k}`, n≥3 — a flat response falsifies the mechanism), + `G-CAL-NOSKIPHANG`, `G-CAL-LATENCY` (the pre-registered **win**), + `G-CAL-COST` and `G-CAL-WASTE` (non-regression only), `G-CAL-SKIPCOUNT` (an + instrument check that catches a run where every floor was trivially + satisfiable), and the Anthropic guardrail from raw wire fields. +- **A cost reduction is deliberately NOT pre-registered as a win.** §2b is + explicit that fewer boundaries "buys latency, not money" (`cad-fewer`: −29% + requests, −14% wall, *same* cost, same quality), and the underlying fit + (`waste ≈ 36.3 − 0.357 × boundaries`, r = −0.586, 12 points) is suggestive, + not established. +- **A retention gate is rejected as vacuous** here as everywhere: `b_constraints` + 40/40 and `c_post_compaction` 20/20 in every run of every arm across probes + 1–6. Deferred to `model_performance-cb2`. +- **The 20,000 starting value is not pinned.** It derives from an 18,458-token + head computed through a 4.59 chars/token constant that is tokenizer- and + model-version-specific (DESIGN-SPACE.md C-4). The eval item requires + re-deriving the head *in tokens* from provider usage first. The README says so + too. +- **Do not run this on the bare estimator.** The default `token_meter: + "estimate"` is `len(str)//4`, never reconciled against provider usage and ~2× + off in production sessions; a predicate acting on a 2× off number refuses the + wrong boundaries. `model_performance-q69` landed the provider-anchored hybrid + meter with provenance precisely so this predicate has an anchored number. + README and the eval item both state `token_meter: "hybrid"` as the operating + condition. + +**Known limitation, disclosed:** a refused call pays one wasted ladder pass +(CPU over an in-memory list, no tokens). Also, like the rest of this module's +shared mutable state, the predicate's call-scoped state assumes +`get_messages_for_request` is not re-entered concurrently — a pre-existing +property of the module, not new exposure, but now with one more field. + +No PII, no team-internal data, no individual attribution in any output. No +merges to main; no files touched outside this module. + diff --git a/docs/lanes/7k2-summary-call-fork/DONE-NOTE.md b/docs/lanes/7k2-summary-call-fork/DONE-NOTE.md new file mode 100644 index 0000000..21d00d7 --- /dev/null +++ b/docs/lanes/7k2-summary-call-fork/DONE-NOTE.md @@ -0,0 +1,214 @@ +# DONE-NOTE — W3-2 / `model_performance-7k2` + +`context-simple: cache-safe forking of the summarization call (summary_call_mode: "fork")` + +**Status: draft PR, mechanism shipped behind an off-by-default flag, nothing +measured.** Default is byte-identical to today. Lane spend: **$0.00** (the +item's authority was $0; no API calls, no DTU, no infrastructure created, +nothing to tear down). + +## 1. Step 0 — the free measurement that could have cancelled this item + +The item said: read the summarizer call; if it already prepends the parent's +assembled request, §2h conflict #8 resolves in favour of [P6]'s 2.4% table +figure and **there is nothing to build**. + +It does not. `_run_summary_compaction_task` built, verbatim: + +```python +request = ChatRequest( + messages=[ + Message(role="system", content=prompt), # the ~955-char prompt + Message(role="user", content=formatted), # the span, re-rendered as plain text + ], + model=self.summarization_model, +) +``` + +No parent history. No tools. Its own `role: "system"` prompt. This is the +standalone shape, so **the branch that cancels the item does not apply** and +the fix is real work. §2d's indirect evidence was right: G3 had to split out +the summarizer's own `instructions` population to get a clean hash precisely +*because* the summarizer sends its own separate system prompt. + +**What this does NOT resolve:** the 8.3–10.9% (prose) vs 2.4% (same source's +own table) conflict about the summarizer's cost *share*. Being standalone +makes the cost avoidable; it does not say how much cost there is. That number +must come from a run's own per-call table, and the follow-up item requires it. + +## 2. What shipped + +`summary_call_mode: "standalone" | "fork"`, default `"standalone"`, consulted +only when `compaction_strategy == "summary"`. + +- **`"standalone"`** — the pre-existing two-message call, unchanged. + `"inline"` is accepted as an alias (the lane brief and the work item named + the same default differently; both mean "today's behaviour"). Unknown values + warn and fall back, matching every other enum in this module. +- **`"fork"`** — the same ask re-issued as a **pure append**: + `[...the exact messages of the last request...] + [one user message: prompt + scope]`. + Pure append is the one mutation measured as a HIT under grow-only + (P4: identical-repeat 9,789 HIT, pure-append 9,789 HIT, truncation 0, + middle-drop 0). + +Three consequences, each load-bearing rather than incidental: + +1. **The span is not re-sent.** It is already inside the prefix. Re-sending it + would cost what standalone costs today *plus* the prefix — a regression + wearing an optimization's clothes. This is the single decision that + determines whether the feature is worth anything. +2. **The prompt moves into the appended `role: "user"` message.** A fork must + not add a `role: "system"` message: providers hoist every system-role + message into one top-level system block, so a per-summarization one would + rewrite the cached system prefix — the exact failure already measured for + the summary tier (cache_read 46,307 → 21,523) and the compaction notice. +3. **Scope must be stated.** Standalone scopes by construction (it can only + see the span). A fork can see everything, so the appended message names the + span: its message count plus a bounded (300-char) verbatim excerpt of its + final message as the "summarize up to here" marker. **This is a real + behavioural difference between the two modes**, not formatting — flagged as + a residual risk in the follow-up item, not hand-waved. + +New optional public seam: **`note_request_sent(messages=None, *, tools=None, +model=None)`** — see §3, which is the finding this lane most wants read. + +Observability: `last_summary_call_stats` property (`mode_requested`, +`mode_used`, `reason`, `prefix_messages`, `fork_fallbacks`) and a `call_mode` +field on `context:pre_summarize` / `context:post_summarize`, so an eval arm +can count *real* forks without patching the module. + +Size: **170 executable lines** across the new helpers (item estimated 50–70). +The overrun is entirely the refusal ladder (§3) and the staleness guard (§4); +the happy path is ~30 lines. + +## 3. The finding: this cannot be done from inside the context module alone + +**Tool specs are part of the cached prefix and this module is never handed +them.** Anthropic serializes `tools` *ahead of* the system block; OpenAI's +implicit cache matches forward from a cached entry. A summarizer request with +`tools=None` diverges from the parent at byte zero, so it hits nothing — and +because it now carries the whole conversation, it costs *more than the +standalone call it replaced*. That is not a missed win, it is a regression. + +I looked for a way to obtain them without changing any caller and found none +that is contract-backed: + +- `amplifier_core.interfaces` defines no tool-registry protocol; the + coordinator exposes `mount_points`, not tool specs in send order. +- The `llm:request` event carries `model`/`message_count`/thinking flags, and + the actual tool payload only under provider `raw: true` — where it is wire + dicts passed through `redact_secrets()`, i.e. not reconstructable into + `ToolSpec` byte-faithfully. +- The default orchestrator lives in the compiled Rust engine, so there is no + seam to read it from. + +So fork mode requires the caller to say what it sent. **Decision taken without +waiting** (per SCOPE-OUTS): add one optional public method rather than a +config knob that lets an operator *assert* alignment they cannot verify. +Passing `tools` **at all** — even `None`/`[]` for a genuinely tool-free +session — is what arms the fork; "not supplied" and "supplied as empty" are +deliberately distinguishable, because guessing between them is exactly how a +fork silently misaligns. + +**Every misalignment refuses and falls back to standalone, loudly** — WARNING +naming the precondition (once per distinct reason, so a session that can never +fork costs a handful of log lines, not one per summarization), a session +counter, and the mode actually used reported on both hooks and the stats +property. The refusals: seam never called · `summarization_model` set (a +summarizer routed elsewhere reads none of the main line's cache — deepseek +states this explicitly) · no request recorded yet · prefix ends on an assistant +turn with unanswered `tool_calls` (appending there would interleave between +`tool_use` and `tool_result`) · span absent from the recorded prefix · the +forked request fails to build. + +Falling back to *standalone* rather than skipping is deliberate: standalone is +correct and costs what it always did. Losing a summary over a cache +optimization would be strictly worse than not optimizing. + +**Consequence for the eval, stated plainly:** the S5-CRAC harness must wire +`note_request_sent()` or the treatment arm silently degrades into the control +arm and every gate is vacuous. That is the first thing the follow-up item +(`model_performance-6da`) tells the runner to verify — on a 1-run smoke, +before any spend. + +## 4. Provider asymmetry, and the staleness trap + +**Anthropic vs OpenAI need different fork sources, and the seam covers both.** +Anthropic places its cache breakpoint on the last *stable* message, walking +back past ephemeral/injected content — which is exactly where this module's +own returned view ends. So `tools` alone is enough there. OpenAI's implicit +cache measured **MISS on strict truncation** (P4), i.e. it needs a strict +superset of a cached request; an orchestrator-injected tail this module never +sees would break that. So on OpenAI the caller must also pass `messages`. +`_capture_fork_prefix` prefers the caller's record and falls back to the +module's own view. + +**The trap I built a guard for.** A caller that wires `note_request_sent()` +once (a startup helper, first turn only) would have turn 40's fork append to +turn 1's request — a guaranteed miss *and* a wasted cache write, wearing a +correct-looking API call. A `_view_serial`/`_sent_serial` pair means a +`messages` record is only used while it still describes the most recent +request served; a stale one is ignored (module view used instead), never +trusted. Tested. + +## 5. Evidence — 282 tests green (250 before, 32 new) + +`tests/test_summary_call_mode_fork.py`, written against the ways this goes +silently wrong rather than the way it is supposed to work: + +| Group | What it pins | +|---|---| +| **A — the default must not move** | The standalone request is asserted against **independently rebuilt** expected content (2 messages, system prompt then formatted span, `model` from `summarization_model`), not against itself. Plus: the fork bookkeeping is never even *written* unless armed (`_last_request_view is None` after 5 requests in both default configs) — an always-on capture would be a silent per-request list allocation on the hot path. Plus: `note_request_sent()` is fully inert when fork is not configured. | +| **B — pure append** | `sha256(fork[:-1]) == sha256(parent_view)` (G-FORK-PREFIX in unit form); exactly one appended message; **no new system message**; tools and model pinned from the seam; the span is **not** re-sent (`formatted not in appended`, and the appended message is shorter than the formatted span); the caller's `messages` record wins verbatim when supplied, including an injected tail. | +| **C — the main line is not the summarizer's scratchpad** | No `_seq` consumed; history byte-identical before/after; `_removed_seqs` unmoved; **`_last_sent_estimate` unmoved** (building the fork through `_finalize_view` would silently rewrite the hybrid meter's conservatism comparand with the summarizer's own request — this is why `_build_fork_request` calls `_strip_internal_metadata` directly); and a forked session serves views **bit-for-bit identical** to an unforked control, same `_removed_seqs`. | +| **D — refusals** | All six refusal paths fall back to the 2-message standalone request with the reason recorded; warning fires; warn-once-per-reason while the counter still counts every one; a failed fork build still produces the summary; hooks report the mode actually used; the stale-record guard prefers the fresh view. | +| **E — tool pairs** | `_select_summary_absorb_seqs` returns **identical** spans in both modes on a tool-pair-heavy history; end-to-end, no tool result is ever served without its call. The call mode changes how the summarizer is *called*, never what is *selected*. | +| **F — reset/lifecycle** | `clear()` and `set_messages()` drop all fork alignment state; the fork prefix is snapshotted at **trigger** time, not at task-scheduling time (proved by mutating `_last_request_view` while the task is in flight); the single-summarization-in-flight guard still holds under `asyncio.gather` of three concurrent requests. | + +`ruff check` clean. `ruff format` was **not** run: the repo is not +format-clean today (7 pre-existing files would be reformatted), so running it +would bury this change in unrelated noise. + +## 6. What is NOT claimed + +- **No cache win. No cost win. Nothing was measured.** G-FORK-PREFIX / + G-FORK-CACHED / G-FORK-COST / G-FORK-NOBOUNDARY are the follow-up item + (`model_performance-6da`, filed `discovered-from` this one, with the arm + design, the blocking prerequisite, and the residual risks). $0 spent here by + mandate. +- **Honest ceiling, unchanged from the item's own statement:** this reduces + the separate summarizer charge only. It does **not** touch the boundary + rebuild, which §2d measured as the dominant cost (+84% boundaries → +83% run + cost). If the summarizer's real share is the table's 2.4% rather than the + prose's 8.3–10.9%, a *perfect* fork is worth ~2% of run cost and the honest + recommendation may be "mechanism proven, not worth enabling". The follow-up + item is required to report that share from the run's own table and to say so + if it lands there. +- **Retention parity is not established.** Fork mode scopes by instruction + where standalone scopes by construction. The follow-up item reports S5 score + and `b_constraints`/`c_post_compaction` per arm so a cost win bought with a + retention loss cannot be reported as a win. +- **`reasoning_effort` / thinking configuration is not reproduced** by the + fork — it is not observable from inside a context module. If a provider keys + its cache on it, the fork misses despite byte-aligned messages. + G-FORK-CACHED is the detector; this is disclosed, not designed around. +- **The compaction-buffer reserve (condition (a) in the item) is not + implemented.** The appended instruction is ~1.2k chars, not the ~8,000-token + reserve the item contemplated, and the fork never grows the *main* line's + request. If the follow-up eval shows the fork's own request crowding the + window, that reserve is the fix, and G-FORK-NOBOUNDARY is the gate that + would catch it. + +## 7. Deliverable ledger + +| Deliverable | Status | +|---|---| +| DRAFT PR on origin, branch `lane/7k2-summary-call-fork`, default byte-identical, tests green | **DONE** | +| Prefix-stability test proving the fork does not touch the main line | **DONE** — Group C (no `_seq`, history byte-identical, `_last_sent_estimate` unmoved, forked-vs-control views identical) | +| Follow-up eval item filed via `work_file` with its arm design | **DONE** — `model_performance-6da` | +| DONE-NOTE.md in the PR body | **DONE** — this section | + +No PII, no team-internal data, no individual attribution. No merges to main. +No files touched outside this module. No infrastructure created; nothing to +tear down. + diff --git a/docs/lanes/README.md b/docs/lanes/README.md new file mode 100644 index 0000000..b35dc1d --- /dev/null +++ b/docs/lanes/README.md @@ -0,0 +1,42 @@ +# Lane notes + +One directory per lane, one `DONE-NOTE.md` per directory. This is the +`artifact-path/v1` root resolved for this repo (item `model_performance-6x4`, +rule R3: no top-level `probes/`, no `ai_working/`, no wave-prefix convention, so +the documented fallback `docs/lanes//` applies). + +**There is no repo-root `DONE-NOTE.md`, and there must never be one again.** +`tools/check_done_note_placement.py` fails the build on it, and runs as part of +`pytest` via `tests/test_done_note_placement.py`. + +## Why + +Six lanes appended their notes into one shared repo-root `DONE-NOTE.md`. Two +lanes writing the same path is not a git conflict — it is git working correctly +— so nothing could raise an alarm. Two things then happened, both silently: + +* PR #30, `revert: unproven default-off features per merge policy (wins only)` + (`e9ac159`), reverted the feature code **and the shared note file with it**, + deleting four lanes' notes from `main` in a single commit. +* One lane's note (`rb1`) never reached `main` at all — it was overwritten out + of the lineage before the revert, and survives only on + `origin/lane/rb1-rebase-conflicted-prs`. + +Every note was recovered from git history and re-homed here by item +`model_performance-sqh`; the enumeration, provenance and round-trip proof are in +[`sqh-context-simple-note-loss/AUDIT.md`](sqh-context-simple-note-loss/AUDIT.md). + +## Index + +| lane | item | subject | +|---|---|---| +| [`x7p-protected-tool-results-bug`](x7p-protected-tool-results-bug/DONE-NOTE.md) | `model_performance-x7p` | `protected_tool_results=0` protected ALL tool results (negative-slice bug) | +| [`x1r-tool-result-budget`](x1r-tool-result-budget/DONE-NOTE.md) | `model_performance-x1r` | tool-result budget (token-denominated, head+tail, per-tool) + spill-to-disk | +| [`2o9-clear-at-least`](2o9-clear-at-least/DONE-NOTE.md) | `model_performance-2o9` | `clear_at_least` — a worth-the-rebuild predicate in front of compaction (+ summary shrink guard) | +| [`7k2-summary-call-fork`](7k2-summary-call-fork/DONE-NOTE.md) | `model_performance-7k2` | `summary_call_mode` — cache-safe forking of the summarization call | +| [`jnt-fork-prefix-capture`](jnt-fork-prefix-capture/DONE-NOTE.md) | `model_performance-jnt` | `_capture_fork_prefix` forked onto an array that was never sent | +| [`rb1-rebase-conflicted-prs`](rb1-rebase-conflicted-prs/DONE-NOTE.md) | `model_performance-rb1` | merge-queue repair: rebased and landed the conflicted lane PRs (#21, #24) | +| [`sqh-context-simple-note-loss`](sqh-context-simple-note-loss/DONE-NOTE.md) | `model_performance-sqh` | this recovery: enumeration, re-homing, and the guard | + +The first four rows' wording is carried over verbatim from the index table of +the shared file's last revision (`5cdbc62`), so nothing that file said is lost. diff --git a/docs/lanes/jnt-fork-prefix-capture/DONE-NOTE.md b/docs/lanes/jnt-fork-prefix-capture/DONE-NOTE.md new file mode 100644 index 0000000..5b1f8e1 --- /dev/null +++ b/docs/lanes/jnt-fork-prefix-capture/DONE-NOTE.md @@ -0,0 +1,171 @@ +# DONE-NOTE - model_performance-jnt + +**`_capture_fork_prefix()` appends to an array that was never sent (11 of 20 +forks measured).** + +**Verdict: CORRUPTING, not cosmetic.** It is not dead code and it produces a +record. It silently substitutes a message array that never went on the wire +for one that did, on a path whose entire purpose is byte-parity with the wire. +Fixed. **No 6da measured number is invalidated** — see §3. + +## 1. The array, named, at file:line + +Everything below is `amplifier_module_context_simple/__init__.py` at the +pre-fix commit `a877b36`. + +| | | +|---|---| +| **the array** | **`self._last_request_view`** | +| created | **:1015** (`__init__`), written **:1778** inside `_finalize_view()` | +| what it holds | this module's own last RETURNED view, pre-strip | +| why it is "never sent" | it is written on **every view served**, and a view served is not a request sent | +| consumed by | **:4187** `_capture_fork_prefix()`, as the fallback source | +| flows to | `_maybe_trigger_summary_compaction` **:4040** → `_run_summary_compaction_task` → `_build_fork_request` **:4422** → the provider | + +The selection at **:4187–4198** preferred the caller's recorded wire array +(`_sent_messages`, **:1017/:1210**) only while +`_sent_serial == _view_serial` (**:4189**), and substituted +`_last_request_view` whenever that equality failed (**:4187**, **:4192–4197** +— `logger.debug`, not warning). + +**Why the equality fails in production.** `_view_serial` counts **views +served**, not **requests sent**, and the two are not 1:1. The real +orchestrator serves the view more than once per sent request: +`amplifier-module-loop-streaming/__init__.py` calls +`context.get_messages_for_request()` at **:3215** and then re-fetches at +**:3329** and **:3453** after persisting an ephemeral injection — up to three +views for one `ChatRequest`. The summary trigger is evaluated inside *every* +one of them (**:1364**, which runs *before* that call's `_finalize_view` at +**:1480/:1482**). So: + +- trigger on the **first** view of a request → serials match → the wire array + is used → **correct fork** (wire offset 0 or 1); +- trigger on a **re-fetched** view → serials differ by 1–2 → the wire array is + discarded in favour of `_last_request_view`, which at that instant holds + **the view the re-fetch just superseded** — built, thrown away, never sent. + +That is the 9-vs-11 split 6da measured, and it reproduces exactly. + +## 2. Consequence — unambiguous + +**Corrupting to fork-mode behaviour, and self-concealing.** + +1. **Guaranteed cache miss.** A superseded view is not a prefix any provider + holds. Fork mode's only justification is appending onto a cached prefix; a + fork that misses pays full price for the whole conversation, which is + *strictly worse* than the standalone call it replaces. +2. **Silent.** `last_summary_call_stats["mode_used"]` reported `"fork"` for + all 20 calls. The substitution was `logger.debug`. There was no field + distinguishing the two sources — this is precisely why 6da had to + reconstruct the distinction from the provider's request log. +3. **Backwards under uncertainty.** The check traded the array with *positive + evidence* of having been sent (the caller said so) for one with *none* + (fate unknown to this module), and did so exactly when uncertainty was + highest. + +Not affected: history, `_seq` allocation, span selection, tool-pair +integrity, the served view, or any default-mode behaviour. The blast radius +is fork mode's cache economics and the honesty of its self-report. + +## 3. Does this invalidate any of 6da's measured numbers? **NO.** + +Stated plainly for the manager, because the item asked for it loudly: + +- **G-FORK-PREFIX (2/7/11 offset distribution, 45% aligned) — VALID, and is + the direct measurement of this bug.** Scored from the wire, not from the + module's self-report. +- **G-FORK-CACHED, G-FORK-NOBOUNDARY, the Anthropic guardrail, quality + parity, and the −0.8% run-cost delta — VALID.** All are wire/usage-derived + and none depend on `_capture_fork_prefix()` having chosen correctly. +- **The §7 correction (summarizer share ≈30%, not 2.4%/8.3–10.9%) — VALID + and untouched.** Independent of the fork path. + +**One caveat, in 6da's favour, not against it:** the −0.8% run-cost delta was +measured with only ~45% of forks byte-aligned. It is a **lower bound** on +what a correctly-aligned fork arm would deliver, not an upper bound. 6da's +"the mechanism works and the lever does not pay / DON'T-SHIP as-is" verdict +therefore **still stands as written**, but its cost figure is now known to +have been measured on a partially-broken treatment and should be **re-measured +before the DON'T-SHIP call is made final**. 6da itself flagged this +("one of them has a cheap fix worth a follow-up item"); this is that fix. + +## 4. The fix (minimal) + +`_capture_fork_prefix()` (**:4176**) no longer substitutes: + +- a recorded wire array, when one exists, is **used** — it is the only source + carrying positive evidence it was on the wire, so it is never traded for one + that carries none. Extra views served since the send are **reported, not + acted on**; +- the module's own view is used **only** when the caller has never supplied a + message array (the documented explicit-breakpoint/Anthropic path, unchanged); +- staleness in the wire record is still caught, but by an **exact** check + instead of a proxy: a record too old to contain the span fails + `_prefix_contains_span` and **refuses LOUDLY** — standalone call, `WARNING`, + `_summary_fork_fallbacks` incremented, named `reason`. "A fork that silently + missed" is no longer reachable on this path. + +Also added, because 6da needed it and could not get it: **`prefix_source`** +(`"wire_record"` / `"module_view"` / `None`) and **`prefix_views_since_send`** +on `last_summary_call_stats`. The next arm can separate the two populations +from the module's own report instead of reconstructing them from the wire. + +**Config surface: unchanged. Default `summary_call_mode` remains +`"standalone"`.** + +## 5. Tests + +`286 passed, 1 skipped` (was 281 passed at `a877b36`). `ruff check`: clean. + +New Group F in `tests/test_summary_call_mode_fork.py`: + +| Test | Pins | +|---|---| +| `test_a_re_fetched_view_does_not_displace_the_recorded_wire_array` | THE regression: a superseding re-fetch must not displace the wire array | +| `test_the_module_view_is_never_substituted_when_a_wire_record_exists` | same defect from the other side: never-sent content cannot reach the fork | +| `test_prefix_source_names_the_module_view_path_honestly` | the module-view path is allowed but reported as what it is | +| `test_prefix_source_is_none_when_the_call_did_not_fork` | a refused fork claims no alignment | +| `test_tool_pair_integrity_and_seq_stability_survive_the_re_fetch_path` | no `_seq` consumed, history byte-identical, same span absorbed, served view identical to an unforked control | + +**These three fail against the pre-fix selection logic and pass against the +fix** — verified by temporarily restoring the old branch and re-running; they +are load-bearing, not decoration. + +One existing test changed: `test_a_stale_caller_message_record_is_ignored_not_trusted` +→ `test_a_stale_caller_message_record_refuses_loudly_not_silently`. It +asserted the substitution *as correct behaviour*; it now asserts the loud +refusal. The rewritten docstring records why the original resolution was +wrong, so the reversal is not silent. + +Default-mode byte-identity re-verified by the pre-existing Group A/C tests, +strengthened with `assert context._fork_prefix_source is None` in +`test_default_mode_never_records_a_fork_prefix`. + +## 6. Residual, disclosed + +The **module-view path** (`note_request_sent(tools=...)` with no `messages`) +can still append to a superseded view — this module genuinely cannot know +whether its own view was sent. Not silently, now: `prefix_source == +"module_view"` says so on every call. **A caller that wants byte-parity must +pass `messages`.** Closing this properly needs a caller-side confirmation +signal, which is an orchestrator change and out of this lane's scope. + +Unchanged and still true: fork mode cannot fork the **first** summarization of +a CLI turn (each turn is a fresh `amplifier run --resume` process, and the +trigger is evaluated before any request is sent). 6da measured 12 of 24 +refusals from this; this fix does not address it. + +## 7. Deliverable ledger + +| Deliverable | Status | +|---|---| +| DRAFT PR on origin, branch `lane/jnt-fork-prefix-capture`, tests green, default inline byte-identical | **DONE** | +| The array named at file:line with why it was never sent + cosmetic-vs-corrupting verdict | **DONE** — §1, §2 (**corrupting**) | +| Explicit statement of whether any of 6da's measured fork numbers are invalidated | **DONE** — §3 (**none invalidated**; −0.8% is a lower bound and warrants re-measurement) | +| DONE-NOTE.md in the PR body | **DONE** — this section | + +**Spend: $0.00.** No API calls, no DTU, no containers, no infrastructure +created — the item was answerable from the code, the shipped tests, and 6da's +existing evidence files. Nothing to tear down; nothing registered in the infra +ledger. No PII or team-internal data. No merge to main. No files touched +outside this module. diff --git a/docs/lanes/rb1-rebase-conflicted-prs/DONE-NOTE.md b/docs/lanes/rb1-rebase-conflicted-prs/DONE-NOTE.md new file mode 100644 index 0000000..189fa5f --- /dev/null +++ b/docs/lanes/rb1-rebase-conflicted-prs/DONE-NOTE.md @@ -0,0 +1,193 @@ +# DONE-NOTE — model_performance-rb1 + +`Merge-queue repair: rebase and land the conflicted lane PRs (unblocks 57p)` + +**Spend: $0.00.** No API calls, no eval runs, no DTU, no containers, no +infrastructure created or registered (nothing to tear down). Everything below is +local `git` / `uv run pytest` / `gh` on this host. The lane's spend authority was +$0 and none of it was used. + +| | | +|---|---| +| starting `origin/main` | `f47c894` (`token_meter "hybrid"`, on top of `d5ded0c` #23 and `c6dfbba` #20) | +| final `origin/main` | `3972070` | +| PRs landed | **#21 → `49e2799`**, **#24 → `3972070`** (both `--squash --admin`) | +| PRs deliberately untouched | routing-matrix **#49** (see §4) | +| suite, start → end | **139 passed → 208 passed, 1 skipped** | + +--- + +## 1. PR #21 — tool-result budget + spill — **DONE, merged `49e2799`** + +Branch `lane/x1r-tool-result-budget`, 3 commits, rebased `c6dfbba` → `f47c894`. + +**What actually conflicted — two things, not one.** The lane brief predicted a +single `DONE-NOTE.md` add/add. There was also a **real code conflict**, and it is +recorded here rather than smoothed over: + +| # | file | site | ours (`origin/main`) | theirs (#21) | +|---|---|---|---|---| +| 1 | `amplifier_module_context_simple/__init__.py` | `clear()` | `self._reset_hybrid_meter_state()` (from #22 `f47c894`) | `self._tool_name_by_call_id = {}` + `self._spilled_paths = set()` | +| 2 | `DONE-NOTE.md` | whole file | x7p note (from #23 `d5ded0c`) | x1r note | + +`README.md` auto-merged; no third-party hunk. + +**How each was resolved.** + +1. **`clear()`** — the diff3 base section was **empty**, i.e. both sides *added* + different lines at the same insertion point; neither modified the other's + code. Resolved by keeping **both**, ours first. Semantically both are + required: `clear()` must reset the hybrid meter anchor *and* drop the + per-tool name map, and neither reset can substitute for the other. +2. **`DONE-NOTE.md`** — resolved by **keeping both notes verbatim**, not by + picking a winner. The file was given the structure it now has: an HTML-comment + convention header declaring it **shared and append-only**, an index table, then + each lane's note under its own original `# DONE-NOTE — ` heading, oldest + first. This is why the same conflict is cheaper for the next lane: the + documented resolution is "append, never replace". + +**Suite:** `187 passed` on the rebased branch (baseline `f47c894` = `139 passed`; +#21 contributes 48 tests). Green before the push, green on `origin/main` after. + +## 2. PR #24 — `replay_last_user_on_compaction` — **DONE, merged `3972070`** + +Branch `lane/l8-replay-last-user`, 1 commit, rebased `c6dfbba` → `49e2799` +(i.e. **after** #21 landed, as instructed). + +**8 conflict hunks — 7 in `__init__.py`, 1 in `README.md`.** Every one of them was +additive-vs-additive at a shared insertion point (module docstring feature +section; `mount()` config docstring; `mount()` kwarg passthrough; `__init__` +signature; `__init__` Args docstring; `__init__` state initialisation; `clear()` +reset; and in `README.md`, two new sections anchored before `## Dependencies`). + +Rather than eyeball eight hunks, the resolution was **mechanical and checked**: +a script walked the diff3 markers and, for each hunk, *asserted the base section +was empty* before emitting ours-then-theirs. Any hunk where the two sides had +actually touched the same pre-existing lines would have been reported and handled +by hand. **None were** — so the rebase is a pure re-application of #24's work on +top of #21 + #22 + #23, with nothing of theirs displaced. + +**Proof the rebase changed nothing about the feature**, rather than an assertion: + +``` +git diff --stat origin/main..pr24 + README.md | 83 ++++ + amplifier_module_context_simple/__init__.py | 277 +++++++++++ + tests/test_replay_last_user.py | 702 ++++++++++++++++++++++++++++ + 3 files changed, 1062 insertions(+) <- 0 deletions +``` + +1,062 insertions and **zero deletions** against the new `origin/main` — identical +to the pre-rebase insertion count. Nothing already on `main` was modified or +removed to make room for the replay feature. + +**Both features' defaults verified to coexist, post-resolution:** + +| knob | default on merged `main` | +|---|---| +| `replay_last_user_on_compaction` | `False` (signature `= False`, and `config.get(..., False)`) | +| `token_meter` | `TOKEN_METER_ESTIMATE` (unchanged by #24 — 0 deletions) | +| `tool_result_budget_tokens` | `None` (#21's no-op default, unchanged) | +| `compaction_strategy` | `COMPACTION_STRATEGY_PROGRESSIVE` | + +**Suite:** `208 passed, 1 skipped` on the rebased branch and on `origin/main` +after merge (`49e2799` = `187 passed`; #24 contributes 22 tests, one of which +skips — see §3). + +## 3. DEVIATION — the "1 failing test" on #24 was **1 SKIP, never a failure** + +The lane brief said #24's branch was "124/125 — find and fix the one failure +before merging, or REFUSE and say why." Measured on the branch **as it was, before +any rebase** (`89ada64`, base `c6dfbba`): + +``` +124 passed, 1 skipped in 5.00s +SKIPPED [1] tests/test_replay_last_user.py:682: amplifier-foundation cannot be +a dependency here (it requires amplifier-core>=1.0.10; this repo pins core +<1.0.10). The frozen reference in +test_predicate_is_strictly_stronger_than_foundation_1_0_0 covers the same +contract unconditionally. +``` + +**There was no failing test to fix, at any point.** The skip is a deliberate, +environment-conditional guard with its reason stated in the skip message, and the +same contract is covered **unconditionally** by a companion test that runs against +a frozen reference. "Fixing" it would mean adding `amplifier-foundation` as a +dependency, which this repo **cannot** take: foundation requires +`amplifier-core>=1.0.10` and this repo pins core `<1.0.10`. That is a dependency +change, not a test fix, and it is outside this lane's scope. + +Recorded as a deviation rather than silently satisfied: the count `124/125` in the +brief was read as pass/fail when it was pass/skip. Nothing was changed to make the +number look different. + +## 4. routing-matrix #49 — **DEFERRED, untouched (as instructed)** + +A live lane owns that repo. This lane issued **no** command against it: not +cloned, not fetched, not checked out, no `gh` call. Nothing to hand over beyond +"still open, still owned elsewhere". + +## 5. Is `model_performance-57p` unblocked? — **Partly. Be precise about which blocker.** + +Filing nothing, per the brief; stating it here instead. + +**Unblocked:** the *merge-queue* blocker is gone. The treatment (`#24`, +`replay_last_user_on_compaction`) is now **on `origin/main` at `3972070`**, still +**default-off**, alongside #21/#22/#23. The eval no longer needs a draft branch, +and no longer has to be run against a tree that conflicts with main. + +**Still blocked, and this lane did not change it:** 57p's own description blocks +it on **S7 existing and being demonstrated to discriminate** — "the T0 progressive +baseline MEASURABLY loses the most recent user instruction". That is a scenario +prerequisite ([00-what-we-know §4.1]: S5-CRAC is saturated at 40/40 constraints +and 20/20 post-compaction in every arm of probes 1–6). Running G1 against a +saturated scenario reproduces the PROBE6 error. **Merging #24 did not create S7.** + +**Two things the eval lane must re-do because `main` moved (confidence: measured):** + +1. **The recorded T0 hash is stale.** 57p pins T0 as "flag off, verified + byte-identical to `origin/main`, sha256 + `c985bbb95ec8aea0b74b058cc4ad109fbee73c822d6739a2df6ec547286789f9`". That hash + was taken against the *old* `origin/main`. Four PRs (#20, #23, #22, #21) have + landed since. T0 is still "the flag off", and the flag is still default-off, but + **the hash no longer identifies the same tree** — re-baseline it against + `3972070` rather than trusting the recorded value. +2. **One-variable discipline now has more knobs to hold still.** #21 added five + tool-result knobs (`tool_result_budget_tokens`, `tool_result_shape`, + `tool_result_budget_by_tool`, `tool_result_exempt_tools`, + `tool_result_spill_dir`), all default no-op. The T0/T1 arms must leave **all** + of them, plus `token_meter` and `compaction_strategy`, at their defaults — the + arms differ only in `replay_last_user_on_compaction`. + +## 6. Suite counts, every measurement point + +| point | tree | result | +|---|---|---| +| baseline, before this lane | `origin/main` `f47c894` | **139 passed** | +| #24's branch, pre-rebase (as the brief found it) | `89ada64` (base `c6dfbba`) | **124 passed, 1 skipped** | +| #21 after rebase + conflict resolution | `c92dcb8` | **187 passed** | +| `origin/main` after #21 merged | `49e2799` | **187 passed** | +| #24 after rebase + conflict resolution | `de2a943` | **208 passed, 1 skipped** | +| `origin/main` after #24 merged (final) | `3972070` | **208 passed, 1 skipped** | + +Command at every point: `uv run pytest -q` (with `-rs` where the skip is quoted). + +## 7. Choices made without waiting for a human + +Per the lane rule that no human decision is waited on — each choice, and why: + +- **`DONE-NOTE.md` conflict → keep both, append-only, add a convention header.** + The alternative (one lane's note wins) destroys a deliverable another lane was + paid for. The header exists so the *next* add/add is resolved the same way + without re-deciding. +- **#24's skip → recorded, not "fixed".** Making it pass requires a dependency + this repo pins against. Recorded honestly instead (§3). +- **Merged with `--admin`.** Self-approval is blocked by the ruleset; `--admin` + squash-merge is the sanctioned path already used for the six merges that landed + earlier today. Both PRs were taken out of draft first (`gh pr ready`) — a draft + cannot merge even with `--admin`. +- **Remote branch deletion:** `--delete-branch` deleted both remote branches; + the *local* delete failed on both ("used by worktree"), which is cosmetic and + left alone rather than tearing down another lane's worktree. + +No PII, no team-internal data, no individual attribution. diff --git a/docs/lanes/sqh-context-simple-note-loss/AUDIT.md b/docs/lanes/sqh-context-simple-note-loss/AUDIT.md new file mode 100644 index 0000000..bedb325 --- /dev/null +++ b/docs/lanes/sqh-context-simple-note-loss/AUDIT.md @@ -0,0 +1,225 @@ +# AUDIT — repo-root `DONE-NOTE.md` loss in `amplifier-module-context-simple` + +**Item:** `model_performance-sqh`. **Method:** git only, no lane self-report +trusted — the same method item `model_performance-kez` proved in the evals repo +(`probes/kez-done-note-collision/AUDIT.md`), re-run here. +**Repo state audited:** `origin/main` = `e9ac159`, fetched 2026-09-02. + +## Headline + +**The count in the item is wrong, and the real damage has a second, worse half.** + +The item was filed as *"8 lanes' DONE-NOTEs deleted from origin/main by the +revert"*, listing `2o9, 6da, 7k2, cb2, jnt, q69, wxs, x1r`. That list came from +grepping `model_performance-[a-z0-9]+` over the file, which counts **mentions in +prose**, not authorship. Measured by `# DONE-NOTE … model_performance-` +headings — the only thing that marks a note as *written by* a lane: + +| | count | +|---|---| +| Distinct blobs the root path ever held (all refs + unreachable objects) | **11** | +| Distinct lane notes ever written at the root path | **6** | +| Still on `origin/main` today | **1** (`x7p`) | +| **Deleted from `main` by the `e9ac159` revert** | **4** (`x1r`, `2o9`, `7k2`, `jnt`) | +| **Never reached `main` at all** (silently overwritten out of the lineage) | **1** (`rb1`) | +| **UNRECOVERABLE** | **0** | + +`6da`, `cb2` and `wxs` are **evals-repo** lanes; they never wrote a note in this +repo and are accounted for in `kez`'s audit there. `q69` and `l8` targeted this +repo but wrote **no DONE-NOTE anywhere in its object store**. Naming them as +victims here would have been a fabricated loss. + +The `rb1` half is the part the item did not know about and is the more dangerous +one: a revert is at least *visible in the log*. `rb1`'s note was lost with no +event at all — see below. + +## Method (reproduce) + +```bash +git fetch origin +# 1. every commit that ever touched the root path, across ALL refs +git log --all --full-history --oneline -- DONE-NOTE.md # 20 commits +# 2. blobs + AUTHORED notes per blob, including UNREACHABLE objects +python3 docs/lanes/sqh-context-simple-note-loss/recover_root_done_notes.py --report +# 3. cross-check the object store directly +git fsck --lost-found +git rev-list --objects --all | grep -i done-note +``` + +Two things make the naive walk under-count, and both bit here: + +* `--full-history` matters: plain `git log origin/main -- DONE-NOTE.md` shows + **6** commits, the `--all --full-history` walk shows **20**, because history + simplification prunes the losing side of a merge. That pruning is exactly what + hides `rb1`. +* **Reachability matters too.** The `--all` walk finds 10 blobs; a sweep of every + *repo-root tree in the object store* finds an **11th** (`231979c1`) that no ref + reaches. The recovery script therefore does both passes. Enumerating from + reachable history alone would have silently missed it. + +## The eleven blobs + +Ascending by size. "authors" = `# DONE-NOTE … model_performance-` headings +actually present in that blob. + +| blob | bytes | authors | first commit seen | +|---|---|---|---| +| `ecc7cccb` | 14,989 | `x7p` | `d5ded0c` / restored by `e9ac159` — **== `origin/main` today** | +| `52052886` | 17,073 | `x1r` | `1f59edc` (x1r's standalone note, pre-merge) | +| `d5ec4350` | 17,158 | `x1r` | `afce8ff` | +| `5bfde271` | 32,657 | `x7p`, `x1r` | `fca37bd` | +| `231979c1` | 32,269 | `x7p`, `x1r` | **unreachable object** — see below | +| `4b822d72` | 32,742 | `x7p`, `x1r` | `49e2799` (PR #21) | +| `be96dd2b` | 42,979 | `x7p`, `x1r`, **`rb1`** | `56270a1` — **never an ancestor of `main`** | +| `0268ee62` | 48,022 | `x7p`, `x1r`, `2o9` | `2c42faa` | +| `412d6122` | 48,173 | `x7p`, `x1r`, `2o9` | `f851d12` (PR #26) | +| `0bd7671a` | 61,868 | `x7p`, `x1r`, `2o9`, `7k2` | `a877b36` (PR #27) | +| `c790066d` | 71,165 | `x7p`, `x1r`, `2o9`, `7k2`, `jnt` | `5cdbc62` (PR #28) — richest | + +`231979c1` is reachable from no ref; it survives only as a dangling tree +(`c17df59c`, a snapshot of the `q69` lane's worktree — it carries +`tests/test_token_meter_hybrid.py`). It is a **mid-conflict snapshot**: it still +contains git's own markers. The complete set of lines it holds that +`c790066d` does not: + +``` +$ git diff c790066d 231979c1 | grep '^+' | grep -v '^+++' ++<<<<<<< HEAD ++||||||| parent of 1f59edc (docs: README section + DONE-NOTE for the tool-result budget and spill) ++======= ++>>>>>>> 1f59edc (docs: README section + DONE-NOTE for the tool-result budget and spill) +``` + +Four conflict markers and **no note content whatsoever**, so nothing is +recoverable from it — but the only way to know that was to find it and read it. +Note also that it is the add/add conflict the shared file's own header comment +predicted, frozen mid-resolution. + +Every mainline transition is a **pure append** (`git diff --numstat`: +`343/0`, `267/0`, `217/0`, `174/0` — zero deletions), so `c790066d` is a strict +superset of every other mainline revision. `be96dd2b` is the one blob that is +**not** on that chain. + +## What happened, in two distinct failures + +**1. The revert (visible).** `e9ac159`, *"revert: unproven default-off features +per merge policy (wins only) (#30)"*, restored the root file to `ecc7cccb` — +`x7p`'s note alone. Four lanes' notes (`x1r`, `2o9`, `7k2`, `jnt`) left `main` +as collateral of a *code* revert, because the notes lived in a file the feature +PRs also touched. `git ls-tree -r origin/main | grep -i done-note` returns only +that root file, so those four exist nowhere else on `main`. + +**2. The silent overwrite (invisible).** `rb1` committed its note at `56270a1` +on top of a tree whose root file held `x7p` + `x1r` (`4b822d72`). Meanwhile the +`2o9` lane branched from the *same* base and appended its own note, producing +`0268ee62`. `2o9`'s line landed; `rb1`'s did not. Because both are ordinary +writes to the same path from a common ancestor, git raised **nothing**: + +``` +$ git merge-base --is-ancestor 56270a1 origin/main; echo $? +1 # rb1's note has never been on main +$ git for-each-ref --contains 56270a1 --format='%(refname)' +refs/heads/lane/rb1-rebase-conflicted-prs +refs/remotes/origin/lane/rb1-rebase-conflicted-prs +``` + +`rb1`'s work (rebasing and landing PRs #21 and #24) *is* on `main`. Only its +record of that work was dropped. This is `kez`'s exact shape, one repo over. + +## Recovery — provenance and proof + +| recovered file | source blob | slice | +|---|---|---| +| `docs/lanes/x7p-protected-tool-results-bug/DONE-NOTE.md` | `c790066d` | lines 19–323 | +| `docs/lanes/x1r-tool-result-budget/DONE-NOTE.md` | `c790066d` | lines 326–651 | +| `docs/lanes/2o9-clear-at-least/DONE-NOTE.md` | `c790066d` | lines 654–916 | +| `docs/lanes/7k2-summary-call-fork/DONE-NOTE.md` | `c790066d` | lines 919–1132 | +| `docs/lanes/jnt-fork-prefix-capture/DONE-NOTE.md` | `c790066d` | lines 1135–1305 | +| `docs/lanes/rb1-rebase-conflicted-prs/DONE-NOTE.md` | `be96dd2b` | lines 652–844 | + +For each lane the **richest** blob containing its note is used, so every note is +its final revision, not an earlier draft. + +**Round-trip proof that the split is lossless.** The splitter captures each +`---` separator verbatim and reassembles index + separators + bodies; the result +must hash to the original object: + +``` +$ python3 docs/lanes/sqh-context-simple-note-loss/recover_root_done_notes.py --report +round-trip be96dd2b: OK (byte-identical) +round-trip c790066d: OK (byte-identical) +``` + +Not one byte was dropped, edited or reflowed. The index table and the file's +header comment — the only part of the shared file that belongs to no single lane +— are preserved verbatim in `RECOVERED-index-header.md` beside this audit, and +restated as a live index in `docs/lanes/README.md`. + +**Cross-check against the standalone revisions.** `x7p`'s and `x1r`'s notes also +existed as standalone root blobs before the sharing began. The recovered files +differ from those blobs by **trailing blank lines only** (`x7p`: +1 line, +`x1r`: +2), which is the padding the shared file inserted above each `---`: + +``` +$ diff <(git cat-file -p ecc7cccb) docs/lanes/x7p-protected-tool-results-bug/DONE-NOTE.md +304a305 +> +``` + +## Unrecoverable + +**None.** Stated as a positive claim with its bound: every revision the root path +ever held is one of the ten blobs above; all ten are reachable from a ref in this +repo; every authored note in every one of them is now filed under the lane that +wrote it. Two independent completeness checks back this: + +* `git log --all --full-history -- DONE-NOTE.md` → 20 commits → 10 reachable blobs. +* A sweep of **every repo-root tree in the object store** + (`git cat-file --batch-all-objects`) → those 10 plus the unreachable + `231979c1` = **11**, and no twelfth. `git fsck --lost-found` independently + reports 3 dangling commits and 6 dangling trees; the only root-note blobs they + carry are `ecc7cccb`, `4b822d72` and `231979c1`, all accounted for. No + orphaned lane note exists in the object store. + +**Bound:** this covers what git can see in *this* repo. A lane that wrote a note +and never committed it is outside git's reach and outside this audit. Of the ten +lanes that targeted this repo (`manifest.tsv`), `q69` and `l8` committed no +DONE-NOTE anywhere; that is an absence of evidence, not a recovered loss, and it +is reported as such rather than counted as damage. + +## One adjacent finding, not fixed here + +`origin/lane/pmt-fork-span-predicate` carries +`probes/pmt-fork-span-predicate/DONE-NOTE.md` (blob `37b8fc5e`) — a per-lane +note, but under a `probes/` directory that exists in no other lane of this repo. +`artifact-path/v1` (item `6x4`) resolves this repo to `docs/lanes//`, so +`tools/check_done_note_placement.py` will flag that path if that branch merges. +That is the guard working as designed; it is called out in the PR body so it is +not a surprise. `pmt`'s note is on its own branch and is **not** at risk — it is +outside the root-file collision this item covers, and moving it belongs to that +lane's PR, not this one. + +## Why it stayed silent, and what changed + +The root file was **structurally unable to raise an alarm**: two lanes writing +the same path is not a conflict, and a revert of a code PR that also touched that +file is an ordinary, correct revert. Three changes, in the order that matters: + +1. **The shared path is gone.** Root `DONE-NOTE.md` is deleted in this PR. The + failure mode has no surface left to occur on. +2. **The instruction already points elsewhere.** `artifact-path/v1` names + `docs/lanes//` for this repo, so new lanes are told a valid path — the + ambiguity that produced six root writes is closed. +3. **A check now watches, *in this repo*.** + `tools/check_done_note_placement.py`, run by `tests/test_done_note_placement.py` + under plain `pytest` (this repo has no CI workflow and no `run_tests.sh`, so + pytest is the build). It fails on a root `DONE-NOTE.md` present, tracked, or + added/modified on a branch, and on two lanes' notes concatenated into one + file. It has no environment-variable bypass, and deleting the root file is + explicitly allowed. Fail-before / pass-after is **proven on scratch repos** in + the test module, not asserted. + +Change 3 is the one that would have caught this on day one — and the reason it +did not is that `kez`'s guard was added only to the evals repo. That is the +generalisable lesson: **a guard that lives in one repo does not protect the nine +others the same lanes write to.** diff --git a/docs/lanes/sqh-context-simple-note-loss/RECOVERED-index-header.md b/docs/lanes/sqh-context-simple-note-loss/RECOVERED-index-header.md new file mode 100644 index 0000000..460771f --- /dev/null +++ b/docs/lanes/sqh-context-simple-note-loss/RECOVERED-index-header.md @@ -0,0 +1,17 @@ + + +# Lane notes index + +| item | subject | +|---|---| +| `model_performance-x7p` | `protected_tool_results=0` protected ALL tool results (negative-slice bug) | +| `model_performance-x1r` | tool-result budget (token-denominated, head+tail, per-tool) + spill-to-disk | +| `model_performance-2o9` | `clear_at_least` — a worth-the-rebuild predicate in front of compaction (+ summary shrink guard) | +| `model_performance-7k2` | `summary_call_mode` — cache-safe forking of the summarization call | + +--- diff --git a/docs/lanes/sqh-context-simple-note-loss/recover_root_done_notes.py b/docs/lanes/sqh-context-simple-note-loss/recover_root_done_notes.py new file mode 100644 index 0000000..8344374 --- /dev/null +++ b/docs/lanes/sqh-context-simple-note-loss/recover_root_done_notes.py @@ -0,0 +1,208 @@ +#!/usr/bin/env python3 +"""Recover every lane DONE-NOTE that was ever written into the repo-root +``DONE-NOTE.md`` of amplifier-module-context-simple, and split each one back +into its own ``docs/lanes//DONE-NOTE.md``. + +Method (item ``model_performance-sqh``; ported from ``model_performance-kez``'s +proven method in the evals repo, ``probes/kez-done-note-collision/AUDIT.md``): + +1. Walk ``git log --all --full-history -- DONE-NOTE.md``. ``--full-history`` + matters: plain ``git log`` prunes the losing side of a merge, which is what + made this file look like an ordinary short-history file. +2. Resolve every one of those commits to a blob; de-duplicate. +3. In each blob, find the *authored* notes -- lines matching + ``^# DONE-NOTE\\b.*model_performance-``. A bare mention of an item id in + prose is NOT authorship; counting mentions is what produced the "8 lanes" + estimate this lane was asked to verify. +4. Take, for each lane, the richest blob that contains its note, and slice the + note out on exact line boundaries. +5. Prove the slice is lossless: re-concatenating index + separators + bodies + must reproduce the original blob's object hash byte for byte. + +Run from the repo root:: + + python3 docs/lanes/sqh-context-simple-note-loss/recover_root_done_notes.py --report + python3 docs/lanes/sqh-context-simple-note-loss/recover_root_done_notes.py --write +""" + +from __future__ import annotations + +import argparse +import re +import subprocess +import sys +from pathlib import Path + +HEADING_RE = re.compile(r"^# DONE-NOTE\b.*?model_performance-([a-z0-9]+)", re.IGNORECASE) + +# Lane id -> lane directory name. Every value here is a real branch name in +# this repo (``git for-each-ref refs/heads/lane refs/remotes/origin/lane``), +# never invented: the artifact-path/v1 rule says is the lane id string +# that the branch, the worktree and the marker path already use. +LANE_DIRS = { + "x7p": "x7p-protected-tool-results-bug", + "x1r": "x1r-tool-result-budget", + "2o9": "2o9-clear-at-least", + "7k2": "7k2-summary-call-fork", + "jnt": "jnt-fork-prefix-capture", + "rb1": "rb1-rebase-conflicted-prs", +} + + +def git(*args: str) -> str: + return subprocess.run( + ["git", *args], check=True, capture_output=True, text=True + ).stdout + + +def git_bytes(*args: str) -> bytes: + return subprocess.run(["git", *args], check=True, capture_output=True).stdout + + +def hash_object(data: bytes) -> str: + return subprocess.run( + ["git", "hash-object", "--stdin"], input=data, capture_output=True, check=True + ).stdout.decode().strip() + + +def distinct_root_blobs() -> list[tuple[str, str, str]]: + """(blob, first_commit_seen, subject) for every distinct blob the root path held. + + Two passes, because neither alone is complete: + + * ``git log --all --full-history`` gives the *reachable* history and, with + it, the commit each blob first appeared in. + * A sweep of **every tree in the object store** then catches blobs that are + no longer reachable from any ref -- a deleted worktree, an abandoned + rebase, a mid-conflict snapshot. Without this pass the enumeration + silently under-counts, which is exactly the failure this item exists to + stop. Such blobs are labelled ``(unreachable object)``. + """ + seen: dict[str, tuple[str, str]] = {} + for c in git("log", "--all", "--full-history", "--format=%H", "--", "DONE-NOTE.md").split(): + try: + blob = git("rev-parse", f"{c}:DONE-NOTE.md").strip() + except subprocess.CalledProcessError: + continue # this commit deleted the path + if blob not in seen: + seen[blob] = (c, git("log", "-1", "--format=%h %s", c).strip()) + + trees = [ + line.split()[0] + for line in git("cat-file", "--batch-all-objects", "--batch-check=%(objectname) %(objecttype)").splitlines() + if line.endswith(" tree") + ] + for t in trees: + try: + entries = git("ls-tree", t) + except subprocess.CalledProcessError: + continue + names = {} + for line in entries.splitlines(): + fields = line.split() + if len(fields) >= 4: + names[fields[3]] = (fields[1], fields[2]) + # only a REPO-ROOT tree counts: a per-lane directory also holds a file + # called DONE-NOTE.md, and that one is correctly placed. + if "DONE-NOTE.md" in names and "pyproject.toml" in names: + kind, blob = names["DONE-NOTE.md"] + if kind == "blob": + seen.setdefault(blob, ("", "(unreachable object)")) + return [(b, c, s) for b, (c, s) in seen.items()] + + +def authored_notes(text: str) -> list[tuple[int, str]]: + """[(1-based heading line, lane id)] for the notes actually authored in *text*.""" + out = [] + for i, line in enumerate(text.splitlines(), start=1): + m = HEADING_RE.match(line) + if m: + out.append((i, m.group(1).lower())) + return out + + +def split_blob(text: str) -> tuple[str, list[tuple[str, str, str]]]: + """Split a shared root file into (index_prefix, [(lane, separator, body), ...]). + + The separator above each note is found by scanning back from the heading to + the nearest ``---`` rule and is captured **verbatim** rather than assumed to + be a fixed number of lines -- the blank-line padding is not uniform across + this file's history. Bodies are sliced on exact line boundaries, so + ``index_prefix + sum(separator + body)`` reproduces the input byte for byte. + """ + lines = text.splitlines(keepends=True) + heads = authored_notes(text) + if not heads: + return text, [] + + def sep_start(heading_idx0: int) -> int: + """0-based index of the ``---`` rule immediately above a heading.""" + for j in range(heading_idx0 - 1, -1, -1): + if lines[j].rstrip("\n") == "---": + return j + return heading_idx0 # no rule found: empty separator + + starts0 = [h - 1 for h, _ in heads] + seps0 = [sep_start(s) for s in starts0] + index_prefix = "".join(lines[: seps0[0]]) + parts = [] + for idx, (start0, (_, lane)) in enumerate(zip(starts0, heads)): + end0 = seps0[idx + 1] if idx + 1 < len(heads) else len(lines) + separator = "".join(lines[seps0[idx] : start0]) + parts.append((lane, separator, "".join(lines[start0:end0]))) + return index_prefix, parts + + +def main() -> int: + ap = argparse.ArgumentParser() + ap.add_argument("--write", action="store_true", help="write docs/lanes//DONE-NOTE.md") + ap.add_argument("--report", action="store_true", help="print the enumeration") + args = ap.parse_args() + + blobs = distinct_root_blobs() + rows = [] + for blob, commit, subject in blobs: + text = git_bytes("cat-file", "-p", blob).decode() + lanes = [lane for _, lane in authored_notes(text)] + rows.append((blob, len(text.encode()), lanes, subject)) + rows.sort(key=lambda r: r[1]) + + if args.report or not args.write: + print(f"distinct blobs the root DONE-NOTE.md ever held: {len(rows)}") + for blob, size, lanes, subject in rows: + print(f" {blob[:8]} {size:>6}B authors={lanes} {subject}") + every = sorted({l for _, _, lanes, _ in rows for l in lanes}) + print(f"distinct lane notes ever at the root path: {len(every)} {every}") + + # richest blob per lane + best: dict[str, tuple[str, str]] = {} + for blob, size, lanes, _ in rows: # ascending size -> last write wins + text = git_bytes("cat-file", "-p", blob).decode() + _, parts = split_blob(text) + for lane, _sep, body in parts: + best[lane] = (blob, body) + + # round-trip proof for each source blob we actually take content from + proofs = [] + for blob in sorted({b for b, _ in best.values()}): + text = git_bytes("cat-file", "-p", blob).decode() + index_prefix, parts = split_blob(text) + rebuilt = index_prefix + "".join(sep + body for _, sep, body in parts) + ok = hash_object(rebuilt.encode()) == blob + proofs.append((blob, ok)) + print(f"round-trip {blob[:8]}: {'OK (byte-identical)' if ok else 'MISMATCH'}") + if not ok: + return 1 + + if args.write: + root = Path(git("rev-parse", "--show-toplevel").strip()) + for lane, (blob, body) in sorted(best.items()): + d = root / "docs" / "lanes" / LANE_DIRS[lane] + d.mkdir(parents=True, exist_ok=True) + (d / "DONE-NOTE.md").write_text(body) + print(f"wrote docs/lanes/{LANE_DIRS[lane]}/DONE-NOTE.md from {blob[:8]}") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/docs/lanes/x1r-tool-result-budget/DONE-NOTE.md b/docs/lanes/x1r-tool-result-budget/DONE-NOTE.md new file mode 100644 index 0000000..1075121 --- /dev/null +++ b/docs/lanes/x1r-tool-result-budget/DONE-NOTE.md @@ -0,0 +1,326 @@ +# DONE-NOTE — W3-3 / `model_performance-x1r` + +**Fix the tool-result budget in context-simple** (250 chars head-only → token +budget, head+tail, per-tool), then spill the truncated middle to disk. + +| | | +|---|---| +| Repo | `amplifier-module-context-simple` | +| Branch | `lane/x1r-tool-result-budget` (forked from `c6dfbba`, `main`) | +| Draft PR | microsoft/amplifier-module-context-simple#21 (DRAFT — do not merge) | +| Tests | **151 green** (103 pre-existing, unchanged + 48 new) | +| Spend | **$0.00** — no API calls, no DTU, no containers, no infrastructure created | +| Default behavior | **byte-identical**, proven by external stash-compare (below) | + +--- + +## 1. Step 0 — the free measurement that decides how much this is worth + +The spec's own instruction: *"If our tool-result share is 15%, phase B is not +worth building and this spec should stop at phase A."* The magnitude case rested +on a practitioner's worked example from somebody else's workload (5 tool results += 81% of a context). So it got measured on ours first. + +Script: `.amplifier/evaluation/treatment-validation/20260902-x1r/step0_tool_result_share.py` +Output: `.../20260902-x1r/step0-results.json` + +Source: every `transcript.jsonl` under two existing capture roots. Counted in +**characters** — the unit this module's estimator actually works in +(`len(str(msg)) // 4`). + +| | `20260902-t0t1` | `20260901-cadence` | +|---|---|---| +| sessions / tool results | 5 / 750 | 12 / 1,729 | +| **tool-result share of transcript chars** | **46.4%** | **47.3%** | +| p50 / p90 / p99 / max (chars) | 412 / 7,904 / 31,751 / 52,319 | 467 / 7,904 / 31,744 / 43,720 | +| over today's 250-char budget | 488 (65%) | 1,096 (63%) | +| chars discarded by today's budget | 1,524,013 (**91%** of tool-result content) | 3,082,300 (**90%**) | +| over 50,000 bytes | **1** | **0** | + +**(knob moved: none — observation) · (model family: n/a, corpus measurement) · +(confidence: measured, n=17 sessions / 2,479 tool results) · (evidence: +`.../20260902-x1r/step0-results.json`)** + +Two conclusions: + +1. **~47% ≫ 20%. Phase B is justified**, which is why this lane shipped spill and + not just the budget fix. +2. **deepseek's shipped 50,000-byte spill threshold would fire once in 2,479 + results on our workload.** The mass is in the 400–32,000 char band. The + mechanism is worth copying; that constant is not. This contradicts the + reference implementation the spill design otherwise follows, and it is the + single most transferable number in this note. + +Caveats, stated: a transcript is the **full history**, not the compacted wire +view, so this is the share of what compaction *sees* — an upper bound on the wire +share in a run that compacts. Token figures are the module's own estimator, not +provider-billed tokens. `p90 = 7,904` is identical in both roots, which suggests a +common fixed-size output rather than a coincidence; not investigated. + +--- + +## 2. What shipped + +Five flags, **all inert by default**: + +| flag | default | effect | +|---|---|---| +| `tool_result_budget_tokens` | `None` | Token-denominated budget. `None` = legacy `truncate_chars` path. | +| `tool_result_shape` | `"head"` | `"head_tail"` splits the budget and keeps both ends with `...[N chars omitted]...`. | +| `tool_result_budget_by_tool` | `{}` | Per-tool token budgets by tool name; beats the global budget. | +| `tool_result_exempt_tools` | `[]` | Never truncated (skill-type outputs). | +| `tool_result_spill_dir` | `None` | Full original written to a content-addressed file; pointer in the replacement text. | + +Precedence per result: **per-tool → global → legacy `truncate_chars`**. + +Roughly 300 LOC of module change against the spec's ~80 (phase A) + ~180 +(phase B) estimate — the overage is comment density and per-knob validation, not +extra mechanism. + +### Defaults rationale + +- **`tool_result_budget_tokens` defaults to `None`, not `62`.** The spec says 62 + (= today's 250 chars). But `truncate_chars` is an existing shipped knob: + someone running `truncate_chars: 500` today would have been silently reset to + 248 chars by a hard 62 default. `None` means "legacy path, whatever + `truncate_chars` says", which is byte-identical for **every** existing + configuration rather than only the default one. This is a deliberate deviation + from the spec, in the safer direction. +- **Recommended values are documented, not shipped.** The README's starter config + (2,000 tokens global, `head_tail`, per-tool map, `load_skill` exempt) is + conservative relative to codex's 10,000. It is *not* a default, because nothing + here has been evaluated against a model yet. +- **Per-tool numbers are transcribed, not measured.** They come from a + specification that publishes zero measurements anywhere, rescaled to the size + distribution in §1. The README says so at the point of use. +- **Tokens, not chars**, because chars/token constants are tokenizer-version + specific and drift (published up to 1.35×, observed up to 1.47× on technical + content) — a char budget silently changes meaning across a model version. The + conversion constant is `4`, deliberately the *same* constant the module's own + estimator uses, so budget and accounting cannot drift apart. +- **`head_tail` keeps the marker, not just the tail.** A model must never reason + from a truncated result without knowing it is truncated. + +### The one design constraint that shaped everything + +`_apply_sticky_decisions` re-derives the replacement text for **every** +sticky-truncated message on **every** request. So the replacement must be a pure +function of content + config. + +The consequence for spill: **the pointer is emitted whether or not the write +succeeded.** If the pointer tracked write success, one transient disk error would +change the bytes of an already-sent message, and under a grow-only prompt cache +every prefix mutation is a full cold rebuild. A dangling pointer is visible and +recoverable. A silently mutated prefix is neither. Write failures log a warning; +`tests/test_tool_result_budget.py::test_spill_write_failure_still_emits_stable_pointer` +pins it. + +Spill paths are content-addressed (`tool-result--.txt`), so +writes are idempotent across repeated requests and across a resumed session. +Write-then-rename, so a reader never sees a half-written file. + +--- + +## 3. Evidence + +### 3.1 Byte-identity — the defaults-are-a-no-op claim + +**Method (external oracle).** `.../20260902-x1r/byte_identity_harness.py` imports +whatever `amplifier_module_context_simple` is on `sys.path`, so neither side +defines its own baseline. 8 scenarios: light / heavy / aggressive pressure; +`truncate_chars` ∈ {10, 60, 250, 5000}; notice on and off; a view every third +turn **plus two consecutive views on turn 5** (the repeated call is what would +expose non-idempotent re-derivation). Canonical JSON of every returned message. +Only `metadata.timestamp` is normalized. + +``` +pre-change (c6dfbba, 2,648 lines, flags absent) sha256 76ee9d3f…3d6a467f +post-change (lane/x1r-tool-result-budget) sha256 76ee9d3f…3d6a467f +3,398,618 bytes IDENTICAL — PASS +``` + +**Non-vacuity:** the dump contains 76 `[truncated:` occurrences — the path is +really exercised. **Negative control:** the same harness with the treatment forced +on yields `4e59f380…` — a different hash, so the harness can see changes when +there are any. + +Artifacts: `byte-identity-PRE.json`, `byte-identity-POST.json`, +`byte-identity-TREATMENT.json`, `.log` files alongside. + +The unit suite pins the same claim from the opposite direction: +`test_default_config_replacement_text_is_byte_identical` asserts the exact legacy +string against an oracle **transcribed from the pre-change source**, not computed +by the new code, plus a second literal spelling so re-baselining the oracle alone +cannot silently pass. + +### 3.2 Tail retention — mechanism demonstration, no model, no spend + +`.../20260902-x1r/tail_retention_demo.py` → `tail-retention-demo.json`. +Four synthetic workloads (`pytest`, `grep`, `git log`, build output) whose answer +sits in the **last line** — the tool list enumerated *before* the run, as the gate +requires. + +| arm | truncated results | tail present | rate | +|---|---|---|---| +| control (shipped defaults, 250 chars, head) | 27 | 0 | **0%** | +| **budget-neutral** (62 tok = 248 chars, `head_tail`) | 28 | 28 | **100%** | +| 4× budget (250 tok = 1,000 chars, `head_tail`) | 1 | 1 | 100% | + +**(knob moved: `tool_result_shape`) · (model family: none — mechanical, no LLM) · +(confidence: measured, n=4 workloads × 30 turns per arm) · (evidence: +`.../20260902-x1r/tail-retention-demo.json`)** + +Row 2 is the clean A/B: **same bytes kept, different shape**, identical sticky +level per tool (4/2/3/3 in both arms). 0% → 100% tail retention at no budget cost. + +**Honest negative in row 3, and it is load-bearing.** Raising the per-result +budget *without* raising `target_usage`/`max_tokens` **trades truncation for +removal**: a bigger budget sheds fewer tokens per truncation, so the ladder +escalates past the truncation rungs into message removal — strictly more lossy. +Three of four workloads went from 11–15 truncated results to **zero, removed +instead**. Anyone tuning this must raise budget and target together, or measure +what they actually got. This is the first thing the follow-up eval should +control for. + +This is a mechanism demonstration, **not** the eval. No model was involved, so it +says nothing about whether an agent *uses* the tail. + +### 3.3 Test suite + +151 green. The 48 new tests cover: byte identity (literal + end-to-end + custom +`truncate_chars`); token budget arithmetic; head/tail split, exact omission +count, and the `content[-0:]`-returns-everything trap; per-tool resolution via +all three sources (harvested `tool_call_id`, `name` field, `metadata.tool_name`) +and both tool-call shapes (OpenAI `function`, Anthropic-ish); exemption including +"never counted as truncated"; spill write / idempotence / content-addressing / +**write-failure byte-stability** / lazy directory creation; **tool-pair atomicity +with every knob enabled**; `_seq` prefix stability and 5× request idempotence +with `head_tail` + spill on; `set_messages` map rebuild; `clear()` reset; config +validation fallbacks for all five flags; and `mount()` plumbing both ways. + +Twelve of them carry an explicit **"test must actually truncate/compact/spill"** +assertion, because a compaction test that escalates to level 8 removes every tool +result and then passes vacuously. That is exactly what the first draft of two of +these tests did. + +--- + +## 4. Discovered, filed, not fixed + +**`model_performance-x7p`** (filed `discovered-from` this item): +`protected_tool_results=0` protects **all** tool results, not none — +`tool_result_indices[-0:]` is the whole list (`__init__.py:1407`, `:1510`, +`:1577`). Compaction then skips every truncation rung and escalates straight to +removal. + +**(confidence: measured** — reproduced in a scratch harness: with +`protected_tool_results=0`, 40 turns × 2,000-char results reached sticky level 8 +with **0** truncations; `protected_tool_results=1` on the identical workload +truncated normally.**)** + +Not fixed here: the default is 5, so no shipped configuration hits it, and +changing it *is* a behavior change for anyone currently setting 0. ~3 lines plus a +regression test. The new tests avoid the trap and say why in a comment. + +--- + +## 5. Deviations from the spec, and why + +1. **Default is `None`, not `62`** — see §2. Protects existing `truncate_chars` + overrides. +2. **Spill lives in `context-simple`, not a new `tool-result-spill` module.** The + item's own task text asks for "spill-to-disk for the truncated middle with a + stable pointer line", which is a compaction-time operation on the truncated + middle — it shares the head/tail code and the `_seq` machinery. **This forfeits + the spec's class-A property**: because the content is already in the + conversation, spilling still shrinks the request, so it is still a boundary and + still a cold rebuild under a grow-only cache. It buys **recoverability**, not + free context. A post-execute hook that intercepts results *before* they enter + the conversation is the class-A design and remains unbuilt. Recorded plainly + rather than claimed. +3. **No cleanup sweep.** deepseek ships a 30-day startup sweep with symlink and + ownership guards. Nothing here deletes anything, ever — deliberately, since a + resumed or forked session may still hold an older pointer. The caller owns the + directory lifecycle; README says so and recommends a session-scoped dir. +4. **No `read`-result exemption by default.** deepseek exempts reads to prevent + `read → spill → read`. That loop cannot form here (spill happens at compaction + time, not tool-execute time). `tool_result_exempt_tools` is the seam if this + ever moves to a post-execute hook. +5. **No model-settable per-call budget.** codex lets the model raise its own limit + to 262,144 per call. That needs a tool-schema surface this module does not own. +6. **No line-based cap.** The spec's "chars before lines" concern is satisfied + structurally: the gate is a pure char count and no line cap exists in this + path, so the "2 lines × 10MB" failure mode is impossible rather than merely + unlikely. Zero LOC. +7. **No eval.** Explicitly out of scope for this lane ($0 authority). Spec below. + +--- + +## 6. Follow-up eval spec (the item this lane does NOT do) + +**Prerequisite that must be settled first:** the budget↔removal interaction in +§3.2. An arm that raises the budget while holding `target_usage` fixed is not +measuring "bigger budget", it is measuring "truncation replaced by removal". Fix +the design before spending. + +**Arms** (n≥3 each, one variable at a time, per the program's own rule): + +| arm | config | +|---|---| +| `control` | shipped defaults (byte-identical to today) | +| `shape_only` | `tool_result_budget_tokens: 62`, `tool_result_shape: "head_tail"` — **budget-neutral**, isolates shape | +| `budget_only` | `tool_result_budget_tokens: 2000`, shape `"head"`, `target_usage` raised to hold boundary count constant | +| `per_tool` | `budget_only` + `tool_result_budget_by_tool` + `tool_result_exempt_tools: ["load_skill"]` | +| `spill` | `per_tool` + `tool_result_spill_dir` | + +**Pre-register before spending** (a gate written after seeing the result is not a +gate; check each for vacuity): + +- **G-TRB-TAIL** — on a tool list enumerated **before** the run (pytest, grep, + git log, build output), the tail is present in the model-visible content of + 100% of truncated results; control 0%. *Already demonstrated mechanically + (§3.2); the eval's job is whether the agent* uses *it.* +- **G-TRB-RECALLS** — redundant re-calls of the same tool with identical + arguments fall vs control. **The real quality signal**, and the one that can + fail honestly. +- **G-TRB-COST** — cost does not rise. Watch the boundary count specifically: + §3.2 says a larger budget pushes work from truncation into removal, and prior + work established that boundary count, not the mechanism, is what moves cost + (a comparable feature measured +84% boundaries → +83% cost). +- **G-SPILL-APPEND** — no request contains a tool result a previous request + contained in longer form; `ID_ONLY = 0`. +- **G-SPILL-TOKENS** — tool-result input tokens fall by ≥ §1's predicted share, + ±20%. §1 is measured *before* the treatment, so it cannot be fitted afterwards. +- **G-SPILL-BOUNDARIES** — boundary count does not rise. +- **G-SPILL-RECOVERY** — **record as VACUOUS on S5-CRAC** and defer. S5 never + needs to recover anything (40/40 constraints, 20/20 post-compaction in every + run of every arm across six probes). It needs the discriminating scenario from + `model_performance-cb2`. +- **G-ANTH-GUARDRAIL** — required for any treatment lane: Anthropic re-warm ≤1 + request on raw wire fields, byte-stable system prompt, zero `ID_ONLY`. +- **S3 quality** — non-regression observation only. Six cells already sit at S3 + median 100 and dial-to-dial differences may be inside grader noise. + +**Cheapest first experiment:** `shape_only` vs `control`. One variable, no budget +change, therefore no boundary-count confound, and §3.2 already says the mechanism +fires. If G-TRB-RECALLS does not move there, it will not move anywhere. + +--- + +## 7. Infrastructure and spend + +**$0.00.** No API calls, no DTU, no Gitea, no containers. Nothing registered in +the infra ledger because nothing was created; nothing to tear down. All evidence +is local CPU work over existing capture roots. + +Artifacts written (outside the module repo, under the authorized capture path +`.amplifier/evaluation/treatment-validation/20260902-x1r/`): +`step0_tool_result_share.py`, `step0-results.json`, `byte_identity_harness.py`, +`byte-identity-{PRE,POST,TREATMENT}.json` + `.log`, `tail_retention_demo.py`, +`tail-retention-demo.json`. + +No PII, no team-internal data, no individual attribution in any output. Spill +paths are the only new content class in a transcript; they are caller-configured +and contain no absolute path this module invents. + + diff --git a/DONE-NOTE.md b/docs/lanes/x7p-protected-tool-results-bug/DONE-NOTE.md similarity index 99% rename from DONE-NOTE.md rename to docs/lanes/x7p-protected-tool-results-bug/DONE-NOTE.md index ecc7ccc..ca71a89 100644 --- a/DONE-NOTE.md +++ b/docs/lanes/x7p-protected-tool-results-bug/DONE-NOTE.md @@ -302,3 +302,4 @@ Nothing blocking. Two observations handed on rather than acted on: is now well-defined at 0 and negative, but e.g. `target_usage=5.0` or `truncate_chars=-1` remain undefined. Worth one item if anyone wants the constructor to fail loud; not filed, because it is a design call, not a defect. + diff --git a/tests/test_done_note_placement.py b/tests/test_done_note_placement.py new file mode 100644 index 0000000..251ee4d --- /dev/null +++ b/tests/test_done_note_placement.py @@ -0,0 +1,150 @@ +"""The repo-root ``DONE-NOTE.md`` guard, wired into this repo's own test suite. + +Item ``model_performance-sqh``. ``tools/check_done_note_placement.py`` is the +checker; this module makes it run under plain ``pytest`` (this repo has no CI +workflow and no ``run_tests.sh``, so pytest *is* the build), and pins its +fail-before / pass-after behaviour on scratch repos rather than asserting it. +""" + +from __future__ import annotations + +import importlib.util +import subprocess +from pathlib import Path + +import pytest + +REPO = Path(__file__).resolve().parent.parent +CHECKER = REPO / "tools" / "check_done_note_placement.py" + + +def _load_checker(): + spec = importlib.util.spec_from_file_location("check_done_note_placement", CHECKER) + assert spec and spec.loader + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +check = _load_checker().check + + +def _scratch_repo(tmp_path: Path) -> Path: + repo = tmp_path / "scratch" + repo.mkdir() + run = lambda *a: subprocess.run(["git", "-C", str(repo), *a], check=True, capture_output=True) + run("init", "-q", "-b", "main") + run("config", "user.email", "t@example.invalid") + run("config", "user.name", "test") + (repo / "README.md").write_text("scratch\n") + run("add", "-A") + run("commit", "-qm", "init") + return repo + + +def test_checker_exists_and_has_no_env_bypass(): + src = CHECKER.read_text() + assert "environ" not in src, "the guard must not have an environment-variable bypass" + + +def test_this_repo_is_clean(): + """The real repo: no root DONE-NOTE.md, every lane note single-author.""" + problems = check(REPO) + assert problems == [], "\n".join(problems) + + +def test_this_repo_has_the_recovered_lane_notes(): + """Regression pin for the notes recovered from git history (item sqh).""" + for lane in ( + "x7p-protected-tool-results-bug", + "x1r-tool-result-budget", + "2o9-clear-at-least", + "7k2-summary-call-fork", + "jnt-fork-prefix-capture", + "rb1-rebase-conflicted-prs", + ): + note = REPO / "docs" / "lanes" / lane / "DONE-NOTE.md" + assert note.is_file(), f"missing recovered note: {note}" + assert note.read_text().lstrip().startswith("# DONE-NOTE"), lane + + +def test_fails_on_a_root_done_note(tmp_path): + """FAIL-BEFORE: a root DONE-NOTE.md is caught.""" + repo = _scratch_repo(tmp_path) + (repo / "DONE-NOTE.md").write_text("# DONE-NOTE - model_performance-aaa\n") + problems = check(repo) + assert problems, "a repo-root DONE-NOTE.md must be a violation" + assert any("repo root" in p for p in problems), problems + + +def test_passes_once_the_note_moves_to_its_lane_dir(tmp_path): + """PASS-AFTER: the same content at docs/lanes// is clean.""" + repo = _scratch_repo(tmp_path) + d = repo / "docs" / "lanes" / "aaa-some-lane" + d.mkdir(parents=True) + (d / "DONE-NOTE.md").write_text("# DONE-NOTE - model_performance-aaa\n") + assert check(repo) == [] + + +def test_fails_on_two_lanes_concatenated_into_one_file(tmp_path): + """The shape that made the original loss invisible: one file, two authors.""" + repo = _scratch_repo(tmp_path) + d = repo / "docs" / "lanes" / "aaa-some-lane" + d.mkdir(parents=True) + (d / "DONE-NOTE.md").write_text( + "# DONE-NOTE - model_performance-aaa\n\nbody\n\n---\n\n" + "# DONE-NOTE - model_performance-bbb\n\nbody\n" + ) + problems = check(repo) + assert any("concatenated" in p for p in problems), problems + + +def test_fails_when_a_branch_adds_the_root_note(tmp_path): + """A branch that re-introduces the shared root file is caught by diff.""" + repo = _scratch_repo(tmp_path) + run = lambda *a: subprocess.run(["git", "-C", str(repo), *a], check=True, capture_output=True) + run("checkout", "-q", "-b", "lane/zzz") + (repo / "DONE-NOTE.md").write_text("# DONE-NOTE - model_performance-zzz\n") + run("add", "-A") + run("commit", "-qm", "oops") + problems = check(repo) + assert any("adds" in p or "tracked" in p for p in problems), problems + + +def test_deleting_the_root_note_on_a_branch_is_allowed(tmp_path): + """Deleting the shared file is the fix, not a violation.""" + repo = _scratch_repo(tmp_path) + run = lambda *a: subprocess.run(["git", "-C", str(repo), *a], check=True, capture_output=True) + (repo / "DONE-NOTE.md").write_text("# DONE-NOTE - model_performance-aaa\n") + run("add", "-A") + run("commit", "-qm", "shared note") + run("checkout", "-q", "-b", "lane/fix") + run("rm", "-q", "DONE-NOTE.md") + d = repo / "docs" / "lanes" / "aaa-some-lane" + d.mkdir(parents=True) + (d / "DONE-NOTE.md").write_text("# DONE-NOTE - model_performance-aaa\n") + run("add", "-A") + run("commit", "-qm", "re-home the note") + assert check(repo) == [] + + +def test_cli_entrypoint_returns_nonzero_on_violation(tmp_path): + repo = _scratch_repo(tmp_path) + (repo / "DONE-NOTE.md").write_text("# DONE-NOTE - model_performance-aaa\n") + p = subprocess.run( + ["python3", str(CHECKER), "--repo", str(repo), "--verbose"], + capture_output=True, + text=True, + ) + assert p.returncode == 1, p.stdout + p.stderr + + +def test_cli_entrypoint_returns_zero_on_this_repo(): + p = subprocess.run( + ["python3", str(CHECKER), "--verbose"], capture_output=True, text=True + ) + assert p.returncode == 0, p.stdout + p.stderr + + +if __name__ == "__main__": + raise SystemExit(pytest.main([__file__, "-v"])) diff --git a/tools/check_done_note_placement.py b/tools/check_done_note_placement.py new file mode 100644 index 0000000..45427d6 --- /dev/null +++ b/tools/check_done_note_placement.py @@ -0,0 +1,163 @@ +#!/usr/bin/env python3 +"""Fail the build if a lane note is written to the repo-root ``DONE-NOTE.md``. + +Why this exists +--------------- +Several parallel lanes each appended their DONE-NOTE to ONE shared repo-root +``DONE-NOTE.md``. Two lanes writing the same path is not a git conflict -- it +is git working correctly -- so the collision was structurally unable to raise an +alarm. Then PR #30 (``revert: unproven default-off features per merge policy``) +reverted the features and took the shared note file with them, deleting four +lanes' notes from ``main`` in one commit (``e9ac159``). A fifth lane's note +(``rb1``) never reached ``main`` at all: it was silently overwritten out of the +lineage before the revert ever happened. + +The evals repo added the same guard after item ``model_performance-kez``. That +guard was repo-local, which is exactly why this recurred here unseen. This is +the port (item ``model_performance-sqh``). + +What it checks +-------------- +1. **No repo-root ``DONE-NOTE.md``** -- present on disk, or tracked in git. +2. **No branch adds or modifies one** -- diffed against the merge-base with + ``origin/main``. *Deleting* the root file is allowed; that is the fix. +3. **No two lanes' notes concatenated into one file** -- more than one + ``# DONE-NOTE ... model_performance-`` heading in a single file is the + shape that made the original loss invisible. +4. **Lane notes live at ``docs/lanes//DONE-NOTE.md``** -- the + ``artifact-path/v1`` root resolved for this repo (item + ``model_performance-6x4``: no ``probes/``, no ``ai_working/``, so the R3 + fallback applies). + +There is deliberately **no environment-variable bypass**. + +Usage:: + + python3 tools/check_done_note_placement.py [--verbose] [--repo PATH] + +Exit code 0 = clean, 1 = violation. Also runs inside the pytest suite as +``tests/test_done_note_placement.py``. +""" + +from __future__ import annotations + +import argparse +import re +import subprocess +import sys +from pathlib import Path + +ROOT_NOTE = "DONE-NOTE.md" +LANE_NOTE_DIR = "docs/lanes" +NOTE_HEADING_RE = re.compile( + r"^# DONE-NOTE\b.*?model_performance-([a-z0-9]+)", re.IGNORECASE | re.MULTILINE +) + + +def _git(repo: Path, *args: str) -> tuple[int, str]: + p = subprocess.run( + ["git", "-C", str(repo), *args], capture_output=True, text=True + ) + return p.returncode, p.stdout.strip() + + +def check(repo: Path, verbose: bool = False) -> list[str]: + """Return a list of human-readable violations (empty == clean).""" + problems: list[str] = [] + + def say(msg: str) -> None: + if verbose: + print(f" {msg}") + + # 1. root note present on disk / tracked + if (repo / ROOT_NOTE).exists(): + problems.append( + f"{ROOT_NOTE} exists at the repo root. Lane notes belong at " + f"{LANE_NOTE_DIR}//DONE-NOTE.md -- one file per lane, so a " + f"revert of one lane's code cannot delete another lane's note." + ) + rc, out = _git(repo, "ls-files", "--error-unmatch", ROOT_NOTE) + if rc == 0 and out: + problems.append(f"{ROOT_NOTE} is tracked in git at the repo root.") + say(f"root {ROOT_NOTE}: {'PRESENT' if (repo / ROOT_NOTE).exists() else 'absent'}") + + # 2. branch adds or modifies the root note (deleting it is fine) + base = None + for ref in ("origin/main", "main"): + rc, out = _git(repo, "merge-base", "HEAD", ref) + if rc == 0 and out: + base = out + break + if base: + rc, out = _git(repo, "diff", "--name-status", f"{base}..HEAD", "--", ROOT_NOTE) + for line in out.splitlines(): + status = line.split("\t", 1)[0] + if status.startswith(("A", "M")): + problems.append( + f"this branch {'adds' if status.startswith('A') else 'modifies'} " + f"the repo-root {ROOT_NOTE} (vs {base[:8]}). Write to " + f"{LANE_NOTE_DIR}//DONE-NOTE.md instead." + ) + say(f"branch diff vs {base[:8]} for {ROOT_NOTE}: {out or 'no change'}") + else: + say("no merge-base with origin/main or main -- skipped the branch-diff check") + + # 3 + 4. every tracked DONE-NOTE.md: single-author, correct location + rc, out = _git(repo, "ls-files", "*DONE-NOTE.md") + tracked = [p for p in out.splitlines() if p] + # also consider untracked-but-present files under docs/lanes, so a lane + # catches itself before it commits + for p in sorted((repo / LANE_NOTE_DIR).glob("*/DONE-NOTE.md")): + rel = p.relative_to(repo).as_posix() + if rel not in tracked: + tracked.append(rel) + + for rel in sorted(tracked): + f = repo / rel + if not f.exists(): + continue + text = f.read_text(errors="replace") + authors = NOTE_HEADING_RE.findall(text) + if len(authors) > 1: + problems.append( + f"{rel} contains {len(authors)} lanes' notes concatenated into one " + f"file ({', '.join(authors)}). Split them: one lane per file." + ) + parts = rel.split("/") + is_lane_note = ( + len(parts) == 4 + and parts[0] == "docs" + and parts[1] == "lanes" + and parts[3] == ROOT_NOTE + ) + if not is_lane_note and rel != ROOT_NOTE: + problems.append( + f"{rel} is a DONE-NOTE outside {LANE_NOTE_DIR}// " + f"(artifact-path/v1 for this repo)." + ) + say(f"{rel}: authors={authors or ['(none)']}") + + return problems + + +def main() -> int: + ap = argparse.ArgumentParser(description=__doc__) + ap.add_argument("--repo", default=None, help="repo root (default: this file's repo)") + ap.add_argument("--verbose", action="store_true") + args = ap.parse_args() + + repo = Path(args.repo) if args.repo else Path(__file__).resolve().parent.parent + if args.verbose: + print(f"check_done_note_placement: {repo}") + problems = check(repo, verbose=args.verbose) + if problems: + print("FAIL -- DONE-NOTE placement:", file=sys.stderr) + for p in problems: + print(f" - {p}", file=sys.stderr) + return 1 + print("OK -- no repo-root DONE-NOTE.md; every lane note is single-author and in place.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) From 483fbdcbbf63010f7c8418d448daf18892e7afdb Mon Sep 17 00:00:00 2001 From: lane/sqh Date: Wed, 2 Sep 2026 18:59:56 -0700 Subject: [PATCH 2/2] test: guard against a repo-root DONE-NOTE.md, with fail-before/pass-after proof (#sqh) --- .../sqh-context-simple-note-loss/DONE-NOTE.md | 173 ++++++++++++++++++ tests/test_done_note_placement.py | 19 +- 2 files changed, 189 insertions(+), 3 deletions(-) create mode 100644 docs/lanes/sqh-context-simple-note-loss/DONE-NOTE.md diff --git a/docs/lanes/sqh-context-simple-note-loss/DONE-NOTE.md b/docs/lanes/sqh-context-simple-note-loss/DONE-NOTE.md new file mode 100644 index 0000000..49d7ea8 --- /dev/null +++ b/docs/lanes/sqh-context-simple-note-loss/DONE-NOTE.md @@ -0,0 +1,173 @@ +# DONE-NOTE — `model_performance-sqh` + +**Subject:** `kez` recurs in `amplifier-module-context-simple` — lane DONE-NOTEs +lost from the shared repo-root `DONE-NOTE.md`. Enumerate, recover, re-home, +guard. + +**Spend: $0.** No API call, no DTU, no infrastructure created, nothing to tear +down. The whole item is a git-history read plus a checker; the spend authority +was $0 and none was used. + +## Headline + +**The item's count was wrong, and the real damage has a second half nobody knew +about.** The item said *8 lanes' notes were deleted from `origin/main` by the +`#30` revert*. Measured by authorship rather than by prose mentions: + +* the repo-root path ever held **11 distinct blobs** (10 reachable + 1 that no + ref reaches) carrying **6 distinct lane notes**; +* the `#30` revert deleted **4** of them from `main` (`x1r`, `2o9`, `7k2`, + `jnt`) — not 8; +* a **5th** (`rb1`) was never on `main` at all. It was silently overwritten out + of the lineage *before* the revert, and survives only on + `origin/lane/rb1-rebase-conflicted-prs`. **This is the worse failure** — a + revert is at least visible in the log; this one produced no event at all; +* **0 unrecoverable.** All six notes are re-homed, with a byte-identical + round-trip proof. + +The four names in the item that are not in my list (`6da`, `cb2`, `wxs`, `q69`) +are not victims: `6da`/`cb2`/`wxs` are **evals-repo** lanes already handled by +`kez` and never wrote a note here, and `q69` (like `l8`) committed no DONE-NOTE +anywhere in this repo's object store. Reporting them as lost would have been a +fabricated loss. + +Full evidence: [`AUDIT.md`](AUDIT.md). + +## Deliverable ledger + +| # | deliverable | state | +|---|---|---| +| 1 | Per-blob enumeration from history, with the lane each belonged to, and a **verified** count | **DONE** — `AUDIT.md` §"The eleven blobs"; reproducible with `recover_root_done_notes.py --report`. Verified, and the item's "8" corrected to 6 notes / 4 revert-deleted. | +| 2 | DRAFT PR, **purely additive** (notes only, no reverted feature code), re-homing every recoverable note | **DONE** — see "Purely additive" below. | +| 3 | Repo-root `DONE-NOTE.md` guard, with a test, adapted to this repo's test setup | **DONE** — `tools/check_done_note_placement.py` + `tests/test_done_note_placement.py`, 10 tests, run by plain `pytest`. | +| 4 | Any unrecoverable note named explicitly | **DONE** — **none**, stated as a positive claim with its bound (`AUDIT.md` §Unrecoverable). | +| 5 | This DONE-NOTE, in the PR body | **DONE** — reproduced in the PR body verbatim. | + +## The method, and the two ways it under-counts + +Ported from `kez`'s proven method (evals repo, +`probes/kez-done-note-collision/AUDIT.md`), plus one addition this repo forced: + +1. `git log --all --full-history -- DONE-NOTE.md` → **20** commits. Plain + `git log origin/main -- DONE-NOTE.md` shows only **6**: history + simplification prunes the losing side of a merge, and `rb1` is exactly what + that prunes. +2. **Reachability is not enough.** The `--all` walk yields 10 blobs; sweeping + every *repo-root tree in the object store* yields an **11th** (`231979c1`) + that no ref reaches — a dangling snapshot of the `q69` worktree, frozen + mid-conflict, still carrying `<<<<<<<` markers. It turned out to hold no + unique note content (the complete set of lines it has that the richest blob + lacks is four conflict markers), **but the only way to know that was to find + it and read it.** An enumeration from reachable history alone would have + silently missed it and still looked complete. +3. Authorship ≠ mention. Only `# DONE-NOTE … model_performance-` headings + count. Grepping item ids over the prose is what produced "8 lanes"; it counts + every lane another lane's note happens to cite. + +## Purely additive — what this PR does and does not do + +This repo is under a **wins-only** merge policy, and `#30` reverted these +features deliberately. So, explicitly: + +* **No reverted feature code is re-introduced.** The diff touches + `docs/lanes/**`, `tools/check_done_note_placement.py` and + `tests/test_done_note_placement.py`. `amplifier_module_context_simple/` is + **untouched**; the notes describe code that is *not* being restored. +* **The one deletion is the shared root note file itself** — + `DONE-NOTE.md` → `docs/lanes/x7p-protected-tool-results-bug/DONE-NOTE.md`, + which git records as a rename. Its content is preserved in full: `x7p`'s note + moves to `x7p`'s directory, and the index/header block that belongs to no + lane is kept verbatim in `RECOVERED-index-header.md`. Nothing is dropped. + Deleting it is required by the item's acceptance criteria (`git ls-tree -r + origin/main | grep -i DONE-NOTE` must not list a root file) and is the change + that removes the collision surface altogether. + +## Evidence + +**Round-trip proof the split is lossless.** The splitter captures each `---` +separator verbatim and reassembles index + separators + bodies; the result must +hash back to the original git object: + +``` +$ python3 docs/lanes/sqh-context-simple-note-loss/recover_root_done_notes.py --report +distinct blobs the root DONE-NOTE.md ever held: 11 +distinct lane notes ever at the root path: 6 ['2o9', '7k2', 'jnt', 'rb1', 'x1r', 'x7p'] +round-trip be96dd2b: OK (byte-identical) +round-trip c790066d: OK (byte-identical) +``` + +**Test suite: 87 → 97 passing, 0 failing.** + +``` +$ uv run pytest -q +97 passed in 5.15s +``` + +The 10 new tests are the guard. Fail-before / pass-after is **proven on scratch +repos inside the test module**, not asserted: a root `DONE-NOTE.md` fails, the +same content at `docs/lanes//` passes, two lanes concatenated into one +file fails, a branch that *adds* the root file fails, and a branch that +*deletes* it passes (deleting is the fix, not a violation). + +**The guard on this working tree:** + +``` +$ python3 tools/check_done_note_placement.py --verbose + root DONE-NOTE.md: absent + branch diff vs e9ac159a for DONE-NOTE.md: D DONE-NOTE.md + docs/lanes/2o9-clear-at-least/DONE-NOTE.md: authors=['2o9'] + … one author per file, six files … +OK -- no repo-root DONE-NOTE.md; every lane note is single-author and in place. +``` + +## Decisions taken without waiting (per SCOPE-OUTS) + +1. **Artifact root.** `GOAL.md` named `probes/sqh-context-simple-note-loss/` and + described this worktree as "the evals repo". It is not — it is a checkout of + `microsoft/amplifier-module-context-simple` with a live `origin`. I used + `artifact-path/v1` (item `6x4`) resolved against *this* repo — R1 does not + apply (no top-level `probes/`), so R3 gives **`docs/lanes//`**, which is + also what the acceptance criteria name and what every other goal file for + this repo states. Creating a `probes/` tree here would have been precisely the + per-lane improvisation `6x4` measured and rejected. +2. **Lane directory naming.** `` is the full lane id (`2o9-clear-at-least`, + not `2o9`), taken from the real branch names in this repo and from + `manifest.tsv` — not invented. +3. **Which revision of each note.** For each lane, the **richest** blob + containing its note, so every note is its final revision rather than an + earlier draft. +4. **`pmt` left alone.** `origin/lane/pmt-fork-span-predicate` carries + `probes/pmt-fork-span-predicate/DONE-NOTE.md` — a per-lane note, but under a + `probes/` directory no other lane here uses. The new guard **will flag that + path if that branch merges**. That is the guard working as designed and it is + called out in the PR body so it is not a surprise; `pmt`'s note is not at risk + and moving it belongs to that lane's PR, not this one. +5. **Publication.** `GOAL.md`'s publication block assumed an eval lane with no + remote. This repo has one and the item asks for a draft PR, so publication is + `required: true` and the marker carries values read back from the remote. + +## What is NOT claimed + +* Not claimed: that the guard would have prevented the loss retroactively. It is + the **last** line of defence. Removing the shared path (this PR) and fixing the + instruction (`6x4`) are the first two, and they mean the guard should never + have anything to catch. +* Not claimed: that nothing was lost outside git. A lane that wrote a note and + never committed it is outside git's reach and outside this audit. Of the ten + lanes that targeted this repo, `q69` and `l8` committed no note — an absence of + evidence, reported as such, not counted as damage. +* Not claimed: that the reverted features should return. This PR takes no + position on the wins-only revert and restores no feature code. + +## What remains open + +1. **The generalisable one.** `kez` added this guard to the evals repo only; + that is exactly why it recurred here unseen. Nine other repos these lanes + write to still have no such check. Porting it repo-by-repo (this item is one) + does not scale — a shared, installable check would. +2. `rb1`'s note is now on `main` for the first time, but **`rb1`'s branch still + carries the root file** (blob `be96dd2b`). If that branch merges after this + PR, the guard fails it — correctly, and it will need a trivial rebase that + drops the root file. Same for `2o9`, `7k2`, `jnt`, `pmt`, `l8`, `x1r`. +3. Nothing checks that a *newly landed* lane actually wrote a note at all; + `q69` and `l8` landed none and no one noticed until this audit. diff --git a/tests/test_done_note_placement.py b/tests/test_done_note_placement.py index 251ee4d..e95d83e 100644 --- a/tests/test_done_note_placement.py +++ b/tests/test_done_note_placement.py @@ -42,9 +42,22 @@ def _scratch_repo(tmp_path: Path) -> Path: return repo -def test_checker_exists_and_has_no_env_bypass(): - src = CHECKER.read_text() - assert "environ" not in src, "the guard must not have an environment-variable bypass" +def test_checker_has_no_env_bypass(): + """No environment variable may switch this guard off. + + Checked against the parsed AST, not a substring of the source -- the module + *docstring* legitimately contains the words "environment-variable bypass". + """ + import ast + + tree = ast.parse(CHECKER.read_text()) + offenders = [] + for node in ast.walk(tree): + if isinstance(node, ast.Attribute) and node.attr in {"environ", "getenv"}: + offenders.append(node.attr) + if isinstance(node, ast.Name) and node.id in {"environ", "getenv"}: + offenders.append(node.id) + assert not offenders, f"guard reads the environment: {offenders}" def test_this_repo_is_clean():