Skip to content

test(deferral): a sharded body's partway failure comes from the store's append - #684

Merged
nissa-seru merged 1 commit into
anima-research:mainfrom
nissa-seru:fix/sharded-throw-store-seam
Oct 22, 2026
Merged

nissa-seru merged 1 commit into
anima-research:mainfrom
nissa-seru:fix/sharded-throw-store-seam

Conversation

@nissa-seru

Copy link
Copy Markdown
Contributor

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 onNewMessage that 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's main, which carries that fix, both tests fail. This is test-only: nothing in src changes.

Problem

Two tests in test/deferred-window-only.test.ts check what the deferral does when a sharded body's write fails partway:

  • "at a tool boundary, a sharded body whose write throws partway lands nothing: it waits for the turn end with what follows, and is stored whole then";
  • "at a tool boundary, when removing the shards of a body whose write threw fails too, the shards that stayed are carried as the next compile renders them".

Both made the write fail by giving the strategy an onNewMessage that 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's main after 0.18.0 was released, catches and logs that throw and lets the write stand. With context-manager main (56ae8c3c) linked in place of the installed 0.18.0, both tests fail at agent-framework main (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

  • throwOnceFromShardAppend wraps the agent's message store's append. 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's catch, which removes what the write stored) is the same in 0.18.0 and on context-manager's main. A double around addMessage couldn'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 host onMessage listener 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's handleMessageRemove, 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's src is the history module's semantic indexer (src/modules/history/semantic.ts), and this file doesn't set up a history module.
  • The two sharded tests use it, at the same shards as before: the first test at the body's first shard, the second at its 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".
  • 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. Its cases left two or three shards before, so a body that kept a single shard, which asCompiled carries as it is, went unchecked.
  • throwOnceFromStrategyHook is 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-manager main. If such a release comes first, agent-framework main's version of this file fails these two tests against it, as it does against context-manager main today, until this change lands. agent-framework's ^0.18.0 takes 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-manager 0.18.0:

  • npm run typecheck and npm run build are clean.
  • test/deferred-window-only.test.ts passes 68 of 68 against 0.18.0, and 68 of 68 with context-manager main (56ae8c3c) built from source and linked in place of the installed one. At agent-framework main, the same file passes 68 of 68 against 0.18.0, and fails the two sharded tests against context-manager main.
  • Each test fails when its double doesn't throw, against 0.18.0 and against context-manager main. The second also fails when the readback of what landed finds nothing: with storedCopiesOf answering nothing found, it fails, among 7. The first test's write leaves nothing stored, so that readback's answer doesn't change it. With asCompiled'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

  • Against a real store failure partway through a body: the tests use the double, as they used the strategy hook before.

  • This change is test-only: it changes one test file and no src/ file, so it carries no fragment (no-changelog label).

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-seru account.

…'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>
@nissa-seru nissa-seru added the no-changelog Exempts this PR from the changelog check (internal-only change) label Oct 22, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nissa-seru
nissa-seru merged commit 5daf344 into anima-research:main Oct 22, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog Exempts this PR from the changelog check (internal-only change)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant