fix: preserve legacy clawhub skill updates (#53206) (thanks @drobison00)

steipete · steipete · commit e530865274cd · 2026-03-23T19:34:05.000-07:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -23,6 +23,7 @@ Docs: https://docs.openclaw.ai
 - Docs/Feishu: replace `botName` with `name` in the channel config examples so the docs match the strict account schema for per-account display names. (#52753) Thanks @haroldfabla2-hue.
 - Doctor/plugins: make `openclaw doctor --fix` remove stale `plugins.allow` and `plugins.entries` refs left behind after plugin removal. Thanks @sallyom
 - Agents/replay: canonicalize malformed assistant transcript content before session-history sanitization so legacy or corrupted assistant turns stop crashing Pi replay and subagent recovery paths.
+- ClawHub/skills: keep updating already-tracked legacy Unicode slugs after the ASCII-only slug hardening, so older installs do not get stuck behind `Invalid skill slug` errors during `openclaw skills update`. (#53206) Thanks @drobison00.
 - Infra/exec trust: preserve shell-multiplexer wrapper binaries for policy checks without breaking approved-command reconstruction, so BusyBox/ToyBox allowlist and audit flows bind to the real wrapper while execution plans stay coherent. (#53134) Thanks @vincentkoc.
 - LINE/runtime-api: pre-export overlapping runtime symbols before the `line-runtime` star export so jiti no longer throws `TypeError: Cannot redefine property` on startup. (#53221) Thanks @Drickon.
 
diff --git a/src/agents/skills-clawhub.test.ts b/src/agents/skills-clawhub.test.ts
@@ -1,3 +1,6 @@
+import fs from "node:fs/promises";
+import os from "node:os";
+import path from "node:path";
 import { beforeEach, describe, expect, it, vi } from "vitest";
 
 const fetchClawHubSkillDetailMock = vi.fn();
@@ -29,7 +32,8 @@ vi.mock("../infra/archive.js", () => ({
   fileExists: fileExistsMock,
 }));
 
-const { installSkillFromClawHub, searchSkillsFromClawHub } = await import("./skills-clawhub.js");
+const { installSkillFromClawHub, searchSkillsFromClawHub, updateSkillsFromClawHub } =
+  await import("./skills-clawhub.js");
 
 describe("skills-clawhub", () => {
   beforeEach(() => {
@@ -95,6 +99,127 @@ describe("skills-clawhub", () => {
     });
   });
 
+  describe("legacy tracked slugs remain updatable", () => {
+    async function createLegacyTrackedSkillFixture(slug: string) {
+      const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skills-clawhub-"));
+      const skillDir = path.join(workspaceDir, "skills", slug);
+      await fs.mkdir(path.join(skillDir, ".clawhub"), { recursive: true });
+      await fs.mkdir(path.join(workspaceDir, ".clawhub"), { recursive: true });
+      await fs.writeFile(
+        path.join(skillDir, ".clawhub", "origin.json"),
+        `${JSON.stringify(
+          {
+            version: 1,
+            registry: "https://legacy.clawhub.ai",
+            slug,
+            installedVersion: "0.9.0",
+            installedAt: 123,
+          },
+          null,
+          2,
+        )}\n`,
+        "utf8",
+      );
+      await fs.writeFile(
+        path.join(workspaceDir, ".clawhub", "lock.json"),
+        `${JSON.stringify(
+          {
+            version: 1,
+            skills: {
+              [slug]: {
+                version: "0.9.0",
+                installedAt: 123,
+              },
+            },
+          },
+          null,
+          2,
+        )}\n`,
+        "utf8",
+      );
+      return { workspaceDir, skillDir };
+    }
+
+    it("updates all tracked legacy Unicode slugs in place", async () => {
+      const slug = "re\u0430ct";
+      const { workspaceDir } = await createLegacyTrackedSkillFixture(slug);
+      installPackageDirMock.mockResolvedValueOnce({
+        ok: true,
+        targetDir: path.join(workspaceDir, "skills", slug),
+      });
+
+      try {
+        const results = await updateSkillsFromClawHub({
+          workspaceDir,
+        });
+
+        expect(fetchClawHubSkillDetailMock).toHaveBeenCalledWith({
+          slug,
+          baseUrl: "https://legacy.clawhub.ai",
+        });
+        expect(downloadClawHubSkillArchiveMock).toHaveBeenCalledWith({
+          slug,
+          version: "1.0.0",
+          baseUrl: "https://legacy.clawhub.ai",
+        });
+        expect(results).toMatchObject([
+          {
+            ok: true,
+            slug,
+            previousVersion: "0.9.0",
+            version: "1.0.0",
+            targetDir: path.join(workspaceDir, "skills", slug),
+          },
+        ]);
+      } finally {
+        await fs.rm(workspaceDir, { recursive: true, force: true });
+      }
+    });
+
+    it("updates a legacy Unicode slug when requested explicitly", async () => {
+      const slug = "re\u0430ct";
+      const { workspaceDir } = await createLegacyTrackedSkillFixture(slug);
+      installPackageDirMock.mockResolvedValueOnce({
+        ok: true,
+        targetDir: path.join(workspaceDir, "skills", slug),
+      });
+
+      try {
+        const results = await updateSkillsFromClawHub({
+          workspaceDir,
+          slug,
+        });
+
+        expect(results).toMatchObject([
+          {
+            ok: true,
+            slug,
+            previousVersion: "0.9.0",
+            version: "1.0.0",
+            targetDir: path.join(workspaceDir, "skills", slug),
+          },
+        ]);
+      } finally {
+        await fs.rm(workspaceDir, { recursive: true, force: true });
+      }
+    });
+
+    it("still rejects an untracked Unicode slug passed to update", async () => {
+      const workspaceDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skills-clawhub-"));
+
+      try {
+        await expect(
+          updateSkillsFromClawHub({
+            workspaceDir,
+            slug: "re\u0430ct",
+          }),
+        ).rejects.toThrow("Invalid skill slug");
+      } finally {
+        await fs.rm(workspaceDir, { recursive: true, force: true });
+      }
+    });
+  });
+
   describe("normalizeSlug rejects non-ASCII homograph slugs", () => {
     it("rejects Cyrillic homograph 'а' (U+0430) in slug", async () => {
       const result = await installSkillFromClawHub({
diff --git a/src/agents/skills-clawhub.ts b/src/agents/skills-clawhub.ts
@@ -66,17 +66,36 @@ const VALID_SLUG_PATTERN = /^[a-z0-9](?:[a-z0-9-]*[a-z0-9])?$/i;
 // eslint-disable-next-line no-control-regex -- detects any character outside printable ASCII
 const NON_ASCII_PATTERN = /[^\x00-\x7F]/;
 
-function normalizeSlug(raw: string): string {
+function normalizeTrackedSlug(raw: string): string {
   const slug = raw.trim();
-  if (NON_ASCII_PATTERN.test(slug)) {
+  if (!slug || slug.includes("/") || slug.includes("\\") || slug.includes("..")) {
     throw new Error(`Invalid skill slug: ${raw}`);
   }
-  if (!slug || !VALID_SLUG_PATTERN.test(slug)) {
+  return slug;
+}
+
+function normalizeSlug(raw: string): string {
+  const slug = normalizeTrackedSlug(raw);
+  if (NON_ASCII_PATTERN.test(slug) || !VALID_SLUG_PATTERN.test(slug)) {
     throw new Error(`Invalid skill slug: ${raw}`);
   }
   return slug;
 }
 
+async function resolveRequestedUpdateSlug(params: {
+  workspaceDir: string;
+  requestedSlug: string;
+  lock: ClawHubSkillsLockfile;
+}): Promise<string> {
+  const trackedSlug = normalizeTrackedSlug(params.requestedSlug);
+  const trackedTargetDir = resolveSkillInstallDir(params.workspaceDir, trackedSlug);
+  const trackedOrigin = await readClawHubSkillOrigin(trackedTargetDir);
+  if (trackedOrigin || params.lock.skills[trackedSlug]) {
+    return trackedSlug;
+  }
+  return normalizeSlug(params.requestedSlug);
+}
+
 function resolveSkillInstallDir(workspaceDir: string, slug: string): string {
   const skillsDir = path.join(path.resolve(workspaceDir), "skills");
   const target = resolveSafeInstallDir({
@@ -225,16 +244,21 @@ async function installExtractedSkill(params: {
   return { ok: true, targetDir };
 }
 
-export async function installSkillFromClawHub(params: {
-  workspaceDir: string;
-  slug: string;
-  version?: string;
-  baseUrl?: string;
-  force?: boolean;
-  logger?: Logger;
-}): Promise<InstallClawHubSkillResult> {
+async function installSkillFromClawHubInternal(
+  params: {
+    workspaceDir: string;
+    slug: string;
+    version?: string;
+    baseUrl?: string;
+    force?: boolean;
+    logger?: Logger;
+  },
+  options?: { allowLegacyTrackedSlug?: boolean },
+): Promise<InstallClawHubSkillResult> {
   try {
-    const slug = normalizeSlug(params.slug);
+    const slug = options?.allowLegacyTrackedSlug
+      ? normalizeTrackedSlug(params.slug)
+      : normalizeSlug(params.slug);
     const { detail, version } = await resolveInstallVersion({
       slug,
       version: params.version,
@@ -309,14 +333,33 @@ export async function installSkillFromClawHub(params: {
   }
 }
 
+export async function installSkillFromClawHub(params: {
+  workspaceDir: string;
+  slug: string;
+  version?: string;
+  baseUrl?: string;
+  force?: boolean;
+  logger?: Logger;
+}): Promise<InstallClawHubSkillResult> {
+  return await installSkillFromClawHubInternal(params);
+}
+
 export async function updateSkillsFromClawHub(params: {
   workspaceDir: string;
   slug?: string;
   baseUrl?: string;
   logger?: Logger;
 }): Promise<UpdateClawHubSkillResult[]> {
   const lock = await readClawHubSkillsLockfile(params.workspaceDir);
-  const slugs = params.slug ? [normalizeSlug(params.slug)] : Object.keys(lock.skills);
+  const slugs = params.slug
+    ? [
+        await resolveRequestedUpdateSlug({
+          workspaceDir: params.workspaceDir,
+          requestedSlug: params.slug,
+          lock,
+        }),
+      ]
+    : Object.keys(lock.skills).map((slug) => normalizeTrackedSlug(slug));
   const results: UpdateClawHubSkillResult[] = [];
   for (const slug of slugs) {
     const targetDir = resolveSkillInstallDir(params.workspaceDir, slug);
@@ -330,13 +373,16 @@ export async function updateSkillsFromClawHub(params: {
       continue;
     }
     const previousVersion = origin?.installedVersion ?? lock.skills[slug]?.version ?? null;
-    const install = await installSkillFromClawHub({
-      workspaceDir: params.workspaceDir,
-      slug,
-      baseUrl,
-      force: true,
-      logger: params.logger,
-    });
+    const install = await installSkillFromClawHubInternal(
+      {
+        workspaceDir: params.workspaceDir,
+        slug,
+        baseUrl,
+        force: true,
+        logger: params.logger,
+      },
+      { allowLegacyTrackedSlug: true },
+    );
     if (!install.ok) {
       results.push(install);
       continue;
diff --git a/src/config/io.compat.test.ts b/src/config/io.compat.test.ts
@@ -196,7 +196,7 @@ describe("config io paths", () => {
     });
   });
 
-  it("still warns for same-base prerelease configs", async () => {
+  it("does not warn for same-base prerelease configs when current version is newer", async () => {
     const parsedVersion = parseOpenClawVersion(VERSION);
     if (!parsedVersion) {
       throw new Error(`Unable to parse VERSION: ${VERSION}`);
@@ -225,8 +225,8 @@ describe("config io paths", () => {
 
       io.loadConfig();
 
-      expect(logger.warn).toHaveBeenCalledWith(
-        `Config was last written by a newer OpenClaw (${touchedVersion}); current version is ${VERSION}.`,
+      expect(logger.warn).not.toHaveBeenCalledWith(
+        expect.stringContaining("Config was last written by a newer OpenClaw"),
       );
       expect(io.configPath).toBe(configPath);
     });
diff --git a/src/cron/service.session-reaper-in-finally.test.ts b/src/cron/service.session-reaper-in-finally.test.ts
@@ -29,6 +29,13 @@ function createDueIsolatedJob(params: { id: string; nowMs: number }): CronJob {
   };
 }
 
+function clearCronTimer(state: { timer: NodeJS.Timeout | null }): void {
+  if (state.timer) {
+    clearTimeout(state.timer);
+    state.timer = null;
+  }
+}
+
 describe("CronService - session reaper runs in finally block (#31946)", () => {
   beforeEach(() => {
     noopLogger.debug.mockClear();
@@ -72,14 +79,18 @@ describe("CronService - session reaper runs in finally block (#31946)", () => {
       sessionStorePath,
     });
 
-    await onTimer(state);
+    try {
+      await onTimer(state);
 
-    // After onTimer finishes (even with a job error), state.running must be
-    // false — proving the finally block executed.
-    expect(state.running).toBe(false);
+      // After onTimer finishes (even with a job error), state.running must be
+      // false — proving the finally block executed.
+      expect(state.running).toBe(false);
 
-    // The timer must be re-armed.
-    expect(state.timer).not.toBeNull();
+      // The timer must be re-armed.
+      expect(state.timer).not.toBeNull();
+    } finally {
+      clearCronTimer(state);
+    }
   });
 
   it("session reaper runs when resolveSessionStorePath is provided", async () => {
@@ -112,12 +123,16 @@ describe("CronService - session reaper runs in finally block (#31946)", () => {
       },
     });
 
-    await onTimer(state);
+    try {
+      await onTimer(state);
 
-    // The resolveSessionStorePath callback should have been invoked to build
-    // the set of store paths for the session reaper.
-    expect(resolvedPaths.length).toBeGreaterThan(0);
-    expect(state.running).toBe(false);
+      // The resolveSessionStorePath callback should have been invoked to build
+      // the set of store paths for the session reaper.
+      expect(resolvedPaths.length).toBeGreaterThan(0);
+      expect(state.running).toBe(false);
+    } finally {
+      clearCronTimer(state);
+    }
   });
 
   it("prunes expired cron-run sessions even when cron store load throws", async () => {
@@ -153,13 +168,16 @@ describe("CronService - session reaper runs in finally block (#31946)", () => {
       sessionStorePath,
     });
 
-    await expect(onTimer(state)).rejects.toThrow("Failed to parse cron store");
-
-    const updatedSessionStore = JSON.parse(await fs.readFile(sessionStorePath, "utf-8")) as Record<
-      string,
-      unknown
-    >;
-    expect(updatedSessionStore).toEqual({});
-    expect(state.running).toBe(false);
+    try {
+      await expect(onTimer(state)).rejects.toThrow("Failed to parse cron store");
+
+      const updatedSessionStore = JSON.parse(
+        await fs.readFile(sessionStorePath, "utf-8"),
+      ) as Record<string, unknown>;
+      expect(updatedSessionStore).toEqual({});
+      expect(state.running).toBe(false);
+    } finally {
+      clearCronTimer(state);
+    }
   });
 });
