fix(hermes): preserve MCP rebuild restart evidence - #8679
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughGateway restart results now distinguish MCP reconciliation refusals from supervisor integrity refusals. Hermes post-restore handling accepts successful restart evidence and continues MCP verification. ChangesHermes MCP restart handling
Estimated code review effort: 1 (Trivial) | ~5 minutes Sequence Diagram(s)sequenceDiagram
participant HermesPostRestore
participant GatewayRestart
participant MCPReconciliation
HermesPostRestore->>GatewayRestart: restart gateway after state restore
GatewayRestart->>MCPReconciliation: reconcile managed MCP intent
MCPReconciliation-->>GatewayRestart: refusal with restarted and healthPassed
GatewayRestart-->>HermesPostRestore: restart result
HermesPostRestore->>HermesPostRestore: classify result and continue verification
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
1 terminology difference from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
1 additional E2E selection from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. Since last review: 0 prior items resolved · 0 still apply · 0 new items found 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
<!-- markdownlint-disable MD041 --> ## Summary Align the Hermes restart-drift regression test with the explicit restart and health evidence added by #8679. This fixes the exact-result assertion that failed after the production behavior merged. ## Related Issue Follow-up to #8679 and #8671. ## Changes - Expect `restarted: true` after the supervisor returns a valid restart marker. - Expect `healthPassed: true` after the recovered Hermes gateway passes its health check. - Preserve the assertions that no post-refusal mutations run and secret-like output remains redacted. ## 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 changes only an exact test expectation for behavior already merged in #8679; runtime behavior, CLI output, configuration, and recovery guidance are unchanged. - [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: Codex security review passed with no findings; the test continues to require restart and health evidence, forbid post-refusal mutations, and verify redaction. - [ ] 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 change is assertion-only and does not alter user-facing behavior or existing recovery guidance. - Agent: Codex Desktop <!-- docs-review-head-sha: feb2e02 --> <!-- docs-review-agents-blob-sha: c4923a3 --> ## 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 validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: affected CLI suites passed 33/33 under Python 3.14; the #8678 stale-recovery case passed 1/1 on the updated base; CLI type-check passed. - [ ] 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: Apurv Kumaria <akumaria@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Summary
Hermes rebuilds no longer fail after a healthy gateway replacement merely because the generic restart checks persisted MCP intent before the rebuild restores the managed adapter projection. The restart result now carries explicit process-replacement and health evidence, so only that expected transitional mismatch can be superseded by MCP restoration; final MCP reconciliation remains mandatory and fail-closed.
Related Issue
Fixes #8671
Changes
restarted: trueandhealthPassed: trueonly when Hermes MCP reconciliation refuses after the supervisor restart marker and gateway health check have both passed.The result provenance is required because
MCP reconciliation refusalis also used for supervisor-side failures that occur before process replacement. The rebuild consumer therefore requires both explicit proofs instead of weakening reconciliation based on the failure label alone.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededdocs/manage-sandboxes/recover-rebuild-sandboxes.mdxanddocs/manage-sandboxes/manage-mcp-servers.mdxalready document the corrected lifecycle and provider-backed MCP restoration.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm run typecheck:cli: passed.npx prek run --from-ref main --to-ref HEAD: passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable to this localized result-contract change. A localnpm run test:fastattempt was not claimed because an isolated untouched DCode finalization test currently fails two unrelated preference-merge assertions.npm run docsbuilds without warnings (doc changes only)The canonical live
mcp-bridge (hermes)lane was not available for a PR branch because it requires the trusted main-only Hermes swap. CI and the next trusted-main lane provide the remaining process-level proof.Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit