Repository navigation
test(mid-turn): the memory-strategy injection tests stub the render's null - #815
nissa-seru merged 2 commits into
Conversation
… null Three tests of a message injected at a tool boundary under the memory strategy relied on context-manager's render giving null for it: a batch bigger than the recent window, a message after a head-window reset that hasn't filled, and a body stored in parts. Each asserted that premise first, then what went in: as the next request sends it, or, for the body stored in parts, joined as the store reads it back. A context-manager change in review makes the render give those messages as the next compile sends them, so the premises fail there, while the behaviour after them still holds. These tests are about this framework's null branch: a null goes in as stored, in its place in the batch. So each now stubs the render through renderWith, as the file's other null tests do, giving null at the same positions as the render of the context-manager that main installs (the batch's two oldest screenshots; the one message after the reset; the long body). Their premise assertions on the render's nulls go, and each asserts instead that the boundary rendered the batch, as the file's other null tests do. The titles say the render gives null, and the comments that said the strategy's own render decides are gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Automated review
Recommendation: merge and fast-follow. 3 medium severity findings can be fixed after the merge.
Reviewed commit 8250020 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 change only touches tests. It keeps three existing tests of the framework's own null-render branch working whichever context-manager render is installed.
5 reasons with sources
It belongs here and adds no scope, dependency or interface.
- It tests behavior the framework already has, and changes no source code. The diff touches only three tests in an existing test file. They cover how the tool-boundary injection handles a message the render gives null. That logic is the framework's own, and the tests already existed. (test/mid-turn-injection-as-compiled.test.ts)
- It uses the stub pattern the file already has instead of adding a new mechanism. The three tests now go through the file's existing
renderWithhelper. They check the result withseen.calls, the same way the file's other null-render tests do. (test/mid-turn-injection-as-compiled.test.ts) - Skipping the changelog fragment is correct for a test-only change. CONTRIBUTING asks for fragments for behavior-affecting changes, and CI requires one only when
src/is touched or theno-changeloglabel is absent. This PR touches onlytest/and carries theno-changeloglabel. (CONTRIBUTING.md, Changelog) - The PR names its relationship to context-manager#314 but has no explicit merge-order line. CONTRIBUTING asks cross-repo companion PRs to state which side is safe to land first. The body names context-manager#314 and reports 15 of 15 passing against both the context-manager version
maininstalls and #314's head, so either order is safe. It never says that outright, and there is no 'Companion PRs' section. This is a small gap in the PR description, not a reason to keep the change out. (CONTRIBUTING.md, How changes land) - The PR body follows the house shape and evidence conventions. It has Problem, Changes, Tests and Not verified sections, gives numeric pass counts, and checks that the tests fail when the boundary is broken. CONTRIBUTING asks for each of these. (CONTRIBUTING.md, What a PR should contain)
Findings
3 findings of medium or high severity, see the inline comments:
- Medium severity, test coverage. The context-manager#275 regression test no longer exercises the memory strategy's real render for the oldest of the batch.
- Medium severity, test coverage. The head-window-reset test ignores the real render completely, so its memory-strategy and reset setup is dead.
- Medium severity, test coverage. The sharded-body test no longer checks what the real render gives a body stored in parts.
1 more finding, less severe
- Low severity, documentation,
test/mid-turn-injection-as-compiled.test.ts:202. PR does not declare the context-manager#314 companion relationship or a merge order. CONTRIBUTING.md asks for cross-repo companion PRs to be declared in the body, in a Companion PRs section, with merge-order guidance: which side is safe to land first and what happens if only one does. This PR exists to unblock context-manager#314 and depends on its behavior, but the body has no Companion PRs section and no merge-order statement.
For a deeper review by several readers and a judge, which takes longer, a maintainer can comment /review deep.
| it('under the memory strategy, a batch bigger than the recent window whose oldest the render gives null goes in whole, as the next request sends it (context-manager#275, R1-1)', async () => { | ||
| await create(new AutobiographicalStrategy({ | ||
| adaptiveResolution: true, foldingStrategy: 'kv-stable', recentWindowTokens: 4000, kvStableReachTokens: 8_000, | ||
| compressionModel: 'mock', targetChunkTokens: 100, autoTickOnNewMessage: false, |
There was a problem hiding this comment.
Medium severity, test coverage. The context-manager#275 regression test no longer exercises the memory strategy's real render for the oldest of the batch.
The test's title still cites context-manager#275 / R1-1 and still sets up a batch bigger than the recent window (recentWindowTokens: 4000, three history rows). The stub now forces positions 0 and 1 to null no matter what real(ids) returns, so the window size and history no longer decide anything. If AutobiographicalStrategy.renderAsNewest regresses for messages before its recent window, for example by returning wrongly cut content instead of null or as-compiled content, this test cannot catch it, because the stub replaces that output. With this change, no test in the file covers the real memory-strategy render for a batch bigger than the recent window, under the installed context-manager or under #314. Keep one test that uses the unstubbed render and asserts only the end-to-end property (live round equals next request), and put the null-branch check in a separate stubbed test.
| it('under the memory strategy, after a head window reset, a message the head has not filled up to and the render gives null goes in as the next request sends it', async () => { | ||
| await create(new AutobiographicalStrategy({ | ||
| adaptiveResolution: true, foldingStrategy: 'kv-stable', headWindowTokens: 4000, compressionModel: 'mock', | ||
| autoTickOnNewMessage: false, |
There was a problem hiding this comment.
Medium severity, test coverage. The head-window-reset test ignores the real render completely, so its memory-strategy and reset setup is dead.
renderWith((ids) => ids.map(() => null)) never calls real. The AutobiographicalStrategy with headWindowTokens, the three history rows and cm.resetHeadWindow(...) therefore have no effect on what the boundary injects. The test now checks the same null branch as the WindowedPassthrough test at lines 189-200, but under a title that claims to cover the head-reset case. A regression in how the memory strategy renders a message after /newtopic, whether to null (current) or as-compiled (#314), would pass unnoticed. Either keep an unstubbed variant that asserts only 'live equals next request', or say plainly that this is a null-branch test and remove the setup that is no longer needed.
| it('under the memory strategy, a body bigger than the recent window that the render gives null goes in as stored, and the rest as the next request sends it', async () => { | ||
| // The strategy stores the long message as several shards. As stored, it | ||
| // goes in joined, as the store reads it back: its image first, then its | ||
| // text as one block, which differs from the blocks as they were queued. | ||
| await create(new AutobiographicalStrategy({ | ||
| adaptiveResolution: true, foldingStrategy: 'kv-stable', recentWindowTokens: 300, kvStableReachTokens: 8_000, | ||
| targetChunkTokens: 200, | ||
| })); | ||
| const long = 'FIRSTLONG ' + Array.from({ length: 600 }, (_, i) => `word${i}`).join(' '); | ||
| const nulls: number[] = []; | ||
| renderWith((ids, real) => { const r = real(ids)!; nulls.push(r.filter((c) => c === null).length); return r; }); | ||
| const seen = renderWith((ids, real) => real(ids)!.map((c, i) => (i === 0 ? null : c))); | ||
| const injected = await turnWith([[text('Intro. '), image(), text(long)], [text('SECONDSHORT and a few words more')]]); | ||
| assert.equal(seen.calls, 1, 'the premise: the boundary rendered the batch'); | ||
| const rows = framework.getAgent('assistant')!.getContextManager().getAllMessages(); | ||
| const row = rows.find((m) => JSON.stringify(m.content).includes('FIRSTLONG'))!; | ||
| assert.ok(rows.filter((m) => m.bodyGroupId !== undefined && m.bodyGroupId === row.bodyGroupId).length > 1, 'the premise: it is stored in parts'); | ||
| assert.equal(nulls[0], 1, 'the premise: the render gave it null'); | ||
| assert.deepEqual(JSON.parse(holding(injected, 'FIRSTLONG')), [image(), text(`Intro. ${long}`)], 'it goes in whole, as the store reads it back'); |
There was a problem hiding this comment.
Medium severity, test coverage. The sharded-body test no longer checks what the real render gives a body stored in parts.
The stub forces index 0 (the sharded FIRSTLONG body) to null. The test therefore only checks that the boundary's as-stored fallback joins shards as getAllMessages reads them back. It no longer checks anything about how the memory strategy renders a sharded body at a boundary, and it never compares FIRSTLONG in the live round with the next request. Under context-manager#314 the real render gives this body as the next compile sends it, and no test here pins that the live round and next request agree for a sharded body. The test can't fail on a regression in the real render path for sharded bodies.
…each stubbed test Stubbing the render's null in the three memory-strategy tests left the file with no test that runs the strategy's own render at a tool boundary and checks that what goes into the live round is what the next request sends. Beside each stubbed test is now one with the same setup and no stub, asserting only that: - a batch bigger than the recent window: each screenshot; - a message after a head-window reset the head hasn't filled up to; - the message after a body bigger than the recent window. For the body itself the two still differ: it goes in whole, joined as the store reads it back, where the next compile sends it in more than one piece, an exception the boundary already names. So that test checks the message after it. These pass with context-manager as main installs it and with a render that gives these messages as the next compile sends them, and each fails when the render's output is changed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
main brought anima-research#826, anima-research#819, anima-research#824, anima-research#821, anima-research#825 (ignore and include read gitignore's forms, and refuse any other), anima-research#815, anima-research#820 and anima-research#818 (answers that write say where the change stands on disk). Where they met this change, in src/modules/workspace/index.ts: - the imports from observe.ts: main's list, which drops includePatternReadable and adds directoryAt and patternRefusal, with this change's DIRECTORY, LEAVES_MOUNT, NOT_FOLLOWED, NOT_REGULAR and rootAbsence; - after StoredThenFailed: this change's ONCE_REOPENED beside anima-research#818's DISK_ORDER and furthestFromDisk. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
268 commits from main, 28 pull requests: #353 (every agent on the store re-initialised after a rollback, suppression, switch, undo or redo), #817, #835, #818, #820, #815, #825, #821, #824 and the rest, with main's manager-release machinery (an ephemeral run's auxiliary membrane, a tracked manager released with its agent) and writeToFork (a fork's channel message held behind a live surgery). Eight hunks: a run's context keeps this branch's shared-inheritance branch (deriveSharedContext) beside main's open with the auxiliary membrane, and either manager is tracked and recorded on the release; a fork's channel message, routed or corrected, is written through main's writeToFork while admission still decides the delivered inbound and the wake; a fork's creation keeps the registry's record beside main's manager release; a fork's disposal keeps Dendrite's one release path, which gains main's reasoningLoss.forget and releaseManager (and the name-only branch its forget). .gitignore and the diagnostics test take main's words. #353's disposeEmptiedForks reads the registry's conversation-fork records and their homes instead of conversationAgentHomes. main's new dev dependency @animalabs/mcpl-core is installed, and the rig's membrane rises to 0.7.2 (connectome-host main asks for it). Eight rows of main's new tests are read onto this branch, each named in a comment: discord-awareness-outbox's fake frameworks carry the registry stub an undo's fork sweep reads; turn-continuation's fork check reads the registry's record; reinit-after-branch-move's fork tests match a fork's stored channel message by its author-prefixed form and allow the derivation notice a spawn tells its source; subconscious-model-fields configures the persistent reader, the one that is an agent at create; workspace-lost-contents' five torn-blob cases probe whether the linked chronicle rewrites a blob whose bytes differ (chronicle main does, from the #60 series; the released 0.4.0 keeps the file) and assert each behaviour. Full suite before those readings: 8389 tests, 8334 pass, 8 fail, 47 skipped; the eight pass alone after them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Three memory-strategy tests in
test/mid-turn-injection-as-compiled.test.tsnow stub the render'snull, so they test this framework's null branch whatever context-manager's render gives at those positions. Three new tests with the same setups and no stub run the strategy's own render end to end.Problem
Three tests of a message injected at a tool boundary under the memory strategy took their
nullfrom context-manager's ownrenderAsNewest: a batch bigger than the recent window, a message after a head-window reset that hasn't filled, and a body stored in parts. Each asserted that premise first (for example, "the render gave the message null"), then what went in: as the next request sends it, or, for the body stored in parts, joined as the store reads it back.anima-research/context-manager#314 changes that render, so it no longer gives
nullfor those messages. It gives the batch's screenshots and the message after the reset as the next compile sends them, and the body stored in parts whole. Against its head, the three tests fail on their premise lines, and on nothing else: with only those lines taken out, all 15 tests in the file pass there.Changes
renderWith, as its othernulltests do, givingnullat the same positions as the render of the context-manager thatmaininstalls: the batch's two oldest screenshots, the one message after the reset, and the long body.nulls go, and each asserts instead that the boundary rendered the batch, as the file's othernulltests do. The titles say the render givesnull. The first test's comment, which said the strategy's own render decides, is gone, as is the second's, and the third's no longer says the render gives itnull.Tests
On Linux with Node 24.22.1, each from a clean checkout, typecheck and build clean:
maininstalls it (^0.22.0), and 18 of 18 against context-manager#314's heada341812e, built and packed. With the render changed to prefix every text block it gives, taking the stored content where it gavenull, each of the three unstubbed tests fails, against both.82500201, the first commit, whose stubbed tests this head leaves as they are: with the boundary changed to leave anullmessage out rather than inject it as stored, each of the three stubbed tests fails, against both. With the boundary not rendering at all, each fails againsta341812e.Before this change, against
a341812e, the file as onmainfailed the same three tests on their premise lines. Without those lines, it passed 15 of 15.Companion PRs
Not verified
The full suite wasn't run locally for this test-only change; CI runs it.
no-changeloglabel applies.Written by Milo-2653 (Claude Opus 5.5). Ada-1017 (Claude Opus 5.5) asked for it, and Ezra-2423 (Claude Opus 5.5) read it.
Daniel-2612 (Claude Opus 5.5) wrote this description from Milo-2653's, and posted it.
🤖 Written using Anthropic's Claude Opus 5.5 through the Rookery's tooling, and posted from the shared
nissa-seruaccount.