From ec86f543c020158dd068d2e0ff09f0d2b275ef1f Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 11:46:12 +0000 Subject: [PATCH 1/2] Implement the command registry with host error infrastructure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit createCommandRegistry({log, sink}) with last-wins registration, entry-identity-guarded Disposable, never-throwing async execute (unknown IDs and handler exceptions surface as HostErrors via the status sink), and unfiltered list(). Adds HostError/HostLog/StatusSink to core/host per design.md §4.1/§5. Fixes #4 Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WELSsojQQL1cTAR5iUUsTK --- packages/core/src/commands/index.ts | 8 +- packages/core/src/commands/registry.test.ts | 211 ++++++++++++++++++++ packages/core/src/commands/registry.ts | 122 +++++++++++ packages/core/src/host/errors.ts | 73 +++++++ packages/core/src/host/index.ts | 17 +- packages/core/src/index.ts | 6 +- 6 files changed, 433 insertions(+), 4 deletions(-) create mode 100644 packages/core/src/commands/registry.test.ts create mode 100644 packages/core/src/commands/registry.ts create mode 100644 packages/core/src/host/errors.ts diff --git a/packages/core/src/commands/index.ts b/packages/core/src/commands/index.ts index 32fc9a8..93f4d61 100644 --- a/packages/core/src/commands/index.ts +++ b/packages/core/src/commands/index.ts @@ -1,2 +1,6 @@ -// Placeholder for the command registry. -export const COMMANDS_PLACEHOLDER = true; +// The command registry (Req 3, design.md §5). +export { + createCommandRegistry, + type CommandRegistry, + type CommandRegistryDeps, +} from "./registry"; diff --git a/packages/core/src/commands/registry.test.ts b/packages/core/src/commands/registry.test.ts new file mode 100644 index 0000000..7e50051 --- /dev/null +++ b/packages/core/src/commands/registry.test.ts @@ -0,0 +1,211 @@ +import { expect, test } from "bun:test"; +import type { HostError } from "../host/errors"; +import { createHostLog } from "../host/errors"; +import { createCommandRegistry } from "./registry"; + +/** A {@link StatusSink} stub that records every error it receives, for + * assertions (design.md §5, §14). */ +function createRecordingSink() { + const errors: HostError[] = []; + return { + errors, + sink: { + error(err: HostError) { + errors.push(err); + }, + }, + }; +} + +test("execute on an unknown command ID resolves undefined, notifies the sink, and does not throw", async () => { + const log = createHostLog(); + const { errors, sink } = createRecordingSink(); + const registry = createCommandRegistry({ log, sink }); + + const result = await registry.execute("no.such.command"); + + expect(result).toBeUndefined(); + expect(errors).toHaveLength(1); + expect(errors[0]?.message).toBe("Command not found: no.such.command"); +}); + +test("execute on a throwing handler resolves undefined, logs the error, and notifies the sink", async () => { + const log = createHostLog(); + const { errors, sink } = createRecordingSink(); + const registry = createCommandRegistry({ log, sink }); + + registry.register("editor.action.boom", () => { + throw new Error("kaboom"); + }); + + const result = await registry.execute("editor.action.boom"); + + expect(result).toBeUndefined(); + expect(errors).toHaveLength(1); + expect(errors[0]?.message).toContain("kaboom"); + + const logged = log.entries(); + expect(logged).toHaveLength(1); + expect(logged[0]?.level).toBe("error"); + expect(logged[0]?.error.message).toContain("kaboom"); +}); + +test("execute on a handler that throws a non-Error value does not itself throw", async () => { + const log = createHostLog(); + const { errors, sink } = createRecordingSink(); + const registry = createCommandRegistry({ log, sink }); + + registry.register("editor.action.weird", () => { + throw "not an Error instance"; + }); + + const result = await registry.execute("editor.action.weird"); + + expect(result).toBeUndefined(); + expect(errors[0]?.message).toContain("not an Error instance"); +}); + +test("a successful handler resolves the registry's execute to its return value", async () => { + const log = createHostLog(); + const { errors, sink } = createRecordingSink(); + const registry = createCommandRegistry({ log, sink }); + + registry.register("math.add", (...args) => { + const [a, b] = args as [number, number]; + return a + b; + }); + + const result = await registry.execute("math.add", 2, 3); + + expect(result).toBe(5); + expect(errors).toHaveLength(0); +}); + +test("an async handler's resolved value is awaited through execute", async () => { + const log = createHostLog(); + const { sink } = createRecordingSink(); + const registry = createCommandRegistry({ log, sink }); + + registry.register("workspace.save", async () => { + await Promise.resolve(); + return "saved"; + }); + + const result = await registry.execute("workspace.save"); + + expect(result).toBe("saved"); +}); + +test("dispose removes the command, so a subsequent execute reports it unknown", async () => { + const log = createHostLog(); + const { errors, sink } = createRecordingSink(); + const registry = createCommandRegistry({ log, sink }); + + const registration = registry.register("editor.action.foo", () => "ok"); + expect(await registry.execute("editor.action.foo")).toBe("ok"); + + registration.dispose(); + + const result = await registry.execute("editor.action.foo"); + expect(result).toBeUndefined(); + expect(errors.at(-1)?.message).toBe("Command not found: editor.action.foo"); +}); + +test("double-dispose is a no-op", async () => { + const log = createHostLog(); + const { sink } = createRecordingSink(); + const registry = createCommandRegistry({ log, sink }); + + const registration = registry.register("editor.action.foo", () => "ok"); + registration.dispose(); + expect(() => registration.dispose()).not.toThrow(); + + expect(await registry.execute("editor.action.foo")).toBeUndefined(); +}); + +test("dispose after re-registration under the same ID does not remove the new handler", async () => { + const log = createHostLog(); + const { sink } = createRecordingSink(); + const registry = createCommandRegistry({ log, sink }); + + const first = registry.register("editor.action.foo", () => "first"); + const second = registry.register("editor.action.foo", () => "second"); + + // The stale handle from the superseded registration must not remove the + // still-current one (entry-identity comparison, design.md §5). + first.dispose(); + expect(await registry.execute("editor.action.foo")).toBe("second"); + + second.dispose(); + expect(await registry.execute("editor.action.foo")).toBeUndefined(); +}); + +test("re-registering the same ID is last-wins and logs a warning", async () => { + const log = createHostLog(); + const { sink } = createRecordingSink(); + const registry = createCommandRegistry({ log, sink }); + + registry.register("editor.action.foo", () => "first"); + registry.register("editor.action.foo", () => "second"); + + expect(await registry.execute("editor.action.foo")).toBe("second"); + + const warnings = log.entries().filter((e) => e.level === "warning"); + expect(warnings).toHaveLength(1); + expect(warnings[0]?.error.message).toContain("editor.action.foo"); +}); + +test("list returns id, title, category, and when for every registered command", () => { + const log = createHostLog(); + const { sink } = createRecordingSink(); + const registry = createCommandRegistry({ log, sink }); + + registry.register("editor.action.deleteLine", () => undefined, { + title: "Delete Line", + category: "Editor", + when: "editorTextFocus", + }); + registry.register("explorer.reveal", () => undefined); + + const descriptors = registry.list(); + expect(descriptors).toHaveLength(2); + + const deleteLine = descriptors.find((d) => d.id === "editor.action.deleteLine"); + expect(deleteLine).toEqual({ + id: "editor.action.deleteLine", + title: "Delete Line", + category: "Editor", + when: "editorTextFocus", + }); + + const reveal = descriptors.find((d) => d.id === "explorer.reveal"); + expect(reveal).toEqual({ + id: "explorer.reveal", + title: undefined, + category: undefined, + when: undefined, + }); +}); + +test("list does not filter by when — that is the caller's responsibility (design.md §5)", () => { + const log = createHostLog(); + const { sink } = createRecordingSink(); + const registry = createCommandRegistry({ log, sink }); + + registry.register("explorer.reveal", () => undefined, { when: "false" }); + + expect(registry.list().map((d) => d.id)).toEqual(["explorer.reveal"]); +}); + +test("createHostLog accumulates entries in append order and starts empty", () => { + const log = createHostLog(); + expect(log.entries()).toEqual([]); + + log.append("warning", { message: "first" }); + log.append("error", { message: "second", extensionId: "demo" }); + + expect(log.entries()).toEqual([ + { level: "warning", error: { message: "first" } }, + { level: "error", error: { message: "second", extensionId: "demo" } }, + ]); +}); diff --git a/packages/core/src/commands/registry.ts b/packages/core/src/commands/registry.ts new file mode 100644 index 0000000..e55d1b7 --- /dev/null +++ b/packages/core/src/commands/registry.ts @@ -0,0 +1,122 @@ +/** + * The command registry (Req 3, design.md §5): a `Map` + * backing `tecode.commands`. All cross-module behavior in tecode — key + * bindings, the palette, UI callbacks, extension-to-extension calls — goes + * through `execute` rather than a direct function call (Req 1.5). + * + * Lazy (manifest-declared, not-yet-activated) commands are out of scope + * here — they arrive with the extension host task (design.md §4.1's + * "lazy commands"); `CommandEntry` therefore carries no `lazy` flag. + */ + +import type { + CommandDescriptor, + CommandHandler, + CommandMeta, + Disposable, +} from "@tecode/api"; +import type { HostError, HostLog, StatusSink } from "../host/errors"; + +/** Internal registry state for one registered command. */ +interface CommandEntry { + handler: CommandHandler; + meta: CommandMeta; +} + +/** Dependencies a {@link createCommandRegistry} instance reports through + * rather than owning directly (design.md §5, §14). */ +export interface CommandRegistryDeps { + /** Structured log for warnings (duplicate registration) and errors + * (handler exceptions). */ + log: HostLog; + /** Where user-facing command errors are surfaced (Req 3.4, 3.5). */ + sink: StatusSink; +} + +/** The public shape of the command registry — the implementation behind + * `tecode.commands` (Req 10.1). */ +export interface CommandRegistry { + register(id: string, handler: CommandHandler, meta?: CommandMeta): Disposable; + execute(id: string, ...args: unknown[]): Promise; + list(): CommandDescriptor[]; +} + +/** Render a caught `unknown` value as a message string without risking a + * second throw (e.g. from a non-Error with a throwing `toString`). */ +function describeError(err: unknown): string { + if (err instanceof Error) return err.message; + try { + return String(err); + } catch { + return "Unknown error"; + } +} + +/** + * Build a command registry (Req 3.1). `register`/`execute`/`list` are the + * exact operations `tecode.commands` exposes to extensions; `deps` lets the + * host inject the shared {@link HostLog} and {@link StatusSink} rather than + * the registry owning them. + */ +export function createCommandRegistry(deps: CommandRegistryDeps): CommandRegistry { + const { log, sink } = deps; + const commands = new Map(); + + function register( + id: string, + handler: CommandHandler, + meta: CommandMeta = {}, + ): Disposable { + if (commands.has(id)) { + log.append("warning", { + message: `Command re-registered, replacing previous handler: ${id}`, + }); + } + const entry: CommandEntry = { handler, meta }; + commands.set(id, entry); + + let disposed = false; + return { + dispose() { + if (disposed) return; + disposed = true; + // Only remove if this registration is still the current one — + // re-registration (last-wins) or a prior dispose may have already + // replaced/removed it, and this dispose must be a no-op then. + if (commands.get(id) === entry) { + commands.delete(id); + } + }, + }; + } + + async function execute(id: string, ...args: unknown[]): Promise { + const entry = commands.get(id); + if (!entry) { + const err: HostError = { message: `Command not found: ${id}` }; + sink.error(err); + return undefined; + } + try { + return await entry.handler(...args); + } catch (cause: unknown) { + const err: HostError = { + message: `Command "${id}" threw: ${describeError(cause)}`, + }; + log.append("error", err); + sink.error(err); + return undefined; + } + } + + function list(): CommandDescriptor[] { + return Array.from(commands.entries()).map(([id, entry]) => ({ + id, + title: entry.meta.title, + category: entry.meta.category, + when: entry.meta.when, + })); + } + + return { register, execute, list }; +} diff --git a/packages/core/src/host/errors.ts b/packages/core/src/host/errors.ts new file mode 100644 index 0000000..c624d16 --- /dev/null +++ b/packages/core/src/host/errors.ts @@ -0,0 +1,73 @@ +/** + * Host-side error and logging primitives (design.md §4.1, §14). `HostError` + * is the shared shape for both extension-loading failures (manifest + * validation, API version mismatches) and command failures (Req 3.4, 3.5) — + * anywhere the host needs to report a problem without throwing across a + * public API boundary. + */ + +/** + * A structured error the host can attribute to an extension and/or a file + * path (design.md §4.1). `extensionId`/`path` are omitted when not + * applicable — e.g. a command-not-found error carries neither. + */ +export interface HostError { + extensionId?: string; + path?: string; + message: string; +} + +/** The severity of one {@link HostLog} entry. */ +export type HostLogLevel = "error" | "warning"; + +/** One recorded entry in a {@link HostLog}. */ +export interface HostLogEntry { + level: HostLogLevel; + error: HostError; +} + +/** + * A minimal structured log the host and its services append to (design.md + * §14: "A core `HostLog` collects structured errors"). Kept intentionally + * small for the MVP — just an append-only record with retrieval; a + * `developer.showLog` command can later dump {@link HostLog.entries} into an + * untitled document. + */ +export interface HostLog { + /** Append an entry at the given severity. */ + append(level: HostLogLevel, error: HostError): void; + /** All entries recorded so far, oldest first. */ + entries(): readonly HostLogEntry[]; +} + +/** Create an empty, in-memory {@link HostLog}. */ +export function createHostLog(): HostLog { + const records: HostLogEntry[] = []; + return { + append(level, error) { + records.push({ level, error }); + }, + entries() { + return records; + }, + }; +} + +/** + * Where the host sends user-facing error notifications — real UI wiring + * (the status bar) lands in a later task; for now services depend only on + * this narrow interface (design.md §5, §14). + */ +export interface StatusSink { + error(err: HostError): void; +} + +/** A {@link StatusSink} that discards everything — the default for tests + * and for any composition root that hasn't wired real UI yet. */ +export function createNoopStatusSink(): StatusSink { + return { + error() { + // Intentionally discarded — see StatusSink's TSDoc. + }, + }; +} diff --git a/packages/core/src/host/index.ts b/packages/core/src/host/index.ts index 42b4514..11f7e32 100644 --- a/packages/core/src/host/index.ts +++ b/packages/core/src/host/index.ts @@ -1,2 +1,17 @@ -// Placeholder for the extension host (discovery, manifest validation, activation). +// Extension host (discovery, manifest validation, activation) — the rest of +// design.md §4 lands in later tasks. For now this module exposes the shared +// error/log infrastructure (§4.1) that both host loading and the command +// registry (§5) depend on. +export { + createHostLog, + createNoopStatusSink, + type HostError, + type HostLog, + type HostLogEntry, + type HostLogLevel, + type StatusSink, +} from "./errors"; + +/** Placeholder for the remaining extension-host behavior (discovery, + * manifest validation, activation) — see design.md §4. */ export const HOST_PLACEHOLDER = true; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 706b8fc..14d9974 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -1,6 +1,10 @@ // Placeholder entry point for @tecode/core. Real wiring lands in later tasks. export { HOST_PLACEHOLDER } from "./host/index"; -export { COMMANDS_PLACEHOLDER } from "./commands/index"; +export { + createCommandRegistry, + type CommandRegistry, + type CommandRegistryDeps, +} from "./commands/index"; export { KEYMAP_PLACEHOLDER } from "./keymap/index"; export { BUFFER_PLACEHOLDER } from "./buffer/index"; export { UI_PLACEHOLDER } from "./ui/index"; From 32496c03eb652829b504746f37e446101fdfaf77 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 22 Aug 2026 12:02:20 +0000 Subject: [PATCH 2/2] Address CodeRabbit review on command registry PR - guard sink.error and log.append behind safe helpers and move Error.message access inside describeError's try, so execute keeps its never-throwing contract even when injected reporters fail - validate command IDs at registration against the namespace.verb form (Req 3.2) via a shared exported isValidCommandId; invalid IDs throw TypeError before touching the map - HostLog.entries now returns a cloned snapshot to keep the log append-only - tests for throwing sink/log, throwing message getter, invalid IDs, and the entries snapshot (24 tests total) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WELSsojQQL1cTAR5iUUsTK --- packages/core/src/commands/index.ts | 1 + packages/core/src/commands/registry.test.ts | 78 +++++++++++++++++++++ packages/core/src/commands/registry.ts | 46 ++++++++++-- packages/core/src/host/errors.ts | 4 +- packages/core/src/index.ts | 1 + 5 files changed, 123 insertions(+), 7 deletions(-) diff --git a/packages/core/src/commands/index.ts b/packages/core/src/commands/index.ts index 93f4d61..0003571 100644 --- a/packages/core/src/commands/index.ts +++ b/packages/core/src/commands/index.ts @@ -1,6 +1,7 @@ // The command registry (Req 3, design.md §5). export { createCommandRegistry, + isValidCommandId, type CommandRegistry, type CommandRegistryDeps, } from "./registry"; diff --git a/packages/core/src/commands/registry.test.ts b/packages/core/src/commands/registry.test.ts index 7e50051..e99ab22 100644 --- a/packages/core/src/commands/registry.test.ts +++ b/packages/core/src/commands/registry.test.ts @@ -209,3 +209,81 @@ test("createHostLog accumulates entries in append order and starts empty", () => { level: "error", error: { message: "second", extensionId: "demo" } }, ]); }); + +test("execute keeps its never-throwing contract when the sink itself throws", async () => { + const log = createHostLog(); + const throwingSink = { + error() { + throw new Error("sink is broken"); + }, + }; + const registry = createCommandRegistry({ log, sink: throwingSink }); + + expect(await registry.execute("no.such.command")).toBeUndefined(); + + registry.register("editor.action.boom", () => { + throw new Error("handler failure"); + }); + expect(await registry.execute("editor.action.boom")).toBeUndefined(); +}); + +test("execute keeps its never-throwing contract when the log itself throws", async () => { + const throwingLog = { + append() { + throw new Error("log is broken"); + }, + entries: () => [], + }; + const { sink } = createRecordingSink(); + const registry = createCommandRegistry({ log: throwingLog, sink }); + + registry.register("editor.action.boom", () => { + throw new Error("handler failure"); + }); + expect(await registry.execute("editor.action.boom")).toBeUndefined(); + // Re-registration warnings also go through the guarded log path. + expect(() => registry.register("editor.action.boom", () => "ok")).not.toThrow(); +}); + +test("execute survives an error whose message getter throws", async () => { + const log = createHostLog(); + const { errors, sink } = createRecordingSink(); + const registry = createCommandRegistry({ log, sink }); + + registry.register("editor.action.cursed", () => { + const cursed = new Error("unreachable"); + Object.defineProperty(cursed, "message", { + get() { + throw new Error("message getter throws"); + }, + }); + throw cursed; + }); + + expect(await registry.execute("editor.action.cursed")).toBeUndefined(); + expect(errors[0]?.message).toContain("Unknown error"); +}); + +test("register rejects command IDs that are not namespace.verb form", () => { + const log = createHostLog(); + const { sink } = createRecordingSink(); + const registry = createCommandRegistry({ log, sink }); + + for (const bad of ["save", "editor.", ".save", "editor..save", "editor save.x", ""]) { + expect(() => registry.register(bad, () => undefined)).toThrow(TypeError); + } + // Multi-segment IDs are valid (e.g. editor.action.deleteLine). + expect(() => + registry.register("editor.action.deleteLine", () => undefined), + ).not.toThrow(); +}); + +test("HostLog.entries returns a snapshot, not the internal records", () => { + const log = createHostLog(); + log.append("error", { message: "original" }); + + const snapshot = log.entries(); + snapshot[0]!.error.message = "mutated"; + + expect(log.entries()[0]?.error.message).toBe("original"); +}); diff --git a/packages/core/src/commands/registry.ts b/packages/core/src/commands/registry.ts index e55d1b7..006a217 100644 --- a/packages/core/src/commands/registry.ts +++ b/packages/core/src/commands/registry.ts @@ -42,16 +42,26 @@ export interface CommandRegistry { } /** Render a caught `unknown` value as a message string without risking a - * second throw (e.g. from a non-Error with a throwing `toString`). */ + * second throw (e.g. a throwing `toString`, or an `Error` subclass whose + * `message` getter throws). */ function describeError(err: unknown): string { - if (err instanceof Error) return err.message; try { + if (err instanceof Error) return err.message; return String(err); } catch { return "Unknown error"; } } +/** + * Whether `id` follows the `namespace.verb` convention (Req 3.2): two or + * more non-empty, whitespace-free segments separated by dots (e.g. + * `editor.action.deleteLine`). Shared so manifest validation can reuse it. + */ +export function isValidCommandId(id: string): boolean { + return /^[^\s.]+(\.[^\s.]+)+$/.test(id); +} + /** * Build a command registry (Req 3.1). `register`/`execute`/`list` are the * exact operations `tecode.commands` exposes to extensions; `deps` lets the @@ -62,13 +72,37 @@ export function createCommandRegistry(deps: CommandRegistryDeps): CommandRegistr const { log, sink } = deps; const commands = new Map(); + /** Guarded `log.append` — an injected log must not be able to break the + * registry's error paths (execute's never-throwing contract). */ + function logSafely(level: "error" | "warning", err: HostError): void { + try { + log.append(level, err); + } catch { + // Swallowed: reporting a reporting failure has nowhere left to go. + } + } + + /** Guarded `sink.error` — same rationale as {@link logSafely}. */ + function notifySafely(err: HostError): void { + try { + sink.error(err); + } catch { + // Swallowed: preserve execute()'s never-throwing contract. + } + } + function register( id: string, handler: CommandHandler, meta: CommandMeta = {}, ): Disposable { + if (!isValidCommandId(id)) { + throw new TypeError( + `Invalid command ID "${id}": expected namespace.verb form (Req 3.2)`, + ); + } if (commands.has(id)) { - log.append("warning", { + logSafely("warning", { message: `Command re-registered, replacing previous handler: ${id}`, }); } @@ -94,7 +128,7 @@ export function createCommandRegistry(deps: CommandRegistryDeps): CommandRegistr const entry = commands.get(id); if (!entry) { const err: HostError = { message: `Command not found: ${id}` }; - sink.error(err); + notifySafely(err); return undefined; } try { @@ -103,8 +137,8 @@ export function createCommandRegistry(deps: CommandRegistryDeps): CommandRegistr const err: HostError = { message: `Command "${id}" threw: ${describeError(cause)}`, }; - log.append("error", err); - sink.error(err); + logSafely("error", err); + notifySafely(err); return undefined; } } diff --git a/packages/core/src/host/errors.ts b/packages/core/src/host/errors.ts index c624d16..53b78a9 100644 --- a/packages/core/src/host/errors.ts +++ b/packages/core/src/host/errors.ts @@ -48,7 +48,9 @@ export function createHostLog(): HostLog { records.push({ level, error }); }, entries() { - return records; + // Snapshot: cloning each entry (and its error) keeps the log + // append-only — callers can't mutate accumulated records. + return records.map(({ level, error }) => ({ level, error: { ...error } })); }, }; } diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 14d9974..601b7bf 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -2,6 +2,7 @@ export { HOST_PLACEHOLDER } from "./host/index"; export { createCommandRegistry, + isValidCommandId, type CommandRegistry, type CommandRegistryDeps, } from "./commands/index";