fix(hermes): isolate provider registrations by dplush · Pull Request #1008 · mnemosyne-oss/mnemosyne

dplush · 2026-09-19T22:37:12Z

Fixes #965

What changed

register_memory_provider() now creates one fresh provider per Hermes registration. register() captures that provider and binds Memory, Sync, and Persona handlers to the same instance, while preserving the first module-level legacy binding for older direct-handler callers.

Evidence

Base: ba33ec2d7d8910f32dbcfc2dec993670e2dd5dba
Head: 8bc4868810e3f517905de4017e135964077082bd
Focused regression matrix: 14 passed
Hermes integration suite: 491 passed, 1 skipped
Provider parity suite: 89 passed
Ruff fatal/changed-file checks, Python compile, and git diff --check: passed
Regression coverage includes distinct registrations, profile-A isolation after profile-B initialization, Memory/Sync/Persona binding, repeated/concurrent registration, and preserved fix(hermes-provider): share one initialized provider with plugin tools #793 single-register behavior.

Non-goals and compatibility boundary

No public tool schema changes, bank-policy changes, lifecycle API redesign, or legacy-provider removal. The module-level pointer remains only as a compatibility bridge; new registration surfaces do not reuse it. Rollback is the single commit revert.

Why not the alternative

Keeping the process-global singleton would preserve #793's original implementation but cannot support multiplexed profiles: later initialization rebinding can redirect existing sessions across profile banks. Duplicating the legacy provider implementation was rejected because this narrow fix only needs registration ownership and handler binding.

Summary

Hermes now creates one MnemosyneMemoryProvider per registration. Tool handlers remain bound to the provider that created them, so multiple agent profiles cannot redirect memory writes to the latest profile.

Architecture and privacy

Preserves isolation between Hermes profiles and their memory banks.
Prevents cross-profile writes caused by shared provider identity and BEAM state.
Keeps the module-level provider only for legacy direct-handler compatibility.
Does not change working, episodic, or BEAM memory tiers.
Does not change retrieval, consolidation, veracity, or sync behavior.
Preserves the local-first design. No remote storage or new data-sharing path is introduced.

Integration surfaces

Updates Hermes provider registration and handler binding.
No changes are reported for MCP or CLI integrations.
Public tool schemas and lifecycle APIs remain unchanged.
Single-registration behavior remains supported.

Maintainability and validation

Explicit provider binding makes registration ownership clear.
Repeated and concurrent registration tests cover isolation and handler stability.
Reported validation includes 14 focused regression tests, 491 Hermes integration tests with 1 skipped, and 89 provider parity tests.
Lint, compilation, and diff checks reportedly pass.
Review finding severity counts are unavailable from the supplied evidence.

coderabbitai · 2026-09-19T22:37:26Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: mnemosyne-oss/mnemosyne/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: f22ede1a-62cc-4dca-afda-b9f4a4b4425a

📥 Commits

Reviewing files that changed from the base of the PR and between ba33ec2 and 8bc4868.

📒 Files selected for processing (3)

integrations/hermes/src/mnemosyne_hermes/__init__.py
integrations/hermes/tests/test_persona_plugin_registration.py
integrations/hermes/tests/test_plugin_tool_initialization.py

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Walkthrough

Hermes now creates a separate MnemosyneMemoryProvider for each registration. Registered tool handlers remain bound to that provider. Legacy module-level provider initialization remains synchronized. Tests cover sequential, concurrent, and persona-handler behavior.

Changes

Hermes provider isolation

Layer / File(s)	Summary
Per-registration provider binding `integrations/hermes/src/mnemosyne_hermes/__init__.py`	`register_memory_provider()` creates and returns a new provider for each call. `register()` binds memory, sync, and persona handlers to that provider. Legacy module-level provider creation remains lock-protected.
Provider isolation validation `integrations/hermes/tests/test_plugin_tool_initialization.py`, `integrations/hermes/tests/test_persona_plugin_registration.py`	Tests verify handler binding, distinct sequential and concurrent providers, earlier-profile handler stability, and persona tool delegation.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 22.73% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: isolating Hermes provider registrations.
Linked Issues check	✅ Passed	Issue `#965` requires one provider instance per Hermes registration and isolation of profile-bound writes. The PR creates a fresh `MnemosyneMemoryProvider` in each `register_memory_provider()` call. `re…
Out of Scope Changes check	✅ Passed	The reported changes support issue `#965`. They update provider registration, handler binding, and regression tests for registration isolation. The summary reports no changes to public tool schemas, ban…

Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

…ration Reviewer ask (mnemosyne-oss#1050, dplush 2026-09-26): the call-time binding incident must be reproducible through the supported provider-registration path (the mnemosyne-oss#1008 entry the real gateway uses), not only through a harness with faked beams. Adds test_supported_path_a_b_a.py: one provider obtained via register_memory_provider(ctx), two homes initializing real file-backed stores, turns arriving A→B→A without re-init. Each write is verified at rest in its own home's sqlite, plus writer-provenance columns per home. Verified as a true regression detector: fails at the misfile assertion on pre-fix main (3e5967a) and passes at this branch head. Test-only; no shipped-code changes.

…ration Reviewer ask (mnemosyne-oss#1050, dplush 2026-09-26): the call-time binding incident must be reproducible through the supported provider-registration path (the mnemosyne-oss#1008 entry the real gateway uses), not only through a harness with faked beams. Adds test_supported_path_a_b_a.py: one provider obtained via register_memory_provider(ctx), two homes initializing real file-backed stores, turns arriving A→B→A without re-init. Each write is verified at rest in its own home's sqlite, plus writer-provenance columns per home. Verified as a true regression detector: fails at the misfile assertion on pre-fix main (3e5967a) and passes at this branch head. Test-only; no shipped-code changes. (cherry picked from commit 2845fea)

…ider-registration-isolation fix(hermes): isolate provider registrations

…t of 519f665, rebased on main) (#1050) * fix(hermes): call-time canonical bindings, writer provenance, self-attesting injects, guarded flush threads (cherry picked from commit 2389239) * fix(hermes): surface branch for _handle_invalidate (bank param + sf_ routing) A replacement-bearing invalidation previously could not land on a shared surface row: the handler was hard-wired to the private beam while _handle_validate already routes by bank. - bank param ('private' | 'surface'), validated, default private. - Bare 'sf_' ids auto-route to surface: the prefix is the generation-pinned id namespace minted by _handle_shared_remember; the private id space is disjoint bare hex. - Audit event and JSON reply record the resolved bank. - tools.py INVALIDATE_SCHEMA exposes bank with the same contract validate already uses, so agents can name the store explicitly. (cherry picked from commit fb76a25) * chore: bump mnemosyne-hermes 0.7.3->0.7.4, core 4.0.0b3->4.0.0b4; changelog entries for #1050 (cherry picked from commit 945ca10) * fix(hermes): keep auto-sleep worker target a zero-argument callable\n\nThe P1b in-turn context spawn wrapped the worker as Thread(target=ctx.run, args=(fn,)).\nHost fakes of threading.Thread invoke target() with no arguments (the parity contract\ncarries the same fake), so the run handle broke the daemon spawn. Copy the context\nonce and close over it. Behavior is unchanged. (cherry picked from commit 6e463bc) * fix(provider): port invalidate bank/sf_ routing to the packaged provider\n\ntest_provider_tool_schemas_match requires schema parity between the\nintegration provider and the packaged hermes_memory_provider. Port the\nsame invalidation branch (explicit bank param, sf_-prefix auto-routing,\nsurface-beam target, bank reported in audit event and response). (cherry picked from commit 3be6c86) * chore: sync version-floor surfaces to hermes 0.7.4 / core 4.0.0b4\n\nRelease-contract surfaces the bump commit missed: dependency floors in\nboth pyprojects, the pip_dependencies floor and plugin manifests\n(4.0.0b4), the catalog wrapper version, and regenerated docs. (cherry picked from commit 5b9cac3) * test(hermes): supported-path A→B→A regression through provider registration Reviewer ask (#1050, dplush 2026-09-26): the call-time binding incident must be reproducible through the supported provider-registration path (the #1008 entry the real gateway uses), not only through a harness with faked beams. Adds test_supported_path_a_b_a.py: one provider obtained via register_memory_provider(ctx), two homes initializing real file-backed stores, turns arriving A→B→A without re-init. Each write is verified at rest in its own home's sqlite, plus writer-provenance columns per home. Verified as a true regression detector: fails at the misfile assertion on pre-fix main (3e5967a) and passes at this branch head. Test-only; no shipped-code changes. (cherry picked from commit 2845fea) * test(hermes): pin the fake turn profile in canonical tool tests test_canonical_tools bound agent_identity explicitly (e.g. profile_a) but left the host-reported turn profile to whatever the interpreter exposed. On a bare interpreter hermes_cli is absent and the #1050 write guard is exempt; run inside a hermes venv, the real active profile clashes with the bound owner and the guard fails closed — three tests turned red on code that behaves exactly as designed. Align the turn profile with the identity each test binds, so the guard sees a concordant owner in every environment and the mismatch path stays covered by the dedicated test_canonical_write_guard_fails_closed_on_mismatch. Test-only. 6/6 in-file; full integrations suite 567 passed / 4 failed, the 4 being pre-existing environment-dependent test_init_retry failures that fail identically at the main baseline in the same environment. (cherry picked from commit 8378499) * ci: drop unused import in test_supported_path_a_b_a.py Test-only half of 64e7b18; the gate-file half left the shipped chain with the acceptance gate (host-side verification artifact, see body). (cherry picked from commit 64e7b18) * fix(hermes): fail closed for unknown in-turn homes; align init reads/writes Review on #1050 (dplush), points 1 and 2: - _binding_slot/_write_slot: a call inside a turn scope whose home has no binding no longer falls back to the last-initialized (ambient) home. Reads resolve to an empty detached binding (every entry point already treats beam-is-None as memory_unavailable / empty prefetch); writes raise instead of persisting into another home's store. Ambient fallback is reserved for genuinely out-of-turn callers, unchanged. - _ensure_initialized_for_tools / _maybe_retry_init: lazy and retry init bind to _hermes_home/env (the last initialized home), so an in-turn call from an uninitialized home used to initialize the WRONG home and hand its beam to the caller. Both paths now return without initializing when the turn home is unknown; the write fails closed. - _initialize_locked: _ambient_key mirrors the home under construction before the first binding write, so paired getters resolve to the same slot the setters target mid-init (B's bank path no longer inherits A's session/identity); the init routing key moved to __dict__['_init_home'] and is cleared in initialize()'s finally, so it cannot outlive its init (A session-switch updating B's stored slot — residue probe). - Tests: rewrite test_unknown_home_at_call_time_falls_back_to_ambient into a negative read+write regression (test_unknown_home_in_turn_fails_closed) plus test_out_of_turn_still_uses_ambient_binding; add two supported-path regressions for the mid-init skew and the post-init residue probe. Verified: 26-failure integration baseline unchanged (0 new failures); test collection clean (5112); diff-lint clean; canon_sign gate PASS. (cherry picked from commit 9ae0531) * fix(hermes): restore ambient binding after a dead failed init CodeRabbit point on 9ae0531: _initialize_locked mirrors the home under construction into _ambient_key BEFORE the Beam build; a construction failure left the ambient key on the dead slot and initialize() cleared only _init_home, so out-of-turn readers (cron prefetch, teardown flush) resolved to the failed home — a dead B init blacked out A's service. Restore the previous ambient when the attempt ends with no Beam in the target slot, with two carve-outs: - deliberate skip contexts keep the skip slot ambient (C13/C27: restoring a live beam would make system_prompt_block() report a stale 'Active' for a session that just went silent); - a pending transient retry keeps the dead ambient, because the retry surfaces fire precisely on _beam-is-None there. Also lands the two supporting review items from the same round: - the changelog entry promised an audit-sink warn-once that the code never implemented; the clause is removed rather than new behavior added at round four (the sink path stays debug-only); - the A->B->A provenance test now writes the returning-A row (a2) and checks every canonical row's writer stamp BY NAME, closing the hole where a2 could land in A wearing B's provenance under two green tests. Regression: test_failed_second_home_init_restores_ambient_reads fails at the ambient assertion on the pre-fix tree (1 failed, 1 passed) and passes here; the skip-context guard passes on both trees by design — it pins the carve-out, not the fix. (cherry picked from commit b9ab1b08f9bea90a10c5306fe120d5e2f7a011ec) * fix(canonical): compare writer provenance on import collisions; stamp task-progress writes Two CodeRabbit points on #1050 whose line anchors were marked outdated by the head moves but which remained live in the current code (verified against the source before conceding): - import_all's id-collision equality check zipped the stored-row snapshot against the pre-provenance 10-field tuple, so an import row repeating a stored row's bytes under a DIFFERENT writer_id was silently skipped as "identical". Content equality now includes writer_id — blank/absent normalizes to "imported" on BOTH sides so legacy pre-provenance exports keep round-tripping idempotently — while writer_home stays store-local and excluded, per the P3b ruling the INSERT path already implements. - The provider's _handle_task_progress called store.remember() without the writer arguments, storing every task:progress row with empty attribution fields. The same stamp derivation as _handle_remember_canonical now feeds the write. Regressions: test_import_collision_compares_writer_identity (with the idempotent control arm) and test_task_progress_write_carries_writer_stamp; both fail at the named assertions on the pre-fix tree and pass here. * ci(test): retarget catalog-plugin pins to the 0.7.4/4.0.0b4 floors CI's core-tests job caught what the round-by-round provider/canonical scoping missed: the version-floor sync commit (5b9cac3 lineage) moved the catalog manifest and wrapper dependency floors but not the three test constants pinning them — the PR head has been red in tests/ since, unrelated to any reviewer point. Reproduced locally (2 failed, 5 passed at this file), green after retargeting. * test(gate): canon_sign head-test v5 — A3 admits typed metadata.supplements out-edge (A5 vocabulary parity) Ruling on keeper's FAIL(1) red=[A3] residual. Store untouched; v5 reads 9/9 PASS at current state, negative control (typed edge stripped on RO copy) re-caught at A6a+A6b = FAIL(2). Stamp-close of sf_18287cec refused: it contradicts the row's recorded supersedes_nothing stance. Gate content: f1f5a9cd… (prior frozen v4 = 7ab0aa11, superseded by this commit, not rewritten). (cherry picked from commit 732f6307c462eb3d299af4a26e0e6a29842985b2) * test(gate): canon_sign head-test v6 — one read txn pins working_memory and conflicts to a single snapshot CodeRabbit review 5330458141 (2026-09-27), major at line 45, verified live against the shipped v5: the A0-A6 SELECT over working_memory and the A7 SELECT over conflicts ran as two independent implicit read transactions (Python's sqlite3 legacy control begins no txn for plain SELECTs), so a concurrent gateway commit between them could be scored as a state combination that never coexisted in the store. Fix is the reviewer's suggestion exactly: db.execute("BEGIN") before the first read. Deferred BEGIN takes the WAL snapshot at the first SELECT; the gate opens mode=ro and issues no DML, so there is no interaction with implicit transaction handling and nothing to commit at exit. Verified by running the gate directly (house law: python <file>, not pytest): - snapshot probe (WAL store, mode=ro reader, writer commit landing between the two reads): v5 pattern splits states (second read sees the mid-read write); v6 pattern pins one snapshot (second read does not; fresh post-run read proves the write was on disk, not lost). Passes on Python 3.10, 3.11, 3.12, 3.13, 3.14. - result-neutrality: v5 vs v6 byte-identical ARM/SUMMARY/RESULT output and exit code on a snapshot copy of the deployed shared store (with one post-review blob-content surface row, sf_08e42d1d..., decoded to text for the comparison; that row crashes both v5 and v6 on data typed before this fix and is a store-side matter, not a gate-code regression). - CI lint recipe (ruff 0.15.22: E9,F63,F7,F82 tree-wide selects and F,RUF022 over the changed file): clean. * test(gate): canon_sign head-test v7 — pinned snapshot reads get a fixture regression; execution moves under __main__ CodeRabbit comment 4119017434 (review 5334529495) on PR #1050: the v6 cross-table snapshot had no regression test. tests/test_canon_sign_gate_ snapshot.py now interleaves a committed conflicts update between the gate's working_memory and conflicts reads through the gate's own helpers, with a no-transaction control so dropping the read txn fails a test. Enabler, owed since head 32f879e: the gate matches pytest's *_test.py collection pattern and its module-level DB open crashed test(3.13) at collection; module-level execution moves under __main__ (inert bare import), the pinned connection and the two reads become named helpers, CLI stdout/exit byte-identical (verified on a store copy) — 3 tests pass on Python 3.10/3.11/3.12/3.13, ruff CI recipe clean. * test(gate): canon-sign snapshot fixture v8 — bare-import inertness asserted directly CodeRabbit nitpick on review 5338915149 (comment 4122373333): the inert-import test only checked absence of leftover db/rows attributes. Now sqlite3.connect is patched across the whole gate module exec and asserted uncalled — a reintroduced module-level open is caught on its real call path, not inferred from side effects. Existing attribute checks kept as the second witness. Test-file-only change; the gate (canon_sign_head_test.py) is untouched. Focused suite green on Python 3.10/3.11/3.12/3.13 (3 passed each); ruff 0.15.22 clean on both CI check sets. * fix(hermes): report a missing replacement distinctly from a missing target CodeRabbit review on #1050 (discussion_r4173890534): BeamMemory.invalidate returns one False for self-replacement, an invisible replacement, and an invisible target, and _handle_invalidate mapped all of them to memory_not_found. A caller whose target is fine but whose replacement id points at the other bank (or nowhere) had no way to learn which id to fix. - reject replacement_id == memory_id up front with a clear error; - after a failed invalidation with a replacement, probe the target through BeamMemory.get (same visibility predicate invalidate uses): a visible target now yields status replacement_not_found with both ids echoed; - missing targets keep memory_not_found; the no-replacement path is unchanged; the MCP server's deliberately opaque status is untouched. Tests: four provider-level cases in tests/test_hermes_memory_provider_shared_crud.py (visible target with bad replacement, missing target with valid replacement, self-replacement, cross-bank replacement then same-bank chaining). * test(hermes): invalidate via explicit surface selector on a bare id CodeRabbit nit on 280e2eb: every new surface case relied on sf_ prefix inference, so a broken bank=selector could hide behind it. Seed two ids in the surface DB that carry no sf_ prefix and show the selector wins: without it the id answers from the private bank (memory_not_found), with it the surface beam invalidates — both with and without a replacement chain. --------- Co-authored-by: SirOaken <siroaken@users.noreply.github.com> Co-authored-by: Astraea-Hermes <Astraea-Hermes@users.noreply.github.com> Co-authored-by: scales <scales@astraea.hermes> Co-authored-by: Abdias J <abdi.moya@gmail.com>

fix(hermes): isolate provider registrations

8bc4868

dplush requested a review from AxDSan as a code owner September 19, 2026 22:37

dplush merged commit c94106a into main Sep 19, 2026
9 checks passed

ether-btc pushed a commit to ether-btc/mnemosyne that referenced this pull request Oct 1, 2026

Merge pull request mnemosyne-oss#1008 from mnemosyne-oss/fix/965-prov…

af0b19a

…ider-registration-isolation fix(hermes): isolate provider registrations

tpowejlnglksdng mentioned this pull request Oct 13, 2026

Multiplexed gateway: plugin singleton adopts the last-initialized session's profile binding — silent cross-profile memory writes and canonical owner flips #1176

Closed

dplush mentioned this pull request Oct 16, 2026

[BUG] MnemosyneMemoryProvider binds home/bank at initialize-time only — cross-profile store bleed in multiplexed Hermes gateways (0.7.0) #1188

Closed

Navigation Menu

Repository navigation

fix(hermes): isolate provider registrations - #1008

fix(hermes): isolate provider registrations#1008
dplush merged 1 commit into
mainfrom
fix/965-provider-registration-isolation

dplush commented Sep 19, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Sep 19, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

dplush commented Sep 19, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Evidence

Non-goals and compatibility boundary

Why not the alternative

Summary

Architecture and privacy

Integration surfaces

Maintainability and validation

Uh oh!

coderabbitai Bot commented Sep 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

dplush commented Sep 19, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Sep 19, 2026 •

edited

Loading