Fix: status could not be serialised when the probe SUCCEEDED
The probe returned `error: undefined` on a dimension match. `undefined` is not a JSON value -- JSON.stringify silently drops it from an object -- so the shape looked harmless and was not: the harness enforces the declared output schema against the canonical value before materialising it, and an explicit undefined property fails that check. A perfectly healthy status call could not be returned at all. Fixed at the boundary rather than at each site. Every return now funnels through `toJsonValue`, which drops undefined properties exactly as JSON.stringify would, maps non-finite numbers to null (same reasoning: JSON.stringify(NaN) is "null"), turns an undefined array slot into null rather than shifting indices, and refuses a circular value instead of recursing forever. `update` was carrying `errorCode: undefined` and `ensureSlopo` a `detail: undefined` on the same path, so a per-site fix would have left two more of these waiting. The probe also now omits `error` rather than assigning undefined, since it is a plausible thing for another consumer to read. Four regression tests, and they were checked against a negative control: with the sanitiser reverted and the undefined restored, exactly the "probe SUCCEEDS" test fails with "does not survive a JSON round trip" and the others still pass, which is what makes it a test of this bug rather than of the JSON round trip in general.
This commit is contained in:
1 parent
534efa8b50
commit
46124b11aa
5 files changed
+249
-9
No files matched your search
+103
-2
@@ -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')
|
||||
})
|
||||
Reference in new issue
Block a user