fix(openviking): harden tool gating, config auth, and runtime shutdown (salvage #51952) - #69221
Merged
Merged
Conversation
`_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.
… atomically Follow-up hardening on the salvaged _ensure_client() (NousResearch#21130 fix): - Failed-config cooldown: after a refresh attempt fails for a given resolved config, skip re-probing for 30s. Previously every provider access against a down endpoint paid a 3s health probe under _client_refresh_lock and emitted a warning (2+ per turn, some on user-facing threads: prefetch, tool calls, session end). Retries still happen after the cooldown or immediately when config changes, and the log message now says so instead of the false 'disabled until config changes'. - Atomic connection snapshot: _conn_snapshot (5-tuple, single assignment) is published only after a health check passes. _new_client() and on_memory_write's writer read it as one load, so background writers can no longer observe a torn mix of old/new identity fields mid-refresh or target an endpoint that never passed health. Field writes in _ensure_client_locked keep tracking the attempted config for the unchanged-config dedupe. - _env_refresh_enabled moves to the top of initialize(): an exception mid-initialize (swallowed by MemoryManager) can no longer leave the provider silently stuck in never-refresh mode. - _search_prefetch_context reuses _new_client() and degrades to '' on construction failure instead of propagating. Mutation-checked: neutering the cooldown or publishing the snapshot on failed health makes the new regression tests fail.
Widens the salvaged .env injection fix (NousResearch#50315) to the sibling site it missed: hermes_cli/memory_setup.py::_write_env_vars is the near-identical core writer the openviking plugin's copy was forked from, is fed directly by interactive _prompt() (pasted API keys), and is reused by other memory plugins (e.g. supermemory imports it). A pasted secret with an embedded CR/LF injected an arbitrary extra KEY=VALUE line on the next read. Same _env_line_safe() treatment as the plugin writer (strip every str.splitlines() separator + NUL), matching config.save_env_value's existing newline strip. Mutation-checked: reverting the sanitizer makes the new regression tests fail.
alt-glitch
added
type/bug
This was referenced Jul 26, 2026
egilewski
added a commit
to egilewski/hermes-agent
that referenced
this pull request
Aug 19, 2026
PR NousResearch#69221 fixed the reported `disabled_toolsets=["memory"]` provider-tool bypass, but the shared injection gate still recognized only that literal. The valid `all` and `*` aliases removed registry tools and then allowed memory providers to re-add their schemas after filtering. Treat both global aliases as memory-provider denials in the gate shared by initial injection and MCP refresh. Explicit memory denial and enabled-provider behavior remain unchanged. Fixes NousResearch#49386 Related NousResearch#46171
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Re-lands #51952 (OpenViking hardening consolidation by @ehz0ah) onto current
mainafter the session-context/orphan-recovery chain (#58871 salvage and follow-ups) landed mid-flight and made the original branch conflict, plus two review follow-up commits that harden degraded-mode behavior and widen the.envinjection fix to the core writer it was forked from.All 15 original commits are cherry-picked with authorship preserved:
.envCR/LF/NUL sanitization at both plugin write sites_ensure_client(): refresh the live client from env after/reload(OpenViking plugin: 403 with API key auth + env vars not reloaded after /reload #21130)disabled_toolsets: ["memory"]blocks external memory-provider tools in initial injection and MCP snapshot refreshConflict resolution vs current main
__init__state: kept main's run-lock/pending-session fields alongside the PR's_env_refresh_enabledand_client_refresh_lock.prefetch(): main's session-start memory context path kept; gate is now_ensure_client()(the PR's intent) instead of the raw_clientcheck._recover_pending_sessions()call preserved on successful attach, run outside the refresh lock.Review follow-ups (2 commits by @kshitijk4poor)
fix(openviking): failed-config retry cooldown (30s) — a down endpoint no longer costs a 3s health probe under_client_refresh_lockplus a warning on every provider access (prefetch, sync_turn, tool calls, session end); retries resume after cooldown or immediately on config change, and the log message now says so. Atomic_conn_snapshot(published only after a passing health check) so lock-free background writers (_new_client,on_memory_write) can't observe a torn old/new identity mix or target an endpoint that never passed health._env_refresh_enabledset at the top ofinitialize()so a swallowed mid-init exception can't leave the provider stuck in never-refresh mode._search_prefetch_contextreuses_new_client()and degrades to""on construction failure.fix(memory-setup): the sibling.envinjection gap —hermes_cli/memory_setup.py::_write_env_vars(the core writer the plugin copy was forked from; fed by interactive pasted API keys; reused by other memory plugins) gets the same_env_line_safe()sanitization, matchingconfig.save_env_value's existing newline strip.Both follow-ups are mutation-checked: neutering the cooldown, publishing the snapshot on failed health, or reverting the core sanitizer makes the new regression tests fail.
Related Issue
Refs #21130
Supersedes #51952 (branch conflicted after the recent OpenViking session-context chain landed). Originals #50315, #21138, #49832, #21136, #59454, #18166 already closed with credit.
Validation
scripts/run_tests.sh(6 targeted files)_ensure_clientrefresh, real-thread shutdown join, disabled-toolset gate truth table, subprocess SIGABRT regressionscripts/check-windows-footguns.pygit diff --check