feat: pluggable memory hook seam for entity backends and LLM egress#287
Open
illeatmyhat wants to merge 23 commits into
Open
feat: pluggable memory hook seam for entity backends and LLM egress#287illeatmyhat wants to merge 23 commits into
illeatmyhat wants to merge 23 commits into
Conversation
Define altk_evolve-owned hook types and frozen payloads, dispatched through the CPEX PluginManager when the optional cpex package is installed (new [hooks] extra). Without cpex — or with hooks.enabled=False, the default — every hook site is a fast no-op and behavior is unchanged. - Hook taxonomy: memory_pre_write, memory_pre_metadata_patch, memory_pre_delete, memory_pre_namespace_delete, memory_post_read, llm_pre_call (purpose-tagged, wired at every litellm completion site). - BaseEntityBackend restructured to template methods: public search_entities / delete_entity_by_id / delete_namespace / update_entity_metadata now own hook dispatch and delegate to protected _*_impl overrides, so backend overrides cannot bypass hooks. Internal reads (conflict-resolution pre-reads, metadata-patch read-before-merge) use _search_entities_impl and never fire memory_post_read. - Sync-async bridge: asyncio.run when no loop is running, dedicated thread (with contextvars propagation) when one is; fire-and-forget plugin tasks are awaited before the loop closes so side effects are never lost. - Halting plugins raise MemoryPolicyViolation — blocked writes are errors, never silent drops. - EvolveConfig.hooks: enabled (default False), plugins_yaml, code-first plugins list (programmatic PluginConfig synthesis), plugin_timeout; initialized by EvolveClient. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lugins - MetadataNormalizerPlugin (memory_pre_write, transform): copies task_id -> trace_id when only the former is present — the MCP server's save_trajectory writes task_id while Phoenix sync writes trace_id, and downstream cascade cleanup keys on trace_id, so MCP-saved sessions currently miss it. Also stamps created_at when absent. - AccessStampPlugin (memory_post_read, fire_and_forget): stamps last_accessed (ISO-8601 UTC) via the metadata-patch path; recursion-safe by construction (patching fires memory_pre_metadata_patch, whose base impl reads through the internal seam). - PIIFilterMemoryPlugin (memory_pre_write + llm_pre_call, transform): thin aliasing subclass of cpex-pii-filter's native plugin — CPEX discovers handlers by method name, so the alias exposes our custom hook names and delegates to the native tool_pre_invoke handler. Behind the new [pii] extra; ImportError-guarded stub otherwise. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- docs/guides/memory-hooks.md: motivation (compliance, normalization, access auditing, recall filtering), hook taxonomy table, cpex-optional design, singleton caveat, plugin-authoring walkthrough. - examples/hooks_plugins.yaml: commented CPEX config enabling all three shipped plugins (the edit-YAML-to-change-behavior property). - examples/hooks_demo.py: end-to-end filesystem-backend demo of normalizer stamping, access stamping, and PII redaction. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds an optional memory-hook seam to backend CRUD paths and LLM calls, with hook types, dispatch management, plugin implementations, client/config wiring, documentation, examples, and tests. ChangesMemory Hooks Feature
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
altk_evolve/hooks/plugins/normalizer.py (1)
55-55: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSame
datetime.UTCcompatibility concern as inaccess_stamp.py.
datetime.datetime.now(datetime.UTC)requires Python 3.11+; the repo's declared minimum is 3.9+, per the earlier learning. This will raiseAttributeErroron 3.9/3.10 runtimes.🐛 Proposed fix
- metadata["created_at"] = datetime.datetime.now(datetime.UTC).isoformat() + metadata["created_at"] = datetime.datetime.now(datetime.timezone.utc).isoformat()[dependency_check]
Based on learnings, "assume the minimum supported Python version is 3.9+" for this repo;
datetime.UTCis unavailable before 3.11.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@altk_evolve/hooks/plugins/normalizer.py` at line 55, The `Normalizer` metadata timestamp assignment uses `datetime.datetime.now(datetime.UTC)`, which is not compatible with the repo’s 3.9+ Python support. Update the `Normalizer` logic to avoid `datetime.UTC` and use a timezone source that works on 3.9/3.10 while keeping the ISO-formatted `created_at` value unchanged. Make the same compatibility-safe change wherever this pattern appears in the plugin code so the timestamp generation works across all supported runtimes.Source: Learnings
🧹 Nitpick comments (6)
altk_evolve/config/hooks.py (1)
16-18: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate
kindcontains a module path before it reaches_register_spec.
HookPluginSpec.kindhas no format validation. If a caller passes a dotted-path-less string,manager._register_spec'sspec.kind.rpartition(".")yields an emptymodule_path, andimportlib.import_module("")fails with an unhelpful error far from the misconfiguration site.🛡️ Suggested validator
+from pydantic import field_validator + class HookPluginSpec(BaseModel): name: str = Field(description="Unique plugin name.") kind: str = Field(description="Dotted import path of the plugin class.") + + `@field_validator`("kind") + `@classmethod` + def _kind_must_be_dotted_path(cls, v: str) -> str: + if "." not in v: + raise ValueError(f"kind must be a dotted import path (module.ClassName), got: {v!r}") + return v🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@altk_evolve/config/hooks.py` around lines 16 - 18, Add format validation for HookPluginSpec.kind so it must include a module path before manager._register_spec is called. Update the HookPluginSpec model in hooks.py to validate kind (for example with a field validator) and reject dotted-path-less strings early with a clear message, since manager._register_spec relies on kind.rpartition(".") and importlib.import_module. Keep the fix focused on HookPluginSpec and the registration flow in _register_spec so invalid plugin specs fail at the source.altk_evolve/hooks/plugins/pii.py (1)
18-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBroad
except ImportErrormay mask unrelated import failures.If
cpex_pii_filter.pii_filterexists but raisesImportErrorfor an unrelated internal reason (e.g., a transitive dependency issue), this silently falls back to the stub with a misleading "install altk-evolve[pii]" message instead of surfacing the real error. Low priority given this mirrors the existingHAS_CPEXpattern elsewhere in the seam.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@altk_evolve/hooks/plugins/pii.py` around lines 18 - 25, The optional-import block in the PII plugin setup is catching all ImportError cases, which can hide real failures from cpex_pii_filter.pii_filter. Update the import guard around _PIIFilterPlugin in the pii.py plugin module to distinguish “module not installed” from other import-time errors, so only the missing-dependency case sets _HAS_PII_FILTER to False while unexpected ImportError details are surfaced instead of silently falling back.altk_evolve/hooks/plugins/access_stamp.py (1)
44-59: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBlocking sync writes inside an awaited fire-and-forget hook amplify read latency.
memory_post_readcalls the synchronousbackend.update_entity_metadata(...)once per returned entity, sequentially, inside anasync def. Perdocs/guides/memory-hooks.md(Line 22), fire-and-forget tasks are awaited before the sync bridge returns, so everysearch_entitiescall now blocks on N sequential synchronous metadata writes before returning to the caller — turning a read into a write-amplified, latency-sensitive operation, and blocking whatever thread runs the coroutine for I/O-bound backends (filesystem/postgres/milvus).♻️ Suggested mitigation
- for entity in payload.entities: - entity_id = entity.get("id") - if not entity_id: - continue - try: - backend.update_entity_metadata(payload.namespace_id, str(entity_id), {"last_accessed": stamp}) - except Exception: - logger.debug("AccessStampPlugin: failed to stamp entity %s.", entity_id, exc_info=True) + import asyncio + + async def _stamp(entity_id: str) -> None: + try: + await asyncio.to_thread( + backend.update_entity_metadata, payload.namespace_id, entity_id, {"last_accessed": stamp} + ) + except Exception: + logger.debug("AccessStampPlugin: failed to stamp entity %s.", entity_id, exc_info=True) + + await asyncio.gather( + *(_stamp(str(e["id"])) for e in payload.entities if e.get("id")) + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@altk_evolve/hooks/plugins/access_stamp.py` around lines 44 - 59, The memory_post_read hook in AccessStampPlugin is doing synchronous per-entity backend.update_entity_metadata writes inside an async hook, which makes reads wait on sequential metadata updates. Move the stamping work out of the read path by batching or deferring it as a fire-and-forget background task, or offload the sync backend call so memory_post_read can return immediately after scheduling the updates. Keep the behavior centered around memory_post_read and backend.update_entity_metadata, but avoid blocking the coroutine on each entity stamp.pyproject.toml (1)
46-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider pinning an upper bound for
cpex-pii-filter.
hookspinscpex>=0.1.0,<0.2, butpiileavescpex-pii-filter>=0.3unbounded. For consistency and to avoid unexpected breakage from a future major/minor release, consider adding an upper bound similar tocpex.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` around lines 46 - 51, The pii dependency entry is unbounded while the related hooks dependency is version-pinned, so update the pii extra to include an upper bound for cpex-pii-filter as well. Edit the dependency list in the pii section to constrain cpex-pii-filter similarly to how cpex is constrained, keeping the version range explicit and consistent with the other plugin dependencies.tests/unit/test_hooks_seam.py (2)
438-457: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOnly one of five wired LLM call sites has a dispatch integration test.
test_llm_pre_call_fires_at_fact_extraction_call_siteverifies fact_extraction, but conflict_resolution, clustering (combine_cluster), guidelines (_generate_guidelines_for_segment), and segmentation (segment_trajectory) have no equivalent test confirmingdispatch_llm_pre_callactually reaches theircompletion()calls. A future refactor could silently drop the dispatch call at one of those sites without failing any test.As per path instructions,
tests/**/*.py: "All new features need tests (unit + e2e where applicable)."✅ Example additional test pattern (repeat per call site)
`@pytest.mark.unit` def test_llm_pre_call_fires_at_clustering_call_site(): from altk_evolve.llm.guidelines import clustering enable_hooks(MessageTagger()) response = Mock() response.choices = [Mock(message=Mock(content=json.dumps({"guidelines": []})))] with patch.object(clustering, "completion", return_value=response) as mock_completion: clustering.combine_cluster([...]) sent = mock_completion.call_args.kwargs["messages"] assert sent[0]["content"].startswith("[tagged:guideline_combination] ")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_hooks_seam.py` around lines 438 - 457, Add missing unit coverage for the other hooked LLM call sites so a regression in dispatch_llm_pre_call is caught everywhere, not just in fact_extraction. Mirror test_llm_pre_call_fires_at_fact_extraction_call_site for conflict_resolution, clustering.combine_cluster, guidelines._generate_guidelines_for_segment, and segmentation.segment_trajectory by patching each module’s completion call and asserting the sent messages are tagged after enable_hooks(MessageTagger()).Source: Path instructions
413-432: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd
update_entitiesto the no-bypass assertion list.
update_entitiesis the method that dispatchesmemory_pre_write(peraltk_evolve/backend/base.py), yet it's excluded fromtemplate_methods. This test is specifically designed to guard against subclasses reintroducing a hook bypass — omitting the write-path entry point leaves that guarantee partially unverified.🛡️ Proposed fix
- template_methods = ("search_entities", "delete_entity_by_id", "delete_namespace", "update_entity_metadata") + template_methods = ("update_entities", "search_entities", "delete_entity_by_id", "delete_namespace", "update_entity_metadata")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_hooks_seam.py` around lines 413 - 432, The hook-bypass guard in test_backends_do_not_override_public_template_methods is missing update_entities, so the write-path template method is not being checked. Add update_entities to template_methods in this test so backend classes like FilesystemEntityBackend, MilvusEntityBackend, and PostgresEntityBackend are also asserted not to override it via vars(backend_cls).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@altk_evolve/backend/base.py`:
- Around line 179-182: `dispatch_memory_pre_write` can fail after
`FilesystemEntityBackend` has already switched `_active_data`, leaving stale
namespace state active. Update the `FilesystemEntityBackend` flow around the
`dispatch_memory_pre_write` call to catch hook failures and reset/clear the
active filesystem state before re-raising, so later reads do not keep using the
wrong namespace. Use the `FilesystemEntityBackend`, `_active_data`, and
`_post_update()` code paths to place the cleanup in the same template method
that currently performs the pre-write dispatch.
In `@altk_evolve/hooks/manager.py`:
- Around line 223-266: The write-family hook dispatchers lack the same
re-entrancy protection that `dispatch_memory_post_read` uses via
`_in_post_read`, so recursive plugin callbacks can loop indefinitely. Add a
matching contextvar-based guard for the write path and apply it in
`dispatch_memory_pre_write`, `dispatch_memory_pre_metadata_patch`,
`dispatch_memory_pre_delete`, and `dispatch_memory_pre_namespace_delete` before
calling `_invoke`. Follow the existing guard pattern from
`dispatch_memory_post_read` so these handlers short-circuit when already inside
the same hook family.
- Around line 84-110: initialize_hooks currently returns immediately when
config.enabled is false, which leaves the process-wide _plugin_manager and
_plugins_enabled state from a previous setup intact. Update initialize_hooks to
clear the existing hook state on the disabled path by calling shutdown_hooks()
before returning None, or otherwise ensure _plugin_manager and _plugins_enabled
are reset there; use the existing initialize_hooks and shutdown_hooks symbols to
keep the one-active-config-per-process behavior consistent.
- Around line 140-155: The code-first plugins are being registered too late,
after the manager has already run initialization, so their initialize() hooks
are skipped. In the hooks manager flow, move the _register_spec loop ahead of
pm.initialize() so plugins are added to the registry before CPEX initializes
them. Use the existing _register_spec helper and pm.initialize() call as the key
points to reorder, keeping the registration logic unchanged otherwise.
---
Duplicate comments:
In `@altk_evolve/hooks/plugins/normalizer.py`:
- Line 55: The `Normalizer` metadata timestamp assignment uses
`datetime.datetime.now(datetime.UTC)`, which is not compatible with the repo’s
3.9+ Python support. Update the `Normalizer` logic to avoid `datetime.UTC` and
use a timezone source that works on 3.9/3.10 while keeping the ISO-formatted
`created_at` value unchanged. Make the same compatibility-safe change wherever
this pattern appears in the plugin code so the timestamp generation works across
all supported runtimes.
---
Nitpick comments:
In `@altk_evolve/config/hooks.py`:
- Around line 16-18: Add format validation for HookPluginSpec.kind so it must
include a module path before manager._register_spec is called. Update the
HookPluginSpec model in hooks.py to validate kind (for example with a field
validator) and reject dotted-path-less strings early with a clear message, since
manager._register_spec relies on kind.rpartition(".") and
importlib.import_module. Keep the fix focused on HookPluginSpec and the
registration flow in _register_spec so invalid plugin specs fail at the source.
In `@altk_evolve/hooks/plugins/access_stamp.py`:
- Around line 44-59: The memory_post_read hook in AccessStampPlugin is doing
synchronous per-entity backend.update_entity_metadata writes inside an async
hook, which makes reads wait on sequential metadata updates. Move the stamping
work out of the read path by batching or deferring it as a fire-and-forget
background task, or offload the sync backend call so memory_post_read can return
immediately after scheduling the updates. Keep the behavior centered around
memory_post_read and backend.update_entity_metadata, but avoid blocking the
coroutine on each entity stamp.
In `@altk_evolve/hooks/plugins/pii.py`:
- Around line 18-25: The optional-import block in the PII plugin setup is
catching all ImportError cases, which can hide real failures from
cpex_pii_filter.pii_filter. Update the import guard around _PIIFilterPlugin in
the pii.py plugin module to distinguish “module not installed” from other
import-time errors, so only the missing-dependency case sets _HAS_PII_FILTER to
False while unexpected ImportError details are surfaced instead of silently
falling back.
In `@pyproject.toml`:
- Around line 46-51: The pii dependency entry is unbounded while the related
hooks dependency is version-pinned, so update the pii extra to include an upper
bound for cpex-pii-filter as well. Edit the dependency list in the pii section
to constrain cpex-pii-filter similarly to how cpex is constrained, keeping the
version range explicit and consistent with the other plugin dependencies.
In `@tests/unit/test_hooks_seam.py`:
- Around line 438-457: Add missing unit coverage for the other hooked LLM call
sites so a regression in dispatch_llm_pre_call is caught everywhere, not just in
fact_extraction. Mirror test_llm_pre_call_fires_at_fact_extraction_call_site for
conflict_resolution, clustering.combine_cluster,
guidelines._generate_guidelines_for_segment, and segmentation.segment_trajectory
by patching each module’s completion call and asserting the sent messages are
tagged after enable_hooks(MessageTagger()).
- Around line 413-432: The hook-bypass guard in
test_backends_do_not_override_public_template_methods is missing
update_entities, so the write-path template method is not being checked. Add
update_entities to template_methods in this test so backend classes like
FilesystemEntityBackend, MilvusEntityBackend, and PostgresEntityBackend are also
asserted not to override it via vars(backend_cls).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a1e7fff-1624-49e9-a194-22c9b8671b1d
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
altk_evolve/backend/base.pyaltk_evolve/backend/filesystem.pyaltk_evolve/backend/milvus.pyaltk_evolve/backend/postgres.pyaltk_evolve/config/evolve.pyaltk_evolve/config/hooks.pyaltk_evolve/frontend/client/evolve_client.pyaltk_evolve/hooks/__init__.pyaltk_evolve/hooks/manager.pyaltk_evolve/hooks/plugins/__init__.pyaltk_evolve/hooks/plugins/access_stamp.pyaltk_evolve/hooks/plugins/normalizer.pyaltk_evolve/hooks/plugins/pii.pyaltk_evolve/hooks/types.pyaltk_evolve/llm/conflict_resolution/conflict_resolution.pyaltk_evolve/llm/fact_extraction/fact_extraction.pyaltk_evolve/llm/guidelines/clustering.pyaltk_evolve/llm/guidelines/guidelines.pyaltk_evolve/llm/guidelines/segmentation.pydocs/guides/memory-hooks.mdexamples/hooks_demo.pyexamples/hooks_plugins.yamlpyproject.tomltests/unit/test_hooks_noop.pytests/unit/test_hooks_plugins.pytests/unit/test_hooks_seam.pytests/unit/test_milvus_backend.pytests/unit/test_postgres_backend.py
pydantic frozen=True only guards attribute assignment — nested entity dicts, metadata patches, and message lists were shared mutable references, so a plugin mutating payload contents in place (without returning modified_payload) had its mutation persisted. Two-part enforcement in the dispatch helpers, applied only after the hooks_active()/has_hooks_for guards so the disabled/no-subscriber fast path pays for no copies: - deep-copy mutable payload contents (entities, metadata_patch, messages) at payload construction, isolating the caller's objects; - _invoke now returns None when no plugin returned a modified_payload, and dispatchers fall back to the caller's original input — changes flow back to the store only via PluginResult.modified_payload. Includes an adversarial regression pair (InPlaceMutator): in-place mutation without modified_payload does not reach the store; the same change returned via modified_payload applies. Module docstring also gains the process-global singleton warning. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tamp read cost Three review findings, docs only — no behavior change: - Internal-delete wording: conflict-resolution DELETE verdicts do not fire memory_pre_delete, and memory_pre_write sees the incoming batch, not the stored entities those verdicts target — so pre-delete subscribers (e.g. legal-hold plugins) cannot veto LLM-initiated deletions. Stated plainly as a known limitation (slated for the future lifecycle/policy hook family) in base.py and the guide, replacing the misleading "covered by memory_pre_write" claim. - Singleton warning: a second client with hooks.enabled=True resets the manager and silently replaces the first client's plugins (PII redaction disabled by unrelated code); enabled=False clients still inherit process-global hooks. Warned in evolve_client.py and the guide's singleton caveat. - AccessStampPlugin read cost: fire-and-forget tasks are awaited at the sync seam, so every public read pays one metadata write per returned entity (~3.7ms vs ~0.1ms for a 10-entity filesystem read; N extra round trips on milvus/postgres). Documented in the plugin docstring, examples/hooks_plugins.yaml, and the guide. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
illeatmyhat
changed the title
Close the documented LLM-delete gap: DELETE verdicts from conflict resolution inside update_entities bypassed memory_pre_delete, so policy plugins (e.g. legal hold) could not veto LLM-initiated deletions. - Single guarded delete path: BaseEntityBackend._guarded_delete fires memory_pre_delete then dispatches to the backend delete impl (_delete_entity_by_id_impl for the public API, _delete_entity for conflict-resolution verdicts). Both callers route through it, so no entity delete through the backend abstraction can skip the hook. - Caller-dependent veto semantics: delete_entity_by_id still propagates MemoryPolicyViolation; the verdict executor catches it per verdict, logs a warning, records the skip on the returned EntityUpdate (event=NONE + skipped_delete metadata), and continues the batch — a legal hold must not abort the whole write. - MemoryPreDeletePayload gains optional metadata (deep-copied): the stored entity's metadata from the conflict-resolution pre-read, or an internal _search_entities_impl fetch on the public path (only when a memory_pre_delete subscriber exists — the disabled path stays zero-overhead; entity not found -> metadata=None, delete proceeds). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The known-limitation paragraph is obsolete: conflict-resolution DELETE verdicts now fire memory_pre_delete through the single guarded delete path. Document the unified semantics (one path, per-caller veto behavior, metadata in the payload) and update the payload table. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The normalizer and access-stamp plugins had their entire logic — including pure domain code — defined under `if HAS_CPEX:`, so without the [hooks] extra it was unimportable and untestable, and every future plugin would inherit that template. Extract engine-agnostic cores at module top level (no cpex imports, plain data in/out, injectable clock): `normalize_entities` (returns None when unchanged) and `build_access_stamps` (returns (id, patch) pairs; the backend side effect stays in the shim, which needs the live backend from GlobalContext.state). The cpex Plugin subclasses shrink to thin shims: parse config, call core, wrap in PluginResult. Names, kinds, defaults, priorities, and YAML compatibility unchanged. pii.py stays core-less by design — adapting cpex-pii-filter IS its domain logic (docstring now says so). New tests/unit/test_hooks_plugin_cores.py (17 tests, no importorskip) is the always-on CI coverage for the domain logic, including a subprocess check that the cores import and work with all cpex imports blocked and the stubs still raise naming the [hooks] extra. Docs teach the core/shim pattern for new plugins. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
illeatmyhat
changed the title
…ngine The hook seam (types, payloads, dispatch points, veto semantics) and plugin cores are engine-agnostic; CPEX is the execution engine we ship, not the foundation the system is built on. Restructure the guide so the seam concept leads cpex-free, all CPEX mechanics live in a scoped 'CPEX engine' section, and adjust module/config docstring framing. No behavior changes, no renames. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nexport HAS_CPEX Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jayaramkr
requested changes
Jul 14, 2026
jayaramkr
left a comment
jayaramkr
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed the full diff and executed the code to verify every finding below. Baseline is green: ruff clean, mypy clean, and 629 passed / 1 skipped with --all-extras (the delta to your 651 is the milvus tests, which need pymilvus locally — your count checks out).
The architecture here is genuinely good, and some of the hardest parts are right — see "What checks out" at the bottom, because I want to be clear you don't need to churn on them.
But the security posture is inverted. This seam is sold as a compliance boundary for PII redaction, and as shipped:
- It fails open —
on_errordefaults toignore, so a redactor that crashes or times out lets raw PII through to the store and to the LLM, with no exception and no signal. - The PII plugin cannot block — it registers
mode=TRANSFORM, and CPEX silently downgradescontinue_processing=FalsetoTruein that mode, making its block path dead code. - There is a redaction bypass on a public MCP path —
update_entity_metadatareturns full entity content without firingmemory_post_read, andpublish_entity/unpublish_entityecho it straight back to the caller. - Payload immutability isn't airtight — a plugin's discarded in-place mutation can still reach the store by riding in on a later plugin's shallow
model_copy.
The net effect of 1 and 2 together: a user can configure PII redaction exactly as your own docs and example YAML describe, and get no protection at all. Those two are mostly a matter of flipping defaults, but they have to be fixed together.
Details inline.
Three things that need raising but have no diff anchor:
No pytest job gates pull requests. .github/workflows/check-code.yaml (the pull_request workflow) runs lockfile, ruff check, ruff format, mypy, plugins-rendered, and npm UI tests — there is no pytest job. Python tests run only via .github/actions/run-tests, which is invoked from release-github.yaml under workflow_dispatch: (a manual release). So the 55 new hook tests — and the whole existing suite — gate nothing on a PR. This is a pre-existing repo condition rather than something this PR introduced, but it's the reason the issues above could land green, and it's worth fixing alongside a feature that leans on "651 passed" as its evidence. (Credit where due: when tests do run, run-tests does uv sync --all-extras, so cpex is installed — the extra isn't the gap; the trigger is.)
consolidate_guidelines makes read-transforms destructive. evolve_client.py:160 → 189 → 212 → 229 fetches the cluster via the public search_entities (so memory_post_read transforms apply), feeds it to the LLM, writes the result back as new entities, and deletes the originals. With any redaction or field-stripping plugin on memory_post_read, consolidation permanently replaces stored content with the redacted version. Not a leak, but silent and irreversible.
Two small hardening items: base.py:164 patch_entity is a public, unhooked content write (only reached internally today, but it sits on the public backend surface — suggest _patch_entity). And the "a backend override cannot skip a hook" guarantee is convention-only — Python has no final, and FilesystemEntityBackend already overrides one template method (update_entities; it does call super(), but it shows the pattern isn't enforced). An __init_subclass__ guard that raises if a subclass defines any of the four public template method names would make the claim actually true.
What checks out — don't churn on these:
- The sync bridge is solid. I expected this to be the bug farm and it isn't. Verified across four contexts — no running loop, inside a running
asyncio.runloop, a plain worker thread, and 16 concurrent threads: no deadlock, no reentrancy bug, no lost exceptions,MemoryPolicyViolationpropagates correctly, and fire-and-forget tasks are genuinely awaited before the loop closes. - The recursion guard is correct — a real
contextvars.ContextVar, set/reset in try/finally, propagated into the bridge thread viacopy_context(). - LLM egress coverage is exhaustive. All 8
completion()call sites underaltk_evolve/llm/are preceded bydispatch_llm_pre_call, and each dispatch is hoisted outside the retry loop so retries reuse the redacted messages. No egress gap. - Template-method routing holds. All three backends implement the
_*_implvariants and none overrides the four public template methods; every external surface (MCP, REST, CLI, Phoenix sync) reaches storage only through them. Unified delete via_guarded_deleteis correct, and the conflict-resolution veto semantics (event="NONE"+skipped_delete, rest of batch applies) work as documented. - "Zero default behavior change" holds, and the ImportError contract is a clear, actionable error rather than a silent no-op.
pii.py's adaptation logic is sound — it recurses into nested entity dicts, redacts metadata as well as content, and handlescontent=Noneand list-of-content-block messages without raising. The problem is its mode and on_error, not its redaction.
Flip on_error default from "ignore" to "fail" (fail-closed) in HookPluginSpec, the three shipped plugins' _default_config(), and examples/hooks_plugins.yaml: a compliance plugin that crashes/times out must halt the operation, not silently pass data through. Catch escaping cpex PluginError/PluginTimeoutError/PluginViolationError in _invoke and re-raise as MemoryPolicyViolation (preserving __cause__ + plugin name/violation info) so the "halting raises MemoryPolicyViolation" contract holds for crashes/timeouts too. dispatch_memory_post_read swallows the violation (logs) so a read-side plugin crash never fails the read. shutdown_hooks now calls executor.reset_runtime_disabled() so a transient on_error="disable" trip does not wedge a plugin out across a reinit. Change the PII plugin's mode from TRANSFORM to SEQUENTIAL (plugin default + YAML): CPEX silently downgrades continue_processing=False->True in TRANSFORM/AUDIT, so a redactor in transform mode can redact but never block; sequential preserves both redaction-chaining and the ability to halt on unredactable PII. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ontract update_entity_metadata's template method now runs the returned RecordedEntity (which carries full content and is echoed to callers like MCP publish/ unpublish) through memory_post_read, so a redacting post_read plugin can no longer be bypassed via the metadata-patch return. Covers filesystem, postgres (RETURNING) and milvus (query) since all three flow through the base template; the internal read-before-merge still uses _search_entities_impl (no post_read) and the _in_post_read guard stops access-stamp recursion. Add a regression test locking in that BOTH shipped write plugins communicate changes via modified_payload and never mutate in place, and document the immutability contract in the guide (a "deep-copy the returned payload" fix does not help — the return channel is the mechanism). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… guard + filesystem state hygiene FilesystemEntityBackend now uses an RLock and, on the metadata-write path (patch_entity), reuses the in-flight _active_data when set instead of doing its own load/save — so a memory_pre_write plugin calling back into update_entity_metadata neither self-deadlocks nor clobbers the outer op's in-flight state; the callback's change is persisted by the outer _post_update. update_entities wraps super() in try/finally so a halted write clears _active_data instead of leaking stale state to the next op (finding 11). Add a shared _in_write contextvar guard across the four write dispatchers (pre_write, pre_metadata_patch, pre_delete, pre_namespace_delete), propagated into the sync-bridge thread, so a write-hook plugin re-entering any write API skips re-dispatch instead of recursing. Tests cover no-deadlock+persistence and the guard suppressing re-dispatch while the write still lands. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rvives
resolve_conflicts previously re-attached metadata only for ADD verdicts;
UPDATE verdicts left metadata={}, and base._update_entity does a wholesale
metadata replace, so plugin-written metadata (normalizer trace_id/created_at,
access-stamp last_accessed) was destroyed on the conflict-resolution path
(the production path for guideline writes).
UPDATE verdicts carry the OLD stored id, not a temp id, so map incoming
entities by serialized content to trace an UPDATE back to its source, then
merge the incoming entity's metadata (which already passed through
memory_pre_write) OVER the stored entity's existing metadata (from the internal
pre-read) — preserving stored-only stamps like last_accessed while applying the
newly normalized values. Falls back to the stored metadata when the source
can't be identified. ADD behavior unchanged.
Update the existing CR test that codified the metadata-wipe, and add a
CR=True integration test with normalizer+access_stamp asserting all three
stamps survive the UPDATE.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Warn loudly when re-initializing hooks would discard an earlier client's already-registered plugins (a silent reset can drop a compliance plugin like PII redaction). Shut the seam down on the disabled path so a client built with hooks.enabled=False no longer inherits another client's process-global plugins; EvolveClient now calls initialize_hooks unconditionally so disabling truly disables. Defer the heavy cpex.framework import (~400ms, pulls fastapi/mcp/ prometheus) out of module load: HAS_CPEX is now a cheap find_spec probe, payload classes stay plain frozen pydantic models at import and gain a cpex-backed PluginPayload twin (via multiple inheritance) only when the engine initializes (active_payload_cls / bind_engine_payloads). Importing any backend no longer pulls cpex.framework when hooks are disabled. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ity rename - config/hooks.py: field_validator on HookPluginSpec.kind rejects non-dotted paths up front with a clear message (a bare name would otherwise ImportError deep inside _register_spec). - hooks/plugins/pii.py: narrow the broad `except ImportError` to fall back to the stub only when the MISSING module is cpex/cpex_pii_filter; an unrelated broken transitive now propagates instead of being masked. - pyproject.toml: bound cpex-pii-filter>=0.3,<0.4 (mirrors cpex <0.2) with rationale — the config fallback is unreachable on the YAML path, so a 0.4 defaults change would silently alter redaction. - backend/base.py + filesystem.py: rename the unhooked public patch_entity -> _patch_entity (only internal callers exist). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- test_hooks_seam.py: add a write-path no-bypass test — FilesystemEntity Backend legitimately overrides update_entities, so instead of the "must not override" list, assert the override delegates to BaseEntityBackend.update_entities exactly once (memory_pre_write fires once through it). - Add dispatch_llm_pre_call call-site tests for the four previously untested sites: guideline generation, guideline combination, segmentation, and conflict resolution (mirrors the fact_extraction test — patch each module's completion, assert the purpose tag). - check-code.yaml: add a run-tests job (reusing the run-tests composite action, which installs all extras) so the cpex-guarded hook tests gate PRs — previously no pytest ran on pull_request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up hardening for the deferred-import (group 5) and narrowed PII import (group 6) changes, surfaced by test_hooks_plugin_cores: - types.py: the find_spec cpex probe must not propagate — a hostile/ blocking meta-path finder (or an edge-case ModuleNotFoundError/ ValueError) raises instead of returning None; treat any failure to locate cpex as "not installed". - pii.py: fall back to the stub when the failing import carries no module name (a blocked cpex import chain can raise a name-less ImportError); a genuine broken transitive always names its offending module, so those still propagate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- docs/guides/memory-hooks.md: add a "Known limitations" section noting delete_namespace fires only memory_pre_namespace_delete, never a per-entity memory_pre_delete (so a legal-hold plugin must subscribe to the namespace hook); document the transform-can't-block trap (halting needs sequential mode) and the fail-closed on_error default and its availability tradeoff. Fix the MEMORY_PRE_DELETE docstring in hooks/types.py to state the same namespace-delete carve-out. - evolve_client.consolidate_guidelines: route the READ through the internal seam (_search_entities_impl) instead of public search_entities. Consolidation writes fetched content back and deletes the originals, so a redacting memory_post_read plugin on the public read would make consolidation permanently persist the redacted view. The internal read round-trips original stored content; LLM egress stays covered by llm_pre_call and the write-back/deletes still fire their hooks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
altk_evolve/backend/filesystem.py (1)
230-246: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the in-flight filesystem transaction namespace- and nesting-aware.
RLockpermits hook callbacks, but the single_active_dataslot cannot safely represent nested operations. Callbacks can corrupt the wrong namespace, clear the outer snapshot, or have deletes resurrected by_post_update.
altk_evolve/backend/filesystem.py#L230-L246: reuse/push active state for nested updates and let only the outermost operation persist.altk_evolve/backend/filesystem.py#L205-L220: verify the active namespace before using the fast path.altk_evolve/backend/filesystem.py#L309-L322: never return active data for a different requested namespace.altk_evolve/backend/filesystem.py#L324-L333: route same-namespace nested deletes through the live snapshot.altk_evolve/backend/filesystem.py#L159-L165: reject or transactionally represent namespace deletion during an active update.Add tests for nested
update_entities, entity deletion, namespace deletion, and cross-namespace callbacks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@altk_evolve/backend/filesystem.py` around lines 230 - 246, Make filesystem transaction state namespace- and nesting-aware: update update_entities to push/reuse active state and let only the outermost operation persist, while restoring the prior state on exit. In altk_evolve/backend/filesystem.py lines 205-220, validate the active namespace before the fast path; lines 309-322, never serve active data for another namespace; lines 324-333, route same-namespace nested deletes through the live snapshot; and lines 159-165, reject or transactionally represent namespace deletion during an active update. Add tests covering nested updates, entity and namespace deletion, and cross-namespace callbacks.
🧹 Nitpick comments (1)
tests/unit/test_combine_guidelines.py (1)
186-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the complete internal-read call.
assert_called_once()still passes if the guideline filter or limit regresses.Proposed test improvement
mock_backend.search_entities.assert_not_called() -mock_backend._search_entities_impl.assert_called_once() +mock_backend._search_entities_impl.assert_called_once_with( + "test-ns", + query=None, + filters={"type": "guideline"}, + limit=10000, +)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/test_combine_guidelines.py` around lines 186 - 188, Strengthen the assertions for the internal read in the test around _search_entities_impl: verify it was called once with the expected guideline filter and limit arguments, rather than only checking invocation count. Keep the existing assertion that the public search_entities API is not called.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/check-code.yaml:
- Around line 106-127: Remove the OPENAI_API_KEY environment variable from the
run-tests job and ensure its checkout does not persist credentials for
PR-controlled code. Keep this job credential-free; move any tests requiring
OpenAI credentials to a protected workflow rather than exposing secrets during
run-tests.
In `@altk_evolve/hooks/plugins/pii.py`:
- Around line 29-43: Update the ImportError handling around the PII plugin
import so fallback occurs only for ModuleNotFoundError targeting cpex or
cpex_pii_filter, including their submodules. Remove the unnamed-error fallback
and re-raise generic ImportError instances; preserve _HAS_PII_FILTER as false
only for the explicitly missing optional dependencies.
In `@tests/unit/test_hooks_seam.py`:
- Around line 409-410: Update the test around the ThreadPoolExecutor invocation
to avoid its context manager, since __exit__ waits for a hung _write after the
timeout. Execute _write through a terminable process or thread, enforce the
existing 15-second timeout, and ensure the test can terminate without waiting
indefinitely for the operation.
- Around line 778-782: Deep-copy the entities when constructing norm_payload in
the MetadataNormalizerPlugin immutability test, ensuring nested metadata is
independent from norm_input. Preserve the existing assertions verifying
modified_payload is set and the original input payload remains unchanged.
---
Outside diff comments:
In `@altk_evolve/backend/filesystem.py`:
- Around line 230-246: Make filesystem transaction state namespace- and
nesting-aware: update update_entities to push/reuse active state and let only
the outermost operation persist, while restoring the prior state on exit. In
altk_evolve/backend/filesystem.py lines 205-220, validate the active namespace
before the fast path; lines 309-322, never serve active data for another
namespace; lines 324-333, route same-namespace nested deletes through the live
snapshot; and lines 159-165, reject or transactionally represent namespace
deletion during an active update. Add tests covering nested updates, entity and
namespace deletion, and cross-namespace callbacks.
---
Nitpick comments:
In `@tests/unit/test_combine_guidelines.py`:
- Around line 186-188: Strengthen the assertions for the internal read in the
test around _search_entities_impl: verify it was called once with the expected
guideline filter and limit arguments, rather than only checking invocation
count. Keep the existing assertion that the public search_entities API is not
called.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a1eca898-1142-4a21-a08c-b44aa57a4ef5
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.github/workflows/check-code.yamlaltk_evolve/backend/base.pyaltk_evolve/backend/filesystem.pyaltk_evolve/config/hooks.pyaltk_evolve/frontend/client/evolve_client.pyaltk_evolve/hooks/__init__.pyaltk_evolve/hooks/manager.pyaltk_evolve/hooks/plugins/__init__.pyaltk_evolve/hooks/plugins/access_stamp.pyaltk_evolve/hooks/plugins/normalizer.pyaltk_evolve/hooks/plugins/pii.pyaltk_evolve/hooks/types.pyaltk_evolve/llm/conflict_resolution/conflict_resolution.pydocs/guides/memory-hooks.mdexamples/hooks_plugins.yamlpyproject.tomltests/unit/test_combine_guidelines.pytests/unit/test_conflict_resolution.pytests/unit/test_hooks_plugin_cores.pytests/unit/test_hooks_seam.py
🚧 Files skipped from review as they are similar to previous changes (7)
- altk_evolve/hooks/init.py
- examples/hooks_plugins.yaml
- altk_evolve/config/hooks.py
- pyproject.toml
- docs/guides/memory-hooks.md
- altk_evolve/backend/base.py
- altk_evolve/hooks/manager.py
…ke update_entities Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The run-tests job in check-code.yaml runs on pull_request and executes PR-authored test code, yet exposed OPENAI_API_KEY (secret) and persisted the checkout token, so a malicious PR could exfiltrate them. - Add job-level `permissions: contents: read`. - Checkout with `persist-credentials: false`. - Drop the OPENAI_API_KEY job env: cpex-guarded hook tests mock `completion`, and live/e2e tests are deselected by the pytest addopts (`-m 'not llm and not e2e'`) the composite action inherits, so no real key is needed on the PR path. Credentialed coverage is unchanged: release-github.yaml has its own run-tests job (matrix 3.12/3.13, with the secret) for the workflow_dispatch/release path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sing module The PII plugin's import guard previously fell back to the stub whenever `ImportError.name` was cpex/cpex_pii_filter OR None. A name-less ImportError typically comes from a broken import inside an installed package, so treating it as "optional dep missing" masked a genuinely broken cpex install and silently disabled a compliance plugin. Fall back to the stub ONLY for a ModuleNotFoundError whose missing module is cpex/cpex_pii_filter (or a submodule of either). Every other ImportError -- name-less, or naming an unrelated broken transitive -- now re-raises. Update the cpex-blocking meta_path finder in test_hooks_plugin_cores to raise a properly-named ModuleNotFoundError(name=name) so it still faithfully simulates an absent optional dependency without weakening the production guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…epcopy-safe FIX3: the re-entrant write-hook deadlock test used a `with ThreadPoolExecutor()` whose __exit__ joins workers, so a regression that hung `_write` would hang CI forever even though `.result(timeout=15)` raised. Run the write in a daemon thread joined via `Event.wait(timeout=15)`; a deadlock now asserts-fails within ~15s and a daemon thread cannot block interpreter/pytest exit. FIX4: the shipped-plugin immutability guard built the payload with a shallow per-entity `dict(e)`, so the nested `metadata` dict was shared with the `*_input` reference -- an in-place mutation would change both and the equality assertion would pass spuriously. Build the payload from `copy.deepcopy(...)` so the reference is independent (both normalizer and PII cases). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Thank you for this — running the code to verify each finding made the difference, and the "security posture is inverted" framing was exactly right. All of it is addressed on the branch ( Blockers
High
Medium / lifecycle — second-client reset now warns loudly and the disabled path truly tears hooks down ( CI — you nailed the root cause: Two notes in the spirit of transparency: plugin timeouts currently surface as |
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
2 participants
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.
What
A general-purpose memory hook seam for
altk_evolve: the package defines its own hook types, frozen pydantic payloads (contents are deep-copied at dispatch, so in-place mutation by a plugin cannot reach the store — changes flow back only viamodified_payload), dispatch at the backend/LLM choke points, veto semantics, and pure engine-agnostic plugin cores. Plugin execution is provided by an engine integration behind a deliberately thin dispatch layer; the engine shipped is the CPEX plugin framework, used when the optionalcpexpackage is installed (new[hooks]extra). Swapping engines would mean reimplementing that thin dispatch layer — not the hook types, payloads, or plugin cores. The CPEX integration itself follows the intended plugin-manager/hook-point usage pattern the framework is designed for (a thin wrapper layer per host application, cf. Mellea's plugin integration).Why
Evolve's memory store is the boundary between agents and durable state. Compliance (PII redaction), normalization (
trace_idstamping), access auditing (last_accessed), and recall filtering all want to intercept that boundary. Instead of baking each concern in, this PR gives Evolve one extension seam with backend-layer choke points that no frontend (client, MCP server, CLI, Phoenix sync) can bypass.Zero default behavior change
EvolveConfig.hooks.enableddefaults to False. Without it — or without cpex installed — every hook site is a fast no-op (a boolean guard, thenhas_hooks_for), and behavior is byte-for-byte identical to today. cpex stays out of core deps because it pulls heavy transitives (fastapi, mcp, prometheus).Hook taxonomy
memory_pre_writeupdate_entities, after namespace validation, before conflict resolution (transforms run before content reaches an LLM)namespace_id,entitiesmemory_pre_metadata_patchupdate_entity_metadatamerges a patchnamespace_id,entity_id,metadata_patchmemory_pre_deletedelete_entity_by_idAND conflict-resolution DELETE verdicts insideupdate_entitiesnamespace_id,entity_id,metadata(stored entity's metadata,Noneif not found)memory_pre_namespace_deletedelete_namespacenamespace_idmemory_post_readsearch_entitiesresults only — internal reads never fire itnamespace_id,entities,query,filtersllm_pre_callcompletion(fact extraction, guidelines, segmentation, clustering, conflict resolution)messages,purpose,modelKey mechanics:
BaseEntityBackend's publicsearch_entities/delete_entity_by_id/delete_namespace/update_entity_metadatanow own hook dispatch and delegate to protected_*_implmethods (milvus/postgres/filesystem overrides renamed accordingly) — a backend override cannot skip a hook._search_entities_implseam; a context-local guard additionally suppresses nestedmemory_post_read.invoke_hookis async-only; the seam usesasyncio.runwhen no loop runs and a context-propagating dedicated thread when one does. Fire-and-forget plugin tasks are awaited before the loop closes so their side effects are never lost.MemoryPolicyViolation— blocked writes are errors, never silent drops.BaseEntityBackend._guarded_delete), so no delete through the backend abstraction can skipmemory_pre_delete; a veto ondelete_entity_by_idraises to the caller, while a vetoed conflict-resolution DELETE verdict skips just that delete (logged, recorded on the returnedEntityUpdateasevent="NONE"+skipped_deletemetadata) and the rest of the batch applies — a legal hold must not abort the whole write.EvolveConfig.hookssupports bothplugins_yaml(CPEX YAML, edit-to-change-behavior) and a code-firstplugins: list[HookPluginSpec](programmaticPluginConfigsynthesis). Initialized byEvolveClient.PluginManageris a process-wide Borg singleton; last initialization wins across multiple clients.What ships
altk_evolve/hooks/(types, manager, dispatch helpers),altk_evolve/config/hooks.py, choke-point wiring inbackend/+llm/+EvolveClient.altk_evolve/hooks/plugins/):MetadataNormalizerPlugin— copiestask_id→trace_id(MCPsave_trajectorywritestask_id; Phoenix sync writestrace_id; cascade cleanup keys ontrace_id) and stampscreated_at.AccessStampPlugin— fire-and-forgetlast_accessedstamping via the metadata-patch path.PIIFilterMemoryPlugin— aliasing subclass of the nativecpex-pii-filterplugin exposingmemory_pre_write+llm_pre_call(separate[pii]extra, since not everyone wanting hooks wants the PII dep).normalize_entities,build_access_stamps) — importable and tested without any extra installed — and the cpexPluginclasses are thin shims (parse config, call core, wrap inPluginResult). The PII adapter is the deliberate exception: adapting cpex-pii-filter onto our hook types is its domain logic.docs/guides/memory-hooks.md, commentedexamples/hooks_plugins.yaml, runnableexamples/hooks_demo.py.Deferred
EvolveConfig(today configured via the plugin'sconfigblock).TenantPluginManager) if multi-config clients become real.Test evidence
uv run pytest tests/), including 55 new tests acrosstest_hooks_noop.py(no-op guarantees, ImportError contract),test_hooks_seam.py(registration, per-choke-point dispatch, transform chaining, halting, unified delete path — CR-verdict hook fire with stored metadata, legal-hold veto skipping one delete while the rest of the batch applies, external delete raising, fetched-metadata payload — payload-immutability enforcement, sync bridge in both loop states, template-method no-bypass, recursion guard, internal-read exclusion, YAML + code-first config),test_hooks_plugins.py(all three shipped plugins through the cpex shims end-to-end), andtest_hooks_plugin_cores.py(17 cpex-free tests of the pure plugin cores: normalization flag/presence matrix with unchanged→None, access-stamp decisions, deterministic injected clock, plus a subprocess check that the cores import and work with cpex imports blocked — verified green under a meta_path cpex blocker).uv run python examples/hooks_demo.py):test_update_entitiesin milvus/postgres) now patch_search_entities_implinstead ofsearch_entities— they were feeding the conflict-resolution pre-read, which is now explicitly an internal read.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
altk-evolve[hooks]/altk-evolve[pii]).Bug Fixes
Documentation
Tests