[nissa-seru]

ModuleRegistry.gatherContext() no longer lets a module's answer after its timeout reach the turn, and a module whose gatherContext() throws before returning a promise is skipped alone. Both bugs are on main today.

Problem

gatherContext() races each module's call against its budget, but the push into the list it returns runs in the call's own .then. So a module that answers after its timeout is still pushed into that list, in one of two windows:

• Before the list is returned, while another module is still awaited within its budget. The late answer is then in the list the registry returns, so it reaches the turn as an answer in time would.
• After the list is returned. For an agent that isn't a conversation fork, scopeInjectionsForAgent returns that same list, and startAgentStream keeps it (injections = moduleInjections) while it awaits the MCPL servers' beforeInference, which are asked in parallel with a 5 s timeout each. So when another module answered in time and an MCPL server is consulted, a late answer that arrives during that wait reaches the turn's request. A conversation fork gets a filtered copy, which the late push doesn't reach.

The modules this hits are the slow ones gatherContext's doc names, such as a retrieval module that times out every turn. A late answer whose system blocks aren't text also prints the line saying an agent given the injection reads a line in place of each such block.

The module's call also runs outside any promise. A gatherContext() that throws before returning one leaves the registry's gatherContext(), and the turn goes without every module's injections. So does one that returns an array without a promise, since the registry calls .then on it. And an answer that isn't an array is spread into the list, so a string adds one entry per character.

Changes

• The timeout marks the module as timed out, and its answer adds nothing to the list after that, nor prints the line about blocks that aren't text.
• The call is made in place, inside a try/catch, before its timer is set, and its result goes through Promise.resolve. A throw becomes a rejection, logged and skipped like any other, and an array returned without a promise is taken as the module's answer. As on main, each module's budget starts once its own call returns, after its synchronous work and every earlier module's, and a later module's synchronous work still counts against it.
• An answer that isn't an array is logged and skipped like a rejection, with or without a promise.
• The method's doc says each of these.

Merge notes. Onto main, this change gives #782 three conflict regions in src/module-registry.ts, where it has none with main alone: #782 reshapes this loop with the same flag and the same in-place call. #218 changes the loop too, and has five conflict regions with main with or without this change. Every other open pull request into main merges the same with or without it.

What this leaves

#788 tracks three things this change leaves as they are on main, each with a run there:

1. A module past its gatherContext budget is called again every turn while its earlier calls keep running.
2. In notifyToolBatchComplete(), one batch hook's synchronous work spends the next hook's bound.
3. A malformed entry in an answer that arrives in time is kept before noteBlocksNotShown() throws on it, so the module is logged as failed while its entries stay in the list. In feat(context): store each afterUser context injection where compile() placed it, so it stays; a module's queryMessages and addMessage take forAgent #782's version of this loop, the answer must be an array of objects, and noteBlocksNotShown() reads it before it's kept.

What each part is for

• The timedOut flag. Without it, the late push lands in the list in either window. A flag read at the push is the smallest change that stops it, and it keeps the push and the line about blocks that aren't text in the module's own .then, after the check, as before. It's per module, so one module's timeout doesn't discard another's answer.
• The call in place, in a try/catch. Without the catch, one throwing module costs every module's injections for the turn. Calling it in place rather than deferring it to a microtask keeps each module's budget starting after its own call. Deferred, every timer would start before any module ran, so one module's synchronous work would spend the next one's budget.
• Promise.resolve around the call. Without it, a module that returns an array without a promise throws out of the loop at .then, the same loss as a throw.
• The array check. Promise.resolve takes whatever a module returns, so without the check a string returned without a promise would be spread into the list, as one returned with a promise already is on main. The check skips that module alone.
• The test helpers.
  • loggedErrors collects what the registry logs and puts console.error back even when the run throws;
  • DelayedModule gives the turn tests a module that injects one text block after a set delay;
  • the turn test's stand-in MCPL server answers beforeInference 150 ms later and counts its calls, so the test can show the turn waited for it;
  • withImage gives the image test modules whose injection is one image block;
  • that test sets the registry's notShownWarned set, since makeRegistry builds the registry with Object.create, which skips field initializers.

Tests

On Linux, Node 24.22.1, at this commit:

• npm run typecheck and npm run build are clean;
• test/gather-context-timeout.test.ts and the new test/gather-context-late-answer.test.ts pass 15 of 15;
• npm test passes 7,030 of 7,074, with 44 skipped and none failing.

The new tests:

• gather-context-timeout:
  • A late module beside a timely one adds nothing to the returned list, checked 200 ms after the list is returned, about 130 ms after the late answer.
  • A late answer that comes while a slower module is still awaited adds nothing to the list, and the slower module's answer is in it.
  • A late answer's image block prints no line about the system prompt, while a timely module's does.
  • A module that throws synchronously is skipped and logged while the other injects.
  • A module that returns an array without a promise has it taken.
  • A module that answers with a string, with or without a promise, is skipped and logged, and no characters reach the list.
  • A module that blocks for 80 ms and then needs 20 ms, ahead of one needing 20 ms, all with 50 ms budgets, leaves both answering.
• gather-context-late-answer:
  • A turn whose MCPL server answers beforeInference 150 ms later, through a real HookOrchestrator, with a module that answers at once and one that times out at 50 ms and answers at 120 ms: the late answer isn't in the turn's system prompt, and the server was asked once and answered.
  • A turn with the same late module beside one that answers in time at 200 ms: the late answer isn't in the system prompt, and the slower one's is.
  • A turn with a module that throws: the other module's injection is in the system prompt.

Each new test's timing rests on timers firing in due order, not on margins.

Red-first and removals, each over both files, with src/module-registry.ts replaced:

• by main's: 9 fail, every new test but the budget test, which main's in-place call already passes;
• with the call deferred with Promise.resolve().then(...): 1 fails, the budget test;
• with the budget timer set before the call: 1 fails, the budget test;
• with the timedOut check removed: 5 fail, the five late-answer tests;
• with the flag declared once for the whole loop: 2 fail, the two slower-module tests;
• with the "not shown" line moved above the check: 1 fails, the image test;
• without Promise.resolve: 2 fail, the array-without-a-promise test and the string test, whose answer without a promise throws out of the loop at .then;
• without the array check: 1 fails, the string test.

---

• Changelog fragment added — changelog.d/gather-context-late-answer.fixed.md.

Written by Kestrel-2409 (Claude Opus 5.5), from @Anarchid's R2-2 and R2-3 on #610, after an internal review. Abigail-2676 (Claude Opus 5.5) read it.

Description checked and posted by Daniel-2612 (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.

[github-actions]

Automated review

Recommendation: merge and fast-follow. 1 medium severity finding can be fixed after the merge.

Reviewed commit 2b72d2a 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 is a bug fix to the module registry's existing fail-open gatherContext() behavior. It changes only that method and comes with tests and a changelog fragment, as the project's conventions ask.

5 reasons with sources

• It fixes behavior the project already promises: modules that time out or fail are skipped, and one module's failure doesn't cost the others their context. On main, the method's doc already says "Fail-open: timed-out or erroring modules are skipped". On main, a late answer is still pushed into the list and a synchronous throw escapes the loop. Both break that promise, and the diff makes both behave as documented. (src/module-registry.ts (gatherContext doc comment); README.md (context injection section))
• The change stays inside ModuleRegistry.gatherContext, the component that owns per-module context gathering. It adds no dependency, no public interface and no new concept, and nothing outside this method changes. Accepting a plain array without a promise is a tolerance inside the registry. It is not a new contract: the Module interface in README.md still declares Promise<ContextInjection[]>. The review findings already cover the mismatch with the interface. (src/module-registry.ts)
• Tests that can fail on the old code come with the change, which CONTRIBUTING asks for. The new tests cover a late answer in each of its two windows, a synchronous throw, a plain array return and a non-array answer. The description gives numbers: 7,030 passed and 44 skipped out of 7,074. (CONTRIBUTING.md (What a PR should contain))
• A changelog fragment is included in the required format. The file is changelog.d/gather-context-late-answer.fixed.md and contains bullet entries only. (CONTRIBUTING.md (Changelog))
• The description declares where it overlaps open PRs, as the contributing guide asks. The description notes conflicts with #782 and #218, which touch the same loop. (CONTRIBUTING.md (How changes land))

Findings

1 finding of medium or high severity, see the inline comment:

• Medium severity. The array check is shallow, so a malformed element is pushed and then logged as if the module were skipped.

2 more findings, less severe

• Low severity, documentation, src/module-registry.ts:447. The registry doc says an array returned without a promise is accepted, but the Module interface still requires a Promise. The new doc comment on gatherContext says "An array returned without a promise is taken as the module's answer". Module.gatherContext in src/types/module.ts:164 is still typed Promise<ContextInjection[]> and its doc says nothing about this. Module authors reading the interface can't tell whether returning a plain array is supported or only tolerated. The two should say the same thing, either by widening the type to ContextInjection[] | Promise<ContextInjection[]> or by calling it a tolerance rather than a contract.
• Low severity, maintainability, test/gather-context-late-answer.test.ts:40-44. The console.error capture is copied into each test instead of using a shared helper. Each of the three tests in gather-context-late-answer.test.ts repeats the same five lines to swap console.error and restore it in t.after. The same change adds the loggedErrors helper for exactly this job in gather-context-timeout.test.ts. Moving the capture into a shared helper in test/helpers/ would remove the copies and keep the restore logic in one place.

[github-actions]

Medium severity. The array check is shallow, so a malformed element is pushed and then logged as if the module were skipped.

The new check only tests Array.isArray(r). Then injections.push(...r) runs before noteBlocksNotShown(module.name, r). If an answer is an array holding null, or an injection with no content array, noteBlocksNotShown throws on injection.position or injection.content.some. That happens after the push, so the .catch logs [module] gatherContext: TypeError…, the same line printed for a module that is skipped. But the bad entries are already in the list returned to startAgentStream, where they can break the turn further on. The change says a malformed answer is "logged and skipped like a rejection", and that only holds for the top-level type. Checking each element, or at least running the check and noteBlocksNotShown before the push, would make the skip real.

[nissa-seru]

The medium finding, a malformed entry kept before noteBlocksNotShown() throws on it, is the same on main, and this pull request keeps that order. It's item 3 of #788, with a run on main. In #782's version of this loop, the answer must be an array of objects, and noteBlocksNotShown() reads it before it's kept.

— Daniel-2612