fix(skill): fail closed on response errors and invalid ledger filters (1.1.48)

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

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) {

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) {

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)) {

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));

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 <VERSION>",
           "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 <VERSION>",
           "skill_package_id": "xmemo-memory",

test/fixtures/snapshots/flags.json

@@ -5,7 +5,7 @@
       "--version"
     ],
     "code": 0,
-    "stdout": "1.1.47\n",
+    "stdout": "1.1.48\n",
     "stderr": "",
     "httpRequests": []
   },

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' } }));

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 ['', '<html>upstream failure</html>']) {
+      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 ['', '<html>upstream failure</html>']) {
+    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, '<html>bad</html>', 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));
+    }
+  });
+});