CommandRegistry.register says out loud when the copy drops an optional member (#1226)

[philcunliffe]

Problem

CommandRegistry.register stores { ...command } and shape-checks only the
four required members. An optional member that is not own-enumerable (a
prototype getter on a class instance, Object.create inheritance, an own
non-enumerable property) is dropped with nothing to refuse: registration
succeeds and the command runs without it.

Every symptom is an absence, measured in PR #1221's review round 2:

• the alias index gets nothing, so the alias is dead
• a command that asked to be hidden lists in hyp --help
• a lost plugin re-derives category from the first word of the command's
  own name instead of additional, and audience becomes everyday instead
  of operator (or machine when hidden was also lost)

Fix

After the required-member checks pass and before the defaulting, register
warns when a known optional member (plugin, category, audience,
bootProfile, group, help, aliases, hidden) is reachable on the
registration but absent from the copy, naming the command and the members.
Placed before the defaulting so a dropped category is named rather than
papered over by the value derived to replace it.

The warning takes { mirrorStderr: true }. That is the rule
LLP 0329 #stderr-mirror
settled for a degradation observable only as an absence: without it the WARN
is dropped before any exporter on a default install (no HYP_DEV_TELEMETRY,
no OTLP endpoint), which is the same silence the drop already has. It is
per-call-site, not a level, so an ordinary registration stays silent, and it
is what makes the behaviour something a test can see (#testable).

The probe is copyMiss, reused exactly as PR #1221 established it, so the new
path keeps both of its properties: presence-only (in walks the chain and
invokes no accessor, pinned at reads === 0 on a class whose optional members
are prototype getters), and no error escapes (a throwing Proxy has trap
costs the warning, never the registration it was only commenting on).

No new LLP: this realizes LLP 0329's existing rule rather than deciding
anything new. docs/PLUGIN_AUTHORING.md said the drop happens "with no error
at all"; that sentence is updated to name the WARN, since it is now the only
sign.

Proof

test/core/command-registry-register.test.js, three tests, the first of which
fails on master (a dropped optional member is warned about at register time)
and passes here; the other two are the negative controls that keep the warning
honest:

• a dropped optional member is warned about at register time - class with
  get plugin(), get aliases(), get hidden() on the prototype and the four
  required members own: the WARN names all three, the command is still
  registered (a warning, not a refusal), and no getter ran.
• a registration with no optional members warns about nothing - the healthy
  path, both a bare registration and a fully-populated plain object, writes
  nothing at all.
• a throwing has trap costs the warning, not the registration - the one trap
  the probe reaches must not break a registration it was only annotating.

npm test (5803 tests, 0 fail), npm run typecheck, and
npm run smoke -- cli_bundled_plugins_activated all green locally.

Scope

Issue #1226 carries three items. This PR addresses item 1 only.

Not addressed, and still open on the issue:

• Item 2: the maintainer disposition of Published plugin types do not enforce what CommandRegistry.register now enforces at runtime #1007's option 1 (whether Published plugin types do not enforce what CommandRegistry.register now enforces at runtime #1007
  closes on PR CommandRegistry.register's refusal names the member the spread left behind (#1007) #1221's option 2 or stays open). That is an explicit maintainer
  decision, not a code change.
• Item 3: the release-notes item naming the three rejected shapes and the
  two snapshotted semantics, for whichever release carries PR Deferred PR #857 review findings: registry copy, group headers, doc hygiene #1004's
  narrowing. That release is unreleased.

Fixes #1226

[philcunliffe]

Verdict

Approve with fixes applied. The change is sound and well-scoped: OPTIONAL_MEMBERS is exactly the optional half of CommandRegistration (hypaware-plugin-kernel-types.d.ts:1000-1028), the probe correctly reuses copyMiss's presence-only in walk, and no registration shipped in this tree trips the new WARN.

Verified against head 94563112:

• npm test green (5803 tests, 0 fail); npm run typecheck clean.
• Smokes cli_bundled_plugins_activated, package_bin_boot, status_diagnostics, walkthrough_picker_to_first_query all ok.
• hyp --help on a temp HYP_HOME emits zero WARN lines, and npm test produces zero stray command-registry lines, so the healthy path really is quiet. verbToCommand (src/core/cli/verb_command.js:53) builds a plain object literal, so verb projections never warn.
• registerGroup (src/core/registry/commands.js:165) stores the caller's object directly rather than a spread, so it has no equivalent silent-drop hole; the PR's scope is right.
• No import cycle: src/core/observability/index.js does not reach the registry, and dispatch.js:19 already imports observability, so there is no new startup cost.
• The PR's claim that a dropped optional member is warned about at register time fails on master reproduces.

Five findings, all low severity. Two were actionable and are fixed in f9d38e19; three are recorded for the maintainer.

---

Fixed

1. LOW (correctness) - the WARN says "registered" before four refusals that can still reject it

src/core/registry/commands.js:83 (at 94563112)

warnDroppedOptionals ran before the audience validation (:97), the bootProfile validation (:100), the duplicate-name check (:103), and the alias-collision check (:106). Reproduced against the head: registering a class instance carrying a prototype get plugin() under an already-taken name printed

[hypaware:command-registry] WARN CommandRegistry.register: 'dup' registered without 'plugin' ... {"status":"degraded","error_kind":"optional_member_not_copied","command_name":"dup",...}

and then threw duplicate command name 'dup'. So the one channel LLP 0329 guarantees an operator can see reported a degraded command that does not exist, and the structured record asserted hyp_operation: command.register / status: degraded for a registration that never landed. Anything counting error_kind: optional_member_not_copied counts refused registrations as live degraded commands.

The PR's stated ordering constraint is only that the probe read the copy before the ??= defaulting, so that a dropped category is named rather than papered over by the value derived to replace it. That is preserved by splitting the probe from the saying:

• droppedOptionals(command, record) reads where the old call sat, before the defaulting.
• warnDroppedOptionals(name, dropped) says it after byName.set and the alias-index loop, once the command is actually registered.

2. LOW (test quality) - the has-trap control did not pin what it is named for

test/core/command-registry-register.test.js:317 (at 94563112)

a throwing has trap costs the warning, not the registration proxied makeCommand({ name: 'has-trapped' }), which has no reachable optional member. assert.equal(text, '') therefore held whether or not the throw was contained, so only the commands.get(...) assertion was load-bearing and the titled claim ("costs the warning") was unpinned. The target now carries a prototype get plugin(), which is exactly the warning the trap is supposed to cost.

A new test, a refused registration is not warned about as a degraded one, pins finding 1. It fails on 94563112 and passes on f9d38e19 - verified by running the post-fix test file against a worktree checked out at the pre-fix head (not ok 16, 15/16 pass).

Checks after the fix: npm test 5804 tests, 0 fail; npm run typecheck clean; the four smokes above still ok.

---

Not fixed - for the maintainer

3. LOW (LLP conformance) - @ref LLP 0329#stderr-mirror [implements] widens a settled decision

src/core/registry/commands.js:282

LLP 0329 settles the mirror for containment refusals: "a refusal that leaves every counter at zero must opt into the stderr mirror." This site is not a refusal (registration succeeds by design) and sits on the plugin-activation path rather than a cache guard. #not-every-warn explicitly declines to widen the mirror across the tree's ~80 warn sites because their healthy-path noise profile is unaudited.

Concretely, this makes a fifth mirrorStderr: true opt-in, and LLP 0335 §#not-a-fifth-mirror:165 still reads "LLP 0329#stderr-mirror settles a per-call-site opt-in for four named containment refusals." Both prior widenings minted a doc carrying **Extends:** LLP 0329 (LLP 0332:9, LLP 0335:9). Per CLAUDE.md's rule for Accepted docs, this widening arguably wants the same treatment rather than an [implements] on a rule about refusals.

The PR body argues the opposite ("No new LLP: this realizes LLP 0329's existing rule"), and the case is genuinely arguable: the negative-control test does answer #not-every-warn's audit concern for this site. Left as a maintainer decision, since minting an LLP is a design act and numbers are minted cross-branch via scripts/llp-numbers.js under CI gating. Note also that LLP 0329's own Extended-by: line (:14) lists only 0332, not 0335, so that forward-ref convention is already imperfectly maintained in-tree.

4. LOW - presence-only probing reports harmless prototype defaults as degraded

src/core/registry/commands.js:285

A base class supplying get hidden() { return false } / get aliases() { return [] } as defaults yields registered without 'aliases', 'hidden' with status: degraded on stderr at every process start and daemon boot, although the stored record behaves identically (hidden === undefined is falsy, aliases ?? [] is []).

Not fixed deliberately: the only ways to suppress it are to invoke the accessor (expressly forbidden by the design, and pinned by assert.equal(reads, 0)) or to drop members from the list (loses real coverage). The warning is also factually true - those members genuinely did not reach the stored record - and the documented remedy is in docs/PLUGIN_AUTHORING.md. This is an inherent cost of the no-accessor-read rule, worth recording rather than patching.

5. LOW - OPTIONAL_MEMBERS is hand-duplicated with nothing pinning it to the interface

src/core/registry/commands.js:249

The list matches CommandRegistration exactly today (all 8 verified). But TypeScript cannot check a runtime string array against an interface, and no test does either, so a future optional member added to hypaware-plugin-kernel-types.d.ts silently escapes the diagnostic - the same "absent with nothing to say so" failure this PR exists to close, one level up.

Not fixed: a drift guard means parsing the .d.ts at test time, which is a new mechanism beyond this PR's scope and outside "the smallest change that fixes the problem". Flagging it as a cheap follow-up (a test asserting the list equals the interface's optional keys, or a comment on the interface pointing back here).

---

Scope note

The PR addresses item 1 of #1226 only, and says so. Items 2 (maintainer disposition of #1007) and 3 (release-notes item) remain open on the issue and are correctly out of scope here.

[philcunliffe]

Verdict (round 2 of 2)

Approve with fixes applied. Round 1's fix is correct about the thing it set out to fix, and I verified that empirically on all four refusal paths, not just the one it wrote a test for. But the code motion it used introduced two new defects of its own, both in warnDroppedOptionals. Both are fixed in 663ae55f.

Round 1's three deferred items were not touched by this head and remain open for the maintainer, unchanged.

---

Round-1 fix: the part that is right

The split is sound. droppedOptionals(command, record) (src/core/registry/commands.js:89) still runs before the ??= defaulting, and the say now runs after byName.set and the alias-index loop.

Round 1 pinned only the duplicate-name refusal with a test. I drove a class instance carrying a prototype get plugin() into each of the four refusals in turn, on both heads, with the same script.

At 94563112 (pre-round-1-fix), every refusal emitted a false "registered" WARN:

invalid audience:    threw ... | command-registry WARN lines = 1
invalid bootProfile: threw ... | command-registry WARN lines = 1
duplicate name:      threw ... | command-registry WARN lines = 1
alias collision:     threw ... | command-registry WARN lines = 1
success (control):   no throw  | command-registry WARN lines = 1

At f9d38e19, none of them does:

invalid audience:    threw ... | command-registry WARN lines = 0
invalid bootProfile: threw ... | command-registry WARN lines = 0
duplicate name:      threw ... | command-registry WARN lines = 0
alias collision:     threw ... | command-registry WARN lines = 0
success (control):   no throw  | command-registry WARN lines = 1

So the fix covers all four paths, and one test for a single code motion is the right amount of test. Also re-checked and clean at this head: OPTIONAL_MEMBERS (:256) is still exactly CommandRegistration's optional keys (plugin, category, audience, bootProfile, group, help, aliases, hidden), in order; the @refs survived the motion with their attachment intact (LLP 0248#semantic-boot directly above record.category ??=, LLP 0329#stderr-mirror directly above warnDroppedOptionals); and the strengthened has-trap test is a real improvement, because round 1's target carried no prototype-resident optional and so its assert.equal(text, '') pinned nothing.

---

Fixed in 663ae55f

1. MEDIUM (correctness) - moving the say after the mutation made register non-atomic

src/core/registry/commands.js:123 and :311 (at f9d38e19)

warnDroppedOptionals sat outside any try. Of its two statements, getLogger is safe here (logs in src/core/observability/runtime.js:76 is an in-repo frozen shim whose getLogger just does new Logger(name, version), a two-field constructor, and a foreign global provider only reaches Logger.emit, which logger.js:171 already wraps) - but the mirror's process.stderr.write (src/core/observability/logger.js:193) is the one step of the emit that is guarded by nothing, and it is the last statement in emit.

Because round 1 moved the call to after byName.set and the alias loop, a throw there no longer costs only the warning. Reproduced on both heads with a process.stderr.write that throws, registering a command with a prototype get plugin() and an aliases:

PRE-FIX  (94563112): register threw: mirror write failed | command live: undefined | size: 0
POST-FIX (f9d38e19): register threw: mirror write failed | command live: proto | alias: proto | size: 1

That second line is a half-applied registration: register reports failure, activatePlugins catches it and files plugin.activate_failed, and the command stays live and dispatchable under a plugin reported as not loaded. Round 1's placement was at least atomic.

It also contradicts the file's own stated rule. copyMiss (:347) wraps the probe precisely so "a throwing has trap costs the warning, never the registration it was only commenting on" - and the say, which now runs when there is far more to lose, had no such guard. The fix carries that rule to the other side: a try/catch around the emit, with the catch documenting that the channel which would carry the report is the thing that just failed.

After the fix, same repro:

POST-FIX register threw: no | command live: proto | alias: proto | size: 1

New test a throwing mirror write costs the warning, not the registration (test/core/command-registry-register.test.js:383) pins it, restoring process.stderr.write in a finally so a failure cannot leak the patched descriptor into the next test.

2. LOW (correctness) - the message was false about three of the eight members

src/core/registry/commands.js:312 (at f9d38e19)

The probe is snapshotted before the defaulting, which is right, but the sentence is now emitted after it. For category, audience and bootProfile the record therefore does carry a value by the time the line is written - the derived one, not the declared one - so 'x' registered without 'category' is false about the record an operator can go and read. With a prototype get category() { return 'observability' }, the old text claimed the command had no category while hyp --help filed it under proto, its own name.

This is the message-accuracy face of round 1's still-open item 4, and it is the half that round 2's own justification for the move makes untenable: if "everything this line asserts is about a registration that happened", the assertion has to be true of that registration.

Fixed by naming the members as declared:

[hypaware:command-registry] WARN CommandRegistry.register: 'proto' registered without the declared 'plugin', 'category' - reachable on the registration but not an own enumerable property, so the registry's copy did not carry it ...

True for all eight members (the five undefaulted ones were declared and not carried either), and the structured fields are untouched: command_name, dropped_members, status: degraded, error_kind: optional_member_not_copied all unchanged, so anything counting on them is unaffected. Grep confirms the string had no other reader in the tree; docs/PLUGIN_AUTHORING.md's description ("naming the command and the members its copy did not carry") stays accurate.

---

Checks on 663ae55f

• npm test: 5805 tests, 5804 pass, 0 fail, 1 skipped (baseline at f9d38e19 was 5804/5803/0; the delta is the one new test). Run with node_modules symlinked into the worktree - a bare worktree's ERR_MODULE_NOT_FOUND noise is a setup artifact, not a result.
• npm run typecheck: clean.
• Round 1's property re-verified after the change: all four refusals still emit zero command-registry lines, and the success control still emits exactly one.
• Healthy path still silent: under a temp HYP_HOME, both hyp --help and hyp dev --help (the all-available command tree, a wider registration surface than --help) exit 0 with zero command-registry lines on stderr.
• Smokes all ok, none of their logs carrying a command-registry line: core_boot_noop, package_bin_boot, cli_bundled_plugins_activated, daemon_install_render, client_attach_idempotent, status_diagnostics, walkthrough_picker_to_first_query, query_grep_roundtrip, local_parquet_export.
• Style: no em dashes, no statement semicolons in either changed file.
• CI at f9d38e19 was green across test (22/24), typecheck (22/24), duplicate-numbers, cross-branch-numbers, CI required, LLP required, with mergeStateStatus: CLEAN.

Considered and dismissed

• Guarding the say inside register rather than inside warnDroppedOptionals. Equivalent, but the containment belongs with the saying, next to the sentence that explains why it exists, and it keeps register readable.
• Moving the say back to just before byName.set. That would also restore atomicity, but it does not restore containment (an escaping throw would still cost the registration, just in the other direction), and nothing stands between that point and the two Map.set calls, so it buys nothing the guard does not.
• droppedOptionals' JSDoc says "in declaration order" where the order is actually OPTIONAL_MEMBERS' order. The two coincide today; cosmetic, not worth a change on the last round.

---

Still open from round 1 (unchanged at this head - maintainer decisions)

3. LOW (LLP conformance) - @ref LLP 0329#stderr-mirror [implements] arguably widens a settled decision

src/core/registry/commands.js:306

Re-confirmed in-tree: llp/0335-a-telemetry-failure-is-said-once-never-thrown.decision.md:165 still reads "LLP 0329#stderr-mirror settles a per-call-site opt-in for four named containment refusals", and this is a fifth mirrorStderr: true opt-in on a non-refusal, on the plugin-activation path. #not-every-warn (llp/0329-...:133) explicitly declines to widen the mirror across the tree's warn sites. Both prior widenings minted a doc carrying **Extends:** LLP 0329. The PR argues the opposite and the case is genuinely arguable - the negative-control tests do answer #not-every-warn's audit concern for this one site, and round 2 has now added the dev --help surface to that evidence. Left as a maintainer call, since minting a number is a design act gated cross-branch by scripts/llp-numbers.js. Note LLP 0329's own Extended-by: (:14) still lists only 0332, not 0335, so the forward-ref convention is already imperfectly maintained here.

4. LOW - presence-only probing reports harmless prototype defaults as degraded

src/core/registry/commands.js:280

A base class supplying get hidden() { return false } / get aliases() { return [] } still warns with status: degraded at every process start, although the stored record behaves identically. Round 2 fixed the message-accuracy half of this (finding 2 above); the false-positive half is unchanged and still deliberate, because the only suppressions are invoking the accessor (forbidden by design, pinned by assert.equal(reads, 0)) or shrinking the list (loses real coverage). The remedy stays documented in docs/PLUGIN_AUTHORING.md.

5. LOW - OPTIONAL_MEMBERS is hand-duplicated with nothing pinning it to the interface

src/core/registry/commands.js:256

Verified still exact today (all 8, in order). But nothing - not tsc, not a test - keeps it so, and a future optional member added to hypaware-plugin-kernel-types.d.ts escapes the diagnostic silently: the same "absent with nothing to say so" failure this PR exists to close, one level up. A cheap follow-up would be a test asserting the array equals the interface's optional keys.

---

Scope note

Unchanged: the PR addresses item 1 of #1226 only, and says so. Items 2 (maintainer disposition of #1007) and 3 (release-notes item) remain open on the issue.

[philcunliffe]

Triage at the review cap (head 663ae55f)

Two review rounds exhausted with three findings still open. Each was re-read against the tree at this head, not from the review prose, and every one is a preference rather than a production risk, so the PR is safe to merge and the residuals are deferred to #1232.

• Finding 3 (src/core/registry/commands.js:306, @ref LLP 0329#stderr-mirror [implements] on a non-refusal site): LLP bookkeeping. Whether this site wants a small extending decision doc or the [implements] stands is a design-record question with no runtime effect. Deferred.
• Finding 4 (commands.js:280, prototype-resident defaults warn as degraded): an accurate, documented noise cost, deliberately not suppressible without invoking the accessor. Nothing in the tree trips it: hyp --help and hyp dev --help on a temp HYP_HOME emit zero command-registry lines. Deferred.
• Finding 5 (commands.js:256, OPTIONAL_MEMBERS has no drift guard against CommandRegistration): a test nicety; the list matches the interface's eight optional keys today, in order. Deferred.

Verified at this head: round 2's two fixes are in the tree (the say runs after both Map.sets inside a try/catch; the message names members "as declared"), npm test 5805 tests / 0 fail, npm run typecheck clean, all eight CI checks green, mergeStateStatus: CLEAN.

One stale note in round 2: LLP 0329's Extended-by: line already lists 0335 at this head (:18).

Follow-up: #1232.

[philcunliffe]

Ship risk: low

Who could be affected: Anyone running the hyp command line, and authors of add-on plugins.

What could happen:

• If a plugin's command is written in a way that quietly loses one of its optional details, the tool now prints a short warning naming that command when it starts. Previously that loss was completely silent, so a dead shortcut or a command that refused to stay hidden had no explanation.
• No command changes what it does, and nothing new is refused. A command that loads today still loads, including one that triggers the new warning.

Why this level: The only new user-visible behaviour is one extra line of explanatory text, and only for a setup that is already subtly broken. Nothing is deleted, no private information is exposed, and the change is easy to undo.

What was checked: The real tool was started with every bundled add-on switched on, and it produced no new output at all, confirming nothing normal triggers the warning. The full test suite passed (5,805 tests). The warning was also deliberately broken three different ways to confirm the checks genuinely catch failures rather than passing by luck, and it was confirmed that the warning can never stop a command from loading, even if the reporting itself fails. The warning text records only the command's name, never any of its contents.