Skip to content

fix(memory): log and degrade gracefully on invalid UTF-8 in memory files - #57755

Closed
JoaoMarcos44 wants to merge 2 commits into
NousResearch:mainfrom
JoaoMarcos44:fix/memory-encoding-corruption-silent-disable
Closed

fix(memory): log and degrade gracefully on invalid UTF-8 in memory files#57755
JoaoMarcos44 wants to merge 2 commits into
NousResearch:mainfrom
JoaoMarcos44:fix/memory-encoding-corruption-silent-disable

Conversation

@JoaoMarcos44

Copy link
Copy Markdown
Contributor

Fixes #57754

Problem

MemoryStore._read_file() (tools/memory_tool.py:691-694) reads MEMORY.md/USER.md with path.read_text(encoding="utf-8") and only catches (OSError, IOError) around it.

UnicodeDecodeError is a ValueError subclass, not an OSError/IOError, so invalid UTF-8 bytes in either file raise past this catch. The exception then propagates into agent_init.py:1239-1253's broad try/except Exception: pass, which silently swallows it. Net effect: memory is disabled for the whole session (agent._memory_store stays None), while agent._memory_enabled/agent._user_profile_enabled remain True from config — and no log line is ever emitted. There is nothing for an operator to go on when debugging "why did memory stop loading."

Fix

Catch UnicodeDecodeError explicitly in _read_file(), log a clear error (file path + underlying decode error + remediation hint), and degrade to an empty entry list — same graceful-degradation behavior as before, but now visible.

try:
    raw = path.read_text(encoding="utf-8")
except (OSError, IOError):
    return []
except UnicodeDecodeError as e:
    logger.error(
        "Memory file %s has invalid UTF-8 (%s) -- treating as empty. "
        "Fix or remove the file to restore memory.",
        path, e,
    )
    return []

This is a minimal, targeted fix at the exact point the exception was escaping — no changes to control flow, call sites, or the broader except Exception: pass in agent_init.py (that catch-all is intentionally permissive by design for other truly-optional memory init steps; narrowing it is a separate, larger change out of scope here).

Verification

Reproduced the bug against main first (confirmed silent failure), then verified the fix on this branch:

from pathlib import Path
from tools.memory_tool import MemoryStore

p = Path(tempfile.mkdtemp()) / "MEMORY.md"
p.write_bytes(b"\xff\xfe invalid utf8 bytes")

result = MemoryStore._read_file(p)
# -> ERROR:tools.memory_tool:Memory file ...MEMORY.md has invalid UTF-8
#    ('utf-8' codec can't decode byte 0xff in position 0: invalid start byte)
#    -- treating as empty. Fix or remove the file to restore memory.
# result: []

Test plan

  • PoC reproduces silent failure on unpatched code (no log emitted, _read_file raises uncaught)
  • After fix: _read_file returns [] and emits a logger.error with file path + decode error
  • No behavior change for valid UTF-8 files or missing files

_read_file() only caught (OSError, IOError) around read_text(encoding="utf-8").
UnicodeDecodeError is a ValueError subclass, so it escaped that catch, propagated
through load_from_disk(), and hit agent_init.py's bare except Exception: pass --
silently disabling memory (both MEMORY.md and USER.md) for the entire session
with zero log output. An operator debugging "why did memory stop working" had
no signal to go on.

Catch UnicodeDecodeError explicitly at the source and log a visible error
before returning an empty entry list, so corruption is diagnosable instead of
silently swallowed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@alt-glitch alt-glitch added type/bug Something isn't working tool/memory Memory tool and memory providers P3 Low — cosmetic, nice to have labels Jul 3, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to the memory non-UTF-8 fix cluster for the same bug (#49508 / #10879 family, and the issue this Fixes, #57754). Note the mechanisms differ: #49516 and #10888 recover content (retry with errors='replace' / utf-8-sig), while this PR degrades to an empty entry list with a log line. Competing fixes — maintainer should pick a degradation strategy (recover-with-replacement vs. fail-visible-empty).

@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 isolating the decode failure. The premise is confirmed on current main: tools/memory_tool.py:692 performs a strict UTF-8 read without catching UnicodeDecodeError, and the initialization path swallows that failure at agent/agent_init.py:1389-1391.

Problems

  • The same uncaught strict read remains in _detect_external_drift() at tools/memory_tool.py:732. _reload_target() calls it at tools/memory_tool.py:303 before replace, remove, and apply_batch, so those mutation paths still fail on invalid bytes.
  • Returning an empty list can lose the corrupt file's contents. add() deliberately skips the drift guard at tools/memory_tool.py:354, then saves its in-memory list at tools/memory_tool.py:382-384; after this fallback that writes over the original invalid bytes.
  • No regression test is added. Existing persistence/load tests at tests/tools/test_memory_tool.py:481-503 and :843-917 cover valid UTF-8 only.

Suggested changes

  • Select one data-preserving corrupt-file strategy and apply it to both strict read sites; the existing discussion correctly notes the trade-off between recovery and fail-visible handling.
  • Add invalid-byte coverage for initialization and subsequent mutation paths, including protection against overwrite.

Automated hermes-sweeper review.

Comment thread tools/memory_tool.py
@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 area/memory Memory subsystem: store, providers, sync, background reviews labels Jul 15, 2026
…clobbering them

teknium1's review on NousResearch#57755 pointed out two gaps in the original fix:

- _detect_external_drift() still did a strict UTF-8 read, so replace/remove/
  apply_batch crashed uncaught on invalid bytes instead of degrading.
- add()'s skip_drift=True path treated a corrupt file as empty, then
  save_to_disk() overwrote it, permanently discarding the original content.

Split a target-agnostic _check_decode_corruption() out of
_detect_external_drift() and run it unconditionally in _reload_target(),
even when skip_drift is set. An undecodable file now backs itself up
(raw bytes, since it can't round-trip through the text path) and refuses
the mutation via the existing drift_backup/_drift_error contract, the
same guard already in place for round-trip drift (NousResearch#26045).

Adds regression coverage for the init-time degrade-and-log path plus
replace/remove/add refusal and backup-preservation on invalid UTF-8,
which the original PR shipped without.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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 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.

Encoding corruption in MEMORY.md/USER.md silently disables memory, with no log

3 participants