From 2d344d7c0a6a503e714b9873a52aa41877274d4d Mon Sep 17 00:00:00 2001 From: pc-gemini Date: Sat, 24 Oct 2026 16:45:48 +0900 Subject: [PATCH] fix(skill): fail closed on invalid responses and ledger filters --- skills/xmemo/CHANGELOG.md | 8 ++ skills/xmemo/scripts/commands/cloud-skill.mjs | 17 ++- skills/xmemo/scripts/lib/cli-input.mjs | 8 +- skills/xmemo/scripts/lib/core.mjs | 3 +- skills/xmemo/scripts/xmemo-skill.mjs | 2 +- test/fixtures/snapshots/auth.json | 6 +- test/fixtures/snapshots/flags.json | 2 +- test/xmemo-skill-cloud-skill.test.js | 6 +- test/xmemo-skill-error-boundaries.test.js | 111 ++++++++++++++++++ 9 files changed, 145 insertions(+), 18 deletions(-) create mode 100644 test/xmemo-skill-error-boundaries.test.js diff --git a/skills/xmemo/CHANGELOG.md b/skills/xmemo/CHANGELOG.md index 973ec1b..e01d616 100644 --- a/skills/xmemo/CHANGELOG.md +++ b/skills/xmemo/CHANGELOG.md @@ -2,6 +2,14 @@ ## [Unreleased] +## 1.1.48 + +### Fixed + +- Return a nonzero server-error exit code when a successful HTTP response cannot be decoded, rather than reporting success for empty or non-JSON responses. +- Fail Cloud Skill detail reads when component retrieval fails or returns an invalid response, preserving authorization errors instead of reporting an empty component list. +- Reject non-finite ledger amount filters and reversed minimum/maximum ranges locally before sending any request. + ## 1.1.47 ### Documentation diff --git a/skills/xmemo/scripts/commands/cloud-skill.mjs b/skills/xmemo/scripts/commands/cloud-skill.mjs index fb9f337..a3d8215 100644 --- a/skills/xmemo/scripts/commands/cloud-skill.mjs +++ b/skills/xmemo/scripts/commands/cloud-skill.mjs @@ -7,7 +7,6 @@ import { makeHttpRequest, handleRestError, outputRestError, - parseJsonResponse, safeJson, sanitizeTerminalText, describeError, @@ -147,14 +146,14 @@ export async function handleCloudSkill(ctx) { options, }); - let components = []; - if (compRes.statusCode >= 200 && compRes.statusCode < 300) { - try { - const compData = parseJsonResponse(compRes, 'Fetch cloud skill components'); - components = Array.isArray(compData) ? compData : (Array.isArray(compData?.components) ? compData.components : []); - } catch { - components = []; - } + const compData = handleRestError(compRes, { + notFoundMessage: `Cloud Skill '${skillId}' components not found.`, + context: 'Fetch cloud skill components', + options, + }); + const components = Array.isArray(compData) ? compData : compData?.components; + if (!Array.isArray(components)) { + outputRestError('invalid_response', 'Cloud Skill components response must contain an array.', options, compData, EXIT_CODE.SERVER_ERROR); } if (options.json) { diff --git a/skills/xmemo/scripts/lib/cli-input.mjs b/skills/xmemo/scripts/lib/cli-input.mjs index 2841b6b..9dbddfb 100644 --- a/skills/xmemo/scripts/lib/cli-input.mjs +++ b/skills/xmemo/scripts/lib/cli-input.mjs @@ -205,10 +205,14 @@ export function validateCommandInput(command, subcommand, positionals, options, throw new Error('--month must be formatted as YYYY-MM.'); } for (const m of ['min-amount', 'max-amount']) { - if (flags[m] !== undefined && (Number.isNaN(Number(flags[m])) || Number(flags[m]) < 0)) { - throw new Error(`--${m} must be a non-negative number.`); + if (flags[m] !== undefined && (!Number.isFinite(Number(flags[m])) || Number(flags[m]) < 0)) { + throw new Error(`--${m} must be a finite non-negative number.`); } } + if (flags['min-amount'] !== undefined && flags['max-amount'] !== undefined + && Number(flags['min-amount']) > Number(flags['max-amount'])) { + throw new Error('--min-amount must not exceed --max-amount.'); + } if (flags.threshold !== undefined) { const threshold = Number(flags.threshold); if (!Number.isFinite(threshold) || threshold < 0 || threshold > 1) { diff --git a/skills/xmemo/scripts/lib/core.mjs b/skills/xmemo/scripts/lib/core.mjs index 0d9dc26..ced34d3 100644 --- a/skills/xmemo/scripts/lib/core.mjs +++ b/skills/xmemo/scripts/lib/core.mjs @@ -97,7 +97,8 @@ export function exitCodeForError(err) { } const status = Number(err.statusCode || err.status || err.httpStatus); if (Number.isInteger(status) && status > 0) { - return exitCodeForHttpStatus(status); + // A thrown error must never become success just because transport returned 2xx. + return exitCodeForHttpStatus(status) || EXIT_CODE.SERVER_ERROR; } const code = String(err.code || '').toUpperCase(); if (['ECONNREFUSED', 'ENOTFOUND', 'EHOSTUNREACH', 'ECONNRESET', 'ETIMEDOUT', 'ESOCKETTIMEDOUT', 'EAI_AGAIN', 'EPIPE'].includes(code)) { diff --git a/skills/xmemo/scripts/xmemo-skill.mjs b/skills/xmemo/scripts/xmemo-skill.mjs index 14ea87a..b43f9a1 100644 --- a/skills/xmemo/scripts/xmemo-skill.mjs +++ b/skills/xmemo/scripts/xmemo-skill.mjs @@ -63,7 +63,7 @@ import { handleVersionCheck } from './lib/update-check.mjs'; import { handleUpgrade } from './commands/upgrade.mjs'; import { handleCloudSkill } from './commands/cloud-skill.mjs'; -const SKILL_VERSION = '1.1.47'; +const SKILL_VERSION = '1.1.48'; async function main() { let { command, subcommand, positionals, options, flags } = parseArgs(process.argv.slice(2)); diff --git a/test/fixtures/snapshots/auth.json b/test/fixtures/snapshots/auth.json index ad69869..1b41bcb 100644 --- a/test/fixtures/snapshots/auth.json +++ b/test/fixtures/snapshots/auth.json @@ -210,7 +210,7 @@ "client_id": "xmemo-skill", "surface": "standalone_skill", "token_type": "skill_token", - "client_version": "1.1.47", + "client_version": "1.1.48", "scopes": [ "memory:read", "memory:write", @@ -287,7 +287,7 @@ "read:temporary_memory", "write:temporary_memory" ], - "client_version": "1.1.47", + "client_version": "1.1.48", "installation_fingerprint": "golden-fingerprint-uuid", "runtime": "node ", "skill_package_id": "xmemo-memory", @@ -331,7 +331,7 @@ "read:temporary_memory", "write:temporary_memory" ], - "client_version": "1.1.47", + "client_version": "1.1.48", "installation_fingerprint": "golden-fingerprint-uuid", "runtime": "node ", "skill_package_id": "xmemo-memory", diff --git a/test/fixtures/snapshots/flags.json b/test/fixtures/snapshots/flags.json index 1f2bfed..d38f26d 100644 --- a/test/fixtures/snapshots/flags.json +++ b/test/fixtures/snapshots/flags.json @@ -5,7 +5,7 @@ "--version" ], "code": 0, - "stdout": "1.1.47\n", + "stdout": "1.1.48\n", "stderr": "", "httpRequests": [] }, diff --git a/test/xmemo-skill-cloud-skill.test.js b/test/xmemo-skill-cloud-skill.test.js index e7af884..591488b 100644 --- a/test/xmemo-skill-cloud-skill.test.js +++ b/test/xmemo-skill-cloud-skill.test.js @@ -234,7 +234,9 @@ test('cloud-skill-list: reports empty list cleanly in terminal mode', async () = test('cloud-skill-show: shows skill detail with --id', async () => { const testServer = createTestServer(); const baseUrl = await testServer.start(); - testServer.setResponse(SAMPLE_SKILL_DETAIL); + testServer.setHandler((req, res) => { + res.end(JSON.stringify(req.url.endsWith('/components') ? [] : SAMPLE_SKILL_DETAIL)); + }); try { const res = await runScript(['cloud-skill-show', '--id', SAMPLE_SKILLS[0].skill_id, '--terminal'], { @@ -590,6 +592,8 @@ test('cloud-skill slug resolution: strict slug match, name fallback, and collisi if (req.url === '/v1/skills') { res.writeHead(200, { 'Content-Type': 'application/json' }); res.end(JSON.stringify(skillsForResolution)); + } else if (req.url?.endsWith('/components')) { + res.end(JSON.stringify([])); } else if (req.url?.startsWith('/v1/skills/id-')) { res.writeHead(200, { 'Content-Type': 'application/json' }); res.end(JSON.stringify({ skill: { skill_id: req.url.split('/')[3], slug: 'resolved' } })); diff --git a/test/xmemo-skill-error-boundaries.test.js b/test/xmemo-skill-error-boundaries.test.js new file mode 100644 index 0000000..e4ec71a --- /dev/null +++ b/test/xmemo-skill-error-boundaries.test.js @@ -0,0 +1,111 @@ +import assert from 'node:assert/strict'; +import http from 'node:http'; +import { spawn } from 'node:child_process'; +import test from 'node:test'; +import { fileURLToPath } from 'node:url'; +import { parseJsonResponse } from '../skills/xmemo/scripts/lib/api.mjs'; +import { exitCodeForError } from '../skills/xmemo/scripts/lib/core.mjs'; + +const script = new URL('../skills/xmemo/scripts/xmemo-skill.mjs', import.meta.url); + +async function withServer(handler, run) { + const requests = []; + const server = http.createServer((req, res) => { + let body = ''; + req.on('data', (chunk) => { body += chunk; }); + req.on('end', () => { + requests.push({ url: req.url, body: body ? JSON.parse(body) : null }); + handler(req, res); + }); + }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + try { + await run(`http://127.0.0.1:${server.address().port}`, requests); + } finally { + server.closeAllConnections(); + await new Promise((resolve) => server.close(resolve)); + } +} + +function runSkill(baseUrl, args) { + return new Promise((resolve, reject) => { + const child = spawn(process.execPath, [fileURLToPath(script), ...args], { + env: { ...process.env, XMEMO_KEY: 'synthetic-boundary-test', XMEMO_BASE_URL: baseUrl, XMEMO_NO_UPDATE_CHECK: '1' }, + }); + let stdout = '', stderr = ''; + child.stdout.on('data', (chunk) => { stdout += chunk; }); + child.stderr.on('data', (chunk) => { stderr += chunk; }); + child.on('error', reject); + child.on('close', (code) => resolve({ code, stdout, stderr })); + }); +} + +test('response parsing errors never exit successfully and preserve HTTP error classes', () => { + for (const [statusCode, expected] of [[200, 3], [201, 3], [204, 3], [400, 1], [401, 2], [403, 2], [500, 3]]) { + for (const body of ['', 'upstream failure']) { + assert.throws(() => parseJsonResponse({ statusCode, body }, 'probe'), (error) => { + assert.equal(exitCodeForError(error), expected); + return true; + }); + } + } +}); + +test('malformed HTTP 200 bodies fail actual memory and Cloud Skill commands', async () => { + for (const body of ['', 'upstream failure']) { + await withServer((_req, res) => res.end(body), async (baseUrl) => { + for (const args of [['recall', '--query', 'probe'], ['read', '--id', 'probe'], ['cloud-skill-list']]) { + const result = await runSkill(baseUrl, [...args, '--json']); + assert.equal(result.code, 3, JSON.stringify(result)); + assert.doesNotMatch(result.stdout, /"ok":true/); + assert.match(result.stderr, /empty response|non-JSON response/); + } + }); + } +}); + +test('Cloud Skill components preserve failures and only accept valid empty arrays', async () => { + const cases = [ + [401, { error: { code: 'unauthorized', message: 'denied' } }, 2], + [403, { error: { code: 'forbidden', message: 'denied' } }, 2], + [404, { detail: 'missing' }, 1], + [500, { error: { code: 'internal_error', message: 'failed' } }, 3], + [200, '', 3], [200, 'bad', 3], [200, {}, 3], + [200, { components: null }, 3], [200, [], 0], [200, { components: [] }, 0], + ]; + for (const [status, data, expected] of cases) { + await withServer((req, res) => { + if (req.url.endsWith('/components')) { + res.writeHead(status); + res.end(typeof data === 'string' ? data : JSON.stringify(data)); + } else res.end(JSON.stringify({ skill: { id: 'probe', name: 'Probe' } })); + }, async (baseUrl) => { + for (const mode of ['--json', '--terminal']) { + const result = await runSkill(baseUrl, ['cloud-skill-show', '--id', 'probe', mode]); + assert.equal(result.code, expected, JSON.stringify({ status, data, result })); + if (expected) assert.doesNotMatch(result.stdout, /"ok":true|Cloud Skill: Probe/); + else if (mode === '--json') assert.deepEqual(JSON.parse(result.stdout).components, []); + } + }); + } +}); + +test('ledger amount validation rejects invalid ranges before network and preserves valid decimals', async () => { + await withServer((_req, res) => res.end(JSON.stringify({ ok: true, result: { transactions: [] } })), async (baseUrl, requests) => { + for (const flag of ['--min-amount', '--max-amount']) { + for (const value of ['Infinity', '1e309', 'NaN', '-1']) { + const result = await runSkill(baseUrl, ['ledger-list', flag, value, '--json']); + assert.equal(result.code, 1, JSON.stringify(result)); + } + } + const reversed = await runSkill(baseUrl, ['ledger-list', '--min-amount', '2', '--max-amount', '1']); + assert.equal(reversed.code, 1); + assert.equal(requests.length, 0); + for (const [min, max] of [['0', '0'], ['0.25', '12.5']]) { + const result = await runSkill(baseUrl, ['ledger-list', '--min-amount', min, '--max-amount', max, '--json']); + assert.equal(result.code, 0, JSON.stringify(result)); + assert.equal(requests.at(-1).body.arguments.min_amount, Number(min)); + assert.equal(requests.at(-1).body.arguments.max_amount, Number(max)); + } + }); +});