CommandRegistry.register's refusal names the member the spread left behind (#1007)

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

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
   /**

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)"
+  )
+}

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\(\)$/)
+})