Skip to content

fix: make memory and plugin encoding robust - #18157

Closed
flyingdoubleG wants to merge 2 commits into
NousResearch:mainfrom
flyingdoubleG:codex/fix-memory-plugin-encoding
Closed

fix: make memory and plugin encoding robust#18157
flyingdoubleG wants to merge 2 commits into
NousResearch:mainfrom
flyingdoubleG:codex/fix-memory-plugin-encoding

Conversation

@flyingdoubleG

@flyingdoubleG flyingdoubleG commented May 1, 2026

Copy link
Copy Markdown
Contributor

What changed

  • Read plugin manifests with explicit UTF-8 encoding in the general plugin manager, memory provider discovery, and context-engine discovery.
  • Make built-in memory file reads tolerate legacy/non-UTF-8 bytes with replacement instead of disabling the memory store on decode errors.
  • Reject memory entries containing the reserved on-disk delimiter line §, preventing one saved memory from being split into multiple entries on the next load.
  • Add regression coverage for UTF-8 plugin manifests on locale-sensitive systems, non-UTF-8 legacy memory files, and delimiter-corruption prevention.

Why

A real Codex-backed memory baseline on Windows surfaced locale-dependent failures: plugin discovery tried to parse UTF-8 plugin.yaml files through the system default GBK codec, producing startup noise and potentially skipping plugins. Memory files can also be hand-edited, synced, or inherited from older runs, so a decode error should not take down the memory store.

Follow-up hard cases also showed that saving content containing the literal entry delimiter (\n§\n) corrupted the next load by splitting one memory into multiple memories. The memory tool now rejects that reserved delimiter before writing.

Validation

  • D:\工作\hermes-agent\.venv-win\Scripts\python.exe -m pytest tests\tools\test_memory_tool.py -q -> 37 passed
  • D:\工作\hermes-agent\.venv-win\Scripts\python.exe -m pytest tests\hermes_cli\test_plugins.py::TestPluginDiscovery::test_manifest_parse_uses_utf8_not_locale -q -> 1 passed
  • Real Codex baseline and hard memory checks were run in isolated HERMES_HOME directories before/after the fixes, including explicit save/recall, correction, forget, do-not-save background review, conflicting memories, scanner-sensitive preference rephrasing, legacy encoding, and delimiter corruption.

Notes

scripts/run_tests.sh could not run in this Windows shell because the available bash.exe is the WSL shim and failed on CRLF/pipefail parsing before pytest started.

@flyingdoubleG
flyingdoubleG marked this pull request as ready for review May 1, 2026 02:08
@flyingdoubleG flyingdoubleG changed the title [codex] fix memory and plugin encoding robustness fix: make memory and plugin encoding robust May 1, 2026
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers labels May 1, 2026
@flyingdoubleG

Copy link
Copy Markdown
Contributor Author

Real-agent validation (2026-05-01): PASS

Isolated HERMES_HOME: D:\工作\hermes-agent-pr-real-agent\pr18157
Command shape: python -m hermes_cli.main --yolo --provider openai-codex -m gpt-5.5 -t memory -z ...
Scenario: asked the real OpenAI Codex-backed Hermes agent to save a long-term memory containing a standalone delimiter line (§).
Agent result: PR18157_AGENT_OK,被拒绝。

This validates the delimiter guard through the actual agent/tool path, not only unit tests.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 12, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the Windows-focused regression coverage. Current main already decodes general manifests explicitly at hermes_cli/plugins.py:1576, context-engine manifests at plugins/context_engine/__init__.py:57, and memory-provider manifests at plugins/memory/__init__.py:161 / 427; the memory portion remains relevant.

Problems

  • tools/memory_tool.py:115 only detects a complete "\n§\n" sequence inside submitted content. An entry ending in "\n§" passes, but adding a subsequent entry joins it with ENTRY_DELIMITER (tools/memory_tool.py:768) and a fresh load splits it (tools/memory_tool.py:701).
  • The tolerant decode is limited to _read_file; current main's _detect_external_drift() still strictly reads at tools/memory_tool.py:732, affecting replace/remove.

Suggested changes

  • Detect delimiter-only lines or use an escaping/structured representation, with an add/save/reload boundary test.
  • Cover both read paths when porting the legacy-decoding behavior.

Automated hermes-sweeper review.

Comment thread tools/memory_tool.py

The memory file format uses a line containing only ``§`` to separate
entries. Allowing that exact line inside one entry corrupts the next load by
splitting the saved item into multiple memories.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This only catches a complete delimiter already inside content. Content ending in \n§ passes on its first save, but adding a later entry joins it with ENTRY_DELIMITER and causes a fresh load to split the original entry. Reject delimiter-only lines (or escape them) and add a save/reload regression for that boundary case.

@teknium1 teknium1 added the area/memory Memory subsystem: store, providers, sync, background reviews label Jul 19, 2026

@GottZ GottZ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This was generated by AI during triage.

Summary

Two PRs touch this complex, but their diffs address distinct causes: #18157 combines now-redundant UTF-8 manifest handling with still-relevant memory-file decoding and delimiter-integrity changes, while #30761 adds a parallel user context-engine discovery path for functionality already available through the general plugin system on main.

Related pull requests

  • #18157 related — (+82/-6) — keep open with a salvage path: retain the memory robustness work, but drop the manifest changes already present on main and address the contributor keep_open review by rejecting every delimiter-only line, including content ending in \n§, adding an add/save/reload boundary test, and applying tolerant decoding to _detect_external_drift() as well as _read_file.
  • #30761 [closed] related — (+132/-44) — close as already implemented on main: consistent with the MAINTAINER-BOT close verdict, the canonical path already scans user plugins at hermes_cli/plugins.py:1347-1352, registers context engines at hermes_cli/plugins.py:614-642, resolves them at agent/agent_init.py:1755-1805, and is covered at tests/hermes_cli/test_plugins.py:1950-1989; although closed, #30761 remains relevant as the competing parallel-discovery design.

Suggested consolidation

Keep #18157 open only with the concrete memory-only salvage path described above; its current diff does not yet cover the delimiter boundary or strict external-drift read identified by the contributor review. Leave #30761 closed as already implemented on main through the cited canonical user-plugin path. The PRs are not duplicates, so neither should be closed as a duplicate of the other.

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 17 kB of PR diffs, 4 kB of issue/PR text, 2 kB of discussion (2 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

@teknium1

teknium1 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

The encoding half was salvaged into #81967 with Co-authored-by credit. Closing. Thanks!

@teknium1 teknium1 closed this Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/memory Memory subsystem: store, providers, sync, background reviews comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state tool/memory Memory tool and memory providers type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants