feat(context-ts): add context strategy presets + rewire defaults to use class - #4256
Conversation
a9150a7 to
3e9923d
Compare
|
@strandly-the-agent pls review |
3e9923d to
90e412e
Compare
Review SummaryAssessment: Request Changes Solid, well-scoped refactor that consolidates context management behind Review Categories
Nice consolidation overall — collapsing the |
90e412e to
d573ca9
Compare
strandly-the-agent
left a comment
There was a problem hiding this comment.
Changes requested — the presets are a good shape; the collateral changes to the reduction path are what need work.
Three blockers, all on documented entry points (evidence for each in the inline comments):
- 🔴
_removalRatio0.3 → 1.0 makes every message-level strategy remove 100% of eligible messages in one pass — for all existingOffload.*users, not just the new presets. - 🔴
proactiveSummarizationhas nopreserveRecent, so at 70% utilization it summarizes the pending user turn away before the model sees it. - 🔴 Presets now run on
NullConversationManager, and the overflow retry re-gates on estimated utilization — a realContextWindowOverflowErrorcan end with no reduction and no retry (measured:'auto'/'agentic'→ no retry; oldSummarizingConversationManager→ retry).
Plus 3 🟡 inline (pins evicted under 'agentic', unvalidated preset strings, two storage tests that assert the same thing) and two design questions I'd want answered before merge: the silently-dropped conversationManager, and root barrel vs ./experimental for the new exports. The api/needs-review label + PR description the bot already asked for still apply — the export-surface decision needs an API reviewer, not a solo approve.
Not re-raised: the instance-form narrowing (fixed in d573ca9), dead isContextManagerPreset, {} vs 'auto' divergence, presets.test.ts toBeDefined(), prettier — the bot review covers those.
Details — evidence ledger, questions, reading order, appendix
Verified
- ✅ Reviewed
d573ca940bc334b53dc0da226104d5ab7cfe4408(delta from90e412e7is the 3-lineinstanceof ContextManagerpassthrough) against merge-base545d9ed0. - ✅
eslint(changed files) exit 0 · ✅tsc --noEmit(src) exit 0 on both heads · 🔴prettier --checkexit 1 on 4 changed files (context-manager.ts,offload/base.ts,offload-strategy.test.ts,agent.context-manager.test.ts) · ✅vitest run --project unit-node32 files / 759 tests passed on90e412e7. - ✅ Probes (scratch vitest files, deleted): real
ContextWindowOverflowErrorat theAfterModelCallEventhook →from('agentic')/from('auto')retry=undefined, 40 messages untouched;SummarizingConversationManager(0.3)retry=true, 29 left.proactiveSummarizationon 10 messages at util 0.7 →[m0, "[Summarized: 9 messages…]"], the question inside the summary.truncate('*').when({utilization:0.5})on 8 messages → 1 left (pre-PR: 2 removed);'auto'summarize on 40 → 5 left. Pinned message at index 3 gone after'agentic'summarize.from('manual')→ default pipeline, no throw;strategies: ['typo']→TypeError: resolvePreset(...) is not iterable. Forcing_removalRatioback to 0.3 leaves the whole suite green.contextManager: 'auto'+ agentstoragedoes write through (context/<session>/scopes/agent/<id>/…) — only the test got weaker. - Passes: triage, context-build, correctness, test-quality, docs-accuracy, API bar-raiser, adversarial. The API and adversarial passes timed out on the deep tier and were re-run on the standard tier, so the API read is somewhat shallower than usual. Raw pass outputs and logs are uploaded as artifacts.
Questions
Blocking
- Is the
_removalRatio1.0 flip meant for all existing strategies, or was it scoped to the new presets?strands-py/.../offload/base.py:269keeps 0.3, design0015:1125still documents 0.3, and0011:102says defaults shouldn't be set on intuition alone. If deliberate, would it be cleaner as its own PR with the benchmark — or as aratioknob onOffloadConditionssooverflowProtectioncan ask for a full sweep while everything else stays incremental? { contextManager: 'auto', conversationManager: new SlidingWindowConversationManager({ windowSize: 20 }) }now silently discards the window (agent.ts:342; nologger.warn). Pre-PR honoured it and the site docs still promise it. Warn, or throw like the stateful-model guard atagent.ts:540-544(Python throws atagent.py:660)?src/experimental/index.ts:12-18already exports ten of these thirteen symbols via@strands-agents/sdk/experimental(#4231);index.ts:279-291now duplicates them in the root barrel under a code comment TypeDoc can't see. Un-exporting later is itself a break — is root export a deliberate promotion? If not, would moving the block tosrc/experimental/index.tsbe simpler? Either wayContextManagerStrategy,Offload,OffloadStrategyBuilder,StrategyPresetNamelack@experimentaltags andContextManagerPresetis referenced by an exported type but not exported.
Non-blocking
staleToolCleanupsets neitherthresholdnorutilization, so it drops older tool results on every model call even at 2% utilization; itspreserveRecent: 5counts tool-result-bearing messages, not "turns" aspresets.ts:24says. Intended?preserveRecent: 0.99≈ keep everything,preserveRecent: 1= keep one message,1.5silently floors to 1; Python types itint. Would a separatepreserveRecentRatiofield avoid the discontinuity?- The four
STRATEGY_PRESET_NAMEShave no Python counterpart and appear in no design doc — is serialized (JSON/YAML) config the reason they're strings rather than exported builders? That'd be worth stating in the PR body; it's the argument for the string layer.
Reading order
strands-ts/src/agent/agent.ts:326-345—resolveConversationManager: every non-undefined/non-falsevalue now yieldsNullConversationManager. The pivot of the PR.strands-ts/src/context-manager/context-manager.ts:83-115—ContextManager.from(), where'auto'/'agentic'become strategy pipelines; compare with whatagent.tsdeleted.strands-ts/src/context-manager/presets.ts— the named presets (the PR's stated purpose); short and self-contained.strands-ts/src/context-manager/strategies/offload/base.ts:159-168, 295-322— widest blast radius:preserveRecentas a ratio and the_removalRatiodefault; these affect every pre-existing strategy.src/index.ts:279-291,types.ts,types/agent.ts— surface changes. Tests last:agent.context-manager.test.tsshows which behaviours were dropped.
Appendix — non-blocking (9)
- Stale site docs (not in this diff, but now false for TS):
site/.../context-management.mdx:42-47(auto = SummarizingConversationManager 0.3/0.85 + ContextOffloader; tool namedretrieve_offloaded_content, actual stash tool isretrieve_context),:53-54(co-provided manager "replaces" the auto one; offloader dedupe),:138(agentic composition),:148(durable-storage advice → now agentstorageorcontextManager: { stash: { storage } });storage.mdx:58-61,247(offloader/prefix →context/<session>/scopes/agent/<id>);context-management.ts:21-27example is now a no-op. Prose is shared py/ts and Python is unchanged, so it needs a<Tabs>split rather than a rewrite. preserveRecentisn't a hard floor when a tool pair straddles the boundary (collectRemovableWithPairpulls the partner out of the preserved window) — pre-existing, but now reachable on every fire because the whole eligible set is selected.- Test gaps beyond the inline one: nothing asserts
ContextManager.from()expansions (1500/8000, previewTokens 750,preserveRecent: 4); nothing pins message-level removal count (the truncate marker assertion atoffload-strategy.test.ts:597-602was loosened rather than pinned); ratiopreserveRecentisn't covered throughOffload.*.when(); the deleted param+plugins duplicate-registration test has no replacement now that the instance form is back. Agent.contextManager(agent.ts:432) is public whileLocalAgent.contextManager(types/agent.ts:313) is@internal— pick one; with instances back,agent.contextManager!.stashis a plausible user route.ContextManager.from()is public but is Agent-construction plumbing returningContextManager | undefined; consider@internal.strategies/offload/index.ts:41@examplesays "over 2500 tokens" beside.when({ threshold: 1500 })— pre-existing text, newly published.context-manager.ts:50"On context overflow, runs the strategy pipeline" omits theBeforeModelCallEventproactive path, which is the primary one for'auto'.presets.ts:44comment restates the module doc;presets.ts:5-6has a blank line mid-paragraph that breaks TSDoc rendering.- Prettier: 4 files,
npm run formatfixes it (also flagged by the bot).
78d46fb to
553695e
Compare
…o ContextManager - Add strategy presets (proactiveSummarization, largeToolOffloading, overflowProtection, staleToolCleanup) as named building blocks that resolve to concrete Offload strategies. Preset names are the stable contract; expansions may change between releases. - Rewire auto/agentic modes to use ContextManager internally instead of SummarizingConversationManager + ContextOffloader. - ContextManagerStrategy now accepts preset strings, a ContextManagerConfig object, or false — users no longer construct ContextManager directly. ContextManager.from() resolves all variants into the internal instance. - Support decimal preserveRecent (0 < n < 1 = ratio, >= 1 = absolute count). - Default _removalRatio to 1.0 — preserveRecent is the sole brake on what gets processed. Emergency truncate keeps its 0.2 override. - Add isContextManagerPreset type guard for centralized preset checking.
553695e to
00ce3dc
Compare
When the provider throws ContextWindowOverflowError, the token estimate undercounts (excludes system prompt and tool specs), so utilization-gated strategies skip even though the window is full. Add an overflow flag to ContextState that bypasses the utilization gate on BaseOffloadStrategy when the overflow hook fires.
|
@strandly-the-agent pls review |
- Guard eager hook against pinned messages (fix #1) - Add {@link ContextManagerConfig} to ContextManagerStrategy TSDoc (fix strands-agents#2) - Remove overflow clear so emergency truncate always fires as safety net (fix strands-agents#3) - Warn when both contextManager and conversationManager are set (fix strands-agents#4) - Fix "turns" → "messages" in staleToolCleanup doc (fix strands-agents#5) - Improve two-pass pinned test to exercise repairAlternation merge (fix strands-agents#8)
Follow-up review — commit
|
Follow-up review — commit
|
|
@strandly-the-agent pls review |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Follow-up on b05df8e — two 🟡s, both fallout from the round-2 changes. Removing the overflow clear (per opieter-aws's thread) fixes the re-gating problem but swings the other way: the emergency truncate now fires on every overflow pass even after summarize has already resolved it, eating two of the four preserved turns (and ignoring pins). And the pinned-merge test still doesn't reach the merge — it passes with the fix reverted. Both have one-line fixes inline. Gates green.
Status:
- ✅ Eager
MessageAddedEventhook now skips pinned messages;logger.warnon co-providedconversationManager;{@link ContextManagerConfig};staleToolCleanupdoc wording. - 🟡
context-manager.ts:208-215— emergency truncate runs unconditionally underoverfloweven when earlier strategies acted. Verified with the'auto'pipeline:[u0+Summarized+u8, a9, u10, a11]after summarize →[u0+Summarized+u8+u10, a11]after emergency. - 🟡
offload-strategy.test.ts:678— with bothq10anda11appended the fixture is back to even length, sopinned-a3sits between twousermessages and never merges; dropa11and the test fails without the fix (verified both ways). - ⏳ Still open: storage-tests thread (
agent.storage.test.ts:15),api/needs-review(bot'sindex.tsthread).
Details
- ✅ Reviewed
b05df8e4d987bb25745c5421cb4b32b711540f9f(delta from82402b3: 6 files, +23/−8). Self-run follow-up; each change verified, the overflow change probed with a working summarizer mock. - ✅
tsc0 ·eslint0 ·prettier0 (agent.ts + context-manager/) ·vitestcontext-manager + agent cm/storage suites 9 files / 142 passed. - ✅ Probe:
new ContextManager()(='auto'), 12 messages,overflow: true, estimate 0.5 (and 1.2→0.5), summarizer returnsSUMMARY. Result[u0+[Summarized: 7]+u8+u10, a11]— versus proactive at 0.9[u0+[Summarized: 7]+u8, a9, u10, a11]; the difference is the emergency bite (dropsa9, mergesu10into the head). With the inline patch: identical to proactive; a noop user strategy still lets emergency fire (a1, u2dropped); existingcontext-manager.test.ts16/16 green. - ✅ Pin test: on this head the
survives repairAlternation mergetest passes with therepairAlternationmetadata line removed. Witha11deleted from the fixture it fails without the fix (expected undefined to be true) and passes with it.
… and fix pinned merge test - Clear strategyContext.overflow before EmergencyTruncateStrategy when earlier strategies already acted, preventing double-reduction after summarize already fixed the overflow - Remove a11 from pinned merge test fixture so the odd-length array forces repairAlternation to actually merge pinned-a3 with a7
Follow-up — no new commit since
|
|
@strandly-the-agent pls review |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Follow-up on ab518bc — both fixes verified; nothing new from me.
- ✅ Emergency truncate re-gates once a prior strategy acted:
'auto'on overflow now yields exactly the proactive result ([u0+Summarized+u8, a9, u10, a11]); with a no-op user strategy the emergency still fires. - ✅ The pinned-merge test now discriminates: with the
repairAlternationmetadata line reverted it fails (expected undefined to be true). - ✅
tsc/eslint/prettierclean; context-manager + agent suites 9 files / 142 tests.
Only two things left on the board, neither a code defect: the storage-tests coverage nit (agent.storage.test.ts:15 thread — a two-line storage.list('context/') assertion, non-blocking), and the api/needs-review label for the root-barrel exports, which is the one reason I'm not stamping an approval solo — happy for an API reviewer to take it from here.
…ernalize presets, fix docstring - Remove _removal_ratio from BaseOffloadStrategy; regular strategies now remove all eligible messages (preserve_recent controls what to keep), matching TS PR strands-agents#4256 - EmergencyTruncateStrategy gets its own _apply_per_message with a module-level _EMERGENCY_REMOVAL_RATIO = 0.2 - Prefix resolve_preset/resolve_strategies with underscore (internal-only) - Rephrase resolve_conversation_manager docstring to avoid leaking NullConversationManager implementation detail
Summary
Adds strategy presets to ContextManager and rewires the
'auto'and'agentic'modes to useContextManagerinternally instead ofSummarizingConversationManager. This makesContextManagerthe single owner of context reduction — overflow recovery, proactive compression, and emergency truncation all flow through the strategy pipeline.What changed
'proactiveSummarization','largeToolOffloading','overflowProtection','staleToolCleanup') that resolve to concreteOffload.*strategies. Usable in thestrategiesarray alongside raw strategies.ContextManager.from()factory: resolvesContextManagerStrategyvalues ('auto'|'agentic'|ContextManagerConfig|false) into internalContextManagerinstances. Validates unknown preset strings.contextManageronAgentConfigaccepts a preset string or config object. The agent constructs and registers the manager — no need to instantiateContextManagerdirectly.ContextManagerclass removed from barrel: it's an internal implementation detail. Only the config types andOffloadbuilder are public._removalRatioremoved:preserveRecentis now the sole control for message-level removal. Decimal (0 < n < 1) = ratio, integer (>= 1) = absolute count.ContextWindowOverflowError(the estimate undercounts without system prompt/tool specs). Emergency truncate also honors the overflow flag but clears it once strategies bring utilization below 1.0.BaseOffloadStrategynow filters out pinned messages (and tool-pair partners) in both message-level and per-block paths. Emergency truncate ignores pins as a last resort.API changes
New exports (all
@experimental):Changed:
AgentConfig.contextManager:ContextManagerStrategy(was not previously typed for presets)Removed from barrel:
ContextManagerclass (internal — use preset strings orContextManagerConfiginstead)Usage
Related Issues
#4053
Type of Change
New feature
Testing
ContextManager.from()validation, overflow bypass, emergency truncate, pinned message protection, strategy pipeline orderingContextWindowOverflowError