From 81efc24334353c8306bbfce84f4dc8e3bb6fe32e Mon Sep 17 00:00:00 2001 From: Anton Gulin Date: Sun, 5 Jul 2026 16:03:58 -0700 Subject: [PATCH] fix(skill_eval): guard against installed skill conflicts, normalize descriptionOverride Reworks #28: the conflict guard now uses the shared runProcess helper (10s timeout, no Bun globals in compiled output) with a pure, unit-tested parser for `opencode debug skill` output. Empty descriptionOverride strings are treated as omitted in both skill_eval and skill_optimize_loop. Also gates the triggerOnly early return from #29 so a triggered run that ends in a failed process still throws when triggerOnly is false. Co-authored-by: Michael Ledin Co-Authored-By: Claude Fable 5 --- plugin/dist/build-manifest.json | 4 +-- plugin/dist/skill-creator.js | 51 +++++++++++++++++++++++++++--- plugin/lib/run-eval.ts | 49 +++++++++++++++++++++++++++- plugin/skill-creator.ts | 18 +++++++++-- plugin/test/conflict-guard.test.ts | 38 ++++++++++++++++++++++ 5 files changed, 150 insertions(+), 10 deletions(-) create mode 100644 plugin/test/conflict-guard.test.ts diff --git a/plugin/dist/build-manifest.json b/plugin/dist/build-manifest.json index 0ed93ca..49edfa1 100644 --- a/plugin/dist/build-manifest.json +++ b/plugin/dist/build-manifest.json @@ -1,6 +1,6 @@ { "entrypoint": "skill-creator.ts", "runtimeEntrypoint": "runtime-entry.ts", - "sourceHash": "6e4c1da97ef75f263cacd5f8e706bd849c99792b84a7a505148fa234de660aaf", - "builtAt": "2026-06-23T08:01:15.963Z" + "sourceHash": "7e988b2130537db9015790ba36e227f7b8a5844bc1200ae7ffeab250cc201368", + "builtAt": "2026-07-05T23:03:33.548Z" } diff --git a/plugin/dist/skill-creator.js b/plugin/dist/skill-creator.js index 12febec..9d186bb 100644 --- a/plugin/dist/skill-creator.js +++ b/plugin/dist/skill-creator.js @@ -10746,7 +10746,7 @@ class JSONSchemaGenerator { if (val === undefined) { if (this.unrepresentable === "throw") { throw new Error("Literal `undefined` cannot be represented in JSON Schema"); - } else {} + } } else if (typeof val === "bigint") { if (this.unrepresentable === "throw") { throw new Error("BigInt literals cannot be represented in JSON Schema"); @@ -12623,6 +12623,42 @@ function buildEvalWarnings(results) { const allZeroWithoutErrors = shouldTriggerResults.every((r) => r.triggers === 0 && r.errors === 0); return allZeroWithoutErrors ? [ALL_ZERO_WARNING] : []; } +function findSkillConflicts(stdoutText, skillName) { + try { + const parsed = JSON.parse(stdoutText); + if (!Array.isArray(parsed)) + return []; + return parsed.flatMap((entry) => { + if (!entry || typeof entry !== "object") + return []; + const record2 = entry; + if (record2.name !== skillName) + return []; + return [ + typeof record2.location === "string" && record2.location.trim() ? record2.location : "unknown location" + ]; + }); + } catch { + return []; + } +} +async function assertNoInstalledSkillConflict(skillName, projectRoot) { + let result; + try { + result = await runProcess(["opencode", "debug", "skill"], { + cwd: projectRoot, + timeoutMs: 1e4 + }); + } catch { + return; + } + if (isFailedProcess(result)) + return; + const locations = findSkillConflicts(result.stdout, skillName); + if (locations.length === 0) + return; + throw new Error(`skill_eval conflict: skill "${skillName}" is already available to opencode at ${locations.join(", ")}. Remove that installed skill or its skills.paths entry before running skill_eval. The eval tool creates a synthetic skill named "${skillName}-skill-" and only counts that temporary skill as triggered; an installed skill with the base name can steal triggers and produce false negatives.`); +} function findProjectRoot(cwd) { let current = cwd ?? process.cwd(); const { root } = parse5(current); @@ -12717,7 +12753,7 @@ async function runSingleQuery(query, skillName, skillDescription, timeout, proje } }); flushBuffer(true); - if (triggered) { + if (triggered && triggerOnly) { return true; } if (isFailedProcess(result)) { @@ -14789,6 +14825,9 @@ function prepareReviewLaunch(args) { benchmarkPath: resolvedBenchmarkPath }; } +function normalizeDescriptionOverride(value) { + return typeof value === "string" && value.trim() ? value : undefined; +} function getAutoUpdatePaths() { const cacheDir = process.env.XDG_CACHE_HOME || join10(homedir(), ".cache"); const configDir = process.env.XDG_CONFIG_HOME || join10(homedir(), ".config"); @@ -14998,10 +15037,11 @@ var SkillCreatorPlugin = async (ctx) => { } const meta = parseSkillMd(args.skillPath); const projectRoot = findProjectRoot(); + await assertNoInstalledSkillConflict(meta.name, projectRoot); const result = await runEval({ evalSet, skillName: meta.name, - description: args.descriptionOverride ?? meta.description, + description: normalizeDescriptionOverride(args.descriptionOverride) ?? meta.description, numWorkers: args.numWorkers ?? 10, timeout: args.timeout ?? 30, projectRoot, @@ -15063,10 +15103,13 @@ var SkillCreatorPlugin = async (ctx) => { async execute(args) { const { readFileSync: readFileSync8 } = await import("fs"); const evalSet = JSON.parse(readFileSync8(args.evalSetPath, "utf-8")); + const meta = parseSkillMd(args.skillPath); + const projectRoot = findProjectRoot(); + await assertNoInstalledSkillConflict(meta.name, projectRoot); const result = await runLoop({ evalSet, skillPath: args.skillPath, - descriptionOverride: args.descriptionOverride ?? null, + descriptionOverride: normalizeDescriptionOverride(args.descriptionOverride) ?? null, numWorkers: args.numWorkers ?? 10, timeout: args.timeout ?? 30, maxIterations: args.maxIterations ?? 5, diff --git a/plugin/lib/run-eval.ts b/plugin/lib/run-eval.ts index 7e11803..6de4b97 100644 --- a/plugin/lib/run-eval.ts +++ b/plugin/lib/run-eval.ts @@ -86,6 +86,53 @@ export function buildEvalWarnings(results: EvalResultItem[]): string[] { return allZeroWithoutErrors ? [ALL_ZERO_WARNING] : [] } +export function findSkillConflicts( + stdoutText: string, + skillName: string, +): string[] { + try { + const parsed = JSON.parse(stdoutText) as unknown + if (!Array.isArray(parsed)) return [] + + return parsed.flatMap((entry) => { + if (!entry || typeof entry !== "object") return [] + const record = entry as Record + if (record.name !== skillName) return [] + return [ + typeof record.location === "string" && record.location.trim() + ? record.location + : "unknown location", + ] + }) + } catch { + return [] + } +} + +export async function assertNoInstalledSkillConflict( + skillName: string, + projectRoot: string, +): Promise { + let result + try { + result = await runProcess(["opencode", "debug", "skill"], { + cwd: projectRoot, + timeoutMs: 10_000, + }) + } catch { + return + } + + if (isFailedProcess(result)) return + + const locations = findSkillConflicts(result.stdout, skillName) + if (locations.length === 0) return + + throw new Error( + `skill_eval conflict: skill "${skillName}" is already available to opencode at ${locations.join(", ")}. Remove that installed skill or its skills.paths entry before running skill_eval. The eval tool creates a synthetic skill named "${skillName}-skill-" and only counts that temporary skill as triggered; an installed skill with the base name can steal triggers and produce false negatives.`, + ) +} + /** * Walk up from `cwd` looking for `.opencode/` or `.claude/` to find the * project root — mirrors how OpenCode discovers its project root. @@ -208,7 +255,7 @@ async function runSingleQuery( flushBuffer(true) - if (triggered) { + if (triggered && triggerOnly) { return true } diff --git a/plugin/skill-creator.ts b/plugin/skill-creator.ts index 1d31193..de177ad 100644 --- a/plugin/skill-creator.ts +++ b/plugin/skill-creator.ts @@ -21,7 +21,11 @@ import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "fs" import { validateSkill } from "./lib/validate" import { parseSkillMd } from "./lib/utils" -import { runEval, findProjectRoot } from "./lib/run-eval" +import { + assertNoInstalledSkillConflict, + runEval, + findProjectRoot, +} from "./lib/run-eval" import { improveDescription } from "./lib/improve-description" import { runLoop } from "./lib/run-loop" import { generateBenchmark, generateMarkdown } from "./lib/aggregate" @@ -130,6 +134,10 @@ function prepareReviewLaunch(args: { } } +function normalizeDescriptionOverride(value: string | undefined): string | undefined { + return typeof value === "string" && value.trim() ? value : undefined +} + // --------------------------------------------------------------------------- type AutoUpdateResult = { checked: boolean @@ -510,11 +518,12 @@ export const SkillCreatorPlugin: Plugin = async (ctx) => { const meta = parseSkillMd(args.skillPath) const projectRoot = findProjectRoot() + await assertNoInstalledSkillConflict(meta.name, projectRoot) const result = await runEval({ evalSet, skillName: meta.name, - description: args.descriptionOverride ?? meta.description, + description: normalizeDescriptionOverride(args.descriptionOverride) ?? meta.description, numWorkers: args.numWorkers ?? 10, timeout: args.timeout ?? 30, projectRoot, @@ -649,11 +658,14 @@ export const SkillCreatorPlugin: Plugin = async (ctx) => { const evalSet: EvalItem[] = JSON.parse( readFileSync(args.evalSetPath, "utf-8"), ) + const meta = parseSkillMd(args.skillPath) + const projectRoot = findProjectRoot() + await assertNoInstalledSkillConflict(meta.name, projectRoot) const result = await runLoop({ evalSet, skillPath: args.skillPath, - descriptionOverride: args.descriptionOverride ?? null, + descriptionOverride: normalizeDescriptionOverride(args.descriptionOverride) ?? null, numWorkers: args.numWorkers ?? 10, timeout: args.timeout ?? 30, maxIterations: args.maxIterations ?? 5, diff --git a/plugin/test/conflict-guard.test.ts b/plugin/test/conflict-guard.test.ts new file mode 100644 index 0000000..d0bbb91 --- /dev/null +++ b/plugin/test/conflict-guard.test.ts @@ -0,0 +1,38 @@ +import { expect, test } from "bun:test" + +import { findSkillConflicts } from "../lib/run-eval" + +test("findSkillConflicts returns locations for matching skills", () => { + expect( + findSkillConflicts( + JSON.stringify([ + { name: "other-skill", location: "/tmp/other" }, + { name: "target-skill", location: "/tmp/target" }, + ]), + "target-skill", + ), + ).toEqual(["/tmp/target"]) +}) + +test("findSkillConflicts uses unknown location when matching entry has no location", () => { + expect( + findSkillConflicts(JSON.stringify([{ name: "target-skill" }]), "target-skill"), + ).toEqual(["unknown location"]) +}) + +test("findSkillConflicts returns empty array when there is no match", () => { + expect( + findSkillConflicts( + JSON.stringify([{ name: "other-skill", location: "/tmp/other" }]), + "target-skill", + ), + ).toEqual([]) +}) + +test("findSkillConflicts returns empty array for invalid JSON", () => { + expect(findSkillConflicts("{", "target-skill")).toEqual([]) +}) + +test("findSkillConflicts returns empty array for non-array JSON", () => { + expect(findSkillConflicts(JSON.stringify({ name: "target-skill" }), "target-skill")).toEqual([]) +})