From bc0160d0f2e0f3a38636b8e5f6888ba828740fa0 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sat, 14 Feb 2026 13:44:31 +0000 Subject: [PATCH] refactor(shared): dedupe requirements evaluation --- src/agents/skills-status.ts | 73 +++++++++++++------------- src/hooks/hooks-status.ts | 69 +++++++++++-------------- src/shared/requirements.test.ts | 63 +++++++++++++++++++++++ src/shared/requirements.ts | 90 +++++++++++++++++++++++++++++++++ 4 files changed, 216 insertions(+), 79 deletions(-) create mode 100644 src/shared/requirements.test.ts create mode 100644 src/shared/requirements.ts diff --git a/src/agents/skills-status.ts b/src/agents/skills-status.ts index 4bb666636b8..e1e5d29625d 100644 --- a/src/agents/skills-status.ts +++ b/src/agents/skills-status.ts @@ -1,5 +1,12 @@ import path from "node:path"; import type { OpenClawConfig } from "../config/config.js"; +import { + buildConfigChecks, + resolveMissingAnyBins, + resolveMissingBins, + resolveMissingEnv, + resolveMissingOs, +} from "../shared/requirements.js"; import { CONFIG_DIR } from "../utils.js"; import { hasBinary, @@ -202,48 +209,36 @@ function buildSkillStatus( const requiredConfig = entry.metadata?.requires?.config ?? []; const requiredOs = entry.metadata?.os ?? []; - const missingBins = requiredBins.filter((bin) => { - if (hasBinary(bin)) { - return false; - } - if (eligibility?.remote?.hasBin?.(bin)) { - return false; - } - return true; + const missingBins = resolveMissingBins({ + required: requiredBins, + hasLocalBin: hasBinary, + hasRemoteBin: eligibility?.remote?.hasBin, + }); + const missingAnyBins = resolveMissingAnyBins({ + required: requiredAnyBins, + hasLocalBin: hasBinary, + hasRemoteAnyBin: eligibility?.remote?.hasAnyBin, + }); + const missingOs = resolveMissingOs({ + required: requiredOs, + localPlatform: process.platform, + remotePlatforms: eligibility?.remote?.platforms, }); - const missingAnyBins = - requiredAnyBins.length > 0 && - !( - requiredAnyBins.some((bin) => hasBinary(bin)) || - eligibility?.remote?.hasAnyBin?.(requiredAnyBins) - ) - ? requiredAnyBins - : []; - const missingOs = - requiredOs.length > 0 && - !requiredOs.includes(process.platform) && - !eligibility?.remote?.platforms?.some((platform) => requiredOs.includes(platform)) - ? requiredOs - : []; - const missingEnv: string[] = []; - for (const envName of requiredEnv) { - if (process.env[envName]) { - continue; - } - if (skillConfig?.env?.[envName]) { - continue; - } - if (skillConfig?.apiKey && entry.metadata?.primaryEnv === envName) { - continue; - } - missingEnv.push(envName); - } + const missingEnv = resolveMissingEnv({ + required: requiredEnv, + isSatisfied: (envName) => + Boolean( + process.env[envName] || + skillConfig?.env?.[envName] || + (skillConfig?.apiKey && entry.metadata?.primaryEnv === envName), + ), + }); - const configChecks: SkillStatusConfigCheck[] = requiredConfig.map((pathStr) => { - const value = resolveConfigPath(config, pathStr); - const satisfied = isConfigPathTruthy(config, pathStr); - return { path: pathStr, value, satisfied }; + const configChecks: SkillStatusConfigCheck[] = buildConfigChecks({ + required: requiredConfig, + resolveValue: (pathStr) => resolveConfigPath(config, pathStr), + isSatisfied: (pathStr) => isConfigPathTruthy(config, pathStr), }); const missingConfig = configChecks.filter((check) => !check.satisfied).map((check) => check.path); diff --git a/src/hooks/hooks-status.ts b/src/hooks/hooks-status.ts index 0a8018e11d5..95d06b26862 100644 --- a/src/hooks/hooks-status.ts +++ b/src/hooks/hooks-status.ts @@ -1,6 +1,13 @@ import path from "node:path"; import type { OpenClawConfig } from "../config/config.js"; import type { HookEligibilityContext, HookEntry, HookInstallSpec } from "./types.js"; +import { + buildConfigChecks, + resolveMissingAnyBins, + resolveMissingBins, + resolveMissingEnv, + resolveMissingOs, +} from "../shared/requirements.js"; import { CONFIG_DIR } from "../utils.js"; import { hasBinary, isConfigPathTruthy, resolveConfigPath, resolveHookConfig } from "./config.js"; import { loadWorkspaceHookEntries } from "./workspace.js"; @@ -115,49 +122,31 @@ function buildHookStatus( const requiredConfig = entry.metadata?.requires?.config ?? []; const requiredOs = entry.metadata?.os ?? []; - const missingBins = requiredBins.filter((bin) => { - if (hasBinary(bin)) { - return false; - } - if (eligibility?.remote?.hasBin?.(bin)) { - return false; - } - return true; + const missingBins = resolveMissingBins({ + required: requiredBins, + hasLocalBin: hasBinary, + hasRemoteBin: eligibility?.remote?.hasBin, + }); + const missingAnyBins = resolveMissingAnyBins({ + required: requiredAnyBins, + hasLocalBin: hasBinary, + hasRemoteAnyBin: eligibility?.remote?.hasAnyBin, + }); + const missingOs = resolveMissingOs({ + required: requiredOs, + localPlatform: process.platform, + remotePlatforms: eligibility?.remote?.platforms, + }); + const missingEnv = resolveMissingEnv({ + required: requiredEnv, + isSatisfied: (envName) => Boolean(process.env[envName] || hookConfig?.env?.[envName]), }); - const missingAnyBins = - requiredAnyBins.length > 0 && - !( - requiredAnyBins.some((bin) => hasBinary(bin)) || - eligibility?.remote?.hasAnyBin?.(requiredAnyBins) - ) - ? requiredAnyBins - : []; - - const missingOs = - requiredOs.length > 0 && - !requiredOs.includes(process.platform) && - !eligibility?.remote?.platforms?.some((platform) => requiredOs.includes(platform)) - ? requiredOs - : []; - - const missingEnv: string[] = []; - for (const envName of requiredEnv) { - if (process.env[envName]) { - continue; - } - if (hookConfig?.env?.[envName]) { - continue; - } - missingEnv.push(envName); - } - - const configChecks: HookStatusConfigCheck[] = requiredConfig.map((pathStr) => { - const value = resolveConfigPath(config, pathStr); - const satisfied = isConfigPathTruthy(config, pathStr); - return { path: pathStr, value, satisfied }; + const configChecks: HookStatusConfigCheck[] = buildConfigChecks({ + required: requiredConfig, + resolveValue: (pathStr) => resolveConfigPath(config, pathStr), + isSatisfied: (pathStr) => isConfigPathTruthy(config, pathStr), }); - const missingConfig = configChecks.filter((check) => !check.satisfied).map((check) => check.path); const missing = always diff --git a/src/shared/requirements.test.ts b/src/shared/requirements.test.ts new file mode 100644 index 00000000000..a2e4f837e59 --- /dev/null +++ b/src/shared/requirements.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, it } from "vitest"; +import { + buildConfigChecks, + resolveMissingAnyBins, + resolveMissingBins, + resolveMissingEnv, + resolveMissingOs, +} from "./requirements.js"; + +describe("requirements helpers", () => { + it("resolveMissingBins respects local+remote", () => { + expect( + resolveMissingBins({ + required: ["a", "b", "c"], + hasLocalBin: (bin) => bin === "a", + hasRemoteBin: (bin) => bin === "b", + }), + ).toEqual(["c"]); + }); + + it("resolveMissingAnyBins requires at least one", () => { + expect( + resolveMissingAnyBins({ + required: ["a", "b"], + hasLocalBin: () => false, + hasRemoteAnyBin: () => false, + }), + ).toEqual(["a", "b"]); + expect( + resolveMissingAnyBins({ + required: ["a", "b"], + hasLocalBin: (bin) => bin === "b", + }), + ).toEqual([]); + }); + + it("resolveMissingOs allows remote platform", () => { + expect( + resolveMissingOs({ + required: ["darwin"], + localPlatform: "linux", + remotePlatforms: ["darwin"], + }), + ).toEqual([]); + expect(resolveMissingOs({ required: ["darwin"], localPlatform: "linux" })).toEqual(["darwin"]); + }); + + it("resolveMissingEnv uses predicate", () => { + expect( + resolveMissingEnv({ required: ["A", "B"], isSatisfied: (name) => name === "B" }), + ).toEqual(["A"]); + }); + + it("buildConfigChecks includes value+status", () => { + expect( + buildConfigChecks({ + required: ["a.b"], + resolveValue: (p) => (p === "a.b" ? 1 : null), + isSatisfied: (p) => p === "a.b", + }), + ).toEqual([{ path: "a.b", value: 1, satisfied: true }]); + }); +}); diff --git a/src/shared/requirements.ts b/src/shared/requirements.ts new file mode 100644 index 00000000000..0de4946ea9a --- /dev/null +++ b/src/shared/requirements.ts @@ -0,0 +1,90 @@ +export type Requirements = { + bins: string[]; + anyBins: string[]; + env: string[]; + config: string[]; + os: string[]; +}; + +export type RequirementConfigCheck = { + path: string; + value: unknown; + satisfied: boolean; +}; + +export function resolveMissingBins(params: { + required: string[]; + hasLocalBin: (bin: string) => boolean; + hasRemoteBin?: (bin: string) => boolean; +}): string[] { + const remote = params.hasRemoteBin; + return params.required.filter((bin) => { + if (params.hasLocalBin(bin)) { + return false; + } + if (remote?.(bin)) { + return false; + } + return true; + }); +} + +export function resolveMissingAnyBins(params: { + required: string[]; + hasLocalBin: (bin: string) => boolean; + hasRemoteAnyBin?: (bins: string[]) => boolean; +}): string[] { + if (params.required.length === 0) { + return []; + } + if (params.required.some((bin) => params.hasLocalBin(bin))) { + return []; + } + if (params.hasRemoteAnyBin?.(params.required)) { + return []; + } + return params.required; +} + +export function resolveMissingOs(params: { + required: string[]; + localPlatform: string; + remotePlatforms?: string[]; +}): string[] { + if (params.required.length === 0) { + return []; + } + if (params.required.includes(params.localPlatform)) { + return []; + } + if (params.remotePlatforms?.some((platform) => params.required.includes(platform))) { + return []; + } + return params.required; +} + +export function resolveMissingEnv(params: { + required: string[]; + isSatisfied: (envName: string) => boolean; +}): string[] { + const missing: string[] = []; + for (const envName of params.required) { + if (params.isSatisfied(envName)) { + continue; + } + missing.push(envName); + } + return missing; +} + +export function buildConfigChecks(params: { + required: string[]; + resolveValue: (pathStr: string) => unknown; + isSatisfied: (pathStr: string) => boolean; +}): RequirementConfigCheck[] { + return params.required.map((pathStr) => { + const value = params.resolveValue(pathStr); + const satisfied = params.isSatisfied(pathStr); + return { path: pathStr, value, satisfied }; + }); +}