[codex] add observational memory provider - #4787
Conversation
PR Review — Observational Memory ProviderReviewed against: observational-memory v0.3.0 source (all integration points verified) CI: all green. API surface alignment: confirmed correct. Scope noteThis PR bundles 4 independent changes:
Consider splitting 2–4 into their own PRs for cleaner bisect/revert if needed. Not a blocker. Issues1.
2. Dead default search query in text = self._build_context(
query=query or _DEFAULT_SEARCH_QUERY,
limit=limit,
include_search=bool(query),
)When 3. Throwaway Config in preload = OMConfig(memory_dir=memory_dir, env_file=env_file)
preload.load_env_file()
# ...
return OMConfig(**kwargs) # second Config createdThe 4. def on_session_end(self, messages):
if self._sync_thread and self._sync_thread.is_alive():
self._sync_thread.join(timeout=10.0)
self._flush_pending(force=True)If the background sync thread is still alive after the 10s join timeout, 5. No size guard on
6. This propagates Nits (non-blocking)
VerdictThe core provider implementation is solid — correct API usage, proper thread safety, good test coverage. The issues above are all minor/moderate. #4 (silent flush drop) is the one I'd want addressed before merge; the rest are fine to defer. |
|
Correction: Review was verified against observational-memory v0.3.1 (current |
Re-review after
|
| # | Finding | Resolution |
|---|---|---|
| 1 | "bot" / "sg." markers too broad |
Replaced with targeted regexes (bot\d{8,}: for Telegram, sg\.[A-Za-z0-9_-]{10,} for SendGrid). New tests confirm both patterns still trigger redaction in large payloads. |
| 2 | Dead _DEFAULT_SEARCH_QUERY |
Removed entirely. |
| 3 | Throwaway Config confusing | Renamed preload → bootstrap_cfg, added explanatory comment. |
| 4 | Silent flush drop (blocker) | on_session_end now warns and defers via _defer_final_flush — waits for active thread to finish, then forces flush. New threaded test validates the full sequence. |
| 5 | No size guard on prompt | _truncate_prompt_section caps at 4K chars with notice. New test confirms. |
| 6 | Env var mutation (informational) | No change needed. |
Nits also addressed: _restore_pending_messages uses in-place prepend, sync test pins min_messages explicitly.
_flush_pending decomposition into _take_pending_messages / _restore_pending_messages / _run_observer_batch / _defer_final_flush is clean. The current_thread() guard correctly prevents the deferred flush from self-blocking.
CI: all green. LGTM — ready to merge.
|
Adding a short positioning note here because it may help frame where this provider fits in the Hermes memory lineup. Observational Memory is strongest when the goal is cross-agent continuity that stays local and inspectable. In practice that means Hermes can share the same markdown memory store with Claude Code and Codex, while still giving users readable files, local search, compact startup context, and optional writeback. Relative to the other providers:
The underlying Mastra's published results for that underlying OM architecture report 84.23% on LongMemEval with I also added a longer version of this positioning note to the standalone plugin README here: |
5166364 to
c743975
Compare
The 0.4.1 release includes a dedicated Hermes JSONL session log parser that filters to user/assistant prose only, achieving ~19x noise reduction on typical sessions. This is required for effective cron-based observation extraction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
results is a dict keyed by command string — iterating over the dict directly yields keys (strings), not the result dicts, causing "string indices must be integers" TypeError. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… writeback Sync from intertwine/hermes-observational-memory@ad91c68: - register() no-ops when called by the general plugin loader (avoids noisy 'no attribute register_memory_provider' warning at startup) - Log explicit warning when writeback is configured but LLM provider is missing, instead of silently disabling Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sync from intertwine/hermes-observational-memory@3d7d3d5: - After each observer batch, check if reflections have fallen behind and run the reflector inline if needed. Builds long-term memory without requiring a separate background scheduler. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
9d623b1 to
20db63a
Compare
ZaynJarvis
left a comment
There was a problem hiding this comment.
Review: Observational Memory Provider + Bundled Fixes
Verdict: Request Changes — please split into two PRs
This PR bundles four independent changes. Three of them (agent/redact.py, gateway/run.py, supply-chain-audit.yml) are clean, independently valuable fixes ready to merge. The OM provider is the main feature and should be reviewed separately.
Part 1: Ready to Extract (cherry-pick into a separate PR) ✅
agent/redact.py — fast-path pre-filter:
The _may_contain_sensitive_markers() optimization is well-designed: cheap in checks on lowercased text before running the full regex suite, with correct E.164 phone fallback. The _FAST_SCAN_MIN_LEN = 4096 threshold is reasonable. This is a clean performance win with no behavioral risk.
gateway/run.py — model resolution fix:
The _resolve_active_model() (or equivalent) now checks HERMES_MODEL env var first, then config, then LLM_MODEL fallback. This prevents auxiliary AIAgent instances (memory flush, /compress) from falling back to the wrong model. Clean and correct.
.github/workflows/supply-chain-audit.yml:
Job summary output for fork PRs (where comment posting is blocked) + regex anchoring fix are both correct CI improvements.
Part 2: Observational Memory Provider — Review After Split
The OM provider itself (plugins/memory/observational_memory/) needs its own review pass:
- Confirm it uses
get_hermes_home()(not hardcoded~/.hermes) for any state files - Verify
is_available(),initialize(),on_session_end()follow theMemoryProviderinterface contract - Check test coverage for the provider logic (264 lines of tests referenced in analysis)
- Verify no circular imports
Action: Please split the three independent fixes (redact.py, gateway/run.py, CI workflow) into one PR and keep the OM provider in a separate focused PR. Both PRs will be reviewed quickly since the non-OM parts are already approved above.
|
Opened a focused replacement PR for the Observational Memory provider here: This refresh is split off from the broader mixed-scope branch so reviewers can look at the OM integration on its own, rebased onto current I’m leaving #4787 in place for history/context, but the new PR is the one to review for the OM-only version. |
|
Closing this in favor of resolved PR scope issues in a new followup PR #12583 |
What changed
observational_memorymemory provider plugin underplugins/memory/hermes memory setup, CLI help text, and user docsWhy
Hermes's new pluggable memory-provider architecture makes Observational Memory a clean fit as a first-class backend. This gives Hermes access to the same local markdown memory store used by Claude Code and Codex, while also letting Hermes contribute back into that shared memory when writeback is enabled.
User impact
observational_memorydirectly fromhermes memory setupprofile.md+active.mdom_context,om_search, andom_rememberValidation
python -m pytest -o addopts='' tests/agent/test_observational_memory_provider.py tests/agent/test_memory_provider.py tests/agent/test_memory_plugin_e2e.pypython -m compileall plugins/memory/observational_memory tests/agent/test_observational_memory_provider.py