mirror of
https://github.com/openclaw/openclaw.git
synced 2026-04-18 15:47:27 +00:00
feat(skills): add cross-platform install fallback for non-brew environments (#17687)
Merged via /review-pr -> /prepare-pr -> /merge-pr.
Prepared head SHA: 3ed4850838
Co-authored-by: mcrolly <60803337+mcrolly@users.noreply.github.com>
Co-authored-by: sebslight <19554889+sebslight@users.noreply.github.com>
Reviewed-by: @sebslight
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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");
|
||||
|
||||
193
src/agents/skills-install-fallback.test.ts
Normal file
193
src/agents/skills-install-fallback.test.ts
Normal file
@@ -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<typeof import("../security/skill-scanner.js")>();
|
||||
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<string, string>,
|
||||
): Promise<string> {
|
||||
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();
|
||||
});
|
||||
});
|
||||
@@ -275,10 +275,15 @@ export async function installSkill(params: SkillInstallRequest): Promise<SkillIn
|
||||
|
||||
const brewExe = hasBinary("brew") ? "brew" : resolveBrewExecutable();
|
||||
if (spec.kind === "brew" && !brewExe) {
|
||||
const formula = spec.formula ?? "this package";
|
||||
const hint =
|
||||
process.platform === "linux"
|
||||
? `Homebrew is not installed. Install it from https://brew.sh or install "${formula}" manually using your system package manager (e.g. apt, dnf, pacman).`
|
||||
: "Homebrew is not installed. Install it from https://brew.sh";
|
||||
return withWarnings(
|
||||
{
|
||||
ok: false,
|
||||
message: "brew not installed",
|
||||
message: `brew not installed — ${hint}`,
|
||||
stdout: "",
|
||||
stderr: "",
|
||||
code: null,
|
||||
@@ -307,7 +312,8 @@ export async function installSkill(params: SkillInstallRequest): Promise<SkillIn
|
||||
return withWarnings(
|
||||
{
|
||||
ok: false,
|
||||
message: "uv not installed (install via brew)",
|
||||
message:
|
||||
"uv not installed — install manually: https://docs.astral.sh/uv/getting-started/installation/",
|
||||
stdout: "",
|
||||
stderr: "",
|
||||
code: null,
|
||||
@@ -350,11 +356,145 @@ export async function installSkill(params: SkillInstallRequest): Promise<SkillIn
|
||||
warnings,
|
||||
);
|
||||
}
|
||||
} else if (hasBinary("apt-get")) {
|
||||
const aptInstallArgv = ["apt-get", "install", "-y", "golang-go"];
|
||||
const aptUpdateArgv = ["apt-get", "update", "-qq"];
|
||||
const isRoot = typeof process.getuid === "function" && process.getuid() === 0;
|
||||
|
||||
if (isRoot) {
|
||||
try {
|
||||
// Best effort: fresh containers often need package indexes populated.
|
||||
await runCommandWithTimeout(aptUpdateArgv, { timeoutMs });
|
||||
} catch {
|
||||
// ignore and continue; install command will return actionable stderr on failure
|
||||
}
|
||||
|
||||
let aptResult;
|
||||
try {
|
||||
aptResult = await runCommandWithTimeout(aptInstallArgv, { timeoutMs });
|
||||
} catch (err) {
|
||||
const stderr = err instanceof Error ? err.message : String(err);
|
||||
return withWarnings(
|
||||
{
|
||||
ok: false,
|
||||
message:
|
||||
"go not installed — automatic install via apt failed. Install manually: https://go.dev/doc/install",
|
||||
stdout: "",
|
||||
stderr,
|
||||
code: null,
|
||||
},
|
||||
warnings,
|
||||
);
|
||||
}
|
||||
if (aptResult.code !== 0) {
|
||||
return withWarnings(
|
||||
{
|
||||
ok: false,
|
||||
message:
|
||||
"go not installed — automatic install via apt failed. Install manually: https://go.dev/doc/install",
|
||||
stdout: aptResult.stdout.trim(),
|
||||
stderr: aptResult.stderr.trim(),
|
||||
code: aptResult.code,
|
||||
},
|
||||
warnings,
|
||||
);
|
||||
}
|
||||
} else {
|
||||
// Check for non-interactive sudo before attempting apt — avoids hanging
|
||||
// in containers or environments where sudo is missing or requires a password.
|
||||
if (!hasBinary("sudo")) {
|
||||
return withWarnings(
|
||||
{
|
||||
ok: false,
|
||||
message:
|
||||
"go not installed — apt-get is available but sudo is not installed. Install manually: https://go.dev/doc/install",
|
||||
stdout: "",
|
||||
stderr: "",
|
||||
code: null,
|
||||
},
|
||||
warnings,
|
||||
);
|
||||
}
|
||||
|
||||
let sudoCheck;
|
||||
try {
|
||||
sudoCheck = await runCommandWithTimeout(["sudo", "-n", "true"], {
|
||||
timeoutMs: 5_000,
|
||||
});
|
||||
} catch (err) {
|
||||
const stderr = err instanceof Error ? err.message : String(err);
|
||||
return withWarnings(
|
||||
{
|
||||
ok: false,
|
||||
message:
|
||||
"go not installed — apt-get is available but sudo is not usable (missing or requires a password). Install manually: https://go.dev/doc/install",
|
||||
stdout: "",
|
||||
stderr,
|
||||
code: null,
|
||||
},
|
||||
warnings,
|
||||
);
|
||||
}
|
||||
if (sudoCheck.code !== 0) {
|
||||
return withWarnings(
|
||||
{
|
||||
ok: false,
|
||||
message:
|
||||
"go not installed — apt-get is available but sudo is not usable (missing or requires a password). Install manually: https://go.dev/doc/install",
|
||||
stdout: sudoCheck.stdout.trim(),
|
||||
stderr: sudoCheck.stderr.trim(),
|
||||
code: sudoCheck.code,
|
||||
},
|
||||
warnings,
|
||||
);
|
||||
}
|
||||
|
||||
try {
|
||||
// Best effort: fresh containers often need package indexes populated.
|
||||
await runCommandWithTimeout(["sudo", ...aptUpdateArgv], { timeoutMs });
|
||||
} catch {
|
||||
// ignore and continue; install command will return actionable stderr on failure
|
||||
}
|
||||
|
||||
let aptResult;
|
||||
try {
|
||||
aptResult = await runCommandWithTimeout(["sudo", ...aptInstallArgv], {
|
||||
timeoutMs,
|
||||
});
|
||||
} catch (err) {
|
||||
const stderr = err instanceof Error ? err.message : String(err);
|
||||
return withWarnings(
|
||||
{
|
||||
ok: false,
|
||||
message:
|
||||
"go not installed — automatic install via apt failed. Install manually: https://go.dev/doc/install",
|
||||
stdout: "",
|
||||
stderr,
|
||||
code: null,
|
||||
},
|
||||
warnings,
|
||||
);
|
||||
}
|
||||
|
||||
if (aptResult.code !== 0) {
|
||||
return withWarnings(
|
||||
{
|
||||
ok: false,
|
||||
message:
|
||||
"go not installed — automatic install via apt failed. Install manually: https://go.dev/doc/install",
|
||||
stdout: aptResult.stdout.trim(),
|
||||
stderr: aptResult.stderr.trim(),
|
||||
code: aptResult.code,
|
||||
},
|
||||
warnings,
|
||||
);
|
||||
}
|
||||
}
|
||||
} else {
|
||||
return withWarnings(
|
||||
{
|
||||
ok: false,
|
||||
message: "go not installed (install via brew)",
|
||||
message: "go not installed — install manually: https://go.dev/doc/install",
|
||||
stdout: "",
|
||||
stderr: "",
|
||||
code: null,
|
||||
|
||||
@@ -74,7 +74,9 @@ function selectPreferredInstallSpec(
|
||||
const goSpec = findKind("go");
|
||||
const uvSpec = findKind("uv");
|
||||
|
||||
if (prefs.preferBrew && hasBinary("brew") && brewSpec) {
|
||||
const brewAvailable = hasBinary("brew");
|
||||
|
||||
if (prefs.preferBrew && brewAvailable && brewSpec) {
|
||||
return brewSpec;
|
||||
}
|
||||
if (uvSpec) {
|
||||
@@ -83,12 +85,25 @@ function selectPreferredInstallSpec(
|
||||
if (nodeSpec) {
|
||||
return nodeSpec;
|
||||
}
|
||||
if (brewSpec) {
|
||||
// Only prefer brew when it is actually installed; otherwise skip to
|
||||
// alternatives so Linux/Docker environments without brew get a working
|
||||
// install option instead of a guaranteed failure.
|
||||
if (brewSpec && brewAvailable) {
|
||||
return brewSpec;
|
||||
}
|
||||
if (goSpec) {
|
||||
return goSpec;
|
||||
}
|
||||
// Prefer download over an unavailable brew spec.
|
||||
const downloadSpec = findKind("download");
|
||||
if (downloadSpec) {
|
||||
return downloadSpec;
|
||||
}
|
||||
// Last resort: return brew spec even without brew so the caller can
|
||||
// surface a descriptive error rather than "no installer found".
|
||||
if (brewSpec) {
|
||||
return brewSpec;
|
||||
}
|
||||
return indexed[0];
|
||||
}
|
||||
|
||||
|
||||
@@ -967,11 +967,10 @@ describe("runReplyAgent fallback reasoning tags", () => {
|
||||
},
|
||||
});
|
||||
|
||||
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);
|
||||
|
||||
@@ -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<string, unknown>) {
|
||||
// ---------- 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: "<available_skills></available_skills>",
|
||||
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"]);
|
||||
|
||||
|
||||
@@ -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) {
|
||||
|
||||
Reference in New Issue
Block a user