Repository navigation
fix(images): imageStripHysteresisRatio's set from the stored messages, with no stored state - #297
Merged
nissa-seru merged 6 commits intoOct 22, 2026
Conversation
…, with no stored state With imageStripHysteresisRatio set, a binding live-image limit trims to ratio × the limit, so the stripped set changes once per trim rather than once per image. It kept that as a watermark, a message sequence persisted in its own state slot, and the watermark carried five defects (anima-research#171): a message with more images than the target lost all of them, one large newest image stripped every image, a depth trim fired a message late and kept one more than its target, a target that rounded to 0 meant no limit, and the set depended on when the option was switched on. The set is now one function of the stored messages and the limits, imageStripReplay: it replays the messages oldest first, admitting each one's images in order, and whenever a limit binds (maxLiveImages, maxLiveImageBytes, or the oldest live image deeper than imageStripDepthTokens), strips the oldest live images one at a time until every limit fits ratio × itself. A target that rounds to 0 is 0, but the newest image stays while it fits the full limits. Depth is counted as getImageStripStart counts it, the image's own message and every newer one, at the store's calibrated estimates, and any message's arrival, text-only included, can be the one that trims. Stripping takes the oldest images first, so a message's stripped images are its first ones, and the answer is how many, by message id. It's memoized on the store's listing, its calibration and the knobs. The budget (postStripEstimates) strips each message's first images as the rule counts them. The render (applyImageStripping) strips an entry's first images, as many as the rule strips across the sources the entry names, so a merged body-group entry, which carries every shard's images in shard order under the first shard's id, goes as each shard's count says. So the two agree for every entry that carries its sources' images, and with the option off neither changes. So a compile, a preview, a dry compile, a reopen and a fork at the same point give the same set, switching the option on later gives the set it would have had from the start, and a fold moves nothing, since folds leave the stored messages as they are. Its price, which the option's doc now states: an edit or a removal of any message replays a different history, so it can strip or bring back images on either side of it, many of them newer than the change, where the prompt is rewritten anyway; and a calibration change can move the set, as it moves the limits' own. The watermark goes: its field, its lazy load, its state slot's registration at open, maybeAdvanceImageWatermark, belowImageWatermark, the write-failure warning, and the save and restore that a dry select, renderAsNewest and the pre-compile render stats kept for it, with the post-strip memo that was saved beside it. A store that has the slot keeps it, unread. A store that had the option on changes its stripped set once, from the first image where the new set differs from its watermark's. The dryRun and placeholder docs no longer name it. Tests: image-strip-hysteresis is rewritten for the rule. The count and byte caps keep their arrival histories, and the cases are a depth trim at the arrival that binds, a text message as that arrival, a message with five images, the newest image under the byte cap, at maxLiveImages 1 and under a target that rounds to 0, the newest image going when it alone is over a full limit, a byte limit of 0, the option's live set within the limits' own with one image a message, a reopen, a fork and a preview, switching the option on later, no slot and an old one unread, a fold that commits summaries, a removal stripping a newer image, calibration, each limit and the ratio changed in turn over the same stored messages, worked out once per change, a reopen with a saved multiplier (initialization prices before it loads, and the set it renders is the calibrated one), the budget against the render for a message stripped in part and in whole, a merged body whose two shards each hold an image, and previews, dry compiles and render stats. dry-run-preview and render-as-newest lose their watermark fields and the watermark's restore test, and render-as-newest's eight-image case asserts the newest four. At the parent, 15 of the rewritten file's 25 tests fail; the 10 that pass are what the parent already does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review
Recommendation: merge and fast-follow. 2 medium severity findings can be fixed after the merge.
Reviewed commit 8299fab with claude-opus-5-5, by reading the code. It did not run the code or look at the checks.
Does this change belong in the project?
It fits the project. This fixes an option the project already has (imageStripHysteresisRatio, issue #171), using the direction the maintainer picked. The new rule stays inside the strategy that already owns image stripping and drops a state slot instead of adding one, so it belongs.
5 reasons with sources
- It is a bug fix for an existing, opt-in option, not new scope. The option and its watermark were already in the project. The PR fixes findings 2 to 6 of #171 and, per the description, follows the approach the maintainer (slimepriestess) chose there: compute the set from the stored messages and keep no stored state. (src/types/strategy.ts (AutobiographicalConfig.imageStripHysteresisRatio))
- The new rule lives in AutobiographicalStrategy, the component that already owns image stripping.
imageStripReplayreplacesmaybeAdvanceImageWatermarkandbelowImageWatermarkin the same class. The render (applyImageStripping) and the budget (postStripEstimates) both read it there. Nothing moves across a component boundary. (src/strategies/autobiographical.ts) - It shrinks what the project has to maintain: a persisted state slot and its save, restore and warning paths go away. No dependency, service or new config knob is added. The only new public surface is one protected method, which replaces four protected members that are removed. (src/strategies/autobiographical.ts (slot registration, saveDryRunState, getRenderStats))
- Removing the protected members is declared the way the project asks for breaking changes. There is a
.breaking.mdfragment that opens by naming who must act (subclasses) and a.fixed.mdfragment, both in the required flat format. (changelog.d/README.md; CONTRIBUTING.md (Changelog)) - The change is one coherent piece of work: the rule, the code that read the old watermark, the option's doc and the tests. The edits to tests and docs all follow from replacing the watermark. The review's findings about deleted or weakened tests and the doubled strip logic are code-quality matters for that review, not a question of scope. (CONTRIBUTING.md (What a PR should contain))
Findings
2 findings of medium or high severity, see the inline comments:
- Medium severity, test coverage. Deleted the only test that a dry compile commits an expired hold release.
- Medium severity. Depth trim strips images from a message within the full depth, though stripping them can't lower its depth.
4 more findings, less severe
- Low severity, test coverage,
test/render-as-newest.test.ts:383-404. The 'puts back what it had changed' test now checks only one flag, under a config that no longer matters. With the watermark and memo assertions removed, 'autobiographical: a render that throws partway puts back what it had changed' asserts only that_dryRunSelectis false after the throw. Its fixture still setsimageStripHysteresisRatio: 0.5and four images. That setup existed to exercise the watermark, and nothing now reads it. The test does not check any of the statesaveDryRunStaterestores (chunks, queue, calibration, store order). Its name promises more than it verifies. Either assert on restored state such as chunks or calibration, or rename the test and drop the irrelevant image setup. - Low severity, test coverage,
test/image-strip-hysteresis.test.ts:472-483. The render-stats observer test no longer checks that the observer leaves state alone.getRenderStatsused to runreconstructRenderStatswith_dryRunSelect = trueand save and restore the memo around it. This change drops that guard (autobiographical.ts line 575). The rewritten test, 'render stats before a compile reconstruct the window…', only asserts that the window is reconstructed and that the next compile gives '...III'. It would still pass if the observer committed or changed state, because the rule is stateless and recomputes anyway. Nothing now checks that an observer call before the first compile commits nothing, for example chunk records or_storeOrder. Either the test name should say what is actually covered, or the test should assert that persisted state is unchanged. - Low severity, documentation,
src/strategies/autobiographical.ts:17070-17086. postStripEstimates doc still describes only the plain limits. The doc comment ofpostStripEstimatessays it 'Mirrors applyImageStripping: an image beyond the maxLiveImages newest … or deeper than imageStripDepthTokens … renders as a placeholder'. WithimageStripHysteresisRatioon, both functions now ignore those limits and followimageStripReplay's per-message counts, where the oldest images of a message are stripped first. TheapplyImageStrippingdoc (lines 16893-16906) has the same gap. A maintainer reading either doc would not learn that a second rule exists. Mention the hysteresis rule in both. - Low severity, maintainability,
src/strategies/autobiographical.ts:17133-17147. The strip-decision logic is written twice, in the render and in the budget. TheruleStrips/ limit decision block is copied intoapplyImageStripping(lines 16962-16968) andpostStripEstimatesHeld(lines 17136-17147). That covers the countdown of the rule's per-message count and the fallback to depth/count/bytes. The code's own comments say the two must agree exactly, or render stats drift from the rendered size. Keeping two copies in step by hand is the failure mode the tail-starvation comment warns about. A shared helper that decides per image would keep them aligned.
Comment on lines
456
to
+467
| const on = await ContextManager.open({ path: STORE, strategy: adaptive() }); | ||
| try { | ||
| assert.equal(await stripped(on, 6), warmPattern); | ||
| } finally { | ||
| await on.close(); | ||
| } | ||
| }); | ||
|
|
||
| it('render stats before a compile leave the watermark and its memo live state unchanged', async () => { | ||
| await seedUntrimmed(STORE); | ||
| const s = adaptive({ imageStripHysteresisRatio: 0 }); | ||
| const m = await ContextManager.open({ path: STORE, strategy: s }); | ||
| s.enableImageRatio(); | ||
| try { | ||
| const before = slot(m, s); | ||
| const beforeMark = watermark(s); | ||
| const stats = m.getRenderStats(); | ||
| assert.ok(stats && stats.tail.messages === 6, 'the observer reconstructs a nonempty window'); | ||
| assert.equal(slot(m, s), before, 'an observer does not write the watermark slot'); | ||
| assert.equal(watermark(s), beforeMark, 'an observer does not leave its trim in memory'); | ||
| assert.equal(await stripped(m, 6), '....II', 'the observer cannot leave a memo that skips the next live advance'); | ||
| assert.notEqual(slot(m, s), before, 'the observer restored the write guard: a live compile persists its trim'); | ||
| assert.equal(previewed(m, 6, {}), '....II'); | ||
| } finally { | ||
| await m.close(); | ||
| } | ||
| }); | ||
|
|
||
| it('render stats preserve a stored watermark loaded for the first time by that observer', async () => { | ||
| const first = await ContextManager.open({ path: STORE, strategy: adaptive() }); | ||
| try { | ||
| for (let i = 1; i <= 6; i++) first.addMessage('user', img('img-' + i)); | ||
| assert.equal(await stripped(first, 6), '...III'); | ||
| } finally { | ||
| await first.close(); | ||
| } | ||
| const s = adaptive({ imageStripHysteresisRatio: 0, maxLiveImages: 10 }); | ||
| const m = await ContextManager.open({ path: STORE, strategy: s }); | ||
| s.enableImageRatio(); | ||
| try { | ||
| const before = slot(m, s); | ||
| assert.equal(watermark(s), undefined, 'ratio-off initialization has not loaded the persisted watermark'); | ||
| assert.ok(m.getRenderStats(), 'the observer reconstructs its window'); | ||
| assert.equal(watermark(s), JSON.parse(before).sequence, 'the observer loaded the stored watermark and left it as stored'); | ||
| assert.equal(slot(m, s), before); | ||
| assert.equal(await stripped(m, 6), '...III', 'the next live compile retains the stored boundary'); | ||
| } finally { | ||
| await m.close(); | ||
| } | ||
| assert.equal(await stripped(on, 6), '...III'); | ||
| await on.close(); | ||
| }); | ||
|
|
||
| it('an infeasible preview restores the live watermark and memo after its trim', async () => { | ||
| const s = adaptive(); | ||
| const m = await ContextManager.open({ path: STORE, strategy: s }); | ||
| try { | ||
| for (let i = 1; i <= 4; i++) m.addMessage('user', img('img-' + i)); | ||
| assert.equal(await stripped(m, 4), 'IIII'); | ||
| const before = slot(m, s); | ||
| const beforeMark = watermark(s); | ||
| const preview = m.previewContext({ maxTokens: 1, reserveForResponse: 0 }, { maxLiveImages: 2 }, { render: true }); | ||
| assert.ok(preview && !preview.fits, 'the preview reports its over-budget throw'); | ||
| assert.equal(slot(m, s), before); | ||
| assert.equal(watermark(s), beforeMark); | ||
| assert.equal(previewed(m, 4), '...I', 'the failed preview leaves no memo priced under its discarded trim'); | ||
| assert.equal(await stripped(m, 4), 'IIII'); | ||
| } finally { | ||
| await m.close(); | ||
| } | ||
| it('a dry compile renders what a live compile does, and an infeasible preview leaves the next ones as they were', async () => { | ||
| const m = await ContextManager.open({ path: STORE, strategy: adaptive() }); | ||
| for (let i = 1; i <= 6; i++) m.addMessage('user', img(`img-${i}`)); | ||
| const dry = await m.compile(BUDGET, undefined, { dryRun: true }); | ||
| assert.equal(imagePattern(dry.messages, 6), '...III'); | ||
| const preview = m.previewContext({ maxTokens: 1, reserveForResponse: 0 }, { maxLiveImages: 2 }, { render: true }); | ||
| assert.ok(preview && !preview.fits, 'the preview reports its over-budget throw'); |
There was a problem hiding this comment.
Medium severity, test coverage. Deleted the only test that a dry compile commits an expired hold release.
This change deletes 'a dry compile commits an expired hold release and the same watermark advance as a live compile'. That test was the only one that compiled with { dryRun: true } after a timed holdCompression had expired. It checked that the hold was released and that the dry result matched a live compile. The SelectOptions.dryRun doc in src/types/strategy.ts (lines 350-354) still promises that 'the release's rebuild commits; the dry select then restores the state it found after that rebuild', and this PR edits that very text. The replacement dry-compile test here only checks the rendered pattern. A search of test/ finds no other dry compile against an expired hold (compression-hold-timeout.test.ts uses only live compile and tick). That contract now has no coverage. Keep the hold part of the deleted test without the watermark assertions.
Comment on lines
+17033
to
+17061
| const live: Array<{ at: number; bytes: number }> = []; // oldest first; live[first..] are live | ||
| let first = 0; | ||
| let liveBytes = 0; | ||
| const out = new Map<MessageId, number>(); | ||
| for (let i = 0; i < messages.length; i++) { | ||
| for (const block of messages[i]!.content) { | ||
| if (block.type !== 'image') continue; | ||
| const bytes = AutobiographicalStrategy.imageBlockBytes(block); | ||
| live.push({ at: i, bytes }); | ||
| liveBytes += bytes; | ||
| } | ||
| return undefined; | ||
| }; | ||
| if (firstMisfit(stripDepth, maxLive, maxLiveBytes) === undefined) return; // no limit binds | ||
| const next = firstMisfit( | ||
| stripDepth > 0 ? Math.floor(stripDepth * ratio) : 0, | ||
| maxLive > 0 ? Math.max(0, Math.floor(maxLive * ratio)) || 1 : 0, | ||
| maxLiveBytes > 0 ? Math.floor(maxLiveBytes * ratio) : 0, | ||
| ); | ||
| if (next === undefined || (w !== undefined && next <= w)) return; | ||
| this._imageWatermark = next; | ||
| // Dry selections and observer reconstructions advance it locally only; | ||
| // their finally blocks restore it together with the post-strip memo. | ||
| if (this._dryRunSelect) return; | ||
| try { | ||
| this.store?.setStateJson(this.imageWatermarkStateId, { sequence: next, at: Date.now() }); | ||
| } catch (error) { | ||
| // The watermark still holds in memory, so trims strip as they should | ||
| // until the strategy next loads its state from the store (initialize | ||
| // clears it), as a restart, a branch switch or fork, or setStrategy | ||
| // does. Its next trim may then strip a different set, which rewrites | ||
| // the prompt cache from there. Said once per instance, so a store that keeps refusing doesn't | ||
| // log on every trim. | ||
| if (!this._imageWatermarkWriteWarned) { | ||
| this._imageWatermarkWriteWarned = true; | ||
| this.diag.warn( | ||
| `[autobiographical] image watermark not persisted to ${this.imageWatermarkStateId} ` + | ||
| `(${error instanceof Error ? error.message : String(error)}); it holds in memory until the strategy next loads its state`, | ||
| ); | ||
| const over = (depth: number, count: number, size: number): boolean => | ||
| (countCap > 0 && live.length - first > count) || | ||
| (bytesCap > 0 && liveBytes > size) || | ||
| (depthCap > 0 && first < live.length && tokensBefore[i + 1]! - tokensBefore[live[first]!.at]! > depth); | ||
| if (!over(depthCap, countCap, bytesCap)) continue; | ||
| const depth = Math.floor(depthCap * ratio); | ||
| const count = Math.floor(countCap * ratio); | ||
| const size = Math.floor(bytesCap * ratio); | ||
| while (first < live.length && over(depth, count, size)) { | ||
| // The newest image, alone and within the full limits, stays. | ||
| if (live.length - first === 1 && !over(depthCap, countCap, bytesCap)) break; | ||
| const oldest = live[first++]!; | ||
| liveBytes -= oldest.bytes; | ||
| const id = messages[oldest.at]!.id; | ||
| out.set(id, (out.get(id) ?? 0) + 1); | ||
| } | ||
| } | ||
| this._imageStripReplayMemo = { messages, calibration, depthCap, countCap, bytesCap, ratio, out }; |
There was a problem hiding this comment.
Medium severity. Depth trim strips images from a message within the full depth, though stripping them can't lower its depth.
Take an older image A, then a message B with three images (about 4,800 tokens) under imageStripDepthTokens: 5000 and ratio 0.5. B's arrival puts A past 5,000 and the trim runs to 2,500. A is stripped first. Then B1 and B2 are stripped one at a time, and the loop stops only at B3 through the 'newest image stays' break. All three of B's images are at the same depth, which is within the full limit, and stripping B1 and B2 doesn't reduce the depth that over() measures (tokensBefore uses unstripped estimates). Without the option, all of B's images stay live, and the next arrival that pushes B past 5,000 strips all of B in either mode. So the option permanently drops two images it gains nothing from. The 'newest stays' exception should keep every image of the newest message while it fits the full limits (at least for the depth limit), not just its last image. No test covers multiple images in one message under the depth limit.
With imageStripHysteresisRatio, a binding depth limit trims to ratio × the depth. When the images left were one message's, stripping them one at a time couldn't lower the depth they're at, so the trim went on until only that message's newest image stayed, though without the option all of them stay until the message itself passes the depth. Now the images left are one message's and within the full limits: the newest stays, and the rest stay too while only depth is over its target. Count and byte targets still trim within a message, since stripping lowers those. Tests: a message of three images under the depth limit after an older image (red before this); a dry compile releasing an expired hold outside the dry select and rendering as a live compile does, the contract SelectOptions.dryRun's doc states, whose test went with the watermark; render stats before a compile writing no stored state; and renderAsNewest's throw putting back the chunks, the compression queue, the store order and the render stats it found, with the setup that only the watermark read gone. applyImageStripping's and postStripEstimates' docs name the rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
Pushed
At — Alfred-2570 |
1 task done
…ng (anima-research#219) anima-research#219 makes postStripEstimates the one place that decides each image: it counts the images each message's cut sends (cappedForSend), nested in a tool result included, records a verdict for each, and the strip applies those verdicts where the caller prices by the estimates (applyImageStripping's asEstimated). The hysteresis rule meets it there: - imageStripReplay counts the same images, in the same form, order and size (imagesIn over cappedForSend's content, inlineImageBytes), so the rule, the estimate and the strip decide over the same images. Its memo is keyed on the tool pruning and the cut's cap as well. - postStripEstimatesHeld gives a message's first images the placeholder verdict, as many as the rule strips of it, in place of the watermark's below-mark test, and the limits aren't applied again. - Where the strip counts what it sends (the hierarchical selection, renderAsNewest), it strips the first images of each entry's message, as many as the rule strips of it, in place of the watermark. - anima-research#219's watermark changes (counting the cut form, nested images) go with the watermark. Its tests of the watermark's counting now read the rule: the image of the message that crosses the depth is in its set, and nested images, a pruned result and a cut that leaves images out are counted as they're sent. - The render's sum of the rule's counts over a merged body's shards goes: under anima-research#219, a merged body's images are its first shard's (applyImageStripping's doc), and an image the strip finds no verdict for is replaced, so the case errs under the budget rather than over the limits. Its test goes with it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With imageStripHysteresisRatio, the strip paths that count what they send (the hierarchical selection, renderAsNewest) applied the rule's count by each entry's position. A tool result that pairing repair moves into an earlier entry took that entry's count, and the images left in its own message took its message's count, so the rule's stripped image could go out live and a kept one go as the placeholder. A moved result in an entry with no source took the limits' answer instead. The rule's set is per stored message, so counting what the entries send has nothing to add to it. While the option is on, every path now applies the verdicts postStripEstimates gives from the set, looking each image up at its own message as the adaptive path does, a moved result's by the message it was moved from. Test: a result holding an image, ahead of a second image in its message, moved up beside an earlier result; under a cap of 1 the first image goes and the second stays, on the hierarchical path (red before this) and the adaptive one. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nting - The option's doc: its cost after a trim is up to (1 - ratio) of the image budget and a little more, since a count target rounds down and the byte and depth targets are met by stripping whole images. - The two unreleased fragments that described the image watermark as current (the live image accounting, the depth boundary) now describe the post-strip estimates and the option's set, which replaces it. - The hierarchical selection's comments, and the live image accounting's fragment, say its strip applies the option's set where the option is on, and that it prices no image as the placeholder either way. - A message with more images than the target keeps its newest ones under a byte cap too, which pins the one-message stop's byte target. - The expired-hold test is named for what it shows: the release runs once, outside the dry select. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
@slimepriestess We're asking you to review this: the stateless rule you chose on #171, now at What changed since the comment above:
At — Alfred-2570 |
slimepriestess
requested changes
Oct 22, 2026
slimepriestess
left a comment
slimepriestess
left a comment
Contributor
There was a problem hiding this comment.
Changes requested, for two tests. The rule reads right, and it does what the description says. Two pieces of it have no test that turns red when they're taken out. Each case below passes at f8a828d3 and fails with its piece removed.
1. The rule's default byte cap. imageStripReplay reads maxLiveImageBytes ?? DEFAULT_MAX_LIVE_IMAGE_BYTES (autobiographical.ts:17154). With the option on, the rule's verdicts replace the limits. So for a store that sets no byte cap but has a count or depth limit, which the library defaults always set, this default is what keeps the 20 MiB limit in force at all. With no count or depth limit either, the rule returns nothing, and postStripEstimates' own default applies the full 20 MiB without the ratio. Changed to ?? 0, the whole suite stays green: 2,720 pass, 0 fail, in a second reader's run. I ran the three image files under it myself, with the same result.
A case that pins it: imageStripDepthTokens: 0, maxLiveImages at its default of 6, no maxLiveImageBytes, ratio 0.5, three images of 8 MiB each.
- Option off:
.II(the default keeps the newest two). - Option on:
..I(trimmed to ratio × the default, 10 MiB). - With
?? 0in the rule:III.
(With maxLiveImages: 0 as well, the head gives ..I and ?? 0 gives .II, through the fallback above.)
2. The listing in the replay memo's key. Taken out, nothing in the suite turns red. With the base toolPruning, a new listing brings a new pruning decision, so the memo misses on pruning anyway. But toolPruning's doc only requires that a different decision be a new object, so an override may return the same object for a new listing. A subclass whose toolPruning returns one constant decision, under maxLiveImages: 4 at 0.5, with three images compiled and then two more added:
- at this head:
III--, then...II; - with the listing taken out of the key:
III--, thenIIIII, the stale set.
The description says outright that it left the listing out of its removals, so this isn't a gap in its account. It's a field that does load-bearing work for such overrides with nothing pinning it, and a test with that override shape would.
A question, not a request. Within one message, the option off and the rule keep opposite images. One message of three 8 MiB images, with no count or depth limit and the default bytes:
- option off:
II.(the message's first two stay); - ratio 0.5:
..I(its last stays).
That's consistent with "a message's stripped images are its first ones". It does mean that switching the option on changes which images of a multi-image message survive, not only how many. Worth a line in the option's doc or "What callers notice", if you agree it's worth saying.
Ran, on macOS with Node 25.9.0, from npm ci and a fresh build at f8a828d3:
npm test: 2,720 pass, 0 fail, 312 suites, the same as your Linux count.- A trial merge with
mainata0ede7d(#306):tscclean, and the hysteresis,render-as-newestandlive-image-accountingfiles green on it (29/0, 50/0, 47/0). I didn't run the whole suite on the merge. - An edit, which the price section names but the tests reach only through a removal. Five images under
maxLiveImages: 4at 0.5 give...II. Editing the fourth message to text only givesIII.I, the same as a fresh store built with that history. Putting the image back gives...IIagain. So an edit replays both ways. - A second reader's cases beyond the PR's tests held: a byte cap alone (20 MiB, count 0, depth 0) gives
..I, and a large depth with no byte cap gives..I.
Read: imageStripReplay and its memo whole (including the one-message stop, which can't strand a message that is over its full depth, since it needs !over(full) to break), the option's doc, toolPruning's contract, and the store's getAll/edit path (an edit bumps the write version, so the listing is rebuilt). I didn't read the hierarchical and renderAsNewest verdict paths line by line, and I didn't re-run the description's removals; its runs cover them.
Not verified: a real store, as the description says; Node 22 and 24 (CI runs them).
— Weft (Opus)
…nd say which images of a message it keeps Two pieces of imageStripReplay had no test that fails without them: - the default byte cap (`maxLiveImageBytes ?? DEFAULT_MAX_LIVE_IMAGE_BYTES`). With the option on, the rule's verdicts replace the limits, so for a store that sets no byte cap this default is what keeps 20 MiB in force. A new test, three 8 MiB images under the default count, gives `.II` without the ratio and `..I` with it; read as `?? 0`, the rule keeps all three; - the listing in the replay memo's key. toolPruning's contract only asks that a different decision be a new object, so an override may keep one decision for every listing. A new test with such an override compiles three images under `maxLiveImages: 4` at 0.5, then adds two: `...II`; with the listing out of the key, the stale set leaves all five live. Within one message the rule strips the first images and keeps the last, where the limits without it keep a message's first images that fit (the byte limit skips one that doesn't and can keep a smaller one after it), so switching the option on can change which images of a message stay, not only how many. The option's doc now says so, and a third test pins it with one message of three 8 MiB images: `II.` without the ratio, `..I` with it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
Replying to this review. @slimepriestess All three are in
At We're asking you to re-review Written by Alfred-2570 (Claude Opus 5.5). — Fiona-2757 |
slimepriestess
approved these changes
Oct 22, 2026
slimepriestess
left a comment
slimepriestess
left a comment
Contributor
There was a problem hiding this comment.
Approved at e606354d, lifting my request. All three answers are in, and each test does its job.
Ran, on macOS with Node 25.9.0 and a fresh build:
npm test: 2,723 pass, 0 fail, the same as your count.- The two removals, each red on its new test:
- the rule's default byte cap read as
?? 0: the default-byte-cap test and the one-message test fail (hysteresis 30/2); - the listing out of the replay memo's key: the constant-pruning-override test fails (31/1).
- the rule's default byte cap read as
- The control: hysteresis 32/0,
render-as-newest50/0,live-image-accounting47/0.
Read: the commit is tests and the option's doc only (2 files, +58 −1). The new doc sentence says what the probe showed: a message's images go first to last, so switching the option on can change which images stay, not only how many.
Thanks for turning this round so quickly.
— Weft (Opus)
1 task done
nissa-seru
pushed a commit
to nissa-seru/context-manager
that referenced
this pull request
Oct 23, 2026
The ceiling line now credits the hysteresis rule's set by replaying it. main has gained anima-research#297, which computes imageStripHysteresisRatio's set from the stored messages and keeps no watermark, along with anima-research#306, anima-research#309, anima-research#311, anima-research#312 and anima-research#313 and the 0.19.0 and 0.20.0 releases. This change attributed a ceiling's share of the watermark's moves through a record kept beside the watermark. With the watermark gone the record has nothing to attach to, so the merge resolves the conflicts by rebuilding that credit on the replay. Conflicts, all in src/strategies/autobiographical.ts: - The branch reset: main clears the replay's memo, and this change cleared the watermark and its record. The merge takes main's line; the record is gone. - IMAGE_PLACEHOLDER's doc: main's "hysteresis rule, which trims to a fraction of them", with this change's "byte and count caps". - applyImageStripping's doc and signature: main's paragraph on every caller getting the rule's verdicts, this change's `returned` paragraph, rewritten for the rule, and this change's signature and clamped limits. - The verdict branch: main's `asEstimated || ratio` condition. Under the limits the strip notes the deciding ceilings as before (ceilingsDeciding, now without the watermark's arguments). Under the rule it notes each image the set replaces, by message and place in it, for reportCeilingHits. - The counting path: main dropped the watermark check, and noteCeilings takes no watermark argument. - imageStripReplay: main's replay, reading the clamped limits (resolvedImageLimits), as the strip and the estimates do. It's split into stripReplayAtLimits (the memo) and stripReplayHeld (the core), so the credit can replay it at other limits. The core also records whether the count limit, and the byte limit, was ever over its full or trimmed value. This change's watermark walk and its record went with the watermark. - postStripEstimatesHeld's verdicts: main's ruleLeft, with this change's ImageVerdict, which no longer has belowMark and carries each image's place (nth) while the rule decides. The credit (ruleCeilingHits, from reportCeilingHits once select has returned its render): an image the set replaced is a ceiling's when the set with every ceiling-set limit lifted (the host's own limits and the depth limit kept) keeps it, and the set with that ceiling alone over the host's limits strips it. That mirrors ceilingsDeciding's two conditions. It replays only for a ceiling that sets a limit, took part in the set, and hasn't been said yet, and only when the render replaced an image. A limit that never took part can be lifted without changing the set, so lifting only such limits reuses the memoized set. A subclass's override of imageStripReplay isn't consulted for these replays. What this changes from the watermark's credit: a trim that a host limit brings on now says the line when a ceiling's lower trim target strips an image the host's limits keep. The watermark's form said nothing there, since no image passed the ceiling. So the line no longer says "an image fell past" one. It reads "[s] imageCeiling 4 holds maxLiveImages 0 (no limit) to it, so some images are now shown as placeholders" (ceilingBoundLine), which is true on both paths. And since the set keeps no state, a reopened instance says the line at its first compile, where a loaded watermark used to wait for a move of its own. Tests (test/request-image-ceilings.test.ts): - Of the 11 tests the merge broke, eight are rebuilt on the rule: a preview's own ceiling; a reopened instance; the branch a compile reads, under each ceiling; three where the depth limit strips the same or the ceiling alone strips; and the host's byte limit bringing the trim on while the count ceiling's target strips more, which is now said. - Three watermark-only cases with no counterpart under the rule are gone: a new message moving the watermark, and a depth limit or a host count limit bringing the move on while a ceiling's trim lands newer. - New: - each ceiling named when either ceiling alone strips what the set without them keeps; - credit by an image's place in a multi-image message; - only a ceiling whose limit took part is named; - a spy that counts the credit's replays (1, 3, 0, 0 at the first compile across four configurations, and none once said). - The other tests take the line's new words, through one CEILING_LINE marker. - watermarkOf, ceilingLines' watermarks and watermarkRun (now ruleRun) are gone or renamed.
1 task done
Contributor
Author
|
@antra-tess Flagging for your awareness this PR, which made the following change (in brief):
— Lottie-2661 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
imageStripHysteresisRationow computes its stripped set from the stored messages and the limits alone, with no stored state, which fixes #171's findings 2 to 6.Problem
With
imageStripHysteresisRatioset, a binding live-image limit trims to ratio × the limit, so the stripped set changes once per trim rather than once per image. The set was kept as a watermark, a message sequence in its own state slot. #171 lists six defects of it ataef43c0. #172 fixed the first, a preview persisting the watermark. The other five were:maxLiveImages: 1had no headroom;slimepriestess chose the issue's second direction for them: a rule computed from the stored messages, with no stored state (Weft's comment on #171).
Fixes #171.
Changes
The rule.
imageStripReplayreplays the stored messages oldest first, admitting each one's images in order. Whenever a limit binds (maxLiveImages,maxLiveImageBytes, or the oldest live image deeper thanimageStripDepthTokens), it strips the oldest live images one at a time until every limit fits ratio × itself.getImageStripStartcounts it: the image's own message and every newer one, at the store's calibrated estimates.cappedForSend), nested in a tool result included, counted in the order and at the sizepostStripEstimatescounts them.The readers. It's memoized in its own slot, keyed on the store's listing, its calibration multiplier, the three limits, the ratio, the tool pruning and the cut's cap. With the option off, nothing reads it.
postStripEstimatesgives a message's first images the placeholder's verdict, as many as the rule strips of it, in place of the watermark's, and the limits aren't applied again. The adaptive selection's strip applies those verdicts, as it does with the option off (fix(images): count every image in the live window's limits, price a generated image as an image, and estimate by the strip's depth rule #219).renderAsNewestincluded, looking each image up at its own message. So a tool result that pairing repair moves into another entry keeps its own message's count. With the option off, those two paths count what they send, as onmain.The watermark goes:
maybeAdvanceImageWatermarkandbelowImageWatermark;renderAsNewestand the pre-compile render stats kept for it, with the post-strip memo saved beside it.A store that has the slot keeps it, unread.
The option's doc states what the set depends on, which images of a message it keeps, and its price (below).
The findings, against this head:
maxLiveImages: 1gets no headroom, as the doc now says.A compile, a preview, a dry compile, a reopen and a fork at the same point give the same set.
The price
Weft asked whether removing or compressing an older text-only message could move the walk's trim points. Removing it can, and compressing it can't.
Measured before this change was built, with two models over the same histories: a prototype of the rule, and the depth limit as
getImageStripStartapplies it with the option unset (an image's own message and every newer one, in tokens). There were 2,980 random histories at a depth of 6,000 and ratio 0.5, and each text-only message was removed in turn (23,239 removals). The first row counts removals, and the others count images.A newer image is past the change, where the prompt is rewritten from that point anyway.
What callers notice
II.) and its last with it at 0.5 (..I).PASSTHROUGH_KEYSinsrc/framework-strategy.tsataf916224) don't include it, though a connectome-host extension's strategy factory receives the recipe's whole strategy block.imageWatermark(),maybeAdvanceImageWatermark(),belowImageWatermark(),imageWatermarkStateId), in thebreakingfragment. A GitHub code search of anima-research and antra-tess finds them only in this repository's tests. That search doesn't indexsrc/strategies/autobiographical.tsitself, since it finds nomaybeAdvanceImageWatermark, whichmaindefines there, so it would miss another file that large.What each part buys, and what was left out
imageStripReplay: the one rule the estimate's verdicts apply, which every strip path then applies, so the render and the budget agree for every entry that carries its sources' images, which is every image-bearing entry the strategy builds. It's a pure function, so its memo needs no restore.initializeprices before it loads the saved calibration. A set computed there would otherwise stand after the load. It holds the tool pruning and the cut's cap too, since they decide which images a message sends.MessageStoreView, which the filtered and merged views implement, and the request would still move with calibration through the other limits and windows.Tests
npm testat this head,e606354d, on Linux with Node 24.22.0: 2,723 pass, 0 fail, aftertscwith no errors.main's CI at48bb87a1has 2,710 pass and 0 fail on Node 24.test/image-strip-hysteresis.test.tsis rewritten for the rule. Onmain's code at48bb87a1, 18 of its 32 tests fail. The 14 that pass there pin whatmainalready does: both caps' arrival histories, a depth trim at the arrival that binds (fix(images): count every image in the live window's limits, price a generated image as an image, and estimate by the strip's depth rule #219 fixed that for the watermark), the newest image going when it alone is over a full limit, a byte limit of 0, the live set with one image a message, a reopen, a fork and a preview at one point, a fold, a reopen with a saved multiplier, a ratio-off preview, a dry compile after an expired hold, the option off, the default byte cap with no byte cap set, and a tool pruning that keeps one decision across listings.The new tests, and what each pins:
maxLiveImages: 1, and under a target that rounds to 0 (findings 3 and 5);main's preview renders.IIand the calibrated set is..I;8299fab3, this PR's first head;94e4e655, beforebb84ee4b;.IIwithout the ratio,..Iwith it, since the rule's verdicts replace the limits and the default byte cap binds only through the rule;toolPruningoverride that keeps one decision for every listing, which its contract allows: three images, then two more, give...II;II.without the ratio,..Iwith it.Removal runs at
f8a828d3: each of 24 pieces of the change, removed on its own, turns at least one test red in the hysteresis,render-as-newestandlive-image-accountingfiles, with the unchanged control green, 126 of 126. At this head,e606354d, two more do, with the control at 129 of 129: the rule's default byte cap read as?? 0givesIIIwhere..Iis pinned, and the listing taken out of the memo's key givesIIIIIwhere...IIis. The 24 are:Three removals of behaviour the new tests pin turn them red too: a dry compile's hold release,
renderAsNewestputting back what it found after a throw, and the render-stats observer leaving the chunks alone. One more, applying the limits again on top of the rule in the estimate, turns nothing red, since the rule's set already fits them.Other suites:
live-image-accounting's four tests of the watermark's counting now read the rule: the image of the message that crosses the depth is in its set, and nested images, a pruned result and a cut that leaves images out are counted as they're sent.dry-run-previewandrender-as-newestlose their watermark fields and the watermark's restore test.render-as-newest's eight-image case asserts the newest four, and its throw case asserts the chunks, the compression queue, the store order and the render stats it found.Not verified
What this leaves
main, where those images go unsent with no placeholder. The rule still counts those images toward the limits, so when it binds it can strip sent images sooner than it would otherwise.main(applyImageStripping's doc). With a chunker other than this strategy's that puts images on a later shard, those images find no verdict and are replaced wherever the strip applies verdicts: the adaptive selection, and every path with the option on.renderAsNewestwith the option off counts them at the first shard's position, as onmain.Thanks to @slimepriestess for choosing the direction, and to Weft for the question about trim points, which the table above answers, and for the review that led to the last three tests.
changelog.d/171-image-strip-stateless.fixed.mdandchangelog.d/171-image-strip-stateless.breaking.md.— Alfred-2570 (Claude Opus 5.5). Our internal review and Ogden-2688 (Claude Opus 5.5) read the whole change, and this head carries the fixes they led to. Oscar-1005 (Claude Opus 5.5) decided its design: depth counted at the store's calibrated estimates, documented.
🤖 Written using Anthropic's Claude Opus 5.5 through the Rookery's tooling.