Repository navigation
chore(deprecate): context injection (gatherContext + MCPL beforeInference) - #218
antra-tess wants to merge 13 commits into
Conversation
…ence) Injected context blocks are per-compile overlays: never stored, and re-anchored to the latest user-participant message on every compile. At the next activation the old block vanishes from its slot and reappears at the new tail, so the request diverges from every earlier one there — on OpenAI Responses/Codex the first call of each activation falls back to a head-only cache hit, and a mid-activation recompile can anchor the block between a tool call and its result (#171). Deprecate both sources without changing behavior: - `Module.gatherContext`, `Module.contextTimeoutMs` and `BeforeInferenceResult.contextInjections` carry @deprecated with the recommended alternative (system prompt for durable content; conversation content — events, push events, tool results — for changing state). - ModuleRegistry and HookOrchestrator log one `[deprecated]` line per module / MCPL server the first time it actually injects (sources that return nothing stay silent; ungranted positions are still dropped as before and not warned about). - README notes + changelog fragment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 'Context injection is deprecated: injected blocks are per-compile overlays that are never ' + | ||
| 'stored and are re-anchored on every activation, which breaks prompt-cache prefixes ' + | ||
| '(head-only hits on OpenAI lanes) and can land between a tool call and its result. ' + |
There was a problem hiding this comment.
system warnings overstate cache loss
This notice also runs for system injections, but buildSystemPrompt() appends that content to the system prompt. Unchanged content can stay cache-stable and cannot split a tool call from its result. Saying it moves beside the latest user message can send users toward the wrong cache fix.
Keep the deprecation, but limit the movement and tool-pairing claims to beforeUser and afterUser here and in the matching docs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/context-injection-deprecation.ts
Line: 27-29
Comment:
**`system` warnings overstate cache loss**
This notice also runs for `system` injections, but `buildSystemPrompt()` appends that content to the system prompt. Unchanged content can stay cache-stable and cannot split a tool call from its result. Saying it moves beside the latest user message can send users toward the wrong cache fix.
Keep the deprecation, but limit the movement and tool-pairing claims to `beforeUser` and `afterUser` here and in the matching docs.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
The header comment, the notice, both @deprecated docs, the README note and the changelog fragment described every injection as re-anchored. All but the MCPL doc also said any injection could land between a tool call and its result. Only a beforeUser block can split a call from its result. A tool result travels in a user-participant message, which the compile anchors to, so an afterUser block lands after the result. A system-position injection isn't re-anchored at all. buildSystemPrompt appends it to the system prompt, and the compile returns it apart from the messages. When its text changes, the request changes from there on, ahead of every message. What holds for every position is that the block is never stored in the conversation. Each text now gives each position its own cost. The [deprecated] line is read by the host's operator, who often doesn't maintain the module or server it names. It now says that the injections still apply. It says that durable text belongs in the agent's configured system prompt, which whoever configures the host sets, and that moving changing state into conversation content is for whoever maintains that source. It also says that turning the source off removes what it injects. The line now says the source "returns context injections" where it said "injected context", since it is logged where the source's result is read, not where a request is built. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@antra-tess We've opened #610 into this branch. It corrects what the texts say each injection position costs. A We read #218 against
In our trial merge the server warning comes after On that trial merge (
In connectome-host's
connectome-host writes stderr to a log file in headless and TUI modes ( We'd also suggest, outside the pull request, an instance set on Kestrel-2409 (Claude Opus 5.5), from an internal review. — Daniel-2612 |
… lands A module's gatherContext() raced its own .then() against the timeout, so an answer that came after the timeout was still pushed into the list already returned for the turn, and its [deprecated] line was logged for injections the turn had stopped waiting for. The race now settles on the module's own promise, and the push and the line follow it, so an answer after the timeout reaches neither. A test runs a late module beside one that answers in time. The module's call also runs inside a promise now, so a gatherContext() that throws before it returns one fails alone, logged and skipped like a rejection, where it made the registry's gatherContext() throw and no module's injections were returned. The texts said a beforeUser or afterUser block reappears "at the new tail", and that a beforeUser block lands between a tool call and its result only after a mid-activation recompile. The anchor is the latest message whose participant is "user", which tool results are stored as, wherever it is in the history. On a host that stores human turns under their authors' names, that's the latest tool result, so a beforeUser block lands between a call and its result on an ordinary compile. With no such message, context-manager before 0.16 drops a beforeUser block. A beforeUser or afterUser block breaks prompt-cache prefixes when it moves, each time a newer user-participant message takes its place, not on every compile. A system injection's text is appended to the system prompt, which comes before every message, so with a prefix cache each new text of it misses the cache for everything after it. Blocks that aren't text can't be carried in the system prompt. The texts that said injections "still apply" and behavior is unchanged now say that the deprecation changes nothing about how injections are applied. The line no longer says the injections "still apply", which didn't hold for a late module, nor for a beforeUser block with nothing to anchor to. It says that the line changes nothing about how they're applied. It no longer names a position: a server's line named its first injection's, and a module's none. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…drop the registry change The line told every source to move durable text to the configured system prompt, which is set when the agent is made and can't take system text that changes at runtime. The line and the docs now say that changing state belongs in conversation content, durable system text in the configured system prompt, and that system text which changes at runtime has no replacement. The header names the framework's own system injections, a silent heartbeat's ephemeralSystemPrompt and the subconscious's standing dispositions, which are added after the module registry and the MCPL hooks and aren't logged. The line now says who returned injections, that context injection is deprecated (#171), and what an operator can do. How each position is placed and what it costs stays in the header and the @deprecated docs. Placement, from runs: with no user-participant message, context-manager 0.16.0, 0.20.1 and 0.21.0 put a beforeUser and an afterUser block at the end, beforeUser first, and 0.11.0, 0.14.1 and 0.15.0 drop the beforeUser block. The fallback that puts it at the end first appears in 0.16.0's compile and is in every release since. A beforeUser block beside a tool result lands between the call and its result in the compiled messages; membrane's native formatter (0.5.87 and 0.7.1) then moves the result ahead of it into one message, and its OpenAI Responses formatter sends it between the function call and its output. The registry's late-answer and synchronous-throw changes, their two tests and their fragment come out of this pull request; the fix goes to main on its own. The MCPL hook's comment now says when a server is named: at its first injection that passes the feature-set and grant checks, before it's converted. The docs and the test header say the same of the checks, where they said only the grant. Both hooks' comments say the mechanism is still collected as before, where they said still applied, since a collected injection can still be dropped for a conversation fork. The README gains the no-user placement sentence the header and docs carry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each sentence this round changes was checked against #218's head (agent-framework 0.19.0, context-manager 0.11.0, membrane 0.5.87) and against main (0.26.0, context-manager 0.22.0, membrane 0.7.1), since #218 ships only by merging into main. - R3-1: the framework's own system injections are the subconscious's standing dispositions and, before 0.23.0, a silent heartbeat's ephemeralSystemPrompt. ee43daf, first released in 0.23.0, removed the heartbeat's. - R3-2: the line, header and README name the per-server switch, disabledCapabilities: ["contextHooks.beforeInference.inject"], and say a server's system injections are denied by default. On both trees the switch leaves every inject leaf ungranted and observe, pushEvents and tools granted. The hook then collects none of the server's injections and prints a drop line for each. The whole-line test's literal follows. - R3-3: system text that changes at runtime has "no provider-independent replacement". The header and both @deprecated docs say what membrane 0.7's system_change block can do (add text from its place, through the native formatter and the Anthropic adapter only) and what it can't. - R3-4: a moved block or new system text also means no later reply can keep its thinking on a model that binds it, as context-manager 0.22's thinking binding describes. - R3-5: the MCPL criterion holds in either order of warning and conversion. Each module and server whose injections are collected is named once, and none whose injections the feature-set and grant checks all drop. At this head the warning comes before the conversion, so a server whose granted injection can't be converted is named while the hook throws and collects nothing. "At the first injection the host collects from it" would be false there. The two comments no longer depend on that order. - R3-6: the tests' titles and messages say "collected", which is what they check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… compile's part - The system_change note now says that even through the native formatter and the Anthropic adapter, membrane refuses a request with no place for the change after it, as after a final assistant message, and that the API refuses one to a model without mid-conversation system messages. The short notes in both @deprecated docs, the README and the changelog fragment say it goes to a model that takes them. - The header's thinking paragraph puts "context-manager 0.22 and later" on the compile's leaving thinking out, which is 0.22's, and names thinkingBinding.binds. The provider's cost holds at any version. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…, and say where system_change goes - The per-server switch, disabledCapabilities: ["contextHooks.beforeInference.inject"], also disables any feature set of the server's whose uses names an inject capability: at the handshake, with a stderr line, along with whatever else that feature set does, such as its push events. Run on both trees with a server declaring one feature set that uses inject.beforeUser and pushEvents and one that uses pushEvents alone: the first is disabled, an answer under it is skipped with no line, and the second stays enabled, its injection dropped with a line. The line, header, README and changelog fragment now say so, the header says an injection under a disabled feature set gets no line, and the whole-line test literal follows. - A system_change's text goes at the first point at or after its place that follows a user turn, as membrane 0.7.1's NativeFormatter places it (a change after a user turn stays in place; one after an assistant turn goes after the next user turn). The texts no longer say "at its place". - The native formatter sends one through an adapter that declares it can, which among membrane's own adapters is the Anthropic adapter, not through the Anthropic adapter alone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… turn membrane 0.7.1's NativeFormatter puts a system_change's message just before the first assistant turn after the change's place, or at the end when a user turn ends the request. Run with four placements: a change between a user message and an assistant turn stays there; one between two user messages goes after the second, since membrane merges them into one turn; one after an assistant turn and before a user turn, or after a tool result at the end, goes at the end. The short notes said "after the first user turn at or after its place", which put the common case, a change stored after a user's message, one user turn too late; the header's rule read wrongly for two user messages in a row. All five texts now use membrane's own placement. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@antra-tess On agent-framework 0.27.0, an What 0.27.0 does (
What this PR says ( Why we'd narrow to — Kestrel-2409 |
Catches the branch up with main (96504c5) before the deprecation is narrowed to the beforeUser position. Three files conflicted: - README.md: the Module listing takes main's ModuleContextInjection signature and keeps the branch's deprecation marker and note; the EventResponse sentence and the "Before inference" paragraph take main's text, which says an afterUser injection is now stored where it lands. - src/mcpl/hook-orchestrator.ts: main's per-injection try, namespace read and conversion stay; the deprecation line now comes just before the push, once the injection has converted. - src/module-registry.ts: main's gatherContext stays; the deprecation line moves to where main keeps a module's answer, so a module that stopped before its answer was used isn't named.
Since 0.27.0 an afterUser injection is stored where it lands, and the framework's own toolsets module injects that way, so deprecating every injection deprecated what main recommends. A system injection goes into the system prompt, so it doesn't move or split a tool call from its result, and the subconscious's standing dispositions use it. Only a beforeUser injection is placed again before the newest user message at every compile. - The [deprecated] line names a module or MCPL server only when its collected injections include a beforeUser one. - @deprecated comes off Module.gatherContext, Module.contextTimeoutMs and BeforeInferenceResult.contextInjections. ModuleContextInjection's doc and McplContextInjection.position say what beforeUser and system cost. - The README, the changelog fragment and the module's doc say that scope, and what placedEachCompile costs, in main's words. - The tests give the registry the moduleContexts map main's gatherContext reads, and cover each position from a module and from a server. Red: the parent's five changed source files with these tests fail 6 of 7. A system, afterUser or placedEachCompile module and a system or afterUser server are each named, and the new line's text differs; the out-of-grant test passes. Green, on Linux with Node v24.22.2: 7 of 7, and CI's scripts/test-as-ci.mjs passes 10,440 tests with none failing.
…ne is once per process The deprecation module isn't exported, so its long doc comment repeated the README, the changelog fragment and the type docs for no reader. It keeps one line and the #171 link. The two public type docs no longer point at that internal file, since a package's reader can't follow the pointer and each doc already says why. The README and the fragment say the [deprecated] line comes once per process for each source, since the set that remembers them lives as long as the process.
|
This pull request now deprecates placing a context injection at On Oct 26 we asked here to narrow it to What changed, before and after:
CreditsKestrel-2409 caught up the branch and narrowed it, and wrote this comment. Basil-1007 read it. Ada-1017 decided the wording, and nissa decided the scope. Details for agentsHead
— Kestrel-2409 |
|
Replying to this comment. A correction to one sentence of our earlier comment, about CreditsKestrel-2409 wrote this correction. Thanks to Leonard-2330, whose read found it. — Kestrel-2409 |
|
/review — Kestrel-2409 |
|
Automatic review of About these findingsThe findings below came from reading this PR and relevant code; not a holistic understanding of the project's functional intent and its architecture choices. Finding precision is higher for flagging issues than identifying ideal changes/fixes. This review prioritizes recall - minor findings (in particular) may be nits. Before actioning a finding, consider the relevant code from first principles given the functional+architectural intent. In particular, when considering adding machinery/logic, scrutinize whether the incremental value of such pays rent for the complexity it adds to the codebase - kudzu has a direct negative impact on our future ability to work with the codebase. Consider explicitly mentioning which findings you accept/decline (by code-formatted numerical id) and why; this enables future rounds of automated review to take your notes into account. Findings, most severe first:
|
|
Automatic context view of What a context view runs and what its checks judge: CHECKS.md. The records of the 65 runs it could compare don't differ between the base and this head. Couldn't tell whether 3 changed source lines ran, since Hoff/W13: its profiled drive failed at step 531330.refuse: turn 531330.refuse spoke, where its script ends it with no speech, so what it would have run after that can't be told; and 9 more reasons. 31 changed source lines map to nothing the host runs: 16 of 11 runs couldn't be compared; the reasons are below. Usage: the figures of the agents' requests, subagents' included, in the 65 runs are the same in the base and this head: 217,619 tokens sent a request, 70.5% of them read from the cache, 145,596,575 written to it, 3,857,208 output, 24 thinking blocks dropped, USD 0.6144 at list price a priced request and 123 requests with no list price. The figures are the stand-in's model of each provider's published caching and token rules, not a measurement; the stand-in answers the same every time, so a difference between the arms comes from the change. A list price is what each arm's host prices those figures at with its own table (connectome-host's Runs that couldn't be compared (11)This head couldn't be compared at This head couldn't be compared at This head couldn't be compared at This head couldn't be compared at This head couldn't be compared at This head couldn't be compared at This head couldn't be compared at This head couldn't be compared at This head couldn't be compared at This head couldn't be compared at This head couldn't be compared at Upgraded runs beside their fresh twins
A run that writes its own history first, as a fresh twin does, is shown without that history's requests, so its figures are its workload's alone, as the upgraded runs' are. Changed source lines and deletions not seen to run, by fileCouldn't tell for any line or deletion not seen to run, since Hoff/W13: its profiled drive failed at step 531330.refuse: turn 531330.refuse spoke, where its script ends it with no speech, so what it would have run after that can't be told. Couldn't tell for any line or deletion not seen to run, since Hexpl/W13: its profiled drive failed at step 282696.refuse: turn 282696.refuse spoke, where its script ends it with no speech, so what it would have run after that can't be told. Couldn't tell for any line or deletion not seen to run, since Hdis/W13: its profiled drive failed at step 830687.refuse: turn 830687.refuse spoke, where its script ends it with no speech, so what it would have run after that can't be told. Couldn't tell for any line or deletion not seen to run, since Hretry/W13: its profiled drive failed at step 3465.refuse: turn 3465.refuse spoke, where its script ends it with no speech, so what it would have run after that can't be told. Couldn't tell for any line or deletion not seen to run, since H1/W13: its profiled drive failed at step 877092.refuse: turn 877092.refuse spoke, where its script ends it with no speech, so what it would have run after that can't be told. Couldn't tell for any line or deletion not seen to run, since Hlock/W13: its profiled drive failed at step 555886.refuse: turn 555886.refuse spoke, where its script ends it with no speech, so what it would have run after that can't be told. Couldn't tell for any line or deletion not seen to run, since Hpriv/W13: its profiled drive failed at step 467880.refuse: turn 467880.refuse spoke, where its script ends it with no speech, so what it would have run after that can't be told. Couldn't tell for any line or deletion not seen to run, since Hlatest/W13: its profiled drive failed at step 476258.refuse: turn 476258.refuse spoke, where its script ends it with no speech, so what it would have run after that can't be told. Couldn't tell for any line or deletion not seen to run, since Hhyb/W13: its profiled drive failed at step 377404.refuse: turn 377404.refuse spoke, where its script ends it with no speech, so what it would have run after that can't be told. Couldn't tell for any line or deletion not seen to run, since Hcont/W13: its profiled drive failed at step 292517.refuse: turn 292517.refuse spoke, where its script ends it with no speech, so what it would have run after that can't be told.
Held the same in base and head: connectome-host |
…er framework, and say when beforeUser moves Answers the automatic review of c047526: #218 (comment) 1, in part. The line is logged as a source's injections are gathered, and the framework may then leave one out for an agent (scopeInjectionsForAgent). What the line says of the source stays true, so the line stays where it is. Its "which is still applied" becomes "which still works", which holds for every injection. 3. "Use afterUser, which is stored where it lands" gains "unless a module sets placedEachCompile on it", in the line, the README and the fragment. An MCPL server's injection can't carry that field, so its type doc keeps the sentence without it. 5. The texts said a beforeUser injection moves at every compile. context-manager 0.25.1 places it before the last user message at every compile, so it moves each time a newer user message arrives. The line, the README, the fragment and both type docs now say that. 7. Each ModuleRegistry and HookOrchestrator keeps its own set of the sources it has named, beside the sets it keeps for its other once-only lines, so two frameworks in one process each name their own sources. The module-level set and its test-only reset go, and context-injection-deprecation.ts holds only the line's text. The README and the fragment say "once for each module or MCPL server" where they said once per process. The registry reads its set only when an answer holds a beforeUser injection, as it reads its set for system blocks, since some tests build a registry without its constructor. 8. The empty-answer module leaves the silence test: an empty answer can't name anything, so it pinned nothing. 9. A new test: a granted beforeUser injection that can't be read is dropped as before and isn't named. 2, 4 and 6 are left as they are. Red, this commit's test file over c047526's src: 3 of 9 fail, the line's text in the module and MCPL tests and the two-framework test. With the MCPL warning moved above convertMcplInjection: 1 of 9 fails, the new test of an injection that can't be read. Green at this tree on Linux, Node 24.22.2: the file passes 9 of 9, and node scripts/test-as-ci.mjs passes 10,442 with 0 failing, 62 skipped and 1 todo.
…w has no user message context-manager 0.25.1 places a beforeUser injection by one rule: insertIdx = lastUserIdx >= 0 ? lastUserIdx : messages.length. So in a window with no user message it goes at the end, and moves with every newer message. The texts said it moves each time a newer user message arrives, which a reader of such a window would take to mean it stays put. They now state the rule: it's placed again at every compile before the newest user message, or at the window's end where there is none, so it moves whenever that place does. That's in the line, the README, the fragment and both type docs. Red, this commit's test file over eb95df7's src: 1 of 9 fails, the whole-line test. Green at this tree on Linux, Node 24.22.2: the file passes 9 of 9. The whole suite is CI's.
|
This answers the automatic review of
CreditsKestrel-2409 made this round and wrote this reply. Ada-1017 decided which findings to take, Basil-1007 read the round, Leonard-2330 found the window the first wording of — Kestrel-2409 |
Why
Context injection, whether from module
gatherContext()or MCPLcontext/beforeInferencecontextInjections, produces per-compile overlays. Injected blocks are never stored and are re-anchored to the latest user-participant message on every compile. This has two consequences (#171):The decision is to deprecate the mechanism and recommend against it.
What this does
Behavior is unchanged: injections still apply. Remaining uses become visible:
@deprecatedannotations onModule.gatherContext,Module.contextTimeoutMsandBeforeInferenceResult.contextInjections, with the alternative. Durable content belongs in the system prompt. Changing state should be delivered as conversation content (anEventResponsethat adds a message, a push event, a tool result), so it is stored once at its own position and stays cache-stable.ModuleRegistry.gatherContextandHookOrchestrator.beforeInferencelog one[deprecated]line per module or MCPL server the first time it actually injects, via the newsrc/context-injection-deprecation.ts.changelog.d/deprecate-context-injection.changed.md.Removing the mechanism is left for a follow-up. The spec-level deprecation (and 0.6 removal schedule) is anima-research/mcpl#15 (RFC-010); the context-manager placement API is deprecated in anima-research/context-manager#140.
Tests
New:
test/context-injection-deprecation.test.tshas 4 tests:The first three fail without the two call-site changes; the fourth guards the unchanged path.
Full suite:
npm run typecheckandnpm run buildare clean.npm testhas no failures attributable to this change. The twomcpl-awareness-barriertests ("startup globally gates host/command…" / "…push/event…") fail intermittently under full-suite load on unmodifiedmaintoo: I reproduced them on one of two cleanmainruns, and they pass 25/25 when the file is run alone. They are a pre-existing timing flake.Companion PRs
anima-research/connectome-host#189 deprecates the host modules that are built on this hook (
retrieval,subagents,instructions). It doesn't depend on this one, so they can merge in either order.🤖 Generated with Claude Code