Repository navigation
fix(framework)!: decline a maxStreamTokens restart that won't fit - #842
nissa-seru wants to merge 7 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review
Recommendation: needs a maintainer's decision. The change belongs, but part of it needs a maintainer's call.
Reviewed commit da323b5 with claude-opus-5-5, by reading the code. It did not run the code or look at the checks.
Does this change belong in the project?
It fits, with concerns. Keeping an open turn's own calls and results out of compression during a mid-turn restart is the framework's own job, and the change builds it on context-manager's existing hold and preview APIs. A maintainer should still sign off on the breaking change to maxStreamTokens, and on the bundled first commit from another author, which only touches docs and sits outside the PR's topic.
Findings
7 findings of medium or high severity, see the inline comments:
- Medium severity. previewCompile calls previewContext without the capability check the existing preview path uses.
- Medium severity. firstTurnRow rescans the whole open turn at every tool boundary and compile, so a long turn costs quadratic work.
- Medium severity. A full preview select runs on the hot path at every boundary over budget, including ones that end the turn.
- Medium severity. Tool-boundary re-hold is not gated on stop(), unlike beforeCompile.
- Medium severity, test coverage. Retry hold-point propagation through provider waits, acceleration holds and merges is untested.
- Medium severity, test coverage. Silent-retry test passes even if the failed attempt's hold was never released or re-placed.
- Medium severity. The turn's hold release may clear a hold someone else placed on the same row.
2 more findings, less severe
- Low severity, maintainability,
src/agent.ts:2018-2032. previewCompile re-implements previewContextFor instead of sharing it. framework.ts already haspreviewContextFor, which resolves the thinking binding and callspreviewContextbehind a capability guard.Agent.previewCompileadds a second copy of the same steps, with a different budget source (compileBudget()here,{budgetTokens, agent.maxTokens}there) and without the guard. Two divergent ways to preview the same compile will drift apart. A shared helper that takes the budget would keep the binding and capability handling in one place. - Low severity, test coverage,
src/framework.ts:10040-10063. No test covers hold release on the start-throw and displaced-routing paths. The change adds two more release points. One is in the start's catch (20060-20063), for a silent start whosebeginAgentTurnthrows. The other is inbeginAgentTurn(20662-20663), when a new routing displaces one that is not the open logical turn. Neither is exercised bytest/turn-rows-held.test.ts. A regression there would leave a compression hold on the agent's store indefinitely, so old rows would never compress. Each release point needs a test.
For a deeper review by several readers and a judge, which takes longer, a maintainer can comment /review deep.
| previewCompile(): { fits: boolean } | null { | ||
| const thinkingBinding = this.thinkingBindingOption(); | ||
| return this.contextManager.previewContext( | ||
| this.compileBudget() ?? CONTEXT_MANAGER_DEFAULT_BUDGET, | ||
| undefined, | ||
| thinkingBinding ? { thinkingBinding } : undefined, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Medium severity. previewCompile calls previewContext without the capability check the existing preview path uses.
previewContextFor in framework.ts (9251-9258) casts the context manager structurally and returns null when previewContext isn't a function. It does this so lightweight host or test context-manager doubles keep working, which agent.ts also notes for getHotContextSettings. The new previewCompile calls this.contextManager.previewContext(...) directly. On a manager without that method it throws a TypeError. softRestartFits treats that as an unexpected throw, logs it, and returns false. So such an agent never takes its maxStreamTokens restart again, and logs an error at every boundary over budget, even though the PR promises that a manager with no preview restarts as before.
| let first: MessageId | undefined; | ||
| const bodiesWithCalls = new Set<string>(); | ||
| for (let end = cm.getMessageCount(); end > 0; end -= TURN_HOLD_SCAN) { | ||
| const { messages } = cm.getMessageWindow(Math.max(0, end - TURN_HOLD_SCAN), Math.min(TURN_HOLD_SCAN, end), { resolveBlobs: false }); | ||
| for (let i = messages.length - 1; i >= 0; i--) { | ||
| const message = messages[i]; | ||
| if (message.sequence <= after) return first; | ||
| if (message.participant !== agentName) continue; | ||
| const calls = Array.isArray(message.content) && message.content.some((block) => block.type === 'tool_use'); | ||
| if (calls && message.bodyGroupId !== undefined) bodiesWithCalls.add(message.bodyGroupId); | ||
| if ((message.shardIndex ?? 0) > 0) continue; | ||
| if (calls || (message.bodyGroupId !== undefined && bodiesWithCalls.has(message.bodyGroupId))) first = message.id; | ||
| } | ||
| } | ||
| return first; | ||
| } |
There was a problem hiding this comment.
Medium severity. firstTurnRow rescans the whole open turn at every tool boundary and compile, so a long turn costs quadratic work.
holdTurnRows runs at every tool boundary (line 13775), at every start (20673) and in every beforeCompile. Each call goes through firstTurnRow, which walks the store backwards 16 rows at a time with getMessageWindow until it reaches hold.after. That means every call reads every row the turn has stored so far. A turn of N rounds therefore costs O(N²) store reads on the synchronous tool-boundary path. The long turns this change is aimed at are exactly where this grows. The id already found could be cached and checked for presence instead, rescanning only when the held row has been removed.
| const overBudget = currentState.stream | ||
| && !(agent.thinkingBindingOption() !== undefined && agent.physicalWindowTokens !== undefined) | ||
| && agent.lastStreamInputTokens > 0 | ||
| && agent.lastStreamInputTokens > agent.maxStreamTokens; | ||
| && agent.lastStreamInputTokens > agent.maxStreamTokens | ||
| && this.softRestartFits(agent); |
There was a problem hiding this comment.
Medium severity. A full preview select runs on the hot path at every boundary over budget, including ones that end the turn.
softRestartFits calls agent.previewCompile(), which is ContextManager.previewContext. That is a synchronous dry select over the agent's whole store. It sits in the overBudget conjunction, so it runs at every tool boundary once lastStreamInputTokens > maxStreamTokens, whether or not shouldEndTurn is already true and whether or not overPhysical will force a restart anyway. When the preview keeps failing (the case the PR documents), it runs at every remaining boundary of the turn and blocks the event loop each time. With a non-refusal throw it also writes a console.error line at every boundary. The preview should be skipped when the boundary ends the turn, and repeated declines should be rate-limited or logged once.
| const boundaryTurn = this.turnRouting.get(agent.name); | ||
| if (boundaryTurn) this.holdTurnRows(agent, boundaryTurn); |
There was a problem hiding this comment.
Medium severity. Tool-boundary re-hold is not gated on stop(), unlike beforeCompile.
beforeCompile checks !this.providerAdmissionClosed before calling holdTurnRows, because stop() releases every turn's hold in an early step (line 4907) and a later re-hold would outlive the framework. The tool-boundary call at 13775 has no such guard. stop() releases holds before it cancels streams, and a stream whose cancelStream() throws keeps running (uncancelled). So a tool round that completes after the release step and is not dropped by the state checks above re-places a compression hold on the store after stop() released it. That hold is never released, which is the leak the stop test claims cannot happen. The tool-boundary call needs the same providerAdmissionClosed guard.
| if (isTurnRetry(request)) { | ||
| (same as MarkedRequest)[TURN_RETRY] = true; | ||
| withTurnHoldAfter(same as MarkedRequest, turnHoldAfter(request)); | ||
| } |
There was a problem hiding this comment.
Medium severity, test coverage. Retry hold-point propagation through provider waits, acceleration holds and merges is untested.
The change threads TURN_HOLD_AFTER through several paths: holdRequest's dedupe (5771-5774), holdProviderAcceleration, nonOwningProviderWait, the conversation-agent provider wait (20976) and the merged-start Math.min (16931-16934). The only retry test is the silent turn's immediate errorPolicy retry. No test drives a held retry released by a provider-wait or acceleration cooldown, a retry that is deduped into an existing held request, or a merge of several retries with different start points. Any of these could lose or mix up the start point without a test failing. Each path needs its own test.
| try { | ||
| framework.start(); | ||
| const { agent, contextManager } = await framework.createEphemeralAgent({ ...agentConfig(), name: 'worker' }); | ||
| let compressedDuringHooks = 0; | ||
| module.onStart = async (start) => { | ||
| const first = replyOf(framework, 1, 'worker'); | ||
| if (start === 2 && first) { | ||
| contextManager.removeMessage(first); | ||
| for (let ticks = 0; ticks < 300 && contextManager.getPendingWork(); ticks++) await contextManager.tick(); | ||
| compressedDuringHooks = membrane.compressionsOfTheTurn(); | ||
| } | ||
| }; | ||
| contextManager.addMessage('user', [{ type: 'text', text: 'Read the pages, please.' }]); | ||
| const result = await framework.runEphemeralToCompletion(agent, contextManager); | ||
| assert.equal(result.speech, FINAL); | ||
| assert.ok(compressedDuringHooks > 0, "the run's results were compressed while its restart's hooks ran without its hold"); | ||
| assertRestartShowsTurnWhole(membrane, traces, 'physical_window', ROUNDS, 'either', 2); | ||
| } finally { | ||
| await framework.stop(); | ||
| rmSync(dir, { recursive: true, force: true }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Medium severity, test coverage. Silent-retry test passes even if the failed attempt's hold was never released or re-placed.
The test only checks that, at the retry's second tool call, the compression holds include the first attempt's first reply. If the silent failure path never released the hold, or the retry ignored TURN_HOLD_AFTER and the old hold just stayed in place, the assertion still holds. So the test cannot tell "held again from the carried start point" apart from "never released". It also pushes into the private pendingRequests array, which ties it to internal structure. The test should assert that the hold was released between attempts, or that the retry's routing carries the earlier after.
|
|
||
| private softRestartFits(agent: Agent): boolean { | ||
| try { | ||
| return agent.previewCompile()?.fits ?? true; | ||
| } catch (error) { | ||
| // context-manager 0.22 and the released 0.23.0 throw this plain Error from previewContext on the | ||
| // non-adaptive (hierarchical) path once its dry select has succeeded, since that path has no fold plan |
There was a problem hiding this comment.
Medium severity. The turn's hold release may clear a hold someone else placed on the same row.
holdTurnRows calls cm.holdCompression([id]) and releaseTurnRows calls cm.releaseCompression([held.id]) with no record of whether the row was already held. getCompressionHolds() is used as a set, so holds appear not to be reference-counted. A module or host that holds the agent's reply row itself, which ModuleContext and the test at line 557 show is possible, would lose its hold when the turn settles. If holds are counted per caller this doesn't apply; otherwise the framework should skip releasing ids that were held before it took them.
|
We're cutting agent-framework 0.28.0 today without this one. Some concerns from how our deployments run, before it lands: 1. Every one of our residents is in the path this changes. All of them set 2. A really long turn now ends instead of continuing. The hold covers the whole turn from its first reply with a tool call, with no bound. Once the turn's own rows outgrow the budget, the 3. Below the tail, the hold changes little for us. With large kv-unified tails, a turn's rows only fold once the turn outgrows the tail. The partial-view problem the PR measures, with a 22,000 budget, is real, but for us it starts where the all-or-nothing hold starts ending turns. A suggestion: bound the hold. Keep the newest part of the turn whole, up to a share of the budget (or the strategy's raw tail), and let the turn's older rounds fold. Better still if the agent is told what folded, for example with a turn-local summary of its earlier rounds, so it knows what it already did. That keeps the fix for the common case without making the turn's length a failure condition. Smaller points:
🤖 Generated with Claude Code |
…ns bounds a request's size `maxStreamTokens`' public doc said "Max input tokens", and `physicalWindowTokens`' said that, unset, "only the maxStreamTokens restart applies", with nothing more. A host could read that as a bound on a request's size. Where membrane knows the provider's cache convention and a prompt is mostly cached, it doesn't bound a request's size. - The check compares `lastStreamInputTokens` (framework.ts, the `overBudget` check). The `usage` case sets that from membrane's cumulative `inputTokens` (the case's own comment: "The stream's accumulated fresh input, which is what the maxStreamTokens budget checks"). - membrane reports cache reads and writes beside `inputTokens`, as `cacheReadTokens` and `cacheCreationTokens`. Where membrane knows the provider's cache convention, as for Anthropic's `cache-excluded`, `inputTokens` leaves them out (membrane `src/utils/usage.ts`, `normalizeUsageToCacheExcluded`). Where the convention is 'unknown', as for `openai-compatible`, it's the provider's prompt count as reported. - The doc beside the counter in agent.ts already says that `lastStreamInputTokens` "alone omits cached tokens, which are most of a warm stream's window". - The check has read `inputTokens` since `a331a09`, and `dc22ef627a` kept it so on purpose. Since anima-research#98 (issue anima-research#92) the window projection is what bounds a request's size. So this changes no code. The docs now say what the count is and what follows from it: - `src/types/agent.ts`, `maxStreamTokens`: the doc opens with what the setting is, the `inputTokens` total above which the stream restarts. The total runs over the stream's calls, without the cache reads and writes reported beside it. Where membrane knows the provider's cache convention (one other than 'unknown'), `inputTokens` leaves those out, so there, where a prompt is mostly cached, the setting doesn't bound the request's size, and `physicalWindowTokens`, when set, does. - `src/types/agent.ts`, `physicalWindowTokens`: unset, where membrane knows the provider's cache convention, the count the `maxStreamTokens` restart compares leaves out cache reads and writes, so a turn's requests can grow past the window while most of their prompt is cached. - README: the streaming lifecycle's "Budget restart" step said "input tokens". It now says "the stream's running total of `inputTokens`", and adds when `inputTokens` leaves out cache reads and writes. - 0.28.0's changelog says a subconscious's "`maxStreamTokens` restart is its only window guard" (from anima-research#802's fragment). Where membrane knows the provider's cache convention, with no projection and a mostly cached prompt, nothing guards that window mid-turn. A new fragment, `changelog.d/subconscious-window-guard.fixed.md`, says so. The docs give no measured figure for the count. The behaviour they state rests on the runs below. Runs, on Linux with Node 24.22.1 and npm 11.19.1: - agent-framework, this tree after `npm install` (TypeScript 7.0.2, membrane 0.7.2, context-manager 0.22.1) and `npm run build`: `test/physical-window-restart.test.ts`, 23 pass, 0 fail. Its "leaves behavior unchanged when physicalWindowTokens is unset" has a call report 5,000 fresh and 195,000 cache-read tokens under the default `maxStreamTokens`, and the turn goes on with no restart. - membrane v0.7.2, vitest 4.0.18: `tests/unit/usage-cache-convention.test.ts`, 37 pass, and a probe of a yielding stream's usage events over three calls reporting 100, 120 and 130 `inputTokens`, which gave 100, 220 and 350 (the probe's source is in the PR description). - `src/types/agent.ts` scans to the same 712 tokens as at `b1967f954c` (TypeScript 5.9.3's scanner, trivia skipped), so only comments changed. - `scripts/release-changelog.mjs`, run in a scratch copy with the version raised, folds 31 fragments, the edited one among them, with exit 0. Rebased onto 0.28.0 by Paloma-2644: 0.28.0 folded the pending fragment this commit edited into CHANGELOG.md, so its correction is now a new fragment. The release-script run above was of the edited pending fragment, before 0.28.0.
On 0.28.0, once a stream's inputTokens passed maxStreamTokens, the next tool boundary cancelled the stream and restarted it with a recompiled context, whatever that compile would do. A resident's compressions wait while its own stream holds the agent's provider admission, so mid-turn they can run only in the gap a restart leaves. With nothing of the turn summarised to fold, a turn past the budget restarted into a compile that couldn't fit, which refused, and the turn ended with an over_budget failure row. The boundary now asks context-manager's previewContext whether that compile would fit (Agent.previewCompile, at the budget and with the thinking binding the compile is given), and declines the restart when it wouldn't: the stream goes on, and the next boundary over the budget asks again. A preview that answers null takes the restart, so a strategy with no preview restarts as before, and so does the plain Error context-manager 0.23.0 throws on the hierarchical path once its dry select has succeeded (0.23.1 returns null there). An UncoveredDropError from the dry select's coverage check declines the restart, and any other throw declines it with a log line. With physicalWindowTokens unset, nothing else then bounds the stream: it goes on until the provider refuses a request at its own window, and the turn ends with its failure row. The preview can't count the rows the restart's start then stores, so a restart whose preview leaves less room than those rows take can still be refused. test/restart-preview.test.ts has 14 tests, under hierarchical, kv-stable and adaptive kv-unified strategies. Two kv-unified tests drive a turn of 30 or 40 rounds whose stream reports each call's request size, summed over the stream as membrane's usage is: one where nothing awaits, so nothing of the turn is summarised and a restart's compile can't fit, and one where a module awaits at each tool result, so restarts fold the turn's older rounds and a turn longer than the provider's window answers. The docs of maxStreamTokens and README's budget restart step say when the restart is declined, and a breaking fragment says what hosts meet.
d197b9d to
00572ba
Compare
nissa-seru
changed the title
|
@antra-tess #842 is rebuilt without the turn hold, at A long turn (your point 2). With no hold, a restart is taken whenever its preview says the compile fits, as on 0.28.0. In the new tests, a long turn's older rounds are summarised during the turn when a module awaits at its tool results, and not when nothing awaits. In 0.28.0,
In a third test, a stream whose restart is declined sends a request that outgrows the provider's window, the provider refuses it as too long, and the turn ends with a A kv-unified test (your point 1). Six of the 14 tests in The rest. Your point 3, the bounded hold you suggested, and the timeout and owner-less holds in your smaller points were each about the hold, which is gone. Telling the agent which of its rounds a summary replaces isn't in this change. The 14 tests pass with context-manager 0.23.0 and with 0.23.1, on Linux with Node 24.22.1 and membrane 0.8.0. With 0.28.0's — Daniel-2612 |
Anarchid
left a comment
Anarchid
left a comment
There was a problem hiding this comment.
Nightly dual review · round R1 at 00572ba · reviewers: Codex gpt-5.6-sol (xhigh) and Claude claude-opus-5-5, merged and every claim re-verified by the Claude leg · posted by the maintainer's campaign runner. Verdict: CHANGES_REQUESTED (the formal review event is left to the maintainer). Finding ids R1-n are stable; cite them in fix commits.
Reviewed at 00572ba against main e85c57d
Verdict: changes requested. No blockers, three majors, four minors.
The change at this head is small and does what it says in the case its tests drive. When a turn's own rows outgrow the budget, the maxStreamTokens restart is declined and the turn goes on, where 0.28.0 restarted into a compile that refused. What it doesn't claim to touch is where the problems are:
- the error match misreads a hierarchical strategy built from a second context-manager copy, a case main's own dependency bump calls out;
- once a restart is declined, every later boundary repeats a full synchronous select whose answer can't change;
- the new way such a turn ends skips the
over_budgetalert and drain that 0.28.0's ending triggered, and nothing records that a restart was declined.
This is our first review of this PR, so there are no earlier findings to carry over. Findings are numbered R1-n so fix commits can cite them.
Major
R1-1: A hierarchical strategy from a second context-manager copy has its restart declined, with an error logged at every boundary
src/framework.ts:10089-10093
- Evidence:
softRestartFitstreats the hierarchical "no fold plan" refusal as a fit only whenerror.constructor === Errorand the message matches.- In context-manager 0.23.1 that refusal is
NoFoldPlanToPreviewError, a subclass with its ownname(src/types/strategy.ts:554-559there). ContextManager.previewContextturns it intonullonly byinstanceofagainst its own copy (src/context-manager.ts:1986there).- Main's
26679f23("depend on context-manager ^0.23.1") says this outright: a strategy built from another copy, whether a host's 0.23.0 or a second 0.23.1, still throws here. - A 0.23.1-copy
NoFoldPlanToPreviewErrorfails both the constructor test andinstanceof UncoveredDropError, so it is logged with its stack and the restart is declined. The hierarchical select had succeeded, so the restart's compile would have fit.
- In context-manager 0.23.1 that refusal is
- Repro: I adapted this PR's "hierarchical preview answers with 0.23.1's null" test and swapped in an
AutobiographicalStrategyfrom a second installed copy of context-manager 0.23.1.- On main, the turn restarts once and answers on the second stream.
- On main plus this PR, it runs on one stream with no restart, and logs
[context-budget] prime: the restart's preview failed, so the stream goes on: NoFoldPlanToPreviewError: …three times in six rounds. - With
physicalWindowTokensunset, that turn then goes on to the provider's window.
- Convention: the framework already knows this problem.
classifyInferenceErrormatchesOverBudgetErrorandUncoveredDropErrorbynameas well asinstanceofbecause "name… survives that boundary" (src/framework.ts:28216,:28238,:28245).previewStoppedReasonmatches the refusal by message (:1977). - Fix:
- Accept the refusal by
error.name === 'NoFoldPlanToPreviewError', or by message alone, without the constructor test. - Match
UncoveredDropErrorbynametoo. - Add a test with a
NoFoldPlanToPreviewError-named subclass, or a second package copy.
- Accept the refusal by
R1-2: Once declined, the restart is re-previewed at every boundary for an answer that can't change, and each preview is a full synchronous select
src/framework.ts:14024-14028, src/agent.ts:2023-2030
- Evidence:
softRestartFitsruns at every tool boundary wherelastStreamInputTokens > maxStreamTokens.previewContextis a full dry select. In context-manager 0.23.1 it is synchronous by design (autobiographical.ts:2609-2614,:2657).- A declined restart resumes the same stream, which keeps holding the agent's primary provider gate (taken at
:20035). The context manager's compression calls park behind that gate (withAuxiliaryAdmission,:5427-5447). - The remedy the preview names, compression, can't land while the stream continues, so in the case measured below the answer never changes. The boundary asks again all the same.
- It also asks on a boundary that ends the turn, and while the framework is stopping: the
overBudgetexpression checks neithershouldEndTurn(computed at:14017) norproviderAdmissionClosed(:14162).
- Measured, PR's own 30-round kv-unified test (budget 20,000,
maxStreamTokens8,000), sampling a preview and the agent's provider gate at each tool call:- From call 17 to call 30, every preview answers
fits: false, clearedBy: ['compression'], producedCount: 1. - One compression call is parked behind the agent's own gate from call 8 to the end of the turn (
deferredAuxiliary1,primaryDepth1). - The gate ran 24 previews in that turn, 14 of them in a row on an answer that couldn't change.
- From call 17 to call 30, every preview answers
- Measured, cost: I used the kv-unified policy from this PR's tests (bucket sizes 100,
labelCeiling10,000) at a 100,000-token budget, and timed onepreviewContextafter compressing a store of 1k-token rows.- 300 rows: about 0.35–0.43 s.
- 1,200 rows (≈1.2M tokens): about 15 s, and context-manager logged that the solve exceeded its label ceiling.
- context-manager's defaults are finer (bucket size 0, label ceiling 1,000,000).
- On main the same select runs once per restart. Here it runs at every boundary for the rest of the stream, with the event loop blocked for every agent on the framework while it does.
- Fix:
- Skip the preview when the boundary ends the turn or admission is closed.
- Remember a declined verdict for the stream, and ask again only when something that could change it has happened: a compression landed, the live budget changed, or a hold was released, as in the test at
test/restart-preview.test.ts:251. Failing that, cap re-asks per stream. - Optional, and the fuller cure: when
clearedByis['compression']and a compression is parked behind the agent's own gate, take the restart and park its continuation until that compression settles. The parked-continuation path at:20037already does this for calls already in flight. That would let the turn fold its older rounds instead of running to the window.
R1-3: Declined restarts leave no record, and the turn's new ending skips the over_budget alert and drain
src/framework.ts:10082-10095, :14162-14208, :28376-28441
-
Evidence, the decline itself:
- A
fits: falsedecline logs nothing and emits no trace.inference:stream_restarted(:14199-14208) fires only for a restart actually taken. - An
UncoveredDropErrordecline is deliberately not logged (:10090). - README (
:531),src/types/agent.ts:93and the breaking fragment (:4) all say a preview that fails is logged. - An operator can't tell "declined at every boundary since call 17" from "never over budget".
- A
-
Evidence, the turn's end: on 0.28.0, a restart that can't fit ends as
over_budget.classifyInferenceErrorgivesover_budget(:28238).- That raises the
context-refusalops alert at once (:28433-28441) and kicks the OverBudget drain breaker (:28376-28432). - With this PR and no
physicalWindowTokens, the same turn ends on the provider'scontext_lengthat 400, which does neither.
-
Repro, PR's "host that sets no window" scenario (kv-unified, budget 8,000,
maxStreamTokens10, provider window 9,000):main main + this PR Tool calls before the turn ends 4 9 Failure row over_budgetcontext_lengthops:alertone ( context-refusal)none Drain breaker kicked yes no -
Fix:
- Emit a trace on each decline, or once per stream, with its reason (
fits,uncovered_droporpreview_error),inputTokens,maxStreamTokensand the preview'sfinalTokens,budgetTokensandclearedBy. - Make the docs match the code on
UncoveredDropError. - For a stream whose restart was declined, either treat its
context_lengthas the context refusal it is (alert and drain), or say in the breaking fragment that this ending raises nocontext-refusalalert and kicks no drain.
- Emit a trace on each decline, or once per stream, with its reason (
Minor
R1-4: The decline log can say "the stream goes on" just before a physical-window restart
src/framework.ts:10091. The log is written inside softRestartFits, before overPhysical is computed (:14048-14057). That restart is then taken anyway (:14167). Say only that the maxStreamTokens restart was declined, or log after the combined decision. The unexpected-error test unsets physicalWindowTokens (test/restart-preview.test.ts:286), so it can't see this.
R1-5: The tests don't check that the preview and the restart's compile get the same budget, or the cross-copy path
test/restart-preview.test.ts:364-388.
- The binding test records only
args[2]. - No test checks the budget (
args[0]), uses amaxTokensother than the shared 4,000, or has a pending tool-result-guard reserve. - So replacing
compileBudget()withresolveBudget(), or droppingpendingWireReserveTokens, would leave all 14 green. - Today the wiring is right (
src/agent.ts:546-553,:1550,:2026). - Add a budget assertion with a non-default
maxTokensand a pending reserve, and the cross-copy case from R1-1.
R1-6: The reason given for leaving out kvUnifiedImmutablePrefixHash is gone on main, and the hash couldn't change the verdict anyway
The description and the follow-up comment say the preview omits the hash because ^0.23.0 still admits 0.23.0.
- Main now requires
^0.23.1(26679f23), so after a rebase that reason no longer holds. - The hash only weights cost. In 0.23.1's Pareto solver, feasibility is settled by
forest.minimumTokens(maxTokens)before cache relevance is computed (src/adaptive/kv-unified-pareto.ts:208-214there). Leaving it out doesn't changefits. - Replace that "Not verified" item with this. On rebase, have
previewCompile's doc tie the 0.23.0 plainErrorto a host strategy built from a 0.23.0 copy.
R1-7: The migration note doesn't say how to keep 0.28.0's ceiling
changelog.d/restart-preview.breaking.md:9-16. A host that used maxStreamTokens with contextBudgetTokens as a cost ceiling now has requests that grow from the budget up to the provider's window: 9 tool calls instead of 4 in R1-3's repro, each past the budget. The only switch that brings a ceiling back is physicalWindowTokens. That restart is taken whatever its compile would do, so setting it below the provider's window caps the turn there. Say that in the migration note for hosts whose budget is a cost bound rather than a window.
Verified fine
- No remnant of the turn hold. The src diff has no
holdCompression/releaseCompression. The only uses are the history hold intest/restart-preview.test.ts:260-261. The breaking change at this head is the declinable restart, not a hold. - Runs on main. The PR merges cleanly onto main. On main plus this PR, with npm's context-manager 0.23.1 and membrane 0.8.0 on Node 22.22.1:
- build succeeds;
restart-preview.test.tspasses 14 of 14;- the whole suite runs 8,807 tests, of which 8,743 pass, 0 fail and 64 are skipped.
- Preview and compile agree on what they're given.
- Both take
compileBudget(), including the guard's pending reserve. CONTEXT_MANAGER_DEFAULT_BUDGETmatchesContextManager.compile's default (context-manager.ts:1226-1229in 0.23.1).- The thinking binding is the compile's.
- The preview omits
identity, which it doesn't read.
- Both take
- Unstored injections don't change the verdict.
compilepassesbeforeUser/afterUserinjections into the select only for calibration (autobiographical.ts:14095in 0.23.1), not against the budget. - What the preview counts. This boundary's results are stored (
src/framework.ts:13831) before the preview runs (:14028), so it counts them. Its overlap guard is unreachable while the select is synchronous. - Same-copy and other-restart behaviour is as stated.
- 0.23.1 returns
nullfor a hierarchical select that fits, and an infeasible preview when it doesn't. - 0.23.0 throws exactly the plain
Errormatched here (autobiographical.ts:2648in 0.23.0). - The physical-window restart and the binding-with-window exemption are untouched.
- 0.23.1 returns
- The subconscious is unaffected. It runs
WindowedPassthroughStrategy, which has no preview, so its restart is taken as before. The fragment quotes 0.28.0's entry accurately (CHANGELOG.md:804-810). - Membrane claims hold.
inputTokensis cumulative over a stream's calls, leaves out cache reads and writes where the convention is known, and passes the provider's prompt count through for'unknown'(membrane 0.8.0src/utils/usage.ts). - Citations and naming. The description's line citations at
59d60958are accurate (:14007-14010,:20017,:13446,:5643-5644,:5386-5390,:5435). The fragment names followscripts/release-changelog.mjs.
Method. Two passes, merged after re-verifying each claim.
Sol pass: Codex gpt-5.6-sol at xhigh effort, read-only sandbox, about 8 minutes, 76 commands, 5.40M input tokens (5.21M cached), 28.1k output (17.1k reasoning). It read the diff against main, context-manager 0.23.0 and 0.23.1, and membrane 0.8.0 from source. It could not install or run anything (no network, no node_modules).
Claude pass: fetched main and the PR, merged them, installed from npm, and built and ran the new file and the whole suite. It also ran four instrumented A/B scripts on main and main plus this PR: the cross-copy strategy, the 30-round gate sampling, preview timing on larger stores, and the no-window ending.
Who caught what: R1-1 and R1-2 were found by both passes independently; the reproductions and measurements are Claude's. R1-3 is Claude's, with Sol catching the UncoveredDropError docs mismatch inside it. R1-4 and R1-5 are Sol's. R1-6 and R1-7 are Claude's.
Rejected: Claude also suspected that unstored injections, the missing prefix hash, and the preview overlap guard could make the preview disagree with the compile. All three were refuted (see Verified fine).
Not reached: a real provider, a production store, or context-manager's default kv-unified policy under timing.
Answers the review of the declinable restart (R1-1 to R1-7). - R1-1: a hierarchical strategy built from another copy of context-manager throws that copy's refusal from previewContext, which this copy's ContextManager turns into null only for its own class. The refusal is now matched by its name or its message, so a 0.23.1 copy's NoFoldPlanToPreviewError and a 0.23.0 copy's plain Error both read as a compile that fits. An UncoveredDropError is matched by name too. - R1-2: a restart declined because its compile won't fit is remembered for its stream with what its preview rested on: the budget, the compression holds, the live context settings, and the strategy's compression state (its counts of compressed chunks and summaries where it reports them, and otherwise its pending work). The preview, a full synchronous select, isn't asked again until one of them changes. While the stream holds the agent's provider gate, the compression the preview waits for stays parked behind that gate: in the reviewed 30-round case, 15 boundaries declined on one stream with the same answer, and they now run one select. A decline whose preview says a newer message could make room is asked again at each boundary, as are a failed preview and an uncovered drop. The preview isn't asked at a boundary that ends the turn, while the framework stops, or where the window projection restarts the stream (R1-4): that restart is the window's, and is traced as physical_window even where the stream is past maxStreamTokens too; its boundary leaves the turn's routing to the restart, as 0.28.0 did for every stream past maxStreamTokens. Tests pin the memo, and its re-ask on a change of each kind of thing it rests on: the budget, a compression hold, a live setting, the pending work and a count. - R1-3: each boundary that declines the restart emits inference:restart_declined with its reason (wont_fit, uncovered_drop or preview_error), the stream's count and maxStreamTokens, the preview's figures, and whether it previewed (a remembered decline carries the figures of the preview it took). With no physicalWindowTokens, such a stream that outgrows the provider's window ends there with a context_length failure row, and that ending now raises the context-refusal alert and kicks the compression drain, as 0.28.0's refused restart compile did. The docs say what is logged and traced. - R1-5: tests that the preview and the restart's compile get the same budget, with a maxTokens other than the default and a pending tool-result reserve, and the cross-copy refusals. - R1-6: previewCompile's doc ties the plain Error to a strategy built from a 0.23.0 copy. - R1-7: the migration note says physicalWindowTokens is what bounds a turn's requests for a host whose budget bounds cost.
…orts A declined restart's remembered preview is keyed on the strategy's counts of compressed chunks and summaries where getStats reports them, and otherwise on its pending work. A getStats that returned an object holding none of those counts keyed on nulls, so a compression landing never changed the key, and the decline was never previewed again for the rest of its stream. The key now takes the counts only when any of them is a number, and the pending work otherwise. Tests: such a strategy's decline is previewed again once its pending work changes; a decline that named no way to make room while compression work was pending is previewed again once a compression lands; a decline whose preview gives no clearedBy is remembered until the budget changes; the window projection's restart trace gives physicalWindowTokens as its budget and the projection as its inputTokens, and the stream's inference-log entry records physical_window; a present-while-acting restart is traced as the window projection's where it forces one; the context-refusal alert of a declined stream advises physicalWindowTokens; a hierarchical preview this copy's ContextManager answers as null is seen as null, and another 0.23.1 copy's refusal is matched by its name alone; the long turn's declines say the compile won't fit and come after its restarts; and a remembered decline carries the figures of the preview that declined. The docs say a decline whose preview failed or met a drop no summary covers is previewed again at each boundary, as one a newer message could clear is, and that a pending-work key can stay the same across a landing. Two comments no longer recount release history.
|
@Anarchid #842 is now at R1-1, a refusal from another copy of context-manager. The refusal is now matched by its name, R1-2, a preview at every boundary.
R1-3, no record of a decline, and an ending without the alert or the drain.
R1-4, the log before a window restart. The preview isn't asked where the window projection restarts the stream, so that boundary declines nothing and logs nothing. The restart's trace gives R1-5, the budget and the cross-copy path. "The restart's preview is given the budget the restart's compile is given, the agent's maxTokens and a pending tool-result reserve included" sets R1-6, the prefix hash. R1-7, the migration note. Runs in clean checkouts whose files are — Daniel-2612 |
|
@antra-tess Here is the trade #842 makes, setting by setting, on long agentic turns configured the way you described your residents: What ran. A mock provider and one long turn: 30 or 40 tool rounds, each tool result about 1,000 tokens. context-manager 0.23.1 under kv-unified with a 6,000-token recent window,
" Where In one of them (30 rounds, a 30k window, budget 16k, Our last reply to you said the next boundary over the budget asks again, and that has changed. #842 now remembers a decline like this one, which named compression as what would make room, and previews that stream's restart again only once something the preview rested on changes, such as a compression landing or the budget. Where #842 finishes and
Over the grid's 162 settings, — Daniel-2612 |
The merge of main met two of its rules at a tool boundary: the delivery rule's notice that someone addressed the agent from another channel, and the latest-inbound re-pin. Both skipped a boundary that restarts past maxStreamTokens. The merge keeps that skip as budgetRestart: past maxStreamTokens and restarting, whether the window projection forces the restart as well or the restart's preview fits. A boundary whose restart is declined goes on with its stream, so it gives the notice and re-pins as any other boundary does. The re-pin's window case already tests that. These two cases test the notice under the delivery rule: a budget restart with an addressed message from another channel stores no notice at its boundary, and the restart's start tells the agent once, "at your budget restart", with and without the window projection forcing the restart as well. With the notice gated on overBudget, which is false when the window projection forces the restart, the window case fails at its boundary.
Once a stream's
inputTokenspassmaxStreamTokens, the next tool boundary declines the restart when context-manager'spreviewContextsays the restart's compile won't fit, or when the preview throws anything but context-manager's refusal to preview a hierarchical strategy, and the stream goes on. Each boundary that declines it emits aninference:restart_declinedtrace. A decline because the compile won't fit, whose preview doesn't name a newer message as what would make room, is remembered for its stream, and the preview isn't asked again until the budget, the compression holds, the live context settings or the strategy's compression state changes. With nophysicalWindowTokens, such a stream can go on until the provider refuses a request as too long, and thatcontext_lengthending now raises thecontext-refusalops alert and kicks the compression drain. A strategy with no preview restarts as before, and so does a hierarchical strategy whose preview gives that refusal, now matched by its name or its message. Since00572ba111, the branch has mergedmainate85c57db9a,94ed06569aanswers the review of that head (R1-1 to R1-7), anda740da8040follows it with a fix to how a decline is remembered, and more tests. Since then,26bc74f94amergesmainat0f11975e48, andcd581ebe11adds two tests where that merge meetsmain's notice that someone addressed the agent from another channel.Problem
On
main(0f11975e48), the restart condition compares the stream'sinputTokenstotal withmaxStreamTokens, and nothing in it asks what the restart's compile would do (src/framework.ts:16521–16524). With0f11975e48'ssrc/framework.ts,src/agent.ts,src/types/agent.tsandsrc/types/trace.ts, four of this change's tests fail on their check that the turn ends without a failure row, finding anover_budgetone: the three whose names begin "a maxStreamTokens restart whose preview can't fit is declined", and a 30-round kv-unified turn with nothing awaiting at its tool results.Changes
The restart decision (
src/framework.ts:16626–16631,declineBudgetRestartat:12323–12347). PastmaxStreamTokens, the boundary asksAgent.previewCompileand declines the restart when:fits: false);UncoveredDropError, matched by its class or by its name;[context-budget]line (:16635).A preview that fits, or that answers
null, takes the restart. So does an error the decision matches as that refusal, by its name,NoFoldPlanToPreviewError, or by the message of the plainError0.23.0 throws (:1178–1189). The doc above that match says such a refusal reaches it from a strategy built from another copy of context-manager.What a decline rests on (
src/agent.ts:2089–2100,restartPreviewKey). Afits: falsedecline whoseclearedBydoesn't name'newer-message'is remembered for its stream with a key. The key holds the budget the preview was given, the compression holds, the livetailTokens,transitionPaceTokensandpreparedWindowTokens, and the strategy's compression state. That state is its counts of compressed chunks and summaries wheregetStatsreports any of them as numbers, and otherwise its pending work. While the stream and the key are unchanged, a later boundary takes the remembered decline and runs no preview. After a decline that names a newer message, or one whose preview threw an unexpected error, the next boundary asks the preview again. Neither decline carries a key, and nor does an uncovered drop (src/framework.ts:12330–12341).Where no preview is asked (
src/framework.ts:16626). The decision asks no preview at a boundary that ends the turn, or where the window projection restarts the stream. The same condition includes!this.providerAdmissionClosed, which Not verified covers. The window projection's restart gives the reasonphysical_windowin itsinference:stream_restartedtrace (:16822), and the boundary leaves the turn's routing to that restart (:16631).The trace (
src/types/trace.ts,src/framework.ts:16632–16648). Each declining boundary emitsinference:restart_declinedwith:reason:wont_fit,uncovered_droporpreview_error;previewed, false for a remembered decline;inputTokensandmaxStreamTokens;wont_fit, the preview'sfinalTokens,budgetTokensandclearedBy, which a remembered decline carries from the preview that declined.The ending (
src/framework.ts:31689–31694,:31753–31758). Acontext_lengthfailure on a stream whose restart was declined is handled as a context refusal. It raises thecontext-refusalops alert, whose advice namesphysicalWindowTokens, and kicks the compression drain.Where the merge of
0f11975e48meets it (26bc74f94a).mainskips two steps at a tool boundary whereoverBudgetholds, the check under Problem (src/framework.ts:16521–16524, read at:16567and:16613, at0f11975e48): underspeech.locus: 'delivery', the notice that someone addressed the agent from another channel, and under'latest-inbound', the re-pin to such a message's channel. The merge skips each where the boundary restarts pastmaxStreamTokens(budgetRestart,:16631, read at:16662and:16709), with the window projection forcing the restart as well or not. With a message from another channel addressing the agent at a budget restart, under'delivery', the boundary stores no such notice, and the restart's start tells the agent once. In these tests the message names no author, and that notice opens "[routing] Someone addressed you from discord:dm:antra at your budget restart." (test/present-while-acting.test.ts:1548).authorName(src/framework.ts:697–698) gives "Someone" only where the message's metadata names no author. Under'latest-inbound', a budget restart the window projection forces as well re-pins to such a message, and moves nothing for one withdrawn before it starts.src/agent.ts.Agent.previewCompile(:2071–2078) callspreviewContextat the budget and with the thinking binding the compile is given. It andrestartPreviewKeyare marked internal.Docs. The docs of
maxStreamTokensandphysicalWindowTokens, and README's budget restart step, say whatmaxStreamTokenscounts, when the restart is declined, what a remembered decline rests on, and how a declined stream with nophysicalWindowTokensends. They say that where membrane knows the provider's cache convention,inputTokensleaves out cache reads and writes, so there, on a mostly cached prompt,maxStreamTokensdoesn't bound a request's size, andphysicalWindowTokensdoes. The first commit,ffc26be96b, carries most of the part on what the count is.Changelog.
changelog.d/restart-preview.breaking.mdsays what hosts meet.changelog.d/subconscious-window-guard.fixed.mdcorrects 0.28.0's entry calling themaxStreamTokensrestart a subconscious's only window guard.Who needs to act
maxStreamTokensrestart and sets nophysicalWindowTokens. A stream whose restart is declined goes on, where onmainthe restart is taken. In the new tests such a turn then answers, or, when a request outgrows the provider's window, ends with acontext_lengthfailure row, thecontext-refusalalert and the drain. WithphysicalWindowTokensset, a restart the window projection forces asks no preview.contextBudgetTokensbounds what a request costs, rather than the provider's window. The breaking fragment's migration note says that a declined restart lets the stream's requests grow toward the provider's window, and thatphysicalWindowTokensis what bounds them.inference:stream_restarted. A restart the window projection forces givesphysical_window, with the projection asinputTokensandphysicalWindowTokensasbudget, where the stream is pastmaxStreamTokenstoo. Onmainthat restart givescontext_budget.A restart whose preview fits, and a strategy with no preview, such as
PassthroughStrategy, are unchanged.Tests
test/restart-preview.test.tsis new, with 29 tests. They cover:NoFoldPlanToPreviewError(answered asnull), as an error of another class namedNoFoldPlanToPreviewError, and as a plainErrorwith 0.23.0's message, all taking the restart, and a strategy with no preview;UncoveredDropError(declined and traced, not logged);physical_window;clearedBywas empty;maxTokensother than the default and a pending tool-result reserve, and the thinking binding, for a model that binds its thinking;context_lengthfailure row, thecontext-refusalalert advisingphysicalWindowTokens, and the drain;physicalWindowTokens:test/present-while-acting.test.tsadds four cases of a message from another channel addressing the agent at a budget restart. Under'latest-inbound', two where the window projection forces the restart as well check that the restart givesphysical_windowand how the turn is routed. Under'delivery', two fromcd581ebe11, with and without the window projection forcing the restart, check that the boundary stores no notice that someone addressed the agent from another channel, and that the restart's start tells the agent once.Checks
On Linux with Node 24.22.1, context-manager 0.24.1, chronicle 0.5.0 and membrane 0.8.0, at this head,
cd581ebe11:restart-preview.test.tsand 99 inpresent-while-acting.test.ts.0f11975e48'ssrc/framework.ts,src/agent.ts,src/types/agent.tsandsrc/types/trace.ts, 28 of the 128 fail: 25 of the 29 new tests, and three of the fourpresent-while-actingcases above, each at its check that the restart is the window projection's. The fourth, under'delivery'with no window projection, passes there too. The build reports 20 type errors, all in the new test file's uses of the new trace, and still emits.budgetRestartreplaced by this change'soverBudget(src/framework.ts:16628), which is false where the window projection forces the restart. With the notice's, "a budget restart the window projection forces as well gives no addressed-elsewhere notice at its boundary under the delivery rule" fails, and the other 127 pass. With the re-pin's, at26bc74f94a, beforecd581ebe11's two tests, "a budget restart the window projection forces as well moves nothing for such a message withdrawn before it starts" fails, and the other 125 pass. These two runs print Node 24.22.1 and the same package versions, and no platform.The checks below ran before the merge of
0f11975e48, and so did the settings in our comment of October 28 at 13:27 UTC on the trade this change makes, which ran ata740da8040, and formainate85c57db9a, each with#867's5f26d006b9merged in. On Linux with Node 24.22.1, membrane 0.8.0, chronicle 0.4.0 and context-manager 0.23.1, in clean checkouts whose files area740da8040's:restart-preview.test.tsand 95 inpresent-while-acting.test.ts.e85c57db9a'ssrc/framework.ts,src/agent.ts,src/types/agent.tsandsrc/types/trace.ts, 27 of the 124 fail: 25 of the 29 new tests, and the two newpresent-while-actingcases. The build reports 20 type errors, all in the new test file's uses of the new trace, and still emits.UncoveredDropErrorname match each fail their own cross-copy test.getStatsexists fails 1, and on the pending work alone fails 2.context_budgetfails 3, and leaving the routing to it only wheremaxStreamTokensalone forced it fails 1.Not verified
previewCompilepasses nokvUnifiedImmutablePrefixHash(src/agent.ts:2073–2077), which the restart's compile can be given (:1870). No test in this change compares a preview's verdict with and without it.src/agent.ts:2094–2099lists. The rows a stream adds aren't in it, and neither is context-manager's calibration multiplier. No test in this change drives a remembered decline that new rows, or a fall in the multiplier, would let fit.getStatsreports none of the counts as numbers. Its key holds its pending work's text (src/agent.ts:2098). No test in this change drives a compression landing that leaves that text reading the same.afterUserinjections. No test in this change drives it.!this.providerAdmissionClosed(src/framework.ts:16626), whichstopOwnedsets (:5481). With that term removed ata740da8040, all 124 tests then in the two files still passed, so they don't show what the term does.previewContextthe test overrides, errors of another class namedNoFoldPlanToPreviewErrororUncoveredDropErrorwith words of the test's own, and a plainErrorwith 0.23.0's message (test/restart-preview.test.ts:240–244,:315–324). No run builds a strategy from a second copy.maxStreamTokens. No test in this change has a message from another channel address the agent at either.changelog.d/restart-preview.breaking.mdandchangelog.d/subconscious-window-guard.fixed.md.Paloma-2644 wrote the change, and Jerome-2511 wrote its first commit. Ada-1017 made the design calls. Ezra-2423 and Alfred-2570 read the review round and the fix commit, Alfred for whether each part pays for what it adds, and an internal review read the round and an earlier version of the fix commit. For the merge of
0f11975e48, Ezra-2423 read where it meets this change, andcd581ebe11's tests. Daniel-2612 wrote this description, and Lottie-2661 its update for that merge.