Repository navigation
test(deferral): a sharded body's partway failure comes from the store's append - #684
Merged
nissa-seru merged 1 commit intoOct 22, 2026
Conversation
…'s append The two tests of a sharded body whose write throws partway made the throw by giving the strategy an onNewMessage that throws synchronously. That throw escaped the append only because of anima-research/context-manager#288. The fix for it, merged into context-manager's main after 0.18.0, logs the throw and lets the write stand, so against that main both tests fail. - throwOnceFromShardAppend wraps the agent's message store's append: the real append stores the shard, then the double throws once for the shard the test names, from inside the write, so context-manager's partway rollback runs as it did with the hook's throw. Only appends that carry a body group are offered to its matcher. - Both sharded tests use it. throwOnceFromStrategyHook, which nothing else used, is gone. - The second test also runs with one shard left landed (a throw at shard 1), for the text body and for the one with an image, so a body that kept a single shard is carried and compared too. - throwOnceAfterStoring's doc now says the hook's throw is caught too. Test-only: no changelog fragment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Automated review
Recommendation: merge. The review found nothing of medium or high severity.
Reviewed commit 402715f 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 PR only changes tests. It keeps two existing deferral tests working once context-manager's fix for #288 ships, and it doesn't change how the framework behaves.
4 reasons with sources
It fits the project.
- It changes one test file and leaves behavior the project already has alone. The diff only replaces how two existing sharded-body tests trigger a failure partway through a write. It swaps a strategy-hook double for a double around the message store's append and adds two cases where the failure comes at shard 1. Nothing in src/ changes. (test/deferred-window-only.test.ts)
- It declares the companion PR in context-manager and says why the merge order doesn't matter, as the guide asks. The guide wants cross-repo companion PRs declared with merge-order guidance. The body says the tests pass against both 0.18.0 and context-manager main, so this PR can land before or after a release. (CONTRIBUTING.md, How changes land)
- No changelog fragment is needed, since the change is test-only and carries the no-changelog label. Fragments are required for anything that affects behavior, and CI's check only applies when src/ is touched. (CONTRIBUTING.md, Changelog)
- The PR gives the evidence the guide asks for: test counts, a Not verified section, and proof that the tests can fail. It reports 68 of 68 passing against both context-manager versions and the full suite at 6,076 passing and 44 skipped. It also shows each test failing when its double doesn't throw. (CONTRIBUTING.md, What a PR should contain)
Findings
No findings of medium or high severity.
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.
The two deferral tests of a sharded body whose write throws partway now make that throw with a test double around the message store's append, inside the write. Until now they gave the strategy an
onNewMessagethat throws synchronously. That throw escaped the append only because of anima-research/context-manager#288, which anima-research/context-manager#293 fixed. Against context-manager'smain, which carries that fix, both tests fail. This is test-only: nothing insrcchanges.Problem
Two tests in
test/deferred-window-only.test.tscheck what the deferral does when a sharded body's write fails partway:Both made the write fail by giving the strategy an
onNewMessagethat throws synchronously. context-manager calls that hook from its own store listener, inside the append, so in 0.18.0 and earlier the throw escapes the append partway through the body, and context-manager removes the shards the write stored. anima-research/context-manager#293, merged into context-manager'smainafter 0.18.0 was released, catches and logs that throw and lets the write stand. With context-managermain(56ae8c3c) linked in place of the installed 0.18.0, both tests fail at agent-frameworkmain(0f6c6024), and the file's other 66 pass.The deferral's handling of a write that fails partway is unchanged. This changes only how these two tests make it fail.
Changes
throwOnceFromShardAppendwraps the agent's message store'sappend. The real append stores the shard, and then the double throws once for the shard the test names. Only appends that carry a body group are offered to its matcher, so an unsharded message never counts or throws. The throw comes from inside the write, after a shard is stored, so context-manager's partway rollback runs as it did with the hook's throw. That rollback (appendMessage'scatch, which removes what the write stored) is the same in 0.18.0 and on context-manager'smain. A double aroundaddMessagecouldn't do this, since it can only throw once the whole body is stored. One thing differs from the hook. The hook threw in place of the strategy's own handling of that shard, and its throw stopped the store's loop over its listeners, so neither the strategy nor any hostonMessagelistener heard the shard's add. With the double, the strategy and every listener hear the add, and in the first test listeners also hear the removal after it. The second test's removal fails. In the first, by our reading of context-manager'shandleMessageRemove, nothing calls the strategy for the removal, since it does that only when a removed message held a compression hold, and these tests place none. No host listener runs in these tests: the only one in agent-framework'ssrcis the history module's semantic indexer (src/modules/history/semantic.ts), and this file doesn't set up a history module.throwAt-th shard. The first test's log assertion looks for the double's error text, "the store failed partway through a body", in place of "strategy failed".asCompiledcarries as it is, went unchecked.throwOnceFromStrategyHookis gone, since nothing else used it.throwOnceAfterStoring's doc now says context-manager's fix for A synchronous throw from a strategy's onNewMessage escapes addMessage after the row is stored context-manager#288 catches the hook's throw too.Companion PRs
anima-research/context-manager#293 is merged into context-manager's
main, and no context-manager release carries it as of 2026-10-22. This change is safe to land before such a release, or without one: both tests pass against 0.18.0 as installed and against context-managermain. If such a release comes first, agent-frameworkmain's version of this file fails these two tests against it, as it does against context-managermaintoday, until this change lands. agent-framework's^0.18.0takes any 0.18.x on a fresh install.Tests
On Linux x86_64 with Node 24.22.0, from a fresh
npm install, which resolves@animalabs/context-manager0.18.0:npm run typecheckandnpm run buildare clean.test/deferred-window-only.test.tspasses 68 of 68 against 0.18.0, and 68 of 68 with context-managermain(56ae8c3c) built from source and linked in place of the installed one. At agent-frameworkmain, the same file passes 68 of 68 against 0.18.0, and fails the two sharded tests against context-managermain.main. The second also fails when the readback of what landed finds nothing: withstoredCopiesOfanswering nothing found, it fails, among 7. The first test's write leaves nothing stored, so that readback's answer doesn't change it. WithasCompiled's one-copy early return removed, the second test fails at its new case "a short text, an image, then the long text, throw at shard 1", against both.npm test: 6,120 tests, 6,076 passing, 44 skipped, none failing.Not verified
src/file, so it carries no fragment (no-changeloglabel).Written by Hamish-2699 (Claude Opus 5.5). Ada-1017 (Claude Opus 5.5) made the design calls, and Gordon-2414 (Claude Opus 5.5) reviewed it. Our internal review read it, and this head carries the fixes it led to.
Description checked and posted by Lottie-2661 (Claude Opus 5.5).
🤖 Written using Anthropic's Claude Opus 5.5 through the Rookery's tooling, and posted from the shared
nissa-seruaccount.