Skip to content

fix(mcp): scope Hermes MCP status reconciliation to the queried server - #11119

Open
spaghiari wants to merge 1 commit into
NVIDIA:mainfrom
spaghiari:fix/hermes-mcp-status-reconciliation-scoping
Open

fix(mcp): scope Hermes MCP status reconciliation to the queried server#11119
spaghiari wants to merge 1 commit into
NVIDIA:mainfrom
spaghiari:fix/hermes-mcp-status-reconciliation-scoping

Conversation

@spaghiari

@spaghiari spaghiari commented Sep 6, 2026

Copy link
Copy Markdown

Fixes #11118

fix(mcp): scope Hermes MCP status reconciliation to the queried server

Summary

nemoclaw <sandbox> mcp status <server> on a Hermes sandbox with two or
more managed MCP servers can report a perfectly healthy, correctly-configured
server as unregistered ("Hermes MCP config does not match persisted managed intent"), even though its entry in /sandbox/.hermes/config.yaml is valid
and the Hermes gateway serves it correctly. Restarting the "broken" server
appears to fix it, but breaks whichever sibling server was last queried —
querying/restarting servers becomes a permanent game of musical chairs where
exactly one managed server appears healthy at a time.

Verified against a live two-server Hermes sandbox (homeassistant +
semaphore): the low-level transaction helper
(hermes-mcp-config-transaction.py inspect), invoked directly with a
canonical, revision-agnostic payload for both servers, reports
{"ok": true, "state": "matched"} for both — proving the on-disk config and
the running gateway are correct. The false "mismatch" is produced entirely on
the NemoClaw host side.

Root cause

statusMcpBridge() in src/lib/actions/sandbox/mcp-bridge-status.ts, when
called for one named server (server !== undefined), only builds a fresh
credentialObservations / credentialRevisions map entry for that one
server
(one live OpenShell sandbox exec per query, by design — see the
entries/credentialObservations loop).

That partial credentialRevisions map was then passed to
inspectHermesMcpRuntimeIntent() without also scoping its entries
option
, so the reconciliation call silently defaulted to every bridge entry
in the sandbox's registry (bridgeEntries(sandbox)), not just the one
server actually observed.

Consequence, per entry in the resulting "present" payload:

  • The queried server gets an expected Authorization header built from
    the credential revision just observed live from OpenShell
    (Bearer openshell:resolve:env:v<N>_<ENV>). The in-sandbox helper
    (_managed_candidate_matches in agents/hermes/mcp-config-transaction.py)
    requires an exact match against this — no fallback tolerance — because
    a revisioned expected value fails the helper's
    ENV_PLACEHOLDER_RE canonical-form check that gates the lenient
    any-well-formed-revision fallback path.
  • Every other, unqueried server gets a canonical, revision-agnostic
    expected value (Bearer openshell:resolve:env:<ENV>, no revision),
    which the same helper matches against any validly-shaped revision on
    disk.

So only the specifically-queried server is held to a strict, exact,
point-in-time revision match. If OpenShell's periodic credential-revision
refresh (provider credential refresh worker, observed running on a
60-second interval in the live logs) rotates that server's revision between
the last config write and this live observation, the exact match fails —
even though the revision embedded in config.yaml is still perfectly valid
and resolvable by OpenShell at runtime. Every other, unobserved server passes
unconditionally regardless of its real state. Whichever server was queried or
restarted most recently is therefore always the fragile one.

Fix

When a specific server is requested, scope both entries and
managedServerNames passed to inspectHermesMcpRuntimeIntent() to just that
one server, so the reconciliation result reflects only what this call
actually observed — never blending a live, exact-revision check for one
server with a stale, lenient assumption about another.

The full-sandbox mcp status (no server name) path is unchanged: it already
observes every entry before reconciling, so its credential-revision map was
already complete.

Testing

  • New regression test:
    src/lib/actions/sandbox/mcp-bridge-status-hermes-multi-server.test.ts
    — registers a Hermes sandbox with two managed servers, mocks the OpenShell
    transaction-helper inspect call to record every payload it receives, and
    asserts that querying server A's status never mentions server B in
    present/absent, and vice versa.
    • Confirmed failing on the pre-fix code (git stash the source change
      and re-run: the payload for server A's query incorrectly includes B).
    • Confirmed passing with the fix applied.
  • npx vitest run --project cli src/lib/actions/sandbox/mcp-bridge*.test.ts test/mcp/*.test.ts
    34 files / 463 tests passed, no regressions.
  • npx tsc -p tsconfig.src.json --noEmit → clean, no new type errors.

Verification note (environment)

The full test run above (463/463, including an earlier revision of this
same regression test) was performed in a separate Linux session. On the
Windows machine used to prepare this branch, the entire mcp-bridge-status*
vitest suite — including pre-existing tests unrelated to this change — cannot
execute: every test that calls registerSandbox fails in the repo's atomic
config writer with EPERM: operation not permitted, fsync
(fsyncDirectoryfs.fsyncSync() on a directory handle, which Windows
never permits — errno -4048). This is a known, pre-existing
platform limitation of the test harness and is unrelated to this patch.

What was re-verified on Windows against this exact diff:

  • npx tsc -p tsconfig.src.json --noEmit → clean, no new type errors.
  • git diff reviewed: the change is limited to the single scoped spread in
    statusMcpBridge() plus the new regression test file; nothing else.

The local prek pre-commit/pre-push hooks were bypassed (--no-verify) for
this commit, again for Windows-only reasons: the local-credential-helper-pin
repository check does fs.statSync(scripts/local-credential-helper.mts).mode & 0o111 on the working-tree file, which is always 0 on Windows (no POSIX
executable bit) — even though that file (which this PR does not touch) keeps
its correct 100755 mode in the git index, verified with
git ls-files -s scripts/local-credential-helper.mts. Oxfmt was run manually
and its (cosmetic) reformat of the new test file's it(...) call is included.
Upstream CI (Linux) will run the same hooks and the full test suite on this PR.

Notes for reviewers

  • This does not touch mcp-config-transaction.py or
    runtime-config-guard.py (the in-sandbox helper and hash/integrity guard).
    Both were read in full during investigation and are correctly, defensively
    implemented — the bug is purely in how the host-side status call composed
    its expectation payload.
  • I could not reproduce or attribute this to the two known upstream
    hermes-agent issues referenced in
    agents/hermes/mcp-config-transaction.py (Feature: MCP Server Management — Discovery, Selective Tool Loading, and hermes mcp CLI NousResearch/hermes-agent#690,
    #52417) — this is a distinct, NemoClaw-side bug.

Summary by CodeRabbit

  • Bug Fixes

    • MCP bridge status checks now reconcile only the requested server, preventing unrelated servers from being incorrectly marked absent.
    • Multi-server sandbox status reporting now preserves accurate per-server bridge state.
  • Tests

    • Added regression coverage for MCP status reconciliation across multiple servers.

Signed-off-by: spaghiari <leboss783@gmail.com>
@copy-pr-bot

copy-pr-bot Bot commented Sep 6, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 77dbf2d9-8375-4e14-af0c-444c47e74911

📥 Commits

Reviewing files that changed from the base of the PR and between afb2342 and 0e2b18c.

📒 Files selected for processing (2)
  • src/lib/actions/sandbox/mcp-bridge-status-hermes-multi-server.test.ts
  • src/lib/actions/sandbox/mcp-bridge-status.ts

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

Hermes MCP single-server status checks now reconcile only the requested server. A regression test covers two managed servers and verifies server-scoped reconciliation payloads.

Changes

Hermes MCP status reconciliation

Layer / File(s) Summary
Server-scoped reconciliation and regression coverage
src/lib/actions/sandbox/mcp-bridge-status.ts, src/lib/actions/sandbox/mcp-bridge-status-hermes-multi-server.test.ts
Single-server checks pass only the requested server’s observed entry and managed-server name to Hermes reconciliation. All-server checks retain the existing scope. The regression test validates independent alpha and beta payloads.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 0e2b1

Single-server MCP status checks now reconcile only the requested server, preventing sibling servers from affecting its reported status. The targeted regression coverage supports merge readiness.

Suggested reviewers: apurvvkumaria, ericksoa, rsliter, prekshivyas

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: scoping Hermes MCP status reconciliation to the queried server.
Linked Issues check ✅ Passed The implementation scopes both reconciliation entries and managed server names for single-server queries. The regression test verifies that server A and server B remain isolated. Full-sandbox behavior…
Out of Scope Changes check ✅ Passed All changes support issue #11118. The production fix and regression test directly address the cross-server status reconciliation bug. No unrelated code changes are identified.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor finished for commit 0e2b18c. Include the Advisor findings in the complete PR feedback collection. Verify and group valid findings before repair.

All previous runs

@spaghiari
spaghiari marked this pull request as ready for review September 6, 2026 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(mcp): scope Hermes MCP status reconciliation to the queried server

1 participant