From f104dab33203feb180768c0a9309e177c9ef7598 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sun, 3 May 2026 14:30:27 -0400 Subject: [PATCH] refactor(query): deepen dispatch policy seam with structured result contract (#3066) * refactor(query): deepen dispatch policy seam with structured result contract Closes #3065. - unify query dispatch outcome as typed success/failure union - include error kind/details + final exit_code in failure path - align native and fallback paths under one dispatch policy seam - make CLI query path consume seam result (thin adapter) - add ADR + context term for Dispatch Policy Module * refactor(query): strengthen dispatch seam with shared error mapper and typed details - add query-dispatch-error-mapper module shared by native/fallback paths - remove ad-hoc inline mapping in dispatch/fallback executors - lock error-details schema in mapper + dispatch tests - document structured dispatch contract in QUERY-HANDLERS.md * fix(query): return structured fallback failure when path resolution throws - guard resolveGsdToolsPath in cjs dispatch path - map thrown resolution errors to fallback_failure result - add regression test for structured failure contract --- .changeset/steady-ravens-shape.md | 6 ++ CONTEXT.md | 14 ++++ docs/adr/0001-dispatch-policy-module.md | 3 + sdk/src/cli.ts | 4 +- sdk/src/query/QUERY-HANDLERS.md | 19 ++++- sdk/src/query/query-dispatch-contract.ts | 26 +++++- .../query/query-dispatch-error-mapper.test.ts | 42 ++++++++++ sdk/src/query/query-dispatch-error-mapper.ts | 51 ++++++++++++ sdk/src/query/query-dispatch.test.ts | 80 +++++++++++++++++-- sdk/src/query/query-dispatch.ts | 78 +++++++++++------- sdk/src/query/query-fallback-executor.test.ts | 14 +++- sdk/src/query/query-fallback-executor.ts | 11 ++- 12 files changed, 297 insertions(+), 51 deletions(-) create mode 100644 .changeset/steady-ravens-shape.md create mode 100644 CONTEXT.md create mode 100644 docs/adr/0001-dispatch-policy-module.md create mode 100644 sdk/src/query/query-dispatch-error-mapper.test.ts create mode 100644 sdk/src/query/query-dispatch-error-mapper.ts diff --git a/.changeset/steady-ravens-shape.md b/.changeset/steady-ravens-shape.md new file mode 100644 index 000000000..2a88ec7e3 --- /dev/null +++ b/.changeset/steady-ravens-shape.md @@ -0,0 +1,6 @@ +--- +type: Changed +pr: 3065 +--- + +**Dispatch policy seam now returns a structured result contract** across native and fallback query execution paths (`ok`, typed error `kind`, `details`, and final `exit_code`), with CLI consuming the unified result instead of mixed throw/result handling. \ No newline at end of file diff --git a/CONTEXT.md b/CONTEXT.md new file mode 100644 index 000000000..fadc675d3 --- /dev/null +++ b/CONTEXT.md @@ -0,0 +1,14 @@ +# Context + +## Domain terms + +### Dispatch Policy Module +Module owning dispatch error mapping, fallback policy, timeout classification, and CLI exit mapping contract. + +Canonical error kind set: +- `unknown_command` +- `native_failure` +- `native_timeout` +- `fallback_failure` +- `validation_error` +- `internal_error` diff --git a/docs/adr/0001-dispatch-policy-module.md b/docs/adr/0001-dispatch-policy-module.md new file mode 100644 index 000000000..93cca43a6 --- /dev/null +++ b/docs/adr/0001-dispatch-policy-module.md @@ -0,0 +1,3 @@ +# Dispatch policy module as single seam for query execution outcomes + +We decided to centralize query dispatch outcomes in one Dispatch Policy Module that returns a structured union result (`ok` success or failure with typed `kind`, `details`, and final `exit_code`) instead of mixing throws and ad-hoc error mapping across CLI and SDK paths. This keeps fallback policy, timeout classification, and exit mapping in one place for better locality, prevents drift between native and fallback behavior, and makes callers thin adapters over a stable interface. diff --git a/sdk/src/cli.ts b/sdk/src/cli.ts index ead7e6a08..74ff07071 100644 --- a/sdk/src/cli.ts +++ b/sdk/src/cli.ts @@ -361,9 +361,9 @@ export async function main(argv: string[] = process.argv.slice(2)): Promise; } -export interface QueryDispatchResult { - stdout?: string; +export interface QueryDispatchSuccessResult { + ok: true; + stdout: string; stderr: string[]; - error?: QueryDispatchError; + exit_code: 0; } + +export interface QueryDispatchFailureResult { + ok: false; + error: QueryDispatchError; + stderr: string[]; + exit_code: number; +} + +export type QueryDispatchResult = QueryDispatchSuccessResult | QueryDispatchFailureResult; diff --git a/sdk/src/query/query-dispatch-error-mapper.test.ts b/sdk/src/query/query-dispatch-error-mapper.test.ts new file mode 100644 index 000000000..01c4c2e45 --- /dev/null +++ b/sdk/src/query/query-dispatch-error-mapper.test.ts @@ -0,0 +1,42 @@ +import { describe, it, expect } from 'vitest'; +import { + mapNativeDispatchError, + mapFallbackDispatchError, + toDispatchFailure, +} from './query-dispatch-error-mapper.js'; + +describe('query dispatch error mapper', () => { + it('maps native timeout errors', () => { + const err = mapNativeDispatchError( + new Error('gsd-tools timed out after 30000ms: state load'), + 'state.load', + [], + ); + expect(err.kind).toBe('native_timeout'); + expect(err.code).toBe(1); + expect(err.details).toMatchObject({ command: 'state.load', args: [], timeout_ms: 30000 }); + }); + + it('maps native non-timeout errors', () => { + const err = mapNativeDispatchError(new Error('boom'), 'state.json', []); + expect(err.kind).toBe('native_failure'); + expect(err.code).toBe(1); + expect(err.details).toMatchObject({ command: 'state.json', args: [] }); + }); + + it('maps fallback errors', () => { + const err = mapFallbackDispatchError(new Error('spawn ENOENT'), 'state', ['load']); + expect(err.kind).toBe('fallback_failure'); + expect(err.code).toBe(1); + expect(err.details).toMatchObject({ command: 'state', args: ['load'], backend: 'cjs' }); + }); + + it('builds failure result union', () => { + const out = toDispatchFailure({ kind: 'internal_error', code: 1, message: 'Error: x' }, ['warn']); + expect(out.ok).toBe(false); + if (out.ok) throw new Error('expected failure'); + expect(out.exit_code).toBe(1); + expect(out.stderr).toEqual(['warn']); + expect(out.error.kind).toBe('internal_error'); + }); +}); diff --git a/sdk/src/query/query-dispatch-error-mapper.ts b/sdk/src/query/query-dispatch-error-mapper.ts new file mode 100644 index 000000000..cdb28e58d --- /dev/null +++ b/sdk/src/query/query-dispatch-error-mapper.ts @@ -0,0 +1,51 @@ +import type { QueryDispatchError, QueryDispatchErrorKind, QueryDispatchResult } from './query-dispatch-contract.js'; + +export function toDispatchFailure( + error: QueryDispatchError, + stderr: string[] = [], +): QueryDispatchResult { + return { + ok: false, + stderr, + exit_code: error.code, + error, + }; +} + +export function mapNativeDispatchError(error: unknown, command: string, args: string[]): QueryDispatchError { + const message = error instanceof Error ? error.message : String(error); + const kind: QueryDispatchErrorKind = message.includes('timed out after') + ? 'native_timeout' + : 'native_failure'; + return { + kind, + code: 1, + message: `Error: ${message}`, + details: { + command, + args, + ...(kind === 'native_timeout' ? { timeout_ms: parseTimeoutMs(message) } : {}), + }, + }; +} + +export function mapFallbackDispatchError(error: unknown, command: string, args: string[]): QueryDispatchError { + const message = error instanceof Error ? error.message : String(error); + return { + kind: 'fallback_failure', + code: 1, + message: `Error: gsd-tools.cjs fallback failed: ${message}`, + details: { + command, + args, + backend: 'cjs', + }, + }; +} + +function parseTimeoutMs(message: string): number | undefined { + const m = message.match(/timed out after\s+(\d+)ms/i); + if (!m) return undefined; + const n = Number.parseInt(m[1], 10); + return Number.isFinite(n) ? n : undefined; +} diff --git a/sdk/src/query/query-dispatch.test.ts b/sdk/src/query/query-dispatch.test.ts index 6cbff02ef..0e3a871c9 100644 --- a/sdk/src/query/query-dispatch.test.ts +++ b/sdk/src/query/query-dispatch.test.ts @@ -35,8 +35,10 @@ describe('runQueryDispatch', () => { dispatchNative: async () => ({ data: { ok: true } }), }, ['state', 'json']); - expect(out.error).toBeUndefined(); + expect(out.ok).toBe(true); + if (!out.ok) throw new Error('expected success'); expect(out.stdout).toBe('{\n "ok": true\n}\n'); + expect(out.exit_code).toBe(0); }); it('applies --pick to native json output', async () => { @@ -49,8 +51,10 @@ describe('runQueryDispatch', () => { dispatchNative: async () => ({ data: { nested: { value: 7 } } }), }, ['state', 'json', '--pick', 'nested.value']); - expect(out.error).toBeUndefined(); + expect(out.ok).toBe(true); + if (!out.ok) throw new Error('expected success'); expect(out.stdout).toBe('7\n'); + expect(out.exit_code).toBe(0); }); it('returns structured error for unknown command when fallback disabled', async () => { @@ -63,9 +67,12 @@ describe('runQueryDispatch', () => { dispatchNative: async () => ({ data: {} }), }, ['unknown-cmd']); - expect(out.error?.code).toBe(10); - expect(out.error?.message).toContain('Unknown command: "unknown-cmd"'); - expect(out.error?.message).toContain('Attempted dotted:'); + expect(out.ok).toBe(false); + if (out.ok) throw new Error('expected failure'); + expect(out.error.code).toBe(10); + expect(out.error.kind).toBe('unknown_command'); + expect(out.error.message).toContain('Unknown command: "unknown-cmd"'); + expect(out.error.message).toContain('Attempted dotted:'); }); it('runs cjs fallback and formats text mode', async () => { @@ -79,11 +86,30 @@ describe('runQueryDispatch', () => { dispatchNative: async () => ({ data: {} }), }, ['unknown-cmd', '--help']); - expect(out.error).toBeUndefined(); + expect(out.ok).toBe(true); + if (!out.ok) throw new Error('expected success'); expect(out.stdout).toBe('USAGE: help text\n'); expect(out.stderr[0]).toContain('falling back to gsd-tools.cjs'); }); + it('returns structured fallback failure when resolveGsdToolsPath throws', async () => { + const registry = createRegistry(); + const out = await runQueryDispatch({ + registry, + projectDir: tmpDir, + cjsFallbackEnabled: true, + resolveGsdToolsPath: () => { throw new Error('path boom'); }, + dispatchNative: async () => ({ data: {} }), + }, ['unknown-cmd']); + + expect(out.ok).toBe(false); + if (out.ok) throw new Error('expected failure'); + expect(out.error.kind).toBe('fallback_failure'); + expect(out.error.code).toBe(1); + expect(out.error.message).toContain('path boom'); + expect(out.error.details).toMatchObject({ command: 'unknown-cmd', backend: 'cjs' }); + }); + it('returns requires-command error for empty argv', async () => { const registry = createRegistry(); const out = await runQueryDispatch({ @@ -93,7 +119,45 @@ describe('runQueryDispatch', () => { resolveGsdToolsPath: () => '', dispatchNative: async () => ({ data: {} }), }, []); - expect(out.error?.code).toBe(10); - expect(out.error?.message).toContain('requires a command'); + expect(out.ok).toBe(false); + if (out.ok) throw new Error('expected failure'); + expect(out.error.code).toBe(10); + expect(out.error.kind).toBe('validation_error'); + expect(out.error.message).toContain('requires a command'); + expect(out.error.details).toEqual({ reason: 'missing_command' }); + }); + + it('maps native timeout to native_timeout kind with details', async () => { + const registry = createRegistry(); + const out = await runQueryDispatch({ + registry, + projectDir: tmpDir, + cjsFallbackEnabled: true, + resolveGsdToolsPath: () => '', + dispatchNative: async () => { throw new Error('gsd-tools timed out after 30000ms: state load'); }, + }, ['state', 'load']); + + expect(out.ok).toBe(false); + if (out.ok) throw new Error('expected failure'); + expect(out.error.kind).toBe('native_timeout'); + expect(out.error.code).toBe(1); + expect(out.error.details).toMatchObject({ command: 'state.load', args: [], timeout_ms: 30000 }); + }); + + it('maps native error to native_failure kind with details', async () => { + const registry = createRegistry(); + const out = await runQueryDispatch({ + registry, + projectDir: tmpDir, + cjsFallbackEnabled: true, + resolveGsdToolsPath: () => '', + dispatchNative: async () => { throw new Error('boom'); }, + }, ['state', 'json']); + + expect(out.ok).toBe(false); + if (out.ok) throw new Error('expected failure'); + expect(out.error.kind).toBe('native_failure'); + expect(out.error.code).toBe(1); + expect(out.error.details).toMatchObject({ command: 'state.json', args: [] }); }); }); diff --git a/sdk/src/query/query-dispatch.ts b/sdk/src/query/query-dispatch.ts index f7ca7f74b..319a010cb 100644 --- a/sdk/src/query/query-dispatch.ts +++ b/sdk/src/query/query-dispatch.ts @@ -4,7 +4,8 @@ import { normalizeQueryCommand } from './normalize-query-command.js'; import { explainQueryCommandNoMatch, resolveQueryCommand, type QueryCommandResolution } from './command-resolution.js'; import { runCjsFallbackDispatch } from './query-fallback-executor.js'; import type { QueryResult } from './utils.js'; -import type { QueryDispatchError, QueryDispatchResult } from './query-dispatch-contract.js'; +import type { QueryDispatchResult, QueryDispatchErrorKind } from './query-dispatch-contract.js'; +import { mapNativeDispatchError, toDispatchFailure } from './query-dispatch-error-mapper.js'; export interface QueryDispatchDeps { registry: QueryRegistry; @@ -23,6 +24,20 @@ interface DispatchPlan { matched: QueryCommandResolution | null; } +function fail( + kind: QueryDispatchErrorKind, + code: number, + message: string, + details?: Record, + stderr: string[] = [], +): QueryDispatchResult { + return toDispatchFailure({ kind, code, message, details }, stderr); +} + +function success(stdout: string, stderr: string[] = []): QueryDispatchResult { + return { ok: true, stdout, stderr, exit_code: 0 }; +} + function planQueryDispatch(queryArgv: string[], registry: QueryRegistry, cjsFallbackEnabled: boolean): DispatchPlan { const queryCommand = queryArgv[0]; if (!queryCommand) { @@ -41,14 +56,14 @@ function planQueryDispatch(queryArgv: string[], registry: QueryRegistry, cjsFall return { mode: 'error', normalized: { command: normCmd, args: normArgs, tokens: normalizedTokens }, matched: null }; } -function extractPick(queryArgv: string[]): { queryArgs: string[]; pickField?: string; error?: QueryDispatchError } { +function extractPick(queryArgv: string[]): { queryArgs: string[]; pickField?: string; error?: QueryDispatchResult } { const queryArgs = [...queryArgv]; const pickIdx = queryArgs.indexOf('--pick'); if (pickIdx === -1) return { queryArgs }; if (pickIdx + 1 >= queryArgs.length) { return { queryArgs, - error: { code: 10, message: 'Error: --pick requires a field name' }, + error: fail('validation_error', 10, 'Error: --pick requires a field name', { field: '--pick', reason: 'missing_value' }), }; } const pickField = queryArgs[pickIdx + 1]; @@ -68,11 +83,11 @@ function formatOutput(data: unknown, format: QueryResult['format'], pickField?: export async function runQueryDispatch(deps: QueryDispatchDeps, queryArgv: string[]): Promise { const picked = extractPick(queryArgv); - if (picked.error) return { stderr: [], error: picked.error }; + if (picked.error) return picked.error; const { queryArgs, pickField } = picked; if (queryArgs.length === 0 || !queryArgs[0]) { - return { stderr: [], error: { code: 10, message: 'Error: "gsd-sdk query" requires a command' } }; + return fail('validation_error', 10, 'Error: "gsd-sdk query" requires a command', { reason: 'missing_command' }); } const plan = planQueryDispatch(queryArgs, deps.registry, deps.cjsFallbackEnabled); @@ -80,46 +95,47 @@ export async function runQueryDispatch(deps: QueryDispatchDeps, queryArgv: strin const normArgs = plan.normalized.args; if (!normCmd || !String(normCmd).trim()) { - return { stderr: [], error: { code: 10, message: 'Error: "gsd-sdk query" requires a command' } }; + return fail('validation_error', 10, 'Error: "gsd-sdk query" requires a command', { reason: 'empty_normalized_command' }); } if (plan.mode === 'error') { const noMatch = queryArgs[0] ? explainQueryCommandNoMatch(queryArgs[0], queryArgs.slice(1), deps.registry) : null; - return { - stderr: [], - error: { - code: 10, - message: `Error: Unknown command: "${[normCmd, ...normArgs].join(' ')}". Use a registered \`gsd-sdk query\` subcommand (see sdk/src/query/QUERY-HANDLERS.md) or invoke \`node …/gsd-tools.cjs\` for CJS-only operations. CJS fallback is disabled (GSD_QUERY_FALLBACK=registered). To enable fallback, unset GSD_QUERY_FALLBACK or set it to a non-restricted value.${noMatch ? ` Attempted dotted: ${noMatch.attempted.dotted.slice(0, 2).join(' | ')}.` : ''}`, - }, - }; + return fail( + 'unknown_command', + 10, + `Error: Unknown command: "${[normCmd, ...normArgs].join(' ')}". Use a registered \`gsd-sdk query\` subcommand (see sdk/src/query/QUERY-HANDLERS.md) or invoke \`node …/gsd-tools.cjs\` for CJS-only operations. CJS fallback is disabled (GSD_QUERY_FALLBACK=registered). To enable fallback, unset GSD_QUERY_FALLBACK or set it to a non-restricted value.${noMatch ? ` Attempted dotted: ${noMatch.attempted.dotted.slice(0, 2).join(' | ')}.` : ''}`, + { normalized: [normCmd, ...normArgs].join(' '), attempted: noMatch?.attempted.dotted.slice(0, 2) ?? [] }, + ); } if (plan.mode === 'cjs') { - const gsdPath = deps.resolveGsdToolsPath(deps.projectDir); - return runCjsFallbackDispatch({ - projectDir: deps.projectDir, - gsdToolsPath: gsdPath, - normCmd, - normArgs, - ws: deps.ws, - pickField, - }); + try { + const gsdPath = deps.resolveGsdToolsPath(deps.projectDir); + return await runCjsFallbackDispatch({ + projectDir: deps.projectDir, + gsdToolsPath: gsdPath, + normCmd, + normArgs, + ws: deps.ws, + pickField, + }); + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + return fail('fallback_failure', 1, `Error: gsd-tools.cjs fallback failed: ${msg}`, { + command: normCmd, + args: normArgs, + backend: 'cjs', + }); + } } const matched = plan.matched!; try { const result = await deps.dispatchNative(matched.cmd, matched.args); - return { - stderr: [], - stdout: formatOutput(result.data, result.format, pickField), - }; + return success(formatOutput(result.data, result.format, pickField)); } catch (e) { - const msg = e instanceof Error ? e.message : String(e); - return { - stderr: [], - error: { code: 1, message: `Error: ${msg}` }, - }; + return toDispatchFailure(mapNativeDispatchError(e, matched.cmd, matched.args)); } } diff --git a/sdk/src/query/query-fallback-executor.test.ts b/sdk/src/query/query-fallback-executor.test.ts index 3e4fd7caf..6742c2b95 100644 --- a/sdk/src/query/query-fallback-executor.test.ts +++ b/sdk/src/query/query-fallback-executor.test.ts @@ -32,6 +32,8 @@ describe('runCjsFallbackDispatch', () => { normCmd: 'state', normArgs: ['load'], }); + expect(result.ok).toBe(true); + if (!result.ok) throw new Error('expected success'); expect(result.stdout).toBe('{\n "ok": true\n}\n'); }); @@ -43,6 +45,8 @@ describe('runCjsFallbackDispatch', () => { normCmd: 'phase', normArgs: ['add', '--help'], }); + expect(result.ok).toBe(true); + if (!result.ok) throw new Error('expected success'); expect(result.stdout).toBe('USAGE: help text\n'); }); @@ -56,6 +60,8 @@ describe('runCjsFallbackDispatch', () => { ws: 'ws-1', pickField: 'args', }); + expect(result.ok).toBe(true); + if (!result.ok) throw new Error('expected success'); expect(result.stdout).toBe('[\n "state",\n "load",\n "--ws",\n "ws-1"\n]\n'); }); @@ -66,7 +72,11 @@ describe('runCjsFallbackDispatch', () => { normCmd: 'state', normArgs: ['load'], }); - expect(result.error?.code).toBe(1); - expect(result.error?.message).toContain('fallback failed'); + expect(result.ok).toBe(false); + if (result.ok) throw new Error('expected failure'); + expect(result.error.code).toBe(1); + expect(result.error.kind).toBe('fallback_failure'); + expect(result.error.message).toContain('fallback failed'); + expect(result.error.details).toMatchObject({ command: 'state', args: ['load'], backend: 'cjs' }); }); }); diff --git a/sdk/src/query/query-fallback-executor.ts b/sdk/src/query/query-fallback-executor.ts index 449339ec4..879d5960b 100644 --- a/sdk/src/query/query-fallback-executor.ts +++ b/sdk/src/query/query-fallback-executor.ts @@ -2,6 +2,7 @@ import { execFile } from 'node:child_process'; import { readFile } from 'node:fs/promises'; import { extractField } from './registry.js'; import type { QueryDispatchResult } from './query-dispatch-contract.js'; +import { mapFallbackDispatchError, toDispatchFailure } from './query-dispatch-error-mapper.js'; interface CjsFallbackQueryResult { mode: 'json' | 'text'; @@ -99,14 +100,16 @@ export async function runCjsFallbackDispatch(input: RunCjsFallbackDispatchInput) const fallback = await runCjsFallbackQuery(projectDir, gsdToolsPath, normCmd, normArgs, ws); if (fallback.stderr.trim()) stderr.push(fallback.stderr.trimEnd()); return { + ok: true, stderr, - stdout: formatFallbackOutput(fallback.output, fallback.mode, pickField), + stdout: formatFallbackOutput(fallback.output, fallback.mode, pickField) ?? '', + exit_code: 0, }; } catch (err) { const msg = err instanceof Error ? err.message : String(err); - return { + return toDispatchFailure( + mapFallbackDispatchError(msg, normCmd, normArgs), stderr, - error: { code: 1, message: `Error: gsd-tools.cjs fallback failed: ${msg}` }, - }; + ); } }