diff --git a/index.js b/index.js index 843b22f..1c1fcdb 100644 --- a/index.js +++ b/index.js @@ -34,6 +34,7 @@ import Schema from '@deepseek-ai/schemastery' import { missingRequired, normalizeConfig, renderSlopoConfig } from './lib/config.js' import { probeEmbeddingEndpoint } from './lib/probe.js' +import { toJsonValue } from './lib/json.js' import { resolveDshHome, workspaceStateDir, @@ -431,7 +432,18 @@ export function apply(ctx, config) { exec.agent === undefined ? {} : { session: exec.agent.session }, ) - const foreground = (value) => ({ kind: 'foreground', action: args.action, configPath: paths.configPath, logPath: paths.logFile, ...value }) + // Every return funnels through here. The harness enforces the declared + // output schema against this value BEFORE materialising it, and an + // explicit `undefined` property is not lossless JSON — which is exactly + // how a probe result carrying `error: undefined` broke the status call. + const foreground = (value) => + toJsonValue({ + kind: 'foreground', + action: args.action, + configPath: paths.configPath, + logPath: paths.logFile, + ...value, + }) if (args.action === 'status') { const binary = findSlopo(dshHome) @@ -698,7 +710,7 @@ export function apply(ctx, config) { return { cancel: () => controller.abort(), done } }, }) - return { kind: 'background', action: args.action, jobId, logPath: paths.logFile, configPath: paths.configPath } + return toJsonValue({ kind: 'background', action: args.action, jobId, logPath: paths.logFile, configPath: paths.configPath }) } const result = await run(command, { env, signal: exec.signal }) diff --git a/lib/json.js b/lib/json.js new file mode 100644 index 0000000..51ee180 --- /dev/null +++ b/lib/json.js @@ -0,0 +1,65 @@ +/** + * Coerce a value into something that is genuinely lossless JSON. + * + * `undefined` is the trap. It is not a JSON value: `JSON.stringify` silently + * drops it from an object, so a result carrying `{ ok: true, error: undefined }` + * looks harmless on the way out and is not — the harness enforces the declared + * output schema against the canonical value *before* materialising it, and an + * `undefined` property is not valid lossless JSON. The tool call fails with a + * serialisation error the caller cannot act on. + * + * Building the value through one chokepoint is what makes that impossible to + * reintroduce: any future branch may write `foo: undefined` and it is dropped, + * exactly as `JSON.stringify` would have dropped it. + * + * Non-finite numbers become `null` for the same reason (`JSON.stringify(NaN)` + * is `"null"`), and a circular reference throws rather than recursing forever. + * Handles are not JSON values at all, so they are converted to `""`. + */ + +/** + * @param {unknown} value + * @param {Set} [seen] + * @returns {unknown} a value that survives JSON round-tripping unchanged + */ +export function toJsonValue(value, seen = new Set()) { + if (value === null) return null + switch (typeof value) { + case 'string': + case 'boolean': + return value + case 'number': + return Number.isFinite(value) ? value : null + case 'bigint': + return value.toString() + case 'undefined': + case 'function': + case 'symbol': + return undefined + case 'object': + break + default: + return undefined + } + + if (seen.has(value)) throw new TypeError('cannot serialise a circular value') + seen.add(value) + try { + if (Array.isArray(value)) { + // An array slot cannot be dropped the way an object key can, so an + // unserialisable element becomes null rather than shifting the indices. + return value.map((item) => { + const converted = toJsonValue(item, seen) + return converted === undefined ? null : converted + }) + } + const output = {} + for (const [key, entry] of Object.entries(value)) { + const converted = toJsonValue(entry, seen) + if (converted !== undefined) output[key] = converted + } + return output + } finally { + seen.delete(value) + } +} diff --git a/lib/probe.js b/lib/probe.js index fba3990..886a399 100644 --- a/lib/probe.js +++ b/lib/probe.js @@ -143,7 +143,7 @@ export async function probeEmbeddingEndpoint({ } const actual = vector.length const expected = Number(dimensions) - return { + const result = { checked: true, reachable: true, ok: actual === expected, @@ -151,11 +151,14 @@ export async function probeEmbeddingEndpoint({ sentModel, dimensions: actual, expected, - error: - actual === expected - ? undefined - : `the model returns ${actual} dimensions but embeddingDimensions is ${expected}`, } + // Omitted rather than assigned undefined: an explicit `undefined` property + // is not lossless JSON and fails the tool's output validation before the + // result is ever materialised. + if (actual !== expected) { + result.error = `the model returns ${actual} dimensions but embeddingDimensions is ${expected}` + } + return result } catch (error) { const aborted = error?.name === 'AbortError' return { diff --git a/test/json.test.js b/test/json.test.js new file mode 100644 index 0000000..5293066 --- /dev/null +++ b/test/json.test.js @@ -0,0 +1,59 @@ +import assert from 'node:assert/strict' +import { test } from 'node:test' + +import { toJsonValue } from '../lib/json.js' + +test('drops undefined properties, which is the shape that broke status', () => { + const value = toJsonValue({ ok: true, error: undefined, url: 'http://x' }) + assert.deepEqual(value, { ok: true, url: 'http://x' }) + assert.equal(Object.hasOwn(value, 'error'), false) +}) + +test('the result survives a JSON round trip unchanged', () => { + const value = toJsonValue({ + nested: { keep: 1, drop: undefined, deeper: { alsoDrop: undefined } }, + list: [1, 'two', { three: true, four: undefined }], + }) + assert.deepEqual(value, JSON.parse(JSON.stringify(value))) +}) + +test('an undefined array slot becomes null rather than shifting the indices', () => { + assert.deepEqual(toJsonValue([1, undefined, 3]), [1, null, 3]) +}) + +test('non-finite numbers become null, exactly as JSON.stringify would', () => { + assert.deepEqual(toJsonValue({ a: Number.NaN, b: Infinity, c: -Infinity, d: 1.5 }), { + a: null, + b: null, + c: null, + d: 1.5, + }) +}) + +test('functions, symbols and undefined vanish; bigints become strings', () => { + const value = toJsonValue({ fn: () => {}, sym: Symbol('x'), gone: undefined, big: 10n, keep: 'yes' }) + assert.deepEqual(value, { big: '10', keep: 'yes' }) +}) + +test('null and primitives pass through', () => { + assert.equal(toJsonValue(null), null) + assert.equal(toJsonValue('a'), 'a') + assert.equal(toJsonValue(0), 0) + assert.equal(toJsonValue(false), false) +}) + +test('a circular value is refused rather than recursed into', () => { + const circular = { name: 'loop' } + circular.self = circular + assert.throws(() => toJsonValue(circular), /circular/) +}) + +test('a repeated but non-circular reference is fine', () => { + const shared = { id: 1 } + assert.deepEqual(toJsonValue({ a: shared, b: shared }), { a: { id: 1 }, b: { id: 1 } }) +}) + +test('an already-clean value is returned unchanged in value', () => { + const clean = { kind: 'foreground', ok: true, exitCode: 0, report: 'cluster 1:' } + assert.deepEqual(toJsonValue(clean), clean) +}) diff --git a/test/plugin.test.js b/test/plugin.test.js index 6f2bfdc..a9cd763 100644 --- a/test/plugin.test.js +++ b/test/plugin.test.js @@ -76,7 +76,7 @@ const shell = { * `jobs` is deliberately absent, which forces the tool down its foreground path * so the assertions can see a result rather than a job handle. */ -function harness(overrides = {}, { withSettings = true, withJobs = false, sandboxMode, sandboxPolicyResolver } = {}) { +function harness(overrides = {}, { withSettings = true, withJobs = false, withShell = true, sandboxMode, sandboxPolicyResolver } = {}) { const config = { ...FULL_CONFIG, ...overrides } // A per-harness shell, so a test can vary sandboxMode and every resolved // request is recorded for assertion. @@ -101,7 +101,7 @@ function harness(overrides = {}, { withSettings = true, withJobs = false, sandbo const ctx = { logger: () => ({ info() {}, warn() {}, debug() {}, error() {} }), get(name) { - if (name === 'shell') return scopedShell + if (name === 'shell') return withShell ? scopedShell : undefined if (name === 'jobs') return withJobs ? state.jobs : undefined if (name === 'sandboxPolicy') return sandboxPolicyResolver return undefined @@ -432,3 +432,104 @@ test('configure never touches the shell, and status only runs a read-only versio assert.match(request.command, /--version/, 'status may only run a read-only version check') } }) + +/** + * Walk a returned value and fail on anything that is not lossless JSON. + * + * `assert.deepStrictEqual` against a JSON round trip catches an `undefined` + * property (Node distinguishes `{a:1,b:undefined}` from `{a:1}`), but the walk + * names the offending path, which matters when the value is a deep diagnostic. + */ +function assertLossless(label, value) { + assert.deepEqual(value, JSON.parse(JSON.stringify(value)), `${label}: does not survive a JSON round trip`) + const walk = (node, path) => { + if (node === null || typeof node !== 'object') return + if (Array.isArray(node)) { + node.forEach((item, index) => walk(item, `${path}[${index}]`)) + return + } + for (const [key, entry] of Object.entries(node)) { + assert.notEqual(entry, undefined, `${label}: ${path}.${key} is undefined, which is not lossless JSON`) + walk(entry, `${path}.${key}`) + } + } + walk(value, label) +} + +test('status returns lossless JSON when the endpoint probe SUCCEEDS', async () => { + // The regression: the probe used to return `error: undefined` on a match, and + // an explicit undefined property failed the harness's output validation, so a + // perfectly healthy status call could not be serialised at all. + const { createServer } = await import('node:http') + const server = createServer((request, response) => { + response.writeHead(200, { 'content-type': 'application/json' }) + response.end(JSON.stringify({ data: [{ embedding: new Array(768).fill(0.01) }] })) + }) + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)) + const { port } = server.address() + try { + const { state } = harness({ + embeddingModel: 'ollama/example:latest', + embeddingDimensions: 768, + embeddingApiBase: `http://127.0.0.1:${port}`, + }) + const value = await state.tools.get('slopo').execute({ action: 'status' }, execContext('/tmp')) + assertLossless('status', value) + assert.equal(value.ok, true) + assert.equal(value.details.embeddingEndpoint.ok, true) + assert.equal(value.details.embeddingEndpoint.dimensions, 768) + assert.equal(Object.hasOwn(value.details.embeddingEndpoint, 'error'), false) + } finally { + await new Promise((resolve) => server.close(resolve)) + } +}) + +test('status returns lossless JSON when the endpoint probe FAILS', async () => { + const { state } = harness({ + embeddingModel: 'ollama/example:latest', + embeddingDimensions: 768, + // Nothing is listening on this port, so the fetch rejects. + embeddingApiBase: 'http://127.0.0.1:1', + }) + const value = await state.tools.get('slopo').execute({ action: 'status' }, execContext('/tmp')) + assertLossless('status (unreachable)', value) + assert.equal(value.ok, false) + assert.ok(value.details.embeddingEndpoint.error.length > 0) +}) + +test('every action that returns without executing anything is lossless JSON', async () => { + const cases = [ + [{ action: 'status' }, 'status (unconfigured)'], + [{ action: 'configure', config: { embeddingModel: 'x', nope: 1 } }, 'configure (unknown setting)'], + [{ action: 'configure', config: { embeddingModel: 'x' } }, 'configure (ok)'], + [{ action: 'configure', config: 'not-an-object' }, 'configure (bad patch)'], + [{ action: 'review' }, 'review (unconfigured)'], + ] + for (const [args, label] of cases) { + const { state } = harness() + const value = await state.tools.get('slopo').execute(args, execContext('/tmp')) + assertLossless(label, value) + } +}) + +test('an action refused before it runs is lossless JSON', async () => { + const configured = { + embeddingModel: 'ollama/example:latest', + embeddingDimensions: 768, + embeddingApiKey: 'secret', + embeddingApiBase: 'http://127.0.0.1:11434', + } + + const noShell = harness(configured, { withShell: false }) + const refused = await noShell.state.tools.get('slopo').execute({ action: 'review' }, execContext('/tmp')) + assertLossless('review (no shell)', refused) + assert.equal(refused.errorCode, 'NO_SHELL') + + const outside = harness(configured, { + sandboxMode: 'workspace-write', + sandboxPolicyResolver: { resolve: () => ({ mode: 'workspace-write', workspaceRoot: '/some/workspace' }) }, + }) + const value = await outside.state.tools.get('slopo').execute({ action: 'review' }, execContext('/some/workspace')) + assertLossless('review (state outside sandbox)', value) + assert.equal(value.errorCode, 'STATE_OUTSIDE_SANDBOX') +})