fix(openviking): harden lifecycle, endpoint safety, and dashboard config - #77747
Merged
kshitijk4poor merged 14 commits intoAug 3, 2026
Merged
kshitijk4poor merged 14 commits into
kshitijk4poor merged 14 commits into
Conversation
kshitijk4poor
enabled auto-merge (rebase)
August 3, 2026 13:52
This was referenced Aug 3, 2026
alt-glitch
added
type/bug
## Summary - Normalize OpenViking endpoints through `is_always_blocked_url` and fall back to the default local endpoint when poisoned. - Keep intentional loopback / LAN self-host working. - Add focused unit tests. ## Salvage / credit Memory-provider endpoint floor sibling of RetainDB/Supermemory always-blocked hardening (avoids over-broad NousResearch#4984-style private-IP bans). (cherry picked from commit 8fa607d)
(cherry picked from commit 389a90b)
`_start_local_openviking_server()` spawned `openviking-server` unconditionally. Both callers — `initialize()` and the runtime unreachable handler — reach it from a health probe, and that probe can time out client-side while the server is up and serving. The spawned process then loses the data-directory lock and exits immediately with `DataDirectoryLocked`; because the probe keeps timing out, the cycle repeats every cooldown window (~5 min observed). The existing 30s `_failed_refresh` cooldown paces the loop but cannot stop it, since it expires while the underlying condition persists. Probe the target host:port before spawning and treat an occupied port as already-started. This guards both call sites at their single convergence point. The probe deliberately tests only that a listener owns the port — enough to know a second server would lose the lock — and says nothing about that listener's health. The parse/probe now precedes the PATH lookup, so a reachable server is reported as running even when `openviking-server` is not on PATH. Fixes NousResearch#74846 (cherry picked from commit b49427d)
The provider used to disable OpenViking permanently when the server was unreachable. That was fixed: `_ensure_client()` now reconnects lazily, with a 30s cooldown gate in `_ensure_client_locked`. Only one of the seven user-facing warnings was updated to match. The other six still told the user memory was "disabled for this Hermes run", which is no longer true — every one of those paths is retried on the next access. A user who reads the old message has no reason to retry, which is very likely how NousResearch#5721 ("never recovers") came to be filed against behaviour that already recovers. All six sites were traced to confirm none is terminal for the run: the `initialize()`-time and waiter-thread failures never arm `_failed_refresh` (only line 2439 does), so they retry on the very next access with no cooldown at all. The replacement wording deliberately omits the "(after cooldown)" parenthetical used at the already-correct site — that detail is only accurate where `_failed_refresh` was just armed. The neutral phrasing is true at all six. Also promotes two clause separators to periods to avoid "…; …disabled;" collisions. (cherry picked from commit 8346403)
`_committed_session_ids` is a permanent per-sid latch, and `_session_needs_commit` checks it before the turn counter by design — a racing sync_turn can re-increment `_turn_count` after commit+reset, so the guard must win to stop a double-commit. That is correct for a session being left behind. It is wrong for one that keeps its id. `compress_context()` commits before rewriting the transcript in both modes, and with `compression.in_place: true` (the default) `on_session_switch` receives the same id and does not rotate. The latch then rejects every later commit for a still-live session — the next compression, /new, normal session end, startup recovery — so every post-compression turn is silently never extracted. Rotation mode is unaffected because a fresh child id is minted and starts clean, which is what confirms the latch's intent was only ever to dedupe the departing id. Clear the latch when compression completes without rotation. Turns arriving after that point are genuinely new, and this is a defined moment rather than a race. The rotation path is untouched, so the old id stays latched and its _finalize_session_async still dedupes against the compression commit. Fixes NousResearch#74695 (cherry picked from commit d1e5c3d)
Review feedback: the previous test called _mark_session_committed directly, so it verified the guard's behavior but not the wiring that sets it — a future break in the commit_memory_session -> same-id compression-boundary path would not be caught. Add a lifecycle regression that drives the real sequence: on_session_end commits through the actual path, on_session_switch(same id, reason="compression") crosses the boundary, sync_turn records a genuinely new turn, and a second on_session_end must produce a second commit POST. Without the fix it fails showing exactly one commit call, which is the reported data loss: every turn after the first compression is dropped. The rotation and /undo tests stay as scope guards. (cherry picked from commit 0ca5a33)
…nViking and RetainDB OpenViking is_available() only consulted env vars and use_ovcli_config, so an endpoint saved to config.yaml (e.g. by the Dashboard) reported needs_config; _resolve_connection_settings() likewise never folded config.yaml's non-secret fields into its chain. RetainDB initialize() read base_url/project from the environment only, ignoring the values the Dashboard writes to config.yaml. Both now resolve non-secret fields as env -> (ovcli ->) config.yaml -> default; secrets still come from the environment. Adds regression tests for both. Fixes NousResearch#68209 (cherry picked from commit dca57915b97b5705b30927a062e1d0f2f23d3841)
…s as fallback _recall_config() previously read all settings (recall_limit, score_threshold, recall_resources, etc.) exclusively from environment variables. This forced users to store behavioural configuration in .env, violating the Hermes convention that .env is for secrets only. The infrastructure to load config.yaml -> memory.openviking was already in place via _load_hermes_openviking_config(), but _recall_config() never called it. Fix: call _load_hermes_openviking_config() and pass its values as the default parameter to _env_int/_env_float/_env_bool. Env vars still override config.yaml values, preserving backward compatibility. Closes NousResearch#62540 (cherry picked from commit 6aadf12)
…HOME tests Add three tests to TestOpenVikingConfigSchema: 1. test_recall_config_reads_from_config_yaml — writes memory.openviking settings in config.yaml and verifies _recall_config() consumes them. 2. test_recall_config_env_overrides_config_yaml — writes both config.yaml and OPENVIKING_RECALL_* env vars, verifies env takes precedence. 3. test_recall_config_partial_config_yaml — partially populated config.yaml falls back to defaults for omitted keys. All 46 openviking_plugin tests pass (43 existing + 3 new). (cherry picked from commit b8d7834)
Review follow-up for salvaged PR NousResearch#76782. Three setup-wizard validation functions called _normalize_openviking_url outside their try/except blocks. Since _normalize_openviking_url now raises _OpenVikingEndpointError for blocked or malformed endpoints, an invalid endpoint would crash the wizard instead of returning a friendly (False, message) tuple. - _validate_openviking_auth: move _normalize_openviking_url inside try - _validate_openviking_root_access: same - _validate_openviking_setup_values: catch _OpenVikingEndpointError explicitly - Remove dead ternary in _normalize_openviking_url safety check (candidate always has http/https scheme by that point) - Replace redundant float('-inf') < x < float('inf') with math.isfinite() in _setting_float; drop the redundant infinity check from _setting_int (is_integer() already rejects inf/nan)
kshitijk4poor
force-pushed
the
salvage/openviking-hardening
branch
from
August 3, 2026 14:45
7e76987 to
6d395a3
Compare
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
7 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
Consolidates 6 open PRs into one coherent fix for the OpenViking memory provider — endpoint SSRF safety, anonymous identity probing before credentials, port-occupancy guard, accurate warning messages, in-place compression re-arm, config.yaml recall settings with typed Dashboard controls, and RetainDB config.yaml readback.
Changes
plugins/memory/openviking/__init__.py— endpoint safety (reject always-blocked URLs, validate syntax), anonymous identity probing before sending credentials, port-occupancy check before spawning server, stale "disabled for this Hermes run" warnings replaced with accurate "temporarily unavailable" messages, in-place compression re-arm via_clear_session_committed, config.yaml recall settings with typed_setting_int/_setting_float/_setting_boolmethods,save_configfor Dashboard persistenceplugins/memory/retaindb/__init__.py— reads non-secretbase_url/projectfrom config.yamlagent/memory_provider.py— ABC docstring updated withtype/minimum/maximum/stepschema fieldshermes_cli/web_server.py— numeric field coercion, range validation, and schema normalization for integer/number kindsweb/src/lib/api.ts,web/src/pages/PluginsPage.tsx— numeric input types with min/max/steptests/— 325+ tests covering anonymous headers, credential ordering, modern/legacy identity, retry probes, foreign listeners, port identity, invalid health payloads, bare loopback/autostart port agreement, blocked endpoints, invalid profile warnings, config precedence, typed Dashboard persistence, compression lifecyclecontributors/emails/— mappings for fatbigpig979@gmail.com (ddy4633) and jeff.mettel@gmail.com (jeff-mettel)Follow-up commit (review findings):
_normalize_openviking_urlcalls in_validate_openviking_auth,_validate_openviking_root_access, and_validate_openviking_setup_valueswith try/except — these called it outside their try blocks, so an invalid endpoint would crash the setup wizard instead of returning a friendly error_normalize_openviking_urlsafety check (candidate always has http/https scheme by that point)float("-inf") < x < float("inf")withmath.isfinite()in_setting_float; drop the redundant infinity check from_setting_int(is_integer()already rejects inf/nan)Source PRs (all authorship preserved via cherry-pick)
Closes #74846
Closes #74695
Closes #68209
Closes #62540
Related: #5721
Validation