Skip to content

feat: per-user memory isolation via template scopes#568

Open
eightHundreds wants to merge 6 commits intoCortexReach:masterfrom
eightHundreds:feat/per-user-scope-isolation
Open

feat: per-user memory isolation via template scopes#568
eightHundreds wants to merge 6 commits intoCortexReach:masterfrom
eightHundreds:feat/per-user-scope-isolation

Conversation

@eightHundreds
Copy link
Copy Markdown

Summary

Implements per-user memory isolation (#555) to prevent cross-user memory bleed when multiple users DM the same bot (e.g., Feishu).

  • Add template variable support in scopes.default (user:${accountId}, ${channelId}, etc.) — resolved at hook layer, scope system stays purely static
  • Add wildcard pattern support in agentAccess (user:*) for explicit multi-user read access
  • Fix SQL wildcard filtering to match application-layer ACL semantics (_% pattern)
  • Narrow smart extractor dedup scope to [defaultScope] instead of accessibleScopes

Configuration Example

{
  "scopes": {
    "default": "user:${accountId}",
    "agentAccess": {
      "bot-1": ["global", "user:*"]
    }
  }
}

Each user's memories are written to their own scope (e.g., user:alice), while the bot can read across all user scopes via the user:* wildcard.

Key Design Decisions

  • Plan B architecture: scope system stays purely static, template resolution is a hook-layer concern (resolveHookDefaultScope)
  • No auto-wildcard: getAccessibleScopes does NOT auto-append wildcards — wildcards only via explicit agentAccess config
  • Backward compatible: static scopes.default works exactly as before, no breaking changes

Files Changed

  • src/scopes.ts — template/wildcard utilities, wildcard-aware isAccessible, safe getDefaultScope
  • index.tsresolveHookDefaultScope() for hook-layer template resolution
  • src/store.tsscopeFilterToSqlCondition (SQL LIKE with escaping), scopeFilterIncludes (app-layer wildcard matching)
  • test/scope-template-wildcard.test.mjs — 30 new tests

Test plan

  • All 59 tests pass (29 existing + 30 new)
  • Manual test with Feishu bot: verify user A's memories are isolated from user B
  • Verify backward compatibility: existing static scope configs work unchanged

🤖 Generated with Claude Code

eightHundreds and others added 3 commits April 9, 2026 11:04
Add support for template variables in `scopes.default` (e.g. `user:${accountId}`)
and wildcard patterns in `agentAccess` (e.g. `user:*`) to enable per-user memory
isolation when multiple users interact with the same bot.

Key changes:
- Template utilities: hasTemplateVars, resolveTemplateScope, matchesWildcardScope,
  inferWildcardFromTemplate in src/scopes.ts
- Hook-layer template resolution via resolveHookDefaultScope() in index.ts
- SQL wildcard support: scopeFilterToSqlCondition with proper LIKE escaping
- Application-layer wildcard matching: scopeFilterIncludes replaces includes()
- Smart extractor dedup narrowed to [defaultScope] instead of accessibleScopes
- 30 new tests covering template/wildcard utilities and integration scenarios

Design: scope system stays purely static (Plan B), template resolution is a
hook-layer concern. No auto-wildcard injection into accessible scopes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The project already has jiti as a devDependency. No need to bump
^2.6.0 to ^2.6.1 in this PR.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@eightHundreds eightHundreds force-pushed the feat/per-user-scope-isolation branch from 8852a81 to 911031c Compare April 9, 2026 03:05
@rwmjhb
Copy link
Copy Markdown
Collaborator

rwmjhb commented Apr 9, 2026

Cross-user memory bleed in shared bots is a real problem and per-user scope
isolation is the right approach. The design (template scopes + wildcard ACLs) makes
sense. A few things need to be fixed before this is safe to merge.

Must fix

  1. Test regression — scopeManager.validateScope is not a function.
    recall-text-cleanup.test.mjs (at least lines 740 and 760) crashes because
    resolveImplicitWriteScope now calls scopeManager.validateScope(...), but
    existing test fixtures provide a partial scopeManager mock without this method.
    The production path (where the plugin supplies a full MemoryScopeManager) is
    fine, but all partial mocks need validateScope added, or the call should guard
    with a capability check.

  2. System bypass agent throws when scopes.default is absent.
    resolveImplicitWriteScope falls back to
    scopeManager.getDefaultScope(agentId) for system-bypass IDs, but
    getDefaultScope throws when called with a system bypass ID. Before this PR,
    all three hook sites explicitly used config.scopes?.default ?? 'global' for
    bypass agents. The helper loses that safe fallback — it should replicate the same
    guard.

  3. vectorSearch() doesn't honor wildcard scopes.
    scopeFilterToSqlCondition and scopeFilterIncludes are wired into list, BM25,
    and delete paths, but vectorSearch() still builds SQL with scope = '...' and
    filters with scopeFilter.includes(rowScope). Semantic retrieval, dedup checks,
    and admission control all call vectorSearch, so wildcard ACLs like user:*
    silently don't work for the most important retrieval path.

Worth discussing

  • scopeFilter narrowing in smart extractor (index.ts:2811): the hook now
    passes scopeFilter: [defaultScope] instead of scopeFilter: accessibleScopes.
    This limits dedup to the write scope only. Was this intentional (to prevent
    cross-scope contamination) or a side effect? For agents with global + user:*
    access, this could hide duplicate memories that already exist in other scopes.

  • Migration footgun: with the new agentAccess: { 'bot-1': ['global', 'user:*'] }
    config, isAccessible('agent:bot-1', 'bot-1') returns false. Existing agents
    that relied on implicit self-scope access would stop seeing their own memories
    after upgrading. Worth documenting explicitly or adding agent:<id> automatically
    when explicit ACLs are set.

  • Wildcard validation is permissive: validateScope and setAgentAccess accept
    any *-suffixed string without checking whether the prefix is a known namespace.
    A typo like users:* would silently succeed. Low priority, but worth a note in
    docs.

Fix the three blockers and resolve the scopeFilter question, then this is ready for
another look.

Copy link
Copy Markdown
Collaborator

@AliceLJY AliceLJY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thorough review of all changed files. This is well-designed.

Architecture — Plan B (scope system stays static, template resolution at hook layer) is the right call. Clean separation between config-time scope validation and runtime context binding.

Security — checked:

  • SQL wildcard injection: escapeSqlLiteral + LIKE meta-character escaping for % and _
  • Overly broad wildcard patterns: inferWildcardFromTemplate correctly rejects compound templates like agent:${agentId}:user:${accountId} that would produce agent:*
  • Failed template resolution: returns undefined + skips write (not empty-string write to bogus scope) ✅

Backward compatibility — static scopes.default works exactly as before. All existing scope-filter codepaths upgraded to wildcard-aware variants (scopeFilterToSqlCondition, scopeFilterIncludes) with consistent semantics.

Key design decisions I agree with:

  • Dedup scope narrowed to [defaultScope] instead of accessibleScopes — prevents cross-user dedup matching in multi-tenant setups
  • No auto-wildcard in getAccessibleScopes — wildcards only via explicit agentAccess config
  • getDefaultScope() returns "global" for template defaults when no agentId — safe fallback

Tests — 30 unit tests + integration tests with mock plugin harness. Coverage looks adequate.

One note: manual Feishu bot test is still unchecked in the test plan. Would be good to confirm multi-user isolation works end-to-end before merge.

LGTM — approve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants