From 0144af81a41a9899f248fe970aab998446ead9ca Mon Sep 17 00:00:00 2001 From: philcunliffe Date: Wed, 2 Sep 2026 16:09:59 +0000 Subject: [PATCH 1/3] The registry copy's refusal names the member the spread left behind (#1007) `CommandRegistry.register` copies the registration with a spread and runs its shape checks on the copy, so only own enumerable properties survive. That narrowing is deliberate, but the published `hypaware-plugin-kernel-types.d.ts` cannot express it: TypeScript has no notion of property ownership or enumerability, so class MyCommand implements CommandRegistration { name = 'my cmd'; summary = '...'; usage = '...' run() { ... } // on the prototype } compiles clean under `tsc --strict` (verified) and then throws. Because a failing `activate()` is caught per plugin and logged as `plugin.activate_failed`, the boundary error is the entire diagnosis its author gets, and it read `'my cmd' missing run()` about a registration that visibly declares `run()`. `copyMiss` appends the cause when the rejected member is reachable on the argument but absent from the copy, for all four checked members, and stays silent when the member is genuinely missing so it cannot send the next author hunting a prototype that is not there. The rule is now also stated where a plugin author reads it: the published `register` declaration and the "Registering commands" section of the authoring guide. Refs #1007 --- docs/PLUGIN_AUTHORING.md | 8 +++ hypaware-plugin-kernel-types.d.ts | 6 +++ src/core/registry/commands.js | 47 ++++++++++++++++-- test/core/command-registry-register.test.js | 54 +++++++++++++++++++++ 4 files changed, 111 insertions(+), 4 deletions(-) diff --git a/docs/PLUGIN_AUTHORING.md b/docs/PLUGIN_AUTHORING.md index be1efe94b..d3acdf25c 100644 --- a/docs/PLUGIN_AUTHORING.md +++ b/docs/PLUGIN_AUTHORING.md @@ -194,6 +194,14 @@ 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. + 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..e657a3fd8 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,34 @@ 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 '' + if (/** @type {any} */ (command)[key] === undefined) 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..12de35088 100644 --- a/test/core/command-registry-register.test.js +++ b/test/core/command-registry-register.test.js @@ -146,3 +146,57 @@ 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\(\)$/) +}) From e0bd021569069e7833ea7b167445050d51f7420a Mon Sep 17 00:00:00 2001 From: philcunliffe Date: Wed, 2 Sep 2026 16:55:43 +0000 Subject: [PATCH 2/3] The copy diagnosis reads presence, not value, so it cannot run caller code Review of #1221. copyMiss read `command[key]` to decide the member was reachable. On the exact shape it exists to diagnose - a class instance - that read goes through the prototype, where it can run an accessor: a getter that throws replaced the boundary TypeError with its own unrelated message, which is the opposite of what the clause is for, and a lazily-initializing getter fired on a path that rejects, against this function's own promise that a rejected registration comes back exactly as it arrived. `in` walks the chain without invoking anything, and a Proxy `has` trap that objects is caught rather than allowed to break the error. Also: the authoring note taught that a member lost to the spread is loudly refused, but only the four required members are checked. An optional one (`aliases`, `hidden`, `audience`, `help`) is dropped silently, so a prototype-resident `aliases` is a dead alias and a prototype-resident `hidden` still lists in `hyp --help`. Said so. Refs #1007 --- docs/PLUGIN_AUTHORING.md | 8 +++++ src/core/registry/commands.js | 15 +++++++- test/core/command-registry-register.test.js | 38 +++++++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/docs/PLUGIN_AUTHORING.md b/docs/PLUGIN_AUTHORING.md index d3acdf25c..585b70774 100644 --- a/docs/PLUGIN_AUTHORING.md +++ b/docs/PLUGIN_AUTHORING.md @@ -202,6 +202,14 @@ 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 (`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`. 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/src/core/registry/commands.js b/src/core/registry/commands.js index e657a3fd8..8e757d94f 100644 --- a/src/core/registry/commands.js +++ b/src/core/registry/commands.js @@ -262,7 +262,20 @@ export function createCommandRegistry() { */ function copyMiss(command, record, key) { if (key in record) return '' - if (/** @type {any} */ (command)[key] === undefined) 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 12de35088..580d12d2e 100644 --- a/test/core/command-registry-register.test.js +++ b/test/core/command-registry-register.test.js @@ -200,3 +200,41 @@ test('a genuinely absent member is reported without the copy diagnosis', () => { // 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 a different door. + const trapped = 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(trapped), /'trapped' missing run\(\)/) +}) From 6ac7f8406d5e25f408f231a74120b63724017d5e Mon Sep 17 00:00:00 2001 From: philcunliffe Date: Wed, 2 Sep 2026 17:39:01 +0000 Subject: [PATCH 3/3] The Proxy case tests the trap `in` actually reaches, and the doc names `plugin` The Proxy sub-case of the accessor test used a `get` trap, but `in` consults `has` and never `get`, so the `try`/`catch` it was written to justify was never entered: removing the `try`/`catch` left the suite green. Its assertion was also unanchored, so it passed whether or not the clause was appended. Anchor it, and add the throwing `has` trap alongside it. All three branches of `copyMiss` are now gated: reverting to the value read, dropping the `try`/`catch`, and dropping the genuinely-absent guard each fail a test. The optional-member paragraph listed `aliases`, `hidden`, `audience` and `help` but not `plugin`, which is the member a plugin registration is most likely to carry and the one whose loss is not simply an absent field: the registry derives `category` and `audience` from it, so a prototype-resident `plugin` files the command under a category named after the first word of its own name and gives it the `everyday` audience instead of `operator`. Co-Authored-By: Claude Opus 5 (1M context) --- docs/PLUGIN_AUTHORING.md | 16 +++++++++------ test/core/command-registry-register.test.js | 22 ++++++++++++++++++--- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/docs/PLUGIN_AUTHORING.md b/docs/PLUGIN_AUTHORING.md index 585b70774..79f3d7195 100644 --- a/docs/PLUGIN_AUTHORING.md +++ b/docs/PLUGIN_AUTHORING.md @@ -203,12 +203,16 @@ 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 (`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`. Assign optional members onto the instance too, or register a -plain object. +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 diff --git a/test/core/command-registry-register.test.js b/test/core/command-registry-register.test.js index 580d12d2e..8b450161f 100644 --- a/test/core/command-registry-register.test.js +++ b/test/core/command-registry-register.test.js @@ -226,8 +226,11 @@ test('the copy diagnosis does not run a prototype accessor to make its case', () ) assert.equal(reads, 0, 'the rejection path must not invoke the getter') - // A Proxy is the same argument through a different door. - const trapped = new Proxy( + // 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) { @@ -236,5 +239,18 @@ test('the copy diagnosis does not run a prototype accessor to make its case', () } } ) - assert.throws(() => commands.register(trapped), /'trapped' missing run\(\)/) + 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\(\)$/) })