feat(context): add context manager#2547
Conversation
lizradway
requested a deployment
to
manual-approval
GitHub Actions
Waiting
lizradway
requested a deployment
to
manual-approval
GitHub Actions
Waiting
lizradway
force-pushed
the
context-manager
branch
from
06171e3 to
a1337f0
Compare
lizradway
requested a deployment
to
manual-approval
GitHub Actions
Waiting
lizradway
requested a deployment
to
manual-approval
GitHub Actions
Waiting
lizradway
force-pushed
the
context-manager
branch
from
a1337f0 to
a97206c
Compare
lizradway
requested a deployment
to
manual-approval
GitHub Actions
Waiting
lizradway
requested a deployment
to
manual-approval
GitHub Actions
Waiting
lizradway
force-pushed
the
context-manager
branch
from
a97206c to
8f0feca
Compare
lizradway
requested a deployment
to
manual-approval
GitHub Actions
Waiting
lizradway
requested a deployment
to
manual-approval
GitHub Actions
Waiting
lizradway
force-pushed
the
context-manager
branch
from
8f0feca to
3ef3431
Compare
lizradway
requested a deployment
to
manual-approval
GitHub Actions
Waiting
lizradway
requested a deployment
to
manual-approval
GitHub Actions
Waiting
lizradway
changed the title
lizradway
force-pushed
the
context-manager
branch
from
3ef3431 to
a70ae1c
Compare
lizradway
requested a deployment
to
manual-approval
GitHub Actions
Waiting
lizradway
requested a deployment
to
manual-approval
GitHub Actions
Waiting
lizradway
force-pushed
the
context-manager
branch
from
a70ae1c to
50e7824
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Tool-pair partner protection: if adjacent message is protected and they form a pair | ||
| const msg = messages[index]! | ||
| const hasToolResult = msg.content.some((b) => b.type === 'toolResultBlock') | ||
| if (hasToolResult && index > 0 && index - 1 < protectFirst) return true |
There was a problem hiding this comment.
Issue: This does not verify that messages[index - 1] actually contains a matching toolUseBlock. It only checks that the current message has a toolResult and the previous message is within protectFirst. If message at index - 1 happens to be a regular text message at the boundary of protectFirst, this incorrectly marks the current message as protected.
Suggestion: Add a check that validates the previous message contains a toolUseBlock with a matching toolUseId:
if (hasToolResult && index > 0 && index - 1 < protectFirst) {
const prev = messages[index - 1]!
const resultIds = new Set(
msg.content.filter((b): b is ToolResultBlock => b.type === 'toolResultBlock').map((b) => b.toolUseId)
)
if (prev.content.some((b) => b.type === 'toolUseBlock' && resultIds.has((b as ToolUseBlock).toolUseId))) {
return true
}
}In practice, the LLM API ordering (toolUse always precedes toolResult) may prevent this from manifesting as a user-visible bug, but the validation keeps the function correct regardless of message arrangement.
| export type OffloaderConfig = { | ||
| /** Token threshold above which tool results are offloaded. Defaults to 2500. */ | ||
| threshold?: number | ||
| /** Number of tokens to keep as an inline preview. Defaults to 500. */ |
There was a problem hiding this comment.
Issue: previewTokens default is documented as 500 here (and repeated on line 46), but the actual fallback on line 159 is ?? 1500. The PR description's table also states 1500.
Suggestion: Update the TSDoc to say "Defaults to 1500" to match the implementation.
| if (hasToolResult && index > 0 && index - 1 < protectFirst) return true | ||
|
|
||
| const hasToolUse = msg.content.some((b) => b.type === 'toolUseBlock') | ||
| if (hasToolUse && index + 1 < messages.length && index + 1 < protectFirst) return true |
There was a problem hiding this comment.
Issue: This condition is unreachable. We only arrive here when index >= protectFirst (line 123 already returned true for index < protectFirst). For index + 1 < protectFirst to be true, we'd need index < protectFirst - 1, which contradicts index >= protectFirst.
Suggestion: Remove this dead branch or rewrite the tool-pair partner logic. If the intent is "protect a toolUse whose partner toolResult is in the protected range", note that toolResult always comes after toolUse in message ordering, so the toolUse is always at a lower index — meaning it would already be protected by line 123.
| continue | ||
| } | ||
|
|
||
| const hasToolUse = msg.content.some((b) => b.type === 'toolUseBlock') |
There was a problem hiding this comment.
Issue: The findValidTrimPoint function checks for toolUseBlock on user-role messages (lines 73-80), but toolUseBlocks only appear in assistant messages. Since line 63 already skips non-user messages, this branch is dead code.
The same pattern appears in adjustSplitForToolPairs in summarize.ts.
Suggestion: Remove the dead hasToolUse check or restructure the logic to correctly handle the trim boundary. The actual concern is: don't start the "kept" portion at a toolResult (user message) that is the result of a toolUse (assistant message) immediately before it — which is already handled by the toolResultBlock check on line 68.
| * ```typescript | ||
| * // Config shorthand (most users) | ||
| * const agent = new Agent({ contextManager: "auto" }) | ||
| * |
There was a problem hiding this comment.
Issue: The TSDoc @example (lines 82-88) shows passing a ContextManager class instance to Agent({ contextManager: cm }), but ContextManagerParam is typed as ContextStrategyValue | ContextManagerConfig — it doesn't accept a ContextManager instance. This example would fail type-checking.
Suggestion: Either update ContextManagerParam to also accept ContextManager instances (if that's the intended "power user" path), or fix the example to show the config-object approach:
const agent = new Agent({ contextManager: { storage: new S3Storage("bucket") } })
| continue | ||
| } | ||
|
|
||
| const hasToolUse = msg.content.some((b) => b.type === 'toolUseBlock') |
There was a problem hiding this comment.
Issue: The adjustSplitForToolPairs function has the same dead code pattern as findValidTrimPoint in truncate.ts — checking for toolUseBlock on messages that have already passed the role !== 'user' skip (line 128-129 skips non-user messages, so the message at idx is always user-role, which never contains toolUseBlock).
Suggestion: Same as the comment on truncate.ts — consider removing the dead branch or documenting why it exists as defensive coding.
| ) | ||
| } | ||
| this._conversationManager = new NullConversationManager() | ||
| } else if (contextManagerPlugin) { |
There was a problem hiding this comment.
Issue: When a non-stateful model has both contextManager and conversationManager set, the conversationManager is silently ignored (line 365-366 takes priority). This could confuse users who set both accidentally.
Suggestion: Consider logging a warning when both are provided, e.g.:
} else if (contextManagerPlugin) {
if (config?.conversationManager) {
logger.warn('contextManager takes priority over conversationManager — conversationManager will be ignored')
}
this._conversationManager = new NullConversationManager()
}
|
Assessment: Comment Well-architected feature with clear separation of concerns between the facade ( Review Categories
The overall design aligns well with SDK tenets — particularly composability and "provide both low-level and high-level APIs". |
lizradway
force-pushed
the
context-manager
branch
from
29d1f4d to
686bdfb
Compare
lizradway
requested a deployment
to
manual-approval
GitHub Actions
Waiting
| /** Positive: protect first N messages. Negative: protect last N messages. */ | ||
| protectFirst?: number |
There was a problem hiding this comment.
What if I want both first and last?
There was a problem hiding this comment.
realistically, protectLast is default in all compression behavior (ie. sliding window) and protectFirst is far more useful (to protect prompt/first user messages).
if we see demand/usecase for it i think it would be nice to add, but i think for now I'd want to keep the api surface minimal. feel free to push back if you'd want to ship immedietely.
| /** Ratio of messages to summarize (0.1–0.8). Defaults to 0.3. */ | ||
| summaryRatio?: number | ||
| /** Minimum recent messages to preserve during summarization. Defaults to 10. */ | ||
| preserveRecentMessages?: number |
There was a problem hiding this comment.
protectLast would have more synergy with protectFirst. That said, I'd be in favor of exposing an object instead, but let's at least align the two for consistency.
There was a problem hiding this comment.
i think that is fair, right now for sliding window i just ported over the exact impl, but can update this.
| * Compression configuration. | ||
| * - `true`: enable with defaults (truncate, proactive at 0.7). | ||
| * - `'truncate'` / `'summarize'`: enable specific strategy with defaults. | ||
| * - `CompressionStrategy.Truncate(...)` / `CompressionStrategy.Summarize(...)`: full config. |
There was a problem hiding this comment.
Do we actually expose these types?
| * @param model - The model to use for token counting | ||
| * @returns Estimated token count, or undefined if estimation fails | ||
| */ | ||
| export async function estimateInputTokens(messages: Message[], model: Model): Promise<number | undefined> { |
There was a problem hiding this comment.
In the current implementation we add the option to estimate systemPrompt and toolSpecs. Can we support this here to avoid regression? Then we can also forward Agent._estimateInputTokens() to use this function and avoid maintaining 2 copies.
There was a problem hiding this comment.
yes good idea, can add!
| */ | ||
| export type CompressionConfig = | ||
| | true | ||
| | import('./compression/context-compression.js').CompressionMethod |
There was a problem hiding this comment.
why are we not just importing top level? What does this buy us?
| /** Strategy name. Only "auto" is supported currently. */ | ||
| strategy?: ContextStrategyValue | ||
| /** Storage backend for cached tool results. Defaults to InMemoryStorage. */ | ||
| storage?: Storage |
There was a problem hiding this comment.
If it's just cached tool results should we narrow the parameter name?
There was a problem hiding this comment.
Or if it is also other things... Storage alone is too concise
| if (config.compression) { | ||
| const userProvided = userPlugins?.some((p) => p.name === 'strands:context-compression') | ||
| if (!userProvided) { | ||
| let compressionConfig: import('./compression/context-compression.js').CompressionOptions | undefined |
| const plugins: Plugin[] = [] | ||
|
|
||
| if (config.compression) { | ||
| const userProvided = userPlugins?.some((p) => p.name === 'strands:context-compression') |
There was a problem hiding this comment.
Can we do instanceof instead of this name matching pattern?
| * A message is protected if it is pinned, within the protectFirst range, | ||
| * or is a tool-pair partner of a protected message. | ||
| */ | ||
| export function isProtected(messages: Message[], index: number, protectFirst?: number): boolean { |
There was a problem hiding this comment.
Maybe a more descriptive function name
| const SUMMARIZATION_PROMPT = `You are a conversation summarizer. Provide a concise summary of the conversation history. | ||
|
|
||
| Format Requirements: | ||
| - You MUST create a structured and concise summary in bullet-point format. |
There was a problem hiding this comment.
confirming bullet-point format
| * @param options - Summarization options | ||
| * @returns `true` if messages were summarized, `false` if not enough to summarize | ||
| */ | ||
| export async function summarize(messages: Message[], model: Model, options?: SummarizeOptions): Promise<boolean> { |
There was a problem hiding this comment.
could expose sys prompt for the summarization
| * @param message - The message to check | ||
| * @returns `true` if the message has `metadata.custom.pinned === true` | ||
| */ | ||
| export function isPinned(message: Message): boolean |
There was a problem hiding this comment.
After offline discussion, it sounds like "pin" verbage is coming from tool result pairs. I.e. we don't say protect/ed directly because it can mean two messages.
I think this mismatch is confusing. I wouldn't mind just letting protected automatically include a pair.
lizradway
force-pushed
the
context-manager
branch
from
686bdfb to
8a6f0db
Compare
lizradway
requested a deployment
to
manual-approval
GitHub Actions
Waiting
4 participants
Description
Implements the v1
contextManagerfacade as designed in strands-agents/docs#831.Adds a
contextManagerparameter toAgentConfigthat pre-composes the SDK's context management primitives into a single configuration surface. An internalContextManagerplugin composes sub-plugins (ContextCompression,ContextOffloader) that handle the actual behavior.Architecture
Sub-plugins work independently when used standalone. User-provided plugins with matching names take precedence over managed sub-plugins. When
contextManageris set,ContextCompressiontakes priority —NullConversationManageris used (same pattern as other dedicated-param plugins likeretryStrategy,sessionManager).What ships
contextManagerparameter onAgentConfig— accepts"auto"or a config objectContextCompressionplugin — proactive/reactive compression with own reduction logic (truncate or summarize)ContextOffloader— stays invended-plugins/context-offloader/, composed internally by ContextManagercontext-manager/compression/protection.ts) —pinMessageToolfor agent-controlled pinning at runtime. Internal utilities (pinMessage,unpinMessage,isPinned,isProtected) not exported; programmatic pinning API deferred.protectFirst— number of messages at the start of the conversation to protect from evictionestimateInputTokens()utility — shared token estimation insrc/context-manager/token-estimation.ts<summary>XML tags — summarized messages are wrapped in<summary>tags so the model can distinguish framework-injected summaries from user contentconversationManagermarked as pending deprecation — still works, JSDoc-taggedPublic API Surface
New on
AgentConfig:contextManager?: ContextManagerParamNew exports:
pinMessageTool(agent-invokable tool)ContextManagerParam(type)All classes (
ContextManager,ContextCompression,ContextOffloader) are internal.ContextOffloaderremains accessible via thevended-plugins/context-offloadersub-path for backward compat.Configuration model
Two semantics depending on whether
strategy: 'auto'is present:Override (strategy: 'auto') — starts with everything enabled, you override specific settings:
Additive (no strategy) — starts with nothing, you enable what you want:
Compression config (discriminated union on
method)Defaults (when enabled via "auto")
Plugin registration
contextManagermust be passed via the dedicated parameter — same pattern asconversationManager,retryStrategy, andsessionManager. No guards for misuse inplugins[](consistent with other special-cased plugins).Deprecation Plan
The following are marked as pending deprecation in v1 and will be removed in v2:
AgentConfig.conversationManager→contextManager: { compression: ... }Agent._estimateInputTokens()→ sharedestimateInputTokens()utilityBeforeModelCallEvent.projectedInputTokens→ futurecontextManagerbudget APIConversationManager,SlidingWindowConversationManager,SummarizingConversationManager,NullConversationManager→ContextCompressionpluginvended-plugins/context-offloader/sub-path → import from@strands-agents/sdkdirectlyBreaking Changes
None. All changes are additive. Existing behavior is unchanged when
contextManageris not set.Related Issues
Documentation PR
strands-agents/docs#831
Type of Change
New feature
Testing
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.