fix(skills): activate Hermes skills in fresh sessions - #8535
fix(skills): activate Hermes skills in fresh sessions#8535souvikDevloper wants to merge 13 commits into
Conversation
Refs NVIDIA#8504 Signed-off-by: souvikDevloper <138186578+souvikDevloper@users.noreply.github.com>
|
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 skill installation and removal now use new-session activation guidance. E2E coverage validates skill discovery, execution, fresh-session behavior, authenticated inference routing, and canary redaction. ChangesHermes skill lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HermesE2E
participant SkillInstaller
participant HermesSession
participant FakeInferenceServer
HermesE2E->>SkillInstaller: Install fixture skill
SkillInstaller-->>HermesE2E: Confirm installation
HermesE2E->>HermesSession: Start a new chat and invoke the skill
HermesSession->>FakeInferenceServer: Send authenticated request with canary
FakeInferenceServer-->>HermesE2E: Return request metadata with canary presence
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
2 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
2 additional E2E selections 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. 3 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: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
@apurvvkumaria hey can u please review it and let me know |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/e2e/fixtures/hermes-skill-runtime/SKILL.md (1)
1-14: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd the SPDX header after the YAML front matter.
parseFrontmatterrequires---on the first line. Add the required HTML SPDX header after the closing front-matter delimiter so skill installation continues to work.🤖 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/fixtures/hermes-skill-runtime/SKILL.md` around lines 1 - 14, Add the required HTML SPDX header immediately after the closing YAML front-matter delimiter in the Hermes skill definition, while keeping the opening delimiter on the first line and preserving the existing skill content unchanged.Source: Coding guidelines
🤖 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/e2e/fixtures/hermes-skill-runtime/SKILL.md`:
- Around line 1-14: Add the required HTML SPDX header immediately after the
closing YAML front-matter delimiter in the Hermes skill definition, while
keeping the opening delimiter on the first line and preserving the existing
skill content unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 802468e7-615d-4794-9d0d-ae399790f908
📒 Files selected for processing (16)
docs/reference/commands.mdxsrc/lib/actions/sandbox/skill-install.test.tssrc/lib/actions/sandbox/skill-install.tssrc/lib/skill-install.test.tssrc/lib/skill-install.tssrc/lib/skill-remote.test.tssrc/lib/skill-remote.tstest/e2e/fixtures/fake-openai-compatible.tstest/e2e/fixtures/hermes-skill-runtime/SKILL.mdtest/e2e/fixtures/inference-adapter.tstest/e2e/lib/fake-openai-compatible-api.mtstest/e2e/live/hermes-e2e-phases.tstest/e2e/live/hermes-e2e.test.tstest/e2e/live/hermes-skill-lifecycle.tstest/e2e/support/hosted-inference.test.tstest/e2e/support/inference-adapter.test.ts
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Please refresh this branch onto the current main commit and run the required PR workflows; all five required checks are missing. The Hermes fresh-session activation scope is bounded to existing skill behavior and includes source, docs, and live E2E coverage, so I found no additional blocking defect in the described contract.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed the requested changes in the current branch revision:
Validation completed after both changes:
Fresh required workflows are now running. The earlier Nemotron advisor failure was an optional partial second-opinion run; the primary advisor completed with no blocking findings. |
The branch refresh and fixture license correction address this review. Required checks remain separate from code-review findings.
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Maintainer refresh complete on the current branch. The Hermes skill install/removal guidance and live lifecycle proof were reviewed against the session-start reload hook, CLI behavior, generated agent documentation variants, and privacy-safe authenticated inference evidence. Validation passed for 65 CLI assertions, 23 E2E-support assertions, CLI typechecking, repository diff gates, and the documentation build (0 errors, 2 existing warnings). Independent documentation and nine-category security reviews passed with no findings, every commit is GitHub Verified, and there are no unresolved review threads. Fresh CI and human approval are still required before merge. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
@cv @apurvvkumaria The only remaining red check is the optional PR review advisor (Nemotron 3 Ultra). Its log shows a model-protocol failure during security-trust-analysis: the model omitted the required pr_review_read_terminology tool result. This is unrelated to the PR code; the primary GPT-5.6 Terra advisor and the publisher both passed, and GitHub reports no required checks for this branch. I tried to rerun only the failed job, but GitHub requires repository-admin permission. Could one of you rerun the failed job if you want the advisory badge cleared? Otherwise it is safe to ignore; human approval remains the merge blocker. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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/hermes-skill-lifecycle.ts`:
- Around line 47-131: The Hermes lifecycle setup must report the initial
preparation phase before cleanup begins. In the surrounding lifecycle setup that
invokes cleanupHermes("pre-cleanup"), call progress.phase("prepare clean Hermes
runner") immediately beforehand, using the existing progress reporter and
preserving the declared HERMES_E2E_PHASES phase name.
🪄 Autofix
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: 8cb3024b-60f8-4ec3-bdaa-3a2e3896b655
📒 Files selected for processing (16)
docs/reference/commands.mdxsrc/lib/actions/sandbox/skill-install.test.tssrc/lib/actions/sandbox/skill-install.tssrc/lib/skill-install.test.tssrc/lib/skill-install.tssrc/lib/skill-remote.test.tssrc/lib/skill-remote.tstest/e2e/fixtures/fake-openai-compatible.tstest/e2e/fixtures/hermes-skill-runtime/SKILL.mdtest/e2e/fixtures/inference-adapter.tstest/e2e/lib/fake-openai-compatible-api.mtstest/e2e/live/hermes-e2e-phases.tstest/e2e/live/hermes-e2e.test.tstest/e2e/live/hermes-skill-lifecycle.tstest/e2e/support/hosted-inference.test.tstest/e2e/support/inference-adapter.test.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- docs/reference/commands.mdx
- test/e2e/live/hermes-e2e-phases.ts
- src/lib/actions/sandbox/skill-install.ts
- src/lib/skill-remote.test.ts
- test/e2e/support/hosted-inference.test.ts
- test/e2e/live/hermes-e2e.test.ts
- test/e2e/fixtures/hermes-skill-runtime/SKILL.md
- src/lib/skill-install.ts
- test/e2e/support/inference-adapter.test.ts
- test/e2e/lib/fake-openai-compatible-api.mts
- test/e2e/fixtures/fake-openai-compatible.ts
- src/lib/skill-remote.ts
- src/lib/actions/sandbox/skill-install.test.ts
- src/lib/skill-install.test.ts
- test/e2e/fixtures/inference-adapter.ts
| export async function assertHermesSkillLifecycle({ | ||
| env, | ||
| host, | ||
| inference, | ||
| redactionValues, | ||
| sandboxName, | ||
| }: HermesSkillLifecycleOptions): Promise<void> { | ||
| const exec = async ( | ||
| args: string[], | ||
| artifactName: string, | ||
| remoteTimeoutSeconds = 60, | ||
| hostTimeoutMs = 90_000, | ||
| ) => { | ||
| const result = await host.command( | ||
| "nemohermes", | ||
| [sandboxName, "exec", "--no-stdin", "--timeout", String(remoteTimeoutSeconds), "--", ...args], | ||
| { artifactName, env, redactionValues, timeoutMs: hostTimeoutMs }, | ||
| ); | ||
| expect(result.exitCode, resultText(result)).toBe(0); | ||
| return result; | ||
| }; | ||
|
|
||
| const skillFixtureText = fs.readFileSync(path.join(HERMES_SKILL_FIXTURE, "SKILL.md"), "utf8"); | ||
| expect(skillFixtureText).toContain(E2E_MOCK_REQUEST_CANARY); | ||
| expect(HERMES_SKILL_PROMPT).not.toContain(E2E_MOCK_REQUEST_CANARY); | ||
| expect(HERMES_SKILL_PROMPT).not.toMatch(/PONG/i); | ||
|
|
||
| const skillInstall = await host.command( | ||
| "nemohermes", | ||
| [sandboxName, "skill", "install", HERMES_SKILL_FIXTURE], | ||
| { | ||
| artifactName: "phase-4-hermes-skill-install", | ||
| cwd: REPO_ROOT, | ||
| env, | ||
| redactionValues, | ||
| timeoutMs: 120_000, | ||
| }, | ||
| ); | ||
| expect(skillInstall.exitCode, resultText(skillInstall)).toBe(0); | ||
| expect(stripAnsi(resultText(skillInstall))).toContain(`Skill '${HERMES_SKILL_ID}' installed`); | ||
| expect(stripAnsi(resultText(skillInstall))).toContain( | ||
| "Start a new chat session to load the skill; a gateway restart is not required.", | ||
| ); | ||
|
|
||
| await exec( | ||
| ["test", "-f", `/sandbox/.hermes/skills/${HERMES_SKILL_ID}/SKILL.md`], | ||
| "phase-4-hermes-skill-disk-check", | ||
| ); | ||
| const skillList = await exec(["hermes", "skills", "list"], "phase-4-hermes-skills-list"); | ||
| expect(stripAnsi(resultText(skillList))).toContain(HERMES_SKILL_ID); | ||
|
|
||
| const sessionsBeforeSkill = await exec( | ||
| ["hermes", "sessions", "list"], | ||
| "phase-4-hermes-skill-sessions-before", | ||
| ); | ||
| const requestOffset = inference.requestSummaries()?.length; | ||
| const skillChat = await exec( | ||
| ["hermes", "chat", "--skills", HERMES_SKILL_ID, "--query", HERMES_SKILL_PROMPT, "--quiet"], | ||
| "phase-4-hermes-skill-chat", | ||
| 360, | ||
| 420_000, | ||
| ); | ||
| expect(stripAnsi(resultText(skillChat))).toMatch(/\bPONG\b/i); | ||
|
|
||
| const sessionsAfterSkill = await exec( | ||
| ["hermes", "sessions", "list"], | ||
| "phase-4-hermes-skill-sessions-after", | ||
| ); | ||
| expect( | ||
| onlyNewHermesSessionId( | ||
| hermesSessionIds(resultText(sessionsBeforeSkill)), | ||
| hermesSessionIds(resultText(sessionsAfterSkill)), | ||
| ), | ||
| ).toMatch(/^\d{8}_\d{6}_[a-zA-Z0-9]+$/); | ||
|
|
||
| if (requestOffset === undefined) return; | ||
| const skillRequests = (inference.requestSummaries() ?? []) | ||
| .slice(requestOffset) | ||
| .filter((request) => request.method === "POST" && INFERENCE_REQUEST_PATHS.has(request.path)); | ||
| expect(skillRequests.length).toBeGreaterThan(0); | ||
| expect( | ||
| skillRequests.some((request) => request.auth === "ok" && request.requestCanaryPresent === true), | ||
| "installed Hermes skill canary did not reach an authenticated mock inference request", | ||
| ).toBe(true); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline test/e2e/live/hermes-e2e.test.ts --items all
ast-grep outline test/e2e/live/hermes-e2e-phases.ts --items all
rg -n -C 5 '\bassertHermesSkillLifecycle\s*\(|meta\.e2ePhases|\.progress\b|progress\(' \
test/e2e/live/hermes-e2e.test.ts \
test/e2e/live/hermes-e2e-phases.ts \
test/e2e/live/hermes-skill-lifecycle.tsRepository: NVIDIA/NemoClaw
Length of output: 4751
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- phase definitions ---'
cat -n test/e2e/live/hermes-e2e-phases.ts
printf '%s\n' '--- caller around skill lifecycle ---'
sed -n '430,535p' test/e2e/live/hermes-e2e.test.ts
printf '%s\n' '--- test metadata and progress API ---'
rg -n -C 8 'HERMES_E2E_PHASES|e2ePhases|progress' test/e2e/live test/e2e/fixturesRepository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Hermes phase list ---'
cat -n test/e2e/live/hermes-e2e-phases.ts
printf '%s\n' '--- Hermes test metadata, callback, and lifecycle call ---'
rg -n -C 12 'HERMES_E2E_PHASES|e2ePhases|async \(\{|assertHermesSkillLifecycle|progress\.phase' \
test/e2e/live/hermes-e2e.test.ts
printf '%s\n' '--- phase names and progress calls in the Hermes test ---'
python3 - <<'PY'
from pathlib import Path
p = Path("test/e2e/live/hermes-e2e.test.ts")
for i, line in enumerate(p.read_text().splitlines(), 1):
if "HERMES_E2E_PHASES" in line or "e2ePhases" in line or "progress.phase" in line or "assertHermesSkillLifecycle" in line:
print(f"{i}: {line}")
PYRepository: NVIDIA/NemoClaw
Length of output: 11540
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- E2E phase guidance ---'
rg -n -C 6 'e2ePhases|progress\.phase|phase coverage|phase checker|semantic phase' \
test/e2e/README.md test/e2e tools
printf '%s\n' '--- checker implementation and references ---'
rg -n -C 8 'e2ePhases|progress\.phase|phase coverage' \
--glob '*.ts' --glob '*.mts' --glob '*.js' --glob '*.mjs' \
test toolsRepository: NVIDIA/NemoClaw
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- guide references ---'
rg -n -C 10 'e2ePhases|progress\.phase|phase coverage|checker' test/e2e/README.md
printf '%s\n' '--- candidate checker files ---'
rg -l 'e2ePhases|phase coverage|progress\.phase' test tools \
--glob '*.ts' --glob '*.mts' --glob '*.js' --glob '*.mjs' |
while read -r file; do
if rg -q 'checker|coverage|e2ePhases' "$file"; then
printf '%s\n' "$file"
fi
done
printf '%s\n' '--- exact Hermes phase references ---'
rg -n 'HERMES_E2E_PHASES|meta: \{ e2ePhases|progress\.phase' \
test/e2e/live/hermes-e2e.test.ts test/e2e/live/hermes-e2e-phases.tsRepository: NVIDIA/NemoClaw
Length of output: 8282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import ast
import re
from pathlib import Path
phase_file = Path("test/e2e/live/hermes-e2e-phases.ts").read_text()
test_file = Path("test/e2e/live/hermes-e2e.test.ts").read_text()
declared = re.findall(r'^\s*"([^"]+)",\s*$', phase_file, re.MULTILINE)
reported = re.findall(r'progress\.phase\("([^"]+)"\)', test_file)
print("declared:", declared)
print("reported:", reported)
assert declared == reported, "Declared and reported Hermes phases differ"
print("phase coverage: OK")
PYRepository: NVIDIA/NemoClaw
Length of output: 933
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '238,320p' test/e2e/live/hermes-e2e.test.ts
printf '%s\n' '--- all progress API usages in the setup block ---'
rg -n -C 5 'progress|prepare clean Hermes runner|preClean|cleanup|dockerInfo|probeModels' \
test/e2e/live/hermes-e2e.test.ts | head -n 180Repository: NVIDIA/NemoClaw
Length of output: 9674
Report the initial Hermes preparation phase. Call progress.phase("prepare clean Hermes runner") before cleanupHermes("pre-cleanup"). HERMES_E2E_PHASES declares this phase, but the test reports only the remaining six phases.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 68-68: Filesystem path is not a string literal; a request-/variable-derived path can enable path traversal. Validate and normalize the path before use.
Context: fs.readFileSync(path.join(HERMES_SKILL_FIXTURE, "SKILL.md"), "utf8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(detect-non-literal-fs-filename-typescript)
🤖 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/hermes-skill-lifecycle.ts` around lines 47 - 131, The Hermes
lifecycle setup must report the initial preparation phase before cleanup begins.
In the surrounding lifecycle setup that invokes cleanupHermes("pre-cleanup"),
call progress.phase("prepare clean Hermes runner") immediately beforehand, using
the existing progress reporter and preserving the declared HERMES_E2E_PHASES
phase name.
Source: Path instructions
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
test/e2e/live/hermes-e2e.test.ts:300 starts cleanupHermes("pre-cleanup") before reporting the declared prepare clean Hermes runner phase from test/e2e/live/hermes-e2e-phases.ts. This leaves the first destructive cleanup boundary outside the semantic phase log, so its duration and failure cannot be attributed to the declared phase. Call progress.phase("prepare clean Hermes runner") immediately before the cleanup call, then run npm run test:e2e-phases:check as regression evidence.
3 participants
Summary
Correct Hermes skill activation guidance so installs, updates, and removals take effect in a new chat session without a gateway restart. Add a public-CLI live E2E proof for install, native discovery, and use in a fresh Hermes session, with privacy-safe evidence that the skill reached an authenticated inference request.
This is a bounded foundation for the design-gated epic. It does not select or claim support for an Enterprise AI reference stack.
Related Issue
Refs #8504
Changes
nemohermes, verify its sandbox path and nativehermes skills listdiscovery, and use it in a newly createdhermes chat --skillssession.Root cause and prevention
The generic non-OpenClaw path assumed that every agent needed a gateway restart, even though the shipped NemoClaw Hermes plugin reloads skills at session start. Existing tests covered the Hermes install destination but did not cover activation guidance or a live install-to-inference path. Capability unit tests and the new live lifecycle assertion protect both gaps.
Epic scope boundary
This PR does not select a partner control plane, define Team Wisdom evaluation or artifact promotion, validate tenant isolation/privacy/persistence, publish a reference composition, or close #8504. The epic still needs named and versioned candidates with owners, selection gates and weighted dogfood evidence, a frozen ADR/RACI/data-flow design, an immutable skill evaluation/approval/promotion state machine with rollback receipts, a threat model, and publication/support decisions.
Type of Change
Quality Gates
Documentation Writer Review
docs-updateddocs/reference/commands.mdx; reviewed the final Hermes-only install and removal guidance against the session-start reload hook, CLI output, tests, generated agent command variants, navigation, and documentation exclusions.npm run docspassed with 0 errors and 2 existing warnings.DGX Station Hardware Evidence
Not applicable;
scripts/prepare-dgx-station-host.shis unchanged.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 testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changesnpm run docsbuilds without warnings (doc changes only)Validation details
vitest --project cli src/lib/skill-install.test.ts src/lib/skill-remote.test.ts: 52 passed.vitest --project e2e-support test/e2e/support/inference-adapter.test.ts: 9 passed.validate:prrun reached platform-only checkout failures in unchanged files: executable-bit/digest checks and CRLF parsing ofscripts/check-version-tag-sync.sh. Linux CI is the broad gate.Signed-off-by: souvikDevloper 138186578+souvikDevloper@users.noreply.github.com
Summary by CodeRabbit
New Features
Documentation