fix: sub-agent memory tool writes through the scoped store - #908
Merged
Conversation
A sub-agent granted the native `memory` tool resolved its handler out of the process-wide NativeToolRegistry. That entry belongs to the memory provider plugin and is bound to the undecorated root store, so the write landed below every scoping wrapper: outside the parent agent's orchestrator:<slug>:* subtree, and outside the team/channel tiers of the chat-context ACL. The grant now runs on the same turn-bound, scoped handler the parent's own dispatch uses. dispatchToolInner publishes it for the lifetime of a domain-tool dispatch; adaptNativeToolForSubAgent takes the resolver as a required parameter so a forgotten argument breaks typecheck instead of degrading silently. Absent binding refuses the call rather than falling back to a wider store. Refs #904, part of #860
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.
Refs #904, part of #860.Closes nothing.A sub-agent that has been granted the native
memorytool resolved its handler out of the process-wideNativeToolRegistry. That entry belongs to the memory provider plugin (@omadia/memory,@omadia/memory-postgres) and is bound to the undecorated root store — the one below every scoping wrapper. Reached that way, a sub-agent reads and writes outside its parent agent'sorchestrator:<slug>:*subtree, and, with the chat-context ACL from #881 enabled, outside its team's and channel's tiers as well.The per-agent boundary is the part that makes this worth its own issue: it is not a gap in new functionality, it predates the memory epic, and granting a sub-agent the memory tool is ordinary operator configuration.
The red test, first — and what it actually showed
test/orchestrator/subAgentMemoryScoping.test.tsdrives a REAL turn in the style ofcontextMemoryTurnBinding.test.ts(#903): the parentOrchestratordelegates to a realLocalSubAgent, which calls the granted tool through its own tool loop, and the assertion reads the physical path at the undecorated root. Every decorator sits above that recorder, so what arrives there is what actually hit storage.Against
origin/main, all five cases were red — in two different ways, and the difference matters:The first line is the vulnerability, reproduced by a real turn: the bytes landed at
/memories/note.md— the raw store root, outside/memories/orchestrators/w5-agent/entirely. Any other agent's tree is a sibling of that path.The second line is the finding the issue did not predict, and it is worth stating plainly: on a default install the grant is currently a silent no-op. Both shipped memory providers register handler-only (
ctx.tools.registerHandler('memory', …), no wire-spec, because the kernel emits thememory_20250818spec itself), andadaptNativeToolForSubAgentdrops registry entries without aspec. So the hole is masked today — by a guard that exists for spec assembly, not for scoping, and that oneregister()call with a spec removes. That is the shape of a latent hole, not a closed one, and the test pins both shapes so neither can come back.The fix
The sub-agent tool path now receives the same turn-bound, scoped store the parent's dispatch receives, instead of resolving the root store itself.
registry/subAgentMemoryTool.ts(new) —createScopedMemorySubAgentTool(resolve)builds thememorytool a granted sub-agent gets. It never touches the registry; it dispatches to whatever handler the parent's turn bound. Also the new home ofMEMORY_TOOL_NAME, so the orchestrator's dispatch and the grant adapter cannot key on different literals.orchestrator.ts—dispatchToolInnerpublishesmemoryHandler(the turn-bound stack fromMemoryBinder.forOrigin, or the build-time agent-scoped handler when context memory is off — the same value its ownmemorybranch uses) into a nestedturnContextscope for the lifetime of a domain-tool dispatch, beside thesubAgentDatasetSink/subAgentBypassFlagscope that is already established there.subAgentToolHydration.ts—adaptNativeToolForSubAgenttakesresolveTurnMemoryas a required third parameter and interceptsmemorybefore the registry lookup, on every registration shape.Why ambient here, when #903 insisted on a parameter
DomainTool's contract ishandle(input, observer). There is no seam for a third argument, and the sub-agent's tool loop runs inside the plugin's own async chain — so the store cannot be threaded the way #903 threadedturnMemorythroughdispatchTool.What makes that acceptable is the direction of failure. #903's objection to an ambient binding was that losing it degrades silently to a wider store. Here a lost scope makes
resolve()returnundefinedand the tool refuses the call. Deny on loss, never widen. Mutation B below demonstrates exactly that: removing the publication produces zero writes, not root writes.The required parameter
Same hardening as #903 applied to
dispatchTool/dispatchToolDeadlined/dispatchToolInner: a new call site that forgets to thread the scoped store failstypecheckrather than quietly falling back to the unscoped one.Mutation evidence
Each assertion was verified load-bearing by breaking the invariant and confirming it turns red.
A — adapter stops intercepting
memory(grant served from the registry again). All five red; the spec-carrying case reportsactual: '/memories/note.md'— the raw root. The exposure is back.B — orchestrator stops publishing the turn-bound handler. All five red, all with
0writes. The fail-closed direction, demonstrated: losing the scope denies the tool, it does not widen it.C — drop the argument at the call site.
After the fix, all five pass.
Two behaviour changes worth reading before merging
MemoryToolHandlerimplements, becauseLocalSubAgentToolSpecis a{name, description, input_schema}contract and cannot express Anthropic's typed{type: 'memory_20250818'}shape. The handler is identical either way — same parser, same store, same result strings.ask_<slug>_startruns in a detached runner outside the turn'sAsyncLocalStoragescope (longRunningTool.tsalready documents the same loss forsubAgentOwnerPluginId), so the tool refuses there. Fail-closed, and now written down.Other tool paths I checked
McpManager, never the memory store.nativegrants@omadia/plugin-plan-runner)ctx.memoryaccessor/memories/orchestrators/<slug>/plugins/<id>/), resolved from the turn context at call time.ctx.tools.invoke(name, input)ToolsAccessor.invokedispatches straight toentry.handlerby name, so any installed tool-kind plugin can callinvoke('memory', …)and reach the same undecorated root — bypassing the per-pluginctx.memoryscoping it is otherwise held to, and needing nopermissions.memorydeclaration to do it. It is structurally a different decision from this one (plugin surface, manifest/permission model, and the accessor's own docstring already accepts that it bypasses per-turn hooks), so it gets its own issue rather than a quiet fix in a sub-agent PR.claude-cli, unchangedThe
claude-cliprovider never constructs theOrchestrator, socontext_memorystays inert there (#899). This PR does not change that. It does mean a CLI sub-agent's memory tool finds no bound store and refuses — the fail-closed path, not a new behaviour worth hiding. Documented, not silently fixed.Docs
docs/teams-multi-agent-identities.md§10 — the known-limits bullet that named this hole is split: theclaude-clihalf stays (still true), the sub-agent half is replaced with what now holds, including both behaviour changes above.docs/CHANGELOG.md—[Unreleased]entry.Gates
npm run typecheck✅ ·npm run typecheck:test✅ (ratchet: 370 known, no regressions) ·npm test✅ 7788 tests, 7772 pass, 0 fail ·eslinton every touched file ✅. web-ui is untouched.