diff --git a/docs/PLUGIN_AUTHORING.md b/docs/PLUGIN_AUTHORING.md index be1efe94b..79f3d7195 100644 --- a/docs/PLUGIN_AUTHORING.md +++ b/docs/PLUGIN_AUTHORING.md @@ -194,6 +194,26 @@ ctx.commands.register({ }) ``` +Register a plain object. The registry stores a shallow copy of what you pass +and runs its shape checks on that copy, so only *own enumerable* properties +survive: a class instance whose `run()` lives on its prototype, or a member +defined non-enumerable, is refused with `missing run()` even though the +registration visibly declares it. TypeScript cannot warn you here, because it +has no notion of property ownership, so the error arrives at runtime as a +`plugin.activate_failed` log line and the plugin does not load. + +Only the four required members are checked, so only they are refused. An +optional member the copy leaves behind (`plugin`, `aliases`, `hidden`, +`audience`, a `help` string) is dropped with no error at all: registration +succeeds and the command runs with that member simply absent, so a +prototype-resident `aliases` is a dead alias and a prototype-resident +`hidden` still lists in `hyp --help`. `plugin` is worth naming separately, +because the registry derives `category` and `audience` from it: losing it +does not leave a field blank, it files the command under a category named +after the first word of its own name and gives it the `everyday` audience +instead of `operator`. Assign optional members onto the instance too, or +register a plain object. + Every declared command is public CLI surface: it appears in `hyp --help` and in its group's subcommand table, and a visible diagnostic should carry a `help` string explaining what its output means. A command whose caller is a diff --git a/hypaware-plugin-kernel-types.d.ts b/hypaware-plugin-kernel-types.d.ts index f8441354d..7bb73e64e 100644 --- a/hypaware-plugin-kernel-types.d.ts +++ b/hypaware-plugin-kernel-types.d.ts @@ -949,6 +949,12 @@ export interface CommandRegistry { * The copy is own enumerable properties only, and the shape checks run * on it, so a registration whose members live on a prototype (a class * instance) is rejected here rather than stored half-formed. + * + * This declaration cannot express that rule: TypeScript has no notion of + * property ownership or enumerability, so a class whose `run()` sits on + * its prototype satisfies `CommandRegistration` under `--strict` and then + * throws at this call. Register a plain object, or assign the members onto + * the instance itself. */ register(command: CommandRegistration): void /** diff --git a/src/core/registry/commands.js b/src/core/registry/commands.js index 31548fd2a..8e757d94f 100644 --- a/src/core/registry/commands.js +++ b/src/core/registry/commands.js @@ -60,16 +60,24 @@ export function createCommandRegistry() { /** @type {CommandRegistration} */ const record = { ...command } if (typeof record.name !== 'string' || record.name.length === 0) { - throw new TypeError('CommandRegistry.register: command.name must be a non-empty string') + throw new TypeError( + `CommandRegistry.register: command.name must be a non-empty string${copyMiss(command, record, 'name')}` + ) } if (typeof record.summary !== 'string') { - throw new TypeError(`CommandRegistry.register: '${record.name}' missing summary`) + throw new TypeError( + `CommandRegistry.register: '${record.name}' missing summary${copyMiss(command, record, 'summary')}` + ) } if (typeof record.usage !== 'string') { - throw new TypeError(`CommandRegistry.register: '${record.name}' missing usage`) + throw new TypeError( + `CommandRegistry.register: '${record.name}' missing usage${copyMiss(command, record, 'usage')}` + ) } if (typeof record.run !== 'function') { - throw new TypeError(`CommandRegistry.register: '${record.name}' missing run()`) + throw new TypeError( + `CommandRegistry.register: '${record.name}' missing run()${copyMiss(command, record, 'run')}` + ) } // Fill the common metadata at the registry boundary so third-party // commands participate without boilerplate. Canonical registrations can @@ -229,3 +237,47 @@ export function createCommandRegistry() { return { register, registerGroup, unregister, get, getGroup, listGroups, list, has, size, match } } + +/** + * Explain a shape check the stored record failed but the registration as + * passed would have satisfied. The record is `{ ...command }`, which carries + * own enumerable properties and nothing else, so a member living on a + * prototype (a class instance, an `Object.create` registration) or defined + * non-enumerable is simply not in what the checks read. + * + * The published `CommandRegistration` type cannot warn about it up front: + * TypeScript has no notion of property ownership or enumerability, so a class + * whose `run()` sits on the prototype compiles clean under `--strict`. And a + * plugin whose `activate()` throws is caught per plugin and logged as + * `plugin.activate_failed`, so the plugin simply does not load. That leaves + * this clause as the whole diagnosis its author gets, and a bare + * `missing run()` about a registration that visibly declares `run()` sends + * them looking in the wrong place. + * + * @param {CommandRegistration} command the registration as passed + * @param {CommandRegistration} record the own-enumerable copy the checks read + * @param {string} key the member the check rejected + * @returns {string} a clause to append, or '' when the member is genuinely + * absent and there is nothing to explain + */ +function copyMiss(command, record, key) { + if (key in record) return '' + // Presence, not value. Reading `command[key]` would run a prototype + // accessor, and a class instance is one of the shapes this clause exists to + // diagnose: a lazily-initializing getter would fire on a path that rejects, + // against the promise above that a rejected registration comes back exactly + // as it arrived, and a throwing one would replace this boundary error with + // its own, which is the opposite of what this function is for. `in` walks + // the chain without invoking anything, and the `has` trap of a Proxy + // registration, the one thing left that can object, does not get to break + // the error either. + try { + if (!(key in /** @type {any} */ (command))) return '' + } catch { + return '' + } + return ( + ` - '${key}' is reachable on the registration but is not an own enumerable property, ` + + "so the registry's copy did not carry it (a prototype member, or one defined non-enumerable)" + ) +} diff --git a/test/core/command-registry-register.test.js b/test/core/command-registry-register.test.js index 701783a2e..8b450161f 100644 --- a/test/core/command-registry-register.test.js +++ b/test/core/command-registry-register.test.js @@ -146,3 +146,111 @@ test('the run() the checks accepted is the run() the registry stores', () => { commands.register(/** @type {any} */ (shifty)) assert.equal(commands.get('shifty')?.run, accepted) }) + +// The compiler cannot warn about any of this. A class instance whose `run()` +// lives on the prototype satisfies `CommandRegistration` under `tsc --strict`, +// because TypeScript's type system has no notion of own or enumerable +// properties, and `hypaware-plugin-kernel-types.d.ts` is published, so +// `register` is a third-party API. That leaves the boundary error as the whole +// diagnosis, read out of a `plugin.activate_failed` log line after the plugin +// quietly failed to load - and "missing run()" about a registration that +// visibly declares `run()` sends the author looking in the wrong place. +test('the boundary error says why a member did not survive the copy', () => { + const commands = createCommandRegistry() + class Prototyped { + constructor() { + this.name = 'prototyped' + this.summary = 'run() lives on the prototype' + this.usage = 'hyp prototyped' + } + async run() { + return 0 + } + } + assert.throws( + () => commands.register(/** @type {any} */ (new Prototyped())), + /'prototyped' missing run\(\).*'run' is reachable on the registration but is not an own enumerable property/s + ) + + // Same cause, a different member, and reached through `Object.create` + // rather than through a class. + const inherited = Object.create({ summary: 'inherited', usage: 'hyp inherited', run: async () => 0 }) + inherited.name = 'inherited' + assert.throws( + () => commands.register(inherited), + /'inherited' missing summary.*'summary' is reachable on the registration but is not an own enumerable property/s + ) + + // Own, but not enumerable, so the spread does not carry it either. + const hidden = makeCommand({ name: 'hidden' }) + delete hidden.run + Object.defineProperty(hidden, 'run', { value: async () => 0, enumerable: false }) + assert.throws( + () => commands.register(hidden), + /'hidden' missing run\(\).*is not an own enumerable property/s + ) +}) + +// The diagnosis has to stay off a registration that really is incomplete, +// or it would send the next author hunting a prototype that is not there. +test('a genuinely absent member is reported without the copy diagnosis', () => { + const commands = createCommandRegistry() + const bare = makeCommand() + delete bare.run + // Anchored: nothing follows "missing run()" when there is nothing to explain. + assert.throws(() => commands.register(bare), /'demo' missing run\(\)$/) +}) + +// The clause has to read the argument to know the member was reachable, and +// one of the shapes it exists to diagnose puts that member on a prototype - +// where reading it can run caller code. A registration this function rejects +// comes back exactly as it arrived, and a getter that throws must not replace +// the boundary error with its own. +test('the copy diagnosis does not run a prototype accessor to make its case', () => { + const commands = createCommandRegistry() + let reads = 0 + class Lazy { + constructor() { + this.name = 'lazy' + this.summary = 'run() is built on first read' + this.usage = 'hyp lazy' + } + get run() { + reads += 1 + throw new Error('provider not configured yet') + } + } + assert.throws( + () => commands.register(/** @type {any} */ (new Lazy())), + /'lazy' missing run\(\).*not an own enumerable property/s + ) + assert.equal(reads, 0, 'the rejection path must not invoke the getter') + + // A Proxy is the same argument through two more doors, and they are + // different doors: the spread consults `get`, while `in` consults `has` + // and never `get`. Both assertions are anchored, so a clause appended + // where none belongs fails them too. + const getTrapped = new Proxy( + /** @type {any} */ ({ name: 'trapped', summary: 's', usage: 'hyp trapped' }), + { + get(target, key) { + if (key === 'run') throw new Error('trap boom') + return target[key] + } + } + ) + assert.throws(() => commands.register(getTrapped), /'trapped' missing run\(\)$/) + + // `has` is the one trap `in` does reach, so it is the one thing left that + // can object. It must not get to replace the boundary error either: the + // diagnosis goes quiet and the registry still says what it refused. + const hasTrapped = new Proxy( + /** @type {any} */ ({ name: 'has-trapped', summary: 's', usage: 'hyp has-trapped' }), + { + has() { + throw new Error('has boom') + } + } + ) + assert.throws(() => commands.register(hasTrapped), /'has-trapped' missing run\(\)$/) +})