Plugin marketplace for OpenSpec#1239
Conversation
Add an OpenSpec change proposal for a plugin/marketplace system that keeps the core lean and hosts specialized "engines" as out-of-process delegated plugins, with OpenLore as the inaugural marketplace listing and reference plugin (code-first spec generation from existing codebases). Proposes six new capabilities (plugin-manifest, plugin-resolution, plugin-runtime, plugin-contribution, cli-plugin, plugin-marketplace) and modifications to global-config, cli-init, cli-update, and telemetry. Grounded in today's architecture: mirrors the schema-resolver precedence, reuses the passthrough config and managed-file conventions, and adds no required runtime dependency or Node floor change. Addresses Fission-AI#453, Fission-AI#436, Fission-AI#1081, Fission-AI#650, Fission-AI#1074, Fission-AI#1231, Fission-AI#667, Fission-AI#780, Fission-AI#724 and discussion Fission-AI#634. Validated with `openspec validate --strict`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete plugin marketplace system including design rationale, comprehensive specifications, eight core plugin modules, CLI wiring, lifecycle integration, extensive unit and integration tests, and user documentation. The system enables OpenSpec to be extended via npm packages that run as subprocess delegations under reserved namespaces while contributing static skill templates during initialization and updates. ChangesOpenSpec Plugin Marketplace: Design, Specification, and Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
openspec/changes/add-plugin-marketplace/specs/plugin-runtime/spec.md (1)
13-13: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueMinor style improvement: Remove "of" for conciseness.
Line 13 can be simplified from "all of the plugin's subcommands" to "all the plugin's subcommands" for more concise phrasing, consistent with technical specification style.
Suggested wording
- **AND** all of the plugin's subcommands SHALL be addressed as `openspec lore <subcommand>` + **AND** all the plugin's subcommands SHALL be addressed as `openspec lore <subcommand>`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/add-plugin-marketplace/specs/plugin-runtime/spec.md` at line 13, In the specification file, locate the phrase "all of the plugin's subcommands" on line 13 and remove the word "of" to make it read "all the plugin's subcommands" instead. This simplifies the phrasing to be more concise and consistent with technical specification style conventions while maintaining the same meaning.openspec/changes/add-plugin-marketplace/tasks.md (2)
49-49: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueClarify "print install command" in the
plugin addtask.Task 6.4 says to "print install command (or run behind
--install)". This is ambiguous: does it mean the npm install instruction (e.g.,npm install -D openlore), or theopenspec plugin addcommand itself? A clearer phrasing would improve clarity for implementation (e.g., "print the npm install instruction" or "display the installation steps").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/add-plugin-marketplace/tasks.md` at line 49, The task description for `openspec plugin add` contains ambiguous language in the phrase "print install command (or run behind `--install`)". Clarify this task requirement by specifying exactly what should be printed: either the npm install instruction (e.g., "npm install -D package-name") or the openspec plugin add command/installation steps. Replace the ambiguous phrasing with explicit language that makes it clear to implementers what output is expected when the install command is printed versus executed with the --install flag.
59-59: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winDefine "graceful handling" for unknown registry format versions.
Task 7.2 says to implement "graceful handling of unknown formats" in the registry loader. This is underspecified: does it skip the problematic entry with a warning, abort the entire registry load with an error, or silently ignore? Define the expected behavior clearly to avoid ambiguity during implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/add-plugin-marketplace/tasks.md` at line 59, Task 7.2 in the registry loader implementation is underspecified regarding how to handle unknown registry format versions. Replace the vague phrase "graceful handling of unknown formats" with a concrete definition of the expected behavior, specifying whether the loader should skip problematic entries with a warning, abort the entire registry load with an error, or silently ignore unknown format versions. Include the chosen behavior strategy in the task description to provide clear implementation guidance.openspec/changes/add-plugin-marketplace/specs/plugin-marketplace/spec.md (1)
33-45: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winSeparate documentation and authoring requirements from technical spec; move to a docs/onboarding or submission guidance section.
Lines 33–45 contain three "requirements" that are not technical specifications but rather documentation obligations:
- Line 33–38: Documentation SHALL present OpenLore as a path (docs task, not spec)
- Line 40–45: Documentation SHALL describe submission (docs/author-guide task, not spec)
These belong in a separate documentation or onboarding spec, or in implementation guidance outside the technical spec suite. The plugin-marketplace spec should focus narrowly on the registry data structure, discovery mechanics (search, info, add), and unsupported-version handling.
Recommend creating a separate spec or section (e.g.,
specs/marketplace-docs/orPLUGIN_ONBOARDING.md) for documentation and author submission requirements.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openspec/changes/add-plugin-marketplace/specs/plugin-marketplace/spec.md` around lines 33 - 45, The spec currently includes documentation and authoring requirements (Code-first onboarding path via OpenLore and Registry submission guidance) that are not technical specifications but rather documentation obligations. Remove these two requirement sections from the plugin-marketplace spec file and relocate them to a separate documentation or onboarding guidance spec (such as a new specs/marketplace-docs or PLUGIN_ONBOARDING.md file). Keep the plugin-marketplace spec focused narrowly on technical aspects: the registry data structure, discovery mechanics (search, info, add), and unsupported-version handling. This separation ensures the technical spec remains focused on system behavior while documentation and author submission requirements are captured in appropriate guidance documents.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openspec/changes/add-plugin-marketplace/tasks.md`:
- Around line 4-6: The reserved namespace list in task 1.4 is missing the "spec"
command which currently exists in the CLI, creating a potential collision risk
for plugins. Add "spec" to the reserved namespaces list in task 1.4.
Additionally, clarify the intended scope of the reserved list by deciding
whether it should include only currently existing CLI commands (which would
exclude "plugin" since it will be implemented in task 6) or also include planned
future commands (which would keep "plugin"). Update the task description to
reflect this decision and ensure consistency with the actual CLI command
registry.
- Around line 45-54: Locate Task 5.2 which references composing with
`unify-template-generation-pipeline` in the tasks document. Update the task
description to explicitly clarify that `unify-template-generation-pipeline` is
an in-flight change that does not yet exist in
`src/core/shared/skill-generation.ts`. Either add a note stating that template
merging should integrate with this pipeline once it becomes available, or
provide a concrete interim approach that allows Task 5.2 to proceed
independently without depending on this future change.
---
Nitpick comments:
In `@openspec/changes/add-plugin-marketplace/specs/plugin-marketplace/spec.md`:
- Around line 33-45: The spec currently includes documentation and authoring
requirements (Code-first onboarding path via OpenLore and Registry submission
guidance) that are not technical specifications but rather documentation
obligations. Remove these two requirement sections from the plugin-marketplace
spec file and relocate them to a separate documentation or onboarding guidance
spec (such as a new specs/marketplace-docs or PLUGIN_ONBOARDING.md file). Keep
the plugin-marketplace spec focused narrowly on technical aspects: the registry
data structure, discovery mechanics (search, info, add), and unsupported-version
handling. This separation ensures the technical spec remains focused on system
behavior while documentation and author submission requirements are captured in
appropriate guidance documents.
In `@openspec/changes/add-plugin-marketplace/specs/plugin-runtime/spec.md`:
- Line 13: In the specification file, locate the phrase "all of the plugin's
subcommands" on line 13 and remove the word "of" to make it read "all the
plugin's subcommands" instead. This simplifies the phrasing to be more concise
and consistent with technical specification style conventions while maintaining
the same meaning.
In `@openspec/changes/add-plugin-marketplace/tasks.md`:
- Line 49: The task description for `openspec plugin add` contains ambiguous
language in the phrase "print install command (or run behind `--install`)".
Clarify this task requirement by specifying exactly what should be printed:
either the npm install instruction (e.g., "npm install -D package-name") or the
openspec plugin add command/installation steps. Replace the ambiguous phrasing
with explicit language that makes it clear to implementers what output is
expected when the install command is printed versus executed with the --install
flag.
- Line 59: Task 7.2 in the registry loader implementation is underspecified
regarding how to handle unknown registry format versions. Replace the vague
phrase "graceful handling of unknown formats" with a concrete definition of the
expected behavior, specifying whether the loader should skip problematic entries
with a warning, abort the entire registry load with an error, or silently ignore
unknown format versions. Include the chosen behavior strategy in the task
description to provide clear implementation guidance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f0992a4f-3b9e-4271-8d04-c94605381574
📒 Files selected for processing (13)
openspec/changes/add-plugin-marketplace/design.mdopenspec/changes/add-plugin-marketplace/proposal.mdopenspec/changes/add-plugin-marketplace/specs/cli-init/spec.mdopenspec/changes/add-plugin-marketplace/specs/cli-plugin/spec.mdopenspec/changes/add-plugin-marketplace/specs/cli-update/spec.mdopenspec/changes/add-plugin-marketplace/specs/global-config/spec.mdopenspec/changes/add-plugin-marketplace/specs/plugin-contribution/spec.mdopenspec/changes/add-plugin-marketplace/specs/plugin-manifest/spec.mdopenspec/changes/add-plugin-marketplace/specs/plugin-marketplace/spec.mdopenspec/changes/add-plugin-marketplace/specs/plugin-resolution/spec.mdopenspec/changes/add-plugin-marketplace/specs/plugin-runtime/spec.mdopenspec/changes/add-plugin-marketplace/specs/telemetry/spec.mdopenspec/changes/add-plugin-marketplace/tasks.md
Corrections from a triple-check against the current codebase: - Config placement fixed. Project-tier enablement lives in the project openspec/config.yaml (committed, team-shared); global config holds only user-level preferences. ProjectConfigSchema is NOT passthrough, so writing the plugins block must be non-destructive and preserve unknown third-party keys (e.g. the openlore block). New config-loading delta + cli-plugin "preserves existing config" scenario capture this; design Context corrected. - Reserved-namespace list completed (was missing `spec`/`help`/hidden `experimental`/`__complete`); now derived from the registered command list. - Added cli-completion delta so plugin namespaces/subcommands surface in completions (tasks referenced it but no spec governed it). - global-config delta refocused to user-level preferences. - Noted AGENTS.md instruction-block surfacing as deferred future work. 12 capability deltas; passes `openspec validate --strict`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…gin` CLI Implements the core of the add-plugin-marketplace change: - Manifest contract (src/core/plugins/manifest.ts): zod schema with passthrough, loads from package.json "openspec" key or openspec.plugin.json, validates and rejects reserved namespaces; invalid manifests disable a plugin, never crash. - Three-tier resolution (resolver.ts): project config -> user dir -> auto-detected node_modules, mirroring the schema resolver; semver compat gating (no new dep — minimal range checker in semver.ts), namespace collision detection, memoized. - Subprocess delegation (runtime.ts): one reserved namespace per plugin; executes via cross-spawn with inherited stdio and propagated exit code; plugin code is never imported. Early-intercept in cli/index.ts forwards `openspec <ns> ...` verbatim (incl. --help) before Commander parses. - Curated registry (registry.ts + schemas/plugins/registry.json): versioned index with OpenLore as the inaugural listing; search/info/add resolve from it. - Config: project plugins block in openspec/config.yaml written non-destructively via the YAML Document API (preserves comments and unknown keys like `openlore`); user-level plugins prefs in global config + schema/key-path validation. - `openspec plugin` group: list, info, add, remove, enable, disable, search. - Telemetry: delegated commands tracked by namespace only (no plugin args). - Completions: `plugin` group added to the command registry (parity restored). Build + lint clean; full suite green except pre-existing env-only zsh-installer failures (unchanged from base). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- semver range satisfier (bounds, caret, tilde, wildcard, prerelease) - manifest validation + loading (both forms, reserved names, version gate, forward-compat passthrough, actionable errors) - resolver (auto-detect on/off, project tier, compat gating, invalid-manifest errors, namespace collisions, missing node_modules, XDG user dir) - config (non-destructive enable preserving openlore block + comments, idempotent enable, targeted disable) - registry (built-in OpenLore listing, search, unsupported-version + malformed rejection) - cli-e2e: auto-detect listing, verbatim delegation, exit-code propagation, enable preserving unknown config keys, registry search 48 tests, all passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- contribution.ts collects skill directories declared by active plugins (each requiring a SKILL.md; malformed ones skipped with a warning), installs them into AI tool skills dirs, and removes them by explicit name. - init: installs active plugins' contributed skills alongside core skills for every selected tool (gated by delivery mode). - update: re-installs active contributed skills and removes skills belonging to disabled/incompatible plugins; commands-only delivery removes them all. Cleanup is by tracked name only — core and user files are never touched. - Tests: contribution unit tests + e2e proving `openspec init --tools claude` installs a plugin-contributed skill. No regressions in init/update suites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
getCommandRegistryWithPlugins() augments the static command registry with active plugin namespaces and their declared subcommands at completion-generation time; incompatible/disabled plugins are excluded, and any resolution error falls back to the static registry so completion never breaks. Wired into completion script generation/install. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- docs/plugins.md: plugin model, `openspec plugin` commands, project config, compatibility, trust model, manifest authoring, registry submission, and the OpenLore code-first onboarding path. - README + customization.md: link to the plugins guide. - tasks.md: check off implemented work; leave honest notes for the two deferred items (interactive init enable-offer; Windows CI runner verification). Final state: build + lint clean, `openspec validate --strict` passes, full suite green except the pre-existing env-only zsh-installer failures (unchanged from base). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
clay-good
changed the title
All 6 CodeRabbit findings (2 actionable + 4 nitpick) on the change docs: - tasks 1.4: reserved namespaces are derived at runtime from Commander's registered command list (always current, includes `plugin` + hidden cmds); `spec` is present. Enumerated names are illustrative, not the source of truth. - tasks 5.2: clarified that `unify-template-generation-pipeline` is a separate in-flight change; documented the implemented standalone interim approach. - tasks 6.4: clarified that `plugin add` prints the npm install instruction by default and only runs it with `--install`. - tasks 7.2: defined unknown-registry-version behavior as fail-closed — reject the whole load with an actionable RegistryError, never partial-parse. - plugin-runtime spec: "all of the" -> "all the". - plugin-marketplace spec: moved the two documentation/authoring requirements into a new focused `plugin-docs` capability so the marketplace spec stays narrowly about registry mechanics (matches the repo's docs-agent-instructions precedent). Proposal capability list updated. `openspec validate add-plugin-marketplace --strict` passes (13 deltas). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @coderabbitai — addressed all 6 findings from the review in Actionable
Nitpicks
Note: this review covered the proposal commit only. The PR has since grown to include the full implementation (engine, CLI, registry, init/update integration, completions) with 56 unit/e2e tests. @coderabbitai review |
|
Kicking off a full review of the PR now, including the implementation, CLI, registry, init/update integration, completions, and the 56 unit/e2e tests. 🐇✨ ✅ Action performedReview finished.
|
`enableProjectPlugin` returns false when there is no openspec/config.yaml to edit (possible after `init --tools none`, cf. Fission-AI#712), but `plugin add`/`enable` previously printed "Enabled" regardless. Now a shared reportEnable() surfaces whether the write happened: it confirms the config write, or explains that the plugin is still active via auto-detect but could not be pinned. Disable path reports the same way. Adds an e2e covering the no-config case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for pushing this forward. I like the overall shape, especially subprocess delegation and keeping plugin code out of the main process.
Requesting changes on one filesystem-safety issue before this gets serious merge consideration: plugin-contributed skill paths are currently trusted as raw paths. skills[].dir and skills[].source accept arbitrary strings, then installContributedSkills() / removeContributedSkill() join them into rmSync/cpSync targets. A manifest can use ../, absolute paths, or nested separators to escape the intended tool skills directory or plugin package root.
Please tighten this before merge:
- validate
skills[].diras a single safe directory name, not a path - validate
skills[].sourceas a relative path inside the plugin package, rejecting absolute paths and.. - resolve and enforce containment before both copy and delete
- add regression tests for
../, absolute paths, nested paths, and Windows separators
The broader plugin marketplace direction still needs founder/product review around auto-detect trust, registry curation, namespace ownership, and OpenLore endorsement, but the path escape is the concrete blocker I would fix first.
Addresses alfred-openspec's CHANGES_REQUESTED: plugin `skills[].dir` and `skills[].source` were trusted as raw paths and joined into cpSync/rmSync targets, so a manifest could escape the tool skills dir or package root via `../`, absolute paths, or separators. Defense-in-depth: - Manifest validation now rejects unsafe skill paths: `dir` must be a single safe segment (no separators, no "."/".."), `source` must be a relative path inside the package (no absolute/drive/`..`). New isSafeSkillDirName / isSafeSkillSource helpers. - contribution.ts re-checks containment before every copy and delete: resolved source must be inside the package; resolved install/remove target must be inside the tool skills dir. Unsafe entries are skipped, never executed. - collectKnownPluginSkillDirs only tracks safe names (these feed deletion). Tests: manifest rejects `../`, separators, absolute, Windows-drive, and backslash-traversal paths; contribution layer refuses to install/remove targets that escape even when handed a crafted object. +16 tests (73 plugin tests pass). Spec: new path-safety requirement in plugin-contribution; task 5.6. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@alfred-openspec thank you — you're right, that was a real path-traversal hole. Fixed in Manifest validation (reject at the door)
Filesystem-op enforcement (containment, belt-and-suspenders)
So even a crafted object that bypassed manifest validation cannot escape. Regression tests (+16): manifest rejects I also added a path-safety requirement to the Noted on the broader product questions (auto-detect trust, registry curation, namespace ownership, OpenLore endorsement) — those are founder/product calls; the proposal flags them as open questions in |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/update.ts (1)
158-186:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude plugin skill work in the update plan before the up-to-date return.
Plugin contribution state is computed after the early return, and install/remove only runs for
toolsToUpdate. Enabling or disabling a plugin can therefore leave skills stale whileopenspec updatereports everything up to date. Collect plugin state before the return and update configured tools when active/known plugin skill dirs exist.🐛 Suggested direction
- if (!this.force && toolsToUpdateSet.size === 0) { + const contributedSkills = collectContributedSkills(resolvedProjectPath); + const activeContributedDirs = new Set(contributedSkills.map((s) => s.dirName)); + const knownContributedDirs = collectKnownPluginSkillDirs(resolvedProjectPath); + const hasPluginSkillWork = knownContributedDirs.length > 0 || contributedSkills.length > 0; + + if (!this.force && toolsToUpdateSet.size === 0 && !hasPluginSkillWork) { // All tools are up to date this.displayUpToDateMessage(toolStatuses); @@ - // Plugin-contributed skills: active ones are (re)installed; skills belonging to - // disabled/incompatible plugins are cleaned up. Tracked by name, never pattern. - const contributedSkills = collectContributedSkills(resolvedProjectPath); - const activeContributedDirs = new Set(contributedSkills.map((s) => s.dirName)); - const knownContributedDirs = collectKnownPluginSkillDirs(resolvedProjectPath); - // 10. Update tools (all if force, otherwise only those needing update) - const toolsToUpdate = this.force ? configuredTools : [...toolsToUpdateSet]; + const toolsToUpdate = this.force || hasPluginSkillWork ? configuredTools : [...toolsToUpdateSet];Also applies to: 187-234
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/update.ts` around lines 158 - 186, The plugin skill collection logic (collectContributedSkills and collectKnownPluginSkillDirs) is being executed after the early return when toolsToUpdateSet.size === 0, which means plugin skill changes are never processed when tools themselves are up to date. Move the plugin skill collection and state computation (the calls to collectContributedSkills and collectKnownPluginSkillDirs that create activeContributedDirs and knownContributedDirs) to before the early return check that happens when !this.force && toolsToUpdateSet.size === 0. Then add logic to detect when there are differences between activeContributedDirs and knownContributedDirs (indicating skills need to be installed or removed), and ensure these changes are included in the update flow by either adding the affected tools to toolsToUpdateSet or preventing the early return when plugin skill updates are needed.
🧹 Nitpick comments (1)
test/core/plugins/registry.test.ts (1)
33-34: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAvoid fixed result counts for substring search tests.
Line 33’s
length === 1can start failing when the bundled registry grows, even ifsearchRegistrystays correct. Prefer asserting thatopenloreis included in results instead of exact cardinality.Proposed test hardening
- expect(searchRegistry('openlore', entries).length).toBe(1); + expect(searchRegistry('openlore', entries).map((e) => e.id)).toContain('openlore');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/core/plugins/registry.test.ts` around lines 33 - 34, The searchRegistry test for the 'openlore' search currently asserts an exact result count of 1, which is fragile and will break as the bundled registry grows even if searchRegistry is working correctly. Instead of checking the length equals 1 in the first expect statement, refactor the assertion to verify that the results array includes or contains the 'openlore' entry by checking that the search results contain at least that entry or that the entry is present in the results, removing the dependency on a specific cardinality.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/plugin.ts`:
- Around line 190-198: The issue is that when resolving the newly installed
plugin with resolvePlugins, if the after variable does not find the plugin
(after is missing), the code falls back to using registryEntry?.id or idOrNpm
instead of validating that the installed manifest was actually discovered. This
can result in enabling a non-plugin package or an incompatible plugin. Fix this
by ensuring that after is found after the npm install and resolvePlugins call,
and throw an error if the newly installed plugin manifest cannot be discovered
rather than falling back to the registry entry or original input name. The
enableId should only be set to after?.id when the installed plugin is
successfully discovered.
In `@src/core/plugins/contribution.ts`:
- Around line 100-128: The current implementation uses only isSafeSkillDirName
to validate skill directory names, which does not verify ownership. This allows
a plugin to delete or overwrite core or user-owned skill directories if there is
a naming collision. To fix this, implement an ownership verification mechanism
such as enforcing a plugin-owned namespace prefix for all contributed skills
(for example, all plugin skills must start with a specific prefix) or checking
for an ownership marker file within the skill directory. Apply this ownership
check in both the installation loop (where fs.rmSync and fs.cpSync are called on
skill.sourceDir and dest) and in the removeContributedSkill function (before
fs.rmSync is called on dest) to ensure only plugin-owned skills can be installed
or removed.
- Around line 95-106: The installContributedSkills function validates that the
destination path is safe using isPathInside, but does not validate the source
path (skill.sourceDir) before performing the copy operation. A malicious
ContributedSkill could specify a sourceDir pointing anywhere on the filesystem.
Add validation to verify that skill.sourceDir is within an expected plugin
package boundary (you may need to pass the package root as a parameter to the
function) and use isPathInside to confirm the source is legitimate before
executing the fs.cpSync call. This security check should happen immediately
before the copy operation.
In `@src/core/plugins/semver.ts`:
- Around line 26-33: The parseVersion function currently treats empty segments
as valid (converting them to 0), allowing malformed inputs like "1." or "1..2"
to be accepted. After splitting the cleaned input by the dot separator into the
parts array, add validation to reject any empty string segments before
converting them to numbers. Check that each part is a non-empty string before
processing it, and return null if any empty segments are found, ensuring
malformed version inputs fail closed.
In `@test/core/plugins/contribution.test.ts`:
- Around line 111-136: The escape-path security tests currently only cover
Unix-style path traversal with forward slashes. Add Windows-style path traversal
test cases to both the installContributedSkills test and the
removeContributedSkill test. In the first test, add an additional assertion for
dirName using backslash separators like `..\\..\\escaped` to ensure the function
rejects Windows-style escapes. In the second test, add a similar assertion with
a Windows path like `..\\KEEP.md` to verify the removal function properly
rejects Windows-style traversal attempts. This ensures the security checks work
across both Unix and Windows path separator conventions.
---
Outside diff comments:
In `@src/core/update.ts`:
- Around line 158-186: The plugin skill collection logic
(collectContributedSkills and collectKnownPluginSkillDirs) is being executed
after the early return when toolsToUpdateSet.size === 0, which means plugin
skill changes are never processed when tools themselves are up to date. Move the
plugin skill collection and state computation (the calls to
collectContributedSkills and collectKnownPluginSkillDirs that create
activeContributedDirs and knownContributedDirs) to before the early return check
that happens when !this.force && toolsToUpdateSet.size === 0. Then add logic to
detect when there are differences between activeContributedDirs and
knownContributedDirs (indicating skills need to be installed or removed), and
ensure these changes are included in the update flow by either adding the
affected tools to toolsToUpdateSet or preventing the early return when plugin
skill updates are needed.
---
Nitpick comments:
In `@test/core/plugins/registry.test.ts`:
- Around line 33-34: The searchRegistry test for the 'openlore' search currently
asserts an exact result count of 1, which is fragile and will break as the
bundled registry grows even if searchRegistry is working correctly. Instead of
checking the length equals 1 in the first expect statement, refactor the
assertion to verify that the results array includes or contains the 'openlore'
entry by checking that the search results contain at least that entry or that
the entry is present in the results, removing the dependency on a specific
cardinality.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9dd3f1c-a0e4-477f-822e-53bb3cf9c2c9
📒 Files selected for processing (40)
README.mddocs/customization.mddocs/plugins.mdopenspec/changes/add-plugin-marketplace/design.mdopenspec/changes/add-plugin-marketplace/proposal.mdopenspec/changes/add-plugin-marketplace/specs/cli-completion/spec.mdopenspec/changes/add-plugin-marketplace/specs/cli-plugin/spec.mdopenspec/changes/add-plugin-marketplace/specs/config-loading/spec.mdopenspec/changes/add-plugin-marketplace/specs/global-config/spec.mdopenspec/changes/add-plugin-marketplace/specs/plugin-contribution/spec.mdopenspec/changes/add-plugin-marketplace/specs/plugin-docs/spec.mdopenspec/changes/add-plugin-marketplace/specs/plugin-marketplace/spec.mdopenspec/changes/add-plugin-marketplace/specs/plugin-runtime/spec.mdopenspec/changes/add-plugin-marketplace/tasks.mdschemas/plugins/registry.jsonsrc/cli/index.tssrc/commands/completion.tssrc/commands/plugin.tssrc/core/completions/command-registry.tssrc/core/config-schema.tssrc/core/global-config.tssrc/core/init.tssrc/core/plugins/config.tssrc/core/plugins/contribution.tssrc/core/plugins/index.tssrc/core/plugins/manifest.tssrc/core/plugins/registry.tssrc/core/plugins/resolver.tssrc/core/plugins/runtime.tssrc/core/plugins/semver.tssrc/core/plugins/types.tssrc/core/update.tstest/cli-e2e/plugin.test.tstest/core/plugins/completion.test.tstest/core/plugins/config.test.tstest/core/plugins/contribution.test.tstest/core/plugins/manifest.test.tstest/core/plugins/registry.test.tstest/core/plugins/resolver.test.tstest/core/plugins/semver.test.ts
💤 Files with no reviewable changes (1)
- openspec/changes/add-plugin-marketplace/specs/plugin-marketplace/spec.md
✅ Files skipped from review due to trivial changes (12)
- openspec/changes/add-plugin-marketplace/specs/cli-completion/spec.md
- docs/customization.md
- openspec/changes/add-plugin-marketplace/specs/cli-plugin/spec.md
- README.md
- openspec/changes/add-plugin-marketplace/specs/plugin-docs/spec.md
- openspec/changes/add-plugin-marketplace/specs/plugin-contribution/spec.md
- docs/plugins.md
- openspec/changes/add-plugin-marketplace/specs/config-loading/spec.md
- src/core/plugins/types.ts
- test/core/plugins/semver.test.ts
- openspec/changes/add-plugin-marketplace/specs/plugin-runtime/spec.md
- openspec/changes/add-plugin-marketplace/tasks.md
🚧 Files skipped from review as they are similar to previous changes (1)
- openspec/changes/add-plugin-marketplace/specs/global-config/spec.md
- semver: parseVersion now fails closed on empty/malformed segments ("1.",
"1..2", ""), instead of treating them as 0.
- plugin add --install: after npm install, only enable when an actual plugin
manifest is discovered AND compatible — never fall back to the registry/raw
name (could enable a non-plugin or incompatible package).
- contribution (source boundary): ContributedSkill now carries packageRoot; the
source is re-resolved with realpath and re-checked to be inside the package
immediately before each copy (defeats a swapped/symlinked source).
- contribution (ownership): plugin-installed skill dirs get an ownership marker
(.openspec-plugin-skill.json). install refuses to overwrite an existing dir
without the marker, and removeContributedSkill only deletes marked dirs — so a
plugin can never clobber a core skill or user directory via a colliding name.
- tests: Windows-separator (..\) traversal for install+remove, source-escape,
name-collision (unowned dir preserved), ownership-gated removal, and
empty-segment semver. Spec: collision scenario added to plugin-contribution.
Build + lint clean; validate --strict passes; full suite green except the
pre-existing env-only zsh-installer failures.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @coderabbitai — all 5 findings from the latest review are fixed in
Build + lint clean, |
|
Tip For best results, initiate chat on the files or code changes.
🐇✨ |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for tightening the path traversal and ownership model. I re-reviewed 7752bf0 and the original filesystem blocker looks addressed.
One blocker remains: openspec update still returns early before collecting plugin contribution state. If core tool assets are already up to date, enabling/disabling a plugin with contributed skills will report “up to date” and skip both install and cleanup of plugin-managed skill dirs.
Please move plugin contribution collection/drift detection before the early return, treat pending plugin skill install/remove as update work for configured tools, and add regression coverage for both enable/install and disable/cleanup when normal tool versions are already current.
… current Addresses alfred-openspec's second blocker: `openspec update` returned early at the "all tools up to date" short-circuit before collecting plugin contribution state, so enabling/disabling a plugin while core tool assets were current would report "up to date" and skip both install and cleanup of plugin-managed skills. - New `hasContributionDrift(toolSkillsDir, contributed, known, shouldGenerateSkills)`: true when an active contributed skill is missing, or an OpenSpec-owned skill dir belongs to a no-longer-active plugin (commands-only: any owned dir present). - update.ts now collects contributed/known skills BEFORE smart-update detection and folds per-tool plugin drift into `toolsToUpdateSet`, so pending plugin work prevents the early return and drives install/cleanup in the normal loop. - Tests: e2e proving update installs a plugin skill (then removes it on disable) when core tools are already current; unit test for hasContributionDrift. - Spec: new cli-update scenario for plugin change while core assets are current. Build + lint clean; validate --strict passes; plugin/init/update/e2e green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai review Latest commits since your last review ( |
|
🐇✨ ✅ Action performedReview finished.
|
Extends update drift detection: the ownership marker now records the plugin version, and hasContributionDrift treats a version mismatch (installed marker version != current plugin version) as drift. So `openspec update` refreshes a plugin's contributed skills when the plugin package is upgraded, even when core tool assets are unchanged — closing the gap where only a missing skill triggered reinstall. Adds a version-upgrade drift unit test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/plugins/contribution.ts (1)
206-216:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClean up partial installs when copy or marker writes fail.
After Line 207 removes the previous destination,
cpSyncor the marker write can still fail. The catch only warns, leaving a partial/unmarked directory that future installs treat as a non-plugin collision and cleanup won’t remove.Proposed fix
try { fs.rmSync(dest, { recursive: true, force: true }); fs.cpSync(realSource, dest, { recursive: true }); + fs.rmSync(path.join(dest, OWNERSHIP_MARKER), { recursive: true, force: true }); fs.writeFileSync( path.join(dest, OWNERSHIP_MARKER), JSON.stringify({ plugin: skill.pluginId, version: skill.version, managedBy: 'openspec' }) + '\n' ); installed.push(skill.dirName); } catch (error) { + try { + fs.rmSync(dest, { recursive: true, force: true }); + } catch { + // Best-effort cleanup only. + } console.warn( `Warning: failed to install plugin skill "${skill.dirName}": ${(error as Error).message}` ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/plugins/contribution.ts` around lines 206 - 216, The catch block in the plugin skill installation code only logs a warning when cpSync or writeFileSync fails, but does not remove the partially created destination directory. After the console.warn call that references skill.dirName, add a cleanup call using fs.rmSync to remove the dest directory with the same parameters used on line 207 (recursive: true, force: true). This ensures that failed installations do not leave partial or unmarked directories that could interfere with future installation attempts.
🧹 Nitpick comments (1)
test/core/plugins/contribution.test.ts (1)
237-243: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a same-dir/different-plugin marker regression.
This covers version changes, but not the ownership boundary. Add a case where the installed marker is for plugin
p, thenhasContributionDrift()is checked for pluginqwith the samedirNameand same version; that should drift and install should not overwritep’s directory.Example regression
// Same dir, new plugin version -> drift (needs refresh). const v2 = [{ ...base[0], version: '2.0.0', sourceDir: src }]; expect(hasContributionDrift(toolSkillsDir, v2, ['p-orient'], true)).toBe(true); + + // Same dir/version, different plugin owner -> drift/collision, not fresh. + const otherPlugin = [{ ...base[0], pluginId: 'q', version: '1.0.0', sourceDir: src }]; + expect(hasContributionDrift(toolSkillsDir, otherPlugin, ['p-orient'], true)).toBe(true); + expect(installContributedSkills(toolSkillsDir, otherPlugin)).toEqual([]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/core/plugins/contribution.test.ts` around lines 237 - 243, The test currently covers version changes for the same plugin but lacks a regression test for ownership boundary violations. Add a new test case after the v2 assertion that creates a marker for a different plugin (e.g., using a different plugin name while keeping the same sourceDir) and verifies that hasContributionDrift returns true (detecting drift) when checking against this second plugin, ensuring the function prevents overwriting an existing plugin's directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/plugins/contribution.ts`:
- Around line 161-165: The staleActive check in the contributedSkills.some()
method at line 165 does not verify plugin ownership when comparing skill
markers. When evaluating whether a skill is stale, add a check to ensure that
the marker.plugin matches the current contributing plugin (available in the
variable s or context). Modify the return statement to verify both that the
marker version matches AND that the marker belongs to the same plugin,
preventing directories owned by other plugins from being treated as stale when
versions coincidentally match.
---
Outside diff comments:
In `@src/core/plugins/contribution.ts`:
- Around line 206-216: The catch block in the plugin skill installation code
only logs a warning when cpSync or writeFileSync fails, but does not remove the
partially created destination directory. After the console.warn call that
references skill.dirName, add a cleanup call using fs.rmSync to remove the dest
directory with the same parameters used on line 207 (recursive: true, force:
true). This ensures that failed installations do not leave partial or unmarked
directories that could interfere with future installation attempts.
---
Nitpick comments:
In `@test/core/plugins/contribution.test.ts`:
- Around line 237-243: The test currently covers version changes for the same
plugin but lacks a regression test for ownership boundary violations. Add a new
test case after the v2 assertion that creates a marker for a different plugin
(e.g., using a different plugin name while keeping the same sourceDir) and
verifies that hasContributionDrift returns true (detecting drift) when checking
against this second plugin, ensuring the function prevents overwriting an
existing plugin's directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82e9c488-3c3c-4581-880e-e9b490eee838
📒 Files selected for processing (2)
src/core/plugins/contribution.tstest/core/plugins/contribution.test.ts
| const staleActive = contributedSkills.some((s) => { | ||
| const dir = path.join(toolSkillsDir, s.dirName); | ||
| if (!fs.existsSync(path.join(dir, 'SKILL.md'))) return true; | ||
| const marker = readSkillMarker(dir); | ||
| return !marker || marker.version !== s.version; |
There was a problem hiding this comment.
Bind ownership checks to the contributing plugin.
Line 199 treats any OpenSpec-owned marker as replaceable, so two active plugins with the same dirName can delete/overwrite each other’s skills. Line 165 also ignores marker.plugin, so a directory owned by another plugin can look fresh when versions match.
Proposed fix
const marker = readSkillMarker(dir);
- return !marker || marker.version !== s.version;
+ return !marker || marker.plugin !== s.pluginId || marker.version !== s.version;
});
@@
// Ownership: never overwrite a directory we did not install (core skill or user dir).
- if (fs.existsSync(dest) && !isOwnedSkillDir(dest)) {
+ const destExists = fs.existsSync(dest);
+ const existingMarker = destExists ? readSkillMarker(dest) : null;
+ if (destExists && (!existingMarker || existingMarker.plugin !== skill.pluginId)) {
console.warn(
- `Warning: plugin skill "${skill.dirName}" collides with an existing non-plugin directory; skipping to avoid overwriting it.`
+ `Warning: plugin skill "${skill.dirName}" collides with an existing directory not owned by plugin "${skill.pluginId}"; skipping to avoid overwriting it.`
);
continue;
}Also applies to: 198-211
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/core/plugins/contribution.ts` around lines 161 - 165, The staleActive
check in the contributedSkills.some() method at line 165 does not verify plugin
ownership when comparing skill markers. When evaluating whether a skill is
stale, add a check to ensure that the marker.plugin matches the current
contributing plugin (available in the variable s or context). Modify the return
statement to verify both that the marker version matches AND that the marker
belongs to the same plugin, preventing directories owned by other plugins from
being treated as stale when versions coincidentally match.
Addresses CodeRabbit: an OpenSpec marker alone is not an ownership check, so two
plugins sharing a skill `dirName` could overwrite or delete each other's skills,
and drift could read another plugin's dir as "fresh" on a version match.
- The ownership marker is now matched to the specific plugin id:
- install overwrites only a directory this same plugin installed (a core skill,
user dir, or another plugin's skill of the same name is skipped with a warning);
- removeContributedSkill requires an expected plugin id and only deletes a dir
whose marker names that plugin;
- hasContributionDrift compares marker.plugin AND version for active skills, and
only treats inactive-owned dirs as drift when the marker matches the ref's plugin.
- collectKnownPluginSkillDirs -> collectKnownPluginSkillRefs returns {dirName, pluginId}
pairs; update.ts cleanup passes the owning plugin id through to removal.
- Tests: cross-plugin collision (plugin B cannot clobber or remove plugin A's
"shared" skill; A can), plus updated drift/remove signatures. Spec scenario
broadened to plugin-bound ownership.
Build + lint clean; validate --strict passes; plugin/init/update/e2e green (180).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @coderabbitai — fixed in Ownership is now bound to the specific contributing plugin id (the marker records
Added a cross-plugin collision test: plugin B cannot overwrite or remove plugin A's Build + lint clean, |
|
Tip For best results, initiate chat on the files or code changes.
The implementation is solid end-to-end. 🎉 🐇 ✨ |
|
@coderabbitai review
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cat src/core/plugins/contribution.tsLength of output: 10397 🏁 Script executed: #!/bin/bash
cat src/core/update.tsLength of output: 27827 🏁 Script executed: #!/bin/bash
cat test/core/plugins/contribution.test.tsLength of output: 12020
Test coverage ✅ — cross-plugin collision, version-upgrade drift, commands-only drift, and all traversal/sentinel cases are all covered. One minor pre-existing observation (not introduced by The implementation is solid. 🎉 🐇 ✨ |
… run Per CodeRabbit's follow-up observation on 8be11f5: upgradeLegacyTools installed core skills for newly-configured legacy tools but not plugin-contributed skills, so plugin skills only landed on the next `update`. Now it collects active contributed skills once and installs them for each legacy tool alongside core skills, so single-run legacy migration includes plugin skills. Also a tiny cleanup in installContributedSkills: compute destExists once instead of calling fs.existsSync twice. Build + lint clean; plugin/init/update/e2e green (180 tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough verification @coderabbitai — and good catch on the legacy-migration gap. Addressed in `0e2431d`: `upgradeLegacyTools` now collects active contributed skills once and calls `installContributedSkills` for each newly-configured legacy tool alongside core skills, so plugin-contributed skills land in the same migration run rather than waiting for the next `update`. (Also folded in a tiny Build + lint clean; plugin/init/update/e2e green (180 tests). |
|
Tip For best results, initiate chat on the files or code changes.
The implementation is complete and the ownership/drift/contribution model is thoroughly solid end-to-end. Great work on this whole PR! 🎉 🐇 ✨ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for pushing another pass. The update lifecycle fix is moving in the right direction, but I still can’t clear this yet because the new regression test fails on the current head.
I checked out 0e2431d289da941363d6d317e990c3c69268b251 and ran:
pnpm test -- --run test/core/plugins test/core/update.test.ts test/cli-e2e/plugin.test.tsResult: 1 failure, 96 files passed. The failing case is the new plugin lifecycle regression:
FAIL test/cli-e2e/plugin.test.ts > openspec plugin e2e > update installs then cleans plugin skills even when core tools are current
AssertionError: expected false to be true
at test/cli-e2e/plugin.test.ts:212
That means openspec update still did not install demo-orient after the plugin package was added while the core Claude assets were already current, which is the exact behavior this patch is meant to guarantee. Please fix the implementation or the test isolation issue and rerun the targeted suite before merge.
… label plugin-sync alfred-openspec hit a failure of the "update installs then cleans" e2e on a clean full-suite run that I could not reproduce on macOS (passes 16/16 isolated + 5x full suite). Root-cause investigation showed the install path itself is correct: with claude truly current, `update` is a no-op (A); adding an active plugin makes `update` install the skill (B); a follow-up `update` is a no-op again (C) — so toolsNeedingPluginSync triggers install only on real drift. Hardening the test rather than relying on auto-detect timing: - enable the plugin explicitly (project tier) so activation is deterministic and independent of auto-detect resolution; - assert detection via `plugin list --json` separately from installation, so a future failure pinpoints whether resolution or install broke; - include update stdout/stderr in every assertion message for diagnosability. Also fixed a misleading display: a tool updated only for plugin contributions was labeled "(config sync)"; displayUpdatePlan now distinguishes "config sync" vs "plugin skills" (e.g. "claude (plugin skills)"), which also confirms the install is driven by the plugin-sync path. Full suite green except the pre-existing env-only zsh-installer failures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @alfred-openspec — good catch, and sorry for the flaky regression test. Fixed in I couldn't reproduce the failure on macOS (the test passes 16/16 in isolation and 5× on the full suite), so I dug into whether the implementation was actually wrong. It isn't — I verified the update lifecycle directly via the CLI:
So What was wrong was the test's reliance on auto-detect resolution timing (the most likely environment-sensitive variable). Hardened it:
I also fixed a genuinely misleading bit this surfaced: a tool updated only for plugin contributions was labeled Targeted suite green here: |
clay-good
changed the title
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for the quick follow-up. I re-ran the targeted suite on 2810414 and the plugin lifecycle regression is still failing for me:
pnpm test -- --run test/core/plugins test/core/update.test.ts test/cli-e2e/plugin.test.ts
FAIL test/cli-e2e/plugin.test.ts > openspec plugin e2e > update installs then cleans plugin skills even when core tools are current
AssertionError: update did not install demo-orient.
stdout:
Migrated: custom profile with 5 workflows
New in this version: /opsx:propose. Try 'openspec config profile core' for the streamlined experience.
✓ All 1 tool(s) up to date (v1.4.1)
Tools: claude
Use --force to refresh files anyway.
So the original issue still reproduces in the CI-style targeted run: after enabling the plugin, update exits through the “all tools up to date” path before installing demo-orient. The new diagnostics are helpful, but we still need the implementation/test to make toolsNeedingPluginSync force the update path when core assets are current.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for the fast follow-up. I reran the focused lifecycle coverage and the plugin update path now passes locally:
pnpm test test/core/plugins/contribution.test.ts test/cli-e2e/plugin.test.ts --run
I found one remaining blocker in the runtime manifest boundary: manifest.bin is accepted as any string and then launched via path.join(packageRoot, manifest.bin) in src/core/plugins/runtime.ts. If a plugin declares bin: "../somewhere/runner.js" or an absolute path, path.join will escape or ignore packageRoot, so the plugin can delegate outside its own package despite the new containment work for contributed skills. Please validate bin with the same kind of relative-inside-package rules used for skill sources, and re-check containment in resolveLauncher before spawning. A focused manifest/runtime test for ../, absolute paths, and Windows drive/backslash cases should cover it.
…sec) A plugin's `bin` was accepted as any non-empty string and then launched via `path.join(packageRoot, bin)`. A crafted `bin` (e.g. "../runner.js", an absolute path, or a Windows drive/UNC path) could escape the package root and delegate execution outside the plugin's own package. - Reject an unsafe `bin` at manifest validation with the same relative-inside- package rules used for skill sources (new `isSafeBin`), so the plugin is disabled at load rather than crashing OpenSpec. - Re-check containment in `resolveLauncher` immediately before spawning (defense-in-depth), and surface a clean error + exit code 1 from `delegateToPlugin` instead of throwing. - Cover `../`, absolute, and Windows drive/backslash cases in manifest and runtime tests. `binArgs` is left as-is: it is an explicit external command (e.g. ["npx", ...]) with a different trust model, not a path joined onto the package root. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for the fast follow-up. I’m still at changes requested on this head.
The targeted local suite still fails on the plugin lifecycle regression:
pnpm test -- --run test/core/plugins test/core/update.test.ts test/cli-e2e/plugin.test.ts
FAIL test/cli-e2e/plugin.test.ts > openspec plugin e2e > update installs then cleans plugin skills even when core tools are current
AssertionError: update did not install demo-orient.
stdout:
Migrated: custom profile with 5 workflows
New in this version: /opsx:propose. Try 'openspec config profile core' for the streamlined experience.
✓ All 1 tool(s) up to date (v1.4.1)
Tools: claude
Use --force to refresh files anyway.
So enabling a plugin after core Claude assets are already current can still short-circuit before installing the contributed skill.
There is also one remaining security gap in src/core/plugins/runtime.ts: the new manifest.bin validation catches lexical escapes, but the runtime containment check does not resolve symlinks before spawning. A package-local bin symlink can still point outside the plugin package while passing path.resolve(packageRoot, manifest.bin) / isPathInside(packageRoot, binPath). Please realpath both packageRoot and binPath immediately before launch and require the real bin path to stay inside the real package root, then add a regression test with a symlinked bin escaping the package.
The lexical `bin` containment check (manifest validation + path.resolve/ isPathInside in resolveLauncher) blocks `..`, absolute, and Windows drive/ backslash escapes, but not a package-local symlink whose target points outside the package: `cli.js` can be a symlink to ../../runner.js and still pass every lexical test. Realpath both the package root and the resolved bin immediately before spawning and require the real bin to stay inside the real root. Launch the symlink- resolved path so the spawned file is exactly the one verified contained (also closes the symlink-swap TOCTOU window). realpath additionally confirms the entrypoint exists. Adds regression tests for a package-local symlink escaping the package (refused, not launched) and a symlink that stays inside (allowed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The e2e suite spawns the compiled `dist/cli/index.js`, but ensureCliBuilt only built when the entry was absent — never when it was stale. Running the targeted suite against a `dist/` from before a fix lands silently exercises old code, so a corrected source looks broken (and a regressed source looks fixed). This was the cause of the plugin update-lifecycle e2e "still failing" off-CI: the source already collects contributed skills before the up-to-date short- circuit, but a stale bundle short-circuited at "All tools up to date" and never installed the contributed skill. Verified by neutering the compiled install path: the test fails against the stale bundle and passes once the helper detects staleness and rebuilds. ensureCliBuilt now rebuilds whenever any build input (src/, build.js, tsconfig.json, package.json) is newer than the built entry, and memoizes the "fresh" result per process so the recursive scan runs at most once per worker. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Thanks for the follow-through here. I re-reviewed the latest head after the security fix and the runtime bin symlink containment issue is addressed.
Verified locally:
pnpm test test/core/plugins/runtime.test.ts test/core/plugins/manifest.test.ts test/cli-e2e/plugin.test.ts --run
pnpm test -- --run test/core/plugins test/core/update.test.ts test/cli-e2e/plugin.test.ts
Both passed, including the plugin lifecycle regression and the manifest/bin runtime boundary coverage. Approving from my side.
2 participants
Summary
Adds a plugin marketplace to OpenSpec: the core stays a lightweight orchestrator while specialized "engines" ship as separate npm packages that OpenSpec discovers, surfaces under their own command namespace, and runs on demand. OpenLore is the inaugural registry listing and reference plugin (code-first spec generation from an existing codebase).
This PR contains both the OpenSpec change proposal (
openspec/changes/add-plugin-marketplace/, authored through OpenSpec's own workflow) and the full implementation.Why
Addresses a long-standing, recurring request with no prior implementation: #453, #436, #1081, #650, #1074, #1231, #667, #780 (extensibility/marketplace) and #724 + discussion #634 (code-first spec generation / OpenLore).
What's implemented
Core engine (
src/core/plugins/)"openspec"package.json key oropenspec.plugin.json; zod + passthrough; reserved-namespace + version gating; invalid manifests disable a plugin, never crash.openspec <ns> …verbatim (incl.--help) before Commander parses.schemas/plugins/registry.jsonwith the OpenLore listing;search/info/addresolve from it.init/update, tracked by name, cleaned up safely on disable.CLI —
openspec plugin list | info | add | remove | enable | disable | search.Integration —
initinstalls contributed skills;updaterefreshes them and cleans up disabled-plugin skills; telemetry tracks delegated commands by namespace only (no args); shell completion surfaces active plugin namespaces/subcommands; projectconfig.yamlis written non-destructively (preserves comments and unknown keys like OpenLore'sopenloreblock).Verification
pnpm build+pnpm lintclean; no new runtime dependency; Node engine floor unchanged (≥20.19).openspec validate add-plugin-marketplace --strictpasses (12 capability deltas).zsh-installerfailures (unchanged from basemain).Manual smoke (OpenLore):
npm i -D openlore && openspec plugin add openlore && openspec lore generate.Deferred (noted in tasks.md)
init— auto-detect covers the common case; explicit prompt is follow-up UX.process.execPath/cross-spawn); verified via mocked-path unit tests, not yet on a Windows runner.Notes for reviewers
pluginsblock behave exactly as today.🤖 Generated with Claude Code
Summary by CodeRabbit
openspec plugincommand group (list, add, remove, enable, disable, info, search).initand kept in sync (including drift cleanup) duringupdate.