refactor: migrate claude to the target registry (no behavior change) - #19
Merged
Merged
Conversation
Re-checking the originally planned Cursor "fixes" against the vendored plugin.schema.json / marketplace.schema.json (this repo's own conformance oracle, already passing against the current shape) showed they were wrong: - displayName/category/tags ARE valid plugin.json fields per the schema (additionalProperties: false, and they're explicitly listed) — not marketplace-entry-only fields as previously assumed. - Marketplace `owner` is genuinely optional (required: ["name", "plugins"] does not include it) — not required as previously assumed. Moving category/tags to the marketplace entry would have been actively wrong: entries only allow name/source/description (additionalProperties: false). None of that is changed here. What this commit actually does: - Migrates cursor onto PluginTargetDefinition, preserving every existing field and behavior (verified by the vendored-schema conformance test staying green). - Fixes one genuine, low-risk issue: the manifest builder's own hardcoded default-components list could diverge from this target's actual defaultComponents (components.ts). Replaced both with one list of schema-valid pointer fields, checked directly against the plugin's real resolved componentDirs — eliminates the divergence risk with no observable behavior change (confirmed via a new test exercising the one case that could have differed: an explicit `components: [...]` override). - Ports update-check's hook-injection into the new shared engine (src/targets/engine.ts), which previously only existed in the legacy emitCursor/emitClaude path — migrating cursor without this would have silently dropped update-check support. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
buildPluginManifest used the target-level `version` param directly instead of `pluginConfig.version ?? version`, silently dropping a per-plugin version override — a real regression from the pre-migration behavior, caught by porting the equivalent test from the claude migration (no test previously covered this for cursor specifically). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Ports emitClaude/validateClaude into src/targets/claude.ts as a PluginTargetDefinition, verified directly against `claude plugin validate --strict`: a minimal manifest with only `name` fails that check, confirming the existing version/description/author.name requirements already match the real CLI rather than over-constraining it. Applies the per-plugin version override in buildPluginManifest (pluginConfig.version ?? version) up front, matching the identical fix just made to cursor's copy of this pattern.
steve-calvert-glean
added
the
internal
…t-registry # ------------------------ >8 ------------------------ # Do not modify or remove the line above. # Everything below it will be ignored. # # Conflicts: # src/targets/registry.ts
![glean-it[bot]](https://avatars.githubusercontent.com/u/48140418?s=60&v=4){kind=small}
|
|
||
| defaultComponents: ["skills", "agents", "hooks", "scripts", "assets"], | ||
|
|
||
| resolvePluginPath: (pluginName, pluginConfig, targetConfig) => |
There was a problem hiding this comment.
Issue: In src/targets/claude.ts, resolvePluginPath does not normalize the computed pluginPath with toPosix(), unlike the legacy emitClaude and the migrated Copilot target, so on Windows the marketplace entry source field will contain backslashes (e.g. "./plugins\my-plugin") instead of the expected POSIX-style path, changing behavior and potentially breaking consumers that expect forward slashes.
Suggested fix: Update resolvePluginPath in src/targets/claude.ts to mirror the legacy Claude and Copilot implementations by wrapping the joined path in toPosix(), and import toPosix from ../fs.js. For example:
import path from "node:path";
import { toPosix } from "../fs.js";
...
resolvePluginPath: (pluginName, pluginConfig, targetConfig) =>
pluginConfig.path ??
toPosix(path.join(targetConfig.pluginRoot ?? "plugins", pluginName)),🔧 Tag @ glean-for-engineering to fix or click here to fix in Glean
💬 Help us improve! Was this comment helpful? React with 👍 or 👎
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Recreated after #15 was auto-closed when its stacked base branch was deleted by the #14 squash-merge (GitHub can't reopen a PR whose base branch no longer exists — the branch and commits were never lost). Same content as #15.
Summary
emitClaude/validateClaudeintosrc/targets/claude.tsas aPluginTargetDefinition, verified directly againstclaude plugin validate --strict: a minimal manifest with onlynamefails that check, confirming the existingversion/description/author.namerequirements already match the real CLI. No validation behavior change — this is a structural port.buildPluginManifest(pluginConfig.version ?? version) from the start, avoiding the bug just fixed on cursor's copy of this pattern in refactor: migrate cursor to the target registry #14 (a new regression test covers this: "applies per-target and per-plugin version overrides").@deprecatedmarkers to the now-supersededemitClaude/validateClaude, matching copilot/antigravity's already-migrated pattern.Test plan
npm run typechecknpm run lintnpm test(59/59 passing, including claude's pre-existing version-override regression test)npm run buildnode dist/cli.js docs --check