feat(context): add message pinning to conversation managers#1105
feat(context): add message pinning to conversation managers#1105lizradway wants to merge 1 commit into
Conversation
|
Assessment: Comment Clean, well-designed feature that adds opt-in message pinning. The API surface is intuitive and the PR description is thorough. A few things need attention before merging. Review Categories
Solid foundation for agentic context management — the utility functions are clean and well-tested. |
|
Assessment: Comment Well-designed feature with a clean, composable API. The implementation is solid, tests cover the key scenarios (positive/negative protection, all-protected edge case, pinned-in-middle), and the PR description is thorough with good examples. Review Themes
Nice work on the bounds checking, warning logging for the all-protected case, and the immutable |
|
Assessment: Comment Previous review items have been well-addressed (TSDoc, toolUseId matching, bounds checking). Three items remain from the prior round. Remaining Items
|
|
Assessment: Comment Clean, well-structured feature. The previous review issues have been addressed (TSDoc, toolUseId matching, bounds checking, tests for both managers). Four items remain. Remaining Items
The API design is composable and well-documented — the overloaded |
|
Assessment: Approve All prior review feedback has been addressed — warning log added in the summarizing manager, out-of-bounds test added for Remaining Item
Clean, well-tested feature with a composable API surface. The |
| } | ||
|
|
||
| /** | ||
| * Returns a new Message marked as pinned (protected from eviction during context reduction). |
There was a problem hiding this comment.
In the PR, can you give reasoning towards why immutable vs modifying the message directly?
There was a problem hiding this comment.
Message (+ metadata) are readonly :/
I think metadata being readonly is extremely unnecessary since this isn't getting sent over the wire. thoughts on changing this?
| * When added to an agent's tools array, allows the agent to protect important | ||
| * messages from eviction during context reduction. | ||
| */ | ||
| export const pinMessageTool = tool({ |
There was a problem hiding this comment.
I think this is the first time we're vending a tool that is not on a class of some sort; did we have alternative suggestions as to where this should live?
There was a problem hiding this comment.
i thought about both vended tool and just community tool, but i think the path forward is for this to live/be added to the agent in the ContextManager class once it exists (via addTools on plugin)
| * Protected messages are compacted to the front of the array before the summary message. | ||
| * For agent-controlled pinning, add `pinMessageTool` to the agent's tools array. | ||
| */ | ||
| protectedMessages?: number |
There was a problem hiding this comment.
Is there a case where we'd want to support both? (can be a follow-up)
There was a problem hiding this comment.
naming; thoughts on proectedMessageCount?
There was a problem hiding this comment.
both as in both agent controlled and manually?
or both as in first/last N?
because yes to both lol. agentic management context should always be able to exist along side framework-managed.
hm i agree im not sold on the name protectedMessages since it doesn't specify much, but i think proectedMessageCount suggests an actual count without the ordering part. i can brainstorm some alternatives
There was a problem hiding this comment.
protectedMessageRange?
protectedMessageWindow?
|
Assessment: Approve All previously raised issues have been addressed (TSDoc, bounds checking, toolUseId matching, warning log consistency, test coverage). The implementation is clean, well-tested (96 tests pass), and the API surface is composable and intuitive. Remaining Item
Note: @zastrowm has open design questions (immutable vs mutable, tool vending pattern, naming) that should be resolved before merge. |
| * | ||
| * For agent-controlled pinning, add `pinMessageTool` to the agent's tools array. | ||
| */ | ||
| protectedMessageRange?: number |
There was a problem hiding this comment.
To me, range is misleading given that we're not giving a range available. What about making it an object and giving more freedom to the user?
protectedMessages?: { first?: number; last?: number }
Makes it more extensible (I can see use cases for last), and solves the naming problem
Description
Adds opt-in message pinning to conversation managers. Pinned messages are protected from eviction during context reduction — they survive both sliding-window trimming and summarization.
This lays the groundwork for the agentic context management strategy described in strands-agents/docs#831, where agents can protect important context from being lost.
API Surface
What's included
pinMessage/unpinMessageutility functions (stores pin state inmetadata.custom.pinned)isPinned: overloaded function —isPinned(message)for raw check,isPinned(messages, index)also protects tool-pair partners matched bytoolUseId(prevents orphaned toolUse/toolResult pairs)pinMessageTool: agent-invokable tool built with thetool()factory + Zod, supports both pin and unpin actions (defaults to pin)protectedMessageRangeconfig on both conversation managers: positive = protect first N, negative = protect last NSlidingWindowConversationManagerskips protected messages during trimming and tool-result truncationSummarizingConversationManagerexcludes protected messages from summarization, compacting them to the front before the summary message@strands-agents/sdk/context-manager/compressionUsage
File structure
Related Issues
strands-agents/docs#831
Documentation PR
TK in context facade docs PR
Type of Change
New feature
Testing
How have you tested the change?
npm run checkpinMessage/unpinMessage/isPinnedoverloads (18 tests)pinMessageToolpin/unpin/default/validation (6 tests)protectedMessageRangeconfig on both managers (8 tests)Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.