Deferred PR #857 review findings: registry copy, group headers, doc hygiene - #1004
Conversation
…ygiene (#946) Four independent findings deferred from the PR #857 review, all still live on master. 1. CommandRegistry.register mutated its argument. The category/audience/ bootProfile defaulting wrote into the caller's object and ran before the duplicate-name and alias-collision checks, so a rejected registration left the caller mutated and a frozen module-level constant threw a TypeError out of the defaulting instead of getting registered. The defaulting now writes into a copy the registry stores. That copy broke one hidden coupling: verb_command.js recognized its own projections by WeakSet identity, and the registry no longer stores the object verbToCommand returned, so releasing a verb name stopped retracting its CLI command (LLP 0264). The mark is now a module-private symbol property, which an object spread carries onto the stored record and which stays out of Object.keys, JSON, and CommandRegistration. 2. Metadata-only groups rendered help with no header. hyp cache, hyp client history, and hyp dev plugin have no bare command, and core registered no group description for them, so their --help opened on a naked usage: line and a subcommand table. That is the gap LLP 0214 #d2 exists to close, in the new tree. registerCoreCommands now registers a description for each. 3. AGENTS.md used the pre-rollover hyp smoke spelling in 17 places. Rolled onto the canonical hyp dev smoke. 4. LLP 0248 and LLP 0266 shipped as Draft. Flipped to Accepted and Active (a status flip is a permitted mechanical edit under LLP 0156). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e is documented `isVerbProjection` swapped `WeakSet.has` for a property read, which is not total: `WeakSet.has(null)` answered false, `null[VERB_PROJECTION]` throws. `retractCommand` guards only `undefined`, so an injected command registry answering `null` where the contract says `undefined` now took daemon boot down on the one path whose whole discipline is that it must never throw (the sibling test for a registry without `unregister` pins that rule). Named both nullish cases and pinned the behaviour with a test that fails at cd372f8 and passes on origin/master. `retractCommand`'s docstring still said "the test is identity", which this branch replaced with a provenance mark; reworded to match, the same way `verb_command.js` already was. Documented `CommandRegistry.register` in the published kernel contract: the registry stores a shallow copy, so a frozen registration is accepted, a rejected one comes back unmutated, and edits made after registering no longer reach `get`/`list`. That last clause is an observable change for third-party plugins and was only stated in a source comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review round 1 -
|
| tests | pass | fail | skipped | |
|---|---|---|---|---|
origin/master a4c6350b |
5165 | 5164 | 0 | 1 |
cd372f8b (as submitted) |
5173 | 5172 | 0 | 1 |
2682f4cc (after review fixes) |
5174 | 5173 | 0 | 1 |
npm run typecheck clean at all three. Smokes core_boot_noop, package_bin_boot, cli_bundled_plugins_activated pass at 2682f4cc.
Findings
1. isVerbProjection stopped being total, on the one path whose rule is that it must never throw - medium, fixed
src/core/cli/verb_command.js:91 (at cd372f8b):
return command !== undefined && /** @type {any} */ (command)[VERB_PROJECTION] === trueWeakSet.prototype.has is total for any argument; a property read is not. The guard covers undefined but not null, so isVerbProjection(null) throws TypeError: Cannot read properties of null (reading 'Symbol(hypaware.verbProjection)') where the WeakSet returned false.
That is reachable. retractCommand (src/core/registry/verbs.js:171-173) does const command = registry.get(name) then guards only command === undefined, and the command registry is something the kernel accepts by injection (createKernelRuntime({ commandRegistry })). CommandRegistry.get is typed CommandRegistration | undefined, so null is off-contract - but the whole design of this function is that off-contract registries degrade rather than throw: the sibling case (a registry with no unregister) is explicitly tolerated and warned, with a test pinning it, on the stated grounds that "a throw here would take daemon boot down". A null from get() is the same class of input and now takes the same boot down.
Evidence that this is a regression of this branch, not a pre-existing hole - the new test run against each tree:
# cd372f8b, with only the new test applied
not ok 14 - a command registry whose get() answers null degrades, never throws
error: "Cannot read properties of null (reading 'Symbol(hypaware.verbProjection)')"
# fail 1
# origin/master a4c6350b, same test
# pass 15
# fail 0
Fixed in 2682f4cc: both nullish cases named in isVerbProjection, @param widened to CommandRegistration | undefined | null, and the case pinned by test/core/verb-registry.test.js:161 alongside the existing tolerance tests.
2. retractCommand's docstring still claims the test is object identity - low, fixed
src/core/registry/verbs.js:135 (at cd372f8b): "The test is identity, not bookkeeping". That sentence described the WeakSet. This branch replaced identity with a provenance mark precisely because identity stopped working, and updated the twin sentence in verb_command.js:17 ("Identity answers that" to "Provenance answers that") but not this one. CLAUDE.md's "keep refs honest" rule applies to the prose the @ref sits in. Fixed: reworded to "the mark that projection carries, not a ledger kept here", which keeps the actual contrast (mark vs per-registry ledger) the paragraph goes on to argue.
3. The copy rule is a plugin-facing contract change and was documented only in a source comment - low, fixed
hypaware-plugin-kernel-types.d.ts:929 (at cd372f8b): register(command: CommandRegistration): void, no docstring. Item 1 changes three things third parties can observe: a frozen registration is now accepted, a rejected registration comes back unmutated, and - the one that can bite silently - mutating the registration object after registering no longer reaches get/list. This file is what external plugins compile against; the reasoning lives in src/core/registry/commands.js where they will not see it. Fixed: docstring added, including that the copy is shallow and run is shared, not cloned.
4. Two plugin-owned groups still render headerless help - low, NOT fixed (follow-up)
The task asked whether there are metadata-only groups beyond the three. In core, no - I enumerated every visible prefix over registerCoreCommands's registry and [] remain without a bare command or a registered description. (mcp looks like a fourth but is an alias of mcp serve, so it resolves and gets a header. Good catch by the new gate test's comment.)
Outside core, two plugin namespaces have the identical defect, and only context-graph calls registerGroup (hypaware-core/plugins-workspace/context-graph/src/index.js:98):
client claude-desktop-hypaware-core/plugins-workspace/claude-desktop/src/index.js:86,101,115,139,162register five subcommands, noregisterGroupclient claude-account-hypaware-core/plugins-workspace/claude-account/src/index.js:94,105,116register three, noregisterGroup
Rendered through the real dispatcher:
===== hyp client claude-desktop --help =====
usage: hyp client claude-desktop <subcommand> [args...]
Subcommands:
install ...
===== hyp cache --help ===== <- after this PR
hyp cache - Inspect and maintain the local query cache
usage: hyp cache <subcommand> [args...]
The cache is the local Iceberg store every query reads. These
Not fixed here on purpose: it is pre-existing, the fix belongs in each plugin's activate() (new user-facing prose for two groups), it is outside issue #946, and the PR's own gate test is honestly scoped to core groups. Worth its own issue. No manifest coupling blocks it - diagnose.js:369 only requires that the manifest declare a command under the group, which both do.
5. PR-body citation nit - informational, not actionable
The body says a status flip is "an explicitly permitted mechanical edit under LLP 0156". LLP 0156 is about repairing a collided LLP number by renumbering the later claimant; it says nothing about statuses. The permission is real but lives in AGENTS.md's LLP conventions ("Mechanical edits are still fine: typos, broken links, status changes, and renumbering that does not change meaning"), where the (LLP 0156) parenthetical attaches to the renumbering clause. The edits themselves are correct; only the citation is loose. Not touching the PR body.
What I checked and found clean
The Symbol, adversarially. Verified by running the real code, not by reading it:
- Survives the registry's
{ ...command }andObject.assign- object spread copies own enumerable symbol keys, andmarkVerbProjection's plain assignment makes it enumerable.isVerbProjection(commands.get('demo verb'))istrueafter registration. - Invisible where the comment claims:
Object.keysgivesname,summary,usage,run;JSON.stringifydrops it;Object.entriesis 4 entries;npm run typecheckis clean, so it is outside theCommandRegistrationsurface. - Does not survive
JSON.parse(JSON.stringify(...))orstructuredClone. Neither is on any command-registration path:structuredCloneappears twice in the tree, both insrc/core/remote/credentials.js, neither touching commands. No command list is serialized and re-registered anywhere. - Cannot be forged or lost by another copy: zero call sites spread or
Object.assigna registration (the only...commandin the tree iscommands.js:69), and everyctx.commands.register(...)passes a fresh inline literal, so no object is registered twice or derived from a projection. No class-based or getter-based registrations, so the spread cannot drop a prototype method or double-evaluate a getter. - One latent trap worth knowing, not worth changing. Enumerability is exactly what makes the spread work, and it is also what makes the mark visible to
util.inspect(soconsole.logof a projection prints[Symbol(hypaware.verbProjection)]: true) and toassert.deepStrictEqual, which does compare own enumerable symbol keys. No test does either today, and I confirmed a futureassert.deepStrictEqual(registry.get('query sql'), {...})would fail confusingly. The comment's claim is narrower than that and is accurate as written; making the property non-enumerable would break the spread, so the trade-off is the right one.
LLP 0264 verb retraction, end to end rather than "the 5 tests pass". Over the real registries: fresh projection retracts; the pre-projected path (registerCoreCommands registers first, verb registry skips because the name is taken, unregister still retracts) retracts; a plugin's own same-named command survives; an alias of a projection resolves as a projection and is retracted with it.
The ASI reasoning behind the helper. Confirmed, and it is worse than "breaks": a statement-initial /** @type {any} */ (command)[S] = true after an object literal parses as a call and dies with TypeError: Symbol(...) is not a function. A computed [VERB_PROJECTION]: true key inside the literal would have avoided the helper but would have to fight @ts-check on a literal annotated CommandRegistration. The helper is the right call.
Item 1 blast radius. Nobody in the tree registers and then mutates: every commands.register(...) call site passes an inline object literal, and grep -E "commands\.register\(([A-Za-z_$][A-Za-z0-9_$]*)\)" is empty. The only consumer that depended on the registry storing the same object was isVerbProjection, which the branch found and fixed. registry.get(name) is still stable across calls. hypaware-plugin-kernel-types.d.ts now says so (finding 3).
Item 2's byte-identical claim. Verified both halves. node bin/hypaware.js --help diffs clean between a4c6350b and cd372f8b. registry.list() after registerCoreCommands is identical name-for-name; the only difference in the dump is the three new entries in listGroups(), which is metadata only and read solely by src/core/plugin_doctor/dry_run.js:169 over a per-plugin registry that never calls registerCoreCommands, so no plugin doctor run sees them. All three groups now render a header (dev plugin gets summary-only, no paragraph - fine, renderGroupHelp handles the partial).
Items 3/4. hyp smoke is gone from AGENTS.md (17 occurrences: 16 battery lines plus the repo-layout line); npm run smoke -- <flow> correctly left alone, since that is the package script and still exists. dev smoke is the real registration (core_commands.js:841). Both LLP edits are single **Status:** lines with nothing else touched, and both land on the majority status for their type in this corpus (Decisions: 142 Accepted; Plans: 19 Active). Nothing either document settled was altered.
Conventions. No semicolons, no em dashes anywhere in the diff (mine included), JSDoc-only types, no inline import('...'), no @typedef, and the added @import of CommandGroupRegistration uses the root-anchored .js specifier. All three new @ref anchors resolve: llp/0214...:70 {#d2}, llp/0264...:76 {#verb}, llp/0248...:143 {#semantic-boot}.
Pushed
2682f4cc on fix/issue-946, touching exactly src/core/cli/verb_command.js, src/core/registry/verbs.js, hypaware-plugin-kernel-types.d.ts, test/core/verb-registry.test.js.
Nothing here needs a human decision.
…copyability is stated `register()` checked the caller's object and stored a copy of it. Those can disagree: a spread carries own enumerable properties and nothing else, so a class instance whose `run()` is on the prototype passed the shape check and stored a record with no `run`, surfacing as a TypeError inside dispatch rather than as the boundary error the check exists to raise. A getter that answers differently on a second read slipped past the same way. The copy now happens first and every check reads it. Also corrects the projection mark's docstring: an enumerable symbol on a stored registration can be lifted with `Object.getOwnPropertySymbols` and restamped, which the `WeakSet` it replaced could not. That is fine here (a forged mark only gets the forger's own command retracted) but the comment claimed it was no more forgeable than the set was, which is not true.
Review round 2 -
|
| tests | pass | fail | skipped | |
|---|---|---|---|---|
origin/master a4c6350b |
5165 | 5164 | 0 | 1 |
2682f4cc (round 1 head) |
5174 | 5173 | 0 | 1 |
20dd24cb (after round 2 fixes) |
5176 | 5175 | 0 | 1 |
npm run typecheck clean at 20dd24cb. Smokes core_boot_noop, package_bin_boot, cli_bundled_plugins_activated, walkthrough_backfill_client_history pass. node bin/hypaware.js --help is still byte-identical to origin/master.
Findings
1. register() checks one object and stores a different one - medium, fixed
src/core/registry/commands.js:45-56 (checks) and :69 (copy), at 2682f4cc:
if (typeof command.run !== 'function') { // <- reads the argument
throw new TypeError(`CommandRegistry.register: '${command.name}' missing run()`)
}
...
const record = { ...command } // <- stores something elseRound 1's fix made the defaulting write into a copy, which is right. But the shape checks were left reading the argument, and a spread carries own enumerable properties and nothing else. The two can therefore disagree, and when they do the registry stores a record it never checked.
Concretely, run against 2682f4cc:
class Cmd {
constructor() { this.name = 'proto'; this.summary = 's'; this.usage = 'u' }
async run() { return 0 }
}
r.register(new Cmd())
typeof r.get('proto').run // 'undefined'
await r.get('proto').run() // TypeError: run is not a functionThe check passes (it finds run on the prototype), the copy drops it, and the failure surfaces inside dispatch when a user types the command, not at the boundary whose whole job is to refuse this. On origin/master the same registration worked, so this is a regression of this branch, and it lands on hypaware-plugin-kernel-types.d.ts CommandRegistration, which is an interface: a class instance satisfies it structurally, so a third-party plugin author writing a class-based command compiles clean and breaks at runtime.
Same hole from the other side, also verified failing at 2682f4cc: a registration exposing run as a getter is read twice, once by the check and once by the spread, so the function the check accepted is not necessarily the function the registry stores.
Fixed in 20dd24cb: the copy moves above the checks and every check reads record, so the object that was validated is the object that is stored. The class-instance case now fails loudly at registration with the existing 'proto' missing run() error instead of silently at dispatch. Two tests added at test/core/command-registry-register.test.js, both confirmed red at 2682f4cc and green at 20dd24cb:
# 2682f4cc, new tests only
not ok 8 - the shape checks run on the stored record, not on the argument
not ok 9 - the run() the checks accepted is the run() the registry stores
# fail 2
The public docstring on CommandRegistry.register gained the corresponding sentence, so the own-enumerable-only rule is stated where plugin authors read it. No signature changed.
2. The projection mark's docstring claims it is unforgeable, and it is not - low, fixed
src/core/cli/verb_command.js:27-28 at 2682f4cc: "A module-private symbol is no more forgeable than the set was (nothing outside this file can name it)".
That is the one claim in the paragraph that did not survive the WeakSet to Symbol swap. A WeakSet closed over by this module genuinely cannot be added to from outside. An enumerable own symbol on a stored registration can be read straight back out:
const syms = Object.getOwnPropertySymbols(registry.get('query sql'))
// [ Symbol(hypaware.verbProjection) ]
isVerbProjection({ name: 'x', summary: 's', usage: 'u', run: () => 0, [syms[0]]: true }) // trueEnumerability is exactly what makes the spread carry the mark, so this is not fixable without breaking the mechanism, and the consequence is benign: forging the mark only gets the forger's own command retracted when that verb name is released, and plugins are in-process code anyway. So the mechanism is right and only the sentence was wrong, which is the same "keep the prose honest" rule round 1 applied to retractCommand's docstring.
Fixed: the paragraph now says the mark is copyable, why that is accepted, and what forging it does and does not buy.
Focus items, checked and clean
1. isVerbProjection totality (src/core/cli/verb_command.js:91-96). Run, not read. Over undefined, null, 0, NaN, '', false, true, 1n, a Symbol, {}, [], a function, Object.create(null), and a frozen object: all false, none throw. Property access on a primitive boxes rather than throws, so naming the two nullish cases is genuinely sufficient; there is no third throwing input in the language short of an exotic object. Not too permissive either: a real projection is true, a spread copy of one is true, a plain command with the same shape is false.
The one input that still throws is a Proxy whose get trap throws, where WeakSet.has returned false. Not actionable: that is a registry injected specifically to sabotage the host, several steps beyond the off-contract null the guard exists for, and no total rewrite short of a try/catch covers it.
Behavior at the caller is right: retractCommand (src/core/registry/verbs.js:171-180) takes the command_not_verb_projection warn-and-return branch on null, which is the documented degrade path, not the boot-killing throw.
2. The docstring corrections. src/core/registry/verbs.js:135 now reads "the mark that projection carries, not a ledger kept here", which matches what the code does and preserves the contrast the paragraph goes on to argue. hypaware-plugin-kernel-types.d.ts:929 is a comment-only change: the diff adds nine *-prefixed lines and touches no declaration, and the contents are accurate (verified each clause by running it, including that a rejected registration comes back with exactly its original key set and that get(name).run === registration.run). My addition is likewise comment-only. npm run typecheck with skipLibCheck: false is clean. Published type surface unchanged.
3. Registry copy vs register-then-mutate. No such call site exists, confirmed three ways rather than by one grep. Every commands.register(...) in the tree passes a fresh inline literal (core_commands.js:75,86 build theirs per call); dry_run.js:222 is the source registry, not the command registry; context-graph/src/index.js:60 is a contract registry. The only post-registration field writes anywhere in the tree (dispatch.js:811, plugin_catalog.js:137-139) are on locally built row/descriptor objects, never on anything the command registry stores. No Object.defineProperty on a registration anywhere, so the copy cannot drop a non-enumerable member of a real registration.
4. The cli-consistency-gate assertion and future groups. No false-failure path I can construct for a legitimate group. Prefixes come only from non-hidden command names, so a hidden group contributes nothing and an alias-only prefix (backfill, plugin) is never synthesized as a group. A prefix that resolves to a command is headed by that command's canonical name, which is what dispatch prints for an alias (hyp mcp renders as mcp serve, verified live). The one residual softness is that "has a summary" is tested as doesNotMatch(/undefined/) on the assembled header, so a future group whose summary literally contains the word "undefined" would fail spuriously. Informational only, and I would not change it: the assertion that actually holds the line is out.startsWith(header) against the real dispatcher.
Core groups do not leak into the plugin doctor. dry_run.js:82 builds its own createCommandRegistry() and never calls registerCoreCommands, so snapshotRegistry's listGroups() (:169) still sees only what the plugin registered, and diagnose.js:369's manifest agreement check is unaffected by the three new core groups.
No user-visible regression from the group registrations. Diffed origin/master against the branch through the real dispatcher for cache, cache bogus, client history, client history bogus, dev plugin bogus, client, query, plugin, backfill. Every unknown-subcommand error and exit code is identical; the only differences are the three new header-plus-paragraph blocks. Group prose checked against the registry rather than trusted: cache really has exactly the three subcommands the paragraph describes and they really carry the query status/refresh/maintain aliases it cites; client history plan exists and really is the dry-run entry point.
Doc hygiene. CLAUDE.md is a symlink to AGENTS.md, so the hyp smoke to hyp dev smoke sweep covers both. The remaining hyp smoke occurrences in the tree are all in llp/ records and llp/tombstones/, which are immutable by convention and correctly left alone.
Conventions. No semicolons and no em dashes in the diff, mine included (the one ; a grep flags is prose inside a comment). JSDoc types only, no inline import('...'), no @typedef, root-anchored .js type-import specifiers. All three @ref anchors resolve: llp/0214-...:70 {#d2}, llp/0264-...:76 {#verb}, llp/0248-...:143 {#semantic-boot}.
Still out of scope, as recorded: the two plugin-owned headerless groups (claude-desktop/src/index.js:86, claude-account/src/index.js:94) tracked as #1005. Not touched.
Pushed
20dd24cb on fix/issue-946, touching exactly src/core/registry/commands.js, src/core/cli/verb_command.js, hypaware-plugin-kernel-types.d.ts, test/core/command-registry-register.test.js.
Triage - final head
|
philcunliffe
added
the
neutral:approved
Ship risk:
|
| registration shape | base a4c6350b |
head 20dd24cb |
|---|---|---|
class instance, run() on prototype |
ok, run()=ran |
THROWS 'a' missing run() |
Object.create(proto), members inherited |
ok, run()=ran |
THROWS command.name must be a non-empty string |
non-enumerable own run |
ok, run()=ran |
THROWS 'c' missing run() |
live getter for summary |
re-evaluated per read (v2,v3) |
snapshotted once (v1,v1) |
mutate registration after register() |
reaches the registry (true) |
does not (false) |
| frozen object literal | THROWS Cannot add property category |
ok (this is the fix) |
src/core/cli/verb_command.js:84,105 — the WeakSet provenance ledger becomes a module-private enumerable Symbol (VERB_PROJECTION), because the registry's copy destroys the object identity isVerbProjection used. isVerbProjection at :104 is also made total on null/undefined; a throw there runs on the daemon-boot retraction path (src/core/registry/verbs.js:171).
src/core/cli/core_commands.js:98-125 — CORE_COMMAND_GROUPS registers metadata-only descriptions for cache, client history, dev plugin, the three core groups with no bare command. registerGroup itself is unchanged in this diff.
AGENTS.md — 17 lines, hyp smoke to hyp dev smoke; every changed line contains smoke (0 non-smoke changed lines).
llp/0248 Draft to Accepted, llp/0266 Draft to Active — one header line each, nothing else.
Surface
- Files accounted for: all 11.
- Behavioral:
src/core/registry/commands.js,src/core/cli/verb_command.js,src/core/cli/core_commands.js. - Comment-only:
src/core/registry/verbs.js(+7/-7, theretractCommanddoc block; no statement changed),hypaware-plugin-kernel-types.d.ts(+13/-0, a JSDoc block onregister; no type shape changed). - Tests:
test/core/command-registry-register.test.js(new, 148 lines),test/core/cli-consistency-gate.test.js(+37),test/core/verb-registry.test.js(+16). - Docs/status:
AGENTS.md(CLAUDE.mdis a symlink to it),llp/0248-…decision.md,llp/0266-…plan.md.
- Behavioral:
- Callers/contracts/config: exhaustive scan of all 248
.register(sites acrosssrc/,bin/,hypaware-core/plugins-workspace/,test/:- Post-register mutation of a named registration: zero production hits. Every
ctx.commands.register(...)in context-graph, codex, claude, claude-account, claude-desktop, gascity, vector-search, ai-gateway, context-graph-enrich passes an unnamed inline object literal. The only hit is the test that pins the new semantics (test/core/command-registry-register.test.js:79-82). - Identity comparison against
get()/list()/match(): zero.src/core/cli/dispatch.js:391,402,424,510-518,src/core/cli/group_help.js:39,src/core/plugin_doctor/dry_run.js:168-180,src/core/cli/core_commands.js:85all read by name or property. The one former identity site is theverbs.js:171/verb_command.js:105pair this PR converted. - Class instances /
Object.create/ prototype-bornerun/ non-enumerable props / getters passed toregister(): zero in production; the only occurrences are the deliberate negative tests attest/core/command-registry-register.test.js:116-127,141. NoObject.create(exists anywhere in the repo. registerGroupavailability: every actualregisterCoreCommandscaller builds its registry withcreateCommandRegistry();src/core/cli/dispatch.js:212calls it only on a registry it constructed itself, so the newregistry.registerGroup(...)loop cannot hit a registry that lacks the method.this-binding:dispatch.js:518callsmatched.command.run(...), sothisinside a shorthandrun()is now the stored copy. No commandrunbody insrc/core/cli,src/core/commands, or any pluginindex.jsusesthis. Inert.- Group-metadata leakage:
src/core/plugin_doctor/dry_run.js:82builds a barecreateCommandRegistry()and never callsregisterCoreCommands, so the three new core groups do not appear in a plugin's dry-runcommandGroupssnapshot.
- Post-register mutation of a named registration: zero production hits. Every
- Concurrency/lifecycle: none introduced. Both registries are synchronous in-memory
Maps. The one lifecycle-adjacent path isVerbRegistry.unregistertoretractCommandat daemon boot, which is whyisVerbProjectionwas made total; proved below. - Sensitive surfaces: public/cross-package contract.
hypaware-plugin-kernel-types.d.tsis listed inpackage.jsonfilesfor the publishedhypaware@1.25.0, soCommandRegistry.registeris a third-party plugin API. This diff changes its runtime acceptance. No auth, secrets, schema/migration, destructive operation, lockfile/CI, or deployment change.
Critical safety fact
Narrowing register() to own-enumerable properties rejects no registration shape actually produced anywhere in this repo or its bundled plugins, keeps every stored record dispatchable and every verb projection identifiable across the copy, and when it does reject, it throws loudly at the registry boundary inside the per-plugin activate() catch rather than storing a run-less record or taking boot down.
Evidence level: 4
Proof: node /tmp/.../proof.mjs (imports the shipped src/core/registry/commands.js, src/core/cli/verb_command.js, src/core/registry/verbs.js from the detached head worktree) → exit 0
PASS A: class instance with prototype run() rejected at boundary; registry untouched
PASS B: all 14 reject paths throw, store nothing, and leave the caller object byte-identical
PASS C: frozen registration accepted; stored copy is dispatchable; caller unmodified
PASS D: Symbol mark survives the registry spread; stays out of Object.keys and JSON
PASS E: isVerbProjection total over nullish + 8 primitives; plugin look-alike is not a projection
PASS F: end-to-end - verb release retracts its own projection, spares a same-named plugin command
ALL CLAUSES HOLD at HEAD 20dd24cb
Negative control (the same script against a worktree at base a4c6350b) → exit 1:
AssertionError: Missing expected exception: FAIL A1: prototype-run registration was NOT rejected
expected: /missing run\(\)/
The proof therefore fails loudly when the fact is false; it is not vacuous.
Corroboration at level 5 (real application): npm test → exit 0, # tests 5176 / # pass 5175 / # fail 0 / # skipped 1; npm run typecheck → exit 0; node bin/hypaware.js dev smoke {core_boot_noop,package_bin_boot,cli_bundled_plugins_activated} all pass (the last activates the full bundled plugin set through the changed boundary); node bin/hypaware.js --help is byte-identical between base and head (diff empty), confirming group registration is metadata-only; and hyp cache/client history/dev plugin --help now each render a header line.
Confirmed risks
- Public-contract narrowing that the published types do not enforce. A third-party plugin registering a class instance still typechecks against the shipped
CommandRegistration— TypeScript structural typing does not distinguish own from prototype members. Verified: aclass MyCommand implements CommandRegistrationpassed toreg.register(...)compiles clean againsthypaware-plugin-kernel-types.d.ts(tsc -p . --strict→ exit 0) while the head runtime throws'my cmd' missing run(). How it breaks: such a plugin fails to activate after upgradinghypaware. Likelihood: low in absolute terms (zero in-repo instances, and the shape is unidiomatic here), but unknowable for plugins outside this repo. Consequence: bounded and reversible —src/core/runtime/loader.js:108catches per plugin, logsplugin.activate_failedwitherror_kindand the exact TypeError message, and pushes{ ok: false }; the kernel, the daemon, and every other plugin keep running. Detection: theplugin.activate_failedlog event andhyp status. - Two silent semantic changes, not just loud ones. A live getter on a registration is now snapshotted at register time, and a post-registration edit no longer reaches the registry. Both are intentional and pinned by tests (
test/core/command-registry-register.test.js:79-82,141), and the.d.tsprose added athypaware-plugin-kernel-types.d.ts:928-942states both. But unlike the three rejection cases these fail quietly in a third-party plugin: stale metadata, no error. Consequence: cosmetic-to-moderate (wrong help text or wrongaudience/bootProfile), no correctness or safety impact on dispatch. - Adjacent, pre-existing, not introduced here:
src/core/plugin_doctor/dry_run.js:219doesregistry.register({ ...contribution, start: inertStart })on a source contribution, andcreateSourceRegistry().registerstores by reference with no copy. A source contribution built as a class instance would lose its prototype members there, and the guard at:218(typeof contribution.start === 'function') passes on a prototypestart(). This is the exact defect the command registry now guards against, still open one registry over. Out of scope for this PR; worth a follow-up issue.
Cleared
- Verb retraction (LLP 0264) survives the copy. Clause D/F: the
Symbolmark rides the spread onto the stored record,isVerbProjection(stored)istrue, and releasing a pre-projected core verb retracts exactly its own CLI command while a plugin's own same-named command survives. This is the silent local-cache regression LLP 0264 warns about, and it does not occur. isVerbProjectioncannot take daemon boot down. Clause E: total andfalseoverundefined,null,0,'',false,NaN,'str',42,Symbol(),true. TheWeakSetwas total for free; the property read is explicitly guarded atverb_command.js:104.- The pre-copy
!command || typeof !== 'object'guard is load-bearing and still reads the argument (commands.js:41). Spreadingnullor a primitive would succeed silently and then fail the name check with a misleading message. Clause B coversnull,undefined,'not-an-object', and42: all four throw, store nothing. - No reject path leaks state. Clause B: 14 reject paths (12 shape/validation plus duplicate-name and alias-collision) each throw, leave
registry.size() === 0(or unchanged), and leave the caller's own-key list byte-identical.byName.setis the last statement in the function, after every check. - Top-level help and
list()are untouched by the group work.diffofhyp --helpbase vs head is empty; group metadata lives in a separategroupsmap thatlist()never reads (commands.js:181-190). - Forgeability of the
Symbolmark grants no new capability.Object.getOwnPropertySymbolscan lift it, butsrc/core/runtime/activation.js:141(commands: runtime.commands) already hands every plugin the live mutable registry, so a plugin can alreadyunregisteror shadow any command with no symbol involved. There is no trust boundary between in-process plugins to breach. Independently confirmed at that line. - Docs and LLP items carry no runtime risk.
AGENTS.md:hyp dev smoke <flow>is the canonical spelling andhyp smokeis still a working hidden alias (bothnode bin/hypaware.js dev smoke --helpandnode bin/hypaware.js smoke --helpprint the same header), so the rollover points at a real command and breaks no existing one. The two LLP edits are single**Status:**lines, the mechanical class LLP 0156 explicitly permits.
Rationale
Reach is high: CommandRegistry.register is part of hypaware-plugin-kernel-types.d.ts, which package.json files publishes with hypaware@1.25.0, and this diff changes what that public contract accepts at runtime without changing what it accepts at the type level. Under the rubric a public/cross-package contract is a sensitive surface and is at least high unless the diff is demonstrably non-behavioral — this one is demonstrably behavioral, with a measured six-row differential.
Consequence is medium, not high: the worst realistic outcome is a third-party plugin failing to activate, loudly, isolated by the per-plugin catch at src/core/runtime/loader.js:108, with a named log event and a one-line fix. Nothing here touches auth, secrets, persisted data, migrations, or availability of the kernel itself.
Evidence is level 4 on the critical fact — a focused differential script over the real shipped modules with a verified failing negative control at base — corroborated at level 5 by the full 5176-test suite, a clean typecheck, three passing boot smokes including full bundled-plugin activation, and byte-identical top-level help.
Final level is the maximum applicable, so: high. This is a classification of surface, not a finding of a defect. The change is correct, well tested, internally consistent, and breaks nothing in this repo; it is high because it narrows a published plugin API and no in-repo evidence can speak for plugins outside the repo. Two independent review rounds and a triage pass reached compatible conclusions; the load-bearing facts above were re-established here from the head code rather than inherited.
Before merge
The cheapest durable check that catches the real failure: make the published type enforce what the runtime now enforces, so a class-instance registration fails at a plugin author's tsc instead of at their user's activation. CommandRegistration is a plain interface, and the JSDoc added at hypaware-plugin-kernel-types.d.ts:928-942 states the own-enumerable rule in prose only. Either tighten register(command: CommandRegistration) toward an own-property-only shape, or accept the prose and add a release-note line for hypaware@1.26.0 naming the three now-rejected shapes and the two now-snapshotted semantics from the table above, so a plugin author upgrading has something to diff against.
Secondary, and independent of this PR: open a follow-up for src/core/plugin_doctor/dry_run.js:219, where the source registry has the un-copied twin of the bug this PR fixed.
philcunliffe
removed
the
neutral:approved
Conflict: the AGENTS.md release-checklist smoke battery. master (#998) added `opencode_capture` to the list; this branch rolled every entry to the canonical `hyp dev smoke` spelling. Kept both: master's full list, `opencode_capture` in its position included, on the `hyp dev smoke` spelling. No `hyp smoke ` occurrence remains in the file. hypaware-plugin-kernel-types.d.ts merged cleanly. This branch's addition there is a `CommandRegistry.register` docstring only, so nothing in the contract's shape moved.
|
Triage of the post-merge head Independent audit of the merge resolution, all confirmed:
The PR's invariants, re-verified in the merged tree (
Verification on the merged head: Known non-blocking residuals carried over from the earlier triage, unchanged by the merge: plugin-owned headerless groups (#1005), the gate test's |
Ship risk:
|
philcunliffe
added
the
neutral:approved
1 participant
All four deferred findings from the PR #857 review, re-verified live on
origin/masterata4c6350bbefore any code was written. PR #857 itself is merged (dfe50ae7,git merge-base --is-ancestorconfirms it is an ancestor of master), so none of the four had been quietly fixed since triage.1.
CommandRegistry.registermutated its argument before validating itRoot cause.
src/core/registry/commands.jsfilledcategory,audience, andbootProfilewithcommand.x ??= ...directly on the caller's object, and did it before the duplicate-name check and the alias-collision check. Two consequences: a registration the registry goes on to reject came back mutated, and a plugin passing a frozen module-level constant gotTypeError: Cannot add property category, object is not extensibleout of the defaulting instead of a registered command or a registry error.Fix. The defaulting writes into
const record = { ...command }, and the registry stores that record. Validation order is unchanged, so no error message or precedence moves.One coupling this surfaced.
src/core/cli/verb_command.jsrecognized its own projections byWeakSetidentity. Once the registry stores a copy, the objectVerbRegistry.unregisterreads back is no longer the objectverbToCommandreturned,isVerbProjectionreturned false, and releasing a verb name stopped retracting its CLI command - exactly the silent local-cache regression LLP 0264 §verb warns about. Five tests inverb-registry.test.js/command-registry-unregister.test.jscaught it. The mark is now a module-privateSymbolproperty: an object spread carries it onto the stored record, nothing outside that file can name it (so it is no more forgeable than the set was), and being a symbol it stays out ofObject.keys, JSON, and the declaredCommandRegistrationshape.2. Metadata-only groups rendered help with no header
Root cause.
hyp cache,hyp client history, andhyp dev pluginexist only as a shared prefix. They have no bare command built bymakeGroupCommandto speak for them, and core registered noCommandGroupRegistrationfor them either, sorenderGroupHelpgotgroupCommand: undefinedand skipped the header. This is the gap LLP 0214 #d2 exists to close, reopened in the new command tree.Fix.
registerCoreCommandsnow registers aCORE_COMMAND_GROUPSdescription for each of the three. Group registration is metadata only, so nothing is added tolist()and top-level help is byte-identical. (The issue namescacheandclient history;dev pluginis the same defect in the same enumeration and is fixed with them.)Evidence: failing before, passing after
New test file
test/core/command-registry-register.test.jsand one new case intest/core/cli-consistency-gate.test.js, both run against a cleanorigin/masterworktree first.User-visible, same two commands before and after:
hyp client history --helpandhyp dev plugin --helpgain their headers the same way.3.
AGENTS.mdpre-rolloverhyp smokespellings17 occurrences (16 in the release-checklist smoke battery plus the repo-layout line), rolled onto the canonical
hyp dev smoke.smokeis still a hidden alias ofdev smoke, so this is drift, not breakage.CLAUDE.mdis a symlink toAGENTS.md, so it follows.4. LLP 0248 and LLP 0266 shipped as
Draftllp/0248-task-oriented-cli-rollover.decision.mdtoAccepted(the status every other shipped Decision carries) andllp/0266-cli-compatibility-rollover.plan.mdtoActive(the status most shipped Plans carry). A status flip is an explicitly permitted mechanical edit under LLP 0156; nothing either document settled is touched.Suite numbers
Both runs on a fresh worktree with
npm install(there is no lockfile, sonpm ciis not available).origin/master(a4c6350b)No pre-existing failures showed up in this environment on either side.
npm run typecheckis clean. Smokescli_bundled_plugins_activated,package_bin_boot, andcore_boot_nooppass.Fixes #946