♻️ refactor: Export the Summarization Primitives - #450
Conversation
|
Marco, I dug through this PR, the current manual-compaction branch in LibreChat (#15089), the agents summary injection/accounting path, and DeepSeek Harness's current compaction design. My conclusion is: your ownership decision is right, but I don't think these five exports are the best final interface. The problem you identified is real. Where I think the current shape falls short is module depth.
That removes literal duplication, but callers still have to know how to compose the actual invariants. The deletion test says this module earns some keep—the same rules otherwise reappear in two callers—but its interface is nearly as complex as its implementation, and it does not yet create locality for the summary contract. The most important example is
There is also a changed premise to resolve before sharing the prompts. LibreChat #15089 currently has deliberately different DeepSeek Harness reinforces the ownership direction but not the primitive-export shape:
I don't think agents should copy DeepSeek's plugin kernel or force LibreChat's HTTP/billing/persistence flow through the live graph node. The useful lesson is narrower: share policy and summary-boundary semantics behind a deep module; keep the two operational adapters separate. LibreChat genuinely owns locks, branch traversal, billing, persistence and multi-pass host behavior. Agents owns prompt/update semantics, the persisted summary carrier, and its accounting contract. My recommended revision before treating this as the completed architecture:
So: strong agreement with the intent and location; strong concern with the current shallow public interface. If this must land as a small unblocker, I would frame it as a preparatory step paired immediately with the LibreChat consumer and contract verification—not the final summarization seam. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c30a081778
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@berry-13 fix the merge conflicts when you can |
The summarize node keeps these to itself, so a caller that compacts a conversation outside a run has to reimplement them and then drift from them: LibreChat carries its own copy of the wrapper overhead constant, the checkpoint prompts, the parameter split, and the instruction builder. Move them to a module the barrel re-exports, leaving the node importing what it used to define. buildSummarizationInstruction now trims the prior summary so a blank one asks for a fresh checkpoint instead of a consolidation of nothing.
SUMMARY_WRAPPER_OVERHEAD_TOKENS claimed 33 tokens for a carrier that measures 48 on o200k_base and more on Anthropic, and its JSDoc described wrapper text that has never existed in the tree. Nothing could catch either, because the number lived in the summarize node while the string it was measuring lived in AgentContext. Replace it with buildSummaryCarrierText, which both sides now call. The node sizes a summary by measuring the message it will actually inject, and AgentContext builds that message from the same function, so the two can no longer disagree. A caller with no tokenizer, which is one of the conditions overflow recovery summarizes under, estimates from the same string rather than reserving nothing. Stop consulting the provider's output_tokens for the stored count. On a reasoning summarizer it includes hidden thinking that never reaches the checkpoint, so every later context calculation reserved room for tokens that are never sent. Provider usage stays a billing input. Narrow the barrel to the three names a caller compacting outside a run actually needs. The two default prompts stay internal: LibreChat's manual flow deliberately words its own, so exporting them would publish an API with no consumer. The langfuse routing fixture sizes a message by its character length and read the summary as a SystemMessage, which that counter flattens to 1, so a correctly measured carrier no longer fits its 120 token budget. Its ceiling now sits between the carrier and the transcript.
c30a081 to
412ce10
Compare
The no-counter branch estimated the carrier at four characters per token. That is a mean for English prose, not an upper bound, and this branch is the one that matters: shouldSummarizeOverflow fires precisely when there is no counter, so the count it produces is persisted and then reserved by instructionTokens on the overflow retry. Undercounting there is what makes the retry overflow again. Measured against o200k_base and Anthropic's tokenizer, four characters per token understates base64 by 1.5x and Korean by 4.6x, while coefficients large enough to cover those overestimate English prose by roughly 4x. No character heuristic is both safe and useful, so drop it and fall back to the tokenizer this package already bundles, picking the encoding from the summarizer's model. Provider output_tokens stays out of it. On a reasoning summarizer it includes hidden thinking that never reaches the checkpoint, so using it as a floor would persist a reservation many times the summary's real size.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11d2740fb2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The fallback picked its encoding from summarizationConfig.model, which is
the summarizer, not the model the carrier is re-injected into. That model
is undefined for ordinary self-summarization, so an Anthropic agent fell
through to encodingForModel('') and measured itself with o200k_base, and
a dedicated cheap summarizer could point the encoding at a different
provider entirely.
The count is spent by AgentContext.instructionTokens against the agent's
own context window, so it has to be denominated in the agent's tokenizer.
Anthropic counts run well above o200k_base on the same text: a Korean
summary measures 163 tokens against claude and 100 against o200k_base, so
the wrong encoding under-reserved the overflow retry by 39 percent.
Take the model from the agent's clientOptions, mirroring how Run.create
picks the encoding for the counter this stands in for, so a Claude model
reached through Bedrock or OpenRouter still resolves correctly. Fall back
to the provider when no model was recorded, treating ANTHROPIC as Claude
but not BEDROCK, which also serves Llama, Titan and Mistral.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1afff952c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (agentContext.tokenCounter) { | ||
| return agentContext.tokenCounter(carrier); |
There was a problem hiding this comment.
In a heterogeneous multi-agent run with indexTokenCountMap, Run.create derives one counter from agents[0], and StandardGraph passes that same counter to every AgentContext. A later Claude agent therefore takes this branch and measures its carrier with the first agent's tokenizer, never reaching encodingForReceivingAgent; for CJK-heavy summaries this can materially under-reserve the persisted checkpoint and cause another overflow. The fallback added for the earlier missing-counter issue should also be used when the shared counter does not represent this receiving agent, or counters should be created per agent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 4713b01. Confirmed: Run.create (src/run.ts:732) derives one counter from agents[0] and StandardGraph (src/graphs/Graph.ts:1495) hands it to every AgentContext, so a Claude agent behind a GPT first agent measured its carrier in o200k_base — 100 tokens against the claude tokenizer's 163 on the Korean sample, a 1.6x under-reservation on the retry.
Took the fallback route rather than per-agent counters: AgentContext.fromConfig shares one indexTokenCountMap object across agents, so per-agent counters would write divergent units into the same map.
createTokenCounter now stamps each counter with its encoding (encodingOfTokenCounter), and computeSummaryTokenCount takes the bundled tokenizer whenever the stamped encoding disagrees with encodingForReceivingAgent. A host-built counter is unstamped and stays authoritative, since its units are the ones the host's own token map is denominated in — covered by two tests in src/summarization/tests/node.test.ts (the first fails on the old code with exactly the 100-vs-163 gap).
Run.create derives one token counter from agents[0] and StandardGraph hands that same counter to every AgentContext, so a Claude agent behind a GPT first agent measured its summary carrier in o200k_base and persisted a count 1.6x below what the retry reserves. Counters built by createTokenCounter now carry their encoding, and the summary carrier takes the bundled tokenizer whenever that encoding disagrees with the receiving agent's. A host-supplied counter is unstamped and stays authoritative, since its units are the ones the host's own token map is denominated in.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4713b01c5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| */ | ||
| function encodingForReceivingAgent(agentContext: AgentContext): EncodingName { | ||
| const model = | ||
| (agentContext.clientOptions as { model?: string } | undefined)?.model ?? ''; |
There was a problem hiding this comment.
When a Claude-backed OpenRouter or custom agent supplies its model through LangChain's supported clientOptions.modelName alias, this lookup treats the model as absent and falls back to o200k_base because the provider is not ANTHROPIC. CJK-heavy checkpoint carriers can consequently be undercounted and under-reserved during overflow recovery. Fresh evidence beyond the earlier tokenizer finding is that the receiving-model fix reads only model, while this repository also configures agents through modelName; resolve both aliases before falling back to the provider.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 836c91d. Confirmed the alias is real in this repo, not just LangChain-supported: buildSummarizationClientConfig writes both keys (src/summarization/node.ts:274), and two private helpers already resolved both — extractClientOptionsModel in src/llm/invoke.ts and extractConfiguredModel in src/tools/subagent/SubagentExecutor.ts — while encodingForReceivingAgent read only model.
Run.create had the identical defect one level up (src/run.ts:736 read only model), so the run-wide counter a host gets when it supplies indexTokenCountMap without a counter was o200k_base for a Claude agent configured through the alias. Fixed both, since the summary path only self-heals there because the encoding stamp catches the mismatch.
Rather than a third copy, the resolution now lives in one place: resolveClientOptionsModel in src/llm/request.ts, used by all four sites.
Tests: 'resolves the receiving model through the modelName alias' in src/summarization/tests/node.test.ts (OpenRouter provider, so the provider fallback cannot mask it: 100 tokens before, 163 after) and src/specs/run-token-counter.test.ts covering the derived counter's encoding for both keys and the no-model fallback. Left the alias-blind reads in AgentContext tool-multiplier and Graph cache-control alone: different behavior, not tokenizer selection.
LangChain accepts modelName as an alias for model and this repository configures agents through both, so reading only model reported an unconfigured model and fell through to the provider: a Claude-backed OpenRouter agent got o200k_base and undercounted its checkpoint carrier by 1.6x on CJK. Both the summary carrier's tokenizer and the run-wide counter Run.create derives now resolve either key. Two private copies of that resolution already existed in invoke and SubagentExecutor; all four sites now share resolveClientOptionsModel.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 836c91d4e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return agentContext.provider === Providers.ANTHROPIC | ||
| ? 'claude' | ||
| : 'o200k_base'; |
There was a problem hiding this comment.
When a host registers a custom provider with family: 'anthropic' and its client options omit the model or use an opaque deployment alias, this exact-enum check falls back to o200k_base even though src/provider-registration.ts exposes the provider family as the definitive signal. The persisted carrier can then be materially undercounted for CJK-heavy summaries and under-reserved during overflow recovery; consult getProviderFamily(agentContext.provider) before selecting the default encoding. This custom-provider family path is fresh evidence beyond the previously reported model-alias cases.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a455fb0. Real gap: built-in registrations set family (src/llm/providers.ts:70 for Anthropic, :83 for Bedrock), a host registerProvider call can set family: 'anthropic' on any provider name, and isThinkingEnabled (src/llm/request.ts:18) already reads the trait that way while this fallback matched the enum exactly.
The provider fallback is now provider === Providers.ANTHROPIC || getProviderFamily(provider) === 'anthropic'. Kept the enum check ahead of the lookup rather than relying on the family alone: the registry is only populated once @/llm/providers is imported, which the root barrel deliberately defers, so a family lookup can legitimately come back undefined for the built-in. Bedrock is still excluded, since its family is bedrock and it also serves Llama, Titan and Mistral.
Test 'honors a registered anthropic family with no configured model' in src/summarization/tests/node.test.ts registers a real host provider through registerProvider with no model on either option key, and fails on the pre-fix code with the same 100-vs-163 gap.
A host can register its own provider with family: 'anthropic', which serves Claude under whatever name and deployment alias the host chose. An exact match on the ANTHROPIC enum missed those, so a custom provider with no configured model measured its checkpoint carrier in o200k_base and under-reserved it by 1.6x on CJK. The provider fallback now reads the family, the way isThinkingEnabled already does, with the enum check still ahead of it because the registry is only populated once @/llm/providers is imported.
|
@codex review the current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a455fb0d1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const model = resolveClientOptionsModel(agentContext.clientOptions); | ||
| if (model != null) { | ||
| return encodingForModel(model); | ||
| } |
There was a problem hiding this comment.
When a host-registered family: 'anthropic' provider configures an opaque model/deployment alias such as model: 'production', this early return passes that alias to encodingForModel, selects o200k_base, and never consults the definitive provider family. CJK-heavy checkpoint carriers can therefore still be undercounted and under-reserved during overflow recovery. Fresh evidence after the claimed family fix is that its new test covers only an absent model, while the nonempty opaque-alias path bypasses the added family check; resolve the Anthropic family before falling back to model-name inference.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7cb14f1. encodingForModel matches the substring claude, so a name it rejects is the absence of a signal rather than evidence of a non-Claude model, and treating it as the answer is what skipped the family lookup. encodingForReceivingAgent now returns early only on a positive name match, so an opaque alias falls through to the provider and a host-registered family: 'anthropic' still resolves to claude; Bedrock and OpenRouter Claude names keep resolving on the name, as before. Covered by 'honors a registered anthropic family behind an opaque model alias', which configures model: 'production' on an anthropic-family provider along with the o200k_base counter Run.create stamps from that same alias, and asserts the Korean carrier is measured in claude: 100 tokens before the fix against 163 after.
A model name without `claude` in it is the absence of a signal, not evidence of a non-Claude model, so an opaque deployment alias on a host-registered `family: 'anthropic'` provider skipped the family lookup and measured the carrier with `o200k_base`, undercounting a Korean checkpoint by 1.63x.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
createSummarizeNodeis a LangGraph node fused to a live run: it needs anAgentContext, the graph'scontentData/contentIndexMap/ dispatchers, the breaker controller and epoch, andStreamLimitState. Anything that wants to compact a conversation outside a run cannot call it, and the primitives it uses are private tonode.ts, so the caller reimplements them and then drifts from them.That is what happened in LibreChat's manual
/compactwork: it carries its own copy of the summary wrapper overhead constant, both checkpoint prompts, themaxSummaryTokensparameter split, and the instruction builder. Four separate places where the two compaction paths can silently disagree while writing to the same summary boundary.This moves those four into
src/summarization/shared.ts, which the module barrel re-exports, so they are reachable from the package root.node.tsimports what it used to define and keeps re-exporting the prompts for its existing importers. No behavior change on the automatic path.One small fix while moving it:
buildSummarizationInstructionnow trims the prior summary, so a blank one asks for a fresh checkpoint instead of a consolidation of nothing.Newly exported:
SUMMARY_WRAPPER_OVERHEAD_TOKENSDEFAULT_SUMMARIZATION_PROMPT,DEFAULT_UPDATE_SUMMARIZATION_PROMPTseparateSummarizationParameters(was the privateseparateParameters)buildSummarizationInstructionChange Type
Testing
npx jest src/summarization src/specs/summarization src/specs/summarize-prune src/specs/multi-agent-summarization: 9 suites, 145 passed, 10 skipped.llm/google,llm/anthropic,llm/vertexai, 12 tests) fail identically on a pristineorigin/maincheckout and are unrelated to this change.tsc -p tsconfig.json --noEmit: clean.src/summarization/__tests__/shared.test.ts, 7 tests. The trim case was confirmed to fail without the fix.Checklist