fix(skills): flatten skill directories to avoid nesting limitations #45
Conversation
PR SummaryIntroduces flat skill directories and zero-config Claude Code marketplace discovery, with safer cleanup and broader test coverage.
Written by Cursor Bugbot for commit 6ccabf3. This will update automatically on new commits. Configure here. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughMerge Claude Code marketplaces into config loading; adapt install/uninstall/sync to use flattened Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as plugin-install
participant Loader as Config Loader
participant Claude as Claude Code loader
participant Market as Merged Marketplaces
participant Sync as Sync Strategy
participant FS as Filesystem (.cursor)
User->>CLI: run plugin-install(pluginId, options)
CLI->>Loader: load AIPM config
CLI->>Claude: loadClaudeCodeMarketplaces()
Claude-->>CLI: claude marketplaces (prefixed) or {}
CLI->>Market: merge AIPM + Claude marketplaces
CLI->>Market: resolve marketplace & manifest
CLI->>CLI: resolvePluginPath (manifest → git/url vs dir)
alt meta-plugin detected
CLI->>Sync: syncMetaPluginToCursor(manifest, pluginPath)
Sync->>FS: ensure `.cursor/skills` exists and copy flattened skills
FS-->>Sync: copy results
else regular plugin
CLI->>Sync: syncPluginToCursor(manifest, pluginPath)
Sync->>FS: copy skills (flatten dirs, preserve files nested)
end
CLI->>Loader: persist enabled plugin to config (if initialized)
CLI-->>User: success / dry-run output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
yordis
force-pushed
the
yordis/fix-skills
branch
from
70151b8 to
a5bb358
Compare
yordis
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @package.json:
- Line 36: Update the "@typescript/native-preview" dependency entry in
package.json: replace the outdated version string "7.0.0-dev.20251014.1" with
the latest preview version (e.g. "7.0.0-dev.20250906.1") so the package uses the
most recent preview release; run npm/yarn install and update lockfile afterward
to persist the change.
- Line 24: The "typecheck" npm script currently uses the preview Go-native
typechecker "tsgo" (referenced in the "typecheck" script in package.json) which
lacks full feature parity; update the setup to avoid CI surprises by either (a)
replacing or augmenting the "typecheck" script so it runs the stable "tsc
--noEmit" (or a two-step: "tsgo --noEmit || tsc --noEmit") or (b) add explicit
documentation and CI guards where the script is invoked (the CI jobs referenced
around lines 21 and 28) that verify the project does not rely on missing
features (declaration emit, project references, etc.); choose one approach and
implement the script/CI change and a short README note to make the limitation
explicit.
In @src/commands/plugin-install.ts:
- Line 1: Prettier formatting failed for src/commands/plugin-install.ts; run the
formatter (prettier --write) on that file or the repo to apply the project's
style rules, then stage the formatted file and update the PR; ensure any import
ordering or semicolon/spacing changes (e.g., the import line "import merge from
'lodash.merge'") are preserved by the formatter so CI passes.
🧹 Nitpick comments (5)
src/config/loader.ts (1)
156-166: Consider refining the type assertion.Line 163 uses
as neverto bypass TypeScript's type checking. WhileconvertClaudeMarketplaceToAIPMincludes runtime validation viaMarketplaceSourceSchema.parse(), the type assertion could be improved for better type safety.♻️ Suggested type refinement
function accumulateClaudeMarketplace( acc: Record<string, MarketplaceSource>, - [marketplaceName, marketplaceConfig]: [string, unknown], + [marketplaceName, marketplaceConfig]: [string, ClaudeMarketplaceObjectEntry], ): Record<string, MarketplaceSource> { const prefixedName = `claude/${marketplaceName}`; acc[prefixedName] = convertClaudeMarketplaceToAIPM( marketplaceName, - marketplaceConfig as never, + marketplaceConfig, ); return acc; }Import
ClaudeMarketplaceObjectEntryat the top:+import type { ClaudeMarketplaceObjectEntry } from '../helpers/claude-code-config';tests/commands/plugin-install.test.ts (1)
56-69: Consider more defensive test assertions.This test only verifies that the
.cursor/skillsdirectory exists but doesn't check for specific skill files or directories. While this makes the test more flexible, consider adding at least one specific assertion to ensure the plugin was actually installed (not just that the directory was created).💡 Suggestion: Add specific skill verification
// Verify the plugin was installed to .cursor/ const skillsDir = join(testDir, '.cursor', 'skills'); expect(await fileExists(skillsDir)).toBe(true); + + // Verify at least one skill was installed (with flattened naming) + const entries = await readdir(skillsDir); + const hasAipmSkill = entries.some(name => + name.startsWith('aipm-claude-anthropic-agent-skills-example-skills') + ); + expect(hasAipmSkill).toBe(true);src/commands/plugin-install.ts (3)
29-36: Consider using a specific type instead ofany.
Record<string, any>loses type safety. Consider defining or reusing aMarketplacetype interface for better type checking and IDE support.Additionally, the catch block silently swallows all errors. While making AIPM config optional is correct, consider logging at debug level for troubleshooting:
} catch { - // AIPM config is optional - proceed without it + // AIPM config is optional - proceed without it + defaultIO.logDebug?.('AIPM config not found or invalid, proceeding without it'); }
42-66: Redundant condition check.On line 46,
!availableMarketplaces.includes(marketplaceName)is always true at this point since we're inside theif (!marketplace)block—if the marketplace existed inallMarketplaces,marketplacewouldn't be falsy.♻️ Suggested simplification
- if (!availableMarketplaces.includes(marketplaceName) && marketplaceName.startsWith('claude/')) { + if (marketplaceName.startsWith('claude/')) {
123-127: Misleading comment—pluginPathis always set at this point.The comment on lines 123-124 suggests
pluginPathmight not be set for git/url sources in dry-run. However, if we reach line 125,cmd.dryRunisfalse(we returned on line 120), which meansshouldValidatewastrue(per line 91:isDirectorySource || !cmd.dryRun), ensuring the validation block executed andpluginPathwas set.This throw is effectively a defensive assertion against unexpected logic changes, not handling a known code path.
♻️ Suggested comment update
- // For git/url sources in dry-run, we skip validation so pluginPath won't be set - // In this case, we can't perform the sync, so we already returned above + // Defensive check: pluginPath should always be set at this point since we + // only skip validation for git/url sources during dry-run (which returns above) if (!pluginPath) { throw new Error(`Plugin path not resolved for '${pluginName}'`); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
package.jsonsrc/commands/plugin-install.tssrc/config/loader.tssrc/helpers/claude-code-config.tssrc/helpers/sync-strategy.tstests/commands/plugin-install.test.ts
🧰 Additional context used
📓 Path-based instructions (7)
{package.json,package-lock.json,yarn.lock,pnpm-lock.yaml}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun installinstead ofnpm install,yarn install, orpnpm install
Files:
package.json
package.json
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun run <script>instead ofnpm run <script>,yarn run <script>, orpnpm run <script>
Files:
package.json
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>orts-node <file>for running scripts
Bun automatically loads .env files, so don't use the dotenv package
UseBun.serve()with built-in WebSocket, HTTPS, and routes support instead ofexpress
Usebun:sqlitefor SQLite database access instead ofbetter-sqlite3
UseBun.redisfor Redis client instead ofioredis
UseBun.sqlfor Postgres database access instead ofpgorpostgres.js
Use the built-inWebSocketAPI instead of thewspackage
PreferBun.fileovernode:fs's readFile/writeFile methods
UseBun.$for shell command execution instead of execa
Files:
src/config/loader.tssrc/helpers/sync-strategy.tssrc/commands/plugin-install.tstests/commands/plugin-install.test.tssrc/helpers/claude-code-config.ts
**/*.{html,ts,tsx,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor building
Files:
src/config/loader.tssrc/helpers/sync-strategy.tssrc/commands/plugin-install.tstests/commands/plugin-install.test.tssrc/helpers/claude-code-config.ts
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Import .css files directly in TypeScript/JavaScript frontend files and Bun's CSS bundler will handle bundling
Files:
src/config/loader.tssrc/helpers/sync-strategy.tssrc/commands/plugin-install.tstests/commands/plugin-install.test.tssrc/helpers/claude-code-config.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun --hot <file.ts>for hot module reloading during development
Files:
src/config/loader.tssrc/helpers/sync-strategy.tssrc/commands/plugin-install.tstests/commands/plugin-install.test.tssrc/helpers/claude-code-config.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun testinstead ofjestorvitestfor running tests
Files:
tests/commands/plugin-install.test.ts
🧠 Learnings (10)
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `bun <file>` instead of `node <file>` or `ts-node <file>` for running scripts
Applied to files:
package.jsontests/commands/plugin-install.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `bun test` instead of `jest` or `vitest` for running tests
Applied to files:
package.jsontests/commands/plugin-install.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{html,ts,tsx,css} : Use `bun build <file.html|file.ts|file.css>` instead of `webpack` or `esbuild` for building
Applied to files:
package.json
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.ts : Use `bun --hot <file.ts>` for hot module reloading during development
Applied to files:
package.json
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `Bun.$` for shell command execution instead of execa
Applied to files:
package.jsontests/commands/plugin-install.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to package.json : Use `bun run <script>` instead of `npm run <script>`, `yarn run <script>`, or `pnpm run <script>`
Applied to files:
package.json
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to {package.json,package-lock.json,yarn.lock,pnpm-lock.yaml} : Use `bun install` instead of `npm install`, `yarn install`, or `pnpm install`
Applied to files:
package.jsontests/commands/plugin-install.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `Bun.sql` for Postgres database access instead of `pg` or `postgres.js`
Applied to files:
package.json
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `bun:sqlite` for SQLite database access instead of `better-sqlite3`
Applied to files:
package.json
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Prefer `Bun.file` over `node:fs`'s readFile/writeFile methods
Applied to files:
src/helpers/sync-strategy.tstests/commands/plugin-install.test.ts
🧬 Code graph analysis (4)
src/config/loader.ts (2)
src/schema.ts (1)
MarketplaceSource(96-96)src/helpers/claude-code-config.ts (3)
convertClaudeMarketplaceToAIPM(315-347)isClaudeCodeInstalled(96-103)readClaudeCodeMarketplaces(109-138)
src/helpers/sync-strategy.ts (4)
src/helpers/fs.ts (2)
fileExists(26-33)ensureDir(16-24)src/errors.ts (2)
isFileNotFoundError(5-7)DirectoryNotFoundError(48-56)src/constants.ts (2)
DIR_AIPM_NAMESPACE(9-9)DIR_SKILLS(20-20)src/helpers/io.ts (1)
defaultIO(79-79)
tests/commands/plugin-install.test.ts (2)
src/commands/plugin-install.ts (1)
pluginInstall(21-157)src/helpers/fs.ts (1)
fileExists(26-33)
src/helpers/claude-code-config.ts (1)
src/schema.ts (2)
MarketplaceSource(96-96)MarketplaceSourceSchema(5-10)
🪛 GitHub Actions: CI
src/config/loader.ts
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix.
src/helpers/sync-strategy.ts
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix.
src/commands/plugin-install.ts
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix.
src/helpers/claude-code-config.ts
[warning] 1-1: Prettier formatting issues detected. Run 'prettier --write' to fix.
🔇 Additional comments (10)
src/helpers/claude-code-config.ts (1)
11-12: LGTM! Runtime validation improves type safety.Adding
MarketplaceSourceSchema.parse()wrappers ensures that all returned values conform to theMarketplaceSourcetype at runtime. This is a solid improvement that catches potential data inconsistencies early.Also applies to: 318-346
src/config/loader.ts (1)
171-182: LGTM! Zero-config Claude Code marketplace loading.The implementation correctly loads Claude Code marketplaces without requiring AIPM configuration, with appropriate fallback when Claude Code is not installed.
tests/commands/plugin-install.test.ts (1)
20-43: Verify test reliability in CI environments.This test depends on Claude Code being installed with the
anthropic-agent-skillsmarketplace and thedocument-skillsplugin being available. Ensure that:
- CI environments have Claude Code properly configured
- The required marketplace and plugin are available
- Tests don't fail due to missing external dependencies
Consider whether these integration tests should:
- Be marked as integration tests that run separately
- Include setup steps to install/configure Claude Code
- Mock the Claude Code marketplace responses for more reliable testing
src/helpers/sync-strategy.ts (3)
78-111: LGTM! Skill flattening addresses nesting limitation.The implementation correctly handles both skill subdirectories (with flattened naming) and standalone files (with nested structure for backwards compatibility). The error handling and directory creation logic are appropriate.
280-299: Well-designed flattening helper.The
getFlattenedSkillNamefunction provides a clear and consistent approach to generating flat directory names. The documentation clearly explains why this flattening is necessary, referencing the Claude Code issue.
365-388: Excellent error handling improvement.The graceful degradation for missing skills (lines 378-388) prevents a single missing skill from breaking the entire sync operation. This is a significant reliability improvement, as manifests may reference skills that don't exist yet or have been moved.
src/commands/plugin-install.ts (4)
1-19: LGTM!Imports and schema definition are clean. The
PluginInstallOptionsSchemacorrectly validates the requiredpluginIdand optionalcwd/dryRunparameters using Zod.
68-84: LGTM!Clean separation of path resolution logic for git/url vs directory sources, with appropriate error handling when no path is configured.
86-116: LGTM!The
shouldValidatelogic correctly differentiates between directory sources (always validate since it's cheap) and git/url sources (skip expensive clone operations during dry-run). Error handling is comprehensive with clear messages.
129-151: LGTM!The meta-plugin handling correctly distinguishes between:
- True meta-plugins with skills defined in the marketplace manifest → uses
syncMetaPluginToCursor- Meta-plugin paths where skills are in
plugin.json→ falls back tosyncPluginToCursorThis addresses the PR objective of adding fallback support for meta-plugins. The sync result formatting and success messaging are clear.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/commands/plugin-install.ts:
- Line 1: The file has Prettier formatting violations (e.g., the import
statement "import merge from 'lodash.merge';" likely misformatted); run Prettier
to reformat the file (e.g., run prettier --write on the file) or apply the
project's Prettier rules so the import and surrounding code conform to the
formatter, then stage the reformatted changes before merging.
- Around line 94-96: The conditional is overly defensive because
MarketplaceSourceSchema guarantees marketplace.source; replace the
isDirectorySource calculation to only check equality against 'directory' (use
marketplace.source === 'directory' for isDirectorySource) and leave
shouldValidate as isDirectorySource || !cmd.dryRun; update any related comments
and remove the redundant !marketplace.source branch, and ensure callers
referencing isDirectorySource/shouldValidate keep the same semantics.
In @src/commands/plugin-uninstall.ts:
- Around line 150-174: The flattened skill cleanup (reading skillsDir, computing
flattenedPrefix, iterating entries and removing directories) currently only runs
in the non-meta branch and must be moved to always run during uninstall;
relocate the block that builds flattenedPrefix and removes directories (using
skillsDir, flattenedPrefix, entry.name, deletedCount, isFileNotFoundError and
rm) out of the conditional that distinguishes meta vs non-meta plugins so it
executes for both syncPluginToCursor and syncMetaPluginToCursor cases,
preserving the existing error handling for directory read/removal.
🧹 Nitpick comments (9)
src/commands/plugin-uninstall.ts (1)
156-156: Consider extracting the flattened prefix logic to avoid duplication.The flattened prefix construction
aipm-${marketplaceName.replace(/\//g, '-')}-${pluginName}duplicates the logic fromgetFlattenedSkillNameinsync-strategy.ts. This creates maintenance burden and collision risk if the naming pattern changes.Proposed refactor
Export
getFlattenedSkillNamefromsync-strategy.tsand use it here:+import { getFlattenedSkillName } from '../helpers/sync-strategy'; ... - const flattenedPrefix = `aipm-${marketplaceName.replace(/\//g, '-')}-${pluginName}`; + const flattenedPrefix = getFlattenedSkillName(DIR_AIPM_NAMESPACE, marketplaceName, pluginName);src/helpers/sync-strategy.ts (4)
83-91: Simplify error handling for optional skills directory.The error handling wraps a
readdiroperation and throwsDirectoryNotFoundErrorfor non-ENOENT errors. However, since the skills directory is optional (checked withfileExistsat line 83), the try-catch should just skip processing if the directory doesn't exist or can't be read, rather than throwing a specialized error.Simplified error handling
if (await fileExists(skillsSourcePath)) { - let entries: Dirent[] = []; try { - entries = await readdir(skillsSourcePath, { withFileTypes: true }); + const entries = await readdir(skillsSourcePath, { withFileTypes: true }); + + for (const entry of entries) { + const skillSourcePath = join(skillsSourcePath, entry.name); + + if (entry.isDirectory()) { + // Flatten skill subdirectories to avoid nesting limitations + const flattenedSkillName = getFlattenedSkillName(DIR_AIPM_NAMESPACE, marketplaceName, pluginName, entry.name); + const skillTargetPath = join(cursorDir, 'skills', flattenedSkillName); + await ensureDir(join(cursorDir, 'skills')); + await cp(skillSourcePath, skillTargetPath, { recursive: true }); + result.skillsCount++; + } else if (entry.isFile()) { + // Handle files directly in skills/ directory (maintain nested structure for backwards compatibility) + const fileTargetPath = join(cursorDir, 'skills', DIR_AIPM_NAMESPACE, marketplaceName, pluginName, entry.name); + await ensureDir(join(cursorDir, 'skills', DIR_AIPM_NAMESPACE, marketplaceName, pluginName)); + await cp(skillSourcePath, fileTargetPath); + result.skillsCount++; + } + } } catch (error: unknown) { if (!isFileNotFoundError(error)) { - throw new DirectoryNotFoundError(skillsSourcePath, { cause: error }); + throw error; } } - - for (const entry of entries) { - const skillSourcePath = join(skillsSourcePath, entry.name); - - if (entry.isDirectory()) { - // Flatten skill subdirectories to avoid nesting limitations - const flattenedSkillName = getFlattenedSkillName(DIR_AIPM_NAMESPACE, marketplaceName, pluginName, entry.name); - const skillTargetPath = join(cursorDir, 'skills', flattenedSkillName); - await ensureDir(join(cursorDir, 'skills')); - await cp(skillSourcePath, skillTargetPath, { recursive: true }); - result.skillsCount++; - } else if (entry.isFile()) { - // Handle files directly in skills/ directory (maintain nested structure for backwards compatibility) - const fileTargetPath = join(cursorDir, 'skills', DIR_AIPM_NAMESPACE, marketplaceName, pluginName, entry.name); - await ensureDir(join(cursorDir, 'skills', DIR_AIPM_NAMESPACE, marketplaceName, pluginName)); - await cp(skillSourcePath, fileTargetPath); - result.skillsCount++; - } - } }
280-309: Document or mitigate the collision risk.The comment clearly documents the theoretical collision risk when marketplace or plugin names contain hyphens. The suggested mitigation (hash suffix) is not implemented. While the comment states this is "mitigated in practice" by centralized management, this could become an issue as the system scales.
Add hash suffix for collision prevention
+import { createHash } from 'node:crypto'; + function getFlattenedSkillName( namespace: string, marketplaceName: string, pluginName: string, skillPath?: string, ): string { const parts = [namespace, marketplaceName.replace(/\//g, '-'), pluginName]; if (skillPath) { parts.push(skillPath.replace(/\//g, '-')); } - return parts.join('-'); + const baseName = parts.join('-'); + + // Add 8-character hash suffix to prevent collisions + const hash = createHash('sha256') + .update(`${namespace}/${marketplaceName}/${pluginName}/${skillPath || ''}`) + .digest('hex') + .substring(0, 8); + + return `${baseName}-${hash}`; }Note: This would require updating cleanup logic in uninstall and sync to use the same hash-based matching.
298-298: Consider exportinggetFlattenedSkillNamefor reuse.The function is currently private but the same flattening logic is duplicated in
plugin-uninstall.ts(line 156). Exporting this function would improve maintainability and ensure consistent naming across the codebase.-function getFlattenedSkillName( +export function getFlattenedSkillName( namespace: string, marketplaceName: string, pluginName: string, skillPath?: string, ): string {
388-398: Improve error visibility for skill copy failures.The error handling catches all errors during skill copying but only logs an info message for ENOENT. Non-ENOENT errors are rethrown but this happens silently in a loop, which could leave the sync in a partially completed state without clear indication of what failed.
Enhanced error logging
try { await cp(fullSkillPath, targetPath, { recursive: true }); skillsCount++; } catch (error: unknown) { if (!isFileNotFoundError(error)) { - throw error; + const errorMsg = getErrorMessage(error); + defaultIO.logError( + `❌ Failed to copy skill '${skillNamePath}' from ${fullSkillPath}: ${errorMsg}`, + ); + throw error; } defaultIO.logInfo( `⚠️ Skill not found in manifest: ${skillNamePath} (expected at ${fullSkillPath})`, ); }src/commands/sync.ts (1)
106-121: Improve error handling in cleanup routine.The cleanup routine silently ignores ALL errors (lines 118-120), not just file-not-found errors. This could hide real issues like permission problems or I/O errors that would prevent proper syncing of enabled plugins.
Selective error handling
// Clean up flattened skill directories for disabled plugins // Remove all aipm-* skills; they'll be resync'd with enabled plugins const skillsDir = join(targetDir, 'skills'); if (await fileExists(skillsDir)) { try { const entries = await readdir(skillsDir, { withFileTypes: true }); for (const entry of entries) { if (entry.isDirectory() && entry.name.startsWith('aipm-')) { - await rm(join(skillsDir, entry.name), { recursive: true, force: true }); + try { + await rm(join(skillsDir, entry.name), { recursive: true, force: true }); + } catch (error: unknown) { + if (!isFileNotFoundError(error)) { + defaultIO.logInfo( + `⚠️ Could not remove skill directory ${entry.name}: ${getErrorMessage(error)}`, + ); + } + } } } - } catch { - // Ignore errors during cleanup + } catch (error: unknown) { + if (!isFileNotFoundError(error)) { + defaultIO.logInfo( + `⚠️ Could not read skills directory during cleanup: ${getErrorMessage(error)}`, + ); + } } }src/commands/plugin-install.ts (3)
30-41: Fragile error handling based on error messages.The code checks for "not found" in error messages (line 37) to distinguish between different error types. This is fragile because error messages can change and may not be consistently formatted across different error sources.
Use error types instead
let aipmMarketplaces: Record<string, any> = {}; try { const config = await loadPluginsConfig(cwd); aipmMarketplaces = config.config.marketplaces || {}; } catch (error: unknown) { // AIPM config is optional - proceed without it // Log at info level so users know about config issues but can continue - if (!(error instanceof Error && error.message.includes('not found'))) { + if (!isFileNotFoundError(error)) { const message = getErrorMessage(error); defaultIO.logInfo(`ℹ️ Config loading: ${message}`); } }
128-132: Remove redundant pluginPath check.The check at lines 128-132 is redundant because the code already returns early for dry-run at line 123-126. If execution reaches line 128, it means we're not in dry-run mode and
shouldValidatewas true, which guaranteespluginPathwas set.Remove redundant check
if (cmd.dryRun) { defaultIO.logInfo(`[DRY RUN] Would sync ${cmd.pluginId} to .cursor/skills/`); return; } - // For git/url sources in dry-run, we skip validation so pluginPath won't be set - // In this case, we can't perform the sync, so we already returned above - if (!pluginPath) { - throw new Error(`Plugin path not resolved for '${pluginName}'`); - } + // At this point, pluginPath must be set (we validated above) + if (!pluginPath) { + throw new Error(`Internal error: Plugin path not resolved for '${pluginName}'`); + } const cursorDir = join(cwd, DIR_CURSOR);
137-151: Simplify meta-plugin detection logic.The meta-plugin detection and sync logic is nested and could be simplified for better readability. The current structure has three branches: meta-plugin with skills in manifest, meta-plugin without skills in manifest, and regular plugin.
Simplified control flow
const cursorDir = join(cwd, DIR_CURSOR); const isMetaPluginCheck = isMetaPlugin(marketplacePath, pluginPath, manifest); - // Check if it's a meta-plugin with skills defined in the manifest let syncResult; - if (isMetaPluginCheck && manifest) { + + // Meta-plugins with skills in manifest use special sync + if (isMetaPluginCheck && manifest) { const pluginEntry = manifest.plugins.find((p) => p.name === pluginName); - if (pluginEntry && pluginEntry.skills && pluginEntry.skills.length > 0) { - // True meta-plugin with skills in manifest + const hasSkillsInManifest = pluginEntry?.skills && pluginEntry.skills.length > 0; + + if (hasSkillsInManifest) { syncResult = await syncMetaPluginToCursor(marketplacePath, manifest, pluginName, marketplaceName, cursorDir); } else { - // Meta-plugin path but skills are in plugin.json (not in marketplace manifest) - // Fall back to regular plugin sync + // Fallback: meta-plugin path but skills defined in plugin.json syncResult = await syncPluginToCursor(pluginPath, marketplaceName, pluginName, cursorDir); } } else { + // Regular plugin sync syncResult = await syncPluginToCursor(pluginPath, marketplaceName, pluginName, cursorDir); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/commands/plugin-install.tssrc/commands/plugin-uninstall.tssrc/commands/sync.tssrc/helpers/sync-strategy.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>orts-node <file>for running scripts
Bun automatically loads .env files, so don't use the dotenv package
UseBun.serve()with built-in WebSocket, HTTPS, and routes support instead ofexpress
Usebun:sqlitefor SQLite database access instead ofbetter-sqlite3
UseBun.redisfor Redis client instead ofioredis
UseBun.sqlfor Postgres database access instead ofpgorpostgres.js
Use the built-inWebSocketAPI instead of thewspackage
PreferBun.fileovernode:fs's readFile/writeFile methods
UseBun.$for shell command execution instead of execa
Files:
src/commands/sync.tssrc/commands/plugin-uninstall.tssrc/commands/plugin-install.tssrc/helpers/sync-strategy.ts
**/*.{html,ts,tsx,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor building
Files:
src/commands/sync.tssrc/commands/plugin-uninstall.tssrc/commands/plugin-install.tssrc/helpers/sync-strategy.ts
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Import .css files directly in TypeScript/JavaScript frontend files and Bun's CSS bundler will handle bundling
Files:
src/commands/sync.tssrc/commands/plugin-uninstall.tssrc/commands/plugin-install.tssrc/helpers/sync-strategy.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun --hot <file.ts>for hot module reloading during development
Files:
src/commands/sync.tssrc/commands/plugin-uninstall.tssrc/commands/plugin-install.tssrc/helpers/sync-strategy.ts
🧠 Learnings (1)
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Prefer `Bun.file` over `node:fs`'s readFile/writeFile methods
Applied to files:
src/helpers/sync-strategy.ts
🧬 Code graph analysis (4)
src/commands/sync.ts (1)
src/helpers/fs.ts (1)
fileExists(26-33)
src/commands/plugin-uninstall.ts (2)
src/constants.ts (2)
DIR_CURSOR(4-4)DIR_SKILLS(20-20)src/errors.ts (1)
isFileNotFoundError(5-7)
src/commands/plugin-install.ts (8)
src/helpers/plugin.ts (2)
parsePluginId(13-21)isMetaPlugin(45-58)src/config/loader.ts (2)
loadPluginsConfig(87-142)loadClaudeCodeMarketplaces(171-182)src/errors.ts (1)
getErrorMessage(13-15)src/helpers/claude-code-config.ts (1)
isClaudeCodeInstalled(96-103)src/helpers/git.ts (1)
resolveMarketplacePath(34-103)src/helpers/marketplace.ts (1)
getMarketplaceType(13-15)src/helpers/fs.ts (1)
fileExists(26-33)src/constants.ts (1)
DIR_CURSOR(4-4)
src/helpers/sync-strategy.ts (4)
src/helpers/fs.ts (2)
fileExists(26-33)ensureDir(16-24)src/errors.ts (2)
isFileNotFoundError(5-7)DirectoryNotFoundError(48-56)src/constants.ts (2)
DIR_AIPM_NAMESPACE(9-9)DIR_SKILLS(20-20)src/helpers/io.ts (1)
defaultIO(79-79)
🪛 GitHub Actions: CI
src/commands/plugin-install.ts
[warning] 1-1: Prettier formatting issues detected in this file. Run 'prettier --write' to fix formatting.
src/helpers/sync-strategy.ts
[warning] 1-1: Prettier formatting issues detected in this file. Run 'prettier --write' to fix formatting.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
src/helpers/sync-strategy.ts (2)
1-1: Fix Prettier formatting issues.The pipeline detected Prettier formatting issues in this file. Run
prettier --write src/helpers/sync-strategy.tsto fix them before merging.As per pipeline failure logs.
⛔ Skipped due to learnings
Learnt from: CR Repo: TrogonStack/aipm PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-11-24T17:14:51.342Z Learning: Applies to **/*.{ts,tsx,js,jsx} : Prefer `Bun.file` over `node:fs`'s readFile/writeFile methods
103-109: Document the backwards compatibility requirement and rationale for the mixed approach.The code maintains files directly in
skills/with nested structure (lines 103-109) while flattening subdirectories (lines 96-102). While test coverage exists for this file-handling path, the backwards compatibility requirement lacks clear justification:
- What existing plugins depend on files directly in
skills/?- Why not flatten files consistently with directories?
Add documentation explaining:
- The specific plugins or use cases requiring this nested file structure
- When this special case applies and why
- Whether this is a temporary measure for migration or permanent API support
If this is defensive coding without actual use cases, consider consolidating to a single strategy (e.g., flatten everything) for a more predictable directory structure.
src/commands/plugin-install.ts (1)
44-44: The review comment's concern about nested properties is unfounded. TheMarketplaceSourceSchemais a flat object with only top-level string properties (source,url,path,branch) — no nested objects exist. More importantly, the codebase intentionally uses deep merge for config composition (confirmed by tests), and the current behavior allows both Claude Code auto-discovered marketplaces and AIPM-configured marketplaces to coexist. Switching to shallow merge would break this by causing AIPM marketplaces to completely override Claude ones rather than merging them together.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/commands/sync.ts:
- Line 1: The file's import line formatting (e.g., the import of readdir, rm,
stat) fails Prettier; run Prettier with --write (or your project's formatting
script like npm run format) on the file to fix formatting, then re-add the
formatted commit so the pipeline passes.
🧹 Nitpick comments (2)
src/commands/plugin-install.ts (2)
37-37: String-based error checking is fragile.The check
!(error instanceof Error && error.message.includes('not found'))relies on specific error message text, which is brittle and hard to maintain. Consider using error types or codes for more robust error handling.♻️ Potential improvement
If the error types are available, use them instead:
- if (!(error instanceof Error && error.message.includes('not found'))) { + if (error instanceof ConfigNotFoundError) { + // Silently ignore config not found + } else { const message = getErrorMessage(error); defaultIO.logInfo(`ℹ️ Config loading: ${message}`); }Alternatively, check for specific error codes if available.
47-71: User-friendly error messages with good context.The enhanced error handling provides helpful feedback about Claude Code installation status and available marketplaces, improving the developer experience.
Optional: Consider early returns to reduce nesting
The nested conditionals could be flattened for improved readability:
if (!marketplace) { const availableMarketplaces = Object.keys(allMarketplaces); - let errorMessage: string; - if (!availableMarketplaces.includes(marketplaceName) && marketplaceName.startsWith('claude/')) { - const claudeCodeInstalled = await isClaudeCodeInstalled(); - if (!claudeCodeInstalled) { - errorMessage = `Claude Code is not installed...`; - } else { - errorMessage = `Marketplace '${marketplaceName}' not found in Claude Code...`; - } - } else if (availableMarketplaces.length === 0) { - errorMessage = `Marketplace '${marketplaceName}' not found. No marketplaces available.`; - } else { - errorMessage = `Marketplace '${marketplaceName}' not found...`; - } + + let errorMessage: string; + if (availableMarketplaces.length === 0) { + errorMessage = `Marketplace '${marketplaceName}' not found. No marketplaces available.`; + } else if (marketplaceName.startsWith('claude/') && !availableMarketplaces.includes(marketplaceName)) { + const claudeCodeInstalled = await isClaudeCodeInstalled(); + errorMessage = claudeCodeInstalled + ? `Marketplace '${marketplaceName}' not found in Claude Code. Available marketplaces: ${availableMarketplaces.join(', ')}` + : `Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like '${marketplaceName}'.`; + } else { + errorMessage = `Marketplace '${marketplaceName}' not found. Available marketplaces: ${availableMarketplaces.join(', ')}`; + } const error = new Error(errorMessage);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/commands/plugin-install.tssrc/commands/sync.tssrc/config/loader.tssrc/helpers/claude-code-config.tssrc/helpers/sync-strategy.tstests/commands/sync.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/config/loader.ts
- src/helpers/claude-code-config.ts
- src/helpers/sync-strategy.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>orts-node <file>for running scripts
Bun automatically loads .env files, so don't use the dotenv package
UseBun.serve()with built-in WebSocket, HTTPS, and routes support instead ofexpress
Usebun:sqlitefor SQLite database access instead ofbetter-sqlite3
UseBun.redisfor Redis client instead ofioredis
UseBun.sqlfor Postgres database access instead ofpgorpostgres.js
Use the built-inWebSocketAPI instead of thewspackage
PreferBun.fileovernode:fs's readFile/writeFile methods
UseBun.$for shell command execution instead of execa
Files:
src/commands/sync.tstests/commands/sync.test.tssrc/commands/plugin-install.ts
**/*.{html,ts,tsx,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor building
Files:
src/commands/sync.tstests/commands/sync.test.tssrc/commands/plugin-install.ts
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Import .css files directly in TypeScript/JavaScript frontend files and Bun's CSS bundler will handle bundling
Files:
src/commands/sync.tstests/commands/sync.test.tssrc/commands/plugin-install.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun --hot <file.ts>for hot module reloading during development
Files:
src/commands/sync.tstests/commands/sync.test.tssrc/commands/plugin-install.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun testinstead ofjestorvitestfor running tests
Files:
tests/commands/sync.test.ts
🧠 Learnings (8)
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `bun test` instead of `jest` or `vitest` for running tests
Applied to files:
tests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `Bun.$` for shell command execution instead of execa
Applied to files:
tests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `bun <file>` instead of `node <file>` or `ts-node <file>` for running scripts
Applied to files:
tests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Prefer `Bun.file` over `node:fs`'s readFile/writeFile methods
Applied to files:
tests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to {package.json,package-lock.json,yarn.lock,pnpm-lock.yaml} : Use `bun install` instead of `npm install`, `yarn install`, or `pnpm install`
Applied to files:
tests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Bun automatically loads .env files, so don't use the dotenv package
Applied to files:
tests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to package.json : Use `bun run <script>` instead of `npm run <script>`, `yarn run <script>`, or `pnpm run <script>`
Applied to files:
tests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.ts : Use `bun --hot <file.ts>` for hot module reloading during development
Applied to files:
tests/commands/sync.test.ts
🧬 Code graph analysis (3)
src/commands/sync.ts (3)
src/constants.ts (2)
DIR_SKILLS(20-20)DIR_AIPM_NAMESPACE(9-9)src/helpers/io.ts (1)
defaultIO(79-79)src/helpers/hooks-merger.ts (1)
mergeHooks(85-90)
tests/commands/sync.test.ts (2)
src/commands/sync.ts (1)
sync(47-250)src/commands/plugin-disable.ts (1)
pluginDisable(16-59)
src/commands/plugin-install.ts (6)
src/helpers/plugin.ts (1)
parsePluginId(13-21)src/config/loader.ts (2)
loadPluginsConfig(87-142)loadClaudeCodeMarketplaces(168-179)src/helpers/claude-code-config.ts (1)
isClaudeCodeInstalled(96-103)src/helpers/git.ts (1)
resolveMarketplacePath(34-103)src/helpers/marketplace.ts (1)
loadMarketplaceManifest(78-86)src/helpers/sync-strategy.ts (1)
syncMetaPluginToCursor(363-400)
🪛 GitHub Actions: CI
src/commands/sync.ts
[warning] 1-1: Code style issues found in this file. Run Prettier with --write to fix.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (7)
src/commands/sync.ts (2)
96-117: LGTM! Cleanup logic is safe and well-designed.The cleanup correctly removes only AIPM-managed directories (flattened
aipm-*and nestedaipm/) while preserving user files. The try-catch appropriately handles the case where the skills directory doesn't exist yet.
120-127: Good reordering to ensure cleanup runs even with no plugins.Moving the early return after cleanup ensures orphaned flattened skill directories are removed even when all plugins are disabled, which aligns with the PR objectives.
tests/commands/sync.test.ts (1)
714-766: Well-designed test for flattened skill cleanup.The test correctly validates that flattened skill directories (prefixed with
aipm-) are cleaned up when plugins are disabled, which aligns with the cleanup logic added insrc/commands/sync.ts.src/commands/plugin-install.ts (4)
43-45: LGTM! Marketplace merge precedence is correct.The merge order gives AIPM marketplaces precedence over Claude Code marketplaces, which aligns with the documented priority (local > project > global > claude). Name conflicts are already handled in
loadPluginsConfig.
73-83: Path resolution correctly handles different source types.The logic appropriately separates git/url sources (which need cloning/downloading) from directory sources (which use the path directly). The subsequent null check at line 85 ensures safety.
94-121: Smart validation strategy for dry-run mode.The logic correctly skips expensive validation for git/url sources in dry-run mode while always validating directory sources, improving performance without sacrificing correctness.
137-151: Good fallback implementation for meta-plugin skills.The logic correctly handles meta-plugins with skills defined in the marketplace manifest versus those with skills in
plugin.json, as described in the PR objectives. The fallback ensures skills are discovered regardless of their definition location.
yordis
force-pushed
the
yordis/fix-skills
branch
from
f3fa0b9 to
f285018
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/plugin-install.ts (1)
4-68: Behavior is coherent, but it makes tests host-dependent unless Claude marketplaces are mockable.This change is the root cause of the current CI failures: installing a
claude/...plugin now correctly errors when Claude Code isn’t installed, but the test suite expects success.Also:
if (config.plugins[cmd.pluginId]) return;depends on merged-config semantics; if “installed in another source” is a thing, consider logging that explicitly (or checking target config) to avoid confusion.
🤖 Fix all issues with AI agents
In @src/commands/sync.ts:
- Around line 103-125: The current bare catch after reading skillsDir swallows
all errors; change it to catch the error object (e) and only ignore
ENOENT/not-found errors by checking e.code (or an isFileNotFoundError helper)
and rethrow any other errors so permission/IO issues surface; keep the
successful-cleanup logs and optionally emit a debug/info log when skipping due
to ENOENT.
In @src/helpers/sync-strategy.ts:
- Around line 78-111: Root-level files in skills/ are being copied into a nested
path using DIR_AIPM_NAMESPACE (entry.isFile branch) while subdirectories are
flattened (entry.isDirectory branch using getFlattenedSkillName), causing
inconsistent layout; change the entry.isFile branch to use the same flattening
logic as the directory branch: compute flattenedSkillName via
getFlattenedSkillName(DIR_AIPM_NAMESPACE, marketplaceName, pluginName,
entry.name), create/ensure the target dir join(cursorDir, 'skills',
flattenedSkillName) and copy the file into that flattened directory (increment
result.skillsCount as before), instead of writing to the nested
aipm/marketplace/plugin path.
In @tests/commands/plugin-install.test.ts:
- Around line 20-70: The test suite depends on the external Claude Code host and
will fail in CI; update the tests to mock loadClaudeCodeMarketplaces() and
isClaudeCodeInstalled() (the same mocks used in plugin-search.test.ts) so the
"should error if marketplace not found" case returns the marketplace-not-found
error instead of the "Claude Code is not installed" message, or alternatively
relax the expectation to accept either error string; locate mocks around
pluginInstall usage in tests/commands/plugin-install.test.ts and apply the mocks
before calling pluginInstall (or change the expect to allow both messages).
In @tests/commands/plugin-uninstall.test.ts:
- Around line 915-954: Replace the placeholder assertion with a real assertion:
create the two skill directories described (e.g.,
.cursor/skills/aipm-local-plugin-skill1 and
.cursor/skills/aipm-local-plugin-extra-skill1) after writing the config at
pluginsPath, invoke the uninstall flow for "plugin@local" with removeFiles: true
(use the same uninstall helper used elsewhere in these tests), then assert that
the plugin-skill1 path was removed and the plugin-extra-skill1 path still
exists; use the existing variables/pluginsPath and aipmDir to locate files and
the uninstall helper used in other tests to trigger cleanup.
🧹 Nitpick comments (6)
src/helpers/sync-strategy.ts (2)
280-309: Flattened naming: collision risk is documented, but still untested/unmitigated in code.
Given the warning in the docstring, I’d at least add a focused unit/integration test that proves collisions won’t silently clobber directories (and/or that collision detection kicks in where needed).
375-397: Meta-plugin skill flattening looks consistent; consider clearer “not found” behavior.
Catching onlyENOENTand logging is good. One small nit:cp()can also throwENOTDIR(bad manifest path shape); that currently gets rethrown (fine), but you may want a slightly clearer error message to help users fix the manifest.tests/commands/sync.test.ts (1)
2-2: Good regression test for orphaned flattened skill cleanup; consider hardening around missing.cursor/skills.
If the implementation ever removes the.cursor/skillsdirectory itself,readdir(skillsDir)will throw and the failure message may be unclear.Also applies to: 714-766
src/helpers/claude-code-config.ts (1)
11-13: Return-type tightening is nice; ensure Claude marketplace loading remains best-effort if parsing ever throws.
If any upstream caller expects “Claude config errors shouldn’t break sync/install”, consider catching and warning at the integration boundary.Also applies to: 315-347
src/config/loader.ts (1)
156-179: Avoidas neverin Claude marketplace reducer; keep types aligned withreadClaudeCodeMarketplaces().
The cast makes it easy to accidentally feed invalid shapes without the compiler catching it.Example direction
- Change
accumulateClaudeMarketplaceto acceptClaudeKnownMarketplacesentries (or the concrete entry type), notunknown.- Or drop the helper and keep the explicit, checked loop (if conflict handling is required here too).
src/commands/plugin-uninstall.ts (1)
177-230: Flattened-skill cleanup: collision mode leaves orphaned dirs; consider exact-name deletion + useforce: true.
- In the
hasConflictingNamesbranch, you only deleteentry.name === flattenedPrefix, but flattened skill dirs are typicallyflattenedPrefix + '-' + <skill>(so nothing gets removed in the collision case).- You’re also using
rm(..., { recursive: true })(noforce), whilesrc/commands/sync.tsusesforce: truefor similar cleanup.Proposed adjustments (safer delete + consistent rm)
- await rm(join(skillsDir, entry.name), { recursive: true }); + await rm(join(skillsDir, entry.name), { recursive: true, force: true });- await rm(installedPath, { recursive: true }); + await rm(installedPath, { recursive: true, force: true });For the collision case, the robust fix is to delete only the exact flattened directory names derived from the plugin’s declared skills (manifest or plugin.json), instead of relying on prefix matching.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
package.jsonsrc/commands/plugin-install.tssrc/commands/plugin-uninstall.tssrc/commands/sync.tssrc/config/loader.tssrc/helpers/claude-code-config.tssrc/helpers/sync-strategy.tstests/commands/plugin-install.test.tstests/commands/plugin-uninstall.test.tstests/commands/sync.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>orts-node <file>for running scripts
Bun automatically loads .env files, so don't use the dotenv package
UseBun.serve()with built-in WebSocket, HTTPS, and routes support instead ofexpress
Usebun:sqlitefor SQLite database access instead ofbetter-sqlite3
UseBun.redisfor Redis client instead ofioredis
UseBun.sqlfor Postgres database access instead ofpgorpostgres.js
Use the built-inWebSocketAPI instead of thewspackage
PreferBun.fileovernode:fs's readFile/writeFile methods
UseBun.$for shell command execution instead of execa
Files:
tests/commands/plugin-uninstall.test.tssrc/config/loader.tssrc/commands/plugin-uninstall.tstests/commands/plugin-install.test.tstests/commands/sync.test.tssrc/commands/plugin-install.tssrc/helpers/sync-strategy.tssrc/commands/sync.tssrc/helpers/claude-code-config.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun testinstead ofjestorvitestfor running tests
Files:
tests/commands/plugin-uninstall.test.tstests/commands/plugin-install.test.tstests/commands/sync.test.ts
**/*.{html,ts,tsx,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor building
Files:
tests/commands/plugin-uninstall.test.tssrc/config/loader.tssrc/commands/plugin-uninstall.tstests/commands/plugin-install.test.tstests/commands/sync.test.tssrc/commands/plugin-install.tssrc/helpers/sync-strategy.tssrc/commands/sync.tssrc/helpers/claude-code-config.ts
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Import .css files directly in TypeScript/JavaScript frontend files and Bun's CSS bundler will handle bundling
Files:
tests/commands/plugin-uninstall.test.tssrc/config/loader.tssrc/commands/plugin-uninstall.tstests/commands/plugin-install.test.tstests/commands/sync.test.tssrc/commands/plugin-install.tssrc/helpers/sync-strategy.tssrc/commands/sync.tssrc/helpers/claude-code-config.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun --hot <file.ts>for hot module reloading during development
Files:
tests/commands/plugin-uninstall.test.tssrc/config/loader.tssrc/commands/plugin-uninstall.tstests/commands/plugin-install.test.tstests/commands/sync.test.tssrc/commands/plugin-install.tssrc/helpers/sync-strategy.tssrc/commands/sync.tssrc/helpers/claude-code-config.ts
🧠 Learnings (8)
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `bun test` instead of `jest` or `vitest` for running tests
Applied to files:
tests/commands/plugin-install.test.tstests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `Bun.$` for shell command execution instead of execa
Applied to files:
tests/commands/plugin-install.test.tstests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to {package.json,package-lock.json,yarn.lock,pnpm-lock.yaml} : Use `bun install` instead of `npm install`, `yarn install`, or `pnpm install`
Applied to files:
tests/commands/plugin-install.test.tstests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Prefer `Bun.file` over `node:fs`'s readFile/writeFile methods
Applied to files:
tests/commands/plugin-install.test.tstests/commands/sync.test.tssrc/helpers/sync-strategy.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `bun <file>` instead of `node <file>` or `ts-node <file>` for running scripts
Applied to files:
tests/commands/plugin-install.test.tstests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Bun automatically loads .env files, so don't use the dotenv package
Applied to files:
tests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to package.json : Use `bun run <script>` instead of `npm run <script>`, `yarn run <script>`, or `pnpm run <script>`
Applied to files:
tests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.ts : Use `bun --hot <file.ts>` for hot module reloading during development
Applied to files:
tests/commands/sync.test.ts
🧬 Code graph analysis (6)
src/config/loader.ts (2)
src/schema.ts (1)
MarketplaceSource(96-96)src/helpers/claude-code-config.ts (3)
convertClaudeMarketplaceToAIPM(315-347)isClaudeCodeInstalled(96-103)readClaudeCodeMarketplaces(109-138)
src/commands/plugin-uninstall.ts (2)
src/constants.ts (2)
DIR_CURSOR(4-4)DIR_SKILLS(20-20)src/errors.ts (1)
isFileNotFoundError(5-7)
tests/commands/sync.test.ts (1)
src/commands/sync.ts (1)
sync(54-257)
src/helpers/sync-strategy.ts (4)
src/helpers/fs.ts (2)
fileExists(26-33)ensureDir(16-24)src/errors.ts (2)
isFileNotFoundError(5-7)DirectoryNotFoundError(48-56)src/constants.ts (2)
DIR_AIPM_NAMESPACE(9-9)DIR_SKILLS(20-20)src/helpers/io.ts (1)
defaultIO(79-79)
src/commands/sync.ts (3)
src/constants.ts (2)
DIR_SKILLS(20-20)DIR_AIPM_NAMESPACE(9-9)src/helpers/io.ts (1)
defaultIO(79-79)src/helpers/hooks-merger.ts (1)
mergeHooks(85-90)
src/helpers/claude-code-config.ts (1)
src/schema.ts (2)
MarketplaceSource(96-96)MarketplaceSourceSchema(5-10)
🪛 GitHub Actions: CI
tests/commands/plugin-install.test.ts
[error] 29-29: Test failed due to plugin installation error: Claude Code is not installed.
src/commands/plugin-install.ts
[error] 65-65: Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like 'claude/anthropic-agent-skills'.
🪛 GitHub Check: Lint and Test
tests/commands/plugin-install.test.ts
[failure] 51-51: error: expect(received).toThrow(expected)
Expected substring: "Marketplace 'claude/nonexistent-marketplace' not found in Claude Code"
Received message: "Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like 'claude/nonexistent-marketplace'."
at <anonymous> (/home/runner/work/aipm/aipm/tests/commands/plugin-install.test.ts:51:52)
at <anonymous> (/home/runner/work/aipm/aipm/tests/commands/plugin-install.test.ts:45:66)
src/commands/plugin-install.ts
[failure] 65-65: error: Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like 'claude/anthropic-agent-skills'.
at pluginInstall (/home/runner/work/aipm/aipm/src/commands/plugin-install.ts:65:21)
at async <anonymous> (/home/runner/work/aipm/aipm/tests/commands/plugin-install.test.ts:64:13)
[failure] 65-65: error: Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like 'claude/anthropic-agent-skills'.
at pluginInstall (/home/runner/work/aipm/aipm/src/commands/plugin-install.ts:65:21)
at async <anonymous> (/home/runner/work/aipm/aipm/tests/commands/plugin-install.test.ts:29:13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (7)
src/commands/plugin-uninstall.ts (1)
1-1: Import change looks fine (enables directory scan cleanup).tests/commands/plugin-install.test.ts (1)
2-2: Minor: import change is fine.src/commands/sync.ts (2)
1-12: Constants + fs imports for cleanup make sense.
127-134: Early return moved after cleanup is aligned with the stated objective.src/commands/plugin-install.ts (3)
91-123: Validation/dry-run gating looks good.
134-149: Meta-plugin fallback to plugin.json path is a solid pragmatic fix.
150-161: Persisting installed plugin into .aipm/config.json after sync is sensible.
yordis
force-pushed
the
yordis/fix-skills
branch
from
77d1375 to
2263232
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/commands/plugin-uninstall.ts:
- Around line 150-220: The flattened skill cleanup logic is duplicated; extract
it into a helper function (e.g., cleanupFlattenedSkills) that accepts skillsDir,
marketplaceName, pluginName, config, pluginId (cmd.pluginId), and optional
pluginPath and returns the number of deleted dirs; move the try/readdir loop,
flattenedPrefix calculation, otherPluginsInMarketplace and hasConflictingNames
checks, skillsToDelete population, and rm/delete logic into that function and
call it from both branches (marketplace config present and missing), ensuring
behavior stays the same when hasConflictingNames is true (only delete from
skillsToDelete if pluginPath present, otherwise be conservative) and preserve
FileNotFound handling via isFileNotFoundError.
In @tests/commands/plugin-install.test.ts:
- Around line 20-70: Tests assume Claude Code is present but CI doesn't have it,
causing pluginInstall tests to fail; fix by mocking Claude Code presence in the
test suite: in tests/commands/plugin-install.test.ts add setup/teardown that
makes the code path that checks installation detect Claude Code as installed
(e.g., set and later delete an env flag or stub the detection helper used by
pluginInstall), and update the marketplace-not-found assertion to tolerate the
real CLI-not-installed message if you choose not to mock (or prefer mocking,
keep original assertion); target the pluginInstall call and the test
setup/teardown around the "Claude Code marketplace integration (zero-config)"
describe block.
🧹 Nitpick comments (3)
tests/commands/plugin-uninstall.test.ts (1)
915-954: Placeholder test provides no validation.This test documents the prefix collision fix but contains only a placeholder assertion
expect(true).toBe(true)with a comment stating "real test is in sync.test.ts". This provides no actual validation and can be misleading.Consider one of these approaches:
- Remove the placeholder test entirely since the behavior is tested in sync.test.ts
- Implement a minimal validation that exercises the uninstall path with conflicting plugin names
- Convert to a documentation comment in the actual test file if the intent is purely documentary
If you choose to keep this test, it should at minimum:
- Create a fixture with conflicting plugin names (e.g., "plugin@local" and "plugin-extra@local")
- Create flattened skill directories for both
- Uninstall one plugin with
removeFiles: true- Verify that only the correct plugin's skills were deleted
Example skeleton:
test('should avoid prefix matching when plugin names are ambiguous', async () => { // Setup marketplace with plugin and plugin-extra // ... create flattened skills for both ... await pluginUninstall({ pluginId: 'plugin@local', cwd: testDir, removeFiles: true, }); // Verify plugin skills removed but plugin-extra skills remain expect(await fileExists(join(testDir, '.cursor', 'skills', 'aipm-local-plugin-skill1'))).toBe(false); expect(await fileExists(join(testDir, '.cursor', 'skills', 'aipm-local-plugin-extra-skill1'))).toBe(true); });src/helpers/sync-strategy.ts (1)
280-309: Documented collision risk could be prevented proactively.The comments correctly identify a theoretical collision risk when using hyphens as delimiters:
marketplace "a-b"+plugin "c-d"→"aipm-a-b-c-d"marketplace "a"+plugin "b-c-d"→"aipm-a-b-c-d"(collision!)While the mitigation strategies mentioned are reasonable, consider preventing collisions proactively rather than waiting for production issues.
🛡️ Proposed collision-proof implementation
Option 1: Add a short hash suffix
function getFlattenedSkillName( namespace: string, marketplaceName: string, pluginName: string, skillPath?: string, ): string { const parts = [namespace, marketplaceName.replace(/\//g, '-'), pluginName]; if (skillPath) { parts.push(skillPath.replace(/\//g, '-')); } - return parts.join('-'); + const baseName = parts.join('-'); + // Add 6-char hash to prevent collisions (e.g., "aipm-a-b-c-d-a1b2c3") + const hash = require('crypto') + .createHash('sha256') + .update(parts.join('/')) // Use original separator for hashing + .digest('hex') + .substring(0, 6); + return `${baseName}-${hash}`; }Option 2: Use a safer delimiter
function getFlattenedSkillName( namespace: string, marketplaceName: string, pluginName: string, skillPath?: string, ): string { - const parts = [namespace, marketplaceName.replace(/\//g, '-'), pluginName]; + // Use double-underscore as delimiter (unlikely in names, visually distinct) + const parts = [namespace, marketplaceName.replace(/\//g, '__'), pluginName]; if (skillPath) { - parts.push(skillPath.replace(/\//g, '-')); + parts.push(skillPath.replace(/\//g, '__')); } - return parts.join('-'); + return parts.join('__'); }Option 2 is simpler and more readable. Example output:
aipm__a-b__c-dvsaipm__a__b-c-d(no collision).src/commands/plugin-install.ts (1)
125-129: Consider removing potentially unreachable error check.This error check appears to be unreachable:
- In dry-run mode with git/url sources (where
pluginPathmight be null), execution already returned at line 122- In all other cases,
shouldValidateis true, ensuringpluginPathis set at line 99♻️ Simplify by removing dead code
If this check is confirmed unreachable, consider removing it:
- // For git/url sources in dry-run, we skip validation so pluginPath won't be set - // In this case, we can't perform the sync, so we already returned above - if (!pluginPath) { - throw new Error(`Plugin path not resolved for '${pluginName}'`); - } -Alternatively, if you want to keep it as a defensive assertion, add a comment explaining the edge case it protects against.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
package.jsonsrc/commands/plugin-install.tssrc/commands/plugin-uninstall.tssrc/commands/sync.tssrc/config/loader.tssrc/helpers/claude-code-config.tssrc/helpers/sync-strategy.tstests/commands/plugin-install.test.tstests/commands/plugin-uninstall.test.tstests/commands/sync.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/config/loader.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>orts-node <file>for running scripts
Bun automatically loads .env files, so don't use the dotenv package
UseBun.serve()with built-in WebSocket, HTTPS, and routes support instead ofexpress
Usebun:sqlitefor SQLite database access instead ofbetter-sqlite3
UseBun.redisfor Redis client instead ofioredis
UseBun.sqlfor Postgres database access instead ofpgorpostgres.js
Use the built-inWebSocketAPI instead of thewspackage
PreferBun.fileovernode:fs's readFile/writeFile methods
UseBun.$for shell command execution instead of execa
Files:
src/commands/plugin-uninstall.tstests/commands/plugin-install.test.tstests/commands/plugin-uninstall.test.tssrc/helpers/claude-code-config.tssrc/commands/plugin-install.tstests/commands/sync.test.tssrc/commands/sync.tssrc/helpers/sync-strategy.ts
**/*.{html,ts,tsx,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor building
Files:
src/commands/plugin-uninstall.tstests/commands/plugin-install.test.tstests/commands/plugin-uninstall.test.tssrc/helpers/claude-code-config.tssrc/commands/plugin-install.tstests/commands/sync.test.tssrc/commands/sync.tssrc/helpers/sync-strategy.ts
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Import .css files directly in TypeScript/JavaScript frontend files and Bun's CSS bundler will handle bundling
Files:
src/commands/plugin-uninstall.tstests/commands/plugin-install.test.tstests/commands/plugin-uninstall.test.tssrc/helpers/claude-code-config.tssrc/commands/plugin-install.tstests/commands/sync.test.tssrc/commands/sync.tssrc/helpers/sync-strategy.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun --hot <file.ts>for hot module reloading during development
Files:
src/commands/plugin-uninstall.tstests/commands/plugin-install.test.tstests/commands/plugin-uninstall.test.tssrc/helpers/claude-code-config.tssrc/commands/plugin-install.tstests/commands/sync.test.tssrc/commands/sync.tssrc/helpers/sync-strategy.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun testinstead ofjestorvitestfor running tests
Files:
tests/commands/plugin-install.test.tstests/commands/plugin-uninstall.test.tstests/commands/sync.test.ts
🧠 Learnings (8)
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `bun test` instead of `jest` or `vitest` for running tests
Applied to files:
tests/commands/plugin-install.test.tstests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `Bun.$` for shell command execution instead of execa
Applied to files:
tests/commands/plugin-install.test.tstests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to {package.json,package-lock.json,yarn.lock,pnpm-lock.yaml} : Use `bun install` instead of `npm install`, `yarn install`, or `pnpm install`
Applied to files:
tests/commands/plugin-install.test.tstests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Prefer `Bun.file` over `node:fs`'s readFile/writeFile methods
Applied to files:
tests/commands/plugin-install.test.tstests/commands/sync.test.tssrc/helpers/sync-strategy.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `bun <file>` instead of `node <file>` or `ts-node <file>` for running scripts
Applied to files:
tests/commands/plugin-install.test.tstests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Bun automatically loads .env files, so don't use the dotenv package
Applied to files:
tests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to package.json : Use `bun run <script>` instead of `npm run <script>`, `yarn run <script>`, or `pnpm run <script>`
Applied to files:
tests/commands/sync.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.ts : Use `bun --hot <file.ts>` for hot module reloading during development
Applied to files:
tests/commands/sync.test.ts
🧬 Code graph analysis (5)
src/commands/plugin-uninstall.ts (2)
src/constants.ts (2)
DIR_CURSOR(4-4)DIR_SKILLS(20-20)src/errors.ts (1)
isFileNotFoundError(5-7)
src/helpers/claude-code-config.ts (1)
src/schema.ts (2)
MarketplaceSource(96-96)MarketplaceSourceSchema(5-10)
src/commands/plugin-install.ts (10)
src/helpers/plugin.ts (2)
parsePluginId(13-21)isMetaPlugin(45-58)src/config/loader.ts (2)
loadPluginsConfig(87-142)loadClaudeCodeMarketplaces(168-179)src/helpers/io.ts (1)
defaultIO(79-79)src/helpers/claude-code-config.ts (1)
isClaudeCodeInstalled(96-103)src/helpers/git.ts (1)
resolveMarketplacePath(34-103)src/helpers/marketplace.ts (3)
loadMarketplaceManifest(78-86)getMarketplaceType(13-15)resolvePluginPath(237-263)src/helpers/fs.ts (1)
fileExists(26-33)src/constants.ts (1)
DIR_CURSOR(4-4)src/helpers/sync-strategy.ts (3)
syncMetaPluginToCursor(363-400)syncPluginToCursor(39-119)formatSyncResult(405-431)src/helpers/aipm-config.ts (2)
loadTargetConfig(99-105)saveConfig(110-116)
src/commands/sync.ts (3)
src/constants.ts (2)
DIR_SKILLS(20-20)DIR_AIPM_NAMESPACE(9-9)src/helpers/io.ts (1)
defaultIO(79-79)src/helpers/hooks-merger.ts (1)
mergeHooks(85-90)
src/helpers/sync-strategy.ts (4)
src/helpers/fs.ts (2)
fileExists(26-33)ensureDir(16-24)src/errors.ts (2)
isFileNotFoundError(5-7)DirectoryNotFoundError(48-56)src/constants.ts (2)
DIR_AIPM_NAMESPACE(9-9)DIR_SKILLS(20-20)src/helpers/io.ts (1)
defaultIO(79-79)
🪛 GitHub Actions: CI
tests/commands/plugin-install.test.ts
[error] 29-29: Test failed due to plugin-install error: Claude Code is not installed.
src/commands/plugin-install.ts
[error] 65-65: Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like 'claude/anthropic-agent-skills'.
🪛 GitHub Check: Lint and Test
tests/commands/plugin-install.test.ts
[failure] 51-51: error: expect(received).toThrow(expected)
Expected substring: "Marketplace 'claude/nonexistent-marketplace' not found in Claude Code"
Received message: "Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like 'claude/nonexistent-marketplace'."
at <anonymous> (/home/runner/work/aipm/aipm/tests/commands/plugin-install.test.ts:51:52)
at <anonymous> (/home/runner/work/aipm/aipm/tests/commands/plugin-install.test.ts:45:66)
src/commands/plugin-install.ts
[failure] 65-65: error: Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like 'claude/anthropic-agent-skills'.
at pluginInstall (/home/runner/work/aipm/aipm/src/commands/plugin-install.ts:65:21)
at async <anonymous> (/home/runner/work/aipm/aipm/tests/commands/plugin-install.test.ts:64:13)
[failure] 65-65: error: Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like 'claude/anthropic-agent-skills'.
at pluginInstall (/home/runner/work/aipm/aipm/src/commands/plugin-install.ts:65:21)
at async <anonymous> (/home/runner/work/aipm/aipm/tests/commands/plugin-install.test.ts:29:13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (20)
src/commands/plugin-uninstall.ts (2)
1-1: LGTM - Import addition for directory listing.The
readdirimport is necessary for the new flattened skill cleanup logic.
165-168: Verify collision detection logic for edge cases.The collision detection checks if
otherPluginName.startsWith(pluginName)(lines 167, 261). This correctly identifies when one plugin name is a prefix of another. However, consider these edge cases:
pluginName = "my-plugin"andotherPluginName = "my-plugin-extra"→ Correctly detected ✓pluginName = "my-plugin"andotherPluginName = "my-plugin"→ Already filtered byid !== cmd.pluginId✓pluginName = "my"andotherPluginName = "my-plugin"→ Correctly detected as conflict ✓The logic appears correct. The additional check
&& otherPluginName !== pluginNameis redundant since the filter already excludesid !== cmd.pluginId, but it serves as a defensive safeguard.Consider adding a test case that verifies the behavior with deeply nested prefix scenarios (e.g., "plugin", "plugin-a", "plugin-a-extra") to ensure the conflict detection handles all cases correctly.
Also applies to: 259-262
src/helpers/sync-strategy.ts (3)
1-1: LGTM - Import addition for type safety.Adding the
Direnttype import improves type safety for the directory entry handling in the new flattened skill logic.
358-396: LGTM - Flattened skill syncing for meta-plugins.The updated
syncMetaPluginToCursorcorrectly:
- Flattens skill paths using the new
getFlattenedSkillNamehelper- Strips the
skills/prefix before flattening- Handles missing skills gracefully with informational logging
- Uses recursive copy for directory structures
The implementation is consistent with the overall flattening strategy and addresses the Claude Code limitation referenced in issue #10238.
78-111: The mixed flattening approach is correct and well-justified.The implementation properly handles two legitimate use cases:
- Skill subdirectories in
skills/→ flattened to.cursor/skills/aipm-marketplace-plugin-skill/(addresses Claude Code's limitation with nested directories)- Files directly in
skills/→ nested at.cursor/skills/aipm/marketplace/plugin/file(backwards compatibility)Both patterns are actively tested (see
createPluginWithTypesin sync.test.ts which creates a file directly inskills/root) and cleanup logic explicitly handles both flattened and nested paths. The approach is clearly documented in comments and solves real technical constraints of Claude Code.tests/commands/sync.test.ts (2)
2-2: LGTM - Import addition for test validation.The
readdirimport is necessary for the new test that validates flattened skill directory cleanup.
714-766: LGTM - Comprehensive test for orphaned skill cleanup.This test provides good coverage for the flattened skill cleanup scenario:
- Creates a plugin with multiple skills ✓
- Syncs to create flattened skill directories ✓
- Verifies flattened skills exist using
readdirand prefix filtering ✓- Disables the plugin ✓
- Re-syncs and verifies cleanup removed orphaned directories ✓
The test correctly validates the expected behavior and aligns with the PR objectives.
src/commands/sync.ts (4)
1-12: LGTM! Imports support the new cleanup functionality.The additions of
readdirandDIR_SKILLSare used appropriately in the cleanup logic below.
103-124: LGTM! Cleanup strategy correctly isolates AIPM artifacts.The cleanup logic properly removes only AIPM-managed directories (those starting with
aipm-and the nestedaipmnamespace), preserving any user-created skill directories. Running this cleanup before the early return ensures orphaned skill directories are removed even when no plugins are enabled, which aligns with the PR objectives.
127-134: LGTM! Early return correctly positioned after cleanup.Moving the early return to occur after the cleanup ensures that disabled plugins' artifacts are removed even when no plugins remain enabled. The hook cleanup with an empty array also ensures hooks from disabled plugins are properly removed.
136-136: LGTM! Log message appropriately placed.The sync announcement now appears after cleanup operations, providing a clear sequence of operations in the user-visible output.
src/helpers/claude-code-config.ts (2)
11-12: LGTM! Type imports support enhanced type safety.The addition of
MarketplaceSourcetype andMarketplaceSourceSchemaenables runtime validation of marketplace conversions, ensuring type safety when converting Claude Code marketplace format to AIPM format.
315-346: LGTM! Enhanced type safety with runtime validation.Wrapping all return values with
MarketplaceSourceSchema.parse()adds defensive runtime validation. Since the function constructs these objects internally, validation should always succeed. If validation fails, theZodErrorwill propagate to the caller, which is appropriate behavior for detecting schema mismatches during development.src/commands/plugin-install.ts (7)
4-8: LGTM! Imports support Claude Code integration.The new imports enable loading and checking Claude Code marketplaces, expanding the plugin installation capabilities to support both AIPM and Claude Code sources.
28-38: LGTM! Early check prevents duplicate installations.Checking if the plugin is already installed before performing any expensive operations (cloning, validation) is a good optimization that provides immediate feedback to the user.
40-68: LGTM! Marketplace merging with clear error messages.The marketplace loading and merging logic properly integrates Claude Code sources. The error messages are context-aware and helpful, distinguishing between Claude Code not being installed versus a marketplace truly not existing.
Note on merge precedence:
merge({}, claudeMarketplaces, aipmMarketplaces)means AIPM config takes precedence over Claude Code marketplaces when there are naming conflicts. This is the expected behavior since users' explicit AIPM configuration should override auto-discovered Claude Code settings.Note regarding pipeline failures: The test failures at line 65 are expected behavior—the code correctly throws an error when Claude Code marketplaces are requested but Claude Code is not installed. This is not a code issue.
70-86: LGTM! Optimized marketplace path resolution.The conditional path resolution correctly handles directory sources without calling
resolveMarketplacePath, avoiding unnecessary overhead while still properly handling git/url sources that require cloning or downloading.
88-118: LGTM! Smart validation optimization for dry-run mode.The conditional validation logic correctly optimizes dry-run performance by skipping expensive operations (cloning git repositories) for git/url sources while still fully validating directory sources which are immediately available.
134-148: LGTM! Proper meta-plugin detection and handling.The logic correctly distinguishes between meta-plugins with skills defined in the marketplace manifest versus those with skills in plugin.json, choosing the appropriate sync strategy for each case. This addresses the PR objective of adding a fallback for meta-plugins where skills are defined in plugin.json.
150-161: LGTM! Plugin installation tracked in project config.The plugin installation is correctly saved to the project config (
.aipm/config.json) rather than the local config. This ensures installed plugins are tracked in the project-level configuration, which is appropriate for theplugin-installcommand.The clear success messages provide good user feedback about both the installation and the config update.
|
|
||
| // Remove flattened skill directories for this plugin | ||
| // Pattern: .cursor/skills/aipm-<marketplace>-<plugin>[-subskill] | ||
| // Must avoid prefix collisions when plugin names are prefixes of others | ||
| // (e.g., "my-plugin" shouldn't match "my-plugin-extra") | ||
| const skillsDir = join(cwd, DIR_CURSOR, DIR_SKILLS); | ||
| try { | ||
| const entries = await readdir(skillsDir, { withFileTypes: true }); | ||
| const flattenedPrefix = `aipm-${marketplaceName.replace(/\//g, '-')}-${pluginName}`; | ||
|
|
||
| // Check for conflicting plugin names in this marketplace | ||
| const otherPluginsInMarketplace = Object.keys(config.plugins).filter( | ||
| (id) => id.endsWith(`@${marketplaceName}`) && id !== cmd.pluginId, | ||
| ); | ||
|
|
||
| const hasConflictingNames = otherPluginsInMarketplace.some((id) => { | ||
| const otherPluginName = id.substring(0, id.lastIndexOf('@')); | ||
| return otherPluginName.startsWith(pluginName) && otherPluginName !== pluginName; | ||
| }); | ||
|
|
||
| // When conflicts exist, we need exact skill information to avoid false positives | ||
| // Read the plugin's skills directory to get the exact list | ||
| const skillsToDelete = new Set<string>(); | ||
|
|
||
| if (hasConflictingNames && pluginPath) { | ||
| // Load exact skill information from plugin directory | ||
| const skillsSourcePath = join(pluginPath, 'skills'); | ||
| try { | ||
| const skillEntries = await readdir(skillsSourcePath, { withFileTypes: true }); | ||
| for (const skillEntry of skillEntries) { | ||
| if (skillEntry.isDirectory()) { | ||
| // Reconstruct the exact flattened name for this skill | ||
| const flatSkillName = `${flattenedPrefix}-${skillEntry.name}`; | ||
| skillsToDelete.add(flatSkillName); | ||
| } | ||
| } | ||
| } catch { | ||
| // No skills directory - plugin has no skills | ||
| } | ||
| } | ||
|
|
||
| for (const entry of entries) { | ||
| if (!entry.isDirectory()) continue; | ||
|
|
||
| let shouldDelete = false; | ||
|
|
||
| if (hasConflictingNames) { | ||
| // With conflicts, only delete skills we explicitly found in the plugin directory | ||
| shouldDelete = skillsToDelete.has(entry.name); | ||
| } else { | ||
| // No conflicts - safe to use prefix matching | ||
| shouldDelete = entry.name === flattenedPrefix || entry.name.startsWith(flattenedPrefix + '-'); | ||
| } | ||
|
|
||
| if (shouldDelete) { | ||
| try { | ||
| await rm(join(skillsDir, entry.name), { recursive: true }); | ||
| deletedCount++; | ||
| } catch (error) { | ||
| if (!isFileNotFoundError(error)) { | ||
| throw error; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } catch (error) { | ||
| // Ignore errors reading skills directory | ||
| if (!isFileNotFoundError(error)) { | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Significant code duplication in flattened skill cleanup logic.
The flattened skill directory cleanup logic (lines 150-220 and 245-294) is nearly identical in both branches (marketplace config present vs. missing). This duplication makes maintenance more difficult and violates the DRY principle.
♻️ Proposed refactor to extract common cleanup logic
Extract the cleanup logic into a helper function that can be called from both branches:
/**
* Removes flattened skill directories for a plugin
* @param skillsDir - Path to .cursor/skills directory
* @param marketplaceName - Name of the marketplace
* @param pluginName - Name of the plugin being uninstalled
* @param config - Full config containing all plugins
* @param pluginId - The plugin ID being uninstalled
* @param pluginPath - Optional path to plugin directory (for reading exact skills)
* @returns Number of deleted directories
*/
async function cleanupFlattenedSkills(
skillsDir: string,
marketplaceName: string,
pluginName: string,
config: any,
pluginId: string,
pluginPath?: string,
): Promise<number> {
let deletedCount = 0;
try {
const entries = await readdir(skillsDir, { withFileTypes: true });
const flattenedPrefix = `aipm-${marketplaceName.replace(/\//g, '-')}-${pluginName}`;
// Check for conflicting plugin names in this marketplace
const otherPluginsInMarketplace = Object.keys(config.plugins).filter(
(id) => id.endsWith(`@${marketplaceName}`) && id !== pluginId,
);
const hasConflictingNames = otherPluginsInMarketplace.some((id) => {
const otherPluginName = id.substring(0, id.lastIndexOf('@'));
return otherPluginName.startsWith(pluginName) && otherPluginName !== pluginName;
});
// When conflicts exist, we need exact skill information
const skillsToDelete = new Set<string>();
if (hasConflictingNames && pluginPath) {
const skillsSourcePath = join(pluginPath, 'skills');
try {
const skillEntries = await readdir(skillsSourcePath, { withFileTypes: true });
for (const skillEntry of skillEntries) {
if (skillEntry.isDirectory()) {
const flatSkillName = `${flattenedPrefix}-${skillEntry.name}`;
skillsToDelete.add(flatSkillName);
}
}
} catch {
// No skills directory - plugin has no skills
}
}
for (const entry of entries) {
if (!entry.isDirectory()) continue;
let shouldDelete = false;
if (hasConflictingNames) {
if (pluginPath) {
// With conflicts and pluginPath, only delete skills we explicitly found
shouldDelete = skillsToDelete.has(entry.name);
} else {
// With conflicts but no pluginPath, be conservative and don't delete
shouldDelete = false;
}
} else {
// No conflicts - safe to use prefix matching
shouldDelete = entry.name === flattenedPrefix || entry.name.startsWith(flattenedPrefix + '-');
}
if (shouldDelete) {
try {
await rm(join(skillsDir, entry.name), { recursive: true });
deletedCount++;
} catch (error) {
if (!isFileNotFoundError(error)) {
throw error;
}
}
}
}
} catch (error) {
// Ignore errors reading skills directory
if (!isFileNotFoundError(error)) {
throw error;
}
}
return deletedCount;
}Then use it in both branches:
}
} else {
for (const subdir of PLUGIN_SUBDIRS) {
const installedPath = join(cwd, DIR_CURSOR, subdir, DIR_AIPM_NAMESPACE, marketplaceName, pluginName);
try {
await rm(installedPath, { recursive: true });
deletedCount++;
} catch (error) {
if (!isFileNotFoundError(error)) {
throw error;
}
}
}
- // Remove flattened skill directories for this plugin
- // Pattern: .cursor/skills/aipm-<marketplace>-<plugin>[-subskill]
- // Must avoid prefix collisions when plugin names are prefixes of others
- const skillsDir = join(cwd, DIR_CURSOR, DIR_SKILLS);
- try {
- const entries = await readdir(skillsDir, { withFileTypes: true });
- const flattenedPrefix = `aipm-${marketplaceName.replace(/\//g, '-')}-${pluginName}`;
-
- // Check for conflicting plugin names in this marketplace
- const otherPluginsInMarketplace = Object.keys(config.plugins).filter(
- (id) => id.endsWith(`@${marketplaceName}`) && id !== cmd.pluginId,
- );
-
- const hasConflictingNames = otherPluginsInMarketplace.some((id) => {
- const otherPluginName = id.substring(0, id.lastIndexOf('@'));
- return otherPluginName.startsWith(pluginName) && otherPluginName !== pluginName;
- });
-
- // When conflicts exist, we need exact skill information to avoid false positives
- // Read the plugin's skills directory to get the exact list
- const skillsToDelete = new Set<string>();
-
- if (hasConflictingNames && pluginPath) {
- // Load exact skill information from plugin directory
- const skillsSourcePath = join(pluginPath, 'skills');
- try {
- const skillEntries = await readdir(skillsSourcePath, { withFileTypes: true });
- for (const skillEntry of skillEntries) {
- if (skillEntry.isDirectory()) {
- const flatSkillName = `${flattenedPrefix}-${skillEntry.name}`;
- skillsToDelete.add(flatSkillName);
- }
- }
- } catch {
- // No skills directory - plugin has no skills
- }
- }
-
- for (const entry of entries) {
- if (!entry.isDirectory()) continue;
-
- let shouldDelete = false;
-
- if (hasConflictingNames) {
- // With conflicts, only delete skills we explicitly found in the plugin directory
- shouldDelete = skillsToDelete.has(entry.name);
- } else {
- // No conflicts - safe to use prefix matching
- shouldDelete = entry.name === flattenedPrefix || entry.name.startsWith(flattenedPrefix + '-');
- }
-
- if (shouldDelete) {
- try {
- await rm(join(skillsDir, entry.name), { recursive: true });
- deletedCount++;
- } catch (error) {
- if (!isFileNotFoundError(error)) {
- throw error;
- }
- }
- }
- }
- } catch (error) {
- // Ignore errors reading skills directory
- if (!isFileNotFoundError(error)) {
- throw error;
- }
- }
+ const skillsDir = join(cwd, DIR_CURSOR, DIR_SKILLS);
+ const cleanedCount = await cleanupFlattenedSkills(
+ skillsDir,
+ marketplaceName,
+ pluginName,
+ config,
+ cmd.pluginId,
+ pluginPath,
+ );
+ deletedCount += cleanedCount;
}Apply the same pattern in the marketplace config missing branch (lines 245-294).
Also applies to: 245-294
🤖 Prompt for AI Agents
In @src/commands/plugin-uninstall.ts around lines 150 - 220, The flattened skill
cleanup logic is duplicated; extract it into a helper function (e.g.,
cleanupFlattenedSkills) that accepts skillsDir, marketplaceName, pluginName,
config, pluginId (cmd.pluginId), and optional pluginPath and returns the number
of deleted dirs; move the try/readdir loop, flattenedPrefix calculation,
otherPluginsInMarketplace and hasConflictingNames checks, skillsToDelete
population, and rm/delete logic into that function and call it from both
branches (marketplace config present and missing), ensuring behavior stays the
same when hasConflictingNames is true (only delete from skillsToDelete if
pluginPath present, otherwise be conservative) and preserve FileNotFound
handling via isFileNotFoundError.
2263232 to
76886b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @tests/commands/plugin-install.test.ts:
- Around line 45-54: The failing test is hitting the "Claude Code not installed"
branch; mock isClaudeCodeInstalled() to return true and mock
loadClaudeCodeMarketplaces() to return a small marketplaces array (including an
entry for 'claude/nonexistent-marketplace' or other test fixtures) before
calling pluginInstall in the 'should error if marketplace not found in Claude
Code' test so the code path reaches the marketplace-not-found error; ensure you
restore/reset mocks after the test.
- Around line 56-69: The failing test "should work from any directory without
project setup" calls pluginInstall and assumes Claude Code is present; mirror
the mocking used in the earlier tests by stubbing the function that
detects/ensures Claude Code (e.g., isClaudeCodeInstalled or
ensureClaudeInstalled / detectClaudeBinary) to return success/no-op for this
test before invoking pluginInstall, then restore the stub after the test; keep
the rest of the test assertions (fileExists on join(testDir, '.cursor',
'skills')) unchanged so the test no longer depends on actual Claude Code being
installed.
- Around line 21-43: Tests in the "Claude Code marketplace integration
(zero-config)" block fail because Claude Code detection isn't mocked and
isClaudeCodeInstalled()/loadClaudeCodeMarketplaces() return false/empty; fix by
either calling setupTestEnvironment() in the beforeEach for that describe block
to create the .claude/plugins directory used by loadClaudeCodeMarketplaces(), or
add spies to mock isClaudeCodeInstalled() to return true and
readClaudeCodeMarketplaces()/loadClaudeCodeMarketplaces() to return the expected
marketplace data (see plugin-search.test.ts spy pattern); update the tests at
lines where pluginInstall(...) is invoked so the environment or spies are
applied before pluginInstall runs.
🧹 Nitpick comments (3)
tests/commands/plugin-uninstall.test.ts (1)
915-954: Consider removing or improving this placeholder test.This test contains only a placeholder assertion (
expect(true).toBe(true)) and doesn't validate any behavior. While the comments are helpful for documentation, a test that always passes without checking anything can:
- Give false confidence about test coverage
- Add maintenance burden without value
- Confuse future developers
Consider one of these alternatives:
- Remove the test and move the explanatory comments to the source code where the fix is implemented
- Add minimal validation (e.g., verify the config structure or that uninstall doesn't throw)
- Mark the test as skipped with
.skipand add a TODO comment♻️ Option 1: Remove test, keep comments in source
Move the explanation to
src/commands/plugin-uninstall.tsnear the conflict detection logic (around line 160-168).♻️ Option 2: Add minimal validation
- expect(true).toBe(true); // Placeholder - real test is in sync.test.ts + // Verify that uninstall doesn't throw when conflicting names exist + const options = { + pluginId: 'plugin@local', + cwd: testDir, + removeFiles: true, + }; + + // Should complete without error despite the conflict + await expect(pluginUninstall(options)).resolves.not.toThrow(); + + const config = JSON.parse(await Bun.file(pluginsPath).text()); + expect(config.plugins['plugin@local']).toBeUndefined(); + expect(config.plugins['plugin-extra@local']).toBeDefined();src/config/loader.ts (1)
156-163: Use proper typing instead ofas nevercast.The parameter type is declared as
[string, unknown]and then cast toas neverwhen callingconvertClaudeMarketplaceToAIPM. This bypasses TypeScript's type safety.Since
claudeCodeMarketplaceshas typeClaudeKnownMarketplaces(which isRecord<string, ClaudeMarketplaceObjectEntry>), the entries will be[string, ClaudeMarketplaceObjectEntry][].♻️ Proposed fix with proper typing
+import type { ClaudeMarketplaceObjectEntry } from '../helpers/claude-code-config'; + function accumulateClaudeMarketplace( acc: Record<string, MarketplaceSource>, - [marketplaceName, marketplaceConfig]: [string, unknown], + [marketplaceName, marketplaceConfig]: [string, ClaudeMarketplaceObjectEntry], ): Record<string, MarketplaceSource> { const prefixedName = `claude/${marketplaceName}`; - acc[prefixedName] = convertClaudeMarketplaceToAIPM(marketplaceName, marketplaceConfig as never); + acc[prefixedName] = convertClaudeMarketplaceToAIPM(marketplaceName, marketplaceConfig); return acc; }src/helpers/sync-strategy.ts (1)
280-309: Name collision risk acknowledged but not mitigated.The function documentation thoroughly explains the theoretical collision risk when using hyphens as delimiters. While the risk is mitigated by centralized marketplace management and naming conventions, consider adding:
- A validation check to warn or reject marketplace/plugin names containing hyphens
- A hash suffix as suggested in the comment to eliminate collision risk entirely
💡 Example validation approach
Add validation in the marketplace loading or plugin install flow:
function validateNameForFlattening(marketplaceName: string, pluginName: string): void { if (marketplaceName.includes('-') || pluginName.includes('-')) { console.warn( `⚠️ Warning: Marketplace '${marketplaceName}' or plugin '${pluginName}' contains hyphens. ` + `This may cause name collisions in flattened skill directories.` ); } }Or add a deterministic hash suffix to eliminate collisions:
import { createHash } from 'node:crypto'; function getFlattenedSkillName( namespace: string, marketplaceName: string, pluginName: string, skillPath?: string, ): string { const parts = [namespace, marketplaceName.replace(/\//g, '-'), pluginName]; if (skillPath) { parts.push(skillPath.replace(/\//g, '-')); } const base = parts.join('-'); // Add short hash suffix to prevent collisions const hash = createHash('sha256') .update(`${namespace}:${marketplaceName}:${pluginName}:${skillPath || ''}`) .digest('hex') .slice(0, 8); return `${base}-${hash}`; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
package.jsonsrc/commands/plugin-install.tssrc/commands/plugin-uninstall.tssrc/commands/sync.tssrc/config/loader.tssrc/helpers/claude-code-config.tssrc/helpers/sync-strategy.tstests/commands/plugin-install.test.tstests/commands/plugin-uninstall.test.tstests/commands/sync.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- tests/commands/sync.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Usebun <file>instead ofnode <file>orts-node <file>for running scripts
Bun automatically loads .env files, so don't use the dotenv package
UseBun.serve()with built-in WebSocket, HTTPS, and routes support instead ofexpress
Usebun:sqlitefor SQLite database access instead ofbetter-sqlite3
UseBun.redisfor Redis client instead ofioredis
UseBun.sqlfor Postgres database access instead ofpgorpostgres.js
Use the built-inWebSocketAPI instead of thewspackage
PreferBun.fileovernode:fs's readFile/writeFile methods
UseBun.$for shell command execution instead of execa
Files:
src/commands/plugin-uninstall.tstests/commands/plugin-install.test.tstests/commands/plugin-uninstall.test.tssrc/helpers/sync-strategy.tssrc/config/loader.tssrc/helpers/claude-code-config.tssrc/commands/plugin-install.tssrc/commands/sync.ts
**/*.{html,ts,tsx,css}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun build <file.html|file.ts|file.css>instead ofwebpackoresbuildfor building
Files:
src/commands/plugin-uninstall.tstests/commands/plugin-install.test.tstests/commands/plugin-uninstall.test.tssrc/helpers/sync-strategy.tssrc/config/loader.tssrc/helpers/claude-code-config.tssrc/commands/plugin-install.tssrc/commands/sync.ts
**/*.{tsx,jsx,ts,js}
📄 CodeRabbit inference engine (CLAUDE.md)
Import .css files directly in TypeScript/JavaScript frontend files and Bun's CSS bundler will handle bundling
Files:
src/commands/plugin-uninstall.tstests/commands/plugin-install.test.tstests/commands/plugin-uninstall.test.tssrc/helpers/sync-strategy.tssrc/config/loader.tssrc/helpers/claude-code-config.tssrc/commands/plugin-install.tssrc/commands/sync.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun --hot <file.ts>for hot module reloading during development
Files:
src/commands/plugin-uninstall.tstests/commands/plugin-install.test.tstests/commands/plugin-uninstall.test.tssrc/helpers/sync-strategy.tssrc/config/loader.tssrc/helpers/claude-code-config.tssrc/commands/plugin-install.tssrc/commands/sync.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
bun testinstead ofjestorvitestfor running tests
Files:
tests/commands/plugin-install.test.tstests/commands/plugin-uninstall.test.ts
🧠 Learnings (4)
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use `bun test` instead of `jest` or `vitest` for running tests
Applied to files:
tests/commands/plugin-install.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use `Bun.$` for shell command execution instead of execa
Applied to files:
tests/commands/plugin-install.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to {package.json,package-lock.json,yarn.lock,pnpm-lock.yaml} : Use `bun install` instead of `npm install`, `yarn install`, or `pnpm install`
Applied to files:
tests/commands/plugin-install.test.ts
📚 Learning: 2025-11-24T17:14:51.342Z
Learnt from: CR
Repo: TrogonStack/aipm PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:14:51.342Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Prefer `Bun.file` over `node:fs`'s readFile/writeFile methods
Applied to files:
tests/commands/plugin-install.test.tssrc/helpers/sync-strategy.ts
🧬 Code graph analysis (7)
src/commands/plugin-uninstall.ts (2)
src/constants.ts (2)
DIR_CURSOR(4-4)DIR_SKILLS(20-20)src/errors.ts (1)
isFileNotFoundError(5-7)
tests/commands/plugin-install.test.ts (2)
src/commands/plugin-install.ts (1)
pluginInstall(22-181)src/helpers/fs.ts (1)
fileExists(26-33)
src/helpers/sync-strategy.ts (4)
src/helpers/fs.ts (2)
fileExists(26-33)ensureDir(16-24)src/errors.ts (2)
isFileNotFoundError(5-7)DirectoryNotFoundError(48-56)src/constants.ts (2)
DIR_AIPM_NAMESPACE(9-9)DIR_SKILLS(20-20)src/helpers/io.ts (1)
defaultIO(79-79)
src/config/loader.ts (2)
src/schema.ts (1)
MarketplaceSource(96-96)src/helpers/claude-code-config.ts (3)
convertClaudeMarketplaceToAIPM(315-347)isClaudeCodeInstalled(96-103)readClaudeCodeMarketplaces(109-138)
src/helpers/claude-code-config.ts (1)
src/schema.ts (2)
MarketplaceSource(96-96)MarketplaceSourceSchema(5-10)
src/commands/plugin-install.ts (5)
src/helpers/plugin.ts (1)
parsePluginId(13-21)src/config/loader.ts (1)
loadPluginsConfig(87-142)src/helpers/claude-code-config.ts (1)
isClaudeCodeInstalled(96-103)src/helpers/marketplace.ts (2)
loadMarketplaceManifest(78-86)getMarketplaceType(13-15)src/helpers/aipm-config.ts (1)
loadTargetConfig(99-105)
src/commands/sync.ts (3)
src/constants.ts (2)
DIR_SKILLS(20-20)DIR_AIPM_NAMESPACE(9-9)src/helpers/io.ts (1)
defaultIO(79-79)src/helpers/hooks-merger.ts (1)
mergeHooks(85-90)
🪛 GitHub Actions: CI
tests/commands/plugin-install.test.ts
[error] 29-29: Failed to install plugin: Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like 'claude/anthropic-agent-skills'.
src/commands/plugin-install.ts
[error] 75-75: Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like 'claude/anthropic-agent-skills'.
🪛 GitHub Check: Lint and Test
tests/commands/plugin-install.test.ts
[failure] 51-51: error: expect(received).toThrow(expected)
Expected substring: "Marketplace 'claude/nonexistent-marketplace' not found in Claude Code"
Received message: "Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like 'claude/nonexistent-marketplace'."
at <anonymous> (/home/runner/work/aipm/aipm/tests/commands/plugin-install.test.ts:51:52)
at <anonymous> (/home/runner/work/aipm/aipm/tests/commands/plugin-install.test.ts:45:66)
src/commands/plugin-install.ts
[failure] 75-75: error: Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like 'claude/anthropic-agent-skills'.
at pluginInstall (/home/runner/work/aipm/aipm/src/commands/plugin-install.ts:75:21)
at async <anonymous> (/home/runner/work/aipm/aipm/tests/commands/plugin-install.test.ts:64:13)
[failure] 75-75: error: Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like 'claude/anthropic-agent-skills'.
at pluginInstall (/home/runner/work/aipm/aipm/src/commands/plugin-install.ts:75:21)
at async <anonymous> (/home/runner/work/aipm/aipm/tests/commands/plugin-install.test.ts:29:13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (15)
src/helpers/claude-code-config.ts (2)
11-12: LGTM! Type safety improvement.The addition of
MarketplaceSourcetype and the function signature change properly enforces type safety. The use ofMarketplaceSourceSchema.parse()ensures runtime validation of the converted marketplace configuration.Also applies to: 318-318
321-346: Validation wrapping is correctly applied across all return paths.All three code paths properly wrap their return values with
MarketplaceSourceSchema.parse(), ensuring runtime validation. The constructed objects align with the expected schema structure.src/config/loader.ts (1)
168-179: LGTM! Clean separation of Claude Code marketplace loading.The function properly handles the case where Claude Code isn't installed and uses a clean reduce pattern to build the marketplace map. The separation from the main config loading makes the code more modular.
src/commands/plugin-uninstall.ts (2)
151-220: Well-designed prefix collision handling with exact matching fallback.The implementation correctly addresses the prefix collision issue:
- Detects when plugin names are prefixes of other plugin names in the same marketplace
- Falls back to exact skill name matching when conflicts exist (reading from the plugin's skills directory)
- Uses safe prefix matching when no conflicts are detected
The hyphen boundary in the prefix check (
flattenedPrefix + '-') correctly prevents false matches between plugins like "my" and "my-plugin".
246-294: Appropriately conservative cleanup when marketplace config is missing.When the marketplace configuration is unavailable, the code correctly:
- Applies the same conflict detection logic
- Takes a conservative approach by not deleting anything when conflicts exist (since exact skill information can't be determined)
- Uses prefix matching only when safe (no conflicts)
This prevents accidental deletion while still providing cleanup when unambiguous.
src/commands/sync.ts (2)
103-124: LGTM! Cleanup runs before early return to remove orphaned skills.The cleanup routine correctly:
- Runs before checking for enabled plugins, ensuring orphaned skills from disabled plugins are removed
- Handles both flattened (
aipm-*) and nested (aipm/) skill directory structures- Gracefully handles the case where the skills directory doesn't exist yet
The "clear and rebuild" strategy ensures no stale artifacts remain.
127-136: Control flow correctly prioritizes cleanup over early return.Moving the enabled plugins check and log message to after the cleanup block ensures that orphaned flattened skill directories are cleaned up even when no plugins are currently enabled. This aligns with the PR objective to address Claude Code's limitations with nested directories.
src/helpers/sync-strategy.ts (3)
36-36: Good documentation of the flattening constraint.The comment clearly explains the flattening requirement and links to the relevant issue.
78-111: Well-structured skills syncing with flattening.The logic correctly:
- Flattens skill subdirectories to avoid Claude Code nesting limitations
- Maintains backwards compatibility for files directly in skills/
- Handles errors appropriately with non-fatal behavior when skills directory doesn't exist
358-396: Meta-plugin syncing correctly implements flattening.The implementation:
- Consistently applies the flattening strategy
- Handles missing skills gracefully with informative logging rather than failing
- Uses appropriate recursive copy for skill directories
src/commands/plugin-install.ts (5)
30-40: Well-implemented zero-config mode.The optional AIPM initialization correctly enables zero-config Claude Code usage while maintaining backward compatibility with AIPM-initialized projects.
44-48: Appropriate already-installed check.The check correctly short-circuits when a plugin is already installed, and only applies when AIPM is initialized (zero-config mode doesn't track installation state).
50-78: Excellent error handling with helpful user guidance.The error messages effectively:
- Distinguish between Claude Code not installed vs. marketplace not found
- List available marketplaces for discoverability
- Provide actionable guidance for users
80-103: Well-structured marketplace resolution.The conditional logic correctly:
- Handles different marketplace source types (git/url vs. directory)
- Optimizes dry-run by skipping expensive git operations
- Always validates directory sources since they're readily available
144-167: Robust meta-plugin handling and config persistence.The implementation correctly:
- Distinguishes between meta-plugins with skills in manifest vs. plugin.json
- Falls back gracefully to regular plugin sync when needed
- Conditionally saves config only when AIPM is initialized
- Provides clear logging for both zero-config and AIPM-initialized modes
| test('should install a plugin from Claude Code without init', async () => { | ||
| // AIPM auto-discovers Claude Code marketplaces without requiring configuration | ||
| // No .aipm/ directory needed | ||
| const options = { | ||
| pluginId: 'custom-plugin@local', | ||
| pluginId: 'document-skills@claude/anthropic-agent-skills', | ||
| cwd: testDir, | ||
| }; | ||
|
|
||
| await pluginInstall(options); | ||
|
|
||
| const commandsPath = join(testDir, '.cursor', 'commands', 'aipm', 'local', 'custom-plugin', 'test.md'); | ||
| expect(await fileExists(commandsPath)).toBe(true); | ||
| }); | ||
| }); | ||
| // Verify skills were synced to .cursor/ | ||
| const skillsDir = join(testDir, '.cursor', 'skills'); | ||
| expect(await fileExists(skillsDir)).toBe(true); | ||
|
|
||
| describe('validation', () => { | ||
| test('should reject empty pluginId', async () => { | ||
| const options = { | ||
| pluginId: '', | ||
| cwd: testDir, | ||
| }; | ||
| // Verify each skill gets its own flattened directory (no nesting) | ||
| // Addresses Claude Code limitation with nested skill directories | ||
| const xlsxSkillDir = join(skillsDir, 'aipm-claude-anthropic-agent-skills-document-skills-xlsx'); | ||
| expect(await fileExists(xlsxSkillDir)).toBe(true); | ||
|
|
||
| await expect(pluginInstall(options)).rejects.toThrow(); | ||
| // Verify internal structure is preserved within the skill | ||
| const skillMd = join(xlsxSkillDir, 'SKILL.md'); | ||
| expect(await fileExists(skillMd)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "plugin-install.test.ts" | head -5Repository: TrogonStack/aipm
Length of output: 99
🏁 Script executed:
git ls-files --others --exclude-standard
git ls-files tests/ | grep -E "\.(ts|tsx|js|jsx)$" | head -20Repository: TrogonStack/aipm
Length of output: 806
🏁 Script executed:
cat -n ./tests/commands/plugin-install.test.ts | head -100Repository: TrogonStack/aipm
Length of output: 3175
🏁 Script executed:
wc -l ./tests/commands/plugin-install.test.tsRepository: TrogonStack/aipm
Length of output: 102
🏁 Script executed:
find . -type f -name "plugin-install.ts" | grep -v testRepository: TrogonStack/aipm
Length of output: 92
🏁 Script executed:
cat -n ./src/commands/plugin-install.ts | head -80Repository: TrogonStack/aipm
Length of output: 3913
🏁 Script executed:
find . -type f -name "loader.ts" | grep configRepository: TrogonStack/aipm
Length of output: 82
🏁 Script executed:
cat -n ./src/config/loader.ts | head -100Repository: TrogonStack/aipm
Length of output: 4110
🏁 Script executed:
cat -n ./src/config/loader.ts | tail -50Repository: TrogonStack/aipm
Length of output: 1947
🏁 Script executed:
grep -n "loadClaudeCodeMarketplaces" ./src/config/loader.tsRepository: TrogonStack/aipm
Length of output: 160
🏁 Script executed:
grep -r "mock\|Mock" tests/commands/plugin-install.test.tsRepository: TrogonStack/aipm
Length of output: 42
🏁 Script executed:
find tests -name "*.test.ts" -exec grep -l "isClaudeCodeInstalled\|loadClaudeCodeMarketplaces" {} \;Repository: TrogonStack/aipm
Length of output: 137
🏁 Script executed:
cat -n tests/helpers/claude-code-config.test.ts | head -80Repository: TrogonStack/aipm
Length of output: 3388
🏁 Script executed:
cat -n tests/commands/plugin-search.test.ts | head -100Repository: TrogonStack/aipm
Length of output: 4068
🏁 Script executed:
grep -n "spyOn\|mock" tests/commands/plugin-search.test.ts | head -20Repository: TrogonStack/aipm
Length of output: 522
🏁 Script executed:
cat -n tests/commands/plugin-search.test.ts | grep -A 30 "spyOn"Repository: TrogonStack/aipm
Length of output: 3020
🏁 Script executed:
find tests -name "*test-support*" -o -name "*setup*" | grep -v node_modulesRepository: TrogonStack/aipm
Length of output: 89
🏁 Script executed:
cat -n tests/helpers/test-support.ts 2>/dev/null | head -80Repository: TrogonStack/aipm
Length of output: 3139
🏁 Script executed:
cat -n tests/commands/plugin-install.test.tsRepository: TrogonStack/aipm
Length of output: 3175
🏁 Script executed:
grep -n "setupTestEnvironment\|spyOn\|mock" tests/commands/plugin-install.test.tsRepository: TrogonStack/aipm
Length of output: 42
🏁 Script executed:
find . -name "*.yml" -o -name "*.yaml" | grep -E "(github|ci|workflow)" | head -10Repository: TrogonStack/aipm
Length of output: 185
🏁 Script executed:
cat -n .github/workflows/*.yml 2>/dev/null | head -150Repository: TrogonStack/aipm
Length of output: 3906
🏁 Script executed:
cat -n tests/commands/plugin-install.test.ts | sed -n '45,70p'Repository: TrogonStack/aipm
Length of output: 1152
🏁 Script executed:
grep -r "Claude Code is not installed" src/ tests/ 2>/dev/nullRepository: TrogonStack/aipm
Length of output: 227
🏁 Script executed:
cat -n tests/commands/claude-code-auto-discovery.test.ts | head -120Repository: TrogonStack/aipm
Length of output: 5012
🏁 Script executed:
cat -n tests/helpers/claude-code-config.ts | grep -A 5 "isClaudeCodeInstalled"Repository: TrogonStack/aipm
Length of output: 127
🏁 Script executed:
cat -n src/helpers/claude-code-config.ts | head -100Repository: TrogonStack/aipm
Length of output: 3759
🏁 Script executed:
cat -n src/helpers/claude-code-config.ts | sed -n '96,110p'Repository: TrogonStack/aipm
Length of output: 598
Critical: Tests fail in CI because Claude Code detection is not mocked.
All three tests in the "Claude Code marketplace integration (zero-config)" describe block (lines 21-69) assume Claude Code is installed but don't mock the detection. Since the test environment doesn't create a .claude/plugins directory, isClaudeCodeInstalled() returns false, and loadClaudeCodeMarketplaces() returns an empty object. The tests then fail with:
Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like 'claude/anthropic-agent-skills'.
Follow the pattern used in claude-code-auto-discovery.test.ts by adding setupTestEnvironment() to the beforeEach hook and creating the required .claude/plugins directory structure, or use spyOn() to mock isClaudeCodeInstalled() and readClaudeCodeMarketplaces() (as shown in plugin-search.test.ts line 318).
🧰 Tools
🪛 GitHub Actions: CI
[error] 29-29: Failed to install plugin: Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like 'claude/anthropic-agent-skills'.
🤖 Prompt for AI Agents
In @tests/commands/plugin-install.test.ts around lines 21 - 43, Tests in the
"Claude Code marketplace integration (zero-config)" block fail because Claude
Code detection isn't mocked and
isClaudeCodeInstalled()/loadClaudeCodeMarketplaces() return false/empty; fix by
either calling setupTestEnvironment() in the beforeEach for that describe block
to create the .claude/plugins directory used by loadClaudeCodeMarketplaces(), or
add spies to mock isClaudeCodeInstalled() to return true and
readClaudeCodeMarketplaces()/loadClaudeCodeMarketplaces() to return the expected
marketplace data (see plugin-search.test.ts spy pattern); update the tests at
lines where pluginInstall(...) is invoked so the environment or spies are
applied before pluginInstall runs.
| test('should error if marketplace not found in Claude Code', async () => { | ||
| const options = { | ||
| pluginId: 'plugin-2@local', | ||
| pluginId: 'some-plugin@claude/nonexistent-marketplace', | ||
| cwd: testDir, | ||
| }; | ||
|
|
||
| await pluginInstall(options); | ||
|
|
||
| expect(await fileExists(installed1Path)).toBe(true); | ||
|
|
||
| const config = JSON.parse(await Bun.file(pluginsPath).text()); | ||
| expect(config.plugins['plugin-1@local']).toBeDefined(); | ||
| expect(config.plugins['plugin-2@local']).toBeDefined(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('nested plugin directories', () => { | ||
| test('should install plugin from nested directory when using simple name', async () => { | ||
| const marketplaceDir = join(testDir, 'marketplace'); | ||
| await mkdir(marketplaceDir, { recursive: true }); | ||
|
|
||
| // Create plugin in nested directory: available-plugins/code-review-ai | ||
| const nestedPluginDir = join(marketplaceDir, 'available-plugins', 'code-review-ai'); | ||
| await mkdir(join(nestedPluginDir, '.claude-plugin'), { recursive: true }); | ||
| await writeFile( | ||
| join(nestedPluginDir, '.claude-plugin', 'plugin.json'), | ||
| JSON.stringify({ | ||
| name: 'code-review-ai', | ||
| version: '1.2.0', | ||
| author: 'Test Author', | ||
| }), | ||
| ); | ||
|
|
||
| // Create a command file | ||
| await mkdir(join(nestedPluginDir, 'commands'), { recursive: true }); | ||
| await writeFile(join(nestedPluginDir, 'commands', 'review.md'), '# Review command'); | ||
|
|
||
| const pluginsPath = join(testDir, '.aipm', 'config.json'); | ||
| const aipmDir = join(testDir, '.aipm'); | ||
| await mkdir(aipmDir, { recursive: true }); | ||
| await writeFile( | ||
| pluginsPath, | ||
| JSON.stringify({ | ||
| marketplaces: { | ||
| local: { source: 'directory', path: './marketplace' }, | ||
| }, | ||
| plugins: {}, | ||
| }), | ||
| await expect(pluginInstall(options)).rejects.toThrow( | ||
| "Marketplace 'claude/nonexistent-marketplace' not found in Claude Code", | ||
| ); | ||
|
|
||
| const options = { | ||
| pluginId: 'code-review-ai@local', | ||
| cwd: testDir, | ||
| }; | ||
|
|
||
| await pluginInstall(options); | ||
|
|
||
| // Verify plugin was installed | ||
| const config = JSON.parse(await Bun.file(pluginsPath).text()); | ||
| expect(config.plugins['code-review-ai@local']).toBeDefined(); | ||
| expect(config.plugins['code-review-ai@local'].enabled).toBe(true); | ||
|
|
||
| // Verify files were synced to .cursor/ | ||
| const commandsPath = join(testDir, '.cursor', 'commands', 'aipm', 'local', 'code-review-ai', 'review.md'); | ||
| expect(await fileExists(commandsPath)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Test assertion fails due to missing Claude Code installation.
The test expects the error message "Marketplace 'claude/nonexistent-marketplace' not found in Claude Code", but receives "Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like 'claude/nonexistent-marketplace'." instead.
This is the same root cause as the previous test - Claude Code is not installed in the CI environment, so the error handling takes a different path.
Apply the same mocking approach suggested in the previous comment to ensure isClaudeCodeInstalled() returns true and loadClaudeCodeMarketplaces() returns test data.
🧰 Tools
🪛 GitHub Check: Lint and Test
[failure] 51-51: error: expect(received).toThrow(expected)
Expected substring: "Marketplace 'claude/nonexistent-marketplace' not found in Claude Code"
Received message: "Claude Code is not installed. Install Claude Code to use Claude Code marketplaces like 'claude/nonexistent-marketplace'."
at <anonymous> (/home/runner/work/aipm/aipm/tests/commands/plugin-install.test.ts:51:52)
at <anonymous> (/home/runner/work/aipm/aipm/tests/commands/plugin-install.test.ts:45:66)
🤖 Prompt for AI Agents
In @tests/commands/plugin-install.test.ts around lines 45 - 54, The failing test
is hitting the "Claude Code not installed" branch; mock isClaudeCodeInstalled()
to return true and mock loadClaudeCodeMarketplaces() to return a small
marketplaces array (including an entry for 'claude/nonexistent-marketplace' or
other test fixtures) before calling pluginInstall in the 'should error if
marketplace not found in Claude Code' test so the code path reaches the
marketplace-not-found error; ensure you restore/reset mocks after the test.
| test('should work from any directory without project setup', async () => { | ||
| // Zero-config means no .aipm/ directory or initialization needed | ||
| const options = { | ||
| pluginId: 'simple-plugin@claude/test-marketplace', | ||
| pluginId: 'example-skills@claude/anthropic-agent-skills', | ||
| cwd: testDir, | ||
| }; | ||
|
|
||
| // Should succeed without any config files or aipm init | ||
| await pluginInstall(options); | ||
|
|
||
| const config = JSON.parse(await Bun.file(pluginsPath).text()); | ||
| expect(config.plugins['simple-plugin@claude/test-marketplace']).toBeDefined(); | ||
| expect(config.plugins['simple-plugin@claude/test-marketplace'].enabled).toBe(true); | ||
| // Verify the plugin was installed to .cursor/ | ||
| const skillsDir = join(testDir, '.cursor', 'skills'); | ||
| expect(await fileExists(skillsDir)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Test fails due to missing Claude Code installation.
This test has the same issue as the previous two - it assumes Claude Code is installed in the CI environment. Apply the same mocking strategy to make the test environment-agnostic.
🤖 Prompt for AI Agents
In @tests/commands/plugin-install.test.ts around lines 56 - 69, The failing test
"should work from any directory without project setup" calls pluginInstall and
assumes Claude Code is present; mirror the mocking used in the earlier tests by
stubbing the function that detects/ensures Claude Code (e.g.,
isClaudeCodeInstalled or ensureClaudeInstalled / detectClaudeBinary) to return
success/no-op for this test before invoking pluginInstall, then restore the stub
after the test; keep the rest of the test assertions (fileExists on
join(testDir, '.cursor', 'skills')) unchanged so the test no longer depends on
actual Claude Code being installed.
61f3d2b to
328738c
Compare
Each skill now gets a completely flat directory name (e.g., aipm-marketplace-plugin-skill) with no nesting, addressing Claude Code's limitation with nested directories (issue #10238). Also fixed fallback for meta-plugins where skills are defined in plugin.json rather than marketplace manifest. Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
yordis/fix-skills
branch
from
328738c to
6ccabf3
Compare
Each skill now gets a completely flat directory name (e.g., aipm-marketplace-plugin-skill) with no nesting, addressing Claude Code's limitation with nested directories (issue #10238). Also fixed fallback for meta-plugins where skills are defined in plugin.json rather than marketplace manifest.