diff --git a/.changeset/176-hub-p1.2-review-findings.md b/.changeset/176-hub-p1.2-review-findings.md new file mode 100644 index 000000000..c48c1fe47 --- /dev/null +++ b/.changeset/176-hub-p1.2-review-findings.md @@ -0,0 +1,7 @@ +--- +type: Changed +pr: 176 +--- +`CommandRoutingHub` P1.2 review findings addressed: Hub now runtime-validates handler-returned `{ ok: false, ... }` results against the typed schema and coerces malformed variants to `HandlerFailure` with a contract-violation message (Finding 1). The catch path wraps non-Error throwables in an Error with `.thrown` attached so the original value is never silently dropped (Finding 2). All four factory functions (`makeUnknownCommand`, `makeInvalidArgs`, `makeHandlerRefusal`, `makeHandlerFailure`) now return `Object.freeze`'d objects to protect the variant invariant (Finding 3). `makeHandlerFailure` validates its `cause` argument and wraps non-Error causes in an Error with `.thrown` so downstream callers can safely access `.cause.stack` (Finding 4). + + diff --git a/.changeset/176-typed-result-discriminated-union.md b/.changeset/176-typed-result-discriminated-union.md new file mode 100644 index 000000000..76fefaae8 --- /dev/null +++ b/.changeset/176-typed-result-discriminated-union.md @@ -0,0 +1,7 @@ +--- +type: Changed +pr: 176 +--- +`CommandRoutingHub` error results are now a typed discriminated union (ADR-0174 P1.2). The `errorKind` field is renamed to `kind` and each variant carries only its own typed payload: `UnknownCommand` → `{ kind, command }`; `InvalidArgs` → `{ kind, arg, reason }`; `HandlerRefusal` → `{ kind, reason }`; `HandlerFailure` → `{ kind, message, cause? }`. Factory functions `makeUnknownCommand`, `makeInvalidArgs`, `makeHandlerRefusal`, `makeHandlerFailure` are exported for handler and caller use. The generic `message`/`details` escape hatches are removed from Hub-emitted errors. + + diff --git a/get-shit-done/bin/lib/command-routing-hub.cjs b/get-shit-done/bin/lib/command-routing-hub.cjs index ddd0951f5..f51662431 100644 --- a/get-shit-done/bin/lib/command-routing-hub.cjs +++ b/get-shit-done/bin/lib/command-routing-hub.cjs @@ -1,7 +1,7 @@ 'use strict'; /** - * Command Routing Hub — issue #3788, simplified in #175. + * Command Routing Hub — issue #3788, simplified in #175, typed in #176. * * A pure-result dispatch hub that centralizes CJS routing, * the error taxonomy, and the no-throw contract that all command-family routers @@ -12,21 +12,27 @@ * hub.dispatch({ family, subcommand, args, cwd, raw }) -> Result * * Result = { ok: true, data } - * | { ok: false, errorKind, message, details? } + * | { ok: false, kind: 'UnknownCommand', command: string } + * | { ok: false, kind: 'InvalidArgs', arg: string, reason: string } + * | { ok: false, kind: 'HandlerRefusal', reason: string } + * | { ok: false, kind: 'HandlerFailure', message: string, cause?: Error } * * Invariants: * - Hub always routes through CJS handlers. There is no SDK path (#175). * - Hub never prints to stdout/stderr, never calls process.exit. * - Hub never throws — all internal throws are caught and converted to - * { ok: false, errorKind: 'HandlerFailure', message, details }. - * - The errorKind taxonomy is closed. Callers switch on ERROR_KINDS values. + * { ok: false, kind: 'HandlerFailure', message, cause }. + * - The kind taxonomy is closed. Callers switch on ERROR_KINDS values. + * - Each error variant carries ONLY its own typed payload (#176). + * No cross-variant `message`/`details` escape hatches. */ /** - * Closed errorKind enum. Export as a frozen object so callers can switch on + * Closed error-kind enum. Export as a frozen object so callers can switch on * ERROR_KINDS.UnknownCommand etc. without relying on bare string literals. * * #175: SdkLoadFailed and SdkDispatchFailed removed — Hub is CJS-only. + * #176: Field renamed errorKind → kind; payloads are typed per variant. * * @readonly */ @@ -41,9 +47,152 @@ const ERROR_KINDS = Object.freeze({ HandlerFailure: 'HandlerFailure', }); +// ─── Internal helpers ───────────────────────────────────────────────────────── + +/** + * Safe JSON serialisation that never throws. + * @param {unknown} value + * @returns {string} + */ +function _safeJson(value) { + try { + return JSON.stringify(value); + } catch { + return String(value); + } +} + +// ─── Typed-payload factories (#176) ────────────────────────────────────────── +// Each factory returns a frozen discriminated-union variant for its kind. +// No cross-variant fields bleed between variants. +// Finding 3: all factory returns are Object.freeze'd so callers cannot mutate +// the variant invariant. + +/** + * @param {string} command - The unrecognised command string (family or family+subcommand). + * @returns {Readonly<{ ok: false, kind: 'UnknownCommand', command: string }>} + */ +function makeUnknownCommand(command) { + return Object.freeze({ ok: false, kind: ERROR_KINDS.UnknownCommand, command }); +} + +/** + * @param {string} arg - The argument token that failed validation. + * @param {string} reason - Human-readable explanation of the failure. + * @returns {Readonly<{ ok: false, kind: 'InvalidArgs', arg: string, reason: string }>} + */ +function makeInvalidArgs(arg, reason) { + return Object.freeze({ ok: false, kind: ERROR_KINDS.InvalidArgs, arg, reason }); +} + +/** + * @param {string} reason - Human-readable explanation for the refusal. + * @returns {Readonly<{ ok: false, kind: 'HandlerRefusal', reason: string }>} + */ +function makeHandlerRefusal(reason) { + return Object.freeze({ ok: false, kind: ERROR_KINDS.HandlerRefusal, reason }); +} + +/** + * @param {string} message - Human-readable description of the failure. + * @param {Error} [cause] - The original thrown Error, when available. + * Non-Error values (strings, plain objects, etc.) are wrapped in an Error + * with `.thrown` set to the original value. null/undefined → no cause field. + * @returns {{ ok: false, kind: 'HandlerFailure', message: string, cause?: Error }} + */ +function makeHandlerFailure(message, cause) { + const obj = { ok: false, kind: ERROR_KINDS.HandlerFailure, message }; + if (cause != null) { + if (cause instanceof Error) { + obj.cause = cause; + } else { + // Finding 4: wrap non-Error cause so downstream .cause.stack never silently returns undefined + const wrapper = new Error('non-Error cause: ' + _safeJson(cause)); + wrapper.thrown = cause; + obj.cause = wrapper; + } + } + return Object.freeze(obj); +} + +// ─── Handler-return shape validator (Finding 1) ─────────────────────────────── + +/** + * Required payload fields per ok:false kind. + * `required` — fields that MUST be present (non-undefined) for the variant to be valid. + * `allowed` — the complete set of allowed fields (including ok, kind). + * + * @type {Record }>} + */ +const _VARIANT_SCHEMA = { + UnknownCommand: { + required: ['command'], + allowed: new Set(['ok', 'kind', 'command']), + }, + InvalidArgs: { + required: ['arg', 'reason'], + allowed: new Set(['ok', 'kind', 'arg', 'reason']), + }, + HandlerRefusal: { + required: ['reason'], + allowed: new Set(['ok', 'kind', 'reason']), + }, + HandlerFailure: { + required: ['message'], + allowed: new Set(['ok', 'kind', 'message', 'cause']), + }, +}; + +/** + * Validates a handler-returned { ok: false, ... } result against the typed schema. + * + * Returns null if valid, or a string describing the contract violation. + * + * @param {object} result + * @returns {string|null} + */ +function _validateErrResult(result) { + const { kind } = result; + const schema = _VARIANT_SCHEMA[kind]; + + // Unknown kind — not in the closed enum + if (!schema) { + return `handler returned unknown kind '${kind}': expected one of ${Object.keys(_VARIANT_SCHEMA).join(', ')}`; + } + + // Missing required fields + for (const field of schema.required) { + if (result[field] === undefined) { + return ( + `handler returned malformed Result variant: ` + + `kind '${kind}' requires field '${field}' but it is missing. ` + + `got: ${_safeJson(result)}` + ); + } + } + + // Extraneous fields outside the typed payload + for (const key of Object.keys(result)) { + if (!schema.allowed.has(key)) { + return ( + `handler returned malformed Result variant: ` + + `kind '${kind}' does not allow field '${key}'. ` + + `expected fields: ${[...schema.allowed].join(', ')}. ` + + `got: ${_safeJson(result)}` + ); + } + } + + return null; // valid +} + /** * @typedef {{ ok: true, data: unknown }} OkResult - * @typedef {{ ok: false, errorKind: string, message: string, details?: unknown }} ErrResult + * @typedef {{ ok: false, kind: 'UnknownCommand', command: string }} UnknownCommandResult + * @typedef {{ ok: false, kind: 'InvalidArgs', arg: string, reason: string }} InvalidArgsResult + * @typedef {{ ok: false, kind: 'HandlerRefusal', reason: string }} HandlerRefusalResult + * @typedef {{ ok: false, kind: 'HandlerFailure', message: string, cause?: Error }} HandlerFailureResult + * @typedef {UnknownCommandResult | InvalidArgsResult | HandlerRefusalResult | HandlerFailureResult} ErrResult * @typedef {OkResult | ErrResult} HubResult */ @@ -75,12 +224,13 @@ function createHub({ cjsRegistry, manifest } = {}) { try { return _dispatch(req); } catch (err) { - return { - ok: false, - errorKind: ERROR_KINDS.HandlerFailure, - message: err instanceof Error ? err.message : String(err), - details: { originalError: err }, - }; + if (err instanceof Error) { + return makeHandlerFailure(err.message, err); + } + // Finding 2: preserve non-Error throwables via a wrapper Error with .thrown + const wrapper = new Error('non-Error thrown: ' + _safeJson(err)); + wrapper.thrown = err; + return makeHandlerFailure(String(err), wrapper); } } @@ -91,18 +241,10 @@ function createHub({ cjsRegistry, manifest } = {}) { if (_manifest) { const knownSubcommands = _manifest[family]; if (!knownSubcommands) { - return { - ok: false, - errorKind: ERROR_KINDS.UnknownCommand, - message: `Unknown command family: ${family}`, - }; + return makeUnknownCommand(String(family)); } if (subcommand && !knownSubcommands.includes(subcommand)) { - return { - ok: false, - errorKind: ERROR_KINDS.UnknownCommand, - message: `Unknown subcommand: ${family} ${subcommand}`, - }; + return makeUnknownCommand(`${family} ${subcommand}`); } } @@ -111,37 +253,35 @@ function createHub({ cjsRegistry, manifest } = {}) { function _dispatchCjs({ family, subcommand, args, cwd, raw }) { if (!_cjsRegistry) { - return { - ok: false, - errorKind: ERROR_KINDS.UnknownCommand, - message: `No CJS registry provided for family: ${family}`, - }; + return makeUnknownCommand(String(family)); } const familyHandlers = _cjsRegistry[family]; if (!familyHandlers) { - return { - ok: false, - errorKind: ERROR_KINDS.UnknownCommand, - message: `Unknown command family: ${family}`, - }; + return makeUnknownCommand(String(family)); } const handler = subcommand ? familyHandlers[subcommand] : familyHandlers['']; if (typeof handler !== 'function') { - return { - ok: false, - errorKind: ERROR_KINDS.UnknownCommand, - message: `Unknown subcommand: ${family} ${subcommand}`, - }; + return makeUnknownCommand(subcommand ? `${family} ${subcommand}` : String(family)); } // Invoke the handler. It must return a HubResult or throw. // If it throws, the outer try/catch in dispatch() catches it. const result = handler({ family, subcommand, args, cwd, raw }); - // If the handler returned a well-formed HubResult, pass it through. + // If the handler returned a HubResult, validate ok:false variants against the typed schema. if (result && typeof result === 'object' && 'ok' in result) { + if (!result.ok) { + // Finding 1: runtime-validate ok:false variant shape; coerce malformed to HandlerFailure + const violation = _validateErrResult(result); + if (violation !== null) { + return makeHandlerFailure( + 'handler returned malformed Result variant: ' + violation, + new Error('expected ' + (result.kind || '') + ', got ' + _safeJson(result)) + ); + } + } return result; } @@ -160,4 +300,8 @@ function createHub({ cjsRegistry, manifest } = {}) { module.exports = { createHub, ERROR_KINDS, + makeUnknownCommand, + makeInvalidArgs, + makeHandlerRefusal, + makeHandlerFailure, }; diff --git a/get-shit-done/bin/lib/phase-command-router.cjs b/get-shit-done/bin/lib/phase-command-router.cjs index b520634dd..8f935480f 100644 --- a/get-shit-done/bin/lib/phase-command-router.cjs +++ b/get-shit-done/bin/lib/phase-command-router.cjs @@ -2,8 +2,8 @@ const { PHASE_SUBCOMMANDS } = require('./command-aliases.generated.cjs'); -// ─── CommandRoutingHub (issue #3788, simplified in #175) ────────────────────── -const { createHub, ERROR_KINDS } = require('./command-routing-hub.cjs'); +// ─── CommandRoutingHub (issue #3788, simplified in #175, typed in #176) ─────── +const { createHub, ERROR_KINDS, makeInvalidArgs } = require('./command-routing-hub.cjs'); /** * Manifest-backed phase subcommand router. @@ -78,12 +78,12 @@ function routePhaseCommand({ phase, args, cwd, raw, error }) { if (token === '--id') { const id = args[i + 1]; if (!id || id.startsWith('--')) { - return { ok: false, errorKind: 'InvalidArgs', message: '--id requires a value' }; + return makeInvalidArgs('--id', '--id requires a value'); } customId = id; i++; } else if (token.startsWith('--')) { - return { ok: false, errorKind: 'InvalidArgs', message: `phase add does not support ${token}` }; + return makeInvalidArgs(token, `phase add does not support ${token}`); } else { descArgs.push(token); } @@ -97,15 +97,15 @@ function routePhaseCommand({ phase, args, cwd, raw, error }) { if (descFlagIdx !== -1) { const rawDescriptions = args[descFlagIdx + 1]; if (!rawDescriptions || rawDescriptions.startsWith('--')) { - return { ok: false, errorKind: 'InvalidArgs', message: '--descriptions must be a JSON array' }; + return makeInvalidArgs('--descriptions', '--descriptions must be a JSON array'); } try { descriptions = JSON.parse(rawDescriptions); } catch { - return { ok: false, errorKind: 'InvalidArgs', message: '--descriptions must be a JSON array' }; + return makeInvalidArgs('--descriptions', '--descriptions must be a JSON array'); } if (!Array.isArray(descriptions)) { - return { ok: false, errorKind: 'InvalidArgs', message: '--descriptions must be a JSON array' }; + return makeInvalidArgs('--descriptions', '--descriptions must be a JSON array'); } } else { descriptions = args.slice(2).filter(a => a !== '--raw'); @@ -115,7 +115,7 @@ function routePhaseCommand({ phase, args, cwd, raw, error }) { }, insert: (_ctx) => { if (args.includes('--dry-run')) { - return { ok: false, errorKind: 'InvalidArgs', message: 'phase insert does not support --dry-run' }; + return makeInvalidArgs('--dry-run', 'phase insert does not support --dry-run'); } phase.cmdPhaseInsert(cwd, args[2], args.slice(3).join(' '), raw); return { ok: true, data: null }; @@ -130,12 +130,12 @@ function routePhaseCommand({ phase, args, cwd, raw, error }) { continue; } if (token.startsWith('--')) { - return { ok: false, errorKind: 'InvalidArgs', message: `phase remove does not support ${token}` }; + return makeInvalidArgs(token, `phase remove does not support ${token}`); } positional.push(token); } if (positional.length !== 1) { - return { ok: false, errorKind: 'InvalidArgs', message: 'phase remove accepts exactly one phase number' }; + return makeInvalidArgs('', 'phase remove accepts exactly one phase number'); } phase.cmdPhaseRemove(cwd, positional[0], { force: forceFlag }, raw); return { ok: true, data: null }; @@ -177,12 +177,17 @@ function routePhaseCommand({ phase, args, cwd, raw, error }) { // CJS handlers call output() themselves (inside phase.cmdPhase*()). // No further output call is needed here. if (!result.ok) { - if (result.errorKind === ERROR_KINDS.UnknownCommand) { + if (result.kind === ERROR_KINDS.UnknownCommand) { const available = availableSubcommands.join(', '); error(`Unknown phase subcommand. Available: ${available}`); return; } - // InvalidArgs, HandlerRefusal, HandlerFailure + if (result.kind === ERROR_KINDS.InvalidArgs || result.kind === ERROR_KINDS.HandlerRefusal) { + // #176: typed payload — reason holds the human-readable message + error(result.reason); + return; + } + // HandlerFailure: message field error(result.message); return; } diff --git a/tests/command-routing-hub.test.cjs b/tests/command-routing-hub.test.cjs index 94acffb56..624b2c42d 100644 --- a/tests/command-routing-hub.test.cjs +++ b/tests/command-routing-hub.test.cjs @@ -18,7 +18,14 @@ const { describe, test } = require('node:test'); const assert = require('node:assert/strict'); -const { createHub, ERROR_KINDS } = require('../get-shit-done/bin/lib/command-routing-hub.cjs'); +const { + createHub, + ERROR_KINDS, + makeUnknownCommand, + makeInvalidArgs, + makeHandlerRefusal, + makeHandlerFailure, +} = require('../get-shit-done/bin/lib/command-routing-hub.cjs'); // ─── Frozen taxonomy lock ───────────────────────────────────────────────────── // #175: SdkDispatchFailed and SdkLoadFailed are removed from the closed enum. @@ -200,9 +207,9 @@ describe('CommandRoutingHub — happy path, CJS dispatch', () => { }); }); -// ─── errorKind: UnknownCommand ──────────────────────────────────────────────── +// ─── kind: UnknownCommand ───────────────────────────────────────────────────── -describe('CommandRoutingHub — errorKind: UnknownCommand', () => { +describe('CommandRoutingHub — kind: UnknownCommand', () => { test('unknown family in manifest returns UnknownCommand', () => { const hub = createHub({ cjsRegistry: {}, @@ -212,7 +219,7 @@ describe('CommandRoutingHub — errorKind: UnknownCommand', () => { const result = hub.dispatch({ family: 'bogus', subcommand: 'add', args: [], cwd: '/', raw: false }); assert.ok(!result.ok); - assert.equal(result.errorKind, ERROR_KINDS.UnknownCommand); + assert.equal(result.kind, ERROR_KINDS.UnknownCommand); }); test('unknown subcommand in manifest returns UnknownCommand', () => { @@ -224,7 +231,7 @@ describe('CommandRoutingHub — errorKind: UnknownCommand', () => { const result = hub.dispatch({ family: 'phase', subcommand: 'nonexistent', args: [], cwd: '/', raw: false }); assert.ok(!result.ok); - assert.equal(result.errorKind, ERROR_KINDS.UnknownCommand); + assert.equal(result.kind, ERROR_KINDS.UnknownCommand); }); test('missing family in cjsRegistry returns UnknownCommand (no manifest)', () => { @@ -235,7 +242,7 @@ describe('CommandRoutingHub — errorKind: UnknownCommand', () => { const result = hub.dispatch({ family: 'bogus-family', subcommand: 'sub', args: [], cwd: '/', raw: false }); assert.ok(!result.ok); - assert.equal(result.errorKind, ERROR_KINDS.UnknownCommand); + assert.equal(result.kind, ERROR_KINDS.UnknownCommand); }); test('missing subcommand in cjsRegistry returns UnknownCommand', () => { @@ -246,21 +253,22 @@ describe('CommandRoutingHub — errorKind: UnknownCommand', () => { const result = hub.dispatch({ family: 'phase', subcommand: 'not-there', args: [], cwd: '/', raw: false }); assert.ok(!result.ok); - assert.equal(result.errorKind, ERROR_KINDS.UnknownCommand); + assert.equal(result.kind, ERROR_KINDS.UnknownCommand); }); }); -// ─── errorKind: InvalidArgs ─────────────────────────────────────────────────── +// ─── kind: InvalidArgs ──────────────────────────────────────────────────────── -describe('CommandRoutingHub — errorKind: InvalidArgs', () => { +describe('CommandRoutingHub — kind: InvalidArgs', () => { test('handler returning InvalidArgs result propagates it', () => { const hub = createHub({ cjsRegistry: { phase: { insert: (_ctx) => ({ ok: false, - errorKind: ERROR_KINDS.InvalidArgs, - message: 'phase insert requires a phase number', + kind: ERROR_KINDS.InvalidArgs, + arg: 'phase-number', + reason: 'phase insert requires a phase number', }), }, }, @@ -269,22 +277,22 @@ describe('CommandRoutingHub — errorKind: InvalidArgs', () => { const result = hub.dispatch({ family: 'phase', subcommand: 'insert', args: [], cwd: '/', raw: false }); assert.ok(!result.ok); - assert.equal(result.errorKind, ERROR_KINDS.InvalidArgs); - assert.ok(result.message.includes('phase number')); + assert.equal(result.kind, ERROR_KINDS.InvalidArgs); + assert.ok(result.reason.includes('phase number')); }); }); -// ─── errorKind: HandlerRefusal ──────────────────────────────────────────────── +// ─── kind: HandlerRefusal ───────────────────────────────────────────────────── -describe('CommandRoutingHub — errorKind: HandlerRefusal', () => { +describe('CommandRoutingHub — kind: HandlerRefusal', () => { test('handler returning HandlerRefusal result propagates it', () => { const hub = createHub({ cjsRegistry: { phase: { 'list-plans': (_ctx) => ({ ok: false, - errorKind: ERROR_KINDS.HandlerRefusal, - message: 'phase list-plans is SDK-only', + kind: ERROR_KINDS.HandlerRefusal, + reason: 'phase list-plans is SDK-only', }), }, }, @@ -293,13 +301,13 @@ describe('CommandRoutingHub — errorKind: HandlerRefusal', () => { const result = hub.dispatch({ family: 'phase', subcommand: 'list-plans', args: [], cwd: '/', raw: false }); assert.ok(!result.ok); - assert.equal(result.errorKind, ERROR_KINDS.HandlerRefusal); + assert.equal(result.kind, ERROR_KINDS.HandlerRefusal); }); }); -// ─── errorKind: HandlerFailure ──────────────────────────────────────────────── +// ─── kind: HandlerFailure ───────────────────────────────────────────────────── -describe('CommandRoutingHub — errorKind: HandlerFailure', () => { +describe('CommandRoutingHub — kind: HandlerFailure', () => { test('hub does not throw when CJS handler throws — returns HandlerFailure', () => { const hub = createHub({ cjsRegistry: { @@ -315,11 +323,11 @@ describe('CommandRoutingHub — errorKind: HandlerFailure', () => { }); assert.ok(!result.ok); - assert.equal(result.errorKind, ERROR_KINDS.HandlerFailure); + assert.equal(result.kind, ERROR_KINDS.HandlerFailure); assert.ok(result.message.includes('handler blew up')); }); - test('HandlerFailure details.originalError carries the thrown error', () => { + test('HandlerFailure cause carries the thrown error', () => { const originalError = new Error('boom'); const hub = createHub({ cjsRegistry: { @@ -332,8 +340,8 @@ describe('CommandRoutingHub — errorKind: HandlerFailure', () => { const result = hub.dispatch({ family: 'state', subcommand: 'load', args: [], cwd: '/', raw: false }); assert.ok(!result.ok); - assert.equal(result.errorKind, ERROR_KINDS.HandlerFailure); - assert.strictEqual(result.details.originalError, originalError); + assert.equal(result.kind, ERROR_KINDS.HandlerFailure); + assert.strictEqual(result.cause, originalError); }); }); @@ -349,7 +357,7 @@ describe('CommandRoutingHub — hub never throws', () => { }); assert.ok(!result.ok); - assert.equal(result.errorKind, ERROR_KINDS.UnknownCommand); + assert.equal(result.kind, ERROR_KINDS.UnknownCommand); }); test('hub does not throw when dispatch receives malformed request', () => { @@ -366,6 +374,158 @@ describe('CommandRoutingHub — hub never throws', () => { }); }); +// ─── P1.2: Typed-payload discriminated union (#176) ────────────────────────── +// Each error variant carries ONLY its own typed payload. +// `errorKind` field renamed to `kind`; generic `message`/`details` removed +// from variants that have dedicated fields. + +describe('CommandRoutingHub — P1.2 typed-payload discriminated union (#176)', () => { + // ── UnknownCommand: { ok, kind, command } — no message, no details ────────── + test('UnknownCommand has exactly { ok, kind, command } — nothing else', () => { + const hub = createHub({ + cjsRegistry: {}, + manifest: { phase: ['add'] }, + }); + + const result = hub.dispatch({ family: 'bogus', subcommand: 'add', args: [], cwd: '/', raw: false }); + + assert.ok(!result.ok); + assert.equal(result.kind, ERROR_KINDS.UnknownCommand); + assert.equal(typeof result.command, 'string'); + assert.ok(result.command.length > 0, 'command field must be non-empty'); + // Strict field set — no errorKind, no message, no details + const keys = Object.keys(result).sort(); + assert.deepStrictEqual(keys, ['command', 'kind', 'ok']); + }); + + test('UnknownCommand for unknown subcommand carries the command string', () => { + const hub = createHub({ + cjsRegistry: {}, + manifest: { phase: ['add'] }, + }); + + const result = hub.dispatch({ family: 'phase', subcommand: 'nonexistent', args: [], cwd: '/', raw: false }); + + assert.ok(!result.ok); + assert.equal(result.kind, ERROR_KINDS.UnknownCommand); + assert.ok(result.command.includes('nonexistent'), `Expected command to include 'nonexistent', got: ${result.command}`); + }); + + test('UnknownCommand from missing cjsRegistry family carries the command string', () => { + const hub = createHub({ + cjsRegistry: { state: { load: () => ({ ok: true, data: null }) } }, + }); + + const result = hub.dispatch({ family: 'bogus-family', subcommand: 'sub', args: [], cwd: '/', raw: false }); + + assert.ok(!result.ok); + assert.equal(result.kind, ERROR_KINDS.UnknownCommand); + assert.ok(result.command.includes('bogus-family'), `Expected command to include 'bogus-family', got: ${result.command}`); + }); + + // ── InvalidArgs: { ok, kind, arg, reason } — no message, no details ───────── + test('InvalidArgs result from handler is propagated with kind/arg/reason fields', () => { + const hub = createHub({ + cjsRegistry: { + phase: { + insert: (_ctx) => ({ + ok: false, + kind: ERROR_KINDS.InvalidArgs, + arg: '--dry-run', + reason: 'phase insert does not support --dry-run', + }), + }, + }, + }); + + const result = hub.dispatch({ family: 'phase', subcommand: 'insert', args: [], cwd: '/', raw: false }); + + assert.ok(!result.ok); + assert.equal(result.kind, ERROR_KINDS.InvalidArgs); + assert.equal(result.arg, '--dry-run'); + assert.ok(result.reason.includes('--dry-run')); + // Strict field set + const keys = Object.keys(result).sort(); + assert.deepStrictEqual(keys, ['arg', 'kind', 'ok', 'reason']); + }); + + // ── HandlerRefusal: { ok, kind, reason } — no message, no details ──────────── + test('HandlerRefusal result from handler is propagated with kind/reason fields', () => { + const hub = createHub({ + cjsRegistry: { + phase: { + 'list-plans': (_ctx) => ({ + ok: false, + kind: ERROR_KINDS.HandlerRefusal, + reason: 'phase list-plans is SDK-only', + }), + }, + }, + }); + + const result = hub.dispatch({ family: 'phase', subcommand: 'list-plans', args: [], cwd: '/', raw: false }); + + assert.ok(!result.ok); + assert.equal(result.kind, ERROR_KINDS.HandlerRefusal); + assert.ok(result.reason.includes('SDK-only')); + // Strict field set + const keys = Object.keys(result).sort(); + assert.deepStrictEqual(keys, ['kind', 'ok', 'reason']); + }); + + // ── HandlerFailure: { ok, kind, message, cause? } — cause carries the Error ── + test('HandlerFailure from throw has { ok, kind, message, cause } — no details', () => { + const hub = createHub({ + cjsRegistry: { + phase: { + add: (_ctx) => { throw new Error('handler blew up'); }, + }, + }, + }); + + const result = hub.dispatch({ family: 'phase', subcommand: 'add', args: ['desc'], cwd: '/', raw: false }); + + assert.ok(!result.ok); + assert.equal(result.kind, ERROR_KINDS.HandlerFailure); + assert.ok(result.message.includes('handler blew up')); + assert.ok(result.cause instanceof Error); + // Strict field set (cause present when Error thrown) + const keys = Object.keys(result).sort(); + assert.deepStrictEqual(keys, ['cause', 'kind', 'message', 'ok']); + }); + + test('HandlerFailure cause carries the original thrown Error object', () => { + const originalError = new Error('boom'); + const hub = createHub({ + cjsRegistry: { + state: { + load: (_ctx) => { throw originalError; }, + }, + }, + }); + + const result = hub.dispatch({ family: 'state', subcommand: 'load', args: [], cwd: '/', raw: false }); + + assert.ok(!result.ok); + assert.equal(result.kind, ERROR_KINDS.HandlerFailure); + assert.strictEqual(result.cause, originalError); + }); + + // ── ERROR_KINDS values used as `kind` discriminator — still work ───────────── + test('ERROR_KINDS.UnknownCommand === result.kind for UnknownCommand', () => { + const hub = createHub({ cjsRegistry: {} }); + const result = hub.dispatch({ family: 'nope', subcommand: 'x', args: [], cwd: '/', raw: false }); + assert.equal(result.kind, ERROR_KINDS.UnknownCommand); + }); + + test('ERROR_KINDS values are stable string constants matching their key names', () => { + assert.equal(ERROR_KINDS.UnknownCommand, 'UnknownCommand'); + assert.equal(ERROR_KINDS.InvalidArgs, 'InvalidArgs'); + assert.equal(ERROR_KINDS.HandlerRefusal, 'HandlerRefusal'); + assert.equal(ERROR_KINDS.HandlerFailure, 'HandlerFailure'); + }); +}); + // ─── No SDK path — single-dispatch invariant ────────────────────────────────── // #175: Hub is always CJS. There is no SDK path to fall through to. @@ -402,6 +562,287 @@ describe('CommandRoutingHub — single CJS dispatch invariant (#175)', () => { assert.ok(known.ok); assert.ok(!unknown.ok); - assert.equal(unknown.errorKind, ERROR_KINDS.UnknownCommand); + assert.equal(unknown.kind, ERROR_KINDS.UnknownCommand); + }); +}); + +// ─── P1.2 Review Finding 1: Hub runtime-validates ok:false handler returns ──── +// A handler that returns { ok: false, kind: 'InvalidArgs', message: 'oops' } +// (missing `reason`, has stray `message`) must NOT pass through unchanged. +// Hub must coerce it to a HandlerFailure with a contract-violation message. + +describe('CommandRoutingHub — Finding 1: runtime-validation of handler ok:false returns', () => { + test('malformed InvalidArgs return (missing reason, has stray message) is coerced to HandlerFailure', () => { + const hub = createHub({ + cjsRegistry: { + phase: { + add: (_ctx) => ({ + ok: false, + kind: 'InvalidArgs', + message: 'oops', // wrong: should be reason, not message + // missing: arg, reason + }), + }, + }, + }); + + const result = hub.dispatch({ family: 'phase', subcommand: 'add', args: [], cwd: '/', raw: false }); + + assert.ok(!result.ok, 'result must be an error'); + assert.equal(result.kind, ERROR_KINDS.HandlerFailure, + `Expected HandlerFailure but got kind: ${result.kind}`); + assert.ok( + result.message.includes('malformed') || result.message.includes('contract') || + result.message.includes('InvalidArgs') || result.message.includes('reason'), + `Expected contract-violation message, got: ${result.message}` + ); + }); + + test('malformed HandlerRefusal return (missing reason) is coerced to HandlerFailure', () => { + const hub = createHub({ + cjsRegistry: { + phase: { + add: (_ctx) => ({ + ok: false, + kind: 'HandlerRefusal', + message: 'refuse', // wrong: should be reason + // missing: reason + }), + }, + }, + }); + + const result = hub.dispatch({ family: 'phase', subcommand: 'add', args: [], cwd: '/', raw: false }); + + assert.ok(!result.ok); + assert.equal(result.kind, ERROR_KINDS.HandlerFailure); + assert.ok(typeof result.message === 'string' && result.message.length > 0); + }); + + test('malformed HandlerFailure return (missing message) is coerced to HandlerFailure', () => { + const hub = createHub({ + cjsRegistry: { + phase: { + add: (_ctx) => ({ + ok: false, + kind: 'HandlerFailure', + // missing: message + details: 'something', // extraneous + }), + }, + }, + }); + + const result = hub.dispatch({ family: 'phase', subcommand: 'add', args: [], cwd: '/', raw: false }); + + assert.ok(!result.ok); + assert.equal(result.kind, ERROR_KINDS.HandlerFailure); + assert.ok(typeof result.message === 'string' && result.message.length > 0); + }); + + test('well-formed InvalidArgs return is NOT coerced — passes through unchanged', () => { + const hub = createHub({ + cjsRegistry: { + phase: { + add: (_ctx) => ({ + ok: false, + kind: 'InvalidArgs', + arg: '--dry-run', + reason: 'not supported', + }), + }, + }, + }); + + const result = hub.dispatch({ family: 'phase', subcommand: 'add', args: [], cwd: '/', raw: false }); + + assert.ok(!result.ok); + assert.equal(result.kind, ERROR_KINDS.InvalidArgs); + assert.equal(result.arg, '--dry-run'); + assert.equal(result.reason, 'not supported'); + }); + + test('unknown kind in ok:false return is coerced to HandlerFailure', () => { + const hub = createHub({ + cjsRegistry: { + phase: { + add: (_ctx) => ({ + ok: false, + kind: 'SomeLegacyKind', + errorKind: 'SomeLegacyKind', + }), + }, + }, + }); + + const result = hub.dispatch({ family: 'phase', subcommand: 'add', args: [], cwd: '/', raw: false }); + + assert.ok(!result.ok); + assert.equal(result.kind, ERROR_KINDS.HandlerFailure); + }); +}); + +// ─── P1.2 Review Finding 2: Non-Error throws preserve the original throwable ── +// When a handler throws a non-Error (plain object, string, number), the Hub must +// wrap it in an Error and attach .thrown = originalValue. + +describe('CommandRoutingHub — Finding 2: non-Error throws preserve original throwable', () => { + test('handler throwing a plain object → HandlerFailure with cause.thrown === original', () => { + const thrown = { custom: 'payload', code: 42 }; + const hub = createHub({ + cjsRegistry: { + phase: { + add: (_ctx) => { throw thrown; }, + }, + }, + }); + + const result = hub.dispatch({ family: 'phase', subcommand: 'add', args: [], cwd: '/', raw: false }); + + assert.ok(!result.ok); + assert.equal(result.kind, ERROR_KINDS.HandlerFailure); + assert.ok(result.cause instanceof Error, + `result.cause must be an Error, got: ${typeof result.cause}`); + assert.strictEqual(result.cause.thrown, thrown, + 'cause.thrown must be the original thrown object'); + }); + + test('handler throwing a string → HandlerFailure with cause.thrown === original string', () => { + const hub = createHub({ + cjsRegistry: { + phase: { + add: (_ctx) => { throw 'just a string'; }, + }, + }, + }); + + const result = hub.dispatch({ family: 'phase', subcommand: 'add', args: [], cwd: '/', raw: false }); + + assert.ok(!result.ok); + assert.equal(result.kind, ERROR_KINDS.HandlerFailure); + assert.ok(result.cause instanceof Error, + `result.cause must be an Error, got: ${typeof result.cause}`); + assert.strictEqual(result.cause.thrown, 'just a string', + 'cause.thrown must be the original thrown string'); + }); + + test('handler throwing a number → HandlerFailure with cause.thrown === original number', () => { + const hub = createHub({ + cjsRegistry: { + phase: { + add: (_ctx) => { throw 404; }, + }, + }, + }); + + const result = hub.dispatch({ family: 'phase', subcommand: 'add', args: [], cwd: '/', raw: false }); + + assert.ok(!result.ok); + assert.equal(result.kind, ERROR_KINDS.HandlerFailure); + assert.ok(result.cause instanceof Error); + assert.strictEqual(result.cause.thrown, 404); + }); + + test('handler throwing a real Error still works — cause is the Error itself (no .thrown wrapping)', () => { + const original = new Error('real error'); + const hub = createHub({ + cjsRegistry: { + phase: { + add: (_ctx) => { throw original; }, + }, + }, + }); + + const result = hub.dispatch({ family: 'phase', subcommand: 'add', args: [], cwd: '/', raw: false }); + + assert.ok(!result.ok); + assert.equal(result.kind, ERROR_KINDS.HandlerFailure); + assert.strictEqual(result.cause, original, 'Error throws must have cause === original Error'); + // No .thrown on real Error cause + assert.equal(result.cause.thrown, undefined); + }); +}); + +// ─── P1.2 Review Finding 3: Factory returns are Object.frozen ───────────────── +// Each makeXxx factory must return a frozen object so callers cannot mutate +// the variant invariant. + +describe('CommandRoutingHub — Finding 3: factory returns are Object.frozen', () => { + test('makeUnknownCommand returns a frozen object', () => { + const result = makeUnknownCommand('phase bogus'); + assert.ok(Object.isFrozen(result), + 'makeUnknownCommand must return a frozen object'); + }); + + test('makeInvalidArgs returns a frozen object', () => { + const result = makeInvalidArgs('--dry-run', 'not supported'); + assert.ok(Object.isFrozen(result), + 'makeInvalidArgs must return a frozen object'); + }); + + test('makeHandlerRefusal returns a frozen object', () => { + const result = makeHandlerRefusal('SDK-only'); + assert.ok(Object.isFrozen(result), + 'makeHandlerRefusal must return a frozen object'); + }); + + test('makeHandlerFailure returns a frozen object', () => { + const result = makeHandlerFailure('something broke', new Error('orig')); + assert.ok(Object.isFrozen(result), + 'makeHandlerFailure must return a frozen object'); + }); + + test('frozen factory results cannot be mutated', () => { + const result = makeUnknownCommand('phase bogus'); + // In strict mode, mutation of a frozen object throws TypeError + assert.throws( + () => { result.command = 'tampered'; }, + TypeError, + 'Mutating a frozen factory result must throw TypeError' + ); + }); +}); + +// ─── P1.2 Review Finding 4: makeHandlerFailure wraps non-Error causes ───────── +// If cause is provided but is not an Error, wrap it so .cause instanceof Error. +// Attach .thrown = originalCause so it is not silently dropped. + +describe('CommandRoutingHub — Finding 4: makeHandlerFailure wraps non-Error causes', () => { + test('makeHandlerFailure("msg", "string-cause") → cause instanceof Error', () => { + const result = makeHandlerFailure('msg', 'string-cause'); + assert.ok(result.cause instanceof Error, + `cause must be an Error, got: ${typeof result.cause}`); + }); + + test('makeHandlerFailure("msg", "string-cause") → cause.thrown === "string-cause"', () => { + const result = makeHandlerFailure('msg', 'string-cause'); + assert.strictEqual(result.cause.thrown, 'string-cause', + 'cause.thrown must be the original non-Error cause'); + }); + + test('makeHandlerFailure with a plain object cause → cause instanceof Error with .thrown', () => { + const obj = { code: 42, detail: 'bad' }; + const result = makeHandlerFailure('msg', obj); + assert.ok(result.cause instanceof Error); + assert.strictEqual(result.cause.thrown, obj); + }); + + test('makeHandlerFailure with a real Error cause → cause is the original Error (no wrapping)', () => { + const original = new Error('real'); + const result = makeHandlerFailure('msg', original); + assert.strictEqual(result.cause, original, + 'Real Error causes must not be wrapped'); + }); + + test('makeHandlerFailure without cause → result.cause is undefined', () => { + const result = makeHandlerFailure('msg'); + assert.equal(result.cause, undefined); + }); + + test('makeHandlerFailure with null cause → behaves as no cause (undefined)', () => { + // null is not an Error, but "not provided" — treat as absent + const result = makeHandlerFailure('msg', null); + // null should not be wrapped into an Error — it's equivalent to "no cause" + assert.equal(result.cause, undefined); }); });