From 67b4f9a4e8a8517c7708efe7874d0d772953ed0c Mon Sep 17 00:00:00 2001 From: ScriptedAlchemy Date: Tue, 1 Sep 2026 18:15:41 +0000 Subject: [PATCH] fix(adapters): review follow-ups from #131/#141 Cursor's 64-character plugin-name bound was only enforced by the unified `plugin` planner, so the standalone `cursor` target accepted and emitted a manifest for an over-long name. The pinned official schema (cursor/plugins@0701892) constrains the name's charset but carries no maxLength, so both planners now assert the bound through one shared message. The capability-state exhaustive `default` branches returned the capability object itself, so an untyped adapter's misspelled state read as truthy support and could enable hooks or MCP. They now raise a typed CapabilityStateError, and the registry rejects a malformed declaration at registration so a bad state never reaches `supports()` or capability intersection. --- .changeset/adapter-review-followups.md | 5 ++ .../src/adapters/capability-state.ts | 11 +-- packages/agent-bundle/src/adapters/cursor.ts | 15 +++- packages/agent-bundle/src/adapters/plugin.ts | 6 +- .../agent-bundle/src/adapters/registry.ts | 22 ++++++ packages/agent-bundle/src/api.ts | 1 + .../agent-bundle/src/core/capabilities.ts | 71 +++++++++++++++++++ .../tests/adapter-capability-states.test.ts | 68 +++++++++++++++++- .../agent-bundle/tests/cursor-adapter.test.ts | 35 +++++++++ 9 files changed, 223 insertions(+), 11 deletions(-) create mode 100644 .changeset/adapter-review-followups.md diff --git a/.changeset/adapter-review-followups.md b/.changeset/adapter-review-followups.md new file mode 100644 index 000000000..fa71ef5af --- /dev/null +++ b/.changeset/adapter-review-followups.md @@ -0,0 +1,5 @@ +--- +"agent-bundle": patch +--- + +Hold Cursor's 64-character plugin-name bound in the standalone `cursor` planner as well as the unified `plugin` planner, and reject capability states outside the four-state contract with a typed `CapabilityStateError` at the registry boundary instead of returning a fabricated truthy state. diff --git a/packages/agent-bundle/src/adapters/capability-state.ts b/packages/agent-bundle/src/adapters/capability-state.ts index 21ec5540f..0f91def3a 100644 --- a/packages/agent-bundle/src/adapters/capability-state.ts +++ b/packages/agent-bundle/src/adapters/capability-state.ts @@ -1,4 +1,5 @@ import { sha256Hex, stableJson } from '../core/digest.ts'; +import { CapabilityStateError, unknownCapabilityStateError } from '../core/capabilities.ts'; import type { CapabilityEvidence, CapabilityState } from '../core/capabilities.ts'; import type { TargetAdapterMetadata } from './types.ts'; @@ -41,7 +42,7 @@ export const capabilityIsSupported = (capability: CapabilityState | undefined): return false; default: { const exhaustive: never = capability; - return exhaustive; + throw unknownCapabilityStateError(exhaustive); } } }; @@ -64,7 +65,7 @@ const evidenceFor = (capability: CapabilityState): CapabilityEvidence | undefine return undefined; default: { const exhaustive: never = capability; - return exhaustive; + throw unknownCapabilityStateError(exhaustive); } } }; @@ -81,7 +82,7 @@ const precedenceFor = (capability: CapabilityState): 0 | 1 | 2 | 3 => { return 3; default: { const exhaustive: never = capability; - return exhaustive; + throw unknownCapabilityStateError(exhaustive); } } }; @@ -98,7 +99,7 @@ const reasonFor = (capability: CapabilityState, precedence: 1 | 2 | 3): string | return precedence === 3 ? capability.reason : undefined; default: { const exhaustive: never = capability; - return exhaustive; + throw unknownCapabilityStateError(exhaustive); } } }; @@ -164,7 +165,7 @@ export const intersectCapabilityStates = ( return Object.freeze({ reason: mergedReason(left, right, precedence), state: 'prohibited' }); default: { const exhaustive: never = precedence; - return exhaustive; + throw new CapabilityStateError(`Capability precedence ${String(exhaustive)} has no intersection rule.`); } } }; diff --git a/packages/agent-bundle/src/adapters/cursor.ts b/packages/agent-bundle/src/adapters/cursor.ts index e12032a89..c22d590a9 100644 --- a/packages/agent-bundle/src/adapters/cursor.ts +++ b/packages/agent-bundle/src/adapters/cursor.ts @@ -65,6 +65,7 @@ export const cursorMcpValidator = validateMcp; export const cursorHooksValidator = validateHooks; const cursorNamePattern = /^[a-z0-9](?:[a-z0-9.-]*[a-z0-9])?$/u; +const cursorNameMaxLength = 64; const cursorVariablePattern = /\$\{([A-Z][A-Z0-9_]*)(?::-[^}]*)?\}/gu; const cursorBuiltInVariables = new Set(['CLAUDE_PLUGIN_ROOT', 'CURSOR_PLUGIN_ROOT']); @@ -87,7 +88,16 @@ export const cursorVariables = (mcp: Record | undefined): Recor /** True when a plugin name satisfies Cursor's lowercase kebab-case contract. */ export const isValidCursorPluginName = (name: string): boolean => - cursorNamePattern.test(name) && name.length <= 64; + cursorNamePattern.test(name) && name.length <= cursorNameMaxLength; + +/** + * The pinned official schema constrains the name's charset but carries no + * `maxLength`, so the 64-character bound only holds if both Cursor-producing + * planners state it. They share this message so neither can drift. + */ +export const cursorPluginNameError = (name: string): string => + `Plugin name ${JSON.stringify(name)} is not a valid Cursor plugin name ` + + `(lowercase kebab-case, at most ${cursorNameMaxLength} characters).`; /** The schema-collision guard the bundle emits when no hook lowers to Cursor. */ export const emptyCursorHooksDocument = Object.freeze({ hooks: {}, version: 1 }); @@ -296,6 +306,9 @@ const mcpPlanContext: CursorMcpServerPlanContext = Object.freeze({ codePrefix: c export const planCursorArtifacts = (model: NormalizedPlugin): TargetArtifactPlan => { const isSelected = (targets: readonly string[]): boolean => targets.includes(cursorName); const diagnostics: Diagnostic[] = []; + if (!isValidCursorPluginName(model.metadata.name)) { + diagnostics.push(errorDiagnostic('cursor.name', cursorPluginNameError(model.metadata.name))); + } const servers: Record> = Object.create(null) as Record>; for (const server of model.mcpServers) { if (!isSelected(server.targets)) continue; diff --git a/packages/agent-bundle/src/adapters/plugin.ts b/packages/agent-bundle/src/adapters/plugin.ts index cc5802066..3ef3f0371 100644 --- a/packages/agent-bundle/src/adapters/plugin.ts +++ b/packages/agent-bundle/src/adapters/plugin.ts @@ -19,6 +19,7 @@ import { cursorHooksValidator, cursorManifest, cursorMcpValidator, + cursorPluginNameError, cursorPluginValidator, cursorVariables, emptyCursorHooksDocument, @@ -359,10 +360,7 @@ const plan = (model: NormalizedPlugin): TargetArtifactPlan => { let cursorHookEntries: readonly TargetHookEntry[] = Object.freeze([]); if (!isValidCursorPluginName(model.metadata.name)) { - diagnostics.push(errorDiagnostic( - 'plugin.cursor.name', - `Plugin name ${JSON.stringify(model.metadata.name)} is not a valid Cursor plugin name (lowercase kebab-case).`, - )); + diagnostics.push(errorDiagnostic('plugin.cursor.name', cursorPluginNameError(model.metadata.name))); } else { const emitCursorHooks = hookDocument !== undefined && hookDocumentValid; // Cursor's envelope is not the shared Claude/Codex format, so its hooks diff --git a/packages/agent-bundle/src/adapters/registry.ts b/packages/agent-bundle/src/adapters/registry.ts index 62fa3e6b9..b3705d6b7 100644 --- a/packages/agent-bundle/src/adapters/registry.ts +++ b/packages/agent-bundle/src/adapters/registry.ts @@ -1,3 +1,4 @@ +import { CapabilityStateError, capabilityStateNames, isCapabilityState } from '../core/capabilities.ts'; import type { CapabilityState } from '../core/capabilities.ts'; import { dataArrayValues } from '../core/strict-json.ts'; import type { @@ -350,6 +351,26 @@ const snapshotMcpRuntime = (adapter: TargetAdapter): TargetMcpRuntimeContract | }); }; +/** + * The registry is a runtime boundary for third-party and JavaScript adapters, + * whose declarations the compiler never checked. Rejecting a malformed state + * here keeps it out of `supports()` and capability intersection entirely. + */ +const assertCapabilityContract = (adapter: TargetAdapter): void => { + const capabilities = record(adapter.capabilities); + if (capabilities === undefined) { + throw new CapabilityStateError(`Target adapter "${adapter.name}" must declare capabilities as a record.`); + } + for (const [capability, state] of Object.entries(capabilities)) { + // An absent capability is an honest "not declared"; a present malformed one is not. + if (state === undefined || isCapabilityState(state)) continue; + throw new CapabilityStateError( + `Target adapter "${adapter.name}" capability "${capability}" must declare one of ` + + `${capabilityStateNames.join('/')} with that state's required fields.`, + ); + } +}; + export class TargetRegistry implements NormalizationTargetRegistry { readonly #adapters = new Map(); readonly #artifactLayouts = new Map(); @@ -369,6 +390,7 @@ export class TargetRegistry implements NormalizationTargetRegistry { if (extension !== undefined && this.#extensions.has(extension.key)) { throw new Error(`Config extension key "${extension.key}" is already registered.`); } + assertCapabilityContract(adapter); const metadata = snapshotMetadata(adapter.metadata); const artifactValidation = snapshotArtifactValidation(adapter, metadata); const nativeHookSource = snapshotNativeHookSource(adapter); diff --git a/packages/agent-bundle/src/api.ts b/packages/agent-bundle/src/api.ts index 3f556fe88..38fd6b26b 100644 --- a/packages/agent-bundle/src/api.ts +++ b/packages/agent-bundle/src/api.ts @@ -131,6 +131,7 @@ export type { export { HookService } from './services/hook-service.ts'; export type { HookListOptions, HookSimulationOptions } from './services/hook-service.ts'; export { createDefaultRegistry, TargetRegistry } from './adapters/registry.ts'; +export { CapabilityStateError, capabilityStateNames, isCapabilityState } from './core/capabilities.ts'; export type { TargetAdapter, TargetAdapterMetadata, diff --git a/packages/agent-bundle/src/core/capabilities.ts b/packages/agent-bundle/src/core/capabilities.ts index 7ee879d05..8235c54ba 100644 --- a/packages/agent-bundle/src/core/capabilities.ts +++ b/packages/agent-bundle/src/core/capabilities.ts @@ -1,3 +1,5 @@ +import { CodedError } from './errors.ts'; + /** Evidence backing a supported or degraded capability judgment. */ export interface CapabilityEvidence { readonly capabilityRevision: string; @@ -12,3 +14,72 @@ export type CapabilityState = | { readonly state: 'degraded'; readonly reason: string; readonly evidence?: CapabilityEvidence } | { readonly state: 'unavailable'; readonly reason: string } | { readonly state: 'prohibited'; readonly reason: string }; + +/** Thrown when a declaration escapes the four-state capability contract. */ +export class CapabilityStateError extends CodedError<'ERR_UNKNOWN_CAPABILITY_STATE'> { + constructor(message: string) { + super('CapabilityStateError', 'ERR_UNKNOWN_CAPABILITY_STATE', message); + } +} + +/** A Record over the union so a new state cannot be added without listing it here. */ +const capabilityStateNameFlags: Readonly> = Object.freeze({ + degraded: true, + prohibited: true, + supported: true, + unavailable: true, +}); + +/** The four contract states, sorted for stable diagnostics and error messages. */ +export const capabilityStateNames: readonly string[] = Object.freeze(Object.keys(capabilityStateNameFlags).sort()); + +const isCapabilityStateName = (value: unknown): value is CapabilityState['state'] => + typeof value === 'string' && Object.hasOwn(capabilityStateNameFlags, value); + +const isCapabilityEvidence = (value: unknown): value is CapabilityEvidence => { + if (typeof value !== 'object' || value === null) return false; + const candidate = value as Partial>; + return typeof candidate.capabilityRevision === 'string' && + typeof candidate.capabilitySha256 === 'string' && + typeof candidate.observedVersion === 'string' && + typeof candidate.target === 'string'; +}; + +/** + * Builds the error for a state outside the contract. The `never` parameter is + * what makes every caller's `default` branch a compile-time exhaustiveness + * check: a fifth state stops being assignable to `never` and fails the build. + */ +export const unknownCapabilityStateError = (capability: never): CapabilityStateError => { + const { state } = capability as { readonly state?: unknown }; + return new CapabilityStateError( + `Capability state ${JSON.stringify(state) ?? 'undefined'} is outside the ` + + `${capabilityStateNames.join('/')} contract.`, + ); +}; + +/** + * Validates an untyped capability declaration, including the fields each state + * owns. JavaScript and third-party adapters reach the registry unchecked by the + * compiler, so this is the boundary that keeps malformed states out. + */ +export const isCapabilityState = (value: unknown): value is CapabilityState => { + if (typeof value !== 'object' || value === null) return false; + const candidate = value as { readonly evidence?: unknown; readonly reason?: unknown; readonly state?: unknown }; + const { state } = candidate; + if (!isCapabilityStateName(state)) return false; + switch (state) { + case 'supported': + return isCapabilityEvidence(candidate.evidence); + case 'degraded': + return typeof candidate.reason === 'string' && + (candidate.evidence === undefined || isCapabilityEvidence(candidate.evidence)); + case 'unavailable': + case 'prohibited': + return typeof candidate.reason === 'string'; + default: { + const exhaustive: never = state; + throw unknownCapabilityStateError(exhaustive); + } + } +}; diff --git a/packages/agent-bundle/tests/adapter-capability-states.test.ts b/packages/agent-bundle/tests/adapter-capability-states.test.ts index e1aad8a88..cf044177e 100644 --- a/packages/agent-bundle/tests/adapter-capability-states.test.ts +++ b/packages/agent-bundle/tests/adapter-capability-states.test.ts @@ -3,10 +3,13 @@ import { expect, it } from '@rstest/core'; import { capabilityBooleanView, capabilityEvidence, + capabilityIsSupported, intersectCapabilityStates, supportedCapability, + unavailableCapability, } from '../src/adapters/capability-state.ts'; -import { createDefaultRegistry } from '../src/adapters/registry.ts'; +import { TargetRegistry, createDefaultRegistry } from '../src/adapters/registry.ts'; +import { CapabilityStateError, isCapabilityState } from '../src/core/capabilities.ts'; import type { CapabilityEvidence, CapabilityState } from '../src/core/capabilities.ts'; const evidence = (target: string): CapabilityEvidence => Object.freeze({ @@ -104,6 +107,69 @@ it('keeps the Boolean compatibility view thin and exhaustive', () => { }); }); +const malformed = (value: unknown): CapabilityState => value as CapabilityState; + +it('recognizes only the four contract states with their required fields', () => { + expect(isCapabilityState(supportedCapability(evidence('cursor')))).toBe(true); + expect(isCapabilityState({ state: 'degraded', reason: 'partial' })).toBe(true); + expect(isCapabilityState({ state: 'degraded', reason: 'partial', evidence: evidence('cursor') })).toBe(true); + expect(isCapabilityState(unavailableCapability('missing'))).toBe(true); + expect(isCapabilityState({ state: 'prohibited', reason: 'policy' })).toBe(true); + + // A misspelled state, a state missing the fields it owns, and non-records are all rejected. + expect(isCapabilityState({ state: 'suported' })).toBe(false); + expect(isCapabilityState({ state: 'supported' })).toBe(false); + expect(isCapabilityState({ state: 'supported', evidence: { target: 'cursor' } })).toBe(false); + expect(isCapabilityState({ state: 'unavailable' })).toBe(false); + expect(isCapabilityState({ state: 'degraded', reason: 7 })).toBe(false); + expect(isCapabilityState(undefined)).toBe(false); + expect(isCapabilityState(null)).toBe(false); + expect(isCapabilityState('supported')).toBe(false); +}); + +it('raises a typed error for an unknown state instead of fabricating a truthy one', () => { + const unknown = malformed({ state: 'suported' }); + const supported = supportedCapability(evidence('cursor')); + + // The bug this covers: the exhaustive default returned the capability object, + // so an untyped adapter's typo read as truthy support. + expect(() => capabilityIsSupported(unknown)).toThrow(CapabilityStateError); + expect(() => capabilityIsSupported(unknown)).toThrow(/outside the degraded\/prohibited\/supported\/unavailable contract/u); + expect(() => capabilityBooleanView({ mcp: unknown })).toThrow(CapabilityStateError); + expect(() => intersectCapabilityStates(unknown, supported)).toThrow(CapabilityStateError); + expect(() => intersectCapabilityStates(supported, unknown)).toThrow(CapabilityStateError); + + const thrown = (() => { + try { + capabilityIsSupported(unknown); + return undefined; + } catch (error) { + return error; + } + })(); + expect(thrown).toBeInstanceOf(CapabilityStateError); + if (!(thrown instanceof CapabilityStateError)) throw new Error('Expected a CapabilityStateError.'); + expect(thrown.code).toBe('ERR_UNKNOWN_CAPABILITY_STATE'); + expect(thrown.message).toContain('"suported"'); +}); + +it('rejects a malformed capability declaration when the adapter registers', () => { + const source = createDefaultRegistry().get('cursor'); + + for (const broken of [{ state: 'suported' }, { state: 'supported' }, { state: 'unavailable' }, 'supported']) { + expect(() => new TargetRegistry().register({ + ...source, + capabilities: { ...source.capabilities, mcp: malformed(broken) }, + })).toThrow(CapabilityStateError); + } + expect(() => new TargetRegistry().register({ + ...source, + capabilities: { ...source.capabilities, mcp: malformed({ state: 'suported' }) }, + })).toThrow(/capability "mcp" must declare one of degraded\/prohibited\/supported\/unavailable/u); + + expect(() => new TargetRegistry().register(source)).not.toThrow(); +}); + it('surfaces built-in adapter metadata as immutable capability evidence', () => { const registry = createDefaultRegistry(); const cursor = registry.get('cursor'); diff --git a/packages/agent-bundle/tests/cursor-adapter.test.ts b/packages/agent-bundle/tests/cursor-adapter.test.ts index 3fc437719..10adb7511 100644 --- a/packages/agent-bundle/tests/cursor-adapter.test.ts +++ b/packages/agent-bundle/tests/cursor-adapter.test.ts @@ -5,8 +5,11 @@ import { cursorAdapter, cursorHooksValidator, cursorMcpValidator, + cursorPluginNameError, cursorPluginValidator, + isValidCursorPluginName, } from '../src/adapters/cursor.ts'; +import { pluginAdapter } from '../src/adapters/plugin.ts'; import { readTargetMcpServers } from '../src/services/mcp-runtime.ts'; import { pathTokens, type NormalizedPlugin } from '../src/core/types.ts'; @@ -88,6 +91,38 @@ it('registers cursor as a first-class target with pinned schema validation', () ]); }); +it('holds the 64-character plugin-name bound in both Cursor-producing planners', () => { + const boundary = 'c'.repeat(64); + const overLong = 'c'.repeat(65); + + // The pinned official schema (cursor/plugins@0701892) constrains the name's + // charset but carries no maxLength, so only the planners can hold the bound. + expect(cursorPluginValidator({ name: overLong, version: '1.2.3' })).toBe(true); + expect(isValidCursorPluginName(boundary)).toBe(true); + expect(isValidCursorPluginName(overLong)).toBe(false); + + const model = plugin(); + const named = (name: string): NormalizedPlugin => ({ + ...model, + metadata: { ...model.metadata, name }, + targets: [ + ...model.targets, + { id: 'target:plugin', name: 'plugin', provenance: { kind: 'config', sourcePath: configPath } }, + ], + }); + + expect(cursorAdapter.plan(named(overLong)).diagnostics.filter((entry) => entry.code === 'cursor.name')).toEqual([ + { code: 'cursor.name', message: cursorPluginNameError(overLong), severity: 'error', target: 'cursor' }, + ]); + expect(pluginAdapter.plan(named(overLong)).diagnostics.filter((entry) => entry.code === 'plugin.cursor.name')).toEqual([ + { code: 'plugin.cursor.name', message: cursorPluginNameError(overLong), severity: 'error', target: 'plugin' }, + ]); + + for (const plan of [cursorAdapter.plan(named(boundary)), pluginAdapter.plan(named(boundary))]) { + expect(plan.diagnostics.filter((entry) => entry.code.endsWith('cursor.name'))).toEqual([]); + } +}); + it('validates Cursor documents against the vendored real-host schemas', () => { expect(cursorPluginValidator({ minClientVersions: { cursor: '3.5.0' },