From 042a1c79f9508a0582ffd38eb523b4e770d6c7b0 Mon Sep 17 00:00:00 2001 From: kkdev92 <112151103+kkdev92@users.noreply.github.com> Date: Wed, 16 Sep 2026 19:58:01 +0900 Subject: [PATCH] feat(testing): check that a contributed keybinding binds a declared command VS Code validates the shape of a `contributes.keybindings` entry and never its command. `isValidContributedKeyBinding` checks types only; a keybinding naming a command nothing registers is accepted, given a weight, and does nothing at all when the key is pressed. Renaming or deleting a command and missing the manifest therefore leaves a shortcut that fails in silence, with no warning from the editor and nothing in a test suite to catch it. `DeclaredContributions` gains an optional `keybindings` field. Passing it checks that every entry binds a command the extension declares; its `allow` list names the built-in ids it may bind as well, because putting a key on a built-in is a supported use of the point and would otherwise fail the check. Omitting the field checks no keybindings, so every existing caller is unaffected. Both extensions this was trialled against had already hand-written a check here, which is the argument for having one: quick-utils asserts that every bound command is declared -- the same comparison -- and ClipShot pins the order of a pair of entries, which this deliberately does not do. Of a keybinding this reads the command id and nothing else. The key, `when` and `args` stay the manifest's, and so does the order, which is what decides the winner when several entries share a key. Verified against both consumers' real manifests: neither reports anything as it stands, a typo in a bound command is caught in each, and a built-in is reported without `allow` and accepted with it. Each of the five new assertions that could fail was run against the previous source first, and all five did. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 27 ++++++++ docs/guide.md | 13 ++++ docs/samples/manifest.ts | 5 ++ src/testing/manifest.ts | 62 ++++++++++++++++++- tests/testing/manifest.test.ts | 109 +++++++++++++++++++++++++++++++++ 5 files changed, 214 insertions(+), 2 deletions(-) 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(); + }); +});