Skip to content

fix(openviking): read .env BOM-tolerantly when rewriting credentials - #78940

Closed
Drexuxux wants to merge 1 commit into
NousResearch:mainfrom
Drexuxux:fix/openviking-env-read-utf8-sig
Closed

fix(openviking): read .env BOM-tolerantly when rewriting credentials#78940
Drexuxux wants to merge 1 commit into
NousResearch:mainfrom
Drexuxux:fix/openviking-env-read-utf8-sig

Conversation

@Drexuxux

@Drexuxux Drexuxux commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

What

75afc47 (2026-07-10, fix(memory): read/write .env as UTF-8 in mem0 and hindsight setup) and then f1ea4a5 (2026-07-12, fix(memory): cover the remaining setup-time .env reads with utf-8-sig) hardened this exact class. plugins/memory/openviking/__init__.py::_write_env_vars was missed and still reads with strict UTF-8:

existing_lines = env_path.read_text(encoding="utf-8").splitlines() if env_path.exists() else []

The function copies every existing line through on each update, so the read decides whether a credential update actually lands. Two failure modes, both on .env — the file that holds API keys. Real writer, real files:

--- on main
BOM case    -> ['OPENAI_API_KEY=old', 'OTHER=1', 'OPENAI_API_KEY=new']
cp1252 case -> RAISED UnicodeDecodeError: 'utf-8' codec can't decode byte 0xe9

--- with the fix
BOM case    -> ['OPENAI_API_KEY=new', 'OTHER=1']
cp1252 case -> OK
  • UTF-8 BOM (what a Windows GUI editor leaves): the first line's key reads as \ufeffOPENAI_API_KEY, so it never matches. The stale line survives and the new value is appended as a duplicate. .env loaders keep the first occurrence, so the user changes their key, setup reports success, and the old key is still what gets loaded.
  • cp1252 (Notepad's other default): the read raises and openviking setup aborts.

CONTRIBUTING.md calls out both cases explicitly under Cross-Platform Compatibility, and the sibling writer in plugins/memory/mem0/_setup.py already carries a comment explaining precisely this hazard — it just never reached this file.

The fix

Read exactly like the canonical .env reader, hermes_cli/config.py::save_env_value:

env_path.read_text(encoding="utf-8-sig", errors="replace")

utf-8-sig strips a BOM if present and is a no-op otherwise; errors="replace" keeps a mis-encoded neighbouring line from aborting the write. A plain UTF-8 .env rewrites byte-identically.

One consequence worth naming: on a cp1252 file, an unrelated non-ASCII value is rewritten with a replacement character rather than its original bytes. That is the same trade the canonical writer already makes — the alternative is the current behaviour, where setup fails outright and nothing is written at all.

Scope: hermes_cli/memory_setup.py has the same read, but it is already the subject of #30281 and #60587, so it is deliberately left alone here. plugins/memory/mem0/_setup.py was fixed by f1ea4a5; hermes_cli/doctor.py and hermes_cli/main.py already guard the decode.

Tests

Added TestOpenVikingEnvWriter to tests/plugins/memory/test_openviking_provider.py:

  • a BOM-prefixed .env updates the key in place — exactly one OPENAI_API_KEY line, no leftover =old
  • a cp1252 .env no longer aborts the write
  • a plain UTF-8 .env comes out with the same lines in the same order — the guard against the read change altering ordinary files

Results:

54 passed, 2 skipped

That is the whole file; the 51 pre-existing tests are unchanged.

Red without the source change:

FAILED TestOpenVikingEnvWriter::test_bom_prefixed_env_updates_in_place
E   assert not True          # an "=old" line survived alongside the new one
FAILED TestOpenVikingEnvWriter::test_non_utf8_env_does_not_abort_setup
E   UnicodeDecodeError: 'utf-8' codec can't decode byte 0xe9 in position 8

The third test passes on both sides, which is what it is for.

Regression over tests/plugins/memory/ against the same files on main:

main:      7 failed, 261 passed, 8 skipped   (excluding the 2 new tests failing as above)
with fix:  7 failed, 261 passed, 8 skipped

Set difference is empty — no new failures. The 7 are identical before and after and sit in the hindsight and holographic suites, unrelated to this change.

f1ea4a5 ("cover the remaining setup-time .env reads with utf-8-sig",
following 75afc47 for mem0/hindsight) swept this class; openviking's
_write_env_vars was missed and still reads with strict utf-8.

It copies every existing line through on each update, so the read decides
whether a credential update lands:

  BOM'd .env  -> the first key never matches, so the old line survives and
                 the new value is appended as a duplicate. .env loaders keep
                 the first occurrence, so the update silently does nothing.
  cp1252 .env -> UnicodeDecodeError aborts setup outright.

Read exactly like the canonical hermes_cli/config.py save_env_value
(utf-8-sig + errors="replace"). A plain UTF-8 file rewrites byte-identically.

Scope: hermes_cli/memory_setup.py has the same read but is already the
subject of NousResearch#30281 / NousResearch#60587, so it is left alone here.
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins tool/memory Memory tool and memory providers platform/windows Native Windows-specific behavior or breakage P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have platform/windows Native Windows-specific behavior or breakage sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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.

3 participants