fix(skills): activate Hermes skills in fresh sessions - #8535
fix(skills): activate Hermes skills in fresh sessions#8535souvikDevloper wants to merge 5 commits into
Conversation
Refs NVIDIA#8504 Signed-off-by: souvikDevloper <138186578+souvikDevloper@users.noreply.github.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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughHermes skill installation and removal now use new-session activation guidance. New 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 — InformationalAdvisor assessment: Informational / low confidence Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 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.
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.mdxDGX 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