Merge pull request #185 from modelstudioai/fix/skill-init-issue

fix(skill): retry registry fetches and tolerate blocked backup cleanup

packages/cli/postinstall.js

@@ -181,7 +181,13 @@ function atomicSwap(tmpDir, catalogDir) {
     if (existsSync(backup) && !existsSync(catalogDir)) renameSync(backup, catalogDir);
     throw err;
   }
-  if (existsSync(backup)) rmSync(backup, { recursive: true, force: true });
+  // Best-effort cleanup (symmetric with core skills/extract.ts): the swap already
+  // succeeded, so a backup deletion failure must not fail the pre-download
+  try {
+    if (existsSync(backup)) rmSync(backup, { recursive: true, force: true });
+  } catch {
+    /* keep the backup on disk rather than report a completed swap as failed */
+  }
 }
 
 async function main() {
@@ -214,7 +220,11 @@ async function main() {
     }
     atomicSwap(tmpDir, catalogDir);
   } catch (err) {
-    if (existsSync(tmpDir)) rmSync(tmpDir, { recursive: true, force: true });
+    try {
+      if (existsSync(tmpDir)) rmSync(tmpDir, { recursive: true, force: true });
+    } catch {
+      /* cleanup must not mask the original error */
+    }
     throw err;
   }
 

packages/core/src/advisor/sync.ts

@@ -37,6 +37,8 @@ const MODELS_FILE = "models.jsonl";
 const THROTTLE_MS = 12 * 60 * 60 * 1000; // 12h
 /** Tighter than the interactive default: the silent channel must not stall `bl advisor recommend` */
 const INDEX_TIMEOUT_MS = 3000;
+/** No retries on the silent channel: a failed sync simply tries again on the next recommend */
+const FETCH_ATTEMPTS = 1;
 
 interface SyncState {
   lastChecked: number;
@@ -108,7 +110,7 @@ function wikiLockNeedsBackfill(contentHash: string): boolean {
 /** Fetch skills/index.json via the shared registry client and extract the wiki skill entry; returns null on any failure */
 async function fetchIndexEntry(): Promise<SkillIndexEntry | null> {
   try {
-    const index = await fetchSkillsIndex(INDEX_TIMEOUT_MS);
+    const index = await fetchSkillsIndex(INDEX_TIMEOUT_MS, FETCH_ATTEMPTS);
     return index.skills[WIKI_SKILL_NAME] ?? null;
   } catch {
     return null;
@@ -160,6 +162,7 @@ export async function maybeSyncWikiData(): Promise<boolean> {
       entry,
       detectInstalledAgents(),
       previousLinks,
+      FETCH_ATTEMPTS,
     );
     recordWikiInLock(record.lockEntry);
   } catch {

packages/core/src/skills/extract.ts

@@ -100,5 +100,12 @@ export function atomicSwap(tmpDir: string, destDir: string): void {
     if (existsSync(backup) && !existsSync(destDir)) renameSync(backup, destDir);
     throw err;
   }
-  if (existsSync(backup)) rmSync(backup, { recursive: true, force: true });
+  // Best-effort cleanup: the swap already succeeded, so a backup deletion failure
+  // (permissions, host safe-delete guards on large dirs) must not fail the install;
+  // leftover .old-* dirs are inert (skill status scans ignore them)
+  try {
+    if (existsSync(backup)) rmSync(backup, { recursive: true, force: true });
+  } catch {
+    /* keep the backup on disk rather than report a completed install as failed */
+  }
 }

packages/core/src/skills/installer.ts

@@ -61,12 +61,22 @@ export async function installSkillFromBuffer(
     atomicSwap(tmpDir, dest);
     return { name, path: dest, meta };
   } finally {
-    if (existsSync(tmpDir)) rmSync(tmpDir, { recursive: true, force: true });
+    // Best-effort cleanup: on failure paths this must not mask the original error,
+    // and on success the dir is already renamed away (existsSync → false)
+    try {
+      if (existsSync(tmpDir)) rmSync(tmpDir, { recursive: true, force: true });
+    } catch {
+      /* leave the temp dir rather than hide the real error */
+    }
   }
 }
 
 /** Install a single skill by index entry (download + validate + write to disk) */
-export async function installSkill(name: string, entry: SkillIndexEntry): Promise<InstalledSkill> {
+export async function installSkill(
+  name: string,
+  entry: SkillIndexEntry,
+  downloadAttempts?: number,
+): Promise<InstalledSkill> {
   if (entry.compression && entry.compression !== "tar.br") {
     throw new BailianError(
       `Skill ${name} uses unsupported compression format: ${entry.compression}`,
@@ -74,7 +84,7 @@ export async function installSkill(name: string, entry: SkillIndexEntry): Promis
       "Upgrade bailian-cli to the latest version and retry",
     );
   }
-  const buffer = await downloadSkillAsset(name, entry);
+  const buffer = await downloadSkillAsset(name, entry, downloadAttempts);
   return installSkillFromBuffer(name, buffer, entry.contentHash);
 }
 
@@ -116,14 +126,17 @@ export interface SkillInstallRecord {
  * entry (batch writeSkillLock for commands, best-effort upsertSkillLockEntry for silent channels).
  * recordedLinks = the skill's previously recorded fan-out paths from the lock; lets the
  * fan-out replace copy-fallback artifacts and keeps unvisited paths reclaimable.
+ * downloadAttempts = registry fetch attempts (undefined → interactive default; silent
+ * background channels pass 1 to fail fast instead of stalling the host command).
  */
 export async function installSkillWithFanout(
   name: string,
   entry: SkillIndexEntry,
   agents: AgentTarget[] = detectInstalledAgents(),
   recordedLinks: string[] = [],
+  downloadAttempts?: number,
 ): Promise<SkillInstallRecord> {
-  await installSkill(name, entry);
+  await installSkill(name, entry, downloadAttempts);
   const fanout = fanOutSkillToAgents(name, agents, recordedLinks);
   return {
     lockEntry: buildSkillLockEntry(entry, fanout.links),

packages/core/src/skills/registry.ts

@@ -1,5 +1,6 @@
 import { BailianError } from "../errors/base.ts";
 import { ExitCode } from "../errors/codes.ts";
+import { withRetry } from "../utils/retry.ts";
 import type { SkillIndexEntry, SkillsIndex } from "./types.ts";
 
 /**
@@ -9,8 +10,10 @@ import type { SkillIndexEntry, SkillsIndex } from "./types.ts";
  */
 const DEFAULT_REGISTRY_BASE_URL = "https://bailian-wiki.oss-cn-hangzhou.aliyuncs.com/skills";
 
-const INDEX_TIMEOUT_MS = 10_000;
+const INDEX_TIMEOUT_MS = 30_000;
 const ASSET_TIMEOUT_MS = 120_000;
+/** Interactive channels retry transient failures; silent background channels pass 1 to fail fast */
+const DEFAULT_ATTEMPTS = 3;
 
 export function getSkillRegistryBaseUrl(): string {
   const override = process.env.BAILIAN_SKILL_REGISTRY_URL?.trim();
@@ -20,55 +23,64 @@ export function getSkillRegistryBaseUrl(): string {
 /**
  * Fetch the remote skill index. No local caching — the diff comparison is always
  * "live remote index vs local skill-lock.json".
- * Silent background channels (advisor sync) may pass a tighter timeout than the interactive default.
+ * Silent background channels (advisor sync) may pass a tighter timeout and attempts=1
+ * than the interactive defaults.
  */
-export async function fetchSkillsIndex(timeoutMs: number = INDEX_TIMEOUT_MS): Promise<SkillsIndex> {
-  const url = `${getSkillRegistryBaseUrl()}/index.json`;
-  let res: Response;
-  try {
-    res = await fetch(url, { signal: AbortSignal.timeout(timeoutMs) });
-  } catch (err) {
-    throw new BailianError(
-      `Cannot access skill registry: ${url}`,
-      ExitCode.NETWORK,
-      "Check network connectivity; if using a private mirror, verify BAILIAN_SKILL_REGISTRY_URL configuration",
-      { cause: err },
-    );
-  }
-  if (!res.ok) {
-    throw new BailianError(
-      `Skill registry returned HTTP ${res.status}: ${url}`,
-      ExitCode.NETWORK,
-      res.status === 404
-        ? "Skill index not yet published or registry URL is incorrect; confirm the publisher has generated index.json"
-        : "Remote error, retry later",
-    );
-  }
-  let parsed: unknown;
-  try {
-    parsed = await res.json();
-  } catch (err) {
-    throw new BailianError(
-      "Skill index index.json is not valid JSON",
-      ExitCode.GENERAL,
-      "Remote may be in the middle of publishing, retry later",
-      { cause: err },
-    );
-  }
-  const index = parsed as SkillsIndex;
-  if (
-    typeof index !== "object" ||
-    index === null ||
-    typeof index.skills !== "object" ||
-    index.skills === null
-  ) {
-    throw new BailianError(
-      "Skill index index.json has invalid structure",
-      ExitCode.GENERAL,
-      "Retry later or contact the publisher",
-    );
-  }
-  return index;
+export async function fetchSkillsIndex(
+  timeoutMs: number = INDEX_TIMEOUT_MS,
+  attempts: number = DEFAULT_ATTEMPTS,
+): Promise<SkillsIndex> {
+  return withRetry(
+    async () => {
+      const url = `${getSkillRegistryBaseUrl()}/index.json`;
+      let res: Response;
+      try {
+        res = await fetch(url, { signal: AbortSignal.timeout(timeoutMs) });
+      } catch (err) {
+        throw new BailianError(
+          `Cannot access skill registry: ${url}`,
+          ExitCode.NETWORK,
+          "Check network connectivity; if using a private mirror, verify BAILIAN_SKILL_REGISTRY_URL configuration",
+          { cause: err },
+        );
+      }
+      if (!res.ok) {
+        throw new BailianError(
+          `Skill registry returned HTTP ${res.status}: ${url}`,
+          ExitCode.NETWORK,
+          res.status === 404
+            ? "Skill index not yet published or registry URL is incorrect; confirm the publisher has generated index.json"
+            : "Remote error, retry later",
+        );
+      }
+      let parsed: unknown;
+      try {
+        parsed = await res.json();
+      } catch (err) {
+        throw new BailianError(
+          "Skill index index.json is not valid JSON",
+          ExitCode.GENERAL,
+          "Remote may be in the middle of publishing, retry later",
+          { cause: err },
+        );
+      }
+      const index = parsed as SkillsIndex;
+      if (
+        typeof index !== "object" ||
+        index === null ||
+        typeof index.skills !== "object" ||
+        index.skills === null
+      ) {
+        throw new BailianError(
+          "Skill index index.json has invalid structure",
+          ExitCode.GENERAL,
+          "Retry later or contact the publisher",
+        );
+      }
+      return index;
+    },
+    { attempts },
+  );
 }
 
 /**
@@ -84,29 +96,38 @@ export function resolveAssetFileName(entry?: SkillIndexEntry): string {
 }
 
 /** Download the tar.br archive for a single skill (one skill = one GET) */
-export async function downloadSkillAsset(name: string, entry?: SkillIndexEntry): Promise<Buffer> {
-  const url = `${getSkillRegistryBaseUrl()}/${name}/${resolveAssetFileName(entry)}`;
-  let res: Response;
-  try {
-    res = await fetch(url, { signal: AbortSignal.timeout(ASSET_TIMEOUT_MS) });
-  } catch (err) {
-    throw new BailianError(
-      `Failed to download skill ${name}: ${url}`,
-      ExitCode.NETWORK,
-      "Network error, retryable",
-      {
-        cause: err,
-      },
-    );
-  }
-  if (!res.ok) {
-    throw new BailianError(
-      `Failed to download skill ${name}: HTTP ${res.status}`,
-      ExitCode.NETWORK,
-      res.status === 404
-        ? "index.json and skill object are temporarily inconsistent (publishing in progress), retry later"
-        : "Remote error, retry later",
-    );
-  }
-  return Buffer.from(await res.arrayBuffer());
+export async function downloadSkillAsset(
+  name: string,
+  entry?: SkillIndexEntry,
+  attempts: number = DEFAULT_ATTEMPTS,
+): Promise<Buffer> {
+  return withRetry(
+    async () => {
+      const url = `${getSkillRegistryBaseUrl()}/${name}/${resolveAssetFileName(entry)}`;
+      let res: Response;
+      try {
+        res = await fetch(url, { signal: AbortSignal.timeout(ASSET_TIMEOUT_MS) });
+      } catch (err) {
+        throw new BailianError(
+          `Failed to download skill ${name}: ${url}`,
+          ExitCode.NETWORK,
+          "Network error, retryable",
+          {
+            cause: err,
+          },
+        );
+      }
+      if (!res.ok) {
+        throw new BailianError(
+          `Failed to download skill ${name}: HTTP ${res.status}`,
+          ExitCode.NETWORK,
+          res.status === 404
+            ? "index.json and skill object are temporarily inconsistent (publishing in progress), retry later"
+            : "Remote error, retry later",
+        );
+      }
+      return Buffer.from(await res.arrayBuffer());
+    },
+    { attempts },
+  );
 }

packages/core/tests/skills-installer.test.ts

@@ -1,8 +1,8 @@
-import { existsSync, lstatSync, mkdtempSync, readFileSync, readdirSync, rmSync } from "fs";
-import { createHash } from "crypto";
-import { tmpdir } from "os";
-import { join } from "path";
-import { brotliCompressSync } from "zlib";
+import { existsSync, lstatSync, mkdtempSync, readFileSync, readdirSync, rmSync } from "node:fs";
+import { createHash } from "node:crypto";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { brotliCompressSync } from "node:zlib";
 import tar from "tar-stream";
 import { afterEach, expect, test, vi } from "vite-plus/test";
 import { BailianError } from "../src/errors/base.ts";
@@ -10,6 +10,18 @@ import type { AgentTarget } from "../src/skills/agents.ts";
 import { isSafeEntryName } from "../src/skills/extract.ts";
 import { installSkillFromBuffer, installSkillWithFanout } from "../src/skills/installer.ts";
 import { getSkillsDir } from "../src/skills/lock.ts";
+import { downloadSkillAsset, fetchSkillsIndex } from "../src/skills/registry.ts";
+
+/** rmSync wrapped in a spy so tests can simulate host deletion guards (e.g. safe-delete) */
+const fsMocks = vi.hoisted(() => ({
+  rmSync: vi.fn(),
+}));
+
+vi.mock("node:fs", async (importOriginal) => {
+  const actual = await importOriginal<typeof import("node:fs")>();
+  fsMocks.rmSync.mockImplementation(actual.rmSync);
+  return { ...actual, rmSync: fsMocks.rmSync };
+});
 
 /** Run in an isolated temp config dir, restore env afterwards. */
 async function inTempConfigDir(fn: () => Promise<void>): Promise<void> {
@@ -225,15 +237,83 @@ test("fanout install: downloads, links agents, and builds lock entry with merged
   });
 });
 
-test("fanout install: download failure surfaces as BailianError and leaves no canonical dir", async () => {
+test("fanout install: download failure exhausts retries and leaves no canonical dir", async () => {
   await inTempConfigDir(async () => {
-    vi.stubGlobal(
-      "fetch",
-      vi.fn(async () => ({ ok: false, status: 404 })),
-    );
+    const fetchMock = vi.fn(async () => ({ ok: false, status: 404 }));
+    vi.stubGlobal("fetch", fetchMock);
     await expect(
       installSkillWithFanout("demo", { contentHash: "sha256:whatever" }, []),
     ).rejects.toThrow(BailianError);
+    expect(fetchMock).toHaveBeenCalledTimes(3);
     expect(existsSync(join(getSkillsDir(), "demo"))).toBe(false);
   });
 });
+
+// ---- host deletion guards (safe-delete): backup cleanup must not fail a completed install ----
+
+const GUARD_ERROR =
+  '[safe-delete][SAFE_DELETE_BULK_CONFIRM_REQUIRED] {"count":1583,"threshold":500}';
+
+test("installer: guard blocking backup deletion does not fail the install", async () => {
+  await inTempConfigDir(async () => {
+    await installSkillFromBuffer("demo", await buildTarBr({ "SKILL.md": VALID_SKILL_MD }));
+    fsMocks.rmSync.mockImplementationOnce(() => {
+      throw new Error(GUARD_ERROR);
+    });
+    const v2 = "---\nname: demo\ndescription: demo skill v2\n---\n";
+    const installed = await installSkillFromBuffer("demo", await buildTarBr({ "SKILL.md": v2 }));
+    expect(installed.name).toBe("demo");
+    expect(readFileSync(join(getSkillsDir(), "demo", "SKILL.md"), "utf-8")).toBe(v2);
+    // The blocked backup stays on disk but is inert (status scans ignore .old-*)
+    const leftovers = readdirSync(getSkillsDir()).filter((entry) => entry.startsWith("demo.old-"));
+    expect(leftovers).toHaveLength(1);
+  });
+});
+
+test("installer: temp cleanup failure does not mask the original error", async () => {
+  await inTempConfigDir(async () => {
+    fsMocks.rmSync.mockImplementationOnce(() => {
+      throw new Error(GUARD_ERROR);
+    });
+    const buf = await buildTarBr({ "SKILL.md": VALID_SKILL_MD, "../evil.txt": "pwned\n" });
+    await expect(installSkillFromBuffer("demo", buf)).rejects.toThrow(/unsafe tar entry/);
+  });
+});
+
+// ---- registry retry policy ----
+
+test("registry: index fetch retries transient failures and succeeds", async () => {
+  const indexPayload = { skills: { demo: { contentHash: "sha256:abc" } } };
+  const fetchMock = vi.fn(async () => {
+    if (fetchMock.mock.calls.length < 3) throw new Error("network down");
+    return { ok: true, status: 200, json: async () => indexPayload };
+  });
+  vi.stubGlobal("fetch", fetchMock);
+  const index = await fetchSkillsIndex(1000);
+  expect(index.skills.demo.contentHash).toBe("sha256:abc");
+  expect(fetchMock).toHaveBeenCalledTimes(3);
+});
+
+test("registry: attempts=1 keeps the silent channel fail-fast", async () => {
+  const fetchMock = vi.fn(async () => {
+    throw new Error("network down");
+  });
+  vi.stubGlobal("fetch", fetchMock);
+  await expect(fetchSkillsIndex(1000, 1)).rejects.toThrow(BailianError);
+  expect(fetchMock).toHaveBeenCalledTimes(1);
+});
+
+test("registry: asset download retries transient HTTP errors", async () => {
+  const fetchMock = vi.fn(async () => {
+    if (fetchMock.mock.calls.length === 1) return { ok: false, status: 503 };
+    return {
+      ok: true,
+      status: 200,
+      arrayBuffer: async () => new Uint8Array([1, 2, 3]).buffer,
+    };
+  });
+  vi.stubGlobal("fetch", fetchMock);
+  const buffer = await downloadSkillAsset("demo");
+  expect([...buffer]).toEqual([1, 2, 3]);
+  expect(fetchMock).toHaveBeenCalledTimes(2);
+});