fix(cli): mirror installed Deep Agents skill into agent skills dir - #7724
fix(cli): mirror installed Deep Agents skill into agent skills dir#7724rluo8 wants to merge 13 commits into
Conversation
Signed-off-by: Rui Luo <ruluo@nvidia.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:
📝 WalkthroughWalkthroughSkill lifecycle handling now supports Deep Agents shared skill directories, atomic fresh installs, stricter path validation, ownership-aware removal, and agent-specific mirror/session behavior. Documentation and tests cover OpenClaw, Hermes, and Deep Agents flows. ChangesShared-agent skill lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant installSandboxSkill
participant installFreshSharedSkill
participant DeepAgentsSkills
Operator->>installSandboxSkill: install skill
installSandboxSkill->>installFreshSharedSkill: use shared destination flow
installFreshSharedSkill->>DeepAgentsSkills: verify and atomically activate fresh skill
DeepAgentsSkills-->>installSandboxSkill: return installation result
Operator->>installSandboxSkill: remove skill
installSandboxSkill-->>Operator: refuse automatic removal for shared content
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 7f7dea6 in the TypeScript / code-coverage/cliThe overall coverage in commit 7f7dea6 in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-7724.docs.buildwithfern.com/nemoclaw |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: 2 optional E2E recommendations
1 warning · 0 suggestionsWarningsWarnings do not block.
|
![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)
src/lib/skill-install.ts (1)
267-320: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
postInstallcan silently clobber agent-authored content in a shared mirror.For agents with
sharedWithAgent: true(Deep Agents),mirrorDiris the same path the agent's own tooling (skill-creator) writes user-authored skills into — that's exactly whycheckExisting/removeSkillinsrc/lib/skill-remote.tsspecial-casemirrorSharedWithAgentto avoid treating that directory as NemoClaw-owned.postInstalldoesn't apply the same protection: wheneverpaths.mirrorDiris set it unconditionally runsrm -rf ${dst} && cp -a ${src} ${dst}, even when a fresh (non-update) install happens to collide in name with a skill the user authored directly inagent/skillsvia dcode. That silently destroys the user's content with no warning.
installSandboxSkillalready computesisUpdate/existingCheckbefore callingpostInstall— plumbing that through (or re-probing mirror existence formirrorSharedWithAgentagents) would letpostInstallwarn instead of blindly overwriting when it's a fresh install colliding with existing mirror content.🛡️ Sketch of a guard against clobbering agent-authored mirrors
export function postInstall( ctx: SshContext, paths: SkillPaths, _localSkillDir: string, opts: { skipRefresh?: boolean; sshExecImpl?: typeof sshExec; + isUpdate?: boolean; } = {}, ): { success: boolean; messages: string[] } { const messages: string[] = []; const runSsh = opts.sshExecImpl ?? sshExec; if (paths.mirrorDir) { const src = shellQuote(paths.uploadDir); const dst = `"${paths.mirrorDir}"`; + if (paths.mirrorSharedWithAgent && !opts.isUpdate) { + const mirrorExists = runSsh(ctx, `test -e ${dst}`); + if (mirrorExists && mirrorExists.status === 0) { + messages.push( + `Warning: ${paths.mirrorDir} already exists and may be agent-authored; skipping mirror to avoid overwriting it`, + ); + return { success: true, messages }; + } + } const mirrorParent = `"${paths.mirrorDir.slice(0, paths.mirrorDir.lastIndexOf("/"))}"`; ...🤖 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 `@src/lib/skill-install.ts` around lines 267 - 320, Update installSandboxSkill and postInstall to propagate the existing fresh-install/update state and mirror ownership information, using the existing existingCheck or mirrorSharedWithAgent symbols. In postInstall, when a shared mirror already exists during a fresh install, do not run the rm -rf/cp overwrite; emit a warning and preserve the agent-authored content, while retaining replacement behavior for legitimate updates and unchanged behavior for non-shared mirrors.
🤖 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 `@docs/manage-sandboxes/workspace-files.mdx`:
- Line 173: Update the command example in the workspace-files documentation
table to use the shared $$nemoclaw placeholder instead of nemoclaw, preserving
the existing skill install arguments and description.
---
Outside diff comments:
In `@src/lib/skill-install.ts`:
- Around line 267-320: Update installSandboxSkill and postInstall to propagate
the existing fresh-install/update state and mirror ownership information, using
the existing existingCheck or mirrorSharedWithAgent symbols. In postInstall,
when a shared mirror already exists during a fresh install, do not run the rm
-rf/cp overwrite; emit a warning and preserve the agent-authored content, while
retaining replacement behavior for legitimate updates and unchanged behavior for
non-shared mirrors.
🪄 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: 3933447c-b6b1-4975-a795-a92c96c3a6cd
📒 Files selected for processing (8)
docs/manage-sandboxes/workspace-files.mdxdocs/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.ts
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>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/skill-install-shared.test.ts (1)
83-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThese command-text assertions lock in the script's wording rather than its behavior.
toContain('mv -nT -- "$payload" "$leaf"')and friends fail on any harmless rewrite of the generated script, while the Linux-gated tests below already prove the properties that matter (exact bytes installed, no-clobber, staging cleaned up). The two negative assertions on Line 88 and Line 90 guard against constructs this PR never emits, so they don't protect a contract.Consider keeping only the assertion that the input is a streamed buffer plus the outcome assertions, and dropping the substring matching.
As per path instructions: "Prefer observable outcomes through the public boundary over source-text, private-shape, or mock-call assertions."
🤖 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 `@src/lib/skill-install-shared.test.ts` around lines 83 - 90, The test over-specifies generated script wording through brittle substring assertions. In the relevant test, remove the command-text checks for implementation details such as tar flags, sha256sum, move commands, cleanup markers, and negative path/command substrings; retain the streamed-buffer assertion and observable installation outcome assertions, including the Linux-gated behavior tests.Source: Path instructions
🤖 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 `@src/lib/actions/sandbox/skill-install.ts`:
- Around line 348-350: Update the shared-install success path in the skill
installation flow before its return to print explicit reload guidance, matching
the existing agent-specific or postInstall messaging so Deep Agents users know
when dcode will recognize the newly installed skill.
In `@src/lib/skill-install.ts`:
- Around line 402-408: Update the extraction and manifest flow in the skill
installation logic to preserve executable file modes instead of using
--no-same-permissions. If permissions are normalized for safety, apply an
explicit normalization that retains user execute bits and ensure the manifest
records each file’s mode alongside its content hash so mode changes are attested
consistently with uploadDirectory/scp installs.
---
Nitpick comments:
In `@src/lib/skill-install-shared.test.ts`:
- Around line 83-90: The test over-specifies generated script wording through
brittle substring assertions. In the relevant test, remove the command-text
checks for implementation details such as tar flags, sha256sum, move commands,
cleanup markers, and negative path/command substrings; retain the
streamed-buffer assertion and observable installation outcome assertions,
including the Linux-gated behavior tests.
🪄 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: 0a2385d5-88ba-4c3a-8e3f-b4203481e3aa
📒 Files selected for processing (9)
docs/manage-sandboxes/workspace-files.mdxdocs/reference/commands.mdxsrc/lib/actions/sandbox/skill-install.test.tssrc/lib/actions/sandbox/skill-install.tssrc/lib/skill-install-shared.test.tssrc/lib/skill-install.test.tssrc/lib/skill-install.tssrc/lib/skill-remote.test.tssrc/lib/skill-remote.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/manage-sandboxes/workspace-files.mdx
- src/lib/skill-remote.ts
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>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
wscurran
added
area: cli
|
✨ Thanks for the PR. This fixes the Deep Agents skill mirror so installed skills appear in Related open issues: Related open PRs: Related open issues: |
…7850) <!-- markdownlint-disable MD041 --> ## Summary This credited replacement for #7724 installs Deep Agents Code skills into the directory the agent actually loads while preserving agent-authored content. Rui Luo authored the original fix; commit `165325267` preserves Rui as the author, and the follow-up commits incorporate the maintainer and automated-review hardening. ## Related Issue Fixes #7634 ## Changes - Install fresh Deep Agents Code skills directly under `/sandbox/.deepagents/agent/skills/<name>`. - Refuse automatic update or removal when the destination is shared with agent-authored content. - Copy regular files into a private no-follow host snapshot, attest paths, normalized modes, and SHA-256 digests, and reject identity changes. - Stage and verify the exact archive inside the sandbox before an atomic no-clobber move. - Cover fresh install, collisions, corrupt archives, mode normalization, unsupported paths, and local path races. - Document the supported Deep Agents Code lifecycle and manual confirm-ownership recovery path. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] 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: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [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: Exact-head nine-category maintainer security review passed at `a84264939`; diff fingerprint `4128a6d3c613892f96aa22013e24e2d8f80363c35c552245f2592fba0f84f10d`; review: #7850 (review). - [ ] 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 exact-head root-identity hardening does not change CLI usage, output, configuration, defaults, or supported workflows. Existing docs already state that identity changes and symlinks are rejected, so no docs build was needed. - Agent: Codex Desktop <!-- docs-review-head-sha: a842649 --> <!-- docs-review-agents-blob-sha: c052d60 --> ## 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 — skill-install-focused set at reviewed code parent `7b4ce366a`: 52 passed, 7 Linux-only skipped; CLI typecheck and normal hooks passed. The exact-head merge adds current `main` with no skill-install overlap. - [ ] 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) - [x] 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: Charan Jagwani <cjagwani@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added integrity-verified, no-clobber “fresh” installs for Deep Agents shared skill directories, with stronger collision protection and safer ownership expectations. * **Bug Fixes** * Hardened skill install/remove to reject symlinks and unsafe paths, improve fail-closed handling, and align mirror/session cleanup and verification across configurations. * Improved OpenClaw behavior to refresh session indexing after skill installs/updates and better recover missing gateway components. * **Documentation** * Clarified Deep Agents skill directory semantics and tightened `skill install/remove` rules around `SKILL.md`, ownership, and removal safety. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Rui Luo <ruluo@nvidia.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> Co-authored-by: Rui Luo <ruluo@nvidia.com> Co-authored-by: cjagwani <cjagwani@nvidia.com>
|
Queue handoff (comment only; no changes-requested review): #7850 has now merged into |
|
Closing as superseded by merged #7850. The replacement preserves Rui Luo's original authored commit and co-author credit, and includes the additional no-clobber and race-safety hardening needed for the Deep Agents skill lifecycle. |
7 participants
Summary
skill installuploaded Deep Agents skills to/sandbox/.deepagents/skills/<name>, which Deep Agents Code never reads — its user skill directory is~/.deepagents/<agent>/skills. The CLI reported success while the skill was never registered: absent fromdcode skills listand from the agent's skill index. Install now mirrors into/sandbox/.deepagents/agent/skills/<name>and fails verification if that mirror is missing; remove deletes it. This is the Deep Agents counterpart of the OpenClaw mirror added in #4848.Related Issue
Fixes #7634
Changes
src/lib/skill-install.ts: replace the OpenClaw-onlymirrorDirspecial case with anAGENT_SKILL_MIRRORStable keyed by agent name.postInstall()andverifyInstall()branch onpaths.mirrorDirinstead ofpaths.isOpenClaw, so the existing fix(cli): mirror installed OpenClaw skill into agent home dir (Fixes #4819) #4848 mirror copy and verification apply to any agent that declares one. No new mechanism.src/lib/skill-install.ts: the "Restart the agent gateway" hint now fires only when the agent has neither a mirror nor a session file — Deep Agents is a terminal runtime with no gateway.src/lib/skill-remote.ts:removeSkill()andverifyRemove()branch onmirrorDirtoo, so remove stays symmetric with install.src/lib/skill-remote.ts:checkExisting()deliberately does not probe a shared mirror. It gatesskill remove, so it must answer "did NemoClaw install this?" —uploadDiris the ownership marker. dcode's skill-creator authors user skills straight intoagent/skills([All Platforms][Agent&Skills][GitHub Issue #5753] Deep Agents skill-creator does not persist custom skills into ~/.deepagents/agent/skills, so skills like note-summarizer are lost and cannot survive NemoClaw rebuild #5753); counting that as an install would letskill remove Xdelete a skill the user created in-sandbox.SkillPathsgainsmirrorSharedWithAgentto make this explicit rather than incidental to the agent name.src/lib/actions/sandbox/skill-install.ts: the verification-failure message names the mirror for any agent that has one.docs/reference/commands.mdx: four corrections. Thedeepagentsinstall variant promised a mirror no code implemented. Shared install prose claimed the session index refreshes "for new installs" — it actually refreshes on every install and update, and only for OpenClaw (sessionFileis null elsewhere); that clause moved into theopenclawblock. Shared remove prose had the same scoping bug. Thedeepagentsremove variant claimed a session-index refresh that does not exist.docs/manage-sandboxes/workspace-files.mdx:/sandbox/.deepagents/skills/was labelled "User-level skills available to Deep Agents Code"; it is the upload/snapshot directory, andagent/skills/is what the agent loads.Type of Change
Quality Gates
Documentation Writer Review
docs-updated|no-docs-needed|blockedDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Rui Luo ruluo@nvidia.com
Summary by CodeRabbit