fix(scope): tenant-isolation batch — suppressions, env scope ceiling, forget override, session pinning, entity-link edges (#106–#112) - #113
Closed
devinmlowe wants to merge 8 commits into
Closed
devinmlowe wants to merge 8 commits into
devinmlowe wants to merge 8 commits into
Conversation
… instead of replacing it (#108) resolveCallScoping let a tool call's read_scopes replace ENGRAM_READ_SCOPES and its scope replace ENGRAM_SCOPE, so an env-pinned stdio child (the Hermes stdio transport) could read and write any tenant with one argument. When the env restricts reads, read_scopes is now intersected with it (empty → error) and a client scope must be one of the readable scopes. With no env restriction the params still apply as given, so the shared HTTP daemon path is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K88GYGYEE6wuuhewcsTbso
…oes not pin (#109) `scope: "global"` short-circuits assertScope and widens the query-mode candidate search to every tenant. A stdio child pinned by ENGRAM_SCOPE / ENGRAM_READ_SCOPES could therefore delete another tenant's memories with one argument. handleForget now drops the override when the process env restricts reads; an unpinned server (CLI, shared HTTP daemon) keeps it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K88GYGYEE6wuuhewcsTbso
forgetMemory stored the forgotten memory's scope but isSuppressed, clearSuppression, filterSuppressedFacts and restoreMemory all matched on the content hash alone, so one tenant's forget suppressed dream re-extraction of the same sentence for every tenant and any tenant's remember lifted it. New checkpointed migration suppression_scope_v1 rebuilds the table with a composite primary key (columns unchanged, so it stays additive for an older build). Lookups now match the caller's scope plus 'global' (global knowledge still suppresses everywhere); clearSuppression and restore delete only the exact scope's row. dream dropSuppressed passes the conversation's scope, remember/remember_batch the write scope. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K88GYGYEE6wuuhewcsTbso
…s scope (#111) remember_batch's relates_to_entities path inserted related_to edges without a scope column, so every edge took the 'global' default regardless of the write scope, and an existing edge was never widened when a second tenant supplied the same evidence. linkMemoryToEntities now takes the scope (passed from storeMemoryBatch), writes it on the INSERT and calls widenScope on the update branch — the same shape findOrCreateRelationship already uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K88GYGYEE6wuuhewcsTbso
createOrRefineRecallSession searched with the scopes of the current call, so a session opened with read_scopes and refined without them merged other tenants' results into the same session — which the caller then drills into. The scopes are now stored on the RecallSession at create and reused on every refine. Not fixed here: recall_drill still identifies a result by session id +index and re-checks nothing, because SearchResult metadata carries no scope today. Pinning closes the only path by which a foreign result can enter a session. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K88GYGYEE6wuuhewcsTbso
scopeVisible reads a NULL scope column as 'global', but the IN-clause helper did not — and `col IN (...)` is never true for NULL — so a row with a NULL scope was visible to the semantic readers and invisible to every scopeInClause reader (episodic recall, `engram memories list`, commitments) for any read_scopes, including ones containing 'global'. No in-tree producer of NULL scope exists today (every INSERT coalesces), so this closes a latent divergence rather than an observed leak. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K88GYGYEE6wuuhewcsTbso
ftsIndexedRowids listed the index through an fts5vocab 'instance' table, which has one row per term occurrence: a document whose content and context tokenize to nothing (punctuation or emoji only) appears nowhere in it. Such a row was therefore never reported by auditMemoryIndex and never repaired by `engram validate --fix`, so validate could report clean on a dirty index. The docsize shadow table holds one row per indexed document regardless of terms, and needs no temp table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K88GYGYEE6wuuhewcsTbso
…E.md Covers #106-#112: per-scope suppressions, the env scope ceiling, the forget global override, session scope pinning, scoped entity-link edges, the NULL scope coalesce and the FTS docsize enumeration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K88GYGYEE6wuuhewcsTbso
Owner
Author
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.
Scope-isolation bug hunt (phase 1) turned into seven fixes (phase 2). Every
finding was re-verified against current
mainbefore an issue was filed; thebridge finding from the same audit was already open as #87 and is untouched
here.
Issues fixed
memory_suppressionskeyed on(content_hash, scope)(new checkpointed migrationsuppression_scope_v1); suppression lookups take the caller's scope,clearSuppression/restoredelete only that scope's row, dreamdropSuppressedpasses the conversation's scoperesolveCallScopingintersects a call'sread_scopeswithENGRAM_READ_SCOPESand requires a call'sscopeto be readable — the env is a ceiling, not a defaultforget'sscope: "global"override is ignored when the process env pins the server to a tenantlinkMemoryToEntitiesstamps newrelated_toedges with the caller's scope and widens an existing edge seen from a second scopescopeInClausecoalesces a NULL scope column to'global', matchingscopeVisibleftsIndexedRowidsenumeratesmemories_fts_docsizeinstead of anfts5vocab('instance')scan, so zero-token documents are auditedDocs in 54f90f1 (CHANGELOG
[Unreleased], CLAUDE.md).Predicate log
npm run build && npm run test:runafter every commit. No revert was needed.Two pre-existing tests changed because they asserted the old behaviour, not
because the fix broke them:
tests/interfaces/mcp/per-request-scoping.test.tsasserted that a call'sread_scopescould widen pastENGRAM_SCOPE. That is exactly [mcp] resolveCallScoping lets client read_scopes / scope replace ENGRAM_READ_SCOPES / ENGRAM_SCOPE instead of intersecting — an env-pinned stdio child can read and write any tenant #108; it nowasserts the refusal.
tests/interfaces/cli/update.test.tshardcodedversion 4 -> 5; it nowderives the number from
SCHEMA_VERSION.Each new test was checked to fail without its fix (
#110and#112explicitly reverted and re-run).
Not fixed here
ENGRAM_SCOPE/ENGRAM_READ_SCOPES/ENGRAM_DB_PATHwhen it proxies to the daemon) — already open, separatechange, and the largest remaining hole: under the recommended deployment the
env ceiling added in [mcp] resolveCallScoping lets client read_scopes / scope replace ENGRAM_READ_SCOPES / ENGRAM_SCOPE instead of intersecting — an env-pinned stdio child can read and write any tenant #108 is never seen by the daemon that executes the call.
recall_drillscope re-check (part of [search] recall sessions do not pin read_scopes — a refine with different scopes merges other tenants' results into the session, and recall_drill re-checks nothing #110) — a drill still identifies aresult by session id + index and re-checks nothing, because
SearchResultmetadata carries no scope today. Pinning the session closes the only path by
which a foreign result can enter a session, so the remaining exposure needs a
guessed session UUID. Noted in the commit message and left for a follow-up
that adds scope to the result metadata.
NOT NULLon the four scope columns ([db] scopeInClause does not COALESCE NULL scope to 'global' the way scopeVisible does — a NULL-scope row is visible to one reader and hidden from the other #107) — the COALESCE makes the twohelpers agree; tightening the schema is a separate migration with no current
producer of NULL to justify it.
Draft — not for merge until #87 is decided, since the two changes overlap in
what they promise about stdio isolation.
🤖 Generated with Claude Code
https://claude.ai/code/session_01K88GYGYEE6wuuhewcsTbso