Add CI workflow + fix 7 correctness bugs

.github/workflows/ci.yml

@@ -0,0 +1,40 @@
+name: CI
+
+on:
+  push:
+    branches: [master]
+  pull_request:
+    branches: [master]
+
+jobs:
+  test:
+    name: Lint & Test (Node ${{ matrix.node-version }})
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        # Node 18 is dropped from this matrix: the dev/test toolchain (vitest 4,
+        # tsup/rolldown, commander 14, @inquirer/select 5) requires Node >=20 and
+        # cannot run `npm test`/`npm run build` on Node 18. This is separate from
+        # (and doesn't verify) the package's documented "engines": ">=18" for the
+        # published CLI itself.
+        node-version: [20.x, 22.x, 24.x]
+    steps:
+      - uses: actions/checkout@v4
+
+      - name: Use Node.js ${{ matrix.node-version }}
+        uses: actions/setup-node@v4
+        with:
+          node-version: ${{ matrix.node-version }}
+          cache: npm
+
+      - name: Install dependencies
+        run: npm ci
+
+      - name: Lint (type-check)
+        run: npm run lint
+
+      - name: Run unit & integration tests
+        run: npm test
+
+      - name: Build
+        run: npm run build

src/assets/mcp.ts

@@ -37,10 +37,11 @@ function resolveConfigFilePath(
 async function handleTomlMcp(
   job: InstallJob,
   configFilePath: string,
+  assetName: string,
   newServerConfig: Record<string, unknown>,
 ): Promise<InstallResult> {
   const { parse, stringify } = await import('smol-toml');
-  const { host, assetName } = job;
+  const { host } = job;
 
   await ensureDir(path.dirname(configFilePath));
 
@@ -65,16 +66,25 @@ async function handleTomlMcp(
   if (host.id === 'vibe') {
     // Vibe uses [[mcp_servers]] array format
     const mcpServers = (parsed['mcp_servers'] as Array<Record<string, unknown>>) ?? [];
-    const existing = mcpServers.find((s) => s['name'] === assetName);
-    if (existing) {
-      return { job, status: 'exists', targetPath: configFilePath };
-    }
     const entry: Record<string, unknown> = {
       name: assetName,
       transport: 'stdio',
       command: (newServerConfig['command'] as string) ?? '',
     };
     if (newServerConfig['args']) entry['args'] = newServerConfig['args'];
+
+    const existingIdx = mcpServers.findIndex((s) => s['name'] === assetName);
+    if (existingIdx !== -1) {
+      if (JSON.stringify(mcpServers[existingIdx]) === JSON.stringify(entry)) {
+        return { job, status: 'exists', targetPath: configFilePath };
+      }
+      return {
+        job,
+        status: 'conflict',
+        targetPath: configFilePath,
+        reason: `${configFilePath} already has an mcp_servers entry named '${assetName}' with different content`,
+      };
+    }
     mcpServers.push(entry);
     parsed['mcp_servers'] = mcpServers;
   } else {
@@ -83,14 +93,23 @@ async function handleTomlMcp(
       parsed['mcp_servers'] = {};
     }
     const mcpServers = parsed['mcp_servers'] as Record<string, unknown>;
-    if (assetName in mcpServers) {
-      return { job, status: 'exists', targetPath: configFilePath };
-    }
     const entry: Record<string, unknown> = {
       command: (newServerConfig['command'] as string) ?? '',
     };
     if (newServerConfig['args']) entry['args'] = newServerConfig['args'];
     if (newServerConfig['env']) entry['env'] = newServerConfig['env'];
+
+    if (assetName in mcpServers) {
+      if (JSON.stringify(mcpServers[assetName]) === JSON.stringify(entry)) {
+        return { job, status: 'exists', targetPath: configFilePath };
+      }
+      return {
+        job,
+        status: 'conflict',
+        targetPath: configFilePath,
+        reason: `${configFilePath} already has a '${assetName}' key with different content`,
+      };
+    }
     mcpServers[assetName] = entry;
   }
 
@@ -139,12 +158,21 @@ export const mcpHandler: AssetHandler = {
 
     // Dispatch to TOML handler when config file has .toml extension
     if (configFilePath.endsWith('.toml')) {
-      return handleTomlMcp(job, configFilePath, newServerConfig);
+      return handleTomlMcp(job, configFilePath, assetName, newServerConfig);
     }
 
     const configKey = mcpCapability.configKey ?? 'mcpServers';
 
-    const existingConfig = (await readJSONOrNull<Record<string, unknown>>(configFilePath)) ?? {};
+    let existingConfig: Record<string, unknown>;
+    try {
+      existingConfig = (await readJSONOrNull<Record<string, unknown>>(configFilePath)) ?? {};
+    } catch (err) {
+      return {
+        job,
+        status: 'error',
+        reason: `Failed to parse existing config file as JSON: ${configFilePath}\n  Cause: ${(err as Error).message}`,
+      };
+    }
     const mcpServers = (existingConfig[configKey] as Record<string, unknown>) ?? {};
 
     if (assetName in mcpServers) {

src/assets/skill.ts

@@ -26,10 +26,14 @@ export const skillHandler: AssetHandler = {
     const entryFile = path.join(targetDir, 'SKILL.md');
 
     try {
-      await fs.promises.access(targetDir);
+      // Check the entry file, not just the directory: a directory that
+      // exists without SKILL.md means a previous install was interrupted
+      // partway through copyDirRecursive, and should be retried rather
+      // than permanently reported as already installed.
+      await fs.promises.access(entryFile);
       return { job, status: 'exists', targetPath: entryFile };
     } catch {
-      // target doesn't exist, proceed with install
+      // target doesn't exist or is an incomplete install, proceed with install
     }
 
     await copyDirRecursive(resolvedSource.localPath, targetDir);

src/cli.ts

@@ -89,7 +89,7 @@ Examples:
 
       if (cliInput.host) {
         hostId = cliInput.host;
-      } else if (!process.stdout.isTTY) {
+      } else if (!process.stdin.isTTY || !process.stdout.isTTY) {
         const validIds = getValidHostIds().join(', ');
         process.stderr.write(
           `agent-add error: Non-interactive environment detected. Please specify a host with --host <host>.\n`,

src/installer.ts

@@ -27,6 +27,16 @@ export interface CliInput {
   host?: string;
 }
 
+async function cleanupTempDirs(tempDirs: Set<string>): Promise<void> {
+  for (const dir of tempDirs) {
+    try {
+      await fs.promises.rm(dir, { recursive: true, force: true });
+    } catch {
+      // ignore cleanup errors
+    }
+  }
+}
+
 function getHandler(assetType: AssetType) {
   switch (assetType) {
     case 'mcp': return mcpHandler;
@@ -43,13 +53,13 @@ async function validateAsset(
 ): Promise<string | null> {
   if (assetType === 'skill') {
     if (resolved.type === 'http-file' || resolved.type === 'inline-json' || resolved.type === 'inline-md') {
-      return 'Skill 资产必须指向目录来源（本地路径或 Git URL），不支持内联内容或直接 HTTP(S) URL';
+      return 'Skill assets must point to a directory source (local path or Git URL); inline content and direct HTTP(S) URLs are not supported';
     }
     const skillMdPath = path.join(resolved.localPath, 'SKILL.md');
     try {
       await fs.promises.access(skillMdPath);
     } catch {
-      return `Skill 目录内缺少 SKILL.md 文件（期望路径：${skillMdPath}）`;
+      return `Skill directory is missing a SKILL.md file (expected path: ${skillMdPath})`;
     }
     return null;
   }
@@ -58,10 +68,10 @@ async function validateAsset(
     try {
       await fs.promises.access(resolved.localPath);
     } catch {
-      return `MCP 来源文件不存在：${resolved.localPath}`;
+      return `MCP source file does not exist: ${resolved.localPath}`;
     }
     if (!resolved.localPath.endsWith('.json')) {
-      return `MCP 来源文件扩展名必须为 .json（得到：${resolved.localPath}）`;
+      return `MCP source file extension must be .json (got: ${resolved.localPath})`;
     }
     return null;
   }
@@ -70,10 +80,10 @@ async function validateAsset(
     try {
       await fs.promises.access(resolved.localPath);
     } catch {
-      return `来源文件不存在：${resolved.localPath}`;
+      return `Source file does not exist: ${resolved.localPath}`;
     }
     if (!resolved.localPath.endsWith('.md')) {
-      return `${assetType} 来源文件扩展名必须为 .md（得到：${resolved.localPath}）`;
+      return `${assetType} source file extension must be .md (got: ${resolved.localPath})`;
     }
     return null;
   }
@@ -147,12 +157,17 @@ export async function runInstaller(
     fromExplicitFlag: boolean;
   }> = [];
 
+  const tempDirsToClean = new Set<string>();
+
   let explicitCount = explicitDescriptors.length;
   let idx = 0;
   for (const item of expandedItems) {
     const isGlob = item.source.endsWith('/*');
     const cleanSource = isGlob ? item.source.slice(0, -2) : item.source;
     const resolved = await resolveSource(cleanSource, cwd);
+    if (resolved.tempDir) {
+      tempDirsToClean.add(resolved.tempDir);
+    }
     const fromExplicitFlag = idx < explicitCount;
 
     if (isGlob) {
@@ -162,6 +177,7 @@ export async function runInstaller(
           `agent-add error: No matching files found in directory for ${item.assetType}\n` +
           `  Source: ${item.source}\n`,
         );
+        await cleanupTempDirs(tempDirsToClean);
         process.exit(2);
       }
       const dirName = path.basename(resolved.localPath);
@@ -174,7 +190,7 @@ export async function runInstaller(
         });
       }
     } else {
-      const assetName = inferName(cleanSource);
+      const assetName = inferName(cleanSource, { isDirectorySource: item.assetType === 'skill' });
       resolvedItems.push({
         assetType: item.assetType,
         assetName,
@@ -190,6 +206,7 @@ export async function runInstaller(
     if (validationError) {
       process.stderr.write(`agent-add error: ${validationError}\n`);
       process.stderr.write(`  Source: ${item.resolved.originalSource}\n`);
+      await cleanupTempDirs(tempDirsToClean);
       process.exit(2);
     }
   }
@@ -228,16 +245,7 @@ export async function runInstaller(
     results.push(result);
   }
 
-  for (const item of resolvedItems) {
-    if (item.resolved.tempDir) {
-      try {
-        const fs2 = await import('fs');
-        await fs2.promises.rm(item.resolved.tempDir, { recursive: true, force: true });
-      } catch {
-        // ignore cleanup errors
-      }
-    }
-  }
+  await cleanupTempDirs(tempDirsToClean);
 
   return { host, results };
 }

src/source/git.ts

@@ -8,18 +8,67 @@ import type { SourceType } from './index.js';
 
 const execFileAsync = promisify(execFile);
 
-export async function resolveGit(source: string, type: SourceType): Promise<ResolvedSource> {
+export interface ParsedGitSource {
+  repoUrl: string;
+  ref?: string;
+  subPath?: string;
+}
+
+/**
+ * Split a git source string into repo URL, optional @ref, and optional #subPath.
+ *
+ * Two ref orderings are supported:
+ *   - `repo.git@ref#path` (the documented convention for user-supplied sources)
+ *   - `repo.git#path@ref` (emitted by normalizeGitUrl for GitHub/GitLab
+ *     `/tree/<ref>/<path>` web URLs)
+ *
+ * For non-SSH URLs, the @ref separator before `#` is only searched for within
+ * the path segment (after the authority), so credentials embedded in the URL
+ * (e.g. https://token@github.com/org/repo.git) are not mistaken for a ref
+ * separator.
+ */
+export function parseGitSource(source: string): ParsedGitSource {
   // Step 1: split off #subPath
   const hashIdx = source.indexOf('#');
   const withoutPath = hashIdx !== -1 ? source.slice(0, hashIdx) : source;
-  const subPath = hashIdx !== -1 ? source.slice(hashIdx + 1) || undefined : undefined;
+  const rawSubPath = hashIdx !== -1 ? source.slice(hashIdx + 1) || undefined : undefined;
 
-  // Step 2: split off @ref — for SSH URLs (git@host:...), skip the leading "git@" prefix
+  // Step 2: split off @ref — for SSH URLs (git@host:...), skip the leading "git@" prefix.
+  // For other URLs, only look for @ within the path segment, so userinfo
+  // credentials (https://user:pass@host/... or https://token@host/...) aren't
+  // mistaken for the ref separator.
   const isSSH = withoutPath.startsWith('git@');
-  const searchFrom = isSSH ? 4 : 0;
+  let searchFrom = 0;
+  if (isSSH) {
+    searchFrom = 4;
+  } else {
+    const schemeIdx = withoutPath.indexOf('://');
+    if (schemeIdx !== -1) {
+      const authorityStart = schemeIdx + 3;
+      const pathStart = withoutPath.indexOf('/', authorityStart);
+      searchFrom = pathStart !== -1 ? pathStart : withoutPath.length;
+    }
+  }
   const atIdx = withoutPath.indexOf('@', searchFrom);
   const repoUrl = atIdx !== -1 ? withoutPath.slice(0, atIdx) : withoutPath;
-  const ref = atIdx !== -1 ? withoutPath.slice(atIdx + 1) || undefined : undefined;
+  let ref = atIdx !== -1 ? withoutPath.slice(atIdx + 1) || undefined : undefined;
+
+  // Step 3: if no ref was found before `#`, check for a trailing @ref after
+  // the subPath (the `#path@ref` ordering).
+  let subPath = rawSubPath;
+  if (!ref && rawSubPath) {
+    const subAtIdx = rawSubPath.lastIndexOf('@');
+    if (subAtIdx !== -1) {
+      ref = rawSubPath.slice(subAtIdx + 1) || undefined;
+      subPath = rawSubPath.slice(0, subAtIdx) || undefined;
+    }
+  }
+
+  return { repoUrl, ref, subPath };
+}
+
+export async function resolveGit(source: string, type: SourceType): Promise<ResolvedSource> {
+  const { repoUrl, ref, subPath } = parseGitSource(source);
 
   const tmpDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'agent-add-git-'));
 

src/source/index.ts

@@ -10,6 +10,11 @@ export type SourceType = 'local' | 'git-ssh' | 'git-https' | 'http-file' | 'inli
 // .git suffix: appears as .git at end, before #, @, or /
 const GIT_REPO_SUFFIX_RE = /\.git(\/|@|#|$)/;
 
+// Extensions this tool fetches directly as a single file (mcp/prompt/command/subAgent).
+// If the part of the URL before "#" already looks like one of these, the "#"
+// is treated as a URL fragment rather than a git subPath marker.
+const DIRECT_FILE_EXT_RE = /\.(json|md)$/i;
+
 // GitHub web URL: https://github.com/owner/repo[/tree|blob/ref/path]
 const GITHUB_WEB_RE = /^https?:\/\/(www\.)?github\.com\/([^/#]+\/[^/#]+?)(?:\.git)?\/?(?:\/(tree|blob)\/([^/]+)\/?(.*?))?$/;
 
@@ -73,9 +78,16 @@ export function detectSourceType(source: string): SourceType {
     return 'git-ssh';
   }
   if (source.startsWith('https://') || source.startsWith('http://')) {
-    if (GIT_REPO_SUFFIX_RE.test(source) || source.includes('#')) {
+    if (GIT_REPO_SUFFIX_RE.test(source)) {
       return 'git-https';
     }
+    const hashIdx = source.indexOf('#');
+    if (hashIdx !== -1) {
+      const beforeHash = (source.slice(0, hashIdx).split('?')[0] ?? '');
+      if (!DIRECT_FILE_EXT_RE.test(beforeHash)) {
+        return 'git-https';
+      }
+    }
     return 'http-file';
   }
   // Fallback: try JSON.parse to handle edge cases where MSYS inserts a non-whitespace