refactor: delete legacy per-target emitters/validators now that all targets are migrated - #17
Merged
steve-calvert-glean merged 8 commits intoJul 26, 2026
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.
Ports emitCodex/validateCodex into src/targets/codex.ts as a PluginTargetDefinition, correcting the plugin format's real shape — re-verified directly against developers.openai.com/codex/plugins/build (fetched twice, independently, for consistency) since Codex has no CLI validator or vendored schema to check against: - plugin.json requires only "name"; version/description/author etc. are optional. The previous validator wrongly required version and description. - Every marketplace entry needs policy.installation, policy.authentication, and category — previously unvalidated, so an incomplete entry shipped silently. pluginpack can't infer these, so the base entry stays guess-free and validateOutput now errors clearly when an author never supplies them via the per-plugin `entry` passthrough (already how the existing conformance fixture supplies them). - A marketplace entry's source is a bare string only for local plugins (the only shape pluginpack itself ever emits); url/git-subdir/npm sources are structured objects with an inner "source" discriminator. validateMarketplaceEntry now accepts either shape instead of the previously shared, string-only validator. - plugin.json now declares a `hooks` pointer when hooks/ is present, matching skills/mcpServers (previously only skills/mcpServers were declared, so hooks were emitted but never referenced). Updates CONFORMANCE.md's Codex section, which had pinned a stale, bare-string-only shape from an earlier doc retrieval.
…tion Every migrated target's validateOutput calls validateHooksShape (added in the registry scaffold), which only checked that hooks.json has a "hooks" object — narrower than the legacy per-target validateHooks it replaced, which also required each event's entries to be an array, rejected empty "command" strings, and errored if a command referenced the generated update-check script without that script actually being present. None of that depth had a regression test, so the narrowing was silent. Ports the full check into validateHooksShape once, so every target that already calls it (all 5, post-migration) regains it for free instead of needing the fix repeated per target file.
…argets are migrated All 5 targets (copilot, antigravity, cursor, claude, codex) now have a PluginTargetDefinition in src/targets/registry.ts, so the legacy fallback path adapters.ts existed for is dead: - Deletes src/targets.ts and src/validate.ts entirely (their only consumer was adapters.ts's legacyAdapters map). - Moves withRootFiles into src/targets/engine.ts, next to the artifact helper it depends on. - Tightens the registry's type from Partial<Record<TargetName, ...>> to Record<TargetName, ...> now that every target has an entry — a new TargetName won't build until it has a registry entry, the same exhaustiveness guarantee the deleted legacyAdapters map used to provide. - Simplifies adapters.ts to a thin emitTarget/validateOutput wrapper around the registry + engine, dropping the now-pointless TargetAdapter/adapters indirection that existed only to switch between legacy and registry per target. - Removes targetDefaultComponents/resolveTargetComponents from components.ts (superseded by each target's own defaultComponents). - Updates CLAUDE.md's Architecture/Targets sections and a couple of stale doc-comment references to match.
6 tasks
Cross-references the validateHooksShape fix from the prior commit — what it checks and where it's shared from, for the conformance doc's own "what's actually verified and how" mandate.
steve-calvert-glean
added
the
internal
…y-target-adapters # ------------------------ >8 ------------------------ # Do not modify or remove the line above. # Everything below it will be ignored. # # Conflicts: # src/targets.ts # src/validate.ts
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.
Summary
All 5 targets (copilot, antigravity, cursor, claude, codex) now have a
PluginTargetDefinitioninsrc/targets/registry.ts, so the legacy fallback pathadapters.tsexisted for is dead. This is the final step of the registry migrationCLAUDE.mditself flagged: "Adding a target currently touches ~5 places... consider introducing a single target registry first to localize this."src/targets.tsandsrc/validate.tsentirely (their only remaining consumer wasadapters.ts'slegacyAdaptersmap).withRootFilesintosrc/targets/engine.ts, next to theartifacthelper it depends on.Partial<Record<TargetName, ...>>toRecord<TargetName, ...>now that every target has an entry — a newTargetNamewon't build until it has a registry entry, the same exhaustiveness guarantee the deletedlegacyAdaptersmap used to provide.adapters.tsto a thinemitTarget/validateOutputwrapper around the registry + engine, dropping the now-pointlessTargetAdapter/adaptersindirection that existed only to switch between legacy and registry per target.targetDefaultComponents/resolveTargetComponentsfromcomponents.ts(superseded by each target's owndefaultComponents).CLAUDE.md's Architecture/Targets sections and a couple of stale doc-comment references (emitCursor,validate.ts) to match the new file layout.Also included (found while doing this cleanup, worth calling out separately): every migrated target's
validateOutputwas callingvalidateHooksShape, which only checked thathooks.jsonhas a"hooks"object — narrower than the legacy per-targetvalidateHooksit replaced, which also required each event's entries to be an array, rejected empty"command"strings, and errored if a command referenced the generated update-check script without that script being present. That depth had no regression test, so the narrowing was silent. Ported the full check intovalidateHooksShapeonce (a separate commit in this branch), so every target regains it for free, plus a new regression test.Net: -1497 lines. The build's
dist/chunk-*.jsalso dropped from ~105 KB to ~75 KB, confirming a real amount of dead code was removed, not just moved.Stacking
Based on
refactor/codex-target-registry(#16). Merge in order: #14 → #15 → #16 → this.Test plan
npm run typechecknpm run lintnpm test(64/64 passing)npm run buildnode dist/cli.js docs --check