Repository navigation
Conversation
…writes Passes a host-supplied membrane to the resident's context manager as compressionMembrane (context-manager companion PR), so its compression, merges and transition summaries can go to another provider or credential than the framework's membrane (anima-research/connectome-host#615). The calls are wrapped as the agent's maintenance work: withdrawn under a surgery's hold or once stop() closes admission, and counted in flight so a surgery waits for them. They skip the agent's provider gate, its held models and its stated waits, and a failure on them records no wait and arms no hold: those belong to the agent's own provider. membraneAdmitting takes the membrane to wrap (default: the framework's). createAgent refuses a context manager without compressionMembrane rather than let memory writes go to the agent's own provider unannounced. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Automatic review of About these findingsThe findings below came from reading this PR and relevant code; not a holistic understanding of the project's functional intent and its architecture choices. Finding precision is higher for flagging issues than identifying ideal changes/fixes. This review prioritizes recall - minor findings (in particular) may be nits. Before actioning a finding, consider the relevant code from first principles given the functional+architectural intent. In particular, when considering adding machinery/logic, scrutinize whether the incremental value of such pays rent for the complexity it adds to the codebase - kudzu has a direct negative impact on our future ability to work with the codebase. Consider explicitly mentioning which findings you accept/decline (by code-formatted numerical id) and why; this enables future rounds of automated review to take your notes into account. Findings, most severe first:
|
Contributor
|
We read this at
CreditsOctavia-2412 read the change and ran these probes, and Ada-1017 made the calls and wrote this comment. — Ada-1017 |
Anarchid
reviewed
Nov 10, 2026
Anarchid
left a comment
Anarchid
left a comment
Collaborator
There was a problem hiding this comment.
Nightly dual review · round R1 at ca04367 · 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 ca04367 against main 36b6dfe
Verdict: changes requested
The resident wiring is small and correct as far as it goes. All three PRs use the same names, and the routing works: with context-manager main + #463 built into main + this PR, both routing tests pass. Three things stop it merging as it stands:
- No context-manager the package declares, or that is published, can serve the option.
mainhas moved under the PR. Since #361 every manager-call wrapper reports a landed call, and the new one doesn't, so the budget-hold fix for #304 doesn't apply to these residents.- The wrapper keeps the part of the agent's gate that makes turns wait for compression. It drops the part that told an operator a compression call failed. And the reason the prose gives for keeping the counter (a surgery waits for these calls) isn't how surgery works.
There is no earlier review of ours on this PR, so there are no dispositions to give. Where a finding below was also raised by the automatic review or by the run-based comment, I say so.
On the description: the branch is now 21 first-parent commits behind main, not 2. In between, main raised context-manager to ^0.27.0 and merged #361. The merge is still textually clean, but R1-2 is a semantic conflict.
Blockers
R1-1. No declared or published context-manager can serve compressionMembrane.
- Evidence:
package.json:52declares^0.25.0. After the merge it'smain's^0.27.0.- The latest published release, 0.27.0 (context-manager
main34131f38), has nocompressionMembraneand nogetCompressionMembrane. - context-manager#463 is open and based on the 0.25.0 release commit. It merges cleanly onto context-manager
main, but it will ship as a minor release (0.28.0 at the earliest), which^0.27.0doesn't reach.
- What I ran:
mainmerged with this PR, 0.27.0 installed. That is what CI'snpm installand the dependency-floor legs resolve.test/compression-membrane.test.ts: 1 pass, 2 skipped. The only reachable outcome of setting the option iscreateAgent's refusal.- With context-manager
main+ #463 built in instead: 2 pass, 1 skipped, andtscis clean either way.
- Failure: merged now, the PR ships an
AgentConfigfield that every supported install refuses. Its two routing tests skip in every CI lane, so nothing in CI ever exercises the feature. - Fix: land this after context-manager releases #463, and bump the range in this PR to that release.
- Once the range floor has the API, the post-open refusal and its test can't be reached under the declared range. Either drop them, as the run-based comment suggested, or keep a cheap preflight in
create()for out-of-range installs (R1-9).
- Once the range floor has the API, the post-open refusal and its test can't be reached under the declared range. Either drop them, as the run-based comment suggested, or keep a cheap preflight in
- Found by: both reviewers; also the automatic review's 1.
Major
R1-2. Semantic merge conflict with main: calls on the separate membrane never report managerCallLanded.
- Evidence:
- #361 (the #304 fix, merged into
mainon 2026-11-09, after this head) makes every manager-call wrapper report a landed call:withAuxiliaryAdmission(mainsrc/framework.ts:6854,if (landed) this.managerCallLanded(agentName))conversationAuxiliaryMembraneFor(main:7165)
managerCallLanded(main:7099-7105) counts the call on the agent's budget hold and schedulesreleaseBudgetHoldOnEvidence.budgetHoldMovedfalls back to that count when the manager offers no progress snapshot (main:7679-7684).- The new wrapper (
src/framework.ts:6940-6951here) merges textually clean and never calls it. I confirmed this on the merged tree.
- #361 (the #304 fix, merged into
- Failure: a resident's turn is refused at compile and put on a budget hold. Compression then lands on the separate credential. No evidence check is scheduled for that call. Without a progress snapshot, the hold reads as unmoved until compression stops pending.
- That is #304's symptom ("compression recovers, but failed activations are not retried"). It comes back on exactly the residents this option is for: the ones with compression debt.
- Fix:
- Rebase onto
main. - Mirror
withAuxiliaryAdmission: setlandedon success and callthis.managerCallLanded(agentName)in thefinally. - Add a budget-hold test that runs through the compression membrane.
- Rebase onto
- Found by: Codex only; I verified it on the merged tree.
R1-3. Counting these calls in the agent's auxiliaryInFlight doesn't make surgery wait, as the prose says, and it does make turns wait on the other credential.
- Claims:
- JSDoc at
src/framework.ts:6932-6933("counted in flight, so a surgery waits for them") src/types/agent.ts:57-58changelog.d/compression-membrane.added.md:4-5- the PR body ("Calls count in
auxiliaryInFlight, so a surgery waits for one that's out")
- JSDoc at
- Wiring:
- Surgeries and
stop()wait onmaintenanceRunning()(src/framework.ts:7764-7766): a maintenance pass or an over-budget drain. The callers are at:6378,:15530,:16947and:17145. None of them readsauxiliaryInFlight. - The only thing that waits on that counter is a turn's start (
:26691→waitForAuxiliaryIdle,:6646). Its stated reason (:26769-26773) is that an auxiliary call in flight may record a provider wait that binds this turn. Calls on this membrane record nothing, so that reason doesn't apply to them.
- Surgeries and
- Failure: the strategy's own tick (
autoTickOnNewMessage) starts a write on the other credential.- An operator's
switchBranch/rollback, orstop(), goes ahead without waiting for it. The run-based comment on this PR reproduced both. context-manager's branch fencing discards the stale result, so nothing is written to the wrong branch, but the documented contract doesn't hold. - A turn woken in the meantime parks until the other provider answers, which can take up to the inference bound.
- An operator's
- Fix:
- Preferred: count these calls in their own per-agent counter, leave them out of
gate.auxiliaryInFlight, and have surgery andstop()await that counter. These calls never park, so the wait is bounded byinferenceBounds. - Minimum: correct the four pieces of prose.
- Preferred: count these calls in their own per-agent counter, leave them out of
- Found by: both reviewers; also the automatic review's 2 and 4.
R1-4. A failure on the compression credential no longer reaches any framework signal.
- Evidence:
- Through
main's route, a failed memory write records a provider wait and arms a hold (src/framework.ts:6677-6687). That gives a[provider-wait]line and aprovider-waitops alert that leads toreleaseProviderWait. - The new wrapper (
:6941-6950) has nocatch. - context-manager pauses its lane and rethrows. A failure inside a scheduled maintenance pass therefore still reaches you as a generic
context-maintenance-failedalert. Writes started by the strategy's own tick or speculative drain only leave context-manager's own log lines, and those are the writes that run while the pass skips the resident (R1-5). - After the first failure the paused lane makes no further calls, so nothing throws again.
- Through
- Failure: this option adds a new failure domain. The compression key can be revoked or run out of credit, or the gateway can go down. Memory writes then stop, with no framework alert naming the cause, until the resident runs over budget.
- The run-based comment showed the sharp case. A stated wait too long to hold pauses the lane until someone releases it explicitly, and nothing points an operator to that release.
- Fix: in the wrapper,
catch, raise an ops alert per agent under its own kind and key (for examplecompression-membrane, naming the agent and the error), then rethrow. Still record nothing against the agent's own model. - Found by: Claude only; also the run-based comment's first item.
Minor
R1-5. Scheduled maintenance still skips the resident while the agent's own provider gate is blocked.
- Evidence:
src/framework.ts:7826(main:8463), viaproviderGateBlocked(:6612-6622). The gate counts as blocked during a turn, while a primary start is pending, and under an acceleration or host hold. - Failure: with writes on another credential, a long hold on the agent's own provider still stops scheduled compression. A 429 on the agent's turns arms an acceleration hold, which a stated wait lengthens (
holdProviderAcceleration,:7173on). With a subscription's usage limit, the case #615 is about, that hold can last hours. Only the strategy's own tick on new messages, and the breaker's drain, advance compression then. - Fix: when the manager reports a compression membrane, don't skip the resident for acceleration or host holds. Keep skipping during a turn if ticking mid-turn is unwanted.
- Found by: both reviewers; also the automatic review's 3.
R1-6. No test pins the wrapper.
- What I ran: a mutation in the built framework. I replaced
this.separateCompressionMembraneFor(config.name, config.compressionMembrane)withconfig.compressionMembrane. Both routing tests still pass. - Untested: the withdrawal under a surgery's hold, the withdrawal after
stop(), the inference bound, and the in-flight accounting. - Fix: add tests that a write issued under a surgery's hold, and one issued after
stop(), never reach the compression membrane. After R1-2, add the budget-hold release. - Found by: Claude only.
R1-7. The refusal checks the manager, but the strategy does the routing.
- Evidence:
createAgentrefuses whengetCompressionMembrane()is undefined (src/framework.ts:18100-18101).- In #463 the routing is
AutobiographicalStrategy.compressionMembraneFor(ctx), a method of the strategy instance the host passes in. - A strategy from another context-manager copy, or a custom strategy that calls
ctx.membrane, passes this check and still writes to the agent's provider, without a word.
- Concrete case: connectome-host's
FrontdeskStrategyextendsAutobiographicalStrategyfrom the host's own@animalabs/context-managerdependency (^0.25.0), not from this package's re-export.- Once this package's range passes the host's, npm installs two copies.
- A frontdesk recipe then passes both this check and connectome-host#617's, and still compresses on the agent's provider.
- Fix: have #463 expose a capability on the strategy (a public marker or method). Refuse when
compressionMembraneis set and the strategy doesn't declare that it routes it. - Found by: Codex raised the frontdesk copy on the host side; the generalisation is Claude's.
R1-8. AgentConfig.compressionMembrane is accepted and ignored everywhere except FrameworkConfig.agents.
- Evidence: each of these takes an
AgentConfigand never reads the field:createEphemeralAgent(config)(:11380, opens at:11455)resumeAgent(name, config)(:11762,:11819)deriveAgent, which spreads the parent's config (:11561)- conversation forks, from the template's config (
:20127)
- Failure: the field's doc says "Covers the resident's own context only". Still, a host that passes it explicitly to
createEphemeralAgentgets the silent fallback thatcreateAgent's refusal exists to prevent. The same applies to conversation forks and subagents of a resident set up for #615: their compression stays on the agent's provider, and so do its refusals. - Fix:
- Refuse an explicit
compressionMembraneon those entry points. - Strip the inherited one in
deriveAgentand the conversation-fork config, so it doesn't look as though it applies there. - Or route it. #463's
derive/reopenDerivedalready take an explicitcompressionMembrane.
- Refuse an explicit
- Found by: Claude only.
R1-9. The refusal runs after the store is opened and the manager initialised.
- Evidence:
- The check (
src/framework.ts:18098-18107) runs afterContextManager.open.openregisters the manager's states and initialises the strategy, which can migrate or repair stored state. create()states its own rule: "a refused config must leave nothing behind" (:5125). ItspresentRowscapability check runs before any store is opened (:5136-5141).
- The check (
- What I ran: on an already-initialised store, a refused start added no records. So the effect is limited to first opens and migrations.
- Fix: if a refusal survives R1-1, preflight
typeof ContextManager.prototype.getCompressionMembrane === 'function'for any agent that setscompressionMembrane, next topresentRows. The test already detects support this way. - Found by: both reviewers.
The three PRs' interface
They agree on names, types and the check:
- connectome-host#617 sets
AgentConfig.compressionMembrane(itssrc/index.ts:501-506, and again on session switches). - This PR passes a wrapped proxy as
ContextManagerConfig.compressionMembrane. - #463 stores it, exposes it as
StrategyContext.compressionMembrane, and returns it fromgetCompressionMembrane(). What comes back is this package's proxy, not the host's object. - Both this PR and #617 test
getCompressionMembrane?.() === undefinedrather than identity, so the proxy doesn't trip #617's check. - Every
derivecall here passes its ownmembrane(:11888,:12102), so under #463's rule children get no compression membrane. That matches the scope all three state.
Where they don't line up:
- Versions. None of the three declares a release the others need.
- The capability check sits on the manager, but the routing sits on the strategy (R1-7).
- Redundant checks. With a new framework and an old context-manager, this PR throws in
createAgentbefore #617's post-create check runs. #617's check only catches a framework without the field. That's harmless.
The desk's premise holds. The three PRs cross-reference each other. connectome-host#615 is open. #617 is the one that closes it; this PR is the framework layer.
Verified fine
- Routing: with context-manager
main+ #463, memory writes go only to the compression membrane (0 calls on the framework's). A 429 on it records no provider wait and arms no hold for the agent. I ran both tests. - Branch switch: the resident's manager isn't reopened on a branch switch or surgery; it is reinitialised in place. So the option survives both.
- Proxy:
membraneAdmitting(admit, base)binds and applies againstbaseand keeps the inference bound (:7070-7094). Existing callers are unchanged. - Call path: context-manager makes compression calls only through
complete(), which is the path the proxy wraps. - Operator release:
releaseProviderWaitstill offers the release to the compression lane whichever membrane it pauses on (:7648-7656). - Secrets:
AgentConfigisn't serialised orstructuredCloned anywhere, so a membrane holding credentials doesn't leak into persisted or cloned state. No framework hooks hang offthis.membranethat compression calls would now miss. - Refusal on an initialised store: the release in
create()stops the framework cleanly, and the store's sequence is unchanged afterwards. - Hygiene:
tscis clean on the merged tree against both context-managers.git diff --checkis clean.
Method. Two passes, merged after re-verification.
- Codex (
gpt-5.6-sol, reasoningxhigh, read-only sandbox, no network) read the diff againstmainand both companion heads: 755 s, 152 shell commands, 6.93 M input tokens (6.48 M cached), 30.5 k output tokens (16.0 k reasoning). It ran no tests, since there were no installed dependencies and no registry access. - Claude fetched current
mainof all three repos and both companion heads, and checked the npm registry. It merged this PR intomainand context-manager #463 into itsmain, then rantest/compression-membrane.test.tsagainst both the published 0.27.0 and the #463 build. It also ran one mutation (bypassing the wrapper) and one refusal probe on an initialised store. - Who caught what: both: R1-1, R1-3, R1-5, R1-9. Codex only: R1-2 and the frontdesk copy in R1-7. Claude only: R1-4, R1-6, R1-8. Every Codex claim was re-checked, and none was rejected.
- Not reached: a live host against a real provider, and the full suite.
|
Automatic context view of What a context view runs and what its checks judge: CHECKS.md. The records of the 79 runs don't differ between the base and this head. In the files profiled hosts loaded, 15 changed source lines that map to code ran in no profiled drive: Usage: the figures of the agents' requests, subagents' included, in the 79 runs are the same in the base and this head: 206,752 tokens sent a request, 72.4% of them read from the cache, 138,515,771 written to it, 4,037,703 output, 24 thinking blocks dropped, USD 0.5465 at list price a priced request and 123 requests with no list price. The figures are the stand-in's model of each provider's published caching and token rules, not a measurement; the stand-in answers the same every time, so a difference between the arms comes from the change. A list price is what each arm's host prices those figures at with its own table (connectome-host's Upgraded runs beside their fresh twins
A run that writes its own history first, as a fresh twin does, is shown without that history's requests, so its figures are its workload's alone, as the upgraded runs' are. Changed source lines and deletions not seen to run, by file
Held the same in base and head: connectome-host |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A resident's context manager gets the framework's own membrane, wrapped in auxiliary admission, so compression always uses the provider and credential of the agent's turns. anima-research/connectome-host#615 asks for compression on another provider or credential. Example: turns on a Claude subscription token, compression through an API key or a gateway, where subscription residents keep getting
reasoning_extractionrefusals on compression. anima-research/context-manager#463 addsContextManagerConfig.compressionMembrane. This PR lets a host supply one for a resident.Changes
AgentConfig.compressionMembrane?: Membrane.createAgentpasses it to the resident'sContextManager.openascompressionMembrane, wrapped by a newseparateCompressionMembraneFor(agentName, base).stop()closes admission, is withdrawn (MaintenanceWithdrawnError).auxiliaryInFlight, so a surgery waits for one that's out.inferenceBoundslike the other wrappers.membraneAdmitting(admit, base = this.membrane)takes the membrane to wrap. Existing callers are unchanged.createAgentrefuses with a clear error when the installed context-manager dropscompressionMembrane(it lacksgetCompressionMembrane(), or that returns nothing). Without this, an older context-manager would ignore the option and memory writes would go to the agent's own provider without a word.changelog.d/compression-membrane.added.md.Tests
test/compression-membrane.test.ts:compressionMembrane, skipped otherwise): the framework's membrane gets 0 calls.providerWaitSnapshot('trunk')empty and arms noproviderAccelerationCooldownsentry. I checked that this test fails when the separate membrane is routed through the existingwithAuxiliaryAdmissioninstead.compressionMembrane):createAgentrejects with the error above. Run against the published 0.25.0, it passes and the other two skip.npm testwithAF_TEST_CONCURRENCY=2, both runs against context-manager #463's build in place of the published 0.25.0 dist:main: 10289 pass / 0 fail / 62 skippedThe difference is the new file: 2 pass, 1 skip.
tsc --noEmitis clean against both the published and the #463 context-manager. The branch is 2 commits behindmainand merges without conflicts.Not verified
withdrawMaintenanceUnderSurgeryandwithdrawMaintenanceWhenStoppingas the other wrappers.Out of scope
Only the resident's own context is covered. Conversation forks (
conversationAuxiliaryMembraneFor), ephemeral runs and derived contexts still compress on the framework's membrane: AF gives each of those children its own membrane, and context-manager deliberately doesn't hand them the parent's compression membrane in that case. Extending this to them is a possible follow-up.Companion PRs
ContextManagerConfig.compressionMembrane). Land it first, then bump this package's@animalabs/context-managerrange to the release that has it. Until then this PR is inert unless a host setscompressionMembrane, and if one does,createAgentrefuses as described.agent.strategy.compressionProvider.🤖 Generated with Claude Code