From d4609ff65a6d9279d925665660b0f74754968476 Mon Sep 17 00:00:00 2001 From: Rui Luo Date: Tue, 28 Jul 2026 17:20:16 +0800 Subject: [PATCH 1/6] fix(cli): mirror installed Deep Agents skill into agent skills dir Signed-off-by: Rui Luo --- docs/manage-sandboxes/workspace-files.mdx | 4 +- docs/reference/commands.mdx | 16 ++- src/lib/actions/sandbox/skill-install.test.ts | 1 + src/lib/actions/sandbox/skill-install.ts | 2 +- src/lib/skill-install.test.ts | 78 +++++++++++ src/lib/skill-install.ts | 125 +++++++++++------- src/lib/skill-remote.test.ts | 48 +++++++ src/lib/skill-remote.ts | 32 +++-- 8 files changed, 240 insertions(+), 66 deletions(-) diff --git a/docs/manage-sandboxes/workspace-files.mdx b/docs/manage-sandboxes/workspace-files.mdx index e913106556b..47ab2cfb59b 100644 --- a/docs/manage-sandboxes/workspace-files.mdx +++ b/docs/manage-sandboxes/workspace-files.mdx @@ -170,8 +170,8 @@ Deep Agents Code creates them when you start the first `dcode` session. | `/sandbox/.deepagents/.state/` | Deep Agents Code runtime state, including persisted session and MCP-related state. | | `/sandbox/.deepagents/agent/AGENTS.md` | Global memory file for the default Deep Agents Code agent, loaded at session start. | | `/sandbox/.deepagents/agent/memories/` | Topic-specific markdown memories that Deep Agents Code can read and update across sessions. | -| `/sandbox/.deepagents/skills/` | User-level skills available to Deep Agents Code in the sandbox. | -| `/sandbox/.deepagents/agent/skills/` | Default-agent skills created by the built-in skill creator and preserved by NemoClaw snapshots. | +| `/sandbox/.deepagents/skills/` | Upload and snapshot directory for skills installed with `nemoclaw skill install`. Deep Agents Code does not read this path directly, so the command mirrors each skill into `agent/skills/`. | +| `/sandbox/.deepagents/agent/skills/` | Skills that Deep Agents Code loads at session start, including skills created by the built-in skill creator and mirrors written by `skill install`. Preserved by NemoClaw snapshots. | | `/sandbox/.deepagents/.nemoclaw-mcp.json` | NemoClaw-generated managed MCP projection with OpenShell credential placeholders. NemoClaw reconstructs it from host-side registry state. | | `/sandbox/.deepagents/.state/auth.json` | Upstream auth state. The managed launchers refuse to start when this file contains credentials. | | `/sandbox/.deepagents/.state/chatgpt-auth.json` | Upstream ChatGPT auth state. The managed launchers refuse to start when this file exists. | diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index 261d5e4b8ff..4f96ea27e50 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2315,6 +2315,7 @@ To install an OpenClaw plugin, refer to [Install OpenClaw Plugins](../manage-san For OpenClaw, the command uploads the skill to the OpenClaw state directory and mirrors it into `$HOME/.openclaw/skills/` when the agent home directory differs from the state directory. That mirror makes skills listed by `openclaw skills list` available at session startup. If mirror creation fails, NemoClaw prints a warning so you can reinstall or inspect the home directory permissions. +OpenClaw caches skill content per session, so the command also refreshes the OpenClaw session index after every install and update to avoid stale `SKILL.md` data. @@ -2325,8 +2326,10 @@ Hermes plugins are different from NemoClaw skills. -For Deep Agents, the command uploads skills under `/sandbox/.deepagents/skills/` and mirrors agent-facing skills under `/sandbox/.deepagents/agent/skills/` when the manifest requires that view. -The managed `dcode` launchers discover those skills on the next session without accepting executable hook configuration. +For Deep Agents, the command uploads skills under `/sandbox/.deepagents/skills/` and then mirrors them into `/sandbox/.deepagents/agent/skills/`. +The mirror is required: Deep Agents Code loads user skills from `~/.deepagents//skills`, so a skill that exists only in the upload directory is never discovered. +Verification fails when the mirror is missing, so the command does not report success for a skill the agent cannot load. +The managed `dcode` launchers discover mirrored skills on the next session without accepting executable hook configuration. @@ -2337,16 +2340,16 @@ Files with names starting with `.` (dotfiles) are skipped and listed in the outp Files with unsafe path characters are rejected to prevent shell injection. If the skill already exists on the sandbox, the command updates it in place and preserves chat history. -For new installs, the agent session index is refreshed so the agent discovers the skill on the next session. +In both cases the agent discovers the change on its next session, using the per-agent mechanism described above. ### `$$nemoclaw skill remove ` Remove an installed skill from a running sandbox by skill name. -The command validates the skill name, removes the sandbox upload directory, and refreshes the agent session index so the remaining skills are rediscovered on the next session. +The command validates the skill name and removes the sandbox upload directory, so the remaining skills are rediscovered on the next session. -For OpenClaw, the command also removes the OpenClaw home-directory mirror when present. +For OpenClaw, the command also removes the OpenClaw home-directory mirror when present and refreshes the OpenClaw session index. @@ -2356,7 +2359,8 @@ Run `$$nemoclaw gateway restart` if prompted so the removal takes effect. -For Deep Agents, the command removes the uploaded skill from the managed `/sandbox/.deepagents` skill paths and refreshes the session index for future `dcode` sessions. +For Deep Agents, the command removes the uploaded skill from `/sandbox/.deepagents/skills/` and its mirror at `/sandbox/.deepagents/agent/skills/`. +Removing the mirror is what makes the skill stop loading, because Deep Agents Code reads its skills from that directory at session start; the next `dcode` session no longer lists it. It does not enable project hooks or unmanaged MCP files. diff --git a/src/lib/actions/sandbox/skill-install.test.ts b/src/lib/actions/sandbox/skill-install.test.ts index 44356b8bfd6..76a1f64aa57 100644 --- a/src/lib/actions/sandbox/skill-install.test.ts +++ b/src/lib/actions/sandbox/skill-install.test.ts @@ -41,6 +41,7 @@ import { installSandboxSkill, removeSandboxSkill } from "./skill-install"; const paths = { uploadDir: "/sandbox/.openclaw/skills/demo-skill", mirrorDir: "$HOME/.openclaw/skills/demo-skill", + mirrorSharedWithAgent: false, sessionFile: "/sandbox/.openclaw/agents/main/sessions/sessions.json", isOpenClaw: true, }; diff --git a/src/lib/actions/sandbox/skill-install.ts b/src/lib/actions/sandbox/skill-install.ts index c92509e0196..84901442926 100644 --- a/src/lib/actions/sandbox/skill-install.ts +++ b/src/lib/actions/sandbox/skill-install.ts @@ -342,7 +342,7 @@ export async function installSandboxSkill( } else { console.error( ` Skill uploaded but verification failed: SKILL.md missing at ${paths.uploadDir}` + - (paths.isOpenClaw && paths.mirrorDir ? ` or its agent mirror ${paths.mirrorDir}` : ""), + (paths.mirrorDir ? ` or its agent mirror ${paths.mirrorDir}` : ""), ); process.exit(1); } diff --git a/src/lib/skill-install.test.ts b/src/lib/skill-install.test.ts index d3d1438327b..8611ffdfbfa 100644 --- a/src/lib/skill-install.test.ts +++ b/src/lib/skill-install.test.ts @@ -235,6 +235,23 @@ describe("resolveSkillPaths", () => { expect(paths.isOpenClaw).toBe(false); }); + it("mirrors Deep Agents skills into the agent skills dir dcode loads (#7634)", () => { + // dcode's user skill dir is ~/.deepagents/{agent}/skills (HOME=/sandbox, + // DEFAULT_AGENT_NAME="agent"); it never scans ~/.deepagents/skills, so the + // upload dir alone leaves the skill installed but unloadable. + const agent = { + name: "langchain-deepagents-code", + configPaths: { + dir: "/sandbox/.deepagents", + }, + }; + const paths = resolveSkillPaths(agent, "note-summarizer"); + expect(paths.uploadDir).toBe("/sandbox/.deepagents/skills/note-summarizer"); + expect(paths.mirrorDir).toBe("/sandbox/.deepagents/agent/skills/note-summarizer"); + expect(paths.sessionFile).toBeNull(); + expect(paths.isOpenClaw).toBe(false); + }); + it("returns generic paths for a hypothetical future agent", () => { const agent = { name: "future-agent", @@ -250,6 +267,11 @@ describe("resolveSkillPaths", () => { }); }); +const DEEPAGENTS_AGENT = { + name: "langchain-deepagents-code", + configPaths: { dir: "/sandbox/.deepagents" }, +}; + describe("postInstall", () => { it("refreshes OpenClaw sessions after installing an updated skill", () => { const skillDir = mkdtempSync(join(tmpdir(), "skill-postinstall-")); @@ -334,6 +356,43 @@ describe("postInstall", () => { rmSync(skillDir, { recursive: true, force: true }); } }); + + it("mirrors a Deep Agents skill into agent/skills instead of hinting a restart (#7634)", () => { + // Deep Agents Code is a terminal runtime with no gateway to restart; the + // only way the skill becomes loadable is the agent/skills mirror. + const skillDir = mkdtempSync(join(tmpdir(), "skill-postinstall-dcode-")); + const commands: string[] = []; + try { + writeFileSync(skillDir + "/SKILL.md", "---\nname: note-summarizer\n---\n# Notes\n"); + const paths = resolveSkillPaths(DEEPAGENTS_AGENT, "note-summarizer"); + const result = postInstall( + { configFile: "/tmp/ssh-config", sandboxName: "alpha" }, + paths, + skillDir, + { + sshExecImpl: (_ctx, command) => { + commands.push(command); + return { status: 0, stdout: "", stderr: "" }; + }, + }, + ); + + expect(result).toEqual({ success: true, messages: [] }); + const mirrorCmd = commands.find( + (c) => + c.includes(paths.uploadDir) && + c.includes('"/sandbox/.deepagents/agent/skills/note-summarizer"'), + ); + expect( + mirrorCmd, + "postInstall should mirror the skill into /sandbox/.deepagents/agent/skills", + ).toBeDefined(); + // No sessions.json exists for a terminal runtime, so nothing may clear one. + expect(commands.some((c) => c.includes("sessions.json"))).toBe(false); + } finally { + rmSync(skillDir, { recursive: true, force: true }); + } + }); }); describe("verifyInstall", () => { @@ -367,4 +426,23 @@ describe("verifyInstall", () => { expect(ok).toBe(false); }); + + it("requires SKILL.md in the Deep Agents agent/skills mirror (#7634)", () => { + // Verifying only the upload dir is what let `skill install` report success + // for a skill dcode could never load. + const paths = resolveSkillPaths(DEEPAGENTS_AGENT, "note-summarizer"); + const commands: string[] = []; + verifyInstall({ configFile: "/tmp/ssh-config", sandboxName: "alpha" }, paths, { + sshExecImpl: (_ctx, command) => { + commands.push(command); + return { status: 0, stdout: "EXISTS", stderr: "" }; + }, + }); + + expect( + commands.some((c) => + c.includes('"/sandbox/.deepagents/agent/skills/note-summarizer/SKILL.md"'), + ), + ).toBe(true); + }); }); diff --git a/src/lib/skill-install.ts b/src/lib/skill-install.ts index ffdcaf06e70..a58f60a38b7 100644 --- a/src/lib/skill-install.ts +++ b/src/lib/skill-install.ts @@ -96,14 +96,50 @@ export function parseFrontmatter(content: string): SkillFrontmatter { export interface SkillPaths { /** Upload target directory for the skill */ uploadDir: string; - /** OpenClaw-only mirror directory under the remote home dir, or null */ + /** Directory the agent actually loads skills from, or null when it equals uploadDir */ mirrorDir: string | null; + /** + * Whether the agent's own tooling also writes into mirrorDir. When true the + * mirror is NOT proof that NemoClaw installed a skill, so it must not be used + * as the existence gate for `skill remove`. + */ + mirrorSharedWithAgent: boolean; /** OpenClaw-only: session index to clear, or null */ sessionFile: string | null; /** Whether the agent is OpenClaw (drives refresh behavior) */ isOpenClaw: boolean; } +/** + * Agents whose loader reads skills from somewhere other than `uploadDir`. + * + * NemoClaw uploads to `uploadDir`, which the agent manifest declares as durable + * `state_dirs`, but the agent scans its own directory at session start. Where + * those diverge, an upload without a mirror leaves the skill on disk and + * unregistered — absent from the agent's skill list (#4819 for OpenClaw, #7634 + * for Deep Agents Code, whose user skill dir is `~/.deepagents/{agent}/skills`). + * + * `sharedWithAgent` marks a mirror the agent's own tooling also writes into: + * Deep Agents Code's skill-creator authors user skills straight into + * `agent/skills` (#5753), so a directory there is not proof NemoClaw installed + * it. `checkExisting()` depends on that distinction. + * + * Remove an entry when its agent starts loading skills from `uploadDir`. + */ +const AGENT_SKILL_MIRRORS: Record< + string, + { dir: (dir: string, skillName: string) => string; sharedWithAgent: boolean } +> = { + openclaw: { + dir: (_dir, skillName) => `$HOME/.openclaw/skills/${skillName}`, + sharedWithAgent: false, + }, + "langchain-deepagents-code": { + dir: (dir, skillName) => `${dir}/agent/skills/${skillName}`, + sharedWithAgent: true, + }, +}; + /** * Resolve skill install paths from the agent definition. * Uses a single directory for skill uploads (no immutable/writable split). @@ -119,10 +155,12 @@ export function resolveSkillPaths( const dir = agent ? agent.configPaths.dir : "/sandbox/.openclaw"; const uploadDir = `${dir}/skills/${skillName}`; + const mirror = AGENT_SKILL_MIRRORS[agent ? agent.name : "openclaw"]; return { uploadDir, - mirrorDir: isOpenClaw ? `$HOME/.openclaw/skills/${skillName}` : null, + mirrorDir: mirror ? mirror.dir(dir, skillName) : null, + mirrorSharedWithAgent: mirror ? mirror.sharedWithAgent : false, sessionFile: isOpenClaw ? `${dir}/agents/main/sessions/sessions.json` : null, isOpenClaw, }; @@ -226,8 +264,8 @@ export function uploadDirectory( } /** - * Run post-install steps: session refresh for OpenClaw, or - * non-OpenClaw restart hint. + * Run post-install steps: skill-load mirror for every agent that needs one, + * session refresh for OpenClaw, and a restart hint when neither applies. */ export function postInstall( ctx: SshContext, @@ -241,44 +279,40 @@ export function postInstall( const messages: string[] = []; const runSsh = opts.sshExecImpl ?? sshExec; - if (paths.isOpenClaw) { - // Mirror the uploaded skill into the agent's home dir - // ($HOME/.openclaw/skills/). The skill is uploaded to the OpenClaw - // state dir (uploadDir), which `openclaw skills list` reads, but the agent - // loads skills from $HOME/.openclaw/skills at session start. On sandboxes - // where the agent's $HOME differs from the state dir these paths diverge, - // so without this mirror the skill is listed but never invoked (#4819). - // `skill remove` already deletes this mirror, so install must create it to - // stay symmetric. The copy is skipped when both paths resolve to the same - // directory (the common case where $HOME is the state dir's parent), so it - // is a safe no-op there. - if (paths.mirrorDir) { - const src = shellQuote(paths.uploadDir); - // mirrorDir contains $HOME, which must expand on the remote shell, so we - // use double quotes (not shellQuote). Safe because skill names are - // restricted to [A-Za-z0-9._-] by parseFrontmatter / the name regex. - const dst = `"${paths.mirrorDir}"`; - const mirrorParent = `"${paths.mirrorDir.slice(0, paths.mirrorDir.lastIndexOf("/"))}"`; - const mirrorResult = runSsh( - ctx, - `[ ${src} -ef ${dst} ] || { mkdir -p ${mirrorParent} && rm -rf ${dst} && cp -a ${src} ${dst}; }`, + // Copy the skill into the directory the agent's loader actually reads; see + // AGENT_SKILL_MIRRORS for which agents need this and why. Without it the + // upload never registers as a skill. `skill remove` deletes the mirror, so + // install must create it to stay symmetric. The copy is skipped when both + // paths resolve to the same directory, so it is a no-op for agents whose + // loader reads uploadDir directly. + if (paths.mirrorDir) { + const src = shellQuote(paths.uploadDir); + // mirrorDir may contain $HOME, which must expand on the remote shell, so we + // use double quotes (not shellQuote). Safe because skill names are + // restricted to [A-Za-z0-9._-] by parseFrontmatter / the name regex. + const dst = `"${paths.mirrorDir}"`; + const mirrorParent = `"${paths.mirrorDir.slice(0, paths.mirrorDir.lastIndexOf("/"))}"`; + const mirrorResult = runSsh( + ctx, + `[ ${src} -ef ${dst} ] || { mkdir -p ${mirrorParent} && rm -rf ${dst} && cp -a ${src} ${dst}; }`, + ); + if (!mirrorResult || mirrorResult.status !== 0) { + messages.push( + `Warning: failed to mirror skill into ${paths.mirrorDir} (agent may not load it)`, ); - if (!mirrorResult || mirrorResult.status !== 0) { - messages.push( - `Warning: failed to mirror skill into ${paths.mirrorDir} (agent may not load it)`, - ); - } } + } - // Clear sessions.json so OpenClaw re-discovers skills on the next - // session even after an in-place skill update. - if (paths.sessionFile && !opts.skipRefresh) { - const refreshResult = runSsh(ctx, `printf '{}' > ${shellQuote(paths.sessionFile)}`); - if (!refreshResult || refreshResult.status !== 0) { - messages.push("Warning: failed to clear sessions (agent may need manual restart)"); - } + // Clear sessions.json so OpenClaw re-discovers skills on the next + // session even after an in-place skill update. + if (paths.sessionFile && !opts.skipRefresh) { + const refreshResult = runSsh(ctx, `printf '{}' > ${shellQuote(paths.sessionFile)}`); + if (!refreshResult || refreshResult.status !== 0) { + messages.push("Warning: failed to clear sessions (agent may need manual restart)"); } - } else { + } + + if (!paths.mirrorDir && !paths.sessionFile) { messages.push("Restart the agent gateway to pick up the new skill."); } @@ -288,11 +322,12 @@ export function postInstall( /** * Verify the SKILL.md file exists on the sandbox. * - * For OpenClaw the home mirror ($HOME/.openclaw/skills/) must also exist: - * that is the path the agent loads skills from at session start (#4819), so a - * successful upload whose mirror copy failed must NOT verify as installed — - * otherwise the CLI reports success while the skill stays invisible to the - * agent. This mirrors verifyRemove(), which already checks both paths. + * When the agent has a mirror directory it must also exist: that is the path + * the agent loads skills from at session start (#4819 for OpenClaw, #7634 for + * Deep Agents Code), so a successful upload whose mirror copy failed must NOT + * verify as installed — otherwise the CLI reports success while the skill stays + * invisible to the agent. This mirrors verifyRemove(), which already checks + * both paths. */ export function verifyInstall( ctx: SshContext, @@ -300,8 +335,8 @@ export function verifyInstall( opts: { sshExecImpl?: typeof sshExec } = {}, ): boolean { const checks = [`test -f ${shellQuote(`${paths.uploadDir}/SKILL.md`)}`]; - if (paths.isOpenClaw && paths.mirrorDir) { - // mirrorDir contains $HOME, which must expand on the remote shell, so we + if (paths.mirrorDir) { + // mirrorDir may contain $HOME, which must expand on the remote shell, so we // use double quotes (not shellQuote) — safe because skill names are // restricted to [A-Za-z0-9._-]. checks.push(`test -f "${paths.mirrorDir}/SKILL.md"`); diff --git a/src/lib/skill-remote.test.ts b/src/lib/skill-remote.test.ts index 729514f657d..9dccd3a7985 100644 --- a/src/lib/skill-remote.test.ts +++ b/src/lib/skill-remote.test.ts @@ -84,6 +84,54 @@ describe("removeSkill (unit — no SSH)", () => { "printf '{}' > '/sandbox/.openclaw/agents/main/sessions/sessions.json'", ]); }); + + it("does not treat an agent-authored Deep Agents skill as installed (#7634)", () => { + // dcode's own skill-creator writes user skills into agent/skills (#5753), + // so a directory there can exist without NemoClaw ever installing it. + // checkExisting gates `skill remove`; probing the shared mirror here would + // let remove delete a skill the user created in-sandbox. + const ctx = { configFile: "/tmp/ssh.conf", sandboxName: "test-sandbox" }; + const paths = resolveSkillPaths( + { name: "langchain-deepagents-code", configPaths: { dir: "/sandbox/.deepagents" } }, + "user-authored", + ); + const commands: string[] = []; + checkExisting(ctx, paths, { + sshExecImpl: (_ctx, command) => { + commands.push(command); + return { status: 0, stdout: "ABSENT", stderr: "" }; + }, + }); + + expect(paths.mirrorSharedWithAgent).toBe(true); + expect(commands[0]).toContain("/sandbox/.deepagents/skills/user-authored"); + expect(commands[0]).not.toContain("agent/skills/user-authored"); + }); + + it("removes the Deep Agents agent/skills mirror so the skill stops loading (#7634)", () => { + // Install now populates agent/skills; remove must delete it or dcode keeps + // loading a skill the user removed. + const ctx = { configFile: "/tmp/ssh.conf", sandboxName: "test-sandbox" }; + const paths = resolveSkillPaths( + { name: "langchain-deepagents-code", configPaths: { dir: "/sandbox/.deepagents" } }, + "test-skill", + ); + const commands: string[] = []; + const result = removeSkill(ctx, paths, { + sshExecImpl: (_ctx, command) => { + commands.push(command); + return { status: 0, stdout: "", stderr: "" }; + }, + }); + + expect(result.success).toBe(true); + expect(result.removedMirrorDir).toBe(true); + expect(result.clearedSessions).toBe(false); + expect(commands).toEqual([ + "rm -rf '/sandbox/.deepagents/skills/test-skill'", + 'rm -rf "/sandbox/.deepagents/agent/skills/test-skill"', + ]); + }); }); describe("verifyRemove (unit — no SSH)", () => { diff --git a/src/lib/skill-remote.ts b/src/lib/skill-remote.ts index 8b19d1780a7..60ea58f2157 100644 --- a/src/lib/skill-remote.ts +++ b/src/lib/skill-remote.ts @@ -78,8 +78,12 @@ export function checkExisting( paths: SkillPaths, opts: { sshExecImpl?: typeof sshExec } = {}, ): boolean | null { + // Existence gate for `skill remove`, so it must answer "did NemoClaw install + // this skill?" — uploadDir is the ownership marker. A mirror the agent's own + // tooling writes into is not evidence of an install (#5753), and counting it + // would let `skill remove` delete a skill the user authored in-sandbox. const checks = [`test -e ${shellQuote(paths.uploadDir)}`]; - if (paths.isOpenClaw && paths.mirrorDir) { + if (paths.mirrorDir && !paths.mirrorSharedWithAgent) { checks.push(`test -e "${paths.mirrorDir}"`); } const runSsh = opts.sshExecImpl ?? sshExec; @@ -102,9 +106,9 @@ export interface RemoveResult { /** * Remove a skill from the sandbox by name. - * Deletes the immutable upload directory, the OpenClaw mirror directory - * (if applicable), and clears sessions.json so the agent re-discovers - * the remaining skills on the next session. + * Deletes the immutable upload directory, the agent's skill-load mirror + * directory (if applicable), and clears sessions.json so the agent + * re-discovers the remaining skills on the next session. * * Only the named skill directory is deleted — other skills are untouched. */ @@ -124,13 +128,14 @@ export function removeSkill( messages.push(`Warning: failed to remove upload directory ${paths.uploadDir}`); } - // 2. Remove the OpenClaw mirror ($HOME/.openclaw/skills//) - // mirrorDir contains $HOME which must expand on the remote shell, so we + // 2. Remove the agent's skill-load mirror (see AGENT_SKILL_MIRRORS); leaving + // it behind keeps the removed skill loadable even though uploadDir is gone. + // mirrorDir may contain $HOME which must expand on the remote shell, so we // use double quotes (not shellQuote). This is safe because skill names // are restricted to [A-Za-z0-9._-] by parseFrontmatter / the name // validation regex, so $HOME expansion is the only variable substitution. let removedMirrorDir = false; - if (paths.isOpenClaw && paths.mirrorDir) { + if (paths.mirrorDir) { const removeMirror = runSsh(ctx, `rm -rf "${paths.mirrorDir}"`); removedMirrorDir = removeMirror !== null && removeMirror.status === 0; if (!removedMirrorDir) { @@ -140,18 +145,20 @@ export function removeSkill( // 3. Clear sessions.json so the agent re-discovers the remaining skills. let clearedSessions = false; - if (paths.isOpenClaw && paths.sessionFile) { + if (paths.sessionFile) { const clearResult = runSsh(ctx, `printf '{}' > ${shellQuote(paths.sessionFile)}`); clearedSessions = clearResult !== null && clearResult.status === 0; if (!clearedSessions) { messages.push("Warning: failed to clear sessions (agent may need manual restart)"); } - } else if (!paths.isOpenClaw) { + } + + if (!paths.mirrorDir && !paths.sessionFile) { messages.push("Restart the agent gateway for the removal to take effect."); } return { - success: removedUploadDir && (!paths.isOpenClaw || removedMirrorDir), + success: removedUploadDir && (!paths.mirrorDir || removedMirrorDir), removedUploadDir, removedMirrorDir, clearedSessions, @@ -161,7 +168,8 @@ export function removeSkill( /** * Verify the skill directory no longer exists on the sandbox. - * For OpenClaw sandboxes, both the upload dir and the mirror dir must be gone. + * For agents with a skill-load mirror, both the upload dir and the mirror dir + * must be gone. */ export function verifyRemove( ctx: SshContext, @@ -169,7 +177,7 @@ export function verifyRemove( opts: { sshExecImpl?: typeof sshExec } = {}, ): boolean { const checks = [`test ! -e ${shellQuote(paths.uploadDir)}`]; - if (paths.isOpenClaw && paths.mirrorDir) { + if (paths.mirrorDir) { checks.push(`test ! -e "${paths.mirrorDir}"`); } const runSsh = opts.sshExecImpl ?? sshExec; From ff0d603669518c41d1dccc891039fb076c4f9c42 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Tue, 28 Jul 2026 05:15:38 -0700 Subject: [PATCH 2/6] fix(cli): protect Deep Agents skill installs Signed-off-by: Apurv Kumaria --- docs/manage-sandboxes/workspace-files.mdx | 4 +- docs/reference/commands.mdx | 29 +- src/lib/actions/sandbox/skill-install.test.ts | 122 ++++++- src/lib/actions/sandbox/skill-install.ts | 53 +++ src/lib/skill-install-shared.test.ts | 311 ++++++++++++++++++ src/lib/skill-install.test.ts | 91 ++--- src/lib/skill-install.ts | 274 +++++++++++++-- src/lib/skill-remote.test.ts | 45 ++- src/lib/skill-remote.ts | 22 +- 9 files changed, 821 insertions(+), 130 deletions(-) create mode 100644 src/lib/skill-install-shared.test.ts diff --git a/docs/manage-sandboxes/workspace-files.mdx b/docs/manage-sandboxes/workspace-files.mdx index 47ab2cfb59b..0d63c0b3f4c 100644 --- a/docs/manage-sandboxes/workspace-files.mdx +++ b/docs/manage-sandboxes/workspace-files.mdx @@ -170,8 +170,8 @@ Deep Agents Code creates them when you start the first `dcode` session. | `/sandbox/.deepagents/.state/` | Deep Agents Code runtime state, including persisted session and MCP-related state. | | `/sandbox/.deepagents/agent/AGENTS.md` | Global memory file for the default Deep Agents Code agent, loaded at session start. | | `/sandbox/.deepagents/agent/memories/` | Topic-specific markdown memories that Deep Agents Code can read and update across sessions. | -| `/sandbox/.deepagents/skills/` | Upload and snapshot directory for skills installed with `nemoclaw skill install`. Deep Agents Code does not read this path directly, so the command mirrors each skill into `agent/skills/`. | -| `/sandbox/.deepagents/agent/skills/` | Skills that Deep Agents Code loads at session start, including skills created by the built-in skill creator and mirrors written by `skill install`. Preserved by NemoClaw snapshots. | +| `/sandbox/.deepagents/skills/` | Legacy NemoClaw skill-upload state. Deep Agents Code does not load skills from this path, and `$$nemoclaw skill install` leaves it untouched. | +| `/sandbox/.deepagents/agent/skills/` | Skills that Deep Agents Code loads at session start, including skills created by the built-in skill creator and fresh-name skills installed directly by `skill install`. Preserved by NemoClaw snapshots. | | `/sandbox/.deepagents/.nemoclaw-mcp.json` | NemoClaw-generated managed MCP projection with OpenShell credential placeholders. NemoClaw reconstructs it from host-side registry state. | | `/sandbox/.deepagents/.state/auth.json` | Upstream auth state. The managed launchers refuse to start when this file contains credentials. | | `/sandbox/.deepagents/.state/chatgpt-auth.json` | Upstream ChatGPT auth state. The managed launchers refuse to start when this file exists. | diff --git a/docs/reference/commands.mdx b/docs/reference/commands.mdx index e01453ee908..b647d4e6658 100644 --- a/docs/reference/commands.mdx +++ b/docs/reference/commands.mdx @@ -2328,10 +2328,17 @@ Hermes plugins are different from NemoClaw skills. -For Deep Agents, the command uploads skills under `/sandbox/.deepagents/skills/` and then mirrors them into `/sandbox/.deepagents/agent/skills/`. -The mirror is required: Deep Agents Code loads user skills from `~/.deepagents//skills`, so a skill that exists only in the upload directory is never discovered. -Verification fails when the mirror is missing, so the command does not report success for a skill the agent cannot load. -The managed `dcode` launchers discover mirrored skills on the next session without accepting executable hook configuration. +For Deep Agents, the command installs a fresh skill directly into `/sandbox/.deepagents/agent/skills/`, the directory Deep Agents Code loads at session start. +The command creates an archive from the selected regular files and records their paths and hashes. +It rejects symlinks and special files. +Inside the sandbox, it stages the archive and verifies the same path-and-byte manifest. +It then moves the staged directory into place only if the destination is still absent. +Because Deep Agents and its built-in skill creator also own this directory, the command refuses any name whose file, directory, or symlink already exists. +Updates are not automatic. +Inspect the existing skill and use the agent's native or manual lifecycle after confirming ownership. +The legacy `/sandbox/.deepagents/skills/` path is not written or treated as ownership proof. +The managed `dcode` launchers discover newly installed skills on the next session without accepting executable hook configuration. +Installation does not enable project hooks or unmanaged MCP files. @@ -2340,14 +2347,16 @@ If you pass a plugin-shaped directory to `skill install`, the CLI prints a plugi Files with names starting with `.` (dotfiles) are skipped and listed in the output. Files with unsafe path characters are rejected to prevent shell injection. +Symlinks and other non-regular paths are rejected rather than followed or copied. -If the skill already exists on the sandbox, the command updates it in place and preserves chat history. -In both cases the agent discovers the change on its next session, using the per-agent mechanism described above. +For OpenClaw and Hermes, an existing sandbox skill is updated in place and chat history is preserved. +Deep Agents supports only fresh-name installs because its active skill directory is shared with agent-authored content. +Follow the agent-specific activation guidance above after installation. ### `$$nemoclaw skill remove ` Remove an installed skill from a running sandbox by skill name. -The command validates the skill name and removes the sandbox upload directory, so the remaining skills are rediscovered on the next session. +The command validates the skill name before it applies the agent-specific removal behavior below. @@ -2361,9 +2370,9 @@ Run `$$nemoclaw gateway restart` if prompted so the removal takes effect. -For Deep Agents, the command removes the uploaded skill from `/sandbox/.deepagents/skills/` and its mirror at `/sandbox/.deepagents/agent/skills/`. -Removing the mirror is what makes the skill stop loading, because Deep Agents Code reads its skills from that directory at session start; the next `dcode` session no longer lists it. -It does not enable project hooks or unmanaged MCP files. +For Deep Agents, automatic removal is refused before any sandbox files change. +The active `/sandbox/.deepagents/agent/skills/` directory is shared with agent-authored content, so its presence alone cannot prove NemoClaw owns it. +Inspect the skill and use the agent's native or manual lifecycle after confirming ownership. diff --git a/src/lib/actions/sandbox/skill-install.test.ts b/src/lib/actions/sandbox/skill-install.test.ts index 76a1f64aa57..51dc396e748 100644 --- a/src/lib/actions/sandbox/skill-install.test.ts +++ b/src/lib/actions/sandbox/skill-install.test.ts @@ -18,6 +18,7 @@ const skillInstall = vi.hoisted(() => ({ parseFrontmatter: vi.fn(), collectFiles: vi.fn(), uploadDirectory: vi.fn(), + installFreshSharedSkill: vi.fn(), postInstall: vi.fn(), verifyInstall: vi.fn(), })); @@ -39,14 +40,27 @@ vi.mock("./gateway-state", () => ({ import { installSandboxSkill, removeSandboxSkill } from "./skill-install"; const paths = { + stateDir: "/sandbox/.openclaw", uploadDir: "/sandbox/.openclaw/skills/demo-skill", mirrorDir: "$HOME/.openclaw/skills/demo-skill", - mirrorSharedWithAgent: false, + uploadDirSharedWithAgent: false, sessionFile: "/sandbox/.openclaw/agents/main/sessions/sessions.json", isOpenClaw: true, }; const agent = { name: "openclaw", configPaths: { dir: "/sandbox/.openclaw" } }; +const deepAgent = { + name: "langchain-deepagents-code", + configPaths: { dir: "/sandbox/.deepagents" }, +}; +const sharedPaths = { + stateDir: "/sandbox/.deepagents", + uploadDir: "/sandbox/.deepagents/agent/skills/demo-skill", + mirrorDir: null, + uploadDirSharedWithAgent: true, + sessionFile: null, + isOpenClaw: false, +}; function makeSkillDir(): string { const dir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-action-skill-")); @@ -93,6 +107,7 @@ describe("sandbox skill action orchestration", () => { files: ["SKILL.md"], skippedDotfiles: [], unsafePaths: [], + unsupportedPaths: [], }); skillInstall.uploadDirectory.mockReturnValue({ uploaded: 1, @@ -100,6 +115,11 @@ describe("sandbox skill action orchestration", () => { skippedDotfiles: [], unsafePaths: [], }); + skillInstall.installFreshSharedSkill.mockReturnValue({ + success: true, + uploaded: 1, + contentDigest: "a".repeat(64), + }); skillInstall.postInstall.mockReturnValue({ success: true, messages: [] }); skillInstall.verifyInstall.mockReturnValue(true); }); @@ -194,6 +214,22 @@ describe("sandbox skill action orchestration", () => { expect(process.exitCode).toBeUndefined(); }); + it("refuses Deep Agents removal before obtaining SSH configuration", async () => { + getSessionAgent.mockReturnValue(deepAgent); + skillInstall.resolveSkillPaths.mockReturnValue(sharedPaths); + const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + + await removeSandboxSkill("alpha", { name: "demo-skill" }); + + expect(process.exitCode).toBe(1); + expect(error).toHaveBeenCalledWith( + expect.stringContaining("Automatic removal is unavailable for Deep Agents skills"), + ); + expect(captureSandboxSshConfig).not.toHaveBeenCalled(); + expect(skillInstall.checkExisting).not.toHaveBeenCalled(); + expect(skillInstall.removeSkill).not.toHaveBeenCalled(); + }); + it("stops skill installation at the shared gateway liveness guard (#2276)", async () => { const skillDir = makeSkillDir(); ensureLiveSandboxOrExit.mockRejectedValueOnce(new Error("wrong gateway active")); @@ -248,6 +284,90 @@ describe("sandbox skill action orchestration", () => { expect(process.exitCode).toBeUndefined(); }); + it("fresh-installs and verifies Deep Agents content without generic upload mutation", async () => { + const skillDir = makeSkillDir(); + getSessionAgent.mockReturnValue(deepAgent); + skillInstall.resolveSkillPaths.mockReturnValue(sharedPaths); + let tempConfig = ""; + skillInstall.installFreshSharedSkill.mockImplementation((ctx) => { + tempConfig = ctx.configFile; + return { success: true, uploaded: 1, contentDigest: "a".repeat(64) }; + }); + const log = vi.spyOn(console, "log").mockImplementation(() => undefined); + + try { + await installSandboxSkill("alpha", { command: "install", path: skillDir }); + } finally { + fs.rmSync(skillDir, { recursive: true, force: true }); + } + + expect(skillInstall.installFreshSharedSkill).toHaveBeenCalledWith( + expect.objectContaining({ configFile: tempConfig, sandboxName: "alpha" }), + skillDir, + sharedPaths, + ); + expect(skillInstall.checkExisting).not.toHaveBeenCalled(); + expect(skillInstall.uploadDirectory).not.toHaveBeenCalled(); + expect(skillInstall.postInstall).not.toHaveBeenCalled(); + expect(log).toHaveBeenCalledWith(expect.stringContaining("Skill 'demo-skill' installed")); + expectTempSshConfigCleanedUp(tempConfig); + expect(process.exitCode).toBeUndefined(); + }); + + it("refuses a colliding Deep Agents destination without generic mutation", async () => { + const skillDir = makeSkillDir(); + getSessionAgent.mockReturnValue(deepAgent); + skillInstall.resolveSkillPaths.mockReturnValue(sharedPaths); + skillInstall.installFreshSharedSkill.mockReturnValue({ + success: false, + uploaded: 0, + reason: "destination_exists", + }); + const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + + try { + await installSandboxSkill("alpha", { command: "install", path: skillDir }); + } finally { + fs.rmSync(skillDir, { recursive: true, force: true }); + } + + expect(process.exitCode).toBe(1); + expect(error).toHaveBeenCalledWith( + expect.stringContaining("the Deep Agents skill destination already exists"), + ); + expect(skillInstall.checkExisting).not.toHaveBeenCalled(); + expect(skillInstall.uploadDirectory).not.toHaveBeenCalled(); + expect(skillInstall.postInstall).not.toHaveBeenCalled(); + }); + + it("reports unknown Deep Agents commit state with inspect-before-retry guidance", async () => { + const skillDir = makeSkillDir(); + getSessionAgent.mockReturnValue(deepAgent); + skillInstall.resolveSkillPaths.mockReturnValue(sharedPaths); + skillInstall.installFreshSharedSkill.mockReturnValue({ + success: false, + uploaded: 0, + reason: "remote_state_unknown", + }); + const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + + try { + await installSandboxSkill("alpha", { command: "install", path: skillDir }); + } finally { + fs.rmSync(skillDir, { recursive: true, force: true }); + } + + const output = error.mock.calls.map((args) => args.join(" ")).join("\n"); + expect(process.exitCode).toBe(1); + expect(output).toContain("did not confirm whether the Deep Agents skill was committed"); + expect(output).toContain( + "Inspect /sandbox/.deepagents/agent/skills/demo-skill before retrying", + ); + expect(skillInstall.checkExisting).not.toHaveBeenCalled(); + expect(skillInstall.uploadDirectory).not.toHaveBeenCalled(); + expect(skillInstall.postInstall).not.toHaveBeenCalled(); + }); + it("adds shields recovery guidance when skill upload fails (#6859)", async () => { const skillDir = makeSkillDir(); skillInstall.uploadDirectory.mockReturnValue({ diff --git a/src/lib/actions/sandbox/skill-install.ts b/src/lib/actions/sandbox/skill-install.ts index 84901442926..2519c58ac2d 100644 --- a/src/lib/actions/sandbox/skill-install.ts +++ b/src/lib/actions/sandbox/skill-install.ts @@ -122,6 +122,16 @@ export async function removeSandboxSkill( const agent = agentRuntime.getSessionAgent(sandboxName); const paths = skillInstall.resolveSkillPaths(agent, skillName); + if (paths.uploadDirSharedWithAgent) { + console.error( + " Automatic removal is unavailable for Deep Agents skills because the destination is shared with agent-authored content.", + ); + console.error( + " Inspect and remove the skill with the agent's native or manual workflow after confirming ownership.", + ); + process.exitCode = 1; + return; + } const sshConfigResult = captureSandboxSshConfig(sandboxName, { ignoreError: true, @@ -221,6 +231,10 @@ export async function installSandboxSkill( } const resolvedPath = path.resolve(skillPath); + if (fs.existsSync(resolvedPath) && fs.lstatSync(resolvedPath).isSymbolicLink()) { + console.error(` Skill path '${resolvedPath}' must not be a symbolic link.`); + process.exit(1); + } // Accept a directory containing SKILL.md, or a direct path to SKILL.md. let skillDir: string; @@ -248,6 +262,11 @@ export async function installSandboxSkill( } process.exit(1); } + const skillMdStat = fs.lstatSync(skillMdPath); + if (!skillMdStat.isFile() || skillMdStat.isSymbolicLink()) { + console.error(` SKILL.md at '${skillMdPath}' must be a regular file, not a symbolic link.`); + process.exit(1); + } // 1. Validate frontmatter let frontmatter; @@ -267,6 +286,12 @@ export async function installSandboxSkill( console.error(" File names must match [A-Za-z0-9._-/]. Rename or remove them."); process.exit(1); } + if (collected.unsupportedPaths.length > 0) { + console.error(" Skill directory contains unsupported non-regular paths:"); + for (const p of collected.unsupportedPaths) console.error(` ${p}`); + console.error(" Skills may contain only regular files and directories."); + process.exit(1); + } if (collected.skippedDotfiles.length > 0) { console.log( ` ${D}Skipping ${collected.skippedDotfiles.length} hidden path(s): ${collected.skippedDotfiles.join(", ")}${R}`, @@ -297,6 +322,34 @@ export async function installSandboxSkill( try { const ctx = { configFile: tmpSshConfig.file, sandboxName }; + if (paths.uploadDirSharedWithAgent) { + const fresh = skillInstall.installFreshSharedSkill(ctx, skillDir, paths); + if (!fresh.success || !fresh.contentDigest) { + if (fresh.reason === "destination_exists") { + console.error( + ` Refusing to replace '${frontmatter.name}': the Deep Agents skill destination already exists.`, + ); + console.error( + " Deep Agents skill install supports fresh names only because that directory also contains agent-authored skills.", + ); + } else if (fresh.reason === "snapshot_failed") { + console.error(" Failed to create an exact regular-file snapshot of the local skill."); + } else { + console.error( + " The remote install did not confirm whether the Deep Agents skill was committed.", + ); + console.error( + ` Inspect ${paths.uploadDir} before retrying; NemoClaw will not replace or delete shared agent content.`, + ); + } + process.exitCode = 1; + return; + } + console.log(` ${G}✓${R} Installed ${fresh.uploaded} file(s) into the agent skill directory`); + console.log(` ${G}✓${R} Skill '${frontmatter.name}' installed`); + return; + } + // 5. Check if skill already exists (update vs fresh install). This probe is // advisory for install only: stale SSH config files and transient remote // shell startup failures can make the stat probe inconclusive even when a diff --git a/src/lib/skill-install-shared.test.ts b/src/lib/skill-install-shared.test.ts new file mode 100644 index 00000000000..90d99b67456 --- /dev/null +++ b/src/lib/skill-install-shared.test.ts @@ -0,0 +1,311 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +import { spawnSync } from "node:child_process"; +import { + chmodSync, + existsSync, + lstatSync, + mkdirSync, + mkdtempSync, + readdirSync, + readFileSync, + rmSync, + symlinkSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; + +import { + computeSkillContentDigest, + installFreshSharedSkill, + resolveSkillPaths, +} from "./skill-install"; +import type { SshResult } from "./skill-remote"; + +const CTX = { configFile: "/tmp/ssh-config", sandboxName: "alpha" }; +const AGENT_NAME = "langchain-deepagents-code"; + +function makeSkill(): string { + const dir = mkdtempSync(join(tmpdir(), "nemoclaw-shared-skill-")); + writeFileSync(join(dir, "SKILL.md"), "---\nname: note-summarizer\n---\n# Notes\n"); + mkdirSync(join(dir, "scripts")); + writeFileSync(join(dir, "scripts", "summarize.js"), "export default 'exact';\n"); + return dir; +} + +function pathsFor(stateDir: string) { + return resolveSkillPaths({ name: AGENT_NAME, configPaths: { dir: stateDir } }, "note-summarizer"); +} + +function executeShell( + command: string, + input: string | Buffer | undefined, + env: NodeJS.ProcessEnv = process.env, +): SshResult { + const run = spawnSync("/bin/sh", ["-c", command], { + encoding: "utf8", + env, + input, + }); + return { + status: run.status ?? 1, + stdout: (run.stdout || "").trim(), + stderr: (run.stderr || "").trim(), + }; +} + +describe("fresh shared-agent skill install", () => { + it("streams one host-attested archive to an atomic no-clobber activation", () => { + const skillDir = makeSkill(); + const paths = pathsFor("/sandbox/.deepagents"); + const expected = computeSkillContentDigest(skillDir); + const commands: string[] = []; + try { + const result = installFreshSharedSkill(CTX, skillDir, paths, { + sshExecImpl: (_ctx, command, opts) => { + commands.push(command); + expect(Buffer.isBuffer(opts?.input)).toBe(true); + return { status: 0, stdout: `INSTALLED ${expected}`, stderr: "" }; + }, + }); + + expect(result).toEqual({ + success: true, + uploaded: 2, + contentDigest: expected, + }); + expect(paths.uploadDir).toBe("/sandbox/.deepagents/agent/skills/note-summarizer"); + expect(paths.mirrorDir).toBeNull(); + expect(paths.uploadDirSharedWithAgent).toBe(true); + expect(commands[0]).toContain("tar --no-same-owner --no-same-permissions -xf -"); + expect(commands[0]).toContain("sha256sum"); + expect(commands[0]).toContain('mv -nT -- "$payload" "$leaf"'); + expect(commands[0]).toContain('exists "$payload"'); + expect(commands[0]).toContain("MOVE_FAILED"); + expect(commands[0]).not.toContain("$workspace/active.manifest"); + expect(commands[0]).not.toContain("/sandbox/.deepagents/skills/note-summarizer"); + expect(commands[0]).not.toContain('rm -rf -- "$leaf"'); + } finally { + rmSync(skillDir, { recursive: true, force: true }); + } + }); + + it("rejects a snapshot whose SKILL.md name does not match the resolved destination", () => { + const skillDir = makeSkill(); + const paths = pathsFor("/sandbox/.deepagents"); + writeFileSync(join(skillDir, "SKILL.md"), "---\nname: different-skill\n---\n# Notes\n"); + let called = false; + try { + const result = installFreshSharedSkill(CTX, skillDir, paths, { + sshExecImpl: () => { + called = true; + return { status: 0, stdout: "", stderr: "" }; + }, + }); + + expect(result).toEqual({ + success: false, + uploaded: 0, + reason: "snapshot_failed", + }); + expect(called).toBe(false); + } finally { + rmSync(skillDir, { recursive: true, force: true }); + } + }); + + it("rejects a snapshot that does not contain a regular SKILL.md", () => { + const skillDir = makeSkill(); + const paths = pathsFor("/sandbox/.deepagents"); + rmSync(join(skillDir, "SKILL.md")); + let called = false; + try { + const result = installFreshSharedSkill(CTX, skillDir, paths, { + sshExecImpl: () => { + called = true; + return { status: 0, stdout: "", stderr: "" }; + }, + }); + + expect(result).toEqual({ + success: false, + uploaded: 0, + reason: "snapshot_failed", + }); + expect(called).toBe(false); + } finally { + rmSync(skillDir, { recursive: true, force: true }); + } + }); + + it.runIf(process.platform === "linux")( + "installs exact bytes directly and leaves the legacy upload path untouched", + () => { + const skillDir = makeSkill(); + const stateDir = mkdtempSync(join(tmpdir(), "nemoclaw-shared-state-")); + const paths = pathsFor(stateDir); + const legacy = join(stateDir, "skills", "note-summarizer"); + mkdirSync(legacy, { recursive: true }); + writeFileSync(join(legacy, "legacy.txt"), "preserve me\n"); + try { + const result = installFreshSharedSkill(CTX, skillDir, paths, { + sshExecImpl: (_ctx, command, opts) => executeShell(command, opts?.input), + }); + + expect(result.success).toBe(true); + expect(readFileSync(join(paths.uploadDir, "SKILL.md"), "utf8")).toContain("# Notes"); + expect(readFileSync(join(paths.uploadDir, "scripts", "summarize.js"), "utf8")).toBe( + "export default 'exact';\n", + ); + expect(readFileSync(join(legacy, "legacy.txt"), "utf8")).toBe("preserve me\n"); + } finally { + rmSync(skillDir, { recursive: true, force: true }); + rmSync(stateDir, { recursive: true, force: true }); + } + }, + ); + + it.runIf(process.platform === "linux")( + "refuses existing files, directories, and symlinks without changing them", + () => { + for (const kind of ["file", "directory", "symlink"] as const) { + const skillDir = makeSkill(); + const stateDir = mkdtempSync(join(tmpdir(), `nemoclaw-shared-${kind}-`)); + const paths = pathsFor(stateDir); + mkdirSync(join(stateDir, "agent", "skills"), { recursive: true }); + const outside = join(stateDir, "outside"); + writeFileSync(outside, "outside\n"); + if (kind === "file") { + writeFileSync(paths.uploadDir, "agent file\n"); + } else if (kind === "directory") { + mkdirSync(paths.uploadDir); + writeFileSync(join(paths.uploadDir, "agent.txt"), "agent directory\n"); + } else { + symlinkSync(outside, paths.uploadDir); + } + const before = lstatSync(paths.uploadDir); + try { + const result = installFreshSharedSkill(CTX, skillDir, paths, { + sshExecImpl: (_ctx, command, opts) => executeShell(command, opts?.input), + }); + + expect(result).toEqual({ + success: false, + uploaded: 0, + reason: "destination_exists", + }); + expect(lstatSync(paths.uploadDir).isSymbolicLink()).toBe(before.isSymbolicLink()); + if (kind === "file") { + expect(readFileSync(paths.uploadDir, "utf8")).toBe("agent file\n"); + } else if (kind === "directory") { + expect(readFileSync(join(paths.uploadDir, "agent.txt"), "utf8")).toBe( + "agent directory\n", + ); + } else { + expect(readFileSync(paths.uploadDir, "utf8")).toBe("outside\n"); + } + } finally { + rmSync(skillDir, { recursive: true, force: true }); + rmSync(stateDir, { recursive: true, force: true }); + } + } + }, + ); + + it.runIf(process.platform === "linux")( + "detects a no-clobber race when mv reports success but leaves staging", + () => { + const skillDir = makeSkill(); + const stateDir = mkdtempSync(join(tmpdir(), "nemoclaw-shared-race-")); + const fakeBin = mkdtempSync(join(tmpdir(), "nemoclaw-fake-mv-")); + const paths = pathsFor(stateDir); + const fakeMv = join(fakeBin, "mv"); + writeFileSync(fakeMv, '#!/bin/sh\nmkdir -- "$RACE_DEST"\nexec /usr/bin/mv "$@"\n'); + chmodSync(fakeMv, 0o755); + try { + const result = installFreshSharedSkill(CTX, skillDir, paths, { + sshExecImpl: (_ctx, command, opts) => + executeShell(command, opts?.input, { + ...process.env, + PATH: `${fakeBin}:${process.env.PATH}`, + RACE_DEST: "note-summarizer", + }), + }); + + expect(result.reason).toBe("destination_exists"); + expect(existsSync(paths.uploadDir)).toBe(true); + expect(readdirSync(paths.uploadDir)).toEqual([]); + expect( + readdirSync(join(stateDir, "agent", "skills")).some((name) => + name.startsWith(".nemoclaw-skill."), + ), + ).toBe(false); + } finally { + rmSync(skillDir, { recursive: true, force: true }); + rmSync(stateDir, { recursive: true, force: true }); + rmSync(fakeBin, { recursive: true, force: true }); + } + }, + ); + + it.runIf(process.platform === "linux")( + "fails closed on a corrupt archive and leaves no active destination", + () => { + const skillDir = makeSkill(); + const stateDir = mkdtempSync(join(tmpdir(), "nemoclaw-shared-corrupt-")); + const paths = pathsFor(stateDir); + try { + const result = installFreshSharedSkill(CTX, skillDir, paths, { + sshExecImpl: (_ctx, command) => executeShell(command, Buffer.from("not a tar archive")), + }); + + expect(result.reason).toBe("remote_state_unknown"); + expect(existsSync(paths.uploadDir)).toBe(false); + expect( + readdirSync(join(stateDir, "agent", "skills")).some((name) => + name.startsWith(".nemoclaw-skill."), + ), + ).toBe(false); + } finally { + rmSync(skillDir, { recursive: true, force: true }); + rmSync(stateDir, { recursive: true, force: true }); + } + }, + ); + + it.runIf(process.platform === "linux")( + "classifies a non-collision move failure as unknown remote state", + () => { + const skillDir = makeSkill(); + const stateDir = mkdtempSync(join(tmpdir(), "nemoclaw-shared-move-failure-")); + const fakeBin = mkdtempSync(join(tmpdir(), "nemoclaw-failing-mv-")); + const paths = pathsFor(stateDir); + const fakeMv = join(fakeBin, "mv"); + writeFileSync(fakeMv, "#!/bin/sh\nexit 1\n"); + chmodSync(fakeMv, 0o755); + try { + const result = installFreshSharedSkill(CTX, skillDir, paths, { + sshExecImpl: (_ctx, command, opts) => + executeShell(command, opts?.input, { + ...process.env, + PATH: `${fakeBin}:${process.env.PATH}`, + }), + }); + + expect(result).toEqual({ + success: false, + uploaded: 0, + reason: "remote_state_unknown", + }); + expect(existsSync(paths.uploadDir)).toBe(false); + } finally { + rmSync(skillDir, { recursive: true, force: true }); + rmSync(stateDir, { recursive: true, force: true }); + rmSync(fakeBin, { recursive: true, force: true }); + } + }, + ); +}); diff --git a/src/lib/skill-install.test.ts b/src/lib/skill-install.test.ts index 8611ffdfbfa..3a6a1585359 100644 --- a/src/lib/skill-install.test.ts +++ b/src/lib/skill-install.test.ts @@ -1,7 +1,7 @@ // SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. // SPDX-License-Identifier: Apache-2.0 -import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { describe, expect, it } from "vitest"; @@ -196,13 +196,27 @@ describe("collectFiles", () => { cleanup(); } }); + + it("rejects visible symlinks instead of following their targets", () => { + setup({ "SKILL.md": "---\nname: linked\n---\n" }); + try { + symlinkSync("SKILL.md", join(tmpDir, "alias.md")); + const { files, unsupportedPaths } = collectFiles(tmpDir); + expect(files).toEqual(["SKILL.md"]); + expect(unsupportedPaths).toEqual(["alias.md"]); + } finally { + cleanup(); + } + }); }); describe("resolveSkillPaths", () => { it("returns OpenClaw defaults when agent is null", () => { const paths = resolveSkillPaths(null, "weather"); + expect(paths.stateDir).toBe("/sandbox/.openclaw"); expect(paths.uploadDir).toBe("/sandbox/.openclaw/skills/weather"); expect(paths.mirrorDir).toBe("$HOME/.openclaw/skills/weather"); + expect(paths.uploadDirSharedWithAgent).toBe(false); expect(paths.sessionFile).toBe("/sandbox/.openclaw/agents/main/sessions/sessions.json"); expect(paths.isOpenClaw).toBe(true); }); @@ -215,8 +229,10 @@ describe("resolveSkillPaths", () => { }, }; const paths = resolveSkillPaths(agent, "my-skill"); + expect(paths.stateDir).toBe("/sandbox/.openclaw"); expect(paths.uploadDir).toBe("/sandbox/.openclaw/skills/my-skill"); expect(paths.mirrorDir).toBe("$HOME/.openclaw/skills/my-skill"); + expect(paths.uploadDirSharedWithAgent).toBe(false); expect(paths.sessionFile).toBe("/sandbox/.openclaw/agents/main/sessions/sessions.json"); expect(paths.isOpenClaw).toBe(true); }); @@ -229,13 +245,15 @@ describe("resolveSkillPaths", () => { }, }; const paths = resolveSkillPaths(agent, "demo-skill"); + expect(paths.stateDir).toBe("/sandbox/.hermes"); expect(paths.uploadDir).toBe("/sandbox/.hermes/skills/demo-skill"); expect(paths.mirrorDir).toBeNull(); + expect(paths.uploadDirSharedWithAgent).toBe(false); expect(paths.sessionFile).toBeNull(); expect(paths.isOpenClaw).toBe(false); }); - it("mirrors Deep Agents skills into the agent skills dir dcode loads (#7634)", () => { + it("installs Deep Agents skills directly into the agent skills dir dcode loads (#7634)", () => { // dcode's user skill dir is ~/.deepagents/{agent}/skills (HOME=/sandbox, // DEFAULT_AGENT_NAME="agent"); it never scans ~/.deepagents/skills, so the // upload dir alone leaves the skill installed but unloadable. @@ -246,8 +264,10 @@ describe("resolveSkillPaths", () => { }, }; const paths = resolveSkillPaths(agent, "note-summarizer"); - expect(paths.uploadDir).toBe("/sandbox/.deepagents/skills/note-summarizer"); - expect(paths.mirrorDir).toBe("/sandbox/.deepagents/agent/skills/note-summarizer"); + expect(paths.stateDir).toBe("/sandbox/.deepagents"); + expect(paths.uploadDir).toBe("/sandbox/.deepagents/agent/skills/note-summarizer"); + expect(paths.mirrorDir).toBeNull(); + expect(paths.uploadDirSharedWithAgent).toBe(true); expect(paths.sessionFile).toBeNull(); expect(paths.isOpenClaw).toBe(false); }); @@ -260,18 +280,15 @@ describe("resolveSkillPaths", () => { }, }; const paths = resolveSkillPaths(agent, "test-skill"); + expect(paths.stateDir).toBe("/sandbox/.future"); expect(paths.uploadDir).toBe("/sandbox/.future/skills/test-skill"); expect(paths.mirrorDir).toBeNull(); + expect(paths.uploadDirSharedWithAgent).toBe(false); expect(paths.sessionFile).toBeNull(); expect(paths.isOpenClaw).toBe(false); }); }); -const DEEPAGENTS_AGENT = { - name: "langchain-deepagents-code", - configPaths: { dir: "/sandbox/.deepagents" }, -}; - describe("postInstall", () => { it("refreshes OpenClaw sessions after installing an updated skill", () => { const skillDir = mkdtempSync(join(tmpdir(), "skill-postinstall-")); @@ -356,43 +373,6 @@ describe("postInstall", () => { rmSync(skillDir, { recursive: true, force: true }); } }); - - it("mirrors a Deep Agents skill into agent/skills instead of hinting a restart (#7634)", () => { - // Deep Agents Code is a terminal runtime with no gateway to restart; the - // only way the skill becomes loadable is the agent/skills mirror. - const skillDir = mkdtempSync(join(tmpdir(), "skill-postinstall-dcode-")); - const commands: string[] = []; - try { - writeFileSync(skillDir + "/SKILL.md", "---\nname: note-summarizer\n---\n# Notes\n"); - const paths = resolveSkillPaths(DEEPAGENTS_AGENT, "note-summarizer"); - const result = postInstall( - { configFile: "/tmp/ssh-config", sandboxName: "alpha" }, - paths, - skillDir, - { - sshExecImpl: (_ctx, command) => { - commands.push(command); - return { status: 0, stdout: "", stderr: "" }; - }, - }, - ); - - expect(result).toEqual({ success: true, messages: [] }); - const mirrorCmd = commands.find( - (c) => - c.includes(paths.uploadDir) && - c.includes('"/sandbox/.deepagents/agent/skills/note-summarizer"'), - ); - expect( - mirrorCmd, - "postInstall should mirror the skill into /sandbox/.deepagents/agent/skills", - ).toBeDefined(); - // No sessions.json exists for a terminal runtime, so nothing may clear one. - expect(commands.some((c) => c.includes("sessions.json"))).toBe(false); - } finally { - rmSync(skillDir, { recursive: true, force: true }); - } - }); }); describe("verifyInstall", () => { @@ -426,23 +406,4 @@ describe("verifyInstall", () => { expect(ok).toBe(false); }); - - it("requires SKILL.md in the Deep Agents agent/skills mirror (#7634)", () => { - // Verifying only the upload dir is what let `skill install` report success - // for a skill dcode could never load. - const paths = resolveSkillPaths(DEEPAGENTS_AGENT, "note-summarizer"); - const commands: string[] = []; - verifyInstall({ configFile: "/tmp/ssh-config", sandboxName: "alpha" }, paths, { - sshExecImpl: (_ctx, command) => { - commands.push(command); - return { status: 0, stdout: "EXISTS", stderr: "" }; - }, - }); - - expect( - commands.some((c) => - c.includes('"/sandbox/.deepagents/agent/skills/note-summarizer/SKILL.md"'), - ), - ).toBe(true); - }); }); diff --git a/src/lib/skill-install.ts b/src/lib/skill-install.ts index a58f60a38b7..1b929cd2bef 100644 --- a/src/lib/skill-install.ts +++ b/src/lib/skill-install.ts @@ -8,7 +8,10 @@ // OpenClaw). Non-OpenClaw agents get a "restart gateway" hint until a // generic refresh contract is defined in the manifest schema. +import { spawnSync } from "node:child_process"; +import { createHash } from "node:crypto"; import fs from "node:fs"; +import os from "node:os"; import path from "node:path"; // yaml is a production dependency (used by policies.ts, onboard.ts) @@ -94,16 +97,18 @@ export function parseFrontmatter(content: string): SkillFrontmatter { // ── Path resolution ────────────────────────────────────────────── export interface SkillPaths { + /** Agent state root that contains the resolved skill paths. */ + stateDir: string; /** Upload target directory for the skill */ uploadDir: string; /** Directory the agent actually loads skills from, or null when it equals uploadDir */ mirrorDir: string | null; /** - * Whether the agent's own tooling also writes into mirrorDir. When true the - * mirror is NOT proof that NemoClaw installed a skill, so it must not be used - * as the existence gate for `skill remove`. + * Whether the agent's own tooling also writes into uploadDir. Shared + * destinations support only atomic fresh installs because their existing + * content is not proof that NemoClaw owns it. */ - mirrorSharedWithAgent: boolean; + uploadDirSharedWithAgent: boolean; /** OpenClaw-only: session index to clear, or null */ sessionFile: string | null; /** Whether the agent is OpenClaw (drives refresh behavior) */ @@ -119,25 +124,21 @@ export interface SkillPaths { * unregistered — absent from the agent's skill list (#4819 for OpenClaw, #7634 * for Deep Agents Code, whose user skill dir is `~/.deepagents/{agent}/skills`). * - * `sharedWithAgent` marks a mirror the agent's own tooling also writes into: - * Deep Agents Code's skill-creator authors user skills straight into - * `agent/skills` (#5753), so a directory there is not proof NemoClaw installed - * it. `checkExisting()` depends on that distinction. - * * Remove an entry when its agent starts loading skills from `uploadDir`. */ -const AGENT_SKILL_MIRRORS: Record< - string, - { dir: (dir: string, skillName: string) => string; sharedWithAgent: boolean } -> = { - openclaw: { - dir: (_dir, skillName) => `$HOME/.openclaw/skills/${skillName}`, - sharedWithAgent: false, - }, - "langchain-deepagents-code": { - dir: (dir, skillName) => `${dir}/agent/skills/${skillName}`, - sharedWithAgent: true, - }, +const AGENT_SKILL_MIRRORS: Record string> = { + openclaw: (_dir, skillName) => `$HOME/.openclaw/skills/${skillName}`, +}; + +/** + * Agent-owned skill directories that are also the loader's canonical source. + * + * Deep Agents Code's built-in skill creator writes directly to + * `agent/skills` (#5753). NemoClaw therefore installs there only when the + * destination is absent and never treats an existing directory as managed. + */ +const AGENT_SHARED_SKILL_DIRS: Record string> = { + "langchain-deepagents-code": (dir, skillName) => `${dir}/agent/skills/${skillName}`, }; /** @@ -153,14 +154,15 @@ export function resolveSkillPaths( const isOpenClaw = !agent || agent.name === "openclaw"; const dir = agent ? agent.configPaths.dir : "/sandbox/.openclaw"; - - const uploadDir = `${dir}/skills/${skillName}`; - const mirror = AGENT_SKILL_MIRRORS[agent ? agent.name : "openclaw"]; + const agentName = agent ? agent.name : "openclaw"; + const sharedDir = AGENT_SHARED_SKILL_DIRS[agentName]; + const mirror = AGENT_SKILL_MIRRORS[agentName]; return { - uploadDir, - mirrorDir: mirror ? mirror.dir(dir, skillName) : null, - mirrorSharedWithAgent: mirror ? mirror.sharedWithAgent : false, + stateDir: dir, + uploadDir: sharedDir ? sharedDir(dir, skillName) : `${dir}/skills/${skillName}`, + mirrorDir: mirror ? mirror(dir, skillName) : null, + uploadDirSharedWithAgent: Boolean(sharedDir), sessionFile: isOpenClaw ? `${dir}/agents/main/sessions/sessions.json` : null, isOpenClaw, }; @@ -203,6 +205,7 @@ export interface CollectedFiles { files: string[]; skippedDotfiles: string[]; unsafePaths: string[]; + unsupportedPaths: string[]; } /** @@ -215,6 +218,7 @@ export function collectFiles(dir: string): CollectedFiles { const files: string[] = []; const skippedDotfiles: string[] = []; const unsafePaths: string[] = []; + const unsupportedPaths: string[] = []; function walk(current: string, prefix: string) { for (const entry of fs.readdirSync(current, { withFileTypes: true })) { @@ -231,11 +235,19 @@ export function collectFiles(dir: string): CollectedFiles { } else { files.push(rel); } + } else { + // Never follow symlinks or copy sockets, FIFOs, or device nodes across + // the host-to-sandbox trust boundary. + unsupportedPaths.push(rel); } } } walk(dir, ""); - return { files, skippedDotfiles, unsafePaths }; + files.sort(); + skippedDotfiles.sort(); + unsafePaths.sort(); + unsupportedPaths.sort(); + return { files, skippedDotfiles, unsafePaths, unsupportedPaths }; } /** @@ -247,9 +259,10 @@ export function uploadDirectory( localDir: string, remoteDir: string, ): { uploaded: number; failed: string[]; skippedDotfiles: string[]; unsafePaths: string[] } { - const { files, skippedDotfiles, unsafePaths } = collectFiles(localDir); - if (unsafePaths.length > 0) { - return { uploaded: 0, failed: unsafePaths, skippedDotfiles, unsafePaths }; + const { files, skippedDotfiles, unsafePaths, unsupportedPaths } = collectFiles(localDir); + const rejected = [...unsafePaths, ...unsupportedPaths]; + if (rejected.length > 0) { + return { uploaded: 0, failed: rejected, skippedDotfiles, unsafePaths }; } const failed: string[] = []; for (const rel of files) { @@ -263,6 +276,205 @@ export function uploadDirectory( return { uploaded: files.length - failed.length, failed, skippedDotfiles, unsafePaths }; } +const SHA256_RE = /^[a-f0-9]{64}$/; +const SKILL_SNAPSHOT_TIMEOUT_MS = 30_000; + +function fileSha256(filePath: string): string { + return createHash("sha256").update(fs.readFileSync(filePath)).digest("hex"); +} + +/** Hash the sorted regular-file path and byte set used by a skill archive. */ +export function computeSkillContentDigest(localDir: string, files?: string[]): string { + const selected = files ?? collectFiles(localDir).files; + const manifest = selected + .slice() + .sort() + .map((rel) => `${fileSha256(path.join(localDir, rel))} ${rel}\n`) + .join(""); + return createHash("sha256").update(manifest).digest("hex"); +} + +interface SkillArchiveSnapshot { + archive: Buffer; + contentDigest: string; + files: string[]; + skillName: string; +} + +/** + * Create one immutable archive, then derive the expected manifest from that + * archive rather than re-reading the mutable source tree. + */ +function createSkillArchiveSnapshot( + localDir: string, + files: string[], +): SkillArchiveSnapshot | null { + const archiveResult = spawnSync("tar", ["-cf", "-", "-C", localDir, "--", ...files], { + encoding: null, + env: { ...process.env, COPYFILE_DISABLE: "1" }, + maxBuffer: 256 * 1024 * 1024, + timeout: SKILL_SNAPSHOT_TIMEOUT_MS, + }); + if (archiveResult.status !== 0 || !Buffer.isBuffer(archiveResult.stdout)) return null; + + const snapshotDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-skill-snapshot-")); + try { + const extractResult = spawnSync("tar", ["-xf", "-", "-C", snapshotDir], { + encoding: null, + input: archiveResult.stdout, + maxBuffer: 16 * 1024 * 1024, + timeout: SKILL_SNAPSHOT_TIMEOUT_MS, + }); + if (extractResult.status !== 0) return null; + const snapshot = collectFiles(snapshotDir); + if ( + snapshot.unsafePaths.length > 0 || + snapshot.unsupportedPaths.length > 0 || + !snapshot.files.includes("SKILL.md") || + snapshot.files.join("\n") !== files.slice().sort().join("\n") + ) { + return null; + } + let skillName: string; + try { + skillName = parseFrontmatter( + fs.readFileSync(path.join(snapshotDir, "SKILL.md"), "utf8"), + ).name; + } catch { + return null; + } + return { + archive: archiveResult.stdout, + contentDigest: computeSkillContentDigest(snapshotDir, snapshot.files), + files: snapshot.files, + skillName, + }; + } finally { + fs.rmSync(snapshotDir, { recursive: true, force: true }); + } +} + +function buildFreshSharedInstallScript(paths: SkillPaths, expectedDigest: string): string { + const relativeUpload = path.posix.relative(paths.stateDir, paths.uploadDir); + const parentParts = path.posix.dirname(relativeUpload).split("/"); + const leaf = path.posix.basename(relativeUpload); + if ( + !validateRelativePath(relativeUpload) || + parentParts.some((part) => !validateRelativePath(part)) || + !validateRelativePath(leaf) + ) { + throw new Error("Shared agent skill path is not a safe relative destination"); + } + + const lines = [ + "set -eu", + `root=${shellQuote(paths.stateDir)}`, + `leaf=${shellQuote(leaf)}`, + `expected=${shellQuote(expectedDigest)}`, + 'exists() { [ -e "$1" ] || [ -L "$1" ]; }', + 'safe_rel() { case "$1" in ""|/*|*//*|*[!A-Za-z0-9._/-]*) return 1 ;; esac; case "/$1/" in *"/./"*|*"/../"*) return 1 ;; esac; }', + '[ -d "$root" ] && [ ! -L "$root" ] && [ "$(realpath -e -- "$root")" = "$root" ]', + 'cd -P -- "$root"', + '[ "$(pwd -P)" = "$root" ]', + ]; + + let expectedParent = paths.stateDir; + for (const part of parentParts) { + expectedParent = `${expectedParent}/${part}`; + lines.push( + `part=${shellQuote(part)}`, + '[ ! -L "$part" ]', + 'if [ ! -e "$part" ]; then mkdir -- "$part"; fi', + '[ -d "$part" ] && [ ! -L "$part" ]', + 'cd -P -- "$part"', + `[ "$(pwd -P)" = ${shellQuote(expectedParent)} ]`, + ); + } + + lines.push( + 'if exists "$leaf"; then echo EXISTS; exit 2; fi', + 'workspace="$(mktemp -d .nemoclaw-skill.XXXXXX)"', + 'chmod 700 "$workspace"', + 'payload="$workspace/payload"', + 'cleanup() { if exists "$workspace"; then rm -rf -- "$workspace"; fi; }', + "trap cleanup EXIT HUP INT TERM", + 'mkdir -- "$payload"', + 'tar --no-same-owner --no-same-permissions -xf - -C "$payload"', + '[ -z "$(find "$payload" -mindepth 1 ! -type d ! -type f -print -quit)" ]', + 'find "$payload" -type f -printf "%P\\n" | LC_ALL=C sort > "$workspace/files"', + ': > "$workspace/manifest"', + 'while IFS= read -r rel; do safe_rel "$rel"; hash="$(sha256sum "$payload/$rel" | cut -d " " -f 1)"; printf "%s %s\\n" "$hash" "$rel" >> "$workspace/manifest"; done < "$workspace/files"', + 'staged="$(sha256sum "$workspace/manifest" | cut -d " " -f 1)"', + '[ "$staged" = "$expected" ]', + 'if exists "$leaf"; then echo EXISTS; exit 2; fi', + 'if ! mv -nT -- "$payload" "$leaf"; then if exists "$leaf"; then echo EXISTS; exit 2; fi; echo MOVE_FAILED; exit 3; fi', + 'if exists "$payload"; then echo EXISTS; exit 2; fi', + 'printf "INSTALLED %s\\n" "$expected"', + ); + return lines.join("; "); +} + +export interface FreshSharedSkillInstallResult { + success: boolean; + uploaded: number; + contentDigest?: string; + reason?: "destination_exists" | "snapshot_failed" | "remote_state_unknown"; +} + +/** + * Install into an agent-owned loader directory only when the destination is + * absent. Existing content is never renamed, deleted, or replaced. + */ +export function installFreshSharedSkill( + ctx: SshContext, + localDir: string, + paths: SkillPaths, + opts: { sshExecImpl?: typeof sshExec } = {}, +): FreshSharedSkillInstallResult { + if (!paths.uploadDirSharedWithAgent || paths.mirrorDir) { + return { success: false, uploaded: 0, reason: "remote_state_unknown" }; + } + const collected = collectFiles(localDir); + if ( + collected.files.length === 0 || + collected.unsafePaths.length > 0 || + collected.unsupportedPaths.length > 0 + ) { + return { success: false, uploaded: 0, reason: "snapshot_failed" }; + } + const snapshot = createSkillArchiveSnapshot(localDir, collected.files); + if ( + !snapshot || + !SHA256_RE.test(snapshot.contentDigest) || + snapshot.skillName !== path.posix.basename(paths.uploadDir) + ) { + return { success: false, uploaded: 0, reason: "snapshot_failed" }; + } + const runSsh = opts.sshExecImpl ?? sshExec; + const result = runSsh(ctx, buildFreshSharedInstallScript(paths, snapshot.contentDigest), { + input: snapshot.archive, + }); + if ( + result !== null && + result.status === 0 && + result.stdout === `INSTALLED ${snapshot.contentDigest}` + ) { + return { + success: true, + uploaded: snapshot.files.length, + contentDigest: snapshot.contentDigest, + }; + } + return { + success: false, + uploaded: 0, + reason: + result?.status === 2 && result.stdout === "EXISTS" + ? "destination_exists" + : "remote_state_unknown", + }; +} + /** * Run post-install steps: skill-load mirror for every agent that needs one, * session refresh for OpenClaw, and a restart hint when neither applies. diff --git a/src/lib/skill-remote.test.ts b/src/lib/skill-remote.test.ts index 9dccd3a7985..fd6d12b6ec3 100644 --- a/src/lib/skill-remote.test.ts +++ b/src/lib/skill-remote.test.ts @@ -85,11 +85,7 @@ describe("removeSkill (unit — no SSH)", () => { ]); }); - it("does not treat an agent-authored Deep Agents skill as installed (#7634)", () => { - // dcode's own skill-creator writes user skills into agent/skills (#5753), - // so a directory there can exist without NemoClaw ever installing it. - // checkExisting gates `skill remove`; probing the shared mirror here would - // let remove delete a skill the user created in-sandbox. + it("probes the canonical Deep Agents directory for diagnostics (#7634)", () => { const ctx = { configFile: "/tmp/ssh.conf", sandboxName: "test-sandbox" }; const paths = resolveSkillPaths( { name: "langchain-deepagents-code", configPaths: { dir: "/sandbox/.deepagents" } }, @@ -103,14 +99,13 @@ describe("removeSkill (unit — no SSH)", () => { }, }); - expect(paths.mirrorSharedWithAgent).toBe(true); - expect(commands[0]).toContain("/sandbox/.deepagents/skills/user-authored"); - expect(commands[0]).not.toContain("agent/skills/user-authored"); + expect(paths.uploadDirSharedWithAgent).toBe(true); + expect(commands).toEqual([ + "{ test -e '/sandbox/.deepagents/agent/skills/user-authored'; } && echo EXISTS || echo ABSENT", + ]); }); - it("removes the Deep Agents agent/skills mirror so the skill stops loading (#7634)", () => { - // Install now populates agent/skills; remove must delete it or dcode keeps - // loading a skill the user removed. + it("refuses to remove from the agent-owned Deep Agents directory (#7634)", () => { const ctx = { configFile: "/tmp/ssh.conf", sandboxName: "test-sandbox" }; const paths = resolveSkillPaths( { name: "langchain-deepagents-code", configPaths: { dir: "/sandbox/.deepagents" } }, @@ -124,13 +119,14 @@ describe("removeSkill (unit — no SSH)", () => { }, }); - expect(result.success).toBe(true); - expect(result.removedMirrorDir).toBe(true); + expect(result.success).toBe(false); + expect(result.removedUploadDir).toBe(false); + expect(result.removedMirrorDir).toBe(false); expect(result.clearedSessions).toBe(false); - expect(commands).toEqual([ - "rm -rf '/sandbox/.deepagents/skills/test-skill'", - 'rm -rf "/sandbox/.deepagents/agent/skills/test-skill"', + expect(result.messages).toEqual([ + "Error: automatic removal is unavailable for the agent-owned skill directory /sandbox/.deepagents/agent/skills/test-skill.", ]); + expect(commands).toEqual([]); }); }); @@ -150,6 +146,23 @@ describe("verifyRemove (unit — no SSH)", () => { expect(verifyRemove(ctx, paths)).toBe(false); }); + it("refuses shared Deep Agents verification without an SSH call", () => { + const paths = resolveSkillPaths( + { name: "langchain-deepagents-code", configPaths: { dir: "/sandbox/.deepagents" } }, + "test-skill", + ); + const commands: string[] = []; + const gone = verifyRemove({ configFile: "/tmp/ssh.conf", sandboxName: "test-sandbox" }, paths, { + sshExecImpl: (_ctx, command) => { + commands.push(command); + return { status: 0, stdout: "GONE", stderr: "" }; + }, + }); + + expect(gone).toBe(false); + expect(commands).toEqual([]); + }); + it("verifies both OpenClaw skill directories are gone", () => { const paths = resolveSkillPaths(null, "test-skill"); const ctx = { configFile: "/tmp/ssh.conf", sandboxName: "test-sandbox" }; diff --git a/src/lib/skill-remote.ts b/src/lib/skill-remote.ts index 60ea58f2157..1285ede89d1 100644 --- a/src/lib/skill-remote.ts +++ b/src/lib/skill-remote.ts @@ -78,12 +78,11 @@ export function checkExisting( paths: SkillPaths, opts: { sshExecImpl?: typeof sshExec } = {}, ): boolean | null { - // Existence gate for `skill remove`, so it must answer "did NemoClaw install - // this skill?" — uploadDir is the ownership marker. A mirror the agent's own - // tooling writes into is not evidence of an install (#5753), and counting it - // would let `skill remove` delete a skill the user authored in-sandbox. + // Existence gate for `skill remove`. Shared agent destinations are probed + // only for user-facing diagnostics; removeSkill() still refuses to mutate + // them because their presence is not proof of NemoClaw ownership (#5753). const checks = [`test -e ${shellQuote(paths.uploadDir)}`]; - if (paths.mirrorDir && !paths.mirrorSharedWithAgent) { + if (paths.mirrorDir) { checks.push(`test -e "${paths.mirrorDir}"`); } const runSsh = opts.sshExecImpl ?? sshExec; @@ -120,6 +119,18 @@ export function removeSkill( const messages: string[] = []; const runSsh = opts.sshExecImpl ?? sshExec; + if (paths.uploadDirSharedWithAgent) { + return { + success: false, + removedUploadDir: false, + removedMirrorDir: false, + clearedSessions: false, + messages: [ + `Error: automatic removal is unavailable for the agent-owned skill directory ${paths.uploadDir}.`, + ], + }; + } + // 1. Remove the immutable upload directory (/sandbox/.openclaw/skills//) const uploadDir = shellQuote(paths.uploadDir); const removeUpload = runSsh(ctx, `rm -rf ${uploadDir}`); @@ -176,6 +187,7 @@ export function verifyRemove( paths: SkillPaths, opts: { sshExecImpl?: typeof sshExec } = {}, ): boolean { + if (paths.uploadDirSharedWithAgent) return false; const checks = [`test ! -e ${shellQuote(paths.uploadDir)}`]; if (paths.mirrorDir) { checks.push(`test ! -e "${paths.mirrorDir}"`); From 55e20ef20a46f28d89ba4f3132b27afe7592d879 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Tue, 28 Jul 2026 11:01:45 -0700 Subject: [PATCH 3/6] test(cli): linearize Deep Agents collision coverage Signed-off-by: Apurv Kumaria --- src/lib/skill-install-shared.test.ts | 91 +++++++++++++++------------- 1 file changed, 49 insertions(+), 42 deletions(-) diff --git a/src/lib/skill-install-shared.test.ts b/src/lib/skill-install-shared.test.ts index 90d99b67456..b6cb407242f 100644 --- a/src/lib/skill-install-shared.test.ts +++ b/src/lib/skill-install-shared.test.ts @@ -40,6 +40,30 @@ function pathsFor(stateDir: string) { return resolveSkillPaths({ name: AGENT_NAME, configPaths: { dir: stateDir } }, "note-summarizer"); } +const COLLISION_CASES = [ + { + kind: "file", + prepare: (destination: string, _outside: string) => writeFileSync(destination, "agent file\n"), + assertUnchanged: (destination: string) => + expect(readFileSync(destination, "utf8")).toBe("agent file\n"), + }, + { + kind: "directory", + prepare: (destination: string, _outside: string) => { + mkdirSync(destination); + writeFileSync(join(destination, "agent.txt"), "agent directory\n"); + }, + assertUnchanged: (destination: string) => + expect(readFileSync(join(destination, "agent.txt"), "utf8")).toBe("agent directory\n"), + }, + { + kind: "symlink", + prepare: (destination: string, outside: string) => symlinkSync(outside, destination), + assertUnchanged: (destination: string) => + expect(readFileSync(destination, "utf8")).toBe("outside\n"), + }, +] as const; + function executeShell( command: string, input: string | Buffer | undefined, @@ -168,49 +192,32 @@ describe("fresh shared-agent skill install", () => { }, ); - it.runIf(process.platform === "linux")( - "refuses existing files, directories, and symlinks without changing them", - () => { - for (const kind of ["file", "directory", "symlink"] as const) { - const skillDir = makeSkill(); - const stateDir = mkdtempSync(join(tmpdir(), `nemoclaw-shared-${kind}-`)); - const paths = pathsFor(stateDir); - mkdirSync(join(stateDir, "agent", "skills"), { recursive: true }); - const outside = join(stateDir, "outside"); - writeFileSync(outside, "outside\n"); - if (kind === "file") { - writeFileSync(paths.uploadDir, "agent file\n"); - } else if (kind === "directory") { - mkdirSync(paths.uploadDir); - writeFileSync(join(paths.uploadDir, "agent.txt"), "agent directory\n"); - } else { - symlinkSync(outside, paths.uploadDir); - } - const before = lstatSync(paths.uploadDir); - try { - const result = installFreshSharedSkill(CTX, skillDir, paths, { - sshExecImpl: (_ctx, command, opts) => executeShell(command, opts?.input), - }); + it.runIf(process.platform === "linux").each(COLLISION_CASES)( + "refuses an existing $kind without changing it (#7634)", + ({ kind, prepare, assertUnchanged }) => { + const skillDir = makeSkill(); + const stateDir = mkdtempSync(join(tmpdir(), `nemoclaw-shared-${kind}-`)); + const paths = pathsFor(stateDir); + mkdirSync(join(stateDir, "agent", "skills"), { recursive: true }); + const outside = join(stateDir, "outside"); + writeFileSync(outside, "outside\n"); + prepare(paths.uploadDir, outside); + const before = lstatSync(paths.uploadDir); + try { + const result = installFreshSharedSkill(CTX, skillDir, paths, { + sshExecImpl: (_ctx, command, opts) => executeShell(command, opts?.input), + }); - expect(result).toEqual({ - success: false, - uploaded: 0, - reason: "destination_exists", - }); - expect(lstatSync(paths.uploadDir).isSymbolicLink()).toBe(before.isSymbolicLink()); - if (kind === "file") { - expect(readFileSync(paths.uploadDir, "utf8")).toBe("agent file\n"); - } else if (kind === "directory") { - expect(readFileSync(join(paths.uploadDir, "agent.txt"), "utf8")).toBe( - "agent directory\n", - ); - } else { - expect(readFileSync(paths.uploadDir, "utf8")).toBe("outside\n"); - } - } finally { - rmSync(skillDir, { recursive: true, force: true }); - rmSync(stateDir, { recursive: true, force: true }); - } + expect(result).toEqual({ + success: false, + uploaded: 0, + reason: "destination_exists", + }); + expect(lstatSync(paths.uploadDir).isSymbolicLink()).toBe(before.isSymbolicLink()); + assertUnchanged(paths.uploadDir); + } finally { + rmSync(skillDir, { recursive: true, force: true }); + rmSync(stateDir, { recursive: true, force: true }); } }, ); From 8c983ff6c8e0ed9d6abcc39b4eb0ecc117a7a817 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Tue, 28 Jul 2026 11:21:25 -0700 Subject: [PATCH 4/6] fix(cli): attest Deep Agents skill file modes Signed-off-by: Apurv Kumaria --- src/lib/actions/sandbox/skill-install.test.ts | 3 +++ src/lib/actions/sandbox/skill-install.ts | 1 + src/lib/skill-install-shared.test.ts | 23 ++++++++++++++++++- src/lib/skill-install.ts | 18 +++++++++++---- 4 files changed, 40 insertions(+), 5 deletions(-) diff --git a/src/lib/actions/sandbox/skill-install.test.ts b/src/lib/actions/sandbox/skill-install.test.ts index 51dc396e748..34f385c2119 100644 --- a/src/lib/actions/sandbox/skill-install.test.ts +++ b/src/lib/actions/sandbox/skill-install.test.ts @@ -310,6 +310,9 @@ describe("sandbox skill action orchestration", () => { expect(skillInstall.uploadDirectory).not.toHaveBeenCalled(); expect(skillInstall.postInstall).not.toHaveBeenCalled(); expect(log).toHaveBeenCalledWith(expect.stringContaining("Skill 'demo-skill' installed")); + expect(log).toHaveBeenCalledWith( + expect.stringContaining("Start a new Deep Agents session to load the skill."), + ); expectTempSshConfigCleanedUp(tempConfig); expect(process.exitCode).toBeUndefined(); }); diff --git a/src/lib/actions/sandbox/skill-install.ts b/src/lib/actions/sandbox/skill-install.ts index 2519c58ac2d..5b17e2d47c0 100644 --- a/src/lib/actions/sandbox/skill-install.ts +++ b/src/lib/actions/sandbox/skill-install.ts @@ -347,6 +347,7 @@ export async function installSandboxSkill( } console.log(` ${G}✓${R} Installed ${fresh.uploaded} file(s) into the agent skill directory`); console.log(` ${G}✓${R} Skill '${frontmatter.name}' installed`); + console.log(` ${D}Start a new Deep Agents session to load the skill.${R}`); return; } diff --git a/src/lib/skill-install-shared.test.ts b/src/lib/skill-install-shared.test.ts index b6cb407242f..b48ee3de018 100644 --- a/src/lib/skill-install-shared.test.ts +++ b/src/lib/skill-install-shared.test.ts @@ -33,6 +33,7 @@ function makeSkill(): string { writeFileSync(join(dir, "SKILL.md"), "---\nname: note-summarizer\n---\n# Notes\n"); mkdirSync(join(dir, "scripts")); writeFileSync(join(dir, "scripts", "summarize.js"), "export default 'exact';\n"); + chmodSync(join(dir, "scripts", "summarize.js"), 0o755); return dir; } @@ -104,7 +105,11 @@ describe("fresh shared-agent skill install", () => { expect(paths.uploadDir).toBe("/sandbox/.deepagents/agent/skills/note-summarizer"); expect(paths.mirrorDir).toBeNull(); expect(paths.uploadDirSharedWithAgent).toBe(true); - expect(commands[0]).toContain("tar --no-same-owner --no-same-permissions -xf -"); + expect(commands[0]).toContain("tar --no-same-owner -xf -"); + expect(commands[0]).not.toContain("--no-same-permissions"); + expect(commands[0]).toContain('find "$payload" -type f -perm /111 -exec chmod 755 {} +'); + expect(commands[0]).toContain('find "$payload" -type f ! -perm /111 -exec chmod 644 {} +'); + expect(commands[0]).toContain('mode="$(stat -c "%a" "$payload/$rel")"'); expect(commands[0]).toContain("sha256sum"); expect(commands[0]).toContain('mv -nT -- "$payload" "$leaf"'); expect(commands[0]).toContain('exists "$payload"'); @@ -117,6 +122,18 @@ describe("fresh shared-agent skill install", () => { } }); + it("includes normalized executable modes in the immutable content digest", () => { + const skillDir = makeSkill(); + try { + const executableDigest = computeSkillContentDigest(skillDir); + chmodSync(join(skillDir, "scripts", "summarize.js"), 0o644); + + expect(computeSkillContentDigest(skillDir)).not.toBe(executableDigest); + } finally { + rmSync(skillDir, { recursive: true, force: true }); + } + }); + it("rejects a snapshot whose SKILL.md name does not match the resolved destination", () => { const skillDir = makeSkill(); const paths = pathsFor("/sandbox/.deepagents"); @@ -184,6 +201,10 @@ describe("fresh shared-agent skill install", () => { expect(readFileSync(join(paths.uploadDir, "scripts", "summarize.js"), "utf8")).toBe( "export default 'exact';\n", ); + expect(lstatSync(join(paths.uploadDir, "SKILL.md")).mode & 0o777).toBe(0o644); + expect(lstatSync(join(paths.uploadDir, "scripts", "summarize.js")).mode & 0o777).toBe( + 0o755, + ); expect(readFileSync(join(legacy, "legacy.txt"), "utf8")).toBe("preserve me\n"); } finally { rmSync(skillDir, { recursive: true, force: true }); diff --git a/src/lib/skill-install.ts b/src/lib/skill-install.ts index 1b929cd2bef..a7128783429 100644 --- a/src/lib/skill-install.ts +++ b/src/lib/skill-install.ts @@ -283,13 +283,20 @@ function fileSha256(filePath: string): string { return createHash("sha256").update(fs.readFileSync(filePath)).digest("hex"); } -/** Hash the sorted regular-file path and byte set used by a skill archive. */ +function normalizedSkillFileMode(filePath: string): "644" | "755" { + return (fs.lstatSync(filePath).mode & 0o111) === 0 ? "644" : "755"; +} + +/** Hash the sorted regular-file path, normalized mode, and byte set used by a skill archive. */ export function computeSkillContentDigest(localDir: string, files?: string[]): string { const selected = files ?? collectFiles(localDir).files; const manifest = selected .slice() .sort() - .map((rel) => `${fileSha256(path.join(localDir, rel))} ${rel}\n`) + .map((rel) => { + const filePath = path.join(localDir, rel); + return `${normalizedSkillFileMode(filePath)} ${fileSha256(filePath)} ${rel}\n`; + }) .join(""); return createHash("sha256").update(manifest).digest("hex"); } @@ -399,11 +406,14 @@ function buildFreshSharedInstallScript(paths: SkillPaths, expectedDigest: string 'cleanup() { if exists "$workspace"; then rm -rf -- "$workspace"; fi; }', "trap cleanup EXIT HUP INT TERM", 'mkdir -- "$payload"', - 'tar --no-same-owner --no-same-permissions -xf - -C "$payload"', + 'tar --no-same-owner -xf - -C "$payload"', '[ -z "$(find "$payload" -mindepth 1 ! -type d ! -type f -print -quit)" ]', + 'find "$payload" -type d -exec chmod 755 {} +', + 'find "$payload" -type f -perm /111 -exec chmod 755 {} +', + 'find "$payload" -type f ! -perm /111 -exec chmod 644 {} +', 'find "$payload" -type f -printf "%P\\n" | LC_ALL=C sort > "$workspace/files"', ': > "$workspace/manifest"', - 'while IFS= read -r rel; do safe_rel "$rel"; hash="$(sha256sum "$payload/$rel" | cut -d " " -f 1)"; printf "%s %s\\n" "$hash" "$rel" >> "$workspace/manifest"; done < "$workspace/files"', + 'while IFS= read -r rel; do safe_rel "$rel"; mode="$(stat -c "%a" "$payload/$rel")"; hash="$(sha256sum "$payload/$rel" | cut -d " " -f 1)"; printf "%s %s %s\\n" "$mode" "$hash" "$rel" >> "$workspace/manifest"; done < "$workspace/files"', 'staged="$(sha256sum "$workspace/manifest" | cut -d " " -f 1)"', '[ "$staged" = "$expected" ]', 'if exists "$leaf"; then echo EXISTS; exit 2; fi', From 7c5ff3c5d62678128986ae078ba9fa99532272f5 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Tue, 28 Jul 2026 11:37:46 -0700 Subject: [PATCH 5/6] test(cli): prefer Deep Agents lifecycle outcomes Signed-off-by: Apurv Kumaria --- src/lib/skill-install-shared.test.ts | 16 +--------------- 1 file changed, 1 insertion(+), 15 deletions(-) diff --git a/src/lib/skill-install-shared.test.ts b/src/lib/skill-install-shared.test.ts index b48ee3de018..57a383789b3 100644 --- a/src/lib/skill-install-shared.test.ts +++ b/src/lib/skill-install-shared.test.ts @@ -87,11 +87,9 @@ describe("fresh shared-agent skill install", () => { const skillDir = makeSkill(); const paths = pathsFor("/sandbox/.deepagents"); const expected = computeSkillContentDigest(skillDir); - const commands: string[] = []; try { const result = installFreshSharedSkill(CTX, skillDir, paths, { - sshExecImpl: (_ctx, command, opts) => { - commands.push(command); + sshExecImpl: (_ctx, _command, opts) => { expect(Buffer.isBuffer(opts?.input)).toBe(true); return { status: 0, stdout: `INSTALLED ${expected}`, stderr: "" }; }, @@ -105,18 +103,6 @@ describe("fresh shared-agent skill install", () => { expect(paths.uploadDir).toBe("/sandbox/.deepagents/agent/skills/note-summarizer"); expect(paths.mirrorDir).toBeNull(); expect(paths.uploadDirSharedWithAgent).toBe(true); - expect(commands[0]).toContain("tar --no-same-owner -xf -"); - expect(commands[0]).not.toContain("--no-same-permissions"); - expect(commands[0]).toContain('find "$payload" -type f -perm /111 -exec chmod 755 {} +'); - expect(commands[0]).toContain('find "$payload" -type f ! -perm /111 -exec chmod 644 {} +'); - expect(commands[0]).toContain('mode="$(stat -c "%a" "$payload/$rel")"'); - expect(commands[0]).toContain("sha256sum"); - expect(commands[0]).toContain('mv -nT -- "$payload" "$leaf"'); - expect(commands[0]).toContain('exists "$payload"'); - expect(commands[0]).toContain("MOVE_FAILED"); - expect(commands[0]).not.toContain("$workspace/active.manifest"); - expect(commands[0]).not.toContain("/sandbox/.deepagents/skills/note-summarizer"); - expect(commands[0]).not.toContain('rm -rf -- "$leaf"'); } finally { rmSync(skillDir, { recursive: true, force: true }); } From 3b8ed36a8ead4ddf44cfb991bd802e7722dd9665 Mon Sep 17 00:00:00 2001 From: Apurv Kumaria Date: Tue, 28 Jul 2026 12:14:16 -0700 Subject: [PATCH 6/6] fix(cli): read skill metadata without path races Signed-off-by: Apurv Kumaria --- src/lib/actions/sandbox/skill-install.test.ts | 57 +++++++++++++++++++ src/lib/actions/sandbox/skill-install.ts | 53 ++++++++++++++--- 2 files changed, 102 insertions(+), 8 deletions(-) diff --git a/src/lib/actions/sandbox/skill-install.test.ts b/src/lib/actions/sandbox/skill-install.test.ts index 34f385c2119..ec8d15cc846 100644 --- a/src/lib/actions/sandbox/skill-install.test.ts +++ b/src/lib/actions/sandbox/skill-install.test.ts @@ -248,6 +248,63 @@ describe("sandbox skill action orchestration", () => { expect(skillInstall.uploadDirectory).not.toHaveBeenCalled(); }); + it("refuses a SKILL.md symlink before parsing or contacting the sandbox", async () => { + const skillDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-action-skill-link-")); + const target = path.join(skillDir, "target.md"); + fs.writeFileSync(target, "---\nname: demo-skill\n---\n# Demo\n"); + fs.symlinkSync(target, path.join(skillDir, "SKILL.md")); + const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + const exit = vi.spyOn(process, "exit").mockImplementation(((code?: string | number | null) => { + throw new Error(`process.exit ${code}`); + }) as typeof process.exit); + + try { + await expect( + installSandboxSkill("alpha", { command: "install", path: skillDir }), + ).rejects.toThrow("process.exit 1"); + } finally { + fs.rmSync(skillDir, { recursive: true, force: true }); + } + + expect(error).toHaveBeenCalledWith(expect.stringContaining("must be a regular file")); + expect(skillInstall.parseFrontmatter).not.toHaveBeenCalled(); + expect(captureSandboxSshConfig).not.toHaveBeenCalled(); + expect(exit).toHaveBeenCalledWith(1); + }); + + it("fails closed when SKILL.md is replaced between path validation and descriptor open", async () => { + const skillDir = makeSkillDir(); + const skillMdPath = path.join(skillDir, "SKILL.md"); + const replacement = path.join(skillDir, "replacement.md"); + fs.writeFileSync(replacement, "---\nname: attacker\n---\n# Replacement\n"); + let openedFlags = 0; + vi.spyOn(fs, "openSync").mockImplementationOnce((_candidatePath, flags) => { + openedFlags = flags as number; + fs.rmSync(skillMdPath); + fs.symlinkSync(replacement, skillMdPath); + throw Object.assign(new Error("symbolic link refused"), { code: "ELOOP" }); + }); + const error = vi.spyOn(console, "error").mockImplementation(() => undefined); + const exit = vi.spyOn(process, "exit").mockImplementation(((code?: string | number | null) => { + throw new Error(`process.exit ${code}`); + }) as typeof process.exit); + + try { + await expect( + installSandboxSkill("alpha", { command: "install", path: skillDir }), + ).rejects.toThrow("process.exit 1"); + } finally { + fs.rmSync(skillDir, { recursive: true, force: true }); + } + + expect(openedFlags & fs.constants.O_NOFOLLOW).toBe(fs.constants.O_NOFOLLOW); + expect(openedFlags & fs.constants.O_NONBLOCK).toBe(fs.constants.O_NONBLOCK); + expect(error).toHaveBeenCalledWith(expect.stringContaining("must be a regular file")); + expect(skillInstall.parseFrontmatter).not.toHaveBeenCalled(); + expect(captureSandboxSshConfig).not.toHaveBeenCalled(); + expect(exit).toHaveBeenCalledWith(1); + }); + it("continues skill install when the existence probe is unknown because upload plus verify are authoritative", async () => { const skillDir = makeSkillDir(); let tempConfig = ""; diff --git a/src/lib/actions/sandbox/skill-install.ts b/src/lib/actions/sandbox/skill-install.ts index 5b17e2d47c0..9968254d43c 100644 --- a/src/lib/actions/sandbox/skill-install.ts +++ b/src/lib/actions/sandbox/skill-install.ts @@ -72,6 +72,43 @@ export type SkillRemoveRequest = { extraArgs?: string[]; }; +function lstatOrNull(candidatePath: string): fs.Stats | null { + try { + return fs.lstatSync(candidatePath); + } catch { + return null; + } +} + +type RegularFileRead = + | { content: string; success: true } + | { reason: "invalid" | "missing"; success: false }; + +function readRegularFileNoFollow(candidatePath: string): RegularFileRead { + const noFollow = fs.constants.O_NOFOLLOW; + const nonblock = fs.constants.O_NONBLOCK; + if (typeof noFollow !== "number" || typeof nonblock !== "number") { + return { reason: "invalid", success: false }; + } + + let descriptor: number | undefined; + try { + descriptor = fs.openSync(candidatePath, fs.constants.O_RDONLY | noFollow | nonblock); + if (!fs.fstatSync(descriptor).isFile()) return { reason: "invalid", success: false }; + return { content: fs.readFileSync(descriptor, "utf8"), success: true }; + } catch (error) { + return { + reason: + error instanceof Error && "code" in error && error.code === "ENOENT" + ? "missing" + : "invalid", + success: false, + }; + } finally { + if (descriptor !== undefined) fs.closeSync(descriptor); + } +} + export function printPluginInstallHint(): void { console.error(" This looks like an OpenClaw plugin, not a SKILL.md agent skill."); console.error(" `skill install` only accepts skill directories or direct SKILL.md paths."); @@ -231,7 +268,8 @@ export async function installSandboxSkill( } const resolvedPath = path.resolve(skillPath); - if (fs.existsSync(resolvedPath) && fs.lstatSync(resolvedPath).isSymbolicLink()) { + const resolvedStat = lstatOrNull(resolvedPath); + if (resolvedStat?.isSymbolicLink()) { console.error(` Skill path '${resolvedPath}' must not be a symbolic link.`); process.exit(1); } @@ -239,10 +277,10 @@ export async function installSandboxSkill( // Accept a directory containing SKILL.md, or a direct path to SKILL.md. let skillDir: string; let skillMdPath: string; - if (fs.existsSync(resolvedPath) && fs.statSync(resolvedPath).isDirectory()) { + if (resolvedStat?.isDirectory()) { skillDir = resolvedPath; skillMdPath = path.join(resolvedPath, "SKILL.md"); - } else if (fs.existsSync(resolvedPath) && resolvedPath.endsWith("SKILL.md")) { + } else if (resolvedStat?.isFile() && resolvedPath.endsWith("SKILL.md")) { skillDir = path.dirname(resolvedPath); skillMdPath = resolvedPath; } else { @@ -254,7 +292,8 @@ export async function installSandboxSkill( process.exit(1); } - if (!fs.existsSync(skillMdPath)) { + const skillMdRead = readRegularFileNoFollow(skillMdPath); + if (!skillMdRead.success && skillMdRead.reason === "missing") { console.error(` No SKILL.md found in '${skillDir}'.`); console.error(" The skill directory must contain a SKILL.md file."); if (looksLikeOpenClawPlugin(skillDir)) { @@ -262,8 +301,7 @@ export async function installSandboxSkill( } process.exit(1); } - const skillMdStat = fs.lstatSync(skillMdPath); - if (!skillMdStat.isFile() || skillMdStat.isSymbolicLink()) { + if (!skillMdRead.success) { console.error(` SKILL.md at '${skillMdPath}' must be a regular file, not a symbolic link.`); process.exit(1); } @@ -271,8 +309,7 @@ export async function installSandboxSkill( // 1. Validate frontmatter let frontmatter; try { - const content = fs.readFileSync(skillMdPath, "utf-8"); - frontmatter = skillInstall.parseFrontmatter(content); + frontmatter = skillInstall.parseFrontmatter(skillMdRead.content); } catch (err) { const errorMessage = err instanceof Error ? err.message : String(err); console.error(` ${errorMessage}`);