fix(openviking): harden tool gating, config auth, and runtime shutdown by ehz0ah · Pull Request #51952 · NousResearch/hermes-agent

ehz0ah · 2026-06-24T14:51:39Z

What does this PR do?

Hardens the existing OpenViking memory provider by consolidating focused upstream PRs and related current-main fixes on top of current main.

This keeps the provider aligned with the current OpenViking source contract while addressing these independent bug classes:

prevents embedded line separators and NUL bytes in OpenViking secret values from escaping their .env line and corrupting persisted config;
refreshes the live OpenViking client after /reload so updated OPENVIKING_* settings are picked up without restarting Hermes;
preserves local runtime recovery after /reload, including starting an unavailable local OpenViking server and attaching it in the background;
publishes refreshed config and client state atomically, preventing concurrent accesses from using a stale endpoint, API key, or tenant identity;
serializes local runtime recovery so repeated or concurrent accesses cannot spawn duplicate OpenViking processes;
binds each runtime waiter to the endpoint that started it and prevents stale waiters from overwriting newer config or publishing a client during shutdown;
lets unavailable remote OpenViking endpoints reconnect on a later access instead of leaving the provider disconnected for the whole run;
prevents external memory-provider tools from leaking into sessions where the memory toolset is explicitly disabled, including after MCP tool-snapshot refreshes;
matches trusted-mode missing-identity retry errors structurally while keeping deliberate ROOT API-key data-API denials non-retriable;
joins the runtime-autostart waiter during shutdown and lets its health loop stop promptly, avoiding daemon-thread-at-exit crashes.

The branch preserves contributor authorship and release attribution for the original work:

Related Issue

Refs #21130

Supersedes / safe to close after this lands:

fix(openviking): sanitize embedded newlines when writing .env secrets #50315 — included here as the .env writer hardening commit.
fix(openviking): refresh client from env on /reload #21138 — included here, adapted to the current provider and extended so local recovery, remote retry, concurrent refresh, endpoint changes, and shutdown remain safe.
fix(openviking): join runtime-autostart thread on shutdown (SIGABRT-at-exit) #49832 — included here, adapted to preserve the existing memory-write shutdown drain while adding the runtime-autostart thread drain.
fix(openviking): match tenant-header errors structurally instead of hard-coding strings #59454 — included here, narrowed to trusted-mode missing-identity retries so ROOT API-key data-API denials remain non-retriable.
fix(openviking): resolve 403 with API key auth and late .env loading #21136 — late .env and auth-refresh value is covered by the current _ensure_client() implementation here.
fix(memory): respect toolsets and reconnect openviking #18166 — current main already resolves enabled composite toolsets correctly. This PR adapts the remaining disabled-toolset and reconnect behavior to the current shared memory-provider injection, MCP refresh, and OpenViking runtime-health paths.

Type of Change

🐛 Bug fix (non-breaking change that fixes an issue)
✨ New feature (non-breaking change that adds functionality)
🔒 Security fix
📝 Documentation update
✅ Tests (adding or improving test coverage)
♻️ Refactor (no behavior change)
🎯 New skill (bundled or hub)

Changes Made

agent/memory_manager.py
- Extends the shared provider-tool gate so an explicit disabled_toolsets: ["memory"] takes precedence over default, direct, all, and composite enablement.
- Preserves the existing built-in-memory-tool fallback when memory is not explicitly disabled.
tools/mcp_tool.py
- Reuses the same shared gate when rebuilding the agent tool snapshot, preventing stale provider tools from being re-injected after /reload-mcp or late MCP discovery.
plugins/memory/openviking/__init__.py
- Adds _env_line_safe() and uses it at both .env write sites.
- Sanitizes all line separators recognized by Python splitlines() plus NUL so persisted .env values remain single-line.
- Adds _ensure_client() and wires all live OpenViking access paths through it, including current-query recall, tool calls, sync, session end/switch, and memory mirroring.
- Reuses the existing local runtime-autostart path when /reload resolves to an unavailable local endpoint, while unavailable remote endpoints continue retrying on later accesses.
- Serializes client refresh so connection settings and _client are published as one coherent state.
- Serializes local process starts, keeps startup status ordered before waiter attachment, and ties each waiter to its originating endpoint.
- Stops pending or finalizing waiters from publishing after endpoint changes or shutdown.
- Keeps trusted-identity retry scoped to OpenViking trusted-mode missing-identity errors.
- Keeps ROOT API-key tenant data-API denials non-retriable.
- Adds shutdown-aware _wait_for_openviking_health(..., should_stop=...) and joins _runtime_start_thread while preserving existing memory-write worker draining.
tests/agent/test_memory_provider.py
- Covers disabled-memory precedence across default, direct, all, and composite enabled-toolset configurations.
tests/tools/test_refresh_agent_mcp_tools.py
- Covers removing a stale external memory-provider tool during an MCP snapshot refresh when memory is disabled.
tests/openviking_plugin/test_openviking.py
- Exercises the real reload_env() path with an isolated temporary HERMES_HOME.
- Covers reload from a healthy remote endpoint to an unavailable local endpoint, later remote reconnect, coherent concurrent refresh, and single-owner local recovery.
tests/plugins/memory/test_openviking_provider.py
- Adds .env newline, splitline, and NUL sanitization regressions.
- Covers trusted-mode tenant-header retry and keeps ROOT tenant errors out of the retry path.
- Covers startup status ordering, endpoint-bound waiter cancellation, and runtime waiter attachment.
tests/plugins/memory/test_openviking_shutdown.py
- Covers waiter short-circuiting, runtime-start thread joining, pending-start shutdown, and shutdown during the final health probe.
scripts/release.py
- Adds contributor mappings for the salvaged fix(openviking): join runtime-autostart thread on shutdown (SIGABRT-at-exit) #49832, fix(openviking): match tenant-header errors structurally instead of hard-coding strings #59454, and fix(memory): respect toolsets and reconnect openviking #18166 work.

How to Test

scripts/run_tests.sh tests/agent/test_memory_provider.py tests/tools/test_refresh_agent_mcp_tools.py tests/openviking_plugin/test_openviking.py tests/plugins/memory/test_openviking_provider.py tests/plugins/memory/test_openviking_shutdown.py -- -o addopts=
.venv/bin/python -m py_compile agent/memory_manager.py tools/mcp_tool.py plugins/memory/openviking/__init__.py tests/agent/test_memory_provider.py tests/tools/test_refresh_agent_mcp_tools.py tests/openviking_plugin/test_openviking.py tests/plugins/memory/test_openviking_provider.py tests/plugins/memory/test_openviking_shutdown.py scripts/release.py
.venv/bin/ruff check agent/memory_manager.py tools/mcp_tool.py plugins/memory/openviking/__init__.py tests/agent/test_memory_provider.py tests/tools/test_refresh_agent_mcp_tools.py tests/openviking_plugin/test_openviking.py tests/plugins/memory/test_openviking_provider.py tests/plugins/memory/test_openviking_shutdown.py scripts/release.py
.venv/bin/python scripts/check-windows-footguns.py agent/memory_manager.py tools/mcp_tool.py plugins/memory/openviking/__init__.py tests/agent/test_memory_provider.py tests/tools/test_refresh_agent_mcp_tools.py tests/openviking_plugin/test_openviking.py tests/plugins/memory/test_openviking_provider.py tests/plugins/memory/test_openviking_shutdown.py scripts/release.py
git diff --check upstream/main...HEAD

Local result:

Focused memory/OpenViking suite: 317 tests passed, 0 failed
py_compile: pass
ruff: pass
check-windows-footguns.py: pass
git diff --check upstream/main...HEAD: pass

Checklist

Code

I've read the Contributing Guide
My commit messages follow Conventional Commits
I searched for existing PRs to make sure this isn't a duplicate
My PR contains only changes related to this fix
I've run pytest tests/ -q and all tests pass
I've added tests for my changes
I've tested on my platform: macOS / Darwin local checkout

Documentation & Housekeeping

I've updated relevant documentation — or N/A
I've updated cli-config.yaml.example if I added/changed config keys — N/A
I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
I've considered cross-platform impact per the compatibility guide
I've updated tool descriptions/schemas if I changed tool behavior — N/A

tonydwb

Hermes Agent Review

Verdict: Approved — Clean, well-scoped change with appropriate tests.

Reviewed as part of batch review session 2026-06-24d.

Reviewed by Hermes Agent

`_write_env_vars` in the OpenViking memory provider interpolates each secret straight into a `KEY=VALUE` line, but the values only ever pass through `_clean_config_value`, whose `value.strip()` trims surrounding whitespace and leaves internal CR/LF intact. Because the file is strictly line-oriented and is re-read via `read_text().splitlines()`, a value that carries an embedded newline spills onto a second physical line, and the tail is re-parsed as an independent `KEY=VALUE` entry on the next round trip. A secret pasted with a trailing record (e.g. an `OPENVIKING_API_KEY` copied with an extra line) therefore injects an arbitrary additional variable into the persisted credentials file and silently corrupts it. The fix neutralizes the line terminators at the single chokepoint where values reach the file. A small `_env_line_safe` helper strips `\r`, `\n`, and the NUL byte from each value, and both write sites in `_write_env_vars` (the existing-key update branch and the appended-key branch) route through it, so a value can only ever occupy the single line it is written on. ## What does this PR do? Hardens the OpenViking memory provider's `.env` writer so a malformed or pasted secret value can no longer break out of its `KEY=VALUE` line and inject a rogue variable into the profile-scoped credentials file. ## Related Issue N/A ## Type of Change - [x] 🐛 Bug fix (non-breaking change that fixes an issue) ## Changes Made - `plugins/memory/openviking/__init__.py`: add `_env_line_safe()` which removes `\r`, `\n`, and `\x00` from a value, and apply it to both the updated-key and appended-key write branches in `_write_env_vars()`. - `tests/plugins/memory/test_openviking_provider.py`: add two regression tests covering a fresh write and an in-place key update with embedded CR/LF, asserting no injected line survives the read-back. ## How to Test 1. Run the targeted tests: `pytest tests/plugins/memory/test_openviking_provider.py -k env_writer -q` 2. Reverting the `_env_line_safe` sanitization makes `test_openviking_env_writer_strips_embedded_newlines_in_values` and `test_openviking_env_writer_strips_newlines_when_updating_existing_key` fail with a rogue `INJECTED_KEY=`/`ROGUE=1` line appearing in the file, confirming the tests pin the bug. 3. `ruff check plugins/memory/openviking/__init__.py` and `python scripts/check-windows-footguns.py plugins/memory/openviking/__init__.py` both pass. ## Checklist ### Code - [x] I've read the Contributing Guide - [x] My commit messages follow Conventional Commits - [x] I searched for existing PRs to make sure this isn't a duplicate - [x] My PR contains only changes related to this fix - [x] I've run the relevant tests and they pass - [x] I've added tests for my changes (required for bug fixes) - [x] I've tested on my platform: macOS 15 (Darwin 25.5) ### Documentation & Housekeeping - [x] I've updated relevant documentation (docstrings) — or N/A - [x] I've updated `cli-config.yaml.example` if I added/changed config keys — N/A - [x] I've updated `CONTRIBUTING.md` or `AGENTS.md` if I changed architecture or workflows — N/A - [x] I've considered cross-platform impact (strips CR as well as LF) — done - [x] I've updated tool descriptions/schemas if I changed tool behavior — N/A (cherry picked from commit f29dd2d)

initialize() snapshots OPENVIKING_* into the provider once, so /reload (which only updates os.environ) leaves viking_* tools running against stale auth — users have to restart hermes to pick up keys added to ~/.hermes/.env after startup. Add _ensure_client(), which re-resolves the connection settings via the same _resolve_connection_settings/_load_hermes_openviking_config path initialize() uses and rebuilds + health-checks the client only when an OPENVIKING_* value actually changed; otherwise it reuses the cached client so the hot path stays at one dict comparison with no network calls. Every `if not self._client:` guard in system_prompt_block, queue_prefetch, sync_turn, on_session_end, on_memory_write and handle_tool_call now goes through it. Refreshing is gated behind a flag set at the end of initialize() so the baseline is established before any env re-resolution happens — callers that wire up a client directly keep the existing client untouched. Refs NousResearch#21130 (cherry picked from commit b694d21b7c4ff0330df6051e12dc8991f7ea10a6)

…t-exit) `OpenVikingMemoryProvider.shutdown()` joins in-flight writers, deferred-commit threads, and prefetch threads, but not `_runtime_start_thread` — the tracked `daemon=True` waiter that runs `_finish_runtime_openviking_start`, which blocks on network health probes (`_wait_for_openviking_health` polling + a `_VikingClient.health()` request). If the local OpenViking runtime is slow or unreachable, that waiter can still be blocked in network I/O at interpreter exit. CPython then forcibly kills it during `Py_FinalizeEx` (`PyThread_exit_thread` -> `__pthread_unwind` -> `abort()`), producing SIGABRT (exit 134) with no traceback — the same daemon- thread-at-exit failure class fixed for the Honcho provider. Fix: - `shutdown()` now joins `_runtime_start_thread` (timeout-bounded) alongside the other tracked threads. - `_wait_for_openviking_health()` gains a `should_stop` callback; the waiter passes `lambda: self._shutting_down` so the poll loop bails out promptly once `shutdown()` flips the flag, instead of lingering up to the 60s autostart timeout and timing out the join (which would leave the thread alive). - Add tests/plugins/memory/test_openviking_shutdown.py covering the short-circuit and the shutdown-joins-runtime-thread behaviour. (cherry picked from commit 5471ec70210b35462450aa52f0cd483439fffba7)

Add the AUTHOR_MAP entry required for the salvaged NousResearch#49832 OpenViking shutdown fix so contributor attribution CI can resolve the original author.

…ard-coding strings The _needs_trusted_identity_retry method was hard-coding specific server-side error strings to detect when a request failed due to missing X-OpenViking-Account / X-OpenViking-User headers. Each new server-side error variant required another string added to the client. Replace the string enumeration with a structural match: the error message mentions one of the tenant headers AND the HTTP status is 400. This covers all current error variants: - "Trusted mode requests must include X-OpenViking-Account and User" - "ROOT requests to tenant-scoped APIs must include X-OpenViking-Account" - "Trusted mode requests must include X-OpenViking-Account." - "Trusted mode requests must include X-OpenViking-User." The 400 status guard avoids false-positives on 403 errors such as "USER API keys cannot override X-OpenViking-User", which must not trigger a retry. All 176 existing tests pass. (cherry picked from commit 5a24d67)

Route refreshed unreachable local OpenViking configs through the existing runtime recovery path so /reload can attach to a locally starting server instead of disabling memory until restart. (cherry picked from commit 040e18a)

Avoid spawning multiple local OpenViking server processes while a runtime autostart waiter is already active. Remote endpoints still retry on later accesses because they do not install a local waiter.

teknium1 · 2026-07-15T02:07:40Z

Thanks for consolidating the OpenViking follow-ups. Static review confirms the reported premises on the current checkout: raw .env values are written at plugins/memory/openviking/__init__.py:950 and :956; initialize() snapshots the client configuration at :2133-2169 while handle_tool_call() only checks that cached client at :3299-3301; and the provider-tool gates omit disabled_toolsets in agent/memory_manager.py:82-115 and tools/mcp_tool.py:5376-5386.

The shutdown path also drains three worker families but not _runtime_start_thread at plugins/memory/openviking/__init__.py:3320-3341. The proposed locking, endpoint-bound waiter, and targeted regressions address those concrete paths without adding a new provider or a user-facing non-secret environment setting. No substantive correctness issue was identified in the static diff review. GitHub currently reports the PR mergeable.

Automated hermes-sweeper review.

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 Jun 24, 2026

tonydwb approved these changes Jun 24, 2026

View reviewed changes

ehz0ah force-pushed the fix/openviking-followups-after-recall branch 2 times, most recently from ef61ba4 to 441c4f6 Compare July 5, 2026 06:41

ehz0ah changed the title ~~fix(openviking): harden env reload and shutdown cleanup~~ fix(openviking): harden config auth and runtime shutdown Jul 5, 2026

ehz0ah force-pushed the fix/openviking-followups-after-recall branch 3 times, most recently from 53e0fab to 4e649d5 Compare July 7, 2026 07:17

ehz0ah force-pushed the fix/openviking-followups-after-recall branch from c8f66c7 to c60f60c Compare July 13, 2026 12:17

ehz0ah changed the title ~~fix(openviking): harden config auth and runtime shutdown~~ fix(openviking): harden tool gating, config auth, and runtime shutdown Jul 13, 2026

pprism13 and others added 15 commits July 14, 2026 12:38

chore(release): map koshaji contributor email

820cdf4

Add the AUTHOR_MAP entry required for the salvaged NousResearch#49832 OpenViking shutdown fix so contributor attribution CI can resolve the original author.

fix(openviking): sanitize splitline env separators

61c054f

test(openviking): keep root tenant errors out of trusted retry

be0c301

test(openviking): cover reconnect after startup health failure

b0f7e38

chore(release): map wgd753 contributor email

40977ba

fix(openviking): use readonly config loader

2695306

fix(memory): honor disabled toolsets for provider tools

8326db6

fix(openviking): start local runtime after reload

5290269

Route refreshed unreachable local OpenViking configs through the existing runtime recovery path so /reload can attach to a locally starting server instead of disabling memory until restart. (cherry picked from commit 040e18a)

fix(openviking): serialize local runtime recovery starts

e4d37d4

Avoid spawning multiple local OpenViking server processes while a runtime autostart waiter is already active. Remote endpoints still retry on later accesses because they do not install a local waiter.

fix(openviking): stop pending runtime start on shutdown

bf0d30e

fix(openviking): serialize client refresh state

6eb8676

ehz0ah force-pushed the fix/openviking-followups-after-recall branch from c60f60c to 6eb8676 Compare July 14, 2026 05:13

kshitijk4poor enabled auto-merge (rebase) July 22, 2026 08:50

kshitijk4poor disabled auto-merge July 22, 2026 09:10

kshitijk4poor mentioned this pull request Jul 22, 2026

fix(openviking): harden tool gating, config auth, and runtime shutdown (salvage #51952) #69221

Merged

ehz0ah closed this Jul 24, 2026

This was referenced Jul 30, 2026

[Bug] viking_remember writes to wrong URI path (viking://user/peers/ doesn't exist) #50701

Open

OpenViking plugin: 403 with API key auth + env vars not reloaded after /reload #21130

Open

Navigation Menu

Uh oh!

fix(openviking): harden tool gating, config auth, and runtime shutdown - #51952

fix(openviking): harden tool gating, config auth, and runtime shutdown#51952
ehz0ah wants to merge 15 commits into
NousResearch:mainfrom
ehz0ah:fix/openviking-followups-after-recall

ehz0ah commented Jun 24, 2026 •

edited

Loading

Uh oh!

tonydwb left a comment

Uh oh!

teknium1 commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Uh oh!

Conversation

ehz0ah commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Related Issue

Type of Change

Changes Made

How to Test

Checklist

Code

Documentation & Housekeeping

Uh oh!

tonydwb left a comment

Choose a reason for hiding this comment

Hermes Agent Review

Reviewed as part of batch review session 2026-06-24d.

Uh oh!

teknium1 commented Jul 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

ehz0ah commented Jun 24, 2026 •

edited

Loading