From d19b746928e99507d6192340210cb0360e308abf Mon Sep 17 00:00:00 2001 From: McRolly NWANGWU <60803337+mcrolly@users.noreply.github.com> Date: Sun, 15 Feb 2026 21:25:26 -0600 Subject: [PATCH] feat(skills): add cross-platform install fallback for non-brew environments (#17687) Merged via /review-pr -> /prepare-pr -> /merge-pr. Prepared head SHA: 3ed4850838578b90140cc11c6fd23be6953c87ea Co-authored-by: mcrolly <60803337+mcrolly@users.noreply.github.com> Co-authored-by: sebslight <19554889+sebslight@users.noreply.github.com> Reviewed-by: @sebslight --- CHANGELOG.md | 1 + src/agents/sanitize-for-prompt.test.ts | 4 +- src/agents/skills-install-fallback.test.ts | 193 ++++++++++++++++++ src/agents/skills-install.ts | 146 ++++++++++++- src/agents/skills-status.ts | 19 +- .../agent-runner.misc.runreplyagent.test.ts | 9 +- .../isolated-agent/run.skill-filter.test.ts | 14 +- src/pairing/pairing-store.ts | 15 +- 8 files changed, 384 insertions(+), 17 deletions(-) create mode 100644 src/agents/skills-install-fallback.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 608dde5caf5..9ab48281beb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ Docs: https://docs.openclaw.ai - Gateway/Control UI: preserve requested operator scopes for Control UI bypass modes (`allowInsecureAuth` / `dangerouslyDisableDeviceAuth`) when device identity is unavailable, preventing false `missing scope` failures on authenticated LAN/HTTP operator sessions. (#17682) Thanks @leafbird. - Gateway/Security: redact sensitive session/path details from `status` responses for non-admin clients; full details remain available to `operator.admin`. (#8590) Thanks @fr33d3m0n. - Skills/Security: restrict `download` installer `targetDir` to the per-skill tools directory to prevent arbitrary file writes. Thanks @Adam55A-code. +- Skills/Linux: harden go installer fallback on apt-based systems by handling root/no-sudo environments safely, doing best-effort apt index refresh, and returning actionable errors instead of failing with spawn errors. (#17687) Thanks @mcrolly. - Web Fetch/Security: cap downloaded response body size before HTML parsing to prevent memory exhaustion from oversized or deeply nested pages. Thanks @xuemian168. - Config/Gateway: make sensitive-key whitelist suffix matching case-insensitive while preserving `passwordFile` path exemptions, preventing accidental redaction of non-secret config values like `maxTokens` and IRC password-file paths. (#16042) Thanks @akramcodez. - Dev tooling: harden git `pre-commit` hook against option injection from malicious filenames (for example `--force`), preventing accidental staging of ignored files. Thanks @mrthankyou. diff --git a/src/agents/sanitize-for-prompt.test.ts b/src/agents/sanitize-for-prompt.test.ts index b79a1250bab..32a4ce3d86e 100644 --- a/src/agents/sanitize-for-prompt.test.ts +++ b/src/agents/sanitize-for-prompt.test.ts @@ -45,7 +45,9 @@ describe("buildAgentSystemPrompt uses sanitized workspace/sandbox strings", () = }, }); expect(prompt).toContain("Sandbox container workdir: /workspace"); - expect(prompt).toContain("Sandbox host workspace: /hostspace"); + expect(prompt).toContain( + "Sandbox host mount source (file tools bridge only; not valid inside sandbox exec): /hostspace", + ); expect(prompt).toContain("(mounted at /mntmount)"); expect(prompt).toContain("Sandbox browser observer (noVNC): http://example.test/ui"); expect(prompt).not.toContain("\nui"); diff --git a/src/agents/skills-install-fallback.test.ts b/src/agents/skills-install-fallback.test.ts new file mode 100644 index 00000000000..fedbb77b078 --- /dev/null +++ b/src/agents/skills-install-fallback.test.ts @@ -0,0 +1,193 @@ +import fs from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { installSkill } from "./skills-install.js"; + +const runCommandWithTimeoutMock = vi.fn(); +const scanDirectoryWithSummaryMock = vi.fn(); +const hasBinaryMock = vi.fn(); + +vi.mock("../process/exec.js", () => ({ + runCommandWithTimeout: (...args: unknown[]) => runCommandWithTimeoutMock(...args), +})); + +vi.mock("../infra/net/fetch-guard.js", () => ({ + fetchWithSsrFGuard: vi.fn(), +})); + +vi.mock("../security/skill-scanner.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + scanDirectoryWithSummary: (...args: unknown[]) => scanDirectoryWithSummaryMock(...args), + }; +}); + +vi.mock("../shared/config-eval.js", () => ({ + hasBinary: (...args: unknown[]) => hasBinaryMock(...args), +})); + +vi.mock("../infra/brew.js", () => ({ + resolveBrewExecutable: () => undefined, +})); + +async function writeSkillWithInstaller( + workspaceDir: string, + name: string, + kind: string, + extra: Record, +): Promise { + const skillDir = path.join(workspaceDir, "skills", name); + await fs.mkdir(skillDir, { recursive: true }); + const installSpec = { id: "deps", kind, ...extra }; + await fs.writeFile( + path.join(skillDir, "SKILL.md"), + `--- +name: ${name} +description: test skill +metadata: ${JSON.stringify({ openclaw: { install: [installSpec] } })} +--- + +# ${name} +`, + "utf-8", + ); + await fs.writeFile(path.join(skillDir, "runner.js"), "export {};\n", "utf-8"); + return skillDir; +} + +describe("skills-install fallback edge cases", () => { + let workspaceDir: string; + + beforeEach(async () => { + vi.clearAllMocks(); + workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-fallback-test-")); + scanDirectoryWithSummaryMock.mockResolvedValue({ critical: 0, warn: 0, findings: [] }); + }); + + afterEach(async () => { + await fs.rm(workspaceDir, { recursive: true, force: true }).catch(() => undefined); + }); + + it("apt-get available but sudo missing/unusable returns helpful error for go install", async () => { + await writeSkillWithInstaller(workspaceDir, "go-tool", "go", { + module: "example.com/tool@latest", + }); + + // go not available, brew not available, apt-get + sudo are available, sudo check fails + hasBinaryMock.mockImplementation((bin: string) => { + if (bin === "go") { + return false; + } + if (bin === "brew") { + return false; + } + if (bin === "apt-get" || bin === "sudo") { + return true; + } + return false; + }); + + // sudo -n true fails (no passwordless sudo) + runCommandWithTimeoutMock.mockResolvedValueOnce({ + code: 1, + stdout: "", + stderr: "sudo: a password is required", + }); + + const result = await installSkill({ + workspaceDir, + skillName: "go-tool", + installId: "deps", + }); + + expect(result.ok).toBe(false); + expect(result.message).toContain("sudo"); + expect(result.message).toContain("https://go.dev/doc/install"); + + // Verify sudo -n true was called + expect(runCommandWithTimeoutMock).toHaveBeenCalledWith( + ["sudo", "-n", "true"], + expect.objectContaining({ timeoutMs: 5_000 }), + ); + + // Verify apt-get install was NOT called + const aptCalls = runCommandWithTimeoutMock.mock.calls.filter( + (call) => Array.isArray(call[0]) && (call[0] as string[]).includes("apt-get"), + ); + expect(aptCalls).toHaveLength(0); + }); + + it("handles sudo probe spawn failures without throwing", async () => { + await writeSkillWithInstaller(workspaceDir, "go-tool", "go", { + module: "example.com/tool@latest", + }); + + // go not available, brew not available, apt-get + sudo appear available + hasBinaryMock.mockImplementation((bin: string) => { + if (bin === "go") { + return false; + } + if (bin === "brew") { + return false; + } + if (bin === "apt-get" || bin === "sudo") { + return true; + } + return false; + }); + + runCommandWithTimeoutMock.mockRejectedValueOnce( + new Error('Executable not found in $PATH: "sudo"'), + ); + + const result = await installSkill({ + workspaceDir, + skillName: "go-tool", + installId: "deps", + }); + + expect(result.ok).toBe(false); + expect(result.message).toContain("sudo is not usable"); + expect(result.stderr).toContain("Executable not found"); + + // Verify apt-get install was NOT called + const aptCalls = runCommandWithTimeoutMock.mock.calls.filter( + (call) => Array.isArray(call[0]) && (call[0] as string[]).includes("apt-get"), + ); + expect(aptCalls).toHaveLength(0); + }); + + it("uv not installed and no brew returns helpful error without curl auto-install", async () => { + await writeSkillWithInstaller(workspaceDir, "py-tool", "uv", { + package: "example-package", + }); + + // uv not available, brew not available, curl IS available + hasBinaryMock.mockImplementation((bin: string) => { + if (bin === "uv") { + return false; + } + if (bin === "brew") { + return false; + } + if (bin === "curl") { + return true; + } + return false; + }); + + const result = await installSkill({ + workspaceDir, + skillName: "py-tool", + installId: "deps", + }); + + expect(result.ok).toBe(false); + expect(result.message).toContain("https://docs.astral.sh/uv/getting-started/installation/"); + + // Verify NO curl command was attempted (no auto-install) + expect(runCommandWithTimeoutMock).not.toHaveBeenCalled(); + }); +}); diff --git a/src/agents/skills-install.ts b/src/agents/skills-install.ts index 79b541c09d0..75028a9a8cc 100644 --- a/src/agents/skills-install.ts +++ b/src/agents/skills-install.ts @@ -275,10 +275,15 @@ export async function installSkill(params: SkillInstallRequest): Promise { }, }); - const flushCall = runEmbeddedPiAgentMock.mock.calls.find( - ([params]) => - (params as EmbeddedPiAgentParams | undefined)?.prompt?.includes( - "Pre-compaction memory flush.", - ), + const flushCall = runEmbeddedPiAgentMock.mock.calls.find(([params]) => + (params as EmbeddedPiAgentParams | undefined)?.prompt?.includes( + "Pre-compaction memory flush.", + ), )?.[0] as EmbeddedPiAgentParams | undefined; expect(flushCall?.enforceFinalTag).toBe(true); diff --git a/src/cron/isolated-agent/run.skill-filter.test.ts b/src/cron/isolated-agent/run.skill-filter.test.ts index 58444cab38d..d4d776b7116 100644 --- a/src/cron/isolated-agent/run.skill-filter.test.ts +++ b/src/cron/isolated-agent/run.skill-filter.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; // ---------- mocks ---------- @@ -193,8 +193,12 @@ function makeParams(overrides?: Record) { // ---------- tests ---------- describe("runCronIsolatedAgentTurn — skill filter", () => { + let previousFastTestEnv: string | undefined; + beforeEach(() => { vi.clearAllMocks(); + previousFastTestEnv = process.env.OPENCLAW_TEST_FAST; + delete process.env.OPENCLAW_TEST_FAST; buildWorkspaceSkillSnapshotMock.mockReturnValue({ prompt: "", resolvedSkills: [], @@ -216,6 +220,14 @@ describe("runCronIsolatedAgentTurn — skill filter", () => { }); }); + afterEach(() => { + if (previousFastTestEnv == null) { + delete process.env.OPENCLAW_TEST_FAST; + return; + } + process.env.OPENCLAW_TEST_FAST = previousFastTestEnv; + }); + it("passes agent-level skillFilter to buildWorkspaceSkillSnapshot", async () => { resolveAgentSkillsFilterMock.mockReturnValue(["meme-factory", "weather"]); diff --git a/src/pairing/pairing-store.ts b/src/pairing/pairing-store.ts index d9ae23f3971..86f2cbe85c5 100644 --- a/src/pairing/pairing-store.ts +++ b/src/pairing/pairing-store.ts @@ -373,7 +373,10 @@ export async function listChannelPairingRequests( const normalizedAccountId = accountId?.trim().toLowerCase() || ""; const filtered = normalizedAccountId ? pruned.filter( - (entry) => String(entry.meta?.accountId ?? "").trim().toLowerCase() === normalizedAccountId, + (entry) => + String(entry.meta?.accountId ?? "") + .trim() + .toLowerCase() === normalizedAccountId, ) : pruned; return filtered @@ -421,9 +424,7 @@ export async function upsertChannelPairingRequest(params: { .filter(([_, v]) => Boolean(v)), ) : undefined; - const meta = normalizedAccountId - ? { ...baseMeta, accountId: normalizedAccountId } - : baseMeta; + const meta = normalizedAccountId ? { ...baseMeta, accountId: normalizedAccountId } : baseMeta; let reqs = Array.isArray(value.requests) ? value.requests : []; const { requests: prunedExpired, removed: expiredRemoved } = pruneExpiredRequests( @@ -524,7 +525,11 @@ export async function approveChannelPairingCode(params: { if (!normalizedAccountId) { return true; } - return String(r.meta?.accountId ?? "").trim().toLowerCase() === normalizedAccountId; + return ( + String(r.meta?.accountId ?? "") + .trim() + .toLowerCase() === normalizedAccountId + ); }); if (idx < 0) { if (removed) {