Skip to content

fix(hygiene): keep a carried trim through interrupted and busy boundaries; review-wave-1 fixes and the findings reports - #80

Merged
alpertarhan merged 2 commits into
mainfrom
fix/review-wave-1
Sep 28, 2026
Merged

alpertarhan merged 2 commits into
mainfrom
fix/review-wave-1

Conversation

@alpertarhan

Copy link
Copy Markdown
Owner

Review wave 1: verified findings on the #76–#79 code, plus the findings reports

Four external reviews landed under docs/findings/ (first commit). Every claim acted on here was re-verified against the code and Pi 0.87.1 first; three claims were found wrong and left alone (noted below).

Fixed (second commit)

  1. Carried trim dropped on non-committing boundaries (claude B1, deepseek P1-2, muse P0-2). turn_end nulled applied at its top and restored it only on a contested boundary. Pi persists an aborted/failed response with a fresh timestamp (agent-session.js:333), so after such a turn the next request looked warm, went out untrimmed and rewrote the prefix the interrupted request had just cached. applied now lives until a boundary commits it, unchangedSince fails, the session changes, or contextHygieneEnabled is turned off. Tests append the interrupted response before turn_end (aborted and error), cover a busy boundary (canAutoTrim false) and cleanup turned off.
  2. Cache lifetime from the last response only (claude B2). A fully cached (cacheWrite = 0) or interrupted response after a 1h write made the prefix look 5 minutes old. cachedPrefix() dates the entry by the last response and takes the lifetime from the last response that wrote cache — the host-cache-ledger rule.
  3. Automatic trims need a reachable smart_context (deepseek P1-9, muse P0-5): canAutoTrim checks toolExposure.reachable("history") like artifact offload; ARCHITECTURE claimed it, nothing enforced it. Smoke through src/index.ts wiring with a fake host: eager/lazy trim (1 context_edit), off does not. Readiness & details names the inactive state.
  4. Silent drop of a queued change on an incomplete turn (glm B2): the usual "Context operation not applied" note now appears. Rewind attributed as trim in the ledger (glm B1): staged.kind, new ContextEditKind rewind.
  5. Small: explicit source for noteForeignCompaction instead of startsWith("Another") (claude B4); redundant before_switch/before_fork ledger resets removed (claude B5; session_start resets); write-only markedAt removed; break-even docstring counts the trimmed request (claude B7); veto derivation comment; bench case price a held trim (trimTokens, p95 3.3 ms) beside plan trim; package.json files excludes docs/findings (claude C2; packed count unchanged at 269).

Claims checked and not applied

  • Veto math (deepseek P1-3 = muse P0-3): the code is right. Pi's missCost = (w − r)·P; the warm path still reads X at r, the cold+trim path neither reads nor writes X, so the avoided cost is missCost − w·X. The comment now states the derivation.
  • Conflict notice truncated (claude B3): measured 360 chars with two conflicts, cap 400; only 3+ conflicts truncate.
  • Warming stop is sticky (muse P1-1): Pi re-decides per request (cache-warmer.d.ts:47).
  • Per-turn planning cost (claude P1): measured 4.9 ms at 560 entries / 2.9 MB, 17.8 ms at 2,800 entries / 14.4 MB — real, small; the bench now pins pricing too.
  • lifecycle-e2e flake (deepseek P0-9): 4 full-suite runs green.

Not in this PR

#75-area defects confirmed by re-verification (artifact-storage chunk views — reproduced with a 1.2 MB line; tombstone pruning; Hindsight fail-open ledger; native compaction budget/estimate/backup; bridge lock; Mnemopi readiness; Home settings writeConfig; dashboard in RPC; settled watchdog; receipts; UI batch) are queued for separate PRs.

Four audits of the 9.8.0-canary.7 wave (#75-#79), one folder per
model-harness pair, indexed from docs/README.md and docs/findings/README.md.
Advisory only; the package excludes docs/findings (package.json files).
…ries; lifetime from the last cache writer

From the 2026-09-28 review reports (docs/findings), verified against the
code and Pi 0.87.1 before changing anything:

- turn_end dropped `applied` at its top and restored it only on a contested
  boundary. Pi persists an aborted/failed response with a fresh timestamp,
  so after such a turn the next request looked warm, went out untrimmed and
  rewrote the prefix the interrupted request had just cached (claude B1,
  deepseek P1-2, muse P0-2). `applied` now lives until a boundary commits
  it, unchangedSince fails, the session changes or contextHygieneEnabled is
  turned off.
- The cold check read the cache lifetime from the last response only; a
  fully cached or interrupted response after a 1h write made the prefix look
  5 minutes old (claude B2). cachedPrefix() dates the entry by the last
  response and takes the lifetime from the last response that wrote cache,
  the host-cache-ledger rule.
- canAutoTrim requires smart_context reachable (toolExposure.reachable
  "history"), as artifact offload does; ARCHITECTURE said so but nothing
  enforced it (deepseek P1-9, muse P0-5). Readiness names the inactive state.
- A queued change meeting an incomplete turn now leaves the "not applied"
  note (glm B2); committed rewinds reach the ledger as "rewind" (glm B1).
- noteForeignCompaction takes an explicit source instead of matching the
  notice text (claude B4); the before_switch/before_fork ledger resets were
  redundant with session_start (claude B5); markedAt was write-only.
- Comments: break-even N* counts the trimmed request (claude B7); the veto
  derivation states why the miss really costs missCost - w*X (deepseek P1-3
  and muse P0-3 are wrong: the warm path still reads X at r).
- Bench: price a held trim (trimTokens) beside plan trim (claude P1).
- package.json files excludes docs/findings (claude C2).

Tests: aborted/error turns append the interrupted response before turn_end;
busy boundary keeps the carried trim; cleanup off forgets it; 1h lifetime
survives a fully cached response; rewind attribution; incomplete-turn note.
@alpertarhan

Copy link
Copy Markdown
Owner Author

Receipts (local, env -i, private HOME/TMPDIR) at ca70613:

  • bun run typecheck: clean (src, scripts, tests, bench).
  • bun test: 1568 pass / 0 fail across 120 files (4 earlier full runs on main also green; the reported lifecycle-e2e flake did not reproduce).
  • bun run gate: 376 / 0, EESV adversarial release gate passed (26 suites).
  • bun run bench: exit 0 — plan trim over 120 archived reads p95 2.3 ms, new price a held trim over 120 archived reads p95 3.3 ms (limit 25 ms).
  • bun run build: pass; bun run release:audit: pass (269 packed files; docs/findings excluded).
  • dist/index.js sha256 8de115bff66d9ac1454369ede1480ea628a66785c4fcdbcb2746699e7dc6744f.
  • Mutation checks (each reverted after): dropping applied at turn_end top → 4 tests fail; lifetime from the last message only → the 1h test fails; staged.kind hardcoded → the rewind test fails; no incomplete-turn note → the aborted table row fails; keeping a carried trim with cleanup off → its test fails. A faithful pre-fix pending variant fails exactly the 3 new retention tests.
  • Reachability smoke through src/index.ts with a fake host that activates registered tools: eager → 1 context_edit, lazy → 1, off → 0.

@alpertarhan
alpertarhan marked this pull request as ready for review September 28, 2026 10:42
@alpertarhan
alpertarhan merged commit 1e365cf into main Sep 28, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant