* refactor(hub): tighten Result<T> to typed-payload-per-kind discriminated union (#176) Each Hub error variant now carries only its own typed payload. The generic `errorKind` field is renamed to `kind`; `message`/`details` escape hatches are removed from Hub-emitted errors. Factory functions (makeUnknownCommand, makeInvalidArgs, makeHandlerRefusal, makeHandlerFailure) are exported and used in phase-command-router.cjs. Callers switch on `result.kind`. Part of ADR-0174 P1.2. <!-- docs-exempt: no docs/ changes; API is internal to Hub callers --> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(hub): act on P1.2 review findings (#176) Addresses 4 review findings on PR #221: - Hub now runtime-validates ok:false variants against the typed shape and coerces malformed returns to HandlerFailure with a contract- violation message (codex finding #1, code-review finding #1) - catch path now preserves the original throwable for non-Error throws via an Error wrapper with .thrown attached (codex finding #2) - All 4 factory returns are Object.freeze'd (review finding #9) - makeHandlerFailure validates cause is Error; non-Error causes are wrapped with .thrown attached (review finding #10) Tests added for each finding (TDD red → green). Refs #176. Part of #174. * fix(docs-lint): add docs-exempt markers to both P1.2 changeset fragments Both `176-typed-result-discriminated-union.md` and `176-hub-p1.2-review-findings.md` carry `type: Changed` which triggers the docs-required lint. Neither fragment had a `<!-- docs-exempt: <reason> -->` marker, causing `docs-lint` to fail with `FAIL_DOCS_MISSING`. Added the per-fragment exemption marker to both (the repo has no `no-docs` label). This is a purely internal SDK refactor (ADR-0174 P1.2) with no public docs surface. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
@@ -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<string, { required: string[], allowed: Set<string> }>}
|
||||
*/
|
||||
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 || '<no kind>') + ', got ' + _safeJson(result))
|
||||
);
|
||||
}
|
||||
}
|
||||
return result;
|
||||
}
|
||||
|
||||
@@ -160,4 +300,8 @@ function createHub({ cjsRegistry, manifest } = {}) {
|
||||
module.exports = {
|
||||
createHub,
|
||||
ERROR_KINDS,
|
||||
makeUnknownCommand,
|
||||
makeInvalidArgs,
|
||||
makeHandlerRefusal,
|
||||
makeHandlerFailure,
|
||||
};
|
||||
|
||||
@@ -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-number>', '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;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user