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

[philcunliffe]

What this fixes

Issue #1007, verified still live on c0daf4e2: PR #1004 (merged 46bf5700) made
CommandRegistry.register copy the registration with a spread and run its shape
checks on the copy, so only own enumerable properties survive. The narrowing
is deliberate and correct. The gap is that nothing tells the plugin author.

Reproduced both halves at c0daf4e2:

# runtime, against src/core/registry/commands.js
class:          THROWS -> CommandRegistry.register: 'my cmd' missing run()
Object.create:  THROWS -> CommandRegistry.register: 'oc' missing summary
non-enumerable: THROWS -> CommandRegistry.register: 'ne' missing run()

// types, against the published hypaware-plugin-kernel-types.d.ts
class MyCommand implements CommandRegistration {
  name = 'my cmd'; summary = 's'; usage = 'u'
  async run(argv: string[], ctx: CommandRunContext): Promise<number> { return 0 }
}
declare const reg: CommandRegistry
reg.register(new MyCommand())     // tsc --strict --noEmit  ->  exit 0

Why not the issue's option 1 (tighten the type)

Option 1 is not expressible. TypeScript has no notion of property ownership or
enumerability, so no declaration can separate an own-enumerable run from an
inherited or non-enumerable one. The one structural angle that looks promising,
T extends { constructor: ObjectConstructor }, resolves to false for a plain
object literal, an interface, and a class instance alike, so it rejects
everything rather than distinguishing anything:

type IsPlain<T> = T extends { constructor: ObjectConstructor } ? true : false
type A = IsPlain<{ x: number }>   // false
type B = IsPlain<class instance>  // false
type D = IsPlain<interface>       // false

Nothing in the shipped .d.ts is tightened here, because nothing can be
without rejecting the shapes that do work.

What it does instead

The failure mode the issue is actually worried about is a third-party plugin
that silently does not load: src/core/runtime/loader.js:108 catches per plugin
and logs plugin.activate_failed, so the boundary error text is the entire
diagnosis its author gets. That text was actively misleading, saying
missing run() about a registration that visibly declares run().

• copyMiss(command, record, key) appends the cause when the rejected member is
  reachable on the argument but absent from the own-enumerable copy, applied to
  all four checked members (name, summary, usage, run):

  CommandRegistry.register: 'my cmd' missing run() - 'run' 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)
  

  It stays silent when the member is genuinely absent, so it cannot send the
  next author hunting a prototype that is not there. Reachability is decided
  by presence (key in command) inside a try/catch, never by a value
  read: a prototype accessor is not invoked on the rejection path, and a Proxy
  has trap that throws cannot replace the boundary error with its own.

• The rule is stated where a plugin author reads it: the published register
  declaration (which now says explicitly that the declaration cannot express
  it and what to do instead) and the "Registering commands" section of
  docs/PLUGIN_AUTHORING.md. That is the issue's option 2, in the durable place
  rather than in release notes.

Evidence

Gating tests: the boundary error says why a member did not survive the copy,
covering all three rejected shapes; a companion asserting a genuinely absent
member is reported without the diagnosis (anchored /missing run\(\)$/); and
the copy diagnosis does not run a prototype accessor to make its case, which
pins the accessor at zero invocations and gates both Proxy traps with anchored
assertions.
A runtime test is the right gate here rather than a typecheck assertion,
because the fix is not a type change: the whole point is that the type cannot
carry the constraint, so the only place the rule can be asserted is the error
the registry actually raises.

# revert proof at 6ac7f840: src/core/registry/commands.js at origin/master,
# tests and docs kept
not ok 10 - the boundary error says why a member did not survive the copy
not ok 12 - the copy diagnosis does not run a prototype accessor to make its case
# tests 12  # pass 10  # fail 2
# restored -> 12/12

# mutation proof: dropping copyMiss's try/catch fails test 12
# restored -> 12/12

npm test: 5799 tests, 5798 pass, 0 fail, 1 skipped. npm run typecheck: clean.

Review corrections

Two review rounds, both approvals with the findings fixed in-branch:

• Round 1 (e0bd0215): copyMiss originally read command[key] to decide
  the member was reachable, which ran caller code on the rejection path - a
  prototype getter is one of the very shapes the clause diagnoses, so a
  lazily-initializing getter fired on a path that rejects and a throwing one
  replaced the boundary error with its own. Replaced with the presence check
  (key in command) in try/catch, gated by a test asserting the getter
  is invoked zero times. Round 1 also found the authoring note implied every
  member lost to the spread is loudly refused; only the four required members
  are checked, so the doc now states that optional members are dropped with no
  error at all.
• Round 2 (6ac7f840): the Proxy sub-case used a get trap, but in
  consults has, so the try/catch it was written to justify went
  untested (removing it left the suite green) and its assertion was
  unanchored. Anchored it and added a throwing has trap beside it; all
  three copyMiss branches now fail a test when mutated. Round 2 also added
  plugin to the optional-member doc with its measured consequence: the
  registry derives category and audience from it, so a
  prototype-resident plugin 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.

Scope

No LLP: this is a bug fix and a doc statement of an already-settled rule, not a
new decision. No new dependencies, no new config keys or schema fields.

Refs, not Fixes: the issue's option 1 stays open in the sense that the
compiler still will not reject these shapes, and that cannot be changed. If the
maintainers agree option 1 is dead, #1007 can be closed on this; the remaining
piece is naming the three rejected shapes and the two snapshotted semantics in
the release notes of whichever release carries #1004.

Refs #1007

[philcunliffe]

Verdict: approve with two low findings, both fixed on e0bd0215

Reviewed 0144af81 in a detached worktree against origin/master. The central
decision holds and the fix is the right one. Two low findings, fixed here rather
than sent back.

The impossibility claim: confirmed, independently

This is the load-bearing claim, so I tried to refute it rather than read it.
I compiled twelve candidate constructions under tsc 7.0.2 --strict.
I could not build any type that separates the shapes. Every angle either
accepts all three or rejects all three:

construction	plain object	class instance	interface
T extends { constructor: ObjectConstructor }	false	false	false
T extends { constructor: infer C }	Function	Function	-
keyof T	includes run	includes run	includes run
Clone<T> extends T && T extends Clone<T>	true	identical	-
mutual assignability Klass vs IFace vs non-enumerable literal	mutually assignable in every pair
property-style vs method-style run	both satisfy both
run: (this: void, ...)	satisfied	satisfied	-
unique symbol brand as ?: never	satisfied	satisfied	-
T extends abstract new (...a:any)=>any	false	false	-
T extends { prototype: any }	false	false	-

The PR body's ObjectConstructor table reproduces exactly, including that it is
false for the plain object literal too, i.e. it rejects everything rather than
distinguishing anything. register(new Klass()) compiles with exit 0 under
--strict, as claimed. Issue #1007's option 1 is genuinely not expressible, so
fixing the error text instead of the type is correct, and Refs #1007 rather
than Fixes is right: closingIssuesReferences is [], correctly.

The error text: correct in all four directions

Constructed each shape and read the actual message:

shape	message
prototype member (class)	'proto' missing run() - 'run' is reachable ... not an own enumerable property
own non-enumerable	'nonenum' missing run() - ... not an own enumerable property
Object.create inherited	'oc' missing summary - 'summary' is reachable ...
genuinely absent	'demo' missing run() (silent, correct)

Also silent, correctly, for own-enumerable-but-wrong-type (run: 'nope'),
own-enumerable-undefined, and summary: 7. The key in record guard is the
right ordering: none of name/summary/usage/run exist on Object.prototype,
so it cannot false-negative. The PR's gating test is real: reverting only
src/core/registry/commands.js to origin/master and keeping the tests fails
test 10. A runtime test rather than a typecheck assertion is the correct call,
for the reason the PR gives.

Finding 1 (low, fixed) - the diagnosis ran caller code to make its case

src/core/registry/commands.js:265 read command[key] a second time, through
the prototype chain. A prototype accessor is one of the very shapes this clause
exists to diagnose, so the read invoked caller code on the rejection path, inside
the throw expression. Measured base vs head with a lazily-initializing getter:

class Lazy {
  constructor() { this.name='lazy'; this.summary='s'; this.usage='u' }
  get run() { throw new Error('provider not configured yet') }
}

origin/master : TypeError: CommandRegistry.register: 'lazy' missing run()
                prototype getter invoked: 0 times
0144af81      : Error: provider not configured yet
                prototype getter invoked: 1 time

The author of a plugin using a lazy accessor got an unrelated message in their
plugin.activate_failed line, which is the exact harm this PR set out to remove,
and the getter's side effect fired on a path that rejects, against this
function's own stated promise (commands.js:47-51) that a rejected registration
"comes back exactly as it arrived". Same result through a Proxy get trap
(Error: trap boom).

Fixed by testing presence rather than value: key in command walks the
prototype chain without invoking anything, wrapped in try/catch so a Proxy
has trap cannot break the boundary error either. Both hostile shapes now give
the correct diagnosis, and the getter is invoked 0 times. Gated by a new test,
the copy diagnosis does not run a prototype accessor to make its case, which
asserts the message and reads === 0; reverting only the copyMiss change
fails it (11/12).

Finding 2 (low, fixed) - the doc teaches that the failure is always loud

docs/PLUGIN_AUTHORING.md:197. The new paragraph says a prototype-resident
member "is refused", but only the four required members are checked. An
optional member lost to the same spread vanishes with no signal. Verified: a
class assigning name/summary/usage/run onto the instance but carrying
get aliases() and get hidden() on the prototype registers successfully,
yet commands.get('thalias') is undefined (dead alias) and hidden is
undefined, so audience defaults to everyday and a command that asked to be
hidden lists in hyp --help. An author who reads this paragraph, converts to a
class, and sees registration succeed will reasonably conclude they are safe.

Fixed with a paragraph stating that optional members are dropped without an
error, and what to do. Doc-only: detecting them proactively would need a new
warning path, which is more than this bug calls for.

Other checks, all clean

• Non-object input is already guarded upstream (commands.js:44), so
  register(null) / (undefined) / (42) still give
  command must be an object; copyMiss is never reached with a non-object.
  A null-prototype registration with complete own-enumerable members registers
  normally.
• copyMiss is now provably non-throwing on every hostile input tried.
• Style per CLAUDE.md: no semicolons, no em dashes, no NUL, no @typedef, no
  inline import('...') in a type position. The kernel-types edit is
  comment-only inside the hand-written published .d.ts, which ships via
  package.json files, so the note reaches consumers directly.
• npm test 5798 pass / 0 fail / 1 skipped. npm run typecheck clean.
  npm run build:types emits clean.
• No caller anywhere in the repo matches on these message strings.
• No LLP needed: a bug fix and a doc statement of an already-settled rule.

[philcunliffe]

Verdict: approve. Round 1's fix has no hole; two low findings, both fixed on 6ac7f840

Round 2 reviewed e0bd0215 in a detached worktree against origin/master. The
job was to scrutinise round 1's own finding: key in command still reaches a
Proxy has trap, so I attacked that, plus every other exotic command shape I
could build.

Round 1's key in command fix: no hole found

Ran each hostile shape against the head's commands.js and read the actual
error:

shape	result
has trap throws	'ht' missing run() - clause suppressed, boundary error intact
has trap lies true for an absent key	clause appended (wrong, but the proxy lied to it)
has trap lies false for a present non-enumerable key	clause suppressed - degrades to the old message
get trap throws on run	'tr' missing run(); trap fired only for name,summary,usage (the spread), never for run
class with throwing get run()	correct clause, reads === 0
null-prototype, complete	registers normally
null-prototype, missing run	'np2' missing run(), correctly silent
revoked Proxy	Cannot perform 'ownKeys' on a proxy that has been revoked
command is a function	command must be an object

Nothing here is a hole introduced by this PR. The two has-trap lies are a
caller lying to a diagnostic about itself, and both degrade to the message
master already gave, so neither is worse than the status quo. The revoked
Proxy throws at commands.js:60 on the spread, before copyMiss is ever
reached - unchanged by this PR and unreachable from it. A function is rejected
by the existing guard at commands.js:44.

Four-shape correctness still holds after round 1's change: prototype member
and own non-enumerable both give 'run' is reachable ... not an own enumerable property; Object.create gives the same for summary; a genuinely absent
member stays silent ('ab' missing run()).

The gate is real, and now covers all three branches

Reverting only src/core/registry/commands.js to origin/master and keeping
the tests fails 2 of 12. Mutating just the copyMiss body:

mutation	before this round	now
value read (command[key]) instead of in	fails test 12	fails test 12
try/catch removed	12/12 pass	fails test 12
genuinely-absent guard removed	fails test 11	fails tests 11 and 12

Finding 1 (low, fixed) - the Proxy case tested a trap in never reaches

test/core/command-registry-register.test.js:229. The sub-case was written to
justify the new try/catch, but it used a get trap, and in consults
[[HasProperty]], i.e. has, never get. I instrumented it: during
{ ...command } the trap fires for name, summary, usage only (run is
not an own key of the target), and copyMiss's key in command resolves
ordinarily to false. So the catch was never entered - removing the whole
try/catch left the suite at 12/12. The assertion was also unanchored
(/'trapped' missing run\(\)/), so it passed whether or not a clause was
appended.

The get-trap case is still worth keeping (it does gate a regression back to
the value read), so I anchored it and added the throwing has trap
beside it, also anchored. Both branches are now gated, per the table above.

Finding 2 (low, fixed) - the optional-member list omitted plugin

docs/PLUGIN_AUTHORING.md:205. The new paragraph listed aliases, hidden,
audience and a help string, and told the author the command "runs with that
member simply absent". plugin is the member a plugin registration is most
likely to carry - the code example directly above the paragraph shows
plugin: PLUGIN_NAME - and its loss is not simply an absent field, because
commands.js:86-87 derives two others from it:

record.category ??= record.plugin ? 'additional' : record.name.split(' ')[0]
record.audience ??= record.hidden ? 'machine' : record.category === 'additional' ? 'operator' : ...

Measured with a class carrying get plugin(), get aliases(), get hidden()
on the prototype and the four required members on the instance:

registered: true
get('ws')  -> undefined      (dead alias)
hidden     -> undefined      (a command that asked to be hidden lists in hyp --help)
plugin     -> undefined
category   -> 'widget'       (plain-object control: 'additional')
audience   -> 'everyday'     (plain-object control: 'machine'; without hidden, 'operator')

So the round-1 paragraph's aliases and hidden claims are accurate as
written - I reproduced both. Fixed by adding plugin to the list with
its actual consequence. I deliberately did not claim it corrupts the
hyp --help sectioning: helpSectionFor (dispatch.js:883) maps any category
outside HELP_SECTIONS back to additional, so the section is unchanged and
only the derived audience is wrong.

Other checks, all clean

• Refs #1007 remains right: closingIssuesReferences is [].
• The impossibility claim was independently confirmed in round 1 across 12
  compiled constructions; nothing in this round's diff touches types.
• The .d.ts edit is comment-only in the hand-written published file: no
  @typedef, no inline import('...') in a type position, no TypeScript
  syntax added to src/.
• No em dashes, no semicolons, no NUL bytes in the changed files.
• No caller in the repo matches on these message strings.
• No LLP needed: a bug fix plus a doc statement of an already-settled rule.

Verification on 6ac7f840

npm test          5799 tests, 5798 pass, 0 fail, 1 skipped
npm run typecheck clean
npm run build:types clean

[philcunliffe]

Triage of 6ac7f840 after both review rounds: no unresolved residual is a
blocker. Deferred items are recorded in #1226:

1. Proactive warning when an optional member (plugin, aliases, hidden,
   audience, help) is lost to the spread copy - a new warning path, beyond
   this bug fix; the docs added here state the behaviour (preference).
2. Maintainer disposition of Published plugin types do not enforce what CommandRegistry.register now enforces at runtime #1007's option 1, independently confirmed
   inexpressible in round 1; Refs #1007 is the honest link and the issue
   stays open for a maintainer to close or keep (preference).
3. The release-notes item from this PR's Scope section (preference).

Verified on 6ac7f840 in a clean worktree: npm test 5799 tests, 5798 pass,
0 fail, 1 skipped; npm run typecheck clean; the scoped registry file runs
12/12, and reverting only src/core/registry/commands.js to master fails
tests 10 and 12. The PR body's evidence figures predated the two review
corrections; corrected in place (the body is the squash message).

[philcunliffe]

Ship risk: low

Who could be affected: Only people writing their own HypAware plugins, and only at the moment a plugin fails to start.

What could happen: Nothing new goes wrong. This change adds a sentence to an error message that was already being shown, explaining why a command the plugin clearly declares was still refused. Plugins that worked before still work; plugins that were already failing still fail, for the same reason, with the same message plus that extra sentence. Nothing changes for people simply using HypAware: no command, output, recorded data, privacy setting, or background service behaves differently.

Why this level: The change cannot turn a working plugin into a broken one, cannot lose or expose data, and cannot do anything a user would need to undo. Its entire reach is the wording of one failure message.

What was checked: The old and new behavior were run side by side over sixteen kinds of plugin registration, including unusual and hostile ones, and agreed on every outcome. The full test suite (5,798 checks) and the type check both passed.