Skip to content

feat(hermes-plugin): use OpenViking MCP tools - #5646

Open
ehz0ah wants to merge 22 commits into
volcengine:mainfrom
ehz0ah:feat/hermes-openviking-mcp
Open

ehz0ah wants to merge 22 commits into
volcengine:mainfrom
ehz0ah:feat/hermes-openviking-mcp

Conversation

@ehz0ah

@ehz0ah ehz0ah commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Replace the six provider-specific viking_* tools with OpenViking's standard MCP tools. The server owns tool schemas and results. Automatic recall, capture, commits and native-memory mirroring keep their existing memory-provider lifecycle.

New MCP connections use trust: untrusted. Read-only tools run without approval. Write-capable tools, including search, write, remember and forget, require approval and are refused in unattended runs. Setup preserves existing trust choices, including implicit full trust, plus filters, disabled entries, timeouts, gateway headers and TLS settings.

Requires Hermes v0.21.6 and OpenViking v0.4.23 or newer for the host approval fixes and server read-only annotations. Quick Local's server requirement starts at 0.4.23.

Review fixes

  • Handle null MCP configuration and report connection, authentication and TLS errors without exposing keys or response bodies.
  • Probe health anonymously. Redirects and unrelated success payloads receive no credentials, and redirects are never followed. Preserve hosted 401/403 authentication challenges.
  • Keep managed recovery anonymous on 401/403. Require successful OpenViking health before the ownership probe sends its root key.
  • Repair copied absolute MCP home paths when the memory provider initializes. Change only that entry's env.HERMES_HOME, preserving raw secret references and all other settings. Recognize Windows drive paths, Windows network paths and POSIX paths.
  • Warn once through the CLI warning callback, or logs/stderr when no callback is available. If saving fails, warn without claiming success or stopping automatic memory.
  • Update the capability references and remove the replaced native tool implementations and tests.

Accepted profile-repair limit

MCP discovery can precede memory initialization. A child already running with the copied source home can still use that profile until /reload-mcp or a Hermes restart. The warning explicitly tells users to reload before using OpenViking tools. Gateway operators must check the startup log and restart their gateway. Repair requires the OpenViking memory provider to load and configuration to be writable.

This is a saved-configuration repair, not a fail-closed runtime binding guarantee. The README documents this remaining window. Generic host-side profile binding is requested in NousResearch/hermes-agent#138512, with links to existing upstream work.

Validation

  • 483 tests passed on each of Hermes v0.21.6 and v0.21.7, with retries disabled. Fifteen added cases cover relocation, routed configuration scope, one-time notices, preserved settings, foreign path syntax, unchanged entries and failed or refused saves. The initial targeted run failed on the previous implementation.
  • Real Hermes CLI clone and export/import, actual external-plugin loading, real stdio MCP children and an OpenViking 0.5.0 server passed on both releases. Tool writes and automatic capture used the destination user's account. A new CLI launch passed for each relocated profile. Routed use from another launch profile also passed.
  • With MCP connected before initialization, live tests reproduced the documented old binding, confirmed the one-time repair notice, then verified that native MCP reload switched writes to the destination user. These tests do not claim the old connection is blocked.
  • Real install, enable and validate passed at 88a4e1c56; installed files match the committed plugin. The conflict-resolution merge at f0c614dc0 makes no changes to the tested plugin directory or install-check script.
  • At f0c614dc0, real Quick Local 0.4.23 recovery from a foreign 403 listener passed on Hermes v0.21.7. The listener received no credentials, 20 MCP tools were discovered, and find and health worked. One earlier cold attempt exceeded the existing discovery window. The repeat passed with the same defaults.
  • Syntax, targeted Ruff checks and git diff --check pass.

Earlier validation on this branch also covered interactive setup, read-only calls without approval, an approved write, a denied delete, unattended write refusal, and conversation capture through real remote-model extraction and recall. Those were not repeated for this saved-path change. The latest relocation checks exercise capture and real MCP writes, not a new extraction-quality test.

Local live tests ran on macOS arm64. Gateway approval UI was not exercised. Cross-platform runtime checks are provided by CI. Quick Local keeps its existing release range and documented embedding concurrency limitation. Cold startup can exceed the MCP discovery window and require /reload-mcp.

@ZaynJarvis ZaynJarvis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: #5646 — feat(hermes-plugin): use OpenViking MCP tools

Head reviewed: 9d3fee48bd0fa3614a75a67b367258be3e544cd4 · Base: c5b5229a (current main)

Verdict: Request changes

The core adapter works. I tested it against a real OpenViking server running current main source and a real Hermes with the plugin installed through hermes plugins install and enabled.

What works:

  • Tool schemas are forwarded byte-for-byte: 20 of 20 tools, 0 diffs.
  • Server errors pass through verbatim.
  • Concurrent calls and 300 KB payloads frame correctly over stdio.
  • A timed-out write is sent exactly once.
  • Linked-profile and .env endpoint changes are followed without a restart.
  • Two-profile isolation holds.
  • Memories written through MCP are recalled automatically.
  • Capture filtering excludes MCP read results.
  • Setup preserves the existing MCP policy.

Four issues block it:

  1. The PR's own test fails deterministically on Linux, so CI is red.
  2. A private CA (SSL_CERT_FILE) breaks the MCP tools at runtime even though setup validates successfully.
  3. If OpenViking is unreachable when Hermes starts, the tools disappear for the life of the process.
  4. forget widens from "one of your own memory files" to "recursive delete of anything the key can write", with no approval gate by default.

Blocking

B1. hermes-plugin-tests is red on Linux: test_timed_out_write_is_not_replayed fails every time.

  • Location: tests/test_mcp_tools.py:305-323. The root cause is in mcp_tools.py:148-150 and :178.
  • The HTTP client timeout and the MCP read_timeout_seconds are the same value, so the two timers race. On Linux the first leaf is MCPError(-32001, "Request 'tools/call' timed out"), not httpx2.TimeoutException:
    E  where False = isinstance(MCPError(-32001, "Request 'tools/call' timed out", None), <class 'httpx2.TimeoutException'>)
    FAILED test_mcp_tools.py::test_timed_out_write_is_not_replayed
    
  • CI job 111725982330: 13 files, 392 tests passed, 1 failed.
  • Locally it failed on the CI pin 5bba024d (392/1) and on Hermes v2026.9.24 (389/1, 3 skipped). Running the test alone 8 times gave 8/8 1 failed.
  • The no-replay behavior itself is correct. Only one request was sent, and live evidence below confirms it. But the test asserts on a racy type, and the user-facing error name (MCPError vs ReadTimeout) is also nondeterministic.
  • The PR body's "393 passed" does not hold on Linux.
  • Suggested fix: make one timer clearly authoritative, for example read_timeout_seconds = timeout + margin. Then assert "exactly one tools/call and an error", not a specific class.

B2. Private-CA TLS works for setup and automatic memory but breaks the MCP tools.

  • Location: mcp_tools.py:44-65 (configure env) and :159-164.
  • Hermes spawns stdio servers with a filtered environment. _build_safe_env keeps only PATH, HOME, LANG, … plus XDG_*. So SSL_CERT_FILE and SSL_CERT_DIR never reach the adapter.
  • The provider runs inside the Hermes process and the setup validation runs in the setup process, so both see the CA. Setup therefore reports success, and then every runtime discovery fails.

Repro: a TLS proxy with a private CA in front of OpenViking (https://127.0.0.1:19463), and SSL_CERT_FILE=ca.pem exported for Hermes.

Step Observed
validate_connection (setup path), SSL_CERT_FILE set (True, '')
Real hermes chat -q …: provider Memory provider 'openviking' activated, session committed over TLS
Same run: MCP MCP server 'openviking' failed initial connection after 3 attempts, parking … OpenViking MCP discovery failed (ConnectError), and the model reports NO_OV_TOOLS
Adapter alone, Hermes-filtered env isError: true, OpenViking MCP request failed (ConnectError)
Adapter alone + SSL_CERT_FILE in env healthy
Adapter alone + mcp_servers.openviking.ssl_verify: ca.pem healthy
  • The failure is also mislabelled. A certificate-verify failure surfaces as ConnectError, and setup's wording for the same failure is "Use a server with /mcp enabled".
  • The same filtering applies to HTTPS_PROXY, HTTP_PROXY and NO_PROXY; see the env list in probes/proc-env.out. I did not test proxies separately.
  • Suggested fix: add ${SSL_CERT_FILE}, ${SSL_CERT_DIR} and the proxy variables to the env that configure writes. Alternatively, run setup's MCP check in the filtered env. Report TLS errors distinctly.

B3. If the server is unreachable when Hermes starts, the OpenViking tools are gone for the whole process.

  • Location: mcp_tools.py:128-133. For non-managed (Custom or remote) servers, discover raises immediately on ConnectError.
  • Hermes makes 3 attempts, retries once on the first turn, then parks the server.
  • With the native viking_* tools, the tools stayed registered and started working once the server was back.

Repro (probes/cold_server_cli.py, real hermes chat --cli):

  1. Stop OpenViking.
  2. Start Hermes and send turn 1.
  3. Start OpenViking and wait 75 s.
  4. Send turn 2, asking the model to tool_search for openviking health.

Observed in install-bob/.../logs/agent.log:

19:17:54 MCP server 'openviking' failed initial connection after 3 attempts, parking until a reconnect is requested
19:18:03 Background MCP discovery previously exited with no connected servers; retrying discovery thread
(no further tools/list after 19:18:18; OV healthy from 19:19:10)
19:20:26 conversation turn … → model: no mcp__openviking__health available
  • Setting Hermes's lazy: true did not change this (probes/cold-server-bob-lazy.log).
  • /reload-mcp does recover: 19:29:26 MCP server 'openviking' (stdio): registered 20 tool(s) (probes/cold-server-bob-reload.log).
  • For a gateway that starts before OpenViking (boot ordering), every chat loses the tools until someone notices.
  • README:340-342 covers only the Quick Local first turn, and recommends a restart.
  • Suggested fix: have list_tools serve the last successful tool list, cached per profile, when the server is unreachable, so calls fail per call until it returns. Alternatively, wait on ConnectError for every deployment, not only managed ones. At minimum, document /reload-mcp for both cases.

B4. forget now allows recursive deletes of shared data with no approval by default.

  • Base viking_forget (base __init__.py:556-587, README "intentionally narrow") allowed only one concrete .md file in the caller's own memory space. It rejected directories, resources, skills and generated summaries.
  • Head forwards MCP forget(uri, recursive) unchanged.
  • configure leaves Hermes trust at its default, full, so write-capable calls need no approval.
  • This matters because automatic recall injects stored content into the prompt, and stored content is untrusted.

Repro through the adapter (probes/forget_scope_probe.py). carol is a plain user in alice's account:

alice  write viking://resources/team/shared-{a,b}.md        -> Wrote …
carol  forget viking://resources/team recursive=true         -> Deleted: viking://resources/team
alice  list viking://resources/team                          -> ERR NOT_FOUND: Directory not found
alice  forget viking://user/alice/memories/.overview.md      -> Deleted: …/.overview.md
alice  forget viking://user/alice/memories/entities recursive=true -> Deleted: …/entities
  • The PR documents this as intended. I still think it needs an explicit decision before merge rather than a README line. Options:
    • setup writes a safe default, for example tools.exclude: [forget] or an approval requirement for forget;
    • or the adapter keeps the old forget policy for this one tool.

Non-blocking

  • N1. The README is wrong about add_resource (README:352-354). On main (mcp_endpoint.py:1628-1689), a local path is never read from the server's filesystem. The server returns a signed temp_upload URL.
    • Verified: the adapter returned Local file detected — upload it…, then curl -F file=@… to that URL returned status: success, root_uri: viking://user/alice/resources/hermes-host-only (probes/add-resource.out).
    • Hermes-host files can therefore still be uploaded when the agent has a shell.
    • What is lost is base's automatic upload and directory zipping.
  • N2. Misleading error text.
    • A refused connection on a read-only health call says "A write may have reached the server" (mcp_tools.py:233-234).
    • A stale ssl_verify path during re-setup reports MCP is unavailable (FileNotFoundError). Use a server with /mcp enabled, then loops on Retry (live/runs/10-resetup-bob.log).
    • Discovery failures return JSON-RPC error code: 0.
  • N3. Behavior changes inherited from Hermes MCP policy (not documented).
    • Three consecutive tool errors, for example NOT_FOUND on guessed URIs, pause all OpenViking tools: rejected the last 3 calls … Paused for ~58s (live/runs/11-breaker.jsonl).
    • The tools are deferred: first use took tool_search → tool_describe → call.
    • MCP search has readOnlyHint: false, so on trust: untrusted it needs approval and is never replayed.
  • N4. Setup now hard-requires /mcp, even when the user has enabled: false. The validation runs before save (_setup.py:236,288,383). Automatic memory does not need MCP.
  • N5. The migration notice goes to stdout. In --format stream-json it is a non-JSON line inside the JSONL stream (__init__.py:2875; live/runs/01-nosetup-alice.jsonl line 2).
  • N6. Each tool call is 4 requests on a fresh client: status probe, initialize, notifications/initialized, tools/call (probes/delay-proxy.log). That costs 30 ms on loopback. Against a remote TLS server it adds a handshake and several RTTs per call.
  • N7. The API key is copied into the adapter's environment even though the adapter re-reads the profile .env itself.
    • OPENVIKING_API_KEY is set in /proc/<adapter>/environ, readable by the same user only. argv is clean.
    • No key (user, root) appeared in any of the 45 Hermes, adapter or server logs and outputs I scanned.
  • N8. Capability mapping. The table below is from main mcp_endpoint.py, plus live calls.
Removed tool Replacement Lost or changed
viking_search (mode fast/deep, scope) find (read-only) and search (destructive annotation) scope → target_uri; deep mode → search
viking_read (level abstract/overview/full, ≤3 URIs) read(uris, offset, limit) No abstract/overview level for a file
viking_browse (list/tree/stat) list, tree No stat
viking_remember(content) remember(messages[]) No session or task ID in the result; commits inline
viking_forget (own memory .md only) forget(uri, recursive) Widened; see B4
viking_add_resource (uploads local files and dirs) add_resource Agent must POST to the upload URL; no directory zip (N1)

What was verified

All runs used isolated HOME, HERMES_HOME and OpenViking config, with OPENVIKING_CLI_CONFIG_FILE unset. The server was current main OpenViking source overlaid on the 0.4.22 native libraries (ovmain/, port 19461). Hermes was the CI pin 5bba024d with the bundled openviking provider removed; the plugin was installed and enabled by scripts/check-hermes-plugin-install.py, and the model was Doubao via Ark. Evidence paths are relative to tmp/review-5646/.

Check Result Evidence
Full suite, CI pin, base vs head 383 passed vs 392 passed / 1 failed suite-base-pin.log, suite-head-pin.log
Full suite, Hermes v2026.9.24 380 + 3 skipped vs 389 / 1 failed / 3 skipped suite-base-rel.log, suite-head-rel.log
CI on head 16 success, 3 skipped, 1 failure (hermes-plugin-tests), 1 build still running. All 5 quick-local-platforms jobs green (mcp_discovery_and_recovery: true). checks.json, ci-hermes.log, ci-ql-ubuntu-main.log
Real install, enable, validate, hermes openviking mcp --help pass, for 2 homes install-check.log, install-bob.log, install-check/validate.log
Interactive setup (Custom + user key; linked and .env-only) MCP validated; entry written live/runs/02-setup-{alice,bob}.log
Setup preserves enabled:false, timeout, tools, ssl_verify, trust, custom headers; strips Authorization pass live/runs/10-resetup-bob.log
Upgrade without setup: notice shown; old viking_search call Hermes returns "not a known tool name", graceful live/runs/01-nosetup-alice.jsonl
Hermes → MCP remember / find / read pass; extracted to entities/animals/pixel.md live/runs/03-*.jsonl, 04-*.jsonl
Automatic recall of an MCP-written memory; two-profile isolation alice answers Pixel/teal; bob UNKNOWN; bob's read of alice's URI → PERMISSION_DENIED live/runs/05-*, 06-*
Schema passthrough vs raw server tools/list 20/20 identical probes/stdio-alice-basic.out
Errors (unknown tool, bad args, NOT_FOUND), 12 concurrent calls, 300 KB round-trip verbatim, all OK, byte-equal probes/stdio-alice-basic.out
Server down / restart / endpoint change (linked and .env) while the adapter runs fails fast; recovers; follows new port; restores probes/lifecycle-{alice,bob}.out
Timeout through a real Hermes (5 s timeout, 12 s server delay) exactly 1 tools/call, no replay live/runs/09-timeout.jsonl, probes/delay-proxy.log
Capture filter (direct and tool_call-wrapped) find/read results not in the OV session; remember result is captured live/runs/sess-*.json
Private CA, cold-start server, forget scope, add_resource see B2, B3, B4, N1 probes/*
ruff on new files; git diff --check clean (2 pre-existing _setup.py findings also on base) —
~/.openviking/ovcli.conf.zayn{,.volc} sha256 unchanged —

Not tested

  • Quick Local live cold start through Hermes stdio discovery. I relied on the 5 green quick-local-platforms jobs, which call Connection.request directly, not Hermes discovery.
  • Trusted-mode (X-OpenViking-Account/User retry) against a real server; it is covered only by the fake-server unit test.
  • Live end-to-end on Hermes v2026.9.24 (suite only), and on Windows or macOS.
  • HTTPS_PROXY specifically; I inferred it from the same env filtering as B2.
  • A packaged OpenViking build of main. The server reports version 0.4.22 because it ran main source on the 0.4.22 native libraries.

Review by Zayn (agent) at 9d3fee4.

@ehz0ah
ehz0ah marked this pull request as ready for review October 5, 2026 13:00

@ZaynJarvis ZaynJarvis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: #5646 at 7c6741d9 ("fix(hermes): preserve TLS settings and gate MCP writes")

Head reviewed: 7c6741d95c55808083941cc9abcc8cad1d1f07fa · Previous review: 9d3fee48 · Base: c5b5229a

Verdict: Request changes

The commit fixes B1 and B2, partially fixes B3, and fixes most of the non-blocking items. B4 is addressed by having setup default to trust: untrusted, but that default makes the OpenViking MCP tools unusable in the default Hermes CLI and in one-shot (-q) runs. On both supported Hermes versions, Hermes treats every OpenViking tool as write-capable, reads included. Its classic CLI and -q paths then refuse the approval prompt. See New-1.

Item Status Evidence (paths in tmp/review-5646/)
B1 Timeout test red on Linux Fixed One outer deadline replaces the racing timers. CI hermes-plugin-tests: 404 passed, 0 failed. Local runs: CI pin 404 passed; v2026.9.24 401 passed, 3 skipped; the test alone passed 8 of 8. Live: one tools/call, failed after 3.02 s with "The request timed out", and was not retried (probes/timeout-v2.out, probes/delay-proxy-v2.log).
B2 SSL_CERT_FILE dropped Fixed Same private-CA repro, real hermes chat -q: registered 20 tool(s) over TLS, 12 handshakes OK. Without the CA, the adapter returns a clear message, "TLS verification failed. Check SSL_CERT_FILE, SSL_CERT_DIR or ssl_verify", and does not wait (probes/tls-v2.out, live/runs/v2-tls-bob.jsonl). Proxy variables are now forwarded too; I did not test proxies live.
B3 Server down at startup Partially fixed Discovery now waits up to connect_timeout for every deployment. Server up 27 s after Hermes start: registered 20 tool(s) 1 s later (probes/cold-server-early-bob.log). Server up 90 s after start: Hermes cancels discovery at 60 s (Failed to connect … CancelledError), never retries, and turn 2 has no tools (probes/v2-cold-server-bob.log). The README now documents /reload-mcp. The adapter's own hint never reaches the user here, because Hermes's timeout fires first and logs only CancelledError.
B4 Recursive forget, no approval Addressed, but see New-1 configure() sets trust: untrusted with setdefault (mcp_tools.py:121), so an explicit trust: full is kept. forget now needs approval, but so does every other tool.
N1 add_resource docs Fixed The README now describes the upload URL.
N2 Misleading errors Fixed Distinct TLS, missing-file, timeout and connect hints. JSON-RPC code is -32603.
N3 Breaker and deferral undocumented Fixed Both are now in the README.
N4 Setup requires /mcp when enabled: false Fixed validate_connection skips the check for disabled entries; covered by a unit test.
N5 Notice breaks stream-json Fixed, with a regression See New-2.
N6 4 requests per call Open Unchanged.
N7 API key in adapter env Open (minor) Unchanged.
CI on 7c6741d9 16 success, 3 skipped, 2 builds still running hermes-plugin-tests and all 5 quick-local-platforms jobs are green; API & CLI tests passed.

New issues

New-1 (blocking): with the new default, every OpenViking tool, reads included, needs per-call approval, and the default CLI and -q cannot grant it.

The cause is in Hermes, not this PR, but the new default exposes it:

  • tools/mcp_tool_registration.py:48-52 reads annotations.readOnlyHint. This is identical on CI pin 5bba024d and on v2026.9.24.
  • MCP SDK 2.0.0 names the field read_only_hint: types.ToolAnnotations.model_fields gives ['title', 'read_only_hint', 'destructive_hint', 'idempotent_hint', 'open_world_hint'].
  • So Hermes classifies every tool as write-capable, and trust: untrusted gates all 20.
  • The README (README.md:347-349) says "Some Hermes versions also prompt for reads". In fact both supported versions do.

Live results on the CI pin, after a fresh hermes memory setup openviking that wrote trust: untrusted:

Surface find, read (read-only) search, remember Evidence
hermes chat -q The user did not approve running write-capable MCP tool 'find' … NOT run same live/runs/v2-gate-noyolo.jsonl
hermes chat -q --yolo same denial — live/runs/v2-gate-yolo.jsonl
Interactive hermes chat (classic CLI, the default display.interface) Dangerous-command approval requested on a thread with no approval callback while prompt_toolkit is active; failing closed → user cancelled 'find' — install-check/.../agent.log 21:06:08, probes/v2-cli-approval-explore.log
hermes chat --tui Prompt shown. Enter approves. find completed (41.01s) per call probes/v2-tui-approval2.log, agent.log 21:10:02

Additional points:

  • No "always allow" option exists: Hermes calls the prompt with allow_permanent=False (tools/approval_prompt.py:331). So in the TUI every call, including each find or read, needs a click.
  • Cron and -q workers fail closed by design. I did not test the gateway; by code it routes each call to the chat for approval.
  • Net effect: under the default setup, explicit OpenViking tools are unusable in the default CLI, -q and cron, and need one approval per read in the TUI.

Suggested fix: keep trust: full, which is Hermes's default, and gate only the destructive tools explicitly. For example, have setup write tools.exclude: [forget] and document how to re-enable it, or put forget behind the adapter's old URI policy. Adopt the global untrusted gate only once Hermes reads read_only_hint with SDK 2.

New-2 (non-blocking): users who upgrade without running setup no longer see the migration notice.

  • The notice moved from the runtime callback to logger.warning (__init__.py:2874).
  • I removed mcp_servers from a profile and ran hermes chat -q "Reply with OK.": the notice appeared 0 times on stdout and stderr, and only in logs/agent.log and logs/errors.log (live/runs/v2-notice.*).
  • The explicit tools then vanish silently.
  • Suggested fix: keep the notice on stderr, or in the CLI banner or status, and keep it off the stream-json stdout.

New-3 (minor): in the TUI, Enter accepts the per-call approval. I observed this live. Worth knowing for forget, which is the one call the gate exists for.

Default for existing users

  • Upgrading from 2.1.0: there is no MCP entry, so no tools appear, and with New-2 nothing tells the user why. After running setup, the entry gets trust: untrusted, which leads to New-1.
  • A pre-existing entry without trust also becomes untrusted.
  • An explicit trust: full or untrusted is preserved (unit test test_setup_preserves_policy…, plus the v1 live check).

Re-verified this round

  • Plugin upgraded in place with a real hermes plugins install … --force --ref 7c6741d9 on both profiles (live/upgrade-*.log).
  • Setup re-run through both the linked-profile and .env-only paths. Each wrote the CA and proxy env references and trust: untrusted, and printed "Hermes will use its approval prompts for MCP tool calls." (live/runs/v2-setup-*.log)
  • ruff is clean on mcp_tools.py and its tests. git diff --check is clean.
  • ~/.openviking/ovcli.conf.zayn{,.volc} sha256 is unchanged at the start and end.

Not tested

  • Gateway (messaging) approval flow, and cron.
  • Hermes main or any version that might read read_only_hint. I checked only the CI pin, live, and v2026.9.24, by code and suite.
  • Live proxy forwarding (HTTPS_PROXY), and Quick Local cold start through Hermes. For the latter I relied on the green quick-local-platforms jobs.
  • A packaged OpenViking main build. As before, I ran main source on the 0.4.22 native libraries.

Re-review by Zayn (agent) at 7c6741d.

@ZaynJarvis ZaynJarvis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: #5646 at faf3e01e ("fix(hermes): show MCP upgrade notice and document host requirements")

Head reviewed: faf3e01e8c5c22fa1dbb6f548652dd701e5eac31 · Previous review: 7c6741d9 · Base: c5b5229a

Verdict: Request changes

This commit changes only docs and the upgrade notice. The notice is now visible in the classic CLI and in -q without polluting stdout. The new host-compatibility docs are accurate; I checked every Hermes claim against the API.

The PR still ships trust: untrusted as the setup default, so New-1 is unchanged. On every Hermes release available today, that default does the following:

  • It blocks all OpenViking MCP tools, reads included, in the default classic CLI, in -q and in cron.
  • It leaves the TUI as the only usable surface, with one approval per call.

The PR's own DEVELOPMENT.md says this default should not be released until a Hermes build with both fixes has been validated. Neither fix is in any Hermes release. Documenting the problem is not the same as shipping a usable default.

Status of earlier items

Item Status at faf3e01e Evidence (paths in tmp/review-5646/)
B1 Timeout test Fixed CI hermes-plugin-tests green. Locally: CI pin 405 passed; v2026.9.24 402 passed, 3 skipped (suite-head3-*.log).
B2 SSL_CERT_FILE Fixed (unchanged since v2) Adapter code unchanged since 7c6741d9; v2 evidence stands.
B3 Server down at startup Partially fixed (unchanged) The server is found if it starts within connect_timeout. Otherwise use /reload-mcp, which is documented. Same as v2.
B4 Recursive forget with no approval Addressed by the trust: untrusted default Same as v2; this default is the cause of New-1.
New-1 The default gates every tool; classic CLI, -q and cron cannot approve Still open (blocking). Docs are now accurate; behavior is unchanged See below.
New-2 Upgrade notice only in logs Fixed for the CLI and -q; not visible in the TUI See New-4.
New-3 Enter approves in the TUI Open (minor) Unchanged.
N1–N5 Fixed As in v2.
N6 4 requests per call; N7 key in adapter env Open (minor) Unchanged.
CI on faf3e01e 18 success, 3 skipped, 0 failed checks-v3.json. Includes hermes-plugin-tests, all 5 quick-local-platforms jobs, API & CLI tests, and builds.

New-1 still blocks: the default is unusable on every Hermes release available today

What the default is. configure() still sets trust: untrusted with setdefault (mcp_tools.py:121, unchanged). After a fresh setup, the alice profile has trust: untrusted.

What happens today. The run below is live on the CI pin 5bba024d with the plugin installed at faf3e01e. Calls: find (read-only) and search.

Config find search Evidence
Default (untrusted), hermes chat -q The user did not approve running write-capable MCP tool 'find' … NOT run same denial live/runs/v3-default-q.jsonl
Documented workaround: trust: full Found 1 item(s) Found 1 item(s) live/runs/v3-full-q.jsonl

The classic CLI fails closed with "no approval callback while prompt_toolkit is active" (v2, probes/v2-cli-approval-explore.log). The TUI prompts once per call (v2, probes/v2-tui-approval2.log). The code involved is unchanged in this commit.

Are the new docs honest? Yes. I verified each claim through the GitHub API:

  • Hermes main 80309111 still reads only readOnlyHint (tools/mcp_tool_registration.py:52 at that SHA). The CI pin and v2026.9.24 do the same.
  • MCP SDK 2.0.0 exposes the field as read_only_hint.
  • The two Hermes fixes are both open and unmerged: #130391 ("read the MCP read-only hint the way the installed SDK exposes it"; this is the one the docs cite) and #111270 ("read readOnlyHint from mcp 2.x snake_case annotations"; this is the one the coordinator mentioned).
  • ef1faa4c ("classic-CLI MCP/vault consent prompts reach the approval panel") is an ancestor of main 80309111 (130 commits behind it). It is not in v2026.9.24 (f97608f is 7122 commits behind ef1faa4c) and not in the CI pin. v2026.9.24 is still the latest release.
  • So no Hermes release has either fix, and Hermes main has only the CLI one.

Why the docs are not enough:

  1. A user on any released Hermes who follows the upgrade path ends up with a configuration where the default CLI and every unattended run cannot use any OpenViking tool. That includes the read-only tools that the plugin's own system prompt tells the model to use (__init__.py:1704-1707: "use their search and read tools").
  2. The way out is to hand-edit config.yaml to trust: full. That removes the approval gate entirely, including for forget, so the safety B4 asked for is gone again.
  3. DEVELOPMENT.md (new, "MCP host compatibility") says: "Before releasing this MCP migration with the default approval policy, validate a Hermes build with both fixes … Update the tested host pin and baseline from that evidence." Merging to main makes 2.2.0 installable today, because the README installs by commit SHA and DEVELOPMENT plans the catalog cutover. The precondition the PR sets for itself is not met.
  4. The problem does not fully go away even after #130391 or #111270 lands. On OpenViking main, MCP search is annotated _DESTRUCTIVE_TOOL_ANNOTATIONS, which sets readOnlyHint=False (openviking/server/mcp_endpoint.py:508-509). So every search will still prompt under untrusted, while the system prompt steers the model toward search.

Suggested fix (either is fine):

  • (a) Default to trust: full and gate only destructive operations explicitly, for example tools.exclude: [forget] with a documented opt-in, or the old own-memory-file policy for forget in the adapter. Revisit untrusted once a Hermes release reads read_only_hint.
  • (b) Hold the merge until such a Hermes release exists, and pin CI to it.

Under either option, point the system prompt at find and read rather than search.

New issues

New-4 (non-blocking): the upgrade notice is invisible in the TUI, which is the surface the README now recommends.

  • print(..., file=sys.stderr) (__init__.py:2882) shows up in the classic CLI and in -q, but not on the TUI screen.
  • Live, with a profile that has no mcp_servers.openviking:
Surface Notice visible? Evidence
-q text stderr 1, stdout 0 live/runs/v3-notice-q.*
-q --format stream-json stderr 1, stdout 0, 0 non-JSON stdout lines live/runs/v3-notice-sj.*
Classic CLI (pty) 1 probes/v3-notice--cli.log
--tui (pty) 0; only in logs/agent.log and errors.log (00:22:59), while the turn ran (platform=tui) probes/v3-notice--tui.log
  • I did not test the gateway. By code, the notice goes to the service's stderr, not to chat users.

Re-verified this round

  • Delta reviewed: faf3e01e touches only README.md, DEVELOPMENT.md, the notice in __init__.py, and tests (+58/−4). The adapter and setup code are unchanged.
  • Plugin upgraded in place with a real hermes plugins install … --ref faf3e01e --force on both profiles (live/upgrade3-*.log).
  • ~/.openviking/ovcli.conf.zayn{,.volc} sha256 is unchanged at the start and end.

Not tested

  • Gateway (messaging) approval and notice, and cron.
  • Any Hermes build carrying #130391 or #111270 together with ef1faa4c. None is released; I verified the Hermes main claims by API only, not live.
  • Classic CLI on v2026.9.24, live. I checked it only by code: the same readOnlyHint lookup and the same fail-closed CLI branch.
  • Proxy forwarding live, and Quick Local cold start through Hermes.

Re-review by Zayn (agent) at faf3e01.

@ehz0ah
ehz0ah marked this pull request as draft October 5, 2026 17:35
@ehz0ah
ehz0ah marked this pull request as ready for review October 14, 2026 17:35

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

2 participants