test(hermes): cover MCP integrity through restart - #7743
Conversation
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
added
integration: hermes
|
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 Hermes live E2E coverage for managed MCP additions across shield restoration, gateway restart, locked integrity validation, bridge registration, configuration inspection, and failed-reload rollback. ChangesHermes MCP lifecycle integrity
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HermesTest
participant Sandbox
participant HermesGateway
participant NemoClaw
participant TransactionHelper
HermesTest->>Sandbox: Restore shields
HermesTest->>HermesGateway: Restart gateway
HermesGateway-->>HermesTest: Return restart and health status
HermesTest->>Sandbox: Verify permissions and hash anchors
HermesTest->>NemoClaw: List managed MCP bridges
HermesTest->>TransactionHelper: Inspect expected configuration payload
TransactionHelper-->>HermesTest: Report matched state
HermesTest->>TransactionHelper: Inject managed reload failure
TransactionHelper-->>HermesTest: Roll back configuration and hashes
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/hermes-mcp-transaction-restart.test.ts (2)
51-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
reloadsassertion is a redundant mock-call check.
module.reload_gatewayis mocked to append toreloads, and the test then asserts bothoutcome.reloaded: true(the module's real, public return value) andreloads: ["reload"](a record of the mock invocation). The latter adds no independent confidence over the former — it only proves the stub was called, not any additional observable behavior.As per path instructions, tests should "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions," and this counts as exactly that pattern.
🤖 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/hermes-mcp-transaction-restart.test.ts` around lines 51 - 82, Remove the redundant reloads tracking and its reloads: ["reload"] assertion from the transaction restart test. Keep the public outcome.reloaded: true assertion and the remaining observable integrity assertions, ensuring the test validates behavior through module.execute rather than mock-call details.Source: Path instructions
119-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDigest "current" recomputation reuses the same private functions being validated — risk of a tautological check.
current_digest(andintended/appliedvia_parse_config_hash) are computed here by calling the same private helpers (guard._read_text,guard._parse_config_hash,guard._canonical_mcp_servers_digest) thatinspect_mcp_integrity_snapshot/assert_mcp_integrity_snapshot_currentpresumably use internally. If any of these private digest functions has a bug (e.g. non-deterministic or content-insensitive output), both sides of theintended == applied == currentcomparison would agree on the wrong value, and the test would still pass.
assert_mcp_integrity_snapshot_current(snapshot)above already exercises the real invariant through the module's public validation path — consider relying primarily on that call (and/or comparing against an independently computed reference, e.g. a plainhashlib.sha256over the raw file bytes) rather than re-deriving "current" via the same private canonicalization routine.As per path instructions, tests should "Flag copied production algorithms... and conditionals that make a test pass without exercising its claim."
🤖 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/hermes-mcp-transaction-restart.test.ts` around lines 119 - 134, Remove the tautological digest recomputation in the test after assert_mcp_integrity_snapshot_current(snapshot), especially the use of guard._parse_config_hash and guard._canonical_mcp_servers_digest for validation. Rely on the public assertion for the production invariant, and if explicit digest reporting is required, compute the reference independently from raw file bytes using a standard hashlib digest rather than copied private helpers.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 `@test/hermes-mcp-transaction-restart.test.ts`:
- Around line 25-29: Update runPython to pass the same explicit timeout used by
runGuard and strictHashIsValid in its spawnSync options, ensuring hung Python
scripts terminate rather than blocking the test run.
---
Nitpick comments:
In `@test/hermes-mcp-transaction-restart.test.ts`:
- Around line 51-82: Remove the redundant reloads tracking and its reloads:
["reload"] assertion from the transaction restart test. Keep the public
outcome.reloaded: true assertion and the remaining observable integrity
assertions, ensuring the test validates behavior through module.execute rather
than mock-call details.
- Around line 119-134: Remove the tautological digest recomputation in the test
after assert_mcp_integrity_snapshot_current(snapshot), especially the use of
guard._parse_config_hash and guard._canonical_mcp_servers_digest for validation.
Rely on the public assertion for the production invariant, and if explicit
digest reporting is required, compute the reference independently from raw file
bytes using a standard hashlib digest rather than copied private helpers.
🪄 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: 38c24b95-2ddf-46df-bc94-8f9b88a1263f
📒 Files selected for processing (1)
test/hermes-mcp-transaction-restart.test.ts
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/hermes-mcp-transaction-restart.test.ts (1)
34-166: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEnable this regression test in the targeted suite.
Because this test is skipped, its restart, recovery, and digest assertions provide no CI regression protection for issue
#7499. Replaceit.skipwithitonce the fixture prerequisites are available.As per path instructions, tests must provide behavioral confidence.
🤖 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/hermes-mcp-transaction-restart.test.ts` around lines 34 - 166, Enable the regression test by changing the skipped test declaration in “Hermes managed MCP integrity through restart” from it.skipIf to it.skipIf only for unsupported platforms or identities, ensuring the test runs when fixture prerequisites are available. Preserve the existing Windows/root skip conditions and all restart, recovery, and digest assertions.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.
Outside diff comments:
In `@test/hermes-mcp-transaction-restart.test.ts`:
- Around line 34-166: Enable the regression test by changing the skipped test
declaration in “Hermes managed MCP integrity through restart” from it.skipIf to
it.skipIf only for unsupported platforms or identities, ensuring the test runs
when fixture prerequisites are available. Preserve the existing Windows/root
skip conditions and all restart, recovery, and digest assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 97c156b0-722d-49e9-9348-bffb5e8599fa
📒 Files selected for processing (2)
test/helpers/hermes-restart-config-seal-fixture.tstest/hermes-mcp-transaction-restart.test.ts
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/live/mcp-bridge-hermes-lifecycle.ts (1)
113-170: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the inspector's state via parsed JSON rather than a spaced literal.
toContain('"state": "matched"')couples the assertion tohermes-mcp-config-transaction.py's exact serializer spacing; ajson.dumpsseparator change would fail the test without any behavior change. ParsingeffectiveConfig.stdoutand asserting thestatefield keeps the same claim without the formatting lock-in. Themcp list --jsonblock above already does this correctly.🤖 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 113 - 170, Update the effectiveConfig assertion to parse effectiveConfig.stdout as JSON and assert the parsed state field equals "matched", replacing the exact spaced-string check. Keep the existing command execution, exit assertion, and secret-redaction checks unchanged.
🤖 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 58-85: Remove the ineffective HOST_SECRET and ROTATED_HOST_SECRET
assertions from the redacted restart result in the gateway lifecycle flow.
Validate secret absence at an unredacted boundary, or run a separate canary
probe that does not include these values in redactionValues, while preserving
the existing restart success and health assertions.
---
Nitpick comments:
In `@test/e2e/live/mcp-bridge-hermes-lifecycle.ts`:
- Around line 113-170: Update the effectiveConfig assertion to parse
effectiveConfig.stdout as JSON and assert the parsed state field equals
"matched", replacing the exact spaced-string check. Keep the existing command
execution, exit assertion, and secret-redaction checks unchanged.
🪄 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: 59831b14-33fa-48f8-a237-677620f93fe0
📒 Files selected for processing (4)
test/e2e/live/mcp-bridge-hermes-lifecycle.tstest/e2e/live/mcp-bridge-phases.tstest/e2e/live/mcp-bridge.test.tstest/hermes-mcp-transaction-restart.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/hermes-mcp-transaction-restart.test.ts
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
|
Maintainer waiver requested for an unrelated required-check flake on head
Please confirm whether this non-success check may be waived and the selected |
|
Superseded by #7761, which preserves this exact patch on current main with a fresh GitHub-verified branch because repository rules prohibit force-pushing this published branch. |
<!-- markdownlint-disable MD041 --> ## Summary Fix locked Hermes gateway restart after a managed MCP update. The root-owned restart controller now recognizes the guard-generated v1 MCP state record in the strict config hash while continuing to reject malformed, duplicate, unknown, or ambiguous records. The PR also adds a focused live regression covering managed add, shields restoration, real gateway restart, integrity verification, and failed-reload rollback. It replaces #7743 on current `main`; repository rules forbid force-pushing the published replacement branch. One narrow test-only timeout adjustment salvages a workflow-boundary check that twice exhausted Vitest's default 5-second envelope despite its child process already being bounded to 30 seconds. ## Related Issue Closes #7499 ## Changes - Parse zero or one exact `nemoclaw-hermes-mcp-state-v1` metadata record in the locked Hermes strict hash, preserving legacy two-record hashes. - Reject malformed, duplicate, or unknown metadata and duplicate config hash paths with `GATEWAY_CONFIG_HASH_MISMATCH`. - Exercise the existing Hermes MCP live target through managed add, shields restoration, supervisor-mediated restart, exact locked file/hash verification, and public transaction inspection. - Inject a first-reload failure and require the real rollback path to restore config and both anchors byte-for-byte, prove a healthy rollback reload, and confirm current state through the packaged public `inspect` flow. - Scan raw Hermes files and the startup log after restart to confirm the fake bridge secret is absent. - Give the existing workflow-boundary test a 40-second Vitest envelope around its already-bounded 30-second child process. ## 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: This restores the existing documented locked Hermes restart contract; the v1 hash record is an internal guard-generated implementation detail. - [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: Manual nine-category security review on `79767cd8c` passed all categories. The root ownership/mode checks remain unchanged; parsing is bounded ASCII with an exact known-record regex, and negative tests cover malformed, duplicate, unknown, and ambiguous input. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: The parser restores the existing locked Hermes restart contract while preserving fail-closed validation. The E2E follow-up only strengthens verification through public packaged behavior. - Agent: Codex Desktop `/root/replacement_docs_review` <!-- docs-review-head-sha: 79767cd --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — managed gateway control 4/4; Hermes MCP transaction 27/27; serial MCP live-selection E2E-support 41/41; workflow-boundary E2E-support 33/33; live Hermes target collection/import; semantic E2E phases 115 tests across 73 files; CLI typecheck; exact Vitest project-membership and test-size gates. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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) --- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Expanded end-to-end coverage for managed MCP bridge setup, including Hermes shield restoration across a locked gateway restart. * Added rollback validation that confirms a failed managed reload restores Hermes configuration and integrity anchors. * Tightened Hermes-related secret expectations and updated Hermes phase sequencing. * Added unit coverage for strict locked-hash parsing, covering legacy/current Hermes state variants and rejecting malformed/ambiguous inputs. * Increased timeout for workflow-to-PR reporting integration E2E. * **Bug Fixes** * Strengthened locked Hermes config-hash validation with strict state-line parsing and clearer mismatch handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Charan Jagwani <cjagwani@nvidia.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Carlos Villela <cvillela@nvidia.com>
1 participant
Summary
Adds a focused live regression for the existing managed HTTPS Hermes MCP path from an ordinary sandbox identity through normal shields relock, a real gateway restart, and a failed-reload rollback. The existing Hermes MCP target now follows the reported supported sequence and verifies gateway health plus post-restart and post-rollback integrity without changing production behavior.
Related Issue
Closes #7499
Changes
Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailableenv PATH=/usr/local/bin:$PATH npx vitest run --project integration test/hermes-mcp-config-transaction.test.ts(27 passed)npm run docsbuilds without warnings (doc changes only)Signed-off-by: Charan Jagwani cjagwani@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests