fix(mcp): reconcile Hermes runtime state - #6261
Merged
Merged
Conversation
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
added
integration: hermes
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds MCP intended/applied integrity tracking across Hermes build, runtime guard, transaction, startup, and sandbox flows, reconciles persisted MCP server intent with the running gateway, persists ChangesHermes MCP intended/applied integrity reconciliation
Estimated code review effort: 5 (Critical) | ~120 minutes OpenShell credential boundary runtime version enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Startsh as start.sh
participant Guard as runtime-config-guard.py
participant Transaction as mcp-config-transaction.py
participant Gateway
Startsh->>Guard: inspect_hermes_mcp_integrity()
Guard-->>Startsh: pending or current
alt reconciliation pending
Startsh->>Transaction: commit_hermes_mcp_applied_if_pending()
Transaction->>Guard: refresh_hashes(mcp_transition="apply")
Guard-->>Transaction: applied state committed
else drift detected / commit fails
Startsh->>Gateway: stop uncommitted gateway
Startsh-->>Startsh: fail with mcp-integrity
end
sequenceDiagram
participant Client as connectSandbox
participant Recon as inspectHermesMcpRuntimeIntent
participant Openshell as OpenShell CLI
participant Transaction as hermes-mcp-config-transaction.py
Client->>Recon: inspectHermesMcpRuntimeIntent(sandboxName)
Recon->>Openshell: sandbox exec inspect --payload
Openshell->>Transaction: inspect
Transaction-->>Openshell: matched or mismatch
Openshell-->>Recon: stdout/stderr
Recon-->>Client: ok/state/detail
alt refused
Client->>Client: exitOnMcpReconciliationRefusal (exit 1)
end
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings Action checklist
Test follow-ups to resolve or justifyIf these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.
This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/actions/sandbox/mcp-bridge-remove.ts (1)
97-330: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd the OpenShell runtime-version check before the first provider mutation.
removeMcpBridgeUnlockedmutates providers viadetachProvider,detachMissingProviderReference,unregisterAgentAdapter, anddeleteProvider, but never callsassertMcpCredentialBoundaryRuntimeVersion(). The shared helpers also don’t enforce it, somcp removecan act on an unverified OpenShell binary whilemcp addandmcp restartfail closed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/mcp-bridge-remove.ts` around lines 97 - 330, The remove path is missing the OpenShell runtime-version gate before any provider mutation, so `removeMcpBridgeUnlocked` can proceed on an unverified binary while other MCP lifecycle commands fail closed. Add a call to `assertMcpCredentialBoundaryRuntimeVersion()` in `removeMcpBridgeUnlocked` after the preflight checks and before the first provider-affecting action, so it runs ahead of `detachMissingProviderReference`, `detachProvider`, `unregisterAgentAdapter`, and `deleteProvider`. Keep the check in the same guardrail position as the add/restart flow, using the existing helper name to make the protection consistent across MCP lifecycle commands.
🧹 Nitpick comments (5)
test/mcp-policy-key-ownership.test.ts (1)
11-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated
--versionstub clause into a shared helper.The identical
if [ "$1" = "--version" ]; then printf '%s\n' 'openshell 0.0.72'; exit 0; ficlause is duplicated across 6 inline-generated openshell stubs in this file. A shared constant/helper (e.g. a function returning this snippet, parameterized by version) would reduce the future maintenance cost of an OpenShell version bump across this file (and its siblings using the same literal).♻️ Example consolidation
+const OPENSHELL_VERSION_STUB_CLAUSE = + `if [ "$1" = "--version" ]; then printf '%s\n' 'openshell 0.0.72'; exit 0; fi`; + fs.writeFileSync( path.join(binDir, "openshell"), `#!/bin/sh -if [ "$1" = "--version" ]; then printf '%s\n' 'openshell 0.0.72'; exit 0; fi +${OPENSHELL_VERSION_STUB_CLAUSE} printf '%s\n' "$*" >> ${JSON.stringify(callsPath)} ...Also applies to: 30-30, 80-80, 110-110, 175-175, 291-291, 378-378, 554-554
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/mcp-policy-key-ownership.test.ts` around lines 11 - 12, The inline OpenShell stub scripts in this test file repeat the same --version clause, so extract that snippet into a shared helper or constant and reuse it across the generated stubs. Update the logic around the openshell fixture generation in this test (and any sibling test helpers using the same literal) so the version string is parameterized in one place, making OpenShell version bumps easier to maintain.src/lib/actions/sandbox/process-recovery.ts (1)
765-775: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate reconciliation-refusal result construction.
Both branches build an almost identical refusal object (only
wasRunningdiffers). Extracting a small helper would reduce duplication and keep the two call sites from drifting if a field is later added/renamed.♻️ Proposed fix
+function mcpReconciliationRefusalResult(reconciliation: { detail: string }, wasRunning: boolean) { + return { + checked: true as const, + wasRunning, + recovered: false, + forwardRecovered: false, + mcpReconciliationRefused: true as const, + mcpReconciliationReason: reconciliation.detail, + }; +}Then at each call site:
- const reconciliation = inspectHermesMcpRuntimeIntent(sandboxName); - if (!reconciliation.ok) { - return { - checked: true, - wasRunning: true, - recovered: false, - forwardRecovered: false, - mcpReconciliationRefused: true, - mcpReconciliationReason: reconciliation.detail, - }; - } + const reconciliation = inspectHermesMcpRuntimeIntent(sandboxName); + if (!reconciliation.ok) { + return mcpReconciliationRefusalResult(reconciliation, true); + }(and
falsefor the not-running/recovered branch at line 927)Also applies to: 927-937
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/process-recovery.ts` around lines 765 - 775, The reconciliation refusal return object is duplicated in both branches of process-recovery.ts, with only wasRunning changing between the call sites. Extract a small helper near inspectHermesMcpRuntimeIntent/reconciliation handling that builds the refusal result, and have both branches call it with the appropriate wasRunning value while keeping checked, recovered, forwardRecovered, mcpReconciliationRefused, and mcpReconciliationReason aligned in one place.src/lib/actions/sandbox/connect.ts (1)
240-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating duplicated MCP remediation guidance.
This function's remediation text (
mcp restart/rebuild --yesguidance) is near-identical to the text emitted byprintGatewayRestartFailureingateway-restart.ts(lines 189-194) for the same"MCP reconciliation refusal"condition. Extracting a shared helper (e.g.mcpReconciliationRemediationLines(sandboxName)) would keep the two messages from silently diverging as one is edited without the other.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/connect.ts` around lines 240 - 262, The MCP reconciliation refusal remediation text is duplicated between exitOnMcpReconciliationRefusal and the matching gateway restart failure output, so centralize it in a shared helper to avoid drift. Extract the common “mcp restart” and “rebuild --yes” guidance into a reusable function such as mcpReconciliationRemediationLines(sandboxName), then have exitOnMcpReconciliationRefusal and printGatewayRestartFailure call that helper for their console output.src/lib/actions/sandbox/gateway-restart.ts (2)
189-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated remediation text vs.
connect.ts.Same duplication concern noted in
connect.ts'sexitOnMcpReconciliationRefusal(lines 254-259): themcp restart/rebuild --yesguidance strings are duplicated here with slightly different wording ("restore managed MCP state" vs. "restore the managed MCP configuration, then retry"). Worth a single shared helper for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/gateway-restart.ts` around lines 189 - 194, The MCP refusal remediation text is duplicated between gateway-restart and connect, so consolidate it into one shared helper for consistency. Move the `mcp restart` and `rebuild --yes` guidance used in `gateway-restart.ts` and `connect.ts` into a common function, then have the existing flow in `exitOnMcpReconciliationRefusal` and the gateway restart path call that helper so both sites emit the same wording.
81-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport
HermesMcpReconciliationResulthere instead of re-declaring the union. The inline copy widensstatetostring, so it can drift from the bridge result without a type error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/actions/sandbox/gateway-restart.ts` around lines 81 - 83, The inspectHermesMcpRuntimeIntent return type is being re-declared inline and widens state to string, which can drift from the bridge result. Import and reuse HermesMcpReconciliationResult in gateway-restart.ts instead of the local union so the type stays aligned with the source definition and any future changes are caught automatically.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/actions/sandbox/mcp-bridge-validation.ts`:
- Around line 58-100: The fail-closed error in
assertMcpCredentialBoundaryRuntimeVersion is too generic when OpenShell version
validation fails. Update credentialBoundaryVersionError so the message includes
a short remediation hint for users, especially on version mismatch or
unparseable output, and keep the hint actionable (for example, pin/install the
expected OpenShell version) while preserving the existing expected/actual
detail.
In `@test/hermes-mcp-integrity-state.test.ts`:
- Around line 20-27: The startup parsing helper in runHermesRootMcpStartup adds
a new if-statement that trips the guardrail. Refactor it to follow the same
pattern used by extractShellFunctionFromSource in the sibling integrity test:
assert the match result directly with the test framework instead of branching
and throwing, while keeping the existing startupBlock extraction logic and error
message context.
In `@test/hermes-start-config-integrity.test.ts`:
- Line 40: Add a negative test for the MCP-integrity gate in
hermes-start-config-integrity.test.ts: the current root-mode stub for
inspect_hermes_mcp_integrity() always returns 0, so it only verifies the success
path. Introduce a failing stub case for inspect_hermes_mcp_integrity() that
returns a nonzero exit status and assert that the restart logic emits
HERMES_RESTART_FAILURE_CODE=mcp-integrity, alongside the existing happy-path
coverage.
---
Outside diff comments:
In `@src/lib/actions/sandbox/mcp-bridge-remove.ts`:
- Around line 97-330: The remove path is missing the OpenShell runtime-version
gate before any provider mutation, so `removeMcpBridgeUnlocked` can proceed on
an unverified binary while other MCP lifecycle commands fail closed. Add a call
to `assertMcpCredentialBoundaryRuntimeVersion()` in `removeMcpBridgeUnlocked`
after the preflight checks and before the first provider-affecting action, so it
runs ahead of `detachMissingProviderReference`, `detachProvider`,
`unregisterAgentAdapter`, and `deleteProvider`. Keep the check in the same
guardrail position as the add/restart flow, using the existing helper name to
make the protection consistent across MCP lifecycle commands.
---
Nitpick comments:
In `@src/lib/actions/sandbox/connect.ts`:
- Around line 240-262: The MCP reconciliation refusal remediation text is
duplicated between exitOnMcpReconciliationRefusal and the matching gateway
restart failure output, so centralize it in a shared helper to avoid drift.
Extract the common “mcp restart” and “rebuild --yes” guidance into a reusable
function such as mcpReconciliationRemediationLines(sandboxName), then have
exitOnMcpReconciliationRefusal and printGatewayRestartFailure call that helper
for their console output.
In `@src/lib/actions/sandbox/gateway-restart.ts`:
- Around line 189-194: The MCP refusal remediation text is duplicated between
gateway-restart and connect, so consolidate it into one shared helper for
consistency. Move the `mcp restart` and `rebuild --yes` guidance used in
`gateway-restart.ts` and `connect.ts` into a common function, then have the
existing flow in `exitOnMcpReconciliationRefusal` and the gateway restart path
call that helper so both sites emit the same wording.
- Around line 81-83: The inspectHermesMcpRuntimeIntent return type is being
re-declared inline and widens state to string, which can drift from the bridge
result. Import and reuse HermesMcpReconciliationResult in gateway-restart.ts
instead of the local union so the type stays aligned with the source definition
and any future changes are caught automatically.
In `@src/lib/actions/sandbox/process-recovery.ts`:
- Around line 765-775: The reconciliation refusal return object is duplicated in
both branches of process-recovery.ts, with only wasRunning changing between the
call sites. Extract a small helper near
inspectHermesMcpRuntimeIntent/reconciliation handling that builds the refusal
result, and have both branches call it with the appropriate wasRunning value
while keeping checked, recovered, forwardRecovered, mcpReconciliationRefused,
and mcpReconciliationReason aligned in one place.
In `@test/mcp-policy-key-ownership.test.ts`:
- Around line 11-12: The inline OpenShell stub scripts in this test file repeat
the same --version clause, so extract that snippet into a shared helper or
constant and reuse it across the generated stubs. Update the logic around the
openshell fixture generation in this test (and any sibling test helpers using
the same literal) so the version string is parameterized in one place, making
OpenShell version bumps easier to maintain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dd4c30eb-9393-423a-967d-19180f0233fb
📒 Files selected for processing (37)
agents/hermes/Dockerfileagents/hermes/mcp-config-transaction.pyagents/hermes/runtime-config-guard.pyagents/hermes/start.shsrc/lib/actions/sandbox/connect-flow-hermes-boundary.test.tssrc/lib/actions/sandbox/connect.tssrc/lib/actions/sandbox/gateway-restart.test.tssrc/lib/actions/sandbox/gateway-restart.tssrc/lib/actions/sandbox/mcp-bridge-adapter-status.tssrc/lib/actions/sandbox/mcp-bridge-add-restart.tssrc/lib/actions/sandbox/mcp-bridge-destroy.tssrc/lib/actions/sandbox/mcp-bridge-hermes-reconciliation.test.tssrc/lib/actions/sandbox/mcp-bridge-hermes-reconciliation.tssrc/lib/actions/sandbox/mcp-bridge-input-validation.test.tssrc/lib/actions/sandbox/mcp-bridge-remove.tssrc/lib/actions/sandbox/mcp-bridge-restart.tssrc/lib/actions/sandbox/mcp-bridge-state.tssrc/lib/actions/sandbox/mcp-bridge-status-removal.test.tssrc/lib/actions/sandbox/mcp-bridge-status-state.test.tssrc/lib/actions/sandbox/mcp-bridge-status.tssrc/lib/actions/sandbox/mcp-bridge-validation.tssrc/lib/actions/sandbox/process-recovery.tssrc/lib/state/registry-mcp.tstest/deepagents-mcp-legacy-lifecycle.test.tstest/fixtures/openshell-v0.0.72test/hermes-doctor-config-hash.test.tstest/hermes-mcp-config-transaction.test.tstest/hermes-mcp-integrity-state.test.tstest/hermes-nonroot-strict-hash-reconciliation.test.tstest/hermes-runtime-config-guard.test.tstest/hermes-start-config-integrity.test.tstest/mcp-add-crash-consistency.test.tstest/mcp-destroy-lifecycle.test.tstest/mcp-policy-key-ownership.test.tstest/mcp-restart-policy-order.test.tstest/registry.test.tstest/support/connect-flow-test-harness.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Author
|
Addressed the advisor findings at Implemented:
Resolved with existing focused evidence:
Justified without additional refactoring:
Latest local proof: 106/106 focused CLI/integration/E2E-support tests, both TypeScript checks, Ruff/Python compile, Biome/lint, shell syntax, source-shape, test-size, conditional scan, diff checks, and secret scanning. |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Author
|
Follow-up for the refreshed advisor findings, addressed at
Nemotron’s remaining items are improvements rather than blockers; disposition:
Focused snapshot verification: 57/57 tests plus Ruff, Python compile, Biome, source-shape, test-size, and diff checks. All commits on the PR are GitHub |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/live/mcp-bridge-hermes-lifecycle.ts`:
- Around line 111-125: The inspect payload in the hermes lifecycle test
hardcodes the absent server name instead of using SERVER_NAME, which can make
the assertion drift from the rest of the test. Update the payload built in the
sandbox.execShell call inside the mcp-bridge-hermes-lifecycle test to reference
SERVER_NAME consistently, matching the tombstone check already used earlier in
the same test, so the inspect step always targets the intended server name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f4c8fb74-9055-466a-a8cd-7d8637a7c53f
📒 Files selected for processing (11)
agents/hermes/mcp-config-transaction.pyagents/hermes/runtime-config-guard.pysrc/lib/actions/sandbox/connect-flow-hermes-boundary.test.tssrc/lib/actions/sandbox/connect.tssrc/lib/actions/sandbox/gateway-restart.test.tssrc/lib/actions/sandbox/gateway-restart.tssrc/lib/actions/sandbox/mcp-bridge-hermes-reconciliation.test.tssrc/lib/actions/sandbox/mcp-bridge-hermes-reconciliation.tstest/e2e/live/mcp-bridge-hermes-lifecycle.tstest/e2e/live/mcp-bridge.test.tstest/hermes-mcp-integrity-state.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/lib/actions/sandbox/connect.ts
- src/lib/actions/sandbox/gateway-restart.ts
- src/lib/actions/sandbox/mcp-bridge-hermes-reconciliation.ts
- test/hermes-mcp-integrity-state.test.ts
- agents/hermes/mcp-config-transaction.py
- agents/hermes/runtime-config-guard.py
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Author
|
Completed Nemotron’s three explicit proof requests at
Remaining resolve/justify items:
Validation: combined new tests 10/10; full E2E-support suite 674/674; both TypeScript checks, Biome, test-size, source-shape, conditional-growth, diff checks, hooks, and secret scan pass. The live cloud/OpenShell lane remains gated and was not run locally. The new commit is GitHub |
Contributor
Author
|
Final Nemotron disposition on
The GPT-5.5 advisor now reports |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
test/e2e/live/mcp-bridge-hermes-lifecycle.ts (1)
156-163: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStill hardcodes
"fake"instead ofSERVER_NAME.This was flagged in a prior review round and remains unaddressed. The inspect payload should reference
SERVER_NAMElike the tombstone check at Line 131, so it can't silently drift from the rest of the test if the constant's value changes.🛠️ Proposed fix
trustedSandboxShellScript( [ "set -eu", - '/usr/local/lib/nemoclaw/hermes-mcp-config-transaction.py inspect --payload \'{"present":{},"absent":["fake"]}\'', + `/usr/local/lib/nemoclaw/hermes-mcp-config-transaction.py inspect --payload '${JSON.stringify( + { present: {}, absent: [SERVER_NAME] }, + )}'`, ].join("\n"), ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/live/mcp-bridge-hermes-lifecycle.ts` around lines 156 - 163, The inspect payload in the hermes lifecycle test is still hardcoding the absent server name as "fake" instead of using SERVER_NAME. Update the payload passed to hermes-mcp-config-transaction.py inspect in the test around the sandbox.execShell call so it references SERVER_NAME consistently, matching the tombstone check logic and keeping the test aligned if the constant changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/actions/sandbox/gateway-restart-hermes-drift.test.ts`:
- Around line 116-150: The test is mocking inspectHermesMcpRuntimeIntent, so it
only verifies refusal and sanitization for an arbitrary failed reconciliation
result rather than the real drift-detection path. Update
gateway-restart-hermes-drift.test.ts by either wiring the actual
inspectHermesMcpRuntimeIntent implementation into deps (so the python-backed
detection is exercised end-to-end) or by renaming/refocusing the test to
explicitly cover only restartSandboxGatewayWithDeps refusal/sanitization
behavior; keep the earlier drift-detection assertions separate from this mocked
branch.
---
Duplicate comments:
In `@test/e2e/live/mcp-bridge-hermes-lifecycle.ts`:
- Around line 156-163: The inspect payload in the hermes lifecycle test is still
hardcoding the absent server name as "fake" instead of using SERVER_NAME. Update
the payload passed to hermes-mcp-config-transaction.py inspect in the test
around the sandbox.execShell call so it references SERVER_NAME consistently,
matching the tombstone check logic and keeping the test aligned if the constant
changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 10258781-4870-4a69-8944-22dd84248ab0
📒 Files selected for processing (4)
src/lib/actions/sandbox/gateway-restart-hermes-drift.test.tstest/e2e/live/mcp-bridge-hermes-lifecycle.tstest/e2e/live/mcp-bridge.test.tstest/hermes-mcp-integrity-state.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/live/mcp-bridge.test.ts
- test/hermes-mcp-integrity-state.test.ts
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Author
|
Latest-head advisor disposition for GPT runtime follow-ups
The advisor-selected Nemotron scope disposition
All required PR checks are green on this head. GPT recommends |
Contributor
E2E Target Results — ❌ Some jobs failedRun: 28690073365
|
Contributor
Author
|
Runtime dispatch disposition for E2E run 28690073365 on exact head
No PR code change or blind rerun is warranted: the stable non-Hermes runtime paths validate the MCP implementation, the dev rejection is the intended #6256 security behavior, and Hermes was prevented from reaching the changed code by the known mutable-base infrastructure issue. All required PR checks remain green; GPT's latest-head posture remains |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Author
|
CodeRabbit follow-up completed at
Validation: 104/104 consolidated focused tests, both TypeScript checks, Biome, source-shape/test-size/conditional guardrails, commit hooks, push hooks, and |
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
E2E Target Results — ❌ Some jobs failedRun: 28714607119
|
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
E2E Target Results — ❌ Some jobs failedRun: 28715858064
|
This was referenced Jul 5, 2026
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28767576983
|
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28767576010
|
Contributor
E2E Target Results —
|
| Job | Result |
|---|---|
| mcp-bridge |
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28767850811
|
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28767849656
|
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 28767848673
|
Contributor
Author
Final-head advisor disposition —
|
cv
approved these changes
Jul 6, 2026
7 tasks
cv
pushed a commit
that referenced
this pull request
Jul 7, 2026
## Summary Add the v0.0.75 release-notes entry for the release train, summarizing the user-facing fixes merged since v0.0.74. Release-prep docs for the `nemoclaw-maintainer-cut-release-tag` gate. ## Related Issue Release prep for v0.0.75. Remove this section if none. ## Changes - `docs/about/release-notes.mdx`: add the `## v0.0.75` section (themed intro + grouped bullets with source-page links), matching the existing v0.0.74 style. ### Source summary (doc-impacting PRs → doc page) - #6370 -> `docs/about/release-notes.mdx`: prepared-backup recovery restores gateway state and defers the live route check to onboarding, so upgrade recovery no longer fails on an unset gateway route. - #6305 -> `docs/about/release-notes.mdx`: in-place upgrades recover gateway-orphaned sandboxes. - #6332 -> `docs/about/release-notes.mdx`: same-name `--fresh` re-onboard preserves fresh LangChain Deep Agents Code routing. - #6335 -> `docs/about/release-notes.mdx`: custom Anthropic-compatible inference uses the OpenAI frontend. - #6298 -> `docs/about/release-notes.mdx`: OpenAI-only agents keep the `/v1` base URL on Anthropic-compatible endpoints. - #6304 -> `docs/about/release-notes.mdx`: local docker-driver gateway credentials no longer expire. - #6261 -> `docs/about/release-notes.mdx`: Hermes runtime and managed MCP state reconcile after a runtime change. - #6318 -> `docs/about/release-notes.mdx`: Hermes installs accept a pinned base platform digest. - #6291 -> `docs/about/release-notes.mdx`: OpenClaw local CLI pairing restores its previous connection path. Test-performance, CI, and chore commits since v0.0.74 are excluded as non-user-facing. ## Type of Change - [x] Doc only (prose changes, no code sample modifications) ## Quality Gates - [x] Tests not applicable — justification: documentation-only change (release notes prose). - [x] Docs updated for user-facing behavior changes ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] `npm run docs` builds without warnings introduced by this change — command/result: "Found 0 errors and 2 warnings" (the 2 warnings pre-exist this change). - [x] Doc pages follow the style guide (active voice, no numbered/colon titles, correct NVIDIA/NemoClaw/OpenShell capitalization; skip-terms avoided). - [x] No secrets, API keys, or credentials committed --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added a new **v0.0.75** section to the release notes, highlighting improved sandbox upgrade hardening and prepared-backup recovery, updated inference routing for Anthropic-compatible endpoints, longer-lasting local gateway credential handling, and restored CLI pairing reconnection without re-pairing. Also includes cross-links to related NemoClaw CLI and documentation pages. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jul 8, 2026
21 tasks
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
## Summary Reconcile Hermes MCP intent with the gateway state across transactions and lifecycle recovery, and bind OpenShell credential-boundary validation to the exact host CLI version before provider mutations. This closes the configuration-drift gap from NVIDIA#6257 while implementing the enforceable host-side portion of NVIDIA#6256 without adding a misleading in-image OpenShell stub. ## Related Issue Closes NVIDIA#6257 Addresses NVIDIA#6256 ## Changes - add an exact, uncached `openshell --version` gate before MCP provider/credential mutations; missing, failed, malformed, and mismatched probes fail closed without exposing command output - persist a canonical credential-safe MCP digest as `intended` and `applied`, commit applied state only after a healthy gateway reload, and restore both config and integrity snapshots on rollback - reconcile Hermes startup, restart, resume, rebuild, status, and recovery against persisted managed intent, including removal tombstones, with actionable fail-closed guidance - prevent generic config writes from changing `mcp_servers`, reject malformed or stale integrity state, and avoid blessing concurrent config drift during applied-state commits - preserve the canonical MCP state marker when shields transitions regenerate strict and compatibility hashes, and keep supervisor/API-key test fixtures on the same three-line contract - strictly allowlist credential-safe inspection fields and sanitize all sandbox-derived reconciliation diagnostics before connect/restart output - add focused regression coverage for version probes, additions/removals, pending and malformed state, root/non-root startup, rollback, registry reconciliation, and destroy recovery - extend the live Hermes MCP lane through removal plus a real gateway restart, proving the tombstone persists, effective config stays absent, the retired route remains denied, and credentials do not leak - preserve exact supervised API/dashboard relays across managed Hermes gateway replacements, retrying public health without churning structurally proven listeners - exclude authenticated Hermes config bytes from dataclass representations and cover the redaction ### NVIDIA#6256 runtime-boundary note OpenShell 0.0.72 intentionally does not expose the supervisor identity mount to workload children, and the Hermes workload image does not contain the host OpenShell CLI. Running `openshell --version` in the Python helper would therefore either fail every real transaction or attest an unrelated in-image stub rather than the supervisor enforcing credentials. This change verifies the selected host CLI immediately before every provider mutation and retains exact manifest/policy validation in Python. NVIDIA#6256 remains open for an upstream supervisor capability/version attestation that Hermes startup can verify honestly. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: no new command, option, or operator-managed configuration; failures include inline restart/rebuild recovery guidance - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: reviewed the credential boundary, transactional ordering, rollback, stale-state, redaction, tombstone, and concurrent-drift paths with focused fail-closed regressions; maintainer approval remains required before merge - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [x] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Local verification on final head `44195b56c096ae3ee50f465de9c983f705eda031` passed 190 focused security/lifecycle tests across 10 files, both TypeScript typechecks, the CLI/plugin builds, Python compile/Ruff, ShellCheck/Biome, source-shape, test-size, title, conditional, diff, secret-scan, and commit/push hooks. All ordinary required GitHub checks are green on the final head: 39 passed, 2 intentional skips, and 0 pending/failing. The one unrelated package-contract require-cache flake passed on [clean rerun attempt 2](https://github.com/NVIDIA/NemoClaw/actions/runs/28767840291/attempts/2), with no source change. The exact-head [GPT advisor](NVIDIA#6261 (comment)) recommends `merge_as_is`; CodeRabbit is green, and all 8 review threads are resolved. The remaining stale/false-premise Nemotron items are addressed in the [final-head disposition](NVIDIA#6261 (comment)). All advisor-required exact-head runtime proof passed: [stable MCP bridge](https://github.com/NVIDIA/NemoClaw/actions/runs/28767848673), [Hermes E2E](https://github.com/NVIDIA/NemoClaw/actions/runs/28767849656), [gateway guard recovery](https://github.com/NVIDIA/NemoClaw/actions/runs/28767850811), and [production sandbox images plus downstream E2E](https://github.com/NVIDIA/NemoClaw/actions/runs/28767851667). Stable MCP passed the real OpenClaw, DeepAgents, and Hermes add/restart/rebuild/remove/restart lifecycles, including adjacent Hermes restart and credential rotation; the credential scan passed across 489 artifact files. All 22 PR commits are GitHub `Verified` and DCO-signed. The head is mergeable with a clean current-`main` synthetic merge. The moving OpenShell-dev lane remains optional and is not merge evidence; the supported credential boundary is the exact stable OpenShell 0.0.72 contract. I certify that this contribution is made under the Developer Certificate of Origin. --- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added Hermes MCP integrity tracking with intended/applied state transitions, plus CLI commands to inspect integrity and commit “applied”. * Added Hermes MCP runtime reconciliation remediation with fail-closed behavior during sandbox connect and gateway recovery/restart. * Persist and reconcile managed MCP server names across add/remove/destroy/restore. * **Bug Fixes** * Strengthened startup/restart verification to block drift and pending reconciliation, with improved rollback/reload verification behavior. * Improved failure classification/remediation messaging for MCP integrity and reconciliation-refusal scenarios. * **Tests** * Expanded coverage for Hermes MCP integrity, drift, reconciliation refusal, lifecycle flows, and config hash sealing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
## Summary Add the v0.0.75 release-notes entry for the release train, summarizing the user-facing fixes merged since v0.0.74. Release-prep docs for the `nemoclaw-maintainer-cut-release-tag` gate. ## Related Issue Release prep for v0.0.75. Remove this section if none. ## Changes - `docs/about/release-notes.mdx`: add the `## v0.0.75` section (themed intro + grouped bullets with source-page links), matching the existing v0.0.74 style. ### Source summary (doc-impacting PRs → doc page) - NVIDIA#6370 -> `docs/about/release-notes.mdx`: prepared-backup recovery restores gateway state and defers the live route check to onboarding, so upgrade recovery no longer fails on an unset gateway route. - NVIDIA#6305 -> `docs/about/release-notes.mdx`: in-place upgrades recover gateway-orphaned sandboxes. - NVIDIA#6332 -> `docs/about/release-notes.mdx`: same-name `--fresh` re-onboard preserves fresh LangChain Deep Agents Code routing. - NVIDIA#6335 -> `docs/about/release-notes.mdx`: custom Anthropic-compatible inference uses the OpenAI frontend. - NVIDIA#6298 -> `docs/about/release-notes.mdx`: OpenAI-only agents keep the `/v1` base URL on Anthropic-compatible endpoints. - NVIDIA#6304 -> `docs/about/release-notes.mdx`: local docker-driver gateway credentials no longer expire. - NVIDIA#6261 -> `docs/about/release-notes.mdx`: Hermes runtime and managed MCP state reconcile after a runtime change. - NVIDIA#6318 -> `docs/about/release-notes.mdx`: Hermes installs accept a pinned base platform digest. - NVIDIA#6291 -> `docs/about/release-notes.mdx`: OpenClaw local CLI pairing restores its previous connection path. Test-performance, CI, and chore commits since v0.0.74 are excluded as non-user-facing. ## Type of Change - [x] Doc only (prose changes, no code sample modifications) ## Quality Gates - [x] Tests not applicable — justification: documentation-only change (release notes prose). - [x] Docs updated for user-facing behavior changes ## Verification - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] `npm run docs` builds without warnings introduced by this change — command/result: "Found 0 errors and 2 warnings" (the 2 warnings pre-exist this change). - [x] Doc pages follow the style guide (active voice, no numbered/colon titles, correct NVIDIA/NemoClaw/OpenShell capitalization; skip-terms avoided). - [x] No secrets, API keys, or credentials committed --- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added a new **v0.0.75** section to the release notes, highlighting improved sandbox upgrade hardening and prepared-backup recovery, updated inference routing for Anthropic-compatible endpoints, longer-lasting local gateway credential handling, and restored CLI pairing reconnection without re-pairing. Also includes cross-links to related NemoClaw CLI and documentation pages. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
27 tasks
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
5 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
Reconcile Hermes MCP intent with the gateway state across transactions and lifecycle recovery, and bind OpenShell credential-boundary validation to the exact host CLI version before provider mutations. This closes the configuration-drift gap from #6257 while implementing the enforceable host-side portion of #6256 without adding a misleading in-image OpenShell stub.
Related Issue
Closes #6257
Addresses #6256
Changes
openshell --versiongate before MCP provider/credential mutations; missing, failed, malformed, and mismatched probes fail closed without exposing command outputintendedandapplied, commit applied state only after a healthy gateway reload, and restore both config and integrity snapshots on rollbackmcp_servers, reject malformed or stale integrity state, and avoid blessing concurrent config drift during applied-state commits#6256 runtime-boundary note
OpenShell 0.0.72 intentionally does not expose the supervisor identity mount to workload children, and the Hermes workload image does not contain the host OpenShell CLI. Running
openshell --versionin the Python helper would therefore either fail every real transaction or attest an unrelated in-image stub rather than the supervisor enforcing credentials. This change verifies the selected host CLI immediately before every provider mutation and retains exact manifest/policy validation in Python. #6256 remains open for an upstream supervisor capability/version attestation that Hermes startup can verify honestly.Type of Change
Quality Gates
Verification
Verifiedin GitHubnpx prek run --from-ref main --to-ref HEADpassesnpm testpasses (broad runtime changes only)npm run docsbuilds without warnings (doc changes only)Local verification on final head
44195b56c096ae3ee50f465de9c983f705eda031passed 190 focused security/lifecycle tests across 10 files, both TypeScript typechecks, the CLI/plugin builds, Python compile/Ruff, ShellCheck/Biome, source-shape, test-size, title, conditional, diff, secret-scan, and commit/push hooks.All ordinary required GitHub checks are green on the final head: 39 passed, 2 intentional skips, and 0 pending/failing. The one unrelated package-contract require-cache flake passed on clean rerun attempt 2, with no source change. The exact-head GPT advisor recommends
merge_as_is; CodeRabbit is green, and all 8 review threads are resolved. The remaining stale/false-premise Nemotron items are addressed in the final-head disposition.All advisor-required exact-head runtime proof passed: stable MCP bridge, Hermes E2E, gateway guard recovery, and production sandbox images plus downstream E2E. Stable MCP passed the real OpenClaw, DeepAgents, and Hermes add/restart/rebuild/remove/restart lifecycles, including adjacent Hermes restart and credential rotation; the credential scan passed across 489 artifact files.
All 22 PR commits are GitHub
Verifiedand DCO-signed. The head is mergeable with a clean current-mainsynthetic merge. The moving OpenShell-dev lane remains optional and is not merge evidence; the supported credential boundary is the exact stable OpenShell 0.0.72 contract.I certify that this contribution is made under the Developer Certificate of Origin.
Signed-off-by: Aaron Erickson aerickson@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests