refactor(hermes): add versioned CLI adapter - #8025
Conversation
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
jyaunches
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:
📝 WalkthroughWalkthroughHermes wrapper parsing now uses a versioned CLI adapter manifest. Build validation checks the manifest against Hermes parser metadata and public help commands. The wrapper validates versions, translates managed invocations, preserves pass-through behavior, and rejects ambiguous or incompatible inputs. ChangesHermes adapter refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HermesWrapper
participant HermesAdapter
participant HermesCLI
HermesWrapper->>HermesAdapter: Resolve and validate adapter
HermesWrapper->>HermesAdapter: Parse managed invocation
HermesWrapper->>HermesCLI: Verify upstream version
HermesWrapper->>HermesCLI: Execute translated or pass-through command
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit e478046 in the TypeScript / code-coverage/cliThe overall coverage in commit e478046 in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
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. |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/hermes-wrapper-provider-merge.test.ts (1)
254-272: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument the breaking behavior change.
Document that multi-word
--resumeor--continuesession names combined with--providerand--modelexit with status 2. State that users must pass the session name as one quoted argument. Add this change to the release notes.🤖 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-wrapper-provider-merge.test.ts` around lines 254 - 272, Update the release notes to document the breaking behavior covered by the test “rejects ambiguous session text before provider/model flags owned by another command (`#8011`)”: multi-word --resume or --continue session names combined with --provider and --model now exit with status 2, and users must pass the session name as one quoted argument.
🧹 Nitpick comments (7)
test/hermes-wrapper-oneshot-routing.test.ts (1)
254-254: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the probe version from the adapter instead of a literal.
The fixture hard-codes
0.19.0. The adapter fieldupstream_cli_versionalready holds that value, and the harness reads its own default separately. Read the version from the adapter JSON so an adapter bump does not require edits in two test fixtures.♻️ Proposed change
+const upstreamVersion = JSON.parse(fs.readFileSync(ADAPTER, "utf-8")).upstream_cli_version; ... - 'if [ "${NEMOCLAW_HERMES_ADAPTER_VERSION_PROBE:-}" = "1" ]; then printf "Hermes Agent v0.19.0\\n"; exit 0; fi', + `if [ "\${NEMOCLAW_HERMES_ADAPTER_VERSION_PROBE:-}" = "1" ]; then printf 'Hermes Agent v${upstreamVersion}\\n'; exit 0; fi`,🤖 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-wrapper-oneshot-routing.test.ts` at line 254, Update the probe fixture command in the oneshot routing test to derive the reported Hermes version from the adapter JSON field upstream_cli_version instead of hard-coding 0.19.0, while preserving the existing probe output and control flow.agents/hermes/hermes-wrapper.py (2)
529-534: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the sentinel exception classes above their first raise site.
_parse_managed_invocationraises_AmbiguousProviderModelSessionat Line 453 and_translate_resumed_oneshotraises_UnsupportedResumedOneshotUsageFileat Line 500. Both classes are defined after those functions. Runtime resolution is correct, but the reading order hides the control flow. Place both classes next to_CliAdapterErrorat Line 284.🤖 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 `@agents/hermes/hermes-wrapper.py` around lines 529 - 534, Move _UnsupportedResumedOneshotUsageFile and _AmbiguousProviderModelSession from their current location to immediately alongside _CliAdapterError, before _parse_managed_invocation and _translate_resumed_oneshot can raise them; preserve their class names, inheritance, and docstrings unchanged.
602-636: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated compatibility-probe and refusal pattern.
The two refusal handlers repeat the same three steps: probe the upstream version, print a
[COMPATIBILITY]diagnostic on failure, and return 2. A small helper that takes the refusal message removes the duplication and keeps the exit status consistent.♻️ Proposed refactor
+def _refuse(real_hermes: str, adapter: dict, message: str) -> int: + try: + _require_upstream_cli_version(real_hermes, adapter["upstream_cli_version"]) + except _CliAdapterError as exc: + print(f"[COMPATIBILITY] Refusing to run hermes: {exc}", file=sys.stderr) + return 2 + print(message, file=sys.stderr) + return 2Then each handler becomes a single
return _refuse(real_hermes, adapter, "...")call.🤖 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 `@agents/hermes/hermes-wrapper.py` around lines 602 - 636, Extract the repeated upstream-version probe and refusal handling from the _UnsupportedResumedOneshotUsageFile and _AmbiguousProviderModelSession handlers into a small helper that accepts the refusal message, performs _require_upstream_cli_version, prints the compatibility error on _CliAdapterError, and returns 2. Replace both handlers with a single return call to this helper while preserving their existing refusal messages and exit status.test/hermes-final-image-layout.test.ts (1)
324-324: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe chmod assertion now pins only the command prefix.
indexOfRequired(finalStage, "RUN chmod 755 \\")locates the instruction but no longer pins which paths that instruction normalizes. The mode contracts at Lines 351-352 still assert the final modes, so the coverage gap is small. If you want the normalization list pinned here, assert the continuation lines for the adapter validator path as well.🤖 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-final-image-layout.test.ts` at line 324, Update the chmod assertion around modeNormalize in hermes-final-image-layout.test.ts to also verify the continuation line containing the adapter validator path, preserving the existing final-mode assertions while pinning the complete normalization list.test/hermes-wrapper-provider-merge.test.ts (1)
337-349: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering the ambiguous form with only one of the two flags.
The wrapper raises
_AmbiguousProviderModelSessiononly when a session flag appears with both--providerand--model. An unquoted multi-word session name with just--providerstill passes through. No test pins that boundary. A single case would lock the condition at agents/hermes/hermes-wrapper.py Lines 448-453.🤖 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-wrapper-provider-merge.test.ts` around lines 337 - 349, Add a test in the provider/model passthrough suite covering an unquoted multi-word session name supplied with only --provider, and assert the wrapper’s expected passthrough behavior without raising _AmbiguousProviderModelSession. Keep the existing single-flag tests and target the ambiguity handling around the wrapper’s provider/model session parsing logic.agents/hermes/validate-cli-adapter.py (1)
21-27: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winArity validation has a blind spot for
preparseoptions and forrequiredvs.session.
_validate_actionmaps both"required"and"session"arities to the same expectednargs(None), so it cannot detect a mix-up between them (for example, ifresumewere mistakenly declared"required"instead of"session", the check at Line 37 still passes). This is largely inherent, since Hermes' argparse metadata has no concept of NemoClaw's multi-word session coalescing.More directly fixable: the
preparsebranch (Lines 104-112) never calls_validate_actionand never checksoption["arity"]at all. It only checks that each name takes a value (preparse.get(name) is not True). Ifprofile's declared arity in the contract were wrong (for example"boolean"instead of"required"), this validator would not catch it, even though the file's stated purpose is validating the contract against Hermes' parser metadata.Add an explicit arity check to the preparse branch, for example asserting
option["arity"] == "required"for preparse-surfaced options, so a bad arity value there is caught the same way it would be fortop/chatsurfaces.Also applies to: 29-42, 104-112
🤖 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 `@agents/hermes/validate-cli-adapter.py` around lines 21 - 27, Update the preparse-option validation branch in the contract validation flow to explicitly require each preparse option’s declared arity to be "required", alongside its existing value-taking check. Use the same assertion/error behavior as the top/chat arity validation so incorrect metadata such as "boolean" is rejected; leave the separate required-versus-session limitation unchanged.agents/hermes/Dockerfile (1)
903-906: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRemove
validate-hermes-cli-adapter.pyfrom the final image.The validator runs only during the image build. No runtime path invokes it. Remove it after build-time validation and replace its final
check_metadataassertion withcheck_absent.🤖 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 `@agents/hermes/Dockerfile` around lines 903 - 906, Update the final-image metadata checks in the Dockerfile: remove /usr/local/lib/nemoclaw/validate-hermes-cli-adapter.py after build-time validation, and replace its final check_metadata assertion with check_absent. Keep the other metadata 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 `@agents/hermes/Dockerfile`:
- Around line 427-446: Move the SHA-256 integrity check in the Dockerfile so it
runs before invoking validate-hermes-cli-adapter.py. Keep all three integrity
inputs—hermes-wrapper.py, hermes-cli-adapter-v1.json, and
validate-hermes-cli-adapter.py—in that pre-execution gate, preserving the
existing fail-closed mismatch behavior and then running the validator only after
verification succeeds.
---
Outside diff comments:
In `@test/hermes-wrapper-provider-merge.test.ts`:
- Around line 254-272: Update the release notes to document the breaking
behavior covered by the test “rejects ambiguous session text before
provider/model flags owned by another command (`#8011`)”: multi-word --resume or
--continue session names combined with --provider and --model now exit with
status 2, and users must pass the session name as one quoted argument.
---
Nitpick comments:
In `@agents/hermes/Dockerfile`:
- Around line 903-906: Update the final-image metadata checks in the Dockerfile:
remove /usr/local/lib/nemoclaw/validate-hermes-cli-adapter.py after build-time
validation, and replace its final check_metadata assertion with check_absent.
Keep the other metadata checks unchanged.
In `@agents/hermes/hermes-wrapper.py`:
- Around line 529-534: Move _UnsupportedResumedOneshotUsageFile and
_AmbiguousProviderModelSession from their current location to immediately
alongside _CliAdapterError, before _parse_managed_invocation and
_translate_resumed_oneshot can raise them; preserve their class names,
inheritance, and docstrings unchanged.
- Around line 602-636: Extract the repeated upstream-version probe and refusal
handling from the _UnsupportedResumedOneshotUsageFile and
_AmbiguousProviderModelSession handlers into a small helper that accepts the
refusal message, performs _require_upstream_cli_version, prints the
compatibility error on _CliAdapterError, and returns 2. Replace both handlers
with a single return call to this helper while preserving their existing refusal
messages and exit status.
In `@agents/hermes/validate-cli-adapter.py`:
- Around line 21-27: Update the preparse-option validation branch in the
contract validation flow to explicitly require each preparse option’s declared
arity to be "required", alongside its existing value-taking check. Use the same
assertion/error behavior as the top/chat arity validation so incorrect metadata
such as "boolean" is rejected; leave the separate required-versus-session
limitation unchanged.
In `@test/hermes-final-image-layout.test.ts`:
- Line 324: Update the chmod assertion around modeNormalize in
hermes-final-image-layout.test.ts to also verify the continuation line
containing the adapter validator path, preserving the existing final-mode
assertions while pinning the complete normalization list.
In `@test/hermes-wrapper-oneshot-routing.test.ts`:
- Line 254: Update the probe fixture command in the oneshot routing test to
derive the reported Hermes version from the adapter JSON field
upstream_cli_version instead of hard-coding 0.19.0, while preserving the
existing probe output and control flow.
In `@test/hermes-wrapper-provider-merge.test.ts`:
- Around line 337-349: Add a test in the provider/model passthrough suite
covering an unquoted multi-word session name supplied with only --provider, and
assert the wrapper’s expected passthrough behavior without raising
_AmbiguousProviderModelSession. Keep the existing single-flag tests and target
the ambiguity handling around the wrapper’s provider/model session parsing
logic.
🪄 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: 919e69c9-81a8-4eae-88eb-5c3c7d2950e5
📒 Files selected for processing (14)
.agents/skills/nemoclaw-contributor-update-hermes/SKILL.md.agents/skills/nemoclaw-contributor-update-hermes/references/hermes-contract-map.mdagents/hermes/Dockerfileagents/hermes/hermes-cli-adapter-v1.jsonagents/hermes/hermes-wrapper.pyagents/hermes/image-build-probes.pyagents/hermes/validate-cli-adapter.pytest/helpers/hermes-wrapper-harness.tstest/hermes-dependency-review.test.tstest/hermes-doctor-config-hash.test.tstest/hermes-final-image-layout.test.tstest/hermes-image-build-probes.test.tstest/hermes-wrapper-oneshot-routing.test.tstest/hermes-wrapper-provider-merge.test.ts
💤 Files with no reviewable changes (2)
- test/hermes-image-build-probes.test.ts
- agents/hermes/image-build-probes.py
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 `@agents/hermes/hermes-wrapper.py`:
- Around line 411-412: Update _adapt_cli_argv so an empty separated managed
session selector is rejected rather than returning the passthrough result,
causing main to exit with status 2 without executing Hermes. Adjust the existing
pass-through tests to assert both the nonzero exit status and that Hermes was
not invoked.
🪄 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: aaec26c6-0c66-455b-9cf7-8027e33b8208
📒 Files selected for processing (2)
agents/hermes/Dockerfileagents/hermes/hermes-wrapper.py
🚧 Files skipped from review as they are similar to previous changes (1)
- agents/hermes/Dockerfile
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
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/hermes-final-image-layout.test.ts`:
- Around line 387-394: Strengthen the test in “verifies CLI adapter integrity
before executing its validator” so it proves the integrity gate actually blocks
validator execution, rather than relying on the first occurrence of the mismatch
string. Match the digest comparison and its failure action, or exercise the
mismatch path through the public image-build boundary and assert that
validate-hermes-cli-adapter.py is not invoked.
🪄 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: e26b29b6-573e-4b46-a154-5f623fcde72b
📒 Files selected for processing (5)
agents/hermes/Dockerfileagents/hermes/hermes-cli-adapter-v1.jsonci/source-shape-test-budget.jsontest/hermes-dependency-review.test.tstest/hermes-final-image-layout.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/hermes-dependency-review.test.ts
- agents/hermes/Dockerfile
- agents/hermes/hermes-cli-adapter-v1.json
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
Review follow-up: declining the outside-diff request to add release notes for rejecting an unquoted multi-word session selector combined with provider/model flags. That form is ambiguous shell input rather than a supported CLI contract; issue #8011 explicitly requires the adapter to fail closed instead of risking a mis-translation. Quoted session names remain supported, the behavior is covered by focused tests, and the independent documentation writer confirmed that no user-facing docs page or release note is required. The remaining CodeRabbit nitpicks are non-actionable cleanup suggestions and are intentionally omitted to keep this refactor narrow. |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
|
🌿 Preview your docs: https://nvidia-preview-pr-8025.docs.buildwithfern.com/nemoclaw |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary The risk plan on `main` does not select `mcp-bridge` for Hermes CLI adapter changes. PR #8025 therefore cannot run the live E2E coverage required by issue #8011 before this selector lands. This change adds the required jobs and increments the risk-plan version to 12. Julie Yaunches authored the first commit and the risk-plan wording clarification. Carlos Villela applied the review fixes and authored the compatibility-digest update. GitHub reports all three commits as Verified. ## Related Issue Related to #8011. Unblocks #8025. ## Changes - Select `channels-stop-start` and `mcp-bridge` when the Hermes wrapper, adapter manifest, or adapter validator changes. - Keep all managed-policy and sandbox-boundary jobs selected when the Hermes wrapper changes. - Add `hermes-inference-switch` to the Hermes sandbox-boundary jobs. - Increment the deterministic risk-plan version to 12. - Assert the complete job list for each adapter and managed-policy path. - Document the selector and the messaging-channel and managed MCP lifecycle coverage. - Update the compatibility digest for the version 12 risk-plan output. ## 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 PR changes trusted E2E selection, not a user-visible command or supported behavior. Internal E2E and advisor documentation was updated. - [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 Desktop reviewed commit `34af730876508d272c1dbd15a3755a35a0815cab` against base SHA `4cd4d64fe67143b57707f874afa0b9d269dfeff2`, including the functional changes and compatibility-digest correction. All nine categories received PASS, with no findings. Commit `cc3f505dc941b3dc3c26578724b413491e014bb9` changes only independently reviewed explanatory wording. [Security review](#8107 (comment)). - [ ] 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: `docs-updated` - Evidence: `test/e2e/README.md` and `tools/pr-review-advisor/README.md` document risk-plan version 12 and the Hermes adapter lifecycle mapping. The documentation writer review at head SHA `cc3f505dc` covered terminology, structure, voice, test titles, and code-sample presentation. The focused Vitest command passed 87 tests in 2 files, and the repository hooks passed. - Agent: Codex Desktop <!-- docs-review-head-sha: cc3f505 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable - Station profile/scenario: Not applicable - Result: This PR does not change `scripts/prepare-dgx-station-host.sh`. - Supporting evidence: Not applicable ## 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 - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npm exec -- vitest run test/pr-risk-plan.test.ts test/e2e/support/e2e-cross-runtime-compatibility.test.ts --reporter=dot` passed 87 tests in 2 files at commit `cc3f505dc`. - [x] Applicable broad gate passed — `E2E / PR Gate` passed for commit `cc3f505dc` in [run 30824493943](https://github.com/NVIDIA/NemoClaw/actions/runs/30824493943). - [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: Julie Yaunches <jyaunches@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Expanded automated validation for Hermes CLI adapters, managed policies, sandbox boundaries, and wrapper workflows. * Added checks to preserve provider and model settings during managed inference route changes. * Updated compatibility checks and targeted test-selection expectations. * **Chores** * Updated risk-plan metadata and documentation to reflect the expanded validation coverage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> Signed-off-by: Carlos Villela <cvillela@nvidia.com> Co-authored-by: Julie Yaunches <jyaunches@nvidia.com>
# Conflicts: # test/e2e/README.md # test/pr-risk-plan.test.ts # tools/advisors/risk-plan.mts # tools/pr-review-advisor/README.md
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Approve — reviewed exact head e478046. The versioned adapter consolidates the two owned Hermes 0.19 translations, validates its schema against the pinned parser and coalescer source at image build time, verifies integrity before validator execution, and checks the installed CLI version before rewriting argv. Ambiguous session/provider forms and incompatible source shapes fail closed; passthrough, delimiter, profile, provider/model, resumed one-shot, lifecycle, and live routing behavior have focused coverage. Exact-head CI and all 19 selected E2E families passed. The failed second-opinion advisor lane reported no blocker and does not indicate a product defect.
4 participants
Summary
Replace duplicated Hermes CLI flag and command inventories with one versioned adapter contract. The wrapper now parses each managed invocation once, verifies the installed Hermes version before translation, and fails closed when a session/provider form is ambiguous.
Related Issue
Fixes #8011
Changes
hermes-cli-adapter-v1.jsonas the single contract for the two Hermes 0.19 translations, including exact forms, rationale, and removal conditions.chatsurfaces, not compatibility authority.--as the end of options when detecting managed translation inputs.--usage-file, named-profile translation, and provider/model routing in focused and live tests.channels-stop-startandmcp-bridgewhen the CLI wrapper, adapter manifest, or adapter validator changes.Type of Change
Quality Gates
506153fb451010cca465be105f9fec542f2aeb37; the nine-category review found no findings. Adapter and coalescer inputs use fixed root-owned paths, SHA-256 gates run before validator execution,ast.literal_evalreads only the pinned source's literal boundary set, and incompatible source shapes fail closed. The exact-head E2E delta adds two fixed job IDs for three fixed source paths and cannot select arbitrary workflows. It adds no dependency, port, permission, credential, or network-policy surface.Documentation Writer Review
docs-updatede4780467e. All PR-path blobs are unchanged from the previously reviewed adapter head. The merged Hermes rebuild behavior preserves only allowlisted home-channel assignments and remains consistent with the PR's credential-boundary and adapter documentation. Risk-plan version 12 selected 19 E2E check families for the exact head, includingchannels-stop-startandmcp-bridge. Focused integration validation passed 102 tests with 69 platform-gated skips; isolated cross-runtime compatibility passed 2/2;git diff --check origin/main...HEADpassed.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 exec -- vitest run --project integration test/hermes-cli-adapter-validator.test.ts test/hermes-final-image-layout.test.ts test/hermes-wrapper-oneshot-routing.test.ts test/hermes-wrapper-provider-merge.test.ts test/hermes-home-channel-snapshot.test.ts test/pr-risk-plan.test.tspassed 102 tests with 69 platform-gated skips.npm exec -- vitest run test/e2e/support/e2e-cross-runtime-compatibility.test.ts --reporter=dotpassed 2/2. The adapter risk-plan selector assertions passed.channels-stop-startvariants passed; Hermes, OpenClaw, and DeepAgentsmcp-bridgevariants passed.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Julie Yaunches jyaunches@nvidia.com