fix(cli): mirror installed OpenClaw skill into agent home dir (Fixes #4819)#4848
Conversation
…4819) Skill install uploaded only to the OpenClaw state dir (uploadDir) and the post-install step refreshed sessions but never created the home mirror at $HOME/.openclaw/skills/<name>, despite the call-site comment and `skill remove` both treating that mirror as authoritative. On sandboxes where the agent's $HOME differs from the OpenClaw state dir, `openclaw skills list` reads the state dir (so the skill appears installed) while the agent loads skills from $HOME/.openclaw/skills at session start (empty) — so a freshly installed skill is listed but never invoked. postInstall now mirrors the uploaded skill into the home dir for OpenClaw, symmetric with removeSkill. The copy is a guarded no-op (-ef test) when both paths resolve to the same directory, the common case where $HOME is the state dir's parent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: jason <jama@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 (3)
📝 WalkthroughWalkthroughThe PR adds a mirror installation step to the OpenClaw skill deployment flow. After uploading a skill, ChangesOpenClaw skill mirror installation
Sequence DiagramsequenceDiagram
participant postInstall
participant uploadStep as Upload Step
participant mirrorStep as Mirror Step
participant sshExec
participant sessionRefresh
postInstall->>uploadStep: Upload skill to uploadDir
uploadStep-->>postInstall: upload complete
postInstall->>mirrorStep: Check paths.mirrorDir
alt mirrorDir set
mirrorStep->>sshExec: Execute mkdir -p, rm -rf, cp -a
alt SSH succeeds
sshExec-->>mirrorStep: command success
else SSH fails
sshExec-->>mirrorStep: command failure
mirrorStep->>postInstall: append warning message
end
end
postInstall->>sessionRefresh: Refresh sessions (unless skipRefresh)
sessionRefresh-->>postInstall: refresh complete
postInstall-->>postInstall: return success: true
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
E2E Scenario Advisor RecommendationRequired scenario E2E: None Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
PR Review AdvisorFindings: 0 needs attention, 3 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
Address review on #4848: postInstall only warned on mirror-copy failure and still returned success, while verifyInstall checked just uploadDir, so a failed mirror reported "installed" even though the agent (which loads from $HOME/.openclaw/skills) still could not see the skill. verifyInstall now also requires SKILL.md at the OpenClaw home mirror, symmetric with verifyRemove. A failed mirror copy therefore fails install verification and the CLI reports it (with a mirror-aware message) instead of falsely succeeding. verifyInstall gains an injectable sshExecImpl for tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: jason <jama@nvidia.com>
|
Addressed the PR Review Advisor findings in 8c778e2. "Install can report success even when the home mirror was not created" (worth checking): Correct and important — it would have left the linked bug unfixed while reporting success. "Source-of-truth review" (worth checking): The mirror is intentionally a second authoritative location rather than a relocation of the source of truth. E2E Advisor: Agreed that |
## Summary - Adds the `v0.0.60` section to `docs/about/release-notes.mdx` using the dev announcement from discussion #4877. - Fills the source-doc gaps found during release-prep review across inference, policy tiers, command behavior, security boundaries, Hermes dashboard/tooling, runtime context, and troubleshooting. - Refreshes generated agent skills under `.agents/skills/` from the current Fern docs output and upgrades Fern from `5.44.3` to `5.45.0`. ## Source summary - #4037 -> `docs/reference/architecture.mdx`, `docs/about/how-it-works.mdx`, `docs/about/release-notes.mdx`: Documents system-only runtime context that stays out of visible chat. - #4875 -> `docs/reference/architecture.mdx`, `docs/about/how-it-works.mdx`, `docs/about/release-notes.mdx`: Documents try-first sandbox network/filesystem guidance and clearer failure classification. - #4788 -> `docs/security/best-practices.mdx`, `docs/about/release-notes.mdx`: Documents shared OpenClaw device-approval policy for startup and connect. - #4768 -> `docs/reference/network-policies.mdx`, `docs/network-policy/integration-policy-examples.mdx`, `docs/get-started/quickstart.mdx`, `docs/get-started/quickstart-hermes.mdx`, `docs/reference/commands.mdx`: Documents `weather`, `public-reference`, and Hermes managed-tool gateway preset behavior. - #3788 and #4864 -> `docs/reference/network-policies.mdx`, `docs/reference/commands.mdx`: Documents non-interactive policy-tier fail-fast behavior and interactive prompt fallback. - #4756 and #4866 -> `docs/reference/commands.mdx`: Documents env-aware default sandbox resolution for `list`, `status`, and `tunnel` commands. - #4320 -> `docs/reference/commands.mdx`: Documents `$$nemoclaw tunnel status` behavior. - #4328 -> `docs/reference/commands.mdx`: Documents line-scoped policy preset descriptions in `policy-list`. - #4580 and #4748 -> `docs/reference/architecture.mdx`: Documents package-managed OpenShell gateway service and Docker-driver gateway-marker behavior. - #4598 -> `docs/manage-sandboxes/lifecycle.mdx`: Documents concurrent gateway/dashboard cleanup isolation by sandbox name and port. - #4777 -> `docs/reference/troubleshooting.mdx`: Documents Docker GPU patch rollback behavior. - #4610 -> `docs/reference/troubleshooting.mdx`, `docs/reference/commands.mdx`: Keeps mutable OpenClaw config permission guidance aligned and removes skipped experimental wording. - #4868 -> `docs/reference/commands.mdx`: Keeps `.dockerignore` handling for custom `onboard --from <Dockerfile>` contexts in generated skills. - #4870 -> `docs/reference/commands.mdx`, `docs/manage-sandboxes/runtime-controls.mdx`: Documents `NEMOCLAW_MINIMAL_BOOTSTRAP` and generated skill coverage. - #4641 -> `docs/inference/inference-options.mdx`, `docs/reference/troubleshooting.mdx`: Documents local NVIDIA NIM platform-digest pulls and served-model id adoption. - #4810 and #4867 -> `docs/inference/inference-options.mdx`: Documents stable NGC managed-vLLM image lineage and DGX Station DeepSeek V4 Flash coverage. - #4852 -> `docs/inference/use-local-inference.mdx`, `docs/reference/troubleshooting.mdx`: Documents Ollama model fit filtering, 16K context floor, cold-load retry, and failed-model exclusion. - #4847 -> `docs/inference/switch-inference-providers.mdx`: Documents API-family sync, Hermes `api_mode`, and Bedrock Runtime exception. - #4800 -> `docs/inference/tool-calling-reliability.mdx`: Documents Nemotron managed-inference native tool-search fallback. - #4333 -> `docs/inference/switch-inference-providers.mdx`: Documents interactive multimodal input prompting. - #4086 -> `docs/reference/troubleshooting.mdx`: Keeps proxy bypass normalization in generated troubleshooting coverage. - #4811 and #4855 -> `docs/get-started/quickstart-hermes.mdx`: Documents prebuilt Hermes dashboard assets and TUI recovery without runtime rebuilds. - #4854 -> `docs/inference/switch-inference-providers.mdx`, `docs/reference/commands.mdx`: Documents Hermes proxy API-key placeholder preservation during inference switches. - #4248 -> `docs/manage-sandboxes/messaging-channels.mdx`, `.agents/skills/`: Keeps messaging enrollment behavior aligned with manifest-hook implementation. - #4771 -> `docs/security/best-practices.mdx`, `docs/security/credential-storage.mdx`: Documents Hermes placeholder-only secret boundary for sandbox-visible runtime files. - #4787 -> `docs/security/best-practices.mdx`, `docs/about/release-notes.mdx`: Documents expanded memory scanner examples for OpenAI project keys and Slack app-level tokens. - #4848 -> `docs/reference/commands.mdx`: Documents OpenClaw skill install mirroring into the agent home directory. - #4790 -> `docs/about/release-notes.mdx`: Uses the prior release-prep structure and generated `.agents/skills/` refresh as the template for this release. ## Verification - `python3 scripts/docs-to-skills.py docs/ .agents/skills/ --prefix nemoclaw-user --doc-platform fern-mdx` - `python3 scripts/docs-to-skills.py docs/ .agents/skills/ skills/ --prefix nemoclaw-user --doc-platform fern-mdx --dry-run` - `npm run docs` - `git diff --check` - skip-term scan across `docs/`, `.agents/skills/`, and `skills/` - `npm run build:cli` - `npm run typecheck:cli` - Commit and pre-push hook suites, including markdownlint, gitleaks, env-var docs gate, docs-to-skills verification, and skills YAML tests <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * DeepSeek-V4-Flash now available as default inference model for DGX Station. * Hermes dashboard improved with dedicated port and OAuth-authenticated tool gateway selection. * Added weather and public-reference policy presets for expanded agent capabilities. * Enhanced Ollama model selection with GPU memory filtering and automatic retry for timeouts. * **Bug Fixes** * Improved policy tier validation to prevent invalid configurations. * Better sandbox cleanup scoping by port to prevent conflicts across deployments. * Added GPU patch failure recovery with automatic rollback. * **Documentation** * Expanded troubleshooting guides for inference, security, and sandbox lifecycle. * Added .dockerignore best practices for custom deployments. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Carlos Villela <cvillela@nvidia.com>
Summary
Skill install populated only the OpenClaw state dir but never created the home mirror at
$HOME/.openclaw/skills/<name>, so on sandboxes whose agent$HOMEdiffers from the state dir a skill showed up inopenclaw skills listyet was never loaded by the agent at session start.postInstallnow mirrors the uploaded skill into the agent home dir for OpenClaw, symmetric withskill remove(which already deletes that mirror).Related Issue
Fixes #4819
Changes
src/lib/skill-install.ts:postInstallnow copies the uploaded skill fromuploadDirinto the OpenClaw home mirror ($HOME/.openclaw/skills/<name>) before refreshing sessions. The copy is a guarded no-op via a[ src -ef dst ]test when both paths resolve to the same directory (the common case where$HOMEis the state dir's parent), and emits a warning if the mirror cannot be created.src/lib/skill-install.test.ts: added regression tests assertingpostInstallissues the mirror command for OpenClaw and warns when the mirror cannot be created.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: jason jama@nvidia.com
Summary by CodeRabbit
New Features
Improvements
Tests