From 40acf1f02ed9c5448d16f2112281ecbab354a015 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Mon, 4 May 2026 21:47:10 -0400 Subject: [PATCH] fix: address CodeRabbit findings on query/transport error handling --- sdk/src/gsd-tools-error.test.ts | 5 +++++ sdk/src/gsd-tools-error.ts | 4 ++-- sdk/src/query-raw-output-projection.test.ts | 5 +++++ sdk/src/query-raw-output-projection.ts | 17 +++++++++++------ sdk/src/query-subprocess-adapter.test.ts | 11 +++++++++++ sdk/src/query-subprocess-adapter.ts | 6 ++++-- 6 files changed, 38 insertions(+), 10 deletions(-) diff --git a/sdk/src/gsd-tools-error.test.ts b/sdk/src/gsd-tools-error.test.ts index 704de685a..2e1973198 100644 --- a/sdk/src/gsd-tools-error.test.ts +++ b/sdk/src/gsd-tools-error.test.ts @@ -13,4 +13,9 @@ describe('GSDToolsError constructors', () => { expect(err.classification).toEqual({ kind: 'failure' }); expect(err.exitCode).toBe(1); }); + + it('defaults direct constructor to failure classification', () => { + const err = new GSDToolsError('boom', 'state', ['load'], 1, 'stderr'); + expect(err.classification).toEqual({ kind: 'failure' }); + }); }); diff --git a/sdk/src/gsd-tools-error.ts b/sdk/src/gsd-tools-error.ts index 829811830..713476d1b 100644 --- a/sdk/src/gsd-tools-error.ts +++ b/sdk/src/gsd-tools-error.ts @@ -22,7 +22,7 @@ export class GSDToolsError extends Error { ) { super(message, options); this.name = 'GSDToolsError'; - this.classification = options?.classification; + this.classification = options?.classification ?? failureClassification(); } static timeout( @@ -61,5 +61,5 @@ export class GSDToolsError extends Error { ); } - public readonly classification?: GSDToolsErrorClassification; + public readonly classification: GSDToolsErrorClassification; } diff --git a/sdk/src/query-raw-output-projection.test.ts b/sdk/src/query-raw-output-projection.test.ts index 4b7db4e85..811336dec 100644 --- a/sdk/src/query-raw-output-projection.test.ts +++ b/sdk/src/query-raw-output-projection.test.ts @@ -31,4 +31,9 @@ describe('formatQueryRawOutput', () => { expect(formatQueryRawOutput('state begin-phase', { updated: ['x'] })).toBe('true'); expect(formatQueryRawOutput('state begin-phase', { updated: [] })).toBe('false'); }); + + it('never returns undefined for non-JSON top-level values', () => { + expect(formatQueryRawOutput('commit', undefined)).toBe('undefined'); + expect(formatQueryRawOutput('commit', Symbol('x'))).toBe('Symbol(x)'); + }); }); diff --git a/sdk/src/query-raw-output-projection.ts b/sdk/src/query-raw-output-projection.ts index 51f564435..7dcd3d6e1 100644 --- a/sdk/src/query-raw-output-projection.ts +++ b/sdk/src/query-raw-output-projection.ts @@ -1,5 +1,10 @@ import { formatStateLoadRawStdout } from './query/state-project-load.js'; +function safeStringify(value: unknown): string { + const out = JSON.stringify(value, null, 2); + return out ?? String(value); +} + /** * Raw output projection for native query results. * Owns CLI-facing string contracts for raw mode commands. @@ -11,7 +16,7 @@ export function formatQueryRawOutput(registryCommand: string, data: unknown): st if (registryCommand === 'commit') { if (data == null || typeof data !== 'object' || Array.isArray(data)) { - return JSON.stringify(data, null, 2); + return safeStringify(data); } const d = data as Record; if (d.committed === true) { @@ -32,12 +37,12 @@ export function formatQueryRawOutput(registryCommand: string, data: unknown): st } return r || 'nothing'; } - return JSON.stringify(data, null, 2); + return safeStringify(data); } if (registryCommand === 'config-set') { if (data == null || typeof data !== 'object' || Array.isArray(data)) { - return JSON.stringify(data, null, 2); + return safeStringify(data); } const d = data as Record; if ((d.updated === true || d.set === true) && d.key !== undefined) { @@ -50,12 +55,12 @@ export function formatQueryRawOutput(registryCommand: string, data: unknown): st } return `${d.key}=${String(v)}`; } - return JSON.stringify(data, null, 2); + return safeStringify(data); } if (registryCommand === 'state.begin-phase' || registryCommand === 'state begin-phase') { if (data == null || typeof data !== 'object' || Array.isArray(data)) { - return JSON.stringify(data, null, 2); + return safeStringify(data); } const d = data as Record; const u = d.updated as string[] | undefined; @@ -65,5 +70,5 @@ export function formatQueryRawOutput(registryCommand: string, data: unknown): st if (typeof data === 'string') { return data; } - return JSON.stringify(data, null, 2); + return safeStringify(data); } diff --git a/sdk/src/query-subprocess-adapter.test.ts b/sdk/src/query-subprocess-adapter.test.ts index fae7c4a5b..3e45040cd 100644 --- a/sdk/src/query-subprocess-adapter.test.ts +++ b/sdk/src/query-subprocess-adapter.test.ts @@ -64,6 +64,17 @@ describe('QuerySubprocessAdapter', () => { await expect(adapter.execJson('state', ['load'])).resolves.toEqual({ from: 'file' }); }); + it('execJson resolves relative @file output against projectDir', async () => { + const relDir = join(dir, '.planning'); + await mkdir(relDir, { recursive: true }); + const relFile = join(relDir, 'out.json'); + await writeFile(relFile, JSON.stringify({ from: 'relative-file' })); + const script = await createScript('file-relative.cjs', `process.stdout.write('@file:.planning/out.json');`); + const adapter = createAdapter(script); + + await expect(adapter.execJson('state', ['load'])).resolves.toEqual({ from: 'relative-file' }); + }); + it('execRaw returns trimmed stdout', async () => { const script = await createScript('raw.cjs', `process.stdout.write(' hello ');`); const adapter = createAdapter(script); diff --git a/sdk/src/query-subprocess-adapter.ts b/sdk/src/query-subprocess-adapter.ts index af83aac3d..bd2530ce1 100644 --- a/sdk/src/query-subprocess-adapter.ts +++ b/sdk/src/query-subprocess-adapter.ts @@ -1,4 +1,5 @@ import { execFile } from 'node:child_process'; +import { isAbsolute, resolve } from 'node:path'; import { readFile } from 'node:fs/promises'; import { timeoutMessage } from './query-failure-classification.js'; import type { QueryToolsErrorFactory } from './query-tools-error-factory.js'; @@ -131,11 +132,12 @@ export class QuerySubprocessAdapter { let jsonStr = trimmed; if (jsonStr.startsWith('@file:')) { const filePath = jsonStr.slice(6).trim(); + const resolvedPath = isAbsolute(filePath) ? filePath : resolve(this.deps.projectDir, filePath); try { - jsonStr = await readFile(filePath, 'utf-8'); + jsonStr = await readFile(resolvedPath, 'utf-8'); } catch (err) { const reason = err instanceof Error ? err.message : String(err); - throw new Error(`Failed to read gsd-tools @file: indirection at "${filePath}": ${reason}`); + throw new Error(`Failed to read gsd-tools @file: indirection at "${resolvedPath}": ${reason}`); } }