diff --git a/CHANGELOG.md b/CHANGELOG.md index c421b2e..beafd0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,33 @@ Pre-1.0 releases followed it in spirit; their breaking changes are marked **Brea ## [Unreleased] +### Added + +- `DeclaredContributions.keybindings` for `assertManifestMatches` and + `diffManifest`. Passing it checks that every entry in + `contributes.keybindings` binds a command the extension declares; its `allow` + list names the built-in command ids it may bind as well, which putting a key + on a built-in is a supported use of the contribution point. + + VS Code validates the shape of a keybinding entry and not its command. One + naming an id nothing registers is accepted, given a weight, and does nothing + when the key is pressed — so renaming a command and missing the manifest + leaves a shortcut that fails in silence, with no warning anywhere. Both + extensions this package was trialled against contribute keybindings and had + each written a check of their own: one that every bound command is a declared + one, which is the comparison this now makes, and one pinning the order of two + entries, which this deliberately does not. + + Of a keybinding it reads the command id and nothing else. The key, `when` and + `args` stay the manifest's, and so does the order of the entries — which is + what decides the winner when several share a key, so an extension that leans + on that order still needs an assertion of its own. + + Omitting the field checks no keybindings, which is what every existing caller + does, so this changes nothing for them. `ManifestMismatch['kind']` gains + `'keybinding'`; a caller that switches exhaustively over it will need the new + case. + ## [6.0.0] - 2026-09-14 **A major for one reason: the VS Code floor.** `engines.vscode` moves from diff --git a/docs/guide.md b/docs/guide.md index 0737334..b36a6b6 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -872,6 +872,11 @@ export function checkManifest(manifest: unknown): void { settings: [Settings], commands: Object.values(Contracts), views: ['sample.projects'], + // Opting in checks that every contributed keybinding binds a command + // declared above. VS Code accepts one naming a command nothing registers + // and then does nothing when the key is pressed; `allow` is for the + // built-ins an extension deliberately puts a key on. + keybindings: { allow: ['workbench.action.files.save'] }, }); } ``` @@ -887,6 +892,14 @@ disagreement with the contribution point it concerns, which side is missing it — or `drift`, when both have it and disagree — the id, and the JSON that would settle it when the fix is mechanical. The assertion above is built on it. +Of a keybinding it reads the command id and nothing else. That much is worth +checking because VS Code does not: an entry naming a command nothing registers +passes validation, is given a weight, and does nothing at all when the key is +pressed, so renaming a command and missing the manifest leaves a shortcut that +fails in silence. The key, `when` and `args` remain the manifest's, and so does +the order of the entries — which is what decides the winner when several share +a key, so an extension that leans on that order needs an assertion of its own. + ## The escape hatch diff --git a/docs/samples/manifest.ts b/docs/samples/manifest.ts index 58b3061..c5f6b5e 100644 --- a/docs/samples/manifest.ts +++ b/docs/samples/manifest.ts @@ -27,5 +27,10 @@ export function checkManifest(manifest: unknown): void { settings: [Settings], commands: Object.values(Contracts), views: ['sample.projects'], + // Opting in checks that every contributed keybinding binds a command + // declared above. VS Code accepts one naming a command nothing registers + // and then does nothing when the key is pressed; `allow` is for the + // built-ins an extension deliberately puts a key on. + keybindings: { allow: ['workbench.action.files.save'] }, }); } diff --git a/src/testing/manifest.ts b/src/testing/manifest.ts index e999b9d..12532fe 100644 --- a/src/testing/manifest.ts +++ b/src/testing/manifest.ts @@ -32,6 +32,25 @@ export interface DeclaredContributions { readonly commands?: readonly { readonly descriptor: CommandDescriptor }[]; /** View ids the application registers, independent of manifest container. */ readonly views?: readonly string[]; + /** + * Opts in to checking that every contributed keybinding binds a command this + * extension declares. + * + * VS Code validates only the shape of a keybinding entry, never that its + * command exists: one naming a command nothing registers is accepted, given a + * weight, and does nothing when the key is pressed. A renamed or deleted + * command therefore leaves a shortcut that fails in silence. + * + * Omitting this checks no keybindings at all. + */ + readonly keybindings?: { + /** + * Command ids the manifest may bind without declaring them, for the + * supported case of putting a key on a built-in command such as + * `workbench.action.files.save`. + */ + readonly allow?: readonly string[]; + }; } /** @@ -45,7 +64,7 @@ export interface DeclaredContributions { * a container; a command needs a title). */ export interface ManifestMismatch { - readonly kind: 'command' | 'setting' | 'view'; + readonly kind: 'command' | 'setting' | 'view' | 'keybinding'; readonly direction: 'missing-in-manifest' | 'missing-in-src' | 'drift'; readonly id: string; readonly summary: string; @@ -65,6 +84,7 @@ interface Manifest { readonly properties?: Readonly>>>; }; readonly views?: Readonly>; + readonly keybindings?: readonly { readonly command?: unknown }[]; }; } @@ -122,6 +142,12 @@ function viewIds(manifest: Manifest): readonly string[] { .filter((id): id is string => typeof id === 'string'); } +function boundCommandIds(manifest: Manifest): readonly string[] { + return (manifest.contributes?.keybindings ?? []) + .map((entry) => entry.command) + .filter((id): id is string => typeof id === 'string'); +} + function checkCommands(manifest: Manifest, declared: DeclaredContributions): ManifestMismatch[] { if (declared.commands === undefined) { return []; @@ -265,9 +291,34 @@ function checkViews(manifest: Manifest, declared: DeclaredContributions): Manife ]; } +function checkKeybindings(manifest: Manifest, declared: DeclaredContributions): ManifestMismatch[] { + if (declared.keybindings === undefined) { + return []; + } + const bindable = new Set([ + ...(declared.commands ?? []).map((contract) => contract.descriptor.id), + ...(declared.keybindings.allow ?? []), + ]); + // One report per unresolved id: binding the same command from several entries + // is how a keybinding carries a `when`, and a typo in it is still one mistake. + const unresolved = new Set(boundCommandIds(manifest).filter((id) => !bindable.has(id))); + + return [...unresolved].map((id): ManifestMismatch => ({ + kind: 'keybinding', + direction: 'missing-in-src', + id, + // No `paste`: the fix is a contract, a corrected id or an `allow` entry, + // and which one is a decision the manifest cannot settle. + summary: + `keybinding "${id}" is bound in contributes.keybindings but no contract ` + + `declares it and keybindings.allow does not list it`, + })); +} + /** * Every disagreement between `package.json` and the declarations in `src`, as - * data, in the order the checks run: commands, then settings, then views. + * data, in the order the checks run: commands, then settings, then views, then + * keybindings. * * The same comparison {@link assertManifestMatches} makes, without the throw — * for a tool that wants to print, count or apply the mechanical part of the @@ -290,6 +341,7 @@ export function diffManifest( ...checkCommands(parsed, declared), ...checkSettings(parsed, declared), ...checkViews(parsed, declared), + ...checkKeybindings(parsed, declared), ]; } @@ -311,6 +363,11 @@ export function diffManifest( * localization files or whether VS Code accepts the complete manifest; retain a * packaging/Extension Host lane for those concerns. * + * Of a keybinding it reads the command id and nothing else. The key, `when` and + * `args` stay the manifest's, as does the order of the entries — which is what + * decides the winner when several share a key, so an extension that depends on + * that order needs its own assertion for it. + * * @example * ```ts * it('the manifest agrees with what src declares', () => { @@ -318,6 +375,7 @@ export function diffManifest( * settings: [Settings, EditorSettings], * commands: Object.values(Contracts), * views: Object.values(VIEWS), + * keybindings: { allow: ['workbench.action.files.save'] }, * }); * }); * ``` diff --git a/tests/testing/manifest.test.ts b/tests/testing/manifest.test.ts index 1deeb58..a70231b 100644 --- a/tests/testing/manifest.test.ts +++ b/tests/testing/manifest.test.ts @@ -348,3 +348,112 @@ describe('diffManifest', () => { } }); }); + +/** + * Keybindings that bind a command nothing declares. + * + * VS Code checks the shape of a keybinding entry and not its command: one + * naming an id nothing registers is accepted, given a weight, and silent when + * the key is pressed. Renaming a command and missing the manifest is exactly + * that, and nothing else in this file would notice. + */ +describe('assertManifestMatches, keybindings', () => { + const declared = { commands: [Refresh, Clear], keybindings: {} }; + + function withKeybindings(entries: readonly unknown[]): unknown { + return { + contributes: { + commands: [ + { command: 'sample.refresh', title: 'Refresh' }, + { command: 'sample.clear', title: 'Clear' }, + ], + keybindings: entries, + }, + }; + } + + it('passes when every bound command is declared', () => { + expect(() => { + assertManifestMatches( + withKeybindings([ + { command: 'sample.refresh', key: 'ctrl+r' }, + { command: 'sample.clear', key: 'ctrl+k', when: 'editorTextFocus' }, + ]), + declared + ); + }).not.toThrow(); + }); + + it('names a keybinding whose command nothing declares', () => { + expect(() => { + assertManifestMatches( + withKeybindings([{ command: 'sample.refrehs', key: 'ctrl+r' }]), + declared + ); + }).toThrow(/keybinding "sample\.refrehs" is bound/u); + }); + + it('lets a built-in through only when it is allowed', () => { + const save = 'workbench.action.files.save'; + // Putting a key on a built-in command is a supported use of the point. + expect(() => { + assertManifestMatches(withKeybindings([{ command: save, key: 'ctrl+s' }]), { + ...declared, + keybindings: { allow: [save] }, + }); + }).not.toThrow(); + expect(() => { + assertManifestMatches(withKeybindings([{ command: save, key: 'ctrl+s' }]), declared); + }).toThrow(/keybinding "workbench\.action\.files\.save"/u); + }); + + it('reports once however many entries share the unresolved id', () => { + // Two entries on one command is how a keybinding carries a `when`. + const mismatches = diffManifest( + withKeybindings([ + { command: 'sample.ghost', key: 'ctrl+g' }, + { command: 'sample.ghost', key: 'ctrl+g', when: 'terminalFocus' }, + ]), + declared + ); + + expect(mismatches.map((m) => m.id)).toEqual(['sample.ghost']); + }); + + it('checks no keybindings unless asked', () => { + expect(() => { + assertManifestMatches(withKeybindings([{ command: 'sample.ghost', key: 'ctrl+g' }]), { + commands: [Refresh, Clear], + }); + }).not.toThrow(); + }); + + it('runs after the other checks', () => { + const mismatches = diffManifest( + { + contributes: { + commands: [{ command: 'sample.refresh', title: 'Refresh' }], + keybindings: [{ command: 'sample.ghost', key: 'ctrl+g' }], + }, + }, + declared + ); + + expect(mismatches.map((m) => [m.kind, m.direction, m.id])).toEqual([ + ['command', 'missing-in-manifest', 'sample.clear'], + ['keybinding', 'missing-in-src', 'sample.ghost'], + ]); + }); + + it('has nothing to paste, because the fix is a decision', () => { + const mismatches = diffManifest( + withKeybindings([{ command: 'sample.ghost', key: 'ctrl+g' }]), + declared + ); + + // Asserted before reading `paste`: an empty result would satisfy the + // undefined check while proving nothing. + expect(mismatches).toHaveLength(1); + expect(mismatches[0]?.paste).toBeUndefined(); + }); +});