Kaynağa Gözat

fix(opencode): register skills with Skill.Info 2.0.4 path field; contain per-skill add failures

Reported on PR #2106 (80avin): on OpenCode v2.0.4 the plugin is disabled
at startup with "Plugin disabled after skill.transform failed", losing
both skill registration and bootstrap injection.

Root cause: upstream commit 199aabe9e2 (first released in v2.0.4)
renamed Skill.Info's required file field `location` -> `path` and removed
`slash`. draft.add() decodes payloads with Schema.decodeUnknownSync
against that schema, so our `location` payloads now fail decode with
"Missing key path".

Why the failure was silent: the decode error is thrown during the host's
state rebuild, where the State layer catches it and hard-disables the
whole plugin group asynchronously - the throw never reaches the
try/catch around ctx.skill.transform(), and the session "context" hook
is torn down as collateral.

Fix:
- skill payloads now use `path` (2.0.4 contract); no v2.0.3 compatibility
  retained per review decision
- draft.add() failures are contained per skill inside the transform
  callback, so one rejected payload skips that skill (visible in server
  logs) instead of the host disabling the entire plugin
- new test-skill-registration unit test pins the 2.0.4 payload contract
  (absolute path field, no stale location/slash, hostile-add containment)
  and is registered in run-tests.sh; full suite 3/3 green
GoldJohnKing 1 hafta önce
ebeveyn
işleme
07f9d6f6fc

+ 23 - 4
.opencode/plugins/superpowers.js

@@ -258,8 +258,11 @@ export const SuperpowersPlugin = async ({ client, directory }) => {
  * 1. Registers every skills/<name>/SKILL.md as a native Skill.Info object
  *    via ctx.skill.transform((draft) => draft.add(info)).
  *    V2 removed the old draft.source() directory registration; the draft API
- *    is now { list, add, update, remove } where add() decodes plain objects:
- *    { id, name, description?, slash?, autoinvoke?, location, content }.
+ *    is now { list, add, update, remove } where add() decodes plain objects
+ *    against the host's Skill.Info schema (OpenCode 2.0.4 contract):
+ *    { id, name, description?, autoinvoke?, path, content }. The file field
+ *    is `path` — renamed from `location` in upstream commit 199aabe9e2,
+ *    first released in v2.0.4.
  *    See packages/core/src/plugin/skill.ts and packages/schema/src/skill.ts.
  * 2. Injects bootstrap context via ctx.session.hook("context"), the V2
  *    equivalent of V1's experimental.chat.messages.transform.
@@ -285,13 +288,29 @@ async function setup(ctx) {
           id: entry.name,
           name: frontmatter.name || entry.name,
           ...(frontmatter.description ? { description: frontmatter.description } : {}),
-          location: skillPath,
+          // Skill.Info renamed its required file field `location` -> `path`
+          // in OpenCode v2.0.4 (upstream commit 199aabe9e2).
+          path: skillPath,
           content,
         });
       }
     }
     await ctx.skill.transform((draft) => {
-      for (const skill of skills) draft.add(skill);
+      // draft.add() decodes against the host's Skill.Info schema and throws
+      // synchronously on a mismatch. A throw escaping this callback is what
+      // the host escalates into an asynchronous hard-disable of the entire
+      // plugin ("Plugin disabled after skill.transform failed") — the
+      // try/catch around ctx.skill.transform never sees it, and the
+      // bootstrap hook is torn down as collateral. Contain failures per
+      // skill so one rejected payload skips that skill instead of killing
+      // skills AND bootstrap.
+      for (const skill of skills) {
+        try {
+          draft.add(skill);
+        } catch (err) {
+          console.error(`[superpowers] skill "${skill.id}" rejected by host, skipping:`, err);
+        }
+      }
     });
   } catch (err) {
     // Never break plugin activation: one failing plugin takes down the whole

+ 2 - 0
tests/opencode/run-tests.sh

@@ -45,6 +45,7 @@ while [[ $# -gt 0 ]]; do
             echo "Tests:"
             echo "  test-plugin-loading.sh  Verify plugin installation and structure"
             echo "  test-bootstrap-caching.sh  Verify bootstrap content caching"
+            echo "  test-skill-registration.sh  Verify V2 skill registration contract (2.0.4 path field)"
             echo "  test-tools.sh           Test use_skill and find_skills tools (integration)"
             echo "  test-priority.sh        Test skill priority resolution (integration)"
             exit 0
@@ -61,6 +62,7 @@ done
 tests=(
     "test-plugin-loading.sh"
     "test-bootstrap-caching.sh"
+    "test-skill-registration.sh"
 )
 
 # Integration tests (require OpenCode)

+ 128 - 0
tests/opencode/test-skill-registration.mjs

@@ -0,0 +1,128 @@
+import fs from 'fs';
+import path from 'path';
+import { pathToFileURL } from 'url';
+
+// Verifies the V2 skill registration payload matches OpenCode 2.0.4's
+// Skill.Info contract (packages/schema/src/skill.ts):
+//   { id, name, description?, autoinvoke?, path, content }
+// Upstream commit 199aabe9e2 (first released in v2.0.4) renamed the required
+// file field `location` -> `path`. A wrong field name makes draft.add()
+// throw inside the host's transform rebuild, which asynchronously disables
+// the whole plugin ("Plugin disabled after skill.transform failed") and
+// takes the bootstrap hook down with it — see PR #2106 review by 80avin.
+
+const [, , pluginPath] = process.argv;
+
+if (!pluginPath) {
+  console.error('Usage: node test-skill-registration.mjs PLUGIN_PATH');
+  process.exit(2);
+}
+
+const skillsDir = path.resolve(path.dirname(pluginPath), '../../skills');
+const mod = await import(pathToFileURL(pluginPath).href);
+
+const failures = [];
+
+// --- Run 1: passive capture of every draft.add payload -------------------
+const added = [];
+await mod.default.setup(makeCtx({ add: (skill) => added.push(skill) }));
+
+const expectedIds = fs.existsSync(skillsDir)
+  ? fs.readdirSync(skillsDir, { withFileTypes: true })
+      .filter((e) => e.isDirectory() && !e.name.startsWith('.'))
+      .filter((e) => fs.existsSync(path.join(skillsDir, e.name, 'SKILL.md')))
+      .map((e) => e.name)
+      .sort()
+  : [];
+
+if (added.length === 0) {
+  failures.push('expected setup() to register at least one skill via draft.add()');
+}
+if (JSON.stringify(added.map((s) => s.id).sort()) !== JSON.stringify(expectedIds)) {
+  failures.push(`expected draft.add() ids to match skills dir contents, got ${JSON.stringify(added.map((s) => s.id))}`);
+}
+
+for (const skill of added) {
+  if (typeof skill.path !== 'string' || !path.isAbsolute(skill.path)) {
+    failures.push(`skill "${skill.id}": expected required absolute Skill.Info field "path", got ${JSON.stringify(skill.path)}`);
+  } else if (skill.path !== path.join(skillsDir, skill.id, 'SKILL.md')) {
+    failures.push(`skill "${skill.id}": expected path ${path.join(skillsDir, skill.id, 'SKILL.md')}, got ${skill.path}`);
+  } else if (!fs.existsSync(skill.path)) {
+    failures.push(`skill "${skill.id}": path does not exist on disk: ${skill.path}`);
+  }
+  // Stale 2.0.3-era fields must not leak into the payload: the host strips
+  // unknown keys, but keeping them would silently mask a future regression
+  // to a schema that no longer accepts `path`.
+  if ('location' in skill) {
+    failures.push(`skill "${skill.id}": payload still carries the pre-2.0.4 field "location"`);
+  }
+  if ('slash' in skill) {
+    failures.push(`skill "${skill.id}": payload carries "slash", removed from Skill.Info in 2.0.4`);
+  }
+  if (typeof skill.id !== 'string' || skill.id.length === 0) failures.push(`skill payload missing non-empty "id"`);
+  if (typeof skill.name !== 'string' || skill.name.length === 0) failures.push(`skill "${skill.id}" missing non-empty "name"`);
+  if (typeof skill.content !== 'string' || !skill.content.trim()) failures.push(`skill "${skill.id}" missing non-empty "content"`);
+  if ('description' in skill && typeof skill.description !== 'string') {
+    failures.push(`skill "${skill.id}": "description" must be a string when present`);
+  }
+}
+
+// --- Run 2: hostile draft.add must not abort the remaining registrations --
+// The real host swallows a throw escaping the transform callback and then
+// hard-disables the plugin asynchronously. Locally we can only observe the
+// synchronous half of that contract: when draft.add() rejects one skill, the
+// plugin must keep registering the rest instead of aborting the loop.
+const hostileId = added.length > 1 ? added[Math.floor(added.length / 2)].id : null;
+const survived = [];
+let setupThrew = null;
+try {
+  await mod.default.setup(makeCtx({
+    add: (skill) => {
+      if (skill.id === hostileId) throw new Error('Simulated Skill.Info decode failure');
+      survived.push(skill.id);
+    },
+  }));
+} catch (err) {
+  setupThrew = err;
+}
+if (setupThrew) {
+  failures.push(`expected setup() to contain draft.add() failures, but it threw: ${setupThrew.message}`);
+} else if (hostileId) {
+  const expectedSurvivors = added.map((s) => s.id).filter((id) => id !== hostileId);
+  if (JSON.stringify(survived.sort()) !== JSON.stringify(expectedSurvivors.sort())) {
+    failures.push(`expected all non-rejected skills to still register when one draft.add() throws, got ${JSON.stringify(survived)}`);
+  }
+}
+
+const result = {
+  registered: added.length,
+  ids: added.map((s) => s.id),
+  allPathsValid: added.every((s) => s.path === path.join(skillsDir, s.id, 'SKILL.md') && fs.existsSync(s.path)),
+  staleLocationField: added.some((s) => 'location' in s),
+  hostileRejectedId: hostileId,
+  survivedHostileAdd: JSON.stringify(survived.sort()) === JSON.stringify(added.map((s) => s.id).filter((id) => id !== hostileId).sort()),
+};
+
+if (failures.length > 0) {
+  console.error(JSON.stringify(result, null, 2));
+  for (const failure of failures) {
+    console.error(`FAIL: ${failure}`);
+  }
+  process.exit(1);
+}
+
+console.log(JSON.stringify(result, null, 2));
+
+function makeCtx({ add }) {
+  return {
+    skill: {
+      transform: async (fn) => {
+        await fn({ list: () => [], get: () => undefined, add, update: () => {}, remove: () => {} });
+      },
+    },
+    session: {
+      hook: async () => {},
+      get: async ({ sessionID }) => ({ id: sessionID }), // top-level: no parentID
+    },
+  };
+}

+ 20 - 0
tests/opencode/test-skill-registration.sh

@@ -0,0 +1,20 @@
+#!/usr/bin/env bash
+# Test: V2 Skill Registration Contract (#2106 review)
+# Verifies setup() registers skills matching OpenCode 2.0.4's Skill.Info
+# schema (path field, no stale location/slash) and contains per-skill
+# draft.add() failures instead of aborting registration.
+set -euo pipefail
+
+SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)"
+
+echo "=== Test: V2 Skill Registration Contract ==="
+
+source "$SCRIPT_DIR/setup.sh"
+trap cleanup_test_env EXIT
+
+node "$SCRIPT_DIR/test-skill-registration.mjs" "$SUPERPOWERS_PLUGIN_FILE"
+
+echo "  [PASS] Skill payloads match the 2.0.4 Skill.Info contract"
+echo "  [PASS] A rejected draft.add() skips one skill without aborting the rest"
+echo ""
+echo "=== All skill registration tests passed ==="